fix(desktop): poll the smoke window size until the maximize lands - #97182
Conversation
The main process maximizes the window on ready-to-show, which can fire after domcontentloaded. The test read the size once, so a late maximize showed the 600px outer frame on Linux, where the title bar leaves 573px of content. macOS never hit it because the hidden title bar keeps the content at 600px. Poll for the final size instead, bounded so a missing window manager still fails clearly. Claude-Session: https://claude.ai/code/session_01AAfrGhk4pdw6Y4X5pEnb9f
|
😎 Merged successfully - details. |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
🤖 CI report✅ Trunk lane — non-backend lane (
|
HostHog preview —
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The new test logic uses two separate 10s polls (width then height), which can extend failures to ~20s and conflicts with the stated “up to ten seconds” bound.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the PostHog Desktop Electron smoke test to avoid a race between domcontentloaded (when the Playwright window fixture is ready) and the main-process maximize that occurs later on ready-to-show, which has been causing flaky minimum-dimensions assertions on Linux.
Changes:
- Replaces a single read of
window.innerWidth/innerHeightwith polling, so the assertion observes the final post-maximize size. - Adds a short explanatory comment clarifying why polling is necessary for this test.
File summaries
| File | Description |
|---|---|
| products/desktop/apps/code/tests/e2e/tests/smoke.spec.ts | Switches the minimum window-dimension assertion to poll for the final size instead of reading once. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Test-only change fixing a flaky e2e assertion by polling instead of reading window size once; no production code, CI config, or risky territory touched, and the diff matches the description.
- 1 of the 1 changed files are governed by
products/desktop/AGENT_APPROVALS.md.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 20L/1F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (20L, 1F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 2738c9b · reviewed head 2738c9b |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2738c9bf9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(bounds.width).toBeGreaterThanOrEqual(900); | ||
| expect(bounds.height).toBeGreaterThanOrEqual(600); | ||
| await expect | ||
| .poll(async () => (await bounds()).width, { timeout: 10000 }) |
There was a problem hiding this comment.
With a fresh E2E profile, windowStateStore defaults isMaximized to true (apps/code/src/main/utils/store.ts:83-92), and showWindow() calls maximize() (apps/code/src/main/window.ts:298-307). On the 1920×1080 CI display, these polls can therefore pass on the maximized dimensions even if BrowserWindow's minWidth or minHeight is lowered or removed, so the test no longer catches the minimum-dimension regression its name promises. Exercise an unmaximized resize at the minimum size instead of treating the maximized display size as proof of the constraint.
AGENTS.md reference: AGENTS.md:L234-L234
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, and it was already true before this change: the window opens at 1200x600, so the old single read passed with or without the constraint. This PR only stabilizes the read. A follow-up can shrink the window through the main process and assert it stops at 800x600, which is what the test name promises.
Problem
electron-testjob about one run in five onSmoke Tests › window has correct minimum dimensions, with a height of 573 instead of 600 (run, run).domcontentloaded. The main process maximizes the window onready-to-show, which can fire later.Changes
The deeper fix is
useContentSize: trueon the main window, sominHeightapplies to the content on every platform. That changes window sizing for users and belongs to the desktop team, so this PR stays in the test.How did you test this code?
minWidthorminHeightdrop on the main window fails it.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Fable 5.1). Skills invoked: /writing-tests, /writing-code-comments, /writing-pr-descriptions. Found while babysitting #96681, whose desktop CI hit this. Same shape as #97145: a fixed read racing a window event.
https://claude.ai/code/session_01AAfrGhk4pdw6Y4X5pEnb9f