feat(desktop): add required update mechanism#552
Conversation
📝 WalkthroughWalkthroughAdds a backend endpoint returning minimum desktop version, injects build-time app version into the preload API, exposes that version to renderer, adds a hook to fetch/compare versions, a UI to require updates, integrates the check into the main screen, and adds a TRPC mutation to trigger update checks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DesktopApp as Desktop App (Renderer)
participant Preload as Preload API (window.App)
participant Hook as useVersionCheck
participant Backend as Backend API
participant TRPC as TRPC Auto-updater
participant AutoUpdater as Main Auto-updater
participant UpdateUI as UpdateRequiredPage
User->>DesktopApp: Launch
DesktopApp->>Hook: mount/useVersionCheck()
Hook->>Preload: read appVersion
Preload-->>Hook: return __APP_VERSION__
Hook->>Backend: GET /api/desktop/version
Backend-->>Hook: { minimumVersion, message? }
Hook->>Hook: compare(current, minimum) [semver]
alt current < minimum
Hook-->>DesktopApp: isBlocked = true, requirements
DesktopApp->>UpdateUI: render with props
User->>UpdateUI: "Check for Updates"
UpdateUI->>TRPC: mutation check()
TRPC->>AutoUpdater: invoke checkForUpdates()
AutoUpdater-->>TRPC: update status events (subscribe)
TRPC-->>UpdateUI: progress/status via subscription
UpdateUI->>User: show progress / install / manual link
else current >= minimum
Hook-->>DesktopApp: isBlocked = false
DesktopApp->>User: proceed to normal app
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 ignored due to path filters (1)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used📓 Path-based instructions (5)apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/**/src/**/**/[A-Z]*.tsx📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (7)📚 Learning: 2025-12-28T01:56:39.031ZApplied to files:
📚 Learning: 2025-12-28T01:56:39.031ZApplied to files:
📚 Learning: 2025-12-21T04:39:28.543ZApplied to files:
📚 Learning: 2025-12-21T04:39:28.543ZApplied to files:
📚 Learning: 2025-12-28T01:56:39.031ZApplied to files:
📚 Learning: 2025-12-28T01:56:39.031ZApplied to files:
📚 Learning: 2025-12-28T01:56:39.031ZApplied to files:
🧬 Code graph analysis (3)apps/desktop/src/renderer/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (2)
apps/desktop/src/renderer/screens/main/index.tsx (4)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx (1)
30-36: Consider handling mutation errors.If
checkMutationorinstallMutationfail at the tRPC/IPC level (distinct from auto-updater errors tracked inupdateStatus), users receive no feedback. Consider addingonErrorcallbacks or checkingcheckMutation.error/installMutation.errorin the UI.🔎 Example approach
const handleCheckForUpdate = () => { - checkMutation.mutate(); + checkMutation.mutate(undefined, { + onError: (err) => console.error("Failed to trigger update check:", err), + }); }; const handleInstall = () => { - installMutation.mutate(); + installMutation.mutate(undefined, { + onError: (err) => console.error("Failed to trigger install:", err), + }); };apps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts (2)
79-87: Inconsistent error handling between API errors and network errors.When
!response.ok, the hook fails open witherror: null(lines 56-65). However, on network errors (catch block), it fails open but setserrorto the caught error. This inconsistency could confuse consumers checking bothisBlockedanderror.Consider either:
- Setting
error: nullhere too (fully silent fail-open), or- Creating an error for non-ok responses as well for consistent observability.
🔎 Option 1: Silent fail-open (consistent with non-ok handling)
} catch (error) { // Fail open on network errors setState({ isLoading: false, isBlocked: false, requirements: null, - error: error instanceof Error ? error : new Error("Unknown error"), + error: null, }); }
47-88: Consider aborting fetch on unmount to prevent state updates after unmount.If the component unmounts while
fetchis in-flight,setStatewill be called on an unmounted component. While React 18+ handles this more gracefully, adding anAbortControllerwould be cleaner and avoid potential memory leaks.🔎 Example with AbortController
- const checkVersion = useCallback(async () => { + const checkVersion = useCallback(async (signal?: AbortSignal) => { if (!hasVerified.current) { setState((prev) => ({ ...prev, isLoading: true })); } try { - const response = await fetch(`${API_BASE_URL}/api/desktop/version`); + const response = await fetch(`${API_BASE_URL}/api/desktop/version`, { signal }); // ... rest of implementation } catch (error) { + if (error instanceof Error && error.name === "AbortError") { + return; // Silently ignore aborted requests + } // Fail open on network errors // ... } }, []); useEffect(() => { + const controller = new AbortController(); - checkVersion(); + checkVersion(controller.signal); const handleOnline = () => { if (!hasVerified.current) { - checkVersion(); + checkVersion(controller.signal); } }; window.addEventListener("online", handleOnline); - return () => window.removeEventListener("online", handleOnline); + return () => { + controller.abort(); + window.removeEventListener("online", handleOnline); + }; }, [checkVersion]);Also applies to: 90-103
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api/src/app/api/desktop/version/route.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/src/renderer/env.d.tsapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.tsapps/desktop/src/renderer/screens/main/index.tsx
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/env.d.tsapps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/electron.vite.config.tsapps/desktop/src/preload/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/renderer/env.d.tsapps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/api/src/app/api/desktop/version/route.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/electron.vite.config.tsapps/desktop/src/preload/index.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/renderer/env.d.tsapps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/preload/index.ts
apps/**/src/**/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Structure component folders with one component per file using format
ComponentName/ComponentName.tsxwithindex.tsbarrel export
Files:
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define all Electron IPC channel types in
apps/desktop/src/shared/ipc-channels.tsbefore implementing handlers
Files:
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/screens/main/index.tsx
🧠 Learnings (8)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in `src/main/index.ts` with `override: true` before any other imports in the desktop app
Applied to files:
apps/desktop/src/renderer/env.d.tsapps/desktop/electron.vite.config.tsapps/desktop/src/preload/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/env.d.tsapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/electron.vite.config.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/electron.vite.config.tsapps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx} : Define all Electron IPC channel types in `apps/desktop/src/shared/ipc-channels.ts` before implementing handlers
Applied to files:
apps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/electron.vite.config.tsapps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/**/src/**/**/[A-Z]*.tsx : Structure component folders with one component per file using format `ComponentName/ComponentName.tsx` with `index.ts` barrel export
Applied to files:
apps/desktop/src/renderer/components/UpdateRequiredPage/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/{renderer,lib/electron-router-dom.ts} : Never import Node.js modules in Electron renderer or shared code (`src/renderer/` and `src/lib/electron-router-dom.ts`)
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/main/**/*ipcs.ts : In Electron IPC handlers, always accept a single object parameter, not positional parameters, to maintain type safety
Applied to files:
apps/desktop/src/preload/index.ts
🧬 Code graph analysis (2)
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx (3)
apps/desktop/src/shared/auto-update.ts (2)
AUTO_UPDATE_STATUS(1-7)RELEASES_URL(12-12)apps/desktop/src/renderer/screens/main/components/Background.tsx (1)
Background(1-39)apps/desktop/src/renderer/screens/main/components/AppFrame.tsx (1)
AppFrame(5-7)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
apps/desktop/src/main/lib/auto-updater.ts (1)
checkForUpdates(63-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (15)
apps/desktop/src/renderer/hooks/useVersionCheck/index.ts (1)
1-1: LGTM!Standard barrel export following the project's component structure conventions.
apps/desktop/src/renderer/env.d.ts (1)
3-3: LGTM!Proper ambient type declaration for the Vite-injected global constant.
apps/desktop/src/renderer/components/UpdateRequiredPage/index.ts (1)
1-1: LGTM!Standard barrel export following the project's component structure conventions.
apps/desktop/src/preload/index.ts (1)
4-4: LGTM!The implementation correctly exposes the build-time app version through the preload bridge, making it accessible to the renderer process via
window.App.appVersion.Also applies to: 19-19
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
37-39: LGTM!The mutation correctly triggers the update check via tRPC IPC. The void return is appropriate since update status is communicated through the subscription mechanism (
subscribeendpoint).apps/desktop/electron.vite.config.ts (1)
11-11: LGTM!The version injection is correctly configured for both preload and renderer processes, enabling version checks throughout the app.
Also applies to: 140-140, 179-180
apps/api/src/app/api/desktop/version/route.ts (2)
11-12: Hardcoded version constant is noted for future improvement.The TODO correctly identifies that moving this to a database or environment variable would enable version updates without redeployment. This is a known limitation documented by the developer.
14-20: LGTM!The endpoint structure is correct for Next.js App Router, and the response shape matches the requirements for the version check flow.
apps/desktop/src/renderer/screens/main/index.tsx (2)
40-46: LGTM!Version check is correctly positioned before authentication, ensuring outdated clients are blocked early in the app lifecycle.
180-203: LGTM!The loading and blocking UI flow is correctly implemented with proper null checks on
versionRequirementsbefore rendering the update page.apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx (2)
1-6: LGTM!Imports are well-organized. Good use of shared constants (
AUTO_UPDATE_STATUS,RELEASES_URL) and alias paths per coding guidelines.
42-104: LGTM!The UI implementation handles all states cleanly with appropriate loading indicators, error displays, and button states. Good use of the shared
BackgroundandAppFramecomponents for visual consistency.apps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts (3)
68-68: LGTM!The
window.App.appVersionaccess is correctly wired through the preload script as documented in the AI summary.
15-15: Environment variable is correctly configured.NEXT_PUBLIC_API_URLis properly injected inelectron.vite.config.tsfor the renderer build (line 154-157) via Vite'sdefinemechanism, with a sensible fallback tohttps://api.superset.sh. The naming convention, while Next.js-styled, works correctly with electron-vite's build process.
21-34: ThecompareVersionsfunction is adequate for the app's current numeric-only version format.The function assumes numeric-only version parts and will produce incorrect results if versions include prerelease suffixes (e.g.,
"1.0.0-beta.1"). However, the app currently uses purely numeric versions ("0.0.39"), and both version sources—window.App.appVersionand the API response—are controlled internally, so this is not a practical concern. No changes needed unless the app's versioning strategy changes to include prerelease identifiers.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
force-pushed
the
smiling-spider-85c6c8
branch
from
61a1aba to
878ef5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts (2)
20-33: Semver comparison doesn't handle prerelease tags.The
Number()conversion will produceNaNfor version parts containing non-numeric characters (e.g.,"1.0.0-beta"→Number("0-beta")=NaN). Comparisons withNaNare always false, which could lead to unexpected behavior.If all versions will strictly follow numeric
X.Y.Zformat (as the current"0.0.39"suggests), this is fine. Otherwise, consider using a library likesemveror adding explicit validation.
80-88: Minor: Error stored but fail-open means it's unused.The error is captured in state, but since
isBlockedisfalseon errors, the consuming component likely won't act on it. This is fine if you want the option to log or report errors later, but consider whether the error state adds value in a fail-open design.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/app/api/desktop/version/route.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.tsapps/desktop/src/renderer/screens/main/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/hooks/useVersionCheck/index.ts
- apps/desktop/src/preload/index.ts
- apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
- apps/desktop/electron.vite.config.ts
- apps/desktop/src/renderer/screens/main/index.tsx
- apps/desktop/src/renderer/components/UpdateRequiredPage/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/api/src/app/api/desktop/version/route.tsapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
apps/desktop/src/main/lib/auto-updater.ts (1)
checkForUpdates(63-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy API
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
37-39: LGTM!The new
checkmutation follows the established pattern of other mutations in this router and correctly delegates to thecheckForUpdatesfunction which handles its own error logging internally.apps/api/src/app/api/desktop/version/route.ts (1)
1-12: LGTM - simple and effective endpoint.The implementation is straightforward and correctly returns the minimum version requirement. The fail-open behavior on the client side means this endpoint being unavailable won't block users, which is the right tradeoff.
One minor consideration: if this version needs to change frequently or across environments, you might eventually want to source it from an environment variable. For now, the hardcoded constant is fine and makes the "how to force update" process clear.
apps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts (1)
91-104: Good implementation of network reconnect handling.The
onlineevent listener with thehasVerifiedguard ensures re-checks happen only when needed, and cleanup is properly handled. This aligns well with the PR's "re-check on reconnect" behavior.
| const requirements: VersionRequirements = await response.json(); | ||
| const currentVersion = window.App.appVersion; | ||
| const isBlocked = | ||
| compareVersions(currentVersion, requirements.minimumVersion) < 0; |
There was a problem hiding this comment.
Add defensive check for window.App.appVersion.
If window.App.appVersion is undefined (e.g., preload didn't initialize correctly), compareVersions would receive undefined and produce unexpected results due to NaN comparisons. Consider failing open gracefully when the current version cannot be determined.
🔎 Suggested defensive check
const requirements: VersionRequirements = await response.json();
const currentVersion = window.App.appVersion;
+
+ // Fail open if we can't determine the current version
+ if (!currentVersion) {
+ setState({
+ isLoading: false,
+ isBlocked: false,
+ requirements: null,
+ error: null,
+ });
+ return;
+ }
+
const isBlocked =
compareVersions(currentVersion, requirements.minimumVersion) < 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const requirements: VersionRequirements = await response.json(); | |
| const currentVersion = window.App.appVersion; | |
| const isBlocked = | |
| compareVersions(currentVersion, requirements.minimumVersion) < 0; | |
| const requirements: VersionRequirements = await response.json(); | |
| const currentVersion = window.App.appVersion; | |
| // Fail open if we can't determine the current version | |
| if (!currentVersion) { | |
| setState({ | |
| isLoading: false, | |
| isBlocked: false, | |
| requirements: null, | |
| error: null, | |
| }); | |
| return; | |
| } | |
| const isBlocked = | |
| compareVersions(currentVersion, requirements.minimumVersion) < 0; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts around
lines 68 to 71, the code calls compareVersions with window.App.appVersion which
can be undefined; add a defensive check that reads and validates
window.App?.appVersion before calling compareVersions, and if it's missing or
not a string treat the check as "fail open" (set isBlocked = false) and
optionally log or warn about the missing version so the app does not mistakenly
block users when the preload didn't initialize.
878ef5a to
0e39477
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/src/app/api/desktop/version/route.ts (1)
1-13: LGTM!The endpoint is straightforward and correctly returns the minimum version. The fail-open design (desktop allows access if API is unreachable) handles availability concerns appropriately.
Consider moving
MINIMUM_DESKTOP_VERSIONto an environment variable for easier deployment-time changes without code modifications. However, the current approach with a hardcoded constant is acceptable since version bumps are typically tied to code releases anyway.apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx (1)
35-39: Subscription error handling missing.The subscription should handle potential connection errors to avoid silent failures. Consider adding an
onErrorcallback to gracefully handle subscription failures.🔎 Proposed fix
// Subscribe to auto-update status changes trpc.autoUpdate.subscribe.useSubscription(undefined, { onData: (event) => { setUpdateStatus({ status: event.status, error: event.error }); }, + onError: (error) => { + console.error("[UpdateRequiredPage] Subscription error:", error); + setUpdateStatus({ status: AUTO_UPDATE_STATUS.ERROR, error: error.message }); + }, });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/api/src/app/api/desktop/version/route.tsapps/desktop/electron.vite.config.tsapps/desktop/package.jsonapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsxapps/desktop/src/renderer/components/UpdateRequiredPage/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/index.tsapps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.tsapps/desktop/src/renderer/screens/main/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/hooks/useVersionCheck/useVersionCheck.ts
- apps/desktop/src/renderer/hooks/useVersionCheck/index.ts
- apps/desktop/src/renderer/components/UpdateRequiredPage/index.ts
- apps/desktop/src/renderer/screens/main/index.tsx
- apps/desktop/electron.vite.config.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/preload/index.tsapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/preload/index.tsapps/api/src/app/api/desktop/version/route.tsapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/preload/index.tsapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
apps/**/src/**/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Structure component folders with one component per file using format
ComponentName/ComponentName.tsxwithindex.tsbarrel export
Files:
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define all Electron IPC channel types in
apps/desktop/src/shared/ipc-channels.tsbefore implementing handlers
Files:
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
🧠 Learnings (8)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in `src/main/index.ts` with `override: true` before any other imports in the desktop app
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx} : Define all Electron IPC channel types in `apps/desktop/src/shared/ipc-channels.ts` before implementing handlers
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/{renderer,lib/electron-router-dom.ts} : Never import Node.js modules in Electron renderer or shared code (`src/renderer/` and `src/lib/electron-router-dom.ts`)
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/main/**/*ipcs.ts : In Electron IPC handlers, always accept a single object parameter, not positional parameters, to maintain type safety
Applied to files:
apps/desktop/src/preload/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx
🧬 Code graph analysis (2)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
apps/desktop/src/main/lib/auto-updater.ts (1)
checkForUpdates(63-73)
apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx (3)
apps/desktop/src/shared/auto-update.ts (3)
AutoUpdateStatus(9-10)AUTO_UPDATE_STATUS(1-7)RELEASES_URL(12-12)apps/desktop/src/renderer/screens/main/components/Background.tsx (1)
Background(1-39)apps/desktop/src/renderer/screens/main/components/AppFrame.tsx (1)
AppFrame(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/package.json (1)
112-112: LGTM!The
semverpackage is the appropriate choice for semantic version comparison. The runtime dependency pairs correctly with the existing@types/semverdev dependency.apps/desktop/src/preload/index.ts (2)
4-5: LGTM!Build-time constant declaration for the app version injected via Vite define.
19-19: LGTM!Correctly exposes the app version to the renderer via
contextBridge. This follows the standard Electron pattern for exposing static values and integrates cleanly with the existing API object.apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
37-39: LGTM!The
checkmutation follows the existing patterns in this router and correctly delegates tocheckForUpdates(). As per coding guidelines, tRPC is appropriately used for this IPC communication.apps/desktop/src/renderer/components/UpdateRequiredPage/UpdateRequiredPage.tsx (1)
59-121: LGTM!The component UI is well-structured with clear visual hierarchy, proper loading states, and fallback messaging. The conditional rendering between "Install & Restart" and "Check for Update" buttons correctly reflects the update lifecycle.
Add ability to force desktop users to update when breaking changes are deployed. The app checks version requirements from the API on startup and blocks usage if outdated. Changes: - Add GET /api/desktop/version endpoint returning minimum version - Add useVersionCheck hook with network reconnection handling - Add UpdateRequiredPage component for blocking UI - Add version gate in MainScreen before auth check - Expose app version via window.App.appVersion - Add check mutation to auto-update tRPC router Behavior: - Checks version on startup - Re-checks when network comes back online (if not verified) - Fail-open on network errors (allows offline use) - Uses existing auto-updater for install flow 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
saddlepaddle
force-pushed
the
smiling-spider-85c6c8
branch
from
0e39477 to
0a70717
Compare
1 participant
Summary
Changes
API:
GET /api/desktop/version- Returns{ minimumVersion, message? }Desktop:
useVersionCheckhook - Fetches and compares versions, re-checks on network reconnectUpdateRequiredPagecomponent - Blocking UI with update buttonwindow.App.appVersionexposed via preloadcheckmutation added to auto-update tRPC routerBehavior
How to Force Update
Update
MINIMUM_DESKTOP_VERSIONinapps/api/src/app/api/desktop/version/route.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.