feat(#650): reusable WARM text-wave on Waiting for model… (plan #655) - #657
Conversation
|
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 #657
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/text-wave-waiting-copy @ 8c4bdd6 · 5 files · busy-row text wave
Lenses run: L1, L3, L4, L5, L6, L8, L9 (skip: L2 no API/secret/runner; L7 no host/config bind)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md N/A
GitHub: CONFLICTING vs main @ 3c3c89c (#653). build-harness has not run on this SHA (only Vercel). Do not merge until rebase + green wasm.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L9 | text_wave.paint has no phase-0 special case. headPosition(0, N)=0 but cyclicDistance(i, 0, N) is 0 only at i=0. Tail maps to WARM_RAMP[3] = warm_surface #1a120c on teal_bg #050a0c (~1:1). Plan #655 Goal 4 and its own design note claimed “phase 0 → all distance 0 → solid warm_accent” — that math is false. PR docs then redefined reduced motion as “static wave.” |
(1) prefers-reduced-motion: reduce — host skips ticks, busy_tick stays 0. (2) Old host + new Wasm — same. (3) Every animation frame: glyphs at dist ≳ N/2 paint near-black on near-black. Waiting copy reads as a few orange letters and a hole. |
Defender: “plan locked this 4-stop LUT.” LUT is for filled 4 px cells, not body text. Goal 2 is a visible shimmer; Goal 4 is solid copy at rest. Defender: “static wave is the natural formula.” Then the plan’s reduced-motion lock is unmet and old-host degrade is a frozen, unreadable gradient. Fix: phase == 0 (or a solid flag) → one addText at ramp[0]; while ticking, stop the ramp at warm_muted (or interpolate accent→muted only). |
high |
| Major | L6 | text_wave.test.zig "countScalars: ASCII text" expects 20 for "Waiting for model\u{2026}". That string is 18 scalars (Waiting(7)+ (1)+for(3)+ (1)+model(5)+…(1)). First zig build test-rich fails. No test asserts phase 0 → all colorStep == 0 (the plan’s DoD row 8, which would also fail against the real formula). busy_row_layout.test.zig still paints paintBusyRow(0, 0) only — clock sibling untested. |
Open the PR on the runner (or rebase so build-harness actually fires). text_wave test binary exits non-zero. |
Defender: “we counted ~20 in the cycle-time comment.” Comment ≠ assertion. 18×3/10 Hz = 5.4 s, not a reason to expect 20. Defender: “ellipsis is three dots.” Test uses \u{2026}, one scalar. |
high |
| Major | L8 | Docs hunk edits the pre-#653 Busy spinner row. main now documents two grids (transcript WARM + status-bar idle teal_muted). This PR’s replacement paragraph drops the status-bar instance. GitHub already marks the PR CONFLICTING on that row. |
Resolve the conflict by taking the PR side. Living docs lose the #653 idle-grid lock. | Defender: “we’ll rebase carefully.” The posted diff does not; call it out so the resolve cannot be “accept incoming.” | high |
| Minor | L1 | Clock is a second textLayout (busy_row.zig TEXT_ID) always mounted, including turn_elapsed == 0. Default TextLayoutWidget pad is Rect.all(6). Empty clock still occupies ~12 px. When the clock shows, wave-right pad + clock-left pad inserts ~12 px between … and · mm:ss (old path was one layout, one pad). |
Busy for ≥1 s. Middot no longer sits against the ellipsis. At t=0 a ghost padded box sits after the waiting copy. | Defender: “separate tl is the isolation lock.” Isolation does not require an empty padded widget. Skip the tl when turn_elapsed == 0; .padding = .all(0) on both. Or append the clock as addText at ramp[0] inside the wave tl after the scalars — color, not a second box, is enough isolation. |
high |
Residual risk
Per-scalar addText drops kerning on the waiting label (plan-accepted). Utf8Iterator on hostile invalid UTF-8 stops early (this call site is a literal). Wave still expand = .horizontal like the old tl. No build-harness log exists for 8c4bdd6, so compile/layout on the runner is unproven.
Merge guidance
CONCERNS — do not merge. Rebase onto main (#653), keep both Busy-spinner facts in docs/harness-limits.md, fix the 18-vs-20 test, and make phase 0 solid warm_accent (reduced motion + old host). Stop the moving ramp before warm_surface so letters stay readable on teal_bg. Then wait for build-harness green.
What was not attacked
Live /harness Busy at 10 Hz. Reduced-motion OS toggle. Touch vs mouse. dvui two-textLayout min-size on the testing backend (layout test never sets turn_elapsed > 0). TS / host / runner.
… — rebased + review fixes - L1+L9: phase-0 fast-path solid ramp[0] on all glyphs (no near-black tail) - L6: fix countScalars test 20→18; add phase-0 solid-accent domain test - L8: resolve docs conflict — keep both spinner facts from #653 + text_wave - L1: skip clock textLayout at turn_elapsed==0; padding=0 on both textLayouts
8c4bdd6 to
e10501f
Compare
…main + UTF-8 colorStep
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #657
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/text-wave-waiting-copy @ c803dde · 5 files · busy-row WARM text-wave (plan #655)
Lenses run: L1 L3 L4 L5 L6 L8 L9 (skip: L2 — no API/secrets/runner; L7 — no host/config seams)
AGENTS.md read: yes · docs/feature-divide.md read: yes
Re-review after 8c4bdd6 CONCERNS. Fixed: 18-scalar count, docs keep both spinner instances, clock tl skipped at turn_elapsed==0, both layouts padding=0, build-harness green. Not fixed / newly introduced below.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | text_wave.paint treats phase == 0 as “wave off / solid ramp[0]”. The existing 10 Hz feed does not reserve 0. HarnessHost increments tick unbounded; inv_set_busy_tick @truncates to u8. |
Busy turn ≥ 25.6 s (tick = 256, 512, …). One 100 ms frame paints the whole waiting string solid accent, then the wave resumes at head ≈ 0.33. Long agent turns flash. Reduced-motion / old-host still need the solid path — they are indistinguishable from wrap. |
Defender: “phase 0 is the RM contract.” RM is “host never pushes ticks, value stays 0.” Animation wrap does push 256 → 0. Spinner survives because phase % 8 is a real step; this module overloads 0 as a mode bit. Fix: do not special-case 0, or keep 0 = solid and have the host send (tick % 255) + 1 while Busy (never 0 after start). |
high |
| Major | L1 | Clock left the waiting textLayout. On main, · mm:ss is addText on the same tl as the copy. This PR mounts a second textLayout (busy_row.zig) with .expand = .horizontal beside text_wave’s own expanding tl. |
After 1 s the clock appears. Extra width is shared/absorbed by the expanding wave box; the middot no longer sits on the ellipsis (Waiting for model… · 0:01 → copy then a gap then · 0:01, or clock pinned to the far trail). busy_row_layout.test.zig still only calls paintBusyRow(0, 0) — clock sibling never laid out. |
Defender: “separate tl is the isolation lock; padding is 0.” Isolation is color, not a second expanding widget. Append the clock at ramp[0] / warm_accent on the wave tl after the scalars (or let the caller own the tl and have paint only addText). |
high |
| Major | L9 | Moving ramp still ends at WARM_RAMP[3] = warm_surface #1a120c on teal_bg #050a0c (~1:1). Prior review required stopping at warm_muted while ticking. Docs now advertise the near-black tail. |
Any phase != 0: for N=18, cyclic dist ≥ 7 → step 3. ~5 glyphs per frame are unreadable. Shimmer becomes a traveling hole, not a brightness wave. Same contrast failure as the old phase-0 tail, just moving. |
Defender: “plan locked the 4-stop cell LUT.” That LUT is for 4 px filled cells. Goal 2 is a visible WARM wave on body text. warm_accent → warm_muted (optionally warm_border) is enough; warm_surface is a spinner-off color. |
high |
| Nit | L6 | Test name "phase 0: all scalars map to colorStep 0" asserts the opposite (i=5 → step 2, i=9 → step 3). Useful as a “formula is not solid” lock; the name will mislead the next editor. |
Someone “fixes” assertions to match the name and deletes the only documentation of why the fast-path exists. | Rename; do not change the numbers. | high |
Residual risk
Per-scalar addText still drops kerning (plan-accepted). Utf8Iterator on invalid UTF-8 stops early (this call site is a UTF-8 literal). Cycle time is 18×3/10 Hz = 5.4 s, not the “~6 s” comment. No operator 10 Hz / reduced-motion / 30 s-turn smoke of the PR artifact.
Merge guidance
CONCERNS — do not merge until the Majors are addressed.
- Stop using
phase == 0as a mode bit or keep 0 reserved and stop the host/bridge from delivering wrap-0 while Busy. - Put the clock back in the same text run as the waiting copy.
- Cap the moving ramp at a readable WARM stop (
warm_muted/warm_border), notwarm_surface. - Extend
busy_row_layoutwithpaintBusyRow(phase, elapsed>0)so the clock sibling cannot regress silently.
What was not attacked
Live /harness Busy at 10 Hz, OS reduced-motion toggle, 25.6 s wrap on a real turn, dvui two-textLayout min-size with freetype, prod Gateway, DO runner beyond the green build-harness check.
…me textLayout (plan #655 review round 2) Three Majors + one Nit from adversarial review of PR #657 (@ c803dde): L1 (phase-0 wrap flash): bridge inv_set_busy_tick maps busy ticks to 1..255 wrapping 255→1, never 0. Phase 0 is reserved for idle/stop/error and the text_wave phase-0 fast-path (solid accent). No flash at 25.6 s. L1 (clock separate tl): text_wave gains suffix_text option; busy_row formats the clock suffix and passes it into the same textLayout. Removed separate clock textLayout + TEXT_ID constant. L9 (ramp ends at warm_surface): colorStep capped at 2 (warm_border, #3a2818). Multiplier changed 4.0→3.0. warm_surface #1a120c is ~1:1 on teal_bg and reserved for rect_spinner off-cells. L6 (misleading test name): renamed to document WHY the fast-path exists; updated all assertions for 3.0/2 cap. Added busy_row_layout test for clock suffix width regression guard. Docs: updated ramp description + reduced motion contract.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #657 (round 3)
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/text-wave-waiting-copy @ c0c378c · 7 files · text-wave + bridge phase remap
Lenses run: L1 L3 L4 L5 L6 L8 L9 (skip: L2 — no API/secrets/runner; L7 — no host/config bind)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md N/A
Round-2 Majors in product code look fixed: clock is suffix_text on the same textLayout; inv_set_busy_tick maps busy ticks to 1..255 (never 0); colorStep caps at 2 (warm_border). This SHA is still unmergeable.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L4+L6 | zig build test-rich is red on this head (build-harness run 32099137085). Two new failures, both introduced here. (1) text_wave.test.zig:145 "UTF-8 safety…" expects colorStep == 1, got 0. After the 4.0→3.0 multiplier, cyclicDistance(1, 1.666, 4) is 0.666; 0.666*3/2 is 0.999 → floor 0. The comment claims = 1.0. They hardcoded 1.666 instead of headPosition(5, 4) (5/3). (2) busy_row_layout.test.zig:281 does not compile: phase / elapsed are not accessible from the inner fn paint() (Zig 0.16). That is the clock-width test added to lock the L1 fix. Summary: 785/786 pass, 1 fail, busy_row_layout compile 1 error. |
Open the PR. Required zig → harness.wasm is red. merge-pr must stop. No new harness artifact. |
Defender: “layout/font warnings elsewhere.” Those are not the 2 failed steps. Defender: “will fix in a follow-up.” This SHA cannot ship. Run zig build test-rich before push. |
high |
| Minor | L1 | Bridge wrap is 255 → 1, not cyclic-smooth. Wave headPosition(255, 18) = 13.0, next tick headPosition(1, 18) = 0.33 — a ~13-glyph jump every 25.5 s. Spinner busy_tick % 8 goes 7 → 1 and skips cell 0 (the documented bottom-left head). |
Leave a turn running ≥ 25.5 s. One frame the wave teleports; the 2×4 grid skips a cell. No more solid-accent flash (that Major is gone). | Defender: “never 0 is the contract.” True for text_wave’s fast-path. The hitch is the 255→1 discontinuity, not phase 0. Host-side (tick % 255) + 1 is the same math. Residual unless someone wants a continuous u8 that just never lands on 0. |
high |
| Nit | L8 | bridge.zig:109 still says “Scalar u8 wraps naturally.” It does not. busy_row.zig:55 still cites the retired TEXT_ID 0xffff_ffff. |
Next editor assumes truncate-to-u8 and reintroduces the flash, or hunts a deleted id. | Update the two comments to match the remap + suffix_text. |
high |
Prior Majors (round 2 @ c803dde) — disposition
| Prior | Status |
|---|---|
| L1 phase-0 wrap → solid flash | Fixed in inv_set_busy_tick (0 reserved). Residual hitch above. |
L1 clock in a second expanding textLayout |
Fixed — suffix_text on the same tl. The new layout test that should lock this does not compile. |
L9 ramp ends at warm_surface |
Fixed — cap 2 / warm_border. Tail is still dim (#3a2818 on #050a0c ≈ 1.2:1); not re-raised. |
| L6 test name lie | Fixed (renamed). |
Residual risk
warm_border tail contrast is still poor; warm_muted would actually read. No host-side test of the 1..255 remap (only a comment). formatElapsedClock hours form + 64-byte suffix buf is fine. Per-scalar kerning drop is plan-accepted. I did not paint /harness at 10 Hz.
Merge guidance
CONCERNS — do not merge this SHA. Fix the two test-rich failures (UTF-8 assertion to match the 3.0/floor math; capture phase/elapsed via a struct { var … } or hardcoded helpers, same pattern as paintAndGetRects). Wait for zig → harness.wasm green. Then this is PASS WITH NOTES material.
What was not attacked
Live /harness 10 Hz / reduced-motion OS toggle / 25.5 s wrap on a real turn. Prod Gateway. DO runner beyond the red log.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #657 (round 4)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← feat/text-wave-waiting-copy @ c0ac08d · 7 files · text-wave + bridge phase remap
Lenses run: L1 L3 L4 L5 L6 L8 L9 (skip: L2 — no API/secrets/runner; L7 — no host/config bind)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md N/A
Supersedes CONCERNS on c0c378c. Round-3 Majors in this diff are fixed: UTF-8 colorStep assertion now uses head=2.0 (1.0*3/2=1.5 → step 1); clock-width test hardcodes (0,0) / (7,60) so Zig 0.16 accepts the inner fn; stale u8 wraps naturally / 0xffff_ffff comments are gone. test-rich ran text_wave + busy_row_layout without those two failures.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L1 | Bridge wrap is still 255 → 1. Wave headPosition(255, 18) = 13.0, next tick headPosition(1, 18) = 0.33 — ~13-glyph jump every 25.5 s. Spinner busy_tick % 8 goes 7 → 1 and skips cell 0. |
Leave a turn running ≥ 25.5 s. One frame the wave teleports; the 2×4 grid skips a cell. No solid-accent flash (that Major is gone). | Defender: “never 0 is the contract.” True for the fast-path. The hitch is the discontinuity, not phase 0. Residual. | high |
Prior findings — disposition
| Prior | Status |
|---|---|
L1 phase-0 wrap → solid flash (c803dde) |
Fixed — inv_set_busy_tick reserves 0. Residual hitch above. |
L1 clock in a second expanding textLayout |
Fixed — suffix_text on the same tl. New width test compiles. |
L9 ramp ends at warm_surface |
Fixed — cap 2 / warm_border. Tail still dim; not re-raised. |
| L6 test name lie | Fixed (renamed earlier). |
L4+L6 test-rich red: UTF-8 expect + layout compile (c0c378c) |
Fixed on this SHA. Those two steps are not in the fail list. |
| L8 stale comments | Fixed. |
Residual risk
Required check zig → harness.wasm is red on this SHA (32100120871, 811/812). The only fail is untouched composer_text.test.zig "mixed CRLF and lone CR…". Helper:
const BUF: [64]u8 = undefined;
fn normalize(...) {
var dest: [BUF.len]u8 = BUF;
return composer_text.normalizeInto(src, dest[0..], cap); // text aliases dest
}r.text dangles when normalize returns. expectEqualStrings can pass, then indexOfScalar sees leftover \r in reused stack. This file is not in the PR. Main’s last build-harness is green. Not a finding on this diff. Re-run the workflow before merge-pr; if it dies on the same test again, that is main’s UB, not text-wave.
warm_border #3a2818 on teal_bg is still low contrast. Per-scalar kerning drop is plan-accepted. No host-side unit test of the 1..255 remap. I did not paint /harness at 10 Hz.
Merge guidance
PASS WITH NOTES — review-satisfied for merge-pr once zig → harness.wasm is green on this head (or a no-op re-run). Do not treat the composer_text flake as a text-wave block. The 255→1 hitch can stay residual.
What was not attacked
Live /harness 10 Hz / reduced-motion OS toggle / 25.5 s wrap on a real turn. Prod Gateway.
Summary
Closes #650. Implements plan #655.
Replaces the solid-
warm_accent"Waiting for model…"text inbusy_row.zigwith a left-to-right cyclic WARM color wave — brightness traveling through the glyphs driven by the existing 10 Hz busy tick. The" · mm:ss"clock stays solid and structurally isolated.What shipped
native/harness/src/text_wave.zigrect_spinner.zig)native/harness/src/text_wave.test.zignative/harness/src/busy_row.zigaddTextwithtext_wave.paint(); clock in separatetlblocknative/harness/build.zigtext_waveintest-richdocs/harness-limits.mdArchitecture
addTextwith individual.color_text— the only way to vary color per glyph in dvuitextLayoutblock, never touched by the wave modulerect_spinner.WARM_RAMPreused — same 4-stop LUT as the spinnerTest results (this workspace)
npx tsc --noEmitzig build test-richbuild-harnessTest plan (22 text_wave cases)