fix: Crash of onboarding connected-video page#23047
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes ✨ 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 foundation 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. "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. "Add ready-for-e2e label" took an action on this PR • (08/13/25)1 label was added to this PR based on Keith Williams's automation. |
|
|
||
| const ConnectedVideoStep = (props: ConnectedAppStepProps) => { | ||
| const { nextStep, isPageLoading } = props; | ||
| const ConnectedVideoStepInner = ({ |
There was a problem hiding this comment.
This nested component allows for refactoring in the early return pattern, making it more readable and more predictable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (4)
21-27: Consider more specific prop naming for clarityThe prop name
setAnyInstalledVideoAppscould be more specific. Consider renaming tosetHasInstalledVideoAppsfor better readability and consistency with the state variable name.
67-67: Remove unnecessary double space in classNameThere's a double space between "bg-default" and "border-subtle" in the className string.
- <List className="bg-default border-subtle divide-subtle scroll-bar mx-1 max-h-[45vh] divide-y !overflow-y-scroll rounded-md border p-0 sm:mx-0"> + <List className="bg-default border-subtle divide-subtle scroll-bar mx-1 max-h-[45vh] divide-y !overflow-y-scroll rounded-md border p-0 sm:mx-0">
83-83: Simplify defaultInstall logic with nullish coalescingThe defaultInstall logic can be simplified using the nullish coalescing operator for better readability.
- defaultInstall={!defaultConferencingApp && item.appData?.location?.linkType === "dynamic"} + defaultInstall={!defaultConferencingApp && item.appData?.location?.linkType === "dynamic" ?? false}
28-91: Consider memoizing expensive computationsThe component re-renders on every state change and recalculates
hasAnyInstalledVideoAppsand filters items. Consider usinguseMemoto optimize performance, especially with large lists of video apps.Add at the top of the component:
import { useMemo } from "react";Then update the logic:
+ const filteredVideoApps = useMemo(() => + queryConnectedVideoApps?.items?.filter((item) => item.slug !== "daily-video") ?? [], + [queryConnectedVideoApps?.items] + ); + + const hasAnyInstalledVideoApps = useMemo(() => + filteredVideoApps.some((item) => item.userCredentialIds.length > 0), + [filteredVideoApps] + ); - const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( - (item) => item.userCredentialIds.length > 0 - );And update the render logic:
- {queryConnectedVideoApps?.items && - queryConnectedVideoApps?.items.map((item) => { - if (item.slug === "daily-video") return null; + {filteredVideoApps.map((item) => {
📜 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(3 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
🧬 Code Graph Analysis (1)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (4)
packages/trpc/react/hooks/useMeQuery.ts (1)
useMeQuery(3-11)packages/platform/libraries/index.ts (1)
userMetadata(63-63)packages/ui/components/list/List.tsx (1)
List(15-31)apps/web/components/getting-started/components/AppConnectionItem.tsx (1)
AppConnectionItem(139-139)
⏰ 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: Detect changes
🔇 Additional comments (1)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (1)
93-114: Good architectural split for separation of concernsThe refactor to split data fetching logic (
ConnectedVideoStepInner) from presentation logic (ConnectedVideoStep) follows good React patterns and improves code maintainability. The proper handling of loading states and user feedback through button states is well implemented.
| const { data, status } = useMeQuery(); | ||
|
|
||
| const metadata = userMetadata.parse(data?.metadata); | ||
| if (isPending) { | ||
| return <StepConnectionLoader />; | ||
| } | ||
|
|
||
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | ||
| (item) => item.userCredentialIds.length > 0 | ||
| ); | ||
| if (hasAnyInstalledVideoApps) { | ||
| setAnyInstalledVideoApps(true); | ||
| } | ||
|
|
||
| if (status !== "success") { | ||
| return <StepConnectionLoader />; | ||
| } | ||
|
|
||
| const result = userMetadata.safeParse(data?.metadata); | ||
| if (!result.success) { | ||
| return <StepConnectionLoader />; | ||
| } | ||
| const { data: metadata } = result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed data fetches
The component returns StepConnectionLoader for both loading and error states without distinguishing between them. Consider adding proper error handling to provide better user feedback when the API call fails.
- if (status !== "success") {
- return <StepConnectionLoader />;
- }
+ if (status === "error") {
+ // Consider showing an error message to the user
+ return <div className="text-center text-red-500">{t("error_loading_video_apps")}</div>;
+ }
+
+ if (status !== "success") {
+ return <StepConnectionLoader />;
+ }📝 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.
| const { data, status } = useMeQuery(); | |
| const metadata = userMetadata.parse(data?.metadata); | |
| if (isPending) { | |
| return <StepConnectionLoader />; | |
| } | |
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | |
| (item) => item.userCredentialIds.length > 0 | |
| ); | |
| if (hasAnyInstalledVideoApps) { | |
| setAnyInstalledVideoApps(true); | |
| } | |
| if (status !== "success") { | |
| return <StepConnectionLoader />; | |
| } | |
| const result = userMetadata.safeParse(data?.metadata); | |
| if (!result.success) { | |
| return <StepConnectionLoader />; | |
| } | |
| const { data: metadata } = result; | |
| const { data, status } = useMeQuery(); | |
| if (isPending) { | |
| return <StepConnectionLoader />; | |
| } | |
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | |
| (item) => item.userCredentialIds.length > 0 | |
| ); | |
| if (hasAnyInstalledVideoApps) { | |
| setAnyInstalledVideoApps(true); | |
| } | |
| if (status === "error") { | |
| // Consider showing an error message to the user | |
| return <div className="text-center text-red-500">{t("error_loading_video_apps")}</div>; | |
| } | |
| if (status !== "success") { | |
| return <StepConnectionLoader />; | |
| } | |
| const result = userMetadata.safeParse(data?.metadata); | |
| if (!result.success) { | |
| return <StepConnectionLoader />; | |
| } | |
| const { data: metadata } = result; |
🤖 Prompt for AI Agents
In apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx around
lines 43 to 64, the component currently treats loading and error the same by
returning StepConnectionLoader; update the control flow to explicitly handle the
query status: render a loading state when status/loading/isPending, render a
dedicated error UI (or message with optional retry) when status === "error" or
the fetch failed, and only proceed to parse metadata when status === "success";
also move any setAnyInstalledVideoApps state updates into a useEffect that runs
when queryConnectedVideoApps changes to avoid triggering state updates during
render.
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | ||
| (item) => item.userCredentialIds.length > 0 | ||
| ); | ||
| if (hasAnyInstalledVideoApps) { | ||
| setAnyInstalledVideoApps(true); | ||
| } |
There was a problem hiding this comment.
Potential state update issue with conditional logic
The state update only occurs when hasAnyInstalledVideoApps is true, but there's no handling for when it becomes false (e.g., if a user uninstalls all video apps). This could lead to stale state.
const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some(
(item) => item.userCredentialIds.length > 0
);
- if (hasAnyInstalledVideoApps) {
- setAnyInstalledVideoApps(true);
- }
+ setAnyInstalledVideoApps(hasAnyInstalledVideoApps ?? false);📝 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.
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | |
| (item) => item.userCredentialIds.length > 0 | |
| ); | |
| if (hasAnyInstalledVideoApps) { | |
| setAnyInstalledVideoApps(true); | |
| } | |
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | |
| (item) => item.userCredentialIds.length > 0 | |
| ); | |
| setAnyInstalledVideoApps(hasAnyInstalledVideoApps ?? false); |
🤖 Prompt for AI Agents
In apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx around
lines 49 to 54, the code only calls setAnyInstalledVideoApps(true) when
hasAnyInstalledVideoApps is true, which leaves state stale when it becomes
false; change the logic to always update state with the boolean result (e.g.,
compute const hasAny = !!queryConnectedVideoApps?.items?.some(...) and call
setAnyInstalledVideoApps(hasAny)) and ensure this runs in the appropriate effect
with the query result in the dependency array so the state is reset to false
when no apps are installed.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (1)
93-114: Verify ConnectedVideoStep crash handling and edge casesThe new loading/error guards around both the integrations query and
useMeQuerycover the original undefined-access crash. I audited the other onboarding steps (ConnectCalendars, StepConnectionLoader, onboarding-view) and didn’t find any missing guards on data access.However, two edge cases should be addressed to avoid new crash or render-phase errors:
- In ConnectedVideoStepInner,
queryConnectedVideoApps?.items.some(...)will throw ifitemsisundefined. Change it to:const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items?.some( (item) => item.userCredentialIds.length > 0 );- Calling
setAnyInstalledVideoApps(true)directly in the render body violates React’s render-phase rules (and can trigger a “Too many re-renders” error). Wrap that in auseEffectinstead:useEffect(() => { if (hasAnyInstalledVideoApps) setAnyInstalledVideoApps(true); }, [hasAnyInstalledVideoApps]);(Optional) Consider adding an error boundary around
<OnboardingPage>to catch any other unexpected runtime errors in the step components.
🧹 Nitpick comments (2)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (2)
43-47: Consider adding error handling for failed queries.While the component handles pending states well, it doesn't distinguish between loading and error states. Consider checking for error status to provide better user feedback.
// we want to start loading immediately, after all this is a hook. -const { data, status } = useMeQuery(); +const { data, status, error } = useMeQuery(); if (isPending) { return <StepConnectionLoader />; } + +if (error) { + // Log error or show user-friendly message + console.error("Failed to load user data:", error); + return <StepConnectionLoader />; +}
67-89: Consider extracting the filter condition for daily-video.The hardcoded check for "daily-video" could be made more maintainable by extracting it to a constant or configuration.
+const EXCLUDED_VIDEO_APPS = ["daily-video"]; // Apps installed by default return ( <List className="bg-default border-subtle divide-subtle scroll-bar mx-1 max-h-[45vh] divide-y !overflow-y-scroll rounded-md border p-0 sm:mx-0"> {queryConnectedVideoApps?.items && queryConnectedVideoApps?.items.map((item) => { - if (item.slug === "daily-video") return null; // we dont want to show daily here as it is installed by default + if (EXCLUDED_VIDEO_APPS.includes(item.slug)) return null; // Exclude default apps return (
📜 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(3 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
🧬 Code Graph Analysis (1)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (4)
packages/trpc/react/hooks/useMeQuery.ts (1)
useMeQuery(3-11)packages/platform/libraries/index.ts (1)
userMetadata(63-63)packages/ui/components/list/List.tsx (1)
List(15-31)apps/web/components/getting-started/components/AppConnectionItem.tsx (1)
AppConnectionItem(139-139)
🔇 Additional comments (4)
apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx (4)
1-3: LGTM! Clean type imports.The imports are well-organized with proper type annotations.
21-27: LGTM! Well-structured component separation.The inner component pattern is a good architectural choice for separating data fetching concerns from the parent component's state management.
60-65: LGTM! Excellent error handling with safe parsing.Good defensive programming with
safeParseto handle potentially malformed metadata gracefully.
100-111: Button disabled state is properly controlled.Good implementation of the disabled state based on
hasAnyInstalledVideoApps, ensuring users can only proceed after selecting a video app.
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | ||
| (item) => item.userCredentialIds.length > 0 | ||
| ); | ||
| if (hasAnyInstalledVideoApps) { | ||
| setAnyInstalledVideoApps(true); | ||
| } |
There was a problem hiding this comment.
Potential issue with state updates during render.
Setting state during render (lines 53-54) can cause unnecessary re-renders and potential infinite loops. State updates should be performed in a useEffect hook.
+import { useEffect } from "react";
const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some(
(item) => item.userCredentialIds.length > 0
);
-if (hasAnyInstalledVideoApps) {
- setAnyInstalledVideoApps(true);
-}
+
+useEffect(() => {
+ if (hasAnyInstalledVideoApps) {
+ setAnyInstalledVideoApps(true);
+ }
+}, [hasAnyInstalledVideoApps, setAnyInstalledVideoApps]);📝 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.
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | |
| (item) => item.userCredentialIds.length > 0 | |
| ); | |
| if (hasAnyInstalledVideoApps) { | |
| setAnyInstalledVideoApps(true); | |
| } | |
| import { useEffect } from "react"; | |
| const hasAnyInstalledVideoApps = queryConnectedVideoApps?.items.some( | |
| (item) => item.userCredentialIds.length > 0 | |
| ); | |
| useEffect(() => { | |
| if (hasAnyInstalledVideoApps) { | |
| setAnyInstalledVideoApps(true); | |
| } | |
| }, [hasAnyInstalledVideoApps, setAnyInstalledVideoApps]); |
🤖 Prompt for AI Agents
In apps/web/components/getting-started/steps-views/ConnectedVideoStep.tsx around
lines 49 to 54, the code calls setAnyInstalledVideoApps during render which can
cause extra renders or loops; move this logic into a useEffect: compute whether
any installed video apps exist from queryConnectedVideoApps (guarding for
undefined), then inside a useEffect with queryConnectedVideoApps (or its items)
in the dependency array call setAnyInstalledVideoApps only when the computed
boolean differs from current state to avoid redundant updates.
E2E results are ready! |
3 participants
What does this PR do?
Fixes a 500 error that was introduced by #23040 - as this PR affects the load order; the data connected-video page could now be loaded before the data.metadata was ready, which was an earlier bug but hidden by the invisible crash.
Now this crash is no longer happening, and the connected-video page crashes.