Add shortcuts for terminal close and chat with same Git state - #56
Conversation
- Extend keybinding contracts/defaults with `terminal.close` and `chat.newSameGitState` - Wire web handlers for `mod+w` to close focused terminal and `mod+shift+n` to open a thread preserving branch/worktree - Update docs and keybinding tests across server, web, and contracts
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request adds two new keybindings: a terminal close command (mod+w) that works when terminal focus is active, and a local chat thread command (mod+shift+n) that creates new threads while preserving the current git branch and worktree state. The changes update keybinding definitions, add context-aware shortcut detection helpers, and modify UI components to handle and display these new shortcuts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add mod+w terminal close and gate chat thread creation shortcuts to preserve or clear Git context in server defaults and web UIUpdate default keybindings to include 📍Where to StartStart with the updated default bindings in keybindings.ts, then review shortcut handling in ChatView.tsx and thread creation logic in Sidebar.tsx. Macroscope summarized 9075086. |
Greptile SummaryAdds two new keybinding commands (
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
subgraph Contracts["packages/contracts"]
Schema["keybindingCommandSchema\n+ terminal.close\n+ chat.newSameGitState"]
end
subgraph Server["apps/server"]
Defaults["DEFAULT_KEYBINDINGS\nmod+w → terminal.close\nmod+shift+n → chat.newSameGitState"]
end
subgraph Web["apps/web"]
Matchers["keybindings.ts\nisTerminalCloseShortcut()\nisChatNewSameGitStateShortcut()"]
subgraph ChatView["ChatView.tsx"]
KDHandler["keydown handler"]
CloseCheck{"isTerminalCloseShortcut?"}
CloseGuard{"terminalOpen?"}
CloseAction["closeTerminal(activeTerminalId)"]
end
subgraph Sidebar["Sidebar.tsx"]
SidebarHandler["keydown handler"]
GitStateCheck{"isChatNewSameGitStateShortcut?"}
NewThread["handleNewThread(projectId,\n{branch, worktreePath})"]
end
end
Schema --> Defaults
Defaults --> Matchers
Matchers --> KDHandler
KDHandler --> CloseCheck
CloseCheck -->|Yes| CloseGuard
CloseGuard -->|Yes| CloseAction
Matchers --> SidebarHandler
SidebarHandler --> GitStateCheck
GitStateCheck -->|Yes| NewThread
Last reviewed commit: fb4afca |
| { key: "mod+j", command: "terminal.toggle" }, | ||
| { key: "mod+d", command: "terminal.split", when: "terminalFocus" }, | ||
| { key: "mod+n", command: "terminal.new", when: "terminalFocus" }, | ||
| { key: "mod+w", command: "terminal.close", when: "terminalFocus" }, |
There was a problem hiding this comment.
mod+w conflicts with browser tab-close
Cmd+W / Ctrl+W is the standard shortcut for closing a browser tab. While the terminalFocus guard limits when this fires, most browsers do not allow JavaScript to intercept and preventDefault() on Ctrl+W/Cmd+W — the tab will still close.
This should work fine in Electron (where the app controls key event routing), but in the browser-served mode the shortcut will effectively be unusable and may cause unexpected tab closures. If the app is meant to run in both contexts, consider choosing a different default binding for browser users (e.g. mod+shift+w).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/contracts/src/keybindings.test.ts (1)
17-23: Consider adding chat.newSameGitState parsing coverage for parity.The new command is now part of the schema; adding a matching parse assertion keeps the contract tests symmetrical.
🧪 Suggested test addition
const parsedClose = keybindingRuleSchema.parse({ key: "mod+w", command: "terminal.close", }); assert.strictEqual(parsedClose.command, "terminal.close"); + + const parsedSameGit = keybindingRuleSchema.parse({ + key: "mod+shift+n", + command: "chat.newSameGitState", + }); + assert.strictEqual(parsedSameGit.command, "chat.newSameGitState");
- Replace `chat.newSameGitState` with `chat.newLocal` across contracts, server/web defaults, docs, and tests - Make `chat.new` preserve branch/worktree state while `chat.newLocal` creates a local thread - Show terminal close shortcut labels in drawer action tooltips and tab close controls
Two tasks in one commit because both edit ChatView.tsx and cannot be separated by pathspec. DIFF AS A DOCK PANEL (pingdotgg#56). Part B of our own frozen spec, deferred since Part A shipped. The app had TWO tab systems on screen -- the dockview grid, and a second tab set inside the single chat panel. Diff moves first because it already resolved its own thread identity from the URL rather than taking it from ChatView's scope; the other three take a dozen callbacks each and will need rewriting. The structural problem worth recording: ChatView is a DESCENDANT of the dock (ChatDock -> ChatPanel -> ChatView), so the components that need to open a panel cannot receive a ref to the thing containing them. Solved with a module-scope handle registered on mount and cleared on unmount, matching the two singletons already in that file. It warns loudly rather than no-opping silently when null. THE MIGRATION IS PROVEN COMPLETE, NOT ASSERTED. A review found Cmd/Ctrl+D still driving the old store -- and because the render case had been correctly removed while the tab strip still knew how to draw a Diff tab, the shortcut produced a VISIBLE TAB THAT RENDERED NOTHING. Third instance of "looks wired, does nothing" in this repo. The fix was not to patch two call sites. `"diff"` is gone from RightPanelKind and RightPanelSurface, so the compiler enumerated every straggler: exactly six, across two files, all genuinely dead. This is the template for Files, Terminal and Browser -- move the surface, delete its kind, and completeness becomes a build error rather than something a human eyeballed. The one place that must still recognise the retired kind is the persistence migration, which casts past the union with a comment explaining that handling data older than the current type is its entire job. Cmd+D also lost its toggle in the first pass -- open-and-focus with no close. The lane flagged it rather than letting me find it. Not accepted: the keybinding is named `diff.toggle`, the handler is named onToggleDiff, and "use the tab x" defeats the point of a keyboard shortcut. Now a genuine toggle, with the CLOSE half specifically mutation-proven, because a toggle that only ever opens is the exact bug being fixed. The layout-persistence risk was checked rather than assumed: `persist()` stamps knownPanelIds from the current catalog on every save, so a close survives reload as "closed on purpose" instead of being re-grafted. Storage v7->8 strips persisted "diff" surfaces, and recomputes `isOpen` from surviving surfaces -- without that, a user whose ONLY surface was diff resumed into an empty right panel they never asked for. Invisible to the first test because its fixture always had a second surface. THREE.JS PLAY (pingdotgg#51). `autoOpenPreview` has been declared in contracts, collected by the settings form, and persisted to disk since it was written -- and nothing ever read it. A user could tick the box, save, and nothing happened. It now opens the preview when a script with a previewUrl starts. It waits for the port scanner to report a listener attributed BY PID to the exact terminal the script ran in, then opens the CONFIGURED url rather than the scanner's reported one -- those differ when a port gets bumped, and the user configured what they configured. Guarded on desktop before anything starts, so in a browser it genuinely does nothing rather than failing gracefully. Its mutation proof is the right shape: deleting the open call while still returning `opened: true` turns the test red with zero calls to the mock -- the exact "looks wired, does nothing" shape this task exists to fix, reproduced deliberately to prove the test catches it. Full typecheck, npm test and fmt:check green. Live browser E2E is an OPEN GATE, tracked for pingdotgg#55 -- the dock is half migrated, so a live pass today would measure a transitional state.
…ndpoints (pingdotgg#56) Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Hub: a taken web port is tried again until it is free
Summary
terminal.closeandchat.newSameGitStatein shared contractsmod+wfor terminal close withterminalFocus,mod+shift+nfor new chat with same Git state)terminal.closecloses the active terminal fromChatViewbranchandworktreePathwhen usingchat.newSameGitStateTesting
packages/contracts/src/keybindings.test.ts: validatesterminal.closeis accepted by schema parsingapps/server/src/keybindings.test.ts: verifies default bindings includeterminal.closeandchat.newSameGitStateapps/web/src/keybindings.test.ts: verifies shortcut matching forterminal.closeandchat.newSameGitState, includingterminalFocusbehaviorSummary by CodeRabbit
Release Notes
New Features
UI/UX Improvements