Remove enableNewSecurityPage feature flag - #2604
Merged
Merged
Conversation
The new security page is now the only implementation. Collapses the old/new dispatcher components down to the new ones, deletes the resulting dead legacy code and routes, and removes the E2E specs that existed solely to force the old UI via the flag.
Contributor
✅ Clean — no registry drift, off-token colours, or ad-hoc classesApp:
Generated by lookout audit-changes. |
Re-ports the ACL/permissions/user-management/deep-link E2E specs that were dropped when enableNewSecurityPage was removed, targeting the new UI directly instead of forcing the legacy one via the flag.
malinskibeniamin
approved these changes
Aug 27, 2026
SpicyPete
approved these changes
Aug 28, 2026
malinskibeniamin
approved these changes
Sep 4, 2026
This was referenced Sep 10, 2026
SpicyPete
added a commit
that referenced
this pull request
Sep 11, 2026
…gistry components, plus a smoke spec (#2649) * frontend: the security ACLs tab on Registry components, plus a smoke 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> * frontend: security — cover the ACLs row-actions menu 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> * frontend: security — clear the ACLs review findings 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> * frontend: security — trim the ACLs comments, drop the duplicated row 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
3 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.
Summary
enableNewSecurityPageis removed fromFEATURE_FLAGSand allisFeatureFlagEnabled('enableNewSecurityPage')branches.AclsTab,RolesTab,UsersTab,PermissionsListTab,RoleDetailPage,UserDetailsPage,SecurityLayout,SecurityTabsNav,AclDetailPage, plus links inoverview.tsxandadd-user-step.tsx) down to the new implementation, deleting the now-dead legacy components.acl-create-page.tsx+ its test,acl-update-page.tsx,role-update-page.tsx) and simplified their route files to unconditional redirects.acl-page.ts/role-page.tspage objects) that existed solely to force the old UI viatest.use({ featureFlags: { enableNewSecurityPage: false } }). Accepting this as a known E2E coverage gap rather than porting them to the new UI.Test plan
bun run type:checkbun run lintbun run test:unit(828 tests passing)bun run test:integration(1246 tests passing)