feat: admin ABAC rooms tab #37557
feat: admin ABAC rooms tab #37557
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 |
|
WalkthroughThis PR adds comprehensive ABAC (Attribute-Based Access Control) Rooms management to the admin panel, introducing new query keys, room autocomplete and selection components, contextual create/edit forms, deletion modalities, and associated hooks. Updates include refactored export patterns for existing hooks and adjusted i18n strings for ABAC room workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
📦 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 06:12 (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]
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]
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]
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]
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]
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]
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.35]
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]
Statistics (last 12 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 04 Dec 2025 06:12:51 GMT |
MartinSchoeler
force-pushed
the
feat/abac-rooms
branch
from
6633bef to
e848d40
Compare
MartinSchoeler
force-pushed
the
feat/abac-rooms
branch
from
e848d40 to
009716e
Compare
MartinSchoeler
force-pushed
the
feat/abac-rooms
branch
from
e61851d to
1b061f9
Compare
MartinSchoeler
force-pushed
the
feat/abac-rooms
branch
from
56f2330 to
c56b48d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (23)
apps/meteor/client/views/omnichannel/currentChats/CurrentChatsPage.tsx (1)
351-369: Safer guard onroomslength; consider aligning other checksUpdating this block to
data?.rooms?.length > 0is good—it avoids accessing.lengthon a possibly undefinedroomsand keepsdata.rooms.map(...)safe under this guard.For consistency and to avoid similar edge cases elsewhere, consider updating the other
roomslength checks in this component (e.g., Lines 315, 333, 334) fromdata?.rooms.lengthtodata?.rooms?.lengthas well so all guards behave the same way ifroomsis ever missing.apps/meteor/client/lib/queryKeys.ts (1)
128-132: ABAC rooms query keys are consistent; consider tightening roomId typingThis new
ABACQueryKeys.roomsblock matches the existingroomAttributespattern and looks correct in terms of query key composition.One small optional improvement for type-safety and consistency with
roomsQueryKeys.roomabove:- room: (roomId: string) => [...ABACQueryKeys.rooms.all(), 'room', roomId] as const, + room: (roomId: IRoom['_id']) => [...ABACQueryKeys.rooms.all(), 'room', roomId] as const,Also, given ABAC attributes only exist on private rooms/teams (type
'p'), please ensure the APIs and components that useABACQueryKeys.rooms.*are operating on private rooms only, not public channels. Based on learnings, this keeps the ABAC surface aligned with the server-side model.packages/i18n/src/locales/en.i18n.json (5)
42-45: Deduplicate “Proceed with caution” strings; reuse existing key.You added two new copies of the same annotation. Prefer referencing the existing generic "Proceed_with_caution" to avoid divergent translations.
Example:
- "ABAC_Delete_room_annotation": "Proceed with caution", + "ABAC_Delete_room_annotation": "{{t 'Proceed_with_caution'}}", - "ABAC_Update_room_confirmation_modal_annotation": "Proceed with caution", + "ABAC_Update_room_confirmation_modal_annotation": "{{t 'Proceed_with_caution'}}",Based on learnings, i18n keys should be maintained only in en.i18n.json; reusing a single key reduces translator churn.
Also applies to: 57-59
54-55: Key naming consistency: singular vs plural search labels.You now have both “ABAC_Search_attributes” (line 40, existing) and “ABAC_Search_Attribute” (this change). Either make both plural or reuse the existing key to avoid UI inconsistency.
Suggested:
- "ABAC_Search_Attribute": "Search attribute", + "ABAC_Search_Attribute": "{{t 'ABAC_Search_attributes'}}"or rename to “ABAC_Search_attributes” if intent is identical.
46-49: Title case consistency for ABAC actions.Existing ABAC strings use sentence case (“Edit attribute”); new ones are title case (“Add Attribute”, “Edit Room”). Align casing to one style across the ABAC section.
For example:
- "ABAC_Add_Attribute": "Add Attribute", + "ABAC_Add_Attribute": "Add attribute", - "ABAC_Edit_Room": "Edit Room", + "ABAC_Edit_Room": "Edit room",
62-66: Empty-state capitalization consistency.“No Attributes” vs “No rooms” mixes styles. Recommend sentence case for both.
- "ABAC_No_attributes": "No Attributes", + "ABAC_No_attributes": "No attributes",
5742-5743: Namespace “View_rooms” or align with existing pattern.If this label is ABAC‑navigation specific, consider “ABAC_View_rooms” to keep keys discoverable; otherwise confirm it’s intentionally global alongside “View_channels”.
Briefly confirm where “View_rooms” is used (sidebar, ABAC tab, or global), to avoid leaking ABAC copy into unrelated UIs.
apps/meteor/client/views/admin/ABAC/ABACRoomAutocompleteDummy.tsx (1)
3-5: Consider removing unusedridproperty.The
ridproperty inroomInfois defined but never used in the component. If it's not needed for future functionality, consider simplifying the type.type ABACRoomAutocompleteDummyProps = { - roomInfo: { rid: string; name: string }; + roomInfo: { name: string }; };apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (1)
48-58: Consolidate redundantwaitForblocks.The three separate
waitForblocks checking for Room 1, 2, and 3 can be consolidated into a single assertion block for better test clarity and performance.- await waitFor(() => { - expect(screen.getByText('Room 1')).toBeInTheDocument(); - }); - - await waitFor(() => { - expect(screen.getByText('Room 2')).toBeInTheDocument(); - }); - - await waitFor(() => { - expect(screen.getByText('Room 3')).toBeInTheDocument(); - }); + await waitFor(() => { + expect(screen.getByText('Room 1')).toBeInTheDocument(); + expect(screen.getByText('Room 2')).toBeInTheDocument(); + expect(screen.getByText('Room 3')).toBeInTheDocument(); + });apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (2)
27-27: Address TODO: Use query keys object.The TODO comment indicates that query keys should be moved to a centralized object for consistency with the rest of the codebase. Based on the context,
ABACQueryKeysis available and should be used.Would you like me to generate a script to verify the existence and structure of
ABACQueryKeys.roomsin the codebase?
56-59: Type assertion safety concern.The type assertion
as unknown as IRoomon line 57 bypasses type checking. While this might work at runtime, it risks type safety if the API response structure doesn't match theIRoominterface exactly.Consider adding runtime validation or using a type guard to ensure the selected room has the expected structure:
if (setSelectedRoom && typeof setSelectedRoom === 'function') { - const selectedRoom = result?.data?.rooms.find(({ _id }) => _id === val) as unknown as IRoom; - setSelectedRoom(selectedRoom); + const selectedRoom = result?.data?.rooms.find(({ _id }) => _id === val); + if (selectedRoom) { + setSelectedRoom(selectedRoom); + } }apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx (1)
10-10: Address the TODO comment about endpoint types.Consider verifying the endpoint types to ensure type safety throughout the data flow.
Would you like me to help generate properly typed endpoint definitions or open an issue to track this?
apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx (1)
15-19: Consider adjusting staleTime for better performance.Setting
staleTime: 0causes the data to be considered stale immediately, triggering refetches on every component mount or window focus. This may lead to unnecessary network requests and impact performance.Unless real-time data is critical here, consider setting a reasonable staleTime:
const { data, isLoading, isFetching } = useQuery({ queryKey: ABACQueryKeys.rooms.room(id), queryFn: () => getRoomAttributes({ rid: id }), - staleTime: 0, + staleTime: 5 * 60 * 1000, // 5 minutes });apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx (1)
34-37: Consider optimizing endpoint creation.The
useEndpointcall at line 37 includeswatch('room')in the parameters, causing a new endpoint instance to be created whenever the room value changes in the form. This may be inefficient.If the endpoint supports dynamic
ridparameter, consider:- const attributeId = useRouteParameter('id'); - const createOrUpdateABACRoom = useEndpoint('POST', '/v1/abac/rooms/:rid/attributes', { rid: watch('room') }); + const attributeId = useRouteParameter('id'); + const createOrUpdateABACRoomBase = useEndpoint('POST', '/v1/abac/rooms/:rid/attributes');Then in the mutation:
- await createOrUpdateABACRoom(payload); + await createOrUpdateABACRoomBase({ ...payload, rid: data.room });Alternatively, if the current pattern is intentional for the endpoint's parameterization strategy, this can be kept as-is.
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (4)
43-64:updateActionis declaredasyncbut performs noawait.The function is marked
asyncbut doesn't use any asynchronous operations. This is unnecessary and could be misleading.- const updateAction = useEffectEvent(async (action: () => void) => { + const updateAction = useEffectEvent((action: () => void) => {
77-77: Form submission bypasses native form behavior.The form has an
onSubmithandler, but the Save button (Line 123) usesonClickwithtype="button"(default). This means pressing Enter in the form won't trigger submission. Consider usingtype="submit"on the Save button or removing theonSubmitfrom the form if it's not needed.- <Box is='form' onSubmit={handleSubmit(onSave)} id={formId}> + <Box is='form' id={formId}>Or alternatively, change the Save button to submit the form:
- <Button onClick={handleSave}>{t('Save')}</Button> + <Button type='submit' form={formId} onClick={handleSave}>{t('Save')}</Button>
78-95: UsehtmlForinstead ofidon FieldLabel for proper accessibility.At Line 79,
FieldLabelusesid={nameField}which sets an ID on the label itself, but this doesn't create a proper label-input association. Thearia-labelledby={nameField}on the input at Line 90 establishes a one-way association, but usinghtmlForon the label would be more idiomatic.- <FieldLabel id={nameField} required> + <FieldLabel htmlFor={nameField} required> {t('ABAC_Room_to_be_managed')} </FieldLabel>Note: This depends on whether the
ABACRoomAutocompletecomponent accepts and uses anidprop.
120-125: Consider making the Save buttonprimaryfor visual hierarchy.The Cancel and Save buttons have the same styling. Typically, the primary action (Save) should be visually distinct using
primaryprop.<ButtonGroup stretch> <Button onClick={onClose}>{t('Cancel')}</Button> - <Button onClick={handleSave}>{t('Save')}</Button> + <Button primary onClick={handleSave}>{t('Save')}</Button> </ButtonGroup>apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (2)
34-42: Handler namehandleNewAttributeis misleading.This handler navigates to create a new room, not a new attribute. Consider renaming for clarity.
-const handleNewAttribute = useEffectEvent(() => { +const handleNewRoom = useEffectEvent(() => {And update usage at Line 86:
-<Button onClick={handleNewAttribute} primary mis={8} disabled={isABACAvailable !== true}> +<Button onClick={handleNewRoom} primary mis={8} disabled={isABACAvailable !== true}>
91-94: Loading state not handled; table may flash empty state.When
isLoadingis true anddatais undefined (initial load), the condition(!data || data.rooms?.length === 0) && !isLoadingis false, so neither the no-results state nor the table is shown. Consider showing a loading skeleton.+import { TableSkeleton } from '../../../components/GenericTable'; ... - {(!data || data.rooms?.length === 0) && !isLoading ? ( + {isLoading ? ( + <GenericTable> + <GenericTableHeader> + <GenericTableHeaderCell>{t('Room')}</GenericTableHeaderCell> + <GenericTableHeaderCell>{t('Members')}</GenericTableHeaderCell> + <GenericTableHeaderCell>{t('ABAC_Attributes')}</GenericTableHeaderCell> + <GenericTableHeaderCell>{t('ABAC_Attributes_Values')}</GenericTableHeaderCell> + <GenericTableHeaderCell key='spacer' w={40} /> + </GenericTableHeader> + <GenericTableBody> + {/* Add skeleton rows */} + </GenericTableBody> + </GenericTable> + ) : (!data || data.rooms?.length === 0) ? (apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx (3)
48-50: Loading skeleton may cause layout shift.Returning
InputBoxSkeletonwhile loading replaces the entire field structure (two rows + button), which could cause layout instability. Consider showing skeleton states for both fields and the button, or using a fixed-height container.if (isLoading) { - return <InputBoxSkeleton />; + return ( + <Box display='flex' flexDirection='column' w='full'> + <FieldRow><InputBoxSkeleton /></FieldRow> + <FieldRow><InputBoxSkeleton /></FieldRow> + <Button disabled mbs={4}>{t('Remove')}</Button> + </Box> + ); }
78-93: Values field should be disabled when no attribute is selected.When
currentAttributeis empty/undefined,valueOptionswill be an empty array. The MultiSelect will be functional but show no options, which could confuse users. Consider disabling it until an attribute is selected.<MultiSelect {...field} options={valueOptions} placeholder={t('ABAC_Select_Attribute_Values')} error={errors.attributes?.[index]?.values?.message} + disabled={!currentAttribute} />
95-97: Consider addingdangervariant to the Remove button for visual clarity.The Remove button performs a destructive action. Adding visual distinction would improve UX.
-<Button onClick={onRemove} title={t('Remove')} mbs={4}> +<Button danger onClick={onRemove} title={t('Remove')} mbs={4}> {t('Remove')} </Button>
📜 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 (3)
apps/meteor/client/views/admin/ABAC/__snapshots__/ABACAttributeField.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/ABAC/__snapshots__/ABACRoomAutocomplete.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
apps/meteor/client/lib/queryKeys.ts(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocompleteDummy.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx(1 hunks)apps/meteor/client/views/omnichannel/currentChats/CurrentChatsPage.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(5 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/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsxapps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocompleteDummy.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsxapps/meteor/client/views/omnichannel/currentChats/CurrentChatsPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx
🧠 Learnings (19)
📓 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-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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.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/admin/ABAC/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsxpackages/i18n/src/locales/en.i18n.json
📚 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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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 `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocompleteDummy.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.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/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.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} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx
📚 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
📚 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/omnichannel/currentChats/CurrentChatsPage.tsx
🧬 Code graph analysis (9)
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-261)
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (3)
packages/ui-contexts/src/index.ts (1)
useSetModal(68-68)packages/livechat/src/components/Button/index.tsx (1)
Button(34-99)packages/livechat/src/components/ButtonGroup/index.tsx (1)
ButtonGroup(8-14)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx (3)
apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx (1)
Default(75-75)
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (2)
packages/ui-contexts/src/index.ts (3)
useRouter(60-60)useSetModal(68-68)useToastMessageDispatch(75-75)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)
apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx (2)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
isLoading(279-282)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (4)
apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/lib/appLayout.tsx (1)
render(26-28)packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx (1)
input(149-159)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (4)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)packages/ui-contexts/src/index.ts (1)
useRouter(60-60)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)packages/livechat/src/components/Form/TextInput/index.tsx (1)
TextInput(38-38)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/ui-kit/src/index.ts (1)
Option(2-2)
apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx (3)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
AdminABACRoomFormData(20-23)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx (1)
Default(44-44)
⏰ 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 Packages
🔇 Additional comments (17)
packages/i18n/src/locales/en.i18n.json (1)
5694-5697: Based on my verification, I can now provide the rewritten review comment:
These generic keys follow the established i18n pattern and pose no collision risk.
The locale file already contains numerous single-word generic keys (Accept, Action, Activity, Add, Address, Administration, etc.), and "Value"/"Values" follow this established convention. No existing collision with these exact keys was found, and the related variant keys (Value_messages, Value_users) suggest appropriate context-specific organization. No evidence of ABAC-specific usage or conflicts detected.
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
23-25: LGTM!The new "Rooms" tab follows the established pattern and integrates cleanly with the existing tab structure.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx (1)
71-84: LGTM with optional chaining improvements.The addition of optional chaining (
data.attributes?.length,data?.attributes?.map) improves defensive coding and prevents potential null/undefined access errors. The translation key updates align with ABAC naming conventions.apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx (1)
1-44: LGTM!The Storybook story is well-structured with appropriate mocked endpoints and args. The use of private room types (
t: 'p') aligns with ABAC requirements.apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsx (1)
37-37: TODO comment noted.The TODO comment indicates a known limitation regarding a11y compliance and testability. This is appropriately documented for future work.
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (2)
9-14: LGTM: Correct room type filtering.The query correctly filters for private rooms (
types: ['p']), which aligns with the ABAC requirement that ABAC attributes can only be set on private rooms and teams.Based on learnings, ABAC attributes can only be set on private rooms (type 'p'), not public rooms.
36-38: LGTM: Filtering rooms with existing ABAC attributes.The logic correctly excludes rooms that already have ABAC attributes, preventing duplicate management and ensuring only unmanaged rooms can be selected.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
125-125: Translation key exists and is properly defined.The translation key
Add_Valueis correctly defined inpackages/i18n/src/locales/en.i18n.jsonat line 403, mapping to "Add Value". The change from the literal string to the i18n key reference is valid and follows proper internationalization practices.apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx (1)
1-28: LGTM!Clean component structure that properly integrates the room menu items hook with the GenericMenu UI.
apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx (1)
1-75: LGTM!Well-structured Storybook story with appropriate mocks and decorators for testing the ABACAttributeField component.
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx (1)
54-225: LGTM!Comprehensive test coverage for the useRoomItems hook, including menu structure, ABAC availability checks, navigation, modal interactions, and mutation side effects.
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (1)
1-83: LGTM!Well-structured hook providing room menu actions with proper modal confirmations, mutation handling, and cache invalidation.
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
7-13: LGTM!The rooms tab integration follows the established pattern for room-attributes, with proper ABAC availability checks and contextual bar routing.
Also applies to: 71-91
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (2)
96-108: LGTM!The dynamic attribute field rendering with
useFieldArrayis well-implemented. The remove callback correctly uses the index to remove the specific field.
109-117: The review comment is based on a misunderstanding of the form structure and should be rejected.The
AdminABACRoomFormuses aMultiSelectcomponent for thevaluesfield (ABACAttributeField.tsx, lines 84-89), not individual text input fields. The field has arequiredvalidation rule (line 82), which for a MultiSelect means at least one option must be selected—not that the array needs to be pre-populated with empty strings.Initializing with
values: []is correct because:
- Users select values from a predefined list via the MultiSelect component
- The required validation triggers on form submission if no values are selected
- Initializing with
values: ['']would not work with MultiSelect and would likely cause type errorsThe learning referenced (about AdminABACRoomAttributesForm) describes a different form component that manages individual attribute values with separate text inputs—not the room form under review.
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (1)
96-118: LGTM!The table rendering with room data, attribute keys, and action menu is well-structured. The fallback chain
room.fname || room.name || room._idat Line 113 handles edge cases appropriately.apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx (1)
61-64: LGTM!Resetting values when the attribute key changes is the correct behavior—it prevents stale values from a previously selected attribute from persisting.
apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx (1)
158-225: Consider extracting shared delete flow setup.These two tests share significant setup and execution logic (mock configuration, handler capture, delete trigger, modal wait, confirm execution). A helper function could reduce duplication and improve maintainability.
Example refactor:
const setupDeleteFlow = async (room: typeof mockRoom, deleteEndpoint: jest.Mock) => { let confirmHandler: (() => void) | undefined; mockSetModal.mockImplementation((modal) => { if (modal?.props?.onConfirm) { confirmHandler = modal.props.onConfirm; } }); const { result } = renderHook(() => useRoomItems(room), { wrapper: baseAppRoot.withEndpoint('DELETE', '/v1/abac/rooms/:rid/attributes', deleteEndpoint).build(), }); const deleteAction = result.current[1].onClick; if (deleteAction) { deleteAction(); } await waitFor(() => { expect(mockSetModal).toHaveBeenCalled(); }); return confirmHandler; };Then use it in both tests:
it('should call delete endpoint when delete is confirmed', async () => { const deleteEndpointMock = jest.fn().mockResolvedValue(null); const confirmHandler = await setupDeleteFlow(mockRoom, deleteEndpointMock); if (confirmHandler) { confirmHandler(); } await waitFor(() => { expect(deleteEndpointMock).toHaveBeenCalled(); }); });Additionally, consider adding a test for error handling when the delete endpoint fails.
📜 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/admin/ABAC/hooks/useRoomItems.spec.tsx(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/useRoomItems.spec.tsx
🧠 Learnings (8)
📓 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-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/hooks/useRoomItems.spec.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/admin/ABAC/hooks/useRoomItems.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.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/hooks/useRoomItems.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/hooks/useRoomItems.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/hooks/useRoomItems.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/hooks/useRoomItems.spec.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-261)
⏰ 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/admin/ABAC/hooks/useRoomItems.spec.tsx (3)
1-52: LGTM! Comprehensive test setup.The mock configuration and base app root setup are well-structured, providing all necessary context (router, modals, toast, translations, settings, endpoints) for testing the hook in isolation.
54-112: Excellent test coverage for core functionality.These tests properly verify:
- Menu item structure and properties
- Edit button enabling based on ABAC availability
- Navigation behavior with correct routing parameters
The use of
waitForand specific assertions withtoMatchObjectdemonstrates solid testing practices.
114-156: Well-structured tests for edge cases and modal interaction.The tests correctly verify:
- Edit button disabled state when ABAC is unavailable
- Delete confirmation modal display with proper variant and messaging
- All modal properties are validated
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (2)
54-57: Remove redundant inline comment above pagination reset effect.The comment on Line 54 restates exactly what the
useEffectdoes and conflicts with the guideline to avoid implementation comments in TS/JS files. The code is self-explanatory:- // Whenever the user changes the filter or the text, reset the pagination to the first page useEffect(() => { setCurrent(0); }, [debouncedText, filterType, setCurrent]);Dropping the comment keeps the implementation clean and aligned with the coding guidelines.
34-42: Align handler name with its behavior (room creation, not attribute).
handleNewAttributeactually navigates to the “new room” context (context: 'new'in theroomstab). Renaming it improves readability and avoids confusion with attribute-level UIs:- const handleNewAttribute = useEffectEvent(() => { + const handleNewRoom = useEffectEvent(() => { router.navigate({ name: 'admin-ABAC', params: { tab: 'rooms', context: 'new', }, }); }); ... - <Button onClick={handleNewAttribute} primary mis={8} disabled={isABACAvailable !== true}> + <Button onClick={handleNewRoom} primary mis={8} disabled={isABACAvailable !== true}>This is cosmetic but makes the intent of the handler clearer.
packages/i18n/src/locales/en.i18n.json (4)
49-50: De-duplicate “Add Room” to reuse existing key.You added both ABAC_Add_room and the generic Add_room with the same value. Prefer reusing Add_room in the UI to avoid duplicate translations.
Also applies to: 403-405
47-47: Prefer generic keys where possible (“Attribute Values”, “Value/Values”).You introduced ABAC_Attribute_Values while a generic Attribute_Values exists; likewise new generic Value/Values were added. Where feasible, reference the generic keys from ABAC screens to reduce translator workload and improve consistency.
Also applies to: 753-754, 5694-5694, 5697-5697
57-58: Reuse existing “Proceed with caution” key.ABAC_Update_room_confirmation_modal_annotation duplicates existing Proceed_with_caution. Consider reusing the generic key to avoid duplicated strings.
- "ABAC_Update_room_confirmation_modal_annotation": "Proceed with caution", + // Reuse "Proceed_with_caution"
52-53: Clarify private-room scope for ABAC management (if enforced).If ABAC applies only to private rooms, consider reflecting that constraint near “ABAC_Room_to_be_managed” or via helper text to prevent confusion. Based on learnings, ABAC attributes can only be set on private rooms/teams.
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 selected for processing (4)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx
- apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx
🧰 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/AdminABACRooms.tsx
🧠 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/admin/ABAC/AdminABACRooms.tsx
📚 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
📚 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:
packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (3)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)packages/ui-contexts/src/index.ts (1)
useRouter(60-60)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)
⏰ 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)
packages/i18n/src/locales/en.i18n.json (3)
42-46: ABAC room removal strings look good.Wording and placeholders are clear; no issues.
62-66: Empty-state ABAC texts read well.Clear and consistent with the new tab’s purpose.
5742-5743: Parity with channels (“View rooms” / “View channels”).Good consistency and capitalization.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37557 +/- ##
============================================
Coverage ? 54.48%
============================================
Files ? 2637
Lines ? 50056
Branches ? 11202
============================================
Hits ? 27272
Misses ? 20604
Partials ? 2180
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx
Outdated
Show resolved
Hide resolved
MartinSchoeler
force-pushed
the
feat/abac-rooms
branch
from
b417d1f to
f90ced3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
77-77: FormonSubmitbypasses confirmation modal for updates.The form's
onSubmitdirectly callshandleSubmit(onSave), but the Save button useshandleSavewhich shows a confirmation modal whenroomInfoexists. If the form is submitted via Enter key, the confirmation modal will be bypassed for update operations.- <Box is='form' onSubmit={handleSubmit(onSave)} id={formId}> + <Box is='form' onSubmit={(e) => { e.preventDefault(); handleSave(); }} id={formId}>apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx (1)
31-39: Fix duplicate-attribute validation logic (still allows duplicates).The current filter condition mixes
valueandcurrentAttributeand then checks> 1, which can still let a second duplicate slip through. You only need to count how many attributes share the same key and flag when more than one exists.- const validateRepeatedAttributes = useCallback( - (value: string) => { - const attributes = getValues('attributes'); - // Only one instance of the same attribute is allowed to be in the form at a time - const repeatedAttributes = attributes.filter((attribute) => attribute.key === value && attribute.key !== currentAttribute).length > 1; - return repeatedAttributes ? t('ABAC_No_repeated_attributes') : undefined; - }, - [currentAttribute, getValues, t], - ); + const validateRepeatedAttributes = useCallback( + (value: string) => { + const attributes = getValues('attributes'); + const repeatedAttributes = attributes.filter((attribute) => attribute.key === value).length > 1; + return repeatedAttributes ? t('ABAC_No_repeated_attributes') : undefined; + }, + [getValues, t], + );packages/i18n/src/locales/en.i18n.json (1)
42-60: Tighten ABAC microcopy; reuse shared annotation where possible.
- ABAC_Room_updated reads awkward. Prefer clearer subject-first phrasing.
- ABAC_Room_to_be_managed is clunky; suggest simpler wording.
- Optional: both ABAC_*_annotation keys duplicate the existing generic "Proceed_with_caution". Consider reusing it to avoid translation churn.
Apply:
- "ABAC_Room_to_be_managed": "Room to be ABAC-managed", + "ABAC_Room_to_be_managed": "Room to be managed by ABAC", - "ABAC_Room_updated": "{{roomName}} ABAC room updated", + "ABAC_Room_updated": "ABAC settings for {{roomName}} updated",Optionally (ref): replace these with "Proceed_with_caution":
- "ABAC_Delete_room_annotation": "Proceed with caution", + "ABAC_Delete_room_annotation": "{{Proceed_with_caution}}", - "ABAC_Update_room_confirmation_modal_annotation": "Proceed with caution", + "ABAC_Update_room_confirmation_modal_annotation": "{{Proceed_with_caution}}",
🧹 Nitpick comments (7)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (2)
11-18: Reduce reliance onas anyin mocked roomsThe endpoint mock currently casts each room to
any:rooms: [mockRoom1 as any, mockRoom2 as any, mockRoom3 as any],Since
createFakeRoomis already typed to return anIRoom-compatible shape, it should be possible to keep these strongly typed by tightening the generic oncreateFakeRoomand/or thewithEndpointtypings instead of usingany. That keeps mocks aligned with the real API and helps catch shape drift in future refactors.
24-38: Story-based snapshot and a11y tests look good; consider shared wrapper if requiredUsing
composeStories+test.eachfor snapshots and jest-axe checks is concise and scalable as stories evolve. If any of these stories rely on the same mocked app root / endpoints as the direct component test, consider rendering them with theappRootwrapper as well, to avoid divergence between story behavior in tests vs. in isolation; if they’re already self-contained via decorators, current setup is fine.apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx (1)
158-225: Consider combining tests 6 and 7 to reduce duplication.These two tests share identical setup and flow logic, differing only in their final assertions (endpoint call vs. toast message). Combining them would reduce duplication while maintaining coverage.
Example consolidated test:
it('should call delete endpoint and show success toast when delete is confirmed', async () => { const deleteEndpointMock = jest.fn().mockResolvedValue(null); let confirmHandler: (() => void) | undefined; mockSetModal.mockImplementation((modal) => { if (modal?.props?.onConfirm) { confirmHandler = modal.props.onConfirm; } }); const { result } = renderHook(() => useRoomItems(mockRoom), { wrapper: baseAppRoot.withEndpoint('DELETE', '/v1/abac/rooms/:rid/attributes', deleteEndpointMock).build(), }); const deleteAction = result.current[1].onClick; if (deleteAction) { deleteAction(); } await waitFor(() => { expect(mockSetModal).toHaveBeenCalled(); }); if (confirmHandler) { confirmHandler(); } await waitFor(() => { expect(deleteEndpointMock).toHaveBeenCalled(); expect(mockDispatchToastMessage).toHaveBeenCalledWith({ type: 'success', message: 'Room Test Room removed from ABAC management', }); }); });apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx (1)
9-13: Consider addingenabledoption for license/permission gating.The hook fetches ABAC attributes unconditionally. If ABAC has license or permission requirements, consider adding an
enabledoption to prevent unnecessary requests when those requirements aren't met.Also, the TODO comment at line 12 should be tracked or addressed to avoid leaving it indefinitely.
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
76-91: Simplify explicit boolean comparison and remove unnecessary fragments.The
isABACAvailable === truecomparison is verbose and the empty fragments add no value.{tab === 'room-attributes' && ( - <> - {context === 'new' && isABACAvailable === true && <RoomAttributesContextualBar onClose={() => handleCloseContextualbar()} />} - {context === 'edit' && _id && isABACAvailable === true && ( + {context === 'new' && isABACAvailable && <RoomAttributesContextualBar onClose={() => handleCloseContextualbar()} />} + {context === 'edit' && _id && isABACAvailable && ( <RoomAttributesContextualBarWithData id={_id} onClose={() => handleCloseContextualbar()} /> )} - </> )} {tab === 'rooms' && ( - <> - {context === 'new' && isABACAvailable === true && <RoomsContextualBar onClose={() => handleCloseContextualbar()} />} - {context === 'edit' && _id && isABACAvailable === true && ( + {context === 'new' && isABACAvailable && <RoomsContextualBar onClose={() => handleCloseContextualbar()} />} + {context === 'edit' && _id && isABACAvailable && ( <RoomsContextualBarWithData id={_id} onClose={() => handleCloseContextualbar()} /> )} - </> )}apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx (1)
11-17: Clean up unusedattributeIdprop and naming for clarity.
RoomsContextualBarPropsexposesattributeId, but the component ignores it and instead derivesattributeIdfromuseRouteParameter('id'). Also, the success toasts still interpolate withattributeNameeven though this form manages rooms.To reduce confusion and future bugs:
- Drop
attributeIdfromRoomsContextualBarProps(or actually use it instead of the route param).- Consider renaming the local
attributeIdvariable to something likeroomId(and aligning the toast interpolation key accordingly) to better reflect its purpose.Also applies to: 36-37, 52-57
packages/i18n/src/locales/en.i18n.json (1)
753-755: Avoid overlapping labels if not needed.You now have both "ABAC_Attribute_Values" and generic "Attribute_Values" with identical text. If components can use the generic key, drop the ABAC‑specific one to reduce duplicates and translation load. If contexts differ, keep both. As per i18n workflow, changes only in en file.
📜 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 (3)
apps/meteor/client/views/admin/ABAC/__snapshots__/ABACAttributeField.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/ABAC/__snapshots__/ABACRoomAutocomplete.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
apps/meteor/client/lib/queryKeys.ts(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocompleteDummy.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx(1 hunks)apps/meteor/client/views/omnichannel/currentChats/CurrentChatsPage.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx
- apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx
- apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomAutocompleteDummy.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx
- apps/meteor/client/views/omnichannel/currentChats/CurrentChatsPage.tsx
🧰 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/ABACAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsxapps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx
🧠 Learnings (20)
📓 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/ABACAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsxapps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributeField.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.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 `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useRoomItems.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/ABACRoomAutocomplete.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/ABACRoomAutocomplete.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/ABACRoomAutocomplete.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} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.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/admin/ABAC/hooks/useABACAttributeList.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxpackages/i18n/src/locales/en.i18n.json
📚 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/admin/ABAC/AdminABACRooms.tsx
📚 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
🧬 Code graph analysis (7)
apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
AdminABACRoomFormData(20-23)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (2)
apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx (1)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (3)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)apps/meteor/client/components/GenericTable/GenericTable.tsx (1)
GenericTable(11-24)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
apps/meteor/client/router/page.ts (1)
Page(92-241)
apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx (2)
packages/ui-contexts/src/index.ts (2)
useRouteParameter(64-64)useToastMessageDispatch(77-77)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)
apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
AdminABACRoomFormData(20-23)
⏰ 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 (13)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (3)
1-10: Solid Storybook-driven and RTL test structureThe suite is well-structured: Storybook composition for snapshot + a11y coverage, plus a focused behavioral test using RTL is a good pattern here. Imports and utilities look appropriate for the current testing stack.
15-22: Confirm endpoint path matches ABACRoomAutocomplete implementationThe mock uses
GET /v1/rooms.adminRooms. Other ABAC admin pieces (e.g., AdminABACRooms) are wired to/v1/abac/roomswhereabacAttributesis guaranteed to exist and be non-empty. Based on that, it would be good to double-check thatABACRoomAutocompletereally calls/v1/rooms.adminRoomsand not/v1/abac/rooms, otherwise the test may pass while the component is querying a different endpoint (or vice versa).
40-59: Behavioral test correctly exercises autocomplete flowThe functional test wiring (mocked endpoint + wrapper, clicking the textbox, waiting for each room label) exercises the core behavior of the autocomplete and should catch regressions in data loading or option rendering. Assertions on all three room labels are clear and robust enough given the current scope.
apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx (1)
31-34: Initial data pattern looks appropriate for infinite query bootstrapping.Setting
total: InfinityininitialDataensuresgetNextPageParamtriggers the first fetch. This is a valid pattern for infinite queries that need to fetch immediately.apps/meteor/client/views/admin/ABAC/ABACAttributeField.stories.tsx (2)
36-65: Decorator setup for form and API mocking looks well-structured.The decorator properly mocks the ABAC attributes endpoint with pagination metadata and wraps the story with
FormProvider. This enables testing the component's integration with both the API and form context.
10-29: Mock data is well-defined for testing attribute selection.The three mock attributes cover different scenarios (Department, Security-Level, Location) with varied value sets, providing good coverage for the attribute field's behavior.
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (2)
96-108: Dynamic attribute fields implementation looks correct.The
useFieldArrayintegration for managing multiple attributes with removal capability follows react-hook-form best practices. The key-based rendering ensures proper React reconciliation.
109-117: Attribute limit enforcement is clear.Disabling the "Add Attribute" button when
fields.length >= 10provides clear user feedback. Consider adding a tooltip or helper text to explain the limit.apps/meteor/client/lib/queryKeys.ts (1)
128-132: Query key structure follows established patterns.The new
ABACQueryKeys.roomsstructure mirrors the existingroomAttributespattern, maintaining consistency. The hierarchical key design (all→roomsList/room) enables flexible cache invalidation granularity.apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (1)
54-57: Good call resetting pagination on filter changes.Resetting
currentto0whendebouncedTextorfilterTypechanges avoids landing on invalid pages after filter updates and aligns well with theusePaginationbehavior.packages/i18n/src/locales/en.i18n.json (3)
403-405: LGTM — consistent button labels.“Add Value” / “Add Room” match existing patterns.
5694-5697: LGTM — base labels.“Value” / “Values” are fine and useful across contexts.
5742-5743: LGTM — aligns with “View channels”.“View rooms” label is consistent.
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (1)
16-19: Prefer an event-style callback over passing a state setter prop.Exposing
setSelectedRoomLabel: Dispatch<SetStateAction<string>>couples this component to React state details and was already questioned in a previous review. An API likeonRoomSelected?(value: string, label: string)would be more generic and easier to reuse while still supporting the current use case.
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (1)
41-47: Remove debug logging and makeonChangecall tolerant of missing handler.
console.login the change handler will spam the console in production, andprops.onChangeis invoked unconditionally even though it may be optional in the component’s public API.You can clean this up and be defensive about
onChange:- onChange={(val) => { - props.onChange(val); - console.log(val); - setSelectedRoomLabel(result.data?.find(({ value }) => value === val)?.label?.name || ''); - }} + onChange={(val) => { + props.onChange?.(val); + setSelectedRoomLabel(result.data?.find(({ value }) => value === val)?.label?.name || ''); + }}apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
45-47: Simplify save flow to avoid nestedhandleSubmitcalls.Right now the form calls
handleSubmit(handleSave), andhandleSavein turn callshandleSubmit(onSave)(and wraps that again inupdateActionfor existing rooms). This re-runs validation and makes the control flow harder to reason about.You can let
handleSavereceive validated data directly and delegate toonSaveor the confirmation modal:- const updateAction = useEffectEvent(async (action: () => void) => { + const updateAction = useEffectEvent((action: () => void) => { setModal( <GenericModal @@ - const handleSave = useEffectEvent(() => { - if (roomInfo) { - updateAction(handleSubmit(onSave)); - } else { - handleSubmit(onSave)(); - } - }); + const handleSave = useEffectEvent((data: AdminABACRoomFormData) => { + if (roomInfo) { + updateAction(() => onSave(data)); + return; + } + + onSave(data); + }); @@ - <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}> + <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}>This keeps validation in a single
handleSubmitcall and makes the “new vs. existing room” logic explicit.Also applies to: 68-74, 79-79
📜 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 (5)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx
- apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx
🧰 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/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx
🧠 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: 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/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.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/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.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/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.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/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.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/admin/ABAC/AdminABACRooms.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/ui-kit/src/index.ts (1)
Option(2-2)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (4)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)packages/ui-contexts/src/index.ts (1)
useRouter(62-62)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-133)apps/meteor/client/components/GenericTable/GenericTable.tsx (1)
GenericTable(11-24)
⏰ 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
force-pushed
the
feat/abac-rooms
branch
from
43e910e to
c9c3b4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (1)
17-19: Consider renaming to event-style naming convention.The
onSelectedRoomprop name is acceptable, but passing it as a setter-style callback was previously flagged. The current implementation does follow an event pattern by providing both value and label, which is reasonable.apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
67-78: DoublehandleSubmitwrapping causes redundant validation.The form's
onSubmitwrapshandleSaveinhandleSubmit, buthandleSaveinternally callshandleSubmit(onSave)again. This triggers form validation twice on every submission.Apply this fix - remove the outer
handleSubmitwrapper:- <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}> + <Box is='form' onSubmit={handleSave} id={formId}>apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (1)
109-109:flatMapon a string splits it into individual characters.When
attribute.keyis a string,flatMaptreats it as an iterable and flattens it into individual characters, producing output like"d, e, p, t"instead of"dept".Use
mapinstead offlatMapfor string keys:- <GenericTableCell>{room.abacAttributes?.flatMap((attribute) => attribute.key ?? []).join(', ')}</GenericTableCell> + <GenericTableCell>{room.abacAttributes?.map((attribute) => attribute.key).join(', ')}</GenericTableCell>Based on learnings,
abacAttributesis guaranteed to be defined for all returned rooms from/v1/abac/rooms, so additional null handling is not required.packages/i18n/src/locales/en.i18n.json (3)
52-52: Grammar fix looks good.“ABAC_Attribute_Values”: “Attribute Values” resolves the earlier “Attributes Values” issue.
58-58: Reword “ABAC_Room_updated”.Reads awkwardly. Suggest clearer order.
- "ABAC_Room_updated": "{{roomName}} ABAC room updated", + "ABAC_Room_updated": "ABAC settings for {{roomName}} updated",
67-67: Sentence case for empty‑state copy.Match repository’s sentence‑case convention for similar labels.
- "ABAC_No_attributes": "No Attributes", + "ABAC_No_attributes": "No attributes",As per i18n style learnings. Based on learnings, …
🧹 Nitpick comments (13)
apps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.spec.tsx (1)
16-24: Consider adding behavioral tests.The current test only verifies rendering via snapshot. Consider adding tests for user interactions, such as:
- Clicking the "Remove" button triggers
onConfirm- Clicking the "Cancel" button triggers
onClose- The
roomNameis properly interpolated in the contentExample:
+ it('should call onConfirm when Remove button is clicked', async () => { + const onConfirm = jest.fn(); + const { getByText } = render(<ABACDeleteRoomModal onClose={jest.fn()} onConfirm={onConfirm} roomName='Test Room' />, { + wrapper: appRoot, + }); + + getByText('Remove').click(); + + expect(onConfirm).toHaveBeenCalled(); + }); + + it('should call onClose when Cancel button is clicked', async () => { + const onClose = jest.fn(); + const { getByText } = render(<ABACDeleteRoomModal onClose={onClose} onConfirm={jest.fn()} roomName='Test Room' />, { + wrapper: appRoot, + }); + + getByText('Cancel').click(); + + expect(onClose).toHaveBeenCalled(); + });apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsx (1)
75-168: Consider extracting shared test setup to reduce duplication.Tests 2, 3, and 4 share significant setup logic:
deleteEndpointMockinitializationconfirmHandlercapture pattern viamockSetModal.mockImplementation- Calling
result.current()and waiting for modal- Executing
confirmHandlerThis pattern could be extracted into a helper function to improve maintainability.
Example:
const setupDeleteTest = async (wrapper: ReturnType<typeof baseAppRoot.build>) => { const deleteEndpointMock = jest.fn().mockResolvedValue(null); let confirmHandler: (() => void) | undefined; mockSetModal.mockImplementation((modal) => { if (modal?.props?.onConfirm) { confirmHandler = modal.props.onConfirm; } }); const { result } = renderHook(() => useABACDeleteRoomModal(mockRoom), { wrapper }); result.current(); await waitFor(() => { expect(mockSetModal).toHaveBeenCalled(); }); return { deleteEndpointMock, confirmHandler: () => confirmHandler?.() }; };apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (1)
54-54: Provide a fallback for undefined options.
result.datacan beundefinedduring initial load beforeplaceholderDatakicks in, which may cause AutoComplete to receiveundefinedforoptions.- options={result.data} + options={result.data ?? []}apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (2)
48-58: Consolidate waitFor assertions for cleaner test structure.Multiple sequential
waitForcalls checking for static content can be combined into a singlewaitForblock, reducing test verbosity and potential flakiness.- await waitFor(() => { - expect(screen.getByText('Room 1')).toBeInTheDocument(); - }); - - await waitFor(() => { - expect(screen.getByText('Room 2')).toBeInTheDocument(); - }); - - await waitFor(() => { - expect(screen.getByText('Room 3')).toBeInTheDocument(); - }); + await waitFor(() => { + expect(screen.getByText('Room 1')).toBeInTheDocument(); + expect(screen.getByText('Room 2')).toBeInTheDocument(); + expect(screen.getByText('Room 3')).toBeInTheDocument(); + });
16-17: Type assertion bypasses type safety.The
as anycast on mock rooms loses type checking. Consider using a more specific type or extending the mock to satisfy the expected shape.- rooms: [mockRoom1 as any, mockRoom2 as any, mockRoom3 as any], + rooms: [mockRoom1, mockRoom2, mockRoom3],If type errors arise, extend
createFakeRoomoverrides to include all required properties for the endpoint response type.apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx (1)
21-23: Consider whetherisFetchingcheck is needed here.Showing the skeleton on
isFetchingwill display it during background refetches (e.g., window focus), causing visual flicker even when stale data is available. Typically,isLoadingalone is sufficient for initial load states.- if (isLoading || isFetching) { + if (isLoading) { return <ContextualbarSkeletonBody />; }If you intentionally want to block UI during refetches, consider using
isPendinginstead, which handles the pending state more predictably.apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (1)
30-31: Simplify onClick handler.The arrow function wrapper is unnecessary since
editActionis already a stable callback fromuseEffectEvent.- { id: 'edit', icon: 'edit' as const, content: t('Edit'), onClick: () => editAction(), disabled: isABACAvailable !== true }, + { id: 'edit', icon: 'edit' as const, content: t('Edit'), onClick: editAction, disabled: isABACAvailable !== true },apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
14-19: Consider event-style callback instead of setter prop.Passing
setSelectedRoomLabelas a setter dispatch function couples the parent's state management to this component. An event-style callback likeonRoomLabelChangewould be more idiomatic.type AdminABACRoomFormProps = { onClose: () => void; onSave: (data: AdminABACRoomFormData) => void; roomInfo?: { rid: string; name: string }; - setSelectedRoomLabel: Dispatch<SetStateAction<string>>; + onRoomLabelChange?: (label: string) => void; };apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (2)
33-41: Consider renaminghandleNewAttributetohandleNewRoomfor clarity.The handler navigates to the rooms tab to create a new room, but the name suggests it handles attributes. This could confuse future maintainers.
- const handleNewAttribute = useEffectEvent(() => { + const handleNewRoom = useEffectEvent(() => { router.navigate({ name: 'admin-ABAC', params: { tab: 'rooms', context: 'new', }, }); });Also update the reference on line 85:
- <Button onClick={handleNewAttribute} primary mis={8} disabled={isABACAvailable !== true}> + <Button onClick={handleNewRoom} primary mis={8} disabled={isABACAvailable !== true}>
90-94: Consider adding a loading state indicator.When
isLoadingis true anddatais undefined, the table branch renders but displays an empty table body. Adding a loading skeleton or spinner would improve UX.packages/i18n/src/locales/en.i18n.json (3)
57-57: Tighten phrasing: “to be ABAC-managed”.Slightly awkward. Prefer clearer phrasing.
- "ABAC_Room_to_be_managed": "Room to be ABAC-managed", + "ABAC_Room_to_be_managed": "Room to be added to ABAC management",As per coding guidelines, keep labels concise.
63-64: Consistency with “ABAC‑managed”.Use the same hyphenated adjective seen elsewhere (“ABAC‑managed rooms”).
- "ABAC_Update_room_confirmation_modal_title": "Update ABAC room", + "ABAC_Update_room_confirmation_modal_title": "Update ABAC‑managed room",
77-80: ABAC section wording and hyphenation.Improve clarity and align with “ABAC‑managed” terminology.
- "ABAC_Managed": "ABAC Managed", - "ABAC_Managed_description": "Only compliant users have access to attribute-based access controlled rooms. Attributes determine room access.", - "ABAC_Room_Attributes": "Room Attributes", - "ABAC_Logs": "Logs", + "ABAC_Managed": "ABAC‑managed", + "ABAC_Managed_description": "Only compliant users can access ABAC‑managed rooms. Attributes determine room access.", + "ABAC_Room_Attributes": "Room attributes", + "ABAC_Logs": "ABAC logs",
📜 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/admin/ABAC/__snapshots__/ABACDeleteRoomModal.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
apps/meteor/client/lib/queryKeys.ts(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.stories.tsx
- apps/meteor/client/views/admin/ABAC/RoomsContextualBar.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx
- apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx
- apps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.tsx
🧰 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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsxapps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.spec.tsx
🧠 Learnings (24)
📓 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-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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsxapps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsxpackages/i18n/src/locales/en.i18n.jsonapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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 `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.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/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsxapps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/lib/queryKeys.tspackages/i18n/src/locales/en.i18n.json
📚 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/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/lib/queryKeys.ts
📚 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/admin/ABAC/AdminABACRooms.tsxapps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.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/hooks/useABACDeleteRoomModal.spec.tsxapps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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 `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsx
📚 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
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 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/ABACDeleteRoomModal.spec.tsx
🧬 Code graph analysis (7)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.spec.tsx (2)
apps/meteor/tests/mocks/data.ts (1)
createFakeRoom(46-61)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (4)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)packages/ui-contexts/src/index.ts (1)
useRouter(62-62)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-138)apps/meteor/client/components/GenericTable/GenericTable.tsx (1)
GenericTable(11-24)
apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (3)
packages/ui-contexts/src/index.ts (1)
useSetModal(70-70)packages/ui-client/src/components/Contextualbar/index.ts (2)
ContextualbarScrollableContent(35-35)ContextualbarFooter(32-32)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormProps(30-133)
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(62-62)
apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-261)
apps/meteor/client/views/admin/ABAC/ABACRoomAutocomplete.tsx (2)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-138)packages/ui-kit/src/index.ts (1)
Option(2-2)
apps/meteor/client/views/admin/ABAC/ABACDeleteRoomModal.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 (9)
apps/meteor/client/views/admin/ABAC/RoomsContextualBarWithData.tsx (1)
13-31: LGTM on the data fetching pattern.The component correctly uses
ABACQueryKeys.rooms.room(id)for cache key consistency and handles the loading state before rendering.apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (1)
10-40: Hook follows single responsibility principle.The hook is well-structured, delegating the delete modal logic to
useABACDeleteRoomModaland focusing solely on returning menu item definitions. Good separation of concerns.apps/meteor/client/views/admin/ABAC/AdminABACRoomForm.tsx (1)
107-119: Good use of dynamic field array for attributes.The attribute fields implementation with
useFieldArrayand per-item removal is clean. The 10-attribute limit on line 122 provides sensible bounds.apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx (1)
43-61: Query construction and data fetching look correct.The memoized query object, pagination reset effect, and React Query integration are properly implemented. The dependency arrays are complete.
apps/meteor/client/lib/queryKeys.ts (1)
129-137: Query key structure is well-organized and follows the established pattern.The new
roomsobject mirrors theroomAttributesstructure with appropriate keys for listing, autocomplete, and individual room access. The composition via spreading base paths ensures proper cache invalidation hierarchy.packages/i18n/src/locales/en.i18n.json (4)
416-418: New generic actions approved.“Add Value” / “Add Room” are consistent with adjacent action labels.
766-768: Good normalization of generic attribute labels.“Attribute Values” / “Attributes” additions look consistent.
5708-5712: Singular/plural value keys OK.“Value” / “Values” added cleanly for reuse across UI.
5756-5756: “View rooms” addition looks consistent.Matches existing “View channels” pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx (1)
1-28: Simple, well-scoped menu component looks goodThe props typing, hook usage, and
GenericMenuconfiguration all look correct and consistent with the provideduseRoomItemsimplementation; no functional or correctness issues stand out. If other ABAC admin views standardize on a specificuseTranslationsource (e.g.,@rocket.chat/ui-contexts), you might optionally align this import for consistency, but that's not blocking.apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsx (2)
5-11: Add explicit return type annotation.The hook lacks an explicit return type. Per the coding guidelines emphasizing "accurate typing", consider adding a return type annotation to improve type safety and code clarity.
Apply this diff to add the return type:
-export const useABACDeleteRoomModal = (room: { rid: string; name: string }) => { +export const useABACDeleteRoomModal = (room: { rid: string; name: string }): (() => void) => {
8-10: Consider memoizing the returned function.The returned function is recreated on every render. If passed as a prop (e.g., to a button's
onClick), this could cause unnecessary re-renders of child components. Wrapping the returned function withuseCallbackwould optimize this.Apply this diff to memoize the function:
- return () => { - setModal(<ABACDeleteRoomModal rid={room.rid} roomName={room.name} onClose={() => setModal(null)} />); - }; + return useCallback(() => { + setModal(<ABACDeleteRoomModal rid={room.rid} roomName={room.name} onClose={() => setModal(null)} />); + }, [room.rid, room.name, setModal]);Don't forget to import
useCallback:+import { useCallback } from 'react'; import { useSetModal } from '@rocket.chat/ui-contexts';apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.ts (1)
14-27: Address TODO: Endpoint type safety.The TODO comment indicates that endpoint types need verification. Ensuring proper typing for the
/v1/abac/attributesendpoint will improve type safety and catch potential runtime errors.Do you want me to help verify the endpoint types or open an issue to track this task?
apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx (2)
65-78: Avoid type casting forsetFilter.Line 72 uses type casting to force
setFiltercompatibility. This suggests a type mismatch that should be resolved at the source rather than worked around with casting.Consider updating the
setFilterstate to matchPaginatedSelectFiltered's expected signature:-const [filter, setFilter] = useState<string>(); +const [filter, setFilter] = useState<string | number | undefined>();Then remove the cast:
-setFilter={setFilter as (value: string | number | undefined) => void} +setFilter={setFilter}
77-80: Remove redundant error displays.Error messages are displayed twice for each field: once via the component's
errorprop (lines 77, 87) and once viaFieldError(lines 80, 90). This creates duplicate error messages in the UI.Remove the
errorprop from the components and rely solely onFieldErrorfor consistency:<PaginatedSelectFiltered {...keyField} onChange={(val) => { resetField(`attributes.${index}.values`); keyField.onChange(val); }} filter={filter} setFilter={setFilter as (value: string | number | undefined) => void} options={options} endReached={() => fetchNextPage()} placeholder={t('ABAC_Search_Attribute')} mbe={4} - error={keyFieldState.error?.message} /><MultiSelect {...valuesField} options={valueOptions} placeholder={t('ABAC_Select_Attribute_Values')} - error={valuesFieldState.error?.message} />Also applies to: 87-90
📜 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 (15)
apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(4 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.ts(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.ts(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx
- apps/meteor/client/views/admin/ABAC/ABACAttributeField.spec.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.spec.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
🧰 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/useIsABACAvailable.tsapps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.tsapps/meteor/client/views/admin/ABAC/ABACAttributeField.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx
🧠 Learnings (10)
📓 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/useABACAttributeList.tsapps/meteor/client/views/admin/ABAC/ABACAttributeField.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.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/useABACAttributeList.tsapps/meteor/client/views/admin/ABAC/ABACAttributeField.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.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/admin/ABAC/hooks/useABACAttributeList.tsapps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.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/admin/ABAC/hooks/useABACAttributeList.tsapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.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/useRoomAttributeOptions.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.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/useRoomAttributeOptions.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/useRoomAttributeOptions.spec.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSetModal(70-70)
apps/meteor/client/views/admin/ABAC/AdminABACRoomMenu.tsx (1)
apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (1)
useRoomItems(10-40)
⏰ 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 (9)
apps/meteor/client/views/admin/ABAC/hooks/useABACDeleteRoomModal.tsx (1)
1-3: LGTM!Imports are clean and correctly reference the required dependencies.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx (1)
4-4: LGTM: Export pattern updated correctly.The import has been updated to use a named export, consistent with the refactoring in
useRoomAttributeOptions.tsx.apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx (2)
4-23: LGTM: Test mocks updated correctly.The test file has been updated to:
- Use named imports consistent with the refactored exports
- Introduce a controllable mock for
useIsABACAvailable- Simplify the ui-contexts mock while preserving necessary functionality
124-145: LGTM: ABAC availability test case implemented correctly.The test properly uses the mock to simulate ABAC unavailability and verifies that the edit action is disabled accordingly.
apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.ts (1)
5-10: LGTM: Export pattern refactored correctly.The hook has been converted to a named export, consistent with the broader refactoring across ABAC hooks.
apps/meteor/client/views/admin/ABAC/hooks/useABACAttributeList.ts (2)
11-13: LGTM: Query properly gated by ABAC availability.The query is correctly disabled when ABAC is not available, preventing unnecessary API calls.
34-37: The use ofInfinityfor the initialtotalvalue is an intentional pattern used consistently across multiple pagination hooks in the codebase (useAgentsList, useAvailableAgentsList, useMonitorsList, useTagsList, useUnitsList, useDepartmentsList). This ensureshasNextPagereturns true during the loading state, allowing the UI to properly display pagination controls before actual data arrives. Once the API responds with real data, the actualtotalvalue replacesInfinity, and pagination functions correctly. No changes needed.apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx (1)
9-9: LGTM: Export patterns refactored consistently.Both the import of
useIsABACAvailableand the export ofuseRoomAttributeItemshave been updated to use named exports, aligning with the broader refactoring effort.Also applies to: 12-12
apps/meteor/client/views/admin/ABAC/ABACAttributeField.tsx (1)
24-32: LGTM: Validation logic correctly prevents duplicate attributes.The validation now properly checks if more than one attribute has the same key, preventing duplicates in the form.
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-50
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.