chore: forward-integrate main into v5 - #796
Merged
Merged
Conversation
The "Extra Experimental concurrent mode features" section claimed these features "will not be stable until sometime after React 18 is released". React 18 shipped in 2022 and concurrent mode was abandoned as a concept rather than stabilised, so the section pointed readers at a setup guide for a React feature that does not exist. All three of its reactjs.org/docs/concurrent-mode-* links were dead pages. Replaces it with a plain "## Suspense" section, adds a Suspense bullet to "What is ReactFire?", and documents that suspense is off by default, which the old text never stated (verified against src/firebaseApp.tsx, suspense ?? false). Also corrects the SuspenseWithPerf description. The old text said it instruments load times with Firebase Performance Monitoring and linked those docs, but src/performance.tsx uses only the browser User Timing API and still carries a "Should this import firebase/performance?" TODO. Caught by Armando in review. One concurrent-mode link remains in a comment in example/index.tsx, left for a separate follow-up. Closes #756.
* ci: add a manual firestore flake probe for #776 Every measurement of the #776 flake so far has been local, where the failure is a plain waitFor timeout with no gRPC error. In CI it arrives alongside a gRPC framing desync (RESOURCE_EXHAUSTED: Received message larger than max), which raises the possibility that the local repro and the CI failure are not the same bug. That matters, because the @grpc/grpc-js override proposed as the fix was measured only against the local one. This runs the firestore suite N times per arm, across both Node versions and both grpc-js versions, under CI conditions, so the comparison happens where the failure actually occurs. Notes on the design: - workflow_dispatch only. It never runs on a push, a PR or a schedule, so it costs nothing until someone asks for it. - A fresh emulator per iteration, matching how npm test runs in CI. Reusing one emulator across iterations would measure something else. - Failures are classified, not counted. Only the #776 assertion signature counts toward the rate; emulator start failures are reported separately, because folding them in previously inflated a local rate estimate by roughly 50%. - The job reports rather than fails. A red run here means the probe broke, not that the flake reproduced. - Inputs reach the script through env rather than interpolation, and iterations is validated before it reaches the loop. Classifier dry-run against synthetic logs covering pass, flake, flake-with-gRPC-error and infra-failure returns the expected counts and excludes infra failures from the rate. zizmor 1.25.2 reports no findings beyond the cache-poisoning rule CI suppresses. Refs #776 * ci: address review feedback on the firestore flake probe Six fixes from Armando's review on #780: - Pull `arm` out of the matrix. Both arms now run sequentially in one job on one runner, so the machine is held constant. That was the control the local measurements had and the workflow dropped, and the premise of the probe is that the machine matters. Arm order is forced baseline-then-override because applying the override mutates node_modules for everything after it. - Note in the job summary that this runs one emulator and one file, while CI runs five emulators and the whole suite in parallel, so a clean table is not a verdict on CI. - Count the flake/RESOURCE_EXHAUSTED overlap. Two independent totals could not answer whether the gRPC desync and the #776 assertion co-occur, which is the question the probe exists for. - Give hangs their own bucket. A test timeout produces no assertion line, so it was landing in `infra` and dropping out of the rate entirely. #776 reports a 120s hang. - Persist counts per arm and render the summary in an `always()` step. A run that hits `timeout-minutes` now still reports the arms that finished, instead of losing every count. Default iterations 20 -> 30, matching #776's power note. - Surface unrecognized failures. The flake match is a literal vitest assertion string, so a reword would have quietly turned every real flake into an infra failure. Unmatched assertion lines are now collected and shown as a warning in the summary. Also drops a `set +e` / `set -e` pair that turned errexit on partway through a script that never had it enabled. Verified: classifier dry-run over synthetic logs covering pass, flake, flake-with-gRPC, hang, infra and a reworded assertion returns the expected counts and routes the reword to the unmatched warning; arms input validation rejects empty, unknown and non-JSON values; the summary renders correctly from a partial counts file, which is the timeout-recovery path; zizmor 1.25.2 reports no findings beyond the suppressed cache-poisoning rule.
…781) * docs(example): drop the concurrent mode framing from the demo entry Follow-up to #778, which removed the same obsolete premise from the README but left this copy of it. The comment told readers they need "an experimental build of React to use Concurrent mode" and linked reactjs.org/docs/concurrent-mode-adoption.html, a dead page. React 18 shipped in 2022 and concurrent mode was abandoned as a concept rather than stabilised, so the instruction could not be followed. The two commented-out react/experimental and react-dom/experimental imports existed only to serve that premise and go with it. Comments only. No active code changes, so the demo behaves identically. This removes the last reactjs.org reference in the repository. Refs #756 * docs(example): note that the Suspense path does not run as checked in Armando installed the example's pinned versions and confirmed the uncomment instruction cannot be followed: react-dom is pinned at 17.0.2, where createRoot is undefined at runtime and absent from the root react-dom types, and withSuspense/Firestore.tsx imports useTransition, which is also undefined on 17. Uncommenting the block alongside the existing ReactDOM.render call would also put two roots on one element. The previous wording named a precondition; the replacement read as a complete two-step procedure, which promised more than the file can deliver. This says what is missing instead. Still comments only.
* ci(flake-probe): add a full-suite workload mode The first run of the probe (2026-08-07, run 31204989659) came back 120/120 clean in firestore-only mode with zero RESOURCE_EXHAUSTED. That configuration runs one emulator and one test file, while the #776 failures come from `npm run test`: five emulators and the whole suite. So the isolated suite does not reproduce either failure and cannot serve as a control for the @grpc/grpc-js comparison. Adds a `workload` input, defaulting to `full-suite`, which runs the same command CI runs. `firestore-only` is kept because it isolates the Firestore client and is roughly 3x faster per iteration. Also: - Install the functions deps in full-suite mode. `test.yaml` does this before `npm run test` and the functions emulator does not start without it. - Raise timeout-minutes to 180. Measured per-iteration cost is ~23s full-suite and ~8s firestore-only, so the default 30 per arm is ~28 minutes; the raise is headroom for the 200 cap, which is ~153 minutes full-suite. - Classify a failing `double check - emulator is running` as infra, ahead of the hang check. auth, firestore and database each open with that health check and it fails by timing out, so without this an emulator that never came up was counted as the #776 120s hang. Found by running the real suite on a machine where the RTDB emulator was unreachable, not by inspection. - Make the summary footer workload-aware, so a firestore-only table carries a warning pointing at the 120/120 result. Verified: one full-suite iteration run locally end to end (8 of 9 test files passing, the failure being the unreachable local RTDB emulator), and that real log fed through the classifier, which is what surfaced the health-check misclassification; regression cases for flake, hang, infra and a log where the health check passes alongside a real #776 assertion all classify unchanged; workload input rejects unknown values; zizmor 1.25.2 clean. Refs #776, #783. * ci(flake-probe): fix the errexit abort and scope counting to firestore Three fixes from Armando's review on #785. 1. Restore the `set +e` / `set -e` pair around `emulators:exec`. I removed it in 43f39e5 on the reasoning that the script never enables errexit itself. That is true and irrelevant: GitHub runs an undeclared `run:` step as `bash -e {0}`, so errexit is on from the invocation and `set -uo pipefail` does not clear it. The pair was load-bearing. Effect on main today: the first failing iteration kills the step before `rc=$?` is read. Nothing is classified, the arm's tally is never written, and probe-counts.tsv is empty or half written, so the summarize step reports nothing. The probe can only ever produce a clean table, and the first run that genuinely reproduces the flake is the one that reports least. Verified under `bash -e` before and after: without the guard not even the first iteration prints; with it, both arms tally with a failing iteration in each. The comment now says why it exists, because the reasoning that removed it was superficially sound. 2. Scope the flake, hang and health-check searches to firestore's output. This one comes from the widening. In firestore-only mode anything in the log was necessarily about #776. In full-suite mode it is not: `expected 'loading' to deeply equal 'success'` is just what vitest prints when a data hook's status assertion fails, and it appears in 6 of the 9 test files. Three separate miscounts followed, all confirmed against logs rather than argued: - a failure in another test file counted as a #776 flake - a timeout in any file counted as the #776 120s hang - a non-firestore emulator's health check outranked a genuine firestore flake in the same run, filing it as infra and dropping it from the rate entirely The FAIL line names the file and the assertion or timeout sits on the next line, verified against #781's real overnight failure, so -A1 is the right window. Captured into a variable rather than piped into `grep -q`, so an early-exit SIGPIPE cannot combine with pipefail and read as a silent no-match. The health check is now firestore-specific, so a non-firestore health failure falls through to the final branch and is recorded in probe-unmatched.txt rather than silently miscounted. That branch also captures the first FAIL line now, not only an assertion, so a genuine failure in another test file is visible too. 3. Raise timeout-minutes 180 -> 240. The 180 was sized on a flat 23s per iteration. Across recent CI runs that step ranges 20 to 31s, and a timeout cares about the slow tail: at 31s the 200 cap wants ~207 minutes. The default 30 per arm is safe under any reading; the ceiling exists for the cap, so it is now set past the pessimistic figure. Comment records the range and the reason. Verified: a 7-log corpus, one real (#781's overnight #776 failure) and six synthetic, classified under both the old and new logic. The three miscounts above are fixed with no regressions, and the real #776 failure still classifies as a flake. YAML parses; zizmor 1.25.2 clean. Refs #776, #783.
Bumps [hono](https://github.com/honojs/hono) from 4.12.27 to 4.13.1. - [Release notes](https://github.com/honojs/hono/releases) - [Commits](honojs/hono@v4.12.27...v4.13.1) --- updated-dependencies: - dependency-name: hono dependency-version: 4.13.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The grpc-js override was measured and does not fix the flake (baseline 5/60 vs override 4/60, p = 1.0), so the default arm list drops to baseline alone. The arm stays available as a dispatch input. Each iteration records the firestore, auth and database health-check durations into probe-iterations.tsv, and the summary compares distributions across outcomes: each arm reported separately, every cell carrying its own count of measured iterations, mean of the two middle values at even n. The timing hypothesis this was built to test did not survive it: the recovered durations show no separation between passing and failing runs (Armando's recovery, #776), and the flake mechanism is a Listen-stream desync answered with a 60-second maximum backoff (#776). The probe's remaining job is proving the long-polling mitigation, which baseline-only dispatch makes a single 30-iteration run. Durations come from vitest's json reporter rather than the log: the default reporter prints a per-test line only over the 300ms slow threshold and for every test in a failing file, so the log drops auth entirely. Extraction is scoped by file path. Refs #776
Bumps [@hono/node-server](https://github.com/honojs/node-server) from 1.19.14 to 1.19.17. - [Release notes](https://github.com/honojs/node-server/releases) - [Commits](honojs/node-server@v1.19.14...v1.19.17) --- updated-dependencies: - dependency-name: "@hono/node-server" dependency-version: 1.19.17 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#791) test/firestore.test.tsx flakes in CI at roughly 18% per run (11 of 60 iterations, 2026-08-11). The Firestore emulator intermittently corrupts a Listen frame, so grpc-js reads four body bytes as a length prefix and reports RESOURCE_EXHAUSTED with an absurd size. The SDK special-cases that code with backoff.resetToMax(), parking the stream on a 60 second maximum backoff. This is an unresolved upstream emulator bug, firebase/firebase-tools#8654. There is no fixed version to pin, so the goal is to survive it rather than prevent it. A reconnect is not what rescues the affected tests. The same failure sends the client to OnlineState.Offline after ONLINE_STATE_TIMEOUT_MS (10s), and an offline client raises the pending snapshot from the local cache, empty cache included. Two tests assert that a document is absent, which is exactly what the empty cache reports, so they reach success about ten seconds after the failure with no server involved. They are the only two in the file whose first snapshot cannot be served from local data. Those two currently abandon the wait after one second, before the fallback can fire, and vitest would kill them at five seconds regardless. Raising both ceilings lets the fallback do its work. waitFor polls every 50ms, so a larger budget costs nothing when the stream is healthy: healthy runs measure 80-95ms (9 samples) and 90-140ms (11 samples) per test with the change, against 82-136ms and 101-120ms without it across 3 each. The bands overlap; the wider upper tail on the larger sample is sampling, not cost. The budget is 120s, far above the ~10s the fallback needs, because a ceiling is not a delay. Each test's own timeout clears the sum of the budgets beneath it, or only the first wait could ever spend one. The third wait in useFirestoreDocOnce keeps the 1000ms default deliberately. It waits on the client's own write, raised from the local cache before the acknowledgement returns; measured at 8ms with the client offline, against a control that fails at 1000ms when no write is issued. Verified: the desync reproduced locally twice during this work, having never been seen off CI before, and was rescued both times. One of the two has a full log, carrying both the RESOURCE_EXHAUSTED line and the maximum backoff line, with the vulnerable test taking 9878ms and passing; the other is a 9872ms sample from a batch that discarded output. CI showed the same rescue on this branch at 9795ms. Note that all three observations are with the fix in place: that the old ceilings would have killed a 9878ms wait follows from testing-library's 1000ms default and vitest's 5000ms, not from an observed failure. A simulated 65 second stall survives the new budget while failing under the default, and the per-test ceilings were confirmed to apply by shortening one until it failed. Both typechecks clean and the firestore suite green against the emulator. Scoped cost: a genuine regression in those two tests now takes up to 120s to surface instead of 1s. Refs #776
The probe was added in #780 as a temporary measurement tool for #776, which is now closed. It has no callers: nothing outside the file referenced it, and it was workflow_dispatch only, so removing it changes no scheduled or PR-triggered behaviour. #783's removal conditions were written when the @grpc/grpc-js override was still the candidate fix. That override was measured and did not work (4/60 for the 1.14.4 override against a 5/60 baseline on 1.9.16, Fisher p = 1.0), so it never landed. The fix was #791's timeout ceilings instead, and the probe verified it: 0/60 flakes against an 11/60 baseline, with the desync rate statistically unchanged. Closes #783
…re (#779) 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. The server snapshot deliberately does not return observable.immutableStatus the way getSnapshot does. preloadedObservables is a globalThis cache keyed only by observableId, so on a server it is shared by every concurrent request; seeding the server snapshot from it would let one request render data another request fetched for the same path. Only config is read here, because it arrives from the caller on this render. Today that leak is unreachable because SSR throws first, so fixing the crash without this constraint would trade a crash for a cross-request data disclosure. That guarantee holds on React 18 and up. On 16 and 17 the shim's server path ignores the third argument and returns getSnapshot(), so the cached value still reaches the markup there, unchanged from before. Adds six tests under a "Server rendering" block, covering both renderToString and the streaming renderer, all mutation verified: - dropping the third argument fails all six with React's own error - returning observable.immutableStatus instead (the straightforward implementation) passes four and fails two: the cross-request leak test and the one preferring the caller's initialData over a cached value The server snapshot literal carries no type assertion, so tsc reports TS2741 if a required ObservableStatus field goes missing. Fixes #748.
# Conflicts: # test/useObservable.test.tsx
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
1 participant
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.
Forward-integrates
mainintov5through #779.Why now
#779 landed on
mainas a v4 bug fix, andchore/remove-startwithvalue(#754 Group 2) removes astartWithValuereference that #779 adds. Rebasing that branch ontov5before this merge lands would merge clean and silently drop the removal, so this has to go first.v5was also 10 commits behind.What it carries
getServerSnapshotforuseObservable, the SSR crash fixwaitFortimeout ceilings that rescue two firestore cache-miss testsThe one conflict, and how it was resolved
test/useObservable.test.tsx, both sides editing the same import line:v5had addedFirebaseAppProvider,initializeAppandbaseConfigfor #735's error-surfacing tests,mainhad addedReactFireOptionsfor #779'sServer renderingblock. Resolved as a union, no test on either side dropped.src/useObservable.tsmerged without conflict, and the result keeps both changes: #735's suspense-only re-throw at the end of the hook, and #779'sgetServerSnapshotabove it.Verification
useObservableis 24/24 on the merged tree, which isv5's tests plus fix(ssr): add getServerSnapshot to useObservable's useSyncExternalStore #779's six.useSyncExternalStoretakes it to 6 failed / 18 passed, so fix(ssr): add getServerSnapshot to useObservable's useSyncExternalStore #779's coverage survived the merge rather than being carried across inert.tsconfig.jsonandtsconfig.test.json, React 18.2.0, version printed in the same command as the run.v5's fix: surface observable errors via status instead of re-throwing #735 behavior is intact: its error-surfacing tests are part of the 24 and pass, and the suspense-only throw is still the merged hook's final branch.One consequence of the combination, worth knowing before it is found in a demo
#735 made
status: 'error'the non-suspense contract onv5. #779's server snapshot reads onlyconfig, so it never reports an error. Together, an already-errored cache entry server-renders as loading. Measured on both trees with the same probe:v5before this mergeMissing getServerSnapshotloading,errorundefinedMissing getServerSnapshotloading,errorundefinedstatus: 'error'status: 'error'So the error is invisible in the server HTML and appears only after hydration. This is not introduced by the merge, it follows from #779 and is already
main's behavior;v5is just the branch where consumers are told to handle errors throughstatus. The alternative is reading the shared cache, which is the cross-request leak #779 exists to avoid, so the real fix is per-request scoping. Noted on #790.Merge instructions
Merge commit, not squash and not rebase, so
mainstays a real ancestor ofv5(same as #758 and #775).