feat(desktop): add Workbench/Review mode with FileViewer panes#540
feat(desktop): add Workbench/Review mode with FileViewer panes#540andreasasprou wants to merge 14 commits intosuperset-sh:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adds file viewer functionality to the desktop app, introducing a Monaco-based file editor with diff support, a new per-workspace view mode system switching between workbench and review modes, backend file reading with path validation and security checks, and UI components for tab management and view mode toggling. Changes
Sequence DiagramssequenceDiagram
participant User
participant ChangesView
participant FileViewerPane
participant TRPC
participant Store as TabsStore
participant Editor as Monaco Editor
User->>ChangesView: Click file in changes
ChangesView->>Store: addFileViewerPane(workspaceId, filePath, ...)
rect rgb(200, 220, 240)
note over Store: Pane Creation/Reuse Logic
Store->>Store: Check for active tab
alt No active tab
Store->>Store: Create new tab
end
alt Unlocked file-viewer pane exists
Store->>Store: Reuse pane, update fileViewer state
else No reusable pane
Store->>Store: Create new file-viewer pane
Store->>Store: Update tab layout (side-by-side)
end
end
Store->>FileViewerPane: Render with filePath, workspaceId
FileViewerPane->>TRPC: readWorkingFile(worktreePath, filePath)
rect rgb(240, 200, 200)
note over TRPC: Backend Validation & Security
TRPC->>TRPC: validatePathInWorktree (symlink check)
TRPC->>TRPC: Check file size (MAX_FILE_SIZE)
TRPC->>TRPC: Detect binary content
TRPC->>FileViewerPane: Return { ok, content, reason }
end
FileViewerPane->>Editor: Load content
User->>Editor: Edit file
User->>Editor: Cmd/Ctrl+S
Editor->>FileViewerPane: Save content
FileViewerPane->>TRPC: Save mutation
TRPC->>FileViewerPane: Success, invalidate queries
FileViewerPane->>Editor: Reset dirty state
sequenceDiagram
participant User
participant ViewModeToggle
participant Store as ViewModeStore
participant ContentView
participant TabsContent
participant ChangesContent
User->>ViewModeToggle: Click Workbench/Review
ViewModeToggle->>Store: setWorkspaceViewMode(workspaceId, mode)
Store->>Store: Update viewModeByWorkspaceId[workspaceId]
Note over Store: Persist & notify subscribers
ContentView->>Store: getWorkspaceViewMode(workspaceId)
Store->>ContentView: Return "workbench" or "review"
alt mode === "review"
ContentView->>ChangesContent: Render changes/diffs
else mode === "workbench"
ContentView->>TabsContent: Render tabs & file viewer
TabsContent->>TabsContent: Render GroupStrip (tab list)
TabsContent->>TabsContent: Render TabView (pane content)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (1)
111-123: Path traversal vulnerability insaveFile- missing path validation.The
saveFileprocedure joinsworktreePathandfilePathwithout validation. A maliciousfilePathlike../../../etc/cron.d/maliciouscould write files outside the worktree. This is inconsistent withreadWorkingFilewhich properly validates paths.🔎 Proposed fix
saveFile: publicProcedure .input( z.object({ worktreePath: z.string(), filePath: z.string(), content: z.string(), }), ) .mutation(async ({ input }): Promise<{ success: boolean }> => { - const fullPath = join(input.worktreePath, input.filePath); - await writeFile(fullPath, input.content, "utf-8"); - return { success: true }; + // Validate path is within worktree + const validation = await validatePathInWorktree( + input.worktreePath, + input.filePath, + ); + + if (!validation.valid || !validation.resolvedPath) { + throw new Error( + validation.reason === "outside-worktree" + ? "Cannot write to files outside worktree" + : "File path validation failed", + ); + } + + await writeFile(validation.resolvedPath, input.content, "utf-8"); + return { success: true }; }),
🧹 Nitpick comments (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsx (1)
27-53: Consider addingaria-pressedfor accessibility.The toggle buttons function as a segmented control. Adding
aria-pressedwould improve screen reader support by announcing the selected state.🔎 Suggested enhancement
<button type="button" onClick={() => handleModeChange("workbench")} + aria-pressed={currentMode === "workbench"} className={cn( "px-3 py-1 text-sm font-medium rounded-md transition-all", currentMode === "workbench" ? "bg-background text-foreground shadow-sm" : "text-muted-foreground hover:text-foreground", )} > Workbench </button> <button type="button" onClick={() => handleModeChange("review")} + aria-pressed={currentMode === "review"} className={cn(apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
52-61: Close button has a small touch target.The close button with
p-0.5padding results in a small hit area that may be difficult to tap on touch devices. Consider increasing the padding or hit area while keeping the visual size small.🔎 Suggested approach
<button type="button" onClick={(e) => { e.stopPropagation(); onClose(); }} - className="absolute -right-1 -top-1 p-0.5 rounded-full bg-muted opacity-0 group-hover:opacity-100 transition-opacity hover:bg-destructive hover:text-destructive-foreground" + className="absolute -right-1.5 -top-1.5 p-1 rounded-full bg-muted opacity-0 group-hover:opacity-100 transition-opacity hover:bg-destructive hover:text-destructive-foreground" > - <HiMiniXMark className="size-2.5" /> + <HiMiniXMark className="size-2" /> </button>apps/desktop/src/renderer/stores/tabs/utils.ts (1)
117-122: Consider including.mdxfiles for rendered view mode.The logic defaults to "rendered" for
.mdand.markdownfiles. If MDX files are used in the project, they might also benefit from the rendered view.🔎 Suggested enhancement
} else if ( options.filePath.endsWith(".md") || - options.filePath.endsWith(".markdown") + options.filePath.endsWith(".markdown") || + options.filePath.endsWith(".mdx") ) { defaultViewMode = "rendered"; }apps/desktop/src/renderer/stores/tabs/store.ts (1)
389-398: Consider extracting view mode determination logic to reduce duplication.The view mode determination logic (checking for
diffCategoryand markdown extensions) appears to be duplicated here and likely increateFileViewerPane. Consider extracting a shared helper function likedetermineDefaultViewMode(filePath, diffCategory)to ensure consistent behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (2)
262-303: Consider adding dedicated store actions for pane updates.The direct
useTabsStore.getState()/setState()pattern works but bypasses the action-based updates used elsewhere in the store (e.g.,setFocusedPane,removePane). For consistency and testability, consider addingtoggleFileViewerLock(paneId)andsetFileViewerMode(paneId, mode)actions to the store.
31-69: DuplicateddetectLanguagefunction - consider reusing existing utility.This function duplicates logic from
apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.ts(which is exported at line 199). The shared utility has more comprehensive language mappings (e.g.,toml,ruby,php,makefile,dockerfile,csharp). Consider importing and reusing it for consistency and maintainability.-/** Client-side language detection for Monaco editor */ -function detectLanguage(filePath: string): string { - const ext = filePath.split(".").pop()?.toLowerCase() ?? ""; - const languageMap: Record<string, string> = { - ts: "typescript", - // ... truncated - }; - return languageMap[ext] ?? "plaintext"; -} +import { detectLanguage } from "lib/trpc/routers/changes/utils/parse-status";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/index.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/stores/workspace-view-mode.tsapps/desktop/src/shared/tabs-types.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/stores/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsxapps/desktop/src/renderer/stores/workspace-view-mode.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/shared/tabs-types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsxapps/desktop/src/renderer/stores/tabs/store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/renderer/stores/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsxapps/desktop/src/renderer/stores/workspace-view-mode.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/shared/tabs-types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/renderer/stores/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsxapps/desktop/src/renderer/stores/workspace-view-mode.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/shared/tabs-types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define all Electron IPC channel types in
apps/desktop/src/shared/ipc-channels.tsbefore implementing handlers
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsx
apps/**/src/**/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Structure component folders with one component per file using format
ComponentName/ComponentName.tsxwithindex.tsbarrel export
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsx
🧠 Learnings (5)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/renderer/stores/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/**/src/**/**/[A-Z]*.tsx : Structure component folders with one component per file using format `ComponentName/ComponentName.tsx` with `index.ts` barrel export
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/desktop/src/renderer/stores/workspace-view-mode.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx} : Define all Electron IPC channel types in `apps/desktop/src/shared/ipc-channels.ts` before implementing handlers
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/tabs-types.tsapps/desktop/src/renderer/stores/tabs/types.ts
🧬 Code graph analysis (12)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx (1)
apps/desktop/src/renderer/stores/workspace-view-mode.ts (1)
useWorkspaceViewModeStore(28-56)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(12-14)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getTabDisplayName(17-23)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (2)
apps/desktop/src/renderer/stores/workspace-view-mode.ts (1)
useWorkspaceViewModeStore(28-56)apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(65-237)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (2)
apps/desktop/src/shared/tabs-types.ts (2)
Pane(46-59)FileViewerMode(16-16)apps/desktop/src/renderer/contexts/MonacoProvider.tsx (1)
monaco(107-107)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (3)
apps/desktop/src/renderer/stores/workspace-view-mode.ts (1)
useWorkspaceViewModeStore(28-56)apps/desktop/src/shared/changes-types.ts (1)
ChangedFile(22-28)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx (1)
ChangesView(24-367)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (1)
FileViewerPane(88-537)
apps/desktop/src/renderer/stores/tabs/utils.ts (3)
apps/desktop/src/shared/tabs-types.ts (4)
FileViewerMode(16-16)DiffLayout(21-21)Pane(46-59)FileViewerState(26-41)apps/desktop/src/shared/changes-types.ts (1)
ChangeCategory(15-19)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Pane(6-6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx (2)
apps/desktop/src/shared/changes-types.ts (2)
ChangedFile(22-28)ChangeCategory(15-19)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/PortsList.tsx (1)
PortsList(15-121)
apps/desktop/src/shared/tabs-types.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
PaneType(6-6)apps/desktop/src/shared/changes-types.ts (1)
ChangeCategory(15-19)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
GroupStrip(66-151)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx (1)
TabView(27-169)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/changes-types.ts (1)
ChangeCategory(15-19)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/WorkspaceActionBarLeft/WorkspaceActionBarLeft.tsx (1)
WorkspaceActionBarLeft(5-41)apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/ViewModeToggle.tsx (1)
ViewModeToggle(8-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (28)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
32-40: LGTM! Well-structured type definitions.The
AddFileViewerPaneOptionsinterface and theaddFileViewerPanemethod signature are well-defined with appropriate required and optional fields. The integration withChangeCategoryfrom shared types maintains consistency across the codebase.Also applies to: 65-68
apps/desktop/src/shared/tabs-types.ts (1)
6-6: LGTM! Comprehensive and well-documented type definitions.The new file viewer types are thoughtfully designed:
PaneTypeextension follows the existing patternFileViewerModeandDiffLayoutprovide clear, appropriate optionsFileViewerStateincludes all necessary metadata with helpful inline comments- Optional fields are correctly identified for context-dependent scenarios
Also applies to: 11-11, 16-21, 26-41, 58-58
apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/components/ViewModeToggle/index.ts (1)
1-1: LGTM! Follows the established barrel export pattern.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/index.ts (1)
1-1: LGTM! Follows the established barrel export pattern.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
27-34: LGTM! Clean layout integration.The new layout structure properly integrates
GroupStripabove the existingTabViewwith appropriate flex container management. The use offlex-col,overflow-hidden, andmin-h-0ensures proper content behavior and prevents layout issues.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/index.ts (1)
1-1: LGTM! Follows the established barrel export pattern.apps/desktop/src/renderer/stores/index.ts (1)
8-8: LGTM! Consistent with existing store export pattern.The new workspace-view-mode store export follows the established pattern and makes the store properly accessible through the central stores module.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (2)
44-53: LGTM! Proper Zustand store integration.The view mode state is correctly integrated by subscribing to the actual store data (
viewModeByWorkspaceId) to ensure reactivity. The default to "workbench" mode is a sensible fallback.
56-64: LGTM! Intuitive mode-switching behavior.The NEW_TERMINAL hotkey handler correctly implements the UX flow: when in Review mode, it switches to Workbench mode first before creating a terminal. This aligns with the PR's design goal that terminal panes belong to the Workbench experience. The dependency array is properly updated to include the new reactive values.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx (1)
6-29: LGTM!The component correctly subscribes to the
viewModeByWorkspaceIdmap directly rather than using the getter function, ensuring proper Zustand reactivity when the view mode changes. The fallback to "workbench" is consistent with the store's default behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceActionBar/WorkspaceActionBar.tsx (1)
12-23: LGTM!The three-column layout correctly centers the
ViewModeTogglewhile keeping left and right sections at the edges. The flex-1 container provides proper centering behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx (1)
99-114: LGTM!The routing logic correctly identifies file-viewer panes and passes the appropriate props. The simpler prop set compared to
TabPanealigns withFileViewerPane's auto-split-only behavior based on container dimensions.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
66-118: LGTM!The component properly derives workspace-specific tabs, tracks attention state, and handles tab operations. The memoization strategy is appropriate for the data transformations.
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
104-144: LGTM!The
createFileViewerPaneutility correctly constructs file-viewer panes with sensible defaults. The view mode detection logic and state construction are well-structured.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (1)
7-39: LGTM!The sidebar correctly conditionalizes the
onFileOpenhandler based on view mode, ensuring file clicks only openFileViewerPanein Workbench mode. The Zustand subscription pattern is consistent with other components in this PR.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx (3)
16-24: LGTM!The
ChangesViewPropsinterface properly types the optionalonFileOpencallback, and the component correctly uses optional chaining when invoking it.
137-147: LGTM!Both file selection handlers correctly invoke
onFileOpenwith the appropriate arguments, including thecommitHashfor committed files.
363-364: VerifyPortsListplacement is intentional.
PortsListwas imported from../TabsView/PortsListand added to the bottom ofChangesView. This appears to support the unified sidebar mentioned in the PR objectives, making ports visible in both Workbench and Review modes.If this is intentional, the import path suggests
PortsListmight be better co-located withChangesViewor in a shared components directory since it's no longer exclusive toTabsView.apps/desktop/src/renderer/stores/workspace-view-mode.ts (1)
1-56: LGTM! Clean Zustand store implementation.The store follows best practices with proper TypeScript typing, uses
persistfor localStorage persistence, anddevtoolsfor debugging. The default fallback to "workbench" mode is handled correctly. Based on learnings, Zustand is the preferred state management choice.apps/desktop/src/renderer/stores/tabs/store.ts (1)
7-18: LGTM!Import additions are correctly structured for the new file viewer pane functionality.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (4)
138-151: Good practice: Proper query invalidation after save.The save mutation correctly invalidates
readWorkingFile,getFileContents, andgetStatusqueries to ensure UI consistency after file modifications.
176-194: LGTM! Proper keybinding setup with cross-platform support.The
monaco.KeyMod.CtrlCmdcorrectly handles Cmd+S on macOS and Ctrl+S on Windows/Linux. The dependency array correctly includeshandleSaveRaw.
210-231: LGTM! Conditional query fetching is well-structured.The
enabledconditions properly gate data fetching based on view mode and required parameters, avoiding unnecessary network requests.
381-410: LGTM! Well-configured Monaco Editor with sensible defaults.The editor options provide a good user experience with disabled minimap, word wrap enabled, appropriate font settings, and reasonable padding/scrollbar sizes.
apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (4)
9-23: LGTM! Well-defined constants and result type.The 2 MiB file size limit and 8KB binary check window are reasonable defaults. The discriminated union type provides clear error categorization.
28-61: Robust path validation with symlink resolution - good security practice.The validation correctly handles: absolute paths, path traversal attempts, and symlink escapes. The use of
realpath()to resolve symlinks before checking containment is the right approach.
63-74: LGTM! Standard binary detection heuristic.NUL byte scanning is a widely-used approach for binary detection. The 8KB window balances accuracy with performance.
130-185: LGTM! Well-implemented file read with proper validation chain.The procedure correctly validates paths, checks file size before reading, and detects binary content. The resolved path from
validatePathInWorktreeis consistently used throughout, avoiding TOCTOU issues.
Introduces a workspace-level view mode toggle allowing users to switch between: - **Workbench mode**: Mosaic panes layout with terminals + file viewers for in-flow work - **Review mode**: Dedicated Changes page for focused code review Key changes: - Add ViewModeToggle component in workspace header (prominent segmented control) - Add FileViewerPane with Raw/Rendered/Diff modes, lock/unlock, and split support - Add GroupStrip for group switching above Mosaic content - Unify sidebar to use full ChangesView in both modes (with onFileOpen callback) - Add workspace-view-mode store with per-workspace persistence - Add readWorkingFile tRPC procedure for safe file reads (size/binary checks) - Wire file clicks to open/reuse FileViewer panes (MRU unlocked policy) - Cmd+T in Review mode switches to Workbench first, then creates terminal
- Add !!worktreePath checks to FileViewerPane query enabled conditions - Add !!worktreePath checks to save handler guards (handleSaveRaw, handleSaveDiff) - Fix stale state reference after addTab() in addFileViewerPane action Addresses CodeRabbit review feedback.
… and UX improvements - Add validatePathForWrite() to prevent path traversal attacks in saveFile - Add aria-pressed attribute to ViewModeToggle buttons for accessibility - Increase close button touch target in GroupStrip for better UX - Add .mdx file support for rendered view mode
P0 Security: - Add path validation to getUnstagedVersions before readFile (prevents traversal) - Key Editor/DiffViewer by filePath to force remount (fixes stale Cmd+S closure) P1 Fixes: - Update originalContentRef when raw content loads (dirty tracking fix) - Add .mdx to isMarkdown check for toolbar consistency - Gate diff editable to staged/unstaged only (against-main/committed now read-only) P2 Performance: - Add safeGitShow helper with 2MB size limit on all git show calls - Add size check before reading working tree files in getUnstagedVersions
- P0-1: Add file-viewer type and fileViewer object to paneSchema for tab persistence - P0-2: Fix symlink escape vulnerability in validatePathForWrite by checking if target is symlink and resolving parent directory paths - P1-1: Pass defaultBranch to FileViewerPane for against-main diffs - P1-2: Switch staged diff to unstaged after save (matches Review mode behavior) - P2: Use Buffer.byteLength instead of string.length in safeGitShow for accurate UTF-8 byte counting
P0 (critical):
- Remove duplicate saveFile from git-operations.ts that was overwriting
the hardened version in file-contents.ts (security vulnerability)
P1 (must fix):
- Use basename()/dirname() from node:path instead of split('/') for
cross-platform Windows compatibility in validatePathForWrite
- Add preflight size check with git cat-file -s in safeGitShow to
prevent memory spikes from large blobs before materializing content
P2 (UX):
- Fix split pane to clone file-viewer state instead of creating terminal
when splitting a file-viewer pane (locked by default)
Question fix:
- Show GroupStrip with add button even when tabs.length === 0 so users
have visible UI to create new terminal (not just hotkey)
…ocalDb
P0 (CRITICAL SECURITY):
- Add validateWorktreePathInDb() that verifies worktreePath exists in
localDb.worktrees before any filesystem operations
- Without this, a compromised renderer could read/write arbitrary files
by passing worktreePath='/' and filePath='.ssh/id_rsa'
- Applied to getFileContents, saveFile, and readWorkingFile procedures
P1 (correctness):
- Replace startsWith('..') checks with segment-aware containsPathTraversal()
and isPathOutsideBase() helpers that use path.sep
- Fixes false positives on valid paths like '..foo/bar' (directories
starting with '..')
- Cross-platform compatible (handles both / and \ separators)
P2 (performance):
- Guard killTerminalForPane() calls on pane.type === 'terminal'
- Prevents unnecessary IPC and warning logs when closing file-viewer panes
…ve editor drafts P0 (security): - Extract assertWorktreePathInDb to shared security.ts - Returns worktree record to avoid duplicate queries - Apply validation to ALL routes: git-operations, status, staging, branches P1 (data loss): - Preserve unsaved editor content across view mode switches - Store draft in ref before switching away from raw mode - Restore draft when returning to raw mode - Clear draft on save and file change - Update dirty state on editor mount with draft content
…otection P0 Security (BLOCK): - Add validatePathInWorktree() check before rm() in deleteUntracked - Prevents path traversal (../) and symlink escape attacks - Consolidated path validation utilities in security.ts P1 Data Loss: - Track save source (Raw vs Diff) with savingFromRawRef - Only clear draft when saving from Raw mode - Disable Diff editing when Raw draft exists (forces user to save/discard) P2: - Use ['--', filePath] in git.add() to handle paths starting with -
…th validation - Reduce security.ts from 213 to 65 lines - Remove async symlink/realpath checking (users own their repos) - Remove segment-aware path traversal (..foo edge case not worth complexity) - Keep worktreePath DB validation (the real security boundary) - Keep simple .. and absolute path checks (sufficient for 99.99% of cases)
P0 (blocking): - Add path validation (rejects .. and absolute) to applyUntrackedLineCount - Add 1MB size cap to prevent OOM on large untracked files during polling P2: - Add type guard in updateTabLayout - only call killTerminalForPane for terminal panes - Use ['--', branch] in switchBranch to ensure branch treated as refname
…lidation - Add path-validation.ts with industry-standard containment check (path.relative) - Add secure-fs.ts with self-validating FS wrappers (symlink escape protection) - Add git-commands.ts with semantic helpers (gitSwitchBranch vs gitCheckoutFile) - Fix switchBranch: use 'git switch' instead of 'git checkout --' (was file checkout) - Fix path traversal check: segment-aware (allows ..foo, rejects ..) - Fix symlink bypass: check parent dirs for new files, use stat not lstat - Fix worktree root deletion: explicit allowRoot check - All FS operations now go through secureFs with validation built-in
Add a setting to control whether Cmd+clicking file paths in the terminal opens them in an external editor (default) or in the in-app FileViewerPane. - Add terminalLinkBehavior setting to local-db schema with migration - Add getTerminalLinkBehavior/setTerminalLinkBehavior tRPC procedures - Refactor createTerminalInstance to accept onFileLinkClick callback - Wire up setting in Terminal.tsx with ref pattern (avoids terminal recreation) - Add Select UI in BehaviorSettings for choosing link behavior
Remove async symlink escape detection from path validation since the threat model doesn't justify it: a compromised renderer already has terminal access for arbitrary command execution. Changes: - Replace resolveSecurePath (async) with resolvePathInWorktree (sync) - Remove assertNoSymlinkEscape and checkSymlinks option - Add validateRelativePath for simple path safety checks - Update secure-fs.ts to use new sync functions - Update threat model documentation in path-validation.ts
andreasasprou
force-pushed
the
feat/desktop-workbench-review-mode
branch
from
f2f4849 to
0bfdbe6
Compare
1 participant
Why
Users need to view code, diffs, and docs while keeping terminals visible in the same window. The existing Changes page is great for focused review, but doesn't support in-flow work where you want terminals and file viewers side-by-side.
What / How
Introduces a workspace-level Workbench | Review view mode toggle:
Key implementation details:
onFileOpencallback to open FileViewer panesInline Editing Support
FileViewerPane now supports full editing in both Raw and Diff modes:
Configurable Terminal File Link Behavior
Added a global setting to control how Cmd+clicking file paths in the terminal behaves:
Future Work
⌘+\or similar to toggle between Workbench/Review (currently mouse-only)Out of Scope
Files
New
GroupStrip/GroupStrip.tsx- Group switching strip componentFileViewerPane/FileViewerPane.tsx- File viewer with Raw/Rendered/Diff modes + inline editingViewModeToggle/ViewModeToggle.tsx- Segmented control for mode toggleworkspace-view-mode.ts- Per-workspace view mode store0004_add_terminal_link_behavior_setting.sql- Migration for terminal link behavior settingModified
file-contents.ts- AddedreadWorkingFileprocedureContentView/index.tsx- Routes to TabsContent or ChangesContent based on modeChangesView.tsx- AddedonFileOpencallback propSidebar/index.tsx- PassesonFileOpenin Workbench modeWorkspaceActionBar.tsx- Added ViewModeToggle to headerWorkspaceView/index.tsx- View mode reactivity and ⌘+T behaviortabs/store.ts- AddedaddFileViewerPaneactiontabs/types.ts- Added FileViewerPane typestabs/utils.ts- Added pane utility functionsshared/tabs-types.ts- Addedfile-viewerpane typeTerminal/helpers.ts- Refactored to acceptonFileLinkClickcallbackTerminal/Terminal.tsx- Wired up terminal link behavior settingBehaviorSettings.tsx- Added terminal link behavior dropdownsettings/index.ts(tRPC) - Added getter/setter for terminal link behaviorschema.ts/zod.ts(local-db) - AddedterminalLinkBehaviorcolumnTesting
Core Features
bun run typecheck)Terminal Link Behavior