Fix(webapp) prevent incidents url being called every frame #2956
Conversation
|
WalkthroughThis PR modifies the dependency array of a Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 |
matt-aitken
deleted the
fix(webapp)-prevent-incidents-url-being-called-every-frame
branch
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fetcher.load("/resources/incidents"); | ||
| } | ||
| }, [fetcher]); | ||
| }, []); |
There was a problem hiding this comment.
🟡 Stale closure causes fetcher.state check to always read 'idle'
By removing fetcher from the useCallback dependency array, the fetchIncidents callback captures the fetcher object from the initial render. Since Remix's useFetcher returns a new object on each render, the captured fetcher.state will always be "idle" (the initial state), making the guard if (fetcher.state === "idle") ineffective.
Click to expand
How the bug is triggered
The callback is created once with an empty dependency array:
const fetchIncidents = useCallback(() => {
if (fetcher.state === "idle") { // Always sees "idle" from first render
fetcher.load("/resources/incidents");
}
}, []); // fetcher not in depsWhen the interval fires every 60 seconds, fetcher.state in the closure always reads "idle" regardless of whether a request is currently in progress.
Actual vs Expected
- Expected: Skip calling
fetcher.load()if a request is already in progress - Actual: The guard is bypassed because
fetcher.statealways reads as"idle", sofetcher.load()is called even if a request is pending
Impact
This could cause unnecessary duplicate network requests if the previous request hasn't completed within 60 seconds. In practice, the impact is limited because:
- The 60-second interval makes overlapping requests unlikely
fetcher.loadcancels previous pending requests
A proper fix would use a ref to track the fetcher state or use fetcher.state directly outside the callback.
Recommendation: Use a ref to track loading state, or check fetcher.state outside the callback and pass it as a parameter. Alternative: Since fetcher.load is stable and cancels previous requests, the guard could be removed entirely if duplicate requests are acceptable.
Was this helpful? React with 👍 or 👎 to provide feedback.
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
Removes the fetcher from the dependency array