regression(ABAC): Add missing attribute value in use validation to form #37859
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
WalkthroughAdds a pre-delete in-use check for locked ABAC attribute values that shows a disclaimer with a "View rooms" link when deletion is blocked, reads search parameters into ABAC attributes/rooms pages, adds a navigation hook to view filtered rooms, and updates tests and translations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AttributesForm
participant API as /v1/abac/attributes/:key/is-in-use
participant Router
User->>AttributesForm: Click delete on locked attribute value
AttributesForm->>API: GET is-in-use? (key from getValues('name'))
alt inUse = true
API-->>AttributesForm: { inUse: true }
AttributesForm-->>User: Show disclaimer with "View rooms" link
User->>AttributesForm: Click "View rooms"
AttributesForm->>Router: replace -> admin-ABAC?searchTerm=key&type=attribute
Router-->>User: Render ABAC rooms filtered by attribute
else inUse = false
API-->>AttributesForm: { inUse: false }
AttributesForm->>AttributesForm: removeLockedAttributeField(index)
AttributesForm-->>User: UI updates (value removed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization 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)
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 |
Contributor
📦 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 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 06:10", "12/20 06:18 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 26 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sat, 20 Dec 2025 06:18:07 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37859 +/- ##
========================================
Coverage 67.69% 67.70%
========================================
Files 3476 3476
Lines 113895 113895
Branches 20956 20956
========================================
+ Hits 77098 77108 +10
+ Misses 34609 34600 -9
+ Partials 2188 2187 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
force-pushed
the
abac/prevent-remove
branch
from
1961375 to
d41c658
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx (1)
52-94: Add error handling for the async endpoint call.The
isAttributeUsed()call on line 53 lacks error handling. If the endpoint fails due to network issues or backend errors, the function will throw and provide a poor user experience.🔎 Apply this diff to add error handling:
const deleteAction = useEffectEvent(async () => { + try { const isUsed = await isAttributeUsed(); if (isUsed.inUse) { return setModal( <GenericModal variant='warning' icon={null} title={t('ABAC_Cannot_delete_attribute')} confirmText={t('View_rooms')} - // TODO Route to rooms tab once implemented onConfirm={() => { viewRoomsAction(attribute.key); setModal(null); }} onCancel={() => setModal(null)} > <Trans i18nKey='ABAC_Cannot_delete_attribute_content' values={{ attributeName: attribute.key }} components={{ bold: <Box is='span' fontWeight='bold' /> }} /> </GenericModal>, ); } + } catch (error) { + dispatchToastMessage({ type: 'error', message: error }); + return; + } setModal( <GenericModal variant='danger' icon={null} title={t('ABAC_Delete_room_attribute')} confirmText={t('Delete')} onConfirm={() => { deleteMutation.mutateAsync(undefined); }} onCancel={() => setModal(null)} > <Trans i18nKey='ABAC_Delete_room_attribute_content' values={{ attributeName: attribute.key }} components={{ bold: <Box is='span' fontWeight='bold' /> }} /> </GenericModal>, ); });
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (2)
282-287: Redundant assertion afterwaitFor.The assertion on line 286 duplicates the check already performed inside
waitForon line 283. Consider removing the redundant line.🔎 Suggested fix:
await waitFor(() => { expect(screen.getByText('Cannot delete attribute value assigned to rooms.')).toBeInTheDocument(); }); - - expect(screen.getByText('Cannot delete attribute value assigned to rooms.')).toBeInTheDocument(); });
269-276: Consider including base translations in the mock.The test-specific
mockAppRootonly includes the disclaimer translation. If the component renders other translated strings during the interaction, this could cause translation key fallbacks to display. Consider extending the translation mock to include base translations from the sharedappRootfor consistency.
📜 Review details
Configuration used: Organization 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 (7)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(4 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 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/client/views/admin/ABAC/hooks/useAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.tsapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
🧠 Learnings (14)
📓 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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
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-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.tsapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.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/admin/ABAC/hooks/useAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
📚 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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.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/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
🧬 Code graph analysis (4)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSearchParameter(66-66)
apps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.ts (1)
packages/ui-contexts/src/index.ts (1)
useRouter(63-63)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (1)
packages/ui-contexts/src/index.ts (2)
useRouter(63-63)useSearchParameter(66-66)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
🔇 Additional comments (7)
apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx (2)
10-10: LGTM!The import is correctly added to support navigation to the rooms view when an attribute is in use.
22-22: LGTM!The hook is correctly instantiated and the returned action is properly used in the warning modal handler.
packages/i18n/src/locales/en.i18n.json (1)
34-34: New ABAC in-use attribute value message looks goodThe key name and copy are consistent with existing ABAC strings (e.g.,
ABAC_Cannot_delete_attribute), and the<1>View rooms</1>wrapper matches the component-based i18n pattern used elsewhere. No issues from an i18n or UX wording perspective.apps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.ts (1)
1-25: LGTM!The hook is well-structured with clear single responsibility. Using
useEffectEventensures the callback identity remains stable across renders, and the navigation configuration correctly targets the ABAC rooms view with the appropriate search parameters.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
12-12: LGTM!The integration of
useSearchParameterenables deep-linking to a pre-filtered attributes view, which aligns with the navigation from the "View rooms" action in the disclaimer flow. The nullish coalescing to empty string is appropriate.Also applies to: 25-27
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (2)
142-161: Disclaimer implementation looks correct.The conditional disclaimer rendering with
Transfor i18n interpolation is appropriate. The inline link correctly callsviewRoomsActionwith the current form name value.One minor accessibility consideration: the
Box is='a'without anhrefattribute won't behave as a true anchor (no middle-click, no right-click "Open in new tab"). Since this is an internal navigation that replaces the current view, the current approach is acceptable.
77-86: Async deletion flow with in-use check.The implementation correctly:
- Short-circuits if the disclaimer is already shown for this index (prevents redundant API calls)
- Checks the endpoint and shows disclaimer if in use
- Proceeds with removal otherwise
The
useEffectEventwrapper ensures stable function identity while accessing current state.
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
Show resolved
Hide resolved
apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx
Outdated
Show resolved
Hide resolved
MartinSchoeler
force-pushed
the
abac/prevent-remove
branch
from
dcc79fa to
2c605a6
Compare
tassoevan
force-pushed
the
abac/prevent-remove
branch
from
2c605a6 to
af912e8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
48-48: Stale attribute name in endpoint path parameter.The endpoint is created with
getValues('name')evaluated at render time. If the user edits the name field and then attempts to delete a locked attribute, the in-use check will query against the original name rather than the current value.🔎 Recommended fix (as per previous review)
-const isAttributeUsed = useEndpoint('GET', '/v1/abac/attributes/:key/is-in-use', { key: getValues('name') }); +const isAttributeUsedEndpoint = useEndpoint('GET', '/v1/abac/attributes/:key/is-in-use');Then update line 78:
-const isInUse = await isAttributeUsed(); +const isInUse = await isAttributeUsedEndpoint({ key: getValues('name') });
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (1)
29-30: Consider synchronizing filter state back to URL parameters.The component reads
searchTermandtypefrom URL parameters on mount but doesn't update the URL when users modify the text input (line 74) or filter dropdown (line 85). This creates UX issues:
- Users cannot bookmark or share their current filtered view
- Browser refresh resets filters to the original URL parameters
- Browser back/forward navigation doesn't reflect filter changes
🔎 Suggested enhancement to sync state to URL
Add a
useEffectto update URL parameters whenevertextorfilterTypechange:+ useEffect(() => { + const params = new URLSearchParams(); + if (debouncedText) { + params.set('searchTerm', debouncedText); + } + if (filterType !== 'all') { + params.set('type', filterType); + } + router.navigate( + { + name: 'admin-ABAC', + params: { tab: 'rooms' }, + search: params.toString(), + }, + { replace: true } + ); + }, [debouncedText, filterType, router]); + const query = useMemo(Note: Use
replace: trueto avoid cluttering browser history with every filter change.Also applies to: 74-74, 85-85
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (1)
260-287: Remove duplicate assertion.The assertion at line 286 is redundant—line 283 already verifies the disclaimer text is present after
waitForcompletes.🔎 Proposed fix
await waitFor(() => { expect(screen.getByText('Cannot delete attribute value assigned to rooms.')).toBeInTheDocument(); }); - - expect(screen.getByText('Cannot delete attribute value assigned to rooms.')).toBeInTheDocument(); });
📜 Review details
Configuration used: Organization 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 (7)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(4 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx
- packages/i18n/src/locales/en.i18n.json
- apps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.ts
🧰 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/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
🧠 Learnings (15)
📓 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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
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/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
📚 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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.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/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (2)
packages/ui-contexts/src/index.ts (1)
useTranslation(81-81)apps/meteor/client/views/admin/ABAC/hooks/useViewRoomsAction.ts (1)
useViewRoomsAction(4-23)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (1)
24-24: LGTM!The baseline endpoint mock is correctly configured to return
inUse: falsefor all existing tests, preventing failures from the new endpoint call in the component.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (5)
13-20: LGTM!The new imports are correctly added to support the in-use validation flow:
useEffectEventfor the async removal handleruseEndpointfor the API calluseStatefor disclaimer stateTransfor internationalized link renderinguseViewRoomsActionfor navigation
40-40: LGTM!Adding
getValuesto the form context is necessary to read the current attribute name for the in-use check and room navigation.
50-50: LGTM!The rename to
removeLockedAttributeFieldimproves clarity, and the disclaimer state plususeViewRoomsActionhook are correctly initialized for the new validation flow.Also applies to: 74-75
77-86: LGTM!The
removeLockedAttributehandler correctly implements the validation flow:
- Checks in-use status via API
- Prevents duplicate disclaimers with early return
- Sets disclaimer state when deletion is blocked
- Removes the field when safe to do so
The implementation is sound aside from the stale name issue at line 48.
142-161: LGTM!The disclaimer rendering is correctly implemented:
- Conditionally displays based on
showDisclaimerstate- Uses
Transwith a custom link component for internationalization- Correctly reads the current form name value when the "View rooms" link is clicked
tassoevan
approved these changes
Dec 19, 2025
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
MartinSchoeler
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/i18n/src/locales/en.i18n.json (1)
34-34: New ABAC in‑use warning key looks correctKey name, wording, and
<1>…</1>placeholder pattern match existing ABAC strings andTransusage. Only very minor nit: neighboring keys likeABAC_Cannot_delete_attributeomit the final period; you could drop the period before<1>for visual consistency, but it’s not required.
📜 Review details
Configuration used: Organization 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)
packages/i18n/src/locales/en.i18n.json(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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
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-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
gaolin1
pushed a commit
to gaolin1/medsense.webchat
that referenced
this pull request
Jan 6, 2026
…rm (RocketChat#37859) Co-authored-by: Tasso Evangelista <2263066+tassoevan@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
When an attribute is in use, the admin should not be able to remove values from it.
Issue(s)
ABAC-103
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Localization
✏️ Tip: You can customize this high-level summary in your review settings.