[feat] Make the playground's keyboard shortcuts discoverable - #6373
Merged
Conversation
…heet The playground binds 41 keyboard shortcuts across six files. Seven of them name a key on screen. Nothing else does, so the rest are unreachable unless you read the source. Add one registry in @agenta/shared that owns every binding's keys and label, a ShortcutKeys component that prints them the way the reader's own keyboard is labelled, and a KeyboardShortcutsSheet on '?' for the eleven bindings that answer no control. Give ApprovalCard a shortcutHints prop so the three ways to show approve and deny can be compared side by side in Storybook. No handler changes. Every key printed here is one the code already binds.
…el and help shortcuts Chrome and Edge open their menu on Alt+F, Firefox opens a menu on Alt+F/E/V/S/B/T/H, and both focus the address bar on Alt+D. Two of the playground's nine Alt letters sat on those keys, so search and the config panel were unreliable on Windows and Linux. Search moves from Alt+F to Alt+K, the config panel from Alt+B to Alt+C, and new session from Alt+C to Alt+N so C reads as Configuration. The files pane gains Alt+O, which it never had. A test now fails if any browser menu letter is ever bound again. Both panel carets name their key in the tooltip. A keyboard button in the session bar opens the shortcuts sheet, and owns the ? hotkey, which is ignored while the caret is in a text field. The approval card keeps its keycaps on the buttons; the other two variants are gone.
Radix's dismissable layer cancels Escape in the capture phase but never stops it propagating, and it does not touch Cmd/Ctrl+Enter at all. Adding a dialog the product tells you to open at any time made that reachable: closing the shortcuts sheet with Escape also denied a parked tool call, and Cmd+Enter approved one the user could not see. Opening the top bar's settings menu and pressing Escape denied a gate the same way. isOverlayOpen now has one home in @agenta/shared instead of two copies, and the approval card, connection dock and recording bar all guard on 'event.defaultPrevented || isOverlayOpen()'. Both halves are load-bearing: Radix cancels the key for a menu the overlay check cannot see, and an antd modal cancels nothing. Two regression tests cover the two halves. Also in this pass: the shortcuts button moves to the right edge of the playground top bar, the sheet widens to three columns so the full list fits a 15 inch screen without scrolling, aria-keyshortcuts on every shortcut-bearing button is computed from the registry rather than hand-written, the / menu and the permission picker become separate groups because they bind different keys, and docs/design/playground-shortcut-discoverability records why each Alt letter is what it is.
Every other story title in web/storybook is namespaced by the package that owns the component, and the file name matches the title's last segment. These two were not: one was titled Playground/Keyboard shortcuts and the other sat outside the Presentational sub-folders. They are now @agenta/ui/Domain/KeyboardShortcuts and @agenta/ui/Presentational/Labels/ShortcutKeys, the domain file is renamed to match, both metas name their component and set layout padded, and both carry the 'Used in' note and the per-story doc comments the neighbouring stories use. Also records in status.md what was verified, including the live pass on the dev stack.
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📘 Docs preview
This comment updates in place on every push. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change centralizes playground shortcuts, adds platform-aware keycaps and a shortcuts sheet, exposes shortcuts on controls, updates session bindings, prevents overlay keystroke leakage, and adds tests, Storybook stories, and design documentation. ChangesPlayground shortcut discoverability
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds discoverable global shortcuts and changes several bindings, but a remaining keyboard-ownership gap can let Cmd/Ctrl+Enter approve a pending action while a menu or unsupported overlay is open. Smaller shortcut-display, empty-session, documentation, and accessibility inconsistencies also remain, so merge should wait for these issues to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant PlaygroundHeader
participant ShortcutsHelpButton
participant KeyboardShortcutsSheet
participant shortcutGroups
participant ShortcutKeys
PlaygroundHeader->>ShortcutsHelpButton: Render help button
ShortcutsHelpButton->>KeyboardShortcutsSheet: Open sheet
KeyboardShortcutsSheet->>shortcutGroups: Read grouped registry entries
shortcutGroups-->>KeyboardShortcutsSheet: Return shortcut metadata
KeyboardShortcutsSheet->>ShortcutKeys: Render platform-specific keycaps
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The listed changes stay within shortcut discoverability, shortcut safety, accessibility, overlay handling, tests, Storybook coverage, and related documentation. The description also identifies excluded areas.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The Placements story rendered a tooltip row and a session tab menu key column that are not in this change. Anyone judging the proposal by eye would have read four shipped placements where there are two, which is the opposite of what a story is for. Both frames are cut and the story is renamed PanelTooltips after what it actually shows. The follow-up placements stay listed in status.md. Adds a Decorative story for the new aria-hidden prop. Inside a button the label already names the action, so the caps are hidden and the key travels as aria-keyshortcuts; in the sheet the caps are the content and stay announced. Neither state is reachable by clicking, which is the bar web/AGENTS.md sets for a story. Completes both metas against the house pattern in CellRenderers.stories.tsx: component plus subcomponents, and the Used in note.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
web/oss/src/components/AgentChatSlice/AgentChatPanel.tsx (1)
247-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the one-line comment rule to all changed source comments.
Split each block into short one-line comments or reduce it to one line.
web/oss/src/components/AgentChatSlice/AgentChatPanel.tsx#L247-L249: shorten the Files pane comment.web/packages/agenta-chat/src/components/ApprovalCard.tsx#L134-L135: shorten the keycap comment.web/packages/agenta-chat/src/components/ApprovalCard.tsx#L160-L162: shorten the overlay comment.web/packages/agenta-chat/src/components/ConnectionDock.tsx#L503-L505: shorten the overlay comment.web/packages/agenta-chat/src/components/RecordingBar.tsx#L57-L59: shorten the overlay comment.web/oss/src/components/AgentChatSlice/hooks/useSessionShortcuts.ts#L48-L61: shorten the shortcut documentation block.As per coding guidelines,
Hard rule. At most ONE short line per comment.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 46f0e2ef-e3a1-4beb-86c5-6d5dfd879609
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
docs/design/playground-shortcut-discoverability/README.mddocs/design/playground-shortcut-discoverability/context.mddocs/design/playground-shortcut-discoverability/decisions.mddocs/design/playground-shortcut-discoverability/plan.mddocs/design/playground-shortcut-discoverability/research.mddocs/design/playground-shortcut-discoverability/status.mdweb/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/components/OpenFilesPaneButton.tsxweb/oss/src/components/AgentChatSlice/components/ShowConfigPanelButton.tsxweb/oss/src/components/AgentChatSlice/hooks/useSessionShortcuts.test.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionShortcuts.tsweb/oss/src/components/Playground/Components/PlaygroundHeader/index.tsxweb/packages/agenta-chat/src/components/ApprovalCard.tsxweb/packages/agenta-chat/src/components/ConnectionDock.tsxweb/packages/agenta-chat/src/components/RecordingBar.tsxweb/packages/agenta-chat/src/hooks/usePushToTalk.tsweb/packages/agenta-chat/tests/unit/ApprovalCard.test.tsxweb/packages/agenta-shared/src/utils/index.tsweb/packages/agenta-shared/src/utils/overlay.tsweb/packages/agenta-shared/src/utils/shortcuts.tsweb/packages/agenta-shared/tests/unit/shortcuts.test.tsweb/packages/agenta-ui/package.jsonweb/packages/agenta-ui/src/shortcuts/KeyboardShortcutsSheet.tsxweb/packages/agenta-ui/src/shortcuts/ShortcutKeys.tsxweb/packages/agenta-ui/src/shortcuts/ShortcutsHelpButton.tsxweb/packages/agenta-ui/src/shortcuts/index.tsweb/storybook/package.jsonweb/storybook/stories/domain/KeyboardShortcuts.stories.tsxweb/storybook/stories/presentational/ShortcutKeys.stories.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 180eef91-2d3f-4e38-955c-13619f2cdd60
📒 Files selected for processing (2)
web/storybook/stories/domain/KeyboardShortcuts.stories.tsxweb/storybook/stories/presentational/ShortcutKeys.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/storybook/stories/presentational/ShortcutKeys.stories.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
mmabrouk
marked this pull request as draft
August 30, 2026 09:24
mmabrouk
marked this pull request as ready for review
August 30, 2026 09:24
Each side panel has two controls, one to open it and one to close it, and only the open one named its key. Both panels start open, so the control you see by default was the one with no hint. Hovering the configuration panel's collapse caret showed 'Hide configuration' and nothing else, and the files pane's caret did the same. Both collapse controls now name their key, so the pair matches whichever way the panel is sitting. The session tab's right-click menu gains a right-aligned key column on Rename, Archive and Close. The + button names Alt+N, the session search box names Alt+K in its placeholder, and the stop button carries Esc as aria-keyshortcuts. The key column decorates only the menu this component renders. The shared menuItems is left alone, because the sessions list page renders the same rows and none of these keys work there. Restores the session tab menu to the Placements story. It was cut when it was only a proposal; it ships now.
Three visual defects, all mine. The menu key column was not right-aligned. The wrapper span did not grow inside the menu item's own flex row, so the keycap sat against the label instead of at the edge. It looked right in Storybook only because that mock built its own row rather than mirroring the real ContextMenuItem. The mock now copies the real item's classes, so the two cannot drift apart again. The sheet's first column started lower than the others. Every group heading carries a default top margin, which is kept at the top of column one and dropped at each column break. The heading now sets mt-0. The sheet hid two whole groups. A multi-column box with a fixed height overflows sideways into extra columns instead of growing downward, so on a laptop-height window 'Renaming a session' and 'Help' sat in a fourth column past the right edge, reachable only by scrolling sideways. The height cap and the scrolling move to a wrapper, leaving the column box at auto height. My earlier check measured only the vertical axis, which is why I missed it. Measured after the fix at 760, 1024, 1440 and 1512 wide: the columns share one top, nothing sits off screen, and nothing scrolls sideways.
The mobile app renders the same ApprovalCard with touch set, and the keycaps are gated on that flag. Nobody can reach the state by clicking on a desktop, so a regression there would ship a keycap to every mobile user before anyone saw it. The story pins it: zero keycaps on touch, three on desktop.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b8b8d35d-0e04-45b6-ab78-ca8b8ae7fbfd
📒 Files selected for processing (1)
web/storybook/stories/domain/KeyboardShortcuts.stories.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
mmabrouk
commented
Aug 30, 2026
mmabrouk
left a comment
mmabrouk
left a comment
Member
Author
There was a problem hiding this comment.
Review summary
- Risky:
web/oss/src/components/AgentChatSlice/AgentConversation.tsx:450- pressing Escape to close a Radix menu can also stop the active agent run. - Missing tests: the run-level Escape handler has no regression test for an event cancelled during capture and allowed to propagate.
I recommend fixing this before merge. I reviewed head 2a4d6dad2be0ec4f230da3028a34e996d8881e21. The focused shortcut tests passed locally: shared 13, approval card 11, and session shortcuts 22. Shared, chat, and UI type checks passed. Storybook lint passed. A full local Storybook build exceeded the available 4 GB heap.
Review found that the overlay guard watched only role=dialog. Radix gives a dropdown, a context menu, a popover and a select the SAME dismissable layer, so all of them let a keystroke keep propagating. With the top bar's settings menu open, Cmd+Enter still reached the approval card behind it and approved a gate nobody could see. Escape was covered only because Radix cancels that one key. isOverlayOpen now matches any open dialog, alertdialog, menu or listbox. A role with no data-state still does not match, which is what keeps the Alt shortcuts working inside the antd revision drawer. Also from the review: - The run-level Escape checked only the overlay, so a menu closing also stopped the agent. It now checks defaultPrevented too. - Alt+O toggled a per-session pane even with no active session, where the panel renders none. It returns early instead. - ShortcutKeys borrowed the registry's mirror chord when a caller passed an explicit chord, printing a key that chord never named. - The ? hotkey opened a second layer on top of an open one. Found while testing the above, not in the review. - The counts disagreed across the workspace and the story. Every mention now states both numbers: forty-three already shipped, forty-five in the registry. - The story promised aria-keyshortcuts on two mock buttons that did not carry it, and one doc comment ran past the one-line rule. Eight new tests pin the widened guard, one pins the no-session case, and one pins Cmd+Enter under a menu, which defaultPrevented alone cannot catch.
…reuse it The plan is for /m to become the whole app, not just the phone surface. Four of the five shortcut pieces already sat in packages /m depends on. The hook did not: it lived in web/oss, even though it imports only React and @agenta/shared/utils and takes every action as a callback. Nothing about it was desktop-specific. It moves to @agenta/ui/shortcuts beside the keycaps, the sheet and the help button, so a mobile host wires the same callbacks rather than reimplementing anything. A phone never sends an Alt chord, so mounting it on a touch surface is inert. The old test covered two things at once, the hook and an app-layer rename consumer that needs OSS atoms. It splits: the hook half moves to the package (20 tests), the rename half stays in web/oss as useInlineRenameRequest.test.ts. Storybook gains an On mobile story showing the approval card at phone width and beside its desktop twin, so a keycap leaking onto touch is caught by looking. Also fixes a story that threw at render. shortcutAria was used without its import, which neither tsc nor eslint catches here: SWC transpiles stories without type checking, so it built clean and failed only in the browser. My regression sweep missed it too, because a thrown story renders Storybook's error boundary as ordinary text. The sweep now reads that text. 60 renders, ten pages across two themes and three widths, all clean.
Five findings, all real. One keystroke answered gates you could not see. Every visited session stays mounted behind display:none, so two parallel runs both parking a gate meant one Cmd+Enter approved the hidden one too. A new isOnScreen helper gates the card's window listener, and a test mounts two cards with one hidden. The overlay guard saw only Radix. antd sets no data-state and leaves its popups mounted after they close, so a dropdown, select or popover on screen still let Cmd+Enter through to a parked gate. The guard now matches those by class and filters by visibility, using checkVisibility where the browser has it and an ancestor walk where it does not. A tooltip stays excluded: it is passive. Alt+N was the macOS tilde dead key. I moved New session onto it yesterday, which would have stopped every Mac user typing 'n'-with-tilde in the composer. It is now the + key, matching the button in the tab strip, which no browser and no layout claims. decisions.md now lists all five macOS dead keys so this cannot happen again. aria-keyshortcuts told two lies. The help button claimed a standalone ? key, which is Shift+/ on a US layout and Shift+ss on a German one, and shortcutAria returned an empty string that React rendered as an empty attribute. It now returns undefined, and refuses any key it cannot express honestly. decisions.md claimed the Alt letters were safe. They are safe in ENGLISH Firefox. German Firefox reserves Alt+A, Alt+C and Alt+X for Ansicht, Chronik and Extras, which collide with archive, configuration and next session. The document now says so plainly, and status.md carries the open question of whether Alt plus a letter can work at all across locales. Suites: agenta-shared 459, agenta-chat 574, agenta-ui 168, oss AgentChatSlice 268. Verified live that an antd dropdown silences Alt+C, a closed one does not, and nothing on a resting page matches the guard.
Codex showed the safe-letter list only holds for English Firefox. status.md now carries the choice for a human: keep Alt plus a letter and accept the collision for non-English Firefox users, or move to a three-key chord the way Linear does and pay in ergonomics. The AZERTY legend mismatch is recorded beside it.
Member
Author
|
ashrafchowdury
approved these changes
Aug 31, 2026
Contributor
|
Awesome, thank you @mmabrouk |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The agent playground binds 43 keyboard shortcuts across six files. Six of them name a key on screen. The rest were unreachable unless you read the source, and eleven of them answer no control at all, so no amount of tooltips could ever surface them.
Alt+1…9and theAlt+Z/Alt+Xpair switch sessions, and there is no button anywhere to hang a hint on.Two of the nine Alt letters also sat on keys the browser claims.
Alt+Fopens the menu in Chrome and Edge and the File menu in Firefox, andAlt+Bopens Firefox's Bookmarks menu. Search and the configuration panel were unreliable on Windows and Linux.Changes
One registry owns every binding.
web/packages/agenta-shared/src/utils/shortcuts.tsholds all 45 entries with their keys, labels and conditions. Tooltips, keycaps, the sheet and everyaria-keyshortcutsvalue read from it, so a label cannot drift away from the handler.Keys appear on the control that already does the job. The approval card's Approve and Deny buttons carry keycaps. Both side panel carets name their key in the tooltip: the
»that shows the configuration and the«that shows the files pane.A shortcuts sheet covers the rest. It opens on
?and from a keyboard button at the right edge of the playground top bar. The hotkey matches the produced character, so it works where?is Shift+ß or Shift+comma, and it is ignored while the caret is in any text field.Three Alt letters moved, and one shortcut is new.
Alt+FAlt+KAlt+Fopens the browser menu.Kis the search key across the industry.Alt+BAlt+CAlt+Bopens Firefox's Bookmarks menu.Creads as Configuration.Alt+CAlt+NNreads as New, and it freesCfor the panel above.Alt+OA unit test now fails if any browser menu letter is ever bound again.
docs/design/playground-shortcut-discoverability/decisions.mdrecords why each letter is what it is, so the next person does not undo it.A bug this PR would otherwise have shipped. Radix's dismissable layer cancels Escape in the capture phase but never stops it propagating, and it does not touch
Cmd/Ctrl+Enterat all. Adding a dialog the product tells you to open at any time made that reachable:?, then Escape to close the sheet. The gate was denied, silently.Cmd/Ctrl+Enterwith the sheet open. The gate was approved, unseen.isOverlayOpen()now has one home in@agenta/sharedinstead of two copies, and the approval card, connection dock and recording bar all guard onevent.defaultPrevented || isOverlayOpen(). Both halves earn their place: Radix cancels the key for a menu the overlay check cannot see, and an antd modal cancels nothing.The last case was already broken before this branch. The dropdown and the approval card have coexisted for a while.
Tests
ApprovalCard.test.tsx(11): two new cases cover the overlay leak. I removed each half of the guard in turn and confirmed that its test, and only its test, failed.shortcuts.test.ts(13, new): the registry has unique ids, every entry sits in a titled group, and everyaria-keyshortcutsvalue uses only ARIA modifier names.useSessionShortcuts.test.ts(22): includes the new "binds no letter a browser menu already claims".@agenta/shared440,@agenta/chat568,web/ossAgentChatSlice 287. Type-checks and package lints clean.?opens the sheet from the page and stays shut in the composer, the sheet is three columns with no scrollbar on a 15 inch screen, andAlt+Ccollapses and restores the configuration panel.Scope and risk
Not touched: the composer's own Enter behaviour, the elicitation dock's key handling, the push-to-talk chord, and every
Cmd/Ctrl+Enterbinding. The registry documents them; none of their code changed.Worth a reviewer's eye:
isOverlayOpen()moved from the app layer into@agenta/shared. Six callers now import it. The workflow revision drawer is antd'sDrawer, which rendersrole="dialog"with nodata-state, so the guard deliberately does not fire there. That is why the Alt shortcuts still work inside that drawer, and it must stay that way.AgentChatPanel.tsxmoves theuseSessionFilesPanecall aboveuseSessionShortcuts, because the new files-pane toggle needs it. Hook order changed; nothing is conditional.aria-hiddenand the buttons carryaria-keyshortcuts, so the accessible names stay "Approve" and "Deny". Two existing tests that matched the button text exactly now match by prefix.Still open, and not verifiable from a Linux box: on Linux a window manager can claim an Alt chord before the browser sees it. Pressing the ten Alt chords once on a Linux desktop and once on Windows would close that out.
What to QA
?.?with the page focused. The sheet opens, three columns, no scrollbar on a normal laptop screen.?; the sheet does not open.Alt+C. The configuration panel collapses. Press it again; it comes back. Hover the»caret: the tooltip namesAlt+C.Alt+O. The files pane opens and closes. Hover the«caret: the tooltip namesAlt+O.Alt+K. The session search box takes focus. PressAlt+Nfor a new session, thenAlt+ZandAlt+Xto step between sessions.?and then Escape. The sheet must close and the gate must stay open. Then open the settings menu in the top bar and press Escape. Same: the menu closes, the gate stays.Cmd/Ctrl+Enterapproves, Escape denies.https://claude.ai/code/session_01TqiMMWEa63QRi6XqEXwwUi
Closes #6391