chore: migrate groups.removeModerator endpoint to new OpenAPI pattern with AJV validations#39364
chore: migrate groups.removeModerator endpoint to new OpenAPI pattern with AJV validations#39364codewithharshal wants to merge 10 commits intoRocketChat:developfrom
Conversation
…d migrate to modern route definition
|
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 |
|
|
🦋 Changeset detectedLatest commit: 84f37a4 The changes in this PR will be included in the next version bump. 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 |
|
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:
WalkthroughReworks the groups.removeModerator route into a grouped Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "groupsEndpoints"
participant Validator as "AJV Validator"
participant Handler as "Route Handler"
Client->>API: POST /v1/groups.removeModerator (body)
API->>Validator: validate request schema
Validator-->>API: validation result
alt valid
API->>Handler: invoke handler with validated body
Handler-->>API: performs user & room lookup, removeRoomModerator
API-->>Client: 200 { success: true }
else invalid
API-->>Client: 400 / 401 error
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 |
![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 4 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/groups.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/groups.ts:937">
P1: Import existing `withUserIdProps` instead of duplicating schema compilation logic</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ahmed-n-abdeltwab
left a comment
ahmed-n-abdeltwab
left a comment
There was a problem hiding this comment.
I recommend running the formatting and linting runners Local for these 3 files, to make sure there no mistakes
|
@ahmed-n-abdeltwab Oh, I see there is a linting error in some files because the GroupsRemoveModeratorProps export was removed. After fixing that, I think it will be ready to go. |
…emoveModerator endpoint.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/groups.ts`:
- Around line 911-918: The 200 response schema uses ajv.compile<void> which
mismatches the actual schema ({ success: true}) and causes ExtractRoutesFromAPI
to infer the wrong return type; update the generic on ajv.compile in this
response block to the correct shape (e.g., replace void with a type representing
{ success: true }) so the compiled schema type aligns with the schema and
ExtractRoutesFromAPI yields the correct return type.
- Line 12: The import in groups.ts uses the internal path
'@rocket.chat/rest-typings/dist/v1/groups/BaseProps' — replace it with the
public entry import 'withUserIdProps' from '@rocket.chat/rest-typings' (i.e.,
change the import in apps/meteor/app/api/server/v1/groups.ts to import {
withUserIdProps } from '@rocket.chat/rest-typings'); if that symbol is not
exported from the main package, add an export for withUserIdProps in
packages/rest-typings/src/index.ts alongside the other validators so the public
entry point exposes it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34c7860a-a49c-489c-9264-33577b2149ed
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/groups.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:
apps/meteor/app/api/server/v1/groups.ts
🧠 Learnings (8)
📓 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: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Learnt from: 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-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.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/app/api/server/v1/groups.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.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: 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.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/groups.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/groups.ts
🔇 Additional comments (3)
apps/meteor/app/api/server/v1/groups.ts (3)
42-42: LGTM!The
ExtractRoutesFromAPIimport is correctly added to support the new type export pattern.
921-932: LGTM!The handler logic is correctly preserved from the original implementation, matching the existing pattern used by similar endpoints like
groups.addModerator.
1326-1331: LGTM!The type export and module augmentation correctly integrate the new endpoint into the
@rocket.chat/rest-typingstype system usingExtractRoutesFromAPI.
| @@ -1,5 +1,6 @@ | |||
| export type * from './groups'; | |||
|
|
|||
| export * from './BaseProps'; | |||
There was a problem hiding this comment.
Because it wasn't listed in the groups/index.ts file, the chain was broken. The root package (@rocket.chat/rest-typings) didn't know withUserIdProps existed, so when your API file tried to import it from root, TypeScript threw the "has no exported member" error. By adding export * from './BaseProps'; to the index.ts file, we fixed the broken link in the chain!
There was a problem hiding this comment.
why not moving it to the this file apps/meteor/app/api/server/v1/groups.ts ?
There was a problem hiding this comment.
Ooh, ok. Last time I made a mistake that way, and the coderabbit gave me some errors, but now I think I know why, and I might solve it this time.
3 participants
Proposed Changes (including screenshots)
This PR migrates the
groups.removeModeratorendpoint from the legacyAPI.v1.addRoutepattern to the OpenAPI-compliant format using AJV schema validation, continuing the REST API migration effort.Key Changes
API.v1.addRoutewith chained.post()forgroups.removeModerator.packages/rest-typings/src/v1/groups.tssince it is now auto-generated viaExtractRoutesFromAPI.Swagger UI
The endpoint is documented in Swagger UI at:
Test Case
This follows the same migration pattern established in:
RocketChat/Rocket.Chat-Open-API#150
Summary by CodeRabbit
Refactor
Chores