completly remove sandbox related code as the downstream libraries do … #378
Conversation
…not work with it on various os
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR removes sandbox/cloud-storage plumbing, simplifies MCP/sdk option handling to autonomous defaults, adds feature description history and APIs, introduces a skip-verification Auto Mode flag, and replaces localStorage-first settings with API-first hydration and debounced server sync. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant EditDialog as Edit Feature Dialog
participant PersistHook as UI Persist Hook
participant API as HttpApiClient
participant Server as Server Route
participant Loader as FeatureLoader
participant Store as Feature Storage
User->>EditDialog: Edit or request enhancement
EditDialog->>EditDialog: Track source & enhancementMode
User->>EditDialog: Submit changes
EditDialog->>PersistHook: onUpdate(id, updates, source, mode)
PersistHook->>API: POST /api/features/update (includes source, mode)
API->>Server: HTTP request
Server->>Loader: update(projectPath, id, updates, source, mode)
Loader->>Loader: Append DescriptionHistoryEntry & persist
Loader->>Store: Save updated feature
Store-->>Loader: Persist OK
Loader-->>Server: Return updated feature
Server-->>API: 200 OK
API-->>PersistHook: Success
PersistHook-->>EditDialog: Complete
EditDialog->>User: Show confirmation
sequenceDiagram
autonumber
participant App as Root
participant Auth as Session Verification
participant Hydrator as Settings Hydration
participant Server as Settings API
participant AppStore as App Store
participant Sync as Settings Sync
App->>Auth: verifySession()
Auth->>Server: GET /api/session/verify
Server-->>Auth: session result
Auth-->>App: result
App->>Hydrator: hydrateStoreFromSettings()
Hydrator->>Server: GET /api/settings/global
Server-->>Hydrator: Global settings
Hydrator->>AppStore: Hydrate app & setup stores
Hydrator-->>App: signalMigrationComplete
App->>Sync: initialize (wait for signal)
AppStore->>Sync: on change -> debounce -> POST /api/settings/global
Sync->>Server: POST updates
Server-->>Sync: 200 OK
Sync-->>App: sync complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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 focuses on a significant architectural change by entirely deprecating and removing the sandbox feature, which was causing instability across different operating systems. Concurrently, it enhances the auto mode's flexibility by allowing users to bypass dependency verification requirements, streamlining the development workflow. Several UI and backend adjustments support these core changes, improving the overall robustness and user experience of the application. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/tests/unit/lib/sdk-options.test.ts (1)
178-247: Add test coverage for the new permission modelThe tests verify functional behavior but don't validate the security-critical permission modes introduced in this PR. Consider adding tests to ensure:
- Spec/feature generation use
permissionMode: 'default'(safe)- Chat/auto-mode use
permissionMode: 'bypassPermissions'(verify this is intentional)- The
allowDangerouslySkipPermissionsflag is set appropriately📝 Example test structure
describe('permission modes', () => { it('should use default permission mode for spec generation', async () => { const { createSpecGenerationOptions } = await import('@/lib/sdk-options.js'); const options = createSpecGenerationOptions({ cwd: '/test/path' }); expect(options.permissionMode).toBe('default'); expect(options.allowDangerouslySkipPermissions).toBeUndefined(); }); it('should use bypass permission mode for auto-mode', async () => { const { createAutoModeOptions } = await import('@/lib/sdk-options.js'); const options = createAutoModeOptions({ cwd: '/test/path' }); expect(options.permissionMode).toBe('bypassPermissions'); expect(options.allowDangerouslySkipPermissions).toBe(true); }); // Add similar tests for chat, suggestions, custom options... });
🤖 Fix all issues with AI agents
In @apps/ui/src/store/app-store.ts:
- Around line 1578-1583: The setter setSkipVerificationInAutoMode currently
updates state then awaits syncSettingsToServer without rollback on failure;
mirror the pattern used in setAutoLoadClaudeMd by wrapping the import/await of
syncSettingsToServer in try/catch and, on error, revert the store to the
previous value (call set({ skipVerificationInAutoMode: previousValue })), then
rethrow or handle the error as appropriate so UI and server remain consistent.
🧹 Nitpick comments (5)
apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx (1)
75-97: Consider removingcurrentTimefrom effect dependencies to avoid unnecessary churn.Including
currentTimein the dependency array causes the effect to cleanup and re-setup the interval every second when it updates. This works but is inefficient.♻️ Suggested optimization
useEffect(() => { if (!feature.justFinishedAt || feature.status !== 'waiting_approval') { return; } const finishedTime = new Date(feature.justFinishedAt).getTime(); const twoMinutes = 2 * 60 * 1000; - const timeRemaining = twoMinutes - (currentTime - finishedTime); - - if (timeRemaining <= 0) { - return; - } // eslint-disable-next-line no-undef const interval = setInterval(() => { - setCurrentTime(Date.now()); + const now = Date.now(); + setCurrentTime(now); + if (now - finishedTime >= twoMinutes) { + clearInterval(interval); + } }, 1000); return () => { // eslint-disable-next-line no-undef clearInterval(interval); }; - }, [feature.justFinishedAt, feature.status, currentTime]); + }, [feature.justFinishedAt, feature.status]);libs/types/src/settings.ts (1)
409-410: Clarify what "verification" means in the commentThe comment "Skip verification requirement in auto-mode (treat 'completed' same as 'verified')" is helpful but could be more explicit about the user-facing impact. Consider expanding it to explain:
- What verification normally requires
- What happens when skipped
- Any risks or implications
📝 Suggested improvement
- /** Skip verification requirement in auto-mode (treat 'completed' same as 'verified') */ + /** Skip verification requirement in auto-mode + * + * When enabled, auto-mode will treat 'completed' features as 'verified' without manual review. + * This speeds up the pipeline but bypasses manual quality checks. + * Default: false (verification required) + */ skipVerificationInAutoMode: boolean;apps/ui/src/components/views/board-view.tsx (1)
892-901: Consider updating local state synchronously for consistency.The branch assignment logic updates the server state via
persistFeatureUpdate, but the local state (viaupdateFeature) is not updated. This could cause a brief UI inconsistency where the feature appears without a branch assignment.Optional: Update local state synchronously
if (!feature.branchName) { const primaryBranch = (currentProject.path ? getPrimaryWorktreeBranch(currentProject.path) : null) || 'main'; + // Update local state immediately for UI consistency + updateFeature(feature.id, { branchName: primaryBranch }); + // Also persist to server await persistFeatureUpdate(feature.id, { branchName: primaryBranch, }); }This ensures the UI immediately reflects the branch assignment before the feature starts.
apps/ui/src/hooks/use-auto-mode.ts (1)
97-112: Effect may re-run unnecessarily whenisAutoModeRunningchanges.Including
isAutoModeRunningin the dependency array causes the effect to re-execute every time the running state changes. While thedesired !== isAutoModeRunningguard prevents infinite loops, the effect still re-reads from sessionStorage on every state change, which is wasteful. Consider removingisAutoModeRunningfrom the dependencies since the restoration logic only needs to run once per project.♻️ Suggested improvement
useEffect(() => { if (!currentProject) return; const session = readAutoModeSession(); const desired = session[currentProject.path]; if (typeof desired !== 'boolean') return; - if (desired !== isAutoModeRunning) { + // Get current state directly to avoid stale closure + const currentState = useAppStore.getState().autoModeByProject[currentProject.id]; + const currentIsRunning = currentState?.isRunning ?? false; + + if (desired !== currentIsRunning) { logger.info( `[AutoMode] Restoring session state for ${currentProject.path}: ${desired ? 'ON' : 'OFF'}` ); setAutoModeRunning(currentProject.id, desired); } - }, [currentProject, isAutoModeRunning, setAutoModeRunning]); + }, [currentProject, setAutoModeRunning]);apps/ui/src/hooks/use-settings-migration.ts (1)
133-174: Unsafe type casts in hydration logic may mask type mismatches.The hydration code uses multiple
as unknown ascasts (lines 140, 162-164) which bypass TypeScript's type checking. If the server returns a different shape than expected (e.g., theme values not in theThemeModeunion), these casts will silently accept invalid data.Consider validating critical fields or using type guards:
♻️ Suggested type validation for theme
+const VALID_THEMES = ['light', 'dark', 'system', 'retro', 'dracula', 'nord', 'monokai', 'tokyonight', 'solarized', 'gruvbox', 'catppuccin', 'onedark', 'synthwave', 'red', 'cream', 'sunset', 'gray'] as const; + +function isValidTheme(theme: unknown): theme is typeof VALID_THEMES[number] { + return typeof theme === 'string' && VALID_THEMES.includes(theme as any); +} useAppStore.setState({ - theme: serverSettings.theme as unknown as import('@/store/app-store').ThemeMode, + theme: isValidTheme(serverSettings.theme) ? serverSettings.theme : current.theme,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
apps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/auto-mode/routes/follow-up-feature.tsapps/server/src/routes/context/routes/describe-file.tsapps/server/src/routes/context/routes/describe-image.tsapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/server/src/services/settings-service.tsapps/server/tests/unit/lib/sdk-options.test.tsapps/server/tests/unit/providers/claude-provider.test.tsapps/ui/src/components/dialogs/index.tsapps/ui/src/components/dialogs/sandbox-rejection-screen.tsxapps/ui/src/components/dialogs/sandbox-risk-dialog.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/board-view/dialogs/auto-mode-settings-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsxapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsxapps/ui/src/hooks/use-auto-mode.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tslibs/dependency-resolver/src/index.tslibs/dependency-resolver/src/resolver.tslibs/types/src/provider.tslibs/types/src/settings.ts
💤 Files with no reviewable changes (8)
- libs/types/src/provider.ts
- apps/server/src/routes/context/routes/describe-file.ts
- apps/server/src/lib/settings-helpers.ts
- apps/ui/src/lib/http-api-client.ts
- apps/ui/src/components/dialogs/sandbox-rejection-screen.tsx
- apps/ui/src/components/dialogs/index.ts
- apps/server/src/services/agent-service.ts
- apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
libs/types/src/settings.tslibs/dependency-resolver/src/index.tsapps/server/src/routes/auto-mode/routes/follow-up-feature.tsapps/server/src/routes/worktree/routes/diffs.tsapps/ui/src/hooks/use-auto-mode.tsapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/server/tests/unit/providers/claude-provider.test.tsapps/ui/src/hooks/use-settings-migration.tsapps/server/src/routes/context/routes/describe-image.tsapps/ui/src/routes/__root.tsxapps/server/tests/unit/lib/sdk-options.test.tslibs/dependency-resolver/src/resolver.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/server/src/services/settings-service.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/dialogs/auto-mode-settings-dialog.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsxapps/server/src/providers/claude-provider.tsapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsxapps/ui/src/components/views/settings-view.tsxapps/server/src/lib/sdk-options.tsapps/ui/src/components/views/board-view.tsxapps/server/src/services/auto-mode-service.tsapps/ui/src/store/app-store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
libs/types/src/settings.tslibs/dependency-resolver/src/index.tsapps/server/src/routes/auto-mode/routes/follow-up-feature.tsapps/server/src/routes/worktree/routes/diffs.tsapps/ui/src/hooks/use-auto-mode.tsapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/server/tests/unit/providers/claude-provider.test.tsapps/ui/src/hooks/use-settings-migration.tsapps/server/src/routes/context/routes/describe-image.tsapps/ui/src/routes/__root.tsxapps/server/tests/unit/lib/sdk-options.test.tslibs/dependency-resolver/src/resolver.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/server/src/services/settings-service.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/dialogs/auto-mode-settings-dialog.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsxapps/server/src/providers/claude-provider.tsapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsxapps/ui/src/components/views/settings-view.tsxapps/server/src/lib/sdk-options.tsapps/ui/src/components/views/board-view.tsxapps/server/src/services/auto-mode-service.tsapps/ui/src/store/app-store.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/auto-mode/routes/follow-up-feature.tsapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/context/routes/describe-image.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/server/src/services/settings-service.tsapps/server/src/providers/claude-provider.tsapps/server/src/lib/sdk-options.tsapps/server/src/services/auto-mode-service.ts
🧠 Learnings (3)
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/server/src/routes/auto-mode/routes/follow-up-feature.tsapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view.tsx
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/board-view/dialogs/auto-mode-settings-dialog.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsxapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/board-view.tsx
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Applies to **/*.{ts,tsx} : Use `resolveModelString()` from automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Applied to files:
apps/ui/src/store/app-store.ts
🧬 Code graph analysis (8)
apps/server/src/routes/auto-mode/routes/follow-up-feature.ts (1)
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(16-126)
apps/server/src/routes/worktree/routes/diffs.ts (2)
libs/git-utils/src/diff.ts (1)
getGitRepositoryDiffs(248-283)apps/server/src/routes/worktree/common.ts (1)
logError(156-156)
apps/ui/src/hooks/use-settings-migration.ts (3)
libs/types/src/settings.ts (1)
GlobalSettings(386-487)apps/server/src/types/settings.ts (1)
GlobalSettings(20-20)apps/ui/src/lib/storage.ts (1)
getItem(21-28)
apps/server/tests/unit/lib/sdk-options.test.ts (1)
apps/server/src/lib/sdk-options.ts (4)
createChatOptions(392-419)MAX_TURNS(79-91)TOOL_PRESETS(62-74)createAutoModeOptions(430-454)
apps/server/src/routes/worktree/routes/file-diff.ts (2)
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(16-126)apps/server/src/routes/worktree/common.ts (1)
logError(156-156)
apps/ui/src/components/views/board-view/board-header.tsx (1)
apps/ui/src/components/views/board-view/dialogs/auto-mode-settings-dialog.tsx (1)
AutoModeSettingsDialog(19-68)
apps/ui/src/components/views/board-view.tsx (1)
libs/dependency-resolver/src/resolver.ts (1)
getBlockingDependencies(221-230)
apps/server/src/services/auto-mode-service.ts (2)
libs/types/src/feature.ts (1)
Feature(24-58)libs/dependency-resolver/src/resolver.ts (1)
areDependenciesSatisfied(190-212)
⏰ 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 (46)
apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx (5)
1-6: LGTM!Imports are well-organized. The
getBlockingDependenciescorrectly uses the@automaker/dependency-resolvershared package per coding guidelines.
8-48: Clean implementation of uniform badge styling.The
uniformBadgeClassconstant andCardBadgescomponent are well-structured. The early return pattern on line 21 is clean, and the error badge styling with CSS variables ensures theme consistency.
54-64: Good use of shared package for dependency resolution.The blocking dependencies calculation is well-memoized and correctly uses
getBlockingDependenciesfrom@automaker/dependency-resolver.
99-108: Clean badge visibility logic.The consolidated
showBadgescheck with early return is a good pattern. Thefeature.status === 'backlog'check on line 100 is technically redundant sinceblockingDependencieswill be empty when status isn't'backlog'(line 60), but it adds clarity and defensive safety.
110-224: Well-structured badge rendering with consistent styling.All badges follow the uniform styling pattern and include proper tooltips for accessibility. The blocked badge tooltip (lines 184-197) provides helpful context by listing blocking dependency descriptions. The
animate-pulseon the "Just Finished" badge is a nice touch for drawing attention.apps/server/src/routes/auto-mode/routes/follow-up-feature.ts (1)
34-36: Clarify the intentionality and implications of overridingfollowUpFeature's worktree default.The
followUpFeaturemethod inAutoModeServicedefaultsuseWorktreestotrue(line 862 of auto-mode-service.ts), but this route explicitly passesfalseviauseWorktrees ?? false. This overrides the method's original design. While this aligns withrun-featureandresume-featurebehavior at the route level, it represents a breaking change forfollowUpFeaturespecifically.The retrieved learning states: "Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution." The git history confirms this change is intentional—part of a commit to "completely remove sandbox related code"—but this represents a deliberate shift away from worktree-based isolation.
Ensure this behavioral change is documented and that any downstream safety mechanisms exist to prevent unintended modifications to the main branch when worktrees are disabled.
⛔ Skipped due to learnings
Learnt from: CR Repo: AutoMaker-Org/automaker PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-28T05:07:48.147Z Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent executionapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsx (4)
3-3: LGTM! Clean removal of unused import.The
Shieldimport has been correctly removed as it's no longer needed after removing the sandbox mode UI. TheFileCodeimport is properly retained for the CLAUDE.md integration feature.
6-9: LGTM! Props interface correctly simplified.The interface has been properly updated to remove sandbox-related props, leaving only the Auto-load CLAUDE.md configuration. The remaining props are well-typed and align with the component's simplified functionality.
11-24: LGTM! Documentation accurately updated.The component documentation has been correctly updated to reflect the removal of sandbox mode settings. The usage example now only shows the Auto-load CLAUDE.md props, matching the simplified component interface.
25-81: Component refactoring verified. Parent components have been properly updated to only pass the current props (autoLoadClaudeMdandonAutoLoadClaudeMdChange). No instances of the removed props (enableSandboxModeandonEnableSandboxModeChange) exist in the codebase. The component implementation is clean, with all props correctly wired and the UI properly simplified.libs/dependency-resolver/src/index.ts (1)
15-15: LGTM! Public API extended with new type export.The addition of
DependencySatisfactionOptionsto the public API surface is correct and aligns with the broader changes introducing skipVerification semantics for dependency resolution.apps/server/tests/unit/providers/claude-provider.test.ts (1)
82-102: LGTM! Test correctly validates the new allowedTools behavior.The test has been properly updated to verify that
allowedToolsis not injected into SDK options when not explicitly provided. The name change accurately reflects the new expectation, and the use ofexpect.not.objectContaining()correctly asserts the absence of the property.apps/server/src/routes/worktree/routes/file-diff.ts (3)
18-23: LGTM! Request body properly extended with optional useWorktrees flag.The addition of the optional
useWorktreesparameter maintains backward compatibility while enabling the new bypass behavior.
33-37: LGTM! Early-return bypass logic is correct.The strict equality check
useWorktrees === falseproperly bypasses worktree probing when explicitly disabled, returning an empty diff. This aligns with the PR's goal to simplify worktree handling.
66-73: LGTM! Error handling properly suppresses expected ENOENT errors.The refined error handling correctly treats ENOENT as an expected, non-error case when a feature has no worktree, reducing log noise while still logging genuine errors.
apps/server/src/routes/worktree/routes/diffs.ts (3)
14-18: LGTM! Request body properly extended with optional useWorktrees flag.The addition of the optional
useWorktreesparameter is consistent with the pattern in file-diff.ts and maintains backward compatibility.
28-39: LGTM! Early-return bypass logic correctly falls back to main project path.The strict equality check properly bypasses worktree probing when explicitly disabled, falling back to computing diffs from the main project path. This approach reduces noisy logs and simplifies the worktree flow.
56-63: LGTM! Error handling properly suppresses expected ENOENT errors.The refined error handling correctly treats ENOENT as an expected, non-error case when a feature has no worktree, reducing log noise while preserving the fallback behavior to the main project path.
apps/ui/src/routes/__root.tsx (1)
31-31: LGTM! Sandbox-related code successfully removed from root layout.The removal of sandbox-related state and UI flow from the root layout is clean. The core routing, authentication, and theme handling logic remain intact and functional. This aligns with the PR's goal to remove sandbox complexity across the application.
apps/server/src/lib/sdk-options.ts (3)
153-169: Simplified MCP options structure looks cleanThe refactored
buildMcpOptionsnow returns a simple object with justmcpServerOptions, which is cleaner than the previous approach. The removal of permission-related logic aligns with the broader simplification, though the security implications should be verified.
302-316: Good: Spec generation correctly overrides to safe permission modeOverriding
permissionMode: 'default'for spec generation is the right choice, as it only needs read-only tools and should not execute dangerous operations. The comment on Line 305 correctly references the security issue this addresses.
337-349: Good: Feature generation also uses safe permission modeSimilar to spec generation, overriding to
permissionMode: 'default'is appropriate here since feature generation only needs read-only tools.apps/server/src/services/settings-service.ts (1)
532-535: Good: Safe default for new verification settingThe migration correctly defaults
skipVerificationInAutoModetofalsewhen not present in localStorage. This is the conservative choice that maintains existing behavior (requiring verification).apps/ui/src/components/views/settings-view.tsx (1)
34-35: LGTM: Clean UI wiring for new settingThe integration of
skipVerificationInAutoModefollows the established pattern used for other settings in this component. The prop passing is consistent and type-safe.Also applies to: 131-131, 140-140
apps/server/src/routes/context/routes/describe-image.ts (1)
403-403: LGTM - Logging cleanup aligned with sandbox removal.The removal of sandbox-related options from the log output is consistent with the PR's objective to remove sandbox code.
apps/ui/src/components/views/board-view/board-header.tsx (1)
1-1: LGTM - Clean integration of Auto Mode Settings.The integration of the Auto Mode Settings dialog follows established patterns:
- Proper state management with local and store state
- Accessible button with title attribute
- Data-testids for testing
- Clean separation of concerns
Also applies to: 7-7, 12-12, 43-43, 46-47, 105-122
apps/ui/src/components/views/board-view.tsx (4)
736-741: Extensive logging additions are helpful for debugging.The added logging statements provide good visibility into the auto-mode decision-making process. The use of appropriate log levels (info vs. debug) is correct.
Also applies to: 766-773, 781-786, 828-842, 864-869, 878-883
794-826: Fallback logic ensures auto-mode continues across worktree views.The logic now prefers backlog features visible in the current view but falls back to all backlog features when none are visible. This ensures auto-mode doesn't stop when users switch to non-primary worktrees.
The comment at line 795 clearly explains this deviation from the column filtering logic.
853-862: Eligibility logic correctly honors skip verification setting.The conditional filtering properly implements the skip verification feature:
- Blocked features are excluded only when
enableDependencyBlockingis true ANDskipVerificationInAutoModeis false- When skip verification is enabled, all sorted backlog features are eligible
- The comment clearly explains the intent
944-944: Correct dependency added to effect.Adding
skipVerificationInAutoModeto the dependency array is necessary since the effect uses this value in the eligibility filtering logic (lines 854, 862).apps/ui/src/components/views/board-view/dialogs/auto-mode-settings-dialog.tsx (1)
1-68: Well-structured and accessible dialog component.The
AutoModeSettingsDialogfollows best practices:
- Clean TypeScript interface for props
- Proper use of Dialog components
- Semantic HTML with Label for accessibility
- Data-testids for testing
- Clear, descriptive text explaining the setting's behavior
- Visual icon (FastForward) reinforces the "skip" concept
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (3)
82-82: Sorting logic correctly implements skip verification.The sorting logic properly considers
skipVerificationInAutoMode:
- When enabled (along with
enableDependencyBlocking), blocking dependencies are checked- When disabled or
skipVerificationInAutoModeis true, all features are treated as unblocked- This matches the implementation in
board-view.tsxfor consistencyAlso applies to: 809-816
828-836: Eligibility check and user message are well-implemented.The eligibility logic correctly allows features with dependencies when
skipVerificationInAutoModeis enabled. The user-facing message helpfully directs users to the new Auto Mode setting as an alternative to completing dependencies.
852-852: Correct dependency added to callback.Adding
skipVerificationInAutoModeto thehandleStartNextFeaturescallback dependencies is necessary since the function references this value in the sorting and eligibility logic.apps/ui/src/hooks/use-auto-mode.ts (1)
10-38: Well-structured session persistence helpers.The session storage helpers are robust with proper SSR guards (
typeof window === 'undefined'), defensive JSON parsing, and silent error handling for private browsing mode. The per-project keying approach correctly isolates auto-mode state across projects.apps/server/src/providers/claude-provider.ts (1)
80-84: Clean simplification of SDK options.The removal of complex autonomous-mode logic that computed default tools based on MCP server presence simplifies the code significantly. The conditional spread pattern
...(allowedTools && { allowedTools })is idiomatic and correctly passes through caller-provided tools only when explicitly set.apps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsx (1)
6-11: Clean removal of sandbox-related props and UI.The interface simplification removes sandbox warning props cleanly, and the component now focuses solely on project deletion functionality. The empty state condition is correctly simplified.
apps/ui/src/hooks/use-settings-migration.ts (1)
261-282: Good improvement: Prefer live Zustand state over localStorage.The refactor to use
useAppStore.getState()directly avoids race conditions where Zustand persistence might lag behindset()calls. The fallback to localStorage (lines 274-282) provides resilience for edge cases where the store might not be ready.apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx (1)
317-340: Well-implemented new setting for skip verification in auto mode.The UI follows the established pattern in this component with proper checkbox wiring, accessible label association (
htmlFor), and clear explanatory text. The FastForward icon appropriately conveys the "speed up" nature of this feature.libs/dependency-resolver/src/resolver.ts (2)
199-211: Verify intended behavior:skipVerificationallows deps in any non-running status.When
skipVerificationistrue, dependencies in statuses like'backlog','failed', or'error'are considered "satisfied" (only'running'blocks). This means a feature could be picked up even if its dependency hasn't started yet or has failed.If the intent is only to skip the
'verified'requirement while still requiring'completed', consider:if (skipVerification) { // When skipping verification, only block if dependency is currently running - return dep.status !== 'running'; + return dep.status === 'completed' || dep.status === 'verified' || dep.status !== 'running'; }Or if the current behavior is intentional (allow features to start in parallel with non-running deps), please confirm this aligns with the expected auto-mode behavior described in the UI text: "as long as they are not currently running."
177-180: Clean interface addition for dependency checking options.The
DependencySatisfactionOptionsinterface is well-defined with clear JSDoc explaining theskipVerificationflag's purpose. The optional parameter design maintains backward compatibility.apps/ui/src/store/app-store.ts (3)
1712-1721: Good addition of rollback logic forsetAutoLoadClaudeMd.The rollback pattern that reverts state on sync failure is a solid improvement for maintaining consistency between UI and server state.
467-467: State field properly integrated.The
skipVerificationInAutoModefield is correctly added toAppState, initialized tofalsein initial state (line 1011), and included inpartializefor persistence (line 3000).
27-27: No action needed.ThemeModeis properly exported at lines 44-61 of app-store.ts. Line 27 only re-exportsModelAliasfrom@automaker/types; it doesn't affectThemeMode, which remains a local export used by multiple downstream files (sidebar.tsx, welcome-view.tsx, terminal-themes.ts, use-theme-preview.ts, use-project-creation.ts, and project-selector-with-options.tsx).apps/server/src/services/auto-mode-service.ts (2)
1785-1792: LGTM! Correct integration of skipVerificationInAutoMode setting.The new setting correctly replaces the sandbox verification model. The implementation properly:
- Fetches the global setting with safe null handling
- Defaults to
false(conservative, requires full verification)- Passes the option to
areDependenciesSatisfiedwhich adjusts dependency blocking behaviorWhen enabled, this allows features to proceed if dependencies aren't fully verified yet (as long as they're not currently running), which can speed up auto-mode execution.
2202-2213: LGTM! Well-implemented heartbeat logging for visibility.The heartbeat mechanism provides valuable feedback during long model initialization times. Key strengths:
- Logs every 15 seconds with elapsed time tracking
- Stops after first message arrives (line 2218) to avoid noise
- Properly cleaned up in
finallyblock (line 2735), preventing memory leaks regardless of success/error/abortThis addresses a real user experience gap where silent waits could appear frozen.
Also applies to: 2218-2218, 2735-2735
apps/ui/src/store/app-store.ts
Outdated
| setSkipVerificationInAutoMode: async (enabled) => { | ||
| set({ skipVerificationInAutoMode: enabled }); | ||
| // Sync to server settings file | ||
| const { syncSettingsToServer } = await import('@/hooks/use-settings-migration'); | ||
| await syncSettingsToServer(); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing rollback on sync failure, unlike setAutoLoadClaudeMd.
setSkipVerificationInAutoMode doesn't revert state if server sync fails, while setAutoLoadClaudeMd (lines 1713-1721) implements rollback. This inconsistency could leave UI and server out of sync.
♻️ Add rollback on sync failure for consistency
setSkipVerificationInAutoMode: async (enabled) => {
+ const previous = get().skipVerificationInAutoMode;
set({ skipVerificationInAutoMode: enabled });
// Sync to server settings file
const { syncSettingsToServer } = await import('@/hooks/use-settings-migration');
- await syncSettingsToServer();
+ const ok = await syncSettingsToServer();
+ if (!ok) {
+ logger.error('Failed to sync skipVerificationInAutoMode setting to server - reverting');
+ set({ skipVerificationInAutoMode: previous });
+ }
},📝 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.
| setSkipVerificationInAutoMode: async (enabled) => { | |
| set({ skipVerificationInAutoMode: enabled }); | |
| // Sync to server settings file | |
| const { syncSettingsToServer } = await import('@/hooks/use-settings-migration'); | |
| await syncSettingsToServer(); | |
| }, | |
| setSkipVerificationInAutoMode: async (enabled) => { | |
| const previous = get().skipVerificationInAutoMode; | |
| set({ skipVerificationInAutoMode: enabled }); | |
| // Sync to server settings file | |
| const { syncSettingsToServer } = await import('@/hooks/use-settings-migration'); | |
| const ok = await syncSettingsToServer(); | |
| if (!ok) { | |
| logger.error('Failed to sync skipVerificationInAutoMode setting to server - reverting'); | |
| set({ skipVerificationInAutoMode: previous }); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In @apps/ui/src/store/app-store.ts around lines 1578 - 1583, The setter
setSkipVerificationInAutoMode currently updates state then awaits
syncSettingsToServer without rollback on failure; mirror the pattern used in
setAutoLoadClaudeMd by wrapping the import/await of syncSettingsToServer in
try/catch and, on error, revert the store to the previous value (call set({
skipVerificationInAutoMode: previousValue })), then rethrow or handle the error
as appropriate so UI and server remain consistent.
![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 effectively removes all sandbox-related code, which aligns with the stated objective of resolving compatibility issues with downstream libraries. This cleanup simplifies the codebase and removes deprecated functionality. Additionally, a new feature, skipVerificationInAutoMode, has been introduced, providing more flexibility in how auto-mode handles feature dependencies. The changes are well-implemented and include good practices such as improved logging and state persistence across UI refreshes. The shift in getBaseOptions to bypassPermissions is a notable security posture change, justified by the autonomous mode context.
… tracking - Migrated settings persistence from localStorage to an API-first approach, ensuring consistency between Electron and web modes. - Introduced `useSettingsSync` hook for automatic synchronization of settings to the server with debouncing. - Enhanced feature update logic to track description changes with a history, allowing for better management of feature descriptions. - Updated various components and services to utilize the new settings structure and description history functionality. - Removed persist middleware from Zustand store, streamlining state management and improving performance.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
43-43: Remove unused constant.
MAX_RECENT_FOLDERSis defined but never referenced. The recent folders limit is enforced by the store (max 10 as per the store implementation), not by this component.🧹 Proposed fix
-const MAX_RECENT_FOLDERS = 5; - export function FileBrowserDialog({
🤖 Fix all issues with AI agents
In @apps/ui/src/hooks/use-settings-migration.ts:
- Around line 375-379: Do not cast localSettings to GlobalSettings; keep
finalSettings typed as Partial<GlobalSettings> (or Partial<typeof
GlobalSettings>) to avoid hiding missing fields and change the
hydrateStoreFromSettings function signature to accept Partial<GlobalSettings> so
its internal nullish-coalescing/defaulting logic is type-safe; update all call
sites (including where finalSettings is passed) to match the new signature and
remove the unsafe cast of localSettings as GlobalSettings, leaving the existing
?? defaults in hydrateStoreFromSettings to handle missing values.
🧹 Nitpick comments (12)
apps/ui/src/lib/workspace-config.ts (1)
56-111: Consider extractinglastProjectDironce at function start.The function calls
useAppStore.getState().lastProjectDirthree times (lines 71, 90, 102) across different error-handling paths. While functionally correct, extracting this value once at the beginning would reduce redundant store access.♻️ Suggested optimization
export async function getDefaultWorkspaceDirectory(): Promise<string | null> { + const lastUsedDir = useAppStore.getState().lastProjectDir; + try { const httpClient = getHttpApiClient(); const result = await httpClient.workspace.getConfig(); if (result.success) { // If ALLOWED_ROOT_DIRECTORY is configured, use it if (result.configured && result.workspaceDir) { return result.workspaceDir; } // If ALLOWED_ROOT_DIRECTORY is not set, use priority: // 1. Last used directory (from store, synced via API) // 2. Documents/Automaker // 3. DATA_DIR as fallback - const lastUsedDir = useAppStore.getState().lastProjectDir; if (lastUsedDir) { return lastUsedDir; } // Try to get Documents/Automaker const documentsPath = await getDefaultDocumentsPath(); if (documentsPath) { return documentsPath; } // Fallback to DATA_DIR if available if (result.defaultDir) { return result.defaultDir; } } // If API call failed, still try last used dir and Documents - const lastUsedDir = useAppStore.getState().lastProjectDir; if (lastUsedDir) { return lastUsedDir; } const documentsPath = await getDefaultDocumentsPath(); return documentsPath; } catch (error) { logger.error('Failed to get default workspace directory:', error); // On error, try last used dir and Documents - const lastUsedDir = useAppStore.getState().lastProjectDir; if (lastUsedDir) { return lastUsedDir; } const documentsPath = await getDefaultDocumentsPath(); return documentsPath; } }apps/ui/src/app.tsx (1)
45-49: Error logging may fire repeatedly on re-renders.The error logging at lines 47-49 runs on every render when
settingsSyncState.erroris truthy. Consider moving this to auseEffectto log only when the error state changes:♻️ Suggested improvement
+ // Log settings sync errors when they occur + useEffect(() => { + if (settingsSyncState.error) { + logger.error('Settings sync error:', settingsSyncState.error); + } + }, [settingsSyncState.error]); + // Sync settings changes back to server (API-first persistence) const settingsSyncState = useSettingsSync(); - if (settingsSyncState.error) { - logger.error('Settings sync error:', settingsSyncState.error); - }docs/settings-api-migration.md (1)
136-157: Add language specifier to fenced code blocks.The flowchart code blocks at lines 136 and 161 are missing language specifiers. While these are ASCII diagrams, adding a language hint improves rendering consistency.
♻️ Suggested fix
-``` +```text 1. App mounts └── Shows "Loading settings..." screen-``` +```text 1. User changes a setting └── Zustand store updatesapps/server/src/services/feature-loader.ts (1)
277-285: Consider tracking empty description changes.The current logic only tracks non-empty descriptions (
featureData.description.trim()). This means if a feature is created without a description, no initial history entry is added. This is likely intentional, but consider whether tracking empty→filled or filled→empty transitions would be valuable for audit purposes.apps/server/src/routes/features/routes/update.ts (1)
13-36: Consider validating parameter combinations.The route accepts
enhancementModewithout validating that it should only be provided whendescriptionHistorySourceis 'enhance'. While the feature-loader handles this correctly with conditional spreading, adding validation here would provide clearer error messages to API clients.Suggested validation logic
if (!projectPath || !featureId || !updates) { res.status(400).json({ success: false, error: 'projectPath, featureId, and updates are required', }); return; } + + // Validate parameter combinations + if (enhancementMode && descriptionHistorySource !== 'enhance') { + res.status(400).json({ + success: false, + error: 'enhancementMode can only be provided when descriptionHistorySource is "enhance"', + }); + return; + }apps/ui/src/hooks/use-settings-sync.ts (3)
97-139: Consider extracting the update-building logic to reduce duplication.The logic to build the updates object (iterating over
SETTINGS_FIELDS_TO_SYNCandSETUP_FIELDS_TO_SYNCwith special handling forcurrentProjectId) is duplicated insyncToServer,forceSyncSettingsToServer, and during initialization. This same pattern also exists inuse-settings-migration.tsviabuildSettingsUpdateFromStore.♻️ Suggested refactor
Extract a shared helper function:
function buildUpdatesFromStores(): Record<string, unknown> { const appState = useAppStore.getState(); const setupState = useSetupStore.getState(); const updates: Record<string, unknown> = {}; for (const field of SETTINGS_FIELDS_TO_SYNC) { if (field === 'currentProjectId') { updates[field] = appState.currentProject?.id ?? null; } else { updates[field] = appState[field as keyof typeof appState]; } } for (const field of SETUP_FIELDS_TO_SYNC) { updates[field] = setupState[field as keyof typeof setupState]; } return updates; }Then use it in all locations.
269-291: Best-effort sync on unload may not complete.The
beforeunloadhandler callssyncNow()which starts an async operation. As noted in the comment, this may not complete in all browsers. For critical data, consider usingnavigator.sendBeaconas a fallback for the sync payload.♻️ Suggested improvement for more reliable unload sync
const handleBeforeUnload = () => { // Fire-and-forget; may not complete in all browsers, but helps in Electron/webview syncNow(); + + // Use sendBeacon as a more reliable fallback for critical data + if (navigator.sendBeacon) { + const appState = useAppStore.getState(); + const updates = { currentProjectId: appState.currentProject?.id ?? null }; + navigator.sendBeacon('/api/settings/global', JSON.stringify(updates)); + } };Note: Adjust the endpoint path to match your API structure.
339-340: Type casting throughunknownbypasses type safety.The double cast
as unknown as GlobalSettingsindicates a type mismatch between the API response and the expected type. This could mask issues if the API shape changes.Consider adding a type guard or using a proper type definition for the API response to ensure type safety:
// Option 1: Add a type assertion function function isGlobalSettings(obj: unknown): obj is GlobalSettings { return obj !== null && typeof obj === 'object' && 'version' in obj; } // Option 2: Update the API response type to be properly typedapps/ui/src/hooks/use-settings-migration.ts (2)
174-202: Merge decision logic only checks two array types.The
localStorageHasMoreDatafunction only checksprojectsandaiProfilesto determine if localStorage has more data. Other array fields likemcpServers,trashedProjects, orrecentFoldersare not considered. This is acceptable if intentional (these are the most important arrays), but could be documented.Consider adding a comment explaining why only these two arrays are checked:
+/** + * Check if localStorage has more complete data than server + * Returns true if localStorage has projects or AI profiles but server doesn't + * Note: Only checks projects and aiProfiles as these are the most critical user data + */ function localStorageHasMoreData(
513-552: Code duplication with use-settings-sync.ts.The
buildSettingsUpdateFromStorefunction duplicates the same logic found inuse-settings-sync.ts(building updates from app and setup stores). Consider exporting this from one location and importing in the other, or moving to a shared utilities file.apps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (2)
350-358: Clarify the edit-tracking behavior.The current logic only sets source to
'edit'if there's no existing source or it's already'edit'. This means:
- If user types → source = 'edit'
- If user enhances → source = {source: 'enhance', mode: ...}
- If user types after enhance → source stays as enhance
This preserves the "most significant" change, but the comment could be clearer:
📝 Suggested comment improvement
- // Track that this change was a manual edit (unless already enhanced) - if (!descriptionChangeSource || descriptionChangeSource === 'edit') { - setDescriptionChangeSource('edit'); - } + // Only track as manual edit if not already enhanced. + // If user enhanced then made minor tweaks, we still consider it + // an enhancement for history purposes. + if (!descriptionChangeSource || descriptionChangeSource === 'edit') { + setDescriptionChangeSource('edit'); + }
460-511: Improve list key and current-version detection.Two concerns with the history list rendering:
Key using index: The key
${entry.timestamp}-${index}uses index which can cause issues if entries are reordered. Since timestamps should be unique, just use the timestamp.Current version detection: Comparing
entry.description === editingFeature.descriptionmay incorrectly highlight multiple entries if the same text appears twice in history.♻️ Suggested fixes
- .map((entry: DescriptionHistoryEntry, index: number) => { - const isCurrentVersion = entry.description === editingFeature.description; + .map((entry: DescriptionHistoryEntry) => { + // More robust current version check: match the most recent entry + // with the same description (first match in reversed array) + const reversedHistory = [...(feature.descriptionHistory || [])].reverse(); + const latestMatchIndex = reversedHistory.findIndex( + (e) => e.description === editingFeature.description + ); + const isCurrentVersion = + latestMatchIndex !== -1 && + entry.timestamp === reversedHistory[latestMatchIndex].timestamp; // ... rest of component - <button - key={`${entry.timestamp}-${index}`} + <button + key={entry.timestamp}Or simpler - just use timestamp as key and accept the minor UX imperfection:
- key={`${entry.timestamp}-${index}`} + key={entry.timestamp}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/server/src/routes/features/routes/update.tsapps/server/src/services/feature-loader.tsapps/server/src/services/settings-service.tsapps/ui/src/app.tsxapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/hooks/use-board-persistence.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/workspace-config.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/setup-store.tsdocs/settings-api-migration.mdlibs/types/src/feature.tslibs/types/src/index.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/src/routes/__root.tsx
- apps/ui/src/lib/http-api-client.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/board-view/hooks/use-board-persistence.tsapps/ui/src/app.tsxlibs/types/src/feature.tsapps/ui/src/lib/workspace-config.tsapps/ui/src/lib/electron.tslibs/types/src/index.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/store/setup-store.tsapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/server/src/routes/features/routes/update.tsapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/server/src/services/settings-service.tsapps/server/src/services/feature-loader.tslibs/types/src/settings.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/board-view/hooks/use-board-persistence.tsapps/ui/src/app.tsxlibs/types/src/feature.tsapps/ui/src/lib/workspace-config.tsapps/ui/src/lib/electron.tslibs/types/src/index.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/store/setup-store.tsapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/server/src/routes/features/routes/update.tsapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/server/src/services/settings-service.tsapps/server/src/services/feature-loader.tslibs/types/src/settings.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/features/routes/update.tsapps/server/src/services/settings-service.tsapps/server/src/services/feature-loader.ts
🧠 Learnings (2)
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/app.tsxapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsxapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts
🧬 Code graph analysis (8)
apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (2)
libs/types/src/feature.ts (1)
Feature(34-69)apps/ui/src/store/app-store.ts (1)
Feature(277-293)
libs/types/src/feature.ts (1)
libs/types/src/index.ts (1)
DescriptionHistoryEntry(28-28)
apps/server/src/routes/features/routes/update.ts (1)
libs/types/src/feature.ts (1)
Feature(34-69)
apps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (3)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)libs/types/src/feature.ts (1)
DescriptionHistoryEntry(10-15)libs/types/src/index.ts (1)
DescriptionHistoryEntry(28-28)
apps/ui/src/hooks/use-settings-migration.ts (4)
libs/utils/src/index.ts (1)
createLogger(44-44)libs/types/src/settings.ts (1)
GlobalSettings(386-497)apps/ui/src/lib/storage.ts (2)
getItem(21-28)removeItem(51-59)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(2029-2034)
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
apps/ui/src/store/app-store.ts (1)
useAppStore(1078-2901)
apps/server/src/services/settings-service.ts (1)
libs/types/src/settings.ts (1)
GlobalSettings(386-497)
libs/types/src/settings.ts (2)
libs/types/src/index.ts (1)
SETTINGS_VERSION(102-102)apps/server/src/types/settings.ts (1)
SETTINGS_VERSION(36-36)
🪛 LanguageTool
docs/settings-api-migration.md
[grammar] ~177-~177: Ensure spelling is correct
Context: ...2. Server missing arrays (projects, aiProfiles, etc.) → Use localStorage arrays 3. **S...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/settings-api-migration.md
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (38)
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (2)
1-5: LGTM! Import changes align with the store-based state migration.The removal of
useStateand addition ofuseAppStorecorrectly reflects the shift from local state to centralized store management.
86-90: No issues found. The collapse state management is correctly implemented with proper store initialization and API sync.The
worktreePanelCollapsedfield has a default value offalsein the store, preventing undefined behavior on first render. The comment about API sync is accurate—this field is included in theSETTINGS_TO_SYNCarray and synced via theuseSettingsSynchook at the app root level. The store selectors and toggle logic are correctly implemented.apps/ui/src/lib/workspace-config.ts (1)
117-119: LGTM! Store-based persistence correctly implemented.The function properly uses the store action for persistence, maintaining the same public API while migrating from localStorage to store-backed state.
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
62-74: LGTM! Store integration correctly implemented.The migration from localStorage to app-store state is well implemented. The component correctly uses store hooks for reading and updating recent folders, with proper dependency management in the
handleRemoveRecentcallback.apps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsx (2)
359-366: LGTM! Popover positioning improvements applied consistently.The changes to align the popovers to "start" and add collision padding improve the UX by ensuring the secondary popovers don't overflow the viewport. The changes are applied consistently across both Claude model thinking level selection and grouped model variant selection popovers.
However, note that the AI summary claims these changes were made in "three occurrences" but only two PopoverContent instances show modifications. The third PopoverContent at lines 567-572 remains unchanged.
Also applies to: 475-482
19-24: This import cannot be entirely replaced with@automaker/typesas suggested. WhileCLAUDE_MODELS,THINKING_LEVELS, andTHINKING_LEVEL_LABELSexist in@automaker/types,CURSOR_MODELSis not exported from that package—it's locally generated fromCURSOR_MODEL_MAP. Additionally, the localTHINKING_LEVELStype differs from the@automaker/typesversion. The current local definition is appropriate for this use case.Likely an incorrect or invalid review comment.
apps/ui/src/store/setup-store.ts (2)
1-2: Clear documentation of the architectural change.The comment on line 2 properly documents why persist middleware was removed and where settings now sync. This helps future maintainers understand the API-first persistence model.
147-195: Store implementation looks good.The zustand store is properly structured with typed state and actions. The
resetSetupaction correctly preservesisFirstRun: falseto prevent re-triggering the first-run experience after a reset.apps/server/src/services/settings-service.ts (3)
165-173: Migration logic correctly handles existing installations.The v3→v4 migration appropriately defaults
setupCompletetotruefor existing users, preventing them from being forced through the onboarding wizard again. The undefined checks ensure user-configured values are preserved.
528-537: localStorage parsing follows established patterns.The parsing of
automaker-setupmirrors the existingautomaker-storageparsing, with consistent error handling that logs issues but allows migration to continue.
540-547: Verify default behavior consistency between migration paths.The localStorage migration defaults
setupCompletetofalsewhen absent (line 542), while the v3→v4 file migration defaults it totrue(line 169). This appears intentional—localStorage migration handles fresh/partial data where setup may not have completed, whereas file migration assumes existing users have already completed setup.Please confirm this asymmetry is intentional for your migration scenarios.
libs/types/src/settings.ts (3)
390-396: Well-documented onboarding fields.The new onboarding/setup wizard fields are properly typed with clear JSDoc documentation explaining their purpose.
417-418: Clear documentation of auto-mode verification behavior.The
skipVerificationInAutoModefield is well-documented, clearly explaining that it treats 'completed' the same as 'verified' in auto-mode workflows.
652-689: Sensible defaults for new installations.The default values correctly configure new installations to show the setup wizard (
setupComplete: false,isFirstRun: true) while keeping safety-oriented defaults for verification (skipVerificationInAutoMode: false).libs/types/src/index.ts (1)
23-29: Type export properly added.The
DescriptionHistoryEntrytype is correctly exported alongside related feature types, making it available to consumers of@automaker/types.libs/types/src/feature.ts (2)
7-15: Well-designed history tracking type.The
DescriptionHistoryEntryinterface is well-structured with appropriate union types forsourceandenhancementMode. The comment clarifying thatenhancementModeonly applies to 'enhance' source is helpful.
67-67: Feature interface properly extended.The optional
descriptionHistoryfield correctly follows the existing pattern of optional arrays in the Feature interface.apps/ui/src/app.tsx (1)
59-67: Proper initialization guard prevents race conditions.The loading guard ensures
currentProjectand other settings are hydrated before the router renders, preventing the race condition mentioned in the comments. Good UX pattern.docs/settings-api-migration.md (1)
1-219: Comprehensive migration documentation.This documentation thoroughly covers the API-first migration, including rationale, affected files, data flow diagrams, and exported functions. This will be valuable for future maintainers understanding the settings architecture.
Note: The LanguageTool warning about "aiProfiles" is a false positive—it's a valid camelCase identifier.
apps/ui/src/lib/electron.ts (1)
459-465: LGTM! Clean API extension for description history tracking.The addition of optional
descriptionHistorySourceandenhancementModeparameters to theupdatemethod maintains backward compatibility while enabling rich description change tracking throughout the system.apps/server/src/services/feature-loader.ts (2)
341-346: LGTM! Proper conditional inclusion of enhancementMode.The conditional spread syntax correctly includes
enhancementModeonly when the source is 'enhance', maintaining clean data structure.
334-349: Document the intended behavior for clearing descriptions.The condition
updates.description.trim()on line 339 prevents tracking when descriptions are cleared (set to empty string), but the description is still applied to the feature. This creates an inconsistency: changes to non-empty descriptions are tracked in history, but clearing descriptions silently skips history tracking. Clarify whether this is intentional—if clearing descriptions should be audited, add a separate history entry; if not, document this design decision to explain why clearing operations are excluded from the history log.apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (1)
17-48: LGTM! Clean parameter forwarding to API.The extended signature properly forwards the description history parameters through to the API. While there's no client-side validation that
enhancementModeshould only accompanydescriptionHistorySource: 'enhance', this type of business logic validation is appropriately handled on the server side.apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (4)
26-31: LGTM! Proper signature extension.The
persistFeatureUpdatesignature correctly extends to support description history tracking with optional parameters that maintain backward compatibility.
229-231: LGTM! History parameters properly threaded through update flow.The
handleUpdateFeaturefunction correctly accepts and forwards the description history parameters topersistFeatureUpdate. The implementation maintains consistency with the extended API.Also applies to: 275-275
816-823: Verify dependency blocking bypass behavior is intentional.The
skipVerificationInAutoModeflag completely bypasses dependency blocking when determining eligible features to start. This is a significant behavior change that allows features with unmet dependencies to run in auto mode.Ensure this aligns with product requirements:
- When
skipVerificationInAutoModeis true, features with blocking dependencies are treated as unblocked (lines 817-822)- Features with unmet dependencies become eligible to start (line 835)
This could lead to features running before their dependencies are verified, potentially causing integration issues.
Based on learnings: Each feature executes in an isolated git worktree, so running features with unmet dependencies may still be safe from a code conflict perspective, but logical/functional dependencies won't be enforced.
Also applies to: 835-836
859-859: LGTM! Correct dependency tracking.The
skipVerificationInAutoModeflag is properly included in the dependency array, ensuring the callback updates when this setting changes.apps/ui/src/hooks/use-settings-sync.ts (3)
1-22: LGTM - Well-documented module with correct imports.The header comments clearly explain the hook's purpose, the migration coordination pattern, and the server as source of truth. Imports correctly use shared packages (
@automaker/utils/logger,@automaker/types).
28-63: LGTM - Comprehensive field list for sync.The fields cover the necessary app state for persistence. The
as constassertion provides type safety for the field names.
342-381: LGTM - Comprehensive state refresh with sensible defaults.The
refreshSettingsFromServerfunction properly:
- Applies defaults with nullish coalescing
- Deep-merges
keyboardShortcutsto preserve local customizations- Updates both app and setup stores
apps/ui/src/hooks/use-settings-migration.ts (4)
99-168: Robust localStorage parsing with comprehensive field mapping.The parsing logic:
- Handles both Zustand persist format (
statekey) and direct format- Merges standalone localStorage keys (worktree, folders, lastProjectDir)
- Wraps everything in try/catch for resilience
436-508: LGTM - Comprehensive hydration with safe defaults.The
hydrateStoreFromSettingsfunction properly:
- Converts
ProjectRef[]to minimalProject[]with empty features (to be loaded separately)- Restores
currentProjectby ID lookup- Uses sensible defaults via nullish coalescing
- Deep-merges
keyboardShortcutsto preserve existing bindings- Updates both app and setup stores
562-572: LGTM - Clean sync function using the shared helper.Good that it reuses
buildSettingsUpdateFromStoreinternally.
66-94: Well-designed coordination pattern for async initialization.The global promise pattern with
migrationCompletedflag correctly handles:
- Early resolution if migration completes before anything waits
- Lazy promise creation to avoid unnecessary allocations
- Idempotent design that safely prevents re-execution after module hot-reloads in development
The module-level state persists across HMR reloads, and the
migrationCompletedflag ensures that even if the component re-mounts, the migration won't re-run unnecessarily.apps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (4)
59-60: LGTM - Correct imports from shared packages.Using
@automaker/typesforDescriptionHistoryEntryfollows the coding guidelines.
84-87: LGTM - Clean API extension for history tracking.The optional parameters
descriptionHistorySourceandenhancementModeallow gradual adoption without breaking existing callers.
129-136: Intentional behavior: edits after enhancement preserve the enhancement source.The state type allows tracking both enhancement (with mode) and edit sources. Note that if a user enhances, then makes a small manual edit, the source remains "enhance" based on the logic on lines 356-358. This is intentional per the comment "unless already enhanced" but may be surprising to users.
442-515: LGTM - Well-implemented history popover UI.The history feature provides good UX:
- Only shows when history exists
- Displays source labels (Original/Enhanced/Edited) with enhancement mode
- Shows formatted timestamps and description snippets
- Indicates current version
- Restores with toast feedback
| } else if (localSettings) { | ||
| // No server settings, use localStorage | ||
| finalSettings = localSettings as GlobalSettings; | ||
| needsSync = true; | ||
| logger.info('Using localStorage settings (no server settings found)'); |
There was a problem hiding this comment.
Unsafe cast of Partial type to full type.
Casting localSettings as GlobalSettings when localSettings is Partial<GlobalSettings> could lead to undefined values for required fields. While hydrateStoreFromSettings uses defaults (?? operators), TypeScript won't catch missing required fields at compile time.
Consider keeping the type as Partial and updating hydrateStoreFromSettings signature:
-function hydrateStoreFromSettings(settings: GlobalSettings): void {
+function hydrateStoreFromSettings(settings: Partial<GlobalSettings>): void {This maintains type honesty and the existing nullish coalescing defaults already handle missing values.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/ui/src/hooks/use-settings-migration.ts around lines 375 - 379, Do not
cast localSettings to GlobalSettings; keep finalSettings typed as
Partial<GlobalSettings> (or Partial<typeof GlobalSettings>) to avoid hiding
missing fields and change the hydrateStoreFromSettings function signature to
accept Partial<GlobalSettings> so its internal nullish-coalescing/defaulting
logic is type-safe; update all call sites (including where finalSettings is
passed) to match the new signature and remove the unsafe cast of localSettings
as GlobalSettings, leaving the existing ?? defaults in hydrateStoreFromSettings
to handle missing values.
- Implemented session verification on component mount using exponential backoff to handle server live reload scenarios. - Added loading state to the login view while checking for an existing session, improving user experience. - Removed unused setup wizard navigation from the API keys section for cleaner code.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/ui/src/components/views/login-view.tsx:
- Around line 36-71: verifySession currently swallows network errors and returns
false, preventing the exponential backoff in the login-view useEffect from ever
running; change verifySession so that on non-auth-related/network failures it
throws the caught error (retain returning false only for definitive "invalid
session" responses), and in the login-view useEffect keep the loop logic that
treats a returned false as a definitive invalid session (break) but allows
thrown errors to be caught by the catch block to trigger the exponential backoff
and retries; ensure setIsCheckingSession(false) and setAuthState are still set
appropriately after the loop and that navigate({ to: setupComplete ? '/' :
'/setup' }) only runs when a valid session is confirmed.
🧹 Nitpick comments (1)
apps/ui/src/components/views/login-view.tsx (1)
33-34: Improve strict mode guard for session check.The
sessionCheckRefpattern (lines 39-40) has a subtle race: in React 18+ Strict Mode, both effect runs can pass the check at line 39 before either sets line 40 totrue. While the worst case is a duplicateverifySessioncall (which is harmless), a more robust pattern would use a cleanup function to reset the ref or check-and-set atomically.♻️ Suggested improvement
useEffect(() => { - // Prevent duplicate checks in strict mode - if (sessionCheckRef.current) return; - sessionCheckRef.current = true; + if (sessionCheckRef.current) return; + sessionCheckRef.current = true; const checkExistingSession = async () => { // ... existing logic }; checkExistingSession(); + + // Cleanup: allow re-check if component remounts + return () => { + sessionCheckRef.current = false; + }; }, [navigate, setAuthState, setupComplete]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx
🧬 Code graph analysis (2)
apps/ui/src/components/views/login-view.tsx (2)
apps/ui/src/store/setup-store.ts (1)
useSetupStore(147-195)apps/ui/src/lib/http-api-client.ts (1)
verifySession(335-380)
apps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx (1)
apps/ui/src/store/setup-store.ts (1)
useSetupStore(147-195)
⏰ 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 (6)
apps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx (3)
4-4: Clean removal of unused Settings icon.The import statement correctly includes only the icons used in the component after removing the setup wizard navigation.
16-16: Correctly simplified setup store usage.The component now only extracts the Claude auth status fields needed for API key deletion, removing the unused
setSetupCompletereference. The remaining fields are properly used in thedeleteAnthropicKeyfunction.
1-12: The imports in this file are correctly structured. The coding guideline requires using shared packages (@automaker/*) when they exist and avoiding old deep relative paths (like../../../). However, the imports here for stores, config, UI components, and app utilities have no@automaker/*equivalents in the shared libs—they are app-internal modules that belong inapps/ui. The use of the@/alias for app-internal paths and local./imports for sibling components follows the correct pattern.apps/ui/src/components/views/login-view.tsx (3)
21-24: Delay helper is correct but currently unused.The implementation is clean and idiomatic. However, due to the retry logic issue (see comment on lines 36-71), this function is never actually invoked.
96-106: LGTM: Clean loading state UI.The interim loading screen is well-implemented with appropriate feedback for users. The use of
Loader2with animation and centered layout is consistent with the app's design patterns.
12-14: LGTM: Imports are correct.The new imports (
useEffect,useRef,verifySession) are properly structured and necessary for the session verification feature.
- Introduced `SandboxRiskDialog` to prompt users about risks when running outside a containerized environment. - Added `SandboxRejectionScreen` for users who deny the sandbox risk confirmation, providing options to reload or restart the app. - Updated settings view and danger zone section to manage sandbox warning preferences. - Implemented a new API endpoint to check if the application is running in a containerized environment. - Enhanced state management to handle sandbox warning settings across the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/ui/src/styles/global.css (1)
370-379: Consider removing the redundant::-moz-selectionrule.The text selection styling looks good and will improve readability. However, the
::-moz-selectionpseudo-element is no longer necessary, as Firefox has supported the standard::selectionpseudo-element since version 62 (2018).♻️ Simplified implementation
/* Text selection styling for readability */ ::selection { background-color: var(--primary); color: var(--primary-foreground); } - -::-moz-selection { - background-color: var(--primary); - color: var(--primary-foreground); -}apps/ui/src/routes/__root.tsx (1)
45-48: Consider removing the now-redundantsetupHydratedstate.Since
setupHydratedis alwaystrueand never changes, the state variable is no longer necessary. You could replace all usages withtruedirectly or remove the guard checks entirely.♻️ Suggested simplification
- // Since we removed persist middleware (settings now sync via API), - // we consider the store "hydrated" immediately - the useSettingsMigration - // hook in App.tsx handles loading settings from the API - const [setupHydrated, setSetupHydrated] = useState(true); + // Note: Settings now sync via API - useSettingsMigration in App.tsx handles loadingThen remove the
setupHydratedguard in the routing effect (line 233) and the dependency (line 259).apps/ui/src/hooks/use-settings-migration.ts (2)
66-94: Module-level state may cause issues during testing or HMR.The global variables (
migrationCompleted,migrationCompleteResolve,migrationCompletePromise) persist across module reloads. This could cause stale state in hot module replacement or test environments where the module is re-imported.Consider adding a reset function for testing
+/** + * Reset migration state (for testing purposes only) + */ +export function __resetMigrationState(): void { + migrationCompleteResolve = null; + migrationCompletePromise = null; + migrationCompleted = false; +}
160-163: Wrap nestedJSON.parsein try-catch.If
recentFolderscontains malformed JSON, this will throw and terminate the entireparseLocalStorageSettingsfunction, losing all other valid settings.Proposed fix
- recentFolders: recentFolders ? JSON.parse(recentFolders) : (state.recentFolders as string[]), + recentFolders: recentFolders ? safeJsonParse(recentFolders, state.recentFolders as string[]) : (state.recentFolders as string[]),Add a helper at the top of the function:
function safeJsonParse<T>(json: string, fallback: T): T { try { return JSON.parse(json) as T; } catch { return fallback; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/ui/src/components/dialogs/sandbox-rejection-screen.tsxapps/ui/src/components/dialogs/sandbox-risk-dialog.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/styles/global.csslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/src/lib/http-api-client.ts
- apps/ui/src/components/dialogs/sandbox-rejection-screen.tsx
- apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view.tsxlibs/types/src/settings.tsapps/ui/src/routes/__root.tsxapps/ui/src/hooks/use-settings-migration.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view.tsxlibs/types/src/settings.tsapps/ui/src/routes/__root.tsxapps/ui/src/hooks/use-settings-migration.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: webdevcody
Repo: AutoMaker-Org/automaker PR: 378
File: apps/server/src/lib/sdk-options.ts:141-148
Timestamp: 2026-01-07T15:20:53.127Z
Learning: AutoMaker intentionally uses `permissionMode: 'bypassPermissions'` with `allowDangerouslySkipPermissions: true` in base SDK options for fully autonomous operation. This is an accepted architectural decision, with Docker containerization as the primary security boundary.
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/settings-view.tsxapps/ui/src/routes/__root.tsx
🧬 Code graph analysis (2)
libs/types/src/settings.ts (2)
libs/types/src/index.ts (1)
SETTINGS_VERSION(102-102)apps/server/src/types/settings.ts (1)
SETTINGS_VERSION(36-36)
apps/ui/src/routes/__root.tsx (4)
apps/ui/src/components/dialogs/sandbox-rejection-screen.tsx (1)
SandboxRejectionScreen(11-53)apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx (1)
SandboxRiskDialog(28-108)apps/ui/src/lib/electron.ts (1)
isElectron(789-799)apps/ui/src/lib/utils.ts (1)
isMac(60-65)
⏰ 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 (13)
apps/ui/src/routes/__root.tsx (6)
319-322: LGTM!Clean early return pattern for the sandbox rejection screen. The logic correctly blocks all other rendering when the user has denied the sandbox risk warning.
324-326: LGTM!Good use of a derived variable for the dialog visibility state, improving readability across multiple render paths.
329-342: LGTM!The login route correctly renders the
SandboxRiskDialogas an overlay, ensuring users see the sandbox warning even on the login page. The React Fragment wrapper is appropriate for returning multiple siblings.
378-384: LGTM!Good implementation of the Electron titlebar drag region. The
pointer-events-noneensures it doesn't interfere with underlying UI interactions, and the conditionalpl-20padding on Mac correctly accounts for the traffic light buttons.
393-398: Verify: Empty streamer panel appears intentional but has no visible content.The streamer panel div reserves space (pushes content via
marginRight) but renders as an empty container. If this is a placeholder for future content, consider adding a TODO comment. If it should display content, the implementation appears incomplete.
375-407: LGTM on the overall layout structure.The refactored layout correctly:
- Renders
SandboxRiskDialogoutside<main>for proper overlay behavior- Uses a Fragment to return multiple siblings
- Consolidates the sidebar and content area within the main flex container
apps/ui/src/components/views/settings-view.tsx (2)
34-36: LGTM! Clean integration ofskipVerificationInAutoModesetting.The new setting is properly destructured from the store and correctly wired to
FeatureDefaultsSectionwith both the value prop and change handler.Also applies to: 133-133, 142-142
55-56: No action required.skipSandboxWarningremains in the type definitions (line 490 of settings.ts) as an optional field, is fully integrated in the app store with initialization and sync logic, and is correctly used in settings-view.tsx at lines 55-56 and 154-156. The field was not removed.Likely an incorrect or invalid review comment.
apps/ui/src/hooks/use-settings-migration.ts (2)
439-509: Thorough hydration implementation with good defaults.The function correctly handles missing values with nullish coalescing and populates both
useAppStoreanduseSetupStore. The type assertion on line 462 forThemeModeis acceptable since the value comes from persisted settings.
514-554:buildSettingsUpdateFromStorecorrectly mirrors hydration fields.The serialization includes all settings that are hydrated, including the new
skipVerificationInAutoModeandskipSandboxWarningfields. Good symmetry withhydrateStoreFromSettings.libs/types/src/settings.ts (3)
390-396: Well-documented additions toGlobalSettings.The new fields are properly typed and documented:
- Onboarding fields (
setupComplete,isFirstRun,skipClaudeSetup) support setup wizard state persistenceskipVerificationInAutoModeenables auto-mode workflow customizationcurrentProjectIdenables project restoration across sessionsAlso applies to: 417-418, 463-464
654-691: Default values are consistent with type definitions.All new fields have appropriate defaults in
DEFAULT_GLOBAL_SETTINGS:
setupComplete: false- users start with incomplete setupisFirstRun: true- enables first-run experienceskipClaudeSetup: false- don't skip by defaultskipVerificationInAutoMode: false- safer default requiring verificationcurrentProjectId: null- no project selected initially
623-623: Migration logic for SETTINGS_VERSION 3 → 4 exists and properly handles schema changes.Verified in
apps/server/src/services/settings-service.ts(lines 165-173): When loading settings with version < 4, the code adds three new fields with sensible defaults—setupComplete: true,isFirstRun: false, andskipClaudeSetup: false—to avoid forcing re-onboarding for existing installs. The migrated settings are persisted to disk with the updated version number.
- Changed SameSite attribute for session cookies from 'strict' to 'lax' to allow cross-origin fetches, improving compatibility with various client requests. - Updated cookie clearing logic in the authentication route to use `res.cookie()` for better reliability in cross-origin environments. - Refactored the login view to implement a state machine for managing authentication phases, enhancing clarity and maintainability. - Introduced a new logged-out view to inform users of session expiration and provide options to log in or retry. - Added account and security sections to the settings view, allowing users to manage their account and security preferences more effectively.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/routes/auth/index.ts (1)
10-10: Update the outdated documentation comment.Line 10 states "SameSite=Strict" but the implementation now uses
sameSite: 'lax'(line 238). This inconsistency between documentation and code can mislead future developers.📝 Proposed fix
- * - SameSite=Strict: Only sent for same-site requests (protects against CSRF) + * - SameSite=Lax: Sent for same-site requests and top-level navigation (balances CSRF protection with cross-origin usability)
🤖 Fix all issues with AI agents
In @apps/server/src/lib/auth.ts:
- Line 265: The inline comment next to the cookie option sameSite: 'lax' is
inaccurate about when cookies are sent; update the comment for the sameSite
property (the sameSite: 'lax' setting) to state that 'lax' sends cookies for
same-site requests and top-level navigations (e.g., clicking links) but does NOT
send cookies for cross-origin fetch/XHR requests or embedded resources like
iframes/images; replace the existing comment with this corrected, concise
description.
In @apps/server/src/routes/auth/index.ts:
- Around line 232-242: The logout route is duplicating cookie options instead of
reusing getSessionCookieOptions(), risking inconsistency; replace the manually
specified options in the res.cookie(cookieName, '', {...}) call with the result
of getSessionCookieOptions() (e.g., res.cookie(cookieName, '', {
...getSessionCookieOptions(), maxAge: 0, expires: new Date(0) }) or equivalent)
so cookieName and session cookie config remain centralized in
getSessionCookieOptions(); ensure any logout-specific overrides (empty value,
immediate expiration) are applied on top of the shared options.
In @apps/ui/src/app.tsx:
- Line 6: The import LoadingState in app.tsx is unused; remove the named import
LoadingState from the import statement (or delete the whole import if it becomes
empty) so there are no unused imports, and run the linter/TS compile to confirm
no remaining references to LoadingState exist.
In @apps/ui/src/components/views/login-view.tsx:
- Around line 224-229: The useEffect starting with initialCheckDone runs
checkServerAndSession which may dispatch after the component unmounts; modify
the effect to create an AbortController (or a mounted flag), pass its signal to
checkServerAndSession(dispatch, setAuthState, signal), and in the effect cleanup
call controller.abort() (or flip mounted to false) to prevent post-unmount
dispatches; then update checkServerAndSession to accept an optional AbortSignal
parameter and check signal.aborted (or the mounted flag) before any
dispatch/setAuthState calls and return early if aborted.
🧹 Nitpick comments (6)
apps/ui/src/components/views/settings-view/account/account-section.tsx (1)
13-25: Use the logger utility for consistent error handling.The error handling uses
console.errorinstead of the logger utility that's used elsewhere in the codebase. For consistency and better observability, consider using the logger.♻️ Proposed refactor
Add the logger import at the top of the file:
+import { createLogger } from '@automaker/utils/logger'; import { useState } from 'react'; import { useNavigate } from '@tanstack/react-router';Then update the error handling:
+const logger = createLogger('AccountSection'); + export function AccountSection() { const navigate = useNavigate(); const [isLoggingOut, setIsLoggingOut] = useState(false); const handleLogout = async () => { setIsLoggingOut(true); try { await logout(); // Reset auth state useAuthStore.getState().resetAuth(); // Navigate to logged out page navigate({ to: '/logged-out' }); } catch (error) { - console.error('Logout failed:', error); + logger.error('Logout failed:', error); setIsLoggingOut(false); } };apps/ui/src/components/views/settings-view/components/settings-navigation.tsx (2)
7-12: Remove unusednavItemsprop from interface.The
navItemsprop is declared inSettingsNavigationPropsbut is never used in the component—navigation items are now sourced fromGLOBAL_NAV_ITEMSandPROJECT_NAV_ITEMS. This dead prop should be removed to avoid confusion.♻️ Proposed fix
interface SettingsNavigationProps { - navItems: NavigationItem[]; activeSection: SettingsViewId; currentProject: Project | null; onNavigate: (sectionId: SettingsViewId) => void; }
24-27: Remove redundantkeyprop from button element.The
keyprop on the<button>element (line 26) has no effect sincekeyis a React-specific prop that only applies at the list iteration level. The key is already correctly passed to<NavButton key={item.id} ...>in the parent mapping.♻️ Proposed fix
return ( <button - key={item.id} onClick={() => onNavigate(item.id)} className={cn(apps/ui/src/hooks/use-settings-sync.ts (1)
270-292:beforeunloadhandler cannot reliably await async operations.The
handleBeforeUnloadcallback callssyncNow(), which is async. However,beforeunloadevent handlers cannot await async operations—the browser may terminate before the sync completes. While the comment acknowledges this ("Fire-and-forget; may not complete"), consider usingnavigator.sendBeacon()for more reliable last-chance sync in web mode.♻️ Alternative using sendBeacon for critical data
const handleBeforeUnload = () => { // Use sendBeacon for more reliable delivery on page unload const serverUrl = getServerUrlSync?.() ?? ''; if (serverUrl && navigator.sendBeacon) { const updates = buildUpdatesObject(); // Extract to reusable function navigator.sendBeacon( `${serverUrl}/api/settings/global`, JSON.stringify(updates) ); } else { // Fallback fire-and-forget syncNow(); } };apps/ui/src/components/views/login-view.tsx (1)
316-318: Redundant ternary expression.Line 317 has a ternary where both branches return the same value. Since
state.apiKeyexists on bothawaiting_loginandlogging_inphases, this can be simplified.♻️ Proposed fix
const isLoggingIn = state.phase === 'logging_in'; - const apiKey = state.phase === 'awaiting_login' ? state.apiKey : state.apiKey; + const apiKey = state.apiKey; const error = state.phase === 'awaiting_login' ? state.error : null;apps/ui/src/routes/__root.tsx (1)
253-260: Consider aligning API response types withGlobalSettings.The double cast through
unknownworks but indicates a type mismatch between the API response andGlobalSettings. This could be improved by ensuring the API client returns properly typed responses, which would provide better type safety.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/server/src/lib/auth.tsapps/server/src/routes/auth/index.tsapps/ui/src/app.tsxapps/ui/src/components/views/logged-out-view.tsxapps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/account/account-section.tsxapps/ui/src/components/views/settings-view/account/index.tsapps/ui/src/components/views/settings-view/components/settings-navigation.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/security/index.tsapps/ui/src/components/views/settings-view/security/security-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxapps/ui/src/routes/logged-out.tsxapps/ui/src/store/app-store.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view/account/index.tsapps/ui/src/components/views/logged-out-view.tsxapps/server/src/lib/auth.tsapps/ui/src/routes/logged-out.tsxapps/server/src/routes/auth/index.tsapps/ui/src/components/views/settings-view/security/security-section.tsxapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/app.tsxapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/ui/src/components/views/settings-view/security/index.tsapps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/routes/__root.tsxapps/ui/src/components/views/settings-view/account/account-section.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/settings-view.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/components/views/settings-view/components/settings-navigation.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view/account/index.tsapps/ui/src/components/views/logged-out-view.tsxapps/server/src/lib/auth.tsapps/ui/src/routes/logged-out.tsxapps/server/src/routes/auth/index.tsapps/ui/src/components/views/settings-view/security/security-section.tsxapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/app.tsxapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/ui/src/components/views/settings-view/security/index.tsapps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/routes/__root.tsxapps/ui/src/components/views/settings-view/account/account-section.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/settings-view.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/components/views/settings-view/components/settings-navigation.tsx
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/lib/auth.tsapps/server/src/routes/auth/index.ts
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Frontend UI must use TanStack Router for file-based routing, organize components in components/views/, implement stores with Zustand, and use custom hooks in hooks/ directory
Applied to files:
apps/ui/src/components/views/logged-out-view.tsxapps/ui/src/routes/logged-out.tsx
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/logged-out-view.tsxapps/ui/src/routes/logged-out.tsxapps/ui/src/components/views/settings-view/security/security-section.tsxapps/ui/src/app.tsxapps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsxapps/ui/src/components/views/login-view.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/settings-view/account/account-section.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/components/settings-navigation.tsx
🧬 Code graph analysis (10)
apps/ui/src/components/views/logged-out-view.tsx (1)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/routes/logged-out.tsx (2)
apps/ui/src/routes/__root.tsx (1)
Route(488-490)apps/ui/src/components/views/logged-out-view.tsx (1)
LoggedOutView(5-33)
apps/ui/src/components/views/settings-view/security/security-section.tsx (2)
apps/ui/src/components/views/settings-view/security/index.ts (1)
SecuritySection(1-1)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/app.tsx (1)
apps/ui/src/hooks/use-settings-sync.ts (1)
useSettingsSync(86-295)
apps/ui/src/routes/__root.tsx (7)
apps/ui/src/store/app-store.ts (1)
getStoredTheme(71-77)apps/ui/src/config/theme-options.ts (1)
themeOptions(78-337)apps/ui/src/lib/http-api-client.ts (1)
verifySession(372-412)apps/ui/src/hooks/use-settings-migration.ts (2)
hydrateStoreFromSettings(444-519)signalMigrationComplete(77-82)apps/ui/src/components/dialogs/index.ts (2)
SandboxRejectionScreen(6-6)SandboxRiskDialog(7-7)apps/ui/src/lib/electron.ts (1)
isElectron(789-799)apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx (1)
SandboxRiskDialog(28-108)
apps/ui/src/components/views/settings-view/account/account-section.tsx (3)
apps/ui/src/lib/http-api-client.ts (1)
logout(341-358)apps/ui/src/store/auth-store.ts (1)
useAuthStore(25-29)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/lib/http-api-client.ts (2)
libs/types/src/feature.ts (1)
Feature(34-69)apps/ui/src/store/app-store.ts (1)
Feature(301-317)
apps/ui/src/components/views/settings-view.tsx (2)
apps/ui/src/components/views/settings-view/account/account-section.tsx (1)
AccountSection(9-77)apps/ui/src/components/views/settings-view/security/security-section.tsx (1)
SecuritySection(11-71)
apps/ui/src/hooks/use-settings-migration.ts (6)
libs/utils/src/logger.ts (1)
createLogger(127-219)libs/types/src/settings.ts (1)
GlobalSettings(386-499)apps/ui/src/lib/storage.ts (3)
getItem(21-28)removeItem(51-59)setItem(36-44).github/scripts/upload-to-r2.js (1)
result(67-157)apps/ui/src/store/app-store.ts (1)
THEME_STORAGE_KEY(65-65)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(2112-2117)
apps/ui/src/components/views/settings-view/components/settings-navigation.tsx (3)
apps/ui/src/components/views/settings-view/config/navigation.ts (3)
NavigationItem(19-23)GLOBAL_NAV_ITEMS(31-44)PROJECT_NAV_ITEMS(47-49)apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
SettingsViewId(3-17)apps/ui/src/lib/utils.ts (1)
cn(5-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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (27)
apps/server/src/lib/auth.ts (1)
265-265: State-changing operations properly use non-GET methods; CSRF risk is mitigated.The codebase correctly uses POST, PUT, and DELETE for all state-changing operations, with no state-changing GET endpoints detected. This means
sameSite: 'lax'is safe—lax mode restricts cookies for cross-site POST/PUT/DELETE requests while allowing GET, which only retrieves data and doesn't change state.apps/server/src/routes/auth/index.ts (1)
238-238: Update the outdated comment in the file header to match the actual implementation.Lines 8-10 state
SameSite=Strictbut the code usessameSite: 'lax'. The verification confirms thatgetSessionCookieOptions()returns'lax'(line 265), and both the login route (line 183) and logout route (line 238) consistently use'lax'. Update the comment to reflect the actual cookie policy:* The session cookie is: * - HTTP-only: JavaScript cannot read it (protects against XSS) * - SameSite=Lax: Sent on same-site requests including cross-origin fetches (protects against CSRF)apps/ui/src/components/views/logged-out-view.tsx (1)
1-33: LGTM! Clean logged-out view implementation.The component follows best practices with TanStack Router navigation, clear UI messaging, and appropriate retry/login actions. The layout is well-structured with proper Tailwind utilities.
apps/ui/src/hooks/use-settings-migration.ts (1)
66-99: Good coordination pattern for hydration timing.The promise-based coordination between migration and sync hooks is clean and prevents race conditions. Module-level state is appropriate for this singleton coordination mechanism.
apps/ui/src/lib/http-api-client.ts (4)
48-76: Well-structured unauthorized handling flow.The separation of concerns between
notifyLoggedOut(UI notification) andhandleUnauthorized(cleanup + notification) is clean. Best-effort cookie clearing with error suppression is appropriate for this failure scenario.
372-412: Excellent error handling improvements in session verification.The clear distinction between authentication failures (401/403 → return false) and transient errors (throw for retry) improves reliability. The timeout prevents hanging during server restarts, and the comment explaining why logout isn't called on failure is helpful for maintainability.
507-510: Consistent unauthorized handling across API methods.The uniform pattern of detecting 401/403, calling
handleUnauthorized(), and throwing ensures all API methods handle session expiry consistently. This will prevent silent failures and ensure proper UI redirection.Also applies to: 693-696, 722-725, 753-756, 783-786
1269-1282: Clean backward-compatible API extension.The optional
descriptionHistorySourceandenhancementModeparameters extend the features API without breaking existing callers. This supports the feature description history tracking mentioned in the PR objectives.apps/ui/src/components/views/settings-view/account/index.ts (1)
1-1: LGTM! Standard barrel export pattern.Clean re-export for the AccountSection component.
apps/ui/src/routes/logged-out.tsx (1)
1-6: LGTM! Standard route definition following TanStack Router patterns.The file-based route correctly wires the
/logged-outpath to theLoggedOutViewcomponent, following the learned patterns for TanStack Router usage in this codebase.apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
15-16: LGTM! Clean type extension.The addition of 'account' and 'security' view identifiers correctly extends the SettingsViewId union type to support the new settings sections introduced in this PR.
apps/ui/src/components/views/settings-view/security/index.ts (1)
1-1: LGTM! Standard barrel export pattern.The re-export follows the established convention in the codebase for organizing settings sections.
apps/ui/src/app.tsx (1)
36-44: LGTM! Clear migration to API-first settings sync.The shift from
useSettingsMigrationtouseSettingsSyncaligns with the PR's goal to establish API-first settings persistence. The error logging is appropriate and the comments clearly explain the new flow.apps/ui/src/components/views/settings-view/account/account-section.tsx (1)
27-76: LGTM! Well-structured account section UI.The component structure is clean, accessible, and follows the established design patterns in the settings UI. The logout button includes proper loading states and test identifiers.
apps/ui/src/components/views/settings-view.tsx (4)
19-20: LGTM! Proper import of new settings sections.The imports for
AccountSectionandSecuritySectionare correctly structured and follow the established barrel export pattern.
36-37: LGTM! State management correctly wired.The new state variables
skipVerificationInAutoModeandskipSandboxWarningare properly extracted from the store along with their setters, following the established pattern in this component.Also applies to: 57-58
151-159: LGTM! New sections correctly integrated into render logic.The
accountandsecuritycases are properly added to the switch statement with correct prop wiring. TheAccountSectionrequires no props andSecuritySectionreceives the appropriate sandbox warning state and handler.
135-135: LGTM! skipVerificationInAutoMode prop correctly wired.The new
skipVerificationInAutoModestate and its setter are properly passed toFeatureDefaultsSection, enabling the Auto Mode verification control introduced in this PR.Also applies to: 144-144
apps/ui/src/components/views/settings-view/security/security-section.tsx (1)
1-71: Well-structured security settings component.The component correctly implements the inverted toggle logic where "Show Sandbox Warning" (checked) corresponds to
!skipSandboxWarning. The accessibility attributes (htmlFor,data-testid) and styling are appropriate.apps/ui/src/components/views/settings-view/config/navigation.ts (1)
25-52: Clean navigation configuration with good backward compatibility.The separation into
GLOBAL_NAV_ITEMSandPROJECT_NAV_ITEMSwith a composedNAV_ITEMSexport for backward compatibility is well-designed.Note:
NavigationGroupinterface is exported but not currently used in the provided files. If it's intended for future use, consider adding a comment; otherwise, it could be removed.apps/ui/src/hooks/use-settings-sync.ts (2)
348-387:currentProjectIdis synced butcurrentProjectis not restored.The sync logic (line 107-109) sends
currentProjectIdto the server, butrefreshSettingsFromServerdoesn't restorecurrentProjectfromserverSettings.currentProjectId. If the user expects their last-open project to be restored on refresh/reload, this asymmetry will cause the project selection to be lost.If
currentProjectshould be restored, you'd need to look up the project fromserverSettings.projectsusingserverSettings.currentProjectId:// After setting projects, restore currentProject if ID exists const restoredProject = serverSettings.currentProjectId ? serverSettings.projects?.find(p => p.id === serverSettings.currentProjectId) ?? null : null; useAppStore.setState({ // ... existing fields currentProject: restoredProject, });
1-23: Well-documented hook with proper initialization sequencing.The hook correctly waits for migration completion before syncing, preventing the overwriting of server data with empty state. The documentation clearly explains the flow and the importance of initialization order.
apps/ui/src/components/views/settings-view/danger-zone/danger-zone-section.tsx (1)
1-64: Clean simplification removing sandbox-related logic.The component now focuses solely on project deletion, with a clear two-state render (project selected vs. no project). The removal of sandbox-related props aligns with the PR objective.
apps/ui/src/components/views/login-view.tsx (1)
52-87: Clean state machine implementation with well-defined transitions.The reducer correctly handles all state transitions with proper phase guards (e.g.,
UPDATE_API_KEYonly works inawaiting_loginphase). This prevents invalid state transitions.apps/ui/src/routes/__root.tsx (3)
31-56: Good approach to prevent theme flash on page load.Running
applyStoredTheme()synchronously at module load time (before React hydration) correctly prevents the flash of default theme. This is the right pattern for SSR/hydration scenarios.
206-222: Good global handler for session expiration events.The
automaker:logged-outevent listener provides a centralized way to handle 401/403 responses from anywhere in the app. The pathname check prevents redundant navigation.
131-169: Correct timing for sandbox environment check.The sandbox check now correctly waits until the user is authenticated and setup is complete before checking/prompting. This prevents the sandbox dialog from appearing during the login or setup flows.
| if (localSettings) { | ||
| logger.info('Using localStorage data as fallback'); | ||
| hydrateStoreFromSettings(localSettings as GlobalSettings); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unsafe cast from Partial to full type remains unresolved.
Lines 340 and 382 cast localSettings (typed as Partial<GlobalSettings>) to GlobalSettings. This hides potentially missing required fields from TypeScript's type checker. While hydrateStoreFromSettings uses nullish coalescing (??) for defaults, this defensive programming is obscured by the unsafe cast.
As noted in the previous review, consider updating hydrateStoreFromSettings to accept Partial<GlobalSettings>:
export function hydrateStoreFromSettings(settings: Partial<GlobalSettings>): void {This maintains type honesty while the existing ?? defaults handle missing values safely.
Also applies to: 380-384
- Added logic to perform settings migration, merging localStorage data with server settings if necessary. - Introduced `localStorageMigrated` flag to prevent re-migration on subsequent app loads. - Updated `useSettingsMigration` hook to handle migration and hydration of settings. - Ensured localStorage values are preserved post-migration for user flexibility. - Enhanced documentation within the migration logic for clarity.
…tication flow - Revised SameSite attribute for session cookies to clarify its behavior in documentation. - Streamlined cookie clearing logic in the authentication route by utilizing `getSessionCookieOptions()`. - Enhanced the login view to support aborting server checks, improving responsiveness during component unmounting. - Ensured proper handling of server check retries with abort signal integration for better user experience.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/ui/src/hooks/use-settings-migration.ts:
- Around line 402-405: The unsafe cast of localSettings to GlobalSettings should
be removed by changing the hydrateStoreFromSettings signature to accept
Partial<GlobalSettings> (or undefined) so callers like the localSettings branch
in use-settings-migration can pass localSettings directly; update
hydrateStoreFromSettings’s parameter type and any imports/types it uses, adjust
internal logic as needed (it already uses nullish coalescing) to handle missing
fields, and then remove the explicit casts at the call sites (the places using
localSettings and the other call that currently casts) so no unsafe casting
remains.
In @libs/types/src/settings.ts:
- Around line 493-494: The PR promised to remove sandbox-related code but the
optional field skipSandboxWarning remains in the GlobalSettings type and is
still referenced from use-settings-migration (symbols: GlobalSettings and
skipSandboxWarning, and the functions/hooks in use-settings-migration.ts that
reference it); remove the skipSandboxWarning property from the GlobalSettings
interface and delete or refactor any logic in use-settings-migration.ts that
reads/writes skipSandboxWarning (or migrate it into the new settings shape if
you intend to preserve backward compatibility), then run TypeScript build/tests
and update any related tests/comments to ensure no remaining references to
skipSandboxWarning exist.
🧹 Nitpick comments (2)
apps/ui/src/routes/__root.tsx (1)
262-264: Simplify type assertion.The nested type cast
as unknown as Parameters<typeof performSettingsMigration>[0]is verbose. Since bothsettingsResult.settingsandperformSettingsMigrationexpectGlobalSettings, a direct cast would be clearer:♻️ Proposed simplification
- const { settings: finalSettings, migrated } = await performSettingsMigration( - settingsResult.settings as unknown as Parameters<typeof performSettingsMigration>[0] - ); + const { settings: finalSettings, migrated } = await performSettingsMigration( + settingsResult.settings as GlobalSettings + );apps/ui/src/hooks/use-settings-migration.ts (1)
516-591: Consider accepting Partial parameter.The
hydrateStoreFromSettingsfunction uses nullish coalescing (??) for all fields, making it inherently safe to acceptPartial<GlobalSettings>. Updating the signature would eliminate the need for unsafe casts at call sites (lines 404, 451) and improve type safety.♻️ Proposed signature change
-export function hydrateStoreFromSettings(settings: GlobalSettings): void { +export function hydrateStoreFromSettings(settings: Partial<GlobalSettings>): void {This matches the function's existing defensive implementation and eliminates unsafe casts throughout the codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ui/src/hooks/use-settings-migration.tsapps/ui/src/routes/__root.tsxlibs/types/src/settings.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/routes/__root.tsxapps/ui/src/hooks/use-settings-migration.tslibs/types/src/settings.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/routes/__root.tsxapps/ui/src/hooks/use-settings-migration.tslibs/types/src/settings.ts
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Frontend UI must use TanStack Router for file-based routing, organize components in components/views/, implement stores with Zustand, and use custom hooks in hooks/ directory
Applied to files:
apps/ui/src/routes/__root.tsx
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/routes/__root.tsx
🧬 Code graph analysis (1)
apps/ui/src/hooks/use-settings-migration.ts (5)
libs/types/src/settings.ts (1)
GlobalSettings(386-503)apps/ui/src/lib/storage.ts (2)
getItem(21-28)setItem(36-44)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(2112-2117)apps/ui/src/store/app-store.ts (1)
THEME_STORAGE_KEY(65-65)apps/ui/src/store/setup-store.ts (1)
useSetupStore(147-195)
⏰ 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 (18)
apps/ui/src/routes/__root.tsx (6)
135-173: LGTM - Sandbox check properly deferred.The sandbox environment check now correctly waits until after authentication and setup completion before running. This prevents unnecessary sandbox checks during initial authentication flow.
210-226: LGTM - Centralized auth failure handling.The global listener for 401/403 responses provides unified logout handling across all API calls. The pathname check prevents navigation loops.
280-311: LGTM - Comprehensive error handling.All error paths (settings fetch failure, invalid session, auth init error) properly call
signalMigrationComplete()to ensure dependent hooks don't hang waiting for migration. The fallback navigation to/logged-outis appropriate.
327-349: LGTM - Clear routing rules.The routing logic correctly implements the unified authentication flow:
- Unauthenticated →
/logged-out(allowing/login)- Authenticated without setup →
/setup- Authenticated with setup → app access
428-445: LGTM - Appropriate loading states.The loading states during auth check and redirect prevent flashing incorrect content to users. The wait for
authCheckedbefore rendering protected routes is essential for security.
482-486: This comment is inaccurate—sandbox functionality is intentional, not incomplete removal.The
SandboxRiskDialogand related sandbox logic are not leftover code. The feature remains active by design with full implementation: environment detection viacheckSandboxEnvironment(), user preference persistence throughskipSandboxWarningsetting, confirmation handlers, and integration into app initialization. This is a deliberate security feature, not partial cleanup.Likely an incorrect or invalid review comment.
libs/types/src/settings.ts (5)
390-400: LGTM - Well-documented migration fields.The new migration tracking and onboarding fields (
localStorageMigrated,setupComplete,isFirstRun,skipClaudeSetup) are properly documented and support the new settings hydration workflow.
421-422: LGTM - Clear feature flag.The
skipVerificationInAutoModefield is well-documented and provides flexibility for auto-mode workflows.
467-468: LGTM - Project tracking field.The
currentProjectIdfield enables proper restoration of the current project across sessions.
627-627: LGTM - Appropriate version bump.The schema version bump from 3 to 4 correctly reflects the addition of new required fields (
setupComplete,isFirstRun,skipClaudeSetup,currentProjectId,skipVerificationInAutoMode).
658-695: LGTM - Sensible defaults.The
DEFAULT_GLOBAL_SETTINGSincludes appropriate default values for all new fields:
setupComplete: falseandisFirstRun: truefor new installationsskipVerificationInAutoMode: falsefor safer default behaviorcurrentProjectId: nullindicating no project open initiallyapps/ui/src/hooks/use-settings-migration.ts (7)
8-23: LGTM - Clear migration flow documentation.The updated documentation clearly explains the hydration-centric approach and the intentional preservation of localStorage for backward compatibility. This is good design for users who might need to downgrade.
62-95: LGTM - Clean coordination pattern.The migration completion signaling mechanism using promises allows proper sequencing between settings hydration and synchronization hooks. The early-completion check prevents deadlocks.
100-169: LGTM - Comprehensive localStorage parsing.The function correctly handles multiple localStorage sources including Zustand's persist format, standalone keys, and nested state structures. Returning
Partial<GlobalSettings>is type-honest about potentially missing fields.
175-203: LGTM - Smart data comparison.The
localStorageHasMoreDatafunction correctly identifies when localStorage contains data that's missing from the server, focusing on projects and AI profiles as key indicators of migration need.
209-289: LGTM - Intelligent merging strategy.The
mergeSettingsfunction properly merges localStorage data with server settings by preferring non-empty arrays and objects. This prevents data loss during migration while respecting server authority for populated data.
300-357: LGTM - Well-structured migration logic.The extracted
performSettingsMigrationfunction provides reusable migration logic that can be called from__root.tsx. It correctly:
- Checks the migration flag to prevent re-migration
- Merges data only when localStorage has more complete data
- Marks migration as complete in server settings
596-636: LGTM - Complete settings extraction.The
buildSettingsUpdateFromStorefunction correctly gathers all settings from both the app and setup stores, including proper extraction ofcurrentProjectIdfrom thecurrentProjectobject.
| if (localSettings) { | ||
| logger.info('Using localStorage data as fallback'); | ||
| hydrateStoreFromSettings(localSettings as GlobalSettings); | ||
| } |
There was a problem hiding this comment.
Unresolved: Unsafe cast remains from previous review.
As noted in previous reviews, casting localSettings (typed as Partial<GlobalSettings>) to GlobalSettings at line 404 is unsafe. The same issue occurs at line 451. While hydrateStoreFromSettings uses nullish coalescing (??) for defaults, this defensive programming is obscured by the unsafe cast.
Consider updating the hydrateStoreFromSettings signature to accept Partial<GlobalSettings>:
♻️ Recommended fix
-export function hydrateStoreFromSettings(settings: GlobalSettings): void {
+export function hydrateStoreFromSettings(settings: Partial<GlobalSettings>): void {Then remove the unsafe casts at lines 404 and 451:
- hydrateStoreFromSettings(localSettings as GlobalSettings);
+ hydrateStoreFromSettings(localSettings);And:
- finalSettings = localSettings as GlobalSettings;
+ // Keep as Partial, hydrateStoreFromSettings will handle defaults
+ hydrateStoreFromSettings(localSettings);
+ signalMigrationComplete();
+ setState({ checked: true, migrated: needsSync, error: null });
+ return;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/ui/src/hooks/use-settings-migration.ts around lines 402 - 405, The
unsafe cast of localSettings to GlobalSettings should be removed by changing the
hydrateStoreFromSettings signature to accept Partial<GlobalSettings> (or
undefined) so callers like the localSettings branch in use-settings-migration
can pass localSettings directly; update hydrateStoreFromSettings’s parameter
type and any imports/types it uses, adjust internal logic as needed (it already
uses nullish coalescing) to handle missing fields, and then remove the explicit
casts at the call sites (the places using localSettings and the other call that
currently casts) so no unsafe casting remains.
| /** Skip the sandbox environment warning dialog on startup */ | ||
| skipSandboxWarning?: boolean; |
There was a problem hiding this comment.
Inconsistency: Sandbox field remains despite PR objectives.
The PR description states "completly remove sandbox related code," but the skipSandboxWarning field remains in GlobalSettings. This is also referenced in:
- Line 564 of
apps/ui/src/hooks/use-settings-migration.ts - Line 621 of
apps/ui/src/hooks/use-settings-migration.ts
Please confirm whether this field should be removed or is being retained for backward compatibility/migration purposes.
#!/bin/bash
# Find all references to skipSandboxWarning
echo "=== All references to skipSandboxWarning ==="
rg -n "skipSandboxWarning" --type=ts --type=tsx🤖 Prompt for AI Agents
In @libs/types/src/settings.ts around lines 493 - 494, The PR promised to remove
sandbox-related code but the optional field skipSandboxWarning remains in the
GlobalSettings type and is still referenced from use-settings-migration
(symbols: GlobalSettings and skipSandboxWarning, and the functions/hooks in
use-settings-migration.ts that reference it); remove the skipSandboxWarning
property from the GlobalSettings interface and delete or refactor any logic in
use-settings-migration.ts that reads/writes skipSandboxWarning (or migrate it
into the new settings shape if you intend to preserve backward compatibility),
then run TypeScript build/tests and update any related tests/comments to ensure
no remaining references to skipSandboxWarning exist.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/ui/src/components/views/login-view.tsx:
- Around line 274-279: handleRetry creates a new AbortController that is never
cleaned up; store the controller in a ref (e.g., retryControllerRef) so you can
abort previous requests when retry is invoked and abort the active controller in
the component's cleanup effect. Update handleRetry to abort and replace any
existing controller before creating/using a new one and pass
retryControllerRef.current.signal to checkServerAndSession, and add a useEffect
cleanup that calls retryControllerRef.current?.abort() to prevent leaked async
ops.
- Around line 179-206: checkSetupStatus currently performs an async
settings.getGlobal() and dispatches state updates even after unmount; modify
checkSetupStatus to accept an optional AbortSignal (e.g., function
checkSetupStatus(dispatch: React.Dispatch<Action>, signal?: AbortSignal)) and
pass that signal into getHttpApiClient()/settings.getGlobal() if the client
supports a signal, otherwise check signal?.aborted after the await and return
early if aborted; ensure you only call
useSetupStore.getState().setSetupComplete(...) and dispatch(...) when not
aborted. Then update the calling useEffect to create an AbortController, call
checkSetupStatus(dispatch, controller.signal), and abort the controller in the
cleanup to prevent state updates after unmount.
🧹 Nitpick comments (3)
apps/ui/src/app.tsx (1)
35-43: Good architectural shift to non-blocking settings sync.The change from a blocking migration flow to background API-first sync aligns well with the PR objectives. The comments clearly document the new flow where settings are loaded in
__root.tsxafter session verification.Optional refinement: Move error logging to useEffect.
The error logging on line 42 executes during the render phase, which means it will log on every re-render if the error persists. Consider wrapping it in
useEffect:♻️ Proposed refactor
// Sync settings changes back to server (API-first persistence) const settingsSyncState = useSettingsSync(); - if (settingsSyncState.error) { - logger.error('Settings sync error:', settingsSyncState.error); - } + + useEffect(() => { + if (settingsSyncState.error) { + logger.error('Settings sync error:', settingsSyncState.error); + } + }, [settingsSyncState.error]);Optional enhancement: Consider using
loadedandsyncingstates.The hook returns
loadedandsyncingstates that could be used to:
- Verify initialization completed successfully
- Show UI feedback during sync operations (e.g., a subtle indicator when settings are being saved)
These are optional UX improvements but may not be necessary if the current silent background sync approach meets your requirements.
apps/ui/src/components/views/login-view.tsx (2)
258-262: Simplify the dependency array.The conditional expression
state.phase === 'redirecting' ? state.to : nullin the dependency array is overly complex. Since the effect already guards withif (state.phase === 'redirecting'), you can simplify the dependencies.♻️ Proposed fix
useEffect(() => { if (state.phase === 'redirecting') { navigate({ to: state.to }); } - }, [state.phase, state.phase === 'redirecting' ? state.to : null, navigate]); + }, [state, navigate]);Or if you want to be more granular:
- }, [state.phase, state.phase === 'redirecting' ? state.to : null, navigate]); + }, [state.phase, navigate]);The second option works because
stateis the result ofuseReducer, which provides stable object identity when state doesn't change, so the effect will only re-run when the state actually changes.
336-338: Redundant ternary expression.Both branches of the ternary return
state.apiKey, making the condition pointless.♻️ Proposed fix
const isLoggingIn = state.phase === 'logging_in'; - const apiKey = state.phase === 'awaiting_login' ? state.apiKey : state.apiKey; + const apiKey = state.apiKey; const error = state.phase === 'awaiting_login' ? state.error : null;Or if you need to satisfy TypeScript's type narrowing:
- const apiKey = state.phase === 'awaiting_login' ? state.apiKey : state.apiKey; + const apiKey = state.phase === 'awaiting_login' || state.phase === 'logging_in' ? state.apiKey : '';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/server/src/lib/auth.tsapps/server/src/routes/auth/index.tsapps/server/tests/unit/lib/auth.test.tsapps/ui/src/app.tsxapps/ui/src/components/views/login-view.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/server/src/lib/auth.ts
- apps/server/src/routes/auth/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/app.tsxapps/ui/src/components/views/login-view.tsxapps/server/tests/unit/lib/auth.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/app.tsxapps/ui/src/components/views/login-view.tsxapps/server/tests/unit/lib/auth.test.ts
🧠 Learnings (1)
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/app.tsxapps/ui/src/components/views/login-view.tsx
🧬 Code graph analysis (2)
apps/ui/src/app.tsx (1)
apps/ui/src/hooks/use-settings-sync.ts (1)
useSettingsSync(86-295)
apps/ui/src/components/views/login-view.tsx (3)
apps/ui/src/lib/http-api-client.ts (3)
getServerUrlSync(113-113)getHttpApiClient(2112-2117)login(269-303)apps/ui/src/store/setup-store.ts (1)
useSetupStore(147-195)apps/ui/src/store/auth-store.ts (1)
useAuthStore(25-29)
⏰ 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 (4)
apps/ui/src/app.tsx (1)
6-6: LGTM: Import aligns with new settings sync architecture.The import change from
useSettingsMigrationtouseSettingsSynccorrectly supports the PR's goal of replacing the blocking migration flow with an API-first background sync approach.apps/server/tests/unit/lib/auth.test.ts (1)
280-280: Test expectation correctly updated to reflect sameSite: 'lax'.The test accurately validates the session cookie configuration. The change from
'strict'to'lax'is a standard practice that balances security with usability—'lax'still provides CSRF protection while allowing cookies on top-level navigation. CSRF protections are in place through API key authentication and Content-Type validation requirements.apps/ui/src/components/views/login-view.tsx (2)
27-87: LGTM: Well-designed state machine architecture.The state machine provides clear flow control and maintainability. The reducer correctly handles all state transitions with appropriate guards.
110-121: No action needed.AbortSignal.timeout()is widely supported in all modern browsers (since 2022–2023) and is appropriate for this project's environment. The project targets modern browsers (ESNext, React 19, Vite) with Node.js ≥22.0.0, well above the API's minimum requirements. No polyfill is necessary.
| async function checkSetupStatus(dispatch: React.Dispatch<Action>): Promise<void> { | ||
| const httpClient = getHttpApiClient(); | ||
|
|
||
| try { | ||
| const result = await httpClient.settings.getGlobal(); | ||
|
|
||
| if (result.success && result.settings) { | ||
| // Check the setupComplete field from settings | ||
| // This is set to true when user completes the setup wizard | ||
| const setupComplete = (result.settings as { setupComplete?: boolean }).setupComplete === true; | ||
|
|
||
| // IMPORTANT: Update the Zustand store BEFORE redirecting | ||
| // Otherwise __root.tsx routing effect will override our redirect | ||
| // because it reads setupComplete from the store (which defaults to false) | ||
| useSetupStore.getState().setSetupComplete(setupComplete); | ||
|
|
||
| dispatch({ type: 'REDIRECT', to: setupComplete ? '/' : '/setup' }); | ||
| } else { | ||
| // No settings yet = first run = need setup | ||
| useSetupStore.getState().setSetupComplete(false); | ||
| dispatch({ type: 'REDIRECT', to: '/setup' }); | ||
| } | ||
| } catch { | ||
| // If we can't get settings, go to setup to be safe | ||
| useSetupStore.getState().setSetupComplete(false); | ||
| dispatch({ type: 'REDIRECT', to: '/setup' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing cleanup for async operation in checkSetupStatus.
The checkSetupStatus function is called from a useEffect (lines 251-255) but doesn't accept an AbortSignal. If the component unmounts while httpClient.settings.getGlobal() is pending, the subsequent dispatch calls will attempt to update state on an unmounted component.
🛠️ Proposed fix
-async function checkSetupStatus(dispatch: React.Dispatch<Action>): Promise<void> {
+async function checkSetupStatus(dispatch: React.Dispatch<Action>, signal?: AbortSignal): Promise<void> {
const httpClient = getHttpApiClient();
try {
const result = await httpClient.settings.getGlobal();
+
+ if (signal?.aborted) return;
if (result.success && result.settings) {
// Check the setupComplete field from settings
// This is set to true when user completes the setup wizard
const setupComplete = (result.settings as { setupComplete?: boolean }).setupComplete === true;
// IMPORTANT: Update the Zustand store BEFORE redirecting
// Otherwise __root.tsx routing effect will override our redirect
// because it reads setupComplete from the store (which defaults to false)
useSetupStore.getState().setSetupComplete(setupComplete);
dispatch({ type: 'REDIRECT', to: setupComplete ? '/' : '/setup' });
} else {
// No settings yet = first run = need setup
useSetupStore.getState().setSetupComplete(false);
dispatch({ type: 'REDIRECT', to: '/setup' });
}
} catch {
+ if (signal?.aborted) return;
// If we can't get settings, go to setup to be safe
useSetupStore.getState().setSetupComplete(false);
dispatch({ type: 'REDIRECT', to: '/setup' });
}
}Then update the useEffect call at line 253:
useEffect(() => {
if (state.phase === 'checking_setup') {
- checkSetupStatus(dispatch);
+ const controller = new AbortController();
+ checkSetupStatus(dispatch, controller.signal);
+ return () => controller.abort();
}
}, [state.phase]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/ui/src/components/views/login-view.tsx around lines 179 - 206,
checkSetupStatus currently performs an async settings.getGlobal() and dispatches
state updates even after unmount; modify checkSetupStatus to accept an optional
AbortSignal (e.g., function checkSetupStatus(dispatch: React.Dispatch<Action>,
signal?: AbortSignal)) and pass that signal into
getHttpApiClient()/settings.getGlobal() if the client supports a signal,
otherwise check signal?.aborted after the await and return early if aborted;
ensure you only call useSetupStore.getState().setSetupComplete(...) and
dispatch(...) when not aborted. Then update the calling useEffect to create an
AbortController, call checkSetupStatus(dispatch, controller.signal), and abort
the controller in the cleanup to prevent state updates after unmount.
| const handleRetry = () => { | ||
| initialCheckDone.current = false; | ||
| dispatch({ type: 'RETRY_SERVER_CHECK' }); | ||
| const controller = new AbortController(); | ||
| checkServerAndSession(dispatch, setAuthState, controller.signal); | ||
| }; |
There was a problem hiding this comment.
Memory leak: AbortController never cleaned up in handleRetry.
A new AbortController is created each time the user clicks retry, but there's no reference stored and no cleanup. If the component unmounts after clicking retry, or if the user clicks retry multiple times rapidly, these controllers will never be aborted, and the async operations will continue running.
🔧 Proposed fix
Store the controller reference and handle cleanup:
+ const retryControllerRef = useRef<AbortController | null>(null);
+
const handleRetry = () => {
+ // Abort any in-flight retry
+ retryControllerRef.current?.abort();
+
initialCheckDone.current = false;
dispatch({ type: 'RETRY_SERVER_CHECK' });
const controller = new AbortController();
+ retryControllerRef.current = controller;
checkServerAndSession(dispatch, setAuthState, controller.signal);
};And add cleanup in the component's effect:
useEffect(() => {
if (initialCheckDone.current) return;
initialCheckDone.current = true;
const controller = new AbortController();
checkServerAndSession(dispatch, setAuthState, controller.signal);
return () => {
controller.abort();
+ retryControllerRef.current?.abort();
};
}, [setAuthState]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/ui/src/components/views/login-view.tsx around lines 274 - 279,
handleRetry creates a new AbortController that is never cleaned up; store the
controller in a ref (e.g., retryControllerRef) so you can abort previous
requests when retry is invoked and abort the active controller in the
component's cleanup effect. Update handleRetry to abort and replace any existing
controller before creating/using a new one and pass
retryControllerRef.current.signal to checkServerAndSession, and add a useEffect
cleanup that calls retryControllerRef.current?.abort() to prevent leaked async
ops.
The codex-provider.ts imports this function but it was missing from sdk-options.ts. This adds the implementation that checks if sandbox mode is compatible with the working directory (disables sandbox for cloud storage paths). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…login view effect handling - Added additional cloud storage path patterns for macOS and Linux to the checkSandboxCompatibility function, ensuring better compatibility with sandbox environments. - Revised the login view to simplify the initial server/session check logic, removing unnecessary ref guard and improving responsiveness during component unmounting.
- Added a new function `waitForSplashScreenToDisappear` to manage splash screen visibility, ensuring it does not block user interactions. - Integrated splash screen checks in various navigation functions and interaction methods to enhance user experience by waiting for the splash screen to disappear before proceeding. - Updated test setup to disable the splash screen during tests for consistent testing behavior.
…ings - Added `skipSandboxWarning` option to project setup functions to streamline testing. - Implemented logic to disable the splash screen during tests by setting `automaker-splash-shown` in sessionStorage. - Introduced a new package.json for a test project and added a test image to the fixtures for improved testing capabilities.
- Updated the 'Add Context Image' test to simplify file verification by relying on UI visibility instead of disk checks. - Enhanced the 'Feature Manual Review Flow' test with better project setup and API interception to ensure consistent test conditions. - Improved the 'AI Profiles' test by replacing arbitrary timeouts with dynamic checks for profile count. - Refined the 'Project Creation' and 'Open Existing Project' tests to ensure proper project visibility and settings management during tests. - Added mechanisms to prevent settings hydration from restoring previous project states, ensuring tests run in isolation. - Removed unused test image from fixtures to clean up the repository.
- Renamed the upload step to clarify that it includes screenshots, traces, and videos. - Changed the condition for uploading test results to always run, ensuring artifacts are uploaded regardless of test outcome. - Added a new option to ignore if no files are found during the upload process.
…vider - Added a new utility for checking Codex CLI authentication status using the 'codex login status' command. - Integrated the authentication check into the CodexProvider's installation detection and authentication methods. - Updated Codex CLI status display in the UI to reflect authentication status and method. - Enhanced error handling and logging for better debugging during authentication checks. - Refactored related components to ensure consistent handling of authentication across the application.
- Added safeguards to prevent overwriting non-empty arrays with empty arrays during global settings updates, specifically for the 'projects' field. - Implemented logging for updates to assist in diagnosing accidental wipes of critical settings. - Updated tests to verify that projects are preserved during logout transitions and that theme changes are ignored if a project wipe is attempted. - Enhanced the settings synchronization logic to ensure safe handling during authentication state changes.
- Updated Playwright configuration to explicitly unset ALLOWED_ROOT_DIRECTORY for unrestricted testing paths. - Improved E2E fixture setup script to reset server settings to a known state, ensuring test isolation. - Enhanced error handling in ContextView and WelcomeView components to reset state and provide user feedback on failures. - Updated tests to ensure proper navigation and visibility checks during logout processes, improving reliability.
- Removed the redundant call to ignore empty array overwrite for 'enabledCodexModels' in the SettingsService. - Reformatted the description of the 'Heavy Task' profile in the E2E fixture setup script for better readability.
…ization - Changed the E2E settings to enable the use of worktrees for better test isolation. - Modified the test repository initialization to explicitly set the initial branch to 'main', ensuring compatibility across different git versions and avoiding CI environment discrepancies.
- Added useRef to manage AbortController for retry requests in the LoginView component. - Implemented logic to abort any ongoing retry requests before initiating a new server check, improving error handling and user experience during login attempts.
webdevcody
force-pushed
the
remove-sandbox-as-it-is-broken
branch
from
25b3e77 to
69434fe
Compare
- Introduced a new npm script "test:ui" for running UI tests in the apps/ui workspace. - Removed unnecessary login screen handling from the worktree integration test to streamline the test flow.
- Updated the logic in the createListHandler to ensure that the branch name is correctly assigned, especially for the main worktree when it may be missing. - Added checks to handle cases where the worktree directory might not exist, ensuring that removed worktrees are accurately tracked. - Enhanced the final worktree entry handling to account for scenarios where the output does not end with a blank line, improving robustness.
- Deleted the worktree integration test file to streamline the test suite and remove obsolete tests. This change helps maintain focus on relevant test cases and improves overall test management.
- Added support for configuring custom Anthropic API base URLs to enable enterprise deployments and custom endpoints. - Enhanced the Credentials interface to include optional baseUrls field for storing custom API endpoints. - Updated the Claude provider to support ANTHROPIC_BASE_URL environment variable for the Anthropic SDK. - Modified the SettingsService to handle base URLs alongside API keys with proper persistence. - Updated setup API routes to accept and store base URL configuration from the UI. - Combined API key and base URL inputs into a single cohesive configuration section in the setup UI. - Enhanced the HTTP client and Electron API to pass base URL parameters through the save flow. - Updated all type definitions to ensure type safety for the new base URL functionality. This allows users to configure custom Anthropic API endpoints while maintaining backward compatibility for existing users who don't need this feature. Closes AutoMaker-Org#378
…not work with it on various os
Summary by CodeRabbit
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.