fix(onboarding): restore page refresh on Go to Dashboard#1967
fix(onboarding): restore page refresh on Go to Dashboard#1967
Conversation
- The onboarding refactor in afe1ae6 (#1949) introduced session-aware close behavior that distinguishes "manual" vs "automatic" onboarding - For manual sessions, closeModal() short-circuited with history.go() and the NEXT_STEPS onComplete passed `reload: !isManualSession.value`, meaning manual sessions never triggered a page refresh - This broke the "Go to Dashboard" flow: clicking it should always refresh so the dashboard loads with the newly configured state - Fix: always pass `reload: true` for NEXT_STEPS completion, and prioritize the reload path in closeModal() so it executes before the history navigation short-circuit
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OnboardingModal as OnboardingModal.vue
participant Store as onboardingModalStore
participant Storage as OnboardingStorage
participant Browser as window.location
User->>OnboardingModal: complete onboarding / trigger close
OnboardingModal->>Store: onboardingModalStore.closeModal()
OnboardingModal->>Storage: cleanupOnboardingStorage() / clearHistorySession()
OnboardingModal->>Browser: reload() (window.location.reload)
Browser-->>User: page reload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- The previous test asserted that manual sessions used history.go() instead of reloading when completing the NEXT_STEPS step - Updated to verify that closeModal is called and window.location.reload fires, matching the fixed behavior where "Go to Dashboard" always triggers a page refresh regardless of session type
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1967 +/- ##
==========================================
- Coverage 52.12% 52.09% -0.04%
==========================================
Files 1031 1031
Lines 71589 71573 -16
Branches 8102 8093 -9
==========================================
- Hits 37319 37284 -35
- Misses 34145 34164 +19
Partials 125 125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ivationSkip - Remove conditional `!isManualSession` reload gating from all completion paths — onboarding changes server state, so a refresh is always needed regardless of how the modal was opened - goToNextStep now always passes `reload: true` when closing at the last step, and when no steps are available - Simplify handleActivationSkip to delegate to goToNextStep since it was a redundant reimplementation of the same advance-or-close logic and ACTIVATE_LICENSE is never the last step anyway - The only non-reload close paths remaining are the X button exit and browser back navigation, which are cancel/abandon actions
…nd back nav - handleExitConfirm (X button) was closing the modal without reloading, meaning any state changes made during onboarding would not be reflected - handlePopstate (browser back navigation) had the same issue, directly calling the store closeModal instead of the local closeModal with reload - All close paths now consistently pass reload: true so the page always refreshes when exiting onboarding, regardless of how it was dismissed
- Since all callers now always want a reload, remove the options
parameter and the conditional branching from closeModal
- Remove unreachable history.go() fallback and duplicate cleanup block
- Drop { reload: true } from all call sites since it's now the only
behavior
- Replace handleActivationSkip with direct goToNextStep reference in the ACTIVATE_LICENSE onComplete handler since it was just a pass-through wrapper with no additional logic
…TE_LICENSE - baseProps already defines onComplete as goToNextStep, so the explicit override in the ACTIVATE_LICENSE case was unnecessary
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee245d4020
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (options?.reload) { | ||
| window.location.reload(); | ||
| } | ||
| window.location.reload(); |
There was a problem hiding this comment.
closeModal now always calls window.location.reload(), and both non-completion paths (handleExitConfirm from the X/Exit flow and handlePopstate when backing out of a manual session) call this helper. That means abandoning onboarding mid-flow now refreshes the whole app instead of just closing the modal, which can drop unrelated in-page state and is inconsistent with the expected cancel behavior; reload should only happen on completion paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/Onboarding/OnboardingModal.vue`:
- Around line 306-310: The current closeModal function awaits
onboardingModalStore.closeModal() which can reject, causing
cleanupOnboardingStorage(), clearHistorySession(), and window.location.reload()
to be skipped; wrap the cleanup and reload steps in a finally block so they
always run regardless of onboardingModalStore.closeModal() success or failure
(i.e., call onboardingModalStore.closeModal() in try/catch if you want to log
errors, but invoke cleanupOnboardingStorage(), clearHistorySession(), and
window.location.reload() from a finally block) — reference the closeModal
wrapper, onboardingModalStore.closeModal(), cleanupOnboardingStorage(),
clearHistorySession(), and window.location.reload().
🪄 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: a9e1f83d-ce83-4abe-b5c2-bb681671cc3b
📒 Files selected for processing (1)
web/src/components/Onboarding/OnboardingModal.vue
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Onboarding/OnboardingModal.vue (1)
438-443:⚠️ Potential issue | 🔴 Critical
isProgrammaticHistoryExitis not defined — build failure.The variable
isProgrammaticHistoryExitis used on lines 439 and 443 but was never declared. This was likely left behind when the history navigation logic was removed. The pipeline confirms the ESLint error.🐛 Proposed fix: remove orphaned references
const handlePopstate = async (event: PopStateEvent) => { - if (isInternalBootLocked.value && !isProgrammaticHistoryExit.value) { + if (isInternalBootLocked.value) { window.history.forward(); return; } - isProgrammaticHistoryExit.value = false; const nextHistoryState = getHistoryState(event.state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/OnboardingModal.vue` around lines 438 - 443, The code references an undefined variable isProgrammaticHistoryExit inside the handlePopstate handler; remove the orphaned references instead of declaring an unused flag. Edit the handlePopstate function (and any other places referencing isProgrammaticHistoryExit) to drop the conditional checks and the assignment to isProgrammaticHistoryExit.value, keeping the existing isInternalBootLocked check and history.forward() behavior intact so no undefined identifier remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web/src/components/Onboarding/OnboardingModal.vue`:
- Around line 438-443: The code references an undefined variable
isProgrammaticHistoryExit inside the handlePopstate handler; remove the orphaned
references instead of declaring an unused flag. Edit the handlePopstate function
(and any other places referencing isProgrammaticHistoryExit) to drop the
conditional checks and the assignment to isProgrammaticHistoryExit.value,
keeping the existing isInternalBootLocked check and history.forward() behavior
intact so no undefined identifier remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7be4eec4-d88f-4bbb-85c7-901b64adfb54
📒 Files selected for processing (2)
web/__test__/components/Onboarding/OnboardingModal.test.tsweb/src/components/Onboarding/OnboardingModal.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- web/test/components/Onboarding/OnboardingModal.test.ts
- If onboardingModalStore.closeModal() rejects (e.g. transient network error during refetchOnboarding), cleanup and reload were skipped, leaving the user stuck on stale onboarding state - Wrap in try/finally so cleanupOnboardingStorage, clearHistorySession, and window.location.reload always execute regardless of store errors
- isProgrammaticHistoryExit was referenced in handlePopstate but never declared, causing a build error - This flag was originally meant to allow programmatic history.go() calls to bypass the internal boot lock, but closeModal no longer uses history navigation — it always reloads - Simplified the guard to just check isInternalBootLocked
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
2 participants
Summary
afe1ae606) where exiting onboarding no longer triggers a page refreshmanualvsautomatic), but this inadvertently skipped the reload for manual onboarding sessions across all close pathscloseModaland removes dead code left over from the conditional reload logicProblem
The onboarding refactor introduced
!isManualSessiongating on thereloadflag in multiple close paths:NEXT_STEPSonComplete ("Go to Dashboard") — passedreload: !isManualSession.value, so manual sessions got no reloadgoToNextStep(reaching the last step) — same conditional reloadhandleActivationSkip— redundant reimplementation ofgoToNextStepwith the same conditional reloadcloseModal()itself — short-circuited for manual sessions viawindow.history.go(), returning before ever reachingwindow.location.reload()handleExitConfirm(X button) andhandlePopstate(browser back) — closed without any reloadSince onboarding changes server state (settings, activation, etc.), a refresh is always needed when exiting, regardless of how the modal was opened or dismissed.
Solution
closeModal()always reloads — removed theoptionsparameter, conditional branching, and unreachablehistory.go()fallback. It now unconditionally cleans up and reloads in 4 lines.closeModal()—handleExitConfirm(X button),handlePopstate(browser back),goToNextStep, andNEXT_STEPSonComplete all go through the same reload path.handleActivationSkip— it was a redundant wrapper aroundgoToNextStep. TheACTIVATE_LICENSEcase now relies onbaseProps.onCompletewhich already callsgoToNextStep.onCompleteoverride onACTIVATE_LICENSE—basePropsalready definesonCompleteasgoToNextStep.Test plan
Summary by CodeRabbit
Bug Fixes
Tests