feat(desktop): add Workbench/Review mode + group tabs + workspace navigation sidebar #586
Conversation
📝 WalkthroughWalkthroughAdds a security layer for worktree/path validation and secure FS/Git helpers; refactors changes TRPC routers to use them; introduces a file‑viewer pane with read/save flows, a resizable workspace sidebar and per‑workspace view modes/unread state; updates DB schema and many renderer components and stores. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Renderer (FileViewerPane)
participant Store as TabsStore
participant TRPC as TRPC server
participant Security as Security module
participant FS as Filesystem
User->>Renderer: select file (open)
Renderer->>Store: addFileViewerPane(workspaceId, options)
Store-->>Renderer: paneId / focused
Renderer->>TRPC: readWorkingFile(worktreePath, filePath)
TRPC->>Security: assertRegisteredWorktree(worktreePath)
Security-->>TRPC: OK or throw
TRPC->>Security: secureFs.readFileBuffer(worktreePath, filePath)
Security->>FS: resolve & realpath checks
FS-->>Security: Buffer or ENOENT / symlink
alt Buffer received
TRPC->>TRPC: isBinaryContent check
alt text
TRPC-->>Renderer: ReadWorkingFileResult (content)
Renderer->>User: render content/editor
else binary
TRPC-->>Renderer: ReadWorkingFileResult (binary)
Renderer->>User: show binary placeholder
end
else Path validation / symlink error
TRPC-->>Renderer: PathValidationError -> surface error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceControls/OpenInMenuButton.tsx (1)
166-169: Hardcoded shortcut inconsistent with dynamic shortcut display.Line 167 uses hardcoded
⌘Owhile the top-level APP_OPTIONS on line 141 correctly usesopenInShortcut. This inconsistency could confuse users if they've customized hotkeys.Proposed fix
{VSCODE_OPTIONS.map((app) => ( <DropdownMenuItem key={app.id} onClick={() => handleOpenInOtherApp(app.id)} > <img src={app.icon} alt={app.label} className="size-4 object-contain mr-2" /> {app.label} - {app.id === lastUsedApp && ( - <DropdownMenuShortcut>⌘O</DropdownMenuShortcut> + {app.id === lastUsedApp && showOpenInShortcut && ( + <DropdownMenuShortcut>{openInShortcut}</DropdownMenuShortcut> )} </DropdownMenuItem> ))}
🧹 Nitpick comments (36)
apps/desktop/src/lib/trpc/routers/ui-state/index.ts (1)
43-45: Consider simplifying thecwdfield's type unless three states are needed.The pattern
z.string().nullable().optional()allows three distinct states:undefined,null, andstring. This tri-state pattern adds complexity and is unusual in schema design.Question: Is there a specific requirement to distinguish between "field not set" (undefined), "explicitly no cwd" (null), and "has cwd" (string)?
If not, consider simplifying to either:
z.string().optional()if absence means "no cwd"z.string().nullable()if null means "no cwd" (field always present)If the tri-state distinction is intentional (e.g., for tracking state transitions in terminal panes), please document the semantics in a comment for clarity.
💡 Potential simplification if tri-state is not required
- cwd: z.string().nullable().optional(), + cwd: z.string().optional(),apps/desktop/src/renderer/components/MarkdownRenderer/components/SafeImage/SafeImage.tsx (2)
3-31: Strong allowlist-based security approach.The strict data:-only policy effectively prevents all external resource access. The comprehensive documentation clearly enumerates the threat model and blocked types. The implementation correctly handles edge cases (empty, undefined, whitespace).
Optional: Export for testability
Consider exporting
isSafeImageSrcto enable independent unit testing of the validation logic:-function isSafeImageSrc(src: string | undefined): boolean { +export function isSafeImageSrc(src: string | undefined): boolean {This would allow testing edge cases (empty strings, various protocols, case sensitivity) without rendering the full component.
50-71: Component implementation is solid.The two render paths (blocked vs. allowed) are clear and well-implemented. The blocked state provides good UX with an icon and message.
Minor: title attribute discloses blocked URL
Line 55's title attribute exposes the blocked src:
title={`Image blocked: ${src ?? "(empty)"}`}While this is a minor information disclosure, in a local Electron app context where users have access to the markdown source file anyway, it's likely acceptable and may even aid debugging. If stricter information hiding is desired, consider:
- title={`Image blocked: ${src ?? "(empty)"}`} + title="Image blocked for security"apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx (1)
23-29: Consider clarifying the git initialization error message.The error message directs users to "use 'Open project' from the start view" when they're already attempting to open a project via this footer button. This could confuse users about why this button exists if it can't handle git initialization.
Consider either:
- Updating the message to explain the limitation more clearly (e.g., "This folder needs to be initialized as a git repository first. Use the main 'Open project' flow to initialize git.")
- Handling git initialization directly in this flow if feasible
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader.tsx (1)
6-6: Minor: Consider usingsize-4for consistency with other components.Other components in this PR (e.g.,
WorkspaceSidebarControl.tsxlines 26-28) usesize-4for icon sizing. Usingw-4 h-4here works identically but is inconsistent.🔎 Suggested fix
- <LuLayers className="w-4 h-4 text-muted-foreground" /> + <LuLayers className="size-4 text-muted-foreground" />apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceSidebarControl.tsx (1)
36-41: Consider using index for the key to avoid potential duplicates.If
formatHotkeyDisplayreturns duplicate display strings (unlikely but possible), usingkey={key}would cause React key collisions. Using the array index is safer here since the order is stable.🔎 Suggested fix
{formatHotkeyDisplay( getHotkey("TOGGLE_WORKSPACE_SIDEBAR"), getCurrentPlatform(), - ).map((key) => ( - <Kbd key={key}>{key}</Kbd> + ).map((key, index) => ( + <Kbd key={index}>{key}</Kbd> ))}apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsx (1)
1-4: Consider exporting the interface for type reusability.The
WorkspaceDiffStatsPropsinterface could be useful for consumers who need to type-check props before passing them. Consider adding theexportkeyword.Proposed change
-interface WorkspaceDiffStatsProps { +export interface WorkspaceDiffStatsProps { additions: number; deletions: number; }apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (1)
22-33: Consider extracting the handler to improve readability.The inline ternary creating a function or
undefinedworks but is dense. A named function with early return could be clearer.Alternative approach
- // In Workbench mode, open files in FileViewerPane - const handleFileOpen = - viewMode === "workbench" && workspaceId - ? (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { - addFileViewerPane(workspaceId, { - filePath: file.path, - diffCategory: category, - commitHash, - oldPath: file.oldPath, - }); - } - : undefined; + // In Workbench mode, open files in FileViewerPane + const handleFileOpen = useCallback( + (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { + if (viewMode !== "workbench" || !workspaceId) return; + addFileViewerPane(workspaceId, { + filePath: file.path, + diffCategory: category, + commitHash, + oldPath: file.oldPath, + }); + }, + [viewMode, workspaceId, addFileViewerPane], + );This requires adjusting how
ChangesViewhandles a no-op callback vsundefined, but improves readability and memoization.apps/desktop/src/renderer/hooks/useWorkspaceShortcuts.ts (1)
111-117: Consider returning onlysetActiveWorkspace.mutateinstead of the full mutation object.Returning the entire mutation object exposes internal state (isPending, isError, etc.) that consumers may not need. If consumers only need to trigger the mutation, returning just the
mutatefunction provides a cleaner API.Alternative return
return { groups, allWorkspaces, activeWorkspaceId, - setActiveWorkspace, + setActiveWorkspace: setActiveWorkspace.mutate, };Note: This would require updating any consumers that use
setActiveWorkspace.mutate(...)to justsetActiveWorkspace(...)..agents/commands/create-plan-file.md (1)
1-235: Comprehensive ExecPlan template with clear guidance.This documentation provides thorough guidance for creating execution plans. The template emphasizes self-containment, novice-readiness, and living-document practices, which align well with the stated goals of enabling any contributor to execute a plan end-to-end.
The lifecycle management section (done/abandoned folders) provides clear organizational structure for plan tracking.
Optional: Address markdown linting issues if desired.
Static analysis flagged some stylistic markdown issues (fenced code blocks without language specifiers, emphasis style preferences, bare URLs). These are entirely optional fixes and don't impact the functionality of the documentation.
apps/desktop/.agents/plans/20251231-1200-workspace-sidebar-navigation.md (1)
1-691: Thorough execution plan with clear milestones and design decisions.This plan demonstrates careful consideration of the implementation approach:
- Clear user-facing outcomes: The Purpose section describes exactly what users will be able to do after the change.
- Well-structured milestones: The 5 milestones progressively implement the feature with validation steps at each stage.
- Good decision documentation: The Decision Log captures important choices with rationale (e.g., using
string[]instead ofSet<string>for Zustand persist due to JSON serialization).- Reuse of existing patterns: The plan appropriately references existing components (WorkspaceHoverCard, tRPC queries) and patterns.
- Shared behavior extraction: The decision to extract ⌘1-9 shortcuts into a shared hook demonstrates good architectural thinking to avoid duplication across navigation modes.
Optional: Use markdown heading levels for subsections.
Static analysis flags many instances where bold text is used for subsection headings (e.g.,
**1.1 Add setting to database schema**). Consider using heading levels (### 1.1 Add setting to database schema) for better document structure and markdown compliance. This is purely stylistic and doesn't affect the plan's quality.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/index.ts (1)
1-2: Consider direct imports to avoid potential circular dependencies.This barrel file re-exports two components, which is a common pattern but can lead to circular dependency issues. Per coding guidelines: "Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies."
Since this is a simple two-export barrel with no intermediate logic, the risk is low, but consider whether consumers could import directly from
./ResizableWorkspaceSidebarand./WorkspaceSidebarinstead.apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (1)
30-41: Consider error handling for the undo action.The
setUnread.mutatecall in the undo action is fire-and-forget. If it fails, the user won't know the undo didn't work.🔎 Proposed enhancement with error feedback
action: { label: "Undo", onClick: () => { - setUnread.mutate({ id: variables.id, isUnread: true }); + setUnread.mutate( + { id: variables.id, isUnread: true }, + { + onError: () => { + toast.error("Failed to undo"); + }, + }, + ); }, },apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceStatusBadge.tsx (1)
17-36: Consider hoisting config outside the component.The
configobject is recreated on every render. Since it's static, hoisting it to module scope would avoid unnecessary object allocations.🔎 Proposed refactor
+const ICON_CLASS = "w-3 h-3"; + +const STATE_CONFIG = { + open: { + icon: <LuGitPullRequest className={cn(ICON_CLASS, "text-emerald-500")} />, + bgColor: "bg-emerald-500/10", + }, + merged: { + icon: <LuGitMerge className={cn(ICON_CLASS, "text-purple-500")} />, + bgColor: "bg-purple-500/10", + }, + closed: { + icon: <LuCircleDot className={cn(ICON_CLASS, "text-destructive")} />, + bgColor: "bg-destructive/10", + }, + draft: { + icon: <LuGitPullRequest className={cn(ICON_CLASS, "text-muted-foreground")} />, + bgColor: "bg-muted", + }, +} as const; + export function WorkspaceStatusBadge({ state, prNumber, }: WorkspaceStatusBadgeProps) { - const iconClass = "w-3 h-3"; - - const config = { - open: { - icon: <LuGitPullRequest className={cn(iconClass, "text-emerald-500")} />, - bgColor: "bg-emerald-500/10", - }, - // ... etc - }; - - const { icon, bgColor } = config[state]; + const { icon, bgColor } = STATE_CONFIG[state];apps/desktop/src/lib/trpc/routers/changes/branches.ts (2)
163-165: Empty catch block silently swallows errors.Per coding guidelines, errors should not be silently swallowed. Consider logging with context.
🔎 Proposed fix
- } catch {} + } catch (error) { + console.warn("[branches/getCheckedOutBranches] Failed to list worktrees:", error); + }
119-122: Consider logging the fallback case.The catch block returns fallback data, which is reasonable, but logging would help with debugging.
🔎 Proposed enhancement
} catch { + console.warn("[branches/getLocalBranchesWithDates] Failed to get branch dates, using fallback"); return localBranches.map((branch) => ({ branch, lastCommitDate: 0 })); }apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
139-152: Extract magic number to a named constant.The inline
width: "120px"should be a named constant at module top per coding guidelines.🔎 Proposed fix
+const GROUP_ITEM_WIDTH = 120; + export function GroupStrip() { // ... return ( // ... <div key={tab.id} className="h-full shrink-0" - style={{ width: "120px" }} + style={{ width: `${GROUP_ITEM_WIDTH}px` }} >apps/desktop/src/lib/trpc/routers/changes/status.ts (1)
147-176: Secure file-reading implementation looks good, consider adding debug logging for skipped files.The
secureFsintegration properly validates paths and handles symlink escapes. The size limit and error handling provide graceful degradation. However, the silent catch makes debugging difficult when files are unexpectedly skipped.🔎 Optional: Add debug-level logging for skipped files
} catch { - // Skip files that fail validation or reading + // Skip files that fail validation or reading (e.g., symlink escapes, permission errors) + // Intentionally silent - line counts are non-critical metadata }If you have a debug logger available, you could log skipped files at trace/debug level to aid troubleshooting without impacting normal operation.
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
117-127: Consider case-insensitive extension matching for markdown files.The extension check is case-sensitive, so files like
README.MDorCHANGELOG.Markdownwon't get therendereddefault view mode.🔎 Proposed fix for case-insensitive matching
+ const lowerPath = options.filePath.toLowerCase(); if (options.diffCategory) { defaultViewMode = "diff"; } else if ( - options.filePath.endsWith(".md") || - options.filePath.endsWith(".markdown") || - options.filePath.endsWith(".mdx") + lowerPath.endsWith(".md") || + lowerPath.endsWith(".markdown") || + lowerPath.endsWith(".mdx") ) { defaultViewMode = "rendered"; }
141-142: Path separator assumption may fail on Windows.Using
"/"as the path separator works for POSIX paths but Windows paths use"\\". Since this is renderer code (which can't import Node'spathmodule per coding guidelines), consider handling both separators.🔎 Proposed fix for cross-platform path handling
// Use filename for display name - const fileName = options.filePath.split("/").pop() || options.filePath; + const fileName = + options.filePath.split(/[/\\]/).pop() || options.filePath;packages/local-db/src/schema/schema.ts (1)
122-125: Consider movingNavigationStyletozod.tsfor consistency.
TerminalLinkBehavioris defined inzod.tsand imported here, butNavigationStyleis defined locally. For consistency and to enable Zod validation if needed later, consider definingNavigationStyleinzod.tsalongside other settings types.Based on the pattern in
packages/local-db/src/schema/zod.ts(line 107), other settings types likeTerminalLinkBehaviorare defined there.apps/desktop/src/renderer/stores/workspace-view-mode.ts (1)
28-56: Clean per-workspace view mode store implementation.The store follows Zustand best practices with proper typing and persistence. Consider adding a
versionto the persist config for future migration support, similar to the sidebar store.Optional: Add version for future migration support
{ name: "workspace-view-mode-store", + version: 1, },apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceControls/ViewModeToggleCompact.tsx (1)
26-26: Move constant to module level.Per coding guidelines, extract magic numbers to named constants at module top.
Move BUTTON_HEIGHT to module level
+const BUTTON_HEIGHT = 24; + export function ViewModeToggleCompact({ workspaceId, }: ViewModeToggleCompactProps) { // Select only this workspace's mode to minimize rerenders const currentMode = useWorkspaceViewModeStore( (s) => s.viewModeByWorkspaceId[workspaceId] ?? "workbench", ); const setWorkspaceViewMode = useWorkspaceViewModeStore( (s) => s.setWorkspaceViewMode, ); const handleModeChange = (mode: WorkspaceViewMode) => { setWorkspaceViewMode(workspaceId, mode); }; - const BUTTON_HEIGHT = 24; - return (apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ResizableWorkspaceSidebar.tsx (1)
77-91: Add keyboard resize handlers for accessibility.The resize separator has
tabIndex={0}and proper ARIA attributes, but it's missing keyboard event handlers. Users should be able to resize using arrow keys when the separator is focused.🔎 Suggested keyboard handler implementation
Add this handler after
handleMouseDown:+ const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + if (e.key === "ArrowLeft") { + e.preventDefault(); + const newWidth = Math.max(MIN_WORKSPACE_SIDEBAR_WIDTH, width - 10); + setWidth(newWidth); + } else if (e.key === "ArrowRight") { + e.preventDefault(); + const newWidth = Math.min(MAX_WORKSPACE_SIDEBAR_WIDTH, width + 10); + setWidth(newWidth); + } + }, + [width, setWidth], + );Then add it to the separator element:
<div role="separator" aria-orientation="vertical" aria-valuenow={width} aria-valuemin={MIN_WORKSPACE_SIDEBAR_WIDTH} aria-valuemax={MAX_WORKSPACE_SIDEBAR_WIDTH} tabIndex={0} onMouseDown={handleMouseDown} + onKeyDown={handleKeyDown} className={cn(apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
212-230: Extract navigation styles constant for consistency.The
navigationStylesetter uses an inline enumz.enum(["top-bar", "sidebar"]), whileterminalLinkBehaviorreferences the importedTERMINAL_LINK_BEHAVIORSconstant. For consistency and maintainability, consider extracting aNAVIGATION_STYLESconstant.🔎 Suggested refactor
In
packages/local-db/src/schema/zod.ts(or the appropriate constants file):export const NAVIGATION_STYLES = ["top-bar", "sidebar"] as const;Then update the import and validation:
import { settings, TERMINAL_LINK_BEHAVIORS, + NAVIGATION_STYLES, type TerminalPreset, } from "@superset/local-db"; // ... setNavigationStyle: publicProcedure - .input(z.object({ style: z.enum(["top-bar", "sidebar"]) })) + .input(z.object({ style: z.enum(NAVIGATION_STYLES) })) .mutation(({ input }) => {apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (1)
137-147: Consider logging unexpected errors before mapping to "not-found".The catch-all at line 145-146 maps any non-
PathValidationErrorto"not-found", which could mask unexpected failures (e.g., permission errors, disk failures). Per coding guidelines, errors should not be swallowed silently.Proposed fix
} catch (error) { if (error instanceof PathValidationError) { // Map specific error codes to distinct reasons if (error.code === "SYMLINK_ESCAPE") { return { ok: false, reason: "symlink-escape" }; } return { ok: false, reason: "outside-worktree" }; } - // File not found or other read error + // Check for ENOENT specifically + if ( + error instanceof Error && + "code" in error && + error.code === "ENOENT" + ) { + return { ok: false, reason: "not-found" }; + } + // Log unexpected errors before treating as not-found + console.error("[file-contents/readWorkingFile] Unexpected error:", error); return { ok: false, reason: "not-found" }; }apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (2)
87-91: Type assertion bypasses type safety.Casting
value as TerminalLinkBehavioris unsafe if the Select options don't match the type's allowed values. Consider validating the value or using a type-safe approach.Proposed fix using a lookup
+const TERMINAL_LINK_BEHAVIORS: TerminalLinkBehavior[] = ["external-editor", "file-viewer"]; + const handleLinkBehaviorChange = (value: string) => { + if (!TERMINAL_LINK_BEHAVIORS.includes(value as TerminalLinkBehavior)) { + return; + } setTerminalLinkBehavior.mutate({ behavior: value as TerminalLinkBehavior, }); };
13-13: Import NavigationStyle from @superset/local-db instead of defining it locally.The type is already defined and exported in
packages/local-db/src/schema/schema.tsand used elsewhere (e.g., TopBar imports it). Defining it locally in BehaviorSettings.tsx creates unnecessary duplication and risks type drift. Import it like other components do:import type { NavigationStyle } from "@superset/local-db";apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (2)
147-159: Drag item index mutation is intentional but worth a comment.The
item.index = indexmutation on line 156 is a standard react-dnd pattern to prevent repeated hover events, but it can be surprising to readers. Consider adding a brief comment.Suggested comment
const [, drop] = useDrop({ accept: WORKSPACE_TYPE, hover: (item: { id: string; projectId: string; index: number }) => { if (item.projectId === projectId && item.index !== index) { reorderWorkspaces.mutate({ projectId, fromIndex: item.index, toIndex: index, }); + // Update item.index to prevent redundant reorder calls during the same drag item.index = index; } }, });
294-316: DeleteWorkspaceDialog rendered in both branches.The
DeleteWorkspaceDialogis rendered identically in both the branch workspace (lines 307-314) and worktree workspace (lines 344-350) branches. Consider extracting it outside the conditional to reduce duplication.Proposed refactor
if (isBranchWorkspace) { return ( - <> - <ContextMenu> + <ContextMenu> {/* ... context menu content ... */} - </ContextMenu> - <DeleteWorkspaceDialog - workspaceId={id} - workspaceName={name} - workspaceType={type} - open={showDeleteDialog} - onOpenChange={setShowDeleteDialog} - /> - </> + </ContextMenu> ); } - return ( - <> - <HoverCard> + const mainContent = ( + <HoverCard> {/* ... hover card content ... */} - </HoverCard> - <DeleteWorkspaceDialog - workspaceId={id} - workspaceName={name} - workspaceType={type} - open={showDeleteDialog} - onOpenChange={setShowDeleteDialog} - /> - </> + </HoverCard> ); + + return ( + <> + {isBranchWorkspace ? branchContent : mainContent} + <DeleteWorkspaceDialog + workspaceId={id} + workspaceName={name} + workspaceType={type} + open={showDeleteDialog} + onOpenChange={setShowDeleteDialog} + /> + </> + );Also applies to: 318-352
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
159-177: Consider adding user feedback for external editor failures.The URL link provider (lines 144-152) shows a toast on error, but the file link fallback only logs to console. Users won't know why clicking a file link didn't work.
Proposed fix
} else { // Fallback to default behavior (external editor) trpcClient.external.openFileInEditor .mutate({ path, line, column, cwd, }) .catch((error) => { console.error( "[Terminal] Failed to open file in editor:", path, error, ); + toast.error("Failed to open file in editor", { + description: + error instanceof Error + ? error.message + : "Could not open file", + }); }); }apps/desktop/src/renderer/stores/tabs/store.ts (1)
359-374: Inefficient: creates terminal pane then immediately replaces it.When no active tab exists,
addTabcreates a terminal pane, then the code immediately overwrites it with a file-viewer pane. This creates an orphaned terminal session briefly.Proposed refactor
Consider adding an option to
addTabto specify pane type, or create a dedicatedaddTabWithFileVieweraction:// If no active tab, create a new one (this shouldn't normally happen) if (!activeTab) { - const { tabId, paneId } = get().addTab(workspaceId); - // Update the pane to be a file-viewer (must use set() to get fresh state after addTab) - const fileViewerPane = createFileViewerPane(tabId, options); - set((s) => ({ - panes: { - ...s.panes, - [paneId]: { - ...fileViewerPane, - id: paneId, // Keep the original ID - }, - }, - })); - return paneId; + // Create tab with file-viewer pane directly + const newPane = createFileViewerPane("temp", options); + const { tab, pane: _ } = createTabWithPane(workspaceId, state.tabs); + // Replace the auto-created terminal pane with file-viewer + const fileViewerPane = { ...newPane, id: tab.id + "-pane", tabId: tab.id }; + + set({ + tabs: [...state.tabs, { ...tab, layout: fileViewerPane.id }], + panes: { ...state.panes, [fileViewerPane.id]: fileViewerPane }, + activeTabIds: { ...state.activeTabIds, [workspaceId]: tab.id }, + focusedPaneIds: { ...state.focusedPaneIds, [tab.id]: fileViewerPane.id }, + }); + return fileViewerPane.id; }This avoids creating a terminal pane that's immediately discarded.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (1)
110-117: Inconsistent hotkey registration pattern.Lines 55, 65, 73, 81, 90, 98 use
getHotkey("...")for configurable shortcuts, but lines 110 and 121 hardcode"meta+o"and"meta+shift+c". These correspond to defined hotkey IDsOPEN_IN_APPandCOPY_PATHin the hotkeys configuration and should usegetHotkey()for consistency and to allow user customization like other shortcuts.Proposed fix
- useHotkeys("meta+o", () => { + useHotkeys(getHotkey("OPEN_IN_APP"), () => { if (activeWorkspace?.worktreePath) { openInApp.mutate({ path: activeWorkspace.worktreePath, app: lastUsedApp, }); } }, [activeWorkspace?.worktreePath, lastUsedApp]); // Copy path shortcut const copyPath = trpc.external.copyPath.useMutation(); - useHotkeys("meta+shift+c", () => { + useHotkeys(getHotkey("COPY_PATH"), () => { if (activeWorkspace?.worktreePath) { copyPath.mutate(activeWorkspace.worktreePath); } }, [activeWorkspace?.worktreePath]);apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (3)
32-69: DuplicatedetectLanguagefunction.This function duplicates the implementation in
packages/.../parse-status.ts(shown in relevant code snippets). Consider importing from the shared location to maintain a single source of truth.🔎 Suggested approach
If the shared
detectLanguageis accessible from the renderer process, import it:+import { detectLanguage } from "shared/parse-status"; // or appropriate path -/** Client-side language detection for Monaco editor */ -function detectLanguage(filePath: string): string { - const ext = filePath.split(".").pop()?.toLowerCase() ?? ""; - // ... 35 lines of duplicated code -}If there are module boundary concerns (renderer vs main process), consider moving the utility to a shared package that both can import.
374-420: Direct store mutation pattern used for state updates.The handlers like
handleToggleLockandhandleViewModeChangedirectly calluseTabsStore.setState()instead of using actions. While this works, consider if these should be extracted to store actions for consistency with the zustand patterns mentioned in coding guidelines.
662-669: Accessibility concern with click-only focus handler.The
biome-ignoresuppresses keyboard accessibility warnings. Consider ifonFocuswould be more appropriate here since the container could receive focus via tab navigation.
...sktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsx
Outdated
Show resolved
Hide resolved
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx
Outdated
Show resolved
Hide resolved
| function GroupItem({ | ||
| tab, | ||
| isActive, | ||
| needsAttention, | ||
| onSelect, | ||
| onClose, | ||
| }: GroupItemProps) { | ||
| const displayName = getTabDisplayName(tab); | ||
|
|
||
| return ( | ||
| <div className="group relative flex items-end shrink-0 h-full"> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={onSelect} | ||
| className={cn( | ||
| "flex items-center gap-1.5 rounded-t-md transition-all w-full shrink-0 pl-3 pr-6 h-[80%]", | ||
| isActive | ||
| ? "text-foreground border-t border-l border-r border-border" | ||
| : "text-muted-foreground hover:text-foreground hover:bg-tertiary/30", | ||
| )} | ||
| > | ||
| <span className="text-sm whitespace-nowrap overflow-hidden flex-1 text-left"> | ||
| {displayName} | ||
| </span> | ||
| {needsAttention && ( | ||
| <span className="relative flex size-2 shrink-0"> | ||
| <span className="absolute inline-flex h-full w-full animate-ping rounded-full bg-red-400 opacity-75" /> | ||
| <span className="relative inline-flex size-2 rounded-full bg-red-500" /> | ||
| </span> | ||
| )} | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" sideOffset={4}> | ||
| {displayName} | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| <Tooltip delayDuration={500}> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onClose(); | ||
| }} | ||
| className={cn( | ||
| "mt-1 absolute right-1 top-1/2 -translate-y-1/2 cursor-pointer size-5 group-hover:opacity-100", | ||
| isActive ? "opacity-90" : "opacity-0", | ||
| )} | ||
| aria-label="Close group" | ||
| > | ||
| <HiMiniXMark /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| Close group | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract GroupItem to its own file.
Per coding guidelines, each component should be in its own file. GroupItem should be moved to a separate file like GroupItem.tsx.
Create a new file GroupItem.tsx:
// GroupItem.tsx
import { Button } from "@superset/ui/button";
import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip";
import { cn } from "@superset/ui/utils";
import { HiMiniXMark } from "react-icons/hi2";
import type { Tab } from "renderer/stores/tabs/types";
import { getTabDisplayName } from "renderer/stores/tabs/utils";
interface GroupItemProps {
tab: Tab;
isActive: boolean;
needsAttention: boolean;
onSelect: () => void;
onClose: () => void;
}
export function GroupItem({ tab, isActive, needsAttention, onSelect, onClose }: GroupItemProps) {
// ... existing implementation
}Then import it in GroupStrip.tsx.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
around lines 19-82, the inline GroupItem component should be extracted to its
own file; create
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsx
exporting GroupItem as a named export, copy the component implementation there
and add necessary imports (Button, Tooltip, TooltipTrigger, TooltipContent, cn,
HiMiniXMark, Tab type, getTabDisplayName), then in GroupStrip.tsx remove the
inner GroupItem definition and import { GroupItem } from './GroupItem'; ensure
prop types are preserved and update any relative import paths if needed.
...src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx
Show resolved
Hide resolved
- Restore terminalPersistence setting (removed in cherry-pick, needed by superset-sh#589) - Add missing WorkspaceListItem props (projectId, worktreePath, index) - Fix clearWorkspaceAttention -> clearWorkspaceAttentionStatus - Fix pane.needsAttention -> pane.status check
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:
- Line 50: The prop worktreePath in WorkspaceListItem is typed as string but is
checked for truthiness before use, so update its type in the component
props/interface (e.g., change worktreePath: string to worktreePath?: string or
worktreePath: string | null depending on whether callers pass undefined or null)
and adjust any call sites that pass this prop to match the new signature; ensure
any downstream uses (like the conditional at the usage in WorkspaceListItem) no
longer require non-null assertions and update tests/consumers if they assumed a
strict string.
- Around line 158-162: The reorderWorkspaces.mutate call currently omits error
handling; update the call to pass an options object with an onError handler so
failures are surfaced (e.g., call reorderWorkspaces.mutate({ projectId,
fromIndex: item.index, toIndex: index }, { onError: (error) =>
toast.error(`Failed to reorder workspace: ${error.message}`) })). Ensure you
reference the existing reorderWorkspaces.mutate invocation and use toast.error
to show the error message with context.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (2)
73-73: Add error handling forsetActiveWorkspacemutation.The
setActiveWorkspacemutation lacks anonErrorhandler. If the mutation fails (e.g., database errors), the error will be silently swallowed, violating the coding guideline that requires logging errors with context and surfacing feedback to users.🔎 Recommended fix
-const setActiveWorkspace = useSetActiveWorkspace(); +const setActiveWorkspace = useSetActiveWorkspace({ + onError: (error) => { + console.error('[WorkspaceListItem/setActiveWorkspace]', error); + toast.error(`Failed to activate workspace: ${error.message}`); + }, +});Based on past review feedback and coding guidelines requiring error logging with context.
Also applies to: 121-121
83-92: Add console logging to mutation error handlers.The error handlers for
openInFinderandsetUnreadshow toast notifications but don't log errors to the console. Per the coding guidelines, errors must be logged with context using the pattern[domain/operation] messageto aid debugging.🔎 Recommended fix
const openInFinder = trpc.external.openInFinder.useMutation({ - onError: (error) => toast.error(`Failed to open: ${error.message}`), + onError: (error) => { + console.error('[WorkspaceListItem/openInFinder]', error); + toast.error(`Failed to open: ${error.message}`); + }, }); const setUnread = trpc.workspaces.setUnread.useMutation({ onSuccess: () => { utils.workspaces.getAllGrouped.invalidate(); }, - onError: (error) => - toast.error(`Failed to update unread status: ${error.message}`), + onError: (error) => { + console.error('[WorkspaceListItem/setUnread]', error); + toast.error(`Failed to update unread status: ${error.message}`); + }, });As per coding guidelines requiring prefixed console logging with context.
Based on past review feedback and coding guidelines.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx (1)
17-29: Add visual collapse indicator for better UX.The button toggles a collapsible section but provides no visual cue (such as a chevron or arrow icon) indicating the collapsed/expanded state. Users may not discover that the section is interactive without this visual affordance.
🔎 Suggested enhancement
Consider adding an icon (e.g.,
ChevronDownorChevronRight) that rotates or changes based onisCollapsed:+import { ChevronDown } from "lucide-react"; // or your icon library + export function ProjectHeader({ projectName, isCollapsed, onToggleCollapse, workspaceCount, }: ProjectHeaderProps) { return ( <button type="button" onClick={onToggleCollapse} aria-expanded={!isCollapsed} className={cn( "flex items-center gap-2 w-full px-3 py-2 text-sm font-medium", "hover:bg-muted/50 transition-colors", "text-left cursor-pointer", )} > + <ChevronDown + className={cn( + "h-4 w-4 shrink-0 transition-transform", + isCollapsed && "rotate-180" + )} + /> <span className="truncate flex-1">{projectName}</span> <span className="text-xs text-muted-foreground">{workspaceCount}</span> </button> ); }
Optionally add
aria-controlsfor better screen reader experience.If the button controls a specific element with an ID, adding
aria-controlshelps screen readers announce the relationship between the button and the controlled region.Example
<button type="button" onClick={onToggleCollapse} aria-expanded={!isCollapsed} aria-controls="project-workspaces-list" // reference the controlled element ID className={...} >
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxpackages/local-db/drizzle/0006_add_unique_branch_workspace_index.sql
🧰 Additional context used
📓 Path-based instructions (6)
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/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
🧠 Learnings (5)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Avoid using `any` type - maintain type safety in TypeScript code
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Use prefixed console logging with context pattern: [domain/operation] message
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Never swallow errors silently - at minimum log them with context
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/*/src/components/{ui,ai-elements,react-flow}/*.tsx : Use kebab-case single files for shadcn/ui components (e.g., button.tsx, base-node.tsx) in src/components/ui/, src/components/ai-elements, and src/components/react-flow/
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/*/src/components/**/[A-Z]*.tsx : Create component folder structure with one folder per component: ComponentName/ComponentName.tsx with barrel export index.ts
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/index.ts (1)
ProjectHeader(1-1)packages/ui/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx (1)
1-8: LGTM!The imports and interface are clean and well-typed. The component correctly uses object parameters for the props.
packages/local-db/drizzle/0006_add_unique_branch_workspace_index.sql (1)
4-47: Previous critical issue resolved—migration logic is now correct.The addition of line 25 correctly addresses the NULL comparison bug flagged in the previous review. When both workspaces have
NULLtimestamps, the explicitIS NULLchecks now ensure the id tiebreaker is properly applied.The migration logic is sound across all edge cases:
- Both timestamps NULL → lowest id wins (line 25)
- One NULL, one timestamp → timestamp wins (line 24)
- Both timestamps equal → lowest id wins (line 23)
- Different timestamps → most recent wins (line 22)
The UPDATE and DELETE use consistent ordering (
last_opened_at DESC NULLS LAST, id ASC), and the UPDATE correctly reassignssettings.last_active_workspace_idbefore deletion to prevent orphaned references.
| interface WorkspaceListItemProps { | ||
| id: string; | ||
| projectId: string; | ||
| worktreePath: string; |
There was a problem hiding this comment.
Refine the worktreePath type to match usage.
The worktreePath prop is typed as string, but line 133 performs a truthiness check (if (worktreePath)) before using it, suggesting it may be empty, null, or undefined. For type safety and clarity, consider:
worktreePath?: stringif it's optionalworktreePath: string | nullif it can explicitly be null
This will prevent runtime checks from masking type-level guarantees.
Based on past review feedback.
Also applies to: 133-133
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
around line 50, The prop worktreePath in WorkspaceListItem is typed as string
but is checked for truthiness before use, so update its type in the component
props/interface (e.g., change worktreePath: string to worktreePath?: string or
worktreePath: string | null depending on whether callers pass undefined or null)
and adjust any call sites that pass this prop to match the new signature; ensure
any downstream uses (like the conditional at the usage in WorkspaceListItem) no
longer require non-null assertions and update tests/consumers if they assumed a
strict string.
...rc/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts:
- Around line 13-17: The setUnread mutation currently only invalidates
utils.workspaces.getAllGrouped which can leave utils.workspaces.getActive and
utils.workspaces.getAll stale; update the setUnread mutation (setUnread) to
invalidate the same queries as setActive by calling
utils.workspaces.getActive.invalidate(), utils.workspaces.getAll.invalidate(),
and utils.workspaces.getAllGrouped.invalidate() in its onSuccess handler so all
workspace state (including isUnread) is refreshed consistently.
- Around line 37-49: The undo toast currently calls setUnread.mutate({ id:
variables.id, isUnread: true }) without any failure handling; update the
setUnread mutation (the mutation created as setUnread in useSetActiveWorkspace)
to include an onError callback that shows a failure toast or otherwise surfaces
the error to the user, and/or modify the toast action to call
setUnread.mutateAsync(...) and catch errors to display a toast; ensure you
reference setUnread (and the undo toast action) so failures are reported (e.g.,
using processToast/error toast) instead of being silently swallowed.
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx:
- Around line 8-9: Update the useWorkspaceShortcuts hook to return the tRPC
query loading and error flags (add isLoading and isError alongside groups and
activeWorkspaceId, sourcing them from the underlying queries), then update the
WorkspaceSidebar component to read those new isLoading/isError values and:
render a skeleton/spinner when isLoading is true, render an error message when
isError is true, and only render "No workspaces yet" when both isLoading and
isError are false and groups.length === 0; reference the useWorkspaceShortcuts
hook, the WorkspaceSidebar component, and the groups/activeWorkspaceId values
when making these changes.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (1)
51-53: Consider a more type-safe approach for forwarding onSuccess.The
anycast is documented but might be avoidable. tRPC v11 and React Query have well-typed mutation handlers, and the spread pattern could potentially be typed correctly without casting.This is working code and the intent is clear, so this is optional polish. If you'd like, you could explore whether the current tRPC/React Query types support spreading
...restwithout the cast, or alternatively restructure to pass a properly typedcontextparameter explicitly:await options?.onSuccess?.(data, variables, rest[0]);However, the existing approach is acceptable for now.
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx (1)
11-22: Past review addressed! Optional performance refinement available.The mutation concern from the previous review has been successfully resolved—this code is now pure, memoized, and uses
reduce. The logic is correct and will perform well for typical workspace counts.One minor refinement: the spread operator
...acc.indicescreates a new array on each iteration (O(n²) complexity). While negligible for realistic project counts, a simple for-loop is both more readable and O(n):🔎 Optional: simpler and more performant approach
- // Calculate shortcut base indices for each project group using cumulative offsets - const projectShortcutIndices = useMemo( - () => - groups.reduce<{ indices: number[]; cumulative: number }>( - (acc, group) => ({ - indices: [...acc.indices, acc.cumulative], - cumulative: acc.cumulative + group.workspaces.length, - }), - { indices: [], cumulative: 0 }, - ).indices, - [groups], - ); + // Calculate shortcut base indices for each project group using cumulative offsets + const projectShortcutIndices = useMemo(() => { + const indices: number[] = []; + let cumulative = 0; + for (const group of groups) { + indices.push(cumulative); + cumulative += group.workspaces.length; + } + return indices; + }, [groups]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx
🧰 Additional context used
📓 Path-based instructions (6)
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/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx (3)
apps/desktop/src/renderer/hooks/useWorkspaceShortcuts.ts (1)
useWorkspaceShortcuts(18-139)apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader.tsx (1)
WorkspaceSidebarHeader(3-12)apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx (1)
ProjectSection(37-140)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (2)
apps/desktop/src/renderer/react-query/workspaces/index.ts (1)
useSetActiveWorkspace(7-7)packages/ui/src/components/ui/sonner.tsx (1)
toast(40-40)
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx (2)
1-6: LGTM! Clean imports following conventions.The imports correctly use the renderer alias and avoid Node.js modules in the renderer process.
24-52: LGTM! Well-structured component layout.The component structure is clean and maintainable:
- Proper vertical layout with fixed header/footer and scrollable content
- Correct key prop usage in the map
- Helpful empty state messaging
- All props correctly passed to child components
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
Show resolved
Hide resolved
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
Show resolved
Hide resolved
| export function WorkspaceSidebar() { | ||
| const { groups, activeWorkspaceId } = useWorkspaceShortcuts(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find apps/desktop/src/renderer -name "*useWorkspaceShortcuts*" -o -name "*useWorkspace*" | head -20Repository: superset-sh/superset
Length of output: 348
🏁 Script executed:
cat -n apps/desktop/src/renderer/hooks/useWorkspaceShortcuts.tsRepository: superset-sh/superset
Length of output: 5161
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxRepository: superset-sh/superset
Length of output: 2059
Expose loading and error states from useWorkspaceShortcuts hook.
The hook uses tRPC queries that provide isLoading and isError states, but does not expose them. This causes the component to display "No workspaces yet" during loading and errors, which is confusing to users. Update the hook to return isLoading and isError from the queries, then update the component to:
- Show a skeleton or spinner when
isLoadingis true - Show an error message when
isErroris true - Show "No workspaces yet" only when both are false and
groups.length === 0
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx
around lines 8-9, Update the useWorkspaceShortcuts hook to return the tRPC query
loading and error flags (add isLoading and isError alongside groups and
activeWorkspaceId, sourcing them from the underlying queries), then update the
WorkspaceSidebar component to read those new isLoading/isError values and:
render a skeleton/spinner when isLoading is true, render an error message when
isError is true, and only render "No workspaces yet" when both isLoading and
isError are false and groups.length === 0; reference the useWorkspaceShortcuts
hook, the WorkspaceSidebar component, and the groups/activeWorkspaceId values
when making these changes.
# Conflicts: # apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts # apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx # apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx # apps/desktop/src/renderer/screens/main/index.tsx # apps/desktop/src/renderer/stores/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:
- Around line 165-166: The onError handler for reorderWorkspaces only shows a
toast but doesn't log the error; update the onError callback (the
reorderWorkspaces onError handler) to call console.error with a contextual
prefix like "[workspaces/reorder] Failed to reorder workspace:" and include the
error object (or error.message) before or alongside the existing toast.error
call so the error is logged to the console for debugging.
- Line 76: The hook useWorkspaceRename currently takes two positional args
(workspaceId, workspaceName); change its signature to accept a single params
object like useWorkspaceRename({ workspaceId, workspaceName }) and update its
export to export function useWorkspaceRename({ workspaceId, workspaceName }: {
workspaceId: string; workspaceName: string }). Then update the call in
WorkspaceListItem where rename is created to pass an object:
useWorkspaceRename({ workspaceId: id, workspaceName: name }) (and search/replace
other usages to match the new object parameter shape). Ensure TypeScript types
and any internal references inside useWorkspaceRename use the new parameter
names.
♻️ Duplicate comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (3)
50-50: Refine theworktreePathtype to match usage.The
worktreePathprop is typed asstring, but line 133 performs a truthiness check (if (worktreePath)) before using it, suggesting it may be empty, null, or undefined. For type safety and clarity, consider:
worktreePath?: stringif it's optionalworktreePath: string | nullif it can explicitly be nullThis will prevent runtime checks from masking type-level guarantees.
Based on past review feedback.
Also applies to: 133-133
73-73: Add error handling forsetActiveWorkspacemutation.The
setActiveWorkspacemutation lacks anonErrorhandler. If setting the active workspace fails (e.g., database errors), users won't receive feedback, and the error will be silently swallowed, violating the coding guideline.🔎 Recommended fix
- const setActiveWorkspace = useSetActiveWorkspace(); + const setActiveWorkspace = useSetActiveWorkspace({ + onError: (error) => { + console.error('[WorkspaceListItem/setActiveWorkspace]', error); + toast.error(`Failed to activate workspace: ${error.message}`); + }, + });Based on past review feedback and coding guidelines requiring error logging with context.
Also applies to: 121-121
83-92: Add console logging to mutation error handlers.The error handlers for
openInFinderandsetUnreadshow toast notifications but don't log errors to the console. Per the coding guidelines, errors should be logged with context using the pattern[domain/operation] messagebefore or alongside user-facing notifications.🔎 Recommended fix
const openInFinder = trpc.external.openInFinder.useMutation({ - onError: (error) => toast.error(`Failed to open: ${error.message}`), + onError: (error) => { + console.error('[WorkspaceListItem/openInFinder]', error); + toast.error(`Failed to open: ${error.message}`); + }, }); const setUnread = trpc.workspaces.setUnread.useMutation({ onSuccess: () => { utils.workspaces.getAllGrouped.invalidate(); }, - onError: (error) => - toast.error(`Failed to update unread status: ${error.message}`), + onError: (error) => { + console.error('[WorkspaceListItem/setUnread]', error); + toast.error(`Failed to update unread status: ${error.message}`); + }, });As per coding guidelines requiring prefixed console logging with context.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (2)
64-67: Extract magic number to a named constant.Line 67 uses an inline style with a hardcoded width value. Per coding guidelines, magic numbers should be extracted to named constants at the module top.
🔎 Proposed refactor
Add a constant at the top of the file:
import { GroupItem } from "./GroupItem"; + +const TAB_ITEM_WIDTH = 120; export function GroupStrip() {Then use it in the render:
<div key={tab.id} className="h-full shrink-0" - style={{ width: "120px" }} + style={{ width: `${TAB_ITEM_WIDTH}px` }} >As per coding guidelines, extract magic numbers to named constants.
82-89: Consider adding an aria-label to the New Group button.While the button has a tooltip, adding an explicit
aria-labelwould improve accessibility for screen reader users.🔎 Proposed improvement
<Button variant="ghost" size="icon" className="shrink-0 size-7 mb-1" onClick={handleAddGroup} + aria-label="New Group" >apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
99-105: Consider adding error handling to the GitHub status query.The
getGitHubStatusquery lacks anonErrorhandler. While silent failures for queries are more common than for mutations, logging query errors helps with debugging and visibility, especially for lazy-loaded data.🔎 Suggested enhancement
const { data: githubStatus } = trpc.workspaces.getGitHubStatus.useQuery( { workspaceId: id }, { enabled: hasHovered && type === "worktree", staleTime: GITHUB_STATUS_STALE_TIME, + onError: (error) => { + console.error('[WorkspaceListItem/getGitHubStatus]', error); + }, }, );As per coding guidelines requiring error logging with context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/stores/tabs/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/stores/tabs/utils.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
🧠 Learnings (4)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Use prefixed console logging with context pattern: [domain/operation] message
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Never swallow errors silently - at minimum log them with context
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Avoid using `any` type - maintain type safety in TypeScript code
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/*/src/components/**/*.{ts,tsx} : Co-locate component dependencies: utils, hooks, constants, config, tests, stories in the same folder as the component using them
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsx (4)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getTabDisplayName(19-25)packages/ui/src/components/ui/tooltip.tsx (3)
Tooltip(76-76)TooltipTrigger(76-76)TooltipContent(76-76)packages/ui/src/lib/utils.ts (1)
cn(4-6)packages/ui/src/components/ui/button.tsx (1)
Button(60-60)
⏰ 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). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupItem.tsx (3)
8-14: LGTM! Props interface is well-structured.The interface clearly defines all required props with appropriate types and follows the object parameters pattern for the component's multiple parameters.
25-53: LGTM! Tab selection and attention indicator work correctly.The component properly handles active state styling, text overflow, and the pulsating attention indicator animation. The tooltip provides good UX for truncated labels.
54-76: LGTM! Close button implementation is solid.The close button correctly uses
stopPropagationto prevent triggering the tab selection, includes proper accessibility attributes, and has appropriate hover/active visibility behavior with a tooltip delay to reduce UI noise.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (4)
1-7: LGTM! Imports are clean.The imports properly reference the newly extracted
GroupItemcomponent, addressing the previous review feedback about separating components into individual files.
10-18: LGTM! TRPC and store usage follows guidelines.The component correctly uses TRPC for IPC as required by the guidelines. The multiple Zustand selectors are fine given Zustand's built-in optimization for preventing unnecessary re-renders.
20-41: LGTM! Memoization is appropriately applied.Both
useMemohooks have correct dependencies and provide meaningful performance optimization by preventing unnecessary recalculations of filtered tabs and attention state.
43-57: LGTM! Handler functions are correctly implemented.All handlers appropriately check for an active workspace before performing operations, preventing errors when no workspace is selected.
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
35-35: No action needed: Barrel import is safe and does not create circular dependencies.Verification confirms that the imports from
"./components"(BranchSwitcher, DeleteWorkspaceDialog, WorkspaceHoverCardContent) do not create circular dependencies. None of these components import back to WorkspaceListItem or its parent paths—all dependencies flow in one direction only.Likely an incorrect or invalid review comment.
| const setActiveWorkspace = useSetActiveWorkspace(); | ||
| const reorderWorkspaces = useReorderWorkspaces(); | ||
| const [hasHovered, setHasHovered] = useState(false); | ||
| const rename = useWorkspaceRename(id, name); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for useWorkspaceRename hook definition
find apps/desktop/src/renderer -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "useWorkspaceRename" | head -20Repository: superset-sh/superset
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Also search more broadly for the hook
rg "useWorkspaceRename.*=" apps/desktop/src/renderer --type ts --type tsx -A 5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for hook definitions in hooks directory
rg -A 10 "export.*useWorkspaceRename|function useWorkspaceRename|const useWorkspaceRename" apps/desktop/src/renderer --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
cat -n apps/desktop/src/renderer/screens/main/hooks/useWorkspaceRename/useWorkspaceRename.tsRepository: superset-sh/superset
Length of output: 2042
🏁 Script executed:
#!/bin/bash
cat -n apps/desktop/src/renderer/screens/main/hooks/useWorkspaceRename/index.tsRepository: superset-sh/superset
Length of output: 129
Refactor useWorkspaceRename to accept an object parameter.
The hook currently uses two positional parameters (workspaceId, workspaceName), violating the coding guideline that functions with 2+ parameters should accept a single params object with named properties. Refactor the hook signature to:
export function useWorkspaceRename({ workspaceId, workspaceName }: { workspaceId: string; workspaceName: string })Update the call site accordingly:
const rename = useWorkspaceRename({ id, name: name });🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
around line 76, The hook useWorkspaceRename currently takes two positional args
(workspaceId, workspaceName); change its signature to accept a single params
object like useWorkspaceRename({ workspaceId, workspaceName }) and update its
export to export function useWorkspaceRename({ workspaceId, workspaceName }: {
workspaceId: string; workspaceName: string }). Then update the call in
WorkspaceListItem where rename is created to pass an object:
useWorkspaceRename({ workspaceId: id, workspaceName: name }) (and search/replace
other usages to match the new object parameter shape). Ensure TypeScript types
and any internal references inside useWorkspaceRename use the new parameter
names.
| onError: (error) => | ||
| toast.error(`Failed to reorder workspace: ${error.message}`), |
There was a problem hiding this comment.
Add console logging to reorderWorkspaces error handler.
The error handler for reorderWorkspaces shows a toast notification but doesn't log the error to the console. Per the coding guidelines and the pattern established in the past review (lines 83-92), errors should be logged with context using [domain/operation] message before or alongside user-facing notifications.
🔎 Recommended fix
reorderWorkspaces.mutate(
{
projectId,
fromIndex: item.index,
toIndex: index,
},
{
- onError: (error) =>
- toast.error(`Failed to reorder workspace: ${error.message}`),
+ onError: (error) => {
+ console.error('[WorkspaceListItem/reorderWorkspaces]', error);
+ toast.error(`Failed to reorder workspace: ${error.message}`);
+ },
},
);As per coding guidelines requiring prefixed console logging with context.
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
around lines 165-166, The onError handler for reorderWorkspaces only shows a
toast but doesn't log the error; update the onError callback (the
reorderWorkspaces onError handler) to call console.error with a contextual
prefix like "[workspaces/reorder] Failed to reorder workspace:" and include the
error object (or error.message) before or alongside the existing toast.error
call so the error is logged to the console for debugging.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @apps/desktop/src/lib/trpc/routers/changes/file-contents.ts:
- Around line 133-141: In the catch block that handles PathValidationError (the
code that checks "if (error instanceof PathValidationError) { ... }"), add
logging for unexpected errors before mapping them to {"ok": false, reason:
"not-found"}; specifically, call the module's logger (or processLogger) with
context including the error object and the path/operation, then return the
existing generic failure; keep existing handling for SYMLINK_ESCAPE and
outside-worktree unchanged and only log non-PathValidationError exceptions (or
log all exceptions but preserve current returned reasons).
- Around line 198-202: The catch block around the git.show([spec]) call is
currently swallowing errors; capture the exception (e.g. catch (err)) and log it
with context before returning the empty string so failures are observable; use
the repository's logger (e.g. processLogger / logger) if available, otherwise
console.error, and include the spec and a short message like "failed to show git
object" along with the error details.
In @apps/desktop/src/lib/trpc/routers/changes/status.ts:
- Around line 154-166: In the loop over untracked files that calls secureFs.stat
and secureFs.readFile (using worktreePath and file.path), the catch currently
swallows errors; update the catch to log the error with context (including
file.path and worktreePath) using the module's logger (e.g., processLogger or
existing logger), so permission, validation, symlink-escape, or read errors are
recorded; keep the behavior of skipping the file after logging and preserve
setting file.additions/file.deletions only on success (and still honor
MAX_LINE_COUNT_SIZE).
♻️ Duplicate comments (1)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (1)
13-23: Incomplete query invalidation - previous review comment not addressed.The
setUnreadmutation still only invalidatesgetAllGrouped, but the past review correctly identified thatgetActiveandgetAllalso need invalidation sinceisUnreadis workspace state reflected in all three queries. ThesetActivemutation invalidates all three queries (lines 37-41), andsetUnreadshould do the same for consistency.🔎 Proposed fix for consistent invalidation
const setUnread = trpc.workspaces.setUnread.useMutation({ - onSuccess: () => { - utils.workspaces.getAllGrouped.invalidate(); - }, + onSuccess: async () => { + await Promise.all([ + utils.workspaces.getActive.invalidate(), + utils.workspaces.getAll.invalidate(), + utils.workspaces.getAllGrouped.invalidate(), + ]); + }, onError: (error) => { console.error("[workspace/setUnread] Failed to update unread status:", { error: error.message, }); toast.error(`Failed to undo: ${error.message}`); }, });
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (1)
23-24: Good defensive comment.The note clearly prevents accidental re-introduction of
saveFilein this file. Consider adding a brief security rationale to make it even clearer why this matters (e.g., "to prevent path traversal vulnerabilities").Optional: Enhance comment clarity
-// NOTE: saveFile is defined in file-contents.ts with hardened path validation -// Do NOT add saveFile here - it would overwrite the secure version +// NOTE: saveFile is defined in file-contents.ts with hardened path validation +// to prevent path traversal vulnerabilities. Do NOT add saveFile here - +// it would overwrite the secure versionapps/desktop/src/lib/trpc/routers/changes/status.ts (1)
142-142: Consider logging errors for debuggingWhile not part of the current changes, the empty catch block here also swallows errors silently. For consistency with the error handling improvements in this PR, consider adding logging here as well.
🔎 Suggested improvement
- } catch {} + } catch (error) { + console.error('[changes/status] Failed to get branch comparison:', error); + }apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (1)
252-269: LGTM - Proper unstaged version handling with size protection.The logic correctly handles the working tree file with size checks and secure filesystem operations. The error handling at line 266-268 is appropriate for diff display where missing files are valid, though you might consider adding debug-level logging for troubleshooting purposes.
Optional: Add debug logging for troubleshooting
} catch { // File doesn't exist or validation failed - that's ok for diff display + // Uncomment for debugging: + // console.debug("[file-contents/getUnstagedVersions] File read failed:", error); modified = ""; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/lib/trpc/routers/changes/staging.tsapps/desktop/src/lib/trpc/routers/changes/status.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/changes/staging.ts
- apps/desktop/src/lib/trpc/routers/changes/branches.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/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/lib/trpc/routers/changes/status.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/lib/trpc/routers/changes/status.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/lib/trpc/routers/changes/status.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/lib/trpc/routers/changes/file-contents.tsapps/desktop/src/lib/trpc/routers/changes/status.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
🧠 Learnings (1)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Never swallow errors silently - at minimum log them with context
Applied to files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (1)
apps/desktop/src/lib/trpc/routers/changes/security/path-validation.ts (1)
assertRegisteredWorktree(63-90)
apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (3)
apps/desktop/src/lib/trpc/routers/changes/security/path-validation.ts (2)
assertRegisteredWorktree(63-90)PathValidationError(43-51)apps/desktop/src/lib/trpc/routers/changes/security/index.ts (3)
assertRegisteredWorktree(21-21)secureFs(31-31)PathValidationError(24-24)apps/desktop/src/lib/trpc/routers/changes/security/secure-fs.ts (1)
secureFs(274-469)
apps/desktop/src/lib/trpc/routers/changes/status.ts (2)
apps/desktop/src/lib/trpc/routers/changes/security/path-validation.ts (1)
assertRegisteredWorktree(63-90)apps/desktop/src/lib/trpc/routers/changes/security/secure-fs.ts (1)
secureFs(274-469)
⏰ 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). (6)
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Docs
- GitHub Check: Build
🔇 Additional comments (16)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (2)
27-34: LGTM: Proper error handling with logging and user feedback.The error handler correctly logs the failure with context, shows a user-friendly toast, and forwards to the user's callback. The prefixed logging pattern and error surfacing follow the coding guidelines.
35-60: LGTM: Proper invalidation and undo flow with error handling.The parallel query invalidation is efficient, and the undo toast provides good UX. The previous concern about error handling in the undo action is now addressed by the
onErrorhandler in thesetUnreadmutation definition (lines 17-22), which will catch and surface any failures via toast.The
anyusage at line 59 is appropriately documented and necessary for spread argument compatibility with the mutation options.apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (2)
6-6: LGTM: Security import added.The import of
assertRegisteredWorktreeis correctly placed and consistently used throughout the file to validate worktree access.
35-35: Excellent security hardening with consistent validation.The addition of
assertRegisteredWorktreeat the start of each Git operation mutation provides a critical security layer by ensuring only registered worktrees can be accessed. The consistent application across all five mutations (commit, push, pull, sync, createPR) and fail-fast placement is exactly right.Also applies to: 51-51, 73-73, 98-98, 127-127
apps/desktop/src/lib/trpc/routers/changes/status.ts (4)
5-5: LGTM - Security module integrationThe import of security utilities (
assertRegisteredWorktreeandsecureFs) provides the foundation for the hardening improvements in this file.
23-24: LGTM - Critical security gateThe worktree registration check before git operations prevents unauthorized filesystem access. Correct placement ensures all subsequent operations are validated.
68-69: LGTM - Consistent security patternThe registration check is consistently applied across both public query handlers, ensuring comprehensive protection.
147-148: LGTM - Well-documented constantThe extracted constant follows best practices and the inline comment clearly explains the OOM protection rationale. The 1 MiB threshold is reasonable for line counting.
apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (8)
5-9: LGTM - Security imports properly integrated.The security module imports are well-structured and align with the centralized security approach introduced in this PR.
12-16: LGTM - Constants properly defined.Magic numbers are correctly extracted to named constants at module level, following coding guidelines.
18-44: LGTM - Well-structured types and binary detection.The discriminated union type provides clear success/failure semantics, and the binary detection logic using NUL byte scanning is a standard approach. Note that the
truncatedfield in line 130 is alwaysfalsein the current implementation, suggesting it may be reserved for future truncation functionality.
60-60: LGTM - Security validation at procedure entry.Properly validates worktree registration before any file operations, following the security-first approach.
92-96: LGTM - Secure file writing.Correctly uses
secureFs.writeFileto enforce worktree registration and path traversal protections, preventing symlink escape attacks.
211-214: LGTM - Efficient parallel fetching with size protection.Refactoring to use
Promise.allwithsafeGitShowimproves performance while adding memory protection through size checks.
225-228: LGTM - Consistent parallel fetching pattern.Follows the same efficient parallel fetching pattern with size protection as other version helpers.
238-241: LGTM - Parallel staged version fetching.Correctly uses
Promise.allandsafeGitShowfor efficient, size-protected fetching of staged versions.
| } catch (error) { | ||
| if (error instanceof PathValidationError) { | ||
| if (error.code === "SYMLINK_ESCAPE") { | ||
| return { ok: false, reason: "symlink-escape" }; | ||
| } | ||
| return { ok: false, reason: "outside-worktree" }; | ||
| } | ||
| return { ok: false, reason: "not-found" }; | ||
| } |
There was a problem hiding this comment.
Log errors before mapping to generic failure reason.
The catch-all error handler maps any unhandled exception to "not-found" without logging. This violates the coding guideline "Never swallow errors silently - at minimum log them with context" and can hide permission errors, I/O failures, or other unexpected issues that would be valuable for debugging.
🔎 Proposed fix to add error logging
} catch (error) {
if (error instanceof PathValidationError) {
if (error.code === "SYMLINK_ESCAPE") {
return { ok: false, reason: "symlink-escape" };
}
return { ok: false, reason: "outside-worktree" };
}
+ console.error(
+ "[file-contents/readWorkingFile] Unexpected error:",
+ error,
+ );
return { ok: false, reason: "not-found" };
}📝 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.
| } catch (error) { | |
| if (error instanceof PathValidationError) { | |
| if (error.code === "SYMLINK_ESCAPE") { | |
| return { ok: false, reason: "symlink-escape" }; | |
| } | |
| return { ok: false, reason: "outside-worktree" }; | |
| } | |
| return { ok: false, reason: "not-found" }; | |
| } | |
| } catch (error) { | |
| if (error instanceof PathValidationError) { | |
| if (error.code === "SYMLINK_ESCAPE") { | |
| return { ok: false, reason: "symlink-escape" }; | |
| } | |
| return { ok: false, reason: "outside-worktree" }; | |
| } | |
| console.error( | |
| "[file-contents/readWorkingFile] Unexpected error:", | |
| error, | |
| ); | |
| return { ok: false, reason: "not-found" }; | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/changes/file-contents.ts around lines
133-141, In the catch block that handles PathValidationError (the code that
checks "if (error instanceof PathValidationError) { ... }"), add logging for
unexpected errors before mapping them to {"ok": false, reason: "not-found"};
specifically, call the module's logger (or processLogger) with context including
the error object and the path/operation, then return the existing generic
failure; keep existing handling for SYMLINK_ESCAPE and outside-worktree
unchanged and only log non-PathValidationError exceptions (or log all exceptions
but preserve current returned reasons).
| const content = await git.show([spec]); | ||
| return content; | ||
| } catch { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Log git.show errors before returning empty string.
Returning an empty string without logging when git.show fails violates the coding guideline "Never swallow errors silently - at minimum log them with context." Git errors may indicate repository corruption, missing objects, or other issues that should be visible for debugging.
🔎 Proposed fix to add error logging
const content = await git.show([spec]);
return content;
- } catch {
+ } catch (error) {
+ console.error(
+ `[file-contents/safeGitShow] Failed to fetch ${spec}:`,
+ error,
+ );
return "";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const content = await git.show([spec]); | |
| return content; | |
| } catch { | |
| return ""; | |
| } | |
| const content = await git.show([spec]); | |
| return content; | |
| } catch (error) { | |
| console.error( | |
| `[file-contents/safeGitShow] Failed to fetch ${spec}:`, | |
| error, | |
| ); | |
| return ""; | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/changes/file-contents.ts around lines
198-202, The catch block around the git.show([spec]) call is currently
swallowing errors; capture the exception (e.g. catch (err)) and log it with
context before returning the empty string so failures are observable; use the
repository's logger (e.g. processLogger / logger) if available, otherwise
console.error, and include the spec and a short message like "failed to show git
object" along with the error details.
| for (const file of untracked) { | ||
| try { | ||
| const fullPath = join(worktreePath, file.path); | ||
| const content = await readFile(fullPath, "utf-8"); | ||
| const stats = await secureFs.stat(worktreePath, file.path); | ||
| if (stats.size > MAX_LINE_COUNT_SIZE) continue; | ||
|
|
||
| const content = await secureFs.readFile(worktreePath, file.path); | ||
| const lineCount = content.split("\n").length; | ||
| file.additions = lineCount; | ||
| file.deletions = 0; | ||
| } catch {} | ||
| } catch { | ||
| // Skip files that fail validation or reading | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error logging to meet coding guidelines
While the security improvements using secureFs are excellent, the catch block swallows errors silently, violating the coding guideline: "Never swallow errors silently - at minimum log them with context."
Logging these errors would help diagnose:
- Permission issues
- Symlink escape attempts (security-relevant)
- Path validation failures
- Filesystem errors
🔎 Proposed fix: Add contextual error logging
try {
const stats = await secureFs.stat(worktreePath, file.path);
if (stats.size > MAX_LINE_COUNT_SIZE) continue;
const content = await secureFs.readFile(worktreePath, file.path);
const lineCount = content.split("\n").length;
file.additions = lineCount;
file.deletions = 0;
- } catch {
+ } catch (error) {
+ console.error(`[changes/status] Failed to count lines for ${file.path}:`, error);
// Skip files that fail validation or reading
}As per coding guidelines, ...
🤖 Prompt for AI Agents
In @apps/desktop/src/lib/trpc/routers/changes/status.ts around lines 154-166, In
the loop over untracked files that calls secureFs.stat and secureFs.readFile
(using worktreePath and file.path), the catch currently swallows errors; update
the catch to log the error with context (including file.path and worktreePath)
using the module's logger (e.g., processLogger or existing logger), so
permission, validation, symlink-escape, or read errors are recorded; keep the
behavior of skipping the file after logging and preserve setting
file.additions/file.deletions only on success (and still honor
MAX_LINE_COUNT_SIZE).
Happy Path Demo
Happy Paths Demo
Summary
This PR bundles several related features for the desktop app:
Workbench/Review Mode - A workspace-level view mode toggle:
Configurable Workspace Navigation - A setting to choose between:
Configurable Group Tabs Position - A setting to control where terminal group tabs appear:
Also includes:
Design decisions & future direction:
Part 1: Workbench/Review Mode
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 supports full editing in both Raw and Diff modes:
Security Hardening
File Viewer file operations include robust symlink escape protection:
security/folder with path validation, secure fs, and git command wrapperssecureFs.readFile()andreadFileBuffer()enforce symlink-escape checks internallysymlink-escapereason returned separately fromoutside-worktreefor precise error handlingdata:URLs in markdown images; blocks http/https (tracking pixels), file:// (local file access), and relative paths (traversal attacks)Configurable Terminal File Link Behavior
Added a global setting to control how Cmd+clicking file paths in the terminal behaves:
file:line:colformat navigate to the correct position in File ViewerPart 2: Workspace Navigation Sidebar
Architecture
settings.navigationStylecolumnuseWorkspaceShortcutsextracts keyboard shortcuts (⌘1-9) and auto-create logic shared between both modeszustand/persistHow it works
useWorkspaceShortcutshookSidebar Mode Layout Improvements
In sidebar navigation mode, the "Open In" button and "Workbench/Review" toggle have been moved from the TopBar down to the ContentHeader:
Before:
After:
This keeps the profile dropdown at the top while placing workspace controls closer to the content they affect. The controls are visible in both:
Top-bar navigation mode is unchanged.
Key decisions
string[]notSet<string>getGitHubStatuson hoverNew Components
WorkspaceSidebar
WorkspaceSidebar.tsx- Main sidebar componentWorkspaceSidebarHeader.tsx- "Workspaces" title headerWorkspaceSidebarFooter.tsx- "Add project" buttonResizableWorkspaceSidebar.tsx- Wrapper with resize handleProjectSection
ProjectSection.tsx- Collapsible project group with workspacesProjectHeader.tsx- Project name, color dot, collapse toggleWorkspaceListItem
WorkspaceListItem.tsx- Individual workspace row with lazy-loaded PR dataWorkspaceStatusBadge.tsx- PR status badge (open/merged/closed/draft)WorkspaceDiffStats.tsx- Additions/deletions displayShared
useWorkspaceShortcuts.ts- Shared hook for ⌘1-9 shortcuts and auto-create logicworkspace-sidebar-state.ts- Zustand store for sidebar UI statePart 3: Mark as Unread/Read for Workspaces
Right-click any workspace (in top bar tabs or sidebar) to toggle its "unread" state. This is a workspace-level feature, separate from pane-level agent notifications.
How it works
workspace.isUnread = truein SQLite databaseWhy workspace-level instead of pane-level?
Previously, "Mark as Unread" set
needsAttention: trueon all panes in the workspace, causing:The new approach:
workspace.isUnread(manual reminder) vspane.needsAttention(agent notifications)Why SQLite instead of Zustand/localStorage?
We store
isUnreadin SQLite rather than in-memory state for two key reasons:Persistence across app restarts - Users expect "mark as unread" to survive closing and reopening the app, similar to email clients and Slack. SQLite data persists to disk automatically.
Workspace is a database entity - The
workspacestable already exists in SQLite with other workspace metadata. AddingisUnreadas a column keeps related data together and avoids split-brain between SQLite and localStorage.Implementation details
isUnreadcolumn inworkspacestable (migration:0007_add_workspace_is_unread.sql)workspaces.setUnreadtRPC mutationsetActivemutation now returnswasUnreadand auto-clears + invalidates queriesPart 4: Configurable Group Tabs Position
Why
The initial implementation of this PR moved group tabs from the sidebar (via ModeCarousel + TabsView) to the content header (GroupStrip). This was unintended—the original sidebar behavior should be the default to match the
mainbranch.What
A new setting allows users to choose where terminal group tabs appear:
mainbranchHow it works
settings.groupTabsPositioncolumnKey decisions
mainbranch behavior, minimizes breaking changenavigationStylepatternKeyboard Shortcuts
Manual Test Checklist
Workbench/Review Mode
Terminal Link Behavior
With "File viewer": Cmd+click file:line:col → opens at correct line/column(Known issue - see below)Security (Symlink Escape Protection)
(Skipped - requires manual symlink setup)
Navigation Style Setting
Group Tabs Position Setting
Sidebar Mode
Sidebar Mode Layout
Top Bar Mode
Keyboard Shortcuts (Both Modes)
Mark as Unread/Read Feature
Known Issues
Unsaved changes lost on group/workspace switch - Switching between groups or workspaces while a FileViewer has unsaved changes will discard those changes without warning. The unsaved changes dialog currently only protects Raw↔Diff mode switches within the same pane. Follow-up needed to add protection for group and workspace navigation.
Terminal file links don't navigate to line/column - Cmd+clicking a file path with line:column format (e.g.,
src/file.ts:42:10) opens the file but doesn't navigate to the specified line/column. Will fix in follow-up PR.Future Work
⌘+\or similar to toggle between Workbench/ReviewworkspaceId-based APIs - AcceptworkspaceIdinstead ofworktreePathin renderer callsFiles Changed
New Files (Workbench/Review)
GroupStrip/GroupStrip.tsx- Group switching strip componentFileViewerPane/FileViewerPane.tsx- File viewer with Raw/Rendered/Diff modes + inline editingFileViewerPane/UnsavedChangesDialog.tsx- Confirmation dialog for unsaved changes on mode switchChangesView/components/ViewModeToggle/ViewModeToggle.tsx- Segmented control for mode toggleTopBar/WorkspaceControls/ViewModeToggleCompact.tsx- Compact version for content headerworkspace-view-mode.ts- Per-workspace view mode store0004_add_terminal_link_behavior_setting.sql- Migration for terminal link behavior settingNew Files (Security)
security/index.ts- Security module exportssecurity/path-validation.ts- Path validation utilities with symlink escape detectionsecurity/secure-fs.ts- Secure filesystem operationssecurity/git-commands.ts- Secure git command wrappersSafeImage/SafeImage.tsx- Secure image component for markdown (only allows data: URLs)New Files (Workspace Sidebar)
apps/desktop/src/renderer/hooks/useWorkspaceShortcuts.tsapps/desktop/src/renderer/stores/workspace-sidebar-state.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceSidebarControl.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceControls/- New folder with OpenInMenuButton and WorkspaceControlsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/*(10 files)packages/local-db/drizzle/0005_add_navigation_style.sqlNew Files (Mark as Unread)
packages/local-db/drizzle/0007_add_workspace_is_unread.sql- Migration for isUnread columnNew Files (Group Tabs Position)
packages/local-db/drizzle/0008_add_group_tabs_position.sql- Migration for groupTabsPosition columnapps/desktop/plans/20260104-1916-restore-group-tabs-sidebar.md- Implementation planNew Files (Database)
packages/local-db/drizzle/0006_add_unique_branch_workspace_index.sql- Migration to dedupe existing branch workspaces and add unique index (one branch workspace per project)New Files (Hooks)
useWorkspaceDeleteHandler.ts- Shared hook for workspace delete/close dialog stateDeleted Files
WorkspaceActionBar/WorkspaceActionBar.tsx- Removed (controls moved to TopBar/ContentHeader)WorkspaceActionBar/components/WorkspaceActionBarLeft/- RemovedWorkspaceActionBar/components/WorkspaceActionBarRight/- Renamed toTopBar/WorkspaceControls/OpenInMenuButton.tsxModified Files
file-contents.ts- AddedreadWorkingFileprocedureContentView/index.tsx- Routes to TabsContent or ChangesContent based on mode; conditional GroupStrip based on position settingContentHeader/ContentHeader.tsx- AddedtrailingActionprop for right-side controlsChangesView.tsx- AddedonFileOpencallback propSidebar/index.tsx- Conditional ModeCarousel vs ChangesView based on position settingWorkspaceView/index.tsx- View mode reactivity and ⌘+T behaviortabs/store.ts- Removed oldmarkWorkspaceAsUnread(now uses tRPC)tabs/types.ts- RemovedmarkWorkspaceAsUnreadfrom interfaceshared/tabs-types.ts- Addedfile-viewerpane typeTerminal/helpers.ts- Refactored to acceptonFileLinkClickcallbackTerminal/Terminal.tsx- Wired up terminal link behavior settingBehaviorSettings.tsx- Added terminal link behavior, navigation style, and group tabs position dropdownssettings/index.ts(tRPC) - Added getters/setters for all new settingsschema.ts/zod.ts(local-db) - AddedisUnreadcolumn to workspaces,groupTabsPositionto settingsworkspaces.ts(tRPC) - AddedsetUnreadmutation, updatedsetActiveto auto-clear and returnwasUnreaduseSetActiveWorkspace.ts- Added undo toast for auto-cleared unread stateWorkspaceItemContextMenu.tsx- Now uses tRPC mutation, shows Read/Unread toggleWorkspaceItem.tsx- Uses workspace-levelisUnreadfor indicatorWorkspaceGroup.tsx- PassesisUnreadto WorkspaceItemWorkspaceListItem.tsx- Uses workspace-levelisUnreadfor indicatorProjectSection.tsx- PassesisUnreadto WorkspaceListItemTopBar/index.tsx- Conditional rendering based on navigation style; WorkspaceControls only in top-bar modeTopBar/WorkspaceTabs/index.tsx- Refactored to use shared hookMainScreen/index.tsx- Wire up sidebar and navigation styleshared/constants.ts- AddedDEFAULT_NAVIGATION_STYLEandDEFAULT_GROUP_TABS_POSITIONshared/hotkeys.ts- AddedTOGGLE_WORKSPACE_SIDEBAR, renamedTOGGLE_SIDEBARSummary by CodeRabbit
New Features
Security
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.