Repository navigation
Conversation
…ath-status gate onJumpToWorktreeIndex called activateAndRevealWorktree unconditionally, but the rendered sidebar order can contain folder-workspace keys (folder:<id>). Those skipped the getFreshFolderWorkspacePathStatus check that clicking the card enforces, silently activating workspaces whose folder is missing, unmounted, or on a disconnected host. Route the shortcut through a parseWorkspaceKey branch mirroring activateWorktreeFromSidebar and the nav-history activator. Fixes stablyai#10716
📝 WalkthroughWalkthroughWorkspace activation now uses an exported helper that parses workspace keys and dispatches folder workspaces to guarded folder activation while preserving worktree activation. The workspace-index IPC handler calls this helper. New tests cover missing, ambiguous, and existing folder paths, platform-specific digit shortcut routing, and handler wiring. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Re: the Docstring Coverage warning - this is CodeRabbit's default advisory threshold reacting to a small diff (2 eligible functions, 1 documented). The documented one is the new exported |
The fix is correct: Cmd/Ctrl+1-9 on a folder-workspace slot must go through the path-status gate, and it now does. Two review changes: 1. Reuse instead of duplicate. worktree-activation.ts already contained this exact worktree-vs-folder dispatch inline in the nav-history activator. Export it as activateAndRevealWorkspace and point both the nav activator and the number shortcut at it, rather than adding a new module holding a byte-identical third copy of the same branch. 2. Test the real path. The original test mocked @/lib/worktree-activation -- the very module whose folder branch is the fix -- so it passed even with the useIpcEvents wiring reverted. Replaced with a suite that drives the real activator against a real store, plus a wiring assertion, so both the guard and its call site fail on regression. Also covers the AGENTS.md cases the fix targets: missing/unmounted path and ambiguous (disconnected) SSH connection, and asserts the digit chord resolves on darwin/win32/linux so the gate is not macOS-only.
|
Reviewed with an independent reproduction — including a live-app check of Orca opening a terminal into a directory I had just deleted. The bug is real and the gate is the right fix. Two things needed rework, both pushed to this branch. 1. The tests did not exercise the fix. 2. The fix duplicated code it already imported. Cross-platform (AGENTS.md): clean. No Perf: the guard is a synchronous in-memory cache read, not IPC or a Validation: typecheck exit 0, oxlint exit 0, 3516 tests passing across the touched trees. Good catch on the underlying inconsistency — the click path enforcing a gate the shortcut skipped is exactly the kind of asymmetry that's easy to miss. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90d7e0c3-4fed-4184-8551-8571aa4ad73e
📒 Files selected for processing (3)
src/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/lib/workspace-activation-path-gate.test.tssrc/renderer/src/lib/worktree-activation.ts
| const visibleIds = getVisibleWorktreeIds() | ||
| if (index < visibleIds.length) { | ||
| activateAndRevealWorktree(visibleIds[index]) | ||
| activateAndRevealWorkspace(visibleIds[index]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject invalid workspace indices before indexing.
Negative or fractional values pass this check, yielding undefined; the workspace dispatcher then calls parseWorkspaceKey(undefined) and throws. Require a non-negative integer.
Proposed fix
- if (index < visibleIds.length) {
+ if (Number.isInteger(index) && index >= 0 && index < visibleIds.length) {
activateAndRevealWorkspace(visibleIds[index])
}📝 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.
| activateAndRevealWorkspace(visibleIds[index]) | |
| if (Number.isInteger(index) && index >= 0 && index < visibleIds.length) { |
…stablyai#10748) Reviewed with an independent reproduction, including a live-app check. Replaced tests that mocked the module containing the fix, and reused the existing workspace activator instead of adding a 4th copy of the dispatch.
Summary
Pressing Cmd/Ctrl+1–9 on a folder-workspace sidebar slot now enforces the same path-status gate as clicking its card. Previously the number shortcut activated the workspace unconditionally, so a folder workspace whose path is missing,
unmounted, or on a disconnected SSH host activated silently into a folder that isn't there; clicking the same card correctly blocked with an error toast.
The
onJumpToWorktreeIndexhandler now routes through a newactivateWorkspaceNumberShortcut(workspaceId)that branches onparseWorkspaceKey—folder:keys go through the guardedactivateAndRevealFolderWorkspace, everything else throughactivateAndRevealWorktree— mirroringactivateWorktreeFromSidebarand the nav-history activator.Fixes #10716. Credit to @Hanxcellent, whose self-closed #9560 contained this fix.
Screenshots
No visual change - the blocked case reuses the existing path-status error toast
already shown by the sidebar click path.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildAI Review Report
folderWorkspaceIdwhile the worktree path takes the raw key — verified againstactivateAndRevealFolderWorkspace's lookup ofstate.folderWorkspacesby raw id and the two existing dispatch sites, which use the identical shape.revealInSidebar: false) because the shortcut can fire with the sidebar closed;parseWorkspaceKeyreturningnullfor plainworktree ids falls through to the generic path, preserving prior behavior for all non-folder targets.
onJumpToWorktreeIndexIPC event — shortcut registration (CmdOrCtrlaccelerators), labels, and key handling are untouched, so Cmd+N on macOS and Ctrl+N on Linux/Windows route identically. No paths, no shell invocation, no Electron platform branches. The SSH case is the point of the fix: the gate blocks folder workspaces on disconnected SSH hosts. Folder workspaces and git worktrees are both handled per AGENTS.md.Security Audit
workspaceIdcomes from the internally computed rendered sidebar order, not user-typed input;parseWorkspaceKeyis plain prefix parsing with no regex or injection surface.onJumpToWorktreeIndexlistener's behavior is narrowed, not widened.Notes
Call out any platform-specific behavior, risks, or follow-up work.