**Phase 144 final verification pass — complete, 100% green.** - **Verified:** all 3 tasks already implemented in-tree — `document-modal.js` history-aware modal (one state-only entry per open: push on open, adopt on forward re-open, in-place `replaceState(prev)` on button/Escape/backdrop close, `closingViaBack`-guarded back-close, top-level popstate listener), `tests/unit/test_frontend_doc_modal.py` source pins, `tests/e2e/test_modal_back_button.py` (4 tests). No code fixes needed. - **Tests:** `uv run pytest` → 3371 passed · `uv run pytest --cov=app --cov-report=term-missing` → 3371 passed, **app/ 99%** (gate >90%) · `uv run pytest tests/e2e/test_modal_back_button.py -v --no-cov` → 4 passed in isolation · regressions in isolation: `test_document_back_navigation.py` 3 passed, `test_kb_tree_nav.py` 4 passed · `uv run ruff check .` clean, `uv run pyright` 0 errors. - **Completion criteria:** back closes modal (URL/view/drill untouched; second back navigates as usual) ✅ · back→forward re-opens same doc, no duplicate entry ✅ · button/Escape/backdrop close leaves no dead back step ✅ (state byte-identity + modal-free back chain; `replaceState` can't shrink `history.length` — documented deviation, locked design forbids `history.back()/go()`) · all gates green ✅ · no behavior change in completed phases (pre-phase pins + both regression E2Es green) ✅. - **Docs:** README — one feature-description gap fixed (added the phase-144 Back/Forward modal behavior to the Chat section); all other README facts verified against code (routes, every `.env.example` var, all ~40 documented defaults vs `app/config.py`). AGENTS.md — verified consistent, no changes needed. - **Decisions:** no live-infrastructure work touched; all changes left uncommitted in the working tree per harness protocol. - **Next pending phase:** `145_git_source_links`.
361 lines
17 KiB
Python
361 lines
17 KiB
Python
"""Unit: the document modal's history-entry lifecycle (phase 144, task 02).
|
|
|
|
Back while the ``#doc-modal`` document viewer is open must CLOSE the modal
|
|
— not navigate the view behind it (TODO.md L6). The mechanics live in
|
|
``frontend/assets/document-modal.js``: the open owns ONE state-only
|
|
history entry (``{ modal: "doc", source, path }`` — the second arg
|
|
``""`` keeps the URL where it is, so the router's pathname-only
|
|
popstate is a no-op and the Sources drill re-adopts the state it was
|
|
already showing); Back closes the modal (remembering the landed state
|
|
as ``lastPrev``); a button/Escape/backdrop close removes the entry IN
|
|
PLACE (``replaceState(entry.prev)`` — the next Back behaves as if the
|
|
modal never opened); Forward onto the entry re-opens the same document
|
|
(the browser owns its entries — the phase-99 D2 convention).
|
|
|
|
The browser behavior itself is E2E-gated by the phase-144 story suite
|
|
(``tests/e2e/test_modal_back_button.py`` — task 03 drives the real
|
|
history via ``page.goBack()``/``page.goForward()``); like
|
|
``test_remove_confirm_modal.py`` (the house pattern for a frontend
|
|
module whose real behavior is E2E-gated), this module pins the
|
|
source-level contract a silent regression would break:
|
|
|
|
* a second push on the forward re-open (the adopt branch must assign
|
|
``entry`` WITHOUT a ``pushState``);
|
|
* a ``replaceState`` on a non-modal current entry (the close's
|
|
in-place removal is guarded by ``!closingViaBack`` AND
|
|
``history.state.modal === "doc"`` — the fail-safe);
|
|
* a double-registered popstate listener;
|
|
* a ``history.back()``/``history.go()`` introduction (the browser owns
|
|
its entries — the closed set of ``history.*`` sites is the one push,
|
|
the two replaces, and the read-only ``history.state`` accesses);
|
|
* a lost ``lastPrev`` write (it must land BEFORE the
|
|
``closingViaBack``-wrapped close in the back branch);
|
|
* a fallback-path push (the ``window.open`` no-skeleton fallback's
|
|
early ``return`` precedes the first ``history.`` use in the open).
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import re
|
|
from pathlib import Path
|
|
|
|
FRONTEND = Path(__file__).resolve().parents[2] / "frontend"
|
|
MODAL_JS = FRONTEND / "assets" / "document-modal.js"
|
|
|
|
|
|
def _js() -> str:
|
|
return MODAL_JS.read_text(encoding="utf-8")
|
|
|
|
|
|
def _fn(js: str, name: str) -> str:
|
|
"""The source of a top-level function via balanced-brace counting
|
|
(the test_remove_confirm_modal.py helper — ``export function``
|
|
matches the plain ``function name(`` search)."""
|
|
start = js.find(f"function {name}(")
|
|
assert start != -1, f"{name}() must exist in document-modal.js"
|
|
depth = 0
|
|
for i in range(js.find("{", start), len(js)):
|
|
if js[i] == "{":
|
|
depth += 1
|
|
elif js[i] == "}":
|
|
depth -= 1
|
|
if depth == 0:
|
|
return js[start : i + 1]
|
|
raise AssertionError(f"unbalanced braces in {name}()")
|
|
|
|
|
|
def _block(source: str, from_i: int) -> str:
|
|
"""The ``{ … }`` block opening at the first ``{`` at/after
|
|
``from_i`` — balanced-brace walk, braces included."""
|
|
brace = source.find("{", from_i)
|
|
assert brace != -1, "no block opener found"
|
|
depth = 0
|
|
for i in range(brace, len(source)):
|
|
if source[i] == "{":
|
|
depth += 1
|
|
elif source[i] == "}":
|
|
depth -= 1
|
|
if depth == 0:
|
|
return source[brace : i + 1]
|
|
raise AssertionError("unbalanced braces in the entry/popstate block")
|
|
|
|
|
|
def _popstate_handler(js: str) -> str:
|
|
"""The module-top-level popstate handler's ``{ … }`` body."""
|
|
i = js.find('window.addEventListener("popstate"')
|
|
assert i != -1, "the popstate listener must be registered at module top level"
|
|
arrow = js.find("=> {", i)
|
|
assert arrow != -1, "the popstate listener must be an arrow function"
|
|
return _block(js, arrow)
|
|
|
|
|
|
# ---------- the module state: one declaration, no second writer site ----------
|
|
|
|
|
|
def test_history_state_is_declared_exactly_once() -> None:
|
|
"""REGRESSION: a duplicated module-level declaration / second
|
|
writer site for the entry state. ``entry`` / ``lastPrev`` /
|
|
``closingViaBack`` are each declared by EXACTLY ONE ``let`` (the
|
|
single writer sites — open/close, the popstate handler, the
|
|
popstate handler — are pinned by the per-branch tests below)."""
|
|
js = _js()
|
|
assert js.count("let entry = null;") == 1
|
|
assert len(re.findall(r"let\s+entry\s*=", js)) == 1
|
|
assert js.count("let lastPrev = null;") == 1
|
|
assert len(re.findall(r"let\s+lastPrev\s*=", js)) == 1
|
|
assert js.count("let closingViaBack = false;") == 1
|
|
assert len(re.findall(r"let\s+closingViaBack\s*=", js)) == 1
|
|
|
|
|
|
# ---------- the open: one state-only entry, adopt vs push vs replace ----------
|
|
|
|
|
|
def test_open_pushes_exactly_one_state_only_entry() -> None:
|
|
"""REGRESSION: a double push (two entries per open) or a
|
|
URL-MOVING push. The push is exactly ONE
|
|
``history.pushState({ modal: "doc", source, path }, "")`` — the
|
|
``""`` second arg is what keeps the URL where it is (state-only;
|
|
the router's pathname-only popstate stays a no-op)."""
|
|
open_body = _fn(_js(), "openDocumentModal")
|
|
assert open_body.count("history.pushState(") == 1, (
|
|
"exactly one pushState in the open — a second push stacks a dead history step"
|
|
)
|
|
assert 'history.pushState({ modal: "doc", source, path }, "")' in open_body, (
|
|
"the push must be state-only: the state key modal: \"doc\" + the "
|
|
"doc name, and the \"\" second arg (the URL must not move)"
|
|
)
|
|
|
|
|
|
def test_open_entry_chain_adopts_on_forward_and_replaces_on_defensive_reopen() -> None:
|
|
"""REGRESSION: a second push on the forward re-open. The chain is
|
|
``if (entry) → replaceState IN PLACE (keep entry, update
|
|
source/path — the defensive re-open-while-open never stacks)
|
|
→ else if the current entry is already a modal entry → ADOPT
|
|
(assign ``entry = { prev: lastPrev, source, path }`` WITHOUT any
|
|
pushState — the browser already sits on the entry) → else push.
|
|
The push lives ONLY in the final else."""
|
|
open_body = _fn(_js(), "openDocumentModal")
|
|
st_i = open_body.find("const st = history.state;")
|
|
def_i = open_body.find("if (entry) {", st_i)
|
|
adopt_i = open_body.find('else if (st && st.modal === "doc") {', def_i)
|
|
else_i = open_body.find("} else {", adopt_i)
|
|
push_i = open_body.find('history.pushState({ modal: "doc", source, path }, "");', else_i)
|
|
assert -1 < st_i < def_i < adopt_i < else_i < push_i, (
|
|
"chain order: const st → if (entry) → else if (adopt) → else → push"
|
|
)
|
|
# Defensive re-open: replaceState in place, entry KEPT (updated).
|
|
defensive = open_body[def_i:adopt_i]
|
|
assert "entry.source = source;" in defensive
|
|
assert "entry.path = path;" in defensive
|
|
assert 'history.replaceState({ modal: "doc", source, path }, "")' in defensive
|
|
assert "pushState" not in defensive, "a defensive re-open must never push"
|
|
# Forward re-open: ADOPT — assign entry from lastPrev, no push.
|
|
adopt = open_body[adopt_i:else_i]
|
|
assert "entry = { prev: lastPrev, source, path };" in adopt, (
|
|
"the adopt must take lastPrev (the below-entry remembered at the back-close)"
|
|
)
|
|
assert "pushState" not in adopt, (
|
|
"a second push on the forward re-open stacks a dead history step"
|
|
)
|
|
# Normal open: push with prev = the entry we sat on, in the final else.
|
|
assert "entry = { prev: st, source, path };" in open_body[else_i:push_i]
|
|
assert open_body.count("history.pushState(") == 1
|
|
|
|
|
|
def test_window_open_fallback_precedes_the_entry_logic() -> None:
|
|
"""REGRESSION: a fallback-path push. A page WITHOUT the modal
|
|
skeleton falls back to ``window.open(fullPageUrl…)`` in a new tab —
|
|
that path must not touch the history AT ALL: the fallback's early
|
|
``return`` precedes the first ``history.`` use in the open."""
|
|
open_body = _fn(_js(), "openDocumentModal")
|
|
guard_i = open_body.find("if (!modalEl || !titleEl || !metaEl || !contentEl) {")
|
|
open_i = open_body.find('window.open(fullPageUrl(source, path), "_blank", "noopener");')
|
|
ret_i = open_body.find("return;", open_i)
|
|
first_hist_i = open_body.find("history.")
|
|
assert -1 < guard_i < open_i < ret_i < first_hist_i, (
|
|
"the window.open fallback must early-return before any history use"
|
|
)
|
|
|
|
|
|
# ---------- the close: in-place removal, guarded; entry always nulled ----------
|
|
|
|
|
|
def test_close_removes_the_entry_in_place_and_only_on_a_modal_entry() -> None:
|
|
"""REGRESSION: a ``replaceState`` on the wrong entry. The in-place
|
|
removal ``history.replaceState(entry.prev, "")`` is guarded by
|
|
BOTH ``!closingViaBack`` (a back close already moved us off the
|
|
entry — the browser owns that pop) AND the current entry still
|
|
being a modal entry (the fail-safe: a non-modal current entry is
|
|
NEVER overwritten). Exactly one replaceState in the close."""
|
|
close_body = _fn(_js(), "closeDocumentModal")
|
|
assert close_body.count("history.replaceState(") == 1
|
|
guard = 'if (entry && !closingViaBack && history.state && history.state.modal === "doc") {'
|
|
guard_i = close_body.find(guard)
|
|
assert guard_i != -1, (
|
|
"the close's in-place removal must be guarded by !closingViaBack AND "
|
|
"history.state.modal === \"doc\""
|
|
)
|
|
# The single replace sits INSIDE the guard block.
|
|
block = _block(close_body, guard_i)
|
|
assert 'history.replaceState(entry.prev, "");' in block, (
|
|
"the removal must restore entry.prev (byte-for-byte view state)"
|
|
)
|
|
|
|
|
|
def test_close_nulls_the_entry_unconditionally() -> None:
|
|
"""REGRESSION: a lost ``entry = null`` — the adopt's memory
|
|
surviving a close (a later popstate would then close/adopt against
|
|
a stale entry). ``entry = null`` runs OUTSIDE the guarded if,
|
|
between the removal and the hide, on EVERY close (back or not)."""
|
|
close_body = _fn(_js(), "closeDocumentModal")
|
|
guard_i = close_body.find('if (entry && !closingViaBack && history.state')
|
|
brace_i = close_body.find("{", guard_i)
|
|
block = _block(close_body, guard_i)
|
|
assert "entry = null" not in block, "entry = null must not sit inside the guard"
|
|
null_i = close_body.find("entry = null;", brace_i + len(block))
|
|
hide_i = close_body.find("modalEl.hidden = true;", null_i)
|
|
assert -1 < null_i < hide_i, "entry = null must run (before the hide) on every close"
|
|
|
|
|
|
# ---------- the popstate listener: once, mutually exclusive branches ----------
|
|
|
|
|
|
def test_popstate_listener_registered_exactly_once() -> None:
|
|
"""REGRESSION: a double-registered popstate listener (two closes /
|
|
two re-opens per navigation). The module registers EXACTLY ONE
|
|
``window.addEventListener("popstate"`` at top level."""
|
|
js = _js()
|
|
assert js.count('window.addEventListener("popstate"') == 1
|
|
|
|
|
|
def test_popstate_forward_branch_reopens_and_returns() -> None:
|
|
"""REGRESSION: the forward branch falling THROUGH into the
|
|
back-close branch. Forward = the modal is closed and we landed on
|
|
a modal entry — re-open the doc the entry names (trigger null —
|
|
focus goes to the content target) and RETURN before the back
|
|
branch (the two branches are mutually exclusive)."""
|
|
handler = _popstate_handler(_js())
|
|
const_i = handler.find("const st = event.state;")
|
|
flag_i = handler.find('const isModalEntry = st && st.modal === "doc";')
|
|
fwd_i = handler.find("if (isModalEntry && modalEl && modalEl.hidden) {")
|
|
reopen_i = handler.find("openDocumentModal(st.source, st.path, null);", fwd_i)
|
|
ret_i = handler.find("return;", reopen_i)
|
|
back_i = handler.find("if (!isModalEntry && modalEl && !modalEl.hidden && entry) {", ret_i)
|
|
assert -1 < const_i < flag_i < fwd_i < reopen_i < ret_i < back_i, (
|
|
"forward: land on a modal entry with the modal closed → re-open the "
|
|
"named doc (null trigger) → RETURN before the back-close branch"
|
|
)
|
|
|
|
|
|
def test_popstate_back_branch_remembers_lastprev_then_closes_guarded() -> None:
|
|
"""REGRESSION: a lost ``lastPrev`` write (the forward re-open's
|
|
adopt would then restore ``prev: null`` and corrupt the entry
|
|
below). Back = the modal is open and we landed on a NON-modal
|
|
state: write ``lastPrev = st`` FIRST, then the close wrapped in
|
|
``closingViaBack = true`` … ``= false`` (keeps the close from also
|
|
replaceState-ing the entry the browser already popped). The flag
|
|
and the below-entry have their single writer sites here."""
|
|
js = _js()
|
|
handler = _popstate_handler(js)
|
|
back_i = handler.find("if (!isModalEntry && modalEl && !modalEl.hidden && entry) {")
|
|
assert back_i != -1, (
|
|
"the back branch (non-modal land + modal open + entry) is gone"
|
|
)
|
|
lastprev_i = handler.find("lastPrev = st;", back_i)
|
|
setflag_i = handler.find("closingViaBack = true;", lastprev_i)
|
|
close_i = handler.find("closeDocumentModal();", setflag_i)
|
|
resetflag_i = handler.find("closingViaBack = false;", close_i)
|
|
assert -1 < lastprev_i < setflag_i < close_i < resetflag_i, (
|
|
"back: lastPrev = st → closingViaBack = true → close → = false"
|
|
)
|
|
# The single writer sites (no second writer anywhere in the module).
|
|
assert js.count("lastPrev = st;") == 1
|
|
assert js.count("closingViaBack = true;") == 1
|
|
assert handler.count("closingViaBack = false;") == 1
|
|
|
|
|
|
def test_popstate_passes_through_everything_else() -> None:
|
|
"""REGRESSION: a handler branch firing on entries that are not the
|
|
modal's (the router's view entries, the drill's ``kb`` entries) or
|
|
on a modal entry with the modal still open. The forward branch
|
|
requires ``isModalEntry`` (and the modal hidden) and the back
|
|
branch requires ``entry`` (falsy when the modal is closed) — and
|
|
AFTER the back branch closes the handler does NOTHING (the tail is
|
|
empty): every other popstate falls through untouched."""
|
|
handler = _popstate_handler(_js())
|
|
back_i = handler.find("if (!isModalEntry && modalEl && !modalEl.hidden && entry) {")
|
|
assert back_i != -1, "the back branch (non-modal land + modal open + entry) is gone"
|
|
brace_i = handler.find("{", back_i)
|
|
back_block = _block(handler, back_i)
|
|
tail = handler[brace_i + len(back_block) :]
|
|
# The only thing after the back branch is the handler's own closing
|
|
# brace — no code runs on any other popstate (the pass-through).
|
|
assert tail.strip() in ("", "}"), (
|
|
"no code after the back branch — the pass-through must be a no-op"
|
|
)
|
|
|
|
|
|
# ---------- the no-leak guarantee: the closed set of history.* sites ----------
|
|
|
|
|
|
def test_history_call_sites_are_the_closed_set() -> None:
|
|
"""REGRESSION: a ``history.back()``/``history.go()`` introduction,
|
|
or ANY untracked ``history.*`` call site. The browser owns its
|
|
entries (the phase-99 D2 convention): the module's ONLY
|
|
``history.`` sites are the one ``pushState`` (the open), the two
|
|
``replaceState`` (the defensive re-open, the non-back close), and
|
|
the read-only ``history.state`` accesses."""
|
|
js = _js()
|
|
sites = re.findall(r"history\.\w+", js)
|
|
assert sorted(set(sites)) == [
|
|
"history.pushState",
|
|
"history.replaceState",
|
|
"history.state",
|
|
], f"an untracked history.* site slipped in: {sorted(set(sites))}"
|
|
assert sites.count("history.pushState") == 1
|
|
assert sites.count("history.replaceState") == 2
|
|
assert "history.back(" not in js, "the browser owns its entries — no history.back()"
|
|
assert "history.go(" not in js, "the browser owns its entries — no history.go()"
|
|
|
|
|
|
# ---------- the pre-phase contract a regression could plausibly break ----------
|
|
|
|
|
|
def test_pre_phase_contract_pins_still_hold() -> None:
|
|
"""The phase-26 contracts the entry logic sits beside (a regression
|
|
in the rework must not break them): the Escape capture closes via
|
|
``closeDocumentModal()``; the per-open keydown capture is added on
|
|
open and removed on close (document-level, capture phase); the
|
|
``fetchSeq`` stale-guard lines survive (both checkpoints in the
|
|
fetch chain + the close bump); the trigger-focus restore in the
|
|
close (best-effort, contained in the DOM)."""
|
|
js = _js()
|
|
keydown = _fn(js, "onModalKeydown")
|
|
esc_i = keydown.find('if (e.key === "Escape") {')
|
|
prevent_i = keydown.find("e.preventDefault();", esc_i)
|
|
close_i = keydown.find("closeDocumentModal();", prevent_i)
|
|
assert -1 < esc_i < prevent_i < close_i, "Escape: prevent + closeDocumentModal()"
|
|
open_body = _fn(js, "openDocumentModal")
|
|
close_body = _fn(js, "closeDocumentModal")
|
|
assert (
|
|
'document.addEventListener("keydown", onModalKeydown, true);' in open_body
|
|
), "the per-open capture is added on open (document-level, capture)"
|
|
assert (
|
|
'document.removeEventListener("keydown", onModalKeydown, true);' in close_body
|
|
), "the per-open capture is removed on close"
|
|
# The fetchSeq stale guard: the sequence, both checkpoints, the
|
|
# catch checkpoint, and the close bump (any in-flight fetch is
|
|
# stale after a close).
|
|
assert open_body.count("const seq = ++fetchSeq;") == 1
|
|
assert open_body.count("if (seq !== fetchSeq) return;") == 2, (
|
|
"both render checkpoints (pre- and post-json) must stay stale-guarded"
|
|
)
|
|
assert "if (seq === fetchSeq) renderNotFound(path);" in open_body
|
|
assert "fetchSeq += 1;" in close_body
|
|
# The trigger-focus restore (best-effort: contained in the DOM).
|
|
assert (
|
|
"triggerEl && typeof triggerEl.focus === \"function\" && document.contains(triggerEl)"
|
|
in close_body
|
|
)
|
|
assert "triggerEl.focus({ preventScroll: true });" in close_body
|
|
assert "triggerEl = null;" in close_body
|