Make every command behave the same from every invocation source#612
Open
Juliusolsson05 wants to merge 6 commits into
Open
Make every command behave the same from every invocation source#612Juliusolsson05 wants to merge 6 commits into
Juliusolsson05 wants to merge 6 commits into
Conversation
Six classification agents swept all 99 catalog commands and the surface wiring after a user report that Alt+P does nothing. The rule this establishes: invoking a command that shows a surface makes it visible; invoking it again dismisses it -- from EVERY invocation source, not just from inside the palette. Three independent layers break that today, and no single-layer fix is sufficient. A toggle written in a command body is unreachable if the router refuses the second keypress; a router fix is pointless if the command has no way to ask whether it is already open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, drop dead bridge members
Seven discrete defects the classification sweep turned up. Independent of
the three activation layers the plan describes; landing first because one
of them is a regression.
THE EDITOR GUARDS WERE SWAPPED. `save-editor-file` carried
`globalEditorOpen || focusedCwd` plus a comment describing Cmd+Alt+E
fullscreen, and `toggle-editor-fullscreen` carried `when: globalEditorOpen`.
Each broke in its own direction:
- Cmd+Alt+E with the editor CLOSED was a silent no-op. `when` runs during
admission, before `run`, so the branch that opens the editor straight
into fullscreen was unreachable from all four invocation sources. That
branch is the intended behavior -- the migrated comment says so
outright: "the chord has always meant 'give me a big editor' as ONE
gesture... or routing would silently drop half the feature." Routing
did. This is a regression from the governance PR, which moved the chord
off a hard-coded keybind branch onto the command.
- File -> Save with the editor closed was ADMITTED and did nothing.
`requestSaveActiveEditorFile` dispatches a window event whose only
listener is EditorWorkbench, which does not mount until the editor is
open.
Each guard now sits on the command it describes, with the comment.
OPEN-IF-CLOSED IS NOW IDEMPOTENT. `ui.toggleGlobalEditor` was the only
editor primitive on the command bridge, so four call sites wanting "make
sure the editor is up" each wrote read-then-toggle against `flags
.globalEditorOpen` -- a value snapshotted when the CommandContext memo was
built. A stale `true` CLOSES the editor the user just asked to open.
`openGlobalEditor`/`closeGlobalEditor` already existed on the store; they
are now on the bridge, and all four sites use them.
BURIED ADMISSION AGREES WITH THE LIST. `revive-pane` and `kill-buried-pane`
checked `state.buried.length > 0` across the whole workspace while the
picker filters by `sourceTabId`, so both could be admitted from a tab with
nothing buried and land on an empty list.
DEAD BRIDGE MEMBERS REMOVED. Eight members of `CommandContext.ui` had zero
command call sites: openResumePicker, openAgentStatusPanel,
closeAgentStatusPanel, toggleStatusMode, toggleWorktreeBadges,
toggleUsageHeader, cycleUsageHeaderLevel, setDangerousAgentsEnabled. Their
store subscriptions, object-literal entries and memo deps went with them --
removing the type alone would have left exactly the mechanism-with-no-
consumer shape this whole sweep exists to find. Plus a dead
`toggleCommandPalette` subscription in App.tsx and three unused
commandState imports.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nine commands did nothing when invoked by a keybinding or the native menu.
Two of them ship default chords, so they were dead from the keyboard in the
shipped app: Resume Session (Cmd+Shift+R) and Prompt Template (Alt+P). The
user reported Alt+P; the sweep found the other eight.
The family is every command whose `run` calls `ui.enterXMode()` — the two
buried commands, the three prompt-template ones, the three AI-workspace
ones, and Resume Session.
WHY THEY FAILED. A chord does not open the palette. It sets
`pendingCommandInvocation`, which mounts the palette host INVISIBLY
(`visible={false}`), runs the command, and unmounts it in the same commit.
`ui.enterXMode` was a `useState` setter on that host, so the mode was
written to a component that was already invisible and about to be
destroyed.
A second mechanism would have killed it even if the host had survived: the
mount-time reset effect calls `setMode('commands')`, and passive effects
run AFTER layout effects in the same commit, so 'commands' was queued last
and won. StrictMode double-invokes it for good measure.
`keepPaletteOpen: true` never helped and was never going to. It is read
only by the palette's OWN executeCommand; the chord path goes through
`dispatchCommand`, which has no such branch. Nine commands have been
declaring a flag that does nothing outside the palette.
THE FIX. `paletteMode` becomes uiShell state, and `setPaletteMode` also
sets `commandPaletteOpen: true` — the open is the correctness fix, not a
convenience, because a sub-mode is only meaningful rendered and every
caller wanted the palette up. Coupling them in the action means a future
mode-entry point cannot forget it. Closing resets the mode, so reopening
never resumes an abandoned sub-flow.
Store state has no component lifecycle, so neither mechanism can reach it.
That is the whole argument -- there is no effect-ordering claim to get
wrong. The tempting three-edit alternative (open the palette in `run`,
extend the excluded-id set, skip the reset when an invocation was pending)
was rejected for the opposite reason: omitting any one part leaves the bug,
and its correctness rests entirely on React's effect ordering.
Two consequences worth noting. The mount reset now skips entirely when the
mount was caused by a pending invocation, because that command may have
seeded state synchronously and the passive reset would wipe it. And the
close-after-run rule now asks the LIVE flag — "did this command turn the
palette on?" — instead of consulting a hardcoded id list that held only
`open-command-palette` and could never have covered these nine.
Tests drive the store rather than the component, deliberately: the bug was
not a rendering bug, it was state having a lifecycle it should not have had.
Verified they fail when `setPaletteMode` stops opening the palette.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`useKeybinds` bails out of the entire key handler while any Radix dialog is
mounted. That gate is correct and load-bearing -- without it, Cmd+W closes a
pane behind an open confirmation dialog -- but it also means a surface built
as a dialog makes ITS OWN chord unreachable while it is up.
That is the whole explanation for why some chords round-trip and some do
not, and none of it was in the commands:
Alt+R Reader, Alt+S Spotlight round-trip render inline, no owner marker
Agent Status round-trips plain <aside>
Cmd+Shift+U Usage dead Radix dialog
Remote Panel dead Radix dialog, though its store
action is already a correct toggle
Without this commit, every toggle written in a command body would be
unreachable by keyboard: the second press never reaches the binding router,
so `run` is never called.
THE EXEMPTION IS ONE CASE. A chord crosses the gate only when it maps to
the command that owns the surface currently holding the interaction.
Pressing Cmd+Shift+U while Usage is up is unambiguously "dismiss Usage" --
it cannot be a stray shortcut leaking into a modal, because the modal is the
thing that command controls. Context is restricted to 'global': a surface
owns the screen, so a grid- or dispatch-scoped binding has no business
firing underneath it.
WHY A TABLE rather than asking the command via `getState(ctx)?.value ===
'on'`, which would be derived and could not drift: evaluating `getState`
needs a CommandContext, and building one assembles ~76 workspace actions.
Deferring exactly that cost is why the router forwards an id through
`requestCommandInvocation` instead of dispatching inline (#494). Paying it
on every keystroke that lands while a dialog is open would be a real
regression traded for a keyboard nicety. Drift is bounded by types instead:
the value is `keyof UiShellState`, and the commands read the same flag
through `CommandContext.flags`.
The table deliberately excludes per-target pickers (a second press should
RE-TARGET, not close), the two commands that write something on the way in
(Save Debug Logs, Attach Recording Note), the Settings page (a destination,
not a peek), and the palette itself. Those exclusions are pinned by a test,
so dropping one later is a decision rather than an accident.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ss it Ten surfaces were open-only: invoking the command again did nothing, because the command had no way to ask whether it was already open. The store knew, and every `close*` action already existed -- neither was reachable from a command. Only `closePalette` was on the bridge. Usage, Keyboard Shortcuts, Agent Activity, Close Old Agents, Switch Agents, Prompt Search, Remote Panel, Reorder Tabs, Pin Agents and the New Tab path picker now round-trip, each following the `tiled-tabs` pattern that was already in the tree: read the flag, branch, call the one-directional action. These surfaces have no toggle action of their own -- open and close are separate store actions -- so the command is where the decision has to live. Every flag is THREE edits: the `flags` type, the context object literal, and the memo dependency array. Missing the third is a silent staleness bug -- the flag is read once and never updates -- so all three are done for all ten, and the count is visible in the diff. WHAT IS DELIBERATELY NOT HERE. The nine per-target surfaces store a `SessionId | null`, not a boolean, and collapsing that would lose the case that matters: pressing the chord while View Prompts is open for a DIFFERENT session must re-target, not close. Save Debug Logs and Attach Recording Note each write something on the way in, so a second press legitimately writes a second one. Settings is a destination, not a peek. The palette itself would flicker on a double-tap. Those exclusions are pinned by a test rather than left implicit. `toggle-remote-panel` needed only the badge: its store action was already a correct toggle, but `remotePanelOpen` was the one panel flag missing from `flags`, making it the sole panel command in the app with no Open/Closed state while eleven siblings had one. The new test asserts every command in the surface-ownership table has a `getState`. It failed on first run and caught a real gap -- `usage.open` toggled but reported nothing -- which is the point: the table lets a chord cross the interaction gate, and the command's own state and run are what dismiss once it does. A command listed there without a badge is one whose second press reaches a `run` that may still only open, which is precisely the reported bug wearing a different hat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Started from a user report that ⌥P (Prompt Template…) does nothing. Six classification agents then swept all 99 catalog commands and the surface/store wiring. ⌥P turned out to be one of nine.
The rule this establishes
Three independent layers broke that, and no single-layer fix is sufficient: a toggle written in a command body is unreachable if the router refuses the second keypress, and a router fix is pointless if the command can't ask whether it's already open.
What was broken
Nine commands were silent no-ops from chord and native menu. Two shipped default chords, so they were dead from the keyboard in the released app:
resume-session(⌘⇧R) andprompt-template(⌥P). A chord doesn't open the palette — it mounts it invisibly, runs the command, and unmounts in the same commit, soui.enterXMode()wrote React state to a component about to be destroyed. A second mechanism would have killed it anyway: the mount reset effect is passive, so it runs after the layout effect that dispatched and queues'commands'last.keepPaletteOpen: truenever helped on that path. It's read only by the palette's own execute path — nine commands have been declaring a flag that does nothing outside the palette.⌘⇧R is a regression from #608, which moved the chord off a hard-coded router branch onto the command.
Ten surfaces were open-only. The store knew whether they were open and every
close*action existed; neither was reachable from a command. OnlyclosePalettewas on the bridge.Dialog-based surfaces couldn't be dismissed by their own chord at all.
useKeybindsbails out of the whole handler while any Radix dialog is mounted. That gate is load-bearing — without it ⌘W closes a pane behind a confirmation dialog — but it's why ⌥R Reader round-trips (renders inline) and ⌘⇧U Usage doesn't (dialog). The difference was never in the commands.The editor guards were transposed.
save-editor-filecarriedglobalEditorOpen || focusedCwdplus a comment describing ⌘⌥E fullscreen;toggle-editor-fullscreencarriedwhen: globalEditorOpen, which runs during admission and made its own open-into-fullscreen branch unreachable. Each broke in its own direction: ⌘⌥E with the editor closed was a silent no-op, and File → Save with the editor closed was admitted and did nothing.What changed
1319dbbaopenGlobalEditor, drop 8 deaduimembers34c600c2f342a6fd8ac304b3getStateDecisions worth reviewing
Store-backed mode over a smaller patch. The three-edit alternative touches fewer files and has three parts that must all be present, with a correctness argument resting on React effect ordering. The store version has no ordering argument at all.
A table, not a derived predicate, for surface ownership. Asking the command (
getState(ctx)?.value === 'on') couldn't drift — and needs aCommandContext, which assembles ~76 workspace actions. Deferring that is why the router forwards an id instead of dispatching inline (#494). Drift is bounded bykeyof UiShellStateinstead.Deliberately not toggled, pinned by test: per-target pickers (a second press should re-target, not close),
save-debug-logs/attach-recording-note(each writes something on the way in),open-settings(destination, not peek), the palette itself (double-tap flicker).Verification
tsc -bclean afterrm -rf .tsc-outcheck:keybindingsOK — 39 binding sets, 13 reserved, 5 approved overlapscommandPaletteOpencoupling fails two casesusage.opentoggled with no badge)Known remaining, documented in the plan
RemotePanelstill renders aDialogwhile registered as a side panel — now cosmetic rather than functional. Escape still doesn't dismiss Settings (dead code behind the ownership gate; separate concern since Settings is excluded from toggling by design).🤖 Generated with Claude Code