Skip to content

UN-4567 [FIX] Keep Manage Keys table inside the modal - #2417

Closed
ritwik-g wants to merge 2 commits into
mainfrom
UN-4567-manage-keys-layout
Closed

ritwik-g wants to merge 2 commits into
mainfrom
UN-4567-manage-keys-layout

Conversation

@ritwik-g

@ritwik-g ritwik-g commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

What

  • Manage Keys modal (API deployment, ETL and Task pipeline row menu): the keys table now uses tableLayout="fixed" with relative widths for API Key (62%), Active (4.5rem) and Actions (5.5rem). Description takes the remaining width and wraps a long value inside its cell.
  • The key fills the cell between Copy and Show/Hide and truncates with an ellipsis, so Show/Hide never spills into the next column. Before this, at a 20px root font the Edit icon covered Show (self-review round 1). The full key stays in the DOM.
  • Edit and Delete sit in one inline-flex, nowrap container, so they stay side by side.

Why

  • UN-4567, a regression from UN-4553 / UN-4553 [FIX] Hide API keys and webhook auth keys by default; copy keys without revealing #2398 (rc.452). The wider API Key column (Copy + masked key + Show/Hide) squeezed Actions, so its two inline-flex icons wrapped onto two lines. A long description with no break points widened the table past the modal, which put Active and Actions behind a horizontal scroll (UN-4553 test report c47593). Toggling Active reset that scroll; with no scroll left, that goes away too.

How

  • Under the browser's default auto layout a column width is only a hint (see the tableLayout note in DataTable.jsx; ResourceTable already uses fixed for the same reason). With fixed, the declared widths hold.
  • The Description text stays a direct text node of its <td>, with no wrapper element. The UI regression specs find a key's row with //td[text()='<name>'] and //td[contains(text(),'API Key')], so the wrap is done with CSS on .keys-table td:first-child (overflow-wrap: anywhere) and not with a truncating Typography.Text.
  • Show/Hide is pinned to the cell's right edge by the flexing key, so revealing a key doesn't move it. At 700px the revealed UUID loses its last couple of characters behind "…". The modal width (700 vs 800) is an open design question for Tannushree, and this PR leaves it at 700.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • No. Only layout changes. The masking, Copy-without-reveal, per-row Show/Hide and re-mask-on-reopen behaviour from UN-4553 [FIX] Hide API keys and webhook auth keys by default; copy keys without revealing #2398 is unchanged, and ManageKeys.test.jsx passes (9/9). I checked the locators the UI regression specs use for this modal (api-deploy-page.ts, pipeline-page.ts from unstract-ui-test-automation#369) against the rendered DOM. Row by description, key span, Show API key button, role=switch and the trash icon each resolve to exactly one element.
  • One side note, not caused by this PR: pipeline-page.ts's apiKeyCopyLocator (//th[text()='API Key']…) matches nothing because DataTable wraps every header title in a <span>. Nothing calls that getter.

Database Migrations

  • None

Env Config

  • None

Relevant Docs

  • None

Related Issues or PRs

Dependencies Versions

  • None

Notes on Testing

  • I rendered the modal from a throwaway Vite harness in headless Chromium at 1100×750, with three keys: a short description, a two-word one and a 70-character unbroken one.
    • Before: table scrollWidth 1082px in a 650px box (horizontal scroll); the Delete icon sits 30px below Edit on every row.
    • After: table 650px in 650px (no scroll); Edit and Delete on the same line; the long description wraps in its cell.
  • QA: open Manage Keys on an API deployment and on an ETL or Task pipeline. Add a key whose description is a long string with no spaces. Check that there's no horizontal scroll, that Edit and Delete sit on one line, and that Show/Hide on a key doesn't shift the Active or Actions columns.

Screenshots

Before/after screenshots were taken locally (headless harness above). The measurements are in Notes on Testing; I can attach the images on request.

Checklist

I have read and understood the Contribution Guidelines.

🤖 Generated with Claude Code

Lay the keys table out fixed with widths for API Key, Active and Actions,
so Description takes the remaining width and wraps a long unbroken value
instead of widening the table. Edit and Delete sit in one nowrap
container so they no longer stack.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ritwik-g ritwik-g left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — round 1 (self-review-loop), tip 1363ff641

Verdict — REQUEST CHANGES

Mode: LITE — single-pass, 19/19 lenses, one reader, no subagents. Eligible: 2 files / 24 lines, no disqualifiers.

Summary — Critical: 0 · High: 0 · Medium: 1 · Low: 0 · Lenses run: 19/19

Findings

1. [Medium] [Lens 3] — Fixed px column width can't hold the ch-sized key, so Show/Hide spills into the next column

  • Location: frontend/src/components/deployments/manage-keys/ManageKeys.jsx:L267-L268
  • Failure mode: the API Key column is fixed at 390px, but its content scales with font size: .manage-keys__key has min-width: 36ch (ManageKeys.css:L29), and the buttons and gaps use rem-based Tailwind sizes. At Chrome's "Large" font setting (20px root) the cell overflows enough that the Actions column's Edit icon sits on top of the Show key button. The click lands on Edit, so the key can't be revealed. At the default 16px root the eye button already ends 8px past its cell.
  • Evidence: headless Chromium, harness rendering ManageKeys at tip. 16px root: cell right edge 715, eye button right edge 723. 20px root: Playwright's click on Show API key fails with "lucide-pencil … from <td …text-center> subtree intercepts pointer events".
  • Suggested fix: size the columns in relative units, and let the key flex inside the cell with an ellipsis, so Show/Hide is pinned to the cell's right edge and can't overlap the next column.
  • Confidence: High
  • Verification: VERIFIED — headless Chromium (playwright 1228 shell) on a Vite harness, root font-size 16px and 20px; bounding-rect measurements and the intercepted click above

Lens checklist

  1. Spec & intent — Clean
  2. Architectural fit & precedent — Clean
  3. Correctness & edge cases — See findings #1
  4. Security — Clean
  5. Data integrity & migrations — N/A — no data or schema touched
  6. Concurrency — N/A — no shared state or async ordering changed
  7. API & contract compatibility — N/A — no wire format, endpoint or payload touched
  8. Reliability & resilience — N/A — no external calls
  9. Performance & cost — Clean
  10. Observability — N/A — no new code path that logs or meters
  11. Operational safety — N/A — presentational change, no rollout surface
  12. LLM/agent-specific — N/A — no model, prompt or tool path
  13. Testing — Clean
  14. Dependencies & build — N/A — no dependency or build change
  15. Code quality — Clean
  16. Doc & comment accuracy — Clean
  17. Cross-document rule consistency — Clean
  18. On-prem deployment experience — N/A — no chart, values, schema, on-prem settings or on-prem docs touched
  19. Scaling & workload lifecycle — N/A — no worker, autoscaling or lifecycle settings touched

Notes on the lenses:

  • Lens 1: the title follows TICKET [TYPE] text, as the workspace CLAUDE.md requires.
  • Lens 2: tableLayout="fixed" matches the ResourceTable precedent.
  • Lens 4: the masking and reveal logic is untouched, and the key text isn't put into any new attribute or tooltip.
  • Lens 13: the existing 9 tests pass and none were edited. The change adds no branching logic, and jsdom can't measure layout, so I verified it by rendering.
  • Lens 15: biome check's warnings are all on pre-existing lines, and formatting is clean.
  • Lens 17: the sweep found no other references to keys-table or the new classes. git log <mb>..origin/main is empty, so the branch is current.

Open questions for the author

  • With a 700px modal, Description gets about 100px once API Key holds a full UUID, so a two-word description like "Production ingest" wraps. Is that acceptable, or should the modal widen? Changing the width is a visual choice.

Assumptions made

  • The Chromium in the harness, with no app shell, lays out the table the same way the app does. The before state reproduced the reported stacking and horizontal scroll, which supports this.

boundary-sweep: not triggered (no outbound client, endpoint, log line or error path)
rule-storage-check: not covered — no markdown in the diff

dataIndex: "api_key",
key: "api_key",
// Copy, a revealed UUID key (.manage-keys__key) and Show/Hide.
width: 390,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] [Lens 3] — Fixed px column width can't hold the ch-sized key, so Show/Hide spills into the next column (finding #1, round 1, tip 1363ff641)

390px doesn't scale, but the cell's content does (.manage-keys__key { min-width: 36ch } and the rem-based buttons). At a 20px root font, the Actions column's Edit icon covers the Show key button, so the click hits Edit and the key can't be revealed. At 16px the eye button already ends 8px past the cell (715 vs 723).

Suggested fix: relative column units, and a key that flexes inside the cell with an ellipsis, so Show/Hide stays pinned inside the cell.

Verification: VERIFIED. Headless Chromium on a Vite harness at 16px and 20px root font, with bounding-rect measurements and a Playwright click intercepted by lucide-pencil.

The API Key column was a fixed 390px while the key's 36ch minimum width
and the rem-sized buttons grow with the font. At a 20px root font the
Edit icon covered the Show key button. The key now fills the cell
between Copy and Show/Hide and truncates with an ellipsis; column widths
are relative (62%, 4.5rem, 5.5rem). The full key stays in the text for
Copy and the UI regression specs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@ritwik-g ritwik-g closed this Oct 30, 2026
@ritwik-g
ritwik-g deleted the UN-4567-manage-keys-layout branch October 30, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant