fix(calendar): handle reminderMinutesBeforeStart = 0 in all code paths#39799
fix(calendar): handle reminderMinutesBeforeStart = 0 in all code paths#39799tanayyo1 wants to merge 6 commits intoRocketChat:developfrom
Conversation
Fixes RocketChat#38965 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 60e58d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTreat explicit zero ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Pull request overview
This PR fixes calendar reminder scheduling when reminderMinutesBeforeStart is set to 0 (a valid value that was previously ignored due to truthy checks), aiming to cover all reminder computation/update paths.
Changes:
- Remove truthy-based reminder time calculation in
create()and apply??fallback consistently. - Update
import()to use the same defaulting/reminder time computation approach ascreate(). - Change
update()andCalendarEvent.updateEvent()to treat0as a valid value via!= nullchecks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/models/src/models/CalendarEvent.ts |
Ensures reminderMinutesBeforeStart = 0 is persisted by avoiding truthy checks in $set construction. |
apps/meteor/server/services/calendar/service.ts |
Adjusts reminder time computation in create()/import() and fixes update() to accept 0 without being filtered out. |
Comments suppressed due to low confidence (1)
apps/meteor/server/services/calendar/service.ts:71
- In
import(), you computeminutesusing the?? defaultMinutesForNotificationsfallback, butupdateDatastill setsreminderMinutesBeforeStartfrom the originalreminderMinutesBeforeStartfield. When that input isundefined/null, you end up persisting areminderTimebased on the default while not persisting the corresponding minutes value, leaving the record inconsistent and potentially scheduling reminders unexpectedly. Use the computedminutesvalue consistently when buildingupdateData(and when inserting/updating imported events).
const minutes = reminderMinutesBeforeStart ?? defaultMinutesForNotifications;
const reminderTime = getShiftedTime(startTime, -minutes);
const updateData: Omit<InsertionModel<ICalendarEvent>, 'uid' | 'notificationSent'> = {
startTime,
...(endTime && { endTime }),
subject,
description,
meetingUrl,
reminderMinutesBeforeStart,
reminderTime,
externalId,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async create(data: Omit<InsertionModel<ICalendarEvent>, 'reminderTime' | 'notificationSent'>): Promise<ICalendarEvent['_id']> { | ||
| const { uid, startTime, endTime, subject, description, reminderMinutesBeforeStart, meetingUrl, busy } = data; | ||
| const minutes = reminderMinutesBeforeStart ?? defaultMinutesForNotifications; | ||
| const reminderTime = minutes ? getShiftedTime(startTime, -minutes) : undefined; | ||
| const reminderTime = getShiftedTime(startTime, -minutes); | ||
|
|
||
| const insertData: InsertionModel<ICalendarEvent> = { | ||
| uid, |
There was a problem hiding this comment.
This PR changes reminder handling semantics for create(), import(), and update() around the edge case reminderMinutesBeforeStart = 0. There are unit tests for CalendarService in this repo, but none covering the 0-minute reminder case across these code paths; adding tests that assert the persisted reminderMinutesBeforeStart and computed reminderTime for 0 (and for the default fallback) would prevent regressions like the falsy check bug from reappearing.
|
|
||
| const meetingUrl = await this.getMeetingUrl(data); | ||
| const reminderTime = reminderMinutesBeforeStart && startTime ? getShiftedTime(startTime, -reminderMinutesBeforeStart) : undefined; | ||
| const reminderTime = reminderMinutesBeforeStart != null && startTime ? getShiftedTime(startTime, -reminderMinutesBeforeStart) : undefined; |
There was a problem hiding this comment.
update() recomputes reminderTime only when both reminderMinutesBeforeStart != null and startTime is provided in the patch. Since this method accepts Partial<ICalendarEvent> (and is exercised in tests with patches that omit startTime), updating just reminderMinutesBeforeStart (including the 0 case this PR is targeting) would update the minutes but leave reminderTime unchanged in the DB. Consider computing the reminder time using the effective start time (e.g., patch startTime or the existing event.startTime) whenever reminderMinutesBeforeStart is provided.
| const reminderTime = reminderMinutesBeforeStart != null && startTime ? getShiftedTime(startTime, -reminderMinutesBeforeStart) : undefined; | |
| const effectiveStartTime = startTime ?? event.startTime; | |
| const reminderTime = | |
| reminderMinutesBeforeStart != null && effectiveStartTime | |
| ? getShiftedTime(effectiveStartTime, -reminderMinutesBeforeStart) | |
| : undefined; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/services/calendar/service.ts (1)
60-70:⚠️ Potential issue | 🟠 MajorPersist the same effective minutes used to compute
reminderTimein import.This block computes
reminderTimefromminutes, but stores rawreminderMinutesBeforeStart. When input minutes are omitted, these values can diverge.Proposed fix
const updateData: Omit<InsertionModel<ICalendarEvent>, 'uid' | 'notificationSent'> = { startTime, ...(endTime && { endTime }), subject, description, meetingUrl, - reminderMinutesBeforeStart, + reminderMinutesBeforeStart: minutes, reminderTime, externalId, ...(busy !== undefined && { busy }), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/calendar/service.ts` around lines 60 - 70, The code computes an effective minutes value (const minutes = reminderMinutesBeforeStart ?? defaultMinutesForNotifications) and uses it to derive reminderTime via getShiftedTime, but then persists the original reminderMinutesBeforeStart which can differ when the input was omitted; update the object-building for updateData (used as Omit<InsertionModel<ICalendarEvent>, 'uid' | 'notificationSent'>) to persist the effective minutes value (minutes) for the reminderMinutesBeforeStart field so stored reminderMinutesBeforeStart and computed reminderTime remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/server/services/calendar/service.ts`:
- Around line 117-121: The computed reminderTime can become stale on partial
updates because it only uses payload values; change the calculation to derive
effective values from the incoming data OR the existing record so partial
updates recompute correctly: fetch the existing event's
startTime/reminderMinutesBeforeStart, compute effectiveStart = data.startTime ??
existing.startTime and effectiveReminderMinutes =
data.reminderMinutesBeforeStart ?? existing.reminderMinutesBeforeStart, then set
reminderTime = (effectiveStart && effectiveReminderMinutes != null) ?
getShiftedTime(effectiveStart, -effectiveReminderMinutes) : undefined; update
the logic around the reminderTime variable (near the getMeetingUrl call) to use
these effective values.
---
Outside diff comments:
In `@apps/meteor/server/services/calendar/service.ts`:
- Around line 60-70: The code computes an effective minutes value (const minutes
= reminderMinutesBeforeStart ?? defaultMinutesForNotifications) and uses it to
derive reminderTime via getShiftedTime, but then persists the original
reminderMinutesBeforeStart which can differ when the input was omitted; update
the object-building for updateData (used as Omit<InsertionModel<ICalendarEvent>,
'uid' | 'notificationSent'>) to persist the effective minutes value (minutes)
for the reminderMinutesBeforeStart field so stored reminderMinutesBeforeStart
and computed reminderTime remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35eb0e55-41f9-4cd1-b617-e71925a876d1
📒 Files selected for processing (2)
apps/meteor/server/services/calendar/service.tspackages/models/src/models/CalendarEvent.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/models/src/models/CalendarEvent.tsapps/meteor/server/services/calendar/service.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/models/src/models/CalendarEvent.tsapps/meteor/server/services/calendar/service.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/models/src/models/CalendarEvent.tsapps/meteor/server/services/calendar/service.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/services/calendar/service.ts
🔇 Additional comments (2)
packages/models/src/models/CalendarEvent.ts (1)
74-74: Good nullish guard for reminder minutes.This correctly allows
0to be persisted while still skipping missing values.apps/meteor/server/services/calendar/service.ts (1)
27-29: Create flow now handlesreminderMinutesBeforeStart = 0correctly.
minutes+getShiftedTime(startTime, -minutes)preserves event-start reminders.
…r zero-minute reminders the update() path only recalculated reminderTime when startTime was explicitly passed in the patch. updating just reminderMinutesBeforeStart (including the 0 case this fix targets) would leave reminderTime stale. now falls back to the existing event's startTime when the patch doesn't include one. also adds unit tests covering reminderMinutesBeforeStart = 0 across create(), import(), and update() to prevent this regression from reappearing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/unit/server/services/calendar/service.tests.ts">
<violation number="1" location="apps/meteor/tests/unit/server/services/calendar/service.tests.ts:301">
P2: The added update fallback-path test uses `10` instead of `0`, so it does not verify the `0` edge case when `startTime` is omitted and existing event `startTime` is used.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/unit/server/services/calendar/service.tests.ts (1)
288-308: Strengthen assertion to verify exact fallback computation.Line 306 currently asserts only
Datetype, so a wrong timestamp could still pass. Since this test claims fallback to existingstartTime, assert the exact shifted value.Proposed test assertion tightening
const updateArgs = CalendarEventMock.updateEvent.firstCall.args[1]; expect(updateArgs.reminderMinutesBeforeStart).to.equal(10); - expect(updateArgs.reminderTime).to.be.an.instanceOf(Date); + expect(updateArgs.reminderTime).to.deep.equal(new Date(fakeStartTime.getTime() - 10 * 60 * 1000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/server/services/calendar/service.tests.ts` around lines 288 - 308, The test currently only asserts reminderTime is a Date; update the assertion in the 'should recalculate reminderTime...' spec to verify the exact fallback computation uses existing fakeEvent.startTime minus the new reminderMinutesBeforeStart (10 minutes). After calling service.update, compute the expected timestamp from fakeStartTime (e.g. fakeStartTime.getTime() - 10 * 60 * 1000) and assert CalendarEventMock.updateEvent.firstCall.args[1].reminderTime.getTime() equals that expected value, while keeping the existing assertion that reminderMinutesBeforeStart equals 10.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/tests/unit/server/services/calendar/service.tests.ts`:
- Around line 288-308: The test currently only asserts reminderTime is a Date;
update the assertion in the 'should recalculate reminderTime...' spec to verify
the exact fallback computation uses existing fakeEvent.startTime minus the new
reminderMinutesBeforeStart (10 minutes). After calling service.update, compute
the expected timestamp from fakeStartTime (e.g. fakeStartTime.getTime() - 10 *
60 * 1000) and assert
CalendarEventMock.updateEvent.firstCall.args[1].reminderTime.getTime() equals
that expected value, while keeping the existing assertion that
reminderMinutesBeforeStart equals 10.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb9d618f-cbce-4a34-8e51-53a02ac4c98f
📒 Files selected for processing (2)
apps/meteor/server/services/calendar/service.tsapps/meteor/tests/unit/server/services/calendar/service.tests.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/server/services/calendar/service.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/unit/server/services/calendar/service.tests.ts
🧠 Learnings (4)
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
apps/meteor/tests/unit/server/services/calendar/service.tests.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/unit/server/services/calendar/service.tests.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/unit/server/services/calendar/service.tests.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/unit/server/services/calendar/service.tests.ts
🔇 Additional comments (3)
apps/meteor/tests/unit/server/services/calendar/service.tests.ts (3)
169-184: Good coverage for create-path0reminder handling.This validates both persisted
reminderMinutesBeforeStartand computedreminderTimeat event start, which directly protects the reported regression.
202-220: Import-path test looks correct for explicit zero reminders.Nice addition verifying
0survives import and thatreminderTimeis derived correctly.
266-287: Update-path test forreminderMinutesBeforeStart = 0is solid.This test correctly exercises the non-truthy edge case in update flow.
…rtTime change import() was adding a 5-minute default reminder to events that never asked for one — only 0 should be treated as valid, not undefined. update() only recomputed reminderTime when reminderMinutesBeforeStart was in the patch. changing just startTime left notifications firing at the old offset. now recomputes when either field changes, falling back to the existing event values. adds test for the startTime-only update case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/calendar/service.ts">
<violation number="1" location="apps/meteor/server/services/calendar/service.ts:60">
P2: Import path dropped default reminder fallback, so imported events without `reminderMinutesBeforeStart` can lose scheduled reminders.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const { uid, startTime, endTime, subject, description, reminderMinutesBeforeStart, busy } = data; | ||
| const meetingUrl = data.meetingUrl ? data.meetingUrl : await this.parseDescriptionForMeetingUrl(description); | ||
| const reminderTime = reminderMinutesBeforeStart ? getShiftedTime(startTime, -reminderMinutesBeforeStart) : undefined; | ||
| const reminderTime = reminderMinutesBeforeStart != null ? getShiftedTime(startTime, -reminderMinutesBeforeStart) : undefined; |
There was a problem hiding this comment.
P2: Import path dropped default reminder fallback, so imported events without reminderMinutesBeforeStart can lose scheduled reminders.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/services/calendar/service.ts, line 60:
<comment>Import path dropped default reminder fallback, so imported events without `reminderMinutesBeforeStart` can lose scheduled reminders.</comment>
<file context>
@@ -57,8 +57,7 @@ export class CalendarService extends ServiceClassInternal implements ICalendarSe
const meetingUrl = data.meetingUrl ? data.meetingUrl : await this.parseDescriptionForMeetingUrl(description);
- const minutes = reminderMinutesBeforeStart ?? defaultMinutesForNotifications;
- const reminderTime = getShiftedTime(startTime, -minutes);
+ const reminderTime = reminderMinutesBeforeStart != null ? getShiftedTime(startTime, -reminderMinutesBeforeStart) : undefined;
const updateData: Omit<InsertionModel<ICalendarEvent>, 'uid' | 'notificationSent'> = {
</file context>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed the feedback from the review bots:
|
Fixes #38965
reminderMinutesBeforeStart = 0gets silently dropped because 0 is falsy in JS. this was only partially addressed in #38966 (fixedcreate(), missed the rest).the same bug exists in 4 places:
create()— condition is unnecessary sinceminutesis always a number after the??fallback. removed it entirely (per review feedback on fix: handle reminderMinutesBeforeStart = 0 correctly #38966)import()— same fix, added the??fallback + removed the ternaryupdate()— changed truthy check to!= nullso 0 passes throughCalendarEvent.updateEvent()— model was also dropping 0 before it reached the DBSummary by CodeRabbit
Bug Fixes
Tests
Chores