-
Notifications
You must be signed in to change notification settings - Fork 12k
perf: server-fetch data for all pages in /settings/my-account #20712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a8db37a
0f78fff
6149ea5
f9c0549
8111869
d4bde99
e8f24c2
f463b60
866000d
d801a26
58fc67a
f0214f9
130d97d
af5aeba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| "use server"; | ||
|
|
||
| import { revalidatePath } from "next/cache"; | ||
|
|
||
| export async function revalidateSettingsAppearance() { | ||
| revalidatePath("/settings/my-account/appearance"); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { SkeletonLoader } from "~/settings/my-account/appearance-skeleton"; | ||
|
|
||
| export default function Loading() { | ||
| return <SkeletonLoader />; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { CalendarListContainerSkeletonLoader } from "@components/apps/CalendarListContainer"; | ||
|
|
||
| export default function Loading() { | ||
| return <CalendarListContainerSkeletonLoader />; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| "use server"; | ||
|
|
||
| import { revalidatePath } from "next/cache"; | ||
|
|
||
| export async function revalidateSettingsGeneral() { | ||
| revalidatePath("/settings/my-account/general"); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { SkeletonLoader } from "~/settings/my-account/general-skeleton"; | ||
|
|
||
| export default function Loading() { | ||
| return <SkeletonLoader />; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,15 @@ | ||||||||||||||
| import { createRouterCaller } from "app/_trpc/context"; | ||||||||||||||
| import { _generateMetadata } from "app/_utils"; | ||||||||||||||
| import { getTranslate } from "app/_utils"; | ||||||||||||||
| import { revalidatePath } from "next/cache"; | ||||||||||||||
| import { cookies, headers } from "next/headers"; | ||||||||||||||
| import { redirect } from "next/navigation"; | ||||||||||||||
|
|
||||||||||||||
| import SettingsHeader from "@calcom/features/settings/appDir/SettingsHeader"; | ||||||||||||||
| import { getServerSession } from "@calcom/features/auth/lib/getServerSession"; | ||||||||||||||
| import { meRouter } from "@calcom/trpc/server/routers/viewer/me/_router"; | ||||||||||||||
| import { getTravelSchedule } from "@calcom/web/app/cache/travelSchedule"; | ||||||||||||||
|
|
||||||||||||||
| import GeneralQueryView from "~/settings/my-account/general-view"; | ||||||||||||||
| import { buildLegacyRequest } from "@lib/buildLegacyCtx"; | ||||||||||||||
|
|
||||||||||||||
| import GeneralView from "~/settings/my-account/general-view"; | ||||||||||||||
|
|
||||||||||||||
| export const generateMetadata = async () => | ||||||||||||||
| await _generateMetadata( | ||||||||||||||
|
|
@@ -16,17 +21,20 @@ export const generateMetadata = async () => | |||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| 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); | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+28
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix incorrect return statement with redirect. The if (!userId) {
- return redirect(redirectUrl);
+ redirect(redirectUrl);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| 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 ?? []} />; | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Component no longer uses SettingsHeader which affects UI consistency across settings pages |
||||||||||||||
| }; | ||||||||||||||
|
Comment on lines
23
to
38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 |
||||||||||||||
|
|
||||||||||||||
| export default Page; | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { SkeletonLoader } from "~/settings/my-account/profile-skeleton"; | ||
|
|
||
| export default function Loading() { | ||
| return <SkeletonLoader />; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| import { createRouterCaller } from "app/_trpc/context"; | ||
| import { _generateMetadata } from "app/_utils"; | ||
| import { getTranslate } from "app/_utils"; | ||
| import { redirect } from "next/navigation"; | ||
|
|
||
| import SettingsHeader from "@calcom/features/settings/appDir/SettingsHeader"; | ||
| import { APP_NAME } from "@calcom/lib/constants"; | ||
| import { meRouter } from "@calcom/trpc/server/routers/viewer/me/_router"; | ||
|
|
||
| import ProfileView from "~/settings/my-account/profile-view"; | ||
|
|
||
|
|
@@ -16,16 +16,13 @@ export const generateMetadata = async () => | |
| ); | ||
|
|
||
| const Page = async () => { | ||
| const t = await getTranslate(); | ||
| const meCaller = await createRouterCaller(meRouter); | ||
| const user = await meCaller.get({ includePasswordAdded: true }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if (!user) { | ||
| redirect("/auth/login"); | ||
| } | ||
|
|
||
| return ( | ||
| <SettingsHeader | ||
| title={t("profile")} | ||
| description={t("profile_description", { appName: APP_NAME })} | ||
| borderInShellHeader={true}> | ||
| <ProfileView /> | ||
| </SettingsHeader> | ||
| ); | ||
| return <ProfileView user={user} />; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }; | ||
|
|
||
| export default Page; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "use server"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { revalidateTag, unstable_cache } from "next/cache"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { NEXTJS_CACHE_TTL } from "@calcom/lib/constants"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { MembershipRepository } from "@calcom/lib/server/repository/membership"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const CACHE_TAGS = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| HAS_TEAM_PLAN: "MembershipRepository.findFirstAcceptedMembershipByUserId", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } as const; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling and consider cache key uniqueness. The cache implementation needs improvement:
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const revalidateHasTeamPlan = async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| revalidateTag(CACHE_TAGS.HAS_TEAM_PLAN); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| "use server"; | ||
|
|
||
| import { revalidatePath } from "next/cache"; | ||
|
|
||
| export async function revalidateSettingsProfile() { | ||
| revalidatePath("/settings/my-account/profile"); | ||
| } | ||
|
|
||
| export async function revalidateSettingsCalendars() { | ||
| revalidatePath("/settings/my-account/calendars"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider consolidating redirect logic and adding error handling.
The current implementation has room for improvement:
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
🤖 Prompt for AI Agents