Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/branch-review-ledger.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
37 changes: 33 additions & 4 deletions docs/operator-apply-performance-latency-remediation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
BigSimmo marked this conversation as resolved.

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
Expand All @@ -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);
```
Expand Down
89 changes: 79 additions & 10 deletions tests/ui-phone-scroll.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<HTMLElement>('[data-testid="calculators-phone-dock"]');
const reserve = document.querySelector<HTMLElement>('[data-testid="calculators-search-page"]');
Expand Down Expand Up @@ -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(
Expand Down
Loading