From fb1c80b1cd8f824150e89a1bb6866bcdb7dd84d1 Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 22 Aug 2026 03:17:49 -0400 Subject: [PATCH 1/2] test(e2e): measure a dropped node against the copy it is welded to (#566) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scroll-follow half of "a dropped node stays where it was put, and scrolls with the page" has been red on main since the landing-v5 port (#524) merged, taking the whole e2e browser lane with it. It is not flaky: three CI retries and every local run land within 0.2px of each other, ~98px short of where the assertion expects the node. The product is right and the journey was wrong. `faq` is the one section in DragField's `TRACKS`, and `engine/sim.ts::syncClusters()` positions a tracked cluster at `field.top + a.top + dy`, where `dy = (track.top - field.top) - trackTop`. That collapses to `cluster_y = track.top + const`: the cluster is welded to the FAQ question column, which is `sticky; top:110`. Instrumented over the journey's own 300px scroll: scrollY +300.00 #faq top -300.00 track top -202.81 <- pinned at top:110 partway through cluster top -202.80 probe node y -202.60 error 97.40 (CI: 98.2-98.4) The column had exactly 202.81px of travel left before its pin, so ~97px of the scroll happened with the copy held still and the cluster correctly held still with it. Welding those clusters to their copy is deliberate — 8bb34869 added it because they slid 374px out from under the words they belong to. So the assertion was measuring the one coupling this section does not have. The other two scroll journeys in this file already measure relatively — a ring against its own cluster, a cluster against its act's stage — and both pass; this one reached for raw scrollY, on the single cluster where that is untrue. It picked cluster 4 for elbow room, not knowing `faq` is also the only tracked field. Re-frames it against the weld instead, read from the field's own `data-drag-track` so the test follows the product rather than duplicating it: drop the weld and `weldTop()` falls back to the field, which is the plain 1:1 page-scroll reference. The intent is unchanged — placed, not detached — and it keeps its teeth: a guard that the weld target really moved (>100px) plus the exact 300px scroll, so a node pinned to the screen over a page that never scrolled still fails. Pointing the same assertion at the untracked section reproduces the 97.40px failure, so it is load-bearing, not vacuous. Verified on the full local stack: this spec 6/6 twice, full suite 47/48 with one unrelated gradebook flake that passed 3/3 on re-run (retries are CI-only). Closes #566 Co-Authored-By: Claude Opus 5 --- frontend/e2e/landing-drag-field.spec.ts | 48 ++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/frontend/e2e/landing-drag-field.spec.ts b/frontend/e2e/landing-drag-field.spec.ts index c0db5c7b..7079cb25 100644 --- a/frontend/e2e/landing-drag-field.spec.ts +++ b/frontend/e2e/landing-drag-field.spec.ts @@ -69,6 +69,11 @@ async function openLanding(page: Page): Promise { * nothing left to autoscroll into and a 300px scroll assertion cannot be met. */ const CLUSTER = "4"; +/** + * The section that cluster lives in, so the journeys can ask the page what + * the cluster is welded to rather than assuming it is welded to nothing. + */ +const CLUSTER_SECTION = "faq"; /** A satellite, not the course puck: the link spring pulls hardest on these. */ const SATELLITE = 1; @@ -120,6 +125,30 @@ async function probe(page: Page): Promise { }); } +/** + * Where the page content a cluster is welded to currently sits on screen. + * + * A field names its weld target in `data-drag-track` (DragField.tsx's + * `TRACKS`), and `engine/sim.ts` translates the cluster so that it holds a + * fixed offset from it. `faq` is the one section that has one: its question + * column is `sticky; top:110`, so the section can scroll 300px while the copy + * the clusters belong to moves only ~202 and the clusters go with the copy. + * + * Read from the attribute rather than naming the column here, so this stays + * in step with the product: if the weld is ever dropped, this falls back to + * the field itself, which is the plain 1:1 page-scroll reference. + */ +async function weldTop(page: Page, section = CLUSTER_SECTION): Promise { + return page.evaluate((s) => { + const field = document.querySelector(`#${s} .drag-field`); + if (!field) throw new Error(`no drag field in #${s}`); + const sel = field.getAttribute("data-drag-track"); + const el = sel ? document.querySelector(sel) : field; + if (!el) throw new Error(`weld target ${sel} is not in the document`); + return el.getBoundingClientRect().top; + }, section); +} + test("nodes do not move of their own accord while the page scrolls", async ({ page }) => { const height = await openLanding(page); @@ -347,10 +376,27 @@ test("a dropped node stays where it was put, and scrolls with the page", async ( expect(Math.hypot(settled.x - dropped.x, settled.y - dropped.y)).toBeLessThan(SWAY_PX); // Placed, not detached: it belongs to the page and moves with it. + // + // "The page" is the copy its cluster is welded to, NOT raw scrollY. The faq + // question column is `sticky; top:110`, so scrolling 300px moves the + // section 300px and the copy only as far as the pin allows — and the + // clusters go with the copy, which is the whole point of `TRACKS`. + // Measuring against scrollY asserted the one coupling this section + // deliberately does not have, and cost the node ~98px it never owed. + const weldBefore = await weldTop(page); await page.evaluate(() => window.scrollBy(0, 300)); await page.waitForTimeout(600); const scrolled = await probe(page); - expect(Math.hypot(scrolled.x - settled.x, scrolled.y - (settled.y - 300))) + const travelled = (await weldTop(page)) - weldBefore; + + // Not a fixed overlay floating over the site: the copy it is welded to + // really did move under it. Without this the assertion below would pass on + // a node that is pinned to the screen and a page that never scrolled. + expect(Math.abs(travelled), "the weld target should have moved").toBeGreaterThan(100); + expect(scrolled.scrollY - settled.scrollY, "the page should have scrolled").toBe(300); + + // ...and the node went exactly that far, inside its sway. + expect(Math.hypot(scrolled.x - settled.x, scrolled.y - (settled.y + travelled))) .toBeLessThan(SWAY_PX); }); From 9da3e4c57de66e69aa45ec9165d717283b9ec36c Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 22 Aug 2026 03:43:22 -0400 Subject: [PATCH 2/2] test(e2e): let the journey own its oracle, and put the 1:1 case back (#566) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit found the fix had bought its elegance with the thing the test is for. `weldTop()` read the reference out of `data-drag-track` — the product's own declaration — and fell back to the field when it was absent. So deleting the `faq` entry from DragField's `TRACKS` would regress the product (clusters slide 374px out from under the words again, the bug 8bb34869 fixed) while the test re-framed itself to match and stayed green. A test that derives its expectation from the code under test cannot fail when that code is wrong. Proven, not argued: emptying `TRACKS` against the first version left it passing; against this one the offset drifts 96.69px and it fails, while the unmutated page sits at 0.45px. The oracle now belongs to the test — `FAQ_COPY` names the column — and the reference is resolved section-scoped, the way `engine/sim.ts:296` resolves it (`(field.closest('section') ?? document)`), so the journey measures against the element the sim actually bound to instead of whatever matches first document-wide. Also from the review: - The invariant is expressed as an offset that must not change, reusing the idiom the act-tutor journey already uses, rather than a signed delta that only worked out because the page scrolls downward. - The "did the reference move" guard is signed (`< -100`), not `Math.abs(...) > 100`: scrolling down must carry content up, and an absolute value would bless a copy that translated the wrong way. - Preconditions run before the guard, so a page that failed to scroll says so instead of blaming the weld. - Node and reference are sampled in ONE `page.evaluate`; two round trips compared two different frames of a still-integrating sim. - `toBeCloseTo(300, 0)` rather than exact float equality on a browser-computed scroll offset. - The dead `section` parameter is gone. It was never passed, and `weldTop(page, CLUSTER)` — plausible, given the adjacent constant — built `#4 .drag-field` and threw a raw SyntaxError. The deeper finding was coverage: re-framing the only page-relative assertion in the file left nothing tying a cluster to raw document scroll, while the file header still teaches that symptom 4 survived every earlier test because they measured against a field instead of the page. So the plain case is restored where it is actually true — a new journey on cluster 6 in `newsletter`, which has no `TRACKS` entry, no sticky stage, and unlike `cta` has room below it for the scroll. It carries the original assertion verbatim; measured 1:1 error is 0.58px. Verified on the full local stack, rebased onto main at 7863210a. Control first, on UNPATCHED main: 47 passed, 1 failed — and the one failure is this journey, so the lane is still red on current main and this is still the fix it needs. With the patch applied, two consecutive full suites: 49 passed, 0 failed. The spec alone, three consecutive runs: 7/7 each time. That also closes out the gradebook.spec.ts:35 question raised against the previous revision. It failed 2 of 3 full-suite runs then, always on a tree carrying an in-flight #553, and I could not tell a suite-context flake from a real intermittent in enrollment resolution. #553 has since merged; across the three full suites above it passed every time. It has not reproduced on current main, so there is nothing to file. Co-Authored-By: Claude Opus 5 --- frontend/e2e/landing-drag-field.spec.ts | 154 ++++++++++++++++++------ 1 file changed, 116 insertions(+), 38 deletions(-) diff --git a/frontend/e2e/landing-drag-field.spec.ts b/frontend/e2e/landing-drag-field.spec.ts index 7079cb25..df5a665e 100644 --- a/frontend/e2e/landing-drag-field.spec.ts +++ b/frontend/e2e/landing-drag-field.spec.ts @@ -69,11 +69,32 @@ async function openLanding(page: Page): Promise { * nothing left to autoscroll into and a 300px scroll assertion cannot be met. */ const CLUSTER = "4"; + /** - * The section that cluster lives in, so the journeys can ask the page what - * the cluster is welded to rather than assuming it is welded to nothing. + * The copy cluster 4 is welded to: the `faq` question column. + * + * Named here, NOT read out of the field's `data-drag-track`. Taking the + * reference from the product would make this journey follow the product: + * delete `faq` from `DragField.tsx`'s `TRACKS` and the clusters go back to + * sliding 374px out from under the words they annotate — the regression + * 8bb34869 fixed — but a test that re-derived its own expectation would + * re-frame itself along with it and stay green on exactly that. The oracle + * has to belong to the test. + * + * `faq` is the only section in `TRACKS`, and this column is `sticky; top:110`, + * so the section can scroll 300px while the copy moves ~202 — which is why + * measuring a cluster here against raw `scrollY` was wrong. */ -const CLUSTER_SECTION = "faq"; +const FAQ_SECTION = "faq"; +const FAQ_COPY = '[data-drag-anchor="faq"]'; + +/** + * A cluster in a section with no weld and no pin, where moving 1:1 with the + * document IS the contract. `newsletter` holds clusters 6 and 7, is absent + * from `TRACKS`, and — unlike `cta`, which sits ~120px from the end of the + * document — has room below it for a 300px scroll assertion. + */ +const UNTRACKED_CLUSTER = "6"; /** A satellite, not the course puck: the link spring pulls hardest on these. */ const SATELLITE = 1; @@ -126,27 +147,33 @@ async function probe(page: Page): Promise { } /** - * Where the page content a cluster is welded to currently sits on screen. + * The probe ring and the FAQ copy it is welded to, sampled in ONE frame. * - * A field names its weld target in `data-drag-track` (DragField.tsx's - * `TRACKS`), and `engine/sim.ts` translates the cluster so that it holds a - * fixed offset from it. `faq` is the one section that has one: its question - * column is `sticky; top:110`, so the section can scroll 300px while the copy - * the clusters belong to moves only ~202 and the clusters go with the copy. + * One `page.evaluate`, not two: the sim is still integrating when this runs + * (`SCROLL_QUIET_MS` expired long before), so sampling the node and its + * reference in separate round trips compares two different frames. * - * Read from the attribute rather than naming the column here, so this stays - * in step with the product: if the weld is ever dropped, this falls back to - * the field itself, which is the plain 1:1 page-scroll reference. + * The column is resolved the way `engine/sim.ts` resolves it — scoped to the + * field's own section, not `document`-wide — so this measures against the + * element the sim actually bound to. A global lookup would silently diverge + * from the product the moment a second match appeared earlier in the page. */ -async function weldTop(page: Page, section = CLUSTER_SECTION): Promise { - return page.evaluate((s) => { - const field = document.querySelector(`#${s} .drag-field`); - if (!field) throw new Error(`no drag field in #${s}`); - const sel = field.getAttribute("data-drag-track"); - const el = sel ? document.querySelector(sel) : field; - if (!el) throw new Error(`weld target ${sel} is not in the document`); - return el.getBoundingClientRect().top; - }, section); +async function probeAgainstCopy( + page: Page, +): Promise<{ x: number; y: number; copyTop: number; scrollY: number }> { + return page.evaluate(({ section, copy }) => { + const field = document.querySelector(`#${section} .drag-field`); + if (!field) throw new Error(`no .drag-field in #${section}`); + const el = (field.closest("section") ?? document).querySelector(copy); + if (!el) throw new Error(`${copy} is not inside #${section}`); + const b = document.querySelector("[data-e2e-probe]")!.getBoundingClientRect(); + return { + x: b.left + b.width / 2, + y: b.top + b.height / 2, + copyTop: el.getBoundingClientRect().top, + scrollY: window.scrollY, + }; + }, { section: FAQ_SECTION, copy: FAQ_COPY }); } test("nodes do not move of their own accord while the page scrolls", async ({ page }) => { @@ -375,28 +402,79 @@ test("a dropped node stays where it was put, and scrolls with the page", async ( // there. SWAY_PX is the envelope that buys. expect(Math.hypot(settled.x - dropped.x, settled.y - dropped.y)).toBeLessThan(SWAY_PX); - // Placed, not detached: it belongs to the page and moves with it. + // Placed, not detached: it belongs to the copy it was dropped over. // - // "The page" is the copy its cluster is welded to, NOT raw scrollY. The faq - // question column is `sticky; top:110`, so scrolling 300px moves the - // section 300px and the copy only as far as the pin allows — and the - // clusters go with the copy, which is the whole point of `TRACKS`. - // Measuring against scrollY asserted the one coupling this section - // deliberately does not have, and cost the node ~98px it never owed. - const weldBefore = await weldTop(page); + // Measured as an OFFSET that must not change, which is the idiom the + // act-tutor journey above already uses for the same invariant — no delta + // arithmetic whose sign only works out because the page happens to scroll + // down. The node's distance from the FAQ column is the thing being pinned. + const before = await probeAgainstCopy(page); await page.evaluate(() => window.scrollBy(0, 300)); await page.waitForTimeout(600); - const scrolled = await probe(page); - const travelled = (await weldTop(page)) - weldBefore; + const after = await probeAgainstCopy(page); + + // Preconditions first, most-basic first, so a failure names its own cause + // instead of sending the next reader to `TRACKS` and `syncClusters()`. + // `toBeCloseTo`, not `toBe`: `scrollY` is a browser-computed offset that is + // only integral because `deviceScaleFactor` happens to be 1. + expect(after.scrollY - before.scrollY, "the page should have scrolled") + .toBeCloseTo(300, 0); + // Signed, not `Math.abs`: scrolling DOWN must carry page content UP. A + // regression that translated the copy downward under a downward scroll + // would satisfy an absolute-value guard, and a node loyally following it + // would satisfy everything below. + // + // The journey can only tell "welded" from "pinned to the screen" while the + // copy still has travel left before its own pin — a node welded to an + // already-pinned column is stationary, and so is a detached overlay. That + // is a property of where `centreCluster` parks the cluster, not of the + // product, so assert it and say so rather than assume it. + expect( + after.copyTop - before.copyTop, + "the FAQ copy was already pinned at the drop position — this journey needs " + + "it mid-travel; check #faq's layout or centreCluster's offset", + ).toBeLessThan(-100); + + // ...and through all of that the node held its place against the copy. + expect(Math.hypot( + after.x - before.x, + (after.y - after.copyTop) - (before.y - before.copyTop), + )).toBeLessThan(SWAY_PX); +}); + +/** + * The other half of "belongs to the page", on a cluster where that means what + * it sounds like. + * + * The journey above deliberately measures against the FAQ copy, because that + * is what its cluster is welded to. Something still has to pin the plain + * case — a placed node in an untracked, unpinned section travelling exactly + * with the document — or the file loses the reference frame its own header + * (symptom 4) is about, and a cluster field that detached into a fixed + * overlay would have nothing left to catch it. + */ +test("a dropped node in an untracked section travels 1:1 with the document", async ({ page }) => { + await openLanding(page); + await centreCluster(page, UNTRACKED_CLUSTER); + const ring = await ringOf(page, SATELLITE, UNTRACKED_CLUSTER); - // Not a fixed overlay floating over the site: the copy it is welded to - // really did move under it. Without this the assertion below would pass on - // a node that is pinned to the screen and a page that never scrolled. - expect(Math.abs(travelled), "the weld target should have moved").toBeGreaterThan(100); - expect(scrolled.scrollY - settled.scrollY, "the page should have scrolled").toBe(300); + await page.mouse.move(ring.x, ring.y); + await page.mouse.down(); + await page.mouse.move(ring.x - 120, ring.y - 60, { steps: 20 }); + await page.mouse.up(); + await page.waitForTimeout(4_000); + const settled = await probe(page); + + await page.evaluate(() => window.scrollBy(0, 300)); + await page.waitForTimeout(600); + const scrolled = await probe(page); - // ...and the node went exactly that far, inside its sway. - expect(Math.hypot(scrolled.x - settled.x, scrolled.y - (settled.y + travelled))) + const moved = scrolled.scrollY - settled.scrollY; + expect(moved, "the page should have scrolled").toBeCloseTo(300, 0); + // No `TRACKS` entry and no sticky stage: this cluster owes the document the + // whole 300px. This is the assertion the faq journey used to carry, on the + // cluster where it is actually true. + expect(Math.hypot(scrolled.x - settled.x, scrolled.y - (settled.y - moved))) .toBeLessThan(SWAY_PX); });