refactor: migrate statistics endpoints to new chained API pattern#38884
refactor: migrate statistics endpoints to new chained API pattern#38884smirk-dev wants to merge 2 commits intoRocketChat:developfrom
Conversation
|
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: cb07e54 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:
WalkthroughThree statistics REST endpoints were migrated from the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API.v1 Handler
participant DB as Statistics Store
participant Tele as Telemetry Service
Client->>API: GET /v1/statistics or /v1/statistics.list
API->>DB: fetchStatistics / getStatistics (with parsed query)
DB-->>API: statistics data
API-->>Client: 200 { success, data... }
Client->>API: POST /v1/statistics.telemetry { events[] }
API->>API: validate body (AJV) and parse events
API->>Tele: telemetryEvent(eventName, payload) for each event
Tele-->>API: ack
API-->>Client: 200 { success: true }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-stats-api-chained-pattern.mdapps/meteor/app/api/server/v1/stats.ts
🧰 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/app/api/server/v1/stats.ts
🧠 Learnings (2)
📚 Learning: 2026-02-10T16:32:49.806Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:49.806Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON which automatically deserializes Date objects. Stream handlers (e.g., sdk.stream()) receive Date fields as Date objects directly and do not require manual conversion using `new Date()`. Only REST API responses require manual date conversion because they return plain JSON where dates are serialized as strings.
Applied to files:
.changeset/refactor-stats-api-chained-pattern.md
📚 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/stats.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/stats.ts (1)
packages/rest-typings/src/v1/statistics.ts (2)
isStatisticsProps(37-37)isStatisticsListProps(69-69)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
.changeset/refactor-stats-api-chained-pattern.md (1)
1-5: LGTM!Changelog entry is well-scoped (
patch) and accurately describes the migration.apps/meteor/app/api/server/v1/stats.ts (2)
13-42: LGTM!
statisticsendpoint is correctly structured:authRequired,isStatisticsPropsparam validation, intentionally broad 200 schema (justified by dynamicIStatsfields), and correctrefreshstring-to-boolean conversion.
115-115: This usage is intentional and correct —telemetryEvent.call()is a method on theTelemetryEventclass, notFunction.prototype.call().The
TelemetryEventclass defines its owncallmethod with signaturecall<T extends TelemetryEvents>(eventName: T, data: TelemetryMap[T]). The invocationtelemetryEvent.call(eventName, params)correctly passeseventNameas the first argument andparamsas the second, matching the designed method signature. No change needed.Likely an incorrect or invalid review comment.
🤖 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/stats.ts`:
- Around line 50-68: The TypeScript generic passed to ajv.compile (the anonymous
generic starting at ajv.compile<...>) is missing the success property required
by the JSON schema; update the generic type to include success: boolean so it
matches the schema (i.e., the compiled validator for ajv.compile<{ statistics:
unknown[]; count: number; offset: number; total: number; success: boolean }>)
ensuring the type guard produced by ajv.compile aligns with the schema.
- Around line 111-113: The handler currently reads this.bodyParams and uses
events?.params?.forEach which silently no-ops on missing/malformed input; add
explicit validation and reject bad requests. Call this.validateParams (or
equivalent request validator) on this.bodyParams with a schema that requires
params to be an array of objects with eventName: string (and any other required
keys), then replace the optional-chain loop with events.params.forEach so
malformed/missing params cause a 4xx via the validator; ensure error handling
returns a proper BadRequest response when validation fails.
- Line 96: The ajv.compile call is using the wrong generic (void); define a
concrete TypeScript type or interface that matches the response schema (e.g., a
SuccessResponse type representing { success: true }) and use that type as the
generic for ajv.compile instead of void so the compiled validator becomes a
meaningful type guard; update the ajv.compile invocation (and any downstream
validator variable) to use this new type.
smirk-dev
force-pushed
the
refactor/migrate-stats-api-chained-pattern
branch
from
bb2c550 to
13b5fb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-stats-api-chained-pattern.mdapps/meteor/app/api/server/v1/stats.ts
🧰 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/app/api/server/v1/stats.ts
🧠 Learnings (3)
📚 Learning: 2026-02-10T16:32:49.806Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:49.806Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON which automatically deserializes Date objects. Stream handlers (e.g., sdk.stream()) receive Date fields as Date objects directly and do not require manual conversion using `new Date()`. Only REST API responses require manual date conversion because they return plain JSON where dates are serialized as strings.
Applied to files:
.changeset/refactor-stats-api-chained-pattern.md
📚 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/app/api/server/v1/stats.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/stats.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/stats.ts (2)
apps/meteor/app/api/server/index.ts (1)
API(51-51)apps/meteor/app/statistics/server/index.ts (1)
getLastStatistics(6-6)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.changeset/refactor-stats-api-chained-pattern.md (1)
1-5: LGTM — changelog entry is accurate and properly formatted.
🤖 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/stats.ts`:
- Around line 8-36: The GET route registration API.v1.get('statistics', ...) is
missing validateParams; add a validateParams property to the route options using
the isStatisticsProps validator (imported from `@rocket.chat/rest-typings`) so
query params for the statistics endpoint are validated before action runs;
likewise update the statistics.list GET route to include validateParams:
isStatisticsListProps; ensure you import isStatisticsProps and
isStatisticsListProps and place validateParams alongside authRequired/response
in the options object so getLastStatistics still receives validated
this.queryParams.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 104-112: The handler currently reads this.bodyParams and silently
no-ops when params is missing or not an array; add input validation (e.g., a
validateParams schema) to enforce that body has shape { params: Array<{
eventName: string; [key: string]: unknown }> } before the handler runs,
returning a 4xx on invalid input, and then within the handler use the
already-parsed params to iterate and call telemetryEvent.call(eventName, params)
so malformed requests no longer return success; reference this.bodyParams,
telemetryEvent.call, and the handler that returns API.v1.success() when adding
the validateParams check.
- Around line 43-48: The AJV compile generic used in ajv.compile for the
statistics.list response is missing the required "success" property, making the
produced type guard incorrect; update the generic passed to ajv.compile (the
object type used where you currently have statistics: unknown[]; count: number;
offset: number; total: number) to include success: boolean (or success: true if
you want to narrow to a successful response) so the compiled validator's type
matches the JSON schema's required "success" field.
- Around line 89-99: The AJV compile call currently uses the meaningless generic
void (ajv.compile<void>) which makes the validator a type guard for undefined;
change the generic to a proper response type that matches the schema (for
example replace ajv.compile<void> with ajv.compile<{ success: true }>, or define
a named interface like interface SuccessResponse { success: true } and use
ajv.compile<SuccessResponse>()), ensuring the validator signature becomes (data:
unknown) => data is { success: true } and aligns with the schema.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38884 +/- ##
========================================
Coverage 70.55% 70.56%
========================================
Files 3189 3189
Lines 112703 112703
Branches 20429 20412 -17
========================================
+ Hits 79519 79527 +8
+ Misses 31123 31115 -8
Partials 2061 2061
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
smirk-dev
force-pushed
the
refactor/migrate-stats-api-chained-pattern
branch
from
13b5fb5 to
abf99ca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/stats.ts (1)
12-22: TypeScript generic is narrower than the JSON schema forrefresh.The generic constrains
refreshto'true' | 'false', but the JSON schema only specifiestype: 'string'without anenumconstraint, so any string will pass validation at runtime. Either add anenumto the schema or widen the generic tostring.🛠️ Proposed fix — add enum constraint to schema
query: ajv.compile<{ refresh?: 'true' | 'false' }>({ type: 'object', properties: { refresh: { type: 'string', nullable: true, + enum: ['true', 'false'], }, }, required: [], additionalProperties: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/stats.ts` around lines 12 - 22, The TypeScript generic on ajv.compile (compile<{ refresh?: 'true' | 'false' }>) is stricter than the JSON schema (refresh: { type: 'string' }), so update them to match: either add an enum to the schema (properties.refresh.enum = ['true','false']) so the runtime schema enforces the same values, or change the generic to compile<{ refresh?: string }> to reflect the schema; locate the ajv.compile call and apply one of these two consistent fixes to ensure type and schema align.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-stats-api-chained-pattern.mdapps/meteor/app/api/server/v1/stats.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/refactor-stats-api-chained-pattern.md
🧰 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/app/api/server/v1/stats.ts
🧠 Learnings (2)
📚 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/app/api/server/v1/stats.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/stats.ts
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/app/api/server/v1/stats.ts (5)
1-6: LGTM on imports.The imports are clean and appropriate for the new chained API pattern. Note that
isStatisticsPropsandisStatisticsListPropsmentioned in the PR description are no longer used since the new pattern employs inlinequery/bodyschemas instead ofvalidateParams.
37-46: LGTM on thestatisticshandler.Clean extraction of
refreshwith a sensible default, and the async flow is correct.
88-104: LGTM on thestatistics.listhandler.Pagination, query parsing, and the return flow look correct.
142-151: LGTM on thestatistics.telemetryhandler.Body validation now requires
paramsvia thebodyschema, and the iteration overevents.paramsis safe post-validation. The fire-and-forgetvoid telemetryEvent.call(...)is intentional for telemetry dispatch.
53-64: The review comment is based on an incorrect assumption about AJV configuration — no changes needed.The shared AJV instance in
packages/rest-typings/src/v1/Ajv.tsis initialized withcoerceTypes: true, which automatically converts string query parameters to numbers during validation. Thecountandoffsetparameters declared astype: 'number'in the stats.ts query schema are correct and follow the established pattern used throughout the codebase (e.g., identical declarations inapps/meteor/app/api/server/v1/rooms.ts).Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 66-71: The TypeScript generic passed to ajv.compile for the 200
response is missing the success property, so update the generic type used in
ajv.compile<{ statistics: unknown[]; count: number; offset: number; total:
number; }> to include success: boolean (e.g. add success: boolean) so it matches
the JSON schema and the AJV type guard for the 200 response; ensure the generic
shape exactly mirrors the schema properties (statistics, count, offset, total,
success) used in the compile call.
- Around line 128-138: The AJV call uses ajv.compile<void> which incorrectly
types the validator as "no value"; change the generic to a concrete interface
matching the schema (e.g., a type like SuccessResponse = { success: true }) and
update the ajv.compile generic to ajv.compile<SuccessResponse> for the schema
object with properties { success: { type: 'boolean', enum: [true] } }, required
['success'], additionalProperties: false so the validator's type guard is
meaningful.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 12-22: The TypeScript generic on ajv.compile (compile<{ refresh?:
'true' | 'false' }>) is stricter than the JSON schema (refresh: { type: 'string'
}), so update them to match: either add an enum to the schema
(properties.refresh.enum = ['true','false']) so the runtime schema enforces the
same values, or change the generic to compile<{ refresh?: string }> to reflect
the schema; locate the ajv.compile call and apply one of these two consistent
fixes to ensure type and schema align.
abf99ca to
40b9909
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-stats-api-chained-pattern.mdapps/meteor/app/api/server/v1/stats.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/refactor-stats-api-chained-pattern.md
🧰 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/app/api/server/v1/stats.ts
🧠 Learnings (2)
📚 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/app/api/server/v1/stats.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/stats.ts
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/app/api/server/v1/stats.ts (3)
46-48: [rewritten comment]
[classification tag]
58-69: The sharedajvinstance from@rocket.chat/rest-typingsis properly configured withcoerceTypes: true, sotype: 'number'validation for query-string parameters will work correctly. String values like"10"will be coerced to numbers and pass validation as expected. No changes needed.Likely an incorrect or invalid review comment.
150-152: ThetelemetryEvent.callpattern is correct.telemetryEventis an instance of theTelemetryEventclass, which implements a customcall<T extends TelemetryEvents>(eventName: T, data: TelemetryMap[T])method that retrieves and invokes the registered event handler. This is notFunction.prototype.call.
🤖 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/stats.ts`:
- Around line 13-22: The JSON schema passed to ajv.compile for the query object
does not enforce the TypeScript-annotated union for refresh; update the schema
for the refresh property (in the ajv.compile call) to include an enum constraint
with the two allowed string values 'true' and 'false' (and keep nullable: true
if you still want to allow null), so the compiled validator matches the TS
generic { refresh?: 'true' | 'false' } and rejects arbitrary strings.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 71-89: The TypeScript generic passed to ajv.compile narrows the
validated type but omits the required success property; update the generic on
ajv.compile (the ajv.compile<...> invocation that defines { statistics, count,
offset, total }) to include success: boolean so it matches the JSON schema
object (and ensure the required list in the schema still includes 'success') to
keep the type guard in sync with the schema.
- Line 133: The AJV schema is being compiled with ajv.compile<void>, which
narrows to undefined and contradicts the response schema that returns { success:
true }; update the type parameter to a concrete interface matching the schema
(e.g., a type like { success: true } or a named ResponseSuccess type) used where
ajv.compile<void> is called so the compiled validator and downstream code (the
validator variable in stats.ts where ajv.compile<void> appears) have the correct
type instead of void.
Migrates statistics (GET), statistics.list (GET), and statistics.telemetry (POST) from the legacy addRoute() pattern to the new chained .get()/.post() API pattern with typed AJV response schemas and query parameter validation using existing isStatisticsProps and isStatisticsListProps validators. Part of RocketChat#38876
smirk-dev
force-pushed
the
refactor/migrate-stats-api-chained-pattern
branch
from
40b9909 to
15cdd41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-stats-api-chained-pattern.mdapps/meteor/app/api/server/v1/stats.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/refactor-stats-api-chained-pattern.md
🧰 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/app/api/server/v1/stats.ts
🧠 Learnings (2)
📚 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/app/api/server/v1/stats.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/stats.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/stats.ts (1)
packages/core-typings/src/IStats.ts (1)
IStats(20-276)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/stats.ts (1)
10-24: No action required. Thequeryandbodyoption keys are valid and correctly defined in theTypedOptionsinterface (definition.ts lines 290–291), with corresponding typed extraction inTypedThis(queryParams/bodyParams). The stats.ts implementation correctly uses these keys, and query/body validation runs as expected. The deprecatedvalidateParamswas appropriately replaced with thequeryandbodypattern across the endpoints.Likely an incorrect or invalid review comment.
🤖 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/stats.ts`:
- Around line 47-49: The current handler checks the retrieved variable "stats"
and throws a plain Error which becomes an HTTP 500; replace that with a
framework-appropriate error so the client receives a 4xx (e.g., not-found)
response. Update the "if (!stats) { ... }" branch to throw a Meteor.Error with a
short error code like "not-found" and the descriptive message "No statistics
found" (or, if this handler uses Express-style response objects, return
res.status(404).json({ error: 'No statistics found' }) instead) so the framework
maps it to a 404/4xx rather than a 500.
- Around line 26-35: The JSON schema validator uses ajv.compile<IStats> but
IStats has no success field, so the validator incorrectly narrows the runtime
type; change the call to not assert IStats (either remove the generic or supply
an accurate type like { success: true } or a new IStatsResponse interface) and
update any code that relied on the bogus type narrow: locate the
ajv.compile<IStats> usage in stats.ts and replace it with ajv.compile<{ success:
true }>(...) or define and use a proper response interface, ensuring downstream
code treats the validated value according to the actual schema rather than
IStats.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 72-77: The TypeScript generic passed to ajv.compile for the 200
response is missing the required "success" field; update the generic type used
in the ajv.compile call to include success: boolean (i.e., add a success:
boolean property alongside statistics, count, offset, total) so the compiled
validator's type guard matches the JSON schema.
- Around line 14-24: The AJV schema for the query (the object passed into
ajv.compile in the query definition) defines refresh as type: 'string' but the
TypeScript generic expects 'true' | 'false', so the validator currently accepts
any string; update the schema for the refresh property to include an enum:
['true','false'] (keeping nullable: true if you still want null) so the compiled
type guard matches the TS generic used by the query.
- Around line 134-144: The AJV validator for the statistics.telemetry 200
response is typed as ajv.compile<void>, which is incorrect; change the generic
to match the JSON schema (for example ajv.compile<{ success: true }>) or a named
TypeScript type/interface that describes the response shape (e.g., type
TelemetrySuccess = { success: true }) and use ajv.compile<TelemetrySuccess> so
the runtime schema and compile-time type guard align with the { success: true }
schema.
When _internalRouteActionHandler catches errors and returns status 400, the Router's response validation in TEST_MODE requires a matching response validator. Without a 400 validator, it throws 'Missing response validator' which becomes HTTP 500. Also remove unnecessary null check that wasn't in original code.
|
I already have a PR open for this one too. Please close yours. You should also trace my existing PR here: PR #150 before opening any new ones. so we don't have collisions between our PRs |
2 participants
Summary
Migrates the
statistics(GET),statistics.list(GET), andstatistics.telemetry(POST) endpoints from the legacyAPI.v1.addRoute()pattern to the new chained.get()/.post()API pattern.Changes
addRoutecalls withAPI.v1.get()andAPI.v1.post()chained patternvalidateParamsusing existingisStatisticsPropsandisStatisticsListPropsAJV validators from@rocket.chat/rest-typingsfor query parameter validationstatistics: flexible response schema (IStats has many dynamic fields)statistics.list: validatesstatisticsarray,count,offset,totalfieldsstatistics.telemetry: validates success-only responseMotivation
Part of the ongoing effort to migrate REST API endpoints to the new chained API pattern that enables typed response validation and future OpenAPI spec generation.
Closes part of #38876
Summary by CodeRabbit