From 75d5034f766308b04545d5d837c1e6e392fa64ce Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:13 -0700 Subject: [PATCH 1/8] Check what a failed answer save leaves behind, not just how often spend is recorded --- CHANGELOG.md | 3 +++ tests/test_chat_stream.py | 27 ++++++++++++++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5c7917..cc8d877 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,9 @@ ### Fixed +- The test of a streamed answer whose save fails to commit now runs the real + spend recording. It checks that the day's total is the spend once (not 0, + not doubled) and that no answer or citation is left behind. - **A streamed answer's spend survives a failed save.** Spend is marked as recorded only after its commit succeeds. Before, a rolled-back commit took the spend with it, and it was never recorded again. diff --git a/tests/test_chat_stream.py b/tests/test_chat_stream.py index 195da7e..6c5a692 100644 --- a/tests/test_chat_stream.py +++ b/tests/test_chat_stream.py @@ -6,13 +6,17 @@ import flask import openai import pytest +from sqlalchemy import func, select from sqlalchemy.orm import Session from src.api import deps -from src.models.chat import MessageSource +from src.models.chat import Message, MessageSource +from src.models.user import User from src.services.generation import Answer, Delta, Done, Notice, Reset, Stage from src.services.passages import content_hash from src.services.retrieval import RetrievedChunk +from src.services.usage import record_tokens as real_record +from src.services.usage import tokens_used_today from tests.test_chat import _auth_headers CHUNKS = [RetrievedChunk("c1", "A deductible is...", "wiki_Health.txt", 0.9)] @@ -258,24 +262,33 @@ def source(*_args, **_kwargs): def test_spend_is_recorded_again_when_saving_the_answer_fails_to_commit(client): """The commit that saves the answer also carries the spend. If it fails, - the rollback undoes both, and the spend must still be recorded.""" + the rollback undoes both: the spend is recorded again, once, and nothing + of the answer is left behind.""" headers, thread_id = _thread(client, "stream-commit@example.com") real_commit = Session.commit - state = {"recorded": 0, "failed": False} + state = {"spends": [], "failed": False} - def record(*_args): - state["recorded"] += 1 + def record(db, user_id, tokens): + state["spends"].append(tokens) + real_record(db, user_id, tokens) def flaky_commit(session): - if state["recorded"] == 1 and not state["failed"]: + if len(state["spends"]) == 1 and not state["failed"]: state["failed"] = True raise RuntimeError("commit failed") return real_commit(session) with patch("src.api.routes.chat.answer_query_events", return_value=iter([Done(Answer("A.", CHUNKS))])), \ + patch("src.api.routes.chat.token_usage", return_value=21), \ patch("src.api.routes.chat.record_tokens", side_effect=record), \ patch.object(Session, "commit", flaky_commit): resp = client.post(_url(thread_id), json={"content": "x"}, headers=headers, buffered=True) assert _events(resp)[-1] == ("error", {"error": "generation failed"}) - assert state == {"recorded": 2, "failed": True} + assert state == {"spends": [21, 21], "failed": True} + db = deps.SessionLocal() + user_id = db.scalar(select(User.id).where(User.email == "stream-commit@example.com")) + assert tokens_used_today(db, user_id) == 21 # the rolled-back 21 is gone, not doubled + assert [m["role"] for m in _saved(client, headers, thread_id)] == ["user"] + left = select(func.count()).select_from(MessageSource).join(Message).where(Message.thread_id == uuid.UUID(thread_id)) + assert db.scalar(left) == 0 From 8ea47ef925646b6f601f151c1dd2bf6c6c3d067d Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:23 -0700 Subject: [PATCH 2/8] Keep a citation's seal number when its answer is reloaded --- CHANGELOG.md | 4 ++++ src/api/routes/chat.py | 4 +++- src/models/chat.py | 9 ++++++++- tests/test_chat_stream.py | 40 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc8d877..373c292 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,10 @@ ### Fixed +- **A citation keeps its seal number after a reload.** A streamed answer + listed its sources in retrieval order, and a reload in relevance order, so + the numbers could change. Both now list them most relevant first, ties by + chunk and label compared byte-wise (`COLLATE "C"` in SQL). - The test of a streamed answer whose save fails to commit now runs the real spend recording. It checks that the day's total is the spend once (not 0, not doubled) and that no answer or citation is left behind. diff --git a/src/api/routes/chat.py b/src/api/routes/chat.py index c67107d..1c0122d 100644 --- a/src/api/routes/chat.py +++ b/src/api/routes/chat.py @@ -138,10 +138,12 @@ def _save_answer(db, thread, question: str, result: Answer) -> Message: thread_id=thread.id, role="assistant", content=result.text, + # In the order a reload returns them (Message.sources), so a seal keeps + # its number: the live answer is serialized from this list as built. sources=[ MessageSource(chunk_id=c.chunk_id, source=c.source, relevance=c.score, content_sha256=content_hash(c.content)) - for c in result.chunks + for c in sorted(result.chunks, key=lambda c: (-c.score, c.chunk_id, c.source)) ], # A snapshot of what the answer showed, in the order shown (ADR 0011). plans=[_plan_row(position, plan) for position, plan in enumerate(result.plans)], diff --git a/src/models/chat.py b/src/models/chat.py index 1c5aac5..57602cd 100644 --- a/src/models/chat.py +++ b/src/models/chat.py @@ -63,8 +63,15 @@ class Message(Base): created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now(), nullable=False) thread: Mapped["Thread"] = relationship(back_populates="messages") + # Most relevant first; (chunk_id, source) is unique per answer, so ties are + # ordered too, byte-wise ("C") as Python compares them: _save_answer builds + # the list in this same order. sources: Mapped[list["MessageSource"]] = relationship( - back_populates="message", cascade="all, delete-orphan", order_by="MessageSource.relevance.desc()" + back_populates="message", + cascade="all, delete-orphan", + order_by=lambda: ( + MessageSource.relevance.desc(), MessageSource.chunk_id.collate("C"), MessageSource.source.collate("C") + ), ) plans: Mapped[list["MessagePlan"]] = relationship( back_populates="message", cascade="all, delete-orphan", order_by="MessagePlan.position" diff --git a/tests/test_chat_stream.py b/tests/test_chat_stream.py index 6c5a692..3098cde 100644 --- a/tests/test_chat_stream.py +++ b/tests/test_chat_stream.py @@ -292,3 +292,43 @@ def flaky_commit(session): assert [m["role"] for m in _saved(client, headers, thread_id)] == ["user"] left = select(func.count()).select_from(MessageSource).join(Message).where(Message.thread_id == uuid.UUID(thread_id)) assert db.scalar(left) == 0 + + +@pytest.fixture +def kept_on_commit(client, monkeypatch): + """Sessions that keep their objects on commit, as production's SessionLocal + does (src/core/db.py). The live answer is then serialized from the list + _save_answer built, rather than re-read in the relationship's order.""" + make_session = deps.SessionLocal + + def keeping(): + session = make_session() + session.expire_on_commit = False + return session + + monkeypatch.setattr(deps, "SessionLocal", keeping) + return client + + +def test_done_numbers_sources_as_a_reload_does(kept_on_commit): + """Seals are numbered by source order, so a live answer and the same answer + reloaded must list its sources alike: most relevant first, then byte-wise + by chunk and label, whatever order retrieval gave them in.""" + client = kept_on_commit + headers, thread_id = _thread(client, "stream-order@example.com") + chunks = [ + RetrievedChunk("c_low", "low", "wiki_Low.txt", 0.31), + RetrievedChunk("c_tie", "high", "plan a - Summary of Benefits.pdf", 0.92), + RetrievedChunk("c_tie", "high", "Plan B - Summary of Benefits.pdf", 0.92), + RetrievedChunk("c_mid", "mid", "wiki_Mid.txt", 0.55), + RetrievedChunk("b_tie", "high", "wiki_Tie.txt", 0.92), + ] + done = _events(_stream(client, headers, thread_id, [Done(Answer("A.", chunks))]))[-1][1] + + def order(sources): + return [(s["chunk_id"], s["source"]) for s in sources] + + expected = [("b_tie", "wiki_Tie.txt"), ("c_tie", "Plan B - Summary of Benefits.pdf"), + ("c_tie", "plan a - Summary of Benefits.pdf"), ("c_mid", "wiki_Mid.txt"), ("c_low", "wiki_Low.txt")] + assert order(done["message"]["sources"]) == expected + assert order(_saved(client, headers, thread_id)[1]["sources"]) == expected From 93da4c04516bd4db9ccf44122803a21d44219176 Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:23 -0700 Subject: [PATCH 3/8] Keep query values out of database errors and logs --- CHANGELOG.md | 3 +++ src/core/db.py | 4 +++- tests/test_api_logging.py | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 373c292..2b26de5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -761,6 +761,9 @@ ### Security +- The database engine hides bound values (`hide_parameters=True`), so an + email, question or answer never appears in a SQL error or its log line. + Postgres's own message can still quote a value it failed to parse. - **A streamed round that never reports its usage is counted by estimate:** the whole prompt plus the output cap. This covers a client that disconnects, Stop, or a dropped connection. Without it, ending streams diff --git a/src/core/db.py b/src/core/db.py index 89bcc30..cc72051 100644 --- a/src/core/db.py +++ b/src/core/db.py @@ -6,7 +6,9 @@ from ..policypal.config import settings -engine = create_engine(settings.database_url, pool_pre_ping=True) +# hide_parameters: bound values (emails, questions, answers) stay out of SQL +# errors and logs; the statement itself is still shown. +engine = create_engine(settings.database_url, pool_pre_ping=True, hide_parameters=True) SessionLocal = sessionmaker(bind=engine, autoflush=False, expire_on_commit=False) diff --git a/tests/test_api_logging.py b/tests/test_api_logging.py index 30ee799..b8836d1 100644 --- a/tests/test_api_logging.py +++ b/tests/test_api_logging.py @@ -9,8 +9,11 @@ from logging.handlers import RotatingFileHandler import pytest +from sqlalchemy import text +from sqlalchemy.exc import DBAPIError from src.core import logging as app_logging +from src.core.db import engine def test_creating_the_app_installs_the_file_handler(app): @@ -30,6 +33,18 @@ def test_the_libraries_that_would_log_secrets_stay_at_warning(app): assert logging.getLogger(name).level == logging.WARNING +def test_a_database_error_leaves_out_the_values_it_was_given(): + """SQLAlchemy's error text, which is logged, carries the bound values by + default: an email, a question or an answer. (Postgres's own message can + still quote a value it failed on; this error doesn't, so only SQLAlchemy's + part is tested.)""" + with engine.connect() as connection, pytest.raises(DBAPIError) as caught: + connection.execute(text("SELECT :value, 1 / 0"), {"value": "someone@example.com"}) + + assert "someone@example.com" not in str(caught.value) + assert "hidden due to hide_parameters" in str(caught.value) + + @pytest.fixture def production(monkeypatch): monkeypatch.setattr(app_logging.settings, "environment", "production") From ff40f70295c454361c6c04d086445e5ce878dcce Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:23 -0700 Subject: [PATCH 4/8] Count a thread title's length after trimming it --- CHANGELOG.md | 2 ++ src/schemas/chat.py | 15 ++++++--------- tests/test_chat.py | 14 ++++++++------ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b26de5..ebd8123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,8 @@ ### Fixed +- Renaming a thread checks the 1 to 200 characters after trimming, as ADR + 0027 says. A title that fits once its spaces are removed was refused. - **A citation keeps its seal number after a reload.** A streamed answer listed its sources in retrieval order, and a reload in relevance order, so the numbers could change. Both now list them most relevant first, ties by diff --git a/src/schemas/chat.py b/src/schemas/chat.py index 2a08ced..172ae14 100644 --- a/src/schemas/chat.py +++ b/src/schemas/chat.py @@ -10,17 +10,14 @@ class ThreadCreateRequest(BaseModel): class ThreadRenameRequest(BaseModel): - # max_length is checked before the validator strips, so a title padded - # past 200 characters is refused too. - title: str = Field(max_length=200) + # 1 to 200 characters after trimming (ADR 0027): stripped before the + # length is checked, and anything but a string is left for `str` to refuse. + title: str = Field(min_length=1, max_length=200) - @field_validator("title") + @field_validator("title", mode="before") @classmethod - def _not_blank(cls, value: str) -> str: - value = value.strip() - if not value: - raise ValueError("title must not be blank") - return value + def _trimmed(cls, value): + return value.strip() if isinstance(value, str) else value class ThreadResponse(BaseModel): diff --git a/tests/test_chat.py b/tests/test_chat.py index b99f378..ccdbc07 100644 --- a/tests/test_chat.py +++ b/tests/test_chat.py @@ -409,9 +409,9 @@ def test_rename_does_not_move_a_thread_up_the_list(client): assert datetime.fromisoformat(after[1]["updated_at"]) == week_ago -@pytest.mark.parametrize("title", ["", " ", "x" * 201]) -def test_rename_rejects_a_blank_or_long_title(client, title): - headers = _auth_headers(client, email=f"rename-bad{len(title)}@example.com") +@pytest.mark.parametrize("title", ["", " ", "x" * 201, " " + "x" * 201 + " ", 123, None]) +def test_rename_rejects_a_blank_long_or_non_text_title(client, title): + headers = _auth_headers(client, email=f"rename-bad-{uuid.uuid4().hex[:8]}@example.com") thread_id = _new_thread(client, headers) resp = client.patch(f"/api/chat/threads/{thread_id}", json={"title": title}, headers=headers) @@ -419,13 +419,15 @@ def test_rename_rejects_a_blank_or_long_title(client, title): assert resp.status_code == 422 -def test_rename_accepts_a_title_of_exactly_200_characters(client): - headers = _auth_headers(client, email="rename-200@example.com") +@pytest.mark.parametrize("title", ["x" * 200, " " + "x" * 200 + " "]) +def test_rename_accepts_200_characters_counted_after_trimming(client, title): + headers = _auth_headers(client, email=f"rename-200-{len(title)}@example.com") thread_id = _new_thread(client, headers) - resp = client.patch(f"/api/chat/threads/{thread_id}", json={"title": "x" * 200}, headers=headers) + resp = client.patch(f"/api/chat/threads/{thread_id}", json={"title": title}, headers=headers) assert resp.status_code == 200 + assert resp.get_json()["title"] == "x" * 200 @pytest.mark.parametrize("thread_id", ["00000000-0000-0000-0000-000000000000", "not-a-uuid"]) From ac66cd3d9295083bec59893867137c8c821f1033 Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:23 -0700 Subject: [PATCH 5/8] Correct ADR 0027's timeout bound: it left out the CMS pricing call --- CHANGELOG.md | 3 +++ docs/decisions/0027-streaming-answers-and-cited-passages.md | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebd8123..1046842 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,9 @@ ### Fixed +- ADR 0027 notes that its timeout bound left out the CMS pricing call: at + most 8 s in each of two tool rounds, so the theoretical bound passes + Azure's 240 s ingress timeout. - Renaming a thread checks the 1 to 200 characters after trimming, as ADR 0027 says. A title that fits once its spaces are removed was refused. - **A citation keeps its seal number after a reload.** A streamed answer diff --git a/docs/decisions/0027-streaming-answers-and-cited-passages.md b/docs/decisions/0027-streaming-answers-and-cited-passages.md index c913617..8d5342f 100644 --- a/docs/decisions/0027-streaming-answers-and-cited-passages.md +++ b/docs/decisions/0027-streaming-answers-and-cited-passages.md @@ -15,6 +15,12 @@ It amends two earlier ADRs: Sub-project 3 of the California expansion, which had planned to take this number, takes ADR 0028 instead. +Corrected on 2026-09-27: the timeout bound under Consequences counts only +the model calls, which with retries alone reach 240 s. Each of the two tool +rounds can add a live CMS pricing call of up to 8 s +(`cms_live_timeout_seconds`), so the theoretical bound is past Azure's +ingress timeout, not close to it. The usual worst case is still well under it. + ## Context The chat worked, but three things made it read like a prototype: From ffb631f051f1e544b6ba3df4107aa5640a4bdcac Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:32 -0700 Subject: [PATCH 6/8] Say PolicyPal can't be reached when a stream drops, and let the reader go --- CHANGELOG.md | 4 +++ frontend/src/services/apiClient.js | 20 ++++++++++---- frontend/src/services/apiClient.test.js | 35 +++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1046842..1d9d48b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,10 @@ ### Fixed +- **A connection lost mid-answer says PolicyPal can't be reached,** not the + browser's raw error, and `apiStream` always cancels its reader, so the + connection is let go however the stream ends. An abort is still rethrown + as itself. - ADR 0027 notes that its timeout bound left out the CMS pricing call: at most 8 s in each of two tool rounds, so the theoretical bound passes Azure's 240 s ingress timeout. diff --git a/frontend/src/services/apiClient.js b/frontend/src/services/apiClient.js index 50487bb..f1a341a 100644 --- a/frontend/src/services/apiClient.js +++ b/frontend/src/services/apiClient.js @@ -68,7 +68,8 @@ export async function apiFetch(path, { method = "GET", token, body } = {}) { } // A POST answered with text/event-stream. Errors before the stream reject as -// apiFetch's do; an abort is rethrown as the AbortError it is. +// apiFetch's do, and so does a connection lost part-way; an abort is rethrown +// as the AbortError it is. export async function apiStream(path, { token, body, signal, onEvent }) { const headers = { "Content-Type": "application/json", Accept: "text/event-stream" }; if (token) headers.Authorization = `Bearer ${token}`; @@ -90,10 +91,19 @@ export async function apiStream(path, { token, body, signal, onEvent }) { const parser = createSseParser(onEvent); const reader = response.body.getReader(); const decoder = new TextDecoder(); - for (;;) { - const { value, done } = await reader.read(); - if (done) break; - parser.push(decoder.decode(value, { stream: true })); + try { + for (;;) { + const { value, done } = await reader.read(); + if (done) break; + parser.push(decoder.decode(value, { stream: true })); + } + } catch (err) { + if (err?.name === "AbortError") throw err; + throw new ApiError(UNREACHABLE, 0); + } finally { + // Frees the connection whichever way the loop ended; a finished stream + // makes this a no-op, and a failed one can't fail it any further. + reader.cancel().catch(() => {}); } parser.push(decoder.decode()); parser.end(); diff --git a/frontend/src/services/apiClient.test.js b/frontend/src/services/apiClient.test.js index c0700e6..95e2ef6 100644 --- a/frontend/src/services/apiClient.test.js +++ b/frontend/src/services/apiClient.test.js @@ -108,6 +108,17 @@ function streamOf(...chunks) { }); } +// A body whose connection drops after the first chunk, with `failure` as the +// browser's rejection, and a cancel to watch. +function droppingBody(chunk, failure) { + const read = vi + .fn() + .mockResolvedValueOnce({ value: new TextEncoder().encode(chunk), done: false }) + .mockRejectedValueOnce(failure); + const cancel = vi.fn().mockResolvedValue(undefined); + return { getReader: () => ({ read, cancel }), cancel }; +} + describe("apiStream", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -155,6 +166,30 @@ describe("apiStream", () => { expect(error.name).toBe("AbortError"); }); + it("says PolicyPal can't be reached when the connection drops mid-stream, and lets the reader go", async () => { + const body = droppingBody('event: delta\ndata: {"text":"a"}\n\n', new TypeError("network error")); + vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true, status: 200, body })); + const onEvent = vi.fn(); + + const error = await apiStream("/x", { token: "tok", body: {}, onEvent }).catch((e) => e); + + expect(error).toBeInstanceOf(ApiError); + expect(error.message).toMatch(/Can't reach PolicyPal/); + expect(error.message).not.toMatch(/network error/); + expect(onEvent).toHaveBeenCalledWith({ event: "delta", data: { text: "a" } }); + expect(body.cancel).toHaveBeenCalledOnce(); + }); + + it("rethrows an abort mid-stream as the AbortError it is, and lets the reader go", async () => { + const body = droppingBody('event: stage\ndata: {"stage":"searching"}\n\n', new DOMException("aborted", "AbortError")); + vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true, status: 200, body })); + + const error = await apiStream("/x", { token: "tok", body: {}, onEvent: vi.fn() }).catch((e) => e); + + expect(error.name).toBe("AbortError"); + expect(body.cancel).toHaveBeenCalledOnce(); + }); + it("says PolicyPal can't be reached when the network fails", async () => { vi.stubGlobal("fetch", vi.fn().mockRejectedValue(new TypeError("Failed to fetch"))); await expect(apiStream("/x", { token: "tok", body: {}, onEvent: vi.fn() })).rejects.toThrow( From bb80a545f5fee8388374e62202c80def83e7d9d0 Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:32 -0700 Subject: [PATCH 7/8] Show a question sent while scrolled up, and announce a finished answer --- CHANGELOG.md | 6 +++ .../features/chat/transcript/Transcript.jsx | 26 +++++++++- .../chat/transcript/Transcript.test.jsx | 47 +++++++++++++++++++ frontend/src/pages/ChatPage.test.jsx | 3 +- 4 files changed, 79 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d9d48b..0a80947 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,12 @@ ### Fixed +- **A question sent while scrolled up is shown,** and the view follows its + answer again. Scrolling up while it streams still stops the following. +- **A finished answer is announced to screen readers:** "Answer ready, N + sources" (one per cited document, as the seals count them), from a status + region that stays mounted. The streaming region was removed at `done`, so + nothing was said. History never announces. - **A connection lost mid-answer says PolicyPal can't be reached,** not the browser's raw error, and `apiStream` always cancels its reader, so the connection is let go however the stream ends. An abort is still rethrown diff --git a/frontend/src/features/chat/transcript/Transcript.jsx b/frontend/src/features/chat/transcript/Transcript.jsx index a38b296..1416f65 100644 --- a/frontend/src/features/chat/transcript/Transcript.jsx +++ b/frontend/src/features/chat/transcript/Transcript.jsx @@ -1,6 +1,7 @@ import { useEffect, useRef, useState } from "react"; import ErrorBanner from "../../../components/ErrorBanner"; import { toExchanges } from "../../../utils/exchanges"; +import { sourceNumbers } from "../../../utils/markers"; import EmptyState from "./EmptyState"; import Exchange from "./Exchange"; import JumpToLatest from "./JumpToLatest"; @@ -11,6 +12,17 @@ const PINNED_PX = 80; // Further than this above it, "Jump to latest" appears (spec ยง4.3). const JUMP_PX = 200; +// What a screen reader hears when an answer finishes: the streaming region is +// replaced by the saved answer, which announces nothing. Sources are counted +// as the seals number them, one per document. +function readyText(messages, finishedId) { + const answer = finishedId && messages.find((m) => m.id === finishedId); + if (!answer) return ""; + const count = sourceNumbers(answer.sources).size; + if (count === 0) return "Answer ready"; + return `Answer ready, ${count} ${count === 1 ? "source" : "sources"}`; +} + function Content({ messages, status, error, onPrompt, onRetry, children }) { if (status === "loading" && messages.length === 0) { return ( @@ -71,10 +83,15 @@ export default function Transcript({ endRef.current?.scrollIntoView({ block: "end", behavior: still ? "auto" : "smooth" }); } - // The scroll position is the DOM's: follow new text only while pinned. + // The scroll position is the DOM's: follow new text only while pinned. A + // question just sent (a draft appearing) is always shown, and pins the view + // again, wherever the reader had scrolled to. + const drafting = useRef(false); useEffect(() => { + if (live && !drafting.current) pinned.current = true; + drafting.current = Boolean(live); if (pinned.current) endRef.current?.scrollIntoView({ block: "end" }); - }, [messages.length, live?.text, live?.stages.length, live?.notices.length]); + }, [messages.length, live]); return ( <> @@ -103,6 +120,11 @@ export default function Transcript({
{away && } + {/* Always mounted, and empty while an answer streams, so the next + answer is announced even when its text is the same as the last. */} +

+ {live ? "" : readyText(messages, finishedId)} +

); } diff --git a/frontend/src/features/chat/transcript/Transcript.test.jsx b/frontend/src/features/chat/transcript/Transcript.test.jsx index c6db4e1..524fff2 100644 --- a/frontend/src/features/chat/transcript/Transcript.test.jsx +++ b/frontend/src/features/chat/transcript/Transcript.test.jsx @@ -129,4 +129,51 @@ describe("Transcript", () => { rerender(); expect(scrollIntoView).toHaveBeenCalledTimes(1); }); + + it("announces a finished answer with its count of cited documents", () => { + const sources = [ + { id: "s1", source: "Plan A - Summary of Benefits.pdf", chunk_id: "c1", relevance: 0.9 }, + { id: "s2", source: "Plan A - Summary of Benefits.pdf", chunk_id: "c2", relevance: 0.8 }, + { id: "s3", source: "wiki_Copayment.txt", chunk_id: "c3", relevance: 0.7 }, + ]; + const cited = [messages[0], { ...messages[1], sources }]; + const { rerender } = renderTranscript({ messages: cited }); + const status = () => screen.getByRole("status"); + expect(status()).toBeEmptyDOMElement(); // history never announces + + rerender(); + expect(status()).toHaveTextContent("Answer ready, 2 sources"); + }); + + it("says just that the answer is ready when it cites nothing, and stays quiet while one streams", () => { + const live = { questionId: "q2", stages: ["writing"], notices: [], text: "It" }; + const { rerender } = renderTranscript({ finishedId: "a1" }); + const status = () => screen.getByRole("status"); + expect(status()).toHaveTextContent(/^Answer ready$/); + + const asked = [...messages, { id: "q2", role: "user", content: "And a copay?", created_at: "2026-01-01T00:01:00Z" }]; + rerender(); + expect(status()).toBeEmptyDOMElement(); + }); + + it("shows a question you send even while scrolled up, then follows its answer", () => { + const scrollIntoView = vi.fn(); + const props = { messages, status: "ready", live: null, onOpenSource: vi.fn() }; + const { container, rerender } = render(); + container.querySelector(".transcript-end").scrollIntoView = scrollIntoView; + const scroller = container.querySelector(".scroll"); + scrollTo(scroller, { top: 100 }); + + const question = { id: "temp-1", role: "user", content: "And a copay?", created_at: "2026-01-01T00:01:00Z" }; + const live = { questionId: "temp-1", stages: [], notices: [], text: "" }; + rerender(); + expect(scrollIntoView).toHaveBeenCalledTimes(1); + + rerender(); + expect(scrollIntoView).toHaveBeenCalledTimes(2); + + scrollTo(scroller, { top: 100 }); // scrolling up again still stops the following + rerender(); + expect(scrollIntoView).toHaveBeenCalledTimes(2); + }); }); diff --git a/frontend/src/pages/ChatPage.test.jsx b/frontend/src/pages/ChatPage.test.jsx index e7e72e4..8ea4ff0 100644 --- a/frontend/src/pages/ChatPage.test.jsx +++ b/frontend/src/pages/ChatPage.test.jsx @@ -242,7 +242,8 @@ describe("ChatPage", () => { await user.type(screen.getByRole("textbox", { name: "Thread name" }), " 101{Enter}"); expect(renameThread).toHaveBeenCalledWith("t1", "Deductibles 101"); - expect(await screen.findByRole("status")).toHaveTextContent("Couldn't rename this thread. Try again."); + // By its text: the transcript keeps a status region of its own. + expect(await screen.findByText("Couldn't rename this thread. Try again.")).toHaveAttribute("role", "status"); }); it("renames the open thread from its title", async () => { From 03058135492e1ef2a9849078a7f2bddd17f708f2 Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 27 Sep 2026 12:32:32 -0700 Subject: [PATCH 8/8] Keep focus inside the phone drawer, and hide the chat behind it --- CHANGELOG.md | 4 ++ frontend/src/features/chat/AppShell.jsx | 22 +++++--- frontend/src/features/chat/AppShell.test.jsx | 42 +++++++++++++++ frontend/src/features/chat/focus.js | 19 +++++++ frontend/src/features/chat/focus.test.js | 53 +++++++++++++++++++ frontend/src/features/chat/shell.css | 6 +++ .../features/chat/sources/SourcesPanel.jsx | 18 +------ 7 files changed, 140 insertions(+), 24 deletions(-) create mode 100644 frontend/src/features/chat/focus.js create mode 100644 frontend/src/features/chat/focus.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a80947..1cecc32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,10 @@ ### Fixed +- **The phone drawer keeps Tab inside it,** as its `aria-modal` promised, and + hides the chat behind it from screen readers (`aria-hidden`; `inert` would + blur the button that opened it, and focus couldn't return there). The trap + is shared with the Sources sheet (`features/chat/focus.js`). - **A question sent while scrolled up is shown,** and the view follows its answer again. Scrolling up while it streams still stops the following. - **A finished answer is announced to screen readers:** "Answer ready, N diff --git a/frontend/src/features/chat/AppShell.jsx b/frontend/src/features/chat/AppShell.jsx index 642a0ab..3edc0f9 100644 --- a/frontend/src/features/chat/AppShell.jsx +++ b/frontend/src/features/chat/AppShell.jsx @@ -1,13 +1,13 @@ import { AnimatePresence, motion } from "motion/react"; import { useEffect, useRef } from "react"; import { useMediaQuery } from "../../hooks/useMediaQuery"; +import { FOCUSABLE, trapTab } from "./focus"; import "./shell.css"; const SIDEBAR_WIDTH = 276; -const FOCUSABLE = 'a[href], button:not([disabled]), input, [tabindex]:not([tabindex="-1"])'; -// The phone's sidebar: a drawer over the chat, with focus moved into it and -// returned when it closes. +// The phone's sidebar: a drawer over the chat, with focus moved into it, kept +// there, and returned when it closes. function Drawer({ children, onClose }) { const ref = useRef(null); @@ -36,9 +36,12 @@ function Drawer({ children, onClose }) { aria-modal="true" aria-label="Your questions" onKeyDown={(event) => { - if (event.key !== "Escape") return; - event.stopPropagation(); // not also the page's Esc - onClose(); + if (event.key === "Escape") { + event.stopPropagation(); // not also the page's Esc + onClose(); + } else { + trapTab(event, ref.current); + } }} initial={{ x: "-100%" }} animate={{ x: 0 }} @@ -75,7 +78,12 @@ export default function AppShell({ sidebar, main, panel = null, sidebarHidden = {sidebar} )} - {main} + {/* Behind an open drawer the chat is hidden, as aria-modal says; the + drawer keeps Tab. Not inert: that would blur the button that opened + the drawer before the drawer could note it, and focus couldn't go back. */} +
+ {main} +
{panel} ); diff --git a/frontend/src/features/chat/AppShell.test.jsx b/frontend/src/features/chat/AppShell.test.jsx index 19c610e..eef0192 100644 --- a/frontend/src/features/chat/AppShell.test.jsx +++ b/frontend/src/features/chat/AppShell.test.jsx @@ -37,6 +37,7 @@ describe("AppShell", () => { expect(container.firstChild).toHaveClass("collapsed"); expect(screen.queryByRole("complementary", { name: "Your questions" })).not.toBeInTheDocument(); expect(container.querySelector(".side-wrap")).toHaveAttribute("inert"); + expect(container.querySelector(".shell-main")).not.toHaveAttribute("aria-hidden"); }); describe("on a phone", () => { @@ -66,6 +67,47 @@ describe("AppShell", () => { expect(onCloseDrawer).toHaveBeenCalledTimes(2); }); + it("keeps Tab inside the open drawer, and the chat behind it out of reach", async () => { + const twoButtons = ( + + ); + const chat = ( +
+ +
+ ); + const { container } = render(); + const drawer = screen.getByRole("dialog", { name: "Your questions" }); + + for (let i = 0; i < 4; i += 1) { + await userEvent.tab(); + expect(drawer).toContainElement(document.activeElement); + } + await userEvent.tab({ shift: true }); + expect(drawer).toContainElement(document.activeElement); + expect(container.querySelector(".shell-main")).toHaveAttribute("aria-hidden", "true"); + }); + + it("gives the chat back, and focus to what opened the drawer, once it closes", async () => { + const opener = ( +
+ +
+ ); + const { container, rerender } = render(); + screen.getByRole("button", { name: "Open your questions" }).focus(); + + rerender(); + expect(screen.getByRole("button", { name: "New question" })).toHaveFocus(); + rerender(); + + await waitFor(() => expect(screen.getByRole("button", { name: "Open your questions" })).toHaveFocus()); + expect(container.querySelector(".shell-main")).not.toHaveAttribute("aria-hidden"); + }); + it("removes the drawer once closed", async () => { const { rerender } = render(); rerender(); diff --git a/frontend/src/features/chat/focus.js b/frontend/src/features/chat/focus.js new file mode 100644 index 0000000..ec07384 --- /dev/null +++ b/frontend/src/features/chat/focus.js @@ -0,0 +1,19 @@ +// What Tab can reach in the chat's modal layers: the phone drawer and the +// Sources sheet. +export const FOCUSABLE = 'a[href], button:not([disabled]), input:not([disabled]), [tabindex]:not([tabindex="-1"])'; + +// Tab and Shift+Tab stay inside a modal layer, wrapping at either end. Moved +// by hand, since focus can sit on an element that isn't itself in the tab +// order (a selected source card). +export function trapTab(event, container) { + if (event.key !== "Tab") return; + const focusable = [...container.querySelectorAll(FOCUSABLE)]; + if (focusable.length === 0) return; + const current = document.activeElement; + const position = (el) => current.compareDocumentPosition(el); + const after = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_FOLLOWING); + const before = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_PRECEDING); + const next = event.shiftKey ? (before.at(-1) ?? focusable.at(-1)) : (after[0] ?? focusable[0]); + event.preventDefault(); + next.focus(); +} diff --git a/frontend/src/features/chat/focus.test.js b/frontend/src/features/chat/focus.test.js new file mode 100644 index 0000000..7fbf4c4 --- /dev/null +++ b/frontend/src/features/chat/focus.test.js @@ -0,0 +1,53 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { trapTab } from "./focus"; + +function layer(html) { + const container = document.createElement("div"); + container.innerHTML = html; + document.body.append(container); + return container; +} + +function tab(container, shiftKey = false) { + const event = { key: "Tab", shiftKey, preventDefault: vi.fn() }; + trapTab(event, container); + return event; +} + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("trapTab", () => { + it("wraps from the last element to the first, and back with Shift", () => { + const container = layer('Three'); + const [one, , three] = container.querySelectorAll("button, input, a"); + + three.focus(); + expect(tab(container).preventDefault).toHaveBeenCalled(); + expect(one).toHaveFocus(); + + tab(container, true); + expect(three).toHaveFocus(); + }); + + it("moves on from an element outside the tab order, such as a selected card", () => { + const container = layer('
Card
'); + container.querySelector("div").focus(); + + tab(container); + expect(container.querySelectorAll("button")[1]).toHaveFocus(); + }); + + it("skips disabled controls and leaves other keys alone", () => { + const container = layer(""); + const [one, , two] = container.querySelectorAll("button"); + one.focus(); + + tab(container); + expect(two).toHaveFocus(); + const other = { key: "Enter", preventDefault: vi.fn() }; + trapTab(other, container); + expect(other.preventDefault).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/features/chat/shell.css b/frontend/src/features/chat/shell.css index 6d00bcc..d8b34a6 100644 --- a/frontend/src/features/chat/shell.css +++ b/frontend/src/features/chat/shell.css @@ -19,6 +19,12 @@ grid-template-columns: minmax(0, 1fr); } +/* Only there to hide the main column behind the phone drawer: its child + stays the grid item. */ +.shell-main { + display: contents; +} + .shell-error { display: flex; flex-direction: column; diff --git a/frontend/src/features/chat/sources/SourcesPanel.jsx b/frontend/src/features/chat/sources/SourcesPanel.jsx index 77db55b..1d9d406 100644 --- a/frontend/src/features/chat/sources/SourcesPanel.jsx +++ b/frontend/src/features/chat/sources/SourcesPanel.jsx @@ -4,26 +4,10 @@ import { useEffect, useRef } from "react"; import IconButton from "../../../components/IconButton"; import { useMediaQuery } from "../../../hooks/useMediaQuery"; import { sourceNumbers } from "../../../utils/markers"; +import { trapTab } from "../focus"; import SourceCard from "./SourceCard"; import "./sources.css"; -const FOCUSABLE = 'a[href], button:not([disabled]), [tabindex]:not([tabindex="-1"])'; - -// Tab and Shift+Tab stay inside a modal sheet, wrapping at either end. Moved -// by hand, since focus can sit on a card that isn't itself in the tab order. -function trapTab(event, container) { - if (event.key !== "Tab") return; - const focusable = [...container.querySelectorAll(FOCUSABLE)]; - if (focusable.length === 0) return; - const current = document.activeElement; - const position = (el) => current.compareDocumentPosition(el); - const after = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_FOLLOWING); - const before = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_PRECEDING); - const next = event.shiftKey ? (before.at(-1) ?? focusable.at(-1)) : (after[0] ?? focusable[0]); - event.preventDefault(); - next.focus(); -} - // The cited passages of one answer (Main.dc.html's panel): a column beside the // transcript, or a bottom sheet with a scrim at 720 px and below (Mobile.dc.html). export default function SourcesPanel({ message, selectedSourceId, hoveredSourceId, onSelect, onClose }) {