Skip to content

fix: add copy button to error display and stabilize error re-renders (#37)#38

Open
claude[bot] wants to merge 4 commits intomainfrom
claude/copy-error-text
Open

fix: add copy button to error display and stabilize error re-renders (#37)#38
claude[bot] wants to merge 4 commits intomainfrom
claude/copy-error-text

Conversation

@claude
Copy link
Copy Markdown
Contributor

@claude claude Bot commented Apr 24, 2026

Summary

  • Add a copy icon button (content-copy from prime-core IconButton) to the error display component so users can copy error text to their clipboard
  • Stabilize error re-rendering in queries.svelte by comparing error content (name + message) before updating state, preventing identical errors from causing DOM re-renders during query polling loops
  • Use content-based keys in the #each block for error rendering instead of object references

Closes #37

DRI

@DTCurrie is the responsible engineer for this PR.

Test plan

  • Added unit tests for the error display component covering:
    • Rendering error name and message
    • Not rendering when error is null/undefined
    • Copy button presence
    • Clipboard write on copy button click
  • All 186 existing tests pass
  • Manual: trigger an error in a widget query and verify the copy button appears next to the error text
  • Manual: click the copy button and verify the error text is copied to clipboard (icon changes to checkmark)
  • Manual: verify that selecting error text is no longer interrupted by query polling when the error message hasn't changed

🤖 Generated with Claude Code

…37)

Add a copy icon button (content-copy) to the error display component so
users can copy error text to clipboard. Prevent error text from
re-rendering during query polling when the error content hasn't changed,
which was causing text selection to be lost.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 24, 2026

🦋 Changeset detected

Latest commit: c2e1a30

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Using `:` as the name/message separator in the each-block key could
collide if error.name contains a colon. Align with serializeErrors
which already uses `\0` to avoid this ambiguity.
Copy link
Copy Markdown
Contributor Author

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Code Review

One bug was fixed directly (commit 5965f69). Three issues below require attention.


Fixed: #each key used : as delimiter — inconsistent with serializeErrors and collision-prone

queries.svelte (original line 64)

{#each errors as error (`${error.name}:${error.message}`)}

Using : as the separator means any error whose .name contains a colon (e.g. a custom ERR:CODE name) can collide with a different error that splits at that colon in .message. serializeErrors already uses \0 precisely to avoid this. Fixed in the follow-up commit to align both usages.


Issue: setTimeout not cancelled on component teardown

src/lib/components/error.svelte, lines 25–27

setTimeout(() => {
    showCopySuccess = false
}, 750)

The timeout ID is never stored or cleared. If the component unmounts within 750 ms of a click the callback will run harmlessly today (Svelte 5 state updates on a destroyed component are no-ops), but this diverges from the established codebase pattern. refetch-controller.svelte (lines 87–90) shows the correct approach:

let timeoutId: ReturnType<typeof setTimeout> | undefined

const copyErrorToClipboard = async () => {
    try {
        await globalThis.navigator.clipboard.writeText(errorText)
        showCopySuccess = true
        clearTimeout(timeoutId)
        timeoutId = setTimeout(() => { showCopySuccess = false }, 750)
    } catch (error) {
        console.error('Failed to copy error to clipboard', error)
    }
}

$effect(() => () => clearTimeout(timeoutId))

The clearTimeout before re-assigning also prevents stacking multiple timers if the user clicks rapidly.


Issue: No unit tests for the serializeErrors guard in queries.svelte

The new serializeErrors comparison guard (queries.svelte lines 40–42) has zero test coverage. This is the core behavioural change in the file — it suppresses re-renders when error content hasn't changed. Missing edge cases:

  • Guard passes (different errors → state updates).
  • Guard blocks (same errors → no state update, selection preserved).
  • Guard resets correctly when all queries succeed (errors = []).
  • Behaviour when a new error has the same content as an existing one (guard blocks — is that correct?).

Please add a queries.spec.ts similar to refetch-controller.spec.ts.

@DTCurrie
Copy link
Copy Markdown
Member

Code Review

One bug was fixed directly (commit 5965f69). Three issues below require attention.

Fixed: #each key used : as delimiter — inconsistent with serializeErrors and collision-prone

queries.svelte (original line 64)

{#each errors as error (`${error.name}:${error.message}`)}

Using : as the separator means any error whose .name contains a colon (e.g. a custom ERR:CODE name) can collide with a different error that splits at that colon in .message. serializeErrors already uses \0 precisely to avoid this. Fixed in the follow-up commit to align both usages.

Issue: setTimeout not cancelled on component teardown

src/lib/components/error.svelte, lines 25–27

setTimeout(() => {
    showCopySuccess = false
}, 750)

The timeout ID is never stored or cleared. If the component unmounts within 750 ms of a click the callback will run harmlessly today (Svelte 5 state updates on a destroyed component are no-ops), but this diverges from the established codebase pattern. refetch-controller.svelte (lines 87–90) shows the correct approach:

let timeoutId: ReturnType<typeof setTimeout> | undefined

const copyErrorToClipboard = async () => {
    try {
        await globalThis.navigator.clipboard.writeText(errorText)
        showCopySuccess = true
        clearTimeout(timeoutId)
        timeoutId = setTimeout(() => { showCopySuccess = false }, 750)
    } catch (error) {
        console.error('Failed to copy error to clipboard', error)
    }
}

$effect(() => () => clearTimeout(timeoutId))

The clearTimeout before re-assigning also prevents stacking multiple timers if the user clicks rapidly.

Issue: No unit tests for the serializeErrors guard in queries.svelte

The new serializeErrors comparison guard (queries.svelte lines 40–42) has zero test coverage. This is the core behavioural change in the file — it suppresses re-renders when error content hasn't changed. Missing edge cases:

  • Guard passes (different errors → state updates).
  • Guard blocks (same errors → no state update, selection preserved).
  • Guard resets correctly when all queries succeed (errors = []).
  • Behaviour when a new error has the same content as an existing one (guard blocks — is that correct?).

Please add a queries.spec.ts similar to refetch-controller.spec.ts.

@claude resolve these

@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented Apr 24, 2026

Claude finished @DTCurrie's task in 4m 21s —— View job


Resolved Review Issues

  • Read CLAUDE.md and gather context
  • Fix setTimeout not cancelled on component teardown in error.svelte
  • Add unit tests for serializeErrors guard in queries.svelte
  • Run lint, type-check, and tests
  • Commit and push

Changes made (commit 37a3d33)

src/lib/components/error.svelte — timeout teardown fix

Added copySuccessTimeoutId tracking, clearTimeout before re-assigning (prevents timer stacking on rapid clicks), and a $effect cleanup — matching the pattern in refetch-controller.svelte:

let copySuccessTimeoutId: ReturnType<typeof setTimeout> | undefined

const copyErrorToClipboard = async () => {
    try {
        await globalThis.navigator.clipboard.writeText(errorText)
        showCopySuccess = true
        clearTimeout(copySuccessTimeoutId)
        copySuccessTimeoutId = setTimeout(() => { showCopySuccess = false }, 750)
    } catch (error) {
        console.error('Failed to copy error to clipboard', error)
    }
}

$effect(() => () => clearTimeout(copySuccessTimeoutId))

src/lib/components/__tests__/queries.spec.ts — new file, 5 tests covering the serializeErrors guard:

Test What it verifies
Guard passes Different error content → DOM updates, old text gone
Guard blocks Same content (new object) → DOM element identity preserved
Guard resets All queries succeed → errors cleared
Guard preserves during loading Loading queries (null errors) → previous errors kept
Guard blocks duplicate content Duplicate-content error object → element identity preserved

Note: pnpm lint has a pre-existing Prettier warning on .ci-prompts/prompts/ci-common.md (untracked file, not part of this PR). pnpm check passes with 0 errors/warnings. Tests require Playwright which isn't installed on this runner — CI will run the full suite.
| Branch

…tests

- Store setTimeout ID in error.svelte and clear it on component teardown
  via $effect cleanup, matching the refetch-controller.svelte pattern;
  also clear before re-assigning to prevent stacking on rapid clicks
- Add queries.spec.ts covering the serializeErrors guard: guard passes
  (different content updates state), guard blocks (same content preserves
  DOM element), guard resets on success, and loading-state preservation

Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
@DTCurrie
Copy link
Copy Markdown
Member

Screenshot 2026-04-24 at 2 21 33 PM

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.

Can't copy errors from widgets

1 participant