feat(desktop): ⌘⇧L opens diff viewer in v2 workspace#3556
Conversation
Rename TOGGLE_EXPAND_SIDEBAR to OPEN_DIFF_VIEWER (same binding). In v2, focus any existing diff pane or open one in a new tab, and flip the workspace sidebar to the Changes tab. V1 keeps its existing expand-sidebar behavior under the new ID.
📝 WalkthroughWalkthroughThis PR replaces the Changes
Sequence DiagramsequenceDiagram
actor User
participant HotkeySystem as Hotkey System
participant Handler as OPEN_DIFF_VIEWER Handler
participant StateManager as Workspace State
participant TabSystem as Tab/Pane System
User->>HotkeySystem: Press OPEN_DIFF_VIEWER (⌘⇧L)
HotkeySystem->>Handler: Invoke handler
Handler->>StateManager: Open right sidebar
Handler->>StateManager: Set activeTab to "changes"
Handler->>TabSystem: Scan existing tabs for diff pane
alt Diff pane exists
TabSystem->>Handler: Return existing diff pane
Handler->>StateManager: Set as active tab/pane
else No diff pane found
Handler->>TabSystem: Create new tab with diff pane
TabSystem->>StateManager: Add to store
end
StateManager->>User: Diff viewer displayed/focused
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts (1)
83-90: Consider deterministic tab ordering for diff-pane lookup.Iterating
state.tabsand returning the first match works, but if multiple diff panes exist across tabs, the one found depends on insertion order ofstate.tabsandObject.values(tab.panes). If you want to prefer the currently active tab's diff pane (so ⌘⇧L in a tab that contains a diff pane always focuses that pane — matching the "V2: ⌘⇧L with diff pane in current tab → focus the pane" test case), check the active tab first.♻️ Suggested refinement
const state = store.getState(); - for (const tab of state.tabs) { + const orderedTabs = state.activeTabId + ? [ + ...state.tabs.filter((t) => t.id === state.activeTabId), + ...state.tabs.filter((t) => t.id !== state.activeTabId), + ] + : state.tabs; + for (const tab of orderedTabs) { for (const pane of Object.values(tab.panes)) { if (pane.kind !== "diff") continue; state.setActiveTab(tab.id); state.setActivePane({ tabId: tab.id, paneId: pane.id }); return; } }🤖 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 83 - 90, The lookup for a diff pane should prefer the currently active tab: first inspect state.tabs to find the tab with id === state.activeTabId (or use state.activeTab) and, if that tab has any pane with pane.kind === "diff", call state.setActiveTab(...) and state.setActivePane(...) for that pane and return; only if the active tab has no diff panes, fall back to iterating the other tabs (state.tabs) and their panes (Object.values(tab.panes)) to find the first diff pane and activate it as before.
🤖 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/screens/main/components/WorkspaceView/RightSidebar/index.tsx`:
- Around line 198-203: The tooltip's HotkeyLabel currently uses
id="OPEN_DIFF_VIEWER" but the hotkey now opens/focuses a diff pane (see
useWorkspaceHotkeys.ts) while the button runs handleExpandToggle/setMode to
expand/collapse; fix by either changing the HotkeyLabel id to a true toggle
hotkey (e.g., USE a new id like "TOGGLE_SIDEBAR" that maps to the
expand/collapse handler) or by updating the label text to reflect the actual
hotkey action (e.g., when collapsed show "Open diff viewer" and when expanded
show "Collapse sidebar"), or remove HotkeyLabel and use plain text if no
matching hotkey exists; update the code around HotkeyLabel
(id="OPEN_DIFF_VIEWER") and ensure consistency with useWorkspaceHotkeys.ts and
the button's handleExpandToggle/setMode behavior.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:
- Around line 83-90: The lookup for a diff pane should prefer the currently
active tab: first inspect state.tabs to find the tab with id ===
state.activeTabId (or use state.activeTab) and, if that tab has any pane with
pane.kind === "diff", call state.setActiveTab(...) and state.setActivePane(...)
for that pane and return; only if the active tab has no diff panes, fall back to
iterating the other tabs (state.tabs) and their panes (Object.values(tab.panes))
to find the first diff pane and activate it as before.
🪄 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: 819a881e-87d6-4a63-945b-3eacd2b95f18
📒 Files selected for processing (4)
apps/desktop/src/renderer/hotkeys/registry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx
| <TooltipContent side="bottom" showArrow={false}> | ||
| <HotkeyLabel | ||
| label={isExpanded ? "Collapse sidebar" : "Expand sidebar"} | ||
| id="TOGGLE_EXPAND_SIDEBAR" | ||
| id="OPEN_DIFF_VIEWER" | ||
| /> | ||
| </TooltipContent> |
There was a problem hiding this comment.
Tooltip text no longer matches the hotkey's behavior.
The expand/collapse button's tooltip displays "Expand sidebar"/"Collapse sidebar" with the OPEN_DIFF_VIEWER hotkey (⌘⇧L). In the v2 workspace, ⌘⇧L no longer simply expands/collapses the sidebar — per the new handler in useWorkspaceHotkeys.ts it opens or focuses a diff pane and switches the sidebar to the Changes tab. Clicking the button still expands/collapses via handleExpandToggle (which calls setMode), but the displayed shortcut doesn't actually perform that action in v2. Consider either:
- Using a different hotkey id that matches expand/collapse semantics, or
- Updating the tooltip label so the shortcut hint is truthful (e.g. "Open diff viewer" when collapsed), or
- Dropping the
HotkeyLabelhere and using plain text if no dedicated hotkey exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx`
around lines 198 - 203, The tooltip's HotkeyLabel currently uses
id="OPEN_DIFF_VIEWER" but the hotkey now opens/focuses a diff pane (see
useWorkspaceHotkeys.ts) while the button runs handleExpandToggle/setMode to
expand/collapse; fix by either changing the HotkeyLabel id to a true toggle
hotkey (e.g., USE a new id like "TOGGLE_SIDEBAR" that maps to the
expand/collapse handler) or by updating the label text to reflect the actual
hotkey action (e.g., when collapsed show "Open diff viewer" and when expanded
show "Collapse sidebar"), or remove HotkeyLabel and use plain text if no
matching hotkey exists; update the code around HotkeyLabel
(id="OPEN_DIFF_VIEWER") and ensure consistency with useWorkspaceHotkeys.ts and
the button's handleExpandToggle/setMode behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR renames the hotkey action
The rename is complete — no remaining references to Key changes:
Confidence Score: 4/5PR is safe to merge; the rename is complete with no stale references, v1 behavior is preserved, and the new v2 handler is logically correct. All four changed files reviewed. Rename is complete (confirmed by grep), types verify Object.values(tab.panes) yields pane objects with valid .id fields, addTab auto-activates the new tab, and DiffPane gracefully handles path empty string. No critical or blocking issues found. useWorkspaceHotkeys.ts is the most complex new code path but is logically correct.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hotkeys/registry.ts | Clean rename of TOGGLE_EXPAND_SIDEBAR → OPEN_DIFF_VIEWER with updated label and a new description field; no issues. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts | New OPEN_DIFF_VIEWER handler correctly opens/focuses diff pane in v2; sidebar guard is intentionally non-blocking (falls through to diff pane logic even without local state). |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx | V1 hotkey re-bound to OPEN_DIFF_VIEWER with identical expand/collapse behavior; straightforward rename, no logic change. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx | HotkeyLabel id updated to OPEN_DIFF_VIEWER; label text stays hardcoded as Expand/Collapse sidebar so the tooltip remains accurate for v1. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
K["⌘⇧L pressed"] --> V{Which workspace?}
V -->|V2| S1["Open sidebar, set activeTab = changes"]
S1 --> SCAN["Scan state.tabs for pane where kind === diff"]
SCAN --> FOUND{Found?}
FOUND -->|Yes| FOCUS["setActiveTab + setActivePane → return"]
FOUND -->|No| ADD["addTab with new diff pane"]
ADD --> AUTO["Store auto-sets activeTabId to new tab"]
V -->|V1| CHK{isSidebarOpen?}
CHK -->|No| OPEN["setSidebarOpen + setSidebarMode = Changes"]
CHK -->|Yes| TOGGLE{"currentMode = Changes?"}
TOGGLE -->|Yes| TABS["setSidebarMode = Tabs"]
TOGGLE -->|No| CHANGES["setSidebarMode = Changes"]
Reviews (1): Last reviewed commit: "feat(desktop): ⌘⇧L opens diff viewer in ..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
upstream PR superset-sh#3556 で追加された OPEN_DIFF_VIEWER と fork 既存の TOGGLE_EXPAND_SIDEBAR が同じ ⌘⇧L に binding され、V1 workspace の page.tsx で両方が useHotkey 登録されて発火順が不定になっていた。 加えて RightSidebar のサイドバー展開ボタンが誤って OPEN_DIFF_VIEWER のラベルを表示していた。 - V1 page.tsx: OPEN_DIFF_VIEWER の useHotkey 登録を削除 (ハンドラは SEARCH_IN_FILES と完全に同一で冗長だった) - RightSidebar: HotkeyLabel id を TOGGLE_EXPAND_SIDEBAR に戻す - V2 は OPEN_DIFF_VIEWER を引き続き使用 (upstream 意図を維持) V1/V2 は別ルートなので useHotkey の enabled で自然に分岐し、 同じキーで異なる動作をしても競合しない。
1 participant
Summary
TOGGLE_EXPAND_SIDEBAR→OPEN_DIFF_VIEWER(same ⌘⇧L / Ctrl+Shift+Alt+L binding) — the v1 action was effectively "open the diff viewer," so unifying the ID.Test plan
Summary by cubic
⌘⇧L (Ctrl+Shift+Alt+L) now opens or focuses the diff viewer in the v2 workspace. v1 behavior is unchanged.
New Features
Refactors
TOGGLE_EXPAND_SIDEBARtoOPEN_DIFF_VIEWERand updated labels, registry, and tooltip.Written for commit 6fea6da. Summary will update on new commits.
Summary by CodeRabbit
Release Notes