[Chakra exit · Phase 2 · PR 16] frontend: the security ACLs tab on Registry components, plus a smoke spec - #2649
Merged
Merged
Conversation
…spec #2604 replaced the users, roles and permissions tabs with new designs and left the ACLs tab on the old one, so it kept the last Chakra import in `pages/security`. There is no `acls-tab-new`, and the route is live at `/security/acls`, so this is a parity swap rather than throwaway work: Chakra's `SearchField` becomes the shared `SearchInput` and its `DataTable` becomes the Registry one. Whether ACLs also gets the new security design is a separate product call. Both sortable columns get a `DataTableColumnHeader` — the Registry sorts only through that component, where Chakra painted an affordance on every header — and the table passes `enableHiding: false`, since there is no column-visibility toolbar to undo a hide with. Pagination keeps legacy parity at 50 a page with the pager only past that; the two `size:` props are dropped because the Registry never reads them. The row-actions menu moves into its own component that owns its queries. That leaves the columns array closing over nothing but a stable state setter, so it is built once: `DataTableColumnHeader` is a dropdown trigger, and a re-created header tears an open sort menu down. Also drops the dead `rs.mock('@redpanda-data/ui')` from `roles-tab.test.tsx` — 156 lines stubbing a package that file's tree no longer touches, since `roles-tab.tsx` is now a re-export of the Chakra-free `roles-tab-new`. Both suites are unchanged at 993/1427, so those tests now exercise the real components. The new Playwright spec covers what the swap risked: the filter renders, both headers expose a sort trigger, and the trigger opens a menu. Verified against a live container, and it fails if the `DataTableColumnHeader` is swapped back for a plain string header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Clean — no registry drift, off-token colours, or ad-hoc classesApp:
Generated by lookout audit-changes. |
The Playwright spec covers the table chrome, but a fresh cluster has no ACLs, so it never clicks a row menu — which is the part of this swap that moved most, into its own component. Five tests over the real components: the rows and their hosts render, a Group principal is badged, the menu opens with all three delete options, and the user-delete options are enabled only for a principal that actually has a SASL account. That last pair is the `canDeleteUser` logic the extraction restructured, and the menu opening at all confirms the trigger still works after the empty `onClick` came off its render slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six from a review of the swap, two of them mine and shipping-visible: - the 300px width was on `className`, which lands on the `<input>`, not `containerClassName`, which sizes the positioned wrapper. `InputEnd` is absolute against that wrapper, so the "Clear search" X rendered at the far right of the page as soon as anyone typed. Every other call site uses `containerClassName`. - `AclRowActions` owned its own queries, and `useListUsersQuery` wraps `useInfiniteQueryWithAllPages`, which auto-fetches every page from an effect — so each row ran its own `fetchNextPage()` loop. The queries move back to the parent and reach the row through context, which is also what the stable-columns goal actually needed: `useInvalidateUsersCache` returns a new function every render, so a `useMemo` dep list could never have been stable. `columns` is now a module constant, closing over nothing. Four behaviour findings, all pre-existing in the Chakra original and folded in because the extraction put them in reach: - `canDeleteUser` matched only on name, so a Group principal sharing a name with a SASL user offered — and performed — a delete of that unrelated account. Now requires `principalType === 'User'`. - in "Delete (User and ACLs)" a failed ACL delete fell through to the account delete, orphaning the ACLs it had just failed to remove, and its `onFailure` was overwritten by the second leg's. It now returns. - the Principal sort key was the `User:`-prefixed `principal` while the cell renders `principalName`, so the newly-reachable sort ordered the column differently from what is on screen. Now an `accessorFn`. - no `getRowId`, so rows were index-keyed: a refetch that reordered the list retargeted an open action menu at a different principal. Also drops the dead `deleteButton` class, whose SCSS is nested under `.stringList .reorderableList` and includes an `opacity: 0` that would hide the button if anyone ever unscoped it. Two more tests: a Group row never offers the user deletes even when a user shares its name, and the Principal sort matches the rendered name. Both fail if their fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…type Comments to the constraint; the reasoning is in the PR. The context and table-options notes and both spec preambles were carrying paragraphs. Two code cleanups: `SimpleAcl` — whose own comment already says it is "used by ACLs tab and Permissions List tab" — is now exported and consumed, instead of the page hand-copying four of its five fields where the two could drift apart. And the ten-line `DeleteACLsRequest` literal buried mid-object is now a named `allAclsFor(principal, host)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eblairmckee
reviewed
Sep 11, 2026
| deleteRoleMutationMock: rs.fn().mockResolvedValue(undefined), | ||
| })); | ||
|
|
||
| rs.mock('@redpanda-data/ui', () => { |
Contributor
There was a problem hiding this comment.
i presume chakra is gone from the roles-tab then?
eblairmckee
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Takes the last Chakra page file off the package:
security/tabs/acls-tab.tsx.mastergoes 25 → 24, and once #2645 and #2647 merge that leaves five — all of them PR 13's own, which are deleted rather than migrated.#2604 replaced the users, roles and permissions tabs with new designs and left ACLs on the old one. There is no
acls-tab-newand the route is live at/security/acls(enterpriseusers-authorization.spec.tsnavigates straight to it), so this is a parity swap, not throwaway work. Whether ACLs also gets the new security design is a separate product call — this only removes the dependency.Reviewer's guide
SearchField→SearchInput/-to-focus and Escape-to-clear like every other list.DataTable→ the Registry'sDataTableColumnHeader: the Registry sorts only through that component, where Chakra painted an affordance on every header, so a naive swap leaves sorting unreachable with nothing failing. The action column getsenableSorting: false, and the table passesenableHiding: false— there is no column-visibility toolbar to undo a hide with.DEFAULT_TABLE_PAGE_SIZE) with the pager only past that, matchingtransforms-listandconnect/overview. Chakra's own default wasdefaultPageSize = 50and it rendered the footer only whendata.length > defaultPageSize.size:propsDataTablenever readsgetSize(), soNumber.POSITIVE_INFINITYandsize: 60were inert either way.AclRowActionsDataTableColumnHeaderis a dropdown trigger, and a re-created header tears an open sort menu down mid-interaction.Also in here
The dead
rs.mock('@redpanda-data/ui')inroles-tab.test.tsxis deleted — 156 lines stubbing ~30 components for a file whose tree no longer touches the package, sinceroles-tab.tsxis now justexport { RolesTabNew as RolesTab }androles-tab-new.tsxhas zero Chakra imports. Both suites stay at 993 / 1427 with no failures, so those tests now exercise the real components instead of stubdivs. This also clears one of PR 13's eleven bare-grep mop-up files, leaving ten.Coverage
The ACLs table had no spec of its own — only an enterprise authorization test visits the route.
acls-tab.spec.tscovers what the swap actually risked: the filter renders, both headers expose a sort trigger, and the trigger opens a menu rather than sorting on one click. Verified against a live container, and verified to fail when theDataTableColumnHeaderis swapped back for a plain string header. A fresh cluster has no ACLs, so it asserts the chrome rather than rows.Gates
type:checkclean ·lint:check0 diagnostics onacls-tab.tsx(down from 2 onmaster— the Chakra import was itself anoRestrictedImportserror, and the extraction removed an emptyonClick), parsed with the colour codes stripped · 993 unit · 1427 integration, both identical tomaster· the new spec passes 2/2 in theconsolevariant.Flagged, not fixed
acl-delete-multi-match.spec.tsistest.skipend to end, both cases, pending UX-1216. A spec file is not evidence of coverage.users-tab.tsx,roles-tab.tsx,permissions-list-tab.tsx) now only forward to their-newcounterparts. Collapsing them so the routes import-newdirectly is Remove enableNewSecurityPage feature flag #2604 follow-up hygiene, not Chakra work.permissions-list-tab.test.tsx:121still mocksenableNewSecurityPage, a flag that no longer exists in source. Vestigial after Remove enableNewSecurityPage feature flag #2604.🤖 Generated with Claude Code