chore(api): Enforce Schema Validation on groups.list and groups.listAll#39226
Open
Harshit2405-2004 wants to merge 14 commits intoRocketChat:developfrom
Open
chore(api): Enforce Schema Validation on groups.list and groups.listAll#39226Harshit2405-2004 wants to merge 14 commits intoRocketChat:developfrom
Harshit2405-2004 wants to merge 14 commits intoRocketChat:developfrom
Conversation
Adds minLength constraints to users.forgotPassword and users.deleteOwnAccount to replicate legacy truthiness checks. Removes unaligned nullable fields from users.getAvatar definition.
🦋 Changeset detectedLatest commit: 30e898c 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
|
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 |
Contributor
|
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 AJV per-route parameter validation for multiple users endpoints (getAvatar, deleteOwnAccount, resetAvatar, forgotPassword) and groups endpoints (list, listAll); adds new request param types and validators in rest-typings and expands groups list schema to accept an optional name filter. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API Server
participant Validators as AJV Validators (rest-typings)
participant Handler as Route Handler
participant DB as Database
Client->>API: HTTP request (e.g., users.getAvatar / groups.list)
API->>Validators: validateParams(payload)
Validators-->>API: valid / invalid
alt valid
API->>Handler: invoke handler with validated params
Handler->>DB: query / update
DB-->>Handler: result
Handler-->>API: response payload
API-->>Client: 200 OK + payload
else invalid
API-->>Client: 400 Bad Request (validation errors)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 8 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="packages/rest-typings/src/v1/groups/GroupsListProps.ts">
<violation number="1" location="packages/rest-typings/src/v1/groups/GroupsListProps.ts:8">
P2: Schema validation disallows the `name` filter even though the request type allows it, so AJV will reject a valid `GroupsListProps` with `name` when `additionalProperties: false` is enabled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/add-users-rest-param-validations.md:
- Line 6: Update the changeset text to include the groups validation migration
by adding a sentence that states the PR also migrated `groups.list` and
`groups.listAll` to explicit Ajv schema validation; reference the existing users
endpoints (`users.getAvatar`, `users.deleteOwnAccount`, `users.resetAvatar`,
`users.forgotPassword`) and append that `groups.list` and `groups.listAll` were
similarly migrated so the release note accurately reflects all validation
changes.
In `@packages/rest-typings/src/v1/groups/GroupsListProps.ts`:
- Around line 4-32: The schema defined by groupsListPropsSchema does not include
the declared name field from the GroupsListProps type, causing valid requests to
be rejected due to additionalProperties: false; update groupsListPropsSchema to
add a name property with { type: 'string', nullable: true } inside properties so
the schema matches PaginatedRequest<{ name?: string }> and allows the optional
name query parameter.
In `@packages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts`:
- Around line 3-7: The UsersGetAvatarParamsGET type currently allows an empty
object and should require at least one selector; update the
UsersGetAvatarParamsGET definition to a union of three mutually exclusive shapes
(one with userId and the others set to never, one with username and the others
never, and one with user and the others never) so callers must provide exactly
one of userId | username | user; adjust any related uses such as
getUserFromParams to accept the new union type if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/add-users-rest-param-validations.mdapps/meteor/app/api/server/v1/groups.tsapps/meteor/app/api/server/v1/users.tspackages/rest-typings/src/v1/groups/GroupsListProps.tspackages/rest-typings/src/v1/users.tspackages/rest-typings/src/v1/users/UsersDeleteOwnAccountParamsPOST.tspackages/rest-typings/src/v1/users/UsersForgotPasswordParamsPOST.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/rest-typings/src/v1/users/UsersDeleteOwnAccountParamsPOST.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.tspackages/rest-typings/src/v1/users/UsersForgotPasswordParamsPOST.tspackages/rest-typings/src/v1/users.tspackages/rest-typings/src/v1/groups/GroupsListProps.tsapps/meteor/app/api/server/v1/groups.tsapps/meteor/app/api/server/v1/users.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 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/users/UsersDeleteOwnAccountParamsPOST.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.tspackages/rest-typings/src/v1/users/UsersForgotPasswordParamsPOST.tspackages/rest-typings/src/v1/users.tspackages/rest-typings/src/v1/groups/GroupsListProps.tsapps/meteor/app/api/server/v1/groups.tsapps/meteor/app/api/server/v1/users.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/users/UsersDeleteOwnAccountParamsPOST.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.tspackages/rest-typings/src/v1/users/UsersForgotPasswordParamsPOST.tspackages/rest-typings/src/v1/users.tspackages/rest-typings/src/v1/groups/GroupsListProps.tsapps/meteor/app/api/server/v1/groups.tsapps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:09.561Z
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.
Applied to files:
.changeset/add-users-rest-param-validations.md
📚 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:
.changeset/add-users-rest-param-validations.md
📚 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/groups.tsapps/meteor/app/api/server/v1/users.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/groups.tsapps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/api/server/v1/groups.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/app/api/server/v1/groups.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/app/api/server/v1/groups.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/app/api/server/v1/groups.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/groups.tsapps/meteor/app/api/server/v1/users.ts
📚 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/users.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/groups.ts (1)
packages/rest-typings/src/v1/groups/GroupsListProps.ts (1)
isGroupsListProps(34-34)
🔇 Additional comments (5)
packages/rest-typings/src/v1/users/UsersForgotPasswordParamsPOST.ts (1)
1-3: Type definition is clean and fit-for-purpose.This is a clear, minimal contract for the forgot-password payload and is ready to be consumed by route validation.
packages/rest-typings/src/v1/users/UsersDeleteOwnAccountParamsPOST.ts (1)
1-4: Payload typing looks correct.The required/optional field split is appropriate for the endpoint contract and aligns well with schema-based validation.
apps/meteor/app/api/server/v1/groups.ts (1)
4-4: Validation wiring forgroups.listandgroups.listAlllooks correct.Import and per-route
validateParamsusage are cleanly integrated and scoped to the intended endpoints.Also applies to: 656-656, 695-695
apps/meteor/app/api/server/v1/users.ts (1)
24-27: Per-route validator integration is consistent and well-scoped.The new
validateParamshooks for the migrated users endpoints are wired correctly and align with the rest-typings validators added in this PR.Also applies to: 90-93, 371-374, 738-741, 912-915
packages/rest-typings/src/v1/users.ts (1)
20-52: New users validators and exports are correctly integrated.The added AJV validators and re-exports provide a consistent public typing/validation surface for the migrated routes.
Also applies to: 416-418
ame filter inside GroupsListProps schema validation
Author
|
Resolved the GroupsListProps schema gap raised by @cubic-dev-ai by explicitly defining |
…atar selectors Updates UsersGetAvatarParamsGET to a union type and uses oneOf in the Ajv schema. Also updates changeset to include groups endpoints.
ggazzo
changed the title
Contributor
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/users.ts`:
- Around line 24-45: The current users.getAvatar endpoint typing allows a loose
object with optional userId/username/user which mismatches the validator
isUsersGetAvatarProps and the strict UserGetAvatarParamsGET union; update the
endpoint's parameter type/signature to use UsersGetAvatarParamsGET (the oneOf
union) instead of { userId?: string; username?: string; user?: string } so the
TypeScript signature enforces exactly one selector and matches
isUsersGetAvatarProps at runtime; ensure any places calling users.getAvatar are
adjusted to the union shape and update any helper types/interfaces so they
reference UsersGetAvatarParamsGET rather than the permissive optional-object
type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/add-users-rest-param-validations.mdpackages/rest-typings/src/v1/users.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/rest-typings/src/v1/users.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: 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.
📚 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/users.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.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/users.tspackages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts
📚 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:
packages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.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:
packages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts.changeset/add-users-rest-param-validations.md
📚 Learning: 2026-02-24T19:09:09.561Z
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.
Applied to files:
.changeset/add-users-rest-param-validations.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/add-users-rest-param-validations.md
🔇 Additional comments (3)
packages/rest-typings/src/v1/users/UsersGetAvatarParamsGET.ts (1)
3-6: Good strict selector typing for avatar params.This union correctly enforces a single selector path and prevents the empty-object case.
packages/rest-typings/src/v1/users.ts (1)
47-64: New validators fordeleteOwnAccountandforgotPasswordlook solid.Required fields plus
additionalProperties: falseare well-defined and consistent..changeset/add-users-rest-param-validations.md (1)
6-6: Changeset summary is now accurate and complete.Nice update including both users and groups validation migrations.
…nion type Updates UsersEndpoints to use UsersGetAvatarParamsGET instead of a loose object, ensuring TypeScript enforcement of the unique selector requirement.
coderabbitai
bot
added
type: chore
and removed
type: feature
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
Member
|
/jira ARCH-1464 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39226 +/- ##
===========================================
+ Coverage 70.85% 70.87% +0.02%
===========================================
Files 3208 3208
Lines 113426 113426
Branches 20489 20526 +37
===========================================
+ Hits 80363 80390 +27
+ Misses 31012 30982 -30
- Partials 2051 2054 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
6 issues found across 3000 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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=".agent/skills/agents-v2-py/SKILL.md">
<violation number="1" location=".agent/skills/agents-v2-py/SKILL.md:41">
P3: Authentication snippet references os.environ without importing os, so the standalone example will raise NameError if copied verbatim.</violation>
</file>
<file name=".agent/skills/airflow-dag-patterns/resources/implementation-playbook.md">
<violation number="1" location=".agent/skills/airflow-dag-patterns/resources/implementation-playbook.md:58">
P2: Branching example uses `datetime(...)` without importing `datetime`, so the snippet will raise a `NameError` when copied and parsed.</violation>
<violation number="2" location=".agent/skills/airflow-dag-patterns/resources/implementation-playbook.md:313">
P2: Sensor example references `@task.sensor` and `PokeReturnValue` without importing them, so the snippet will raise `NameError` when copied/executed.</violation>
</file>
<file name=".agent/skills/api-design-principles/assets/rest-api-template.py">
<violation number="1" location=".agent/skills/api-design-principles/assets/rest-api-template.py:31">
P2: Wildcard TrustedHost and CORS defaults in a production-ready template disable host header validation and allow any origin. This negates the security value of the middleware and can expose the API to unwanted cross-origin access unless manually corrected.</violation>
<violation number="2" location=".agent/skills/api-design-principles/assets/rest-api-template.py:148">
P2: HTTPException detail uses a dict for details, but ErrorResponse.details requires a list of ErrorDetail; this will raise a validation error in the exception handler and turn the 404 into a 500.</violation>
<violation number="3" location=".agent/skills/api-design-principles/assets/rest-api-template.py:167">
P2: PATCH accepts explicit nulls for required fields and assigns them to the User model without validation. This can produce an invalid User instance and cause response validation errors or contract violations. Filter out None updates or validate before assignment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
.agent/skills/airflow-dag-patterns/resources/implementation-playbook.md
Outdated
Show resolved
Hide resolved
.agent/skills/airflow-dag-patterns/resources/implementation-playbook.md
Outdated
Show resolved
Hide resolved
.agent/skills/api-design-principles/assets/rest-api-template.py
Outdated
Show resolved
Hide resolved
.agent/skills/api-design-principles/assets/rest-api-template.py
Outdated
Show resolved
Hide resolved
.agent/skills/api-design-principles/assets/rest-api-template.py
Outdated
Show resolved
Hide resolved
Harshit2405-2004
force-pushed
the
feat-groups-list-validation
branch
from
c230848 to
4720e84
Compare
Harshit2405-2004
force-pushed
the
feat-groups-list-validation
branch
from
4720e84 to
82cf45a
Compare
Member
|
@Harshit2405-2004 Just make sure your local is working check the lint, TypeScript and tests, otherwise I'll be forced to stop reviewing it. |
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.
Description
This PR migrates the legacy untyped REST API endpoints for
/v1/groups.listand/v1/groups.listAllover to the strict@rocket.chat/rest-typingsvalidation ecosystem, utilizing TypeBoxJSONSchemaimplementations to enforce rigorous client payloads natively.Previously,
groups.listomittedvalidateParamsfully, and the underlyingGroupsListPropsschema compiled with an empty{}definition. This PR fixes the schema definition by addingPaginatedRequesttype bounds (count,offset,sort,fields,query) directly into the Ajv compiler bounds and connects it to the meteor routing declarations.Issue it fixes
(Enhancement/Tech Debt) Relieves implicit
any/empty JSON validation assumptions on legacygroupsendpoints and bridges missing boundaries.Proposed changes
GroupsListPropsschema in@rocket.chat/rest-typings/src/v1/groups/GroupsListProps.tswith explicit strict configuration properties natively.validateParams: isGroupsListPropsonto thegroups.listandgroups.listAllroute definition bodies insideapps/meteor/app/api/server/v1/groups.ts.Type of Change
Verification Procedure
yarn workspace @rocket.chat/rest-typings typecheckexits exactly as expected.yarn workspace meteor typecheckpasses without generating mismatched validation parameter types acrossserver/v1/groups.ts.Summary by CodeRabbit
New Features
Improvements
Task: ARCH-2018