Feat/chunk availability#7
Conversation
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
PR SummaryWhat Changed
Key Changes by Area
Files Changed
Review Focus Areas
Architecture
|
![codity-chait[bot]](https://avatars.githubusercontent.com/u/228176587?s=60&v=4){kind=small}
| ...routingParams, | ||
| }); | ||
| const { roundRobinChunkInfo, handleLoadNextRoundRobinChunk } = useRoundRobinChunking({ | ||
| roundRobinChunkInfo: schedule.data?.roundRobinChunkInfo, |
There was a problem hiding this comment.
This newly added property access relies on schedule.data having roundRobinChunkInfo, but useAvailableSlots is typed with GetAvailableSlotsResponse from packages/platform/atoms/booker/types.ts, and that type only declares slots. The result is a TypeScript contract break (Property 'roundRobinChunkInfo' does not exist on type 'GetAvailableSlotsResponse') that blocks strict type-check/build for the platform atom path.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/atoms/booker/BookerPlatformWrapper.tsx
Lines: 362-362
Issue Type: functional-high
Severity: high
Issue Description:
This newly added property access relies on `schedule.data` having `roundRobinChunkInfo`, but `useAvailableSlots` is typed with `GetAvailableSlotsResponse` from `packages/platform/atoms/booker/types.ts`, and that type only declares `slots`. The result is a TypeScript contract break (`Property 'roundRobinChunkInfo' does not exist on type 'GetAvailableSlotsResponse'`) that blocks strict type-check/build for the platform atom path.
Current Code:
roundRobinChunkInfo: schedule.data?.roundRobinChunkInfo,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
|
@codity review |
PR SummaryWhat Changed
Key Changes by Area
Files Changed
Review Focus Areas
ArchitectureDesign Decisions
Scalability & Extensibility
Risks
|
| ...routingParams, | ||
| }); | ||
| const { roundRobinChunkInfo, handleLoadNextRoundRobinChunk } = useRoundRobinChunking({ | ||
| roundRobinChunkInfo: schedule.data?.roundRobinChunkInfo, |
There was a problem hiding this comment.
This new access does not exist on the platform atom response type. packages/platform/atoms/booker/types.ts:134-136 still defines GetAvailableSlotsResponse as only { slots: Record<string, SlotInfo[]> }, and useAvailableSlots is parameterized with that type. As soon as this line is added, the platform package fails type-checking because roundRobinChunkInfo is not a known property on schedule.data.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/atoms/booker/BookerPlatformWrapper.tsx
Lines: 362-362
Issue Type: functional-high
Severity: high
Issue Description:
This new access does not exist on the platform atom response type. `packages/platform/atoms/booker/types.ts:134-136` still defines `GetAvailableSlotsResponse` as only `{ slots: Record<string, SlotInfo[]> }`, and `useAvailableSlots` is parameterized with that type. As soon as this line is added, the platform package fails type-checking because `roundRobinChunkInfo` is not a known property on `schedule.data`.
Current Code:
roundRobinChunkInfo: schedule.data?.roundRobinChunkInfo,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| resetDeps: [ | ||
| props.username, | ||
| props.eventSlug, | ||
| props.entity.orgSlug, | ||
| props.entity.eventTypeId, | ||
| event.data?.id, | ||
| ], |
There was a problem hiding this comment.
Once a user clicks "Load more availability", roundRobinChunkSettings stays in manual mode until this reset list changes. The problem is that the list only keys off event identity, while useScheduleForEvent also sends the manual chunk offset on every date/month/duration/team-member change (packages/features/bookings/Booker/utils/event.ts:125-126). A concrete failure case is: open chunk 3 for one day, then pick a different day: the next availability request still starts at chunk 3, so chunks 0-2 are skipped and the new day shows incomplete availability.
Suggested fix
resetDeps: [
props.username,
props.eventSlug,
props.entity.orgSlug,
props.entity.eventTypeId,
event.data?.id,
selectedDate,
props.month,
props.duration,
dayCount,
props.teamMemberEmail,
],Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/BookerWebWrapper.tsx
Lines: 170-176
Issue Type: functional-medium
Severity: medium
Issue Description:
Once a user clicks "Load more availability", `roundRobinChunkSettings` stays in manual mode until this reset list changes. The problem is that the list only keys off event identity, while `useScheduleForEvent` also sends the manual chunk offset on every date/month/duration/team-member change (`packages/features/bookings/Booker/utils/event.ts:125-126`). A concrete failure case is: open chunk 3 for one day, then pick a different day: the next availability request still starts at chunk 3, so chunks 0-2 are skipped and the new day shows incomplete availability.
Current Code:
resetDeps: [
props.username,
props.eventSlug,
props.entity.orgSlug,
props.entity.eventTypeId,
event.data?.id,
],
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaBackend (Slots):
API Schema:
Frontend (Booker):
State Management:
Files Changed
Review Focus Areas
ArchitectureDesign Decisions:
Scalability & Extensibility:
Risks:
|
| useEffect(() => { | ||
| setRoundRobinChunkSettings(null); | ||
| setRoundRobinChunkInfo(null); | ||
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); |
There was a problem hiding this comment.
The reset useEffect spreads resetDeps directly into the dependency array using [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]. This is explicitly forbidden by React's rules of hooks: the ESLint react-hooks/exhaustive-deps rule rejects dynamic/spread dependency arrays, and the React runtime does not guarantee stable identity for the spread items across renders. More critically, setRoundRobinChunkInfo and setRoundRobinChunkSettings are Zustand setters that are recreated whenever the store is recreated. During mount, both effects run sequentially: the first effect (line 25-27) sets the chunk info, and the reset effect immediately clears it. This causes the store to always be null on the first render, breaking initial chunk display. The reset effect should only run when the actual identity-relevant props (username, eventSlug, etc.) change, not on every render that includes a stable setter.
Suggested fix
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
// Intentionally spreading resetDeps — these are stable primitive values (strings/numbers/null)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, resetDeps);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 29-32
Issue Type: robustness-high
Severity: high
Issue Description:
The reset `useEffect` spreads `resetDeps` directly into the dependency array using `[setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]`. This is explicitly forbidden by React's rules of hooks: the ESLint `react-hooks/exhaustive-deps` rule rejects dynamic/spread dependency arrays, and the React runtime does not guarantee stable identity for the spread items across renders. More critically, `setRoundRobinChunkInfo` and `setRoundRobinChunkSettings` are Zustand setters that are recreated whenever the store is recreated. During mount, both effects run sequentially: the first effect (line 25-27) sets the chunk info, and the reset effect immediately clears it. This causes the store to always be null on the first render, breaking initial chunk display. The reset effect should only run when the actual identity-relevant props (username, eventSlug, etc.) change, not on every render that includes a stable setter.
Current Code:
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| useEffect(() => { | ||
| setRoundRobinChunkInfo(roundRobinChunkInfo ?? null); | ||
| }, [roundRobinChunkInfo, setRoundRobinChunkInfo]); |
There was a problem hiding this comment.
The effect that syncs roundRobinChunkInfo into the store runs on every render where roundRobinChunkInfo changes (including when it becomes undefined or null). Because the reset effect (lines 29-32) runs after this effect on initial mount (both fire on first mount), the store is first set then immediately cleared, resulting in roundRobinChunkInfo always being null for one render cycle. This leads to the 'Load more' button not appearing on the initial load even when there is more data to fetch. The sync effect should guard against the reset-on-mount issue by checking if the value is actually changing from a non-null to null state deliberately.
Suggested fix
useEffect(() => {
if (roundRobinChunkInfo !== undefined) {
setRoundRobinChunkInfo(roundRobinChunkInfo ?? null);
}
}, [roundRobinChunkInfo, setRoundRobinChunkInfo]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 25-27
Issue Type: robustness-medium
Severity: medium
Issue Description:
The effect that syncs `roundRobinChunkInfo` into the store runs on every render where `roundRobinChunkInfo` changes (including when it becomes `undefined` or `null`). Because the reset effect (lines 29-32) runs after this effect on initial mount (both fire on first mount), the store is first set then immediately cleared, resulting in `roundRobinChunkInfo` always being `null` for one render cycle. This leads to the 'Load more' button not appearing on the initial load even when there is more data to fetch. The sync effect should guard against the reset-on-mount issue by checking if the value is actually changing from a non-null to null state deliberately.
Current Code:
useEffect(() => {
setRoundRobinChunkInfo(roundRobinChunkInfo ?? null);
}, [roundRobinChunkInfo, setRoundRobinChunkInfo]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const handleLoadNextRoundRobinChunk = useCallback(() => { | ||
| if (!roundRobinChunkInfo?.hasMoreNonFixedHosts || isFetching) return; | ||
| const currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0; | ||
| setRoundRobinChunkSettings({ | ||
| manual: true, | ||
| chunkOffset: currentOffset + 1, | ||
| }); | ||
| }, [roundRobinChunkInfo, roundRobinChunkSettings, isFetching, setRoundRobinChunkSettings]); |
There was a problem hiding this comment.
handleLoadNextRoundRobinChunk computes the next offset as currentOffset + 1 where currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0. The fallback reads roundRobinChunkInfo.chunkOffset which is the offset of the currently loaded chunk from the server response. On first load (no manual chunking yet), roundRobinChunkSettings is null, so currentOffset = roundRobinChunkInfo.chunkOffset: the offset of the last auto-loaded chunk. The next request will use chunkOffset + 1. However, in sequential auto mode, the server sets chunkOffset to the index where it stopped (where availability was found), not the total number of chunks loaded. So if the server found availability at chunk 0 (offset 0), the user clicking 'Load More' sends chunkOffset: 1: which is the correct next chunk. This seems correct. But consider: if auto-mode processed chunks 0 and 1 (because chunk 0 had no availability) and stopped at chunk 1 with chunkOffset: 1, clicking 'Load More' sends chunkOffset: 2, skipping to chunk 2: also correct. The logic works. However, there is a subtle race: if the user clicks 'Load More' twice quickly before the first fetch completes, isFetching guard at line 35 should block the second click. But isFetching is derived from schedule.isFetching passed as a prop, and the store state update (via setRoundRobinChunkSettings) happens synchronously while isFetching may not yet be true for the next render cycle. This creates a narrow window where double-clicking could increment chunkOffset twice.
Suggested fix
const handleLoadNextRoundRobinChunk = useCallback(() => {
if (!roundRobinChunkInfo?.hasMoreNonFixedHosts || isFetching) return;
// Use functional update pattern with a ref to prevent double-invocation races
const currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0;
setRoundRobinChunkSettings({
manual: true,
chunkOffset: currentOffset + 1,
});
}, [roundRobinChunkInfo, roundRobinChunkSettings, isFetching, setRoundRobinChunkSettings]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 34-41
Issue Type: functional-medium
Severity: medium
Issue Description:
`handleLoadNextRoundRobinChunk` computes the next offset as `currentOffset + 1` where `currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0`. The fallback reads `roundRobinChunkInfo.chunkOffset` which is the offset of the **currently loaded** chunk from the server response. On first load (no manual chunking yet), `roundRobinChunkSettings` is `null`, so `currentOffset = roundRobinChunkInfo.chunkOffset`: the offset of the last auto-loaded chunk. The next request will use `chunkOffset + 1`. However, in sequential auto mode, the server sets `chunkOffset` to the index where it **stopped** (where availability was found), not the total number of chunks loaded. So if the server found availability at chunk 0 (offset 0), the user clicking 'Load More' sends `chunkOffset: 1`: which is the correct next chunk. This seems correct. But consider: if auto-mode processed chunks 0 and 1 (because chunk 0 had no availability) and stopped at chunk 1 with `chunkOffset: 1`, clicking 'Load More' sends `chunkOffset: 2`, skipping to chunk 2: also correct. The logic works. However, there is a subtle race: if the user clicks 'Load More' twice quickly before the first fetch completes, `isFetching` guard at line 35 should block the second click. But `isFetching` is derived from `schedule.isFetching` passed as a prop, and the store state update (via `setRoundRobinChunkSettings`) happens synchronously while `isFetching` may not yet be `true` for the next render cycle. This creates a narrow window where double-clicking could increment `chunkOffset` twice.
Current Code:
const handleLoadNextRoundRobinChunk = useCallback(() => {
if (!roundRobinChunkInfo?.hasMoreNonFixedHosts || isFetching) return;
const currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0;
setRoundRobinChunkSettings({
manual: true,
chunkOffset: currentOffset + 1,
});
}, [roundRobinChunkInfo, roundRobinChunkSettings, isFetching, setRoundRobinChunkSettings]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const showLoadMoreButton = | ||
| roundRobinChunkInfo?.hasMoreNonFixedHosts && onLoadNextRoundRobinChunk; |
There was a problem hiding this comment.
showLoadMoreButton is truthy when roundRobinChunkInfo?.hasMoreNonFixedHosts && onLoadNextRoundRobinChunk: but both renderLoadMoreButton() and the sticky footer (line 303) are gated on showLoadMoreButton. However showLoadMoreButton is used in the scrollable container's className at line 274 with "pb-14". If onLoadNextRoundRobinChunk is defined but hasMoreNonFixedHosts is false, no button is shown and no padding is applied: correct. But the inverse case: if hasMoreNonFixedHosts is true and the callback is undefined (which can happen during SSR or if the parent forgets to pass the callback), the padding is never added but logically the button 'should' appear. More importantly, the Button inside renderLoadMoreButton() re-references onLoadNextRoundRobinChunk directly on line 230 without the nullish guard that showLoadMoreButton provides: if someone calls renderLoadMoreButton() outside this guard, a TypeError: onLoadNextRoundRobinChunk is not a function can occur.
Suggested fix
const showLoadMoreButton =
Boolean(roundRobinChunkInfo?.hasMoreNonFixedHosts && onLoadNextRoundRobinChunk);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 193-194
Issue Type: functional-medium
Severity: medium
Issue Description:
`showLoadMoreButton` is truthy when `roundRobinChunkInfo?.hasMoreNonFixedHosts && onLoadNextRoundRobinChunk`: but both `renderLoadMoreButton()` and the sticky footer (line 303) are gated on `showLoadMoreButton`. However `showLoadMoreButton` is used in the scrollable container's className at line 274 with `"pb-14"`. If `onLoadNextRoundRobinChunk` is defined but `hasMoreNonFixedHosts` is `false`, no button is shown and no padding is applied: correct. But the inverse case: if `hasMoreNonFixedHosts` is `true` and the callback is `undefined` (which can happen during SSR or if the parent forgets to pass the callback), the padding is never added but logically the button 'should' appear. More importantly, the `Button` inside `renderLoadMoreButton()` re-references `onLoadNextRoundRobinChunk` directly on line 230 without the nullish guard that `showLoadMoreButton` provides: if someone calls `renderLoadMoreButton()` outside this guard, a `TypeError: onLoadNextRoundRobinChunk is not a function` can occur.
Current Code:
const showLoadMoreButton =
roundRobinChunkInfo?.hasMoreNonFixedHosts && onLoadNextRoundRobinChunk;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const totalLoadedHosts = | ||
| roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + | ||
| roundRobinChunkInfo.loadedNonFixedHosts; |
There was a problem hiding this comment.
The totalLoadedHosts calculation uses roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + roundRobinChunkInfo.loadedNonFixedHosts. This is only valid when chunkOffset represents the 0-based index of the last loaded chunk. However, in manual chunking mode the server returns the requested chunk's offset directly, which is correct. But in sequential (auto) mode, the offset is the index of the chunk where availability was found: not necessarily the total number of chunks loaded. For example, if chunk 0 had no availability and chunk 1 did, chunkOffset=1 and loadedNonFixedHosts = chunkSize, so totalLoadedHosts = 1*chunkSize + chunkSize = 2*chunkSize, suggesting two chunks were loaded. This matches the auto-sequential behavior. But the progress bar will show an incorrect percentage if chunks have variable sizes (the last chunk may have fewer hosts than chunkSize).
Suggested fix
const totalLoadedHosts =
(roundRobinChunkInfo.chunkOffset + 1) * roundRobinChunkInfo.chunkSize;
// Note: last chunk may be smaller; clamp to totalNonFixedHosts
const clampedTotalLoadedHosts = Math.min(
totalLoadedHosts,
roundRobinChunkInfo.totalNonFixedHosts || roundRobinChunkInfo.totalHosts
);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 199-201
Issue Type: functional-medium
Severity: medium
Issue Description:
The `totalLoadedHosts` calculation uses `roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + roundRobinChunkInfo.loadedNonFixedHosts`. This is only valid when `chunkOffset` represents the 0-based index of the **last loaded** chunk. However, in manual chunking mode the server returns the requested chunk's offset directly, which is correct. But in sequential (auto) mode, the offset is the index of the chunk where availability was found: not necessarily the total number of chunks loaded. For example, if chunk 0 had no availability and chunk 1 did, `chunkOffset=1` and `loadedNonFixedHosts = chunkSize`, so `totalLoadedHosts = 1*chunkSize + chunkSize = 2*chunkSize`, suggesting two chunks were loaded. This matches the auto-sequential behavior. But the progress bar will show an incorrect percentage if chunks have variable sizes (the last chunk may have fewer hosts than `chunkSize`).
Current Code:
const totalLoadedHosts =
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| roundRobinManualChunking: roundRobinChunkSettings?.manual, | ||
| roundRobinChunkOffset: roundRobinChunkSettings?.chunkOffset, |
There was a problem hiding this comment.
roundRobinChunkOffset is passed directly (potentially undefined) to useSchedule at line 126. Inside useSchedule (useSchedule.ts line 125), the condition typeof roundRobinChunkOffset === 'number' is used to spread it into the query input: so undefined is correctly omitted. However, when roundRobinChunkSettings has chunkOffset: 0, the condition roundRobinChunkSettings?.manual on line 125 is true (manual mode enabled), but chunkOffset is 0. The tRPC input spread at useSchedule.ts:124-125 uses ...(roundRobinManualChunking ? { roundRobinManualChunking: true } : {}) which correctly adds the flag, and ...(typeof roundRobinChunkOffset === 'number' ? { roundRobinChunkOffset } : {}): but roundRobinChunkOffset = 0 is falsy-equivalent, and typeof 0 === 'number' is true, so this is fine. The more subtle bug: if roundRobinChunkSettings is non-null but manual is false, then roundRobinManualChunking will be undefined (not passed), but roundRobinChunkOffset WILL be passed if it's a number. This is inconsistent: a non-manual chunk offset without the manual flag will be sent to the server, potentially causing unexpected behaviour.
Suggested fix
roundRobinManualChunking: roundRobinChunkSettings?.manual ?? undefined,
roundRobinChunkOffset: roundRobinChunkSettings?.manual ? roundRobinChunkSettings.chunkOffset : undefined,Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/utils/event.ts
Lines: 125-126
Issue Type: functional-medium
Severity: medium
Issue Description:
`roundRobinChunkOffset` is passed directly (potentially `undefined`) to `useSchedule` at line 126. Inside `useSchedule` (useSchedule.ts line 125), the condition `typeof roundRobinChunkOffset === 'number'` is used to spread it into the query input: so `undefined` is correctly omitted. However, when `roundRobinChunkSettings` has `chunkOffset: 0`, the condition `roundRobinChunkSettings?.manual` on line 125 is `true` (manual mode enabled), but `chunkOffset` is `0`. The tRPC input spread at `useSchedule.ts:124-125` uses `...(roundRobinManualChunking ? { roundRobinManualChunking: true } : {})` which correctly adds the flag, and `...(typeof roundRobinChunkOffset === 'number' ? { roundRobinChunkOffset } : {})`: but `roundRobinChunkOffset = 0` is falsy-equivalent, and `typeof 0 === 'number'` is `true`, so this is fine. The more subtle bug: if `roundRobinChunkSettings` is non-null but `manual` is `false`, then `roundRobinManualChunking` will be `undefined` (not passed), but `roundRobinChunkOffset` WILL be passed if it's a number. This is inconsistent: a non-manual chunk offset without the manual flag will be sent to the server, potentially causing unexpected behaviour.
Current Code:
roundRobinManualChunking: roundRobinChunkSettings?.manual,
roundRobinChunkOffset: roundRobinChunkSettings?.chunkOffset,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
The @Transform for roundRobinManualChunking uses (value ? value.toLowerCase() === 'true' : false). When value is true (boolean, not string: which happens when the value comes from a non-query-string source such as a JSON body), value.toLowerCase() will throw a TypeError: value.toLowerCase is not a function, crashing the request handler. The existing skipContactOwner field on line 130 has the same pattern and is equally vulnerable. Callers from the internal API path may pass a boolean directly. The transform should handle boolean input as well as string input.
Suggested fix
@Transform(({ value }) => {
if (typeof value === "boolean") return value;
return value ? String(value).toLowerCase() === "true" : false;
})
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 230-234
Issue Type: security-medium
Severity: medium
Issue Description:
The `@Transform` for `roundRobinManualChunking` uses `(value ? value.toLowerCase() === 'true' : false)`. When `value` is `true` (boolean, not string: which happens when the value comes from a non-query-string source such as a JSON body), `value.toLowerCase()` will throw a `TypeError: value.toLowerCase is not a function`, crashing the request handler. The existing `skipContactOwner` field on line 130 has the same pattern and is equally vulnerable. Callers from the internal API path may pass a boolean directly. The transform should handle boolean input as well as string input.
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
Same TypeError vulnerability as in slots-2024-04-15: (value ? value.toLowerCase() === 'true' : false) will throw if value is already a boolean. Since this is duplicated from the other input class, the same fix applies.
Suggested fix
@Transform(({ value }) => {
if (typeof value === "boolean") return value;
return value ? String(value).toLowerCase() === "true" : false;
})
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts
Lines: 114-118
Issue Type: security-medium
Severity: medium
Issue Description:
Same `TypeError` vulnerability as in slots-2024-04-15: `(value ? value.toLowerCase() === 'true' : false)` will throw if `value` is already a boolean. Since this is duplicated from the other input class, the same fix applies.
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| roundRobinManualChunking: roundRobinManualChunking ? true : undefined, | ||
| roundRobinChunkOffset, |
There was a problem hiding this comment.
In the useApiV2AvailableSlots call, roundRobinManualChunking is set via roundRobinManualChunking ? true : undefined (line 160), which means if roundRobinManualChunking is explicitly false (passed from the hook consumer), it becomes undefined: so the server never sees roundRobinManualChunking: false. This is fine for the API since omitting == false. However, roundRobinChunkOffset on line 161 is passed directly without a guard. If roundRobinManualChunking is falsy but roundRobinChunkOffset is a number (e.g. 0), the offset is sent without the manual flag, which is semantically incorrect and could cause unexpected server behaviour as noted in the server schema (the schema accepts them independently). The offset should only be sent when manual chunking is enabled.
Suggested fix
roundRobinManualChunking: roundRobinManualChunking ? true : undefined,
roundRobinChunkOffset: roundRobinManualChunking ? roundRobinChunkOffset : undefined,Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/schedules/lib/use-schedule/useSchedule.ts
Lines: 160-161
Issue Type: functional-medium
Severity: medium
Issue Description:
In the `useApiV2AvailableSlots` call, `roundRobinManualChunking` is set via `roundRobinManualChunking ? true : undefined` (line 160), which means if `roundRobinManualChunking` is explicitly `false` (passed from the hook consumer), it becomes `undefined`: so the server never sees `roundRobinManualChunking: false`. This is fine for the API since omitting == false. However, `roundRobinChunkOffset` on line 161 is passed directly without a guard. If `roundRobinManualChunking` is falsy but `roundRobinChunkOffset` is a number (e.g. `0`), the offset is sent without the manual flag, which is semantically incorrect and could cause unexpected server behaviour as noted in the server schema (the schema accepts them independently). The offset should only be sent when manual chunking is enabled.
Current Code:
roundRobinManualChunking: roundRobinManualChunking ? true : undefined,
roundRobinChunkOffset,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 4 low-priority suggestions for code improvement Click to expand nitpicks
|
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaBackend Chunking Logic: Added API & Types: Extended slot query inputs with Frontend State Management: New UI Components: Sticky "Load more availability" button with progress ring in Files Changed
Review Focus Areas
ArchitectureDesign Decisions:
Scalability & Extensibility:
Risks:
|
| useEffect(() => { | ||
| setRoundRobinChunkSettings(null); | ||
| setRoundRobinChunkInfo(null); | ||
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); |
There was a problem hiding this comment.
The reset useEffect spreads resetDeps into the dependency array using the spread operator (...resetDeps). This violates the React hooks rules: dependency arrays must be statically analyzable literals; spreading a dynamic array causes the effect to run on every render (since the identity of the spread values changes each render when an array literal is passed), or alternatively to be entirely ignored by React's linter, making the reset behavior unreliable. In BookerWebWrapper.tsx (line 170), the resetDeps array is created inline as a new array literal on every render, so this effect fires on every render, clearing both roundRobinChunkSettings and roundRobinChunkInfo immediately after they are set: effectively preventing the chunking feature from ever advancing beyond the first chunk. The setRoundRobinChunkInfo and setRoundRobinChunkSettings function references are stable (Zustand setters), but they are included in the deps alongside the spread, which compounds the issue.
Suggested fix
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
// resetDeps intentionally spread — callers must memoize this array
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 29-32
Issue Type: functional-high
Severity: high
Issue Description:
The reset `useEffect` spreads `resetDeps` into the dependency array using the spread operator (`...resetDeps`). This violates the React hooks rules: dependency arrays must be statically analyzable literals; spreading a dynamic array causes the effect to run on every render (since the identity of the spread values changes each render when an array literal is passed), or alternatively to be entirely ignored by React's linter, making the reset behavior unreliable. In `BookerWebWrapper.tsx` (line 170), the `resetDeps` array is created inline as a new array literal on every render, so this effect fires on every render, clearing both `roundRobinChunkSettings` and `roundRobinChunkInfo` immediately after they are set: effectively preventing the chunking feature from ever advancing beyond the first chunk. The `setRoundRobinChunkInfo` and `setRoundRobinChunkSettings` function references are stable (Zustand setters), but they are included in the deps alongside the spread, which compounds the issue.
Current Code:
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| useEffect(() => { | ||
| setRoundRobinChunkInfo(roundRobinChunkInfo ?? null); | ||
| }, [roundRobinChunkInfo, setRoundRobinChunkInfo]); |
There was a problem hiding this comment.
The first useEffect writes roundRobinChunkInfo to the store unconditionally on every change. The second useEffect (the reset effect) also calls setRoundRobinChunkInfo(null). Because both effects run on the same render cycle when resetDeps changes, the order of execution between effects 1 and 2 is deterministic (effect 1 runs before effect 2 in declaration order), but effect 1 runs again after effect 2 resets to null: if roundRobinChunkInfo prop is still non-null from the previous schedule data, it will re-populate the store with stale chunk info from the old event/user. This creates a race where a user navigates to a new event type: the reset effect clears chunk info, but the sync effect immediately restores the stale chunk info from the previous schedule query (which hasn't been garbage-collected yet by react-query).
Suggested fix
const isResettingRef = useRef(false);
useEffect(() => {
if (!isResettingRef.current) {
setRoundRobinChunkInfo(roundRobinChunkInfo ?? null);
}
isResettingRef.current = false;
}, [roundRobinChunkInfo, setRoundRobinChunkInfo]);
useEffect(() => {
isResettingRef.current = true;
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 25-27
Issue Type: functional-medium
Severity: medium
Issue Description:
The first `useEffect` writes `roundRobinChunkInfo` to the store unconditionally on every change. The second `useEffect` (the reset effect) also calls `setRoundRobinChunkInfo(null)`. Because both effects run on the same render cycle when `resetDeps` changes, the order of execution between effects 1 and 2 is deterministic (effect 1 runs before effect 2 in declaration order), but effect 1 runs again after effect 2 resets to null: if `roundRobinChunkInfo` prop is still non-null from the previous schedule data, it will re-populate the store with stale chunk info from the old event/user. This creates a race where a user navigates to a new event type: the reset effect clears chunk info, but the sync effect immediately restores the stale chunk info from the previous schedule query (which hasn't been garbage-collected yet by react-query).
Current Code:
useEffect(() => {
setRoundRobinChunkInfo(roundRobinChunkInfo ?? null);
}, [roundRobinChunkInfo, setRoundRobinChunkInfo]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const totalLoadedHosts = | ||
| roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + | ||
| roundRobinChunkInfo.loadedNonFixedHosts; |
There was a problem hiding this comment.
The progress calculation for the "Load more" button computes totalLoadedHosts using chunkOffset * chunkSize + loadedNonFixedHosts. This formula is incorrect: chunkOffset is a zero-based index of the current chunk, not the count of fully processed previous chunks. When the user is on chunk 0 (the first chunk), chunkOffset * chunkSize = 0, so totalLoadedHosts = loadedNonFixedHosts which is correct. But on chunk 1, totalLoadedHosts = 1 * chunkSize + loadedNonFixedHosts = chunkSize + chunkSize, double-counting. The correct formula to get the cumulative loaded count should be (chunkOffset + 1) * chunkSize capped at totalNonFixedHosts, or more simply (chunkOffset * chunkSize) + loadedNonFixedHosts: BUT only if loadedNonFixedHosts represents the count for the current chunk alone (which it does per the RoundRobinChunkInfo type). That part is actually correct. However, looking at the auto-chunking logic in util.ts: when hasMoreNonFixedHosts = true the user is shown the button, and chunkOffset is the index of the last loaded chunk. The calculation is in fact (chunkOffset * chunkSize) + loadedNonFixedHosts. For chunk 0 with 21 loaded: 0*50+21=21. For chunk 1 with 21 loaded: 1*50+21=71. This actually appears to be double-counting (50+21=71 when only 71 total were loaded, which happens to be numerically right but accidentally). The real bug is when loadedNonFixedHosts < chunkSize (last chunk): e.g., total=105, chunkSize=21, chunkOffset=4, loadedNonFixedHosts=21: 4*21+21=105 ✓. But on intermediate chunks, a user could receive progressPercentage > 100 if roundRobinChunkInfo becomes stale and totalHosts is 0, which is guarded by the totalHosts > 0 check. The Math.min(100, ...) caps it. The bigger concern is that the formula conflates chunkOffset (0-based index of the most recently fetched chunk) with the number of fully-fetched previous chunks. If in manual mode chunkOffset = 3 but chunks 0-2 were never fetched, the progress would report 3*chunkSize + loadedNonFixedHosts as if all 3 prior chunks were processed.
Suggested fix
const totalLoadedHosts = Math.min(
(roundRobinChunkInfo.chunkOffset + 1) * roundRobinChunkInfo.chunkSize,
roundRobinChunkInfo.totalNonFixedHosts || roundRobinChunkInfo.totalHosts || 0
);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 199-201
Issue Type: functional-medium
Severity: medium
Issue Description:
The progress calculation for the "Load more" button computes `totalLoadedHosts` using `chunkOffset * chunkSize + loadedNonFixedHosts`. This formula is incorrect: `chunkOffset` is a zero-based index of the current chunk, not the count of fully processed previous chunks. When the user is on chunk 0 (the first chunk), `chunkOffset * chunkSize = 0`, so `totalLoadedHosts = loadedNonFixedHosts` which is correct. But on chunk 1, `totalLoadedHosts = 1 * chunkSize + loadedNonFixedHosts = chunkSize + chunkSize`, double-counting. The correct formula to get the cumulative loaded count should be `(chunkOffset + 1) * chunkSize` capped at `totalNonFixedHosts`, or more simply `(chunkOffset * chunkSize) + loadedNonFixedHosts`: BUT only if `loadedNonFixedHosts` represents the count for the current chunk alone (which it does per the `RoundRobinChunkInfo` type). That part is actually correct. However, looking at the auto-chunking logic in `util.ts`: when `hasMoreNonFixedHosts = true` the user is shown the button, and `chunkOffset` is the index of the last loaded chunk. The calculation is in fact `(chunkOffset * chunkSize) + loadedNonFixedHosts`. For chunk 0 with 21 loaded: `0*50+21=21`. For chunk 1 with 21 loaded: `1*50+21=71`. This actually appears to be double-counting (50+21=71 when only 71 total were loaded, which happens to be numerically right but accidentally). The real bug is when `loadedNonFixedHosts < chunkSize` (last chunk): e.g., total=105, chunkSize=21, chunkOffset=4, loadedNonFixedHosts=21: `4*21+21=105` ✓. But on intermediate chunks, a user could receive `progressPercentage > 100` if `roundRobinChunkInfo` becomes stale and `totalHosts` is 0, which is guarded by the `totalHosts > 0` check. The `Math.min(100, ...)` caps it. The bigger concern is that the formula conflates `chunkOffset` (0-based index of the most recently fetched chunk) with the number of fully-fetched previous chunks. If in manual mode `chunkOffset = 3` but chunks 0-2 were never fetched, the progress would report `3*chunkSize + loadedNonFixedHosts` as if all 3 prior chunks were processed.
Current Code:
const totalLoadedHosts =
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| roundRobinManualChunking: roundRobinChunkSettings?.manual, | ||
| roundRobinChunkOffset: roundRobinChunkSettings?.chunkOffset, |
There was a problem hiding this comment.
roundRobinManualChunking is passed as roundRobinChunkSettings?.manual which can be false (when settings are set with manual: false). In useSchedule.ts lines 124-125, when roundRobinManualChunking is falsy, the parameter is omitted from the input object: ...(roundRobinManualChunking ? { roundRobinManualChunking: true } : {}). This means a manual: false setting is silently dropped, which is the intended behavior. However, roundRobinChunkOffset is passed directly as roundRobinChunkSettings?.chunkOffset and in useSchedule.ts it's only conditionally spread: ...(typeof roundRobinChunkOffset === 'number' ? { roundRobinChunkOffset } : {}). When roundRobinChunkSettings is null (default), chunkOffset is undefined, so offset is correctly omitted. But if roundRobinChunkSettings has manual: false but a non-zero chunkOffset, the offset IS sent to the server without roundRobinManualChunking: true. The server would then receive an offset without the manual flag, causing it to use auto-chunking from offset N rather than from 0, potentially returning incorrect slot data.
Suggested fix
roundRobinManualChunking: roundRobinChunkSettings?.manual || undefined,
roundRobinChunkOffset: roundRobinChunkSettings?.manual ? roundRobinChunkSettings.chunkOffset : undefined,Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/utils/event.ts
Lines: 125-126
Issue Type: functional-medium
Severity: medium
Issue Description:
`roundRobinManualChunking` is passed as `roundRobinChunkSettings?.manual` which can be `false` (when settings are set with `manual: false`). In `useSchedule.ts` lines 124-125, when `roundRobinManualChunking` is falsy, the parameter is omitted from the input object: `...(roundRobinManualChunking ? { roundRobinManualChunking: true } : {})`. This means a `manual: false` setting is silently dropped, which is the intended behavior. However, `roundRobinChunkOffset` is passed directly as `roundRobinChunkSettings?.chunkOffset` and in `useSchedule.ts` it's only conditionally spread: `...(typeof roundRobinChunkOffset === 'number' ? { roundRobinChunkOffset } : {})`. When `roundRobinChunkSettings` is `null` (default), `chunkOffset` is `undefined`, so offset is correctly omitted. But if `roundRobinChunkSettings` has `manual: false` but a non-zero `chunkOffset`, the offset IS sent to the server without `roundRobinManualChunking: true`. The server would then receive an offset without the manual flag, causing it to use auto-chunking from offset N rather than from 0, potentially returning incorrect slot data.
Current Code:
roundRobinManualChunking: roundRobinChunkSettings?.manual,
roundRobinChunkOffset: roundRobinChunkSettings?.chunkOffset,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
The @Transform for roundRobinManualChunking uses (value ? value.toLowerCase() === 'true' : false). When value is already a boolean true (passed programmatically, not from a query string), calling value.toLowerCase() on a boolean will throw a TypeError: value.toLowerCase is not a function. The same pattern exists in slots-2024-09-04/inputs/get-slots.input.ts line 114. The existing isTeamEvent field on line 197 of the same file has the same pattern but has been shipping without issue: however, for a new field this is a latent bug that would surface if the API is called programmatically with a boolean value.
Also reported at: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts L114–L118
Suggested fix
@Transform(({ value }) => {
if (typeof value === "boolean") return value;
if (value === undefined || value === null) return false;
return String(value).toLowerCase() === "true";
})
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 230-234
Issue Type: robustness-medium
Severity: medium
Issue Description:
The `@Transform` for `roundRobinManualChunking` uses `(value ? value.toLowerCase() === 'true' : false)`. When `value` is already a boolean `true` (passed programmatically, not from a query string), calling `value.toLowerCase()` on a boolean will throw a `TypeError: value.toLowerCase is not a function`. The same pattern exists in `slots-2024-09-04/inputs/get-slots.input.ts` line 114. The existing `isTeamEvent` field on line 197 of the same file has the same pattern but has been shipping without issue: however, for a new field this is a latent bug that would surface if the API is called programmatically with a boolean value.
_Also reported at: `packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts` L114–L118_
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaBackend/Availability Engine: Added Frontend/Booker UI: New API/Types: Extended input schemas in tRPC ( Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Dynamic chunk sizing (20% of hosts, max 50) balances latency vs. request count for large teams. Early termination when availability is found avoids unnecessary work. Fixed hosts are always included in each chunk to preserve collective availability constraints. Manual chunking mode enables progressive UI loading separate from automatic backend optimization. Risks:
|
| useEffect(() => { | ||
| setRoundRobinChunkSettings(null); | ||
| setRoundRobinChunkInfo(null); | ||
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); |
There was a problem hiding this comment.
Spread syntax with dynamic arrays in useEffect dependency array is invalid React Hook usage. The eslint-plugin-react-hooks rule explicitly forbids spreading an array into a dependency array ([setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]). React requires dependency arrays to be static (fixed-length literal arrays) so it can run its static analysis. At runtime the spread still works as plain JavaScript, but since resetDeps can change length between renders (e.g., if the caller passes a different-length array), the effect will behave inconsistently and React will emit a lint error / may produce stale-closure bugs. The effect is supposed to reset chunk state whenever the event identity changes, but because of this dynamic spread the dependency list is not stable across renders. The correct pattern is to accept a stable primitive hash or use a dedicated useEffect in the caller.
Suggested fix
// Derive a stable string key from the reset dependencies so the
// hook rule is satisfied and length changes can't silently break things.
// eslint-disable-next-line react-hooks/exhaustive-deps
const resetKey = resetDeps.map(String).join("|");
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [resetKey, setRoundRobinChunkInfo, setRoundRobinChunkSettings]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 29-32
Issue Type: functional-high
Severity: high
Issue Description:
Spread syntax with dynamic arrays in `useEffect` dependency array is invalid React Hook usage. The eslint-plugin-react-hooks rule explicitly forbids spreading an array into a dependency array (`[setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]`). React requires dependency arrays to be static (fixed-length literal arrays) so it can run its static analysis. At runtime the spread still works as plain JavaScript, but since `resetDeps` can change length between renders (e.g., if the caller passes a different-length array), the effect will behave inconsistently and React will emit a lint error / may produce stale-closure bugs. The effect is supposed to reset chunk state whenever the event identity changes, but because of this dynamic spread the dependency list is not stable across renders. The correct pattern is to accept a stable primitive hash or use a dedicated `useEffect` in the caller.
Current Code:
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const totalLoadedHosts = | ||
| roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + | ||
| roundRobinChunkInfo.loadedNonFixedHosts; |
There was a problem hiding this comment.
Progress percentage calculation is incorrect. chunkOffset * chunkSize computes the number of non-fixed hosts in previously completed chunks. Adding loadedNonFixedHosts (the count in the current chunk) would double-count the current chunk because chunkOffset already counts the current chunk index (0-based). For example, at offset=1 with chunkSize=21 and loadedNonFixedHosts=21, the formula gives 21*21+21=462 out of 105 total → 100%, but only 2 chunks of 5 have been loaded, so the true progress is ~40%.
The correct formula is: totalLoadedHosts = (chunkOffset) * chunkSize + loadedNonFixedHosts: but chunkOffset is 0-based, so it already equals the number of completed prior chunks. The current code at line 200 does exactly this (chunkOffset * chunkSize + loadedNonFixedHosts). The bug is subtle: when the server returns chunkOffset=1 it means the first index at which data was returned, i.e., 1 prior chunk has been fully processed: so the formula is actually correct for sequential mode but may give >100% in manual mode when chunkOffset jumps ahead. The real issue is the absence of a Math.min guard for the numerator.
Suggested fix
const totalLoadedHosts = Math.min(
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts,
roundRobinChunkInfo.totalNonFixedHosts || roundRobinChunkInfo.totalHosts || 0
);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 199-201
Issue Type: functional-medium
Severity: medium
Issue Description:
Progress percentage calculation is incorrect. `chunkOffset * chunkSize` computes the number of non-fixed hosts in *previously completed* chunks. Adding `loadedNonFixedHosts` (the count in the *current* chunk) would double-count the current chunk because `chunkOffset` already counts the current chunk index (0-based). For example, at offset=1 with chunkSize=21 and loadedNonFixedHosts=21, the formula gives 21*21+21=462 out of 105 total → 100%, but only 2 chunks of 5 have been loaded, so the true progress is ~40%.
The correct formula is: `totalLoadedHosts = (chunkOffset) * chunkSize + loadedNonFixedHosts`: but `chunkOffset` is 0-based, so it already equals the number of *completed prior* chunks. The current code at line 200 does exactly this (`chunkOffset * chunkSize + loadedNonFixedHosts`). The bug is subtle: when the server returns `chunkOffset=1` it means the *first index at which data was returned*, i.e., 1 prior chunk has been fully processed: so the formula is actually correct for sequential mode but may give >100% in manual mode when `chunkOffset` jumps ahead. The real issue is the absence of a `Math.min` guard for the numerator.
Current Code:
const totalLoadedHosts =
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| cx="10" | ||
| cy="10" | ||
| r="8" | ||
| stroke="#FFFFFF" |
There was a problem hiding this comment.
Hard-coded color value #FFFFFF is used for the SVG progress ring stroke. This is a presentational concern but it means the button icon will be invisible on light-themed or high-contrast mode backgrounds. More importantly, using hard-coded hex literals rather than CSS custom-property tokens bypasses the application's theming contract and accessibility guarantees (prefers-contrast: more). The surrounding circle already uses a CSS class text-emphasis, suggesting the intent was to use the design token; the progress arc should follow suit.
| stroke="#FFFFFF" | |
| stroke="currentColor" |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 215-215
Issue Type: security-medium
Severity: medium
Issue Description:
Hard-coded color value `#FFFFFF` is used for the SVG progress ring stroke. This is a presentational concern but it means the button icon will be invisible on light-themed or high-contrast mode backgrounds. More importantly, using hard-coded hex literals rather than CSS custom-property tokens bypasses the application's theming contract and accessibility guarantees (`prefers-contrast: more`). The surrounding circle already uses a CSS class `text-emphasis`, suggesting the intent was to use the design token; the progress arc should follow suit.
Current Code:
stroke="#FFFFFF"
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const renderLoadMoreButton = () => { | ||
| if (!showLoadMoreButton) return null; | ||
|
|
||
| const totalLoadedHosts = | ||
| roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + | ||
| roundRobinChunkInfo.loadedNonFixedHosts; | ||
| const totalHosts = roundRobinChunkInfo.totalNonFixedHosts || roundRobinChunkInfo.totalHosts || 0; | ||
| const progressPercentage = | ||
| totalHosts > 0 ? Math.min(100, Math.round((totalLoadedHosts / totalHosts) * 100)) : 0; | ||
| const circumference = 2 * Math.PI * 8; | ||
| const dashArray = (progressPercentage / 100) * circumference; | ||
|
|
||
| const ProgressIcon = ( | ||
| <svg width="20" height="20" viewBox="0 0 20 20" className="text-emphasis" aria-hidden> | ||
| <circle cx="10" cy="10" r="8" stroke="rgba(255,255,255,0.35)" strokeWidth="2" fill="none" /> | ||
| <circle | ||
| cx="10" | ||
| cy="10" | ||
| r="8" | ||
| stroke="#FFFFFF" | ||
| strokeWidth="2" | ||
| fill="none" | ||
| strokeDasharray={`${dashArray} ${circumference}`} | ||
| strokeLinecap="round" | ||
| transform="rotate(-90 10 10)" | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| return ( | ||
| <Button | ||
| type="button" | ||
| color="primary" | ||
| className="w-full justify-center gap-2" | ||
| onClick={onLoadNextRoundRobinChunk} | ||
| disabled={isLoading || schedule?.isFetching} | ||
| CustomStartIcon={ProgressIcon} | ||
| aria-label={t("round_robin_load_next_hosts")}> | ||
| {t("round_robin_load_next_hosts")} | ||
| </Button> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
renderLoadMoreButton is defined as a plain function inside the component body and re-creates the entire ProgressIcon JSX subtree (including circumference, dashArray calculations and two SVG circle nodes) on every render, regardless of whether the chunk info has changed. Since showLoadMoreButton already guards the container, the function should at minimum be wrapped in useMemo to avoid re-computing the SVG on every parent re-render. The recalculation is cheap individually but happens on every slot-hover or form-field interaction.
Suggested fix
const loadMoreButton = useMemo(() => {
if (!showLoadMoreButton) return null;
const totalLoadedHosts =
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 196-237
Issue Type: performance-medium
Severity: medium
Issue Description:
`renderLoadMoreButton` is defined as a plain function inside the component body and re-creates the entire `ProgressIcon` JSX subtree (including `circumference`, `dashArray` calculations and two SVG circle nodes) on every render, regardless of whether the chunk info has changed. Since `showLoadMoreButton` already guards the container, the function should at minimum be wrapped in `useMemo` to avoid re-computing the SVG on every parent re-render. The recalculation is cheap individually but happens on every slot-hover or form-field interaction.
Current Code:
const renderLoadMoreButton = () => {
if (!showLoadMoreButton) return null;
const totalLoadedHosts =
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| skipContactOwner: z.boolean().nullish(), | ||
| rrHostSubsetIds: z.array(z.number()).nullish(), | ||
| roundRobinManualChunking: z.boolean().optional(), | ||
| roundRobinChunkOffset: z.coerce.number().int().min(0).optional(), |
There was a problem hiding this comment.
roundRobinChunkOffset is validated with .min(0) on the server schema, but on the client side in useRoundRobinChunking.ts handleLoadNextRoundRobinChunk uses Math.max(0, ...) only when computing the manual offset in calculateAvailabilityWithRoundRobinChunks. At the hook level the offset is currentOffset + 1 without any upper-bound guard. If a client sends a chunkOffset larger than the total number of chunks, the server's safeChunkOffset = Math.min(manualChunkOffset, chunkCount - 1) clamps it: but only if chunkCount > 0. If hostChunks is somehow empty (e.g., all hosts are fixed), chunkCount is set to 1 via hostChunks.length || 1, but hostChunks[safeChunkOffset] could still be undefined for offsets ≥ 1, returning hostsForChunk = [...[], ...fixedHosts] which silently falls back to fixed-hosts-only availability: producing incorrect results without any error signal to the UI.
| roundRobinChunkOffset: z.coerce.number().int().min(0).optional(), | |
| roundRobinChunkOffset: z.coerce.number().int().min(0).max(10000).optional(), |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/trpc/server/routers/viewer/slots/types.ts
Lines: 38-38
Issue Type: robustness-medium
Severity: medium
Issue Description:
`roundRobinChunkOffset` is validated with `.min(0)` on the server schema, but on the client side in `useRoundRobinChunking.ts` `handleLoadNextRoundRobinChunk` uses `Math.max(0, ...)` only when computing the manual offset in `calculateAvailabilityWithRoundRobinChunks`. At the hook level the offset is `currentOffset + 1` without any upper-bound guard. If a client sends a `chunkOffset` larger than the total number of chunks, the server's `safeChunkOffset = Math.min(manualChunkOffset, chunkCount - 1)` clamps it: but only if `chunkCount > 0`. If `hostChunks` is somehow empty (e.g., all hosts are fixed), `chunkCount` is set to 1 via `hostChunks.length || 1`, but `hostChunks[safeChunkOffset]` could still be `undefined` for offsets ≥ 1, returning `hostsForChunk = [...[], ...fixedHosts]` which silently falls back to fixed-hosts-only availability: producing incorrect results without any error signal to the UI.
Current Code:
roundRobinChunkOffset: z.coerce.number().int().min(0).optional(),
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
Same @Transform issue as in slots-2024-04-15/inputs/index.ts: roundRobinManualChunking returns false for absent/null values instead of undefined, causing the field to be treated as explicitly set to false by downstream validators and handlers that distinguish between an omitted parameter and an explicitly false one. This is a copy-paste of the same defect.
Suggested fix
@Transform(({ value }) => {
if (value === undefined || value === null || value === "") return undefined;
if (typeof value === "boolean") return value;
return value.toLowerCase() === "true";
})
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts
Lines: 114-118
Issue Type: functional-medium
Severity: medium
Issue Description:
Same `@Transform` issue as in `slots-2024-04-15/inputs/index.ts`: `roundRobinManualChunking` returns `false` for absent/null values instead of `undefined`, causing the field to be treated as explicitly set to `false` by downstream validators and handlers that distinguish between an omitted parameter and an explicitly false one. This is a copy-paste of the same defect.
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 3 low-priority suggestions for code improvement Click to expand nitpicks
|
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaBackend/Availability: Added Frontend/UI: Added API/Types: Extended slot query inputs with Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Chunking is disabled when RR weights are enabled because weights require full host evaluation. Fixed hosts are always included in every chunk. The 20%/50-host chunk sizing and 100-host threshold are tuned for typical query latency vs. coverage tradeoffs. Scalability & Extensibility: Manual chunking mode allows platform consumers to implement custom loading patterns. The Risks:
|
| const showLoadMoreButton = | ||
| roundRobinChunkInfo?.hasMoreNonFixedHosts && onLoadNextRoundRobinChunk; | ||
|
|
||
| const renderLoadMoreButton = () => { | ||
| if (!showLoadMoreButton) return null; | ||
|
|
||
| const totalLoadedHosts = | ||
| roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize + | ||
| roundRobinChunkInfo.loadedNonFixedHosts; |
There was a problem hiding this comment.
totalLoadedHosts computation can overflow the actual total. When roundRobinChunkInfo.chunkOffset is 3 and chunkSize is 21, chunkOffset * chunkSize = 63 already larger than the total number of non-fixed hosts that were actually loaded. The issue: chunkOffset is the 0-based index of the current chunk, so previously loaded non-fixed hosts are chunkOffset * chunkSize, but the very first chunk is offset 0, meaning hosts loaded before the current chunk = chunkOffset * chunkSize, plus loadedNonFixedHosts in the current chunk. That formula looks correct: but only when loadedNonFixedHosts refers to hosts in the current chunk alone. Looking at the server code in util.ts, loadedNonFixedHosts: hostChunk.length is indeed per-chunk. So the formula actually skips all intermediate chunks' host counts and is likely to produce a progressPercentage that jumps non-linearly and can exceed 100 before Math.min. While Math.min(100, ...) clamps the display, the underlying counter logic is semantically wrong for non-manual (sequential) chunking after a second automatic batch fires.
Suggested fix
// chunkOffset is 0-based; hosts loaded up to and including the current chunk:
const totalLoadedHosts = Math.min(
(roundRobinChunkInfo.chunkOffset + 1) * roundRobinChunkInfo.chunkSize,
roundRobinChunkInfo.totalNonFixedHosts || roundRobinChunkInfo.totalHosts || 0
);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: apps/web/modules/bookings/components/AvailableTimeSlots.tsx
Lines: 193-201
Issue Type: functional-medium
Severity: medium
Issue Description:
`totalLoadedHosts` computation can overflow the actual total. When `roundRobinChunkInfo.chunkOffset` is 3 and `chunkSize` is 21, `chunkOffset * chunkSize` = 63 already larger than the total number of non-fixed hosts that were actually loaded. The issue: `chunkOffset` is the 0-based index of the *current* chunk, so previously loaded non-fixed hosts are `chunkOffset * chunkSize`, but the very first chunk is offset 0, meaning hosts loaded before the current chunk = `chunkOffset * chunkSize`, plus `loadedNonFixedHosts` in the current chunk. That formula looks correct: but only when `loadedNonFixedHosts` refers to hosts in the *current* chunk alone. Looking at the server code in `util.ts`, `loadedNonFixedHosts: hostChunk.length` is indeed per-chunk. So the formula actually skips all intermediate chunks' host counts and is likely to produce a `progressPercentage` that jumps non-linearly and can exceed 100 before `Math.min`. While `Math.min(100, ...)` clamps the display, the underlying counter logic is semantically wrong for non-manual (sequential) chunking after a second automatic batch fires.
Current Code:
const totalLoadedHosts =
roundRobinChunkInfo.chunkOffset * roundRobinChunkInfo.chunkSize +
roundRobinChunkInfo.loadedNonFixedHosts;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
The @Transform for roundRobinManualChunking uses (value ? value.toLowerCase() === 'true' : false). When value is the boolean true (already transformed by class-transformer on a JSON body), calling .toLowerCase() on it will throw TypeError: value.toLowerCase is not a function, crashing the request pipeline before validation runs. The existing pattern on other boolean fields in this file uses value && value.toLowerCase() === 'true' which short-circuits on falsy (but that fails the opposite way when value is already false). The correct defensive pattern is to handle both the already-boolean case and the string case.
Also reported at: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts L114–L118
Suggested fix
@Transform(({ value }) => {
if (typeof value === "boolean") return value;
if (typeof value === "string") return value.toLowerCase() === "true";
return false;
})
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 230-234
Issue Type: security-medium
Severity: medium
Issue Description:
The `@Transform` for `roundRobinManualChunking` uses `(value ? value.toLowerCase() === 'true' : false)`. When `value` is the boolean `true` (already transformed by class-transformer on a JSON body), calling `.toLowerCase()` on it will throw `TypeError: value.toLowerCase is not a function`, crashing the request pipeline before validation runs. The existing pattern on other boolean fields in this file uses `value && value.toLowerCase() === 'true'` which short-circuits on falsy (but that fails the opposite way when `value` is already `false`). The correct defensive pattern is to handle both the already-boolean case and the string case.
_Also reported at: `packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts` L114–L118_
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 4 low-priority suggestions for code improvement Click to expand nitpicks
|
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAvailability Calculation: Added API/Types: Extended Frontend/UI: "Load more availability" button with progress indicator in Platform: Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Chunking stops at first availability found to minimize calendar API calls. This trades completeness for speed: users may see fewer initial slots but get faster response times. Manual chunking mode allows users to explicitly request more hosts when needed. Risks:
|
Workflow DiagramsAutomatically generated sequence diagrams showing the workflows in this PR 1. Round Robin Chunked Availability Loading FlowComplex complexity • Components: useRoundRobinChunking hook, BookerPlatformWrapper, BookerWebWrapper sequenceDiagram
title: Round Robin Chunked Availability Loading Workflow
actor User
participant AvailableTimeSlots as AvailableTimeSlots Component
participant BookerStore as BookerStore Zustand
participant useRoundRobinChunking as useRoundRobinChunking Hook
participant useAvailableSlots as useAvailableSlots Hook
participant API as Availability API
participant Database as Database
Note over User,Database: Initial Load Flow
User->>BookerStore: Select date/event
BookerStore->>useRoundRobinChunking: Initialize with default chunkOffset 0
useRoundRobinChunking->>BookerStore: Set roundRobinChunkSettings
BookerStore->>useAvailableSlots: Trigger with chunkOffset 0
useAvailableSlots->>API: GET /slots with roundRobinChunkOffset 0
API->>Database: Query host availability (first chunk)
Database-->>API: Return slots + roundRobinChunkInfo
API-->>useAvailableSlots: Return {slots, roundRobinChunkInfo}
useAvailableSlots->>BookerStore: Update schedule data
AvailableTimeSlots->>BookerStore: Read roundRobinChunkInfo
Note right of AvailableTimeSlots: Check hasMoreNonFixedHosts flag
alt hasMoreNonFixedHosts is true
AvailableTimeSlots->>AvailableTimeSlots: Render "Load More" button with progress indicator
Note right of AvailableTimeSlots: Progress percentage calculated from loaded vs total hosts
end
Note over User,Database: User Requests More Availability
User->>AvailableTimeSlots: Click "Load More Availability" button
AvailableTimeSlots->>useRoundRobinChunking: Call handleLoadNextRoundRobinChunk
useRoundRobinChunking->>BookerStore: Update roundRobinChunkSettings.chunkOffset (increment)
BookerStore->>useAvailableSlots: Detect chunkOffset change, trigger refetch
useAvailableSlots->>API: GET /slots with new roundRobinChunkOffset
API->>Database: Query next chunk of hosts availability
Database-->>API: Return additional slots + updated roundRobinChunkInfo
API-->>useAvailableSlots: Merge new slots with existing
useAvailableSlots->>BookerStore: Update schedule data with combined slots
AvailableTimeSlots->>BookerStore: Re-read roundRobinChunkInfo
alt hasMoreNonFixedHosts is now false
AvailableTimeSlots->>AvailableTimeSlots: Hide "Load More" button
Note right of AvailableTimeSlots: All hosts loaded, show complete availability
else hasMoreNonFixedHosts still true
AvailableTimeSlots->>AvailableTimeSlots: Update progress indicator
Note right of AvailableTimeSlots: Show updated percentage of loaded hosts
end
Note over User,Database: Context Change Reset Flow
alt Event, username, orgSlug, or eventTypeId changes
useRoundRobinChunking->>useRoundRobinChunking: Detect reset dependency change
useRoundRobinChunking->>BookerStore: Reset chunkOffset to 0
BookerStore->>useAvailableSlots: Trigger fresh fetch
Note right of useRoundRobinChunking: Previous chunk state discarded
end
Note: Diagrams show detected patterns only. Complex workflows may require manual review. |
![codity-ai[bot]](https://avatars.githubusercontent.com/in/1102452?s=60&v=4){kind=small}
| useEffect(() => { | ||
| setRoundRobinChunkSettings(null); | ||
| setRoundRobinChunkInfo(null); | ||
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); |
There was a problem hiding this comment.
Spreading ...resetDeps into a useEffect dependency array creates a variable-length dep array. React tracks dependency array length between renders and will emit a warning if the count changes. More critically, eslint-plugin-react-hooks/exhaustive-deps cannot statically analyse spread syntax, so the linter silently stops enforcing exhaustiveness for the entire effect: any future accidental omission of a real dep won't be caught. The conventional fix is to flatten the list to a primitive cache-key string so the array length is always 2.
Suggested fix
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, JSON.stringify(resetDeps)]);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 29-32
Issue Type: robustness-medium
Severity: medium
Issue Description:
Spreading `...resetDeps` into a `useEffect` dependency array creates a variable-length dep array. React tracks dependency array length between renders and will emit a warning if the count changes. More critically, `eslint-plugin-react-hooks/exhaustive-deps` cannot statically analyse spread syntax, so the linter silently stops enforcing exhaustiveness for the entire effect: any future accidental omission of a real dep won't be caught. The conventional fix is to flatten the list to a primitive cache-key string so the array length is always 2.
Current Code:
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => { | ||
| if (value === undefined || value === null || value === "") { | ||
| return undefined; | ||
| } | ||
| const parsedValue = typeof value === "string" ? parseInt(value, 10) : value; | ||
| return Number.isNaN(parsedValue) ? undefined : parsedValue; | ||
| }) | ||
| @IsNumber() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinChunkOffset?: number; |
There was a problem hiding this comment.
The roundRobinChunkOffset field has no @Min(0) or @IsInt() constraint in the platform API DTO. A caller can supply a negative integer (e.g., -5) and it will pass class-validator without a 400 error. The tRPC path correctly applies .min(0) in the Zod schema, and the server's Math.max(0, roundRobinChunkOffset) clamps the value at runtime: but the platform API endpoint never rejects the invalid value, silently treating it as 0. @Min and @IsInt are already imported in this file.
Also reported at: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts L120–L130
Suggested fix
@Transform(({ value }) => {
if (value === undefined || value === null || value === "") {
return undefined;
}
const parsedValue = typeof value === "string" ? parseInt(value, 10) : value;
return Number.isNaN(parsedValue) ? undefined : parsedValue;
})
@IsNumber()
@IsInt()
@Min(0)
@IsOptional()
@ApiHideProperty()
roundRobinChunkOffset?: number;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 236-246
Issue Type: functional-medium
Severity: medium
Issue Description:
The `roundRobinChunkOffset` field has no `@Min(0)` or `@IsInt()` constraint in the platform API DTO. A caller can supply a negative integer (e.g., `-5`) and it will pass class-validator without a 400 error. The tRPC path correctly applies `.min(0)` in the Zod schema, and the server's `Math.max(0, roundRobinChunkOffset)` clamps the value at runtime: but the platform API endpoint never rejects the invalid value, silently treating it as 0. `@Min` and `@IsInt` are already imported in this file.
_Also reported at: `packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts` L120–L130_
Current Code:
@Transform(({ value }) => {
if (value === undefined || value === null || value === "") {
return undefined;
}
const parsedValue = typeof value === "string" ? parseInt(value, 10) : value;
return Number.isNaN(parsedValue) ? undefined : parsedValue;
})
@IsNumber()
@IsOptional()
@ApiHideProperty()
roundRobinChunkOffset?: number;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 4 low-priority suggestions for code improvement Click to expand nitpicks
|
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAvailability Engine: Added Frontend State: Extended booker store with Platform API: Added UI Components: Added Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Dynamic chunk sizing (20% of hosts, capped at 50) balances query size against total host count. Sequential processing with early termination prioritizes user-perceived speed over backend parallelism. Manual chunking via offset allows frontend-driven "Load more" without server-side session state. Scalability & Extensibility: Chunking is intentionally capped at 50 hosts to prevent oversized queries. The 100-host threshold is hardcoded; making this configurable is out of scope. Weight-based routing bypasses chunking entirely since weights require full host set evaluation. Risks:
|
| useEffect(() => { | ||
| setRoundRobinChunkSettings(null); | ||
| setRoundRobinChunkInfo(null); | ||
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); |
There was a problem hiding this comment.
The reset effect spreads resetDeps directly into the dependency array of useEffect, which violates the React hooks rules. The ESLint react-hooks/exhaustive-deps rule does not understand spread syntax in dependency arrays, and more critically the React runtime treats the spread as a series of positional arguments: if resetDeps length ever changes between renders the hook silently breaks (hooks must be called with the same number of arguments every render). Furthermore, including setRoundRobinChunkInfo and setRoundRobinChunkSettings (stable store functions) together with the actual reset deps means the reset fires on every render where the store reference changes (e.g. HMR, context re-initialization). In practice, the effect can fire immediately after the user clicks 'Load more', erasing the roundRobinChunkSettings they just set and preventing the next chunk from ever loading.
Suggested fix
const resetDepsRef = useRef(resetDeps);
useEffect(() => {
resetDepsRef.current = resetDeps;
});
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, resetDeps); // resetDeps must have a stable length; see useRef pattern above if it can changePrompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 29-32
Issue Type: functional-high
Severity: high
Issue Description:
The reset effect spreads `resetDeps` directly into the dependency array of `useEffect`, which violates the React hooks rules. The ESLint `react-hooks/exhaustive-deps` rule does not understand spread syntax in dependency arrays, and more critically the React runtime treats the spread as a series of positional arguments: if `resetDeps` length ever changes between renders the hook silently breaks (hooks must be called with the same number of arguments every render). Furthermore, including `setRoundRobinChunkInfo` and `setRoundRobinChunkSettings` (stable store functions) together with the actual reset deps means the reset fires on every render where the store reference changes (e.g. HMR, context re-initialization). In practice, the effect can fire immediately after the user clicks 'Load more', erasing the `roundRobinChunkSettings` they just set and preventing the next chunk from ever loading.
Current Code:
useEffect(() => {
setRoundRobinChunkSettings(null);
setRoundRobinChunkInfo(null);
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const handleLoadNextRoundRobinChunk = useCallback(() => { | ||
| if (!roundRobinChunkInfo?.hasMoreNonFixedHosts || isFetching) return; | ||
| const currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0; | ||
| setRoundRobinChunkSettings({ | ||
| manual: true, | ||
| chunkOffset: currentOffset + 1, | ||
| }); |
There was a problem hiding this comment.
The handleLoadNextRoundRobinChunk callback uses roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0 to determine the current offset, but the intent is to advance to the next chunk beyond what the API last returned. When the server responds with chunkOffset: N, the client state (roundRobinChunkSettings) may still hold the previous request offset (N-1) or null (first click). The fallback to roundRobinChunkInfo.chunkOffset is correct for the first click, but on subsequent clicks the stale roundRobinChunkSettings.chunkOffset is used instead of the server-confirmed roundRobinChunkInfo.chunkOffset, causing the offset to be calculated as (N-1) + 1 = N (re-requesting the same chunk) instead of N + 1. The correct base should always be the server-confirmed roundRobinChunkInfo.chunkOffset.
| const handleLoadNextRoundRobinChunk = useCallback(() => { | |
| if (!roundRobinChunkInfo?.hasMoreNonFixedHosts || isFetching) return; | |
| const currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0; | |
| setRoundRobinChunkSettings({ | |
| manual: true, | |
| chunkOffset: currentOffset + 1, | |
| }); | |
| const currentOffset = roundRobinChunkInfo.chunkOffset ?? 0; |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 34-40
Issue Type: functional-medium
Severity: medium
Issue Description:
The `handleLoadNextRoundRobinChunk` callback uses `roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0` to determine the current offset, but the intent is to advance to the *next* chunk beyond what the API last returned. When the server responds with `chunkOffset: N`, the client state (`roundRobinChunkSettings`) may still hold the *previous* request offset (N-1) or null (first click). The fallback to `roundRobinChunkInfo.chunkOffset` is correct for the first click, but on subsequent clicks the stale `roundRobinChunkSettings.chunkOffset` is used instead of the server-confirmed `roundRobinChunkInfo.chunkOffset`, causing the offset to be calculated as `(N-1) + 1 = N` (re-requesting the same chunk) instead of `N + 1`. The correct base should always be the server-confirmed `roundRobinChunkInfo.chunkOffset`.
Current Code:
const currentOffset = roundRobinChunkSettings?.chunkOffset ?? roundRobinChunkInfo.chunkOffset ?? 0;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| return chunkResultWithInfo; | ||
| } | ||
| lastResult = chunkResultWithInfo; | ||
| } | ||
|
|
||
| if (lastResult) { | ||
| return lastResult; | ||
| } | ||
|
|
||
| const fallbackResult = await calculateForHosts(fixedHosts); | ||
| return { | ||
| ...fallbackResult, | ||
| roundRobinChunkInfo: buildChunkInfo({ | ||
| chunkOffset: 0, | ||
| loadedNonFixedHosts: 0, |
There was a problem hiding this comment.
In automatic (non-manual) chunking mode, calculateAvailabilityWithRoundRobinChunks calls calculateHostsAndAvailabilities sequentially for every chunk until it finds one with availability. For a 1000-host team with a 50-host chunk size, this means up to 20 sequential calculateHostsAndAvailabilities calls in the hot availability-check path. Each call fetches calendar data and checks busy times for 50+ hosts. With no timeout or early-exit beyond finding the first chunk with results, a worst-case request (no availability at all) will make 20 sequential heavy DB/calendar calls, potentially timing out or causing severe latency for the user.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/trpc/server/routers/viewer/slots/util.ts
Lines: 1148-1162
Issue Type: performance-medium
Severity: medium
Issue Description:
In automatic (non-manual) chunking mode, `calculateAvailabilityWithRoundRobinChunks` calls `calculateHostsAndAvailabilities` sequentially for every chunk until it finds one with availability. For a 1000-host team with a 50-host chunk size, this means up to 20 sequential `calculateHostsAndAvailabilities` calls in the hot availability-check path. Each call fetches calendar data and checks busy times for 50+ hosts. With no timeout or early-exit beyond finding the first chunk with results, a worst-case request (no availability at all) will make 20 sequential heavy DB/calendar calls, potentially timing out or causing severe latency for the user.
Current Code:
let lastResult: ChunkedAvailabilityResult | null = null;
for (let index = 0; index < hostChunks.length; index += 1) {
const hostChunk = hostChunks[index];
const hostsForChunk = [...hostChunk, ...fixedHosts];
const chunkResult = await calculateForHosts(hostsForChunk);
const chunkInfo = buildChunkInfo({
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @@ -1222,7 +1403,7 @@ export class AvailableSlotsService { | |||
| // if start time is not within first two weeks, check if there are any available slots | |||
| if (!aggregatedAvailability.length) { | |||
| // if no available slots check if first two weeks are available, otherwise fallback | |||
| const firstTwoWeeksAvailabilities = await this.calculateHostsAndAvailabilities({ | |||
| const firstTwoWeeksAvailabilities = await this.calculateAvailabilityWithRoundRobinChunks({ | |||
| input, | |||
| eventType, | |||
| hosts: [...eligibleQualifiedRRHosts, ...eligibleFixedHosts], | |||
| @@ -1233,12 +1414,7 @@ export class AvailableSlotsService { | |||
| silentCalendarFailures, | |||
| mode, | |||
| }); | |||
| if ( | |||
| !getAggregatedAvailability( | |||
| firstTwoWeeksAvailabilities.allUsersAvailability, | |||
| eventType.schedulingType | |||
| ).length | |||
| ) { | |||
| if (!firstTwoWeeksAvailabilities.aggregatedAvailability.length) { | |||
| diff = 1; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
When the fallback path (hasFallbackRRHosts && diff > 0) is taken, the roundRobinChunkInfo returned to the client reflects the fallback host set (eligibleFallbackRRHosts + eligibleFixedHosts), but the totalHosts / totalNonFixedHosts fields inside buildChunkInfo are captured from the original allHosts array via closure from the outer call. This means the chunk metadata reported to the UI (total hosts, hasMoreNonFixedHosts, etc.) is incorrect for the fallback host set, leading to a 'Load more' button that shows wrong progress and may display 'more availability' when all fallback hosts have already been loaded.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/trpc/server/routers/viewer/slots/util.ts
Lines: 1395-1420
Issue Type: functional-medium
Severity: medium
Issue Description:
When the fallback path (`hasFallbackRRHosts && diff > 0`) is taken, the `roundRobinChunkInfo` returned to the client reflects the fallback host set (eligibleFallbackRRHosts + eligibleFixedHosts), but the `totalHosts` / `totalNonFixedHosts` fields inside `buildChunkInfo` are captured from the **original** `allHosts` array via closure from the outer call. This means the chunk metadata reported to the UI (total hosts, hasMoreNonFixedHosts, etc.) is incorrect for the fallback host set, leading to a 'Load more' button that shows wrong progress and may display 'more availability' when all fallback hosts have already been loaded.
Current Code:
({
allUsersAvailability,
usersWithCredentials,
currentSeats,
aggregatedAvailability,
roundRobinChunkInfo,
} = await this.calculateAvailabilityWithRoundRobinChunks({
input,
eventType,
hosts: [...eligibleFallbackRRHosts, ...eligibleFixedHosts],
loggerWithEventDetails,
startTime,
endTime,
bypassBusyCalendarTimes,
silentCalendarFailures,
mode,
}));
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
The @Transform for roundRobinManualChunking uses the expression value ? value.toLowerCase() === 'true' : false. This means that if the query string sends roundRobinManualChunking=false (the string "false"), value is truthy, so value.toLowerCase() === 'true' evaluates to false correctly. However, if the value is already a boolean false (when used programmatically), value.toLowerCase() will throw a TypeError because false.toLowerCase is not a function. The same pattern is used for several existing fields (e.g. skipContactOwner) but for new numeric roundRobinChunkOffset the guard correctly handles the boolean case. This is a latent bug that will surface when the value is passed as a boolean rather than a string query param.
Suggested fix
@Transform(({ value }) => {
if (typeof value === "boolean") return value;
if (value === undefined || value === null) return false;
return String(value).toLowerCase() === "true";
})
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 230-234
Issue Type: functional-medium
Severity: medium
Issue Description:
The `@Transform` for `roundRobinManualChunking` uses the expression `value ? value.toLowerCase() === 'true' : false`. This means that if the query string sends `roundRobinManualChunking=false` (the string `"false"`), `value` is truthy, so `value.toLowerCase() === 'true'` evaluates to `false` correctly. However, if the value is already a boolean `false` (when used programmatically), `value.toLowerCase()` will throw a TypeError because `false.toLowerCase` is not a function. The same pattern is used for several existing fields (e.g. `skipContactOwner`) but for new numeric `roundRobinChunkOffset` the guard correctly handles the boolean case. This is a latent bug that will surface when the value is passed as a boolean rather than a string query param.
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAvailability Engine: Added UI/UX: Added API/Platform: Extended slot query inputs with State Management: Extended Booker store with Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Dynamic chunk sizing (20% of hosts, capped at 50) balances query size against request count. Sequential processing with early termination optimizes for common case where early hosts have availability. Manual chunking mode allows explicit pagination control for UI "load more" pattern. Scalability & Extensibility: Chunking threshold (100 hosts) and sizing constants are hardcoded. Out of scope: making these configurable per-event-type or tenant. Risks: Intentional: Weight-based ordering is preserved by disabling chunking when RR weights are enabled, trading performance for fairness. Unintentional: |
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaBackend/Availability: Added API/Types: Extended slot query inputs across tRPC and platform APIs ( Frontend/Booker: New Platform: Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Chunking activates only when non-fixed hosts exceed 100 and RR weights are disabled. This keeps simple cases fast while solving the large-team performance problem. Manual chunking mode gives frontend control over pagination vs. automatic sequential processing. Scalability & Extensibility: Chunk size scales with team size (20% of hosts) but caps at 50 to prevent oversized queries. The Risks:
|
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAvailability Calculation: Added API/Types: Extended Platform API inputs ( Frontend State: New Platform Integration: Both Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Sequential chunk processing prioritizes early termination when availability is found. This trades worst-case latency (all chunks) for typical-case speed. Manual chunking is exposed to UI rather than auto-loading to prevent excessive backend calls when users are unlikely to book far-future slots. Scalability & Extensibility: Chunk sizing is intentionally capped at 50 hosts to bound single-request latency. The 20% ratio provides reasonable granularity without excessive round trips for 100-500 host teams. Out of scope: adaptive chunk sizing based on historical availability patterns. Risks:
|
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAvailability Engine: New API/Types: Added Frontend/UI: New State Management: Extended booker store with Files Changed
Review Focus Areas
ArchitectureDesign Decisions
Scalability & Extensibility
Risks
|
| useEffect(() => { | ||
| setRoundRobinChunkSettings(null); | ||
| setRoundRobinChunkInfo(null); | ||
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); |
There was a problem hiding this comment.
Spreading resetDeps (a variable-length array) into the dependency array violates React's Rules of Hooks, which require dependency arrays to have a constant number of elements across renders. This causes React to silently skip effect cleanup or retain stale closures when resetDeps length changes, and breaks ESLint's exhaustive-deps rule. The fix is to include resetDeps as a single dependency rather than spreading it.
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]); | |
| }, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, resetDeps]); |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/bookings/Booker/components/hooks/useRoundRobinChunking.ts
Lines: 32-32
Issue Type: functional-critical
Severity: critical
Issue Description:
Spreading `resetDeps` (a variable-length array) into the dependency array violates React's Rules of Hooks, which require dependency arrays to have a constant number of elements across renders. This causes React to silently skip effect cleanup or retain stale closures when `resetDeps` length changes, and breaks ESLint's exhaustive-deps rule. The fix is to include `resetDeps` as a single dependency rather than spreading it.
Current Code:
}, [setRoundRobinChunkInfo, setRoundRobinChunkSettings, ...resetDeps]);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | ||
| @IsBoolean() | ||
| @IsOptional() | ||
| @ApiHideProperty() | ||
| roundRobinManualChunking?: boolean; |
There was a problem hiding this comment.
The @Transform decorator on line 114 calls value.toLowerCase() without type-checking, causing a TypeError when value is a boolean true from JSON input. While the decorator attempts to handle truthy values, it doesn't account for non-string truthy values like native booleans. This crashes the request when the API receives roundRobinManualChunking: true as a JSON boolean instead of a string.
| @Transform(({ value }) => (value ? value.toLowerCase() === "true" : false)) | |
| @IsBoolean() | |
| @IsOptional() | |
| @ApiHideProperty() | |
| roundRobinManualChunking?: boolean; | |
| @Transform(({ value }) => (typeof value === "string" ? value.toLowerCase() === "true" : Boolean(value))) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts
Lines: 114-118
Issue Type: functional-high
Severity: high
Issue Description:
The `@Transform` decorator on line 114 calls `value.toLowerCase()` without type-checking, causing a TypeError when `value` is a boolean `true` from JSON input. While the decorator attempts to handle truthy values, it doesn't account for non-string truthy values like native booleans. This crashes the request when the API receives `roundRobinManualChunking: true` as a JSON boolean instead of a string.
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
There was a problem hiding this comment.
The @Transform decorator calls value.toLowerCase() on any truthy value without checking its type first. When a native boolean true is sent in the JSON request body (not a string), value.toLowerCase is undefined, causing a TypeError that crashes the request handler. The fix must handle both string and boolean inputs by checking the type before calling string methods.
| @Transform(({ value }) => (typeof value === 'string' ? value.toLowerCase() === 'true' : !!value)) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 197-201
Issue Type: functional-critical
Severity: critical
Issue Description:
The `@Transform` decorator calls `value.toLowerCase()` on any truthy value without checking its type first. When a native boolean `true` is sent in the JSON request body (not a string), `value.toLowerCase` is undefined, causing a `TypeError` that crashes the request handler. The fix must handle both string and boolean inputs by checking the type before calling string methods.
Current Code:
@Transform(({ value }) => (value ? value.toLowerCase() === "true" : false))
@IsBoolean()
@IsOptional()
@ApiHideProperty()
roundRobinManualChunking?: boolean;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
There was a problem hiding this comment.
The roundRobinChunkOffset field is validated only with @IsNumber(), allowing negative values and floats to pass validation, whereas the tRPC schema enforces .int().min(0). When a float (e.g., 1.5) or negative value reaches the server, using it as an array index into hostChunks silently returns undefined, causing an empty chunk to be returned instead of rejecting the invalid input. This creates a data validation inconsistency between client and server schemas, leading to unexpected behavior.
Also reported at: packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts L71–L81
Suggested fix
@Transform(({ value }) => {
if (value === undefined || value === null || value === "") {
return undefined;
}
const parsedValue = typeof value === "string" ? parseInt(value, 10) : value;
return Number.isNaN(parsedValue) ? undefined : parsedValue;
})
@IsNumber()
@IsInt()
@Min(0)
@IsOptional()
@ApiHideProperty()
roundRobinChunkOffset?: number;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/platform/types/slots/slots-2024-04-15/inputs/index.ts
Lines: 114-124
Issue Type: functional-high
Severity: high
Issue Description:
The `roundRobinChunkOffset` field is validated only with `@IsNumber()`, allowing negative values and floats to pass validation, whereas the tRPC schema enforces `.int().min(0)`. When a float (e.g., `1.5`) or negative value reaches the server, using it as an array index into `hostChunks` silently returns `undefined`, causing an empty chunk to be returned instead of rejecting the invalid input. This creates a data validation inconsistency between client and server schemas, leading to unexpected behavior.
_Also reported at: `packages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts` L71–L81_
Current Code:
@Transform(({ value }) => {
if (value === undefined || value === null || value === "") {
return undefined;
}
const parsedValue = typeof value === "string" ? parseInt(value, 10) : value;
return Number.isNaN(parsedValue) ? undefined : parsedValue;
})
@IsNumber()
@IsOptional()
@ApiHideProperty()
roundRobinChunkOffset?: number;
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
3 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