refactor(sdk)!: remove WebSockets + consolidate tooling/coverage - #190
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates SSE/WS into a single StreamHandler, removes the websocket handler and WS transport from the SDK, consolidates routing to GET /v1/stream, updates tests and E2E fixtures/orchestrator, and applies supporting tooling/config and docs changes. ChangesStream handler migration and routing
Client/SDK transport and tests
E2E, orchestrator, playground, and fixtures
Tooling and module changes
Docs and marketing
🎯 4 (Complex) | ⏱️ ~45 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request initiates a series of large-scale improvements to the SDK and API, focusing on developer experience, type safety, and error handling. The current changes focus on optimizing the development environment and restoring test coverage for core streaming features, setting the stage for broader architectural updates. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request configures golangci-lint to allow parallel runners in the Makefile and re-enables previously skipped end-to-end tests for SSE and WebSocket streaming. The review feedback correctly identifies that these newly enabled tests do not clean up their streams and subscriptions if an assertion fails or a timeout occurs. It is recommended to wrap the test logic in try...finally blocks to guarantee proper resource cleanup and prevent connection leaks.
- README: streaming example route /v1/stream/sse -> /v1/stream - sse.ts: fix stale "auth token for upgrade" comment (SSE has no upgrade) - CHANGELOG: stop the forward-looking Unreleased entries from advertising the now-removed WS transport; add entries for biome, coverage enforcement, WS removal, pnpm + Makefile consolidation, node engines - package.json x4: engines.node normalized to >=22; drop redundant packageManager from docs/package.json Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
main's #205 access-control guide documented the pre-removal /v1/stream/sse + /v1/stream/ws routes and "WebSocket subscribers". This branch removes WS and collapses the streaming route to /v1/stream, so reconcile the merged-in guide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ci(setup-env): read node-version-file: .nvmrc instead of a hardcoded "22" so the Node version has one source of truth (bump = edit .nvmrc only). - build(pnpm): engineStrict: true in pnpm-workspace.yaml so `pnpm install` errors (not just warns) when the active Node doesn't satisfy engines.node (>=22 in every package.json). pnpm 11 honors engineStrict here, not in .npmrc (verified by forcing a mismatch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
taitelee
left a comment
There was a problem hiding this comment.
Reviewed go file modifications and doc edits related to removing websocket support. Looks good. There does not seem to be dangling/outdated references to it.
## Why Prep for flipping the repo public: the `wave-rf-runners` ARC pool must never serve a public repo (GitHub's hard rule on self-hosted + public), and GitHub-hosted standard runners are free for public repos — with public `ubuntu-latest` being the 4-core/16 GB tier. Reverses the #77 calculus, exactly as planned in the public-launch gates. ## What 1. **`runs-on: [wave-rf-runners]` → `ubuntu-latest`** — all 11 jobs across all 10 workflows. 2. **`docker/setup-buildx-action` in `publish-dev.yml` + `release.yml`** (`d7f5e7f` = v4.1.0, SHA verified against upstream): `dockers_v2` builds the `linux/amd64+arm64` manifest via buildx, and the hosted default docker driver can't do multi-platform — the runner-pod `BUILDX_BUILDER` (remote BuildKit VM) used to provide this. No QEMU needed: `Dockerfile.goreleaser` stages are `$BUILDPLATFORM`-pinned / COPY-only by design. `goreleaser-validate.yml` correctly stays buildx-free (`build --single-target` never touches `dockers_v2`). 3. **Cache-key fix**: the pnpm/Playwright/Astro keys hashed `clients/ts/`, `tests/e2e/sdk/`, `docs/` lockfiles — paths deleted in the #190 root-workspace consolidation — so `hashFiles()` was empty and the keys degenerated to constants that never invalidated. Now keyed on the root `pnpm-lock.yaml`. Old degenerate entries stay reachable via unchanged `restore-keys`, so the first run warm-starts and re-saves under the correct key. No falcondev cache-server or registry-proxy references existed in any workflow (the redirects were runner-pod env) — `actions/cache` falls back to GitHub's backend transparently. Footprint ~2–3 GB vs the 10 GB/repo cap. ## Verification - `make ci` green locally (full pipeline incl. integration + e2e) - pre-push reviewer: ship_it, zero findings; docs review skipped on the record (workflows-only diff) - actionlint: clean for this diff (5 pre-existing info-level shellcheck nits on untouched lines) - repo-wide grep: zero remaining `wave-rf-runners` / `self-hosted` / `BUILDX_BUILDER` references ## Watch-items (post-merge) - **First multi-arch publish on main**: the buildx push path gets its first real exercise on the post-merge `publish-dev.yml` run — watch it. (A pre-merge dispatch from this branch would clobber the rolling `:dev` tag, so deliberately not done.) - **Docker Hub pulls**: test suites pull `clickhouse/clickhouse-server:latest` unauthenticated; hosted runners share egress IPs, so rate-limit flakes become *possible*. If they appear: GHCR mirror or registry login. - **Until the flip**, this repo is private → runs use the 2-core tier and bill against org included minutes; the `ci.yml` 4-core comment describes the post-flip state. Well inside the 45-min cap either way. - **Open PRs** still carry workflow files targeting the old pool — each needs a merge-from-main after this lands (`@dependabot rebase` for the bot ones), same dynamic as #77. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Problem Every Dependabot **npm** PR has been failing CI since the #190 root-lockfile consolidation — e.g. #211 (`@vitest/coverage-v8` in `tests/e2e/sdk`) and #337 (the `docs` group). They fail fast on Lint/Unit/E2E/Coverage and **a rebase never fixes them**. Root cause is the Dependabot config, not the dependencies. The repo is a pnpm workspace with a **single root `pnpm-lock.yaml`**, but `.github/dependabot.yml` pointed its three npm entries at the *member* directories (`directory: /docs`, `/clients/ts`, `/tests/e2e/sdk`). Dependabot only updates a lockfile co-located with the manifest it targets, so it edited the member `package.json` but **left the root lockfile untouched**. CI then runs `pnpm install --frozen-lockfile`, the lockfile's recorded specifier no longer matches `package.json`, and it aborts with `ERR_PNPM_OUTDATED_LOCKFILE`. A rebase only replays the same `package.json`-only change onto fresh `main` — it never regenerates the lockfile, so the mismatch returns every run. (#337, created well after #190, fails identically — confirming it's structural, not a stale branch.) ## Fix Replace the three per-member npm entries with **one entry at the workspace root** (`directory: /`, group `npm-deps`, prefix `deps`). At the root, Dependabot reads `pnpm-workspace.yaml`, walks every member, and updates the single root lockfile inside the PR. As a bonus it now also covers the root `package.json`'s own devDeps (biome, markdownlint, nyc), which the per-member configs never did. **Trade-off:** one combined weekly npm PR with a single `deps:` prefix, instead of three per-area PRs (`docs:` / `deps(sdk):` / `deps(tests):`). This matches the existing grouped `go-deps` / `actions-deps` configs and the team's stated preference for fewer Monday PRs. ## Docs sync - `docs/src/content/docs/development.md` — Dependabot section: five configs → three, plus the root-lockfile rationale. - `SECURITY.md` — supply-chain line: "three npm workspaces" → one root npm config. - `CHANGELOG.md` — `[Unreleased] → Changed` entry. ## Note (out of scope) `pnpm-workspace.yaml` sets `minimumReleaseAge: 10080` (7-day cooldown), which Dependabot doesn't honor. When the new config regenerates lockfiles, a dep released <7 days ago can be held by the cooldown at install time — so a freshly-cut bump may need a few days before it merges cleanly. Pre-existing; the no-auto-merge + admin-review gate covers it. ## Follow-up on the broken PRs Once this merges, the old per-member configs no longer exist, so #211 and #337 should be **closed** (not rebased — rebasing keeps the old member-directory branch and reproduces the failure). The new root config regenerates correct combined PRs on the next Dependabot run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Foundation pass for the SDK/API: removes dead transport code, consolidates build/test tooling, and turns on coverage enforcement — the groundwork the feature work builds on. This PR is intentionally scoped to cleanup/infra only. The features originally planned here have been split into focused, one-PR-each sub-issues tracked under the epic #194.
What's in this PR
ws.ts,ws-manager.ts,stream_ws.go; renamesstream_sse.go→stream.go. (One transport is simpler to focus on, and SSE tends to work more consistently through corporate networks where WS may be blocked.)pnpm-workspace.yaml+ lockfile (shadows stray ancestor workspace files); drops the per-project lockfiles underclients/ts,docs,tests/e2e/sdk.docs/Makefile) with a first-class SDK target.biome.json) plus a repo-wide format pass..testcoverage.yml,scripts/cov, vitest config, e2e fixtures moved totests/e2e/fixtures. Closes chore(coverage): restore SDK statements coverage threshold to ≥50 #176.clients/ts/playground/scratch dir and stalePLAN.md; add editor configs (.vscode,.zed,.editorconfig).Feature work — moved out, tracked in #194
Split into individually-reviewable PRs so this doesn't become a mega-PR:
NDJSON upload (#195) · bounded parallel uploads (#196) · live DLQ streaming (#197) · better TS errors (#198) · builder/command restructure (#199) · cache HIT/MISS + debug internals (#200) · exported error types (#201) · auto dedupe ID (#202) · SSE auth → Fetch
EventSource(#203) · live-vs-replay (#98) · multi-subscription SSE replay (#204)Related Issues