Skip to content

Conversation

@juliajforesti
Copy link
Contributor

@juliajforesti juliajforesti commented Sep 25, 2025

FDR-164

Proposed changes (including videos or screenshots)

Prevent adding external users to a non federated room through Members tab

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes

    • Prevents adding external users to non‑federated rooms from the Members tab and shows a localized error when attempted.
  • New Features

    • Adds client-side validation to the user selection in the Members tab.
    • Shows inline error feedback and improves accessibility by linking error messages to the input for screen readers.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Sep 25, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Sep 25, 2025

🦋 Changeset detected

Latest commit: f26895c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

Walkthrough

Adds client-side validation and accessibility wiring to the Members tab Add Users flow: prevents selecting external users when the current room is non-federated, surfaces a localized FieldError linked via useId, and adds a changeset entry for a patch release.

Changes

Cohort / File(s) Summary of changes
Client: Members tab validation & accessibility
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx
Added external-user detection helper, extended form state with errors, introduced useId for error association, added client-side validation blocking external users in non-federated rooms, wired aria-describedby and rendered FieldError when validation fails.
Release metadata
.changeset/two-owls-flow.md
Added a changeset entry describing a patch to prevent adding external users to non-federated rooms via Members tab.

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant UI as Members Tab (AddUsers)
  participant Validator as Client Validation
  participant Server as Room API

  User->>UI: Pick users to add
  UI->>Validator: Evaluate room.isFederated + selected users
  alt Non-federated room & external users detected
    Validator-->>UI: Return validation error
    UI-->>User: Show inline FieldError (aria-describedby) and block submit
  else No external users or room is federated
    UI->>Server: Submit add-users request
    Server-->>UI: Success / Failure
    UI-->>User: Present result
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • ggazzo
  • ricardogarim

Poem

A rabbit hops into the members view,
“No strangers in this burrow, only true!”
IDs in place, errors tidy and bright,
Federated rules keep the roster right.
Hooray—safe hops and code done light. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly identifies the addition of validation for external users on the Members tab and matches the primary change implemented, making it clear and specific without extraneous details.
Linked Issues Check ✅ Passed The changes introduce client‐side validation in the AddUsers component to prevent adding external users in non-federated rooms, which directly fulfills the FDR-164 objective of enforcing this rule on the Members tab.
Out of Scope Changes Check ✅ Passed All modifications are confined to adding validation logic and error handling for external users in the Members tab and a corresponding changeset entry, with no unrelated or extraneous changes introduced.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/validate-add-external-users

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b727e66 and f26895c.

📒 Files selected for processing (1)
  • apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx
⏰ 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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.39%. Comparing base (de51a86) to head (f26895c).
⚠️ Report is 6 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37071      +/-   ##
===========================================
+ Coverage    67.37%   67.39%   +0.02%     
===========================================
  Files         3326     3328       +2     
  Lines       113207   113360     +153     
  Branches     20535    20571      +36     
===========================================
+ Hits         76268    76396     +128     
- Misses       34338    34359      +21     
- Partials      2601     2605       +4     
Flag Coverage Δ
e2e 57.30% <60.00%> (+<0.01%) ⬆️
unit 71.15% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@juliajforesti juliajforesti marked this pull request as ready for review September 26, 2025 12:39
@juliajforesti juliajforesti requested a review from a team as a code owner September 26, 2025 12:39
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
.changeset/two-owls-flow.md (1)

5-5: Fix grammar in changeset summary

Tiny nit: switch to “an external user” for smoother wording.

Apply this diff to adjust the sentence:

-Prevents adding a external user to a non federated room through Members tab
+Prevents adding an external user to a non federated room through Members tab
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (1)

26-27: Default the users array before validating

React Hook Form can call a validator with undefined (e.g., before the field is registered or if the autocomplete clears by emitting undefined). In that case hasExternalUsers will throw on .some. Giving the helper a default [] keeps the check safe while preserving behavior.

Apply this diff to guard against undefined inputs:

-const hasExternalUsers = (users: string[]): boolean => users.some((user) => user.startsWith('@'));
+const hasExternalUsers = (users: string[] = []): boolean => users.some((user) => user.startsWith('@'));
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between de51a86 and b727e66.

📒 Files selected for processing (2)
  • .changeset/two-owls-flow.md (1 hunks)
  • apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (5 hunks)

Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants