feat(frontend): Command Palette UX/keyboard/tests and Webhook Logs accessibility fixes - #1586
Merged
emdevelopa merged 4 commits intoSep 26, 2026
Conversation
|
@Mawuli-tech is attempting to deploy a commit to the Emmanuel's projects Team on Vercel. A member of the Team first needs to authorize it. |
…command execution (emdevelopa#1509) CommandPalette has no optimistic-UI-update-tied-to-a-network-call pattern anywhere today: command selection either navigates via router.push, toggles theme synchronously, or opens the client-side AssetConverter view (which already manages its own loading/error state around its Horizon query). The only genuine async, failable side effect in this component is the "Copy API Key" command's clipboard write. Previously, select() called setOpen(false) synchronously before awaiting navigator.clipboard.writeText(), so a failed copy left the palette already dismissed with only an error toast and no way to retry from where the user was. There was also no guard against firing the async action twice (e.g. double Enter) and no visible pending state during the await. Fix: track which command is currently executing (executingId), disable re-entrant selection while an async command is in flight, and stop optimistically closing the palette before the outcome is known. The palette now stays open with a busy indicator on the row and a disabled search input during the clipboard write, closing only on success; on failure it "rolls back" by simply remaining open on the command list so the user can see the error and retry.
…-select (emdevelopa#1510) CommandPalette already had solid baseline keyboard support: global Cmd/Ctrl+K toggle, ArrowUp/ArrowDown navigation with wraparound, Enter to select, Escape to close, and correct role="listbox"/role="option"/ aria-activedescendant wiring on the search combobox. Reviewing the actual keydown handler turned up three concrete, missing conventions rather than inventing new ones: - No focus trap: Tab had no handler at all, so pressing Tab while the palette was open would move real DOM focus out of the dialog and onto whatever was behind the backdrop, even though the dialog is meant to be modal. Tab/Shift+Tab now cycle the active result the same way ArrowDown/ArrowUp do, giving them parity and keeping focus trapped. - No Home/End to jump to the first/last result, a standard list/palette convention. Added, but only intercepted when the query is empty so native cursor-to-start/end text editing in the search input isn't hijacked while a query is being typed. - No number-key quick-select. Digits 1-9 now jump straight to and run the Nth visible result, again only when the query is empty so digits keep working as normal search input once typing has started. Both new interactions respect the executingId guard from emdevelopa#1509 so they can't fire while an async command is already in flight.
…lopa#1512) Investigated the actual webhook-logs feature (page.tsx -> WebhookLogs.tsx -> WebhookDetailModal.tsx -> the shared ui/Modal.tsx) rather than assuming the issue's "focus trap" framing applied to the log table itself. The plain, read-only <table> listing deliveries doesn't need a focus trap. The originally-planned fix for this issue also wired up Modal.tsx's dead dialogRef (never attached via ref={dialogRef}, so its existing Tab-cycling focus-trap implementation never ran) and added tabIndex/role="button"/keyboard handling to each WebhookLogs row, both genuinely missing at the time. Rebasing onto main found both already fixed independently: Modal.tsx already has ref={dialogRef}, role="dialog", aria-modal, and aria-labelledby (merged via emdevelopa#1584, for Fiat Onramp Modal's own focus-trap issue), and WebhookLogs.tsx's rows already have tabIndex={0}/role="button"/Enter+Space handling (merged via emdevelopa#1582, WebhookLogs' own keyboard-navigation issue), both fixing the exact same underlying dead code and accessibility gaps this issue also targeted, just filed and closed under different issue numbers. Kept upstream's versions as-is rather than reintroducing equivalent but differently-worded duplicates. What's left, and genuinely new: the <table> itself had no accessible name and its <th> cells had no scope="col", and neither of the other two merged PRs touched this (no sortable columns exist here, so aria-sort doesn't apply, unlike RecentPayments.tsx elsewhere in this codebase). Added aria-label to the table and scope="col" to every header cell.
…ria-hidden bug found while writing it (emdevelopa#1511) CommandPalette.test.ts only ever tested the pure data/search layer (commandPaletteData.ts) -- zero tests rendered the actual component, so open/close, keyboard navigation (including the Tab focus trap, Home/End and number quick-select added in emdevelopa#1510), mouse selection, and the async copy-API-key flow added in emdevelopa#1509 had no coverage at all. Adds CommandPalette.test.tsx (22 cases) covering all of the above, following this codebase's existing @testing-library/react convention (see MultisigApprovalModal.test.tsx). While writing the very first test, role-based queries for the dialog (getByRole('dialog')) could not find it at all, even though the element was genuinely present in document.body.innerHTML. The backdrop div wrapping the dialog had aria-hidden="true" on it -- almost certainly intended to hide only the decorative backdrop from assistive technology, but placed on the wrapper it hides the entire subtree, including the interactive dialog itself, from every screen reader. This is a real, pre-existing accessibility bug (present on upstream/main, unrelated to emdevelopa#1509/emdevelopa#1510's logic changes), not a test authoring mistake -- confirmed testing-library's role query is accessibility-tree-based and correctly reports the dialog as absent per ARIA semantics as long as an ancestor is aria-hidden. Fixed by removing aria-hidden from the backdrop wrapper and adding aria-modal="true" to the dialog itself instead -- the standard, correct way to signal that everything outside a modal dialog is inert, without hiding the dialog from AT. This mirrors exactly how emdevelopa#1512 fixed the equivalent gap in the shared Modal.tsx component. Also added an Element.prototype.scrollIntoView polyfill to the shared test setup (jsdom does not implement it at all, not even as a no-op, so CommandPalette's active-item-scroll-into-view effect threw on every render once the dialog could actually be found and exercised). This is a project-wide jsdom gap in the same category as the existing matchMedia/IntersectionObserver/ResizeObserver stubs already in this file, not something specific to CommandPalette -- confirmed it also fixes 22 pre-existing failures unrelated to this PR (NotificationCenter, TwoFactorAuthSetup, and others that were silently broken by the same missing API), verified before and after against a stash of just this one change.
Mawuli-tech
force-pushed
the
feature/command-palette-and-webhook-logs-ux
branch
from
September 26, 2026 07:21
29c80c0 to
50e7a26
Compare
|
@Mawuli-tech Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
2 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
Implements all 4 assigned frontend UX/accessibility issues on
CommandPaletteandWebhookLogs, plus a real accessibility bug found while writing test coverage and a shared jsdom test-infra gap that turned out to affect the whole suite. While rebasing, found that #1512's originally-plannedModal.tsx/row-keyboard-nav fixes had already landed independently under different issue numbers, so that commit was narrowed to only what genuinely remained.closes #1509
closes #1510
closes #1511
closes #1512
Changes
[Frontend] Implement optimistic state rollback on network failure in Command Palette #1509, Optimistic state rollback on network failure in Command Palette:
CommandPalettehad no optimistic-UI-update-tied-to-a-network-call pattern anywhere: command selection either navigates, toggles theme synchronously, or opens the client-side asset converter (which already manages its own loading/error state). The only genuine async, failable side effect is the "Copy API Key" command's clipboard write, which previously closed the palette optimistically before awaiting the result, so a failed copy left the user with a dismissed palette and only a toast, with no way to retry from where they were. Fixed: the palette now stays open with a visible pending state and a disabled search input during the write, closing only on success; on failure it "rolls back" by simply remaining open on the command list. Also added a guard against re-entrant selection while the async command is in flight.[Frontend] Enhance keyboard navigation and shortcut bindings for Command Palette #1510, Enhanced keyboard navigation and shortcut bindings for Command Palette: baseline keyboard support (Cmd/Ctrl+K, arrows, Enter, Escape) was already solid. Reviewing the actual handler turned up three concrete, missing conventions: no focus trap (Tab had no handler at all, so it escaped the modal to the page behind it; now Tab/Shift+Tab cycle the active result, giving them parity with arrow keys and keeping focus trapped), no Home/End to jump to the first/last result (added, but only intercepted when the query is empty so native text-cursor behavior isn't hijacked while typing), and no number-key quick-select for 1-9 (added, same empty-query guard).
[Frontend] Implement WCAG 2.1 AA accessible focus trap and ARIA attributes in Webhook Logs Table #1512, WCAG 2.1 AA accessible focus trap and ARIA attributes in Webhook Logs Table: investigated the actual feature (
page.tsxtoWebhookLogs.tsxtoWebhookDetailModal.tsxto the sharedui/Modal.tsx) rather than assuming the issue's "focus trap" framing applied to the log table itself; a plain read-only table doesn't need one. The originally-planned fix here also wired upModal.tsx's deaddialogRef(never attached viaref={dialogRef}, so its existing Tab-cycling focus-trap implementation never ran) and addedtabIndex/role="button"/keyboard handling to eachWebhookLogsrow, both genuinely missing when this work started. While rebasing ontomain, found both already fixed independently:Modal.tsxalready hasref={dialogRef},role="dialog",aria-modal, andaria-labelledby(merged via PR feat(frontend): implement WCAG 2.1 AA accessible focus trap and ARIA attributes in Fiat Onramp Modal #1584, filed for the Fiat Onramp Modal's own separate focus-trap issue), andWebhookLogs.tsx's rows already havetabIndex={0}/role="button"/Enter+Space handling (merged via PR feat(frontend): add keyboard navigation to WebhookLogs table rows #1582, filed for WebhookLogs' own separate keyboard-navigation issue), both fixing the exact same underlying dead code and accessibility gaps this issue also targeted, just filed and closed under different issue numbers. Kept upstream's versions as-is rather than reintroducing equivalent but differently-worded duplicates. What's left, and genuinely new: the<table>itself had no accessible name and its<th>cells had noscope="col", and neither of the other two merged PRs touched this (no sortable columns exist here, soaria-sortdoesn't apply, unlikeRecentPayments.tsxelsewhere in this codebase). Addedaria-labelto the table andscope="col"to every header cell.[Frontend] Add comprehensive component unit and snapshot tests for Command Palette #1511, Comprehensive component unit and snapshot tests for Command Palette: the existing
CommandPalette.test.tsonly ever tested the pure data/fuzzy-search layer; zero tests rendered the actual component, so open/close, keyboard navigation (including everything [Frontend] Enhance keyboard navigation and shortcut bindings for Command Palette #1510 added), mouse selection, and the async copy-API-key flow from [Frontend] Implement optimistic state rollback on network failure in Command Palette #1509 had no coverage at all. AddedCommandPalette.test.tsx(22 cases) covering all of the above, following this codebase's existing@testing-library/reactconvention.Bugs found and fixed while writing #1511's tests (not separately assigned issues, but directly surfaced by this work)
getByRole("dialog")could not find the dialog at all, even though it was genuinely present in the rendered HTML. The backdrop wrapping the dialog hadaria-hidden="true"on it, almost certainly meant to hide only the decorative backdrop from assistive technology, but placed on the outer wrapper it hides the entire subtree, including the interactive dialog, from every screen reader. This is a real bug onupstream/main, unrelated to [Frontend] Implement optimistic state rollback on network failure in Command Palette #1509/[Frontend] Enhance keyboard navigation and shortcut bindings for Command Palette #1510's logic changes, confirmed viagit show upstream/main:.... Fixed by removingaria-hiddenfrom the wrapper and addingaria-modal="true"to the dialog instead, the correct way to signal a modal without hiding it from AT, mirroring exactly what the (independently merged)Modal.tsxfix already does.Element.prototype.scrollIntoViewat all (not even as a no-op), soCommandPalette's active-item-scroll-into-view effect threw once the dialog could actually be rendered and exercised in a test. Added a polyfill to the sharedsrc/test/setup.ts, in the same category as the existingmatchMedia/IntersectionObserver/ResizeObserverstubs already there. This incidentally fixes other pre-existing test failures unrelated to this PR (inNotificationCenter,TwoFactorAuthSetup, and others silently broken by the same missing API).Test plan
npm run lint(frontend): clean, 0 errors, only pre-existing unrelated warnings.npx vitest run src/components/CommandPalette.test.tsx src/components/WebhookLogs.test.tsx: 26/26 passing, run repeatedly for reliability.npx vitest run src/components/MultisigApprovalModal.test.tsx src/components/FiatOnrampModal.test.tsx: 58/58 passing, confirming the (independently merged) sharedModal.tsxfocus-trap/ARIA fix isn't broken by anything in this branch.upstream/mainbaseline (which itself includes two newly-merged accessibility PRs); none of the remaining failures touch anything in this PR (confirmed by file name; they're all in unrelated components likeKycSubmissionForm,TwoFactorAuthSetup,freighter.test.ts, etc.).