perf: server-fetch data for all pages in /settings/my-account#20712
perf: server-fetch data for all pages in /settings/my-account#20712
Conversation
hbjORbj
changed the title
dosubot
bot
added
the
performance
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (04/15/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
mrge found 8 issues across 25 files. View them in mrge.io
| }; | ||
|
|
||
| export function CalendarListContainer({ | ||
| connectedCalendars: data, |
There was a problem hiding this comment.
Parameter destructuring renames 'connectedCalendars' to 'data' internally which creates confusing access patterns throughout the component
| const mutation = trpc.viewer.me.updateProfile.useMutation({ | ||
| onSuccess: async (data) => { | ||
| await utils.viewer.me.invalidate(); | ||
| revalidateSettingsAppearance(); |
There was a problem hiding this comment.
Calling revalidateSettingsAppearance in onSuccess is redundant as it's also called in onSettled
| const mutation = trpc.viewer.me.updateProfile.useMutation({ | ||
| onSuccess: async (res) => { | ||
| await utils.viewer.me.invalidate(); | ||
| revalidateSettingsGeneral(); |
There was a problem hiding this comment.
Duplicate revalidation calls in both onSuccess and onSettled callbacks
|
|
||
| const GeneralView = ({ localeProp, user, travelSchedules, revalidatePage }: GeneralViewProps) => { | ||
| const GeneralView = ({ user, travelSchedules }: GeneralViewProps) => { | ||
| const localeProp = user.locale ?? "en"; |
There was a problem hiding this comment.
Missing user.locale default handling in the original component
| description={t("profile_description", { appName: APP_NAME })} | ||
| borderInShellHeader={true}> | ||
| <SkeletonContainer> | ||
| <div className="border-subtle space-y-6 rounded-b-lg border border-t-0 px-4 py-8"> |
There was a problem hiding this comment.
Inconsistent border radius class: 'rounded-b-lg' is used here while other skeleton components use 'rounded-b-xl'
| description={t("profile_description", { appName: APP_NAME })} | ||
| borderInShellHeader={true}> | ||
| <SkeletonContainer> | ||
| <div className="border-subtle space-y-6 rounded-b-lg border border-t-0 px-4 py-8"> |
There was a problem hiding this comment.
Missing responsive padding: This component doesn't include 'sm:px-6' which is present in other similar skeleton components
| if (!user) { | ||
| redirect("/auth/login"); | ||
| } | ||
| return <GeneralView user={user} travelSchedules={travelSchedules ?? []} />; |
There was a problem hiding this comment.
Component no longer uses SettingsHeader which affects UI consistency across settings pages
| ); | ||
| }; | ||
|
|
||
| export const ConferencingAppsViewWebWrapper = () => { |
There was a problem hiding this comment.
The ConferencingAppsViewWebWrapper component signature has been changed without updating all usages. This breaks backward compatibility.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
E2E results are ready! |
|
This PR is being marked as stale due to inactivity. |
|
✅ No security or compliance issues detected. Reviewed everything up to af5aeba. Security Overview
Detected Code Changes
Reply to this PR with |
…ttings-my-account-server-fetch
There was a problem hiding this comment.
cubic found 5 issues across 33 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| <ProfileView /> | ||
| </SettingsHeader> | ||
| ); | ||
| return <ProfileView user={user} />; |
There was a problem hiding this comment.
Directly passing the user object as a prop may result in prop drilling if ProfileView passes it further down. Prefer composition or context for user data to avoid deep prop drilling, per project guidelines.
| const Page = async () => { | ||
| const t = await getTranslate(); | ||
| const meCaller = await createRouterCaller(meRouter); | ||
| const user = await meCaller.get({ includePasswordAdded: true }); |
There was a problem hiding this comment.
The get() call uses an object with includePasswordAdded: true, which may result in overfetching if the implementation uses Prisma's include instead of select. Per project guidelines, always use select to fetch only required fields and avoid unnecessary data exposure.
| } | ||
|
|
||
| static async findFirstAcceptedMembershipByUserId(userId: number) { | ||
| return await prisma.membership.findFirst({ |
There was a problem hiding this comment.
Query returns all membership columns because no select clause is provided, violating the project guideline "Only select data you need" and potentially exposing unnecessary data.
| keys: Parameters<typeof unstable_cache>[1], | ||
| opts: Parameters<typeof unstable_cache>[2] | ||
| ) => { | ||
| const wrap = async (params: unknown[]): Promise<string> => { |
There was a problem hiding this comment.
The params argument is typed as unknown[] but is immediately cast to P in fn(...(params as P)), which may lead to runtime errors if the types do not match. Consider enforcing type safety for params to ensure it matches P.
| } | ||
| ); | ||
|
|
||
| export const revalidateTravelSchedules = async () => { |
There was a problem hiding this comment.
The revalidateTag function from next/cache is synchronous and does not return a Promise, so marking this function as async is unnecessary and may cause confusion.
| export const revalidateTravelSchedules = async () => { | |
| export const revalidateTravelSchedules = () => { |
There was a problem hiding this comment.
cubic found 5 issues across 33 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| const Page = async () => { | ||
| const t = await getTranslate(); | ||
| const meCaller = await createRouterCaller(meRouter); | ||
| const user = await meCaller.get({ includePasswordAdded: true }); |
There was a problem hiding this comment.
The get() call uses an object with includePasswordAdded: true, which may result in overfetching if the implementation uses Prisma's include instead of select. Per project guidelines, always use select to fetch only required fields and avoid unnecessary data exposure.
| <ProfileView /> | ||
| </SettingsHeader> | ||
| ); | ||
| return <ProfileView user={user} />; |
There was a problem hiding this comment.
Directly passing the user object as a prop may result in prop drilling if ProfileView passes it further down. Prefer composition or context for user data to avoid deep prop drilling, per project guidelines.
| } | ||
|
|
||
| static async findFirstAcceptedMembershipByUserId(userId: number) { | ||
| return await prisma.membership.findFirst({ |
There was a problem hiding this comment.
Query returns all membership columns because no select clause is provided, violating the project guideline "Only select data you need" and potentially exposing unnecessary data.
| keys: Parameters<typeof unstable_cache>[1], | ||
| opts: Parameters<typeof unstable_cache>[2] | ||
| ) => { | ||
| const wrap = async (params: unknown[]): Promise<string> => { |
There was a problem hiding this comment.
The params argument is typed as unknown[] but is immediately cast to P in fn(...(params as P)), which may lead to runtime errors if the types do not match. Consider enforcing type safety for params to ensure it matches P.
WalkthroughThis update introduces significant refactoring and feature enhancements to the account settings and integrations sections of the web application. Data fetching for user profile, general settings, appearance, and calendar integrations is moved from client-side components to server-side logic, ensuring that components now receive all necessary data as props. Several new cache modules and revalidation functions are added to manage and invalidate cached data for membership, travel schedules, and specific settings pages. Skeleton loader components are introduced for various settings subpages to provide visual feedback during loading states. The UI structure is unified by wrapping major settings sections in a consistent Estimated code review effort4 (~90 minutes) Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/lib/server/repository/membership.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (10)
packages/platform/atoms/connect/conferencing-apps/ConferencingAppsViewWebWrapper.tsx (1)
182-182: Component signature change handled correctly.The removal of props from
ConferencingAppsViewWebWrapperrepresents a breaking change, but this has been properly coordinated with the consuming page component. The internal localization pattern usinguseLocaleis consistent with the broader refactoring objectives.packages/lib/server/repository/membership.ts (1)
113-125: Add select clause to limit returned data.The method returns all membership columns without a select clause, which violates the project guideline "Only select data you need" and potentially exposes unnecessary data.
Consider adding a select clause to return only the necessary fields:
static async findFirstAcceptedMembershipByUserId(userId: number) { return await prisma.membership.findFirst({ where: { accepted: true, userId, team: { slug: { not: null, }, }, }, + select: { + id: true, + teamId: true, + userId: true, + accepted: true, + role: true, + // Add other necessary fields + }, }); }packages/lib/unstable_cache/unstable_cache.ts (1)
13-16: Address type safety concern with params casting.The
paramsparameter is typed asunknown[]but immediately cast toPwithout validation. This creates a runtime type safety risk if the actual parameters don't match the expected generic typeP.Consider adding runtime type validation or constraining the
wrapfunction parameter type:- const wrap = async (params: unknown[]): Promise<string> => { + const wrap = async (params: P): Promise<string> => { const result = await fn(...(params as P)); return stringify(result); };Alternatively, if
unknown[]is required by the underlyingunstable_cacheAPI, add runtime validation to ensure type safety.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/profile/page.tsx (2)
19-23: Good server-side data fetching pattern, but verify security implications.The server-side data fetching approach aligns well with the PR objectives. The authentication check with redirect is appropriate.
However, ensure that the
includePasswordAdded: trueflag only exposes necessary password-related metadata (like whether a password was added) and not actual password data.
25-25: Consider composition patterns to avoid prop drilling.The direct prop passing works for this level, but as mentioned in past reviews, consider using composition or context patterns if the user data needs to be passed further down the component tree.
apps/web/modules/settings/my-account/profile-skeleton.tsx (1)
21-21: Fix border radius inconsistency and add responsive padding.The component has styling inconsistencies compared to other skeleton components:
- Uses
rounded-b-lgwhile other skeleton components userounded-b-xl- Missing responsive padding
sm:px-6that's present in similar componentsApply this diff to fix the inconsistencies:
- <div className="border-subtle space-y-6 rounded-b-lg border border-t-0 px-4 py-8"> + <div className="border-subtle space-y-6 rounded-b-xl border border-t-0 px-4 py-8 sm:px-6">apps/web/app/cache/travelSchedule.ts (1)
24-26: Remove unnecessary async modifier from revalidation function.The
revalidateTagfunction is synchronous and doesn't return a Promise, so marking this function as async is unnecessary and may cause confusion.-export const revalidateTravelSchedules = async () => { +export const revalidateTravelSchedules = () => {apps/web/modules/settings/my-account/appearance-view.tsx (1)
131-154: Remove redundant revalidation calls from onSuccess callback.The revalidation functions are called in both
onSuccessandonSettledcallbacks. SinceonSettledruns afteronSuccess, the calls inonSuccessare redundant.const mutation = trpc.viewer.me.updateProfile.useMutation({ onSuccess: async (data) => { await utils.viewer.me.invalidate(); - revalidateSettingsAppearance(); - revalidateHasTeamPlan(); showToast(t("settings_updated_successfully"), "success"); resetBrandColorsThemeReset({ brandColor: data.brandColor, darkBrandColor: data.darkBrandColor }); resetBookerLayoutThemeReset({ metadata: data.metadata }); resetUserThemeReset({ theme: data.theme }); resetUserAppThemeReset({ appTheme: data.appTheme }); },apps/web/modules/settings/my-account/general-view.tsx (1)
69-70: Duplicate revalidation calls in both onSuccess and onSettled callbacksThe revalidation functions are being called in both
onSuccessandonSettled. SinceonSettledruns after both success and error cases, successful mutations will trigger revalidation twice, which is inefficient.Consider removing the revalidation calls from
onSuccessand keeping them only inonSettled, or vice versa based on your error handling needs.const mutation = trpc.viewer.me.updateProfile.useMutation({ onSuccess: async (res) => { await utils.viewer.me.invalidate(); - revalidateSettingsGeneral(); - revalidateTravelSchedules(); reset(getValues()); showToast(t("settings_updated_successfully"), "success"); await update(res);Also applies to: 85-86
apps/web/components/apps/CalendarListContainer.tsx (1)
100-100: Parameter destructuring renames 'connectedCalendars' to 'data' internally which creates confusing access patterns throughout the componentThe destructuring pattern
connectedCalendars: datamakes the code harder to follow by using a generic name internally. Consider using the original prop name for clarity.-export function CalendarListContainer({ - connectedCalendars: data, +export function CalendarListContainer({ + connectedCalendars, installedCalendars, heading = true, fromOnboarding, }: CalendarListContainerProps) {Then update all references from
datatoconnectedCalendarsthroughout the component.
🧹 Nitpick comments (1)
apps/web/modules/settings/my-account/general-view.tsx (1)
173-173: Remove unnecessary React fragmentsThe static analysis correctly identified unnecessary fragments that contain only single child elements. These can be simplified for cleaner code.
- <Label className="text-emphasis"> - <>{t("language")}</> - </Label> + <Label className="text-emphasis"> + {t("language")} + </Label>Apply similar changes to lines 190, 270, 291, and 312.
Also applies to: 190-190, 270-270, 291-291, 312-312
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
apps/web/app/(use-page-wrapper)/apps/installed/[category]/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/actions.ts(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/loading.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/calendars/loading.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/calendars/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/conferencing/page.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/actions.ts(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/loading.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/out-of-office/page.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/profile/loading.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/profile/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/push-notifications/page.tsx(1 hunks)apps/web/app/cache/membership.ts(1 hunks)apps/web/app/cache/path/settings/my-account/index.ts(1 hunks)apps/web/app/cache/travelSchedule.ts(1 hunks)apps/web/components/apps/CalendarListContainer.tsx(3 hunks)apps/web/modules/apps/installed/[category]/installed-category-view.tsx(3 hunks)apps/web/modules/settings/my-account/appearance-skeleton.tsx(1 hunks)apps/web/modules/settings/my-account/appearance-view.tsx(5 hunks)apps/web/modules/settings/my-account/general-skeleton.tsx(1 hunks)apps/web/modules/settings/my-account/general-view.tsx(6 hunks)apps/web/modules/settings/my-account/profile-skeleton.tsx(1 hunks)apps/web/modules/settings/my-account/profile-view.tsx(10 hunks)apps/web/modules/settings/my-account/push-notifications-view.tsx(2 hunks)packages/features/settings/outOfOffice/OutOfOfficeEntriesList.tsx(3 hunks)packages/lib/server/repository/membership.ts(1 hunks)packages/lib/unstable_cache/index.ts(1 hunks)packages/lib/unstable_cache/unstable_cache.ts(1 hunks)packages/platform/atoms/connect/conferencing-apps/ConferencingAppsViewWebWrapper.tsx(2 hunks)packages/trpc/server/routers/viewer/teams/hasTeamPlan.handler.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
packages/features/settings/outOfOffice/OutOfOfficeEntriesList.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/page.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/modules/settings/my-account/general-view.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/components/apps/CalendarListContainer.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (12)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/conferencing/page.tsx (1)
packages/platform/atoms/connect/conferencing-apps/ConferencingAppsViewWebWrapper.tsx (1)
ConferencingAppsViewWebWrapper(182-229)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/calendars/loading.tsx (1)
apps/web/components/apps/CalendarListContainer.tsx (1)
CalendarListContainerSkeletonLoader(80-90)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/loading.tsx (1)
apps/web/modules/settings/my-account/appearance-skeleton.tsx (1)
SkeletonLoader(8-37)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/out-of-office/page.tsx (1)
packages/features/settings/outOfOffice/OutOfOfficeEntriesList.tsx (1)
OutOfOfficeEntriesList(66-84)
apps/web/app/(use-page-wrapper)/apps/installed/[category]/page.tsx (4)
apps/web/app/_trpc/context.ts (1)
createRouterCaller(18-22)packages/trpc/server/routers/viewer/calendars/_router.tsx (1)
calendarsRouter(11-25)packages/trpc/server/routers/viewer/apps/_router.tsx (1)
appsRouter(33-135)apps/web/modules/apps/installed/[category]/installed-category-view.tsx (1)
InstalledApps(189-262)
packages/trpc/server/routers/viewer/teams/hasTeamPlan.handler.ts (2)
packages/trpc/server/routers/viewer/teams/procedures/hasTeamPlan.ts (1)
hasTeamPlan(7-10)packages/lib/server/repository/membership.ts (1)
MembershipRepository(72-435)
apps/web/modules/settings/my-account/profile-skeleton.tsx (3)
apps/web/modules/settings/my-account/general-skeleton.tsx (1)
SkeletonLoader(7-23)apps/web/modules/settings/my-account/appearance-skeleton.tsx (1)
SkeletonLoader(8-37)packages/lib/constants.ts (1)
APP_NAME(27-27)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/profile/loading.tsx (1)
apps/web/modules/settings/my-account/profile-skeleton.tsx (1)
SkeletonLoader(13-35)
apps/web/modules/settings/my-account/appearance-view.tsx (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/actions.ts (1)
revalidateSettingsAppearance(5-7)apps/web/app/cache/membership.ts (1)
revalidateHasTeamPlan(25-27)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/loading.tsx (4)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/loading.tsx (1)
Loading(3-5)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/calendars/loading.tsx (1)
Loading(3-5)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/profile/loading.tsx (1)
Loading(3-5)apps/web/modules/settings/my-account/general-skeleton.tsx (1)
SkeletonLoader(7-23)
apps/web/modules/settings/my-account/appearance-skeleton.tsx (2)
apps/web/modules/settings/my-account/general-skeleton.tsx (1)
SkeletonLoader(7-23)apps/web/modules/settings/my-account/profile-skeleton.tsx (1)
SkeletonLoader(13-35)
apps/web/app/cache/travelSchedule.ts (2)
packages/lib/unstable_cache/index.ts (1)
unstable_cache(3-3)packages/lib/constants.ts (1)
NEXTJS_CACHE_TTL(231-231)
🪛 Biome (1.9.4)
apps/web/modules/settings/my-account/general-view.tsx
[error] 173-173: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 190-190: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 270-270: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 291-291: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 312-312: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ 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: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (34)
apps/web/modules/settings/my-account/push-notifications-view.tsx (2)
4-4: Import addition looks good.The SettingsHeader import is correctly added to support the UI structure unification.
13-22: UI structure unification implemented correctly.The wrapping of the existing content with SettingsHeader component follows the consistent pattern described in the AI summary. The props are correctly passed with localized title and description.
However, note that this component retains client-side rendering approach, which doesn't align with the PR objective of implementing server-side data fetching for all pages in /settings/my-account.
Could you verify if push notifications should also be moved to server-side data fetching like other settings pages, or if client-side rendering is intentional for this specific component?
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/push-notifications/page.tsx (1)
14-16: Page component simplification is well-executed.The transformation from async to sync function and direct rendering of PushNotificationsView aligns with the pattern of moving translation and header responsibilities to view components, as mentioned in the AI summary.
However, this doesn't implement server-side data fetching as stated in the PR objectives. The push notifications page remains a client-side rendered component.
The changes don't align with the PR objective of implementing "server-fetch data for all pages in /settings/my-account" - this page retains client-side rendering.
Likely an incorrect or invalid review comment.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/conferencing/page.tsx (1)
15-15: LGTM! Component interface simplified successfully.The removal of translation props aligns well with the broader refactoring to consolidate localization within components. This makes the page component cleaner and more maintainable.
packages/platform/atoms/connect/conferencing-apps/ConferencingAppsViewWebWrapper.tsx (2)
172-180: Well-structured component extraction with proper localization.The
AddConferencingButtoncomponent is cleanly extracted with internal localization using theuseLocalehook. This follows React best practices for component composition and self-contained functionality.
210-212: Internal localization implementation is correct.The direct use of
t("conferencing")andt("conferencing_description")within the component follows the established pattern for internal localization, making the component self-contained and reducing external dependencies.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/actions.ts (1)
1-7: LGTM! Clean server action for cache revalidation.The implementation follows Next.js server action best practices with proper "use server" directive and targeted path revalidation. This aligns well with the PR's goal of moving to server-side data fetching patterns.
apps/web/app/cache/path/settings/my-account/index.ts (1)
1-11: LGTM! Consistent cache revalidation pattern.Both functions follow the established pattern for server-side cache revalidation with proper "use server" directive and targeted path invalidation. This supports the PR's objective of moving to server-side data fetching.
apps/web/modules/settings/my-account/general-skeleton.tsx (1)
7-23: LGTM! Clean skeleton loader implementation.The skeleton loader follows a consistent pattern with proper localization, SettingsHeader wrapper, and appropriate skeleton elements. This supports the separation of loading states from the main page components as part of the server-side data fetching refactoring.
apps/web/modules/settings/my-account/appearance-skeleton.tsx (1)
8-37: LGTM! Well-structured appearance skeleton loader.The skeleton loader appropriately reflects the complexity of the appearance settings UI with custom animated elements for theme selection and proper use of SectionBottomActions. The implementation is consistent with the established pattern while being tailored to the specific page layout.
packages/lib/unstable_cache/index.ts (1)
1-3: LGTM! Clean re-export pattern.The barrel export pattern is implemented correctly, providing a clean API surface by re-exporting the
cachefunction asunstable_cacheto align with Next.js naming conventions.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/actions.ts (1)
1-7: LGTM! Properly implemented server action.The server action correctly uses the "use server" directive and implements cache revalidation for the general settings page. The implementation follows Next.js best practices for cache invalidation.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/loading.tsx (1)
1-5: LGTM! Consistent loading component implementation.The loading component follows the established pattern across the settings pages, providing a consistent user experience during server-side data fetching. The implementation is clean and appropriately delegates to the dedicated skeleton loader component.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/calendars/loading.tsx (1)
1-5: LGTM! Appropriate use of specialized skeleton loader.The loading component correctly uses the calendar-specific skeleton loader, which provides a more contextually appropriate loading state for calendar-related content. The implementation is consistent with the overall loading component pattern while being tailored to the specific needs of the calendars page.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/loading.tsx (1)
1-5: LGTM! Clean loading component implementation.The implementation correctly follows the Next.js App Router pattern for loading states and maintains consistency with similar loading components across other settings pages.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/profile/loading.tsx (1)
1-5: LGTM! Consistent loading component implementation.The implementation follows the same pattern as other loading components in the settings section, maintaining consistency across the application.
apps/web/app/(use-page-wrapper)/apps/installed/[category]/page.tsx (2)
1-1: LGTM! Proper imports for server-side data fetching.The new imports correctly bring in the necessary utilities for creating tRPC router callers and accessing the specific routers needed for calendar and app data fetching.
Also applies to: 8-9
34-51: Excellent server-side data fetching implementation.The concurrent fetching approach using
Promise.allis efficient and the data is properly passed down to the component. This change aligns well with the performance objectives mentioned in the PR.Key benefits:
- Concurrent fetching reduces overall loading time
- Server-side data fetching improves initial page load performance
- Clean separation of data fetching from UI rendering
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/out-of-office/page.tsx (1)
14-16: LGTM! Good simplification following the established pattern.The component correctly delegates UI responsibility to the
OutOfOfficeEntriesListcomponent, which now handles its own header, translations, and structure internally. This improves separation of concerns and consistency across settings pages.packages/features/settings/outOfOffice/OutOfOfficeEntriesList.tsx (2)
26-26: LGTM! Proper imports for the enhanced component structure.The new imports correctly bring in the
SettingsHeadercomponent and expand theOutOfOfficeToggleGroupimport to include the group component, supporting the improved UI structure.Also applies to: 43-43
67-67: Excellent implementation of consistent UI structure.The addition of the
SettingsHeaderwrapper with localized title and description creates consistency across settings pages. The CTA area properly groups the toggle and button components with appropriate spacing, and the existing functionality is preserved within the new structure.Also applies to: 70-83
apps/web/modules/settings/my-account/profile-skeleton.tsx (1)
13-35: Well-structured skeleton component with consistent patterns.The skeleton component follows good patterns with proper localization and uses the SettingsHeader wrapper consistently with other skeleton components. The layout structure effectively mimics the actual profile page content.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/calendars/page.tsx (2)
30-30: Clean data flow with props passing.The pattern of passing fetched data as props to the container component maintains clean separation of concerns between data fetching and UI rendering.
19-28: No extra auth check needed—TRPC endpoints are already protectedBoth the calendars and apps integrations calls use authedProcedure and will error without a valid session. Additionally, this page lives under the
(use-page-wrapper)/settingslayout, which enforces user authentication.• trpc/server/routers/viewer/calendars/_router.tsx →
connectedCalendars: authedProcedure…
• trpc/server/routers/viewer/apps/_router.tsx →integrations: authedProcedure…
• Page route is wrapped by(use-page-wrapper)ensuring only authenticated users reach itapps/web/app/cache/travelSchedule.ts (1)
13-22: Well-structured cache implementation with proper TTL and tagging.The cache implementation follows good patterns:
- Uses repository pattern for data access
- Proper TTL configuration
- Tag-based invalidation for targeted cache clearing
- Clean separation of concerns
packages/trpc/server/routers/viewer/teams/hasTeamPlan.handler.ts (2)
10-15: Excellent refactoring to repository pattern.The refactoring successfully moves from direct Prisma queries to the repository pattern, which provides better:
- Separation of concerns
- Code reusability
- Testability
- Maintainability
The logic remains functionally equivalent while being more modular.
1-2: Good import organization with proper typing.The imports are well-organized with the repository import and proper type imports for the session user.
apps/web/modules/apps/installed/[category]/installed-category-view.tsx (1)
15-15: LGTM! Clean refactoring to prop-based data passing.The changes properly implement server-side data fetching by:
- Adding appropriate type definitions for the new props
- Updating the component signature to accept the pre-fetched data
- Passing the props correctly to
CalendarListContainerThis architectural shift improves performance by moving data fetching to the server.
Also applies to: 183-187, 189-189, 239-244
apps/web/modules/settings/my-account/profile-view.tsx (2)
81-81: Consistent cache revalidation pattern implemented correctlyThe addition of
revalidateSettingsProfile()calls after TRPC cache invalidations ensures server-side cache consistency across all mutations. This is a good practice for Next.js App Router applications.Also applies to: 108-108, 120-120, 145-145, 176-176, 184-184, 293-293
254-257: Well-structured UI with SettingsHeader wrapperThe implementation of
SettingsHeaderprovides a consistent layout structure across settings pages with appropriate title and description. TheborderInShellHeaderprop maintains visual consistency.Also applies to: 462-462
apps/web/modules/settings/my-account/general-view.tsx (2)
56-56: Proper locale default handling implementedGood addition of the fallback to "en" when user.locale is not available. This prevents potential issues with undefined locale values throughout the component.
154-363: Well-structured component with improved UI organizationThe refactoring successfully:
- Implements consistent UI with
SettingsHeader- Properly organizes the travel schedule section with styled containers
- Maintains clean separation between form controls and toggle settings
apps/web/components/apps/CalendarListContainer.tsx (2)
125-125: Proper cache revalidation for calendar settingsThe addition of
revalidateSettingsCalendars()calls ensures server-side cache consistency after calendar-related mutations, following the same pattern as other settings modules.Also applies to: 131-131
69-90: Well-architected component structure with proper separation of concernsThe refactoring introduces:
- Reusable
AddCalendarButtoncomponent- Dedicated skeleton loader with consistent UI
- Proper integration with
SettingsHeaderfor uniform layoutThis architecture improves maintainability and user experience.
Also applies to: 136-180
| const result = await fn(...(params as P)); | ||
| return stringify(result); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for serialization failures.
The stringify operation could fail for certain data types or circular references. Consider wrapping the serialization in a try-catch block to provide better error handling.
const wrap = async (params: unknown[]): Promise<string> => {
const result = await fn(...(params as P));
- return stringify(result);
+ try {
+ return stringify(result);
+ } catch (error) {
+ throw new Error(`Failed to serialize cache result: ${error}`);
+ }
};📝 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 result = await fn(...(params as P)); | |
| return stringify(result); | |
| const wrap = async (params: unknown[]): Promise<string> => { | |
| const result = await fn(...(params as P)); | |
| try { | |
| return stringify(result); | |
| } catch (error) { | |
| throw new Error(`Failed to serialize cache result: ${error}`); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In packages/lib/unstable_cache/unstable_cache.ts around lines 14 to 15, the call
to stringify the result can throw errors for unsupported data types or circular
references. Wrap the stringify call in a try-catch block to catch serialization
errors and handle them gracefully, such as by logging the error or throwing a
more descriptive error message.
| const result = await cachedFn(params); | ||
| return parse(result); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for deserialization failures.
The parse operation could fail if the cached data is corrupted or incompatible. Consider adding error handling for deserialization failures.
return async (...params: P): Promise<T> => {
const result = await cachedFn(params);
- return parse(result);
+ try {
+ return parse(result);
+ } catch (error) {
+ throw new Error(`Failed to deserialize cached data: ${error}`);
+ }
};📝 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 result = await cachedFn(params); | |
| return parse(result); | |
| return async (...params: P): Promise<T> => { | |
| const result = await cachedFn(params); | |
| try { | |
| return parse(result); | |
| } catch (error) { | |
| throw new Error(`Failed to deserialize cached data: ${error}`); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In packages/lib/unstable_cache/unstable_cache.ts around lines 21 to 22, the call
to parse the cached result lacks error handling, which can cause unhandled
exceptions if the cached data is corrupted or incompatible. Wrap the parse call
in a try-catch block to catch any deserialization errors, and handle them
appropriately, such as logging the error and returning a fallback value or
rethrowing with additional context.
| const Page = async () => { | ||
| const t = await getTranslate(); | ||
| const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); | ||
| const userId = session?.user?.id; | ||
| const redirectUrl = "/auth/login?callbackUrl=/settings/my-account/appearance"; | ||
|
|
||
| return ( | ||
| <SettingsHeader title={t("appearance")} description={t("appearance_description")}> | ||
| <AppearancePage /> | ||
| </SettingsHeader> | ||
| ); | ||
| if (!userId) { | ||
| redirect(redirectUrl); | ||
| } | ||
|
|
||
| const [meCaller, hasTeamPlan] = await Promise.all([ | ||
| createRouterCaller(meRouter), | ||
| getCachedHasTeamPlan(userId), | ||
| ]); | ||
|
|
||
| const user = await meCaller.get(); | ||
|
|
||
| if (!user) { | ||
| redirect(redirectUrl); | ||
| } | ||
| const isCurrentUsernamePremium = | ||
| user && hasKeyInMetadata(user, "isPremium") ? !!user.metadata.isPremium : false; | ||
| const hasPaidPlan = IS_SELF_HOSTED ? true : hasTeamPlan?.hasTeamPlan || isCurrentUsernamePremium; | ||
|
|
||
| return <AppearancePage user={user} hasPaidPlan={hasPaidPlan} />; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating redirect logic and adding error handling.
The current implementation has room for improvement:
- The redirect URL is repeated and could be extracted to a constant
- Missing error handling for the async operations
- The concurrent fetch could fail independently
const Page = async () => {
const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });
const userId = session?.user?.id;
- const redirectUrl = "/auth/login?callbackUrl=/settings/my-account/appearance";
+ const REDIRECT_URL = "/auth/login?callbackUrl=/settings/my-account/appearance";
if (!userId) {
- redirect(redirectUrl);
+ redirect(REDIRECT_URL);
}
- const [meCaller, hasTeamPlan] = await Promise.all([
- createRouterCaller(meRouter),
- getCachedHasTeamPlan(userId),
- ]);
+ try {
+ const [meCaller, hasTeamPlan] = await Promise.all([
+ createRouterCaller(meRouter),
+ getCachedHasTeamPlan(userId),
+ ]);
- const user = await meCaller.get();
+ const user = await meCaller.get();
- if (!user) {
- redirect(redirectUrl);
- }
+ if (!user) {
+ redirect(REDIRECT_URL);
+ }
const isCurrentUsernamePremium =
user && hasKeyInMetadata(user, "isPremium") ? !!user.metadata.isPremium : false;
const hasPaidPlan = IS_SELF_HOSTED ? true : hasTeamPlan?.hasTeamPlan || isCurrentUsernamePremium;
return <AppearancePage user={user} hasPaidPlan={hasPaidPlan} />;
+ } catch (error) {
+ // Log error for monitoring
+ console.error("Failed to fetch user data:", error);
+ redirect(REDIRECT_URL);
+ }
};📝 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 Page = async () => { | |
| const t = await getTranslate(); | |
| const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); | |
| const userId = session?.user?.id; | |
| const redirectUrl = "/auth/login?callbackUrl=/settings/my-account/appearance"; | |
| return ( | |
| <SettingsHeader title={t("appearance")} description={t("appearance_description")}> | |
| <AppearancePage /> | |
| </SettingsHeader> | |
| ); | |
| if (!userId) { | |
| redirect(redirectUrl); | |
| } | |
| const [meCaller, hasTeamPlan] = await Promise.all([ | |
| createRouterCaller(meRouter), | |
| getCachedHasTeamPlan(userId), | |
| ]); | |
| const user = await meCaller.get(); | |
| if (!user) { | |
| redirect(redirectUrl); | |
| } | |
| const isCurrentUsernamePremium = | |
| user && hasKeyInMetadata(user, "isPremium") ? !!user.metadata.isPremium : false; | |
| const hasPaidPlan = IS_SELF_HOSTED ? true : hasTeamPlan?.hasTeamPlan || isCurrentUsernamePremium; | |
| return <AppearancePage user={user} hasPaidPlan={hasPaidPlan} />; | |
| }; | |
| const Page = async () => { | |
| const session = await getServerSession({ | |
| req: buildLegacyRequest(await headers(), await cookies()), | |
| }); | |
| const userId = session?.user?.id; | |
| const REDIRECT_URL = "/auth/login?callbackUrl=/settings/my-account/appearance"; | |
| if (!userId) { | |
| redirect(REDIRECT_URL); | |
| } | |
| try { | |
| const [meCaller, hasTeamPlan] = await Promise.all([ | |
| createRouterCaller(meRouter), | |
| getCachedHasTeamPlan(userId), | |
| ]); | |
| const user = await meCaller.get(); | |
| if (!user) { | |
| redirect(REDIRECT_URL); | |
| } | |
| const isCurrentUsernamePremium = | |
| user && hasKeyInMetadata(user, "isPremium") | |
| ? !!user.metadata.isPremium | |
| : false; | |
| const hasPaidPlan = IS_SELF_HOSTED | |
| ? true | |
| : hasTeamPlan?.hasTeamPlan || isCurrentUsernamePremium; | |
| return <AppearancePage user={user} hasPaidPlan={hasPaidPlan} />; | |
| } catch (error) { | |
| // Log error for monitoring | |
| console.error("Failed to fetch user data:", error); | |
| redirect(REDIRECT_URL); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/appearance/page.tsx
lines 25 to 49, consolidate the redirect URL into a single constant to avoid
repetition. Add try-catch blocks around the async operations like
getServerSession, createRouterCaller, getCachedHasTeamPlan, and meCaller.get to
handle potential errors gracefully. Ensure that if any of these async calls
fail, the user is redirected appropriately or an error state is handled,
preventing unhandled promise rejections and improving reliability.
| export const getCachedHasTeamPlan = unstable_cache( | ||
| async (userId: number) => { | ||
| const hasTeamPlan = await MembershipRepository.findFirstAcceptedMembershipByUserId(userId); | ||
|
|
||
| return { hasTeamPlan: !!hasTeamPlan }; | ||
| }, | ||
| ["getCachedHasTeamPlan"], | ||
| { | ||
| revalidate: NEXTJS_CACHE_TTL, | ||
| tags: [CACHE_TAGS.HAS_TEAM_PLAN], | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider cache key uniqueness.
The cache implementation needs improvement:
- No error handling if the repository call fails
- The cache key should include the userId for uniqueness
- Consider the implications of using
unstable_cache
export const getCachedHasTeamPlan = unstable_cache(
async (userId: number) => {
- const hasTeamPlan = await MembershipRepository.findFirstAcceptedMembershipByUserId(userId);
-
- return { hasTeamPlan: !!hasTeamPlan };
+ try {
+ const hasTeamPlan = await MembershipRepository.findFirstAcceptedMembershipByUserId(userId);
+ return { hasTeamPlan: !!hasTeamPlan };
+ } catch (error) {
+ console.error(`Failed to fetch team plan for user ${userId}:`, error);
+ // Return false as a safe default when cache fetch fails
+ return { hasTeamPlan: false };
+ }
},
- ["getCachedHasTeamPlan"],
+ ["getCachedHasTeamPlan", (userId) => userId.toString()],
{
revalidate: NEXTJS_CACHE_TTL,
tags: [CACHE_TAGS.HAS_TEAM_PLAN],
}
);📝 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.
| export const getCachedHasTeamPlan = unstable_cache( | |
| async (userId: number) => { | |
| const hasTeamPlan = await MembershipRepository.findFirstAcceptedMembershipByUserId(userId); | |
| return { hasTeamPlan: !!hasTeamPlan }; | |
| }, | |
| ["getCachedHasTeamPlan"], | |
| { | |
| revalidate: NEXTJS_CACHE_TTL, | |
| tags: [CACHE_TAGS.HAS_TEAM_PLAN], | |
| } | |
| ); | |
| export const getCachedHasTeamPlan = unstable_cache( | |
| async (userId: number) => { | |
| try { | |
| const hasTeamPlan = await MembershipRepository.findFirstAcceptedMembershipByUserId(userId); | |
| return { hasTeamPlan: !!hasTeamPlan }; | |
| } catch (error) { | |
| console.error(`Failed to fetch team plan for user ${userId}:`, error); | |
| // Return false as a safe default when cache fetch fails | |
| return { hasTeamPlan: false }; | |
| } | |
| }, | |
| ["getCachedHasTeamPlan", (userId) => userId.toString()], | |
| { | |
| revalidate: NEXTJS_CACHE_TTL, | |
| tags: [CACHE_TAGS.HAS_TEAM_PLAN], | |
| } | |
| ); |
🤖 Prompt for AI Agents
In apps/web/app/cache/membership.ts around lines 12 to 23, add error handling to
catch and handle any errors from
MembershipRepository.findFirstAcceptedMembershipByUserId to prevent unhandled
exceptions. Modify the cache key to include the userId parameter to ensure cache
entries are unique per user. Review the use of unstable_cache for potential
stability or behavior concerns and document or adjust usage accordingly.
| if (!userId) { | ||
| return redirect(redirectUrl); | ||
| } |
There was a problem hiding this comment.
Fix incorrect return statement with redirect.
The redirect() function from Next.js navigation throws internally and doesn't return a value. Remove the return statement.
if (!userId) {
- return redirect(redirectUrl);
+ redirect(redirectUrl);
}📝 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 (!userId) { | |
| return redirect(redirectUrl); | |
| } | |
| if (!userId) { | |
| redirect(redirectUrl); | |
| } |
🤖 Prompt for AI Agents
In
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/page.tsx
around lines 28 to 30, the code incorrectly uses 'return redirect(redirectUrl);'
but the redirect() function throws internally and does not return a value.
Remove the 'return' keyword and just call 'redirect(redirectUrl);' to fix the
issue.
| const Page = async () => { | ||
| const t = await getTranslate(); | ||
| const revalidatePage = async () => { | ||
| "use server"; | ||
| revalidatePath("settings/my-account/general"); | ||
| }; | ||
|
|
||
| return ( | ||
| <SettingsHeader title={t("general")} description={t("general_description")} borderInShellHeader={true}> | ||
| <GeneralQueryView revalidatePage={revalidatePage} /> | ||
| </SettingsHeader> | ||
| ); | ||
| const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); | ||
| const userId = session?.user?.id; | ||
| const redirectUrl = "/auth/login?callbackUrl=/settings/my-account/general"; | ||
|
|
||
| if (!userId) { | ||
| return redirect(redirectUrl); | ||
| } | ||
|
|
||
| const meCaller = await createRouterCaller(meRouter); | ||
| const [user, travelSchedules] = await Promise.all([meCaller.get(), getTravelSchedule(userId)]); | ||
| if (!user) { | ||
| redirect(redirectUrl); | ||
| } | ||
| return <GeneralView user={user} travelSchedules={travelSchedules ?? []} />; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for concurrent data fetching.
Similar to the appearance page, this implementation lacks error handling for the async operations.
const Page = async () => {
const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });
const userId = session?.user?.id;
- const redirectUrl = "/auth/login?callbackUrl=/settings/my-account/general";
+ const REDIRECT_URL = "/auth/login?callbackUrl=/settings/my-account/general";
if (!userId) {
- return redirect(redirectUrl);
+ redirect(REDIRECT_URL);
}
- const meCaller = await createRouterCaller(meRouter);
- const [user, travelSchedules] = await Promise.all([meCaller.get(), getTravelSchedule(userId)]);
- if (!user) {
- redirect(redirectUrl);
- }
- return <GeneralView user={user} travelSchedules={travelSchedules ?? []} />;
+ try {
+ const meCaller = await createRouterCaller(meRouter);
+ const [user, travelSchedules] = await Promise.all([meCaller.get(), getTravelSchedule(userId)]);
+ if (!user) {
+ redirect(REDIRECT_URL);
+ }
+ return <GeneralView user={user} travelSchedules={travelSchedules ?? []} />;
+ } catch (error) {
+ console.error("Failed to fetch user data:", error);
+ redirect(REDIRECT_URL);
+ }
};🤖 Prompt for AI Agents
In
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/general/page.tsx
lines 23 to 38, the concurrent data fetching with Promise.all lacks error
handling. Wrap the Promise.all call in a try-catch block to catch any errors
from meCaller.get() or getTravelSchedule(userId). In the catch block, handle the
error appropriately, such as logging it and redirecting to the login page or
showing an error message, to prevent unhandled promise rejections and improve
robustness.
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
LGTM! - tested, works great :)
4 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?