fix(web): make the finished-processing refresh effect actually fire#1188
fix(web): make the finished-processing refresh effect actually fire#1188abhay-codes07 wants to merge 1 commit into
Conversation
The effect in useProcessingDocuments that refetches document lists when a document finishes processing depended on [processingMap.keys, queryClient.refetchQueries]. Both are prototype methods - Map.prototype.keys is the same function object for every Map instance - so the dependencies never changed, the effect ran exactly once on mount (when both the previous and current id sets are empty), and the delayed documents-with-memories / dashboard-recents refetch never happened. The eslint-disable on the dependency array hid the lint error that would have caught this. Depend on processingMap itself, and memoize docs on data so the map's identity only changes when the poll payload changes (React Query's structural sharing keeps data referentially stable between identical polls). Without that, the map would get a new identity every render and each re-run's cleanup could cancel the pending 1s/4s refresh timers before they fire.
![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed — found 1 issue(s). I reviewed the change to apps/web/hooks/use-processing-documents.ts, which is intended to make the finished-processing refresh effect re-run when the processing map changes. The new dependency fixes the previous inert dependency, but it also makes the effect cleanup cancel pending delayed refreshes on subsequent processing-list changes.
Findings
apps/web/hooks/use-processing-documents.ts
- The effect cleanup can cancel the delayed refresh timers for a document that just finished when
processingMapchanges again before those timers fire.
Verdict
| // `processingMap` (not `processingMap.keys` — that's the shared | ||
| // Map.prototype method, identical for every map, so the effect would | ||
| // never re-run and finished docs would never trigger a refresh). | ||
| }, [processingMap, queryClient]) |
There was a problem hiding this comment.
This cleanup now runs every time processingMap changes, so a later poll can cancel the delayed refetches that were scheduled for a document that just finished. For example, if document A disappears and the hook schedules the 1s/4s refreshes, then another upload or sync changes the processing list before those timers fire, React runs this cleanup and clears both timers. Consider tracking pending timer IDs in a ref and clearing them only on unmount, or otherwise avoid canceling finished-document refreshes on unrelated processingMap changes.
TestingThe testing subagent verified the Commands run: node /tmp/test-processing-documents-effect.mjs
bunx biome ci apps/web/hooks/use-processing-documents.ts
cd apps/web && bunx tsc --noEmit --pretty false --skipLibCheck
cd apps/web
bun run dev:app -- --hostname 0.0.0.0Result: Verdict✅ Passed. The tested behavior works in the focused harness and in the running app surface; the TypeScript audit reported only unrelated existing diagnostics. Attached Images and Videos 🎥 View recording: pr-1188-memories-grid-transition-early.webm |
What
The
useProcessingDocumentseffect that's supposed to refetchdocuments-with-memoriesanddashboard-recentswhen a document finishes processing never fires. Its dependency array isprocessingMap.keys(uncalled) isMap.prototype.keys— the same function object for every Map instance — andqueryClient.refetchQueriesis likewise a stable method reference. Both deps are constant across renders, so the effect runs exactly once on mount, when the previous and current id sets are both empty. The prev/current diff that detects "these docs just finished" never re-executes, and the delayed refresh the comment describes is dead code. Theeslint-disable-next-line react-hooks/exhaustive-depson the array is what kept the lint rule from flagging it.Effect for users: after adding a document, the memories grid and dashboard recents don't pick up the extracted memories when processing completes — the card stays stale until something else happens to refetch.
How
Two changes, both needed:
[processingMap, queryClient].docsis memoized ondatasoprocessingMaponly changes identity when the poll payload actually changes. React Query's structural sharing keepsdatareferentially stable between identical polls, so this holds. Without this second change the map would get a fresh identity every render, and each effect re-run's cleanup could cancel the pending 1s/4s refresh timers before they fire — trading "never refreshes" for "sometimes loses the refresh".With both in place: a doc disappearing from the processing poll → new
data→ newprocessingMap→ effect re-runs → diff catches the finished id → delayed refetches fire. Renders caused by anything else don't re-run the effect, so the timers survive.Both stray
eslint-disablecomments are removed — the dependency arrays are now honest.Testing
biome checkclean; notscerrors in the touched file (the web app has pre-existing errors elsewhere andignoreBuildErrorsin its Next config — none involve this file).new Map().keys === new Map().keys→true), and the structural-sharing guarantee that makes the fix safe is React Query's documented default. Happy to add a hook test if you'd like a@testing-library/reactsetup introduced for it.cc @MaheshtheDev
Session Details
(aside)to your comment to have me ignore it.