refactor: Migrate custom-user-status API to standardized format (Rule 7)#39816
Closed
Makeepan-dev wants to merge 2 commits intoRocketChat:developfrom
Closed
refactor: Migrate custom-user-status API to standardized format (Rule 7)#39816Makeepan-dev wants to merge 2 commits intoRocketChat:developfrom
Makeepan-dev wants to merge 2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Makeepan-dev
force-pushed
the
fix/api-migration-mailer
branch
from
1963dd7 to
5eb30b6
Compare
🦋 Changeset detectedLatest commit: 8f30c16 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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughMigrates custom user status endpoints from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API.v1.post (route)
participant Service as UserStatusService/DB
participant Response as API.v1.response
Client->>API: POST /v1/custom-user-status.create (body validated by AJV)
API->>Service: insertOrUpdateUserStatus(userStatusData)
Service-->>API: persistedId
API->>Service: findOneByName(name)
Service-->>API: userStatus
API->>Response: return success({ customUserStatus: userStatus })
Client->>API: POST /v1/custom-user-status.update (body validated by AJV)
API->>Service: findOneById(_id)
Service-->>API: existingStatus
API->>Service: insertOrUpdateUserStatus(updatedData)
Service-->>API: updatedId
API->>Service: findOneById(_id)
Service-->>API: updatedStatus
API->>Response: return success({ customUserStatus: updatedStatus })
Client->>API: POST /v1/custom-user-status.delete (body validated by AJV)
API->>Service: removeById(customUserStatusId)
Service-->>API: removed
API->>Response: return success({})
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![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
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
10-12: Consider removing explanatory comments.The comments on lines 10-11 explain the Rule 7 migration rationale. While helpful context, they can be deferred to PR description or ADRs to keep the implementation comment-free. As per coding guidelines, avoid code comments in TypeScript/JavaScript implementation files.
🔧 Suggested diff
-// RULE 7: Colocate all endpoint typings and AJV validators within the handler file. -// This removes the dependency on manual definitions in the separate rest-typings package. import { API } from '../../../../api/server';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 10 - 12, Remove the explanatory Rule 7 comments preceding the import in this handler file so the implementation remains comment-free; locate the comment block above the import statement (import { API } from '../../../../api/server';) in apps/meteor/app/api/server/v1/custom-user-status.ts and delete those two lines, leaving only the import and implementation—move any long-form rationale to the PR description or ADR if needed.
🤖 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/custom-user-status.ts`:
- Around line 400-409: The OpenAPI property definitions for name and statusType
currently list only "type: string" but must reflect the AJV schema's nullable:
true; update the OpenAPI annotation in custom-user-status.ts so the properties
"name" and "statusType" explicitly allow null (e.g., add nullable: true or the
OpenAPI equivalent) while keeping _id required, ensuring the documentation
matches the AJV schema used by the code that validates these fields.
- Around line 84-106: The TypeScript type CustomUserStatusUpdateProps doesn’t
reflect the AJV schema's nullable:true for name and statusType; update the type
declaration for CustomUserStatusUpdateProps so name and statusType are allowed
to be null (e.g., name?: string | null; statusType?: string | null) to match
CustomUserStatusUpdatePropsSchema, then run type checks/build and ensure
isCustomUserStatusUpdateProps usage remains valid.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 10-12: Remove the explanatory Rule 7 comments preceding the import
in this handler file so the implementation remains comment-free; locate the
comment block above the import statement (import { API } from
'../../../../api/server';) in
apps/meteor/app/api/server/v1/custom-user-status.ts and delete those two lines,
leaving only the import and implementation—move any long-form rationale to the
PR description or ADR if needed.
🪄 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: 1886a28e-3d8d-44f7-a514-473708b888af
📒 Files selected for processing (3)
apps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/index.tspackages/rest-typings/src/v1/customUserStatus.ts
💤 Files with no reviewable changes (2)
- packages/rest-typings/src/index.ts
- packages/rest-typings/src/v1/customUserStatus.ts
📜 Review details
🧰 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/custom-user-status.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
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: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
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.
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:26.083Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.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/custom-user-status.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.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/custom-user-status.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-20T13:52:26.083Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:26.083Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.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/custom-user-status.ts
📚 Learning: 2026-03-10T08:13:52.153Z
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.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.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/custom-user-status.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/app/api/server/v1/custom-user-status.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/app/api/server/v1/custom-user-status.ts
🔇 Additional comments (4)
apps/meteor/app/api/server/v1/custom-user-status.ts (4)
280-322: LGTM!The create endpoint migration correctly uses the new AJV schema pattern with appropriate request body validation and response schema. The handler logic is preserved.
356-384: LGTM!The delete endpoint correctly migrates to the new pattern. The
API.v1.success({})return properly results in{ success: true }matching the response schema.
456-462: LGTM on the nullish coalescing fix!Using
??instead of||correctly preserves explicit empty-string inputs fornameandstatusType. This is an intentional behavioral fix as documented in the PR objectives.
478-483: LGTM!The module augmentation correctly re-exposes
CustomUserStatusEndpointsto the globalEndpointsinterface, following the required Rule 7 migration pattern.
Comment on lines
+84
to
+106
| type CustomUserStatusUpdateProps = { _id: string; name?: string; statusType?: string }; | ||
|
|
||
| const CustomUserStatusUpdatePropsSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| _id: { | ||
| type: 'string', | ||
| }, | ||
| name: { | ||
| type: 'string', | ||
| nullable: true, | ||
| }, | ||
| statusType: { | ||
| type: 'string', | ||
| nullable: true, | ||
| }, | ||
| }, | ||
| // Only _id is strictly required for updates; other fields are optional. | ||
| required: ['_id'], | ||
| additionalProperties: false, | ||
| }; | ||
|
|
||
| const isCustomUserStatusUpdateProps = ajv.compile<CustomUserStatusUpdateProps>(CustomUserStatusUpdatePropsSchema); |
Contributor
There was a problem hiding this comment.
TypeScript type doesn't reflect nullable: true in AJV schema.
The AJV schema (lines 93-94, 97-98) allows null for name and statusType via nullable: true, but the TypeScript type on line 84 defines them as optional strings without null. This mismatch means TypeScript won't warn when code assumes these values can't be null.
The handler logic (line 460-461) correctly uses ?? to handle null, so this isn't a runtime bug, but aligning types improves type safety.
🛡️ Proposed fix to align TypeScript type with AJV schema
-type CustomUserStatusUpdateProps = { _id: string; name?: string; statusType?: string };
+type CustomUserStatusUpdateProps = { _id: string; name?: string | null; statusType?: string | null };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 84 - 106,
The TypeScript type CustomUserStatusUpdateProps doesn’t reflect the AJV schema's
nullable:true for name and statusType; update the type declaration for
CustomUserStatusUpdateProps so name and statusType are allowed to be null (e.g.,
name?: string | null; statusType?: string | null) to match
CustomUserStatusUpdatePropsSchema, then run type checks/build and ensure
isCustomUserStatusUpdateProps usage remains valid.
Comment on lines
+400
to
+409
| * properties: | ||
| * _id: | ||
| * type: string | ||
| * name: | ||
| * type: string | ||
| * statusType: | ||
| * type: string | ||
| * required: | ||
| * - _id | ||
| * responses: |
Contributor
There was a problem hiding this comment.
OpenAPI annotation doesn't indicate nullable for name and statusType.
The AJV schema (lines 93-94, 97-98) defines name and statusType with nullable: true, but the OpenAPI annotation here only shows type: string. This will make the API documentation misleading since clients won't know these fields accept null.
📝 Proposed fix to align OpenAPI with AJV schema
_id:
type: string
name:
type: string
+ nullable: true
statusType:
type: string
+ nullable: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 400 - 409,
The OpenAPI property definitions for name and statusType currently list only
"type: string" but must reflect the AJV schema's nullable: true; update the
OpenAPI annotation in custom-user-status.ts so the properties "name" and
"statusType" explicitly allow null (e.g., add nullable: true or the OpenAPI
equivalent) while keeping _id required, ensuring the documentation matches the
AJV schema used by the code that validates these fields.
Contributor
Author
|
Hi team! 👋 I've modernized the custom-user-status API by migrating it to the AJV validation standard and implementing Rule 7 colocation for the typings and schemas. Note for reviewers: I spent most of the day troubleshooting the monorepo build on a Windows environment. I've identified and fixed several path-related issues (e.g., rm -rf and cp compatibility) across 50+ package.json files. To keep this PR atomic and clean, I've stashed those environment fixes and will submit them as a separate 'Windows Compatibility' PR shortly. Looking forward to your feedback! 🚀 |
Member
|
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! 🚀 |
Contributor
Author
|
No problem at all, @ggazzo! I completely understand the need for a mass migration to streamline the review process. 🚀 I'd be happy to help review and test the changes in #39820. Since I've been working deeply with the Windows build environment and AJV structures today, I'll keep a close eye on those aspects specifically. Thanks for the heads-up! |
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
2 participants
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.
Proposed changes (including videos or screenshots)
This PR refactors the
custom-user-statusAPI to adhere to Project Rule 7 by colocating all endpoint typings and AJV schemas directly within the handler file.Key Changes:
packages/rest-typingsintoapps/meteor/app/api/server/v1/custom-user-status.ts.@openapiannotations for.list,.create,.delete, and.updateendpoints, including accurate response schemas.||) with nullish coalescing (??) in theupdatehandler to correctly preserve explicit empty-string inputs fornameandstatusType.deletehandler to return a consistent{ success: true }object as required by the response schema.packages/rest-typings/src/v1/customUserStatus.tsand updated the globalEndpointsinterface via module augmentation.Issue(s)
Relates to Rule 7 refactoring efforts.
Steps to test or reproduce
yarn buildinpackages/rest-typingsto ensure that removing the types didn't break the package build./api/v1/docs(Swagger) once the server is running.updateendpoint with an empty string fornameto confirm it is no longer ignored.Further comments
The Windows compatibility fixes made during development (such as
cross-envandMETEOR_OFFLINE_CATALOG) were not included in this PR to keep it focused on the API refactor.Summary by CodeRabbit