ref(onboarding): Make onboarding session exits explicit - #121022
Draft
jaydgoss wants to merge 1 commit into
Draft
Conversation
setSelectedPlatform(undefined) routed through removeOnboarding, so clearing one field wiped the entire onboarding session. That coupling was invisible at the call sites and made the field setter unsafe for any state that should outlive a platform change. Field setters are now local to their field, and the five full-flow exits that genuinely want a clean session call resetOnboarding explicitly: global Skip Onboarding, the SCM header Skip, back-from-select-platform, project deletion without state preservation, and the legacy welcome-step cleanup. The SCM welcome path uses clearDerivedState instead, which already existed for repository changes. User-visible effect: the platform picker's clear control in the SCM flow no longer discards the connected repository.
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
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.
TLDR
Clearing the selected platform in SCM onboarding no longer discards the connected repository and every other staged choice. The five exits that genuinely want a clean slate now say so explicitly.
Details
setSelectedPlatform(undefined)was routed throughremoveOnboarding(), so a setter named after one field wiped the entire session. That coupling was invisible at every call site, and it made the setter unsafe for any state that should outlive a platform change — which is the blocker for the messaging destination VDY-140 stacks on top of this. The refactor lands on its own because it changes legacy onboarding behavior and deserves review on those terms, not buried in the experiment work.OnboardingContextProvidernow exposesresetOnboardingalongside the existingclearDerivedState, andsetSelectedPlatformbehaves like every sibling setter: it writes one field. The five full-flow exits callresetOnboardingdirectly — the global Skip Onboarding link inonboarding.tsx, the SCM headerOnboardingSkipButton, back-from-select-platformand project deletion without state preservation inuseBackActions, and the legacy cleanup inuseConfigureSdk.useWelcomeAnalyticsEffectis the one caller that wanted neither: on the SCM path it clears repo-derived state viaclearDerivedState, which already existed for repository changes, and falls back to a full reset only for the legacy flow.Routing the exits through a named function rather than keeping the overload means the destructive call is greppable and each site states its intent. The alternative — leaving the setter destructive and adding a second non-destructive setter — preserves the trap for the next caller who reasonably assumes a field setter touches one field.
The user-visible consequence is in
scmPlatformFeaturesCore, whose clearable platform Select callsonPlatformChange(undefined). That control previously destroyed the SCM integration and repository selection as a side effect of clearing a platform; it now leaves them intact. New coverage inonboardingContext.spec.tsxpins both halves: clearing a field preserves the rest of the session, andresetOnboardingclears persisted state. The reset test seedssessionStoragedirectly rather than using theinitialValueprop, becauseuseSessionStorage'sremoveItemrestores in-memory state back toinitialValue— a seeded prop would be restored rather than cleared, which the real provider never does since it passes noinitialValue.Refs VDY-140