chore: ABAC add/edit room attributes form #37244
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds a new AdminABACRoomAttributesForm React component with react-hook-form field arrays for editable and locked attribute values, two Storybook stories, and a comprehensive test suite covering rendering, accessibility, validation, dynamic interactions, and submit/cancel behavior. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as AdminABACRoomAttributesForm
participant RHF as react-hook-form
participant Actions as onSave/onCancel
rect rgba(220,240,220,0.35)
Note over UI,RHF: Initialize form with name, attributeValues, lockedAttributes
end
User->>UI: Edit name / add value / remove value
UI->>RHF: update field arrays (append/remove)
RHF-->>UI: updated form state (values, errors, validity)
alt Form valid
User->>UI: Click Save
UI->>Actions: onSave(formData)
else Cancelled
User->>UI: Click Cancel
UI->>Actions: onCancel()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37244 +/- ##
===========================================
+ Coverage 66.90% 68.02% +1.12%
===========================================
Files 3413 3361 -52
Lines 117501 115580 -1921
Branches 21501 20809 -692
===========================================
+ Hits 78610 78627 +17
+ Misses 36209 34270 -1939
- Partials 2682 2683 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
force-pushed
the
feat/abac-room-att-form
branch
from
1b12e37 to
f45a4b5
Compare
MartinSchoeler
force-pushed
the
feat/abac-room-att-form
branch
from
f45a4b5 to
a7c4d90
Compare
MartinSchoeler
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
25-29: Consider strengthening type safety foronSaveparameter.The
onSavecallback is typed withdata: unknown, but you have a well-definedAdminABACRoomAttributesFormFormDatatype. Using the specific type would provide better type safety for both the component and its consumers.Apply this diff to improve type safety:
type AdminABACRoomAttributesFormProps = { - onSave: (data: unknown) => void; + onSave: (data: AdminABACRoomAttributesFormFormData) => void; onCancel: () => void; description: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.stories.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
apps/meteor/client/components/Contextualbar/index.ts (2)
ContextualbarScrollableContent(35-35)ContextualbarFooter(32-32)
🔇 Additional comments (11)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.stories.tsx (1)
21-44: LGTM! Clean story setup.The Default story properly initializes the form with an empty name and a single empty attribute value, which demonstrates the basic use case effectively.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (5)
1-72: Excellent test setup and infrastructure!The test suite demonstrates strong testing practices:
- UUID normalization (lines 60-63) elegantly handles react-hook-form's random field IDs for stable snapshots
- Accessibility checks with jest-axe ensure a11y compliance across all stories
- Comprehensive translation mocks cover the form's i18n requirements
74-107: Validation tests are thorough and well-structured.These tests properly verify that required field validation works correctly for both the name field and attribute values, using appropriate async handling with
waitFor.
109-171: Interaction tests comprehensively verify add/remove functionality.The tests correctly verify:
- Adding new attribute values
- Removing editable attribute values
- Removing locked attributes (correctly accounting for the restriction that the first locked attribute cannot be removed)
173-200: Button state tests ensure good UX.These tests verify that the Add Value button is appropriately disabled when there are empty values, preventing users from accumulating unfilled fields.
202-259: Form submission and mixed attribute tests are comprehensive.These tests properly verify:
- Form submission flow with
onSavecallback- Cancel button functionality
- Mixed locked/regular attributes with correct disabled states
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (5)
43-48: Verify that field array validation errors are properly displayed.The
minLength: 1rule (line 46) creates validation errors aterrors.attributeValues.root, butgetAttributeValuesError()only checks forerrors.attributeValues?.length(line 57), which detects individual field errors, not the field array root error.Consider whether the field array root error should be displayed. If the UI logic (preventing removal of the last field) already ensures at least one value exists, the
minLengthrule may be redundant. Otherwise, updategetAttributeValuesErrorto check for root-level errors:const getAttributeValuesError = useCallback(() => { + if (errors.attributeValues?.root?.message) { + return errors.attributeValues.root.message; + } if (errors.attributeValues?.length && errors.attributeValues?.length > 0) { return t('Required_field', { field: t('Values') }); } return ''; }, [errors.attributeValues, t]);Also applies to: 56-61
91-102: Confirm that preventing removal of the first locked attribute is intentional.Line 100 prevents the first locked attribute (index 0) from being removed by not rendering its remove button. This ensures at least one locked attribute remains when locked attributes are present.
Verify this is the intended UX behavior. If users should be able to remove all locked attributes, adjust the condition:
-{index !== 0 && <IconButton aria-label={t('Remove')} icon='trash' onClick={() => removeLockedAttribute(index)} />} +<IconButton aria-label={t('Remove')} icon='trash' onClick={() => removeLockedAttribute(index)} />
103-115: Attribute value removal logic maintains form integrity.The conditional removal button logic (line 111) ensures at least one attribute value field is always present when there are no locked attributes, while allowing full flexibility when locked attributes exist. This maintains good form UX by preventing an empty state.
117-123: Add Value button logic provides good UX.The disabled condition (line 120) prevents users from adding new fields when existing ones are empty or have validation errors. The comment on line 119 helpfully explains the need for direct value checking due to react-hook-form's behavior after
append().
70-134: Form structure and submission logic are well-implemented.The form properly uses
handleSubmitfor validation, disables the Save button when errors are detected, and correctly separates scrollable content from the footer actions.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.stories.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (1)
203-225: Consider simplifying the save test for better clarity.The test removes a value (lines 216-217) before submitting the form, which adds unnecessary complexity. For a focused test of the save functionality, consider either:
- Remove the trash button interaction to test pure save behavior, or
- Rename the test to indicate it's testing "remove and save" if that specific workflow is important
Example of a more focused save test:
it('should call onSave with correct data when form is submitted', async () => { const defaultValues = { name: 'Test Attribute', attributeValues: [{ value: 'Value 1' }, { value: 'Value 2' }], }; render( <FormProviderWrapper defaultValues={defaultValues}> <AdminABACRoomAttributesForm {...defaultProps} /> </FormProviderWrapper>, { wrapper: appRoot }, ); - const trashButtons = screen.queryAllByRole('button', { name: 'Remove' }); - await userEvent.click(trashButtons[0]); - const saveButton = screen.getByRole('button', { name: 'Save' }); await userEvent.click(saveButton); await waitFor(() => { expect(defaultProps.onSave).toHaveBeenCalled(); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#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-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: When generating tests, provide complete runnable TypeScript test files with proper imports, clear describe/test blocks, and adherence to these guidelines
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)
🔇 Additional comments (4)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (4)
57-66: Good approach to handling dynamic UUIDs in snapshots.The UUID normalization strategy is a pragmatic solution for dealing with react-hook-form's dynamic field IDs. The regex pattern correctly matches UUID v4 format and ensures stable snapshots.
26-44: Well-designed test utility for form context.The
FormProviderWrapperis cleanly implemented and properly typed, making tests more maintainable by centralizing the react-hook-form setup. Good use ofmode: 'onChange'for validation testing.
46-261: Comprehensive test coverage with good testing practices.The test suite demonstrates strong coverage including:
- Storybook story rendering and snapshots
- Accessibility validation with jest-axe
- Form validation and error states
- Dynamic interactions (add/remove values)
- Edge cases (locked attributes, mixed states, button states)
The use of Storybook stories for testing and accessibility checks aligns with best practices.
1-1: Remove the unnecessary ESLint disable comment.The eslint-disable comment on line 1 is not needed. The stories file imports the same type without this disable and no other identifiers in the test file violate naming conventions. This comment can be safely removed.
Likely an incorrect or invalid review comment.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (1)
147-167: The past review comment is incorrect; this test logic is actually correct.The test has 2 locked attributes and 1 empty regular attribute. Based on the component logic:
- Trash buttons appear for:
lockedAttributes[1](second locked) andattributeValues[0](first regular, because locked attributes exist)- In DOM order,
trashButtons[0]is the trash button forlockedAttributes[1]- Clicking
trashButtons[0]callsremoveLockedAttribute(1), removing the item at index 1 ('Locked Value 2')- The assertions correctly verify that 'Locked Value 1' remains and 'Locked Value 2' is removed
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
25-29: Consider using typed onSave parameter.The
onSavecallback acceptsunknown, which loses type safety. Consider using the defined form data type instead.Apply this diff to improve type safety:
type AdminABACRoomAttributesFormProps = { - onSave: (data: unknown) => void; + onSave: (data: AdminABACRoomAttributesFormFormData) => void; onCancel: () => void; description: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#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-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: When generating tests, provide complete runnable TypeScript test files with proper imports, clear describe/test blocks, and adherence to these guidelines
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
PR: RocketChat/Rocket.Chat#36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
apps/meteor/client/components/Contextualbar/index.ts (2)
ContextualbarScrollableContent(35-35)ContextualbarFooter(32-32)
⏰ 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). (6)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (13)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (8)
1-24: LGTM! Clean test setup.The test setup properly uses Storybook composition, mock providers with translations, and jest-axe for accessibility testing.
26-44: LGTM! Proper FormProvider wrapper for testing.The wrapper correctly provides form context with appropriate default values and onChange validation mode.
57-68: LGTM! Comprehensive story testing.Testing all Storybook stories for both snapshots and accessibility violations ensures consistent quality across variants.
70-103: LGTM! Proper validation testing.The tests correctly verify required field validation with translated error messages and proper async handling.
105-145: LGTM! Dynamic field array operations tested correctly.The tests properly verify adding and removing values, correctly expecting only 1 trash button when the first value must be preserved.
169-196: LGTM! Button state tests are correct.The tests properly verify that the Add Value button is disabled when there are empty values and enabled when all values are filled.
198-234: LGTM! Callback tests are correct.The tests properly verify form submission and cancellation flows with appropriate mock assertions.
236-256: LGTM! Mixed attributes scenario tested correctly.The test properly verifies that locked inputs are disabled, regular inputs are enabled, and trash buttons appear only where appropriate (1 button for the regular value since the locked value is at index 0).
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (5)
31-54: LGTM! Proper react-hook-form setup.The component correctly uses
useFormContext,useFieldArray, anduseIdfor form management and accessibility.
56-68: LGTM! Comprehensive error handling.The error checking logic properly handles both array-level and item-level validation errors for both editable and locked attributes.
70-86: LGTM! Proper form structure and Name field.The form correctly uses
handleSubmit, and the Name field has proper validation, error display, and accessibility attributes.
87-122: LGTM! Well-designed field array logic.The Values field correctly handles both locked and editable attributes with smart trash button logic that ensures at least one value is always present. The Add Value button appropriately prevents adding new values when existing ones are empty or invalid.
124-133: LGTM! Proper form footer implementation.The footer buttons correctly handle cancellation and submission, with the Save button appropriately disabled based on validation state.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (1)
148-170: Fix incorrect test assertions for locked attribute removal.The test clicks the first trash button (
trashButtons[0]) but expects the second item to be removed. This logic is backwards—clicking the first trash button should remove the first item ('Locked Value 1'), not the second item ('Locked Value 2').Apply this diff to fix the assertions:
await userEvent.click(trashButtons[0]); - expect(screen.getByDisplayValue('Locked Value 1')).toBeInTheDocument(); - expect(screen.queryByDisplayValue('Locked Value 2')).not.toBeInTheDocument(); + expect(screen.queryByDisplayValue('Locked Value 1')).not.toBeInTheDocument(); + expect(screen.getByDisplayValue('Locked Value 2')).toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#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-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: When generating tests, provide complete runnable TypeScript test files with proper imports, clear describe/test blocks, and adherence to these guidelines
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
PR: RocketChat/Rocket.Chat#36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)
🔇 Additional comments (7)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (7)
1-44: LGTM!The test setup is well-structured with proper imports, mock translations, and a reusable FormProviderWrapper that correctly initializes react-hook-form context with appropriate default values and validation mode.
46-68: LGTM!Excellent test coverage with parameterized tests for all Storybook stories, including both snapshot and accessibility validation using jest-axe.
70-103: LGTM!The validation tests properly verify required field validation for both the name field and attribute values, using appropriate async assertions with waitFor.
105-123: LGTM!The test correctly verifies that clicking the Add Value button adds a new field to the form.
172-199: LGTM!The tests correctly verify that the Add Value button is disabled when there are empty values and enabled when all values are filled, ensuring good UX for preventing duplicate empty fields.
201-237: LGTM!The tests correctly verify the form submission flow with onSave being called with form data and onCancel being invoked when the Cancel button is clicked.
239-258: The behavior is intentional and consistent across both test scenarios—the review comment is incorrect.The component's Remove button logic ensures that:
- The first locked attribute can never be removed (
index !== 0)- Regular attributes can be removed when they're the first item IF locked attributes exist (
index !== 0 || lockedAttributesFields.length > 0)In the locked-only scenario (lines 148-170): Only the second locked attribute has a Remove button.
In the mixed scenario (lines 239-258): The first locked attribute has no button, but the first regular attribute has one (because
lockedAttributesFields.length > 0is true).Both cases result in exactly 1 Remove button, which is consistent and intentional design—it prevents scenarios where zero attributes remain while allowing controlled removal of non-first items.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
Show resolved
Hide resolved
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
MartinSchoeler
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Adds a new form to be used with the ABAC admin pages.
Issue(s)
ABAC-59
Steps to test or reproduce
Further comments
I've created this in a separate PR due the complexity of this form. This way it is easier to review it
Summary by CodeRabbit
New Features
Tests
Documentation