feat(harness): wide modal help-overlay table, wheel stays in panel, backdrop-close (plan #781) - #783
Conversation
…ackdrop-close (plan #781) help_overlay: convert the absolute post-paint box into a modal dvui.floatingWindow subwindow so pointer+wheel stay in the panel (never scroll the transcript), close on a backdrop click-outside (NEW), and fill most of the band (retire the fixed 460x320 HELP_OVERLAY_W/H cap). Two-column table (fixed chord column + remaining-width help column) so every chord/help string starts on a stable x; loop-unique id_extra per widget kind removes the duplicate-id red outlines; row de-dupe + KEY_TABLE order + WARM-muted context-off rows unchanged. metrics: HELP_OVERLAY_W/H retired for HELP_OVERLAY_W_FRACTION/H_FRACTION + MIN/FLOOR + HELP_OVERLAY_CHORD_COL_W (human-approved cap change 2026-08-22). tests: help_overlay.test.zig extended past string pins to paint locks (wide panel, two-column x, ~390px on-canvas + internal scroll, wheel-isolation, backdrop-click close). docs: harness-limits.md (Keyboard row + Help overlay section), AGENTS.md ownership row. Gates: zig fmt --check; zig build test-rich (help_overlay 65/65); test-rich-invariants; zig build harness -Doptimize=Debug.
|
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 #783
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/help-overlay · 6 files · in-canvas help overlay (modal floatingWindow, band-fraction size, two-column table, backdrop-close)
Lenses run: L1, L3, L6, L8, L9 (skip: L2 no trust-boundary/API/secret/workflow files; L4 no GHA/wasm-artifact paths; L5 no poll/history/body-size change; L7 no host/deploy pin; L6 DI/cost N/A — zig dvui_testing, not PGlite/createDbConnection)
AGENTS.md read: yes (docs/feature-divide.md also — overlay stays Wasm; no dual-chat)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L6 | Goal 2 is not locked. help_overlay.test.zig · "wheel over the panel scrolls the overlay list, not the transcript" mounts only help.paint — no transcript scrollArea, no state.transcript_scroll. The test comment admits the transcript is not in the frame. The assertion is ctx_scroll.offset increased. |
Revert floatingWindow to the old absolute dvui.box (the #780 leak). Overlay list can still scroll; production state.transcript_scroll moves with the wheel. CI stays green because nothing in the test frame is the transcript. |
Defender: “offset moving proves the overlay captured the wheel; a modal windowFor cannot also deliver to the transcript.” That is only true if the production widget tree and paint order stay this shape. The test does not construct that tree, so it cannot fail the bug this PR exists to fix. |
high |
| Minor | L6 | Plan test 2c (Esc / Ctrl+/ / leader ? still reach keymap_dispatch while the modal is open) is not implemented. #781 named “modal wall swallows Esc” as a risk; the PR text treats “documented in the plan” as a substitute for a lock. |
Open overlay, press Esc. If a dvui bump (or a child) starts marking Esc handled, help_close never runs and the overlay sticks. No test in help_overlay.test.zig injects a key while paint is up. Existing keymap.zig cases never build the modal. |
Defender: traced vendored FloatingWindowWidget.processEventsAfter (pin 4f810ef) — it only handles tab next/prev; keymap_dispatch walks unhandled keys and ignores eventMatch / target_windowId. Current pin is probably fine. That is an untested invariant on the exact surface the plan required locking. |
high |
| Minor | L1 | help_overlay.paint · dvui.floatingWindow sets .resize = .none but never dragAreaSets an empty/header rect. Default drag_area is the full panel; processEventsAfter starts a middle-drag on an unhandled press inside. Next frame rect_store is recomputed from the band, so the panel snaps back. |
Open help. Click-drag on the title / padding (not a textLayout). Panel translates with the cursor for the rest of the frame, then jumps back to the centered band rect. | Defender: children handle presses on rows, so drag only starts on chrome; snap-back bounds the displace. Still a visible grab/glitch on the new modal, and it fights any press-drag on unhandled interior pixels. | high |
| Minor | L6 | Two-column x-lock only compares overlay-chord-0/1 and overlay-help-0/1. After prev_action de-dupe those are submit and queue_save — both rowChord = "Ctrl/Cmd+Enter". Goal 4’s hard case (↑/Esc vs the long chords) is not asserted. |
Shrink HELP_OVERLAY_CHORD_COL_W so "Ctrl/Cmd+Enter" grows past the min and "↑" does not. Help x diverges on later rows. Test still passes. |
Defender: min_size_content.w = HELP_OVERLAY_CHORD_COL_W (140) equalizes short chords. Likely true today; the test still does not lock the claim it titles. |
high |
| Minor | L1 | Help textLayout has no .expand = .horizontal / max width. Chord column reserves 140px. On a ~390 band: panel_w = min(max(0.92·390, 360), 390−32) = 358, minus padding/border/row pad → ~180px for help. horizontal = .none on ctx_scroll. Long copy ("Thinking default collapsed", "Send (enqueue when busy)") can clip instead of wrap. The ~390 test only checks the scroll-area widget rect stays on-canvas. |
Open help on a ~390-wide canvas. Long help strings clip at the panel edge; no h-scrollbar (repo no-h-scroll) and no wrap. | Defender: textLayout may still wrap to the box-allocated width. Without expand, rectFor sizes to unwrapped min_size, so wrap is not guaranteed. Did not rasterize glyphs here — medium. |
medium |
| Nit | L8 | native/harness/build.zig help_overlay test block still says “no frame” / “paint is not invoked”. Tests now testing.init + settle + paint. |
Next agent skips adding a dvui_testing dep or misreads the target as pure. | Comment-only; the import is already dvui_testing. |
high |
| Nit | L1 | pub var ctx_scroll is never reset when the overlay closes (ui.zig flips help_overlay_open; New/Clear does not zero the ScrollInfo). |
Scroll the list, Esc, reopen — list resumes mid-table, first rows off-viewport. | Might be desirable memory; still surprising for a help cheatsheet. | high |
Residual risk
Did not run zig build test-rich in this workspace (no Zig 0.16 on PATH). Keyboard-through-modal is inferred from the vendored FloatingWindowWidget pin, not executed. First-frame floatingWindow hide (w=h=0) plus focusSubwindow is stock dvui; composer focused_widget_id on the base window should survive close via Window.end unused-subwindow fallback — not operator-checked. Backdrop close on press-or-release is unhandled-event scanning after fw.deinit; same-frame click-through to composer is probably prevented by mouse.floating_win tagging, but test 2b never mounts composer/queue-band to prove “underlying not steered.”
Merge guidance
- CONCERNS: do not merge on the Goal 2 test as written. Mount a transcript (or a stand-in
ScrollInfopainted before the overlay, same order asui.zig) and assert its offset is unchanged when the wheel hitsoverlay-scroll-area. While there: add 2c (Esc / Ctrl+/ with the modal up), compare a short-chord row against a long-chord row,dragAreaSet(.{})(or header-only), and give the help column a real remaining-width wrap. Minors/nits can ride with an explicit accept; the Goal 2 lock cannot.
What was not attacked
Live DO runner, prod Gateway, Wasm in-browser operator pass (duplicate-id console, real trackpad, ~390 device), zig build test-rich / test-rich-invariants / harness -Doptimize=Debug execution in this session, dvui versions other than pin 4f810ef1d695b55fb714a13536a930b9304d619e.
address_pr_feedback #783 — living planning comment (round 3)Review: adversarial round 3 → CONCERNS (head Items → status
Key decisions / deviations
Gates (green locally, harness-only — no TS gates apply)
StateBranch Next: re-run |
…#783 Major L6) The Goal-2 wheel-isolation lock was vacuous: it mounted only help.paint, so a wheel that leaked to the transcript (the #780 bug) could not fail it. Frame.paint now builds a transcript scroll container with overflow rows BEFORE the overlay modal (same order ui.zig paints after the transcript laid out), and the test asserts BOTH infos: overlay ctx_scroll offset moves while the transcript stand-in offset stays put — a regression to the absolute dvui.box now moves the transcript offset and fails the test.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #783 (round 2)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/help-overlay · 6 files · 2 commits (a002060 + a456b44 transcript stand-in for Goal 2)
Lenses run: L1, L3, L6, L8, L9 (skip: L2 no trust-boundary/API/secret files; L4 no GHA/artifact path change — zig → harness.wasm green on HEAD; L5 no poll/history/body-size; L7 no host pin; L6 DI/cost N/A — existing dvui_testing backend, not PGlite)
AGENTS.md read: yes (docs/feature-divide.md — keymap/help stays in-canvas; no DOM cheatsheet)
Round 1 was CONCERNS (Major L6: wheel test mounted only help.paint, so a transcript leak could not fail). a456b44 addresses that. This pass re-attacks HEAD, including the follow-up.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L6 | Plan #781 test 2c is still absent: no paint+dispatch lock that Esc / Ctrl+/ / leader ? still reach keymap_dispatch while the modal is open. DoD and the plan risk table both named “modal wall swallows Esc.” |
A later dvui pin where FloatingWindowWidget.processEventsAfter handles more than Tab (or a first-frame focus sink marks Esc handled) ships a help overlay that cannot be dismissed from the keyboard. Click-outside still works; Busy Esc-wins is unproven in this tree. |
Defender: keymap_dispatch.dispatch still walks raw dvui.events() after overlay paint (ui.zig ~714). Vendored FloatingWindowWidget only consumes Tab next/prev, not Escape. keymap.zig already pins help_open → help_close. Residual is the combined modal+dispatch path, not the table. |
high |
| Nit | L8 | docs/harness-limits.md Help overlay still says backdrop-close is “NEW”; metrics comments cite “plan #781”. Living-docs rule was timeless / no process artifacts. |
Harmless for operators; next doc pass treats “NEW” as current. | Caps/names themselves are correct. | high |
| Nit | L6 | Wheel test never resets help.ctx_scroll (only Frame.transcript_scroll). File order currently paints-then-wheels once, so after > before holds. |
A later test above it that wheels, or shuffled Zig order, leaves the overlay at scrollMax; after > before fails green-to-flake, or a no-op wheel looks like a product regression. |
Sequential source order today. Cheap ctx_scroll = .{} in the test setup. |
medium |
What the follow-up actually locked (L6)
Frame.paint now builds a transcript scrollArea with overflow rows before help.paint, matching ui.zig (transcript deinit/processEvents runs first). The Goal 2 test asserts both infos: overlay ctx_scroll moves and the stand-in offset stays put.
That is the real #780 leak: without a subwindow, windowFor is the base window, the earlier transcript consumes the wheel, overlay offset would not move / transcript offset would. A revert to the pre-#781 absolute dvui.box now fails CI. Round-1 Major L6 is closed.
(Not required for Goal 2: dropping .modal = true but keeping a floatingWindow would still pass the wheel test, because windowFor hits the overlay rect. Modal remains locked by the backdrop-click test, not by the wheel test.)
Residual risk
Did not run zig build test-rich here. GHA zig → harness.wasm is green on a456b44. Keyboard dismiss through the modal is source-plausible (see 2c refutation) but untested as a pair. Operator: no live /harness wheel/Esc. pub var ctx_scroll is not zeroed on close (reopen keeps last offset) — pre-existing shape, not a leak.
Merge guidance
- PASS WITH NOTES: safe to merge from this attack. The Goal 2 lock is no longer vacuous.
- Optional same-tree follow-up: a 2c paint+dispatch Esc/Ctrl+/ case, reset
ctx_scrollin the wheel test, drop “NEW” from living docs. None of those should gate the merge.
What was not attacked
Live /harness wheel + Esc, test-rich / test-rich-invariants in this session, dvui versions other than pin 4f810ef, composer click-through on the same frame as a backdrop close (modal windowFor should deny it; not driven).
…agAreaSet, wrap, scroll reset (increments 2-7)
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #783 (round 3)
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/help-overlay · 8 files · 3 commits (HEAD e600706 — keys-through-modal, dragAreaSet, wrap, scroll reset)
Lenses run: L1, L3, L6, L8, L9 (skip: L2 no trust-boundary/API/secret files; L4 no GHA/artifact path change — zig → harness.wasm green on HEAD; L5 no poll/history/body-size; L7 no host pin; L6 DI/cost N/A — existing dvui_testing backend, not PGlite)
AGENTS.md read: yes (docs/feature-divide.md — keymap/help stays in-canvas; no DOM cheatsheet)
Round 1 CONCERNS (vacuous Goal 2). Round 2 PASS WITH NOTES after the transcript stand-in. This pass re-attacks e600706.
Round-2 items
| Round-2 | Status on e600706 |
|---|---|
| Minor L6 plan test 2c (Esc / Ctrl+/ through modal) | Partially closed. New test asserts those key events stay unhandled after overlay processEventsAfter. Does not run keymap_dispatch or flip help_overlay_open. Necessary, not sufficient. |
| Nit L8 living-docs “NEW” / plan #781 | Unchanged (docs/harness-limits.md still says “NEW — it is a modal”). |
Nit L6 wheel test never resets ctx_scroll |
Closed for close-via-Esc / backdrop (resetScroll). Not closed for Ctrl+/ toggle, leader ? toggle, or New/Clear. |
Goal 2 dual-offset wheel lock from a456b44 is intact.
Findings (HEAD)
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L6 | “middle-drag” test cannot fail the dragAreaSet(.{}) lock. Vendored enums.Button.pointer() is left or touch only (enums.zig:136-138). FloatingWindowWidget.processEventsAfter starts a drag only inside if (me.button.pointer()). The test injects .middle press/release, which the handler never sees. before/after rects match with or without the empty drag area. |
A revert of fw.dragAreaSet(.{}) leaves CI green. An unhandled left press on the padding ring still one-frame-drags the panel (the snap-back the comment describes). |
Defender: production empty drag_area is the right fix; .contains on {} is false. The lock is just pointed at the wrong button (dragPart .middle ≠ mouse .middle). |
high |
| Minor | L1 | resetScroll() is only called from help_close and backdrop-close. help_toggle / help_toggle_leader flip help_overlay_open without it; New/Clear (ui.zig:520, state.zig:182) also skip it. |
Operator scrolls the table, hits Ctrl+/ (documented close), opens again → list is mid-table. Same after New/Clear in a live Wasm instance. The new invariant (“reopen resumes at the top”) does not hold on the primary toggle. | Defender: Esc and click-outside are covered. Ctrl+/ is still the Keyboard-row toggle. Sticky offset is not data loss, but it is the bug they just named. | high |
| Minor | L1+L6 | Help-column wrap sets .max_size_content.h = TOUCH_H - 6 and .min_size_content.h = TOUCH_H - 6. WidgetData.init / minSizeSetAndRefresh clamp min_size to max_sizeGet(), so a wrapped string cannot report a taller min — extra lines clip inside a one-line well. The new wrap test runs at 800×600, where help_max_w ≈ 564 and “Send (enqueue when busy)” fits one line; equal widths only prove .expand = .horizontal, not wrap-vs-clip. |
At ~390 (help_max_w ≈ 186) the long help strings wrap inside a 34 px cap and clip. The ~390 overflow test only checks the scroll-area vs win.w, not row height or visible wrap. |
Defender: break_lines default is true; .expand = .horizontal does force leftover width (the 800 px test). Ceiling help_max_w is fine. The height cap is what undoes wrap at the phone width the plan cares about. |
high |
| Nit | L6 | Test 2c never asserts a key event was present in dvui.events(), and never calls keymap_dispatch. An empty key list still yields any_key_handled == false. |
A pressKey / step mismatch would keep 2c green while Esc is swallowed elsewhere. |
pressKey does addEventKey down+up before step, so this frame’s events() should contain them. Overlay source still only handles Tab next/prev. Residual is the missing dispatch/close assertion. |
medium |
| Nit | L8 | help_max_w hardcodes panel pad 12 and row pad 4 instead of the same literals the Options.padding uses. Docs still say backdrop-close is “NEW”. |
Pad drift desyncs the wrap ceiling. “NEW” ages into the timeless row. | Values currently match. | high |
Residual risk
Did not run zig build test-rich here. GHA zig → harness.wasm is green on e600706. keymap_dispatch → help_overlay import is acyclic. Operator: no live /harness wrap at 390, left-drag on padding, or Ctrl+/ reopen. pub var ctx_scroll still lives across a toggle-close.
Merge guidance
- CONCERNS: do not merge on the wrap-height clip or the vacuous drag lock. Cheap fixes: drop
max_size_content.h(keep width ceiling /expand); drive the drag test with.left(orpress+motion thatpointer()accepts); callresetScroll()whenever the overlay goes false (help_toggle, leader?, New/Clear). - Do not re-open Goal 2. The transcript stand-in dual-offset lock still fails a box regression.
- 2c as “keys unhandled” is acceptable if the three production misses above are fixed; adding a dispatch/close assertion is optional.
What was not attacked
Live /harness at 390, test-rich / test-rich-invariants in this session, dvui pins other than 4f810ef, first-frame floatingWindow hide vs composer focus.
…croll on toggle/New/Clear, wrap-height clip, shared pad constants (plan #781)
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #783 (round 4)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/help-overlay · 8 files · 4 commits (HEAD 0d71b84 — left-drag lock, resetScroll on toggle/New/Clear, wrap-height, shared pad constants)
Lenses run: L1, L3, L6, L8, L9 (skip: L2 no trust-boundary/API/secret files; L4 no GHA/artifact path change — zig → harness.wasm green on HEAD; L5 no poll/history/body-size; L7 no host pin; L6 DI/cost N/A — existing dvui_testing backend, not PGlite)
AGENTS.md read: yes (docs/feature-divide.md — keymap/help stays in-canvas; no DOM cheatsheet)
Round 1 CONCERNS (vacuous Goal 2). Round 2 PASS WITH NOTES (transcript stand-in). Round 3 CONCERNS (wrong mouse button on drag lock, incomplete resetScroll, wrap height cap, 2c empty-list hole). This pass re-attacks 0d71b84.
Round-3 items
| Round-3 | Status on 0d71b84 |
|---|---|
Minor L6 drag test used .middle (Button.pointer() is left/touch only) |
Closed. Test now injects .left press/motion/release on the padding ring. Revert of dragAreaSet(.{}) would translate the panel this frame (12*PX ≫ COL_EPS). |
Minor L1 resetScroll skipped on Ctrl+/, leader ?, New/Clear |
Closed on the named paths: help_toggle / help_toggle_leader always reset; New/Clear n==0 block in ui.zig resets; Esc and backdrop still reset. |
Minor L1+L6 wrap max_size_content.h = TOUCH_H-6 clipped; wrap test only at 800 px |
Closed. .max_size_content = .width(help_max_w) (MaxSize.h = max_float_safe). New test paints the same long string at 800 then at a 390 band and asserts narrow_h > one_line_h + PX. |
| Nit L6 2c never asserted a key event was present | Closed. any_key_present is set from dvui.events() and asserted. |
Nit L8 help_max_w hardcoded 12/4; docs said “NEW” |
Closed. PANEL_PAD_X / ROW_PAD_X shared with Options.padding. Docs dropped “NEW”; reopen-at-top + wrap-not-clip are stated. |
Goal 2 dual-offset wheel lock from a456b44 is intact.
Findings (HEAD)
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L1 | state.resetTranscriptScroll() (onInit) still sets help_overlay_open = false without help_overlay.resetScroll(). |
In-process re-init (comment at ui.zig:137 — wasm reload / host re-mount without module re-zero) leaves ctx_scroll mid-table; next open is not at the top. |
Named close paths (Esc, backdrop, toggle, New/Clear n==0) reset. A full Wasm instantiate would re-zero the pub var. Edge is in-process onInit only. |
high |
| Nit | L8 | help_overlay.zig drag comment still says “unhandled middle-drag press” after the lock was corrected to left/touch pointer(). |
Next reader re-introduces a .middle test. |
Production dragAreaSet(.{}) is button-agnostic (empty rect). Test name/body now say left. |
high |
No surviving Major/Minor on wrap, drag, or the documented close paths.
Residual risk
Did not run zig build test-rich here. GHA zig → harness.wasm is green on 0d71b84. 2c still does not call keymap_dispatch / flip help_overlay_open (keys-unhandled is the actual modal-wall risk; dispatch remains a raw dvui.events() walk after paint). Wrap-height test reuses the 800×600 window and restyles the band to 390 — help_max_w is from paint’s band args, so wrap math matches production; a tiny fallback font that still fits the long string on one line at 186 px would false-fail, not false-pass. Operator: no live /harness wrap at 390 or Ctrl+/ reopen.
Merge guidance
- PASS WITH NOTES: safe to merge from this attack. Round-3 wrap clip, vacuous drag lock, and toggle/New/Clear reset are actually closed.
- Optional:
resetScroll()insideresetTranscriptScroll/onInit; drop “middle-drag” from the comment. Neither should gate the merge. - Do not re-open Goal 2.
What was not attacked
Live /harness at 390, test-rich / test-rich-invariants in this session, dvui pins other than 4f810ef, first-frame floatingWindow hide vs composer focus.
…e-init, drop stale middle-drag comment (PR #783)
address_pr_feedback #783 — round 4 (PASS WITH NOTES, 2 Nits) resolvedAdversarial round 4 on PR #783 returned PASS WITH NOTES (head
Gates (green locally, harness-only)
StateBranch Next (implementer): re-run |
Plan #781 — wide modal help-overlay table · wheel stays in panel · backdrop-close
Implements plan #781 (source issue #780): the in-canvas help overlay was a tiny 460×320 absolute box that leaked scroll to the transcript and painted red duplicate-id outlines.
What changed
floatingWindowsubwindow (was an absolutedvui.boxpainted after the transcript). Being a modal subwindow,windowForroutes pointer + wheel to it before the earlier-built transcriptscrollArea, so scrolling stays in the panel — the transcript'sstate.transcript_scrollnever moves while open (Goal 2).paintscans an unhandled press/release outside the panel and the caller flipshelp_overlay_openoff. Esc / Ctrl+//leader?toggling is unchanged (a floatingWindow only routes mouse events, sokeymap_dispatchatui.zig:707still sees the chords — test 2c documented in the plan).HELP_OVERLAY_W/H(fixed 460×320) is retired forHELP_OVERLAY_W_FRACTION/H_FRACTION+MIN/FLOORfloors (human-approved cap change, 2026-08-22). Width-first; ~390 px stays on-canvas with no horizontal overflow and internal scroll.HELP_OVERLAY_CHORD_COL_W) + remaining-width help column, so every chord and every help string starts on a stable x (Goal 4).id_extraon every looping widget (distinct bases per box/chord/help) — removes the duplicate-id rederrorOutline(Goal 1).prev_actionper distinct action),KEY_TABLErow+help source, and WARM-muted context-off greying are unchanged. TEAL only; EMBER never.Tests
help_overlay.test.zigextended past the #761 string pins to paint locks under the dvui testing backend:Gates
All green locally:
zig fmt --check·zig build test-rich(help_overlay 65/65) ·zig build test-rich-invariants·zig build harness -Doptimize=Debug(full Wasm compile).Ops / docs
docs/harness-limits.md(Keyboard row + new "Help overlay" section),AGENTS.mdownership row (metrics/caps names landed).Refs #781 — implementation (not merged).