chore: ABAC Attributes room info view #37484
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds a RoomInfoABACSection component, tests, Storybook story, and three English i18n entries to display a room's ABAC attributes in the Room Info contextual bar when ABAC and the "show attributes" setting are enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant RoomInfo
participant ABAC as RoomInfoABACSection
participant Settings
Note over Settings: reads UI settings (ABAC_Enabled, ABAC_ShowAttributesInRooms)
RoomInfo->>ABAC: mount(room)
ABAC->>Settings: get ABAC_Enabled
alt ABAC disabled
ABAC-->>RoomInfo: render null
else ABAC enabled
ABAC->>Settings: get ABAC_ShowAttributesInRooms
alt show disabled
ABAC-->>RoomInfo: render null
else show enabled
ABAC->>ABAC: inspect room.abacAttributes
alt empty
ABAC-->>RoomInfo: render null
else non-empty
ABAC-->>RoomInfo: render divider, label, description, and attribute Tag lists
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
MartinSchoeler
force-pushed
the
chore/abac-room-info
branch
from
c2c55c4 to
0f20549
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37484 +/- ##
========================================
Coverage 68.97% 68.98%
========================================
Files 3358 3359 +1
Lines 114215 114224 +9
Branches 20534 20535 +1
========================================
+ Hits 78780 78793 +13
+ Misses 33344 33340 -4
Partials 2091 2091
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 (4)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsx (1)
132-133: Consider using optional chaining for safer null handling.The condition
room.abacAttributes !== undefinedwould evaluate totrueifabacAttributesisnull(sincenull !== undefined), then accessingnull.lengthwould throw a runtime error.Apply this diff to use optional chaining:
{/* @ts-expect-error - abacAttributes is not yet implemented in Rooms properties */} -{room.abacAttributes !== undefined && room.abacAttributes.length > 0 && <RoomInfoABACSection room={room} />} +{room.abacAttributes?.length > 0 && <RoomInfoABACSection room={room} />}This also simplifies the condition while safely handling both
undefinedandnull.packages/i18n/src/locales/en.i18n.json (2)
80-82: Polish microcopy for clarity.ABAC_Managed_description reads a bit awkwardly. Suggest concise phrasing that aligns with existing tone.
Apply this diff:
- "ABAC_Managed_description": "Only compliant users have access to attribute-based access controlled rooms. Attributes determine room access.", + "ABAC_Managed_description": "Only compliant users can access rooms protected by attribute‑based access control. Attributes determine room access.",
80-82: Pre-add i18n for admin setting and empty state.To meet the acceptance criteria and avoid hardcoded strings later, add keys for the ABAC “Show room attributes” setting and an empty-state message.
Apply this diff near the ABAC entries:
"ABAC_Managed": "ABAC Managed", "ABAC_Managed_description": "Only compliant users can access rooms protected by attribute‑based access control. Attributes determine room access.", "ABAC_Room_Attributes": "Room Attributes", + "ABAC_ShowAttributesInRooms": "Show room attributes", + "ABAC_ShowAttributesInRooms_Description": "Display ABAC room attributes in the Room Info panel when ABAC is enabled.", + "ABAC_No_Room_Attributes": "No room attributes", + "ABAC_No_Room_Attributes_Hint": "This room has no ABAC attributes to display.",This keeps UI copy ready when the setting and backend arrive and provides a graceful empty state.
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (1)
29-59: LGTM! Clean and accessible rendering logic.The rendering structure follows Fuselage and InfoPanel patterns well, with proper spacing and semantic markup. The use of
attribute.nameandvalueas React keys is appropriate, assuming attributes and their values are unique within the context.Optional: Consider handling edge case of empty values array.
If an attribute could have an empty
valuesarray, the current implementation will render the attribute name without any value tags. If this is intentional (e.g., for placeholder attributes), the current behavior is fine. Otherwise, you might want to add a check:{room.abacAttributes.map((attribute) => ( - <Box key={attribute.name} mb={16}> + attribute.values.length > 0 && <Box key={attribute.name} mb={16}> {attribute.name}However, this is likely unnecessary if the data model guarantees attributes always have values.
📜 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 ignored due to path filters (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/__snapshots__/RoomInfoABACSection.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx(1 hunks)apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx(1 hunks)apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx(3 hunks)apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsx(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsxpackages/i18n/src/locales/en.i18n.jsonapps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/ui-contexts/src/index.ts (1)
useSetting(69-69)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
🔇 Additional comments (10)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsx (1)
32-32: LGTM!The import path is correct and the component is properly integrated into the render logic below.
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx (2)
5-5: LGTM!The decorator refactoring properly wraps all stories with
FakeRoomProvider, ensuring room context is available. This is a clean pattern that supports the new ABAC story and maintains consistency across all story variants.Also applies to: 14-20
71-84: LGTM!The ABAC story is well-structured and provides realistic sample data. The
abacAttributesstructure (array of objects withnameandvaluesproperties) demonstrates various scenarios, including attributes with multiple values. The story will effectively showcase the new ABAC UI component.apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx (4)
1-14: LGTM! Clean test setup.The imports and type definitions are well-organized and appropriate for the test suite. The
RoomWithABACtype correctly extendsIRoomwith theabacAttributesstructure that matches the component's requirements.
16-24: LGTM! Reusable test helper.The
createRoomWithABAChelper is clean and promotes DRY principles across the test suite.
25-55: LGTM! Comprehensive conditional rendering coverage.The test suite thoroughly validates all conditional rendering paths, ensuring the component correctly responds to ABAC settings and attribute presence. Good use of the shared
appRootWithABACEnabledsetup to reduce duplication.
57-78: LGTM! Excellent test coverage with accessibility validation.Great to see accessibility testing with
jest-axealongside snapshot testing. This ensures the component is both accessible and visually consistent.apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (3)
1-8: LGTM! Clean imports.All imports are necessary and properly organized for the component's functionality.
9-17: Type definition is clear and well-documented.The TODO comment appropriately documents that this is a temporary type extension until
abacAttributesis added to theIRoominterface. This aligns with the PR's preparatory nature.Based on learnings
19-27: LGTM! Correct conditional rendering logic.The component properly validates all required conditions before rendering, with a clean early-return pattern. The logic correctly aligns with the test coverage.
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx
Show resolved
Hide resolved
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
Show resolved
Hide resolved
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (2)
33-40: Design choice: Tag component for section header.The Tag component is used here as a visual badge to indicate "ABAC Managed" status, which provides visual prominence. A previous reviewer questioned whether using a Tag for the section header might be confusing when the section body also contains Tags for attribute values. Consider whether a different visual treatment (e.g., bold text with icon) might provide clearer hierarchy.
44-60: Consider semantic HTML for better accessibility.While the current implementation includes ARIA labels (aria-labelledby), using semantic HTML elements (
<ul>and<li>) would improve accessibility for screen readers. A previous reviewer suggested this approach and offered to help with the implementation.Example structure using semantic elements:
<InfoPanelLabel id='room-attributes-list-label'>{t('ABAC_Room_Attributes')}</InfoPanelLabel> <Box is='ul' aria-labelledby='room-attributes-list-label' style={{ listStyle: 'none', padding: 0 }}> {room.abacAttributes.map((attribute) => ( <Box is='li' key={attribute.name} mb={16}> <Box is='span' id={`room-attribute-${attribute.name}-label`}> {attribute.name} </Box> <Box is='ul' display='flex' mbs={8} alignItems='center' aria-labelledby={`room-attribute-${attribute.name}-label`} style={{ listStyle: 'none', padding: 0 }}> {attribute.values.map((value) => ( <Box is='li' mie={4} key={value}> <Tag medium>{value}</Tag> </Box> ))} </Box> </Box> ))} </Box>
🧹 Nitpick comments (2)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx (1)
74-81: Consider removing the duplicate FakeRoomProvider decorator.The ABAC story redefines the FakeRoomProvider decorator (lines 76-80) which already exists in the default decorators (lines 15-21). In Storybook, story-level decorators are applied before component-level decorators, so both will run. The duplicate wrapper is redundant—only the mockAppRoot decorator (line 75) is needed for the ABAC story.
Consider simplifying to:
ABAC.decorators = [ mockAppRoot().withSetting('ABAC_Enabled', true).withSetting('ABAC_ShowAttributesInRooms', true).buildStoryDecorator(), - (fn) => ( - <FakeRoomProvider roomOverrides={roomArgs}> - <Contextualbar height='100vh'>{fn()}</Contextualbar> - </FakeRoomProvider> - ), ];apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (1)
19-27: LGTM! Component initialization and guards are correct.The component properly checks ABAC settings and attribute presence before rendering.
Optional: Line 25 could be slightly more concise:
-if (!abacEnabled || !showAttributesInRoom || room.abacAttributes?.length === 0) { +if (!abacEnabled || !showAttributesInRoom || !room.abacAttributes?.length) {
📜 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 ignored due to path filters (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/__snapshots__/RoomInfoABACSection.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx(1 hunks)apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx(3 hunks)apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsxapps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/ui-contexts/src/index.ts (1)
useSetting(69-69)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
⏰ 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
🔇 Additional comments (5)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx (3)
2-2: LGTM! Imports support the ABAC story.The mockAppRoot and FakeRoomProvider imports are correctly used to provide the necessary context for testing ABAC functionality in Storybook.
Also applies to: 6-6
15-21: LGTM! Default decorator provides room context.The FakeRoomProvider wrapper correctly supplies room context to all stories, enabling the RoomInfo component to access room data.
82-93: LGTM! ABAC story args are well-structured.The ABAC story correctly includes sample abacAttributes data for testing the UI. The @ts-expect-error comment appropriately documents that this is preparatory work before the attribute schema is formally added to the Room type.
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (2)
1-7: LGTM! Imports are appropriate.All necessary types, UI components, and hooks are correctly imported for the ABAC section functionality.
9-17: LGTM! Type definition aligns with preparatory nature of PR.The temporary type union with abacAttributes is appropriately documented with a TODO comment, consistent with the PR's objective to add UI components before the backend implementation exists.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 ignored due to path filters (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/__snapshots__/RoomInfoABACSection.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
⏰ 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: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (2)
44-60: Well-structured accessibility implementation.The semantic HTML structure with proper ARIA labeling addresses the accessibility concerns raised in previous reviews. The lists are correctly associated with their labels using
aria-labelledby, enabling better screen reader navigation.Note: Ensure the null check on line 25 is fixed to prevent reaching line 46 when
abacAttributesis undefined.
4-4: No changes needed—this is the standard translation pattern in the codebase.The import of
useTranslationfrom 'react-i18next' is consistent with established conventions. Verification confirms this pattern is used throughoutapps/meteor/client/views/, including multiple files in the samecontextualBardirectory (UserInfoWithData.tsx, ReportUserModal.tsx, OTR.tsx, etc.). This is the standard approach; no deviation exists.Likely an incorrect or invalid review comment.
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
Show resolved
Hide resolved
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
📚 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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/ui-contexts/src/index.ts (1)
useSetting(69-69)
⏰ 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
🔇 Additional comments (3)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx (3)
1-17: LGTM! Type definition correctly addresses previous feedback.The type definition properly declares
abacAttributesas optional, which aligns with the component's runtime checks and addresses the previous review feedback.
33-40: Tag usage pattern is intentional but consider visual distinction.The component uses a
Tagfor the "ABAC_Managed" header/badge and also usesTagcomponents for attribute values. While this might initially seem confusing (as noted in a previous review), the usage serves different semantic purposes:
- Header Tag: Indicates the room is ABAC-managed (badge/indicator)
- Value Tags: Display individual attribute values (pills/chips)
The implementation is clean and follows React best practices. If visual confusion becomes an issue in user testing, consider adding distinct styling or using a different component for the header.
19-27: Guard clause correctly handles all edge cases and the incomplete feature state.Verification confirms that
ABAC_EnabledandABAC_ShowAttributesInRoomssettings do not currently exist in the codebase—no server-side registration, no EE settings definitions. The component's guard clause!abacEnabled || !showAttributesInRoom || !room.abacAttributes?.lengthis properly defensive: when these settings are undefined (as they currently are),useSetting()returnsundefined, which correctly triggers an earlynullreturn. The optional chaining?.lengthand early return also protect the.map()call at line 46 from runtime errors.The TODO comment at line 9 confirms ABAC is an incomplete feature, and the code is appropriately written to handle this state. No changes needed.
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.tsx
Outdated
Show resolved
Hide resolved
aleksandernsilva
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 13:47 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 4 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 19 Nov 2025 13:47:10 GMT |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx (1)
27-55: Add tests for undefined attributes and content verification.The conditional rendering tests are solid, but consider these additions for more comprehensive coverage:
- Add a test for when
abacAttributesisundefined(not just an empty array), to ensure the component handles both cases correctly.- Add a test that verifies the actual attribute key-value pairs are rendered in the UI, not just the presence of the 'ABAC_Managed' heading.
Example test for verifying content:
it('should render attribute key-value pairs correctly', () => { const room = createRoomWithABAC([ { key: 'Chat-sensitivity', values: ['Classified', 'Top-Secret'] }, { key: 'Country', values: ['US-only'] }, ]); render(<RoomInfoABACSection room={room} />, { wrapper: appRootWithABACEnabled }); expect(screen.getByText('Chat-sensitivity')).toBeInTheDocument(); expect(screen.getByText(/Classified.*Top-Secret/)).toBeInTheDocument(); expect(screen.getByText('Country')).toBeInTheDocument(); expect(screen.getByText('US-only')).toBeInTheDocument(); });
📜 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 (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
⏰ 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
🔇 Additional comments (3)
apps/meteor/client/views/room/contextualBar/Info/RoomInfo/ABAC/RoomInfoABACSection.spec.tsx (3)
9-23: LGTM! Clean test setup.The type definition and helper function are well-structured. The type assertion in the helper is appropriate for the test context.
57-68: Excellent accessibility coverage!Great to see jest-axe being used to verify accessibility compliance. This helps ensure the ABAC attributes UI is accessible to all users.
69-78: Snapshot test looks good.The snapshot test will help catch any unintended UI changes in the ABAC section.
Proposed changes (including videos or screenshots)
Adds a new field in room info that shows the ABAC room attributes when the setting
ABAC_ShowAttributesInRoomsis enabled. Since neither abac attributes nor setting exists on develop, this is a chore, only adding the components to be used later onIssue(s)
ABAC-73
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Documentation
Tests
Chores