Skip to content

fix: stack clip regions so nested clips restore the parent rect#85

Open
natemoo-re wants to merge 5 commits into
mainfrom
nm/repro/nested-clip
Open

fix: stack clip regions so nested clips restore the parent rect#85
natemoo-re wants to merge 5 commits into
mainfrom
nm/repro/nested-clip

Conversation

@natemoo-re
Copy link
Copy Markdown
Member

@natemoo-re natemoo-re commented Jun 5, 2026

  • Fixes 🐛 Nested clip regions leak (clip is a single rect, not a stack) #77
  • Clip state was a single rect, so closing an inner clip turned clipping off entirely and later siblings leaked past the outer bounds (both vertically and horizontally)
  • Replaces the scalar clip rect with a fixed-depth stack (16): SCISSOR_START pushes intersect(parent, child), SCISSOR_END pops and restores the parent rect while the stack is non-empty
  • The active top mirrors into the existing clipx/clipy/clipw/cliph scalars, so setcell is untouched; depth-1 nesting reduces to the previous behavior; stack depth resets each frame
  • The regression lives in test/clip.test.ts under describe("clip") rather than a standalone file
  • Test plan: deno test green (8 passed); deno lint + deno fmt --check clean

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Jun 5, 2026

Open in StackBlitz

npm i https://pkg.pr.new/clayterm@85

commit: b5a355d

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

Size Increased — +0.5 KB

108.0 KB unpacked

Clip state was a single rect, so closing an inner clip turned
clipping off entirely and later siblings leaked past the outer
bounds. Replace it with a fixed-depth stack: SCISSOR_START pushes
intersect(parent, child) and SCISSOR_END pops, restoring the parent
rect while the stack is non-empty. The active top mirrors into the
existing clipx/clipy/clipw/cliph scalars, so setcell is unchanged.

Fixes #77
@natemoo-re natemoo-re force-pushed the nm/repro/nested-clip branch from 4300a52 to 4b35dc4 Compare June 5, 2026 06:14
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented Jun 5, 2026

Merging this PR will degrade performance by 54.46%

❌ 1 regressed benchmark
✅ 22 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation long input burst (200 bytes) 576.1 µs 1,264.9 µs -54.46%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing nm/repro/nested-clip (b5a355d) with main (48ff4bb)

Open in CodSpeed

@cowboyd
Copy link
Copy Markdown
Collaborator

cowboyd commented Jun 6, 2026

@natemoo-re This is a great addition. I've made a few tweaks:

  1. Visual tests. I've found that when investigating as a human, having a visual test for layout where possible really helps our little brains hone in on the context rapidly, so I proposed a few tests that accomplish the same thing, but instead of asserting against bytes, assert agains pictures.
  2. Specs To develop clayterm, I've been using spec-oriented development. I've found that it helps agents keep really precise project-wide context when making micro-code decisions. My usual workflow is research -> spec -> test -> implementation. I've updated the spec in this case, let me know what you think.

As for feedback, This seems like a very necessary addittion. The thing that I added to the spec that isn't currently present in the implementation is the behavior when we overflow the clip stack. Right now, it will silently corrupt every command that comes after the overflow. I've specified the behavior to add the constraint that even in overflow, the stack must be balanced, and also that in the event of overflow, it must be signaled back via the error channel that overflow occurred so that the front end can issue a warning.

In practice, I have no idea what kind of UI that would in any way be usable would also have a clip stack of 16, but hey 🤷🏻

Lemme know what you think!

@cowboyd
Copy link
Copy Markdown
Collaborator

cowboyd commented Jun 6, 2026

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation long input burst (200 bytes) 576.1 µs 1,264.9 µs -54.46%

Highly suspicious since this is an input parser benchmark, and the only code changes were to the render pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Nested clip regions leak (clip is a single rect, not a stack)

2 participants