Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds keyboard shortcuts to toggle the left sidebar (Cmd/Ctrl+B) and right workspace panel (Cmd/Ctrl+J), wiring new shortcut events from main global registration through IPC to renderer handlers, introducing a sidebar Pinia store, updating shortcut settings/i18n, and adding tests for main and renderer behaviors. Changes
Sequence DiagramsequenceDiagram
actor User
participant Main as Main Process
participant IPC as IPC / Electron
participant Renderer as Renderer Window
participant Store as Sidebar/Sidepanel Store
participant Component as WindowSideBar Component
User->>Main: Press Cmd/Ctrl+B or Cmd/Ctrl+J
Main->>Main: globalShortcut handler runs
Main->>Main: verify focused window ∈ presenter.getAllWindows()
alt focused chat window
Main->>IPC: send SHORTCUT_EVENTS.TOGGLE_SIDEBAR / TOGGLE_WORKSPACE
IPC->>Renderer: deliver shortcut event
Renderer->>Store: invoke handleToggleSidebar / handleToggleWorkspace
Store->>Store: toggle collapsed or toggleWorkspace(sessionId)
Store->>Component: reactive update
Component->>Component: update UI (width/classes)
else not focused or non-chat
Main->>Main: no IPC dispatch
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
160-170:⚠️ Potential issue | 🟠 MajorMissing peer dependencies for markstream-vue@0.0.12-beta.1.
The package declares peer dependencies on
shiki,stream-markdown, and@terrastruct/d2, but onlyshikiis available transitively viastream-monaco@0.0.22(which depends onshiki@^3.19.0, satisfying the peer requirement ofshiki@^3.13.0). However,stream-markdownand@terrastruct/d2are genuinely absent from the dependency tree.Either add these as explicit dependencies/devDependencies if markstream-vue requires them, or verify with the package maintainer whether these peer dependencies are optional and can be safely unmet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 160 - 170, markstream-vue@0.0.12-beta.1 declares peerDependencies on shiki, stream-markdown, and `@terrastruct/d2` but only shiki is present transitively via stream-monaco; add the missing peers by adding "stream-markdown" and "@terrastruct/d2" to package.json (either "dependencies" or "devDependencies" depending on runtime need) so markstream-vue's peers are satisfied, or alternatively confirm with the markstream-vue maintainer that those peers are optional and document that decision; update package.json accordingly and run a fresh install to validate the dependency tree.
🧹 Nitpick comments (1)
docs/specs/sidebar-workspace-shortcuts/spec.md (1)
23-25: Optional: vary Non-Goals bullet phrasing for readability.Lines 23–25 all begin with “No …”. Rewording one bullet would make the section read more naturally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/sidebar-workspace-shortcuts/spec.md` around lines 23 - 25, The three Non-Goals bullets all start with “No …”, which is repetitive; update one or two bullets to vary phrasing for better flow — e.g., keep the first as “No persistence change for sidebar collapsed state.” but change the second to “We will not add new buttons, menu items, or redesign the layout.” and the third to “This increment will not include a full localization sweep for every existing locale.” Edit the three lines shown in the diff to use this varied wording while keeping the original intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/shortcutPresenter.ts`:
- Around line 71-93: The shortcut handlers currently send SHORTCUT_EVENTS to any
focused window; restrict them to the chat renderer by verifying the focused
window is the chat window before calling sendToWebContents. In the ToggleSidebar
and ToggleWorkspace handlers, replace the loose focusedWindow?.isFocused() check
with a check against the chat window (e.g., compare focusedWindow to
presenter.windowPresenter.getChatWindow(), or check focusedWindow?.type ===
'chat' / focusedWindow?.webContents.id ===
presenter.windowPresenter.getChatWindow()?.webContents.id) and only call
presenter.windowPresenter.sendToWebContents(...) when that check passes.
---
Outside diff comments:
In `@package.json`:
- Around line 160-170: markstream-vue@0.0.12-beta.1 declares peerDependencies on
shiki, stream-markdown, and `@terrastruct/d2` but only shiki is present
transitively via stream-monaco; add the missing peers by adding
"stream-markdown" and "@terrastruct/d2" to package.json (either "dependencies"
or "devDependencies" depending on runtime need) so markstream-vue's peers are
satisfied, or alternatively confirm with the markstream-vue maintainer that
those peers are optional and document that decision; update package.json
accordingly and run a fresh install to validate the dependency tree.
---
Nitpick comments:
In `@docs/specs/sidebar-workspace-shortcuts/spec.md`:
- Around line 23-25: The three Non-Goals bullets all start with “No …”, which is
repetitive; update one or two bullets to vary phrasing for better flow — e.g.,
keep the first as “No persistence change for sidebar collapsed state.” but
change the second to “We will not add new buttons, menu items, or redesign the
layout.” and the third to “This increment will not include a full localization
sweep for every existing locale.” Edit the three lines shown in the diff to use
this varied wording while keeping the original intent.
🪄 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: eb642cb3-b30a-433e-9491-004bb95a8f87
📒 Files selected for processing (26)
docs/specs/sidebar-workspace-shortcuts/spec.mdpackage.jsonsrc/main/events.tssrc/main/presenter/configPresenter/shortcutKeySettings.tssrc/main/presenter/shortcutPresenter.tssrc/renderer/settings/components/ShortcutSettings.vuesrc/renderer/src/App.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/composables/useAppIpcRuntime.tssrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/ui/sidebar.tstest/main/presenter/shortcutPresenter.test.tstest/renderer/components/App.startup.test.tstest/renderer/components/WindowSideBar.test.ts
Comment on lines
+71
to
+93
| if (this.shortcutKeys.ToggleSidebar) { | ||
| globalShortcut.register(this.shortcutKeys.ToggleSidebar, () => { | ||
| const focusedWindow = presenter.windowPresenter.getFocusedWindow() | ||
| if (focusedWindow?.isFocused()) { | ||
| void presenter.windowPresenter.sendToWebContents( | ||
| focusedWindow.webContents.id, | ||
| SHORTCUT_EVENTS.TOGGLE_SIDEBAR | ||
| ) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| if (this.shortcutKeys.ToggleWorkspace) { | ||
| globalShortcut.register(this.shortcutKeys.ToggleWorkspace, () => { | ||
| const focusedWindow = presenter.windowPresenter.getFocusedWindow() | ||
| if (focusedWindow?.isFocused()) { | ||
| void presenter.windowPresenter.sendToWebContents( | ||
| focusedWindow.webContents.id, | ||
| SHORTCUT_EVENTS.TOGGLE_WORKSPACE | ||
| ) | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Restrict shortcut dispatch to chat window only.
At Line 73 and Line 85, dispatch currently targets any focused window. If the settings window is focused, TOGGLE_SIDEBAR / TOGGLE_WORKSPACE are sent to the wrong renderer context.
💡 Suggested fix
if (this.shortcutKeys.ToggleSidebar) {
globalShortcut.register(this.shortcutKeys.ToggleSidebar, () => {
const focusedWindow = presenter.windowPresenter.getFocusedWindow()
- if (focusedWindow?.isFocused()) {
- void presenter.windowPresenter.sendToWebContents(
- focusedWindow.webContents.id,
- SHORTCUT_EVENTS.TOGGLE_SIDEBAR
- )
- }
+ const settingsWindowId = presenter.windowPresenter.getSettingsWindowId()
+ if (!focusedWindow?.isFocused() || focusedWindow.id === settingsWindowId) return
+ void presenter.windowPresenter.sendToWebContents(
+ focusedWindow.webContents.id,
+ SHORTCUT_EVENTS.TOGGLE_SIDEBAR
+ )
})
}
if (this.shortcutKeys.ToggleWorkspace) {
globalShortcut.register(this.shortcutKeys.ToggleWorkspace, () => {
const focusedWindow = presenter.windowPresenter.getFocusedWindow()
- if (focusedWindow?.isFocused()) {
- void presenter.windowPresenter.sendToWebContents(
- focusedWindow.webContents.id,
- SHORTCUT_EVENTS.TOGGLE_WORKSPACE
- )
- }
+ const settingsWindowId = presenter.windowPresenter.getSettingsWindowId()
+ if (!focusedWindow?.isFocused() || focusedWindow.id === settingsWindowId) return
+ void presenter.windowPresenter.sendToWebContents(
+ focusedWindow.webContents.id,
+ SHORTCUT_EVENTS.TOGGLE_WORKSPACE
+ )
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/shortcutPresenter.ts` around lines 71 - 93, The shortcut
handlers currently send SHORTCUT_EVENTS to any focused window; restrict them to
the chat renderer by verifying the focused window is the chat window before
calling sendToWebContents. In the ToggleSidebar and ToggleWorkspace handlers,
replace the loose focusedWindow?.isFocused() check with a check against the chat
window (e.g., compare focusedWindow to
presenter.windowPresenter.getChatWindow(), or check focusedWindow?.type ===
'chat' / focusedWindow?.webContents.id ===
presenter.windowPresenter.getChatWindow()?.webContents.id) and only call
presenter.windowPresenter.sendToWebContents(...) when that check passes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/presenter/shortcutPresenter.ts (1)
84-107: Optional: DRY the two toggle shortcut handlers.Both registrations share the same logic; extracting a tiny helper would reduce repetition.
♻️ Suggested refactor
+ const registerFocusedChatToggle = (accelerator: string | undefined, event: string) => { + if (!accelerator) return + globalShortcut.register(accelerator, () => { + const focusedWindow = getFocusedChatWindow() + if (!focusedWindow) return + void presenter.windowPresenter.sendToWebContents(focusedWindow.webContents.id, event) + }) + } + - if (this.shortcutKeys.ToggleSidebar) { - globalShortcut.register(this.shortcutKeys.ToggleSidebar, () => { - const focusedWindow = getFocusedChatWindow() - if (focusedWindow) { - void presenter.windowPresenter.sendToWebContents( - focusedWindow.webContents.id, - SHORTCUT_EVENTS.TOGGLE_SIDEBAR - ) - } - }) - } - - if (this.shortcutKeys.ToggleWorkspace) { - globalShortcut.register(this.shortcutKeys.ToggleWorkspace, () => { - const focusedWindow = getFocusedChatWindow() - if (focusedWindow) { - void presenter.windowPresenter.sendToWebContents( - focusedWindow.webContents.id, - SHORTCUT_EVENTS.TOGGLE_WORKSPACE - ) - } - }) - } + registerFocusedChatToggle(this.shortcutKeys.ToggleSidebar, SHORTCUT_EVENTS.TOGGLE_SIDEBAR) + registerFocusedChatToggle(this.shortcutKeys.ToggleWorkspace, SHORTCUT_EVENTS.TOGGLE_WORKSPACE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/shortcutPresenter.ts` around lines 84 - 107, Both ToggleSidebar and ToggleWorkspace handlers duplicate the same logic; extract a small helper to DRY them. Create a helper function (e.g., registerToggleShortcut) that accepts a shortcut key and SHORTCUT_EVENTS value, calls globalShortcut.register(key, () => { const focusedWindow = getFocusedChatWindow(); if (focusedWindow) void presenter.windowPresenter.sendToWebContents(focusedWindow.webContents.id, event); }), then replace the two explicit globalShortcut.register calls for this.shortcutKeys.ToggleSidebar and this.shortcutKeys.ToggleWorkspace with calls to registerToggleShortcut(this.shortcutKeys.ToggleSidebar, SHORTCUT_EVENTS.TOGGLE_SIDEBAR) and registerToggleShortcut(this.shortcutKeys.ToggleWorkspace, SHORTCUT_EVENTS.TOGGLE_WORKSPACE).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/shortcutPresenter.ts`:
- Around line 84-107: Both ToggleSidebar and ToggleWorkspace handlers duplicate
the same logic; extract a small helper to DRY them. Create a helper function
(e.g., registerToggleShortcut) that accepts a shortcut key and SHORTCUT_EVENTS
value, calls globalShortcut.register(key, () => { const focusedWindow =
getFocusedChatWindow(); if (focusedWindow) void
presenter.windowPresenter.sendToWebContents(focusedWindow.webContents.id,
event); }), then replace the two explicit globalShortcut.register calls for
this.shortcutKeys.ToggleSidebar and this.shortcutKeys.ToggleWorkspace with calls
to registerToggleShortcut(this.shortcutKeys.ToggleSidebar,
SHORTCUT_EVENTS.TOGGLE_SIDEBAR) and
registerToggleShortcut(this.shortcutKeys.ToggleWorkspace,
SHORTCUT_EVENTS.TOGGLE_WORKSPACE).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b47eb7a9-3cb9-4e4e-a48f-7a69f7850274
📒 Files selected for processing (2)
src/main/presenter/shortcutPresenter.tstest/main/presenter/shortcutPresenter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/shortcutPresenter.test.ts
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 by CodeRabbit