-
Notifications
You must be signed in to change notification settings - Fork 11.6k
refactor: calEventParser CalendarEvent ISP #25890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/lib/CalEventParser.ts">
<violation number="1" location="packages/lib/CalEventParser.ts:181">
P0: Critical bug: When `uid` is null/undefined, the fallback UUID generation now uses `JSON.stringify(uid)` instead of the original `JSON.stringify(calEvent)`. This means all events without a UID will generate the **same** fallback UUID (since `JSON.stringify(null)` always produces `"null"`), leading to potential booking UID collisions.
The original code used the entire calEvent object to generate a unique fallback UID. Consider passing the full calEvent or relevant identifying properties to maintain uniqueness.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
…fix type errors
- Update getUid call sites to pass only uid instead of whole CalendarEvent
- Update getLocation call sites to pass narrow shape with videoCallData, additionalInformation, location, uid
- Update narrow input shapes to accept null for location (matching CalendarEvent type)
- Update recurringEvent type to accept RecurringEvent | null instead of boolean
- Update videoCallData type to be more flexible ({ type?: string; url?: string })
- Fix ManageLink.tsx to pass recurringEvent directly instead of converting to boolean
Co-Authored-By: [email protected] <[email protected]>
- Update getPublicVideoCallUrl test to pass uid instead of calEvent - Update getVideoCallPassword tests to pass videoCallData instead of calEvent Co-Authored-By: [email protected] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 16 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/lib/bookings/getLabelValueMapFromResponses.ts">
<violation number="1" location="packages/lib/bookings/getLabelValueMapFromResponses.ts:6">
P2: Use `import type` for `Prisma` since it's only used for type annotations (`Prisma.JsonObject`). This avoids bundling unnecessary Prisma runtime code.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
59cf9a9 to
37c28c4
Compare
What does this PR do?
Refactored CalEventParser to use narrow input shapes instead of passing the full CalendarEvent. This reduces coupling, improves type safety, and makes helpers easier to reuse.
Summary of Changes
Refactors
getWhat(title),getDescription(t, description),getAppsStatus(t, appsStatus),getUid(uid),getVideoCallPassword(videoCallData)).getLabelValueMapFromResponsesnow takes{ customInputs, userFieldsResponses, responses, eventTypeId }(Prisma/CalEventResponses types).getCancelLinkandgetBookingUrl.Updated Call Sites
Type Fixes
nullforlocation(matching CalendarEvent type)recurringEventtype to acceptRecurringEvent | nullinstead of booleanvideoCallDatatype to be more flexible{ type?: string; url?: string }for compatibility with various call sitesTest Updates
getPublicVideoCallUrl(calEvent.uid)andgetVideoCallPassword(calEvent.videoCallData)Migration
Update call sites to pass only the fields each helper needs:
getWhat(calEvent.title),getDescription(t, calEvent.description),isDailyVideoCall(calEvent.videoCallData),getPublicVideoCallUrl(calEvent.uid)getLocation, pass:{ videoCallData, additionalInformation, location, uid }Mandatory Tasks (DO NOT REMOVE)