Repository navigation
Keep History controls reachable as entries accumulate - #1041
Conversation
The accepted wide row left its map to the drawing engine's own fit(), but that engine answers from its own prior renders rather than this frame's measuring pass — measured, an eighty-four-cell map the measuring pass confirmed whole came back 82.0078125 from the engine that commits the frame, once a twelfth entry grew the map's text past whatever the drawing engine had last measured for it. The control beside the map inherited the fraction and ReplRenderError ended the running command. Stating the map at the width this frame already measured removes the second, independent ask, mirroring how the row's gap is already stated rather than left to grow() beside it.
PR #1041: Keep History on whole cells after twelve entries3 files, +170 / -65 Scope✅ PR scope looks good. StructuralOxlint structural signals:
Slop✅ Slop indicators look low. Static AnalysisOxlint: 130 diagnostics across 2 files (10 rules) no-shadow (73): packages/cli/src/repl/program.ts, packages/cli/src/repl/application.ts CorrectnessNo extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Review of 10a936f4 (first green CI: run 41253230878)
Verdict: request changes. (Posted as a comment review: GitHub does not let the author account of a pull request set the request-changes state.) The change does what it says for the twelfth entry, and I reproduced both sides: base c731892f ends on entry 12 with the issue's ReplRenderError ... x=120.0078125 in 3 of 3 runs, and this head completes 14 entries, Draft typing and History open/navigate/close in 3 of 3 runs. One correctness finding keeps it short of #1017's acceptance ("twelve and further entries finish without ending the command"; "every History/action control remains reachable"): at 160x36 the nineteenth entry ends the command with the same refusal, and [exit] stops being drawn after the eighteenth. Details and a concrete recommendation are in the inline comment on wideHistoryMapWidth.
What I checked
- The three-file diff against
c731892f; the accepted non-widehistorySplitpath and the renderer'sintegral()refusal are untouched. - The cause: the map was the one box on the accepted wide row still asking the drawing engine to
fit()its text while the gap beside it was stated from the measuring pass; stating the map from the same pass is consistent withpackages/cli/src/repl/renderer.ts's two-engine design and with the existinghistoryGapidiom. The probe pass reads the fitted width before the value is stated, so there is no circularity. - The regression test's evidence (inline comment: one assertion holds before the drawer opens, and the entry count stops short of the row-full point).
- Comment volume and one untrue doc claim (inline comment, non-blocking).
Performance (measured)
Workload: the repository's own page object at 160x36, sequential plain entries awaited to their rail mark, 24 Draft keystrokes, three History cycles (open, ArrowUp x2, ArrowDown x2, close). Boundary: completed presentation write into the test suite's in-process terminal emulator. That is the application's frame output, not a TTY flush and not a visible paint. Same host, Deno 2.9.7, runs sequential, 3 per condition, about 1.6 s active window each, startup excluded.
Equal state on both trees (11 entries, then Draft and History):
| metric | head (3 runs) | base (3 runs) |
|---|---|---|
| active-window writes per second | 62.5, 62.8, 60.1 | 62.6, 61.8, 61.7 |
| Enter to rail mark, p50 ms | 61.1, 59.1, 59.4 | 59.7, 60.5, 60.0 |
| Draft keystroke to shown, p50 / p95 ms | 6.6 / 7.0, 6.6 / 6.9, 6.6 / 7.3 | 6.6 / 7.0, 6.8 / 7.5, 6.5 / 6.8 |
| History open/nav/close, p50 / p95 ms | 8.8 / 17.8, 8.7 / 18.6, 8.9 / 54.2 | 8.5 / 17.2, 8.8 / 18.8, 8.9 / 17.7 |
Warmed prefix (entries 1 to 11 inside the 14-entry runs, 33 samples pooled): Enter to rail mark p50 / p95 64.7 / 116.8 ms at head against 65.0 / 116.0 ms at base; write interval p50 about 10 ms on both. Head at 14 entries: write interval p50 8.3 to 8.7 ms, p95 45 to 46 ms, worst 77 ms; Draft keystroke p50 6.4 to 6.6 ms; History p50 8.7 to 9.0 ms. The 54.2 ms History p95 is one sample in one run; the other five runs sit at 17 to 19 ms. The "Enter to rail mark" latency includes executing the entry and journal I/O, not only rendering.
CPU profile (Node v26.5.1 --cpu-prof, head, 14-entry workload): about 1.17 s of 4.8 s sampled is the workload, the rest is loader startup. Hot stacks are the emulator's cell reads in the page object's write hook (harness cost), the wasm layout engine under measure from fitPlain/prepareFrame, and detach under describe -> drawerContent -> presentationFor. Nothing this PR adds appears as a leaf.
Conclusion: no measurable cost from the change; head and base agree within run-to-run noise on every equal-state metric.
Limitations: in-process emulator boundary; Deno 2.9.7 has no --cpu-prof, so the profile is from Node and is not comparable to the Deno timings; three runs per condition; the baseline cannot reach entries 12 to 14, so those have no baseline; no FPS budget exists for this surface and none is asserted.
| export function wideHistoryMapWidth( | ||
| widthOf: (id: string) => number | undefined, | ||
| ): number | undefined { | ||
| const map = widthOf(HISTORY_MAP); |
There was a problem hiding this comment.
Correctness: at 160x36 the command still ends, now on the nineteenth entry, and [exit] is lost at the eighteenth.
Consequence: #1017's acceptance is "twelve and further entries finish without ending the command" and "every History/action control remains reachable". At this head, continuing the issue's own workload past fourteen entries at the accepted size still ends xmd repl.
Evidence (same page object the regression uses, useReplPage at 160x36, Entry N text. awaited to its rail mark, run at this head):
- Entry 17: the History row fills all 160 columns:
HISTORY 1 ✓ 2 ✓ ... 17 ✓ ── HEAD · SETTLED [history] [exit]. - Entry 18: the row reads
... 18 ✓ ── HEAD · SETTLED [history]and[exit]is no longer drawn. - Entry 19:
ReplRenderError: the engine placed node-15 at a fractional position (x=120.0091323852539, y=34, width=11, height=1); the command ends. - At 200 columns the same sequence happens at entries 24 and 25 (
x=160.00685119628906), so the point tracks the row width. Basec731892fends at entry 12 (x=120.0078125) in 3 of 3 runs; this head completes 14 in 3 of 3.
Why: compactHistoryRuns emits a run for every entry (about three cells each) and nothing bounds it. The stated width is the probe pass's fitted width of the whole text, which protects the row only while that text fits beside the controls. Once it does not, the row is back in the arrangement the renderer refuses, and the control row has already dropped a control to get there.
Recommendation: bound the accepted wide map by what the controls need, the way the non-wide row already does. prepareFrame runs the parts probe (historyProbeOf / historySplitOf) only under profileFor(view.size) !== "wide"; running it for the wide profile too and stating the map at min(natural, row - needed) with the gap derived from that keeps the accepted layout while the text fits and shortens the band before a control is lost or refused. Windowing the compact rail to the entries that fit is the other option, but that is a visual decision for the accepted layout's owner. Either way, extend the regression past the row-full point at 160 columns (entry 19 today) instead of stopping at 14, since 14 sits inside the range the fix already covers.
| // History's own actions stay reachable: its drawer still opens and shows | ||
| // the entries just admitted. | ||
| yield* openHistory(page); | ||
| expect(page.screen().shows("Entry 14")).toBe(true); |
There was a problem hiding this comment.
Test evidence: this assertion holds before the drawer opens.
page.screen().shows("Entry 14") is already true on the screen before openHistory (the transcript shows the entry's text; measured true at this head in every run), so it does not show that the drawer lists the entries just admitted. openHistory itself already proves the drawer opened and owns its keys.
Recommendation: read the drawer's own rows. packages/cli/tests/repl-agent-journey.test.ts has a historyMarkers helper over drawerLines that filters admitted|recorded rows, and chooseMarker scopes its lookup rightOf: "[close]"; asserting the admitted entry's marker that way is discriminating. Two smaller notes: expect(page.repl.terminal.ending).toBeUndefined() on line 2918 cannot fail here, because the render refusal propagates out of submit as a thrown error before this line runs (in the base runs it arrives out of entrySettles with terminal.ending still undefined), so the fourteen awaited rail marks are the real evidence; and per the finding on wideHistoryMapWidth, fourteen entries is below the point where the rail fills the row at this width.
| * and absent at every other profile, whose rows ask their map a different | ||
| * way. | ||
| */ | ||
| readonly wideHistoryMap?: number | undefined; |
There was a problem hiding this comment.
Minimalism: the same incident is narrated three times, and this doc overstates absence. Non-blocking.
The 84 against 82.0078125 story appears here, in wideHistoryMapWidth's doc, and in a twenty-line inline comment on the HISTORY_MAP box. AGENTS.md's Code Rules ask comments to describe the surprising behavior; the reason fits in one place (the drawing engine answers a fit() from its own render history, so a width this frame already measured is stated rather than asked again), and the measured numbers belong in the commit message and this PR.
Also, prepareFrame computes historyWideMap on every profile, and the probe manifest holds box:history:map on all of them, so off the wide profile the value is present and unused rather than "absent at every other profile".
Recommendation: keep the reason once on wideHistoryMapWidth, make this field's doc and the box comment one line each pointing to it, and either compute the value only on the wide profile (beside the existing profileFor(view.size) !== "wide" branch in prepareFrame) or drop the absence claim.
, PR #1041 review) The prior fix stated the map's width from a measured value instead of the drawing engine's own fit(), but left that width unbounded. The map's text grows about three cells an entry, so once it outgrew what the row's gap-and-controls split could still spare, the engine's own remainder arithmetic went fractional again — and separately, the measuring engine's own fit() answer for a long enough map can itself go fractional, not only the drawing engine's. At 160x36 the eighteenth entry silently dropped [exit] and the nineteenth crashed the command; at 200x36 the same thing happened at twenty-four and twenty-five. wideGapWidth and wideHistoryMapWidth are consolidated into one wideHistoryWidths: it bounds the map to what the row's controls need once the half-split gap would leave them short, or once the map's own width cannot be trusted as a whole number at all, dropping the gap to zero in that regime rather than taking a share of the controls' own room. The accepted row's appearance is unchanged while the text fits. The regression test now drives past the row-full point at both 160 and 200 columns, checks every action stays reachable at every entry, reads the drawer's own admitted rows instead of the transcript (which was already true before the drawer opened), and confirms History selection and the draft survive drawer interaction. Consolidates the repeated measuring/drawing rationale into wideHistoryWidths's own doc and corrects the field docs, which now compute only on the wide profile.
taras
changed the title
Growing History maps can push action controls off screen and pass fractional geometry to the renderer. The wide layout now bounds the map and gap using the complete controls’ measured space, keeping History and Exit reachable as entries accumulate. Strict whole-cell validation remains in place.
The terminal regression checks every entry through 24 entries at 160 columns and 30 at 200, reads the drawer’s own rows, and preserves selection and the draft. An isolated cold-reopen addendum also passes with 18 completed entries plus one unfinished submission at 160×36 and 200×36, with no provider prompts or replay. The user’s saved session is untouched.
All three first-green review findings are addressed. Five focused test files and changed-file lint/format pass; the affected original Opus review and cold-reopen verification pass at the signed head. The saved performance harness was rerun with its limitations documented. CI verifies the pushed head before delivery; the one full Fable review does not repeat.
Closes #1017