diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 68e2fc45c0..9c2d0731e3 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -167,3 +167,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-30 | claude/top-search-design-mockups-w53znc | 939d5799b9999f3f63928e1b2c95d097f07eff90 | open PR changed-scope review | APPROVE: PR 1400 closeout and issue IDs 131-134 are unique, internally consistent, and preserve the append-only ledgers. | check:branch-review-ledger PASS; check:outstanding-issues PASS; diff review; no unresolved threads | | 2026-07-30 | cursor/safe-branch-cleanup-78a8 | c5190f38834ef2e928edc4876d971aa9d5d69fe7 | open PR changed-scope review | APPROVE after fixes: cleanup refs are discoverable, provider evidence is accurate, and issue 108 closure is preserved against current main. | check:branch-review-ledger PASS; check:outstanding-issues PASS; two review threads resolved | | 2026-07-30 | claude/outstanding-issues-triage-24c8ow | 8d2710fd6cbdc84e8c50a6c9bc0a1e1a0cd612c8 | open PR changed-scope review | APPROVE: completed items 095, 096, 104, 109, and 115 move to archive with no deletion, duplicate ID, or stale next-id. | check:outstanding-issues PASS; check:branch-review-ledger PASS; diff review; no unresolved threads | +| 2026-07-30 | claude/latency-findings-impl-s8g01v | e7ff5e933ba1f34d5adbd46dd77c38aced11ed44 | open PR changed-scope review | APPROVE: ordering-risk documentation is accurate and the near-bottom refusal guard now proves its geometry is non-vacuous before asserting no hide. | diff check PASS; focused test review; no unresolved threads; exact-head Production UI required | diff --git a/docs/operator-apply-performance-latency-remediation.md b/docs/operator-apply-performance-latency-remediation.md index 6ffc528165..4402e8be49 100644 --- a/docs/operator-apply-performance-latency-remediation.md +++ b/docs/operator-apply-performance-latency-remediation.md @@ -60,9 +60,28 @@ RAG-path index: `fetchDocumentTitleAliasRows` (`src/lib/rag/rag-candidate-source can change the title-alias set feeding candidate assembly. No query text changes — but recall does not follow from that. -- `documents_status_id_idx` and the `documents_title_bare_trgm_idx` benefit to - `src/app/api/documents/route.ts:193` are ordering-safe: that path is a user-facing document - list with no retrieval consequence. +- The `documents_title_bare_trgm_idx` benefit to `src/app/api/documents/route.ts:193` is + ordering-safe: that path is a user-facing document list with no retrieval consequence. +- **CORRECTED AGAIN 2026-07-30 — `documents_status_id_idx` is NOT ordering-safe.** The + correction above named the unordered `.limit(12)` as the RAG hazard but attributed it only to + the trigram index. Re-read the statement it is talking about + (`rag-candidate-sources.ts:482`): + + ```js + query = query.or(filters).eq("status", "indexed").limit(12); + ``` + + One statement carries **both** `.eq("status", "indexed")` and the unordered `LIMIT 12` — and + `(status, id)` is exactly the index that serves that equality. So the same mechanism already + documented for the trigram index applies here verbatim: a new plan for the `status` predicate + can return a different twelve title-alias rows into candidate assembly. Treat + `documents_status_id_idx` as **canary-gated**, not as the safe half of this pair. + + The genuinely safe consumer is the other one: `search-scope.ts:271-277` pages with an explicit + `.order("id")`, so its selection is stable and only its sort cost changes. Two consumers, two + verdicts — do not generalise from the ordered one to the unordered one, which is the error + this note corrects. + - The **RAG-path** use of the bare-column trigram indexes is **canary-gated**, full stop. Ordering that `.limit(12)` with a stable `ORDER BY` does **not** lift the gate: an unordered `LIMIT` has no stable selection to preserve, so imposing an order can pick a different twelve than the @@ -73,15 +92,25 @@ not follow from that. changes, not one gate that ordering unlocks. Do not apply on the retracted semantics-neutral claim. -Create them outside a transaction: +**None of these three is safe to run as a block.** All three are reachable from +`rag-candidate-sources.ts:482`, whose unordered `LIMIT 12` has no stable selection to preserve — +see the two corrections above. The `if not exists` guards make each statement individually +re-runnable; they do **not** make the set semantics-neutral. Every one of them needs the live +eval-canary pair before it stays. ```sql +-- CANARY-GATED: serves the `title ILIKE` half of rag-candidate-sources.ts:482. create index concurrently if not exists documents_title_bare_trgm_idx on public.documents using gin (title gin_trgm_ops); +-- CANARY-GATED: serves the `file_name ILIKE` half of the same unordered LIMIT 12. create index concurrently if not exists documents_file_name_bare_trgm_idx on public.documents using gin (file_name gin_trgm_ops); +-- CANARY-GATED (corrected 2026-07-30; previously mislabelled ordering-safe): +-- (status, id) serves the `.eq("status","indexed")` on that same statement, so it can +-- change which twelve title-alias rows reach candidate assembly. Its OTHER consumer, +-- search-scope.ts:271-277, is ordered and genuinely safe — that is not transitive. create index concurrently if not exists documents_status_id_idx on public.documents (status, id); ``` diff --git a/tests/ui-phone-scroll.spec.ts b/tests/ui-phone-scroll.spec.ts index e64f50db73..b9eb28fe0a 100644 --- a/tests/ui-phone-scroll.spec.ts +++ b/tests/ui-phone-scroll.spec.ts @@ -1214,13 +1214,19 @@ test("page-owned focus clearance places a below-fold calculator control above th ); }); -test("calculator combined chrome stays visible with only 96px of near-bottom runway", async ({ page }) => { +// Renamed 2026-07-30: the old title said "96px of near-bottom runway", but 96 is +// `collapseRunwaySlack`, which only exists on the in-flow branch of +// `computeScrollHideUpdate`. Reserve-only overlay has no slack term at all — its +// near-bottom refusal is `offset <= postCollapseMaxOffset + bottomClampTolerance`. +// One test covers both owners, so the title names the behaviour rather than one +// motion's constant. No flake-ledger or allowlist entry referenced the old name. +test("calculator combined chrome refuses a near-bottom hide that would clamp the reader", async ({ page }) => { await page.emulateMedia({ reducedMotion: "no-preference" }); await page.setViewportSize(phoneViewport); await gotoPhoneSurface(page, "/calculators", 112); await addPhoneScrollRunway(page); - const frames = await page.evaluate(async () => { + const { frames, diag } = await page.evaluate(async () => { const main = document.getElementById("main-content"); const dock = document.querySelector('[data-testid="calculators-phone-dock"]'); const reserve = document.querySelector('[data-testid="calculators-search-page"]'); @@ -1250,23 +1256,86 @@ test("calculator combined chrome stays visible with only 96px of near-bottom run const safeAreaRelease = headerRelease > 0 && safeArea ? safeArea.getBoundingClientRect().height : 0; const reserveRelease = Math.max(0, Number.parseFloat(getComputedStyle(reserve).paddingBottom) || 0); const collapseBudget = headerRelease + safeAreaRelease + reserveRelease; - // In-flow: 96px after combined chrome release must refuse hide (collapse - // runway slack). Reserve-only overlay: refuse when the current offset would - // not fit the post-reserve range — sit inside the reserve-release band of - // the bottom rather than 96px above a mis-counted header budget. + // Both target offsets are derived from `computeScrollHideUpdate`'s own + // refusal clauses (use-hide-on-scroll.ts) rather than from a chosen number, + // because the two motions refuse for genuinely different reasons: + // + // in-flow refuse while runwayAfterCollapse <= revealIntentDistance + // + collapseRunwaySlack (12 + 96) + // reserve-only refuse while offset > postCollapseMaxOffset + // + bottomClampTolerance + // + // Mirrored constants — keep in step with use-hide-on-scroll.ts. + const revealIntentDistance = 12; + const collapseRunwaySlack = 96; + const bottomClampTolerance = 1; const maxOffset = Math.max(0, scrollOwner.scrollHeight - scrollOwner.clientHeight); - scrollOwner.scrollTop = phoneOverlayMotion - ? Math.max(0, maxOffset - Math.max(8, Math.min(reserveRelease, 48))) - : Math.max(0, maxOffset - collapseBudget - 96); + const postCollapseMaxOffset = Math.max(0, maxOffset - collapseBudget); + // Reserve-only: land strictly inside the refusal band but not at the very + // bottom, so this stays a boundary case rather than the trivial one. + const overlayTarget = Math.min( + maxOffset, + postCollapseMaxOffset + bottomClampTolerance + Math.max(1, Math.round(reserveRelease / 3)), + ); + const inFlowTarget = Math.max(0, maxOffset - collapseBudget - collapseRunwaySlack); + scrollOwner.scrollTop = phoneOverlayMotion ? overlayTarget : inFlowTarget; + // Capture the offset we actually landed on BEFORE dispatching, and never + // re-read it afterwards. If the policy regresses and the hide is wrongly + // allowed, the reserve collapses, maxOffset shrinks and the browser clamps + // scrollTop — so a post-loop read reports the offset after the bug rather + // than the offset under test. Asserting the band against that clamped value + // made a policy regression surface as "the test setup is wrong", which is + // the opposite of a useful failure. Found by removing the refusal clause and + // watching this test fail for the wrong reason (2026-07-30). + const targetScrollTop = scrollOwner.scrollTop; (mainOwnsScroll ? main : window).dispatchEvent(new Event("scroll", { bubbles: true })); const frames = [read()]; for (let index = 0; index < 18; index += 1) { await new Promise((resolve) => requestAnimationFrame(() => resolve(undefined))); frames.push(read()); } - return frames; + return { + frames, + diag: { + phoneOverlayMotion, + reserveRelease, + collapseBudget, + maxOffset, + postCollapseMaxOffset, + targetScrollTop, + // The other reserve-only clause. If the post-collapse range were shorter + // than this, hide would be refused for being too short overall and the + // near-bottom clause would never be reached — the test would pass + // without testing anything. + minimumRangeForHide: 8 + 24, + inFlowRunwayAfterCollapse: maxOffset - targetScrollTop - collapseBudget, + inFlowRefusalCeiling: revealIntentDistance + collapseRunwaySlack, + }, + }; }); + // Non-vacuity first. "Chrome stayed visible" is only evidence of a refusal if + // the scroll position actually sat inside the refusal band AND the other + // clauses were satisfied — otherwise chrome stays visible for an unrelated + // reason and this test silently guards nothing. Added 2026-07-30: the overlay + // branch previously used chosen offsets, so nothing checked that it landed + // anywhere meaningful. + expect(diag.collapseBudget, "chrome must have geometry to release, or nothing can be refused").toBeGreaterThan(1); + if (diag.phoneOverlayMotion) { + expect( + diag.postCollapseMaxOffset, + "post-collapse range must clear the short-range clause, so the near-bottom clause is what refuses", + ).toBeGreaterThanOrEqual(diag.minimumRangeForHide); + expect(diag.targetScrollTop, "reserve-only offset must sit inside the near-bottom refusal band").toBeGreaterThan( + diag.postCollapseMaxOffset + 1, + ); + } else { + expect( + diag.inFlowRunwayAfterCollapse, + "in-flow runway after collapse must sit inside the slack refusal band", + ).toBeLessThanOrEqual(diag.inFlowRefusalCeiling); + } + expect(frames.every((frame) => !frame.headerHidden && !frame.dockHidden)).toBe(true); for (let index = 1; index < frames.length; index += 1) { expect(frames[index].scrollTop, "rejected near-bottom collapse cannot clamp scrollTop").toBeCloseTo(