You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#761 (fixes #760) added the search-result → Combined view logic to PageViewerOrFallback, and along the way fixed a regression where stepping between search results imposed Combined on a non-paged document and 404'd. The root cause was that PageViewerOrFallback is not remounted when the /viewer/:uri route param changes (<Route path="/viewer/:uri" component={PageViewerOrFallback} /> in App.js), so its per-document state outlives the document and goes stale across navigation.
That fix is correct and verified, but it papers over the structural issue rather than removing it. This issue captures the cleanup so it doesn't bloat the bugfix PR.
The smell
Because the component instance persists across document navigations, per-document state has to be defended individually:
A reset effect clears decidedForUri / searchMatchesInPages on uri change — this is the textbook "reset state when a prop changes" anti-pattern the React docs say to solve with key.
The cached page-count response is tagged with its uri and treated as "unknown" on mismatch, because a plain setResponse(null) reset raced with the decision effect in the same effect-flush.
Two different mechanisms (reset-on-change vs tag-and-compare) for one underlying fact: the component lives longer than the document it represents.
Proposed change
Remount per document. Give the route a key={uri} (e.g. render={({ match }) => <PageViewerOrFallback key={match.params.uri} />}). This resets all per-document state for free, which lets us delete the reset effect and drop the uri-tag (back to plain response?.pageCount). Smaller, and removes the whole class of stale-across-navigation bugs.
(Optional, further) Collapse the fetch → probe → decide effect dance into a small useReducer / explicit state machine. The view decision currently coordinates two async inputs (page count + page-match probe) plus view across several effects nudging each other; a reducer would make the states (loading → probing → decided) explicit and reduce effect reliance.
Verify before merging the refactor
Blast radius of remount: the footer, sidebar, and nav hooks (useWorkspaceNavigation, useSearchHighlightStepper, usePageFind) would re-initialise; rotation/scale would reset per document (arguably more correct, but a behaviour change).
Stepping UX: the authors may have avoided a key deliberately so next/prev stepping feels smooth rather than full-reloading. Worth confirming it doesn't regress — though note the current fix already renders blank-then-content during the transition (totalPages === null → return null), so the visual transition is likely already equivalent to a remount.
Non-goals
No behaviour change intended — this is a pure refactor. The search → Combined behaviour and the next/prev fix from Take search results to the Combined view when the match is there #761 must be preserved (re-run the same browser check: paged result → Combined; next result on a non-paged doc → its own view, no Combined imposed, no runtime error; previous → back to Combined).
Context
#761 (fixes #760) added the search-result → Combined view logic to
PageViewerOrFallback, and along the way fixed a regression where stepping between search results imposed Combined on a non-paged document and 404'd. The root cause was thatPageViewerOrFallbackis not remounted when the/viewer/:uriroute param changes (<Route path="/viewer/:uri" component={PageViewerOrFallback} />inApp.js), so its per-document state outlives the document and goes stale across navigation.That fix is correct and verified, but it papers over the structural issue rather than removing it. This issue captures the cleanup so it doesn't bloat the bugfix PR.
The smell
Because the component instance persists across document navigations, per-document state has to be defended individually:
decidedForUri/searchMatchesInPagesonurichange — this is the textbook "reset state when a prop changes" anti-pattern the React docs say to solve withkey.responseis tagged with itsuriand treated as "unknown" on mismatch, because a plainsetResponse(null)reset raced with the decision effect in the same effect-flush.Two different mechanisms (reset-on-change vs tag-and-compare) for one underlying fact: the component lives longer than the document it represents.
Proposed change
Remount per document. Give the route a
key={uri}(e.g.render={({ match }) => <PageViewerOrFallback key={match.params.uri} />}). This resets all per-document state for free, which lets us delete the reset effect and drop the uri-tag (back to plainresponse?.pageCount). Smaller, and removes the whole class of stale-across-navigation bugs.(Optional, further) Collapse the fetch → probe → decide effect dance into a small
useReducer/ explicit state machine. The view decision currently coordinates two async inputs (page count + page-match probe) plusviewacross several effects nudging each other; a reducer would make the states (loading → probing → decided) explicit and reduce effect reliance.Verify before merging the refactor
useWorkspaceNavigation,useSearchHighlightStepper,usePageFind) would re-initialise;rotation/scalewould reset per document (arguably more correct, but a behaviour change).keydeliberately so next/prev stepping feels smooth rather than full-reloading. Worth confirming it doesn't regress — though note the current fix already renders blank-then-content during the transition (totalPages === null → return null), so the visual transition is likely already equivalent to a remount.Non-goals
Refs #760, #761.