Fix progress dots mapping, persist variant, clamp restored step#1363
Conversation
- Add totalSteps parameter to OnboardingProgressDots (default 7) and compute step-to-dot mapping dynamically instead of hardcoded switch. Pass totalSteps: 5 from FirstMeetingFlowView so all 5 dots are used across 5 steps. - Persist CLI-selected onboarding variant to UserDefaults immediately in OnboardingWindow.show() so it survives app restarts. - Clamp restored step to variant-specific max (4 for firstMeeting, 6 for default) in OnboardingState.init() to prevent out-of-range rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8b72737a
ℹ️ 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".
| state.onboardingVariant = .firstMeeting | ||
| UserDefaults.standard.set(OnboardingVariant.firstMeeting.rawValue, forKey: "onboarding.variant") |
There was a problem hiding this comment.
Re-clamp restored step after CLI variant override
When --onboarding-variant first_meeting is passed, show() switches state.onboardingVariant after OnboardingState.init() has already clamped currentStep using the previously persisted variant. If a user has onboarding.step saved as 5 or 6 from the default flow, this path leaves currentStep out of range for first-meeting (0...4), and FirstMeetingFlowView falls into its default case (EmptyView) instead of rendering a valid step. This makes the new clamp ineffective for the CLI-switch scenario the commit is targeting.
Useful? React with 👍 / 👎.
| guard totalSteps > 1 else { return 0 } | ||
| let fraction = Double(currentStep) / Double(totalSteps - 1) | ||
| return min(Int(fraction * Double(totalDots - 1) + 0.5), totalDots - 1) |
There was a problem hiding this comment.
🟡 Default flow progress dots regression: step 5 no longer lights the last dot
The new generic formula for activeDotIndex changes the default flow's step-to-dot mapping in a way that breaks the previous behavior.
Detailed mapping comparison and impact
The old hand-coded mapping was:
- Steps 0,1 → dot 0
- Step 2 → dot 1
- Step 3 → dot 2
- Step 4 → dot 3
- Steps 5,6 (default) → dot 4
The new formula Int(fraction * Double(totalDots - 1) + 0.5) with totalSteps=7 produces:
- Step 0 → dot 0
- Steps 1,2 → dot 1
- Step 3 → dot 2
- Steps 4,5 → dot 3
- Step 6 → dot 4
The critical difference is step 5 (ScreenPermissionStepView): it previously mapped to dot 4 (all 5 dots lit), but now maps to dot 3 (only 4 dots lit). This means users on the Screen Permission step see only 4/5 dots filled instead of 5/5, which is a visual regression.
Additionally, step 1 (NamingStepView) now advances the dot from 0 to 1, whereas before steps 0 and 1 shared dot 0. This changes the perceived progress pacing for the default onboarding flow.
Was this helpful? React with 👍 or 👎 to provide feedback.
| state.onboardingVariant = .firstMeeting | ||
| UserDefaults.standard.set(OnboardingVariant.firstMeeting.rawValue, forKey: "onboarding.variant") |
There was a problem hiding this comment.
🔴 Step clamping in init() runs before CLI variant override, leaving step out of range
When launching with --onboarding-variant first_meeting after a previous default-flow session saved a step > 4, the step is not properly clamped because the variant override happens after init() has already clamped.
Root cause and reproduction
OnboardingState is initialized at OnboardingWindow declaration time (clients/macos/vellum-assistant/Features/Onboarding/OnboardingWindow.swift:7). The init() at OnboardingState.swift:69-93 reads the variant from UserDefaults and clamps the step accordingly. However, in OnboardingWindow.show() at OnboardingWindow.swift:20-25, the variant is overridden from the command line argument after init() has already completed.
Reproduction:
- Run default onboarding, reach step 5, kill the app (saves
onboarding.step=5,onboarding.variant=default) - Relaunch with
--onboarding-variant first_meeting OnboardingState.init()reads variant as.defaultfrom UserDefaults → clamps withmaxStep=6→currentStepstays at 5show()setsstate.onboardingVariant = .firstMeeting- Now
currentStep=5withonboardingVariant=.firstMeeting— step 5 hits thedefault: EmptyView()case inFirstMeetingFlowView.swift:82-83, rendering a blank screen
This only occurs on the first launch after switching variants via CLI when a higher step was previously saved. Subsequent launches work correctly because the variant is now persisted.
| state.onboardingVariant = .firstMeeting | |
| UserDefaults.standard.set(OnboardingVariant.firstMeeting.rawValue, forKey: "onboarding.variant") | |
| state.onboardingVariant = .firstMeeting | |
| UserDefaults.standard.set(OnboardingVariant.firstMeeting.rawValue, forKey: "onboarding.variant") | |
| let maxStep = 4 | |
| if state.currentStep > maxStep { | |
| state.currentStep = maxStep | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
totalStepsparameter toOnboardingProgressDots(default 7) with dynamic step-to-dot mapping, and passtotalSteps: 5fromFirstMeetingFlowViewso all 5 dots are correctly used across the 5-step flow--onboarding-variant first_meeting) toUserDefaultsimmediately inOnboardingWindow.show()so it survives app restartsOnboardingState.init()to prevent out-of-range rendering when the saved step exceeds the variant's step countAddresses review feedback from #1350.
Test plan
--onboarding-variant first_meetingand verify all 5 progress dots light up sequentially across steps 0-4--onboarding-variant first_meeting, relaunch, and verify variant persistsonboarding.stepto 6 in UserDefaults, launch with first-meeting variant, and verify step is clamped to 4🤖 Generated with Claude Code