fix(js): report dropped publishes and wire mismatches

[maxi-maxima]

What this changes

• makes WingfoilClient.publish() return whether a frame reached an open WebSocket
• propagates that boolean through the Solid, Svelte, and Vue publisher helpers
• reports a Hello wire-version mismatch once per connection without rejecting compatible peers
• documents the best-effort/no-buffering contract and adds focused client regressions

Why

The client previously dropped publishes silently during wasm startup and reconnect windows, while subscription replay made the API look more durable than it is. It also stored the server's Hello version without comparing it to the local wire version, so incompatibilities surfaced later as opaque decode noise.

Closes #836.

How it was verified

• cd js && pnpm run lint
• cd js && pnpm test - 40 passed, including 5 new client tests
• git diff --check

The clean checkout does not contain generated js/src/wasm, and this Windows host does not have wasm-pack. The TypeScript and Vitest runs therefore used a gitignored module-shape stub; the client tests mock the codec functions they exercise. Upstream CI remains the proof for the real WASM build.

Notes for the reviewer

Publishes are deliberately not queued: replaying stale UI or order events after reconnect is domain-unsafe. A version mismatch is likewise informational rather than fatal because adjacent wire versions can remain compatible.

[0-jake-0]

Reviewed. This is in scope, respects both compatibility boundaries #836 set (boolean not buffering; surface the mismatch without rejecting), and I found no correctness bugs. Comments below are documentation and follow-up sized, not blockers.

Verification — the gap you flagged is now closed

You couldn't run against a real wasm build, and neither did CI (see finding 1), so I ran it here:

• pnpm install --frozen-lockfile, then a locally stubbed js/src/wasm/ (gitignored) standing in for the wasm-pack output — the same shape tests/codec.test.ts already mocks. The client tests mock every codec function they touch, so the stub only has to satisfy module resolution.
• pnpm run lint — clean.
• pnpm test — 40 passed, matching your report.
• Mutation check: with the new tests in place I reverted js/src/index.ts to the base commit (932107e). 4 of the 5 new tests fail, so they are real regressions rather than tautologies. The fifth ("does not report matching versions") passes vacuously against base, which is the expected shape for a negative test.

1. CI did not run the JS suite on this PR

The checks on this PR are clippy, Lint (fmt & clippy), Test (wingfoil), cargo audit, dependency review and pnpm audit. No web-integration run — and that is the only workflow that runs pnpm test.

web-integration.yml triggers on push, workflow_call and workflow_dispatch, with no pull_request; its only caller (integration-tests.yml) is itself workflow_call-only. Because this PR comes from a fork, no push event reaches this repo, so a JS-only change merges on a Rust-only signal.

Not caused by this PR — pre-existing, and worth its own issue — but it is why the local run above matters, and it means the js/tests/ gate that landed with the "the vitest suite ran nowhere until now" comment still doesn't cover fork PRs.

2. The retry advice conflates two different falses

README.md ("Publishing is best effort") says callers "should check the return value and apply their own domain-specific retry policy". But false covers two cases that want opposite handling:

• transient — wasm booting, socket not open. Retry is right.
• permanent — JSON cannot carry the value (the case that also console.warns). Retry spins forever.

One sentence separating them makes the contract actionable, e.g. noting that only the permanent failure warns, so a retry policy should bound itself rather than loop. The same distinction is worth a line in the publish doc comment (js/src/index.ts:317-320).

3. The quick-start snippet models the pattern the new section warns against

README.md:39 is const sent = client.publish("ui", { kind: "click", note: "hi" }); — assigned and never used, which is exactly the discard the section below it argues against. Something like:

if (!client.publish("ui", { kind: "click", note: "hi" })) {
  // dropped — see "Publishing is best effort"
}

teaches the contract in the place most readers stop.

4. LatencyTracker is the one first-party consumer still blind

js/src/tracing.ts:164 and :205 call publish and discard the result. send() increments and returns this.seq unconditionally, so a dropped probe consumes a sequence number and produces an onResponse that simply never fires — the same silent-drop class #836 is about. Solid, Svelte and Vue got the boolean; tracing didn't.

Changing send()'s number return is more than this PR should take on, but a doc line on send() saying the publish may be dropped (and that the returned seq is therefore not a promise of a round trip) would close the loop cheaply. tests/tracing.test.ts:34's FakeClient.publish also still returns void — harmless today because it goes through as unknown as WingfoilClient, and because tsconfig.json only includes src/** so tests aren't type-checked at all.

5. Test fixture duplication

tests/client.test.ts's wasm vi.mock factory and FakeSocket are near-copies of tests/codec.test.ts:6-18 and its FakeSocket. The vi.mock call has to stay per-file because of hoisting, but the factory object and the socket double could live in a shared tests/fixtures.ts — the new FakeSocket is a strict superset of codec's, so consolidation is one-directional. Cheaper to do now, at two copies, than at four.

6. Two small precision points

• "once per connection" (PR body and commit message) is really once per Hello frame — nothing dedupes, and the test sends a single Hello. True in practice since the server sends one, just don't lean on the stronger claim.
• console.error for a version-1 server is what #836 asked for, so no objection. Worth noting the tension it creates: the README.md paragraph directly below says a version-1 server "stays compatible", and that supported configuration now prints a red error on every connect. If it grates in practice, warn for the known-compatible direction (server older than client) and error otherwise would keep the signal without crying wolf. Apps that want to react programmatically already can — ConnectionState.open carries version and wireVersion() is exported — which might be worth a README pointer.

---

Generated by Claude Code

[maxi-maxima]

Thanks for the thorough review. I addressed the publish-contract precision in 323498f: the quick start now checks the boolean, retries are documented as bounded with transient vs unencodable cases called out, LatencyTracker.send() says that a sequence number is not a round-trip promise, and the mismatch test no longer claims stronger deduplication than the code provides. TypeScript typecheck and all 40 Vitest tests pass.

I kept fixture extraction out of this behavioral PR. I also filed #877 for the fork-PR JavaScript CI gap so that the workflow trigger can be reviewed independently.

[0-jake-0]

Re-reviewed at 323498f. All four actionable points from the last pass are addressed, and this time I ran it against a real wasm build rather than a stub, which closes the verification gap both of us had.

Verification

• wasm-pack v0.13.1 + rustup target add wasm32-unknown-unknown, then pnpm run build:wasm — genuine js/src/wasm/, not a module-shape stub
• pnpm run lint — clean
• pnpm test — 40 passed across 5 files (client 5, codec 8, burst 5, reconnect 7, tracing 15)

So the 40-test claim in your PR body holds on the real codec, not just the stub. Combined with the mutation check from the previous pass (4 of 5 new tests fail against 932107e), the new coverage is doing real work.

The delta

• README.md:39 now branches on the boolean instead of assigning and discarding — the quick start teaches the contract at the place most readers stop (finding 3 ✅)
• "Publishing is best effort" now separates the transient false (booting, reconnecting) from the permanent one (JSON cannot carry the value), and says retries should be bounded (finding 2 ✅)
• The publish doc comment carries the same distinction rather than deferring to the README (finding 2 ✅)
• LatencyTracker.send() states that the returned seq is not a promise of a round trip (finding 4 ✅)
• The mismatch test no longer claims deduplication the code doesn't do (finding 6a ✅)

Remaining, none blocking

• The README still says the client "logs one explicit error for that connection". It is really once per Hello frame — true in practice, since the server sends one per connection. Fine as written; just don't build a stronger claim on it later.
• Finding 5 (extracting the vi.mock factory and FakeSocket into tests/fixtures.ts) deliberately deferred — agreed, it doesn't belong in a behavioural PR. Worth an issue so the third copy doesn't land before the second gets consolidated.
• Finding 6b (warn when the server is older than the client, error otherwise) not taken. It was offered as optional and the current behaviour is what #836 asked for, so no objection.

Before merge

Lint (fmt & clippy) on this PR is cancelled (started 02:30, cancelled 03:01) and clippy is neutral, which is why GitHub reports unstable. Nothing in this diff is Rust, so that is infrastructure noise — it needs a re-run rather than a change.

More usefully: #878 registers the pull_request trigger on web-integration.yml, which is the gap finding 1 opened. If that lands first and this branch is rebased or repushed, this PR gets a real pnpm test + typecheck run in CI instead of my local one — and becomes the first JS change here to merge on a JS signal. That's the merge order I'd suggest: #878, then this.

Good, disciplined PR — the boolean-not-buffering line and the "informational, not fatal" mismatch call are both the right side of the compatibility boundaries #836 drew.

---

Generated by Claude Code

[0-jake-0]

Merged as 81d4c32. Thank you — this was a genuinely good contribution, and a few things stood out worth naming.

You got both compatibility boundaries #836 drew on the right side without being told: a boolean rather than a buffer, and a mismatch that is surfaced rather than fatal. Those are the two calls the issue left open, and either one taken the other way would have been a much harder change to walk back once the package shipped.

The verification write-up was honest about its own limits — you said plainly that the Windows host had no wasm-pack and that the run went through a stub. That's what let me know exactly which gap to close on my end, and the real wasm build reproduced your 40 passing tests unchanged. A PR that had simply claimed "tests pass" would have been worth less.

You also turned review feedback around cleanly, and rather than quietly working around the CI gap you found, you filed #877 for it so the workflow trigger could be argued on its own merits. That's the right instinct.

Thanks again — please do send more.

---

Generated by Claude Code