Replace ProgressBlock successor-aware margin prop with intrinsic resumption marker#198
Draft
gnguralnick wants to merge 10 commits into
Draft
Replace ProgressBlock successor-aware margin prop with intrinsic resumption marker#198gnguralnick wants to merge 10 commits into
gnguralnick wants to merge 10 commits into
Conversation
Apply only the CSS fix to the live (FastAPI) workspace, scoped to just the spacing change the user approved. The regression test on mngr/fix-permission-spacing targets the new Flask test harness on origin/main and rides along in the upstream PR rather than the live build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g scenario A UI fix delegated to a worker validated against a fixture it invented rather than the real conversation that motivated the change, producing a CSS selector that never matched the real DOM plus a passing-but-meaningless test. Three process gaps closed: - Delegate: the lead must capture the real motivating DOM/measurements and pass them in the brief as the worker's reproduction fixture and acceptance target. - Preview: the preview opens on the worker's empty agent, not the motivating conversation; the lead must open the real agent, re-measure, and confirm the number actually moved before asking the user. - Worker sub-skill: the fixture must reproduce the lead-provided DOM shape (not a guess), assert that shape, and the regression test must fail-before/pass-after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The teardown section ran `unpreview` (which kills the preview servers and deregisters their services) but never closed the `si-preview` dockview tab opened earlier via `layout.py open`. After teardown the user was left with a stale tab pointing at a now-deregistered service. Add an explicit `layout.py close si-preview` step, framed as a separate concern (layout panel vs. service lifecycle), to run on every teardown path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit ff7d110)
…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. Previously this was patched 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. Removes the old CSS rules and adds tests: a unit test that the resolution section is flagged (and normal boundaries are not), and an e2e test that reproduces the real shape (request card + separate trailing prose + hidden grant + carryover) and asserts 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> (cherry picked from commit 906fb54)
Replace the threaded SectionView.opened_by_permission_resolution flag (removed from turn-grouping and its 5 threading points) with a derivation in conversation-rows: a shared sectionRendersUserBubble() helper drives both the bubble-rendering decision and the flush decision, so the preceding block's turn-bottom margin is dropped whenever the next section renders no user bubble. Identical 18px boundary, no new data-model state, generalizes to any bubble-less boundary. (Live branch drops the Python e2e test; it lives in the upstream PR.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mption marker
ProgressBlock took a flushBottomMargin prop set on the PRECEDING (card-ending)
block describing its relationship to the next row -- an inter-row layout concern
leaking into the component API, and one that only covered a preceding block with
tk steps. Refactor so the boundary block is marked for what it intrinsically is.
- ProgressBlock: drop flushBottomMargin / progress-block--flush-bottom; add an
isResumption attr that tags the root with progress-block--resumption.
- conversation-rows: mark the resumption section's own block via
isResumption = s > 0 && !sectionRendersUserBubble(section); no successor
lookahead.
- style.css: replace the flush-bottom rule with
:has(+ .progress-block--resumption) { margin-bottom: 0 } -- keyed on an
explicit intentional marker (not a DOM-position proxy) and collapsing the gap
after a preceding element of any type.
- Update the conversation-rows unit tests to assert isResumption, and re-add the
rendered-boundary e2e test keyed on the new marker (fail-before 46px /
pass-after 18px).
Identical 18px boundary; card/prose/carryover unchanged; a final unresolved
permission keeps its end-of-turn margin.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace ProgressBlock's flushBottomMargin prop (which described the block's
relationship to the next row) with an intrinsic isResumption marker: the block
now 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) and 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>
…r-cleanup # Conflicts: # apps/system_interface/imbue/system_interface/test_e2e.py
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.
Summary
Refactors the permission-resolution boundary spacing fix so the boundary block is marked for what it intrinsically is, rather than the preceding block carrying a successor-aware prop.
ProgressBlock.ts: removed theflushBottomMargin?: booleanattr (andprogress-block--flush-bottomclass) — a component should not take a prop about its successor. AddedisResumption?: boolean, which tags the root withprogress-block--resumption.conversation-rows.ts: dropped thenextHasNoBubblelookahead; the current section's block is markedisResumption = s > 0 && !sectionRendersUserBubble(section).style.css: replaced the flush rule 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 element of any type.conversation-rows.test.ts) assert the new attr; the rendered-boundary Playwright e2e test (test_e2e.py) was re-added keyed on the new marker, with a shared_serve_agent_chathelper.Net effect: identical ~18px boundary; card/prose/carryover unchanged; a final unresolved permission keeps its end-of-turn margin.
Verification
Branch includes a merge of latest
origin/main(FastAPI→Flask migration); the e2e fixture was reconciled to main's threaded-server API.🤖 Generated with Claude Code