Reduce permission-boundary spacing fix to a render-layer derivation#197
Draft
gnguralnick wants to merge 10 commits into
Draft
Reduce permission-boundary spacing fix to a render-layer derivation#197gnguralnick 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)
The boundary spacing collapse no longer threads a new SectionView.opened_by_permission_resolution flag through five places in turn-grouping. That boolean is fully derivable at the render layer: a section renders a user bubble iff it has a non-hidden user_event, and the only bubble-less *boundary* section in practice is the one a permission grant/deny opens (user_event: null). conversation-rows now factors that decision into a single shared helper, sectionRendersUserBubble, used BOTH for the existing bubble-rendering decision AND to decide the flush, so the two can never drift. When a section emits a progress block and the NEXT section renders no user bubble, it passes flushBottomMargin to the ProgressBlock, which adds progress-block--flush-bottom (margin-bottom: 0) -- exactly as before at the CSS/DOM layer. turn-grouping is reverted to its base form (no flag). Tests: unit tests for the shared helper and the flush decision (a section followed by a bubble-less section flushes; followed by a normal user-bubble section, or as the last section, does not). The e2e test that reproduces the real shape (request card + separate trailing prose + hidden grant + carryover) is unchanged and still asserts the rendered DOM (flush class present) and the ~18px boundary, failing at the ~46px void before the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
…ion-boundary # Conflicts: # apps/system_interface/imbue/system_interface/test_e2e.py
# Conflicts: # apps/system_interface/imbue/system_interface/test_e2e.py
…ul path Clarify in conversation-rows that the oversized void is specifically a progress-block-to-progress-block gap (two stacked turn-margins), so the flush is only applied on the stepful branch. A no-steps section's tail renders as inline .message-style rows that carry their own smaller spacing, so no oversized void arises there. Comment-only; no behavior change. Addresses a point raised in architecture review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Fixes the empty ~46px vertical gap at a permission grant/deny boundary in the chat progress view (apps/system_interface), in its reduced, render-layer-derived form — no new
SectionViewflag threaded throughturn-grouping.When the user grants/denies a permission, the app injects a hidden notification whose verdict folds onto the permission card and which opens a fresh progress block carrying the open step over (intended turn-boundary carryover). Because that boundary has no user bubble, two progress blocks' turn-margins stacked into an empty void. This collapses it to a single clean ~18px turn break.
Approach
turn-grouping.ts: unchanged from base. Noopened_by_permission_resolutionflag, none of the five threading points the earlier heavy form added.conversation-rows.ts: one shared helper,sectionRendersUserBubble(section), drives both the bubble-rendering decision and the flush decision, so they can't drift. When a section emits a progress block and the next section renders no user bubble, it passesflushBottomMarginto the ProgressBlock.ProgressBlock.ts:flushBottomMargin?: boolean-> addsprogress-block--flush-bottom.style.css:.progress-block--flush-bottom { margin-bottom: 0; }. No:has(...)rule.Verification
test_permission_resolution_boundary_has_no_empty_void) reproduces the real chat shape (request card + separate trailing prose + hidden grant + carryover) and asserts the rendered DOM and the ~18px boundary; confirmed fail-before / pass-after.Tests
cd apps/system_interface && uv run pytest-> 479 passed, 10 skipped, coverage 87.26%. One pre-existing, unrelated failure (agent_manager_test.py::test_start_observe_spawns_long_lived_subprocess, a sandbox tmux resource-guard misfire in a file this branch does not touch; fails identically on the clean base).The branch incorporates origin/main's uvicorn->Flask server migration (merged in); the e2e harness was adapted to
make_threaded_serveraccordingly.Generated with Claude Code