feat: signup corporate-only experiment with exposure and new_signup events#7979
feat: signup corporate-only experiment with exposure and new_signup events#7979Zaimwa9 wants to merge 10 commits into
Conversation
This reverts commit 56c672e.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe signup flow now records Flagsmith Estimated code review effort: 3 (Moderate) | ~20 minutes Comment |
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a04fcc89-95a6-4ac2-aec1-a2a6a17d5749
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
frontend/common/stores/account-store.jsfrontend/common/useSignupExperiment.tsfrontend/package.jsonfrontend/web/components/pages/HomePage.tsx
talissoncosta
left a comment
There was a problem hiding this comment.
Looks good, just two small comments. Do you mind checking them?
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 (2)
frontend/common/useSignupExperiment.ts (2)
9-11: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the variant when the hook is disabled.
If
useEnvironmentFlagchanges fromfalsetotrueafter a variant has been loaded, this early return leaves the old variant in state.HomePagecan then apply corporate-only blocking to excluded invite/login flows. Clear the state and returnundefinedwhile disabled.Proposed fix
if (useEnvironmentFlag) { + setVariant(undefined) return } ... - return variant + return useEnvironmentFlag ? undefined : variantAlso applies to: 25-26
12-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
flagsmith.identifyfailuresIf
flagsmith.identifyrejects, this async call is left unhandled andvariantnever gets set. Catch the error here and make the fail-open/fail-closed behaviour explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdc0e798-7e26-40be-af09-442fd68a0cad
📒 Files selected for processing (1)
frontend/common/useSignupExperiment.ts
talissoncosta
left a comment
There was a problem hiding this comment.
Thanks for checking the comments!
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)
frontend/common/useSignupExperiment.ts (1)
15-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetry-on-failure is a no-op.
identifyAndExpose().catch(() => { started.current = false })only mutates a ref; it doesn't trigger a re-render or change the effect's dependencies (useEnvironmentFlag,♻️ Possible fix: force a retry via state instead of a ref-only flag
- const [variant, setVariant] = useState<string>() - const started = useRef(false) + const [variant, setVariant] = useState<string>() + const [retryToken, setRetryToken] = useState(0) + const started = useRef(false) useEffect(() => { if (useEnvironmentFlag || started.current || !isFreeEmailDomain(email)) { return } started.current = true const identifyAndExpose = async () => { ... } identifyAndExpose().catch(() => { started.current = false + setTimeout(() => setRetryToken((t) => t + 1), 2000) }) - }, [useEnvironmentFlag, email]) + }, [useEnvironmentFlag, email, retryToken])
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa4c7104-a304-4965-83b8-4d2013c98677
📒 Files selected for processing (3)
frontend/common/stores/account-store.jsfrontend/common/useSignupExperiment.tsfrontend/web/components/pages/HomePage.tsx
|
@coderabbitai review |
✅ Action performedReview finished.
|
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Sets up our first dogfood experiment, Signup corporate domains only.
The experiment tests whether requiring a work email address at signup affects conversion. Control keeps today's behaviour (an informational message for free email domains); the
signup_corporate_onlyvariant disables the Create Account button for free email domains and shows "Please use your work email address to create your account."@flagsmith/flagsmithto 12.1.0 fortrackEvent,trackExposureEvent,getExperimentFlagand thevariantkey on flags.useSignupExperimenthook: identifies anonymous signup visitors with a transient identity (persistentsignup_anonymous_idso returning visitors keep their variant), resolves thesignup_corporate_onlyflag's variant, and records a$flag_exposure. Takes a boolean to fall back to environment flags — no identify, no exposure, control experience — for non-participants (invitees, login views, logged-in users,preventEmailPassword).new_signupconversion event on successful registration, attributed to the same anonymous identity, withfree_email_domain,invite,signup_methodandutm_sourcemetadata.How did you test this code?
Manually, against the staging configuration with the flag and experiment set up:
/signuplogged out — the network tab showsidentities/?identifier=<uuid>&transient=true, followed (≤10s) by aPOST /v1/eventscontaining a$flag_exposurewith the variant asvalue(verified 202).new_signupwith the same identifier and metadata.