fix: streaming race conditions (#631) + blank-page workspace binding (#804)

Closes #631. Closes #804. Bug A (thinking card below answer / double render / stuck cursor): trailing rAF after 'done' inserted a duplicate live-turn wrapper into already-settled DOM. Fixed via _streamFinalized flag + cancelAnimationFrame in all terminal handlers (done/apperror/cancel/_handleStreamError) + _scheduleRender guard. All three reported symptoms were the same root cause. Bug B (accumulator reset): original fix reset assistantText/reasoningText inside _wireSSE on reconnect. Reverted — server uses one-shot queue.Queue(), no replay on reconnect, reset would wipe valid pre-drop content causing data loss. Bug A fix alone resolves all symptoms. #804 (blank page workspace): syncWorkspaceDisplays uses S._profileDefaultWorkspace as fallback; workspace chip enabled when hasWorkspace (not hasSession); promptNewFile/promptNewFolder/ switchToWorkspace/promptWorkspacePath auto-create session on blank page; boot.js hydrates _profileDefaultWorkspace from /api/settings before any session exists. Opus max-effort review + Nathan independent review + full browser QA. 1765/1765 tests.
2026-04-21 18:47:40 -07:00
parent c3807482be
commit 859602340e
9 changed files with 431 additions and 11 deletions
--- a/tests/test_regressions.py
+++ b/tests/test_regressions.py
@@ -433,7 +433,7 @@ def test_done_handler_sets_busy_false_before_renderMessages(cleanup_test_session
    if done_idx < 0:
        done_idx = src.find("es.addEventListener('done'")
    assert done_idx >= 0
-    done_block = src[done_idx:done_idx+2500]
+    done_block = src[done_idx:done_idx+2900]
    # S.busy=false must appear before renderMessages() within the done handler
    busy_pos = done_block.find("S.busy=false;")
    render_pos = done_block.find("renderMessages()")
--- a/tests/test_sprint36.py
+++ b/tests/test_sprint36.py
@@ -162,7 +162,7 @@ def test_sse_cancel_handler_calls_set_busy():
    if idx == -1:
        idx = src.find('addEventListener("cancel"')
    assert idx != -1
-    block = src[idx:idx + 1000]
+    block = src[idx:idx + 1200]
    assert "setBusy(false)" in block, (
        "SSE cancel handler no longer calls setBusy(false)"
    )
--- a/tests/test_streaming_race_fix.py
+++ b/tests/test_streaming_race_fix.py
@@ -0,0 +1,175 @@
+"""Tests for #631 — streaming race conditions in messages.js
+
+Bug A: A trailing 'token'/'reasoning' event queued a requestAnimationFrame that
+fired after 'done' had already called renderMessages(), causing the thinking card
+to reappear below the final answer or the response to render twice.
+
+Bug B: On SSE reconnect, the closure variables (assistantText, reasoningText)
+were not reset. Server replays token events into the new EventSource, causing
+text to accumulate again from the stale values — response doubled, stuck cursor.
+
+Fixes:
+- _streamFinalized flag + _pendingRafHandle stored for cancellation
+- done/apperror/cancel: set _streamFinalized, cancel pending rAF, call finalizeThinkingCard
+- _scheduleRender: guard on _streamFinalized
+- _wireSSE: reset accumulators when (re)opening source, unless stream already finalized
+- error handler: bail if _streamFinalized (same as _terminalStateReached)
+"""
+import pathlib
+import re
+
+REPO = pathlib.Path(__file__).parent.parent
+
+
+def read(rel):
+    return (REPO / rel).read_text(encoding='utf-8')
+
+
+class TestStreamFinalized:
+    """_streamFinalized flag and rAF cancellation."""
+
+    def test_stream_finalized_declared(self):
+        src = read('static/messages.js')
+        assert '_streamFinalized' in src, (
+            "_streamFinalized must be declared in attachLiveStream"
+        )
+
+    def test_pending_raf_handle_declared(self):
+        src = read('static/messages.js')
+        assert '_pendingRafHandle' in src, (
+            "_pendingRafHandle must be declared to enable rAF cancellation"
+        )
+
+    def test_schedule_render_guards_on_stream_finalized(self):
+        src = read('static/messages.js')
+        m = re.search(r'function _scheduleRender\(\)\{.*?\n  \}', src, re.DOTALL)
+        assert m, "_scheduleRender not found"
+        fn = m.group(0)
+        assert '_streamFinalized' in fn, (
+            "_scheduleRender must return early when _streamFinalized is true"
+        )
+
+    def test_raf_handle_stored_in_schedule_render(self):
+        src = read('static/messages.js')
+        assert '_pendingRafHandle=requestAnimationFrame' in src or \
+               '_pendingRafHandle = requestAnimationFrame' in src, (
+            "rAF handle must be stored in _pendingRafHandle for cancellation"
+        )
+
+    def test_done_sets_stream_finalized(self):
+        src = read('static/messages.js')
+        m = re.search(r"source\.addEventListener\('done'.*?\}\);", src, re.DOTALL)
+        assert m, "'done' handler not found"
+        fn = m.group(0)
+        assert '_streamFinalized=true' in fn or '_streamFinalized = true' in fn, (
+            "'done' handler must set _streamFinalized=true"
+        )
+        assert 'cancelAnimationFrame' in fn, (
+            "'done' handler must cancel any pending rAF"
+        )
+        assert 'finalizeThinkingCard' in fn, (
+            "'done' handler must call finalizeThinkingCard() to close thinking card"
+        )
+
+    def test_apperror_sets_stream_finalized(self):
+        src = read('static/messages.js')
+        m = re.search(r"source\.addEventListener\('apperror'.*?\}\);", src, re.DOTALL)
+        assert m, "'apperror' handler not found"
+        fn = m.group(0)
+        assert '_streamFinalized=true' in fn or '_streamFinalized = true' in fn, (
+            "'apperror' handler must set _streamFinalized=true"
+        )
+        assert 'cancelAnimationFrame' in fn
+
+    def test_cancel_sets_stream_finalized(self):
+        src = read('static/messages.js')
+        m = re.search(r"source\.addEventListener\('cancel'.*?\}\);", src, re.DOTALL)
+        assert m, "'cancel' handler not found"
+        fn = m.group(0)
+        assert '_streamFinalized=true' in fn or '_streamFinalized = true' in fn, (
+            "'cancel' handler must set _streamFinalized=true"
+        )
+        assert 'cancelAnimationFrame' in fn
+
+
+class TestReconnectAccumulatorPreservation:
+    """Bug B regression guard: the accumulators must NOT be reset on reconnect.
+
+    The original PR description claimed the server "replays buffered token
+    events" on SSE reconnect, and proposed resetting `assistantText` /
+    `reasoningText` inside `_wireSSE` to absorb that replay.  That is not
+    how the server actually works — `api/routes._handle_sse_stream` reads
+    a one-shot `queue.Queue()` that delivers each event to exactly one
+    consumer.  When a client reconnects with the same `stream_id`, it
+    picks up from the queue's current position; already-delivered tokens
+    are NOT re-sent.  Resetting the accumulators on reconnect would wipe
+    the already-displayed content and restart the response from the first
+    post-reconnect token — a data-loss regression.
+
+    The "doubled response" / "stuck cursor" symptom that originally
+    motivated the reset is fully explained by Bug A (trailing rAF after
+    `done` inserting a duplicate live-turn wrapper).  The Bug A fix
+    (_streamFinalized guard + cancelAnimationFrame in terminal handlers)
+    resolves both symptoms without needing a reset.
+    """
+
+    def test_wire_sse_does_not_reset_accumulators(self):
+        """Regression guard: _wireSSE must not contain a literal
+        accumulator-reset statement.  Preserves pre-reconnect content so
+        the user sees the full response across a drop+reconnect."""
+        src = read('static/messages.js')
+        m = re.search(r'function _wireSSE\(source\)\{.*?\n  \}', src, re.DOTALL)
+        assert m, "_wireSSE not found"
+        fn = m.group(0)
+        assert "assistantText=''" not in fn and 'assistantText = ""' not in fn, (
+            "_wireSSE must NOT reset assistantText — the server does not replay "
+            "events on reconnect, so the reset would wipe valid pre-drop content"
+        )
+        assert "reasoningText=''" not in fn and 'reasoningText = ""' not in fn, (
+            "_wireSSE must NOT reset reasoningText on reconnect"
+        )
+
+    def test_closure_initialises_accumulators_empty(self):
+        """Initial-connect safety: accumulators are initialised to empty at
+        the closure scope in attachLiveStream, not inside _wireSSE.  That
+        covers the first call; reconnects must preserve whatever was
+        accumulated before the drop."""
+        src = read('static/messages.js')
+        m = re.search(
+            r'function attachLiveStream\(.*?function _closeSource',
+            src,
+            re.DOTALL,
+        )
+        assert m, "attachLiveStream prelude not found"
+        prelude = m.group(0)
+        assert "let assistantText=''" in prelude or 'let assistantText = ""' in prelude, (
+            "assistantText must be initialised to '' at closure scope — "
+            "this is the only legitimate reset; _wireSSE must not re-reset"
+        )
+
+    def test_error_handler_guards_on_stream_finalized(self):
+        """`error` must still bail out when `_streamFinalized` is true —
+        otherwise a trailing network 'error' event after `done` would
+        attempt a reconnect against a stream that already completed."""
+        src = read('static/messages.js')
+        m = re.search(r"source\.addEventListener\('error'.*?\}\);", src, re.DOTALL)
+        assert m, "'error' handler not found"
+        fn = m.group(0)
+        assert '_streamFinalized' in fn, (
+            "'error' reconnect handler must bail if _streamFinalized is true"
+        )
+
+    def test_handle_stream_error_sets_stream_finalized(self):
+        """Opus review Q1: _handleStreamError is called after the reconnect fails.
+        It calls renderMessages() which settles the DOM. Any pending rAF must be
+        cancelled before that renderMessages call — same as done/apperror/cancel."""
+        src = read('static/messages.js')
+        m = re.search(r'function _handleStreamError\(\)\{.*?\n  \}', src, re.DOTALL)
+        assert m, "_handleStreamError not found"
+        fn = m.group(0)
+        assert '_streamFinalized=true' in fn or '_streamFinalized = true' in fn, (
+            "_handleStreamError must set _streamFinalized=true (Opus Q1 fix)"
+        )
+        assert 'cancelAnimationFrame' in fn, (
+            "_handleStreamError must cancel any pending rAF before renderMessages() runs"
+        )
--- a/tests/test_workspace_blank_page_fix.py
+++ b/tests/test_workspace_blank_page_fix.py
@@ -0,0 +1,152 @@
+"""Tests for #804 — blank new-chat page loses default workspace binding
+
+Fixes:
+- syncWorkspaceDisplays() uses S._profileDefaultWorkspace as fallback when no session
+- composerChip.disabled uses hasWorkspace (not hasSession) so chip is enabled on blank page
+- boot.js reads default_workspace from /api/settings and sets S._profileDefaultWorkspace
+- promptNewFile/promptNewFolder auto-create a session bound to default workspace
+"""
+import pathlib
+import re
+
+REPO = pathlib.Path(__file__).parent.parent
+
+
+def read(rel):
+    return (REPO / rel).read_text(encoding='utf-8')
+
+
+class TestSyncWorkspaceDisplaysFallback:
+    """syncWorkspaceDisplays must show default workspace when no session."""
+
+    def test_uses_profile_default_workspace_as_fallback(self):
+        src = read('static/panels.js')
+        m = re.search(r'function syncWorkspaceDisplays\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m, "syncWorkspaceDisplays not found"
+        fn = m.group(0)
+        assert '_profileDefaultWorkspace' in fn, (
+            "syncWorkspaceDisplays must read S._profileDefaultWorkspace as fallback "
+            "when no active session is present"
+        )
+
+    def test_has_workspace_not_has_session_for_chip_disable(self):
+        src = read('static/panels.js')
+        m = re.search(r'function syncWorkspaceDisplays\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m
+        fn = m.group(0)
+        # composerChip.disabled must use hasWorkspace, not hasSession
+        assert 'composerChip.disabled=!hasWorkspace' in fn or \
+               'composerChip.disabled = !hasWorkspace' in fn, (
+            "composerChip.disabled must use !hasWorkspace (not !hasSession) so the chip "
+            "is enabled on the blank new-chat page when a default workspace is configured"
+        )
+        assert 'composerChip.disabled=!hasSession' not in fn, (
+            "composerChip.disabled must not use !hasSession — this was the regression"
+        )
+
+
+class TestBootJsProfileDefaultWorkspace:
+    """boot.js must read default_workspace from /api/settings into S._profileDefaultWorkspace."""
+
+    def test_boot_reads_default_workspace_from_settings(self):
+        src = read('static/boot.js')
+        assert '_profileDefaultWorkspace' in src, (
+            "boot.js must set S._profileDefaultWorkspace from the /api/settings "
+            "default_workspace field so it is available before any session is created"
+        )
+
+    def test_boot_sets_profile_default_workspace_in_settings_block(self):
+        """The settings block (lines ~758-800 in boot.js) must set
+        S._profileDefaultWorkspace from the /api/settings response."""
+        src = read('static/boot.js')
+        # Find the settings fetch and the _profileDefaultWorkspace assignment
+        # and confirm both are in the same settings-read block (within ~50 lines)
+        ws_idx = src.find('_profileDefaultWorkspace')
+        settings_idx = src.find("await api('/api/settings')")
+        assert ws_idx != -1, "_profileDefaultWorkspace not found in boot.js"
+        assert settings_idx != -1, "await api('/api/settings') not found in boot.js"
+        # Both must be within 300 chars of each other (same block)
+        assert abs(ws_idx - settings_idx) < 1000, (
+            "S._profileDefaultWorkspace must be set in the same settings-fetch block"
+        )
+
+
+class TestPromptNewFileNoSession:
+    """promptNewFile/promptNewFolder must auto-create a session on blank page."""
+
+    def test_prompt_new_file_auto_creates_session(self):
+        src = read('static/ui.js')
+        m = re.search(r'async function promptNewFile\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m, "promptNewFile not found"
+        fn = m.group(0)
+        # Must have auto-create path (not just early return when no session)
+        assert '_profileDefaultWorkspace' in fn, (
+            "promptNewFile must read S._profileDefaultWorkspace to auto-create "
+            "a session when called on the blank new-chat page"
+        )
+        assert 'session/new' in fn, (
+            "promptNewFile must call /api/session/new to create a session "
+            "bound to the default workspace when S.session is null"
+        )
+
+    def test_prompt_new_folder_auto_creates_session(self):
+        src = read('static/ui.js')
+        m = re.search(r'async function promptNewFolder\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m, "promptNewFolder not found"
+        fn = m.group(0)
+        assert '_profileDefaultWorkspace' in fn, (
+            "promptNewFolder must read S._profileDefaultWorkspace for auto-create path"
+        )
+        assert 'session/new' in fn, (
+            "promptNewFolder must call /api/session/new to create session on blank page"
+        )
+
+    def test_prompt_new_file_still_returns_early_without_default(self):
+        """If no default workspace, the function should return early (not crash)."""
+        src = read('static/ui.js')
+        m = re.search(r'async function promptNewFile\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m
+        fn = m.group(0)
+        # Must have a guard for empty workspace
+        assert "if(!ws) return" in fn or "if(!ws)return" in fn, (
+            "promptNewFile must return early if no default workspace is configured"
+        )
+
+
+class TestWorkspaceSwitcherBlankPage:
+    """Opus review Q6: workspace switcher dropdown must not silently fail on blank page."""
+
+    def test_switch_to_workspace_auto_creates_session(self):
+        src = read('static/panels.js')
+        m = re.search(r'async function switchToWorkspace\(.*?\n\}', src, re.DOTALL)
+        assert m, "switchToWorkspace not found"
+        fn = m.group(0)
+        assert '_profileDefaultWorkspace' in fn or 'session/new' in fn, (
+            "switchToWorkspace must auto-create session on blank page (Opus Q6 fix)"
+        )
+        assert 'session/new' in fn, (
+            "switchToWorkspace must call /api/session/new when S.session is null"
+        )
+
+    def test_prompt_workspace_path_auto_creates_session(self):
+        src = read('static/panels.js')
+        m = re.search(r'async function promptWorkspacePath\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m, "promptWorkspacePath not found"
+        fn = m.group(0)
+        assert 'session/new' in fn, (
+            "promptWorkspacePath must call /api/session/new when S.session is null"
+        )
+
+    def test_sync_workspace_displays_dropdown_close_uses_has_workspace(self):
+        src = read('static/panels.js')
+        m = re.search(r'function syncWorkspaceDisplays\(\)\{.*?\n\}', src, re.DOTALL)
+        assert m, "syncWorkspaceDisplays not found"
+        fn = m.group(0)
+        # Line 555: dropdown force-close must use hasWorkspace, not hasSession
+        assert '!hasWorkspace && composerDropdown' in fn or '!hasWorkspace&&composerDropdown' in fn, (
+            "syncWorkspaceDisplays must use !hasWorkspace (not !hasSession) to decide "
+            "whether to force-close the dropdown (Opus Q6 fix)"
+        )
+        assert '!hasSession && composerDropdown' not in fn, (
+            "Regression guard: !hasSession for dropdown close must be removed"
+        )