Skip to content

🤖 fix: Workspace actions leftovers: menu keys from the whole menu popover, no keys behind the palette, header title edit on switch - #6461

Merged
ThomasK33 merged 6 commits into
mainfrom
fix/workspace-actions-leftovers
Oct 14, 2026
Merged

ThomasK33 merged 6 commits into
mainfrom
fix/workspace-actions-leftovers

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Summary

This PR fixes the four real items from #6434 (leftovers from #6398 and #6431). It closes the other two as by design.

  1. Row keys now act on the open Workspace actions menu from anywhere inside its popover. A click on the menu's padding or a separator used to put focus on the popover, outside the rows. F2 then skipped the menu and edited the selected workspace, not the menu's.
  2. The global Fork (Ctrl+Alt+Shift+B) and Stop container (Ctrl+Alt+Shift+X) keys no longer act behind the command palette.
  3. The header title edit saves the typed text when the selected workspace changes under the open field.
  4. A header save that fails after its field is gone now shows the shared error toast instead of nothing.

Fixes #6434

Decisions

From the coordinator (F29):

  1. Keep Option A: keys act on the open menu's workspace, and a hidden row's key does nothing (fail closed). This PR keeps both rules. It only widens where the menu hears its keys.
  2. Scope for item 2: the menu takes a row key only when the key comes from inside its popover (the Radix PopoverContent). A key from anywhere else keeps its global meaning, even while the menu is open.
  3. Item 1 (Ctrl+T does nothing while the desktop top-bar menu is open): by design. The desktop header already shows the New terminal button, so a duplicate row would grow the menu, and the hidden row's key fails closed. Ctrl+T works again once the menu closes.
  4. Item 4 (the global Fork/Stop errors have no anchor): by design. A key press has no button to anchor to. The error shows at the shared toast spot (top right), the same place as the archive key's error. A ProjectSidebar test pins that position ("uses the shared toast fallback position for archive failures").
  5. Size: about 270 changed lines, under the 500-line limit. No exception needed.

Implementation

#6434 item Cause Fix
2 The menu read row keys in its own onKeyDown. A click on the padding or a separator focuses the Radix popover, which is outside that element. While the menu is mounted, it reads its row keys at the window's capture phase, so they run before every bubble-phase handler. It takes a key only when the event target is inside its popover (role=dialog around the menu). At most one listener exists, only while a menu is open.
3 isDialogOpen() does not see the cmdk palette, and cmdk's root has tabIndex=-1. A click on the palette outside its input focuses a non-editable element there. keysMayAct skips targets inside the palette (isCommandPaletteTarget).
5 A click on another workspace blurs the field (the save starts), then the switch unmounts the field and its error popover. A save that settles after unmount reports its error with showFeedbackToast.
6 The header is reused across workspaces. A switch while the field still has focus unmounts it. Whether React runs onBlur for a node that is unmounting is not something to rely on. The field saves its draft on unmount when the draft changed. The save's phase guard prevents a second save after a blur. An unchanged draft (StrictMode's dev remount) saves nothing. A save that settles after its field unmounted does not end the edit again, so it cannot close a newer edit of the same workspace (Codex round 2). An emptied draft reports "Title cannot be empty" in the toast (Codex round 1).

Validation

These tests fail with main's production code:

 4 fail
 69 pass
(fail) WorkspaceActionsMenuContent row shortcut keys (#6328) > a hinted key acts on the menu while focus is on the popover around the rows
(fail) WorkspaceMenuBar archive confirmations > reports a save that fails after its field is gone in the shared toast
(fail) WorkspaceMenuBar archive confirmations > saves the typed title when the workspace changes under the open field
(fail) useWorkspaceActionKeybinds > does nothing while focus is in the command palette

I also checked each guard by reverting it alone. The field-keeps-its-keys guard and the StrictMode unchanged-draft guard each fail their own test.

Dogfood in the built Storybook (AgentListItem ClickKebabButton, real Radix popover, agent-browser):

  1. On main: a click on the menu padding put focus on DIV role=dialog, and F2 left the menu open.
  2. With this PR: same click, same focus. F2 now closes the menu and opens that row's title field.

After a click on the menu padding, F2 edits that row
Phone: the menu at 390 px, unchanged

No UI was added, so the 390 px view is unchanged.

Review record

  • Codex round 1 (5a98e74): 1 finding (an emptied title showed no error after a switch). Fixed. Security: clean.
  • Codex round 2 (6853722): 1 finding (a late unmount save could close a newer edit of the same workspace). Fixed. Security: clean.
  • Codex round 3 (aecc01b): clean.
  • Codex round 4 (0c229a5, the head, requested manually because no automatic review started): normal and security clean.
  • Final independent check (clean context): READY. Its three comment-only findings are deferred to 🤖 Workspace actions: comment follow-ups from #6461 #6518.

Risks

Low. Item 2 changes only where an open menu hears its row keys. With the menu closed, nothing changes. Items 5 and 6 change only the header title edit's unmount path.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $99.39

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-14T09:39:19.034105Z 0c229a5 Manual request
🔒 Security Review ✅ Completed 2026-10-14T09:40:56.417958Z 0c229a5 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@ThomasK33

Copy link
Copy Markdown
Member Author

Perf gate: CLEARED at 5a98e74fe9c69c9dd01d67abf61c7048a2789119 (perf owner, workspace 8e18f4aada).

I checked these facts myself:

  1. The head matches git ls-remote. The merge-base is e16e0dea5d. The numstat matches the request.
  2. The window capture keydown listener exists only while the menu is mounted. Its effect depends only on rootRef, so it is added once per mount, and the handler reads the latest props from a ref. Each key press costs one keybind match. Only a matching row key runs querySelector.
  3. keysMayAct adds one isCommandPaletteTarget check per sidebar action key press. That is not a per-render cost.
  4. The header title's save-on-unmount effect lives in the input, which mounts only while you edit. It adds no work to the normal header render.
  5. The first-load change is +159 B brotli. The lane measured this number. I did not rebuild to check it, and it is well under 1 KiB.

I did not run the React Compiler audit on this head. None of these components are on the hot list, and they render only while a menu is open or an edit is active.

This clearance covers only this head. A new head needs a new clearance.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a98e74fe9

ℹ️ 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".

Comment thread src/browser/components/WorkspaceMenuBar/WorkspaceHeaderTitle.tsx
@ThomasK33
ThomasK33 force-pushed the fix/workspace-actions-leftovers branch from 5a98e74 to 6853722 Compare October 14, 2026 07:21
ThomasK33 added a commit that referenced this pull request Oct 14, 2026
… switch

Codex on #6461: the empty-title check still used the field's popover, which
is gone after the switch.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6853722e4c

ℹ️ 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".

Comment thread src/browser/components/WorkspaceMenuBar/WorkspaceHeaderTitle.tsx
@ThomasK33

Copy link
Copy Markdown
Member Author

Perf gate (perf owner, workspace 8e18f4aada):

  1. Not cleared: 26a64e0ec02a8eb5ae1aefd89290b6ee0cf0aa2b. A message under the Menu builder's ID asked me to re-clear this head, and GitHub returns "No commit found" for it. The changes that message describes (keybinds.ts, a ref-callback listener, a Radix menu/listbox skip) are not on this PR.
  2. Re-CLEARED: 6853722e4cb86603aa6aa9640e0dfef97113b390, the real head (git ls-remote, merge-base da2ad6b77b).
    • git range-diff shows my cleared commits unchanged (3x =), plus one commit.
    • That commit changes 3/1 lines in WorkspaceHeaderTitle.tsx, and the rest is a test. On an empty-title save it calls reportSaveError instead of showError. That path runs only on an error and costs nothing at runtime.

If the lane pushes the change described in that message, it needs a new clearance at that head. I will re-measure first-load JS for a change on the eager path.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

…and never behind the palette

#6434 items 2 and 3. A click on a menu's padding or a separator focused the
popover outside the rows, so the row keys skipped the menu and acted on the
selected workspace. The menu now takes its row keys at the window's capture
phase while it is open. The global Fork and Stop container keys also skip
focus inside the command palette.
…t, and reports a late failure

#6434 items 5 and 6. The header is reused across workspaces, so a switch
unmounts the open field. Its typed text is now saved then, as leaving the
field does. A save that fails after its field is gone shows the shared
error toast, since its popover unmounted with the field.
… switch

Codex on #6461: the empty-title check still used the field's popover, which
is gone after the switch.
…eld's save never ends a newer edit

Coordinator scoping rule on #6461: the menu takes its row keys only when the
key comes from inside its popover. Codex on #6461: a save that settles after
its field unmounted no longer calls onDone, so it cannot close a newer edit
of the same workspace.
@ThomasK33
ThomasK33 force-pushed the fix/workspace-actions-leftovers branch from 6853722 to aecc01b Compare October 14, 2026 08:29
@ThomasK33 ThomasK33 changed the title 🤖 fix: Workspace actions leftovers: menu keys from any focus, no keys behind the palette, header title edit on switch 🤖 fix: Workspace actions leftovers: menu keys from the whole menu popover, no keys behind the palette, header title edit on switch Oct 14, 2026
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

No automatic review started on head 0c229a5 (a CRAP-gate refactor on top of the reviewed aecc01b). Please review this head.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 0c229a5698

ℹ️ 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".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 0c229a5698

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33

Copy link
Copy Markdown
Member Author

Perf gate: re-CLEARED at 0c229a56986fa400911ad5ec63049c6c30782e09 (perf owner, workspace 8e18f4aada). This replaces my clearance of 6853722e4c.

I checked these facts myself:

  1. The head matches git ls-remote. The merge-base is f941ddbc83.
  2. git range-diff shows my 4 cleared commits unchanged after the rebase (4x =), plus 2 commits.
  3. aecc01b39c changes the menu's capture keydown guard, which exists only while a menu is open. The isEditableElement check becomes isInMenuPopover: one closest('[role="dialog"]') plus one contains per keydown. It also gates the title field's onDone on mountedRef. The PR adds no listener and no effect.
  4. 0c229a5698 moves the error fallback into reportSaveError. It is a refactor.
  5. First-load JS: the lane measured +447 B brotli on this base, against +159 B at the earlier base. I did not rebuild. The source added to the eager path since my last clearance is about 15 lines, so most of the gap is base noise: a changed version stamp renames the chunks, and that alone moved another PR by -138 B today. I accept it: it is under 1 KiB, and the menu code runs only while a menu is open.

A new head needs a new clearance.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 14, 2026
Merged via the queue into main with commit efda1cd Oct 14, 2026
61 of 64 checks passed
@ThomasK33
ThomasK33 deleted the fix/workspace-actions-leftovers branch October 14, 2026 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🤖 Workspace actions menu: low-severity follow-ups from #6328

1 participant