fix: exclude deactivated users from discussion invite candidates#38921
fix: exclude deactivated users from discussion invite candidates#38921ABhavikj123 wants to merge 10 commits into
Conversation
🦋 Changeset detectedLatest commit: 51f2ade 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 |
|
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughWhen creating a discussion, candidate invitees (message author + provided users) are deduplicated and checked for Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
![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
🧹 Nitpick comments (2)
apps/meteor/app/discussion/server/methods/createDiscussion.ts (2)
149-154: Rename callback parameters to avoid shadowing the outeruser: IUser.The
useridentifier used in both.filter()and.map()shadows the destructureduser: IUserparameter of the enclosingcreatefunction (Line 73), which will triggerno-shadowlint warnings.♻️ Suggested rename
- const activeUsernames = new Set( - activeUsers - .filter((user) => user.active !== false) - .map((user) => user.username) - .filter(Boolean), - ); + const activeUsernames = new Set( + activeUsers + .filter((u) => u.active !== false) + .map((u) => u.username) + .filter(Boolean), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/discussion/server/methods/createDiscussion.ts` around lines 149 - 154, The Set construction for activeUsernames shadows the outer parameter named user in the enclosing create function (user: IUser), causing no-shadow lint warnings; rename the callback parameter(s) in the activeUsers array methods (e.g., change the `.filter((user) => ...)` and `.map((user) => user.username)` callbacks to use a different identifier like `u` or `active`), keeping the same logic and references to activeUsers and activeUsernames so the behavior of create remains unchanged.
144-144: Remove implementation comment — coding guideline violation.As per coding guidelines,
**/*.{ts,tsx,js}files should avoid code comments in the implementation.♻️ Suggested fix
- // auto invite the replied message owner, excluding deactivated users const candidateInvitedUsers = [...new Set((message ? [message.u.username, ...users] : users).filter(Boolean))];As per coding guidelines: "Avoid code comments in the implementation" for
**/*.{ts,tsx,js}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/discussion/server/methods/createDiscussion.ts` at line 144, Remove the implementation comment "// auto invite the replied message owner, excluding deactivated users" from apps/meteor/app/discussion/server/methods/createDiscussion.ts (inside the createDiscussion method/handler) to comply with the guideline banning implementation comments in **/*.{ts,tsx,js}; rely on self-documenting code or move any necessary explanation to documentation/PR description instead.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/discussion/server/methods/createDiscussion.tsapps/meteor/tests/end-to-end/api/rooms.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/tests/end-to-end/api/rooms.tsapps/meteor/app/discussion/server/methods/createDiscussion.ts
🧠 Learnings (4)
📚 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/tests/end-to-end/api/rooms.tsapps/meteor/app/discussion/server/methods/createDiscussion.ts
📚 Learning: 2025-11-24T17:08:17.065Z
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 apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/rooms.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/app/discussion/server/methods/createDiscussion.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/discussion/server/methods/createDiscussion.ts
🧬 Code graph analysis (1)
apps/meteor/app/discussion/server/methods/createDiscussion.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(169-239)
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/rooms.ts (1)
1700-1706: No verification needed —sendSimpleMessageproperly supportsuserCredentialsand uses it correctly.The function at
apps/meteor/tests/data/chat.helper.ts:6-34explicitly acceptsuserCredentialsas a parameter (line 10, with defaultcredentials), and passes it to the request via.set(userCredentials)(line 33). The test code at lines 1700-1706 correctly sends the message under deactivated user credentials, properly exercising the pmid-author filter path as intended.
🤖 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/tests/end-to-end/api/rooms.ts`:
- Around line 1727-1740: The test reads response.body.discussion._id without
asserting the createDiscussion response shape; add explicit assertions on the
createDiscussion response (e.g.,
expect(response.body).to.have.property('success', true);
expect(response.body).to.have.property('discussion');
expect(response.body.discussion).to.have.property('_id')) before using
response.body.discussion._id so failures are reported as clear assertion errors;
update the test around the createDiscussion response variable (response)
accordingly.
---
Nitpick comments:
In `@apps/meteor/app/discussion/server/methods/createDiscussion.ts`:
- Around line 149-154: The Set construction for activeUsernames shadows the
outer parameter named user in the enclosing create function (user: IUser),
causing no-shadow lint warnings; rename the callback parameter(s) in the
activeUsers array methods (e.g., change the `.filter((user) => ...)` and
`.map((user) => user.username)` callbacks to use a different identifier like `u`
or `active`), keeping the same logic and references to activeUsers and
activeUsernames so the behavior of create remains unchanged.
- Line 144: Remove the implementation comment "// auto invite the replied
message owner, excluding deactivated users" from
apps/meteor/app/discussion/server/methods/createDiscussion.ts (inside the
createDiscussion method/handler) to comply with the guideline banning
implementation comments in **/*.{ts,tsx,js}; rely on self-documenting code or
move any necessary explanation to documentation/PR description instead.
|
Hi maintainers, could you please assign the required milestone / QA label if anything is needed from my side? Thanks |
1 participant
Proposed changes (including videos or screenshots)
pmidusers[]Issue(s)
Closes #38918
Steps to test or reproduce
yarn --cwd apps/meteor eslint app/discussion/server/methods/createDiscussion.ts tests/end-to-end/api/rooms.tscd apps/meteorTS_NODE_COMPILER_OPTIONS='{"module":"commonjs"}' ./node_modules/.bin/mocha --config ./.mocharc.api.js --spec tests/end-to-end/api/rooms.ts --grep "should not auto-invite deactivated users from pmid author and users list"users[]Further comments
createDiscussion.rooms.isMemberto avoid false confidence from endpoints that already hide inactive users.Summary by CodeRabbit