fix(desktop): resync renderer after Windows maximize - #3472
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for isolating the workaround behind a small Windows-only scheduler. The lifecycle guards and coalescing look sound, and I did not find a production correctness issue in the implementation. The remaining gap is that the test proves calls against a mock, while the bug and the proposed fix both live in Electron's real Windows layout behavior; I left that boundary inline.
AI-assisted review disclosure: OpenAI Codex performed an independent exact-head review. I verified the implementation, lifecycle guards, test scope, and live PR state, and I made the final review decision.
| assert.deepEqual(fixture.calls, []); | ||
|
|
||
| fixture.deferred.shift()?.(); | ||
| assert.deepEqual(fixture.calls, ['layout', 'invalidate']); |
There was a problem hiding this comment.
[P2] This unit test proves only that the two mocked methods are called; it cannot show that Electron 43 actually expands and repaints the renderer after the native Windows maximize transition, which is the entire user-visible contract of this PR. Could we add a Windows packaged/CI smoke that restores → maximizes → waits one frame, then checks renderer innerWidth/innerHeight (or WebContents size) against the maximized client bounds and restores again? If we keep only one additional test, the real Windows transition is more valuable than another mock branch.
ea8bb87 to
bd88d4b
Compare
|
Heads up on the failing It fails at That's a regression from #3001, which merged into main earlier today: The We'll approve once #3484 is resolved and the Windows job can go green on this head. AI-assisted review. |
|
Received. I'll handle it
𐂂 𐂂
***@***.***
|
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the diagnosis here is the part that matters, and it is right: maximize is where this reproduces, but the actual gap is that Electron 43's root View layout/repaint does not follow the native maximize. Naming the trigger and the cause separately is what keeps this from becoming a CSS patch.
Reviewed at exact head 83d3d12174620640ea8475cde7673534c8c3142a against base f83b469b557a30036d11572d3c14d69bc0766eaa. No P0–P3.
What we checked, since 495 lines for a window-resize fix invites the question:
- The production change is 48 lines and sits inside the existing main-window lifecycle. Re-calling
setContentView(window.contentView)is not a workaround —NativeWindowViews::SetContentViewdetaches and re-attaches the content view and then runsFlushPendingRootLayout, which is exactly the layout flush that did not happen. Reusing the samecontentViewmeans no new size state is introduced;pendingonly coalesces events. There is no second size authority here. - The remaining ~270 lines are the verifier, and they buy something real: the previous coverage asserted that a mock was called. This reads the native client area through PowerShell
GetClientRect/IsZoomedand the renderer viewport through CDP, then walks normal → maximized → normal checking DPR andhtml/body/#root/.appFramealignment against the native client. For a bug that only exists at the native/renderer boundary on Windows, that is the only evidence that means anything from a macOS checkout. - Guards cover the failure paths: window destroyed, WebContents destroyed, restore, non-Windows. We looked for a stale-
pendingpath that could fire against a destroyed window and did not find one.
Verified locally: helper tests 4/4, node --test scripts/verify-packaged-app.test.mjs scripts/verify-windows-harness.test.mjs 51/51, strict TS compile clean, git diff --check clean.
Not approving yet, and it is not about the code. The red test and package runs on this head are stale merge refs — they were created at 11:23Z against a base that predates the midTurn fix that landed on main at 11:49Z, and the failure in the log is packages/cli/src/pi-tui-runner.ts missing midTurn, which has nothing to do with this PR. A rerun will not help; pull_request runs test the merge commit computed at event time, so only a new push recomputes it. Please rebase, and we will take the result on the new head.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
|
Heads up — this has drifted into conflict with Worth knowing before you rebase: #3397 landed on 2026-08-22 and added ASF license headers across ~2685 files, so a rebase will touch more than you'd expect, and any file you add now needs a header ( I'd like to review this — just ping me once it's rebased and CI is green. |
Summary
Fixes #3416
Verification
npm --workspace @maka/desktop run typecheckapps/desktop/dist/main/__tests__/shell-env.test.jsthree times in isolation: 12/12 passed on every run.Validation limit
The current host is macOS, so the packaged Windows 11 maximize transition still needs Windows CI or a Windows machine for final visual confirmation.