feat(desktop): wire up v2 workspace hotkeys#3190
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes workspace-scoped keyboard shortcuts into a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Hotkey as HotkeyHandler
participant Router as Router/useMatchRoute
participant Nav as navigateToV2Workspace
participant Store as WorkspaceList
rect rgba(200,200,255,0.5)
User->>Hotkey: Press NEXT_WORKSPACE / PREV_WORKSPACE
Hotkey->>Router: useMatchRoute -> resolve currentWorkspaceId
Hotkey->>Store: read flattenedWorkspaces
Hotkey->>Hotkey: compute next/prev index (wrap)
Hotkey->>Nav: navigateToV2Workspace(nextWorkspaceId)
Nav-->>Router: update route
end
sequenceDiagram
participant User as User
participant Hotkey as useWorkspaceHotkeys
participant Store as WorkspaceStore
participant Pane as PaneManager
rect rgba(200,255,200,0.5)
User->>Hotkey: Press workspace hotkey (e.g., NEW_TERMINAL, SPLIT_RIGHT)
Hotkey->>Store: store.getState() (tabs, activeTabId, panes)
Hotkey->>Pane: addTab / splitPane / removeActiveTab / focusPane / equalizeSplit
Pane-->>Store: update state
Hotkey-->>User: UI updates (new tab/pane/focus)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
saddlepaddle
force-pushed
the
satya/v2-hotkeys
branch
2 times, most recently
from
c345f3a to
a75b368
Compare
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:227">
P1: The new workspace hotkey hook enables `CLOSE_TAB` via direct `removeTab`, which bypasses the unsaved-changes confirmation flow (`onBeforeCloseTab`) and can drop dirty edits.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts:152">
P2: `SPLIT_AUTO` is wired as a fixed right split, so the auto-split shortcut does not match its intended behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
| }, [collections, workspaceId]); | ||
|
|
||
| useV2WorkspaceHotkeys({ store }); |
Contributor
There was a problem hiding this comment.
P1: The new workspace hotkey hook enables CLOSE_TAB via direct removeTab, which bypasses the unsaved-changes confirmation flow (onBeforeCloseTab) and can drop dirty edits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx, line 227:
<comment>The new workspace hotkey hook enables `CLOSE_TAB` via direct `removeTab`, which bypasses the unsaved-changes confirmation flow (`onBeforeCloseTab`) and can drop dirty edits.</comment>
<file context>
@@ -223,10 +224,8 @@ function WorkspaceContent({
});
}, [collections, workspaceId]);
+ useV2WorkspaceHotkeys({ store });
useHotkey("TOGGLE_SIDEBAR", toggleSidebar);
- useHotkey("NEW_GROUP", addTerminalTab);
</file context>
| state.splitPane({ | ||
| tabId: active.tabId, | ||
| paneId: active.pane.id, | ||
| position: "right", |
Contributor
There was a problem hiding this comment.
P2: SPLIT_AUTO is wired as a fixed right split, so the auto-split shortcut does not match its intended behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts, line 152:
<comment>`SPLIT_AUTO` is wired as a fixed right split, so the auto-split shortcut does not match its intended behavior.</comment>
<file context>
@@ -0,0 +1,231 @@
+ state.splitPane({
+ tabId: active.tabId,
+ paneId: active.pane.id,
+ position: "right",
+ newPane: {
+ kind: "terminal",
</file context>
a75b368 to
91011e9
Compare
Greptile SummaryThis PR wires up the full set of v2 workspace hotkeys — tab creation (⌘T/⌘⇧T/⌘⇧B), tab management (⌘W/⌘⇧W, cycling, jump-to-index), pane management (split, equalize, prev/next), and workspace cycling (⌘⌥↑/↓). The majority of handlers are correctly implemented and consistent with the hotkey registry. One functional bug stands out:
Confidence Score: 4/5Safe to merge after fixing SPLIT_AUTO to use dimension-aware direction detection. The vast majority of hotkeys (19 out of ~21) are correctly implemented and consistent with the registry. The SPLIT_AUTO bug makes ⌘E a redundant alias for ⌘D rather than causing data loss or security risk. The PREV_WORKSPACE -1 edge case is unlikely in practice due to the outer guard. Score is 4 rather than 5 because one advertised feature (auto-split direction) demonstrably does not work. useV2WorkspaceHotkeys.ts lines 145-158 (SPLIT_AUTO handler) needs dimension-aware position logic before the feature is complete. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Key press] --> B{Hotkey}
B -->|⌘T| C[addTab terminal]
B -->|⌘⇧T| D[addTab chat]
B -->|⌘⇧B| E[addTab browser]
B -->|⌘W| F[closePane active pane]
B -->|⌘⇧W| G[removeTab activeTabId]
B -->|⌘⌥← / ⌘⌥→| H[setActiveTab prev/next with wrap]
B -->|⌘⌥1-9| I[setActiveTab by index]
B -->|⌘⇧← / ⌘⇧→| J[setActivePane prev/next with wrap]
B -->|⌘E SPLIT_AUTO| K[splitPane - position right - BUG]
B -->|⌘D SPLIT_RIGHT| L[splitPane - position right]
K -.same as.- L
B -->|⌘⇧D SPLIT_DOWN| M[splitPane - position down]
B -->|⌘⇧E / ⌘⇧S| N[splitPane - right - chat or browser]
B -->|⌘⇧0| O[equalizeSplit top-level layout]
B -->|⌘⌥↑ PREV_WORKSPACE| P[navigate prevIndex - no -1 guard]
B -->|⌘⌥↓ NEXT_WORKSPACE| Q[navigate nextIndex - has -1 guard]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+60
to
+67
| useHotkey("PREV_WORKSPACE", () => { | ||
| if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return; | ||
| const index = flattenedWorkspaces.findIndex( | ||
| (w) => w.id === currentWorkspaceId, | ||
| ); | ||
| const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1; | ||
| navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate); | ||
| }); |
There was a problem hiding this comment.
PREV_WORKSPACE doesn't guard index === -1
When currentWorkspaceId is set but the workspace is absent from flattenedWorkspaces (e.g. it has creationStatus and is filtered out), findIndex returns -1. Since -1 <= 0 is true, prevIndex silently wraps to flattenedWorkspaces.length - 1 (the last workspace).
NEXT_WORKSPACE (line 75) explicitly handles this with || index === -1, routing to index 0. The inconsistency means PREV navigates to the last workspace while NEXT navigates to the first on the same edge case — asymmetric and potentially surprising.
Consider a no-op guard for consistency:
Suggested change
| useHotkey("PREV_WORKSPACE", () => { | |
| if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return; | |
| const index = flattenedWorkspaces.findIndex( | |
| (w) => w.id === currentWorkspaceId, | |
| ); | |
| const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1; | |
| navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate); | |
| }); | |
| useHotkey("PREV_WORKSPACE", () => { | |
| if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return; | |
| const index = flattenedWorkspaces.findIndex( | |
| (w) => w.id === currentWorkspaceId, | |
| ); | |
| if (index === -1) return; | |
| const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1; | |
| navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate); | |
| }); |
![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: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts (1)
70-102: Deduplicate tab-cycling handlers to reduce drift risk.Line 70-102 repeats the same index/wrap logic four times (
PREV_TAB,NEXT_TAB,PREV_TAB_ALT,NEXT_TAB_ALT). A shared helper will keep behavior aligned across all variants.Refactor sketch
+const cycleTab = useCallback( + (step: -1 | 1) => { + const state = store.getState(); + if (!state.activeTabId || state.tabs.length === 0) return; + const index = state.tabs.findIndex((t) => t.id === state.activeTabId); + if (index === -1) return; + const next = (index + step + state.tabs.length) % state.tabs.length; + state.setActiveTab(state.tabs[next].id); + }, + [store], +); + -useHotkey("PREV_TAB", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const prevIndex = index <= 0 ? state.tabs.length - 1 : index - 1; - state.setActiveTab(state.tabs[prevIndex].id); -}); +useHotkey("PREV_TAB", () => cycleTab(-1)); -useHotkey("NEXT_TAB", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const nextIndex = - index >= state.tabs.length - 1 || index === -1 ? 0 : index + 1; - state.setActiveTab(state.tabs[nextIndex].id); -}); +useHotkey("NEXT_TAB", () => cycleTab(1)); -useHotkey("PREV_TAB_ALT", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const prevIndex = index <= 0 ? state.tabs.length - 1 : index - 1; - state.setActiveTab(state.tabs[prevIndex].id); -}); +useHotkey("PREV_TAB_ALT", () => cycleTab(-1)); -useHotkey("NEXT_TAB_ALT", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const nextIndex = - index >= state.tabs.length - 1 || index === -1 ? 0 : index + 1; - state.setActiveTab(state.tabs[nextIndex].id); -}); +useHotkey("NEXT_TAB_ALT", () => cycleTab(1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts around lines 70 - 102, The tab-cycling hotkey handlers duplicate the same index/find/wrap logic across useHotkey calls (PREV_TAB, NEXT_TAB, PREV_TAB_ALT, NEXT_TAB_ALT); extract a small helper (e.g., computeWrappedTabId or changeActiveTab(direction)) that calls store.getState(), finds the current index via tabs.findIndex, computes the wrapped prev/next index, and calls state.setActiveTab(...) so each useHotkey simply calls that helper with a direction enum or ±1; update all four handlers to reuse the helper to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts`:
- Around line 60-76: The hotkey handlers in useDashboardSidebarShortcuts (the
PREV_WORKSPACE and NEXT_WORKSPACE callbacks) call findIndex on
flattenedWorkspaces which can return -1; add an explicit check for index === -1
and return early (no-op) before computing prevIndex/nextIndex and calling
navigateToV2Workspace so we don't unexpectedly jump when the currentWorkspaceId
is not present in flattenedWorkspaces; update both handlers (the callbacks using
currentWorkspaceId, flattenedWorkspaces, findIndex, and navigateToV2Workspace)
to perform this guard.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts:
- Around line 145-157: The SPLIT_AUTO hotkey currently always passes position:
"right", so it mirrors SPLIT_RIGHT; change the handler in useV2WorkspaceHotkeys
(the useHotkey("SPLIT_AUTO", ...) callback) to compute the split position the
same way the pane action logic does in page.tsx (look at the auto-split decision
around the pane action handling) and pass that computed position to
state.splitPane rather than the hard-coded "right"; keep the newPane payload and
terminal creation the same and reuse the same decision function/logic (or inline
the same checks) so SPLIT_AUTO truly auto-selects left/right based on active
pane layout.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts:
- Around line 70-102: The tab-cycling hotkey handlers duplicate the same
index/find/wrap logic across useHotkey calls (PREV_TAB, NEXT_TAB, PREV_TAB_ALT,
NEXT_TAB_ALT); extract a small helper (e.g., computeWrappedTabId or
changeActiveTab(direction)) that calls store.getState(), finds the current index
via tabs.findIndex, computes the wrapped prev/next index, and calls
state.setActiveTab(...) so each useHotkey simply calls that helper with a
direction enum or ±1; update all four handlers to reuse the helper to keep
behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1082389-0fde-4fc4-b54e-bf6935c6ddc3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceHotkeys/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Comment on lines
+60
to
+76
| useHotkey("PREV_WORKSPACE", () => { | ||
| if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return; | ||
| const index = flattenedWorkspaces.findIndex( | ||
| (w) => w.id === currentWorkspaceId, | ||
| ); | ||
| const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1; | ||
| navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate); | ||
| }); | ||
|
|
||
| useHotkey("NEXT_WORKSPACE", () => { | ||
| if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return; | ||
| const index = flattenedWorkspaces.findIndex( | ||
| (w) => w.id === currentWorkspaceId, | ||
| ); | ||
| const nextIndex = | ||
| index >= flattenedWorkspaces.length - 1 || index === -1 ? 0 : index + 1; | ||
| navigateToV2Workspace(flattenedWorkspaces[nextIndex].id, navigate); |
Contributor
There was a problem hiding this comment.
Handle missing workspace index explicitly before cycling.
On Line 62 / Line 71, findIndex can return -1 (e.g., current route workspace is filtered out of flattenedWorkspaces). Current logic then navigates to last/first workspace (Line 65 / Line 75), which is an unexpected jump. Prefer a no-op when index === -1.
Suggested patch
useHotkey("PREV_WORKSPACE", () => {
if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return;
const index = flattenedWorkspaces.findIndex(
(w) => w.id === currentWorkspaceId,
);
+ if (index === -1) return;
const prevIndex = index <= 0 ? flattenedWorkspaces.length - 1 : index - 1;
- navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate);
+ void navigateToV2Workspace(flattenedWorkspaces[prevIndex].id, navigate);
});
useHotkey("NEXT_WORKSPACE", () => {
if (!currentWorkspaceId || flattenedWorkspaces.length === 0) return;
const index = flattenedWorkspaces.findIndex(
(w) => w.id === currentWorkspaceId,
);
+ if (index === -1) return;
const nextIndex =
- index >= flattenedWorkspaces.length - 1 || index === -1 ? 0 : index + 1;
- navigateToV2Workspace(flattenedWorkspaces[nextIndex].id, navigate);
+ index >= flattenedWorkspaces.length - 1 ? 0 : index + 1;
+ void navigateToV2Workspace(flattenedWorkspaces[nextIndex].id, navigate);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts`
around lines 60 - 76, The hotkey handlers in useDashboardSidebarShortcuts (the
PREV_WORKSPACE and NEXT_WORKSPACE callbacks) call findIndex on
flattenedWorkspaces which can return -1; add an explicit check for index === -1
and return early (no-op) before computing prevIndex/nextIndex and calling
navigateToV2Workspace so we don't unexpectedly jump when the currentWorkspaceId
is not present in flattenedWorkspaces; update both handlers (the callbacks using
currentWorkspaceId, flattenedWorkspaces, findIndex, and navigateToV2Workspace)
to perform this guard.
Comment on lines
+145
to
+157
| useHotkey("SPLIT_AUTO", () => { | ||
| const state = store.getState(); | ||
| const active = state.getActivePane(); | ||
| if (!active) return; | ||
| state.splitPane({ | ||
| tabId: active.tabId, | ||
| paneId: active.pane.id, | ||
| position: "right", | ||
| newPane: { | ||
| kind: "terminal", | ||
| data: { terminalId: crypto.randomUUID() } as TerminalPaneData, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
SPLIT_AUTO is not auto-splitting right now.
Line 152 hard-codes "right", so SPLIT_AUTO behaves the same as SPLIT_RIGHT. This diverges from the expected auto behavior (and from the existing pane action logic in apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx, Line 198-200).
Suggested patch
useHotkey("SPLIT_AUTO", () => {
const state = store.getState();
const active = state.getActivePane();
if (!active) return;
+ const position =
+ active.pane.parentDirection === "horizontal" ? "down" : "right";
state.splitPane({
tabId: active.tabId,
paneId: active.pane.id,
- position: "right",
+ position,
newPane: {
kind: "terminal",
data: { terminalId: crypto.randomUUID() } as TerminalPaneData,
},
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useHotkey("SPLIT_AUTO", () => { | |
| const state = store.getState(); | |
| const active = state.getActivePane(); | |
| if (!active) return; | |
| state.splitPane({ | |
| tabId: active.tabId, | |
| paneId: active.pane.id, | |
| position: "right", | |
| newPane: { | |
| kind: "terminal", | |
| data: { terminalId: crypto.randomUUID() } as TerminalPaneData, | |
| }, | |
| }); | |
| useHotkey("SPLIT_AUTO", () => { | |
| const state = store.getState(); | |
| const active = state.getActivePane(); | |
| if (!active) return; | |
| const position = | |
| active.pane.parentDirection === "horizontal" ? "down" : "right"; | |
| state.splitPane({ | |
| tabId: active.tabId, | |
| paneId: active.pane.id, | |
| position, | |
| newPane: { | |
| kind: "terminal", | |
| data: { terminalId: crypto.randomUUID() } as TerminalPaneData, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspaceHotkeys/useV2WorkspaceHotkeys.ts
around lines 145 - 157, The SPLIT_AUTO hotkey currently always passes position:
"right", so it mirrors SPLIT_RIGHT; change the handler in useV2WorkspaceHotkeys
(the useHotkey("SPLIT_AUTO", ...) callback) to compute the split position the
same way the pane action logic does in page.tsx (look at the auto-split decision
around the pane action handling) and pass that computed position to
state.splitPane rather than the hard-coded "right"; keep the newPane payload and
terminal creation the same and reuse the same decision function/logic (or inline
the same checks) so SPLIT_AUTO truly auto-selects left/right based on active
pane layout.
Add useV2WorkspaceHotkeys hook with: - Tab management: close tab/pane, prev/next tab, jump to tab 1-9 - Pane management: prev/next pane, split auto/right/down/chat/browser, equalize splits - Workspace navigation: prev/next workspace
saddlepaddle
force-pushed
the
satya/v2-hotkeys
branch
from
91011e9 to
f80eb46
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts (3)
50-62: Hardcoded localhost URL for browser panes.The URL
http://localhost:3000is hardcoded for new browser tabs and split-with-browser operations. If this is intentional for local development preview, consider extracting it to a constant or configuration to make the intent clearer and allow easier updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts around lines 50 - 62, Replace the hardcoded "http://localhost:3000" used when creating browser panes with a single source of truth: introduce a constant or config entry (e.g., BROWSER_DEFAULT_URL) and use it wherever browser panes are created; update the useHotkey("NEW_BROWSER") handler and any split-with-browser code paths to reference that constant (respecting the BrowserPaneData shape) so the default URL is configurable and not baked into useWorkspaceHotkeys or addTab calls.
67-73: Hotkey name doesn't match behavior.
CLOSE_TERMINALcloses any active pane (terminal, chat, or browser), not specifically terminals. If this is intentional reuse of an existing hotkey binding, the behavior is fine. Otherwise, consider renaming toCLOSE_PANEor adding a pane type check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts around lines 67 - 73, The hotkey binding registered via useHotkey("CLOSE_TERMINAL", ...) closes any active pane regardless of type; either rename the hotkey to "CLOSE_PANE" to reflect current behavior or add a pane type check before calling state.closePane (use store.getState().getActivePane() and inspect active.pane.type or similar) so only terminal panes are closed; update the hotkey string and any documentation/usages if renaming, or implement the conditional guard around state.closePane to restrict closure to terminal panes.
82-114: Duplicate logic for tab navigation hotkeys.
PREV_TAB/PREV_TAB_ALTandNEXT_TAB/NEXT_TAB_ALThave identical implementations. Extract the navigation logic into helper functions to reduce duplication.♻️ Proposed refactor
+ const navigateToPrevTab = useCallback(() => { + const state = store.getState(); + if (!state.activeTabId || state.tabs.length === 0) return; + const index = state.tabs.findIndex((t) => t.id === state.activeTabId); + const prevIndex = index <= 0 ? state.tabs.length - 1 : index - 1; + state.setActiveTab(state.tabs[prevIndex].id); + }, [store]); + + const navigateToNextTab = useCallback(() => { + const state = store.getState(); + if (!state.activeTabId || state.tabs.length === 0) return; + const index = state.tabs.findIndex((t) => t.id === state.activeTabId); + const nextIndex = + index >= state.tabs.length - 1 || index === -1 ? 0 : index + 1; + state.setActiveTab(state.tabs[nextIndex].id); + }, [store]); + - useHotkey("PREV_TAB", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const prevIndex = index <= 0 ? state.tabs.length - 1 : index - 1; - state.setActiveTab(state.tabs[prevIndex].id); - }); - - useHotkey("NEXT_TAB", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const nextIndex = - index >= state.tabs.length - 1 || index === -1 ? 0 : index + 1; - state.setActiveTab(state.tabs[nextIndex].id); - }); - - useHotkey("PREV_TAB_ALT", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const prevIndex = index <= 0 ? state.tabs.length - 1 : index - 1; - state.setActiveTab(state.tabs[prevIndex].id); - }); - - useHotkey("NEXT_TAB_ALT", () => { - const state = store.getState(); - if (!state.activeTabId || state.tabs.length === 0) return; - const index = state.tabs.findIndex((t) => t.id === state.activeTabId); - const nextIndex = - index >= state.tabs.length - 1 || index === -1 ? 0 : index + 1; - state.setActiveTab(state.tabs[nextIndex].id); - }); + useHotkey("PREV_TAB", navigateToPrevTab); + useHotkey("NEXT_TAB", navigateToNextTab); + useHotkey("PREV_TAB_ALT", navigateToPrevTab); + useHotkey("NEXT_TAB_ALT", navigateToNextTab);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts around lines 82 - 114, The four hotkey handlers (useHotkey("PREV_TAB"), "PREV_TAB_ALT", "NEXT_TAB", "NEXT_TAB_ALT") duplicate the same tab navigation logic; extract the logic into two small helpers (e.g., getPrevTabId(state) and getNextTabId(state) or navigateToPrevTab(store)/navigateToNextTab(store)) that compute the target tab id (handling index === -1 and wrap-around) and then call state.setActiveTab(id); replace each handler body with a call to the appropriate helper to remove duplication while keeping state.setActiveTab as the single setter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:
- Around line 157-185: The SPLIT_AUTO handler currently duplicates SPLIT_RIGHT;
update the useHotkey("SPLIT_AUTO", ...) callback so it computes the split
direction dynamically instead of hardcoding "right": obtain the active pane via
store.getState().getActivePane(), read its dimensions (e.g., active.pane.width
and active.pane.height or comparable layout metrics), decide direction (e.g.,
choose "right" if width > height else "bottom"), then call state.splitPane with
that computed position; leave useHotkey("SPLIT_RIGHT", ...) as-is or
remove/rename one of the handlers if auto-behavior is not desired. Ensure you
reference the existing functions/objects state.splitPane, store.getState(),
getActivePane, and the "SPLIT_AUTO"/"SPLIT_RIGHT" hotkey identifiers when making
the change.
- Around line 137-155: The PREV_PANE and NEXT_PANE hotkey handlers use
Object.keys(tab.panes) (in useHotkey callbacks) which follows insertion order
instead of visual layout order; change each handler to call the existing
utilities getPreviousPaneId(tab.layout, tab.activePaneId) and
getNextPaneId(tab.layout, tab.activePaneId) (respectively) obtained from the
utils module, check the returned paneId exists, and then call
state.setActivePane({ tabId: tab.id, paneId: returnedId }) — remove the manual
index/wrapping logic and use tab.layout and those util functions to determine
the next/previous pane.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:
- Around line 50-62: Replace the hardcoded "http://localhost:3000" used when
creating browser panes with a single source of truth: introduce a constant or
config entry (e.g., BROWSER_DEFAULT_URL) and use it wherever browser panes are
created; update the useHotkey("NEW_BROWSER") handler and any split-with-browser
code paths to reference that constant (respecting the BrowserPaneData shape) so
the default URL is configurable and not baked into useWorkspaceHotkeys or addTab
calls.
- Around line 67-73: The hotkey binding registered via
useHotkey("CLOSE_TERMINAL", ...) closes any active pane regardless of type;
either rename the hotkey to "CLOSE_PANE" to reflect current behavior or add a
pane type check before calling state.closePane (use
store.getState().getActivePane() and inspect active.pane.type or similar) so
only terminal panes are closed; update the hotkey string and any
documentation/usages if renaming, or implement the conditional guard around
state.closePane to restrict closure to terminal panes.
- Around line 82-114: The four hotkey handlers (useHotkey("PREV_TAB"),
"PREV_TAB_ALT", "NEXT_TAB", "NEXT_TAB_ALT") duplicate the same tab navigation
logic; extract the logic into two small helpers (e.g., getPrevTabId(state) and
getNextTabId(state) or navigateToPrevTab(store)/navigateToNextTab(store)) that
compute the target tab id (handling index === -1 and wrap-around) and then call
state.setActiveTab(id); replace each handler body with a call to the appropriate
helper to remove duplication while keeping state.setActiveTab as the single
setter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 379aa61f-6429-4aba-8b2b-d3c62771e41f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts
Comment on lines
+137
to
+155
| useHotkey("PREV_PANE", () => { | ||
| const state = store.getState(); | ||
| const tab = state.getActiveTab(); | ||
| if (!tab || !tab.activePaneId) return; | ||
| const paneIds = Object.keys(tab.panes); | ||
| const index = paneIds.indexOf(tab.activePaneId); | ||
| const prevIndex = index <= 0 ? paneIds.length - 1 : index - 1; | ||
| state.setActivePane({ tabId: tab.id, paneId: paneIds[prevIndex] }); | ||
| }); | ||
|
|
||
| useHotkey("NEXT_PANE", () => { | ||
| const state = store.getState(); | ||
| const tab = state.getActiveTab(); | ||
| if (!tab || !tab.activePaneId) return; | ||
| const paneIds = Object.keys(tab.panes); | ||
| const index = paneIds.indexOf(tab.activePaneId); | ||
| const nextIndex = index >= paneIds.length - 1 ? 0 : index + 1; | ||
| state.setActivePane({ tabId: tab.id, paneId: paneIds[nextIndex] }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if WorkspaceStore has methods for ordered pane traversal
ast-grep --pattern 'getPanesInOrder($$$)'
ast-grep --pattern 'getOrderedPanes($$$)'
rg -n "pane.*order|ordered.*pane" --type ts -iRepository: superset-sh/superset
Length of output: 2716
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts | sed -n '130,160p'Repository: superset-sh/superset
Length of output: 1398
🏁 Script executed:
# Check the structure of tab.panes and whether utilities are imported
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts | head -50Repository: superset-sh/superset
Length of output: 1667
🏁 Script executed:
# Check how getNextPaneIdInVisualOrder and getPreviousPaneIdInVisualOrder are used elsewhere
rg -n "getNextPaneIdInVisualOrder|getPreviousPaneIdInVisualOrder" --type tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the implementation of these utility functions
cat -n apps/desktop/src/renderer/stores/tabs/utils.ts | sed -n '510,560p'Repository: superset-sh/superset
Length of output: 2052
🏁 Script executed:
# Check the WorkspaceStore type and tab structure
rg -n "type.*Tab|interface.*Tab" apps/desktop/src/renderer/stores/tabs --type ts -A 5 | head -40Repository: superset-sh/superset
Length of output: 3047
🏁 Script executed:
# Check where getNextPaneId and getPreviousPaneId are actually used
rg -n "getNextPaneId|getPreviousPaneId" apps/desktop/src --type tsRepository: superset-sh/superset
Length of output: 787
🏁 Script executed:
# Check the full WorkspaceStore definition
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts | sed -n '1,20p'Repository: superset-sh/superset
Length of output: 764
🏁 Script executed:
# Check if there's a layout field in the tab or state
rg -n "\.layout" apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/useWorkspaceHotkeys --type tsRepository: superset-sh/superset
Length of output: 443
Use visual layout order for pane navigation instead of insertion order.
The code uses Object.keys(tab.panes) which returns insertion order, but tab.layout (a MosaicNode<string>) is available and required for visual order traversal (left-to-right, top-to-bottom). Replace with getNextPaneId() and getPreviousPaneId() from the utils module, which are already used elsewhere in the codebase (workspace/page.tsx). These utilities also handle wrapping correctly with modulo arithmetic, eliminating the need for manual index boundary checks.
Current code
useHotkey("PREV_PANE", () => {
const state = store.getState();
const tab = state.getActiveTab();
if (!tab || !tab.activePaneId) return;
const paneIds = Object.keys(tab.panes);
const index = paneIds.indexOf(tab.activePaneId);
const prevIndex = index <= 0 ? paneIds.length - 1 : index - 1;
state.setActivePane({ tabId: tab.id, paneId: paneIds[prevIndex] });
});
useHotkey("NEXT_PANE", () => {
const state = store.getState();
const tab = state.getActiveTab();
if (!tab || !tab.activePaneId) return;
const paneIds = Object.keys(tab.panes);
const index = paneIds.indexOf(tab.activePaneId);
const nextIndex = index >= paneIds.length - 1 ? 0 : index + 1;
state.setActivePane({ tabId: tab.id, paneId: paneIds[nextIndex] });
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts
around lines 137 - 155, The PREV_PANE and NEXT_PANE hotkey handlers use
Object.keys(tab.panes) (in useHotkey callbacks) which follows insertion order
instead of visual layout order; change each handler to call the existing
utilities getPreviousPaneId(tab.layout, tab.activePaneId) and
getNextPaneId(tab.layout, tab.activePaneId) (respectively) obtained from the
utils module, check the returned paneId exists, and then call
state.setActivePane({ tabId: tab.id, paneId: returnedId }) — remove the manual
index/wrapping logic and use tab.layout and those util functions to determine
the next/previous pane.
Comment on lines
+157
to
+185
| useHotkey("SPLIT_AUTO", () => { | ||
| const state = store.getState(); | ||
| const active = state.getActivePane(); | ||
| if (!active) return; | ||
| state.splitPane({ | ||
| tabId: active.tabId, | ||
| paneId: active.pane.id, | ||
| position: "right", | ||
| newPane: { | ||
| kind: "terminal", | ||
| data: { terminalId: crypto.randomUUID() } as TerminalPaneData, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| useHotkey("SPLIT_RIGHT", () => { | ||
| const state = store.getState(); | ||
| const active = state.getActivePane(); | ||
| if (!active) return; | ||
| state.splitPane({ | ||
| tabId: active.tabId, | ||
| paneId: active.pane.id, | ||
| position: "right", | ||
| newPane: { | ||
| kind: "terminal", | ||
| data: { terminalId: crypto.randomUUID() } as TerminalPaneData, | ||
| }, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
SPLIT_AUTO and SPLIT_RIGHT have identical behavior.
Both hotkeys perform a right split with a terminal. The name "SPLIT_AUTO" suggests it should automatically determine the split direction (e.g., based on pane dimensions), but it's hardcoded to "right".
If auto-direction detection is intended, implement the logic. Otherwise, consider removing the duplication or renaming for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts
around lines 157 - 185, The SPLIT_AUTO handler currently duplicates SPLIT_RIGHT;
update the useHotkey("SPLIT_AUTO", ...) callback so it computes the split
direction dynamically instead of hardcoding "right": obtain the active pane via
store.getState().getActivePane(), read its dimensions (e.g., active.pane.width
and active.pane.height or comparable layout metrics), decide direction (e.g.,
choose "right" if width > height else "bottom"), then call state.splitPane with
that computed position; leave useHotkey("SPLIT_RIGHT", ...) as-is or
remove/rename one of the handlers if auto-behavior is not desired. Ensure you
reference the existing functions/objects state.splitPane, store.getState(),
getActivePane, and the "SPLIT_AUTO"/"SPLIT_RIGHT" hotkey identifiers when making
the change.
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 7, 2026
…3190) Add useV2WorkspaceHotkeys hook with: - Tab management: close tab/pane, prev/next tab, jump to tab 1-9 - Pane management: prev/next pane, split auto/right/down/chat/browser, equalize splits - Workspace navigation: prev/next workspace
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 7, 2026
cherry-pick済み: - e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190) - 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177) - 11ed4f8 V2 terminal env (superset-sh#3184) - 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196) - 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199) - 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198) - d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197) - 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205) - 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194) - 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218) フォーク独自対応: - cleanupMainWindowResources()をexit pathに移動維持 (#3205対応) - BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線 - BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正（リマップ対応） - ansi_up依存維持
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.
Summary
useV2WorkspaceHotkeyshook with tab management (close, prev/next, jump 1-9), pane management (prev/next, split auto/right/down/chat/browser, equalize), and tab creation (new terminal/chat/browser)useDashboardSidebarShortcutswith cycling (wraps around)OPEN_IN_APPalready handled by sharedOpenInMenuButtonin TopBarNot ported (v2 features not built yet):
REOPEN_TAB— needs tab history trackingRUN_WORKSPACE_COMMAND— not built for v2COPY_PATH/OPEN_PR— need host service workspace dataTOGGLE_EXPAND_SIDEBAR— v2 sidebar doesn't have expand/collapse modesOPEN_PRESET_1..9— presets use v1 tab systemTest plan
Summary by cubic
Wired up all v2 workspace hotkeys for full keyboard control of tabs, panes, sidebar, and workspace navigation. Integrated
useWorkspaceHotkeysin the v2 workspace and added wrap-around prev/next workspace switching viauseDashboardSidebarShortcuts.New Features
useWorkspaceHotkeysfor v2 workspaces; adds right sidebar toggle.Refactors
useWorkspaceHotkeysanduseDashboardSidebarShortcuts; removed inline handlers from the v2 workspace page.Written for commit f80eb46. Summary will update on new commits.
Summary by CodeRabbit