regression(ABAC): Adjust forms spacing and accessibility. #37872
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughReplaced several ABAC admin form wrappers with ContextualbarScrollableContent-based forms, added per-field ARIA attributes and indexed FieldError alerts, increased attributes search debounce from 200ms to 400ms, changed cache invalidation to use ABACQueryKeys.roomAttributes.list({}), and forwarded new props to room attribute field components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MartinSchoeler
force-pushed
the
abac-space
branch
from
e67154c to
19556b8
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 09:35", "12/20 13:45 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 26 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sat, 20 Dec 2025 13:45:24 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37872 +/- ##
===========================================
- Coverage 67.77% 67.70% -0.07%
===========================================
Files 3476 3476
Lines 113895 113895
Branches 20956 20956
===========================================
- Hits 77190 77114 -76
- Misses 34508 34595 +87
+ Partials 2197 2186 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
26-26: Verify the debounce timing adjustment.The debounce delay has been doubled from 200ms to 400ms. While this reduces the frequency of search queries, it may impact the perceived responsiveness of the search feature. Confirm that 400ms provides the right balance between performance and user experience.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.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/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Learnt from: 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/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.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 (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
packages/ui-client/src/components/Contextualbar/index.ts (1)
ContextualbarScrollableContent(35-35)
🔇 Additional comments (3)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (3)
84-85: LGTM!The flattened form structure with ContextualbarScrollableContent as the form container is clean and maintains proper submit handling.
86-105: LGTM!The name field ARIA implementation is complete and correct. The error handling with role='alert' and matching id references provides proper accessibility for screen readers.
155-162: LGTM!The bottom margin adjustment (mb={8}) on the Add Value button improves the vertical rhythm of the form layout.
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
Show resolved
Hide resolved
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.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/ABACAttributesTab/AttributesContextualBar.tsx
🧠 Learnings (3)
📓 Common learnings
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/ABACAttributesTab/AttributesContextualBar.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/ABACAttributesTab/AttributesContextualBar.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx (1)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(142-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx (1)
79-79: Ensure consistent use oflist({})across all ABAC query key invalidations.The change from
list()tolist({})aligns with React Query's key matching behavior. Since query keys with object property set to undefined are not considered equal to missing property, callinginvalidateQuerieswithlist({})will only match queries created withlist({}). This fix ensures proper cache invalidation by matching the exact key shape used when creating queries with the ABAC attributes endpoint.
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (2)
110-135: Fix unconditional aria-describedby and improve ID formatting.Two accessibility issues previously flagged in past reviews remain:
Unconditional aria-describedby (line 116): References an error element that only renders when
errors.lockedAttributes?.[index]?.valueexists (line 129), creating broken ARIA references when no error is present.String concatenation in IDs (lines 116, 130): Using
${valuesField + index}produces IDs like"abc0-error"instead of the clearer"abc-0-error"from template literal interpolation.🔎 Proposed fix
<TextInput disabled aria-labelledby={valuesField} - aria-describedby={`${valuesField + index}-error`} + aria-describedby={errors.lockedAttributes?.[index]?.value ? `${valuesField}-${index}-error` : undefined} error={errors.lockedAttributes?.[index]?.value?.message || ''} aria-invalid={errors.lockedAttributes?.[index]?.value ? 'true' : 'false'} aria-required='true' {...register(`lockedAttributes.${index}.value`, { required: t('Required_field', { field: t('Values') }), validate: (value: string) => validateRepeatedValues(value), })} /> {index !== 0 && ( <IconButton mis={8} small title={t('ABAC_Remove_attribute')} icon='trash' onClick={() => removeLockedAttribute(index)} /> )} </FieldRow> {errors.lockedAttributes?.[index]?.value && ( - <FieldError id={`${valuesField + index}-error`} role='alert'> + <FieldError id={`${valuesField}-${index}-error`} role='alert'> {errors.lockedAttributes?.[index]?.value?.message} </FieldError> )}
136-160: Fix unconditional aria-describedby and improve ID formatting.The same accessibility issues from the locked attributes section apply here:
Unconditional aria-describedby (line 141): References an error element that only renders conditionally (line 154).
String concatenation in IDs (lines 141, 155): Using
${valuesField + index}instead of template literal interpolation${valuesField}-${index}.🔎 Proposed fix
<TextInput aria-labelledby={valuesField} - aria-describedby={`${valuesField + index}-error`} + aria-describedby={errors.attributeValues?.[index]?.value ? `${valuesField}-attr-${index}-error` : undefined} error={errors.attributeValues?.[index]?.value?.message || ''} aria-invalid={errors.attributeValues?.[index]?.value ? 'true' : 'false'} aria-required='true' {...register(`attributeValues.${index}.value`, { required: t('Required_field', { field: t('Values') }), validate: (value: string) => validateRepeatedValues(value), })} /> {(index !== 0 || lockedAttributesFields.length > 0) && ( <IconButton mis={8} small title={t('ABAC_Remove_attribute')} icon='trash' onClick={() => remove(index)} /> )} </FieldRow> {errors.attributeValues?.[index]?.value && ( - <FieldError id={`${valuesField + index}-error`} role='alert'> + <FieldError id={`${valuesField}-attr-${index}-error`} role='alert'> {errors.attributeValues[index].value.message} </FieldError> )}Note: Using
-attr-in the ID distinguishes attribute value errors from locked attribute errors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.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/ABACAttributesTab/AttributesForm.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.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 (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
packages/ui-client/src/components/Contextualbar/index.ts (1)
ContextualbarScrollableContent(35-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
84-85: LGTM! Spacing adjustments and form structure improvements.The spacing changes (mis={8} on IconButtons, mb={8} on Add Value button) and the restructured form container using ContextualbarScrollableContent align well with the PR objectives. The disabled logic on the Add Value button correctly prevents adding new values when validation errors exist or empty values are present.
Also applies to: 126-126, 151-151, 161-168
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
🧠 Learnings (4)
📓 Common learnings
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.
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-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/ABACRoomsTab/RoomFormAttributeFields.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/RoomFormAttributeFields.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/RoomFormAttributeFields.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (2)
packages/ui-client/src/components/Contextualbar/index.ts (1)
ContextualbarScrollableContent(35-35)apps/meteor/client/meteor/minimongo/OrderedDict.ts (1)
append(72-74)
⏰ 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: ⚙️ Variables Setup
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (2)
77-77: LGTM: Form wrapper updated appropriately.The change from Box to
ContextualbarScrollableContentwithis='form'properly structures the form for the contextual bar UI and maintains the submit handler behavior.
78-112: Excellent accessibility implementation.The ARIA attributes are correctly implemented with proper id references:
FieldLabelhasid={nameField}- Input uses
aria-labelledby={nameField}aria-describedbymatches theFieldErroridrole='alert'ensures error announcementsThis follows accessibility best practices and aligns with the patterns from related PRs.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (3)
64-82: LGTM: Key field accessibility correctly implemented.The ARIA attributes for the key field are properly implemented with matching id references between
aria-describedbyandFieldError.
103-107: LGTM: Remove button correctly hidden for first field.The conditional rendering ensures the first attribute field (index === 0) cannot be removed, which aligns with the required field logic and established patterns in the codebase.
Based on learnings, this matches the expected behavior where the first attribute is mandatory.
13-16: LGTM: Required prop correctly added.The optional
requiredprop with a default value offalseis properly typed and integrated, allowing the first field to be marked as required while others remain optional.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx
Show resolved
Hide resolved
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx
Outdated
Show resolved
Hide resolved
MartinSchoeler
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
110-134: ARIA attributes correctly implemented; minor duplicate key.The accessibility improvements address the previous review feedback with proper conditional
aria-describedby,aria-invalid, and matching FieldError ids.Line 112 has a redundant
key={field.id}since the parent Fragment on line 111 already has the key.🔎 Optional fix to remove redundant key
{lockedAttributesFields.map((field, index) => ( <Fragment key={field.id}> - <FieldRow key={field.id}> + <FieldRow>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.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
⏰ 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). (2)
- GitHub Check: ⚙️ Variables Setup
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (3)
84-105: Accessibility improvements look good.The conditional
aria-describedbyand proper ARIA attributes on the name field correctly address the previous review feedback. The error association via matching ids is properly implemented.
136-160: Attribute values accessibility correctly implemented.The conditional
aria-describedby, properaria-invalidstate, and matching FieldError ids address the previous review feedback. The remove button logic correctly handles the case where the first value can be removed if locked attributes exist.
161-177: Button logic and form submission look correct.The Add Value button properly disables when there are errors or empty values. The submit button correctly considers all error states and dirty flag.
The comment on line 164 explains a non-obvious react-hook-form behavior. Per coding guidelines, implementation comments should be avoided, but this one documents a framework quirk that could confuse future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.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/ABACRoomsTab/RoomFormAttributeFields.tsx
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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.
📚 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/ABACRoomsTab/RoomFormAttributeFields.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/RoomFormAttributeFields.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/RoomFormAttributeFields.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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (2)
80-84: LGTM on the FieldError accessibility implementation.The
FieldErrorcomponents correctly implement:
- Unique
idattributes matching thearia-describedbyreferencesrole="alert"for live region announcements to screen readersThis follows the pattern established in other ABAC/Omnichannel form components. Based on learnings, this is consistent with how ARIA mismatches were addressed in similar components like MessageForm.
Also applies to: 99-103
104-108: LGTM on conditional Remove button.The logic
index !== 0correctly ensures the first attribute field is mandatory and cannot be removed, while subsequent fields show the Remove button. This aligns with the established behavior. Based on learnings from the AdminABACRoomAttributesForm tests.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.tsx (1)
22-35: LGTM on the structural accessibility improvements.The changes correctly:
- Set
id={field.id}onFieldLabelfor programmatic label association- Apply
required={index === 0}to bothFieldLabelandRoomFormAttributeField, ensuring only the first attribute is mandatory- Pass
labelIdandindexprops to enable per-field accessibility and validation logicNote: The child component
RoomFormAttributeFieldshould usearia-labelledby={labelId}(notaria-label) to properly reference this label'sid—see the related comment on that file.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx
Outdated
Show resolved
Hide resolved
dougfabris
force-pushed
the
abac-space
branch
from
2eed4fb to
72acce3
Compare
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
MartinSchoeler
changed the title
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kodiakhq
bot
removed
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (2)
155-174: Add role='alert' to disclaimer FieldError for consistency.The disclaimer FieldError at line 156 lacks the
role='alert'attribute that's present on all other FieldError components in this form (lines 122, 151, 196). Adding it ensures consistent accessibility announcements for important user-facing messages.🔎 Proposed fix
{showDisclaimer.includes(index) && ( - <FieldError> + <FieldError role='alert'> <Trans i18nKey='ABAC_Cannot_delete_attribute_value_in_use'
56-56: Consider removing inline comments per coding guidelines.The coding guidelines prefer avoiding code comments in TypeScript/JavaScript implementations. While these comments provide useful context, consider whether the logic can be made self-explanatory through better naming or structure.
As per coding guidelines, TypeScript/JavaScript files should avoid implementation comments.
Also applies to: 205-205
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.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 (1)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
packages/ui-client/src/components/Contextualbar/index.ts (1)
ContextualbarScrollableContent(35-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (3)
105-126: LGTM! Name field accessibility is well-implemented.The name field now has proper ARIA attributes with conditional
aria-describedby,aria-required, andaria-invalid. The FieldError is correctly linked with a matching id and role='alert'.
131-154: LGTM! Locked attributes have proper ARIA implementation.The locked attribute fields have all required ARIA attributes (
aria-labelledby,aria-describedby,aria-invalid,aria-required) with conditional error linkage and properly indexed FieldError components.
177-201: LGTM! Attribute values have proper ARIA implementation.The attribute value fields have all required ARIA attributes with proper conditional error linkage, indexed FieldError components with matching IDs, and correct role='alert' attributes.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Accessibility
Style
Behavior
Chores
✏️ Tip: You can customize this high-level summary in your review settings.