fix: consistent Copied feedback on every copy button - #61
Conversation
vishu-bh
left a comment
There was a problem hiding this comment.
Thanks @marekdano !
Solid improvements, Just some minor things to check
|
|
||
| const copy = useCallback( | ||
| async (value: string) => { | ||
| const ok = await copyToClipboard(value); |
There was a problem hiding this comment.
Multiple clicks can leave overlapping copyToClipboard promises. An older request resolving last can overwrite newer feedback and reset its timer. Also, if the component unmounts before this await completes, cleanup runs before any timer exists; completion then updates unmounted state and schedules an uncleared timer.
Could we track mounted state plus a request sequence, allowing only the latest mounted invocation to update status/timer? Please add deferred-promise tests covering out-of-order completion and unmount-before-resolution.
| aria-label={`Copy ${title.toLowerCase()}`} | ||
| <CopyButton | ||
| value={schemaText} | ||
| label={`Copy ${title.toLowerCase()}`} |
There was a problem hiding this comment.
CopyButton displays label in its idle tooltip, so this formerly aria-only English string is now visible in Spanish and Portuguese locales. Same pattern exists in TestConnectionPanel and both server details panels. Please move these labels to locale messages. PromptPreviewResult should also use an actionable label such as “Copy JSON” instead of only “JSON”.
| <DialogContent className="max-w-2xl"> | ||
| <DialogContent | ||
| className="max-w-2xl" | ||
| onOpenAutoFocus={(event) => { |
There was a problem hiding this comment.
This autofocus override fixes a specific layered-tooltip regression, but current tests do not assert initial focus or single-press Escape dismissal for this dialog. Please add a test confirming the footer Close button receives initial focus and one Escape closes the dialog.
Signed-off-by: Marek Dano <mk.dano@gmail.com>
Signed-off-by: Marek Dano <mk.dano@gmail.com>
5875826 to
bb3f5d0
Compare
Signed-off-by: Marek Dano <mk.dano@gmail.com>
Closes #44
Summary
Copy-to-clipboard buttons existed in 17 places behaving four different ways — most gave no signal that a click did anything, including
TokenCreatedDialog, where a copy failure is costliest since the token is shown exactly once.This consolidates all of them onto two new shared primitives:
useCopyToClipboard(src/hooks/useCopyToClipboard.ts) — owns the clipboard write, transientidle/copied/errorstatus, reset timeout, and unmount cleanup.CopyButton(src/components/ui/copy-button.tsx) — the icon button: a hybrid tooltip that stays uncontrolled on hover/focus (so the ordinary "Copy X" hint keeps working) but force-opens on copy so touch users see the confirmation too; a fixedaria-label; a singlerole="status"region for the transient announcement; and internalstopPropagationso it can't be forgotten at a call site.copyToClipboard(src/lib/clipboard.ts) now returnsPromise<boolean>instead of firing-and-forgetting, so success/failure can actually be shown.Consolidated onto
CopyButtonCopyValueandCodeBlocknow delegate to it instead of managing their own stateToolsTable,ToolSchemaDialog,ResourcesTable,VirtualServerDetailsPanel,PromptDefinitionTable,TokenCreatedDialog,MCPServerDetailsPanel, andTestConnectionPanelnow show the same feedbackServersTableandToolFormdropped their hand-rolleduseState/setTimeoutcopy logic (including a timer leak inToolForm)Root
TooltipProviderMounted once in
App.tsx(delayDuration=0), replacing three duplicated local providers incode-block.tsx,card-tag.tsx, andGateways.tsx.CopyButtonoverrides to ~400ms locally so the 14 newly-tooltipped icon buttons don't feel twitchy on hover.i18n
Added
common.copied/common.copyFailedtoen-US,es-ES,pt-BR; retired the three now-redundant per-feature "Copied!" keys (mcpServer.table.copied,tools.form.copied,prompts.details.code.copySuccess).Bug fix along the way
Wrapping the copy buttons in
TokenCreatedDialogandToolSchemaDialogin tooltips brokeEscape-to-close for those dialogs: Radix auto-focuses the first focusable descendant on open, and when that's aCopyButton, its tooltip opens on focus and becomes the topmost dismissable layer — so the firstEscapepress closed the tooltip instead of the dialog. Fixed by redirecting each dialog's initial auto-focus to its Close button.Non-goals (per issue)
document.execCommandfallback for non-secure contextsTest plan
npx vitest run— 2948 tests passing (171 files), including newuseCopyToClipboardandCopyButtonunit testsnpx tsc -b --noEmit— cleannpx eslint src— cleannpx playwright test— all 213 e2e tests passing across every spec, including theToolSchemaDialogandTokenCreatedDialogflows and the escape-to-close drawer test