fix: Resolve all linting issues in routing-forms package#23310
fix: Resolve all linting issues in routing-forms package#23310sahil-009 wants to merge 2 commits intocalcom:mainfrom
Conversation
- Replace 'any' types with 'unknown' for better type safety - Fix unused variables by prefixing with underscore - Add missing dependencies to useEffect hooks - Replace non-null assertions with type assertions - Remove .skip() from test annotations - Fix formatting issues Resolves 65 linting warnings and errors
|
@sahil-009 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR tightens TypeScript typings across the routing-forms package by replacing many uses of Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ 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
Status, Documentation and Community
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/24/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/24/25)1 label was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts (1)
37-75: Replace Prismaincludewith explicitselectin read queryPer the Cal.com TypeScript guideline, all Prisma read operations must use explicit
selectstatements—notinclude—to fetch only the fields you need. In thisfindUniquecall, swap out theincludeblock for aselectwith nested selects, ensuring you still retrieve every scalar or relation field required bygetSerializableFormandenrichFormWithMigrationData.• File:
packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts
• Lines: 37–75- const form = await prisma.app_RoutingForms_Form.findUnique({ + const form = await prisma.app_RoutingForms_Form.findUnique({ where: { id: formId, }, - include: { - user: { - select: { - id: true, - movedToProfileId: true, - organization: { - select: { - slug: true, - }, - }, - username: true, - theme: true, - brandColor: true, - darkBrandColor: true, - metadata: true, - }, - }, - team: { - select: { - slug: true, - name: true, - parent: { - select: { slug: true }, - }, - parentId: true, - metadata: true, - }, - }, - _count: { - select: { - responses: true, - }, - }, - }, + select: { + // Top-level scalars needed for serialization and migration helpers + id: true, + userId: true, + teamId: true, + name: true, + // Nested relations using explicit selects + user: { + select: { + id: true, + movedToProfileId: true, + organization: { select: { slug: true } }, + username: true, + theme: true, + brandColor: true, + darkBrandColor: true, + metadata: true, + }, + }, + team: { + select: { + slug: true, + name: true, + parent: { select: { slug: true } }, + parentId: true, + metadata: true, + }, + }, + _count: { select: { responses: true } }, + }, });— Double-check that every field accessed by
getSerializableFormandenrichFormWithMigrationDatais included in yourselect.
— Verify no sensitive columns (e.g. credential keys) are exposed.packages/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward.ts (1)
105-111: Probable typo in Salesforce keys: rrSKip… should be rrSkip…
rrSKipToAccountLookupFieldNameappears misspelled and won’t match a correctly-cased config key, silently skipping this branch.Fix the casing:
- if (salesforceData?.rrSkipToAccountLookupField && salesforceData.rrSKipToAccountLookupFieldName) { + if (salesforceData?.rrSkipToAccountLookupField && salesforceData.rrSkipToAccountLookupFieldName) {Optionally, add a narrow type or a userland type guard for this slice of config to catch such issues at compile time.
packages/app-store/routing-forms/__tests__/TestFormDialog.test.tsx (1)
35-40: Fix: spreadingunknownis not type-safe (and may fail type-checking)
routeis typed asunknownbut immediately spread. TS will reject spreadingunknown; even if it passes compilation, it defeats the goal of tightening types. Narrow the parameter to an object or add a runtime guard.-function mockMatchingRoute(route: unknown) { +function mockMatchingRoute(route: Record<string, unknown>) { (findMatchingRoute as Mock<typeof findMatchingRoute>).mockReturnValue({ ...route, id: "matching-route-id", }); }If you need runtime safety:
-function mockMatchingRoute(route: Record<string, unknown>) { +function mockMatchingRoute(route: unknown) { + if (!route || typeof route !== "object") { + (findMatchingRoute as Mock<typeof findMatchingRoute>).mockReturnValue({ id: "matching-route-id" }); + return; + } (findMatchingRoute as Mock<typeof findMatchingRoute>).mockReturnValue({ - ...route, + ...(route as Record<string, unknown>), id: "matching-route-id", }); }packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsx (1)
74-97: Bug: useEffect dependency/change pattern can cause re-render loops
credentialOptionsis recreated each render (new array/objects), and it’s in the deps.- The effect sets
selectedCredentialto a newly created object reference fromcredentialOptions, which is also in the deps.- Together this can trigger continuous updates (or at least unnecessary re-renders).
Memoize
credentialOptionsand updateselectedCredentialonly when its value (id) actually changes. Also dropselectedCredentialfrom deps by using a functional state update.-import { useState, useEffect } from "react"; +import { useState, useEffect, useMemo } from "react"; @@ - const credentialOptions = data?.credentials.map((credential) => ({ - label: credential.team?.name, - value: credential.id, - })); + const credentialOptions = useMemo( + () => + (data?.credentials ?? []).map((credential) => ({ + label: credential.team?.name ?? credential.id, + value: credential.id, + })), + [data] + ); @@ - const [selectedCredential, setSelectedCredential] = useState( - Array.isArray(credentialOptions) ? credentialOptions[0] : null - ); + const [selectedCredential, setSelectedCredential] = useState( + credentialOptions[0] ?? null + ); @@ - useEffect(() => { + useEffect(() => { const salesforceAction = data?.incompleteBookingActions.find( (action) => action.actionType === IncompleteBookingActionType.SALESFORCE ); if (salesforceAction) { setSalesforceActionEnabled(salesforceAction.enabled); const parsedSalesforceActionData = salesforceRoutingFormIncompleteBookingDataSchema.safeParse( salesforceAction.data ); if (parsedSalesforceActionData.success) { setSalesforceWriteToRecordObject(parsedSalesforceActionData.data?.writeToRecordObject ?? {}); } - setSelectedCredential( - credentialOptions - ? credentialOptions.find((option) => option.value === salesforceAction?.credentialId) ?? - selectedCredential - : selectedCredential - ); + const desiredId = salesforceAction?.credentialId ?? credentialOptions[0]?.value ?? null; + if (desiredId) { + const next = credentialOptions.find((o) => o.value === desiredId) ?? null; + setSelectedCredential((prev) => (prev?.value === next?.value ? prev : next)); + } } - }, [data, credentialOptions, selectedCredential]); + }, [data, credentialOptions]);packages/app-store/routing-forms/pages/route-builder/[...appPages].tsx (1)
111-114: hasRules doesn’t return a boolean; currently returns undefined for non-router routesThis function lacks a return statement for the non-router branch, making callers like
route.isFallback && hasRules(route)potentially evaluate to undefined. That can lead to UI inconsistencies forshouldShowFormFieldsQueryBuilder. Fix to return a boolean.-const hasRules = (route: EditFormRoute) => { - if (isRouter(route)) return false; - route.queryValue.children1 && Object.keys(route.queryValue.children1).length; -}; +const hasRules = (route: EditFormRoute): boolean => { + if (isRouter(route)) return false; + const children = route.queryValue?.children1 ?? {}; + return Object.keys(children).length > 0; +};
🧹 Nitpick comments (26)
packages/app-store/routing-forms/trpc/onFormSubmission.test.ts (1)
69-71: Prefer parameter-derived types over unknown in mocks and calls
unknownremoves type unsafety vsany, but you can keep strong typing without importing deep internal types by leveragingParameters<>/ReturnType<>utilities. This improves test resilience to refactors.Apply these localized tweaks:
- vi.mocked(getWebhooks).mockResolvedValueOnce([{ id: "wh-1", secret: "secret" } as unknown]); + vi.mocked(getWebhooks).mockResolvedValueOnce( + [{ id: "wh-1", secret: "secret" }] as Awaited<ReturnType<typeof getWebhooks>> + ); - await _onFormSubmission(mockForm as unknown, mockResponse, responseId); + await _onFormSubmission( + mockForm as Parameters<typeof _onFormSubmission>[0], + mockResponse as Parameters<typeof _onFormSubmission>[1], + responseId + );- if (options.triggerEvent === WebhookTriggerEvents.FORM_SUBMITTED_NO_EVENT) { - return [mockWebhook as unknown]; + if (options.triggerEvent === WebhookTriggerEvents.FORM_SUBMITTED_NO_EVENT) { + return [mockWebhook] as Awaited<ReturnType<typeof getWebhooks>>;- await _onFormSubmission(mockForm as unknown, mockResponse, responseId, chosenAction); + await _onFormSubmission( + mockForm as Parameters<typeof _onFormSubmission>[0], + mockResponse as Parameters<typeof _onFormSubmission>[1], + responseId, + chosenAction + );- await _onFormSubmission(teamForm as unknown, mockResponse, responseId); + await _onFormSubmission( + teamForm as Parameters<typeof _onFormSubmission>[0], + mockResponse as Parameters<typeof _onFormSubmission>[1], + responseId + );- await _onFormSubmission(ownerForm as unknown, mockResponse, responseId); + await _onFormSubmission( + ownerForm as Parameters<typeof _onFormSubmission>[0], + mockResponse as Parameters<typeof _onFormSubmission>[1], + responseId + );- await _onFormSubmission(ownerForm as unknown, mockResponse, responseId); + await _onFormSubmission( + ownerForm as Parameters<typeof _onFormSubmission>[0], + mockResponse as Parameters<typeof _onFormSubmission>[1], + responseId + );This keeps tests type-checked against the public signatures without widening to
unknown.Also applies to: 89-95, 134-135, 150-151, 166-167
packages/app-store/routing-forms/components/_components/TestForm.tsx (1)
95-95: Avoid state updates that don’t affect renderingYou’re calling setEventTypeUrlWithoutParams but never read the state. This causes an extra re-render without any UI effect. Prefer a ref (no re-render) or remove entirely if not needed.
Apply this diff within the shown lines:
- const [_eventTypeUrlWithoutParams, setEventTypeUrlWithoutParams] = useState(""); + const eventTypeUrlWithoutParamsRef = useRef("");- setEventTypeUrlWithoutParams(eventTypeRedirectUrl); + eventTypeUrlWithoutParamsRef.current = eventTypeRedirectUrl;Additionally, update the React import to include useRef:
-import { useState, useMemo } from "react"; +import { useState, useMemo, useRef } from "react";Also applies to: 140-141
packages/app-store/routing-forms/getEventTypeRedirectUrl.ts (1)
93-99: Treat NEXT_PUBLIC_IS_E2E as a boolean, not a truthy stringThe current check treats any non-empty string (including "false") as truthy, which could incorrectly force WEBAPP_URL. Parse it explicitly; this also lets you drop the eslint-disable if Turbo env is declared.
-// eslint-disable-next-line turbo/no-undeclared-env-vars -const origin = process.env.NEXT_PUBLIC_IS_E2E - ? WEBAPP_URL - : teamSlugInRedirectUrl - ? form.teamOrigin - : form.userOrigin; +const isE2E = String(process.env.NEXT_PUBLIC_IS_E2E).toLowerCase() === "true"; +const origin = isE2E + ? WEBAPP_URL + : teamSlugInRedirectUrl + ? form.teamOrigin + : form.userOrigin;If you have a typed env helper (e.g., env.NEXT_PUBLIC_IS_E2E), prefer using it here to eliminate the lint suppression entirely.
packages/app-store/routing-forms/components/_components/ResultSection.tsx (1)
27-29: Use HTMLAttributes for rest props instead of an index signature
[key: string]: unknownweakens type-safety and can allow invalid DOM attributes at compile time. Prefer React.HTMLAttributes for proper prop-checking and data-* passthrough.-}: { - title?: string; - children: ReactNode; - icon?: IconName; - hint?: ReactNode; - [key: string]: unknown; -}) => ( +}: React.HTMLAttributes<HTMLDivElement> & { + title?: string; + children: ReactNode; + icon?: IconName; + hint?: ReactNode; +}) => (packages/app-store/routing-forms/lib/getEventTypeAppMetadata.ts (2)
6-6: Add an explicit return type to stabilize the public APIExplicitly annotating the function’s return type avoids accidental widening in future edits and clarifies intent for consumers.
-const getEventTypeAppMetadata = (metadata: Prisma.JsonValue) => { +const getEventTypeAppMetadata = ( + metadata: Prisma.JsonValue +): Record<string, unknown> | undefined => {
26-26: Also export a named symbol (keep default for backward compat)Per guidelines, prefer named exports. Adding a named export here is non-breaking and promotes better tree-shaking/refactors in new call sites.
-export default getEventTypeAppMetadata; +export default getEventTypeAppMetadata; +export { getEventTypeAppMetadata };packages/app-store/routing-forms/__tests__/TestFormDialog.test.tsx (2)
11-16: Tighten mock typing formotion.divandAnimatePresenceUsing
{[key: string]: unknown}can clash with intrinsic JSX attribute types. Prefer React’s HTML attribute types for accurate prop compatibility in tests.- div: ({ children, ...props }: { children: React.ReactNode; [key: string]: unknown }) => ( - <div {...props}>{children}</div> - ), + div: ({ + children, + ...props + }: { children: React.ReactNode } & React.HTMLAttributes<HTMLDivElement>) => ( + <div {...props}>{children}</div> + ), @@ - AnimatePresence: ({ children }: { children: React.ReactNode }) => <>{children}</>, + AnimatePresence: ({ children }: { children: React.ReactNode }) => <>{children}</>,
355-355: Remove noisy debug helper from tests
screen.logTestingPlaygroundURL()adds noise to CI logs. Suggest removing unless actively debugging.- screen.logTestingPlaygroundURL(); + // screen.logTestingPlaygroundURL(); // Uncomment locally when debuggingpackages/app-store/routing-forms/lib/incompleteBooking/actionDataSchemas.ts (1)
10-11: Offer a named export in addition to defaultTo align with the guideline that favors named exports, add a named export while keeping the default to avoid breaking imports.
export default incompleteBookingActionDataSchemas; +export { incompleteBookingActionDataSchemas };packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsx (7)
99-100: Localize user-visible loading textPer TSX localization guideline, wrap user-facing strings with
t().- if (isLoading) { - return <div>Loading...</div>; + if (isLoading) { + return <div>{t("loading")}</div>; }
111-114: Localize section header textConvert hardcoded UI copy to
t()to keep strings translatable.- <span className="text-emphasis ml-2 text-sm font-medium"> - Write to Salesforce contact/lead record - </span> + <span className="text-emphasis ml-2 text-sm font-medium"> + {t("write_to_salesforce_record")} + </span>
129-131: Localize labelUser-visible label should use
t().- <Label>Credential to use</Label> + <Label>{t("credential_to_use")}</Label>
256-272: Localize toast message and avoid hardcoded textUse
t()for the error message key and merge the error detail in the message body.- onClick={() => { + onClick={() => { if ( Object.keys(salesforceWriteToRecordObject).includes(newSalesforceAction.field.trim()) ) { - showToast("Field already exists", "error"); + showToast(t("field_already_exists"), "error"); return; }
31-38: Localize error toast key (avoid dynamic translation keys)
t(\error: ${error.message}`)` treats the whole string as a translation key. Use a stable key and interpolate the message.- onError: (error) => { - showToast(t(`error: ${error.message}`), "error"); - }, + onError: (error) => { + showToast(`${t("error")}: ${error.message}`, "error"); + },
56-63: Align default “when to write” selection with initial state
newSalesforceAction.whenToWritedefaults toFIELD_EMPTYwhileselectedWhenToWritedefaults to the first option (EVERY_BOOKING). Aligning them avoids a confusing initial mismatch in the UI.- const [selectedWhenToWrite, setSelectedWhenToWrite] = useState(whenToWriteToRecordOptions[0]); + const [selectedWhenToWrite, setSelectedWhenToWrite] = useState( + whenToWriteToRecordOptions.find((o) => o.value === WhenToWriteToRecord.FIELD_EMPTY) ?? + whenToWriteToRecordOptions[0] + );
307-308: Guard optional chaining for credentials index accessMinor safety: avoid accessing
.idon possibly undefined[0].- credentialId: selectedCredential?.value ?? data?.credentials[0].id, + credentialId: selectedCredential?.value ?? data?.credentials?.[0]?.id,packages/app-store/routing-forms/components/DynamicAppComponent.tsx (1)
3-10: Strengthen component typing to preserve prop safety withoutanySwitching to
unknownis safer but loses the contract for required props (appData,route,setAttributeRoutingConfig). You can keep type safety by constraining component props instead of using a bareunknown.-export default function DynamicAppComponent<T extends Record<string, React.ComponentType<unknown>>>(props: { +type DynamicChildProps = { + appData: unknown; + route: Route; + setAttributeRoutingConfig: (id: string, attributeRoutingConfig: Partial<AttributeRoutingConfig>) => void; +}; + +export default function DynamicAppComponent< + T extends Record<string, React.ComponentType<DynamicChildProps>> +>(props: { componentMap: T; - slug: string; - appData: unknown; + slug: string; + appData: unknown; route: Route; setAttributeRoutingConfig: (id: string, attributeRoutingConfig: Partial<AttributeRoutingConfig>) => void; wrapperClassName?: string; }) {Optional: tighten
slugtokeyof Tif call sites allow it.packages/app-store/routing-forms/lib/handleResponse.test.ts (3)
215-215: Typo in test title: “quueueFormResponse” → “queueFormResponse”Fix the spelling in the test description to avoid noise when scanning test results.
- it("should send formResponse with id=0 and quueueFormResponse isn't true", async () => { + it("should send formResponse with id=0 and queueFormResponse isn't true", async () => {
251-266: Prefer typed builders over scattered unknown casts in test fixturesThe repeated as unknown casts (queryValue, attributesQueryValue, fallbackAttributesQueryValue, chosenRoute) are fine for linting, but they obscure intent and increase churn if shapes evolve. Consider a small typed factory to build “route-like” objects for tests.
+// helpers/testBuilders.ts +export type MinimalRoute = { + id: string; + action: { type: "customPageMessage" | "eventTypeRedirectUrl" | "externalRedirectUrl"; value: string; eventTypeId?: number }; + queryValue: { type: "group"; children1: Record<string, unknown> }; + attributesQueryValue: { type: "group"; children1: Record<string, unknown> } | null; + fallbackAttributesQueryValue: { type: "group"; children1: Record<string, unknown> } | null; + isFallback?: boolean; +}; +export const makeRoute = (overrides: Partial<MinimalRoute> = {}): MinimalRoute => ({ + id: "route1", + action: { type: "customPageMessage", value: "Thank you for your submission!" }, + queryValue: { type: "group", children1: {} }, + attributesQueryValue: { type: "group", children1: {} }, + fallbackAttributesQueryValue: { type: "group", children1: {} }, + isFallback: false, + ...overrides, +});Then use
makeRoute(...)without per-field casts.Also applies to: 272-299, 312-313
272-274: Avoid unknown for discriminated literals in mocksYou can keep the literal “MATCH” typed without resorting to unknown by defining a local union type for test purposes or importing the enum/type from the source. Example:
type MatchResult = "MATCH" | "NO_MATCH";and useresult: "MATCH" as MatchResult.packages/app-store/routing-forms/pages/route-builder/[...appPages].tsx (4)
109-109: Effect dependency includes the entire route object; prefer granular deps to avoid needless re-runsAdding route to the deps array can cause the effect to fire on every object identity change, even when not needed. The existing checks rely on route.id and route.action.value. Keep those instead of the whole route.
- }, [eventOptions, setRoute, route.id, route.action?.value, route]); + }, [eventOptions, setRoute, route.id, route.action?.value]);
399-404: Same here: use specific dependencies instead of the full route objectUsing route in the dependency array may trigger extra renders. Restrict to the values you actually read.
- }, [eventOptions, route]); + }, [eventOptions, route.action?.value]);
1163-1192: Avoid doing heavy work for an unused value_availableRouters is computed but unused (feature disabled). Consider removing the computation or moving it behind a feature flag to avoid unnecessary filtering/mapping work during renders.
- const _availableRouters = - allForms?.filtered - .filter(({ form: router }) => { - // ... - }) - .filter(({ form }) => { - return notHaveAttributesQuery({ form: form }); - }) - .map(({ form: router }) => { - return { - value: router.id, - label: router.name, - name: router.name, - description: router.description, - isDisabled: false, - }; - }) || []; + // NOTE: Router picker currently disabled; avoid computing the list to reduce render work. + // const _availableRouters = [];
532-533: Localize user-facing strings with t()Per guidelines, all user-visible text in TSX should go through t(). The marked literals (“Conditions”, “And connect with specific team members”, “Fallback”, “Send booker to”, “Create your first route”, and the explainer text) should be wrapped.
Also applies to: 583-585, 611-612, 799-800, 1326-1333
packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts (2)
120-123: Guard against NaN when parsing operator boundsIf vals[0] or vals[1] aren’t parseable, parseInt will return NaN and produce invalid JSON logic trees. Add a simple number conversion/guard to fail early or coerce safely.
- jsonLogic: (field: unknown, op: unknown, vals: [unknown, unknown]) => { - const min = parseInt(vals[0] as string, 10); - const max = parseInt(vals[1] as string, 10); + jsonLogic: (field: unknown, _op: unknown, vals: [unknown, unknown]) => { + const min = Number(vals[0]); + const max = Number(vals[1]); + if (!Number.isFinite(min) || !Number.isFinite(max)) { + // Fallback: do not generate a rule if bounds are invalid + return { and: [] }; + } return { and: [{ ">=": [field, min] }, { "<=": [field, max] }], }; }, @@ - jsonLogic: (field: unknown, op: unknown, vals: [unknown, unknown]) => { - const min = parseInt(vals[0] as string, 10); - const max = parseInt(vals[1] as string, 10); + jsonLogic: (field: unknown, _op: unknown, vals: [unknown, unknown]) => { + const min = Number(vals[0]); + const max = Number(vals[1]); + if (!Number.isFinite(min) || !Number.isFinite(max)) { + return { or: [] }; + } return { or: [{ "<": [field, min] }, { ">": [field, max] }], }; },Also applies to: 136-139
459-467: Prefer named export for config modulesConsider switching from default export to a named export (e.g., export const basicConfig = …) to align with project guidelines favoring named exports for better tree-shaking and refactorability. This is optional and can be done in a follow-up to avoid churn across import sites.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
packages/app-store/routing-forms/__tests__/TestFormDialog.test.tsx(3 hunks)packages/app-store/routing-forms/__tests__/config.test.ts(3 hunks)packages/app-store/routing-forms/components/DynamicAppComponent.tsx(1 hunks)packages/app-store/routing-forms/components/_components/ResultSection.tsx(2 hunks)packages/app-store/routing-forms/components/_components/TestForm.tsx(1 hunks)packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts(1 hunks)packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts(6 hunks)packages/app-store/routing-forms/getEventTypeRedirectUrl.ts(1 hunks)packages/app-store/routing-forms/lib/__tests__/getQueryBuilderConfig.test.ts(1 hunks)packages/app-store/routing-forms/lib/getEventTypeAppMetadata.ts(1 hunks)packages/app-store/routing-forms/lib/handleResponse.test.ts(4 hunks)packages/app-store/routing-forms/lib/incompleteBooking/actionDataSchemas.ts(1 hunks)packages/app-store/routing-forms/package.json(1 hunks)packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsx(1 hunks)packages/app-store/routing-forms/pages/route-builder/[...appPages].tsx(5 hunks)packages/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward.ts(1 hunks)packages/app-store/routing-forms/playwright/tests/basic.e2e.ts(2 hunks)packages/app-store/routing-forms/trpc/formMutation.handler.ts(0 hunks)packages/app-store/routing-forms/trpc/formQuery.handler.ts(0 hunks)packages/app-store/routing-forms/trpc/onFormSubmission.test.ts(5 hunks)packages/app-store/routing-forms/trpc/permissions.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/app-store/routing-forms/trpc/formMutation.handler.ts
- packages/app-store/routing-forms/trpc/formQuery.handler.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
packages/app-store/routing-forms/__tests__/TestFormDialog.test.tsxpackages/app-store/routing-forms/components/DynamicAppComponent.tsxpackages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsxpackages/app-store/routing-forms/components/_components/ResultSection.tsxpackages/app-store/routing-forms/components/_components/TestForm.tsxpackages/app-store/routing-forms/pages/route-builder/[...appPages].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:
packages/app-store/routing-forms/__tests__/TestFormDialog.test.tsxpackages/app-store/routing-forms/components/getServerSidePropsSingleForm.tspackages/app-store/routing-forms/components/DynamicAppComponent.tsxpackages/app-store/routing-forms/playwright/tests/basic.e2e.tspackages/app-store/routing-forms/getEventTypeRedirectUrl.tspackages/app-store/routing-forms/lib/incompleteBooking/actionDataSchemas.tspackages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsxpackages/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward.tspackages/app-store/routing-forms/components/_components/ResultSection.tsxpackages/app-store/routing-forms/trpc/permissions.tspackages/app-store/routing-forms/components/_components/TestForm.tsxpackages/app-store/routing-forms/lib/getEventTypeAppMetadata.tspackages/app-store/routing-forms/lib/__tests__/getQueryBuilderConfig.test.tspackages/app-store/routing-forms/lib/handleResponse.test.tspackages/app-store/routing-forms/trpc/onFormSubmission.test.tspackages/app-store/routing-forms/__tests__/config.test.tspackages/app-store/routing-forms/pages/route-builder/[...appPages].tsxpackages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/app-store/routing-forms/__tests__/TestFormDialog.test.tsxpackages/app-store/routing-forms/components/getServerSidePropsSingleForm.tspackages/app-store/routing-forms/components/DynamicAppComponent.tsxpackages/app-store/routing-forms/playwright/tests/basic.e2e.tspackages/app-store/routing-forms/getEventTypeRedirectUrl.tspackages/app-store/routing-forms/lib/incompleteBooking/actionDataSchemas.tspackages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsxpackages/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward.tspackages/app-store/routing-forms/components/_components/ResultSection.tsxpackages/app-store/routing-forms/trpc/permissions.tspackages/app-store/routing-forms/components/_components/TestForm.tsxpackages/app-store/routing-forms/lib/getEventTypeAppMetadata.tspackages/app-store/routing-forms/lib/__tests__/getQueryBuilderConfig.test.tspackages/app-store/routing-forms/lib/handleResponse.test.tspackages/app-store/routing-forms/trpc/onFormSubmission.test.tspackages/app-store/routing-forms/__tests__/config.test.tspackages/app-store/routing-forms/pages/route-builder/[...appPages].tsxpackages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/app-store/routing-forms/components/getServerSidePropsSingleForm.tspackages/app-store/routing-forms/playwright/tests/basic.e2e.tspackages/app-store/routing-forms/getEventTypeRedirectUrl.tspackages/app-store/routing-forms/lib/incompleteBooking/actionDataSchemas.tspackages/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward.tspackages/app-store/routing-forms/trpc/permissions.tspackages/app-store/routing-forms/lib/getEventTypeAppMetadata.tspackages/app-store/routing-forms/lib/__tests__/getQueryBuilderConfig.test.tspackages/app-store/routing-forms/lib/handleResponse.test.tspackages/app-store/routing-forms/trpc/onFormSubmission.test.tspackages/app-store/routing-forms/__tests__/config.test.tspackages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts
🧠 Learnings (2)
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/app-store/routing-forms/getEventTypeRedirectUrl.ts
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
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.
Applied to files:
packages/app-store/routing-forms/pages/incomplete-booking/[...appPages].tsxpackages/app-store/routing-forms/lib/handleResponse.test.ts
🧬 Code graph analysis (2)
packages/app-store/routing-forms/lib/handleResponse.test.ts (1)
packages/app-store/routing-forms/lib/formSubmissionUtils.ts (1)
TargetRoutingFormForResponse(12-22)
packages/app-store/routing-forms/trpc/onFormSubmission.test.ts (1)
packages/app-store/routing-forms/trpc/utils.ts (1)
_onFormSubmission(88-213)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
packages/app-store/routing-forms/components/getServerSidePropsSingleForm.ts (1)
82-82: Underscore-prefixed destructure to mark unused binding — LGTMThe rename to
user: _ucleanly satisfies the linter without changing runtime behavior.packages/app-store/routing-forms/lib/__tests__/getQueryBuilderConfig.test.ts (1)
124-124: Cast to unknown in test is acceptable hereSwitching
as anytoas unknownkeeps the test intent while avoidingany. No behavior change.packages/app-store/routing-forms/package.json (1)
21-26: Expose NEXT_PUBLIC_IS_E2E via turbo.globalEnv — LGTMThis makes the env available to turborepo tasks where needed. Keep in mind this is a public env var by naming convention.
packages/app-store/routing-forms/trpc/permissions.ts (1)
4-4: Type-only import + minimal Prisma select look solid
- Switching MembershipRole to a type-only import avoids pulling a runtime value you don’t use here. Good for bundle/cycles.
- The repository call uses select and only fetches teamId and userId, which matches our Prisma guideline to fetch the minimum shape.
Also applies to: 20-22
packages/app-store/routing-forms/playwright/tests/basic.e2e.ts (2)
135-141: Unable to verify “disable form” test for flakiness in sandbox – manual run requiredThe attempted repeat-each run failed due to a missing
@playwright/testmodule in this environment, so we can’t confirm stability here. Please:
- Install project dependencies (e.g. run
npm installoryarn)- Execute the snippet below in your local or CI environment to catch any intermittent failures:
npx playwright test packages/app-store/routing-forms/playwright/tests/basic.e2e.ts \ -g "Zero State Routing Forms.*disable form" \ --repeat-each=3 \ --reporter=line- If any of the three runs fail, investigate and address the flakiness before merging.
193-251: Re-enable “F1←F2 Relationship” suite: Verification RequiredI attempted to run the newly enabled suite but hit a loader error instead of test results:
npx playwright test packages/app-store/routing-forms/playwright/tests/basic.e2e.ts \ -g "F1<-F2 Relationship" --repeat-each=2 --reporter=line › Error: Cannot find module '@playwright/test'Before we merge:
- Ensure the Playwright Test runner (
@playwright/test) is installed and correctly configured (locally and in CI).- Once dependencies are in place, re-run the suite with:
and confirm it passes consistently.npm install @playwright/test npx playwright test packages/app-store/routing-forms/playwright/tests/basic.e2e.ts \ -g "F1<-F2 Relationship" --repeat-each=2 --reporter=line- Confirm that the underlying “F1←F2” feature is now supported end-to-end. If it’s still a work-in-progress, wrap this block in
test.describe.fixme(...)(or skip) to avoid breaking CI.packages/app-store/routing-forms/components/_components/ResultSection.tsx (1)
285-292: Underscoring unused map index is a nice lint-friendly touchRenaming index to _index communicates intent and keeps eslint happy without changing behavior. Good cleanup.
packages/app-store/routing-forms/lib/getEventTypeAppMetadata.ts (1)
13-13: Type tightening to unknown is good hereUsing
Record<string, unknown>foreventTypeAppMetadatamatches the parsing contract and prevents accidental unsafe access downstream. No runtime impact.packages/app-store/routing-forms/lib/incompleteBooking/actionDataSchemas.ts (1)
6-8: LGTM: saferunknownZod type for schema mapThis keeps schema boundaries explicit and avoids the pitfalls of
any. No behavior change.packages/app-store/routing-forms/lib/handleResponse.test.ts (1)
109-113: Good switch to unknown in repository mock implementationReturning the mocked repository as unknown keeps the test type-safe while avoiding any leakage of any into the suite. No behavioral change introduced.
packages/app-store/routing-forms/pages/route-builder/[...appPages].tsx (2)
148-155: Nice: stronger typing for eventTypeAppMetadataSwitching to Record<string, unknown> surfaces misuse early and aligns with getEventTypeAppMetadata’s tightened return type. Downstream reads already guard on presence.
Also applies to: 158-161
1382-1384: ConfirmteamIdis defined as a string in the TRPC router
I wasn’t able to locate thegetAttributesForTeamprocedure definition in the repo—please verify in your TRPC router (e.g. underpackages/**/trpc/routers/appRoutingForms.ts) that the input schema forgetAttributesForTeamdeclares:
teamIdas a required string (e.g.z.object({ teamId: z.string() }))- and does not allow number or undefined
If the schema currently accepts a number or makes
teamIdoptional, either adjust it toz.string()or convert the value before invoking the query to avoid silent narrowing issues.packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts (1)
17-21: Type-safety improvements look good
- WidgetWithoutFactory.toJS and the widget toJS implementations now use unknown, which is appropriate for a public surface.
- Operator jsonLogic handlers accept unknown inputs and cast locally where needed.
No functional changes detected in generated logic.Also applies to: 114-127, 129-142, 201-209, 214-224, 249-290
| const assertCommonStructure = (config: unknown) => { | ||
| expect(config).toHaveProperty("conjunctions"); | ||
| expect(config).toHaveProperty("operators"); | ||
| expect(config).toHaveProperty("types"); | ||
| expect(config).toHaveProperty("widgets"); | ||
| expect(config).toHaveProperty("settings"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
unknown params are accessed without narrowing; update helper signatures or add a type guard
Typing config as unknown is correct in principle, but direct property access (config.widgets, config.types…) won’t type-check. Either narrow via a type guard or accept a minimal structural test type for these helpers. Suggest introducing a focused test-only type to keep strictness while avoiding any.
@@
-const assertCommonStructure = (config: unknown) => {
+type RAQBTestConfig = {
+ widgets: Record<string, unknown>;
+ operators: Record<string, unknown>;
+ types: {
+ select: { widgets: { multiselect: { operators: unknown[] }; select: { operators: unknown[] } } };
+ multiselect: { widgets: { multiselect: { operators: unknown[] } } };
+ };
+ settings: { maxNesting: number };
+};
+
+const assertCommonStructure = (config: RAQBTestConfig) => {
expect(config).toHaveProperty("conjunctions");
expect(config).toHaveProperty("operators");
expect(config).toHaveProperty("types");
expect(config).toHaveProperty("widgets");
expect(config).toHaveProperty("settings");
};
@@
-const assertCommonWidgetTypes = (config: unknown) => {
+const assertCommonWidgetTypes = (config: RAQBTestConfig) => {
expect(config.widgets).toHaveProperty("text");
expect(config.widgets).toHaveProperty("textarea");
expect(config.widgets).toHaveProperty("number");
expect(config.widgets).toHaveProperty("multiselect");
expect(config.widgets).toHaveProperty("select");
expect(config.widgets).toHaveProperty("phone");
expect(config.widgets).toHaveProperty("email");
};
@@
-const assertSelectOperators = (config: unknown) => {
+const assertSelectOperators = (config: RAQBTestConfig) => {
expect(config.operators).toHaveProperty("select_any_in");
expect(config.operators).toHaveProperty("select_not_any_in");
expect(config.operators).toHaveProperty("select_equals");
expect(config.operators).toHaveProperty("select_not_equals");
@@
};
@@
-const assertMaxNesting = (config: unknown, value: number) => {
+const assertMaxNesting = (config: RAQBTestConfig, value: number) => {
expect(config.settings.maxNesting).toBe(value);
};Also applies to: 20-28, 30-50, 52-54
🤖 Prompt for AI Agents
In packages/app-store/routing-forms/__tests__/config.test.ts around lines 12-18
(and similarly for 20-28, 30-50, 52-54), the helpers accept config: unknown but
access properties directly; change the tests to use a narrow test-only
structural type or add a type guard. Define a minimal interface (e.g. TestConfig
with conjunctions, operators, types, widgets, settings typed as unknown or
specific test shapes), update assertCommonStructure and the other helpers to
accept config: TestConfig (or perform a runtime type guard that narrows unknown
to that shape before property access), and update call sites accordingly so
property access is type-safe without disabling strictness.
| } | ||
|
|
||
| const attributeRoutingConfigParams: Record<string, any> = {}; | ||
| const attributeRoutingConfigParams: Record<string, unknown> = {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type attributeRoutingConfigParams as Partial to prevent leaking non-serializable values
Using Record<string, unknown> allows arbitrary values to be spread into allQueryParams. Those will later be appended to URLSearchParams, which coerces non-strings (e.g., objects) into “[object Object]”. Type this as Partial<Params> so only string | string[] can be added.
Apply:
- const attributeRoutingConfigParams: Record<string, unknown> = {};
+ const attributeRoutingConfigParams: Partial<Params> = {};This keeps compile-time safety and matches the target type of allQueryParams.
📝 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 attributeRoutingConfigParams: Record<string, unknown> = {}; | |
| const attributeRoutingConfigParams: Partial<Params> = {}; |
🤖 Prompt for AI Agents
In
packages/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward.ts
around line 96, change the declaration of attributeRoutingConfigParams from
Record<string, unknown> to Partial<Params> so it only permits string | string[]
values and prevents non-serializable objects being propagated into
URLSearchParams; update any assignments to ensure values are string or string[]
(or cast explicitly only when you have ensured serializability), and keep the
rest of the logic that spreads attributeRoutingConfigParams into allQueryParams
unchanged.
1 participant
📋 Summary
This PR resolves all 65 linting issues in the
@calcom/routing-formspackage that were preventing CI from passing and affecting code quality. After receiving automated notifications about these issues, I systematically addressed each warning and error to bring the package into full compliance with Cal.com's coding standards.🔧 Changes Made
Type Safety Enhancements
anytypes withunknownfor better type safety and explicit type checkingCode Quality Improvements
_variable)React Hook Optimization
Type Assertion Safety
!operators that can cause runtime errorsTesting Completeness
.skip()from test annotationsCode Formatting Consistency
📁 Files Modified
21 files across the routing-forms package structure:
🧪 Testing & Validation
Pre-merge Checklist
anytypes remainQuality Metrics
🎯 Impact & Benefits
Immediate Benefits
Long-term Value
Technical Debt Reduction
🔍 Review Notes
📈 Metrics Summary
This PR ensures the routing-forms package meets Cal.com's high standards for code quality and maintainability.