fix(dashboard): clear the CopyButton reset timer#528
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe ChangesCopyButton timer lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated code review started - full review. Results will be posted here. |
55b9f9b to
c09de15
Compare
|
⏳ Review in progress — Phase 0 complete. Existing comments: 2 issue comments (1 from coderabbitai, 1 from hermes-exosphere). No review threads exist. Now analyzing code and running tests... |
🔍 Automated Code Review📋 Executive SummaryThis PR fixes two real bugs in the 📊 Change Architecturegraph TD
A["CopyButton Component<br/>copy-button.tsx"] -->|"fixed: ref-track timer + cleanup"| B["armRevertTimer()<br/>useCallback"]
B -->|"clearTimeout before re-arm"| C["Rapid Re-click Fix<br/>✅ Stale timer cleared"]
C --> D["Revert Timer<br/>setTimeout(setCopied(false), 2000)"]
A -->|"useEffect cleanup on unmount"| E["Unmount Safety<br/>✅ Timer cleared"]
E --> F["No React Warning"]
G["Tests<br/>copy-button.test.tsx"] -->|"+61 lines, 2 new tests"| C
G -->|"+61 lines, 2 new tests"| E
H["Missing: type=button"] -.->|"⚠️ Claimed but not implemented"| A
style A fill:#87CEEB
style C fill:#90EE90
style E fill:#90EE90
style H fill:#FF6347
style G fill:#90EE90
Legend: 🟢 New/Fixed | 🔵 Modified | 🔴 Issue Found 🔴 Breaking Changes✅ No breaking changes detected. The component's public API (
|
| Check | Result | Notes |
|---|---|---|
| Race condition (rapid re-click) | ✅ Fixed | clearTimeout(revertTimerRef.current) before setting new timeout |
| Unmounted state update | ✅ Fixed | useEffect cleanup clears timer on unmount |
| Ref initialization | ✅ Correct | useRef<ReturnType<typeof setTimeout>>(undefined) — proper typing |
armRevertTimer dependency |
✅ Correct | useCallback([], []) — stable reference, no re-creation |
handleCopy dependency |
✅ Correct | Added armRevertTimer to dep array — no stale closure |
| Both clipboard paths updated | ✅ Both paths | Both navigator.clipboard.writeText success AND fallbackCopyText fallback call armRevertTimer() |
| Error path (both fail) | ✅ No timer armed | Catch block does NOT call armRevertTimer() when both methods fail |
| Pattern consistency | ✅ Matches raw-log-viewer.tsx |
Same Ref + clearTimeout + useEffect cleanup pattern |
No logical errors, race conditions, or bugs detected in the implementation.
Tests (__tests__/components/copy-button.test.tsx):
| Test | Coverage | Quality |
|---|---|---|
| "reverts to copy icon after 2s" (existing) | Happy path, single click | ✅ Good |
| "keeps checkmark after rapid clicks" (NEW) | Edge: rapid re-click at t=1.5s | ✅ Excellent — asserts at t=2.0s (stale timer prevented) AND t=3.5s (new timer fires correctly) |
| "does not update state after unmount" (NEW) | Edge: unmount mid-window | ✅ Good — spies on console.error, unmounts at t=0.5s, advances past 2s, asserts no warning |
| "falls back to execCommand" (existing) | Error: clipboard unavailable | ✅ Good |
| "does not show check when both fail" (existing) | Error: both clipboard methods fail | ✅ Good |
| "renders copy icon by default" (existing) | Initial state | ✅ Good |
| "copies text and shows check" (existing) | Happy path | ✅ Good |
| "applies custom className" (existing) | Props passthrough | ✅ Good |
Edge case analysis for the unmount test:
- The test uses
vi.spyOn(console, "error")and asserts.not.toHaveBeenCalled(). This is a valid test — if the component'ssetCopied(false)fires after unmount, React callsconsole.errorwith the "state update on unmounted component" warning. - ✅ Correct approach — the spy catches any
console.errorcall. The test would fail ifconsole.errorwas called for any reason, which is appropriate since no code in the test path should produce errors.
🧪 Evidence — Build & Test Results
TypeScript: ✅ Clean — bunx tsc --noEmit — no errors
Lint: ✅ Clean — bun run lint — 0 errors (5 pre-existing warnings unrelated to this change: 4 no-img-element, 1 unused eslint-disable directive)
Unit Tests — CopyButton only:
✓ __tests__/components/copy-button.test.tsx (8 tests) 469ms
All 8 tests pass (6 existing + 2 new)
Full Test Suite:
Test Files 121 passed (121)
Tests 2066 passed (2066)
Duration 66.21s
✅ All 2066 tests pass across 121 test files. Zero failures. No regressions.
Build:
✅ TypeScript compilation passes (bunx tsc --noEmit)
✅ Lint passes (0 errors)
🔗 Issue Linkage
This PR closes #523 — "Dashboard CopyButton: the 'copied ✓' timer is never cleared". The issue described both defects (rapid re-click and unmount mid-window) with reproduction steps. The implementation directly addresses both bugs using the exact approach suggested in the issue ("Store the timeout id in a useRef, clearTimeout it before re-arming, and clear it in a useEffect(() => () => clearTimeout(ref.current), []) cleanup").
One discrepancy: The issue also noted "Bonus: the <button> is also missing type=\"button\" at :53" — the PR claims to have added this but the code does NOT include it.
👥 Human Review Feedback
No human review comments exist on this PR. Two bot comments:
coderabbitai[bot]: Auto-generated review stack link — informational onlyhermes-exosphere: "Automated code review started" — prior review start marker
No unresolved review threads.
💡 Suggestions
-
Add
type="button": This was documented as part of the fix — it should be added to prevent implicit form submission. Single-line addition:<button type="button" // ← add this onClick={handleCopy}
-
Consider extracting the magic number
2000: The 2-second revert timeout is used in botharmRevertTimerand tests. Extracting it to a named constant (e.g.,const REVERT_DELAY_MS = 2000) at the module level would improve maintainability and testability:const REVERT_DELAY_MS = 2000;
-
Consider adding a test for rapid clicks in the fallback path: The existing tests for clipboard unavailable (
execCommand) don't test rapid re-click. SincearmRevertTimer()is called in all paths (clipboard API AND fallback), this could be a gap. However, the fallback is synchronous while the API path is async, so the timer behavior is consistent — low priority.
🏆 Verdict
VERDICT: APPROVED_WITH_SUGGESTIONS
The core fix is correct, well-tested, and follows existing patterns. All tests pass, TypeScript is clean, lint is clean. One blocking issue: type="button" must be added as documented (or the PR description updated if it was intentionally omitted).
Automated code review · 2026-07-16 12:00:50 UTC
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved with suggestions (see summary + inline comment). Core fix is correct and well-tested. One issue: type="button" is claimed in description/commit but missing from code. ✅
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
c09de15 to
c472d02
Compare
|
Good catch on the mismatch — that was my doing, and the resolution is to restore the code rather than edit the description. I'd stripped Verified after restoring: |
|
Automated code review started - full review. Results will be posted here. |
|
⏳ Re-review in progress — Phase 0-2 complete. Previously reported issue has been resolved. Running build & tests now... |
|
✅ Resolved 1 previously-reported issue — the |
🔍 Automated Code Review (Re-review)📋 Executive SummaryThis PR fixes two real bugs in the 📊 Change Architecturegraph TD
A["CopyButton<br/>copy-button.tsx<br/>(modified)"] -->|"ref-track timer + cleanup"| B["armRevertTimer()<br/>useCallback"]
B -->|"clearTimeout before re-arm"| C["Rapid Re-click Fix<br/>Stale timer cleared"]
C --> D["Revert Timer<br/>setTimeout(setCopied(false), 2000)"]
A -->|"useEffect cleanup on unmount"| E["Unmount Safety<br/>Timer cleared"]
E --> F["No React Warning"]
G["Tests<br/>copy-button.test.tsx<br/>(+61 lines)"] -->|"2 new tests"| C
G -->|"2 new tests"| E
H["type='button'<br/>Added"] -.-> A
style A fill:#87CEEB
style C fill:#90EE90
style E fill:#90EE90
style H fill:#90EE90
style G fill:#90EE90
Legend: Green = New/Fixed | Blue = Modified Breaking ChangesNo breaking changes detected. The component's public API ( Issues FoundNo issues found. All previously reported issues have been addressed:
Logical / Bug AnalysisCopy Button (
No logical errors, race conditions, or bugs detected in the implementation. Tests (
Evidence — Build & Test ResultsTypeScript: Clean — Lint: 0 errors (5 pre-existing warnings in unrelated files — 4 CopyButton Tests — 8/8 passed: Full Test Suite — 2,066/2,066 passed: Build — Successful: Issue LinkageThis PR closes #523 — "Dashboard Human Review Feedback
No other human review feedback exists on this PR. Suggestions
VerdictVERDICT: APPROVED All previously reported issues are resolved. The implementation is correct, well-tested, follows existing patterns ( Automated code review · 2026-07-16 17:55 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review (re-review): All previous issues resolved. TypeScript clean, lint clean, all 2,066 tests pass, build succeeds. VERDICT: APPROVED.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.gg/qaFM2uYFb |
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Went through the timer paths — a re-click resets the window properly and the unmount cleanup does what it says on the tin. Tests are a nice touch, and Same note as elsewhere: needs a CHANGELOG.md entry. That rule is stuck in CLAUDE.md instead of CONTRIBUTING.md so it's our fault it isn't discoverable — add a line and I'm happy. |
Per review: the CHANGELOG-per-PR rule in CLAUDE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the careful timer-path walkthrough! CHANGELOG entry added ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/components/copy-button.test.tsx`:
- Around line 103-127: Update the “does not update state or warn after
unmounting mid-window” test to spy on clearTimeout and assert it is called
during unmount, replacing the ineffective console.error assertion. Remove the
console.error spy and preserve the existing timer advancement and CopyButton
interaction flow.
In `@app/components/copy-button.tsx`:
- Line 28: Update the revertTimerRef useRef declaration to include undefined in
its generic type, preserving the current undefined initialization and timeout
return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 164b567b-1355-4f92-893d-9067c3af6903
📒 Files selected for processing (3)
CHANGELOG.md__tests__/components/copy-button.test.tsxapp/components/copy-button.tsx
|
|
||
| export function CopyButton({ text, className }: CopyButtonProps) { | ||
| const [copied, setCopied] = useState(false); | ||
| const revertTimerRef = useRef<ReturnType<typeof setTimeout>>(undefined); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix TypeScript error on useRef initialization.
undefined is not assignable to ReturnType<typeof setTimeout>. You must explicitly include | undefined in the generic type parameter to avoid a compilation error.
🛠️ Proposed fix
- const revertTimerRef = useRef<ReturnType<typeof setTimeout>>(undefined);
+ const revertTimerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const revertTimerRef = useRef<ReturnType<typeof setTimeout>>(undefined); | |
| const revertTimerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/copy-button.tsx` at line 28, Update the revertTimerRef useRef
declaration to include undefined in its generic type, preserving the current
undefined initialization and timeout return type.
|
Both CodeRabbit comments evaluated against the code rather than taken on faith: Comment 1 (vacuous unmount test): correct — confirmed empirically and fixed in Comment 2 (useRef TS error): refuted. Full gates re-run on the new head: tsc clean, |
Per review: the CHANGELOG-per-PR rule in CLAUDE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d17dc3d to
da72a9e
Compare
|
Rebased onto current Cause: No source changes in the rebase. Re-verified on the new head: |
The revert-to-copy-icon setTimeout was fired on every click with no stored id and no cleanup, causing two bugs: - Rapid re-click: clicking again before the 2s window elapsed left the stale timer from the first click running, so it could flip the checkmark back to the copy icon early, contradicting the just-shown feedback from the second click. - Unmount mid-window: if the component unmounted (e.g. sessions table re-paginating/filtering) before the timer fired, setCopied ran on an unmounted component, logging a React warning. Store the timer id in a ref, clearTimeout it before re-arming on each copy, and clear it in a useEffect cleanup on unmount. Also add the missing type="button" the issue called out, so the button can't act as an implicit form submit. Closes FailproofAI#523 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per review: the CHANGELOG-per-PR rule in CLAUDE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The prior assertion (errorSpy not called) is vacuous under React 18+, which dropped the state-update-on-unmounted-component warning: it passed even with the useEffect cleanup removed. Replace it with a load-bearing vi.getTimerCount() check — the armed 2s revert timer must be pending mid-window (1) and cleared to 0 by the unmount cleanup. The errorSpy assertion is kept as a secondary belt-and-suspenders check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
da72a9e to
7c04e77
Compare
|
Rebased onto current Re-verified on the new head: (Disclosure: this rebase and verification were done with AI assistance.) |
3 participants
What
Fixes both defects reported in #523 for
CopyButton(app/components/copy-button.tsx), where the "copied ✓" revert timer was fired with no stored id and no cleanup:Rapid re-click — clicking Copy again before the previous 2s window elapsed left the stale timer from the first click still armed, so it could flip the checkmark back to the copy icon early (0.5s ahead of schedule in the issue's example), contradicting the feedback from the just-completed second click.
Fixed by storing the timer id in a
useRefandclearTimeout-ing it before arming a new one on every copy, so only the latest click's timer controls the revert.Unmount mid-window — if the component unmounted (e.g. the sessions table re-paginating/filtering a row away) before the 2s elapsed,
setCopiedstill ran, producing a React "state update on an unmounted component" warning.Fixed by clearing the same ref's timer in a
useEffectcleanup (return () => clearTimeout(...), empty deps) that runs on unmount.This follows the exact idiom already used elsewhere in
app/components/for ref-tracked, re-armable timers (e.g.raw-log-viewer.tsx'shighlightTimerRef), rather than introducing a new pattern.Bonus (also called out in the issue): added the missing
type="button"to the<button>so it can't act as an implicit form submit.What's unchanged
text,className) — no new props, no new dependency.Tests
__tests__/components/copy-button.test.tsxalready existed with component-test infrastructure (Vitest + Testing Library + fake timers), so I extended it rather than inventing new infra:console.errorwas never called (no state-update-on-unmounted-component warning).All existing tests in the file continue to pass unmodified.
Gates (CONTRIBUTING.md)
bun run lint— 0 errors (5 pre-existing warnings in unrelated files:<img>/no-img-elementand one unused eslint-disable, none touched by this change).bunx tsc --noEmit— clean, no output.bun run test:run— 2065 passed, 1 pre-existing failure in__tests__/hooks/integrations.test.ts(writeHookEntries adds a packages-array entry to a fresh settings.json) unrelated to this change — that test asserts the literal substring"failproofai"appears in a path derived from the checkout directory name, and fails identically on unmodifiedmainin this same checkout (verified viagit stash).bun run build— compiles successfully, TypeScript passes, all routes generate.Closes #523
Prepared with AI assistance (Claude Sonnet 5), human-reviewed before submission.
Summary by CodeRabbit