Skip to content

Fix permission-resolution boundary spacing structurally (replaces #189)#192

Open
gnguralnick wants to merge 3 commits into
mainfrom
mngr/fix-permission-boundary-structural
Open

Fix permission-resolution boundary spacing structurally (replaces #189)#192
gnguralnick wants to merge 3 commits into
mainfrom
mngr/fix-permission-boundary-structural

Conversation

@gnguralnick

Copy link
Copy Markdown
Contributor

Replaces #189, which patched this at the wrong level.

Problem

When a permission request is granted/denied, the hidden grant/deny user message is treated as a turn boundary: the verdict is folded onto the permission card and a new progress block opens, carrying the open step over. That turn-boundary behavior is intended and correct (the agent really stops at the permission and resumes after approval).

But because that boundary has no user bubble, the card-ending block's margin-bottom: 28px and the resumption block's margin-top: 18px stack into a ~46px empty void in the flex-column message list — a spacing side effect of a message that is supposed to be invisible.

Why not the CSS approach (#189)

#189 zeroed the margin with .progress-block:has(> .pv--timeline > .pv-timeline-nodes > .pv-permission:last-child). That selector is a fragile proxy for "this turn ended on a permission resolution": it fires for a permission that is genuinely the last thing in an unresolved turn (collapsing a margin that should stay), and misses a resolution boundary if any work followed the card before the grant.

Fix (structural)

  • turn-grouping.ts: new SectionView.opened_by_permission_resolution, set on exactly the section the resolution branch opens.
  • conversation-rows.ts: when the next section carries that flag, the preceding (card-ending) ProgressBlock is told to drop its turn-bottom margin — keyed on the structural marker, not on the card's DOM position.
  • ProgressBlock.ts: applies progress-block--flush-bottom when asked.
  • style.css: .progress-block--flush-bottom { margin-bottom: 0; }.

The card, the trailing prose, the turn boundary, and the carried-over step are all unchanged; only the empty void collapses to a single ~18px turn break.

Verification

Measured in a real rendered build (Playwright, 1300x1600): boundary gap 46px → 18px. A permission that is the genuinely-last thing in an unresolved turn keeps its normal end-of-turn margin (the bug in #189's proxy).

Tests: a unit test asserting the flag is set on a resolution boundary and not on a normal user-bubble boundary; an e2e test that reproduces the real DOM shape (request card + a separate trailing-prose message producing a real .pv-final + hidden grant + carryover) and asserts the boundary gap, failing at 46px before the change and passing at 18px after.

🤖 Generated with Claude Code

…proxy

A hidden permission grant/deny opens a fresh progress block carrying the open
step over (the intended turn-boundary carryover). Because that boundary has no
user bubble, the card-ending block's turn-bottom margin and the resumption
block's top margin stacked into a ~46px empty void in the flex-column message
list.

This was previously patched (closed PR) with a CSS
`:has(.pv-permission:last-child)` selector -- a fragile proxy: it fires for a
permission that is genuinely the last thing in an unresolved turn, and misses a
resolution boundary if work followed the card before the grant.

Mark the boundary structurally instead: turn-grouping sets a new
`SectionView.opened_by_permission_resolution` flag on exactly the section a
resolution opens. conversation-rows reads the next section's flag and tells the
preceding ProgressBlock to drop its turn-bottom margin
(`progress-block--flush-bottom`), so the seam is just the resumption block's
normal ~18px top margin. The card, prose, and carried-over step are unchanged.

Tests: a unit test that the resolution section is flagged (and normal boundaries
are not), and an e2e test reproducing the real shape (request card + separate
trailing prose producing a real .pv-final + hidden grant + carryover) asserting
the boundary gap is a single ~18px break (fails at 46px before this change).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…flag)

Replaces the opened_by_permission_resolution boolean (and its five threading
points in turn-grouping: SectionView, SectionBuilder, newSection, ensureSection,
finalizeSection) with a derivation in conversation-rows. A shared
sectionRendersUserBubble() helper drives both the bubble-rendering decision and
the flush decision, so they can't drift; the preceding block drops its
turn-bottom margin whenever the next section renders no user bubble.

turn-grouping is back to clean (zero changes for this fix). Identical 18px
boundary, no new data-model state, and it generalizes to any bubble-less
boundary. Unit tests moved to conversation-rows.test.ts (helper + flush
decision, including the last-section / genuinely-final-permission case).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gnguralnick

Copy link
Copy Markdown
Contributor Author

Updated: simplified to a render-layer derivation.

The first commit added a SectionView.opened_by_permission_resolution boolean threaded through five points in turn-grouping.ts. That bit is fully derivable at the render layer: conversation-rows already decides whether each section renders a user bubble (every hidden user message is non-boundary, so the only bubble-less boundary section is a permission resolution). The follow-up commit removes the flag entirely — turn-grouping.ts is back to zero changes — and derives the flush in conversation-rows via a shared sectionRendersUserBubble() helper used by both the bubble decision and the flush decision (so they can't drift). Identical 18px result; no new data-model state; generalizes to any bubble-less boundary. Unit tests moved to conversation-rows.test.ts.

Replace ProgressBlock's flushBottomMargin prop (which described the block's
relationship to the next row) with an intrinsic isResumption marker: the block
declares only what it is (a turn resumed after a hidden, bubble-less boundary)
via a progress-block--resumption class. conversation-rows derives it from
s > 0 && !sectionRendersUserBubble(section); CSS owns the adjacency with
:has(+ .progress-block--resumption) { margin-bottom: 0 } keyed on the explicit
marker (not a DOM-position proxy), collapsing the gap after a preceding block of
any type. Identical 18px boundary.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gnguralnick

Copy link
Copy Markdown
Contributor Author

Updated: removed the successor-aware prop.

ProgressBlock's flushBottomMargin (which described the block's relationship to the next row) is replaced by an intrinsic isResumption marker — the block declares only what it is (a turn resumed after a hidden, bubble-less boundary) via a progress-block--resumption class. conversation-rows derives it from s > 0 && !sectionRendersUserBubble(section), and CSS owns the adjacency with :has(+ .progress-block--resumption) { margin-bottom: 0 }, keyed on the explicit marker (not a DOM-position proxy) and collapsing the gap after a preceding block of any type (so it also covers a preceding no-steps turn). Identical 18px boundary.

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.

1 participant