fix: ats web app select all roles checkbox in edit roles page#831
fix: ats web app select all roles checkbox in edit roles page#831luigi-io wants to merge 3 commits intodevelopmentfrom
Conversation
Signed-off-by: Luigi Navarro <luigi@io.builders>
Signed-off-by: Luigi Navarro <luigi@io.builders>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
Fixes the “Select all roles” interaction on the ATS web app’s edit roles page by preventing unintended toggling behavior and adding automated coverage around the select-all scenarios.
Changes:
- Updated the select-all UI interaction in
HandleRolesto avoid clicking the entire row triggering role selection changes. - Added a focused test suite covering select-all behavior for various initial role states.
- Updated Jest setup to mock
@zag-js/focus-visibleto prevent JSDOM-related focus issues; added a changeset entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/ats/web/src/views/DigitalSecurityDetails/Components/RoleManagement/HandleRoles.tsx | Adjusts select-all checkbox/label click handling and adds an accessible name. |
| apps/ats/web/src/views/DigitalSecurityDetails/Components/RoleManagement/tests/HandleRoles.test.tsx | Adds tests validating select-all toggle behavior across multiple starting role configurations. |
| apps/ats/web/jest.setup.tsx | Mocks @zag-js/focus-visible to avoid JSDOM focus property issues during tests. |
| .changeset/clean-buckets-scream.md | Declares a patch release entry describing the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should leave only 'admin' role selected when clicking 'Select all roles' checkbox", async () => { | ||
| const user = userEvent.setup(); | ||
| // All available roles for BOND security type | ||
| const allRoles = [ | ||
| "admin", |
There was a problem hiding this comment.
This test name says it should “leave only 'admin' role selected”, but the test only asserts the “Select all roles” checkbox becomes unchecked. Update the test name to match the actual expectation, or add assertions that verify the per-role checkbox state (admin checked and others unchecked).
| it("when all selected and clicked, should leave empty array", async () => { | ||
| const user = userEvent.setup(); | ||
| const currentRoles: string[] = []; | ||
|
|
There was a problem hiding this comment.
This test claims the second click “should leave empty array”, but it never verifies the individual role checkboxes are all unchecked (it only checks the select-all checkbox state). Add assertions on the role checkbox states (or on the form value if exposed) to ensure the roles selection is actually cleared.
| aria-label={t("selectAllRoles")} | ||
| /> | ||
| <Text textStyle="BodyRegularXS" cursor="pointer" onClick={handleSelectAllRoles}> | ||
| {t("selectAllRoles")} | ||
| </Text> | ||
| </HStack> |
There was a problem hiding this comment.
The selectable text is implemented as a plain Text with onClick, which is not keyboard-accessible and isn’t semantically associated with the checkbox. Consider using a real <label htmlFor="selectAllRoles"> (or rendering the label as Checkbox children, as done elsewhere in the app) so clicking the label toggles the checkbox while preserving accessibility, without reintroducing the double-toggle issue from the wrapper click handler.
| aria-label={t("selectAllRoles")} | |
| /> | |
| <Text textStyle="BodyRegularXS" cursor="pointer" onClick={handleSelectAllRoles}> | |
| {t("selectAllRoles")} | |
| </Text> | |
| </HStack> | |
| > | |
| {t("selectAllRoles")} | |
| </Checkbox> | |
| </HStack> |
| // All available roles for BOND security type | ||
| const allRoles = [ | ||
| "admin", | ||
| "minter", | ||
| "freezer", | ||
| "controller", | ||
| "pause", | ||
| "controlList", | ||
| "corporateActions", | ||
| "document", | ||
| "snapshot", |
There was a problem hiding this comment.
This test hard-codes the complete BOND roles list. Since the source of truth already exists in rolesList.tsx, it would be more maintainable to derive allRoles from rolesList (and the mocked security type) so the test doesn’t become stale when roles are added/removed/renamed.
Signed-off-by: Luigi Navarro <luigi@io.builders>
2 participants
Signed-off-by: Luigi Navarro luigi@io.builders
Description
Fix select all roles checkbox in edit roles page in ats web app
Type of change
Testing
• Automated tests – npm run ats:test
• Manual verification – Check and uncheck select all roles checkbox in ats web app, with and without other
Node version:
Checklist