feat(desktop): add Workbench/Review mode and workspace navigation sidebar#559
Closed
andreasasprou wants to merge 43 commits intosuperset-sh:mainfrom
Closed
feat(desktop): add Workbench/Review mode and workspace navigation sidebar#559andreasasprou wants to merge 43 commits intosuperset-sh:mainfrom
andreasasprou wants to merge 43 commits intosuperset-sh:mainfrom
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces left-sidebar workspace navigation as an alternative to the top-bar mode, adds a file-viewer pane component for editing and viewing files within tabs, implements per-workspace view-mode switching (workbench vs. review), and adds a comprehensive security layer for git operations and filesystem access with path validation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Settings as BehaviorSettings<br/>(React Component)
participant TRPC as TRPC Router<br/>(settings)
participant DB as LocalDB<br/>(SQLite)
participant MainScreen as MainScreen<br/>(React Component)
participant TopBar as TopBar<br/>(React Component)
participant Sidebar as WorkspaceSidebar<br/>(React Component)
User->>Settings: Change navigation style<br/>(select Top bar / Sidebar)
Settings->>TRPC: setNavigationStyle(style)
Note over TRPC: Apply optimistic<br/>update
TRPC->>DB: Upsert settings.navigation_style
DB-->>TRPC: OK
TRPC-->>Settings: Success
Settings->>MainScreen: Re-query via useQuery
MainScreen->>MainScreen: Compute effectiveNavigationStyle
rect rgba(34, 197, 94, 0.1)
Note over MainScreen,Sidebar: If sidebar mode enabled
MainScreen->>TopBar: Pass navigationStyle prop
MainScreen->>Sidebar: Render ResizableWorkspaceSidebar
end
rect rgba(239, 68, 68, 0.1)
Note over MainScreen,TopBar: If top-bar mode enabled
MainScreen->>TopBar: Pass navigationStyle prop
TopBar->>TopBar: Render WorkspacesTabs instead
end
TopBar-->>User: UI updates (show sidebar control or tabs)
Sidebar-->>User: Sidebar visibility toggles
sequenceDiagram
actor User
participant Terminal as Terminal<br/>(React Component)
participant Store as TabsStore<br/>(Zustand)
participant TRPC as TRPC Router<br/>(settings)
participant FilePane as FileViewerPane<br/>(React Component)
participant Editor as Monaco Editor<br/>(or Markdown)
User->>Terminal: Click file path link<br/>in terminal output
Terminal->>TRPC: Query getTerminalLinkBehavior
TRPC-->>Terminal: "file-viewer" | "external-editor"
rect rgba(34, 197, 94, 0.1)
Note over Terminal,FilePane: If file-viewer behavior
Terminal->>Store: addFileViewerPane(workspaceId,<br/>{filePath, diffCategory, ...})
Store->>Store: Create/reuse file-viewer pane
Store-->>Terminal: Return paneId
Terminal->>FilePane: Mount with filePath
FilePane->>Editor: Fetch content via readWorkingFile
Editor-->>FilePane: Render markdown/raw/diff
FilePane-->>User: Display file in editor pane
end
rect rgba(239, 68, 68, 0.1)
Note over Terminal: Else external-editor behavior
Terminal->>TRPC: external.openFileInEditor.mutate
TRPC-->>User: Open in external editor
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (33)
.agents/commands/create-plan-file.md (7)
29-42: Add language identifier to fenced code block.The example code block lacks a language identifier, which improves readability and syntax highlighting in documentation renderers.
🔎 Proposed fix
- Example questions: - ``` + Example questions: + ```markdown I reviewed the existing auth implementation in apps/web/src/app/auth/. Where should the new OAuth provider live?
56-64: Reduce sentence-start repetition for readability.Three successive sentences begin with "Every ExecPlan must", which creates a rhythmic monotony. Varying the sentence structure would improve flow and readability.
🔎 Proposed revision
Instead of:
* Every ExecPlan must be fully self-contained. Self-contained means… * Every ExecPlan is a living document. Contributors are required… * Every ExecPlan must enable a complete novice to implement…Consider:
* Every ExecPlan must be fully self-contained. Self-contained means… * As a living document, ExecPlans require contributors to revise… * A complete novice should be able to implement the feature end-to-end…
76-81: Vary sentence structure to improve readability.Three successive sentences begin with "Do not", which creates repetitive emphasis. Reordering or restructuring a few clauses would maintain the directive tone while improving prose flow.
🔎 Suggested revision
Instead of:
Avoid common failure modes. Do not rely on undefined jargon. Do not describe "the letter of a feature" so narrowly that the resulting code compiles but does nothing meaningful. Do not outsource key decisions to the reader.Consider:
Avoid common failure modes: do not rely on undefined jargon; avoid describing "the letter of a feature" so narrowly that the resulting code compiles but does nothing meaningful; and take responsibility for key decisions rather than outsourcing them to the reader.
88-88: Format bare URL in backticks for consistency.The URL
http://localhost:3000/healthappears unformatted in prose. Wrapping it in backticks improves readability and distinguishes it as a code reference.- Acceptance should be phrased as behavior a human can verify ("after starting the server, navigating to http://localhost:3000/health returns HTTP 200 with body OK") + Acceptance should be phrased as behavior a human can verify ("after starting the server, navigating to `http://localhost:3000/health` returns HTTP 200 with body OK")
133-133: Simplify "overall goal" for conciseness.The phrase "overall goal" is somewhat redundant. Simply "goal" conveys the same meaning more directly.
- Each milestone must be independently verifiable and incrementally implement the overall goal of the execution plan. + Each milestone must be independently verifiable and incrementally implement the goal of the execution plan.
72-72: Standardize emphasis style to underscores.Per the codebase conventions, emphasis should use underscores (
_text_) rather than asterisks (*text*) for consistency.- When writing an ExecPlan to a Markdown (.md) file where the content of the file *is only* the single ExecPlan, you should omit the triple backticks. + When writing an ExecPlan to a Markdown (.md) file where the content of the file _is only_ the single ExecPlan, you should omit the triple backticks.
244-259: Add language identifier to directory structure code block.The directory structure code block lacks a language identifier. Adding one improves formatting and readability.
- ``` + ```plaintext apps/<app-name>/.agents/plans/ # App-specific plans <active-plans>.md done/ abandoned/packages/local-db/drizzle/0005_add_navigation_style.sql (1)
1-1: Consider adding a DEFAULT value to the migration.The migration adds
navigation_styleas a nullable text column without a SQL-level default. While the application layer handles defaults via the TRPC router, adding a DEFAULT constraint in the migration would ensure data consistency and simplify null handling.🔎 Proposed fix with DEFAULT constraint
-ALTER TABLE `settings` ADD `navigation_style` text; +ALTER TABLE `settings` ADD `navigation_style` text DEFAULT 'top-bar';This aligns with the
DEFAULT_NAVIGATION_STYLEconstant mentioned in the PR context and ensures existing rows have a consistent value.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceStatusBadge.tsx (1)
22-25: Align color naming with existing component.The merged state uses
purple-500here, but the similarPRStatusBadgecomponent (referenced in relevant snippets) usesviolet-500for the merged state. For visual consistency across the UI, consider aligning with the existing component.🔎 Proposed fix for color consistency
merged: { - icon: <LuGitMerge className={cn(iconClass, "text-purple-500")} />, - bgColor: "bg-purple-500/10", + icon: <LuGitMerge className={cn(iconClass, "text-violet-500")} />, + bgColor: "bg-violet-500/15", },This matches the color and opacity used in
PRStatusBadge(apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/PRStatusBadge/PRStatusBadge.tsx).apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
148-170: Consider consistent error handling for custom link click handler.The fallback path (lines 155-168) includes comprehensive error logging with path details. However, when
onFileLinkClickis provided (lines 151-152), any errors will bubble up without logging. Consider whether callers consistently handle errors, or if defensive logging here would be beneficial.🔎 Optional: Add error boundary
if (onFileLinkClick) { - onFileLinkClick(path, line, column); + try { + onFileLinkClick(path, line, column); + } catch (error) { + console.error( + "[Terminal] Failed to handle file link click:", + path, + error, + ); + } } else {packages/local-db/src/schema/schema.ts (1)
115-118: Consider relocating NavigationStyle to zod.ts for consistency.While the current inline definition works,
TerminalLinkBehavioris defined inzod.tsfollowing the same const-array-to-type pattern. MovingNavigationStylethere would maintain consistency and make both enums discoverable in one place.Suggested refactor
In
packages/local-db/src/schema/zod.ts:+/** + * Navigation style options + */ +export const NAVIGATION_STYLES = ["top-bar", "sidebar"] as const; + +export type NavigationStyle = (typeof NAVIGATION_STYLES)[number];Then in this file:
import type { ExternalApp, GitHubStatus, GitStatus, + NavigationStyle, TerminalLinkBehavior, TerminalPreset, WorkspaceType, } from "./zod"; -/** - * Navigation style for workspace display - */ -export type NavigationStyle = "top-bar" | "sidebar";apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (1)
22-33: Consider useMemo for handleFileOpen callback.While the current conditional definition works, wrapping it in
useMemowould make the dependency chain explicit and prevent unnecessary rerenders ofChangesViewwhen unrelated state changes.Suggested refactor
- const handleFileOpen = - viewMode === "workbench" && workspaceId - ? (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { - addFileViewerPane(workspaceId, { - filePath: file.path, - diffCategory: category, - commitHash, - oldPath: file.oldPath, - }); - } - : undefined; + const handleFileOpen = useMemo( + () => + viewMode === "workbench" && workspaceId + ? (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { + addFileViewerPane(workspaceId, { + filePath: file.path, + diffCategory: category, + commitHash, + oldPath: file.oldPath, + }); + } + : undefined, + [viewMode, workspaceId, addFileViewerPane], + );apps/desktop/src/lib/trpc/routers/changes/status.ts (1)
147-176: Secure file reading with appropriate safeguards.The implementation correctly:
- Uses
secureFsfor path validation and symlink safety- Caps file size at 1 MiB to prevent OOM on large files
- Silently skips files that fail validation or reading
Consider adding debug-level logging in the catch block to aid troubleshooting without affecting production behavior.
🔎 Optional: Add debug logging for skipped files
} catch { - // Skip files that fail validation or reading + // Skip files that fail validation or reading (debug: consider logging for troubleshooting) }If you have a logger available, you could optionally log at debug level:
} catch (err) { // Log at debug level for troubleshooting, silently skip in production logger?.debug?.(`Skipped untracked file ${file.path}:`, err); }apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx (1)
6-15: Consider usinguseMemofor shortcut index calculation.The
projectShortcutIndicescomputation runs on every render. While the current implementation is correct, memoizing the calculation based ongroupswould be more efficient if the component re-renders frequently.🔎 Optional: Memoize shortcut indices
+import { useMemo } from "react"; import { useWorkspaceShortcuts } from "renderer/hooks/useWorkspaceShortcuts"; // ... export function WorkspaceSidebar() { const { groups, activeWorkspaceId } = useWorkspaceShortcuts(); - // Calculate shortcut base indices for each project group - let shortcutIndex = 0; - const projectShortcutIndices = groups.map((group) => { - const baseIndex = shortcutIndex; - shortcutIndex += group.workspaces.length; - return baseIndex; - }); + // Calculate shortcut base indices for each project group + const projectShortcutIndices = useMemo(() => { + let index = 0; + return groups.map((group) => { + const baseIndex = index; + index += group.workspaces.length; + return baseIndex; + }); + }, [groups]);apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ResizableWorkspaceSidebar.tsx (1)
77-91: Consider adding keyboard support for the resize handle.The resize handle has appropriate ARIA attributes (
role="separator",aria-orientation,aria-valuenow, etc.) andtabIndex={0}, but lacksonKeyDownhandler for keyboard-based resizing. Users who focus the handle via keyboard cannot adjust the width.🔎 Suggested keyboard handler
+ const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + const step = e.shiftKey ? 50 : 10; + if (e.key === "ArrowLeft") { + e.preventDefault(); + setWidth(Math.max(MIN_WORKSPACE_SIDEBAR_WIDTH, width - step)); + } else if (e.key === "ArrowRight") { + e.preventDefault(); + setWidth(Math.min(MAX_WORKSPACE_SIDEBAR_WIDTH, width + step)); + } + }, + [width, setWidth], + ); <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/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx (1)
30-39: Consider awaitingtoast.promisefor consistent error handling.The
toast.promisecall is not awaited, sohandleOpenNewProjectreturns immediately after starting workspace creation. Whiletoast.promisehandles displaying errors, this means:
- The button re-enables before workspace creation completes (only
openNew.isPendingis checked on line 55)- If
createBranchWorkspacefails, the outercatchblock won't handle it🔎 Proposed fix
// Create a main workspace on the current branch for the new project - toast.promise( + await toast.promise( createBranchWorkspace.mutateAsync({ projectId: result.project.id }), { loading: "Opening project...", success: "Project opened", error: (err) => err instanceof Error ? err.message : "Failed to open project", }, );apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx (1)
6-14: Consider importing theWorkspacetype from a shared location.The local
Workspaceinterface duplicates fields that likely exist in a shared schema type. This could lead to drift if the workspace schema evolves.If a shared workspace type exists (e.g., from the database schema or TRPC router), consider importing it instead:
-interface Workspace { - id: string; - projectId: string; - worktreePath: string; - type: "worktree" | "branch"; - branch: string; - name: string; - tabOrder: number; -} +import type { Workspace } from "renderer/types/workspaces"; // or appropriate pathapps/desktop/src/renderer/hooks/useWorkspaceShortcuts.ts (1)
32-63: Potential stale reference in effect dependency.The
createBranchWorkspacemutation object is included in the dependency array but will be a new reference on each render (from the hook return). This causes the effect to re-run on every render, though theisCreatingguard prevents actual re-execution.Consider using a ref for the mutation or extracting the stable
mutatefunction:🔎 Suggested improvement
+ const createBranchWorkspaceMutate = createBranchWorkspace.mutate; + // Auto-create main workspace for new projects (one-time per project) useEffect(() => { if (isCreating) return; for (const group of groups) { // ... - createBranchWorkspace.mutate( + createBranchWorkspaceMutate( { projectId }, { onSettled: () => { setIsCreating(false); }, }, ); break; } - }, [groups, isCreating, createBranchWorkspace]); + }, [groups, isCreating, createBranchWorkspaceMutate]);apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
34-37: Consider usingcnutility for className consistency.The codebase appears to use the
cnutility (imported in other files like WorkspaceListItem.tsx) for conditional class merging. Using template literals with ternary operators here works but is inconsistent with the rest of the codebase.🔎 Suggested change
+import { cn } from "@superset/ui/utils"; + <button type="button" onClick={onSelect} - className={` - px-3 py-1 text-xs rounded-md transition-colors flex items-center gap-1.5 max-w-[120px] - ${isActive ? "bg-accent text-accent-foreground" : "hover:bg-muted text-muted-foreground hover:text-foreground"} - `} + className={cn( + "px-3 py-1 text-xs rounded-md transition-colors flex items-center gap-1.5 max-w-[120px]", + isActive + ? "bg-accent text-accent-foreground" + : "hover:bg-muted text-muted-foreground hover:text-foreground", + )} >apps/desktop/src/renderer/stores/tabs/utils.ts (1)
117-123: Case-sensitive file extension check may miss uppercase variants.The
.endsWith()checks for markdown extensions are case-sensitive. Files likeREADME.MDornotes.Markdownwon't be detected as renderable markdown.🔎 Proposed fix
+ 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"; }apps/desktop/src/renderer/stores/tabs/store.ts (2)
359-374: Edge case: Creating a file-viewer when no active tab creates a terminal pane first, then overwrites it.When
!activeTab, the code callsaddTab()which creates a terminal pane, then immediately replaces it with a file-viewer pane. This works but leaves orphaned terminal state (the terminal pane'sisNew: trueflag,name: "Terminal") partially in the overwritten object. Consider creating the tab with a file-viewer pane directly.🔎 Proposed fix - create file-viewer tab directly
// 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; + const state = get(); + const tabId = generateId("tab"); + const fileViewerPane = createFileViewerPane(tabId, options); + const workspaceTabs = state.tabs.filter( + (t) => t.workspaceId === workspaceId, + ); + const tab: Tab = { + id: tabId, + name: fileViewerPane.name, + workspaceId, + layout: fileViewerPane.id, + createdAt: Date.now(), + }; + set({ + tabs: [...state.tabs, tab], + panes: { ...state.panes, [fileViewerPane.id]: fileViewerPane }, + activeTabIds: { ...state.activeTabIds, [workspaceId]: tabId }, + focusedPaneIds: { ...state.focusedPaneIds, [tabId]: fileViewerPane.id }, + }); + return fileViewerPane.id; }Note: You'd need to import
generateIdand theTabtype if not already available.
393-403: Duplicated view mode determination logic.The view mode logic here duplicates
createFileViewerPaneinutils.ts(lines 114-123). Consider extracting a shared helper to avoid drift.🔎 Proposed fix - extract helper in utils.ts
In
utils.ts, add:export const getDefaultFileViewerMode = ( filePath: string, diffCategory?: ChangeCategory, ): FileViewerMode => { if (diffCategory) return "diff"; const lowerPath = filePath.toLowerCase(); if ( lowerPath.endsWith(".md") || lowerPath.endsWith(".markdown") || lowerPath.endsWith(".mdx") ) { return "rendered"; } return "raw"; };Then in
store.ts:- // Determine default view mode - let viewMode: "raw" | "rendered" | "diff" = "raw"; - if (options.diffCategory) { - viewMode = "diff"; - } else if ( - options.filePath.endsWith(".md") || - options.filePath.endsWith(".markdown") || - options.filePath.endsWith(".mdx") - ) { - viewMode = "rendered"; - } + const viewMode = getDefaultFileViewerMode( + options.filePath, + options.diffCategory, + );apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
217-218: Consider importing navigation style enum values from a shared source.The navigation style values are hardcoded as
z.enum(["top-bar", "sidebar"])whileTERMINAL_LINK_BEHAVIORSis imported. For consistency and to avoid drift, consider importing from@superset/local-dbor a shared constants file.Based on the relevant snippet from
packages/local-db/src/schema/schema.ts,NavigationStyleis defined as a type but there's no exported array constant for Zod validation.🔎 Proposed fix - add and use a shared constant
In
packages/local-db/src/schema/zod.ts(alongsideTERMINAL_LINK_BEHAVIORS):export const NAVIGATION_STYLES = ["top-bar", "sidebar"] as const;Then in the router:
- .input(z.object({ style: z.enum(["top-bar", "sidebar"]) })) + .input(z.object({ style: z.enum(NAVIGATION_STYLES) }))apps/desktop/src/renderer/stores/workspace-sidebar-state.ts (2)
54-72: Potential UX issue:setWidthwith small positive values snaps to minimum width.If a user drags the sidebar to a very small width (e.g., 50px) intending to close it, the
width > 0check will clamp it toMIN_WORKSPACE_SIDEBAR_WIDTH(220px) and keep it open. Consider adding a threshold below which the sidebar closes.🔎 Proposed fix - add close threshold
+const CLOSE_THRESHOLD = 100; // Close if dragged below this width setWidth: (width) => { + // Close the sidebar if dragged below threshold + if (width > 0 && width < CLOSE_THRESHOLD) { + set({ + width: 0, + isOpen: false, + }); + return; + } + const clampedWidth = Math.max( MIN_WORKSPACE_SIDEBAR_WIDTH, Math.min(MAX_WORKSPACE_SIDEBAR_WIDTH, width), ); if (width > 0) { set({ width: clampedWidth, lastOpenWidth: clampedWidth, isOpen: true, }); } else { set({ width: 0, isOpen: false, }); } },
86-88:isProjectCollapsedas a store method is unconventional for Zustand.Zustand stores typically expose selectors externally rather than as store methods. While this works, it's a bit unusual. The method itself is correct.
This is fine as-is, but an alternative pattern would be to use a selector:
// External usage const isCollapsed = useWorkspaceSidebarStore( (state) => state.collapsedProjectIds.includes(projectId) );apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
14-17:activeWorkspacequery runs even in top-bar mode where it's not displayed.The
trpc.workspaces.getActive.useQuery()runs regardless of navigation style, but the result is only rendered in sidebar mode. This is minor since the query is likely cached and used elsewhere, but could be skipped for top-bar mode.🔎 Proposed fix - conditionally enable query
- const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery(); + const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery( + undefined, + { enabled: navigationStyle === "sidebar" }, + );apps/desktop/src/renderer/stores/workspace-view-mode.ts (1)
50-52: Consider adding aversionto the persist config for future migrations.The
workspace-sidebar-state.tsstore usesversion: 1, but this store omits it. Adding a version now makes future schema migrations easier.🔎 Proposed fix
{ name: "workspace-view-mode-store", + version: 1, },apps/desktop/src/lib/trpc/routers/changes/file-contents.ts (1)
177-200:safeGitShowsilently returns empty string on errors, which may mask legitimate issues.While this is intentional for "file doesn't exist" cases, it also swallows unexpected errors (permission issues, git corruption, etc.). Consider logging unexpected errors for debugging.
🔎 Proposed fix - log unexpected errors
async function safeGitShow( git: ReturnType<typeof simpleGit>, spec: string, ): Promise<string> { try { // Preflight: check blob size before loading into memory try { const sizeOutput = await git.raw(["cat-file", "-s", spec]); const blobSize = Number.parseInt(sizeOutput.trim(), 10); if (!Number.isNaN(blobSize) && blobSize > MAX_FILE_SIZE) { return `[File content truncated - exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit]`; } } catch { // cat-file failed (blob doesn't exist) - let git.show handle the error } const content = await git.show([spec]); return content; - } catch { + } catch (error) { + // Expected: file doesn't exist in the specified ref + // Log unexpected errors for debugging + const message = error instanceof Error ? error.message : String(error); + if (!message.includes("does not exist") && !message.includes("bad revision")) { + console.warn(`[safeGitShow] Unexpected error for spec "${spec}":`, message); + } return ""; } }apps/desktop/src/lib/trpc/routers/changes/security/secure-fs.ts (1)
100-109: Consider distinguishing validation errors from non-existence.Returning
falsefor both validation failures and non-existent files could mask malicious path attempts. Consider returning a discriminated result or logging validation failures for security monitoring, while keeping the current behavior for callers that only need a boolean.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx (1)
169-187: Consider extracting state update to a store action.The direct state mutation via
useTabsStore.setState()inside the mutation callback works but bypasses the store's action pattern used elsewhere. Consider adding anupdatePaneFileVieweraction to the store for consistency.apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (2)
13-14: ImportNavigationStylefrom@superset/local-dbinstead of redefining.This type is already exported from
packages/local-db/src/schema/schema.ts. Import it alongsideTerminalLinkBehaviorto maintain a single source of truth.🔎 Proposed fix
-import type { TerminalLinkBehavior } from "@superset/local-db"; +import type { NavigationStyle, TerminalLinkBehavior } from "@superset/local-db"; import { Label } from "@superset/ui/label"; ... -type NavigationStyle = "top-bar" | "sidebar"; - export function BehaviorSettings() {
117-119: Use shared constants for default values.Import and use
DEFAULT_NAVIGATION_STYLE,DEFAULT_CONFIRM_ON_QUIT, andDEFAULT_TERMINAL_LINK_BEHAVIORfromapps/desktop/src/shared/constants.tsinstead of hardcoding the fallback values at lines 118, 144, and 163 for consistency with server-side defaults.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
79-103: Consider user-facing error feedback for file open failures.The current implementation logs errors to console, but users won't see why a file failed to open. Consider showing a toast notification on failure for better UX.
Comment on lines
+73
to
+80
| ## Progress | ||
|
|
||
| - [ ] Initial plan created and awaiting approval | ||
| - [ ] (Pending) Milestone 1: Add navigation style setting | ||
| - [ ] (Pending) Milestone 2: Create WorkspaceSidebar component | ||
| - [ ] (Pending) Milestone 3: Create sidebar-mode TopBar variant | ||
| - [ ] (Pending) Milestone 4: Wire up setting to conditionally render layouts | ||
| - [ ] (Pending) Milestone 5: Polish and validation |
Contributor
There was a problem hiding this comment.
Update progress checkboxes to reflect implementation status.
The document states it's a "living document" where Progress should be kept up to date, but all milestones are still marked as pending despite implementation code being in this PR.
🤖 Prompt for AI Agents
In apps/desktop/.agents/plans/20251231-1200-workspace-sidebar-navigation.md
around lines 73-80, the Progress checkboxes are stale; update them to reflect
the actual implementation in this PR by marking the appropriate items as
completed (e.g., check "Initial plan created", and whichever Milestone steps are
implemented — change “[ ]” to “[x]”), add a one-line status note with the PR
number or date and who completed the work, and keep any remaining pending items
unchanged; ensure the file still reads as a living document by adding a short
instruction to update Progress on future changes.
Comment on lines
+1
to
+10
| import { LuLayers } from "react-icons/lu"; | ||
|
|
||
| export function WorkspaceSidebarHeader() { | ||
| return ( | ||
| <div className="flex items-center gap-2 px-3 py-2 border-b border-border"> | ||
| <LuLayers className="w-4 h-4 text-muted-foreground" /> | ||
| <span className="text-sm font-medium">Workspaces</span> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restructure to follow component folder convention.
Based on coding guidelines, components should follow the pattern ComponentName/ComponentName.tsx with an index.ts barrel export. This file should be restructured as:
WorkspaceSidebar/WorkspaceSidebarHeader/WorkspaceSidebarHeader.tsxWorkspaceSidebar/WorkspaceSidebarHeader/index.ts(re-exporting the component)
Based on learnings, this organizational pattern improves consistency and maintainability across the codebase.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader.tsx
lines 1-10: the component file must be moved to the folder-per-component
convention. Create directory
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/,
move the current WorkspaceSidebarHeader.tsx into that directory (keeping the
same component name and contents), and add an index.ts that re-exports the
component (export { WorkspaceSidebarHeader } from "./WorkspaceSidebarHeader";).
Update any imports in the codebase that referenced the old file path to import
from the new barrel export path.
Comment on lines
+218
to
+241
| // Editor mount handler - set up Cmd+S keybinding | ||
| const handleEditorMount: OnMount = useCallback( | ||
| (editor) => { | ||
| editorRef.current = editor; | ||
| // Store original content for dirty tracking (only if not restoring draft) | ||
| // If we have draft content, originalContentRef is already set to the file content | ||
| if (!draftContentRef.current) { | ||
| originalContentRef.current = editor.getValue(); | ||
| } | ||
| // P1: Update dirty state based on restored draft content | ||
| setIsDirty(editor.getValue() !== originalContentRef.current); | ||
|
|
||
| // Register save action with Cmd+S / Ctrl+S | ||
| editor.addAction({ | ||
| id: "save-file", | ||
| label: "Save File", | ||
| keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyS], | ||
| run: () => { | ||
| handleSaveRaw(); | ||
| }, | ||
| }); | ||
| }, | ||
| [handleSaveRaw], | ||
| ); |
Contributor
There was a problem hiding this comment.
Stale closure in Monaco save action registration.
The handleSaveRaw callback is captured at mount time in the Monaco action. If handleSaveRaw changes (e.g., when filePath or worktreePath changes), the registered action will still call the old closure. Use a ref pattern to ensure the action always calls the current handler.
🔎 Proposed fix using ref pattern
+ // Ref to ensure Monaco action always calls current save handler
+ const handleSaveRawRef = useRef(handleSaveRaw);
+ handleSaveRawRef.current = handleSaveRaw;
+
// Editor mount handler - set up Cmd+S keybinding
- const handleEditorMount: OnMount = useCallback(
- (editor) => {
+ const handleEditorMount: OnMount = useCallback((editor) => {
editorRef.current = editor;
// Store original content for dirty tracking (only if not restoring draft)
// If we have draft content, originalContentRef is already set to the file content
if (!draftContentRef.current) {
originalContentRef.current = editor.getValue();
}
// P1: Update dirty state based on restored draft content
setIsDirty(editor.getValue() !== originalContentRef.current);
// Register save action with Cmd+S / Ctrl+S
editor.addAction({
id: "save-file",
label: "Save File",
keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyS],
run: () => {
- handleSaveRaw();
+ handleSaveRawRef.current();
},
});
- },
- [handleSaveRaw],
- );
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Editor mount handler - set up Cmd+S keybinding | |
| const handleEditorMount: OnMount = useCallback( | |
| (editor) => { | |
| editorRef.current = editor; | |
| // Store original content for dirty tracking (only if not restoring draft) | |
| // If we have draft content, originalContentRef is already set to the file content | |
| if (!draftContentRef.current) { | |
| originalContentRef.current = editor.getValue(); | |
| } | |
| // P1: Update dirty state based on restored draft content | |
| setIsDirty(editor.getValue() !== originalContentRef.current); | |
| // Register save action with Cmd+S / Ctrl+S | |
| editor.addAction({ | |
| id: "save-file", | |
| label: "Save File", | |
| keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyS], | |
| run: () => { | |
| handleSaveRaw(); | |
| }, | |
| }); | |
| }, | |
| [handleSaveRaw], | |
| ); | |
| // Ref to ensure Monaco action always calls current save handler | |
| const handleSaveRawRef = useRef(handleSaveRaw); | |
| handleSaveRawRef.current = handleSaveRaw; | |
| // Editor mount handler - set up Cmd+S keybinding | |
| const handleEditorMount: OnMount = useCallback((editor) => { | |
| editorRef.current = editor; | |
| // Store original content for dirty tracking (only if not restoring draft) | |
| // If we have draft content, originalContentRef is already set to the file content | |
| if (!draftContentRef.current) { | |
| originalContentRef.current = editor.getValue(); | |
| } | |
| // P1: Update dirty state based on restored draft content | |
| setIsDirty(editor.getValue() !== originalContentRef.current); | |
| // Register save action with Cmd+S / Ctrl+S | |
| editor.addAction({ | |
| id: "save-file", | |
| label: "Save File", | |
| keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyS], | |
| run: () => { | |
| handleSaveRawRef.current(); | |
| }, | |
| }); | |
| }, []); |
Comment on lines
+291
to
+297
| // P1-1: Update originalContentRef when raw content loads (dirty tracking fix) | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: Only update baseline when content loads | ||
| useEffect(() => { | ||
| if (rawFileData?.ok === true && !isDirty) { | ||
| originalContentRef.current = rawFileData.content; | ||
| } | ||
| }, [rawFileData]); |
Contributor
There was a problem hiding this comment.
Potential stale closure issue with isDirty reference.
The effect depends on rawFileData but also checks isDirty which isn't in the dependency array. If isDirty changes between renders, this check may use a stale value. Consider including isDirty in dependencies or using a ref.
🔎 Suggested fix
- // biome-ignore lint/correctness/useExhaustiveDependencies: Only update baseline when content loads
useEffect(() => {
if (rawFileData?.ok === true && !isDirty) {
originalContentRef.current = rawFileData.content;
}
- }, [rawFileData]);
+ }, [rawFileData, isDirty]);🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsx
around lines 291-297, the useEffect reads isDirty but doesn’t include it in the
dependency array which can cause a stale-closure bug; update the effect so it
either includes isDirty in the dependency array (e.g., [rawFileData, isDirty])
or read a stable ref (e.g., isDirtyRef.current) inside the effect, and remove or
adjust the biome-ignore comment accordingly so linting accurately reflects the
intended dependencies.
Comment on lines
+40
to
+43
| // Get worktree path for file viewer panes | ||
| const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery(); | ||
| const worktreePath = activeWorkspace?.worktreePath ?? ""; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the FileViewerPane component
fd -t f "FileViewerPane" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/Repository: superset-sh/superset
Length of output: 193
🏁 Script executed:
# Read the FileViewerPane component to understand worktreePath handling
wc -l apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxRepository: superset-sh/superset
Length of output: 197
🏁 Script executed:
# Search for worktreePath usage in FileViewerPane
rg -n "worktreePath" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxRepository: superset-sh/superset
Length of output: 622
🏁 Script executed:
# Read the FileViewerPane component to understand worktreePath handling and error states
sed -n '70,300p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxRepository: superset-sh/superset
Length of output: 6943
🏁 Script executed:
# Continue reading FileViewerPane to see what gets rendered when worktreePath is empty
sed -n '300,400p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxRepository: superset-sh/superset
Length of output: 2720
🏁 Script executed:
# Continue reading to see the complete render logic and error handling
sed -n '400,500p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxRepository: superset-sh/superset
Length of output: 2724
🏁 Script executed:
# Read the final render/return section to see the complete UI structure
sed -n '500,624p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/FileViewerPane.tsxRepository: superset-sh/superset
Length of output: 3678
🏁 Script executed:
# Check the TabView component where worktreePath is derived and passed to FileViewerPane
sed -n '1,150p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxRepository: superset-sh/superset
Length of output: 4373
Add a visual indicator when workspace is not available to file-viewer panes.
When activeWorkspace is undefined, worktreePath defaults to an empty string. FileViewerPane correctly disables queries and handlers when this occurs (using !!worktreePath guards), but the UI shows generic messages like "No diff available" or "File not found" instead of clearly indicating the workspace is unavailable. Users cannot distinguish between missing workspace vs. actual file issues.
Consider rendering an explicit message when worktreePath is empty: "No workspace loaded. Switch to a workspace to enable file editing."
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx
around lines 40-43, the code defaults worktreePath to an empty string which
causes file-viewer panes to show generic messages; update the TabView to detect
when worktreePath is falsy and render an explicit, visible message like "No
workspace loaded. Switch to a workspace to enable file editing." (e.g., a small
banner or centered notice above/inside the pane) instead of rendering the
FileViewerPane or relying on its generic messages; ensure this check happens
before passing worktreePath to child panes so they remain disabled and the
message is keyboard-accessible and styled consistently with app UI.
Contributor
Author
PR Review Fixes AppliedAddressed all review feedback in commit a149549: P0 ✅ Branch workspace security validationExtended P1 ✅ Terminal file-viewer absolute pathsTerminal now normalizes absolute paths to worktree-relative before opening file viewer pane. P2 ✅ Misleading security commentsFixed comments that incorrectly claimed symlink checks exist. Follow-up: Migrate to
|
| Current (after this fix) | Proposed |
|---|---|
Renderer resolves path, passes worktreePath |
Renderer passes workspaceId |
| Server validates path against worktrees + projects | Server resolves path from workspace record |
| Two validation paths | Single source of truth |
Benefits of migration:
- Eliminates path spoofing/casing edge cases
- Naturally supports branch workspaces without special-casing
- Cleaner API semantics
Migration approach:
- Add
workspaceIdas optional input to changes routers - Create
resolveWorkspacePathById()helper using existinggetWorkspacePath() - When
workspaceIdprovided, resolve server-side; fallback to legacyworktreePath - Update renderer to pass
workspaceId - Deprecate
worktreePathinput
Effort: ~2-3 hours. Should be a clean follow-up PR after this ships.
Introduces a workspace-level view mode toggle allowing users to switch between: - **Workbench mode**: Mosaic panes layout with terminals + file viewers for in-flow work - **Review mode**: Dedicated Changes page for focused code review Key changes: - Add ViewModeToggle component in workspace header (prominent segmented control) - Add FileViewerPane with Raw/Rendered/Diff modes, lock/unlock, and split support - Add GroupStrip for group switching above Mosaic content - Unify sidebar to use full ChangesView in both modes (with onFileOpen callback) - Add workspace-view-mode store with per-workspace persistence - Add readWorkingFile tRPC procedure for safe file reads (size/binary checks) - Wire file clicks to open/reuse FileViewer panes (MRU unlocked policy) - Cmd+T in Review mode switches to Workbench first, then creates terminal
- Add !!worktreePath checks to FileViewerPane query enabled conditions - Add !!worktreePath checks to save handler guards (handleSaveRaw, handleSaveDiff) - Fix stale state reference after addTab() in addFileViewerPane action Addresses CodeRabbit review feedback.
… and UX improvements - Add validatePathForWrite() to prevent path traversal attacks in saveFile - Add aria-pressed attribute to ViewModeToggle buttons for accessibility - Increase close button touch target in GroupStrip for better UX - Add .mdx file support for rendered view mode
P0 Security: - Add path validation to getUnstagedVersions before readFile (prevents traversal) - Key Editor/DiffViewer by filePath to force remount (fixes stale Cmd+S closure) P1 Fixes: - Update originalContentRef when raw content loads (dirty tracking fix) - Add .mdx to isMarkdown check for toolbar consistency - Gate diff editable to staged/unstaged only (against-main/committed now read-only) P2 Performance: - Add safeGitShow helper with 2MB size limit on all git show calls - Add size check before reading working tree files in getUnstagedVersions
- P0-1: Add file-viewer type and fileViewer object to paneSchema for tab persistence - P0-2: Fix symlink escape vulnerability in validatePathForWrite by checking if target is symlink and resolving parent directory paths - P1-1: Pass defaultBranch to FileViewerPane for against-main diffs - P1-2: Switch staged diff to unstaged after save (matches Review mode behavior) - P2: Use Buffer.byteLength instead of string.length in safeGitShow for accurate UTF-8 byte counting
P0 (critical):
- Remove duplicate saveFile from git-operations.ts that was overwriting
the hardened version in file-contents.ts (security vulnerability)
P1 (must fix):
- Use basename()/dirname() from node:path instead of split('/') for
cross-platform Windows compatibility in validatePathForWrite
- Add preflight size check with git cat-file -s in safeGitShow to
prevent memory spikes from large blobs before materializing content
P2 (UX):
- Fix split pane to clone file-viewer state instead of creating terminal
when splitting a file-viewer pane (locked by default)
Question fix:
- Show GroupStrip with add button even when tabs.length === 0 so users
have visible UI to create new terminal (not just hotkey)
…ocalDb
P0 (CRITICAL SECURITY):
- Add validateWorktreePathInDb() that verifies worktreePath exists in
localDb.worktrees before any filesystem operations
- Without this, a compromised renderer could read/write arbitrary files
by passing worktreePath='/' and filePath='.ssh/id_rsa'
- Applied to getFileContents, saveFile, and readWorkingFile procedures
P1 (correctness):
- Replace startsWith('..') checks with segment-aware containsPathTraversal()
and isPathOutsideBase() helpers that use path.sep
- Fixes false positives on valid paths like '..foo/bar' (directories
starting with '..')
- Cross-platform compatible (handles both / and \ separators)
P2 (performance):
- Guard killTerminalForPane() calls on pane.type === 'terminal'
- Prevents unnecessary IPC and warning logs when closing file-viewer panes
…ve editor drafts P0 (security): - Extract assertWorktreePathInDb to shared security.ts - Returns worktree record to avoid duplicate queries - Apply validation to ALL routes: git-operations, status, staging, branches P1 (data loss): - Preserve unsaved editor content across view mode switches - Store draft in ref before switching away from raw mode - Restore draft when returning to raw mode - Clear draft on save and file change - Update dirty state on editor mount with draft content
…otection P0 Security (BLOCK): - Add validatePathInWorktree() check before rm() in deleteUntracked - Prevents path traversal (../) and symlink escape attacks - Consolidated path validation utilities in security.ts P1 Data Loss: - Track save source (Raw vs Diff) with savingFromRawRef - Only clear draft when saving from Raw mode - Disable Diff editing when Raw draft exists (forces user to save/discard) P2: - Use ['--', filePath] in git.add() to handle paths starting with -
…th validation - Reduce security.ts from 213 to 65 lines - Remove async symlink/realpath checking (users own their repos) - Remove segment-aware path traversal (..foo edge case not worth complexity) - Keep worktreePath DB validation (the real security boundary) - Keep simple .. and absolute path checks (sufficient for 99.99% of cases)
P0 (blocking): - Add path validation (rejects .. and absolute) to applyUntrackedLineCount - Add 1MB size cap to prevent OOM on large untracked files during polling P2: - Add type guard in updateTabLayout - only call killTerminalForPane for terminal panes - Use ['--', branch] in switchBranch to ensure branch treated as refname
…lidation - Add path-validation.ts with industry-standard containment check (path.relative) - Add secure-fs.ts with self-validating FS wrappers (symlink escape protection) - Add git-commands.ts with semantic helpers (gitSwitchBranch vs gitCheckoutFile) - Fix switchBranch: use 'git switch' instead of 'git checkout --' (was file checkout) - Fix path traversal check: segment-aware (allows ..foo, rejects ..) - Fix symlink bypass: check parent dirs for new files, use stat not lstat - Fix worktree root deletion: explicit allowRoot check - All FS operations now go through secureFs with validation built-in
Add a setting to control whether Cmd+clicking file paths in the terminal opens them in an external editor (default) or in the in-app FileViewerPane. - Add terminalLinkBehavior setting to local-db schema with migration - Add getTerminalLinkBehavior/setTerminalLinkBehavior tRPC procedures - Refactor createTerminalInstance to accept onFileLinkClick callback - Wire up setting in Terminal.tsx with ref pattern (avoids terminal recreation) - Add Select UI in BehaviorSettings for choosing link behavior
Remove async symlink escape detection from path validation since the threat model doesn't justify it: a compromised renderer already has terminal access for arbitrary command execution. Changes: - Replace resolveSecurePath (async) with resolvePathInWorktree (sync) - Remove assertNoSymlinkEscape and checkSymlinks option - Add validateRelativePath for simple path safety checks - Update secure-fs.ts to use new sync functions - Update threat model documentation in path-validation.ts
…s sidebar) Add a setting to let users choose between displaying workspaces as horizontal tabs in the TopBar (current behavior) or in a dedicated left sidebar (new feature, similar to Linear/GitHub Desktop). Key changes: - Add navigationStyle column to settings table (migration 0005) - Add navigation style dropdown in Behavior Settings - Create WorkspaceSidebar component with collapsible project sections - Create shared useWorkspaceShortcuts hook (⌘1-9 shortcuts, auto-create) - Update TopBar to conditionally render based on navigation style - Add ⌘⇧B hotkey to toggle workspace sidebar 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ition P0: Add CreateWorkspaceButton to TopBar when in sidebar mode - Button was previously only rendered via WorkspaceTabs - Now renders in top-right section when navigationStyle is sidebar P1: Fix race condition in createBranchWorkspace - Add unique partial index on (projectId) WHERE type='branch' - Use INSERT ON CONFLICT DO NOTHING to handle concurrent calls - If conflict, fetch the existing workspace instead of failing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WorkspaceListItem now has full feature parity: - Close/delete button with confirmation dialog - Context menu (Rename, Open in Finder) - Hover card with PR details, checks, reviews - Needs attention indicator (red pulse) - Inline rename (double-click) - Drag & drop reordering - BranchSwitcher for branch workspaces Other changes: - Remove CreateWorkspaceButton from TopBar in sidebar mode - Add per-project "Add workspace" dropdown (New Workspace, Quick Create) - Add preSelectedProjectId to modal store for pre-selecting project - Simplify GroupStrip to use consistent browser-tab style 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Extract useWorkspaceDeleteHandler hook for shared delete logic - Use existing extractPaneIdsFromLayout from tabs/utils instead of inline collectPaneIds - Add named constants for magic numbers (staleTime, delays, shortcut index) - Reduces code duplication between WorkspaceItem and WorkspaceListItem 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ar mode Move SidebarControl to shared location (screens/main/components/) and conditionally render based on navigation style: - top-bar mode: toggle in TopBar (unchanged) - sidebar mode: toggle in WorkspaceActionBar (left side, before branch info) This improves UX by placing the sidebar toggle closer to the content it controls when in sidebar navigation mode.
Default to Mac layout (80px padding) while platform query is loading, since undefined === 'darwin' evaluates to false, causing 16px padding to be used initially on Mac before the query resolves.
…minal links P0: Fix branch workspace support in assertRegisteredWorktree - Extended validation to check both worktrees.path AND projects.mainRepoPath - Branch workspaces use mainRepoPath which wasn't being validated P1: Fix terminal file-viewer links for absolute paths - Normalize absolute paths to worktree-relative before opening file viewer - File viewer expects relative paths but terminal links can be absolute P2: Fix misleading security comments - Removed claims about symlink checks that aren't implemented - Comments now accurately describe worktree registration + path traversal validation
- Add markWorkspaceAsUnread action to tabs store that sets needsAttention=true for all panes in a workspace - Add context menu item with LuEyeOff icon to: - WorkspaceItemContextMenu (top bar tabs) - WorkspaceListItem (sidebar) for both branch and worktree workspaces - Leverages existing needsAttention indicator system (red pulsing dot) - Logs when no panes exist in workspace (empty workspace edge case)
Previously, clicking a workspace only set it as active but didn't clear the needsAttention state on panes. This meant 'Mark as Unread' worked but clicking the workspace didn't mark it as read. Added clearWorkspaceAttention action that clears needsAttention for all panes in a workspace, called when clicking workspace in both top bar and sidebar.
P0: Make unique branch workspace migration resilient to existing duplicates - Dedupe existing duplicate branch workspaces before creating unique index - Update settings.last_active_workspace_id if it points to deleted workspace P1: Fix isResizing persistence and store selector usage - Exclude ephemeral isResizing from Zustand persistence via partialize - Use selector pattern in MainScreen to avoid unnecessary re-renders P1: Fix delete handler to show dialog when canDeleteData is undefined - Safe default: show confirmation when query fails P2: Fix path boundary check in terminal file links - Use path.startsWith(cwd + '/') to avoid false prefix matches P2: Add missing drizzle migration snapshot - Created 0006_snapshot.json for consistency
P0: Add symlink escape protection for File Viewer writes - Block writes if file's realpath escapes worktree boundary - Add isSymlinkEscaping() method for UI to detect and warn users - Update threat model docs to reflect new symlink protection P1: Fix race condition in createBranchWorkspace ordering - Insert workspace first, then shift others only on success - Losers of the race no longer corrupt tabOrder - Exclude newly inserted workspace from shift operation P2: Fix migration to keep most recently used duplicate - Select survivor by last_opened_at DESC (not arbitrary MIN(id)) - Use id ASC as tiebreaker when last_opened_at is equal
andreasasprou
force-pushed
the
workspace-sidebar
branch
from
391f555 to
d4e1456
Compare
andreasasprou
changed the title
16 tasks
Consolidate two navigation bars into a single unified top bar: - Create WorkspaceControls component group in TopBar/ - BranchIndicator: shows current branch (keyboard-focusable) - OpenInMenuButton: compact Open button with dropdown - ViewModeToggleCompact: Workbench/Review toggle (24px height) - Remove WorkspaceActionBar from WorkspaceView entirely - Delete unused WorkspaceActionBar component folder Improvements based on Oracle review: - Optimize Zustand selector to minimize rerenders - Add toast error handling for open/copy mutations - Add path to Open button tooltip for discoverability - Add focus-visible rings for keyboard accessibility - Use text-xs for consistent typography Also includes minor lint auto-fixes (import sorting, template literals)
- Simplify useWorkspaceDeleteHandler to always show dialog instead of auto-deleting clean workspaces - Update tooltip from 'Delete workspace' to 'Close or delete' for clarity - Add isUnread state for workspace unread tracking - Add workspace setUnread mutation and schema migration
- Fix hotkey() typo to defineHotkey() in hotkeys.ts - Add getHotkey() helper that returns string (not null) for useHotkeys compatibility - Replace all HOTKEYS.*.keys usages with getHotkey() calls - Fix 'against-main' to 'against-base' in ChangeCategory references - Add proper type annotation in WorkspaceSidebarControl.tsx
Replace intense bg-tertiary-active background with subtle border-b-2 border-border for active tab state - cleaner visual that's less heavy
Change from bottom border to top/left/right borders for a traditional tab appearance that connects to content below
…n mode Show SidebarControl in GroupStrip header and EmptyTabView when using sidebar navigation style, allowing users to toggle the changes sidebar
…egression The sidebar toggle was missing when switching to Review mode because it was rendered inside TabsContent, which doesn't render in review mode. Changes: - Create ContentHeader component at ContentView level with leadingAction slot - ContentView now owns SidebarControl placement for both workbench and review modes - Remove SidebarControl from TabsContent and EmptyTabView (now handled by parent) - Remove unused BranchIndicator component and HOTKEYS import (cleanup)
CMD+T creates a new Group (tab container), not a terminal pane. This aligns the hotkey naming with the existing 'New Group' button tooltip.
34 tasks
andreasasprou
added a commit
to andreasasprou/superset
that referenced
this pull request
Jan 2, 2026
…ked) into release
In sidebar navigation mode, move the 'Open In' button and 'Workbench/Review' toggle from TopBar down to ContentHeader alongside the group tabs. This keeps the profile dropdown in TopBar while placing workspace controls closer to the content they affect. Changes: - ContentHeader: Add trailingAction prop for right-side controls - ContentView: Pass WorkspaceControls as trailingAction when isSidebarMode - TopBar: Only render WorkspaceControls when not in sidebar mode The controls are visible in both Workbench (with group tabs) and Review (without group tabs) modes. Top-bar navigation mode is unchanged.
This was referenced Jan 3, 2026
…andling Security fixes (code review feedback): - P0-1: Fix ENOENT handling in secure-fs to detect dangling symlinks and validate parent directory chains for write operations - P0-2: Add centralized symlink-escape enforcement to secureFs.readFile and readFileBuffer - reads now blocked if symlink escapes worktree - Add 'symlink-escape' error reason to distinguish from other validation errors - Use path.relative() for safer boundary checks instead of string prefix matching - P1: Handle undefined workspaceCwd gracefully in Terminal file-viewer mode by falling back to external editor when workspace is still initializing - P2: Update FileViewerState schema comment to clarify intentional omission of transient fields (initialLine/initialColumn) - P2: Document unique branch-workspace index limitation in schema.ts Feature completion: - Terminal file links now pass line/column to File Viewer for initial scroll - FileViewerPane applies initial line/column navigation in raw mode - Add initialLine/initialColumn to FileViewerState and store types
- P0: Add dangling symlink validation to prevent writes outside worktree - P2: Fix isPathWithinWorktree to not incorrectly reject '..config' dirs - P1: Add symlink-escape error message in FileViewerPane - P1: Fix line/column navigation when clicking same file with new coords
P2 from review: The dangling symlink target check in assertParentInWorktree
was still using startsWith('..') which incorrectly rejects '..config' dirs.
Now uses the same pattern as isPathWithinWorktree.
P0: secureFs.delete symlink escape - Check if target is symlink via lstat before deletion - Symlinks: delete the link itself (safe - lives in worktree) - Files/dirs: validate realpath is within worktree before rm - Prevents attack: docs -> /victim, delete docs/file deletes /victim/file P1: Block external images in markdown renderer - Add SafeImage component that blocks http:// and https:// URLs - Shows 'External image blocked' placeholder for external images - Allows relative paths and data: URLs (safe for repo content) - Prevents tracking pixels and privacy leaks from untrusted repos Design decisions (reviewer questions): - Markdown remote images: Blocked by default. No opt-in toggle yet. - Deleting symlinks: Allow deleting the symlink file itself (it lives in worktree). Never follow the symlink during deletion.
P0: Previous fix only blocked http/https but missed: - file:// URLs (arbitrary local file access) - Absolute paths /... or \... (become file:// in Electron) - Relative paths with .. (escape repo boundary) - UNC paths //server/share (Windows NTLM credential leak) Now uses strict allowlist: ONLY data: URLs are allowed. All other sources show 'Image blocked' placeholder. Future: Could add opt-in secure loader for repo-relative images via secureFs validation + blob: URL serving.
Contributor
Author
|
Recreating PR from main repo now that I have direct access |
35 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR bundles two related features for the desktop app:
Workbench/Review Mode - A workspace-level view mode toggle:
Configurable Workspace Navigation - A setting to choose between:
Also includes:
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:
secureFs.readFile()andreadFileBuffer()enforce symlink-escape checks internallysymlink-escapereason returned separately fromoutside-worktreefor precise error handlingConfigurable 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 queriesKeyboard Shortcuts
Manual Test Checklist
Workbench/Review Mode
Terminal Link Behavior
Security (Symlink Escape Protection)
Navigation Style Setting
Sidebar Mode
Sidebar Mode Layout
Top Bar Mode
Keyboard Shortcuts (Both Modes)
Mark as Unread/Read Feature
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 editingViewModeToggle/ViewModeToggle.tsx- Segmented control for mode toggleworkspace-view-mode.ts- Per-workspace view mode store0004_add_terminal_link_behavior_setting.sql- Migration for terminal link behavior settingNew 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/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 columnModified Files
file-contents.ts- AddedreadWorkingFileprocedureContentView/index.tsx- Routes to TabsContent or ChangesContent based on mode; passes WorkspaceControls in sidebar modeContentHeader/ContentHeader.tsx- AddedtrailingActionprop for right-side controlsChangesView.tsx- AddedonFileOpencallback propSidebar/index.tsx- PassesonFileOpenin Workbench modeWorkspaceActionBar.tsx- Added ViewModeToggle to headerWorkspaceView/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 and navigation style dropdownssettings/index.ts(tRPC) - Added getters/setters for new settingsschema.ts/zod.ts(local-db) - AddedisUnreadcolumn to workspacesworkspaces.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_STYLEshared/hotkeys.ts- AddedTOGGLE_WORKSPACE_SIDEBAR, renamedTOGGLE_SIDEBARsecure-fs.ts- Added symlink escape protection for reads and ENOENT handling