test(e2e): streaming-UX prerequisites — controllable mock streams, testids, chat fixtures#2088
Conversation
Support-level tests driving the mock over real HTTP: slow-stream pacing, held-stream hold/release, /__streams introspection, abort safety and /__reset stream release. Fails until the modes exist. The default-path tests (instant `pong` + usage, JSON completion) pass already by design — they are the regression guard proving metering specs 09-14 keep their behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Model-name-triggered pacing, so no app change is needed to drive it — the seeded user's currentAiModel arrives as body.model: e2e/slow-stream → N chunks paced on a timer (default 40 × 250ms ≈ 10s) e2e/held-stream → first chunk, then held until POST /__release-stream any other model → the original instant path, byte-for-byte untouched Plus GET /__streams (open/held) so specs wait for a stream to be live instead of sleeping, POST /__stream-config for pacing, and /__reset now releases open streams + restores defaults so a held stream can never leak across specs. Abort-safe: every write is guarded on writableEnded/destroyed and the connection-level close clears the chunk timer, so Stop (7.4) cannot crash the mock or pin the event loop past server.close(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Attributes only — no behavior change, so this can merge ahead of the epic's rendering PRs and its testids survive the cutover. Adds the leaf's table (surface roots, list, bubbles + data-role/-message-id, skeleton, Stop/Send, textarea, error banner, history items) and reserves history-live-badge for the Durability epic's badge. Two board corrections, made on the 7.0b leaf before implementing (the table was written against wt-9vu2uxbg; this branch is master-based): - The sidebar shares no bubble/list/indicator component with the other surfaces — it renders CompactMessageRenderer and its own inline list, "Thinking..." indicator and skeleton. The leaf's own example selector (sidebar-chat-tab → chat-message) could not have resolved from the listed files, so those four sites are tagged too. - The sidebar gate is already `!isInitialized` alone; the isMessagesLoading term is gone (now the in-place selectMessagesAreaMode indicator). Tagged the real gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
seedChatConversation / seedChatPage seed chat_messages rows sharing a conversationId (alternating roles, 1s apart so createdAt ordering is unambiguous); seedConversationMessages does the same for the unified messages table behind the global assistant. Reuses the existing createChatMessage/createAgentPage factories rather than new inserts. authedContext turns a seedUser() session token into a logged-in browser context, mirroring global-setup's cookie shape exactly. Chat specs need their own user: the shared storageState user is provider 'openai' and would never reach the mock. Two contexts on one token is safe — per-tab identity is the client-generated X-Browser-Session-Id. 15-chat-fixture-smoke.spec.ts is the harness's own proof: seeded messages render, a slow-stream send grows a bubble, a held stream keeps Stop up until released. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a controllable mock OpenRouter stream server, deterministic chat database and browser fixtures, stable chat DOM selectors, and Playwright/Vitest coverage for seeded messages, paced streaming, held streams, cleanup, and abort handling. ChangesChat E2E coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SmokeTest
participant ChatFixture
participant BrowserChat
participant MockOpenRouter
SmokeTest->>ChatFixture: Seed chat page and create authenticated context
ChatFixture->>BrowserChat: Navigate and send message
BrowserChat->>MockOpenRouter: Request streaming completion
MockOpenRouter-->>BrowserChat: Emit assistant SSE chunks
SmokeTest->>MockOpenRouter: Release held streams
BrowserChat-->>SmokeTest: Update message and stop controls
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2111ab92e8
ℹ️ 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".
| const startedAt = opts.startedAt ?? new Date(Date.now() - 60_000); | ||
|
|
||
| for (let i = 0; i < contents.length; i++) { | ||
| await factories.createChatMessage(pageId, { |
There was a problem hiding this comment.
This helper only inserts chat_messages rows, but the page chat initializer lists conversations through conversationRepository.listConversations, whose privacy filter only returns rows with a matching conversations.userId or isShared=true. As a result, seedChatPage() creates history that the UI's initial GET /conversations?pageSize=1 cannot see, so the new smoke test and later specs open a fresh empty conversation instead of rendering the seeded one. Please create the matching conversations row for each seeded page conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 79051ad — thank you, this was a real bug that would have made every later spec silently test the wrong thing.
Verified the diagnosis against the source rather than taking it on faith: countConversations / listConversations (apps/web/src/lib/repositories/conversation-repository.ts:258-300) LEFT JOIN conversations conv and then filter WHERE conv."userId" = $user OR conv."isShared" = true. With no conversations row the join yields NULL, both predicates evaluate to NULL, and the conversation is filtered out — so the page would open a fresh empty conversation exactly as you describe.
Proved it end-to-end on a live Postgres by replaying the repository's own SQL against seeded data:
messages-only (the OLD seeder) visible to UI query: 0 <- the bug
after adding the conversations row (the FIX): 1
seedChatConversation now inserts the row with the same shape the app's own createConversation uses (type: 'page', contextId: pageId, isShared: false), with lastMessageAt/updatedAt derived from the last seeded message rather than now so history ordering is faithful. Re-verified against the real query: both conversations are returned, newest-first (B then A — B is what the page opens on), and a different user still sees none of them, so the privacy filter is exercised rather than bypassed.
Leaving this thread open for your verification.
| const assistant = page | ||
| .getByTestId('ai-chat-view') | ||
| .getByTestId('chat-message') | ||
| .filter({ has: page.locator('[data-role="assistant"]') }) |
There was a problem hiding this comment.
This filter looks for a descendant matching [data-role="assistant"], but the new data-role is placed on the chat-message element itself, not inside it. When a streamed assistant bubble exists, this locator still resolves to no elements, so the slow-stream smoke test times out at toBeVisible(); select [data-testid="chat-message"][data-role="assistant"] or otherwise filter on the message element itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 79051ad — correct on both the mechanism and the consequence.
filter({ has: ... }) scopes to a descendant match, but 7.0b puts data-role on the chat-message element itself (MessageRenderer.tsx:367, CompactMessageRenderer.tsx:360), so the locator resolved to zero elements and the slow-stream test would have hung to timeout at toBeVisible().
Now selects on the element itself, as you suggested:
const assistant = page
.getByTestId('ai-chat-view')
.locator('[data-testid="chat-message"][data-role="assistant"]')
.last();Added a comment at the call site recording why it must be an attribute selector rather than a has: filter, since this is an easy trap to re-introduce in 7.1-7.5 which will select bubbles the same way.
One caveat worth stating plainly: this spec cannot execute in my environment — the web app does not boot under bun (require('pg') from the Next server bundle; filed as D.1 on the board and documented in the PR body), so the fix is verified by code reading against the emitted attributes rather than by a green run. A reviewer on an environment where the app boots should confirm it.
Leaving this thread open for your verification.
…ement (7.0c) Two P2 review findings, both confirmed real against the source: 1. seedChatConversation inserted chat_messages but no conversations row. listConversations/countConversations LEFT JOIN conversations and filter `WHERE conv."userId" = $user OR conv."isShared" = true`, so with no row the join yields NULL, both predicates are NULL, and the seeded history is invisible — the page would open a fresh empty conversation instead. Now inserts the row with the same shape the app's own createConversation uses (type 'page', contextId = pageId), with lastMessageAt/updatedAt set from the last seeded message rather than "now". Proven against the real query on a live DB: a messages-only conversation counts 0 to the UI's privacy filter, 1 once the row exists. Both seeded conversations are now returned, newest-first (B, then A), and a different user still sees none of them. 2. The smoke spec filtered assistant bubbles with `has: [data-role=...]`, which matches a DESCENDANT — but data-role sits on the chat-message element itself, so it resolved to nothing and the slow-stream test would have timed out. Now an attribute selector on the element itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79051ad09c
ℹ️ 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".
| baseURL: string, | ||
| ): Promise<BrowserContext> { | ||
| const url = new URL(baseURL); | ||
| const context = await browser.newContext(); |
There was a problem hiding this comment.
When these chat specs create pages through this manual browser.newContext(), the context does not inherit the project use.baseURL. gotoChatPage() later calls page.goto('/dashboard/...'), so every spec using authedContext() will try to navigate to a relative URL without a base URL and fail before reaching the chat UI. Pass baseURL into newContext() (or navigate with an absolute URL) along with adding the cookie.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed — authedContext now passes baseURL into newContext({ baseURL }).
You were exactly right, and the repo already had the precedent: 07-file-uploads.spec.ts:99 does newContext({ baseURL: baseURL ?? 'http://localhost:3000' }) for the same reason. A manually-created context does not inherit the project's use.baseURL, so every relative page.goto('/dashboard/…') would have failed before reaching the chat UI. Comment added at the call site recording why, since 7.1-7.5 all build contexts through this helper.
This one is now proven by execution rather than by reading: the smoke spec passes 3/3 against a real production build. Getting there also surfaced three further defects in the same code path that only running it could reveal — the cookie-consent banner (fixed bottom-0 z-[100]) silently ate every composer click, a hydration race discarded the typed message so Send never enabled, and — most importantly — 7.0a's model-name pacing never worked through the app at all, because resolveProviderModel rewrites unknown model ids to DEFAULT_MODEL before the provider call. All fixed; details in the PR description.
Thank you for these three — every one was real, and the first two were load-bearing for the specs this PR exists to enable.
Leaving open for your verification.
….0c)
The smoke spec now RUNS, and it found four real defects that no amount of
direct-to-mock testing could have surfaced. 3/3 green end-to-end.
1. Model-name pacing never worked through the app (7.0a's core premise).
resolveProviderModel rewrites any model outside the static catalog to
DEFAULT_MODEL before the provider call (ai-providers-config.ts:485-488),
so `e2e/slow-stream` never reached the mock — the app sent
`openai/gpt-5.3-chat` instead. Verified against a production build: the
client POST carries selectedModel=e2e/held-stream, the mock records
openai/gpt-5.3-chat. Fix: an explicit `POST /__stream-config {mode}`,
applied regardless of the model that arrives. Model-name triggers are
retained for direct-to-mock callers. Default stays 'instant', so the
metering specs are untouched; /__reset restores it so a mode can't leak.
/__streams now reports `mode` — "already finished" and "mode never
applied" are indistinguishable from open:0 alone.
2. The cookie-consent banner ate every composer click. It is
`fixed bottom-0 z-[100]` and Playwright refuses to click through it, so
chat-send never became actionable and the click hung until the test
timeout. authedContext now pre-records a necessary-only consent decision
using the real CONSENT_* constants, so a version bump fails loudly.
3. A hydration race silently dropped the typed message. ai-chat-view is
visible from server-rendered markup before React hydrates the composer; a
fill() landing in that window is discarded, Send never enables, and the
click waits forever. sendChatMessage retries fill until Send enables — no
sleep, proceeds the instant the composer is live.
4. Seeding an odd `contents` leaves a dangling user message, which reads as a
turn in flight and disables the composer. Seed complete exchanges.
Also adds `pg` to apps/web dependencies — the verified fix for D.1. `pg` is
in serverExternalPackages, so the server bundle emits a bare require('pg')
resolved from next's own directory; bun's isolated store has no hoisted copy,
so the app died at boot and NO browser e2e could run at all. This one line is
what makes the whole suite runnable. Flagged for reviewer judgement: it is a
product manifest change, and the D.1 note lists the alternatives.
Verified: metering specs 09+10 produce IDENTICAL results with this mock and
with origin/master's mock on the same app build (same 5 pre-existing env
failures, same 3 passes) — proving no regression. Default mock path is also
byte-identical to master across all 8 request/endpoint cases.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
The documented command — `bun run --filter '@pagespace/e2e' test:e2e`, in apps/e2e/README.metering.md and on the epic board — could never have worked. It runs with cwd=apps/e2e, and global-setup derived `E2E_DIR` as `path.join(process.cwd(), 'apps/e2e')`, so it wrote to apps/e2e/apps/e2e/ and died on ENOENT before a single test. The writer was cwd-based while the reader (fixtures/seed-state.ts) is already `__dirname`-relative, so the two only agreed when the runner happened to be invoked from the repo root. Resolving from the file's own location makes them agree by construction, from any cwd. Teardown had the same bug with a quieter failure: from a non-root cwd it took the "No .seed-state.json — nothing to clean up" branch and silently leaked the seeded user and drive instead of cleaning them up. All three invocations now pass 3/3 and clean up after themselves: bun run --filter '@pagespace/e2e' test:e2e (the documented one) cd apps/e2e && bun run test:e2e (the epic body's recipe) ./apps/e2e/node_modules/.bin/playwright test --config apps/e2e/playwright.config.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…icit instant Findings from an adversarial review pass of 475d5db. It refuted several of its own hypotheses (the instant-path invariant, the consent cookie's encoding and suppression, sendChatMessage's flakiness, and pg's blast radius all verified clean) and left four real ones: - A comment claimed importing CONSENT_VERSION makes a version bump "fail these specs loudly". It does the opposite: the bump flows into the fixture's cookie, parseConsentCookie keeps accepting it, and the banner stays suppressed silently. Claiming a guard the code does not enforce is worse than no comment, so the comment now states what is actually true — including what it does NOT buy you. - /__streams reported `mode` specifically so a spec could tell "already finished" from "mode never applied", but mockStreams() typed it away, so no Playwright spec could reach it. The justification is now delivered. - An explicit {mode:'instant'} could not override the model-name triggers, because 'instant' doubled as "unset". The override is now nullable: null = unset (model names apply), any explicit mode wins — including 'instant'. Covered by a test that would hold forever under the old logic. - The poll ceilings summed past the 90s test timeout, so the 30s budgets were unreachable fiction: a run needing them died on "Test timeout exceeded" instead of the targeted assertion. Budget raised above their sum. Also updates the module docblock, which still described the model-name-only design, and moves `pg` into alphabetical position. Filed as an epic-level D (NOT fixed here — product scope, outside this PR's diff): the composer silently discards text typed before hydration. That is a real user-facing bug on a cold load, and sendChatMessage's retry means every 7.1-7.5 spec is now structurally blind to it. Support tests 15/15, smoke spec 3/3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Declared and assigned, never called — `res.on('close', cleanup)` handles
abandonment directly. Dead API surface on a type that 7.1-7.5 will read.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The chat surfaces mount simultaneously and each renders its own composer, so
an unscoped getByTestId('chat-send') matches TWO elements once the right
sidebar is open and fails Playwright's strict mode. It passed only because
rightSidebarOpen defaults to false — an accidental precondition that would
have broken the first 7.1-7.5 spec to open the sidebar (7.1 asserts on the
sidebar's gate, so that is not hypothetical).
sendChatMessage now takes a Page | Locator scope, and the smoke spec passes
`getByTestId('ai-chat-view')` — the per-surface pattern 7.1-7.5 must follow.
Verified live against a production build, which also completes 7.0b's stated
acceptance ("a snippet can resolve each selector uniquely per surface") for
the surfaces the smoke spec never mounts:
sidebar-chat-tab 1
↳ scoped chat-messages-area 1
↳ scoped chat-textarea 1
↳ scoped chat-send 1
history-conversation-item 2 (+ data-conversation-id present)
unscoped chat-textarea/-send 2 ← why scoping is mandatory
Those scoped sidebar queries would all be ZERO without the sidebar testids
added by this PR's 7.0b board correction — the leaf's own example selector
targeted components the sidebar does not render.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e2e/fixtures/chat.fixture.ts`:
- Around line 1-5: Update the consent constants import in the chat fixture to
use the public package export `@pagespace/lib/consent`, replacing the direct
internal consent-core source path. Keep importing CONSENT_COOKIE_NAME and
CONSENT_VERSION unchanged.
In `@apps/e2e/support/db.ts`:
- Around line 230-250: Update seedConversationMessages so the
conversations.lastMessageAt value matches the timestamp of the final seeded
message, using the same calculation as the last message’s createdAt (startedAt +
(msgs.length - 1) * 1000). Keep the existing deterministic staggered timestamps
and align this behavior with seedChatConversation.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1be913f-8958-4c2a-b81c-cc73c8dfffa5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
apps/e2e/fixtures/chat.fixture.tsapps/e2e/global-setup.tsapps/e2e/global-teardown.tsapps/e2e/package.jsonapps/e2e/support/__tests__/mock-openrouter.test.tsapps/e2e/support/db.tsapps/e2e/support/http.tsapps/e2e/support/mock-openrouter.tsapps/e2e/tests/15-chat-fixture-smoke.spec.tsapps/e2e/vitest.config.tsapps/web/package.jsonapps/web/src/components/ai/chat/input/ChatTextarea.tsxapps/web/src/components/ai/chat/input/InputActions.tsxapps/web/src/components/ai/chat/layouts/ChatLayout.tsxapps/web/src/components/ai/page-agents/PageAgentHistoryTab.tsxapps/web/src/components/ai/shared/chat/ChatErrorBanner.tsxapps/web/src/components/ai/shared/chat/ChatMessagesArea.tsxapps/web/src/components/ai/shared/chat/CompactMessageRenderer.tsxapps/web/src/components/ai/shared/chat/MessageRenderer.tsxapps/web/src/components/ai/shared/chat/StreamingIndicator.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx
…ge export Two CodeRabbit findings, both confirmed real against the source. 1. seedConversationMessages set lastMessageAt to `startedAt + msgs.length * 1000` — one second PAST the last message, which is Date.now() however the messages were staggered. Every seeded conversation's lastMessageAt therefore collapsed to "now", destroying exactly the recency ordering the staggered timestamps exist to create, and diverging from seedChatConversation (which already sets it to the last message's own timestamp). Verified on a real DB: delta is now 0ms (was +1000), and two conversations seeded in sequence order deterministically by lastMessageAt. Also returns early on an empty msgs array — neither the old code nor the suggested fix handled it, and both would have written a lastMessageAt for a conversation with no messages (the suggested form: startedAt - 1000). 2. The consent constants reached into packages/lib/src/consent/consent-core. packages/lib exposes `./consent`, so this now imports @pagespace/lib/consent and declares @pagespace/lib in apps/e2e's devDependencies rather than relying on workspace hoisting for an undeclared dep. Note this deliberately diverges from the four neighbouring relative-src imports in global-setup.ts/support/db.ts: for e2e specifically the package export is the better target, because it resolves to the same BUILT module the app under test consumes, rather than to source the app never runs. Proven at runtime, not just by typecheck: the smoke spec still passes 3/3, and its send tests can only pass if the consent cookie still suppresses the banner — a broken import or wrong constant would put the banner back over the composer and hang the click. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Cross-PR check against #2089 (the in-flight global-assistant cutover) — clean, testids surviveThe epic body warns that 7.0b "should merge FIRST if the epic's rendering PRs are in flight, so its testids survive the cutover". #2089 ( Tested rather than assumed, via
So merge order does not matter between these two, and the epic's "merge 7.0b first" caution is satisfied either way — no coordination needed. This also closes 7.0b's central claim empirically against a second, independent cutover: the testids already survived #2079 (merged into this branch, verified — render path One thing for #2089's author to know rather than discover: |
…y review A /simplify pass (4 agents: reuse, simplification, efficiency, altitude) on the diff. Efficiency found nothing worth fixing — correctly judging the test-infra costs negligible and refuting a suspected closure leak. **Real bug, in my own earlier fix.** playwright.config.ts passed `storageState: './storageState.json'`, which Playwright hands straight to readFile WITHOUT resolving it against the config dir — so it is cwd-relative, while global-setup (since 2b0f6dc) writes the file next to itself. The two only agreed when started from apps/e2e. From the repo root — which I had documented as a supported invocation — every storageState-backed spec (02-08) died on `ENOENT: ./storageState.json`. My own root-invocation check missed it because spec 15 opts out of storageState. Proven before/after: spec 02 went from ENOENT to failing identically from both cwds on an unrelated pre-existing assertion. Anchored to __dirname; writer and reader now agree from any cwd. **Deleted the model-name triggers.** Both the simplification and altitude agents independently recommended it, and my earlier board note claiming they "cost nothing" was wrong. They cost: the `StreamMode | null` state (null existed ONLY to mean "defer to the model name"), the precedence ternary, two exported constants, a test that existed solely to pin the precedence between the two mechanisms, and the same "this path is unreachable from a real send" warning restated in five places to steer readers off an unusable door. Their only consumers were the support tests that tested them. The one theoretical benefit — per-request pacing — is unreachable by construction, since the app rewrites the model id. Now one mechanism, one canonical explanation; the support tests drive `mode` and send the model the app actually sends (openai/gpt-5.3-chat), so they exercise the same door a real send comes through. Also from the review: - consentCookie now BUILDS the decision with the app's own rejectNonEssential + serializeConsentState instead of hand-rolling the literal — the comment claimed "byte-identical to rejectNonEssential()" and "matches what the app writes"; now the code enforces both instead of asserting them. - http.ts imports StreamMode instead of re-spelling the union three times. - sendChatMessage takes a Locator, not `Page | Locator` — the doc said "SHOULD be a surface root"; now it is a compile error, not prose. - Smoke spec closes contexts in afterEach: a trailing `context.close()` is skipped when an assertion throws, leaking a context on every red run. - Test title said "slow-stream model" while the comment below explained the model is not what paces it. - Dropped `export` from in-file-only constants; cleanup() no longer resets a variable nothing reads. Kept, with reasons: seedConversationMessages (unused today, but 7.0c's spec requires it for the GlobalAssistantView specs); the session-cookie duplication with global-setup (extracting touches shared auth infra all specs depend on — regression risk outweighs a cosmetic DRY win). Support tests 14/14, smoke 3/3, typecheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
Two independent review passes flagged the same gap: `test:support` was a bespoke script name nothing invoked. CI runs `bunx turbo run test:coverage`, and apps/e2e had neither `test` nor `test:coverage` — so Turbo skipped the package entirely and the 14 tests guarding this PR's CRITICAL constraint (that the default mock path stays byte-identical, which metering specs 09-14 depend on) never executed anywhere but my terminal. A later edit to the dispatch could have perturbed the instant path and merged green. Renamed to the convention every other workspace already follows (`test` + `test:coverage`, the hook CI actually calls). No `--coverage`: there are no thresholds for harness code and coverage of a test mock is not the point — executing the guard is. I had deferred this as 7.6's scope. That was wrong: 7.6 is wiring the PLAYWRIGHT run, which needs a web app, a database and a browser. These are plain node HTTP tests — verified to pass with a completely empty environment (`env -i`), so nothing about CI can starve them. Verified with CI's own command: `turbo run test:coverage --filter='!@pagespace/processor'` now includes @pagespace/e2e in its graph, and `--filter='@pagespace/e2e'` runs 14/14 green in 704ms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Prerequisite leaves 7.0a, 7.0b, 7.0c of the "Deterministic Chat Rendering & Send" epic (PR 7 page — board
z9d17y0zh0jjqk4f4vsomld4). These are the harness the spec leaves 7.1-7.5 stand on; the spec leaves themselves wait for PR6 and are not in this PR.Three disjoint file scopes, one PR:
apps/e2e/support,apps/e2efixtures, anddata-testidattributes inapps/web.7.0a — controllable mock OpenRouter streams
The mock wrote one
pongchunk + usage +[DONE]synchronously and ended — the stream was over before Playwright could observe anything, making every mid-stream assertion in 7.1-7.5 impossible.Streams are now controllable by an explicit mode, set with
POST /__stream-config { mode }and honoured regardless of which model arrives:instant(default)slowheldPOST /__release-streamThe mode — not a pacing model name — is the only pacing control. The original design steered the mock by model name, which cannot work: the app rewrites any model outside its static catalog before the provider call, so
e2e/slow-streamnever reaches the mock. That is finding 1 under Verification below, and it is what 7.1-7.5 must build on.Plus
GET /__streams→{open, held, mode}(so specsexpect.polluntil a stream is actually live instead of sleeping),POST /__stream-configalso for pacing (chunks,intervalMs), and/__resetreleases open streams + restores defaults (includingmode: 'instant') so a held stream can never leak across specs. Abort-safe: every write is guarded onwritableEnded/destroyed, and the connection-level close clears the chunk timer, so Stop (7.4) can't crash the mock or pin the event loop pastserver.close().7.0b — data-testid instrumentation (pure additive)
Attributes only, no behavior change, so this can merge ahead of the epic's rendering PRs and its testids survive the cutover. 14 testids, each at exactly one production site.
Two board corrections, made on the 7.0b leaf before implementing (its table was written against worktree
wt-9vu2uxbg; this branch is master-based):CompactMessageRenderer(notMessageRenderer) plus its own inline list, "Thinking…" indicator and skeleton. The leaf's own example selector (sidebar-chat-tab→chat-message) could never have resolved from the files it listed, so those four sites are tagged too — otherwise 7.1/7.4's per-surface scoping would have been silently broken.isMessagesLoading. On master it isif (!isInitialized)alone; the term is already gone, replaced by the in-placeselectMessagesAreaModeindicator. Tagged the real gate. Consequence for 7.1: the load-bearing no-flash assertion for the sidebar ischat-loading-skeletonabsence, not the spinner.chat-layout-bodywas tagged — ChatLayout's AnimatePresence wrappers do survive on master. Note itskey={conversationId}makes a conversation switch an intentional remount, so 7.1's "same node after send" is only valid within one conversation.7.0c — chat fixtures
seedChatConversation/seedChatPage/seedConversationMessages(unifiedmessagestable) +authedContext(browser, sessionToken, baseURL)mirroring global-setup's exact cookie shape. Reuses the existingcreateChatMessage/createAgentPagefactories rather than new inserts. Chat specs must seed their own user: the sharedstorageStateuser is provideropenaiand would never reach the mock.Review fixes (79051ad)
Two P2 findings from Codex, both confirmed real against the source and fixed:
seedChatConversationnever created theconversationsrow.listConversations/countConversationsLEFT JOIN conversationsthen filterWHERE conv."userId" = $user OR conv."isShared" = true— with no row the join yields NULL, both predicates are NULL, and the seeded history is invisible, so the page would have opened a fresh empty conversation and every later spec would have silently tested the wrong thing. Now inserts the row with the same shape the app's owncreateConversationuses (type: 'page',contextId: pageId), withlastMessageAt/updatedAtfrom the last seeded message rather thannow.Proved against the repository's own SQL on a live Postgres —
0visible before,1after; both seeded conversations returned newest-first (B then A, B being what the page opens on); a different user still sees none, so the privacy filter is exercised rather than bypassed.The smoke spec's assistant-bubble locator matched nothing.
filter({ has: '[data-role=assistant]' })scopes to a descendant, butdata-rolesits on thechat-messageelement itself, so the slow-stream test would have timed out. Now an attribute selector on the element itself, with a comment recording why — 7.1-7.5 will select bubbles the same way.Open review threads — fixed, but deliberately left open
Three Codex threads are fixed and replied to, and left open rather than self-resolved so you can verify them yourself:
support/db.ts:188(theconversationsrow +lastMessageAt, above),tests/15-chat-fixture-smoke.spec.ts(the assistant-bubble locator, above), andfixtures/chat.fixture.ts. Each covers a real bug I shipped believing the code was correct, which is exactly the category a reviewer should confirm rather than take my word for.Verification — the suite RUNS now, and the smoke spec is green
tests/15-chat-fixture-smoke.spec.ts→ 3/3 passing in ~9s, in a real browser against a real production build: the seeded conversation renders, a slow-stream send grows an assistant bubble mid-stream, and a held stream keeps Stop up until released.Getting there required unblocking D.1 (
apps/webnow declarespg— one line; see below) and surfaced four real defects that no direct-to-mock test could ever have caught:resolveProviderModelrewrites any model outside the static catalog toDEFAULT_MODELbefore the provider call (ai-providers-config.ts:485-488), soe2e/slow-streamnever reached the mock; the app sentopenai/gpt-5.3-chat. Proven against a production build: the client POST carriesselectedModel: "e2e/held-stream"while the mock recordsopenai/gpt-5.3-chat. Fix: an explicitPOST /__stream-config { mode }, honoured regardless of the model that arrives — the mode is the only pacing control (a later /simplify pass deleted the vestigial model-name triggers: they were unreachable from any real send, yet forced a nullable state, a precedence ternary, a test that existed only to pin that precedence, and five separate "don't use this" comments). Default staysinstant,/__resetrestores it. 7.1-7.5 must set the mode, not seed a pacing model — recorded on the 7.0a leaf.fixed bottom-0 z-[100]): Playwright won't click through it, sochat-sendnever became actionable and the click hung to timeout.authedContextnow pre-records a necessary-only consent decision using the realCONSENT_*constants.ai-chat-viewis visible from server-rendered markup before React hydrates the composer, so an earlyfill()is discarded and Send never enables.sendChatMessageretries fill until Send enables — no sleep.contentsarray leaves a dangling user message, which reads as a turn in flight and disables the composer.Metering regression check — settled by A/B across the WHOLE suite, not by assertion. Specs 09-14 were run against the same app build twice: with this PR's mock, and with
origin/master's. Identical results both times — 8 passed / 8 failed, the same tests in each column. So this PR causes no regression. (13-metering-reconcilepasses, exercising the mock's/generationpath end-to-end. The 8 failures reproduce on master's own mock; filed as D.2, with09 pulse/cronadditionally proven order-dependent — it passes 3/3 in isolation.) The default mock path is separately byte-identical to master across all 8 request/endpoint cases.Also green:
bun run typecheck16/16,apps/e2esupport tests 14/14, and 924 web unit tests post-merge — including the newly-merged AiChatView cutover's own suites.pginapps/webdependenciespgis inserverExternalPackages, so the server bundle emits a barerequire('pg')resolved from next's own directory; bun's isolated store has no hoisted copy, so the app died at boot and no browser e2e could run at all — in this worktree and the main checkout. Declaringpginapps/webfixes it and is what makes the whole suite runnable. It is a product manifest change, so it is flagged rather than assumed; the alternatives are listed on D.1.Reviewer run recipe
Every invocation now works — two separate cwd bugs, both fixed.
bun run --filter '@pagespace/e2e' test:e2e(the documented command) previously could not work:global-setup/global-teardownderived their path fromprocess.cwd()while the reader (fixtures/seed-state.ts) was already__dirname-relative, so it wrote toapps/e2e/apps/e2e/and died on ENOENT before a single test; teardown failed more quietly, taking the "nothing to clean up" branch and silently leaking the seeded user and drive. Fixing that exposed a second one, caught by the /simplify pass:playwright.config.tspassedstorageState: './storageState.json', which Playwright hands straight toreadFilewithout resolving against the config dir — so from the repo root every storageState-backed spec (02-08) died onENOENT. (My own root check missed it: spec 15 opts out of storageState.) Both are now anchored to__dirname, and all three invocations work for all specs —--filter,cd apps/e2e && bun run test:e2e, and the root-relative binary.Use the workspace playwright binary, not
bunx playwright— bunx fetches a second copy and Playwright aborts with "two different versions of @playwright/test".Three environment traps remain, each of which silently breaks the suite:
next build+next start):buildCSPPolicynever emits'unsafe-eval'in any mode, so undernext devthe client throws a CSPEvalError, never hydrates, and every page sits at "Loading..." forever. API-only specs (09-14) don't notice; every UI spec dies.WEB_APP_URLmust exactly match the browsed origin —localhost≠127.0.0.1, else origin validation blocks every mutating request with403 ORIGIN_INVALID.page-content-store.ts(S3 only, no local mode), so with noAWS_*it 500s withCredentialsProviderError. Point it at a throwaway MinIO, not the real bucket.Plus: local
DATABASE_URL(postgresql://user:password@localhost:5433/pagespace_test— the.envhostpostgresdoesn't resolve from the host machine, and the container drifts stale, sodb:migrateit first),OPENROUTER_BASE_URL=http://127.0.0.1:4998/api/v1,OPENROUTER_DEFAULT_API_KEY=sk-e2e, andCRON_SECRET/STRIPE_WEBHOOK_SECRET/CSRF_SECRETshared with the Playwright process. Let Playwright start the mock — a hand-started one left from an older build serves stale code and produces baffling failures.The mock's support tests now run in CI (230bb49)
apps/e2egainedtest+test:coverage(vitest — no DB, no browser; they pass underenv -i), the convention every other workspace follows and the script CI'sturbo run test:coverageactually invokes. I had first deferred this to 7.6 and that was wrong: 7.6 is wiring the Playwright run, which needs a web app, a database and a browser. These 14 tests guard this PR's critical constraint — that the default mock path stays byte-identical, which metering specs 09-14 depend on — and until this commit they ran nowhere but my terminal, so a later edit to the dispatch could have perturbed the instant path and merged green.CI is green on this head commit: Lint & TypeScript Check, Unit Tests (now including
@pagespace/e2e), and CodeRabbit all pass; branch isCLEANand 0 commits behind master.🤖 Generated with Claude Code
https://claude.ai/code/session_017c9ywLBipnmS1TmsVvbdev