refactor(panel): split render into region builders reading the model #9

Merged

yuris merged 2 commits from refactor/t5b-render into main 2026-09-01 07:21:41 +00:00

Conversation 2 Commits 2 Files changed 2 +565 -367

yuris commented 2026-09-01 06:49:26 +00:00

Owner

Copy link

Wave 4, second and last. PR #8 built the model beside an untouched render();
this moves render() onto it and splits the 256-line function into region
builders. After this render() assembles regions and derives nothing.

Region-builder inventory

after	lines	before	lines
render (assembly)	46	render	256
buildCard — clock, subject, today, action	48	(inline in render)	—
buildRecent — the chip row	20	recentButtons	20
buildTrend — the chart caption	10	(inline in render)	—
buildChart — the 14 day bars	35	dayColumns	40
buildSearch — the field and its magnifier	15	(inline in render)	—
scopeButton	10	(closure inside render)	—
buildScope — two scope buttons, chip, total	33	(inline in render)	—
buildList — the windowed rows	52	activityRows	62
buildPager — number strip and range label	55	(inline in render)	—
display — the (value, isKey) contract	3	tagLabel	3
	327		381

viewModel went 127 → 134: one field and its comment (below). panel.luau
1547 → 1607, test.luau 1234 → 1372.

Three seams differ from the brief's sketch, because that is what the code is:

• The chip is buildScope's, not buildSearch's. It rides in the header
  row so that appearing and disappearing costs the list no rows — render's own
  comment said so on main. The search box sets it; the scope row shows it.
• buildTrend exists as its own ten lines. It is a sibling of the chart row
  in the right column, not a child of it, and folding it into buildChart would
  have changed the tree.
• scopeButton moved to module level from a closure inside render. Same
  three props, and one fewer closure per repaint — which is not a saving worth
  quoting against what the split does cost. Measured, below.

What the split costs, measured

render on main derived as it painted; render here builds a 35-table model
first, every repaint. That is a real cost and the honest number is a loss, not a
saving. Both panels loaded in one luau process, worst state the panel can reach
(200 all-time rows, page 4 of 25), min of 9 interleaved rounds x 500, five runs:

per repaint	main	this branch
wall time	0.048–0.051 ms	0.051–0.054 ms
heap allocated (min of 60)	51 KiB	57 KiB
ui node / prop tables	104 / 104	104 / 104

+0.003 ms and +6 KiB, or +4–8% of a repaint that uses 0.19% of the 25 ms
per-handler budget: 0.19% → 0.21%. A page click pays one extra viewModel()
on top, since the handler reads pager.pageBase off the model before rendering:
0.049 ms → 0.070 ms of 25 ms. Equal node and prop counts are the tree identity
restated — no builder allocates per row or per day where main did not, and the
whole delta is the model itself.

256 lines of derive-as-you-paint down to 46 lines of assembly, for 0.02% of the
budget. Worth it, and worth stating as a price rather than implied to be free.

render() derives nothing — the audit, case by case

Everything a builder now puts in a prop, and which of the three legal sources it
came from. No case needed a new display field; the one field added is not a
displayed value at all.

From the model — every value in PR #8's field inventory, unchanged.

Layout constants — LEFT_W, RIGHT_W, ACTION_W, SUBJECT_H, BAR_W,
BAR_H, BAR_GAP, LEAD_W, DUR_W, ROW_H, CHIP_H, every gap, padding,
radius, fontSize, fontWeight, colour role, alignment, and the row dot's 8x8.

Constant translation keys, which vary with nothing: panel.title,
panel.recent, panel.no_tags, panel.no_history, panel.nothing_day,
panel.scope_all, panel.clear_filter, panel.search_placeholder,
panel.tag_placeholder, panel.start/panel.stop, panel.newer/panel.older.
The two in render itself — panel.title and panel.recent — are the left
column's section labels; they head the two builders below them and are part of
the composition, not of a region.

Substituted keys where the builder keeps the literal and the model carries the
value, exactly as #8 argued: panel.today{todayTotal},
panel.trend/panel.trend_tag{DAYS, filter}, panel.filter_chip{filter},
panel.filter_by{tooltipTag}, panel.start_tag{tag},
panel.page_range{firstShown, lastShown, total}.

Literals that are a shape and not text — ‹ and ›. --:-- and -- were
literals in render on main; both are the model's now (clockText,
Fmt.dayLabel), so the builders hold two fewer than render did.

Six computations survive in builders. Each one is layout, and here is the
argument for each rather than an assurance:

1. math.max(2, math.floor(BAR_H * day.seconds / peak)) — buildChart. A pixel
   height. BAR_H is layout, seconds and chartPeak are the model's; #8
   named this split explicitly and test.luau reads BAR_H back off the tree
   rather than copying it, so the constant is never spelled twice.
2. p.pages <= 1, p.current > 1, p.current < p.pages — buildPager. Whether
   the row exists and whether each arrow is enabled. Comparisons between two
   model numbers; #8 omitted pager.visible for this reason and the omission
   still holds.
3. m.scope == "day" / m.scope == "all" — buildScope. Which of two buttons
   is tinted. A model field against the constant that button names; a
   dayIsSelected field would be the same comparison with a longer name.
4. m.filter ~= "" — buildScope. Whether the chip node exists. Same class:
   "" is the model's own spelling of "no tag".
5. subjectW = LEFT_W - ACTION_W - 24 — buildCard. Arithmetic on three layout
   constants. main spelled this twice, once as subjectW and once inline;
   collapsed to the one local, same number.
6. "tag-" .. m.draftKey, "search-" .. m.searchKey, tostring(slot.page) —
   widget identity and a number-to-label. The counter is the model's; the prefix
   is the widget's own name and belongs to whoever builds that widget.

The one thing render does that is not building a node:

shown, matching = m.windowRows, m.pager.total

activityRows used to write shown on its way to building the list. A callback
is resolved by global name at this plugin API and cannot be handed an argument,
so onRowN indexes shown and gotoPage clamps against matching when the
click arrives — both have to outlive the repaint. The model computed both; this
publishes them, and it is the only assignment left in render besides
focusOnce = false, which is wave 5's.

That needed one new model field:

windowRows = windowRows,   -- the slice itself, beside the display shape of it

The raw rows, because filterFromRow compares UNTAGGED and rows[i].tagText
has already turned that into a key. The alternative I rejected: publish
m.rows and have filterFromRow read rows[i].tooltipTag, which is nil
exactly when the tag is the sentinel — #8's review argued that field's presence
is the filter-target answer, so this would have cost zero new fields. I did
not take it, for two reasons: it makes a field named for the tooltip the click
target, and it edits filterFromRow, which #8 hash-pinned as identical to
main's. Challenge it if you disagree; it is a two-line change either way.

windowRows has no prop, so it is pinned in compare() against the rows that
do — same length, same order, and each row's tag against the tagText the
builder displays, keyed where the sentinel needed it.

The pageBase duplication is closed

local pageBase = 1 at :240 is gone, along with pageBase = firstShown in
render and render's second Query.pageStrip call. m.pager.pageBase is the
only spelling left. The handlers read it off the model:

local function gotoStripPage(slot)
	gotoPage(viewModel().pager.pageBase + slot - 1)
end

function onPage1() gotoStripPage(1) end
...
function onPage5() gotoStripPage(5) end

Why this and not baking the page into five handler names: you cannot. The
five names are fixed public surface and the strip slides; a name can only
carry its slot number, which is what the base exists to offset. There is no
third option that keeps one spelling — a narrow local function pageBase() over
the same module state would be a second spelling, which is precisely what the
deferred item forbids.

The cost, measured rather than asserted. One viewModel() on a page click,
in the worst state the panel can reach (all-time, 45 matches, page 4 of 6):

ui nodes per repaint: 103        (each with its own props table)
model tables:          30        (rows 8, slice 8, bars 3, strip 5, chips 4, root + pager)
pure-core calls per model: 12

The click already pays for a repaint that builds the same model plus those 103
nodes, so the handler's extra model is about an eighth of a click it was already
making, and the window it walks is bounded by ALL_WALK_MAX = 200. Against the
25 ms per-handler budget this is not measurable. The thing that would have
been expensive is a builder calling viewModel() — one model per region instead
of one per repaint, on a panel that repaints once a second — and test.luau now
pins that count.

One honest consequence, not hidden. pageBase was written at paint time;
the model is read at click time. Those differ only along a path that changes
state without repainting, and there is exactly one: loadDay(index) with an
index days does not hold returns before its render(), reachable via
noctalia msg … day 99. On that path the strip on screen is stale in every
other respect too, and the new reading is the fresher of the two. Noted because
it is a real behaviour delta that the tree harness cannot see.

Also closed, the second REFACTOR.md item: buildChart reads m.chartPeak
instead of calling barPeak() a second time. barPeak now has exactly one
caller, the model.

Tree identity

render()'s output must be identical to main's, not equivalent. A harness
requires git show main:timew/panel.luau alongside this branch's copy — each
gets its own global table from the luau CLI, so both are stubbed identically —
drives the two through the same sequence of shell callbacks, and flattens both
trees with props key-sorted, one node per line plus a child count per node. It
lives outside the repo, because main's copy is not a file in the repo.

en.json is not hand-copied into it: jq generates the table from
timew/translations/en.json, so the two sides resolve keys through the real
locale and a missing key raises.

$ luau treediff.luau
states: 27
bytes: main 164387, branch 164387
differences: 0
tree-identical

27 states. The 14 test.luau drives, plus a bare update() repaint, a day
scope whose filter matches no row, the empty-search state reached by clearing
the chip (search empty with its key bumped, no chip, list back to
everything), all six pager positions on the 6-page list rather than three, both
ends reached through ‹/› so arrow enablement is pinned at both boundaries,
and the five pager handlers each driven from a painted page 4 where the strip's
base is 2. Every state the brief named is in there: filtered day scope, filtered
all-time scope, an untagged row, a day with no intervals, and the deferred
backwards-range edge (17-16 / 12), which is reproduced, not fixed —
gotoPage still clamps against a stale matching and I did not touch it.

The harness fails when it should. Three mutants against the branch copy, each
restored cmp-clean:

mutant	result
buildScope row gap = 4 → 5	27 differences, first at row(align="center", gap=5, …)
selected = row.isFiltered → true	26 differences, named down to the row button
onPage1() → gotoPage(1)	1 difference, in pager handler onPage1 from page 4

The second mutant is why byte counts alone are not the check: gap = 4 → 5 is
byte-identical in length, and only the line diff catches it.

Tests

The existing 1234 lines pass unchanged. Nothing reached into a function I
deleted: dayColumns, activityRows, recentButtons and tagLabel were all
module locals with no export, and the suite reads the tree through the
panel.render stub. Nothing needed adapting — so there is no per-test
justification to give, which is the strongest form of the answer. compare(),
which indexes the tree structurally in all 14 states, is the part that would
have broken first, and it did not.

It also earned its keep: my first buildList dropped the row's
ui.spacer({ flexGrow = 1 }), and compare() caught it at kids[5] before I
had run anything else.

Three new asserts, plus windowRows in compare(). Every one proven live by
mutation, all mutation in a /tmp copy, every restore cmp-clean against the
pre-mutation file.

1. One model per repaint. The trap in this split is a builder calling
viewModel() itself: nothing about the tree would look wrong, so the identity
harness cannot see it. Counted as a parity — Fmt, Parse and Query are
exported by reference, so wrapping their entries is visible inside the module
with no hook the panel would not otherwise have — and a repaint must reach the
pure core exactly as many times as taking the model alone does.

barPeak is a Query entry for this count's sake. As the module local it was
on main it reached none of the three watched tables, so a builder re-deriving
the chart peak added no counted call and this parity passed — the one
duplication the count exists to catch that it could not see. Its body is
main's byte for byte; the declaration line moved, and it now takes the day
list as an argument like every other entry in the pure core, where days is not
yet in scope to be read any other way.

mutant	failure
render calls viewModel() twice	test.luau:1315 — 24 calls against the model's 12
buildChart re-derives Query.barRoles	test.luau:1315 — 15 against 12
the counter stops counting (test-side)	test.luau:1314 — "the counter counts something, or the parity below is vacuous"
the pure core is never handed back (test-side)	test.luau:1310 — the restore canary

The canary was vacuous on the first try: this file's Query alias at :50 is
panel.Query, so comparing them after the restore compared a wrapper with
itself. Fixed by taking the real function before wrapping.

2. render publishes the slice a click indexes.

mutant	failure
render stops publishing shown	test.luau:1340, and :692 on the full suite
render publishes a stale shown	test.luau:1340
filterFromRow reads the wrong field of the slice	test.luau:1340
the model publishes a short slice	test.luau:974 — windowRows is the slice m.rows describes -- 1 vs 3
the model publishes the unwindowed rows	test.luau:974 — 3 vs 2, in the filtered day state

The first three fire at :1340 only once the earlier onIpc("row", …) section
is cut out of a sandbox copy — on the full suite :692 catches them first,
which is the older guard on the same contract. Said plainly rather than claimed
as a first failure. The two fixture preconditions above them (#slice == 2, and
the slice carrying (untagged) rather than the key) are guarded upstream, at
:86 for the sentinel and by compare() for the length; their job is to make
the two clicks below them unambiguous.

3. The pager handlers agree with the model's base. Driven from a painted
page 4 of 6, where the base is 2 rather than 1, and asserting the strongest
form: each handler pages to the number its own button showed.

mutant	failure
onPage1() → gotoPage(1)	test.luau:1362 — "onPage1 pages to the number its button showed, 2"
gotoStripPage drops the base	test.luau:1362
the strip numbers handlers by page, not slot	test.luau:1358 — "the strip's slot 1 is onPage1"

Field comparisons 1385 → 1485: windowRows adds two per row plus one per state.

Gate

$ nix shell nixpkgs#luau -c luau test.luau
viewModel: 14 states, 1485 field comparisons
ok
EXIT=0

$ noctalia plugins lint timew            # from .worktrees/t5b-render, not main's
yuris/timew
  ok

0 errors, 0 warnings
EXIT=0

$ git diff --stat main...HEAD
 test.luau        | 144 ++++++++++-
 timew/panel.luau | 741 ++++++++++++++++++++++++++++++-------------------------
 2 files changed, 534 insertions(+), 351 deletions(-)

$ grep -c 'getfenv\|setfenv' timew/panel.luau
0

$ awk '/^function viewModel\(\)$/,/^end$/' timew/panel.luau | grep -c noctalia
0

luau-analyze reports the same diagnostics as main — the pre-existing host
globals and unused handler names — with one fewer noctalia read, 39 → 38,
because tagLabel's tr call and render's two inline isKey branches
collapsed into one display.

No plugin source added or removed, no timew data touched, the running shell
untouched.

Screenshots to take before merging

Visual verification is yours. The tree is byte-identical for every state below,
so a difference in any of them is a shell-side layout effect the harness cannot
reach:

1. Idle, freshly opened — --:--, the tag field focused, -- on the scope
   button, both empty-state placeholders.
2. Tracking a multi-word tag — the subject as a label, the chip row at its
   four-tag cap, the stop glyph.
3. Day scope with rows, unfiltered — row alignment, the duration column
   against the header total.
4. Day scope filtered by a tag — the chip in the header row, the tinted row
   buttons, the trend caption's tag form.
5. All-time, filtered — the lead column's dates, a dimmer bar on a day
   carrying the tag beside a dark zero day.
6. All-time, page 4 of 6 — the strip reading 2 3 4 5 6 with 4 selected,
   both arrows enabled, and the range label. Click 2 and confirm it lands
   on page 2: that is the pageBase change, and it is the one interaction no
   still image proves.
7. A day with an untagged row — the translated (untagged) label with no
   tooltip on it, and a tooltip on the row below.
8. A day with no intervals — the dated scope button and the empty-list
   placeholder, with the chart still drawn above it.

Wave 4, second and last. PR #8 built the model beside an untouched `render()`; this moves `render()` onto it and splits the 256-line function into region builders. After this `render()` assembles regions and derives nothing. ## Region-builder inventory | after | lines | before | lines | |---|--:|---|--:| | `render` (assembly) | **46** | `render` | **256** | | `buildCard` — clock, subject, today, action | 48 | *(inline in `render`)* | — | | `buildRecent` — the chip row | 20 | `recentButtons` | 20 | | `buildTrend` — the chart caption | 10 | *(inline in `render`)* | — | | `buildChart` — the 14 day bars | 35 | `dayColumns` | 40 | | `buildSearch` — the field and its magnifier | 15 | *(inline in `render`)* | — | | `scopeButton` | 10 | *(closure inside `render`)* | — | | `buildScope` — two scope buttons, chip, total | 33 | *(inline in `render`)* | — | | `buildList` — the windowed rows | 52 | `activityRows` | 62 | | `buildPager` — number strip and range label | 55 | *(inline in `render`)* | — | | `display` — the `(value, isKey)` contract | 3 | `tagLabel` | 3 | | | **327** | | **381** | `viewModel` went 127 → 134: one field and its comment (below). `panel.luau` 1547 → 1607, `test.luau` 1234 → 1372. Three seams differ from the brief's sketch, because that is what the code is: - **The chip is `buildScope`'s, not `buildSearch`'s.** It rides in the header row so that appearing and disappearing costs the list no rows — `render`'s own comment said so on `main`. The search box *sets* it; the scope row *shows* it. - **`buildTrend` exists** as its own ten lines. It is a sibling of the chart row in the right column, not a child of it, and folding it into `buildChart` would have changed the tree. - **`scopeButton` moved to module level** from a closure inside `render`. Same three props, and one fewer closure per repaint — which is not a saving worth quoting against what the split does cost. Measured, below. ## What the split costs, measured `render` on `main` derived as it painted; `render` here builds a 35-table model first, every repaint. That is a real cost and the honest number is a loss, not a saving. Both panels loaded in one `luau` process, worst state the panel can reach (200 all-time rows, page 4 of 25), min of 9 interleaved rounds x 500, five runs: | per repaint | `main` | this branch | |---|--:|--:| | wall time | 0.048–0.051 ms | 0.051–0.054 ms | | heap allocated (min of 60) | 51 KiB | 57 KiB | | `ui` node / prop tables | 104 / 104 | 104 / 104 | **+0.003 ms and +6 KiB, or +4–8% of a repaint that uses 0.19% of the 25 ms per-handler budget: 0.19% → 0.21%.** A page click pays one extra `viewModel()` on top, since the handler reads `pager.pageBase` off the model before rendering: 0.049 ms → 0.070 ms of 25 ms. Equal node and prop counts are the tree identity restated — no builder allocates per row or per day where `main` did not, and the whole delta is the model itself. 256 lines of derive-as-you-paint down to 46 lines of assembly, for 0.02% of the budget. Worth it, and worth stating as a price rather than implied to be free. ## `render()` derives nothing — the audit, case by case Everything a builder now puts in a prop, and which of the three legal sources it came from. No case needed a new *display* field; the one field added is not a displayed value at all. **From the model** — every value in PR #8's field inventory, unchanged. **Layout constants** — `LEFT_W`, `RIGHT_W`, `ACTION_W`, `SUBJECT_H`, `BAR_W`, `BAR_H`, `BAR_GAP`, `LEAD_W`, `DUR_W`, `ROW_H`, `CHIP_H`, every gap, padding, radius, `fontSize`, `fontWeight`, colour role, alignment, and the row dot's 8x8. **Constant translation keys**, which vary with nothing: `panel.title`, `panel.recent`, `panel.no_tags`, `panel.no_history`, `panel.nothing_day`, `panel.scope_all`, `panel.clear_filter`, `panel.search_placeholder`, `panel.tag_placeholder`, `panel.start`/`panel.stop`, `panel.newer`/`panel.older`. The two in `render` itself — `panel.title` and `panel.recent` — are the left column's section labels; they head the two builders below them and are part of the composition, not of a region. **Substituted keys where the builder keeps the literal and the model carries the value**, exactly as #8 argued: `panel.today{todayTotal}`, `panel.trend`/`panel.trend_tag{DAYS, filter}`, `panel.filter_chip{filter}`, `panel.filter_by{tooltipTag}`, `panel.start_tag{tag}`, `panel.page_range{firstShown, lastShown, total}`. **Literals that are a shape and not text** — `‹` and `›`. `--:--` and `--` were literals in `render` on `main`; both are the model's now (`clockText`, `Fmt.dayLabel`), so the builders hold two fewer than `render` did. Six computations survive in builders. Each one is layout, and here is the argument for each rather than an assurance: 1. `math.max(2, math.floor(BAR_H * day.seconds / peak))` — `buildChart`. A pixel height. `BAR_H` is layout, `seconds` and `chartPeak` are the model's; #8 named this split explicitly and `test.luau` reads `BAR_H` back off the tree rather than copying it, so the constant is never spelled twice. 2. `p.pages <= 1`, `p.current > 1`, `p.current < p.pages` — `buildPager`. Whether the row exists and whether each arrow is enabled. Comparisons between two model numbers; #8 omitted `pager.visible` for this reason and the omission still holds. 3. `m.scope == "day"` / `m.scope == "all"` — `buildScope`. Which of two buttons is tinted. A model field against the constant that button *names*; a `dayIsSelected` field would be the same comparison with a longer name. 4. `m.filter ~= ""` — `buildScope`. Whether the chip node exists. Same class: `""` is the model's own spelling of "no tag". 5. `subjectW = LEFT_W - ACTION_W - 24` — `buildCard`. Arithmetic on three layout constants. `main` spelled this twice, once as `subjectW` and once inline; collapsed to the one local, same number. 6. `"tag-" .. m.draftKey`, `"search-" .. m.searchKey`, `tostring(slot.page)` — widget identity and a number-to-label. The counter is the model's; the prefix is the widget's own name and belongs to whoever builds that widget. **The one thing `render` does that is not building a node:** ```lua shown, matching = m.windowRows, m.pager.total ``` `activityRows` used to write `shown` on its way to building the list. A callback is resolved by global name at this plugin API and cannot be handed an argument, so `onRowN` indexes `shown` and `gotoPage` clamps against `matching` when the click arrives — both have to outlive the repaint. The model computed both; this publishes them, and it is the only assignment left in `render` besides `focusOnce = false`, which is wave 5's. That needed one new model field: ```lua windowRows = windowRows, -- the slice itself, beside the display shape of it ``` The raw rows, because `filterFromRow` compares `UNTAGGED` and `rows[i].tagText` has already turned that into a key. **The alternative I rejected:** publish `m.rows` and have `filterFromRow` read `rows[i].tooltipTag`, which is nil exactly when the tag is the sentinel — #8's review argued that field's presence *is* the filter-target answer, so this would have cost zero new fields. I did not take it, for two reasons: it makes a field named for the tooltip the click target, and it edits `filterFromRow`, which #8 hash-pinned as identical to `main`'s. Challenge it if you disagree; it is a two-line change either way. `windowRows` has no prop, so it is pinned in `compare()` against the rows that do — same length, same order, and each row's tag against the `tagText` the builder displays, keyed where the sentinel needed it. ## The `pageBase` duplication is closed `local pageBase = 1` at `:240` is gone, along with `pageBase = firstShown` in `render` and `render`'s second `Query.pageStrip` call. `m.pager.pageBase` is the only spelling left. The handlers read it off the model: ```lua local function gotoStripPage(slot) gotoPage(viewModel().pager.pageBase + slot - 1) end function onPage1() gotoStripPage(1) end ... function onPage5() gotoStripPage(5) end ``` **Why this and not baking the page into five handler names:** you cannot. The five names are fixed public surface and the strip *slides*; a name can only carry its slot number, which is what the base exists to offset. There is no third option that keeps one spelling — a narrow `local function pageBase()` over the same module state would *be* a second spelling, which is precisely what the deferred item forbids. **The cost, measured rather than asserted.** One `viewModel()` on a page click, in the worst state the panel can reach (all-time, 45 matches, page 4 of 6): ``` ui nodes per repaint: 103 (each with its own props table) model tables: 30 (rows 8, slice 8, bars 3, strip 5, chips 4, root + pager) pure-core calls per model: 12 ``` The click already pays for a repaint that builds the same model *plus* those 103 nodes, so the handler's extra model is about an eighth of a click it was already making, and the window it walks is bounded by `ALL_WALK_MAX` = 200. Against the 25 ms per-handler budget this is not measurable. The thing that *would* have been expensive is a builder calling `viewModel()` — one model per region instead of one per repaint, on a panel that repaints once a second — and `test.luau` now pins that count. **One honest consequence, not hidden.** `pageBase` was written at paint time; the model is read at click time. Those differ only along a path that changes state without repainting, and there is exactly one: `loadDay(index)` with an index `days` does not hold returns before its `render()`, reachable via `noctalia msg … day 99`. On that path the strip on screen is stale in every other respect too, and the new reading is the fresher of the two. Noted because it is a real behaviour delta that the tree harness cannot see. Also closed, the second `REFACTOR.md` item: `buildChart` reads `m.chartPeak` instead of calling `barPeak()` a second time. `barPeak` now has exactly one caller, the model. ## Tree identity `render()`'s output must be *identical* to `main`'s, not equivalent. A harness requires `git show main:timew/panel.luau` alongside this branch's copy — each gets its own global table from the luau CLI, so both are stubbed identically — drives the two through the same sequence of shell callbacks, and flattens both trees with props key-sorted, one node per line plus a child count per node. It lives outside the repo, because `main`'s copy is not a file in the repo. `en.json` is not hand-copied into it: `jq` generates the table from `timew/translations/en.json`, so the two sides resolve keys through the real locale and a missing key raises. ``` $ luau treediff.luau states: 27 bytes: main 164387, branch 164387 differences: 0 tree-identical ``` **27 states.** The 14 `test.luau` drives, plus a bare `update()` repaint, a day scope whose filter matches no row, the empty-search state reached by clearing the chip (`search` empty with its key bumped, no chip, list back to everything), all six pager positions on the 6-page list rather than three, both ends reached through `‹`/`›` so arrow enablement is pinned at both boundaries, and the five pager handlers each driven from a painted page 4 where the strip's base is 2. Every state the brief named is in there: filtered day scope, filtered all-time scope, an untagged row, a day with no intervals, and the deferred backwards-range edge (`17-16 / 12`), which is **reproduced, not fixed** — `gotoPage` still clamps against a stale `matching` and I did not touch it. The harness fails when it should. Three mutants against the branch copy, each restored `cmp`-clean: | mutant | result | |---|---| | `buildScope` row `gap = 4` → `5` | 27 differences, first at `row(align="center", gap=5, …)` | | `selected = row.isFiltered` → `true` | 26 differences, named down to the row button | | `onPage1()` → `gotoPage(1)` | 1 difference, in `pager handler onPage1 from page 4` | The second mutant is why byte counts alone are not the check: `gap = 4` → `5` is byte-identical in length, and only the line diff catches it. ## Tests **The existing 1234 lines pass unchanged.** Nothing reached into a function I deleted: `dayColumns`, `activityRows`, `recentButtons` and `tagLabel` were all module locals with no export, and the suite reads the tree through the `panel.render` stub. Nothing needed adapting — so there is no per-test justification to give, which is the strongest form of the answer. `compare()`, which indexes the tree structurally in all 14 states, is the part that would have broken first, and it did not. It also earned its keep: my first `buildList` dropped the row's `ui.spacer({ flexGrow = 1 })`, and `compare()` caught it at `kids[5]` before I had run anything else. Three new asserts, plus `windowRows` in `compare()`. Every one proven live by mutation, all mutation in a `/tmp` copy, every restore `cmp`-clean against the pre-mutation file. **1. One model per repaint.** The trap in this split is a builder calling `viewModel()` itself: nothing about the tree would look wrong, so the identity harness cannot see it. Counted as a parity — `Fmt`, `Parse` and `Query` are exported by reference, so wrapping their entries is visible inside the module with no hook the panel would not otherwise have — and a repaint must reach the pure core exactly as many times as taking the model alone does. `barPeak` is a `Query` entry for this count's sake. As the module local it was on `main` it reached none of the three watched tables, so a builder re-deriving the chart peak added no counted call and this parity passed — the one duplication the count exists to catch that it could not see. Its body is `main`'s byte for byte; the declaration line moved, and it now takes the day list as an argument like every other entry in the pure core, where `days` is not yet in scope to be read any other way. | mutant | failure | |---|---| | `render` calls `viewModel()` twice | `test.luau:1315` — 24 calls against the model's 12 | | `buildChart` re-derives `Query.barRoles` | `test.luau:1315` — 15 against 12 | | the counter stops counting (test-side) | `test.luau:1314` — "the counter counts something, or the parity below is vacuous" | | the pure core is never handed back (test-side) | `test.luau:1310` — the restore canary | The canary was vacuous on the first try: this file's `Query` alias at `:50` *is* `panel.Query`, so comparing them after the restore compared a wrapper with itself. Fixed by taking the real function before wrapping. **2. `render` publishes the slice a click indexes.** | mutant | failure | |---|---| | `render` stops publishing `shown` | `test.luau:1340`, and `:692` on the full suite | | `render` publishes a stale `shown` | `test.luau:1340` | | `filterFromRow` reads the wrong field of the slice | `test.luau:1340` | | the model publishes a short slice | `test.luau:974` — `windowRows is the slice m.rows describes -- 1 vs 3` | | the model publishes the unwindowed rows | `test.luau:974` — `3 vs 2`, in the filtered day state | The first three fire at `:1340` only once the earlier `onIpc("row", …)` section is cut out of a sandbox copy — on the full suite `:692` catches them first, which is the older guard on the same contract. Said plainly rather than claimed as a first failure. The two fixture preconditions above them (`#slice == 2`, and the slice carrying `(untagged)` rather than the key) are guarded upstream, at `:86` for the sentinel and by `compare()` for the length; their job is to make the two clicks below them unambiguous. **3. The pager handlers agree with the model's base.** Driven from a painted page 4 of 6, where the base is 2 rather than 1, and asserting the strongest form: each handler pages to the number its own button *showed*. | mutant | failure | |---|---| | `onPage1()` → `gotoPage(1)` | `test.luau:1362` — "onPage1 pages to the number its button showed, 2" | | `gotoStripPage` drops the base | `test.luau:1362` | | the strip numbers handlers by page, not slot | `test.luau:1358` — "the strip's slot 1 is onPage1" | Field comparisons 1385 → 1485: `windowRows` adds two per row plus one per state. ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 14 states, 1485 field comparisons ok EXIT=0 $ noctalia plugins lint timew # from .worktrees/t5b-render, not main's yuris/timew ok 0 errors, 0 warnings EXIT=0 $ git diff --stat main...HEAD test.luau | 144 ++++++++++- timew/panel.luau | 741 ++++++++++++++++++++++++++++++------------------------- 2 files changed, 534 insertions(+), 351 deletions(-) $ grep -c 'getfenv\|setfenv' timew/panel.luau 0 $ awk '/^function viewModel\(\)$/,/^end$/' timew/panel.luau | grep -c noctalia 0 ``` `luau-analyze` reports the same diagnostics as `main` — the pre-existing host globals and unused handler names — with one fewer `noctalia` read, 39 → 38, because `tagLabel`'s `tr` call and `render`'s two inline `isKey` branches collapsed into one `display`. No plugin source added or removed, no `timew` data touched, the running shell untouched. ## Screenshots to take before merging Visual verification is yours. The tree is byte-identical for every state below, so a difference in any of them is a shell-side layout effect the harness cannot reach: 1. **Idle, freshly opened** — `--:--`, the tag field focused, `--` on the scope button, both empty-state placeholders. 2. **Tracking a multi-word tag** — the subject as a label, the chip row at its four-tag cap, the stop glyph. 3. **Day scope with rows, unfiltered** — row alignment, the duration column against the header total. 4. **Day scope filtered by a tag** — the chip in the header row, the tinted row buttons, the trend caption's tag form. 5. **All-time, filtered** — the lead column's dates, a dimmer bar on a day carrying the tag beside a dark zero day. 6. **All-time, page 4 of 6** — the strip reading `2 3 4 5 6` with 4 selected, both arrows enabled, and the range label. **Click `2`** and confirm it lands on page 2: that is the `pageBase` change, and it is the one interaction no still image proves. 7. **A day with an untagged row** — the translated `(untagged)` label with no tooltip on it, and a tooltip on the row below. 8. **A day with no intervals** — the dated scope button and the empty-list placeholder, with the chart still drawn above it.

yuris added 1 commit 2026-09-01 06:49:26 +00:00

refactor(panel): split render into region builders reading the model ... ee2291fc4f

render() was 256 lines doing three jobs; PR #8 extracted the first into a
pure viewModel(). This moves render() onto it: one model per repaint, one
builder per region, and assembly for what is left. Every value a builder
puts in a prop is a model field or a layout constant.

Closes the pageBase duplication: the module-level `local pageBase = 1` is
gone and onPage1()..onPage5() read m.pager.pageBase off the model, which
makes it the only spelling of the strip's base. buildChart reads
m.chartPeak rather than calling barPeak() a second time.

The rendered tree is byte-identical to main's across 27 states, proven by
a harness that renders both files under the same stubs and the real
en.json and diffs the flattened trees. The existing suite passes
unchanged; three new asserts pin one model per repaint, the published
window slice, and the pager handlers against the model's base.

yuris reviewed 2026-09-01 07:09:37 +00:00

yuris left a comment

Copy link

Verdict: APPROVE

Reviewed by rebuilding the acceptance check rather than reading yours: a second
tree-identity harness, my own core-call instrumentation, my own timings, and
mutation against a throwaway copy of the worktree. The tree holds. One test-side
hole to close, two notes.

Findings

1. barPeak is invisible to the one-model-per-repaint parity — P2

timew/panel.luau:929, timew/panel.luau:1008, guarded at test.luau:1303.

The parity counts calls into Fmt/Parse/Query. barPeak is a module local
whose body reaches none of them:

$ sed -n '929,937p' timew/panel.luau
local function barPeak()
	local peak = 1
	for _, day in ipairs(days) do
		if day.seconds > peak then
			peak = day.seconds
		end
	end
	return peak
end

So a builder that re-derives the peak adds zero counted calls. The comment
directly above the parity names this exact duplication as what it is guarding
against — test.luau:1261-1262 "which is what main did twice -- barPeak again in
dayColumns, Query.window again in activityRows" — and it cannot see it. Both
spellings pass:

$ sed -i 's|^\tfor _, day in ipairs(chart) do$|\tpeak = barPeak()\n\tfor _, day in ipairs(chart) do|' timew/panel.luau
$ luau test.luau
viewModel: 14 states, 1485 field comparisons
ok

$ sed -i 's|^\tfor _, day in ipairs(chart) do$|\tfor _, d in ipairs(days) do if d.seconds > peak then peak = d.seconds end end\n\tfor _, day in ipairs(chart) do|' timew/panel.luau
$ luau test.luau
viewModel: 14 states, 1485 field comparisons
ok

The tree-identity harness cannot see it either — it is a cost, not a value — so
this mutant survives both gates:

$ luau treediff.luau          # with buildChart re-deriving the peak
states: 32
bytes: main 244419, branch 244419
node lines compared: 2372 per side
differences: 0
tree-identical

barPeak is already a pure function of days; it only takes its argument
implicitly. Route it through the table the counter watches and the existing
assert covers it for free:

function Query.barPeak(days)

with its one call site at :1008:

	local chartPeak = Query.barPeak(days)

Proven, on top of that change: the suite still passes clean, and the mutant that
passed above now dies where it should.

$ luau test.luau                       # fix applied, no mutant
viewModel: 14 states, 1485 field comparisons
ok

$ luau test.luau                       # fix applied, buildChart re-derives
./test.luau:1303: a repaint made 14 pure-core calls against the model's 13: render is not assembling one model, or a builder is re-deriving one

That moves barPeak's declaration line, which #8 hash-pinned; the body is
untouched. If you would rather not move it, the honest alternative is to narrow
test.luau:1261-1262 so it stops claiming to cover the barPeak half — but
then the duplication your own comment names has no guard at all. I prefer the
move.

2. test.luau:1323 names the published slice and asserts the model's — P3

$ sed -n '1322,1323p' test.luau
local slice = panel.viewModel().windowRows
assert(#slice == 2, "two intervals on the day, so two rows in the published slice")

The subject is viewModel().windowRows, not the module local render published
into. The publication is genuinely pinned two lines down, by onRow1/onRow2
landing on the right filter — I killed a dropped shown, matching = … and it
died at test.luau:684 — so nothing is weakened. Only the message overclaims.

assert(#slice == 2, "two intervals on the day, so two rows in the model's slice")

3. The repaint is measurably slower and heavier; the only stated perf claim is a saving — P3

timew/panel.luau:1290. The PR body claims scopeButton at module level "costs
a once-a-second repaint one fewer closure". True, and it is one closure against
the 35-table model that render now builds every repaint and main never built
at all. Measured, both sides in one process, worst state the panel can reach
(200 all-time rows, page 4 of 25), min of 9 interleaved rounds x 500:

main   repaint             0.0482 ms
branch repaint             0.0528 ms
main   viewModel()         0.0186 ms
branch viewModel()         0.0184 ms
repaint: branch - main = +0.0046 ms  (+9.5%);  25 ms budget uses 0.19% -> 0.21%

heap allocated by ONE call (min / median of 60, KiB)
main   repaint         min    51.00  median    52.00
branch repaint         min    58.00  median    58.00

ui nodes/props tables per repaint:  main 104/104   branch 104/104
model tables the branch repaint adds: 35

No builder allocates per row or per day where main did not — the node and prop
counts are equal, which is the tree identity restated. The whole delta is the
model, +7 KiB and +4.6 us, against a 25 ms budget. Nothing to fix in the code;
the fix is to say so, since "one fewer closure" is the only number in the PR
body and it points the wrong way.

Verified

Tree identity — my own harness, 32 states, zero differences. Both
git show main:timew/panel.luau and this branch loaded in one luau process,
each with its own global table, stubbed identically, driven through the same
sequence, flattened one line per node with props key-sorted and a child count.
tr resolves through a table jq generates from timew/translations/en.json,
so a key no locale defines raises.

$ luau treediff.luau
tree identity: main vs refactor/t5b-render
states: 32
bytes: main 244419, branch 244419
node lines compared: 2372 per side
differences: 0
tree-identical

The harness asserts its own two soundness preconditions: that the two modules do
not share a global table (they would otherwise both write one panelTree and
every comparison would be vacuous), and that no function value ever reaches a
prop slot — the no-closures-in-callbacks rule, checked on every one of those
2372 nodes rather than argued.

States: idle; a scope-click repaint; onOpen's own paint with the focus request
still pending; tracking a multi-word tag; a second-tick repaint while tracking;
tracking untagged; day scope with rows; day scope filtered; day scope filtered by
a tag no row carries; all-time; all-time filtered by a tag some days carry; the
chip cleared back to an empty search; a day with an untagged row; pages 1-6 of a
6-page list; both ends reached through </>; the five named page handlers each
driven from a painted page 4 where the base is 2; a day with no intervals; the
backwards range; the bar's published elapsedText; an untagged row in the
all-time list; the search box mid-typing.

The backwards range is reproduced, not fixed — 17-16 / 12 on both sides.

Nine mutants against the branch copy, every one killed, every one restored
cmp-clean:

mutant	differences
buildScope row gap = 4 -> 5	32
buildList selected = row.isFiltered -> true	133
onPage1() -> gotoPage(1)	12
subjectW - 24 -> - 25	64
buildList drops the row spacer	873
buildScope drops its spacer	1103
math.max(2, ...) -> math.max(1, ...)	58
buildPager p.current > 1 -> true	2
display drops the tr	31

The max(2 -> 1) mutant is byte-count-identical on both sides (244419 each) and
shows up only in the line diff, which is the case for not trusting a byte total.

One model per repaint — instrumented, not assumed. Wrapping every
Fmt/Parse/Query entry and counting a bare update() against one
viewModel(), in the worst state:

branch  function                repaint    model
        Fmt.clock                     1        1
        Fmt.dayLabel                  1        1
        Fmt.span                      1        1
        Query.barRoles                3        3
        Query.pageCount               1        1
        Query.pageOf                  1        1
        Query.pageStrip               1        1
        Query.tagColor                8        8
        Query.window                  1        1
        TOTAL                        18       18

Query.window is 1 per repaint and viewModel calls it exactly once, so
render builds exactly one model and no builder re-derives — subject to finding
1, which is the one derivation this instrument cannot see. main scores 18/18
too, which is the like-for-like the timing above compares.

The pageBase collapse — the staleness question, ruled. There is exactly
one path in this module that moves pager state and returns without a repaint,
and it is the one you named: loadDay(index) early-returns at
timew/panel.luau:674 when days[index] is nil, after having already written
listOffset, selected and mode. Reachable as
noctalia msg plugin yuris/timew:panel all day 99. Every other mutator renders.
I drove it on both sides:

main    painted   strip [2->onPage1 3->onPage2 4->onPage3 5->onPage4 6->onPage5]  range 25-32 / 200
main    day 99    screen unchanged (no repaint)
main    onPage1() list "Nothing tracked"
branch  painted   strip [2->onPage1 3->onPage2 4->onPage3 5->onPage4 6->onPage5]  range 25-32 / 200
branch  day 99    screen unchanged (no repaint)
branch  onPage1() list 17:48 | 12:00 | 09:00

main reads the base its last paint wrote, 2, and pages an all-time base into a
three-row day list: page 2, which is empty. The branch reads a base derived from
the state the click actually finds, 1, and shows the day. The branch is
right, and by a wider margin than the PR body claims — this is not "the fresher
of two equally stale readings", it is the difference between an empty list and
the correct one. The strip on screen is stale in every other respect too (the
scope switch is lying as well), which is why the honest reading is the fresh one.
The counter-argument that main honours the number the button showed does not
survive the screenshot: the number the button showed belongs to a list that is no
longer on screen.

main's second divergence window — pageBase = firstShown sits inside
if pages > 1 at main:1394, so a list that shrinks to one page leaves the
module local stale — closes with it, and was unreachable anyway because a
single-page list draws no strip to click.

The forward declaration is one binding, not two.

main    env.viewModel=nil  env.render=nil  env.onPage1=function  module export present=true
branch  env.viewModel=nil  env.render=nil  env.onPage1=function  module export present=true

function viewModel() at :967 assigns the local viewModel at :271; nothing
leaks to the module's global table, exactly as render has always behaved. And
onPage1() in the probe above executed rather than raising, which is the direct
proof that gotoStripPage's upvalue is the binding the definition fills.

One spelling of pageBase left. local pageBase = 1 (main:240) is gone;
the branch spells it in comments, the viewModel local at :1036, the model
field at :1092, and the single read at :877.

The six layout claims, one at a time. All six hold.

1. math.max(2, math.floor(BAR_H * day.seconds / peak)) — layout, and
   character-identical to main:1115. peak cannot be zero on either side:
   barPeak seeds peak = 1 and its body is cmp-identical to main's, and the
   max(2, ...) floors every bar at 2 px besides.
2. p.pages <= 1, p.current > 1, p.current < p.pages — layout: whether the
   row exists and whether each arrow is live. pager.visible would be this
   comparison with a longer name, as #8 argued.
3. m.scope == "day" / == "all" — layout: which of two buttons is tinted,
   against the constant that button names.
4. m.filter ~= "" — layout: whether the chip node exists.
5. subjectW = LEFT_W - ACTION_W - 24 — layout, and the collapse is safe. Main
   spelled it at :1251 as a local and again inline at :1274; both are the
   same token sequence, and the -24 -> -25 mutant proves both consumption sites
   move together — main printed width=120 on the input and on the card's
   inner column in the same state while the branch printed 119 on both.
6. "tag-" .. m.draftKey, "search-" .. m.searchKey, tostring(slot.page) —
   widget identity and a number-to-label.

The audit undercounts, though the character claim survives every one. Five more
expressions live in builders and none are in the list: if #out == 0 in
buildRecent:1202, buildChart:1263 and buildList:1403 (the three empty-state
placeholders, all three present on main), RIGHT_W - 20 at buildSearch:1280,
BAR_H - h at buildChart:1243, filter == "" at buildTrend:1212 (the same
class as #4, at a site the list does not mention) and row.tooltipTag == nil at
buildList:1391. All layout. Call it eleven, not six.

scopeButton captures nothing. Three parameters, and CHIP_H — a module
constant — is the only other name in its body. Nothing it used to read from
render's scope.

focusOnce is wave 5's, still, and not newly broken. render still clears
it at :1529 exactly as main:1472 did. My onOpen paint, focus request pending state paints focus=boolean:true on both sides and every later state
paints false on both, so the flag's whole lifecycle is byte-identical.

windowRows and the rejected alternative. The rejection reason is the real
one: the model's row tables (:985-1002) carry
when/tagText/tagIsKey/duration/dotColour/tooltipTag/isFiltered/handler and no
tag, so publishing m.rows would have forced filterFromRow to compare a
different field. And filterFromRow is cmp-identical to main's six lines, as
are barPeak and gotoPage. The field is also the cheaper of the two: it is a
reference to a table the model already built, where putting tag back on each
display row would be eight more hash slots per repaint.

Ten builders, and every one earns its name. I went looking for the fold and
did not find it.

• display, 3 lines, 3 call sites — replaces main's tagLabel (3 lines, 2
  sites) plus one inline branch in render. Fewer spellings of the contract, not
  more, and luau-analyze agrees: one fewer noctalia read.
• scopeButton, 10 lines, 2 call sites — two seven-prop buttons that differ in
  three values. At module level it costs no closure; inside buildScope it would
  cost one per repaint.
• buildTrend, 10 lines, 1 call site, 1 node — the weakest of the ten, and it
  still survives. Folding it into buildChart changes the tree, which is
  disqualifying; inlining it into render puts the filter == "" key choice
  back into the function whose whole claim is that it chooses nothing. It stays
  because the alternative breaks the rule this PR is enforcing.

Ten names for seven regions is the right count when one of the three extras is a
shared leaf helper, one is a shared sub-builder and one is a region that happens
to be a single node.

Tests, adapted honestly. Nothing reached a deleted local: tagLabel,
recentButtons, dayColumns and activityRows were unexported module locals
and main's export table is only Fmt/Parse/Query/Timew/viewModel, so no assert
could ever have spelled them; the two hits in the branch test (:1262, :1310) are
both comments.
Zero asserts modified, zero deleted: grep -c "^-" over the test half of the diff
is 9 and every one of those lines is a -- comment, grep -cE "^-[[:space:]]*(assert\(|same\()" is 0.
Thirteen new assert/same statements, and 174 added lines. Four killed in my own
sandbox copy of the worktree:

render calls viewModel() twice            -> test.luau:1303
onPage1 freezes on its slot number        -> test.luau:1348
gotoStripPage drops the base              -> test.luau:1348
render stops publishing shown             -> test.luau:684

The restore canary at :1298 is real, not the vacuous first version: :1270
takes the function object before the wrap, so a surviving wrapper is a different
object. The compare() windowRows block is a self-consistency tripwire rather
than a publication check — rows is derived from windowRows inside the same
call — but the behavioural weight sits correctly on :1326/:1328, which is
where my dropped-publication mutant died.

Gates, run from .worktrees/t5b-render.

$ luau test.luau
viewModel: 14 states, 1485 field comparisons
ok

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

$ awk '/getfenv|setfenv/{n++} END{print n+0}' timew/panel.luau timew/bar.luau
0

$ luau-analyze timew/panel.luau      # against the same run on main:timew/panel.luau
main 131 diagnostics, branch 130
< ./timew/panel.luau: TypeError: Unknown global 'noctalia'; consider assigning to it first

One fewer noctalia read and nothing else changed — your 39 -> 38, confirmed in
kind. Everything the diff touches outside the builder region is comment prose,
the local viewModel forward declaration, the pageBase deletion, the two
deleted builders and the windowRows field; I read all thirteen of those hunks.

Both worktrees end git status --porcelain empty. Every mutation ran in
/tmp/pr9, each restored cmp-clean against its pre-mutation file.

Not checked

• No live shell. Every claim above is harness-side. The tree is identical in
  32 states, so anything that still differs is a shell-side layout effect, and
  only a screenshot can see it.
• en.json only. One locale. A translation whose substituted length pushes
  the strip off a RIGHT_W row is invisible here.
• PR #8's field values. I compared trees, not model fields; waves 1-4 own
  those and I did not re-derive them.
• Async interleavings beyond the ones test.luau and my harness drive. A
  walk landing between two other callbacks is not enumerated.
• Timew-routed re-derivation. Finding 1's hole has a sibling: Timew is
  deliberately outside the counter, so a builder re-deriving isFiltered through
  Timew.tagMatches is caught only if it changes a value. I did not chase
  whether that is worth closing too.

Screenshots worth taking

The tree is byte-identical in all of these, so a difference is shell-side.

1. Idle, freshly opened — --:--, focus in the tag field, -- on the scope
   button, both empty-state placeholders.
2. Tracking a multi-word tag — subject as a label at SUBJECT_H, the chip
   row at its four-tag cap, the stop glyph.
3. Day scope, filtered by a tag — the chip in the header row, tinted row
   buttons, the trend caption in its _tag form.
4. All-time, filtered — the lead column's dates, a dimmer bar on a day
   carrying the tag beside a dark zero day.
5. All-time, page 4 of 6 — strip reading 2 3 4 5 6 with 4 selected, both
   arrows live, the range label. Then click 2 and confirm it lands on page
   2. This is the pageBase change and no still image proves it.
6. The stale-strip path — from state 5, run
   noctalia msg plugin yuris/timew:panel all day 99, then click the button
   still reading 2. Expect the day's rows, not Nothing tracked. This is the
   one behaviour that differs from main, and the only screenshot that shows the
   improvement.
7. A day with an untagged row — the translated (untagged) with no tooltip
   on it, and a tooltip on the row below.
8. A day with no intervals — dated scope button, empty-list placeholder,
   chart still drawn above.

**Verdict: APPROVE** Reviewed by rebuilding the acceptance check rather than reading yours: a second tree-identity harness, my own core-call instrumentation, my own timings, and mutation against a throwaway copy of the worktree. The tree holds. One test-side hole to close, two notes. ## Findings ### 1. `barPeak` is invisible to the one-model-per-repaint parity — P2 `timew/panel.luau:929`, `timew/panel.luau:1008`, guarded at `test.luau:1303`. The parity counts calls into `Fmt`/`Parse`/`Query`. `barPeak` is a module local whose body reaches none of them: ``` $ sed -n '929,937p' timew/panel.luau local function barPeak() local peak = 1 for _, day in ipairs(days) do if day.seconds > peak then peak = day.seconds end end return peak end ``` So a builder that re-derives the peak adds zero counted calls. The comment directly above the parity names this exact duplication as what it is guarding against — `test.luau:1261-1262` "which is what main did twice -- barPeak again in dayColumns, Query.window again in activityRows" — and it cannot see it. Both spellings pass: ``` $ sed -i 's|^\tfor _, day in ipairs(chart) do$|\tpeak = barPeak()\n\tfor _, day in ipairs(chart) do|' timew/panel.luau $ luau test.luau viewModel: 14 states, 1485 field comparisons ok $ sed -i 's|^\tfor _, day in ipairs(chart) do$|\tfor _, d in ipairs(days) do if d.seconds > peak then peak = d.seconds end end\n\tfor _, day in ipairs(chart) do|' timew/panel.luau $ luau test.luau viewModel: 14 states, 1485 field comparisons ok ``` The tree-identity harness cannot see it either — it is a cost, not a value — so this mutant survives both gates: ``` $ luau treediff.luau # with buildChart re-deriving the peak states: 32 bytes: main 244419, branch 244419 node lines compared: 2372 per side differences: 0 tree-identical ``` `barPeak` is already a pure function of `days`; it only takes its argument implicitly. Route it through the table the counter watches and the existing assert covers it for free: ```suggestion function Query.barPeak(days) ``` with its one call site at `:1008`: ```suggestion local chartPeak = Query.barPeak(days) ``` Proven, on top of that change: the suite still passes clean, and the mutant that passed above now dies where it should. ``` $ luau test.luau # fix applied, no mutant viewModel: 14 states, 1485 field comparisons ok $ luau test.luau # fix applied, buildChart re-derives ./test.luau:1303: a repaint made 14 pure-core calls against the model's 13: render is not assembling one model, or a builder is re-deriving one ``` That moves `barPeak`'s declaration line, which #8 hash-pinned; the body is untouched. If you would rather not move it, the honest alternative is to narrow `test.luau:1261-1262` so it stops claiming to cover the `barPeak` half — but then the duplication your own comment names has no guard at all. I prefer the move. ### 2. `test.luau:1323` names the published slice and asserts the model's — P3 ``` $ sed -n '1322,1323p' test.luau local slice = panel.viewModel().windowRows assert(#slice == 2, "two intervals on the day, so two rows in the published slice") ``` The subject is `viewModel().windowRows`, not the module local `render` published into. The publication is genuinely pinned two lines down, by `onRow1`/`onRow2` landing on the right filter — I killed a dropped `shown, matching = …` and it died at `test.luau:684` — so nothing is weakened. Only the message overclaims. ```suggestion assert(#slice == 2, "two intervals on the day, so two rows in the model's slice") ``` ### 3. The repaint is measurably slower and heavier; the only stated perf claim is a saving — P3 `timew/panel.luau:1290`. The PR body claims `scopeButton` at module level "costs a once-a-second repaint one fewer closure". True, and it is one closure against the 35-table model that `render` now builds every repaint and `main` never built at all. Measured, both sides in one process, worst state the panel can reach (200 all-time rows, page 4 of 25), min of 9 interleaved rounds x 500: ``` main repaint 0.0482 ms branch repaint 0.0528 ms main viewModel() 0.0186 ms branch viewModel() 0.0184 ms repaint: branch - main = +0.0046 ms (+9.5%); 25 ms budget uses 0.19% -> 0.21% heap allocated by ONE call (min / median of 60, KiB) main repaint min 51.00 median 52.00 branch repaint min 58.00 median 58.00 ui nodes/props tables per repaint: main 104/104 branch 104/104 model tables the branch repaint adds: 35 ``` No builder allocates per row or per day where `main` did not — the node and prop counts are equal, which is the tree identity restated. The whole delta is the model, +7 KiB and +4.6 us, against a 25 ms budget. Nothing to fix in the code; the fix is to say so, since "one fewer closure" is the only number in the PR body and it points the wrong way. ## Verified **Tree identity — my own harness, 32 states, zero differences.** Both `git show main:timew/panel.luau` and this branch loaded in one `luau` process, each with its own global table, stubbed identically, driven through the same sequence, flattened one line per node with props key-sorted and a child count. `tr` resolves through a table `jq` generates from `timew/translations/en.json`, so a key no locale defines raises. ``` $ luau treediff.luau tree identity: main vs refactor/t5b-render states: 32 bytes: main 244419, branch 244419 node lines compared: 2372 per side differences: 0 tree-identical ``` The harness asserts its own two soundness preconditions: that the two modules do not share a global table (they would otherwise both write one `panelTree` and every comparison would be vacuous), and that no function value ever reaches a prop slot — the no-closures-in-callbacks rule, checked on every one of those 2372 nodes rather than argued. States: idle; a scope-click repaint; `onOpen`'s own paint with the focus request still pending; tracking a multi-word tag; a second-tick repaint while tracking; tracking untagged; day scope with rows; day scope filtered; day scope filtered by a tag no row carries; all-time; all-time filtered by a tag some days carry; the chip cleared back to an empty search; a day with an untagged row; pages 1-6 of a 6-page list; both ends reached through `<`/`>`; the five named page handlers each driven from a painted page 4 where the base is 2; a day with no intervals; the backwards range; the bar's published `elapsedText`; an untagged row in the all-time list; the search box mid-typing. The backwards range is reproduced, not fixed — `17-16 / 12` on both sides. Nine mutants against the branch copy, every one killed, every one restored `cmp`-clean: | mutant | differences | |---|--:| | `buildScope` row `gap = 4` -> `5` | 32 | | `buildList` `selected = row.isFiltered` -> `true` | 133 | | `onPage1()` -> `gotoPage(1)` | 12 | | `subjectW` `- 24` -> `- 25` | 64 | | `buildList` drops the row spacer | 873 | | `buildScope` drops its spacer | 1103 | | `math.max(2, ...)` -> `math.max(1, ...)` | 58 | | `buildPager` `p.current > 1` -> `true` | 2 | | `display` drops the `tr` | 31 | The `max(2 -> 1)` mutant is byte-count-identical on both sides (244419 each) and shows up only in the line diff, which is the case for not trusting a byte total. **One model per repaint — instrumented, not assumed.** Wrapping every `Fmt`/`Parse`/`Query` entry and counting a bare `update()` against one `viewModel()`, in the worst state: ``` branch function repaint model Fmt.clock 1 1 Fmt.dayLabel 1 1 Fmt.span 1 1 Query.barRoles 3 3 Query.pageCount 1 1 Query.pageOf 1 1 Query.pageStrip 1 1 Query.tagColor 8 8 Query.window 1 1 TOTAL 18 18 ``` `Query.window` is 1 per repaint and `viewModel` calls it exactly once, so `render` builds exactly one model and no builder re-derives — subject to finding 1, which is the one derivation this instrument cannot see. `main` scores 18/18 too, which is the like-for-like the timing above compares. **The `pageBase` collapse — the staleness question, ruled.** There is exactly one path in this module that moves pager state and returns without a repaint, and it is the one you named: `loadDay(index)` early-returns at `timew/panel.luau:674` when `days[index]` is nil, after having already written `listOffset`, `selected` and `mode`. Reachable as `noctalia msg plugin yuris/timew:panel all day 99`. Every other mutator renders. I drove it on both sides: ``` main painted strip [2->onPage1 3->onPage2 4->onPage3 5->onPage4 6->onPage5] range 25-32 / 200 main day 99 screen unchanged (no repaint) main onPage1() list "Nothing tracked" branch painted strip [2->onPage1 3->onPage2 4->onPage3 5->onPage4 6->onPage5] range 25-32 / 200 branch day 99 screen unchanged (no repaint) branch onPage1() list 17:48 | 12:00 | 09:00 ``` `main` reads the base its last paint wrote, 2, and pages an all-time base into a three-row day list: page 2, which is empty. The branch reads a base derived from the state the click actually finds, 1, and shows the day. **The branch is right**, and by a wider margin than the PR body claims — this is not "the fresher of two equally stale readings", it is the difference between an empty list and the correct one. The strip on screen is stale in every other respect too (the scope switch is lying as well), which is why the honest reading is the fresh one. The counter-argument that main honours the number the button showed does not survive the screenshot: the number the button showed belongs to a list that is no longer on screen. `main`'s second divergence window — `pageBase = firstShown` sits inside `if pages > 1` at `main:1394`, so a list that shrinks to one page leaves the module local stale — closes with it, and was unreachable anyway because a single-page list draws no strip to click. **The forward declaration is one binding, not two.** ``` main env.viewModel=nil env.render=nil env.onPage1=function module export present=true branch env.viewModel=nil env.render=nil env.onPage1=function module export present=true ``` `function viewModel()` at `:967` assigns the `local viewModel` at `:271`; nothing leaks to the module's global table, exactly as `render` has always behaved. And `onPage1()` in the probe above executed rather than raising, which is the direct proof that `gotoStripPage`'s upvalue is the binding the definition fills. **One spelling of `pageBase` left.** `local pageBase = 1` (`main:240`) is gone; the branch spells it in comments, the `viewModel` local at `:1036`, the model field at `:1092`, and the single read at `:877`. **The six layout claims, one at a time.** All six hold. 1. `math.max(2, math.floor(BAR_H * day.seconds / peak))` — layout, and character-identical to `main:1115`. `peak` cannot be zero on either side: `barPeak` seeds `peak = 1` and its body is `cmp`-identical to main's, and the `max(2, ...)` floors every bar at 2 px besides. 2. `p.pages <= 1`, `p.current > 1`, `p.current < p.pages` — layout: whether the row exists and whether each arrow is live. `pager.visible` would be this comparison with a longer name, as #8 argued. 3. `m.scope == "day"` / `== "all"` — layout: which of two buttons is tinted, against the constant that button names. 4. `m.filter ~= ""` — layout: whether the chip node exists. 5. `subjectW = LEFT_W - ACTION_W - 24` — layout, and the collapse is safe. Main spelled it at `:1251` as a local and again inline at `:1274`; both are the same token sequence, and the `-24 -> -25` mutant proves both consumption sites move together — main printed `width=120` on the input **and** on the card's inner column in the same state while the branch printed `119` on both. 6. `"tag-" .. m.draftKey`, `"search-" .. m.searchKey`, `tostring(slot.page)` — widget identity and a number-to-label. The audit undercounts, though the character claim survives every one. Five more expressions live in builders and none are in the list: `if #out == 0` in `buildRecent:1202`, `buildChart:1263` and `buildList:1403` (the three empty-state placeholders, all three present on main), `RIGHT_W - 20` at `buildSearch:1280`, `BAR_H - h` at `buildChart:1243`, `filter == ""` at `buildTrend:1212` (the same class as #4, at a site the list does not mention) and `row.tooltipTag == nil` at `buildList:1391`. All layout. Call it eleven, not six. **`scopeButton` captures nothing.** Three parameters, and `CHIP_H` — a module constant — is the only other name in its body. Nothing it used to read from `render`'s scope. **`focusOnce` is wave 5's, still, and not newly broken.** `render` still clears it at `:1529` exactly as `main:1472` did. My `onOpen paint, focus request pending` state paints `focus=boolean:true` on both sides and every later state paints `false` on both, so the flag's whole lifecycle is byte-identical. **`windowRows` and the rejected alternative.** The rejection reason is the real one: the model's row tables (`:985-1002`) carry `when/tagText/tagIsKey/duration/dotColour/tooltipTag/isFiltered/handler` and no `tag`, so publishing `m.rows` would have forced `filterFromRow` to compare a different field. And `filterFromRow` is `cmp`-identical to main's six lines, as are `barPeak` and `gotoPage`. The field is also the cheaper of the two: it is a reference to a table the model already built, where putting `tag` back on each display row would be eight more hash slots per repaint. **Ten builders, and every one earns its name.** I went looking for the fold and did not find it. - `display`, 3 lines, 3 call sites — replaces main's `tagLabel` (3 lines, 2 sites) plus one inline branch in `render`. Fewer spellings of the contract, not more, and `luau-analyze` agrees: one fewer `noctalia` read. - `scopeButton`, 10 lines, 2 call sites — two seven-prop buttons that differ in three values. At module level it costs no closure; inside `buildScope` it would cost one per repaint. - `buildTrend`, 10 lines, 1 call site, 1 node — the weakest of the ten, and it still survives. Folding it into `buildChart` changes the tree, which is disqualifying; inlining it into `render` puts the `filter == ""` key choice back into the function whose whole claim is that it chooses nothing. It stays because the alternative breaks the rule this PR is enforcing. Ten names for seven regions is the right count when one of the three extras is a shared leaf helper, one is a shared sub-builder and one is a region that happens to be a single node. **Tests, adapted honestly.** Nothing reached a deleted local: `tagLabel`, `recentButtons`, `dayColumns` and `activityRows` were unexported module locals and main's export table is only `Fmt/Parse/Query/Timew/viewModel`, so no assert could ever have spelled them; the two hits in the branch test (`:1262`, `:1310`) are both comments. Zero asserts modified, zero deleted: `grep -c "^-" ` over the test half of the diff is 9 and every one of those lines is a `--` comment, `grep -cE "^-[[:space:]]*(assert\(|same\()"` is 0. Thirteen new `assert`/`same` statements, and 174 added lines. Four killed in my own sandbox copy of the worktree: ``` render calls viewModel() twice -> test.luau:1303 onPage1 freezes on its slot number -> test.luau:1348 gotoStripPage drops the base -> test.luau:1348 render stops publishing shown -> test.luau:684 ``` The restore canary at `:1298` is real, not the vacuous first version: `:1270` takes the function object before the wrap, so a surviving wrapper is a different object. The `compare()` `windowRows` block is a self-consistency tripwire rather than a publication check — `rows` is derived from `windowRows` inside the same call — but the behavioural weight sits correctly on `:1326`/`:1328`, which is where my dropped-publication mutant died. **Gates, run from `.worktrees/t5b-render`.** ``` $ luau test.luau viewModel: 14 states, 1485 field comparisons ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ awk '/getfenv|setfenv/{n++} END{print n+0}' timew/panel.luau timew/bar.luau 0 $ luau-analyze timew/panel.luau # against the same run on main:timew/panel.luau main 131 diagnostics, branch 130 < ./timew/panel.luau: TypeError: Unknown global 'noctalia'; consider assigning to it first ``` One fewer `noctalia` read and nothing else changed — your 39 -> 38, confirmed in kind. Everything the diff touches outside the builder region is comment prose, the `local viewModel` forward declaration, the `pageBase` deletion, the two deleted builders and the `windowRows` field; I read all thirteen of those hunks. Both worktrees end `git status --porcelain` empty. Every mutation ran in `/tmp/pr9`, each restored `cmp`-clean against its pre-mutation file. ## Not checked - **No live shell.** Every claim above is harness-side. The tree is identical in 32 states, so anything that still differs is a shell-side layout effect, and only a screenshot can see it. - **`en.json` only.** One locale. A translation whose substituted length pushes the strip off a `RIGHT_W` row is invisible here. - **PR #8's field values.** I compared trees, not model fields; waves 1-4 own those and I did not re-derive them. - **Async interleavings** beyond the ones `test.luau` and my harness drive. A walk landing between two other callbacks is not enumerated. - **`Timew`-routed re-derivation.** Finding 1's hole has a sibling: `Timew` is deliberately outside the counter, so a builder re-deriving `isFiltered` through `Timew.tagMatches` is caught only if it changes a value. I did not chase whether that is worth closing too. ## Screenshots worth taking The tree is byte-identical in all of these, so a difference is shell-side. 1. **Idle, freshly opened** — `--:--`, focus in the tag field, `--` on the scope button, both empty-state placeholders. 2. **Tracking a multi-word tag** — subject as a label at `SUBJECT_H`, the chip row at its four-tag cap, the stop glyph. 3. **Day scope, filtered by a tag** — the chip in the header row, tinted row buttons, the trend caption in its `_tag` form. 4. **All-time, filtered** — the lead column's dates, a dimmer bar on a day carrying the tag beside a dark zero day. 5. **All-time, page 4 of 6** — strip reading `2 3 4 5 6` with 4 selected, both arrows live, the range label. **Then click `2` and confirm it lands on page 2.** This is the `pageBase` change and no still image proves it. 6. **The stale-strip path** — from state 5, run `noctalia msg plugin yuris/timew:panel all day 99`, then click the button still reading `2`. Expect the day's rows, not `Nothing tracked`. This is the one behaviour that differs from `main`, and the only screenshot that shows the improvement. 7. **A day with an untagged row** — the translated `(untagged)` with no tooltip on it, and a tooltip on the row below. 8. **A day with no intervals** — dated scope button, empty-list placeholder, chart still drawn above.

yuris referenced this pull request from a commit 2026-09-01 07:20:11 +00:00

test(panel): count barPeak in the one-model parity, and say what the slice assert checks

yuris added 1 commit 2026-09-01 07:20:11 +00:00

test(panel): count barPeak in the one-model parity, and say what the slice assert checks ... f29d78ff58

Review round 2 on #9.

barPeak was a module local reading module state, so it reached none of the
Fmt/Parse/Query entries the one-model-per-repaint parity wraps. A builder
re-deriving the chart peak therefore added no counted call and the assert still
passed -- while the comment above that assert claimed to guard exactly this
duplication ("barPeak again in dayColumns"). It is now Query.barPeak(days),
which the counter watches: body byte-identical to main's, declaration line moved
up into the pure core, where `days` is not yet in scope to be read any other
way, so the argument cannot rot back into a module-state read. Its floor of 1 --
the reason an all-idle fortnight does not divide by zero -- gets the unit half
every other Query entry already had.

test.luau:1331 asserted on viewModel().windowRows while its message called it
the published slice. The module local `render` publishes into has no export and
cannot be read from the test; the publication is pinned by the two clicks below,
so the message now names what the line actually checks.

Tree identity unchanged: 32 states, 2372 node lines per side, 0 differences.

yuris commented 2026-09-01 07:21:32 +00:00

Author

Owner

Copy link

Both findings applied in f29d78f, third one answered in the PR body. Every
mutant below ran in a copy of the worktree at /tmp/pr9fx, restored cmp-clean
after each.

1. barPeak is invisible to the parity — applied, and further than suggested

Taken, with one change to the placement: barPeak went into the pure core
beside the other Query entries rather than staying at :929 under a new name.
The reason is that the argument has to be load-bearing. Spelled
function Query.barPeak(days) at its old site, the parameter shadows the module
local days twelve lines above it, so deleting the parameter leaves a function
that still works and is silently invisible to the counter again. In the pure core
days is not declared until 10 lines below Query.tagColor, so the argument is
the only way the list can arrive — the guarantee is scope order, not discipline.
The body is main's byte for byte:

$ git show HEAD:timew/panel.luau | sed -n '930,937p' > before.txt
$ sed -n '235,242p' timew/panel.luau > after.txt
$ cmp before.txt after.txt && echo identical
identical

The hole, before the change — your peak = barPeak() spelling in
buildChart, on ee2291f:

$ luau test.luau
viewModel: 14 states, 1485 field comparisons
ok
exit=0

The same duplication, after — spelled peak = Query.barPeak(days), since
the local no longer exists to be called. The counter sees it:

$ luau test.luau
./test.luau:1315: a repaint made 14 pure-core calls against the model's 13: render is not assembling one model, or a builder is re-deriving one
exit=1

And clean, unmutated: viewModel: 14 states, 1485 field comparisons / ok.
cost3.luau reads the change directly — the per-repaint column is 19/19 now,
with Query.barPeak 1 1 in it where the row did not exist before.

One limit, stated rather than left to be found. Your second spelling — the
peak loop hand-inlined into buildChart instead of called — still survives:

$ sed -i 's|for _, day in ipairs(chart) do|for _, d in ipairs(days) do if d.seconds > peak then peak = d.seconds end end\n\tfor _, day in ipairs(chart) do|' timew/panel.luau
$ luau test.luau
ok
exit=0

A call counter cannot see code that makes no call, and no rearranging of
barPeak fixes that. What the move does close is the reachable-by-accident
spelling: a builder that calls the helper that already exists. The
copy-the-loop-by-hand spelling is now the only one, and it has to survive code
review rather than a sed.

Unit half. barPeak is exported now, and it was the only Query entry with
no unit assert, so it got the three every sibling has — the floor of 1 is the
whole reason the chart's divisor is never zero, and that was untestable while it
was a module local. All three proven live, each mutant restored cmp-clean:

mutant in Query.barPeak	failure
local peak = 1 → 0	./test.luau:164: an all-idle fortnight floors at 1 rather than dividing by 0, exit 1
day.seconds > peak → <	./test.luau:163: the longest day is the peak, exit 1
return peak → if next(days) == nil then 0 else peak	./test.luau:165: no days at all still floors at 1, exit 1

Tree identity, re-run after the move — your harness, unmodified, with
pbranch.luau refreshed from the committed file:

$ luau treediff.luau
states: 32
bytes: main 244419, branch 244419
node lines compared: 2372 per side
differences: 0
tree-identical

Two comments moved with it. viewModel's placement note no longer claims it
sits below barPeak because a Luau local is only in scope after its definition —
that reason is gone, and the forward declaration for the page handlers is the
only one left. And the parity's own comment now says why barPeak lives in
Query and that it must stay there, since the next reader's instinct is to put
a days-shaped helper back beside days.

2. :1331 named the published slice — applied

Reworded, and pointed at where the publication actually is checked:

assert(#slice == 2, "two intervals on the day, so two rows in the model's slice; the publication of that slice is what onRow1 and onRow2 below read")

No behavioural change, so nothing to mutate here; the publication mutant still
dies where you found it, at :692 after the shift:

$ sed -i 's/^\tshown, matching = m.windowRows, m.pager.total$/\t-- MUTANT/' timew/panel.luau
$ luau test.luau
./test.luau:692: the control row does filter, so the refusal above is not a dead click path
exit=1

3. The perf claim — corrected, and it was pointing the wrong way

You are right that "one fewer closure" was the only number in the body and that
it flattered the change. The body now carries a What the split costs section
with the loss in it, and the closure bullet says the saving is not worth quoting.
Re-measured with your cost3.luau after the barPeak move, five runs:

A repaint costs 0.051–0.054 ms on this branch against 0.048–0.051 ms on
main — +0.003 ms and +6 KiB of heap per repaint, +4–8%, which moves the 25 ms
per-handler budget from 0.19% used to 0.21%. The five deltas were +4.2, +6.0,
+6.0, +6.3, +7.8% — same order as your +9.5%, and the noise between runs is
wider than the gap. Node and prop tables stay 104/104 on both sides, so nothing
allocates per row or per day; the whole delta is the model's 35 tables. A page
click pays one extra viewModel() on top: 0.049 → 0.070 ms of 25 ms.

Stated in the body as the trade it is: 256 lines of derive-as-you-paint down to
46 lines of assembly, for 0.02% of the budget.

Also corrected in the body while I was in it: panel.luau 1547 → 1607 and
test.luau 1234 → 1372 (the three unit asserts), and the test.luau:NNNN
references in the three mutation tables. Those were stale beyond my shift —
:1221, :859, :1239 and :1341 were not those asserts on ee2291f either;
they date from a revision about a hundred lines shorter. They now point at the
asserts their own quoted messages name, on the merged tree: :1310, :1314,
:1315, :974, :1340, :1358, :1362.

Gate, from .worktrees/t5b-render on f29d78f

$ luau test.luau
viewModel: 14 states, 1485 field comparisons
ok

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

$ awk '/getfenv|setfenv/{n++} END{print n+0}' timew/*.luau
0

$ luau-analyze timew/panel.luau | grep -c .      # against HEAD~1's 130
130

$ git diff --stat main...HEAD
 test.luau        | 144 +++-
 timew/panel.luau | 741 +++++++++++++++++++-----------------

$ git status --porcelain

Not checked, still: anything shell-side. Your eight screenshots — 5 and 6 in
particular, the pageBase click and the stale-strip path — are the only thing
that can see it, and they are next.

Merging.

Both findings applied in `f29d78f`, third one answered in the PR body. Every mutant below ran in a copy of the worktree at `/tmp/pr9fx`, restored `cmp`-clean after each. ## 1. `barPeak` is invisible to the parity — applied, and further than suggested Taken, with one change to the placement: `barPeak` went into the pure core beside the other `Query` entries rather than staying at `:929` under a new name. The reason is that the argument has to be load-bearing. Spelled `function Query.barPeak(days)` at its old site, the parameter shadows the module local `days` twelve lines above it, so deleting the parameter leaves a function that still works and is silently invisible to the counter again. In the pure core `days` is not declared until 10 lines below `Query.tagColor`, so the argument is the only way the list can arrive — the guarantee is scope order, not discipline. The body is `main`'s byte for byte: ``` $ git show HEAD:timew/panel.luau | sed -n '930,937p' > before.txt $ sed -n '235,242p' timew/panel.luau > after.txt $ cmp before.txt after.txt && echo identical identical ``` **The hole, before the change** — your `peak = barPeak()` spelling in `buildChart`, on `ee2291f`: ``` $ luau test.luau viewModel: 14 states, 1485 field comparisons ok exit=0 ``` **The same duplication, after** — spelled `peak = Query.barPeak(days)`, since the local no longer exists to be called. The counter sees it: ``` $ luau test.luau ./test.luau:1315: a repaint made 14 pure-core calls against the model's 13: render is not assembling one model, or a builder is re-deriving one exit=1 ``` And clean, unmutated: `viewModel: 14 states, 1485 field comparisons` / `ok`. `cost3.luau` reads the change directly — the per-repaint column is 19/19 now, with `Query.barPeak 1 1` in it where the row did not exist before. **One limit, stated rather than left to be found.** Your second spelling — the peak loop hand-inlined into `buildChart` instead of called — still survives: ``` $ sed -i 's|for _, day in ipairs(chart) do|for _, d in ipairs(days) do if d.seconds > peak then peak = d.seconds end end\n\tfor _, day in ipairs(chart) do|' timew/panel.luau $ luau test.luau ok exit=0 ``` A call counter cannot see code that makes no call, and no rearranging of `barPeak` fixes that. What the move does close is the reachable-by-accident spelling: a builder that calls the helper that already exists. The copy-the-loop-by-hand spelling is now the only one, and it has to survive code review rather than a `sed`. **Unit half.** `barPeak` is exported now, and it was the only `Query` entry with no unit assert, so it got the three every sibling has — the floor of 1 is the whole reason the chart's divisor is never zero, and that was untestable while it was a module local. All three proven live, each mutant restored `cmp`-clean: | mutant in `Query.barPeak` | failure | |---|---| | `local peak = 1` → `0` | `./test.luau:164: an all-idle fortnight floors at 1 rather than dividing by 0`, exit 1 | | `day.seconds > peak` → `<` | `./test.luau:163: the longest day is the peak`, exit 1 | | `return peak` → `if next(days) == nil then 0 else peak` | `./test.luau:165: no days at all still floors at 1`, exit 1 | **Tree identity, re-run after the move** — your harness, unmodified, with `pbranch.luau` refreshed from the committed file: ``` $ luau treediff.luau states: 32 bytes: main 244419, branch 244419 node lines compared: 2372 per side differences: 0 tree-identical ``` Two comments moved with it. `viewModel`'s placement note no longer claims it sits below `barPeak` because a Luau local is only in scope after its definition — that reason is gone, and the forward declaration for the page handlers is the only one left. And the parity's own comment now says why `barPeak` lives in `Query` and that it must stay there, since the next reader's instinct is to put a `days`-shaped helper back beside `days`. ## 2. `:1331` named the published slice — applied Reworded, and pointed at where the publication actually is checked: ```lua assert(#slice == 2, "two intervals on the day, so two rows in the model's slice; the publication of that slice is what onRow1 and onRow2 below read") ``` No behavioural change, so nothing to mutate here; the publication mutant still dies where you found it, at `:692` after the shift: ``` $ sed -i 's/^\tshown, matching = m.windowRows, m.pager.total$/\t-- MUTANT/' timew/panel.luau $ luau test.luau ./test.luau:692: the control row does filter, so the refusal above is not a dead click path exit=1 ``` ## 3. The perf claim — corrected, and it was pointing the wrong way You are right that "one fewer closure" was the only number in the body and that it flattered the change. The body now carries a *What the split costs* section with the loss in it, and the closure bullet says the saving is not worth quoting. Re-measured with your `cost3.luau` after the `barPeak` move, five runs: **A repaint costs 0.051–0.054 ms on this branch against 0.048–0.051 ms on `main` — +0.003 ms and +6 KiB of heap per repaint, +4–8%, which moves the 25 ms per-handler budget from 0.19% used to 0.21%.** The five deltas were +4.2, +6.0, +6.0, +6.3, +7.8% — same order as your +9.5%, and the noise between runs is wider than the gap. Node and prop tables stay 104/104 on both sides, so nothing allocates per row or per day; the whole delta is the model's 35 tables. A page click pays one extra `viewModel()` on top: 0.049 → 0.070 ms of 25 ms. Stated in the body as the trade it is: 256 lines of derive-as-you-paint down to 46 lines of assembly, for 0.02% of the budget. Also corrected in the body while I was in it: `panel.luau` 1547 → **1607** and `test.luau` 1234 → **1372** (the three unit asserts), and the `test.luau:NNNN` references in the three mutation tables. Those were stale beyond my shift — `:1221`, `:859`, `:1239` and `:1341` were not those asserts on `ee2291f` either; they date from a revision about a hundred lines shorter. They now point at the asserts their own quoted messages name, on the merged tree: `:1310`, `:1314`, `:1315`, `:974`, `:1340`, `:1358`, `:1362`. ## Gate, from `.worktrees/t5b-render` on `f29d78f` ``` $ luau test.luau viewModel: 14 states, 1485 field comparisons ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ awk '/getfenv|setfenv/{n++} END{print n+0}' timew/*.luau 0 $ luau-analyze timew/panel.luau | grep -c . # against HEAD~1's 130 130 $ git diff --stat main...HEAD test.luau | 144 +++- timew/panel.luau | 741 +++++++++++++++++++----------------- $ git status --porcelain ``` Not checked, still: anything shell-side. Your eight screenshots — 5 and 6 in particular, the `pageBase` click and the stale-strip path — are the only thing that can see it, and they are next. Merging.

yuris merged commit 6f94c8e120 into main 2026-09-01 07:21:41 +00:00

yuris deleted branch refactor/t5b-render 2026-09-01 07:21:41 +00:00

yuris referenced this pull request from a commit 2026-09-01 07:21:42 +00:00

refactor(panel): split render into region builders reading the model (#9)

yuris referenced this pull request 2026-09-01 07:47:30 +00:00

refactor(panel): collapse the module state into one table behind a single commit #10

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

yuris/noctalia-timew!9

No description provided.