feat(harness): composer wrap — scroll_horizontal = false so long lines wrap instead of h-scrolling - #709
Conversation
…ak_lines fires (plan #706)
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #709
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/composer-wrap @ 39073b7 · 2 files · max_size_content.w max_float_safe → @max(120, avail.w)
Lenses run: L1, L3, L5, L8, L9 (skip: L2 no secrets; L4 no workflow change, zig → harness.wasm green; L6 no new tests — plan-locked frame(), noted below; L7 no BYO)
AGENTS.md read: yes · docs/feature-divide.md yes (composer stays Wasm)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L9 | ui.zig ~681 changes the outer textEntry max_size_content.w. Pinned dvui TextEntryWidget.init strip()s that cap (Options.strip sets max_size_content = null) before creating the inner ScrollArea / TextLayout. Default scroll_horizontal = true → ScrollContainerWidget.rectFor horizontal=.auto gives the child maxw = @max(viewport.w, min_size.w). TextLayoutWidget min width is the unwrapped line (current_line_width — “if break_lines was false”). Wrap then uses contentRect().w of that huge child rect, so break_lines still never fires. Composer chrome is already Options.rect + expand.horizontal locked to avail.w (ui.zig:595–597); the outer cap cannot shrink a parent that is not sized from children. Wrong knob. The public API that bounds wrap to the field (and kills the h-gutter) is scroll_horizontal = false on the textEntry init opts. |
1. /harness, type/paste a single line longer than the composer field (no newline). 2. Expected (#702): wrap at the field, chrome grows, no h-scroll. 3. After this PR: caret still pans sideways; hug stays ~44 px. Same as main. |
Defender: plan-review said “wrap is at allocated width, not the cap.” That would mean today’s max_float_safe already wraps — it doesn’t; that’s why #702 exists. Defender: “finite cap stops min_size inflating the parent.” Parent width is parent.contentRect().w via Options.rect+expand, not child min_size. Defender: “first frame contentRect=0 falls back to min 120, wrap happens.” Next frame the .auto scroll child is unwrapped-wide again; wrap undoes. Issue #702 already named the right width (“allocated row minus the icon column”) — that is the viewport the inner layout never sees. |
high |
| Minor | L8 | docs/harness-limits.md Wrap/grow row now says the viewport-width cap is “a finite cap so break_lines fires” and “fullscreen (~1920 px) is effectively unwrapped.” The cap is stripped before wrap; fullscreen wrap-vs-not is not what this line does. Living docs describe a mechanism that is not on the path. |
Operator reads the table, types a 400 px URL on a 390 px phone, expects wrap. Still h-scrolls. Docs blame “fullscreen unwrapped” which is the wrong story. | Defender: “docs match the plan.” The plan’s mechanism is what this review is attacking. Do not ship a keyboard/layout contract that the binary does not implement. | high |
Residual risk
The real one-line fix is .scroll_horizontal = false (keep break_lines / multiline). That makes ScrollContainer horizontal=.none, child width = field viewport, wrap at the glyphs, no h-bar — which is the no-h-scroll policy (#344/#457/#579) and does not need avail.w. After that, leftover-icon-column math is irrelevant: the viewport is already field-minus-icons. Queue-row editor stays single-line / max_float_safe (out of scope). First-frame contentRect.w==0 still uses min-width fallback (pre-existing hug clamp).
Merge guidance
CONCERNS — do not merge. Rebind wrap to scroll_horizontal = false (or otherwise get a finite width onto the inner textLayout, which strip() currently prevents). Revert the Wrap/grow docs sentence until the binary actually wraps. Do not treat zig build green as evidence of wrap — it does not paint a textEntry.
What was not attacked
Live /harness paste of a long URL (no wasm runtime in this sandbox; zig not on PATH). dvui web-backend vs testing-backend scroll init differences beyond the pinned 4f810ef sources. Queue-band single-line ellipsis. COMPOSER_* height caps (unchanged).
…size_content cap The max_size_content cap at ui.zig ~681 was the wrong knob — dvui's TextEntryWidget.init calls Options.strip() which nullifies max_size_content before passing it to the inner ScrollArea/TextLayout (line 199). With the default scroll_horizontal=true (.auto), ScrollContainerWidget.rectFor gives the child @max(viewport.w, min_size.w) where min_size.w is the unwrapped line width — so break_lines still never fires. Fix: .scroll_horizontal = false (InitOptions line 671) makes ScrollContainerWidget horizontal=.none, child gets viewport.w at line 291, break_lines wraps at the field width. Revert max_size_content.w from @max(120, avail.w) back to dvui.max_float_safe (redundant — stripped anyway). Adversarial review #709 CONCERNS Major L1+L9 + Minor L8.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #709 (R2)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/composer-wrap @ 62c4cb4 · 2 files · scroll_horizontal = false on composer textEntry
Lenses run: L1, L3, L5, L8, L9 (skip: L2 no secrets; L4 no workflow change, zig → harness.wasm green; L6 no new tests — plan-locked frame(); L7 no BYO/config)
AGENTS.md read: yes · docs/feature-divide.md yes (composer stays Wasm)
R1 @ 39073b7 was CONCERNS: max_size_content.w = @max(120, avail.w) is stripped by TextEntryWidget.init (Options.strip() at pinned dvui L199) before the inner ScrollArea/TextLayout. Default scroll_horizontal=true → ScrollContainerWidget.rectFor .auto → maxw = @max(viewport.w, min_size.w) with min_size.w = unwrapped line → break_lines never fires.
62c4cb4 uses the actual knob. Pinned dvui 4f810ef:
TextEntryWidget.initL114:scroll_horizontal = false→horizontal = .noneScrollContainerWidget.rectForL291:.none→maxw = self.si.viewport.w(not unwrappedmin_size.w)TextLayoutWidgetL1278–1342:break_lineswraps atcontentRect().wScrollAreaWidgetL120: h-bar block is skipped whenhorizontal == .none— no gutter even thoughhorizontal_barstill defaults.autofor multiline
Outer max_size_content.w stays max_float_safe (still stripped; comment now says so). Height hug (composer_last_h via minSizeGet after te.deinit()) is unchanged.
Trial-merge into origin/main: clean (ui.zig + docs/harness-limits.md).
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L8 | PR title and body still describe the discarded R1 cap (max_size_content.w = @max(120, avail.w) at ui.zig:681). Head code + docs/harness-limits.md Wrap/grow row document scroll_horizontal = false. |
Next agent or human implements from the PR description, re-introduces the no-op cap, thinks #702 is fixed. | Defender: “git history has the fix commit; living docs on disk are right.” True for main after merge. The merge card and #706 plan body still teach the wrong knob. Update the PR title/body (and plan #706 when convenient) so they match 62c4cb4. Not a product bug. |
high |
Residual risk
First frame contentRect().w == 0 → TextLayout falls through stripped min-size to a 500 px wrap guess (TextLayoutWidget L1279–1288), then settles — same one-frame hug lag already documented. Unbreakable tokens wrap mid-glyph (dvui always consumes ≥1 codepoint). Queue-row editor stays single-line (multiline = false) — out of plan scope. No live /harness paste-a-URL smoke in this attack. horizontal_bar left at multiline .auto is dead because .none skips the bar; do not “fix” it unless a later dvui pin changes that gate.
Merge guidance
PASS WITH NOTES — R1 Major is fixed; safe to merge from this attack. Optional: retitle/rewrite the PR body so it does not re-teach max_size_content. Do not --approve from this review.
What was not attacked
Live /harness long-URL / 10-line paste / 390 px smoke. Caret/selection across wrapped lines (no viewport.x path in TextEntryWidget). Parallel #708 (Escape scan; different ui.zig hunk; trial-merge of this PR vs current main was clean).
Summary
Source issue: #702
Plan: #706
Problem
The composer textEntry defaulted
scroll_horizontal = trueon the innerScrollAreaWidget. With horizontal scroll.auto,ScrollContainerWidget.rectForgives the child@max(viewport.w, min_size.w)wheremin_size.wis the unwrapped line width —break_lines = truenever fires. Long single lines h-scrolled instead of wrapping.max_size_content.wcannot fix this —TextEntryWidget.initstrips it (Options.strip()→max_size_content = null) before creating the innerScrollArea/TextLayout.Fix
One line in
ui.zig—scroll_horizontal = falseon the composer textEntryInitOptions:This sets
ScrollContainerWidgethorizontal = .none→ child getsviewport.w(field width) →break_lineswraps at the glyphs.max_size_content.wstaysdvui.max_float_safe(stripped, irrelevant to wrap). Height hug (composer_last_h) is unchanged —max_size_content.hstill caps vertical grow.Queue band is unaffected (single-line,
multiline = false).Changes (2 files, +9/−5)
native/harness/src/ui.zigscroll_horizontal = falseon composer textEntry; revertedmax_size_content.w; updated commentdocs/harness-limits.mdscroll_horizontal = falsemechanismGates
zig fmt --checkzig build harness -Doptimize=Debugzig build test-rich-invariantsbuild-harnessCINo TS changes — no vitest/typecheck/di-gate/build. No protocol bump, no bridge changes, no caps changed.
Status: Stop — do not merge. Ready for adversarial review.