fix: normalize busyMap key generation to prevent incorrect interval matching#28936
fix: normalize busyMap key generation to prevent incorrect interval matching#28936Akash504-ai wants to merge 2 commits intocalcom:mainfrom
Conversation
|
Welcome to Cal.diy, @Akash504-ai! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughA new test suite for 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/lib/intervalLimits/limitManager.ts (1)
85-91: ReusecreateKeyto prevent future drift.The key construction here is now identical to
LimitManager.createKey(line 37-40). Inlining it risks reintroducing the same mismatch this PR is fixing if one side is later changed. Prefer calling the existing helper.♻️ Proposed refactor
- const tzStart = params.timeZone ? params.start.tz(params.timeZone) : params.start; - this.busyMap.set(`${params.unit}-${tzStart.startOf(params.unit).toISOString()}`, { + const tzStart = params.timeZone ? params.start.tz(params.timeZone) : params.start; + const key = LimitManager.createKey(params.start, params.unit, params.timeZone); + this.busyMap.set(key, { start: tzStart.toISOString(), end: tzStart.endOf(params.unit).toISOString(), title: params.title, source: params.source, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/intervalLimits/limitManager.ts` around lines 85 - 91, The busyMap key is being constructed inline but duplicates the logic in LimitManager.createKey; replace the inline template string `${params.unit}-${tzStart.startOf(params.unit).toISOString()}` with a call to the existing helper (e.g., this.createKey(...)) so the code uses LimitManager.createKey when calling busyMap.set; keep the same tzStart, params.unit, and other values (start/end/title/source) but pass the key returned by createKey instead of building it inline.packages/lib/__tests__/limitManager.test.ts (1)
7-43: Consider expanding coverage to week/month/year and timezone cases.The current two tests only exercise
unit: "day"without atimeZone. Given that the bug being fixed stems from key normalization acrossstartOf(unit)boundaries, it would be valuable to also assert:
- Same-week / same-month / same-year bucketing (including the week-spans-two-months branch at limitManager.ts line 52-54).
- A case where
timeZoneshifts the calendar day (e.g., a UTC time that falls on a different day inAmerica/Los_Angeles) — this is the primary real-world scenario inpackages/trpc/server/routers/viewer/slots/util.tswheretimeZoneis always passed.Not blocking for this fix, but would guard against regressions in the ancestor-unit fallthrough logic in
isAlreadyBusy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/__tests__/limitManager.test.ts` around lines 7 - 43, Add tests that cover ancestor units and timezone behavior for LimitManager: extend the test suite to include cases calling addBusyTime and isAlreadyBusy with unit values "week", "month", and "year" (exercising the week-spans-two-months branch) and assert same- and different-bucket behavior; also add a test using a non-UTC timeZone (e.g., "America/Los_Angeles") where a UTC timestamp maps to a different local day to verify isAlreadyBusy respects timeZone normalization. Use the same helpers (LimitManager, addBusyTime, isAlreadyBusy) and dayjs timestamps as in existing tests so they clearly target the bug in key normalization across startOf(unit) boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lib/__tests__/limitManager.test.ts`:
- Around line 7-43: Add tests that cover ancestor units and timezone behavior
for LimitManager: extend the test suite to include cases calling addBusyTime and
isAlreadyBusy with unit values "week", "month", and "year" (exercising the
week-spans-two-months branch) and assert same- and different-bucket behavior;
also add a test using a non-UTC timeZone (e.g., "America/Los_Angeles") where a
UTC timestamp maps to a different local day to verify isAlreadyBusy respects
timeZone normalization. Use the same helpers (LimitManager, addBusyTime,
isAlreadyBusy) and dayjs timestamps as in existing tests so they clearly target
the bug in key normalization across startOf(unit) boundaries.
In `@packages/lib/intervalLimits/limitManager.ts`:
- Around line 85-91: The busyMap key is being constructed inline but duplicates
the logic in LimitManager.createKey; replace the inline template string
`${params.unit}-${tzStart.startOf(params.unit).toISOString()}` with a call to
the existing helper (e.g., this.createKey(...)) so the code uses
LimitManager.createKey when calling busyMap.set; keep the same tzStart,
params.unit, and other values (start/end/title/source) but pass the key returned
by createKey instead of building it inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d6acc0a-3840-4b5d-9579-29cd6e08c574
📒 Files selected for processing (2)
packages/lib/__tests__/limitManager.test.tspackages/lib/intervalLimits/limitManager.ts
What does this PR do?
Fixes an inconsistency in
LimitManagerwhere busy map keys were generated differently betweencreateKeyandaddBusyTime.Previously:
createKeynormalized timestamps usingstartOf(unit)addBusyTimeused raw timestampsThis mismatch caused:
isAlreadyBusy()to return incorrect resultsExample:
Adding a busy time at
2026-04-19T15:30(day)Checking for
2026-04-19T10:00(same day)Before:
falseAfter:
trueVisual Demo (For contributors especially)
N/A - logic-level fix, no UI involved.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
LimitManagerinstancestart = 2026-04-19T15:30unit = "day"Automated tests added in:
packages/lib/__tests__/limitManager.test.ts