Skip to content

perf: fetch server side data for workflow details page #20778

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 41 additions & 22 deletions apps/web/app/(use-page-wrapper)/workflows/[workflow]/page.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
import { createRouterCaller } from "app/_trpc/context";
import type { PageProps } from "app/_types";
import { _generateMetadata } from "app/_utils";
import type { Metadata } from "next";
import { cookies, headers } from "next/headers";
import { notFound } from "next/navigation";
import { redirect } from "next/navigation";
import { cache } from "react";
import { z } from "zod";

// import { cookies, headers } from "next/headers";
// import { getServerSession } from "@calcom/features/auth/lib/getServerSession";
// import { buildLegacyRequest } from "@lib/buildLegacyCtx";
import { getServerSession } from "@calcom/features/auth/lib/getServerSession";
import LegacyPage from "@calcom/features/ee/workflows/pages/workflow";
import { WorkflowRepository } from "@calcom/lib/server/repository/workflow";
import { eventTypesRouter } from "@calcom/trpc/server/routers/viewer/eventTypes/_router";
import { meRouter } from "@calcom/trpc/server/routers/viewer/me/_router";
import { workflowsRouter } from "@calcom/trpc/server/routers/viewer/workflows/_router";

import { buildLegacyRequest } from "@lib/buildLegacyCtx";

const querySchema = z.object({
workflow: z
Expand Down Expand Up @@ -41,33 +47,46 @@ export const generateMetadata = async ({ params }: PageProps): Promise<Metadata
};

const Page = async ({ params }: PageProps) => {
// const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });
// const user = session?.user;
const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });

if (!session?.user?.id) {
redirect("/auth/login");
}

const parsed = querySchema.safeParse(await params);
if (!parsed.success) {
notFound();
}
const workFlowId = parsed.data.workflow;

const [workflowCaller, eventCaller, userCaller] = await Promise.all([
createRouterCaller(workflowsRouter),
createRouterCaller(eventTypesRouter),
createRouterCaller(meRouter),
]);

const workflowData = await workflowCaller.get({ id: workFlowId });

if (!workflowData) return notFound();

const isOrg = workflowData?.team?.isOrganization ?? false;
const teamId = workflowData?.teamId ?? undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: what happens when teamId is undefined? does it break anything? if so, shouldn't we show 404 page?

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case it returns [] - empty array same as what we have as fallback rn , nothing's breaking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can improve it by skipping the verifiedNumbers call if teamId is undefined and return [] straight away


// const workflow = await WorkflowRepository.getById({ id: +parsed.data.workflow });
// let verifiedEmails, verifiedNumbers;
// try {
// verifiedEmails = await WorkflowRepository.getVerifiedEmails({
// userEmail: user?.email ?? null,
// userId: user?.id ?? null,
// teamId: workflow?.team?.id,
// });
// } catch (err) {}
// try {
// verifiedNumbers = await WorkflowRepository.getVerifiedNumbers({
// userId: user?.id ?? null,
// teamId: workflow?.team?.id,
// });
// } catch (err) {}
const [verifiedEmails, verifiedNumbers, eventsData, user] = await Promise.all([
workflowCaller.getVerifiedEmails({ teamId }),
teamId ? workflowCaller.getVerifiedNumbers({ teamId }) : Promise.resolve([]),
eventCaller.getTeamAndEventTypeOptions({ teamId, isOrg }),
userCaller.get(),
]);

return (
<LegacyPage
workflow={parsed.data.workflow}
// workflowData={workflow} verifiedEmails={verifiedEmails} verifiedNumbers={verifiedNumbers}
user={user}
eventsData={eventsData}
workflowId={workFlowId}
workflow={workflowData}
verifiedNumbers={verifiedNumbers}
verifiedEmails={verifiedEmails}
/>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function UsersTable({ setSMSLockState }: Props) {
const { data: usersAndTeams } = trpc.viewer.admin.getSMSLockStateTeamsUsers.useQuery();

if (!usersAndTeams) {
return <></>;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why this change was necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for better readability/code quality

}

const users = usersAndTeams.users.locked.concat(usersAndTeams.users.reviewNeeded);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const AddActionDialog = (props: IAddActionDialog) => {
const [isPhoneNumberNeeded, setIsPhoneNumberNeeded] = useState(false);
const [isSenderIdNeeded, setIsSenderIdNeeded] = useState(false);
const [isEmailAddressNeeded, setIsEmailAddressNeeded] = useState(false);

const { data: actionOptions } = trpc.viewer.workflows.getWorkflowActionOptions.useQuery();
Copy link
Contributor

@hbjORbj hbjORbj Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query should stay client-side because the "Add action" Dialog should not block the initial load. For example, imagine that the query fetching for Add Action Dialog failed. Now it blocks the entire page load. Handling a query failure is another topic, but I am mentioning it to help you decide which is something you should fetch server-side vs client-side. The more server-side fetching we do, the slower the initial load is, and hence we should determine carefully which queries to turn to server-side and keep client-side.

So, Please think about this for other queries you switched to server-side in this PR and make changes accordingly!

Screenshot 2025-04-21 at 11 04 31 AM

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically we where doing this same call two times : one for the page (form) and another for this Dialog so I just removed it from here and passed from the page - I understand your concerns but that's not the case here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you understood my point. I am saying we should not have workflowCaller.getWorkflowActionOptions() in RSC.

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean - we should call getWorkflowActionOptions client side and pass to this modal as well (as we are already doing in this PR) , sounds good btw

image
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply let const { data: actionOptions } = trpc.viewer.workflows.getWorkflowActionOptions.useQuery(); just stay in packages/features/ee/workflows/components/AddActionDialog.tsx unless you have a better optimization in mind.

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply let const { data: actionOptions } = trpc.viewer.workflows.getWorkflowActionOptions.useQuery(); just stay in packages/features/ee/workflows/components/AddActionDialog.tsx unless you have a better optimization in mind.

Then we'll end up doing two calls for actionOptions as soon as the page renders (current behaviour on prod) - one in WorkflowDetailsPage.tsx and another in AddActionDialog.tsx (we require this data on both places)

I have updated it to call just once just inside WorkflowDetailsPage and simply pass it to AddActionDialog as prop
Thoughts ?

Copy link
Contributor

@hbjORbj hbjORbj Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good but is it necessary? Client-side trpc queries are cached, no? especially since these two queries happen almost at the same time?

I love to avoid prop drilling as much as possible unless it's necessary

Would be great to have you test this quickly 🙏

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API calls increases making them look lot more - having one looked clean to me , anyways I have added an trpc call in the Dialog as well


const formSchema = z.object({
Expand Down
23 changes: 23 additions & 0 deletions packages/features/ee/workflows/components/WorkFlowFormSkeleton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { SkeletonText } from "@calcom/ui/components/skeleton";

export default function WorkFlowFormSkeleton() {
return (
<div className="min-w-80 bg-default border-subtle w-full space-y-6 rounded-md border p-7">
<div className="flex items-center gap-4">
<SkeletonText className="w-6 rounded-full" />
<div className="flex w-full flex-col gap-2">
<SkeletonText className="w-28" />
<SkeletonText className="w-40" />
</div>
</div>
<div className="border-subtle border-t" />
{Array.from({ length: 2 }).map((_, idx) => (
<div key={idx} className="space-y-2">
<SkeletonText className="w-28" />
<SkeletonText className="w-full" />
</div>
))}
<SkeletonText className="w-full" />
</div>
);
}
42 changes: 32 additions & 10 deletions packages/features/ee/workflows/components/WorkflowDetailsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import { useState, useEffect } from "react";
import type { UseFormReturn } from "react-hook-form";
import { Controller } from "react-hook-form";

import WorkFlowFormSkeleton from "@calcom/features/ee/workflows/components/WorkFlowFormSkeleton";
import { SENDER_ID, SENDER_NAME, SCANNING_WORKFLOW_STEPS } from "@calcom/lib/constants";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import type { WorkflowActions } from "@calcom/prisma/enums";
import { WorkflowTemplates } from "@calcom/prisma/enums";
import { trpc } from "@calcom/trpc/react";
import type { RouterOutputs } from "@calcom/trpc/react";
import { InfoBadge } from "@calcom/ui/components/badge";
import { Button } from "@calcom/ui/components/button";
Expand All @@ -24,6 +26,8 @@ import WorkflowStepContainer from "./WorkflowStepContainer";
type User = RouterOutputs["viewer"]["me"]["get"];

interface Props {
verifiedNumbers: RouterOutputs["viewer"]["workflows"]["getVerifiedNumbers"] | undefined;
verifiedEmails: RouterOutputs["viewer"]["workflows"]["getVerifiedEmails"] | undefined;
form: UseFormReturn<FormValues>;
workflowId: number;
selectedOptions: Option[];
Expand All @@ -36,10 +40,22 @@ interface Props {
}

export default function WorkflowDetailsPage(props: Props) {
const { form, workflowId, selectedOptions, setSelectedOptions, teamId, isOrg, allOptions } = props;
const {
form,
workflowId,
verifiedEmails,
verifiedNumbers,
selectedOptions,
setSelectedOptions,
teamId,
isOrg,
allOptions,
} = props;
const { t } = useLocale();
const router = useRouter();

const { data: actionOptions } = trpc.viewer.workflows.getWorkflowActionOptions.useQuery();

const [isAddActionDialogOpen, setIsAddActionDialogOpen] = useState(false);

const [reload, setReload] = useState(false);
Expand Down Expand Up @@ -175,21 +191,26 @@ export default function WorkflowDetailsPage(props: Props) {

{/* Workflow Trigger Event & Steps */}
<div className="bg-muted border-subtle w-full rounded-md border p-3 py-5 md:ml-3 md:p-8">
{form.getValues("trigger") && (
<div>
<WorkflowStepContainer
form={form}
user={props.user}
teamId={teamId}
readOnly={props.readOnly}
/>
</div>
{form.getValues("trigger") ? (
<WorkflowStepContainer
verifiedNumbers={verifiedNumbers}
verifiedEmails={verifiedEmails}
form={form}
user={props.user}
teamId={teamId}
readOnly={props.readOnly}
actionOptions={actionOptions}
/>
) : (
<WorkFlowFormSkeleton />
)}
{form.getValues("steps") && (
<>
{form.getValues("steps")?.map((step) => {
return (
<WorkflowStepContainer
verifiedNumbers={verifiedNumbers}
verifiedEmails={verifiedEmails}
key={step.id}
form={form}
user={props.user}
Expand All @@ -198,6 +219,7 @@ export default function WorkflowDetailsPage(props: Props) {
setReload={setReload}
teamId={teamId}
readOnly={props.readOnly}
actionOptions={actionOptions}
/>
);
})}
Expand Down
28 changes: 17 additions & 11 deletions packages/features/ee/workflows/components/WorkflowStepContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,16 @@ import { TimeTimeUnitInput } from "./TimeTimeUnitInput";
type User = RouterOutputs["viewer"]["me"]["get"];

type WorkflowStepProps = {
verifiedNumbers?: RouterOutputs["viewer"]["workflows"]["getVerifiedNumbers"] | undefined;
verifiedEmails?: RouterOutputs["viewer"]["workflows"]["getVerifiedEmails"] | undefined;
step?: WorkflowStep;
form: UseFormReturn<FormValues>;
user: User;
reload?: boolean;
setReload?: Dispatch<SetStateAction<boolean>>;
teamId?: number;
readOnly: boolean;
actionOptions?: RouterOutputs["viewer"]["workflows"]["getWorkflowActionOptions"];
};

const getTimeSectionText = (trigger: WorkflowTriggerEvents, t: TFunction) => {
Expand All @@ -80,20 +83,22 @@ export default function WorkflowStepContainer(props: WorkflowStepProps) {
const { t, i18n } = useLocale();
const utils = trpc.useUtils();

const { step, form, reload, setReload, teamId } = props;
const { data: _verifiedNumbers } = trpc.viewer.workflows.getVerifiedNumbers.useQuery(
{ teamId },
{ enabled: !!teamId }
);
const {
verifiedNumbers: _verifiedNumbers,
verifiedEmails,
step,
form,
reload,
setReload,
teamId,
actionOptions,
} = props;

const { hasActiveTeamPlan } = useHasActiveTeamPlan();

const { data: _verifiedEmails } = trpc.viewer.workflows.getVerifiedEmails.useQuery({ teamId });

const timeFormat = getTimeFormatStringFromUserTimeFormat(props.user.timeFormat);

const verifiedNumbers = _verifiedNumbers?.map((number) => number.phoneNumber) || [];
const verifiedEmails = _verifiedEmails || [];
const [isAdditionalInputsDialogOpen, setIsAdditionalInputsDialogOpen] = useState(false);

const [verificationCode, setVerificationCode] = useState("");
Expand Down Expand Up @@ -125,7 +130,6 @@ export default function WorkflowStepContainer(props: WorkflowStepProps) {

const [timeSectionText, setTimeSectionText] = useState(getTimeSectionText(form.getValues("trigger"), t));

const { data: actionOptions } = trpc.viewer.workflows.getWorkflowActionOptions.useQuery();
const triggerOptions = getWorkflowTriggerOptions(t);
const templateOptions = getWorkflowTemplateOptions(t, step?.action, hasActiveTeamPlan);
if (step && !form.getValues(`steps.${step.stepNumber - 1}.reminderBody`)) {
Expand Down Expand Up @@ -165,13 +169,15 @@ export default function WorkflowStepContainer(props: WorkflowStepProps) {

const getEmailVerificationStatus = () =>
!!step &&
!!verifiedEmails.find((email: string) => email === form.getValues(`steps.${step.stepNumber - 1}.sendTo`));
!!verifiedEmails?.find(
(email: string) => email === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)
);

const [numberVerified, setNumberVerified] = useState(getNumberVerificationStatus());
const [emailVerified, setEmailVerified] = useState(getEmailVerificationStatus());

useEffect(() => setNumberVerified(getNumberVerificationStatus()), [verifiedNumbers.length]);
useEffect(() => setEmailVerified(getEmailVerificationStatus()), [verifiedEmails.length]);
useEffect(() => setEmailVerified(getEmailVerificationStatus()), [verifiedEmails?.length]);

const addVariableEmailSubject = (variable: string) => {
if (step) {
Expand Down
Loading
Loading