feat(workspace): trees + diffs sidepanel via daemon port - #46
Conversation
Replace the workspace file tree, read-only code viewer, and hand-rolled diff parser with @pierre/trees and @pierre/diffs. Add inline file editing, a top-level Diffs tab, and port the workspace presenter to the daemon so the feature works in web/headless mode too. Monaco + CodeMirror are removed; chat file links open the diff for in-workspace files.
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe workspace gains filesystem editing routes, daemon support, Trees navigation, Pierre-based code and diff views, inline editing, Git diff rendering, and watcher-driven updates. Monaco workspace viewers and related infrastructure are removed. ChangesTrees and Diffs workspace
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceUI
participant WorkspaceClient
participant DaemonDispatcher
participant WorkspacePresenter
participant FileSystem
User->>WorkspaceUI: Open workspace or Diffs tab
WorkspaceUI->>WorkspaceClient: Load tree, status, or diff
WorkspaceClient->>DaemonDispatcher: Send workspace route
DaemonDispatcher->>WorkspacePresenter: Validate and execute request
WorkspacePresenter->>FileSystem: Read, write, watch, or inspect workspace
FileSystem-->>WorkspacePresenter: Workspace data or invalidation
WorkspacePresenter-->>DaemonDispatcher: Route response
DaemonDispatcher-->>WorkspaceClient: Validated response
WorkspaceClient-->>WorkspaceUI: Update tree, editor, or diff view
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
React Doctor found 22 new issues in 10 files · 7 errors & 15 warnings · score 53 / 100 (Critical) · 30 fixed · vs Errors
15 warnings
Reviewed by React Doctor for commit |
Confidence Score: 1/5The PR is not safe to merge until external-link authorization is separated from mutation privileges and workspace search is contained across symlinks. The daemon currently permits an externally resolved markdown file to be overwritten or deleted and allows recursive search to escape the workspace or enter symlink cycles. Files Needing Attention: apps/daemon/src/workspace/daemonWorkspacePresenter.ts
|
| Filename | Overview |
|---|---|
| apps/daemon/src/workspace/daemonWorkspacePresenter.ts | Implements the daemon workspace port, but external-link authorization leaks into mutation routes and recursive search does not contain symlink traversal. |
| apps/daemon/src/dispatch/daemonDispatcher.ts | Wires the new workspace contracts to presenter methods; the handlers expose the presenter’s filesystem-boundary defects to authenticated route callers. |
| apps/daemon/src/index.ts | Instantiates the workspace presenter and adds the raw preview endpoint with allow-list enforcement. |
| packages/shared-contracts/src/routes/workspace.routes.ts | Defines the expanded workspace route schemas, including path mutation, link resolution, Git, and search operations. |
| packages/ui/src/components/sidepanel/TreesFileTree.tsx | Replaces the custom file-node renderer with the Pierre tree and adds create, rename, move, delete, search, and Git-status interactions. |
| packages/ui/src/components/sidepanel/DiffsPanel.tsx | Adds the top-level staged and unstaged diff browsing experience. |
| packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx | Adds inline text editing and save handling through the workspace client. |
Prompt To Fix All With AI
### Issue 1
apps/daemon/src/workspace/daemonWorkspacePresenter.ts:520
**External links grant mutation access**
When an authenticated client resolves an existing absolute or `file://` path outside the workspace, `authorizeExactFile` adds it to the same allow-list used by `writeFile`, `deletePath`, and `renameOrMovePath`, allowing that host file to be overwritten, deleted, or moved. Restrict exact-file authorization to preview/read operations rather than treating it as general workspace authorization.
**How this was verified:** The resolved external path is inserted into `allowedExactPaths`, and each mutation method authorizes its target through `isPathAllowed`, which accepts that set.
### Issue 2
apps/daemon/src/workspace/daemonWorkspacePresenter.ts:783-792
**Search escapes through directory symlinks**
When a registered workspace contains a directory symlink, `stat` follows its target and the recursive search continues without rechecking containment or tracking visited directories. A link outside the workspace exposes external filenames and paths, while a link to an ancestor can repeatedly traverse the same directories and consume daemon CPU and I/O.
**How this was verified:** `searchFiles` checks only the initial root, while `collectSearchMatches` follows each child with `stat` and recursively visits directories without containment or cycle checks.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(workspace): trees+diffs sidepanel, ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!stat.isFile()) return null; | ||
| // Authorize the resolved file for subsequent preview/open reads, even when it | ||
| // lives outside a registered workspace (e.g. a chat link to another project). | ||
| const normalizedPath = this.authorizeExactFile(resolvedPath); |
There was a problem hiding this comment.
When an authenticated client resolves an existing absolute or file:// path outside the workspace, authorizeExactFile adds it to the same allow-list used by writeFile, deletePath, and renameOrMovePath, allowing that host file to be overwritten, deleted, or moved. Restrict exact-file authorization to preview/read operations rather than treating it as general workspace authorization.
How this was verified: The resolved external path is inserted into allowedExactPaths, and each mutation method authorizes its target through isPathAllowed, which accepts that set.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/workspace/daemonWorkspacePresenter.ts
Line: 520
Comment:
**External links grant mutation access**
When an authenticated client resolves an existing absolute or `file://` path outside the workspace, `authorizeExactFile` adds it to the same allow-list used by `writeFile`, `deletePath`, and `renameOrMovePath`, allowing that host file to be overwritten, deleted, or moved. Restrict exact-file authorization to preview/read operations rather than treating it as general workspace authorization.
**How this was verified:** The resolved external path is inserted into `allowedExactPaths`, and each mutation method authorizes its target through `isPathAllowed`, which accepts that set.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| isDirectory = (await fsp.stat(childPath)).isDirectory(); | ||
| } catch { | ||
| continue; | ||
| } | ||
| const relativePath = relativePrefix ? `${relativePrefix}/${name}` : name; | ||
| if (name.toLowerCase().includes(needle)) { | ||
| results.push({ name, path: childPath, isDirectory }); | ||
| } | ||
| if (isDirectory) { | ||
| await this.collectSearchMatches(childPath, needle, relativePath, results, limit); |
There was a problem hiding this comment.
When a registered workspace contains a directory symlink, stat follows its target and the recursive search continues without rechecking containment or tracking visited directories. A link outside the workspace exposes external filenames and paths, while a link to an ancestor can repeatedly traverse the same directories and consume daemon CPU and I/O.
How this was verified: searchFiles checks only the initial root, while collectSearchMatches follows each child with stat and recursively visits directories without containment or cycle checks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/workspace/daemonWorkspacePresenter.ts
Line: 783-792
Comment:
**Search escapes through directory symlinks**
When a registered workspace contains a directory symlink, `stat` follows its target and the recursive search continues without rechecking containment or tracking visited directories. A link outside the workspace exposes external filenames and paths, while a link to an ancestor can repeatedly traverse the same directories and consume daemon CPU and I/O.
**How this was verified:** `searchFiles` checks only the initial root, while `collectSearchMatches` follows each child with `stat` and recursively visits directories without containment or cycle checks.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Pull request overview
This PR modernizes the workspace sidepanel by replacing the custom file tree, Monaco-based read-only viewer, and bespoke unified-diff renderer with @pierre/trees and @pierre/diffs, while also porting workspace routes/presenter functionality to the daemon so the same feature set can run in web/headless mode.
Changes:
- Replace workspace file tree + diff/code viewers with Pierre components, adding inline file editing and a top-level Diffs tab.
- Add new workspace route contracts + UI client wrappers for text reads and filesystem mutations (write/create/delete/move).
- Implement daemon-side workspace presenter (FS + git + watchers + preview endpoint) and wire workspace routes into the daemon dispatcher; remove Monaco tooling/deps.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates rich-rendering description (removes CodeMirror mention). |
| packages/ui/vite.config.ts | Removes Monaco Vite plugin wiring and related optimizeDeps entries. |
| packages/ui/src/stores/ui/sidepanel.ts | Adds Diffs tab state/actions (openDiffs, selection gating). |
| packages/ui/src/components/workspace/WorkspaceFileNode.tsx | Deletes legacy hand-rolled file tree node renderer. |
| packages/ui/src/components/trace/TraceDialog.tsx | Replaces Monaco JSON viewer with DiffsCodePane. |
| packages/ui/src/components/sidepanel/WorkspaceViewer.tsx | Adds view/edit mode (inline editing) and swaps diff/code panes to Pierre-based panes. |
| packages/ui/src/components/sidepanel/WorkspacePanel.tsx | Replaces legacy tree with TreesFileTree and removes Git section UI in favor of Diffs surfaces. |
| packages/ui/src/components/sidepanel/viewer/WorkspaceDiffView.tsx | Deletes custom unified diff renderer. |
| packages/ui/src/components/sidepanel/viewer/WorkspaceCodePane.tsx | Deletes Monaco-based read-only code viewer. |
| packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx | New <PatchDiff> wrapper that splits multi-file patches for rendering. |
| packages/ui/src/components/sidepanel/viewer/diffsOptions.ts | Centralizes @pierre/diffs theme options based on app theme. |
| packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx | New inline editable file surface using @pierre/diffs editor + save workflow. |
| packages/ui/src/components/sidepanel/viewer/DiffsCodePane.tsx | New read-only code viewer using @pierre/diffs <File>. |
| packages/ui/src/components/sidepanel/TreesFileTree.tsx | New adapter for @pierre/trees tree with rename/dnd/create/delete + git status wiring. |
| packages/ui/src/components/sidepanel/DiffsPanel.tsx | New top-level Diffs tab that lists changes + renders patches. |
| packages/ui/src/components/sidepanel/ChatSidePanel.tsx | Adds Diffs tab button and routes to <DiffsPanel>. |
| packages/ui/src/components/markdown/useMarkdownLinkNavigation.ts | Routes in-workspace file links to Diffs tab selection; normalizes local file hrefs. |
| packages/ui/package.json | Adds @pierre/diffs/@pierre/trees; removes Monaco-related deps. |
| packages/ui/api/WorkspaceClient.ts | Adds typed wrappers for new workspace edit routes. |
| packages/shared/src/types/presenters/workspace.d.ts | Adds "diffs" tab + new workspace presenter edit APIs. |
| packages/shared-contracts/src/routes/workspace.routes.ts | Adds route contracts for readFileText/write/create/delete/renameOrMove. |
| packages/shared-contracts/src/routes.ts | Registers new workspace routes in ARGOS_ROUTE_CATALOG. |
| packages/shared-contracts/src/domainSchemas.ts | Coerces file metadata dates to handle daemon JSON transport. |
| packages/shared-contracts/src/desktop-only.ts | Documents which workspace routes remain desktop-only (open/reveal). |
| docs/features/trees-diffs-workspace/tasks.md | Tracks implementation work items for Trees+Diffs workspace. |
| docs/features/trees-diffs-workspace/spec.md | Defines feature spec/ACs/constraints (now needs alignment with implementation). |
| docs/features/trees-diffs-workspace/plan.md | Documents implementation plan and architecture boundaries. |
| bun.lock | Updates lockfile for new deps (Pierre libs, chokidar, shiki updates, removals). |
| apps/landing/src/components/Spotlight.tsx | Updates marketing copy to remove CodeMirror wording. |
| apps/desktop/test/main/presenter/workspacePresenter.test.ts | Adds tests for read/write/create/delete/rename workspace operations. |
| apps/desktop/src/main/routes/index.ts | Adds route cases for new workspace edit routes (desktop dispatcher). |
| apps/desktop/src/main/presenter/workspacePresenter/index.ts | Implements readFileText/write/create/delete/rename with allow-list + binary sniffing. |
| apps/daemon/src/workspace/daemonWorkspacePresenter.ts | Adds daemon-side workspace presenter (FS/git/watchers/preview URL). |
| apps/daemon/src/index.ts | Adds /api/v1/workspace/preview endpoint and daemon presenter instantiation/baseUrl wiring. |
| apps/daemon/src/dispatch/daemonDispatcher.ts | Wires all workspace routes into the daemon dispatcher when presenter provided. |
| apps/daemon/package.json | Adds chokidar dependency (and dependency ordering adjustments). |
Suppressed comments (1)
docs/features/trees-diffs-workspace/spec.md:60
- The Non-Goals section still says porting write/edit routes to the daemon is out of scope, but the PR adds
DaemonWorkspacePresenterwith write/edit routes. This bullet is now incorrect.
## Non-Goals
- Replace the markdown/html/image/pdf preview pane with `@pierre/diffs`.
- Port write/edit routes to the daemon (web/headless mode stays read-only for workspace files in this iteration).
- Multi-file staging/unstaging/commit actions in the Diffs tab (future work).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| async writeFile(filePath: string, content: string): Promise<void> { | ||
| if (!this.isPathAllowed(filePath)) { | ||
| console.warn(`[Workspace] Blocked write attempt for unauthorized path: ${filePath}`); | ||
| return; | ||
| } |
| async writeFile(filePath: string, content: string): Promise<void> { | ||
| if (!this.isPathAllowed(filePath)) return; | ||
| const normalizedPath = path.resolve(filePath); | ||
| await fsp.mkdir(path.dirname(normalizedPath), { recursive: true }); | ||
| await fsp.writeFile(normalizedPath, content, "utf8"); | ||
| } |
| const handleSelectionChange = useCallback( | ||
| (selected: readonly string[]) => { | ||
| const first = selected[0]; | ||
| if (!first) return; | ||
| sidepanelStore.selectFile(sessionId, toAbsolutePath(workspacePath, first), { open: false }); | ||
| }, | ||
| [sessionId, sidepanelStore, workspacePath], | ||
| ); |
|
|
||
| - Follow the typed route/client boundary: new capabilities go through `shared-contracts/routes`, `routes/index.ts` dispatcher, `WorkspacePresenter`, and `WorkspaceClient`. No new `window.api`/legacy paths. | ||
| - All filesystem writes must be inside a registered workspace/workdir (security boundary already enforced by `isPathAllowed`). | ||
| - Desktop is the primary target (the presenter lives in main). The daemon/headless path only needs to remain non-breaking for the existing read routes; write routes are desktop-only for now. |
| - **AC-5** Context menu + tree affordances support "New File" and "New Folder" creation and "Delete". | ||
| - **AC-6** Git-status row signals (added/modified/deleted/untracked/...) render in the tree via Trees' built-in `gitStatus`. | ||
| - **AC-7** Selecting a changed file in the Git section renders its diff with `@pierre/diffs <PatchDiff>` (staged + unstaged). | ||
| - **AC-8** A new top-level **Diffs** tab exists beside Workspace/Browser; it lists all changed files (from `getGitStatus`) and renders them via `@pierre/diffs <CodeView>` (virtualized multi-file) using the workspace's unified diff. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
docs/features/trees-diffs-workspace/tasks.md-57-60 (1)
57-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep renderer verification status accurate.
Presenter tests do not cover
TreesFileTree,DiffsPanel, orDiffsEditorPane. Line 60 also treats manual verification as coverage, but the PR status says manual testing is still pending. Complete AC-1 through AC-10 manual checks before claiming coverage, or add renderer or end-to-end tests for rename, drag-and-drop, deletion, patch selection, save, and invalidation refresh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/trees-diffs-workspace/tasks.md` around lines 57 - 60, Update the Tests section around item 9.2 to accurately reflect renderer verification: either complete and document manual checks for AC-1 through AC-10, including the listed tree and diff interactions, or add renderer/end-to-end coverage for them. Do not claim coverage from presenter tests alone or mark manual verification complete while it remains pending.docs/features/trees-diffs-workspace/spec.md-49-60 (1)
49-60: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the daemon and desktop route split consistent.
The current route classification supports daemon-backed workspace operations and desktop-only shell operations, but the feature records describe a different contract.
docs/features/trees-diffs-workspace/spec.md#L49-L60: state that workspace filesystem, Git, edit, search, watcher, and preview routes run through the daemon; keep onlyworkspace.revealFileInFolderandworkspace.openFiledesktop-only.docs/features/trees-diffs-workspace/plan.md#L120-L123: replace the statement that write routes return a desktop-only error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/trees-diffs-workspace/spec.md` around lines 49 - 60, The route contract documentation is inconsistent with the intended daemon/desktop split. In docs/features/trees-diffs-workspace/spec.md lines 49-60, state that workspace filesystem, Git, edit, search, watcher, and preview routes use the daemon, with only workspace.revealFileInFolder and workspace.openFile remaining desktop-only. In docs/features/trees-diffs-workspace/plan.md lines 120-123, replace the statement that write routes return a desktop-only error to match this daemon-backed contract.packages/ui/src/components/sidepanel/TreesFileTree.tsx-149-156 (1)
149-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip directory selections.
Trees reports directory paths with a trailing slash.
toAbsolutePathstrips that slash, so a directory click callsselectFilewith a directory path. The viewer then requests a file preview for a directory.🛡️ Proposed guard
(selected: readonly string[]) => { const first = selected[0]; if (!first) return; + if (first.endsWith("/")) return; sidepanelStore.selectFile(sessionId, toAbsolutePath(workspacePath, first), { open: false }); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx` around lines 149 - 156, Update handleSelectionChange to ignore directory selections before calling selectFile: detect the trailing slash on the selected path and return early for directories. Preserve file selection behavior and only pass normalized file paths to sidepanelStore.selectFile.packages/ui/src/components/sidepanel/DiffsPanel.tsx-226-226 (1)
226-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBoth ternary branches return the same value.
themeStore.isDark ? "unified" : "unified"always resolves to"unified". The theme read has no effect. State the intended value, or select the correct alternative style.🐛 Proposed fix
- <DiffsPatchPane patch={patch} diffStyle={themeStore.isDark ? "unified" : "unified"} /> + <DiffsPatchPane patch={patch} diffStyle="unified" />Remove the now unused
themeStorebinding at Line 40 if no other code uses it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/DiffsPanel.tsx` at line 226, Update the DiffsPatchPane invocation to remove the redundant themeStore.isDark ternary and pass the intended diffStyle value directly, or select the correct alternate style if dark mode should differ; remove the themeStore binding if it is unused after this change.packages/ui/src/components/markdown/useMarkdownLinkNavigation.ts-132-143 (1)
132-143: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCanonicalize paths before selecting a diff
When a changed file is a symlink,
resolveMarkdownLinkedFilereturns its real path, whilegetGitStatusstores the symlink path. Strict comparison then misses the active file, andgetGitDiffcan target the wrong path. Use one canonical path representation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/markdown/useMarkdownLinkNavigation.ts` around lines 132 - 143, Canonicalize the resolved path before diff selection in the markdown link navigation flow, especially around setDiffsSelection and openDiffs. Ensure the path representation matches the symlink paths used by getGitStatus so active-file comparison and getGitDiff target the correct file, while preserving the existing workspace and external-file branching.packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx-16-24 (1)
16-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHunk-less file diffs disappear silently.
splitIntoFilePatcheskeeps only segments that contain@@. That filter also removes pure renames, mode-only changes, and empty-file additions, not only binary files. If every segment is filtered out, the pane renders "No changes" even though the patch describes real changes.List the dropped file names instead of hiding them.
🛠️ Proposed change
-const splitIntoFilePatches = (patch: string): string[] => { +const parseFileName = (segment: string): string => { + const match = /^diff --git a\/(.+?) b\//m.exec(segment); + return match?.[1] ?? "file"; +}; + +const splitIntoFilePatches = (patch: string): { renderable: string[]; skipped: string[] } => { const segments = patch .split(/(?=^diff --git )/m) .map((segment) => segment.trim()) .filter(Boolean); - // Skip segments with no hunks (e.g. "Binary files … differ") — PatchDiff - // cannot render them. - return segments.filter((segment) => segment.includes("@@")); + // PatchDiff cannot render segments with no hunks (binary files, pure + // renames, mode changes), so report them separately instead of hiding them. + return { + renderable: segments.filter((segment) => segment.includes("@@")), + skipped: segments.filter((segment) => !segment.includes("@@")).map(parseFileName), + }; };Render the
skippednames above the patch list, and show "No changes" only when both lists are empty.Also applies to: 35-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx` around lines 16 - 24, Update splitIntoFilePatches to preserve the names of hunk-less file-diff segments, including renames, mode-only changes, and empty-file additions, while continuing to return renderable hunks separately. In the DiffsPatchPane render flow, display the skipped file names above the patch list and show “No changes” only when both renderable patches and skipped names are empty.packages/ui/src/components/sidepanel/WorkspaceViewer.tsx-104-104 (1)
104-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale comment.
The comment states "via Monaco". This PR removes Monaco. The editor is
DiffsEditorPane, backed by@pierre/diffs.📝 Proposed change
- // Edit mode (inline file editing via Monaco). Reset when the file changes. + // Edit mode (inline file editing via DiffsEditorPane). Reset when the file changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx` at line 104, Update the comment above the edit-mode reset logic in WorkspaceViewer to remove the outdated Monaco reference and identify DiffsEditorPane, backed by `@pierre/diffs`, as the editor used for inline file editing.packages/ui/src/components/sidepanel/WorkspaceViewer.tsx-114-127 (1)
114-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the failure when the file cannot be read for editing.
If
readFileTextreturnscontent: null, the function returns without entering edit mode and without any message. The user sees the Edit button do nothing. Both presenters returncontent: nullfor a blocked path, a non-file, a file aboveREAD_TEXT_MAX_BYTES, a binary file, or a read error (apps/daemon/src/workspace/daemonWorkspacePresenter.tslines 552-570). ThecanEditcheck only testsfilePreview.kind === "text", so the size limit and the read-error path remain reachable.Show a toast, as
DiffsEditorPanedoes for save failures.🛠️ Proposed change
try { const result = await workspaceClient.readFileText(openFilePath); - if (result.content === null) return; + if (result.content === null) { + toast.error(result.exists ? "This file cannot be edited" : "File not found"); + return; + } setEditContent(result.content); setEditMode(true); } catch (error) { console.error("[WorkspaceViewer] failed to load file for editing", error); + toast.error("Failed to open the file for editing"); } finally {Add the import:
import { toast } from "sonner";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx` around lines 114 - 127, Update enterEditMode in WorkspaceViewer to import and use sonner’s toast when readFileText returns content: null, informing the user that the file could not be loaded for editing before returning. Keep the existing error logging for thrown failures, and ensure the loading state is still cleared through finally.packages/ui/src/components/sidepanel/WorkspaceViewer.tsx-214-221 (1)
214-221: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRefresh the preview before leaving edit mode.
writeFiletriggers a debounced watcher refresh, butonSaved={exitEditMode}switches to read-only mode immediately.filePreviewcan therefore show stale content until the refresh completes. Refresh the selected preview before callingexitEditMode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx` around lines 214 - 221, Update the edit-mode save flow in WorkspaceViewer so the selected file preview is refreshed after the DiffsEditorPane save completes and before exitEditMode switches back to read-only mode. Ensure the refreshed preview is available before invoking exitEditMode, rather than passing exitEditMode directly as onSaved.packages/ui/src/components/sidepanel/viewer/DiffsCodePane.tsx-5-10 (1)
5-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
source.languageto<File>.@pierre/diffsusesnamefor language detection andlangas the override. Artifact IDs do not guarantee a recognized extension, so artifact code can render as plain text. Addlang: source.language ?? undefinedand includesource.languagein theuseMemodependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/viewer/DiffsCodePane.tsx` around lines 5 - 10, Update the File rendering logic in DiffsCodePane to pass the source language as the lang override using source.language ?? undefined, and add source.language to the related useMemo dependency array so changes recompute the rendered output.
🧹 Nitpick comments (12)
packages/ui/src/components/sidepanel/TreesFileTree.tsx (1)
194-215: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRefresh the tree after a mutation instead of relying only on the watcher.
handleCreateandhandleDeletedepend on a watcher invalidation event to refresh the paths. The watcher registration at Lines 260-264 can fail and only logs the error. In that case, created and deleted entries never appear or disappear. Callreloadafter a successful mutation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx` around lines 194 - 215, Update handleCreate and handleDelete to call reload immediately after their respective workspaceClient mutations succeed, ensuring the tree refreshes without relying on watcher events. Preserve the existing error handling and do not reload when the mutation fails; include reload in each callback’s dependencies.packages/ui/src/components/sidepanel/WorkspacePanel.tsx (2)
80-87: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
useWorkspaceSyncstill walks the file tree thatTreesFileTreenow owns.The panel no longer consumes
fileTreeorloadingFiles, butuseWorkspaceSynccontinues to build the tree and restore expanded directories throughexpandDirectory.TreesFileTreeperforms its own full walk. The workspace therefore loads twice on every invalidation. Add an option touseWorkspaceSyncthat disables tree synchronization, and enable it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/WorkspacePanel.tsx` around lines 80 - 87, Update useWorkspaceSync to accept an option that disables file-tree synchronization, including tree building and expandDirectory restoration, while preserving its existing behavior by default. Pass this option in the WorkspacePanel useWorkspaceSync call so TreesFileTree remains the sole owner of file-tree loading.
381-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable workspace git-diff path.
selectDiffhas no call sites. RemoveselectedDiffPath,selectedGitDiff, andloadingGitDifffrom the workspace viewer path, or connectselectDiffto a live producer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/WorkspacePanel.tsx` around lines 381 - 393, Remove the unreachable workspace git-diff state and path by deleting selectedDiffPath, selectedGitDiff, and loadingGitDiff along with their related workspace viewer logic; alternatively, wire selectDiff to an active producer. Keep the existing TreesFileTree and artifact rendering behavior unchanged.packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the file path as the React key.
key={index}ties thePatchDiffinstance to the position in the list. When the patch changes and files are added or removed, React reuses the highlighted output of a different file at the same index. Derive the key from thediff --githeader path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx` around lines 48 - 50, Update the filePatches mapping in DiffsPatchPane to use each patch’s path from its diff --git header as the PatchDiff React key instead of the array index, ensuring keys remain stable when files are added, removed, or reordered.packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx (1)
79-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the save shortcut to the editor.
The listener is attached to
window. It callspreventDefault()for Cmd/Ctrl+S while the pane is mounted, even when the focus is in the chat input or in another dialog. Attach the listener to the pane container instead, and check that the container holds the focus.♻️ Proposed change
+ const containerRef = useRef<HTMLDivElement | null>(null); + useEffect(() => { const onKeyDown = (event: KeyboardEvent) => { const isSave = (event.metaKey || event.ctrlKey) && (event.key === "s" || event.key === "S"); - if (isSave) { + if (isSave && containerRef.current?.contains(document.activeElement)) { event.preventDefault(); void handleSave(); } };Then set
ref={containerRef}on the rootdivat line 92.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx` around lines 79 - 89, Scope the Cmd/Ctrl+S handler in the save-shortcut useEffect to the editor pane container instead of window, using the existing containerRef. Only prevent the default and call handleSave when containerRef.current contains the event target or otherwise holds focus, and attach/remove the listener on that container. Set ref={containerRef} on the root div.packages/ui/src/components/sidepanel/WorkspaceViewer.tsx (1)
43-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
createWorkspaceClient()runs during render in both new viewer components. The factory returns a new object on every render, so everyuseCallbackthat depends on the client is recreated on every render.WorkspacePanel.tsxline 71 already wraps the same factory inuseMemo.
packages/ui/src/components/sidepanel/WorkspaceViewer.tsx#L43-L43: wrap the call inuseMemo(() => createWorkspaceClient(), [])soenterEditModekeeps a stable identity.packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx#L27-L27: wrap the call inuseMemo(() => createWorkspaceClient(), [])sohandleSavestops re-registering thekeydownlistener on every render.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx` at line 43, Memoize the createWorkspaceClient() result with useMemo(() => createWorkspaceClient(), []) in WorkspaceViewer.tsx at lines 43-43 so enterEditMode remains stable, and apply the same change in DiffsEditorPane.tsx at lines 27-27 so handleSave does not re-register the keydown listener on each render.packages/ui/vite.config.ts (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove stale Monaco API references. No source file imports Monaco, and no package manifest declares a Monaco dependency. Remove the unused
monacoOptionsprop and call site, then update the remaining Monaco-specific comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/vite.config.ts` at line 59, Remove the unused monacoOptions prop and its call site, then clean up the remaining Monaco-specific comments and stale references in the relevant configuration and component code. Preserve the existing behavior for all non-Monaco dependencies, including the optimizeDeps include entries.apps/daemon/src/index.ts (1)
832-834: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe preview 404 responses skip
withCors.The success path wraps the response with
withCors, but both 404 paths do not. A cross-origin web UI cannot read those responses and reports a network error instead of a missing file.♻️ Proposed change
- if (!targetPath || !workspacePresenter.isPathAllowed(targetPath)) { - return new Response("Not found", { status: 404 }); - } + const notFound = () => withCors(new Response("Not found", { status: 404 })); + if (!targetPath || !workspacePresenter.isPathAllowed(targetPath)) { + return notFound(); + } try { const file = Bun.file(targetPath); - if (!(await file.exists())) return new Response("Not found", { status: 404 }); + if (!(await file.exists())) return notFound();Apply the same helper to the
catchbranch.Also applies to: 843-845
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/daemon/src/index.ts` around lines 832 - 834, Update the preview handler’s two 404 return paths—the targetPath validation branch and the catch branch—to wrap their Response objects with the same withCors helper used by the success path, preserving the existing 404 status and message.apps/daemon/src/workspace/daemonWorkspacePresenter.ts (2)
227-231: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
destroyleaves the authorization sets populated.
destroyclearswatchRuntimesbut notallowedPathsorallowedExactPaths. The desktop presenter clearsallowedExactPathsin itsdestroy. Clear both sets so a disposed presenter cannot authorize a laterisPathAllowedcall from the HTTP preview endpoint.♻️ Proposed change
destroy(): void { const runtimes = Array.from(this.watchRuntimes.values()); this.watchRuntimes.clear(); for (const runtime of runtimes) void this.disposeRuntime(runtime); + this.allowedPaths.clear(); + this.allowedExactPaths.clear(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/daemon/src/workspace/daemonWorkspacePresenter.ts` around lines 227 - 231, Update daemonWorkspacePresenter.destroy to clear both allowedPaths and allowedExactPaths in addition to watchRuntimes, ensuring subsequent isPathAllowed calls cannot use stale authorization entries after disposal.
690-707: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
appendUntrackedDiffsspawns up to 100 git processes at once.
Promise.allstarts onegit diff --no-indexper untracked file with no concurrency limit. A fresh clone or a large untracked directory produces a process burst that competes with the daemon's own work.Run the diffs in small batches, or reduce
UNTRACKED_FULL_DIFF_MAX_FILES.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/daemon/src/workspace/daemonWorkspacePresenter.ts` around lines 690 - 707, Limit concurrency in appendUntrackedDiffs when invoking runGitDiffNoIndex instead of starting every diff through Promise.all at once. Process untrackedFiles in small sequential batches with a modest fixed batch size, preserve the existing maximum file cap and combined diff ordering, and continue returning unstagedPatch when no diffs are produced or an error occurs.apps/desktop/test/main/presenter/workspacePresenter.test.ts (1)
643-647: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test locks in the silent write denial.
The test asserts only that no file appears. It passes because
writeFilereturns without an error. IfwriteFilestarts to throw on denial (see the comment onapps/desktop/src/main/presenter/workspacePresenter/index.ts), update this test to assert the rejection.💚 Proposed update after the presenter change
it("does not write outside an allowed workspace", async () => { const file = path.join(outsidePath, "nope.txt"); - await presenter.writeFile(file, "x"); + await expect(presenter.writeFile(file, "x")).rejects.toThrow(); expect(fs.existsSync(file)).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/test/main/presenter/workspacePresenter.test.ts` around lines 643 - 647, Update the “does not write outside an allowed workspace” test to assert that presenter.writeFile rejects when given outsidePath, while retaining the assertion that no file is created. Use the test’s existing presenter.writeFile call and match the expected denial error behavior introduced in the presenter.apps/daemon/src/dispatch/daemonDispatcher.ts (1)
1523-1625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard on the presenter once and report a clear error.
Every workspace branch repeats
workspacePresenter && route === .... IfworkspacePresenteris undefined, all fifteen branches fall through to Line 3001 and throwUnknown route: workspace.readFileText, which misreports the cause. The file already uses a clearer pattern forpiProfiles(Line 1196).♻️ Proposed change
+ if (route.startsWith("workspace.") && route !== workspaceBrowseDirectoryRoute.name && !workspacePresenter) { + throw new Error("Workspace presenter is unavailable"); + } + - if (workspacePresenter && route === workspaceRegisterRoute.name) { + if (workspacePresenter && route === workspaceRegisterRoute.name) {Keep the per-branch
workspacePresenter &&for type narrowing, and rely on the new guard for the error message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/daemon/src/dispatch/daemonDispatcher.ts` around lines 1523 - 1625, In the workspace dispatch section of the daemon dispatcher, add a single guard before the route-specific branches that detects an unavailable workspacePresenter and reports a clear presenter-unavailable error for workspace routes. Preserve each branch’s existing workspacePresenter && condition for type narrowing and leave the individual route handling unchanged, following the established piProfiles guard pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/daemon/src/index.ts`:
- Around line 828-846: Update the preview response created in the workspace
preview handler to include X-Content-Type-Options: nosniff and a restrictive
sandbox Content-Security-Policy header, while preserving the existing inferred
content type and no-store cache behavior. Apply the same security headers to
every preview response path that serves workspace HTML, PDF, or SVG content.
- Line 944: Update the workspacePresenter.setBaseUrl call to reuse the host
normalization applied by pluginPresenter.setSettingsBaseUrl, ensuring wildcard
and IPv6 hosts resolve to browser-loadable URLs with correct IPv6 brackets and
an appropriate preview host.
In `@apps/daemon/src/workspace/daemonWorkspacePresenter.ts`:
- Around line 424-474: Update readFilePreview to enforce the existing
READ_TEXT_MAX_BYTES cap using the already available stats before reading
content. In the markdown and image branches, return null or the established
oversized-file response when stats.size exceeds the cap, preventing full reads
and base64 encoding for oversized files while preserving current behavior for
eligible files.
- Around line 761-795: Update collectSearchMatches to use fsp.lstat before
recursing so symbolic links are skipped rather than followed, and add the
SEARCH_MAX_DEPTH constant alongside SEARCH_MAX_RESULTS. Track recursion depth in
collectSearchMatches, stop when the maximum depth is reached, and pass the
incremented depth into recursive calls while preserving existing result-limit
behavior.
In `@apps/desktop/src/main/presenter/workspacePresenter/index.ts`:
- Around line 980-994: Update writeFile in
apps/desktop/src/main/presenter/workspacePresenter/index.ts (Lines 980-994) and
apps/daemon/src/workspace/daemonWorkspacePresenter.ts (Lines 572-577) to throw
an error when isPathAllowed rejects the path instead of returning, so
dispatchers do not report failed writes as successful. Update the matching
denial test in apps/desktop/test/main/presenter/workspacePresenter.test.ts
(Lines 643-647) to expect the rejection.
- Around line 980-1060: Read authorization currently permits markdown-linked
paths to become writable; add a write-only predicate based on
getWorkspaceRootForPath and replace isPathAllowed checks in writeFile,
createEntry, deletePath, and renameOrMovePath. Apply this change in
apps/desktop/src/main/presenter/workspacePresenter/index.ts lines 980-1060 and
apps/daemon/src/workspace/daemonWorkspacePresenter.ts lines 572-601; ensure
read/preview authorization remains unchanged while all write targets require a
registered workspace root.
In `@docs/features/trees-diffs-workspace/spec.md`:
- Around line 40-45: Align the Diffs-tab rendering contract across all three
documents: in docs/features/trees-diffs-workspace/spec.md lines 40-45, choose
either virtualized CodeView or selected-file PatchDiff and make AC-8 consistent
with that behavior; in docs/features/trees-diffs-workspace/plan.md lines 85-89,
update the DiffsPanel responsibility to the same contract; and in
docs/features/trees-diffs-workspace/tasks.md lines 51-55, update task 8.1’s
status and wording to match the implementation and revised acceptance criteria.
In `@docs/features/trees-diffs-workspace/tasks.md`:
- Line 84: Keep the desktop main WorkspacePresenter and its dispatcher
references in place; do not remove it or its route cases until
workspace.revealFileInFolder and workspace.openFile are handled on desktop or
supported by the daemon. Ensure both routes continue calling
runtime.workspacePresenter.
In `@packages/ui/src/components/sidepanel/DiffsPanel.tsx`:
- Around line 116-119: Update the workspacePath effect in DiffsPanel so it skips
its initial mount and only calls resetDiffsSelection and setPatch when
workspacePath changes afterward. Add and use a useRef-based first-render guard,
preserving the existing reset behavior for subsequent workspace changes.
In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx`:
- Around line 98-119: Update the reload flow associated with collectPaths and
onInvalidated to debounce successive invalidation events using a nullable
reloadTimer, clearing the timer during cleanup. When an invalidation only
changes git status, reuse the previously collected path list instead of
rerunning the full tree walk; reserve collectPaths traversal for invalidations
that require structural refresh.
- Around line 296-312: Update the create actions in TreesFileTree’s New File and
New Folder MenuButton handlers to use the tree’s existing inline-create control
or application dialog instead of window.prompt. Ensure the selected parent path
and file-versus-folder flag are passed through to handleCreate, and retain
closing the menu after initiating creation.
In `@packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx`:
- Around line 61-77: Update handleSave to re-read filePath before
workspaceClient.writeFile and compare the current on-disk content with
originalRef.current. If they differ, prompt the user for confirmation and abort
the write when they decline; only write the editor buffer after confirmation or
when no conflict exists, preserving the existing save state and callbacks.
In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx`:
- Around line 109-112: Expose DiffsEditorPane’s internal dirty state through an
optional onDirtyChange callback, invoking it whenever dirty changes. In
WorkspaceViewer, track that state and confirm with the user before exitEditMode
or resetting edit mode when openFilePath changes; only clear editMode and
editContent after confirmation, while preserving the existing behavior for clean
edits.
In `@packages/ui/src/components/trace/TraceDialog.tsx`:
- Around line 296-297: Update the TraceDialog rendering around DiffsCodePane to
add a size guard for formattedJson: render the existing syntax-highlighted
DiffsCodePane only for suitably small bodies, and render the full content as
plain text for large bodies to avoid main-thread tokenization. Preserve the
current trace.json display and layout, and use an appropriate existing
plain-text rendering pattern if available.
---
Minor comments:
In `@docs/features/trees-diffs-workspace/spec.md`:
- Around line 49-60: The route contract documentation is inconsistent with the
intended daemon/desktop split. In docs/features/trees-diffs-workspace/spec.md
lines 49-60, state that workspace filesystem, Git, edit, search, watcher, and
preview routes use the daemon, with only workspace.revealFileInFolder and
workspace.openFile remaining desktop-only. In
docs/features/trees-diffs-workspace/plan.md lines 120-123, replace the statement
that write routes return a desktop-only error to match this daemon-backed
contract.
In `@docs/features/trees-diffs-workspace/tasks.md`:
- Around line 57-60: Update the Tests section around item 9.2 to accurately
reflect renderer verification: either complete and document manual checks for
AC-1 through AC-10, including the listed tree and diff interactions, or add
renderer/end-to-end coverage for them. Do not claim coverage from presenter
tests alone or mark manual verification complete while it remains pending.
In `@packages/ui/src/components/markdown/useMarkdownLinkNavigation.ts`:
- Around line 132-143: Canonicalize the resolved path before diff selection in
the markdown link navigation flow, especially around setDiffsSelection and
openDiffs. Ensure the path representation matches the symlink paths used by
getGitStatus so active-file comparison and getGitDiff target the correct file,
while preserving the existing workspace and external-file branching.
In `@packages/ui/src/components/sidepanel/DiffsPanel.tsx`:
- Line 226: Update the DiffsPatchPane invocation to remove the redundant
themeStore.isDark ternary and pass the intended diffStyle value directly, or
select the correct alternate style if dark mode should differ; remove the
themeStore binding if it is unused after this change.
In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx`:
- Around line 149-156: Update handleSelectionChange to ignore directory
selections before calling selectFile: detect the trailing slash on the selected
path and return early for directories. Preserve file selection behavior and only
pass normalized file paths to sidepanelStore.selectFile.
In `@packages/ui/src/components/sidepanel/viewer/DiffsCodePane.tsx`:
- Around line 5-10: Update the File rendering logic in DiffsCodePane to pass the
source language as the lang override using source.language ?? undefined, and add
source.language to the related useMemo dependency array so changes recompute the
rendered output.
In `@packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx`:
- Around line 16-24: Update splitIntoFilePatches to preserve the names of
hunk-less file-diff segments, including renames, mode-only changes, and
empty-file additions, while continuing to return renderable hunks separately. In
the DiffsPatchPane render flow, display the skipped file names above the patch
list and show “No changes” only when both renderable patches and skipped names
are empty.
In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx`:
- Line 104: Update the comment above the edit-mode reset logic in
WorkspaceViewer to remove the outdated Monaco reference and identify
DiffsEditorPane, backed by `@pierre/diffs`, as the editor used for inline file
editing.
- Around line 114-127: Update enterEditMode in WorkspaceViewer to import and use
sonner’s toast when readFileText returns content: null, informing the user that
the file could not be loaded for editing before returning. Keep the existing
error logging for thrown failures, and ensure the loading state is still cleared
through finally.
- Around line 214-221: Update the edit-mode save flow in WorkspaceViewer so the
selected file preview is refreshed after the DiffsEditorPane save completes and
before exitEditMode switches back to read-only mode. Ensure the refreshed
preview is available before invoking exitEditMode, rather than passing
exitEditMode directly as onSaved.
---
Nitpick comments:
In `@apps/daemon/src/dispatch/daemonDispatcher.ts`:
- Around line 1523-1625: In the workspace dispatch section of the daemon
dispatcher, add a single guard before the route-specific branches that detects
an unavailable workspacePresenter and reports a clear presenter-unavailable
error for workspace routes. Preserve each branch’s existing workspacePresenter
&& condition for type narrowing and leave the individual route handling
unchanged, following the established piProfiles guard pattern.
In `@apps/daemon/src/index.ts`:
- Around line 832-834: Update the preview handler’s two 404 return paths—the
targetPath validation branch and the catch branch—to wrap their Response objects
with the same withCors helper used by the success path, preserving the existing
404 status and message.
In `@apps/daemon/src/workspace/daemonWorkspacePresenter.ts`:
- Around line 227-231: Update daemonWorkspacePresenter.destroy to clear both
allowedPaths and allowedExactPaths in addition to watchRuntimes, ensuring
subsequent isPathAllowed calls cannot use stale authorization entries after
disposal.
- Around line 690-707: Limit concurrency in appendUntrackedDiffs when invoking
runGitDiffNoIndex instead of starting every diff through Promise.all at once.
Process untrackedFiles in small sequential batches with a modest fixed batch
size, preserve the existing maximum file cap and combined diff ordering, and
continue returning unstagedPatch when no diffs are produced or an error occurs.
In `@apps/desktop/test/main/presenter/workspacePresenter.test.ts`:
- Around line 643-647: Update the “does not write outside an allowed workspace”
test to assert that presenter.writeFile rejects when given outsidePath, while
retaining the assertion that no file is created. Use the test’s existing
presenter.writeFile call and match the expected denial error behavior introduced
in the presenter.
In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx`:
- Around line 194-215: Update handleCreate and handleDelete to call reload
immediately after their respective workspaceClient mutations succeed, ensuring
the tree refreshes without relying on watcher events. Preserve the existing
error handling and do not reload when the mutation fails; include reload in each
callback’s dependencies.
In `@packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx`:
- Around line 79-89: Scope the Cmd/Ctrl+S handler in the save-shortcut useEffect
to the editor pane container instead of window, using the existing containerRef.
Only prevent the default and call handleSave when containerRef.current contains
the event target or otherwise holds focus, and attach/remove the listener on
that container. Set ref={containerRef} on the root div.
In `@packages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsx`:
- Around line 48-50: Update the filePatches mapping in DiffsPatchPane to use
each patch’s path from its diff --git header as the PatchDiff React key instead
of the array index, ensuring keys remain stable when files are added, removed,
or reordered.
In `@packages/ui/src/components/sidepanel/WorkspacePanel.tsx`:
- Around line 80-87: Update useWorkspaceSync to accept an option that disables
file-tree synchronization, including tree building and expandDirectory
restoration, while preserving its existing behavior by default. Pass this option
in the WorkspacePanel useWorkspaceSync call so TreesFileTree remains the sole
owner of file-tree loading.
- Around line 381-393: Remove the unreachable workspace git-diff state and path
by deleting selectedDiffPath, selectedGitDiff, and loadingGitDiff along with
their related workspace viewer logic; alternatively, wire selectDiff to an
active producer. Keep the existing TreesFileTree and artifact rendering behavior
unchanged.
In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx`:
- Line 43: Memoize the createWorkspaceClient() result with useMemo(() =>
createWorkspaceClient(), []) in WorkspaceViewer.tsx at lines 43-43 so
enterEditMode remains stable, and apply the same change in DiffsEditorPane.tsx
at lines 27-27 so handleSave does not re-register the keydown listener on each
render.
In `@packages/ui/vite.config.ts`:
- Line 59: Remove the unused monacoOptions prop and its call site, then clean up
the remaining Monaco-specific comments and stale references in the relevant
configuration and component code. Preserve the existing behavior for all
non-Monaco dependencies, including the optimizeDeps include entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f989757-7e13-4094-8447-9d8855192b35
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
README.mdapps/daemon/package.jsonapps/daemon/src/dispatch/daemonDispatcher.tsapps/daemon/src/index.tsapps/daemon/src/workspace/daemonWorkspacePresenter.tsapps/desktop/src/main/presenter/workspacePresenter/index.tsapps/desktop/src/main/routes/index.tsapps/desktop/test/main/presenter/workspacePresenter.test.tsapps/landing/src/components/Spotlight.tsxdocs/features/trees-diffs-workspace/plan.mddocs/features/trees-diffs-workspace/spec.mddocs/features/trees-diffs-workspace/tasks.mdpackages/shared-contracts/src/desktop-only.tspackages/shared-contracts/src/domainSchemas.tspackages/shared-contracts/src/routes.tspackages/shared-contracts/src/routes/workspace.routes.tspackages/shared/src/types/presenters/workspace.d.tspackages/ui/api/WorkspaceClient.tspackages/ui/package.jsonpackages/ui/src/components/markdown/useMarkdownLinkNavigation.tspackages/ui/src/components/sidepanel/ChatSidePanel.tsxpackages/ui/src/components/sidepanel/DiffsPanel.tsxpackages/ui/src/components/sidepanel/TreesFileTree.tsxpackages/ui/src/components/sidepanel/WorkspacePanel.tsxpackages/ui/src/components/sidepanel/WorkspaceViewer.tsxpackages/ui/src/components/sidepanel/viewer/DiffsCodePane.tsxpackages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsxpackages/ui/src/components/sidepanel/viewer/DiffsPatchPane.tsxpackages/ui/src/components/sidepanel/viewer/WorkspaceCodePane.tsxpackages/ui/src/components/sidepanel/viewer/WorkspaceDiffView.tsxpackages/ui/src/components/sidepanel/viewer/diffsOptions.tspackages/ui/src/components/trace/TraceDialog.tsxpackages/ui/src/components/workspace/WorkspaceFileNode.tsxpackages/ui/src/stores/ui/sidepanel.tspackages/ui/vite.config.ts
💤 Files with no reviewable changes (3)
- packages/ui/src/components/sidepanel/viewer/WorkspaceCodePane.tsx
- packages/ui/src/components/workspace/WorkspaceFileNode.tsx
- packages/ui/src/components/sidepanel/viewer/WorkspaceDiffView.tsx
| // Workspace file preview (html/pdf/svg) served as raw bytes. The path must | ||
| // resolve inside a registered/allow-listed workspace; otherwise 404. | ||
| if (url.pathname === "/api/v1/workspace/preview" && request.method === "GET") { | ||
| const targetPath = url.searchParams.get("path"); | ||
| if (!targetPath || !workspacePresenter.isPathAllowed(targetPath)) { | ||
| return new Response("Not found", { status: 404 }); | ||
| } | ||
| try { | ||
| const file = Bun.file(targetPath); | ||
| if (!(await file.exists())) return new Response("Not found", { status: 404 }); | ||
| return withCors( | ||
| new Response(file, { | ||
| headers: { "Content-Type": inferPreviewContentType(targetPath), "Cache-Control": "no-store" }, | ||
| }), | ||
| ); | ||
| } catch { | ||
| return new Response("Not found", { status: 404 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The preview endpoint serves workspace HTML and SVG as active content on the daemon origin.
inferPreviewContentType returns text/html; charset=utf-8 for .html and image/svg+xml for .svg. The response comes from the same origin that serves the API and, in web mode, the web UI (Line 790). Any HTML or SVG file inside a registered workspace therefore runs script with access to that origin. A repository under review is untrusted input.
Add X-Content-Type-Options: nosniff and a sandboxing CSP to the preview response.
🔒 Proposed fix
return withCors(
new Response(file, {
- headers: { "Content-Type": inferPreviewContentType(targetPath), "Cache-Control": "no-store" },
+ headers: {
+ "Content-Type": inferPreviewContentType(targetPath),
+ "Cache-Control": "no-store",
+ "X-Content-Type-Options": "nosniff",
+ "Content-Security-Policy": "sandbox; default-src 'none'; img-src data: blob:; style-src 'unsafe-inline'",
+ },
}),
);Also applies to: 154-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/daemon/src/index.ts` around lines 828 - 846, Update the preview response
created in the workspace preview handler to include X-Content-Type-Options:
nosniff and a restrictive sandbox Content-Security-Policy header, while
preserving the existing inferred content type and no-store cache behavior. Apply
the same security headers to every preview response path that serves workspace
HTML, PDF, or SVG content.
| }); | ||
|
|
||
| const serverPort = (server as any).port ?? port; | ||
| workspacePresenter.setBaseUrl(`http://${host}:${serverPort}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The preview base URL is wrong for wildcard and IPv6 hosts.
host can be 0.0.0.0, ::, or ::1 (see Lines 962-966 and the bracketing at Line 946). setBaseUrl interpolates host directly, so it produces http://0.0.0.0:9527 or http://:::9527. Neither is a URL a browser can load, so previewUrl breaks for HTML, PDF, and SVG previews whenever the daemon binds a wildcard or IPv6 address.
Reuse the same normalization already applied to pluginPresenter.setSettingsBaseUrl.
🐛 Proposed fix
const serverPort = (server as any).port ?? port;
- workspacePresenter.setBaseUrl(`http://${host}:${serverPort}`);
+ const previewHost = host === "0.0.0.0" || host === "::" ? "127.0.0.1" : host.includes(":") ? `[${host}]` : host;
+ workspacePresenter.setBaseUrl(`http://${previewHost}:${serverPort}`);
if (!isNonLoopbackHost(host)) {
const originHost = host === "::1" ? "[::1]" : host;
pluginPresenter.setSettingsBaseUrl(`http://${originHost}:${serverPort}`);
}📝 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.
| workspacePresenter.setBaseUrl(`http://${host}:${serverPort}`); | |
| const serverPort = (server as any).port ?? port; | |
| const previewHost = | |
| host === "0.0.0.0" || host === "::" ? "127.0.0.1" : host.includes(":") ? `[${host}]` : host; | |
| workspacePresenter.setBaseUrl(`http://${previewHost}:${serverPort}`); | |
| if (!isNonLoopbackHost(host)) { | |
| const originHost = host === "::1" ? "[::1]" : host; | |
| pluginPresenter.setSettingsBaseUrl(`http://${originHost}:${serverPort}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/daemon/src/index.ts` at line 944, Update the
workspacePresenter.setBaseUrl call to reuse the host normalization applied by
pluginPresenter.setSettingsBaseUrl, ensuring wildcard and IPv6 hosts resolve to
browser-loadable URLs with correct IPv6 brackets and an appropriate preview
host.
| async readFilePreview(filePath: string): Promise<WorkspaceFilePreview | null> { | ||
| if (!this.isPathAllowed(filePath)) return null; | ||
| let stats: fs.Stats; | ||
| try { | ||
| stats = fs.statSync(filePath); | ||
| if (!stats.isFile()) return null; | ||
| } catch { | ||
| return null; | ||
| } | ||
|
|
||
| const normalizedPath = this.normalizePathForAccess(filePath); | ||
| const workspaceRoot = this.getWorkspaceRootForPath(normalizedPath); | ||
| const extension = path.extname(normalizedPath).toLowerCase(); | ||
| const mimeType = inferMimeType(normalizedPath); | ||
|
|
||
| // Extension decides only the special preview kinds (markdown/html/pdf/svg/image). | ||
| // For everything else (source, config, unknown), sniff the content: a NUL byte | ||
| // in the leading bytes means binary; otherwise treat as text — default to text, | ||
| // detect binary by content rather than an extension allowlist. | ||
| const extensionKind = previewKindFromExtension(extension); | ||
| let kind: WorkspaceFilePreviewKind; | ||
| let content = ""; | ||
| let thumbnail: string | undefined; | ||
|
|
||
| if (extensionKind === undefined) { | ||
| const isBinary = await sniffFileBinary(filePath); | ||
| kind = isBinary ? "binary" : "text"; | ||
| if (kind === "text") { | ||
| try { | ||
| content = await fsp.readFile(filePath, "utf8"); | ||
| } catch { | ||
| content = ""; | ||
| } | ||
| } | ||
| } else { | ||
| kind = extensionKind; | ||
| if (kind === "markdown") { | ||
| try { | ||
| content = await fsp.readFile(filePath, "utf8"); | ||
| } catch { | ||
| content = ""; | ||
| } | ||
| } else if (kind === "image") { | ||
| try { | ||
| content = (await fsp.readFile(filePath)).toString("base64"); | ||
| thumbnail = content; | ||
| } catch { | ||
| content = ""; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
readFilePreview reads the whole file with no size cap.
readFileText guards with READ_TEXT_MAX_BYTES (Line 561). readFilePreview does not. A large text file is read fully into memory, and a large image is additionally base64-encoded, which multiplies the size by about 1.33. Both then travel through the route response.
stats is already available at Line 428. Add the same cap before the read.
🛡️ Proposed fix
if (extensionKind === undefined) {
+ if (stats.size > READ_TEXT_MAX_BYTES) {
+ kind = "binary";
+ } else {
const isBinary = await sniffFileBinary(filePath);
kind = isBinary ? "binary" : "text";
if (kind === "text") {
try {
content = await fsp.readFile(filePath, "utf8");
} catch {
content = "";
}
}
+ }
} else {Apply an equivalent guard to the markdown and image branches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/daemon/src/workspace/daemonWorkspacePresenter.ts` around lines 424 -
474, Update readFilePreview to enforce the existing READ_TEXT_MAX_BYTES cap
using the already available stats before reading content. In the markdown and
image branches, return null or the established oversized-file response when
stats.size exceeds the cap, preventing full reads and base64 encoding for
oversized files while preserving current behavior for eligible files.
| const collectPaths = useCallback( | ||
| async (dirPath: string, depth: number, acc: string[]): Promise<void> => { | ||
| if (depth > MAX_TREE_DEPTH || acc.length > MAX_TREE_NODES) return; | ||
| let nodes: WorkspaceFileNode[]; | ||
| try { | ||
| nodes = (await workspaceClient.expandDirectory(dirPath)) as WorkspaceFileNode[]; | ||
| } catch { | ||
| return; | ||
| } | ||
| for (const node of nodes) { | ||
| if (acc.length > MAX_TREE_NODES) break; | ||
| const relativePath = toRelativePath(workspacePath, node.path); | ||
| // Trees infers "directory" from a trailing slash (canonical dir path), so | ||
| // mark dirs explicitly — otherwise empty folders render as files. | ||
| acc.push(node.isDirectory ? `${relativePath}/` : relativePath); | ||
| if (node.isDirectory) { | ||
| await collectPaths(node.path, depth + 1, acc); | ||
| } | ||
| } | ||
| }, | ||
| [workspaceClient, workspacePath], | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Debounce the reload and avoid a full tree re-walk on every invalidation.
collectPaths walks the whole workspace with one sequential expandDirectory call per directory, up to 8000 nodes. reload runs on every onInvalidated event. A build or a branch switch emits many invalidation events, so the panel issues thousands of sequential IPC calls repeatedly. Add a debounce window and reuse the previous path list when only git status changed.
♻️ Proposed debounce
off = workspaceClient.onInvalidated((payload) => {
if (payload.workspacePath !== workspacePath) return;
- void reload();
+ if (reloadTimer !== null) window.clearTimeout(reloadTimer);
+ reloadTimer = window.setTimeout(() => {
+ reloadTimer = null;
+ void reload();
+ }, 250);
});Declare let reloadTimer: number | null = null; next to off, and clear it in the cleanup function.
Also applies to: 269-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx` around lines 98 -
119, Update the reload flow associated with collectPaths and onInvalidated to
debounce successive invalidation events using a nullable reloadTimer, clearing
the timer during cleanup. When an invalidation only changes git status, reuse
the previously collected path list instead of rerunning the full tree walk;
reserve collectPaths traversal for invalidations that require structural
refresh.
| label="New File" | ||
| icon="lucide:file-plus" | ||
| onClick={() => { | ||
| const name = window.prompt("New file name"); | ||
| if (name) void handleCreate(item.path, name, false); | ||
| close(); | ||
| }} | ||
| /> | ||
| <MenuButton | ||
| label="New Folder" | ||
| icon="lucide:folder-plus" | ||
| onClick={() => { | ||
| const name = window.prompt("New folder name"); | ||
| if (name) void handleCreate(item.path, name, true); | ||
| close(); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Electron support window.prompt in the renderer process?
💡 Result:
Electron does not support window.prompt in the renderer process [1][2][3]. Attempts to call window.prompt will throw an error stating that the function is not supported [1][2][4]. Electron maintains this design for several reasons, primarily because the standard window.prompt implementation in web browsers is synchronous and blocks the main UI thread, which is generally avoided in Electron's architecture [5][4][6]. Additionally, Electron does not provide a built-in native equivalent, and there are no current plans to implement one [7][5]. If you need to prompt a user for input in an Electron application, the recommended approach is to build a custom solution [5]: 1. Create a separate BrowserWindow to act as a custom modal dialog [7][4]. 2. Use the inter-process communication (IPC) modules (ipcMain and ipcRenderer) to communicate between your renderer process and the main process to handle the input flow [4][8]. 3. For simpler tasks, you can also use the built-in dialog API, though it is not a direct drop-in replacement for window.prompt [9].
Citations:
- 1: https://github.com/electron/electron/blob/c16c6da2/lib/renderer/window-setup.ts
- 2: https://github.com/electron/electron/blob/1ad832a4/lib/renderer/window-setup.ts
- 3: https://stackoverflow.com/questions/38041293/electron-dialog-box-with-window-prompt
- 4: [Feature Request]: Custom window.prompt / alert Middleware electron/electron#31560
- 5: Why prompt is not supported? electron/electron#472
- 6: Why prompt is not supported? electron/electron#472
- 7: [Bug]: Prompt window not loaded in browserWindow electron/electron#40341
- 8: https://github.com/atom/electron/blob/master/docs/api/ipc-renderer.md
- 9: https://electronjs.org/docs/latest/api/dialog
🏁 Script executed:
#!/bin/bash
set -eu
file="packages/ui/src/components/sidepanel/TreesFileTree.tsx"
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" 2>/dev/null || true
printf '%s\n' '--- relevant source ---'
sed -n '1,380p' "$file"
printf '%s\n' '--- prompt and create-action references ---'
rg -n -C 3 'window\.prompt|handleCreate|New File here|New File|New Folder' packages/ui/srcRepository: dvaJi/argos
Length of output: 21104
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Electron declarations and renderer setup ---'
rg -n -i -C 3 'electron|window\.prompt|prompt\s*=|prompt\(' \
package.json packages apps electron . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' \
2>/dev/null | head -n 240
printf '%s\n' '--- relevant package manifests ---'
find . -maxdepth 4 -type f \( -name package.json -o -name '*electron*' \) -print | head -n 120Repository: dvaJi/argos
Length of output: 16880
Replace window.prompt with the tree’s inline-create control or an application dialog. The three create actions call window.prompt, which Electron does not support in the renderer. They fail instead of collecting a name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/sidepanel/TreesFileTree.tsx` around lines 296 -
312, Update the create actions in TreesFileTree’s New File and New Folder
MenuButton handlers to use the tree’s existing inline-create control or
application dialog instead of window.prompt. Ensure the selected parent path and
file-versus-folder flag are passed through to handleCreate, and retain closing
the menu after initiating creation.
| const handleSave = useCallback(async () => { | ||
| const editor = editorRef.current; | ||
| if (!editor || !dirty || saving) return; | ||
| setSaving(true); | ||
| try { | ||
| const text = editor.getText(); | ||
| await workspaceClient.writeFile(filePath, text); | ||
| originalRef.current = text; | ||
| setDirty(false); | ||
| onSaved?.(); | ||
| } catch (error) { | ||
| console.error("[DiffsEditorPane] save failed", error); | ||
| toast.error(`Failed to save ${fileBasename}`); | ||
| } finally { | ||
| setSaving(false); | ||
| } | ||
| }, [dirty, saving, editorRef, workspaceClient, filePath, onSaved, fileBasename]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The save overwrites concurrent external changes.
handleSave writes the full editor buffer to filePath. It does not compare against the on-disk state. In this workspace an agent or an external tool can modify the same file while the editor is open. The watcher in DiffsPanel.tsx confirms that external writes occur. The save then discards those changes with no warning.
Detect the conflict before you write. Re-read the file and compare it against originalRef.current, then prompt the user if the two differ.
🛠️ Proposed change
setSaving(true);
try {
const text = editor.getText();
+ const current = await workspaceClient.readFileText(filePath);
+ if (current.content !== null && current.content !== originalRef.current) {
+ toast.error(`${fileBasename} changed on disk. Reopen the file before you save.`);
+ return;
+ }
await workspaceClient.writeFile(filePath, text);📝 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.
| const handleSave = useCallback(async () => { | |
| const editor = editorRef.current; | |
| if (!editor || !dirty || saving) return; | |
| setSaving(true); | |
| try { | |
| const text = editor.getText(); | |
| await workspaceClient.writeFile(filePath, text); | |
| originalRef.current = text; | |
| setDirty(false); | |
| onSaved?.(); | |
| } catch (error) { | |
| console.error("[DiffsEditorPane] save failed", error); | |
| toast.error(`Failed to save ${fileBasename}`); | |
| } finally { | |
| setSaving(false); | |
| } | |
| }, [dirty, saving, editorRef, workspaceClient, filePath, onSaved, fileBasename]); | |
| const handleSave = useCallback(async () => { | |
| const editor = editorRef.current; | |
| if (!editor || !dirty || saving) return; | |
| setSaving(true); | |
| try { | |
| const text = editor.getText(); | |
| const current = await workspaceClient.readFileText(filePath); | |
| if (current.content !== null && current.content !== originalRef.current) { | |
| toast.error(`${fileBasename} changed on disk. Reopen the file before you save.`); | |
| return; | |
| } | |
| await workspaceClient.writeFile(filePath, text); | |
| originalRef.current = text; | |
| setDirty(false); | |
| onSaved?.(); | |
| } catch (error) { | |
| console.error("[DiffsEditorPane] save failed", error); | |
| toast.error(`Failed to save ${fileBasename}`); | |
| } finally { | |
| setSaving(false); | |
| } | |
| }, [dirty, saving, editorRef, workspaceClient, filePath, onSaved, fileBasename]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/sidepanel/viewer/DiffsEditorPane.tsx` around lines
61 - 77, Update handleSave to re-read filePath before workspaceClient.writeFile
and compare the current on-disk content with originalRef.current. If they
differ, prompt the user for confirmation and abort the write when they decline;
only write the editor buffer after confirmation or when no conflict exists,
preserving the existing save state and callbacks.
| useEffect(() => { | ||
| setEditMode(false); | ||
| setEditContent(""); | ||
| }, [openFilePath]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Unsaved edits are discarded without warning.
Two paths drop the editor content silently:
- Line 109-112: when
openFilePathchanges, the effect setseditModeto false.DiffsEditorPaneunmounts and its unsaved buffer is lost. The user can change the file selection in the tree while the editor is dirty. - Line 129-131:
exitEditMode(the View button) closes the editor with no confirmation.
DiffsEditorPane tracks dirty internally, so the parent cannot detect the state. Expose the dirty state through a callback prop, then confirm before you leave edit mode.
🛠️ Sketch of the fix
In DiffsEditorPane.tsx, add an onDirtyChange?: (dirty: boolean) => void prop and call it when dirty changes. In WorkspaceViewer.tsx:
+ const [editDirty, setEditDirty] = useState(false);
+
const exitEditMode = useCallback(() => {
+ if (editDirty && !window.confirm("Discard unsaved changes?")) return;
setEditMode(false);
- }, []);
+ }, [editDirty]);Apply the same guard, or an autosave, before the file-change reset.
Also applies to: 129-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/sidepanel/WorkspaceViewer.tsx` around lines 109 -
112, Expose DiffsEditorPane’s internal dirty state through an optional
onDirtyChange callback, invoking it whenever dirty changes. In WorkspaceViewer,
track that state and confirm with the user before exitEditMode or resetting edit
mode when openFilePath changes; only clear editMode and editContent after
confirmation, while preserving the existing behavior for clean edits.
| <div className="flex-1 min-h-0 bg-muted/30"> | ||
| <DiffsCodePane source={{ id: "trace-body", content: formattedJson, name: "trace.json" }} /> |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Large trace bodies now highlight on the main thread.
DiffsCodePane renders <File ... disableWorkerPool /> (DiffsCodePane.tsx line 51). Shiki then tokenizes on the main thread. formattedJson contains the full request body, and the truncated flag on the trace record shows that these payloads can be large. A large body freezes the dialog and the rest of the renderer while it highlights. The previous implementation used a virtualized editor with a plain-text fallback.
Add a size guard that falls back to plain text.
🛠️ Proposed change
+const MAX_HIGHLIGHT_BYTES = 200_000;
...
<div className="flex-1 min-h-0 bg-muted/30">
- <DiffsCodePane source={{ id: "trace-body", content: formattedJson, name: "trace.json" }} />
+ {formattedJson.length > MAX_HIGHLIGHT_BYTES ? (
+ <pre className="h-full overflow-auto p-3 text-xs whitespace-pre-wrap break-words">
+ {formattedJson}
+ </pre>
+ ) : (
+ <DiffsCodePane source={{ id: "trace-body", content: formattedJson, name: "trace.json" }} />
+ )}
</div>📝 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.
| <div className="flex-1 min-h-0 bg-muted/30"> | |
| <DiffsCodePane source={{ id: "trace-body", content: formattedJson, name: "trace.json" }} /> | |
| <div className="flex-1 min-h-0 bg-muted/30"> | |
| {formattedJson.length > MAX_HIGHLIGHT_BYTES ? ( | |
| <pre className="h-full overflow-auto p-3 text-xs whitespace-pre-wrap break-words"> | |
| {formattedJson} | |
| </pre> | |
| ) : ( | |
| <DiffsCodePane source={{ id: "trace-body", content: formattedJson, name: "trace.json" }} /> | |
| )} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/trace/TraceDialog.tsx` around lines 296 - 297,
Update the TraceDialog rendering around DiffsCodePane to add a size guard for
formattedJson: render the existing syntax-highlighted DiffsCodePane only for
suitably small bodies, and render the full content as plain text for large
bodies to avoid main-thread tokenization. Preserve the current trace.json
display and layout, and use an appropriate existing plain-text rendering pattern
if available.
…guard) Greptile P1: split read-authorization from mutation-authorization — exact-file paths resolved from chat links authorize read/preview only, not write/delete/rename (desktop + daemon). Copilot: writeFile now throws on unauthorized so Save surfaces failures instead of reporting false success; search skips symlinked dirs and tracks visited paths to prevent workspace escape/cycles; TreesFileTree ignores directory selections; spec Non-Goals/AC-8 updated to match the daemon-port + PatchDiff implementation.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!workspacePath) return; | ||
| setLoading(true); | ||
| setError(null); | ||
| try { |
There was a problem hiding this comment.
React Doctor · react-hooks-js/todo (error)
This component misses React Compiler's automatic memoization & re-renders more than it should. Rewrite the flagged code so the compiler can optimize it.
Fix → Todo: (BuildHIR::lowerStatement) Handle TryStatement with a finalizer ('finally') clause
| async (filePath: string | null) => { | ||
| if (!workspacePath) return; | ||
| setLoadingPatch(true); | ||
| try { |
There was a problem hiding this comment.
React Doctor · react-hooks-js/todo (error)
This component misses React Compiler's automatic memoization & re-renders more than it should. Rewrite the flagged code so the compiler can optimize it.
Fix → Todo: (BuildHIR::lowerStatement) Handle TryStatement with a finalizer ('finally') clause
| // Reset selection when the workspace changes (different session/project). | ||
| useEffect(() => { | ||
| resetDiffsSelection(); | ||
| setPatch(""); |
There was a problem hiding this comment.
React Doctor · react-doctor/no-adjust-state-on-prop-change (warning)
This effect adjusts state after a prop changes, so users briefly see the stale value.
Fix → Remove the adjustment effect by deriving values during render, resetting the component with a key, or updating related state in the event that changes the prop. Avoid tracking the previous prop in more state, which preserves the duplication. See https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
| // Reset selection when the workspace changes (different session/project). | ||
| useEffect(() => { | ||
| resetDiffsSelection(); | ||
| setPatch(""); |
There was a problem hiding this comment.
React Doctor · react-hooks-js/set-state-in-effect (warning)
This synchronous effect update causes an extra render: Calling setState synchronously within an effect can trigger cascading renders. Prefer deriving or initializing the value before render. If the effect must read a browser API after mount, treat this as advisory or suppress it with // react-doctor-disable-next-line react-hooks-js/set-state-in-effect.
Fix → Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
- Update external systems with the latest state from React.
- Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
| // full-workspace patch by default (null = "All changes", an explicit user choice). | ||
| useEffect(() => { | ||
| if (!selectionReady) return; | ||
| void loadPatch(selectedPath); |
There was a problem hiding this comment.
React Doctor · react-hooks-js/set-state-in-effect (warning)
This synchronous effect update causes an extra render: Calling setState synchronously within an effect can trigger cascading renders. Prefer deriving or initializing the value before render. If the effect must read a browser API after mount, treat this as advisory or suppress it with // react-doctor-disable-next-line react-hooks-js/set-state-in-effect.
Fix → Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
- Update external systems with the latest state from React.
- Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
|
|
||
| export function DiffsPatchPane({ patch, diffStyle = "unified", className }: DiffsPatchPaneProps) { | ||
| const base = useDiffsBaseOptions(); | ||
| const filePatches = useMemo(() => splitIntoFilePatches(patch), [patch]); |
There was a problem hiding this comment.
React Doctor · react-doctor/react-compiler-no-manual-memoization (warning)
React Compiler can cache this value automatically. Verify that removing useMemo preserves behavior before simplifying it.
Fix → Profile compiler-managed code and remove useMemo, useCallback, or memo only when the manual cache no longer carries behavioral or performance intent.
| const base = useDiffsBaseOptions(); | ||
| const filePatches = useMemo(() => splitIntoFilePatches(patch), [patch]); | ||
|
|
||
| const options = useMemo( |
There was a problem hiding this comment.
React Doctor · react-doctor/react-compiler-no-manual-memoization (warning)
React Compiler can cache this value automatically. Verify that removing useMemo preserves behavior before simplifying it.
Fix → Profile compiler-managed code and remove useMemo, useCallback, or memo only when the manual cache no longer carries behavioral or performance intent.
| data-testid="diffs-patch-pane" | ||
| > | ||
| {filePatches.map((filePatch, index) => ( | ||
| <PatchDiff key={index} patch={filePatch} options={options} disableWorkerPool /> |
There was a problem hiding this comment.
React Doctor · react-doctor/no-array-index-as-key (warning)
Your users can see & submit the wrong data when this list reorders or filters, so use a stable id like key={item.id}, not the array index "index".
Fix → Use a stable id from the item, like key={item.id} or key={item.slug}. Index keys break when the list reorders or filters.
| */ | ||
| export function useDiffsBaseOptions() { | ||
| const themeStore = useThemeStore(); | ||
| return useMemo( |
There was a problem hiding this comment.
React Doctor · react-doctor/react-compiler-no-manual-memoization (warning)
React Compiler can cache this value automatically. Verify that removing useMemo preserves behavior before simplifying it.
Fix → Profile compiler-managed code and remove useMemo, useCallback, or memo only when the manual cache no longer carries behavioral or performance intent.
| @@ -227,8 +189,6 @@ export default function TraceDialog({ messageId, sessionId, onClose }: TraceDial | |||
| const resetState = useCallback(() => { | |||
There was a problem hiding this comment.
React Doctor · react-hooks-js/preserve-manual-memoization (error)
This component misses React Compiler's automatic memoization & re-renders more than it should: Compilation Skipped: Existing memoization could not be preserved. Rewrite the flagged code so the compiler can optimize it.
Fix → React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output.
Summary
Replaces the workspace sidepanel''s hand-rolled file tree, read-only Monaco code viewer, and custom unified-diff parser with @pierre/trees and @pierre/diffs, adds inline file editing and a top-level Diffs tab, and ports the workspace presenter to the daemon so the feature works in web/headless mode too. Monaco + CodeMirror are removed entirely.
BEFORE / AFTER
Changes
DaemonWorkspacePresenter(allow-list, FS reads/writes, git status/diff, chokidar watchers →workspace.invalidated, HTTP preview endpoint) + all 16 workspace routes wired intocreateDaemonDispatcher. Works for desktop and web/headless.TreesFileTree(@pierre/trees) with inline rename, drag-and-drop move, create/delete, git-status row signals, search; replaces the deletedWorkspaceFileNoderenderer.@pierre/diffs<File>(read-only) and<File edit>viaEditProvider(inline edit, Save / Cmd-Ctrl+S). Monaco removed.@pierre/diffs<PatchDiff>(splits multi-file + staged/unstaged patches); new top-level Diffs tab..cs/.csproj/.slnand unknown source files render instead of showing as binary.new file modeinjected so they show the added icon).monaco-editor,stream-monaco,@dvaji/vite-plugin-monaco-editor+ Vite workers;WorkspaceCodePane,WorkspaceDiffView,WorkspaceFileNode; CodeMirror mentions in README/landing.Notes / follow-ups
WorkspacePresenteris now dead code for these routes (cleanup pending).Test plan
bun run format,bun run lint(architecture + agent-cleanup + route-catalog guards),bun run typecheck(daemon + desktop + UI),bun run buildworkspacePresentertests (25 pass, incl. 12 new for read/write/create/delete/rename + allow-list/traversal)Summary by CodeRabbit