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 8 commits into
Conversation
This reverts commit 56c672e.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
Bot
added
the
front-end
📝 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-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| @@ -0,0 +1,24 @@ | |||
| import { useEffect, useState } from 'react' | |||
| import flagsmith from '@flagsmith/flagsmith' | |||
| import { storageGet, storageSet } from './safeLocalStorage' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Relative import violates path-import guideline.
./safeLocalStorage is a relative import; per repository guidelines this file should import via the common/ path instead.
♻️ Proposed fix
-import { storageGet, storageSet } from './safeLocalStorage'
+import { storageGet, storageSet } from 'common/safeLocalStorage'As per coding guidelines, "Use only common/, components/, and project/ import paths; do not use relative imports" for frontend/**/*.{js,jsx,ts,tsx}.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { storageGet, storageSet } from './safeLocalStorage' | |
| import { storageGet, storageSet } from 'common/safeLocalStorage' |
Source: Coding guidelines
There was a problem hiding this comment.
This is enforced by the repo's linter: @dword-design/import-alias/prefer-alias rewrites imports to the common/ alias except for same-directory imports, which it keeps relative — eslint --fix converts common/safeLocalStorage back to ./safeLocalStorage here. Keeping the linter's form.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "import-alias" frontend/.eslintrc* 2>/dev/null
fd -H '.eslintrc*' frontend -x cat {}Length of output: 5028
Confirmed — the .eslintrc.js config enables @dword-design/import-alias/prefer-alias with common aliased to ./common/, and this plugin intentionally keeps same-directory imports relative rather than rewriting them to the alias. Since useSignupExperiment.ts and safeLocalStorage both live in frontend/common/, the relative import here is the linter's expected/enforced form, not a violation.
@Zaimwa9 thanks for the clarification — that makes sense, and my original suggestion would have been immediately reverted by eslint --fix. Withdrawing the comment.
(=^･ω･^=) ah, makes sense now
| export default function useSignupExperiment(useEnvironmentFlag: boolean) { | ||
| const [variant, setVariant] = useState<string>() | ||
|
|
||
| useEffect(() => { | ||
| if (useEnvironmentFlag) { | ||
| return | ||
| } | ||
| const identifyAndExpose = async () => { | ||
| const id = storageGet('signup_anonymous_id') || crypto.randomUUID() | ||
| storageSet('signup_anonymous_id', id) | ||
| // @ts-expect-error transient is missing from the SDK's identify type | ||
| await flagsmith.identify(id, {}, true) | ||
| const flag = flagsmith.getExperimentFlag('signup_corporate_only') | ||
| setVariant(flag?.enabled ? flag.variant : undefined) | ||
| } | ||
| identifyAndExpose() | ||
| }, [useEnvironmentFlag]) | ||
|
|
||
| return variant | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Local useState in a common/ hook may conflict with the Redux Toolkit state-management guideline.
This hook manages variant with plain React useState rather than Redux Toolkit/RTK Query as prescribed for files under frontend/common/**/*.{ts,tsx}. If this experiment variant needs to be consumed by more than one component, consider modelling it as store state instead of a bespoke hook-local state.
As per coding guidelines, "Use Redux Toolkit and RTK Query for state management, with the store located in common/store.ts" for frontend/common/**/*.{ts,tsx}.
Source: Coding guidelines
| useEffect(() => { | ||
| if (useEnvironmentFlag) { | ||
| return | ||
| } | ||
| const identifyAndExpose = async () => { | ||
| const id = storageGet('signup_anonymous_id') || crypto.randomUUID() | ||
| storageSet('signup_anonymous_id', id) | ||
| // @ts-expect-error transient is missing from the SDK's identify type | ||
| await flagsmith.identify(id, {}, true) | ||
| const flag = flagsmith.getExperimentFlag('signup_corporate_only') | ||
| setVariant(flag?.enabled ? flag.variant : undefined) | ||
| } | ||
| identifyAndExpose() | ||
| }, [useEnvironmentFlag]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files 'frontend/common/*' | rg 'useSignupExperiment|HomePage|signup|Experiment'Repository: Flagsmith/flagsmith
Length of output: 406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'frontend/common/*' | rg 'useSignupExperiment|HomePage|signup|Experiment'Repository: Flagsmith/flagsmith
Length of output: 413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search usages:\n'
rg -n "useSignupExperiment" frontend/common || true
printf '\nHook file:\n'
sed -n '1,200p' frontend/common/useSignupExperiment.ts
printf '\nHomePage candidates:\n'
rg -n "blockGenericEmailDomain|signup_corporate_only|variant" frontend/common || trueRepository: Flagsmith/flagsmith
Length of output: 1970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path('frontend/common/useSignupExperiment.ts'),
]
for p in paths:
print(f'--- {p} ---')
if p.exists():
for i, line in enumerate(p.read_text().splitlines(), 1):
print(f'{i:4}: {line}')
else:
print('missing')
PYRepository: Flagsmith/flagsmith
Length of output: 1185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'useSignupExperiment usages:\n'
rg -n "useSignupExperiment" frontend/common || true
printf '\nuseSignupExperiment.ts:\n'
cat -n frontend/common/useSignupExperiment.ts
printf '\nPotential consumers / related terms:\n'
rg -n "blockGenericEmailDomain|signup_corporate_only|variant" frontend/common || trueRepository: Flagsmith/flagsmith
Length of output: 2180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'frontend/**/*.ts' 'frontend/**/*.tsx' | rg 'useSignupExperiment|HomePage|Signup|signup' || trueRepository: Flagsmith/flagsmith
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'HomePage outline:\n'
ast-grep outline frontend/web/components/pages/HomePage.tsx --view expanded || true
printf '\nHomePage relevant section:\n'
sed -n '1,260p' frontend/web/components/pages/HomePage.tsx
printf '\nSearch for signup email gating:\n'
rg -n "blockGenericEmailDomain|useSignupExperiment|signup_corporate_only|generic email|corporate" frontend/web/components/pages/HomePage.tsx frontend/common || trueRepository: Flagsmith/flagsmith
Length of output: 10025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'HomePage around email gating and submit:\n'
sed -n '560,760p' frontend/web/components/pages/HomePage.tsx
printf '\nSubmit handlers:\n'
rg -n "handle.*Submit|onSubmit|blockGenericEmailDomain|isFreeEmailDomain|signupVariant" frontend/web/components/pages/HomePage.tsxRepository: Flagsmith/flagsmith
Length of output: 8780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Submit handler around first form:\n'
sed -n '340,430p' frontend/web/components/pages/HomePage.tsx
printf '\nSubmit handler around second form:\n'
sed -n '490,550p' frontend/web/components/pages/HomePage.tsxRepository: Flagsmith/flagsmith
Length of output: 7473
Wait for the experiment flag before treating signup as ready. useSignupExperiment() returns undefined on the first render, and HomePage only enforces the restriction in render state. A fast submit can reach register() before flagsmith.identify() resolves, so the corporate-only gate and exposure never apply for that attempt. Add a ready/loading signal or re-check in the submit handler, and guard the async state update on unmount.
There was a problem hiding this comment.
@Zaimwa9, understood — I'll leave this as-is. Let me know if you'd like to revisit it later.
(๑•́ - •̀๑)
|
|
||
| const signupVariant = useSignupExperiment( | ||
| !isSignup || isInvite || !!preventEmailPassword || !!AccountStore.getUser(), | ||
| ) | ||
| const blockGenericEmailDomain = | ||
| signupVariant === 'signup_corporate_only' && isFreeEmailDomain(email) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated magic string 'signup_corporate_only' across files.
The variant name is hardcoded here and again in useSignupExperiment.ts (getExperimentFlag('signup_corporate_only')). Extracting a shared constant would prevent silent drift if the flag/variant name ever changes.
♻️ Suggested shared constant
// common/useSignupExperiment.ts
export const SIGNUP_CORPORATE_ONLY_VARIANT = 'signup_corporate_only'- const blockGenericEmailDomain =
- signupVariant === 'signup_corporate_only' && isFreeEmailDomain(email)
+ const blockGenericEmailDomain =
+ signupVariant === SIGNUP_CORPORATE_ONLY_VARIANT && isFreeEmailDomain(email)There was a problem hiding this comment.
@Zaimwa9, understood — I'll leave it as is.
(◡﹏◡✿)
2 participants
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.