Skip to content

test(desktop): wait for a painted frame before the native smoke's CDP click - #6007

Merged
liugddx merged 1 commit into
apache:mainfrom
liugddx:fix/5995-workhub-flakes
Oct 9, 2026
Merged

liugddx merged 1 commit into
apache:mainfrom
liugddx:fix/5995-workhub-flakes

Conversation

@liugddx

@liugddx liugddx commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Summary

This fixes failure 2 of #5995: the WorkHub native browser smoke loses the CDP click it sends right after Main closes. It changes only the test script; product code is untouched.

After main.destroy(), the live view is reparented into the floating window and navigated, and the smoke immediately dispatches one CDP mouse press/release. Under Xvfb the page may not have committed a frame (and hit-test data) in its new window yet, and Chromium drops input it cannot route. Seen on main (f10a74d8e, run 37731524494) and on feature/chat-image-delivery (run 37588623379).

The smoke now waits for load plus two requestAnimationFrames before the click, capped at 2 s, and logs which path it took. It still sends exactly one click and keeps the same 3 s observation, so a genuinely lost event still fails.

Scope change. An earlier revision of this PR also replaced the native workbar menu in workhub-layout.spec.ts (failure 1). @ggbdpq had already taken #5995 and opened #6008 for that, so this PR drops the spec change and leaves failure 1 to #6008. The two PRs no longer overlap. The Storybook failure (failure 3) still has no identified root cause.

Refs #5995

Verification

  • node --check and biome lint on the script: clean.
  • On the earlier revision's CI run, which carried this same smoke change, the step passed and logged Background page input readiness: frames, visible. That shows the wait resolves through the frame path rather than the timeout.
  • I can't run the smoke locally: Xvfb isn't available, and on a visible Windows desktop it fails before reaching the changed code. The failure is intermittent, so one green CI run doesn't prove the fix.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code diagnosed the failure (#5995) and wrote the change. I reviewed it.

Checklist

  • Tests cover the change and fail without it. This is a test-harness fix for an intermittent failure, so there is no deterministic failing case.
  • Lint, format, typecheck and the affected suites pass locally. Lint and syntax check pass; the smoke itself runs only in CI, as described above.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

🤖 Generated with Claude Code

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Oct 8, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Full review of d71b0289 (+34/-22, 2 files, test-only): apps/desktop/e2e/workhub-layout.spec.ts and apps/desktop/scripts/workhub-browser-presentation-smoke.mjs.

Spec change (#5995 §1). The one-shot Menu.prototype.popup stub now records the menu and does not call the real popup. The close step emits menu-will-close and then calls options.callback. This matches popupNativeMenu in src/main/native-menu.ts, which deletes the menu from openMenus and resolves the renderer request only from that callback. So aria-expanded drops through the real product path, and closePopup (the crash site) is gone. The new menu.items.length > 0 assertion is a useful guard that the stub caught the real menu.

Smoke change (#5995 §2). Before the single CDP click, the smoke waits for load plus two requestAnimationFrames, capped at 2 s. It still sends only one click, so a genuinely lost input still fails. On this head's CI run the smoke logged Background page input readiness: frames, visible.

P3: real native-popup coverage is lost. The workbar add-panel is the only renderer caller of window:popupMenu/popupNativeMenu. The later "任务操作" menu in this spec is a DOM role=menuitem menu, not Menu.popup. After this change, no e2e opens a real native menu next to the live WorkHub WebContentsView, which was the original intent of this segment. This is a reasonable trade for removing a main-process crash. Still, the comment at line 119 ("An open native Workbar menu must coexist ...") now overstates what is tested, and the lost coverage is worth recording as a follow-up in #5995.

Overlap with #6008. #6008 (ggbdpq) makes the same spec fix: stub Menu.prototype.popup, drive the popup callback from menu-will-close, drop closePopup. It edits the same hunks (lines ~119-157), so whichever lands second will conflict. Only one should land. This PR is a superset: it also fixes §2 and adds the items-captured assertion. If #6007 lands, #6008 can be closed. If #6008 lands first, this PR should be rebased down to the smoke change only.

No protocol files are touched, so there is no epoch impact. The PR is MERGEABLE. CI test passes on d71b0289, and the changed surfaces ran: Desktop e2e 34/34 passed, and the WorkHub native presentation smoke passed.

No P0-P2 findings.

Comment thread apps/desktop/e2e/workhub-layout.spec.ts Outdated
await expect(page.locator('.maka-session-workbar[data-placement="right"]')).toBeVisible();
// A real native menu must coexist with the live sibling WebContentsView.
// DOM tests cannot detect replacing that view with a frozen screenshot.
// An open native Workbar menu must coexist with the live sibling

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: With the popup stubbed, no native menu is open here, and the workbar add-panel is the only popupNativeMenu caller. The "任务操作" menu later in this spec is a DOM menu. So after this change no e2e covers a real native menu next to the live WorkHub WebContentsView. Consider rewording this comment to say what is asserted now (Maka's menu-open state keeps the view visible), and note the lost native coverage in #5995.

… click

After Main closes, the WorkHub native browser smoke reparents the live
view into the floating window, navigates it, and immediately dispatches
one CDP mouse press/release. Under Xvfb the page may not have committed a
frame (and hit-test data) in its new window yet, and Chromium drops input
it cannot route, so the click was intermittently lost.

Wait for load plus two requestAnimationFrame callbacks, capped at 2 s,
before the single click, and log which path was taken. Still exactly one
click and the same 3 s observation, so a genuinely lost event still fails.

Refs apache#5995.

Generated-by: Claude Code
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@liugddx
liugddx force-pushed the fix/5995-workhub-flakes branch from d71b028 to b0ee0fe Compare October 9, 2026 00:41
@liugddx liugddx changed the title test(desktop): stop WorkHub flakes from native menus and early CDP clicks test(desktop): wait for a painted frame before the native smoke's CDP click Oct 9, 2026
@liugddx

liugddx commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

@Astro-Han I've narrowed this to the native-smoke change only (failure 2 of #5995). #6008 has landed the spec fix for failure 1, so the overlap and the spec-comment P3 from your review no longer apply here. The head is now b0ee0fe7c: one file, +12. Would you take another look?

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review: d71b0289 (our last reviewed head) to b0ee0fe7. After #6008 landed, the branch was cut down to the smoke-script fix (single commit b0ee0fe7, +12 lines in apps/desktop/scripts/workhub-browser-presentation-smoke.mjs). That hunk is byte-identical to the one we reviewed on d71b0289: wait for load plus two animation frames (bounded at 2 s), log the readiness, and still dispatch only one click. The workhub-layout.spec.ts changes are gone, so the overlap with #6008 is resolved and our earlier P3 about native-menu coverage no longer applies to this PR.

Findings: None at P0-P3. No protocol files, so there is no epoch impact. The PR is MERGEABLE, and CI test passes on b0ee0fe7.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@liugddx
liugddx merged commit 0af3d5a into apache:main Oct 9, 2026
1 check passed
@liugddx
liugddx deleted the fix/5995-workhub-flakes branch October 9, 2026 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants