feat(ws): browser client cut-over to /rpc-ws (#326, PR 3/3 of #314) - #349
Conversation
…#314) One websocket per (tab, apiBase), owned by a versioned globalThis registry in @conciv/contract. A DynamicLink is the single selection point: at boot it dials /rpc-ws with a bounded open timeout and, on failure, sticks to fetch/SSE for that tab with a console breadcrumb. widget.transport pins either transport explicitly. makeRpcClient stays on fetch for the CLI, testkit and node integration tests; makeExtRpcClient resolves the shared connection in the browser and keeps its fetch form for node fixtures. - partysocket wraps the socket; the oRPC link gets a thin delegate that overrides only readyState (partysocket reports the stale CLOSED socket during backoff) and forwards close unchanged, so iterators never stall silently. - ClientRetryPlugin owns call re-issue; the wrapper owns socket reconnect. The recorder's hand-rolled 1s control backoff is gone, so the two no longer stack. - Boot no longer blocks first paint on navigation.get: the widget renders with an empty history cache and applies the restored entry only if no local write landed first; the pending read is cancelled on rebind and dispose. - Six tabs in one browser context now each get one socket, zero rpc over http and a working chat round trip. On the fetch transport the same gate cannot even mount the sixth widget. - Test estate: rpc observers attach at page creation (a late observer cannot see an already-open socket), proxy counts websocket upgrades alongside http, and the whiteboard drag-batching test counts procedure calls through the rpc observer instead of http requests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… coverage (#326) - entry-standalone.tsx threads settings.transport into makeBrowserRpcClient and stamps window.__CONCIV_API_BASE__ once at boot so every downstream ext-rpc client (composer terminal, highlight) resolves the same apiBase instead of re-reading a URL the router has since rewritten. - a disposed rpc connection drops peer control frames instead of throwing (mount-impl.tsx, browser-transport.ts) plus the forced-drop/rebind ITs that prove it. - new packages/embed-style playwright IT (apps/conciv/test/transport-standalone.it.test.ts) proves the standalone entry: pinned fetch never opens a websocket, pinned websocket never falls back to fetch — driven against the built standalone page, not just the embed widget. - pnpm-lock.yaml rebuilt from the pre-drift base commit with only partysocket@1.3.0 + its event-target-polyfill transitive added; the unrelated floating @tanstack/* re-resolution is gone. - recorder flush-socket.it.test.ts now measures actual outbound ws frame bytes at the socket.send boundary (post oRPC envelope encode) against @conciv/serve's real DEFAULT_MAX_PAYLOAD_BYTES, not the client's own MAX_FLUSH_BYTES constant compared against itself pre-encode. - extracted the duplicated http-rpc-request-tracking snippet into @conciv/extension-testkit's httpRpcRequestUrls so the new standalone IT does not triple a fragment fallow already saw twice in embed's tests. Probe error-state item from the #326 rework list is NOT included: the "SSE also fails at boot -> widget connection-error state" promised in the canonical spec has no implementation anywhere in the app (no meta/context signal, no UI) to write an honest test against. Flagged for a follow-up design decision rather than inventing product UI here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds shared browser RPC transport selection with WebSocket pooling, fetch fallback, explicit preferences, lifecycle teardown, rebinding, extension integration, and transport-focused integration tests. ChangesBrowser RPC transport cutover
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Widget
participant BrowserRpcConnection
participant RPCWebSocket
participant FetchSSE
Widget->>BrowserRpcConnection: request RPC link
BrowserRpcConnection->>RPCWebSocket: probe /rpc-ws
alt WebSocket available
RPCWebSocket-->>BrowserRpcConnection: accept connection
BrowserRpcConnection-->>Widget: return WebSocket link
else WebSocket unavailable
BrowserRpcConnection->>FetchSSE: select fetch/SSE
FetchSSE-->>BrowserRpcConnection: return fetch link
BrowserRpcConnection-->>Widget: return fetch link
end
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
There was a problem hiding this comment.
Pull request overview
Moves browser RPC traffic from fetch/SSE to a shared WebSocket transport, addressing per-origin HTTP connection starvation.
Changes:
- Adds shared, reconnecting WebSocket transport with probing and fetch fallback.
- Integrates transport selection, rebind behavior, and asynchronous navigation restoration.
- Adds browser integration coverage for transport selection, reconnection, and multi-tab usage.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.changeset/ws-browser-client-cutover.md |
Documents the transport cutover. |
apps/conciv/src/data/settings.ts |
Adds transport configuration parsing. |
apps/conciv/src/entry-standalone.tsx |
Moves standalone RPC to the browser client. |
apps/conciv/test/helpers/fake-core.ts |
Pins fake-core tests to fetch. |
apps/conciv/test/settings.test.ts |
Covers the default transport setting. |
packages/contract/package.json |
Adds PartySocket. |
packages/contract/src/browser-transport.ts |
Implements shared browser transport management. |
packages/contract/src/client.ts |
Adds browser, deferred, and rebindable clients. |
packages/contract/src/index.ts |
Exports browser transport APIs. |
packages/contract/tsconfig.json |
Enables DOM types. |
packages/embed/README.md |
Documents transport behavior and overrides. |
packages/embed/src/mount-impl.tsx |
Integrates browser transport and nonblocking boot. |
packages/embed/src/navigation-storage.ts |
Restores navigation asynchronously. |
packages/embed/test/connection-pool.it.test.ts |
Adds the six-tab regression gate. |
packages/embed/test/embed.it.test.ts |
Installs RPC observers before navigation. |
packages/embed/test/helpers/navigation.ts |
Shares per-page RPC observers. |
packages/embed/test/helpers/proxy.ts |
Adds WebSocket blocking and drop controls. |
packages/embed/test/rebind.it.test.ts |
Updates rebind assertions for WebSockets. |
packages/embed/test/transport-selection.it.test.ts |
Tests probe, fallback, and override behavior. |
packages/extension/src/client-host.ts |
Uses browser RPC for source opening. |
packages/extension/src/ext-rpc.ts |
Shares browser connections with extension RPC. |
packages/extension/src/index.ts |
Updates extension RPC type exports. |
packages/extensions/recorder/src/client/boot.ts |
Uses RPC retry for the control stream. |
packages/extensions/whiteboard/src/client/change-feed.ts |
Moves reconnect handling into call context. |
packages/extensions/whiteboard/test/canvas-drag-batching.it.test.ts |
Observes RPC calls across transports. |
pnpm-lock.yaml |
Records dependency graph changes. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
pnpm-lock.yaml:921
- The only manifest dependency change is
partysocket, but this lockfile regeneration also upgrades the React Router/Start stack and rewrites unrelated Hono, Vitest, and OXC resolutions. That unrelated churn contradicts the stated lockfile-hygiene rework and makes this transport change carry unreviewed dependency updates. Preserve the existing resolutions and add only thepartysocketdependency graph.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| close: () => { | ||
| state.open = false | ||
| socket.close() | ||
| }, |
| const existing = connections.get(key) | ||
| if (existing) return existing |
| const params = new URLSearchParams(window.location.search) | ||
| const router = createConcivRouter({ | ||
| rpc: makeRpcClient(params.get('core') ?? ''), | ||
| rpc: makeBrowserRpcClient(params.get('core') ?? ''), |
| export type NavigationStorage = WebStorage & {restored: Promise<void>; dispose: () => void} | ||
|
|
||
| export function makeNavigationStorage(rpc: RpcClient, onRestore: (href: string) => void): NavigationStorage { | ||
| const state = {cache: null as string | null, lastStamp: 0, wroteLocally: false, cancelled: false} |
|
|
||
| describe('six widget tabs sharing one browser connection pool', () => { | ||
| it('gives every tab one rpc websocket, no rpc over http, and a working chat round trip in the last tab', async () => { | ||
| const context = await browser.newContext() |
| const disposers = [ | ||
| storage.dispose, |
…bes, honest socket state (#326) - unmount now closes the (tab, apiBase) connection and drops its registry entry, so partysocket and its reconnect timers no longer outlive the widget; a later mount re-creates the connection through the same registry and runs the full transport probe again. - rebind to the base the widget is already on is no longer a no-op: it drops the connection so the next call re-probes, which is what makes the #314 "error-retry re-runs the FULL probe" amendment reachable from the handle. - socketDelegate reports partysocket's real readyState via shouldReconnect (open / connecting-while-it-will-reconnect / closed) instead of calling every non-open state CONNECTING, so oRPC fails a send fast rather than awaiting an open that is never coming. - disposeSocket no longer double-dispatches close: partysocket's close() already emits one synchronously, so the synthetic event is now only used for the branches where it emits none (no socket dialled, or already closing/closed). - navigation-storage keeps an explicit state type instead of an `as` cast. - embed ITs: shared handle/chat helpers, a toggleable upgrade block in the test proxy, plus the two new behaviour tests (unmount closes the socket and remount rides a fresh one; a fetch-fallback tab rides the websocket after a same-base rebind once upgrades work again). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Copilot review triage (fixes in Fixed
Already fixed before this review landed (both in
Not changing
Also in 🤖 Generated with Claude Code |
Resolve rebind.it.test.ts conflict: keep main's fuller "delivers the next turn" assertions (title + post-rebind message-count checks) on top of our ws-transport rebind test helpers (observedPage, sendChatMessage, rebindHandle, trafficCount).
…326) The extension testkit navigated its page inside launch(), so a test that called observeRpc() afterwards attached page.on('websocket') after the widget's shared rpc socket was already open and saw nothing. Under the old fetch transport every call was a fresh HTTP request a late observer still caught, which is why canvas-drag-batching only broke once the browser rpc client started probing websockets. getExtensionTestApi now opens every page (including secondClient) with a memoized per-page observer attached before goto, embed's duplicate helper delegates to it, and the ping fixture moved out of test/ so the test host rebuilds when it changes (build inputs exclude test/**). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
apps/conciv/test/settings.test.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover all transport normalization branches.
Line 11 checks only the default value. Add assertions for explicit
websocket, explicitfetch, and an invalid value that falls back toauto. These values controlmakeBrowserRpcClientinapps/conciv/src/entry-standalone.tsx.Suggested assertions
+ it('normalizes transport preferences', () => { + expect(parseConcivSettings('{"transport":"websocket"}').transport).toBe('websocket') + expect(parseConcivSettings('{"transport":"fetch"}').transport).toBe('fetch') + expect(parseConcivSettings('{"transport":"invalid"}').transport).toBe('auto') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/conciv/test/settings.test.ts` at line 11, Add test coverage in the settings test around the existing transport assertion for all normalization branches: explicit websocket, explicit fetch, and an invalid value falling back to auto. Keep the assertions aligned with the transport value consumed by makeBrowserRpcClient.packages/contract/src/browser-transport.ts (2)
144-151: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a larger reconnect ceiling.
MAX_RECONNECT_DELAY_MSis 1000. If the core server stays down, each tab retries about once per second indefinitely. With six tabs this produces sustained connection attempts. A higher ceiling, for example 5000-10000 ms, reduces load while keeping recovery fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/contract/src/browser-transport.ts` around lines 144 - 151, Increase the MAX_RECONNECT_DELAY_MS ceiling used by reconnectingSocket to a substantially larger value, such as 5000–10000 ms, while preserving the existing exponential reconnection behavior and other timing constants.
75-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
JSON.parseagainst non-JSON string frames.
isPeerRequestFrameparsesdatawithout error handling. If a non-JSON string frame reachessendafter the connection is closed,JSON.parsethrows aSyntaxErrorinstead of the intendedCLOSED_CONNECTION_MESSAGEerror. Callers then see a misleading failure.♻️ Proposed fix
function isPeerRequestFrame(data: string | ArrayBufferLike | Blob | ArrayBufferView): boolean { if (typeof data !== 'string') return true - const frame: unknown = JSON.parse(data) - if (typeof frame !== 'object' || frame === null) return true - return !('t' in frame) + try { + const frame: unknown = JSON.parse(data) + if (typeof frame !== 'object' || frame === null) return true + return !('t' in frame) + } catch { + return true + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/contract/src/browser-transport.ts` around lines 75 - 80, Update isPeerRequestFrame to guard JSON.parse with error handling for non-JSON string frames, returning the existing peer-request classification that allows send to produce CLOSED_CONNECTION_MESSAGE instead of propagating SyntaxError. Preserve the current handling for valid JSON, non-string data, null, and non-object frames.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/conciv/test/helpers/static-app.ts`:
- Around line 26-30: Update the candidate selection in the static app request
handler to use statSync(candidate).isFile() rather than existsSync(candidate),
selecting candidate only when it is a regular file and otherwise falling back to
index.html.
In `@packages/contract/src/browser-transport.ts`:
- Around line 82-91: Update disposeSocket to account for the socket’s readyState
before closing: when the underlying socket is already CLOSING, retain the close
listener and avoid dispatching the synthetic close event, allowing the pending
native close to be observed; only remove the listener and synthesize close when
no later close event can arrive. Preserve idempotent close handling for OPEN and
already CLOSED sockets.
In `@packages/contract/src/client.ts`:
- Around line 45-50: Update the guard in bind to check state.base !== null,
matching bound(), so an empty API base is treated as already bound and
subsequent bind calls throw instead of replacing state.ready.
In `@packages/embed/README.md`:
- Around line 17-19: Remove the connection-error and retry behavior claim from
the README section describing WebSocket and fetch/SSE failures, leaving only
behavior that is currently implemented.
In `@packages/embed/src/navigation-storage.ts`:
- Around line 27-32: Update the navigation write setup in the storage
implementation to retain the Debouncer returned by debounce, guard its callback
with state.cancelled, and cancel that debouncer during storage.dispose(). Ensure
pending writes are skipped after disposal and cannot use a re-bound rpc client.
In `@packages/embed/test/connection-pool.it.test.ts`:
- Line 67: Add a concise comment at the browser.newContext() call explaining
that the test intentionally creates six pages within one BrowserContext, and
that using browser.newPage() would isolate them and alter the behavior being
tested.
---
Nitpick comments:
In `@apps/conciv/test/settings.test.ts`:
- Line 11: Add test coverage in the settings test around the existing transport
assertion for all normalization branches: explicit websocket, explicit fetch,
and an invalid value falling back to auto. Keep the assertions aligned with the
transport value consumed by makeBrowserRpcClient.
In `@packages/contract/src/browser-transport.ts`:
- Around line 144-151: Increase the MAX_RECONNECT_DELAY_MS ceiling used by
reconnectingSocket to a substantially larger value, such as 5000–10000 ms, while
preserving the existing exponential reconnection behavior and other timing
constants.
- Around line 75-80: Update isPeerRequestFrame to guard JSON.parse with error
handling for non-JSON string frames, returning the existing peer-request
classification that allows send to produce CLOSED_CONNECTION_MESSAGE instead of
propagating SyntaxError. Preserve the current handling for valid JSON,
non-string data, null, and non-object frames.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 13a692a8-acf6-4583-ab57-9606bd180339
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.changeset/ws-browser-client-cutover.md.changeset/ws-connection-teardown.mdapps/conciv/src/data/settings.tsapps/conciv/src/entry-standalone.tsxapps/conciv/test/helpers/fake-core.tsapps/conciv/test/helpers/listen-local.tsapps/conciv/test/helpers/proxy.tsapps/conciv/test/helpers/static-app.tsapps/conciv/test/settings.test.tsapps/conciv/test/transport-standalone.it.test.tspackages/contract/package.jsonpackages/contract/src/browser-transport.tspackages/contract/src/client.tspackages/contract/src/index.tspackages/contract/tsconfig.jsonpackages/core/test/api/rpc-ws.it.test.tspackages/embed/README.mdpackages/embed/src/mount-impl.tsxpackages/embed/src/navigation-storage.tspackages/embed/test/connection-pool.it.test.tspackages/embed/test/create-conciv.it.test.tspackages/embed/test/embed.it.test.tspackages/embed/test/forced-drop.it.test.tspackages/embed/test/helpers/chat.tspackages/embed/test/helpers/handle.tspackages/embed/test/helpers/navigation.tspackages/embed/test/helpers/proxy.tspackages/embed/test/rebind.it.test.tspackages/embed/test/transport-selection.it.test.tspackages/extension-testkit/fixtures/ping/client.tsxpackages/extension-testkit/fixtures/ping/router.tspackages/extension-testkit/fixtures/ping/server.tspackages/extension-testkit/package.jsonpackages/extension-testkit/src/get-extension-test-api.tspackages/extension-testkit/src/launch.tspackages/extension-testkit/src/rpc-observer.tspackages/extension-testkit/test/boot-server.it.test.tspackages/extension-testkit/test/call-tool.it.test.tspackages/extension-testkit/test/fixtures/ping/client.tsxpackages/extension-testkit/test/page-rpc-observer.it.test.tspackages/extension-testkit/test/session.it.test.tspackages/extension-testkit/test/smoke.it.test.tspackages/extension-testkit/tsconfig.jsonpackages/extension-testkit/vite.test-host.config.tspackages/extension/src/client-host.tspackages/extension/src/ext-rpc.tspackages/extension/src/index.tspackages/extensions/recorder/package.jsonpackages/extensions/recorder/src/client/boot.tspackages/extensions/recorder/test/flush-socket.it.test.tspackages/extensions/whiteboard/src/client/change-feed.tspackages/extensions/whiteboard/test/canvas-drag-batching.it.test.tspackages/serve/src/serve.ts
💤 Files with no reviewable changes (1)
- packages/extension-testkit/test/fixtures/ping/client.tsx
| const write = debounce( | ||
| (state: NavigationState, updatedAt: number) => { | ||
| void rpc.navigation.set({...state, updatedAt}).catch(() => {}) | ||
| (navigation: NavigationState, updatedAt: number) => { | ||
| void rpc.navigation.set({...navigation, updatedAt}).catch(() => {}) | ||
| }, | ||
| {wait: WRITE_DELAY_MS}, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the debounce import and its declared API.
rg -n 'debounce' packages/embed/src/navigation-storage.ts
rg -n --type=json '"`@tanstack/pacer`"|"debounce"' -g 'package.json' | head -20
fd -t d 'pacer' node_modules --max-depth 3Repository: conciv-dev/conciv
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- navigation-storage.ts ---'
cat -n packages/embed/src/navigation-storage.ts
printf '%s\n' '--- mount-impl.tsx relevant calls ---'
rg -n -C 8 'storage\.dispose|closeConnection|rebind' packages/embed/src/mount-impl.tsx
printf '%s\n' '--- debounce package declarations and versions ---'
rg -n -C 3 '"`@tanstack/pacer`"|`@tanstack/pacer`|debounce\(' \
package.json pnpm-lock.yaml packages pnpm-workspace.yaml 2>/dev/null | head -200
printf '%s\n' '--- available pacer files ---'
fd -i 'pacer' . --type f --exclude node_modules --exclude dist | head -100Repository: conciv-dev/conciv
Length of output: 15848
🌐 Web query:
@tanstack/pacer 0.21.1 debounce cancel API
💡 Result:
In TanStack Pacer, the mechanism for stopping execution depends on whether you are using the synchronous Debouncer or the asynchronous AsyncDebouncer [1][2]. Synchronous Debouncing The Debouncer class provides a cancel method to stop any pending execution [1][3]. debouncer.cancel // Clears the pending timeout and resets internal state [3] Asynchronous Debouncing For AsyncDebouncer, there are two distinct methods for handling execution flow, as asynchronous work often involves in-flight promises that may need to be aborted [4][2]. 1. cancel: This method cancels any pending execution that has not yet started [4][2]. It does not affect any execution already in progress [4][2]. debouncer.cancel // Cancels pending execution, does not abort in-flight work [4][2] 2. abort: This method aborts all ongoing executions using an internal AbortController [4][2]. It does not cancel pending executions that have not yet started [4][2]. To utilize this effectively, you must pass the abort signal provided by the debouncer to your asynchronous operation [2]. const debouncer = new AsyncDebouncer(async (...) => { const signal = debouncer.getAbortSignal; // Access the signal const response = await fetch(url, { signal }); // Pass to operation return response.json; }, options); debouncer.abort; // Aborts the in-flight fetch operation [2] Summary of Differences Method | Scope | Effect --- | --- | --- cancel | Pending (not yet started) | Clears the timeout or queue, preventing future execution [3][4][2]. abort | In-flight (currently running) | Signals underlying async operations (like fetch) to stop [4][2]. Framework hooks (e.g., useDebouncer, useAsyncDebouncedCallback) typically handle cleanup automatically by calling cancel or abort on component unmount [1][2]. You can often customize this behavior using the onUnmount option to call flush instead if desired [1][2].
Citations:
- 1: https://tanstack.com/pacer/latest/docs/guides/debouncing.md
- 2: https://tanstack.dev/pacer/latest/docs/guides/async-debouncing
- 3: https://github.com/TanStack/pacer/blob/main/packages/pacer/src/debouncer.ts
- 4: https://github.com/TanStack/pacer/blob/main/packages/pacer/src/async-debouncer.ts
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- rebindable RPC implementation ---'
rg -n -C 12 'makeRebindableRpcClient|rebindClient' packages
printf '%s\n' '--- debounce implementation for v0.21.1 ---'
curl -fsSL https://raw.githubusercontent.com/TanStack/pacer/v0.21.1/packages/pacer/src/debounce.ts 2>/dev/null || true
curl -fsSL https://raw.githubusercontent.com/TanStack/pacer/v0.21.1/packages/pacer/src/debouncer.ts 2>/dev/null || true
printf '%s\n' '--- package metadata and exported debounce source ---'
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/package.json
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/dist/esm/debounce.js 2>/dev/null || trueRepository: conciv-dev/conciv
Length of output: 9976
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exported debounce implementation ---'
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/dist/index.js \
| grep -n -A35 -B10 'function debounce\|const debounce\|export.*debounce' | head -160
printf '%s\n' '--- debounce type declarations ---'
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/dist/index.d.cts \
| grep -n -A20 -B10 'debounce' | head -160
printf '%s\n' '--- DynamicLink behavior ---'
rg -n -C 20 'class DynamicLink|function DynamicLink|type DynamicLink' packagesRepository: conciv-dev/conciv
Length of output: 4324
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Debouncer implementation ---'
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/dist/debouncer.js \
| grep -n -A80 -B10 'class Debouncer\|function debounce' | head -220
printf '%s\n' '--- Debouncer declarations ---'
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/dist/debouncer.d.cts \
| grep -n -A35 -B10 'declare class Debouncer\|declare function debounce' | head -220
printf '%s\n' '--- DynamicLink definitions in all tracked files ---'
rg -n -C 15 'DynamicLink' --glob '!pnpm-lock.yaml' --glob '!*.map' .Repository: conciv-dev/conciv
Length of output: 17022
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Debouncer cancellation behavior ---'
curl -fsSL https://unpkg.com/@tanstack/pacer@0.21.1/dist/debouncer.js \
| sed -n '90,185p'
printf '%s\n' '--- API-shape verifier ---'
python3 - <<'PY'
import re
import urllib.request
js = urllib.request.urlopen(
'https://unpkg.com/@tanstack/pacer@0.21.1/dist/debouncer.js',
timeout=10,
).read().decode()
dts = urllib.request.urlopen(
'https://unpkg.com/@tanstack/pacer@0.21.1/dist/debouncer.d.cts',
timeout=10,
).read().decode()
assert re.search(r'function debounce\(fn, initialOptions\)\s*\{\s*return new Debouncer\(fn, initialOptions\)\.maybeExecute;', js)
assert re.search(r'cancel:\s*\(\)\s*=>\s*void', js) or '.cancel = ' in js or 'cancel() {' in js
assert re.search(r'declare function debounce[\s\S]*\): \(\.\.\.args: Parameters<TFn>\) => void;', dts)
assert re.search(r'cancel:\s*\(\)\s*=>\s*void', dts)
print('debounce() returns only maybeExecute; cancel() belongs to Debouncer.')
PYRepository: conciv-dev/conciv
Length of output: 5712
Cancel disposed navigation writes
When rebindClient changes the shared rpc link before storage.dispose(), a pending write can send the previous navigation state to the new API base. Guard the callback with state.cancelled and retain a Debouncer instance so dispose can call debouncer.cancel(). debounce() returns only maybeExecute; write.cancel() is not available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/embed/src/navigation-storage.ts` around lines 27 - 32, Update the
navigation write setup in the storage implementation to retain the Debouncer
returned by debounce, guard its callback with state.cancelled, and cancel that
debouncer during storage.dispose(). Ensure pending writes are skipped after
disposal and cannot use a re-bound rpc client.
|
|
||
| describe('six widget tabs sharing one browser connection pool', () => { | ||
| it('gives every tab one rpc websocket, no rpc over http, and a working chat round trip in the last tab', async () => { | ||
| const context = await browser.newContext() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='packages/embed/test/connection-pool.it.test.ts'
printf '%s\n' '--- file excerpt ---'
sed -n '1,130p' "$file"
printf '%s\n' '--- browser page/context usage ---'
rg -n -C 2 'browser\.(newContext|newPage)|context\.newPage|page\.|test\(' "$file"
printf '%s\n' '--- related test files ---'
git ls-files 'packages/embed/test' | rg '\.(test|spec)\.(ts|tsx|js|jsx)$' | head -80Repository: conciv-dev/conciv
Length of output: 6210
Document the BrowserContext exception. This test intentionally creates six pages in one context; replacing browser.newContext() with browser.newPage() would isolate the pages and change the behavior under test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/embed/test/connection-pool.it.test.ts` at line 67, Add a concise
comment at the browser.newContext() call explaining that the test intentionally
creates six pages within one BrowserContext, and that using browser.newPage()
would isolate them and alter the behavior being tested.
Source: Coding guidelines
…ferred bind, navigation write races, static EISDIR) - browser-transport.ts: disposeSocket no longer synthesizes a close event when the underlying socket is still CLOSING, since partysocket's close() leaves its close listener attached in that branch and the real close event will still arrive — synthesizing one too was a double terminal event for the peer. - client.ts: makeDeferredRpcClient's bind() and bound() now agree on `state.base !== null`, and an empty api base is rejected at bind time instead of silently marking the client bound. - navigation-storage.ts: the debounced navigation write is now a retained Debouncer so dispose() can cancel() it, closing the window where a rebind swaps the shared rpc link while a write scheduled against the old base is still pending. - static-app.ts (test helper): use statSync(...).isFile() instead of existsSync, which is also true for directories and let createReadStream throw an unhandled EISDIR. - embed README: drop the documented connection-error retry state that does not exist yet (#350). - connection-pool.it.test.ts: name the describe block to explain why it shares one browserContext across six tabs instead of six newPage()s, without adding a banned code comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CodeRabbit review addressed in
Two small seams added solely for testability: Gates green (contract/embed/app, serial), fallow 0 introduced. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/contract/test/browser-transport.test.ts (1)
10-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the class-based test double.
FakeNativeSocketis a class in a TypeScript file. Replace it with a constructable function-based test double.As per coding guidelines, "
**/*.{ts,tsx,js,jsx}: Use functions instead of classes, exceptBaseTextAdapterinpackages/harness/src/_shared/text-adapter.ts, which may remain a subclass because library typing requires it."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/contract/test/browser-transport.test.ts` around lines 10 - 30, Replace the class-based FakeNativeSocket test double with a constructable function that preserves its EventTarget behavior, static instances tracking, socket state constants, properties, send, and close behavior. Update the function’s prototype or construction logic as needed so existing tests can instantiate it with new while avoiding a class declaration.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/conciv/test/static-app.test.ts`:
- Around line 10-12: Update the test teardown around app to declare app as
optional and guard the app.close() call in afterAll so cleanup runs only when
setup assigned an app instance.
---
Nitpick comments:
In `@packages/contract/test/browser-transport.test.ts`:
- Around line 10-30: Replace the class-based FakeNativeSocket test double with a
constructable function that preserves its EventTarget behavior, static instances
tracking, socket state constants, properties, send, and close behavior. Update
the function’s prototype or construction logic as needed so existing tests can
instantiate it with new while avoiding a class declaration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 687bf725-f5e4-4268-8e21-eb3873f19ffe
📒 Files selected for processing (10)
apps/conciv/test/helpers/static-app.tsapps/conciv/test/static-app.test.tspackages/contract/src/browser-transport.tspackages/contract/src/client.tspackages/contract/test/browser-transport.test.tspackages/contract/test/deferred-client.test.tspackages/embed/README.mdpackages/embed/src/navigation-storage.tspackages/embed/test/connection-pool.it.test.tspackages/embed/test/navigation-storage.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/embed/README.md
- packages/embed/test/connection-pool.it.test.ts
- packages/contract/src/browser-transport.ts
- apps/conciv/test/helpers/static-app.ts
- packages/contract/src/client.ts
- packages/embed/src/navigation-storage.ts
| afterAll(async () => { | ||
| await app.close() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n apps/conciv/test/static-app.test.ts
printf '%s\n' '--- related hook patterns ---'
rg -n -U 'beforeAll\\([\\s\\S]{0,500}?afterAll\\(' apps packages --glob '*.{test,spec}.ts' --glob '*.{test,spec}.tsx' | head -200Repository: conciv-dev/conciv
Length of output: 1102
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n apps/conciv/test/static-app.test.ts
printf '%s\n' '--- related hook patterns ---'
rg -n -U 'beforeAll\([\s\S]{0,500}?afterAll\(' apps packages --glob '*.{test,spec}.ts' --glob '*.{test,spec}.tsx' | head -200Repository: conciv-dev/conciv
Length of output: 16668
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- helper implementation ---'
fd -i 'static-app' . --type f
helper=$(fd -i 'static-app' . --type f | head -1)
if [ -n "${helper:-}" ]; then
cat -n "$helper"
fiRepository: conciv-dev/conciv
Length of output: 2168
🌐 Web query:
Vitest documentation beforeAll fails afterAll runs
💡 Result:
In Vitest, if a beforeAll hook fails, the subsequent tests in that suite are skipped [1], and the afterAll hook will typically not run for that suite [2]. This behavior can make it difficult to perform necessary cleanup if the setup performed in beforeAll fails midway. To ensure that cleanup logic always executes—even if setup fails—the recommended approach is to use the cleanup function returned by beforeAll or use the aroundAll hook. 1. Cleanup function in beforeAll: You can return a function from your beforeAll hook. Vitest guarantees that this cleanup function will be executed after the suite finishes, even if the setup logic itself encountered issues [3][4]. beforeAll(async => { await setup; return async => { await teardown; }; }); 2. aroundAll hook: Alternatively, you can use aroundAll to wrap your entire suite. This allows you to use a try...finally block to ensure teardown logic runs regardless of whether the setup (or the suite execution itself) fails [2][5]. aroundAll(async (runSuite) => { try { await setup; await runSuite; } finally { await teardown; } }); Using these patterns helps avoid resource leaks that occur when a test suite is aborted before it can reach the afterAll block [2].
Citations:
- 1: test state is in "pending" when
beforeAllfailed vitest-dev/vitest#4820 - 2: aroundAll(): To ensure teardown even if beforeAll aborted. vitest-dev/vitest#2694
- 3: https://vitest.dev/api/hooks.html
- 4: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/hooks.md
- 5: https://vitest.dev/guide/lifecycle
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- listenLocal implementation ---'
fd -i 'listen-local' apps packages --type f --exec cat -n {}
printf '%s\n' '--- Vitest version and configuration references ---'
rg -n 'vitest|pool|sequence|hook' package.json pnpm-lock.yaml vitest.config.* apps/conciv --glob '!**/node_modules/**' | head -240Repository: conciv-dev/conciv
Length of output: 25443
🌐 Web query:
site:vitest.dev/api/hooks beforeAll afterAll failure cleanup Vitest 4
💡 Result:
In Vitest, if a beforeAll hook fails, the tests within that suite will not execute, and the corresponding afterAll hook will generally not be executed for that suite [1]. To ensure cleanup logic runs even if setup fails, you can use the cleanup function feature of beforeAll. When a beforeAll hook returns a function, Vitest treats that returned function as a cleanup task (equivalent to afterAll) that is guaranteed to run after the suite finishes, regardless of whether the setup succeeded or if the tests passed or failed [1]. Example usage: beforeAll(async => { await startMocking; // This returned function acts as an afterAll cleanup return async => { await stopMocking; }; });
Citations:
🌐 Web query:
site:vitest.dev/guide/lifecycle beforeAll afterAll failure Vitest
💡 Result:
In Vitest, if a beforeAll hook fails, the tests within that suite will not be executed [1]. The afterAll hook will still execute even if the beforeAll hook fails, provided that the suite itself was initiated [1]. This ensures that any necessary cleanup operations defined in afterAll are performed regardless of whether the setup phase in beforeAll succeeded [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -eu
for url in \
'https://vitest.dev/api/hooks.html' \
'https://vitest.dev/guide/lifecycle.html' \
'https://raw.githubusercontent.com/vitest-dev/vitest/v4.1.10/packages/runner/src/run.ts'
do
printf '\n--- %s ---\n' "$url"
curl -LfsS "$url" | grep -in -E -C 4 'beforeAll|afterAll|cleanup|fail' | head -240 || true
doneRepository: conciv-dev/conciv
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
url='https://raw.githubusercontent.com/vitest-dev/vitest/v4.1.10/packages/runner/src/run.ts'
printf '%s\n' "--- $url ---"
curl -LfsS "$url" | nl -ba | grep -E -C 12 'beforeAll|afterAll|runSuite|callSuiteHook|suite\.result' | head -240
printf '%s\n' '--- lifecycle page text around failure terms ---'
curl -LfsS 'https://vitest.dev/guide/lifecycle.html' |
sed 's/<[^>]*>/ /g; s/&/\&/g; s/&`#39`;/'"'"'/g' |
tr -s ' ' |
grep -ioE '.{0,180}(beforeAll|afterAll|fail|skip).{0,240}' |
head -80Repository: conciv-dev/conciv
Length of output: 4424
🏁 Script executed:
#!/bin/bash
set -eu
url='https://raw.githubusercontent.com/vitest-dev/vitest/v4.1.10/packages/runner/src/run.ts'
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -LfsS "$url" -o "$tmp"
printf '%s\n' "--- $url ($(wc -c < "$tmp") bytes) ---"
grep -n -E -C 18 'beforeAll|afterAll|runSuite|callSuiteHook|suite\.result|hasFailed' "$tmp" | head -320Repository: conciv-dev/conciv
Length of output: 10665
🏁 Script executed:
#!/bin/bash
set -eu
url='https://raw.githubusercontent.com/vitest-dev/vitest/v4.1.10/packages/runner/src/run.ts'
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -LfsS "$url" -o "$tmp"
grep -n -E 'export async function runSuite|async function runSuite|runSuite\(' "$tmp"
sed -n '760,980p' "$tmp"Repository: conciv-dev/conciv
Length of output: 6778
Guard teardown when setup fails.
Vitest runs afterAll even when beforeAll rejects. In that case, app is undefined, so app.close() throws a second error. Declare app as optional and close it only when assigned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/conciv/test/static-app.test.ts` around lines 10 - 12, Update the test
teardown around app to declare app as optional and guard the app.close() call in
afterAll so cleanup runs only when setup assigned an app instance.
Closes #326. Final PR of the #314 rollout: the widget's browser rpc moves from fetch/SSE to one WebSocket per (tab, apiBase), killing the 6-connections-per-host starvation.
DRAFT — one rework commit still landing (standalone-override test, lockfile hygiene, probe error-state assertion, recorder frame-boundary hardening). The cut-over commit is complete and reviewable.
Design (binding spec on #326 + user amendment on #314)
Evidence
Codex full-branch adversarial review: frame classification proven sound vs oRPC 1.14.7 frame taxonomy, no lifecycle leaks, probe bounded + sticky. Rework items from that review are the pending commit.
🤖 Generated with Claude Code
Summary by CodeRabbit