fix(calendar): address review comments and improve subject search#39236
Open
gauravsingh001-cyber wants to merge 3 commits intoRocketChat:developfrom
Open
fix(calendar): address review comments and improve subject search#39236gauravsingh001-cyber wants to merge 3 commits intoRocketChat:developfrom
gauravsingh001-cyber wants to merge 3 commits intoRocketChat:developfrom
Conversation
Contributor
|
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 |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds end-to-end subject search for calendar events: new validated, rate-limited REST route Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Route (calendar-events.search)
participant Service as CalendarService
participant Model as CalendarEvent Model
participant DB as Database
Client->>API: GET /v1/calendar-events.search?text=Meeting
API->>Service: searchBySubject(userId, "Meeting")
Service->>Service: trim/validate text
alt text empty
Service-->>API: []
else text present
Service->>Model: findBySubject(uid, "Meeting")
Model->>Model: escapeRegExp("Meeting") / build case-insensitive regex
Model->>DB: query by uid & subject regex (limit 50, sort startTime)
DB-->>Model: matching events[]
Model-->>Service: events[]
Service-->>API: events[]
end
API-->>Client: { success: true, data: [...] }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
coderabbitai
bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
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="packages/models/src/models/CalendarEvent.ts">
<violation number="1" location="packages/models/src/models/CalendarEvent.ts:7">
P1: Duplicate escapeRegExp utility: should import from '@rocket.chat/string-helpers'</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| import { BaseRaw } from './BaseRaw'; | ||
|
|
||
| function escapeRegExp(text: string): string { |
Contributor
There was a problem hiding this comment.
P1: Duplicate escapeRegExp utility: should import from '@rocket.chat/string-helpers'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/models/src/models/CalendarEvent.ts, line 7:
<comment>Duplicate escapeRegExp utility: should import from '@rocket.chat/string-helpers'</comment>
<file context>
@@ -4,6 +4,10 @@ import type { FindCursor, IndexDescription, Collection, Db, UpdateResult } from
import { BaseRaw } from './BaseRaw';
+function escapeRegExp(text: string): string {
+ return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/app/api/server/v1/calendar.ts`:
- Around line 28-41: The route handler for 'calendar-events.search' lacks
validateParams, so add a validateParams function to the API.v1.addRoute call
that validates this.queryParams.text (e.g., ensure text is present when required
or is an optional string and not an object) before calling
Calendar.searchBySubject; update the route configuration object (the same one
containing async get()) to include validateParams that returns a sanitized
params object or throws on invalid input so Calendar.searchBySubject(userId,
text) only receives a validated string.
In `@apps/meteor/server/services/calendar/service.ts`:
- Around line 102-108: The searchBySubject method materializes an unbounded
cursor with CalendarEvent.findBySubject(...).toArray(), which can OOM for large
calendars; modify searchBySubject(uid, text) to apply a server-side limit (and
optionally offset/page parameters) to the query before calling toArray (or
return the cursor for caller pagination), e.g. add limit and/or pagination args
to searchBySubject and pass them into CalendarEvent.findBySubject so the cursor
is capped rather than fully materialized.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/app/api/server/v1/calendar.tsapps/meteor/server/services/calendar/service.tspackages/core-services/src/types/ICalendarService.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). (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:
packages/models/src/models/CalendarEvent.tspackages/core-services/src/types/ICalendarService.tsapps/meteor/server/services/calendar/service.tsapps/meteor/app/api/server/v1/calendar.ts
🧠 Learnings (4)
📚 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.tspackages/core-services/src/types/ICalendarService.tsapps/meteor/server/services/calendar/service.tsapps/meteor/app/api/server/v1/calendar.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.tspackages/core-services/src/types/ICalendarService.tsapps/meteor/server/services/calendar/service.tsapps/meteor/app/api/server/v1/calendar.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/calendar.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/calendar.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/calendar.ts (1)
packages/core-services/src/index.ts (1)
Calendar(179-179)
🔇 Additional comments (3)
packages/models/src/models/CalendarEvent.ts (2)
7-9: Safe regex escaping implementation.Escaping user input before building the regex is correct and prevents regex metacharacter injection.
55-67: Subject search query shape is solid.User scoping (
uid), escaped case-insensitive matching, and deterministic sorting are all correctly wired.packages/core-services/src/types/ICalendarService.ts (1)
10-16: Interface exposure is consistent.Adding
searchBySubjecttoICalendarServicecleanly aligns the contract with the new service/API behavior.
…revent unbounded query
coderabbitai
bot
added
community
and removed
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rest-typings/src/v1/calendar/CalendarEventSearchProps.ts (1)
11-14: Captextlength in schema to bound search workload.
textis currently unbounded. Adding amaxLengthhelps avoid oversized query payloads reaching regex-based search paths.Suggested schema hardening
text: { type: 'string', nullable: false, + maxLength: 256, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rest-typings/src/v1/calendar/CalendarEventSearchProps.ts` around lines 11 - 14, The schema property "text" in CalendarEventSearchProps is unbounded; add a maxLength constraint to the text property definition (e.g., maxLength: 1024 or another acceptable limit) to prevent oversized query payloads from reaching regex-based search paths; update the "text" field object (the one with type: 'string', nullable: false) to include maxLength and adjust any validation tests or consumers that assume unlimited length.
🤖 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/app/api/server/v1/calendar.ts`:
- Around line 32-34: Remove the inline implementation comment in the route
config by deleting the trailing "// 👈 yaha use hoga" after the validateParams
property (validateParams: isCalendarEventSearchProps) so the config remains
comment-free; leave the property and its value unchanged and ensure surrounding
formatting/commas are preserved in the same object where authRequired and
rateLimiterOptions are defined.
---
Nitpick comments:
In `@packages/rest-typings/src/v1/calendar/CalendarEventSearchProps.ts`:
- Around line 11-14: The schema property "text" in CalendarEventSearchProps is
unbounded; add a maxLength constraint to the text property definition (e.g.,
maxLength: 1024 or another acceptable limit) to prevent oversized query payloads
from reaching regex-based search paths; update the "text" field object (the one
with type: 'string', nullable: false) to include maxLength and adjust any
validation tests or consumers that assume unlimited length.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/app/api/server/v1/calendar.tsapps/meteor/server/services/calendar/service.tspackages/rest-typings/src/v1/calendar/CalendarEventSearchProps.tspackages/rest-typings/src/v1/calendar/index.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:
packages/rest-typings/src/v1/calendar/index.tsapps/meteor/server/services/calendar/service.tsapps/meteor/app/api/server/v1/calendar.tspackages/rest-typings/src/v1/calendar/CalendarEventSearchProps.ts
🧠 Learnings (5)
📚 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/rest-typings/src/v1/calendar/index.tsapps/meteor/server/services/calendar/service.tsapps/meteor/app/api/server/v1/calendar.tspackages/rest-typings/src/v1/calendar/CalendarEventSearchProps.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/rest-typings/src/v1/calendar/index.tsapps/meteor/server/services/calendar/service.tsapps/meteor/app/api/server/v1/calendar.tspackages/rest-typings/src/v1/calendar/CalendarEventSearchProps.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/calendar.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/api/server/v1/calendar.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/calendar.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/calendar.ts (2)
packages/rest-typings/src/v1/calendar/CalendarEventSearchProps.ts (1)
isCalendarEventSearchProps(20-20)packages/core-services/src/index.ts (1)
Calendar(179-179)
🔇 Additional comments (2)
apps/meteor/server/services/calendar/service.ts (1)
102-110: Good guard + bounded search result size.The empty-text early return and server-side
limitmake this route safer and more predictable under load.packages/rest-typings/src/v1/calendar/index.ts (1)
44-46: Typed endpoint contract addition looks solid.The new
/v1/calendar-events.searchsignature is consistent with the existing endpoint typing structure.
coderabbitai
bot
added
type: feature
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #39039
What this PR does
This PR updates the subject-based calendar event search implementation by:
searchBySubjecttoICalendarServicecalendar-events.searchescapeRegExputility to properly escape user inputWhy this change is needed
Previously, the search function was added only inside the service and was not exposed through the interface or API.
This update ensures the feature is fully usable and follows the existing project architecture.
Checklist
Summary by CodeRabbit