feat: Virtru as attribute store#40634
Merged
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2026-03-27T14:52:56.865ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (3)
WalkthroughAdds pluggable ABAC attribute-store support (local and Virtru): new VirtruClient and identity helpers, attribute-store interface with Local/Virtru implementations, AbacService refactor for store selection/delegation and transitions (with audit), server API scoping/guards, client UI/redaction handling, settings/permission changes, i18n, and extensive tests. ChangesABAC Pluggable Attribute Store Implementation
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
|
🦋 Changeset detectedLatest commit: 7e3fa4d The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40634 +/- ##
===========================================
+ Coverage 69.76% 69.86% +0.09%
===========================================
Files 3327 3333 +6
Lines 123134 123455 +321
Branches 21963 22009 +46
===========================================
+ Hits 85902 86246 +344
+ Misses 33873 33849 -24
- Partials 3359 3360 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
force-pushed
the
feat/abac-virtru-attribute-store
branch
2 times, most recently
from
b2dc287 to
2a00e01
Compare
|
Warning These are security findings reported by the security scanners configured in Layne. Findings may contain false positives - review them and fix what makes sense. Layne found 3 high issues in this PR. View 3 finding(s)
|
Member
|
/layne exception-approve LAYNE-8598b68f3ce8435a LAYNE-e568b73325f960fd LAYNE-5929c5f8cb7138d7 reason: these are acceptable and valid ignoreSsrfValidation entries, not vulnerabilities |
|
✅ Exception recorded for LAYNE-8598b68f3ce8435a, LAYNE-e568b73325f960fd, LAYNE-5929c5f8cb7138d7 by @julio-rocketchat: "these are acceptable and valid ignoreSsrfValidation entries, not vulnerabilities". Re-running scan... |
KevLehman
force-pushed
the
feat/abac-virtru-attribute-store
branch
from
3d5c112 to
85976be
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a pluggable ABAC “attribute store” with a new Virtru-backed implementation, enabling externally managed room-attribute entitlements and redaction of ABAC attributes for admins who don’t possess the required entitlements. It also adds auditing for attribute-store transitions and UI/REST typing updates to surface the new redaction flag.
Changes:
- Add
LocalAttributeStore/VirtruAttributeStoreabstractions, selection logic inAbacService, and Virtru client/identity helpers. - Implement room scoping/redaction (
abacAttributesRedacted) across ABAC admin UI and admin room endpoints, plus a new audit event for attribute-store switching (with automatic wipe). - Add settings/i18n keys and expand tests (unit + E2E) for Virtru attribute-store behavior.
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rest-typings/src/v1/rooms.ts | Adds abacAttributesRedacted typing to admin-room endpoint response. |
| packages/logger/src/index.ts | Allows default log level to be initialized from process.env.LOG_LEVEL. |
| packages/i18n/src/locales/en.i18n.json | Adds strings for attribute-store UI, redaction placeholder, and audit labels. |
| packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts | Adds attribute-store type/reason and new audit event type. |
| packages/core-typings/src/IServerEvent.ts | Registers abac.attribute.store.switched server event. |
| packages/core-typings/src/IRoom.ts | Introduces IRoomAbacRedaction and combined admin type. |
| packages/core-typings/src/Abac.ts | Adds type guards for PDP and attribute-store settings. |
| packages/core-services/src/types/IAbacService.ts | Extends ABAC service contract for scoping, redaction, and store detection. |
| ee/packages/abac/src/user-auto-removal.spec.ts | Updates ABAC EE tests to include License module mocking. |
| ee/packages/abac/src/store/VirtruAttributeStore.ts | Implements Virtru-backed attribute listing, entitlements, scoping, and write-guards. |
| ee/packages/abac/src/store/VirtruAttributeStore.spec.ts | Unit tests for Virtru attribute store caching, scoping, and guards. |
| ee/packages/abac/src/store/types.ts | Adds attribute-store interfaces and selection-context types. |
| ee/packages/abac/src/store/LocalAttributeStore.ts | Implements local attribute-store behavior (DB-backed catalog, no scoping). |
| ee/packages/abac/src/store/LocalAttributeStore.spec.ts | Unit tests for local attribute store behavior. |
| ee/packages/abac/src/store/index.ts | Exports attribute store implementations/types. |
| ee/packages/abac/src/service.spec.ts | Updates ABAC service tests for store delegation, scoping, transitions, and wipe/audit behavior. |
| ee/packages/abac/src/pdp/VirtruPDP.ts | Refactors Virtru PDP to use shared VirtruClient + identity helpers. |
| ee/packages/abac/src/pdp/types.ts | Adds Virtru entitlements request/response typings. |
| ee/packages/abac/src/index.ts | Adds attribute-store selection, transition wipe, scoping APIs, and write guards. |
| ee/packages/abac/src/helper.ts | Exposes stripTrailingSlashes helper for config normalization. |
| ee/packages/abac/src/errors.ts | Adds new ABAC error codes for external-store and authorization cases. |
| ee/packages/abac/src/clients/virtru/VirtruClient.ts | Introduces reusable Virtru client (health check, token cache, API calls). |
| ee/packages/abac/src/clients/virtru/VirtruClient.spec.ts | Unit tests for VirtruClient token caching and request behavior. |
| ee/packages/abac/src/clients/virtru/identity.ts | Adds entity identifier + FQN build/parse helpers. |
| ee/packages/abac/src/clients/virtru/identity.spec.ts | Unit tests for Virtru identity helpers. |
| ee/packages/abac/src/audit.ts | Adds audit helper for attribute-store switch event emission. |
| apps/meteor/tests/end-to-end/api/abac.ts | Adds extensive E2E coverage for Virtru attribute-store behavior (mock server). |
| apps/meteor/tests/data/mock-server.helper.ts | Adds seedGetEntitlements mock helper for Virtru entitlements endpoint. |
| apps/meteor/ee/server/settings/abac.ts | Adds ABAC_Attribute_Store setting (select + warning alert). |
| apps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.ts | Patches admin-room list scoping/redaction for ABAC-managed rooms (EE). |
| apps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.ts | Patches single admin-room scoping/redaction (EE). |
| apps/meteor/ee/server/hooks/abac/index.ts | Registers ABAC EE hooks for scoping. |
| apps/meteor/ee/server/api/abac/schemas.ts | Updates ABAC API schemas for redaction and new audit event. |
| apps/meteor/ee/server/api/abac/index.ts | Blocks local catalog CRUD when external store is effective; extends audit filtering. |
| apps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.ts | Adds client-side external-store detection for UI gating. |
| apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts | Adds query staleTime for attribute list caching. |
| apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx | Hides Room Attributes tab when external attribute store is active. |
| apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx | Prevents navigation to Room Attributes tab when external store is active. |
| apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx | Shows attribute-store setting when PDP is non-local. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx | Displays redaction placeholder when abacAttributesRedacted is set. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsx | Passes redacted state to contextual bar. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx | Threads redacted flag down to the room form. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsx | Excludes redacted rooms from autocomplete selection. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx | Shows external-store helper + supports disabling fields (redacted). |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx | Adds tests for disabled state rendering. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx | Adds disabled prop to key/value inputs and remove button. |
| apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx | Disables edits and shows warning callout when room attributes are redacted. |
| apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx | Adds rendering for attribute-store switched audit events. |
| apps/meteor/app/api/server/v1/rooms.ts | Extends admin-room endpoints to return redaction flag + scopes adminRooms listing. |
| apps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.ts | Adds patch injection point for scoping admin-room pages. |
| apps/meteor/app/api/server/lib/scopeAdminRoomForAbac.ts | Adds patch injection point for scoping a single admin-room. |
| apps/meteor/app/api/server/lib/rooms.ts | Applies per-room scoping hook when fetching an admin room. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
KevLehman
force-pushed
the
feat/abac-virtru-attribute-store
branch
from
2a04c9c to
4a2069d
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/packages/abac/src/index.ts (1)
533-546:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t let
{}bypass the room write guard.The empty-clear path returns before
store.assertCanModifyRoom(room, actor). A caller that the active store would reject can still send an empty attribute map and remove every ABAC attribute from the room without passing the external authorization check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/abac/src/index.ts` around lines 533 - 546, The early-return path in setRoomAbacAttributes lets callers bypass authorization: before unsetting attributes it checks attributes.length and returns without calling store.assertCanModifyRoom, so someone can pass {} to clear attributes without permission. Fix by enforcing the store guard before the empty-clear branch—call await store.assertCanModifyRoom(room, actor) (or move the existing call) prior to checking and potentially calling Rooms.unsetAbacAttributesById and Audit.objectAttributesRemoved so the write guard always runs for any attribute change.ee/packages/abac/src/pdp/VirtruPDP.ts (1)
194-234:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail when some requested usernames were never resolved.
Users.findByUsernames(usernames)can return fewer rows than were requested, and those missing usernames are silently omitted fromdecisionRequests. In that case this method can resolve successfully andAbacService.checkUsernamesMatchAttributes()will audit every supplied username as compliant, even though some were never evaluated at all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/abac/src/pdp/VirtruPDP.ts` around lines 194 - 234, The code currently builds decisionRequests only for rows returned by Users.findByUsernames(usernames) and silently ignores any requested usernames that weren't found; before building decisionRequests (or after fetching users) compare the input usernames array to the resolved users (from Users.findByUsernames) to detect missing usernames, and if any are missing throw OnlyCompliantCanBeAddedToRoomError (or a more specific error) so the method fails instead of treating unresolved usernames as compliant; update the logic around Users.findByUsernames, the users variable, and the subsequent decisionRequests loop (which uses getUserEntityKey, buildAttributeFqns, and buildEntityIdentifier) to ensure you validate resolution first.
🧹 Nitpick comments (3)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
27-31: 💤 Low valueConsider extracting the tab-exclusion logic to reduce duplication.
The condition
!(t === 'room-attributes' && isExternalStore)appears in both thefirstAllowedTabcomputation and theisAllowedTabcheck. Extracting it to a helper function would improve maintainability.♻️ Optional refactor to reduce duplication
+ const isTabAllowed = (tab: string): boolean => { + return tabPermissions[tab as ABACTab] && !(tab === 'room-attributes' && isExternalStore); + }; + - const firstAllowedTab = ABAC_TAB_ORDER.find((t) => tabPermissions[t] && !(t === 'room-attributes' && isExternalStore)); + const firstAllowedTab = ABAC_TAB_ORDER.find(isTabAllowed); const isAllowedTab = - (ABAC_TAB_ORDER as readonly string[]).includes(tab ?? '') && - tabPermissions[tab as ABACTab] && - !(tab === 'room-attributes' && isExternalStore); + (ABAC_TAB_ORDER as readonly string[]).includes(tab ?? '') && isTabAllowed(tab ?? '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx` around lines 27 - 31, The tab-exclusion check "!(t === 'room-attributes' && isExternalStore)" is duplicated in the computations for firstAllowedTab and isAllowedTab; extract that logic into a small helper function (e.g., isTabAllowedOrExcluded(tab: string | undefined): boolean or isRoomAttributesVisible(tab: string): boolean) and use it in both places so ABAC_TAB_ORDER.find(...) and the isAllowedTab expression both call the helper; ensure the helper accepts the same types used by ABAC_TAB_ORDER/tab/tabPermissions and still checks tabPermissions[tab as ABACTab] where needed.ee/packages/abac/src/clients/virtru/VirtruClient.spec.ts (1)
54-54: ⚡ Quick winPrefer explicit typing over
as anyfor type safety.The type assertion
as anybypasses TypeScript's type checking. Consider using explicit typing for the fetch options.♻️ Suggested improvement
- expect((apiCallArgs?.[1] as any).headers.Authorization).toBe('Bearer tok'); + const fetchOptions = apiCallArgs?.[1] as { headers: { Authorization: string } }; + expect(fetchOptions?.headers.Authorization).toBe('Bearer tok');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/abac/src/clients/virtru/VirtruClient.spec.ts` at line 54, The test currently uses a permissive cast "(apiCallArgs?.[1] as any)" which bypasses type checking; replace the `as any` with the appropriate fetch options type (e.g., RequestInit or the specific fetch options interface used in your project) and/or type the `apiCallArgs` variable as a tuple like [string, RequestInit?], then assert the header via `(apiCallArgs?.[1] as RequestInit).headers.Authorization` or preferably `(apiCallArgs?.[1] as RequestInit)?.headers['Authorization']`; update the VirtruClient.spec.ts test to use that explicit typing so TypeScript can validate the shape of the fetch options.ee/packages/abac/src/store/LocalAttributeStore.ts (1)
55-57: ⚡ Quick winRemove inline no-op comment from implementation.
Prefer an empty/explicit no-op body without a code comment to keep implementation aligned with repository conventions.
As per coding guidelines "Avoid code comments in the implementation".Proposed change
async assertCanModifyRoom(_room: Pick<IRoom, '_id' | 'abacAttributes'>, _actor: AbacActor): Promise<void> { - // nop + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/abac/src/store/LocalAttributeStore.ts` around lines 55 - 57, The method LocalAttributeStore.assertCanModifyRoom currently contains an inline no-op comment; remove the comment and leave an explicit empty method body (no comment) to match repository conventions—update the async assertCanModifyRoom(_room: Pick<IRoom, '_id' | 'abacAttributes'>, _actor: AbacActor): Promise<void> implementation to be an empty/explicit no-op without any inline comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.ts`:
- Around line 15-17: The current early-return when user is missing in
scopeAdminRoomsForAbac (if (!user) { return next(rooms, uid); }) falls back to
unscoped results; instead filter out managed rooms and pass the filtered list to
next so managed-room visibility is denied. Replace the return next(rooms, uid)
with code that builds filteredRooms = rooms.filter(r => !r.managed) (or the
actual room property used to mark managed rooms) and call return
next(filteredRooms, uid); ensure you reference the existing variables user,
rooms and the next(...) callback.
In `@ee/packages/abac/src/clients/virtru/VirtruClient.ts`:
- Around line 94-115: The PDP request/response payloads are being logged
directly by virtruClientLogger in the VirtruClient code (see uses of
virtruClientLogger.debug and virtruClientLogger.error around the serverFetch
call); remove raw body and raw response/data from logs and instead log only
non-sensitive metadata (endpoint, status, sizes) or a sanitized/redacted
summary. Implement or call a helper (e.g., redactSensitivePayload or
sanitizeForLogging) and use it in the two debug calls and the error log so you
never emit the full JSON body or response text — for errors log response.status
plus a redacted/truncated response indicator, and for success log endpoint and a
redacted summary (or length) instead of the full data. Ensure serverFetch
logging points (before request and after response) reference the redaction
helper when including payload info.
In `@ee/packages/abac/src/index.ts`:
- Around line 112-114: The ABAC_Enabled setting listener only flips
this.abacEnabled but doesn't re-evaluate the active attribute store or trigger
transition logic; update the onSettingChanged('ABAC_Enabled', ...) handler to,
after setting this.abacEnabled, call the code that recomputes the effective
store (e.g., invoke resolveAttributeStore()) and then call
onAttributeStoreTransition(...) with the old and new store values so the
wipe/audit path executes; reference the existing methods resolveAttributeStore
and onAttributeStoreTransition and ensure you preserve async/await behavior and
error handling when invoking them.
- Around line 194-202: computeEffectiveStoreType currently hardcodes licensed:
true which diverges from resolveAttributeStore's real license check and can
mis-detect transitions; change computeEffectiveStoreType so it uses the live
license state instead of true (for example by making it async and calling
License.hasModule('abac') or by accepting a licensed boolean parameter
propagated from resolveAttributeStore), then use that real licensed value in the
AttributeStoreSelectionContext and eligibility check for
attributeStores[selected].isEligible(ctx).
In `@ee/packages/abac/src/service.spec.ts`:
- Around line 1313-1382: The tests labeled "virtru mode" are stubbing
attributeStores.local.store so they pass even if resolveAttributeStore() no
longer picks Virtru; instead wire the fake store to attributeStores.virtru.store
(not local) and set the same effective-store / licensing flags the code path
uses (same pattern as scopeRoomsForAdmin) so resolveAttributeStore() will choose
the Virtru store; update the three virtru-mode tests to assign (service as
any).attributeStores.virtru.store = fakeStore and set the service's
effective-store/license/feature flags exactly as scopeRoomsForAdmin does before
calling listAbacRooms.
---
Outside diff comments:
In `@ee/packages/abac/src/index.ts`:
- Around line 533-546: The early-return path in setRoomAbacAttributes lets
callers bypass authorization: before unsetting attributes it checks
attributes.length and returns without calling store.assertCanModifyRoom, so
someone can pass {} to clear attributes without permission. Fix by enforcing the
store guard before the empty-clear branch—call await
store.assertCanModifyRoom(room, actor) (or move the existing call) prior to
checking and potentially calling Rooms.unsetAbacAttributesById and
Audit.objectAttributesRemoved so the write guard always runs for any attribute
change.
In `@ee/packages/abac/src/pdp/VirtruPDP.ts`:
- Around line 194-234: The code currently builds decisionRequests only for rows
returned by Users.findByUsernames(usernames) and silently ignores any requested
usernames that weren't found; before building decisionRequests (or after
fetching users) compare the input usernames array to the resolved users (from
Users.findByUsernames) to detect missing usernames, and if any are missing throw
OnlyCompliantCanBeAddedToRoomError (or a more specific error) so the method
fails instead of treating unresolved usernames as compliant; update the logic
around Users.findByUsernames, the users variable, and the subsequent
decisionRequests loop (which uses getUserEntityKey, buildAttributeFqns, and
buildEntityIdentifier) to ensure you validate resolution first.
---
Nitpick comments:
In `@apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx`:
- Around line 27-31: The tab-exclusion check "!(t === 'room-attributes' &&
isExternalStore)" is duplicated in the computations for firstAllowedTab and
isAllowedTab; extract that logic into a small helper function (e.g.,
isTabAllowedOrExcluded(tab: string | undefined): boolean or
isRoomAttributesVisible(tab: string): boolean) and use it in both places so
ABAC_TAB_ORDER.find(...) and the isAllowedTab expression both call the helper;
ensure the helper accepts the same types used by
ABAC_TAB_ORDER/tab/tabPermissions and still checks tabPermissions[tab as
ABACTab] where needed.
In `@ee/packages/abac/src/clients/virtru/VirtruClient.spec.ts`:
- Line 54: The test currently uses a permissive cast "(apiCallArgs?.[1] as any)"
which bypasses type checking; replace the `as any` with the appropriate fetch
options type (e.g., RequestInit or the specific fetch options interface used in
your project) and/or type the `apiCallArgs` variable as a tuple like [string,
RequestInit?], then assert the header via `(apiCallArgs?.[1] as
RequestInit).headers.Authorization` or preferably `(apiCallArgs?.[1] as
RequestInit)?.headers['Authorization']`; update the VirtruClient.spec.ts test to
use that explicit typing so TypeScript can validate the shape of the fetch
options.
In `@ee/packages/abac/src/store/LocalAttributeStore.ts`:
- Around line 55-57: The method LocalAttributeStore.assertCanModifyRoom
currently contains an inline no-op comment; remove the comment and leave an
explicit empty method body (no comment) to match repository conventions—update
the async assertCanModifyRoom(_room: Pick<IRoom, '_id' | 'abacAttributes'>,
_actor: AbacActor): Promise<void> implementation to be an empty/explicit no-op
without any inline comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca0b0c7d-29ab-4777-82e8-b276af8e3a5a
📒 Files selected for processing (51)
apps/meteor/app/api/server/lib/rooms.tsapps/meteor/app/api/server/lib/scopeAdminRoomForAbac.tsapps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.tsapps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/ee/server/hooks/abac/index.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.tsapps/meteor/ee/server/settings/abac.tsapps/meteor/tests/data/mock-server.helper.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/audit.tsee/packages/abac/src/clients/virtru/VirtruClient.spec.tsee/packages/abac/src/clients/virtru/VirtruClient.tsee/packages/abac/src/clients/virtru/identity.spec.tsee/packages/abac/src/clients/virtru/identity.tsee/packages/abac/src/errors.tsee/packages/abac/src/helper.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsee/packages/abac/src/pdp/types.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/store/LocalAttributeStore.spec.tsee/packages/abac/src/store/LocalAttributeStore.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsee/packages/abac/src/store/VirtruAttributeStore.tsee/packages/abac/src/store/index.tsee/packages/abac/src/store/types.tsee/packages/abac/src/user-auto-removal.spec.tspackages/core-services/src/types/IAbacService.tspackages/core-typings/src/Abac.tspackages/core-typings/src/IRoom.tspackages/core-typings/src/IServerEvent.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tspackages/i18n/src/locales/en.i18n.jsonpackages/rest-typings/src/v1/rooms.ts
📜 Review details
⏰ 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). (14)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test API Livechat (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test API Livechat (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test Federation Matrix
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.0 (1/1)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/RoomFormAutocomplete.tsxapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsxapps/meteor/app/api/server/lib/scopeAdminRoomForAbac.tsapps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.tsee/packages/abac/src/clients/virtru/VirtruClient.spec.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsxapps/meteor/ee/server/settings/abac.tspackages/core-typings/src/IRoom.tspackages/core-typings/src/IServerEvent.tsapps/meteor/app/api/server/lib/rooms.tsapps/meteor/ee/server/hooks/abac/index.tsee/packages/abac/src/errors.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.tsapps/meteor/tests/data/mock-server.helper.tsee/packages/abac/src/clients/virtru/identity.spec.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.tspackages/core-typings/src/Abac.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/helper.tsapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxee/packages/abac/src/store/types.tsee/packages/abac/src/audit.tsee/packages/abac/src/store/LocalAttributeStore.tsee/packages/abac/src/store/LocalAttributeStore.spec.tspackages/rest-typings/src/v1/rooms.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxpackages/core-services/src/types/IAbacService.tsee/packages/abac/src/store/index.tsee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.tsee/packages/abac/src/clients/virtru/VirtruClient.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/store/VirtruAttributeStore.tsee/packages/abac/src/pdp/types.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/clients/virtru/identity.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/clients/virtru/VirtruClient.spec.tsee/packages/abac/src/clients/virtru/identity.spec.tsee/packages/abac/src/store/LocalAttributeStore.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsee/packages/abac/src/service.spec.ts
🧠 Learnings (12)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsxapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsxapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsxapps/meteor/app/api/server/lib/scopeAdminRoomForAbac.tsapps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.tsee/packages/abac/src/clients/virtru/VirtruClient.spec.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsxapps/meteor/ee/server/settings/abac.tspackages/core-typings/src/IRoom.tspackages/core-typings/src/IServerEvent.tsapps/meteor/app/api/server/lib/rooms.tsapps/meteor/ee/server/hooks/abac/index.tsee/packages/abac/src/errors.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.tsapps/meteor/tests/data/mock-server.helper.tsee/packages/abac/src/clients/virtru/identity.spec.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.tspackages/core-typings/src/Abac.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/helper.tsapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxee/packages/abac/src/store/types.tsee/packages/abac/src/audit.tsee/packages/abac/src/store/LocalAttributeStore.tsee/packages/abac/src/store/LocalAttributeStore.spec.tspackages/rest-typings/src/v1/rooms.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxpackages/core-services/src/types/IAbacService.tsee/packages/abac/src/store/index.tsee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.tsee/packages/abac/src/clients/virtru/VirtruClient.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/store/VirtruAttributeStore.tsee/packages/abac/src/pdp/types.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/clients/virtru/identity.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/lib/scopeAdminRoomForAbac.tsapps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.tsee/packages/abac/src/clients/virtru/VirtruClient.spec.tsapps/meteor/ee/server/settings/abac.tspackages/core-typings/src/IRoom.tspackages/core-typings/src/IServerEvent.tsapps/meteor/app/api/server/lib/rooms.tsapps/meteor/ee/server/hooks/abac/index.tsee/packages/abac/src/errors.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.tsapps/meteor/tests/data/mock-server.helper.tsee/packages/abac/src/clients/virtru/identity.spec.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.tspackages/core-typings/src/Abac.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/helper.tsee/packages/abac/src/store/types.tsee/packages/abac/src/audit.tsee/packages/abac/src/store/LocalAttributeStore.tsee/packages/abac/src/store/LocalAttributeStore.spec.tspackages/rest-typings/src/v1/rooms.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/store/index.tsee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.tsee/packages/abac/src/clients/virtru/VirtruClient.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/store/VirtruAttributeStore.tsee/packages/abac/src/pdp/types.tsapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/clients/virtru/identity.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/lib/scopeAdminRoomForAbac.tsapps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.tsee/packages/abac/src/clients/virtru/VirtruClient.spec.tsapps/meteor/ee/server/settings/abac.tspackages/core-typings/src/IRoom.tspackages/core-typings/src/IServerEvent.tsapps/meteor/app/api/server/lib/rooms.tsapps/meteor/ee/server/hooks/abac/index.tsee/packages/abac/src/errors.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.tsapps/meteor/tests/data/mock-server.helper.tsee/packages/abac/src/clients/virtru/identity.spec.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.tspackages/core-typings/src/Abac.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/helper.tsee/packages/abac/src/store/types.tsee/packages/abac/src/audit.tsee/packages/abac/src/store/LocalAttributeStore.tsee/packages/abac/src/store/LocalAttributeStore.spec.tspackages/rest-typings/src/v1/rooms.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/store/index.tsee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.tsee/packages/abac/src/clients/virtru/VirtruClient.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/store/VirtruAttributeStore.tsee/packages/abac/src/pdp/types.tsapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/clients/virtru/identity.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/clients/virtru/VirtruClient.spec.tsee/packages/abac/src/clients/virtru/identity.spec.tsee/packages/abac/src/store/LocalAttributeStore.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/abac/src/clients/virtru/VirtruClient.spec.tsee/packages/abac/src/clients/virtru/identity.spec.tsee/packages/abac/src/store/LocalAttributeStore.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/abac/src/clients/virtru/VirtruClient.spec.tsee/packages/abac/src/clients/virtru/identity.spec.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxee/packages/abac/src/store/LocalAttributeStore.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.
Applied to files:
packages/rest-typings/src/v1/rooms.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
🪛 OpenGrep (1.22.0)
ee/packages/abac/src/clients/virtru/identity.ts
[ERROR] 36-36: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (45)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsx (1)
31-31: LGTM!apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx (1)
117-124: LGTM!apps/meteor/app/api/server/lib/scopeAdminRoomForAbac.ts (1)
4-6: LGTM!apps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.ts (1)
3-8: LGTM!apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx (1)
16-16: LGTM!Also applies to: 21-21, 84-84
apps/meteor/ee/server/hooks/abac/scopeAdminRoomForAbac.ts (1)
8-20: LGTM!ee/packages/abac/src/clients/virtru/VirtruClient.spec.ts (2)
1-18: LGTM!
19-90: LGTM!apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsx (1)
25-31: LGTM!packages/core-typings/src/IRoom.ts (1)
391-393: LGTM!packages/core-typings/src/IServerEvent.ts (1)
6-6: LGTM!Also applies to: 76-76
apps/meteor/app/api/server/lib/rooms.ts (1)
1-1: LGTM!Also applies to: 5-5, 60-77
apps/meteor/ee/server/hooks/abac/index.ts (1)
2-3: LGTM!ee/packages/abac/src/errors.ts (1)
16-18: LGTM!Also applies to: 107-123
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
6-18: LGTM!Also applies to: 84-92
apps/meteor/ee/server/settings/abac.ts (1)
32-44: ⚡ Quick winABAC_Attribute_Store enableQuery is intentionally tied to Virtru PDP
enableQuery: virtruPdpQuerycorrectly limitsABAC_Attribute_Storeto cases whereABAC_Enabled=trueandABAC_PDP_Type='virtru'.- The ABAC service also enforces this at runtime: when PDP switches to
local, it cascadesABAC_Attribute_Storeback tolocal, and when resolving the store it falls back to the local store if the selected store isn’t eligible for the current context.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx (1)
21-21: LGTM!apps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.ts (1)
1-7: LGTM!apps/meteor/tests/data/mock-server.helper.ts (1)
74-82: LGTM!ee/packages/abac/src/clients/virtru/identity.spec.ts (1)
1-37: LGTM!apps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.ts (1)
1-14: LGTM!Also applies to: 19-22
packages/core-typings/src/Abac.ts (1)
1-1: LGTM!Also applies to: 13-15
apps/meteor/ee/server/api/abac/index.ts (1)
1-1: LGTM!Also applies to: 44-48, 253-254, 280-281, 301-303, 323-325, 345-347, 436-442, 461-461
ee/packages/abac/src/helper.ts (1)
320-320: LGTM!apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
6-7: LGTM!Also applies to: 13-14, 27-31
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx (1)
46-49: LGTM!Also applies to: 139-170
ee/packages/abac/src/store/types.ts (1)
1-33: LGTM!ee/packages/abac/src/audit.ts (1)
10-11: LGTM!Also applies to: 152-154
ee/packages/abac/src/store/LocalAttributeStore.ts (1)
1-55: LGTM!Also applies to: 58-58
ee/packages/abac/src/store/LocalAttributeStore.spec.ts (1)
1-60: LGTM!packages/rest-typings/src/v1/rooms.ts (1)
4-5: LGTM!Also applies to: 909-910
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (1)
12-13: LGTM!Also applies to: 36-37, 66-67, 121-129
packages/core-services/src/types/IAbacService.ts (1)
5-5: LGTM!Also applies to: 33-35, 53-53
ee/packages/abac/src/store/index.ts (1)
1-3: LGTM!ee/packages/abac/src/user-auto-removal.spec.ts (1)
23-25: LGTM!apps/meteor/app/api/server/v1/rooms.ts (1)
4-4: LGTM!Also applies to: 93-93, 790-800, 1459-1459, 1497-1497
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (1)
1-1: LGTM!Also applies to: 19-19, 27-27, 114-121, 124-124, 135-135
packages/i18n/src/locales/en.i18n.json (1)
29-38: LGTM!Also applies to: 131-134
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx (1)
1-2: LGTM!Also applies to: 11-11, 14-17, 24-49
apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (1)
8-8: LGTM!Also applies to: 16-16
ee/packages/abac/src/store/VirtruAttributeStore.spec.ts (1)
1-279: LGTM!apps/meteor/tests/end-to-end/api/abac.ts (1)
17-17: LGTM!Also applies to: 3497-4201
ee/packages/abac/src/store/VirtruAttributeStore.ts (1)
22-192: LGTM!ee/packages/abac/src/pdp/types.ts (1)
72-88: LGTM!apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (1)
15-15: LGTM!Also applies to: 18-25, 82-82, 106-106, 115-115
KevLehman
force-pushed
the
feat/abac-virtru-attribute-store
branch
from
68d3760 to
5d478cf
Compare
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx (1)
16-29: ⚡ Quick winReuse the shared external-store hook here.
This banner is keyed only off
ABAC_Attribute_Store, but the client layer already introduceduseIsExternalAttributeStoreto derive external mode from the full configuration. Reusing that hook here avoids the form hint drifting from the tab/route gating logic.Suggested refactor
import { Box, Field, FieldLabel, InputBoxSkeleton } from '`@rocket.chat/fuselage`'; -import { useSetting } from '`@rocket.chat/ui-contexts`'; import { useTranslation } from 'react-i18next'; import RoomFormAttributeField from './RoomFormAttributeField'; import { useAttributeList } from '../hooks/useAttributeList'; +import { useIsExternalAttributeStore } from '../hooks/useIsExternalAttributeStore'; type RoomFormAttributeFieldsProps = { fields: { id: string }[]; remove: (index: number) => void; disabled?: boolean; }; const RoomFormAttributeFields = ({ fields, remove, disabled = false }: RoomFormAttributeFieldsProps) => { const { t } = useTranslation(); - const attributeStore = useSetting('ABAC_Attribute_Store', 'local'); + const isExternalAttributeStore = useIsExternalAttributeStore(); const { data: attributeList, isLoading } = useAttributeList(); if (isLoading || !attributeList) { return <InputBoxSkeleton />; } return ( <> - {attributeStore !== 'local' && ( + {isExternalAttributeStore && ( <Box mbe={8} color='annotation' fontSize='p2'> {t('ABAC_Picker_External_Store_Helper')} </Box> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx` around lines 16 - 29, The banner currently checks attributeStore via useSetting('ABAC_Attribute_Store', 'local') and can drift from the app-wide logic; replace that by using the shared hook useIsExternalAttributeStore (import it into RoomFormAttributeFields.tsx), remove the local useSetting call and the attributeStore variable, and condition the helper banner on the boolean returned by useIsExternalAttributeStore (e.g., if isExternalAttributeStore) so the form hint follows the same external-store derivation as the rest of the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/packages/abac/src/clients/virtru/identity.ts`:
- Around line 23-27: The FQNs built by buildAttributeFqns currently interpolate
attr.key and value as raw URL path segments which breaks on reserved characters;
update buildAttributeFqns to percent-encode each path segment (attr.key and
value) with encodeURIComponent (or equivalent) when constructing
`https://.../attr/{key}/value/{value}` and ensure the complementary parser
(parseAttributeFqns or any function in the same file that reads these FQNs) uses
decodeURIComponent to decode those segments back to the original key and value
so round-tripping works for characters like / ? # % and spaces.
---
Nitpick comments:
In
`@apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx`:
- Around line 16-29: The banner currently checks attributeStore via
useSetting('ABAC_Attribute_Store', 'local') and can drift from the app-wide
logic; replace that by using the shared hook useIsExternalAttributeStore (import
it into RoomFormAttributeFields.tsx), remove the local useSetting call and the
attributeStore variable, and condition the helper banner on the boolean returned
by useIsExternalAttributeStore (e.g., if isExternalAttributeStore) so the form
hint follows the same external-store derivation as the rest of the client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 474053dc-0493-47d8-af5a-1630d7509007
📒 Files selected for processing (51)
apps/meteor/app/api/server/lib/rooms.tsapps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/hooks/useAttributeList.tsapps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/ee/server/hooks/abac/index.tsapps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.tsapps/meteor/ee/server/lib/abac/index.tsapps/meteor/ee/server/settings/abac.tsapps/meteor/tests/data/mock-server.helper.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/audit.tsee/packages/abac/src/clients/virtru/VirtruClient.spec.tsee/packages/abac/src/clients/virtru/VirtruClient.tsee/packages/abac/src/clients/virtru/identity.spec.tsee/packages/abac/src/clients/virtru/identity.tsee/packages/abac/src/errors.tsee/packages/abac/src/helper.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsee/packages/abac/src/pdp/types.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/store/LocalAttributeStore.spec.tsee/packages/abac/src/store/LocalAttributeStore.tsee/packages/abac/src/store/VirtruAttributeStore.spec.tsee/packages/abac/src/store/VirtruAttributeStore.tsee/packages/abac/src/store/index.tsee/packages/abac/src/store/types.tsee/packages/abac/src/user-auto-removal.spec.tspackages/core-services/src/types/IAbacService.tspackages/core-typings/src/Abac.tspackages/core-typings/src/IRoom.tspackages/core-typings/src/IServerEvent.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tspackages/i18n/src/locales/en.i18n.jsonpackages/rest-typings/src/v1/rooms.tsturbo.json
✅ Files skipped from review due to trivial changes (2)
- turbo.json
- packages/i18n/src/locales/en.i18n.json
🚧 Files skipped from review as they are similar to previous changes (37)
- apps/meteor/ee/server/settings/abac.ts
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsx
- ee/packages/abac/src/store/index.ts
- apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx
- ee/packages/abac/src/store/types.ts
- apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx
- packages/core-typings/src/IRoom.ts
- ee/packages/abac/src/clients/virtru/identity.spec.ts
- apps/meteor/client/views/admin/ABAC/hooks/useIsExternalAttributeStore.ts
- ee/packages/abac/src/pdp/types.ts
- apps/meteor/app/api/server/lib/scopeAdminRoomsForAbac.ts
- apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx
- ee/packages/abac/src/user-auto-removal.spec.ts
- ee/packages/abac/src/store/LocalAttributeStore.ts
- ee/packages/abac/src/audit.ts
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx
- apps/meteor/ee/server/api/abac/schemas.ts
- ee/packages/abac/src/errors.ts
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
- apps/meteor/ee/server/hooks/abac/scopeAdminRoomsForAbac.ts
- ee/packages/abac/src/helper.ts
- packages/core-typings/src/Abac.ts
- ee/packages/abac/src/store/LocalAttributeStore.spec.ts
- apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx
- ee/packages/abac/src/clients/virtru/VirtruClient.spec.ts
- apps/meteor/ee/server/api/abac/index.ts
- packages/core-services/src/types/IAbacService.ts
- apps/meteor/app/api/server/v1/rooms.ts
- packages/core-typings/src/IServerEvent.ts
- ee/packages/abac/src/store/VirtruAttributeStore.ts
- ee/packages/abac/src/store/VirtruAttributeStore.spec.ts
- ee/packages/abac/src/pdp/VirtruPDP.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/ee/server/hooks/abac/index.tsapps/meteor/ee/server/lib/abac/index.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/tests/data/mock-server.helper.tspackages/rest-typings/src/v1/rooms.tsapps/meteor/app/api/server/lib/rooms.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/clients/virtru/VirtruClient.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/clients/virtru/identity.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/service.spec.ts
🧠 Learnings (8)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/ee/server/hooks/abac/index.tsapps/meteor/ee/server/lib/abac/index.tsapps/meteor/tests/data/mock-server.helper.tspackages/rest-typings/src/v1/rooms.tsapps/meteor/app/api/server/lib/rooms.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/clients/virtru/VirtruClient.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/clients/virtru/identity.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/ee/server/hooks/abac/index.tsapps/meteor/ee/server/lib/abac/index.tsapps/meteor/tests/data/mock-server.helper.tspackages/rest-typings/src/v1/rooms.tsapps/meteor/app/api/server/lib/rooms.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/clients/virtru/VirtruClient.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/clients/virtru/identity.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/ee/server/hooks/abac/index.tsapps/meteor/ee/server/lib/abac/index.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/tests/data/mock-server.helper.tspackages/rest-typings/src/v1/rooms.tsapps/meteor/app/api/server/lib/rooms.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/clients/virtru/VirtruClient.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/clients/virtru/identity.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.
Applied to files:
packages/rest-typings/src/v1/rooms.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/abac/src/service.spec.ts
🪛 OpenGrep (1.22.0)
ee/packages/abac/src/clients/virtru/identity.ts
[ERROR] 36-36: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (43)
ee/packages/abac/src/clients/virtru/VirtruClient.ts (1)
94-114: Remove raw PDP payloads from logs.These log statements still emit request bodies, response bodies, and error text, which can contain entity identifiers and entitlement data.
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
6-17: LGTM!Also applies to: 84-92
packages/rest-typings/src/v1/rooms.ts (1)
4-4: LGTM!Also applies to: 905-909
ee/packages/abac/src/clients/virtru/identity.ts (1)
5-21: LGTM!ee/packages/abac/src/index.ts (13)
112-114: The past review noted that this listener does not recompute the effective store or callonAttributeStoreTransition. However, the test suite now explicitly documents thatABAC_Enabledchanges should not trigger store transitions (see tests at lines 1534-1542 and 1760-1770 in the spec file). This appears to be an intentional design decision—toggling ABAC enablement does not wipe room attributes.If this behavior is unintended, the tests would need correction; otherwise, the existing implementation aligns with the documented expectations.
1-47: LGTM!
66-83: LGTM!
89-110: LGTM!
116-166: LGTM!
169-227: LGTM!
229-237: LGTM!
239-283: LGTM!
349-433: LGTM!
526-569: LGTM!
571-628: LGTM!Also applies to: 630-659, 661-688, 690-735
814-815: LGTM!
963-963: LGTM!ee/packages/abac/src/service.spec.ts (9)
1233-1356: The tests now correctly stubattributeStores.virtru.store(lines 1292, 1326, 1343) instead of the local store, addressing the past review comment about ensuring the virtru store selection path is actually exercised.
1-99: LGTM!
343-358: LGTM!
821-822: LGTM!
1007-1126: LGTM!
1358-1451: LGTM!
1453-1564: LGTM!
1566-1807: LGTM!
1809-1880: LGTM!apps/meteor/app/api/server/lib/rooms.ts (4)
1-8: LGTM!
21-21: LGTM!
52-57: LGTM!
60-78: LGTM!apps/meteor/ee/server/hooks/abac/index.ts (1)
1-3: LGTM!apps/meteor/ee/server/lib/abac/index.ts (1)
10-10: LGTM!apps/meteor/tests/end-to-end/api/abac.ts (10)
17-17: LGTM!
3020-3021: LGTM!Also applies to: 3057-3058
3502-3521: LGTM!
3523-3579: LGTM!
3581-3719: LGTM!
3721-3831: LGTM!
3833-3892: LGTM!
3894-4035: LGTM!
4037-4135: LGTM!
4137-4274: LGTM!apps/meteor/tests/data/mock-server.helper.ts (1)
74-82: LGTM!
coderabbitai
Bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 52 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
coderabbitai
Bot
removed
the
type: feature
tassoevan
approved these changes
Jun 1, 2026
ricardogarim
requested changes
Jun 3, 2026
ricardogarim
approved these changes
Jun 3, 2026
KevLehman
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
gabriellsh
pushed a commit
that referenced
this pull request
Jun 5, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC3-4
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
Tests