feat(plugin-page-builder): resist target switching near a zone boundary [DRAFT — design refuted, reworking] - #810
mobeenabdullah wants to merge 2 commits into
Conversation
With the pointer resting near a boundary between two drop zones, ownership flipped on every pixel of jitter, so the insertion line strobed and a drop landed wherever the pointer happened to be sampled. A margin rather than a dwell: a dwell charges latency on every deliberate move and puts wall-clock dependence into the canvas, while a margin costs nothing while the pointer moves decisively and is deterministic enough to assert a width on rather than wait for. Both default detectors report a value inversely proportional to the distance from the droppable's centre, so scaling by d / (d - band) applies the band in distance space for either without knowing which produced the number. Adding a constant cannot work: an inverse is non-linear, so a fixed bonus buys a pixel width that varies with how far the pointer already is. The constant is a bound on the DIFFERENCE of the two centre distances, which is twice the pointer movement it buys, because the pointer recedes from one centre while approaching the other. The test measures the achieved width instead of converting the constant, so the conversion is under test rather than assumed identically in both places.
…drop target Wires the banded detector into all three droppables the canvas registers: the gap zone, and a node's insert-before and append targets. Each wraps the default detector rather than replacing it, so the ranking stays whatever the library computed and the band is the single difference from stock behaviour. Only the candidate already holding the drag is weakened, read from the drag operation rather than tracked here, so there is no state to seed or clear when a drag ends. The detector is built once per module: an identity that changed per render would be reassigned on the droppable mid-drag. @dnd-kit/abstract and @dnd-kit/collision were already resolved as transitive dependencies; they are declared now because this package imports them directly.
|
@codex review |
|
Warning Review limit reached
Next review available in: 37 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 ignored due to path filters (2)
📒 Files selected for processing (5)
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 review |
![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: ba7022f85b
ℹ️ 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".
| type: BLOCK_TYPE, | ||
| accept: BLOCK_TYPE, | ||
| data: { kind: "dropzone", parentId, slot, index }, | ||
| collisionDetector: STICKY_COLLISION, |
There was a problem hiding this comment.
Once this detector keeps boundary jitter stable, both e2e/tests/canvas/acceptance.spec.ts:552-561 and e2e/tests/canvas/scenarios.spec.ts:437-448 still call test.fail(true, ...) immediately before asserting that no target transitions occurred. Playwright therefore reports the now-successful assertions as “Expected to fail, but passed,” so implementing this feature makes both root canvas E2E tests fail; update those markers and their stale capability annotations with the implementation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted, and thank you for catching it — this would have turned a working feature into two red root E2E tests.
test.fail(true, ...) inverts the result, so making the assertion pass reports "Expected to fail, but passed". Both acceptance.spec.ts:552-561 and scenarios.spec.ts:437-448 carry those markers.
Those files belong to the canvas lane, and I said in this PR that I would not edit them. That is still right, but it is not sufficient — my change makes their markers wrong, which is a coordination obligation rather than a licence to leave them broken. I am raising it with that lane so the markers come off in the same change that makes them stale, and #810 stays in draft until that is arranged.
| accept: BLOCK_TYPE, | ||
| disabled: dropBeforeIndex == null, | ||
| data: { kind: "dropzone", parentId, slot, index: dropBeforeIndex ?? 0 }, | ||
| collisionDetector: STICKY_COLLISION, |
There was a problem hiding this comment.
When append:<grid> becomes incumbent while the pointer is over grid padding or a gap and the pointer then enters a child, the grid append shape and the child's insert-before shape overlap, so both produce high-priority pointer-intersection collisions. If their centers coincide or are less than the 20px band apart, as in a single-cell or narrow nested grid, the challenger cannot become 20px closer by the triangle inequality; the boosted append target therefore remains active over the child and the drop appends inside the grid instead of inserting before that child. Avoid applying this sibling-boundary hysteresis between ancestor append and descendant insert-before targets, or bound it using the competing targets' geometry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted, and it is the composition problem rather than a tuning issue.
Your triangle-inequality argument is the decisive part: when append:<grid> is incumbent and the pointer enters a child, the two shapes overlap and both report high-priority pointer intersections. If their centres are less than the band apart — a single-cell or narrow nested grid — the challenger cannot close a 20px gap no matter where the pointer goes, so the append target is permanently sticky and the drop lands inside the grid instead of before the child. That is a functional regression, not a degraded margin.
It also confirms a distinction the canvas lane and I had already agreed on and which I then failed to implement: hysteresis is a SIBLING-boundary rule, and ancestor-versus-descendant is a different question that must be settled by geometry before any stickiness applies. Regions first, stickiness only among the survivors. I built the second half without the first.
This will be part of the rework rather than a separate guard, since bolting an exclusion onto the current shape would be a third rule interacting with the two that already disagree.
| value: bandedValue( | ||
| collision.value, | ||
| centreDistance(centre, pointer), | ||
| band | ||
| ), |
There was a problem hiding this comment.
For ShapeIntersection collisions, collision.value is intersectionRatio / distance, and the incumbent's ratio normally shrinks while the challenger's grows as the dragged rectangle crosses between gap zones. This scaling therefore compares ratioInc / (distanceInc - band) with ratioNew / distanceNew; it only reduces to a 20px center-distance advantage when the ratios happen to be equal, as they are in the unit fixture that hardcodes both scores as 1 / distance. With a tall source leaving one narrow gap zone while entering the next, the changing ratios can overcome the band after only a couple of pointer pixels, so the promised 8–12px margin and jitter resistance disappear; calculate the decision from the competing geometry or exercise the wrapper with real shape-intersection scores.
AGENTS.md reference: AGENTS.md:L183-L186
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed by measurement, and it invalidates this PR's headline claim. I have moved #810 back to draft rather than patch around it.
You are exactly right about the mechanism. For ShapeIntersection the value is intersectionRatio / distance, and the ratios are not equal between incumbent and challenger — the incumbent's shrinks as the source leaves and the challenger's grows as it enters. My scaling therefore compares ratioInc / (dInc - band) against ratioNew / dNew, which reduces to a distance band only when the ratios coincide.
And my unit fixture made them coincide, hardcoding 1 / distance for both sides. So the test asserted the one case the mechanism does not take: gap zones are zero-height, contain no point, and therefore never win pointer containment — they compete through shape overlap, which is precisely the path where the original [1,2,1,2] flip was measured. The fixture never reached the mechanism it was written to cover, on the claim the whole change rests on.
Measured over realistic geometries with the band at 20:
| source h | zone h | centre sep | achieved |
|---|---|---|---|
| 200 | 8 | 40 | 10.01px |
| 60 | 8 | 40 | 8.22px |
| 20 | 8 | 40 | 6.01px |
| 60 | 40 | 60 | 4.48px |
The requirement is 8-12px of pointer movement. 4.48px is below the floor, so this does not meet it — the "measured 10.01px" I put in the PR description is true only of the equal-ratio case my fixture constructed.
The fix is the one you name: decide from the competing GEOMETRY rather than by scaling a value that conflates ratio and distance. That is a design change, not an adjustment, so I would rather rework it than tune the constant — a constant chosen to rescue the worst geometry would overshoot every other one.
@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: |
mobeenabdullah
changed the title
github-actions
Bot
added
type: docs
|
Closing this rather than leaving it open, since the approach it implements has been refuted by measurement and none of the code here survives into the fix. The branch is preserved and this is reopenable if that turns out to be wrong. Why it cannot be finished as written. The band scales Asking what both attempts shared, rather than what each did: both assumed the ranking can be adjusted by modifying the score. Two further defects found here, both real and both carried forward:
Nothing is lost. The refutation, the measurements, the agreed rework and the constraint that forces container ownership into stage one are in The rework is one detector: stage 1 resolves candidate regions by geometry and container ownership — identical rects at a nested boundary defeat geometry alone — and stage 2 applies stickiness only among survivors with the same resolved intent, with the band computed from competing geometry rather than from a score. It starts once |
1 participant
Caution
This PR's central claim was refuted by measurement. It is in draft and the approach is being reworked. Do not cite the numbers below without reading this box.
WITHDRAWN: "measured 10.01px". That figure is true only of the fixture that produced it.
shapeIntersectionreportsintersectionRatio / distance, and the two ratios are not equal — the incumbent's shrinks as the source leaves while the challenger's grows as it enters. Scaling the value byd / (d - band)therefore reduces to a distance band only when the ratios coincide, and the unit fixture made them coincide by hardcoding1 / distancefor both sides.That is the pointer-containment shape.
Correction to an earlier version of this box: I wrote that gap zones "are zero-height, contain no point and never win containment". True at rest, wrong during a drag —
.nx-pb-dropzone[data-drag]{height:6px;margin:3px 0}onmain, so a zone is 6px tall mid-drag and does contain the pointer. The collision type therefore depends on pointer position: inside a zone's band it ispointerIntersection(1/d, no ratio); in the 6px gap between two zones it isshapeIntersection(ratio/d, ratios unequal).The refutation stands, because a boundary is exactly where no zone contains the pointer — so hysteresis operates in the unequal-ratio path and the measurements below hold. But the mechanism is a mix of both types, which is worse than the original claim:
sortCollisionsranks TYPE above value, so a pointer crossing from the gap into a zone changes type and the band is never consulted at all. Any band expressed invalueis silently bypassed on that transition.Measured across realistic geometries at
band = 20:Requirement is 8-12px of pointer movement. The floor is breached, so the requirement is not met.
Two further defects, both real:
append:<grid>is incumbent and the pointer enters a child, the shapes overlap and both report high-priority pointer intersections. If their centres are closer than the band, the challenger cannot close the gap by the triangle inequality — the append target is permanently sticky and the drop lands inside the grid instead of before the child. A functional regression, not a degraded margin.acceptance.spec.ts:552-561andscenarios.spec.ts:437-448carrytest.fail(true, ...)on the hysteresis assertions.test.failinverts the result, so any correct implementation turns both root E2E specs red with "Expected to fail, but passed". Their removal must land in the SAME change as the implementation — there is no ordering of two PRs that avoids a redmain.Five mutations passed on the original file and none of them caught this, because each mutated the implementation while the fixture stayed in the equal-ratio case. Mutation testing checks the code against the test; it cannot tell you the test is aimed elsewhere.
The rework, agreed with the canvas lane: one detector, not two. Resolve candidate regions by geometry first (pointer offset within the rect along the drag axis, a pixel-width leading strip deciding before-versus-append), then apply stickiness only among survivors carrying the same resolved intent, with the band computed from the competing geometry rather than by scaling a score that conflates ratio and distance. Width asserted in pointer pixels via
dragToInsetInZone(#806), never inferred from a score.The defect
With the pointer resting near a boundary between two drop zones, ownership flipped on every pixel of jitter — measured by the canvas lane as
[1,2,1,2,1,2,...]across a 2px oscillation. The insertion line strobed between two targets and a drop landed wherever the pointer happened to be sampled. It had been passing only because no fixture put the pointer near a boundary.A margin, not a dwell
Plan 04 permits either an 8-12px distance margin or a >100ms dwell.
A dwell charges latency on every deliberate move and puts wall-clock dependence into the canvas — which is the failure class this area has repeatedly produced. A margin costs nothing while the pointer moves decisively, resists only near a boundary, and is deterministic enough to assert a width on rather than wait for.
Three facts read from
@dnd-kitsource, not assumedThe first design here was wrong, and these are why.
1.
Collision.valueis a reciprocal.pointerIntersectionreturns1 / distance;shapeIntersectionreturnsintersectionRatio / distance. A fixed bonus therefore buys a pixel width that varies non-linearly with how far the pointer already is — it cannot express "8-12px" at all.2.
sortCollisionsorders priority → TYPE → value, not priority → value. Containment (PointerIntersection/High) beats overlap (ShapeIntersection/Normal) before value is consulted. So a value-space band damps same-priority, same-type switches only. That is the correct scope — entering a zone outright should take effect at once — but it has to be stated rather than discovered.3. So the band is applied in distance space and converted back through the library's own formula. Both detectors have value ∝ 1/d, so one expression covers both without branching on type:
The library's number carries whatever else it encodes; the only difference from stock is one subtraction.
The constant is not in pixels of pointer movement
The comparison reduces to
dIncumbent − dChallenger < band, so the constant bounds the difference of centre distances. For two zones the pointer travels between, movingxpast the boundary lengthens one distance byxand shortens the other byx— the difference grows at twice the pointer's rate.TARGET_SWITCH_BAND_CENTRE_DELTA_PX = 20therefore buys ~10px of pointer movement. A constant of 10 would read as "10px" and deliver 5px, under the requirement's floor while appearing inside its range.The test measures the achieved width rather than converting the constant, so the conversion is under test instead of being assumed identically in two places. Measured: 10.01px.
Verification
bandedValueand the width are covered by unit tests; 5 mutations each fail the intended assertion:Infinityguard removedInfinitymatters:pointerIntersectionhas no zero guard, so the pointer on a centre yields exactlyInfinity. Scaling it would return a finite number, leaving the incumbent weaker at the position where it is most clearly correct.Package suite: 665 passing, typecheck and lint clean.
Scope
All three droppables the canvas registers are wired — the gap zone plus a node's insert-before and append targets.
@dnd-kit/abstractand@dnd-kit/collisionwere already resolved transitively; declared now because this package imports them directly.Not included: the end-to-end width assertion in the real canvas. That needs
dragToInsetInZonefrom #806 to place the pointer at a known depth;dragUntilInsideZonestops at the first step inside a zone, which is where a correct margin implementation has deliberately not switched. The acceptance target ine2e/tests/canvas/acceptance.spec.tsbelongs to the canvas lane and is untouched here.