Conversation
- Add SettingsService to handle reading/writing global and project settings. - Introduce API routes for managing settings, including global settings, credentials, and project-specific settings. - Implement migration functionality to transfer settings from localStorage to file-based storage. - Create common utilities for settings routes and integrate logging for error handling. - Update server entry point to include new settings routes.
- Consolidate destructuring of useSetupStore into a single line for cleaner code. - Remove unnecessary blank line at the beginning of the file.
- Update the link in the README for the Agentic Jumpstart course to include a GitHub-specific query parameter. - Ensure consistent userData path across development and production environments in the Electron app, with error handling for path setting. - Improve the isElectron function to check for Electron context more robustly.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds file-based settings persistence and migration: a new SettingsService, API routes under /api/settings, UI migration/sync hooks, a splash screen and theme selection step, plus many new theme CSS files and related client/server wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant FS
User->>UI: Open app
UI->>API: GET /api/settings/status
API->>FS: Check settings files
FS-->>API: files/status
API-->>UI: { needsMigration, hasGlobalSettings, hasCredentials, dataDir }
alt needsMigration
UI->>UI: Collect localStorage keys
UI->>API: POST /api/settings/migrate (data)
API->>API: validate & parse migration payload
API->>FS: write global settings, credentials, project files (atomic writes)
FS-->>API: write results
API-->>UI: migration result (migratedGlobalSettings, migratedCredentials, migratedProjectCount, errors)
UI->>UI: clear migrated localStorage keys (keep necessary)
end
UI->>UI: Show SplashScreen / Setup (Theme selection)
UI->>API: PUT /api/settings/global (on theme select)
API->>FS: update global settings file (atomic)
FS-->>API: updated settings
API-->>UI: { success, settings }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the initial user experience by integrating a theme selection step into the onboarding flow. It also includes a major architectural improvement by migrating user settings from ephemeral browser localStorage to persistent file-based storage on the server, ensuring data integrity and better management. A new splash screen adds a professional touch to the application's startup. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-architected feature: a file-based settings persistence system, replacing the previous localStorage-only approach. This includes a robust migration path for existing users. The changes are extensive, touching both the backend server to add new API endpoints and services, and the frontend to handle migration, settings synchronization, and UI updates.
Key improvements include:
- Persistent Settings: Global, project, and credential settings are now stored in JSON files, ensuring they persist across sessions and app updates.
- Data Migration: A migration flow is implemented to seamlessly move user settings from
localStorageto the new file-based system. - Enhanced Onboarding: The setup flow is improved with a new theme picker, providing a better user experience from the start.
- Code Refactoring: The CSS for themes has been modularized into separate files, improving maintainability.
The implementation is solid, with good practices like atomic file writes on the server to prevent data corruption. My review includes a few suggestions to improve the maintainability of the settings synchronization logic on the frontend. Overall, this is an excellent contribution that greatly improves the application's robustness and user experience.
| const parsed = JSON.parse(localStorageData["automaker-storage"]); | ||
| appState = parsed.state || parsed; | ||
| } catch (e) { | ||
| errors.push(`Failed to parse automaker-storage: ${e}`); |
There was a problem hiding this comment.
For better error diagnostics during migration, it's a good practice to log the error message specifically, rather than the whole error object which can be verbose and less clear when stringified. This will make debugging migration issues easier.
| errors.push(`Failed to parse automaker-storage: ${e}`); | |
| errors.push(`Failed to parse automaker-storage: ${(e as Error).message}`); |
| const updates = { | ||
| theme: state.theme, | ||
| sidebarOpen: state.sidebarOpen, | ||
| chatHistoryOpen: state.chatHistoryOpen, | ||
| kanbanCardDetailLevel: state.kanbanCardDetailLevel, | ||
| maxConcurrency: state.maxConcurrency, | ||
| defaultSkipTests: state.defaultSkipTests, | ||
| enableDependencyBlocking: state.enableDependencyBlocking, | ||
| useWorktrees: state.useWorktrees, | ||
| showProfilesOnly: state.showProfilesOnly, | ||
| defaultPlanningMode: state.defaultPlanningMode, | ||
| defaultRequirePlanApproval: state.defaultRequirePlanApproval, | ||
| defaultAIProfileId: state.defaultAIProfileId, | ||
| muteDoneSound: state.muteDoneSound, | ||
| enhancementModel: state.enhancementModel, | ||
| keyboardShortcuts: state.keyboardShortcuts, | ||
| aiProfiles: state.aiProfiles, | ||
| projects: state.projects, | ||
| trashedProjects: state.trashedProjects, | ||
| projectHistory: state.projectHistory, | ||
| projectHistoryIndex: state.projectHistoryIndex, | ||
| lastSelectedSessionByProject: state.lastSelectedSessionByProject, | ||
| }; |
There was a problem hiding this comment.
The updates object is constructed by manually listing properties from the state. This approach is prone to errors and difficult to maintain, as any new setting added to the global state must also be manually added here to be synced. Consider refactoring the Zustand store to have a dedicated globalSettings slice that mirrors the GlobalSettings type from the backend. This would allow you to sync the entire slice without manually listing properties, making the code more robust and easier to maintain.
| const importantSettingsChanged = | ||
| state.theme !== previousState.theme || | ||
| state.projects !== previousState.projects || | ||
| state.trashedProjects !== previousState.trashedProjects || | ||
| state.keyboardShortcuts !== previousState.keyboardShortcuts || | ||
| state.aiProfiles !== previousState.aiProfiles || | ||
| state.maxConcurrency !== previousState.maxConcurrency || | ||
| state.defaultSkipTests !== previousState.defaultSkipTests || | ||
| state.enableDependencyBlocking !== previousState.enableDependencyBlocking || | ||
| state.useWorktrees !== previousState.useWorktrees || | ||
| state.showProfilesOnly !== previousState.showProfilesOnly || | ||
| state.muteDoneSound !== previousState.muteDoneSound || | ||
| state.enhancementModel !== previousState.enhancementModel || | ||
| state.defaultPlanningMode !== previousState.defaultPlanningMode || | ||
| state.defaultRequirePlanApproval !== previousState.defaultRequirePlanApproval || | ||
| state.defaultAIProfileId !== previousState.defaultAIProfileId; |
There was a problem hiding this comment.
The current method of checking for changes in important settings involves a long chain of manual comparisons. This is fragile and can easily become outdated as new settings are added or state is refactored. A more maintainable approach would be to either group these settings into a single settings object within the store for simpler deep comparison, or use a utility function for deep object comparison to detect changes automatically.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
apps/ui/src/styles/themes/cream.css (1)
1-116: LGTM!Well-structured theme with consistent use of the oklch color space for perceptual uniformity. All required theme variables are defined with appropriate warm earth tones.
Note: The scrollbar styling uses WebKit-specific pseudo-elements. Firefox users won't see styled scrollbars. Consider adding Firefox scrollbar properties if cross-browser consistency is desired:
.cream { scrollbar-color: oklch(0.7 0.03 60) oklch(0.9 0.015 70); scrollbar-width: thin; }apps/ui/src/components/views/setup-view/steps/theme-step.tsx (1)
44-69: Consider accessibility improvements for screen readers.The theme selection buttons work well visually but could benefit from accessibility enhancements.
🔎 Suggested improvements
<button key={option.value} data-testid={option.testId} + aria-label={`Select ${option.label} theme${isSelected ? ' (currently selected)' : ''}`} + aria-pressed={isSelected} onMouseEnter={() => handleThemeHover(option.value)} onMouseLeave={handleThemeLeave} onClick={() => handleThemeClick(option.value)}apps/ui/src/App.tsx (1)
18-22: Move migration logging into useEffect.The migration success logging runs on every render when
migratedis true. This should be wrapped in auseEffectto log only once when migration completes.🔎 Proposed fix
// Run settings migration on startup (localStorage -> file storage) const migrationState = useSettingsMigration(); - if (migrationState.migrated) { - console.log("[App] Settings migrated to file storage"); - } + + useEffect(() => { + if (migrationState.migrated) { + console.log("[App] Settings migrated to file storage"); + } + }, [migrationState.migrated]);apps/ui/src/styles/themes/monokai.css (1)
66-69: Consider adding status color variables for consistency.Other themes in this PR (sunset.css, gray.css) define status color variables (
--status-success,--status-warning,--status-error,--status-info,--status-backlog,--status-in-progress,--status-waiting). Adding these to the Monokai theme would ensure consistent behavior across all themes.apps/server/src/routes/settings/routes/update-global.ts (1)
10-34: Consider adding schema validation for settings updates.The handler performs basic object type checking but relies on type assertions without runtime validation. While the SettingsService may handle this, adding explicit schema validation (e.g., using Zod or similar) would provide stronger guarantees against malformed or malicious input.
Example with Zod:
import { z } from 'zod'; const GlobalSettingsUpdateSchema = z.object({ theme: z.enum(['light', 'dark', 'system']).optional(), maxConcurrency: z.number().int().min(1).max(10).optional(), // ... other fields }).partial(); // In handler: const parseResult = GlobalSettingsUpdateSchema.safeParse(req.body); if (!parseResult.success) { return res.status(400).json({ success: false, error: 'Invalid settings format', details: parseResult.error.issues }); } const updates = parseResult.data;apps/ui/src/components/splash-screen.tsx (3)
45-59: Consider accessibility: respect user's motion preferences.The splash screen uses intensive animations but doesn't check for
prefers-reduced-motion. Users with vestibular disorders or motion sensitivity should have animations disabled.🔎 Proposed enhancement
export function SplashScreen({ onComplete }: { onComplete: () => void }) { + const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; + const [phase, setPhase] = useState<"enter" | "hold" | "exit" | "done">( "enter" ); const particles = useMemo(() => generateParticles(50), []); useEffect(() => { + // Skip animations if user prefers reduced motion + if (prefersReducedMotion) { + const timer = setTimeout(() => { + setPhase("done"); + onComplete(); + }, 100); + return () => clearTimeout(timer); + } + const timers: NodeJS.Timeout[] = []; // ... rest of animation logic
3-6: Extract timing constants to a config object.While the constants are defined at the module level, grouping them into a configuration object would improve maintainability and make it easier to adjust the animation timing as a cohesive unit.
🔎 Proposed refactor
-const TOTAL_DURATION = 2300; // Total animation duration in ms (tightened from 4000) -const LOGO_ENTER_DURATION = 500; // Tightened from 1200 -const PARTICLES_ENTER_DELAY = 100; // Tightened from 400 -const EXIT_START = 1800; // Adjusted for shorter duration +const ANIMATION_TIMING = { + TOTAL_DURATION: 2300, + LOGO_ENTER_DURATION: 500, + PARTICLES_ENTER_DELAY: 100, + EXIT_START: 1800, +} as const; // Usage: -timers.push(setTimeout(() => setPhase("hold"), LOGO_ENTER_DURATION)); +timers.push(setTimeout(() => setPhase("hold"), ANIMATION_TIMING.LOGO_ENTER_DURATION));
75-88: Consider extracting inline styles to a separate CSS file.The inline
<style>tag with keyframes works, but extracting these animations to a dedicated CSS module or the global styles would improve separation of concerns and enable better CSS tooling support.apps/ui/src/hooks/use-settings-migration.ts (1)
55-151: Consider centralizing error handling for the migration flow.The migration logic is solid overall. A few observations:
- Line 137:
result.errors.join(", ")assumeserrorsis always an array. If the server returnsundefinedornull, this will throw.- The hook silently sets state on error but doesn't provide a way for callers to retry the migration.
🔎 Suggested defensive check
} else { console.warn( "[Settings Migration] Migration had errors:", result.errors ); setState({ checked: true, migrated: false, - error: result.errors.join(", "), + error: Array.isArray(result.errors) ? result.errors.join(", ") : "Migration failed", }); }apps/ui/src/styles/themes/dark.css (1)
94-112: Minor: Inconsistent indentation and consider reducing!importantusage.Line 96-98 has extra indentation compared to other rules. The
!importantdeclarations on lines 105-111 may indicate specificity conflicts that could be resolved by adjusting selector specificity instead.🔎 Indentation fix
/* Theme-specific overrides */ - .dark .content-bg { +.dark .content-bg { background: linear-gradient(135deg, oklch(0.04 0 0), oklch(0.08 0 0), oklch(0.04 0 0)); - } +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
README.md(1 hunks)apps/server/src/index.ts(3 hunks)apps/server/src/lib/automaker-paths.ts(1 hunks)apps/server/src/routes/settings/common.ts(1 hunks)apps/server/src/routes/settings/index.ts(1 hunks)apps/server/src/routes/settings/routes/get-credentials.ts(1 hunks)apps/server/src/routes/settings/routes/get-global.ts(1 hunks)apps/server/src/routes/settings/routes/get-project.ts(1 hunks)apps/server/src/routes/settings/routes/migrate.ts(1 hunks)apps/server/src/routes/settings/routes/status.ts(1 hunks)apps/server/src/routes/settings/routes/update-credentials.ts(1 hunks)apps/server/src/routes/settings/routes/update-global.ts(1 hunks)apps/server/src/routes/settings/routes/update-project.ts(1 hunks)apps/server/src/services/settings-service.ts(1 hunks)apps/server/src/types/settings.ts(1 hunks)apps/ui/src/App.tsx(1 hunks)apps/ui/src/components/splash-screen.tsx(1 hunks)apps/ui/src/components/views/setup-view.tsx(5 hunks)apps/ui/src/components/views/setup-view/steps/index.ts(1 hunks)apps/ui/src/components/views/setup-view/steps/theme-step.tsx(1 hunks)apps/ui/src/hooks/use-settings-migration.ts(1 hunks)apps/ui/src/lib/electron.ts(1 hunks)apps/ui/src/lib/http-api-client.ts(1 hunks)apps/ui/src/main.ts(1 hunks)apps/ui/src/routes/__root.tsx(3 hunks)apps/ui/src/store/app-store.ts(1 hunks)apps/ui/src/store/setup-store.ts(1 hunks)apps/ui/src/styles/global.css(0 hunks)apps/ui/src/styles/theme-imports.ts(1 hunks)apps/ui/src/styles/themes/catppuccin.css(1 hunks)apps/ui/src/styles/themes/cream.css(1 hunks)apps/ui/src/styles/themes/dark.css(1 hunks)apps/ui/src/styles/themes/dracula.css(1 hunks)apps/ui/src/styles/themes/gray.css(1 hunks)apps/ui/src/styles/themes/gruvbox.css(1 hunks)apps/ui/src/styles/themes/light.css(1 hunks)apps/ui/src/styles/themes/monokai.css(1 hunks)apps/ui/src/styles/themes/nord.css(1 hunks)apps/ui/src/styles/themes/onedark.css(1 hunks)apps/ui/src/styles/themes/red.css(1 hunks)apps/ui/src/styles/themes/retro.css(1 hunks)apps/ui/src/styles/themes/solarized.css(1 hunks)apps/ui/src/styles/themes/sunset.css(1 hunks)apps/ui/src/styles/themes/synthwave.css(1 hunks)apps/ui/src/styles/themes/tokyonight.css(1 hunks)
💤 Files with no reviewable changes (1)
- apps/ui/src/styles/global.css
🧰 Additional context used
🧬 Code graph analysis (15)
apps/server/src/routes/settings/index.ts (8)
apps/server/src/services/settings-service.ts (1)
SettingsService(91-542)apps/server/src/routes/settings/routes/status.ts (1)
createStatusHandler(10-28)apps/server/src/routes/settings/routes/get-global.ts (1)
createGetGlobalHandler(9-23)apps/server/src/routes/settings/routes/update-global.ts (1)
createUpdateGlobalHandler(10-34)apps/server/src/routes/settings/routes/get-credentials.ts (1)
createGetCredentialsHandler(9-23)apps/server/src/routes/settings/routes/update-credentials.ts (1)
createUpdateCredentialsHandler(10-39)apps/server/src/routes/settings/routes/get-project.ts (1)
createGetProjectHandler(10-34)apps/server/src/routes/settings/routes/migrate.ts (1)
createMigrateHandler(9-54)
apps/server/src/routes/settings/routes/update-project.ts (3)
apps/server/src/types/settings.ts (1)
ProjectSettings(184-200)apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/settings/routes/get-credentials.ts (2)
apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/settings/routes/update-credentials.ts (3)
apps/server/src/types/settings.ts (1)
Credentials(146-153)apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/ui/src/App.tsx (3)
apps/ui/src/hooks/use-settings-migration.ts (1)
useSettingsMigration(47-154)apps/ui/src/utils/router.ts (1)
router(11-15)apps/ui/src/components/splash-screen.tsx (1)
SplashScreen(38-309)
apps/server/src/routes/settings/routes/update-global.ts (3)
apps/server/src/types/settings.ts (1)
GlobalSettings(95-141)apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/settings/routes/status.ts (3)
apps/server/src/services/settings-service.ts (2)
hasGlobalSettings(156-159)hasCredentials(249-252)apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/settings/routes/get-project.ts (2)
apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/types/settings.ts (1)
apps/ui/src/store/app-store.ts (9)
ThemeMode(19-36)KanbanCardDetailLevel(38-38)AgentModel(258-258)PlanningMode(267-267)ThinkingLevel(264-264)ModelProvider(261-261)KeyboardShortcuts(151-179)AIProfile(270-279)DEFAULT_KEYBOARD_SHORTCUTS(182-213)
apps/ui/src/routes/__root.tsx (2)
apps/ui/src/store/setup-store.ts (1)
useSetupStore(125-183)apps/ui/tests/utils/project/setup.ts (1)
setupComplete(620-635)
apps/ui/src/components/views/setup-view.tsx (2)
apps/ui/src/store/setup-store.ts (1)
useSetupStore(125-183)apps/ui/src/components/views/setup-view/steps/theme-step.tsx (1)
ThemeStep(12-90)
apps/server/src/routes/settings/routes/get-global.ts (3)
apps/server/src/services/settings-service.ts (1)
SettingsService(91-542)apps/server/src/routes/settings/common.ts (1)
logError(15-15)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/ui/src/store/app-store.ts (1)
apps/ui/src/hooks/use-settings-migration.ts (2)
syncSettingsToServer(160-205)syncProjectSettingsToServer(232-261)
apps/ui/src/hooks/use-settings-migration.ts (2)
apps/ui/src/lib/electron.ts (1)
isElectron(510-520)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1023-1028)
apps/server/src/services/settings-service.ts (2)
apps/server/src/types/settings.ts (15)
GlobalSettings(95-141)DEFAULT_GLOBAL_SETTINGS(226-252)SETTINGS_VERSION(267-267)Credentials(146-153)DEFAULT_CREDENTIALS(254-261)CREDENTIALS_VERSION(268-268)ProjectSettings(184-200)DEFAULT_PROJECT_SETTINGS(263-265)PROJECT_SETTINGS_VERSION(269-269)KeyboardShortcuts(32-53)AIProfile(56-65)ProjectRef(68-74)TrashedProjectRef(77-80)BoardBackgroundSettings(158-168)WorktreeInfo(173-179)apps/server/src/lib/automaker-paths.ts (5)
getGlobalSettingsPath(102-104)ensureDataDir(123-126)getCredentialsPath(109-111)getProjectSettingsPath(116-118)ensureAutomakerDir(87-91)
⏰ 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: e2e
🔇 Additional comments (45)
README.md (1)
11-11: LGTM!Adding UTM tracking parameter for analytics is a standard practice for tracking referral sources.
apps/ui/src/lib/electron.ts (1)
510-520: LGTM!The enhanced
isElectron()detection is more robust:
- SSR-safe with
typeof window === "undefined"check- Multiple detection paths for reliability
- Optional chaining prevents runtime errors
apps/ui/src/components/views/setup-view/steps/theme-step.tsx (1)
12-89: Well-implemented theme selection step.The component correctly:
- Integrates with the app store for theme state management
- Implements hover preview with proper cleanup
- Uses responsive grid layout
- Shows visual selection feedback
Note: The hover preview won't work on touch devices, but tap-to-select functions correctly, which is acceptable UX.
apps/server/src/lib/automaker-paths.ts (1)
92-126: LGTM!Clean additions that follow the existing patterns in this file:
- Consistent naming with other path utilities
- Good separation of credentials from general settings for security
ensureDataDirusesrecursive: truefor safe idempotent directory creationapps/ui/src/styles/themes/retro.css (1)
134-137: Theretro-glowanimation is already defined inapps/ui/src/styles/global.cssat line 657 and is available to this theme file. No issue here.Likely an incorrect or invalid review comment.
apps/ui/src/components/views/setup-view/steps/index.ts (1)
3-3: LGTM! Clean barrel export addition.The ThemeStep export is properly added alongside existing setup step exports.
apps/ui/src/store/setup-store.ts (1)
54-60: LGTM! Proper type extension.The "theme" step is correctly added to the SetupStep union, positioned logically in the setup flow sequence.
apps/ui/src/main.ts (1)
307-319: LGTM! Robust path normalization.The userData path initialization is well-implemented with:
- Conditional update (only if path differs)
- Try/catch for error handling
- Informative logging
apps/ui/src/styles/theme-imports.ts (1)
1-22: LGTM! Effective tree-shaking prevention.This aggregation module correctly ensures all theme CSS files are bundled for dynamic loading.
apps/ui/src/App.tsx (2)
10-16: LGTM! Clean session-based splash gating.The sessionStorage approach correctly ensures the splash screen displays only once per session.
24-27: LGTM! Proper callback memoization.The
handleSplashCompletecallback is correctly memoized withuseCallbackand manages both sessionStorage persistence and state updates.apps/ui/src/routes/__root.tsx (2)
69-96: LGTM! Robust hydration-aware routing.The implementation correctly:
- Waits for setup store hydration before enforcing routing rules
- Subscribes to
onFinishHydrationcallback when needed- Properly manages subscription cleanup
- Prevents race conditions during store rehydration
The routing logic is sound and the dependencies are correct.
24-26: LGTM! Safe hydration state initialization.The optional chaining ensures safe access to the persist API, with proper fallback to
false.apps/server/src/routes/settings/routes/update-project.ts (1)
10-48: LGTM! Solid route handler implementation.The handler properly:
- Validates required parameters with appropriate error responses
- Uses the settings service to perform updates
- Implements consistent error handling and logging
- Returns well-structured responses
apps/ui/src/styles/themes/red.css (1)
1-69: Theme looks good overall.The Red theme defines a comprehensive set of CSS variables using consistent OKLCh color values. The color palette is cohesive with the crimson/red aesthetic.
For full parity with other themes (sunset.css, gray.css), consider adding status color variables and scrollbar/component overrides in a follow-up.
apps/ui/src/styles/themes/sunset.css (1)
1-111: LGTM!The Sunset theme is well-structured with a complete set of CSS variables including status colors and scrollbar overrides. This serves as a good reference implementation for other themes.
apps/server/src/routes/settings/routes/get-global.ts (1)
9-23: LGTM!The handler follows a clean factory pattern with proper dependency injection, making it testable. Error handling is consistent with the shared utilities, and the async/await pattern is correctly implemented.
apps/server/src/routes/settings/common.ts (1)
1-15: LGTM!Clean utility module that centralizes logging and error handling for settings routes. The scoped logger ("Settings") will aid in log filtering and debugging.
apps/ui/src/styles/themes/gray.css (1)
1-110: LGTM!The Gray theme is comprehensive with a complete variable set including status colors and scrollbar overrides. The low-chroma OKLCh values create a clean, minimal aesthetic appropriate for the theme.
apps/ui/src/styles/themes/light.css (1)
63-102: XML syntax highlighting rules look good.The light theme XML highlighting uses appropriate contrast levels for readability on light backgrounds.
apps/server/src/routes/settings/routes/status.ts (1)
10-28: LGTM! Well-structured status endpoint.The handler correctly computes migration status and follows consistent error handling patterns. The logic is clean and the response structure aligns with other settings routes.
apps/server/src/routes/settings/routes/get-credentials.ts (1)
9-23: LGTM! Secure credential retrieval.The handler correctly fetches masked credentials for safe UI display, preventing exposure of sensitive API keys. Error handling and response structure are consistent with the API conventions.
apps/server/src/routes/settings/routes/get-project.ts (1)
10-34: LGTM! Proper validation and error handling.The handler correctly validates the required
projectPathparameter and follows consistent patterns. The use of POST for this endpoint is well-justified by the comment regarding special characters in paths.apps/server/src/index.ts (1)
40-40: LGTM! Clean integration of settings service.The SettingsService is properly instantiated and wired into the Express app. The settings API is correctly mounted after authentication middleware, ensuring protected access to configuration endpoints.
Also applies to: 45-45, 113-113, 143-143
apps/server/src/routes/settings/routes/update-credentials.ts (1)
10-39: LGTM! Good security practices.The handler properly validates input, updates credentials through the service layer, and returns only masked credentials for confirmation. This prevents accidental exposure of sensitive API keys.
apps/ui/src/store/app-store.ts (1)
2476-2516: Consider deep equality checks for object references.The change detection uses reference comparison (
!==) for objects likeprojects,trashedProjects,keyboardShortcuts, andaiProfiles. Since these are managed by Zustand with immutable updates, this should work correctly. However, if any action accidentally mutates these objects in place, syncs would be missed.This is acceptable given Zustand's immutable patterns, but consider documenting this assumption or adding runtime checks in development mode.
Additionally, verify that all store actions that modify these objects follow immutable update patterns. For example, ensure
setKeyboardShortcuts,addAIProfile, etc. create new object references rather than mutating existing ones.apps/server/src/routes/settings/routes/migrate.ts (1)
9-54: LGTM! Well-structured migration handler.The handler properly validates input, provides detailed logging, and returns comprehensive migration results including success status, migration counts, and any errors encountered. Good separation of concerns with the migration logic delegated to the SettingsService.
apps/ui/src/components/views/setup-view.tsx (2)
18-27: LGTM! Theme step properly integrated.The theme step is well-integrated into the setup flow between "welcome" and "claude" steps. The step name mapping logic correctly handles all transitions.
119-124: LGTM! Theme step rendering implemented correctly.The ThemeStep component is properly rendered with appropriate
onNextandonBackhandlers that maintain the flow consistency.apps/ui/src/styles/themes/synthwave.css (1)
75-78: Thesynthwave-glowanimation is properly defined inapps/ui/src/styles/global.cssat line 703 and will be applied correctly when both stylesheets are loaded. No action needed.Likely an incorrect or invalid review comment.
apps/ui/src/hooks/use-settings-migration.ts (2)
1-52: LGTM on hook structure and constants.The hook documentation is clear, the ref-based guard against double execution is correct for React StrictMode, and the localStorage key constants are well-organized.
232-261: LGTM on project settings sync utility.The function correctly guards for Electron mode and handles errors gracefully.
apps/ui/src/styles/themes/dark.css (2)
1-92: LGTM on the core theme variables.The comprehensive set of CSS custom properties using oklch color space is well-structured. The semantic grouping (backgrounds, foregrounds, brand colors, status indicators, etc.) makes the theme maintainable.
127-166: LGTM on XML syntax highlighting tokens.The syntax highlighting color choices provide good contrast and follow common conventions for code editors.
apps/server/src/services/settings-service.ts (4)
45-61: Atomic write implementation looks correct.The temp-file-then-rename pattern ensures atomicity. The cleanup in the catch block is appropriate.
One minor consideration:
Date.now()could theoretically collide under extreme concurrency. If this becomes an issue, consider adding a random suffix.
66-77: Non-ENOENT errors are swallowed silently.When a file exists but contains invalid JSON (corruption), this returns the default value and only logs an error. The caller has no way to know the file was corrupted. Consider whether this is the desired behavior or if corruption should propagate differently.
102-121: LGTM on getGlobalSettings with backwards compatibility.The spread pattern correctly applies defaults while preserving existing values, including nested
keyboardShortcuts.
477-510: LGTM on per-project migration loop.The error handling per-project is good - it collects errors without failing the entire migration. The approach of aggregating project paths from multiple sources before iterating is clean.
apps/server/src/types/settings.ts (4)
31-65: LGTM on interface definitions.The
KeyboardShortcutsandAIProfileinterfaces align well with the UI counterparts shown in the relevant code snippets.
92-141: LGTM on GlobalSettings interface.The interface is comprehensive and well-documented with inline comments explaining each section.
202-269: LGTM on default values and version constants.The defaults are sensible, and versioning the settings formats will help with future migrations.
5-29: No issues found. TheAgentModeltype is properly aligned between server and UI, both defining identical values:"opus" | "sonnet" | "haiku".apps/ui/src/lib/http-api-client.ts (3)
840-888: LGTM on settings status and global settings API.The API structure follows the established patterns in this client and provides appropriate type annotations for the response shapes.
913-951: Using POST forgetProjectis unconventional but acceptable.Line 942 uses POST to fetch project settings, likely to avoid URL-encoding the
projectPath. This is a reasonable trade-off, though documenting this deviation from REST conventions would help future maintainers.
953-967: No action required—client and server payload structure are correctly aligned.The server handler explicitly expects the payload wrapped as
{ data: ... }(line 12 of migrate.ts), and the client sends exactly that structure. The migration will work as intended.Likely an incorrect or invalid review comment.
| // Project settings | ||
| router.post("/project", createGetProjectHandler(settingsService)); | ||
| router.put("/project", createUpdateProjectHandler(settingsService)); |
There was a problem hiding this comment.
Use GET method for retrieving project settings.
Line 31 uses POST for retrieving project settings, which violates REST conventions. GET requests should be used for read operations.
Consider either:
- Change to
router.get("/project")and acceptprojectPathas a query parameter - Use a path parameter:
router.get("/project/:projectPath")(requires URL encoding for paths)
🔎 Recommended refactor
Option 1 - Query parameter approach:
// Project settings
- router.post("/project", createGetProjectHandler(settingsService));
+ router.get("/project", createGetProjectHandler(settingsService));
router.put("/project", createUpdateProjectHandler(settingsService));Then update the handler in apps/server/src/routes/settings/routes/get-project.ts to read from query params:
const { projectPath } = req.query as { projectPath?: string };Committable suggestion skipped: line range outside the PR's diff.
| async getProjectSettings(projectPath: string): Promise<ProjectSettings> { | ||
| const settingsPath = getProjectSettingsPath(projectPath); | ||
| const settings = await readJsonFile<ProjectSettings>( | ||
| settingsPath, | ||
| DEFAULT_PROJECT_SETTINGS | ||
| ); | ||
|
|
||
| return { | ||
| ...DEFAULT_PROJECT_SETTINGS, | ||
| ...settings, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the getProjectSettingsPath function implementation
rg -n "getProjectSettingsPath" --type ts -A 10 -B 2
# Also check for path validation utilities
rg -n "path.*traversal|normalize|resolve" --type ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 7076
🏁 Script executed:
#!/bin/bash
# Find the getAutomakerDir function
rg -n "function getAutomakerDir|export function getAutomakerDir" --type ts -A 5
# Check for any path validation in automaker-paths.ts
rg -n "resolve|normalize|traversal" apps/server/src/lib/automaker-paths.tsRepository: AutoMaker-Org/automaker
Length of output: 479
🏁 Script executed:
#!/bin/bash
# Find callers of getProjectSettings
rg -n "\.getProjectSettings|getProjectSettings\(" --type ts -B 2 -A 2
# Also check if there's any validation on projectPath in the settings service
rg -n "projectPath" apps/server/src/services/settings-service.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 2547
🏁 Script executed:
#!/bin/bash
# Examine the get-project.ts route file
cat -n apps/server/src/routes/settings/routes/get-project.tsRepository: AutoMaker-Org/automaker
Length of output: 1351
🏁 Script executed:
#!/bin/bash
# Find readJsonFile implementation
rg -n "function readJsonFile|export.*readJsonFile" --type ts -A 10
# Find ensureAutomakerDir implementation
rg -n "export.*ensureAutomakerDir" --type ts -A 10Repository: AutoMaker-Org/automaker
Length of output: 1891
🏁 Script executed:
#!/bin/bash
# Look for any validation middleware or path validation utilities
rg -n "projectPath|validatePath|sanitize" apps/server/src/routes/settings/ --type ts
# Check if there's a validation layer or middleware
rg -n "middleware|validator|validate" apps/server/src/routes/ --type ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 3190
🏁 Script executed:
#!/bin/bash
# Examine the validatePath function in security.js
rg -n "validatePath" apps/server/src/lib/security.ts -A 20Repository: AutoMaker-Org/automaker
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Check more context around validatePath and allowedPaths
cat -n apps/server/src/lib/security.ts | head -70Repository: AutoMaker-Org/automaker
Length of output: 2015
Add path traversal validation for projectPath parameter.
The projectPath parameter from user input lacks validation for path traversal patterns. Apply path validation similar to the fs routes that use validatePath, or at minimum verify that the resolved path remains within the intended project directory using path.resolve() combined with a containment check (e.g., startsWith(baseDir)).
async getProjectSettings(projectPath: string): Promise<ProjectSettings> {
const settingsPath = getProjectSettingsPath(projectPath);
const settings = await readJsonFile<ProjectSettings>(
settingsPath,
DEFAULT_PROJECT_SETTINGS
);
return {
...DEFAULT_PROJECT_SETTINGS,
...settings,
};
}
🤖 Prompt for AI Agents
In apps/server/src/services/settings-service.ts around lines 261 to 272, the
projectPath parameter is used without path-traversal validation; update the
function to validate the incoming projectPath (reuse the existing validatePath
utility if available) or resolve the settingsPath and the allowed base project
directory with path.resolve(...) and ensure the resolved settingsPath is
contained within the baseDir (e.g., startsWith(baseDir) or using path.relative
to confirm it doesn't escape), and throw an error or reject the request if the
containment check fails before calling readJsonFile.
| // Extract global settings | ||
| const globalSettings: Partial<GlobalSettings> = { | ||
| theme: (appState.theme as GlobalSettings["theme"]) || "dark", | ||
| sidebarOpen: | ||
| appState.sidebarOpen !== undefined | ||
| ? (appState.sidebarOpen as boolean) | ||
| : true, | ||
| chatHistoryOpen: (appState.chatHistoryOpen as boolean) || false, | ||
| kanbanCardDetailLevel: | ||
| (appState.kanbanCardDetailLevel as GlobalSettings["kanbanCardDetailLevel"]) || | ||
| "standard", | ||
| maxConcurrency: (appState.maxConcurrency as number) || 3, | ||
| defaultSkipTests: | ||
| appState.defaultSkipTests !== undefined | ||
| ? (appState.defaultSkipTests as boolean) | ||
| : true, | ||
| enableDependencyBlocking: | ||
| appState.enableDependencyBlocking !== undefined | ||
| ? (appState.enableDependencyBlocking as boolean) | ||
| : true, | ||
| useWorktrees: (appState.useWorktrees as boolean) || false, | ||
| showProfilesOnly: (appState.showProfilesOnly as boolean) || false, | ||
| defaultPlanningMode: | ||
| (appState.defaultPlanningMode as GlobalSettings["defaultPlanningMode"]) || | ||
| "skip", | ||
| defaultRequirePlanApproval: | ||
| (appState.defaultRequirePlanApproval as boolean) || false, | ||
| defaultAIProfileId: | ||
| (appState.defaultAIProfileId as string | null) || null, | ||
| muteDoneSound: (appState.muteDoneSound as boolean) || false, | ||
| enhancementModel: | ||
| (appState.enhancementModel as GlobalSettings["enhancementModel"]) || | ||
| "sonnet", | ||
| keyboardShortcuts: | ||
| (appState.keyboardShortcuts as KeyboardShortcuts) || | ||
| DEFAULT_GLOBAL_SETTINGS.keyboardShortcuts, | ||
| aiProfiles: (appState.aiProfiles as AIProfile[]) || [], | ||
| projects: (appState.projects as ProjectRef[]) || [], | ||
| trashedProjects: | ||
| (appState.trashedProjects as TrashedProjectRef[]) || [], | ||
| projectHistory: (appState.projectHistory as string[]) || [], | ||
| projectHistoryIndex: (appState.projectHistoryIndex as number) || -1, | ||
| lastSelectedSessionByProject: | ||
| (appState.lastSelectedSessionByProject as Record<string, string>) || | ||
| {}, | ||
| }; |
There was a problem hiding this comment.
Use nullish coalescing (??) instead of logical OR (||) for falsy-but-valid values.
Several settings use || defaultValue which will incorrectly override valid falsy values like 0, false, or "". For example:
- Line 362:
maxConcurrencyof0would become3 - Line 392:
projectHistoryIndexof0would become-1
🔎 Partial fix for numeric fields
- maxConcurrency: (appState.maxConcurrency as number) || 3,
+ maxConcurrency: (appState.maxConcurrency as number) ?? 3,
...
- projectHistoryIndex: (appState.projectHistoryIndex as number) || -1,
+ projectHistoryIndex: (appState.projectHistoryIndex as number) ?? -1,🤖 Prompt for AI Agents
In apps/server/src/services/settings-service.ts around lines 351–396 the code
uses the logical OR (||) to apply defaults which will overwrite valid falsy
values (0, false, "", etc.); change those usages to nullish coalescing (??) so
only null or undefined trigger the default — specifically replace occurrences
like (appState.maxConcurrency as number) || 3, (appState.chatHistoryOpen as
boolean) || false, (appState.useWorktrees as boolean) || false,
(appState.showProfilesOnly as boolean) || false,
(appState.defaultRequirePlanApproval as boolean) || false,
(appState.defaultAIProfileId as string | null) || null, (appState.muteDoneSound
as boolean) || false, (appState.enhancementModel as
GlobalSettings["enrichmentModel"]) || "sonnet", (appState.keyboardShortcuts as
KeyboardShortcuts) || DEFAULT_GLOBAL_SETTINGS.keyboardShortcuts,
(appState.aiProfiles as AIProfile[]) || [], (appState.projects as ProjectRef[])
|| [], (appState.trashedProjects as TrashedProjectRef[]) || [],
(appState.projectHistory as string[]) || [], and (appState.projectHistoryIndex
as number) || -1 to use ?? with the same default values (keep the existing !==
undefined ternaries as they are); after making the changes, run TypeScript
build/tests to ensure types and behavior are correct.
| /** | ||
| * Credentials - stored in {DATA_DIR}/credentials.json | ||
| */ | ||
| export interface Credentials { | ||
| version: number; | ||
| apiKeys: { | ||
| anthropic: string; | ||
| google: string; | ||
| openai: string; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for credentials, writeFile, writeFileSync, chmod references in the TypeScript/JavaScript files
echo "=== Searching for credentials file handling ==="
rg -n "credentials|writeFile|chmod|0o[0-9]|mode:" --type ts --type js apps/server/src/ | head -50
echo ""
echo "=== Searching for DATA_DIR references ==="
rg -n "DATA_DIR" --type ts --type js apps/server/src/ | head -20
echo ""
echo "=== Checking for credentials-related files ==="
find apps/server/src/ -type f -name "*credential*" -o -name "*secret*" -o -name "*key*" 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 6421
🏁 Script executed:
#!/bin/bash
# Check the actual implementation of credentials writing in settings-service.ts
echo "=== Settings Service - around line 209 (atomicWriteJson call) ==="
sed -n '200,220p' apps/server/src/services/settings-service.ts
echo ""
echo "=== Settings Service - around line 50 (fs.writeFile call) ==="
sed -n '40,60p' apps/server/src/services/settings-service.ts
echo ""
echo "=== Finding atomicWriteJson definition ==="
rg -n "atomicWriteJson" --type ts apps/server/src/
echo ""
echo "=== Checking for mode parameter in writeFile calls ==="
rg -B2 -A2 "writeFile.*credentials" --type ts apps/server/src/Repository: AutoMaker-Org/automaker
Length of output: 1810
🏁 Script executed:
#!/bin/bash
# Check if there are any chmod calls for credentials file
echo "=== Checking for chmod calls ==="
rg -n "chmod" --type ts --type js apps/server/src/
echo ""
echo "=== Check full atomicWriteJson function ==="
sed -n '45,65p' apps/server/src/services/settings-service.ts
echo ""
echo "=== Check if credentials.json path has any security comments ==="
sed -n '100,115p' apps/server/src/lib/automaker-paths.ts
echo ""
echo "=== Check types/settings.ts header for security docs ==="
sed -n '140,155p' apps/server/src/types/settings.tsRepository: AutoMaker-Org/automaker
Length of output: 1511
Add restrictive file permissions and security documentation for credentials file.
The Credentials interface stores API keys as plain strings, and the atomicWriteJson function writes them to disk without setting restrictive file permissions. Currently, fs.writeFile() is called without a mode: 0o600 option, and there's no subsequent chmod() call to restrict access.
Update the file write to include { mode: 0o600 } to ensure only the owner can read/write the credentials file:
await fs.writeFile(tempPath, content, { mode: 0o600, encoding: "utf-8" });Additionally, document the security implications in the Credentials interface header comment to clarify that this file should never be committed or shared.
🤖 Prompt for AI Agents
In apps/server/src/types/settings.ts around lines 143 to 153, the Credentials
interface stores sensitive API keys but lacks documentation about secure
handling; update the header comment above the Credentials interface to state
this file contains secrets (never commit, do not share, restrict file
permissions) and mention it is written to {DATA_DIR}/credentials.json. Also
locate the atomicWriteJson or the code that writes the credentials file and
ensure the write sets restrictive permissions by passing { mode: 0o600 } to
fs.writeFile (and/or call fs.chmod(tempPath, 0o600) after write) so only the
owner can read/write the credentials file.
| export async function syncSettingsToServer(): Promise<boolean> { | ||
| if (!isElectron()) return false; | ||
|
|
||
| try { | ||
| const api = getHttpApiClient(); | ||
| const automakerStorage = localStorage.getItem("automaker-storage"); | ||
|
|
||
| if (!automakerStorage) { | ||
| return false; | ||
| } | ||
|
|
||
| const parsed = JSON.parse(automakerStorage); | ||
| const state = parsed.state || parsed; | ||
|
|
||
| // Extract settings to sync | ||
| const updates = { | ||
| theme: state.theme, | ||
| sidebarOpen: state.sidebarOpen, | ||
| chatHistoryOpen: state.chatHistoryOpen, | ||
| kanbanCardDetailLevel: state.kanbanCardDetailLevel, | ||
| maxConcurrency: state.maxConcurrency, | ||
| defaultSkipTests: state.defaultSkipTests, | ||
| enableDependencyBlocking: state.enableDependencyBlocking, | ||
| useWorktrees: state.useWorktrees, | ||
| showProfilesOnly: state.showProfilesOnly, | ||
| defaultPlanningMode: state.defaultPlanningMode, | ||
| defaultRequirePlanApproval: state.defaultRequirePlanApproval, | ||
| defaultAIProfileId: state.defaultAIProfileId, | ||
| muteDoneSound: state.muteDoneSound, | ||
| enhancementModel: state.enhancementModel, | ||
| keyboardShortcuts: state.keyboardShortcuts, | ||
| aiProfiles: state.aiProfiles, | ||
| projects: state.projects, | ||
| trashedProjects: state.trashedProjects, | ||
| projectHistory: state.projectHistory, | ||
| projectHistoryIndex: state.projectHistoryIndex, | ||
| lastSelectedSessionByProject: state.lastSelectedSessionByProject, | ||
| }; | ||
|
|
||
| const result = await api.settings.updateGlobal(updates); | ||
| return result.success; | ||
| } catch (error) { | ||
| console.error("[Settings Sync] Failed to sync settings:", error); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON.parse can throw on malformed localStorage data.
Line 171 parses automakerStorage without error handling. If the localStorage data is corrupted, this will throw an unhandled exception and return false silently.
🔎 Proposed fix
try {
const api = getHttpApiClient();
const automakerStorage = localStorage.getItem("automaker-storage");
if (!automakerStorage) {
return false;
}
- const parsed = JSON.parse(automakerStorage);
+ let parsed;
+ try {
+ parsed = JSON.parse(automakerStorage);
+ } catch {
+ console.error("[Settings Sync] Failed to parse automaker-storage");
+ return false;
+ }
const state = parsed.state || parsed;📝 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.
| export async function syncSettingsToServer(): Promise<boolean> { | |
| if (!isElectron()) return false; | |
| try { | |
| const api = getHttpApiClient(); | |
| const automakerStorage = localStorage.getItem("automaker-storage"); | |
| if (!automakerStorage) { | |
| return false; | |
| } | |
| const parsed = JSON.parse(automakerStorage); | |
| const state = parsed.state || parsed; | |
| // Extract settings to sync | |
| const updates = { | |
| theme: state.theme, | |
| sidebarOpen: state.sidebarOpen, | |
| chatHistoryOpen: state.chatHistoryOpen, | |
| kanbanCardDetailLevel: state.kanbanCardDetailLevel, | |
| maxConcurrency: state.maxConcurrency, | |
| defaultSkipTests: state.defaultSkipTests, | |
| enableDependencyBlocking: state.enableDependencyBlocking, | |
| useWorktrees: state.useWorktrees, | |
| showProfilesOnly: state.showProfilesOnly, | |
| defaultPlanningMode: state.defaultPlanningMode, | |
| defaultRequirePlanApproval: state.defaultRequirePlanApproval, | |
| defaultAIProfileId: state.defaultAIProfileId, | |
| muteDoneSound: state.muteDoneSound, | |
| enhancementModel: state.enhancementModel, | |
| keyboardShortcuts: state.keyboardShortcuts, | |
| aiProfiles: state.aiProfiles, | |
| projects: state.projects, | |
| trashedProjects: state.trashedProjects, | |
| projectHistory: state.projectHistory, | |
| projectHistoryIndex: state.projectHistoryIndex, | |
| lastSelectedSessionByProject: state.lastSelectedSessionByProject, | |
| }; | |
| const result = await api.settings.updateGlobal(updates); | |
| return result.success; | |
| } catch (error) { | |
| console.error("[Settings Sync] Failed to sync settings:", error); | |
| return false; | |
| } | |
| } | |
| export async function syncSettingsToServer(): Promise<boolean> { | |
| if (!isElectron()) return false; | |
| try { | |
| const api = getHttpApiClient(); | |
| const automakerStorage = localStorage.getItem("automaker-storage"); | |
| if (!automakerStorage) { | |
| return false; | |
| } | |
| let parsed; | |
| try { | |
| parsed = JSON.parse(automakerStorage); | |
| } catch { | |
| console.error("[Settings Sync] Failed to parse automaker-storage"); | |
| return false; | |
| } | |
| const state = parsed.state || parsed; | |
| // Extract settings to sync | |
| const updates = { | |
| theme: state.theme, | |
| sidebarOpen: state.sidebarOpen, | |
| chatHistoryOpen: state.chatHistoryOpen, | |
| kanbanCardDetailLevel: state.kanbanCardDetailLevel, | |
| maxConcurrency: state.maxConcurrency, | |
| defaultSkipTests: state.defaultSkipTests, | |
| enableDependencyBlocking: state.enableDependencyBlocking, | |
| useWorktrees: state.useWorktrees, | |
| showProfilesOnly: state.showProfilesOnly, | |
| defaultPlanningMode: state.defaultPlanningMode, | |
| defaultRequirePlanApproval: state.defaultRequirePlanApproval, | |
| defaultAIProfileId: state.defaultAIProfileId, | |
| muteDoneSound: state.muteDoneSound, | |
| enhancementModel: state.enhancementModel, | |
| keyboardShortcuts: state.keyboardShortcuts, | |
| aiProfiles: state.aiProfiles, | |
| projects: state.projects, | |
| trashedProjects: state.trashedProjects, | |
| projectHistory: state.projectHistory, | |
| projectHistoryIndex: state.projectHistoryIndex, | |
| lastSelectedSessionByProject: state.lastSelectedSessionByProject, | |
| }; | |
| const result = await api.settings.updateGlobal(updates); | |
| return result.success; | |
| } catch (error) { | |
| console.error("[Settings Sync] Failed to sync settings:", error); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/ui/src/hooks/use-settings-migration.ts around lines 160 to 205,
JSON.parse(automakerStorage) can throw if localStorage contains malformed data;
wrap the parse in its own try/catch (or use a safeParse helper) so parse errors
are caught, log the parsing error with context, return false (or clear/backup
the corrupted localStorage key) instead of letting an exception bubble, and only
proceed to build and send the updates when parsing succeeds.
| /* ======================================== | ||
| SYNTHWAVE '84 THEME | ||
| Neon dreams of the 80s | ||
| ======================================== */ |
There was a problem hiding this comment.
Fix incorrect theme comment.
The comment block references "SYNTHWAVE '84 THEME" but this file is onedark.css. This appears to be a copy-paste error.
🔎 Proposed fix
/* ========================================
- SYNTHWAVE '84 THEME
- Neon dreams of the 80s
+ ONE DARK THEME OVERRIDES
+ Component-specific customizations
======================================== */📝 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.
| /* ======================================== | |
| SYNTHWAVE '84 THEME | |
| Neon dreams of the 80s | |
| ======================================== */ | |
| /* ======================================== | |
| ONE DARK THEME OVERRIDES | |
| Component-specific customizations | |
| ======================================== */ |
🤖 Prompt for AI Agents
In apps/ui/src/styles/themes/onedark.css around lines 71 to 74, the block
comment incorrectly labels the file as "SYNTHWAVE '84 THEME"; replace that
comment with a correct One Dark theme header (e.g., "ONE DARK THEME" and a short
accurate descriptor like "A balanced dark theme") so the file comment matches
the actual theme and remove any leftover synthwave/neon wording.
| /* ======================================== | ||
| DRACULA THEME | ||
| Inspired by the popular Dracula VS Code theme | ||
| ======================================== */ |
There was a problem hiding this comment.
Incorrect comment block - copy-paste artifact.
The comment mentions "DRACULA THEME" but this file defines the Retro theme.
🔎 Suggested fix
-/* ========================================
- DRACULA THEME
- Inspired by the popular Dracula VS Code theme
- ======================================== */
+/* ========================================
+ RETRO / CYBERPUNK THEME
+ Neon green terminal aesthetic
+ ======================================== */📝 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.
| /* ======================================== | |
| DRACULA THEME | |
| Inspired by the popular Dracula VS Code theme | |
| ======================================== */ | |
| /* ======================================== | |
| RETRO / CYBERPUNK THEME | |
| Neon green terminal aesthetic | |
| ======================================== */ |
🤖 Prompt for AI Agents
In apps/ui/src/styles/themes/retro.css around lines 82 to 85, the comment block
incorrectly says "DRACULA THEME" (a copy-paste artifact) while this file defines
the Retro theme; update the comment to accurately describe the Retro theme
(e.g., change heading and description to "RETRO THEME" and a short line like
"Inspired by classic retro color palettes") so the header matches the file
purpose and remove any leftover references to Dracula.
| /* ======================================== | ||
| GRUVBOX THEME | ||
| Retro groove color scheme | ||
| ======================================== */ |
There was a problem hiding this comment.
Fix incorrect theme comment.
The comment block references "GRUVBOX THEME" but this file is solarized.css. This appears to be a copy-paste error.
🔎 Proposed fix
/* ========================================
- GRUVBOX THEME
- Retro groove color scheme
+ SOLARIZED THEME OVERRIDES
+ Component-specific customizations
======================================== */📝 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.
| /* ======================================== | |
| GRUVBOX THEME | |
| Retro groove color scheme | |
| ======================================== */ | |
| /* ======================================== | |
| SOLARIZED THEME OVERRIDES | |
| Component-specific customizations | |
| ======================================== */ |
🤖 Prompt for AI Agents
In apps/ui/src/styles/themes/solarized.css around lines 71 to 74, the comment
block incorrectly labels the section "GRUVBOX THEME" (a copy-paste error);
update the comment to correctly reference the Solarized theme and an appropriate
short description (e.g., "SOLARIZED THEME — Calm, low-contrast color scheme") so
the header matches the file and intent.
| /* Red Theme - Bold crimson/red aesthetic */ | ||
|
|
There was a problem hiding this comment.
Remove or correct the misleading comment.
The comment references "Red Theme - Bold crimson/red aesthetic" in a Synthwave theme file. This is a copy-paste error.
🔎 Suggested fix
-/* Red Theme - Bold crimson/red aesthetic */
-
+/* Theme-specific overrides for Synthwave */📝 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.
| /* Red Theme - Bold crimson/red aesthetic */ | |
| /* Theme-specific overrides for Synthwave */ |
🤖 Prompt for AI Agents
In apps/ui/src/styles/themes/synthwave.css around lines 71 to 72, remove or
update the misleading comment "Red Theme - Bold crimson/red aesthetic" which is
a copy-paste error; replace it with an accurate comment relevant to the
Synthwave theme (e.g., "Synthwave Theme - neon/purple aesthetic") or delete the
line entirely so the file comments correctly reflect the theme.
| /* ======================================== | ||
| SOLARIZED DARK THEME | ||
| The classic color scheme by Ethan Schoonover | ||
| ======================================== */ |
There was a problem hiding this comment.
Incorrect comment block - copy-paste artifact.
The comment mentions "SOLARIZED DARK THEME" but this file defines the Tokyonight theme. This appears to be a copy-paste error from a template.
🔎 Suggested fix
-/* ========================================
- SOLARIZED DARK THEME
- The classic color scheme by Ethan Schoonover
- ======================================== */
+/* ========================================
+ TOKYONIGHT THEME
+ Inspired by the Tokyo Night VS Code theme
+ ======================================== */📝 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.
| /* ======================================== | |
| SOLARIZED DARK THEME | |
| The classic color scheme by Ethan Schoonover | |
| ======================================== */ | |
| /* ======================================== | |
| TOKYONIGHT THEME | |
| Inspired by the Tokyo Night VS Code theme | |
| ======================================== */ |
🤖 Prompt for AI Agents
In apps/ui/src/styles/themes/tokyonight.css around lines 71 to 74, the comment
header incorrectly reads "SOLARIZED DARK THEME" (a copy-paste artifact); update
the comment to correctly identify the Tokyonight theme (e.g., "TOKYONIGHT THEME"
or a brief Tokyonight description) so the header matches the file contents and
remove any leftover references to Solarized.
- Add comprehensive unit tests for SettingsService, covering global and project settings management, including creation, updates, and merging with defaults. - Implement tests for handling credentials, ensuring proper masking and merging of API keys. - Introduce tests for migration from localStorage, validating successful data transfer and error handling. - Enhance error handling in subprocess management tests, ensuring robust timeout and output reading scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/server/tests/unit/lib/sdk-options.test.ts (1)
148-180: LGTM! Tests correctly verify optional parameter propagation.The new test cases for
createSuggestionsOptionsproperly verify thatsystemPrompt,abortController, andoutputFormatare included in the returned options when provided. The tests follow the existing pattern established in this file and align with the implementation shown in the relevant code snippets.Optional: Consider adding negative test cases
For completeness, you could add a test verifying that these optional fields are NOT present when NOT provided:
it("should not include optional fields when not provided", async () => { const { createSuggestionsOptions } = await import("@/lib/sdk-options.js"); const options = createSuggestionsOptions({ cwd: "/test/path" }); expect(options.systemPrompt).toBeUndefined(); expect(options.abortController).toBeUndefined(); expect(options.outputFormat).toBeUndefined(); });This verifies the conditional spread operators work correctly in both directions.
apps/server/tests/unit/lib/worktree-metadata.test.ts (1)
70-94: Consider aligning test pattern with existing tests for consistency.The new test cases use a different pattern than the existing tests in this suite. Specifically, the
branchparameter passed towriteWorktreeMetadataandreadWorktreeMetadatadoesn't match themetadata.branchproperty:
- Line 71:
branch = ""but line 73:branch: "branch"(literal string)- Line 85:
branch = "///"but line 87:branch: "branch"(literal string)In contrast, existing tests (lines 34-44, 46-56, 58-68) follow the pattern where the branch parameter matches the metadata.branch property (e.g.,
const branch = "feature/test-branch"andmetadata: { branch, ... }).While these tests verify round-trip consistency, they don't reflect realistic usage where the metadata about a branch would contain the actual branch name used as the lookup key.
🔎 Suggested refactor to align with established pattern
it("should handle empty branch name", async () => { const branch = ""; const metadata: WorktreeMetadata = { - branch: "branch", + branch, createdAt: new Date().toISOString(), }; // Empty branch name should be sanitized to "_branch" await writeWorktreeMetadata(testProjectPath, branch, metadata); const result = await readWorktreeMetadata(testProjectPath, branch); expect(result).toEqual(metadata); }); it("should handle branch name that becomes empty after sanitization", async () => { // Test branch that would become empty after removing invalid chars const branch = "///"; const metadata: WorktreeMetadata = { - branch: "branch", + branch, createdAt: new Date().toISOString(), }; await writeWorktreeMetadata(testProjectPath, branch, metadata); const result = await readWorktreeMetadata(testProjectPath, branch); expect(result).toEqual(metadata); });apps/server/tests/unit/lib/subprocess-manager.test.ts (1)
271-299: Test doesn't verify that timeout is reset on each output.The test advances time by 150ms with a 200ms timeout and checks the process isn't killed. However, this doesn't prove the timeout resets on each output—it only verifies the process isn't killed before the initial timeout expires.
To properly verify timeout reset behavior, the test should simulate outputs spaced over time intervals where each interval is less than the timeout, but the total duration exceeds it. For example, with a 200ms timeout, emit outputs at 150ms, 300ms, and 450ms. If the timeout resets correctly, the process completes successfully despite taking 450ms total.
🔎 Suggested approach to verify timeout reset behavior
- it("should reset timeout when output is received", async () => { - vi.useFakeTimers(); - const mockProcess = createMockProcess({ - stdoutLines: [ - '{"type":"first"}', - '{"type":"second"}', - '{"type":"third"}', - ], - exitCode: 0, - delayMs: 50, - }); - - vi.mocked(cp.spawn).mockReturnValue(mockProcess); - - const generator = spawnJSONLProcess({ - ...baseOptions, - timeout: 200, - }); - - const promise = collectAsyncGenerator(generator); - - // Advance time but not enough to trigger timeout - await vi.advanceTimersByTimeAsync(150); - // Process should not be killed yet - expect(mockProcess.kill).not.toHaveBeenCalled(); - - vi.useRealTimers(); - await promise; - }); + it("should reset timeout when output is received", async () => { + vi.useFakeTimers(); + const mockProcess = new EventEmitter() as any; + const stdout = new Readable({ read() {} }); + const stderr = new Readable({ read() {} }); + mockProcess.stdout = stdout; + mockProcess.stderr = stderr; + mockProcess.kill = vi.fn(); + + vi.mocked(cp.spawn).mockReturnValue(mockProcess); + + const generator = spawnJSONLProcess({ + ...baseOptions, + timeout: 200, // 200ms timeout + }); + + const promise = collectAsyncGenerator(generator); + + // Emit outputs at intervals that would exceed timeout without reset + // Each output is within timeout, but total time exceeds it + process.nextTick(() => { + setTimeout(() => stdout.push('{"type":"first"}\n'), 150); // at 150ms + setTimeout(() => stdout.push('{"type":"second"}\n'), 300); // at 300ms (would timeout without reset) + setTimeout(() => stdout.push('{"type":"third"}\n'), 450); // at 450ms (would timeout without reset) + setTimeout(() => { + stdout.push(null); + mockProcess.emit("exit", 0); + }, 500); + }); + + // Advance through all the timeouts + await vi.advanceTimersByTimeAsync(500); + + vi.useRealTimers(); + const results = await promise; + + // Process should complete successfully with all outputs + expect(mockProcess.kill).not.toHaveBeenCalled(); + expect(results).toHaveLength(3); + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/server/tests/unit/lib/automaker-paths.test.ts(2 hunks)apps/server/tests/unit/lib/error-handler.test.ts(3 hunks)apps/server/tests/unit/lib/sdk-options.test.ts(3 hunks)apps/server/tests/unit/lib/security.test.ts(2 hunks)apps/server/tests/unit/lib/subprocess-manager.test.ts(1 hunks)apps/server/tests/unit/lib/worktree-metadata.test.ts(1 hunks)apps/server/tests/unit/providers/claude-provider.test.ts(1 hunks)apps/server/tests/unit/services/settings-service.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/server/tests/unit/lib/security.test.ts (1)
apps/server/src/lib/security.ts (2)
initAllowedPaths(15-35)getAllowedPaths(61-63)
apps/server/tests/unit/lib/error-handler.test.ts (1)
apps/server/src/lib/error-handler.ts (2)
isCancellationError(30-38)classifyError(78-105)
apps/server/tests/unit/lib/subprocess-manager.test.ts (2)
apps/server/src/lib/subprocess-manager.ts (1)
spawnJSONLProcess(26-153)apps/server/tests/utils/helpers.ts (1)
collectAsyncGenerator(8-14)
apps/server/tests/unit/providers/claude-provider.test.ts (1)
apps/server/tests/utils/helpers.ts (1)
collectAsyncGenerator(8-14)
apps/server/tests/unit/lib/automaker-paths.test.ts (1)
apps/server/src/lib/automaker-paths.ts (4)
getGlobalSettingsPath(102-104)getCredentialsPath(109-111)getProjectSettingsPath(116-118)ensureDataDir(123-126)
apps/server/tests/unit/lib/sdk-options.test.ts (1)
apps/server/src/lib/sdk-options.ts (3)
createSuggestionsOptions(213-226)createAutoModeOptions(265-279)createCustomOptions(286-305)
🪛 Biome (2.1.2)
apps/server/tests/unit/providers/claude-provider.test.ts
[error] 243-245: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ 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: e2e
🔇 Additional comments (19)
apps/server/tests/unit/lib/sdk-options.test.ts (2)
243-264: LGTM! Tests correctly verify optional parameter propagation.The new test cases for
createAutoModeOptionsproperly verify thatsystemPromptandabortControllerare included when provided. The implementation is consistent with the pattern used throughout this test file.
295-330: LGTM! Tests correctly verify optional parameter propagation.The new test cases for
createCustomOptionsproperly verify thatsandbox,systemPrompt, andabortControllerare included when provided. The sandbox test appropriately verifies the complete object structure with both properties.apps/server/tests/unit/lib/subprocess-manager.test.ts (1)
301-326: Good test for stdout error handling.This test properly verifies the error path when reading stdout fails. It correctly expects both error propagation (rejection) and logging behavior.
apps/server/tests/unit/providers/claude-provider.test.ts (1)
237-260: LGTM! Comprehensive error handling test.The test correctly verifies that errors thrown by the SDK's async generator are propagated and logged. The async generator intentionally throws without yielding to simulate SDK failures.
Note: The static analysis warning about the generator not containing
yieldis a false positive—the generator is designed to throw immediately to test error handling.apps/server/tests/unit/lib/security.test.ts (2)
56-68: LGTM! Proper test coverage for WORKSPACE_DIR handling.The test correctly verifies that
WORKSPACE_DIRis included in allowed paths when set, aligning with the implementation insecurity.ts.
73-73: Good practice: Test isolation maintained.Explicitly deleting
WORKSPACE_DIRin these tests ensures they aren't affected by environment state from other tests.Also applies to: 88-88
apps/server/tests/unit/lib/error-handler.test.ts (2)
5-5: LGTM! Comprehensive coverage for cancellation error detection.The tests thoroughly validate
isCancellationErrorbehavior, including all supported keywords ('cancelled', 'canceled', 'stopped', 'aborted'), case insensitivity, and negative cases.Also applies to: 36-62
123-157: LGTM! Correct precedence validation.The tests properly verify that
classifyErrorprioritizes error types as: authentication > abort > cancellation, which matches the implementation inerror-handler.ts.apps/server/tests/unit/lib/automaker-paths.test.ts (1)
144-229: LGTM! Thorough coverage of new path utilities.The tests comprehensively validate:
- Path construction for global settings, credentials, and project settings
- Edge cases like trailing slashes
- Directory creation with
ensureDataDir, including idempotency and nested paths- Proper setup/teardown with temp directories
apps/server/tests/unit/services/settings-service.test.ts (10)
1-38: LGTM! Excellent test structure and setup.Proper test isolation with unique temp directories per test run and thorough cleanup in
afterEach.
40-93: LGTM! Comprehensive global settings tests.Tests thoroughly validate:
- Default settings when file doesn't exist
- Reading existing settings
- Merging with defaults for partial settings
- Deep merging of nested objects (keyboardShortcuts)
95-160: LGTM! Update and creation logic well-tested.Tests verify creation, merging, deep merging of keyboard shortcuts, and automatic data directory creation. The verification includes both in-memory results and persisted file content.
175-212: LGTM! Credentials handling properly validated.Tests cover defaults, reading existing credentials, and merging with defaults for missing API keys, ensuring backward compatibility.
214-283: LGTM! Credentials update and deep merge tested.Tests verify creation, merging, and deep merging of API keys, ensuring individual keys are preserved during partial updates.
285-325: LGTM! Masking logic thoroughly tested.Tests cover empty keys, correct masking format, and edge cases like short keys. The masking pattern (e.g., "sk-a...cdef") is validated.
342-378: LGTM! Project settings read and merge tested.Tests verify default settings, reading existing project settings, and merging with defaults for partial settings.
380-464: LGTM! Project updates and directory creation validated.Tests verify creation, merging, deep merging of board background settings, and automatic
.automakerdirectory creation.
480-616: LGTM! Comprehensive migration testing.Excellent coverage of localStorage migration scenarios:
- Global settings, credentials, and project settings migration
- Direct localStorage values (lastProjectDir, recentFolders, worktreePanelCollapsed)
- Invalid JSON handling
- Error handling for read-only directories
The tests ensure robust migration with proper error recovery.
625-641: LGTM! Atomic write error handling tested.Tests verify cleanup behavior when writes fail due to permission errors, using a read-only directory simulation.
…rage) This commit addresses CodeRabbit feedback from PR #186 by adding detailed documentation to all public APIs in the settings module: **Server-side documentation:** - SettingsService class: 12 public methods with parameter and return types - Settings types (settings.ts): All type aliases, interfaces, and constants documented with usage context - Route handlers (8 endpoints): Complete endpoint documentation with request/response schemas - Automaker paths utilities: All 13 path resolution functions fully documented **Client-side documentation:** - useSettingsMigration hook: Migration flow and state documented - Sync functions: Three sync helpers (settings, credentials, project) with usage guidelines - localStorage constants: Clear documentation of migration keys and cleanup strategy All docstrings follow JSDoc format with: - Purpose and behavior description - Parameter documentation with types - Return value documentation - Usage examples where applicable - Cross-references between related functions This improves code maintainability, IDE autocomplete, and developer onboarding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
1 participant
better UX, makes it feel personal from the start
Summary by CodeRabbit
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.