Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughWorktree operation routing now collects exact and repository-derived route candidates and resolves them using the focused execution host. Local and runtime projections can be selected when focus uniquely matches one candidate, while unmatched duplicate projections remain ambiguous or missing. Terminal PTY routing now uses the focused HUB without IPC transport in that case and fails closed when focus matches neither HUB. Ownership, deletion, and branch-removal tests cover the updated outcomes. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/src/lib/worktree-operation-route-focus.ts (1)
34-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a dedicated unit test file for this module.
preferFocusedRoute/resolveFromCandidateRoutes/addOperationRouteare pure, reusable helpers, but coverage is only indirect viaworktree-operation-route.test.ts/terminal-worktree-route.test.ts. A focusedworktree-operation-route-focus.test.tswould pin edge cases (e.g., >2 candidates with multiple focus matches,settingsnull/undefined) more cheaply than exercising them through the full aggregation pipeline.src/renderer/src/lib/worktree-operation-route.ts (1)
230-242: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScope the projection scan to the worktree's own repo instead of all repos.
Both loops iterate every repo's worktree array (
Object.values(state.worktreesByRepo ?? {})/...detectedWorktreesByRepo) and filter by id insidecollectOwnerRoutes, even thoughgetRepoIdFromWorktreeId(worktreeId)— already used a few lines below at line 241 and at line 169 — can derive the owning repo id directly. This turns an O(worktrees-in-one-repo) lookup into an O(total-worktrees-across-all-repos) scan on every call, on what is likely a fairly hot path (terminal connect / owner-routed operations).⚡ Proposed fix to scope the scan by repo id
export function resolveExplicitWorktreeOperationRouteResult( state: WorktreeOperationRouteState, worktreeId: string ): WorktreeOperationRouteResolution { const exactRoutes = new Map<string, WorktreeOperationRoute>() const exactRepoIds = new Set<string>() + const worktreeRepoId = getRepoIdFromWorktreeId(worktreeId) // Why: scan every projection so focus can select the unique active-host owner (`#10491`). - for (const worktrees of Object.values(state.worktreesByRepo ?? {})) { - collectOwnerRoutes(state, worktrees, worktreeId, exactRoutes, exactRepoIds) - } - for (const result of Object.values(state.detectedWorktreesByRepo ?? {})) { - collectOwnerRoutes(state, result.worktrees, worktreeId, exactRoutes, exactRepoIds) - } + collectOwnerRoutes( + state, + state.worktreesByRepo?.[worktreeRepoId] ?? [], + worktreeId, + exactRoutes, + exactRepoIds + ) + collectOwnerRoutes( + state, + state.detectedWorktreesByRepo?.[worktreeRepoId]?.worktrees ?? [], + worktreeId, + exactRoutes, + exactRepoIds + )If there's a legacy scenario where a worktree id can live under a different catalog key than its own repo-id prefix, please confirm before applying — otherwise this preserves identical behavior while avoiding the full scan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd4b9cdf-9483-45fe-831a-313bd5addfb8
📒 Files selected for processing (8)
src/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/lib/terminal-worktree-route.test.tssrc/renderer/src/lib/worktree-operation-route-focus.tssrc/renderer/src/lib/worktree-operation-route.test.tssrc/renderer/src/lib/worktree-operation-route.tssrc/renderer/src/lib/worktree-runtime-owner.test.tssrc/renderer/src/runtime/file-explorer-delete-owner-provenance.test.tssrc/renderer/src/store/slices/worktrees.test.ts
5c5af57 to
c370412
Compare
…ost (stablyai#10491) When the same worktree/repo id is projected on multiple hosts, route terminal and owner operations to the unique owner matching the focused host instead of always failing closed as ambiguous. Still fail closed when focus does not discriminate among candidates.
c370412 to
e2520e5
Compare
Sync update (
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Summary
Fixes
Closes #10491
Test plan
ELI5
When the same project appears on local and remote hosts, creating a terminal or agent could fail as "ambiguous identity." If your focused host uniquely picks an owner, Orca uses that host instead of hard-failing.