fix(desktop): resolve v2 default from local db, not stale localStorage#4176
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR migrates V2 cloud opt-in initialization from eager, localStorage-based defaults to lazy resolution via database existence checks. New tRPC endpoints expose workspace and project existence; the store adopts a tri-state model; a resolver component populates the opt-in and fresh-install flags on mount; and prior initialization logic is removed. ChangesV2 Cloud Opt-In Lazy Initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts`:
- Around line 98-105: The hasAny procedure currently counts rows regardless of
soft-deletion; update the query inside hasAny to exclude soft-deleted workspaces
by adding a condition that the soft-delete column is null (e.g.,
workspaces.deletedAt is null) to the localDb select-from chain so it only
returns non-deleted rows; modify the query that uses localDb.select({ id:
workspaces.id }).from(workspaces).limit(1).all() inside hasAny to include the
where/filter on workspaces.deletedAt being null.
In
`@apps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsx`:
- Around line 13-20: The Promise.all([...].then(...)) call can reject and cause
an unhandled rejection; wrap the fetches with error handling by appending a
.catch handler (or convert to async/await with try/catch) around
Promise.all(utils.workspaces.hasAny.fetch(), utils.projects.hasAny.fetch()) so
any rejection is caught, log or report the error, and still call setOptInV2 with
a safe default (e.g., false) unless cancelled; ensure you check cancelled and
useV2LocalOverrideStore.getState().optInV2 in both the success .then and the
.catch paths so the resolver always sets a value or exits cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9317ef3-b517-4bf7-89c9-ce4c265eb8c2
📒 Files selected for processing (8)
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsxapps/desktop/src/renderer/components/V2DefaultResolver/index.tsapps/desktop/src/renderer/hooks/useIsV2CloudEnabled.tsapps/desktop/src/renderer/lib/hasPriorSupersetUsage.tsapps/desktop/src/renderer/routes/-layout.tsxapps/desktop/src/renderer/stores/v2-local-override.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/lib/hasPriorSupersetUsage.ts
| void Promise.all([ | ||
| utils.workspaces.hasAny.fetch(), | ||
| utils.projects.hasAny.fetch(), | ||
| ]).then(([hasWorkspace, hasProject]) => { | ||
| if (cancelled) return; | ||
| if (useV2LocalOverrideStore.getState().optInV2 !== null) return; | ||
| setOptInV2(!hasWorkspace && !hasProject); | ||
| }); |
There was a problem hiding this comment.
Handle resolver fetch failures to avoid unhandled rejections.
If either hasAny.fetch() rejects, the promise chain is unhandled and the resolver silently never sets a value for this run.
Suggested fix
void Promise.all([
utils.workspaces.hasAny.fetch(),
utils.projects.hasAny.fetch(),
- ]).then(([hasWorkspace, hasProject]) => {
- if (cancelled) return;
- if (useV2LocalOverrideStore.getState().optInV2 !== null) return;
- setOptInV2(!hasWorkspace && !hasProject);
- });
+ ])
+ .then(([hasWorkspace, hasProject]) => {
+ if (cancelled) return;
+ if (useV2LocalOverrideStore.getState().optInV2 !== null) return;
+ setOptInV2(!hasWorkspace && !hasProject);
+ })
+ .catch(() => {
+ // Keep unresolved state on failure; retry on next mount.
+ });📝 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.
| void Promise.all([ | |
| utils.workspaces.hasAny.fetch(), | |
| utils.projects.hasAny.fetch(), | |
| ]).then(([hasWorkspace, hasProject]) => { | |
| if (cancelled) return; | |
| if (useV2LocalOverrideStore.getState().optInV2 !== null) return; | |
| setOptInV2(!hasWorkspace && !hasProject); | |
| }); | |
| void Promise.all([ | |
| utils.workspaces.hasAny.fetch(), | |
| utils.projects.hasAny.fetch(), | |
| ]) | |
| .then(([hasWorkspace, hasProject]) => { | |
| if (cancelled) return; | |
| if (useV2LocalOverrideStore.getState().optInV2 !== null) return; | |
| setOptInV2(!hasWorkspace && !hasProject); | |
| }) | |
| .catch(() => { | |
| // Keep unresolved state on failure; retry on next mount. | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsx`
around lines 13 - 20, The Promise.all([...].then(...)) call can reject and cause
an unhandled rejection; wrap the fetches with error handling by appending a
.catch handler (or convert to async/await with try/catch) around
Promise.all(utils.workspaces.hasAny.fetch(), utils.projects.hasAny.fetch()) so
any rejection is caught, log or report the error, and still call setOptInV2 with
a safe default (e.g., false) unless cancelled; ensure you check cancelled and
useV2LocalOverrideStore.getState().optInV2 in both the success .then and the
.catch paths so the resolver always sets a value or exits cleanly.
Greptile SummaryThis PR replaces the broken
Confidence Score: 3/5The fix is directionally correct and the logic is sound for new installs, but the async fetch path has no error handling — a failed IPC call on startup silently leaves The resolver's apps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsx — the async fetch needs a
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsx | New component that resolves the v2 default by querying local DB; missing error handling means a failed fetch leaves optInV2 as null indefinitely, silently keeping fresh users on v1. |
| apps/desktop/src/renderer/stores/v2-local-override.ts | Correctly changes initial optInV2 to null (unresolved) and removes the stale hasPriorSupersetUsage dependency; self-healing described in PR description won't apply to users who already persisted true from the broken release. |
| apps/desktop/src/renderer/hooks/useIsV2CloudEnabled.ts | Correctly tightened to optInV2 === true so null (unresolved) maps to v1, preventing premature v2 activation during async resolution. |
| apps/desktop/src/lib/trpc/routers/projects/projects.ts | Adds hasAny procedure with an efficient LIMIT 1 query; straightforward and correct. |
| apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts | Adds hasAny procedure with an efficient LIMIT 1 query; mirrors the projects implementation and is correct. |
| apps/desktop/src/renderer/routes/-layout.tsx | Mounts V2DefaultResolver inside ElectronTRPCProvider but outside AuthProvider, which is correct since hasAny uses publicProcedure. |
| apps/desktop/src/renderer/lib/hasPriorSupersetUsage.ts | Deleted; was checking a stale tabs-storage localStorage key that has been null on every install since the tabs state was migrated to lowdb in #303. |
Sequence Diagram
sequenceDiagram
participant RL as RootLayout
participant VDR as V2DefaultResolver
participant Store as v2-local-override (Zustand)
participant TRPC as electronTrpc
participant DB as localDb (SQLite)
RL->>Store: hydrate from localStorage ("v2-local-override-v2")
Note over Store: optInV2 = null (no prior value)<br/>or true/false (returning user)
RL->>VDR: mount
VDR->>Store: read optInV2
alt "optInV2 !== null"
VDR-->>VDR: return early (existing value preserved)
else "optInV2 === null"
VDR->>TRPC: workspaces.hasAny.fetch()
VDR->>TRPC: projects.hasAny.fetch()
TRPC->>DB: SELECT id FROM workspaces LIMIT 1
TRPC->>DB: SELECT id FROM projects LIMIT 1
DB-->>TRPC: hasWorkspace, hasProject
TRPC-->>VDR: [hasWorkspace, hasProject]
VDR->>Store: "setOptInV2(!hasWorkspace && !hasProject)"
Store-->>Store: persist to localStorage
end
Note over Store: useIsV2CloudEnabled returns optInV2 === true
Comments Outside Diff (1)
-
apps/desktop/src/renderer/stores/v2-local-override.ts, line 15-26 (link)Self-healing does not apply to users who already ran the broken release
The PR description states "users who got force-pulled into v2 but never explicitly toggled have no persisted override yet, so they self-heal back to v1 on next launch." However, Zustand's
persistmiddleware writes state to localStorage on every state change. Any user who launched the broken build (commitaf7a1a21d) will already have{"state":{"optInV2":true},...}saved under"v2-local-override-v2". On upgrade, the persist middleware hydrates that storedtrue, sooptInV2 !== null, andV2DefaultResolverexits early without querying the DB. Those users stay on v2 rather than self-healing to v1.Self-healing only benefits users who (a) cleared their localStorage, or (b) never launched the app under the broken release. If the broken release was already shipped, a persist key rotation (e.g. rename to
"v2-local-override-v3") would be needed to force re-resolution for everyone.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/stores/v2-local-override.ts Line: 15-26 Comment: **Self-healing does not apply to users who already ran the broken release** The PR description states "users who got force-pulled into v2 but never explicitly toggled have no persisted override yet, so they self-heal back to v1 on next launch." However, Zustand's `persist` middleware writes state to localStorage on every state change. Any user who launched the broken build (commit `af7a1a21d`) will already have `{"state":{"optInV2":true},...}` saved under `"v2-local-override-v2"`. On upgrade, the persist middleware hydrates that stored `true`, so `optInV2 !== null`, and `V2DefaultResolver` exits early without querying the DB. Those users stay on v2 rather than self-healing to v1. Self-healing only benefits users who (a) cleared their localStorage, or (b) never launched the app under the broken release. If the broken release was already shipped, a persist key rotation (e.g. rename to `"v2-local-override-v3"`) would be needed to force re-resolution for everyone. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsx:13-20
**Unhandled rejection leaves `optInV2` permanently `null`**
The `Promise.all` is prefixed with `void`, which silences the unhandled-rejection warning but means a tRPC error (e.g., IPC hiccup on startup) causes both `hasAny` calls to throw, the `.then` callback never runs, and `optInV2` stays `null` forever for that session. Because `useIsV2CloudEnabled` treats `null` as `false`, a fresh-install user would see v1 indefinitely and never get the intended v2 default — even after the IPC channel recovers — until they manually restart or toggle.
### Issue 2 of 2
apps/desktop/src/renderer/stores/v2-local-override.ts:15-26
**Self-healing does not apply to users who already ran the broken release**
The PR description states "users who got force-pulled into v2 but never explicitly toggled have no persisted override yet, so they self-heal back to v1 on next launch." However, Zustand's `persist` middleware writes state to localStorage on every state change. Any user who launched the broken build (commit `af7a1a21d`) will already have `{"state":{"optInV2":true},...}` saved under `"v2-local-override-v2"`. On upgrade, the persist middleware hydrates that stored `true`, so `optInV2 !== null`, and `V2DefaultResolver` exits early without querying the DB. Those users stay on v2 rather than self-healing to v1.
Self-healing only benefits users who (a) cleared their localStorage, or (b) never launched the app under the broken release. If the broken release was already shipped, a persist key rotation (e.g. rename to `"v2-local-override-v3"`) would be needed to force re-resolution for everyone.
Reviews (1): Last reviewed commit: "fix(desktop): resolve v2 default from lo..." | Re-trigger Greptile
| void Promise.all([ | ||
| utils.workspaces.hasAny.fetch(), | ||
| utils.projects.hasAny.fetch(), | ||
| ]).then(([hasWorkspace, hasProject]) => { | ||
| if (cancelled) return; | ||
| if (useV2LocalOverrideStore.getState().optInV2 !== null) return; | ||
| setOptInV2(!hasWorkspace && !hasProject); | ||
| }); |
There was a problem hiding this comment.
Unhandled rejection leaves
optInV2 permanently null
The Promise.all is prefixed with void, which silences the unhandled-rejection warning but means a tRPC error (e.g., IPC hiccup on startup) causes both hasAny calls to throw, the .then callback never runs, and optInV2 stays null forever for that session. Because useIsV2CloudEnabled treats null as false, a fresh-install user would see v1 indefinitely and never get the intended v2 default — even after the IPC channel recovers — until they manually restart or toggle.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/V2DefaultResolver/V2DefaultResolver.tsx
Line: 13-20
Comment:
**Unhandled rejection leaves `optInV2` permanently `null`**
The `Promise.all` is prefixed with `void`, which silences the unhandled-rejection warning but means a tRPC error (e.g., IPC hiccup on startup) causes both `hasAny` calls to throw, the `.then` callback never runs, and `optInV2` stays `null` forever for that session. Because `useIsV2CloudEnabled` treats `null` as `false`, a fresh-install user would see v1 indefinitely and never get the intended v2 default — even after the IPC channel recovers — until they manually restart or toggle.
How can I resolve this? If you propose a fix, please make it concise.The v2 default introduced in #4115 used `localStorage.getItem("tabs-storage")` to detect returning users, but tabs state was migrated off localStorage to app-state.json (lowdb) in #303 (2025-12-10). The key has been null for every install since, so `!hasPriorSupersetUsage()` evaluated to true for all users and force-flipped anyone without an explicit toggle into v2 on first launch. Resolve the default from the canonical signal instead: ≥1 row in `projects` or `workspaces` in localDb means a returning user. `optInV2` starts `null`, `V2DefaultResolver` runs both `*.hasAny` queries once and persists the result. Users who got force-pulled but never toggled have no persisted override yet, so they self-heal back to v1 on next launch.
The onboarding gate at _authenticated/layout.tsx only checked isV2CloudEnabled && !requiredComplete, so any existing v1 user who explicitly toggled v2 ON was force-walked through /setup/* — wrong: they already have projects/workspaces and should land on the dashboard. Split the v2 default into two flags on the override store: - optInV2: user-controllable (resolver default OR experimental toggle). - isFreshInstall: sticky one-shot detection from the resolver, never changed by user toggles. Onboarding gate now requires isFreshInstall === true. Resolver runs whenever either flag is null, so users with persisted optInV2 from the broken release also get isFreshInstall backfilled on next launch.
1f10963 to
aaf74f1
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…opt-ins V2DefaultResolver wrote optInV2=true to v2-local-override-v2 for every fresh-detected install in the ~24h window since #4176 shipped. Those writes are indistinguishable from explicit user toggles in the store, so without bumping the key those users would silently stay on v2 even though they never made a real choice. Bumping to v2-local-override-v3 clears the field for everyone: - existing v1 users: no change (their key was already null/absent) - recently auto-flipped users: land back on v1, can opt in if they want - explicit opt-ins: have to toggle once more Acceptable trade-off vs leaving a small group force-stuck on v2.
* fix(desktop): stop auto-opting users into v2 + onboarding Two recurring complaints on top of each other: fresh installs were being flipped to v2 without explicit opt-in, and v2 users were then force-walked through the experimental onboarding flow on first launch. v2 + onboarding aren't ready to be defaults — only users who explicitly opt in via Settings → Experimental should land there. - Delete V2DefaultResolver. Its only job was to auto-set optInV2=true on detected-fresh installs. With it gone, optInV2 stays null until the user toggles it, and useIsV2CloudEnabled returns false. - Remove the /setup/* redirect in _authenticated/layout.tsx so fresh v2 users land on the dashboard like v1 users. - Drop isFreshInstall from the store + the useIsFreshInstall hook (only consumer was the redirect gate). - Drop the projects.hasAny / workspaces.hasAny tRPC procedures (only consumer was V2DefaultResolver). Preserved: the optInV2 toggle and "Restart onboarding" action in Settings → Experimental, the full /setup/* route tree + onboarding store, and any persisted optInV2=true from explicit toggles. * fix(desktop): bump v2-local-override persist key to clear stale auto-opt-ins V2DefaultResolver wrote optInV2=true to v2-local-override-v2 for every fresh-detected install in the ~24h window since #4176 shipped. Those writes are indistinguishable from explicit user toggles in the store, so without bumping the key those users would silently stay on v2 even though they never made a real choice. Bumping to v2-local-override-v3 clears the field for everyone: - existing v1 users: no change (their key was already null/absent) - recently auto-flipped users: land back on v1, can opt in if they want - explicit opt-ins: have to toggle once more Acceptable trade-off vs leaving a small group force-stuck on v2. * Revert "fix(desktop): bump v2-local-override persist key to clear stale auto-opt-ins" This reverts 3ca8703. The bump also clears optInV2 for users who explicitly toggled v2 on and went through onboarding — they'd silently land back on v1 with no projects visible (v2 workspaces live in cloud, not v1 local), looking broken. Worse trade-off than leaving the small ~24h auto-flip cohort stuck on v2.
superset-sh#4176) * fix(desktop): resolve v2 default from local db, not stale localStorage The v2 default introduced in superset-sh#4115 used `localStorage.getItem("tabs-storage")` to detect returning users, but tabs state was migrated off localStorage to app-state.json (lowdb) in #303 (2025-12-10). The key has been null for every install since, so `!hasPriorSupersetUsage()` evaluated to true for all users and force-flipped anyone without an explicit toggle into v2 on first launch. Resolve the default from the canonical signal instead: ≥1 row in `projects` or `workspaces` in localDb means a returning user. `optInV2` starts `null`, `V2DefaultResolver` runs both `*.hasAny` queries once and persists the result. Users who got force-pulled but never toggled have no persisted override yet, so they self-heal back to v1 on next launch. * fix(desktop): only fresh installs are forced into onboarding The onboarding gate at _authenticated/layout.tsx only checked isV2CloudEnabled && !requiredComplete, so any existing v1 user who explicitly toggled v2 ON was force-walked through /setup/* — wrong: they already have projects/workspaces and should land on the dashboard. Split the v2 default into two flags on the override store: - optInV2: user-controllable (resolver default OR experimental toggle). - isFreshInstall: sticky one-shot detection from the resolver, never changed by user toggles. Onboarding gate now requires isFreshInstall === true. Resolver runs whenever either flag is null, so users with persisted optInV2 from the broken release also get isFreshInstall backfilled on next launch.
…h#4177) * fix(desktop): stop auto-opting users into v2 + onboarding Two recurring complaints on top of each other: fresh installs were being flipped to v2 without explicit opt-in, and v2 users were then force-walked through the experimental onboarding flow on first launch. v2 + onboarding aren't ready to be defaults — only users who explicitly opt in via Settings → Experimental should land there. - Delete V2DefaultResolver. Its only job was to auto-set optInV2=true on detected-fresh installs. With it gone, optInV2 stays null until the user toggles it, and useIsV2CloudEnabled returns false. - Remove the /setup/* redirect in _authenticated/layout.tsx so fresh v2 users land on the dashboard like v1 users. - Drop isFreshInstall from the store + the useIsFreshInstall hook (only consumer was the redirect gate). - Drop the projects.hasAny / workspaces.hasAny tRPC procedures (only consumer was V2DefaultResolver). Preserved: the optInV2 toggle and "Restart onboarding" action in Settings → Experimental, the full /setup/* route tree + onboarding store, and any persisted optInV2=true from explicit toggles. * fix(desktop): bump v2-local-override persist key to clear stale auto-opt-ins V2DefaultResolver wrote optInV2=true to v2-local-override-v2 for every fresh-detected install in the ~24h window since superset-sh#4176 shipped. Those writes are indistinguishable from explicit user toggles in the store, so without bumping the key those users would silently stay on v2 even though they never made a real choice. Bumping to v2-local-override-v3 clears the field for everyone: - existing v1 users: no change (their key was already null/absent) - recently auto-flipped users: land back on v1, can opt in if they want - explicit opt-ins: have to toggle once more Acceptable trade-off vs leaving a small group force-stuck on v2. * Revert "fix(desktop): bump v2-local-override persist key to clear stale auto-opt-ins" This reverts 3ca8703. The bump also clears optInV2 for users who explicitly toggled v2 on and went through onboarding — they'd silently land back on v1 with no projects visible (v2 workspaces live in cloud, not v1 local), looking broken. Worse trade-off than leaving the small ~24h auto-flip cohort stuck on v2.
Summary
Two related bugs:
1. v2 default signal was always wrong. #4115 used
localStorage.getItem("tabs-storage")to detect returning users, but tabs state moved off localStorage toapp-state.json(lowdb) back in #303 (2025-12-10). The key has beennullfor every install since, so!hasPriorSupersetUsage()wastruefor everyone and force-flipped anyone without an explicit toggle into v2.2. Existing v1 users who opted into v2 got force-walked through onboarding. The gate in
_authenticated/layout.tsxonly requiredisV2CloudEnabled && !requiredComplete, with no "is this a fresh user?" check — so any existing user who toggled v2 ON in Experimental Settings was redirected to/setup/providers.Fix
workspaces.hasAnyandprojects.hasAnyprocedures (cheapLIMIT 1reads onlocalDb/host.db).v2-local-overridestore now has two flags, bothboolean | null:optInV2— user-controllable (resolver default or Experimental toggle).isFreshInstall— sticky one-shot detection set by the resolver; never changes from a user toggle.V2DefaultResolver(mounted inRootLayout) runs whenever either flag is null, queries the twohasAnyprocedures once, and fills in whichever flags are still null with!hasWorkspace && !hasProject. Users who got force-pulled by the broken release but never toggled have no persistedoptInV2, so they self-heal back to v1.isV2CloudEnabled && isFreshInstall === true && !requiredComplete && !isOnSetupRoute— so existing users opting into v2 land on the dashboard, not setup.useIsV2CloudEnabledreturnsoptInV2 === true(null → v1, no flicker before resolution).hasPriorSupersetUsage.ts.Test plan
optInV2→ back to v1.isFreshInstall=false; if they hadn't completed onboarding they stop being force-walked through it.isFreshInstallis sticky).Summary by CodeRabbit
Release Notes
New Features
Improvements
Removed