Uh oh!
There was an error while loading. Please reload this page.
test(e2e): measure a dropped node against the copy it is welded to (#566) - #568
Merged
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe landing drag-field E2E tests now compare dropped nodes with FAQ copies during scrolling and verify that nodes in untracked sections move with the document. ChangesLanding drag-field scroll validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk:⚪ Minimal · up to This change corrects the drag-field E2E assertions for welded FAQ content and restores direct scroll coverage for newsletter content; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 9da3e4c | Commit Preview URL Branch Preview URL | Aug 22 2026, 07:32 PM |
This was referenced Aug 22, 2026
) 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 — 8bb3486 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 <noreply@anthropic.com>
…566) 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 8bb3486 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 7863210. 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 <noreply@anthropic.com>
AndresL230force-pushed
the
fix/566-drag-field-weld-frame
branch
from
August 22, 2026 19:30
5774111 to
9da3e4cCompareUh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
e2e (browser lane)red onmain, failing since the landing-v5 port (#524) merged on 2026-08-16.Closes#566.
The failure
frontend/e2e/landing-drag-field.spec.ts— "a dropped node stays where it was put, and scrolls with the page" — fails its scroll-follow assertion:Not flaky: three CI retries land within 0.2px of each other, and it reproduces identically across two CI runs six days apart and every local run.
Root cause
The product is right; the journey was wrong.
faqis the only section inDragField.tsx'sTRACKS, andengine/sim.ts::syncClusters()positions a tracked cluster atfield.top + a.top + dywheredy = (track.top - field.top) - trackTop. That collapses tocluster_y = track.top + const— the cluster is welded to the FAQ question column, which issticky; top:110.Instrumented over the journey's own 300px scroll:
scrollY#faqtoptop:110pin partwayThe column had exactly 202.81px of travel left before pinning, so ~97px of the scroll happened with the copy held still — and the cluster correctly held still with it. That weld is deliberate:
8bb34869added it because the clusters were sliding 374px out from under the words they annotate.The journey asserted raw
scrollYon the single cluster where 1:1 page coupling is deliberately untrue. It picked cluster 4 for elbow room, not knowingfaqis also the only tracked field.The change
Two commits — the second is the response to review, and it matters more than the first.
The oracle belongs to the test. The first attempt read the reference out of the field's own
data-drag-trackand fell back to the field when absent. That was self-defeating: deletingfaqfromTRACKSwould regress the product and re-frame the test to match, keeping it green on exactly the bug it exists to catch. Verified by mutation, not argument — emptyingTRACKSleaves the first version passing; against the landed version the offset drifts 96.69px and it fails, while the unmutated page sits at 0.45px. The column is now named in the spec (FAQ_COPY) and resolved section-scoped, the waysim.ts:296resolves it.The invariant is an offset that must not change, reusing the idiom the act-tutor journey already uses, instead of a signed delta that only worked because the page scrolls downward. Also from review: the "did the reference move" guard is signed (
< -100) rather thanMath.abs(...) > 100; preconditions run before the guard so a page that failed to scroll says so; node and reference are sampled in onepage.evaluaterather than two frames;toBeCloseTo(300, 0)rather than exact float equality on a browser-computed offset; and the deadsectionparameter is gone.The 1:1 case is restored where it is true. Re-framing the file's only page-relative assertion would have left nothing tying a cluster to raw document scroll, while the file header still teaches that symptom 4 survived every earlier test "because they all measured a cluster against its own field rather than against the page". So a new journey covers cluster 6 in
newsletter— noTRACKSentry, no sticky stage, and unlikectait has room below for the scroll. It carries the original assertion verbatim; measured 1:1 error is 0.58px.Verification
Rebased onto main at
7863210a.Control, on unpatched main: 47 passed, 1 failed — and the single failure is this journey. The lane is still red on current main, so this is still the fix it needs.
landing-drag-field.spec.ts×3Mutation test — the assertion is load-bearing, not vacuous:
faqdropped fromTRACKSThe new
newsletterjourney measures 1:1 error of 0.58px.tsc --noEmitandeslintclean.On the gradebook question raised against the previous revision:
gradebook.spec.ts:35failed 2 of 3 full-suite runs then, always on a tree carrying an in-flight #553, and the signature (theExamscategory, which exists only on the fall-2025 enrollment) matched "resolved the wrong enrollment" as readily as "rendered slowly". #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.Note the
e2e (browser lane)workflow is main-only, so this PR's own checks cannot exercise the lane it fixes — hence the local gate above.🤖 Generated with Claude Code
Summary by CodeRabbit