Keep the last good panel data when a refresh fails - #53
Conversation
usePolled documents that "a failed refresh keeps the last good data on screen", but it could only honour that for transport failures. An upstream declining arrives as a *successful* 200 carrying `available: false`, so setData() overwrote the last good payload with it and PanelBody fell straight through to PanelEmpty. The result was that one failed poll blanked a panel until the server's TTL lapsed — up to 60s for the calendar, 30s for requests and activity — and memoize would go on serving that cached failure for the rest of the TTL even after the upstream recovered. PanelBody now remembers the most recent available payload and keeps rendering it when a later response is unavailable, above a marker that names the reason and carries the same hint PanelEmpty would. Staleness is stated rather than hidden: showing stale numbers silently would be worse than showing none. Left memoize alone deliberately. It is generic over T and knows nothing about Result, and caching failures is what stops a dead upstream being hit every five seconds — the display decision belongs on the client, where Result is already understood. Verified end to end against a live Seerr behind a toggleable proxy: good data renders, breaking the upstream keeps the rows and adds "Showing last known data — HTTP 503", and healing it clears the marker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughPanel rendering now retains the latest available result and displays it with a “Showing last known data” warning when a subsequent response is unavailable, while preserving empty-state behavior when no prior result exists. ChangesPanel stale-data rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dashboard/web/src/components/Panel.tsx`:
- Around line 118-121: Update useLastAvailable so the lastAvailable ref is
assigned only inside a useLayoutEffect dependent on data, recording data when it
is available after the render commits. Remove the render-time assignment while
preserving the existing T | null return behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24e4ee1c-9279-467d-8433-a4f5b97bec6b
📒 Files selected for processing (1)
dashboard/web/src/components/Panel.tsx
| function useLastAvailable<T extends object>(data: Result<T> | null): T | null { | ||
| const lastAvailable = useRef<T | null>(null); | ||
| if (data?.available) lastAvailable.current = data; | ||
| return lastAvailable.current; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant component and hook usage.
fd -a 'Panel\.tsx$|.*' | sed 's#^\./##' | rg 'dashboard/web/src/components/Panel\.tsx|src/components/Panel\.tsx' || true
if [ -f dashboard/web/src/components/Panel.tsx ]; then
echo '--- outline ---'
ast-grep outline dashboard/web/src/components/Panel.tsx || true
echo '--- relevant lines ---'
sed -n '1,180p' dashboard/web/src/components/Panel.tsx | nl -ba
fi
# Search for Result type and usages of useLastAvailable to understand scope/types.
echo '--- useLastAvailable usages ---'
rg -n "useLastAvailable|Result<|interface Result|type Result" dashboard/web/src -S || true
# If not in specified path, search repo.
echo '--- repo-wide usages ---'
rg -n "useLastAvailable|interface Result|type Result" -S || trueRepository: joshdev8/AutoPlexx
Length of output: 867
🌐 Web query:
React concurrent rendering abandoned render may leave refs mutated useEffect layout effect documentation
💡 Result:
In React's concurrent rendering architecture, the distinction between the render phase and the commit phase is fundamental to maintaining application stability. Render Phase (Interruptible) The render phase, where React calls your component functions to build the new Fiber tree, is explicitly designed to be pure and interruptible [1][2][3]. React may invoke your component function multiple times, pause work to handle higher-priority updates, or completely abandon a render cycle if the work is deemed no longer relevant [4][5][3]. Because of this, it is critical that rendering logic remains free of side effects [5][6][3]. This includes: - Avoiding direct DOM mutations. - Avoiding writing to mutable refs during the render execution [5]. - Avoiding triggering asynchronous operations or other external side effects [5][3]. If you mutate a ref directly in the render body, that mutation could be persisted even if React discards the current rendering work, leading to inconsistent state or "phantom" effects [5]. Commit Phase (Uninterruptible) Once the render phase is complete and React has determined the final UI state, it moves to the commit phase [1][3]. This phase is synchronous and uninterruptible [1][3]. It is only during this stage that React applies changes to the host environment (the DOM) [1][3]. Because useLayoutEffect and useEffect are scheduled during the commit phase, they are guaranteed to run only after the render is finalized and committed [1][7]. - Refs are attached and updated as part of the commit phase (typically in the mutation or layout stage) [1]. - useLayoutEffect runs synchronously after DOM mutations but before the browser paints [8][1]. - useEffect runs asynchronously after the browser has painted [1][9][3]. By isolating side effects, ref updates, and DOM interactions within these effect hooks, you ensure that these operations only occur when React is certain that the component's output will indeed be reflected in the user interface [5][7]. Attempts to perform these actions during rendering can lead to bugs where effects observe inconsistent trees or perform redundant, incorrect operations [10][4].
Citations:
- 1: https://deepwiki.com/facebook/react/2.1-fiber-work-loop-and-scheduling
- 2: https://deepwiki.com/react/react/2.3-render-phase:-beginwork-and-completework
- 3: https://medium.com/@avnishtomar/render-phase-vs-commit-phase-the-two-step-brain-of-react-a0d265ab2dcd
- 4: https://github.com/icyJoseph/react-tearing-study
- 5: https://dev.to/paulshen/will-this-work-in-react-concurrent-mode-43d0
- 6: https://react.dev/learn/render-and-commit
- 7: https://blog.codeminer42.com/everything-you-need-to-know-about-concurrent-react-with-a-little-bit-of-suspense/
- 8: https://react.dev/reference/react/useLayoutEffect
- 9: https://react.dev/learn/synchronizing-with-effects
- 10: Check for store mutations before commit react/react#22290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Panel.tsx imports and hook/body section ---'
sed -n '1,170p' dashboard/web/src/components/Panel.tsx
echo '--- useLastAvailable usages ---'
rg -n "useLastAvailable|type Result|interface Result" dashboard/web/src -S || true
echo '--- repository-wide useLastAvailable/type Result usages ---'
rg -n "useLastAvailable|type Result|interface Result" -S || trueRepository: joshdev8/AutoPlexx
Length of output: 6149
Track last available payloads after commit, not during render.
Assigning lastAvailable.current while the component function runs can leave the ref pointing to data that was produced during a concurrent render cycle that React later discarded. Move this assignment into useLayoutEffect(..., [data]) so it only records available payloads that are part of the committed render.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/web/src/components/Panel.tsx` around lines 118 - 121, Update
useLastAvailable so the lastAvailable ref is assigned only inside a
useLayoutEffect dependent on data, recording data when it is available after the
render commits. Remove the render-time assignment while preserving the existing
T | null return behavior.
1 participant
Follows up the
memoizefinding from the #51 review. Investigating it turned up something larger than the finding described.The bug
usePolled's docstring promises "a failed refresh keeps the last good data on screen." It can only honour that for transport failures. An upstream declining arrives as a successful200carryingavailable: false, sosetData(json)replaced the last good payload with it andPanelBodyfell through toPanelEmpty.Probing the real
memoizeshows the server side compounding it:Step 4 is the part the review didn't mention: the cached failure is served for the whole TTL even after the upstream is healthy. Combined, a single failed poll blanked a panel for up to 60s (Upcoming) or 30s (Requests, Activity) — the exact outcome the invariant exists to prevent.
The fix
PanelBodyremembers the most recentavailablepayload and keeps rendering it when a later response is unavailable, above a marker naming the reason and carrying the same hintPanelEmptywould.Staleness is stated, not hidden — silently showing stale numbers would be worse than showing none, and the hint keeps the fix discoverable without the panel going blank.
Why not fix
memoizeThe review proposed making
memoizeretain the last good value. I deliberately didn't:Tand knows nothing aboutResult; teaching it the discriminant couples the cache to the payload type.The display decision belongs where
Resultis already understood.Verification
End to end against a live Seerr behind a toggleable proxy, without reloading between steps (a reload would reset the retained value and prove nothing):
Showing last known data — HTTP 503Also confirmed the first-load case is untouched: with no prior good payload, an unavailable response still renders
PanelEmpty.Typecheck, lint, 42/42 tests and build all clean.
Not covered
GaugesandVpnCarddon't usePanelBodyand still drop upstream reasons — the other half of that review finding. Left for a separate change.🤖 Generated with Claude Code
Summary by CodeRabbit