refactor(api): migrate statistics endpoints to chained API pattern#39624
refactor(api): migrate statistics endpoints to chained API pattern#39624smirk-dev wants to merge 1 commit intoRocketChat:developfrom
Conversation
Migrate statistics, statistics.list, and statistics.telemetry endpoints from legacy API.v1.addRoute() to the new chained router API with AJV validators and typed response schemas. Part of RocketChat#38876 Co-Authored-By: Claude Opus 4.6 <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 |
|
WalkthroughThis pull request consolidates multiple statistics API endpoints into a single unified definition and introduces telemetry validation. It adds type definitions, schemas, and validators for telemetry request bodies, augments REST API typings, and exposes the new statistics types through the public API surface. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Server
participant Validator as Validator
participant Handler as Telemetry Handler
participant Service as Telemetry Service
Client->>API: POST /statistics.telemetry<br/>(with event data)
API->>Validator: Validate request body<br/>with isTelemetryBody
alt Validation Success
Validator-->>API: Valid TelemetryBody
API->>Handler: Iterate events from body
loop For each event
Handler->>Handler: Cast eventName to<br/>TelemetryEvents type
Handler->>Service: Invoke telemetryEvent<br/>(eventName, data)
Service-->>Handler: Event processed
end
Handler-->>API: All events processed
API-->>Client: Response (validated)
else Validation Fails
Validator-->>API: Invalid body
API-->>Client: Error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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}
There was a problem hiding this comment.
1 issue found across 2 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="apps/meteor/app/api/server/v1/stats.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/stats.ts:49">
P2: Response schemas claim `IStats`-typed payloads but do not validate `IStats` fields at runtime, creating a false API contract.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| authRequired: true, | ||
| query: isStatisticsProps, | ||
| response: { | ||
| 200: ajv.compile<IStats & { success: true }>({ |
There was a problem hiding this comment.
P2: Response schemas claim IStats-typed payloads but do not validate IStats fields at runtime, creating a false API contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/stats.ts, line 49:
<comment>Response schemas claim `IStats`-typed payloads but do not validate `IStats` fields at runtime, creating a false API contract.</comment>
<file context>
@@ -1,13 +1,64 @@
+ authRequired: true,
+ query: isStatisticsProps,
+ response: {
+ 200: ajv.compile<IStats & { success: true }>({
+ type: 'object',
+ additionalProperties: true,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/api/server/v1/stats.ts (1)
64-69:⚠️ Potential issue | 🟠 MajorType mismatch:
Statistics.findLast()declaresPromise<IStats>but returnsPromise<IStats | undefined>.
Statistics.findLast()at line 29 returnsrecords?.[0], which isundefinedwhen no statistics exist. The method signature at line 19 incorrectly omitsundefinedfrom the return type. WhengetLastStatisticspasses thisundefinedvalue toAPI.v1.success(), the response body becomesundefinedinstead of{ success: true, ...IStats }, violating the 200 response schema that requires bothsuccess: trueandIStatsfields.Fix: Update
Statistics.findLast()return type toPromise<IStats | undefined>and add a null check in the stats endpoint before callingAPI.v1.success(), or provide a default empty statistics object.🤖 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 64 - 69, Statistics.findLast currently declares Promise<IStats> but can return undefined; change its signature to Promise<IStats | undefined> and update any callers accordingly (notably getLastStatistics and the stats endpoint). In the stats endpoint that calls getLastStatistics before returning API.v1.success, add a null check for the returned value and either return a default empty IStats object or respond with an appropriate API.v1.failure/empty payload; ensure API.v1.success is only called with a defined IStats so the 200 response schema (success: true + IStats) is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 64-69: Statistics.findLast currently declares Promise<IStats> but
can return undefined; change its signature to Promise<IStats | undefined> and
update any callers accordingly (notably getLastStatistics and the stats
endpoint). In the stats endpoint that calls getLastStatistics before returning
API.v1.success, add a null check for the returned value and either return a
default empty IStats object or respond with an appropriate API.v1.failure/empty
payload; ensure API.v1.success is only called with a defined IStats so the 200
response schema (success: true + IStats) is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a25d694-a7a6-4914-94ae-e4fa5e85aad0
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/stats.tspackages/rest-typings/src/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/index.tsapps/meteor/app/api/server/v1/stats.ts
🧠 Learnings (7)
📓 Common learnings
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:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
📚 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/index.tsapps/meteor/app/api/server/v1/stats.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/index.tsapps/meteor/app/api/server/v1/stats.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/stats.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/stats.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/stats.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/stats.ts
🔇 Additional comments (4)
packages/rest-typings/src/index.ts (1)
260-261: LGTM!The wildcard re-export follows the established pattern used by other endpoint modules in this file and correctly exposes the statistics validators and types through the public API surface.
apps/meteor/app/api/server/v1/stats.ts (3)
72-110: LGTM!The paginated response schema correctly validates all required fields, and the endpoint handler properly integrates with
getStatistics.
112-140: LGTM!The
ajv.compile<void>pattern for the 200 response is consistent with the established convention for migration PRs. The type casts on line 135 preserve existing runtime behavior. Based on learnings from previous OpenAPI migration PRs, the patternajv.compile<void>({...})is intentionally used even when the endpoint returns{ success: true }.
142-147: LGTM!The type export and module augmentation follow the established pattern for endpoint migrations, properly integrating with the rest-typings type system.
|
Hey, thanks for the contribution! 🙏 I'm closing this PR because the endpoint(s) covered here have already been migrated as part of a larger batch migration I'm working on (see #39820). I decided to go with a mass migration approach because reviewing individual PRs for each endpoint was taking too much of my time. That said, you're more than welcome to help by reviewing and testing the changes in #39820 to make sure everything is working correctly. Your input would be really valuable! Thanks again for your effort! 🚀 |
2 participants
Summary
Migrates all 3 statistics-related REST API endpoints from the legacy
API.v1.addRoute()pattern to the new chained router API with full AJV validation and typed response schemas.Endpoints migrated:
GET statistics— withisStatisticsPropsquery validator and typedIStatsresponseGET statistics.list— withisStatisticsListPropsquery validator and paginated response schemaPOST statistics.telemetry— with newisTelemetryBodyAJV validator and void response schemaChanges:
apps/meteor/app/api/server/v1/stats.ts: Converted all 3addRoutecalls to chained.get()/.post()pattern with response schemas for 200, 400, and 401 status codespackages/rest-typings/src/index.ts: Addedexport * from './v1/statistics'to re-export existing validators (isStatisticsProps,isStatisticsListProps)ExtractRoutesFromAPI+ module augmentation for automatic type inferenceisTelemetryBodyAJV validator for the telemetry endpoint body paramsTest plan
GET /api/v1/statisticsreturns stats with proper validationGET /api/v1/statistics.listreturns paginated resultsPOST /api/v1/statistics.telemetryaccepts valid telemetry payloadsPart of #38876
cc @ggazzo — would appreciate your review on this migration batch
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements