yuris/noctalia-timew
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(panel): add a pure viewModel beside render #8

Merged
yuris merged 2 commits from refactor/t5a-viewmodel into main 2026-09-01 06:26:12 +00:00
Conversation 2 Commits 2 Files changed 2 +682 -1
yuris commented 2026-09-01 05:17:34 +00:00
Owner
Copy link

Wave 4, first of two. render() is 256 lines doing three jobs: deriving display
values from module state, deciding layout, and plumbing props. This PR extracts
the first job only, into a pure viewModel(), and proves it agrees with what
render() computes today. It does not change render() — that swap is the
next PR, deliberately separated because it is where layout regressions hide.

Additive proof

$ git diff --numstat main
488     0       test.luau
155     0       timew/panel.luau

$ git diff -U0 main -- timew/panel.luau | grep '^@@'
@@ -946,0 +947,154 @@ end          # the new section
@@ -1390,0 +1545 @@ return {         # the export line

$ git diff main -- timew/panel.luau | grep -c '^-[^-]'
0

Two hunks, both pure insertions, zero deletions. Function bodies extracted from
main and from this branch and compared byte for byte:

function lines sha256[..16]
render() 256 cb49497f54832bec identical
dayColumns() 40 e5db7af47407e3a9 identical
activityRows() 62 b7424253c42d3969 identical
recentButtons() 20 2e0e8629102155c8 identical
barPeak() 9 6604fef8dddc0f8d identical
tagLabel() 3 10af6912f94b5536 identical
passesFilter() 3 d5a548161463d92c identical
onOpen() 14 816c6d73ac83970a identical
onIpc() 30 9c6e7fd75a962c0e identical
loadDay() 23 5402ea6eaae8a4af identical
setFilter() 13 cf121eac21a975df identical
gotoPage() 6 c46d111e90c78d7e identical
filterFromRow() 6 8c7079f31c01003f identical
refresh() 11 81f79932057a0b99 identical

Stronger still: delete the two inserted hunks from this branch's panel.luau
and the result is main's byte for byte — both hash 1a14f5964482f6d1.

Where it sits, and why not higher

The brief said "after the pure core, before the UI builders". It is one slot
lower than that: immediately after barPeak, before dayColumns. barPeak is
a state derivation the model owns too, a Luau local is only in scope after its
definition, and moving barPeak up would have made this a non-additive diff.
Copying its six-line loop to satisfy a section boundary would have left two
spellings of it. So recentButtons and barPeak sit above the new section;
dayColumns, activityRows and render — the three builders that build what
the model describes — sit below. Challenge this if you disagree; the
alternative is moving barPeak, which costs the additive proof above.

Field inventory

Every field, and what it replaces in render.

Clock / subject card

field replaces
isActive active, which picks the clock colour, the glyph, the variant, the tooltip and the handler (1124, 1130-1136)
clockText the if not active / elapsedText / Fmt.clock(os.time() - startEpoch) chain (1089-1092)
subjectText + subjectIsKey tagLabel(tags) (1101) — key, not translated text
todayTotal total, the value panel.today substitutes (1127)
draft, draftKey, focusOnce the idle branch's value, key = "tag-"..draftKey, focus (1104-1110)

Search and filter

field replaces
search, searchKey the search input's value and key = "search-"..searchKey (1221-1222)
filter the chip's presence and text (1154-1156) and the trend caption's branch (1206-1208)

Scope switch

field replaces
scope mode, which tints one of the two scope buttons (1180)
dayTitle + dayTitleIsKey Fmt.dayLabel(days[selected], selected) and the tr-or-not on 1069
dayTotalText if day then Fmt.span(day.seconds) else "0:00" (1070)
allTotalText allTotal
scopeTotalText the header total's pick, if mode == "all" then allTotal else dayTotal (1195)

Chart — days[], oldest first (render order), plus chartPeak

field replaces
chartPeak barPeak() (957)
seconds day.seconds, the numerator of math.floor(BAR_H * day.seconds / peak) (960)
fill, letterColor Query.barRoles(mode, filter, day.seconds, isSelected) (963)
isSelected i == selected and mode == "day" (961)
letter string.sub(day.dow, 1, 1) (983)
handler "onDay" .. i (972, 979)

seconds + chartPeak, not a pixel height: BAR_H is layout and stays in
render.

List — rows[], the Query.window slice

field replaces
when if mode == "all" then row.date .. " " .. row.at else row.at (1012)
tagText + tagIsKey tagLabel(row.tag) (1031)
duration row.duration (1053)
dotColour Query.tagColor(row.tag, TAG_COLORS) (1025)
tooltipTag the tag panel.filter_by substitutes; nil on the untagged row (1047)
isFilterTarget filterFromRow's row.tag ~= UNTAGGED guard (802)
isFiltered the tag button's selected (1038)
handler "onRow" .. i (1026, 1048)

isFilterTarget and isFiltered are two different questions and both are in
the model: an untagged row is neither; a tagged row is always a target and is
tinted only while it matches the filter.

Pager

field replaces
pages Query.pageCount(matching, ROWS_MAX) (1238); pages > 1 is the row's visibility
current Query.pageOf(listOffset, ROWS_MAX) (1240)
pageBase, strip[] Query.pageStrip(pages, current, PAGE_BUTTONS) and the for p = firstShown, lastShown loop (1251-1275); each entry is { page, isCurrent, handler }
firstShown, lastShown, total panel.page_range's three numbers (1293-1295)

Chips

recent[] = { tag, handler } — the loop in recentButtons (916-927). The tag
is both the button text and what panel.start_tag substitutes.

Deliberate omissions

Named so a reviewer can challenge each one rather than hunt for it:

  • chipText, and the whole panel.filter_chip / panel.trend_tag /
    panel.today / panel.filter_by / panel.start_tag / panel.page_range
    family.     These keys take substitutions, so an isKey flag buys nothing — the
    builder would need an args table too. The model carries the value (filter,
    todayTotal, tooltipTag, a chip's tag, the three pager numbers) and the
    builder keeps the literal key. Fmt.dayLabel's (value, isKey) contract is
    used only where it fits: substitution-free keys.
  • Constant keys — 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, and the ‹/›
    literals. Nothing about them varies with state. The empty states are the
    builder reading an empty days / rows / recent list.
  • pager.visible. It is pages > 1, one comparison in the builder.
  • Layout — BAR_H, RIGHT_W, LEAD_W, DUR_W, ROW_H, CHIP_H, gaps,
    alignment, which ui.* node carries a value. Not derived from state.

Renamed from the brief's sketch: tooltip → tooltipTag (it is the tag, not a
sentence), and todayText → todayTotal for the same reason.

Equivalence evidence

render() is untouched, so there is no second rendering to diff. Instead:
drive the module into a state, take viewModel(), read the tree render()
built from that same state, and compare field by field, resolving keys through
timew/translations/en.json as a table.

$ nix shell nixpkgs#luau -c luau test.luau
viewModel: 13 states, 1332 field comparisons
ok

13 states, 1332 field comparisons, 0 mismatches. The states:

  1. idle, nothing fetched — every empty state, both literal placeholders
  2. tracking a multi-word tag, chip row over its RECENT_MAX cap
  3. tracking with no tag — the only state where subjectIsKey is true
  4. day scope with rows, unfiltered
  5. day scope filtered by a tag — the only state where the model's window runs a predicate
  6. all-time scope
  7. all-time, filtered by a tag some days carry and others do not
  8. a day with an untagged row
  9. all-time, page 1 of 6
  10. all-time, a middle page (strip slid off 1)
  11. all-time, the last page (short remainder)
  12. a day with no intervals
  13. listOffset past the match count — the backwards pager range

Two fields are not on the tree, and both are accounted for, not waved past:

  • rows[i].isFilterTarget has no prop. It is filterFromRow's UNTAGGED
    guard, and what the panel shows of it is the tooltip's presence. Pinned
    against the tooltip in every state, and against the sentinel directly in state
    8. Not a bug and not layout — a real derivation whose only visible
    consequence is the tooltip.
  • days[i].isSelected has no prop either: it is one of Query.barRoles' four
    inputs, and the two roles it decides (fill, letterColor) are both on the
    tree. Asserted as barRoles(scope, filter, seconds, isSelected) against the
    model's own pair, and both hasTag branches are exercised (states 5 and 7).

allTotalText and dayTotalText are each on the tree only in the scope that
shows them; the pick is asserted every state and both branches are taken.

The deferred pager bug is reproduced, not fixed

gotoPage clamps against the matching of the last render. State 13 pages to
the end of a 20-row day, then lets a background refresh reload the same day with
12 rows — loadDay rewinds the pager only on a real day change, so listOffset
stays at 16. The model reports what render draws:

assert(past.pager.firstShown == 17 and past.pager.lastShown == 16)
assert(#past.rows == 0 and past.pager.total == 12)   -- "17-16 / 12"
assert(past.pager.current == 3 and past.pager.pages == 2)

Clamping in the model would have hidden it from the change that fixes it.

Contract asserts, and their mutation proof

New asserts beyond the field comparison:

  • a host trap — noctalia, ui and panel each swapped for a table that
    raises on any read for the duration of one viewModel() call, so tr,
    runAsync, state.set and ui.* are all covered by one thing. The trap is
    itself asserted to be able to fail.
  • idempotence over five calls, and a render either side of them that must not
    move — a mutation to any module local shows up as a different tree.
  • type(handler) == "string" on every row, day, page and chip.
  • the untagged row: key not sentinel, isKey true, tooltip == nil,
    isFilterTarget == false, with the tagged row beside it as the control.
  • pager.pages/current/pageBase/#strip against Query.pageCount,
    Query.pageOf and Query.pageStrip on the same inputs.
  • shell-side: grep -c 'getfenv\|setfenv' timew/panel.luau → 0, and the
    viewModel body greps clean of noctalia., ui., runAsync.

Mutation battery: 61 mutations run. First pass of 25 found 23 live and 2
dead. Declaring both, and how they were closed:

dead assert why it was dead fix
subjectIsKey = false passed no state tracked an untagged interval, so the flag was only ever asserted on its false branch new state 3, "tracking with no tag"
chartPeak = 3600 passed the fixture's tallest day happened to be exactly 1:00:00 fixture changed to 2:31:07 so no plausible constant matches the peak

Both were missing states, not bendable asserts — a state the harness never
reaches is the same blind spot as a missing assert, so the state list grew from
11 to 13 (the second gap also motivated state 5, day-scope-with-a-filter, which
turned out to be the only state that makes the model's window predicate live).

Second pass of 51 mutations plus 10 follow-ups: 61 live, 0 dead. A sample:

LIVE  untagged row gains a tooltip
      a day with an untagged row: rows[1].tooltipTag -- nil vs "Filter by (untagged)"
LIVE  viewModel translates a key
      viewModel must not reach the host: viewModel reached noctalia.tr; it is state in, plain data out
LIVE  viewModel mutates module state
      viewModel is not idempotent, so it is carrying state between calls
LIVE  window loses its predicate
      day scope, filtered by a tag: one row per modelled row -- 2 vs 3
LIVE  chart peak guessed instead of measured
      tracking a multi-word tag: days[1].seconds over chartPeak -- 96 vs 241
LIVE  isSelected ignores the scope
      all-time scope: days[3].fill -- "surface_variant" vs "primary"
LIVE  backwards range nudged forward only when it is backwards
      listOffset past the match count: pager.firstShown/lastShown/total -- "17-16 / 12" vs "17-17 / 12"

Each mutation was applied to a copy of the file, run, then restored from the
in-memory original and re-run; the suite is green after every restore and the
final file hashes identically to the pre-mutation source.

Gate

$ nix shell nixpkgs#luau -c luau test.luau
viewModel: 13 states, 1332 field comparisons
ok

$ noctalia plugins lint timew
yuris/timew
  ok
0 errors, 0 warnings

$ git diff --stat main...HEAD
 test.luau        | 488 +++++++++++++++++++++++++++++++++++++
 timew/panel.luau | 155 +++++++++++
 2 files changed, 643 insertions(+)

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

What a reviewer should challenge

  1. The section's position — below barPeak rather than above
    recentButtons. Reasoned above; the alternative costs the additive proof.
  2. en.json copied into test.luau. The luau CLI ships no io library
    (probed: type(io) is nil), so the file cannot be read from the suite.
    The copy is kept whole rather than trimmed to the keys used so it diffs by
    eye, and tr raises on a missing key — which is what caught two mutations
    that emitted a raw tag where a key was expected. It can still drift from the
    shipped file; nothing here prevents that.
  3. DAYS, ROWS_MAX, PAGE_BUTTONS as literals in test.luau. They are
    module locals with no export. DAYS = 14 is already pinned by the seq 0 13
    command literal; ROWS_MAX and PAGE_BUTTONS are pinned by the pager
    asserts — a different ROWS_MAX gives a different page count for the same 45
    rows, and a different PAGE_BUTTONS a different strip width on a 6-page
    list. BAR_H is not mirrored: it is read back off the tree as
    padHeight + barHeight.
  4. focusOnce. onOpen sets it and its own render clears it, so the
    true case is not observable from outside onOpen — by the time the model
    can be called it is already false. The suite asserts exactly that
    asymmetry (onOpen's tree has focus == true, the model that follows says
    false) rather than pretending to verify the branch. That write inside
    render is wave 5's problem.
  5. clockText's elapsedText branch is untested. elapsedText is only set
    by a noctalia.state.watch callback and the suite's state.watch is a
    no-op, so the middle branch of the three never runs. The other two do.
  6. os.time() inside viewModel. It is a clock read, not a mutation and
    not a host call, and it is exactly what render does. The suite freezes it
    so the model and the render that follows cannot straddle a second.
  7. viewModel recomputes the window rather than reading shown/matching.
    It has to — writing them is activityRows' job and the model writes nothing.
    Same source, same predicate, same cap, and states 4-13 compare the two
    answers row for row.

Not merging; a reviewer comments first.

Wave 4, first of two. `render()` is 256 lines doing three jobs: deriving display values from module state, deciding layout, and plumbing props. This PR extracts the first job only, into a pure `viewModel()`, and proves it agrees with what `render()` computes today. It does **not** change `render()` — that swap is the next PR, deliberately separated because it is where layout regressions hide. ## Additive proof ``` $ git diff --numstat main 488 0 test.luau 155 0 timew/panel.luau $ git diff -U0 main -- timew/panel.luau | grep '^@@' @@ -946,0 +947,154 @@ end # the new section @@ -1390,0 +1545 @@ return { # the export line $ git diff main -- timew/panel.luau | grep -c '^-[^-]' 0 ``` Two hunks, both pure insertions, zero deletions. Function bodies extracted from `main` and from this branch and compared byte for byte: | function | lines | sha256[..16] | | |---|---|---|---| | `render()` | 256 | `cb49497f54832bec` | identical | | `dayColumns()` | 40 | `e5db7af47407e3a9` | identical | | `activityRows()` | 62 | `b7424253c42d3969` | identical | | `recentButtons()` | 20 | `2e0e8629102155c8` | identical | | `barPeak()` | 9 | `6604fef8dddc0f8d` | identical | | `tagLabel()` | 3 | `10af6912f94b5536` | identical | | `passesFilter()` | 3 | `d5a548161463d92c` | identical | | `onOpen()` | 14 | `816c6d73ac83970a` | identical | | `onIpc()` | 30 | `9c6e7fd75a962c0e` | identical | | `loadDay()` | 23 | `5402ea6eaae8a4af` | identical | | `setFilter()` | 13 | `cf121eac21a975df` | identical | | `gotoPage()` | 6 | `c46d111e90c78d7e` | identical | | `filterFromRow()` | 6 | `8c7079f31c01003f` | identical | | `refresh()` | 11 | `81f79932057a0b99` | identical | Stronger still: delete the two inserted hunks from this branch's `panel.luau` and the result is `main`'s byte for byte — both hash `1a14f5964482f6d1`. ## Where it sits, and why not higher The brief said "after the pure core, before the UI builders". It is one slot lower than that: immediately after `barPeak`, before `dayColumns`. `barPeak` is a state derivation the model owns too, a Luau local is only in scope after its definition, and moving `barPeak` up would have made this a non-additive diff. Copying its six-line loop to satisfy a section boundary would have left two spellings of it. So `recentButtons` and `barPeak` sit above the new section; `dayColumns`, `activityRows` and `render` — the three builders that build what the model describes — sit below. **Challenge this if you disagree**; the alternative is moving `barPeak`, which costs the additive proof above. ## Field inventory Every field, and what it replaces in `render`. ### Clock / subject card | field | replaces | |---|---| | `isActive` | `active`, which picks the clock colour, the glyph, the variant, the tooltip and the handler (1124, 1130-1136) | | `clockText` | the `if not active / elapsedText / Fmt.clock(os.time() - startEpoch)` chain (1089-1092) | | `subjectText` + `subjectIsKey` | `tagLabel(tags)` (1101) — key, not translated text | | `todayTotal` | `total`, the value `panel.today` substitutes (1127) | | `draft`, `draftKey`, `focusOnce` | the idle branch's `value`, `key = "tag-"..draftKey`, `focus` (1104-1110) | ### Search and filter | field | replaces | |---|---| | `search`, `searchKey` | the search input's `value` and `key = "search-"..searchKey` (1221-1222) | | `filter` | the chip's presence and text (1154-1156) and the trend caption's branch (1206-1208) | ### Scope switch | field | replaces | |---|---| | `scope` | `mode`, which tints one of the two scope buttons (1180) | | `dayTitle` + `dayTitleIsKey` | `Fmt.dayLabel(days[selected], selected)` and the `tr`-or-not on 1069 | | `dayTotalText` | `if day then Fmt.span(day.seconds) else "0:00"` (1070) | | `allTotalText` | `allTotal` | | `scopeTotalText` | the header total's pick, `if mode == "all" then allTotal else dayTotal` (1195) | ### Chart — `days[]`, oldest first (render order), plus `chartPeak` | field | replaces | |---|---| | `chartPeak` | `barPeak()` (957) | | `seconds` | `day.seconds`, the numerator of `math.floor(BAR_H * day.seconds / peak)` (960) | | `fill`, `letterColor` | `Query.barRoles(mode, filter, day.seconds, isSelected)` (963) | | `isSelected` | `i == selected and mode == "day"` (961) | | `letter` | `string.sub(day.dow, 1, 1)` (983) | | `handler` | `"onDay" .. i` (972, 979) | `seconds` + `chartPeak`, not a pixel height: `BAR_H` is layout and stays in `render`. ### List — `rows[]`, the `Query.window` slice | field | replaces | |---|---| | `when` | `if mode == "all" then row.date .. " " .. row.at else row.at` (1012) | | `tagText` + `tagIsKey` | `tagLabel(row.tag)` (1031) | | `duration` | `row.duration` (1053) | | `dotColour` | `Query.tagColor(row.tag, TAG_COLORS)` (1025) | | `tooltipTag` | the tag `panel.filter_by` substitutes; `nil` on the untagged row (1047) | | `isFilterTarget` | `filterFromRow`'s `row.tag ~= UNTAGGED` guard (802) | | `isFiltered` | the tag button's `selected` (1038) | | `handler` | `"onRow" .. i` (1026, 1048) | `isFilterTarget` and `isFiltered` are two different questions and both are in the model: an untagged row is neither; a tagged row is always a target and is tinted only while it matches the filter. ### Pager | field | replaces | |---|---| | `pages` | `Query.pageCount(matching, ROWS_MAX)` (1238); `pages > 1` is the row's visibility | | `current` | `Query.pageOf(listOffset, ROWS_MAX)` (1240) | | `pageBase`, `strip[]` | `Query.pageStrip(pages, current, PAGE_BUTTONS)` and the `for p = firstShown, lastShown` loop (1251-1275); each entry is `{ page, isCurrent, handler }` | | `firstShown`, `lastShown`, `total` | `panel.page_range`'s three numbers (1293-1295) | ### Chips `recent[] = { tag, handler }` — the loop in `recentButtons` (916-927). The tag is both the button text and what `panel.start_tag` substitutes. ## Deliberate omissions Named so a reviewer can challenge each one rather than hunt for it: - **`chipText`, and the whole `panel.filter_chip` / `panel.trend_tag` / `panel.today` / `panel.filter_by` / `panel.start_tag` / `panel.page_range` family.** These keys take substitutions, so an `isKey` flag buys nothing — the builder would need an args table too. The model carries the *value* (`filter`, `todayTotal`, `tooltipTag`, a chip's `tag`, the three pager numbers) and the builder keeps the literal key. `Fmt.dayLabel`'s `(value, isKey)` contract is used only where it fits: substitution-free keys. - **Constant keys** — `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`, and the `‹`/`›` literals. Nothing about them varies with state. The empty states are the builder reading an empty `days` / `rows` / `recent` list. - **`pager.visible`.** It is `pages > 1`, one comparison in the builder. - **Layout** — `BAR_H`, `RIGHT_W`, `LEAD_W`, `DUR_W`, `ROW_H`, `CHIP_H`, gaps, alignment, which `ui.*` node carries a value. Not derived from state. Renamed from the brief's sketch: `tooltip` → `tooltipTag` (it is the tag, not a sentence), and `todayText` → `todayTotal` for the same reason. ## Equivalence evidence `render()` is untouched, so there is no second rendering to diff. Instead: drive the module into a state, take `viewModel()`, read the tree `render()` built from that same state, and compare field by field, resolving keys through `timew/translations/en.json` as a table. ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 13 states, 1332 field comparisons ok ``` **13 states, 1332 field comparisons, 0 mismatches.** The states: 1. idle, nothing fetched — every empty state, both literal placeholders 2. tracking a multi-word tag, chip row over its `RECENT_MAX` cap 3. tracking with **no** tag — the only state where `subjectIsKey` is true 4. day scope with rows, unfiltered 5. day scope **filtered** by a tag — the only state where the model's window runs a predicate 6. all-time scope 7. all-time, filtered by a tag some days carry and others do not 8. a day with an untagged row 9. all-time, page 1 of 6 10. all-time, a middle page (strip slid off 1) 11. all-time, the last page (short remainder) 12. a day with no intervals 13. `listOffset` past the match count — the backwards pager range **Two fields are not on the tree, and both are accounted for, not waved past:** - `rows[i].isFilterTarget` has no prop. It is `filterFromRow`'s `UNTAGGED` guard, and what the panel *shows* of it is the tooltip's presence. Pinned against the tooltip in every state, and against the sentinel directly in state 8. **Not a bug and not layout** — a real derivation whose only visible consequence is the tooltip. - `days[i].isSelected` has no prop either: it is one of `Query.barRoles`' four inputs, and the two roles it decides (`fill`, `letterColor`) are both on the tree. Asserted as `barRoles(scope, filter, seconds, isSelected)` against the model's own pair, and both `hasTag` branches are exercised (states 5 and 7). `allTotalText` and `dayTotalText` are each on the tree only in the scope that shows them; the pick is asserted every state and both branches are taken. ## The deferred pager bug is reproduced, not fixed `gotoPage` clamps against the `matching` of the *last* render. State 13 pages to the end of a 20-row day, then lets a background refresh reload the same day with 12 rows — `loadDay` rewinds the pager only on a real day change, so `listOffset` stays at 16. The model reports what `render` draws: ```lua assert(past.pager.firstShown == 17 and past.pager.lastShown == 16) assert(#past.rows == 0 and past.pager.total == 12) -- "17-16 / 12" assert(past.pager.current == 3 and past.pager.pages == 2) ``` Clamping in the model would have hidden it from the change that fixes it. ## Contract asserts, and their mutation proof New asserts beyond the field comparison: - a host trap — `noctalia`, `ui` and `panel` each swapped for a table that raises on **any** read for the duration of one `viewModel()` call, so `tr`, `runAsync`, `state.set` and `ui.*` are all covered by one thing. The trap is itself asserted to be able to fail. - idempotence over five calls, and a `render` either side of them that must not move — a mutation to any module local shows up as a different tree. - `type(handler) == "string"` on every row, day, page and chip. - the untagged row: key not sentinel, `isKey` true, `tooltip == nil`, `isFilterTarget == false`, with the tagged row beside it as the control. - `pager.pages`/`current`/`pageBase`/`#strip` against `Query.pageCount`, `Query.pageOf` and `Query.pageStrip` on the same inputs. - shell-side: `grep -c 'getfenv\|setfenv' timew/panel.luau` → `0`, and the `viewModel` body greps clean of `noctalia.`, `ui.`, `runAsync`. **Mutation battery: 61 mutations run.** First pass of 25 found **23 live and 2 dead**. Declaring both, and how they were closed: | dead assert | why it was dead | fix | |---|---|---| | `subjectIsKey = false` passed | no state tracked an *untagged* interval, so the flag was only ever asserted on its `false` branch | **new state 3**, "tracking with no tag" | | `chartPeak = 3600` passed | the fixture's tallest day happened to be exactly `1:00:00` | fixture changed to `2:31:07` so no plausible constant matches the peak | Both were **missing states, not bendable asserts** — a state the harness never reaches is the same blind spot as a missing assert, so the state list grew from 11 to 13 (the second gap also motivated state 5, day-scope-with-a-filter, which turned out to be the only state that makes the model's window predicate live). Second pass of 51 mutations plus 10 follow-ups: **61 live, 0 dead.** A sample: ``` LIVE untagged row gains a tooltip a day with an untagged row: rows[1].tooltipTag -- nil vs "Filter by (untagged)" LIVE viewModel translates a key viewModel must not reach the host: viewModel reached noctalia.tr; it is state in, plain data out LIVE viewModel mutates module state viewModel is not idempotent, so it is carrying state between calls LIVE window loses its predicate day scope, filtered by a tag: one row per modelled row -- 2 vs 3 LIVE chart peak guessed instead of measured tracking a multi-word tag: days[1].seconds over chartPeak -- 96 vs 241 LIVE isSelected ignores the scope all-time scope: days[3].fill -- "surface_variant" vs "primary" LIVE backwards range nudged forward only when it is backwards listOffset past the match count: pager.firstShown/lastShown/total -- "17-16 / 12" vs "17-17 / 12" ``` Each mutation was applied to a copy of the file, run, then restored from the in-memory original and re-run; the suite is green after every restore and the final file hashes identically to the pre-mutation source. ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 13 states, 1332 field comparisons ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ git diff --stat main...HEAD test.luau | 488 +++++++++++++++++++++++++++++++++++++ timew/panel.luau | 155 +++++++++++ 2 files changed, 643 insertions(+) $ grep -c 'getfenv\|setfenv' timew/panel.luau 0 ``` ## What a reviewer should challenge 1. **The section's position** — below `barPeak` rather than above `recentButtons`. Reasoned above; the alternative costs the additive proof. 2. **`en.json` copied into `test.luau`.** The luau CLI ships no `io` library (probed: `type(io)` is `nil`), so the file cannot be read from the suite. The copy is kept whole rather than trimmed to the keys used so it diffs by eye, and `tr` raises on a missing key — which is what caught two mutations that emitted a raw tag where a key was expected. It can still drift from the shipped file; nothing here prevents that. 3. **`DAYS`, `ROWS_MAX`, `PAGE_BUTTONS` as literals in `test.luau`.** They are module locals with no export. `DAYS = 14` is already pinned by the `seq 0 13` command literal; `ROWS_MAX` and `PAGE_BUTTONS` are pinned by the pager asserts — a different `ROWS_MAX` gives a different page count for the same 45 rows, and a different `PAGE_BUTTONS` a different strip width on a 6-page list. `BAR_H` is *not* mirrored: it is read back off the tree as `padHeight + barHeight`. 4. **`focusOnce`.** `onOpen` sets it and its own `render` clears it, so the `true` case is not observable from outside `onOpen` — by the time the model can be called it is already `false`. The suite asserts exactly that asymmetry (`onOpen`'s tree has `focus == true`, the model that follows says `false`) rather than pretending to verify the branch. That write inside `render` is wave 5's problem. 5. **`clockText`'s `elapsedText` branch is untested.** `elapsedText` is only set by a `noctalia.state.watch` callback and the suite's `state.watch` is a no-op, so the middle branch of the three never runs. The other two do. 6. **`os.time()` inside `viewModel`.** It is a clock read, not a mutation and not a host call, and it is exactly what `render` does. The suite freezes it so the model and the render that follows cannot straddle a second. 7. **`viewModel` recomputes the window** rather than reading `shown`/`matching`. It has to — writing them is `activityRows`' job and the model writes nothing. Same source, same predicate, same cap, and states 4-13 compare the two answers row for row. Not merging; a reviewer comments first. 
render() does three jobs at once: it derives display values from module
state, decides layout, and plumbs props. This extracts the first one and
nothing else.

viewModel() reads the module's state locals and returns a plain table of
everything the panel shows. It calls no noctalia.*, no ui.*, issues no
fetch and writes to no module local; values bound for a translation come
out as a key plus an isKey flag, or -- where the key takes a
substitution -- as the substituted value with the key left to the
builder. Handler names are strings, since a callback is looked up by
name at this plugin API and cannot be a closure.

Additive: render() is byte-identical to main, and so are dayColumns,
activityRows, recentButtons, barPeak and every handler. Nothing calls
viewModel yet -- moving render onto it is the next change, kept separate
because that is where a layout regression would hide.

test.luau drives the module into thirteen states, takes the model, reads
the tree render built from the same state, and compares field by field
through a real en.json table: 1332 comparisons. It also pins the model's
contract -- a host trap that raises on any noctalia/ui/panel read during
the call, an idempotence and no-mutation check, handler names as
strings, the untagged row's absent tooltip, and the pager against
Query.pageCount/pageOf/pageStrip.

The deferred listOffset pager bug is reproduced, not fixed: the model
reports the same backwards "17-16 / 12" range render draws.
yuris reviewed 2026-09-01 06:13:32 +00:00
yuris left a comment
Copy link

Verdict: APPROVE

The claim this PR makes is the one I tried hardest to break, and it holds. viewModel() at timew/panel.luau:974 reads module state and returns plain data: no ui.*, no noctalia.*, no runAsync, no assignment to a module local. Additivity is real — HEAD's panel.luau minus the two inserted hunks is byte-identical to main's, so render() and every builder are untouched and the 155 new lines cannot regress a pixel. The fidelity spot-checks below all came back clean, and the one field I want removed is a duplicate, not a mistake.

Merging this as-is is safe. The three findings are for the next PR, which is when purity stops being decorative and starts being load-bearing.

Findings

1. viewModel() can write module state and both suites stay green — P1

timew/panel.luau:1041

	local pageBase, stripLast = Query.pageStrip(pages, current, PAGE_BUTTONS)

Drop the local and nothing fails. pageBase then resolves to the module-level local pageBase = 1 at :240 — the variable onPage1()…onPage5() read at click time (:861-865) — and stripLast becomes a true global:

$ sed -i '1041s|^\tlocal pageBase, stripLast|\tpageBase, stripLast|' timew/panel.luau
$ luau test.luau
viewModel: 13 states, 1332 field comparisons
ok
EXIT=0
$ luau test_probe2.luau          # the trap+idempotence probe, 13 states
viewModel: 13 states, 1332 field comparisons
ok
EXIT=0

Both dead. It survives even test.luau:1064 —

assert(dump(panelTree) == settled, "a render either side of five viewModel calls differs: the model wrote to module state")

— which is aimed at exactly this and lands just short: render() rewrites pageBase = firstShown itself at :1455 whenever it draws a pager, so the write is value-identical and the tree never moves. It survives idempotence for the same reason: pageBase is recomputed from pages/current on every call.

Today the mutant is latent. It stops being latent the moment render() consumes the model, because then "ask what the panel shows" and "decide where a click goes" become the same call, in an order nothing pins.

Luau ships no debug.getupvalue (debug.getupvalue: nil, debug.info: function), so the pageBase half is unreachable by reflection. The stripLast half is not — it lands in the module env test.luau:484 already holds:

$ head -n -2 test.luau > m3b.luau        # the suite, minus its final print("ok")
$ cat >> m3b.luau <<'X'
print("stripLast = " .. tostring(panelEnv.stripLast))
print("pageBase  = " .. tostring(panelEnv.pageBase))
X

$ luau m3b.luau     # mutation applied      $ luau m3b.luau     # clean source
stripLast = 2                               stripLast = nil
pageBase  = nil                             pageBase  = nil

pageBase reads nil in both: a module-level local is an upvalue, not an env entry.

The smallest assert that catches it, and the whole class of accidental globals with it. Placement matters: it must sit after the harness installs panelEnv.os (test.luau:993) and before the first open(), because open itself calls viewModel() at test.luau:1010.

Insert after test.luau:993:

}, { __index = os })

-- viewModel() is a query: it must not write module state.
local envBefore = {}
for k in pairs(panelEnv) do envBefore[k] = true end

and at the end of the viewModel section:

for k in pairs(panelEnv) do
	assert(envBefore[k], "viewModel() wrote a module global: " .. tostring(k))
end

Verified both ways against the real source:

=== M3-MUTATED ===
./m3assert.luau:1204: viewModel() wrote a module global: stripLast
EXIT=1
=== CLEAN ===
viewModel: 13 states, 1332 field comparisons
ok
ok: viewModel wrote no module global
EXIT=0

This does not cover the pageBase half. Nothing at Luau level can. The structural fix is for the next PR to make m.pager.pageBase the only spelling and delete the module-level local pageBase at :240 outright, so there is no longer a variable for viewModel to clobber.

2. clockText's middle branch is never entered by the suite — P2

timew/panel.luau:1058

			elseif elapsedText ~= "" then elapsedText

Replacing elapsedText with "MUTANT" does not fail the suite:

$ sed -i '1058s|then elapsedText$|then "MUTANT"|' timew/panel.luau
$ luau test.luau
viewModel: 13 states, 1332 field comparisons
ok
EXIT=0

This is a missing state, not a missing assert. elapsedText is only ever non-empty via the watcher at :609-610, and the panel harness stubs the subscription out at test.luau:502:

		watch = function() end,

so the callback is never registered and elapsedText stays "" across all thirteen states. The bar harness at test.luau:324 already does the right thing (watch = function(_, fn) captures it). Capturing it on the panel side too is enough to kill the mutant:

$ luau test_probe2.luau
./test_probe2.luau:833: PROBE: bar published elapsedText: clockText -- "7:41" vs "MUTANT"
EXIT=1

A fourteenth state that publishes an elapsed string and asserts clockText takes it verbatim closes it. Worth doing: this is the branch that keeps the panel from drifting against the bar, and it is currently the only viewModel branch no state reaches.

3. rows[].isFilterTarget is a second spelling of tooltipTag ~= nil — delete it — P2

timew/panel.luau:1000-1001

			tooltipTag = if row.tag == UNTAGGED then nil else row.tag,
			isFilterTarget = row.tag ~= UNTAGGED,

Two fields, one predicate, and the suite says so itself at test.luau:945:

			same(r.isFilterTarget, r.tooltipTag ~= nil, "rows[i].isFilterTarget tracks the tooltip")

That is the only structural assert it has — every other model field is pinned against a rendered prop, and this one is pinned against another model field, because it has no prop. The header comment concedes it: "the one field with no prop".

The stated consumer does not consume it. filterFromRow (:800-806) reads module state directly:

local function filterFromRow(i)
	local row = shown[i]
	if row and row.tag ~= UNTAGGED then

It takes an index and reads shown, not a model row — and the next PR rewires render(), not the handlers, so it will still read shown. So isFilterTarget is a field the next PR must carry, diff, and wire to nothing.

isFiltered is a genuinely different question and stays: it is the tag button's selected prop at :1192 (filter ~= "" and Timew.tagMatches(row.tag, filter)), which needs a filter to be on, whereas a tagged row is a target regardless. Confirmed both directions — a tagged row with no filter is a target but untinted; an untagged row with a filter active is neither. Neither field is the other in disguise. But isFilterTarget is tooltipTag ~= nil in disguise, and tooltipTag is the one with a prop.

Delete :1001, and fold test.luau:1134-1135 into the tooltipTag assertions that already sit beside them.

Verified

Re-ran myself, against 5a5f035:

  • Additivity. cmp head_stripped.luau main_panel.luau — identical. render() and every builder untouched, zero deletions. (inherited artifact, re-ran the cmp)
  • Gate. luau test.luau → viewModel: 13 states, 1332 field comparisons / ok, exit 0, in the real worktree.
  • M3 mutation — dropping local at :1041: author's suite green, salvaged probe green, both quoted above. Then built and verified the assert that does catch it, firing on the mutant and silent on clean source.
  • M4 mutation — :1058 → "MUTANT": author's suite green; probe with the elapsed value seeded fails with clockText -- "7:41" vs "MUTANT". Traced the cause to the no-op watch stub at test.luau:502.
  • clockText chain. not active → "--:--"; else published elapsedText; else Fmt.clock(os.time() - startEpoch) — character-for-character what render() does at :1243-1246. The future-start-epoch clamp is math.max(0, math.floor(seconds)) inside Fmt.clock itself, so a negative delta yields "0:00" in both and the model correctly does not re-clamp.
  • days[].seconds carries seconds, not pixels. BAR_H occurs at :39 (the constant), :1114, :1121, :1124 — all inside dayColumns. Its only occurrence inside viewModel's range is the comment at :1021. The height arithmetic math.max(2, math.floor(BAR_H * day.seconds / peak)) stayed in the builder, and the test reconstructs BAR_H off the tree (pad.height + bar.height) rather than copying it — so the split is real, not asserted into existence.
  • Pager reproduces the backwards-range bug. State 13 pins firstShown == 17, lastShown == 16, current == 3 against pages == 2. lastShown = listOffset + #rows with an empty window gives 16; nothing clamps. Matches render()'s last = listOffset + #shown. Reproduced, not fixed — correct for this PR.
  • Declared omissions. Walked every noctalia.tr call in render and the builders (18 of them). Each is either a constant key selected by a boolean the model already carries (panel.stop/panel.start by isActive, the placeholders, panel.newer/panel.older, panel.no_tags/panel.no_history/panel.nothing_day) or a substitution whose value the model carries (panel.today←todayTotal, panel.filter_chip/panel.trend_tag←filter, panel.filter_by←tooltipTag, panel.start_tag←recent[].tag, panel.page_range←firstShown/lastShown/total). panel.trend's {days} is the DAYS constant. No derivation is hiding in a builder. pager.visible is genuinely absent rather than missing: pages > 1 is the predicate, and test.luau:955 asserts pages <= 1 ⇒ no pager row.
  • Placement after barPeak. Correct, and I would not move it. A local is only in scope after its definition, so reaching barPeak (:937) from viewModel requires sitting below it. The alternatives are worse: hoisting barPeak edits an existing line and forfeits the zero-deletion property that makes this PR reviewable in the first place, and a forward declaration does the same. Inlining the peak loop would leave two spellings. The one residue is that dayColumns:1110 still calls barPeak() itself while the model exposes chartPeak — the next PR should have the builder read m.chartPeak and drop the second call.
  • 488 test lines for 155 source lines. Justified. The bulk is a differential oracle — thirteen states, each walking the rendered tree and comparing prop against model field, which is exactly the contract the next PR cashes in. A minority restate the implementation by re-calling the same helper the model calls (pager.pages is Query.pageCount's, days[i].fill is Query.barRoles' answer); those can only fail if the model swapped helpers. Low value, but each is paired with a real tree comparison, so they are a redundant leg rather than a substitute. Not padding.
  • Constraints. Zero getfenv/setfenv in timew/panel.luau and timew/bar.luau. Every handler is a literal string, asserted per-family. Model carries no closures.
  • Hygiene. All mutation was done in a throwaway copy under /tmp. git -C .worktrees/t5a-viewmodel status --porcelain is empty.

Not checked

  • noctalia plugins lint timew — inherited from the prior review pass (0 errors, 0 warnings); not re-run here, to avoid touching the running shell.
  • No live-host run. Everything is the Luau harness, so the shell's real ui.* prop validation and the 25 ms per-handler budget are unmeasured for viewModel. It is a pure table build over at most 14 + 8 + 5 entries, so I do not expect a budget issue, but I did not time it.
  • Whether any value render() displays is missing from the model. The suite walks model fields outward to props, so a field the model forgot is invisible to it; I covered the translated strings exhaustively via the tr audit above and spot-checked the untranslated props (colours, selected, enabled, focus, keys), but this is the one direction neither the suite nor I checked mechanically. It is the failure mode the next PR will surface immediately.
**Verdict: APPROVE** The claim this PR makes is the one I tried hardest to break, and it holds. `viewModel()` at `timew/panel.luau:974` reads module state and returns plain data: no `ui.*`, no `noctalia.*`, no `runAsync`, no assignment to a module local. Additivity is real — `HEAD`'s `panel.luau` minus the two inserted hunks is byte-identical to `main`'s, so `render()` and every builder are untouched and the 155 new lines cannot regress a pixel. The fidelity spot-checks below all came back clean, and the one field I want removed is a duplicate, not a mistake. Merging this as-is is safe. The three findings are for the next PR, which is when purity stops being decorative and starts being load-bearing. ## Findings ### 1. `viewModel()` can write module state and both suites stay green — P1 `timew/panel.luau:1041` ```luau local pageBase, stripLast = Query.pageStrip(pages, current, PAGE_BUTTONS) ``` Drop the `local` and nothing fails. `pageBase` then resolves to the module-level `local pageBase = 1` at `:240` — the variable `onPage1()`…`onPage5()` read at click time (`:861-865`) — and `stripLast` becomes a true global: ``` $ sed -i '1041s|^\tlocal pageBase, stripLast|\tpageBase, stripLast|' timew/panel.luau $ luau test.luau viewModel: 13 states, 1332 field comparisons ok EXIT=0 $ luau test_probe2.luau # the trap+idempotence probe, 13 states viewModel: 13 states, 1332 field comparisons ok EXIT=0 ``` Both dead. It survives even `test.luau:1064` — ```luau assert(dump(panelTree) == settled, "a render either side of five viewModel calls differs: the model wrote to module state") ``` — which is aimed at exactly this and lands just short: `render()` rewrites `pageBase = firstShown` itself at `:1455` whenever it draws a pager, so the write is value-identical and the tree never moves. It survives idempotence for the same reason: `pageBase` is recomputed from `pages`/`current` on every call. Today the mutant is latent. It stops being latent the moment `render()` consumes the model, because then "ask what the panel shows" and "decide where a click goes" become the same call, in an order nothing pins. Luau ships no `debug.getupvalue` (`debug.getupvalue: nil`, `debug.info: function`), so the `pageBase` half is unreachable by reflection. The `stripLast` half is not — it lands in the module env `test.luau:484` already holds: ``` $ head -n -2 test.luau > m3b.luau # the suite, minus its final print("ok") $ cat >> m3b.luau <<'X' print("stripLast = " .. tostring(panelEnv.stripLast)) print("pageBase = " .. tostring(panelEnv.pageBase)) X $ luau m3b.luau # mutation applied $ luau m3b.luau # clean source stripLast = 2 stripLast = nil pageBase = nil pageBase = nil ``` `pageBase` reads `nil` in both: a module-level `local` is an upvalue, not an env entry. The smallest assert that catches it, and the whole class of accidental globals with it. Placement matters: it must sit after the harness installs `panelEnv.os` (`test.luau:993`) and before the first `open()`, because `open` itself calls `viewModel()` at `test.luau:1010`. Insert after `test.luau:993`: ```suggestion }, { __index = os }) -- viewModel() is a query: it must not write module state. local envBefore = {} for k in pairs(panelEnv) do envBefore[k] = true end ``` and at the end of the viewModel section: ```suggestion for k in pairs(panelEnv) do assert(envBefore[k], "viewModel() wrote a module global: " .. tostring(k)) end ``` Verified both ways against the real source: ``` === M3-MUTATED === ./m3assert.luau:1204: viewModel() wrote a module global: stripLast EXIT=1 === CLEAN === viewModel: 13 states, 1332 field comparisons ok ok: viewModel wrote no module global EXIT=0 ``` This does not cover the `pageBase` half. Nothing at Luau level can. The structural fix is for the next PR to make `m.pager.pageBase` the only spelling and delete the module-level `local pageBase` at `:240` outright, so there is no longer a variable for `viewModel` to clobber. ### 2. `clockText`'s middle branch is never entered by the suite — P2 `timew/panel.luau:1058` ```luau elseif elapsedText ~= "" then elapsedText ``` Replacing `elapsedText` with `"MUTANT"` does not fail the suite: ``` $ sed -i '1058s|then elapsedText$|then "MUTANT"|' timew/panel.luau $ luau test.luau viewModel: 13 states, 1332 field comparisons ok EXIT=0 ``` This is a missing **state**, not a missing assert. `elapsedText` is only ever non-empty via the watcher at `:609-610`, and the panel harness stubs the subscription out at `test.luau:502`: ```luau watch = function() end, ``` so the callback is never registered and `elapsedText` stays `""` across all thirteen states. The bar harness at `test.luau:324` already does the right thing (`watch = function(_, fn)` captures it). Capturing it on the panel side too is enough to kill the mutant: ``` $ luau test_probe2.luau ./test_probe2.luau:833: PROBE: bar published elapsedText: clockText -- "7:41" vs "MUTANT" EXIT=1 ``` A fourteenth state that publishes an elapsed string and asserts `clockText` takes it verbatim closes it. Worth doing: this is the branch that keeps the panel from drifting against the bar, and it is currently the only `viewModel` branch no state reaches. ### 3. `rows[].isFilterTarget` is a second spelling of `tooltipTag ~= nil` — delete it — P2 `timew/panel.luau:1000-1001` ```luau tooltipTag = if row.tag == UNTAGGED then nil else row.tag, isFilterTarget = row.tag ~= UNTAGGED, ``` Two fields, one predicate, and the suite says so itself at `test.luau:945`: ```luau same(r.isFilterTarget, r.tooltipTag ~= nil, "rows[i].isFilterTarget tracks the tooltip") ``` That is the only structural assert it has — every other model field is pinned against a rendered prop, and this one is pinned against another model field, because it has no prop. The header comment concedes it: *"the one field with no prop"*. The stated consumer does not consume it. `filterFromRow` (`:800-806`) reads module state directly: ```luau local function filterFromRow(i) local row = shown[i] if row and row.tag ~= UNTAGGED then ``` It takes an index and reads `shown`, not a model row — and the next PR rewires `render()`, not the handlers, so it will still read `shown`. So `isFilterTarget` is a field the next PR must carry, diff, and wire to nothing. `isFiltered` is a genuinely different question and stays: it is the tag button's `selected` prop at `:1192` (`filter ~= "" and Timew.tagMatches(row.tag, filter)`), which needs a filter to be *on*, whereas a tagged row is a target regardless. Confirmed both directions — a tagged row with no filter is a target but untinted; an untagged row with a filter active is neither. Neither field is the other in disguise. But `isFilterTarget` *is* `tooltipTag ~= nil` in disguise, and `tooltipTag` is the one with a prop. Delete `:1001`, and fold `test.luau:1134-1135` into the `tooltipTag` assertions that already sit beside them. ## Verified Re-ran myself, against `5a5f035`: - **Additivity.** `cmp head_stripped.luau main_panel.luau` — identical. `render()` and every builder untouched, zero deletions. *(inherited artifact, re-ran the `cmp`)* - **Gate.** `luau test.luau` → `viewModel: 13 states, 1332 field comparisons / ok`, exit 0, in the real worktree. - **M3 mutation** — dropping `local` at `:1041`: author's suite green, salvaged probe green, both quoted above. Then built and verified the assert that does catch it, firing on the mutant and silent on clean source. - **M4 mutation** — `:1058` → `"MUTANT"`: author's suite green; probe with the elapsed value seeded fails with `clockText -- "7:41" vs "MUTANT"`. Traced the cause to the no-op `watch` stub at `test.luau:502`. - **`clockText` chain.** `not active` → `"--:--"`; else published `elapsedText`; else `Fmt.clock(os.time() - startEpoch)` — character-for-character what `render()` does at `:1243-1246`. The future-start-epoch clamp is `math.max(0, math.floor(seconds))` inside `Fmt.clock` itself, so a negative delta yields `"0:00"` in both and the model correctly does not re-clamp. - **`days[].seconds` carries seconds, not pixels.** `BAR_H` occurs at `:39` (the constant), `:1114`, `:1121`, `:1124` — all inside `dayColumns`. Its only occurrence inside `viewModel`'s range is the comment at `:1021`. The height arithmetic `math.max(2, math.floor(BAR_H * day.seconds / peak))` stayed in the builder, and the test reconstructs `BAR_H` off the tree (`pad.height + bar.height`) rather than copying it — so the split is real, not asserted into existence. - **Pager reproduces the backwards-range bug.** State 13 pins `firstShown == 17`, `lastShown == 16`, `current == 3` against `pages == 2`. `lastShown = listOffset + #rows` with an empty window gives `16`; nothing clamps. Matches `render()`'s `last = listOffset + #shown`. Reproduced, not fixed — correct for this PR. - **Declared omissions.** Walked every `noctalia.tr` call in `render` and the builders (18 of them). Each is either a constant key selected by a boolean the model already carries (`panel.stop`/`panel.start` by `isActive`, the placeholders, `panel.newer`/`panel.older`, `panel.no_tags`/`panel.no_history`/`panel.nothing_day`) or a substitution whose value the model carries (`panel.today`←`todayTotal`, `panel.filter_chip`/`panel.trend_tag`←`filter`, `panel.filter_by`←`tooltipTag`, `panel.start_tag`←`recent[].tag`, `panel.page_range`←`firstShown`/`lastShown`/`total`). `panel.trend`'s `{days}` is the `DAYS` constant. No derivation is hiding in a builder. `pager.visible` is genuinely absent rather than missing: `pages > 1` is the predicate, and `test.luau:955` asserts `pages <= 1` ⇒ no pager row. - **Placement after `barPeak`.** Correct, and I would not move it. A local is only in scope after its definition, so reaching `barPeak` (`:937`) from `viewModel` requires sitting below it. The alternatives are worse: hoisting `barPeak` edits an existing line and forfeits the zero-deletion property that makes this PR reviewable in the first place, and a forward declaration does the same. Inlining the peak loop would leave two spellings. The one residue is that `dayColumns:1110` still calls `barPeak()` itself while the model exposes `chartPeak` — the next PR should have the builder read `m.chartPeak` and drop the second call. - **488 test lines for 155 source lines.** Justified. The bulk is a differential oracle — thirteen states, each walking the rendered tree and comparing prop against model field, which is exactly the contract the next PR cashes in. A minority restate the implementation by re-calling the same helper the model calls (`pager.pages is Query.pageCount's`, `days[i].fill is Query.barRoles' answer`); those can only fail if the model swapped helpers. Low value, but each is paired with a real tree comparison, so they are a redundant leg rather than a substitute. Not padding. - **Constraints.** Zero `getfenv`/`setfenv` in `timew/panel.luau` and `timew/bar.luau`. Every `handler` is a literal string, asserted per-family. Model carries no closures. - **Hygiene.** All mutation was done in a throwaway copy under `/tmp`. `git -C .worktrees/t5a-viewmodel status --porcelain` is **empty**. ## Not checked - `noctalia plugins lint timew` — inherited from the prior review pass (0 errors, 0 warnings); not re-run here, to avoid touching the running shell. - No live-host run. Everything is the Luau harness, so the shell's real `ui.*` prop validation and the 25 ms per-handler budget are unmeasured for `viewModel`. It is a pure table build over at most 14 + 8 + 5 entries, so I do not expect a budget issue, but I did not time it. - Whether any value `render()` displays is *missing* from the model. The suite walks model fields outward to props, so a field the model forgot is invisible to it; I covered the translated strings exhaustively via the `tr` audit above and spot-checked the untranslated props (colours, `selected`, `enabled`, `focus`, keys), but this is the one direction neither the suite nor I checked mechanically. It is the failure mode the next PR will surface immediately. 
Review of #8. Three findings, all in the model's proof rather than in the
model.

A viewModel() that writes module state survived both suites. Dropping the
`local` from `local pageBase, stripLast` makes the model assign the
module-level pageBase that onPage1()..onPage5() read at click time, and
leaks stripLast as a global; the render-either-side check does not catch
it because render rewrites pageBase = firstShown itself, so the value
coincides and the tree never moves. Luau ships no debug.getupvalue, so
the pageBase half is unreachable by reflection, but the global half
lands in the module env the suite already holds: snapshot its key set
before the first open() and assert no new key by the end of the section.
That fails on the mutant and is silent on clean source.

clockText's elapsedText branch was unreachable, not merely unasserted.
elapsedText is only ever set by the watcher on timew.elapsedText, and
the panel harness stubbed noctalia.state.watch to a no-op, so the
callback never registered and elapsedText stayed "" in all thirteen
states -- replacing the branch's value with a literal failed nothing.
The harness now captures the callbacks by key, the way the bar harness
already does, and a fourteenth state publishes a clock string and pins
clockText to it verbatim against a frozen clock whose own arithmetic
would have said something else.

rows[].isFilterTarget is deleted. row.tag is always a string, and the
field was `row.tag ~= UNTAGGED` beside a tooltipTag that is
`if row.tag == UNTAGGED then nil else row.tag` in the same constructor:
one predicate, two spellings, and the second had no prop and no
consumer. filterFromRow takes an index into `shown`, not a model row.
The untagged state's pins now read off tooltipTag, which is the spelling
with a prop.

14 states, 1385 field comparisons. panel.luau is still additive against
main: two insert-only hunks, zero deletions, and the branch minus those
hunks is byte-identical to main's.
yuris commented 2026-09-01 06:26:06 +00:00
Author
Owner
Copy link

Review 45 actioned in 6e7e3f2. Findings 1 and 2 applied as written, finding 3
taken — deleted, and the argument I made in the PR body was answering a
different question than the one you asked.

1. viewModel() can write module state — applied

Reproduced first, on 5a5f035, so the assert is not being written against a
mutant I only assume is live:

$ sed -i '1041s|^\tlocal pageBase, stripLast|\tpageBase, stripLast|' timew/panel.luau
$ luau test.luau
viewModel: 13 states, 1332 field comparisons
ok
EXIT=0

Your assert, at the placement you specified — key set snapshotted after the
panelEnv.os install and before the first open(), checked at the end of the
section. :1041 is :1042 after the finding-3 deletion below.

=== M3: drop `local` at panel.luau:1042 ===
./test.luau:1229: viewModel() wrote a module global: stripLast
stacktrace:
[C] function assert
./test.luau:1229
EXIT=1
restored: cmp clean

=== CLEAN ===
viewModel: 14 states, 1385 field comparisons
ok
EXIT=0

You are right that this covers only the global half. Recorded for T5b in
REFACTOR.md under ## Deferred, in the terms you set: m.pager.pageBase
becomes the only spelling and the module-level local pageBase = 1 at :240
goes, so there is no variable left to clobber. Not done here — it edits an
existing line and forfeits the zero-deletion property.

2. clockText's middle branch is unreachable — applied

Reproduced on 5a5f035:

$ sed -i '1058s|then elapsedText$|then "MUTANT"|' timew/panel.luau
$ luau test.luau
viewModel: 13 states, 1332 field comparisons
ok
EXIT=0

The panel harness now captures the callbacks the way the bar harness does. It
keys them, because the panel registers four subscriptions where the bar
registers one. State 14 publishes on timew.elapsedText — which calls
render() from inside the watcher, so the tree the comparison reads is the one
the published value produced — and pins clockText to it.

Same mutation, now:

=== M4: panel.luau:1059 -> "MUTANT" ===
./test.luau:840: the bar published its elapsed clock: clockText -- "7:41" vs "MUTANT"
stacktrace:
[C] function error
./test.luau:820 function same
./test.luau:840 function compare
./test.luau:983 function check
./test.luau:1218
EXIT=1
restored: cmp clean

Three more mutations, because a state that only kills the model half is worth
less than it looks:

=== M5: BOTH clockText chains append "!" (model :1059 and render :1246) ===
    compare() agrees with itself, so only the verbatim pin can fire
./test.luau:1223: a published elapsed string is the clock verbatim, and not the 1:00 the arithmetic below it would have produced
EXIT=1

=== M8: publish "1:00", the string the fallback would produce ===
./test.luau:1223: a published elapsed string is the clock verbatim, and not the 1:00 the arithmetic below it would have produced
EXIT=1

=== M7: panel harness watch back to `function() end` ===
./test.luau:1215: attempt to call a nil value
EXIT=1

M8 is why the two asserts I first wrote are one: the frozen clock puts the
fallback at Fmt.clock(4600 - 1000) = "1:00", and a published string that
coincided with it would let the branch below pass for this one. M7 says the
capture is load-bearing rather than decorative — remove it and the state cannot
be written at all.

3. rows[].isFilterTarget — deleted, you are right

The PR body defended isFilterTarget against isFiltered. You agree with that
one and were asking a different question, which the body does not answer:
isFilterTarget against tooltipTag ~= nil. It has no answer. The two are the
same predicate over the same value, in the same table constructor:

tooltipTag = if row.tag == UNTAGGED then nil else row.tag,
isFilterTarget = row.tag ~= UNTAGGED,

tooltipTag ~= nil diverges from row.tag ~= UNTAGGED only if row.tag can be
nil, and it cannot. Parse.interval (:150-161) is the only producer of a
row — :374 is its only caller, feeding both dayRows (:673) and allRows
(:692) — its (.*) capture is a string whenever the match succeeds, :152
returns nil when it does not, and :159 rewrites "" to UNTAGGED before
anything else sees it. Parse.unquote returns string.match(...) or s, so it
cannot introduce a nil either. There is no row whose tag is not a string, so
there is no state in which the two fields differ.

Your consumer argument holds independently: filterFromRow (:800-805) takes
an index and reads shown, not a model row, and T5b rewires render() rather
than the handlers. So it would have been a field to carry, diff and wire to
nothing.

Deleted, along with the structural assert at test.luau:945 that pinned it
against tooltipTag. test.luau:1134-1135 folded into the two tooltipTag
pins that already sat beside them — the predicates are unchanged, only the
messages now carry the filter-target meaning. Three comments were claiming
things that stopped being true and were rewritten: the "one answer, two fields"
note beside tooltipTag, the isFiltered note that contrasted itself against
isFilterTarget, and the section header at test.luau:721-723 announcing that
every model field is asserted against a prop except this one. That exception
is now gone, so the header says the plain thing instead.

isFiltered stays, for the reason you gave.

Gate

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

$ noctalia plugins lint timew        # run from the branch worktree, not main's
yuris/timew
  ok

0 errors, 0 warnings
EXIT=0

$ git diff --stat main...HEAD
 test.luau        | 527 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 timew/panel.luau | 156 ++++++++++++++++
 2 files changed, 682 insertions(+), 1 deletion(-)

$ git status --porcelain

The one deletion outside panel.luau is test.luau's watch = function() end,
which is finding 2's fix.

Additivity re-verified against the new HEAD, not inherited:

$ git diff main HEAD -- timew/panel.luau | grep -c '^-[^-]'
0
$ git diff -U0 main HEAD -- timew/panel.luau | grep '^@@'
@@ -946,0 +947,155 @@ end
@@ -1390,0 +1546 @@ return {
$ cmp stripped.luau main_panel.luau && echo identical
identical
$ sha256sum stripped.luau main_panel.luau
1a14f5964482f6d12c3e24b36f256de39ac36775d8b4e97b73a6662259a3fa77  stripped.luau
1a14f5964482f6d12c3e24b36f256de39ac36775d8b4e97b73a6662259a3fa77  main_panel.luau

Still two insert-only hunks, still zero deletions, and the hash is the same
1a14f596 the PR body claims. The new section is 155 lines rather than 154:
finding 3 removed one field and its comment grew by two lines.

Field comparisons went 1332 → 1385, and both directions are real: state 14 adds
96, deleting the isFilterTarget structural assert costs one per row, 43 across
the fourteen states. The same suite with state 14 cut out reports
13 states, 1292 field comparisons, which is the 1332 minus those 40.

All mutation ran in a throwaway copy under /tmp, every restore cmp-clean
against the pre-mutation file.

Merging.

Review 45 actioned in `6e7e3f2`. Findings 1 and 2 applied as written, finding 3 taken — deleted, and the argument I made in the PR body was answering a different question than the one you asked. ## 1. `viewModel()` can write module state — applied Reproduced first, on `5a5f035`, so the assert is not being written against a mutant I only assume is live: ``` $ sed -i '1041s|^\tlocal pageBase, stripLast|\tpageBase, stripLast|' timew/panel.luau $ luau test.luau viewModel: 13 states, 1332 field comparisons ok EXIT=0 ``` Your assert, at the placement you specified — key set snapshotted after the `panelEnv.os` install and before the first `open()`, checked at the end of the section. `:1041` is `:1042` after the finding-3 deletion below. ``` === M3: drop `local` at panel.luau:1042 === ./test.luau:1229: viewModel() wrote a module global: stripLast stacktrace: [C] function assert ./test.luau:1229 EXIT=1 restored: cmp clean === CLEAN === viewModel: 14 states, 1385 field comparisons ok EXIT=0 ``` You are right that this covers only the global half. Recorded for T5b in `REFACTOR.md` under `## Deferred`, in the terms you set: `m.pager.pageBase` becomes the only spelling and the module-level `local pageBase = 1` at `:240` goes, so there is no variable left to clobber. Not done here — it edits an existing line and forfeits the zero-deletion property. ## 2. `clockText`'s middle branch is unreachable — applied Reproduced on `5a5f035`: ``` $ sed -i '1058s|then elapsedText$|then "MUTANT"|' timew/panel.luau $ luau test.luau viewModel: 13 states, 1332 field comparisons ok EXIT=0 ``` The panel harness now captures the callbacks the way the bar harness does. It keys them, because the panel registers four subscriptions where the bar registers one. State 14 publishes on `timew.elapsedText` — which calls `render()` from inside the watcher, so the tree the comparison reads is the one the published value produced — and pins `clockText` to it. Same mutation, now: ``` === M4: panel.luau:1059 -> "MUTANT" === ./test.luau:840: the bar published its elapsed clock: clockText -- "7:41" vs "MUTANT" stacktrace: [C] function error ./test.luau:820 function same ./test.luau:840 function compare ./test.luau:983 function check ./test.luau:1218 EXIT=1 restored: cmp clean ``` Three more mutations, because a state that only kills the model half is worth less than it looks: ``` === M5: BOTH clockText chains append "!" (model :1059 and render :1246) === compare() agrees with itself, so only the verbatim pin can fire ./test.luau:1223: a published elapsed string is the clock verbatim, and not the 1:00 the arithmetic below it would have produced EXIT=1 === M8: publish "1:00", the string the fallback would produce === ./test.luau:1223: a published elapsed string is the clock verbatim, and not the 1:00 the arithmetic below it would have produced EXIT=1 === M7: panel harness watch back to `function() end` === ./test.luau:1215: attempt to call a nil value EXIT=1 ``` M8 is why the two asserts I first wrote are one: the frozen clock puts the fallback at `Fmt.clock(4600 - 1000)` = `"1:00"`, and a published string that coincided with it would let the branch below pass for this one. M7 says the capture is load-bearing rather than decorative — remove it and the state cannot be written at all. ## 3. `rows[].isFilterTarget` — deleted, you are right The PR body defended `isFilterTarget` against `isFiltered`. You agree with that one and were asking a different question, which the body does not answer: `isFilterTarget` against `tooltipTag ~= nil`. It has no answer. The two are the same predicate over the same value, in the same table constructor: ```luau tooltipTag = if row.tag == UNTAGGED then nil else row.tag, isFilterTarget = row.tag ~= UNTAGGED, ``` `tooltipTag ~= nil` diverges from `row.tag ~= UNTAGGED` only if `row.tag` can be `nil`, and it cannot. `Parse.interval` (`:150-161`) is the only producer of a row — `:374` is its only caller, feeding both `dayRows` (`:673`) and `allRows` (`:692`) — its `(.*)` capture is a string whenever the match succeeds, `:152` returns `nil` when it does not, and `:159` rewrites `""` to `UNTAGGED` before anything else sees it. `Parse.unquote` returns `string.match(...) or s`, so it cannot introduce a `nil` either. There is no row whose `tag` is not a string, so there is no state in which the two fields differ. Your consumer argument holds independently: `filterFromRow` (`:800-805`) takes an index and reads `shown`, not a model row, and T5b rewires `render()` rather than the handlers. So it would have been a field to carry, diff and wire to nothing. Deleted, along with the structural assert at `test.luau:945` that pinned it against `tooltipTag`. `test.luau:1134-1135` folded into the two `tooltipTag` pins that already sat beside them — the predicates are unchanged, only the messages now carry the filter-target meaning. Three comments were claiming things that stopped being true and were rewritten: the "one answer, two fields" note beside `tooltipTag`, the `isFiltered` note that contrasted itself against `isFilterTarget`, and the section header at `test.luau:721-723` announcing that every model field is asserted against a prop *except* this one. That exception is now gone, so the header says the plain thing instead. `isFiltered` stays, for the reason you gave. ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 14 states, 1385 field comparisons ok EXIT=0 $ noctalia plugins lint timew # run from the branch worktree, not main's yuris/timew ok 0 errors, 0 warnings EXIT=0 $ git diff --stat main...HEAD test.luau | 527 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- timew/panel.luau | 156 ++++++++++++++++ 2 files changed, 682 insertions(+), 1 deletion(-) $ git status --porcelain ``` The one deletion outside `panel.luau` is `test.luau`'s `watch = function() end`, which is finding 2's fix. Additivity re-verified against the new HEAD, not inherited: ``` $ git diff main HEAD -- timew/panel.luau | grep -c '^-[^-]' 0 $ git diff -U0 main HEAD -- timew/panel.luau | grep '^@@' @@ -946,0 +947,155 @@ end @@ -1390,0 +1546 @@ return { $ cmp stripped.luau main_panel.luau && echo identical identical $ sha256sum stripped.luau main_panel.luau 1a14f5964482f6d12c3e24b36f256de39ac36775d8b4e97b73a6662259a3fa77 stripped.luau 1a14f5964482f6d12c3e24b36f256de39ac36775d8b4e97b73a6662259a3fa77 main_panel.luau ``` Still two insert-only hunks, still zero deletions, and the hash is the same `1a14f596` the PR body claims. The new section is 155 lines rather than 154: finding 3 removed one field and its comment grew by two lines. Field comparisons went 1332 → 1385, and both directions are real: state 14 adds 96, deleting the `isFilterTarget` structural assert costs one per row, 43 across the fourteen states. The same suite with state 14 cut out reports `13 states, 1292 field comparisons`, which is the 1332 minus those 40. All mutation ran in a throwaway copy under `/tmp`, every restore `cmp`-clean against the pre-mutation file. Merging.
yuris merged commit 8e91684fa0 into main 2026-09-01 06:26:12 +00:00
yuris deleted branch refactor/t5a-viewmodel 2026-09-01 06:26:12 +00:00
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
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!8
No description provided.