fix(ui): document viewer back button returns to the page you came from (chat or sources)

frontend/assets/app.js

@@ -65,14 +65,21 @@ const reducedMotion =
   typeof matchMedia === "function" && matchMedia("(prefers-reduced-motion: reduce)").matches;
 const SCROLL = reducedMotion ? "auto" : "smooth";
 
-/* ---------- document viewer link (phase 10) ----------
+/* ---------- document viewer link (phase 10; phase 13 adds `back`) ----------
- * Every cited document opens in the viewer, in a NEW tab. Both query
+ * Every cited document opens in the viewer, in a NEW tab. All query
  * values are percent-encoded: real paths contain slashes and sometimes
- * spaces, which would otherwise corrupt the query string. (The renderer
+ * spaces, which would otherwise corrupt the query string. `back` tells the
+ * viewer which page to return to when its back button is clicked — the
+ * chips live in the chat, so chat passes "/" (the viewer validates it:
+ * only same-origin relative URLs are honored; Sources links omit it and
+ * get the viewer's /sources.html default). (The renderer
  * renderMarkdown/escapeHtml now lives in assets/markdown.js — a classic
  * script loaded by index.html and document.html before these modules.) */
-export function documentUrl(source, path) {
+export function documentUrl(source, path, back = "/") {
-  return "/document.html?source=" + encodeURIComponent(source) + "&path=" + encodeURIComponent(path);
+  let url =
+    "/document.html?source=" + encodeURIComponent(source) + "&path=" + encodeURIComponent(path);
+  if (back) url += "&back=" + encodeURIComponent(back);
+  return url;
 }
 
 /* ---------- avatar glyphs (phase 08: emoji-free chrome) ----------
@@ -291,7 +298,7 @@ function appendSources(wrap, sources) {
     const chip = document.createElement("a");
     chip.className = "source-chip";
     chip.setAttribute("role", "listitem");
-    chip.href = documentUrl(s.source, s.path);
+    chip.href = documentUrl(s.source, s.path, "/"); // back → the chat page
     chip.target = "_blank"; // open the full document in a new tab
     chip.rel = "noopener";
     chip.textContent = label;

frontend/assets/document.js

@@ -28,14 +28,31 @@ const notFoundEl = document.querySelector("#doc-not-found");
 const mainEl = document.querySelector("#main");
 const backLink = document.querySelector("#doc-back");
 
-/* Back: prefer the browser's own history when there is one (the viewer was
+/* Back button (phase 13): the return target comes from the `back` query
- * opened from this tab's session); a fresh tab lands on the Sources page. */
+ * param, not the browser history — both entry points (chat source chips
-backLink.addEventListener("click", (e) => {
+ * and the Sources table) open the viewer in a NEW tab, where there is no
-  if (window.history.length > 1) {
+ * history to go back to. The param is honored only for same-origin
-    e.preventDefault();
+ * relative URLs (starts with "/" but not "//"), so absolute (https://…),
-    window.history.back();
+ * protocol-relative (//…), and pseudo-protocol (javascript:…) values are
-  }
+ * rejected; anything else falls back to the Sources page. The static
-});
+ * href="/sources.html" in document.html remains the no-JS fallback, and
+ * with the href set the anchor's default click behavior IS the
+ * deterministic navigation (no browser-history heuristics). */
+const backParam = params.get("back") || "";
+const backTarget =
+  backParam.startsWith("/") && !backParam.startsWith("//")
+    ? backParam
+    : "/sources.html";
+backLink.href = backTarget;
+const backLabel = backLink.querySelector("span");
+if (backLabel) {
+  backLabel.textContent =
+    backTarget === "/"
+      ? "Chat"
+      : backTarget === "/sources.html"
+        ? "Sources"
+        : "Back";
+}
 
 function fmtDate(iso) {
   try {

tests/e2e/test_chat_rag.py

@@ -98,12 +98,13 @@ def test_on_topic_question_streams_grounded_answer(
 
     # Grounded: a kubernetes.md source chip renders under the bubble
     # (top-N docs can add more chips; the question's doc must be among them).
-    # Phase 10: chips open the document viewer in a new tab (encoded URL).
+    # Phase 10: chips open the document viewer in a new tab (encoded URL);
+    # phase 13 appends back=/ so the viewer's back button returns to chat.
     chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md")
     expect(chip).to_have_count(1)
     expect(chip.first).to_contain_text("kubernetes.md")
     expect(chip.first).to_have_attribute(
-        "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md"
+        "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F"
     )
     expect(chip.first).to_have_attribute("target", "_blank")
     expect(chip.first).to_have_attribute("rel", "noopener")

tests/e2e/test_document_back_navigation.py

@@ -0,0 +1,206 @@
+"""Phase 13 E2E (Playwright): the viewer's back button returns to the
+page the document was opened from.
+
+Story: ``.agent/user_stories/document-back-navigation.md``
+Run in isolation (DB must be up: ``podman compose up -d db``):
+
+    uv run pytest tests/e2e/test_document_back_navigation.py -v --no-cov
+
+Both entry points (chat source chips, Sources table links) open the viewer
+in a NEW tab, where there is no browser history — so the return target is
+carried in the viewer URL: chat chips append ``&back=%2F`` (resolves to
+"Chat"), Sources links omit the param (the viewer's default
+``/sources.html`` applies → "Sources"). The viewer only honors
+same-origin relative ``back`` values; everything else falls back to
+``/sources.html``.
+
+Test → story mapping (Playwright Mapping Rule):
+1. ``test_back_from_chat_returns_to_chat`` — question → source chip →
+   new tab with ``&back=%2F`` → back link href ``/`` labeled "Chat" →
+   click → the chat page.
+2. ``test_back_from_sources_returns_to_sources`` — Sources table link →
+   new tab without a ``back`` param → back link href ``/sources.html``
+   labeled "Sources" → click → the Sources page.
+3. ``test_malicious_back_param_is_rejected`` — absolute,
+   protocol-relative, and ``javascript:`` ``back`` values all fall back
+   to ``/sources.html`` (labeled "Sources", navigable).
+"""
+from __future__ import annotations
+
+import asyncio
+import re
+from pathlib import Path
+from threading import Thread
+from typing import Any
+
+from playwright.sync_api import Page, expect
+from sqlalchemy import text
+
+from app.config import Settings
+from app.db import SessionLocal
+from app.rag.importer import ImportSummary, import_sources
+from app.rag.llm import LLMClient
+
+REPO = Path(__file__).resolve().parents[2]
+FIXTURES = REPO / "tests" / "fixtures" / "docs"
+QUESTION = "How is my Kubernetes cluster set up?"
+# Seeded fixture doc (source=docs) shared by every test in this file.
+DOC_SOURCE = "docs"
+DOC_PATH = "homelab%2Fkubernetes.md"
+DOC_TITLE = "Kubernetes Homelab Cluster"
+
+
+async def _import_fixtures(mock_port: int) -> ImportSummary:
+    kwargs: dict[str, Any] = {"_env_file": None, "llm_base_url": f"http://127.0.0.1:{mock_port}/v1"}
+    settings = Settings(**kwargs)  # pyright: ignore[reportCallIssue]
+    return await import_sources([FIXTURES], LLMClient(settings))
+
+
+def _run_in_thread(coro: Any) -> Any:
+    """Run a coroutine on a worker thread.
+
+    Playwright's sync API keeps an asyncio loop running on the test thread,
+    so ``asyncio.run`` cannot be called directly from a test body.
+    """
+    box: dict[str, Any] = {}
+
+    def runner() -> None:
+        try:
+            box["value"] = asyncio.run(coro)
+        except BaseException as e:  # noqa: BLE001 — re-raised on the test thread
+            box["error"] = e
+
+    t = Thread(target=runner)
+    t.start()
+    t.join()
+    if "error" in box:
+        raise box["error"]
+    return box["value"]
+
+
+def _reset_db(mock_port: int, seed: bool) -> ImportSummary | None:
+    """Truncate the KB (and query log), then optionally re-import fixtures."""
+    with SessionLocal() as db:
+        db.execute(text("TRUNCATE chunks, documents, query_log"))
+        db.commit()
+    if not seed:
+        return None
+    return _run_in_thread(_import_fixtures(mock_port))
+
+
+# ---------------------------------------------------------------------------
+# 1. Chat source chip → viewer with back=/ → back returns to the chat
+# ---------------------------------------------------------------------------
+
+
+def test_back_from_chat_returns_to_chat(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.set_default_timeout(30_000)
+    page.goto(app_url)
+
+    page.fill("#message-input", QUESTION)
+    page.click("#send-btn")
+
+    chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md")
+    expect(chip).to_have_count(1, timeout=30_000)
+    # Chat chips carry back=/ (encoded %2F) so the viewer knows where home is.
+    expect(chip.first).to_have_attribute(
+        "href", f"/document.html?source={DOC_SOURCE}&path={DOC_PATH}&back=%2F"
+    )
+
+    with page.expect_popup() as popup_info:
+        chip.first.click()
+    viewer = popup_info.value
+    expect(viewer).to_have_url(
+        re.compile(
+            re.escape(f"{app_url}/document.html?source={DOC_SOURCE}&path={DOC_PATH}&back=%2F")
+        )
+    )
+    # The cited document actually rendered (this is the viewer, not an error).
+    expect(viewer.locator("#doc-title")).to_have_text(DOC_TITLE)
+    # Back link resolved to the chat page, labeled "Chat".
+    back = viewer.locator("#doc-back")
+    expect(back).to_have_attribute("href", "/")
+    expect(back).to_have_text("Chat")
+
+    # Click: deterministic anchor navigation back to the chat page.
+    back.click()
+    expect(viewer).to_have_url(f"{app_url}/")
+    expect(viewer.locator("#composer")).to_be_visible()
+
+
+# ---------------------------------------------------------------------------
+# 2. Sources table link → viewer without back param → back returns to Sources
+# ---------------------------------------------------------------------------
+
+
+def test_back_from_sources_returns_to_sources(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.goto(f"{app_url}/sources.html")
+
+    row = page.locator("#docs-tbody tr", has_text="kubernetes.md")
+    expect(row).to_have_count(1)
+    link = row.locator("td:nth-child(2) a.doc-link")
+    expect(link).to_have_count(1)
+    # Sources links carry NO back param — the viewer's default target
+    # (/sources.html) applies.
+    expect(link).to_have_attribute(
+        "href", f"/document.html?source={DOC_SOURCE}&path={DOC_PATH}"
+    )
+
+    with page.expect_popup() as popup_info:
+        link.click()
+    viewer = popup_info.value
+    assert "back=" not in viewer.url, f"unexpected back param: {viewer.url}"
+    expect(viewer.locator("#doc-title")).to_have_text(DOC_TITLE)
+    # Back link kept the default target, labeled "Sources".
+    back = viewer.locator("#doc-back")
+    expect(back).to_have_attribute("href", "/sources.html")
+    expect(back).to_have_text("Sources")
+
+    back.click()
+    expect(viewer).to_have_url(f"{app_url}/sources.html")
+    expect(viewer.locator("#docs-table")).to_be_visible()
+
+
+# ---------------------------------------------------------------------------
+# 3. Hostile back values (absolute, protocol-relative, pseudo-protocol)
+#    are all rejected in favor of the same-origin default
+# ---------------------------------------------------------------------------
+
+
+def test_malicious_back_param_is_rejected(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    errors: list[str] = []
+    dialogs: list[str] = []
+    page.on("pageerror", lambda e: errors.append(str(e)))
+
+    def _catch_dialog(d) -> None:  # a fired dialog == executed script
+        dialogs.append(d.message)
+        d.dismiss()
+
+    page.on("dialog", _catch_dialog)
+
+    viewer_base = f"{app_url}/document.html?source={DOC_SOURCE}&path={DOC_PATH}"
+    # Anything that is not a same-origin relative URL must be rejected:
+    # an absolute https URL, a protocol-relative URL, and a javascript:
+    # pseudo-protocol.
+    for evil in ("https%3A%2F%2Fevil.com", "%2F%2Fevil.com", "javascript%3Aalert(1)"):
+        page.goto(f"{viewer_base}&back={evil}")
+        expect(page.locator("#doc-title")).to_have_text(DOC_TITLE)
+        back = page.locator("#doc-back")
+        expect(back).to_have_attribute("href", "/sources.html")
+        expect(back).to_have_text("Sources")
+
+    # And the fallback is really navigable: clicking lands on Sources.
+    page.click("#doc-back")
+    expect(page).to_have_url(f"{app_url}/sources.html")
+
+    assert dialogs == [], f"dialog fired — a back param escaped validation: {dialogs}"
+    assert errors == [], f"console crashes: {errors}"

tests/e2e/test_document_viewer.py

@@ -99,10 +99,11 @@ def test_source_chip_opens_document(
 
     chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md")
     expect(chip).to_have_count(1, timeout=30_000)
-    # New-tab contract: same-origin viewer URL, both query values encoded
+    # New-tab contract: same-origin viewer URL, all query values encoded
-    # (the path's slashes come out as %2F — exactly why encoding matters).
+    # (the path's slashes come out as %2F — exactly why encoding matters),
+    # plus back=/ (phase 13) so the viewer's back button returns to chat.
     expect(chip.first).to_have_attribute(
-        "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md"
+        "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F"
     )
     expect(chip.first).to_have_attribute("target", "_blank")
     expect(chip.first).to_have_attribute("rel", "noopener")
@@ -112,7 +113,9 @@ def test_source_chip_opens_document(
     viewer = popup_info.value
     expect(viewer).to_have_url(
         re.compile(
-            re.escape(f"{app_url}/document.html?source=docs&path=homelab%2Fkubernetes.md")
+            re.escape(
+                f"{app_url}/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F"
+            )
         )
     )
     expect(viewer.locator("#doc-title")).to_have_text("Kubernetes Homelab Cluster")

tests/unit/test_document_viewer.py

@@ -160,18 +160,26 @@ def test_content_requires_both_params() -> None:
 
 def test_viewer_url_builder_present_in_chat_and_sources() -> None:
     """Both entry points (chat chips, Sources rows) build the same
-    encoded viewer URL and open it in a new tab with rel=noopener."""
+    encoded viewer URL and open it in a new tab with rel=noopener.
+
+    Phase 13: the chat builder additionally carries ``back=/`` (encoded
+    %2F) so the viewer's back button returns to the chat; Sources links
+    intentionally omit the param (the viewer's /sources.html default)."""
     for name, js in (("app.js", _read(APP_JS)), ("sources.js", _read(SOURCES_JS))):
-        assert "function documentUrl(source, path)" in js, name
         assert '"/document.html?source=" + encodeURIComponent(' in js, name
         assert '"&path=" + encodeURIComponent(' in js, name
 
     app_js = _read(APP_JS)
-    assert "chip.href = documentUrl(s.source, s.path)" in app_js
+    # Chat: 3-arg builder with back defaulting to the chat page.
+    assert 'function documentUrl(source, path, back = "/")' in app_js
+    assert '"&back=" + encodeURIComponent(back)' in app_js
+    assert 'chip.href = documentUrl(s.source, s.path, "/")' in app_js
     assert 'chip.target = "_blank"' in app_js
     assert 'chip.rel = "noopener"' in app_js
 
     sources_js = _read(SOURCES_JS)
+    # Sources: unchanged 2-arg builder — no back param in the URL.
+    assert "function documentUrl(source, path)" in sources_js
     assert 'link.className = "doc-link"' in sources_js
     assert "link.href = documentUrl(d.source, d.path)" in sources_js
     assert 'link.target = "_blank"' in sources_js
@@ -188,7 +196,9 @@ def _run_node(script: str) -> str:
 
 
 def _extract_function(js: str, name: str) -> str:
-    match = re.search(rf"(?:export )?function {name}\(source, path\) \{{.*?\n\}}", js, re.S)
+    match = re.search(
+        rf"(?:export )?function {name}\(source, path(?:, back = \"/\")?\) \{{.*?\n\}}", js, re.S
+    )
     assert match, f"{name}(source, path) not found"
     return match.group(0).replace("export ", "", 1)
 
@@ -196,18 +206,21 @@ def _extract_function(js: str, name: str) -> str:
 @pytest.mark.skipif(not HAVE_NODE, reason="node not available")
 def test_viewer_url_builder_encodes_spaces_and_slashes() -> None:
     """Behavioral check of the real builder (app.js) under node: slashes
-    and spaces in source/path values must come out percent-encoded."""
+    and spaces in source/path values must come out percent-encoded, and
+    the back target is appended + encoded (phase 13)."""
     fn = _extract_function(_read(APP_JS), "documentUrl")
     out = _run_node(
         f"{fn}\n"
         "console.log(documentUrl('Homelab', 'kubernetes.md'));\n"
         "console.log(documentUrl('Homelab', 'notes/my file.yaml'));\n"
-        "console.log(documentUrl('H omelab', 'a/b.md'));"
+        "console.log(documentUrl('H omelab', 'a/b.md'));\n"
+        "console.log(documentUrl('Homelab', 'kubernetes.md', '/sources.html'));"
     )
     assert out.splitlines() == [
-        "/document.html?source=Homelab&path=kubernetes.md",
+        "/document.html?source=Homelab&path=kubernetes.md&back=%2F",
-        "/document.html?source=Homelab&path=notes%2Fmy%20file.yaml",
+        "/document.html?source=Homelab&path=notes%2Fmy%20file.yaml&back=%2F",
-        "/document.html?source=H%20omelab&path=a%2Fb.md",
+        "/document.html?source=H%20omelab&path=a%2Fb.md&back=%2F",
+        "/document.html?source=Homelab&path=kubernetes.md&back=%2Fsources.html",
     ]
 
 
@@ -259,10 +272,19 @@ def test_markdown_renderer_stays_xss_safe_and_unchanged() -> None:
 def test_viewer_js_rendering_contracts() -> None:
     """document.js: raw formats go in via textContent (never parsed as
     HTML), markdown via the shared renderer, 404 → designed not-found
-    state, back link prefers browser history when there is one."""
+    state, and (phase 13) the back link resolves the ``back`` param —
+    same-origin relative URLs only, /sources.html default, no browser
+    history heuristics (both entry points are fresh tabs)."""
     js = _read(DOCUMENT_JS)
     assert "pre.textContent = doc.content" in js  # raw formats: text node
     assert "renderMarkdown(doc.content)" in js  # md/markdown: shared renderer
     assert "showNotFound" in js
-    assert "history.length > 1" in js
+    # Phase 13: deterministic back-target resolution, no history heuristics.
+    assert "history.length" not in js
+    assert "window.history.back" not in js
+    assert 'backParam.startsWith("/")' in js  # same-origin relative only…
+    assert 'backParam.startsWith("//")' in js  # …and not protocol-relative
+    assert 'backLink.href = backTarget' in js  # deterministic anchor navigation
+    assert '"/sources.html"' in js  # default target + no-JS fallback value
+    assert '"Chat"' in js and '"Sources"' in js  # labels for the two entry points
     assert "encodeURIComponent" in js  # content fetch uses the same encoding