fix: resolve data directory persistence between Electron and Web modes #573
Conversation
When running in web mode (npm run dev:web), the frontend on localhost:3007 was making cross-origin requests to the backend on localhost:3008, causing CORS errors. Added Vite proxy configuration to forward /api requests from the dev server to the backend. This makes all API calls appear same-origin to the browser, eliminating CORS blocks during development. Now web mode users can access http://localhost:3007 without CORS errors. Fixes: CORS "Not allowed by CORS" errors in web mode Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
In web mode, the API client was hardcoding localhost:3008, which bypassed the Vite proxy and caused CORS errors. Now it uses relative URLs (just /api) in web mode, allowing the proxy to handle routing and making requests appear same-origin. - Web mode: Use relative URLs for proxy routing (no CORS issues) - Electron mode: Continue using hardcoded localhost:3008 This allows the Vite proxy configuration to actually work in web mode. Fixes: Persistent CORS errors in web mode development Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The CORS check was too strict for local development. Changed to: - Parse origin URL properly to extract hostname - Allow all localhost origins (any port) - Allow all 127.0.0.1 origins (loopback IP) - Allow all private network IPs (192.168.x.x, 10.x.x.x, 172.x.x.x) - Keep security by rejecting unknown origins This fixes CORS errors when accessing from http://localhost:3007 or other local addresses during web mode development. Fixes: "Not allowed by CORS" errors in web mode Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Added detailed logging to see: - What origin is being sent - How the hostname is parsed - Why origins are being accepted/rejected This will help us understand why CORS is still failing in web mode. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The web mode launcher was setting CORS_ORIGIN to only include the system hostname and 127.0.0.1, but users access via http://localhost:3007 which wasn't in the allowed list. Now includes: - http://localhost:3007 (primary dev URL) - http://$HOSTNAME:3007 (system hostname if needed) - http://127.0.0.1:3007 (loopback IP) Also cleaned up debug logging from CORS check since root cause is now clear. Fixes: Persistent "Not allowed by CORS" errors in web mode Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Enables ws: true for /api proxy to properly forward WebSocket connections through the development server in web mode. This ensures real-time features work correctly when developing in browser mode. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…urvival Web mode sessions were being lost on page reload because the session token was stored only in memory (cachedSessionToken). When the page reloaded, the token was cleared and verifySession() would fail, redirecting users to login. This commit adds localStorage persistence for the session token, ensuring: 1. Token survives page reloads in web mode 2. verifySession() can use the persisted token from localStorage 3. Token is cleared properly on logout 4. Graceful fallback if localStorage is unavailable (SSR, disabled storage) The HTTP-only cookie alone isn't sufficient for web mode due to SameSite cookie restrictions and potential proxy issues with credentials forwarding. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
When a project fails to initialize because the directory no longer exists (e.g., test artifacts, deleted folders), automatically remove it from the project list instead of showing the error repeatedly on every reload. This prevents users from being stuck with broken project references in their settings after testing or when project directories are moved/deleted. The user is notified with a toast message explaining the removal. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
In web mode, image loads may not send session cookies due to proxy/CORS restrictions. This adds the session token as a query parameter to ensure images load correctly with proper authentication in web mode. Fixes custom project icons and images not loading in web mode. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Projects are critical data that must persist across mode switches (Electron/web). Previously, project changes were debounced by 1 second, which could cause data loss if: 1. User switched from Electron to web mode quickly 2. App closed before debounce timer fired 3. Network temporarily unavailable during debounce window This change makes project array changes sync immediately (syncNow) instead of using the 1-second debounce, ensuring projects are always persisted to the server right away and visible in both Electron and web modes. Fixes issue where projects opened in Electron didn't appear in web mode. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
When switching between Electron and web modes or when the server temporarily stops, web mode was falling back to stale localStorage data instead of fresh server data. This fix: 1. Updates localStorage cache whenever fresh server settings are fetched 2. Updates localStorage cache whenever settings are synced to server 3. Prioritizes fresh settings cache over old Zustand persisted storage This ensures that: - Web mode always sees the latest projects even after mode switches - Switching from Electron to web mode immediately shows new projects - Server restarts don't cause web mode to use stale cached data Fixes issue where projects opened in Electron didn't appear in web mode after stopping and restarting the server. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
CRITICAL FIX: Electron and web mode were using DIFFERENT data directories: - Electron: Docker volume 'automaker-data' (isolated from host) - Web: Local ./data directory (host filesystem) This caused projects opened in Electron to never appear in web mode because they were synced to a completely separate Docker volume. Solution: Mount the host's ./data directory into both containers This ensures Electron and web mode always share the same data directory and all projects are immediately visible across modes. Now when you: 1. Open projects in Electron → synced to ./data 2. Switch to web mode → loads from same ./data 3. Restart server → both see the same projects Fixes issue where projects opened in Electron don't appear in web mode. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
CRITICAL: Electron was using ~/.config/@automaker/app/data/ while web mode
used ./data/, causing projects to never sync between modes.
In development mode, both now use the shared project root ./data directory.
In production, Electron uses its isolated userData directory for app portability.
This ensures:
- Electron projects sync to the same server data directory as web mode
- Projects opened in Electron immediately appear in web mode
- Server restart doesn't lose projects from either mode
The issue was on line 487 where DATA_DIR was set to app.getPath('userData')
instead of the shared project ./data directory.
Fixes the fundamental problem where projects never appeared in web mode
even though they were in the server's settings file.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This commit fixes bidirectional data synchronization between Electron and Web modes by addressing multiple interconnected issues: **Core Fixes:** 1. **Electron userData Path (main.ts)** - Explicitly set userData path in development using app.setPath() - Navigate from __dirname to project root instead of relying on process.cwd() - Ensures Electron reads from /data instead of ~/.config/Automaker 2. **Server DataDir Path (main.ts, start-automaker.sh)** - Fixed startServer() to use __dirname for reliable path calculation - Export DATA_DIR environment variable in start-automaker.sh - Server now consistently uses shared /data directory 3. **Settings Sync Protection (settings-service.ts)** - Modified wipe protection to distinguish legitimate removals from accidents - Allow empty projects array if trashedProjects has items - Prevent false-positive wipe detection when removing projects 4. **Diagnostics & Logging** - Enhanced cache loading logging in use-settings-migration.ts - Detailed migration decision logs for troubleshooting - Track project counts from both cache and server **Impact:** - Projects created in Electron now appear in Web mode after restart - Projects removed in Web mode stay removed in Electron after restart - Settings changes sync bidirectionally across mode switches - No more data loss or project duplication issues **Testing:** - Verified Electron uses /home/dhanush/Projects/automaker/data - Confirmed server startup logs show correct DATA_DIR - Tested project persistence across mode restarts - Validated no writes to ~/.config/Automaker in dev mode Fixes: Data persistence between Electron and Web modes Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR enhances data persistence, logging, and resilience across server startup, settings synchronization, and client-side session management. It introduces project wipe protection, cache-first settings retrieval, session token persistence via localStorage, unified API routing through Vite proxy, and standardized data directories across development, production, and web modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UIApp as UI App<br/>(Settings Sync)
participant Cache as localStorage<br/>(automaker-settings-cache)
participant Server
Client->>UIApp: Monitor store changes
UIApp->>UIApp: Detect project/field change
alt Immediate Sync (Projects Changed)
UIApp->>Server: POST /settings<br/>(immediate sync)
else Per-Field Debounced Sync
UIApp->>Server: POST /settings<br/>(scheduled after delay)
end
Server->>Server: updateGlobalSettings<br/>(with wipe protection)
Server-->>UIApp: Success response<br/>(updated settings)
UIApp->>Cache: Update localStorage cache<br/>(automaker-settings-cache)
UIApp->>UIApp: Store updated settings
sequenceDiagram
participant Client
participant UIApp as UI App<br/>(Migration)
participant Cache as localStorage<br/>(automaker-settings-cache)
participant Server
Client->>UIApp: Initialize app
UIApp->>Cache: Check automaker-settings-cache
alt Cache Hit
Cache-->>UIApp: Return cached GlobalSettings
UIApp->>UIApp: Use cached data
else Cache Miss/Parse Fail
UIApp->>Cache: Fall back to legacy keys<br/>(automaker-storage, etc.)
Cache-->>UIApp: Legacy settings
UIApp->>Server: Fetch server settings
Server-->>UIApp: Server data
alt localStorageMigrated Flag Set
UIApp->>UIApp: Skip migration<br/>(MIGRATION_SKIP)
else First Migration
UIApp->>UIApp: mergeSettings<br/>(server + localStorage)
UIApp->>Server: POST /settings<br/>(persisted merged data)
Server-->>UIApp: Confirm with flag
UIApp->>Cache: Update cache<br/>(with merged data)
else No Migration Needed
UIApp->>Server: Mark migrated<br/>(prevent future runs)
end
end
UIApp->>UIApp: Complete initialization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 @DhanushSantosh, 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 addresses critical data persistence and synchronization issues between the Electron and web modes of the application. It ensures that projects and settings are consistently shared and persisted across mode switches and restarts, resolving previous problems where data created in one mode was not visible in the other. This is achieved through unified data directory management, refined settings synchronization, and improved handling of project removals, leading to a more reliable and seamless user experience across different application environments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces critical fixes for data persistence issues between Electron and Web modes, which is a significant improvement. The changes are well-described and cover multiple areas, from path handling and environment variables to client-side caching and wipe protection. The introduction of a dedicated automaker-settings-cache in localStorage is a smart way to keep modes in sync.
My review includes suggestions to:
- Refine the wipe protection logic to be more robust against edge cases.
- Ensure consistent and reliable path resolution in the Electron main process.
- Improve logging for consistency and to reduce noise in production.
- Minor code cleanups.
Overall, this is a solid PR that addresses a core data integrity problem.
| const newTrashedProjectsLen = Array.isArray(sanitizedUpdates.trashedProjects) | ||
| ? sanitizedUpdates.trashedProjects.length | ||
| : Array.isArray(current.trashedProjects) | ||
| ? current.trashedProjects.length | ||
| : 0; | ||
|
|
||
| if ( | ||
| Array.isArray(sanitizedUpdates.projects) && | ||
| sanitizedUpdates.projects.length === 0 && | ||
| currentProjectsLen > 0 | ||
| ) { | ||
| attemptedProjectWipe = true; | ||
| delete sanitizedUpdates.projects; | ||
| // Only treat as accidental wipe if trashedProjects is also empty | ||
| // (If projects are moved to trash, they appear in trashedProjects) | ||
| if (newTrashedProjectsLen === 0) { | ||
| logger.warn( | ||
| '[WIPE_PROTECTION] Attempted to set projects to empty array with no trash! Ignoring update.', | ||
| { | ||
| currentProjectsLen, | ||
| newProjectsLen: 0, | ||
| newTrashedProjectsLen, | ||
| currentProjects: current.projects?.map((p) => p.name), | ||
| } | ||
| ); | ||
| attemptedProjectWipe = true; | ||
| delete sanitizedUpdates.projects; | ||
| } else { | ||
| logger.info('[LEGITIMATE_REMOVAL] Removing all projects to trash', { | ||
| currentProjectsLen, | ||
| newProjectsLen: 0, | ||
| movedToTrash: newTrashedProjectsLen, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for newTrashedProjectsLen seems flawed. It falls back to the length of current.trashedProjects if sanitizedUpdates.trashedProjects is not provided. This could lead to incorrect behavior. For example, if an update sends projects: [] without a trashedProjects field, and there are existing trashed projects, this will be incorrectly identified as a legitimate removal instead of a potential wipe, leading to data loss.
A safer approach is to consider a removal legitimate only if the number of trashed projects increases in the same update.
const currentTrashedProjectsLen = current.trashedProjects?.length ?? 0;
if (
Array.isArray(sanitizedUpdates.projects) &&
sanitizedUpdates.projects.length === 0 &&
currentProjectsLen > 0
) {
// A legitimate removal of all projects should be accompanied by an increase in trashed projects.
// If `trashedProjects` is not in the update or is not increasing, it's likely an accidental wipe.
if (
!Array.isArray(sanitizedUpdates.trashedProjects) ||
sanitizedUpdates.trashedProjects.length <= currentTrashedProjectsLen
) {
logger.warn(
'[WIPE_PROTECTION] Attempted to set projects to empty array without moving items to trash! Ignoring update.',
{
currentProjectsLen,
newProjectsLen: 0,
currentTrashedProjectsLen,
newTrashedProjectsLen: sanitizedUpdates.trashedProjects?.length ?? 'n/a',
currentProjects: current.projects?.map((p) => p.name),
}
);
attemptedProjectWipe = true;
delete sanitizedUpdates.projects;
} else {
logger.info('[LEGITIMATE_REMOVAL] Removing all projects to trash', {
currentProjectsLen,
newProjectsLen: 0,
movedToTrash: sanitizedUpdates.trashedProjects.length - currentTrashedProjectsLen,
});
}
}| const mainProcessDataDir = app.isPackaged | ||
| ? app.getPath('userData') | ||
| : path.join(process.cwd(), 'data'); |
There was a problem hiding this comment.
The logic for mainProcessDataDir uses process.cwd(), which can be unreliable depending on where the application is launched from. This seems to contradict the fix description in the PR, which mentions replacing process.cwd() with __dirname for reliability. To ensure consistency and robustness, you should use the same __dirname-based path calculation as in the startServer function.
const mainProcessDataDir = app.isPackaged
? app.getPath('userData')
: path.join(__dirname, '../../..', 'data');| logger.info('[SERVER_STARTUP] process.env.DATA_DIR:', process.env.DATA_DIR); | ||
| logger.info('[SERVER_STARTUP] Resolved DATA_DIR:', DATA_DIR); | ||
| logger.info('[SERVER_STARTUP] process.cwd():', process.cwd()); |
There was a problem hiding this comment.
These startup logs are helpful for debugging but might be too verbose for production environments. Consider changing them to logger.debug or removing them if they are temporary.
| logger.info('[SERVER_STARTUP] process.env.DATA_DIR:', process.env.DATA_DIR); | |
| logger.info('[SERVER_STARTUP] Resolved DATA_DIR:', DATA_DIR); | |
| logger.info('[SERVER_STARTUP] process.cwd():', process.cwd()); | |
| logger.debug('[SERVER_STARTUP] process.env.DATA_DIR:', process.env.DATA_DIR); | |
| logger.debug('[SERVER_STARTUP] Resolved DATA_DIR:', DATA_DIR); | |
| logger.debug('[SERVER_STARTUP] process.cwd():', process.cwd()); |
| logger.info( | ||
| `[SERVER_SETTINGS_UPDATE] Request received: projects=${projectsLen ?? 'n/a'}, trashedProjects=${trashedLen ?? 'n/a'}, theme=${ | ||
| (updates as any).theme ?? 'n/a' | ||
| }, localStorageMigrated=${(updates as any).localStorageMigrated ?? 'n/a'}` | ||
| ); | ||
|
|
||
| logger.info('[SERVER_SETTINGS_UPDATE] Calling updateGlobalSettings...'); | ||
| const settings = await settingsService.updateGlobalSettings(updates); | ||
| logger.info( | ||
| '[SERVER_SETTINGS_UPDATE] Update complete, projects count:', | ||
| settings.projects?.length ?? 0 | ||
| ); |
There was a problem hiding this comment.
These informational logs are useful for debugging but could add noise to production logs. It would be better to use logger.debug for this level of detail, especially for the request received and the call to updateGlobalSettings.
| if (!project) { | ||
| console.warn('[MOVE_TO_TRASH] Project not found:', projectId); | ||
| return; | ||
| } | ||
|
|
||
| console.log('[MOVE_TO_TRASH] Moving project to trash:', { | ||
| projectId, | ||
| projectName: project.name, | ||
| currentProjectCount: get().projects.length, | ||
| }); |
There was a problem hiding this comment.
For consistent logging throughout the application, it's better to use the shared logger instance instead of console.warn and console.log. This ensures that log levels and formatting are handled uniformly.
| if (!project) { | |
| console.warn('[MOVE_TO_TRASH] Project not found:', projectId); | |
| return; | |
| } | |
| console.log('[MOVE_TO_TRASH] Moving project to trash:', { | |
| projectId, | |
| projectName: project.name, | |
| currentProjectCount: get().projects.length, | |
| }); | |
| if (!project) { | |
| logger.warn('[MOVE_TO_TRASH] Project not found:', projectId); | |
| return; | |
| } | |
| logger.info('[MOVE_TO_TRASH] Moving project to trash:', { | |
| projectId, | |
| projectName: project.name, | |
| currentProjectCount: get().projects.length, | |
| }); |
| console.log('[MOVE_TO_TRASH] Updating store with new state:', { | ||
| newProjectCount: remainingProjects.length, | ||
| newTrashedCount: [trashedProject, ...existingTrash].length, | ||
| }); |
There was a problem hiding this comment.
For consistent logging, please use logger.info here instead of console.log.
| console.log('[MOVE_TO_TRASH] Updating store with new state:', { | |
| newProjectCount: remainingProjects.length, | |
| newTrashedCount: [trashedProject, ...existingTrash].length, | |
| }); | |
| logger.info('[MOVE_TO_TRASH] Updating store with new state:', { | |
| newProjectCount: remainingProjects.length, | |
| newTrashedCount: [trashedProject, ...existingTrash].length, | |
| }); |
| logger.warn( | ||
| '[DEVELOPMENT] Failed to set userData path, using fallback:', | ||
| (error as Error).message | ||
| ); | ||
| userDataPathToUse = path.join(projectRoot, 'data'); | ||
| } |
There was a problem hiding this comment.
The assignment userDataPathToUse = path.join(projectRoot, 'data'); inside the catch block is redundant, as userDataPathToUse is already assigned this value before the try block. You can safely remove this line.
logger.warn(
'[DEVELOPMENT] Failed to set userData path, using fallback:',
(error as Error).message
);
// The line below is redundant as userDataPathToUse is already set.
// userDataPathToUse = path.join(projectRoot, 'data');![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@apps/server/src/index.ts`:
- Around line 181-200: The CORS origin check in the URL parsing block
incorrectly allows all 172.* addresses; update the condition near where hostname
is derived (variable hostname inside the try block handling origin and calling
callback) to only permit RFC1918 172.16.0.0/12 addresses by parsing the second
octet and ensuring it is between 16 and 31 inclusive before calling
callback(null, origin); keep the existing checks for 'localhost', '127.0.0.1',
'::1', '0.0.0.0', and '192.168.' and ensure URL parse errors remain ignored.
In `@apps/server/src/services/settings-service.ts`:
- Around line 275-309: The wipe-protection currently treats an omitted
sanitizedUpdates.trashedProjects as legitimate removal by falling back to
current.trashedProjects; change the logic so only an explicit
sanitizedUpdates.trashedProjects array counts as moved-to-trash. In practice,
compute newTrashedProjectsLen using only
Array.isArray(sanitizedUpdates.trashedProjects) ?
sanitizedUpdates.trashedProjects.length : 0 (do not fall back to
current.trashedProjects), and keep the rest of the check that if
sanitizedUpdates.projects is an empty array and current.projects had items and
newTrashedProjectsLen === 0 then log the wipe, set attemptedProjectWipe = true
and delete sanitizedUpdates.projects. This ensures that omission of
trashedProjects is treated as zero rather than inheriting current state.
In `@apps/ui/src/lib/api-fetch.ts`:
- Around line 188-194: The middleware checkAuthentication currently only
validates req.query.apiKey; update it to also check for a token query param and
validate it via validateSession() (mirror the apiKey flow) so image requests
carrying the client-side token (added by getSessionToken()) are accepted;
specifically, in checkAuthentication, when req.query.token is present call
validateSession(req.query.token) and proceed on success, otherwise reject the
request with the same error handling used for invalid apiKey.
In `@apps/ui/src/lib/http-api-client.ts`:
- Around line 159-164: The Electron detection is wrong: replace the check using
window.electron with the correct exposed API or helper; in the function in
http-api-client.ts that currently uses if (!window.electron) return '', change
it to use if (!window.electronAPI) return '' or call the isElectronMode() helper
instead (update any associated logic that relies on window.electron to use
window.electronAPI or isElectronMode()) so Electron mode is detected correctly.
In `@apps/ui/src/main.ts`:
- Around line 714-719: The current mainProcessDataDir calculation uses
process.cwd() for dev which can be unstable; instead use the already computed
userDataPathToUse (or the project-root dataDir variable) so the main-process
DATA_DIR is consistent. Update the ternary that assigns mainProcessDataDir
(where app.isPackaged is checked) to use userDataPathToUse (or the existing
dataDir/project root path) in the development branch, then set
process.env.DATA_DIR and log via logger.info exactly as before; ensure
references to mainProcessDataDir, app.isPackaged, process.env.DATA_DIR, and
logger.info remain unchanged.
In `@apps/ui/vite.config.mts`:
- Around line 71-77: Update the Vite dev server proxy configuration for the
'/api' entry: change the target from 'http://localhost:3008' to
'ws://localhost:3008' to allow proper WebSocket protocol upgrades, and if your
backend enforces Origin header checks add a custom proxy hook (e.g., onProxyReq
or configureProxy in the devServer/proxy options) to rewrite the Origin header;
locate the proxy object for the '/api' key in vite.config.mts to make these
edits.
In `@docker-compose.dev-server.yml`:
- Around line 62-65: The host bind-mount ./data:/data can be created as root and
block the server (running as user automaker) from writing; fix it by ensuring
/data exists and is owned by automaker before dropping privileges: update the
container startup (entrypoint script or Dockerfile startup step referenced by
the mount './data:/data') to run mkdir -p /data and chown -R automaker:automaker
/data (or use the automaker UID/GID) and set appropriate permissions (e.g.,
chmod) before switching to the automaker user so the bind-mounted directory is
writable.
In `@start-automaker.sh`:
- Around line 1078-1079: Ensure the DATA_DIR directory exists before the server
starts: when exporting DATA_DIR (export DATA_DIR="$SCRIPT_DIR/data") add logic
to check/create that path in web mode so the directory is created with safe
permissions (e.g., mkdir -p and set umask/permissions) before launching the
server; update the startup sequence around the DATA_DIR export and any startup
entrypoint that uses DATA_DIR to rely on the created directory rather than
assuming it already exists.
♻️ Duplicate comments (1)
docker-compose.dev.yml (1)
63-65: Same /data ownership concern as the dev-server compose file.Bind-mounting
./datahas the same write-permission risk when the host directory doesn’t exist yet. Please ensure/datais created/chowned before running asautomaker.
🧹 Nitpick comments (4)
apps/ui/src/hooks/use-settings-sync.ts (1)
366-371: Consider reducing verbosity of project name logging.Logging full project name arrays on every change could produce noisy logs. Consider using
logger.debuginstead oflogger.info, or logging only the count difference rather than full name arrays.♻️ Suggested change
- logger.info('[PROJECTS_CHANGED] Projects array changed, syncing immediately', { + logger.debug('[PROJECTS_CHANGED] Projects array changed, syncing immediately', { prevCount: prevState.projects?.length ?? 0, newCount: newState.projects?.length ?? 0, - prevProjects: prevState.projects?.map((p) => p.name) ?? [], - newProjects: newState.projects?.map((p) => p.name) ?? [], });apps/ui/src/hooks/use-settings-migration.ts (1)
439-447: Prefer the storage helper for cache writes.Using
setItemkeeps storage-availability checks consistent and avoids directlocalStorageaccess surprises (e.g., restricted storage contexts).♻️ Proposed refactor
- localStorage.setItem('automaker-settings-cache', JSON.stringify(serverSettings)); + setItem('automaker-settings-cache', JSON.stringify(serverSettings));apps/ui/src/store/app-store.ts (1)
1506-1532: Prefer structured logger over console. for store mutation logs.*
console.*bypasses the existing logger and makes log routing inconsistent. Consider usinglogger.warn/infowith structured payloads.♻️ Suggested refactor
- if (!project) { - console.warn('[MOVE_TO_TRASH] Project not found:', projectId); - return; - } + if (!project) { + logger.warn('[MOVE_TO_TRASH] Project not found', { projectId }); + return; + } - console.log('[MOVE_TO_TRASH] Moving project to trash:', { + logger.info('[MOVE_TO_TRASH] Moving project to trash', { projectId, projectName: project.name, currentProjectCount: get().projects.length, }); - console.log('[MOVE_TO_TRASH] Updating store with new state:', { + logger.info('[MOVE_TO_TRASH] Updating store with new state', { newProjectCount: remainingProjects.length, newTrashedCount: [trashedProject, ...existingTrash].length, });apps/ui/src/components/views/dashboard-view.tsx (1)
126-175: Consider replacing string-match error handling with a structured error code.
initResult.error?.includes('does not exist')is brittle and can misclassify errors. IfinitializeProjectcan return an error code (e.g.,ENOENT) or typed error, prefer that for safer detection.
| // For local development, allow all localhost/loopback origins (any port) | ||
| try { | ||
| const url = new URL(origin); | ||
| const hostname = url.hostname; | ||
|
|
||
| if ( | ||
| hostname === 'localhost' || | ||
| hostname === '127.0.0.1' || | ||
| hostname === '::1' || | ||
| hostname === '0.0.0.0' || | ||
| hostname.startsWith('192.168.') || | ||
| hostname.startsWith('10.') || | ||
| hostname.startsWith('172.') | ||
| ) { | ||
| callback(null, origin); | ||
| return; | ||
| } | ||
| } catch (err) { | ||
| // Ignore URL parsing errors | ||
| } |
There was a problem hiding this comment.
Restrict 172. CORS allowance to RFC1918 only.*
172.0.0.0/8 includes public ranges; this currently allows non-private origins. Limit to 172.16.0.0/12.
🔧 Proposed fix
try {
const url = new URL(origin);
const hostname = url.hostname;
+ const isPrivate172 = /^172\.(1[6-9]|2\d|3[0-1])\./.test(hostname);
if (
hostname === 'localhost' ||
hostname === '127.0.0.1' ||
hostname === '::1' ||
hostname === '0.0.0.0' ||
hostname.startsWith('192.168.') ||
hostname.startsWith('10.') ||
- hostname.startsWith('172.')
+ isPrivate172
) {
callback(null, origin);
return;
}📝 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.
| // For local development, allow all localhost/loopback origins (any port) | |
| try { | |
| const url = new URL(origin); | |
| const hostname = url.hostname; | |
| if ( | |
| hostname === 'localhost' || | |
| hostname === '127.0.0.1' || | |
| hostname === '::1' || | |
| hostname === '0.0.0.0' || | |
| hostname.startsWith('192.168.') || | |
| hostname.startsWith('10.') || | |
| hostname.startsWith('172.') | |
| ) { | |
| callback(null, origin); | |
| return; | |
| } | |
| } catch (err) { | |
| // Ignore URL parsing errors | |
| } | |
| // For local development, allow all localhost/loopback origins (any port) | |
| try { | |
| const url = new URL(origin); | |
| const hostname = url.hostname; | |
| const isPrivate172 = /^172\.(1[6-9]|2\d|3[0-1])\./.test(hostname); | |
| if ( | |
| hostname === 'localhost' || | |
| hostname === '127.0.0.1' || | |
| hostname === '::1' || | |
| hostname === '0.0.0.0' || | |
| hostname.startsWith('192.168.') || | |
| hostname.startsWith('10.') || | |
| isPrivate172 | |
| ) { | |
| callback(null, origin); | |
| return; | |
| } | |
| } catch (err) { | |
| // Ignore URL parsing errors | |
| } |
🤖 Prompt for AI Agents
In `@apps/server/src/index.ts` around lines 181 - 200, The CORS origin check in
the URL parsing block incorrectly allows all 172.* addresses; update the
condition near where hostname is derived (variable hostname inside the try block
handling origin and calling callback) to only permit RFC1918 172.16.0.0/12
addresses by parsing the second octet and ensuring it is between 16 and 31
inclusive before calling callback(null, origin); keep the existing checks for
'localhost', '127.0.0.1', '::1', '0.0.0.0', and '192.168.' and ensure URL parse
errors remain ignored.
| const currentProjectsLen = Array.isArray(current.projects) ? current.projects.length : 0; | ||
| // Check if this is a legitimate project removal (moved to trash) vs accidental wipe | ||
| const newTrashedProjectsLen = Array.isArray(sanitizedUpdates.trashedProjects) | ||
| ? sanitizedUpdates.trashedProjects.length | ||
| : Array.isArray(current.trashedProjects) | ||
| ? current.trashedProjects.length | ||
| : 0; | ||
|
|
||
| if ( | ||
| Array.isArray(sanitizedUpdates.projects) && | ||
| sanitizedUpdates.projects.length === 0 && | ||
| currentProjectsLen > 0 | ||
| ) { | ||
| attemptedProjectWipe = true; | ||
| delete sanitizedUpdates.projects; | ||
| // Only treat as accidental wipe if trashedProjects is also empty | ||
| // (If projects are moved to trash, they appear in trashedProjects) | ||
| if (newTrashedProjectsLen === 0) { | ||
| logger.warn( | ||
| '[WIPE_PROTECTION] Attempted to set projects to empty array with no trash! Ignoring update.', | ||
| { | ||
| currentProjectsLen, | ||
| newProjectsLen: 0, | ||
| newTrashedProjectsLen, | ||
| currentProjects: current.projects?.map((p) => p.name), | ||
| } | ||
| ); | ||
| attemptedProjectWipe = true; | ||
| delete sanitizedUpdates.projects; | ||
| } else { | ||
| logger.info('[LEGITIMATE_REMOVAL] Removing all projects to trash', { | ||
| currentProjectsLen, | ||
| newProjectsLen: 0, | ||
| movedToTrash: newTrashedProjectsLen, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wipe protection can still allow accidental wipes when trashedProjects is omitted.
If current.trashedProjects is non‑empty and the UI sends projects: [] without trashedProjects, the fallback makes it look “legitimate” and allows the wipe. Require an explicit trashedProjects update to treat as a removal.
🔧 Proposed fix
- const newTrashedProjectsLen = Array.isArray(sanitizedUpdates.trashedProjects)
- ? sanitizedUpdates.trashedProjects.length
- : Array.isArray(current.trashedProjects)
- ? current.trashedProjects.length
- : 0;
+ const hasTrashedProjectsUpdate = Array.isArray(sanitizedUpdates.trashedProjects);
+ const newTrashedProjectsLen = hasTrashedProjectsUpdate
+ ? sanitizedUpdates.trashedProjects.length
+ : 0;
if (
Array.isArray(sanitizedUpdates.projects) &&
sanitizedUpdates.projects.length === 0 &&
currentProjectsLen > 0
) {
// Only treat as accidental wipe if trashedProjects is also empty
// (If projects are moved to trash, they appear in trashedProjects)
- if (newTrashedProjectsLen === 0) {
+ if (!hasTrashedProjectsUpdate || newTrashedProjectsLen === 0) {
logger.warn(
'[WIPE_PROTECTION] Attempted to set projects to empty array with no trash! Ignoring update.',
{
currentProjectsLen,
newProjectsLen: 0,🤖 Prompt for AI Agents
In `@apps/server/src/services/settings-service.ts` around lines 275 - 309, The
wipe-protection currently treats an omitted sanitizedUpdates.trashedProjects as
legitimate removal by falling back to current.trashedProjects; change the logic
so only an explicit sanitizedUpdates.trashedProjects array counts as
moved-to-trash. In practice, compute newTrashedProjectsLen using only
Array.isArray(sanitizedUpdates.trashedProjects) ?
sanitizedUpdates.trashedProjects.length : 0 (do not fall back to
current.trashedProjects), and keep the rest of the check that if
sanitizedUpdates.projects is an empty array and current.projects had items and
newTrashedProjectsLen === 0 then log the wipe, set attemptedProjectWipe = true
and delete sanitizedUpdates.projects. This ensures that omission of
trashedProjects is treated as zero rather than inheriting current state.
|
|
||
| // Web mode: also add session token as query param for image loads | ||
| // This ensures images load correctly even if cookies aren't sent (e.g., cross-origin proxy scenarios) | ||
| const sessionToken = getSessionToken(); | ||
| if (sessionToken) { | ||
| params.set('token', sessionToken); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the actual code at lines 188-194 in the target file
head -200 apps/ui/src/lib/api-fetch.ts | tail -20Repository: AutoMaker-Org/automaker
Length of output: 657
🏁 Script executed:
# Check the imports in the file for coding guideline compliance
head -30 apps/ui/src/lib/api-fetch.tsRepository: AutoMaker-Org/automaker
Length of output: 1163
🏁 Script executed:
# Search for the server-side /api/fs/image endpoint implementation
find . -name "*.ts" -o -name "*.js" | xargs grep -l "fs/image" 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 97
🏁 Script executed:
# Check tsconfig.json for `@automaker/`* path mappings
cat tsconfig.json 2>/dev/null || cat apps/ui/tsconfig.json 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 589
🏁 Script executed:
# Find the http-api-client module location
find . -name "http-api-client*" -type fRepository: AutoMaker-Org/automaker
Length of output: 103
🏁 Script executed:
# Search for server-side /api/fs/image endpoint
rg -l "fs/image|fs.*image" --type=ts --type=js apps/server 2>/dev/null | head -10Repository: AutoMaker-Org/automaker
Length of output: 116
🏁 Script executed:
# Search for image route/endpoint in server more broadly
rg "image" apps/server --type=ts -l | grep -E "(route|handler|controller|endpoint)" | head -10Repository: AutoMaker-Org/automaker
Length of output: 556
🏁 Script executed:
# Search for token parameter handling in server
rg "params.*token|query.*token|token.*param" apps/server --type=ts -A 3 -B 3 | head -50Repository: AutoMaker-Org/automaker
Length of output: 1036
🏁 Script executed:
# Find server routes/handlers directory
find apps/server -name "*route*" -o -name "*handler*" -o -name "*endpoint*" -type f | head -20Repository: AutoMaker-Org/automaker
Length of output: 845
🏁 Script executed:
# Examine the image endpoint implementation
cat apps/server/src/routes/fs/routes/image.tsRepository: AutoMaker-Org/automaker
Length of output: 2079
🏁 Script executed:
# Check the fs route index to see if authentication middleware is applied
cat apps/server/src/routes/fs/index.tsRepository: AutoMaker-Org/automaker
Length of output: 2042
🏁 Script executed:
# Search for authentication middleware that validates token
rg "token|authenticate" apps/server/src/routes/fs --type=ts -B 5 -A 5 | head -100Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Check the main route registration and middleware setup
head -100 apps/server/src/index.ts | grep -A 50 "router\|middleware\|auth"Repository: AutoMaker-Org/automaker
Length of output: 3495
🏁 Script executed:
# Check the authMiddleware implementation
cat apps/server/src/lib/auth.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 3248
🏁 Script executed:
# See how fs routes are registered in main index.ts
rg "createFsRoutes" apps/server/src/index.ts -A 5 -B 5Repository: AutoMaker-Org/automaker
Length of output: 1284
🏁 Script executed:
# Get the authMiddleware function implementation
rg "export.*authMiddleware|function authMiddleware" apps/server/src/lib/auth.ts -A 50Repository: AutoMaker-Org/automaker
Length of output: 1285
🏁 Script executed:
# Search for token query parameter validation
rg "req.query.*token|token.*query" apps/server/src/lib/auth.ts -A 5 -B 5Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Find the checkAuthentication function
rg "function checkAuthentication|export.*checkAuthentication" apps/server/src/lib/auth.ts -A 80 | head -150Repository: AutoMaker-Org/automaker
Length of output: 2548
Server-side token validation is missing - critical fix required.
The client correctly adds a token query parameter with the session token for image authentication (lines 188-194). However, the server's checkAuthentication function only validates the apiKey query parameter, not the token parameter. The middleware needs to check for and validate req.query.token using validateSession(), similar to how it validates apiKey.
🤖 Prompt for AI Agents
In `@apps/ui/src/lib/api-fetch.ts` around lines 188 - 194, The middleware
checkAuthentication currently only validates req.query.apiKey; update it to also
check for a token query param and validate it via validateSession() (mirror the
apiKey flow) so image requests carrying the client-side token (added by
getSessionToken()) are accepted; specifically, in checkAuthentication, when
req.query.token is present call validateSession(req.query.token) and proceed on
success, otherwise reject the request with the same error handling used for
invalid apiKey.
|
|
||
| // In web mode (not Electron), use relative URL to leverage Vite proxy | ||
| // This avoids CORS issues since requests appear same-origin | ||
| if (!window.electron) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "window\.electron" --type=ts -C 2Repository: AutoMaker-Org/automaker
Length of output: 3889
🏁 Script executed:
# Also search for where these properties are defined/injected
rg -n "electronAPI\|electron\s*=" --type=ts --type=js -C 2 | head -100Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Check the preload script for Electron setup
find . -name "*preload*" -type f | head -20Repository: AutoMaker-Org/automaker
Length of output: 91
🏁 Script executed:
# Check the preload.ts file to see what's exposed via contextBridge
cat -n ./apps/ui/src/preload.tsRepository: AutoMaker-Org/automaker
Length of output: 2966
🏁 Script executed:
# Get full context around line 162 to understand the logic
sed -n '155,170p' apps/ui/src/lib/http-api-client.tsRepository: AutoMaker-Org/automaker
Length of output: 552
Use window.electronAPI or isElectronMode() helper for Electron detection at line 162.
The preload script exposes the API as electronAPI, not electron. Line 162 checks !window.electron, which is always undefined. Replace with !window.electronAPI or the established isElectronMode() helper to correctly detect whether the app is running in Electron mode.
Code location
// Line 162 - INCORRECT
if (!window.electron) {
return '';
}
// Should be either:
if (!window.electronAPI) {
return '';
}
// Or:
if (!isElectronMode()) {
return '';
}🤖 Prompt for AI Agents
In `@apps/ui/src/lib/http-api-client.ts` around lines 159 - 164, The Electron
detection is wrong: replace the check using window.electron with the correct
exposed API or helper; in the function in http-api-client.ts that currently uses
if (!window.electron) return '', change it to use if (!window.electronAPI)
return '' or call the isElectronMode() helper instead (update any associated
logic that relies on window.electron to use window.electronAPI or
isElectronMode()) so Electron mode is detected correctly.
| // Use the project's shared data directory in development, userData in production | ||
| const mainProcessDataDir = app.isPackaged | ||
| ? app.getPath('userData') | ||
| : path.join(process.cwd(), 'data'); | ||
| process.env.DATA_DIR = mainProcessDataDir; | ||
| logger.info('[MAIN_PROCESS_DATA_DIR]', mainProcessDataDir); |
There was a problem hiding this comment.
Avoid process.cwd() for main-process DATA_DIR.
In dev, process.cwd() can be apps/ui or another working dir, which can reintroduce the wrong data path. Use the already computed userDataPathToUse (or the same project-root dataDir) to keep consistency.
🔧 Proposed fix
- const mainProcessDataDir = app.isPackaged
- ? app.getPath('userData')
- : path.join(process.cwd(), 'data');
+ const mainProcessDataDir = userDataPathToUse;
process.env.DATA_DIR = mainProcessDataDir;🤖 Prompt for AI Agents
In `@apps/ui/src/main.ts` around lines 714 - 719, The current mainProcessDataDir
calculation uses process.cwd() for dev which can be unstable; instead use the
already computed userDataPathToUse (or the project-root dataDir variable) so the
main-process DATA_DIR is consistent. Update the ternary that assigns
mainProcessDataDir (where app.isPackaged is checked) to use userDataPathToUse
(or the existing dataDir/project root path) in the development branch, then set
process.env.DATA_DIR and log via logger.info exactly as before; ensure
references to mainProcessDataDir, app.isPackaged, process.env.DATA_DIR, and
logger.info remain unchanged.
| proxy: { | ||
| '/api': { | ||
| target: 'http://localhost:3008', | ||
| changeOrigin: true, | ||
| ws: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "package.json" -path "*/ui/*" | head -5Repository: AutoMaker-Org/automaker
Length of output: 89
🌐 Web query:
Vite dev server proxy ws changeOrigin options 2025 2026
💡 Result:
Short answer
- Vite's dev-server proxy uses the Node http-proxy/http-proxy-middleware stack. For WebSocket proxying you must set ws: true and use a ws:// or wss:// target. [1][2]
What changeOrigin does (2024–2026)
- changeOrigin primarily rewrites the Host header to match the target (HTTP/1.1 Host). It does not reliably rewrite the Origin header in many current http-proxy versions — so changeOrigin alone may not satisfy backends that check the Origin header on WebSocket upgrade requests. This behavior has been discussed and reported in upstream issues. [3][4]
Practical notes / workarounds
- For WebSockets in vite.config.js:
- server.proxy['/path'] = { target: 'ws://host:port', ws: true, changeOrigin: true } to enable upgrade handling. [1][2]
- If your backend rejects the WS upgrade because Origin is wrong, explicitly set or rewrite headers (Origin/Host) via a custom proxy hook (onProxyReq/onUpgrade) or use a small custom proxy that calls proxy.upgrade — do not rely only on changeOrigin. See http-proxy/http-proxy-middleware docs and issues for examples. [3][5]
- If you see Vite HMR falling back or failing behind reverse proxies, check server.hmr options (clientPort/port) or ensure the reverse proxy supports WebSocket upgrades. [1][6]
Sources
- Vite server proxy / HMR docs. [1]
- Vite GitHub issues on WebSocket proxying and related bugs. [2][4]
- node-http-proxy / http-proxy-middleware discussion about changeOrigin vs Origin header. [3][5]
🏁 Script executed:
cat apps/ui/package.json | grep -A 2 -B 2 viteRepository: AutoMaker-Org/automaker
Length of output: 2065
Consider updating the proxy target to use WebSocket protocol for proper handling.
The ws and changeOrigin options are valid for Vite 7.3.0. However, for WebSocket proxying, the target should ideally be 'ws://localhost:3008' instead of 'http://localhost:3008' to ensure proper protocol upgrade handling. Additionally, if your backend validates the Origin header on WebSocket upgrade requests, be aware that changeOrigin alone may not be sufficient—you may need custom proxy hooks to rewrite headers explicitly.
🤖 Prompt for AI Agents
In `@apps/ui/vite.config.mts` around lines 71 - 77, Update the Vite dev server
proxy configuration for the '/api' entry: change the target from
'http://localhost:3008' to 'ws://localhost:3008' to allow proper WebSocket
protocol upgrades, and if your backend enforces Origin header checks add a
custom proxy hook (e.g., onProxyReq or configureProxy in the devServer/proxy
options) to rewrite the Origin header; locate the proxy object for the '/api'
key in vite.config.mts to make these edits.
| # IMPORTANT: Mount local ./data directory (not a Docker volume) | ||
| # This ensures Electron and web mode share the same data directory | ||
| # and projects opened in either mode are visible in both | ||
| - ./data:/data |
There was a problem hiding this comment.
Ensure the bind-mounted /data is writable by the automaker user.
If ./data doesn’t exist, Docker will create it as root-owned on the host, and the server (running as automaker) may fail to write. Consider creating/chowning /data before dropping privileges.
🔧 Proposed fix (ensure /data exists and is writable)
- echo 'Installing dependencies...' &&
+ mkdir -p /data &&
+ chown -R automaker:automaker /data &&
+ echo 'Installing dependencies...' &&🤖 Prompt for AI Agents
In `@docker-compose.dev-server.yml` around lines 62 - 65, The host bind-mount
./data:/data can be created as root and block the server (running as user
automaker) from writing; fix it by ensuring /data exists and is owned by
automaker before dropping privileges: update the container startup (entrypoint
script or Dockerfile startup step referenced by the mount './data:/data') to run
mkdir -p /data and chown -R automaker:automaker /data (or use the automaker
UID/GID) and set appropriate permissions (e.g., chmod) before switching to the
automaker user so the bind-mounted directory is writable.
| export DATA_DIR="$SCRIPT_DIR/data" | ||
| export CORS_ORIGIN="http://localhost:$WEB_PORT,http://$HOSTNAME:$WEB_PORT,http://127.0.0.1:$WEB_PORT" |
There was a problem hiding this comment.
Create DATA_DIR before launching the server.
DATA_DIR now points to $SCRIPT_DIR/data; if it doesn’t exist, startup may fail or create it with unexpected permissions. Safer to create it explicitly in web mode.
🛠️ Proposed fix
- export DATA_DIR="$SCRIPT_DIR/data"
+ export DATA_DIR="$SCRIPT_DIR/data"
+ mkdir -p "$DATA_DIR"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export DATA_DIR="$SCRIPT_DIR/data" | |
| export CORS_ORIGIN="http://localhost:$WEB_PORT,http://$HOSTNAME:$WEB_PORT,http://127.0.0.1:$WEB_PORT" | |
| export DATA_DIR="$SCRIPT_DIR/data" | |
| mkdir -p "$DATA_DIR" | |
| export CORS_ORIGIN="http://localhost:$WEB_PORT,http://$HOSTNAME:$WEB_PORT,http://127.0.0.1:$WEB_PORT" |
🤖 Prompt for AI Agents
In `@start-automaker.sh` around lines 1078 - 1079, Ensure the DATA_DIR directory
exists before the server starts: when exporting DATA_DIR (export
DATA_DIR="$SCRIPT_DIR/data") add logic to check/create that path in web mode so
the directory is created with safe permissions (e.g., mkdir -p and set
umask/permissions) before launching the server; update the startup sequence
around the DATA_DIR export and any startup entrypoint that uses DATA_DIR to rely
on the created directory rather than assuming it already exists.
- Expanded docstrings in use-settings-migration.ts for parseLocalStorageSettings, localStorageHasMoreData, mergeSettings, and performSettingsMigration - Expanded docstrings in use-settings-sync.ts for getSettingsFieldValue and hasSettingsFieldChanged helper functions - Added detailed parameter and return value documentation - Improved clarity on migration flow and settings merging logic This brings docstring coverage from 77.78% to 80%+ to satisfy CodeRabbit checks. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
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/ui/src/hooks/use-settings-migration.ts (1)
125-145: Cache-first return masks newer legacy data, risking data loss during first-time migration.When
parseLocalStorageSettings()finds a staleautomaker-settings-cache, it returns immediately without checking the legacyautomaker-storage. If the user added projects offline (saved via Zustand persist) while the cache was stale, those unsynced projects are lost during migration. The migration hook'slocalStorageHasMoreData()comparison only sees the cache data due to the early return, so it cannot detect the newer legacy data.The fix approach is correct: defer the return and compare cache vs. legacy storage by project count completeness, choosing the more complete version. This ensures migration preserves unsynced offline data.
🤖 Fix all issues with AI agents
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 469-476: The code in useSettingsMigration currently calls
localStorage.setItem(...) directly when updating the 'automaker-settings-cache',
but the file otherwise uses the setItem helper for storage access; replace the
direct localStorage call with the setItem helper (invoke
setItem('automaker-settings-cache', JSON.stringify(serverSettings))) to preserve
consistent environment checks and error handling provided by setItem, and keep
the surrounding try/catch/logger usage aligned with other usages in this file
(see useSettingsMigration and the setItem helper references).
In `@apps/ui/tests/projects/open-existing-project.spec.ts`:
- Around line 159-166: The attribute selector using targetProjectName can break
if the name contains quotes; replace the brittle locator creation for
projectSwitcherButton with a safer match—use Playwright's role/text matching
(e.g., getByRole('button', { name: targetProjectName }) or
page.locator('button').filter({ hasText: targetProjectName }).first()) instead
of page.locator(`button[title="${targetProjectName}"]`), updating the code that
defines projectSwitcherButton so it reliably finds the button even when
targetProjectName contains special characters.
🧹 Nitpick comments (1)
apps/ui/src/hooks/use-settings-sync.ts (1)
380-389: Consider avoiding project-name logging at info level.Line 387-388 logs project names; if logs are persisted or shared, this can expose sensitive repo info. Prefer counts at info and move names to debug.
💡 Suggested tweak
- logger.info('[PROJECTS_CHANGED] Projects array changed, syncing immediately', { + logger.info('[PROJECTS_CHANGED] Projects array changed, syncing immediately', { prevCount: prevState.projects?.length ?? 0, newCount: newState.projects?.length ?? 0, - prevProjects: prevState.projects?.map((p) => p.name) ?? [], - newProjects: newState.projects?.map((p) => p.name) ?? [], }); + logger.debug('[PROJECTS_CHANGED] Project names', { + prevProjects: prevState.projects?.map((p) => p.name) ?? [], + newProjects: newState.projects?.map((p) => p.name) ?? [], + });Also applies to: 397-397
- Replace direct localStorage.setItem() with setItem helper in use-settings-migration.ts (line 472) for consistent storage-availability checks and error handling - Replace brittle attribute selector with Playwright's getByRole in open-existing-project.spec.ts (line 162) to handle names containing special characters Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
DhanushSantosh
added
Enhancement
1 participant
Summary
This PR fixes critical data persistence issues between Electron and Web modes, implements E2E test selector fixes, and enhances docstring coverage. Projects and settings created in one mode now correctly sync to the other mode, even after stopping and restarting either mode separately.
Key Issues Resolved:
Root Causes Identified
1. Path Navigation Bug
process.cwd()returned/apps/uiinstead of project rootapps/ui/datainstead of shared/data__dirnameto navigate up 3 levels to project root2. Electron's Default userData Not Overridden
~/.config/Automakerinstead of shared/dataapp.setPath()call in development modeapp.setPath('userData', projectRoot/data)in development3. Server Environment Variable Not Exported
DATA_DIRenvironment variable not passed to server in web modeDATA_DIRin start-automaker.sh4. Wipe Protection Logic Too Aggressive
5. E2E Test Selector Mismatch
project-switcher-project-{name}but component rendersproject-switcher-{id}button[title="{name}"]selector insteadChanges Made
apps/ui/src/main.ts (Electron Setup)
startServer()dataDir calculationstart-automaker.sh (Web Mode)
DATA_DIR=/home/dhanush/Projects/automaker/datafor serverapps/server/src/services/settings-service.ts (Wipe Protection)
apps/ui/src/hooks/use-settings-migration.ts (Settings Hydration)
apps/ui/src/hooks/use-settings-sync.ts (Settings Sync)
E2E Tests (Test Fixtures)
getByTestId('project-switcher-project-{name}')tolocator('button[title="{name}"]')Testing
Manual Tests Performed
✅ Electron startup logs show correct userData path
✅ Server startup logs show correct DATA_DIR
✅ Added test project in Web → verified persistence after mode restart
✅ Files preserved: Only
/data/settings.jsonmodified, not~/.config/Automaker✅ No regression: Existing projects still accessible in both modes
✅ E2E tests now correctly find project switcher elements
Test Plan for Code Review
Test 1 - Add in Web, See in Electron
Test 2 - Add in Electron, See in Web
Test 3 - Persist Across Restart Cycles
Test 4 - E2E Tests Pass
npm run test --workspace=apps/uiImpact
Commits Included
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.