Security page refactor

[jvorcak]

[malinskibeniamin]

@claude review

[claude]

Claude finished @malinskibeniamin's task in 3m 27s —— View job

---

Code Review: Security page refactor

Tasks

• Gather context and understand PR
• Read CLAUDE.md for repo guidelines
• Review changed files
• Post comprehensive review feedback

---

Summary

Good overall refactor — moving from full-page routes to dialogs for create/edit flows is a cleaner UX pattern, and the shared AclsCard, SecurityTabsNav, and DescriptionWithHelp components help reduce duplication. A few bugs and quality gaps are worth addressing before merge.

---

🐛 Bugs

1. Broken E2E test: create-acls-button testId removed [user-management.spec.ts:72]

The test clicks create-acls-button and then asserts URL /security/acls/create, but the ACL create route was deleted and the button's testId was replaced with go-to-user-details-button in the confirmation modal (user-create.tsx:364). This test will fail.

// user-management.spec.ts:72-80 — references deleted route + removed testId
await page.getByTestId('create-acls-button').click();
await expect(page).toHaveURL(/\/security\/acls\/create/);

Similarly, acl.spec.ts and acl-page.ts still navigate directly to /security/acls/create, which was removed in this PR. Fix this →

---

2. Non-reactive role membership lookup [use-principal-permissions.ts:105]

rolesApi.roleMembers is accessed as a plain property inside useMemo, not via a store hook. This means the memo won't re-run reactively when memberships change — the permissions list will be stale until the page remounts.

// use-principal-permissions.ts:105
for (const [roleName, members] of rolesApi.roleMembers) {  // ← not reactive

---

3. Dead code [user-details.tsx:48]

The return value of formatMechanism(...) is computed but immediately discarded. This is almost certainly a bug — the mechanism label was supposed to be used somewhere (e.g., displayed in the user info section).

// user-details.tsx:48
formatMechanism(currentUser?.mechanism); // result is unused

---

⚠️ Code Quality

4. Raw <input> elements instead of registry <Input>

Two places use raw <input> with copy-pasted Tailwind classes instead of the <Input> registry component, which goes against the project's code standards (new code MUST use Registry components):

• permissions-list-tab.tsx:368 — search input
• add-acl-dialog.tsx:199 — group principal input (inconsistent with the <Combobox> used for the User type)

Fix this →

---

5. Addressing existing reviewer comments

• role-create-dialog.tsx:58 (@malinskibeniamin) — Agreed, use ConnectError.from(error) to get structured gRPC error info (code, message) instead of falling back to the raw JS error message:

  // current
  toast.error(`Failed to create role: ${err instanceof Error ? err.message : String(err)}`);
  // suggested
  const connectErr = ConnectError.from(err);
  toast.error(`Failed to create role: ${connectErr.message}`);

• role-detail-page.tsx:100 (@malinskibeniamin) — The toast should mention the removed member name for clarity. Suggest: `User "${parsePrincipal(memberPrincipal).name}" removed from role`

• acls-card.tsx:204 (@malinskibeniamin) — <h2 className="font-semibold text-base"> could use the <Typography> component if not mixing Chakra/Tailwind. Given this file is pure Tailwind, this is a low-priority style preference.

• description-with-help.tsx (@malinskibeniamin) — The Drawer-for-all-help-text pattern is a valid concern. For short single-line hints a tooltip (title attribute or <Tooltip>) is less disruptive. Consider accepting a compact?: boolean prop or rendering a <Tooltip> when children is a short string. For the current usages (paragraphs of text), the Drawer is appropriate.

• permissions-list-tab.tsx:100 (@malinskibeniamin) — The summaryText IIFE is readable but verbose. The pluralize helper (if it exists in this codebase) would clean it up. Worth a follow-up.

• permissions-list-tab.tsx:244 (@malinskibeniamin) — Using <Empty> from the registry for the "No principals" empty state would be consistent with other pages. Worth a follow-up.

• add-acl-dialog.tsx:256 (@malinskibeniamin) — The reviewer asks "Is this a card?" — looking at the code, the context is the pattern-type segmented control (<div className="inline-flex h-10 w-full items-center...">). This is not a card, it's a tab/toggle group. Not an issue.

---

🔍 Minor Observations

• SecurityTabsNav — the Roles tab shows for all non-serverless envs but is disabled based on featureRolesApi. The disabled tab renders but isn't clickable — consider hiding it entirely when disabled to reduce confusion.

• acls-card.tsx — row IDs are assigned with a mutable rowCounter that resets on each render. Since setSelected(new Set()) is called after deletions, this doesn't cause visible bugs, but it's fragile. A stable key derived from the ACL fields (principal + resourceType + operation + ...) would be more robust and could also protect against duplicate-selection bugs.

• user-create-dialog.tsx — the dialog doesn't reset step back to 'form' after the dialog is closed while at 'confirmation'. Re-opening will briefly show the confirmation screen before handleClose fires. Add setStep('form') to handleClose.

---

[jvorcak]

@claude review

[claude]

Claude finished @jvorcak's task in 5m 9s —— View job

---

Code Review: Security page refactor (re-review after feature-flag commits)

The PR is now well-gated behind enableNewSecurityPage (routes/security.tsx:33,213-215, plus beforeLoad redirects on legacy ACL/role create/update routes). Most of the previous review's items were addressed (toast wording, ConnectError.from, Heading registry, pluralizeWithNumber, <Empty>, e2e button id). Remaining issues below.

---

🐛 Bugs

1. Non-reactive rolesApi.roleMembers access [use-principal-permissions.ts:105, roles-tab-new.tsx:119, user-details-new.tsx:94]

rolesApi is a Proxy over useRolesStore.getState() (backend-api.ts:2336-2344), so reads are one-shot — they don't subscribe to store updates. Both new files read rolesApi.roleMembers inside render:

// roles-tab-new.tsx:119
members: rolesApi.roleMembers.get(r.name) ?? [],
// use-principal-permissions.ts:105
for (const [roleName, members] of rolesApi.roleMembers) { ... }

After refreshRoleMembers() runs, the store updates but consumers won't re-render. Use a selector hook: const roleMembers = useRolesStore((s) => s.roleMembers); and pass it as a useMemo dependency.

---

2. Dead code re-introduced [user-details-new.tsx:48]

formatMechanism(currentUser?.mechanism); // result discarded

The legacy user-details.tsx no longer has this line, but the new file does. Either render the label (it's defined for a reason) or remove the call.

---

3. UserAclsCell counts resources, not ACLs [users-tab-new.tsx:457-471]

select: (r) => r.resources.length,
...
<TagsValue>{`${aclCount} ACL${aclCount !== 1 ? 's' : ''}`}</TagsValue>

listACLs returns resources[] where each resource contains multiple acls[]. Counting resources.length and labelling it "N ACLs" undercounts when a resource has >1 ACL rule. Either count r.resources.reduce((s, x) => s + x.acls.length, 0) or relabel.

---

4. Raw <input> elements still present

Per frontend/AGENTS.md: "New code MUST use Registry components." These two raw inputs remain:

• permissions-list-tab-new.tsx:451-456 — search input (note: users-tab-new.tsx correctly uses ListLayoutSearchInput, so just swap that in here)
• add-acl-dialog.tsx:196-203 — group principal field

Fix this →

---

5. Inconsistent principal format on role-membership updates

• user-roles-card-new.tsx:59,66 uses bare userName:

  add: [{ principal: userName }]

• role-detail-page-new.tsx:83,101 uses prefixed User: form:

  add: [{ principal: `User:${userName}` }]

The legacy user-edit-modals.tsx:215,223 uses the bare form, so the new role-detail page is the outlier. Two code paths writing different principal formats to the same RPC will eventually surface as a "user not in role" mismatch when one side filters by User:foo and the other stored foo. Pick one and align.

---

6. add-acl-dialog.tsx:147 — generic error message

setSubmitError(err instanceof Error ? err.message : String(err));

Same fix the reviewer asked for in role-create-dialog.tsx: use ConnectError.from(err).message so the structured gRPC code/message surfaces.

---

7. user-create-dialog.tsx handleClose doesn't reset step [user-create-dialog.tsx:52-54]

users-tab-new.tsx works around this by remounting via key={createDialogKey} (line 333,360). The dialog itself is still buggy — any other consumer that doesn't bump a key will see the confirmation flash on reopen. Fix at the source:

const handleClose = useCallback(() => {
  setStep('form');
  // ...also reset username/password/mechanism if they aren't already
  onOpenChange(false);
}, [onOpenChange]);

---

⚠️ Code Quality

8. Mutable rowCounter in acls-card.tsx:104-126 — still resets on every render and assigns string IDs ('0', '1', ...). After deletion, the surviving rows get re-numbered. setSelected(new Set()) masks it today, but a derived stable key (e.g. ${principal}|${resourceType}|${resourceName}|${operation}|${permissionType}|${host}|${patternType}) is more robust.

9. permissions-list-tab-new.tsx:231-234,246-249 — array index used as React key (<AclTableRow key={i} />). Acceptable while rows are read-only, but if filtering/sorting is added later it will cause reconciliation churn. Same stable composite key would help.

10. useSecurityBreadcrumbs is now legacy-only — only acls-tab.tsx:271 (legacy AclsTabLegacy) calls it; the new flow uses setPageHeader. The deletion of use-security-breadcrumbs.test.tsx is fine, but the hook itself can probably be removed once the legacy ACLs tab is retired.

11. description-with-help.tsx — author opted to keep Drawer, which is reasonable for paragraph-length copy. Worth leaving a // TODO compact?: boolean comment so the reviewer's idea isn't lost if shorter content shows up later.

---

🔍 Minor

• routes/security.tsx:33 redirects /security → /security/users under the flag, but if userData.canManageUsers === false the Users tab is disabled (security-tabs-nav.tsx:38-40). Landing on a tab the user can't interact with is awkward — consider redirecting to the first non-disabled tab instead.
• UserAclsCell has no isLoading skeleton (other cells do). Empty cells will briefly show "None" before the count appears.
• roles-tab-new.tsx:119 reads rolesApi.roleMembers synchronously during render. If refreshRoleMembers() hasn't completed yet, every row will show 0 assigned principals until the next render is triggered by something else.

---

Tasks

• Re-checked previous review fixes
• Reviewed feature-flag plumbing
• Reviewed new files (*-new.tsx, add-acl-dialog, acls-card, hooks, dialogs)
• Cross-checked previous reviewer comments
  • Branch: security-page-refactor-2

[SpicyPete]

Just a few minor comments on the code, going to run this locally next.

[SpicyPete]

Some misc feedback as I am poking around the new pages.

As this is all behind a feature flag nothing here is a blocker.

1. visual issues

2. Hide can't unhide

In this column, we can hide the column, but there's no way to un-hide it? Might be a issue at the registry level. refreshing the page brings it back.

3. Missing form feedback

This form says the arguments are invalid, but it's not clear why or what's wrong

4. destructive guards

Here we can remove principals without any confirmation. I'm not sure what we do organization wide or if it's in scope of this PR, but I think destructive actions should have confirmation guards

5. Principals count

Shows 0 principals, but I have 1 assigned

6. Users layout

We are showing "users" then just below it "users" again, and then the first actions are to delete the user. I think we can have a better layout, and not surface the admin actions of change password/delete as the first actions on the users.

[jvorcak]

@SpicyPete

1. We need to put the alert back. If we want this to be inline like I suggested, I'd need to put more work into licensing. For now, an alert works. Good catch!
2. I think that's a problem with the UI Registry
3. Fixed
4. Nice point, I'll implement
5. I think it was just a bad place when the data refetches
6. 👍

[SpicyPete]

Nice, looks like those issues are resolved.
Just one minor issue I'm seeing: after creating a user it shows a dialog asking to write down my password, but the input is disabled and I'm not able to view the password.

[weeco]

1. Hovering ACLs pill on users list

Desired:

Actual: No tooltip

2. When clicking the ACL pill

IMO it should not be clickable, we don't need it if we have the hover.

3. NIT: Different colors, sizes and hover area for buttons in password field

Why is the regenerate button purple, and the copy button so different?

4. NIT: Can use an input field and copy button in the user creation success dialog for the username?

Actual:

Desired (copyable username, still shown in plaintext in an input field?):

5. User actions - no icons and danger variant

Actual:

Desired:

6. Incosistent height from user button and search field / filter buttons

Actual:

Desired:

The input / search as well as the filter button should match the height/size of the create buttons on the pages. It looks weirdly misaligned.

7. Permissions cards are hard to parse due to missing spacing etc.

It's very hard to parse the permissions in the current layout and spacing since its a nested table. The original mock proposed collapsible cards for each principal with a very distinct header on the principal. This makes it much easier to recognize the different principals and map the ACLs to them

Actual:

Desired:

8. Padding on users details page:

9. NIT: Render ACL resource types as pill

Actual:

Desired:

10. User details page header

Actual:

Desired (show principal and mechanism explicitly, move buttons to the right in the same row as title to balance):

11. NIT: Make modal larger for allow all ops:

Actual:

12. Make delete button same size as other buttons:

[weeco]

The ADD ACLs dialogue should work as designed, you shouldn't insist on an existing user or principal. Both should accept free inputs, currently it would just not accept my user:

Also the whole Users functionality is gated if redpanda admin api is not configured. That's no longer true, because we use the Kafka API for creating users, updating passwords or listing users.