Memory Leak in SetInterval#657
Open
annrose2277-glitch wants to merge 1 commit into
Open
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
gssoc:approved
level:intermediate
quality:clean
type:accessibility
labels
Contributor
📝 WalkthroughWalkthroughActivityFeed's GitHub events fetch now validates HTTP response status before consuming the response body. On non-OK responses it clears events and stops loading early. Parsed JSON is normalized to an array, replacing the previous behavior of accepting non-array payloads. ChangesGitHub Events Fetch Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ActivityFeed.tsx (1)
28-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale async GitHub event responses from overwriting state
useEffectcleanup only clears the polling interval; it doesn’t cancel in-flightfetchcalls or guard against older responses. Whenusernamechanges, a slower previous request can still resolve and callsetEvents/setLoadingwith stale data.Suggested fix
useEffect(() => { + const controller = new AbortController(); + const fetchEvents = async () => { try { setLoading(true); const res = await fetch( - `https://api.github.com/users/${username}/events` + `https://api.github.com/users/${username}/events`, + { signal: controller.signal } ); if (!res.ok) { setEvents([]); setLoading(false); return; } const data = await res.json(); + if (controller.signal.aborted) return; setEvents(Array.isArray(data) ? data : []); setLoading(false); } catch (err) { + if ((err as DOMException).name === "AbortError") return; console.error(err); + setEvents([]); setLoading(false); } }; fetchEvents(); const interval = setInterval(fetchEvents, 30000); - return () => clearInterval(interval); + return () => { + clearInterval(interval); + controller.abort(); + }; }, [username]);🤖 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 `@src/components/ActivityFeed.tsx` around lines 28 - 57, The effect's fetchEvents can let slow responses overwrite state when username changes; modify it to create an AbortController for each fetch and pass controller.signal to fetch inside fetchEvents, catch and ignore abort errors, and call controller.abort() in the useEffect cleanup (in addition to clearInterval) so in-flight requests are cancelled when username changes; also optionally capture the current username in a local variable inside the effect (e.g., const current = username) and before calling setEvents/setLoading verify the username still matches to further guard against stale responses.
🤖 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.
Outside diff comments:
In `@src/components/ActivityFeed.tsx`:
- Around line 28-57: The effect's fetchEvents can let slow responses overwrite
state when username changes; modify it to create an AbortController for each
fetch and pass controller.signal to fetch inside fetchEvents, catch and ignore
abort errors, and call controller.abort() in the useEffect cleanup (in addition
to clearInterval) so in-flight requests are cancelled when username changes;
also optionally capture the current username in a local variable inside the
effect (e.g., const current = username) and before calling setEvents/setLoading
verify the username still matches to further guard against stale responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84670e5d-ea6a-488b-90ac-2b8365574265
📒 Files selected for processing (1)
src/components/ActivityFeed.tsx
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.
Related Issue
Description
Resolved a memory leak and potential stale closure bug in
ActivityFeed.tsx. ThesetIntervalfunction was relying onfetchEvents, but the function was missing from theuseEffectdependency array. Refactored the fetch logic so that intervals are properly cleared and recreated without causing multiple API calls to stack up in the background.How Has This Been Tested?
useEffectcleanup function properly destroys the old interval before starting a new one.Type of Change
Summary by CodeRabbit