fix: Platform Prefetch for slots due to NaN != NaN issue#23913
fix: Platform Prefetch for slots due to NaN != NaN issue#23913ThyMinimalDev merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughAdds a new hook 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 |
|
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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
hariombalhara
force-pushed
the
fix-platform-prefetch-na-comparison
branch
from
f8deee1 to
daed3c8
Compare
hariombalhara
force-pushed
the
fix-platform-prefetch-na-comparison
branch
from
daed3c8 to
6428b9e
Compare
hariombalhara
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (2)
96-101: Removeanyand the eslint disable by typinggetStateValues.You can keep type‑safety without
anyusing a generic andunknown.-// eslint-disable-next-line @typescript-eslint/no-explicit-any -const getStateValues = useCallback((state: any): BookerStoreValues => { +type StoreShape = Record<string, unknown>; +const getStateValues = useCallback((state: StoreShape): BookerStoreValues => { return Object.fromEntries( Object.entries(state).filter(([_, value]) => typeof value !== "function") ) as BookerStoreValues; }, []);
150-153: Revisit blanketreact-hooks/exhaustive-depsdisables.Setter refs from stores are usually stable; if not, wrap them in
useEvent/useStableCallbackor add them safely. Add a brief code comment explaining why suppression is necessary to avoid future regressions.Also applies to: 155-159, 230-231, 467-469
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
5-14: ConstrainbookerLayout.layoutto the published union type
Replace the unboundedstringwith theAllPossibleLayoutsalias (which is"month_view" | "week_view" | "column_view" | "mobile") exported by the embed-core package. This avoids runtime-type gymnastics and ensures future changes to layouts stay in sync. For example:-import type { UsePrefetchParams } from './usePrefetch'; +import type { AllPossibleLayouts } from '@calcom/embeds/embed-core/src/types'; interface UsePrefetchParams { date: string; month: string | null; bookerLayout: { - layout: string; + layout: AllPossibleLayouts; extraDays: number; columnViewExtraDays: { current: number }; }; bookerState: BookerState; }
📜 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 (3)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts(1 hunks)packages/platform/atoms/booker/BookerPlatformWrapper.tsx(5 hunks)packages/platform/atoms/booker/BookerWebWrapper.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/BookerPlatformWrapper.tsxpackages/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/BookerPlatformWrapper.tsxpackages/features/bookings/Booker/components/hooks/usePrefetch.tspackages/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/BookerPlatformWrapper.tsxpackages/features/bookings/Booker/components/hooks/usePrefetch.tspackages/platform/atoms/booker/BookerWebWrapper.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/bookings/Booker/components/hooks/usePrefetch.ts
🧬 Code graph analysis (3)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-52)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (2)
packages/features/bookings/Booker/types.ts (1)
BookerState(155-155)packages/embeds/embed-core/src/types.ts (1)
BookerLayouts(6-6)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-52)
⏰ 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
🔇 Additional comments (4)
packages/platform/atoms/booker/BookerWebWrapper.tsx (2)
24-24: Good move: centralizing prefetch logic.Importing the shared hook reduces duplication and keeps behavior consistent across wrappers.
142-147: Hook usage looks correct.Passing
date,month,bookerLayout, andbookerStatestraight through is aligned with the new API.packages/platform/atoms/booker/BookerPlatformWrapper.tsx (2)
18-18: Nice consolidation with usePrefetch.This lowers divergence from Web wrapper and makes fixes land in one place.
246-253: Correct integration withuseTimesForSchedule.Wiring
prefetchNextMonthandmonthCounthere matches the intent.
Found other direct.month()usages that could produce NaN-driven truthiness — verify and guard them:
- packages/features/troubleshooter/store.ts — lines 57, 68:
newSelection.month() !== currentSelection.month()- packages/features/bookings/Booker/store.ts — lines 218, 240:
newSelection.month() !== currentSelection.month()- packages/features/calendars/DatePicker.tsx — lines 264, 345, 412:
day.month() !== browsingDate.month()/new Date(browsingDate.year(), browsingDate.month())- packages/features/schedules/lib/use-schedule/useTimesForSchedule.ts — line 29:
monthDayjs.month() === now.month()- packages/platform/atoms/booker/BookerPlatformWrapper.tsx — line 240:
month: month || ""- apps/web/playwright/booking-limits.e2e.ts — line 50:
dayjs().month() >= 9 ? ...- packages/features/bookings/Booker/components/hooks/usePrefetch.ts — uses
dayjs(...).month()but already guards with!isNaN(...)(lines ~17–23, 42–44).Add NaN-safe guards (e.g.,
dayjs(...).isValid(),!isNaN(...), or coerce/fallback before comparing) where needed.
E2E results are ready! |
hariombalhara
force-pushed
the
fix-platform-prefetch-na-comparison
branch
2 times, most recently
from
f887bfa to
3ca77a8
Compare
| bookerState: BookerState; | ||
| } | ||
|
|
||
| export const usePrefetch = ({ date, month, bookerLayout, bookerState }: UsePrefetchParams) => { |
There was a problem hiding this comment.
we should use useMemo here, so that the references to prefetchNextMonth,
monthCount change only when date, month, bookerLayout, bookerState change
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
225-231: Stale prefill risk: missing dependency onprefillFormParamName.
prefillFormParamswon’t update whenfirstName/lastNamechanges because it ignoresprefillFormParamName. Remove the suppression and depend on it.const prefillFormParams = useMemo(() => { return { name: prefillFormParamName, guests: defaultGuests ?? [], }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [defaultName, defaultGuests]); + }, [prefillFormParamName, defaultGuests]);
🧹 Nitpick comments (6)
packages/platform/atoms/booker/BookerWebWrapper.tsx (2)
142-147: Verify month source consistency between hook and schedule.
usePrefetchis fedmonthfrom the store, whileuseScheduleForEventbelow usesprops.month. If these can diverge, prefetch decisions may not match the queried month. Consider aligning sources.Apply one of:
- Pass the effective month into the hook:
- const { prefetchNextMonth, monthCount } = usePrefetch({ - date, - month, + const { prefetchNextMonth, monthCount } = usePrefetch({ + date, + month: props.month ?? month, bookerLayout, bookerState, });
- Or switch
useScheduleForEventto the storemonthfor consistency (if that’s the canonical value).
217-221: Avoid disabling exhaustive-deps; guard the effect instead.Remove the lint disable by guarding against redundant pushes and depending on the callback.
- useEffect(() => { - if (hasSession) onOverlaySwitchStateChange(true); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [hasSession]); + useEffect(() => { + if (!hasSession) return; + // Only push if the overlay flag isn't already set + const alreadyOn = searchParams?.get("overlayCalendar") === "true"; + if (!alreadyOn) onOverlaySwitchStateChange(true); + }, [hasSession, onOverlaySwitchStateChange, searchParams]);packages/platform/atoms/booker/BookerPlatformWrapper.tsx (4)
96-101: Dropanyand the lint suppression; type the arg.
BookerStoreValuesis available—use it for the param and return to avoidany.Apply:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const getStateValues = useCallback((state: any): BookerStoreValues => { + const getStateValues = useCallback((state: BookerStoreValues): BookerStoreValues => { return Object.fromEntries( Object.entries(state).filter(([_, value]) => typeof value !== "function") ) as BookerStoreValues; }, []);
150-154: Avoid blanketexhaustive-depsdisable; include stable setter.Include
setSelectedDurationin deps and remove the suppression (Zustand setters are stable).useEffect(() => { setSelectedDuration(props.duration ?? null); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [props.duration]); + }, [props.duration, setSelectedDuration]);If your store returns non-stable setters, keep the suppression but add a code comment explaining why.
156-159: Same here: prefer adding the setter to deps.Keeps lints green without suppression.
useEffect(() => { setOrg(props.entity?.orgSlug ?? null); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [props.entity?.orgSlug]); + }, [props.entity?.orgSlug, setOrg]);
465-469: Minor: include setter in deps to drop lint suppression.Safer to add
setSelectedDatethan to suppress.useEffect(() => { setSelectedDate({ date: selectedDateProp, omitUpdatingParams: true }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [selectedDateProp]); + }, [selectedDateProp, setSelectedDate]);If the setter isn’t stable, keep the suppression and document why.
📜 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 (3)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts(1 hunks)packages/platform/atoms/booker/BookerPlatformWrapper.tsx(5 hunks)packages/platform/atoms/booker/BookerWebWrapper.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/bookings/Booker/components/hooks/usePrefetch.ts
🧰 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/BookerPlatformWrapper.tsxpackages/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/BookerPlatformWrapper.tsxpackages/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/BookerPlatformWrapper.tsxpackages/platform/atoms/booker/BookerWebWrapper.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js `.utc()` in hot paths like loops
Applied to files:
packages/platform/atoms/booker/BookerPlatformWrapper.tsx
🧬 Code graph analysis (2)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-52)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-52)
🔇 Additional comments (4)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
24-24: Good abstraction: prefetch logic moved into a shared hook.Consolidating prefetch/monthCount into
usePrefetchremoves duplication and helps avoid the NaN !== NaN pitfall in multiple places.packages/platform/atoms/booker/BookerPlatformWrapper.tsx (3)
18-18: Hook adoption LGTM.Switching to
usePrefetchcentralizes the NaN-handling logic and removes duplication across wrappers.
238-243: Passing rawmonth(no""coercion) is correct.This avoids Day.js invalid-date pitfalls and aligns with the new hook’s intent. If
monthcan beundefined, coalesce tonullexplicitly.- month, + month: month ?? null,Can you confirm the store’s
monthtype isstring | null(neverundefined)?
489-490: Unmount cleanup effect LGTM.One-time cleanup with an empty dep array is correct here; the suppression is appropriate.
hariombalhara
force-pushed
the
fix-platform-prefetch-na-comparison
branch
from
3ca77a8 to
15ed4d5
Compare
| !isNaN(dateMonth) && | ||
| !isNaN(monthAfterAddingExtraDays) && | ||
| dateMonth !== monthAfterAddingExtraDays) || | ||
| (bookerLayout.layout === BookerLayouts.COLUMN_VIEW && | ||
| !isNaN(dateMonth) && | ||
| !isNaN(monthAfterAddingExtraDaysColumnView) && | ||
| dateMonth !== monthAfterAddingExtraDaysColumnView) || |
There was a problem hiding this comment.
Added these NaN checks as well because the root cause is that date string itself could be invalid causing the issue with all other derived variables from it.
I will be adding unit tests for it in a followup and then this could be simplified when needed, ideally we return early when such a situation arise instead of having NaN checks everywhere
There was a problem hiding this comment.
hariombalhara
force-pushed
the
fix-platform-prefetch-na-comparison
branch
from
772a862 to
c14c092
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
238-243: Thanks for removing month coercion to ""Month is now passed as-is, avoiding Day.js invalid-date pitfalls. This addresses the earlier concern.
🧹 Nitpick comments (5)
packages/platform/atoms/booker/BookerWebWrapper.tsx (2)
67-67: Avoid “Invalid Date” sentinel; pass raw selectedDate/null to the hookFormatting selectedDate to "YYYY-MM-DD" introduces the literal "Invalid Date" on empty input and causes a reparse in the hook. Pass the raw value (or null) to reduce parsing and keep semantics explicit.
- const date = dayjs(selectedDate).format("YYYY-MM-DD"); + const date = selectedDate ?? null;Also applies to: 142-147
219-219: Revisit exhaustive-deps disableIf feasible, make onOverlaySwitchStateChange stable (or include it in deps). Otherwise add a brief comment explaining why re-running on its identity changes would be harmful.
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (3)
236-236: Avoid double Day.js parse; pass selectedDate/null into usePrefetchFormatting then re-parsing is unnecessary and may yield the "Invalid Date" string. Pass the raw value (or null) and let the hook handle validity.
- const date = dayjs(selectedDate).format("YYYY-MM-DD"); + const date = selectedDate ?? null; const { prefetchNextMonth, monthCount } = usePrefetch({ date, month, bookerLayout, bookerState, });Also applies to: 238-243
96-101: Remove any + lint disable; type-safely project store valuesAvoid any by projecting via a generic-safe Record and returning BookerStoreValues.
-// eslint-disable-next-line @typescript-eslint/no-explicit-any -const getStateValues = useCallback((state: any): BookerStoreValues => { - return Object.fromEntries( - Object.entries(state).filter(([_, value]) => typeof value !== "function") - ) as BookerStoreValues; -}, []); +const getStateValues = useCallback((state: unknown): BookerStoreValues => { + const entries = Object.entries(state as Record<string, unknown>).filter( + ([, value]) => typeof value !== "function" + ); + return Object.fromEntries(entries) as BookerStoreValues; +}, []);
152-152: Audit exhaustives-deps disablesWhere possible, stabilize callbacks/inputs or include them in deps. If not feasible, add a short rationale to each disable to prevent future regressions.
Also applies to: 158-158, 231-231, 467-467
📜 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 (3)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts(1 hunks)packages/platform/atoms/booker/BookerPlatformWrapper.tsx(5 hunks)packages/platform/atoms/booker/BookerWebWrapper.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/bookings/Booker/components/hooks/usePrefetch.ts
🧰 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.tsxpackages/platform/atoms/booker/BookerPlatformWrapper.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.tsxpackages/platform/atoms/booker/BookerPlatformWrapper.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.tsxpackages/platform/atoms/booker/BookerPlatformWrapper.tsx
🧠 Learnings (2)
📓 Common learnings
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.936Z
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.
📚 Learning: 2025-09-18T13:38:50.936Z
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.936Z
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/booker/BookerWebWrapper.tsxpackages/platform/atoms/booker/BookerPlatformWrapper.tsx
🧬 Code graph analysis (2)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-56)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
packages/features/bookings/Booker/components/hooks/usePrefetch.ts (1)
usePrefetch(16-56)
🔇 Additional comments (2)
packages/platform/atoms/booker/BookerWebWrapper.tsx (1)
24-24: Hook import for prefetch — LGTMGood move centralizing logic in usePrefetch to remove duplication and NaN edge cases.
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
18-18: Hook import for prefetch — LGTMConsistent with Web wrapper and removes duplicated month/prefetch logic.
3 participants
Uh oh!
There was an error while loading. Please reload this page.