Conversation
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
keithwillcode
added
core
hariombalhara
force-pushed
the
dont-prefetch-next-2-months-when-2-weeks-cross
branch
from
260011b to
811d4fb
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
hariombalhara
force-pushed
the
dont-prefetch-next-2-months-when-2-weeks-cross
branch
from
811d4fb to
60b36f9
Compare
hariombalhara
force-pushed
the
dont-prefetch-next-2-months-when-2-weeks-cross
branch
from
60b36f9 to
f8deee1
Compare
dosubot
bot
added
platform
| !isNaN(_monthAfterAdding1Month) && | ||
| !isNaN(_monthAfterAddingExtraDaysColumnView) && |
There was a problem hiding this comment.
This is the fix. Everything else is just extracted variables
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
67-67: Consider validating date format before useThe date is formatted without first checking if
selectedDateis valid. While Day.js handles invalid dates gracefully, it's better to validate early.Add validation:
- const date = dayjs(selectedDate).format("YYYY-MM-DD"); + const date = selectedDate && dayjs(selectedDate).isValid() + ? dayjs(selectedDate).format("YYYY-MM-DD") + : dayjs().format("YYYY-MM-DD");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/platform/atoms/booker/BookerWebWrapper.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/platform/atoms/booker/BookerWebWrapper.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/platform/atoms/booker/BookerWebWrapper.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/platform/atoms/booker/BookerWebWrapper.tsx
🧬 Code graph analysis (1)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
packages/embeds/embed-core/src/types.ts (1)
BookerLayouts(6-6)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
packages/platform/atoms/booker/BookerWebWrapper.tsx (3)
154-161: Improved month-boundary logic for prefetchingThe refactored prefetch logic correctly handles NaN comparisons by explicitly checking for valid dates and using derived month calculations. This prevents the
NaN !== NaNissue that would always evaluate to true.
163-170: Good NaN guards but consider edge casesThe explicit NaN checks prevent invalid comparisons. However, the condition might be overly restrictive - both values must be valid numbers for the comparison to work.
Consider handling the case where only one value is NaN:
const monthCount = ((bookerLayout.layout !== BookerLayouts.WEEK_VIEW && bookerState === "selecting_time") || bookerLayout.layout === BookerLayouts.COLUMN_VIEW) && - !isNaN(_monthAfterAdding1Month) && - !isNaN(_monthAfterAddingExtraDaysColumnView) && - _monthAfterAdding1Month !== _monthAfterAddingExtraDaysColumnView + !isNaN(_monthAfterAdding1Month) && + !isNaN(_monthAfterAddingExtraDaysColumnView) && + _monthAfterAdding1Month !== _monthAfterAddingExtraDaysColumnView ? 2 + : !isNaN(_monthAfterAdding1Month) && !isNaN(_monthAfterAddingExtraDaysColumnView) + ? undefined : undefined;Actually, the current logic is fine - returning
undefinedfor any NaN case is the safe default.
149-153: Do not change — using the storemonthhere is intentional.The
monthvalue is the Booker store's browsing month (YYYY‑MM, see packages/features/bookings/Booker/store.ts) and the prefetch logic intentionally checks whether the browsing month equals the current month and whether today is past two weeks into that month (packages/platform/atoms/booker/BookerWebWrapper.tsx ~lines 149–152). Replacing with thedateparameter would change the semantics and can cause incorrect prefetching.Likely an incorrect or invalid review comment.
| const _month = dayjs(date).month(); | ||
| const _monthAfterAdding1Month = dayjs(date).add(1, "month").month(); | ||
| const _monthAfterAddingExtraDays = dayjs(date).add(bookerLayout.extraDays, "day").month(); | ||
| const _monthAfterAddingExtraDaysColumnView = dayjs(date) | ||
| .add(bookerLayout.columnViewExtraDays.current, "day") | ||
| .month(); | ||
|
|
There was a problem hiding this comment.
Fix NaN propagation in month calculations
The current implementation doesn't handle invalid dates properly. When date is invalid, dayjs(date).month() returns NaN, which then propagates through all the derived calculations. This can cause the prefetch logic to behave unexpectedly.
Add validation to prevent NaN propagation:
- const _month = dayjs(date).month();
- const _monthAfterAdding1Month = dayjs(date).add(1, "month").month();
- const _monthAfterAddingExtraDays = dayjs(date).add(bookerLayout.extraDays, "day").month();
- const _monthAfterAddingExtraDaysColumnView = dayjs(date)
- .add(bookerLayout.columnViewExtraDays.current, "day")
- .month();
+ const parsedDate = dayjs(date);
+ const _month = parsedDate.isValid() ? parsedDate.month() : NaN;
+ const _monthAfterAdding1Month = parsedDate.isValid() ? parsedDate.add(1, "month").month() : NaN;
+ const _monthAfterAddingExtraDays = parsedDate.isValid() ? parsedDate.add(bookerLayout.extraDays, "day").month() : NaN;
+ const _monthAfterAddingExtraDaysColumnView = parsedDate.isValid()
+ ? parsedDate.add(bookerLayout.columnViewExtraDays.current, "day").month()
+ : NaN;📝 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.
| const _month = dayjs(date).month(); | |
| const _monthAfterAdding1Month = dayjs(date).add(1, "month").month(); | |
| const _monthAfterAddingExtraDays = dayjs(date).add(bookerLayout.extraDays, "day").month(); | |
| const _monthAfterAddingExtraDaysColumnView = dayjs(date) | |
| .add(bookerLayout.columnViewExtraDays.current, "day") | |
| .month(); | |
| const parsedDate = dayjs(date); | |
| const _month = parsedDate.isValid() ? parsedDate.month() : NaN; | |
| const _monthAfterAdding1Month = parsedDate.isValid() ? parsedDate.add(1, "month").month() : NaN; | |
| const _monthAfterAddingExtraDays = parsedDate.isValid() ? parsedDate.add(bookerLayout.extraDays, "day").month() : NaN; | |
| const _monthAfterAddingExtraDaysColumnView = parsedDate.isValid() | |
| ? parsedDate.add(bookerLayout.columnViewExtraDays.current, "day").month() | |
| : NaN; |
E2E results are ready! |
4 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist