Async import of the appStore packages#10
Conversation
🤖 Grapple PR ReviewSummaryThis PR modifies 12 files. Title: "Async import of the appStore packages" Changes: 12 files (+82 / -107 lines)
Resolves: #2 (Add guest management functionality to existing bookings) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 12 files"/]
f1["🟡 getCalendar.ts +2/-2"]
pr_0 --> f1
f2["🟡 index.ts +29/-59"]
pr_0 --> f2
f3["🟡 reschedule.ts +2/-2"]
pr_0 --> f3
f4["🟡 reschedule.ts +2/-2"]
pr_0 --> f4
f5["🟡 CalendarManager.ts +8/-7"]
pr_0 --> f5
f6["🟡 EventManager.ts +1/-2"]
pr_0 --> f6
f7["🟡 videoClient.ts +20/-16"]
pr_0 --> f7
f8["🟡 handleCancelBooking.ts +12/-11"]
pr_0 --> f8
f9["🟡 handleNewBooking.ts +1/-1"]
pr_0 --> f9
f10["🟡 deletePayment.ts +1/-1"]
pr_0 --> f10
f11["🟡 handlePayment.ts +1/-1"]
pr_0 --> f11
f12["🟡 bookings.tsx +3/-3"]
pr_0 --> f12
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR converts the appStore package imports from synchronous static imports to dynamic async imports using import(), and propagates the resulting async changes throughout all call sites that access appStore entries. The goal is to enable code-splitting and lazy loading of app store packages.
Changes: 12 files (+82 / -107 lines)
- 🟡 12 files modified:
packages/app-store/_utils/getCalendar.ts,packages/app-store/index.ts,packages/app-store/vital/lib/reschedule.ts,packages/app-store/wipemycalother/lib/reschedule.ts,packages/core/CalendarManager.ts+7 more
Resolves: #2 (Add guest management functionality to existing bookings)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 12 files"/]
f1["🟡 getCalendar.ts +2/-2"]
pr_0 --> f1
f2["🟡 index.ts +29/-59"]
pr_0 --> f2
f3["🟡 reschedule.ts +2/-2"]
pr_0 --> f3
f4["🟡 reschedule.ts +2/-2"]
pr_0 --> f4
f5["🟡 CalendarManager.ts +8/-7"]
pr_0 --> f5
f6["🟡 EventManager.ts +1/-2"]
pr_0 --> f6
f7["🟡 videoClient.ts +20/-16"]
pr_0 --> f7
f8["🟡 handleCancelBooking.ts +12/-11"]
pr_0 --> f8
f9["🟡 handleNewBooking.ts +1/-1"]
pr_0 --> f9
f10["🟡 deletePayment.ts +1/-1"]
pr_0 --> f10
f11["🟡 handlePayment.ts +1/-1"]
pr_0 --> f11
f12["🟡 bookings.tsx +3/-3"]
pr_0 --> f12
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
9 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 9 |
| 🟠 Major | 7 |
| 🟡 Minor | 3 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-07T18:29:14.767Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 112.7s
- Low-confidence findings suppressed: 9
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — Unnecessary work (confidence: 100%)
All dynamic import() calls are evaluated eagerly at module load time, not lazily. The entire point of dynamic imports for code-splitting is that they are called conditionally or on-demand. Placing all import() calls as top-level object property initializers means every app module is fetched immediately when this file is first imported — identical behavior to static imports, but with added Promise overhead on every access. No lazy loading or code-splitting benefit is achieved.
Evidence:
- Each
import('./appname')expression is evaluated when the object literal is constructed, which happens at module initialization time. - Every consumer must now await each appStore entry even though all modules were already loaded at startup.
- The stated goal of 'lazy loading of app store packages' is not achieved — this pattern only helps with circular dependency resolution, not deferred loading.
Agent: performance
| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); | ||
| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); | ||
| } else if (bookingRef.type.endsWith("_video")) { | ||
| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); |
There was a problem hiding this comment.
🔴 CRITICAL — Concurrency issues (confidence: 100%)
An async callback is passed to forEach(). forEach() does not await async callbacks — it fires each async function and discards the returned Promise. This means calendar.deleteEvent() and deleteMeeting() calls are fire-and-forget: errors are silently swallowed, the surrounding try/catch cannot catch rejections, and the function may return before deletions complete.
Evidence:
- Array.prototype.forEach does not handle Promise return values from callbacks.
- The async keyword on the callback creates a Promise that is never awaited by forEach.
- If getCalendar() rejects or deleteEvent() fails, the error is lost with no logging or retry.
- Same pattern existed before but was synchronous — the async conversion makes this actively dangerous.
Agent: performance
| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); | ||
| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); | ||
| } else if (bookingRef.type.endsWith("_video")) { | ||
| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); |
There was a problem hiding this comment.
🔴 CRITICAL — Concurrency issues (confidence: 100%)
Same async-in-forEach anti-pattern as vital/lib/reschedule.ts. Calendar delete operations are fire-and-forget with no error handling and no guarantee of completion before the function returns.
Evidence:
- Array.prototype.forEach discards Promise return values.
- Exceptions thrown inside the async callback will become unhandled rejections.
- Rescheduling may complete while calendar events are still pending deletion.
Agent: performance
| @@ -28,6 +28,7 @@ export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => | |||
| const calendar = getCalendar(credential); | |||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | |||
There was a problem hiding this comment.
🟠 MAJOR — Unresolved Promise in return type (confidence: 100%)
In getCalendarCredentials(), the calendar property in the returned objects is now a Promise (since getCalendar() is now async), but the function is not async and does not await the result. This means every consumer of getCalendarCredentials() receives objects where calendar is a Promise, not a Calendar instance. While getConnectedCalendars() correctly awaits item.calendar on line 47, and getCachedResults() was refactored to use Promise.all, any other current or future caller that destructures calendar directly without awaiting will receive a Promise object, which is truthy, causing silent logic failures (e.g., if (calendar) would always be true even if the resolved value is null).
Evidence:
- Line 28:
const calendar = getCalendar(credential);— getCalendar is now async, returns Promise - The return type of
getCalendarCredentialsimplicitly changes thecalendarfield fromCalendar | nulltoPromise - This is a subtle API contract change that TypeScript may not flag at all call sites depending on type inference
- Intent specification notes: 'any consumer that destructures calendar directly without awaiting will receive a Promise object instead of a Calendar instance, causing silent failures'
Agent: logic
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟠 MAJOR — No lazy loading benefit (confidence: 100%)
The import() calls are executed immediately at module evaluation time (when the appStore object is constructed), not lazily on first access. Every dynamic import is invoked eagerly as part of the object literal initialization, meaning all app packages are loaded as soon as appStore/index.ts is first imported. This provides no code-splitting or lazy-loading benefit over static imports — it only converts the values from modules to Promises of modules, adding async overhead without the intended benefit.
Evidence:
- All
import()calls are in the object literal value positions, meaning they execute when the object is created - The module is evaluated once on first import, at which point all 28+ dynamic imports fire simultaneously
- For actual lazy loading, the imports should be wrapped in functions:
applecalendar: () => import('./applecalendar')and called at the point of use - Intent specification notes: 'appStore entries are top-level dynamic imports evaluated at module load time (not lazily)'
Agent: logic
| return acc; | ||
| }, []); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Concurrency issues (confidence: 100%)
getVideoAdapters() now uses a sequential for...of loop with await for each credential, serializing all appStore lookups. The previous reduce()-based implementation was synchronous; now N video credentials result in N sequential awaits on import Promises that are already resolved (since all imports fired at module load). While functionally correct, this adds unnecessary sequential microtask overhead when Promise.all would be both simpler and faster.
Evidence:
- Each
await appStore[appName]in the for...of loop is a sequential await on an already-resolved Promise. - With N credentials, this creates N sequential microtask checkpoints instead of 1 with Promise.all.
- The original reduce() was O(N) synchronous; the new for...of is O(N) with N unnecessary sequential awaits.
Agent: performance
| } | ||
| return acc; | ||
| }, []); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR — Sequential awaits where parallelism is possible (confidence: 100%)
The refactored getVideoAdapters uses a sequential for...of loop with await for each credential. Since the dynamic imports are independent of each other, they could be resolved in parallel using Promise.all, which would be faster when multiple video credentials exist.
Evidence:
- Lines 24-32: Sequential
for (const cred of withCredentials) { const app = await appStore[...]; ... } - Each iteration waits for the previous import to resolve before starting the next
- The original reduce pattern was synchronous and processed all credentials without delay
Agent: architecture
| export const getCalendar = async (credential: CredentialPayload | null): Promise<Calendar | null> => { | ||
| if (!credential || !credential.key) return null; | ||
| let { type: calendarType } = credential; | ||
| if (calendarType?.endsWith("_other_calendar")) { |
There was a problem hiding this comment.
🟡 MINOR — Error handling (confidence: 93%)
If a dynamic import() fails (e.g., module not found at runtime), the await on line 12 will throw an unhandled rejection. Since getCalendar is called in many places and callers may not always have adequate error handling for import failures, this could cause unexpected crashes. Consider wrapping the await in a try/catch that returns null on failure, consistent with the existing pattern of returning null for unknown calendar types.
Evidence:
- Line 12:
const calendarApp = await appStore[calendarType.split('_').join('') as keyof typeof appStore]; - If the key doesn't exist in appStore, this would be
await undefinedwhich is fine, but a failed import() would throw - Intent specification notes: 'If an appStore dynamic import() fails, the error may propagate as an unhandled rejection'
Agent: logic
| export const getCalendar = (credential: CredentialPayload | null): Calendar | null => { | ||
| export const getCalendar = async (credential: CredentialPayload | null): Promise<Calendar | null> => { | ||
| if (!credential || !credential.key) return null; | ||
| let { type: calendarType } = credential; |
There was a problem hiding this comment.
🟡 MINOR — Error handling for dynamic imports (confidence: 93%)
If a dynamic import() in the appStore fails (e.g., module not found for a corrupted or removed app), the await on line 11 will throw an unhandled rejection. Since getCalendar() is called in many places including loops, a single failed import could crash the entire operation. Consider wrapping the await in a try/catch that returns null on failure, consistent with the existing null-return pattern.
Evidence:
- Line 11:
const calendarApp = await appStore[calendarType.split('_').join('') as keyof typeof appStore] - If the import() Promise rejects, the error propagates up to whichever caller invoked getCalendar()
- getCachedResults() and getConnectedCalendars() iterate over multiple credentials — one bad import would abort the entire batch
Agent: architecture
| }); | ||
|
|
||
| return credentials.length ? credentials : []; | ||
| }); |
There was a problem hiding this comment.
🔵 INFO — Code patterns (confidence: 96%)
getCalendarCredentials() returns an array with a calendar property that is now a Promise, but this function is synchronous and doesn't await the Promise. Callers that destructure calendar directly will receive a Promise object instead of a Calendar instance.
Evidence:
- Line 27:
const calendar = getCalendar(credential);— getCalendar now returns a Promise but is called without await - Line 28:
return app.variant === 'calendar' ? [{ integration: app, credential, calendar }] : [];— calendar is a Promise, not a Calendar - getCalendarCredentials is synchronous (no async keyword) so cannot await
- getConnectedCalendars correctly awaits on line 45, but other callers may not
Agent: style
| export const getCalendar = async (credential: CredentialPayload | null): Promise<Calendar | null> => { | ||
| if (!credential || !credential.key) return null; | ||
| let { type: calendarType } = credential; | ||
| if (calendarType?.endsWith("_other_calendar")) { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
If a dynamic
import()fails (e.g., module not found at runtime), theawaiton line 12 will throw an unhandled rejection. SincegetCalendaris called in many places and callers may not always have adequate error handling for import failures, this could cause unexpected crashes. Consider wrapping the await in a try/catch that returns null on failure, consistent with the existing pattern of returning null for unknown calendar types.
| if (calendarType?.endsWith("_other_calendar")) { | |
| let calendarApp; | |
| try { | |
| calendarApp = await appStore[calendarType.split("_").join("") as keyof typeof appStore]; | |
| } catch (e) { | |
| log.error(`Failed to load calendar app for type ${calendarType}`, e); | |
| return null; | |
| } |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (58 lines, 1 file) — review recommended)
All dynamic import() calls are evaluated eagerly at module load time, not lazily. The entire point of dynamic imports for code-splitting is that they are called conditionally or on-demand. Placing all import() calls as top-level object property initializers means every app module is fetched immediately when this file is first imported — identical behavior to static imports, but with added Promise overhead on every access. No lazy loading or code-splitting benefit is achieved.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -1,33 +1,33 @@
const appStore = {
- // example: import("./example"),
- applecalendar: import("./applecalendar"),
- caldavcalendar: import("./caldavcalendar"),
- closecom: import("./closecom"),
- dailyvideo: import("./dailyvideo"),
- googlecalendar: import("./googlecalendar"),
- googlevideo: import("./googlevideo"),
- hubspot: import("./hubspot"),
- huddle01video: import("./huddle01video"),
- jitsivideo: import("./jitsivideo"),
- larkcalendar: import("./larkcalendar"),
- office365calendar: import("./office365calendar"),
- office365video: import("./office365video"),
- plausible: import("./plausible"),
- salesforce: import("./salesforce"),
- zohocrm: import("./zohocrm"),
- sendgrid: import("./sendgrid"),
- stripepayment: import("./stripepayment"),
- tandemvideo: import("./tandemvideo"),
- vital: import("./vital"),
- zoomvideo: import("./zoomvideo"),
- wipemycalother: import("./wipemycalother"),
- giphy: import("./giphy"),
- zapier: import("./zapier"),
- exchange2013calendar: import("./exchange2013calendar"),
- exchange2016calendar: import("./exchange2016calendar"),
- exchangecalendar: import("./exchangecalendar"),
- facetime: import("./facetime"),
- sylapsvideo: import("./sylapsvideo"),
+ // example: () => import("./example"),
+ applecalendar: () => import("./applecalendar"),
+ caldavcalendar: () => import("./caldavcalendar"),
+ closecom: () => import("./closecom"),
+ dailyvideo: () => import("./dailyvideo"),
+ googlecalendar: () => import("./googlecalendar"),
+ googlevideo: () => import("./googlevideo"),
+ hubspot: () => import("./hubspot"),
+ huddle01video: () => import("./huddle01video"),
+ jitsivideo: () => import("./jitsivideo"),
+ larkcalendar: () => import("./larkcalendar"),
+ office365calendar: () => import("./office365calendar"),
+ office365video: () => import("./office365video"),
+ plausible: () => import("./plausible"),
+ salesforce: () => import("./salesforce"),
+ zohocrm: () => import("./zohocrm"),
+ sendgrid: () => import("./sendgrid"),
+ stripepayment: () => import("./stripepayment"),
+ tandemvideo: () => import("./tandemvideo"),
+ vital: () => import("./vital"),
+ zoomvideo: () => import("./zoomvideo"),
+ wipemycalother: () => import("./wipemycalother"),
+ giphy: () => import("./giphy"),
+ zapier: () => import("./zapier"),
+ exchange2013calendar: () => import("./exchange2013calendar"),
+ exchange2016calendar: () => import("./exchange2016calendar"),
+ exchangecalendar: () => import("./exchangecalendar"),
+ facetime: () => import("./facetime"),
+ sylapsvideo: () => import("./sylapsvideo"),
};
export default appStore;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); | ||
| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); | ||
| } else if (bookingRef.type.endsWith("_video")) { | ||
| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (20 lines, 1 file) — review recommended)
An async callback is passed to forEach(). forEach() does not await async callbacks — it fires each async function and discards the returned Promise. This means calendar.deleteEvent() and deleteMeeting() calls are fire-and-forget: errors are silently swallowed, the surrounding try/catch cannot catch rejections, and the function may return before deletions complete.
--- a/packages/app-store/vital/lib/reschedule.ts
+++ b/packages/app-store/vital/lib/reschedule.ts
@@ -122,13 +122,15 @@ const Reschedule = async (bookingUid: string, cancellationReason: string) => {
(ref) => !!credentialsMap.get(ref.type)
);
try {
- bookingRefsFiltered.forEach(async (bookingRef) => {
- if (bookingRef.uid) {
- if (bookingRef.type.endsWith("_calendar")) {
- const calendar = await getCalendar(credentialsMap.get(bookingRef.type));
- return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent);
- } else if (bookingRef.type.endsWith("_video")) {
- return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid);
+ await Promise.all(
+ bookingRefsFiltered.map(async (bookingRef) => {
+ if (bookingRef.uid) {
+ if (bookingRef.type.endsWith("_calendar")) {
+ const calendar = await getCalendar(credentialsMap.get(bookingRef.type));
+ await calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent);
+ } else if (bookingRef.type.endsWith("_video")) {
+ await deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid);
+ }
}
- }
- });
+ })
+ );
} catch (error) {
if (error instanceof Error) {
logger.error(error.message);🤖 Grapple PR auto-fix • critical • Review this diff before applying
| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); | ||
| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); | ||
| } else if (bookingRef.type.endsWith("_video")) { | ||
| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (20 lines, 1 file) — review recommended)
Same async-in-forEach anti-pattern as vital/lib/reschedule.ts. Calendar delete operations are fire-and-forget with no error handling and no guarantee of completion before the function returns.
--- a/packages/app-store/wipemycalother/lib/reschedule.ts
+++ b/packages/app-store/wipemycalother/lib/reschedule.ts
@@ -122,14 +122,16 @@ const Reschedule = async (bookingUid: string, cancellationReason: string) => {
(ref) => !!credentialsMap.get(ref.type)
);
try {
- bookingRefsFiltered.forEach(async (bookingRef) => {
- if (bookingRef.uid) {
- if (bookingRef.type.endsWith("_calendar")) {
- const calendar = await getCalendar(credentialsMap.get(bookingRef.type));
- return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent);
- } else if (bookingRef.type.endsWith("_video")) {
- return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid);
+ await Promise.all(
+ bookingRefsFiltered.map(async (bookingRef) => {
+ if (bookingRef.uid) {
+ if (bookingRef.type.endsWith("_calendar")) {
+ const calendar = await getCalendar(credentialsMap.get(bookingRef.type));
+ return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent);
+ } else if (bookingRef.type.endsWith("_video")) {
+ return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid);
+ }
}
- }
- });
+ })
+ );
} catch (error) {
if (error instanceof Error) {
logger.error(error.message);🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @@ -28,6 +28,7 @@ export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => | |||
| const calendar = getCalendar(credential); | |||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (20 lines, 1 file) — review recommended)
In
getCalendarCredentials(), thecalendarproperty in the returned objects is now aPromise<Calendar | null>(sincegetCalendar()is now async), but the function is not async and does not await the result. This means every consumer ofgetCalendarCredentials()receives objects wherecalendaris a Promise, not a Calendar instance. WhilegetConnectedCalendars()correctly awaitsitem.calendaron line 47, andgetCachedResults()was refactored to usePromise.all, any other current or future caller that destructurescalendardirectly without awaiting will receive a Promise object, which is truthy, causing silent logic failures (e.g.,if (calendar)would always be true even if the resolved value is null).
--- a/packages/core/CalendarManager.ts
+++ b/packages/core/CalendarManager.ts
@@ -23,14 +23,17 @@ const log = logger.getChildLogger({ prefix: ["CalendarManager"] });
-export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => {
- const calendarCredentials = getApps(credentials)
+export const getCalendarCredentials = async (credentials: Array<CredentialPayload>) => {
+ const calendarCredentials = await Promise.all(getApps(credentials)
.filter((app) => app.type.endsWith("_calendar"))
- .flatMap((app) => {
- const credentials = app.credentials.flatMap((credential) => {
- const calendar = getCalendar(credential);
- return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : [];
- });
+ .flatMap((app) => {
+ return app.credentials
+ .filter(() => app.variant === "calendar")
+ .map(async (credential) => {
+ const calendar = await getCalendar(credential);
+ return { integration: app, credential, calendar };
+ });
+ }));
- return credentials.length ? credentials : [];
- });
-
return calendarCredentials;
};
@@ -38,7 +41,7 @@ export const getConnectedCalendars = async (
calendarCredentials: ReturnType<typeof getCalendarCredentials>,🤖 Grapple PR auto-fix • major • Review this diff before applying
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (58 lines, 1 file) — review recommended)
The
import()calls are executed immediately at module evaluation time (when the appStore object is constructed), not lazily on first access. Every dynamic import is invoked eagerly as part of the object literal initialization, meaning all app packages are loaded as soon asappStore/index.tsis first imported. This provides no code-splitting or lazy-loading benefit over static imports — it only converts the values from modules to Promises of modules, adding async overhead without the intended benefit.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -1,31 +1,31 @@
const appStore = {
- // example: import("./example"),
- applecalendar: import("./applecalendar"),
- caldavcalendar: import("./caldavcalendar"),
- closecom: import("./closecom"),
- dailyvideo: import("./dailyvideo"),
- googlecalendar: import("./googlecalendar"),
- googlevideo: import("./googlevideo"),
- hubspot: import("./hubspot"),
- huddle01video: import("./huddle01video"),
- jitsivideo: import("./jitsivideo"),
- larkcalendar: import("./larkcalendar"),
- office365calendar: import("./office365calendar"),
- office365video: import("./office365video"),
- plausible: import("./plausible"),
- salesforce: import("./salesforce"),
- zohocrm: import("./zohocrm"),
- sendgrid: import("./sendgrid"),
- stripepayment: import("./stripepayment"),
- tandemvideo: import("./tandemvideo"),
- vital: import("./vital"),
- zoomvideo: import("./zoomvideo"),
- wipemycalother: import("./wipemycalother"),
- giphy: import("./giphy"),
- zapier: import("./zapier"),
- exchange2013calendar: import("./exchange2013calendar"),
- exchange2016calendar: import("./exchange2016calendar"),
- exchangecalendar: import("./exchangecalendar"),
- facetime: import("./facetime"),
- sylapsvideo: import("./sylapsvideo"),
+ // example: () => import("./example"),
+ applecalendar: () => import("./applecalendar"),
+ caldavcalendar: () => import("./caldavcalendar"),
+ closecom: () => import("./closecom"),
+ dailyvideo: () => import("./dailyvideo"),
+ googlecalendar: () => import("./googlecalendar"),
+ googlevideo: () => import("./googlevideo"),
+ hubspot: () => import("./hubspot"),
+ huddle01video: () => import("./huddle01video"),
+ jitsivideo: () => import("./jitsivideo"),
+ larkcalendar: () => import("./larkcalendar"),
+ office365calendar: () => import("./office365calendar"),
+ office365video: () => import("./office365video"),
+ plausible: () => import("./plausible"),
+ salesforce: () => import("./salesforce"),
+ zohocrm: () => import("./zohocrm"),
+ sendgrid: () => import("./sendgrid"),
+ stripepayment: () => import("./stripepayment"),
+ tandemvideo: () => import("./tandemvideo"),
+ vital: () => import("./vital"),
+ zoomvideo: () => import("./zoomvideo"),
+ wipemycalother: () => import("./wipemycalother"),
+ giphy: () => import("./giphy"),
+ zapier: () => import("./zapier"),
+ exchange2013calendar: () => import("./exchange2013calendar"),
+ exchange2016calendar: () => import("./exchange2016calendar"),
+ exchangecalendar: () => import("./exchangecalendar"),
+ facetime: () => import("./facetime"),
+ sylapsvideo: () => import("./sylapsvideo"),
};
export default appStore;🤖 Grapple PR auto-fix • major • Review this diff before applying
| @@ -28,6 +28,7 @@ export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => | |||
| const calendar = getCalendar(credential); | |||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | |||
| }); | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (29 lines, 1 file) — review recommended)
The
getCalendarCredentialsfunction returns objects where thecalendarproperty is now aPromise<Calendar | null>(sincegetCalendar()is now async), but the function signature and return type are not updated to reflect this. The function itself remains synchronous, so callers receive{ integration, credential, calendar: Promise<Calendar | null> }. WhilegetConnectedCalendarscorrectly awaitsitem.calendar(line 46), any other current or future consumer ofgetCalendarCredentialsthat destructurescalendardirectly will get a Promise object instead of a Calendar instance, causing silent type-level failures (the truthiness checkif (!calendar)will always pass since a Promise is truthy).
--- a/packages/core/CalendarManager.ts
+++ b/packages/core/CalendarManager.ts
@@ -23,16 +23,16 @@ const log = logger.getChildLogger({ prefix: ["CalendarManager"] });
-export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => {
- const calendarCredentials = getApps(credentials)
+export const getCalendarCredentials = async (credentials: Array<CredentialPayload>) => {
+ const calendarCredentials = await Promise.all(getApps(credentials)
.filter((app) => app.type.endsWith("_calendar"))
.flatMap((app) => {
- const credentials = app.credentials.flatMap((credential) => {
- const calendar = getCalendar(credential);
- return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : [];
+ const credentials = app.credentials.flatMap((credential) => {
+ return app.variant === "calendar" ? [{ integration: app, credential }] : [];
});
return credentials.length ? credentials : [];
- });
+ })
+ .map(async (item) => ({
+ ...item,
+ calendar: await getCalendar(item.credential),
+ })));
return calendarCredentials;
};
@@ -43,8 +43,8 @@ export const getConnectedCalendars = async (
const connectedCalendars = await Promise.all(
calendarCredentials.map(async (item) => {
try {
- const { integration, credential } = item;
- const calendar = await item.calendar;
+ const { calendar, integration, credential } = item;
+
// Don't leak credentials to the client
const credentialId = credential.id;
if (!calendar) {
@@ -138,7 +138,7 @@ export const getCachedResults = async (
selectedCalendars: SelectedCalendar[]
): Promise<EventBusyDate[][]> => {
const calendarCredentials = withCredentials.filter((credential) => credential.type.endsWith("_calendar"));
- const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential)));
+ const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential)));
performance.mark("getBusyCalendarTimesStart");
const results = calendars.map(async (c, i) => {
/** Filter out nulls */
@@ -229,7 +229,7 @@ export const createEvent = async (
calEvent: CalendarEvent
): Promise<EventResult<NewCalendarEventType>> => {
const uid: string = getUid(calEvent);
- const calendar = await getCalendar(credential);
+ const calendar = await getCalendar(credential);
let success = true;
let calError: string | undefined = undefined;
@@ -281,7 +281,7 @@ export const updateEvent = async (
externalCalendarId: string | null
): Promise<EventResult<NewCalendarEventType>> => {
const uid = getUid(calEvent);
- const calendar = await getCalendar(credential);
+ const calendar = await getCalendar(credential);
let success = false;
let calError: string | undefined = undefined;
let calWarnings: string[] | undefined = [];
@@ -327,7 +327,7 @@ export const updateEvent = async (
-export const deleteEvent = async (
+export const deleteEvent = async (
credential: CredentialPayload,
uid: string,
event: CalendarEvent
): Promise<unknown> => {
- const calendar = await getCalendar(credential);
+ const calendar = await getCalendar(credential);
if (calendar) {
return calendar.deleteEvent(uid, event);
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (56 lines, 1 file) — review recommended)
The
import()calls are executed eagerly at module evaluation time (whenindex.tsis first imported), not lazily on demand. TheappStoreobject literal evaluates all its property initializers immediately, so every dynamicimport()fires at once during module load. This provides no code-splitting or lazy-loading benefit — all app modules are still loaded upfront, just asynchronously. The refactor adds significant complexity (async propagation through ~12 files) without achieving the stated goal of lazy loading.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -1,31 +1,31 @@
const appStore = {
// example: () => import("./example"),
- applecalendar: import("./applecalendar"),
- caldavcalendar: import("./caldavcalendar"),
- closecom: import("./closecom"),
- dailyvideo: import("./dailyvideo"),
- googlecalendar: import("./googlecalendar"),
- googlevideo: import("./googlevideo"),
- hubspot: import("./hubspot"),
- huddle01video: import("./huddle01video"),
- jitsivideo: import("./jitsivideo"),
- larkcalendar: import("./larkcalendar"),
- office365calendar: import("./office365calendar"),
- office365video: import("./office365video"),
- plausible: import("./plausible"),
- salesforce: import("./salesforce"),
- zohocrm: import("./zohocrm"),
- sendgrid: import("./sendgrid"),
- stripepayment: import("./stripepayment"),
- tandemvideo: import("./tandemvideo"),
- vital: import("./vital"),
- zoomvideo: import("./zoomvideo"),
- wipemycalother: import("./wipemycalother"),
- giphy: import("./giphy"),
- zapier: import("./zapier"),
- exchange2013calendar: import("./exchange2013calendar"),
- exchange2016calendar: import("./exchange2016calendar"),
- exchangecalendar: import("./exchangecalendar"),
- facetime: import("./facetime"),
- sylapsvideo: import("./sylapsvideo"),
+ applecalendar: () => import("./applecalendar"),
+ caldavcalendar: () => import("./caldavcalendar"),
+ closecom: () => import("./closecom"),
+ dailyvideo: () => import("./dailyvideo"),
+ googlecalendar: () => import("./googlecalendar"),
+ googlevideo: () => import("./googlevideo"),
+ hubspot: () => import("./hubspot"),
+ huddle01video: () => import("./huddle01video"),
+ jitsivideo: () => import("./jitsivideo"),
+ larkcalendar: () => import("./larkcalendar"),
+ office365calendar: () => import("./office365calendar"),
+ office365video: () => import("./office365video"),
+ plausible: () => import("./plausible"),
+ salesforce: () => import("./salesforce"),
+ zohocrm: () => import("./zohocrm"),
+ sendgrid: () => import("./sendgrid"),
+ stripepayment: () => import("./stripepayment"),
+ tandemvideo: () => import("./tandemvideo"),
+ vital: () => import("./vital"),
+ zoomvideo: () => import("./zoomvideo"),
+ wipemycalother: () => import("./wipemycalother"),
+ giphy: () => import("./giphy"),
+ zapier: () => import("./zapier"),
+ exchange2013calendar: () => import("./exchange2013calendar"),
+ exchange2016calendar: () => import("./exchange2016calendar"),
+ exchangecalendar: () => import("./exchangecalendar"),
+ facetime: () => import("./facetime"),
+ sylapsvideo: () => import("./sylapsvideo"),
};
export default appStore;🤖 Grapple PR auto-fix • major • Review this diff before applying
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (2 lines, 1 file))
The
sylapsvideoentry has been moved from its original alphabetical/grouped position to the very end of the object (line 30). While it IS present in the new code, the reordering combined with it being the last entry makes it easy to accidentally drop in future edits. More importantly, verify this was intentional — the original code hadsylapsvideobetweenjitsivideoandlarkcalendar, and the new code places it afterfacetime.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -8,8 +8,8 @@
hubspot: import("./hubspot"),
huddle01video: import("./huddle01video"),
jitsivideo: import("./jitsivideo"),
+ sylapsvideo: import("./sylapsvideo"),
larkcalendar: import("./larkcalendar"),
office365calendar: import("./office365calendar"),
office365video: import("./office365video"),
plausible: import("./plausible"),
@@ -26,6 +26,5 @@
exchange2013calendar: import("./exchange2013calendar"),
exchange2016calendar: import("./exchange2016calendar"),
exchangecalendar: import("./exchangecalendar"),
facetime: import("./facetime"),
- sylapsvideo: import("./sylapsvideo"),
};🤖 Grapple PR auto-fix • major • Review this diff before applying
| return acc; | ||
| }, []); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (22 lines, 1 file) — review recommended)
getVideoAdapters() now uses a sequential for...of loop with await for each credential, serializing all appStore lookups. The previous reduce()-based implementation was synchronous; now N video credentials result in N sequential awaits on import Promises that are already resolved (since all imports fired at module load). While functionally correct, this adds unnecessary sequential microtask overhead when Promise.all would be both simpler and faster.
--- a/packages/core/videoClient.ts
+++ b/packages/core/videoClient.ts
@@ -20,18 +20,15 @@ const translator = short();
// factory
const getVideoAdapters = async (withCredentials: CredentialPayload[]): Promise<VideoApiAdapter[]> => {
- const videoAdapters: VideoApiAdapter[] = [];
-
- for (const cred of withCredentials) {
+ const adapters = await Promise.all(
+ withCredentials.map(async (cred) => {
const appName = cred.type.split("_").join(""); // Transform `zoom_video` to `zoomvideo`;
const app = await appStore[appName as keyof typeof appStore];
-
- if (app && "lib" in app && "VideoApiAdapter" in app.lib) {
- const makeVideoApiAdapter = app.lib.VideoApiAdapter as VideoApiAdapterFactory;
- const videoAdapter = makeVideoApiAdapter(cred);
- videoAdapters.push(videoAdapter);
- }
- }
-
- return videoAdapters;
+ if (app && "lib" in app && "VideoApiAdapter" in app.lib) {
+ const makeVideoApiAdapter = app.lib.VideoApiAdapter as VideoApiAdapterFactory;
+ return makeVideoApiAdapter(cred);
+ }
+ return null;
+ })
+ );
+ return adapters.filter((adapter): adapter is VideoApiAdapter => adapter !== null);
};🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| return acc; | ||
| }, []); | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (22 lines, 1 file) — review recommended)
The refactored
getVideoAdaptersuses a sequentialfor...ofloop withawaitfor each credential. Since the dynamic imports are independent of each other, they could be resolved in parallel usingPromise.all, which would be faster when multiple video credentials exist.
--- a/packages/core/videoClient.ts
+++ b/packages/core/videoClient.ts
@@ -21,18 +21,16 @@ const translator = short();
// factory
const getVideoAdapters = async (withCredentials: CredentialPayload[]): Promise<VideoApiAdapter[]> => {
- const videoAdapters: VideoApiAdapter[] = [];
-
- for (const cred of withCredentials) {
+ const adapters = await Promise.all(
+ withCredentials.map(async (cred) => {
const appName = cred.type.split("_").join(""); // Transform `zoom_video` to `zoomvideo`;
- const app = await appStore[appName as keyof typeof appStore];
-
- if (app && "lib" in app && "VideoApiAdapter" in app.lib) {
+ const app = await appStore[appName as keyof typeof appStore];
+ if (app && "lib" in app && "VideoApiAdapter" in app.lib) {
const makeVideoApiAdapter = app.lib.VideoApiAdapter as VideoApiAdapterFactory;
- const videoAdapter = makeVideoApiAdapter(cred);
- videoAdapters.push(videoAdapter);
- }
- }
-
- return videoAdapters;
+ return makeVideoApiAdapter(cred);
+ }
+ return null;
+ })
+ );
+ return adapters.filter((adapter): adapter is VideoApiAdapter => adapter !== null);
};🤖 Grapple PR auto-fix • minor • Review this diff before applying
| export const getCalendar = (credential: CredentialPayload | null): Calendar | null => { | ||
| export const getCalendar = async (credential: CredentialPayload | null): Promise<Calendar | null> => { | ||
| if (!credential || !credential.key) return null; | ||
| let { type: calendarType } = credential; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (12 lines, 1 file) — review recommended)
If a dynamic
import()in the appStore fails (e.g., module not found for a corrupted or removed app), theawaiton line 11 will throw an unhandled rejection. SincegetCalendar()is called in many places including loops, a single failed import could crash the entire operation. Consider wrapping the await in a try/catch that returns null on failure, consistent with the existing null-return pattern.
--- a/packages/app-store/_utils/getCalendar.ts
+++ b/packages/app-store/_utils/getCalendar.ts
@@ -9,7 +9,14 @@ export const getCalendar = async (credential: CredentialPayload | null): Promise
if (!credential || !credential.key) return null;
let { type: calendarType } = credential;
if (calendarType?.endsWith("_other_calendar")) {
calendarType = calendarType.split("_other_calendar")[0];
}
- const calendarApp = await appStore[calendarType.split("_").join("") as keyof typeof appStore];
+ // Wrap the dynamic import in try/catch so that a missing or corrupted app
+ // module does not abort callers that iterate over multiple credentials
+ // (e.g. getCachedResults, getConnectedCalendars). Consistent with the
+ // null-return pattern used below for unimplemented calendar types.
+ let calendarApp: Awaited<(typeof appStore)[keyof typeof appStore]> | undefined;
+ try {
+ calendarApp = await appStore[calendarType.split("_").join("") as keyof typeof appStore];
+ } catch (e) {
+ log.warn(`Failed to load calendar app for type ${calendarType}`, e);
+ return null;
+ }
if (!(calendarApp && "lib" in calendarApp && "CalendarService" in calendarApp.lib)) {
log.warn(`calendar of type ${calendarType} is not implemented`);
return null;🤖 Grapple PR auto-fix • minor • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR converts the appStore from synchronous static imports to asynchronous dynamic imports, requiring all consumers of appStore entries to await the resulting Promises. The change aims to enable code splitting and lazy loading of app store packages.
Changes: 12 files (+82 / -107 lines)
- 🟡 12 files modified:
packages/app-store/_utils/getCalendar.ts,packages/app-store/index.ts,packages/app-store/vital/lib/reschedule.ts,packages/app-store/wipemycalother/lib/reschedule.ts,packages/core/CalendarManager.ts+7 more
Resolves: #2 (Add guest management functionality to existing bookings)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 12 files"/]
f1["🟡 getCalendar.ts +2/-2"]
pr_0 --> f1
f2["🟡 index.ts +29/-59"]
pr_0 --> f2
f3["🟡 reschedule.ts +2/-2"]
pr_0 --> f3
f4["🟡 reschedule.ts +2/-2"]
pr_0 --> f4
f5["🟡 CalendarManager.ts +8/-7"]
pr_0 --> f5
f6["🟡 EventManager.ts +1/-2"]
pr_0 --> f6
f7["🟡 videoClient.ts +20/-16"]
pr_0 --> f7
f8["🟡 handleCancelBooking.ts +12/-11"]
pr_0 --> f8
f9["🟡 handleNewBooking.ts +1/-1"]
pr_0 --> f9
f10["🟡 deletePayment.ts +1/-1"]
pr_0 --> f10
f11["🟡 handlePayment.ts +1/-1"]
pr_0 --> f11
f12["🟡 bookings.tsx +3/-3"]
pr_0 --> f12
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
4 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 4 |
| 🟠 Major | 2 |
| 🟡 Minor | 2 |
Review details
- Reviewed at: 2026-04-08T02:24:35.422Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 82.5s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟠 MAJOR — Lazy Loading Defeated (confidence: 100%)
All dynamic import() calls are executed eagerly at module load time since they are invoked inline in the object literal. Every app package will begin loading simultaneously when this module is first imported, defeating the stated goal of code splitting and lazy loading. The Promises are created immediately — not deferred until first access.
Evidence:
- Each value in the appStore object is
import('./somepackage'), which is evaluated immediately when the object is constructed - The intent specification states the goal is 'to enable code splitting and lazy loading of app store packages'
- For true lazy loading, the imports should be wrapped in functions:
applecalendar: () => import('./applecalendar')and called at point of use
Agent: architecture
| const calendar = getCalendar(credential); | ||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Unawaited Promise in Type Contract (confidence: 100%)
In getCalendarCredentials(), the calendar field is now assigned the return value of getCalendar(credential) which is a Promise (since getCalendar is now async). However, the function is not async and does not await this value. The returned objects have calendar as a Promise, not a resolved Calendar. Any consumer that destructures calendar and checks if (!calendar) will get a truthy Promise object, bypassing null checks and leading to runtime errors when calling calendar methods on a Promise.
Evidence:
- Line 28:
const calendar = getCalendar(credential);— getCalendar is now async (returns Promise) - getCalendarCredentials is synchronous — it does not await the result
- Line 47 in getConnectedCalendars correctly does
const calendar = await item.calendar;which works around this, but getCachedResults on line 141 doesawait Promise.all(calendarCredentials.map((credential) => getCalendar(credential)))— a separate call rather than using getCalendarCredentials - The type contract is implicitly broken: consumers expect
calendar: Calendar | nullbut receivecalendar: Promise
Agent: architecture
| } | ||
| ): Promise<boolean> => { | ||
| const paymentApp = appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore]; | ||
| const paymentApp = await appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore]; |
There was a problem hiding this comment.
🟡 MINOR — Error Handling Gap (confidence: 93%)
The await appStore[...] call has no try/catch. If the dynamic import fails (e.g., missing module, syntax error in the imported module), an unhandled exception will propagate up. Given this is in a payment deletion flow, a failed import could leave payments in an inconsistent state.
Evidence:
- Line 16:
const paymentApp = await appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore]; - Dynamic imports can reject if the module is not found or has errors
- The same pattern in handlePayment.ts (line 26) and handleCancelBooking.ts (line 589) also lacks try/catch around the dynamic import
Agent: architecture
| if (reference.type.includes("_calendar")) { | ||
| const calendar = getCalendar(credential); | ||
| const calendar = await getCalendar(credential); | ||
| if (calendar) { |
There was a problem hiding this comment.
🟡 MINOR — Inconsistent Async Pattern (confidence: 79%)
The await getCalendar(credential) calls on lines 243 and 265 are inside a for...of loop iterating over booking references. Each calendar is fetched sequentially. While correct, this is inconsistent with how getCachedResults uses Promise.all for parallel resolution. For cancel booking flows with multiple calendar references, sequential awaiting could add unnecessary latency.
Evidence:
- Lines 243, 265:
const calendar = await getCalendar(credential);inside a for loop - CalendarManager.ts line 141 uses
Promise.allfor the same pattern
Agent: architecture
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (58 lines, 1 file) — review recommended)
All dynamic
import()calls are executed eagerly at module load time since they are invoked inline in the object literal. Every app package will begin loading simultaneously when this module is first imported, defeating the stated goal of code splitting and lazy loading. The Promises are created immediately — not deferred until first access.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -1,31 +1,31 @@
const appStore = {
- // example: import("./example"),
- applecalendar: import("./applecalendar"),
- caldavcalendar: import("./caldavcalendar"),
- closecom: import("./closecom"),
- dailyvideo: import("./dailyvideo"),
- googlecalendar: import("./googlecalendar"),
- googlevideo: import("./googlevideo"),
- hubspot: import("./hubspot"),
- huddle01video: import("./huddle01video"),
- jitsivideo: import("./jitsivideo"),
- larkcalendar: import("./larkcalendar"),
- office365calendar: import("./office365calendar"),
- office365video: import("./office365video"),
- plausible: import("./plausible"),
- salesforce: import("./salesforce"),
- zohocrm: import("./zohocrm"),
- sendgrid: import("./sendgrid"),
- stripepayment: import("./stripepayment"),
- tandemvideo: import("./tandemvideo"),
- vital: import("./vital"),
- zoomvideo: import("./zoomvideo"),
- wipemycalother: import("./wipemycalother"),
- giphy: import("./giphy"),
- zapier: import("./zapier"),
- exchange2013calendar: import("./exchange2013calendar"),
- exchange2016calendar: import("./exchange2016calendar"),
- exchangecalendar: import("./exchangecalendar"),
- facetime: import("./facetime"),
- sylapsvideo: import("./sylapsvideo"),
+ // example: () => import("./example"),
+ applecalendar: () => import("./applecalendar"),
+ caldavcalendar: () => import("./caldavcalendar"),
+ closecom: () => import("./closecom"),
+ dailyvideo: () => import("./dailyvideo"),
+ googlecalendar: () => import("./googlecalendar"),
+ googlevideo: () => import("./googlevideo"),
+ hubspot: () => import("./hubspot"),
+ huddle01video: () => import("./huddle01video"),
+ jitsivideo: () => import("./jitsivideo"),
+ larkcalendar: () => import("./larkcalendar"),
+ office365calendar: () => import("./office365calendar"),
+ office365video: () => import("./office365video"),
+ plausible: () => import("./plausible"),
+ salesforce: () => import("./salesforce"),
+ zohocrm: () => import("./zohocrm"),
+ sendgrid: () => import("./sendgrid"),
+ stripepayment: () => import("./stripepayment"),
+ tandemvideo: () => import("./tandemvideo"),
+ vital: () => import("./vital"),
+ zoomvideo: () => import("./zoomvideo"),
+ wipemycalother: () => import("./wipemycalother"),
+ giphy: () => import("./giphy"),
+ zapier: () => import("./zapier"),
+ exchange2013calendar: () => import("./exchange2013calendar"),
+ exchange2016calendar: () => import("./exchange2016calendar"),
+ exchangecalendar: () => import("./exchangecalendar"),
+ facetime: () => import("./facetime"),
+ sylapsvideo: () => import("./sylapsvideo"),
};
export default appStore;🤖 Grapple PR auto-fix • major • Review this diff before applying
| const calendar = getCalendar(credential); | ||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (26 lines, 1 file) — review recommended)
In
getCalendarCredentials(), thecalendarfield is now assigned the return value ofgetCalendar(credential)which is aPromise<Calendar | null>(since getCalendar is now async). However, the function is not async and does not await this value. The returned objects havecalendaras a Promise, not a resolved Calendar. Any consumer that destructurescalendarand checksif (!calendar)will get a truthy Promise object, bypassing null checks and leading to runtime errors when calling calendar methods on a Promise.
--- a/packages/core/CalendarManager.ts
+++ b/packages/core/CalendarManager.ts
@@ -23,15 +23,18 @@ const log = logger.getChildLogger({ prefix: ["CalendarManager"] });
-export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => {
- const calendarCredentials = getApps(credentials)
+export const getCalendarCredentials = async (credentials: Array<CredentialPayload>) => {
+ const calendarCredentials = await Promise.all(
+ getApps(credentials)
.filter((app) => app.type.endsWith("_calendar"))
- .flatMap((app) => {
- const credentials = app.credentials.flatMap((credential) => {
- const calendar = getCalendar(credential);
- return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : [];
- });
-
+ .flatMap((app) =>
+ app.credentials.flatMap((credential) =>
+ app.variant === "calendar"
+ ? [getCalendar(credential).then((calendar) => ({ integration: app, credential, calendar }))]
+ : []
+ )
+ )
+ ).then((results) =>
+ results.filter((item): item is Exclude<typeof item, { calendar: null }> & { calendar: NonNullable<typeof item.calendar> } | typeof item =>
+ true
+ )
+ );
- return credentials.length ? credentials : [];
- });
-
return calendarCredentials;
};🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| ): Promise<boolean> => { | ||
| const paymentApp = appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore]; | ||
| const paymentApp = await appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore]; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (13 lines, 1 file) — review recommended)
The
await appStore[...]call has no try/catch. If the dynamic import fails (e.g., missing module, syntax error in the imported module), an unhandled exception will propagate up. Given this is in a payment deletion flow, a failed import could leave payments in an inconsistent state.
--- a/packages/lib/payment/deletePayment.ts
+++ b/packages/lib/payment/deletePayment.ts
@@ -13,7 +13,15 @@ const deletePayment = async (
} | null;
}
): Promise<boolean> => {
- const paymentApp = await appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore];
+ // Dynamic import may reject if the module is missing or has a syntax error;
+ // return false to signal failure without leaving the caller in an inconsistent state.
+ let paymentApp;
+ try {
+ paymentApp = await appStore[paymentAppCredentials?.app?.dirName as keyof typeof appStore];
+ } catch (e) {
+ console.warn(
+ `Payment app module for "${paymentAppCredentials?.app?.dirName}" could not be loaded:`,
+ e
+ );
+ return false;
+ }
if (!(paymentApp && "lib" in paymentApp && "PaymentService" in paymentApp.lib)) {
console.warn(`payment App service of type ${paymentApp} is not implemented`);
return false;🤖 Grapple PR auto-fix • minor • Review this diff before applying
| if (reference.type.includes("_calendar")) { | ||
| const calendar = getCalendar(credential); | ||
| const calendar = await getCalendar(credential); | ||
| if (calendar) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (32 lines, 1 file) — review recommended)
The
await getCalendar(credential)calls on lines 243 and 265 are inside afor...ofloop iterating over booking references. Each calendar is fetched sequentially. While correct, this is inconsistent with how getCachedResults usesPromise.allfor parallel resolution. For cancel booking flows with multiple calendar references, sequential awaiting could add unnecessary latency.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -240,7 +240,7 @@ async function handler(req: CustomRequest) {
integrationsToDelete.push(deleteMeeting(credential, reference.uid));
}
if (reference.type.includes("_calendar")) {
- const calendar = getCalendar(credential);
+ const calendar = await getCalendar(credential);
if (calendar) {
integrationsToDelete.push(
calendar?.deleteEvent(reference.uid, evt, reference.externalCalendarId)
@@ -262,7 +262,7 @@ async function handler(req: CustomRequest) {
);
}
if (reference.type.includes("_calendar")) {
- const calendar = getCalendar(credential);
+ const calendar = await getCalendar(credential);
if (calendar) {
integrationsToDelete.push(
calendar?.updateEvent(reference.uid, updatedEvt, reference.externalCalendarId)
@@ -449,7 +449,7 @@ async function handler(req: CustomRequest) {
(credential) => credential.id === credentialId
);
if (calendarCredential) {
- const calendar = getCalendar(calendarCredential);
+ const calendar = await getCalendar(calendarCredential);
if (
bookingToDelete.eventType?.recurringEvent &&
bookingToDelete.recurringEventId &&
@@ -458,10 +458,10 @@ async function handler(req: CustomRequest) {
- bookingToDelete.user.credentials
- .filter((credential) => credential.type.endsWith("_calendar"))
- .forEach(async (credential) => {
- const calendar = getCalendar(credential);
+ const calendarCredentialsForRecurring = bookingToDelete.user.credentials.filter((credential) =>
+ credential.type.endsWith("_calendar")
+ );
+ for (const credential of calendarCredentialsForRecurring) {
+ const calendar = await getCalendar(credential);
for (const updBooking of updatedBookings) {
const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
if (bookingRef) {
@@ -471,7 +471,7 @@ async function handler(req: CustomRequest) {
}
}
- });
+ }
}
} else {
// For bookings made before the refactor we go through the old behaviour of running through each calendar credential
- bookingToDelete.user.credentials
- .filter((credential) => credential.type.endsWith("_calendar"))
- .forEach((credential) => {
- const calendar = getCalendar(credential);
- apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>);
- });
+ const calendarCredentials = bookingToDelete.user.credentials.filter((credential) =>
+ credential.type.endsWith("_calendar")
+ );
+ for (const credential of calendarCredentials) {
+ const calendar = await getCalendar(credential);
+ apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>);
+ }
}
}
@@ -586,7 +586,7 @@ async function handler(req: CustomRequest) {
// Posible to refactor TODO:
- const paymentApp = appStore[paymentAppCredential?.app?.dirName as keyof typeof appStore];
+ const paymentApp = await appStore[paymentAppCredential?.app?.dirName as keyof typeof appStore];
if (!(paymentApp && "lib" in paymentApp && "PaymentService" in paymentApp.lib)) {
console.warn(`payment App service of type ${paymentApp} is not implemented`);
return null;🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR converts the appStore package imports from static synchronous imports to dynamic async imports (using import() expressions), and updates all call sites to await the resulting promises. The goal is to enable lazy/async loading of app store packages to improve startup performance.
Changes: 12 files (+82 / -107 lines)
- 🟡 12 files modified:
packages/app-store/_utils/getCalendar.ts,packages/app-store/index.ts,packages/app-store/vital/lib/reschedule.ts,packages/app-store/wipemycalother/lib/reschedule.ts,packages/core/CalendarManager.ts+7 more
Resolves: #2 (Add guest management functionality to existing bookings)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 12 files"/]
f1["🟡 getCalendar.ts +2/-2"]
pr_0 --> f1
f2["🟡 index.ts +29/-59"]
pr_0 --> f2
f3["🟡 reschedule.ts +2/-2"]
pr_0 --> f3
f4["🟡 reschedule.ts +2/-2"]
pr_0 --> f4
f5["🟡 CalendarManager.ts +8/-7"]
pr_0 --> f5
f6["🟡 EventManager.ts +1/-2"]
pr_0 --> f6
f7["🟡 videoClient.ts +20/-16"]
pr_0 --> f7
f8["🟡 handleCancelBooking.ts +12/-11"]
pr_0 --> f8
f9["🟡 handleNewBooking.ts +1/-1"]
pr_0 --> f9
f10["🟡 deletePayment.ts +1/-1"]
pr_0 --> f10
f11["🟡 handlePayment.ts +1/-1"]
pr_0 --> f11
f12["🟡 bookings.tsx +3/-3"]
pr_0 --> f12
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
4 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 4 |
| 🟠 Major | 3 |
| 🟡 Minor | 1 |
Review details
- Reviewed at: 2026-04-12T02:15:25.197Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 74.7s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| @@ -28,6 +28,7 @@ export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => | |||
| const calendar = getCalendar(credential); | |||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | |||
There was a problem hiding this comment.
🟠 MAJOR — Unresolved Promise stored in object (confidence: 100%)
getCalendar() is now async and returns Promise, but in getCalendarCredentials() the .map() callback is not async and does not await the result. This means item.calendar is a Promise, not Calendar | null. While getConnectedCalendars() at line 47 does await item.calendar, any other consumer of getCalendarCredentials() that accesses .calendar without awaiting will receive a Promise object that is always truthy, bypassing null checks.
Evidence:
- Line 28:
const calendar = getCalendar(credential);— getCalendar is now async, so this returns a Promise - Line 29:
return app.variant === 'calendar' ? [{ integration: app, credential, calendar }] : [];— calendar is a Promise here - The type system may not catch this if the return type is not explicitly typed, since Promise is a valid object
- Any code that does
if (!item.calendar)will always be false since a Promise object is truthy
Agent: logic
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟠 MAJOR — No lazy loading benefit (confidence: 99%)
The dynamic import() expressions are invoked immediately as object property initializers when the module is loaded. This means all app store modules are still loaded eagerly at startup — the imports execute immediately and produce Promises that resolve with the already-loading modules. The stated performance goal of lazy/async loading is not achieved; instead, this adds Promise resolution overhead to every call site without any code-splitting benefit.
Evidence:
- Each property value is
import('./...')which executes immediately when the object literal is evaluated - The appStore object is evaluated at module load time (top-level const)
- True lazy loading would require wrapping in arrow functions:
applecalendar: () => import('./applecalendar')and calling only when needed
Agent: logic
| const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential))); | ||
| performance.mark("getBusyCalendarTimesStart"); | ||
| const results = calendars.map(async (c, i) => { | ||
| /** Filter out nulls */ |
There was a problem hiding this comment.
🟠 MAJOR — Null/undefined guard after await (confidence: 94%)
getCachedResults resolves calendars via Promise.all(map(getCalendar)), but getCalendar can return null. The subsequent results map at line 142 iterates over calendars and checks if (!c) — however, the null calendars still produce entries in the results array. The existing code already handled this, but it's worth verifying that the Promise.all resolution doesn't change behavior when some promises resolve to null.
Evidence:
- Line 141:
const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential))); - getCalendar can return null for invalid/missing credentials
- Line 142-143: The existing null check
if (!c)should still work since await null === null
Agent: logic
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 MINOR — Type safety (confidence: 95%)
The type of appStore values changed from module namespace objects to Promise. However, keyof typeof appStore and typeof appStore used in call sites (e.g., appStore[key as keyof typeof appStore]) now represent Promise-typed values. Any call site not included in this PR that accesses appStore without awaiting will receive a Promise object instead of a module, and since Promise objects are truthy and have no lib property, the 'lib' in app guard will return false — silently failing instead of throwing an error.
Evidence:
- Multiple call sites use
appStore[... as keyof typeof appStore] - The type of each value is now
Promiseinstead oftypeof import('./...') - Any uncovered call site will silently fail due to the
'lib' in appguard returning false for a Promise object
Agent: logic
| const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential))); | ||
| performance.mark("getBusyCalendarTimesStart"); | ||
| const results = calendars.map(async (c, i) => { | ||
| /** Filter out nulls */ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
getCachedResults resolves calendars via Promise.all(map(getCalendar)), but getCalendar can return null. The subsequent
resultsmap at line 142 iterates over calendars and checksif (!c)— however, the null calendars still produce entries in the results array. The existing code already handled this, but it's worth verifying that the Promise.all resolution doesn't change behavior when some promises resolve to null.
| /** Filter out nulls */ | |
| const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential))); |
🤖 Grapple PR auto-fix • major • confidence: 94%
| @@ -28,6 +28,7 @@ export const getCalendarCredentials = (credentials: Array<CredentialPayload>) => | |||
| const calendar = getCalendar(credential); | |||
| return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (16 lines, 1 file) — review recommended)
getCalendar() is now async and returns Promise<Calendar | null>, but in getCalendarCredentials() the .map() callback is not async and does not await the result. This means
item.calendaris a Promise<Calendar | null>, not Calendar | null. While getConnectedCalendars() at line 47 doesawait item.calendar, any other consumer of getCalendarCredentials() that accesses.calendarwithout awaiting will receive a Promise object that is always truthy, bypassing null checks.
--- a/packages/core/CalendarManager.ts
+++ b/packages/core/CalendarManager.ts
@@ -26,7 +26,7 @@ export const getCalendarCredentials = (credentials: Array<CredentialPayload>) =>
const credentials = app.credentials.flatMap((credential) => {
const calendar = getCalendar(credential);
- return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : [];
+ return app.variant === "calendar" ? [{ integration: app, credential, calendar: calendar as Promise<Calendar | null> }] : [];
});
return credentials.length ? credentials : [];
@@ -44,8 +44,8 @@ export const getConnectedCalendars = async (
const connectedCalendars = await Promise.all(
calendarCredentials.map(async (item) => {
try {
- const { integration, credential } = item;
- const calendar = await item.calendar;
+ const { integration, credential } = item;
+ const calendar = await item.calendar;
// Don't leak credentials to the client
const credentialId = credential.id;
if (!calendar) {
@@ -139,7 +139,7 @@ export const getCachedResults = async (
selectedCalendars: SelectedCalendar[]
): Promise<EventBusyDate[][]> => {
const calendarCredentials = withCredentials.filter((credential) => credential.type.endsWith("_calendar"));
- const calendars = calendarCredentials.map((credential) => getCalendar(credential));
+ const calendars = await Promise.all(calendarCredentials.map((credential) => getCalendar(credential)));
performance.mark("getBusyCalendarTimesStart");
const results = calendars.map(async (c, i) => {
/** Filter out nulls */
@@ -230,7 +230,7 @@ export const createEvent = async (
calEvent: CalendarEvent
): Promise<EventResult<NewCalendarEventType>> => {
const uid: string = getUid(calEvent);
- const calendar = getCalendar(credential);
+ const calendar = await getCalendar(credential);
let success = true;
let calError: string | undefined = undefined;
@@ -281,7 +281,7 @@ export const updateEvent = async (
externalCalendarId: string | null
): Promise<EventResult<NewCalendarEventType>> => {
const uid = getUid(calEvent);
- const calendar = getCalendar(credential);
+ const calendar = await getCalendar(credential);
let success = false;
let calError: string | undefined = undefined;
let calWarnings: string[] | undefined = [];
@@ -327,12 +327,12 @@ export const updateEvent = async (
};
};
-export const deleteEvent = (
+export const deleteEvent = async (
credential: CredentialPayload,
uid: string,
event: CalendarEvent
): Promise<unknown> => {
- const calendar = getCalendar(credential);
+ const calendar = await getCalendar(credential);
if (calendar) {
return calendar.deleteEvent(uid, event);
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (58 lines, 1 file) — review recommended)
The dynamic import() expressions are invoked immediately as object property initializers when the module is loaded. This means all app store modules are still loaded eagerly at startup — the imports execute immediately and produce Promises that resolve with the already-loading modules. The stated performance goal of lazy/async loading is not achieved; instead, this adds Promise resolution overhead to every call site without any code-splitting benefit.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -1,31 +1,31 @@
const appStore = {
- // example: import("./example"),
- applecalendar: import("./applecalendar"),
- caldavcalendar: import("./caldavcalendar"),
- closecom: import("./closecom"),
- dailyvideo: import("./dailyvideo"),
- googlecalendar: import("./googlecalendar"),
- googlevideo: import("./googlevideo"),
- hubspot: import("./hubspot"),
- huddle01video: import("./huddle01video"),
- jitsivideo: import("./jitsivideo"),
- larkcalendar: import("./larkcalendar"),
- office365calendar: import("./office365calendar"),
- office365video: import("./office365video"),
- plausible: import("./plausible"),
- salesforce: import("./salesforce"),
- zohocrm: import("./zohocrm"),
- sendgrid: import("./sendgrid"),
- stripepayment: import("./stripepayment"),
- tandemvideo: import("./tandemvideo"),
- vital: import("./vital"),
- zoomvideo: import("./zoomvideo"),
- wipemycalother: import("./wipemycalother"),
- giphy: import("./giphy"),
- zapier: import("./zapier"),
- exchange2013calendar: import("./exchange2013calendar"),
- exchange2016calendar: import("./exchange2016calendar"),
- exchangecalendar: import("./exchangecalendar"),
- facetime: import("./facetime"),
- sylapsvideo: import("./sylapsvideo"),
+ // example: () => import("./example"),
+ applecalendar: () => import("./applecalendar"),
+ caldavcalendar: () => import("./caldavcalendar"),
+ closecom: () => import("./closecom"),
+ dailyvideo: () => import("./dailyvideo"),
+ googlecalendar: () => import("./googlecalendar"),
+ googlevideo: () => import("./googlevideo"),
+ hubspot: () => import("./hubspot"),
+ huddle01video: () => import("./huddle01video"),
+ jitsivideo: () => import("./jitsivideo"),
+ larkcalendar: () => import("./larkcalendar"),
+ office365calendar: () => import("./office365calendar"),
+ office365video: () => import("./office365video"),
+ plausible: () => import("./plausible"),
+ salesforce: () => import("./salesforce"),
+ zohocrm: () => import("./zohocrm"),
+ sendgrid: () => import("./sendgrid"),
+ stripepayment: () => import("./stripepayment"),
+ tandemvideo: () => import("./tandemvideo"),
+ vital: () => import("./vital"),
+ zoomvideo: () => import("./zoomvideo"),
+ wipemycalother: () => import("./wipemycalother"),
+ giphy: () => import("./giphy"),
+ zapier: () => import("./zapier"),
+ exchange2013calendar: () => import("./exchange2013calendar"),
+ exchange2016calendar: () => import("./exchange2016calendar"),
+ exchangecalendar: () => import("./exchangecalendar"),
+ facetime: () => import("./facetime"),
+ sylapsvideo: () => import("./sylapsvideo"),
};
export default appStore;🤖 Grapple PR auto-fix • major • Review this diff before applying
| exchangecalendar: import("./exchangecalendar"), | ||
| facetime: import("./facetime"), | ||
| sylapsvideo: import("./sylapsvideo"), | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (18 lines, 1 file) — review recommended)
The type of
appStorevalues changed from module namespace objects toPromise<typeof import(...)>. However,keyof typeof appStoreandtypeof appStoreused in call sites (e.g.,appStore[key as keyof typeof appStore]) now represent Promise-typed values. Any call site not included in this PR that accesses appStore without awaiting will receive a Promise object instead of a module, and since Promise objects are truthy and have nolibproperty, the'lib' in appguard will return false — silently failing instead of throwing an error.
--- a/packages/app-store/index.ts
+++ b/packages/app-store/index.ts
@@ -1,33 +1,51 @@
const appStore = {
// example: import("./example"),
applecalendar: import("./applecalendar"),
caldavcalendar: import("./caldavcalendar"),
closecom: import("./closecom"),
dailyvideo: import("./dailyvideo"),
googlecalendar: import("./googlecalendar"),
googlevideo: import("./googlevideo"),
hubspot: import("./hubspot"),
huddle01video: import("./huddle01video"),
jitsivideo: import("./jitsivideo"),
larkcalendar: import("./larkcalendar"),
office365calendar: import("./office365calendar"),
office365video: import("./office365video"),
plausible: import("./plausible"),
salesforce: import("./salesforce"),
zohocrm: import("./zohocrm"),
sendgrid: import("./sendgrid"),
stripepayment: import("./stripepayment"),
tandemvideo: import("./tandemvideo"),
vital: import("./vital"),
zoomvideo: import("./zoomvideo"),
wipemycalother: import("./wipemycalother"),
giphy: import("./giphy"),
zapier: import("./zapier"),
exchange2013calendar: import("./exchange2013calendar"),
exchange2016calendar: import("./exchange2016calendar"),
exchangecalendar: import("./exchangecalendar"),
facetime: import("./facetime"),
sylapsvideo: import("./sylapsvideo"),
};
+export type AppStoreModules = typeof appStore;
+export type AppKeys = keyof AppStoreModules;
+
+/**
+ * Type-safe async accessor for appStore entries.
+ * Always use this instead of `appStore[key]` directly to ensure the Promise is
+ * awaited before use. Direct indexing yields a Promise object — accessing
+ * properties like `lib` on it will silently return undefined rather than
+ * throwing, because Promise objects are truthy and `'lib' in promise === false`.
+ *
+ * @example
+ * const app = await getAppStore("zoomvideo");
+ * if (app && "lib" in app) { ... }
+ */
+export async function getAppStore<T extends AppKeys>(name: T): Promise<Awaited<AppStoreModules[T]>> {
+ return appStore[name];
+}
+
export default appStore;🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #2 - Replicating Greptile benchmark for Grapple PR evaluation.