Async import of the appStore packages - #10
Conversation
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 12 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/trpc/server/routers/viewer/bookings.tsx">
<violation number="1" location="packages/trpc/server/routers/viewer/bookings.tsx:553">
P1: Bug: `async` callback inside `forEach` creates fire-and-forget promises. `forEach` does not await the returned promises, so calendar/video deletion errors become unhandled promise rejections and the subsequent email/webhook code runs before deletions complete. Use `Promise.all` with `.map()` instead.</violation>
</file>
<file name="packages/app-store/vital/lib/reschedule.ts">
<violation number="1" location="packages/app-store/vital/lib/reschedule.ts:125">
P1: Bug: `async` callback in `forEach` is not awaited — errors silently swallowed and operations race with subsequent code. `Array.prototype.forEach` ignores the promises returned by async callbacks, so the `try/catch` block will never catch errors from `getCalendar()` or `deleteEvent()`, and the reschedule email may be sent before deletions complete. Use `await Promise.all(arr.map(...))` instead.</violation>
</file>
<file name="packages/features/bookings/lib/handleCancelBooking.ts">
<violation number="1" location="packages/features/bookings/lib/handleCancelBooking.ts:243">
P1: Bug: `await` inside `.forEach(async ...)` — the async callbacks are fire-and-forget. `.forEach` does not await promises returned by its callback, so the `apiDeletes.push(deletedEvent)` calls inside will execute after the loop appears to complete. Calendar event deletions for recurring bookings will be silently skipped.
Convert this `.forEach(async ...)` to a `for...of` loop, as was correctly done in the other code path in this same PR.</violation>
</file>
<file name="packages/app-store/wipemycalother/lib/reschedule.ts">
<violation number="1" location="packages/app-store/wipemycalother/lib/reschedule.ts:125">
P1: Bug: `async` callback inside `.forEach()` creates fire-and-forget promises. `Array.prototype.forEach` ignores the return value of its callback, so the `await getCalendar(...)` and subsequent `deleteEvent`/`deleteMeeting` calls become unhandled floating promises. The `try/catch` block will never catch their errors, and the code proceeds to send emails before deletions complete.
Use `Promise.all` with `.map()` (or a `for...of` loop) to properly await all operations:</violation>
</file>
<file name="packages/app-store/index.ts">
<violation number="1" location="packages/app-store/index.ts:3">
P2: All dynamic `import()` calls are eagerly evaluated at module load time, defeating the purpose of async imports. Every module begins loading as soon as `appStore` is first imported, rather than on-demand when a specific app is needed.
To achieve true lazy loading, wrap each import in a factory function:
```ts
const appStore = {
applecalendar: () => import("./applecalendar"),
...
};
Then consumers would call await appStore[key](). This ensures only the requested module is loaded.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
| (ref) => !!credentialsMap.get(ref.type) | ||
| ); | ||
| bookingRefsFiltered.forEach((bookingRef) => { | ||
| bookingRefsFiltered.forEach(async (bookingRef) => { |
There was a problem hiding this comment.
P1: Bug: async callback inside forEach creates fire-and-forget promises. forEach does not await the returned promises, so calendar/video deletion errors become unhandled promise rejections and the subsequent email/webhook code runs before deletions complete. Use Promise.all with .map() instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/bookings.tsx, line 553:
<comment>Bug: `async` callback inside `forEach` creates fire-and-forget promises. `forEach` does not await the returned promises, so calendar/video deletion errors become unhandled promise rejections and the subsequent email/webhook code runs before deletions complete. Use `Promise.all` with `.map()` instead.</comment>
<file context>
@@ -550,10 +550,10 @@ export const bookingsRouter = router({
(ref) => !!credentialsMap.get(ref.type)
);
- bookingRefsFiltered.forEach((bookingRef) => {
+ bookingRefsFiltered.forEach(async (bookingRef) => {
if (bookingRef.uid) {
if (bookingRef.type.endsWith("_calendar")) {
</file context>
| ); | ||
| try { | ||
| bookingRefsFiltered.forEach((bookingRef) => { | ||
| bookingRefsFiltered.forEach(async (bookingRef) => { |
There was a problem hiding this comment.
P1: Bug: async callback in forEach is not awaited — errors silently swallowed and operations race with subsequent code. Array.prototype.forEach ignores the promises returned by async callbacks, so the try/catch block will never catch errors from getCalendar() or deleteEvent(), and the reschedule email may be sent before deletions complete. Use await Promise.all(arr.map(...)) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/vital/lib/reschedule.ts, line 125:
<comment>Bug: `async` callback in `forEach` is not awaited — errors silently swallowed and operations race with subsequent code. `Array.prototype.forEach` ignores the promises returned by async callbacks, so the `try/catch` block will never catch errors from `getCalendar()` or `deleteEvent()`, and the reschedule email may be sent before deletions complete. Use `await Promise.all(arr.map(...))` instead.</comment>
<file context>
@@ -122,10 +122,10 @@ const Reschedule = async (bookingUid: string, cancellationReason: string) => {
);
try {
- bookingRefsFiltered.forEach((bookingRef) => {
+ bookingRefsFiltered.forEach(async (bookingRef) => {
if (bookingRef.uid) {
if (bookingRef.type.endsWith("_calendar")) {
</file context>
| } | ||
| if (reference.type.includes("_calendar")) { | ||
| const calendar = getCalendar(credential); | ||
| const calendar = await getCalendar(credential); |
There was a problem hiding this comment.
P1: Bug: await inside .forEach(async ...) — the async callbacks are fire-and-forget. .forEach does not await promises returned by its callback, so the apiDeletes.push(deletedEvent) calls inside will execute after the loop appears to complete. Calendar event deletions for recurring bookings will be silently skipped.
Convert this .forEach(async ...) to a for...of loop, as was correctly done in the other code path in this same PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleCancelBooking.ts, line 243:
<comment>Bug: `await` inside `.forEach(async ...)` — the async callbacks are fire-and-forget. `.forEach` does not await promises returned by its callback, so the `apiDeletes.push(deletedEvent)` calls inside will execute after the loop appears to complete. Calendar event deletions for recurring bookings will be silently skipped.
Convert this `.forEach(async ...)` to a `for...of` loop, as was correctly done in the other code path in this same PR.</comment>
<file context>
@@ -240,7 +240,7 @@ async function handler(req: CustomRequest) {
}
if (reference.type.includes("_calendar")) {
- const calendar = getCalendar(credential);
+ const calendar = await getCalendar(credential);
if (calendar) {
integrationsToDelete.push(
</file context>
| ); | ||
| try { | ||
| bookingRefsFiltered.forEach((bookingRef) => { | ||
| bookingRefsFiltered.forEach(async (bookingRef) => { |
There was a problem hiding this comment.
P1: Bug: async callback inside .forEach() creates fire-and-forget promises. Array.prototype.forEach ignores the return value of its callback, so the await getCalendar(...) and subsequent deleteEvent/deleteMeeting calls become unhandled floating promises. The try/catch block will never catch their errors, and the code proceeds to send emails before deletions complete.
Use Promise.all with .map() (or a for...of loop) to properly await all operations:
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/wipemycalother/lib/reschedule.ts, line 125:
<comment>Bug: `async` callback inside `.forEach()` creates fire-and-forget promises. `Array.prototype.forEach` ignores the return value of its callback, so the `await getCalendar(...)` and subsequent `deleteEvent`/`deleteMeeting` calls become unhandled floating promises. The `try/catch` block will never catch their errors, and the code proceeds to send emails before deletions complete.
Use `Promise.all` with `.map()` (or a `for...of` loop) to properly await all operations:</comment>
<file context>
@@ -122,10 +122,10 @@ const Reschedule = async (bookingUid: string, cancellationReason: string) => {
);
try {
- bookingRefsFiltered.forEach((bookingRef) => {
+ bookingRefsFiltered.forEach(async (bookingRef) => {
if (bookingRef.uid) {
if (bookingRef.type.endsWith("_calendar")) {
</file context>
| import * as zohocrm from "./zohocrm"; | ||
| import * as zoomvideo from "./zoomvideo"; | ||
|
|
||
| const appStore = { |
There was a problem hiding this comment.
P2: All dynamic import() calls are eagerly evaluated at module load time, defeating the purpose of async imports. Every module begins loading as soon as appStore is first imported, rather than on-demand when a specific app is needed.
To achieve true lazy loading, wrap each import in a factory function:
const appStore = {
applecalendar: () => import("./applecalendar"),
...
};Then consumers would call await appStore[key](). This ensures only the requested module is loaded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/index.ts, line 3:
<comment>All dynamic `import()` calls are eagerly evaluated at module load time, defeating the purpose of async imports. Every module begins loading as soon as `appStore` is first imported, rather than on-demand when a specific app is needed.
To achieve true lazy loading, wrap each import in a factory function:
```ts
const appStore = {
applecalendar: () => import("./applecalendar"),
...
};
Then consumers would call await appStore[key](). This ensures only the requested module is loaded.
Test 2nn
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#2](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/2)*