fix: clear setTimeout on unmount in observations/starred/history (#2)#9
Open
mahek395 wants to merge 1 commit into
Open
fix: clear setTimeout on unmount in observations/starred/history (#2)#9mahek395 wants to merge 1 commit into
mahek395 wants to merge 1 commit into
Conversation
…nLake#2) - Replace local timeout variables with useRef - Clear pending timeouts in useEffect cleanup - Prevents state update on unmounted component warning Closes OpenLake#2
|
@mahek395 is attempting to deploy a commit to the OpenLake_Website Team on Vercel. A member of the Team first needs to authorize it. |
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 [savedTables, setSavedTables] = useState([]); | ||
| const [loading, setLoading] = useState(true); | ||
| const timeoutRef = useRef(null); |
There was a problem hiding this comment.
workspace-updated can still leave an orphaned observations timeout.
This fetch path is re-entrant: observation saves dispatch workspace-updated, and a second fetchObservations() can start before the first one finishes. With a single shared ref, the older request can clear the newer timeout in finally, while its own timeout survives cleanup and can still fire later.
Suggested fix
return () => {
window.removeEventListener('workspace-updated', handleUpdate);
- if (timeoutRef.current) clearTimeout(timeoutRef.current);
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = null;
+ }
}; const fetchObservations = async () => {
- timeoutRef.current = setTimeout(() => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = null;
+ }
+
+ const timeoutId = setTimeout(() => {
setLoading(false);
console.warn('Fetch timed out for observations');
}, 8000);
+ 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-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/observations/page.js` at 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).
| 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).
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.
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