Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a complete user onboarding feature to the platform. It introduces new RPC endpoints for bulk user invitations and multi-step onboarding management (get/create/finish), along with corresponding protobuf definitions, database schema with migration, backend handlers, and a frontend multi-step onboarding flow with PostHog feature flagging integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 8
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)
43-59:⚠️ Potential issue | 🟠 MajorPostHog init guard prevents route listener registration and allows undefined-key init.
On line 43, the early return when
posthog.__loadedis true prevents the route listener setup (lines 53–54) and its cleanup function from being registered. If PostHog is initialized before this effect (or re-initialized), pageview tracking will be silently skipped.Additionally, line 45 uses a non-null assertion on
NEXT_PUBLIC_POSTHOG_KEYwithout validation, allowingposthog.init()to execute with an undefined key. While line 76 disables the feature flag provider when the key is missing, the library initialization still occurs with an invalid value.Per PostHog documentation and best practices, initialization should happen once globally with key validation, not conditionally in a component effect with undocumented workarounds. The internal
posthog.__loadedproperty is not a stable public API.💡 Proposed fix
useEffect(() => { + const posthogKey = process.env.NEXT_PUBLIC_POSTHOG_KEY; + if (!posthogKey) return; + - if (posthog.__loaded) return; + 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', - }); + 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 43 - 59, The effect currently early-returns when posthog.__loaded is true which skips registering the route listener and cleanup, and it calls posthog.init with an unchecked NEXT_PUBLIC_POSTHOG_KEY; fix by removing reliance on the internal posthog.__loaded flag, validate process.env.NEXT_PUBLIC_POSTHOG_KEY before calling posthog.init (skip init if missing), always register the Router.events.on('routeChangeComplete', handleRouteChange) and corresponding Router.events.off in the cleanup regardless of init state, and call posthog.init only once (e.g., guard with your own module-level initialized boolean or move init to a top-level bootstrap) so that posthog.init, posthog.__loaded checks, handleRouteChange, and Router.events.on/off behave correctly.
🧹 Nitpick comments (6)
studio/src/hooks/use-onboarding.ts (1)
4-4: Add an explicit return type to this exported hook.The repo’s TypeScript rules require explicit function return types; this public hook currently relies on inference.
As per coding guidelines "Use explicit type annotations for function parameters and return types in TypeScript".✏️ Suggested annotation
-import { useContext } from 'react'; +import { ContextType, useContext } from 'react'; import { OnboardingContext } from '@/components/onboarding/onboarding-provider'; -export const useOnboarding = () => useContext(OnboardingContext); +export const useOnboarding = (): ContextType<typeof OnboardingContext> => useContext(OnboardingContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/hooks/use-onboarding.ts` at line 4, The hook useOnboarding currently relies on inference; add an explicit return type by importing or declaring the context value type (e.g., OnboardingContextValue or OnboardingContextType) and annotate the hook as (): OnboardingContextValue => useContext(OnboardingContext); ensure you import the type (using `import type { ... }`) if it lives alongside OnboardingContext and update the export signature for useOnboarding to use that explicit return type.controlplane/src/core/bufservices/onboarding/finishOnboarding.ts (1)
41-47: Non-null assertion relies on repository implementation detail.The
!assertion ononboarding.finishedAtassumes thefinish()method always sets this field. While currently true, a defensive approach would handle the edge case explicitly.♻️ Defensive alternative
return { response: { code: EnumStatusCode.OK, }, federatedGraphsCount, - finishedAt: onboarding.finishedAt!.toISOString(), + finishedAt: onboarding.finishedAt?.toISOString() ?? new Date().toISOString(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/onboarding/finishOnboarding.ts` around lines 41 - 47, The returned value uses a non-null assertion onboarding.finishedAt! which assumes finish() always sets finishedAt; change to defensively handle a missing finishedAt by checking onboarding.finishedAt (or awaiting/ensuring finish() completed) and returning a safe value or error: e.g., if onboarding.finishedAt is null/undefined, call finish() or set finishedAt = new Date() or return an appropriate EnumStatusCode/error response; update the return in finishOnboarding (reference onboarding.finishedAt and finish()) to avoid the `!` and explicitly handle the null case.studio/src/components/onboarding/onboarding-provider.tsx (2)
14-17: Consider usinginterfacefor object shapes.Per coding guidelines, prefer interfaces over type aliases for object shapes in TypeScript. This would be consistent with
OnboardingStateon line 19.♻️ Suggested change
-type Onboarding = { +interface Onboarding { finishedAt?: Date; federatedGraphsCount: number; -}; +}As per coding guidelines: "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 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 14 - 17, Replace the type alias Onboarding with an interface to match project guidelines and the existing OnboardingState; change the declaration from "type Onboarding = { finishedAt?: Date; federatedGraphsCount: number; }" to an interface named Onboarding with the same members, keep all references/usages of Onboarding and the separate OnboardingState unchanged, and ensure any imports/exports still resolve after the rename.
71-83: Redundant condition inenabledcomputation.The trailing
&& onboardingFlagis redundant sinceonboardingFlag.enabledalready impliesonboardingFlagis truthy.♻️ Suggested simplification
const value = useMemo( () => ({ onboarding, - enabled: Boolean(onboardingFlag.enabled && featureFlagStatus === 'success' && onboardingFlag), + enabled: Boolean(onboardingFlag.enabled && featureFlagStatus === 'success'), setOnboarding,🤖 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 71 - 83, The computed `enabled` property in the `value` memo includes a redundant `&& onboardingFlag` because checking `onboardingFlag.enabled` already guarantees `onboardingFlag` is truthy; update the object returned in the `useMemo` (the `value` constant) to remove the trailing `&& onboardingFlag` so `enabled` becomes `Boolean(onboardingFlag.enabled && featureFlagStatus === 'success')`, leaving the rest of the `useMemo` and its dependency array unchanged.studio/src/components/posthog-feature-flag-provider.tsx (2)
33-36: Consider usinginterfacefor object shapes.Per coding guidelines, prefer interfaces over type aliases for object shapes. The
PostHogFeatureFlagStateinterface is correctly usinginterface, butinitialStatecould have an explicit type annotation for clarity.As per coding guidelines: "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/posthog-feature-flag-provider.tsx` around lines 33 - 36, Add an explicit interface-based type annotation to the initialState variable to match the PostHogFeatureFlagState interface: ensure the declaration of initialState uses the PostHogFeatureFlagState interface (e.g., const initialState: PostHogFeatureFlagState = { status: 'idle', onboarding: { enabled: false } }), so the object shape is clearly tied to the interface; verify PostHogFeatureFlagState is declared as an interface and update the initialState declaration (variable name: initialState, type: PostHogFeatureFlagState) accordingly.
56-62: Full-screen loader blocks app while PostHog loads.The provider shows a blocking loader until the feature flag resolves. If PostHog is slow or unreachable, the entire application remains blocked. Consider adding a timeout to fall back to
disabledstate after a reasonable delay (e.g., 3-5 seconds).♻️ Example timeout approach
export const PostHogFeatureFlagProvider = ({ children, disabled }: { children: ReactNode; disabled: boolean }) => { const onboardingFlag = useFeatureFlagEnabled('cosmo-onboarding-v1'); const [state, dispatch] = useReducer(postHogFeatureFlagReducer, initialState); useEffect(() => { if (disabled) { dispatch({ type: 'DISABLED' }); - } else if (onboardingFlag === undefined) { + return; + } + + if (onboardingFlag === undefined) { dispatch({ type: 'LOADING' }); + const timeout = setTimeout(() => { + dispatch({ type: 'DISABLED' }); + }, 5000); + return () => clearTimeout(timeout); } else { dispatch({ type: 'LOADED', onboardingEnabled: onboardingFlag }); } }, [onboardingFlag, disabled]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/posthog-feature-flag-provider.tsx` around lines 56 - 62, The full-screen Loader blocking behavior comes from the initial conditional that waits for state.status to be 'success' or 'disabled'; add a timeout in the component (useEffect) that after ~3000–5000ms sets state.status to 'disabled' if it is still pending so the app can render; implement by starting a timer in useEffect when component mounts or when state.status === 'pending', call the same state updater used in this component to set status = 'disabled' if still pending, clear the timeout when state.status transitions to 'success' or 'disabled' or on unmount, and keep the existing render branch that shows the <Loader /> when loading so slow/unreachable PostHog falls back safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/bufservices/user/inviteUsers.ts`:
- Around line 47-68: The current mapping treats any rejection (including
auditLogRepo.addAuditLog failures) as a failed invitation; change the per-email
flow so inviteUser() errors are the only failures that populate
invitationErrors: call await service.inviteUser(...) first, then call
auditLogRepo.addAuditLog(...) inside its own try/catch that swallows or logs
audit errors but does not rethrow; update the Promise.allSettled handling so
that results reflect only inviteUser failures (reference inviteUser,
auditLogRepo.addAuditLog, results, and invitationErrors) and ensure audit
failures are recorded separately or logged without failing the invitation
promise.
In `@studio/src/components/app-provider.tsx`:
- Around line 188-191: The redirect guard mistakenly tests router.pathname (a
route template) with a regex expecting numeric steps, so onboarding routes like
`/onboarding/1` still redirect; update the test to use the actual URL
(router.asPath or router.asPath || router.asPath) or perform a startsWith check
against '/onboarding' (e.g., use router.asPath and replace the regex test with a
check like router.asPath.startsWith('/onboarding') or apply the same regex to
router.asPath) in the condition that currently references router.pathname and
the onboarding regex to ensure onboarding step URLs are excluded from the
redirect.
In `@studio/src/components/onboarding/step-1.tsx`:
- Around line 56-61: The onClick handler in onboarding Step1
(studio/src/components/onboarding/step-1.tsx) currently persists hardcoded
placeholder values via the mutate({ slack: true, email: false }) call; replace
this with real form state or avoid persisting until wiring is complete. Update
the onClick to read the actual form values (e.g., slackEnabled/emailEnabled
fields from your form state or hook used in the component) and pass those to
mutate, or if the form isn't ready, remove the mutate call and/or send
null/undefined values and add a TODO/follow-up issue to wire the form; ensure
the change targets the onClick handler and the mutate invocation in Step1
component.
- Around line 32-35: The toast fallback is unreliable because
error.details.toString() can be empty or undefined; update the onError handler
(the onError callback that calls toast) to safely stringify and trim
error.details and fallback to other sources if empty—e.g., use
String(error.details ?? error.message ?? '').trim() and if that result is falsy
use the literal 'We had issues with storing your data. Please try again.'—ensure
you reference the existing onError callback and the toast invocation so the
toast always shows a non-empty message.
In `@studio/src/components/onboarding/step-3.tsx`:
- Around line 35-37: The onError toast in the onboarding step uses
error.details.toString() ?? 'We had issues...' which still yields an empty
string if details is blank; update the onError handler (the onError callback
where toast is called) to compute a safe message from error.details by checking
for existence and trimmed length (e.g., use error?.details, call toString(),
trim(), and fall back to the static message when empty), and pass that resulting
non-empty string into toast({ description: ... }) so the fallback message is
shown when details is missing or empty.
In `@studio/src/hooks/use-onboarding-navigation.ts`:
- Around line 41-64: The effect currently redirects to onboarding without
checking the feature flag; update the redirect guard to return early when the
`enabled` flag is falsy so users aren't redirected when `cosmo-onboarding-v1` is
disabled. Specifically, in the effect that reads `initialLoadSuccess`,
`skipped`, `data`, `initialRedirect.current` and calls `Router.replace(path)`,
add a top-level check for `if (!enabled) return;` (and ensure `enabled` is
present in the dependency array) so the redirect to `/onboarding/${currentStep
?? 1}` only runs when the feature is enabled.
In `@studio/src/lib/track.ts`:
- Around line 36-37: The top-level early return currently checks
NEXT_PUBLIC_POSTHOG_KEY and prevents the Reo identification block from running;
remove NEXT_PUBLIC_POSTHOG_KEY from that initial guard so only server-side
(typeof window === 'undefined') returns early, then wrap or gate only the
PostHog initialization code (the block that reads
process.env.NEXT_PUBLIC_POSTHOG_KEY and calls posthog.init) with the
NEXT_PUBLIC_POSTHOG_KEY check so the Reo identification block (lines 75–79) can
run independently in the browser/production environment.
In `@studio/src/pages/onboarding/`[step].tsx:
- Around line 10-22: The switch currently returns null when router.query.step is
undefined or an array, causing a blank page; normalize the raw router.query.step
(e.g., read a stepRaw from router.query.step, if Array.isArray(stepRaw) take the
first element, and if undefined default to '1') and use that normalized string
in the switch so the default case renders a deterministic fallback like <Step1
/> instead of null; update references to step in the switch to use the
normalized value (symbols to change: router.query, step variable, and the switch
that returns Step1/Step2/Step3).
---
Outside diff comments:
In `@studio/src/pages/_app.tsx`:
- Around line 43-59: The effect currently early-returns when posthog.__loaded is
true which skips registering the route listener and cleanup, and it calls
posthog.init with an unchecked NEXT_PUBLIC_POSTHOG_KEY; fix by removing reliance
on the internal posthog.__loaded flag, validate
process.env.NEXT_PUBLIC_POSTHOG_KEY before calling posthog.init (skip init if
missing), always register the Router.events.on('routeChangeComplete',
handleRouteChange) and corresponding Router.events.off in the cleanup regardless
of init state, and call posthog.init only once (e.g., guard with your own
module-level initialized boolean or move init to a top-level bootstrap) so that
posthog.init, posthog.__loaded checks, handleRouteChange, and
Router.events.on/off behave correctly.
---
Nitpick comments:
In `@controlplane/src/core/bufservices/onboarding/finishOnboarding.ts`:
- Around line 41-47: The returned value uses a non-null assertion
onboarding.finishedAt! which assumes finish() always sets finishedAt; change to
defensively handle a missing finishedAt by checking onboarding.finishedAt (or
awaiting/ensuring finish() completed) and returning a safe value or error: e.g.,
if onboarding.finishedAt is null/undefined, call finish() or set finishedAt =
new Date() or return an appropriate EnumStatusCode/error response; update the
return in finishOnboarding (reference onboarding.finishedAt and finish()) to
avoid the `!` and explicitly handle the null case.
In `@studio/src/components/onboarding/onboarding-provider.tsx`:
- Around line 14-17: Replace the type alias Onboarding with an interface to
match project guidelines and the existing OnboardingState; change the
declaration from "type Onboarding = { finishedAt?: Date; federatedGraphsCount:
number; }" to an interface named Onboarding with the same members, keep all
references/usages of Onboarding and the separate OnboardingState unchanged, and
ensure any imports/exports still resolve after the rename.
- Around line 71-83: The computed `enabled` property in the `value` memo
includes a redundant `&& onboardingFlag` because checking
`onboardingFlag.enabled` already guarantees `onboardingFlag` is truthy; update
the object returned in the `useMemo` (the `value` constant) to remove the
trailing `&& onboardingFlag` so `enabled` becomes
`Boolean(onboardingFlag.enabled && featureFlagStatus === 'success')`, leaving
the rest of the `useMemo` and its dependency array unchanged.
In `@studio/src/components/posthog-feature-flag-provider.tsx`:
- Around line 33-36: Add an explicit interface-based type annotation to the
initialState variable to match the PostHogFeatureFlagState interface: ensure the
declaration of initialState uses the PostHogFeatureFlagState interface (e.g.,
const initialState: PostHogFeatureFlagState = { status: 'idle', onboarding: {
enabled: false } }), so the object shape is clearly tied to the interface;
verify PostHogFeatureFlagState is declared as an interface and update the
initialState declaration (variable name: initialState, type:
PostHogFeatureFlagState) accordingly.
- Around line 56-62: The full-screen Loader blocking behavior comes from the
initial conditional that waits for state.status to be 'success' or 'disabled';
add a timeout in the component (useEffect) that after ~3000–5000ms sets
state.status to 'disabled' if it is still pending so the app can render;
implement by starting a timer in useEffect when component mounts or when
state.status === 'pending', call the same state updater used in this component
to set status = 'disabled' if still pending, clear the timeout when state.status
transitions to 'success' or 'disabled' or on unmount, and keep the existing
render branch that shows the <Loader /> when loading so slow/unreachable PostHog
falls back safely.
In `@studio/src/hooks/use-onboarding.ts`:
- Line 4: The hook useOnboarding currently relies on inference; add an explicit
return type by importing or declaring the context value type (e.g.,
OnboardingContextValue or OnboardingContextType) and annotate the hook as ():
OnboardingContextValue => useContext(OnboardingContext); ensure you import the
type (using `import type { ... }`) if it lives alongside OnboardingContext and
update the export signature for useOnboarding to use that explicit 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: ebcf66cc-aeaa-42e3-8427-179838d24c87
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (34)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/migrations/0137_icy_sasquatch.sqlcontrolplane/migrations/meta/0137_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/onboarding/createOnboarding.tscontrolplane/src/core/bufservices/onboarding/finishOnboarding.tscontrolplane/src/core/bufservices/onboarding/getOnboarding.tscontrolplane/src/core/bufservices/user/inviteUsers.tscontrolplane/src/core/repositories/OnboardingRepository.tscontrolplane/src/db/schema.tscontrolplane/test/invite-users.test.tscontrolplane/test/onboarding/onboarding.test.tsproto/wg/cosmo/platform/v1/platform.protostudio/README.mdstudio/next.config.mjsstudio/src/components/app-provider.tsxstudio/src/components/dashboard/workspace-provider.tsxstudio/src/components/federatedgraphs-cards.tsxstudio/src/components/layout/onboarding-layout.tsxstudio/src/components/onboarding/onboarding-provider.tsxstudio/src/components/onboarding/step-1.tsxstudio/src/components/onboarding/step-2.tsxstudio/src/components/onboarding/step-3.tsxstudio/src/components/posthog-feature-flag-provider.tsxstudio/src/hooks/use-onboarding-navigation.tsstudio/src/hooks/use-onboarding.tsstudio/src/hooks/use-session-storage.tsstudio/src/lib/track.tsstudio/src/pages/_app.tsxstudio/src/pages/onboarding/[step].tsxstudio/src/pages/onboarding/index.tsx
|
@CodeRabbit pause |
✅ Actions performedReviews paused. |
e4bcd44 to
9236b46
Compare
9236b46 to
723e9e8
Compare
|
@CodeRabbit stop |
|
🧠 Learnings used✅ Actions performedReviews paused. |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
…p-to-live-metrics-topic
Note
This is a topic branch/PR which accumulates code from other PRs.
The code on this branch is reviewed already.
Summary by CodeRabbit
New Features
Tests
Documentation
This PR contains changes from PR stack. All changes on this branch are already reviewed.
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.
How does the onboarding work?
Linear contains most of the information but in a nutshell: onboarding in Studio has companion CLI command
wgc demo. The CLI is needed in order to create the demo federated graph and its subgraph and runs the router.Onboarding is triggered based on two conditions:
This means we do not allow simultaneous onboarding for multiple people in the same organization. The intention of the onboarding is for user to run it in the organization we create for them after the signup.
The onboarding flow is triggered when user signs up. The
wgc democommand can also open the onboarding UI in the web browser but it's there just as a helping guide.User can skip the onboarding at any time on any step in the web wizard. We track the step and whether they've skipped in the browser storage. The user can always resume onboarding by visiting the index page (graph index) where new empty state resides. This empty state links to onboarding web wizard. Once they create federated graph (either demo or any other one), we don't allow to resume the onboarding.
Once the onboarding is finished, user is not able to restart the onboarding (due to condition outlined above).
wgc demooffers you to delete the demo federated graph, so you can use that to drop the federated graph and subgraphs to restart the onboarding.After the onboarding is finished, we show a UI to offer the user to invite other users and read our documentation.
How to test?
If you want to test the whole functionality end to end, it is a bit more involved. The subgraphs are implemented as gRPC plugins, which means we need to connect to staging plugin registry to publish them and pull them to the router.
make infra-downenable-staging-registry.sh(ping me on Slack to get it). This will make changes to compose files and update your.envfiles.make infra-upcosmo-onboarding-v1flag and add your testing email which you will use for sign up:make start-cp&make start-studio). Go tolocalhost:3000, sign up with the email you specified in PostHogpnpm wgc demoto run the demo command. The demo command will display log file once plugins start to get published and when router starts.Release process
We will first release it with just the flag enabled on our testing accounts to do further testing in production. After we're sure there are no issues, we can discuss gradual release to subset of users.
Stack: