Repository navigation
test(missions): serve SSE in navigation fixtures - #1244
Conversation
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Verdict: Changes requested
Severity counts: P0: 0 | P1: 0 | P2: 1 | P3: 0
Reviewed head 735a394c7c9d3ac19803db5dab053ac9192f0bb2 against freshly fetched dev (abb14f83c6168b9eb04001359574e5d31ecc72d0).
[P2] Preserve the startup navigation budget for the new response wait
Location: packages/ui/tests/browser/mission-cross-navigation.test.ts:65-68 (especially line 66).
page.waitForResponse() uses Playwright's ordinary 30-second timeout, whereas armFixtureNavigationBudget(browser) only raises navigation timeouts to 120 seconds. Starting this wait alongside page.goto() makes every cross-navigation setup fail at 30 seconds if a cold fixture takes longer to reach its first EventSource request, even though the navigation remains within the explicitly supported startup budget. This reintroduces the cold-start failure that the shared navigation-budget helper documents and handles.
Reproduction: In an external scratch copy of this exact test, delay only the initial document route by 31 seconds, preserving the valid SSE response, real production imports and all assertions. The current code fails with page.waitForResponse: Timeout 30000ms exceeded while waiting for event "response". The same control with only { timeout: 120_000 } added to this response wait passes in approximately 49 seconds, including both network-failure probes. No reviewed worktree files were changed.
Fix: Give the initial SSE response wait the same explicit 120-second startup budget as the navigation (or share that existing budget), while retaining its predicate and strict network assertions.
Transport and regression assessment
- The SSE correction itself is sound. Both fixtures previously let
/api/eventsreach the JSON fallback. The actual singleton lifecycle isserverEvents->connectWorkspaceEvents->serverApi.connectEvents-> nativeEventSource; MIME rejection reachesattachEventSourceHandlers, then the existing disconnect/reconnect path closes the source. Before testing the corrected head, removing only the SSE branch in a scratch copy makes the new regression fail deterministically withapplication/json !== text/event-streamand Chromium's exact MIME-rejection diagnostic. The precise historical intermittentERR_ABORTEDwas not reproduced by this contract control. - Both changed routes return finite SSE comments through
route.fulfill, matching established fixtures and leaving no held server response. Test pages/contexts own the production source and reconnect timers; the additional failing EventSource explicitly closes itself. Held navigation routes are released before page cleanup, and the owned-server teardown checks remain intact. - The new regression uses the real shell's EventSource response and actual browser
requestfailedevents. Its unrelated aborted fetch producesnet::ERR_ABORTED, and its native EventSource failure producesnet::ERR_FAILED. The existing navigation state/write-count assertions, native session dispatcher, and narrow pre-existing cancellation exceptions are unchanged; no broad network-error suppression was added.
Validation
node --import tsx --test --test-concurrency=1 tests/browser/mission-cross-navigation.test.ts tests/browser/mission-navigation.test.ts: three runs, 37/37 each (111/111 passed) on the unchanged reviewed head.- Additional scratch custody control: the actual SSE response's
finished()resolves without error, and the owned browser context count is 0 after page cleanup; both strict network probes still pass. fixture-wiring.test.ts: 1/1 passed.fixture-navigation-budget.test.ts: 3/3 passed.fixture-shutdown.test.ts: 7/7 passed, including actual failed/partial requests, drained optimizer contexts and owned-cache cleanup.event-source-handlers.test.tsandserver-events-lifecycle.test.ts: 6/6 passed.- UI
npm run typecheck --workspace @codenomad/ui: passed. - Server
npx tsc --noEmit -p tsconfig.json: passed. git diff --check: passed. Branch/head and worktree cleanliness preserved. Windows, Node25.2.1, Chromium153.0.8010.12.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Verdict: Approve
Severity counts: P0: 0 | P1: 0 | P2: 0 | P3: 0
Reviewed exact head 0e5820489aeda482369157eaf530e63e26a6fcb1 against latest fetched dev (abb14f83c6168b9eb04001359574e5d31ecc72d0) and the previously reviewed head. No new defects found.
Previous P2: closed
packages/ui/tests/browser/mission-cross-navigation.test.ts:66 now gives the initial SSE response wait an explicit timeout: 120_000, matching the existing navigation startup budget. This is the only change since the first review. The response predicate, navigation assertions and strict network-error collectors remain unchanged.
I regenerated the delayed-start control from this exact head in an external scratch file: delaying only the initial document route by 31 seconds now passes the targeted regression in approximately 54 seconds, including detection of both unrelated net::ERR_ABORTED and genuine EventSource net::ERR_FAILED. The corresponding control in the first review failed at the ordinary 30-second response timeout. This verifies closure of the finding without changing the reviewed worktree.
Validation
- Both affected suites together: 37/37 passed, including the targeted SSE/network regression, navigation ownership checks and owned-server cleanup assertions.
- Delayed-start targeted regression: 1/1 passed.
- Fixture wiring and navigation-budget checks: 4/4 passed.
- UI typecheck: passed.
git diff --check: passed.
The SSE fixture correction and its strict failure-detection coverage remain sound. Branch fix/mission-navigation-cancellation, exact head and worktree cleanliness were preserved; no repository edits, commits, pushes, merge or shared-daemon operations were performed. Validation used Windows and Chromium 153.0.8010.12.
0e58204 to
2bc24cd
Compare
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Verdict: Approve — P0: 0 | P1: 0 | P2: 0 | P3: 0
Evidence: 151/151 full-suite checks passed, plus 4/4 targeted cancellation controls, a delayed-start control and a streaming-body control. Expected-negative controls rejected the raw cancellation collector, malformed SSE MIME type and removal of exact request binding.
Reviewed exact head a4fd2e0177b34d8a141c45043120abaec48c6b72 against freshly fetched/current dev (186d8fc6fa1e979c531af1a5531f039eaafc2b4a). No actionable defects found.
Ownership and failure detection
- Traced the production path through
RightPanel->usePanelExtensions->panelExtensionsApi.list->authenticatedFetchandresponse.json(). Connected status refreshes supersede the previous controller-owned read; disconnect and disposal abort that same demand. Production behavior is unchanged. - The shared helper requires the exact GET URL,
net::ERR_ABORTED, a fixture identity bound to its first PlaywrightRequest, and that call's owning aborted signal plus nativeDOMException/AbortErrorrejection. Missing/unknown identities fail their receipt check; another request reusing an identity cannot match its boundRequest. Every raw failure is retained, and pending classifications are drained before assertions. - Reproduced all three forced native-SSE-open cases with an unconditional raw-failure collector in an external scratch copy: all three fail the existing empty-network-errors assertion. The unchanged head passes those cases and the genuine same-URL, missing/unknown/reused identity rejection test: 4/4. Removing only the exact
Requestbinding in a scratch helper makes the reused-identity assertion fail, verifying that guard is exercised. - A separate scratch control used the actual shell/controller, shared observer and a real HTTP server returning partial JSON. After confirming
Response.json()had started, controller supersession producednet::ERR_ABORTED, a true owning receipt and exactly one classified cancellation. Destroying a subsequent partial response instead producednet::ERR_INCOMPLETE_CHUNKED_ENCODING, a false cancellation receipt and a retained failure. The owned context closed and the fixture HTTP server was closed afterward. - Observation state and listeners are scoped to each fixture page; the temporary fetch wrapper is restored synchronously in
finally, including while body consumption remains pending. Held routes are released during cleanup. The shared helper and its window bridge are confined totests/browser; production source exposes no new test API.
SSE, navigation and rebase
- Both navigation fixtures still provide valid finite SSE comments, and the initial response wait retains its explicit 120-second budget. A scratch JSON-response control fails the SSE MIME assertion; a 31-second delayed-document control passes the real regression in approximately 50 seconds, including unrelated
ERR_ABORTEDand genuine EventSourceERR_FAILEDdetection. - Real components/native session dispatch remain in use. Existing navigation state/write-count, selected-tab and page-error assertions are preserved. The new panel-read classification is shared with native-family rather than duplicated; existing unrelated cancellation exceptions remain unchanged.
- The documentation conflict retained both the computed-Monaco-diff readiness guidance and the SSE convention. The Git-history suite, including deferred worker-result/native-selection coverage, passes.
Commands and results
From packages/ui, with serial browser execution:
- Navigation suites (
mission-cross-navigation.test.ts,mission-navigation.test.ts): three runs, 38/38 each — 114/114. - Native-family plus fixture wiring/navigation-budget checks: 16/16.
- Git-history: 21/21.
- Targeted catalog/native-SSE-open controls: 4/4.
- Additional delayed-start and streaming-body scratch controls: 2/2 after preparing the scratch harness.
- UI and server workspace typechecks: passed.
git diff --check: passed.
Branch fix/mission-navigation-cancellation, exact head and worktree cleanliness are preserved. No repository edits, commits, pushes, merge, CI actions or shared OpenCode daemon operations were performed. Scratch controls and logs are outside the checkout. Validation used Windows / Chromium 153.0.8010.12.
Oversized modified files: none under the repository's source/test thresholds. The largest modified test is packages/ui/tests/browser/mission-native-family.test.ts, approximately 645 lines; the new shared helper is 58 lines.
a4fd2e0 to
9776aed
Compare
|
Rebased onto latest dev
|
The navigation fixtures import the production event transport but let /api/events fall through to their generic JSON response. Chromium rejects that MIME type and the normal reconnect handler closes the source; a body cancellation can then appear as an unrelated navigation network failure. The same malformed response is present on dev and PR #1217, whose reported assertion runs before page cleanup. Use the existing finite SSE-comment fixture convention in both navigation setups. Observe the actual native EventSource response during setup and check its status and MIME type in a real-shell browser regression. Keep the network collector unchanged and prove that an unrelated ERR_ABORTED fetch and a genuinely failed EventSource remain recorded. Native session frames continue through the real dispatcher; production behavior is intact. The new contract assertion fails against the old application/json stub. Both navigation suites pass three consecutive serial runs (37 tests each), including captured unchanged session/preview state and empty failure lists. Fixture-wiring, UI and server typechecks and diff whitespace checks pass.
Give the initial native EventSource response wait the same explicit 120-second budget as fixture navigation. Playwright action waits otherwise stop setup at 30 seconds even when a cold document is still within the supported navigation budget. Keep the exact response predicate, strict error assertions and post-startup failure-probe timeouts unchanged; the sibling navigation fixture adds no response wait. An isolated 31-second delayed-document control fails with the old 30000ms response timeout and passes with this one-line change in about 46 seconds, including both network probes. Both navigation suites, navigation-budget and fixture-wiring tests pass together (41/41), UI typecheck passes, and diff whitespace checks pass. The diagnostic control is temporary; no sleeps, retries or error exclusions are added.
The cross-navigation shell imports the real panel-extension controller. Its SSE connection supersedes an optional pending catalog read, so treating every ERR_ABORTED as a network defect fails the reader session-ABA and preview navigation checks before page cleanup. A held production read followed by an actual native EventSource open reproduces all three failures deterministically. Extract the native-family fixture's existing request-identity, owning aborted signal and native DOMException AbortError observation into one shared fixture module. Both collectors retain every raw failure and separately record only verified catalog cancellations. Bind each identity to its first Playwright Request so another request reusing a valid cancelled identity remains a failure. Production transport, controller behavior and navigation ownership are intact. Gate the initial SSE response in the three affected reader paths, hold the controller-owned catalog request, then open the real stream and prove its cancellation receipt alongside the original state/write-count assertions. Negative controls retain same-URL network aborts, missing and unknown identities, reused valid identities, unrelated aborted fetches and failed EventSources. Drain receipt classifications before asserting; serve the real list's empty array contract. Document the shared fixture convention without public UI changes. Validation on Windows / Chromium 153: deterministic red controls fail all three navigation assertions without classification; green targeted controls pass 4/4. Both full navigation suites pass three serial runs at 38/38 each. Native-family passes 12/12, Git-history 21/21, fixture wiring 1/1 and navigation budget 3/3. UI and server typechecks and whitespace checks pass. Thirty captured navigation records preserve identical before/after state, including nine forced SSE-open cancellations with retained raw failures and zero unexpected network/page errors.
9776aed to
1101d18
Compare
|
Rebased onto latest dev No conflicts. All three approved commits remain equivalent ( Fresh targeted serial browser validation passed 54/54: cross-navigation 14/14, navigation 24/24, native-family 12/12, fixture wiring 1/1, navigation budget 3/3. UI and server workspace typechecks and whitespace checks passed. Application code and assertions were untouched; working tree clean. Pushed with the exact force lease against |
Summary
text/event-streamresponses in both Mission navigation fixtures instead of the generic JSON fallback; preserve the bounded 120-second EventSource readiness wait.tests/browser/panel-read-fixture.ts. Retain every raw failure and separately classify only the exact production request's owning aborted signal plus nativeDOMException/AbortErrorreceipt.Root cause and justified follow-up
The original fixtures imported the production event transport but replied to
/api/eventswith200 application/jsonand{}. Chromium rejects that MIME type; the existing error handler closes the rejected EventSource and schedules reconnection. The SSE contract regression deterministically fails against the JSON stub and passes with the valid SSE comment response. Native session frames continue through the real dispatcher.Both approved commits were rebased onto dev
186d8fc6f, preserving #1246/#1249 bounded Mission reads, #1247's native-family cancellation correction and #1248's Monaco readiness fix. The only conflict was adjacent guidance indev-docs/BROWSER_TESTING.md; both the Monaco readiness and SSE bullets were retained.The initial rebased head
2bc24cd79001a77ff679dd5d56bcb62b1a0f5925exposed three cross-navigation failures on/api/panel-extensions net::ERR_ABORTED: readersession-aba,previewandpreview-aba. TracingRightPanel->usePanelExtensions->panelExtensionsApi.listconfirmed the lifecycle handled by #1247:refresh()aborts the previous controller-owned list read when SSE reports connected; disconnect/visibility cleanup also retires demand. #1247's instrumentation was confined to native-family, leaving this PR's touched cross-navigation fixture without exact receipt handling.This additional fixture correction is included because those failures blocked PR validation. It extracts and reuses the existing mechanism instead of duplicating another collector. Production transport, controller behavior and navigation ownership are unchanged.
Deterministic regression evidence
/api/panel-extensions net::ERR_ABORTED, before page cleanup.ERR_ABORTEDand failed-EventSource controls also pass.Validation at
a4fd2e0177b34d8a141c45043120abaec48c6b72Windows / Chromium 153, serial browser execution, no automatic retries.
From
packages/ui:Three complete runs, 38/38 each: cross-navigation 14/14 and navigation 24/24; 114 passing checks.
37/37: native-family 12/12, Git-history 21/21, fixture wiring 1/1 and navigation budget 3/3. Includes #1247's existing cancellation controls and #1248's deferred computed-diff/native-selection regression.
npm run typecheck --workspace @codenomad/ui— pass.npm run typecheck --workspace @neuralnomads/codenomad— pass.git diff --check— pass..codenomad/validation/pr1244/run-1,run-2,run-3in the dedicated worktree; all 30 JSON records independently checked for identical before/after state and empty page/unexpected-network-error arrays.Total full-suite validation: 151/151 passing checks, plus 4/4 targeted green controls. Existing assertions and timing budgets retained; no sleeps or retries added. Console subprocesses hidden. No production behavior or public help changes. Pushed with the exact lease against
2bc24cd79001a77ff679dd5d56bcb62b1a0f5925; no merge, review, CI manipulation or shared OpenCode daemon changes.