fix(desktop): resolve window presentation mode in startup context - #5296
Conversation
Generated-by: Codex
84931a1 to
bfd607e
Compare
jackwener
left a comment
There was a problem hiding this comment.
Independent review — startup initialisation, dependency cycles, boot-order coverage, and overlap with #5302
Reviewed SHA: bfd607e7a39ff358f3c84c239b6004dec582fa1a, verified as the live head immediately before publishing. Base ea990cab7, 5 files, +149/−27. The PR is a draft.
No P0–P2.
The crash, and why hoisting is the right shape
The original failure is a temporal dead zone: runtime-host-boot.ts declared const revealMode near line 494, while the storage-root repair dialog — raised while that same module body is still awaiting earlier startup steps — closes over it. Reading a const before its declaration executes throws, and that is the reported ReferenceError.
This PR does not move the declaration earlier within the same module; it removes it and hoists the single definition into startup-context.ts, which both main.ts and the boot module import. That is a stronger fix than relocation, because the duplicate computation is what allowed the two copies to drift in the first place — startup-presentation.ts had its own startupRevealMode() with a different expression than boot's.
Dependency cycles — the risk this fix could have introduced
A module-level const that other modules import is exactly the construct that produces a TDZ when a cycle exists, so the fix could have reproduced the bug it removes. It does not: startup-context.ts imports only electron and ./window-reveal.js, and window-reveal.ts has no imports at all. Nothing can re-enter startup-context while its body is still running, and only main.ts and startup-presentation.ts import it. Under ESM the imported module's body completes before the importer's begins, so revealMode is initialised before any reader runs.
The one semantic difference, and why it is safe
Boot previously computed Boolean(e2eFixture) || isIsolatedE2e; the hoisted version computes isIsolatedE2e || Boolean(process.env.MAKA_E2E_FIXTURE). Those are not obviously the same expression, so I checked rather than assumed.
resolveE2eFixture returns null when the variable is unset, and throws when the build is packaged or the scenario is unknown; resolveDesktopE2eFixture catches and, because the variable is set in exactly those cases, exits the process. So at the point boot used the value, Boolean(e2eFixture) and Boolean(process.env.MAKA_E2E_FIXTURE) agree — the disagreeing cases have already terminated.
The hoist also computes the value earlier than the fixture validation, which would matter if a packaged build with a stray MAKA_E2E_FIXTURE could reach a window before exiting. It cannot change the outcome: resolveWindowRevealMode short-circuits on isPackaged first and returns 'active' regardless of the automation flag. The packaged path is unreachable for the automated branch by construction.
Boot-order coverage
This is the claim I most wanted to check, because a startup-ordering fix is easy to "cover" with a test that constructs an order the product never takes.
The new test does not do that. It compiles the real runtime-host-boot.ts and startup-context.ts sources with esbuild, wraps boot in an async function, and executes them in a VM context with './startup-context.js' resolved to the real compiled context module. It then drives an actual stale-marker repair to the dialog and asserts dialogs[0].revealMode === 'active' — the exact site that threw.
It is also load-bearing rather than vacuous. Replacing the exported revealMode with undefined turns both cases red; restoring it returns 2/2. A regression that leaves the value unresolved at the dialog is caught.
Overlap with #5302 — worth a decision before either lands
#5302 (0171bdc9, not a draft, one file) fixes the same crash by relocating the same const from line ~494 to ~359, just ahead of the desktopDiagnostics closure that raises the dialog. Comparing them:
- They conflict. Both edit the same deletion site in
runtime-host-boot.ts; whichever merges first leaves the other needing a rebase. - This PR subsumes #5302. It deletes the boot-local constant outright, so the relocation has nothing left to relocate.
- #5302 keeps the original expression, so it carries no equivalence question at all — the difference analysed above simply does not arise there.
- #5302 leaves the duplication in place:
startup-presentation.tswould still compute its own answer with a different expression. That divergence is the underlying defect, and only this PR removes it.
My reading is that this PR is the more complete fix and #5302 is the smaller, faster one; they should not both land. Which to take is a maintainer decision, not a review finding, and I am not making it — but landing them independently would produce a conflict and leave dead code, so it is worth settling explicitly.
Verification basis and limits
Built @maka/core, @maka/storage, @maka/runtime, @maka/runtime-host, @maka/ui and the desktop main process at this SHA, and confirmed the emitted artifact carried the change before running. The new suite passes 2/2, plus the ablation above.
I did not run the full desktop suite — unrelated test files in that project fail to typecheck here because @maka/mcp is not built in my tree, which is an environment gap on my side and not a property of this change. I did not exercise a real Electron launch, so the window-visibility behaviour the author reports from an isolated Electron run remains his evidence rather than mine, as do his 43 targeted tests and the old-order ablation. My conclusions are independent of CI status, which had not completed for this head.
Automated review, agent-operated. Posted from the shared jackwener GitHub account; the reviewing agent is @kabi-opus (human owner: 卡比卡比 / @WAWQAQ), acting at the PR author's request. This is an AI-assisted review and does not replace independent human review. No merge is performed and none is authorised by this review.
likun666661
left a comment
There was a problem hiding this comment.
No blocking findings. Computing revealMode once in the existing startup-context removes the early repair-dialog TDZ and consolidates duplicate calculations without introducing a new abstraction. Its inputs are available before asynchronous boot, and the reveal policy is preserved. Independently verified 43 focused tests using an isolated source snapshot with existing local workspace dependencies. Restoring the parent boot source makes both new regression cases fail with the reported ReferenceError. Also checked 64 startup-context input combinations. Full typecheck and a real Electron launch were not rerun for this review. Review submitted by an AI assistant at the explicit request of the account owner.
Summary
Refs #5194. When a workspace's stored device identity differs from its current directory, Desktop must ask whether to repair it before opening settings or starting the Runtime Host. The boot dialog introduced in #5194 reads
revealModebefore the boot module reaches its declaration, so the recovery path crashes withCannot access 'revealMode' before initializationinstead of displaying the question.Resolve the process-wide reveal mode once in the existing
startup-contextmodule. Main startup diagnostics, startup/handoff presentation, and Runtime Host boot now consume that value. This removes the boot-local calculation and the separatestartupRevealMode()wrapper, so window presentation no longer depends on how far asynchronous Host boot has progressed. The existing reveal policy remains responsible for active/inactive/hidden behavior. Storage identity checks and explicit repair consent are unchanged; there is no data migration or compatibility path.Verification
npm run format,npm run lint, andgit diff --checkpass.ea990cab7ffa768dc1a574b539f73a83f8faabb8, rebuilt workspace dependencies, and reran Desktopbuild:mainsuccessfully. All 43 focused tests pass after the rebase. The previously reported base TS7006 is no longer present. Full renderer/preload typecheck was not rerun.AI use
Tool(s) and scope: Codex — traced the regression, consolidated startup reveal policy, added and mutation-tested the regression coverage, and verified the isolated Electron dialog.
Checklist
Does this PR entail a change in behavior?