Add reusable AnimatedCounter component for stats#2803
Open
pranayukey200 wants to merge 1 commit into
Open
Conversation
|
@pranayukey200 is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughA new ChangesAnimated Counter Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (3)
Frontend/src/components/AnimatedCounter.jsx (3)
47-54: ⚡ Quick winAdd ARIA live region for accessibility.
The animated counter displays dynamic content that changes visually, but screen readers won't announce these updates. Add
aria-live="polite"to inform assistive technology users when the count reaches its final value.♿ Add ARIA attributes
return ( - <div ref={ref} className="p-4"> + <div ref={ref} className="p-4" aria-live="polite" aria-atomic="true"> <div className="text-4xl font-extrabold mb-1 text-white tabular-nums"> {prefix}{display}{suffix} </div>🤖 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 `@Frontend/src/components/AnimatedCounter.jsx` around lines 47 - 54, The AnimatedCounter component's dynamic numeric output isn't announced to screen readers; update the return JSX to add an ARIA live region on the element that renders the changing value (the div with className "text-4xl font-extrabold mb-1 text-white tabular-nums") by adding aria-live="polite" so assistive tech will announce updates; ensure you modify the AnimatedCounter component's render/return (the div containing {prefix}{display}{suffix}) to include the attribute and keep existing refs/props intact.
48-53: 💤 Low valueConsider moving padding to parent for better composability.
The component applies
p-4padding internally, which may conflict with parent layout expectations. Typically, spacing is controlled by the parent container rather than the leaf component, improving reusability across different layouts.♻️ Remove internal padding
- return ( - <div ref={ref} className="p-4"> + return ( + <div ref={ref}> <div className="text-4xl font-extrabold mb-1 text-white tabular-nums">Then update the parent in LandingPage.jsx to add padding if needed.
🤖 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 `@Frontend/src/components/AnimatedCounter.jsx` around lines 48 - 53, AnimatedCounter.jsx currently applies internal padding via the root div's className="p-4", which reduces composability; remove the "p-4" from the root div (keep the ref and other classes intact) so the component is padding-agnostic, then add the needed padding on the parent(s) that render <AnimatedCounter /> (e.g., update LandingPage.jsx containers) to preserve layout where required.
8-24: ⚡ Quick winOptimize observer effect to avoid recreation.
When
triggeredchanges fromfalsetotrue, the effect re-runs and creates a new observer that will never trigger again (due to the!triggeredguard). Consider restructuring to prevent unnecessary observer recreation.♻️ Prevent observer recreation after trigger
useEffect(() => { const el = ref.current; - if (!el) return; + if (!el || triggered) return; const observer = new IntersectionObserver( ([entry]) => { - if (entry.isIntersecting && !triggered) { + if (entry.isIntersecting) { setTriggered(true); } }, { threshold: 0.5 } ); observer.observe(el); return () => observer.disconnect(); - }, [triggered]); + }, []);🤖 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 `@Frontend/src/components/AnimatedCounter.jsx` around lines 8 - 24, The effect recreates the IntersectionObserver when `triggered` changes causing a new observer that never fires; update the useEffect containing `IntersectionObserver` (the block referencing `ref.current`, `observer.observe`, and `observer.disconnect`) to run only once (remove `triggered` from the dependency array) and either: 1) have the observer callback call `setTriggered(true)` and then disconnect the observer, or 2) use a mutable ref (e.g., `triggeredRef.current`) checked/updated inside the callback so the observer can remain and stop observing once the animation is triggered; ensure you still call `observer.disconnect()`/`observer.unobserve(el)` when triggered or on cleanup.
🤖 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 `@Frontend/src/components/AnimatedCounter.jsx`:
- Around line 26-45: The effect's animation uses requestAnimationFrame inside
useEffect (step callback) but never stores or cancels the frame ID, so the
animation can continue after unmount or dependency changes and call setDisplay
on an unmounted component; fix by capturing the returned frame id from
requestAnimationFrame (e.g., frameId = requestAnimationFrame(step)) and return a
cleanup function from useEffect that calls cancelAnimationFrame(frameId) (and
also cancel any pending frame after each requestAnimationFrame call), ensuring
the step/requestAnimationFrame loop is stopped when triggered/target/isWord
change or the component unmounts.
- Line 31: AnimatedCounter.jsx currently does const to = parseFloat(target)
which can produce NaN and render "NaN" if target is non-numeric; update the
AnimatedCounter component to validate target before using it (e.g., attempt
const candidate = parseFloat(target) then if Number.isNaN(candidate) handle
gracefully by logging/warning and using a safe fallback like 0 or early-return),
ensuring you update any downstream code that uses to (the variable assigned from
parseFloat(target)) to rely on the validated value.
- Around line 3-4: The AnimatedCounter component initializes display to target
when isWord is true but never updates it on prop changes because the animated
effect returns early for isWord; add a useEffect in AnimatedCounter that watches
[isWord, target] and, when isWord is true, calls setDisplay(target) so the
displayed word updates whenever the target prop changes (refer to the display
state and the early-returning animation effect).
---
Nitpick comments:
In `@Frontend/src/components/AnimatedCounter.jsx`:
- Around line 47-54: The AnimatedCounter component's dynamic numeric output
isn't announced to screen readers; update the return JSX to add an ARIA live
region on the element that renders the changing value (the div with className
"text-4xl font-extrabold mb-1 text-white tabular-nums") by adding
aria-live="polite" so assistive tech will announce updates; ensure you modify
the AnimatedCounter component's render/return (the div containing
{prefix}{display}{suffix}) to include the attribute and keep existing refs/props
intact.
- Around line 48-53: AnimatedCounter.jsx currently applies internal padding via
the root div's className="p-4", which reduces composability; remove the "p-4"
from the root div (keep the ref and other classes intact) so the component is
padding-agnostic, then add the needed padding on the parent(s) that render
<AnimatedCounter /> (e.g., update LandingPage.jsx containers) to preserve layout
where required.
- Around line 8-24: The effect recreates the IntersectionObserver when
`triggered` changes causing a new observer that never fires; update the
useEffect containing `IntersectionObserver` (the block referencing
`ref.current`, `observer.observe`, and `observer.disconnect`) to run only once
(remove `triggered` from the dependency array) and either: 1) have the observer
callback call `setTriggered(true)` and then disconnect the observer, or 2) use a
mutable ref (e.g., `triggeredRef.current`) checked/updated inside the callback
so the observer can remain and stop observing once the animation is triggered;
ensure you still call `observer.disconnect()`/`observer.unobserve(el)` when
triggered or on cleanup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdedbe2d-f22e-4565-8cc4-bc2f23fe976f
📒 Files selected for processing (2)
Frontend/src/components/AnimatedCounter.jsxFrontend/src/pages/LandingPage.jsx
Comment on lines
+3
to
+4
| export default function AnimatedCounter({ target, suffix = '', prefix = '', label, isWord = false }) { | ||
| const [display, setDisplay] = useState(isWord ? target : '0'); |
There was a problem hiding this comment.
isWord mode doesn't react to target prop changes.
When isWord is true, display is initialized to target but won't update if the target prop changes after mount. The animation effect (lines 26-45) returns early when isWord is true, so no update mechanism exists.
🔄 Add effect to sync display with target in word mode
const [display, setDisplay] = useState(isWord ? target : '0');
const [triggered, setTriggered] = useState(false);
const ref = useRef(null);
+
+ useEffect(() => {
+ if (isWord) {
+ setDisplay(target);
+ }
+ }, [isWord, target]);🤖 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 `@Frontend/src/components/AnimatedCounter.jsx` around lines 3 - 4, The
AnimatedCounter component initializes display to target when isWord is true but
never updates it on prop changes because the animated effect returns early for
isWord; add a useEffect in AnimatedCounter that watches [isWord, target] and,
when isWord is true, calls setDisplay(target) so the displayed word updates
whenever the target prop changes (refer to the display state and the
early-returning animation effect).
Comment on lines
+26
to
+45
| useEffect(() => { | ||
| if (!triggered || isWord) return; | ||
|
|
||
| const duration = 1500; | ||
| const start = performance.now(); | ||
| const to = parseFloat(target); | ||
|
|
||
| const step = (now) => { | ||
| const progress = Math.min((now - start) / duration, 1); | ||
| const eased = 1 - Math.pow(1 - progress, 3); | ||
| const current = Math.round(to * eased); | ||
| setDisplay(String(current)); | ||
|
|
||
| if (progress < 1) { | ||
| requestAnimationFrame(step); | ||
| } | ||
| }; | ||
|
|
||
| requestAnimationFrame(step); | ||
| }, [triggered, target, isWord]); |
There was a problem hiding this comment.
Cancel animation frame on cleanup to prevent memory leak.
The animation effect uses requestAnimationFrame without storing or cancelling the frame ID. If the component unmounts during animation or dependencies change, the animation continues running and updating unmounted state, causing memory leaks and console warnings.
🐛 Store and cancel animation frame in cleanup
useEffect(() => {
if (!triggered || isWord) return;
const duration = 1500;
const start = performance.now();
const to = parseFloat(target);
+ let rafId;
const step = (now) => {
const progress = Math.min((now - start) / duration, 1);
const eased = 1 - Math.pow(1 - progress, 3);
const current = Math.round(to * eased);
setDisplay(String(current));
if (progress < 1) {
- requestAnimationFrame(step);
+ rafId = requestAnimationFrame(step);
}
};
- requestAnimationFrame(step);
+ rafId = requestAnimationFrame(step);
+
+ return () => {
+ if (rafId) cancelAnimationFrame(rafId);
+ };
}, [triggered, target, isWord]);🤖 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 `@Frontend/src/components/AnimatedCounter.jsx` around lines 26 - 45, The
effect's animation uses requestAnimationFrame inside useEffect (step callback)
but never stores or cancels the frame ID, so the animation can continue after
unmount or dependency changes and call setDisplay on an unmounted component; fix
by capturing the returned frame id from requestAnimationFrame (e.g., frameId =
requestAnimationFrame(step)) and return a cleanup function from useEffect that
calls cancelAnimationFrame(frameId) (and also cancel any pending frame after
each requestAnimationFrame call), ensuring the step/requestAnimationFrame loop
is stopped when triggered/target/isWord change or the component unmounts.
|
|
||
| const duration = 1500; | ||
| const start = performance.now(); | ||
| const to = parseFloat(target); |
There was a problem hiding this comment.
Validate target before parsing to prevent NaN display.
parseFloat(target) will return NaN if target is not a valid numeric string (e.g., if accidentally passed "Zero" without isWord=true), causing the display to show "NaN" to users.
🛡️ Add validation for numeric target
const duration = 1500;
const start = performance.now();
const to = parseFloat(target);
+
+ if (isNaN(to)) {
+ console.error(`AnimatedCounter: invalid numeric target "${target}"`);
+ setDisplay(target);
+ return;
+ }
const step = (now) => {📝 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.
Suggested change
| const to = parseFloat(target); | |
| const duration = 1500; | |
| const start = performance.now(); | |
| const to = parseFloat(target); | |
| if (isNaN(to)) { | |
| console.error(`AnimatedCounter: invalid numeric target "${target}"`); | |
| setDisplay(target); | |
| return; | |
| } | |
| const step = (now) => { |
🤖 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 `@Frontend/src/components/AnimatedCounter.jsx` at line 31, AnimatedCounter.jsx
currently does const to = parseFloat(target) which can produce NaN and render
"NaN" if target is non-numeric; update the AnimatedCounter component to validate
target before using it (e.g., attempt const candidate = parseFloat(target) then
if Number.isNaN(candidate) handle gracefully by logging/warning and using a safe
fallback like 0 or early-return), ensuring you update any downstream code that
uses to (the variable assigned from parseFloat(target)) to rely on the validated
value.
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
1 participant
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.
**Feature: Add reusable AnimatedCounter component for stats
This PR extracts the AnimatedStat component from LandingPage.jsx into a proper, reusable AnimatedCounter component at `src/components/AnimatedCounter.jsx, then updates LandingPage.jsx to use the new component! The stats are still animated via IntersectionObserver, only trigger once when visible, use smooth easing and include all original functionality!
Issue #2802 Comment
Hey there! I've implemented the animated counters feature!
I've:
PR ready for review at: https://github.com/pranayukey200/HELPDESK.AI/pull/new/feature/2802-animated-counters !
Closes #2802!
Summary by CodeRabbit