fix(onboarding): let segment overrides match the entry decision - #8288
fix(onboarding): let segment overrides match the entry decision#8288talissoncosta wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
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 (1)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The onboarding entry decision now passes the user's email as a transient trait so segment-based overrides can match while bucketing remains identifier-based; no actionable merge-blocking risk remains beyond normal checks and review. 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 |
github-actions
Bot
added
the
front-end
457e7f4 to
d1b2b10
Compare
The entry decision identifies with an empty identifier and no traits, so no trait-based segment override on onboarding_quickstart_flow could ever match it. That is why the flagsmith_team override never applied, and the team never saw the variant it targets. Pass the signed-in user's email as a transient trait so overrides that match on email can apply. Email is the only trait available before the organisation exists, so overrides on plan or organisation still cannot match. Bucketing is unaffected: the percentage split is keyed on the identifier the API assigns, not on traits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d1b2b10 to
87605bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: b4f3e352-5384-4e49-89b0-c15b0a6ec1d7
📒 Files selected for processing (2)
frontend/common/utils/onboardingEntry.tsfrontend/web/components/App.js
| // @ts-expect-error transient is missing from the SDK's identify type | ||
| await flagsmith.identify('', {}, true) | ||
| await flagsmith.identify('', email ? { email } : {}, true) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
npm pack --silent --ignore-scripts \
--pack-destination "$tmp_dir" \
'`@flagsmith/flagsmith`@12.1.0' >/dev/null
tarball="$(find "$tmp_dir" -maxdepth 1 -name '*.tgz' -print -quit)"
tar -xzf "$tarball" -C "$tmp_dir"
rg -n -C 6 'identify|transient' "$tmp_dir/package"
rg -n -C 4 'flagsmith\.identify|decideOnboardingEntry' frontendRepository: Flagsmith/flagsmith
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
npm pack --silent --ignore-scripts \
--pack-destination "$tmp_dir" \
'`@flagsmith/flagsmith`@12.1.0' >/dev/null
tarball="$(find "$tmp_dir" -maxdepth 1 -name '*.tgz' -print -quit)"
tar -xzf "$tarball" -C "$tmp_dir"
printf '%s\n' '--- package declarations ---'
rg -n -C 8 'identify\s*\(' "$tmp_dir/package" --glob '*.d.ts' --glob '*.ts' --glob '*.js' --glob '*.mjs' | head -n 240
printf '%s\n' '--- transient handling ---'
rg -n -C 10 'transient' "$tmp_dir/package" --glob '*.d.ts' --glob '*.ts' --glob '*.js' --glob '*.mjs' | head -n 260
printf '%s\n' '--- repository dependency and call site ---'
rg -n -C 5 '`@flagsmith/flagsmith`|flagsmith\.identify|decideOnboardingEntry' frontend package.json package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null | head -n 180Repository: Flagsmith/flagsmith
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
npm pack --silent --ignore-scripts \
--pack-destination "$tmp_dir" \
'`@flagsmith/flagsmith`@12.1.0' >/dev/null
tarball="$(find "$tmp_dir" -maxdepth 1 -name '*.tgz' -print -quit)"
tar -tzf "$tarball" | grep -E '\.(d\.ts|ts)$' | head -n 80
tar -xzf "$tarball" -C "$tmp_dir"
printf '%s\n' '--- identify declarations ---'
find "$tmp_dir/package" -type f \( -name '*.d.ts' -o -name '*.ts' \) -print0 |
xargs -0 grep -n -H -E 'identify[[:space:]]*\(' || true
printf '%s\n' '--- package metadata ---'
python3 - "$tmp_dir/package/package.json" <<'PY'
import json, sys
with open(sys.argv[1]) as f:
data = json.load(f)
for key in ("name", "version", "types", "typings", "main", "module"):
print(f"{key}: {data.get(key)}")
PY
printf '%s\n' '--- repository dependency and call site ---'
rg -n -C 4 '`@flagsmith/flagsmith`|flagsmith\.identify|decideOnboardingEntry' \
frontend package.json package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null | head -n 180Repository: Flagsmith/flagsmith
Length of output: 15134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
npm pack --silent --ignore-scripts \
--pack-destination "$tmp_dir" \
'`@flagsmith/flagsmith`@12.1.0' >/dev/null
tarball="$(find "$tmp_dir" -maxdepth 1 -name '*.tgz' -print -quit)"
tar -xzf "$tarball" -C "$tmp_dir"
printf '%s\n' '--- public declarations ---'
for file in "$tmp_dir/package/index.d.ts" \
"$tmp_dir/package/flagsmith-core.d.ts" \
"$tmp_dir/package/types.d.ts" \
"$tmp_dir/package/src/flagsmith-core.d.ts"; do
if [ -f "$file" ]; then
echo "--- $file"
cat -n "$file" | grep -E -C 8 'identify|transient' | head -n 180 || true
fi
done
printf '%s\n' '--- package documentation ---'
find "$tmp_dir/package" -maxdepth 1 -type f -iname 'readme*' -print0 |
xargs -0 grep -n -i -C 8 'transient' || true
printf '%s\n' '--- relevant source methods ---'
python3 - "$tmp_dir/package/src/flagsmith-core.ts" <<'PY'
import sys
path = sys.argv[1]
lines = open(path).read().splitlines()
for start, end in ((600, 630), (740, 775)):
print(f"--- {path}:{start}-{end}")
for number in range(start, min(end, len(lines)) + 1):
print(f"{number}: {lines[number-1]}")
PYRepository: Flagsmith/flagsmith
Length of output: 4226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transient-related source locations ---'
rg -n -C 4 '\btransient\b' . \
-g '*.py' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' \
-g '!frontend/package-lock.json' -g '!**/node_modules/**' \
| head -n 320Repository: Flagsmith/flagsmith
Length of output: 27428
Replace the broad type suppression with a narrow type fix.
@flagsmith/flagsmith 12.1.0 declares only two identify parameters, although its implementation supports the third transient parameter. Add a narrow type augmentation or update the SDK declaration. The empty identifier and true correctly create a fully transient identity, so the email trait is not persisted.
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Minor but let's not keep the misleading comment
| // No trait means no segment override can match, and email is the only trait | ||
| // we have before the organisation exists. |
There was a problem hiding this comment.
Ok I think this comment is misleading in the sense that it made me understand that we needed a trait to have a proper evaluation while it's more to identify flagsmith' team.
That being said, that also means we need to be careful (maybe adding it in the description of the flag in the app) that for any extra segment overrides for this specific flag, we need to pass it (if available) here
There was a problem hiding this comment.
Thanks for the call. Reworded in 2b78a28.
// Only used to match segment overrides, not for bucketing. An override on
// another trait needs that trait passed here too.Does it make sense to you ?
I am planning to add similar note to the flag description in the app.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19353 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19353 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19353 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19353 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
2 participants
Changes
Contributes to #7540
Signing up with a
@flagsmith.comemail gave me the legacy flow, even though theflagsmith_teamsegment ononboarding_quickstart_flowshould have put me into the new one.The segment was fine. The entry decision identifies with no traits (
flagsmith.identify('', {}, true)), and segment overrides match on traits, so none could ever apply. This passes the user's email through as a transient trait.Bucketing is unchanged: the split is keyed on the identifier the API assigns, not on traits.
How did you test this code?
Reproduced the symptom by signing up with a
@flagsmith.comemail. Unit tests and eslint pass.Not confirmed end to end: sign up with an email in the
flagsmith_teamsegment and check you land on the new onboarding.