Repository navigation
fix(github): keep PR action menu trigger in place - #1736
Conversation
Co-authored-by: Codex <codex@openai.com> Signed-off-by: James Kraus <jamespkraus@gmail.com>
Co-authored-by: Codex <codex@openai.com> Signed-off-by: James Kraus <jamespkraus@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blockers. Targeting [aria-haspopup="menu"] directly is the right fix: the menu trigger is the only such child of .pullActions, and it matches the existing [aria-haspopup="menu"] selectors in SoloWorkspaces.module.css.
Validation at 075293a: CI is green. Locally, the new browser journey passes in Chromium. With only the CSS change reverted, it fails in both Chromium and WebKit at item.click because the menu has already closed. So the test does guard this regression.
Remaining gate is the one in the description: native desktop and human acceptance.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Code review at 075293a: no findings.
- Root cause confirmed: Base UI
MenuTriggerwraps the trigger in pre/postFocusGuardspans only while open, so:last-childstops matching on open and the trigger losesmargin-left: automid-press. The guards areposition: fixed(visuallyHidden), so they are out of flex flow; matching[aria-haspopup="menu"](set unconditionally byMenuTrigger) keeps the layout identical open and closed. No other child of.pullActionscarries that attribute. - Ran
tests/browser/github-actions.spec.mjs: passes in chromium and webkit at head; with the pre-fix selector restored it fails in both (Convert to draftclick times out after the menu closes). Biome clean on the changed files; CI green.
Scores: correctness 10, elegance 9, minimalness 9. The 110-line fixture is large next to a one-line fix, but it follows the existing tests/fixtures/*.tsx pattern and real pointer hit-testing is the only way to cover this. Non-blocking: JSON.parse(JSON.stringify(...)) in the fixture only serves to produce an untyped value for the generic GitHubCli return; a cast would say that more directly.
* origin/main: (39 commits) fix(github): keep PR action menu trigger in place (#1736) fix(goose): preserve default-model focus during pending saves (#1495) test(fixtures): reduce polling waits and repeated signing (#1493) test(inbox): wait for conversation focus before filtering (#1491) fix(desktop): keep the main webview running while it is hidden (#1713) chore(release): version desktop previews from 0.7.0 (#1643) fix(agent-harness): remove the unavailable-tools line from the base prompt (#1715) fix(agents): keep View transcript visible in narrow Pi and goose activity panes (#1717) fix(github-plugin): prefix GitHub plugin tools with github_ (#1726) perf(agents): load Claude and Codex activity without scanning every session (#1674) Improve new DM previews, picker performance, and composer focus (#1664) Polish individual message reactions and emoji settings (#1628) Improve dark-mode separators and composer contrast (#1711) Make decision providers plugins, and move TypeSafe into one (#1353) fix(avatars): use static pack portraits in chat (#1670) fix(channels): read the open thread when returning to a channel (#1639) Add app-wide GIF shortcut and composer previews (#1558) Add PR failure/conflict actions and sidebar unlinking (#1599) Polish queued follow-up previews and controls (#1554) feat(conversation): make emoji and mentions built in (#1673) ... Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
…-stop * origin/main: (132 commits) feat(workflows): add combined filters and recent sorting (block#1699) ci: smoke test the browser perf suite on pull requests (block#1662) feat(acp): share one ACP client and server pool across harnesses (block#1227) Preserve recipient picker rows and scroll position during paging (block#1739) Group matching emoji reactions at nine people (block#1740) Open channel Canvas from the channel menu in a side-pane tab (block#570) fix(github): keep PR action menu trigger in place (block#1736) fix(goose): preserve default-model focus during pending saves (block#1495) test(fixtures): reduce polling waits and repeated signing (block#1493) test(inbox): wait for conversation focus before filtering (block#1491) fix(desktop): keep the main webview running while it is hidden (block#1713) chore(release): version desktop previews from 0.7.0 (block#1643) fix(agent-harness): remove the unavailable-tools line from the base prompt (block#1715) fix(agents): keep View transcript visible in narrow Pi and goose activity panes (block#1717) fix(github-plugin): prefix GitHub plugin tools with github_ (block#1726) perf(agents): load Claude and Codex activity without scanning every session (block#1674) Improve new DM previews, picker performance, and composer focus (block#1664) Polish individual message reactions and emoji settings (block#1628) Improve dark-mode separators and composer contrast (block#1711) Make decision providers plugins, and move TypeSafe into one (block#1353) ... Signed-off-by: Joah Gerstenberg <joah@squareup.com> # Conflicts: # src/bundled/voice-notes/VoiceCapture.tsx
Keep the PR menu usable after a click. Base UI inserts hidden elements that break the button's
:last-childalignment. The button moves before mouse-up, closing the menu. Target the menu trigger directly instead.Adds one browser regression, with none removed. It lets the menu render before mouse-up, selects Convert to draft, and verifies the draft state. Real pointer hit-testing catches what jsdom misses. It fails before the fix and passes in Chromium and WebKit across both themes, narrow and wide layouts, and enlarged text.
Before/after demo uses fictional data.
pr-demo.mp4
Draft pending native desktop and human acceptance.