fix(onboarding): auto-open incomplete onboarding on 7.3+#1940
Merged
Ajit-Mehrotra merged 2 commits intomainfrom Mar 20, 2026
Merged
fix(onboarding): auto-open incomplete onboarding on 7.3+#1940Ajit-Mehrotra merged 2 commits intomainfrom
Ajit-Mehrotra merged 2 commits intomainfrom
Conversation
- Purpose: show the onboarding modal once for any incomplete user on supported versions, including licensed users upgrading from 7.2.x to 7.3.x. - Before: auto-open and close-to-complete behavior only applied when registration state looked like a fresh install (ENOKEYFILE). - Problem: licensed users who had never completed onboarding stayed gated out of the modal after upgrading to 7.3.x. - Change: remove the fresh-install requirement from the backend auto-open and close lifecycle predicates so incomplete 7.3+ users are treated consistently. - Coverage: add service specs for licensed incomplete users and update the onboarding docs to match the actual behavior.
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAuto-open logic now triggers for any incomplete onboarding on supported versions; fresh-install checks no longer gate auto-open or completion marking. Tests were adjusted and extended for licensed-user scenarios, and documentation updated to reflect the new auto-open rules. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1940 +/- ##
==========================================
- Coverage 51.52% 51.51% -0.01%
==========================================
Files 1025 1025
Lines 70467 70460 -7
Branches 7791 7790 -1
==========================================
- Hits 36307 36297 -10
- Misses 34037 34040 +3
Partials 123 123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/components/Onboarding/UPGRADE_ONBOARDING.md (1)
5-5: Consider tightening ownership wording for visibility decisions.Line 5 can read as if visibility is fully client-owned, while later notes correctly emphasize backend
shouldOpen. Consider clarifying that backend computesshouldOpen, and the client applies final UI guards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/UPGRADE_ONBOARDING.md` at line 5, The wording implies the client fully decides modal visibility; update the copy to state that the backend computes the recommended visibility flag (`shouldOpen`) based on onboarding API fields (`completed`, `completedAtVersion`) and the web client applies final UI guards (version checks, local UX rules) before showing the modal; explicitly mention `shouldOpen` as backend-owned and the client as applying presentation-level safeguards to avoid ambiguity.api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts (1)
587-603: Make the “licensed” close-path test explicitly licensed.The test title says “licensed” but setup only changes
isFreshInstall. Consider setting registration mocks toPRO/registered as you did in Line 420-448, so the intent is unambiguous.Suggested test-hardening tweak
onboardingTrackerMock.getCurrentVersion.mockReturnValue('7.3.0'); + onboardingStateMock.getRegistrationState.mockReturnValue('PRO'); + onboardingStateMock.isRegistered.mockReturnValue(true); + onboardingStateMock.hasActivationCode.mockResolvedValue(false); + onboardingStateMock.requiresActivationStep.mockReturnValue(false); onboardingStateMock.isFreshInstall.mockReturnValue(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts` around lines 587 - 603, The test "marks licensed incomplete onboarding complete when closed on supported versions" claims to exercise the licensed path but doesn't set registration state; add the registration mock setup to explicitly mark the instance as licensed/PRO (as done earlier in the suite) so the intent is unambiguous. In this test, after configuring onboardingTrackerMock and onboardingStateMock, set the registration mock (e.g., registrationServiceMock or registrationMock used elsewhere in the file) to return a PRO/registered result so the code path for licensed installs is exercised before calling service.closeOnboarding(); keep existing assertions on onboardingTrackerMock.markCompleted and setForceOpen unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts`:
- Around line 413-415: Remove the unnecessary await call to getOnboardingState()
in closeOnboarding(): the local variable onboardingState (from
getOnboardingState()) is never used, causing extra async work and potential
failures; delete the line "const onboardingState = await
this.getOnboardingState();" and ensure the closeOnboarding() logic continues to
use the existing state/currentVersion/isVersionSupported() checks (e.g., the
existing "state" and "shouldAutoOpen" computation) without referencing
onboardingState anywhere else.
---
Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts`:
- Around line 587-603: The test "marks licensed incomplete onboarding complete
when closed on supported versions" claims to exercise the licensed path but
doesn't set registration state; add the registration mock setup to explicitly
mark the instance as licensed/PRO (as done earlier in the suite) so the intent
is unambiguous. In this test, after configuring onboardingTrackerMock and
onboardingStateMock, set the registration mock (e.g., registrationServiceMock or
registrationMock used elsewhere in the file) to return a PRO/registered result
so the code path for licensed installs is exercised before calling
service.closeOnboarding(); keep existing assertions on
onboardingTrackerMock.markCompleted and setForceOpen unchanged.
In `@web/src/components/Onboarding/UPGRADE_ONBOARDING.md`:
- Line 5: The wording implies the client fully decides modal visibility; update
the copy to state that the backend computes the recommended visibility flag
(`shouldOpen`) based on onboarding API fields (`completed`,
`completedAtVersion`) and the web client applies final UI guards (version
checks, local UX rules) before showing the modal; explicitly mention
`shouldOpen` as backend-owned and the client as applying presentation-level
safeguards to avoid ambiguity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4dfd6c05-c5b6-4536-991d-97e58cc51479
📒 Files selected for processing (3)
api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.tsapi/src/unraid-api/graph/resolvers/customization/onboarding.service.tsweb/src/components/Onboarding/UPGRADE_ONBOARDING.md
api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts
Outdated
Show resolved
Hide resolved
- Purpose: resolve the PR review items on the onboarding gating follow-up. - Before: closeOnboarding() still performed an unused onboarding-state fetch, the licensed close-path spec did not explicitly mock a licensed state, and the upgrade onboarding doc blurred backend vs client ownership of modal visibility. - Problem: the unused fetch added avoidable async work in the close path, the test intent was less explicit than its title, and the docs could mislead readers about how shouldOpen is computed. - Change: removed the unused async fetch, made the licensed spec explicitly registered/PRO, and clarified that the API computes shouldOpen while the web layer applies final UI guards. - Validation: pnpm --filter ./api exec vitest run src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts && pnpm lint
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
pushed a commit
that referenced
this pull request
Mar 23, 2026
🤖 I have created a release *beep* *boop* --- ## [4.31.0](v4.30.1...v4.31.0) (2026-03-23) ### Features * **api:** support encrypted array start inputs ([#1944](#1944)) ([018a8d5](018a8d5)) * **onboarding:** add shared loading states ([#1945](#1945)) ([776c8cc](776c8cc)) * Serverside state for onboarding display ([#1936](#1936)) ([682d51c](682d51c)) ### Bug Fixes * **api:** reconcile emhttp state without spinning disks ([#1946](#1946)) ([d3e0b95](d3e0b95)) * **onboarding:** auto-open incomplete onboarding on 7.3+ ([#1940](#1940)) ([f0241a8](f0241a8)) * **onboarding:** replace internal boot native selects ([#1942](#1942)) ([d6ea032](d6ea032)) * preserve onboarding resume state on reload ([#1941](#1941)) ([91f7fe9](91f7fe9)) * recover VM availability after reconnect ([#1947](#1947)) ([e064de7](e064de7)) * Unify callback server payloads ([#1938](#1938)) ([f58fcc0](f58fcc0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
7.3.0+)Why
Licensed users upgrading from
7.2.xto7.3.xcould remain incomplete but never see the onboarding modal because auto-open was limited to fresh-install (ENOKEYFILE) states.Validation
pnpm lintpnpm testpnpm type-checkSummary by CodeRabbit