Repository navigation
test(desktop): wait for a painted frame before the native smoke's CDP click - #6007
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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>
d71b028 to
b0ee0fe
Compare
|
@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 |
Astro-Han
left a comment
There was a problem hiding this comment.
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.
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 onmain(f10a74d8e, run 37731524494) and onfeature/chat-image-delivery(run 37588623379).The smoke now waits for
loadplus tworequestAnimationFrames 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 --checkandbiome linton the script: clean.Background page input readiness: frames, visible. That shows the wait resolves through the frame path rather than the timeout.AI use
Select exactly one:
Tool(s) and scope: Claude Code diagnosed the failure (#5995) and wrote the change. I reviewed it.
Checklist
Does this PR entail a change in behavior?
🤖 Generated with Claude Code