test(builder): fix the canvas acceptance geometry faults - #736
Conversation
startDragAt is contractually allowed to move past the drag threshold, and the PoC driver shifts 12px doing so. The delta was computed from the source point, so every panel drag overshot by exactly that much -- and a replacement driver with a different activation motion would overshoot by a different amount, which is the seam this suite exists to keep swappable. Asking the driver where the pointer actually is keeps the gesture landing in the same place whichever driver is behind it.
… shortfall readBlockBoxes measures inside the iframe, so its rects are frame-local, while the pointer moves in host coordinates. The collision case used one as the other -- off by the frame's origin and wrong again by its scale. At 100% zoom with the frame near the top-left the two are close enough to look correct, which is how it survived. The containment assertion had the same fault from the other side: it compared a host pointer against a frame-local bottom edge, a question neither coordinate answers. Correcting them retracted a shortfall I had recorded against the canvas. This case was marked as an expected failure because descending from nx-inner appeared to find no active zone until past the container's own bottom edge. That measurement was taken with the unconverted coordinates, so the pointer was never inside the container it was meant to be in. Converted, the canvas resolves to the innermost container and the case passes. The shortfall was the harness.
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughCanvas end-to-end tests now share incremental pointer dragging, use a tall scrolling fixture, expand nested collision checks, validate overflow before autoscroll tests, and measure drag-specific performance against idle movement. ChangesCanvas acceptance coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to The PR corrects drag coordinates and frame-offset calculations, but the nested-region acceptance test can still stop before traversing the full region and allow an incorrect geometry implementation to pass. Merge readiness is moderate until the test derives its movement budget from the mapped region or asserts full coverage; the remaining helper and benchmark issues are bounded follow-ups. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
@codex please review this PR |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26a827a1f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…slowest move Autoscroll was measured against NESTED_FIXTURE, roughly 400px of authored height in a 1400px viewport. The canvas had no scroll range at all, so canvasScrollTop could not change and the target could not pass however correctly autoscroll were implemented. TALL_FIXTURE overflows deliberately and the test asserts the overflow before measuring, so a fixture that stopped overflowing fails loudly instead of reporting a missing behaviour. Its own fixture rather than borrowing LARGE_FIXTURE: that one is sized for a perf budget, and tuning its block count for timing would silently take the scroll range away. The perf budget seeded 500 blocks and never checked they RENDERED, so a canvas that mounted six of them was timed while the test claimed to measure a large tree. It reads the rendered boxes first. And it averaged. One 2-second stall among twenty fast moves means a comfortable mean while the editor visibly locks up, which is exactly the shape a canvas that re-measures the tree produces when a rect cache misses. Every move is timed individually now and the SLOWEST has to sit under the budget.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7469d4f8e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two previous versions computed a coordinate inside nx-inner and moved there. Starting a drag expands every gap zone from zero height, so the layout that coordinate described no longer existed by the time the pointer arrived, and how far it shifted depended on load -- which is why it resolved to the inner container locally and to the root on CI. Capturing the rects after activation narrowed it and did not close it. Descending until the OWNER is the nested container asks the question directly instead of predicting where the answer will be. Three consecutive full runs hold. The performance budget is now differential. Absolute wall clock includes the Playwright round trip and runner scheduling, so a single GC pause read as a re-measured tree. Twenty moves are timed with no drag running as a control, twenty with one live, and the budget applies to the difference of the medians: the transport cost is common to both and cancels, and a median rather than a max stops one outlier deciding the result. The panel-drag delta correction moves into a shared dragPointerTo. Three suites carried their own copy of that arithmetic and each had the same fault, so fixing one left two.
|
Both P1s and the two P2 measurement findings are fixed. Detail per thread below; three things are worth stating at PR level. The collision case: you were right, and correcting it also invalidated a retraction I had made. I had marked this an expected failure, then un-marked it after fixing a coordinate conversion, on the strength of a green local run. CI disagreed — Capturing after activation narrowed it and did not close it. What closed it was giving up on aiming: the case now descends until the OWNER is the nested container rather than predicting where that will be. Three consecutive full runs hold. The performance budget is differential now, which is the substance of your point. Absolute wall clock includes the protocol round trip and runner scheduling, so a single GC pause read as a re-measured tree — and that was a fault I introduced in this PR by switching from a mean to a max. It now times twenty moves with no drag as a control, twenty with one live, and applies the budget to the difference of the medians. The transport cost is common to both and cancels; the median stops one outlier deciding it. I deleted a test and caught it by counting. Replacing the perf block by line range consumed the adjacent One case remains intermittent: |
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ee09b743b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…egion Restoring the undo case by line range spliced its BODY into the performance test without its own wrapper, so a passing budget assertion then marked the performance case expected-to-fail and threw from the undo reader. The undo test now stands on its own and the performance test ends at its assertion. The depth loop exited on the first sample that agreed, which passes an implementation resolving correctly at one depth while the outer container wins everywhere else in the same region. It now samples every active zone while the pointer is inside the region and requires all of them to be owned by it. Comments describing the code's history are removed again. The rule is that a comment explains the code; where a correction came from belongs in the commit message and the task file.
|
@codex please review this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
e2e/tests/canvas/driver.ts (1)
303-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the remediation history from the doc comment.
Lines 303-304 describe how three suites previously duplicated the arithmetic and how a partial correction behaved. That is past work, not behavior of this helper. Keep the rationale that explains the code: the delta must come from the post-activation pointer, and the motion is stepped rather than a single jump.
Based on learnings: source-file comments should document behavior and the rationale for non-obvious logic only, and should avoid references to tasks, conversations, review findings, or historical remediation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/canvas/driver.ts` around lines 303 - 304, Remove the remediation-history sentences from the doc comment near the shared arithmetic helper. Retain only the behavioral rationale: calculate the delta from the post-activation pointer and perform the motion in steps rather than as a single jump.Source: Learnings
e2e/tests/canvas/acceptance.spec.ts (1)
415-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating the viewport height as a literal.
The acceptance assertion and fixture rationale both encode
1400independently. If the Playwright viewport changes, these values can become stale without a targeted failure. Read the configured viewport height or expose one shared constant, and use it in bothe2e/tests/canvas/acceptance.spec.tsande2e/tests/canvas/fixtures.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/canvas/acceptance.spec.ts` around lines 415 - 418, The viewport height is duplicated as 1400 across the canvas test and fixture. In e2e/tests/canvas/acceptance.spec.ts lines 415-418, update the authored-height assertion to read the viewport height from the page or shared project configuration; in e2e/tests/canvas/fixtures.ts lines 122-136, express the TALL_COUNT * TALL_BLOCK_HEIGHT sizing relationship using that same shared viewport-height source or export it for reuse, so both locations remain synchronized. Apply the same fix in `@e2e/tests/canvas/fixtures.ts` around lines 122 - 136: The fixture comment and sizing rationale duplicate the same viewport-height assumption.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/canvas/acceptance.spec.ts`:
- Around line 484-509: Use the same pointer movement delta in the idle control
loop and the dragging loop around the median calculation: update the idle loop’s
4px movement to match the dragging loop’s 12px movement, while leaving the
sampling and canvasCost calculation unchanged.
- Around line 154-165: Update the descent loop in the canvas acceptance test to
derive its iteration limit from the mapped region height (`inner.height *
scale`) or otherwise assert after the loop that the pointer passed `bottom.y`,
ensuring the test samples the entire nested region. Also capture `active` and
`nearest` for every non-null sample rather than only when `active < 0`, so the
nearest-zone assertion validates the full descent.
In `@e2e/tests/canvas/driver.ts`:
- Around line 309-321: Update dragPointerTo to validate steps >= 1 immediately
at the start of the helper, before reading the pointer or entering the movement
loop, and fail when the precondition is violated.
In `@e2e/tests/canvas/scenarios.spec.ts`:
- Around line 52-55: Update the drag setup around startPanelDrag/startDragAt to
use the default step count by removing the explicit 1 argument, and delete the
duplicated helper rationale comment. Preserve the subsequent stepped
dragPointerTo movement unchanged.
---
Nitpick comments:
In `@e2e/tests/canvas/acceptance.spec.ts`:
- Around line 415-418: The viewport height is duplicated as 1400 across the
canvas test and fixture. In e2e/tests/canvas/acceptance.spec.ts lines 415-418,
update the authored-height assertion to read the viewport height from the page
or shared project configuration; in e2e/tests/canvas/fixtures.ts lines 122-136,
express the TALL_COUNT * TALL_BLOCK_HEIGHT sizing relationship using that same
shared viewport-height source or export it for reuse, so both locations remain
synchronized.
Apply the same fix in `@e2e/tests/canvas/fixtures.ts` around lines 122 - 136: The
fixture comment and sizing rationale duplicate the same viewport-height
assumption.
In `@e2e/tests/canvas/driver.ts`:
- Around line 303-304: Remove the remediation-history sentences from the doc
comment near the shared arithmetic helper. Retain only the behavioral rationale:
calculate the delta from the post-activation pointer and perform the motion in
steps rather than as a single jump.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3a4decb-e1d1-41d5-b0fd-d412fecbea80
📒 Files selected for processing (4)
e2e/tests/canvas/acceptance.spec.tse2e/tests/canvas/driver.tse2e/tests/canvas/fixtures.tse2e/tests/canvas/scenarios.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1564d088f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nd-kit uses The depth descent stopped on an iteration limit rather than at the far edge of the nested region, so a canvas resolving depth correctly near the top and letting the outer container win lower down passed. The step count now follows from the mapped span and the exit is asserted from the pointer's position. Both edges of the nested region are excluded by one drop-zone height: the insertion gap at a boundary belongs to the outer container, so a sample landing there is a position both containers can claim. The zone assertion followed "the nearest zone wins", which is not a rule this canvas follows. `@dnd-kit/collision` resolves pointer intersection first and falls back to the dragged shape's overlap only when the pointer is inside no zone, so next to a boundary the nearest zone and the resolved zone legitimately differ. Measured: one sample of 28 diverged by one ordinal. It is now exact where a zone contains the pointer and bounded to one ordinal elsewhere, and the descent steps by half the shortest zone so it cannot step over a zone and leave the exact half with nothing to check. The perf control moved 4px per step against the drag's 12px and over a different element, so part of the difference it subtracts was distance and position rather than drag work. Both samples now differ only in whether a drag is live. Also: `dragPointerTo` rejects a step count below one instead of resolving without moving, the tall fixture derives its ids once for both the rendered nodes and the declared list, and three doc comments describing code that is no longer there are removed.
… probes a precondition The depth descent computed the nested region once and then asserted against it for the whole traversal. Gap zones expand when the drag activates and the canvas reflows as they do, so the span moved underneath the descent: the pointer left the container while assertions written against the stale span still called it inside, and the outer container was recorded owning a zone that contained the pointer. Two runs in six. The region is now re-measured every sample, and the runaway guard is bounded by the whole document rather than by a span that grows. Ownership is asserted where the collision rule is unambiguous — where the pointer is inside a zone — because under the shape-overlap fallback the ancestor's insertion gap before the container is a legitimate candidate. What that does not establish is depth priority, and the comment says so: this canvas registers no collision priority at all, so the nested container wins by being the zone under the pointer rather than by being the deeper one. Both indicator probes started a panel drag and read an indicator without establishing that one exists. The reader raises only when it finds an indicator it cannot model and answers a count of zero when the canvas draws none, so the refusal being asserted was contingent on where the drag happened to stop. Measured, it resolved with a count of zero and the missing indicator read as a canvas that had gained the capability. `puts the indicator in the gap the pointer is over` no longer expects to fail. Its reason was not true — the driver maps the indicator's frame rect into host coordinates, so it is comparable with the pointer. What kept it red was the unestablished precondition paired with a bound tighter than the fault it names: a correct indicator 31px from the pointer was rejected by a 24px constant while the blocks it must not trail are ~92px apart. The bound is now the shortest block in the tree under the drag.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e2445f091
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The comment recounted the marking the test used to carry, why it previously stayed red and the constant it replaced. Only the current rationale remains: why the bound comes from the tree under the drag, and why the rect is comparable with the pointer.
The collision case passes, and what it measures is weaker than the property it is named for. Depth priority decides which container wins when both could claim the pointer; this canvas has no such rule, and dnd-kit's default detector ranks by pointer containment then by the dragged shape's overlap, neither of which takes depth as an input. The case passes because the nested container's own gap zones cover its interior, so the container under the pointer is also the owner of the zone under it. Separating the two needs a position where an ancestor's zone competes inside a descendant, and this fixture offers none: every ancestor zone lies outside the nested container's box. The annotation now records that as a shortfall, which keeps the plan point off the covered list without marking an expected failure that would report red for a canvas answering every position correctly. Also removes run tallies from four comments, and a mechanism one of them named that the live re-measurement showed was not the cause.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a014efe30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…containment unconditionally The containment and nearest-zone readers each queried and mapped the drop-zone geometry themselves. Two readings of one thing agree until a selector, an ordering, the frame origin or the mapping is corrected in one and not the other, and the depth probe then compares two different models of the canvas: either a failure naming the canvas for a fault in the harness, or a pass that is self-consistent and wrong. Both now derive from one mapped snapshot. Containment was recorded only where a target had already activated, which drops exactly the samples that matter. A canvas missing most of its zones while activating one leaves every miss out of the record, and the samples that survive all agree. It is measured at every sample inside the region now, and a zone that contains the pointer without activating a target is a failure rather than a row that leaves the set. The indicator bound came from `Math.min()` over the blocks below the root, which is `Infinity` when only the root is measured — a child selector that stopped matching, or a replacement driver reporting less. The precondition passed on it and the distance assertion then accepted an indicator anywhere on screen. The children must exist, and the derived bound must be a real distance.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
PR #709 merged with 19 review threads open. Eight described work that had already landed in the same merge; eleven were real defects on
main, recorded intasks/left-tasks/2026-08-12-1620-canvas-acceptance-findings-merged-unaddressed.md.This closes the two geometry faults, which were ranked first because every drag-based case sits on them.
The drag delta was measured from the wrong point
startDragAtis contractually allowed to move past the drag activation threshold, and the PoC driver shifts 12px doing so.dragFromPanelcomputed its delta from the source point, so every panel drag overshot by exactly that — and a replacement driver with different activation motion would overshoot by a different amount, which is precisely the seam this suite exists to keep swappable.It now measures from
driver.pointer()after activation.Frame-local rects were used as host coordinates
readBlockBoxesmeasures inside the iframe. The pointer moves in host space. The collision case used one as the other — off by the frame's origin, and wrong again by its scale.At 100% zoom with the frame near the top-left the two are close enough to look correct. That is how it survived review. The containment assertion had the same fault from the other side, comparing a host pointer against a frame-local bottom edge.
Both now convert through
mapFramePointToHost, which exists for exactly this.Correcting it retracted a shortfall I had recorded against the canvas
The collision case was marked an expected failure, on the reasoning that descending from
nx-innerfound no active zone until y=384 — past the container's own bottom edge at 320. I concluded the canvas creates gap zones at the outer level only.That measurement was taken with the unconverted coordinates. The pointer was never inside the container it was meant to be in. Converted, the canvas resolves to the innermost container correctly and the case passes.
The shortfall was the harness. That is the second target I marked against this canvas that turned out to be my own measurement — the first was the hysteresis case, where the transition log's baseline entry was read as a flip.
Both retractions matter beyond this PR: a false expected-failure puts a target in front of B-6/B-7 that the canvas already meets, and work would have been spent "fixing" behaviour that was correct.
Verification
canvas/suite: 40 passed.check-typesandlintclean one2e.One case,
draws exactly one insertion indicator, in host chrome, remains intermittent — 39/40 on one run, 40/40 on the next. That is pre-existing and not touched here. What has been ruled out by measurement, and why the next attempt should treat it as a modelling question rather than a timing one, is written up in the task file.Summary by CodeRabbit