fix(ssr): add getServerSnapshot to useObservable's useSyncExternalStore

[tyler-reitz]

Fixes #748. v5 is not the default branch, so that keyword will not fire on merge. #748 needs hand-closing.

useObservable called useSyncExternalStore with two arguments. React requires a third, getServerSnapshot, whenever the tree is server rendered or hydrated. Without it React throws Missing getServerSnapshot, which is required for server-rendered content and the surrounding subtree silently falls back to client rendering, which is why a Next.js App Router page using any reactfire hook loses SSR for that subtree.

The part worth reviewing

The issue proposes returning "the same seeded immutableStatus the client snapshot returns". That version is unsafe and this PR deliberately does not do it.

preloadedObservables is a Map on globalThis, keyed only by observableId. In a browser that is one user's cache. On a server it is shared by every concurrent request, so a getServerSnapshot that read observable.immutableStatus would render data request A fetched into request B's HTML whenever both touch the same path.

That leak is unreachable today only because SSR throws before it can happen. So fixing the crash the obvious way would trade a crash for a cross-request data disclosure, which is the worse of the two.

This implementation reads only config for anything data-bearing, and config arrives from the caller on the current render, so it is per-request. With initialData it reports success and that value; without it, loading. (firstValuePromise does read the shared observable, but it is a Promise<void> that resolves without a value, so nothing crosses through it.) The result is held in a ref so the value is stable across renders, which is what React's "The result of getServerSnapshot should be cached" check wants.

Verification

Four tests under a new Server rendering block, and both mutations were run rather than assumed:

• Drop the third argument: all four fail, with React's own Missing getServerSnapshot error. So the tests are load-bearing rather than decorative.
• Return observable.immutableStatus instead (the straightforward implementation): 3 of 4 pass and only the leak test fails. That is the test that earns its place, and it is why the constraint above is written down in the code rather than left to reviewer memory.

tsc passes on both tsconfig.json and tsconfig.test.json; useObservable is 22/22; eslint reports 0 errors on both changed files.

Notes

• The use-sync-external-store/shim is a red herring. Its own implementation ignores getServerSnapshot on purpose, but it resolves to React.useSyncExternalStore whenever React exposes it, so on React 18 and 19 the third argument reaches React's real implementation. Reading only the shim would suggest this fix cannot work.
• No reference docs regeneration, since no exported symbol changed.
• The globalThis cache is still a cross-request hazard for anything that does read it on a server. This PR contains the hazard at the one place it would otherwise become reachable; it does not fix the cache itself. That is separate SSR work.
• One eslint-disable for react-hooks/exhaustive-deps, with the reason in a comment above it: callers routinely pass a fresh config literal each render, and the ref means the value is computed once per component instance anyway.

Scope: this does not make SSR work, and the title should not be read that way

Suspense mode is unchanged and still cannot server-render. With suspense: true the throw at src/useObservable.ts:80 happens before the hook runs, so getServerSnapshot is never reached: a cold cache fails with A component suspended while responding to synchronous input, and only initialData gets you a render.

In non-suspense mode without initialData, a server render produces loading placeholders, not data. That is inherent to the model rather than a gap in it: observableId alone cannot distinguish this request's data from another request's, which is the whole reason this PR refuses to read the cache. Real server-side data needs per-request cache scoping, which is a separate piece of work.

What this PR does is narrower and worth stating plainly: the subtree renders instead of crashing, and it hydrates cleanly.

[armando-navarro]

Thanks Tyler. This is a good fix and the call you flagged is the right one. I reproduced the failure on v5 first, then went after the reasoning rather than taking it on trust. Approving. Everything below is non-blocking.

On the decision you asked about: you were right to refuse what #748 proposed, and it is worth stating more strongly than "unsafe". Take your fix out, so getServerSnapshot returns observable.immutableStatus the way the issue asked, and the server HTML comes back as <div>success:first-request-secret</div>, a value an earlier render had left in the shared cache. One render's data really does reach another render's markup. Yours does not, which is the point.

Two things the code gets right that nothing holds in place

• The initialData branch of getServerSnapshot is uncovered. Change it so it always returns loading, false and undefined, ignoring initialData entirely, and all 22 tests still pass. "reports initialData on the server" never reaches it, because the overlay at src/useObservable.ts:149-164 sets status, data and hasEmitted itself whenever !observable.hasValue && hasData. Same reason it survived your mutation B. The branch matters when the shared cache already holds a value for that id and the caller also passes initialData: the overlay is skipped and your server snapshot is what ships, rendering success:my-own-data, correctly preferring the caller's value over the stored one.
• The natural test is your leak test with a seed added. Do the first render exactly as :432 already does, so the shared cache is holding a value for that id, then server-render the same observableId with initialData, asserting the caller's seed renders and the stored value does not appear. That is the scenario the branch protects, and it doubles as a second cross-request check. I checked that such a test would actually catch a regression rather than just pass: with that branch changed to ignore initialData, the same case renders loading:undefined and the assertion fails.
• The ref is uncovered too, though I am flagging that rather than asking for it. All 22 still pass if the ref is taken out and a fresh object is built on every call, since renderToString calls getServerSnapshot once and only hydration exercises it. A test for it would have to render on the server, hydrate that markup on the client, and watch what React logs. Your file already spies on console.error at :176, so the genuinely new part is the hydrating, but that is still more setup than the rest of the suite needs.

On the ref, the half of your question I nearly skipped

You wrote that the ref keeps the value stable for React's cached-result check. That is right, and it earns its place:

• Take the ref out so a fresh object is built on every call, and hydrating the no-initialData case warns The result of getServerSnapshot should be cached to avoid an infinite loop. Put the ref back and the same run is silent.
• Small correction: React's string names getServerSnapshot, not getSnapshot as your description has it.
• Your exhaustive-deps suppression looks safe. I tried to make a stale value visible and could not: re-rendering one instance with initialData changed renders the new value, because the overlay recomputes from the current config. It is the overlay, not the ref, keeping it fresh.

What I verified

• The failure reproduces on v5 with your exact error, and also under streaming, which is what App Router actually uses. My other checks used renderToString, so I redid these on renderToPipeableStream: without the third argument onShellError fires with the same message and the server emits an empty string, and with your fix the stream completes with zero errors while the case where the shared cache already holds a value still renders loading:undefined, with that value absent from the markup.
• Both your mutations reproduce exactly as you reported them.
• Hydration is clean in three cases with zero React warnings, including one where the server said loading but the client's cache already held a value. Your description undersells this: taking the rendered status and data from config rather than from the cache also makes hydration deterministic, since both sides compute them from the same props.
• Public type surface unchanged: byte-identical .d.ts between v5 and the branch.
• tsc clean on both configs, vite build fine, eslint 0 errors. src/useObservable.ts carries the same 6 warnings as base, so the new source adds none. The test file goes 11 to 16, all of them the existing no-explicit-any rule on the new Subject<any> annotations.
• Your shim note and your Fixes #748 note are both correct, so nothing needed on the latter.

Three smaller notes

• The comment overclaims a little. src/useObservable.ts:115-116 says only config is safe to read, but :132 just below reads firstValuePromise: observable.firstEmission from the shared cache. Nothing leaks through it, since that is a Promise<void>, and it sits inside the boundary you already drew. Worth changing the wording rather than the code, though. I tried removing the value, and because the field is not optional, status.firstValuePromise.then(...) on the server then fails with Cannot read properties of undefined. Neither tsc nor the suite catches that, because the as ObservableStatus<T> cast covers the missing field.
• The protection is React 18 and up. Below that the shim ignores the third argument on the server too, not only on the client: useSyncExternalStore$1 names it and then returns getSnapshot(). On React 17 both forms render identically so nothing regresses, but a clause scoping the comment to 18+ would stop the next reader over-trusting it. I had this backwards at first.
• Suspense SSR is still unfixed, and without initialData server rendering produces only loading placeholders. With suspense on, the throw at :80 happens before the hook, so getServerSnapshot never runs: cold cache throws A component suspended while responding to synchronous input, with initialData it renders, and suspense off renders. Both are the right scope, and the placeholder behavior is the necessary cost of your model rather than a flaw in it, since observableId alone cannot tell your own data from another request's. A line in the description would stop anyone reading the title as "SSR works now".

Approving. If I have misread anything, especially the two coverage gaps, point me at it and I will look again.

[tyler-reitz]

You read all of it right, and the coverage gap was real. Pushed f2882a3 on top of your approval, so here is what changed.

I reproduced the gap before acting on it. Neutering the branch to always return loading / false / undefined leaves all 22 passing, exactly as you said. Your proposed test is in, and I wrote it against the neutered branch first so a pass would mean something: it fails there with loading:undefined, and passes against the real branch. 23 now.

Your diagnosis of why was the useful part. The overlay handles the ordinary case, so the branch only decides anything when the cache already holds a value for the id and the caller passes initialData. I would not have found that from the failing direction.

The comment is fixed on both counts. It now says why firstValuePromise is safe rather than claiming only config is read, and it records your point that the as ObservableStatus<T> cast would hide that field going missing from both tsc and the suite. getServerSnapshot name corrected. I added your React 18+ clause too.

The description now carries the scope caveat, in its own section: suspense mode still cannot server-render, and without initialData a cold cache renders loading placeholders. Stated as inherent to the model rather than a gap, for the reason you gave.

Not done: the ref coverage. You flagged it rather than asked, and I agree with your reasoning about the setup cost, so I have left it. Worth revisiting when something else in the suite needs a hydration harness.

Two things of yours I am carrying forward rather than answering here. The renderToPipeableStream check is the more valuable verification, since App Router is the real target and my own checks were all renderToString. And your point that taking status and data from config makes hydration deterministic is a better argument for this shape than the one I wrote, so I have taken it.

CI has not run on this push. GitHub Actions has been in a major outage since 15:22 UTC and no workflow runs are being created, so the checks you see are stale against 9f26b11. Locally: both typechecks clean, 23/23.

[tyler-reitz]

Your renderToPipeableStream check sent me back to re-run some unrelated SSR work of mine that was verified on renderToString only. It found something that belongs on this PR, because it is downstream of your review and it is about this code.

In suspense mode under streaming, this hook waits for the data and then renders the placeholder anyway.

The sequence: the component suspends at useObservable.ts:80, React holds the boundary open, the observable emits, the thrown firstEmission resolves, React retries. The retry is still a server render, so useSyncExternalStore reads getServerSnapshot, which this PR deliberately keeps away from the shared cache. The value that just arrived is discarded and the boundary resolves to loading:undefined. React's <!--$--> resolved-boundary marker sits right next to it in the markup, so the suspend really did resolve rather than fall back.

Net effect: the caller pays the full latency and gets the placeholder.

This is not an argument against the PR and I am not proposing a change here. Reading that cache is the cross-request leak you reproduced at the top of your review. Refusing to read it is right, and the alternative is the first-request-secret markup you demonstrated. What it means is that per-request cache scoping is the thing that would let the server render real data, rather than a separate correctness cleanup, since with a request-scoped cache getServerSnapshot could safely read the request's own entry.

One other thing from the same run, since it is worse operationally than what we knew: with a cold cache in suspense mode, streaming HANGS rather than erroring. Under renderToString you get A component suspended while responding to synchronous input. Streaming treats suspending as legitimate and holds the response open, and firstEmission only resolves on a first emission, so an observable that never emits never completes the request. A hung request rather than a crash, and no test we have could have seen it.

Both of these sharpen the scope note I added to the description rather than contradicting it. Nothing here needs action from you, and the approved diff is unchanged.

Also worth saying plainly: my SSR checks were all renderToString, and yours were not, which is why you found things I did not. I have written that down as a rule rather than a one-off.