feat(onboarding): run the tour against the project named in the URL - #8229
Conversation
Visiting /getting-started created show_demo_button in whichever project came back first, whether or not the customer had ever onboarded. Features are project-level, so it appeared in every environment of that project, including production, alongside a new Onboarding tag. The Getting Started nav link is ungated, so any customer could trigger this by clicking it. ensureFlag already computed isFirstFeature for analytics; it now also decides whether to create anything. An empty project still gets the demo flag, an established one gets nothing. That leaves the tour with no flag to teach with, so the page says so and points at the project's own flags instead of walking someone through connecting a project that is already connected. Copy and treatment of that state are provisional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ 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:
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
/getting-started carries no project, so the tour took the org's first project. For an established customer that is arbitrary: you can be inside project B, click Getting Started, and see project A. The entry URL now names it. The nav link carries the project you are in, or the one you were last in, and the page runs against that. An id that is not in this organisation's list is ignored rather than trusted, and a bare /getting-started (a fresh signup) still falls back to the first project or creates one. A query param rather than a nested route because /getting-started is an entry point, not a project-scoped page: it has to work before any project exists. A route like /project/:id/environment/:key/getting-started is the better long-term shape, but it needs an entry that provisions and redirects, and a decision about whether the tour keeps its chromeless layout. Environment choice is unchanged, still Development or the first one. Sending someone to wire up production during a tour would be worse than the problem. Suggested by @Zaimwa9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
talissoncosta
changed the title
0495abd to
eb26894
Compare
github-actions
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a77f25a7-9761-4396-8872-5cf2ee7f58e0
📒 Files selected for processing (7)
frontend/common/hooks/__tests__/useLastEnv.test.tsfrontend/common/hooks/useLastEnv.tsfrontend/web/components/navigation/navbars/TopNavbar.tsxfrontend/web/components/pages/onboarding/hooks/__tests__/onboardingProject.test.tsfrontend/web/components/pages/onboarding/hooks/bootstrapOnboarding.tsfrontend/web/components/pages/onboarding/hooks/onboardingProject.tsfrontend/web/components/pages/onboarding/hooks/useEnsureOnboardingResources.ts
| export const useLastEnv = (): LastEnv | null => { | ||
| const [lastEnv, setLastEnv] = useState<LastEnv | null>(null) | ||
|
|
||
| useEffect(() => { | ||
| if (typeof AsyncStorage === 'undefined') return | ||
| Promise.resolve(AsyncStorage.getItem('lastEnv')).then((raw) => | ||
| setLastEnv(parseLastEnv(raw)), | ||
| ) | ||
| }, []) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f '(usePageTracking|TopNavbar|useLastEnv)\.(ts|tsx)$' frontend |
while IFS= read -r file; do
ast-grep outline "$file" --items all
done
rg -n -C 5 -g '*.ts' -g '*.tsx' \
'AsyncStorage\.setItem\(|lastEnv|useLastEnv\s*\(|usePageTracking\s*\(' frontendRepository: Flagsmith/flagsmith
Length of output: 15096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== useLastEnv =="
cat -n frontend/common/hooks/useLastEnv.ts
echo
echo "== usePageTracking relevant section =="
sed -n '1,90p' frontend/common/hooks/usePageTracking.ts | cat -n
echo
echo "== TopNavbar full =="
cat -n frontend/web/components/navigation/navbars/TopNavbar.tsx
echo
echo "== App references =="
rg -n -C 4 -g '*.ts' -g '*.tsx' 'useLastEnv|lastEnv|getting-started|onboardingProjectId' frontend/web frontend/common
echo
echo "== Route context environment/project assignment areas =="
rg -n -C 3 -g '*.ts' -g '*.tsx' 'organisationId|environmentId|projectId|routeContext|setRouteContext' frontend/web frontend/common | head -n 220Repository: Flagsmith/flagsmith
Length of output: 37713
Keep lastEnv changes in a single source of truth.
useLastEnv only reads storage when it mounts, while usePageTracking, EnvironmentAside, and Nav can write new lastEnv values. TopNavbar can then render the Getting Started link with a project that is no longer the last selected one. Move shared lastEnv state into common/store.ts and make writers update that state when they persist to storage.
Sources: Coding guidelines, Learnings
| useEffect(() => { | ||
| if (typeof AsyncStorage === 'undefined') return | ||
| Promise.resolve(AsyncStorage.getItem('lastEnv')).then((raw) => | ||
| setLastEnv(parseLastEnv(raw)), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle failed storage reads.
If AsyncStorage.getItem('lastEnv') rejects or throws, this effect creates an unhandled error. Set the fallback state to null when the read fails.
Proposed fix
- Promise.resolve(AsyncStorage.getItem('lastEnv')).then((raw) =>
- setLastEnv(parseLastEnv(raw)),
- )
+ void Promise.resolve()
+ .then(() => AsyncStorage.getItem('lastEnv'))
+ .then((raw) => setLastEnv(parseLastEnv(raw)))
+ .catch(() => setLastEnv(null))📝 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.
| useEffect(() => { | |
| if (typeof AsyncStorage === 'undefined') return | |
| Promise.resolve(AsyncStorage.getItem('lastEnv')).then((raw) => | |
| setLastEnv(parseLastEnv(raw)), | |
| ) | |
| useEffect(() => { | |
| if (typeof AsyncStorage === 'undefined') return | |
| void Promise.resolve() | |
| .then(() => AsyncStorage.getItem('lastEnv')) | |
| .then((raw) => setLastEnv(parseLastEnv(raw))) | |
| .catch(() => setLastEnv(null)) |
3f8c70c to
e66002b
Compare
|
Closing this one. Walking the cases, it buys less than it looked like:
The change that actually fixes "established customers shouldn't be in the tour" is gating the nav link on whether the account has already evaluated a flag. Once that lands, everyone who can reach /getting-started is a new user with a single project, and |
1 participant
docs/if required so people know about the feature.Changes
Stacked on #8217. Suggested by @Zaimwa9.
/getting-startedcarries no project, so the tour ran against the org's first one. You could be inside project B, click Getting Started, and see project A.TopNavbaralready hasprojectId), else the one you were last in./getting-started, which is what a fresh signup gets, behaves as before.A query param rather than a nested route:
/getting-startedhas to work before any project exists./project/:id/environment/:key/getting-startedis the better shape and would delete this selection code, but it needs an entry that provisions and redirects, plus a decision on whether the tour keeps its chromeless layout. Worth doing with the drawer.Environment choice is unchanged, still
Developmentor the first one.lastEnvholds an environment too, but using it would mean telling someone to wire up production during a tour.How did you test this code?
12 unit tests: URL project wins, id matched as a string, id outside the org ignored, no param, empty org, plus
parseLastEnvagainst absent and malformed storagetest:unit(406),typecheckandlintcleanInside project B: link carries
?project=<B>and the tour shows BOutside a project: falls back to the one you were last in
After switching org: no stale project from the previous one
URL edited to another org's project: falls back
New signup: unchanged