fix: use server fetched user for all sub pages in /getting-started#23064
fix: use server fetched user for all sub pages in /getting-started#23064
Conversation
WalkthroughOnboarding components were refactored to accept a user prop instead of fetching the current user internally. ConnectedVideoStep, UserProfile, and UserSettings now take user: RouterOutputs["viewer"]["me"]["get"]. ConnectedVideoStep parses metadata from user.metadata and removes useMeQuery; loading is handled via isPending and a useEffect-based state update. UserProfile initializes form fields from the passed user, adds a dedicated avatar update mutation, and preserves the existing bio/onboarding mutation. UserSettings no longer uses a suspense query. The onboarding page now passes user={user} into these child components. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/13/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (2)
50-55: Avoid setting state during render; move to useEffect.Calling setAnyInstalledVideoApps in render is an anti-pattern and can cause unnecessary re-renders. Derive and set it via useEffect.
-import { useState } from "react"; +import { useEffect, useState } from "react"; @@ - const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( - (item) => item.userCredentialIds.length > 0 - ); - if (hasAnyInstalledVideoApps) { - setAnyInstalledVideoApps(true); - } + const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( + (item) => item.userCredentialIds.length > 0 + ); + useEffect(() => { + setAnyInstalledVideoApps(Boolean(hasAnyInstalledVideoApps)); + }, [hasAnyInstalledVideoApps, setAnyInstalledVideoApps]);Also applies to: 2-2
57-63: Don’t block UI if metadata parsing fails; use a safe fallback.Returning a loader on parse failure can stall the step if metadata is absent or malformed. Defaulting to {} is safer and keeps the UX unblocked.
- const result = userMetadata.safeParse(user.metadata); - if (!result.success) { - return <StepConnectionLoader />; - } - const { data: metadata } = result; - const defaultConferencingApp = metadata?.defaultConferencingApp?.appSlug; + const parsed = userMetadata.safeParse(user?.metadata ?? {}); + const metadata = parsed.success ? parsed.data : {}; + const defaultConferencingApp = metadata?.defaultConferencingApp?.appSlug;apps/web/components/getting-started/steps-views/UserProfile.tsx (1)
60-64: Await mutations correctly: use mutateAsync within Promise.all.createEventType.mutate does not return a Promise; your Promise.all will resolve immediately, racing the redirect. Use mutateAsync to properly await creation.
- await Promise.all( - DEFAULT_EVENT_TYPES.map(async (event) => { - return createEventType.mutate(event); - }) - ); + await Promise.all(DEFAULT_EVENT_TYPES.map((event) => createEventType.mutateAsync(event)));
🧹 Nitpick comments (7)
apps/web/components/getting-started/steps-views/UserSettings.tsx (3)
29-29: Destructure hideUsername for consistency and clarity.Minor readability nit: since you already destructure nextStep and user, also destructure hideUsername so you’re not mixing props and destructured variables.
- const { nextStep, user } = props; + const { nextStep, user, hideUsername } = props; @@ - {!props.hideUsername && <UsernameAvailabilityField />} + {!hideUsername && <UsernameAvailabilityField />}Also applies to: 77-77
34-40: Trim name input before validation to prevent whitespace-only names.Avoids “ ” being accepted and keeps validation UX tight.
const userSettingsSchema = z.object({ name: z - .string() + .string() + .trim() .min(1) .max(FULL_NAME_LENGTH_MAX_LIMIT, { message: t("max_limit_allowed_hint", { limit: FULL_NAME_LENGTH_MAX_LIMIT }), }), });
113-116: Avoid forcing lowercase on localized times.Lowercasing the formatted time can degrade localization (e.g., AM/PM or locale-specific casing). Prefer the formatter’s output as-is.
- {t("current_time")} {dayjs().tz(selectedTimeZone).format("LT").toString().toLowerCase()} + {t("current_time")} {dayjs().tz(selectedTimeZone).format("LT").toString()}apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (1)
69-71: Use a stable key (slug) instead of name.Names can change and aren’t guaranteed unique. Slugs are more stable for React keys.
- <li key={item.name}> + <li key={item.slug}>apps/web/components/getting-started/steps-views/UserProfile.tsx (2)
37-37: Potential avatar field mismatch (avatar vs avatarUrl).The mutation returns data.avatarUrl and updateProfile takes avatarUrl, but initial state reads user?.avatar. This can cause the avatar to not show until after an update. Prefer avatarUrl with a backward-compatible fallback.
- const [imageSrc, setImageSrc] = useState<string>(user?.avatar || ""); + const [imageSrc, setImageSrc] = useState<string>(user?.avatarUrl || user?.avatar || "");If the user object only contains avatarUrl consistently, simplify to user?.avatarUrl.
120-128: Remove placeholder on hidden input and fix a class typo.
- Hidden inputs don’t need placeholders; also violates the “localize all text” guideline.
- Typo in class: focus:ring-empthasis → focus:ring-emphasis.
- <input + <input ref={avatarRef} type="hidden" name="avatar" id="avatar" - placeholder="URL" - className="border-default focus:ring-empthasis mt-1 block w-full rounded-sm border px-3 py-2 text-sm focus:border-gray-800 focus:outline-none" + className="border-default focus:ring-emphasis mt-1 block w-full rounded-sm border px-3 py-2 text-sm focus:border-gray-800 focus:outline-none" defaultValue={imageSrc} />apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
151-152: Localize the fallback “Undefined title”.Per guidelines, avoid hardcoded UI strings; use t() and add a translation key.
- {headers[currentStepIndex]?.title || "Undefined title"} + {headers[currentStepIndex]?.title ?? t("undefined_title")}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx(4 hunks)apps/web/components/getting-started/steps-views/UserProfile.tsx(2 hunks)apps/web/components/getting-started/steps-views/UserSettings.tsx(2 hunks)apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/components/getting-started/steps-views/UserSettings.tsxapps/web/modules/getting-started/[[...step]]/onboarding-view.tsxapps/web/components/getting-started/steps-views/UserProfile.tsxapps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/components/getting-started/steps-views/UserSettings.tsxapps/web/modules/getting-started/[[...step]]/onboarding-view.tsxapps/web/components/getting-started/steps-views/UserProfile.tsxapps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx
🧬 Code Graph Analysis (2)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (3)
apps/web/components/getting-started/steps-views/UserSettings.tsx (1)
UserSettings(130-130)scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (1)
ConnectedVideoStep(113-113)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (2)
packages/platform/libraries/index.ts (1)
userMetadata(63-63)scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Detect changes
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (10)
apps/web/components/getting-started/steps-views/UserSettings.tsx (2)
16-16: Prop typing aligned with server-fetched user. LGTM.Importing RouterOutputs for the user prop matches the PR goal and keeps types in sync with TRPC.
25-26: Passing user via props is the right direction.This enforces a single source of truth for user data across steps and removes in-component fetching.
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (3)
16-20: Prop signature expansion (user) matches the PR objective.Using a server-provided user prop removes local fetching and keeps onboarding consistent.
22-30: Inner component typing updated correctly.Accepting user here keeps the data flow explicit and testable.
91-97: Prop passthrough for user looks good.ConnectedVideoStep now cleanly delegates user to the inner component.
apps/web/components/getting-started/steps-views/UserProfile.tsx (2)
13-27: Types and props updated correctly to accept server user.RouterOutputs typing and the new UserProfileProps align with the PR’s intent.
29-35: Form default values correctly sourced from props.Initializing bio from user keeps the form hydrated without extra fetches.
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (3)
165-166: UserSettings now receives server user. LGTM.This aligns the step with the unified data flow for onboarding.
172-173: ConnectedVideoStep receives server user. LGTM.Keeps the video step consistent with the refactor and removes redundant fetching.
178-178: UserProfile receives server user. LGTM.Matches the refactor and avoids extra suspense/loading.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (2)
58-63: Avoid infinite loader on invalid metadata; fall back safelyIf userMetadata.safeParse fails, returning a loader causes an indefinite spinner with no recovery path. Gracefully fall back to no default app instead.
Apply this diff:
- const result = userMetadata.safeParse(user.metadata); - if (!result.success) { - return <StepConnectionLoader />; - } - const { data: metadata } = result; - const defaultConferencingApp = metadata?.defaultConferencingApp?.appSlug; + const parsed = userMetadata.safeParse(user.metadata); + const defaultConferencingApp = parsed.success + ? parsed.data.defaultConferencingApp?.appSlug + : undefined;
70-71: Use a stable unique keyNames are not guaranteed unique; use slug which is the canonical identifier.
Apply this diff:
- <li key={item.name}> + <li key={item.slug}> {item.name && item.logo && (
🧹 Nitpick comments (2)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (2)
31-44: Set parent state via useQuery onSuccess; drop derived state + effectYou can avoid the extra render and the derived state effect by using TRPC’s onSuccess to compute and set hasAnyInstalledVideoApps. This keeps the state update co-located with the data fetch and removes the need for the useEffect.
Apply this diff:
- const { data: queryConnectedVideoApps, isPending } = trpc.viewer.apps.integrations.useQuery({ - variant: "conferencing", - onlyInstalled: false, - - /** - * Both props together sort by most popular first, then by installed first. - * So, installed apps are always shown at the top, followed by remaining apps sorted by descending popularity. - * - * This is done because there could be not so popular app already installed by the admin(e.g. through Delegation Credential) - * and we want to show it at the top so that user can set it as default if he wants to. - */ - sortByMostPopular: true, - sortByInstalledFirst: true, - }); + const { data: queryConnectedVideoApps, isPending, isError } = + trpc.viewer.apps.integrations.useQuery( + { + variant: "conferencing", + onlyInstalled: false, + /** + * Both props together sort by most popular first, then by installed first. + * So, installed apps are always shown at the top, followed by remaining apps sorted by descending popularity. + * + * This is done because there could be not so popular app already installed by the admin(e.g. through Delegation Credential) + * and we want to show it at the top so that user can set it as default if he wants to. + */ + sortByMostPopular: true, + sortByInstalledFirst: true, + }, + { + onSuccess: (res) => { + const anyInstalled = res?.items?.some((i) => i.userCredentialIds.length > 0) ?? false; + setAnyInstalledVideoApps(anyInstalled); + }, + } + ); - useEffect(() => { - setAnyInstalledVideoApps(Boolean(hasAnyInstalledVideoApps)); - }, [hasAnyInstalledVideoApps, setAnyInstalledVideoApps]);Follow-up: once you apply this, remove the now-unused hasAnyInstalledVideoApps computation to satisfy no-unused-vars rules.
Also applies to: 50-53
68-68: Avoid magic string for hidden app; centralize slug(s)Hardcoding "daily-video" makes this brittle. Use a shared constant/set so future additions are one-line changes.
Apply this diff:
- if (item.slug === "daily-video") return null; // we dont want to show daily here as it is installed by default + if (HIDDEN_CONFERENCING_APP_SLUGS.has(item.slug)) return null; // hide apps installed by defaultAdd this helper near the top of the file:
// Slugs of apps we intentionally hide from this list (e.g., installed by default) const HIDDEN_CONFERENCING_APP_SLUGS = new Set(["daily-video"]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (2)
92-98: Prop contract change: verify user is always non-nullConnectedVideoStep now requires user. Confirm the onboarding route guarantees a non-null user in all cases (including SSR and edge cases), or widen the type to allow null and guard render.
If there’s any chance of null:
- Make user?: RouterOutputs["viewer"]["me"]["get"] | null
- Short-circuit render or show a loader/error until user is available.
Also applies to: 105-109
108-109: Localization: good use of t()Button copy is localized correctly.
| if (isPending) { | ||
| return <StepConnectionLoader />; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle query error state (not just pending)
Right now a failed integrations query yields an empty list with no UX feedback. Provide a minimal error state so users aren’t stuck without context.
Apply this diff:
if (isPending) {
return <StepConnectionLoader />;
}
+ if (isError) {
+ return <div>{t("failed_to_load_video_integrations")}</div>
+ }Note: Keep using t() for localization as shown.
📝 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.
| if (isPending) { | |
| return <StepConnectionLoader />; | |
| } | |
| if (isPending) { | |
| return <StepConnectionLoader />; | |
| } | |
| if (isError) { | |
| return <div>{t("failed_to_load_video_integrations")}</div> | |
| } |
🤖 Prompt for AI Agents
In apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx around
lines 54 to 56, the component only handles the isPending state and ignores query
errors; update it to also handle isError (or error) from the integrations query
by returning a minimal error UI when the query fails — use t('...') for the
localized error message, include a brief descriptive line like
t('gettingStarted.integrationsError') and a Retry button that calls refetch (or
the query's retry method) so users get feedback and can retry the request.
E2E results are ready! |
…23064) * use server fetched user for all sub pages * Wrap setAnyInstalledVideoApps call in useEffect --------- Co-authored-by: Alex van Andel <me@alexvanandel.com>
3 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist