chore: Header tags for ABAC rooms #37126
Conversation
|
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 |
|
WalkthroughAdds an ABACHeaderTag component and tests, integrates it into both RoomHeader variants, adds two English i18n keys, and reduces HeaderTag minWidth from x64 to x32 in both Header and HeaderV2 components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RoomView as Room View
participant RoomHeader as RoomHeader
participant ABAC as ABACHeaderTag
participant Tag as HeaderTag
User->>RoomView: open room
RoomView->>RoomHeader: mount with room
RoomHeader->>ABAC: render <ABACHeaderTag room>
alt room.abacAttributes present
ABAC->>Tag: create HeaderTag with label t("ABAC") and title t("ABAC_header_tag_title")
Tag-->>RoomHeader: rendered visible tag
else no abac attributes
ABAC-->>RoomHeader: return null (no tag)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37126 +/- ##
===========================================
- Coverage 67.45% 67.41% -0.05%
===========================================
Files 3332 3334 +2
Lines 113607 113698 +91
Branches 20604 20623 +19
===========================================
+ Hits 76631 76644 +13
- Misses 34378 34449 +71
- Partials 2598 2605 +7
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: 1
🧹 Nitpick comments (1)
apps/meteor/client/components/ABAC/ABACHeaderTag.spec.tsx (1)
14-23: Consider stronger typing for the overrides parameter.The
anytype foroverridesbypasses type safety. Consider usingPartial<IRoom>or a more specific type to catch potential misuse.Apply this diff to improve type safety:
+import type { IRoom } from '@rocket.chat/core-typings'; + -const createMockRoom = (overrides: any = {}) => ({ +const createMockRoom = (overrides: Partial<IRoom> = {}) => ({ _id: 'room-id', t: 'c' as const, name: 'test-room', msgs: 0, u: { _id: 'user-id', username: 'testuser' }, usersCount: 1, _updatedAt: new Date(), ...overrides, -}); +}) as IRoom;
📜 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/components/ABAC/__snapshots__/ABACHeaderTag.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
.changeset/slow-trees-eat.md(1 hunks)apps/meteor/client/components/ABAC/ABACHeaderTag.spec.tsx(1 hunks)apps/meteor/client/components/ABAC/ABACHeaderTag.tsx(1 hunks)apps/meteor/client/views/room/Header/RoomHeader.tsx(2 hunks)apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx(2 hunks)packages/ui-client/src/components/Header/HeaderTag/HeaderTag.tsx(1 hunks)packages/ui-client/src/components/HeaderV2/HeaderTag/HeaderTag.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/components/ABAC/ABACHeaderTag.tsx (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/client/components/ABAC/ABACHeaderTag.spec.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). (1)
- GitHub Check: 🚢 Build Docker Images for Testing (alpine)
🔇 Additional comments (8)
packages/ui-client/src/components/HeaderV2/HeaderTag/HeaderTag.tsx (1)
7-7: LGTM: minWidth reduction improves tag layout.The reduction from
x64tox32eliminates unnecessary padding for smaller tags as described in the PR objectives.packages/ui-client/src/components/Header/HeaderTag/HeaderTag.tsx (1)
5-5: LGTM: minWidth reduction consistent with HeaderV2.The change mirrors the HeaderV2 variant and ensures consistent tag sizing across both header implementations.
.changeset/slow-trees-eat.md (1)
1-6: LGTM: Changeset properly documents the feature.The changeset correctly identifies the affected packages and provides a clear description of the ABAC header tag feature.
apps/meteor/client/components/ABAC/ABACHeaderTag.tsx (2)
1-28: LGTM: Clean component implementation with appropriate conditional rendering.The component correctly:
- Uses translation hooks for i18n support
- Applies warning color styling via Palette
- Returns null when ABAC attributes are absent
- Imports HeaderTag from the correct path
20-22: Confirm translation keys exist in i18n resources. EnsureABAC_header_tag_titleandABAC_header_tagare defined in your locale files (e.g.*.json,*.jsor*.tsunder/localesori18n); add them if they’re missing.apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx (1)
15-15: LGTM: ABACHeaderTag properly integrated into HeaderV2.The component is imported correctly and rendered in an appropriate position within the header row, consistent with other header tags like Favorite and FederatedRoomOriginServer.
Also applies to: 47-47
apps/meteor/client/views/room/Header/RoomHeader.tsx (1)
16-16: LGTM: ABACHeaderTag properly integrated into Header.The component is imported correctly and rendered in an appropriate position, maintaining consistency with the HeaderV2 implementation.
Also applies to: 54-54
apps/meteor/client/components/ABAC/ABACHeaderTag.spec.tsx (1)
25-77: Excellent test coverage with accessibility checks.The test suite thoroughly covers:
- Snapshot testing for visual regression
- Conditional rendering (presence, absence, undefined, null)
- Accessibility validation for both rendered and non-rendered states
The use of
mockAppRootwith translations and thecreateMockRoomhelper demonstrates good testing practices.
MartinSchoeler
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui-client/src/components/HeaderV2/HeaderTag/HeaderTag.spec.tsx (1)
5-8: Add accessibility and behavioral tests.The test suite only includes a snapshot test, which doesn't verify functionality or accessibility. According to best practices, tests should use user-centric queries to ensure the component is accessible and works as expected.
Consider adding tests that verify:
- The component renders the expected text content using
screen.getByText- Accessibility attributes (role, aria-label if applicable)
- Different scenarios or prop variations if the component supports them
Example enhancement:
import { render } from '@testing-library/react'; +import { screen } from '@testing-library/react'; import HeaderTag from './HeaderTag'; +it('should render the tag text', () => { + render(<HeaderTag>Test Tag</HeaderTag>); + expect(screen.getByText('Test Tag')).toBeInTheDocument(); +}); + it('should match snapshot', () => { const { baseElement } = render(<HeaderTag>Test Tag</HeaderTag>); expect(baseElement).toMatchSnapshot(); });Based on learnings
📜 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 (2)
apps/meteor/client/components/ABAC/__snapshots__/ABACHeaderTag.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/HeaderV2/HeaderTag/__snapshots__/HeaderTag.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/ui-client/src/components/HeaderV2/HeaderTag/HeaderTag.spec.tsx(1 hunks)
⏰ 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
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Added a new tag and tooltip to be shown when the room is under Attribute Based Access Control
V1 Header:
V2 Header:
Issue(s)
ABAC-44
Steps to test or reproduce
Not reproducible yet since the abac property on the rooms is yet to be implemented
Further comments
I've changed the min width of the headerTags for it avoid extra padding on smaller tags, an example of the issue:
Let me know if a separate PR would be better.
I've set the PR as
choresince these changes are not available without mocking dataSummary by CodeRabbit
New Features
Style
Tests