Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds StartView and related UI (ActionCard, CloneRepoDialog, StartTopBar), keyboard shortcuts settings, a cloneRepo TRPC mutation and getHomeDir query, theme tweaks plus a new Ember theme, tooltip API props, app-state settingsSection, and the lucide-react dependency. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as CloneRepoDialog (Renderer)
participant TRPCClient as TRPC Client
participant Backend as Desktop Backend (projects router)
participant Git as simple-git
participant DB as Projects Store
participant Workspace as Workspace Creator
User->>Renderer: enter repo URL, click "Clone"
Renderer->>TRPCClient: mutation cloneRepo(url)
TRPCClient->>Backend: invoke cloneRepo handler
Backend->>User: prompt for target directory (folder picker)
User-->>Backend: selects directory or cancels
alt cancelled
Backend-->>TRPCClient: return cancellation response
TRPCClient-->>Renderer: mutation error (cancelled)
Renderer->>User: show cancelled/error
else proceed
Backend->>DB: check existing project at target path
alt project exists & path valid
DB-->>Backend: return existing project
Backend-->>TRPCClient: return reused project
TRPCClient-->>Renderer: success (project)
Renderer->>Workspace: create workspace for project
else path free
Backend->>Git: git.clone(repoUrl, targetPath)
alt clone succeeds
Git-->>Backend: success
Backend->>DB: create new Project record
DB-->>Backend: persisted project
Backend-->>TRPCClient: return new project info
TRPCClient-->>Renderer: success
Renderer->>Workspace: create workspace for new project
else clone fails
Git-->>Backend: error
Backend-->>TRPCClient: return git error
TRPCClient-->>Renderer: display error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
apps/desktop/package.json (1)
67-67: lucide-react addition looks fine; ensure granular icon importsThe new
lucide-reactdependency is reasonable for icon usage. Please double‑check:
- Version compatibility with your current React/Vite stack.
- That call sites import specific icons (e.g.,
import { X } from "lucide-react") rather than a catch‑all to keep the renderer bundle lean.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
1-14: Settings content correctly wired to store-backed sections
- Importing
SettingsSectionfromrenderer/storeskeeps the view aligned with the global settings state.- Adding the
"keyboard"branch andKeyboardShortcutsSettingscleanly extends the existing conditional rendering.- The
flex justify-centercontainer is reasonable for centering both settings panels; just confirm it matches the intended layout on smaller viewports.Based on learnings, using the
rendereralias here is spot‑on.apps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsx (1)
35-35: Consider making loading text configurable.The hardcoded
"Opening..."text may not be appropriate for all actions (e.g., the "Clone repo" button should probably say "Cloning..."). Consider adding an optionalloadingLabelprop.interface ActionCardProps { icon: LucideIcon; label: string; onClick?: () => void; disabled?: boolean; isLoading?: boolean; + loadingLabel?: string; } export function ActionCard({ icon: Icon, label, onClick, disabled = false, isLoading = false, + loadingLabel = "Loading...", }: ActionCardProps) {Then use it:
-{isLoading ? "Opening..." : label} +{isLoading ? loadingLabel : label}apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx (1)
123-128: Potential duplicate key warning with consolidated workspace shortcuts.When workspace shortcuts are consolidated, the synthetic key
"meta+1-9"is used. If the original hotkeys list somehow retained individual workspace entries, you could get duplicate React keys. The current logic prevents this, but using a more unique key (like combining label and keys) would be more robust.<HotkeyRow - key={hotkey.keys} + key={`${hotkey.category}-${hotkey.keys}`} hotkey={hotkey} isEven={index % 2 === 0} />apps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsx (2)
90-97: Consider resetting URL when dialog is closed via Cancel.When the user clicks Cancel, the URL input is not reset. If they reopen the dialog, the previously entered URL will still be visible. Consider resetting on close:
<button type="button" - onClick={onClose} + onClick={() => { + setUrl(""); + onClose(); + }} disabled={isLoading} className="..." > Cancel </button>
58-60: Consider adding backdrop click to close the dialog.The modal overlay doesn't close when clicking outside the dialog content. Adding this would improve UX:
-<div className="fixed inset-0 z-50 flex items-center justify-center bg-black/70 backdrop-blur-sm"> +<div + className="fixed inset-0 z-50 flex items-center justify-center bg-black/70 backdrop-blur-sm" + onClick={(e) => { + if (e.target === e.currentTarget && !isLoading) { + setUrl(""); + onClose(); + } + }} +>apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
146-148: Clone operation lacks timeout and progress feedback.
git.clone()can hang indefinitely on network issues or large repositories. Consider adding a timeout or using simple-git's progress callback for user feedback.const git = simpleGit({ timeout: { block: 60000 } }); // 60 second timeout await git.clone(input.url, clonePath);apps/desktop/src/renderer/screens/main/index.tsx (1)
71-72: Consider excludingisErrorfromshowStartViewcondition.If the query is in an error state,
showStartViewwould betrue(since!isLoading && !activeWorkspace), but the error block on lines 98-141 returns early. The current code is functionally correct due to the early return, but the logic could be clearer.const showStartView = - !isLoading && !activeWorkspace && currentView !== "settings"; + !isLoading && !isError && !activeWorkspace && currentView !== "settings";apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (3)
77-93: Consider extracting the inline SVG logo to a separate component.The 15-line SVG path clutters the component. Extract to a dedicated
SupersetLogocomponent for better readability and reusability.
183-196: Tooltip on a<span>inside a button may have accessibility issues.The
TooltipTriggerwraps a<span>that isn't focusable. Since it's inside an already-interactive button, keyboard users won't be able to focus the tooltip trigger independently. Consider attaching the tooltip to the parent button or ensuring the path info is accessible via other means (e.g.,titleattribute or screen reader text).
201-210: Potential display of negative "remaining" count.If
visibleCountexceedsrecentProjects.length(e.g., user clicks "Load more" multiple times rapidly), the calculationrecentProjects.length - visibleCountcould be negative. ThehasMoreToLoadguard should prevent this, but a safeguard in the display would be defensive.<button type="button" onClick={() => setVisibleCount((c) => c + 50)} className="w-full px-2 py-2 text-muted-foreground text-xs font-normal hover:text-foreground transition-colors" > - Load more ({recentProjects.length - visibleCount}{" "} + Load more ({Math.max(0, recentProjects.length - visibleCount)}{" "} remaining) </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
apps/desktop/package.json(1 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/StartView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(4 hunks)apps/desktop/src/renderer/stores/app-state.ts(1 hunks)apps/desktop/src/shared/hotkeys.ts(1 hunks)apps/desktop/src/shared/themes/built-in/dark.ts(1 hunks)apps/desktop/src/shared/themes/built-in/ember.ts(1 hunks)apps/desktop/src/shared/themes/built-in/index.ts(3 hunks)apps/desktop/src/shared/themes/built-in/light.ts(1 hunks)apps/desktop/src/shared/themes/built-in/monokai.ts(1 hunks)apps/desktop/src/shared/themes/built-in/one-dark.ts(1 hunks)packages/ui/src/components/tooltip.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
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/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/shared/themes/built-in/ember.tsapps/desktop/src/shared/themes/built-in/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsxapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/shared/themes/built-in/monokai.tsapps/desktop/src/shared/themes/built-in/one-dark.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/built-in/dark.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.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/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/shared/themes/built-in/ember.tsapps/desktop/src/shared/themes/built-in/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsxapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/shared/themes/built-in/monokai.tsapps/desktop/src/shared/themes/built-in/one-dark.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/built-in/dark.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.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/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/shared/themes/built-in/ember.tsapps/desktop/src/shared/themes/built-in/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsxapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/shared/themes/built-in/monokai.tsapps/desktop/src/shared/themes/built-in/one-dark.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/stores/app-state.tspackages/ui/src/components/tooltip.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/built-in/dark.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsx
**/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/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsxapps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxpackages/ui/src/components/tooltip.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.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/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsxapps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.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/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsxapps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.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/projects/projects.ts
packages/ui/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use shadcn/ui components and TailwindCSS v4 for all UI component styling in the shared UI package
Files:
packages/ui/src/components/tooltip.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
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/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/renderer/stores/app-state.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/main/**/*.{ts,tsx} : Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Applied to files:
apps/desktop/src/lib/trpc/routers/projects/projects.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
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/SettingsView/SettingsContent.tsx
🧬 Code graph analysis (9)
apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsx (1)
apps/desktop/src/renderer/stores/app-state.ts (2)
useSettingsSection(47-48)useSetSettingsSection(49-50)
apps/desktop/src/shared/themes/built-in/ember.ts (1)
apps/desktop/src/shared/themes/built-in/index.ts (1)
emberTheme(32-32)
apps/desktop/src/shared/themes/built-in/index.ts (1)
apps/desktop/src/shared/themes/built-in/ember.ts (1)
emberTheme(7-97)
apps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsx (1)
apps/website/src/app/api/trpc/[trpc]/route.ts (1)
onError(26-28)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
apps/desktop/src/main/lib/db/index.ts (1)
db(18-25)apps/desktop/src/main/lib/db/schemas.ts (1)
Project(1-9)apps/desktop/src/lib/trpc/routers/projects/utils/colors/colors.ts (1)
assignRandomColor(3-7)
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WindowControls.tsx (1)
WindowControls(4-46)
apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (4)
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx (1)
StartTopBar(5-28)apps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsx (1)
ActionCard(11-39)packages/ui/src/components/tooltip.tsx (3)
Tooltip(74-74)TooltipTrigger(74-74)TooltipContent(74-74)apps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsx (1)
CloneRepoDialog(11-111)
packages/ui/src/components/tooltip.tsx (1)
packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/renderer/screens/main/index.tsx (5)
apps/desktop/src/renderer/stores/app-state.ts (2)
useCurrentView(46-46)useOpenSettings(51-51)apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(28-151)apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)apps/desktop/src/renderer/screens/main/components/AppFrame.tsx (1)
AppFrame(5-7)apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (1)
StartView(28-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (25)
packages/ui/src/components/tooltip.tsx (2)
38-72: LGTM! Arrow customization implemented correctly.The addition of
showArrowandarrowClassNameprops provides flexible tooltip arrow control:
- Default
showArrow = truemaintains backward compatibility.- Conditional rendering is correctly implemented.
- The
cnutility properly merges custom arrow classes with base styles.- Type definitions are properly extended.
19-30: Consider whether the per-tooltip TooltipProvider pattern is appropriate for this use case.The current implementation wraps each
Tooltipinstance with its ownTooltipProvider. While this deviates from the standard Radix UI pattern (where a single provider wraps multiple tooltips at the app level), the design appears intentional for a self-contained UI package component.Rationale for current implementation:
- No app-level provider exists, so each component must be self-sufficient
- Enables per-tooltip
delayDurationcustomization- Aligns with component packaging best practices (self-contained, no external setup required)
Decision point:
If this component is meant to be used in an app with an existing app-levelTooltipProvider, refactor to use that provider instead. Otherwise, the current pattern is acceptable for a packaged component.apps/desktop/src/shared/themes/built-in/monokai.ts (1)
17-17: Monokai card color tweak looks consistentThe updated
ui.cardcolor aligns with the rest of the Monokai surface palette and keeps the theme coherent.apps/desktop/src/shared/themes/built-in/one-dark.ts (1)
17-19: One Dark surface colors updated coherentlyUsing
#2c313cfor bothcardandpopovermatches the existing One Dark surface tones (tertiaryActive,sidebarAccent) and should render consistent panels/tooltips.apps/desktop/src/shared/themes/built-in/light.ts (1)
16-27: Light theme surface/accent adjustments improve contrastThe small lightness reductions for
card,popover, andaccentkeep hue/chroma consistent while likely giving better separation from the pure-white background.apps/desktop/src/shared/themes/built-in/dark.ts (1)
16-18: Dark theme panel colors better separated from backgroundRaising
cardandpopoverlightness above the background should make panels feel more defined while preserving the dark aesthetic.apps/desktop/src/shared/hotkeys.ts (1)
92-103: More explicit split hotkey labels are an improvementThe updated labels (“Split Terminal Window …”) make these shortcuts clearer in the shortcuts UI without affecting behavior.
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsx (1)
10-10: onClick wrapper prevents passing the MouseEvent into openSettingsSwitching to
onClick={() => openSettings()}avoids React passing the click event as the first argument, which is important now thatopenSettingscan take an optional section parameter.apps/desktop/src/shared/themes/built-in/index.ts (1)
3-3: LGTM!The Ember theme is correctly integrated into the built-in themes registry, following the existing pattern for imports, array inclusion, and re-exports.
Also applies to: 14-14, 32-32
apps/desktop/src/shared/themes/built-in/ember.ts (1)
1-97: LGTM!The Ember theme is well-structured with comprehensive UI and terminal color tokens, follows the established pattern from other built-in themes, and includes proper documentation describing the warm dark aesthetic.
apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsx (1)
1-10: LGTM!Clean refactor to use Zustand store-backed state for settings section management, aligning with the coding guidelines preference for Zustand over local state. The pattern correctly uses dedicated hooks from the store.
apps/desktop/src/renderer/screens/main/index.tsx (4)
23-27: LGTM!Simple, self-contained loading spinner component with appropriate Tailwind animation classes.
33-40: LGTM!Good use of tRPC query destructuring to expose the necessary state for loading, error handling, and retry functionality.
51-53: LGTM!The hotkey now correctly opens the settings view with the "keyboard" section, aligning with the updated
openSettingssignature that accepts an optional section parameter.
75-80: LGTM!Clean separation of concerns with
renderContentfunction and conditional rendering forStartViewvs. main content. The structure is readable and maintains good component composition.Also applies to: 143-157
apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (4)
37-49: LGTM!Proper error clearing before mutation and appropriate callback handling for success/error cases.
51-61: LGTM!Simple and clean handler for opening recent projects with proper error handling.
96-113: LGTM!Good error banner implementation with dismissible functionality and proper accessibility (
aria-label).
220-224: LGTM!Clean integration of the
CloneRepoDialogwith proper props for state management and error propagation.apps/desktop/src/renderer/stores/app-state.ts (3)
5-5: LGTM!Good addition of the
SettingsSectiontype to centralize section definitions. Based on learnings, this aligns with the preference for zustand state management.
26-31: LGTM!Clean implementation of optional section parameter with conditional spread. This allows
openSettings()to preserve the current section whileopenSettings("keyboard")navigates to a specific section.
47-50: LGTM!Convenience hooks follow the existing pattern in the file and provide clean access to the new settings section state.
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar.tsx (3)
2-7: LGTM!Clean import organization with proper type import for
SettingsSectionfrom the centralized store.
24-28: LGTM!Good addition of the Keyboard Shortcuts section with consistent structure matching the existing Appearance section.
31-73: LGTM!Component properly accepts and uses the
onSectionChangeprop, maintaining a clean separation between UI and state management. This aligns with the store-backed settings flow introduced in this PR.
| const handleRetry = async () => { | ||
| setIsRetrying(true); | ||
| await refetch(); | ||
| setIsRetrying(false); | ||
| }; |
There was a problem hiding this comment.
Swallowed refetch errors and potential stale isRetrying state.
If refetch() throws, the error is silently swallowed, and isRetrying remains true indefinitely. Consider using try/finally or handling the error explicitly.
const handleRetry = async () => {
setIsRetrying(true);
- await refetch();
- setIsRetrying(false);
+ try {
+ await refetch();
+ } finally {
+ setIsRetrying(false);
+ }
};🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/index.tsx around lines 101 to 105, the
current handleRetry implementation awaits refetch() without error handling so
any thrown error is swallowed and setIsRetrying(true) can remain true forever;
wrap the await refetch() in try/finally (or try/catch/finally) and call
setIsRetrying(false) in the finally block, and in catch either log/display the
error or rethrow it so callers can react.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx (3)
12-18: Guardnavigatoraccess to avoid non‑browser runtime errors
useIsMacusesnavigator.platform/navigator.userAgentdirectly; if this ever runs in a non‑browser context (e.g., SSR, certain tests) you’ll hit aReferenceErrorbefore optional chaining kicks in. Wrapping with atypeof navigator !== "undefined"check makes it safer.function useIsMac(): boolean { - return useMemo(() => { - const platform = navigator.platform?.toUpperCase() ?? ""; - const userAgent = navigator.userAgent?.toUpperCase() ?? ""; - return platform.includes("MAC") || userAgent.includes("MAC"); - }, []); + return useMemo(() => { + if (typeof navigator === "undefined") { + return false; + } + + const platform = navigator.platform?.toUpperCase() ?? ""; + const userAgent = navigator.userAgent?.toUpperCase() ?? ""; + return platform.includes("MAC") || userAgent.includes("MAC"); + }, []); }
56-74: Validate that consolidated workspace shortcut key string formats as intended
consolidateWorkspaceJumpsreplaces individual workspace entries with a single definition usingkeys: "meta+1-9". This is great for de‑duping the list, but it assumesformatKeysForDisplayknows how to render that composite string nicely (e.g.,⌘+1–9, not literally"meta+1-9").If
formatKeysForDisplaydoesn’t special‑case this, consider either:
- Using a representative key like
"meta+1"for display, and relying on the label text to communicate1–9, or- Extending
formatKeysForDisplayto recognize the"1-9"pattern and render it more readably.Also applies to: 83-85
76-147: Overall KeyboardShortcutsSettings structure looks solid and addresses prior Mac‑only hintThe main component’s flow (ordered categories → flattened/consolidated list → label‑based search → table‑like UI) is straightforward, and using
useIsMac+modifierKeycorrectly fixes the Mac‑only⌘ + ?hint from the previous review. The localuseStatefor search is appropriate and keeps state scoped to this view.Only small nit: if there are ever zero hotkeys, the “No shortcuts found matching ""” copy will look odd; you might swap in a generic “No shortcuts available” message when
searchQueryis empty.apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
1-3: Consider using async fs operations consistently.The code uses both
existsSync(line 225) andaccess(line 195) for checking path existence. Consider using the asyncaccessconsistently for better performance in the main process, though the current approach works correctly.-import { existsSync } from "node:fs"; +import { access } from "node:fs/promises";Then replace line 225:
-if (existsSync(clonePath)) { +try { + await access(clonePath); + // Directory exists + return { + canceled: false as const, + success: false as const, + error: `A folder named "${repoName}" already exists at this location. Please choose a different destination.`, + }; +} catch { + // Directory doesn't exist, proceed with clone +}
233-235: Consider adding timeout or progress feedback for large repository clones.The
git.clone()operation has no timeout and could hang indefinitely for large repositories or slow networks. Users have no visibility into clone progress.Consider adding a timeout or using simple-git's progress callback:
const git = simpleGit({ timeout: { block: 120000, // 2 minutes for any blocking operation }, }); await git.clone(input.url, clonePath, { '--progress': null, });For better UX, you could also emit progress events through tRPC subscriptions in a future iteration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/projects/projects.ts(2 hunks)apps/desktop/src/lib/trpc/routers/window.ts(2 hunks)apps/desktop/src/main/windows/main.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/StartView/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
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/window.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/lib/trpc/routers/projects/projects.ts
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/window.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/lib/trpc/routers/projects/projects.ts
**/*.{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/window.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/lib/trpc/routers/projects/projects.ts
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/window.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/windows/main.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/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.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/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/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/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsx
🧠 Learnings (5)
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/lib/**/*.ts : Never import Node.js modules in shared code like `src/lib/electron-router-dom.ts` - this code runs in both main and renderer processes
Applied to files:
apps/desktop/src/lib/trpc/routers/window.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/window.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/src/lib/trpc/routers/window.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
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/StartView/index.tsx
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/window.ts (1)
apps/desktop/src/lib/trpc/index.ts (1)
publicProcedure(16-16)
apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (4)
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx (1)
StartTopBar(5-29)apps/desktop/src/renderer/screens/main/components/StartView/ActionCard.tsx (1)
ActionCard(11-39)packages/ui/src/components/tooltip.tsx (1)
Tooltip(74-74)apps/desktop/src/renderer/screens/main/components/StartView/CloneRepoDialog.tsx (1)
CloneRepoDialog(11-111)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
apps/desktop/src/main/lib/db/index.ts (1)
db(18-25)apps/desktop/src/main/lib/db/schemas.ts (1)
Project(1-9)apps/desktop/src/lib/trpc/routers/projects/utils/colors/colors.ts (1)
assignRandomColor(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (12)
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx (1)
28-51: HotkeyRow implementation is clear and idiomaticThe row component cleanly separates display concerns, uses
formatKeysForDisplayappropriately, and keeps the layout simple. No issues from a correctness or React‑usage perspective.apps/desktop/src/main/windows/main.ts (1)
23-24: LGTM!Adding minimum window size constraints (400x400) is a good UX improvement that prevents the window from being resized to an unusable state.
apps/desktop/src/lib/trpc/routers/window.ts (2)
37-40: LGTM!Clean and minimal implementation of the
getHomeDirendpoint. Consistent with the other query endpoints in this router.
1-1: This import is correct and consistent with the established pattern.All tRPC routers in
src/lib/trpc/routers/are main-process-only server-side code. They're instantiated exclusively insrc/main/windows/main.tsand passed to the tRPC IPC handler. Node.js module imports in this directory are appropriate and match the pattern used across other routers (projects, workspaces, external, etc.).apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx (2)
5-8: LGTM! Loading state handling addresses the flicker issue.The platform detection now properly handles the loading state, preventing the brief UI flicker on macOS that was flagged in a previous review. The logic correctly waits for the query to resolve before determining which controls to show.
10-27: LGTM!The three-section layout is well-structured for a draggable title bar with appropriate padding for macOS traffic lights. The
no-dragclass on interactive elements is correctly applied.apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
16-81: LGTM! Robust URL parsing that addresses previous review concerns.The
extractRepoNamefunction properly handles:
- HTTP/HTTPS URLs with trailing slashes, query strings, and hashes
- SSH-style URLs (
git@github.com:user/repo.git)- Percent-encoded characters
- Safe filename validation
This implementation is significantly more robust than the previous version flagged in past reviews.
192-222: LGTM! Stale project handling addresses past review concern.The implementation correctly verifies that the directory still exists before returning an existing project, and removes stale records when the directory has been deleted.
apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (4)
1-10: LGTM!Imports are well-organized and correctly use alias paths as per coding guidelines. No Node.js modules in renderer code.
12-50: LGTM! Cross-platform path formatting addresses past review concern.The
formatPathfunction now properly handles both Unix and Windows paths by:
- Normalizing path separators to forward slashes
- Using the actual home directory from
trpc.window.getHomeDir- Falling back to Unix pattern matching when home dir is unavailable
This addresses the previous review concern about Windows paths not being simplified.
52-93: LGTM!State management is appropriate with local React state for UI concerns and tRPC queries/mutations for data. Error handling is consistently applied in both handlers.
95-252: LGTM!Well-structured UI with:
- Proper accessibility attributes (aria-label, SVG title)
- User-friendly error display with dismiss capability
- Tooltips for full path visibility on hover
- Pagination for handling large project lists
- Consistent loading state handling
| cloneRepo: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| url: z.string().url(), | ||
| // Trim and convert empty/whitespace strings to undefined | ||
| targetDirectory: z | ||
| .string() | ||
| .trim() | ||
| .optional() | ||
| .transform((v) => (v && v.length > 0 ? v : undefined)), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
SSH-style URLs will fail Zod's .url() validation.
The z.string().url() validator only accepts URLs with valid protocols (http, https, etc.). SSH-style URLs like git@github.com:user/repo.git will fail validation before reaching extractRepoName, even though that function is designed to handle them.
Consider using a custom validation or a more permissive schema:
.input(
z.object({
- url: z.string().url(),
+ url: z.string().min(1, "Repository URL is required"),
// Trim and convert empty/whitespace strings to undefined
targetDirectory: z
.string()
.trim()
.optional()
.transform((v) => (v && v.length > 0 ? v : undefined)),
}),
)Alternatively, add a custom refinement that validates both URL formats:
url: z.string().refine(
(val) => {
// Accept standard URLs
try { new URL(val); return true; } catch {}
// Accept SSH-style URLs (git@host:path)
return /^[\w.-]+@[\w.-]+:.+/.test(val);
},
"Invalid repository URL"
),🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/projects/projects.ts around lines 146 to
157, the input schema uses z.string().url() which rejects SSH-style repo strings
like "git@github.com:user/repo.git"; replace that validator with a
z.string().refine that accepts either a valid URL (using new URL(...) in a
try/catch) or an SSH-style pattern (e.g. /^[\w.-]+@[\w.-]+:.+/), and provide a
clear error message like "Invalid repository URL"; keep the existing
.trim().optional().transform(...) behavior for targetDirectory.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.