amadan
fix(search): the three findings from the second review
A 303 to /signin is a 200 to fetch, which follows it. Left unchecked, an expired session put the whole sign-in page inside the results list with the stamp below vouching for it as the answer to what was in the box. drop.js refuses that page for the same reason; search.js does now too, on res.redirected and on the response's own pathname. A failure that is only announced is invisible. The status region is sr-only, so a failed live search left a sighted reader looking at older results with nothing on screen admitting they were older — this codebase's worst failure shape, in its politest form. The region now carries rst-search-stale and CSS renders the sentence from the attribute the server already wrote, so the words stay in a template and no script composes prose. A later good search clears it. drop.js sets the tray's reload link once, at load, and its own template promises that link keeps the search on screen. True until live search existed; false from the first keystroke. put() repoints it alongside replaceState. Gates: the page must carry the failure sentence, and it must be the SAME sentence the server's own callout renders — proved by taking the database away so the callout actually runs, then comparing the two strings. The first version of that gate could not fail: it searched the whole page for the carried sentence, and the page carries the attribute, so the sentence always found itself. Caught by mutating the wording. Three mutations now: the attribute deleted, the wording drifted, and the fragment growing an rst-refresh wrapper. All three red, then reverted. Looked at the page again: 17 checks in chromium against tools/drive, including a 303 into the results region, the notice visible and then cleared, the tray link following the live query, and the notice resolving a colour in dark mode. One browser check failed and the code was right: Playwright's waitForNavigation resolves on same-document navigations, so it fired on our own replaceState and read the URL before the real navigation. The test waits for the destination now. Reviewed by Fable 5.1 (claude-fable-5-1), twice: REQUEST CHANGES on nine findings, then REQUEST CHANGES on these three. 🤖 Written by Claude Opus 5, reviewed by Fable 5.1 and Paul.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 files changed,
+131
−2
internal/memoria/static/memoria.css+18 −0internal/memoria/static/search.js+25 −1internal/memoriatest/search_test.go+88 −1
diff --git a/internal/memoria/static/memoria.css b/internal/memoria/static/memoria.css| index b37ea17..4bfb536 100644 |
| --- a/internal/memoria/static/memoria.css |
| +++ b/internal/memoria/static/memoria.css |
| @@ -235,6 +235,24 @@ |
| } |
| [rst-result-meta] { font-size: 0.8125rem; margin: 0; } |
| +/* The results on screen no longer describe what is in the box, because |
| + the last live search did not run. Shown, not merely announced: the |
| + status region is sr-only, so without this a sighted reader sees older |
| + results and nothing saying they are older — the confident-zero failure |
| + AGENTS.md names, in its politest form. |
| + |
| + The sentence comes from the region's own rst-search-stale-note, which |
| + the server wrote from the same {{define "searchFailed"}} as the |
| + server-rendered callout. CSS renders the attribute so the words stay |
| + in the template and no script ever composes a sentence. */ |
| +[rst-search-stale]::before { |
| + color: var(--rst-tone-negative-fg, var(--rst-text)); |
| + content: attr(rst-search-stale-note); |
| + display: block; |
| + font-size: 0.9375rem; |
| + margin-bottom: 0.9rem; |
| +} |
| + |
| /* The primed result: the one Enter goes to. Deliberately the same mark |
| as [rst-passage]:target on the document page, because it means the |
| same thing in both places — this is the one you are being sent to. |
diff --git a/internal/memoria/static/search.js b/internal/memoria/static/search.js| index 497b445..4542bc2 100644 |
| --- a/internal/memoria/static/search.js |
| +++ b/internal/memoria/static/search.js |
| @@ -123,6 +123,15 @@ |
| .then(function (res) { |
| if (ctl !== inflight) throw stale(); |
| if (!res.ok) throw new Error("search " + res.status); |
| + // A session that ran out answers a GET with a 303 to /signin, |
| + // which fetch follows and reports as a perfectly good 200. Left |
| + // unchecked, the sign-in page goes into the results region — a |
| + // whole document nested inside a list, with the stamp below |
| + // vouching for it as the answer to what is in the box. drop.js |
| + // refuses the same page for the same reason; this is that guard. |
| + if (res.redirected || new URL(res.url).pathname !== "/search") { |
| + throw new Error("not the results"); |
| + } |
| return res.text(); |
| }) |
| .then(function (html) { |
| @@ -145,7 +154,15 @@ |
| // And the page SAYS so: results that silently describe an |
| // older query are the confident-zero failure wearing a hat. |
| var el = region(); |
| - if (el) el.removeAttribute("rst-search-results-for"); |
| + if (el) { |
| + el.removeAttribute("rst-search-results-for"); |
| + // Said out loud AND shown. The status region is sr-only, so |
| + // announcing alone leaves a sighted reader looking at older |
| + // results with nothing on screen admitting it — which is the |
| + // confident-zero failure with better manners. The sentence |
| + // itself stays in the template; CSS renders the attribute. |
| + el.setAttribute("rst-search-stale", ""); |
| + } |
| announce(el && el.getAttribute("rst-search-stale-note")); |
| } |
| throw err; |
| @@ -172,6 +189,7 @@ |
| var held = links().indexOf(document.activeElement); |
| el.innerHTML = html; |
| el.setAttribute("rst-search-results-for", q); |
| + el.removeAttribute("rst-search-stale"); |
| if (held >= 0) { |
| var all = links(); |
| (all[held] || all[all.length - 1] || input).focus(); |
| @@ -184,6 +202,12 @@ |
| if (window.history && history.replaceState) { |
| var url = q === "" ? location.pathname : location.pathname + "?q=" + encodeURIComponent(q); |
| history.replaceState(null, "", url); |
| + // drop.js sets the tray's reload link once, at load, from the URL |
| + // as it was then. Its own template promises that link keeps the |
| + // search on screen — true before live search existed, and false |
| + // from the first keystroke unless it is repointed here. |
| + var reload = document.querySelector("[rst-tray-reload]"); |
| + if (reload) reload.setAttribute("href", url); |
| } |
| announce(); |
| } |
diff --git a/internal/memoriatest/search_test.go b/internal/memoriatest/search_test.go| index 129b61e..be72140 100644 |
| --- a/internal/memoriatest/search_test.go |
| +++ b/internal/memoriatest/search_test.go |
| @@ -110,7 +110,10 @@ func TestSearchFragmentIsResultsOnly(t *testing.T) { |
| t.Error("the fragment does not prime, so live search and a form " + |
| "submit would disagree about where Enter goes") |
| } |
| - for _, chrome := range []string{"<html", "<body", "rst-page-header", "Add something"} { |
| + // rst-refresh is on the list because a fragment that ever grew the |
| + // wrapper would nest a refreshable region inside one, and drop.js |
| + // would then find two regions with the same name. |
| + for _, chrome := range []string{"<html", "<body", "rst-page-header", "Add something", "rst-refresh"} { |
| if strings.Contains(body, chrome) { |
| t.Errorf("the fragment carries page chrome (%q); it would nest a "+ |
| "whole page inside the results list", chrome) |
| @@ -150,6 +153,90 @@ func TestFragmentAndPageAgreeOnPriming(t *testing.T) { |
| } |
| } |
| +// When search.js's own fetch fails there is no server response to render |
| +// a callout from, so the page must arrive already carrying the sentence. |
| +// Without it the script has nothing to say and nothing to show, and the |
| +// older results stand on screen as though they were current — which is |
| +// this codebase's worst failure shape, reached by deleting one attribute. |
| +// |
| +// The sentence must be the SAME one the server's callout uses. Two |
| +// wordings for "the search did not run" is a page arguing with itself. |
| +// |
| +// Broken by deleting rst-search-stale-note from index.html — red, then |
| +// reverted. Broken again by changing its wording so the two disagree — |
| +// red, then reverted. 2026-09-05. |
| +func TestThePageCarriesTheFailureSentenceItMayNeed(t *testing.T) { |
| + h, _, d := newAppWithLog(t) |
| + store(t, h, "Q3 board pack", "June revenue was 1,350,000 across all regions.") |
| + |
| + // The sentence the page hands to search.js, for a fetch that never |
| + // answers and so has no callout of its own to render. |
| + carried := attr(t, get(t, h, "/").Body.String(), "rst-search-stale-note") |
| + if carried == "" { |
| + t.Fatal("the page carries no failure sentence, so a failed live search has nothing to show") |
| + } |
| + |
| + // The sentence the SERVER renders when the search itself fails. |
| + // Taking the database away is how that path is reached; nothing a |
| + // well-formed request can carry makes store.Search fail. |
| + d.Close() |
| + broken := get(t, h, "/?q=june+revenue").Body.String() |
| + if !strings.Contains(broken, `rst-callout="danger"`) { |
| + t.Fatalf("a failed search did not render its callout, so there is nothing to compare:\n%s", broken) |
| + } |
| + // Compare the callout's OWN words to the carried ones. Searching the |
| + // whole page for the carried sentence is what this gate did first, |
| + // and it could not fail: that page also carries the attribute, so |
| + // the sentence always found itself and a drifted callout went |
| + // unnoticed. Caught by mutating the attribute's wording. |
| + spoken := calloutBody(broken) |
| + if spoken == "" { |
| + t.Fatalf("could not read the failure callout's own words:\n%s", broken) |
| + } |
| + if spoken != carried { |
| + t.Errorf("the sentence search.js would show and the one the server shows disagree:\n"+ |
| + " server: %q\n carried: %q", spoken, carried) |
| + } |
| + if strings.Contains(broken, "rst-result-primed") { |
| + t.Error("a search that broke primed a result") |
| + } |
| +} |
| + |
| +// calloutBody is the text of the first danger callout's body, trimmed. |
| +func calloutBody(body string) string { |
| + i := strings.Index(body, `rst-callout="danger"`) |
| + if i < 0 { |
| + return "" |
| + } |
| + rest := body[i:] |
| + open := strings.Index(rest, "<p rst-callout-body>") |
| + if open < 0 { |
| + return "" |
| + } |
| + rest = rest[open+len("<p rst-callout-body>"):] |
| + shut := strings.Index(rest, "</p>") |
| + if shut < 0 { |
| + return "" |
| + } |
| + return strings.TrimSpace(rest[:shut]) |
| +} |
| + |
| +// attr pulls one attribute's value out of rendered markup. Small enough |
| +// to be obvious, and obvious beats a parser for one attribute. |
| +func attr(t *testing.T, body, name string) string { |
| + t.Helper() |
| + i := strings.Index(body, name+`="`) |
| + if i < 0 { |
| + return "" |
| + } |
| + rest := body[i+len(name)+2:] |
| + j := strings.Index(rest, `"`) |
| + if j < 0 { |
| + return "" |
| + } |
| + return rest[:j] |
| +} |
| + |
| // The stamp is what search.js reads to decide whether the results on |
| // screen still describe what is in the box. Reading the input instead |
| // was the bug Fable found: after a Back, a browser restores the field on |