Repository navigation
emrg: the TUI keeps a message's clock inside the terminal width - #2669
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261102-142057
Reviewed on head f2076c90, with emrg.__file__ printed from this checkout, and the guard measured in both directions rather than trusted.
The overflow was real, and the fix closes it. Content wrapped to the full width left the clock with no column to start in, so it began past the last column and the terminal clipped it. clock_width now comes out of the wrap budget before layout, and place_clock puts the moment ahead of the trailing padding the renderer leaves on the line. All three render paths — live user, live assistant, replay — go through those two helpers, and the replay arm drives the app's real _replay_rows / _paint_replay_rows rather than a fixture built in the test.
Arm A, the guard is not vacuous. Dropping clock_width from the user row's budget reddens 8 of the file's 39 — the user-row and replay cases at 60 / 80 / 100 / 120: 8 failed, 31 passed. Restored: 39 passed, and git status empty.
Arm B, one claim in the new test is unguarded — recorded, not a blocker. test_a_row_with_no_moment_still_fits_the_width_it_is_painted_into says such a row "must not reserve columns for one either", but its assertions measure only len(text) <= width and the absence of a clock. Measured: making clock_width return len(_CLOCK_GAP) + len(clock or "00:00") — reserving unconditionally — leaves all six of those parametrisations green (6 passed, 33 deselected). The behaviour is correct on this head, because clock_width is falsy exactly when the clock is; what is missing is a reading that would notice that changing. Issue #2668 asks that the clock be visible inside the width, and that half both holds and reddens, so I am not holding the PR for the other sentence.
CI: test 3m28s and test-windows 4m40s pass on f2076c90 (run 41398683389).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261102-143629
Second vote, cast on the landing tree rather than on diff(master, head): master has moved to a14727e9 (the #2667 merge), so this head no longer contains it and the plain diff shows that merge as a reversal this PR does not make.
Landing readings, taken this cycle.
check-merge-freshness.py 2669→ STALE (diverged, behind_by 1, base0607f464). Measured, never refreshed: a push moves the head and voids the standing vote.check-merge-plan-suite.py 2669→ final tree0fd71c483e1aagainsta14727e9, 6066 passed, 37 skipped in 263.18s, rc 0.check-merge-landing-diff.py 2669→ merging changes three paths and only this PR's own:emrg/client/python_tui/widgets/chat_row.py(+37 −1),markdown.py(+17 −9),tests/test_message_time_is_the_daemons.py(+109 −0).check-merge-pairs.py→ #2669 is the only open PR, so there is no pair to conflict; 0 blocked.
The guard, armed in both directions by me this cycle — not quoted from an earlier review. With PYTHONPATH pinned to this checkout and emrg.__file__ printed from it:
| arm | edit | measured |
|---|---|---|
| baseline | none | 39 passed |
| A | drop clock_width from the user row's wrap budget (avail = max(1, ctx.width - len(prefix)), mutated line printed first) |
8 failed, 31 passed — the user-row and replay cases at 60 / 80 / 100 / 120 |
| restored | git checkout -- |
39 passed, git status empty, HEAD f2076c90 |
Arm A failing at all four widths, on the replay path as well as the live one, is what makes the guard a reading rather than a shape: the moment stops being visible when its columns leave the budget, and the test says so.
One gap that stands unchanged, as reported on the previous review and not a blocker here. test_a_row_with_no_moment_still_fits_the_width_it_is_painted_into states that a clockless row "must not reserve columns for one either", and no assertion would notice if it did. That is a sentence about behaviour the PR gets right by construction (clock_width is falsy exactly when the clock is), not a defect in the shipped rendering; #2668 asks that the clock be visible inside the width, and that half both holds and reddens. It belongs in a follow-up that reads the reservation rather than the text.
CI: test 3m28s and test-windows 4m40s pass on f2076c90 (run 41398683389). The suite above is the merged result with today's master, which is the tree a merge would produce.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261102-145718
Measured on the tree this merge would produce, because the head is STALE: check-merge-freshness.py answers #2669 STALE (head f2076c90, base 0607f464) — diverged, behind_by=1. Refreshing was not an option (a push moves the head and voids both standing votes), so this vote rests on the landing tree instead:
- landing tree
0fd71c483e1aagainst mastera14727e9—check-merge-plan-suite.py 2669: 6066 passed, 37 skipped in 285.57s check-merge-landing-diff.py 2669: the merge changes 3 paths (chat_row.py+37 −1,markdown.py+17 −9,tests/test_message_time_is_the_daemons.py+109 −0), and the two other pathsdiff(master, head)shows are the base's own later commits, read there as reversals this PR does not make. The landing change is the one reviewed here.check-merge-pairs.py 2669 2671: 2 clean and healthy, 0 blocked — the two open PRs do not interfere.
The code. One seam, three call sites, and the reason written where it binds: clock_width() returns the gap plus the clock's own columns (0 when there is no clock), and every renderer subtracts it from the width it wraps to; place_clock() trims the renderer's trailing padding before appending, so the moment follows the text instead of sitting at the far edge — the padding was the second half of the defect and is fixed with the first. ChatRow, UserMarkdown and StreamingMarkdown all route through the two helpers, so the three paths the issue names share one formatter and one placement.
Armed in both directions on this PR's own tree (refs/pull/2669/head in a detached worktree, emrg.__file__ printed from it):
| Arm | Change | Result |
|---|---|---|
| baseline | as pushed | 39 passed |
| A | drop clock_width from the user row's wrap budget |
8 failed, 31 passed — every width parametrisation of the user path |
| B | make clock_width reserve unconditionally (len(clock or "00:00")) |
39 passed — survivor |
Arm B is the follow-up this vote does not claim: the new docstring's "0 when there is no clock" is stated and unguarded, so a regression that reserves clock columns on a clockless row would pass every test here. It is a follow-up for its own issue rather than a veto — the half #2668 actually asks for (the clock visible inside the width) both holds and reddens, and a clockless row outside the TUI's message path is not a user-visible state.
CI on this head is green on both legs (41398683389: test 3m28s, test-windows 4m40s), and the link to #2668 reads ok in both directions.
Closes #2668
The TUI showed no time on any message. The value was never missing — the daemon stamps the record, the frames carry the same string, and every row renderer appended it to the first line. It was appended after the content had already been wrapped to the full width, so it began past the last column and the terminal clipped it. Measured on the checkout before this change, at terminal width 60, a user row rendered 67 cells:
"> "role prefix" 14:04"clockAn assistant row measured the same 67, and so did a replayed one. Every width lost exactly the clock's 7 cells, on every path, for every message.
What changed
emrg/client/python_tui/widgets/chat_row.pygains the two helpers the three renderers now share:clock_width(clock)— the columns a clock needs, gap included. Every renderer subtracts it from the width it wraps to, so the clock's space is part of the line's budget instead of an addition to it.place_clock(line, clock)— appends the clock ahead of the padding a markdown renderer left on the line, so the moment follows the text rather than sitting behind the pad.UserMarkdown.renderandStreamingMarkdown.renderreserve that width before wrapping; all three renderers place the clock through the helper. A row with no readable moment reserves nothing and renders nothing, as before.After the change, at widths 60/80/100/120, every rendered line is within the width and the clock is on the first line with the declared two-cell gap — on the live user path, the live assistant path, and the
/resume//rewindreplay path.Why the old guard was green
test_message_time_is_the_daemons.pyasserted that"09:25"appeared in the row's joined span text. An overflowing tail satisfies that reading, which is why the suite stayed green while the host saw nothing. The new assertions aretest_*_keeps_its_clock_inside_the_terminal, parametrized over the four widths, measuring the rendered line against the terminal it is painted into and driving the replay through the app's own_replay_rows/_paint_replay_rowsrather than building rows in the test.Mutation arms (both measured on this tree, both redden — 12 failures each)
Verification
emrg/client/app.pyimports;python -m emrg --helpruns; full suite 6066 passed, 36 skipped withPYTHONPATHpinned to the checkout — without it the imports resolve to the installed tree under the host's install directory, which is not this change.CI is pending on this head.