chore(ABAC): refactor room form autocompletes #37817
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 |
|
WalkthroughThe PR refactors ABAC room attribute form rendering by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 19:43", "12/16 20:55 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.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, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 22 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 16 Dec 2025 20:55:47 GMT |
MartinSchoeler
force-pushed
the
reg/abac/selects
branch
from
5974a04 to
c65b1f9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37817 +/- ##
=============================================
- Coverage 54.38% 54.33% -0.05%
=============================================
Files 2639 2639
Lines 50102 50102
Branches 11212 11212
=============================================
- Hits 27248 27224 -24
- Misses 20681 20703 +22
- Partials 2173 2175 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx (1)
17-19: Consider handling error state from the query.Currently, if the attribute list query fails, the component will remain in the skeleton state indefinitely since
isLoadingbecomesfalsebutattributeListremains undefined.-const RoomFormAttributeFields = ({ fields, remove }: RoomFormAttributeFieldsProps) => { +const RoomFormAttributeFields = ({ fields, remove }: RoomFormAttributeFieldsProps) => { const { t } = useTranslation(); - const { data: attributeList, isLoading } = useAttributeList(); + const { data: attributeList, isLoading, isError } = useAttributeList(); - if (isLoading || !attributeList) { + if (isLoading) { return <InputBoxSkeleton />; } + + if (isError || !attributeList) { + return null; // Or render an error message/callout + }apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (1)
16-27: Consider error resilience for parallel fetches.Using
Promise.allmeans if any subsequent page request fails, the entire query fails. For a more resilient approach, considerPromise.allSettledwith filtering of successful results, though the current all-or-nothing behavior may be intentional to ensure complete data.- const remainingPages = await Promise.all(pages); + const results = await Promise.allSettled(pages); + const remainingPages = results + .filter((result): result is PromiseFulfilledResult<Awaited<ReturnType<typeof attributesAutoCompleteEndpoint>>> => result.status === 'fulfilled') + .map((result) => result.value);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/__snapshots__/RoomFormAttributeField.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.spec.tsx(0 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.stories.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts(1 hunks)apps/meteor/ee/server/api/abac/schemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.spec.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/ABACRoomsTab/RoomFormAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts
🧠 Learnings (12)
📓 Common learnings
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: 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: 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.
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/ABACRoomsTab/RoomFormAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts
📚 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/ABACRoomsTab/RoomFormAttributeField.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.ts
📚 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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.spec.tsx
🧬 Code graph analysis (5)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx (1)
apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (1)
useAttributeList(9-39)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (1)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (1)
RoomFormData(21-24)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (1)
RoomFormData(21-24)
apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (2)
apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.ts (1)
useIsABACAvailable(5-10)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(142-159)
⏰ 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 (10)
apps/meteor/ee/server/api/abac/schemas.ts (1)
67-69: LGTM - Type now correctly reflects schema behavior.The type annotation now properly reflects that
countis optional in the runtime schema (norequiredarray means all properties are optional). This aligns with the refactoreduseAttributeListhook usage.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.stories.tsx (1)
36-68: LGTM - Story correctly adapted to new prop-based data flow.The decorator simplification (removing endpoint mocking) and addition of
attributeListin args properly aligns with the refactored component that now receives attribute data via props rather than fetching internally.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx (1)
62-136: LGTM - Comprehensive test coverage for the new component.The test suite covers key scenarios: rendering correct field counts, single/multiple fields, and pre-populated default values. The mock setup and FormProviderWrapper pattern are well-structured.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (2)
10-10: LGTM - Clean component delegation.
107-107: Good refactor - attribute fields rendering now centralized.Delegating to
RoomFormAttributeFieldssimplifies this component and properly centralizes the attribute list fetching logic, addressing the pagination issue where selected values beyond the first page weren't displayed.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx (1)
21-34: LGTM - Clean field mapping with proper key and props.The component correctly maps fields with stable keys and passes the required props to each
RoomFormAttributeField.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (3)
1-3: Good simplification of imports.Switching from
PaginatedSelectFilteredtoSelectFilteredis appropriate since attribute options are now pre-loaded and passed via props, eliminating the need for pagination controls.
67-70: LGTM - Values correctly reset when attribute changes.Using
resetFieldwithdefaultValue: []ensures stale values from a previously selected attribute are cleared when the user switches to a different attribute, addressing the linked issue ABAC-99.
47-55: LGTM - Value options derived correctly from attribute list.The memoization properly depends on both
attributeListandkeyField.value, ensuring options update when either changes.apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (1)
9-38: Refactor addresses the pagination display issue effectively.The approach of fetching all pages upfront and combining them into a single list directly solves the linked issue (ABAC-99) where selected values weren't displayed when they resided beyond the first page. The parallel fetch strategy with
Promise.allis efficient for minimizing total wait time.
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-99
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.