refactor: Migrate App Store Pages/Components to Features Package#23636
refactor: Migrate App Store Pages/Components to Features Package#23636
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughProject-wide refactor updating import paths from @calcom/app-store/routing-forms/... to @calcom/features/routing-forms/.... Changes span web app pages, dialogs, server-side props, insights module, features (Segment, form-builder), a route-builder page, TRPC handler, and server-side utilities/tests. Specific modules moved include page configs, components (FormInputFields, TestForm), react-awesome-query-builder widgets/config, and URL param utilities. One import switched between relative and absolute pathing. Import order adjusted in one file. No code logic, types, exports, or runtime behavior modified beyond module resolution. Possibly related PRs
✨ 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. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/features/routing-forms/pages/route-builder/[...appPages].tsx (3)
399-404: Fix stale state: useEffect missing deps and sets state only onceInitialize/customize slug when eventOptions or action value changes.
-useEffect(() => { - const isCustom = - !isRouter(route) && !eventOptions.find((eventOption) => eventOption.value === route.action.value); - setCustomEventTypeSlug(isCustom && !isRouter(route) ? route.action.value.split("/").pop() ?? "" : ""); -}, []); +useEffect(() => { + const isCustom = + !isRouter(route) && + !eventOptions.find((eventOption) => eventOption.value === route.action.value); + setCustomEventTypeSlug(isCustom ? route.action.value.split("/").pop() ?? "" : ""); +}, [eventOptions, route.action?.value]);
526-533: Localize hardcoded UI strings per guidelinesReplace literal strings with t(). Examples below; apply similarly across file.
-<span className="text-emphasis ml-2 text-sm font-medium">Conditions</span> +<span className="text-emphasis ml-2 text-sm font-medium">{t("conditions")}</span>-<span className="text-emphasis ml-2 text-sm font-medium">Fallback</span> +<span className="text-emphasis ml-2 text-sm font-medium">{t("fallback")}</span>-<span className="text-emphasis ml-2 text-sm font-medium">Send booker to</span> +<span className="text-emphasis ml-2 text-sm font-medium">{t("send_booker_to")}</span>- action.value = "We are not ready for you yet :("; + action.value = t("not_ready_yet_message");- placeholder="https://example.com" + placeholder={t("example_url_placeholder")}- placeholder="event-url" + placeholder={t("event_url_placeholder")}- header="Create your first route" - text="Routes determine where your form responses will be sent based on the answers provided." + header={t("create_your_first_route")} + text={t("routes_determine_destination_info")}Also applies to: 795-801, 611-612, 638-641, 709-723, 771-775, 1324-1333
1-1: Remove leftover app-store importRemove the line
import { routingFormAppComponents } from "@calcom/app-store/routing-forms/appComponents";from packages/features/routing-forms/pages/route-builder/[...appPages].tsx (line 33).
packages/lib/server/getRoutedUrl.ts (1)
97-99: Replaceincludewith top‐levelselectinfindFormByIdIncludeUserTeamAndOrg
The query in packages/lib/server/repository/PrismaRoutingFormRepository.ts (around line 45) still usesinclude. Per Prisma hygiene guidelines, switch to a root‐levelselectand explicitly list only the needed fields to avoid returning unintended data.packages/features/form-builder/Components.tsx (2)
268-274: Localize "Remove email"Hardcoded UI text should use t().
- <Tooltip content="Remove email"> + <Tooltip content={t("remove_email")}>
426-429: Add rel to external link opened in new tabPrevent reverse tabnabbing.
- <a href={label} target="_blank"> + <a href={label} target="_blank" rel="noopener noreferrer">
🧹 Nitpick comments (10)
packages/features/routing-forms/pages/route-builder/[...appPages].tsx (1)
111-114: hasRules should return a booleanCurrent implementation returns a number or undefined; make it explicit.
-const hasRules = (route: EditFormRoute) => { - if (isRouter(route)) return false; - route.queryValue.children1 && Object.keys(route.queryValue.children1).length; -}; +const hasRules = (route: EditFormRoute) => { + if (isRouter(route)) return false; + const count = route.queryValue.children1 + ? Object.keys(route.queryValue.children1).length + : 0; + return count > 0; +};apps/web/modules/insights/insights-virtual-queues-view.tsx (3)
17-23: Avoid setState during render; derive initial selection in an effectPrevents render-time state updates and double renders.
-const [selectedForm, setSelectedForm] = useState<RoutingForm | undefined>( - routingForms && routingForms.length > 0 ? routingForms[0] : undefined -); - -if (routingForms && !selectedForm && routingForms.length > 0) { - setSelectedForm(routingForms[0]); -} +const [selectedForm, setSelectedForm] = useState<RoutingForm | undefined>(undefined); +useEffect(() => { + if (routingForms?.length && !selectedForm) setSelectedForm(routingForms[0]); +}, [routingForms, selectedForm]);
29-31: Localize placeholder textPer tsx localization guideline.
-<Select - placeholder="Select project" +<Select + placeholder={t("select_project")}
51-52: Remove unreachable codeDead return after earlier return.
- return <></>;packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (2)
79-104: Use Prisma select instead of includePer guidelines, select only fields required (form base fields + team/user selections you access). This reduces overfetch and risk of leaking columns.
Would you like me to propose a precise select shape based on getSerializableForm and downstream usage?
331-332: Prefer named export over default exportImproves tree-shaking and refactor safety.
-export default findTeamMembersMatchingAttributeLogicOfRouteHandler; +// export { findTeamMembersMatchingAttributeLogicOfRouteHandler }; // follow-up change, update imports accordinglypackages/features/Segment.tsx (1)
194-195: Localize “Loading…”Align with tsx localization guideline.
- if (isPending) return <span>Loading...</span>; + if (isPending) return <span>{t("loading")}</span>;packages/lib/server/getRoutedUrl.ts (1)
189-190: Use structured logging instead of console.log for server timing.Prefer the existing
loggerinstance (with appropriate level) overconsole.logto keep logs consistent and filterable.- console.log("Server-Timing", getServerTimingHeader(timeTaken)); + log.info({ serverTiming: getServerTimingHeader(timeTaken) }, "Server-Timing");apps/web/components/dialog/RerouteDialog.tsx (1)
361-363: Localize user-facing strings witht()(frontend guideline).Several UI messages are hard-coded. Wrap them with
t()for localization.- showToast( - "Looks like the timeslot is not available for the new team members. Try rescheduling with different timeslot", - "error" - ); + showToast(t("timeslot_unavailable_try_different_timeslot"), "error");- showToast("You cannot reschedule to a past timeslot", "error"); + showToast(t("cannot_reschedule_to_past_timeslot"), "error");- showToast( - "We don't have ID for the new event. Please go to the Routing Form and save it again.", - "error" - ); + showToast(t("no_chosen_event_type_id_please_save_form_again"), "error");- <span className="text-attention"> - <span>Continue with rerouting in the new</span>{" "} - <a - href="javascript:void(0)" - className="text-attention underline" - onClick={() => reroutingState.value?.reschedulerWindow?.focus()}> - tab - </a> - </span> + <span className="text-attention"> + <span>{t("continue_with_rerouting_in_the_new")}</span>{" "} + <a + href="javascript:void(0)" + className="text-attention underline" + onClick={() => reroutingState.value?.reschedulerWindow?.focus()}> + {t("tab")} + </a> + </span>Also applies to: 409-411, 499-503, 645-653
apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx (1)
20-20: Localize hardcoded "Form" in metadata titleWrap the literal with t() to comply with i18n guidelines.
- (t) => (mainPage === "routing-link" ? `Form | Cal.com Forms` : `${t("routing_forms")} | Cal.com Forms`), + (t) => `${mainPage === "routing-link" ? t("form") : t("routing_forms")} | Cal.com Forms`,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx(1 hunks)apps/web/app/(use-page-wrapper)/apps/routing-forms/forms/[[...pages]]/page.tsx(1 hunks)apps/web/components/dialog/RerouteDialog.tsx(1 hunks)apps/web/components/dialog/__tests__/RerouteDialog.test.tsx(1 hunks)apps/web/lib/apps/routing-forms/[...pages]/getServerSideProps.ts(1 hunks)apps/web/modules/insights/insights-virtual-queues-view.tsx(1 hunks)packages/features/Segment.tsx(1 hunks)packages/features/form-builder/Components.tsx(1 hunks)packages/features/routing-forms/pages/route-builder/[...appPages].tsx(1 hunks)packages/lib/server/getLuckyUser.test.ts(1 hunks)packages/lib/server/getRoutedUrl.test.ts(2 hunks)packages/lib/server/getRoutedUrl.ts(1 hunks)packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts(1 hunks)
🧰 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:
apps/web/components/dialog/RerouteDialog.tsxpackages/features/routing-forms/pages/route-builder/[...appPages].tsxpackages/features/Segment.tsxapps/web/modules/insights/insights-virtual-queues-view.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/forms/[[...pages]]/page.tsxapps/web/components/dialog/__tests__/RerouteDialog.test.tsxpackages/features/form-builder/Components.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/components/dialog/RerouteDialog.tsxpackages/features/routing-forms/pages/route-builder/[...appPages].tsxpackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/getLuckyUser.test.tspackages/lib/server/getRoutedUrl.tspackages/features/Segment.tsxapps/web/modules/insights/insights-virtual-queues-view.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/forms/[[...pages]]/page.tsxapps/web/components/dialog/__tests__/RerouteDialog.test.tsxpackages/lib/server/getRoutedUrl.test.tspackages/features/form-builder/Components.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsxapps/web/lib/apps/routing-forms/[...pages]/getServerSideProps.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:
apps/web/components/dialog/RerouteDialog.tsxpackages/features/routing-forms/pages/route-builder/[...appPages].tsxpackages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/getLuckyUser.test.tspackages/lib/server/getRoutedUrl.tspackages/features/Segment.tsxapps/web/modules/insights/insights-virtual-queues-view.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/forms/[[...pages]]/page.tsxapps/web/components/dialog/__tests__/RerouteDialog.test.tsxpackages/lib/server/getRoutedUrl.test.tspackages/features/form-builder/Components.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsxapps/web/lib/apps/routing-forms/[...pages]/getServerSideProps.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/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.tspackages/lib/server/getLuckyUser.test.tspackages/lib/server/getRoutedUrl.tspackages/lib/server/getRoutedUrl.test.tsapps/web/lib/apps/routing-forms/[...pages]/getServerSideProps.ts
🧠 Learnings (4)
📚 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:
apps/web/components/dialog/RerouteDialog.tsxapps/web/modules/insights/insights-virtual-queues-view.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx
📚 Learning: 2025-08-29T22:57:30.382Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23454
File: packages/features/form-builder/FormBuilder.tsx:11-11
Timestamp: 2025-08-29T22:57:30.382Z
Learning: FormBuilder.tsx in packages/features/form-builder/ does not have "use client" directive at the top despite using client-side React hooks and event handlers, which suggests it should be a client component.
Applied to files:
apps/web/components/dialog/RerouteDialog.tsxpackages/features/routing-forms/pages/route-builder/[...appPages].tsxapps/web/modules/insights/insights-virtual-queues-view.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/forms/[[...pages]]/page.tsxpackages/features/form-builder/Components.tsxapps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx
📚 Learning: 2025-08-20T17:34:35.004Z
Learnt from: mdm317
PR: calcom/cal.com#23221
File: packages/features/schedules/components/NewScheduleButton.tsx:1-1
Timestamp: 2025-08-20T17:34:35.004Z
Learning: In the Cal.com codebase, it's an established pattern for client components to directly import and call server actions like `revalidateAvailabilityList()` from `app/(use-page-wrapper)/(main-nav)/availability/actions` within mutation `onSuccess` callbacks. This pattern is consistently used across availability-view.tsx for delete, update, and bulk update operations, and should be followed for create operations as well to maintain consistency.
Applied to files:
apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx
📚 Learning: 2025-08-20T17:34:35.004Z
Learnt from: mdm317
PR: calcom/cal.com#23221
File: packages/features/schedules/components/NewScheduleButton.tsx:1-1
Timestamp: 2025-08-20T17:34:35.004Z
Learning: In the Cal.com codebase, server actions like `revalidateAvailabilityList()` from `app/(use-page-wrapper)/(main-nav)/availability/actions` are imported and called directly in client components within mutation `onSuccess` callbacks. This pattern is consistently used across availability-related components (both in availability-view.tsx for updates and NewScheduleButton.tsx for creates) to ensure the availability list cache is fresh after mutations.
Applied to files:
apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx
⏰ 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). (3)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Detect changes
🔇 Additional comments (14)
apps/web/modules/insights/insights-virtual-queues-view.tsx (1)
6-6: LGTM: TestForm import migrated to featuresMatches the refactor; usage unchanged.
packages/lib/server/getLuckyUser.test.ts (1)
19-21: LGTM: Updated mock path to featuresMock remains minimal and safe; tests should continue to isolate RAQB widgets.
packages/trpc/server/routers/viewer/routing-forms/findTeamMembersMatchingAttributeLogicOfRoute.handler.ts (1)
12-12: LGTM: getUrlSearchParamsToForwardForTestPreview path migratedConsistent with features namespace.
packages/features/Segment.tsx (1)
11-11: LGTM: uiConfig import migrated to featuresMatches the refactor; consumers unchanged.
apps/web/lib/apps/routing-forms/[...pages]/getServerSideProps.ts (1)
5-5: Import path migration looks good — confirm config surface compatibility.Ensure
@calcom/features/routing-forms/pages/app-routing.server-configexposes the same keys and GSSP signatures as before soroutingServerSidePropsConfig[mainPage]resolves identically at runtime.packages/lib/server/getRoutedUrl.ts (1)
8-17: Mixed namespaces (app-store vs features) — unify or confirm intent.This file now imports
getUrlSearchParamsToForwardfrom@calcom/features/..., while the rest still come from@calcom/app-store/.... Please either migrate the remaining imports or confirm that a split is intentional and won’t cause type/runtime drift across modules.apps/web/components/dialog/__tests__/RerouteDialog.test.tsx (1)
51-82: Test mock path update is correct and aligns with component import.Mock now targets
@calcom/features/routing-forms/components/FormInputFields, matching the component import in RerouteDialog.tsx. Looks good.apps/web/components/dialog/RerouteDialog.tsx (1)
17-21: Confirm exported symbol matches module filename.You import
getUrlSearchParamsToForwardForReroutefrom a module namedgetUrlSearchParamsToForward. Verify that module exports the...ForReroutevariant; if not, import from the.../getUrlSearchParamsToForwardForReroutemodule instead.-} from "@calcom/features/routing-forms/pages/routing-link/getUrlSearchParamsToForward"; +} from "@calcom/features/routing-forms/pages/routing-link/getUrlSearchParamsToForwardForReroute";apps/web/app/(use-page-wrapper)/apps/routing-forms/forms/[[...pages]]/page.tsx (1)
3-3: Import path migration LGTM.
Formsmoved to the features namespace and the page continues to render<Forms appUrl="/routing" />. As this file is a Next.js page, default export is exempt from the named-exports guideline.apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/page.tsx (1)
6-7: Import path migration looks goodFeature namespace switch for client/server config aligns with the refactor; no logic changes.
packages/lib/server/getRoutedUrl.test.ts (2)
13-13: Updated import and mock paths are consistentSwitched both import and vi.mock to features path for getUrlSearchParamsToForward; matches the production change.
Also applies to: 39-39
13-13: No stale imports remain All imports of getUrlSearchParamsToForward now reference "@calcom/features/…" and there are no lingering "@calcom/app-store/…" paths.packages/features/form-builder/Components.tsx (2)
4-4: Widgets/props imports migrated to features pathImport move and reordering are fine; no behavior change.
Also applies to: 8-9
1-4: Confirm client boundaryFile uses useEffect; ensure it’s only imported by client components or add "use client" at the top if it’s directly rendered from a Server Component.
| import { useState } from "react"; | ||
|
|
||
| import { TestForm } from "@calcom/app-store/routing-forms/components/_components/TestForm"; | ||
| import type { RoutingForm } from "@calcom/app-store/routing-forms/types/types"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Migrate RoutingForm type import to features
Keep types and components in the same namespace.
-import type { RoutingForm } from "@calcom/app-store/routing-forms/types/types";
+import type { RoutingForm } from "@calcom/features/routing-forms/types/types";📝 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.
| import type { RoutingForm } from "@calcom/app-store/routing-forms/types/types"; | |
| import type { RoutingForm } from "@calcom/features/routing-forms/types/types"; |
🤖 Prompt for AI Agents
In apps/web/modules/insights/insights-virtual-queues-view.tsx around line 5, the
RoutingForm type is imported from the standalone types path; update the import
to pull RoutingForm from the routing-forms features namespace instead (move
import from "@calcom/app-store/routing-forms/types/types" to the features
entrypoint used by that package) so types and components live in the same
namespace; adjust the import path to the package's features export that
re-exports RoutingForm.
| import { Icon } from "@calcom/ui/components/icon"; | ||
|
|
||
| import { routingFormAppComponents } from "../../appComponents"; | ||
| import { routingFormAppComponents } from "@calcom/app-store/routing-forms/appComponents"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import path should use features namespace for consistency with the PR
Align with the refactor by switching from app-store to features.
-import { routingFormAppComponents } from "@calcom/app-store/routing-forms/appComponents";
+import { routingFormAppComponents } from "@calcom/features/routing-forms/appComponents";📝 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.
| import { routingFormAppComponents } from "@calcom/app-store/routing-forms/appComponents"; | |
| import { routingFormAppComponents } from "@calcom/features/routing-forms/appComponents"; |
🤖 Prompt for AI Agents
In packages/features/routing-forms/pages/route-builder/[...appPages].tsx around
line 33, the import uses the app-store namespace; update the import path to use
the features namespace for consistency (replace
"@calcom/app-store/routing-forms/appComponents" with the corresponding
"@calcom/features/routing-forms/appComponents" import), ensure any surrounding
imports/exports still resolve and run tests/TS build to confirm no path errors.
| ConfigFor, | ||
| } from "@calcom/app-store/routing-forms/components/react-awesome-query-builder/config/uiConfig"; | ||
| } from "@calcom/features/routing-forms/components/react-awesome-query-builder/config/uiConfig"; | ||
| import { getQueryBuilderConfigForAttributes } from "@calcom/app-store/routing-forms/lib/getQueryBuilderConfig"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Migrate getQueryBuilderConfigForAttributes import to features
Avoid mixed namespaces.
-import { getQueryBuilderConfigForAttributes } from "@calcom/app-store/routing-forms/lib/getQueryBuilderConfig";
+import { getQueryBuilderConfigForAttributes } from "@calcom/features/routing-forms/lib/getQueryBuilderConfig";📝 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.
| import { getQueryBuilderConfigForAttributes } from "@calcom/app-store/routing-forms/lib/getQueryBuilderConfig"; | |
| import { getQueryBuilderConfigForAttributes } from "@calcom/features/routing-forms/lib/getQueryBuilderConfig"; |
🤖 Prompt for AI Agents
In packages/features/Segment.tsx around line 12, the module is importing
getQueryBuilderConfigForAttributes from the app-store namespace; change the
import to consume the function from the features package instead. Replace the
current import path with the features export (e.g., import from the features
routing-forms export that re-exports getQueryBuilderConfigForAttributes or from
features/routing-forms/getQueryBuilderConfig), ensuring no references to
@calcom/app-store remain so the feature package owns the symbol.
What does this PR do?
The motivation behind all this migration of directories from App Store package to Features Package is to eradicate
@calcom/trpc/reactimport statements in App Store package. App Store package should NOT import Trpc package.Migration of directories in this PR:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?