Skip to content

Commit 16a2163

Browse files
committed
fix: Address critical Suspense lifecycle bugs from code review
Fixed two critical bugs identified in senior-level code review: 1. **Error after success bug**: Previously threw errors to Error Boundary even after initial success. Now only throws during initial load. After first success, errors surface as stale data (matches TanStack Query behavior). 2. **Promise lifecycle bug**: When deps changed, could throw old promise from previous collection. Now properly resets promise when collection changes. Implementation: - Track current collection reference to detect changes - Track hasBeenReady state to distinguish initial vs post-success errors - Reset promise and ready state when collection/deps change - Only throw errors during initial load (!hasBeenReadyRef.current) Tests added: - Verify NO re-suspension on live updates after initial load - Verify suspension only on deps change, not on re-renders This aligns with TanStack Query's Suspense semantics: - Block once during initial load - Stream updates after success without re-suspending - Show stale data if errors occur post-success Credit: Fixes identified by external code review 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 50c261b commit 16a2163

File tree

2 files changed

+165
-8
lines changed

2 files changed

+165
-8
lines changed

packages/react-db/src/useLiveSuspenseQuery.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,25 @@ export function useLiveSuspenseQuery(
124124
deps: Array<unknown> = []
125125
) {
126126
const promiseRef = useRef<Promise<void> | null>(null)
127+
const collectionRef = useRef<Collection<any, any, any> | null>(null)
128+
const hasBeenReadyRef = useRef(false)
127129

128130
// Use useLiveQuery to handle collection management and reactivity
129131
const result = useLiveQuery(configOrQueryOrCollection, deps)
130132

133+
// Reset promise and ready state when collection changes (deps changed)
134+
if (collectionRef.current !== result.collection) {
135+
promiseRef.current = null
136+
collectionRef.current = result.collection
137+
hasBeenReadyRef.current = false
138+
}
139+
140+
// Track when we reach ready state
141+
if (result.status === `ready`) {
142+
hasBeenReadyRef.current = true
143+
promiseRef.current = null
144+
}
145+
131146
// SUSPENSE LOGIC: Throw promise or error based on collection status
132147
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
133148
if (!result.isEnabled) {
@@ -137,27 +152,24 @@ export function useLiveSuspenseQuery(
137152
)
138153
}
139154

140-
if (result.status === `error`) {
141-
// Clear promise and throw error to Error Boundary
155+
// Only throw errors during initial load (before first ready)
156+
// After success, errors surface as stale data (matches TanStack Query behavior)
157+
if (result.status === `error` && !hasBeenReadyRef.current) {
142158
promiseRef.current = null
143159
throw new Error(`Collection "${result.collection.id}" failed to load`)
144160
}
145161

146162
if (result.status === `loading` || result.status === `idle`) {
147-
// Create or reuse promise
163+
// Create or reuse promise for current collection
148164
if (!promiseRef.current) {
149165
promiseRef.current = result.collection.preload()
150166
}
151167
// THROW PROMISE - React Suspense catches this (React 18+ compatible)
152168
throw promiseRef.current
153169
}
154170

155-
// Collection is ready - clear promise
156-
if (result.status === `ready`) {
157-
promiseRef.current = null
158-
}
159-
160171
// Return data without status/loading flags (handled by Suspense/ErrorBoundary)
172+
// If error after success, return last known good state (stale data)
161173
return {
162174
state: result.state,
163175
data: result.data,

packages/react-db/tests/useLiveSuspenseQuery.test.tsx

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,4 +403,149 @@ describe(`useLiveSuspenseQuery`, () => {
403403
{ timeout: 1000 }
404404
)
405405
})
406+
407+
it(`should NOT re-suspend on live updates after initial load`, async () => {
408+
const collection = createCollection(
409+
mockSyncCollectionOptions<Person>({
410+
id: `test-persons-suspense-11`,
411+
getKey: (person: Person) => person.id,
412+
initialData: initialPersons,
413+
})
414+
)
415+
416+
let suspenseCount = 0
417+
const SuspenseCounter = ({ children }: { children: ReactNode }) => {
418+
return (
419+
<Suspense
420+
fallback={
421+
<div>
422+
{(() => {
423+
suspenseCount++
424+
return `Loading...`
425+
})()}
426+
</div>
427+
}
428+
>
429+
{children}
430+
</Suspense>
431+
)
432+
}
433+
434+
const { result } = renderHook(
435+
() => useLiveSuspenseQuery((q) => q.from({ persons: collection })),
436+
{
437+
wrapper: SuspenseCounter,
438+
}
439+
)
440+
441+
// Wait for initial load
442+
await waitFor(() => {
443+
expect(result.current.data).toHaveLength(3)
444+
})
445+
446+
const initialSuspenseCount = suspenseCount
447+
448+
// Make multiple live updates
449+
collection.insert({
450+
id: `4`,
451+
name: `New Person 1`,
452+
age: 40,
453+
454+
isActive: true,
455+
team: `team1`,
456+
})
457+
458+
await waitFor(() => {
459+
expect(result.current.data).toHaveLength(4)
460+
})
461+
462+
collection.insert({
463+
id: `5`,
464+
name: `New Person 2`,
465+
age: 45,
466+
467+
isActive: true,
468+
team: `team2`,
469+
})
470+
471+
await waitFor(() => {
472+
expect(result.current.data).toHaveLength(5)
473+
})
474+
475+
collection.delete(`4`)
476+
477+
await waitFor(() => {
478+
expect(result.current.data).toHaveLength(4)
479+
})
480+
481+
// Verify suspense count hasn't increased (no re-suspension)
482+
expect(suspenseCount).toBe(initialSuspenseCount)
483+
})
484+
485+
it(`should only suspend on deps change, not on every re-render`, async () => {
486+
const collection = createCollection(
487+
mockSyncCollectionOptions<Person>({
488+
id: `test-persons-suspense-12`,
489+
getKey: (person: Person) => person.id,
490+
initialData: initialPersons,
491+
})
492+
)
493+
494+
let suspenseCount = 0
495+
const SuspenseCounter = ({ children }: { children: ReactNode }) => {
496+
return (
497+
<Suspense
498+
fallback={
499+
<div>
500+
{(() => {
501+
suspenseCount++
502+
return `Loading...`
503+
})()}
504+
</div>
505+
}
506+
>
507+
{children}
508+
</Suspense>
509+
)
510+
}
511+
512+
const { result, rerender } = renderHook(
513+
({ minAge }) =>
514+
useLiveSuspenseQuery(
515+
(q) =>
516+
q
517+
.from({ persons: collection })
518+
.where(({ persons }) => gt(persons.age, minAge)),
519+
[minAge]
520+
),
521+
{
522+
wrapper: SuspenseCounter,
523+
initialProps: { minAge: 20 },
524+
}
525+
)
526+
527+
// Wait for initial load
528+
await waitFor(() => {
529+
expect(result.current.data).toHaveLength(3)
530+
})
531+
532+
const suspenseCountAfterInitial = suspenseCount
533+
534+
// Re-render with SAME deps - should NOT suspend
535+
rerender({ minAge: 20 })
536+
rerender({ minAge: 20 })
537+
rerender({ minAge: 20 })
538+
539+
expect(suspenseCount).toBe(suspenseCountAfterInitial)
540+
541+
// Change deps - SHOULD suspend
542+
rerender({ minAge: 30 })
543+
544+
await waitFor(() => {
545+
expect(result.current.data).toHaveLength(1)
546+
})
547+
548+
// Verify suspension happened exactly once more
549+
expect(suspenseCount).toBe(suspenseCountAfterInitial + 1)
550+
})
406551
})

0 commit comments

Comments
 (0)