fix: clear setTimeout on unmount in observations/starred/history (#2)#9
fix: clear setTimeout on unmount in observations/starred/history (#2)#9mahek395 wants to merge 1 commit into
Conversation
|
@mahek395 is attempting to deploy a commit to the OpenLake_Website Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 1 hour. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR fixes a memory leak across three pages by refactoring timeout handling to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Poem
🚥 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/history/page.js`:
- Line 15: The shared timeoutRef causes races where an older fetchHistory()
finally clears a newer timer; instead, when you create a timer inside
fetchHistory (or its caller), capture the returned id in a local variable (e.g.,
const myTimer = setTimeout(...)) and in that fetchHistory invocation’s finally
(or its own cleanup) call clearTimeout(myTimer) rather than clearing
timeoutRef.current; still keep timeoutRef.current for unmount/global
cancellation by setting timeoutRef.current = myTimer when creating the timer and
clearing timeoutRef.current in the component cleanup, but always clear the
locally captured myTimer inside the same invocation so each invocation only
clears its own timer (references: timeoutRef, fetchHistory, clearTimeout,
useRef).
In `@app/observations/page.js`:
- Line 16: The shared timeoutRef causes races between concurrent
fetchObservations runs: an older fetch's finally can clear the newer timeout or
leave its own timeout orphaned. Modify fetchObservations (and related useEffect
handlers for the "workspace-updated" event) to track timeouts per invocation
instead of a single shared timeoutRef—for example generate a unique fetchId (or
create a localTimeoutRef inside fetchObservations) and store the timer id in a
Map/Set keyed by that id, clear only the matching timer in that fetch's finally,
and on cleanup/unmount iterate the Map/Set to clear any remaining timers; ensure
the "workspace-updated" handler uses the same per-fetch mechanism so it cancels
the correct timeout(s).
In `@app/starred/page.js`:
- Line 15: fetchStarred is re-entrant and timeoutRef.current can be overwritten
by a subsequent call causing the first call’s finally to clear the wrong timer;
modify the timeout handling so each fetch captures its own timer id and only
clears it if it still matches the global timeoutRef: when you call setTimeout
store the returned id in a local variable (e.g., localTimeoutId) and assign
timeoutRef.current = localTimeoutId, and in fetchStarred’s finally
clearTimeout(localTimeoutId) only if timeoutRef.current === localTimeoutId; also
ensure you still clear any previous timer before assigning a new one so no
timers leak (update code around timeoutRef, setTimeout, and the finally block in
fetchStarred accordingly).
🪄 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: 6ceb0dd0-73d0-4e90-9f04-4abcf33a5608
📒 Files selected for processing (3)
app/history/page.jsapp/observations/page.jsapp/starred/page.js
| const { user, loading: authLoading } = useAuth(); | ||
| const [historyItems, setHistoryItems] = useState([]); | ||
| const [loading, setLoading] = useState(true); | ||
| const timeoutRef = useRef(null); |
There was a problem hiding this comment.
Overlapping history refreshes can cancel the wrong timeout.
The shared ref fixes only the most recent timer. If fetchHistory() is triggered again before the previous call settles, the older finally clears the new timer and its own timer can outlive unmount. The timer should be cleared by the invocation that created it, not by whatever id is currently in the ref.
Suggested fix
return () => {
window.removeEventListener('workspace-updated', handleUpdate);
- if (timeoutRef.current) clearTimeout(timeoutRef.current);
-};;
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = null;
+ }
+}; const fetchHistory = async () => {
- timeoutRef.current = setTimeout(() => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = null;
+ }
+
+ const timeoutId = setTimeout(() => {
setLoading(false);
console.warn('Fetch timed out (4s)');
}, 4000);
+ timeoutRef.current = timeoutId;
try {
// existing fetch logic
} finally {
- clearTimeout(timeoutRef.current);
+ clearTimeout(timeoutId);
+ if (timeoutRef.current === timeoutId) {
+ timeoutRef.current = null;
+ }
setLoading(false);
}
};Also applies to: 27-30, 33-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/history/page.js` at line 15, The shared timeoutRef causes races where an
older fetchHistory() finally clears a newer timer; instead, when you create a
timer inside fetchHistory (or its caller), capture the returned id in a local
variable (e.g., const myTimer = setTimeout(...)) and in that fetchHistory
invocation’s finally (or its own cleanup) call clearTimeout(myTimer) rather than
clearing timeoutRef.current; still keep timeoutRef.current for unmount/global
cancellation by setting timeoutRef.current = myTimer when creating the timer and
clearing timeoutRef.current in the component cleanup, but always clear the
locally captured myTimer inside the same invocation so each invocation only
clears its own timer (references: timeoutRef, fetchHistory, clearTimeout,
useRef).
| const { user, loading: authLoading } = useAuth(); | ||
| const [starredItems, setStarredItems] = useState([]); | ||
| const [loading, setLoading] = useState(true); | ||
| const timeoutRef = useRef(null); |
There was a problem hiding this comment.
timeoutRef.current can point at the wrong request.
fetchStarred() is re-entrant via workspace-updated. Once a second call overwrites the ref, the first call’s finally clears the newer timer and its own timer becomes untracked, so cleanup only cancels the latest timeout. That reopens the post-unmount callback path this PR is trying to close.
Suggested fix
return () => {
window.removeEventListener('workspace-updated', handleUpdate);
- if (timeoutRef.current) clearTimeout(timeoutRef.current);
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = null;
+ }
}; const fetchStarred = async () => {
- timeoutRef.current = setTimeout(() => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = null;
+ }
+
+ const timeoutId = setTimeout(() => {
setLoading(false);
console.warn('Fetch timed out (4s)');
}, 4000);
+ timeoutRef.current = timeoutId;
try {
// existing fetch logic
} finally {
- clearTimeout(timeoutRef.current);
+ clearTimeout(timeoutId);
+ if (timeoutRef.current === timeoutId) {
+ timeoutRef.current = null;
+ }
setLoading(false);
}
};Also applies to: 27-30, 33-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/starred/page.js` at line 15, fetchStarred is re-entrant and
timeoutRef.current can be overwritten by a subsequent call causing the first
call’s finally to clear the wrong timer; modify the timeout handling so each
fetch captures its own timer id and only clears it if it still matches the
global timeoutRef: when you call setTimeout store the returned id in a local
variable (e.g., localTimeoutId) and assign timeoutRef.current = localTimeoutId,
and in fetchStarred’s finally clearTimeout(localTimeoutId) only if
timeoutRef.current === localTimeoutId; also ensure you still clear any previous
timer before assigning a new one so no timers leak (update code around
timeoutRef, setTimeout, and the finally block in fetchStarred accordingly).
mahek395
force-pushed
the
fix/memory-leak-settimeout-unmount
branch
from
44405a7 to
8448d14
Compare
2 participants
Summary
Fixes memory leak: setTimeout not cleared on component unmount (closes #2).
Changes
app/observations/page.js— replaced local timeout with useRef, cleared in cleanupapp/starred/page.js— same fixapp/history/page.js— same fixWhat was wrong
The safety timeouts in fetchObservations(), fetchStarred(), and fetchHistory()
were local variables not cleared on unmount. If a user navigated away before
fetch completed, the callback fired on an unmounted component causing React
state update warnings.
Testing
Screenshots
N/A — bug fix with no visual changes
Summary by CodeRabbit