Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request refactors worktree path handling by introducing a centralized utility function, propagating it through workspace data structures and UI components, and adding a context menu with "Open in Finder" functionality to workspace items. It includes terminal router updates, UI component modifications, and minor code formatting adjustments. Changes
Sequence DiagramsequenceDiagram
participant User
participant WorkspaceItem
participant ContextMenu as WorkspaceItemContextMenu
participant TRPC
participant System as System (Finder)
User->>WorkspaceItem: Right-click on workspace tab
WorkspaceItem->>ContextMenu: Render context menu
ContextMenu->>User: Display "Rename" and "Open in Finder" options
User->>ContextMenu: Click "Open in Finder"
ContextMenu->>TRPC: Call openInFinder(worktreePath)
TRPC->>System: Execute system open command
System->>User: Open Finder at worktree path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Key areas requiring attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
140-152: Consider usinggetWorktreePathhere for consistency.This procedure manually looks up the worktree path, duplicating the logic now encapsulated in
getWorktreePath. Using the utility here would improve consistency and reduce duplication.getWorkspaceCwd: publicProcedure .input(z.string()) .query(async ({ input: workspaceId }) => { const workspace = db.data.workspaces.find((w) => w.id === workspaceId); if (!workspace) { return undefined; } - - const worktree = db.data.worktrees.find( - (wt) => wt.id === workspace.worktreeId, - ); - return worktree?.path; + return getWorktreePath(workspace.worktreeId); }),apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsx (1)
1-42: Context menu wiring looks good; double‑check worktreePath assumptionsThis component cleanly encapsulates the context menu and correctly guards the
openInFinder.mutatecall behind a truthyworktreePath, while using the trpc alias as per desktop guidelines. One thing to verify is that all callers (and the backing TRPC router) truly guaranteeworktreePathis a string (with “no path” represented as""), notundefined. IfgetWorktreePathcan returnundefinedend‑to‑end, consider either making the propworktreePath?: stringor normalizing it at the data layer so the type here stays accurate. From a UX angle, if some workspaces legitimately have no worktree, it may be worth later disabling or hiding the “Open in Finder” item rather than making it a no‑op on click.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
78-151: Add a null‑check in the drag/drop ref callback for extra safetyThe new structure with
WorkspaceItemContextMenuwrapping the draggable button and inline rename/delete UI looks good and preserves the previous behaviors (activate on mousedown, rename on double‑click, delete via dialog, attention badge, etc.). One small robustness improvement: the ref callback currently callsdrag(drop(node));unconditionally. On unmount, React will invoke the ref withnull, so guarding against that avoids coupling to react‑dnd’s handling ofnull:- <button + <button type="button" - ref={(node) => { - drag(drop(node)); - }} + ref={(node) => { + if (node) { + drag(drop(node)); + } + }}This keeps the behavior identical for mounted elements while making the connector more defensive against lifecycle quirks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(2 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(3 hunks)apps/desktop/src/renderer/globals.css(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(1 hunks)apps/desktop/src/resources/public/theme-boot.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/resources/public/theme-boot.jsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsxapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/index.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsxapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsxapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/index.tsx
apps/desktop/src/lib/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in shared code like
src/lib/electron-router-dom.ts- this code runs in both main and renderer processes
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.tsx: Create one folder per component with structure:ComponentName/ComponentName.tsx+index.tsfor barrel export
Co-locate tests next to the component file they test (e.g.,ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nestedcomponents/subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent'scomponents/folder
One component per file - avoid multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsxapps/desktop/src/renderer/screens/main/index.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods using
window.ipcRenderer.invoke()with object parameters - TypeScript will infer the exact response type automatically
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsxapps/desktop/src/renderer/screens/main/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsxapps/desktop/src/renderer/screens/main/index.tsx
🧬 Code graph analysis (3)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItemContextMenu.tsx (1)
WorkspaceItemContextMenu(17-42)packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (1)
getWorktreePath(6-9)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (1)
getWorktreePath(6-9)
🔇 Additional comments (11)
apps/desktop/src/renderer/screens/main/index.tsx (1)
32-47: Hotkey refactoring looks solid.The three hotkey registrations maintain their original logic with appropriate conditional guards (workspace view checks and workspace ID validation). Dependency arrays are complete and correct for each hook, preventing stale closure issues. The refactoring keeps changes minimal and focused on the hotkey handlers themselves.
apps/desktop/src/renderer/globals.css (1)
74-74: LGTM!Minor numeric formatting cleanup with no visual impact.
apps/desktop/src/resources/public/theme-boot.js (1)
3-12: LGTM!Arrow function IIFE is a valid modernization. Since neither
thisnorargumentsis used here, the behavior is identical.apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (1)
1-9: LGTM!Clean utility function that centralizes worktree path lookup logic. Good use of optional chaining and appropriate return type.
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (2)
6-6: LGTM!Good use of the centralized utility.
49-51: LGTM!Clean refactor using the new utility function. The ternary expression is clear and handles the optional workspace case well.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
15-15: LGTM!Correct import of the new utility.
153-165: LGTM!The type definition is clear and properly includes the new
worktreePathfield.
183-190: LGTM!Good use of spread operator with the new
worktreePathfield. The fallback to empty string is appropriate for cases where the worktree might not exist.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
17-39: Prop plumbing for worktreePath is consistent, just ensure upstream data matchesAdding
worktreePath: stringtoWorkspaceItemPropsand destructuring it alongside the existing fields keeps the API clear and makes it easy to wire into the context menu. This assumes every workspace in this group has a concrete string value; if the TRPC payload can omit or null this field for some workspaces, it would be safer to type it as optional (or normalize it to""at the data layer) to keep types and runtime behavior aligned.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
6-12: worktreePath propagation into WorkspaceItem is straightforward; verify it’s always populatedExtending the
Workspaceinterface withworktreePath: stringand passing it directly intoWorkspaceItemkeeps the data flow simple and matches the new context menu behavior. Just ensure the TRPCworkspacespayload and any mock/test data always includeworktreePathfor every workspace; otherwise you may want to mark it optional here (and inWorkspaceItemProps) or normalize it at the query/transform layer so the type remains truthful.Also applies to: 72-82
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.