Skip to content

UX-986: fix(frontend): repair security role edit route#2332

Merged
malinskibeniamin merged 2 commits intomasterfrom
beniamin-malinski/role-edit-404-fix
Mar 23, 2026
Merged

UX-986: fix(frontend): repair security role edit route#2332
malinskibeniamin merged 2 commits intomasterfrom
beniamin-malinski/role-edit-404-fix

Conversation

@malinskibeniamin
Copy link
Copy Markdown
Contributor

What

Fix the broken Security > Roles edit flow by sending role edits to the canonical update route, keeping legacy /edit URLs working, and adding regression coverage.

Why

Editing a role could land on a not found screen because parts of the UI still navigated to /security/roles/<role>/edit while the router only exposed /security/roles/$roleName/update.

How

  • update stale role edit navigation to use the encoded /update route
  • add a backward-compatible /edit route that redirects to /update and preserves host
  • regenerate the file-based route tree
  • add regression tests for encoded role edit navigation and legacy route redirects

Acceptance Criteria

  • clicking the role edit action no longer opens the not found page
  • existing /security/roles/<role>/edit links redirect to /security/roles/<role>/update
  • host search params are preserved during the redirect
  • regression tests cover the new behavior

Testing Steps

  • cd frontend && bun run test:file:integration src/components/pages/acls/acl-list.roles.test.tsx src/routes/security/roles/-edit-route.test.tsx

@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Mar 23, 2026

Claude finished @malinskibeniamin's task in 1m 23s —— View job


Code Review

  • Gathering context
  • Reading changed files
  • Analyzing code
  • Posting review feedback

The approach is sound: fix the stale navigation calls, add a backward-compatible redirect route, and ship regression tests. A few things worth flagging:

replace: true is not asserted in the redirect test

frontend/src/routes/security/roles/-edit-route.test.tsx asserts status, to, params, and search, but skips redirect.options.replace. The replace: true flag is the piece that prevents a spurious history entry — users who follow a bookmark to /edit should land on /update without a back-button entry pointing to a redirect. Missing this assertion means the flag could be accidentally removed and the tests would still pass.

// both tests should also assert:
expect(redirect.options.replace).toBe(true);

Fix this →


The edit redirect route appears in TypeScript's FileRoutesByTo

Because routeTree.gen.ts registers the new route in FileRoutesByTo, TanStack Router's type system allows passing to="/security/roles/$roleName/edit" to a <Link> component without a type error. That partially undermines the goal of making the /update path the canonical one — new code could inadvertently link to the redirect instead of the real route.

This is a minor concern; the runtime redirect keeps things working. But if the goal is to funnel future code toward /update, it may be worth leaving a comment in edit.tsx like:

// Backward-compatibility shim — navigate to /update directly in new code.

acl-list.roles.test.tsx — large mock surface for a single assertion

The test file is 365 lines of module mocks for one integration assertion (line 363). The mocks are thorough and correct, but if the test suite grows, consider extracting shared mock factories/fixtures so future tests in this file don't need to reproduce the entire setup. Not a blocker but worth noting for maintainability.


Correctness of the core changes ✅

  • acl-list.tsx:663encodeURIComponent(entry.name) + /update path is correct.
  • role-details.tsx:104 — uses the already-encoded encodedRoleName prop, which is correct.
  • edit.tsxreplace: true prevents history pollution; zod schema validates and preserves host; params are forwarded correctly.
  • Route tree regeneration looks clean.

The fix is correct and the test coverage is solid. The only meaningful gap is the missing replace: true assertion.

@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

Addressed the Claude review note:

  • added expect(redirect.options.replace).toBe(true) in both redirect test cases
  • bumped each redirect test to 5 assertions
  • added a small backward-compat shim comment in frontend/src/routes/security/roles/$roleName/edit.tsx to steer new code toward /update

Re-ran:

  • cd frontend && bun run test:file:integration src/routes/security/roles/-edit-route.test.tsx src/components/pages/acls/acl-list.roles.test.tsx

@malinskibeniamin malinskibeniamin changed the title fix(security): repair role edit route fix(frontend): repair security role edit route Mar 23, 2026
@malinskibeniamin malinskibeniamin enabled auto-merge (squash) March 23, 2026 08:22
@malinskibeniamin malinskibeniamin changed the title fix(frontend): repair security role edit route UX-896: fix(frontend): repair security role edit route Mar 23, 2026
@malinskibeniamin malinskibeniamin self-assigned this Mar 23, 2026
@malinskibeniamin malinskibeniamin added bug Something isn't working frontend ui/ux labels Mar 23, 2026
@malinskibeniamin malinskibeniamin changed the title UX-896: fix(frontend): repair security role edit route UX-986: fix(frontend): repair security role edit route Mar 23, 2026
@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

Will fix https://redpandadata.atlassian.net/browse/UX-986 once deployed

@malinskibeniamin malinskibeniamin merged commit 89fae28 into master Mar 23, 2026
16 checks passed
@malinskibeniamin malinskibeniamin deleted the beniamin-malinski/role-edit-404-fix branch March 23, 2026 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working frontend ui/ux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants