fix(desktop): scope v2 open-in-app to local workspaces#2735
Merged
Conversation
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx
Contributor
📝 WalkthroughWalkthroughThis PR establishes a shared Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant V2Page as V2WorkspacePage
participant TopBar as TopBar Component
participant FilesBrowser as WorkspaceFiles
participant WorkspaceTrpc as workspaceTrpc (Client)
participant HostService as Host Service
participant EventStream as WebSocket Events
User->>V2Page: Load V2 workspace
V2Page->>TopBar: Route detected, render V2 UI
TopBar->>FilesBrowser: User clicks files tab
FilesBrowser->>WorkspaceTrpc: workspace.get.useQuery()
WorkspaceTrpc->>HostService: Fetch workspace metadata
HostService-->>WorkspaceTrpc: Returns rootPath, worktreePath
FilesBrowser->>WorkspaceTrpc: filesystem.listDirectory.useQuery(rootPath)
WorkspaceTrpc->>HostService: List directory contents
HostService-->>WorkspaceTrpc: Returns file/folder tree
FilesBrowser->>FilesBrowser: Build tree structure
FilesBrowser->>EventStream: useWorkspaceFsEvents(workspaceId)
EventStream-->>FilesBrowser: Subscribe to /workspace-filesystem/:workspaceId/events
User->>User: Modify files on disk
HostService->>EventStream: FsWatchEvent (file changed)
EventStream-->>FilesBrowser: events: [{ kind, absolutePath }]
FilesBrowser->>FilesBrowser: Invalidate cache, refresh
User->>FilesBrowser: Select file in tree
FilesBrowser->>WorkspaceTrpc: filesystem.readFile(filePath)
WorkspaceTrpc->>HostService: Fetch file content
HostService-->>WorkspaceTrpc: Returns content (auto-detect text/binary)
FilesBrowser->>FilesBrowser: Render file preview
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 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 Tip You can get early access to new features in CodeRabbit.Enable the |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 60 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="packages/workspace-client/src/hooks/useFileDocument/useFileDocument.ts">
<violation number="1" location="packages/workspace-client/src/hooks/useFileDocument/useFileDocument.ts:149">
P2: Do not use an empty `catch {}` here; log the error context before returning `null` so disk-read failures are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="plans/host-service-diff-plan.md">
<violation number="1" location="plans/host-service-diff-plan.md:139">
P2: `getFileDiff` excludes the `untracked` category even though untracked files are part of the planned changes sections; this leaves no documented API path to load untracked file diff content.</violation>
</file>
<file name="packages/workspace-client/src/hooks/useWorkspaceFsEvents/useWorkspaceFsEvents.ts">
<violation number="1" location="packages/workspace-client/src/hooks/useWorkspaceFsEvents/useWorkspaceFsEvents.ts:22">
P2: Avoid using the whole `client` object as an effect dependency here; provider re-renders can change object identity and cause unnecessary FS event resubscriptions.
(Based on your team's feedback about narrowing React effect dependencies to required fields.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/host-service/src/runtime/filesystem/filesystem.ts">
<violation number="1" location="packages/host-service/src/runtime/filesystem/filesystem.ts:50">
P2: Handle `watcherManager.close()` failures with a `try/catch` to prevent shutdown-time unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await/catch.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx:101">
P2: Disabled buttons are used as tooltip triggers, so the "coming soon" tooltips are not reachable.</violation>
</file>
<file name="packages/workspace-client/src/hooks/useWorkspaceFsEventBridge/useWorkspaceFsEventBridge.ts">
<violation number="1" location="packages/workspace-client/src/hooks/useWorkspaceFsEventBridge/useWorkspaceFsEventBridge.ts:17">
P2: Narrow this effect dependency from the whole `client` object to required stable fields to avoid unnecessary bridge re-registration on unrelated provider re-renders.
(Based on your team's feedback about narrowing React effect dependencies.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/workspace-client/src/lib/workspaceFsEventRegistry.ts">
<violation number="1" location="packages/workspace-client/src/lib/workspaceFsEventRegistry.ts:72">
P2: A throwing listener can stop event delivery to other subscribers because listener callbacks are not isolated.</violation>
<violation number="2" location="packages/workspace-client/src/lib/workspaceFsEventRegistry.ts:75">
P1: The transport is not reset on stream failure, which can leave active subscriptions permanently disconnected.</violation>
</file>
<file name="packages/workspace-client/src/hooks/useFileTree/useFileTree.ts">
<violation number="1" location="packages/workspace-client/src/hooks/useFileTree/useFileTree.ts:61">
P2: Handle top-level POSIX paths in `getParentPath`. Returning `trimmedPath` for index `0` makes `/foo` its own parent and can skip root directory refreshes.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/workspace-client/src/hooks/useFileTree/useFileTree.ts:71">
P2: Normalize `rootPath` before relative-path slicing. Without normalization, root/trailing-separator roots can return absolute paths instead of relative ones.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.) [FEEDBACK_USED]</violation>
<violation number="3" location="packages/workspace-client/src/hooks/useFileTree/useFileTree.ts:87">
P1: Normalize `rootPath` before `isWithinPath` prefix checks. Root paths (like `/` or `C:\`) currently fail the `${rootPath}/`/`${rootPath}\\` checks, which can drop all fs events for root workspaces.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| listener(event); | ||
| } | ||
| }, | ||
| onError: (error) => { |
Contributor
There was a problem hiding this comment.
P1: The transport is not reset on stream failure, which can leave active subscriptions permanently disconnected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/lib/workspaceFsEventRegistry.ts, line 75:
<comment>The transport is not reset on stream failure, which can leave active subscriptions permanently disconnected.</comment>
<file context>
@@ -0,0 +1,114 @@
+ listener(event);
+ }
+ },
+ onError: (error) => {
+ console.error("[workspace-client/fs-events] Stream failed:", {
+ hostUrl: state.client.hostUrl,
</file context>
| return absolutePath; | ||
| } | ||
|
|
||
| function isWithinPath(rootPath: string, absolutePath: string): boolean { |
Contributor
There was a problem hiding this comment.
P1: Normalize rootPath before isWithinPath prefix checks. Root paths (like / or C:\) currently fail the ${rootPath}//${rootPath}\\ checks, which can drop all fs events for root workspaces.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/hooks/useFileTree/useFileTree.ts, line 87:
<comment>Normalize `rootPath` before `isWithinPath` prefix checks. Root paths (like `/` or `C:\`) currently fail the `${rootPath}/`/`${rootPath}\\` checks, which can drop all fs events for root workspaces.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.) </comment>
<file context>
@@ -0,0 +1,492 @@
+ return absolutePath;
+}
+
+function isWithinPath(rootPath: string, absolutePath: string): boolean {
+ return (
+ absolutePath === rootPath ||
</file context>
| } | ||
|
|
||
| return result.content; | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P2: Do not use an empty catch {} here; log the error context before returning null so disk-read failures are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/hooks/useFileDocument/useFileDocument.ts, line 149:
<comment>Do not use an empty `catch {}` here; log the error context before returning `null` so disk-read failures are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,322 @@
+ }
+
+ return result.content;
+ } catch {
+ return null;
+ }
</file context>
| workspaceId: string | ||
| absolutePath: string | ||
| oldAbsolutePath?: string | ||
| category: "against-base" | "committed" | "staged" | "unstaged" |
Contributor
There was a problem hiding this comment.
P2: getFileDiff excludes the untracked category even though untracked files are part of the planned changes sections; this leaves no documented API path to load untracked file diff content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/host-service-diff-plan.md, line 139:
<comment>`getFileDiff` excludes the `untracked` category even though untracked files are part of the planned changes sections; this leaves no documented API path to load untracked file diff content.</comment>
<file context>
@@ -0,0 +1,805 @@
+ workspaceId: string
+ absolutePath: string
+ oldAbsolutePath?: string
+ category: "against-base" | "committed" | "staged" | "unstaged"
+ commitHash?: string
+ defaultBranch?: string
</file context>
| return subscribeToWorkspaceFsEvents(client, workspaceId, (event) => { | ||
| onEvent(event); | ||
| }); | ||
| }, [client, enabled, workspaceId]); |
Contributor
There was a problem hiding this comment.
P2: Avoid using the whole client object as an effect dependency here; provider re-renders can change object identity and cause unnecessary FS event resubscriptions.
(Based on your team's feedback about narrowing React effect dependencies to required fields.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/hooks/useWorkspaceFsEvents/useWorkspaceFsEvents.ts, line 22:
<comment>Avoid using the whole `client` object as an effect dependency here; provider re-renders can change object identity and cause unnecessary FS event resubscriptions.
(Based on your team's feedback about narrowing React effect dependencies to required fields.) </comment>
<file context>
@@ -0,0 +1,23 @@
+ return subscribeToWorkspaceFsEvents(client, workspaceId, (event) => {
+ onEvent(event);
+ });
+ }, [client, enabled, workspaceId]);
+}
</file context>
|
|
||
| <div className="flex items-center gap-0.5"> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> |
Contributor
There was a problem hiding this comment.
P2: Disabled buttons are used as tooltip triggers, so the "coming soon" tooltips are not reachable.
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/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx, line 101:
<comment>Disabled buttons are used as tooltip triggers, so the "coming soon" tooltips are not reachable.</comment>
<file context>
@@ -0,0 +1,167 @@
+
+ <div className="flex items-center gap-0.5">
+ <Tooltip>
+ <TooltipTrigger asChild>
+ <Button
+ className="size-6"
</file context>
| } | ||
|
|
||
| return retainWorkspaceFsBridge(client, workspaceId); | ||
| }, [client, enabled, workspaceId]); |
Contributor
There was a problem hiding this comment.
P2: Narrow this effect dependency from the whole client object to required stable fields to avoid unnecessary bridge re-registration on unrelated provider re-renders.
(Based on your team's feedback about narrowing React effect dependencies.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/hooks/useWorkspaceFsEventBridge/useWorkspaceFsEventBridge.ts, line 17:
<comment>Narrow this effect dependency from the whole `client` object to required stable fields to avoid unnecessary bridge re-registration on unrelated provider re-renders.
(Based on your team's feedback about narrowing React effect dependencies.) </comment>
<file context>
@@ -0,0 +1,18 @@
+ }
+
+ return retainWorkspaceFsBridge(client, workspaceId);
+ }, [client, enabled, workspaceId]);
+}
</file context>
| workspaceId: state.workspaceId, | ||
| onEvent: (event) => { | ||
| for (const listener of state.listeners) { | ||
| listener(event); |
Contributor
There was a problem hiding this comment.
P2: A throwing listener can stop event delivery to other subscribers because listener callbacks are not isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/lib/workspaceFsEventRegistry.ts, line 72:
<comment>A throwing listener can stop event delivery to other subscribers because listener callbacks are not isolated.</comment>
<file context>
@@ -0,0 +1,114 @@
+ workspaceId: state.workspaceId,
+ onEvent: (event) => {
+ for (const listener of state.listeners) {
+ listener(event);
+ }
+ },
</file context>
| return trimmedPath.slice(0, lastSeparatorIndex); | ||
| } | ||
|
|
||
| function getRelativePath(rootPath: string, absolutePath: string): string { |
Contributor
There was a problem hiding this comment.
P2: Normalize rootPath before relative-path slicing. Without normalization, root/trailing-separator roots can return absolute paths instead of relative ones.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/hooks/useFileTree/useFileTree.ts, line 71:
<comment>Normalize `rootPath` before relative-path slicing. Without normalization, root/trailing-separator roots can return absolute paths instead of relative ones.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.) </comment>
<file context>
@@ -0,0 +1,492 @@
+ return trimmedPath.slice(0, lastSeparatorIndex);
+}
+
+function getRelativePath(rootPath: string, absolutePath: string): string {
+ if (absolutePath === rootPath) {
+ return "";
</file context>
| ); | ||
|
|
||
| if (lastSeparatorIndex <= 0) { | ||
| return trimmedPath; |
Contributor
There was a problem hiding this comment.
P2: Handle top-level POSIX paths in getParentPath. Returning trimmedPath for index 0 makes /foo its own parent and can skip root directory refreshes.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/hooks/useFileTree/useFileTree.ts, line 61:
<comment>Handle top-level POSIX paths in `getParentPath`. Returning `trimmedPath` for index `0` makes `/foo` its own parent and can skip root directory refreshes.
(Based on your team's feedback about using cross-platform path utilities instead of manual string parsing.) </comment>
<file context>
@@ -0,0 +1,492 @@
+ );
+
+ if (lastSeparatorIndex <= 0) {
+ return trimmedPath;
+ }
+
</file context>
![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: 8
🧹 Nitpick comments (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx (1)
12-12: Prefer a colocated debounce constant for this feature.Importing
SEARCH_DEBOUNCE_MSfrom a distant legacy sidebar module creates unnecessary coupling between separate workspace file UIs.As per coding guidelines, "Co-locate dependencies (utils, hooks, constants, config, tests, stories) next to the file using them".
🤖 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/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx at line 12, The toolbar imports SEARCH_DEBOUNCE_MS from a distant sidebar module causing unwanted coupling; create a colocated constant (e.g., in the same folder as WorkspaceFilesToolbar or a nearby constants.ts) and replace the external import in WorkspaceFilesToolbar.tsx with the new local constant, updating any references to SEARCH_DEBOUNCE_MS to use the colocated symbol so the workspace files UI no longer depends on the legacy sidebar module.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx (1)
39-53: Consider removing the unnecessary wrapper component.
V2OpenInMenuButtonsimply forwards all props toV2OpenInMenuButtonInnerwithout any additional logic. This wrapper can be removed by exportingV2OpenInMenuButtonInnerdirectly asV2OpenInMenuButton.♻️ Suggested simplification
-function V2OpenInMenuButtonInner({ +export function V2OpenInMenuButton({ branch, hostUrl, projectId, workspaceId, }: V2OpenInMenuButtonProps) { const workspaceQuery = useQuery({ queryKey: ["v2-open-in-workspace", hostUrl, workspaceId], queryFn: () => getHostServiceClientByUrl(hostUrl).workspace.get.query({ id: workspaceId, }), }); if (!workspaceQuery.data?.worktreePath) { return null; } return ( <OpenInMenuButton branch={branch} projectId={projectId} worktreePath={workspaceQuery.data.worktreePath} /> ); } - -export function V2OpenInMenuButton({ - branch, - hostUrl, - projectId, - workspaceId, -}: V2OpenInMenuButtonProps) { - return ( - <V2OpenInMenuButtonInner - branch={branch} - hostUrl={hostUrl} - projectId={projectId} - workspaceId={workspaceId} - /> - ); -}🤖 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/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx` around lines 39 - 53, V2OpenInMenuButton is a redundant passthrough that only returns V2OpenInMenuButtonInner with the same props; remove the wrapper by deleting the V2OpenInMenuButton function and instead export V2OpenInMenuButtonInner under the name V2OpenInMenuButton (or re-export it: export { V2OpenInMenuButtonInner as V2OpenInMenuButton }), and update any imports that reference V2OpenInMenuButton to continue working with the single exported component.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesSearchResultItem/WorkspaceFilesSearchResultItem.tsx (1)
1-3: Extract these shared search/file primitives out of the legacyFilesViewtree.Importing
SEARCH_RESULT_ROW_HEIGHTandFileIconfromrenderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/*couples the new v2 workspace UI to an unrelated screen subtree. A smallWorkspaceFiles-local shared module would keep this surface self-contained.As per coding guidelines, "Co-locate dependencies (utils, hooks, constants, config, tests, stories) next to the file using them".
🤖 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/WorkspaceFiles/components/WorkspaceFilesSearchResultItem/WorkspaceFilesSearchResultItem.tsx around lines 1 - 3, The WorkspaceFilesSearchResultItem component currently imports SEARCH_RESULT_ROW_HEIGHT and FileIcon from the legacy FilesView tree; extract those shared primitives into a new co-located module (e.g., a local workspace-files utils file) and update WorkspaceFilesSearchResultItem to import SEARCH_RESULT_ROW_HEIGHT and FileIcon from that new module; specifically, move or re-implement the constant and the FileIcon component used by WorkspaceFilesSearchResultItem into the new local module and adjust any other local v2 workspace components to use the new module instead of renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/* so the v2 workspace UI is self-contained.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/WorkspaceFiles.tsx (1)
120-121: Consider documenting or implementing the stub handlers.The
onNewFileandonNewFolderhandlers are empty stubs. If these are intentionally deferred, consider adding a TODO comment or removing the props if the toolbar doesn't need them yet.💡 Optional: Add TODO comments for clarity
<WorkspaceFilesToolbar isRefreshing={isRefreshing} onCollapseAll={fileTree.collapseAll} - onNewFile={() => {}} - onNewFolder={() => {}} + onNewFile={() => { + // TODO: Implement new file creation + }} + onNewFolder={() => { + // TODO: Implement new folder creation + }} onRefresh={() => void handleRefresh()} onSearchChange={setSearchTerm} searchTerm={searchTerm} />🤖 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/WorkspaceFiles/WorkspaceFiles.tsx around lines 120 - 121, The onNewFile and onNewFolder props passed into the WorkspaceFiles component are empty stubs; either implement their behavior in the WorkspaceFiles component (implement the handlers wired to the toolbar actions, e.g., createNewFile/createNewFolder methods) or explicitly mark them as intentionally unimplemented by adding a TODO comment and a brief explanation, or remove these props entirely if the toolbar does not require them; reference the onNewFile and onNewFolder prop usages in WorkspaceFiles to locate and update the code accordingly.packages/workspace-client/src/lib/workspaceFsEventRegistry.ts (1)
87-99: Consider guarding against double-unsubscribe.The
Math.max(0, state.bridgeCount - 1)on line 96 prevents negative counts, but the returned cleanup function can still be called multiple times. If a consumer accidentally calls the cleanup function twice, the second call would be a no-op for the count but might incorrectly triggerremoveSubscriptionIfInactive. Consider tracking whether cleanup was already called.💡 Optional: Guard against double-unsubscribe
export function retainWorkspaceFsBridge( client: WorkspaceClientContextValue, workspaceId: string, ): () => void { const state = getOrCreateSubscription(client, workspaceId); state.bridgeCount += 1; ensureTransport(state); + let released = false; return () => { + if (released) return; + released = true; state.bridgeCount = Math.max(0, state.bridgeCount - 1); removeSubscriptionIfInactive(state); }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workspace-client/src/lib/workspaceFsEventRegistry.ts` around lines 87 - 99, The cleanup returned by retainWorkspaceFsBridge can run multiple times and still call removeSubscriptionIfInactive even when bridgeCount didn't change; make the cleanup idempotent by capturing a local boolean (e.g., "called" or "released") in the closure, check it at the start of the returned function and return early if already invoked, and only decrement state.bridgeCount and call removeSubscriptionIfInactive the first time (still guarding decrement with Math.max to avoid negatives). This change should be implemented inside retainWorkspaceFsBridge so the closure around state and the new flag prevents double-unsubscribe effects.packages/host-service/src/trpc/router/filesystem/filesystem.ts (1)
6-21: Error detection via string matching is fragile.The check
error.message.startsWith("Workspace not found:")relies on a specific error message format. If the underlying service changes its error messages, this detection will silently fail. Consider using a custom error type or error code instead.♻️ Suggested: Use error codes or custom error types
If the workspace-fs service can provide structured errors:
function getFilesystemService(ctx: HostServiceContext, workspaceId: string) { try { return ctx.runtime.filesystem.getServiceForWorkspace(workspaceId); } catch (error) { - if ( - error instanceof Error && - error.message.startsWith("Workspace not found:") - ) { + if (error instanceof WorkspaceNotFoundError) { throw new TRPCError({ code: "NOT_FOUND", message: error.message, }); } throw error; } }Alternatively, if the service cannot be modified, consider adding a comment documenting this coupling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/filesystem/filesystem.ts` around lines 6 - 21, The current getFilesystemService uses fragile string matching on error.message to detect missing workspaces; change it to detect a structured error instead by checking for a well-known error property or type (e.g., an exported WorkspaceNotFoundError class or an error.code like "WORKSPACE_NOT_FOUND") when handling exceptions from ctx.runtime.filesystem.getServiceForWorkspace; if the filesystem service can be changed, update it to throw that custom error or set error.code, otherwise add a clear comment on this coupling and fall back to checking error.code before using error.message.startsWith to reduce brittleness (refer to getFilesystemService and ctx.runtime.filesystem.getServiceForWorkspace).
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx:
- Around line 89-95: The clear-search icon button in WorkspaceFilesToolbar lacks
an accessible name; update the button that calls handleClearSearch (the one
rendering LuX) to include an accessible label (e.g., add aria-label="Clear
search" or aria-labelledby referencing a visually hidden label) so screen
readers announce its purpose while preserving the icon-only visual; ensure the
attribute is added to the same button element that has
onClick={handleClearSearch}.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceFiles/hooks/useWorkspaceFileSearch/useWorkspaceFileSearch.ts:
- Around line 35-42: The search currently forwards directory matches to the UI
causing dead-end rows; update useWorkspaceFileSearch so it filters out directory
results before mapping (i.e., only include matches where match.kind !==
"directory") when constructing searchResults for the consumer (the returned
searchResults used by WorkspaceFilesSearchResultItem), ensuring you still handle
the optional chain on searchResults?.matches and return an empty array fallback;
alternatively, if directories must be shown, add a folder-specific activation
path in the consumer, but prefer filtering in useWorkspaceFileSearch to keep
WorkspaceFilesSearchResultItem behavior consistent.
In `@packages/workspace-client/src/hooks/useFileDocument/useFileDocument.ts`:
- Around line 277-280: In useFileDocument, do not map every readFileQuery.error
to { kind: "not-found" }; instead inspect the error from readFileQuery (in the
useMemo that builds state) and only return { kind: "not-found" } for explicit
not-found signals (e.g., HTTP 404 / specific error.code or error.status), and
for all other error cases return an error state (e.g., { kind: "error", error:
readFileQuery.error }) so callers of UseFileDocumentResult can differentiate
transport/auth/backend failures from genuine missing files; update the state
construction in useFileDocument to branch on readFileQuery.error details rather
than unconditionally returning not-found.
- Around line 129-163: The external-change handling reads currentPathRef.current
lazily causing races; modify fetchCurrentDiskContent to accept an absolutePath
parameter (use that instead of currentPathRef.current) and update
markExternalChange to take the path that triggered the external change, call
fetchCurrentDiskContent(path) and setConflict with that result; also ensure
callers that invoke markExternalChange pass the triggering path so reads are
scoped to the path that caused the event (referencing fetchCurrentDiskContent,
markExternalChange, currentPathRef, setConflict, setHasExternalChange).
In `@packages/workspace-client/src/hooks/useFileTree/useFileTree.ts`:
- Around line 194-205: The fetch logic in useFileTree suppresses concurrent
loads using stateRef.current.loadingDirectories but always clears
invalidatedDirectories when a fetch completes, which can erase invalidations
that arrived during the in-flight request; fix by snapshotting the invalidation
state when starting the fetch and only clearing invalidatedDirectories for
absolutePath on completion if it was not re-invalidated during the fetch (or
alternatively attach/compare a per-directory fetch token/version recorded at
fetch start and only clear if the current token/version matches), and if it was
re-invalidated, ensure you re-queue a reload for absolutePath; reference
stateRef.current.loadingDirectories, stateRef.current.loadedDirectories,
stateRef.current.invalidatedDirectories and the load/fetch function in
useFileTree.
- Around line 363-372: Move the overflow handling earlier so watcher overflow
always triggers a refresh: in the useFileTree handler, check event.kind ===
"overflow" and call refreshAll() (or return after) before computing
relevantPaths or filtering by isWithinPath(rootPath, ...). This ensures
refreshAll() runs even when event.absolutePath/event.oldAbsolutePath are
undefined; update the logic around relevantPaths, isWithinPath, and refreshAll
in useFileTree to reflect this ordering.
In `@plans/host-service-diff-plan.md`:
- Around line 294-298: WorkspaceDiffStatusSnapshot exposes an untracked list but
the file-diff viewer contract (getFileDiff and WorkspaceFileDiffPayload) has no
way to request or represent previews for untracked files; either add an explicit
"untracked" mode to the viewer contract or ensure untracked entries are excluded
from viewer requests. Update getFileDiff to accept a mode/enum that includes
"untracked", and extend WorkspaceFileDiffPayload to carry an untracked preview
shape (or alternatively document and enforce filtering of
WorkspaceDiffStatusSnapshot.untracked before calling getFileDiff). Reference the
symbols WorkspaceDiffStatusSnapshot, getFileDiff, and WorkspaceFileDiffPayload
when making the change so callers and UI rendering logic can request/handle
untracked previews consistently.
- Around line 129-151: The cache is currently keyed only by workspaceId but the
APIs getStatusSnapshot, getFileDiff, getWorkspaceSummary and
getWorkspaceSummaries accept a defaultBranch (and getWorkspaceSummaries
currently drops it), which will mix results for different bases; update the
caching strategy to include defaultBranch in the cache key (e.g., use
workspaceId + ":" + defaultBranch) for all caches that store workspace
snapshots, file diffs and summaries, propagate defaultBranch through
getWorkspaceSummaries so it isn’t dropped, and/or make defaultBranch
non-overridable by removing the parameter; ensure functions named
getStatusSnapshot, getFileDiff, getWorkspaceSummary and getWorkspaceSummaries
use the new key consistently.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx`:
- Around line 39-53: V2OpenInMenuButton is a redundant passthrough that only
returns V2OpenInMenuButtonInner with the same props; remove the wrapper by
deleting the V2OpenInMenuButton function and instead export
V2OpenInMenuButtonInner under the name V2OpenInMenuButton (or re-export it:
export { V2OpenInMenuButtonInner as V2OpenInMenuButton }), and update any
imports that reference V2OpenInMenuButton to continue working with the single
exported component.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesSearchResultItem/WorkspaceFilesSearchResultItem.tsx:
- Around line 1-3: The WorkspaceFilesSearchResultItem component currently
imports SEARCH_RESULT_ROW_HEIGHT and FileIcon from the legacy FilesView tree;
extract those shared primitives into a new co-located module (e.g., a local
workspace-files utils file) and update WorkspaceFilesSearchResultItem to import
SEARCH_RESULT_ROW_HEIGHT and FileIcon from that new module; specifically, move
or re-implement the constant and the FileIcon component used by
WorkspaceFilesSearchResultItem into the new local module and adjust any other
local v2 workspace components to use the new module instead of
renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/* so the
v2 workspace UI is self-contained.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx:
- Line 12: The toolbar imports SEARCH_DEBOUNCE_MS from a distant sidebar module
causing unwanted coupling; create a colocated constant (e.g., in the same folder
as WorkspaceFilesToolbar or a nearby constants.ts) and replace the external
import in WorkspaceFilesToolbar.tsx with the new local constant, updating any
references to SEARCH_DEBOUNCE_MS to use the colocated symbol so the workspace
files UI no longer depends on the legacy sidebar module.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceFiles/WorkspaceFiles.tsx:
- Around line 120-121: The onNewFile and onNewFolder props passed into the
WorkspaceFiles component are empty stubs; either implement their behavior in the
WorkspaceFiles component (implement the handlers wired to the toolbar actions,
e.g., createNewFile/createNewFolder methods) or explicitly mark them as
intentionally unimplemented by adding a TODO comment and a brief explanation, or
remove these props entirely if the toolbar does not require them; reference the
onNewFile and onNewFolder prop usages in WorkspaceFiles to locate and update the
code accordingly.
In `@packages/host-service/src/trpc/router/filesystem/filesystem.ts`:
- Around line 6-21: The current getFilesystemService uses fragile string
matching on error.message to detect missing workspaces; change it to detect a
structured error instead by checking for a well-known error property or type
(e.g., an exported WorkspaceNotFoundError class or an error.code like
"WORKSPACE_NOT_FOUND") when handling exceptions from
ctx.runtime.filesystem.getServiceForWorkspace; if the filesystem service can be
changed, update it to throw that custom error or set error.code, otherwise add a
clear comment on this coupling and fall back to checking error.code before using
error.message.startsWith to reduce brittleness (refer to getFilesystemService
and ctx.runtime.filesystem.getServiceForWorkspace).
In `@packages/workspace-client/src/lib/workspaceFsEventRegistry.ts`:
- Around line 87-99: The cleanup returned by retainWorkspaceFsBridge can run
multiple times and still call removeSubscriptionIfInactive even when bridgeCount
didn't change; make the cleanup idempotent by capturing a local boolean (e.g.,
"called" or "released") in the closure, check it at the start of the returned
function and return early if already invoked, and only decrement
state.bridgeCount and call removeSubscriptionIfInactive the first time (still
guarding decrement with Math.max to avoid negatives). This change should be
implemented inside retainWorkspaceFsBridge so the closure around state and the
new flag prevents double-unsubscribe effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a1265a5-172c-4dc1-adae-09b7dbe13329
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
apps/desktop/package.jsonapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/lib/workspace-trpc.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/hooks/useCurrentWorkspaceForTopBar/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/hooks/useCurrentWorkspaceForTopBar/useCurrentWorkspaceForTopBar.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/WorkspaceFiles.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilePreview/WorkspaceFilePreview.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilePreview/components/WorkspaceFilePreviewContent/WorkspaceFilePreviewContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilePreview/components/WorkspaceFilePreviewContent/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilePreview/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesSearchResultItem/WorkspaceFilesSearchResultItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesSearchResultItem/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesToolbar/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesTreeItem/WorkspaceFilesTreeItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/components/WorkspaceFilesTreeItem/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/hooks/useWorkspaceFileSearch/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/hooks/useWorkspaceFileSearch/useWorkspaceFileSearch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceFiles/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/providers/WorkspaceTrpcProvider/WorkspaceTrpcProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/screens/main/components/CommandPalette/useCommandPalette.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxpackages/host-service/package.jsonpackages/host-service/src/app.tspackages/host-service/src/filesystem/events.tspackages/host-service/src/filesystem/index.tspackages/host-service/src/index.tspackages/host-service/src/runtime/filesystem/filesystem.tspackages/host-service/src/runtime/filesystem/index.tspackages/host-service/src/trpc/index.tspackages/host-service/src/trpc/router/filesystem/filesystem.tspackages/host-service/src/trpc/router/filesystem/index.tspackages/host-service/src/trpc/router/router.tspackages/host-service/src/types.tspackages/workspace-client/package.jsonpackages/workspace-client/src/hooks/useFileDocument/index.tspackages/workspace-client/src/hooks/useFileDocument/useFileDocument.tspackages/workspace-client/src/hooks/useFileTree/index.tspackages/workspace-client/src/hooks/useFileTree/useFileTree.tspackages/workspace-client/src/hooks/useWorkspaceFsEventBridge/index.tspackages/workspace-client/src/hooks/useWorkspaceFsEventBridge/useWorkspaceFsEventBridge.tspackages/workspace-client/src/hooks/useWorkspaceFsEvents/index.tspackages/workspace-client/src/hooks/useWorkspaceFsEvents/useWorkspaceFsEvents.tspackages/workspace-client/src/index.tspackages/workspace-client/src/lib/workspaceFsEventRegistry.tspackages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsxpackages/workspace-client/src/providers/WorkspaceClientProvider/index.tspackages/workspace-client/src/workspace-trpc.tspackages/workspace-client/tsconfig.jsonpackages/workspace-fs/src/types.tspackages/workspace-fs/src/watch.tsplans/host-service-diff-plan.mdplans/host-service-workspace-filesystem-plan.md
Comment on lines
+89
to
+95
| <button | ||
| className="absolute top-1/2 right-1 -translate-y-1/2 rounded p-0.5 text-muted-foreground transition-colors hover:bg-muted-foreground/20 hover:text-foreground" | ||
| onClick={handleClearSearch} | ||
| type="button" | ||
| > | ||
| <LuX className="size-3.5" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the clear-search icon button.
The icon-only control currently has no accessible label, so assistive tech won’t announce its purpose.
💡 Suggested fix
{localSearchTerm ? (
<button
+ aria-label="Clear search"
className="absolute top-1/2 right-1 -translate-y-1/2 rounded p-0.5 text-muted-foreground transition-colors hover:bg-muted-foreground/20 hover:text-foreground"
onClick={handleClearSearch}
type="button"
>📝 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.
Suggested change
| <button | |
| className="absolute top-1/2 right-1 -translate-y-1/2 rounded p-0.5 text-muted-foreground transition-colors hover:bg-muted-foreground/20 hover:text-foreground" | |
| onClick={handleClearSearch} | |
| type="button" | |
| > | |
| <LuX className="size-3.5" /> | |
| </button> | |
| <button | |
| aria-label="Clear search" | |
| className="absolute top-1/2 right-1 -translate-y-1/2 rounded p-0.5 text-muted-foreground transition-colors hover:bg-muted-foreground/20 hover:text-foreground" | |
| onClick={handleClearSearch} | |
| type="button" | |
| > | |
| <LuX className="size-3.5" /> | |
| </button> |
🤖 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/WorkspaceFiles/components/WorkspaceFilesToolbar/WorkspaceFilesToolbar.tsx
around lines 89 - 95, The clear-search icon button in WorkspaceFilesToolbar
lacks an accessible name; update the button that calls handleClearSearch (the
one rendering LuX) to include an accessible label (e.g., add aria-label="Clear
search" or aria-labelledby referencing a visually hidden label) so screen
readers announce its purpose while preserving the icon-only visual; ensure the
attribute is added to the same button element that has
onClick={handleClearSearch}.
Comment on lines
+35
to
+42
| return { | ||
| searchResults: | ||
| searchResults?.matches.map((match) => ({ | ||
| absolutePath: match.absolutePath, | ||
| isDirectory: match.kind === "directory", | ||
| name: match.name, | ||
| relativePath: match.relativePath, | ||
| })) ?? [], |
Contributor
There was a problem hiding this comment.
Directory matches currently become dead-end search rows.
This hook forwards directory results, but WorkspaceFilesSearchResultItem only activates non-directories. The list can therefore show rows that look selectable but have no effect. Either filter directories here or add a folder-specific activation path before exposing them.
🤖 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/WorkspaceFiles/hooks/useWorkspaceFileSearch/useWorkspaceFileSearch.ts
around lines 35 - 42, The search currently forwards directory matches to the UI
causing dead-end rows; update useWorkspaceFileSearch so it filters out directory
results before mapping (i.e., only include matches where match.kind !==
"directory") when constructing searchResults for the consumer (the returned
searchResults used by WorkspaceFilesSearchResultItem), ensuring you still handle
the optional chain on searchResults?.matches and return an empty array fallback;
alternatively, if directories must be shown, add a folder-specific activation
path in the consumer, but prefer filtering in useWorkspaceFileSearch to keep
WorkspaceFilesSearchResultItem behavior consistent.
Comment on lines
+129
to
+163
| const fetchCurrentDiskContent = useCallback(async (): Promise< | ||
| string | null | ||
| > => { | ||
| try { | ||
| const result = await utils.filesystem.readFile.fetch({ | ||
| workspaceId, | ||
| absolutePath: currentPathRef.current, | ||
| encoding: "utf-8", | ||
| maxBytes, | ||
| }); | ||
|
|
||
| if ( | ||
| result.kind !== "text" || | ||
| result.exceededLimit || | ||
| isBinaryText(result.content) | ||
| ) { | ||
| return null; | ||
| } | ||
|
|
||
| return result.content; | ||
| } catch { | ||
| return null; | ||
| } | ||
| }, [maxBytes, utils.filesystem.readFile, workspaceId]); | ||
|
|
||
| const markExternalChange = useCallback(async (): Promise<void> => { | ||
| setHasExternalChange(true); | ||
| if (mode === "bytes") { | ||
| setConflict({ diskContent: null }); | ||
| return; | ||
| } | ||
|
|
||
| const diskContent = await fetchCurrentDiskContent(); | ||
| setConflict({ diskContent }); | ||
| }, [fetchCurrentDiskContent, mode]); |
Contributor
There was a problem hiding this comment.
Scope external-change reads to the path that triggered them.
Line 186 updates the tracked path, but markExternalChange() still reads currentPathRef.current from the previous render. That means a dirty external rename can fetch conflict text from the old path, and the late async result can still overwrite conflict after the active document changes.
💡 Suggested fix
- const fetchCurrentDiskContent = useCallback(async (): Promise<
- string | null
- > => {
- try {
- const result = await utils.filesystem.readFile.fetch({
- workspaceId,
- absolutePath: currentPathRef.current,
- encoding: "utf-8",
- maxBytes,
- });
+ const fetchCurrentDiskContent = useCallback(
+ async (path = currentPathRef.current): Promise<string | null> => {
+ try {
+ const result = await utils.filesystem.readFile.fetch({
+ workspaceId,
+ absolutePath: path,
+ encoding: "utf-8",
+ maxBytes,
+ });
- if (
- result.kind !== "text" ||
- result.exceededLimit ||
- isBinaryText(result.content)
- ) {
- return null;
- }
+ if (
+ result.kind !== "text" ||
+ result.exceededLimit ||
+ isBinaryText(result.content)
+ ) {
+ return null;
+ }
- return result.content;
- } catch {
- return null;
- }
- }, [maxBytes, utils.filesystem.readFile, workspaceId]);
+ return result.content;
+ } catch {
+ return null;
+ }
+ },
+ [maxBytes, utils.filesystem.readFile, workspaceId],
+ );
- const markExternalChange = useCallback(async (): Promise<void> => {
- setHasExternalChange(true);
- if (mode === "bytes") {
- setConflict({ diskContent: null });
- return;
- }
+ const markExternalChange = useCallback(
+ async (pathOverride = currentPathRef.current): Promise<void> => {
+ setHasExternalChange(true);
+ if (mode === "bytes") {
+ if (currentPathRef.current === pathOverride) {
+ setConflict({ diskContent: null });
+ }
+ return;
+ }
- const diskContent = await fetchCurrentDiskContent();
- setConflict({ diskContent });
- }, [fetchCurrentDiskContent, mode]);
+ const diskContent = await fetchCurrentDiskContent(pathOverride);
+ if (currentPathRef.current === pathOverride) {
+ setConflict({ diskContent });
+ }
+ },
+ [fetchCurrentDiskContent, mode],
+ );
...
if (event.kind === "rename" && event.oldAbsolutePath === path) {
+ currentPathRef.current = event.absolutePath;
setCurrentPath(event.absolutePath);
if (hasLocalChangesRef.current) {
- void markExternalChange();
+ void markExternalChange(event.absolutePath);
return;
}Also applies to: 185-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workspace-client/src/hooks/useFileDocument/useFileDocument.ts`
around lines 129 - 163, The external-change handling reads
currentPathRef.current lazily causing races; modify fetchCurrentDiskContent to
accept an absolutePath parameter (use that instead of currentPathRef.current)
and update markExternalChange to take the path that triggered the external
change, call fetchCurrentDiskContent(path) and setConflict with that result;
also ensure callers that invoke markExternalChange pass the triggering path so
reads are scoped to the path that caused the event (referencing
fetchCurrentDiskContent, markExternalChange, currentPathRef, setConflict,
setHasExternalChange).
Comment on lines
+277
to
+280
| const state = useMemo<UseFileDocumentResult["state"]>(() => { | ||
| if (readFileQuery.error) { | ||
| return { kind: "not-found" }; | ||
| } |
Contributor
There was a problem hiding this comment.
Differentiate missing files from transport failures.
Line 278 turns every readFileQuery.error into { kind: "not-found" }. Network/auth/backend failures will therefore render as a fake missing-file state, which hides the real failure mode and makes the preview misleading whenever the host service errors for any reason other than “file does not exist”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workspace-client/src/hooks/useFileDocument/useFileDocument.ts`
around lines 277 - 280, In useFileDocument, do not map every readFileQuery.error
to { kind: "not-found" }; instead inspect the error from readFileQuery (in the
useMemo that builds state) and only return { kind: "not-found" } for explicit
not-found signals (e.g., HTTP 404 / specific error.code or error.status), and
for all other error cases return an error state (e.g., { kind: "error", error:
readFileQuery.error }) so callers of UseFileDocumentResult can differentiate
transport/auth/backend failures from genuine missing files; update the state
construction in useFileDocument to branch on readFileQuery.error details rather
than unconditionally returning not-found.
Comment on lines
+194
to
+205
| const currentState = stateRef.current; | ||
| if (currentState.loadingDirectories.has(absolutePath)) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| !force && | ||
| currentState.loadedDirectories.has(absolutePath) && | ||
| !currentState.invalidatedDirectories.has(absolutePath) | ||
| ) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't lose invalidations that arrive during a directory fetch.
Lines 195-197 suppress a second load while the first request is in flight, but Lines 229-230 always clear invalidatedDirectories when that request completes. If a filesystem event marks the directory dirty in between, the invalidation from Lines 430-434 gets erased and no follow-up reload is queued, leaving stale children until another event happens.
Also applies to: 222-230, 424-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workspace-client/src/hooks/useFileTree/useFileTree.ts` around lines
194 - 205, The fetch logic in useFileTree suppresses concurrent loads using
stateRef.current.loadingDirectories but always clears invalidatedDirectories
when a fetch completes, which can erase invalidations that arrived during the
in-flight request; fix by snapshotting the invalidation state when starting the
fetch and only clearing invalidatedDirectories for absolutePath on completion if
it was not re-invalidated during the fetch (or alternatively attach/compare a
per-directory fetch token/version recorded at fetch start and only clear if the
current token/version matches), and if it was re-invalidated, ensure you
re-queue a reload for absolutePath; reference
stateRef.current.loadingDirectories, stateRef.current.loadedDirectories,
stateRef.current.invalidatedDirectories and the load/fetch function in
useFileTree.
Comment on lines
+363
to
+372
| const relevantPaths = [event.absolutePath, event.oldAbsolutePath].filter( | ||
| (path): path is string => Boolean(path), | ||
| ); | ||
| if (!relevantPaths.some((path) => isWithinPath(rootPath, path))) { | ||
| return; | ||
| } | ||
|
|
||
| if (event.kind === "overflow") { | ||
| void refreshAll(); | ||
| return; |
Contributor
There was a problem hiding this comment.
Handle overflow before filtering by path.
The relevance filter on Lines 363-368 depends on absolutePath/oldAbsolutePath. If an overflow event arrives without those fields, the handler returns before Line 370 and refreshAll() never runs, so the tree can stay stale after a watcher overflow.
💡 Suggested fix
- const relevantPaths = [event.absolutePath, event.oldAbsolutePath].filter(
- (path): path is string => Boolean(path),
- );
- if (!relevantPaths.some((path) => isWithinPath(rootPath, path))) {
- return;
- }
-
if (event.kind === "overflow") {
void refreshAll();
return;
}
+
+ const relevantPaths = [event.absolutePath, event.oldAbsolutePath].filter(
+ (path): path is string => Boolean(path),
+ );
+ if (!relevantPaths.some((path) => isWithinPath(rootPath, path))) {
+ return;
+ }📝 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.
Suggested change
| const relevantPaths = [event.absolutePath, event.oldAbsolutePath].filter( | |
| (path): path is string => Boolean(path), | |
| ); | |
| if (!relevantPaths.some((path) => isWithinPath(rootPath, path))) { | |
| return; | |
| } | |
| if (event.kind === "overflow") { | |
| void refreshAll(); | |
| return; | |
| if (event.kind === "overflow") { | |
| void refreshAll(); | |
| return; | |
| } | |
| const relevantPaths = [event.absolutePath, event.oldAbsolutePath].filter( | |
| (path): path is string => Boolean(path), | |
| ); | |
| if (!relevantPaths.some((path) => isWithinPath(rootPath, path))) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workspace-client/src/hooks/useFileTree/useFileTree.ts` around lines
363 - 372, Move the overflow handling earlier so watcher overflow always
triggers a refresh: in the useFileTree handler, check event.kind === "overflow"
and call refreshAll() (or return after) before computing relevantPaths or
filtering by isWithinPath(rootPath, ...). This ensures refreshAll() runs even
when event.absolutePath/event.oldAbsolutePath are undefined; update the logic
around relevantPaths, isWithinPath, and refreshAll in useFileTree to reflect
this ordering.
Comment on lines
+129
to
+151
| getStatusSnapshot(input: { | ||
| workspaceId: string | ||
| defaultBranch?: string | ||
| force?: boolean | ||
| }): Promise<WorkspaceDiffStatusSnapshot> | ||
|
|
||
| getFileDiff(input: { | ||
| workspaceId: string | ||
| absolutePath: string | ||
| oldAbsolutePath?: string | ||
| category: "against-base" | "committed" | "staged" | "unstaged" | ||
| commitHash?: string | ||
| defaultBranch?: string | ||
| }): Promise<WorkspaceFileDiffPayload> | ||
|
|
||
| getWorkspaceSummary(input: { | ||
| workspaceId: string | ||
| defaultBranch?: string | ||
| }): Promise<WorkspaceDiffSummary> | ||
|
|
||
| getWorkspaceSummaries(input: { | ||
| workspaceIds: string[] | ||
| }): Promise<Array<{ workspaceId: string; summary: WorkspaceDiffSummary | null }>> |
Contributor
There was a problem hiding this comment.
Key the cache by defaultBranch, or make it non-overridable.
The public APIs let callers vary defaultBranch, but the cache model stores a single entry per workspace with one defaultBranch, and getWorkspaceSummaries drops the parameter entirely. That will mix against-base and summary data as soon as two consumers resolve the same workspace against different bases.
Also applies to: 166-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plans/host-service-diff-plan.md` around lines 129 - 151, The cache is
currently keyed only by workspaceId but the APIs getStatusSnapshot, getFileDiff,
getWorkspaceSummary and getWorkspaceSummaries accept a defaultBranch (and
getWorkspaceSummaries currently drops it), which will mix results for different
bases; update the caching strategy to include defaultBranch in the cache key
(e.g., use workspaceId + ":" + defaultBranch) for all caches that store
workspace snapshots, file diffs and summaries, propagate defaultBranch through
getWorkspaceSummaries so it isn’t dropped, and/or make defaultBranch
non-overridable by removing the parameter; ensure functions named
getStatusSnapshot, getFileDiff, getWorkspaceSummary and getWorkspaceSummaries
use the new key consistently.
Comment on lines
+294
to
+298
| againstBase: ChangedFile[] | ||
| commits: CommitInfo[] | ||
| staged: ChangedFile[] | ||
| unstaged: ChangedFile[] | ||
| untracked: ChangedFile[] |
Contributor
There was a problem hiding this comment.
The diff contract still has no way to load an untracked file preview.
WorkspaceDiffStatusSnapshot includes untracked, and the client section says the changes pane renders that section, but getFileDiff / WorkspaceFileDiffPayload do not admit an untracked category. Either add an untracked mode or explicitly scope untracked rows out of the viewer contract.
Also applies to: 317-328, 537-544, 714-718
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plans/host-service-diff-plan.md` around lines 294 - 298,
WorkspaceDiffStatusSnapshot exposes an untracked list but the file-diff viewer
contract (getFileDiff and WorkspaceFileDiffPayload) has no way to request or
represent previews for untracked files; either add an explicit "untracked" mode
to the viewer contract or ensure untracked entries are excluded from viewer
requests. Update getFileDiff to accept a mode/enum that includes "untracked",
and extend WorkspaceFileDiffPayload to carry an untracked preview shape (or
alternatively document and enforce filtering of
WorkspaceDiffStatusSnapshot.untracked before calling getFileDiff). Reference the
symbols WorkspaceDiffStatusSnapshot, getFileDiff, and WorkspaceFileDiffPayload
when making the change so callers and UI rendering logic can request/handle
untracked previews consistently.
Contributor
There was a problem hiding this comment.
1 issue found across 15 files (changes from recent commits).
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/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:135">
P1: Cross-workspace quick-open results no longer open the selected file; the new branch navigates to the target workspace and returns before applying `filePath`/`files` view state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| navigate: _routerNavigate, | ||
| }) => { | ||
| close(); | ||
| if (targetWorkspaceId !== workspaceId) { |
Contributor
There was a problem hiding this comment.
P1: Cross-workspace quick-open results no longer open the selected file; the new branch navigates to the target workspace and returns before applying filePath/files view state.
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/page.tsx, line 135:
<comment>Cross-workspace quick-open results no longer open the selected file; the new branch navigates to the target workspace and returns before applying `filePath`/`files` view state.</comment>
<file context>
@@ -137,38 +129,24 @@ function V2WorkspaceContent({
- view: "files",
- }),
- });
+ if (targetWorkspaceId !== workspaceId) {
+ void navigate({
+ to: "/v2-workspace/$workspaceId",
</file context>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceSearchBarTrigger/V2WorkspaceSearchBarTrigger.tsx (1)
3-4: Use tsconfig alias imports instead of deep relative paths.Line 3 uses a long relative import chain, which is brittle during folder moves. Prefer the configured path alias for desktop renderer modules.
As per coding guidelines
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when possible.🤖 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/components/TopBar/components/V2WorkspaceSearchBarTrigger/V2WorkspaceSearchBarTrigger.tsx` around lines 3 - 4, Replace the deep relative import for useCollections with the tsconfig path alias for desktop renderer modules: change the import of useCollections from "../../../../../providers/CollectionsProvider" to the configured alias import (the renderer providers alias declared in tsconfig.json, e.g., import { useCollections } from "renderer/providers/CollectionsProvider" or the equivalent `@alias`) so the file V2WorkspaceSearchBarTrigger.tsx uses the aliased module path; do the same for any similar deep-relative imports in this component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceSearchBarTrigger/V2WorkspaceSearchBarTrigger.tsx`:
- Around line 3-4: Replace the deep relative import for useCollections with the
tsconfig path alias for desktop renderer modules: change the import of
useCollections from "../../../../../providers/CollectionsProvider" to the
configured alias import (the renderer providers alias declared in tsconfig.json,
e.g., import { useCollections } from "renderer/providers/CollectionsProvider" or
the equivalent `@alias`) so the file V2WorkspaceSearchBarTrigger.tsx uses the
aliased module path; do the same for any similar deep-relative imports in this
component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ded95c6-06db-4f9d-bf4c-72c09b565c1e
📒 Files selected for processing (13)
apps/desktop/src/renderer/lib/workspace-trpc.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceSearchBarTrigger/V2WorkspaceSearchBarTrigger.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceSearchBarTrigger/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/ChatPaneInterface.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/components/ChatInputFooter/components/SlashCommandPreview/SlashCommandPreview.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/hooks/useSlashCommandExecutor/useSlashCommandExecutor.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatController/useWorkspaceChatController.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatDisplay/useWorkspaceChatDisplay.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/lib/workspace-trpc.ts
✅ Files skipped from review due to trivial changes (4)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatController/useWorkspaceChatController.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatDisplay/useWorkspaceChatDisplay.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceSearchBarTrigger/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsx
This was referenced Mar 30, 2026
8 tasks
This was referenced Apr 10, 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
Testing
Summary by cubic
Scopes the v2 “Open in App” button and hotkey to local workspaces only, defaulting to
cursorwhen no project app is set. Adds@superset/workspace-clientand host-service filesystem to power a Files tab (tree, search, preview) with live updates in v2 workspaces.New Features
@superset/workspace-clientwith hooks (useFileTree,useFileDocument) and a provider for tRPC + WebSocket FS events; desktop now uses this instead of a local client.WorkspaceFilesystemManager+ WS events endpoint).V2WorkspaceOpenInButtonandV2WorkspaceSearchBarTrigger; open-in resolves the localworktreePath.Bug Fixes
cursorwhen no project default is set across entry points (button, menu, empty view, workspace page).Written for commit aaa0738. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes