fix: clear CopyButton timer on re-click and unmount#532
Conversation
Signed-off-by: Abhishek Hiremath <131762197+Abhi-DevHub@users.noreply.github.com>
📝 WalkthroughWalkthrough
ChangesCopy feedback lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes 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 failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/copy-button.tsx (1)
38-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent double execution of the fallback copy method.
If
navigator.clipboard?.writeTextis unavailable,fallbackCopyText(text)is executed in theelse ifbranch on line 42. If it fails (returnsfalseor throws), the execution jumps to thecatchblock on line 46, wherefallbackCopyText(text)is unnecessarily executed a second time on line 49. This double execution results in redundant DOM mutations.Refactor the control flow to attempt the modern clipboard API first, handle its rejection, and then fall back to
fallbackCopyTextexactly once if needed.♻️ Proposed refactor
- const handleCopy = useCallback(async () => { - try { - if (navigator.clipboard?.writeText) { - await navigator.clipboard.writeText(text); - } else if (!fallbackCopyText(text)) { - throw new Error("Both clipboard methods failed"); - } - showCopied(); - } catch (err) { - console.error("Failed to copy to clipboard:", err); - try { - if (fallbackCopyText(text)) { - showCopied(); - } - } catch { - // Both methods failed — do nothing - } - } - }, [text, showCopied]); + const handleCopy = useCallback(async () => { + if (navigator.clipboard?.writeText) { + try { + await navigator.clipboard.writeText(text); + showCopied(); + return; + } catch (err) { + console.error("Failed to copy to clipboard:", err); + } + } + + try { + if (fallbackCopyText(text)) { + showCopied(); + } + } catch { + // Both methods failed — do nothing + } + }, [text, showCopied]);🤖 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` around lines 38 - 56, Refactor handleCopy so navigator.clipboard.writeText is attempted first and fallbackCopyText(text) is invoked exactly once only when the modern API is unavailable or rejects. Preserve showCopied for either successful method and retain the existing failure logging without retrying fallbackCopyText after it has already been attempted.
🤖 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 `@app/components/copy-button.tsx`:
- Line 28: Update the timeoutRef declaration in the copy-button component to
provide an explicit initial value, using an undefined-compatible ref type and
initializing it with undefined to satisfy React 19 typings.
---
Outside diff comments:
In `@app/components/copy-button.tsx`:
- Around line 38-56: Refactor handleCopy so navigator.clipboard.writeText is
attempted first and fallbackCopyText(text) is invoked exactly once only when the
modern API is unavailable or rejects. Preserve showCopied for either successful
method and retain the existing failure logging without retrying fallbackCopyText
after it has already been attempted.
🪄 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: 75d0077a-2270-4266-817f-9e917a07dec1
📒 Files selected for processing (1)
app/components/copy-button.tsx
|
|
||
| export function CopyButton({ text, className }: CopyButtonProps) { | ||
| const [copied, setCopied] = useState(false); | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout>>(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the React version in the project.
cat package.json | grep '"react"' || echo "No react dependency found in package.json"Repository: FailproofAI/failproofai
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json react deps =="
sed -n '1,220p' package.json | grep -nE '"react"|"`@types/react`"|"`@types/react-dom`"|typescript' || true
echo
echo "== copy-button.tsx around timeoutRef =="
nl -ba app/components/copy-button.tsx | sed -n '1,120p'
echo
echo "== lockfile refs (if present) =="
for f in package-lock.json pnpm-lock.yaml yarn.lock; do
if [ -f "$f" ]; then
echo "-- $f --"
grep -nE '(`@types/react`|react@|react-dom@)' "$f" | head -n 20 || true
fi
doneRepository: FailproofAI/failproofai
Length of output: 401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/components/copy-button.tsx =="
sed -n '1,120p' app/components/copy-button.tsx
echo
echo "== react typings in package metadata =="
python3 - <<'PY'
import json
with open('package.json') as f:
pkg = json.load(f)
for k in ('dependencies','devDependencies','peerDependencies'):
d = pkg.get(k, {})
for name in ('react','react-dom','`@types/react`','`@types/react-dom`','typescript'):
if name in d:
print(f'{k}.{name} = {d[name]}')
PYRepository: FailproofAI/failproofai
Length of output: 2441
🌐 Web query:
@types/react 19 useRef no argument overload removed initial value required
💡 Result:
In React 19 and the corresponding @types/react package, the useRef hook has been updated to require an explicit argument, removing the overload that allowed calling useRef() without any initial value [1][2][3]. Key details regarding this change: Reasoning: This change was made to simplify the type signature of useRef, aligning it with other hooks like useState and createContext [1][4][3]. It removes ambiguity and ensures consistent behavior [1][5]. Migration: If you previously used useRef() without arguments, you should now pass undefined as the initial value: useRef(undefined) [1][5][4]. Ref Object Behavior: - All refs returned by useRef are now mutable by default [1][5][3]. - The distinction between MutableRefObject and RefObject has been simplified; useRef now consistently returns a RefObject that is mutable [1][5][6]. - Convenience overloads exist for useRef(null) and useRef(undefined) to ensure proper typing for the current property [1][5][4]. Automated Fixes: The React team provides a codemod as part of the react-19 preset (or specifically refobject-defaults / useRef-required-initial) to automatically convert existing useRef() calls to useRef(undefined) [1][5][4]. You can run this using tools such as npx types-react-codemod@latest preset-19 ./path-to-your-react-ts-files [3].
Citations:
- 1: https://react.dev/blog/2024/04/25/react-19-upgrade-guide
- 2: https://github.com/facebook/react/blob/ee0855f427832e899767f7659c5289364218ab9e/CHANGELOG.md
- 3: https://github.com/react/react/releases/tag/v19.0.0
- 4: [react] Require initial value for
useRefDefinitelyTyped/DefinitelyTyped#64920 - 5: [react] Types for React 19 DefinitelyTyped/DefinitelyTyped#69022
- 6: https://claritydev.net/blog/typescript-typing-react-useref-hook
Pass an initial value to useRef. React 19’s typings require one here, so use 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 timeoutRef declaration
in the copy-button component to provide an explicit initial value, using an
undefined-compatible ref type and initializing it with undefined to satisfy
React 19 typings.
|
Automated code review started - full review. Results will be posted here. |
|
⏳ Build & verify complete. Type-check and test evidence gathered. |
|
|
||
| export function CopyButton({ text, className }: CopyButtonProps) { | ||
| const [copied, setCopied] = useState(false); | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout>>(); |
There was a problem hiding this comment.
🔴 CRITICAL: React 19 useRef requires an initial value
Error: Type error: Expected 1 arguments, but got 0.
With @types/react 19.2.17 and strict: true, React 19 removed the zero-argument overload of useRef. The current code useRef<ReturnType<typeof setTimeout>>() fails the TypeScript check and blocks next build.
Fix:
const timeoutRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);Note: Since this ref is used with clearTimeout() (which accepts undefined), the | undefined type is correct and matches the ref cleanup pattern used on line 30.
|
|
||
| return ( | ||
| <button | ||
| <button type="button" |
There was a problem hiding this comment.
💡 Suggestion: Formatting consistency
The type="button" attribute was placed on the same line as the opening tag, creating an inconsistent line break compared to how all other attributes are on their own lines:
<button type="button"
onClick={handleCopy}Consider moving it to its own line for consistency with the other attributes:
<button
type="button"
onClick={handleCopy}Non-blocking — purely stylistic.
🔍 Automated Code ReviewExecutive SummaryThis PR fixes a CopyButton timer bug (#523) where rapid re-clicks caused stale "Copied" state and unmounting mid-timer caused React state-update warnings. The approach — tracking the timer via Change ArchitectureBreaking ChangesNo breaking changes detected. Pure bug-fix on a single component. No API, schema, config, or dependency changes. Issues Found
Logical / Bug AnalysisTimer management is correct:
Test coverage: 6 tests all pass. Missing edge case: no test for rapid-re-click scenario (click twice quickly). EvidenceBuild (FAILS): Tests (ALL PASS): Issue LinkageCloses #523. Changes correctly address the described bug. Human Review FeedbackNo human review comments. Previous CodeRabbit bot comment flagged the same Suggestions
VerdictVERDICT: CHANGES_REQUESTED Automated code review · 2026-07-16 12:30:00 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review found 1 critical issue (useRef type error blocks build) + 1 suggestion. See summary and inline comments. 🔴
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 |
|
CI's red on this one — Fix itself is the right shape, but #528 covers the same ground with tests and is already green, so I'll probably go with that one. Your #533 is solid though — thanks for both. |
3 participants
Closes #523
What this PR does:
Why:
Rapid re-clicks caused the early revert because the first timer was not cleared.
Unmounting mid-timer caused a React state-update-on-unmounted-component warning.
Summary by CodeRabbit