Skip to content

fix(platform): use icon buttons for inline table row actions and confirm passkey revoke (#1963)#2112

Open
larryro wants to merge 3 commits into
mainfrom
tale/xs7bfc2593mvdnna37sxt8gmxh896fdr
Open

fix(platform): use icon buttons for inline table row actions and confirm passkey revoke (#1963)#2112
larryro wants to merge 3 commits into
mainfrom
tale/xs7bfc2593mvdnna37sxt8gmxh896fdr

Conversation

@larryro

@larryro larryro commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Resolves #1963. Audits non-dropdown table/list row actions and inconsistent delete-confirmation coverage.

  • passkey-section — the ghost "Remove" text button is now a trash IconButton (tooltip via aria-label) that opens a DeleteDialog confirmation. Previously it revoked the passkey immediately on click with no confirmation — the exact gap called out in the issue.
  • moderation-mapping-list — row "Edit" text button → IconButton.
  • chat-filter-config — row Edit/Delete buttons → IconButton for a consistent icon-only action cluster.
  • New i18n keys (passkeys.revokeConfirmTitle/Description, moderationProvider.editMappingAria) in en/de/fr; 2FA admin docs updated in all three locales.

Other feature tables (customers, agents, products, vendors, etc.) already use the canonical EntityRowActions 3-dot menu with confirmation dialogs, so no change was needed there.

Acceptance criteria

  • Inline table actions are icon buttons.
  • All destructive row actions confirm before executing.

Verification

  • typecheck (platform): green.
  • oxlint --type-aware on changed files: 0 warnings / 0 errors.
  • i18n parity test (lib/i18n/messages.test.ts): 23 passed.
  • UI tests (chat-filter-config, moderation-provider-config): 12 passed.
  • commitlint on the commit message: clean.

Tale Agent added 2 commits June 24, 2026 10:09
…irm passkey revoke (#1963)

Convert non-dropdown table/list row actions to IconButtons (with tooltips
via aria-label) and route the destructive passkey revoke through a
DeleteDialog confirmation.

- passkey-section: the ghost "Remove" text button is now a trash IconButton
  that opens a DeleteDialog before revoking (previously revoked on click with
  no confirmation).
- moderation-mapping-list: row Edit text button -> IconButton.
- chat-filter-config: row Edit/Delete buttons -> IconButton for consistency.
- Added passkeys.revokeConfirm{Title,Description} and
  moderationProvider.editMappingAria keys to en/de/fr; updated the 2FA docs.

Resolves #1963.
@larryro

larryro commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Desk review — #1963 (icon buttons for row actions + delete-confirmation audit)

Verdict: NOT READY — one acceptance-criterion gap remains. The conversions in this PR are correct, clean, and fully green on CI, but the issue's first acceptance criterion ("Inline table actions are icon buttons") is an audit of all tables, and a full-repo sweep finds one inline text per-row action button still unconverted.

What was verified (all good)

  • Correctness: passkey-section.tsx confirmRevoke() deletes the correct revokeTarget.id; success clears the target, error/exception keep the dialog open, isRevoking is reset in finally. All converted IconButtons keep their original onClick (and e.stopPropagation() on clickable rows in active-holds/matters). release-requests-section.tsx correctly maps the old title= self-approve hint onto tooltip= (IconButton falls back tooltip ?? aria-label); the real self-approve block still lives in ApproveReleaseDialog (disableConfirm), matching prior behavior.
  • Confirmations: every destructive converted action confirms — passkey revoke → DeleteDialog; chat-filter category delete → ConfirmDialog; close-matter/approve/reject/request-release → their dialogs.
  • a11y: every icon-only button has a non-empty aria-label and an auto-derived tooltip. New i18n keys (moderationProvider.editMappingAria, passkeys.revokeConfirmTitle, passkeys.revokeConfirmDescription) are present in en/de/fr; every other aria/tooltip key already existed.
  • Elegance: no dead code — tCommon removed only where unused (still used elsewhere in chat-filter), Button/Lock still used in active-holds, Button still used in matters. Docs (en/de/fr two-factor) updated accurately.
  • Tests / CI: IconButton unit (23), delete-dialog (3), chat-filter-config + moderation-provider-config (12) all pass locally; all CI checks green (incl. 16-shard Playwright, Type check, Lint, Knip, Format).

Blocking gap

  1. services/platform/app/features/apps/registry/connected/run-list.tsx:109 — the per-row "Watch Live" action (runs.watchLive) is still an inline text <Button variant="secondary">. A full sweep of services/platform/app confirms this is the only remaining inline text per-row action button (all others are converted, use the 3-dot EntityRowActions, or are section/toolbar/empty-state buttons). Per the issue's "audit all tables" scope and AC-1, convert it to an IconButton with a tooltip (e.g. an eye/play icon labelled runs.watchLive), keeping the existing disabled={!id} and navigation onClick. It's non-destructive, so no confirmation is needed.

Non-blocking nits (optional)

  • active-holds-section.tsx:161 requestRelease IconButton omits size="sm" while its peers in matters/release-requests use it (pre-existing; minor visual inconsistency in a dense table).
  • New passkey-revoke confirmation flow has no unit test (it is exercised by the green Playwright suite); a small unit test would lock in the confirm-before-delete behavior.

Once "Watch Live" is converted, this meets the bar.

@larryro

larryro commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Desk review — #1963 icon buttons + delete-confirmation audit

Verdict: ✅ Ready to merge.

Reviewed the whole implementation (not just the diff) across two rounds with parallel review agents, ran the project's own tests locally, and confirmed CI.

CI

All required checks pass (Build platform, Lint, Knip, Format, Unit, Migrations, Opengrep, 16× Playwright platform shards, CodeQL js+py, etc.). The only non-green jobs are fork-only (Trivy, Validate images (fork PR), Smoke test (fork PR)) which skip on a same-repo branch — expected, not a failure.

Tests I ran (locally, this branch)

  • bunx vitest --run lib/i18n/messages.test.ts23 passed (i18n parity).
  • bunx vitest --run --config vitest.ui.config.ts chat-filter-config.test.tsx moderation-provider-config.test.tsx delete-dialog.test.tsx15 passed.

What's correct

  • passkey-section — genuinely closes the real gap: origin/main revoked the passkey immediately on click with no confirmation; now the trash IconButton opens a DeleteDialog. State machine is sound — double-fire is blocked by isDeleting/disabled confirm, error paths keep the dialog open for retry and reset isRevoking in finally, success closes + invalidates, the A→B re-target race resolves to the latest target, and the {name} interpolation degrades to the unnamed label while closing. No stuck-open path.
  • Legal-hold (active-holds / matters / release-requests) — Button→IconButton conversions are faithful; every onClick/stopPropagation preserved, conditional close button preserved. The titletooltip swap on the approve button is correct (IconButton has no visible text, so aria-label supplies the name and tooltip carries the self-approve-blocked hint); self-approve blocking is unchanged (handled downstream in the approve dialog).
  • run-list / chat-filter-config / moderation-mapping-list — all converted buttons keep meaningful translated aria-labels; the IconButton primitive also surfaces them as tooltips. No dead code; the scoped tCommon removal is clean (the remaining tCommon is in a different component).
  • i18n — the 3 new keys (editMappingAria, passkeys.revokeConfirmTitle/Description) exist in en/de/fr with matching {category}/{name} placeholders and correct namespace nesting; call-site params line up. (de-CH is a deliberate sparse overlay; absence there is expected.)

Non-blocking follow-up (optional)

Four sibling governance editors — model-access-editor, budget-editor, feature-flags-editor, default-model-editor — render their inline row edit/delete actions as icon-only <Button variant="ghost" size="icon"><Trash2/></Button> with a title tooltip, rather than the IconButton primitive. They are already icon-only and already gated by ConfirmDialog (onRemoveRule/setDeletingIndex only opens the dialog; deletion happens on confirm), so they satisfy both acceptance criteria — no missing confirmation, no text button. Converting them to IconButton would gain primitive consistency and a proper aria-label (vs the weaker title-only accessible name), but it's a refinement on a pre-existing pattern, out of this PR's scope, not a blocker.

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.

Improvement: Use icon buttons for non-dropdown table row actions; audit delete confirmations

1 participant