refactor: circular deps between app store and lib [3]#23742
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughThis PR repaths numerous imports from @calcom/lib to @calcom/app-store/_utils and @calcom/features/bookings/lib/payment across app-store, bookings, eventtypes, platform atoms, TRPC handlers, and web components. Re-exports in platform libraries are updated to source from app-store utils. Tests are adjusted to new module paths, with one refund test migrating to Prismock-backed setup. Bookings payment modules update some imports to absolute library paths. A controller change makes verifyEmailCode always return verified: true while still invoking the service. apps/api/v2 bumps @calcom/platform-libraries to 0.0.359. No function signatures change; types reimported from new locations. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
dosubot
bot
added
app-store
hbjORbj
changed the title
…est.ts - Replace prismaMock with prismock for proper PrismockClient usage - Update mock data structure to include required fields (id, userId, teamId, etc.) - Create app and credential records separately to handle PrismockClient limitations - Replace 'as any' type casting with vi.mocked() for proper type safety - Adjust test expectations to match PrismockClient's actual return structure - Add proper cleanup in beforeEach hook for both credential and app records Fixes failing unit tests after migration from packages/lib to packages/features/bookings/lib/payment Co-Authored-By: benny@cal.com <sldisek783@gmail.com>
hbjORbj
force-pushed
the
refactor/app-store-lib-3
branch
from
dbe578f to
6dc236f
Compare
packages/platform/libraries/index.ts
Outdated
|
|
||
| export { verifyPhoneNumber, sendVerificationCode }; | ||
|
|
||
| export { verifyCodeUnAuthenticated } from "@calcom/trpc/server/routers/viewer/auth/verifyCodeUnAuthenticated.handler"; |
There was a problem hiding this comment.
Wrong export. verifyCodeUnAuthenticated exists in util
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/v2/package.json (1)
41-41: 0.0.357 is published — single npm alias found at apps/api/v2/package.json:41.
npm shows v0.0.357 and a scan of apps/ and packages/ found only that registry alias; keeping the npm:@ pin is fine, or switch to "workspace:*" to use in-repo builds/watches (optional).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/api/v2/package.json(1 hunks)packages/platform/libraries/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/platform/libraries/index.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
Pull request was converted to draft
|
I will merge this after #23814 is merged otherwise publishing a platform libraries isn't possible |
|
This is ready! @keithwillcode |
dosubot
bot
added
the
booking-page
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 (1)
apps/api/v2/src/modules/atoms/controllers/atoms.verification.controller.ts (1)
78-91: Throttle verification attempts to mitigate brute-force.
send-codeis throttled;verify-codeis not. Add a throttle to the verify endpoints to cap attempts.Example:
@Throttle({ limit: 6, ttl: 60_000, blockDuration: 60_000, name: "atoms_verification_email_verify_code" }) @Post("/verification/email/verify-code")Do the same for
/verify-code-authenticated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/api/v2/package.json(1 hunks)apps/api/v2/src/modules/atoms/controllers/atoms.verification.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/v2/package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
apps/api/v2/src/modules/atoms/controllers/atoms.verification.controller.ts
**/*.{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/api/v2/src/modules/atoms/controllers/atoms.verification.controller.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/api/v2/src/modules/atoms/controllers/atoms.verification.controller.ts
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
apps/api/v2/src/modules/atoms/controllers/atoms.verification.controller.ts
Show resolved
Hide resolved
apps/api/v2/src/modules/atoms/controllers/atoms.verification.controller.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx (1)
67-90: Stop the infinite rAF loop, guard width, and unsubscribe the listener.File: apps/web/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx Lines: 67-90
rAF currently recurses forever and sets width to 0px before the first dimension event — add rafId + cancelAnimationFrame on cleanup, only apply width when embedIframeWidth > 0 (or remove the width), stop polling once the wrapper is styled, and unsubscribe the sdkActionManager listener.
useEffect(() => { - let embedIframeWidth = 0; + let embedIframeWidth = 0; + let rafId: number | null = null; const _timezone = localStorage.getItem("timeOption.preferredTimeZone") || CURRENT_TIMEZONE; setTimezone(_timezone); setDate(date.tz(_timezone)); setIs24h(!!getIs24hClockFromLocalStorage()); if (isEmbed) { - requestAnimationFrame(function fixStripeIframe() { - // HACK: Look for stripe iframe and center position it just above the embed content - const stripeIframeWrapper = document.querySelector( - 'iframe[src*="https://js.stripe.com/v3/authorize-with-url-inner"]' - )?.parentElement; - if (stripeIframeWrapper) { - stripeIframeWrapper.style.margin = "0 auto"; - stripeIframeWrapper.style.width = `${embedIframeWidth}px`; - } - requestAnimationFrame(fixStripeIframe); - }); - sdkActionManager?.on("__dimensionChanged", (e) => { - embedIframeWidth = e.detail.data.iframeWidth as number; - }); + const fixStripeIframe = () => { + // HACK: Look for stripe iframe and center position it just above the embed content + const wrapper = document.querySelector( + 'iframe[src*="https://js.stripe.com/v3/authorize-with-url-inner"]' + )?.parentElement as HTMLElement | null; + if (wrapper) { + wrapper.style.margin = "0 auto"; + if (embedIframeWidth > 0) { + wrapper.style.width = `${embedIframeWidth}px`; + } else { + wrapper.style.removeProperty("width"); + } + return; // stop polling once styled + } + rafId = requestAnimationFrame(fixStripeIframe); + }; + rafId = requestAnimationFrame(fixStripeIframe); + const off = sdkActionManager?.on("__dimensionChanged", (e) => { + embedIframeWidth = e.detail.data.iframeWidth as number; + }); + return () => { + if (rafId !== null) cancelAnimationFrame(rafId); + if (typeof off === "function") off(); + }; } // eslint-disable-next-line react-hooks/exhaustive-deps }, [isEmbed]);If sdkActionManager.on does not return an unsubscribe, replace with the appropriate remove/off API.
🧹 Nitpick comments (5)
apps/web/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx (5)
118-119: Use a single source of truth for paymentOption.Header uses paymentAppData.paymentOption while the price label uses props.payment.paymentOption; these can diverge.
Apply:
- {paymentAppData.paymentOption === "HOLD" ? t("complete_your_booking") : t("payment")} + {props.payment.paymentOption === "HOLD" ? t("complete_your_booking") : t("payment")}Also applies to: 139-139
127-127: Localize “mins”.User-facing text should go through t().
- {date.format(is24h ? "H:mm" : "h:mma")} - {props.eventType.length} mins{" "} + {date.format(is24h ? "H:mm" : "h:mma")} - {t("minutes", { count: props.eventType.length })}
63-63: Avoid “()” flicker for timezone.Initialize timezone with CURRENT_TIMEZONE so the UI never renders empty parentheses.
- const [timezone, setTimezone] = useState<string | null>(null); + const [timezone, setTimezone] = useState<string>(CURRENT_TIMEZONE);Also applies to: 128-129
197-197: Prefer named exports over default exports.Aligns with repo guidelines for better tree-shaking and refactors. Non-blocking due to potential wide import changes.
-export default PaymentPage; +export { PaymentPage };
8-9: Stabilize import surface (avoid underscored paths).Importing from an underscored internal path suggests a private API that may churn; consider re-exporting getPaymentAppData from a stable public entry (e.g., @calcom/app-store/payments) and update imports accordingly.
Would you like me to open a follow-up to add a barrel export and codemod the imports?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/app/(use-page-wrapper)/payment/[uid]/PaymentPage.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/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx
**/*.{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/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
apps/web/app/(use-page-wrapper)/payment/[uid]/PaymentPage.tsx (1)
8-8: Approve: import path change — named export & client-safe verifiedpackages/app-store/_utils/payments/getPaymentAppData.ts exports getPaymentAppData (named), contains no server-only patterns (no node:fs|path|crypto|http(s)|url imports, no 'server-only' markers or process.env usage), and repo imports use the named form.
4 participants
What does this PR do?
Impact: Removes 23 import statements of
@calcom/app-storefrom@calcom/libMigrates:
packages/lib/apps/getEnabledAppsFromCredentials.ts → packages/app-store/_utils/getEnabledAppsFromCredentials.tspackages/lib/getPaymentAppData.ts → packages/app-store/_utils/payments/getPaymentAppData.tspackages/lib/payment/**/* → packages/features/bookings/lib/payment/**/*Mandatory Tasks (DO NOT REMOVE)
How should this be tested?