Closed
Conversation
Contributor
WalkthroughAdds a feature-flag-driven onboarding flow (provider, pages, stepper, form, schemas), integrates FeatureFlagProvider into the app provider tree, updates org-redirect logic to respect onboarding flag, consolidates shared Zod schemas, and introduces a Firefox-friendly source-map webpack option. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio/src/lib/track.ts (1)
6-11: 🛠️ Refactor suggestion | 🟠 MajorRemove dead
window.kodeclaration.The
koproperty (Koala tracking) is no longer used in the codebase. Keeping this declaration adds confusion and technical debt.Based on learnings: "In studio/src/lib/track.ts, remove all references to window.ko (Koala tracking) as it is no longer used."
♻️ Proposed fix
declare global { interface Window { - ko: any; Reo: any; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/lib/track.ts` around lines 6 - 11, Remove the dead Koala tracking declaration by deleting the window.ko entry in the global augmentation inside track.ts: remove the "ko: any;" line from the declare global { interface Window { ... } } block and any other references to window.ko in this file; ensure only active properties (e.g., Reo) remain in the Window interface so TypeScript no longer exposes the unused Koala symbol.
🧹 Nitpick comments (6)
studio/src/components/onboarding/onboarding-form.tsx (2)
59-62: Form submission is not wired up.The
onSubmithandler only logs to the console. Based on the context snippet fromsettings.tsx, the organization name update should useuseMutation(updateOrganizationDetails). Additionally, member invitations and channel preferences likely need separate API calls. This is a placeholder—ensure it's tracked for completion.Would you like me to scaffold the submission logic using the
updateOrganizationDetailsmutation pattern, or open an issue to track this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 59 - 62, The onSubmit handler currently only logs data; replace this placeholder with real submission logic: use the existing mutation pattern (call useMutation(updateOrganizationDetails)) to update organization fields from OnboardingFormValues (e.g., organization name), then call the separate APIs/mutations for member invitations and channel preferences (e.g., inviteMembers, updateChannelPreferences or similarly named hooks) in sequence or in parallel with proper error handling; ensure you handle loading and errors, update local state/cache on success, and wire the resulting submit handler (onSubmit) into the form submission flow.
87-93: Error messages are disconnected from their associated inputs.Rendering validation errors in a separate loop after all input fields causes errors to appear grouped at the bottom rather than inline with the corresponding field. This degrades UX—users must correlate error messages manually.
♻️ Proposed fix: inline errors with their inputs
{fields.map((field, index) => ( <div key={field.id} className="flex items-center gap-2"> <Input placeholder="janedoe@example.com" className="max-w-md" {...register(`members.${index}.email`)} /> <Button type="button" variant="ghost" size="icon-sm" onClick={() => remove(index)}> <Cross1Icon /> </Button> + {errors.members?.[index]?.email && ( + <span className="text-sm text-destructive">{errors.members[index].email.message}</span> + )} </div> ))} - {fields.map((_, index) => - errors.members?.[index]?.email ? ( - <span key={index} className="text-sm text-destructive"> - {errors.members[index].email.message} - </span> - ) : null, - )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 87 - 93, The validation error messages for members are rendered in a separate map so they appear grouped instead of inline; update the rendering so each member input shows its own error by moving the error check into the same map that renders the member input (use the existing fields.map((_, index) => ...) block that renders the input), and render errors.members?.[index]?.email?.message immediately beneath or beside the corresponding input (use errors.members?.[index]?.email to avoid undefined access) so each error is displayed inline with its associated field.studio/src/pages/_app.tsx (1)
41-58: PostHog initialization guard is correct but cleanup logic may be skipped.The early return on line 42 prevents re-initialization, which is good. However, if
posthog.__loadedis alreadytrueon first render (e.g., from a previous HMR cycle), the route change listener is never registered for that session. This may cause pageview tracking to silently stop working after hot reloads in development.Consider moving the listener registration outside the
__loadedguard if pageview tracking should always be active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/_app.tsx` around lines 41 - 58, The useEffect currently exits early when posthog.__loaded is true which prevents registering the Router.events listener; update the useEffect so the pageview listener registration (the handleRouteChange + Router.events.on('routeChangeComplete', handleRouteChange) and its corresponding cleanup Router.events.off(...)) runs regardless of posthog.__loaded while keeping the posthog.init call guarded by posthog.__loaded, i.e., call posthog.init(...) only if not loaded but always attach/detach the Router.events listener for consistent pageview tracking (references: posthog, posthog.__loaded, handleRouteChange, Router.events.on/off inside the useEffect).studio/src/components/onboarding/onboarding-provider.tsx (1)
7-8: Consider adding explicit return type annotation.Per coding guidelines, TypeScript functions should have explicit return type annotations.
As per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript."
♻️ Proposed fix
-export const OnboardingProvider = ({ children }: { children: ReactNode }) => { +export const OnboardingProvider = ({ children }: { children: ReactNode }): ReactNode => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/onboarding-provider.tsx` around lines 7 - 8, The OnboardingProvider function lacks an explicit return type; update the component signature "OnboardingProvider" to include a TypeScript return annotation (e.g., ": JSX.Element" or ": React.ReactElement") so the function becomes exported with an explicit return type, and if necessary add/import the React types (ReactNode/JSX) used in the annotation to satisfy the compiler.studio/src/components/onboarding/stepper.tsx (1)
28-42: Consider adding aria attributes for accessibility.The step markers are purely visual. Screen reader users may benefit from additional context about step state.
♿ Optional accessibility enhancement
<TooltipTrigger asChild> <div + role="listitem" + aria-current={isCurrent ? 'step' : undefined} + aria-label={`Step ${index + 1}: ${step.label}${isCompleted ? ' (completed)' : isCurrent ? ' (current)' : ''}`} className={cn( 'flex h-5 w-5 cursor-default items-center justify-center rounded-full border-2 text-[10px] font-medium transition-colors',And wrap the container with
role="list":- <div className={cn('flex items-center', className)}> + <div role="list" aria-label="Onboarding progress" className={cn('flex items-center', className)}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/stepper.tsx` around lines 28 - 42, The step marker is purely visual and needs ARIA for screen-reader context: wrap the stepper container with role="list" and give each step element role="listitem"; on the step marker div (the one inside TooltipTrigger) add an accessible label (e.g., aria-label=`${step.label} — step ${index+1} — ${isCompleted ? 'completed' : isCurrent ? 'current' : 'not completed'}`) and set aria-current="step" when isCurrent and aria-disabled="true" or aria-checked="true" when appropriate for completed state; keep existing symbols (Tooltip, TooltipTrigger, CheckIcon, isCompleted, isCurrent, step.label) but enhance the div with those ARIA attributes so screen readers get step number, label and state.studio/src/components/feature-flag-provider.tsx (1)
16-23: Add exhaustive check to reducer.The reducer handles all current cases but lacks an exhaustive check. TypeScript won't catch missing cases if new action types are added.
♻️ Add exhaustive check
function featureFlagReducer(_state: FeatureFlagState, action: FeatureFlagAction): FeatureFlagState { switch (action.type) { case 'LOADING': return { status: 'pending', onboarding: { enabled: false } }; case 'LOADED': return { status: 'success', onboarding: { enabled: action.onboardingEnabled } }; + default: { + const _exhaustive: never = action; + return _state; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/feature-flag-provider.tsx` around lines 16 - 23, The reducer featureFlagReducer lacks an exhaustive default branch for FeatureFlagAction, so add a final switch default that enforces exhaustiveness (e.g., narrow action to never and throw an error) to ensure TypeScript will error when new action types are introduced; update the switch in featureFlagReducer to include this exhaustive check using the action variable (or an explicit helper like assertNever) to surface missing cases at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/feature-flag-provider.tsx`:
- Around line 34-55: FeatureFlagProvider can hang on the loading screen because
useFeatureFlagEnabled may stay undefined; modify the useEffect that watches
onboardingFlag to start a timeout (e.g., 2–5s) that dispatches a fallback
dispatch({ type: 'LOADED', onboardingEnabled: <defaultBoolean> }) if
onboardingFlag remains undefined, and clear the timeout when onboardingFlag
becomes defined or on unmount; keep the existing dispatch behavior when
onboardingFlag resolves, and ensure the timeout is cleaned up to avoid
dispatching after unmount.
In `@studio/src/components/onboarding/onboarding-form.tsx`:
- Around line 134-136: The Skip Button in onboarding-form.tsx calls
router.push(`/${org?.slug}/graphs`) and can navigate to "/undefined/graphs" when
org is undefined; change the Button to be disabled when org?.slug is falsy
(e.g., disabled={!org?.slug}) and/or wrap the onClick handler (the router.push
call) with a guard that only calls router.push when org?.slug exists to prevent
navigation to an invalid route.
In `@studio/src/components/onboarding/onboarding-provider.tsx`:
- Around line 10-18: The redirect effect in handleRedirectInOnboardingProvider
currently sends all non-onboarding routes to '/onboarding' and must skip public
auth routes; modify the useEffect so it early-returns if Router.pathname matches
any public paths (e.g. '/login', '/signup', and other auth entry points you
support) before calling Router.replace; implement this by adding a small
PUBLIC_AUTH_PATHS array (or set) and checking PUBLIC_AUTH_PATHS.some(p =>
Router.pathname.startsWith(p)) in the same effect that checks
ONBOARDING_PATH_PREFIX and onboarding.enabled so login/signup are not
intercepted.
In `@studio/src/lib/form-schemas.ts`:
- Around line 8-17: The organizationSlugSchema currently enforces .max(24) which
conflicts with backend constraints (3-32); update organizationSlugSchema to use
.max(32) and adjust the accompanying error message string to "Organization slug
must be maximum 32 characters" so the schema (organizationSlugSchema) aligns
with backend validation and the create page behavior.
In `@studio/src/lib/track.ts`:
- Around line 40-52: The identify call can race with PostHog initialization:
update the logic around posthog.identify/posthog.get_distinct_id (called from
app-provider.tsx) to first ensure PostHog is loaded (e.g. check posthog.__loaded
or a completion callback from the init effect in _app.tsx) and defer/queue the
identify until initialization completes; if not loaded, attach a one-time
listener or retry after init (or use PostHog's provided callback pattern such as
onFeatureFlags/onLoaded) so identify always runs with the real distinct_id
rather than an anonymous placeholder.
In `@studio/src/pages/onboarding/index.tsx`:
- Around line 8-26: The page hardcodes step to 0 causing the wizard to always
render Step1Welcome and always pass 0 to getOnboardingLayout; update
OnboardingPage to derive step dynamically (e.g., useState/useEffect with URL
query param or context) instead of const step = 0, update the switch to use that
dynamic step (referencing OnboardingPage, step, Step1Welcome, Step2Federation,
Step3CreateGraph, Step4RunServices), and pass the runtime step value to
OnboardingPage.getLayout by calling getOnboardingLayout(page, step); if this
hardcode is intentional add a clear TODO comment near const step and the
getLayout assignment explaining why.
---
Outside diff comments:
In `@studio/src/lib/track.ts`:
- Around line 6-11: Remove the dead Koala tracking declaration by deleting the
window.ko entry in the global augmentation inside track.ts: remove the "ko:
any;" line from the declare global { interface Window { ... } } block and any
other references to window.ko in this file; ensure only active properties (e.g.,
Reo) remain in the Window interface so TypeScript no longer exposes the unused
Koala symbol.
---
Nitpick comments:
In `@studio/src/components/feature-flag-provider.tsx`:
- Around line 16-23: The reducer featureFlagReducer lacks an exhaustive default
branch for FeatureFlagAction, so add a final switch default that enforces
exhaustiveness (e.g., narrow action to never and throw an error) to ensure
TypeScript will error when new action types are introduced; update the switch in
featureFlagReducer to include this exhaustive check using the action variable
(or an explicit helper like assertNever) to surface missing cases at compile
time.
In `@studio/src/components/onboarding/onboarding-form.tsx`:
- Around line 59-62: The onSubmit handler currently only logs data; replace this
placeholder with real submission logic: use the existing mutation pattern (call
useMutation(updateOrganizationDetails)) to update organization fields from
OnboardingFormValues (e.g., organization name), then call the separate
APIs/mutations for member invitations and channel preferences (e.g.,
inviteMembers, updateChannelPreferences or similarly named hooks) in sequence or
in parallel with proper error handling; ensure you handle loading and errors,
update local state/cache on success, and wire the resulting submit handler
(onSubmit) into the form submission flow.
- Around line 87-93: The validation error messages for members are rendered in a
separate map so they appear grouped instead of inline; update the rendering so
each member input shows its own error by moving the error check into the same
map that renders the member input (use the existing fields.map((_, index) =>
...) block that renders the input), and render
errors.members?.[index]?.email?.message immediately beneath or beside the
corresponding input (use errors.members?.[index]?.email to avoid undefined
access) so each error is displayed inline with its associated field.
In `@studio/src/components/onboarding/onboarding-provider.tsx`:
- Around line 7-8: The OnboardingProvider function lacks an explicit return
type; update the component signature "OnboardingProvider" to include a
TypeScript return annotation (e.g., ": JSX.Element" or ": React.ReactElement")
so the function becomes exported with an explicit return type, and if necessary
add/import the React types (ReactNode/JSX) used in the annotation to satisfy the
compiler.
In `@studio/src/components/onboarding/stepper.tsx`:
- Around line 28-42: The step marker is purely visual and needs ARIA for
screen-reader context: wrap the stepper container with role="list" and give each
step element role="listitem"; on the step marker div (the one inside
TooltipTrigger) add an accessible label (e.g., aria-label=`${step.label} — step
${index+1} — ${isCompleted ? 'completed' : isCurrent ? 'current' : 'not
completed'}`) and set aria-current="step" when isCurrent and
aria-disabled="true" or aria-checked="true" when appropriate for completed
state; keep existing symbols (Tooltip, TooltipTrigger, CheckIcon, isCompleted,
isCurrent, step.label) but enhance the div with those ARIA attributes so screen
readers get step number, label and state.
In `@studio/src/pages/_app.tsx`:
- Around line 41-58: The useEffect currently exits early when posthog.__loaded
is true which prevents registering the Router.events listener; update the
useEffect so the pageview listener registration (the handleRouteChange +
Router.events.on('routeChangeComplete', handleRouteChange) and its corresponding
cleanup Router.events.off(...)) runs regardless of posthog.__loaded while
keeping the posthog.init call guarded by posthog.__loaded, i.e., call
posthog.init(...) only if not loaded but always attach/detach the Router.events
listener for consistent pageview tracking (references: posthog,
posthog.__loaded, handleRouteChange, Router.events.on/off inside the useEffect).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8433bed8-68bd-4771-9cde-1197e2848bc8
📒 Files selected for processing (19)
studio/README.mdstudio/next.config.mjsstudio/src/components/app-provider.tsxstudio/src/components/feature-flag-provider.tsxstudio/src/components/layout/title-layout.tsxstudio/src/components/onboarding/onboarding-form.tsxstudio/src/components/onboarding/onboarding-layout.tsxstudio/src/components/onboarding/onboarding-provider.tsxstudio/src/components/onboarding/step-1-welcome.tsxstudio/src/components/onboarding/step-2-federation.tsxstudio/src/components/onboarding/step-3-create-graph.tsxstudio/src/components/onboarding/step-4-run-services.tsxstudio/src/components/onboarding/stepper.tsxstudio/src/lib/form-schemas.tsstudio/src/lib/track.tsstudio/src/pages/[organizationSlug]/members.tsxstudio/src/pages/[organizationSlug]/settings.tsxstudio/src/pages/_app.tsxstudio/src/pages/onboarding/index.tsx
Comment on lines
+34
to
+55
| export const FeatureFlagProvider = ({ children }: { children: ReactNode }) => { | ||
| const onboardingFlag = useFeatureFlagEnabled('cosmo-onboarding-v1'); | ||
| const [state, dispatch] = useReducer(featureFlagReducer, initialState); | ||
|
|
||
| useEffect(() => { | ||
| if (onboardingFlag === undefined) { | ||
| dispatch({ type: 'LOADING' }); | ||
| } else { | ||
| dispatch({ type: 'LOADED', onboardingEnabled: onboardingFlag }); | ||
| } | ||
| }, [onboardingFlag]); | ||
|
|
||
| if (state.status !== 'success') { | ||
| return ( | ||
| <div className="fixed inset-0 flex items-center justify-center bg-background"> | ||
| <Loader /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return <FeatureFlagContext.Provider value={state}>{children}</FeatureFlagContext.Provider>; | ||
| }; |
Contributor
There was a problem hiding this comment.
No timeout or fallback for feature flag loading.
If PostHog fails to initialize (ad blocker, network error, slow connection), onboardingFlag may remain undefined indefinitely, leaving users stuck on the loading screen (lines 46-52).
Consider adding a timeout that falls back to a default state:
🛡️ Proposed fix with timeout fallback
export const FeatureFlagProvider = ({ children }: { children: ReactNode }) => {
const onboardingFlag = useFeatureFlagEnabled('cosmo-onboarding-v1');
const [state, dispatch] = useReducer(featureFlagReducer, initialState);
useEffect(() => {
if (onboardingFlag === undefined) {
dispatch({ type: 'LOADING' });
} else {
dispatch({ type: 'LOADED', onboardingEnabled: onboardingFlag });
}
}, [onboardingFlag]);
+ // Fallback timeout if PostHog fails to load
+ useEffect(() => {
+ if (state.status === 'success') return;
+
+ const timeout = setTimeout(() => {
+ if (state.status !== 'success') {
+ dispatch({ type: 'LOADED', onboardingEnabled: false });
+ }
+ }, 5000);
+
+ return () => clearTimeout(timeout);
+ }, [state.status]);
+
if (state.status !== 'success') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/components/feature-flag-provider.tsx` around lines 34 - 55,
FeatureFlagProvider can hang on the loading screen because useFeatureFlagEnabled
may stay undefined; modify the useEffect that watches onboardingFlag to start a
timeout (e.g., 2–5s) that dispatches a fallback dispatch({ type: 'LOADED',
onboardingEnabled: <defaultBoolean> }) if onboardingFlag remains undefined, and
clear the timeout when onboardingFlag becomes defined or on unmount; keep the
existing dispatch behavior when onboardingFlag resolves, and ensure the timeout
is cleaned up to avoid dispatching after unmount.
Comment on lines
+134
to
+136
| <Button type="button" variant="outline" onClick={() => router.push(`/${org?.slug}/graphs`)}> | ||
| Skip | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Guard against undefined organization slug before navigation.
If org is undefined, this navigates to /undefined/graphs, resulting in a 404 or unexpected behavior. Add a guard or disable the button when org is unavailable.
🛡️ Proposed fix
- <Button type="button" variant="outline" onClick={() => router.push(`/${org?.slug}/graphs`)}>
+ <Button type="button" variant="outline" disabled={!org?.slug} onClick={() => router.push(`/${org?.slug}/graphs`)}>📝 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.
Suggested change
| <Button type="button" variant="outline" onClick={() => router.push(`/${org?.slug}/graphs`)}> | |
| Skip | |
| </Button> | |
| <Button type="button" variant="outline" disabled={!org?.slug} onClick={() => router.push(`/${org?.slug}/graphs`)}> | |
| Skip | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 134 - 136,
The Skip Button in onboarding-form.tsx calls router.push(`/${org?.slug}/graphs`)
and can navigate to "/undefined/graphs" when org is undefined; change the Button
to be disabled when org?.slug is falsy (e.g., disabled={!org?.slug}) and/or wrap
the onClick handler (the router.push call) with a guard that only calls
router.push when org?.slug exists to prevent navigation to an invalid route.
Comment on lines
+10
to
+18
| useEffect( | ||
| function handleRedirectInOnboardingProvider() { | ||
| if (!onboarding.enabled) return; | ||
| if (Router.pathname.startsWith(ONBOARDING_PATH_PREFIX)) return; | ||
|
|
||
| Router.replace('/onboarding'); | ||
| }, | ||
| [onboarding.enabled], | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find publicPaths definition in app-provider to confirm the paths that should be excluded
rg -n "publicPaths" --type=ts -C3Repository: wundergraph/cosmo
Length of output: 1242
🏁 Script executed:
# Check the actual onboarding-provider.tsx file
cat -n studio/src/components/onboarding/onboarding-provider.tsxRepository: wundergraph/cosmo
Length of output: 802
🏁 Script executed:
# Find where ONBOARDING_PATH_PREFIX is defined
rg -n "ONBOARDING_PATH_PREFIX" --type=ts -B2 -A2Repository: wundergraph/cosmo
Length of output: 1054
🏁 Script executed:
# Check how onboarding feature flag is defined and used
rg -n "useFeatureFlags\|onboarding\.enabled" --type=ts -C2 | head -50Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Find where OnboardingProvider is imported/used
rg -n "OnboardingProvider" --type=tsx -B2 -A2Repository: wundergraph/cosmo
Length of output: 88
🏁 Script executed:
# Check _app.tsx or main layout to see component tree ordering
fd -e tsx -e ts | grep -E "(_app|_document|app\.|layout\.)" | head -20Repository: wundergraph/cosmo
Length of output: 597
🏁 Script executed:
# Look at feature flag provider to understand when onboarding flag is set
cat -n studio/src/components/feature-flag-provider.tsx | head -50Repository: wundergraph/cosmo
Length of output: 2043
🏁 Script executed:
# Search for OnboardingProvider usage with correct file type
rg -n "OnboardingProvider" --type ts -B2 -A2Repository: wundergraph/cosmo
Length of output: 2726
🏁 Script executed:
# Read _app.tsx to see provider ordering
cat -n studio/src/pages/_app.tsx | head -100Repository: wundergraph/cosmo
Length of output: 4206
Redirect intercepts login/signup paths when onboarding flag is enabled.
This effect redirects all non-onboarding paths to /onboarding when the feature flag is enabled. However, it doesn't exclude public paths like /login and /signup. If the onboarding flag is enabled for unauthenticated users (possible via PostHog feature flags), they'll be redirected away from /login instead of reaching the auth page, breaking the login flow. AppProvider intentionally delegates redirect handling to OnboardingProvider when the flag is enabled (per app-provider.tsx line 193-194), making OnboardingProvider responsible for respecting auth boundaries.
Add checks to exclude public paths:
Proposed fix
+const PUBLIC_PATHS = ['/login', '/signup'];
+
export const OnboardingProvider = ({ children }: { children: ReactNode }) => {
const { onboarding } = useFeatureFlags();
useEffect(
function handleRedirectInOnboardingProvider() {
if (!onboarding.enabled) return;
if (Router.pathname.startsWith(ONBOARDING_PATH_PREFIX)) return;
+ if (PUBLIC_PATHS.includes(Router.pathname)) return;
+ if (Router.pathname.startsWith('/api/')) return;
Router.replace('/onboarding');
},
[onboarding.enabled],
);
return children;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/components/onboarding/onboarding-provider.tsx` around lines 10 -
18, The redirect effect in handleRedirectInOnboardingProvider currently sends
all non-onboarding routes to '/onboarding' and must skip public auth routes;
modify the useEffect so it early-returns if Router.pathname matches any public
paths (e.g. '/login', '/signup', and other auth entry points you support) before
calling Router.replace; implement this by adding a small PUBLIC_AUTH_PATHS array
(or set) and checking PUBLIC_AUTH_PATHS.some(p => Router.pathname.startsWith(p))
in the same effect that checks ONBOARDING_PATH_PREFIX and onboarding.enabled so
login/signup are not intercepted.
Comment on lines
+40
to
+52
| // We allow PostHog tracking for any environment, if the key is provided | ||
| if (process.env.NEXT_PUBLIC_POSTHOG_KEY) { | ||
| // Identify with PostHog | ||
| // We use the id posthog sets to identify the user. This way we do not lose cross domain tracking. | ||
| posthog.identify(posthog.get_distinct_id(), { | ||
| id, | ||
| email, | ||
| organizationId, | ||
| organizationName, | ||
| organizationSlug, | ||
| plan, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential race condition: PostHog may not be initialized when identify() is called.
posthog.identify(posthog.get_distinct_id(), {...}) assumes PostHog is already initialized. However, both the init effect in _app.tsx and the identify() call in app-provider.tsx run in separate useEffect hooks with no guaranteed ordering. If identify() executes before posthog.init() completes, get_distinct_id() returns an uninitialized/anonymous ID, causing silent user misidentification.
Consider guarding with posthog.__loaded or using PostHog's onFeatureFlags callback pattern to ensure initialization is complete.
🛡️ Proposed fix
// We allow PostHog tracking for any environment, if the key is provided
- if (process.env.NEXT_PUBLIC_POSTHOG_KEY) {
+ if (process.env.NEXT_PUBLIC_POSTHOG_KEY && posthog.__loaded) {
// Identify with PostHog
// We use the id posthog sets to identify the user. This way we do not lose cross domain tracking.
posthog.identify(posthog.get_distinct_id(), {📝 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.
Suggested change
| // We allow PostHog tracking for any environment, if the key is provided | |
| if (process.env.NEXT_PUBLIC_POSTHOG_KEY) { | |
| // Identify with PostHog | |
| // We use the id posthog sets to identify the user. This way we do not lose cross domain tracking. | |
| posthog.identify(posthog.get_distinct_id(), { | |
| id, | |
| email, | |
| organizationId, | |
| organizationName, | |
| organizationSlug, | |
| plan, | |
| }); | |
| } | |
| // We allow PostHog tracking for any environment, if the key is provided | |
| if (process.env.NEXT_PUBLIC_POSTHOG_KEY && posthog.__loaded) { | |
| // Identify with PostHog | |
| // We use the id posthog sets to identify the user. This way we do not lose cross domain tracking. | |
| posthog.identify(posthog.get_distinct_id(), { | |
| id, | |
| email, | |
| organizationId, | |
| organizationName, | |
| organizationSlug, | |
| plan, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/lib/track.ts` around lines 40 - 52, The identify call can race
with PostHog initialization: update the logic around
posthog.identify/posthog.get_distinct_id (called from app-provider.tsx) to first
ensure PostHog is loaded (e.g. check posthog.__loaded or a completion callback
from the init effect in _app.tsx) and defer/queue the identify until
initialization completes; if not loaded, attach a one-time listener or retry
after init (or use PostHog's provided callback pattern such as
onFeatureFlags/onLoaded) so identify always runs with the real distinct_id
rather than an anonymous placeholder.
Comment on lines
+8
to
+26
| const OnboardingPage: NextPageWithLayout = () => { | ||
| const step: number = 0; | ||
|
|
||
| switch (step) { | ||
| case 1: | ||
| return <Step2Federation />; | ||
| case 2: | ||
| return <Step3CreateGraph />; | ||
| case 3: | ||
| return <Step4RunServices />; | ||
| case 0: | ||
| default: | ||
| return <Step1Welcome />; | ||
| } | ||
| }; | ||
|
|
||
| OnboardingPage.getLayout = (page) => { | ||
| return getOnboardingLayout(page, 0); | ||
| }; |
Contributor
There was a problem hiding this comment.
Step progression is non-functional.
The step variable is hardcoded to 0 (line 9) and getLayout always passes 0 (line 25), making the wizard permanently stuck on Step1Welcome. The switch statement branches for steps 1-3 are unreachable dead code.
If this is intentional scaffolding for PR #1, consider adding a TODO comment. Otherwise, step state needs to be managed (e.g., via useState, URL query params, or context) and passed dynamically to getOnboardingLayout.
Example approach using URL query params
+import { useRouter } from 'next/router';
+
const OnboardingPage: NextPageWithLayout = () => {
- const step: number = 0;
+ const router = useRouter();
+ const step = typeof router.query.step === 'string' ? parseInt(router.query.step, 10) : 0;
switch (step) {Note: With Next.js page-level getLayout, you'd need a different architecture (e.g., separate pages per step, or lifting step state to context) to synchronize the Stepper's currentStep with the actual step being rendered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/pages/onboarding/index.tsx` around lines 8 - 26, The page
hardcodes step to 0 causing the wizard to always render Step1Welcome and always
pass 0 to getOnboardingLayout; update OnboardingPage to derive step dynamically
(e.g., useState/useEffect with URL query param or context) instead of const step
= 0, update the switch to use that dynamic step (referencing OnboardingPage,
step, Step1Welcome, Step2Federation, Step3CreateGraph, Step4RunServices), and
pass the runtime step value to OnboardingPage.getLayout by calling
getOnboardingLayout(page, step); if this hardcode is intentional add a clear
TODO comment near const step and the getLayout assignment explaining why.
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics
branch
from
23e453c to
0da102a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio/src/pages/_app.tsx (1)
42-57:⚠️ Potential issue | 🟠 MajorAvoid returning before route listener registration.
The early return exits the effect before
routeChangeCompleteis subscribed, so$pageviewtracking can be skipped on mounts where PostHog is already loaded.Proposed fix
useEffect(() => { - if (posthog.__loaded) return; - - posthog.init(process.env.NEXT_PUBLIC_POSTHOG_KEY!, { - api_host: '/ingest', - loaded: (ph) => { - if (process.env.NODE_ENV === 'development') ph.debug(); - }, - debug: process.env.NODE_ENV === 'development', - }); + if (!posthog.__loaded) { + posthog.init(process.env.NEXT_PUBLIC_POSTHOG_KEY!, { + api_host: '/ingest', + loaded: (ph) => { + if (process.env.NODE_ENV === 'development') ph.debug(); + }, + debug: process.env.NODE_ENV === 'development', + }); + } const handleRouteChange = () => posthog.capture('$pageview'); Router.events.on('routeChangeComplete', handleRouteChange);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/_app.tsx` around lines 42 - 57, The early return when posthog.__loaded is truthy prevents registering the routeChangeComplete listener, so change the logic to only skip calling posthog.init when posthog.__loaded is true but always register the handleRouteChange listener and its cleanup; i.e., replace the early return with a conditional around posthog.init (call posthog.init(...) only if !posthog.__loaded), then define handleRouteChange and call Router.events.on('routeChangeComplete', handleRouteChange) and Router.events.off(...) in the effect cleanup so $pageview is always captured even when posthog is already loaded.
♻️ Duplicate comments (1)
studio/src/components/onboarding/onboarding-form.tsx (1)
134-136:⚠️ Potential issue | 🟡 MinorGuard Skip navigation when organization slug is unavailable.
This can still route to
/undefined/graphswhenorgis not ready.Proposed fix
- <Button type="button" variant="outline" onClick={() => router.push(`/${org?.slug}/graphs`)}> + <Button + type="button" + variant="outline" + disabled={!org?.slug} + onClick={() => { + if (!org?.slug) return; + void router.push(`/${org.slug}/graphs`); + }} + > Skip </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 134 - 136, The Skip button in onboarding-form.tsx currently calls router.push(`/${org?.slug}/graphs`) and can navigate to /undefined/graphs if org or org.slug is not ready; update the onClick handler (and/or Button props) to guard against missing org.slug by only calling router.push when org?.slug is truthy, and otherwise either disable the Button or navigate to a safe fallback (e.g., '/') to avoid constructing an invalid path.
🧹 Nitpick comments (2)
studio/src/components/onboarding/stepper.tsx (1)
15-49: Consider adding an explicit return type annotation.Per coding guidelines, TypeScript functions should have explicit return type annotations. The
Stepperfunction is missing a return type.🔧 Proposed fix
-export function Stepper({ steps, currentStep, className }: StepperProps) { +export function Stepper({ steps, currentStep, className }: StepperProps): React.JSX.Element {As per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/stepper.tsx` around lines 15 - 49, The Stepper function is missing an explicit return type; update the Stepper signature (which currently takes StepperProps) to include an explicit React return type such as JSX.Element or React.ReactElement (e.g., change "export function Stepper({ steps, currentStep, className }: StepperProps)" to include ": JSX.Element") so the Stepper component has a clear TypeScript-annotated return type.studio/src/components/onboarding/onboarding-form.tsx (1)
28-29: Add explicit return types to TypeScript functions.Both
OnboardingFormandonSubmitcurrently rely on inferred return types.Proposed fix
-export function OnboardingForm() { +export function OnboardingForm(): JSX.Element { ... - const onSubmit = (data: OnboardingFormValues) => { + const onSubmit = (data: OnboardingFormValues): void => {As per coding guidelines, "
**/*.{ts,tsx}: Use explicit type annotations for function parameters and return types in TypeScript".Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 28 - 29, Add explicit TypeScript return types for the component and its submit handler: annotate OnboardingForm to return JSX.Element (e.g., function OnboardingForm(): JSX.Element) and annotate onSubmit with an explicit signature and return type (include its parameter types and use Promise<void> if async or void if synchronous). Update the onSubmit parameter types to concrete types instead of relying on inference so both OnboardingForm and onSubmit have explicit parameter and return type annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/onboarding/onboarding-form.tsx`:
- Around line 67-72: The Organization Name label and the dynamic member email
inputs lack programmatic association with their inputs; update the Input
component usage for organizationName (the call using
register('organizationName')) to include an explicit id (e.g.,
organizationNameId) and set the <label> htmlFor to that id, add aria-invalid
when errors.organizationName exists, and have the error <span> referenced via
aria-describedby; for the dynamic member email inputs (the inputs created around
lines 79-82) generate stable unique ids per member (e.g., memberEmail-{index} or
using the member.id), set each label htmlFor to that id, pass the id into the
Input, add aria-invalid when the corresponding errors entry exists, and link the
error message with aria-describedby so screen readers announce validation
messages.
- Around line 59-62: The onSubmit handler currently logs raw
OnboardingFormValues (including invited member emails) via console.log(data);
remove or replace this to avoid leaking PII: either remove the console.log
entirely or log a sanitized summary (e.g., only counts or masked values)
instead. Update the onSubmit function (and any helper used there) to
strip/redact fields like invited members, emails, and other personally
identifying fields from the payload before any client-side logging, keeping the
original data intact for submission but never logging raw PII.
---
Outside diff comments:
In `@studio/src/pages/_app.tsx`:
- Around line 42-57: The early return when posthog.__loaded is truthy prevents
registering the routeChangeComplete listener, so change the logic to only skip
calling posthog.init when posthog.__loaded is true but always register the
handleRouteChange listener and its cleanup; i.e., replace the early return with
a conditional around posthog.init (call posthog.init(...) only if
!posthog.__loaded), then define handleRouteChange and call
Router.events.on('routeChangeComplete', handleRouteChange) and
Router.events.off(...) in the effect cleanup so $pageview is always captured
even when posthog is already loaded.
---
Duplicate comments:
In `@studio/src/components/onboarding/onboarding-form.tsx`:
- Around line 134-136: The Skip button in onboarding-form.tsx currently calls
router.push(`/${org?.slug}/graphs`) and can navigate to /undefined/graphs if org
or org.slug is not ready; update the onClick handler (and/or Button props) to
guard against missing org.slug by only calling router.push when org?.slug is
truthy, and otherwise either disable the Button or navigate to a safe fallback
(e.g., '/') to avoid constructing an invalid path.
---
Nitpick comments:
In `@studio/src/components/onboarding/onboarding-form.tsx`:
- Around line 28-29: Add explicit TypeScript return types for the component and
its submit handler: annotate OnboardingForm to return JSX.Element (e.g.,
function OnboardingForm(): JSX.Element) and annotate onSubmit with an explicit
signature and return type (include its parameter types and use Promise<void> if
async or void if synchronous). Update the onSubmit parameter types to concrete
types instead of relying on inference so both OnboardingForm and onSubmit have
explicit parameter and return type annotations.
In `@studio/src/components/onboarding/stepper.tsx`:
- Around line 15-49: The Stepper function is missing an explicit return type;
update the Stepper signature (which currently takes StepperProps) to include an
explicit React return type such as JSX.Element or React.ReactElement (e.g.,
change "export function Stepper({ steps, currentStep, className }:
StepperProps)" to include ": JSX.Element") so the Stepper component has a clear
TypeScript-annotated return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a42b6904-e941-477e-a876-5904960107bb
📒 Files selected for processing (19)
studio/README.mdstudio/next.config.mjsstudio/src/components/app-provider.tsxstudio/src/components/feature-flag-provider.tsxstudio/src/components/layout/title-layout.tsxstudio/src/components/onboarding/onboarding-form.tsxstudio/src/components/onboarding/onboarding-layout.tsxstudio/src/components/onboarding/onboarding-provider.tsxstudio/src/components/onboarding/step-1-welcome.tsxstudio/src/components/onboarding/step-2-federation.tsxstudio/src/components/onboarding/step-3-create-graph.tsxstudio/src/components/onboarding/step-4-run-services.tsxstudio/src/components/onboarding/stepper.tsxstudio/src/lib/form-schemas.tsstudio/src/lib/track.tsstudio/src/pages/[organizationSlug]/members.tsxstudio/src/pages/[organizationSlug]/settings.tsxstudio/src/pages/_app.tsxstudio/src/pages/onboarding/index.tsx
✅ Files skipped from review due to trivial changes (9)
- studio/src/components/onboarding/step-4-run-services.tsx
- studio/src/components/onboarding/step-1-welcome.tsx
- studio/README.md
- studio/src/components/onboarding/step-3-create-graph.tsx
- studio/src/components/layout/title-layout.tsx
- studio/src/pages/[organizationSlug]/settings.tsx
- studio/src/lib/form-schemas.ts
- studio/src/components/onboarding/step-2-federation.tsx
- studio/next.config.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- studio/src/pages/[organizationSlug]/members.tsx
- studio/src/components/onboarding/onboarding-layout.tsx
- studio/src/components/app-provider.tsx
- studio/src/components/onboarding/onboarding-provider.tsx
- studio/src/components/feature-flag-provider.tsx
- studio/src/pages/onboarding/index.tsx
Comment on lines
+59
to
+62
| const onSubmit = (data: OnboardingFormValues) => { | ||
| // TODO: wire up submission | ||
| console.log(data); | ||
| }; |
Contributor
There was a problem hiding this comment.
Remove PII from client-side logging before merge.
console.log(data) includes invited member emails and can leak sensitive data in browser logs.
Proposed fix
- const onSubmit = (data: OnboardingFormValues) => {
+ const onSubmit = (data: OnboardingFormValues): void => {
// TODO: wire up submission
- console.log(data);
+ // Avoid logging form payloads containing member emails.
+ if (process.env.NODE_ENV === 'development') {
+ console.debug('Onboarding form submitted');
+ }
};📝 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.
Suggested change
| const onSubmit = (data: OnboardingFormValues) => { | |
| // TODO: wire up submission | |
| console.log(data); | |
| }; | |
| const onSubmit = (data: OnboardingFormValues): void => { | |
| // TODO: wire up submission | |
| // Avoid logging form payloads containing member emails. | |
| if (process.env.NODE_ENV === 'development') { | |
| console.debug('Onboarding form submitted'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 59 - 62,
The onSubmit handler currently logs raw OnboardingFormValues (including invited
member emails) via console.log(data); remove or replace this to avoid leaking
PII: either remove the console.log entirely or log a sanitized summary (e.g.,
only counts or masked values) instead. Update the onSubmit function (and any
helper used there) to strip/redact fields like invited members, emails, and
other personally identifying fields from the payload before any client-side
logging, keeping the original data intact for submission but never logging raw
PII.
Comment on lines
+67
to
+72
| <label className="text-sm font-medium">Organization Name</label> | ||
| <p className="text-sm text-muted-foreground"> | ||
| This is your organization name. Feel free to keep it or change it. | ||
| </p> | ||
| <Input placeholder="Acme Inc." className="max-w-md" {...register('organizationName')} /> | ||
| {errors.organizationName && <span className="text-sm text-destructive">{errors.organizationName.message}</span>} |
Contributor
There was a problem hiding this comment.
Associate labels with inputs for screen-reader accessibility.
The organization input and dynamic member email inputs are not programmatically labeled, which reduces accessibility.
Proposed fix
-import { useEffect } from 'react';
+import { useEffect } from 'react';
...
- <label className="text-sm font-medium">Organization Name</label>
+ <label htmlFor="organizationName" className="text-sm font-medium">Organization Name</label>
...
- <Input placeholder="Acme Inc." className="max-w-md" {...register('organizationName')} />
+ <Input id="organizationName" placeholder="Acme Inc." className="max-w-md" {...register('organizationName')} />
...
- <Input placeholder="janedoe@example.com" className="max-w-md" {...register(`members.${index}.email`)} />
+ <Input
+ aria-label={`Member email ${index + 1}`}
+ placeholder="janedoe@example.com"
+ className="max-w-md"
+ {...register(`members.${index}.email`)}
+ />Also applies to: 79-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@studio/src/components/onboarding/onboarding-form.tsx` around lines 67 - 72,
The Organization Name label and the dynamic member email inputs lack
programmatic association with their inputs; update the Input component usage for
organizationName (the call using register('organizationName')) to include an
explicit id (e.g., organizationNameId) and set the <label> htmlFor to that id,
add aria-invalid when errors.organizationName exists, and have the error <span>
referenced via aria-describedby; for the dynamic member email inputs (the inputs
created around lines 79-82) generate stable unique ids per member (e.g.,
memberEmail-{index} or using the member.id), set each label htmlFor to that id,
pass the id into the Input, add aria-invalid when the corresponding errors entry
exists, and link the error message with aria-describedby so screen readers
announce validation messages.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Refactor
Style
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.