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

feat(panel): clip the history list in a real scroll viewport, delete the pager (takes the row family's closures) #12

Merged
yuris merged 3 commits from upgrade/u2-scroll into main 2026-09-01 13:48:09 +00:00
Conversation 3 Commits 3 Files changed 3 +524 -538
yuris commented 2026-09-01 13:11:55 +00:00
Owner
Copy link

Deletes the pager and replaces it with a real clipped scroll viewport. The row
list is no longer capped: every row that matches the current filter is on the
tree and reachable by wheel or scrollbar.

This PR takes the row family's closures as a prerequisite of uncapping, not as
scope creep. Unbounded rows and named handlers are incompatible — onRow1..8
existed because the list was eight rows, so "delete the cap" and "rows carry
closures" are one change. Writing out onRow1..onRow200 in order to delete them
in the next PR would be work created to be destroyed. Only the row family
converts here; onDay1..14, onTag1..4, onClearFilter, onScopeDay,
onScopeAll, the four input handlers, onStop, onStartDraft, onOpen and
onIpc are untouched and still named. The next task inherits a smaller
registry than planned — 22 named handlers instead of 30, and no onRow
family.

Closures need no version bump. kUiCallbackClosure (9) has zero call sites
in the shell; luau_ui_render routes any lua_isfunction prop to
registerUiHandler with no version check at all (plugin_bindings.cpp:568-586).

Why the viewport clips: the chain, read out of the shell

height on a ui.scroll does not clip, and that is the whole reason the README
concluded scrolling was impossible. It maps to setMinHeight/setMaxHeight, and
ScrollView measures through Flex::measureByLayout, which honours an exact
constraint or m_explicitHeight and nothing else — min/max never bind, so the
viewport falls through to naturalH and m_maxScrollOffset stays 0
(flex.cpp:770-777, scroll_view.cpp:417-418). The README's conclusion was
right; its implied cause was not.

flexGrow binds, but only down an unbroken chain of known heights. The shell
hands one over and the plugin has to claim it:

Step Source What it gives
PanelManager sizes the panel panel_manager.cpp:2644-2653 content height = declared 450 − 2×Style::panelPadding = 422
PluginPanel::create nests the tree in a flexGrow = 1 column inside a root the panel setSizes outright plugin_panel.cpp:147-166, 289 the tree's own root is the first node that can claim 422
flexGrow = 1 on our root row this PR m_contentFlex is vertical, so grow is along its main axis → the row is arranged at 422
ui.row/ui.column default to align = "stretch" ui_tree_reconciler.cpp:646, 655 the right column is arranged at 422 as its cross size — it needs no flexGrow of its own, which is what lets width = RIGHT_W stay authoritative
a Flex arranged by its parent re-enters layout with an exact height constraint flex.cpp:751-757 + node.cpp:113-120 (m_arranging) heightKnown → grow distribution runs
grow distribution hands the leftover to the scroll as an exact main size flex.cpp:525-539 constraints.hasExactHeight() → measureHeight → setSizeFromLayout → height() > 0 → viewportH bounded → m_maxScrollOffset > 0

Drop the flexGrow from the root and every node below it measures at content
height again, and the list silently stops clipping. That is why the suite
asserts all three of root.flexGrow == 1, scroll.flexGrow == 1 and
scroll.height == nil — any one of them alone is not the property.

onScroll is not wired. It fires once per animation frame during the fling,
not per detent, and both args arrive as strings; the panel needs neither the
offset nor a repaint from it, so the simplest correct answer was to leave it out.

No keys on rows. A generated handler name is the node's path, positional for
an unkeyed node (plugin_bindings.cpp:450-458), so the once-a-second repaint
produces byte-identical names and rewires nothing. Keys buy node identity —
input focus, caret, scroll offset, hover slot — and a row holds none of those:
the caret is in the search field above the scroll and the offset belongs to the
ScrollView, which is one positionally stable child. ALL_WALK_MAX = 200 is
also comfortably under kUiTreeMaxChildren = 256, above which the shell drops
the tail with a warning (plugin_bindings.cpp:378, 544-549); the suite asserts
that pair.

Rulings on the caps that were not automatically obsolete

RECENT_MAX = 4 — kept. The chip row is in the left column, a plain flex
beside the viewport rather than inside it, so it still cannot clip. The cap is
a genuine display cap and the comment on fetchTags now says which column it
is about instead of "the panel". Wrapping the left column in a second viewport
to delete this would put two scrolls in competition for one vertical budget for
four buttons' worth of gain.

DAY_WALK_MAX = 60 / ALL_WALK_MAX = 200 — kept, and reclassified. These
are cost controls: each id is one timew get inside a shell loop, so they bound
how many subprocesses a fetch runs, and a clipping viewport changes nothing
about that. What did change is that they are now the only cap between timew
and the screen, so they are also the row count the tree has to survive — which
newly couples them to kUiTreeMaxChildren = 256. That coupling is stated on the
constants and asserted in the suite beside T.cmd.allIntervals(), which reads
the bound back out of the walk script the constant builds. (It was not so
asserted when this PR was first opened: test.luau:130 was assert(200 < 256, …), two literals, and it could not fail. Caught in review, replaced, and
mutation-proved.) Lowering them would lose data; raising ALL_WALK_MAX past 256
would silently drop the tail.

The listOffset bug from REFACTOR.md: unreachable, not merely unlikely

The bug: listOffset was module state that outlived the list it indexed.
gotoPage clamped it against the matching of the last render, so a
background refetch that shrank the day under a pager nobody rewound left the
offset where it was — firstShown = 17, lastShown = 16, an empty window under
"17-16 / 12". The suite reproduced it deliberately.

It is unreachable by construction now, and the argument is structural rather
than a clamp:

  1. There is no offset. listOffset is deleted, and so are its four
    rewind sites (loadDay, setFilter, both scope handlers, onOpen). A
    stale value needs a value.
  2. There is no second number to be stale against. pageCount, pageOf,
    pageStrip, state.matching and the whole m.pager table are gone. The
    backwards range was two derived numbers disagreeing; there is one list now
    and its own length is the count.
  3. The list is derived inside the single viewModel() call that paints.
    Query.matching reads state.dayRows/state.allRows and returns; commit
    publishes that very table into state.shown and hands the same model to
    render. So the only thing that can disagree with the rows on screen is the
    rows on screen.
  4. The one surviving cross-render reference is bounded by the same
    publication.     A row's closure captures an index, and filterFromRow reads
    state.shown[i] — but the handler table is replaced wholesale each render
    (ui_handler_table.h:5-19), so the only closures reachable are the newest
    render's, and that render's commit published the list they index. A short
    list cannot be indexed by a long list's closure.

same(m.pager, nil, ...) runs in every one of the 12 model states, so a pager
table coming back fails 12 times rather than needing a new test.

Deleted tests → the assert that carries the invariant

Deleted Invariant it protected Where it lives now
Query.window(rows, 0, 3) / (rows, 8, 3) — page slicing a page is cap rows from offset died with the feature. There is no page. The replacement claim is the opposite one: Query.matching(wideSource, …) == 200 — "the row cap is gone, not merely raised"
Query.window(rows, 10, 3) / (rows, 99, 3) — offset at/past the end an offset outliving the list does not wrap or error died with the feature, and is the bug closed above: there is no offset. Structurally replaced by same(m.pager, nil) in all 12 states
Query.window(…, predicate) narrows window and count a predicate filters #matched == 5 + matched[1] == rows[2] and matched[5] == rows[10] — same claim, plus order and by-reference identity the old assert did not make
Query.window({}, 0, 3) empty source empty is empty, not nil #Query.matching({}, …) == 0, "an empty source is empty, not nil" — verbatim
Query.pageCount(0/8/9, 8) (3 asserts) ceil-with-floor-of-1 page arithmetic died with the feature. No page count exists to be wrong
Query.pageOf(0/8/15, 8) (3 asserts) offset → page number died with the feature. No offset, no page number
Query.pageStrip centring, 5-wide and 3-wide (9 asserts) the strip window centres, clamps at both ends, never starts below 1 died with the feature. The strip existed only because a fixed set of names had to cover a variable page count; closures removed the fixed set
#clickedTexts(…) == 8 "the window caps the page at ROWS_MAX" the panel shows ROWS_MAX, not what timew sent inverted: #clickedTexts(panelTree, ROW) == 20 "every matching row is on the tree, not a page of them" + #scrollNode().children == 20
#clickedTexts(…) == 4 "the last page is the remainder" a short last page is not padded died with the feature. Subsumed: there is one list and its length is #m.rows, asserted per state in compare
not onScreen("1-8 / 20") + onScreen("panel.page_range") the range label is a key, not a concatenation half kept as not onScreen("1-8 / 20") ("the range label the panel used to build is gone"); the page_range half died with the label. panel.page_range, panel.newer and panel.older had no consumer left once buildPager went, so all three are deleted from en.json and from the suite's EN copy of it
m.pager.pages/current/pageBase/strip vs Query.* (5 asserts in compare) the model's pager numbers are the pure core's, not re-derived died with the feature, replaced by the negative: same(m.pager, nil) in every state
pager row shape: #kids == strip + 4, ‹/› handlers and enabled, per-slot text/selected/handler, range label (11 asserts in compare) the pager row is built from the model and does no arithmetic died with the feature. The structural claim that replaced it is that the chrome is outside the viewport: #rightKids == 6, search at 4, scope at 5, rightKids[6].kind == "scroll", and every scroll child is a row
page1.pager.pages == 6, #strip == PAGE_BUTTONS, pageBase == 2, #rows == 5 (states 9-11) ROWS_MAX and PAGE_BUTTONS pinned through a 45-row list replaced by #wideModel.rows == 45, #scrollNode().children == 45, #clickedTexts(…, ROW) == 45, and panel.state.shown == panel.state.allRows (the by-reference publication)
onPage1..5 read m.pager.pageBase (state 13, 3 asserts per slot) a click goes to the page its button showed, from a base read at click time replaced by clickRow(45) → filter == "tag45" on a fixture with a distinct tag per row. Same invariant — a click resolves to the thing its node showed — on the mechanism that replaced the strip. The fixture's tags are distinct because a repeated tag would let every closure capture the same index and pass
past.pager.firstShown == 17 and lastShown == 16 (state 13, the reproduced bug) the bug is reproduced, not clamped replaced by the same fixture asserting the fix: 20 rows → 12 rows, #scrollNode().children == 12, children[13] == nil, and clickRow(12) still resolving
paintsFor("a page click", 1, …) a page click costs one repaint died with the feature. Wheel scrolling costs no repaint at all, which is why onScroll is unwired
state.matching == m.pager.total in paintsFor and at the tail the published count is the painted model's died with the field. Its sibling survives and now carries the whole publication: #panel.state.shown == #m.matchedRows plus per-row identity
panelEnv.onRow1() / onRow2() the published slice is what a row click reads strengthened to clickRow(1) / clickRow(2), which invoke the tree's own onClick prop. The suite can no longer name a handler the panel did not put on a node

New coverage with no deleted counterpart: the three layout props
(root.flexGrow, scroll.flexGrow, scroll.height == nil); the shared right
edge (chromeW - rowW == INSET, INSET + rowW == chromeW,
scroll.width == rowW + INSET*2 + GUTTER); r.handler == nil and
type(onClick) == "function" and dot-shares-tag's-closure, per row per state;
ALL_WALK_MAX < kUiTreeMaxChildren.

Geometry: unchanged, deliberately

plugin.toml keeps width = 578, height = 450. The width is untouched, and the
height is now a real budget instead of dead space: deleting the pager row
(CHIP_H 22 + gap 6) hands 28 px back to the list, so the viewport shows
roughly 9-10 rows where the pager showed 8 — and the rest are a wheel away
rather than four clicks.

One thing did have to move, and it is the change most worth challenging: the
history block's right edge is RIGHT_W - 18 rather than RIGHT_W. The scroll
takes Style::spaceXs (4) on each side as m_viewportPaddingH — unreachable
from a plugin, since kScroll's paddingH configures the inner content flex and
not the viewport (ui_tree_reconciler.cpp:1693-1701) — and reserves
scrollbarWidth 6 + scrollbarGap 8 when the content overflows
(scroll_view.cpp:429-431). Both are reserved unconditionally here, because
a gutter that appeared and vanished with the row count would slide the duration
column 14 px sideways the moment the list outgrew the viewport. So rows are
LIST_W and the chrome is CHROME_W = LIST_W + 4, which puts the duration
column and the scope total on the same pixel — the pair the README insists must
line up. The cost is that the history block sits 18 px inside the chart's right
edge, one block down past a separator, and the list reads as indented 4 px under
its own header. The alternative — chrome at RIGHT_W, rows at LIST_W — puts
the 18 px step between the scope total and the durations directly beneath it,
which is worse.

Gates

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

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

$ grep -cE "ROWS_MAX|PAGE_BUTTONS|listOffset|pageStrip" timew/panel.luau
0

Line delta: panel.luau 1782 → 1700 (−82). test.luau 1939 → 2010
(+71). en.json 34 → 31 (−3). Net −14 across the three.

What a reviewer should challenge hardest

  1. The layout chain is read from source, not from a screenshot of this tree.
    Every link is cited above and the shape differs from the probe's verified one
    in three ways: the root is a row carrying flexGrow rather than a column;
    the scroll's parent gets its height purely from the root row's default
    align = "stretch", never from a flexGrow of its own; and nothing sits
    below the scroll, so "a footer stays pinned" is not what is being claimed —
    the chrome above it staying put is. A live screenshot of this exact tree is
    requested and pending; if the real viewportPaddingH or gutter differs from
    4/14 the two constants move and the asserts move with them, but the clipping
    argument does not depend on those numbers.
  2. Query.matching returns the source table by reference when there is no
    predicate. That is deliberate — all-time scope filters in timew, so a copy
    would duplicate up to 200 rows once a second — and it means state.shown,
    state.allRows and m.matchedRows are one table in that scope. Nothing
    mutates it today; a builder that ever did would move state without writing
    state.<field>, which is the shape the note on the state table already
    warns about.
  3. The row closure captures an index, not the row. Capturing the row would
    remove state.shown from the click path entirely. It captures the index
    because onIpc("row", n) arrives with nothing but a number, and one
    filterFromRow(i) for both paths beats two spellings of "row i" — but it
    does mean the publication in commit stays load-bearing, and the argument
    that a stale index is unreachable rests on the handler table being replaced
    wholesale each render rather than on anything in this file.
  4. Two closures fewer than rows. One closure per row is shared by the dot and
    the tag button; compare asserts they are the same value. If they ever
    diverge, a click on the dot and a click on the tag would filter by different
    rows and only that assert would say so.
  5. RECENT_MAX staying is a judgement, not a proof. The left column could
    host its own scroll. I ruled it not worth a second viewport for four buttons;
    disagree and it is a one-line change plus a chain like the one above.
Deletes the pager and replaces it with a real clipped scroll viewport. The row list is no longer capped: every row that matches the current filter is on the tree and reachable by wheel or scrollbar. **This PR takes the row family's closures as a prerequisite of uncapping, not as scope creep.** Unbounded rows and named handlers are incompatible — `onRow1..8` existed *because* the list was eight rows, so "delete the cap" and "rows carry closures" are one change. Writing out `onRow1..onRow200` in order to delete them in the next PR would be work created to be destroyed. Only the row family converts here; `onDay1..14`, `onTag1..4`, `onClearFilter`, `onScopeDay`, `onScopeAll`, the four input handlers, `onStop`, `onStartDraft`, `onOpen` and `onIpc` are untouched and still named. **The next task inherits a smaller registry than planned** — 22 named handlers instead of 30, and no `onRow` family. Closures need no version bump. `kUiCallbackClosure` (9) has **zero call sites** in the shell; `luau_ui_render` routes any `lua_isfunction` prop to `registerUiHandler` with no version check at all (`plugin_bindings.cpp:568-586`). ## Why the viewport clips: the chain, read out of the shell `height` on a `ui.scroll` does not clip, and that is the whole reason the README concluded scrolling was impossible. It maps to `setMinHeight`/`setMaxHeight`, and `ScrollView` measures through `Flex::measureByLayout`, which honours an *exact* constraint or `m_explicitHeight` and nothing else — min/max never bind, so the viewport falls through to `naturalH` and `m_maxScrollOffset` stays 0 (`flex.cpp:770-777`, `scroll_view.cpp:417-418`). The README's conclusion was right; its implied cause was not. `flexGrow` binds, but only down an unbroken chain of known heights. The shell hands one over and the plugin has to claim it: | Step | Source | What it gives | |---|---|---| | `PanelManager` sizes the panel | `panel_manager.cpp:2644-2653` | content height = declared 450 − 2×`Style::panelPadding` = 422 | | `PluginPanel::create` nests the tree in a `flexGrow = 1` column inside a root the panel `setSize`s outright | `plugin_panel.cpp:147-166`, `289` | the tree's own root is the first node that *can* claim 422 | | **`flexGrow = 1` on our root row** | this PR | `m_contentFlex` is vertical, so grow is along its main axis → the row is arranged at 422 | | `ui.row`/`ui.column` default to `align = "stretch"` | `ui_tree_reconciler.cpp:646, 655` | the right column is arranged at 422 as its cross size — it needs no `flexGrow` of its own, which is what lets `width = RIGHT_W` stay authoritative | | a Flex arranged by its parent re-enters layout with an exact height constraint | `flex.cpp:751-757` + `node.cpp:113-120` (`m_arranging`) | `heightKnown` → grow distribution runs | | grow distribution hands the leftover to the scroll as an **exact main size** | `flex.cpp:525-539` | `constraints.hasExactHeight()` → `measureHeight` → `setSizeFromLayout` → `height() > 0` → `viewportH` bounded → `m_maxScrollOffset > 0` | Drop the `flexGrow` from the root and every node below it measures at content height again, and the list silently stops clipping. That is why the suite asserts all three of `root.flexGrow == 1`, `scroll.flexGrow == 1` and `scroll.height == nil` — any one of them alone is not the property. **`onScroll` is not wired.** It fires once per animation frame during the fling, not per detent, and both args arrive as strings; the panel needs neither the offset nor a repaint from it, so the simplest correct answer was to leave it out. **No keys on rows.** A generated handler name is the node's path, positional for an unkeyed node (`plugin_bindings.cpp:450-458`), so the once-a-second repaint produces byte-identical names and rewires nothing. Keys buy node identity — input focus, caret, scroll offset, hover slot — and a row holds none of those: the caret is in the search field above the scroll and the offset belongs to the `ScrollView`, which is one positionally stable child. `ALL_WALK_MAX = 200` is also comfortably under `kUiTreeMaxChildren = 256`, above which the shell drops the tail with a warning (`plugin_bindings.cpp:378, 544-549`); the suite asserts that pair. ## Rulings on the caps that were *not* automatically obsolete **`RECENT_MAX = 4` — kept.** The chip row is in the **left** column, a plain flex *beside* the viewport rather than inside it, so it still cannot clip. The cap is a genuine display cap and the comment on `fetchTags` now says which column it is about instead of "the panel". Wrapping the left column in a second viewport to delete this would put two scrolls in competition for one vertical budget for four buttons' worth of gain. **`DAY_WALK_MAX = 60` / `ALL_WALK_MAX = 200` — kept, and reclassified.** These are cost controls: each id is one `timew get` inside a shell loop, so they bound how many subprocesses a fetch runs, and a clipping viewport changes nothing about that. What *did* change is that they are now the **only** cap between timew and the screen, so they are also the row count the tree has to survive — which newly couples them to `kUiTreeMaxChildren = 256`. That coupling is stated on the constants and asserted in the suite beside `T.cmd.allIntervals()`, which reads the bound back out of the walk script the constant builds. (It was **not** so asserted when this PR was first opened: `test.luau:130` was `assert(200 < 256, …)`, two literals, and it could not fail. Caught in review, replaced, and mutation-proved.) Lowering them would lose data; raising `ALL_WALK_MAX` past 256 would silently drop the tail. ## The `listOffset` bug from `REFACTOR.md`: unreachable, not merely unlikely The bug: `listOffset` was module state that outlived the list it indexed. `gotoPage` clamped it against the `matching` of the *last* render, so a background refetch that shrank the day under a pager nobody rewound left the offset where it was — `firstShown = 17`, `lastShown = 16`, an empty window under "17-16 / 12". The suite reproduced it deliberately. It is unreachable by construction now, and the argument is structural rather than a clamp: 1. **There is no offset.** `listOffset` is deleted, and so are its four rewind sites (`loadDay`, `setFilter`, both scope handlers, `onOpen`). A stale value needs a value. 2. **There is no second number to be stale against.** `pageCount`, `pageOf`, `pageStrip`, `state.matching` and the whole `m.pager` table are gone. The backwards range was two derived numbers disagreeing; there is one list now and its own length is the count. 3. **The list is derived inside the single `viewModel()` call that paints.** `Query.matching` reads `state.dayRows`/`state.allRows` and returns; `commit` publishes that very table into `state.shown` and hands the same model to `render`. So the only thing that can disagree with the rows on screen is the rows on screen. 4. **The one surviving cross-render reference is bounded by the same publication.** A row's closure captures an index, and `filterFromRow` reads `state.shown[i]` — but the handler table is replaced wholesale each render (`ui_handler_table.h:5-19`), so the only closures reachable are the newest render's, and that render's `commit` published the list they index. A short list cannot be indexed by a long list's closure. `same(m.pager, nil, ...)` runs in every one of the 12 model states, so a `pager` table coming back fails 12 times rather than needing a new test. ## Deleted tests → the assert that carries the invariant | Deleted | Invariant it protected | Where it lives now | |---|---|---| | `Query.window(rows, 0, 3)` / `(rows, 8, 3)` — page slicing | a page is `cap` rows from `offset` | **died with the feature.** There is no page. The replacement claim is the opposite one: `Query.matching(wideSource, …) == 200` — "the row cap is gone, not merely raised" | | `Query.window(rows, 10, 3)` / `(rows, 99, 3)` — offset at/past the end | an offset outliving the list does not wrap or error | **died with the feature**, and is the bug closed above: there is no offset. Structurally replaced by `same(m.pager, nil)` in all 12 states | | `Query.window(…, predicate)` narrows window and count | a predicate filters | `#matched == 5` + `matched[1] == rows[2] and matched[5] == rows[10]` — same claim, plus order and by-reference identity the old assert did not make | | `Query.window({}, 0, 3)` empty source | empty is empty, not nil | `#Query.matching({}, …) == 0, "an empty source is empty, not nil"` — verbatim | | `Query.pageCount(0/8/9, 8)` (3 asserts) | ceil-with-floor-of-1 page arithmetic | **died with the feature.** No page count exists to be wrong | | `Query.pageOf(0/8/15, 8)` (3 asserts) | offset → page number | **died with the feature.** No offset, no page number | | `Query.pageStrip` centring, 5-wide and 3-wide (9 asserts) | the strip window centres, clamps at both ends, never starts below 1 | **died with the feature.** The strip existed only because a fixed set of names had to cover a variable page count; closures removed the fixed set | | `#clickedTexts(…) == 8` "the window caps the page at ROWS_MAX" | the panel shows `ROWS_MAX`, not what timew sent | **inverted:** `#clickedTexts(panelTree, ROW) == 20` "every matching row is on the tree, not a page of them" + `#scrollNode().children == 20` | | `#clickedTexts(…) == 4` "the last page is the remainder" | a short last page is not padded | **died with the feature.** Subsumed: there is one list and its length is `#m.rows`, asserted per state in `compare` | | `not onScreen("1-8 / 20")` + `onScreen("panel.page_range")` | the range label is a key, not a concatenation | half kept as `not onScreen("1-8 / 20")` ("the range label the panel used to build is gone"); the `page_range` half **died with the label**. `panel.page_range`, `panel.newer` and `panel.older` had no consumer left once `buildPager` went, so all three are deleted from `en.json` and from the suite's `EN` copy of it | | `m.pager.pages/current/pageBase/strip` vs `Query.*` (5 asserts in `compare`) | the model's pager numbers are the pure core's, not re-derived | **died with the feature**, replaced by the negative: `same(m.pager, nil)` in every state | | pager row shape: `#kids == strip + 4`, `‹`/`›` handlers and `enabled`, per-slot text/selected/handler, range label (11 asserts in `compare`) | the pager row is built from the model and does no arithmetic | **died with the feature.** The structural claim that replaced it is that the *chrome* is outside the viewport: `#rightKids == 6`, search at 4, scope at 5, `rightKids[6].kind == "scroll"`, and every scroll child is a `row` | | `page1.pager.pages == 6`, `#strip == PAGE_BUTTONS`, `pageBase == 2`, `#rows == 5` (states 9-11) | ROWS_MAX and PAGE_BUTTONS pinned through a 45-row list | **replaced by** `#wideModel.rows == 45`, `#scrollNode().children == 45`, `#clickedTexts(…, ROW) == 45`, and `panel.state.shown == panel.state.allRows` (the by-reference publication) | | `onPage1..5` read `m.pager.pageBase` (state 13, 3 asserts per slot) | a click goes to the page its button showed, from a base read at click time | **replaced by** `clickRow(45)` → `filter == "tag45"` on a fixture with a distinct tag per row. Same invariant — a click resolves to the thing its node showed — on the mechanism that replaced the strip. The fixture's tags are distinct *because* a repeated tag would let every closure capture the same index and pass | | `past.pager.firstShown == 17 and lastShown == 16` (state 13, the reproduced bug) | the bug is reproduced, not clamped | **replaced by** the same fixture asserting the fix: 20 rows → 12 rows, `#scrollNode().children == 12`, `children[13] == nil`, and `clickRow(12)` still resolving | | `paintsFor("a page click", 1, …)` | a page click costs one repaint | **died with the feature.** Wheel scrolling costs no repaint at all, which is why `onScroll` is unwired | | `state.matching == m.pager.total` in `paintsFor` and at the tail | the published count is the painted model's | **died with the field.** Its sibling survives and now carries the whole publication: `#panel.state.shown == #m.matchedRows` plus per-row identity | | `panelEnv.onRow1()` / `onRow2()` | the published slice is what a row click reads | **strengthened to** `clickRow(1)` / `clickRow(2)`, which invoke the tree's own `onClick` prop. The suite can no longer name a handler the panel did not put on a node | New coverage with no deleted counterpart: the three layout props (`root.flexGrow`, `scroll.flexGrow`, `scroll.height == nil`); the shared right edge (`chromeW - rowW == INSET`, `INSET + rowW == chromeW`, `scroll.width == rowW + INSET*2 + GUTTER`); `r.handler == nil` and `type(onClick) == "function"` and dot-shares-tag's-closure, per row per state; `ALL_WALK_MAX < kUiTreeMaxChildren`. ## Geometry: unchanged, deliberately `plugin.toml` keeps `width = 578, height = 450`. The width is untouched, and the height is now a real budget instead of dead space: deleting the pager row (`CHIP_H 22` + `gap 6`) hands 28 px back to the list, so the viewport shows roughly 9-10 rows where the pager showed 8 — and the rest are a wheel away rather than four clicks. One thing did have to move, and it is the change most worth challenging: the history block's right edge is `RIGHT_W - 18` rather than `RIGHT_W`. The scroll takes `Style::spaceXs` (4) on each side as `m_viewportPaddingH` — unreachable from a plugin, since `kScroll`'s `paddingH` configures the inner content flex and not the viewport (`ui_tree_reconciler.cpp:1693-1701`) — and reserves `scrollbarWidth 6 + scrollbarGap 8` when the content overflows (`scroll_view.cpp:429-431`). Both are reserved **unconditionally** here, because a gutter that appeared and vanished with the row count would slide the duration column 14 px sideways the moment the list outgrew the viewport. So rows are `LIST_W` and the chrome is `CHROME_W = LIST_W + 4`, which puts the duration column and the scope total on the same pixel — the pair the README insists must line up. The cost is that the history block sits 18 px inside the chart's right edge, one block down past a separator, and the list reads as indented 4 px under its own header. The alternative — chrome at `RIGHT_W`, rows at `LIST_W` — puts the 18 px step *between the scope total and the durations directly beneath it*, which is worse. ## Gates ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 12 states, 1606 field comparisons ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ grep -cE "ROWS_MAX|PAGE_BUTTONS|listOffset|pageStrip" timew/panel.luau 0 ``` Line delta: `panel.luau` 1782 → 1700 (**−82**). `test.luau` 1939 → 2010 (**+71**). `en.json` 34 → 31 (**−3**). Net **−14** across the three. ## What a reviewer should challenge hardest 1. **The layout chain is read from source, not from a screenshot of *this* tree.** Every link is cited above and the shape differs from the probe's verified one in three ways: the root is a `row` carrying `flexGrow` rather than a column; the scroll's parent gets its height purely from the root row's default `align = "stretch"`, never from a `flexGrow` of its own; and nothing sits below the scroll, so "a footer stays pinned" is not what is being claimed — the chrome *above* it staying put is. A live screenshot of this exact tree is requested and pending; if the real `viewportPaddingH` or gutter differs from 4/14 the two constants move and the asserts move with them, but the clipping argument does not depend on those numbers. 2. **`Query.matching` returns the source table by reference** when there is no predicate. That is deliberate — all-time scope filters in timew, so a copy would duplicate up to 200 rows once a second — and it means `state.shown`, `state.allRows` and `m.matchedRows` are one table in that scope. Nothing mutates it today; a builder that ever did would move state without writing `state.<field>`, which is the shape the note on the `state` table already warns about. 3. **The row closure captures an index, not the row.** Capturing the row would remove `state.shown` from the click path entirely. It captures the index because `onIpc("row", n)` arrives with nothing but a number, and one `filterFromRow(i)` for both paths beats two spellings of "row i" — but it does mean the publication in `commit` stays load-bearing, and the argument that a stale index is unreachable rests on the handler table being replaced wholesale each render rather than on anything in this file. 4. **Two closures fewer than rows.** One closure per row is shared by the dot and the tag button; `compare` asserts they are the same value. If they ever diverge, a click on the dot and a click on the tag would filter by different rows and only that assert would say so. 5. **`RECENT_MAX` staying is a judgement, not a proof.** The left column *could* host its own scroll. I ruled it not worth a second viewport for four buttons; disagree and it is a one-line change plus a chain like the one above. 
`ui.scroll` clips via flexGrow, never via height: height maps to
setMinHeight/setMaxHeight and ScrollView's measurement honours only an exact
constraint or m_explicitHeight, so a sized scroll falls through to naturalH and
maxScrollOffset stays 0 (flex.cpp:770-777, scroll_view.cpp:417-418). The README
concluded scrolling was impossible from that measurement; the conclusion was
right and the cause was not.

flexGrow = 1 on the root row claims the panel's content height off the
flexGrow = 1 column PluginPanel::create nests the tree in
(plugin_panel.cpp:147-166, 289); stretch is the ui.row/ui.column default
(ui_tree_reconciler.cpp:646, 655) so the right column inherits that height as
its cross size; and a Flex arranged by its parent re-enters layout with an exact
height constraint (flex.cpp:751-757), which is what lets it hand the leftover to
the flexGrow = 1 scroll as an exact main size.

So the pager goes: PAGE_BUTTONS, Query.pageStrip/pageCount/pageOf, listOffset,
state.matching, gotoPage, onPagePrev/Next, onPage1..5, the numbered buttons, the
range label, buildPager, the model's whole `pager` table, and the ROWS_MAX
windowing in Query.window -- which collapses to Query.matching, a filter with no
offset, no cap and no second return value.

The row family's onClick becomes a closure, as the prerequisite of uncapping
rather than as separate work: onRow1..8 existed because the list was eight rows,
so an uncapped list has no fixed set of names to draw from. Closures are not
gated -- kUiCallbackClosure has zero call sites and luau_ui_render registers any
function-valued prop with no version check (plugin_bindings.cpp:568-586). Every
other named handler is untouched.

RECENT_MAX stays: the chip row is in the left column, beside the viewport rather
than inside it, so it still cannot clip. DAY_WALK_MAX and ALL_WALK_MAX stay and
are reclassified -- cost controls over timew subprocesses, now also the only cap
between timew and the tree, which newly couples ALL_WALK_MAX to the shell's
kUiTreeMaxChildren = 256.

Closes the listOffset bug REFACTOR.md carried, by construction: there is no
offset, no second derived number to be stale against, and the list is derived
inside the one viewModel() call that paints.
The reason on LIST_W was "the duration column must not move", which is true but
weaker than the real constraint. The scrollbar is painted whether the list
scrolls or not: m_scrollbar->setVisible reads the always-true m_showScrollbar
(scroll_view.cpp:447), only the gutter reservation keys off m_scrollbarShown, and
setScrollbarVisible is absent from the kScroll whitelist -- so a plugin cannot
turn the bar off, and a row sized to the content width would sit under it on any
day short enough not to scroll.

Verified on a rendered 578x450 panel: the scope row's right edge and a history
row's right edge both land on x=1212 with the thumb at x=1221..1226, so
4 + 283 + 8 + 6 + 4 = 305 and LIST_W/CHROME_W are correct as written. No code
change.
yuris commented 2026-09-01 13:17:59 +00:00
Author
Owner
Copy link

Live verification: the viewport clips, and the right edges are exact

The PR body derived the layout chain from the shell source and flagged that a
screenshot of this exact tree was pending. It has now been rendered in a
578x450 panel, screenshotted, wheel-scrolled, and measured off the raw PNG. All
three claims that differ from the previously-probed shape hold.

1. Right-edge alignment: exact, zero offset. LIST_W = 283 / CHROME_W = 287
are correct as written.

scope row (Day/All) y=513:  tint #405843 ends x=1212, bg from 1213
history row 02:00   y=570:  tint #424C42 ends x=1212, bg from 1213
scrollbar thumb            #7E847B spans x=1221..1226

Decomposing from the column origin: 4 left pad + 283 content + 8 gap + 6 bar + 4 right pad = 305. Scale is 1 on the test box, so there is no
scaled()/raw-Style mismatch hiding in it. Constants confirmed in style.h:
spaceXs = 4 (:23), spaceSm = 8 (:24), scrollbarWidth = 6 (:48),
scrollbarGap = spaceSm (:49).

2. Clipping: yes. Thumb at the top of the track on open, rows 1-10 fully
visible, row 11 cut mid-height at the panel's inner edge. Measured during the
fling: lastOffset=224 lastMax=635. With 40 rows at height 20 + gap 1 the
content is 839, so the viewport is 839 - 635 = 204 px — 9.7 rows against the
pager's 8, with the remaining 30 a wheel away instead of four clicks.

3. Chrome above stays put. After a four-notch wheel over the scroll body the
rows showed 11:00..21:00 while the trend label, all 14 bars, the horizontal
separator, the search row and the Day/All row were pixel-identical and fully
visible. The viewport clips at the top too — row 11 renders as a partial row.

4. The vertical separator now spans the panel. Stretch does give it the root
row's height (~424 px of the 448 px interior) rather than content height, which
it had before. Sampled down x=917 it peaks mid-panel and fades to the
background within ~14 px of each end, so it reads as a soft vertical fade rather
than a crisp rule. That is ui.separator's own gradient, not a layout artifact;
a hard rule would need a ui.box({ width = 1 }). Left alone as a cosmetic
question outside this PR.

One finding that strengthened a decision, and is now in the code

The scrollbar is painted whether the list scrolls or not:
m_scrollbar->setVisible reads the always-true m_showScrollbar
(scroll_view.cpp:447), only the gutter reservation keys off
m_scrollbarShown (:429-430), and setScrollbarVisible exists in C++ but is
absent from the kScroll whitelist — so a plugin cannot turn it off.

This makes the unconditional gutter reservation the only correct answer rather
than merely the steadier one, and it retires the alternative the PR body
considered and rejected. Measured at 6 rows: content 125 px against a 204 px
viewport, no scrolling, thumb still painted at x=1221..1226. Rows stay 283
wide, so both right edges still land on 1212 and the bar sits in the 14 px of
slack. Had the rows taken flexGrow instead of width = LIST_W they would have
jumped 14 px wider at exactly the row count where the list stopped
scrolling — the worst possible moment for a duration column to move.

3dc78b4 records that reason on LIST_W. Comment only, no code change; both
gates re-run green on it.

Non-finding, checked because it is a silent failure mode

textAlign accepts only start | center | end and warns-and-drops anything
else (ui_tree_reconciler.cpp:253-259), so a "right" would have shipped a
ragged duration column with nothing but a log line. Both call sites here —
buildList's duration label and buildScope's total — already use end.

What this changes in the PR's "challenge hardest" list

Item 1 (the layout chain read from source rather than a screenshot of this tree)
is now closed by measurement: the shape works, the constants are right, and the
one number worth knowing is that the viewport is 204 px. Items 2-5 stand as
written.

## Live verification: the viewport clips, and the right edges are exact The PR body derived the layout chain from the shell source and flagged that a screenshot of *this exact tree* was pending. It has now been rendered in a 578x450 panel, screenshotted, wheel-scrolled, and measured off the raw PNG. All three claims that differ from the previously-probed shape hold. **1. Right-edge alignment: exact, zero offset.** `LIST_W = 283` / `CHROME_W = 287` are correct as written. ``` scope row (Day/All) y=513: tint #405843 ends x=1212, bg from 1213 history row 02:00 y=570: tint #424C42 ends x=1212, bg from 1213 scrollbar thumb #7E847B spans x=1221..1226 ``` Decomposing from the column origin: `4 left pad + 283 content + 8 gap + 6 bar + 4 right pad = 305`. Scale is 1 on the test box, so there is no `scaled()`/raw-`Style` mismatch hiding in it. Constants confirmed in `style.h`: `spaceXs = 4` (:23), `spaceSm = 8` (:24), `scrollbarWidth = 6` (:48), `scrollbarGap = spaceSm` (:49). **2. Clipping: yes.** Thumb at the top of the track on open, rows 1-10 fully visible, row 11 cut mid-height at the panel's inner edge. Measured during the fling: `lastOffset=224 lastMax=635`. With 40 rows at `height 20 + gap 1` the content is 839, so the viewport is `839 - 635 = 204 px` — 9.7 rows against the pager's 8, with the remaining 30 a wheel away instead of four clicks. **3. Chrome above stays put.** After a four-notch wheel over the scroll body the rows showed 11:00..21:00 while the trend label, all 14 bars, the horizontal separator, the search row and the Day/All row were pixel-identical and fully visible. The viewport clips at the top too — row 11 renders as a partial row. **4. The vertical separator now spans the panel.** Stretch does give it the root row's height (~424 px of the 448 px interior) rather than content height, which it had before. Sampled down `x=917` it peaks mid-panel and fades to the background within ~14 px of each end, so it reads as a soft vertical fade rather than a crisp rule. That is `ui.separator`'s own gradient, not a layout artifact; a hard rule would need a `ui.box({ width = 1 })`. Left alone as a cosmetic question outside this PR. ## One finding that strengthened a decision, and is now in the code The scrollbar is painted **whether the list scrolls or not**: `m_scrollbar->setVisible` reads the always-true `m_showScrollbar` (`scroll_view.cpp:447`), only the *gutter reservation* keys off `m_scrollbarShown` (:429-430), and `setScrollbarVisible` exists in C++ but is absent from the `kScroll` whitelist — so a plugin cannot turn it off. This makes the unconditional gutter reservation the only correct answer rather than merely the steadier one, and it retires the alternative the PR body considered and rejected. Measured at 6 rows: content 125 px against a 204 px viewport, no scrolling, thumb still painted at `x=1221..1226`. Rows stay 283 wide, so both right edges still land on 1212 and the bar sits in the 14 px of slack. Had the rows taken `flexGrow` instead of `width = LIST_W` they would have **jumped 14 px wider at exactly the row count where the list stopped scrolling** — the worst possible moment for a duration column to move. `3dc78b4` records that reason on `LIST_W`. Comment only, no code change; both gates re-run green on it. ## Non-finding, checked because it is a silent failure mode `textAlign` accepts only `start | center | end` and warns-and-drops anything else (`ui_tree_reconciler.cpp:253-259`), so a `"right"` would have shipped a ragged duration column with nothing but a log line. Both call sites here — `buildList`'s duration label and `buildScope`'s total — already use `end`. ## What this changes in the PR's "challenge hardest" list Item 1 (the layout chain read from source rather than a screenshot of this tree) is now closed by measurement: the shape works, the constants are right, and the one number worth knowing is that the viewport is 204 px. Items 2-5 stand as written.
yuris reviewed 2026-09-01 13:34:27 +00:00
yuris left a comment
Copy link

Verdict: REQUEST_CHANGES

The layout chain is right, the pager is gone without residue, the closures are correct
under the hazard UPGRADE.md names, and the deleted-test table is honest in 17 of its 18
rows. One row is not, and it is the row that matters: the assert this PR advertises as the
guard between ALL_WALK_MAX and kUiTreeMaxChildren is a comparison of two literals and
cannot fail. Two shipped source comments state that it holds the pair together. It does
not, and I proved it does not.

Everything below was derived independently: the shell chain re-read at
/nix/store/7pgw3p2dhkn8sfpnli7qiyzs9zyhr01d-source, the numbers recomputed, 26 mutations
run against lines this PR wrote in a throwaway copy at /tmp/mut (the worktree was never
written to; final md5sum matches the pristine tree).

Blocking

1. test.luau:130 — the ALL_WALK_MAX / kUiTreeMaxChildren assert cannot fail

assert(200 < 256, "ALL_WALK_MAX is under the shell's per-node child cap, or the tail of the list is dropped unrendered")

Both sides are literals. 200 is not read from ALL_WALK_MAX (a module local, no export),
and 256 is not read from anything. The assert is true at parse time and stays true for
every possible value of the constant it claims to bound.

Two shipped comments claim otherwise:

  • timew/panel.luau:70-71 — "so ALL_WALK_MAX has to stay under that; the 200-row assert
    in test.luau is what holds the pair together."
  • PR body — "That coupling is stated on the constants and asserted in the suite", and
    "the suite asserts that pair."

Proof it is not held together. Two runs, ALL_WALK_MAX = 200 -> 300:

ALL_WALK_MAX = 300, nothing else changed
  -> FAIL at test.luau:349  "the all-time walk takes no tag and defaults to ALL_WALK_MAX"

ALL_WALK_MAX = 300, and test.luau:351/354 updated to `-le 300` / allIntervals(300)
   (i.e. the one-line fix any author would make when that assert fires)
  -> viewModel: 12 states, 1606 field comparisons
     ok

So the only thing that fires is the argv-script literal at :349, whose message is about
walk defaults and says nothing about the tree; the natural repair walks straight past
:130, which still reads 200 < 256. At that point the scroll node is handed up to 300
children and the shell clamps it:

// plugin_bindings.cpp:543-551
const int count = std::min(lua_objlen(L, children), kUiTreeMaxChildren);
if (lua_objlen(L, children) > kUiTreeMaxChildren) {
  kLog.warn("plugin {}: ui node '{}' has more than {} children, extra dropped", ...);
}

(kUiTreeMaxChildren = 256 at plugin_bindings.cpp:378.) The node is kept, the tree still
renders, 44 rows are gone, and the only trace is one shell log line. That is exactly the
silent failure the assert is named for.

The cap is observable from the suite — T.cmd.allIntervals() already carries it — so
the real check costs two lines. Placed at test.luau:355 (after the existing walk asserts,
where T is in scope; line 130 is above the module load and has no T):

local walkCap = tonumber(string.match(T.cmd.allIntervals(), "%-le (%d+) %]"))
assert(walkCap ~= nil and walkCap < 256, "ALL_WALK_MAX " .. tostring(walkCap) .. " is at or over the shell per-node child cap, so the tail of the list is dropped unrendered")

Verified both ways: green on HEAD, and with ALL_WALK_MAX = 300 plus the :351/:354
literals updated it fails with
test.luau:356: ALL_WALK_MAX 300 is at or over the shell per-node child cap, so the tail of the list is dropped unrendered.

Delete test.luau:130 when you add it, and the sentence at timew/panel.luau:70-71 becomes
true as written.

Advisory

2. test.luau:1472-1473 — only row 45's captured index is pinned; interior rows are not

The fixture was built with a distinct tag per row precisely so a closure's index is
observable — "a repeated tag would let every closure capture the same index and pass" —
and then only the last row is clicked. A stale index on any interior row survives the whole
suite:

mutation: buildList closure -> filterFromRow(if i == 5 then 1 else i)
  -> viewModel: 12 states, 1606 field comparisons
     ok

clickRow is called at 1, 2, 12 and 45; rows 3-11 and 13-44 are unverified. The fixture
already pays for the coverage, so collect it. Replacing the two lines at 1472-1473:

for n = 1, 45 do
	clickRow(n)
	assert(panel.state.filter == string.format("tag%02d", n), "row " .. n .. " closure filters by its own tag, got " .. panel.state.filter)
end

Verified: green on HEAD, and it kills the row-5 mutation at test.luau:1474. Each click
repaints, and all-time scope passes no predicate, so the 45 rows survive every iteration.

3. test.luau:966-975 — the shared right edge is pinned on the scope row only

chromeW is read from rightKids[5] (the scope row). rightKids[4], the search row this
PR moved from RIGHT_W to CHROME_W, is never checked, so half the chrome can drift back:

mutation: buildSearch -> ui.row({ ..., width = RIGHT_W })   (panel.luau:1402)
  -> viewModel: 12 states, 1606 field comparisons
     ok

That leaves the search field 18 px wider than the scope row directly under it and the rows
under that — the exact invariant the PR lists as new coverage. One line, after :968:

assert(rightKids[4].props.width == chromeW, "the search row shares the scope row's width: " .. tostring(rightKids[4].props.width) .. " vs " .. chromeW)

Verified: green on HEAD, fails at test.luau:969 under the mutation.

4. timew/panel.luau:26-46 — "the scrollbar is painted whether the list scrolls or not" is not what the source does

Commit 3dc78b4 rests its whole reason on this:

m_scrollbar->setVisible reads the always-true m_showScrollbar (scroll_view.cpp:447) and
only the gutter reservation keys off m_scrollbarShown

:447 makes the Scrollbar container node visible. Its parts are gated one layer down,
and ScrollView::doLayout feeds them the live extents on every pass
(scroll_view.cpp:449, m_scrollbar->update(viewportH, contentHeight, m_scrollOffset)):

// scrollbar.cpp:117-125
m_shown = contentExtent > viewportExtent + 0.5F;
m_track->setVisible(m_shown);
m_thumb->setVisible(m_shown);
m_trackArea->setVisible(m_shown);
m_thumbArea->setVisible(m_shown);
if (!m_shown) {
  m_thumbTravel = 0.0F;
  return;
}

At the 6-row case the comment cites (content 125 px, viewport 204 px), m_shown is false
and track, thumb and both hit areas are hidden — nothing is painted in the gutter. The
reported thumb at x=1221..1226 under 6 rows does not follow from this source.

No code change: LIST_W/CHROME_W are correct either way, because rows carry a fixed
width = LIST_W and cannot grow into the freed 14 px. The paragraph's second argument is
the true one and is worth keeping on its own —
m_scrollbarShown = m_showScrollbar && m_content->height() > viewportH + 0.5F
(scroll_view.cpp:429) does mean the gutter appears and vanishes at the overflow boundary,
which is what would move a flexible row 14 px. Drop the "painted whether the list scrolls or
not" claim and keep the jump argument.

5. timew/translations/en.json:28,29,33 — three dead keys survive the pager

panel.newer, panel.older and panel.page_range had exactly one consumer between them:
buildPager, deleted here. Nothing in timew/ reads any of the three now; the only other
occurrences are the suite's EN mirror (test.luau:1048,1049,1053). The PR body accounts
for page_range only, and does so backwards — it stays "because that table is a verbatim
mirror of en.json", but en.json is the source and the mirror is the copy. newer and
older are not mentioned at all. noctalia plugins lint reports 0 errors, 0 warnings, so
nothing catches this. Delete all three from en.json and from the EN mirror.

6. Small stale references

  • test.luau:685-688 — the comment above clickedProp still reads "onTagN is the chip
    row, onRowN the activity list"    , four lines above the ROW = "^function" selector that
    replaced ^onRow%d+$ precisely because onRowN no longer exists.
  • timew/panel.luau:975 cites plugin_bindings.cpp:568-586 for "luau_ui_render registers
    any function-valued prop"    . 568-586 is luau_ui_render, which creates the fresh handler
    table and swaps it in; the registration is plugin_bindings.cpp:523-524
    (if (lua_isfunction(L, -1)) { out.props.emplace(propName, registerUiHandler(...)); })
    plus registerUiHandler at 463-471. Same for panel.luau:1496 citing 450-458 for the
    node path — uiNodePath is 459-463. The substance is right in both cases (there is no
    version gate anywhere on that path; kUiCallbackClosurePluginApiVersion = 9 is defined at
    plugin_api.h:11 and referenced nowhere in the tree), only the line spans are off.
  • PR body line counts are stale by one commit: panel.luau is 1695 at 3dc78b4, not 1687,
    so -87 not -95, and the net across the two files is -34 not -42.

Verified

The scroll clips, and every ancestor passes an exact height down. Re-derived from
source, independent of the screenshot:

# Node What gives it a height Source
1 m_flex (root column, Stretch) m_flex->setSize(width, height) outside layout pins m_explicitHeight plugin_panel.cpp:289; flex.cpp:123-127
2 m_contentFlex (column, Stretch, flexGrow = 1.0F) grow share of a vertical parent -> exact main constraint plugin_panel.cpp:153-156; flex.cpp:537-538
3 plugin tree root = this PR's ui.row({ ..., flexGrow = 1 }) mounted as m_contentFlex's single child (reconcile pushes one root, no wrapper); non-grow children are measured loose on the main axis (measureItem(item, false, 0.0F)), so without flexGrow here the chain breaks at this node ui_tree_reconciler.cpp:620-623; flex.cpp:510, 528-539
4 right ui.column({ width = RIGHT_W }) — no flexGrow cross-axis Stretch of a horizontal parent; ui.row/ui.column default to FlexAlign::Stretch ui_tree_reconciler.cpp:645-646, 654-655; flex.cpp:493-494
5 arranged -> re-enters layout exact Node::arrange sets m_arranging; Flex::doArrange -> runLayout(LayoutConstraints::exact(...)) node.cpp:113-118; flex.cpp:734-736, 765-767
6 ui.scroll({ flexGrow = 1 }) grow share -> hasExactHeight() -> setSizeFromLayout -> height() > 0 -> bounded viewportH -> m_maxScrollOffset > 0 flex.cpp:770-777; scroll_view.cpp:417-419, 441-442

No non-growing ancestor anywhere on the path. The negative half holds too: height on a
scroll maps to setMinHeight/setMaxHeight (ui_tree_reconciler.cpp:1706-1709), and
ScrollView::doLayout reads only height() (:418) — min/max are clamped in
Flex::runLayout, which ScrollView overrides — so a sized scroll falls to naturalH and
m_maxScrollOffset is 0. One correction for the record: ScrollView::doMeasure is
scroll_view.cpp:486-488, a three-line delegation to Flex::measureByLayout; the height
choice cited as scroll_view.cpp:417-418 is in doLayout. flexGrow is on the kScroll
whitelist (ui_tree_reconciler.cpp:496) and is applied generically before type dispatch
(:960-962).

Geometry, recomputed. RIGHT_W = 19*14 + 3*13 = 305. m_viewportPaddingH = Style::spaceXs = 4
(scroll_view.h:94, style.h:23), gutter = scrollbarWidth 6 + scrollbarGap 8 = 14
(style.h:48-49,24). availableW = 305 - 8 = 297; contentWidth = 297 - 14 = 283 = LIST_W;
CHROME_W = 287. 4 + 283 + 8 + 6 + 4 = 305. Both constants correct as written.

Row closures are correct under the real hazard. Every conditional path in buildList
was walked: onRow is built unconditionally per iteration and assigned unconditionally to
both click targets; the only conditionals are tooltip (not a handler) and the #out == 0
empty-state label. i is fresh per ipairs iteration. m.rows and m.matchedRows are
built one-to-one in the same loop, so the captured index addresses the published list.
render has exactly one caller — commit at panel.luau:402 — and state.shown is
published at :401, before the paint, from the same model object. The cross-render argument
holds at the shell too: handlers register into a fresh table per render and the table is
swapped wholesale only on a successful root read (plugin_bindings.cpp:576, 581-582), and
pushCallback resolves @ui:-prefixed names against the live table only
(luau_host.cpp:3085-3091) — a miss returns before any result machinery
(script_runtime.cpp:702-705), silently, as UPGRADE.md says.

Cross-boundary, producer to consumer. A function-valued onClick is registered
unconditionally by readUiTreeNode (plugin_bindings.cpp:523-524), regardless of node
type, and the prop the tree carries is the generated name string. onClick is on both
consuming whitelists — kBox (ui_tree_reconciler.cpp:456) and kButton (:474) — so
neither of the row's two click targets is dropped. No version gate exists on that path.

Caps. ALL_WALK_MAX = 200 -> at most 200 scroll children, under kUiTreeMaxChildren = 256;
tree depth is 5 against kUiTreeMaxDepth = 32 (plugin_bindings.cpp:377). RECENT_MAX
staying is sound: the chip row is panelTree.children[1], the left column, not inside the
scroll.

The deleted-test table. All 18 rows checked against the file. 17 are honest, including
the four I expected to be soft: the Query.window predicate row is genuinely strengthened
(order and by-reference identity, test.luau:101-106); the pager-row-shape row is really
replaced by the chrome-outside-the-viewport asserts (:936-942); the onPage1..5 row is
really replaced by clickRow(45) on a distinct-tag fixture (:1472-1473, see finding 2 for
its one gap); and the reproduced-bug row is really replaced by 12 rows,
children[13] == nil and a resolving clickRow(12) (:1494-1503). same(m.pager, nil)
does run in all 12 states. The 18th is finding 1.

Gates, re-run at 3dc78b4 in the worktree.

nix shell nixpkgs#luau -c luau test.luau  ->  viewModel: 12 states, 1606 field comparisons / ok
noctalia plugins lint timew              ->  yuris/timew / ok / 0 errors, 0 warnings
grep -cE "ROWS_MAX|PAGE_BUTTONS|listOffset|pageStrip" timew/panel.luau  ->  0

The grep claim holds, and a wider sweep (pageCount|pageOf|gotoPage|onPage|buildPager|m\.pager|page_range)
returns nothing in panel.luau either — no pager concept survives under a new name. The
only pager residue anywhere is the three translation keys in finding 5.

Not checked

  • Live rendering. I did not re-render the panel; the shell chain above is a source
    derivation, and finding 4 is where it disagrees with the reported measurement. Everything
    else in the live comment is consistent with the source.
  • Repaint cost at 200 rows. The list went from 8 rows to up to 200, once a second while
    tracking — roughly 1000 tree tables and 200 registered closures per paint, against the
    25 ms per-handler budget the file cites. The suite's paintsFor counts paints, not their
    cost. I have no measurement either way, so I am not raising it; it is the one thing
    uncapping bought that nothing in the diff bounds.
  • filterFromRow's nil guard (panel.luau:1030) is untested — removing row and
    leaves the suite green — but both the guard and the gap predate this PR (4deaaf2
    panel.luau:998; base suite only exercised onIpc("row", 1) and ("row", 2)), so it is
    out of scope here. noctalia msg plugin yuris/timew:panel all row 99 is the reachable
    path if you want it covered.
  • The 1606 field-comparison count and the 12-state count were taken from the run, not
    audited assert by assert.

Mutations

26 mutations against lines this PR wrote, in a copy at /tmp/mut; every one restored and
the tree re-verified byte-identical (md5sum 1c45dd1e…, diff -r clean). The worktree was
never modified. 4 survivors.

# Mutation Result
1 root row: drop flexGrow = 1 killed :954
2 scroll: drop flexGrow = 1 killed :952
3 scroll: add height = 200 killed :953
4 ui.scroll -> ui.column killed :940
5 dot loses its closure killed :1284 (compare)
6 every closure captures 1 killed :1473
7 only row 5 captures a stale index SURVIVED -> finding 2
8 no-predicate path copies instead of returning by reference killed :101
9 Query.matching re-caps at 8 killed :107
10 commit stops publishing state.shown killed :915
11 rows pinned to RIGHT_W killed :967
12 scope row back to RIGHT_W killed :967
12b search row back to RIGHT_W SURVIVED -> finding 3
13 scroll width = CHROME_W killed :974
14 ALL_WALK_MAX 200 -> 300 killed :349 (argv literal, not :130)
14b 14 + the :351/:354 literals updated SURVIVED -> finding 1
15 filterFromRow drops its nil guard SURVIVED (pre-existing, see Not checked)
16 search row moved inside the viewport killed :929
17 scroll gap = 1 -> gap = 6 SURVIVED (cosmetic; no assert warranted)
18 empty-state label dropped from buildList killed :996
19 commit publishes a copy, not the model's table killed :1469
20 only the first 8 rows get a closure killed :928
21 tag button loses its closure, dot keeps it killed :851
22 chrome order swapped (scope above search) killed :938
23 rows built from state.dayRows, not matchedRows killed :851
24 Query.matching reverses order killed :106
25 commit publishes a second viewModel() call killed :1599

The kill set is genuinely strong — 22 of 26, including every layout prop, both closure
targets, the publication link and the chrome/viewport split. The three actionable survivors
are all in new coverage this PR added, and two of them (2 and 3) are one edit each against
fixtures that already exist.

**Verdict: REQUEST_CHANGES** The layout chain is right, the pager is gone without residue, the closures are correct under the hazard `UPGRADE.md` names, and the deleted-test table is honest in 17 of its 18 rows. One row is not, and it is the row that matters: the assert this PR advertises as the guard between `ALL_WALK_MAX` and `kUiTreeMaxChildren` is a comparison of two literals and cannot fail. Two shipped source comments state that it holds the pair together. It does not, and I proved it does not. Everything below was derived independently: the shell chain re-read at `/nix/store/7pgw3p2dhkn8sfpnli7qiyzs9zyhr01d-source`, the numbers recomputed, 26 mutations run against lines this PR wrote in a throwaway copy at `/tmp/mut` (the worktree was never written to; final `md5sum` matches the pristine tree). --- # Blocking ## 1. `test.luau:130` — the `ALL_WALK_MAX` / `kUiTreeMaxChildren` assert cannot fail ```lua assert(200 < 256, "ALL_WALK_MAX is under the shell's per-node child cap, or the tail of the list is dropped unrendered") ``` Both sides are literals. `200` is not read from `ALL_WALK_MAX` (a module local, no export), and `256` is not read from anything. The assert is true at parse time and stays true for every possible value of the constant it claims to bound. Two shipped comments claim otherwise: - `timew/panel.luau:70-71` — *"so `ALL_WALK_MAX` has to stay under that; the 200-row assert in test.luau is what holds the pair together."* - PR body — *"That coupling is stated on the constants and asserted in the suite"*, and *"the suite asserts that pair."* **Proof it is not held together.** Two runs, `ALL_WALK_MAX = 200 -> 300`: ``` ALL_WALK_MAX = 300, nothing else changed -> FAIL at test.luau:349 "the all-time walk takes no tag and defaults to ALL_WALK_MAX" ALL_WALK_MAX = 300, and test.luau:351/354 updated to `-le 300` / allIntervals(300) (i.e. the one-line fix any author would make when that assert fires) -> viewModel: 12 states, 1606 field comparisons ok ``` So the only thing that fires is the argv-script literal at `:349`, whose message is about walk defaults and says nothing about the tree; the natural repair walks straight past `:130`, which still reads `200 < 256`. At that point the scroll node is handed up to 300 children and the shell clamps it: ```cpp // plugin_bindings.cpp:543-551 const int count = std::min(lua_objlen(L, children), kUiTreeMaxChildren); if (lua_objlen(L, children) > kUiTreeMaxChildren) { kLog.warn("plugin {}: ui node '{}' has more than {} children, extra dropped", ...); } ``` (`kUiTreeMaxChildren = 256` at `plugin_bindings.cpp:378`.) The node is kept, the tree still renders, 44 rows are gone, and the only trace is one shell log line. That is exactly the silent failure the assert is named for. The cap **is** observable from the suite — `T.cmd.allIntervals()` already carries it — so the real check costs two lines. Placed at `test.luau:355` (after the existing walk asserts, where `T` is in scope; line 130 is above the module load and has no `T`): ```suggestion local walkCap = tonumber(string.match(T.cmd.allIntervals(), "%-le (%d+) %]")) assert(walkCap ~= nil and walkCap < 256, "ALL_WALK_MAX " .. tostring(walkCap) .. " is at or over the shell per-node child cap, so the tail of the list is dropped unrendered") ``` Verified both ways: green on `HEAD`, and with `ALL_WALK_MAX = 300` plus the `:351`/`:354` literals updated it fails with `test.luau:356: ALL_WALK_MAX 300 is at or over the shell per-node child cap, so the tail of the list is dropped unrendered`. Delete `test.luau:130` when you add it, and the sentence at `timew/panel.luau:70-71` becomes true as written. --- # Advisory ## 2. `test.luau:1472-1473` — only row 45's captured index is pinned; interior rows are not The fixture was built with a distinct tag per row precisely so a closure's index is observable — *"a repeated tag would let every closure capture the same index and pass"* — and then only the last row is clicked. A stale index on any interior row survives the whole suite: ``` mutation: buildList closure -> filterFromRow(if i == 5 then 1 else i) -> viewModel: 12 states, 1606 field comparisons ok ``` `clickRow` is called at 1, 2, 12 and 45; rows 3-11 and 13-44 are unverified. The fixture already pays for the coverage, so collect it. Replacing the two lines at 1472-1473: ```suggestion for n = 1, 45 do clickRow(n) assert(panel.state.filter == string.format("tag%02d", n), "row " .. n .. " closure filters by its own tag, got " .. panel.state.filter) end ``` Verified: green on `HEAD`, and it kills the row-5 mutation at `test.luau:1474`. Each click repaints, and all-time scope passes no predicate, so the 45 rows survive every iteration. ## 3. `test.luau:966-975` — the shared right edge is pinned on the scope row only `chromeW` is read from `rightKids[5]` (the scope row). `rightKids[4]`, the search row this PR moved from `RIGHT_W` to `CHROME_W`, is never checked, so half the chrome can drift back: ``` mutation: buildSearch -> ui.row({ ..., width = RIGHT_W }) (panel.luau:1402) -> viewModel: 12 states, 1606 field comparisons ok ``` That leaves the search field 18 px wider than the scope row directly under it and the rows under that — the exact invariant the PR lists as new coverage. One line, after `:968`: ```suggestion assert(rightKids[4].props.width == chromeW, "the search row shares the scope row's width: " .. tostring(rightKids[4].props.width) .. " vs " .. chromeW) ``` Verified: green on `HEAD`, fails at `test.luau:969` under the mutation. ## 4. `timew/panel.luau:26-46` — "the scrollbar is painted whether the list scrolls or not" is not what the source does Commit `3dc78b4` rests its whole reason on this: > `m_scrollbar->setVisible` reads the always-true m_showScrollbar (scroll_view.cpp:447) and > only the *gutter reservation* keys off m_scrollbarShown `:447` makes the Scrollbar **container node** visible. Its parts are gated one layer down, and `ScrollView::doLayout` feeds them the live extents on every pass (`scroll_view.cpp:449`, `m_scrollbar->update(viewportH, contentHeight, m_scrollOffset)`): ```cpp // scrollbar.cpp:117-125 m_shown = contentExtent > viewportExtent + 0.5F; m_track->setVisible(m_shown); m_thumb->setVisible(m_shown); m_trackArea->setVisible(m_shown); m_thumbArea->setVisible(m_shown); if (!m_shown) { m_thumbTravel = 0.0F; return; } ``` At the 6-row case the comment cites (content 125 px, viewport 204 px), `m_shown` is false and track, thumb and both hit areas are hidden — nothing is painted in the gutter. The reported thumb at `x=1221..1226` under 6 rows does not follow from this source. No code change: `LIST_W`/`CHROME_W` are correct either way, because rows carry a fixed `width = LIST_W` and cannot grow into the freed 14 px. The paragraph's *second* argument is the true one and is worth keeping on its own — `m_scrollbarShown = m_showScrollbar && m_content->height() > viewportH + 0.5F` (`scroll_view.cpp:429`) does mean the gutter appears and vanishes at the overflow boundary, which is what would move a flexible row 14 px. Drop the "painted whether the list scrolls or not" claim and keep the jump argument. ## 5. `timew/translations/en.json:28,29,33` — three dead keys survive the pager `panel.newer`, `panel.older` and `panel.page_range` had exactly one consumer between them: `buildPager`, deleted here. Nothing in `timew/` reads any of the three now; the only other occurrences are the suite's `EN` mirror (`test.luau:1048,1049,1053`). The PR body accounts for `page_range` only, and does so backwards — it stays "because that table is a verbatim mirror of `en.json`", but `en.json` is the source and the mirror is the copy. `newer` and `older` are not mentioned at all. `noctalia plugins lint` reports `0 errors, 0 warnings`, so nothing catches this. Delete all three from `en.json` and from the `EN` mirror. ## 6. Small stale references - `test.luau:685-688` — the comment above `clickedProp` still reads *"onTagN is the chip row, onRowN the activity list"*, four lines above the `ROW = "^function"` selector that replaced `^onRow%d+$` precisely because `onRowN` no longer exists. - `timew/panel.luau:975` cites `plugin_bindings.cpp:568-586` for *"luau_ui_render registers any function-valued prop"*. `568-586` is `luau_ui_render`, which creates the fresh handler table and swaps it in; the registration is `plugin_bindings.cpp:523-524` (`if (lua_isfunction(L, -1)) { out.props.emplace(propName, registerUiHandler(...)); }`) plus `registerUiHandler` at `463-471`. Same for `panel.luau:1496` citing `450-458` for the node path — `uiNodePath` is `459-463`. The substance is right in both cases (there is no version gate anywhere on that path; `kUiCallbackClosurePluginApiVersion = 9` is defined at `plugin_api.h:11` and referenced nowhere in the tree), only the line spans are off. - PR body line counts are stale by one commit: `panel.luau` is 1695 at `3dc78b4`, not 1687, so **-87** not -95, and the net across the two files is **-34** not -42. --- # Verified **The scroll clips, and every ancestor passes an exact height down.** Re-derived from source, independent of the screenshot: | # | Node | What gives it a height | Source | |---|---|---|---| | 1 | `m_flex` (root column, Stretch) | `m_flex->setSize(width, height)` outside layout pins `m_explicitHeight` | `plugin_panel.cpp:289`; `flex.cpp:123-127` | | 2 | `m_contentFlex` (column, Stretch, **`flexGrow = 1.0F`**) | grow share of a vertical parent -> exact main constraint | `plugin_panel.cpp:153-156`; `flex.cpp:537-538` | | 3 | plugin tree root = this PR's `ui.row({ ..., flexGrow = 1 })` | mounted as `m_contentFlex`'s single child (`reconcile` pushes one root, no wrapper); non-grow children are measured *loose* on the main axis (`measureItem(item, false, 0.0F)`), so **without `flexGrow` here the chain breaks at this node** | `ui_tree_reconciler.cpp:620-623`; `flex.cpp:510`, `528-539` | | 4 | right `ui.column({ width = RIGHT_W })` — no `flexGrow` | cross-axis Stretch of a horizontal parent; `ui.row`/`ui.column` default to `FlexAlign::Stretch` | `ui_tree_reconciler.cpp:645-646, 654-655`; `flex.cpp:493-494` | | 5 | arranged -> re-enters layout exact | `Node::arrange` sets `m_arranging`; `Flex::doArrange` -> `runLayout(LayoutConstraints::exact(...))` | `node.cpp:113-118`; `flex.cpp:734-736, 765-767` | | 6 | `ui.scroll({ flexGrow = 1 })` | grow share -> `hasExactHeight()` -> `setSizeFromLayout` -> `height() > 0` -> bounded `viewportH` -> `m_maxScrollOffset > 0` | `flex.cpp:770-777`; `scroll_view.cpp:417-419, 441-442` | No non-growing ancestor anywhere on the path. The negative half holds too: `height` on a scroll maps to `setMinHeight`/`setMaxHeight` (`ui_tree_reconciler.cpp:1706-1709`), and `ScrollView::doLayout` reads only `height()` (`:418`) — min/max are clamped in `Flex::runLayout`, which `ScrollView` overrides — so a sized scroll falls to `naturalH` and `m_maxScrollOffset` is 0. One correction for the record: `ScrollView::doMeasure` is `scroll_view.cpp:486-488`, a three-line delegation to `Flex::measureByLayout`; the height choice cited as `scroll_view.cpp:417-418` is in `doLayout`. `flexGrow` is on the `kScroll` whitelist (`ui_tree_reconciler.cpp:496`) and is applied generically before type dispatch (`:960-962`). **Geometry, recomputed.** `RIGHT_W = 19*14 + 3*13 = 305`. `m_viewportPaddingH = Style::spaceXs = 4` (`scroll_view.h:94`, `style.h:23`), gutter `= scrollbarWidth 6 + scrollbarGap 8 = 14` (`style.h:48-49,24`). `availableW = 305 - 8 = 297`; `contentWidth = 297 - 14 = 283 = LIST_W`; `CHROME_W = 287`. `4 + 283 + 8 + 6 + 4 = 305`. Both constants correct as written. **Row closures are correct under the real hazard.** Every conditional path in `buildList` was walked: `onRow` is built unconditionally per iteration and assigned unconditionally to both click targets; the only conditionals are `tooltip` (not a handler) and the `#out == 0` empty-state label. `i` is fresh per `ipairs` iteration. `m.rows` and `m.matchedRows` are built one-to-one in the same loop, so the captured index addresses the published list. `render` has exactly one caller — `commit` at `panel.luau:402` — and `state.shown` is published at `:401`, before the paint, from the same model object. The cross-render argument holds at the shell too: handlers register into a fresh table per render and the table is swapped wholesale only on a successful root read (`plugin_bindings.cpp:576, 581-582`), and `pushCallback` resolves `@ui:`-prefixed names against the live table only (`luau_host.cpp:3085-3091`) — a miss returns before any result machinery (`script_runtime.cpp:702-705`), silently, as `UPGRADE.md` says. **Cross-boundary, producer to consumer.** A function-valued `onClick` is registered unconditionally by `readUiTreeNode` (`plugin_bindings.cpp:523-524`), regardless of node type, and the prop the tree carries is the generated name string. `onClick` is on both consuming whitelists — `kBox` (`ui_tree_reconciler.cpp:456`) and `kButton` (`:474`) — so neither of the row's two click targets is dropped. No version gate exists on that path. **Caps.** `ALL_WALK_MAX = 200` -> at most 200 scroll children, under `kUiTreeMaxChildren = 256`; tree depth is 5 against `kUiTreeMaxDepth = 32` (`plugin_bindings.cpp:377`). `RECENT_MAX` staying is sound: the chip row is `panelTree.children[1]`, the left column, not inside the scroll. **The deleted-test table.** All 18 rows checked against the file. 17 are honest, including the four I expected to be soft: the `Query.window` predicate row is genuinely strengthened (order and by-reference identity, `test.luau:101-106`); the pager-row-shape row is really replaced by the chrome-outside-the-viewport asserts (`:936-942`); the `onPage1..5` row is really replaced by `clickRow(45)` on a distinct-tag fixture (`:1472-1473`, see finding 2 for its one gap); and the reproduced-bug row is really replaced by 12 rows, `children[13] == nil` and a resolving `clickRow(12)` (`:1494-1503`). `same(m.pager, nil)` does run in all 12 states. The 18th is finding 1. **Gates, re-run at `3dc78b4` in the worktree.** ``` nix shell nixpkgs#luau -c luau test.luau -> viewModel: 12 states, 1606 field comparisons / ok noctalia plugins lint timew -> yuris/timew / ok / 0 errors, 0 warnings grep -cE "ROWS_MAX|PAGE_BUTTONS|listOffset|pageStrip" timew/panel.luau -> 0 ``` The grep claim holds, and a wider sweep (`pageCount|pageOf|gotoPage|onPage|buildPager|m\.pager|page_range`) returns nothing in `panel.luau` either — no pager concept survives under a new name. The only pager residue anywhere is the three translation keys in finding 5. # Not checked - **Live rendering.** I did not re-render the panel; the shell chain above is a source derivation, and finding 4 is where it disagrees with the reported measurement. Everything else in the live comment is consistent with the source. - **Repaint cost at 200 rows.** The list went from 8 rows to up to 200, once a second while tracking — roughly 1000 tree tables and 200 registered closures per paint, against the 25 ms per-handler budget the file cites. The suite's `paintsFor` counts paints, not their cost. I have no measurement either way, so I am not raising it; it is the one thing uncapping bought that nothing in the diff bounds. - **`filterFromRow`'s nil guard** (`panel.luau:1030`) is untested — removing `row and` leaves the suite green — but both the guard and the gap predate this PR (`4deaaf2` `panel.luau:998`; base suite only exercised `onIpc("row", 1)` and `("row", 2)`), so it is out of scope here. `noctalia msg plugin yuris/timew:panel all row 99` is the reachable path if you want it covered. - The `1606` field-comparison count and the 12-state count were taken from the run, not audited assert by assert. # Mutations 26 mutations against lines this PR wrote, in a copy at `/tmp/mut`; every one restored and the tree re-verified byte-identical (`md5sum 1c45dd1e…`, `diff -r` clean). The worktree was never modified. **4 survivors.** | # | Mutation | Result | |---|---|---| | 1 | root row: drop `flexGrow = 1` | killed `:954` | | 2 | scroll: drop `flexGrow = 1` | killed `:952` | | 3 | scroll: add `height = 200` | killed `:953` | | 4 | `ui.scroll` -> `ui.column` | killed `:940` | | 5 | dot loses its closure | killed `:1284` (compare) | | 6 | every closure captures `1` | killed `:1473` | | 7 | **only row 5 captures a stale index** | **SURVIVED** -> finding 2 | | 8 | no-predicate path copies instead of returning by reference | killed `:101` | | 9 | `Query.matching` re-caps at 8 | killed `:107` | | 10 | `commit` stops publishing `state.shown` | killed `:915` | | 11 | rows pinned to `RIGHT_W` | killed `:967` | | 12 | scope row back to `RIGHT_W` | killed `:967` | | 12b | **search row back to `RIGHT_W`** | **SURVIVED** -> finding 3 | | 13 | scroll `width = CHROME_W` | killed `:974` | | 14 | `ALL_WALK_MAX` 200 -> 300 | killed `:349` (argv literal, *not* `:130`) | | 14b | 14 + the `:351`/`:354` literals updated | **SURVIVED** -> finding 1 | | 15 | `filterFromRow` drops its nil guard | **SURVIVED** (pre-existing, see Not checked) | | 16 | search row moved inside the viewport | killed `:929` | | 17 | scroll `gap = 1` -> `gap = 6` | **SURVIVED** (cosmetic; no assert warranted) | | 18 | empty-state label dropped from `buildList` | killed `:996` | | 19 | `commit` publishes a copy, not the model's table | killed `:1469` | | 20 | only the first 8 rows get a closure | killed `:928` | | 21 | tag button loses its closure, dot keeps it | killed `:851` | | 22 | chrome order swapped (scope above search) | killed `:938` | | 23 | `rows` built from `state.dayRows`, not `matchedRows` | killed `:851` | | 24 | `Query.matching` reverses order | killed `:106` | | 25 | `commit` publishes a second `viewModel()` call | killed `:1599` | The kill set is genuinely strong — 22 of 26, including every layout prop, both closure targets, the publication link and the chrome/viewport split. The three actionable survivors are all in *new* coverage this PR added, and two of them (2 and 3) are one edit each against fixtures that already exist. 
test.luau:130 was `assert(200 < 256, ...)`. Both operands are literals, so it
held nothing: ALL_WALK_MAX is a module local with no export and the assert never
read it. Proven -- ALL_WALK_MAX = 200 -> 300 plus the one-line repair to the
argv literal that does fire (:349) returned the suite to green while the shell
silently dropped 44 rows past kUiTreeMaxChildren. Two comments claimed that line
was the guard; both said something untrue and are corrected here.

The replacement reads the bound out of the walk script Timew.cmd.allIntervals()
builds, which is the one place the constant's value is observable from the
suite, and sits where T is in scope.

Also:

- clickRow over all 45 rows, not just row 45. The fixture was built with a
  distinct tag per row so a captured index is observable; only the last row was
  spending it, and a stale index on any interior row survived.
- The search row's width is pinned to the scope row's. It moved from RIGHT_W to
  CHROME_W in this branch and nothing read it back, so reverting it survived.
- panel.newer, panel.older and panel.page_range deleted from en.json and from
  the suite's EN mirror. Their only consumer was buildPager.
- The tr() miss message named en.json, which it cannot see; it names the copy it
  actually reads.
- The scrollbar paragraph on LIST_W/CHROME_W: `m_scrollbar->setVisible` makes the
  container visible, but track, thumb and both hit areas are gated on
  contentExtent > viewportExtent (scrollbar.cpp:117-121), so a short list paints
  an empty gutter rather than a bar over a row. The claim in 3dc78b4's message
  and in the source comment was wrong; the gutter-jump argument beside it is the
  true one and is what the constants rest on. The measured thumb position is
  dropped, being unverifiable from source.
- Stale citations: the function-valued prop is registered at
  plugin_bindings.cpp:523-524 (registerUiHandler 463-471), not 568-586; uiNodePath
  is 454-459, not 450-458. The onRowN reference above clickedProp is gone.

Every added or changed assert was mutation-proved: each fails on the mutation it
names, with its own message, and the tree restores byte-identically.
yuris commented 2026-09-01 13:47:58 +00:00
Author
Owner
Copy link

Review actioned — be4208e

The blocking finding is right and I reproduced it independently before fixing it.
Everything below is quoted from runs, not restated.

Finding 1 (blocking) — accepted, and re-proved from scratch

Reproduced on a pristine copy of 3dc78b4, ALL_WALK_MAX = 200 -> 300 with the
one-line argv repair any author would make when :349 fires:

=== control: HEAD suite, unmutated
viewModel: 12 states, 1606 field comparisons
ok

=== HEAD suite under ALL_WALK_MAX 200->300 + the one-line argv repair
viewModel: 12 states, 1606 field comparisons
ok
^^ SURVIVED: line 130 (200 < 256) never fired

test.luau:130 is deleted. Your replacement is in, at :364-365, where T is in
scope. Both source comments that claimed :130 held the pair are corrected:
panel.luau:79-81 now says the suite reads the bound back out of the walk script,
and the test.luau:118-125 comment says the check is not in that scope and points
at where it is. The PR body's two sentences are corrected too, and the correction
says plainly that the coupling was not asserted when the PR was opened.

One deliberate keep: your < 256 rather than <= 256. The shell keeps
std::min(objlen, 256) children and only warns above 256, so exactly 256 is legal
and the assert is one value stricter than the shell. The margin is wanted — the
scroll's children are the rows today, and a sibling added inside the viewport
later would eat it — and the message ("at or over") matches the condition exactly.

Finding 2 — accepted

All 45 rows clicked. test.luau:1484-1491.

Finding 3 — accepted

rightKids[4] pinned to chromeW. test.luau:981-984.

Finding 4 — accepted, and the commit message it rests on

Confirmed at source: scrollbar.cpp:117-121 gates m_track, m_thumb,
m_trackArea and m_thumbArea on contentExtent > viewportExtent + 0.5F, so a
short list paints an empty gutter, not a bar over a row. The
"painted whether the list scrolls or not" claim is gone from panel.luau:36-47;
the gutter-jump argument (scroll_view.cpp:429) is kept, since it is the one that
actually pins LIST_W/CHROME_W. The unverifiable x=1221..1226 thumb
measurement is dropped; the x=1212 edge measurement and the arithmetic stay.

3dc78b4's message carries the same wrong claim. I did not rewrite it: this repo
squash-merges with an empty body (4deaaf2 on main is title-only), so the claim
never enters main's history, and force-pushing would dangle the hash this review
cites. The durable artifact — the source comment — is fixed.

Finding 5 — accepted, all three keys

Not just page_range. Deleted from en.json and from the suite's EN copy.
Confirmed no consumer survives:

$ rg -n 'panel\.newer|panel\.older|panel\.page_range|page_range' . -g '!*.json'
(no matches)

The PR body's backwards accounting is rewritten.

Finding 6 — accepted

  • clickedProp's comment no longer claims onRowN exists.
  • plugin_bindings.cpp:568-586 → 523-524 (registerUiHandler at 463-471).
  • plugin_bindings.cpp:450-458 → 454-459. Note your own correction is off by
    the same shape: uiNodePath is 454-459, not 459-463.
  • Line counts recomputed against the final tree: panel.luau −82, test.luau
    +71, en.json −3, net −14.

The sweep you asked for: are there other asserts of that shape?

Literal-vs-literal: one, and it was yours. Every assert( in the file was
extracted with a balanced-paren scan (245 of them) and each condition classified
by whether any identifier it reads is live rather than a local bound to a literal:

consts:  SUMMARY PROSE width hostReads ROW DOM INSET GUTTER DAYS states checks
         stateName D3 DAY3 ALL4 coreCalls paints
flagged: [ { line: 130, cond: "200 < 256", base: [] } ]

A second pass over every literal comparison anywhere in the file (not just inside
assert) returns the same one line. So :130 was alone in that exact shape.

"Cannot observe what its message names": one more, now fixed. Every assert
whose message names an identifier or a multi-digit number the condition does not
mention was listed and each checked. All but one are legitimate — the message
names the mechanism and the condition observes its effect — and I mutation-proved
the three that name a module local, since that is the failure mode :130 had:

Mutation Result
RECENT_MAX 4 → 6 killed :830 fetchTags caps the chip row at RECENT_MAX: got 6 chips for six tags
VIEWPORT_INSET 4 → 6 killed :980 the chrome is one viewport inset wider than a row: 285 vs 279
SCROLLBAR_GUTTER 14 → 20 killed :991 the scroll is the chart's full width with the inset and gutter inside it: 305

So :236's deferral ("RECENT_MAX is the caller's job") is honest, and the
INSET/GUTTER literals at :976 are real mirrors, not restatements.

The one real instance is tr, test.luau:1072. Its message read
"en.json has no key " .. key — but tr reads the EN copy above it, and the
luau CLI has no io, so nothing in this file can see en.json at all. A key
present in the copy and absent from the file passes. That is exactly what let
finding 5 sit there: three keys were dead in both, and both gates were green. The
copy is the only thing it can name, so it names it, and the drift possibility is
now stated in the message rather than papered over by it.

Vacuous-loop shape, checked and clear. Every for wrapping an assert (17 of
them) was listed; each iterates a literal table or a collection whose length is
pinned by an assert immediately above it, so none can pass by iterating zero
times.

Mutations — every assert added or changed

Each mutation is applied in place, the suite run, the tree restored from a
pre-mutation copy and the restore verified by md5sum before the next one.

# Mutation Line Failure
1 ALL_WALK_MAX 200 → 300 + the :351/:354 argv literals repaired (your 14b) :365 ALL_WALK_MAX 300 is at or over the shell per-node child cap, so the tail of the list is dropped unrendered
2 the %-le (%d+) %] capture stops matching :365 ALL_WALK_MAX nil is at or over the shell per-node child cap, …
3 buildSearch → width = RIGHT_W (your 12b) :984 the search row shares the scope row's width: 305 vs 287
4 buildList → filterFromRow(if i == 5 then 1 else i) (your 7) :1490 row 5 closure filters by its own tag, got tag01
5 same, at i == 23 — an interior row your mutation did not name :1490 row 23 closure filters by its own tag, got tag01
6 EN["panel.untagged"] renamed :1072 the EN copy above has no key panel.untagged -- render asked for one no locale defines, or the copy has drifted from en.json

Mutation 2 matters because the new assert is two lines: without it the tonumber
line could rot into a permanent nil and the walkCap ~= nil half would be the
only thing standing. It fires, and the message reads sensibly with nil in it.

Gates

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

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

Worktree clean; the mutation harness was a throwaway and is not in the tree.

Not actioned, on purpose

  • filterFromRow's nil guard. Agreed out of scope — it predates this branch
    (4deaaf2), and covering it needs the onIpc("row", 99) path, which is a
    different change than the one under review.
  • Repaint cost at 200 rows. No measurement either way, and nothing in this
    diff makes it worse than the merge did. Left as your "not checked".
  • gap = 1 → gap = 6 surviving. Cosmetic, no assert warranted, agreed.
## Review actioned — `be4208e` The blocking finding is right and I reproduced it independently before fixing it. Everything below is quoted from runs, not restated. ### Finding 1 (blocking) — accepted, and re-proved from scratch Reproduced on a pristine copy of `3dc78b4`, `ALL_WALK_MAX = 200 -> 300` with the one-line argv repair any author would make when `:349` fires: ``` === control: HEAD suite, unmutated viewModel: 12 states, 1606 field comparisons ok === HEAD suite under ALL_WALK_MAX 200->300 + the one-line argv repair viewModel: 12 states, 1606 field comparisons ok ^^ SURVIVED: line 130 (200 < 256) never fired ``` `test.luau:130` is deleted. Your replacement is in, at `:364-365`, where `T` is in scope. Both source comments that claimed `:130` held the pair are corrected: `panel.luau:79-81` now says the suite reads the bound back out of the walk script, and the `test.luau:118-125` comment says the check is not in that scope and points at where it is. The PR body's two sentences are corrected too, and the correction says plainly that the coupling was *not* asserted when the PR was opened. One deliberate keep: your `< 256` rather than `<= 256`. The shell keeps `std::min(objlen, 256)` children and only warns above 256, so exactly 256 is legal and the assert is one value stricter than the shell. The margin is wanted — the scroll's children are the rows today, and a sibling added inside the viewport later would eat it — and the message ("at or over") matches the condition exactly. ### Finding 2 — accepted All 45 rows clicked. `test.luau:1484-1491`. ### Finding 3 — accepted `rightKids[4]` pinned to `chromeW`. `test.luau:981-984`. ### Finding 4 — accepted, and the commit message it rests on Confirmed at source: `scrollbar.cpp:117-121` gates `m_track`, `m_thumb`, `m_trackArea` and `m_thumbArea` on `contentExtent > viewportExtent + 0.5F`, so a short list paints an empty gutter, not a bar over a row. The "painted whether the list scrolls or not" claim is gone from `panel.luau:36-47`; the gutter-jump argument (`scroll_view.cpp:429`) is kept, since it is the one that actually pins `LIST_W`/`CHROME_W`. The unverifiable `x=1221..1226` thumb measurement is dropped; the `x=1212` edge measurement and the arithmetic stay. `3dc78b4`'s message carries the same wrong claim. I did not rewrite it: this repo squash-merges with an empty body (`4deaaf2` on `main` is title-only), so the claim never enters `main`'s history, and force-pushing would dangle the hash this review cites. The durable artifact — the source comment — is fixed. ### Finding 5 — accepted, all three keys Not just `page_range`. Deleted from `en.json` and from the suite's `EN` copy. Confirmed no consumer survives: ``` $ rg -n 'panel\.newer|panel\.older|panel\.page_range|page_range' . -g '!*.json' (no matches) ``` The PR body's backwards accounting is rewritten. ### Finding 6 — accepted - `clickedProp`'s comment no longer claims `onRowN` exists. - `plugin_bindings.cpp:568-586` → `523-524` (`registerUiHandler` at `463-471`). - `plugin_bindings.cpp:450-458` → `454-459`. Note your own correction is off by the same shape: `uiNodePath` is `454-459`, not `459-463`. - Line counts recomputed against the final tree: `panel.luau` −82, `test.luau` +71, `en.json` −3, net **−14**. --- ## The sweep you asked for: are there other asserts of that shape? **Literal-vs-literal: one, and it was yours.** Every `assert(` in the file was extracted with a balanced-paren scan (245 of them) and each condition classified by whether any identifier it reads is live rather than a local bound to a literal: ``` consts: SUMMARY PROSE width hostReads ROW DOM INSET GUTTER DAYS states checks stateName D3 DAY3 ALL4 coreCalls paints flagged: [ { line: 130, cond: "200 < 256", base: [] } ] ``` A second pass over every literal comparison anywhere in the file (not just inside `assert`) returns the same one line. So `:130` was alone in that exact shape. **"Cannot observe what its message names": one more, now fixed.** Every assert whose message names an identifier or a multi-digit number the condition does not mention was listed and each checked. All but one are legitimate — the message names the mechanism and the condition observes its effect — and I mutation-proved the three that name a module local, since that is the failure mode `:130` had: | Mutation | Result | |---|---| | `RECENT_MAX` 4 → 6 | killed `:830` `fetchTags caps the chip row at RECENT_MAX: got 6 chips for six tags` | | `VIEWPORT_INSET` 4 → 6 | killed `:980` `the chrome is one viewport inset wider than a row: 285 vs 279` | | `SCROLLBAR_GUTTER` 14 → 20 | killed `:991` `the scroll is the chart's full width with the inset and gutter inside it: 305` | So `:236`'s deferral ("`RECENT_MAX` is the caller's job") is honest, and the `INSET`/`GUTTER` literals at `:976` are real mirrors, not restatements. The one real instance is `tr`, `test.luau:1072`. Its message read `"en.json has no key " .. key` — but `tr` reads the `EN` copy above it, and the luau CLI has no `io`, so nothing in this file can see `en.json` at all. A key present in the copy and absent from the file passes. That is exactly what let finding 5 sit there: three keys were dead in both, and both gates were green. The copy is the only thing it can name, so it names it, and the drift possibility is now stated in the message rather than papered over by it. **Vacuous-loop shape, checked and clear.** Every `for` wrapping an assert (17 of them) was listed; each iterates a literal table or a collection whose length is pinned by an assert immediately above it, so none can pass by iterating zero times. --- ## Mutations — every assert added or changed Each mutation is applied in place, the suite run, the tree restored from a pre-mutation copy and the restore verified by `md5sum` before the next one. | # | Mutation | Line | Failure | |---|---|---|---| | 1 | `ALL_WALK_MAX` 200 → 300 + the `:351`/`:354` argv literals repaired (your 14b) | `:365` | `ALL_WALK_MAX 300 is at or over the shell per-node child cap, so the tail of the list is dropped unrendered` | | 2 | the `%-le (%d+) %]` capture stops matching | `:365` | `ALL_WALK_MAX nil is at or over the shell per-node child cap, …` | | 3 | `buildSearch` → `width = RIGHT_W` (your 12b) | `:984` | `the search row shares the scope row's width: 305 vs 287` | | 4 | `buildList` → `filterFromRow(if i == 5 then 1 else i)` (your 7) | `:1490` | `row 5 closure filters by its own tag, got tag01` | | 5 | same, at `i == 23` — an interior row your mutation did not name | `:1490` | `row 23 closure filters by its own tag, got tag01` | | 6 | `EN["panel.untagged"]` renamed | `:1072` | `the EN copy above has no key panel.untagged -- render asked for one no locale defines, or the copy has drifted from en.json` | Mutation 2 matters because the new assert is two lines: without it the `tonumber` line could rot into a permanent `nil` and the `walkCap ~= nil` half would be the only thing standing. It fires, and the message reads sensibly with `nil` in it. ## Gates ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 12 states, 1606 field comparisons ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings ``` Worktree clean; the mutation harness was a throwaway and is not in the tree. ## Not actioned, on purpose - **`filterFromRow`'s nil guard.** Agreed out of scope — it predates this branch (`4deaaf2`), and covering it needs the `onIpc("row", 99)` path, which is a different change than the one under review. - **Repaint cost at 200 rows.** No measurement either way, and nothing in this diff makes it worse than the merge did. Left as your "not checked". - **`gap = 1` → `gap = 6` surviving.** Cosmetic, no assert warranted, agreed.
yuris merged commit 585a58730a into main 2026-09-01 13:48:09 +00:00
yuris deleted branch upgrade/u2-scroll 2026-09-01 13:48:10 +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!12
No description provided.