-
Notifications
You must be signed in to change notification settings - Fork 13k
regression: ABAC table & form fixes #37782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughForm validation was changed to per-field, with form mode set to 'onChange'; duplicate attribute-value validation was added. UI cells and select components now truncate long text. Cache invalidation after save mutations was narrowed to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37782 +/- ##
============================================
Coverage ? 54.28%
============================================
Files ? 2633
Lines ? 50105
Branches ? 11224
============================================
Hits ? 27201
Misses ? 20727
Partials ? 2177
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
14-54: Fix validate callback return value and add defensive guards for watch() results.Lines 40–41: Add null coalescing to safely handle potential undefined values from
watch()before spreading them in line 50.Lines 47–54: In the
validateRepeatedValuescallback, returntrueon success instead ofundefined. According to react-hook-form 7.45.4 behavior, returningundefinedfrom a validate function can produce inconsistent validation state. Additionally, skip the duplicate-check for empty strings to let therequiredrule handle that case without redundant errors.- const attributeValues = watch('attributeValues'); - const lockedAttributes = watch('lockedAttributes'); + const attributeValues = watch('attributeValues') ?? []; + const lockedAttributes = watch('lockedAttributes') ?? []; const validateRepeatedValues = useCallback( (value: string) => { + if (!value) { + return true; + } - // Only one instance of the same attribute value is allowed to be in the form at a time - const repeatedAttributes = [...lockedAttributes, ...attributeValues].filter((attribute) => attribute.value === value).length > 1; - return repeatedAttributes ? t('ABAC_No_repeated_values') : undefined; + const repeatedAttributes = + [...lockedAttributes, ...attributeValues].filter((attribute) => attribute.value === value).length > 1; + return repeatedAttributes ? t('ABAC_No_repeated_values') : true; }, [lockedAttributes, attributeValues, t], );
🧹 Nitpick comments (2)
packages/i18n/src/locales/en.i18n.json (1)
55-57: Localization key addition looks correct; consider slightly more specific copy if needed.
ABAC_No_repeated_valuesis consistent with existing ABAC keys and the JSON placement/formatting looks fine. If this error can appear in contexts where “Value” is ambiguous, consider “Attribute value already added” for clarity.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
86-87: Truncation change looks right; consider adding a “full text” affordance (title/tooltip) for discoverability.
withTruncatedTextshould address the overflow regression, but users may need an easy way to see the full key/value (e.g.,title={attribute.key}/title={attribute.values.join(', ')}) when truncated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/__snapshots__/AttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 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.
📚 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/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 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/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR #36972 through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx (2)
packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
queryClient(269-279)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(142-159)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
packages/ui-contexts/src/index.ts (1)
useTranslation(81-81)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx (2)
packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
queryClient(269-279)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(142-159)
🔇 Additional comments (5)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (1)
109-114: LGTM! Truncation properly addresses overflow issue.The addition of
withTruncatedTextto both ABAC attribute cells correctly addresses the table overflow issue mentioned in the PR objectives.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx (2)
41-42:mode: 'onChange'aligns with per-field validation, but re-check validation cost/UX.
This will run validations on every change (including the repeated-values scan), which is probably intended here—just ensure there’s no noticeable lag with many values.
78-79: Verify TanStack Query invalidation behavior with parameterized queries in this context.The cache invalidation claim assumes
invalidateQueries({ queryKey: list() })won't match queries keyed withlist(query)where query is an object. However, TanStack Query's standard behavior for prefix matching typically would match['abac','room-attributes', undefined]to any key starting with['abac','room-attributes']regardless of the third element.Without access to the actual ABACQueryKeys implementation and how queries are constructed in this file, verify:
- Whether
roomAttributes.all()exists and what query keys it targets- Whether
roomAttributes.list(query)actually uses the query parameter in the key tuple- Whether the current
list()invalidation is insufficient by checking if stale query results persist after save operationsIf the issue is confirmed, update to use
roomAttributes.all()for broader invalidation.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (2)
104-140: Per-field errors are a nice improvement; verify the “remove”/min-length UX doesn’t create a submit-blocking state without a visible error.
You now allow removingattributeValues[0]whenlockedAttributesFields.length > 0(Line 134-136). If the field array still enforcesminLength: 1, that can block submit with only an array-level error (and you won’t render it via per-rowFieldError). Based on learnings about the “first value mandatory / no remove” pattern, please double-check this flow for both “create” and “edit”.
67-74: [rewritten comment]
[classification tag]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
105-121: Per-field validation improves UX, but note optional chaining inconsistency.The Fragment wrappers and per-field validation with
validateRepeatedValuesprovide clear, actionable error messages for each field. However, there's an inconsistency in how error messages are accessed:
- Line 120 uses optional chaining:
errors.lockedAttributes?.[index]?.value?.message- Line 138 omits optional chaining:
errors.attributeValues[index].value.messageWhile both should work (react-hook-form guarantees the structure), consider standardizing to use optional chaining consistently for defensive coding.
Apply this diff for consistency:
- {errors.attributeValues?.[index]?.value && <FieldError>{errors.attributeValues[index].value.message}</FieldError>} + {errors.attributeValues?.[index]?.value && <FieldError>{errors.attributeValues[index].value?.message}</FieldError>}Also applies to: 124-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR #36972 through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
📚 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.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR #36972 through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
⏰ 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 (4)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (1)
105-105: LGTM! Conditional error rendering improves UX.The change to only render the FieldError component when an error exists prevents rendering an empty error container, aligning with the per-field validation pattern applied across the ABAC forms in this PR.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (2)
78-78: LGTM! Text truncation prevents overflow.The addition of
withTruncatedTextto both select components directly addresses the PR objective of preventing cell overflow, ensuring long attribute names and values display cleanly without breaking the layout.Also applies to: 85-85
81-81: LGTM! Consistent error handling pattern.The conditional FieldError rendering aligns with the per-field validation approach used throughout the PR, preventing empty error components from being rendered.
Also applies to: 92-92
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
47-54: LGTM! Duplicate validation addresses PR objective.The
validateRepeatedValuesfunction correctly implements duplicate detection across both locked and editable attribute values, directly addressing the PR objective to "add validation for duplicated values." The validation prevents users from entering the same value multiple times within the form.
Fixes:
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-98
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Style
Localization
✏️ Tip: You can customize this high-level summary in your review settings.