Conversation
v2 click-to-open-externally (FilesTab tree, DiffPane entries, terminal links, file pane header) was always hitting Cursor regardless of the editor the user picked in the v2 Open In dropdown. Root cause: the server-side `resolveDefaultEditor` only consults v1 localDb tables, so it never saw the v2 choice (which lives client-side in tanstack-db `v2SidebarProjects.defaultOpenInApp`). The hook now forwards that choice to `openFileInEditor` as an explicit `app` override. Also fixes a silent path-resolution bug that produced doubled paths like `<worktree>/apps/desktop/apps/desktop/...` when relative diff paths were opened without a cwd: `resolvePath` now throws `RelativePathWithoutCwdError` instead of falling back to Electron's `process.cwd()`, and the tRPC input field is renamed `cwd` → `worktreePath` so the intent is explicit. Extracted `useV2ProjectDefaultApp(projectId)` as the single source of truth for reading/writing the v2 preference — used by both `V2OpenInMenuButton` (write on open) and `useOpenInExternalEditor` (read on open).
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEnforces explicit handling of relative paths by throwing RelativePathWithoutCwdError when no cwd/worktreePath is provided, maps that error to a TRPC BAD_REQUEST via withResolveGuard, renames cwd → worktreePath across UI/trpc calls, and adds a v2 project default-app hook plus related wiring. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Component
participant Hook
participant Router
participant Guard
participant Helper
User->>Component: request open file (path)
Component->>Hook: invoke open(path, line, column)
Hook->>Router: electronTrpcClient.external.openFileInEditor(path, worktreePath?, app?)
Router->>Guard: withResolveGuard -> validate resolvePath
Guard->>Helper: resolvePath(path, worktreePath)
alt resolved path is relative AND worktreePath missing
Helper-->>Guard: throw RelativePathWithoutCwdError
Guard-->>Router: convert to TRPCError(BAD_REQUEST)
Router-->>Component: error
Component-->>User: show validation error
else path valid
Helper-->>Guard: return resolved path
Guard-->>Router: continue
Router-->>Hook: success
Hook-->>Component: success
Component-->>User: file opened
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
148-187:⚠️ Potential issue | 🟡 MinorAll identified
openInAppcallers already pass absolute paths—the validation is safe.The new
isAbsoluteguard (line 161) is purely additive. Inspection of all 6 call sites—ClickablePath,OpenInButton,usePathActions, and others—confirms they all pass paths explicitly named or documented as absolute (absolutePathprop,project.mainRepoPathfrom API,currentPathfrom server state). Terminal link detection separately uses the host-sidestatPathendpoint which resolves relative/tilde paths before returning validated absolute paths.The optional refactor to enforce at the zod layer remains a reasonable improvement for cleaner error semantics, but the current runtime guard is correct and non-breaking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 148 - 187, Move the absolute-path validation into the procedure's Zod input schema so invalid input is rejected before the resolver runs: update the z.object for openInApp to use z.string().refine(nodePath.isAbsolute, { message: "openInApp requires an absolute path" }) for the path field (reference: openInApp, publicProcedure, z.object, nodePath.isAbsolute), then remove the runtime TRPCError isAbsolute check inside the mutation (or keep it as a redundant guard) and ensure the error message matches the Zod validation for consistent semantics.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx (1)
224-231: Nit:handleOpenInEditoris now a pure passthrough.Since
openInExternalEditoralready has a stable identity and accepts(path, opts?), you can drophandleOpenInEditorand passopenInExternalEditordirectly asonOpenInEditor(the prop's(absolutePath) => voidsignature is structurally compatible becauseoptsis optional).♻️ Proposed simplification
const openInExternalEditor = useOpenInExternalEditor(workspaceId); - - const handleOpenInEditor = useCallback( - (absolutePath: string) => { - openInExternalEditor(absolutePath); - }, - [openInExternalEditor], - );Then replace
onOpenInEditor={handleOpenInEditor}at line 596 withonOpenInEditor={openInExternalEditor}.🤖 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/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx around lines 224 - 231, Remove the unnecessary passthrough callback handleOpenInEditor and pass the hook result directly: stop declaring handleOpenInEditor and instead use the stable openInExternalEditor returned from useOpenInExternalEditor(workspaceId) as the onOpenInEditor prop (replace onOpenInEditor={handleOpenInEditor} with onOpenInEditor={openInExternalEditor}); openInExternalEditor already has a compatible signature ((path, opts?) => void) so no wrapper is needed.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts (1)
45-68: Minor:workspaceHostidentity churn may invalidate the returned callback.
workspaceHostis a live-query object whose identity typically changes on every collection update. Depending on it (rather thanworkspaceHost?.hostMachineId) makesopenInExternalEditorunstable, which can defeatReact.memoon consumers likeDiffFileEntrythat receive it asonOpenFile.♻️ Proposed fix
- const workspaceHost = workspacesWithHost[0]; - const projectId = workspaceHost?.projectId ?? undefined; + const workspaceHost = workspacesWithHost[0]; + const hostMachineId = workspaceHost?.hostMachineId ?? null; + const projectId = workspaceHost?.projectId ?? undefined; @@ - if (workspaceHost?.hostMachineId !== machineId) { + if (hostMachineId !== machineId) { toast.error("Can't open remote workspace paths in an external editor"); return; } @@ - [workspaceHost, machineId, projectId, worktreePath, v2PreferredApp], + [hostMachineId, machineId, projectId, worktreePath, v2PreferredApp],🤖 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/useOpenInExternalEditor/useOpenInExternalEditor.ts around lines 45 - 68, The returned callback from useOpenInExternalEditor is unstable because it depends on the whole live-query object workspaceHost; change it to depend only on the stable primitive workspaceHost?.hostMachineId (e.g., capture const hostMachineId = workspaceHost?.hostMachineId and use that in the closure and dependency array) so openInExternalEditor (used by consumers like DiffFileEntry) remains stable; update the dependency list of the useCallback in useOpenInExternalEditor to include hostMachineId instead of workspaceHost while keeping other deps (machineId, projectId, worktreePath, v2PreferredApp).apps/desktop/src/lib/trpc/routers/external/helpers.test.ts (1)
591-621: Optional: new describe block partially duplicates existing coverage.Absolute /
~/file:/// "relative with cwd" are already covered by the earlierdescribeblocks in this file. The one new case worth keeping is the wrapped/quoted relative-path throw. Consolidating (or dropping the redundant sub-tests) would keep the suite tighter; leaving as-is is fine too since the explicit "guards against process.cwd() fallback" grouping does add documentation value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/helpers.test.ts` around lines 591 - 621, The new "resolvePath guards against process.cwd() fallback" describe block duplicates coverage already present for absolute, ~-prefixed, file://, and "relative with cwd" cases; remove the redundant tests and keep only the unique wrapped/quoted relative-path case to reduce duplication. Update the describe block (or collapse it into an earlier describe) so it only contains the test that asserts resolvePath('"apps/desktop/src/index.ts"') throws RelativePathWithoutCwdError (retain the existing test name "throws for a wrapped/quoted relative path with no cwd" and keep references to resolvePath and RelativePathWithoutCwdError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 148-187: Move the absolute-path validation into the procedure's
Zod input schema so invalid input is rejected before the resolver runs: update
the z.object for openInApp to use z.string().refine(nodePath.isAbsolute, {
message: "openInApp requires an absolute path" }) for the path field (reference:
openInApp, publicProcedure, z.object, nodePath.isAbsolute), then remove the
runtime TRPCError isAbsolute check inside the mutation (or keep it as a
redundant guard) and ensure the error message matches the Zod validation for
consistent semantics.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/external/helpers.test.ts`:
- Around line 591-621: The new "resolvePath guards against process.cwd()
fallback" describe block duplicates coverage already present for absolute,
~-prefixed, file://, and "relative with cwd" cases; remove the redundant tests
and keep only the unique wrapped/quoted relative-path case to reduce
duplication. Update the describe block (or collapse it into an earlier describe)
so it only contains the test that asserts
resolvePath('"apps/desktop/src/index.ts"') throws RelativePathWithoutCwdError
(retain the existing test name "throws for a wrapped/quoted relative path with
no cwd" and keep references to resolvePath and RelativePathWithoutCwdError).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx:
- Around line 224-231: Remove the unnecessary passthrough callback
handleOpenInEditor and pass the hook result directly: stop declaring
handleOpenInEditor and instead use the stable openInExternalEditor returned from
useOpenInExternalEditor(workspaceId) as the onOpenInEditor prop (replace
onOpenInEditor={handleOpenInEditor} with onOpenInEditor={openInExternalEditor});
openInExternalEditor already has a compatible signature ((path, opts?) => void)
so no wrapper is needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts:
- Around line 45-68: The returned callback from useOpenInExternalEditor is
unstable because it depends on the whole live-query object workspaceHost; change
it to depend only on the stable primitive workspaceHost?.hostMachineId (e.g.,
capture const hostMachineId = workspaceHost?.hostMachineId and use that in the
closure and dependency array) so openInExternalEditor (used by consumers like
DiffFileEntry) remains stable; update the dependency list of the useCallback in
useOpenInExternalEditor to include hostMachineId instead of workspaceHost while
keeping other deps (machineId, projectId, worktreePath, v2PreferredApp).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5df920a4-753b-4e71-9938-766085e2ae1c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
apps/desktop/src/lib/trpc/routers/external/helpers.test.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsxapps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectDefaultApp/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectDefaultApp/useV2ProjectDefaultApp.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileSearchResultItem/FileSearchResultItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileTreeItem/FileTreeItem.tsx
Greptile SummaryThis PR fixes two root-cause bugs in v2 file-open flows and does a clean refactor of the related code paths. The core changes are:
Confidence Score: 4/5PR is safe to merge; the core fix is correct and well-tested, with one P1 UX regression in the loading-state guard that should be addressed. The root-cause fix (forwarding v2PreferredApp to server, throwing on relative-without-cwd) is logically sound and backed by new tests. The refactor cleanly eliminates duplicated logic. The one concrete regression — showing a misleading toast when host data hasn't loaded yet — is P1 but affects only a short window and tanstack-db is a synchronous local store so the likelihood is low. apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts — loading-state guard change (lines 49-52)
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts | Central v2 file-open hook — correctly forwards worktreePath and v2PreferredApp to the server, but the workspaceHost loading guard was simplified in a way that shows a misleading toast on local workspaces before host data resolves; also adds a second workspace query that may be redundant. |
| apps/desktop/src/lib/trpc/routers/external/helpers.ts | Introduces RelativePathWithoutCwdError and makes resolvePath throw it instead of silently falling back to process.cwd() — clean, well-documented safety improvement. |
| apps/desktop/src/lib/trpc/routers/external/index.ts | Renames cwd → worktreePath, adds app override to openFileInEditor, adds withResolveGuard for clear BAD_REQUEST errors, and adds an early absolute-path check to openInApp — all correct. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectDefaultApp/useV2ProjectDefaultApp.ts | New hook — clean single source of truth for v2 per-project open-in app preference; correctly queries v2SidebarProjects and guards setApp when projectId is undefined. |
| apps/desktop/src/lib/trpc/routers/external/helpers.test.ts | Test coverage for the new RelativePathWithoutCwdError and all edge cases (wrapped paths, ~-prefix, file:// URLs, relative with cwd) — comprehensive. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx | Refactored to delegate to useV2ProjectDefaultApp — removes duplicated tanstack-db query logic, no functional change. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/FilesTab/FilesTab.tsx | Removes duplicated host-locality check and open-in-app logic in favour of useOpenInExternalEditor — significant simplification. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx | Removes inline openFile callback and delegates to useOpenInExternalEditor; old if (!worktreePath) return guard now handled server-side via withResolveGuard. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.ts | Rename-only change: cwd → worktreePath in props interface and internal usage — mechanical, correct. |
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts, line 49-52 (link)Misleading error toast when host data is still loading
When
workspaceHostisundefined(theuseLiveQueryhasn't resolved yet),workspaceHost?.hostMachineIdevaluates toundefined. Sinceundefined !== machineIdis alwaystrue, the user gets a "Can't open remote workspace paths in an external editor" toast even on a fully local workspace — simply because the tanstack-db query hadn't returned yet.The prior code handled these two cases explicitly:
// old: silent early-return while loading, toast only if actually remote if (!workspaceHost || workspaceHost.hostMachineId !== machineId) { if (workspaceHost && workspaceHost.hostMachineId !== machineId) { toast.error("Opening in editor is only supported on local workspaces"); } return; }
The new version collapses both states into a single branch that always fires the toast:
// new: always toasts, even when workspaceHost is undefined if (workspaceHost?.hostMachineId !== machineId) { toast.error("Can't open remote workspace paths in an external editor"); return; }
Restoring the two-case check would fix it:
if (!workspaceHost || workspaceHost.hostMachineId !== machineId) { if (workspaceHost && workspaceHost.hostMachineId !== machineId) { toast.error("Can't open remote workspace paths in an external editor"); } return; }
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts Line: 49-52 Comment: **Misleading error toast when host data is still loading** When `workspaceHost` is `undefined` (the `useLiveQuery` hasn't resolved yet), `workspaceHost?.hostMachineId` evaluates to `undefined`. Since `undefined !== machineId` is always `true`, the user gets a "Can't open remote workspace paths in an external editor" toast even on a fully local workspace — simply because the tanstack-db query hadn't returned yet. The prior code handled these two cases explicitly: ```ts // old: silent early-return while loading, toast only if actually remote if (!workspaceHost || workspaceHost.hostMachineId !== machineId) { if (workspaceHost && workspaceHost.hostMachineId !== machineId) { toast.error("Opening in editor is only supported on local workspaces"); } return; } ``` The new version collapses both states into a single branch that always fires the toast: ```ts // new: always toasts, even when workspaceHost is undefined if (workspaceHost?.hostMachineId !== machineId) { toast.error("Can't open remote workspace paths in an external editor"); return; } ``` Restoring the two-case check would fix it: ```ts if (!workspaceHost || workspaceHost.hostMachineId !== machineId) { if (workspaceHost && workspaceHost.hostMachineId !== machineId) { toast.error("Can't open remote workspace paths in an external editor"); } return; } ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts
Line: 49-52
Comment:
**Misleading error toast when host data is still loading**
When `workspaceHost` is `undefined` (the `useLiveQuery` hasn't resolved yet), `workspaceHost?.hostMachineId` evaluates to `undefined`. Since `undefined !== machineId` is always `true`, the user gets a "Can't open remote workspace paths in an external editor" toast even on a fully local workspace — simply because the tanstack-db query hadn't returned yet.
The prior code handled these two cases explicitly:
```ts
// old: silent early-return while loading, toast only if actually remote
if (!workspaceHost || workspaceHost.hostMachineId !== machineId) {
if (workspaceHost && workspaceHost.hostMachineId !== machineId) {
toast.error("Opening in editor is only supported on local workspaces");
}
return;
}
```
The new version collapses both states into a single branch that always fires the toast:
```ts
// new: always toasts, even when workspaceHost is undefined
if (workspaceHost?.hostMachineId !== machineId) {
toast.error("Can't open remote workspace paths in an external editor");
return;
}
```
Restoring the two-case check would fix it:
```ts
if (!workspaceHost || workspaceHost.hostMachineId !== machineId) {
if (workspaceHost && workspaceHost.hostMachineId !== machineId) {
toast.error("Can't open remote workspace paths in an external editor");
}
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts
Line: 40-43
Comment:
**Redundant workspace query alongside existing `useLiveQuery`**
The hook already queries `v2Workspaces` via `useLiveQuery` to get `projectId`. Adding a second `workspaceTrpc.workspace.get.useQuery` purely to read `worktreePath` means every component that calls `useOpenInExternalEditor` now fires two separate workspace lookups.
If `worktreePath` is a field in the `v2Workspaces` collection, it could be selected in the existing `useLiveQuery` instead:
```ts
.select(({ workspaces, hosts }) => ({
hostMachineId: hosts?.machineId ?? null,
projectId: workspaces.projectId ?? null,
worktreePath: workspaces.worktreePath ?? null,
})),
```
This would eliminate the tRPC round-trip and keep the callback's dependencies driven by a single reactive source.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs(desktop): mention worktreePath not ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+40
to
+43
| const workspaceQuery = workspaceTrpc.workspace.get.useQuery({ | ||
| id: workspaceId, | ||
| }); | ||
| const worktreePath = workspaceQuery.data?.worktreePath ?? undefined; |
There was a problem hiding this comment.
useLiveQuery
The hook already queries v2Workspaces via useLiveQuery to get projectId. Adding a second workspaceTrpc.workspace.get.useQuery purely to read worktreePath means every component that calls useOpenInExternalEditor now fires two separate workspace lookups.
If worktreePath is a field in the v2Workspaces collection, it could be selected in the existing useLiveQuery instead:
.select(({ workspaces, hosts }) => ({
hostMachineId: hosts?.machineId ?? null,
projectId: workspaces.projectId ?? null,
worktreePath: workspaces.worktreePath ?? null,
})),This would eliminate the tRPC round-trip and keep the callback's dependencies driven by a single reactive source.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts
Line: 40-43
Comment:
**Redundant workspace query alongside existing `useLiveQuery`**
The hook already queries `v2Workspaces` via `useLiveQuery` to get `projectId`. Adding a second `workspaceTrpc.workspace.get.useQuery` purely to read `worktreePath` means every component that calls `useOpenInExternalEditor` now fires two separate workspace lookups.
If `worktreePath` is a field in the `v2Workspaces` collection, it could be selected in the existing `useLiveQuery` instead:
```ts
.select(({ workspaces, hosts }) => ({
hostMachineId: hosts?.machineId ?? null,
projectId: workspaces.projectId ?? null,
worktreePath: workspaces.worktreePath ?? null,
})),
```
This would eliminate the tRPC round-trip and keep the callback's dependencies driven by a single reactive source.
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
Author
There was a problem hiding this comment.
SelectV2Workspace is inferred from the Drizzle schema in packages/db/src/schema/schema.ts which does not include worktreePath — v2 worktree paths are computed on the host-service side and returned via workspaceTrpc.workspace.get, they are not replicated into the tanstack-db collection. The second query is the canonical source (used throughout v2: page.tsx, FilesTab, WorkspaceDiff, useWorkspaceChatController, etc.), and react-query dedupes the network call across callsites by query key.
…n-any-file-to-open-external-we-should-be-using-the-cmdo-choice-the-user-made # Conflicts: # bun.lock
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 16 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/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx:56">
P2: Keep the worktreePath guard before opening diff files. This wiring can call the external-editor mutation with `worktreePath: undefined`, which now throws for relative diff paths.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| [worktreePath, projectId], | ||
| ); | ||
| const openInExternalEditor = useOpenInExternalEditor(workspaceId); |
Contributor
There was a problem hiding this comment.
P2: Keep the worktreePath guard before opening diff files. This wiring can call the external-editor mutation with worktreePath: undefined, which now throws for relative diff paths.
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/usePaneRegistry/components/DiffPane/DiffPane.tsx, line 56:
<comment>Keep the worktreePath guard before opening diff files. This wiring can call the external-editor mutation with `worktreePath: undefined`, which now throws for relative diff paths.</comment>
<file context>
@@ -55,24 +53,7 @@ export function DiffPane({ context, workspaceId }: DiffPaneProps) {
- },
- [worktreePath, projectId],
- );
+ const openInExternalEditor = useOpenInExternalEditor(workspaceId);
// O(1) collapsed lookup per child instead of Array.includes.
</file context>
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
deleted the
in-v2-when-clicking-on-any-file-to-open-external-we-should-be-using-the-cmdo-choice-the-user-made
branch
5 tasks
This was referenced Apr 23, 2026
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
resolveDefaultEditoronly consults v1 localDb tables, so it never saw the v2 choice (which lives client-side in tanstack-dbv2SidebarProjects.defaultOpenInApp). The hook now forwards that choice toopenFileInEditoras an explicitappoverride.<worktree>/apps/desktop/apps/desktop/...when relative diff paths were opened without a cwd:resolvePathnow throwsRelativePathWithoutCwdErrorinstead of falling back to Electron'sprocess.cwd(). The tRPC input field is renamedcwd→worktreePathso the intent is explicit, andopenInApprejects non-absolute paths (it has no cwd context).useV2ProjectDefaultApp(projectId)as the single source of truth for reading/writing the v2 preference — used by bothV2OpenInMenuButton(write on successful open) anduseOpenInExternalEditor(read on every file open).useOpenInExternalEditorinstead of calling the tRPC mutation directly, so every v2 file-open goes through one place that forwardsworktreePath+ the v2 app choice automatically.Test plan
bun test apps/desktop/src/lib/trpc/routers/external/helpers.test.ts— 106 pass.bun run typecheck— clean.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Summary by cubic
v2 file-open now respects the CMD+O “Open In” choice across FilesTab, DiffPane, terminal links, and file headers. It adds strict path resolution with clear BAD_REQUEST errors and standardizes
worktreePathacross APIs.Bug Fixes
apptoexternal.openFileInEditor.resolvePaththrowsRelativePathWithoutCwdErrorandwithResolveGuardsurfaces it as BAD_REQUEST;external.openInAppnow requires an absolutepath.Refactors
useV2ProjectDefaultAppas the single source of truth; used byV2OpenInMenuButtonanduseOpenInExternalEditor.useOpenInExternalEditorto auto-forwardworktreePathand the v2app.Written for commit 8661702. Summary will update on new commits.