fix(abs): stop Record Not Found flash; distinguish error from not-found#899
Open
thostetler wants to merge 4 commits into
Open
fix(abs): stop Record Not Found flash; distinguish error from not-found#899thostetler wants to merge 4 commits into
thostetler wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #899 +/- ##
========================================
- Coverage 67.0% 66.8% -0.2%
========================================
Files 346 353 +7
Lines 41042 42130 +1088
Branches 2084 2133 +49
========================================
+ Hits 27489 28104 +615
- Misses 13508 13981 +473
Partials 45 45
🚀 New features to boost your workflow:
|
Abstract pages flashed "Record Not Found" before the real record rendered. The server painted not-found into the HTML on any empty-or-error lookup and the client query then swapped in the record. The not-found branch was also duplicated across all 11 sub-views with no loading state, so any transient !doc rendered RecordNotFound, including non-5xx errors. 1. SSR returns an explicit outcome (found | not-found | error) plus the queryId it seeded the dehydrated cache under, so the client hydrates the same key instead of refetching. 2. Shared useAbsRecordState + AbsRecordBoundary own the loading / not-found / error / content branch. not-found is reserved for a client-confirmed empty result; anything ambiguous shows a skeleton. 3. Migrated all 11 abstract sub-views onto the boundary.
…dening Follow-up hardening on the Record Not Found fix. None of this changes the visible flash behavior; it cuts false negatives at the source, adds the observability to measure them, and trims wasted work. 1. fetchWithRetry retries the SSR lookup on 429/5xx/network so a transient Solr/token blip doesn't emit a false negative. 2. Sentry breadcrumbs for each SSR fetch attempt and the resolved outcome, plus a client message when SSR rendered a negative but the client recovered. 3. Mark not-found/error responses no-store so composeNextGSSP's shared cache header can't edge-cache a negative. 4. Gate each sub-view detail query on doc?.bibcode so it stops firing with an undefined bibcode while the record is unresolved.
thostetler
force-pushed
the
fix/abs-record-not-found-flash
branch
from
335ad52 to
72e8f00
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Risk summary: Medium. This PR changes the SSR contract for all abstract sub-views and centralizes the record state branching; that’s good for consistency, but it increases regression risk around error/rehydration flows and test isolation.
Changes:
- Introduces a shared SSR outcome model (
found | not-found | error) plus a client-side state hook to prevent false “Record Not Found” flashes. - Adds
AbsRecordBoundary+AbsRecordSkeletonand migrates abstract sub-views to render through the boundary. - Adds bounded SSR fetch retries (429/5xx/network) and Sentry breadcrumbs for SSR fetch attempts/outcomes.
Findings (priority order)
high
-
Authenticated state is dropped for SSR error props
- Impact: If SSR returns an error but the client later recovers the record, pages (notably
/abs/[id]/abstract) can incorrectly treat the user as unauthenticated and hide authenticated-only UI (e.g., Add to library). - Location:
src/lib/serverside/absCanonicalization.ts(absErrorProps+ its uses onfetch-not-okandfetch-threwpaths). - Minimal fix: Extend
absErrorPropsto include optionalisAuthenticated, and passisAuthenticated(bootstrapResult.token)in error paths where bootstrap succeeded. - Confidence: high.
- Impact: If SSR returns an error but the client later recovers the record, pages (notably
-
New
fetchWithRetrytest mutates global infra without restoring fetch- Impact: The suite stubs
global.fetchand closes/restarts the shared MSW server; without restoring fetch, this can leak across tests depending on Vitest isolation/worker scheduling and cause flakiness. - Location:
src/lib/serverside/fetchWithRetry.test.ts. - Minimal fix: Don’t close/listen MSW in this suite; restore globals in
afterAll(e.g.,vi.unstubAllGlobals()). - Confidence: high.
- Impact: The suite stubs
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/abs/[id]/abstract.tsx | Switches abstract view to AbsRecordBoundary + useAbsRecordState to avoid not-found flash |
| src/pages/abs/[id]/citations.tsx | Migrates citations sub-view to shared boundary + enables query only when bibcode is present |
| src/pages/abs/[id]/coreads.tsx | Migrates coreads sub-view to shared boundary + enables query only when bibcode is present |
| src/pages/abs/[id]/credits.tsx | Migrates credits sub-view to shared boundary + enables query only when bibcode is present |
| src/pages/abs/[id]/exportcitation/[format].tsx | Migrates export-citation view to boundary (record gating handled centrally) |
| src/pages/abs/[id]/graphics.tsx | Migrates graphics view to boundary; graphics query enabled only when bibcode is present |
| src/pages/abs/[id]/mentions.tsx | Migrates mentions sub-view to shared boundary + enables query only when bibcode is present |
| src/pages/abs/[id]/metrics.tsx | Migrates metrics view to boundary; metrics query enabled only when bibcode is present |
| src/pages/abs/[id]/references.tsx | Migrates references sub-view to shared boundary + enables query only when bibcode is present |
| src/pages/abs/[id]/similar.tsx | Migrates similar sub-view to shared boundary + enables query only when bibcode is present |
| src/pages/abs/[id]/toc.tsx | Migrates TOC sub-view to shared boundary + query enable gating |
| src/lib/abs/absRecordState.ts | Introduces discriminated SSR + UI state model and state derivation |
| src/lib/abs/absRecordState.test.ts | Adds unit tests for deriveAbsRecordState logic |
| src/lib/abs/useAbsRecordState.ts | New hook to unify SSR props + client query hydration/recovery logic |
| src/lib/serverside/absCanonicalization.ts | SSR now returns explicit outcome + queryId, uses retries, sets no-store for negatives, adds Sentry breadcrumbs |
| src/lib/serverside/tests/absCanonicalization.test.ts | Updates/extends SSR tests to assert new outcome behavior and retry semantics |
| src/lib/serverside/fetchWithRetry.ts | Adds bounded server-side fetch retry helper for retryable outcomes |
| src/lib/serverside/fetchWithRetry.test.ts | Adds tests for retry helper (needs isolation fixes) |
| src/components/AbsRecordBoundary/AbsRecordBoundary.tsx | Centralizes loading/not-found/error/content branching across abstract sub-views |
| src/components/AbsRecordBoundary/AbsRecordBoundary.test.tsx | Adds integration-style tests for boundary + hook behavior (mocked query outcomes) |
| src/components/AbsRecordBoundary/AbsRecordSkeleton.tsx | Adds layout-mirroring skeleton to reduce layout shift during loading |
| src/components/AbsRecordBoundary/AbsRecordSkeleton.test.tsx | Adds basic render/accessibility test for the skeleton |
| src/components/AbsRecordBoundary/index.ts | Exports new boundary and skeleton components |
| src/components/Layout/AbsLayout.tsx | Updates layout props typing to include optional children |
Comment on lines
+1
to
+23
| import { afterAll, afterEach, beforeAll, describe, expect, test, vi } from 'vitest'; | ||
|
|
||
| import { fetchWithRetry, isRetryableStatus } from './fetchWithRetry'; | ||
| import { server } from '@/mocks/server'; | ||
|
|
||
| const fetchMock = vi.fn(); | ||
| vi.stubGlobal('fetch', fetchMock as unknown as typeof fetch); | ||
|
|
||
| const res = (status: number): Response => ({ ok: status >= 200 && status < 300, status } as Response); | ||
|
|
||
| beforeAll(() => { | ||
| // disable msw for this suite; we stub fetch manually | ||
| server.close(); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| // restart msw for other suites | ||
| server.listen({ onUnhandledRequest: 'error' }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| fetchMock.mockReset(); | ||
| }); |
Comment on lines
+37
to
+39
| const absErrorProps = (queryId: string, statusCode: number, reason: string): { props: AbsProps } => ({ | ||
| props: { initialDoc: null, queryId, ssr: { outcome: 'error', statusCode, reason } }, | ||
| }); |
| }; | ||
| addOutcomeBreadcrumb({ requestedId, viewPath, outcome: 'error', stage: 'fetch', statusCode: response.status }); | ||
| setNoStore(ctx); | ||
| return absErrorProps(requestedId, response.status, 'fetch-not-ok'); |
| }; | ||
| addOutcomeBreadcrumb({ requestedId, viewPath, outcome: 'error', stage: 'fetch-throw', statusCode: 500 }); | ||
| setNoStore(ctx); | ||
| return absErrorProps(requestedId, 500, 'fetch-threw'); |
Error outcomes dropped isAuthenticated even when bootstrap succeeded, so a client-recovered record on /abs/[id] could hide authenticated-only UI (e.g. Add to library). Thread isAuthenticated through the fetch-not-ok and fetch-threw paths where the token is available.
The suite stubbed global fetch and tore down/restarted the shared MSW server without restoring fetch, risking cross-suite leakage under Vitest worker scheduling. Leave MSW intact and restore fetch via unstubAllGlobals.
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
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.
Abstract pages briefly flashed "Record Not Found" before the real record
rendered — the server painted not-found into the HTML on any empty or errored
SSR lookup, and the client query then swapped in the record. The branch was
duplicated across all 11 sub-views with no loading state, so any transient
missing doc rendered RecordNotFound, and non-5xx errors were mislabeled as
not-found.
seeded the cache under, so the client hydrates the same key.
error / content branch; not-found only for a client-confirmed empty result.
Loading Skeleton:
Error:
Missing Record:
