Conversation
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
keithwillcode
added
core
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
merge this only after #24123 |
dosubot
bot
added
bookings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/features/calendar-view/LargeCalendar.tsx (1)
59-60: Prefer stable booking identifiers.Using the loop index as the event
idwill reshuffle identities whenever the list changes length/order, which can confuse the calendar rendering. We already receivebooking.id; please use that (or another stable unique key) instead.- id: idx, + id: booking.id ?? idx,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/eighty-walls-send.md(1 hunks)docs/platform/atoms/calendar-view.mdx(2 hunks)packages/features/calendar-view/LargeCalendar.tsx(4 hunks)packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/calendar-view/LargeCalendar.tsxpackages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/calendar-view/LargeCalendar.tsxpackages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/calendar-view/LargeCalendar.tsxpackages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-18T13:38:50.958Z
Learnt from: hariombalhara
PR: calcom/cal.com#23913
File: packages/features/bookings/Booker/components/hooks/usePrefetch.ts:43-50
Timestamp: 2025-09-18T13:38:50.958Z
Learning: In usePrefetch hook in packages/features/bookings/Booker/components/hooks/usePrefetch.ts, the !isNaN() checks before comparing monthAfterAdding1Month !== monthAfterAddingExtraDaysColumnView are intentional to prevent the NaN !== NaN issue where NaN values would incorrectly be considered different, leading to wrong monthCount calculations.
Applied to files:
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
🧬 Code graph analysis (2)
packages/features/calendar-view/LargeCalendar.tsx (2)
packages/features/bookings/types.ts (1)
BookerEvent(34-69)packages/platform/atoms/index.ts (1)
useBookings(16-16)
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx (2)
packages/platform/atoms/booker/types.ts (2)
BookerPlatformWrapperAtomPropsForIndividual(91-95)BookerPlatformWrapperAtomPropsForTeam(97-102)packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-56)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
| | eventSlug | Yes | Unique slug created for a particular event | | ||
| | username | Optional | Username of the person whose schedule is to be displayed, required only for individual events | | | | ||
| | teamId | Optional | Id of the team for which the event is created, required only for team events | No newline at end of file |
There was a problem hiding this comment.
Fix table column delimiters.
The username row now has two extra | delimiters, which breaks the three-column layout when rendered. Please drop the trailing separators so the table stays well-formed.
-| username | Optional | Username of the person whose schedule is to be displayed, required only for individual events | | |
+| username | Optional | Username of the person whose schedule is to be displayed, required only for individual events |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | eventSlug | Yes | Unique slug created for a particular event | | |
| | username | Optional | Username of the person whose schedule is to be displayed, required only for individual events | | | | |
| | teamId | Optional | Id of the team for which the event is created, required only for team events | | |
| | eventSlug | Yes | Unique slug created for a particular event | | |
| | username | Optional | Username of the person whose schedule is to be displayed, required only for individual events | | |
| | teamId | Optional | Id of the team for which the event is created, required only for team events | |
🤖 Prompt for AI Agents
In docs/platform/atoms/calendar-view.mdx around lines 58 to 60, the markdown
table row for "username" contains extra pipe characters that create a fourth
empty column and break the three-column layout; remove the two trailing '|'
delimiters from that row so it has exactly three pipe-separated columns (column
name, requirement, description) matching the other rows and keep the table
aligned.
| title: booking.title ?? `Busy`, | ||
| start: new Date(booking.start), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Localize the “Busy” label.
Frontend strings should be wrapped with t() per our TSX guideline. Please move this fallback through the translation layer (e.g. t("busy")) to keep the atom localizable.
As per coding guidelines
supalarry
left a comment
supalarry
left a comment
There was a problem hiding this comment.
Good stuff - have some comments and questions.
I also think it would be good that Sean reviews this since it affects consumer component.
| id: idx, | ||
| title: booking.title ?? `Busy`, | ||
| start: new Date(booking.start), | ||
| end: new Date(booking.end), |
There was a problem hiding this comment.
Why are we switching to new Date from dayjs(event.end).toDate() ?
There was a problem hiding this comment.
no real reason tbh, just for consistency with trouble shooter component here for consistency
| skip: 0, | ||
| status: ["upcoming", "past", "recurring"], | ||
| eventTypeId: event?.data?.id, | ||
| afterStart: startDate.toISOString(), |
There was a problem hiding this comment.
Question - the start and end dates here refer to 1 week or we would fetch for whole month?
Question 2- what if someone has more than 50 bookings?
There was a problem hiding this comment.
the start and end dates here refer to 1 week or we would fetch for whole month
for 1 week because, that way we reduce the load to the server and only fetch data that's needed, which the user clicks the next week button that's when we fetch bookings for that particular the user is on
what if someone has more than 50 bookings?
hmm not sure here, should we increase the number of bookings then to 150? I think ideally its really hard to have 150 bookings for just for one week, what do you think about this?
There was a problem hiding this comment.
updated bookings fetch count to 150
E2E results are ready! |
Ryukemeister
deleted the
rajiv/cal-5461-feature-upcoming-bookings-atom
branch
3 participants
What does this PR do?
Image Demo (if applicable):
For team events
For individual events
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
This can be tested in
packages/platform/examples/baseweek view page