fix(wizard): harden onboarding wizard skip with 6-layer defense#2895
fix(wizard): harden onboarding wizard skip with 6-layer defense#2895
Conversation
Make the wizard dismiss endpoint always public so it works pre-auth, auto-seed last_seen_version for existing installs that predate wizard tracking, detect existing installs via multiple signals (location, audio sources, security config, detections, notes), close the modal immediately before firing the dismiss API call, retry the dismiss POST once on failure, and enable the close button as an escape hatch. Closes #2892
Update determineWizardState docstring to reflect new isExistingInstall logic. Add dismiss retry to dismissOnly() for consistency with dismiss(), completing the multi-site fix.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughModal now shows a visible close button; skip/complete immediately reset frontend wizard state and fire-and-forget a dismiss POST that retries once after 2s on failure; backend exposes Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Browser UI\n(WizardDialog)"
participant WS as "Frontend\nWizardState"
participant API as "Backend API\nPOST /app/wizard/dismiss"
participant DB as "Database / app_metadata"
UI->>WS: user clicks Skip/Done/Close
WS->>WS: reset local wizard state (flow/currentStep/completed/isActive)
WS->>API: POST /app/wizard/dismiss (fire-and-forget)
API->>DB: write dismissal / update last_seen_version (may auto-seed)
alt DB write succeeds
DB-->>API: 200 OK
API-->>WS: 200 OK
else DB write fails
DB-->>API: error
API-->>API: log warning / return error
WS-->>API: retry POST after 2s
API->>DB: retry write
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 enables the close button in the wizard dialog and updates the frontend state management to include a retry mechanism for dismissal and non-blocking calls. On the backend, the wizard dismissal endpoint is now public, and the logic for determining the wizard state has been enhanced to detect existing installations by checking various configuration signals, auto-seeding the version if necessary. Feedback suggests centralizing table name resolution in the backend and extracting duplicated retry logic in the frontend to improve maintainability and reduce code duplication.
| tableName := "notes" | ||
| if tp, ok := c.V2Manager.(interface{ TablePrefix() string }); ok && tp.TablePrefix() != "" { | ||
| tableName = tp.TablePrefix() + tableName | ||
| } |
There was a problem hiding this comment.
The logic for resolving the table name with an optional prefix is duplicated here and in hasZeroDetections. Centralize this parameter resolution before branching logic to ensure consistency and avoid code duplication across different paths.
References
- Centralize parameter normalization or resolution before branching logic to ensure consistency and avoid code duplication across different paths.
There was a problem hiding this comment.
Acknowledged. The table name + prefix pattern is 3 lines and appears in exactly 2 places. Extracting a helper for 2 callers in a focused bug fix would add indirection without meaningful DRY benefit. Will revisit if more callers appear.
| setTimeout(() => { | ||
| api.post(WIZARD_DISMISS_ENDPOINT).catch(() => {}); | ||
| }, 2000); |
There was a problem hiding this comment.
The retry logic for the dismiss API call is duplicated between dismiss and dismissOnly. To avoid code duplication and improve maintainability, extract this shared logic and associated state management into a reusable composable function.
References
- To avoid code duplication and improve maintainability, extract shared logic, such as action handlers and associated state management for modals and API calls, into a reusable composable function.
There was a problem hiding this comment.
Acknowledged. The two functions have different signatures (dismiss captures currentVersion from module state, dismissOnly takes an explicit version parameter). Extracting a shared retry wrapper would need to account for these differences. Keeping them inline is clearer for 2 callers.
![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 (1)
internal/api/v2/app.go (1)
296-300: Consider logging errors inhasNotesfor debugging consistency.
hasZeroDetectionslogs a warning when the database query fails (Line 256), buthasNotessilently swallows errors. While returningfalseon error is the safe default (avoids incorrectly showing the wizard), logging the error would maintain consistency and aid debugging.♻️ Suggested improvement
err := c.V2Manager.DB().WithContext(ctx).Table(tableName). Select("1").Limit(1).Scan(&exists).Error if err != nil { + c.logWarnIfEnabled("Failed to check notes for wizard state", logger.Error(err)) return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/v2/app.go` around lines 296 - 300, The hasNotes function currently swallows DB errors; update it to log the error like hasZeroDetections does: when the call to c.V2Manager.DB().WithContext(ctx).Table(tableName).Select("1").Limit(1).Scan(&exists).Error returns a non-nil err, call the same logger used in hasZeroDetections (e.g., c.Logger or the component logger) to emit a warning or error with context (include tableName and err) and then return false; follow the same logging message pattern used in hasZeroDetections to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/lib/desktop/features/wizard/wizardState.svelte.ts`:
- Around line 113-121: The skip() and complete() functions currently return void
and call dismiss() fire-and-forget, so tests that await
wizardState.skip()/complete() won't wait for the async dismiss() to finish;
change both functions to async and await the dismiss() call (e.g., make function
skip(): Promise<void> and await dismiss(); then call resetState()) so awaiting
them in tests properly waits for the API/mock to be invoked; keep resetState()
after the awaited dismiss() to preserve behavior.
---
Nitpick comments:
In `@internal/api/v2/app.go`:
- Around line 296-300: The hasNotes function currently swallows DB errors;
update it to log the error like hasZeroDetections does: when the call to
c.V2Manager.DB().WithContext(ctx).Table(tableName).Select("1").Limit(1).Scan(&exists).Error
returns a non-nil err, call the same logger used in hasZeroDetections (e.g.,
c.Logger or the component logger) to emit a warning or error with context
(include tableName and err) and then return false; follow the same logging
message pattern used in hasZeroDetections to keep consistency.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87b92a9f-da37-47bf-9170-38640a96c394
📒 Files selected for processing (3)
frontend/src/lib/desktop/features/wizard/WizardDialog.sveltefrontend/src/lib/desktop/features/wizard/wizardState.svelte.tsinternal/api/v2/app.go
Add error logging to hasNotes() for consistency with hasZeroDetections(). Remove stale await from tests after skip()/complete() became sync.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the onboarding wizard by making the dismissal endpoint public and refining the logic to detect existing installations. On the frontend, the wizard dialog now includes a close button, and the state transitions for skipping or completing the wizard are now synchronous with a retry mechanism for the dismissal API call. A review comment suggests using context.Background() for the database write during the auto-seeding process in the GetAppConfig handler to ensure the operation completes even if the client disconnects.
| if c.isExistingInstall(ctx) { | ||
| // Auto-seed: existing install predates wizard tracking. | ||
| // Intentional write inside GET handler (idempotent upsert, fires once per install). | ||
| if err := c.appMetadataRepo.Set(ctx, appMetadataKeyLastSeenVersion, c.Settings.Version); err != nil { | ||
| c.logWarnIfEnabled("Failed to auto-seed last_seen_version", logger.Error(err)) | ||
| } | ||
| return false, false, c.Settings.Version | ||
| } |
There was a problem hiding this comment.
Performing a database write (appMetadataRepo.Set) within a GET handler (GetAppConfig) violates the principle of idempotency. Specifically, using the request context (ctx) means the write will be canceled if the client disconnects. Since this is a background migration task, consider using context.Background() to ensure the write completes even if the connection is dropped. This aligns with the repository's approach to incremental refactoring where a lifecycle-bound context is not yet available.
References
- When performing incremental refactoring, a context.Context parameter can be temporarily introduced with context.Background(). The full implementation with a lifecycle-bound context should follow in a subsequent change and the intent should be documented.
There was a problem hiding this comment.
Keeping request context. The upsert is a single-row operation (sub-millisecond), and if it fails due to context cancellation, the next request retries it (idempotent). Using context.Background() inside a request handler goes against Go conventions and would mask legitimate context propagation. The practical risk of client disconnect during a sub-ms upsert is negligible.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
| if !c.hasZeroDetections(ctx) { | ||
| return true |
There was a problem hiding this comment.
Bug: The logic in determineWizardState was changed to skip the "what's new" wizard for existing installs, but the corresponding test case was not updated, which will cause a CI failure.
Severity: MEDIUM
Suggested Fix
Update the test case TestDetermineWizardState/upgrade from unknown — no last_seen_version, has detections in app_wizard_test.go. Change the expectation for wantNew from true to false to align with the new, intended behavior of the determineWizardState function, which now skips the wizard for existing installations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: internal/api/v2/app.go#L274-L276
Potential issue: The function `determineWizardState` was modified to skip the "what's
new" wizard for existing installations where `lastSeenVersion` is empty. This is
achieved by a check in `isExistingInstall()` which now returns `true` if there are any
detections. However, the test case `TestDetermineWizardState/upgrade from unknown — no
last_seen_version, has detections` in `app_wizard_test.go` was not updated to reflect
this intentional change. It still asserts that `wantNew` should be `true`, but the new
logic correctly returns `false`. This mismatch will cause the test to fail during CI,
blocking the pull request from being merged.
Also affects:
internal/api/v2/app.go:92~102
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Good catch. Fixed in c34fbca - renamed test case and updated expectations to match the new auto-seed behavior.
Rename "upgrade from unknown" test case to "existing install auto-seeds" and update expectations: with detections present, isExistingInstall now returns true, triggering auto-seed instead of showing the what's-new wizard.
| if err != nil { | ||
| c.logWarnIfEnabled("Failed to check notes for wizard state", logger.Error(err)) | ||
| return false | ||
| } | ||
| return exists != 0 |
There was a problem hiding this comment.
Bug: The hasNotes() function queries a non-existent notes table in the v2 database, causing the check to always fail and return false.
Severity: LOW
Suggested Fix
Remove the hasNotes() check from isExistingInstall() as it queries a table that no longer exists in the v2 schema. The check is dead code that generates log noise and is not a reliable signal for detecting an existing installation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: internal/api/v2/app.go#L298-L302
Potential issue: The function `hasNotes()` in `internal/api/v2/app.go` attempts to query
a table named `notes` in the v2 database to determine if the installation is new.
However, the v2 database schema does not contain a `notes` table; this table was part of
the legacy schema and its data is now in the `detections` table. As a result, the query
always fails, causing `hasNotes()` to always return `false` and log a warning. This
renders one of the five checks in `isExistingInstall()` non-functional, potentially
causing the setup wizard to incorrectly appear on existing installations under specific
edge-case conditions.
There was a problem hiding this comment.
The notes table is the legacy V1 table that exists in SQLite databases being migrated to V2 (see startup.go:301). For upgrading users, it contains their detection data. For fresh installs, it doesn't exist, the query errors silently (caught), and returns false - which is the correct behavior. This signal catches exactly the users it's designed for.
1 participant
Summary
last_seen_versionfor existing installs that predate wizard tracking, using 5 detection signals: location configured, audio sources present, security enabled, detections exist, notes existContext
Users upgrading from older versions hit a failure chain where: the V2 detections table is empty (data in legacy tables), the dismiss endpoint requires auth but the wizard shows pre-auth, and
skip()/complete()block on the failed API call before closing the modal. PR #2889 mitigated the localStorage side but the underlying server-side and UI-blocking failures remained.This fix adds defense in depth so no single failure can trap the user in the wizard.
Test plan
Fixes #2892
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests