Conversation
…ng in rooms endpoint
… with response schema Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…chema Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… schema Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ith response schema Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ced error handling
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
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 |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
WalkthroughThis pull request refactors API route registrations across chat, DM/IM, and rooms endpoints from standalone Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 @@
## chore/apis #39827 +/- ##
==============================================
- Coverage 70.58% 70.52% -0.07%
==============================================
Files 3256 3256
Lines 115791 115791
Branches 21085 21060 -25
==============================================
- Hits 81727 81657 -70
- Misses 31994 32067 +73
+ Partials 2070 2067 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Migrates rooms.createDiscussion, adminRooms, autocomplete.adminRooms, adminRooms.getRoom, autocomplete.channelAndPrivate, autocomplete.channelAndPrivate.withPagination, autocomplete.availableForTeams, saveRoomSettings to typed endpoints. Also: - Add IRoomAdmin type and Typia schema for admin room projections - Fix RoomsAdminRoomsProps.types to use enum instead of string[] - Fix RoomsSaveRoomSettingsProps.favorite inner fields to be required Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
5b62bca to
13b16f2
Compare
…eric message types
…union type for better type safety
With typed body/query validators, AJV rejects missing required params before the handler, changing the error message format. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…tring arrays When a query param like types=c arrives as a single string, AJV with coerceTypes: 'array' automatically wraps it into ['c'], matching the array schema without needing manual normalization in handlers. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
39b694d to
803b638
Compare
…JV message Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…name optional GET query validators need ajvQuery (coerceTypes) to handle string→number coercion from URL query params. Also make name optional in availableForTeams to match original handler behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…eForTeams Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… adjust related type definitions
With coerceTypes: 'array' on ajvQuery, a single string value is automatically coerced to an array, making this test case no longer applicable. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
With coerceTypes: 'array' on ajvQuery, single values are automatically coerced to arrays, making the oneOf between array and scalar unnecessary. The oneOf was causing validation failures because both branches matched after coercion. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…elds to RoomsSaveRoomSettingsProps These fields are accepted by saveRoomSettings but were missing from the REST type and schema, causing validation failures with additionalProperties: false. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… validation test The ajvQuery instance uses coerceTypes: 'array', which coerces single-element arrays to scalars, making the previous test input valid after coercion. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…msSaveRoomSettingsProps string[] is not assignable to MessageTypesValues[] which caused a TS2345 error when passing params to saveRoomSettings. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…proper handling of messages
…m validation test String values are coerced to arrays by ajvQuery (coerceTypes: 'array'), so 'invalid' becomes ['invalid'] which passes validation. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…rtmentId With ajvQuery coerceTypes: 'array', a string value matches both the string schema and can be coerced to match the array schema, causing oneOf validation to fail with 'must match exactly one schema'. anyOf allows multiple matches which handles coercion correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
4 issues found across 24 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/im.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/im.ts:814">
P2: `if (!msgs)` is unreachable because `toArray()` always returns an array. This dead branch means the "No messages found" error path never runs.</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx:174">
P1: Disabling “Hide System Messages” can stop working because `systemMessages` is now sent as `undefined` instead of an explicit clearing value.</violation>
</file>
<file name="apps/meteor/app/api/server/v1/rooms.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/rooms.ts:755">
P1: `$ref: '#/components/schemas/IRoomAdmin'` references a schema that is never registered with this `ajv` instance via `addSchema()`. This will cause `ajv.compile()` to throw at module load time, or fail validation in test environments. Use an inline object schema instead (similar to the other endpoints in this file).</violation>
<violation number="2" location="apps/meteor/app/api/server/v1/rooms.ts:1220">
P2: Typo: `projections` should be `projection` (singular). MongoDB silently ignores the unrecognized option, so the full user document is fetched instead of just `_id`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ...((data.joinCode || 'joinCodeRequired' in data) && { joinCode: joinCodeRequired ? data.joinCode : '' }), | ||
| ...((data.systemMessages || !hideSysMes) && { | ||
| systemMessages: hideSysMes && data.systemMessages, | ||
| systemMessages: data.systemMessages, |
There was a problem hiding this comment.
P1: Disabling “Hide System Messages” can stop working because systemMessages is now sent as undefined instead of an explicit clearing value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx, line 174:
<comment>Disabling “Hide System Messages” can stop working because `systemMessages` is now sent as `undefined` instead of an explicit clearing value.</comment>
<file context>
@@ -171,7 +171,7 @@ const EditRoomInfo = ({ room, onClickClose, onClickBack }: EditRoomInfoProps) =>
...((data.joinCode || 'joinCodeRequired' in data) && { joinCode: joinCodeRequired ? data.joinCode : '' }),
...((data.systemMessages || !hideSysMes) && {
- systemMessages: hideSysMes && data.systemMessages,
+ systemMessages: data.systemMessages,
}),
retentionEnabled,
</file context>
| systemMessages: data.systemMessages, | |
| systemMessages: hideSysMes ? data.systemMessages : false, |
| response: { | ||
| 200: ajv.compile<Pick<IRoom, RoomAdminFieldsType>>({ | ||
| allOf: [ | ||
| { $ref: '#/components/schemas/IRoomAdmin' }, |
There was a problem hiding this comment.
P1: $ref: '#/components/schemas/IRoomAdmin' references a schema that is never registered with this ajv instance via addSchema(). This will cause ajv.compile() to throw at module load time, or fail validation in test environments. Use an inline object schema instead (similar to the other endpoints in this file).
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/rooms.ts, line 755:
<comment>`$ref: '#/components/schemas/IRoomAdmin'` references a schema that is never registered with this `ajv` instance via `addSchema()`. This will cause `ajv.compile()` to throw at module load time, or fail validation in test environments. Use an inline object schema instead (similar to the other endpoints in this file).</comment>
<file context>
@@ -653,334 +671,466 @@ API.v1.get(
+ response: {
+ 200: ajv.compile<Pick<IRoom, RoomAdminFieldsType>>({
+ allOf: [
+ { $ref: '#/components/schemas/IRoomAdmin' },
+ { type: 'object', properties: { success: { type: 'boolean', enum: [true] } }, required: ['success'] },
+ ],
</file context>
| if (!msgs) { | ||
| throw new Meteor.Error('error-no-messages', 'No messages found'); | ||
| } |
There was a problem hiding this comment.
P2: if (!msgs) is unreachable because toArray() always returns an array. This dead branch means the "No messages found" error path never runs.
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/im.ts, line 814:
<comment>`if (!msgs)` is unreachable because `toArray()` always returns an array. This dead branch means the "No messages found" error path never runs.</comment>
<file context>
@@ -743,6 +743,162 @@ const dmCreateResponseSchema = ajv.compile<{ room: IRoom & { rid: string } }>({
+
+ const [msgs, total] = await Promise.all([cursor.toArray(), totalCount]);
+
+ if (!msgs) {
+ throw new Meteor.Error('error-no-messages', 'No messages found');
+ }
</file context>
| if (!msgs) { | |
| throw new Meteor.Error('error-no-messages', 'No messages found'); | |
| } | |
| if (msgs.length === 0) { | |
| throw new Meteor.Error('error-no-messages', 'No messages found'); | |
| } |
| } | ||
|
|
||
| const user = await Users.findOneById(this.userId, { projections: { _id: 1 } }); | ||
| const user = await Users.findOneById(this.userId, { projections: { _id: 1 } }); |
There was a problem hiding this comment.
P2: Typo: projections should be projection (singular). MongoDB silently ignores the unrecognized option, so the full user document is fetched instead of just _id.
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/rooms.ts, line 1220:
<comment>Typo: `projections` should be `projection` (singular). MongoDB silently ignores the unrecognized option, so the full user document is fetched instead of just `_id`.</comment>
<file context>
@@ -1049,31 +1199,37 @@ API.v1.post(
+ }
- const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });
+ const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });
- if (!user) {
</file context>
| const user = await Users.findOneById(this.userId, { projections: { _id: 1 } }); | |
| const user = await Users.findOneById(this.userId, { projection: { _id: 1 } }); |
…ge parameter
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
…ge parameter
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
API Changes