fix: validate team create params#38805
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 |
|
|
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:
WalkthroughAdds a typed TeamsCreateProps schema and AJV validator, applies that validator to the teams.create API route, relaxes team room types to optional/partial, and removes a placeholder TODO in TeamService.create; no runtime logic beyond route validation was added. Changes
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. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/services/team/service.ts (1)
42-75:⚠️ Potential issue | 🟠 MajorValidation block runs after code that already consumes the values it validates — produces wrong or misleading errors
The validation block at lines 61–75 is placed after three existing DB-calling checks (lines 43–57) that already consume
team.nameandteam.type. This has two concrete consequences:
- Empty
team.name:checkUsernameAvailability("")runs first (line 43). If it returnsfalse(which is plausible for an empty string), the caller receivesteam-name-already-existsinstead of the intendedinvalid-team-name. The validation fires too late to control the error path.- Invalid
team.type: All three DB calls at lines 43–57 execute beforeinvalid-team-typeis thrown at line 74 — wasted round-trips for an easily detectable bad input.Additionally, the
!team || typeof team !== 'object'guard at line 61 is unreachable dead code:
- Common path (no explicit
room): The function parameter defaultroom = { name: team.name, extraData: {} }is evaluated at the destructuring site — before the function body runs — and throws aTypeErrorifteamisnull/undefined.roomexplicitly provided: Line 43 (checkUsernameAvailability(team.name)) throws first.The entire validation block should be moved to the top of the method (before line 43), and the now-redundant
!team || typeof team !== 'object'check can be dropped.🔀 Proposed fix — move validations before the first DB call
async create(uid: string, { team, room = { name: team.name, extraData: {} }, members, owner }: ITeamCreateParams): Promise<ITeam> { + if (!team.name || typeof team.name !== 'string' || team.name.trim().length === 0) { + throw new Error('invalid-team-name'); + } + if (team.name.length > 255) { + throw new Error('team-name-too-long'); + } + if (team.type !== TeamType.PRIVATE && team.type !== TeamType.PUBLIC) { + throw new Error('invalid-team-type'); + } + if (members !== undefined && members !== null) { + if (!Array.isArray(members)) { + throw new Error('invalid-members-format'); + } + for (const member of members) { + if (typeof member !== 'string' || member.trim().length === 0) { + throw new Error('invalid-member-id'); + } + } + const uniqueMembers = new Set(members); + if (uniqueMembers.size !== members.length) { + throw new Error('duplicate-members'); + } + } + if (owner !== undefined && owner !== null) { + if (typeof owner !== 'string' || owner.trim().length === 0) { + throw new Error('invalid-owner-id'); + } + } + if (!room || typeof room !== 'object') { + throw new Error('invalid-room-data'); + } if (!(await checkUsernameAvailability(team.name))) { throw new Error('team-name-already-exists'); }Then remove the entire displaced block (lines 59–106 in the current file), including the stale TODO comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` around lines 42 - 75, The validation for incoming team data must run before any DB calls in create to avoid misleading/errors and wasted queries: move all checks that validate team.name and team.type (the blocks checking empty/trim, length >255, and TeamType enum) to the very top of async create(uid, { team, room = { name: team.name, extraData: {} }, members, owner }) so they execute before checkUsernameAvailability, Rooms.findOneByName and Users.findOneById; remove the unreachable guard (!team || typeof team !== 'object') and the stale TODO comment about validations since those checks are now handled up-front.
🧹 Nitpick comments (1)
apps/meteor/server/services/team/service.ts (1)
65-71: Trimming inconsistency between the empty check and the length-cap checkLine 65 detects an empty name using
team.name.trim().length === 0(trimmed), but line 69 caps the name length usingteam.name.length > 255(raw, untrimmed). A name consisting of one real character padded with 300 spaces would pass the empty check yet fail the length cap — which is counterintuitive. Applying.trim()consistently to both checks avoids this edge case.♻️ Proposed fix
-if (team.name.length > 255) { +if (team.name.trim().length > 255) { throw new Error('team-name-too-long'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` around lines 65 - 71, The validation is inconsistent: the empty-name check uses team.name.trim() but the length cap uses the raw team.name; to fix, create a trimmedName = team.name.trim() (or reuse team.name = team.name.trim()) and use trimmedName for both checks (the empty check and the length check) and for any subsequent name usage in the same function so both validations and stored value are consistent; update the checks around team.name (the validation block that throws 'invalid-team-name' and 'team-name-too-long') to reference trimmedName.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/server/services/team/service.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/server/services/team/service.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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/server/services/team/service.ts (1)
59-60:⚠️ Potential issue | 🟡 MinorStale TODO comment + new section-header comments violate the "no implementation comments" guideline
- Line 59:
// TODO add validations to \data` and `members`` — the PR addresses this TODO; the comment should be removed.- Lines 60, 77, 89, 96, 103: The newly added section-header comments (
// Validate team data,// Validate members array, etc.) should be removed.As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 77-77, 89-89, 96-96, 103-103
⛔ Skipped due to learnings
Learnt from: MartinSchoeler Repo: RocketChat/Rocket.Chat PR: 37408 File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69 Timestamp: 2025-11-10T19:06:20.146Z Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.Learnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
🤖 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/server/services/team/service.ts`:
- Around line 104-106: The current room guard (if (room && typeof room !==
'object')) lets null slip through; update the check in the team creation flow to
explicitly reject nulls—e.g., change to if (room === null || typeof room !==
'object') throw new Error('invalid-room-data')—so that subsequent use of room
(like accessing room.id in the create team function) triggers the intended
invalid-room-data error rather than a swallowed TypeError; locate the guard
around the room validation in apps/meteor/server/services/team/service.ts (the
block that currently throws 'invalid-room-data' and precedes use of room.id).
---
Outside diff comments:
In `@apps/meteor/server/services/team/service.ts`:
- Around line 42-75: The validation for incoming team data must run before any
DB calls in create to avoid misleading/errors and wasted queries: move all
checks that validate team.name and team.type (the blocks checking empty/trim,
length >255, and TeamType enum) to the very top of async create(uid, { team,
room = { name: team.name, extraData: {} }, members, owner }) so they execute
before checkUsernameAvailability, Rooms.findOneByName and Users.findOneById;
remove the unreachable guard (!team || typeof team !== 'object') and the stale
TODO comment about validations since those checks are now handled up-front.
---
Nitpick comments:
In `@apps/meteor/server/services/team/service.ts`:
- Around line 65-71: The validation is inconsistent: the empty-name check uses
team.name.trim() but the length cap uses the raw team.name; to fix, create a
trimmedName = team.name.trim() (or reuse team.name = team.name.trim()) and use
trimmedName for both checks (the empty check and the length check) and for any
subsequent name usage in the same function so both validations and stored value
are consistent; update the checks around team.name (the validation block that
throws 'invalid-team-name' and 'team-name-too-long') to reference trimmedName.
mdawoud27
force-pushed
the
fix/team-service-params-validation
branch
from
974aaf0 to
87d92b8
Compare
mdawoud27
force-pushed
the
fix/team-service-params-validation
branch
2 times, most recently
from
6791fc0 to
01458e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/server/services/team/service.ts (2)
60-106: Remove section-dividing comments per coding guidelines.
// Validate team data,// Validate members array,// Check for duplicate members,// Validate owner if provided, and// Validate room data if providedare all implementation comments and violate the project rule.♻️ Proposed fix
- // Validate team data if (!team || typeof team !== 'object') { ... - // Validate members array if (members !== undefined && members !== null) { ... - // Check for duplicate members const uniqueMembers = new Set(members); ... - // Validate owner if provided if (owner !== undefined && owner !== null) { ... - // Validate room data if provided if (!room || typeof room !== 'object') {As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` around lines 60 - 106, Remove the implementation comments that divide validation blocks in the team service (the lines containing "// Validate team data", "// Validate members array", "// Check for duplicate members", "// Validate owner if provided", and "// Validate room data if provided") and leave the existing validation logic for the variables team, members, owner, room and the TeamType checks intact; ensure no other inline or section-dividing comments remain around the validations using variables team, members, owner, room and the TeamType enum so the code complies with the project's "avoid code comments in implementation" guideline.
59-59: Remove the now-stale TODO comment.Line 59 (
// TODO add validations to \data` and `members``) predates this PR and is superseded by the validation block added directly below it.♻️ Proposed fix
- // TODO add validations to `data` and `members`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` at line 59, Remove the stale inline comment "// TODO add validations to `data` and `members`" found in the team service implementation (near the createTeam / TeamService method) — the validation block directly below already covers this, so delete that single TODO line to avoid confusion and keep the code clean.apps/meteor/server/startup/migrations/v336.ts (1)
7-7: Remove inline comment per coding guidelines.The comment
// This is a stub migration to allow the app to boot when the DB is ahead of the codebase.is self-evident from thenamefield on line 5 and violates the "Avoid code comments in the implementation" rule.♻️ Proposed fix
async up() { - // This is a stub migration to allow the app to boot when the DB is ahead of the codebase. },As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/startup/migrations/v336.ts` at line 7, Remove the inline implementation comment in apps/meteor/server/startup/migrations/v336.ts (the comment that begins "// This is a stub migration...") so the file only relies on the migration object's name field to convey intent; locate the migration export (reference the migration object's name property) and delete that comment (and any other implementation comments) to comply with the "Avoid code comments in the implementation" guideline.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/server/services/team/service.tsapps/meteor/server/startup/migrations/index.tsapps/meteor/server/startup/migrations/v336.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/server/startup/migrations/v336.tsapps/meteor/server/startup/migrations/index.tsapps/meteor/server/services/team/service.ts
🧠 Learnings (6)
📚 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/server/startup/migrations/v336.tsapps/meteor/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/services/team/service.ts
📚 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:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
🧬 Code graph analysis (1)
apps/meteor/server/startup/migrations/v336.ts (1)
apps/meteor/server/lib/migrations.ts (1)
addMigration(99-107)
🔇 Additional comments (1)
apps/meteor/server/startup/migrations/index.ts (1)
44-44: LGTM. Import is correctly sequenced afterv335.
🤖 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/server/services/team/service.ts`:
- Around line 60-106: The validation block for team/members/owner/room must be
moved so it runs before any dereference of team or room; relocate the entire
block starting with the team null/object checks (the validations you added that
throw 'invalid-team-data', 'invalid-team-name', etc.) to immediately before the
first use of team.name and room.id (i.e. before the call to
checkUsernameAvailability(team.name) and before calling Rooms.findOneByName),
ensuring functions like checkUsernameAvailability and Rooms.findOneByName
operate only on already-validated team and room values.
---
Nitpick comments:
In `@apps/meteor/server/services/team/service.ts`:
- Around line 60-106: Remove the implementation comments that divide validation
blocks in the team service (the lines containing "// Validate team data", "//
Validate members array", "// Check for duplicate members", "// Validate owner if
provided", and "// Validate room data if provided") and leave the existing
validation logic for the variables team, members, owner, room and the TeamType
checks intact; ensure no other inline or section-dividing comments remain around
the validations using variables team, members, owner, room and the TeamType enum
so the code complies with the project's "avoid code comments in implementation"
guideline.
- Line 59: Remove the stale inline comment "// TODO add validations to `data`
and `members`" found in the team service implementation (near the createTeam /
TeamService method) — the validation block directly below already covers this,
so delete that single TODO line to avoid confusion and keep the code clean.
In `@apps/meteor/server/startup/migrations/v336.ts`:
- Line 7: Remove the inline implementation comment in
apps/meteor/server/startup/migrations/v336.ts (the comment that begins "// This
is a stub migration...") so the file only relies on the migration object's name
field to convey intent; locate the migration export (reference the migration
object's name property) and delete that comment (and any other implementation
comments) to comply with the "Avoid code comments in the implementation"
guideline.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/server/services/team/service.ts (2)
60-103: Remove inline section comments per project guidelines.Lines 60, 77, 89, 96, and 103 add explanatory comments (
// Validate team data,// Validate members array, etc.) that the surrounding code already makes self-evident. As per coding guidelines, avoid code comments in the implementation for.tsfiles.♻️ Proposed cleanup
- // Validate team data if (!team || typeof team !== 'object') { - // Validate members array if (members !== undefined && members !== null) { - // Check for duplicate members const uniqueMembers = new Set(members); - // Validate owner if provided if (owner !== undefined && owner !== null) { - // Validate room data if provided - if (!room && typeof room !== 'object') { + if (!room || typeof room !== 'object') {As per coding guidelines: "Avoid code comments in the implementation" (
**/*.{ts,tsx,js}).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` around lines 60 - 103, Remove the inline section comments that duplicate obvious intent and violate the guideline: delete the comment lines like "// Validate team data", "// Validate members array", "// Check for duplicate members", "// Validate owner if provided", and "// Validate room data" while leaving the surrounding validation logic (the checks using team, members, owner, TeamType, uniqueMembers, etc.) unchanged; ensure no other implementation comments remain in this .ts file.
59-59: Remove the stale TODO comment.The validation for
dataandmembersis now in place, making this comment obsolete.- // TODO add validations to `data` and `members`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` at line 59, Remove the stale TODO comment "// TODO add validations to `data` and `members`" from apps/meteor/server/services/team/service.ts since validation is implemented; locate the comment in the top of the team service (the block containing the create/update team logic or functions handling `data` and `members`) and delete that single-line TODO so the codebase no longer contains obsolete comments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/server/services/team/service.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/server/services/team/service.ts
🧠 Learnings (6)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/services/team/service.ts
📚 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:
apps/meteor/server/services/team/service.ts
📚 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/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
🤖 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/server/services/team/service.ts`:
- Around line 104-106: The room validation guard currently uses "if (!room &&
typeof room !== 'object')" which lets null and non-object truthy values slip
through; update that check to use logical OR so it reads "if (!room || typeof
room !== 'object')" to throw for null, undefined, or non-object values—modify
the guard surrounding the room variable in the team service (the if that throws
'invalid-room-data') accordingly.
- Around line 42-106: In the create method of the Team service (async
create(uid: string, { team, room = { name: team.name, ... }, members, owner }:
ITeamCreateParams): Promise<ITeam>), hoist all input validation for team, room,
members, and owner to the very top of the function body before any access to
team.name or room.id; remove the default room value that references team.name
from the parameter list (e.g., make room optional/undefined), validate that team
is a non-null object and team.name is a non-empty string first, then after
validation initialize room = room ?? { name: team.name, extraData: {} } and
continue with calls to checkUsernameAvailability and Rooms.findOneByName. Ensure
duplicate-members check and owner format checks remain in the moved validation
block and update any early uses (checkUsernameAvailability, Rooms.findOneByName)
to operate on the validated team and room values.
---
Nitpick comments:
In `@apps/meteor/server/services/team/service.ts`:
- Around line 60-103: Remove the inline section comments that duplicate obvious
intent and violate the guideline: delete the comment lines like "// Validate
team data", "// Validate members array", "// Check for duplicate members", "//
Validate owner if provided", and "// Validate room data" while leaving the
surrounding validation logic (the checks using team, members, owner, TeamType,
uniqueMembers, etc.) unchanged; ensure no other implementation comments remain
in this .ts file.
- Line 59: Remove the stale TODO comment "// TODO add validations to `data` and
`members`" from apps/meteor/server/services/team/service.ts since validation is
implemented; locate the comment in the top of the team service (the block
containing the create/update team logic or functions handling `data` and
`members`) and delete that single-line TODO so the codebase no longer contains
obsolete comments.
01458e2 to
e71e2a0
Compare
| export class TeamService extends ServiceClassInternal implements ITeamService { | ||
| protected name = 'team'; | ||
|
|
||
| async create(uid: string, { team, room = { name: team.name, extraData: {} }, members, owner }: ITeamCreateParams): Promise<ITeam> { |
There was a problem hiding this comment.
Instead of doing the validation here, wdyt of validating on the callers? For example, the endpoint handlers that call this func should have AJV validators, we can make sure those validations are there and are good, or improve them so it's safer.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (2)
8-8:room?: anydefeats the purpose of adding typed validation.The AJV schema carefully restricts room properties (lines 33–67), but the TypeScript type uses
any, so callers get no compile-time safety. Define a proper type matching the schema shape.♻️ Proposed fix
+type TeamsCreateRoomProps = { + id?: string | null; + name?: string | null; + members?: string[] | null; + readOnly?: boolean | null; + extraData?: Record<string, unknown> | null; + options?: Record<string, unknown> | null; +}; + export type TeamsCreateProps = { name: string; type: TeamType; members?: string[]; - room?: any; + room?: TeamsCreateRoomProps | null; owner?: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rest-typings/src/v1/teams/TeamsCreateProps.ts` at line 8, The room property currently typed as "any" removes compile-time guarantees; replace it with a concrete Room interface/type that mirrors the AJV schema (the object shape validated in lines 33–67) and update TeamsCreateProps to use that type instead of any; add an exported type name like Room or TeamsCreateRoom that includes the exact optional/required fields and nested types from the schema so callers get proper TypeScript validation and IDE hints (update references to room in the file to use the new Room/TeamsCreateRoom type).
1-2: Non-standard import ordering: external package import should precede local imports.Suggested fix
-import { ajv } from '../Ajv'; -import { TeamType } from '@rocket.chat/core-typings'; +import { TeamType } from '@rocket.chat/core-typings'; + +import { ajv } from '../Ajv';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rest-typings/src/v1/teams/TeamsCreateProps.ts` around lines 1 - 2, Reorder the imports in TeamsCreateProps.ts so external package imports come first: move the import of TeamType from '@rocket.chat/core-typings' above the local import of ajv from '../Ajv' (i.e., ensure "import { TeamType } from '@rocket.chat/core-typings';" appears before "import { ajv } from '../Ajv';").
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/meteor/app/api/server/v1/teams.tsapps/meteor/server/services/team/service.tspackages/core-services/src/types/ITeamService.tspackages/rest-typings/src/v1/teams/TeamsCreateProps.tspackages/rest-typings/src/v1/teams/index.ts
💤 Files with no reviewable changes (1)
- apps/meteor/server/services/team/service.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:
packages/core-services/src/types/ITeamService.tspackages/rest-typings/src/v1/teams/index.tsapps/meteor/app/api/server/v1/teams.tspackages/rest-typings/src/v1/teams/TeamsCreateProps.ts
🧠 Learnings (2)
📚 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/teams.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/api/server/v1/teams.ts
🧬 Code graph analysis (4)
packages/core-services/src/types/ITeamService.ts (1)
packages/core-typings/src/ITeam.ts (1)
ITeam(14-20)
packages/rest-typings/src/v1/teams/index.ts (1)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (1)
TeamsCreateProps(4-10)
apps/meteor/app/api/server/v1/teams.ts (2)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (2)
isTeamsCreateProps(79-79)TeamsCreateProps(4-10)packages/rest-typings/src/v1/teams/index.ts (2)
isTeamsCreateProps(17-17)TeamsCreateProps(17-17)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (2)
packages/rest-typings/src/v1/teams/index.ts (2)
TeamsCreateProps(17-17)isTeamsCreateProps(17-17)packages/rest-typings/src/v1/Ajv.ts (1)
ajv(24-24)
🔇 Additional comments (4)
packages/rest-typings/src/v1/teams/index.ts (1)
4-4: LGTM!The re-exports and endpoint signature update correctly wire
TeamsCreatePropsandisTeamsCreatePropsinto the public API surface, consistent with the new validation schema.Also applies to: 17-17, 65-65
apps/meteor/app/api/server/v1/teams.ts (2)
68-88: Validation wiring looks correct.The
validateParams+as TeamsCreatePropspattern is consistent with other endpoints in this file (e.g.,isTeamsConvertToChannelProps,isTeamsDeleteProps). The AJV validator guarantees the shape before the handler runs.
73-83: > Likely an incorrect or invalid review comment.packages/core-services/src/types/ITeamService.ts (1)
18-24: The contract relaxation is safely handled by implementation defaults.Making
ITeamCreateRoomfields partial (all now optional) is intentional and safe. The main consumerTeamService.create()provides a default parameter value (room = { name: team.name, extraData: {} }) that ensures sensible defaults are applied whenroomis undefined. When the room object is used internally, required properties liketype,name, andmembersare explicitly set before being passed toRoom.create(), regardless of what fields were optionally provided inITeamCreateRoom. Existing callers via the API only pass team metadata (name/type) without aroomparameter, confirming this pattern is already in use.
🤖 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/teams/TeamsCreateProps.ts`:
- Around line 4-10: The TeamsCreateProps type doesn't reflect schema-nullable
fields: update the optional properties to allow null as well so
runtime-validated objects match TypeScript. Specifically, change members?:
string[] to members?: string[] | null, change room?: any to room?: any | null,
and change owner?: string to owner?: string | null in the TeamsCreateProps type
so members, room and owner can be undefined or null per the schema.
- Around line 24-32: The AJV instance currently ignores the "nullable" keyword
causing fields like members, room, owner (seen in TeamsCreateProps.ts) to reject
nulls; update the AJV setup in packages/rest-typings/src/v1/Ajv.ts to explicitly
support "nullable" (e.g., register a custom keyword that, when nullable: true,
treats the schema as allowing "null" in the type), or alternatively enable AJV
OpenAPI mode if available, or change affected schemas (e.g., members in
TeamsCreateProps.ts) to use type: ['string','null'] instead of nullable: true;
implement one of these fixes and ensure the shared AJV instance (the exported
Ajv instance/creator in Ajv.ts) is used everywhere so null acceptance is
consistent.
---
Nitpick comments:
In `@packages/rest-typings/src/v1/teams/TeamsCreateProps.ts`:
- Line 8: The room property currently typed as "any" removes compile-time
guarantees; replace it with a concrete Room interface/type that mirrors the AJV
schema (the object shape validated in lines 33–67) and update TeamsCreateProps
to use that type instead of any; add an exported type name like Room or
TeamsCreateRoom that includes the exact optional/required fields and nested
types from the schema so callers get proper TypeScript validation and IDE hints
(update references to room in the file to use the new Room/TeamsCreateRoom
type).
- Around line 1-2: Reorder the imports in TeamsCreateProps.ts so external
package imports come first: move the import of TeamType from
'@rocket.chat/core-typings' above the local import of ajv from '../Ajv' (i.e.,
ensure "import { TeamType } from '@rocket.chat/core-typings';" appears before
"import { ajv } from '../Ajv';").
7e533ab to
9991ab2
Compare
|
@KevLehman I’ve addressed the requested changes and pushed the updates. Please let me know if there’s anything else I should update. |
packages/rest-typings/src/v1/Ajv.ts
Outdated
| type: 'string', | ||
| validate: (_schema: unknown, data: unknown): boolean => typeof data === 'string' && !!data.trim(), | ||
| }); | ||
| ajv.addKeyword({ |
There was a problem hiding this comment.
nullable is part of the schema spec we use.
| name: { | ||
| type: 'string', | ||
| minLength: 1, | ||
| maxLength: 255, |
There was a problem hiding this comment.
| maxLength: 255, |
this would be a breaking change.
| minLength: 1, | ||
| }, | ||
| uniqueItems: true, | ||
| nullable: true, |
There was a problem hiding this comment.
members cannot be null, it's enough having it as not required (same for other optional props)
| additionalProperties: true, | ||
| nullable: true, | ||
| }, | ||
| options: { |
There was a problem hiding this comment.
on the types, options is a property with some optional keys inside. Please make sure to validate those
mdawoud27
force-pushed
the
fix/team-service-params-validation
branch
from
9991ab2 to
123e8d2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (6)
apps/meteor/app/api/server/v1/teams.tsapps/meteor/server/services/team/service.tspackages/core-services/src/types/ITeamService.tspackages/rest-typings/src/v1/Ajv.tspackages/rest-typings/src/v1/teams/TeamsCreateProps.tspackages/rest-typings/src/v1/teams/index.ts
💤 Files with no reviewable changes (1)
- apps/meteor/server/services/team/service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/api/server/v1/teams.ts
- packages/core-services/src/types/ITeamService.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:
packages/rest-typings/src/v1/Ajv.tspackages/rest-typings/src/v1/teams/index.tspackages/rest-typings/src/v1/teams/TeamsCreateProps.ts
🧠 Learnings (3)
📚 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:
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts
📚 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/teams/TeamsCreateProps.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts
🧬 Code graph analysis (2)
packages/rest-typings/src/v1/teams/index.ts (1)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (1)
TeamsCreateProps(4-26)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (2)
packages/rest-typings/src/v1/teams/index.ts (2)
TeamsCreateProps(17-17)isTeamsCreateProps(17-17)packages/rest-typings/src/v1/Ajv.ts (1)
ajv(36-36)
🔇 Additional comments (2)
packages/rest-typings/src/v1/teams/TeamsCreateProps.ts (1)
4-26: TypeScript type is consistent with the schema — the previous| nullconcern is no longer applicable.
members,room, andownerare optional (absent fromrequired) but the schema does not usenullable: trueanywhere, sonullvalues are correctly rejected by AJV. The TypeScript type (members?: string[], etc.) accurately reflects that these fields may be absent (undefined) but nevernull. The past review comment about missing| nullwas predicated on these fields usingnullable: truein the schema, which they do not.packages/rest-typings/src/v1/teams/index.ts (1)
4-5: LGTM.The named
exporton line 17 (rather thanexport *) is the right choice — it explicitly surfacesisTeamsCreatePropsas a value export alongside theTeamsCreatePropstype, and avoids re-exporting the internalteamsCreatePropsSchema. The endpoint signature change on lines 64–67 correctly adoptsTeamsCreatePropsas the canonical param type.Also applies to: 17-17, 64-67
🤖 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/Ajv.ts`:
- Around line 24-35: Remove the custom nullable registration by deleting the
ajv.addKeyword({ keyword: 'nullable', macro: ... }) block in Ajv.ts so Ajv's
native nullable handling is used; ensure no other code depends on that macro
(search for references to the custom macro or its behavior) and run tests to
confirm validation/error messages still behave as expected.
In `@packages/rest-typings/src/v1/teams/TeamsCreateProps.ts`:
- Around line 31-34: The name property in the TeamsCreateProps schema currently
only enforces minLength and lacks a maxLength, so overly long team names bypass
AJV and the "team-name-too-long" error path is unreachable; update the exported
schema (the name field in TeamsCreateProps) to include maxLength: 255 alongside
minLength: 1 so AJV validation will reject names >255 chars and allow the
team-name-too-long branch to run.
- Around line 65-68: The JSON schema for extraData is too permissive
(additionalProperties: true) compared to the TS type Record<string, string |
boolean>; update the extraData schema so each property value is restricted to
string or boolean (e.g. replace additionalProperties: true with
additionalProperties: { type: ["string","boolean"] } or an equivalent anyOf of
string/boolean) to match the TypeScript type and ensure AJV validation rejects
numbers/objects/arrays.
packages/rest-typings/src/v1/Ajv.ts
Outdated
| ajv.addKeyword({ | ||
| keyword: 'nullable', | ||
| macro: (schema: any, parentSchema: any) => { | ||
| if (schema !== true || parentSchema.anyOf) { | ||
| return {}; | ||
| } | ||
| const { nullable: _, ...rest } = parentSchema; | ||
| return { | ||
| anyOf: [{ type: 'null' }, rest], | ||
| }; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/rest-typings/package.json | grep -A 2 -B 2 ajvRepository: RocketChat/Rocket.Chat
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Check if node_modules/ajv exists and find nullable references
if [ -d "node_modules/ajv" ]; then
echo "=== AJV package found ==="
cat node_modules/ajv/package.json | grep -E '"version"'
echo ""
echo "=== Searching for 'nullable' in AJV dist ==="
find node_modules/ajv -name "*.js" -o -name "*.json" | xargs grep -l "nullable" 2>/dev/null | head -10
echo ""
echo "=== Checking for native nullable keyword registration ==="
find node_modules/ajv -name "*.js" | xargs grep -n "addKeyword.*nullable\|keywords.*nullable" 2>/dev/null | head -20
else
echo "node_modules/ajv not found - checking project structure"
ls -la packages/rest-typings/ | head -20
fiRepository: RocketChat/Rocket.Chat
Length of output: 613
🏁 Script executed:
#!/bin/bash
# Look at AJV v8 source to understand how OpenAPI keywords like nullable are handled
find node_modules/ajv -name "*.json" | xargs grep -l "openapi\|nullable" 2>/dev/nullRepository: RocketChat/Rocket.Chat
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check the actual Ajv.ts file to understand the context
cat -n packages/rest-typings/src/v1/Ajv.tsRepository: RocketChat/Rocket.Chat
Length of output: 3858
🌐 Web query:
AJV v8.17.1 nullable keyword support OpenAPI
💡 Result:
- Ajv v8.17.1 supports the OpenAPI 3.0
nullablekeyword out of the box (no plugin/option needed). Ajv treatsnullable: trueas allowingnullin addition to the schema’stype. [1] - Caveat:
nullable: truedoes not automatically extendenum/constto includenull; you must explicitly addnulltoenum(andconstwon’t match unless it’snull). [1] - OpenAPI 3.1 difference: OAS 3.1 effectively moves to standard JSON Schema semantics where nullability is expressed via JSON Schema types (e.g.,
type: ["string","null"]), andnullableis no longer the preferred mechanism. [5]
Examples
OpenAPI 3.0 (works with Ajv nullable)
{ "type": "string", "nullable": true }OpenAPI 3.1 style
{ "type": ["string", "null"] }Sources: Ajv OpenAPI keyword docs (nullable/discriminator) [1], OpenAPI Initiative discussion on dropping nullable for 3.1 [5].
🌐 Web query:
AJV v8 native nullable implementation vs custom addKeyword macro
💡 Result:
Ajv v8 “native” nullable (OpenAPI extension)
Ajv v8 supports the OpenAPI nullable keyword out of the box. (ajv.js.org)
In OpenAPI 3.0, this exists because type must be a single value (not an array) and null isn’t a valid type; nullable: true is the official workaround. (swagger.io)
What you get
- No schema rewriting: Ajv handles it during compilation/validation. (ajv.js.org)
- Typically best performance & simplest error paths, because it’s implemented internally rather than as userland code.
- Intended semantics: “this schema also accepts
null”, without forcing you into verboseanyOfeverywhere.
Tradeoff
nullableis not JSON Schema standard; it’s an OpenAPI-specific extension Ajv recognizes. (ajv.js.org)
(So other JSON Schema validators may ignore it unless they also added OpenAPI support.)
Custom addKeyword({ macro }) to implement “nullable”
A macro keyword typically rewrites:
nullable: true+type: "string"
into something like:anyOf: [{ type: "string" }, { type: "null" }]
(ortype: ["string","null"]depending on your style)
What you get
- Portable JSON Schema output (if you expand to standard constructs like
anyOfortype: ["…","null"]), which can be useful if you must run schemas through multiple validators/tools that don’t knownullable. - Control over edge cases (e.g., how you want
nullableto interact withenum, defaults, etc.).
Tradeoffs
- More moving parts: you must maintain the transformation rules and ensure they match your intended semantics.
- Errors can get noisier (e.g.,
anyOferror reporting), because you’ve introduced composition. - If you expand to multi-type
type: [...], be aware Ajv strictness: Ajv’sstrictTypesdiscourages union types in general, though unions includingnullare specifically called out as an exception (and you can also allow unions viaallowUnionTypes). (ajv.js.org)
Practical recommendation
- If you’re validating OpenAPI 3.0-ish schemas with Ajv, prefer Ajv’s built-in
nullablefor simplicity and cleaner validation. (ajv.js.org) - If you need validator/tool portability (or you’re normalizing schemas into “pure JSON Schema”), use a macro to expand
nullableinto standard JSON Schema (usuallyanyOfwith{type:"null"}), and accept the composition/error-reporting tradeoffs.
If you share your exact target (OpenAPI 3.0 vs 3.1, whether you emit JSON Schema for other tools, and your Ajv strict* options), I can recommend the cleanest expansion pattern.
Citations:
- 1: https://ajv.js.org/v6-to-v8-migration.html?utm_source=openai
- 2: https://swagger.io/docs/specification/v3_0/data-models/keywords/?utm_source=openai
- 3: https://ajv.js.org/v6-to-v8-migration.html?utm_source=openai
- 4: https://ajv.js.org/json-schema.html?utm_source=openai
- 5: https://ajv.js.org/strict-mode.html?utm_source=openai
- 6: https://ajv.js.org/v6-to-v8-migration.html?utm_source=openai
🌐 Web query:
AJV v8 addKeyword duplicate keyword conflict strict mode
💡 Result:
In Ajv v8, there are two common (but different) failure modes that often get conflated:
1) “duplicate keyword” / “Keyword X is already defined”
This happens when the same Ajv instance tries to register a keyword name more than once (e.g., you call a plugin twice, hot-reload runs init twice, or you both pass keywords: [...] in options and call addKeyword for the same name).
Fix options
- Ensure the registration runs only once per Ajv instance.
- Guard the registration:
if (!ajv.getKeyword("myKeyword")) ajv.addKeyword({ keyword: "myKeyword", /* ... */ })- Or explicitly replace it (if you really intend to redefine):
ajv.removeKeyword("myKeyword")
ajv.addKeyword({ keyword: "myKeyword", /* ... */ })Ajv exposes getKeyword and removeKeyword for this purpose. [1]
2) strict mode: unknown keyword: "X"
This is strict schema behavior: by default Ajv v8 treats unknown schema keywords as an error (instead of silently ignoring them), to prevent typos / ambiguous schemas. [2][3]
Fix options
- Actually add/support the keyword (via
addKeyword/ plugin /addVocabulary). Strict mode explicitly recommends defining allowed custom keywords rather than relying on them being ignored. [2] - Or relax strictness if you must:
new Ajv({ strict: false })(turns off all strict restrictions), ornew Ajv({ strictSchema: false })(targets “unknown keywords” behavior). [3]
Note on v8 addKeyword API
In v8, the recommended form is addKeyword({ keyword: "name", ... }) (keyword name is in the definition object). [4]
Sources:
[1] Ajv API (getKeyword, removeKeyword) https://ajv.js.org/api.html
[2] Ajv Strict mode (unknown keywords behavior) https://ajv.js.org/strict-mode.html
[3] Ajv Options (strict, strictSchema) https://ajv.js.org/options
[4] Ajv v6 → v8 migration notes (addKeyword API change) https://ajv.js.org/v6-to-v8-migration.html
Remove the custom nullable macro—AJV v8.17.1 natively supports nullable out of the box.
Ajv v8.17.1 has built-in support for the OpenAPI nullable keyword; it handles nullable: true during validation without requiring a custom macro. The call to ajv.addKeyword({ keyword: 'nullable', ... }) at lines 24–35 will either fail with a "duplicate keyword" error (if Ajv enforces strict registration) or silently shadow the correct native implementation with this custom macro.
Native nullable is superior: Ajv handles it internally with better performance and cleaner error messages, whereas this custom macro expands it into verbose anyOf compositions with noisier error reporting. Remove the custom addKeyword call entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rest-typings/src/v1/Ajv.ts` around lines 24 - 35, Remove the custom
nullable registration by deleting the ajv.addKeyword({ keyword: 'nullable',
macro: ... }) block in Ajv.ts so Ajv's native nullable handling is used; ensure
no other code depends on that macro (search for references to the custom macro
or its behavior) and run tests to confirm validation/error messages still behave
as expected.
| name: { | ||
| type: 'string', | ||
| minLength: 1, | ||
| }, |
There was a problem hiding this comment.
Missing maxLength: 255 on name — the team-name-too-long error path is unreachable.
The PR description explicitly states that team.name must have "a maximum length of 255 characters" and calls out team-name-too-long as a distinct error code. The schema only enforces minLength: 1, so over-length names will pass AJV validation and the team-name-too-long path can never be triggered from this validator.
🐛 Proposed fix
name: {
type: 'string',
minLength: 1,
+ maxLength: 255,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: { | |
| type: 'string', | |
| minLength: 1, | |
| }, | |
| name: { | |
| type: 'string', | |
| minLength: 1, | |
| maxLength: 255, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rest-typings/src/v1/teams/TeamsCreateProps.ts` around lines 31 - 34,
The name property in the TeamsCreateProps schema currently only enforces
minLength and lacks a maxLength, so overly long team names bypass AJV and the
"team-name-too-long" error path is unreachable; update the exported schema (the
name field in TeamsCreateProps) to include maxLength: 255 alongside minLength: 1
so AJV validation will reject names >255 chars and allow the team-name-too-long
branch to run.
e9ec6b5 to
e3724e2
Compare
![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 1 file (changes from recent commits).
Prompt for AI agents (all 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="packages/rest-typings/src/v1/teams/TeamsCreateProps.ts">
<violation number="1" location="packages/rest-typings/src/v1/teams/TeamsCreateProps.ts:66">
P2: Duplicated `extraData` block nests `extraData` inside itself, changing the validation shape for `room.extraData` and likely rejecting valid payloads.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Friendly ping @KevLehman could you please review the latest changes? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38805 +/- ##
===========================================
+ Coverage 70.58% 70.67% +0.08%
===========================================
Files 3178 3187 +9
Lines 111796 112901 +1105
Branches 20213 20550 +337
===========================================
+ Hits 78914 79789 +875
- Misses 30831 31069 +238
+ Partials 2051 2043 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@mdawoud27 pls check the CI checks, and run them locally beforehand, so we don't waste time checking prs that are in an unmergeable state. |
mdawoud27
force-pushed
the
fix/team-service-params-validation
branch
from
becb60f to
f16a349
Compare
|
@KevLehman I've fixed the issues and run the CI checks locally and all of them passed 
|
|
Please, again, check the CI. There's 2 tests failing regarding this fix (on team management), so check why they're failing and fix the code. That's something i shouldn't be telling you, as the CI is there :) |
mdawoud27
force-pushed
the
fix/team-service-params-validation
branch
from
f16a349 to
50974bd
Compare
3 participants
Proposed changes (including videos or screenshots)
This PR introduces comprehensive data validation for the
dataandmembersPRIVATEorPUBLIC).membersinput is an array of valid strings and automatically checks for duplicates to prevent database inconsistencies.Issue(s)
N/A
Steps to test or reproduce
invalid-team-nameorteam-name-too-long)['user1', 'user1']). (Expected:duplicate-members)teamparameter. (Expected:invalid-members-formatorinvalid-team-data)Summary by CodeRabbit