fix: Add users on federated room members wrong behavior
#36994
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 236d249 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
WalkthroughThe AddUsers component now disables the “Add users” button in federated, non-native rooms unless the form is dirty and the handler isn’t pending. A changeset bumps @rocket.chat/meteor with a patch note describing this fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as AddUsers Modal (Federated)
participant F as Form State
participant H as addClickHandler
U->>UI: Open Members tab
U->>UI: Click "Add users"
UI->>F: Check isDirty
UI->>H: Check isPending
alt New logic (federated, non-native)
note over UI: Button enabled only if !isPending AND isDirty
opt Not dirty or pending
UI-->>U: Button disabled
end
opt Dirty and not pending
U->>UI: Click enabled button
UI->>H: Submit add users
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36994 +/- ##
===========================================
+ Coverage 66.33% 66.35% +0.01%
===========================================
Files 3391 3391
Lines 115308 115309 +1
Branches 21129 21131 +2
===========================================
+ Hits 76494 76513 +19
+ Misses 36198 36182 -16
+ Partials 2616 2614 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
juliajforesti
force-pushed
the
fix/add-users
branch
from
e16e761 to
b4155e3
Compare
juliajforesti
force-pushed
the
fix/add-users
branch
from
b4155e3 to
236d249
Compare
juliajforesti
changed the title
Add users button on federated room membersAdd users button on federated room members behavior
juliajforesti
changed the title
Add users button on federated room members behaviorAdd users on federated room members wrong behavior
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/witty-candles-impress.md (1)
5-5: Fix typos and align wording with UI (“Add users”).Correct “memers” → “members” and prefer the UI label “Add users” for consistency.
Apply this diff:
-Fixes button `Add members` not being disabled on a federated room memers tab, when no user is selected +Fix "Add users" button not being disabled on the Members tab of a federated room when no user is selected.apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (2)
96-106: Parity tweak: show loading on federated button while pending.Non‑federated path uses
loading={isSubmitting}. Mirror that UX for the federated path.Apply this diff:
- <Button + <Button primary - disabled={addClickHandler.isPending || !isDirty} + loading={addClickHandler.isPending} + disabled={addClickHandler.isPending || !isDirty}
59-62: Type clarity: coerce to boolean.
room && !isRoomNativeFederated(room)yieldsIRoom | boolean. Coerce to boolean for cleaner types.-const isFederationBlocked = room && !isRoomNativeFederated(room); +const isFederationBlocked = !!room && !isRoomNativeFederated(room);
📜 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.
📒 Files selected for processing (2)
.changeset/witty-candles-impress.md(1 hunks)apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx(1 hunks)
🔇 Additional comments (1)
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (1)
96-104: Good: Button correctly disabled until a selection exists.
disabled={addClickHandler.isPending || !isDirty}matches the non‑federated behavior and prevents the empty modal. Meets FDR‑134.Please sanity‑check in a federated room:
- Initial state: button disabled.
- Select ≥1 user: button enabled.
- Clear all selections: button disabled again.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
FDR-134
Proposed changes (including videos or screenshots)
Add usersbutton on federated room members when no user is selectedIssue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores