feat(desktop): support parent directory navigation and external file paths#2057
Closed
Kitenite wants to merge 6 commits into
Closed
feat(desktop): support parent directory navigation and external file paths#2057Kitenite wants to merge 6 commits into
Kitenite wants to merge 6 commits into
Conversation
…paths in file browser - Add parent directory navigation button (arrow up) in file browser toolbar - Add breadcrumb bar showing current path when browsing outside worktree - Add home button to return to workspace root - Add filesystem.readFile endpoint for reading files by absolute path - Allow file viewer to open files outside the worktree via absolute paths - Remove toast warning when clicking external file links in terminal
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new public tRPC endpoint Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Renderer UI
participant Hook as useFileContent
participant Router as tRPC filesystem.readFile
participant FS as File System
User->>UI: open/click absolute file path
UI->>Hook: request file for path (absolute or relative)
alt path is outside worktree
Hook->>Router: readFile({ filePath: absolute })
Router->>FS: open & read (max 2MB, probe first 8192 bytes for binary)
FS-->>Router: content bytes or error
Router-->>Hook: {ok:true, content, byteLength, truncated} or {ok:false, reason}
else path is inside worktree
Hook->>Router: readWorkingFile(relativePath) (existing worktree flow)
Router->>FS: worktree read (server-side)
FS-->>Router: content or error
Router-->>Hook: worktree response
end
Hook-->>UI: effectiveRawFileData / loading state
UI->>UI: render content or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 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/lib/trpc/routers/filesystem/index.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/filesystem/index.ts:982">
P2: Duplicate binary detection logic — reuse the existing `isBinaryContent` helper defined in this same file (line 292) instead of inlining the check.
(Based on your team's feedback about avoiding duplicating business logic in multiple components.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/filesystem/index.ts:995">
P2: Catch-all block maps every error to `"not-found"`, hiding the real cause (e.g. `EACCES`, `EISDIR`). At minimum, check `err.code === 'ENOENT'` before falling back, and log non-ENOENT errors so failures are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts:34">
P2: Unix-only absolute path detection — `startsWith("/")` won't match Windows paths like `C:\...`, silently breaking external file support on Windows. Consider using Node's `path.isAbsolute(filePath)` which handles both platforms.
(Based on your team's feedback about using cross-platform path utilities instead of manual string operations.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+982
to
+986
| const checkLength = Math.min(buffer.length, BINARY_CHECK_SIZE); | ||
| for (let i = 0; i < checkLength; i++) { | ||
| if (buffer[i] === 0) { | ||
| return { ok: false, reason: "binary" }; | ||
| } |
Contributor
There was a problem hiding this comment.
P2: Duplicate binary detection logic — reuse the existing isBinaryContent helper defined in this same file (line 292) instead of inlining the check.
(Based on your team's feedback about avoiding duplicating business logic in multiple components.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/filesystem/index.ts, line 982:
<comment>Duplicate binary detection logic — reuse the existing `isBinaryContent` helper defined in this same file (line 292) instead of inlining the check.
(Based on your team's feedback about avoiding duplicating business logic in multiple components.) </comment>
<file context>
@@ -944,6 +944,60 @@ export const createFilesystemRouter = () => {
+ const buffer = await fs.readFile(input.filePath);
+
+ // Binary detection (same as readWorkingFile)
+ const checkLength = Math.min(buffer.length, BINARY_CHECK_SIZE);
+ for (let i = 0; i < checkLength; i++) {
+ if (buffer[i] === 0) {
</file context>
Suggested change
| const checkLength = Math.min(buffer.length, BINARY_CHECK_SIZE); | |
| for (let i = 0; i < checkLength; i++) { | |
| if (buffer[i] === 0) { | |
| return { ok: false, reason: "binary" }; | |
| } | |
| if (isBinaryContent(buffer)) { | |
| return { ok: false, reason: "binary" }; | |
| } |
Comment on lines
+995
to
+996
| } catch { | ||
| return { ok: false, reason: "not-found" }; |
Contributor
There was a problem hiding this comment.
P2: Catch-all block maps every error to "not-found", hiding the real cause (e.g. EACCES, EISDIR). At minimum, check err.code === 'ENOENT' before falling back, and log non-ENOENT errors so failures are diagnosable.
(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 apps/desktop/src/lib/trpc/routers/filesystem/index.ts, line 995:
<comment>Catch-all block maps every error to `"not-found"`, hiding the real cause (e.g. `EACCES`, `EISDIR`). At minimum, check `err.code === 'ENOENT'` before falling back, and log non-ENOENT errors so failures are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -944,6 +944,60 @@ export const createFilesystemRouter = () => {
+ truncated: false,
+ byteLength: buffer.length,
+ };
+ } catch {
+ return { ok: false, reason: "not-found" };
+ }
</file context>
Suggested change
| } catch { | |
| return { ok: false, reason: "not-found" }; | |
| } catch (err) { | |
| const code = (err as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| console.error("[filesystem/readFile] Failed:", { path: input.filePath, error: err }); | |
| } | |
| return { ok: false, reason: "not-found" }; |
![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: 5
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/filesystem/index.ts (1)
981-987: Reuse the existing binary detection helper instead of duplicating the loop.This block duplicates
isBinaryContent(buffer), which increases drift risk.Suggested refactor
- // Binary detection (same as readWorkingFile) - const checkLength = Math.min(buffer.length, BINARY_CHECK_SIZE); - for (let i = 0; i < checkLength; i++) { - if (buffer[i] === 0) { - return { ok: false, reason: "binary" }; - } - } + if (isBinaryContent(buffer)) { + return { ok: false, reason: "binary" }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 981 - 987, The duplicated binary detection loop should be replaced with the existing helper: remove the manual loop in the filesystem handler and call isBinaryContent(buffer) (same helper used by readWorkingFile); if it returns true, return { ok: false, reason: "binary" } as before. Ensure the helper is in scope (import or reference) and that the BINARY_CHECK_SIZE behavior is preserved by relying on isBinaryContent(buffer) rather than duplicating the iteration.
🤖 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/lib/trpc/routers/filesystem/index.ts`:
- Around line 951-999: The publicProcedure readFile currently reads any host
path (readFile) directly, bypassing the project's path/worktree validation and
opening an unrestricted IPC file-read. Fix by routing this RPC through the same
validated codepath used elsewhere: call the existing secure read logic (e.g.,
reuse changes.readWorkingFile or the secureFs path validation helper) before
performing fs.stat/fs.readFile, ensuring the input.filePath is validated as
inside the worktree/root and normalized; preserve the original return shape
(ok:true/false, reasons "not-found"|"too-large"|"binary") and retain the
BINARY_CHECK_SIZE and MAX_FILE_SIZE checks but only after the path validation
succeeds. Ensure no direct fs.readFile on arbitrary input.filePath remains in
readFile.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts`:
- Around line 33-34: The Unix-only check using filePath.startsWith("/") (stored
in isAbsolutePath inside useFileContent) fails on Windows; replace all manual
startsWith checks in useFileContent (including the occurrences around
isAbsolutePath and the other similar checks later) with a platform-aware check
by importing and using Node's path.isAbsolute(filePath) (or equivalent
cross-platform utility) so drive-letter and UNC paths are detected as absolute
and routed through the external-read path; update any logic that depends on
isAbsolutePath to use the new isAbsolute result.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileTreeToolbar/FileTreeToolbar.tsx`:
- Around line 115-123: The new icon-only navigation buttons in FileTreeToolbar
lack accessible names; add explicit aria-labels (or aria-labelledby) to the
Button components so screen readers announce their actions. Update the Button
using onNavigateToParent to include aria-label="Navigate to parent folder" (or
similar), and do the same for the corresponding child/forward button (the Button
using onNavigateToChild or equivalent) with aria-label="Navigate to child
folder" — keep the TooltipTrigger/Tooltip intact but ensure the Button has the
accessible name. Ensure labels are concise and descriptive and use the same
unique component/function names (FileTreeToolbar, onNavigateToParent,
onNavigateToChild) to locate the elements.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx`:
- Around line 133-138: The parent navigation uses POSIX-only regex and fails on
Windows paths; update navigateToParent (and the breadcrumb parsing logic around
the other block) to use Node's path utilities or a platform-agnostic approach:
normalize the path, detect and preserve Windows drive roots (e.g., "C:\\"), use
path.dirname (or path.win32/path.posix based on the path) or a regex that
handles both separators ([\\/]) to compute the parent, and then call
setBrowseRoot with the computed parent; ensure the root-case returns the
original root (e.g., "/" or "C:\") rather than an empty string.
- Around line 355-366: The FileTreeToolbar (and other places noted) currently
enables mutating actions even when browsing outside the workspace; update the
component props so mutating handlers are disabled when isOutsideWorktree is
true: for FileTreeToolbar pass undefined (or a no-op and set a disabled flag)
for onNewFile, onNewFolder, and any rename/delete handlers (e.g., the props
passed where you currently call handleNewFile, handleNewFolder, handleRename,
handleDelete), and likewise ensure onNavigateHome remains the only navigation
action exposed; alternatively add a guard inside the handlers (handleNewFile,
handleNewFolder, handleRename, handleDelete) to early-return when
isOutsideWorktree is true so no filesystem mutations are performed outside the
workspace.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts`:
- Around line 981-987: The duplicated binary detection loop should be replaced
with the existing helper: remove the manual loop in the filesystem handler and
call isBinaryContent(buffer) (same helper used by readWorkingFile); if it
returns true, return { ok: false, reason: "binary" } as before. Ensure the
helper is in scope (import or reference) and that the BINARY_CHECK_SIZE behavior
is preserved by relying on isBinaryContent(buffer) rather than duplicating the
iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e525f30e-e408-41ce-8324-dc84dafe87f0
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/filesystem/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileTreeToolbar/FileTreeToolbar.tsx
Comment on lines
+951
to
+999
| readFile: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| filePath: z.string(), | ||
| }), | ||
| ) | ||
| .query( | ||
| async ({ | ||
| input, | ||
| }): Promise< | ||
| | { | ||
| ok: true; | ||
| content: string; | ||
| truncated: boolean; | ||
| byteLength: number; | ||
| } | ||
| | { | ||
| ok: false; | ||
| reason: "not-found" | "too-large" | "binary"; | ||
| } | ||
| > => { | ||
| const MAX_FILE_SIZE = 2 * 1024 * 1024; | ||
| try { | ||
| const stats = await fs.stat(input.filePath); | ||
| if (stats.size > MAX_FILE_SIZE) { | ||
| return { ok: false, reason: "too-large" }; | ||
| } | ||
|
|
||
| const buffer = await fs.readFile(input.filePath); | ||
|
|
||
| // Binary detection (same as readWorkingFile) | ||
| const checkLength = Math.min(buffer.length, BINARY_CHECK_SIZE); | ||
| for (let i = 0; i < checkLength; i++) { | ||
| if (buffer[i] === 0) { | ||
| return { ok: false, reason: "binary" }; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ok: true, | ||
| content: buffer.toString("utf-8"), | ||
| truncated: false, | ||
| byteLength: buffer.length, | ||
| }; | ||
| } catch { | ||
| return { ok: false, reason: "not-found" }; | ||
| } | ||
| }, | ||
| ), |
Contributor
There was a problem hiding this comment.
filesystem.readFile opens unrestricted host-file reads over public IPC.
This procedure accepts any renderer-supplied path and reads it directly, bypassing the worktree/path validation model used by secure file reads (changes.readWorkingFile + secureFs). That materially weakens the filesystem security boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 951 -
999, The publicProcedure readFile currently reads any host path (readFile)
directly, bypassing the project's path/worktree validation and opening an
unrestricted IPC file-read. Fix by routing this RPC through the same validated
codepath used elsewhere: call the existing secure read logic (e.g., reuse
changes.readWorkingFile or the secureFs path validation helper) before
performing fs.stat/fs.readFile, ensuring the input.filePath is validated as
inside the worktree/root and normalized; preserve the original return shape
(ok:true/false, reasons "not-found"|"too-large"|"binary") and retain the
BINARY_CHECK_SIZE and MAX_FILE_SIZE checks but only after the path validation
succeeds. Ensure no direct fs.readFile on arbitrary input.filePath remains in
readFile.
Comment on lines
+115
to
+123
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-6" | ||
| onClick={onNavigateToParent} | ||
| > | ||
| <LuArrowUp className="size-3.5" /> | ||
| </Button> | ||
| </TooltipTrigger> |
Contributor
There was a problem hiding this comment.
Add accessible labels to the new icon-only navigation buttons.
The two new buttons rely only on icons/tooltip. Please add explicit accessible names so screen readers can announce these actions correctly.
Suggested fix
<Button
variant="ghost"
size="icon"
className="size-6"
onClick={onNavigateToParent}
+ aria-label="Go to parent directory"
>
<LuArrowUp className="size-3.5" />
</Button>
@@
<Button
variant="ghost"
size="icon"
className="size-6"
onClick={onNavigateHome}
+ aria-label="Back to workspace root"
>
<LuHouse className="size-3.5" />
</Button>Also applies to: 133-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileTreeToolbar/FileTreeToolbar.tsx`
around lines 115 - 123, The new icon-only navigation buttons in FileTreeToolbar
lack accessible names; add explicit aria-labels (or aria-labelledby) to the
Button components so screen readers announce their actions. Update the Button
using onNavigateToParent to include aria-label="Navigate to parent folder" (or
similar), and do the same for the corresponding child/forward button (the Button
using onNavigateToChild or equivalent) with aria-label="Navigate to child
folder" — keep the TooltipTrigger/Tooltip intact but ensure the Button has the
accessible name. Ensure labels are concise and descriptive and use the same
unique component/function names (FileTreeToolbar, onNavigateToParent,
onNavigateToChild) to locate the elements.
Comment on lines
355
to
366
| <FileTreeToolbar | ||
| searchTerm={searchTerm} | ||
| onSearchChange={setSearchTerm} | ||
| onNewFile={() => handleNewFile(worktreePath)} | ||
| onNewFolder={() => handleNewFolder(worktreePath)} | ||
| onNewFile={() => handleNewFile(effectiveRoot ?? worktreePath)} | ||
| onNewFolder={() => handleNewFolder(effectiveRoot ?? worktreePath)} | ||
| onCollapseAll={handleCollapseAll} | ||
| onRefresh={handleRefresh} | ||
| showHiddenFiles={showHiddenFiles} | ||
| onToggleHiddenFiles={handleToggleHiddenFiles} | ||
| onNavigateToParent={navigateToParent} | ||
| onNavigateHome={isOutsideWorktree ? navigateHome : undefined} | ||
| /> |
Contributor
There was a problem hiding this comment.
Mutating file actions remain enabled while browsing outside the workspace root.
With outside-worktree navigation enabled, New/Rename/Delete flows now target arbitrary absolute paths. This extends destructive operations beyond workspace scope without an explicit guard.
Also applies to: 431-442, 473-486, 505-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx`
around lines 355 - 366, The FileTreeToolbar (and other places noted) currently
enables mutating actions even when browsing outside the workspace; update the
component props so mutating handlers are disabled when isOutsideWorktree is
true: for FileTreeToolbar pass undefined (or a no-op and set a disabled flag)
for onNewFile, onNewFolder, and any rename/delete handlers (e.g., the props
passed where you currently call handleNewFile, handleNewFolder, handleRename,
handleDelete), and likewise ensure onNavigateHome remains the only navigation
action exposed; alternatively add a guard inside the handlers (handleNewFile,
handleNewFolder, handleRename, handleDelete) to early-return when
isOutsideWorktree is true so no filesystem mutations are performed outside the
workspace.
Adopt VS Code's approach: store absolute file paths in FileViewerState and derive relative paths only where needed (git operations via secureFs, display). This eliminates ambiguity between relative and absolute paths across the 11 callers of addFileViewerPane and simplifies the file viewer's path handling for files both inside and outside the worktree.
Contributor
There was a problem hiding this comment.
6 issues found across 12 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/screens/main/components/WorkspaceView/RightSidebar/index.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx:133">
P1: This app targets Windows (confirmed in `electron-builder.ts`), but `startsWith("/")` only detects Unix absolute paths. On Windows, absolute paths like `C:\Users\...` won't be detected, resulting in an invalid concatenated path. Use a cross-platform check such as `path.isAbsolute()` or also check for drive-letter prefixes.
(Based on your team's feedback about using cross-platform path utilities instead of manual string operations.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts:71">
P2: Relative paths with `./` or `../` prefixes produce unnormalized absolute paths (e.g., `/workspace/./src/file.ts`). This may cause file viewer tab deduplication to fail since path identity is typically string-based. Consider normalizing the constructed path.
(Based on your team's feedback about using cross-platform path utilities instead of manual string operations.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts:69">
P2: Significant code duplication with `normalizeWorkspaceFilePath` in the same file. Five blocks of logic (trim, `file://` handling, backslash normalization, `./` stripping, empty-check) are copy-pasted verbatim. Extract a shared helper like `normalizeRawFilePath(filePath)` that both functions call, so future changes (e.g., improving `file://` parsing) only need to happen in one place.
(Based on your team's feedback about avoiding duplicating business logic in multiple components.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts:117">
P2: Relative segments (`../`) in the input are not resolved — the function concatenates them as-is, producing paths like `/home/user/project/../foo` instead of `/home/user/foo`. This can break downstream string-based path comparisons (e.g., `startsWith(workspaceRoot)`) and is inconsistent with the function name "resolve". Consider using Node's `path.resolve` or `path.normalize` to canonicalize the result, or at minimum document that the output may contain `..` segments.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts:32">
P2: Duplicated path derivation logic: this inline memo repeats the same `startsWith`/`slice` pattern already extracted into `toRelativePath()` in the sibling `useFileContent.ts` hook. Consider importing or sharing that helper to keep the logic consistent and avoid subtle divergence (e.g., the existing helper also handles the `absolutePath === worktreePath` edge case).
(Based on your team's feedback about avoiding duplicating business logic in multiple components.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts:36">
P1: Bug: `relativeFilePath` falls back to the absolute path for files outside the worktree, causing save operations to send an absolute path to `secureFs.writeFile`, which will throw a `PathValidationError`. The fallback should return `null` (not `filePath`) so the `!relativeFilePath` guards in `handleSaveRaw` and `handleSaveDiff` correctly prevent saving external files. This matches the pattern used in the sibling `useFileContent.ts` hook (its `toRelativePath` returns `null` for external paths).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts (1)
23-31:⚠️ Potential issue | 🟠 Major
toRelativePathis still separator-sensitive and misclassifies Windows paths.Using
${worktreePath}/only works for POSIX separators; Windows in-worktree files can be routed as “outside worktree.”Proposed fix
function toRelativePath( absolutePath: string, worktreePath: string, ): string | null { - if (absolutePath === worktreePath) return "."; - if (absolutePath.startsWith(`${worktreePath}/`)) { - return absolutePath.slice(worktreePath.length + 1); - } + const normalize = (p: string) => + p.replace(/\\/g, "/").replace(/\/+$/, ""); + const abs = normalize(absolutePath); + const root = normalize(worktreePath); + if (abs.toLowerCase() === root.toLowerCase()) return "."; + const prefix = `${root}/`.toLowerCase(); + if (abs.toLowerCase().startsWith(prefix)) { + return abs.slice(root.length + 1); + } return null; }Also applies to: 49-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts` around lines 23 - 31, toRelativePath is treating separators literally and misclassifies Windows paths; replace the manual string checks with robust path handling: import Node's path module and use path.relative(worktreePath, absolutePath) inside toRelativePath (and the duplicate logic at the other occurrence) to compute the relative path, return "." when result is "" or ".", and return null when the relative path starts with ".." or is absolute (indicating outside the worktree); this removes separator sensitivity and handles Windows/POSIX correctly.apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx (2)
133-138:⚠️ Potential issue | 🟠 MajorPath navigation is still POSIX-only.
Parent/breadcrumb parsing still assumes
/, so Windows drive-letter and UNC paths won’t navigate correctly.Proposed fix
const navigateToParent = useCallback(() => { const current = effectiveRoot; - if (!current || current === "/") return; - const parent = current.replace(/\/[^/]+\/?$/, "") || "/"; - setBrowseRoot(parent); + if (!current) return; + const trimmed = current.replace(/[\\/]+$/, ""); + if (trimmed === "/" || /^[A-Za-z]:$/.test(trimmed)) return; + const parent = trimmed.replace(/[\\/][^\\/]+$/, ""); + const fallbackRoot = /^[A-Za-z]:/.test(trimmed) + ? `${trimmed.slice(0, 2)}\\` + : "/"; + setBrowseRoot(parent || fallbackRoot); }, [effectiveRoot]); const breadcrumbSegments = useMemo(() => { if (!isOutsideWorktree || !effectiveRoot) return null; - const parts = effectiveRoot.split("/").filter(Boolean); + const isWindows = /^[A-Za-z]:[\\/]/.test(effectiveRoot); + const parts = effectiveRoot.split(/[\\/]+/).filter(Boolean); return parts.map((name, i) => ({ name, - path: `/${parts.slice(0, i + 1).join("/")}`, + path: isWindows + ? `${parts[0]}\\${parts.slice(1, i + 1).join("\\")}` + : `/${parts.slice(0, i + 1).join("/")}`, })); }, [isOutsideWorktree, effectiveRoot]);Also applies to: 330-338, 376-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx` around lines 133 - 138, The parent/breadcrumb logic assumes POSIX separators; replace the regex with a platform-aware dirname call: detect Windows paths (drive-letter like /^[A-Za-z]:\\/ or UNC like /^\\\\/) and use Node's path.win32.dirname(effectiveRoot) for those, otherwise use path.posix.dirname(effectiveRoot); then fall back to root "/" or "\\" as appropriate and call setBrowseRoot(parent). Update the same logic in the other occurrences (the blocks referencing effectiveRoot/breadcrumb parsing at the other noted spots) and keep references to navigateToParent, effectiveRoot, and setBrowseRoot when making the change.
350-361:⚠️ Potential issue | 🟠 MajorMutating file actions are still enabled outside the workspace root.
New/Rename/Delete flows are still reachable while browsing external directories, which expands destructive operations beyond workspace scope.
Proposed fix
const handleNewFile = useCallback( async (parentPath: string) => { + if (isOutsideWorktree) return; if (parentPath !== effectiveRoot) { const item = tree .getItems() @@ }, - [effectiveRoot, tree], + [isOutsideWorktree, effectiveRoot, tree], ); const handleNewFolder = useCallback( async (parentPath: string) => { + if (isOutsideWorktree) return; if (parentPath !== effectiveRoot) { @@ }, - [effectiveRoot, tree], + [isOutsideWorktree, effectiveRoot, tree], ); const handleDeleteRequest = useCallback((entry: DirectoryEntry) => { + if (isOutsideWorktree) return; setDeleteEntry(entry); setShowDeleteDialog(true); -}, []); +}, [isOutsideWorktree]); const handleRename = useCallback((entry: DirectoryEntry) => { + if (isOutsideWorktree) return; setRenameEntry(entry); -}, []); +}, [isOutsideWorktree]);Also applies to: 426-437, 468-481, 500-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx` around lines 350 - 361, The toolbar and context-menu still allow destructive operations when browsing external directories; update FileTreeToolbar and any FileTree/ContextMenu usages (where props like onNewFile, onNewFolder, onRename, onDelete, onNavigateHome are passed) to guard with isOutsideWorktree and pass undefined or a no-op when isOutsideWorktree is true (e.g., replace onNewFile={() => handleNewFile(...)} and onNewFolder={() => handleNewFolder(...)} with undefined/no-op, and similarly disable handlers for rename/delete in the components that render context menus), and ensure the UI disables/hides the corresponding buttons/menus so mutating flows (handleNewFile, handleNewFolder, handleRename, handleDelete) are not reachable outside the workspace root.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsx (1)
4-4: Use alias import instead of deep relative traversal.Line 4 introduces a fragile multi-level relative import. Please switch this to the configured alias path for consistency and easier refactors.
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/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsx` at line 4, Replace the fragile deep-relative import of resolveToAbsolutePath with the project tsconfig alias: locate the import statement that imports resolveToAbsolutePath (currently using "../../../../../../ChatPane/ChatInterface/utils/file-paths") and change it to use the configured path alias that maps to the ChatPane/ChatInterface/utils/file-paths module (e.g., the alias defined in tsconfig.json), keeping the same imported symbol resolveToAbsolutePath so references in UserMessage.tsx continue to work.
🤖 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/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts`:
- Around line 90-93: The code only treats POSIX absolute paths
(normalizedPath.startsWith("/")) as absolute and returns them, causing Windows
absolute paths like "C:/repo/file.ts" to be incorrectly joined with
workspaceRoot; update the early-return logic around normalizedPath to also
detect Windows drive-letter absolutes (e.g. /^[A-Za-z]:\//) and UNC/Windows
backslash paths (e.g. startsWith("\\") or normalized leading double slashes) and
return normalizedPath for those cases as well; apply the same change to the
similar block referenced in the later section (the code around lines 104-117) so
both places that join with workspaceRoot first check for Windows-style absolute
paths before joining.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts`:
- Around line 32-37: The code currently returns the absolute filePath for files
outside the worktree and then passes it to changes.saveFile (which expects a
worktree-relative path). Replace the useMemo computation for relativeFilePath to
use path.relative(worktreePath, filePath) (importing Node's path) and if the
computed relative path begins with '..' or is absolute, set relativeFilePath to
undefined (or null) instead of returning the absolute path; then update the save
flows that call changes.saveFile (references: changes.saveFile calls around the
handlers that use relativeFilePath) to guard and skip/save accordingly only when
relativeFilePath is defined so an absolute path is never passed to
changes.saveFile.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts`:
- Around line 70-73: The current check uses path.startsWith("/") and manual
string concat which fails on Windows and doesn't normalize segments; update the
logic in useFileLinkClick.ts to import and use isAbsolute and resolve from
node:path, replace the startsWith check with isAbsolute(path) and compute
filePath via resolve(workspaceCwd, path) when not absolute (or simply
resolve(path) for absolute), ensuring proper cross-platform detection and
normalization for the filePath variable that’s constructed from the path input
and workspaceCwd.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts`:
- Around line 23-31: toRelativePath is treating separators literally and
misclassifies Windows paths; replace the manual string checks with robust path
handling: import Node's path module and use path.relative(worktreePath,
absolutePath) inside toRelativePath (and the duplicate logic at the other
occurrence) to compute the relative path, return "." when result is "" or ".",
and return null when the relative path starts with ".." or is absolute
(indicating outside the worktree); this removes separator sensitivity and
handles Windows/POSIX correctly.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx`:
- Around line 133-138: The parent/breadcrumb logic assumes POSIX separators;
replace the regex with a platform-aware dirname call: detect Windows paths
(drive-letter like /^[A-Za-z]:\\/ or UNC like /^\\\\/) and use Node's
path.win32.dirname(effectiveRoot) for those, otherwise use
path.posix.dirname(effectiveRoot); then fall back to root "/" or "\\" as
appropriate and call setBrowseRoot(parent). Update the same logic in the other
occurrences (the blocks referencing effectiveRoot/breadcrumb parsing at the
other noted spots) and keep references to navigateToParent, effectiveRoot, and
setBrowseRoot when making the change.
- Around line 350-361: The toolbar and context-menu still allow destructive
operations when browsing external directories; update FileTreeToolbar and any
FileTree/ContextMenu usages (where props like onNewFile, onNewFolder, onRename,
onDelete, onNavigateHome are passed) to guard with isOutsideWorktree and pass
undefined or a no-op when isOutsideWorktree is true (e.g., replace onNewFile={()
=> handleNewFile(...)} and onNewFolder={() => handleNewFolder(...)} with
undefined/no-op, and similarly disable handlers for rename/delete in the
components that render context menus), and ensure the UI disables/hides the
corresponding buttons/menus so mutating flows (handleNewFile, handleNewFolder,
handleRename, handleDelete) are not reachable outside the workspace root.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsx`:
- Line 4: Replace the fragile deep-relative import of resolveToAbsolutePath with
the project tsconfig alias: locate the import statement that imports
resolveToAbsolutePath (currently using
"../../../../../../ChatPane/ChatInterface/utils/file-paths") and change it to
use the configured path alias that maps to the
ChatPane/ChatInterface/utils/file-paths module (e.g., the alias defined in
tsconfig.json), keeping the same imported symbol resolveToAbsolutePath so
references in UserMessage.tsx continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a48dd317-aecb-4a13-a1c5-af54a7ea87e5
📒 Files selected for processing (12)
apps/desktop/src/renderer/screens/main/components/CommandPalette/CommandPalette.tsxapps/desktop/src/renderer/screens/main/components/KeywordSearch/useKeywordSearch.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/MastraToolCallBlock/MastraToolCallBlock.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/MessagePartsRenderer/MessagePartsRenderer.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsxapps/desktop/src/shared/tabs-types.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/shared/tabs-types.ts
Replace hand-rolled string manipulation (startsWith/slice, split/pop, regex) with pathe's resolve, relative, basename, dirname, normalize, and isAbsolute. This gives the renderer proper path semantics without needing Node.js built-ins.
Contributor
There was a problem hiding this comment.
3 issues found across 11 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/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts:36">
P2: The `startsWith("..")` guard misclassifies valid in-worktree paths like `..foo` as external. Check for parent-directory traversal segments (`..` or `../`) instead of any `..` prefix.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts:29">
P2: `startsWith("..")` incorrectly marks valid paths like `..foo` as outside the worktree; this breaks handling for dot-prefixed in-worktree files.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts:42">
P2: The `rel.startsWith("..")` guard is too broad and can reject valid files under the workspace (e.g. child directories named `..config`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (isAbsolute(normalizedPath)) { | ||
| const rel = relative(root, normalizedPath); | ||
| // relative() returns a path starting with ".." if outside the root | ||
| if (rel.startsWith("..")) return null; |
Contributor
There was a problem hiding this comment.
P2: The rel.startsWith("..") guard is too broad and can reject valid files under the workspace (e.g. child directories named ..config).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts, line 42:
<comment>The `rel.startsWith("..")` guard is too broad and can reject valid files under the workspace (e.g. child directories named `..config`).</comment>
<file context>
@@ -26,37 +28,24 @@ export function normalizeWorkspaceFilePath({
+ if (isAbsolute(normalizedPath)) {
+ const rel = relative(root, normalizedPath);
+ // relative() returns a path starting with ".." if outside the root
+ if (rel.startsWith("..")) return null;
+ normalizedPath = rel;
}
</file context>
Suggested change
| if (rel.startsWith("..")) return null; | |
| if (rel === ".." || rel.startsWith("../") || isAbsolute(rel)) return null; |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts (1)
32-37:⚠️ Potential issue | 🟠 MajorAbsolute paths still reach
changes.saveFilefor external files.When the file is outside the worktree (
rel.startsWith("..")), this returns the absolutefilePath. From the relevant code snippets,validateRelativePathinpath-validation.tsexplicitly rejects absolute paths by throwingPathValidationErrorwith code"ABSOLUTE_PATH". This will cause save operations to fail for files outside the worktree.Return
nullwhen the file is outside the worktree so the guards at lines 83 and 94 properly skip the save operation.💡 Suggested fix
// Derive worktree-relative path for secureFs save operations const relativeFilePath = useMemo(() => { if (!worktreePath) return filePath; const rel = relative(worktreePath, filePath); - return rel.startsWith("..") ? filePath : rel; + // External files (outside worktree) cannot be saved via secureFs + return rel.startsWith("..") ? null : rel; }, [worktreePath, filePath]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts` around lines 32 - 37, The derived relativeFilePath in the useMemo currently returns the absolute filePath when the file is outside the worktree, which later causes validateRelativePath to throw for absolute paths; change the useMemo so that when rel.startsWith("..") it returns null instead of filePath (keep returning filePath only when worktreePath is falsy), so downstream guards that check for a null relativeFilePath before calling changes.saveFile will correctly skip saving external files; update references to relativeFilePath accordingly to handle the nullable type.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts (1)
72-84: Cache invalidation gap for external files.Files opened via
filesystem.readFile(outside the worktree) won't have their cache invalidated when saved, sinceRightSidebar.invalidateFileContentonly invalidateschanges.readWorkingFileandchanges.getFileContentsqueries using relative paths.Consider adding invalidation for
filesystem.readFileinuseFileSavewhen saving external files, or document this as a known limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts` around lines 72 - 84, The external-file read query electronTrpc.filesystem.readFile.useQuery isn't invalidated on save because RightSidebar.invalidateFileContent currently only clears changes.readWorkingFile and changes.getFileContents (relative paths); update useFileSave (or RightSidebar.invalidateFileContent) to also call invalidate for the filesystem.readFile query keyed by the absolute filePath when saving files outside the worktree (or add a branch in useFileSave to detect !isInsideWorktree and invalidate electronTrpc.filesystem.readFile with the same filePath), ensuring saved external files refresh correctly.
🤖 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/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts`:
- Around line 24-31: The toRelativePath function misclassifies the worktree root
because relative(worktreePath, absolutePath) returns "" when paths are equal;
update toRelativePath to treat an empty string as the root case (return "." or
another sentinel expected by callers) instead of returning null, while still
returning null when rel startsWith(".."); adjust the branch that checks rel ===
"" to return "." so callers that expect a non-empty relative path get "." for
the root.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts`:
- Around line 32-37: The derived relativeFilePath in the useMemo currently
returns the absolute filePath when the file is outside the worktree, which later
causes validateRelativePath to throw for absolute paths; change the useMemo so
that when rel.startsWith("..") it returns null instead of filePath (keep
returning filePath only when worktreePath is falsy), so downstream guards that
check for a null relativeFilePath before calling changes.saveFile will correctly
skip saving external files; update references to relativeFilePath accordingly to
handle the nullable type.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts`:
- Around line 72-84: The external-file read query
electronTrpc.filesystem.readFile.useQuery isn't invalidated on save because
RightSidebar.invalidateFileContent currently only clears changes.readWorkingFile
and changes.getFileContents (relative paths); update useFileSave (or
RightSidebar.invalidateFileContent) to also call invalidate for the
filesystem.readFile query keyed by the absolute filePath when saving files
outside the worktree (or add a branch in useFileSave to detect !isInsideWorktree
and invalidate electronTrpc.filesystem.readFile with the same filePath),
ensuring saved external files refresh correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 074b52f5-836f-43e9-82ed-0f72d7c5018a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/package.jsonapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsxapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths.ts
Comment on lines
+24
to
+31
| function toRelativePath( | ||
| absolutePath: string, | ||
| worktreePath: string, | ||
| ): string | null { | ||
| const rel = relative(worktreePath, absolutePath); | ||
| if (rel.startsWith("..") || rel === "") return null; | ||
| return rel; | ||
| } |
Contributor
There was a problem hiding this comment.
Edge case: toRelativePath returns null for worktree root.
When absolutePath equals worktreePath exactly, relative() returns an empty string "", causing this function to return null. This incorrectly treats the worktree root directory itself as "outside the worktree."
Suggested fix
function toRelativePath(
absolutePath: string,
worktreePath: string,
): string | null {
const rel = relative(worktreePath, absolutePath);
- if (rel.startsWith("..") || rel === "") return null;
+ if (rel.startsWith("..")) return null;
+ // Empty string means absolutePath === worktreePath (root), treat as "."
return rel;
}Alternatively, return "." for the root case if downstream consumers expect a non-empty relative path.
📝 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
| function toRelativePath( | |
| absolutePath: string, | |
| worktreePath: string, | |
| ): string | null { | |
| const rel = relative(worktreePath, absolutePath); | |
| if (rel.startsWith("..") || rel === "") return null; | |
| return rel; | |
| } | |
| function toRelativePath( | |
| absolutePath: string, | |
| worktreePath: string, | |
| ): string | null { | |
| const rel = relative(worktreePath, absolutePath); | |
| if (rel.startsWith("..")) return null; | |
| // Empty string means absolutePath === worktreePath (root), treat as "." | |
| return rel; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts`
around lines 24 - 31, The toRelativePath function misclassifies the worktree
root because relative(worktreePath, absolutePath) returns "" when paths are
equal; update toRelativePath to treat an empty string as the root case (return
"." or another sentinel expected by callers) instead of returning null, while
still returning null when rel startsWith(".."); adjust the branch that checks
rel === "" to return "." so callers that expect a non-empty relative path get
"." for the root.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
…le save 1. Bump store version to 8; migration clears file viewer panes that still have legacy relative filePaths (can't resolve without server worktreePath). 2. useFileContent/useFileSave resolve legacy relative paths against worktreePath so they degrade gracefully if migration doesn't catch them. 3. useFileSave returns null for external files (outside worktree) instead of passing an absolute path to secureFs which would reject it. Save buttons become no-ops for external files (read-only).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts (1)
124-143:⚠️ Potential issue | 🟠 MajorNormalize
oldPathbefore the diff query.
filePathis converted back to a worktree-relative path here, butoldPathis still forwarded as-is. With this PR moving file-viewer state to absolute paths, renamed-file diffs can end up sending an absoluteoldPathintochanges.getFileContents, which is git-backed and not doing the same secureFs normalization.Suggested fix
+ const relativeOldPath = useMemo( + () => (oldPath && worktreePath ? toRelativePath(oldPath, worktreePath) : null), + [oldPath, worktreePath], + ); + const { data: diffData, isLoading: isLoadingDiff } = electronTrpc.changes.getFileContents.useQuery( { worktreePath, filePath: relativePath ?? "", - oldPath, + oldPath: relativeOldPath ?? undefined, category: diffCategory ?? "unstaged", commitHash, defaultBranch: diffCategory === "against-base" ? effectiveBaseBranch : undefined, }, { enabled: !isRemote && isInsideWorktree && viewMode === "diff" && !!diffCategory && !!relativePath && + (!oldPath || !!relativeOldPath) && !!worktreePath, }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts` around lines 124 - 143, The diff query is passing oldPath unnormalized which can be absolute; normalize oldPath to a worktree-relative path the same way filePath/relativePath is normalized before calling electronTrpc.changes.getFileContents.useQuery. Locate the useQuery call in useFileContent.ts and transform oldPath (e.g., using the same helper/logic applied to filePath or relativePath) to a secure worktree-relative value (or undefined if absent) and pass that normalized value as the oldPath parameter to changes.getFileContents so git-backed code receives consistent, relative paths.
🤖 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/stores/tabs/store.ts`:
- Around line 1885-1896: The migration currently deletes entries from
state.panes for pre-absolute-path file viewers but leaves tab.layout references
pointing to those pane IDs; update the migration in store.ts so that after
removing a pane from state.panes you also scan all tabs (state.tabs) and remove
any references to that pane ID from each tab.layout (recursively if needed), and
if a tab ends up with no leaf panes remove that tab entirely from state.tabs (or
clear its layout) so persisted layouts no longer reference missing pane IDs.
- Around line 1889-1893: The current check treats Windows absolute paths as
relative because it uses startsWith("/") and only checks "http"; replace this
logic by importing and using isAbsolute from pathe to detect filesystem absolute
paths for pane.fileViewer.filePath and also broaden the URL check to reject both
"http" and "https" schemes (e.g., test via startsWith("http://") ||
startsWith("https://") or a small URL check) so Windows drive-letter and UNC
paths are not misclassified and thus not deleted during upgrade; update the
condition around pane.fileViewer?.filePath accordingly in the function handling
the v8 migration/cleanup.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.ts`:
- Around line 124-143: The diff query is passing oldPath unnormalized which can
be absolute; normalize oldPath to a worktree-relative path the same way
filePath/relativePath is normalized before calling
electronTrpc.changes.getFileContents.useQuery. Locate the useQuery call in
useFileContent.ts and transform oldPath (e.g., using the same helper/logic
applied to filePath or relativePath) to a secure worktree-relative value (or
undefined if absent) and pass that normalized value as the oldPath parameter to
changes.getFileContents so git-backed code receives consistent, relative paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f413a5a5-2997-46d7-abb3-a3576f27d4a6
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.tsapps/desktop/src/renderer/stores/tabs/store.ts
Comment on lines
+1885
to
+1896
| if (version < 8 && state.panes) { | ||
| // Clear file viewer panes with relative paths (pre-absolute-path era). | ||
| // We can't resolve them without worktreePath (server-side data). | ||
| for (const [id, pane] of Object.entries(state.panes)) { | ||
| if ( | ||
| pane.fileViewer?.filePath && | ||
| !pane.fileViewer.filePath.startsWith("/") && | ||
| !pane.fileViewer.filePath.startsWith("http") | ||
| ) { | ||
| delete state.panes[id]; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't delete panes without repairing the owning tab layout.
This migration removes entries from state.panes, but it never prunes those pane IDs from tab.layout or drops tabs that become empty. After rehydrate, tabs can still point at missing leaves, which is enough to leave an invalid layout in persisted state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/tabs/store.ts` around lines 1885 - 1896, The
migration currently deletes entries from state.panes for pre-absolute-path file
viewers but leaves tab.layout references pointing to those pane IDs; update the
migration in store.ts so that after removing a pane from state.panes you also
scan all tabs (state.tabs) and remove any references to that pane ID from each
tab.layout (recursively if needed), and if a tab ends up with no leaf panes
remove that tab entirely from state.tabs (or clear its layout) so persisted
layouts no longer reference missing pane IDs.
Comment on lines
+1889
to
+1893
| if ( | ||
| pane.fileViewer?.filePath && | ||
| !pane.fileViewer.filePath.startsWith("/") && | ||
| !pane.fileViewer.filePath.startsWith("http") | ||
| ) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/stores/tabs/store.ts | sed -n '1880,1900p'Repository: superset-sh/superset
Length of output: 823
🏁 Script executed:
head -50 apps/desktop/src/renderer/stores/tabs/store.ts | grep -n "import"Repository: superset-sh/superset
Length of output: 717
🏁 Script executed:
sed -n '1,50p' apps/desktop/src/renderer/stores/tabs/store.ts | cat -nRepository: superset-sh/superset
Length of output: 1985
🏁 Script executed:
sed -n '1880,1920p' apps/desktop/src/renderer/stores/tabs/store.ts | cat -nRepository: superset-sh/superset
Length of output: 1865
🏁 Script executed:
grep -r "isAbsolute" apps/desktop/src --include="*.ts" --include="*.tsx"Repository: superset-sh/superset
Length of output: 2948
🏁 Script executed:
cat package.json | jq '.dependencies, .devDependencies' | grep -i patheRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cd apps/desktop && npm list pathe 2>/dev/null || cat package.json | jq '.dependencies.pathe, .devDependencies.pathe'Repository: superset-sh/superset
Length of output: 130
🏁 Script executed:
web_search
pathe isAbsolute Windows absolute path detectionRepository: superset-sh/superset
Length of output: 158
🏁 Script executed:
sed -n '1,10p' apps/desktop/src/renderer/stores/tabs/store.ts | cat -nRepository: superset-sh/superset
Length of output: 670
🏁 Script executed:
sed -n '1889,1893p' apps/desktop/src/renderer/stores/tabs/store.tsRepository: superset-sh/superset
Length of output: 229
Use isAbsolute from pathe to handle Windows paths in the v8 migration.
startsWith("/") only catches POSIX absolute paths. On Windows, drive-letter paths (e.g., C:\path) and UNC paths (e.g., \\server\share) are valid external files but will be treated as relative paths and incorrectly deleted during upgrade.
Use isAbsolute from pathe, which is already available and used elsewhere in the codebase, and improve the HTTP URL check to handle both http and https:
Suggested fix
-import { basename } from "pathe";
+import { basename, isAbsolute } from "pathe";
...
for (const [id, pane] of Object.entries(state.panes)) {
if (
pane.fileViewer?.filePath &&
- !pane.fileViewer.filePath.startsWith("/") &&
- !pane.fileViewer.filePath.startsWith("http")
+ !isAbsolute(pane.fileViewer.filePath) &&
+ !/^https?:\/\//.test(pane.fileViewer.filePath)
) {
delete state.panes[id];
}
}📝 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
| if ( | |
| pane.fileViewer?.filePath && | |
| !pane.fileViewer.filePath.startsWith("/") && | |
| !pane.fileViewer.filePath.startsWith("http") | |
| ) { | |
| if ( | |
| pane.fileViewer?.filePath && | |
| !isAbsolute(pane.fileViewer.filePath) && | |
| !/^https?:\/\//.test(pane.fileViewer.filePath) | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/tabs/store.ts` around lines 1889 - 1893, The
current check treats Windows absolute paths as relative because it uses
startsWith("/") and only checks "http"; replace this logic by importing and
using isAbsolute from pathe to detect filesystem absolute paths for
pane.fileViewer.filePath and also broaden the URL check to reject both "http"
and "https" schemes (e.g., test via startsWith("http://") ||
startsWith("https://") or a small URL check) so Windows drive-letter and UNC
paths are not misclassified and thus not deleted during upgrade; update the
condition around pane.fileViewer?.filePath accordingly in the function handling
the v8 migration/cleanup.
# Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsx # apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx # apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx # apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts # apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx (1)
314-326:⚠️ Potential issue | 🟠 MajorClipboard operations will copy relative paths instead of absolute paths.
Passing
displayPath(a worktree-relative path) toFileEditorContextMenuaffects the "Copy Path" and "Copy Path with Line" context menu actions. Based on theuseEditorActionshook (lines 46-58 and 60-116), these operations writefilePathdirectly to the clipboard.Users typically expect "Copy Path" to provide an absolute path usable in terminals or other tools. With this change, they'll receive relative paths like
src/file.tsinstead of/home/user/project/src/file.ts.Consider passing both the absolute
filePathanddisplayPath, or rename the props to clarify intent:🛠️ Suggested approach
<FileEditorContextMenu editorRef={editorRef} - filePath={displayPath} + filePath={filePath} + displayPath={displayPath} onSplitHorizontal={onSplitHorizontal}Then update
useEditorActionsto use the absolute path for clipboard operations while displaying the relative path in UI elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx` around lines 314 - 326, The clipboard actions are receiving the worktree-relative displayPath, causing "Copy Path" to produce relative paths; update the FileEditorContextMenu props to pass both the absolute path and the display (relative) path (e.g., add a new prop like absoluteFilePath or filePathAbsolute alongside displayPath) and update useEditorActions to read the absolute path prop for clipboard operations (Copy Path and Copy Path with Line) while continuing to use displayPath for UI labels; ensure prop names referenced are FileEditorContextMenu, displayPath, and useEditorActions so the change is easy to locate.
♻️ Duplicate comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx (1)
327-336:⚠️ Potential issue | 🟠 MajorMutating file actions remain enabled while browsing outside the workspace root.
onNewFileandonNewFolderare unconditionally passed to the toolbar, and the context menu (lines 475-486) similarly enables New File/New Folder actions regardless ofisOutsideWorktreestate.Consider disabling these mutating operations when browsing outside the worktree to prevent unintended file system modifications in arbitrary directories:
🛠️ Suggested approach
<FileTreeToolbar searchTerm={searchTerm} onSearchChange={setSearchTerm} - onNewFile={() => handleNewFile(effectiveRoot ?? worktreePath)} - onNewFolder={() => handleNewFolder(effectiveRoot ?? worktreePath)} + onNewFile={isOutsideWorktree ? undefined : () => handleNewFile(effectiveRoot ?? worktreePath)} + onNewFolder={isOutsideWorktree ? undefined : () => handleNewFolder(effectiveRoot ?? worktreePath)} onCollapseAll={handleCollapseAll}Apply similar guards to the context menu items and consider disabling rename/delete for entries outside the worktree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx` around lines 327 - 336, FileTreeToolbar is currently receiving onNewFile/onNewFolder unconditionally and the file context menu also enables New File/New Folder (and likely rename/delete) even when isOutsideWorktree is true; update the FileTreeToolbar props to pass onNewFile={isOutsideWorktree ? undefined : () => handleNewFile(effectiveRoot ?? worktreePath)} and onNewFolder={isOutsideWorktree ? undefined : () => handleNewFolder(effectiveRoot ?? worktreePath)} (or pass a disabled flag) and similarly guard the context menu rendering/props so New File, New Folder, Rename, and Delete actions are either not rendered or are disabled when isOutsideWorktree is true, referencing the handleNewFile, handleNewFolder, isOutsideWorktree and the context menu item render logic.apps/desktop/src/renderer/stores/tabs/store.ts (1)
1923-1935:⚠️ Potential issue | 🟠 MajorMigration leaves orphaned pane IDs in tab layouts.
The migration deletes panes from
state.panesbut doesn't remove the corresponding pane IDs fromtab.layout. After rehydration, tabs may reference non-existent panes, potentially causing rendering failures or empty pane slots.Consider also cleaning up layouts when removing panes:
Suggested approach
if (version < 8 && state.panes) { // Clear file viewer panes with relative paths (pre-absolute-path era). // We can't resolve them without worktreePath (server-side data). + const paneIdsToRemove = new Set<string>(); for (const [id, pane] of Object.entries(state.panes)) { if ( pane.fileViewer?.filePath && !pane.fileViewer.filePath.startsWith("/") && !pane.fileViewer.filePath.startsWith("http") ) { + paneIdsToRemove.add(id); delete state.panes[id]; } } + // Remove orphaned pane IDs from tab layouts + if (paneIdsToRemove.size > 0 && state.tabs) { + for (const tab of state.tabs) { + if (tab.layout) { + tab.layout = removeOrphanedPanesFromLayout(tab.layout, paneIdsToRemove); + } + } + // Remove tabs with empty layouts + state.tabs = state.tabs.filter((tab) => tab.layout != null); + } }You'll need a helper like
removeOrphanedPanesFromLayoutthat recursively prunes pane IDs from theMosaicNodetree structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/store.ts` around lines 1923 - 1935, The migration loop that deletes entries from state.panes when version < 8 leaves dangling pane IDs in tab.layout; add a cleanup step after deleting pane entries to walk each tab.layout (the MosaicNode tree) and remove any references to removed pane IDs. Implement a helper function (e.g., removeOrphanedPanesFromLayout(layout: MosaicNode | null, validPaneIds: Set<string>)) that recursively prunes leaf nodes whose id is not in validPaneIds and collapses/cleans parent nodes as needed, then call it for each tab (use the same tabs iteration that accesses tab.layout) so layouts no longer reference deleted panes. Ensure you reference state.panes and the new helper name so reviewers can find the changes.apps/desktop/src/lib/trpc/routers/filesystem/index.ts (1)
912-940:⚠️ Potential issue | 🔴 CriticalReintroduce a trusted-root check before reading renderer-supplied paths.
This
publicProcedureaccepts anyfilePathfrom the renderer, does not enforce the “absolute path” contract at the schema boundary, and then callsfs.stat/fs.readFiledirectly. That bypasses the validatedsecureFs/readWorkingFileflow used for repo files and lets renderer-controlled inputs escape any explicit user-approved root. Please validate the resolved realpath against an allowed root before reading, instead of reading arbitrary host paths here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 912 - 940, The readFile publicProcedure reads renderer-supplied filePath directly; before calling fs.stat/fs.readFile resolve the path with fs.realpath (or path.resolve+fs.realpath) and verify the resulting realpath is inside the trusted root(s) used elsewhere (e.g. the same root(s) used by secureFs / readWorkingFile) — if the realpath is outside the allowed root return an error (e.g. ok: false with a suitable reason) instead of reading; update readFile to perform this trusted-root check and reuse the existing allowed-root constant or helper to ensure consistency with secureFs/readWorkingFile.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/components/UserMessageText/UserMessageText.tsx (2)
1-1: Consider using a path alias instead of deep relative imports.This 8-level relative path is fragile and hard to maintain. As per coding guidelines, use alias as defined in
tsconfig.jsonwhen possible.♻️ Suggested refactor
If a path alias like
@/is configured, the import could be simplified to something like:-import { resolveToAbsolutePath } from "../../../../../../../../ChatPane/ChatInterface/utils/file-paths"; +import { resolveToAbsolutePath } from "@/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/utils/file-paths";Alternatively, consider whether this utility should be lifted to a shared location (e.g.,
@/lib/utils/file-paths) given it's now used across multiple chat pane implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/components/UserMessageText/UserMessageText.tsx` at line 1, The import uses a fragile deep relative path for resolveToAbsolutePath; change the import to use the project path alias defined in tsconfig (e.g., "@/..." or the configured alias) so UserMessageText imports resolveToAbsolutePath via the alias instead of "../../../../../../../../ChatPane/ChatInterface/utils/file-paths"; if this utility is used by multiple chat panes consider moving it to a shared location (e.g., "@/lib/utils/file-paths") and update the import in the UserMessageText component accordingly.
45-60: **Biome flags array index in key — likely acceptable here but worth noting.**Biome flags the use ofsegmentIndexin the key. "We don't recommend using indexes for keys if the order of items may change." However, in this case:
- The key is composite:
${message.id}-${partIndex}-${segmentIndex}- Message content is immutable — the text and parsed segments don't change after render
- The segments lack intrinsic unique identifiers
Since the list is static (parsed from immutable message text), using the index "as a last resort" when you "don't have stable IDs" is acceptable here. Consider using
segment.relativePathas part of the key if paths are guaranteed unique within a message, but the current approach is reasonable for this use case.💡 Optional: use segment content in key
If
relativePathvalues are unique within a message, you could make the key more semantically stable:<button type="button" - key={`${message.id}-${partIndex}-${segmentIndex}`} + key={`${message.id}-${partIndex}-mention-${segment.relativePath}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/components/UserMessageText/UserMessageText.tsx` around lines 45 - 60, The composite key in UserMessageText currently uses `${message.id}-${partIndex}-${segmentIndex}` which relies on the index; update the key to use a stable segment identifier when available (e.g., `${message.id}-${partIndex}-${segment.relativePath}`) by replacing `segmentIndex` with `segment.relativePath` in the JSX key expression inside the UserMessageText render; if `segment.relativePath` cannot be guaranteed unique, keep the existing composite key as a last resort.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (1)
250-254: Potential false positive in worktree path detection.The
filePath.startsWith(worktreePath)check can produce false positives when directory names share a common prefix. For example, ifworktreePathis/home/userandfilePathis/home/username/file.txt, it would incorrectly be treated as inside the worktree.Consider normalizing paths and checking with a trailing separator:
🛠️ Suggested fix
const displayPath = (() => { - if (!worktreePath || !filePath.startsWith(worktreePath)) return filePath; + if (!worktreePath) return filePath; + const normalizedWorktree = worktreePath.endsWith("/") ? worktreePath : `${worktreePath}/`; + if (!filePath.startsWith(normalizedWorktree) && filePath !== worktreePath) return filePath; const rel = relative(worktreePath, filePath); return rel.startsWith("..") ? filePath : rel; })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx` around lines 250 - 254, The current startsWith(worktreePath) check can falsely detect containment when directory names share prefixes; instead normalize both paths with path.resolve (or path.normalize), then compute rel = relative(resolvedWorktreePath, resolvedFilePath) and use that to decide displayPath: if rel === '' || (rel !== '..' && !rel.startsWith('..' + path.sep)) treat the file as inside the worktree and return rel, otherwise return the original filePath; update the displayPath logic (refer to displayPath, worktreePath, filePath, relative) to remove the startsWith(worktreePath) check and use the normalized relative-path test above.apps/desktop/src/lib/trpc/routers/filesystem/index.ts (1)
942-948: ReuseisBinaryContent()instead of open-coding the scan again.This duplicates the helper already defined at Lines 274-282, so the two read paths can drift on future changes. Reusing the helper keeps binary detection consistent.
♻️ Suggested cleanup
- // Binary detection (same as readWorkingFile) - const checkLength = Math.min(buffer.length, BINARY_CHECK_SIZE); - for (let i = 0; i < checkLength; i++) { - if (buffer[i] === 0) { - return { ok: false, reason: "binary" }; - } - } + if (isBinaryContent(buffer)) { + return { ok: false, reason: "binary" }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 942 - 948, This block duplicates binary detection logic already implemented in isBinaryContent; remove the manual loop and call isBinaryContent(...) instead (pass the buffer and the BINARY_CHECK_SIZE or whatever signature the helper uses) and if it returns true, return { ok: false, reason: "binary" }—this ensures the same binary-detection behavior used by readWorkingFile is reused here and prevents drift between the two paths.
🤖 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/lib/trpc/routers/filesystem/index.ts`:
- Around line 935-940: The current guard only checks stats.size before calling
fs.readFile, which can still allow FIFOs or device nodes; update the logic
around fs.stat/ fs.readFile to first reject non-regular files by verifying
stats.isFile() is true (using the stats object returned from fs.stat for
input.filePath), and only then check stats.size against MAX_FILE_SIZE and
proceed to call fs.readFile; ensure the function returns { ok: false, reason:
"not-file" } (or similar) for non-regular files and keeps the existing { ok:
false, reason: "too-large" } behavior for oversized regular files.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsx`:
- Around line 314-326: The clipboard actions are receiving the worktree-relative
displayPath, causing "Copy Path" to produce relative paths; update the
FileEditorContextMenu props to pass both the absolute path and the display
(relative) path (e.g., add a new prop like absoluteFilePath or filePathAbsolute
alongside displayPath) and update useEditorActions to read the absolute path
prop for clipboard operations (Copy Path and Copy Path with Line) while
continuing to use displayPath for UI labels; ensure prop names referenced are
FileEditorContextMenu, displayPath, and useEditorActions so the change is easy
to locate.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts`:
- Around line 912-940: The readFile publicProcedure reads renderer-supplied
filePath directly; before calling fs.stat/fs.readFile resolve the path with
fs.realpath (or path.resolve+fs.realpath) and verify the resulting realpath is
inside the trusted root(s) used elsewhere (e.g. the same root(s) used by
secureFs / readWorkingFile) — if the realpath is outside the allowed root return
an error (e.g. ok: false with a suitable reason) instead of reading; update
readFile to perform this trusted-root check and reuse the existing allowed-root
constant or helper to ensure consistency with secureFs/readWorkingFile.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx`:
- Around line 327-336: FileTreeToolbar is currently receiving
onNewFile/onNewFolder unconditionally and the file context menu also enables New
File/New Folder (and likely rename/delete) even when isOutsideWorktree is true;
update the FileTreeToolbar props to pass onNewFile={isOutsideWorktree ?
undefined : () => handleNewFile(effectiveRoot ?? worktreePath)} and
onNewFolder={isOutsideWorktree ? undefined : () => handleNewFolder(effectiveRoot
?? worktreePath)} (or pass a disabled flag) and similarly guard the context menu
rendering/props so New File, New Folder, Rename, and Delete actions are either
not rendered or are disabled when isOutsideWorktree is true, referencing the
handleNewFile, handleNewFolder, isOutsideWorktree and the context menu item
render logic.
In `@apps/desktop/src/renderer/stores/tabs/store.ts`:
- Around line 1923-1935: The migration loop that deletes entries from
state.panes when version < 8 leaves dangling pane IDs in tab.layout; add a
cleanup step after deleting pane entries to walk each tab.layout (the MosaicNode
tree) and remove any references to removed pane IDs. Implement a helper function
(e.g., removeOrphanedPanesFromLayout(layout: MosaicNode | null, validPaneIds:
Set<string>)) that recursively prunes leaf nodes whose id is not in validPaneIds
and collapses/cleans parent nodes as needed, then call it for each tab (use the
same tabs iteration that accesses tab.layout) so layouts no longer reference
deleted panes. Ensure you reference state.panes and the new helper name so
reviewers can find the changes.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts`:
- Around line 942-948: This block duplicates binary detection logic already
implemented in isBinaryContent; remove the manual loop and call
isBinaryContent(...) instead (pass the buffer and the BINARY_CHECK_SIZE or
whatever signature the helper uses) and if it returns true, return { ok: false,
reason: "binary" }—this ensures the same binary-detection behavior used by
readWorkingFile is reused here and prevents drift between the two paths.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/components/UserMessageText/UserMessageText.tsx`:
- Line 1: The import uses a fragile deep relative path for
resolveToAbsolutePath; change the import to use the project path alias defined
in tsconfig (e.g., "@/..." or the configured alias) so UserMessageText imports
resolveToAbsolutePath via the alias instead of
"../../../../../../../../ChatPane/ChatInterface/utils/file-paths"; if this
utility is used by multiple chat panes consider moving it to a shared location
(e.g., "@/lib/utils/file-paths") and update the import in the UserMessageText
component accordingly.
- Around line 45-60: The composite key in UserMessageText currently uses
`${message.id}-${partIndex}-${segmentIndex}` which relies on the index; update
the key to use a stable segment identifier when available (e.g.,
`${message.id}-${partIndex}-${segment.relativePath}`) by replacing
`segmentIndex` with `segment.relativePath` in the JSX key expression inside the
UserMessageText render; if `segment.relativePath` cannot be guaranteed unique,
keep the existing composite key as a last resort.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx`:
- Around line 250-254: The current startsWith(worktreePath) check can falsely
detect containment when directory names share prefixes; instead normalize both
paths with path.resolve (or path.normalize), then compute rel =
relative(resolvedWorktreePath, resolvedFilePath) and use that to decide
displayPath: if rel === '' || (rel !== '..' && !rel.startsWith('..' + path.sep))
treat the file as inside the worktree and return rel, otherwise return the
original filePath; update the displayPath logic (refer to displayPath,
worktreePath, filePath, relative) to remove the startsWith(worktreePath) check
and use the normalized relative-path test above.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30e0476f-1328-40d4-8d7d-cbd9e18a02b4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
apps/desktop/package.jsonapps/desktop/src/lib/trpc/routers/filesystem/index.tsapps/desktop/src/renderer/screens/main/components/KeywordSearch/useKeywordSearch.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/components/UserMessageText/UserMessageText.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/FileViewerContent/FileViewerContent.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileTreeToolbar/FileTreeToolbar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsxapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/shared/tabs-types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/components/FileTreeToolbar/FileTreeToolbar.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx
- apps/desktop/package.json
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatMastraPane/ChatMastraInterface/components/ChatMastraMessageList/components/UserMessage/UserMessage.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileSave/useFileSave.ts
Comment on lines
+935
to
+940
| const stats = await fs.stat(input.filePath); | ||
| if (stats.size > MAX_FILE_SIZE) { | ||
| return { ok: false, reason: "too-large" }; | ||
| } | ||
|
|
||
| const buffer = await fs.readFile(input.filePath); |
Contributor
There was a problem hiding this comment.
Reject non-regular files before fs.readFile().
The size check alone is not a safe guard here. FIFOs, device nodes, and other special files can report small or zero sizes, and fs.readFile() can then block or blow past the 2 MB policy. Bail out unless stats.isFile() is true.
🔒 Suggested fix
const stats = await fs.stat(input.filePath);
+ if (!stats.isFile()) {
+ return { ok: false, reason: "not-found" };
+ }
if (stats.size > MAX_FILE_SIZE) {
return { ok: false, reason: "too-large" };
}📝 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 stats = await fs.stat(input.filePath); | |
| if (stats.size > MAX_FILE_SIZE) { | |
| return { ok: false, reason: "too-large" }; | |
| } | |
| const buffer = await fs.readFile(input.filePath); | |
| const stats = await fs.stat(input.filePath); | |
| if (!stats.isFile()) { | |
| return { ok: false, reason: "not-found" }; | |
| } | |
| if (stats.size > MAX_FILE_SIZE) { | |
| return { ok: false, reason: "too-large" }; | |
| } | |
| const buffer = await fs.readFile(input.filePath); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 935 -
940, The current guard only checks stats.size before calling fs.readFile, which
can still allow FIFOs or device nodes; update the logic around fs.stat/
fs.readFile to first reject non-regular files by verifying stats.isFile() is
true (using the stats object returned from fs.stat for input.filePath), and only
then check stats.size against MAX_FILE_SIZE and proceed to call fs.readFile;
ensure the function returns { ok: false, reason: "not-file" } (or similar) for
non-regular files and keeps the existing { ok: false, reason: "too-large" }
behavior for oversized regular files.
Collaborator
Author
|
Closing as stale: created in Jan-Mar with no recent activity. If still relevant, re-open or re-create from a fresh branch. Bulk audit 2026-05-06. |
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
filesystem.readFileendpoint for reading files by absolute pathTest plan
Summary by cubic
Adds parent directory navigation and external file support across the desktop file browser and viewer. Stores absolute paths in
FileViewerStateand usespathefor robust, cross‑platform path handling while showing worktree‑relative paths in the UI when possible.New Features
filesystem.readFile(2MB cap, binary detection), and displays a relative path when the file is inside the workspace; image/diff only for in-worktree files.pathe(resolve,relative,basename,dirname,normalize,isAbsolute).Bug Fixes
Written for commit e65c0f3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior
Documentation