Conversation
…ole schema Resolves AJV strictTypes warnings: - missing type "object" for keyword "additionalProperties" at "#" - missing type "object" for keyword "properties" at "#" - missing type "object" for keyword "required" at "#/oneOf/0" - missing type "object" for keyword "required" at "#/oneOf/1"
ggazzo
force-pushed
the
fix/strict-types-rooms-schema
branch
from
a4f2e07 to
6f05124
Compare
|
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 |
|
|
WalkthroughRefined the RoomsMembersOrderedByRoleProps JSON schema definition by adding explicit "type": "object" annotation and restructuring the oneOf validation branches to use nested-object form instead of plain required arrays. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39483 +/- ##
===========================================
+ Coverage 70.87% 70.89% +0.01%
===========================================
Files 3196 3196
Lines 113288 113299 +11
Branches 20531 20513 -18
===========================================
+ Hits 80294 80321 +27
+ Misses 30943 30930 -13
+ Partials 2051 2048 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira ARCH-1464 |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rest-typings/src/v1/rooms.ts`:
- Around line 667-670: MembersOrderedByRoleProps currently permits both roomId
and roomName as optional, but the JSON schema enforces exactly one via oneOf;
update the TypeScript type to a discriminated union so compile-time matches
runtime: replace the current MembersOrderedByRoleProps definition with a union
such as { roomId: string; roomName?: never; /* keep other props like X */ } | {
roomName: string; roomId?: never; /* keep other props like X */ }, ensuring any
other existing fields on MembersOrderedByRoleProps are preserved in both union
branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5c18b12-8dfd-42bc-a123-a3ebe09cc832
📒 Files selected for processing (1)
packages/rest-typings/src/v1/rooms.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:
packages/rest-typings/src/v1/rooms.ts
🧠 Learnings (4)
📓 Common learnings
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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
packages/rest-typings/src/v1/rooms.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:
packages/rest-typings/src/v1/rooms.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/rooms.ts
| oneOf: [ | ||
| { type: 'object', required: ['roomId'] }, | ||
| { type: 'object', required: ['roomName'] }, | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
helper="$(fd '^PaginatedRequest\.ts$' packages/rest-typings/src | head -n 1)"
if [ -z "$helper" ]; then
echo "PaginatedRequest.ts not found under packages/rest-typings/src" >&2
exit 1
fi
echo "=== PaginatedRequest definition ==="
sed -n '1,160p' "$helper"
echo
echo "=== membersOrderedByRole types + schema ==="
sed -n '630,675p' packages/rest-typings/src/v1/rooms.tsRepository: RocketChat/Rocket.Chat
Length of output: 1241
Mirror this runtime constraint in the TypeScript type signature.
The oneOf schema now enforces exactly one of roomId or roomName, but MembersOrderedByRoleProps still declares both as optional. This allows invalid payloads like {} or { roomId, roomName } to pass type-checking while failing at runtime validation. Update the type to use a discriminated union:
Suggested type tightening
-type MembersOrderedByRoleProps = {
- roomId?: IRoom['_id'];
- roomName?: IRoom['name'];
- status?: string[];
- filter?: string;
-};
+type MembersOrderedByRoleTarget =
+ | { roomId: IRoom['_id']; roomName?: never }
+ | { roomName: IRoom['name']; roomId?: never };
+
+type MembersOrderedByRoleProps = MembersOrderedByRoleTarget & {
+ status?: string[];
+ filter?: string;
+};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rest-typings/src/v1/rooms.ts` around lines 667 - 670,
MembersOrderedByRoleProps currently permits both roomId and roomName as
optional, but the JSON schema enforces exactly one via oneOf; update the
TypeScript type to a discriminated union so compile-time matches runtime:
replace the current MembersOrderedByRoleProps definition with a union such as {
roomId: string; roomName?: never; /* keep other props like X */ } | { roomName:
string; roomId?: never; /* keep other props like X */ }, ensuring any other
existing fields on MembersOrderedByRoleProps are preserved in both union
branches.
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Task: ARCH-2048
Summary by CodeRabbit