test(e2e): three more acceptance properties — depth, refusal, autoscroll - #1109
Merged
Merged
Conversation
Takes the canvas suite from seven of twelve to ten. Each needed a fixture that could express the property at all; none needed a change to the canvas. ## A1 — collision resolves by tree depth The fixture gains a `core/section` holding a text block, so a point is over two containers at once. On a flat document every drop resolves at the root and a canvas ignoring depth entirely passes, which is why this was deferred rather than asserted. The control drags over a ROOT-level block and requires the answer to be the root, so a canvas that always answered "deepest" is not passing by accident either. ## A7 — the invalid-target state `core/columns` accepts only `core/column`, which is the one structural refusal in the core set. Two halves, because a refusal shown nowhere and one shown everywhere pass each other's test. Reaching it took two corrections, both measured. An EMPTY column renders zero pixels tall — top 337, height 0 — and a region with no height contains no pointer. And `core/column` itself accepts a text block, so anywhere over the column resolves to the COLUMN's slot and never meets the restriction: the refusal lives on the ROW. The row is padded so a band of it is not covered by its child, which is the only place the restriction is reachable. ## A8 — autoscroll Needed a document taller than its box; before, `scrollHeight` equalled `clientHeight`, so "it did not scroll" and "there was nowhere to scroll" were the same observation. The test asserts the overflow exists BEFORE asserting the scroll, and that the scroll stops at the bound. The 900px spacer is LAST for a reason worth keeping: placed earlier it pushed the nested containers below the fold, where a pointer cannot reach them and a drop over a nested block resolved to nothing at all. ## What the manifest guard caught Adding nodes broke the population assertions in both specs before any property ran, which is the guard working. It also caught the fixture change breaking five previously-passing tests: the tall spacer put their drag source below the fold, so they now drag from a block above it. And A10's hardcoded "original order" list was replaced by a comparison against what it captured, which cannot go stale when the fixture gains a node. A9 and A11 remain deferred with their reasons, printed by every run.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe canvas fixture now contains nested sections, text, columns, and overflow. Acceptance tests cover nested-region resolution, restricted column drops, undo order, cancellation, and autoscroll. The acceptance manifest marks properties 1, 7, and 8 as covered. ChangesCanvas acceptance coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AcceptanceTests
participant CanvasHarness
participant NestedRegionResolver
participant ColumnsContainer
participant ScrollContainer
AcceptanceTests->>CanvasHarness: start drag over seeded content
CanvasHarness->>NestedRegionResolver: resolve drop region
NestedRegionResolver->>ColumnsContainer: validate nested drop target
ColumnsContainer-->>AcceptanceTests: accept or reject drop
AcceptanceTests->>ScrollContainer: hold drag near bottom edge
ScrollContainer-->>AcceptanceTests: update scroll position to bound
Possibly related PRs
Suggested labels: ✨ 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 |
Contributor
Fallow audit reportFound 37 findings. Details
Generated by fallow. |
Contributor
PR title fails Conventional Commits checkExamples of valid titles:
|
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 free
to 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.
Takes the canvas acceptance suite from 7 of 12 to 10 of 12. Each property needed a fixture that could express it at all; none needed a change to the canvas.
A1 — collision resolves by tree depth
The fixture gains a
core/sectionholding a text block, so a point is over two containers at once. On a flat document every drop resolves at the root and a canvas ignoring depth entirely passes — which is why this was deferred rather than asserted.The control drags over a root-level block and requires the answer to be the root, so a canvas that simply always answered "deepest" isn't passing by accident either.
A7 — the invalid-target state
core/columnsaccepts onlycore/column— the one structural refusal in the core set. Both halves, because a refusal shown nowhere and one shown everywhere pass each other's test.Reaching it took two corrections, both measured rather than reasoned:
core/columnitself accepts a text block, so anywhere over the column resolves to the column's slot and never meets the restriction — the restriction lives on the rowThe row is padded so a band of it isn't covered by its child. That band is the only place the refusal is reachable.
A8 — autoscroll
Needed a document taller than its box. Before,
scrollHeightequalledclientHeight, so "it did not scroll" and "there was nowhere to scroll" were the same observation. The test asserts the overflow exists before asserting the scroll, and that the scroll stops at the bound.The 900px spacer is last on purpose: placed earlier it pushed the nested containers below the fold, where a pointer cannot reach them and a drop over a nested block resolved to nothing at all.
The guard did its job three times
Verification
playwright test canvas/→ 51 passed, exit 0. Lint, e2e typecheck and playground typecheck clean.A9 and A11 stay deferred with their reasons, printed by every run:
getBoundingClientRectfrom the test instruments the page rather than the engine, and the frame-rate half is deliberately not a merge gate.No changeset: test-only.
Summary by CodeRabbit
New Features
Bug Fixes