feat(onboarding): gradual rollout quest screen - #8286
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthroughAdds an Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The new onboarding rollout path currently references Utils without importing it, so the frontend cannot compile; merge should wait for this localized fix. The modal-title mismatch remains a bounded UX follow-up. 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 |
16e67ea to
a5139c3
Compare
Zaimwa9
left a comment
There was a problem hiding this comment.
Overall good thanks with some comments, i think the trait one could be a good improvement
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/web/components/pages/onboarding/OnboardingRolloutQuest/OnboardingRolloutQuest.tsx (1)
20-75: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove each new component into its required component folder.
The new components do not use the required
ComponentName/ComponentName.tsxstructure with co-located styles and a barrel export.
frontend/web/components/pages/onboarding/OnboardingRolloutQuest/OnboardingRolloutQuest.tsx#L20-L75: move the component toOnboardingRolloutQuest/OnboardingRolloutQuest.tsx, move its SCSS beside it, and export it throughOnboardingRolloutQuest/index.ts.frontend/web/components/pages/onboarding/OnboardingRolloutQuest/RolloutComingSoonCard.tsx#L18-L66: move the component toRolloutComingSoonCard/RolloutComingSoonCard.tsx, add co-locatedRolloutComingSoonCard.scssif it owns styles, and export it throughRolloutComingSoonCard/index.ts.As per coding guidelines, “Each new component must live in its own folder with a barrel
index.ts, aComponentName/ComponentName.tsxfile, co-locatedComponentName.scss, any subcomponents”.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4984a96-21b2-4fc9-9e38-006ead4e9a6f
📒 Files selected for processing (6)
frontend/web/components/pages/onboarding/OnboardingFlow/OnboardingFlow.tsxfrontend/web/components/pages/onboarding/OnboardingRolloutQuest/OnboardingRolloutQuest.scssfrontend/web/components/pages/onboarding/OnboardingRolloutQuest/OnboardingRolloutQuest.tsxfrontend/web/components/pages/onboarding/OnboardingRolloutQuest/RolloutComingSoonCard.tsxfrontend/web/components/pages/onboarding/OnboardingRolloutQuest/rolloutSteps.tsxfrontend/web/components/pages/onboarding/OnboardingRolloutQuest/trackRolloutInterest.ts
💤 Files with no reviewable changes (1)
- frontend/web/components/pages/onboarding/OnboardingRolloutQuest/OnboardingRolloutQuest.scss
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 216d6faf-ea33-4734-85cd-21e416aa75f4
📒 Files selected for processing (2)
frontend/web/components/pages/onboarding/OnboardingRolloutQuest/RolloutComingSoonCard.tsxfrontend/web/components/pages/onboarding/OnboardingRolloutQuest/useRolloutQuest.ts
a3b6647 to
7b9d19c
Compare
The "Gradual rollout" card deep-linked straight to the segment overrides tab, which explains none of the steps a rollout takes. It now opens a quest screen first: the three manual steps from the rollout guide, the identify prerequisite, and a coming-soon block gauging demand for doing it in one action. The open quest lives in the URL, so refresh and back behave, and "Create a rollout segment" continues to where the card used to go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Notify me and the feedback link fired onboarding funnel events carrying environment, organisation and project ids. That records that some organisation wants this and leaves nobody to reply to, so "we'll be in touch" was a promise we could not keep. They now also report where the other fake doors report, with the email and organisation name, matching the segment sources door in #8272. The funnel events stay as they were, since they are steps rather than signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OnboardingFlow is long enough without two more imports for four lines of analytics, so the interest tracker is its own module beside the quest it belongs to, with the event names alongside it. It also takes who is asking rather than reading it. That drops AccountStore in favour of the profile query the onboarding hook already runs, so this is a cache read, and the organisation is the renamed one the user is looking at rather than whatever the Flux store last held. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A one-off onboarding screen rather than a component anyone reuses, so there is nothing for a story to document that the screen itself does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The quest had taken 64 of OnboardingFlow's 318 lines: a URL param, four callbacks, a view effect and two tracking helpers, for one of five next steps. useRolloutQuest owns all of it and the flow asks whether it is open. OnboardingFlow drops to 277 lines and its share of the quest to eight: the hook call, a ternary in goToNextStep, and the early return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stylesheet header inventoried the file's contents, which goes stale on the first new rule. Only the Bootstrap collision survives, since that is the part you cannot work out by reading it. Two explanations appeared twice: why the quest gets a screen rather than a deep link, which now lives only in useRolloutQuest where the routing decision is, and that this promises the simplification rather than the capability, which now lives only on the card making the promise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stylesheet set the colour itself, with a comment claiming Bootstrap's .text-secondary would win the cascade. It would not: _bootstrap.scss drops secondary from Bootstrap's colour utility map for exactly that reason, so .text-secondary already resolves to the token. Two colour declarations gone. The line-height stays custom and now says why: .lh-base is 1.375 here, not 1.5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches the segment sources fake door, which is the pattern the codebase now has for these. The quest was the only next step that took over the page, which made one of five cards behave unlike the rest. The URL param goes with it, so refresh no longer keeps the quest open and browser back leaves onboarding rather than closing it. Accepted: the modal is dismissable and the card reopens it. The modal chrome supplies the title and the close control, so the screen's own heading goes, and the hook is down to opening it with its analytics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
p-0 killed the modal body padding and p-4 added the same value back. Both are $spacer * 1.5, so the two cancelled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Most restated the signature or the heading above them. Kept the ones that carry a constraint: the contrast reason for the accent chip, why the mailto has no target, and why the line height is not .lh-base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.btn is already inline-flex with gap 0.5rem, so d-inline-flex align-items-center gap-2 on an inner span restated it one node deeper. The guide link now takes .btn-link's own 0.25rem gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
surface-default is white in light mode and so is the modal body, so the card had nothing to show. border-default sets only a colour, so there was no border to fall back on either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…quest padding: 24px is p-4, list-style: none is .list-unstyled, and the two 16px headings were h3 elements held down by a font-size, so they still carried h3's 40px line height. As h6 they take size, line height, weight and the dark-mode colour from _type.scss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gate the quest on onboarding_rollout_quest, so off keeps today's deep link to the overrides tab. Persist the beta request as a trait rather than local state, so it survives a reload and can be targeted later. Tell the info icon to inherit its colour, since it hardcodes blue and Icon drops className for this case. Use fs-caption and fs-captionSmall instead of hardcoded sizes, and rename the tracking metadata's source to origin now that sources means segment sources. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It called no hooks, so the `use` prefix only bought rules-of-hooks constraints and implied state it never had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Maybe later" left no event, so leaving the quest was only visible as the gap between viewed and continued. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/if required so people know about the feature.Changes
Closes #8036.
"Gradual rollout" deep-linked straight to the segment overrides tab, which explains none of the steps a rollout takes. It now opens a modal first: the three steps from Rollout by Percentage, the
flagsmith.identify(...)prerequisite, and a fake door for doing all three in one action."Create a rollout segment" still goes where the card used to. Gated on
onboarding_rollout_quest, default off, so off is today's behaviour exactly."Notify me" records a
rollout_beta_requestedtrait, so the asked state survives a reload and the beta can later be handed out by targeting it.Screenshot
How did you test this code?
typecheckandlintcleanonboarding_rollout_questoff, it deep-links as beforerollout_beta_requestedcarries the right email and organisation, and the trait persists across a reload