feat(api,web): resume chat streams (#49, #80) - #110
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds refresh-safe chat stream resume across the API and web client, including a new ChangesRefresh-safe stream resume feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ChatsController
participant ChatPage
participant DefaultChatTransport
participant TenantDbService
participant RunStreamBridgeService
Browser->>ChatPage: refresh mid-run
ChatPage->>DefaultChatTransport: reconnect with chat id
DefaultChatTransport->>ChatsController: GET /api/v1/chats/:id/stream
ChatsController->>TenantDbService: runAs(userId)
TenantDbService->>RunStreamBridgeService: stream active run when present
RunStreamBridgeService-->>DefaultChatTransport: SSE replay
DefaultChatTransport-->>ChatPage: resumed messages
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the resume-on-refresh feature (#49), allowing active chat runs to survive page reloads and resume streaming. It introduces the GET /api/v1/chats/:id/stream endpoint, wires it into the web client, and adds Playwright end-to-end tests using a new mock OpenAI-compatible model server. Additionally, the model client was switched to /chat/completions for better compatibility with alternative providers, and auth rate limits were made configurable for testing. Feedback is provided regarding a potential race condition in resumeChatStream where the abort signal listener is registered after an asynchronous database lookup, which could lead to unhandled stream pipeline errors if a client disconnects early.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc1eac8ffa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/chats/chats.controller.spec.ts">
<violation number="1" location="apps/api/src/chats/chats.controller.spec.ts:76">
P2: The new `tenantDb` and `bridge` dependencies are wired into the test setup and injected into the controller, but no test covers the `resumeChatStream` endpoint that depends on them. This means the RLS enforcement path (`tenantDb.runAs(userId, ...)`) and the stream bridge response (`bridge.createUiMessageStreamResponse(...)`) are untested, including the 204 no-active-run branch. Consider adding coverage for the resume endpoint — at minimum a happy-path test that verifies `tenantDb.runAs` is called with the correct userId, and a no-run test that asserts the 204 response.</violation>
</file>
<file name="apps/api/src/auth/constants.ts">
<violation number="1" location="apps/api/src/auth/constants.ts:19">
P2: A fractional AUTH_RATE_LIMIT_PER_MINUTE value is treated as valid even though @nestjs/throttler expects an integer request count. Consider rejecting non-integers so a typo like `0.5` or `10.5` falls back to the safe default instead of producing surprising auth throttling behavior.</violation>
</file>
<file name="apps/api/test/worker-mode.e2e-spec.ts">
<violation number="1" location="apps/api/test/worker-mode.e2e-spec.ts:568">
P3: Add `expect(other.status).toBe(201)` after the cross-tenant registration so a registration failure produces a clear failure at the right line instead of a confusing 204/401 mismatch on the downstream GET. The existing `beforeAll` pattern at line ~199 already does this, so the omission is likely an oversight.</violation>
</file>
<file name="apps/web/app/(chat)/components/chat-page.tsx">
<violation number="1" location="apps/web/app/(chat)/components/chat-page.tsx:179">
P2: Disabling `resume` when `navigateOnFinish` is true can drop the first in-flight response on refresh. Once the first message is submitted, the run already exists server-side, but reloading before `onFinish` prevents reconnecting to `/chats/:id/stream`, so the answer can disappear from the current page session. Persisting the created chat id (or enabling resume after submit) would keep first-turn runs recoverable.</violation>
</file>
<file name="apps/api/src/chats/chats.controller.ts">
<violation number="1" location="apps/api/src/chats/chats.controller.ts:86">
P3: The new resume endpoint can return 400 for malformed chat IDs, but `/docs` will omit that response because the `ParseUUIDPipe` failure is not documented. Consider adding the same `@ApiBadRequestResponse` used by the other `:id` chat endpoints.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
leon0399
force-pushed
the
stack/overnight-resume-e2e
branch
from
July 2, 2026 15:40
dc1eac8 to
0bd424e
Compare
leon0399
force-pushed
the
stack/overnight-auth-hardening
branch
2 times, most recently
from
July 5, 2026 12:37
20be567 to
9f39e2b
Compare
The API half of refresh-safe resume: GET /api/v1/chats/:id/stream returns the chat's active run as an AI SDK UI-message stream (the existing RunStreamBridgeService, reused wholesale — it replays the run's persisted events from the start and follows it live to completion), or 204 when there is nothing to resume. This matches the AI SDK v6 transport contract for reconnectToStream, so the apps/web hookup that closes #49 is a small custom-transport method + a resume flag. "The chat's active run" is well-defined because the per-chat single-flight index admits at most one non-terminal run — the new RunsRepository.findActiveByChatId leans on that invariant. Security: session-guarded like everything else (global guard); a cross-tenant or unknown chat id answers the same 204 as "no active run" — resuming leaks neither existence nor state. Proven in worker-mode e2e: disconnect mid-run, then GET the stream — the full ordered chunk sequence (start → text-start → deltas → text-end → finish) arrives with the complete answer; after completion the endpoint answers 204; another tenant gets 204. The apps/web resume client (transport reconnectToStream + resume flag + browser verification) remains to close #49. Refs #49 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
The client half of refresh-safe resume, matching the API endpoint from the previous commit: - The chat DefaultChatTransport gains prepareReconnectToStreamRequest, pointing the SDK's reconnect at GET /api/v1/chats/:id/stream (the default convention would have targeted .../messages/:id/stream). Shares the existing authAwareFetch + credentials wiring. - Persisted chat sessions mount with resume: true — on load, useChat probes for an active run and, when one exists (worker mode), replays its UI-message stream live from the durable event log. Draft chats (navigateOnFinish) skip the probe: they cannot have a server-side run yet. An idle chat's 204 resolves to null in the SDK — a no-op. Verification: new vitest unit for the URL preparation, web typecheck + lint + build clean, and the full 10-test Playwright browser suite passes against the live api+web stack — the resume probe on every persisted-chat mount regresses nothing in inline mode. Remaining to close #49: a browser test of an actual mid-run refresh, which needs the Playwright-launched API running RUN_EXECUTION_MODE= worker — the natural companion to the #80 chat-flow e2e and the worker-mode soak. Refs #49 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
…49, #80) The browser proof that closes the durable-runs story: - e2e/model-server.ts: a deterministic OpenAI-compatible mock speaking /chat/completions streaming SSE; the Playwright-launched api points OPENAI_BASE_URL at it (#88) — real loop, zero provider spend. A "SLOW" prompt drips tokens over ~4s so tests can reload mid-answer; title-generation calls get a distinct short answer so message and sidebar title are distinguishable. - The whole Playwright api now runs RUN_EXECUTION_MODE=worker: all 12 browser tests exercise the queue-executed pipeline — standing soak evidence for flipping the default (#50). - e2e/chat/chat-flow.spec.ts: create → stream → render (#80's scope), and the headline — reload the page mid-answer; the run survives in the worker, the persisted chat remounts with resume, and the FULL answer appears on screen from the durable event log. Fixes surfaced by building this: - Latent #88 bug: the model client used the provider default, which targets OpenAI's proprietary /responses endpoint — OpenAI-compatible providers (OpenRouter, groq, local; the whole point of OPENAI_BASE_URL) only implement /chat/completions. Now uses openai.chat(model), which works everywhere including OpenAI. - The strict 10/min per-IP auth throttle starved parallel browser workers logging in from one IP: the limit is now env-tunable (AUTH_RATE_LIMIT_PER_MINUTE, harness sets 1000); the production default stays 10/min. Verified: 12/12 Playwright browser tests green (auth + chat flows, all under worker mode), full api unit + RLS/e2e suites green after the /chat/completions change, builds clean. Closes #49 Closes #80 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
…st execute The browser chat-flow + refresh-resume suite this PR introduces was not wired into any CI job — shipped coverage that nothing runs is not coverage. New job boots the full stack (throwaway Postgres via the Playwright webServer orchestration, api + web + mock model server) and drives Chromium against it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
leon0399
force-pushed
the
stack/overnight-resume-e2e
branch
from
July 5, 2026 14:23
0bd424e to
46b2784
Compare
…esume All six distinct bot findings verified and fixed: - the resume endpoint registers its abort signal BEFORE the awaited run lookup (a client disconnecting during the lookup fired 'close' with no listener attached — the bridge then streamed to a destroyed response until its cap), and requestAbortSignal aborts immediately when the response is already destroyed at registration. Deliberately NOT request.destroyed: a POST whose body stream is fully consumed reads as destroyed on a live connection (found by the e2e battery when the first attempt broke every streaming suite) - first-turn resume (web): the draft chat id is recorded per-tab (sessionStorage-backed chat context) at send time — a refresh mid-first-answer re-mounts `/` with the SAME id, fetches the persisted user turn, and the resume probe picks the stream back up; previously the id was lost and the reply stranded until found in the sidebar. Draft navigation semantics unchanged; the stored id clears once the turn finishes and navigation adopts the chat - resumeChatStream unit tests: 204-no-run and bridge-active-run paths, asserting tenant scoping comes from the session-derived userId - AUTH_RATE_LIMIT_PER_MINUTE rejects fractional values (throttler expects an integer count; a typo falls back to the strict default) - the resume endpoint documents the ParseUUIDPipe 400 like its sibling :id endpoints; openapi.json regenerated - the cross-tenant resume e2e asserts the second registration succeeded before relying on its cookie Verified: tsgo + oxlint (type-aware) + prettier clean, web tsc + oxlint clean, api unit green (exit-code checked, maxWorkers capped), full rls-test.sh green (RLS 13, queue 8, e2e 36). Browser Playwright suite runs in the new CI job (previous commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
Owner
Author
|
Rebased onto current master (post #109): the three slice commits replay with the module-refactor import paths fixed and the new resume test re-anchored in the reworked worker-mode suite. Two additions beyond the review fixes: (1) a browser-e2e CI job — the Playwright suite this PR exists to add (#80) was not wired into any CI workflow, i.e. shipped coverage that nothing executes; it now boots the full stack (throwaway Postgres, api + web + mock model server) and drives Chromium on every push. (2) First-turn resume: the draft chat id is recorded per-tab at send time, so refreshing mid-first-answer reconnects instead of stranding the reply until it's found in the sidebar — closing the most common resume case, which the endpoint alone didn't cover. All 8 review threads fixed and answered individually; the abort-registration race fix came with a lesson (request.destroyed reads true for consumed POST bodies on live connections — the e2e battery caught my first attempt breaking every streaming suite before it shipped). Verified per push: api unit + RLS 13 + queue 8 + jest e2e 36; the browser suite runs in the new CI job. |
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/chats/chats.controller.ts">
<violation number="1" location="apps/api/src/chats/chats.controller.ts:302">
P2: The new `response.destroyed` early-abort path (the check for a socket that's already gone) is untested. Both new resume tests mock `destroyed: false`, so the early-return branch never runs. Adding a test with `destroyed: true` would confirm the controller exits without hitting the DB or the bridge.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Inline review round two (self-review of the previous round's additions
plus the remaining unreviewed files) and the browser job's first CI run:
- the flagship "refresh mid-answer" browser test waited for the deep
link BEFORE reloading — but the deep link is only adopted onFinish,
so it reloaded after completion and degenerated into an ordinary
persistence check. Rewritten into two genuine proofs: reload on `/`
at first token (the rehydrated-draft path this PR's client work
enables) and reload mid-second-answer on a persisted chat
- draft rehydration is now hydration-safe and precise: the stored id
loads in a mount effect (a useState initializer rendered a different
tree client-side than the server sent — hydration mismatch), and a
draftRestored flag distinguishes storage-restored drafts from in-app
New Chat mints (which were silently taking the persisted-chat mount
path with a pointless fetch + probe) — also flagged by cubic
- Playwright pins locale 'en-US': CI Chromium ships an empty
navigator.language and new Intl.Locale('') throws (TanStack Query
devtools under next dev), wrecking hydration — the likely cause of
the account-menu button never becoming clickable in the logout test
- the mock model server survives a peer disconnect mid-drip (unhandled
stream error would have taken down every later test's model backend)
- next-env.d.ts restored to the production-build variant (a dev-server
regeneration was committed during the rebase; CI's clean-tree check
rightly rejected it)
Verified: web tsc + oxlint clean, api untouched this round (previous
battery stands). Browser suite validates in the CI job.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
A response whose socket died before the handler ran exits after the single lookup — no bridge, no writes (cubic review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
User-reported regression from the #50 default flip: messages appeared only after completion, not gradually. Root cause: the delta buffer coalesces tokens into model.delta events by SIZE only (400 chars) — written when the event log was a replay record, and carrying an explicit "revisit granularity when the loop moves into the worker" comment that the flip never revisited. With worker execution default, the event log IS the live channel the bridge streams from, so any answer under 400 chars persisted as a single event at stream end. The buffer now flushes on size OR age (150ms since the first buffered char), whichever comes first. Time is injected by the caller (push(text, Date.now())) so the buffer stays pure and timer-free — flushes only happen on a push or the final drain, so worst-case staleness is one token gap. Write amplification stays bounded: ~7 events/sec per active run worst case, under the bridge's own 200ms poll cadence. Verified: delta-buffer unit tests (age flush, clock restart, legacy no-time mode), 182 api unit tests, full 12-test Playwright browser suite through the real worker + bridge path (10m soak), build green. Refs #50 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
- the SSR RangeError persisted after pinning the BROWSER locale because it was never the browser: Node >=21 derives navigator.language from the process locale, and the runner's LANG=C yields an invalid tag that new Intl.Locale() rejects during next-dev SSR chunk evaluation (TanStack Query devtools). The job now runs under en_US.UTF-8 - Playwright traces/report upload as artifacts on failure — the logout click-timeout needs a trace if the locale fix doesn't clear it - the chat-page hydration source-assertion updated to the restructured guard (navigateOnFinish && !rehydratedDraft) — web vitest suite was not part of my local check set; it is now (20/20 green) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements refresh-safe “resume chat streaming” end-to-end: the API adds a chat stream-resume endpoint backed by durable run events, and the web client reconnects to in-flight streams after refresh. It also adds deterministic browser E2E coverage for the create → stream → render flow and mid-stream refresh behavior, plus CI wiring to run the Playwright suite.
Changes:
- API: add
GET /api/v1/chats/:id/streamto replay the active run as an AI SDK UI-message SSE stream (or204when idle), and adjust the OpenAI client to target/chat/completions. - Web: add transport support for AI SDK
reconnectToStreamand enable resume-on-mount; persist draft chat id per-tab to support mid-first-answer refresh. - Tests/CI: add a deterministic mock model server, Playwright chat-flow specs (including refresh-resume), and a dedicated browser-e2e CI job.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ROADMAP.md | Removes the now-shipped #49 item from the roadmap. |
| playwright.config.ts | Adds mock model server + locale override and wires worker-mode env for browser E2E. |
| e2e/model-server.ts | Adds a deterministic OpenAI-compatible streaming mock for browser E2E. |
| e2e/chat/chat-flow.spec.ts | Adds browser coverage for chat streaming and refresh-resume scenarios. |
| CHANGELOG.md | Records shipping notes for resume, E2E coverage, and /chat/completions fix. |
| apps/web/lib/services/chat/transport.ts | Adds /stream URL builder and reconnect request helper. |
| apps/web/lib/services/chat/transport.test.ts | Updates transport tests (currently drops coverage for send-envelope behavior). |
| apps/web/contexts/chat-context.tsx | Adds per-tab draft chat id persistence and draftRestored state. |
| apps/web/app/(chat)/components/chat-page.tsx | Enables AI SDK resume behavior and draft rehydration logic in the chat page. |
| apps/api/test/worker-mode.e2e-spec.ts | Adds worker-mode e2e coverage for GET /chats/:id/stream resume semantics. |
| apps/api/src/models/openai-model-client.ts | Switches streaming client to openai.chat(model) for /chat/completions. |
| apps/api/src/models/model-client.spec.ts | Updates tests to reflect .chat usage. |
| apps/api/src/chats/chats.controller.ts | Adds GET /api/v1/chats/:id/stream endpoint and improves abort handling docs/logic. |
| apps/api/src/chats/chats.controller.spec.ts | Adds controller unit coverage for resume behavior. |
| apps/api/src/chats/chats-repository.spec.ts | Adds repository test for findActiveByChatId scoping/excluding terminal runs. |
| apps/api/src/auth/constants.ts | Adds env-tunable auth throttle constant. |
| apps/api/src/auth/auth.controller.ts | Uses env-tunable throttle limit for login/register. |
| apps/api/openapi.json | Updates OpenAPI output for the new stream-resume endpoint. |
| .github/workflows/ci.yml | Adds a Playwright browser-e2e job to CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The persisted-chat resume test waited for the FULL second answer before reloading — the same degenerate-into-persistence flaw the first test had, in subtler form (.nth(1) of the full-answer locator only exists after completion). It now reloads at the second answer's first token (cubic review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(chat)/components/chat-page.tsx (1)
224-236: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle the refresh-before-create race
apps/web/app/(chat)/components/chat-page.tsx:224-236
setDraftChatId(chatId)runs before the firstsendMessage(...)is acknowledged, but the rehydrated draft path still mounts the persisted-session query and stream probe for that id. Those client reads don’t special-case 404, so a refresh in that window can show an error state instead of a fresh draft. Mirror the server-side 404 fallback here, or persist the draft id only after the create-or-append request succeeds.🤖 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/web/app/`(chat)/components/chat-page.tsx around lines 224 - 236, The draft chat flow in chat-page.tsx can persist the new chat id before sendMessage() succeeds, causing the rehydrated session/query path to probe a not-yet-created chat and surface a 404 error on refresh. Update the send flow around setDraftChatId and sendMessage so the draft id is only persisted after the create-or-append request succeeds, or add the same 404 fallback used on the server-side resume path. Use the existing sendMessage, setDraftChatId, and navigateOnFinish logic in ChatPage to locate and adjust the race.
🧹 Nitpick comments (5)
.github/workflows/ci.yml (1)
118-118: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching Playwright browsers to speed up CI.
playwright install --with-deps chromiumre-downloads the browser on every run. Caching~/.cache/ms-playwrightkeyed on the Playwright version would cut CI time for this job, similar to the turbo cache used in thechecksjob.⚡ Suggested caching step
- run: pnpm install --frozen-lockfile + + - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + id: playwright-cache + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }} + + - if: steps.playwright-cache.outputs.cache-hit != 'true' + run: pnpm exec playwright install --with-deps chromium + - if: steps.playwright-cache.outputs.cache-hit == 'true' + run: pnpm exec playwright install-deps chromium - - run: pnpm exec playwright install --with-deps chromium🤖 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 @.github/workflows/ci.yml at line 118, The CI job currently re-installs the Playwright Chromium browser on every run, which slows the workflow. Update the Playwright setup in the workflow around the `pnpm exec playwright install --with-deps chromium` step to cache the browser directory `~/.cache/ms-playwright` and restore it using a key tied to the Playwright version. Keep the install step, but make it reuse the cache when available so repeated runs avoid downloading browsers unnecessarily.e2e/model-server.ts (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication of SSE response headers.
The same three-header object is written in both the title-generation branch and the main-answer branch. Could be hoisted into a small helper for a single point of truth, but it's isolated to test-only code.
♻️ Optional dedup
+const sseHeaders = { + "content-type": "text/event-stream", + "cache-control": "no-cache", + connection: "keep-alive", +} as const; + if (raw.includes("Generate a short chat title")) { - res.writeHead(200, { - "content-type": "text/event-stream", - "cache-control": "no-cache", - connection: "keep-alive", - }); + res.writeHead(200, sseHeaders); ... } const slow = raw.includes("SLOW"); -res.writeHead(200, { - "content-type": "text/event-stream", - "cache-control": "no-cache", - connection: "keep-alive", -}); +res.writeHead(200, sseHeaders);Also applies to: 89-93
🤖 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 `@e2e/model-server.ts` around lines 76 - 80, The SSE response headers are duplicated in both the title-generation and main-answer branches in the model server test helper. Hoist the shared header object into a small reusable helper in the same area of e2e/model-server.ts and call it from both response paths so there is a single point of truth, using the existing response setup around res.writeHead.apps/api/test/worker-mode.e2e-spec.ts (1)
508-576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset shared fake-model delay in a
finally(orafterEach).
models.client.delayMsis restored to0only at the end of the test. If anyexpectabove throws, the reset is skipped and the shared fake client keeps the 2s delay for whatever test runs next in this file, turning one failure into a confusing cascade.🧹 Guard the reset with try/finally
it('GET /chats/:id/stream resumes the active run after a disconnect', async () => { models.client.delayMs = 2_000; const chatId = crypto.randomUUID(); - - const pending = request(http) - ... - models.client.delayMs = 0; + try { + const pending = request(http) + ... + } finally { + models.client.delayMs = 0; + } });🤖 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/api/test/worker-mode.e2e-spec.ts` around lines 508 - 576, Reset the shared fake-model delay in worker-mode.e2e-spec.ts using a finally block or an afterEach hook around the GET /chats/:id/stream resumes the active run after a disconnect test, because models.client.delayMs is currently restored only at the end of the test and can leak a 2s delay into later tests if any expect fails. Keep the delay setup and all request/assertion logic in the same test, but ensure the cleanup that sets models.client.delayMs back to 0 always runs even on failure.apps/api/src/chats/chats.controller.spec.ts (1)
342-370: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify
abortSignalreaches the bridge.The assertion only checks
runId/userIdviaobjectContaining, so a regression that dropsabortSignalfrom the bridge call wouldn't be caught here, even though it's central to cleanly aborting the stream on disconnect.✅ Suggested strengthening
expect(bridge.createUiMessageStreamResponse).toHaveBeenCalledWith( - expect.objectContaining({ runId: 'run-1', userId: 'verified-user' }), + expect.objectContaining({ + runId: 'run-1', + userId: 'verified-user', + abortSignal: expect.any(AbortSignal), + }), );🤖 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/api/src/chats/chats.controller.spec.ts` around lines 342 - 370, The resumeChatStream test only asserts runId and userId, so it can miss a regression where abortSignal is no longer passed to the bridge. Strengthen the expectation on createUiMessageStreamResponse in chats.controller.spec.ts to verify the bridge call includes abortSignal from the request/response wiring, using the resumeChatStream and bridge call setup in the test to locate the assertion.apps/api/src/models/model-client.spec.ts (1)
65-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
.chataliasing to the same mock defeats the purpose of this test update.
(openaiProvider as unknown as { chat: unknown }).chat = openaiProvider;makes.chat(...)and the baseopenaiProvider(...)indistinguishable calls in assertions (same jest mock, same call log). IfstreamText'smodel: openai.chat(model)regressed back tomodel: openai(model), these tests would still pass. Use a distinct mock for.chatso assertions can verify the chat-completions factory specifically was invoked.✅ Suggested strengthening
- (openaiProvider as unknown as { chat: unknown }).chat = openaiProvider; + const chatModelMock = jest.fn(openaiProvider); + (openaiProvider as unknown as { chat: unknown }).chat = chatModelMock;...then assert
chatModelMock(notopenaiProvider) was called with the expected model id where the test intends to verify the chat-completions path.Also applies to: 111-111, 140-141
🤖 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/api/src/models/model-client.spec.ts` around lines 65 - 66, The model-client tests are aliasing `.chat` to the same `openaiProvider` mock, which makes `openai.chat(...)` indistinguishable from `openai(...)` in assertions. Update the setup in `model-client.spec.ts` to use a separate mock for the `.chat` factory (for example alongside `openaiProvider`) and wire `openaiProvider.chat` to that distinct mock. Then change the relevant assertions in the affected tests to verify the chat mock was called with the expected model id, so the `streamText` path using `openai.chat(model)` is actually exercised.
🤖 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.
Outside diff comments:
In `@apps/web/app/`(chat)/components/chat-page.tsx:
- Around line 224-236: The draft chat flow in chat-page.tsx can persist the new
chat id before sendMessage() succeeds, causing the rehydrated session/query path
to probe a not-yet-created chat and surface a 404 error on refresh. Update the
send flow around setDraftChatId and sendMessage so the draft id is only
persisted after the create-or-append request succeeds, or add the same 404
fallback used on the server-side resume path. Use the existing sendMessage,
setDraftChatId, and navigateOnFinish logic in ChatPage to locate and adjust the
race.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 118: The CI job currently re-installs the Playwright Chromium browser on
every run, which slows the workflow. Update the Playwright setup in the workflow
around the `pnpm exec playwright install --with-deps chromium` step to cache the
browser directory `~/.cache/ms-playwright` and restore it using a key tied to
the Playwright version. Keep the install step, but make it reuse the cache when
available so repeated runs avoid downloading browsers unnecessarily.
In `@apps/api/src/chats/chats.controller.spec.ts`:
- Around line 342-370: The resumeChatStream test only asserts runId and userId,
so it can miss a regression where abortSignal is no longer passed to the bridge.
Strengthen the expectation on createUiMessageStreamResponse in
chats.controller.spec.ts to verify the bridge call includes abortSignal from the
request/response wiring, using the resumeChatStream and bridge call setup in the
test to locate the assertion.
In `@apps/api/src/models/model-client.spec.ts`:
- Around line 65-66: The model-client tests are aliasing `.chat` to the same
`openaiProvider` mock, which makes `openai.chat(...)` indistinguishable from
`openai(...)` in assertions. Update the setup in `model-client.spec.ts` to use a
separate mock for the `.chat` factory (for example alongside `openaiProvider`)
and wire `openaiProvider.chat` to that distinct mock. Then change the relevant
assertions in the affected tests to verify the chat mock was called with the
expected model id, so the `streamText` path using `openai.chat(model)` is
actually exercised.
In `@apps/api/test/worker-mode.e2e-spec.ts`:
- Around line 508-576: Reset the shared fake-model delay in
worker-mode.e2e-spec.ts using a finally block or an afterEach hook around the
GET /chats/:id/stream resumes the active run after a disconnect test, because
models.client.delayMs is currently restored only at the end of the test and can
leak a 2s delay into later tests if any expect fails. Keep the delay setup and
all request/assertion logic in the same test, but ensure the cleanup that sets
models.client.delayMs back to 0 always runs even on failure.
In `@e2e/model-server.ts`:
- Around line 76-80: The SSE response headers are duplicated in both the
title-generation and main-answer branches in the model server test helper. Hoist
the shared header object into a small reusable helper in the same area of
e2e/model-server.ts and call it from both response paths so there is a single
point of truth, using the existing response setup around res.writeHead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ebd4366-7e65-45de-8f64-777625d24d84
📒 Files selected for processing (19)
.github/workflows/ci.ymlCHANGELOG.mdROADMAP.mdapps/api/openapi.jsonapps/api/src/auth/auth.controller.tsapps/api/src/auth/constants.tsapps/api/src/chats/chats-repository.spec.tsapps/api/src/chats/chats.controller.spec.tsapps/api/src/chats/chats.controller.tsapps/api/src/models/model-client.spec.tsapps/api/src/models/openai-model-client.tsapps/api/test/worker-mode.e2e-spec.tsapps/web/app/(chat)/components/chat-page.tsxapps/web/contexts/chat-context.tsxapps/web/lib/services/chat/transport.test.tsapps/web/lib/services/chat/transport.tse2e/chat/chat-flow.spec.tse2e/model-server.tsplaywright.config.ts
💤 Files with no reviewable changes (1)
- ROADMAP.md
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…batch - e2e suites in one serial jest process share the throwaway Postgres, and a suite's STOPPING consumers could steal the next suite's jobs from the shared 'pgboss' schema — observed as two executions interleaving one run's event log ([start, error, text-start, ...]: the dying app's stolen execution aborts and fails the job, the retry streams on top). Each test file now gets its own pg-boss schema via a PGBOSS_SCHEMA test seam (production unset ⇒ default 'pgboss') - the browser resume test asserts the sessionStorage contract directly (recorded at send, identical after reload) — pinpoints which side breaks instead of a generic timeout - draft persistence split (cubic): setDraftChatId is state-only (an in-app New Chat mint is NOT restorable); recordSentDraft persists — only a send makes a draft worth restoring - auth logout spec adapted to the #120 icon-rail shell: expand the sidebar before clicking the account trigger (tooltip-wrapped icons can stay "unstable" for Playwright) - transport.test regains prepareSendMessagesRequest coverage (last-message-only + reject-empty — dropped in the rebase) - worker-mode e2e: afterEach resets the fake's delayMs (a mid-test throw must not leak slow-drip into later tests); resume unit tests use real UUIDs; requestAbortSignal drops its unused request param - CI: artifacts upload on failure() || cancelled() (a timeout-cancel left no trace otherwise) Verified: tsgo + oxlint + prettier clean, web vitest 22, api spot suites + full rls-test.sh green (RLS 13, queue 8, e2e 36 — twice, with the interleaving reproduced then eliminated). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
The instrumentation did its job: the send-time write is proven (storedBefore = the chat id), and the post-reload null is the resumed stream FINISHING during the slow CI page boot — onFinish then legitimately clears the draft id and adopts the deep link. The outcome assertions (full answer visible, deep link adopted) cover both the mid-run and just-finished timings; the equality check only encoded a timing assumption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
Marked TEMP-DIAG(#49-ci); removed once the trace answers why the restored draft state never re-renders on the CI runner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…onFinish The CI trace diagnostics pinned the broken first-turn resume: the resume gate (chatId === draftChatId) flipped TRUE mid-session the moment the send recorded the draft id. The AI SDK immediately probed reconnectToStream, raced the not-yet-committed POST, got 204, and treated the empty resume as a finished stream — onFinish then cleared the draft id and began navigating ~1s after send, so the reload found empty storage and mounted a fresh draft. resume is now an explicit mount-time prop: DraftChatSession false, PersistedChatSession (incl. rehydrated drafts) true. It can never flip mid-session; a draft that finishes navigates to the persisted mount, which is where resume belongs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/app/(chat)/components/chat-page.hydration.test.ts (1)
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text regex assertion is brittle.
Matching
navigateOnFinish && !rehydratedDraftagainst raw source text couples the test to exact code shape rather than behavior — a semantically-equivalent refactor (e.g. reordering the guard, extracting a variable) would fail this test even though behavior is unchanged. Consider testing the actual rendered output (draft vs persisted session) instead, if feasible in this harness.🤖 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/web/app/`(chat)/components/chat-page.hydration.test.ts around lines 43 - 45, The assertion in chat-page.hydration.test.ts is too tightly coupled to the source text inside the hydration logic and should be replaced with a behavior-based check. Update the test around the chat-page hydration flow to verify the actual rendered outcome or draft/session behavior instead of matching the exact guard expression in the source, using the same test harness and the relevant chat-page hydration path.
🤖 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.
Nitpick comments:
In `@apps/web/app/`(chat)/components/chat-page.hydration.test.ts:
- Around line 43-45: The assertion in chat-page.hydration.test.ts is too tightly
coupled to the source text inside the hydration logic and should be replaced
with a behavior-based check. Update the test around the chat-page hydration flow
to verify the actual rendered outcome or draft/session behavior instead of
matching the exact guard expression in the source, using the same test harness
and the relevant chat-page hydration path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4393e14-08f4-45e7-8e22-c2f044a02793
📒 Files selected for processing (17)
.github/workflows/ci.ymlCHANGELOG.mdapps/api/src/chats/chats.controller.spec.tsapps/api/src/chats/chats.controller.tsapps/api/src/queue/queue.module.tsapps/api/src/runs/delta-buffer.spec.tsapps/api/src/runs/delta-buffer.tsapps/api/src/runs/run-execution.service.tsapps/api/test/jest-e2e.jsonapps/api/test/jest-e2e.setup.tsapps/api/test/worker-mode.e2e-spec.tsapps/web/app/(chat)/components/chat-page.hydration.test.tsapps/web/app/(chat)/components/chat-page.tsxapps/web/contexts/chat-context.tsxapps/web/lib/services/chat/transport.test.tse2e/auth/auth.spec.tse2e/chat/chat-flow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/chat/chat-flow.spec.ts
- apps/api/test/worker-mode.e2e-spec.ts
- apps/web/contexts/chat-context.tsx
- apps/api/src/chats/chats.controller.ts
- apps/api/src/chats/chats.controller.spec.ts
- apps/web/app/(chat)/components/chat-page.tsx
Second half of the CI-diagnosed draft-resume failure: a page reload aborts the in-flight POST fetch, and the AI SDK surfaces that abort through onFinish — which then adopted the chat, CLEARED the recorded draft id, and began navigating during page teardown. The reloaded page found empty storage and mounted a fresh draft: exactly the stranded first answer this slice exists to fix. onFinish now checks the SDK's isAbort/isDisconnect/isError flags and only performs the draft→persisted transition on a real completion; an interrupted stream just refreshes the sidebar. The run survives server-side and the reloaded page rehydrates and resumes it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
The investigation is closed — the console probes identified both root causes (derived resume flipping mid-session; abort surfacing through onFinish), the fixes landed, and the browser suite is green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
This was referenced Jul 6, 2026
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
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.
Summary
Issues
Deferred in later stack PRs
Summary by CodeRabbit