Repository navigation
Keep app shortcuts alive next to a retained closed drawer - #1829
Conversation
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request now. This change keeps app shortcuts active after a closed drawer stays mounted for faster future use. I will check security, code quality, performance, architecture, tests, and the user flow. |
| renderProvider( | ||
| <> | ||
| <Handler command={command} name={command} result={true} /> | ||
| <div role="dialog" aria-modal="true" data-state="closed" inert> |
There was a problem hiding this comment.
🚨 slopcop/review — The test combines both new modal exclusions.
This fixture sets both data-state="closed" and inert.
If either branch fails, the other branch keeps the test green.
Could you split this into one closed-state case and one inert-ancestor case?
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change keeps app shortcuts active when the compact sidebar remains in the document after it closes.
It still blocks those shortcuts while the drawer is open.
I found no blocking security, correctness, or performance issue.
I left one non-blocking inline note. The new test combines both modal exclusions, so one exclusion can hide a failure in the other.
The modal check now scans all matching elements after a shortcut match. Chrome measured 0.392 ms with 5,000 synthetic nodes.
Chrome measured 1.899 ms with 20,000 synthetic nodes. This small cost does not block the change.
The architecture scan found no duplicate modal detector. The sidebar and shared drawer use different closed-state accessibility attributes.
A later change can align those attributes and reduce future contract drift.
Validation completed:
-
The command-provider test passed all 18 tests.
-
The retained-sidebar test passed all 13 tests.
-
The
@bb/apptype check passed. -
git diff --checkpassed. -
All active GitHub checks passed.
-
A browser test used a 390 by 844 viewport.
-
The closed drawer stayed mounted with
inert,aria-modal, anddata-state="closed". -
Meta+Shift+Oopened the new-thread composer while that drawer stayed closed. -
The same shortcut stayed blocked after the drawer opened.
The security, quality, performance, and final review agents used GPT-5.6-Sol with high reasoning.
`hasOpenModal` matched any `[aria-modal="true"]` node, but the compact sidebar drawer stays mounted across open/close and is only `inert` while closed. On a window narrower than the 767px compact breakpoint (desktop windows go down to 500px), that retained panel kept `modalOpen` on for the whole session, so every `mainSurface` chord that excludes `modalOpen` — thread.new, panel.toggle, terminal.open, sidebar.toggle — silently declined even though the effective keybindings said otherwise. Treat a candidate as an open modal only when it is neither `inert` nor `data-state="closed"`, so a genuinely open drawer still suppresses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
240ffe3 to
49b9443
Compare
Root cause
AppCommandProvider.hasOpenModal()treated any[aria-modal="true"]node as an open modal. bb's compact sidebar drawer (SidebarMobilePanel) intentionally stays mounted across open/close and only marks its panelinertwhile closed (#1261), so that panel matched the selector permanently.Result: on any window narrower than the 767px compact breakpoint — reachable on desktop, where
MIN_WINDOW_WIDTHis 500 —modalOpenwas stucktruefor the whole session, and everymainSurfacebinding that excludesmodalOpen(thread.new,panel.toggle,terminal.open,sidebar.toggle, …) silently declined, exactly the "observed behavior is narrower than the declared context" symptom reported in #1812.Fix
In the shared app-command path only: a candidate counts as an open modal only when it is neither inside an
[inert]subtree nordata-state="closed". A drawer or dialog that is genuinely open still suppresses commands.Validation
Live in the dev app (
scripts/bb-dev-app, 700px-wide window, thread route), before the fix:thread.new,panel.toggle,terminal.openall no-ops (defaultPrevented === false), the retained node reportingrole=dialog state=closed inert=trueAfter the fix, all of them run with focus in the timeline, on a sidebar button, and in the composer, and with the drawer actually open
thread.newis still suppressed.Unit coverage added in
AppCommandProvider.test.tsx(fails without the fix):thread.newandpanel.toggledispatch from a timeline row, a sidebar entry, and a contenteditable composer while a closed, inert drawer is mounted; a genuinely open drawer still suppresses.turbo run test --filter=@bb/app(commands, promptbox shortcuts, app-keybindings, root-compose handlers: 41 passed),turbo run typecheck lint --filter=@bb/app(0 errors).Note for the reporter
I could not reproduce the composer-only asymmetry described in the issue on a wide window: with the real desktop shell (Electron, CDP-driven), on web, and with a Linux-platform-emulated renderer,
thread.newandpanel.togglealready dispatched from body, sidebar-button, and timeline focus. The defect above is the one path that does kill those exact commands while the effective keybindings still advertise them. If the original report was on a wide window, please reopen with the window size and whether the sidebar was a drawer.Two adjacent gaps found while tracing, left out as separate concerns:
thread.new/panel.newTab/settings.openvia IPC with no context evaluation and anulltarget, unlike every renderer-dispatched chord;WebContentsViewhas focus,resolveDesktopBrowserAppCommandonly resolvesbrowserFocus-scoped bindings, so global chords are dropped there.Closes #1812