[Feature] Add auto-update system with git-based version checking #290
Conversation
Summary of ChangesHello @amoscicki, 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 introduces a new, fully-featured auto-update system for the application. It enables users to automatically check for new versions from an upstream Git repository at configurable intervals, receive toast notifications when updates are available, and apply updates with a single click. The system is built with a flexible architecture that supports dependency injection, making it adaptable to different update mechanisms in the future. 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
|
|
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. 📝 WalkthroughWalkthroughAdds a Git-backed auto-update subsystem: server routes (/api/updates: check, pull, info) and git utilities; new update types and defaults; DI-capable updates Zustand store; polling hook and notifier UI; settings integration, dev proxy, HTTP/Electron API surfaces, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (Settings / Notifier)
participant Store as UpdatesStore
participant Poll as Polling Hook
participant API as HttpApiClient / ElectronAPI
participant Server as Backend (/api/updates)
participant Git as Git (local & remote)
Note over Poll, UI: manual or periodic check triggers
Poll->>API: check()
API->>Server: GET /api/updates/check
Server->>Git: add temp remote & fetch main
Git-->>Server: remote SHAs
Server-->>API: UpdateCheckResult
API->>Store: set updateAvailable & remoteVersion
Store->>UI: state update → show toast
UI->>Store: pullUpdates() (user action)
Store->>API: POST /api/updates/pull
API->>Server: POST /api/updates/pull
Server->>Git: fetch & git merge --ff-only
Git-->>Server: newVersion / alreadyUpToDate
Server-->>API: UpdatePullResult
API->>Store: fetchInfo() & emit updatePulled
Store->>UI: state update → show installed toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/server/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (2)apps/server/src/routes/updates/routes/pull.ts (2)
apps/ui/src/store/updates-store.ts (3)
🔇 Additional comments (10)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive auto-update system, which is a significant and well-architected feature. The use of dependency injection for the update client is excellent, allowing for future flexibility. The overall structure is clean, with good separation of concerns between UI, state management, and server-side logic. However, I've identified a critical security vulnerability related to command injection due to improper handling of user-configurable URLs. I've also included several medium-severity comments to improve maintainability, robustness, and consistency across the new modules. Addressing these points will make this great feature even better.
apps/ui/src/components/views/settings-view/updates/updates-section.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
215-226: Consider handling edge cases for the interval input.The current implementation silently ignores invalid input values. Users might be confused if typing doesn't update the field. Consider allowing the field to show the typed value and validating on blur, or providing visual feedback for invalid values.
apps/ui/src/hooks/use-update-polling.ts (1)
62-87: Consider stabilizing theonCheckcallback reference.Including
onCheckin the dependency array could cause the interval to reset on every render if the callback identity changes. While Zustand'scheckForUpdatesshould be stable, theoptions.onCheckoverride might not be. Consider usinguseCallbackoruseRefto stabilize the reference.🔎 Proposed fix using useRef
+import { useEffect, useRef, useCallback } from 'react'; -import { useEffect, useRef } from 'react'; import { useAppStore } from '@/store/app-store'; import { useUpdatesStore } from '@/store/updates-store'; // ... types ... export function useUpdatePolling(options: UseUpdatePollingOptions = {}): UseUpdatePollingResult { const { autoUpdate } = useAppStore(); const { checkForUpdates, lastChecked } = useUpdatesStore(); // Allow overrides for testing const isEnabled = options.enabled ?? autoUpdate.enabled; const intervalMinutes = options.intervalMinutes ?? autoUpdate.checkIntervalMinutes; - const onCheck = options.onCheck ?? checkForUpdates; + + // Stabilize the check function reference + const onCheckRef = useRef(options.onCheck ?? checkForUpdates); + onCheckRef.current = options.onCheck ?? checkForUpdates; + + const stableOnCheck = useCallback(() => onCheckRef.current(), []); const intervalRef = useRef<NodeJS.Timeout | null>(null); useEffect(() => { // Clear any existing interval if (intervalRef.current) { clearInterval(intervalRef.current); intervalRef.current = null; } // Don't set up polling if disabled if (!isEnabled) { return; } // Check immediately on enable - onCheck(); + stableOnCheck(); // Set up interval const intervalMs = intervalMinutes * 60 * 1000; - intervalRef.current = setInterval(onCheck, intervalMs); + intervalRef.current = setInterval(stableOnCheck, intervalMs); return () => { if (intervalRef.current) { clearInterval(intervalRef.current); intervalRef.current = null; } }; - }, [isEnabled, intervalMinutes, onCheck]); + }, [isEnabled, intervalMinutes, stableOnCheck]); return { isPollingActive: isEnabled, - checkNow: onCheck, + checkNow: stableOnCheck, lastChecked, }; }apps/server/src/routes/updates/routes/pull.ts (1)
90-95: Unused variable:currentBranchis retrieved but never used.The current branch is fetched but not referenced anywhere in the code. If it's not needed, consider removing it to reduce unnecessary git operations.
🔎 Proposed fix
- // Get current branch - const { stdout: branchOutput } = await execAsync('git rev-parse --abbrev-ref HEAD', { - cwd: installPath, - env: execEnv, - }); - const currentBranch = branchOutput.trim(); - // Merge the fetched changesdocs/auto-updates.md (1)
16-21: Align documented defaults and fix markdown lint issuesTwo things worth tightening here:
Default interval mismatch
- Docs table lists “Check interval” default as
5, but the serverGET /infohandler falls back tocheckIntervalMinutes: 15. Please align the table with the actual default (or vice‑versa) so users see the real behavior.Markdownlint fixes
Line 28: the fenced diagram block has no language; change the inner fence to something like
```textto satisfy MD040.Line 304: “Last updated: December 2025” is flagged as emphasis‑as‑heading (MD036). Consider making it a heading, e.g.:
### Last updated December 2025Also applies to: 28-68, 304-304
apps/ui/src/lib/electron.ts (1)
629-673: Consider reusing shared update types for theupdates.infocontract
updates.infocurrently inlines a custom result shape (includingisGitRepoandgitAvailable) instead of directly usingUpdateApiResponse<UpdateInfo>. That works structurally, but it duplicates the server contract and can drift ifUpdateInfochanges (e.g., new fields, changes undermechanismInfo).Consider tightening this in one of these ways:
- Type the method as returning
UpdateApiResponse<UpdateInfo>and surfaceisGitRepo/gitAvailableviamechanismInfoon the UI side, or- Keep the current UI-friendly shape but have it explicitly derived from
UpdateInfoin the HTTP client (e.g., map frommechanismInfo.isGitRepo/gitAvailable), so the server type remains the single source of truth.This keeps the ElectronAPI surface in sync with the shared
@automaker/typesdefinitions and reduces maintenance risk.apps/server/src/routes/updates/common.ts (1)
23-57: Minor env hardening and make use ofisValidGitUrlTwo small improvements here:
- When building
additionalPaths,process.env.HOMEcan be undefined on some environments, which would yield a bogus'undefined/.local/bin'entry:additionalPaths.push(`${process.env.HOME}/.local/bin`);Guard it before pushing:
const home = process.env.HOME; if (home) { additionalPaths.push(`${home}/.local/bin`); }
- You already expose
isValidGitUrl(), but it’s not used in the update routes yet. It’s a good candidate to validateupstreamUrlbefore passing it into git commands, to catch obviously bad values early and centralize URL hygiene.Also applies to: 121-137
apps/ui/src/components/updates/update-notifier.tsx (1)
51-53: Clarify “Later” behavior andisPullinglabel handling in the update toastTwo subtle UX/logic points in the notifier:
“Later” doesn’t currently cause a future reminder for the same version
- The comment says “will show again on next check if update still available”, but:
shownToastForCommitRef.currentis set toremoteVersionShortboth when the toast is shown and again in the “Later” handler.- The effect early‑returns whenever
shownToastForCommitRef.current === remoteVersionShort, and its deps don’t includelastChecked.- Net result: for a given
remoteVersionShort, once the user has seen the toast (and clicked “Later”), they won’t be reminded again until eitherupdateAvailabledrops tofalseor a new remote version appears.Decide which behavior you want and either:
- Adjust the comment to describe “don’t remind again for this version”, or
- Wire in a notion of “check tick” (e.g., include
lastCheckedin the deps and resetshownToastForCommitRefin the cancel handler) so periodic checks can resurface the toast.
isPullinglabel doesn’t actually update the existing toast
- The action label is computed with
isPulling ? 'Updating...' : 'Update Now', but once the toast is created, the effect will not recreate it whileshownToastForCommitRef.current === remoteVersionShort.- If you want to surface progress, you’ll need to update the existing toast via its
id(per Sonner’s API) instead of relying on the effect’sisPullingdependency.Neither issue is a blocker, but tightening this will make the notification behavior more predictable.
Also applies to: 95-107, 119-139
libs/types/src/updates.ts (2)
10-43: LGTM! Well-designed interface with good flexibility.The nullable version fields appropriately handle cases where version info cannot be determined. The optional
updateTypeandmechanismInfofields provide the flexibility needed for different update mechanisms while maintaining backward compatibility.Optional: Consider whether
mechanismInfocould use a discriminated union based onupdateTypefor better type safety:mechanismInfo?: | { type: 'git'; isGitRepo: boolean; gitAvailable: boolean } | { type: 'release'; channel: string } | { type: 'custom'; [key: string]: unknown };This would provide autocomplete and type checking for known mechanisms while still allowing custom implementations.
100-107: Consider discriminated unions for stronger type safety.The current design allows
resultto be undefined even whensuccessistrue, requiring runtime checks. A discriminated union would enforce the relationship betweensuccess,result, anderrorat compile time:🔎 Suggested type-safe refactor
-/** - * API response wrapper for update operations - */ -export interface UpdateApiResponse<T> { - success: boolean; - result?: T; - error?: string; -} +/** + * API response wrapper for update operations + */ +export type UpdateApiResponse<T> = + | { success: true; result: T } + | { success: false; error: string };Benefits:
- TypeScript guarantees
resultexists whensuccessistrue- No need for optional chaining:
response.result?.field- Compiler enforces error handling
Trade-off: Less flexible for partial success or warnings (though you could add a third variant if needed).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
README.mdapps/server/src/index.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.tsapps/server/src/routes/updates/routes/check.tsapps/server/src/routes/updates/routes/info.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/updates/index.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-update-polling.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/updates-store.tsapps/ui/vite.config.mtsdocs/auto-updates.mdlibs/types/src/index.tslibs/types/src/settings.tslibs/types/src/updates.ts
🧰 Additional context used
🧬 Code graph analysis (10)
libs/types/src/updates.ts (1)
libs/types/src/index.ts (4)
UpdateInfo(113-113)UpdateCheckResult(114-114)UpdatePullResult(115-115)UpdateApiResponse(116-116)
apps/server/src/routes/updates/index.ts (4)
apps/server/src/services/settings-service.ts (1)
SettingsService(104-583)apps/server/src/routes/updates/routes/check.ts (1)
createCheckHandler(22-167)apps/server/src/routes/updates/routes/pull.ts (1)
createPullHandler(23-174)apps/server/src/routes/updates/routes/info.ts (1)
createInfoHandler(23-129)
apps/ui/src/store/updates-store.ts (2)
libs/types/src/updates.ts (4)
UpdateApiResponse(103-107)UpdateInfo(13-43)UpdateCheckResult(48-72)UpdatePullResult(77-98)apps/ui/src/lib/electron.ts (1)
getElectronAPI(752-761)
apps/server/src/index.ts (1)
apps/server/src/routes/updates/index.ts (1)
createUpdatesRoutes(16-29)
apps/ui/src/hooks/use-update-polling.ts (2)
apps/ui/src/store/app-store.ts (1)
useAppStore(969-2754)apps/ui/src/store/updates-store.ts (2)
useUpdatesStore(320-320)checkForUpdates(62-68)
apps/ui/src/routes/__root.tsx (2)
apps/ui/src/components/updates/update-notifier.tsx (1)
UpdateNotifier(41-171)apps/ui/src/components/updates/index.ts (1)
UpdateNotifier(7-7)
libs/types/src/settings.ts (1)
libs/types/src/index.ts (2)
AutoUpdateSettings(65-65)DEFAULT_AUTO_UPDATE_SETTINGS(72-72)
apps/server/src/routes/updates/routes/pull.ts (2)
apps/server/src/routes/updates/common.ts (9)
getAutomakerRoot(67-76)isGitAvailable(81-88)isGitRepo(93-100)hasLocalChanges(121-124)getCurrentCommit(105-108)getShortCommit(113-116)execAsync(13-13)execEnv(54-57)logError(17-17)libs/types/src/updates.ts (1)
UpdatePullResult(77-98)
apps/ui/src/components/updates/update-notifier.tsx (3)
apps/ui/src/components/updates/index.ts (2)
UpdateNotifierProps(8-8)UpdateNotifier(7-7)apps/ui/src/store/updates-store.ts (2)
useUpdatesStore(320-320)pullUpdates(70-76)apps/ui/src/hooks/use-update-polling.ts (1)
useUpdatePolling(51-94)
apps/ui/src/components/views/settings-view.tsx (1)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
UpdatesSection(39-301)
🪛 markdownlint-cli2 (0.18.1)
docs/auto-updates.md
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
304-304: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (21)
apps/ui/src/components/views/settings-view/config/navigation.ts (1)
12-12: LGTM!The
RefreshCwicon is a sensible choice for the Updates section, and placing it before Danger Zone follows good UX ordering (less destructive options first).Also applies to: 32-32
apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
12-12: LGTM!Type extension correctly adds
'updates'to the union, consistent with the navigation item added innavigation.ts.apps/ui/vite.config.mts (1)
64-71: LGTM!The proxy configuration correctly forwards
/apirequests to the backend server during development. The default port3008aligns with the server's default, and WebSocket proxying is appropriately enabled for existing real-time features.apps/ui/src/hooks/use-settings-migration.ts (1)
234-234: LGTM!Adding
autoUpdateto the sync payload ensures the auto-update settings are persisted to server storage, consistent with how other settings are synchronized.apps/ui/src/components/updates/index.ts (1)
1-8: LGTM!Clean barrel file following project conventions, enabling simplified imports like
'@/components/updates'.apps/ui/src/routes/__root.tsx (1)
14-14: LGTM!The
UpdateNotifieris appropriately placed in the main layout (excluding setup route) alongside theToastercomponent it uses for notifications. Since it returnsnull, it doesn't affect the DOM structure while enabling background polling and toast notifications.Also applies to: 179-179
README.md (1)
56-56: LGTM!Clear and concise documentation addition. The Auto-Updates section provides a good high-level overview with appropriate links to detailed documentation.
Also applies to: 536-536, 569-578
apps/server/src/index.ts (1)
52-52: LGTM!The updates routes are correctly mounted after the auth middleware, ensuring all endpoints are protected. The dependency injection of
settingsServicefollows the established pattern used by other routes.Also applies to: 166-166
apps/ui/src/components/views/settings-view.tsx (1)
20-21: LGTM!Clean integration of the UpdatesSection component following the existing patterns in the settings view. The props are correctly passed and the switch case is consistent with other sections.
Also applies to: 54-56, 160-161
apps/server/src/routes/updates/index.ts (1)
1-29: LGTM!Well-structured router module with clean separation of concerns. The factory pattern for handlers with dependency injection of
settingsServiceis a good approach for testability.apps/ui/src/lib/http-api-client.ts (1)
1090-1137: LGTM!The updates API surface follows the established patterns in this file. The response type structures align with the server-side route implementations and the
UpdatePullResult/UpdateInfotypes defined inlibs/types/src/updates.ts.libs/types/src/index.ts (1)
65-66: LGTM!Clean addition of the new type exports following the established pattern in the index file.
Also applies to: 72-73, 110-117
libs/types/src/settings.ts (2)
437-456: LGTM!Well-documented interface with sensible defaults. The 15-minute check interval is a reasonable balance between staying current and not being overly aggressive with network requests.
304-308: LGTM!Clean integration of
autoUpdateintoGlobalSettingswith proper default values.Also applies to: 487-488
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
39-105: LGTM on the component logic and store integration.Good use of the centralized updates store with clear separation between UI and state management. The toast notifications provide appropriate user feedback for both success and error states.
apps/server/src/routes/updates/routes/pull.ts (1)
23-173: Overall implementation is well-structured with proper error handling.Good patterns observed:
- Cleanup of temporary remote in both success and error paths
- Specific error messages for common failure cases (non-fast-forward, conflicts)
- Use of
--ff-onlyto prevent merge commits and conflicts- Proper HTTP status codes (400 for user errors, 500 for server errors)
apps/server/src/routes/updates/routes/info.ts (1)
7-128:/infohandler logic looks solidThe handler correctly distinguishes between “no git”, “not a repo”, and “valid repo” and always returns a well‑formed
UpdateInfo, includingmechanismInfoflags. Error logging and 500 responses also look appropriate.apps/ui/src/store/updates-store.ts (1)
23-309: Updates store DI design and state transitions look correctThe updates store cleanly encapsulates update state, guards concurrent
checkForUpdates/pullUpdates, and uses DI for the API client and event emitter. Error handling and timestamp updates (lastChecked/lastPulled) are consistent with theUpdateApiResponseandUpdate*Resultcontracts.libs/types/src/updates.ts (3)
1-8: LGTM! Clear documentation of design intent.The header effectively communicates the abstraction goal and the three supported update mechanisms (git, release, custom).
45-72: LGTM! Type design is appropriate for the use case.The required
localVersion(vs nullablecurrentVersioninUpdateInfo) correctly reflects that an update check requires knowing the current version. The nullableremoteVersionappropriately handles check failures.
74-98: LGTM! Clean interface for update results.All version fields being required is appropriate since an update operation (even if it fails or is already up-to-date) should know the before and after versions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
243-249: Add security attributes towindow.opencall.Opening external URLs without
noopener,noreferrercan expose the application to tabnapping attacks where the opened page can accesswindow.opener.🔎 Proposed fix
<Button variant="outline" size="icon" onClick={() => { const url = autoUpdate.upstreamUrl.replace(/\.git$/, ''); - window.open(url, '_blank'); + window.open(url, '_blank', 'noopener,noreferrer'); }} title="Open repository" >
🧹 Nitpick comments (4)
apps/ui/src/components/updates/update-notifier.tsx (2)
124-146: Toast action label won't update whenisPullingchanges.The
isPullingstate is in the dependency array, but once the toast is shown, changingisPullingwon't update the button label from "Update Now" to "Updating..." because the toast content is static after creation. The early return on line 103 prevents re-creating the toast.Consider either:
- Remove
isPullingfrom dependencies since it doesn't effectively update the toast- Use Sonner's toast update API to modify the existing toast when
isPullingchanges🔎 Option 1: Remove isPulling from dependencies
}, [ updateAvailable, remoteVersionShort, autoUpdate.upstreamUrl, - isPulling, handleUpdateNow, onUpdateAvailable, ]);
131-137: Redundant ref assignment in "Later" onClick handler.Setting
shownToastForCommitRef.current = remoteVersionShorton line 135 is redundant because it's already set to this value on line 107 before the toast is shown.🔎 Proposed fix
cancel: { label: 'Later', onClick: () => { - // Dismiss toast - won't show again for this version until a new version appears - shownToastForCommitRef.current = remoteVersionShort; + // Toast dismisses automatically - ref already set, won't show again for this version }, },apps/server/src/routes/updates/routes/pull.ts (1)
70-95: Consider usingexecFileinstead ofexecfor defense in depth.While
isValidGitUrlprovides protection against shell injection, usingexecFile(orexecFileSync) with an argument array would provide an additional layer of security by avoiding shell interpretation entirely. This is especially relevant sincesourceUrloriginates from user settings.🔎 Example using execFileAsync
import { execFile } from 'child_process'; const execFileAsync = promisify(execFile); // Instead of: await execAsync(`git remote add ${tempRemoteName} "${sourceUrl}"`, { cwd, env }); // Use: await execFileAsync('git', ['remote', 'add', tempRemoteName, sourceUrl], { cwd, env });apps/server/src/routes/updates/common.ts (1)
70-79: Fragile path resolution - consider adding a marker file check.The fixed 5-level directory traversal to find the monorepo root will break if this file is moved. While the inline comment documents the expected path, consider adding a validation check.
🔎 Proposed defensive check
export function getAutomakerRoot(): string { // In ESM, we use import.meta.url to get the current file path // This file is at: apps/server/src/routes/updates/common.ts // So we need to go up 5 levels to get to the monorepo root const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); // Go up from: updates -> routes -> src -> server -> apps -> root - return path.resolve(__dirname, '..', '..', '..', '..', '..'); + const root = path.resolve(__dirname, '..', '..', '..', '..', '..'); + + // Sanity check - verify we found the monorepo root + // This will throw if the file was moved and the path is wrong + const fs = await import('fs'); + const packageJsonPath = path.join(root, 'package.json'); + if (!fs.existsSync(packageJsonPath)) { + throw new Error(`Could not locate monorepo root. Expected package.json at: ${packageJsonPath}`); + } + + return root; }Note: This would require making the function async or using
existsSync.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/server/src/routes/updates/common.tsapps/server/src/routes/updates/routes/check.tsapps/server/src/routes/updates/routes/info.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/ui/src/hooks/use-update-polling.tsapps/ui/src/lib/utils.tsdocs/auto-updates.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/src/hooks/use-update-polling.ts
- docs/auto-updates.md
- apps/server/src/routes/updates/routes/info.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ui/src/components/updates/update-notifier.tsx (4)
apps/ui/src/components/updates/index.ts (2)
UpdateNotifierProps(8-8)UpdateNotifier(7-7)apps/ui/src/store/updates-store.ts (2)
useUpdatesStore(320-320)pullUpdates(70-76)apps/ui/src/hooks/use-update-polling.ts (1)
useUpdatePolling(51-99)apps/ui/src/lib/utils.ts (1)
getRepoDisplayName(72-75)
🔇 Additional comments (11)
apps/ui/src/lib/utils.ts (1)
67-75: LGTM!The utility function correctly extracts the
owner/repoformat from GitHub URLs, handling both HTTPS (/) and SSH (:) path separators. The regex appropriately excludes the.gitsuffix via the[^/.]+pattern, and the fallback to'upstream'is sensible for non-GitHub URLs.apps/ui/src/components/views/settings-view/updates/updates-section.tsx (2)
39-58: LGTM!The component properly uses the centralized updates store and fetches info on mount. The Zustand store action
fetchInfois stable by design, so theuseEffectdependency is correct.
60-105: LGTM!The async handlers correctly access store state via
getState()after awaiting async operations, avoiding stale closure issues. Toast notifications provide appropriate user feedback for all outcomes.apps/ui/src/components/updates/update-notifier.tsx (1)
42-54: LGTM!Good use of refs to track toast state and prevent duplicate notifications. The DI-ready props pattern allows for testing flexibility.
apps/server/src/routes/updates/routes/pull.ts (2)
25-69: LGTM! Previous security concerns addressed.The handler properly validates Git availability, repository status, local changes, and the upstream URL format using
isValidGitUrlbefore executing any shell commands. This addresses the command injection concerns from previous reviews.
75-95: LGTM! Concurrency and cleanup properly handled.Using
crypto.randomBytesfor the temporary remote name prevents conflicts with concurrent pulls, and thetry...finallyblock ensures the remote is always cleaned up regardless of success or failure.apps/server/src/routes/updates/routes/check.ts (3)
24-59: LGTM! Security validations properly implemented.Consistent with
pull.ts, the handler validates Git availability, repository status, and upstream URL format before executing commands.
95-113: LGTM! Correct update detection logic.The
git merge-base --is-ancestorapproach correctly determines update availability: if the local commit is an ancestor of the remote commit, an update is available. If local is ahead or branches have diverged, no update is shown, which is the right behavior for a safe fast-forward-only update system.
129-145: The API contract is properly handled by the UI despite the unconventional pattern.The UI correctly checks
result.error(apps/ui/src/store/updates-store.ts line 232) and treats responses with an error field as a failed check, returning false and storing the error message for display. This prevents clients from displaying stale local version info when upstream is unreachable—instead, the error is properly surfaced to the user. The pattern is unconventional but working as designed.apps/server/src/routes/updates/common.ts (2)
133-145: LGTM! Comprehensive shell metacharacter blocking.The
isValidGitUrlfunction properly validates protocol prefixes and blocks shell metacharacters that could enable command injection. The regex covers the critical characters:;, backticks,|,&,<>,(),$,!,\,[], and spaces.
23-60: LGTM! Comprehensive PATH extension for cross-platform Git discovery.The extended PATH configuration covers common Git installation locations across Windows, macOS (both Intel and Apple Silicon via Homebrew), and Linux. This ensures Git commands work correctly in Electron environments where the PATH may be limited.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (2)
61-74: Consider handling the case whencheckForUpdatesthrows an exception.The
handleCheckForUpdatesfunction doesn't have a try-catch wrapper. IfcheckForUpdates()throws (e.g., network failure), the error won't be caught and no toast will be shown.🔎 Proposed fix
const handleCheckForUpdates = async () => { clearError(); - const hasUpdate = await checkForUpdates(); - - if (hasUpdate) { - toast.success('Update available!', { - description: `New version: ${useUpdatesStore.getState().remoteVersionShort}`, - }); - } else if (!useUpdatesStore.getState().error) { - toast.success('You are up to date!'); - } else { - toast.error(useUpdatesStore.getState().error || 'Failed to check for updates'); + try { + const hasUpdate = await checkForUpdates(); + + if (hasUpdate) { + toast.success('Update available!', { + description: `New version: ${useUpdatesStore.getState().remoteVersionShort}`, + }); + } else if (!useUpdatesStore.getState().error) { + toast.success('You are up to date!'); + } else { + toast.error(useUpdatesStore.getState().error || 'Failed to check for updates'); + } + } catch (err) { + toast.error('Failed to check for updates'); } };
85-100: Persistent toast withduration: Infinitymay cause UX friction.The update success toast stays on screen indefinitely until the user interacts with it. If the user dismisses it accidentally, they would need to trigger another update check to see the restart prompt again.
Consider using a long but finite duration (e.g., 30 seconds) or providing an alternative way to trigger a restart from the Updates section UI.
apps/ui/src/components/updates/update-notifier.tsx (1)
124-138: Toast action label won't update dynamically while pulling.The toast is created once with the
isPullingvalue at creation time. WhenpullUpdates()is called andisPullingbecomestrue, the existing toast's button label won't change to "Updating..." because Sonner toasts don't re-render on prop changes.Consider disabling the action button after click instead, or dismissing and recreating the toast when pulling starts.
🔎 Quick fix: disable button via onClick guard
The
handleUpdateNowalready prevents duplicate calls viaisPullingcheck in the store'spullUpdates. The visual feedback issue is minor since the pull typically completes quickly.apps/server/src/routes/updates/common.ts (1)
70-79: Path resolution relies on fixed directory depth.The function assumes this file is always at
apps/server/src/routes/updates/common.ts. While this works for the current structure, moving the file would break the path resolution.A past review suggested using a marker-file approach (e.g., finding
package.json), but given the stable monorepo structure and low likelihood of moving this file, the current approach is acceptable.apps/server/src/routes/updates/routes/check.ts (1)
76-79: Hardcodedmainbranch may not work for all upstream configurations.The endpoint assumes the upstream uses a
mainbranch. Some repositories still usemasteror other default branch names.Consider making the branch configurable or detecting the default branch dynamically:
#!/bin/bash # Check if there are common alternative default branch patterns in the codebase rg -n "main|master" apps/server/src/routes/updates/ --type ts
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
README.mdapps/server/src/index.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.tsapps/server/src/routes/updates/routes/check.tsapps/server/src/routes/updates/routes/info.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/updates/index.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-update-polling.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/utils.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/updates-store.tsapps/ui/vite.config.mtsdocs/auto-updates.mdlibs/types/src/index.tslibs/types/src/settings.tslibs/types/src/updates.ts
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/auto-updates.md
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/server/src/routes/updates/index.ts
- apps/ui/src/hooks/use-update-polling.ts
- apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts
- libs/types/src/settings.ts
- apps/ui/src/lib/utils.ts
- apps/ui/src/hooks/use-settings-migration.ts
- apps/ui/src/components/views/settings-view/config/navigation.ts
- libs/types/src/updates.ts
- apps/ui/src/components/updates/index.ts
- apps/server/src/routes/updates/routes/info.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/ui/src/components/views/settings-view.tsx (1)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
UpdatesSection(39-295)
apps/server/src/routes/updates/routes/pull.ts (3)
apps/server/src/routes/updates/common.ts (10)
getAutomakerRoot(70-79)isGitAvailable(84-91)isGitRepo(96-103)hasLocalChanges(124-127)isValidGitUrl(133-145)getCurrentCommit(108-111)getShortCommit(116-119)execAsync(13-13)execEnv(57-60)logError(17-17)libs/types/src/index.ts (1)
UpdatePullResult(115-115)libs/types/src/updates.ts (1)
UpdatePullResult(77-98)
apps/server/src/routes/updates/common.ts (2)
apps/server/src/routes/common.ts (1)
createLogError(34-38)apps/server/src/routes/github/routes/common.ts (1)
extendedPath(11-19)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (4)
libs/types/src/index.ts (1)
AutoUpdateSettings(65-65)libs/types/src/settings.ts (1)
AutoUpdateSettings(444-451)apps/ui/src/store/updates-store.ts (3)
useUpdatesStore(320-320)checkForUpdates(62-68)pullUpdates(70-76)apps/ui/src/lib/utils.ts (2)
cn(5-7)getRepoDisplayName(72-75)
apps/ui/src/routes/__root.tsx (2)
apps/ui/src/components/updates/update-notifier.tsx (1)
UpdateNotifier(42-170)apps/ui/src/components/updates/index.ts (1)
UpdateNotifier(7-7)
apps/server/src/index.ts (1)
apps/server/src/routes/updates/index.ts (1)
createUpdatesRoutes(16-29)
🔇 Additional comments (23)
apps/ui/vite.config.mts (1)
64-71: LGTM! Proxy configuration is correctly set up for the updates API.The proxy correctly routes
/apirequests to the backend server during development, with WebSocket support enabled for real-time event streaming.apps/ui/src/routes/__root.tsx (1)
14-14: LGTM! UpdateNotifier is correctly integrated into the root layout.The component is appropriately placed after
Toaster(since it uses sonner for notifications) and is only rendered in the main layout, excluding the setup route.Also applies to: 179-179
apps/ui/src/components/views/settings-view.tsx (1)
20-20: LGTM! Updates section is properly integrated into settings view.The integration follows the established pattern for other settings sections, correctly passing
autoUpdatestate andsetAutoUpdatehandler as props.Also applies to: 56-57, 164-165
apps/server/src/index.ts (1)
52-52: LGTM! Updates routes are correctly mounted with proper authentication.The routes are mounted after
authMiddlewareensuring they require authentication, and thesettingsServicedependency injection follows the established pattern.Also applies to: 166-166
libs/types/src/index.ts (1)
65-65: LGTM! Type exports are well-organized and follow existing conventions.The new exports for
AutoUpdateSettings,DEFAULT_AUTO_UPDATE_SETTINGS, and update-related types are properly grouped and documented.Also applies to: 72-72, 110-117
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (2)
21-21: Past review issue resolved:getRepoDisplayNameis now imported from shared utils.The code duplication concern has been addressed by extracting the helper to
@/lib/utils.
243-253: Past review issue resolved: Security attributes added towindow.open.The
noopener,noreferrerattributes are now correctly applied, preventing tabnapping vulnerabilities.apps/server/src/routes/updates/routes/pull.ts (3)
62-69: Past review issue resolved: URL validation prevents command injection.The
isValidGitUrlcheck properly validates the upstream URL before using it in shell commands, mitigating command injection risks.
75-76: Past review issue resolved: Random remote name prevents concurrency conflicts.Using
crypto.randomBytesfor the temp remote name ensures unique identifiers even with concurrent pull requests.
146-156: Past review issue resolved: Cleanup infinallyblock ensures temp remote removal.The temporary remote is always cleaned up regardless of success or failure, preventing stale remote entries.
apps/ui/src/store/app-store.ts (2)
768-770: Past review issue resolved:setAutoUpdatereturn type correctly declaresPromise<void>.The type declaration now matches the async implementation, aligning with the pattern used by
setAutoLoadClaudeMdandsetEnableSandboxMode.
10-12: LGTM! Auto-update state is properly integrated into the app store.The implementation correctly:
- Imports and uses shared types from
@automaker/types- Initializes with sensible defaults
- Persists settings via
partialize- Syncs changes to the server
Also applies to: 488-490, 945-945, 1586-1594, 2751-2751
apps/ui/src/lib/electron.ts (1)
629-673: LGTM!The updates API surface is well-structured with consistent typing. The optional
updatesproperty and nullable result fields properly handle cases where the API may not be available or updates cannot be fetched.apps/ui/src/components/updates/update-notifier.tsx (1)
42-49: Well-designed DI pattern for testability.The optional callback props (
onUpdateAvailable,onUpdateInstalled) and hook-based polling make this component easily testable and extensible.apps/server/src/routes/updates/routes/check.ts (2)
95-113: Good use ofgit merge-base --is-ancestorfor accurate update detection.This correctly handles the case where local is ahead of remote (avoiding false positive update notifications), which was mentioned as a fix in the PR objectives.
146-156: Good cleanup pattern withtry/finally.The temporary remote is always removed regardless of success or failure, preventing orphaned remotes from accumulating.
apps/server/src/routes/updates/common.ts (2)
133-145: Good URL validation with shell metacharacter blocking.The validation correctly allows standard Git protocols while blocking characters that could enable command injection (
;, backticks,|,&,$, etc.).
23-52: Cross-platform PATH extension is well-implemented.Handles Windows vs Unix path separators and includes common Git installation locations for both platforms, ensuring Git is discoverable in Electron environments where PATH may be restricted.
apps/ui/src/lib/http-api-client.ts (1)
1090-1138: LGTM!The Updates API integration follows the established patterns in the HTTP client. The endpoint methods (
check,pull,info) correctly map to the server routes with appropriate HTTP methods and return types.apps/ui/src/store/updates-store.ts (4)
285-287: Fire-and-forgetfetchInfo()call after pull.The
fetchInfo()call after a successful pull is not awaited. This is likely intentional to avoid blocking thepullUpdatesreturn, but iffetchInfofails, the error won't be surfaced to the caller.This is acceptable since the pull already succeeded and the info refresh is supplementary.
166-173: Clean DI pattern with mutable references.The mutable
_apiClientand_eventEmitterreferences allow runtime dependency swapping viasetApiClient/setEventEmitter, which is useful for testing scenarios.
211-255: Good guard against concurrent update checks.The
isChecking || isPullingguard prevents race conditions from multiple simultaneous check operations.
316-320: Store instantiation is safe for module load time.The default store is created when the module loads, which is standard for Zustand singleton stores.
getElectronAPI()is not called at import time—it's invoked only when the API client's methods (getInfo(),checkForUpdates(),pullUpdates()) are actually executed, at which point the SSR guard ingetElectronAPI()will properly reject any calls during server-side rendering. However, note thatcreateDefaultEventEmitter()does usewindow.dispatchEvent()at module initialization, which could cause issues in SSR contexts if this file is imported on the server.
Docstrings generation was requested by @amoscicki. * #290 (comment) The following files were modified: * `apps/server/src/routes/updates/common.ts` * `apps/server/src/routes/updates/index.ts` * `apps/server/src/routes/updates/routes/check.ts` * `apps/server/src/routes/updates/routes/info.ts` * `apps/server/src/routes/updates/routes/pull.ts` * `apps/ui/src/components/updates/update-notifier.tsx` * `apps/ui/src/components/views/settings-view.tsx` * `apps/ui/src/components/views/settings-view/updates/updates-section.tsx` * `apps/ui/src/hooks/use-settings-migration.ts` * `apps/ui/src/hooks/use-update-polling.ts` * `apps/ui/src/lib/utils.ts` * `apps/ui/src/routes/__root.tsx`
|
@gemini-code-assist Please recheck the following comments and resolve if addressed:
Also asking @coderabbitai to recheck:
|
|
Hello @amoscicki, thanks for the update! I've reviewed the points you raised:
Points 3 and 4 are addressed to @coderabbitai, so I will not comment on those. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3440 🏁 Script executed: Length of output: 1529 @amoscicki I've rechecked all 4 items and can confirm they have been properly addressed:
All concerns have been successfully resolved! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/src/routes/updates/routes/check.ts (1)
120-137: Consider clarifying thesuccess: trueresponse on fetch failure.The handler returns
success: trueeven when the upstream fetch fails (lines 124-136), which is a graceful degradation allowing the UI to show local version info. However, this semantic choice (success refers to the endpoint working, not the fetch succeeding) could be confusing. Theerrorfield in the result clarifies this, but you might consider either:
- Adding a comment explaining this design choice, or
- Adding a top-level
fetchSuccess: booleanfield to make the distinction explicitThis is a minor readability improvement and the current implementation is functional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/routes/updates/common.tsapps/server/src/routes/updates/routes/check.tsapps/server/src/routes/updates/routes/pull.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/routes/updates/common.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/server/src/routes/updates/routes/pull.tsapps/server/src/routes/updates/routes/check.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/updates/routes/pull.tsapps/server/src/routes/updates/routes/check.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/server/src/routes/updates/routes/pull.tsapps/server/src/routes/updates/routes/check.ts
🧬 Code graph analysis (1)
apps/server/src/routes/updates/routes/check.ts (3)
apps/server/src/routes/updates/common.ts (9)
getAutomakerRoot(73-97)isGitAvailable(102-109)isGitRepo(114-121)isValidGitUrl(151-163)getCurrentCommit(126-129)getShortCommit(134-137)withTempGitRemote(169-191)execAsync(15-15)execEnv(59-62)libs/types/src/index.ts (1)
UpdateCheckResult(114-114)libs/types/src/updates.ts (1)
UpdateCheckResult(48-72)
🔇 Additional comments (5)
apps/server/src/routes/updates/routes/pull.ts (3)
25-69: Well-structured validation flow with proper security checks.The handler correctly validates prerequisites in order: git availability, repository status, local changes check, and URL validation before any git operations. The
isValidGitUrlcheck at line 63 addresses the previously-raised command injection concern.
71-107: Clean implementation using the sharedwithTempGitRemotehelper.The code now correctly uses the centralized helper that handles random remote name generation and guaranteed cleanup in a
finallyblock. This addresses the earlier refactoring suggestion about code reuse between check and pull handlers.
113-139: Good error differentiation for actionable user feedback.The handler correctly distinguishes between fast-forward failures and merge conflicts, providing specific guidance for each case. This helps users understand the required manual intervention.
apps/server/src/routes/updates/routes/check.ts (2)
24-59: Consistent validation pattern with pull handler.The check handler follows the same defensive validation sequence as the pull handler, ensuring git availability, repository status, and URL validation before any network operations.
87-103: Correct use ofgit merge-base --is-ancestorfor update detection.The logic properly handles all three cases:
- Local equals remote → no update (line 89 condition)
- Local is ancestor of remote → update available (exit code 0)
- Local has diverged or is ahead → no update (exit code non-zero caught at line 98)
This aligns well with the
--ff-onlymerge strategy used in the pull handler.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/ui/src/hooks/use-update-polling.ts (1)
79-84: Consider adding error handling for the immediate check.The immediate
stableOnCheck()call on line 80 fires without awaiting or catching errors. While the store'scheckForUpdateslikely handles errors internally, an unhandled rejection could occur if the underlying API throws unexpectedly.🔎 Suggested improvement
// Check immediately on enable - stableOnCheck(); + stableOnCheck().catch(() => { + // Errors are handled in the store + });apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
209-227: Input validation works but may cause UX confusion.The validation on lines 216-219 silently ignores invalid values (outside 1-60 range or non-numeric). While this prevents invalid state, users might be confused when typing doesn't update the field. Consider showing validation feedback or allowing the intermediate state.
🔎 Optional improvement for better UX
<Input id="check-interval" type="number" min={1} max={60} value={autoUpdate.checkIntervalMinutes} onChange={(e) => { const value = parseInt(e.target.value, 10); - if (!isNaN(value) && value >= 1 && value <= 60) { - onAutoUpdateChange({ checkIntervalMinutes: value }); - } + if (!isNaN(value)) { + // Clamp to valid range + const clamped = Math.min(60, Math.max(1, value)); + onAutoUpdateChange({ checkIntervalMinutes: clamped }); + } }} className="w-32" disabled={!autoUpdate.enabled} />apps/ui/src/components/updates/update-notifier.tsx (1)
133-137: Redundant assignment in "Later" onClick handler.Line 135 sets
shownToastForCommitRef.current = remoteVersionShort, but this value was already set on line 107. The assignment is redundant since clicking "Later" dismisses the toast but doesn't change the deduplication tracking.🔎 Simplify the handler
cancel: { label: 'Later', onClick: () => { - // Dismiss toast - won't show again for this version until a new version appears - shownToastForCommitRef.current = remoteVersionShort; + // Toast dismissed - won't show again for this version }, },apps/ui/src/lib/http-api-client.ts (1)
1090-1137: Consider importing shared types from@automaker/typesto avoid drift.The inline return types for
updates.info()includeisGitRepoandgitAvailableproperties that aren't present in the sharedUpdateInfotype from@automaker/types, which usesmechanismInfo?: Record<string, unknown>for mechanism-specific data instead. This creates potential for type drift between the API client and consumers likeupdates-store.tsthat import from the shared package.Per coding guidelines, prefer importing from shared packages. While the current pattern is consistent with other APIs in this file, using the shared types would improve maintainability.
🔎 Suggested refactor using shared types
+import type { + UpdateInfo, + UpdateCheckResult, + UpdatePullResult, + UpdateApiResponse, +} from '@automaker/types'; // Updates API updates = { - check: (): Promise<{ - success: boolean; - result?: { - updateAvailable: boolean; - localVersion: string; - ... - }; - error?: string; - }> => this.get('/api/updates/check'), + check: (): Promise<UpdateApiResponse<UpdateCheckResult>> => + this.get('/api/updates/check'), - pull: (): Promise<{ - success: boolean; - result?: { - success: boolean; - ... - }; - error?: string; - }> => this.post('/api/updates/pull', {}), + pull: (): Promise<UpdateApiResponse<UpdatePullResult>> => + this.post('/api/updates/pull', {}), - info: (): Promise<{ - success: boolean; - result?: { - installPath: string; - ... - }; - error?: string; - }> => this.get('/api/updates/info'), + info: (): Promise<UpdateApiResponse<UpdateInfo>> => + this.get('/api/updates/info'), };Note: Ensure the server response shape matches
UpdateInfo(usingmechanismInfofor git-specific fields likeisGitRepoandgitAvailable), or update the shared type accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
README.mdapps/server/src/index.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.tsapps/server/src/routes/updates/routes/check.tsapps/server/src/routes/updates/routes/info.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/updates/index.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-update-polling.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/utils.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/updates-store.tsapps/ui/vite.config.mtsdocs/auto-updates.mdlibs/types/src/index.tslibs/types/src/settings.tslibs/types/src/updates.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/server/src/routes/updates/index.ts
- apps/ui/src/components/views/settings-view.tsx
- apps/ui/src/lib/electron.ts
- libs/types/src/settings.ts
- apps/ui/src/lib/utils.ts
- apps/ui/src/components/updates/index.ts
- apps/ui/src/store/app-store.ts
- apps/server/src/index.ts
- libs/types/src/index.ts
- docs/auto-updates.md
- apps/ui/src/routes/__root.tsx
- apps/ui/vite.config.mts
🧰 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/hooks/use-settings-migration.tsapps/server/src/routes/updates/routes/check.tsapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/hooks/use-update-polling.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/server/src/routes/updates/common.tsapps/ui/src/components/updates/update-notifier.tsxapps/server/src/routes/updates/routes/info.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/store/updates-store.tsapps/ui/src/lib/http-api-client.tslibs/types/src/updates.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/hooks/use-settings-migration.tsapps/server/src/routes/updates/routes/check.tsapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/hooks/use-update-polling.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/server/src/routes/updates/common.tsapps/ui/src/components/updates/update-notifier.tsxapps/server/src/routes/updates/routes/info.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/store/updates-store.tsapps/ui/src/lib/http-api-client.tslibs/types/src/updates.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/updates/routes/check.tsapps/server/src/routes/updates/routes/pull.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/routes/info.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/components/views/settings-view/updates/updates-section.tsxapps/ui/src/components/updates/update-notifier.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,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/server/src/routes/updates/common.ts
🧬 Code graph analysis (5)
apps/ui/src/hooks/use-update-polling.ts (1)
apps/ui/src/store/updates-store.ts (2)
useUpdatesStore(320-320)checkForUpdates(62-68)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (5)
libs/types/src/index.ts (1)
AutoUpdateSettings(65-65)libs/types/src/settings.ts (1)
AutoUpdateSettings(444-451)apps/ui/src/store/updates-store.ts (3)
useUpdatesStore(320-320)checkForUpdates(62-68)pullUpdates(70-76)apps/ui/src/lib/utils.ts (2)
cn(5-7)getRepoDisplayName(72-75)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/server/src/routes/updates/common.ts (2)
apps/server/src/routes/common.ts (1)
createLogError(34-38)apps/server/src/routes/github/routes/common.ts (1)
extendedPath(11-19)
apps/ui/src/store/updates-store.ts (3)
libs/types/src/updates.ts (4)
UpdateApiResponse(103-107)UpdateInfo(13-43)UpdateCheckResult(48-72)UpdatePullResult(77-98)apps/ui/src/lib/electron.ts (1)
getElectronAPI(752-761)apps/ui/src/lib/http-api-client.ts (1)
get(174-178)
libs/types/src/updates.ts (1)
libs/types/src/index.ts (4)
UpdateInfo(122-122)UpdateCheckResult(123-123)UpdatePullResult(124-124)UpdateApiResponse(125-125)
🔇 Additional comments (25)
apps/ui/src/hooks/use-settings-migration.ts (1)
235-235: LGTM! Auto-update settings integration looks good.The addition of
autoUpdateto the sync payload follows the established pattern for syncing global settings to the server.apps/ui/src/components/views/settings-view/config/navigation.ts (1)
12-12: LGTM! Updates navigation item added correctly.The new navigation entry follows the established pattern and properly integrates the RefreshCw icon.
Also applies to: 32-32
apps/server/src/routes/updates/common.ts (4)
1-62: LGTM! Imports and PATH configuration are well-structured.The extended PATH handling comprehensively covers Windows, macOS, and Linux package manager locations, ensuring git availability across platforms.
73-97: LGTM! Robust root directory resolution.The upward traversal looking for
package.jsonwithname === 'automaker'is a solid approach that addresses the fragility concerns from previous reviews.Based on learnings, this implementation successfully resolves the fixed relative path issue.
102-145: LGTM! Git utility functions are well-implemented.All git commands use safe parameterization and the extended environment for cross-platform git discovery.
151-163: LGTM! URL validation provides strong injection protection.The validation correctly blocks shell metacharacters while allowing standard git protocols (HTTPS, SSH, git).
Based on learnings, this addresses the command injection concerns from previous reviews.
apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
12-12: LGTM! View ID type extension is correct.The addition of the 'updates' view ID properly extends the type union to support the new Updates settings section.
README.md (1)
56-56: LGTM! Documentation additions are clear and well-structured.The Auto-Updates documentation is properly integrated into the TOC and Learn More sections, with a concise feature summary and link to detailed documentation.
Also applies to: 536-536, 570-578
apps/server/src/routes/updates/routes/pull.ts (1)
1-145: LGTM!The implementation is well-structured with proper security measures:
- URL validation via
isValidGitUrlprevents command injectionwithTempGitRemotehelper centralizes temporary remote management with proper cleanup- Comprehensive error handling for git-specific scenarios (non-fast-forward, merge conflicts)
- Proper HTTP status codes for different error conditions
apps/server/src/routes/updates/routes/info.ts (1)
1-125: LGTM!Clean implementation with proper handling of edge cases:
- Git unavailable scenario returns partial info with
gitAvailable: false- Non-repo scenario returns info with
isGitRepo: false- Full repo scenario returns complete version, branch, and local changes info
- Correctly uses
DEFAULT_AUTO_UPDATE_SETTINGSfrom shared types to avoid duplicationapps/ui/src/hooks/use-update-polling.ts (2)
59-63: Ref pattern for callback stabilization is well implemented.The use of
useRefto store the current check function anduseCallbackto create a stable wrapper prevents unnecessary interval resets when the check function reference changes. This is a good pattern for avoiding stale closure issues while maintaining stable dependencies.
67-92: LGTM - Interval management is correct.The effect properly:
- Clears existing intervals before setting new ones
- Returns a cleanup function for unmount
- Depends on the right values (
isEnabled,intervalMinutes,stableOnCheck)apps/ui/src/components/views/settings-view/updates/updates-section.tsx (3)
61-74: Store access pattern after async operation is correct.Using
useUpdatesStore.getState()afterawait checkForUpdates()ensures you read the latest state values rather than stale closure values. This is the correct pattern for accessing Zustand store state after async operations.
243-254: Security fix confirmed -window.openincludesnoopener,noreferrer.The third parameter
'noopener,noreferrer'properly prevents tabnapping attacks when opening external URLs. This addresses the previously flagged security concern.
85-100: Toast withduration: Infinityis appropriate for important actions.Using infinite duration for the "Update installed" toast ensures users don't miss the restart prompt. The action/cancel buttons provide clear options. Good UX pattern for critical notifications requiring user action.
apps/server/src/routes/updates/routes/check.ts (2)
87-103: Update detection logic usinggit merge-base --is-ancestoris correct.The logic properly handles all three scenarios:
- Same version:
localVersion === remoteVersion→ no update- Local behind remote:
merge-base --is-ancestorsucceeds (exit 0) → update available- Local ahead or diverged: command fails (exit 1) → no update (safe default)
120-137: Graceful degradation when upstream fetch fails.Returning
success: truewithupdateAvailable: falseand an error message allows the UI to show "up to date" with context about the fetch failure, rather than breaking the entire check flow. Good error handling pattern.libs/types/src/updates.ts (1)
1-107: LGTM - Well-designed type definitions.The interfaces are:
- Thoroughly documented with JSDoc comments
- Abstract enough to support multiple update mechanisms (git, release, custom)
- Using proper generic pattern for
UpdateApiResponse<T>- Using
Record<string, unknown>formechanismInfoprovides flexibility without usinganyapps/ui/src/components/updates/update-notifier.tsx (2)
148-166: LGTM - Cleanup effects are properly structured.Two separate effects handle:
- Unmount cleanup (lines 149-155): Dismisses any active toast
- State reset (lines 158-166): Clears refs and dismisses toast when update is no longer available
Both are correctly implemented with appropriate dependencies.
42-49: Component follows good patterns for headless notification management.
- Renders null (no visible UI)
- Uses refs for deduplication to prevent toast spam
- DI-friendly via optional callback props
- Starts polling via
useUpdatePolling()hookapps/ui/src/store/updates-store.ts (5)
10-44: Well-structured interfaces for dependency injection.The interface definitions for
IUpdatesApiClientandIUpdateEventEmitterare clean and properly documented. Using types from@automaker/typesaligns with coding guidelines.
50-86: LGTM!The default implementations are well-designed:
- Defensive checks for API availability before calling methods
- SSR-safe since
getElectronAPI()is deferred to method invocation- Standard
CustomEventpattern for cross-component communication
92-154: Clean state design with comprehensive coverage.The state interface properly separates concerns with loading states for each operation, error tracking, and timestamps. The initial state correctly initializes all fields.
211-255: Good dual-level error handling.The implementation correctly handles both API response errors (
response.error) and result-level errors (result.error), with consistent timestamp updates even on failure. This ensures robust error reporting to the UI.
316-330: LGTM!The default store instance and selectors follow standard Zustand patterns. The combined
selectIsLoadingselector is a useful convenience for UI components.
- Fix false positive 'update available' when local is ahead of remote Uses git merge-base --is-ancestor to properly check ancestry - Fix UI not refreshing after update from toast notification Emit custom event when update pulled, UpdatesSection listens and refetches 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Rename commit-related fields to version-related (localVersion, remoteVersion, etc.) - Add centralized updates-store with DI support (IUpdatesApiClient, IUpdateEventEmitter) - Separate concerns: store (state), polling hook (when), notifier (toasts), section (UI) - Add UpdateNotifier component for toast notifications - Make updateType optional for backwards compatibility 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add detailed docs/auto-updates.md with architecture and DI guide - Update README with simplified section and link to full docs - Include examples for swapping update mechanism 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add autoUpdate to syncSettingsToServer list - Make setAutoUpdate call syncSettingsToServer after state change 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Addresses the following review comments: Critical (security): - #2649468854: Add URL validation in check.ts - #2649468856: Add URL validation in pull.ts - #2649468858: Strengthen isValidGitUrl to block shell metacharacters Medium (robustness): - #2649468862: Use random temp remote name in check.ts - #2649468863: Refactor check.ts to use try...finally for cleanup - #2649468864: Use DEFAULT_AUTO_UPDATE_SETTINGS in info.ts - #2649468865: Use random temp remote name in pull.ts - #2649468866: Refactor pull.ts to use try...finally for cleanup - #2649468868: Extract getRepoDisplayName to shared utility - #2649468870: Fix docs default interval (5->15 minutes) Additional fixes from CodeRabbit: - Stabilize onCheck callback in use-update-polling.ts - Guard process.env.HOME in common.ts - Fix markdown lint issues in docs - Update "Later" button behavior comment - Remove unused currentBranch variable in pull.ts Skipped (acceptable as-is): - #2649468861: Fragile project root resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add noopener,noreferrer to window.open for security - Fix setAutoUpdate return type to Promise<void> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of using a fixed relative path that breaks if the file moves, now traverse up the directory tree looking for the monorepo's package.json with name "automaker". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Centralizes the temp remote add/remove pattern into a shared helper function in common.ts, used by both check.ts and pull.ts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/server/src/routes/updates/common.ts (1)
169-176: ValidatesourceUrlbefore using it in shell commands.The function should call
isValidGitUrl(sourceUrl)before interpolating the URL into the git command. Even though the URL is wrapped in quotes, validating it provides defense-in-depth against command injection—especially sinceisValidGitUrlexists specifically for this purpose.🔎 Proposed fix
export async function withTempGitRemote<T>( installPath: string, sourceUrl: string, callback: (tempRemoteName: string) => Promise<T> ): Promise<T> { + if (!isValidGitUrl(sourceUrl)) { + throw new Error(`Invalid git URL: ${sourceUrl}`); + } + const tempRemoteName = `automaker-temp-remote-${crypto.randomBytes(8).toString('hex')}`;
🧹 Nitpick comments (2)
apps/ui/src/lib/utils.ts (1)
67-75: Enhance regex to handle .git suffix in GitHub URLs.The regex pattern may not correctly extract repository names from URLs ending with
.git(e.g.,https://github.com/owner/repo.git). The character class[^/.]excludes dots, which would stop matching atrepoinstead ofrepo.git.🔎 Proposed regex improvement
export function getRepoDisplayName(url: string): string { - const match = url.match(/github\.com[/:]([^/]+\/[^/.]+)/); + const match = url.match(/github\.com[/:]([^/]+\/[^/]+?)(?:\.git)?(?:[/?#]|$)/); return match ? match[1] : 'upstream'; }This pattern:
- Captures
owner/repoincluding dots in the repo name- Optionally matches and discards
.gitsuffix- Handles trailing slashes, query params, or end of string
apps/ui/src/store/updates-store.ts (1)
285-286: Consider awaitingfetchInfo()to ensure consistent state.The
fetchInfo()call is fire-and-forget, which means:
- If it fails, the
infostate may remain stale after a successful pull- Any
fetchInfoerror would overwrite the success state in the store (since the error was already cleared at line 261)- Users might see an error message after a successful update
🔎 Recommended fix
// Refresh info to get new commit - get().fetchInfo(); + await get().fetchInfo();Alternatively, if non-blocking behavior is intentional, add a comment explaining the rationale and consider handling fetchInfo errors separately to avoid overwriting the success state.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
README.mdapps/server/src/index.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.tsapps/server/src/routes/updates/routes/check.tsapps/server/src/routes/updates/routes/info.tsapps/server/src/routes/updates/routes/pull.tsapps/ui/src/components/updates/index.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/hooks/use-update-polling.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/utils.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/store/updates-store.tsapps/ui/vite.config.mtsdocs/auto-updates.mdlibs/types/src/index.tslibs/types/src/settings.tslibs/types/src/updates.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/ui/src/components/views/settings-view/config/navigation.ts
- apps/ui/src/components/updates/index.ts
- apps/server/src/index.ts
- apps/ui/src/hooks/use-update-polling.ts
- libs/types/src/index.ts
- apps/server/src/routes/updates/routes/info.ts
- apps/ui/vite.config.mts
- apps/server/src/routes/updates/routes/pull.ts
- apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts
- apps/ui/src/hooks/use-settings-migration.ts
- apps/ui/src/lib/electron.ts
- libs/types/src/updates.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/updates/updates-section.tsxapps/ui/src/lib/utils.tsapps/ui/src/routes/__root.tsxapps/server/src/routes/updates/routes/check.tsapps/ui/src/lib/http-api-client.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/store/updates-store.tslibs/types/src/settings.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:
apps/ui/src/components/views/settings-view/updates/updates-section.tsxapps/ui/src/lib/utils.tsapps/ui/src/routes/__root.tsxapps/server/src/routes/updates/routes/check.tsapps/ui/src/lib/http-api-client.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.tsapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/store/updates-store.tslibs/types/src/settings.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/updates/routes/check.tsapps/server/src/routes/updates/common.tsapps/server/src/routes/updates/index.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/components/views/settings-view/updates/updates-section.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/updates/update-notifier.tsxapps/ui/src/components/views/settings-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,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/server/src/routes/updates/common.ts
🧬 Code graph analysis (7)
apps/ui/src/routes/__root.tsx (2)
apps/ui/src/components/updates/update-notifier.tsx (1)
UpdateNotifier(42-170)apps/ui/src/components/updates/index.ts (1)
UpdateNotifier(7-7)
apps/server/src/routes/updates/routes/check.ts (2)
apps/server/src/routes/updates/common.ts (10)
getAutomakerRoot(73-97)isGitAvailable(102-109)isGitRepo(114-121)isValidGitUrl(151-163)getCurrentCommit(126-129)getShortCommit(134-137)withTempGitRemote(169-191)execAsync(15-15)execEnv(59-62)logError(19-19)libs/types/src/updates.ts (1)
UpdateCheckResult(48-72)
apps/server/src/routes/updates/common.ts (3)
apps/server/src/routes/common.ts (1)
createLogError(34-38)apps/server/src/routes/github/routes/common.ts (1)
extendedPath(11-19)apps/ui/scripts/update-version.mjs (1)
packageJsonPath(33-33)
apps/ui/src/components/views/settings-view.tsx (1)
apps/ui/src/components/views/settings-view/updates/updates-section.tsx (1)
UpdatesSection(39-295)
apps/ui/src/store/updates-store.ts (3)
libs/types/src/updates.ts (4)
UpdateApiResponse(103-107)UpdateInfo(13-43)UpdateCheckResult(48-72)UpdatePullResult(77-98)apps/ui/src/lib/electron.ts (1)
getElectronAPI(773-782)apps/ui/src/lib/http-api-client.ts (1)
get(589-611)
libs/types/src/settings.ts (1)
libs/types/src/index.ts (2)
AutoUpdateSettings(86-86)DEFAULT_AUTO_UPDATE_SETTINGS(93-93)
apps/ui/src/store/app-store.ts (3)
libs/types/src/index.ts (2)
AutoUpdateSettings(86-86)DEFAULT_AUTO_UPDATE_SETTINGS(93-93)libs/types/src/settings.ts (2)
AutoUpdateSettings(513-520)DEFAULT_AUTO_UPDATE_SETTINGS(523-527)apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(198-249)
🪛 Biome (2.1.2)
apps/ui/src/routes/__root.tsx
[error] 23-24: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 61-62: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
libs/types/src/settings.ts
[error] 564-564: Expected an expression but instead found '==='.
Expected an expression here.
(parse)
[error] 564-564: Expected an expression but instead found '='.
Expected an expression here.
(parse)
[error] 565-565: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
apps/ui/src/store/app-store.ts
[error] 502-502: Expected a property, or a signature but instead found '==='.
Expected a property, or a signature here.
(parse)
[error] 993-993: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '==='.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 993-993: Expected an expression but instead found '==='.
Expected an expression here.
(parse)
[error] 993-993: Expected an expression but instead found '='.
Expected an expression here.
(parse)
[error] 993-993: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 2953-2953: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '==='.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 2953-2953: Expected an expression but instead found '==='.
Expected an expression here.
(parse)
[error] 2953-2953: Expected an expression but instead found '='.
Expected an expression here.
(parse)
[error] 2914-2953: Invalid assignment to { // Project management projects: state.projects, currentProject: state.currentProject, trashedProjects: state.trashedProjects, projectHistory: state.projectHistory, projectHistoryIndex: state.projectHistoryIndex, // Features - cached locally for faster hydration (authoritative source is server) features: state.features, // UI state currentView: state.currentView, theme: state.theme, sidebarOpen: state.sidebarOpen, chatHistoryOpen: state.chatHistoryOpen, kanbanCardDetailLevel: state.kanbanCardDetailLevel, boardViewMode: state.boardViewMode, // Settings apiKeys: state.apiKeys, maxConcurrency: state.maxConcurrency, // Note: autoModeByProject is intentionally NOT persisted // Auto-mode should always default to OFF on app refresh defaultSkipTests: state.defaultSkipTests, enableDependencyBlocking: state.enableDependencyBlocking, useWorktrees: state.useWorktrees, currentWorktreeByProject: state.currentWorktreeByProject, worktreesByProject: state.worktreesByProject, showProfilesOnly: state.showProfilesOnly, keyboardShortcuts: state.keyboardShortcuts, muteDoneSound: state.muteDoneSound, enhancementModel: state.enhancementModel, validationModel: state.validationModel, autoLoadClaudeMd: state.autoLoadClaudeMd, enableSandboxMode: state.enableSandboxMode, // MCP settings mcpServers: state.mcpServers, mcpAutoApproveTools: state.mcpAutoApproveTools, mcpUnrestrictedTools: state.mcpUnrestrictedTools, // Prompt customization promptCustomization: state.promptCustomization, ======
This expression cannot be assigned to
(parse)
[error] 2953-2953: expected ) but instead found :
Remove :
(parse)
🔇 Additional comments (6)
apps/ui/src/lib/http-api-client.ts (1)
1574-1621: LGTM! Updates API surface is well-structured.The new updates API object follows the established pattern in this file, with properly typed endpoints for check, pull, and info operations. The inline type definitions are comprehensive and the implementation uses the existing HTTP helper methods consistently.
README.md (1)
56-56: LGTM! Clear and well-organized documentation.The Auto-Updates documentation is appropriately placed in the table of contents and includes a helpful summary with links to detailed documentation. The formatting is consistent with the rest of the README.
Also applies to: 634-634, 668-676
apps/ui/src/components/views/settings-view.tsx (1)
21-21: LGTM! Clean integration of the Updates sectionThe UpdatesSection is properly imported and integrated into the settings view. Props are correctly passed from the app store, and the routing is set up appropriately.
Also applies to: 61-62, 185-186
docs/auto-updates.md (1)
1-304: Excellent documentation! Comprehensive and well-structuredThe auto-updates documentation is thorough and covers:
- Feature overview and configuration
- Architecture with clear component diagrams
- Dependency injection patterns for extensibility
- Type definitions and examples
- Troubleshooting guide
The default check interval has been correctly documented as 15 minutes (matching the code), per the resolved past review comment.
apps/ui/src/store/app-store.ts (1)
790-791: Async implementation looks correctThe
setAutoUpdateaction is properly declared as returningPromise<void>and the implementation correctly syncs settings to the server. This matches the pattern used by other async settings actions likesetAutoLoadClaudeMdand addresses the concern raised in the past review comments.Also applies to: 1659-1667
apps/server/src/routes/updates/routes/check.ts (1)
1-143: LGTM! Solid implementation with security best practicesThe update check endpoint is well-implemented with proper security and error handling:
- ✅ Command injection prevented:
isValidGitUrlvalidation (lines 53-59) blocks malicious URLs before shell execution- ✅ Cleanup guaranteed:
withTempGitRemotehelper ensures temporary remotes are always removed via try-finally- ✅ Proper version comparison: Uses
git merge-base --is-ancestorto correctly determine if an update is available- ✅ Graceful error handling: Fetch failures return
success: truewith an error message in the result, while global failures returnsuccess: falseAll concerns from previous reviews have been successfully addressed.
…rkers - Add isValidGitUrl check inside withTempGitRemote for extra security - Remove leftover ======= conflict markers from rebase 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/types/src/settings.ts (1)
557-564: 🚨 Critical: DuplicateenableSandboxModeproperty in object literalLines 557 and 563 both define
enableSandboxModein theDEFAULT_GLOBAL_SETTINGSobject. In JavaScript/TypeScript, the last duplicate key wins, so Line 563 (true) will override Line 557 (false). This is a leftover merge conflict artifact.Remove the duplicate at Line 563:
🔎 Proposed fix
mcpAutoApproveTools: true, mcpUnrestrictedTools: true, - enableSandboxMode: true, autoUpdate: DEFAULT_AUTO_UPDATE_SETTINGS, };apps/ui/src/store/app-store.ts (1)
985-991: 🚨 Critical: DuplicateenableSandboxModeproperty in initialStateLines 985 and 990 both define
enableSandboxModein theinitialStateobject. The second definition (Line 990:true) will override the first (Line 985:false). This is a leftover merge conflict artifact.Remove the duplicate at Line 990:
🔎 Proposed fix
mcpAutoApproveTools: true, // Default to enabled - bypass permission prompts for MCP tools mcpUnrestrictedTools: true, // Default to enabled - don't filter allowedTools when MCP enabled promptCustomization: {}, // Empty by default - all prompts use built-in defaults - enableSandboxMode: true, // Default to enabled for security (can be disabled if issues occur) autoUpdate: DEFAULT_AUTO_UPDATE_SETTINGS, // Default auto-update settings
🧹 Nitpick comments (1)
apps/server/src/routes/updates/common.ts (1)
21-62: Consider consolidating PATH extension logic.The extended PATH configuration here is more comprehensive than the similar logic in
apps/server/src/routes/github/routes/common.ts. Consider moving this enhanced version to a shared location (e.g.,apps/server/src/routes/common.ts) to eliminate duplication and ensure consistent PATH handling across all server routes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/server/src/routes/updates/common.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.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
- libs/types/src/index.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:
libs/types/src/settings.tsapps/ui/src/store/app-store.tsapps/server/src/routes/updates/common.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.tsapps/ui/src/store/app-store.tsapps/server/src/routes/updates/common.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/updates/common.ts
🧠 Learnings (1)
📚 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,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/server/src/routes/updates/common.ts
🧬 Code graph analysis (2)
libs/types/src/settings.ts (1)
libs/types/src/index.ts (2)
AutoUpdateSettings(86-86)DEFAULT_AUTO_UPDATE_SETTINGS(93-93)
apps/server/src/routes/updates/common.ts (3)
apps/server/src/routes/common.ts (1)
createLogError(34-38)apps/server/src/routes/github/routes/common.ts (1)
extendedPath(11-19)apps/ui/scripts/update-version.mjs (1)
packageJsonPath(33-33)
🔇 Additional comments (12)
libs/types/src/settings.ts (2)
507-526: LGTM! Well-structured auto-update configuration.The
AutoUpdateSettingsinterface and defaults are clearly documented and provide sensible values for the auto-update feature.
368-370: LGTM! Proper integration into GlobalSettings.The
autoUpdatefield is correctly typed and documented.apps/ui/src/store/app-store.ts (5)
15-17: LGTM! Correct imports from shared package.Imports follow the coding guideline to use
@automaker/*packages instead of relative paths.
500-501: LGTM! Proper addition to AppState.The
autoUpdatefield is correctly typed and documented.
788-789: LGTM! Return type correctly matches async implementation.The
setAutoUpdatesignature properly declaresPromise<void>return type, consistent with its async implementation and other async settings actions likesetAutoLoadClaudeMd.
1656-1664: LGTM! Correct async implementation with server sync.The implementation properly merges settings into state and persists changes to the server, following the same pattern as other async settings actions.
2949-2949: LGTM! Proper persistence configuration.The
autoUpdatestate is correctly included in the persisted state viapartialize.apps/server/src/routes/updates/common.ts (5)
1-19: LGTM: Proper imports and setup.The imports from
@automaker/utilsfollow the coding guidelines, and the re-export pattern from the shared server utilities (../common.js) is appropriate for intra-app code sharing.
68-97: Excellent: Robust root discovery implementation.The upward traversal approach successfully addresses the previous fragility concerns. By checking each
package.jsonforname === 'automaker', the function correctly identifies the monorepo root regardless of where this file is located within the directory structure.
99-145: LGTM: Well-structured Git utility functions.The Git helper functions are properly implemented with consistent error handling and appropriate use of
execEnvfor extended PATH support. The boolean checks gracefully returnfalseon errors, while data-fetching functions allow errors to propagate.
147-163: Excellent: Defense-in-depth URL validation.The
isValidGitUrlimplementation successfully addresses previous security concerns by validating both protocol schemes and blocking shell metacharacters. This provides robust protection against command injection vulnerabilities.
165-196: Excellent: Comprehensive temp remote management.This implementation successfully addresses all previous concerns:
- ✅ Validates the URL at entry (line 175) for defense-in-depth security
- ✅ Uses
crypto.randomBytes(8)for collision-resistant random remote names (line 179)- ✅ Ensures cleanup via
finallyblock with graceful error handlingThe pattern centralizes temporary remote lifecycle management effectively.
…omise - Add EventEmitter parameter to createUpdatesRoutes for future progress streaming - Pass events to pull handler (available for WebSocket progress updates) - Await fetchInfo() after pull to ensure state is updated before emitting events 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 participant
Summary
This PR adds an automatic update system that checks for new versions from an upstream repository and allows users to update with a single click. The system uses git-based version comparison to detect available updates and performs safe fast-forward merges.
Features
merge-base --is-ancestorArchitecture
flowchart TB subgraph UI["UI Layer"] UN[UpdateNotifier] US[UpdatesSection] end subgraph State["State Management"] Store[updates-store.ts] Hook[use-update-polling.ts] end subgraph API["API Layer"] Client[IUpdatesApiClient] end subgraph Server["Server Routes"] Check[GET /api/updates/check] Pull[POST /api/updates/pull] Info[GET /api/updates/info] end subgraph Git["Git Operations"] Fetch[git fetch] Merge[git merge --ff-only] Compare[git merge-base --is-ancestor] end UN --> Store US --> Store Hook --> Store Store --> Client Client --> Check Client --> Pull Client --> Info Check --> Fetch Check --> Compare Pull --> Fetch Pull --> MergeUpdate Check Flow
sequenceDiagram participant U as UI (Polling/Manual) participant S as Store participant A as API Client participant R as Server Route participant G as Git U->>S: checkForUpdates() S->>A: checkForUpdates() A->>R: GET /api/updates/check R->>G: git fetch (temp remote) R->>G: git merge-base --is-ancestor G-->>R: exit code (0=behind, 1=ahead) R-->>A: UpdateCheckResult A-->>S: {updateAvailable, versions} S-->>U: State updated Note over U: Toast shown if update availableFiles Changed
New Files
apps/server/src/routes/updates/- Server routes for update operationsapps/ui/src/store/updates-store.ts- Zustand store with DI supportapps/ui/src/hooks/use-update-polling.ts- Polling logic (when to check)apps/ui/src/components/updates/- UpdateNotifier componentapps/ui/src/components/views/settings-view/updates/- Settings UIlibs/types/src/updates.ts- Type definitionsdocs/auto-updates.md- Documentation with DI examplesModified Files
libs/types/src/settings.ts- Added AutoUpdateSettings interfaceapps/ui/src/store/app-store.ts- Added autoUpdate stateapps/ui/src/hooks/use-settings-migration.ts- Sync autoUpdate to serverapps/server/src/index.ts- Register update routesREADME.md- Added auto-updates sectionSettings
Test Plan
Summary by CodeRabbit
New Features
UI
Stores & Hooks
API
Documentation
Types
✏️ Tip: You can customize this high-level summary in your review settings.