yuris/noctalia-timew

Fork 0

refactor(panel): generate the numbered click handlers from a table #4

Merged

yuris merged 2 commits from refactor/t1-handlers into main

2026-08-31 17:36:38 +00:00

yuris commented

2026-08-31 17:15:31 +00:00

Owner

Wave 2 / T1. timew/panel.luau only.

What changed

The 43 numbered click-handler globals were literal one-liners in four blocks, so
every list cap was spelled out in code twice - once as a constant, once as the
last handler in the block. They are now one HANDLERS list of
{ prefix, count, fn } and one loop:

local HANDLERS = {
	{ prefix = "onTag", count = RECENT_MAX, fn = startRecent },
	{ prefix = "onDay", count = DAYS, fn = loadDay },
	{ prefix = "onRow", count = ROWS_MAX, fn = filterFromRow },
	{ prefix = "onPage", count = PAGE_BUTTONS, fn = function(i) gotoPage(pageBase + i - 1) end },
}

local globals = getfenv(0)
for _, family in ipairs(HANDLERS) do
	for i = 1, family.count do
		globals[family.prefix .. i] = function() family.fn(i) end
	end
end

grep -c '^function on' timew/panel.luau: 56 -> 13, a drop of exactly 43. The
generator is the only definition site for the four numbered families. Nothing
else moved: no rewire onto Timew/Fmt/Parse/Query, no change to render,
state, or test.luau. 25 insertions, 47 deletions.

`_G` does not work here, `getfenv(0)` does

The task asked for this to be verified rather than assumed, and _G loses.

luau_host.cpp:1867-1870 calls luaL_sandbox(m_L) on the main state and then
luaL_sandboxthread(m_T) on the plugin thread. luaL_sandboxthread gives the
thread a fresh globals table whose metatable __index is the frozen parent, and
it does not set a _G field on that new table - so the name _G falls
through to the parent's _G, which points at the frozen parent itself.

Probed under the luau CLI, which sandboxes identically (luaL_sandbox on the
global state, luaL_sandboxthread on the run thread):

getfenv type:	function
_G type:	table
_G write:	false	attempt to modify a readonly table
getfenv(0) write:	true
getfenv(1) write:	true

getfenv(0) is the thread's own globals table - which is both where a plain
function onDay1() statement lands and, decisively, the exact table the shell's
handler lookup reads: LuauHost::pushCallback does lua_getglobal(m_T, name)
(luau_host.cpp:3079). So the generated names are visible on precisely the path
the shell uses. Neither getfenv nor setfenv is stripped by the host (the only
lua_setglobal calls near the sandbox are additions; there is no global-removal
list), and adding fresh keys cannot disturb the lua_setsafeenv import cache
because no handler name is ever an import.

Generation runs at chunk load, before the shell can dispatch anything.

`onRow`: 8 generated, not 20

Deliberate: ROWS_MAX, giving 8 names where the literals reached 20.

The proof that no onClick string can name a missing handler is in
activityRows. It fills shown under local cap = ROWS_MAX with
if matching > listOffset and #shown < cap, so #shown <= ROWS_MAX. It then
builds the click targets with for i, row in ipairs(shown) and
onClick = "onRow" .. i (both the dot box and the ghost tag button), so i is
bounded by #shown <= ROWS_MAX. onRow9 is a string the panel cannot produce.
The count now tracks the constant, so raising ROWS_MAX raises the handler count
with it - which the literal block did not do (it is why the block was stuck at
20 while the live cap moved to 8).

The other three are bounded the same way: recentButtons iterates
ipairs(recent) and fetchTags caps #recent < RECENT_MAX; the day bars run
for i = math.min(DAYS, #days), 1, -1; the pager emits
"onPage" .. (p - firstShown + 1) with
lastShown = math.min(pages, firstShown + PAGE_BUTTONS - 1).

`pageBase` is still read at click time

pageBase is a module-level local, so the generated closure body
gotoPage(pageBase + i - 1) reads it through an upvalue when the click arrives,
exactly as function onPage2() gotoPage(pageBase + 1) end did. Only i is
captured. render assigns pageBase = firstShown on every pass, and the strip
slides, so a base captured at generation time would page to a stale window - the
comment records that.

Comments

The API-3 comment above startRecent is kept and extended: it still records why
handlers are looked up by name (a click cannot carry its value at this API) and
now points at HANDLERS as the generation site. Two new comments carry the
ROWS_MAX reasoning and the _G-vs-getfenv(0) finding.

Static proof

Throwaway script, requires panel.luau and prints type(...) for every live
name plus one past the end of each family. Because a required module gets its own
global table, it reaches panel's table through the env of a function panel
exports - the same table the generator wrote to.

onTag1=function  onTag2=function  onTag3=function  onTag4=function  onTag5=nil
onDay1=function  onDay2=function  onDay3=function  onDay4=function  onDay5=function  onDay6=function  onDay7=function  onDay8=function  onDay9=function  onDay10=function  onDay11=function  onDay12=function  onDay13=function  onDay14=function  onDay15=nil
onRow1=function  onRow2=function  onRow3=function  onRow4=function  onRow5=function  onRow6=function  onRow7=function  onRow8=function  onRow9=nil
onPage1=function  onPage2=function  onPage3=function  onPage4=function  onPage5=function  onPage6=nil

A function for all 31 live names, nil one past each end. The script is not
committed.

Gate

$ nix shell nixpkgs#luau -c luau test.luau
ok

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

$ git diff --stat main...HEAD
 timew/panel.luau | 72 ++++++++++++++++++++------------------------------------
 1 file changed, 25 insertions(+), 47 deletions(-)

plugin_lint.cpp only cross-checks declared-vs-read settings keys, require paths
and entry files - it has no handler-string check - so there was no new warning
class to trip. Runtime verification (reload, open panel, IPC-drive the click
paths) is the integrator's.

Generated handler names, for spot-checking against the `onClick` strings

onTag1 .. onTag4 -> startRecent(i) (RECENT_MAX = 4); built at
onClick = "onTag" .. i in recentButtons
onDay1 .. onDay14 -> loadDay(i) (DAYS = 14); built at
onClick = "onDay" .. i, twice per bar (hit box and bar)
onRow1 .. onRow8 -> filterFromRow(i) (ROWS_MAX = 8); built at
onClick = "onRow" .. i, twice per row (dot and tag button)
onPage1 .. onPage5 -> gotoPage(pageBase + i - 1) (PAGE_BUTTONS = 5);
built at onClick = "onPage" .. (p - firstShown + 1)

Untouched hand-written handlers (13): onClearFilter, onScopeDay,
onScopeAll, onPagePrev, onPageNext, onSearchChange, onSearchSubmit,
onDraftChange, onDraftSubmit, onStop, onStartDraft, onOpen, onIpc.

Wave 2 / T1. `timew/panel.luau` only. ## What changed The 43 numbered click-handler globals were literal one-liners in four blocks, so every list cap was spelled out in code twice - once as a constant, once as the last handler in the block. They are now one `HANDLERS` list of `{ prefix, count, fn }` and one loop: ```lua local HANDLERS = { { prefix = "onTag", count = RECENT_MAX, fn = startRecent }, { prefix = "onDay", count = DAYS, fn = loadDay }, { prefix = "onRow", count = ROWS_MAX, fn = filterFromRow }, { prefix = "onPage", count = PAGE_BUTTONS, fn = function(i) gotoPage(pageBase + i - 1) end }, } local globals = getfenv(0) for _, family in ipairs(HANDLERS) do for i = 1, family.count do globals[family.prefix .. i] = function() family.fn(i) end end end ``` `grep -c '^function on' timew/panel.luau`: 56 -> 13, a drop of exactly 43. The generator is the only definition site for the four numbered families. Nothing else moved: no rewire onto `Timew`/`Fmt`/`Parse`/`Query`, no change to `render`, state, or `test.luau`. 25 insertions, 47 deletions. ## `_G` does not work here, `getfenv(0)` does The task asked for this to be verified rather than assumed, and `_G` loses. `luau_host.cpp:1867-1870` calls `luaL_sandbox(m_L)` on the main state and then `luaL_sandboxthread(m_T)` on the plugin thread. `luaL_sandboxthread` gives the thread a fresh globals table whose metatable `__index` is the frozen parent, and it does **not** set a `_G` field on that new table - so the name `_G` falls through to the parent's `_G`, which points at the frozen parent itself. Probed under the `luau` CLI, which sandboxes identically (`luaL_sandbox` on the global state, `luaL_sandboxthread` on the run thread): ``` getfenv type: function _G type: table _G write: false attempt to modify a readonly table getfenv(0) write: true getfenv(1) write: true ``` `getfenv(0)` is the thread's own globals table - which is both where a plain `function onDay1()` statement lands and, decisively, the exact table the shell's handler lookup reads: `LuauHost::pushCallback` does `lua_getglobal(m_T, name)` (`luau_host.cpp:3079`). So the generated names are visible on precisely the path the shell uses. Neither `getfenv` nor `setfenv` is stripped by the host (the only `lua_setglobal` calls near the sandbox are additions; there is no global-removal list), and adding fresh keys cannot disturb the `lua_setsafeenv` import cache because no handler name is ever an import. Generation runs at chunk load, before the shell can dispatch anything. ## `onRow`: 8 generated, not 20 Deliberate: `ROWS_MAX`, giving 8 names where the literals reached 20. The proof that no `onClick` string can name a missing handler is in `activityRows`. It fills `shown` under `local cap = ROWS_MAX` with `if matching > listOffset and #shown < cap`, so `#shown <= ROWS_MAX`. It then builds the click targets with `for i, row in ipairs(shown)` and `onClick = "onRow" .. i` (both the dot box and the ghost tag button), so `i` is bounded by `#shown <= ROWS_MAX`. `onRow9` is a string the panel cannot produce. The count now tracks the constant, so raising `ROWS_MAX` raises the handler count with it - which the literal block did not do (it is why the block was stuck at 20 while the live cap moved to 8). The other three are bounded the same way: `recentButtons` iterates `ipairs(recent)` and `fetchTags` caps `#recent < RECENT_MAX`; the day bars run `for i = math.min(DAYS, #days), 1, -1`; the pager emits `"onPage" .. (p - firstShown + 1)` with `lastShown = math.min(pages, firstShown + PAGE_BUTTONS - 1)`. ## `pageBase` is still read at click time `pageBase` is a module-level `local`, so the generated closure body `gotoPage(pageBase + i - 1)` reads it through an upvalue when the click arrives, exactly as `function onPage2() gotoPage(pageBase + 1) end` did. Only `i` is captured. `render` assigns `pageBase = firstShown` on every pass, and the strip slides, so a base captured at generation time would page to a stale window - the comment records that. ## Comments The API-3 comment above `startRecent` is kept and extended: it still records why handlers are looked up by name (a click cannot carry its value at this API) and now points at `HANDLERS` as the generation site. Two new comments carry the `ROWS_MAX` reasoning and the `_G`-vs-`getfenv(0)` finding. ## Static proof Throwaway script, `require`s `panel.luau` and prints `type(...)` for every live name plus one past the end of each family. Because a required module gets its own global table, it reaches panel's table through the env of a function panel exports - the same table the generator wrote to. ``` onTag1=function onTag2=function onTag3=function onTag4=function onTag5=nil onDay1=function onDay2=function onDay3=function onDay4=function onDay5=function onDay6=function onDay7=function onDay8=function onDay9=function onDay10=function onDay11=function onDay12=function onDay13=function onDay14=function onDay15=nil onRow1=function onRow2=function onRow3=function onRow4=function onRow5=function onRow6=function onRow7=function onRow8=function onRow9=nil onPage1=function onPage2=function onPage3=function onPage4=function onPage5=function onPage6=nil ``` A function for all 31 live names, `nil` one past each end. The script is not committed. ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ git diff --stat main...HEAD timew/panel.luau | 72 ++++++++++++++++++++------------------------------------ 1 file changed, 25 insertions(+), 47 deletions(-) ``` `plugin_lint.cpp` only cross-checks declared-vs-read settings keys, require paths and entry files - it has no handler-string check - so there was no new warning class to trip. Runtime verification (reload, open panel, IPC-drive the click paths) is the integrator's. ## Generated handler names, for spot-checking against the `onClick` strings - `onTag1` .. `onTag4` -> `startRecent(i)` (`RECENT_MAX` = 4); built at `onClick = "onTag" .. i` in `recentButtons` - `onDay1` .. `onDay14` -> `loadDay(i)` (`DAYS` = 14); built at `onClick = "onDay" .. i`, twice per bar (hit box and bar) - `onRow1` .. `onRow8` -> `filterFromRow(i)` (`ROWS_MAX` = 8); built at `onClick = "onRow" .. i`, twice per row (dot and tag button) - `onPage1` .. `onPage5` -> `gotoPage(pageBase + i - 1)` (`PAGE_BUTTONS` = 5); built at `onClick = "onPage" .. (p - firstShown + 1)` Untouched hand-written handlers (13): `onClearFilter`, `onScopeDay`, `onScopeAll`, `onPagePrev`, `onPageNext`, `onSearchChange`, `onSearchSubmit`, `onDraftChange`, `onDraftSubmit`, `onStop`, `onStartDraft`, `onOpen`, `onIpc`.

yuris added 1 commit

2026-08-31 17:15:31 +00:00

refactor(panel): generate the numbered click handlers from a table cc0178fb43

The 43 onTag/onDay/onRow/onPage globals were literal one-liners, so every
cap was spelled out in code. They are now one HANDLERS list of
{ prefix, count, fn } and one loop, and a cap is data.

onRow generates ROWS_MAX (8) names rather than the 20 the literals
reached: activityRows caps `shown` at ROWS_MAX and numbers the buttons
over ipairs(shown), so render cannot build a string past it.

Assignment goes through getfenv(0), not _G: under luaL_sandbox the
plugin thread inherits reads from a frozen parent table and `_G`
resolves to that parent, so `_G[name] = fn` raises "attempt to modify a
readonly table". getfenv(0) is the thread's own globals, which is both
where a plain `function onDay1()` lands and what the shell's
lua_getglobal(m_T, name) handler lookup reads.

yuris reviewed

2026-08-31 17:30:44 +00:00

yuris left a comment

Verdict: REQUEST_CHANGES

The refactor is functionally correct. I could not construct a dead click on any path, the onRow narrowing is safe, pageBase is still read at click time, and the diff is clean. All of that I re-derived rather than took on trust, and it all holds.

What blocks the merge is the mechanism. getfenv(0) permanently disables safeenv for this plugin's globals table, which is the one thing the PR body says it does not do. That is a measured ~2.4x on import-bound work in a 1448-line UI module, spent against a hard 25 ms per-handler CPU budget, to save 22 source lines.

Findings

1. `getfenv(0)` disables `safeenv` for the plugin's whole lifetime — P1

timew/panel.luau:946

The PR body argues: "adding fresh keys cannot disturb the lua_setsafeenv import cache because no handler name is ever an import." The premise is true and the conclusion does not follow, because the deoptimisation does not come from the writes. It comes from the getfenv call itself:

// third_party/luau/VM/src/lbaselib.cpp:124-133
static int luaB_getfenv(lua_State* L)
{
    getfunc(L, 1);
    if (lua_iscfunction(L, -1))
        lua_pushvalue(L, LUA_GLOBALSINDEX);
    else
        lua_getfenv(L, -1);
    lua_setsafeenv(L, -1, false);   // <-- unconditional
    return 1;
}

With getfenv(0), getfunc resolves level 0 to the running chunk and lua_getfenv pushes that chunk's env — m_T->gt, the plugin's own globals. lua_setsafeenv(..., false) then lands on exactly that table. Nothing in Lua can set the flag back, so it is off for the rest of the plugin's life. That gates both GETIMPORT (every ui.*, noctalia.*, math.*, table.*, string.* read) and builtin fastcall, throughout render, activityRows, dayColumns and the pager.

Isolated to the single line — same table writes, same workload, only getfenv(0) vs {} differing:

no getfenv (control)      17.5 ms
getfenv(0) called         40.7 ms
no getfenv (control)      16.4 ms
getfenv(0) called         42.2 ms

Hoisting the hot globals into locals is not a way out; builtin fastcall is gated on the same flag:

hoisted, safeenv ON               16.1 ms
hoisted, safeenv OFF              27.7 ms

Why it is worth a block rather than a note: every click handler runs under a hard CPU budget, and render() runs inside it.

// luau_host.cpp:3099
bool LuauHost::callGlobal(const char* name) { return callGlobalWithBudget(name, std::chrono::milliseconds(25)); }

beginBudget/budgetDeadlineCrossed (luau_host.cpp:2833, 2656) abort the call when it is crossed. main calls no getfenv at all, so this headroom is spent by the patch. I did not measure render() itself — that needs the live host, which is out of scope for a review — so I am reporting the multiplier, not an absolute. The multiplier is enough: it is permanent, module-wide, and was costed at zero.

setfenv is not an escape hatch; it clears the same flag (lbaselib.cpp:140).

There is no third way in pure Lua. Under luaL_sandboxthread a runtime-computed global name requires a reference to the globals table, and getfenv/setfenv are the only routes to it. So the choice is:

(a) Merge-now fix. Revert to the literal function onTagN() / onDayN() / onRowN() / onPageN() statements. SETGLOBAL writes to m_T->gt without ever materialising the table, so it costs nothing. Keep the win the PR was actually after — killing the cap duplication — with an assertion in test.luau instead of a runtime generator:

-- The numbered handler families are the panel's click surface: a name a
-- producer can build but the module does not define is a silent dead click.
-- The caps are spelled twice (constant and last handler), so pin them here.
local g = getfenv(panel.parseDuration)
for prefix, cap in pairs({ onTag = 4, onDay = 14, onRow = 8, onPage = 5 }) do
	for i = 1, cap do
		assert(type(g[prefix .. i]) == "function", prefix .. i .. " is missing")
	end
	assert(g[prefix .. (cap + 1)] == nil, prefix .. (cap + 1) .. " outlives its cap")
end

That is the cap-as-data property with none of the runtime cost, and it fails loudly on exactly the drift the generator was built to prevent. Note getfenv in a throwaway test process is free; the objection is only to it running inside the shipped plugin.

(b) The durable fix. Move the panel to plugin_api = 4. A function-valued handler prop is registered directly:

// plugin_bindings.cpp:523-524
if (lua_isfunction(L, -1)) {
  out.props.emplace(std::string(propName), registerUiHandler(L, read, path, propName));
}

and dispatched through @ui_handlers (luau_host.cpp:3084-3090, ui_handler_table.h:18-19). That deletes the numbered handlers, the caps-to-handler-count coupling, the generator and getfenv in one move — the entire problem T1 was assigned. The installed shell is v5.0.0 and supports API 4. Out of scope for this PR, but it is where this should go, and it is worth knowing before this pattern gets copied into bar.luau, whose timer runs far hotter than the panel's.

2. `HANDLERS` is a one-consumer data structure — P3

timew/panel.luau:931-951

Ruling on the weight question, since the task asked for one explicitly: the four-entry table is heavier than what it does. It is read in exactly one place, 16 lines below its own definition, and it buys nothing a helper does not. Adding a family costs one line either way. It also costs a family.prefix / family.count / family.fn hash lookup where a helper has upvalues, including family.fn on every click.

12 code lines become 10, one nesting level disappears, and the three field names per entry stop being written out:

-- One call per numbered family. The `count` argument is the cap that bounds
-- the `onClick` strings `render` can build, so no click can name a handler
-- that is absent. Each `fn` is resolved now, so it must already be assigned
-- above this point.
local globals = getfenv(0)
local function generate(prefix, count, fn)
	for i = 1, count do
		globals[prefix .. i] = function() fn(i) end
	end
end

generate("onTag", RECENT_MAX, startRecent)
generate("onDay", DAYS, loadDay)
-- ROWS_MAX, not the 20 the literals used to reach: `activityRows` stops
-- filling `shown` at ROWS_MAX and numbers the buttons over `ipairs(shown)`,
-- so an index past ROWS_MAX is a string the panel cannot produce.
generate("onRow", ROWS_MAX, filterFromRow)
-- `pageBase` is read when the click arrives, not when the handler is made:
-- the strip slides, and a captured base would page to a stale window.
generate("onPage", PAGE_BUTTONS, function(i) gotoPage(pageBase + i - 1) end)

Moot if finding 1 is resolved by (a).

3. `HANDLERS` snapshots `fn` by value, adding a silent ordering constraint — P3

timew/panel.luau:933

loadDay is a forward-declared local (:337) assigned at :756. Line 933 reads its value, so the table holds a snapshot. The literal it replaced — function onDay1() loadDay(1) end — held loadDay as an upvalue and resolved it at call time, so the order of the two blocks did not matter. Now it does.

Correct today, because 756 precedes 931. But the constraint is new, unwritten, and unenforced by lint or tests. Move the HANDLERS block up beside the other handlers around :880 — exactly where a tidy-up would put it — or move loadDay down, and fn is nil: every onDayN click raises attempt to call a nil value, with nothing catching it. Wave 2 has concurrent tasks editing this file.

Same file, only the order of two blocks differing:

loadDay assigned BEFORE table (today)  new=loaded 1                     old=loaded 1
loadDay assigned AFTER table           new=attempt to call a nil value  old=loaded 1

One comment line is a proportionate fix — the generate() form in finding 2 has the same property, so the note is worth having either way:

-- `fn` is captured by value here, so every function named below must already
-- be assigned above this point: `loadDay` is the forward-declared local from
-- the top of the file, assigned at `function loadDay`.

Verified

Both gates re-run in the author's worktree at cc0178f:

$ nix shell nixpkgs#luau -c luau test.luau
ok
exit=0

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings
exit=0

Diff scope, and the excluded files:

$ git diff --name-status main...refactor/t1-handlers
M	timew/panel.luau

$ git diff --stat main...refactor/t1-handlers
 timew/panel.luau | 72 ++++++++++++++++++++------------------------------------
 1 file changed, 25 insertions(+), 47 deletions(-)

$ git ls-files | grep -E '\.md$|secretspec.toml|forge.sh'
(no output)

.git/info/exclude carries *.md, secretspec.toml, forge.sh, .worktrees/; none are tracked and none appear in the diff. One file changed, one commit.

Sandbox behaviour, probed rather than assumed. Both halves of the getfenv(0) claim hold:

getfenv exists:                                 function
setfenv exists:                                 function
_G is g0:                                       false
plain `function` stmt lands in getfenv(0):      true
_G write:            false   attempt to modify a readonly table
getfenv(0) write:    true
readable as a plain global name:                function   42
rawget(g0,'probeVia0'):                         function

_G is not the thread's globals table and rejects writes; getfenv(0) is, accepts them, and is where a plain function statement lands. Confirmed against the host: plugin.toml declares entry = "panel.luau" under [[panel]], and an entry is loaded and run on m_T — luau_load(m_T, ...) (luau_host.cpp:3063), lua_pcall(m_T, ...) (:2873), with no lua_setfenv on that path. So getfenv(0) inside the chunk is m_T->gt, the table lua_getglobal(m_T, name) reads at :3079. The names are also plain, so they take that branch rather than the @ui: one (ui_handler_table.h:19).

Handler coverage, derived independently from the four producers. Caps read out of the source, not the PR body:

$ sed -n 's/^local \(RECENT_MAX\|ROWS_MAX\|DAYS\|PAGE_BUTTONS\) = \([0-9]*\).*/\1=\2/p' timew/panel.luau
RECENT_MAX=4
ROWS_MAX=8
DAYS=14
PAGE_BUTTONS=5

family	producer	`onClick` site	index expression	what bounds it	max	generated	verdict
`onTag`	`recentButtons`	`:998`	`"onTag" .. i` over `ipairs(recent)`	`fetchTags` guards `#recent < RECENT_MAX` (`:719`); sole writer of `recent`	4	1..4	covered
`onDay`	`dayColumns`	`:1054`, `:1061`	`"onDay" .. i`	loop header clamps: `for i = math.min(DAYS, #days), 1, -1` (`:1031`)	14	1..14	covered
`onRow`	`activityRows`	`:1112`, `:1126`	`"onRow" .. i` over `ipairs(shown)`	`local cap = ROWS_MAX` (`:1086`) + `#shown < cap` (`:1091`)	8	1..8	covered
`onPage`	pager	`:1330`	`"onPage" .. (p - firstShown + 1)` for `p = firstShown, lastShown`	`lastShown = math.min(pages, firstShown + PAGE_BUTTONS - 1)` (`:1309`)	5	1..5	covered

Non-numbered click strings — onPagePrev (:1318), onPageNext (:1343), onClearFilter (:1225), onScopeDay/onScopeAll (via scopeButton, :1244), onStop/onStartDraft (:1203) — plus the non-click callback props onDraftChange/onDraftSubmit (:1178-1179) and onSearchChange/onSearchSubmit (:1290-1291). All 13 hand-written handlers survive.

Checked in both directions, against the loaded module's own globals table:

caps derived from source:       onTag=4 onDay=14 onRow=8 onPage=5
names a producer can emit that are NOT functions:       none
one past each cap still present:        none
live numbered handlers:                 31
orphan numbered handlers:               none
hand-written handlers missing:          none

31 = 4 + 14 + 8 + 5. No producible name is missing, nothing survives past its cap, and no numbered global exists outside the four families.

The pager, swept rather than argued. firstShown/lastShown/onClick replicated verbatim from :1301-1330 and :1355 over every reachable (pages, current, p):

(pages,current,p) triples checked:      400979
onPage index range emitted:     1..5    PAGE_BUTTONS = 5
index stays within 1..PAGE_BUTTONS:     true
handler targets the page printed on the button: always

The last-page clamp is the case worth naming: at current == pages (for pages >= 5), firstShown = max(1, min(pages-2, pages-4)) = pages-4, lastShown = min(pages, pages) = pages, so the final button emits pages - (pages-4) + 1 = 5. Exactly PAGE_BUTTONS, never past it.

pageBase read at click time. The generated body is function(i) gotoPage(pageBase + i - 1) end; pageBase is the module-level local from :228, so it is an upvalue, and only i is captured. Luau gives each numeric-for iteration a fresh i, so the indices do not collide. Demonstrated on the same closure shape, reassigning the base the way render does at :1355:

pageBase=1  -> onPage1..5 target: 1,2,3,4,5
pageBase=12 -> onPage1..5 target: 12,13,14,15,16
upvalue re-read at click time:    true
frozen-at-generation would have given: 1,2,3,4,5

Answers

2. The onRow narrowing from 20 to 8 — safe. No objection.

Verified three ways, independently of the PR body. Statically, activityRows (:1082-1094) fills shown under local cap = ROWS_MAX with #shown < cap, then numbers buttons over ipairs(shown), so the largest emitted index is #shown <= 8. Dynamically, the enumeration above shows onRow9 absent and no producer able to name it. And on main the extra twelve were already unreachable: main defines onRow1..onRow20 but ROWS_MAX is already 8 there, so onRow9..20 were dead weight, not live surface. The narrowing removes handlers that nothing could ever call.

On the IPC path specifically — noctalia msg plugin yuris/timew:panel all row 12 is unaffected, because onIpc does not route through the globals at all:

-- panel.luau:1418
elseif event == "row" then
    filterFromRow(tonumber(payload) or 1)

It calls the local directly. filterFromRow(12) reads shown[12], gets nil, and its if row then guard makes it a no-op. Identical before and after:

branch: onRow8 present: function   onRow9 present: nil
branch: onIpc('row','12'): true
main:   onRow12 present: function
main:   onIpc('row','12'): true

Query.window's cap argument is not a leak either. It honours whatever cap it is given, and test.luau:94-95 already pins that (assert(#win == 3 and count == 10, ...)), so a future rewire of activityRows onto it stays bounded as long as the caller passes ROWS_MAX — the same invariant, already covered.

One consequence worth stating plainly: the patch removes twelve handlers of slack, so #shown <= ROWS_MAX is now load-bearing for handler existence with zero margin. That is the right design — the count should track the constant — but it is a good reason to want the test.luau assertion from finding 1(a), which pins it mechanically instead of by reading two functions.

3. getfenv(0) — correct, and the wrong mechanism to ship.

Both halves of the correctness claim hold, and I verified them rather than taking them: the write succeeds at chunk load, and it lands in the same table lua_getglobal(m_T, name) reads, because an entry chunk is loaded and executed on m_T with no lua_setfenv in the way. The author's luau_host.cpp citations are accurate. _G genuinely loses.

The deprecation risk is the smaller half of the problem and I would not block on it: getfenv is deprecated in Luau but present, the host strips nothing (the only lua_setfenv uses are in the require path, which entries do not take), and if a future shell did remove it the plugin would fail loudly at load rather than silently — 31 dead handlers is not a subtle symptom.

What I object to is the cost, which is finding 1: the call permanently deoptimises the module, and the PR body's analysis of safeenv addresses the writes rather than the call, so the trade was never actually priced. What I would do instead: revert to literal statements and move the cap invariant into test.luau (1a), which keeps every stated goal of the task — the caps stop being spelled twice, drift fails a gate — at zero runtime cost. Longer term, API 4 (1b) deletes the numbered-handler scheme outright, and that is the change actually worth making.

5. HANDLERS — heavier than it needs to be.

Ruling: four small generate(...) calls beat the four-entry table. The table has exactly one consumer sixteen lines below it, adding a family costs one line either way, and { prefix = , count = , fn = } spells out three field names per entry that positional arguments make free — plus a family.fn lookup on every click. See finding 2. It is a P3: the direction of the PR is right and 25-replacing-47 is a real improvement, the specific shape is just two lines and one indirection heavier than the simplest thing that works.

6. Diff scope and the API-3 comment — both good.

One file, one commit, no excluded file tracked or committed. The API-3 comment survives at :873-875 and the extension is accurate: handlers still cannot carry a value at this API, and HANDLERS really is the generation site. The retained clause "which also caps every list" now reads slightly against the grain — the handler block used to set the cap and now follows it — but the appended sentence "so a cap is data, not 43 lines" says exactly that, so the pair is not misleading. No change needed.

Finally, credit where it is due: the static proof in the description is sound. I rebuilt it from scratch, in both directions and with the caps read out of the source rather than the prose, and it came back clean. The functional half of this PR is well argued; it is the mechanism's price tag that needs revisiting.

**Verdict: REQUEST_CHANGES** The refactor is functionally correct. I could not construct a dead click on any path, the `onRow` narrowing is safe, `pageBase` is still read at click time, and the diff is clean. All of that I re-derived rather than took on trust, and it all holds. What blocks the merge is the mechanism. `getfenv(0)` permanently disables `safeenv` for this plugin's globals table, which is the one thing the PR body says it does not do. That is a measured ~2.4x on import-bound work in a 1448-line UI module, spent against a hard 25 ms per-handler CPU budget, to save 22 source lines. ## Findings ### 1. `getfenv(0)` disables `safeenv` for the plugin's whole lifetime — P1 `timew/panel.luau:946` The PR body argues: *"adding fresh keys cannot disturb the `lua_setsafeenv` import cache because no handler name is ever an import."* The premise is true and the conclusion does not follow, because the deoptimisation does not come from the writes. It comes from the `getfenv` call itself: ```cpp // third_party/luau/VM/src/lbaselib.cpp:124-133 static int luaB_getfenv(lua_State* L) { getfunc(L, 1); if (lua_iscfunction(L, -1)) lua_pushvalue(L, LUA_GLOBALSINDEX); else lua_getfenv(L, -1); lua_setsafeenv(L, -1, false); // <-- unconditional return 1; } ``` With `getfenv(0)`, `getfunc` resolves level 0 to the running chunk and `lua_getfenv` pushes that chunk's env — `m_T->gt`, the plugin's own globals. `lua_setsafeenv(..., false)` then lands on exactly that table. Nothing in Lua can set the flag back, so it is off for the rest of the plugin's life. That gates both `GETIMPORT` (every `ui.*`, `noctalia.*`, `math.*`, `table.*`, `string.*` read) and builtin fastcall, throughout `render`, `activityRows`, `dayColumns` and the pager. Isolated to the single line — same table writes, same workload, only `getfenv(0)` vs `{}` differing: ``` no getfenv (control) 17.5 ms getfenv(0) called 40.7 ms no getfenv (control) 16.4 ms getfenv(0) called 42.2 ms ``` Hoisting the hot globals into locals is not a way out; builtin fastcall is gated on the same flag: ``` hoisted, safeenv ON 16.1 ms hoisted, safeenv OFF 27.7 ms ``` Why it is worth a block rather than a note: every click handler runs under a hard CPU budget, and `render()` runs inside it. ```cpp // luau_host.cpp:3099 bool LuauHost::callGlobal(const char* name) { return callGlobalWithBudget(name, std::chrono::milliseconds(25)); } ``` `beginBudget`/`budgetDeadlineCrossed` (luau_host.cpp:2833, 2656) abort the call when it is crossed. `main` calls no `getfenv` at all, so this headroom is spent by the patch. I did not measure `render()` itself — that needs the live host, which is out of scope for a review — so I am reporting the multiplier, not an absolute. The multiplier is enough: it is permanent, module-wide, and was costed at zero. `setfenv` is not an escape hatch; it clears the same flag (`lbaselib.cpp:140`). There is no third way in pure Lua. Under `luaL_sandboxthread` a runtime-computed global name requires a reference to the globals table, and `getfenv`/`setfenv` are the only routes to it. So the choice is: **(a) Merge-now fix.** Revert to the literal `function onTagN()` / `onDayN()` / `onRowN()` / `onPageN()` statements. `SETGLOBAL` writes to `m_T->gt` without ever materialising the table, so it costs nothing. Keep the win the PR was actually after — killing the cap duplication — with an assertion in `test.luau` instead of a runtime generator: ```lua -- The numbered handler families are the panel's click surface: a name a -- producer can build but the module does not define is a silent dead click. -- The caps are spelled twice (constant and last handler), so pin them here. local g = getfenv(panel.parseDuration) for prefix, cap in pairs({ onTag = 4, onDay = 14, onRow = 8, onPage = 5 }) do for i = 1, cap do assert(type(g[prefix .. i]) == "function", prefix .. i .. " is missing") end assert(g[prefix .. (cap + 1)] == nil, prefix .. (cap + 1) .. " outlives its cap") end ``` That is the cap-as-data property with none of the runtime cost, and it fails loudly on exactly the drift the generator was built to prevent. Note `getfenv` in a throwaway test process is free; the objection is only to it running inside the shipped plugin. **(b) The durable fix.** Move the panel to `plugin_api = 4`. A function-valued handler prop is registered directly: ```cpp // plugin_bindings.cpp:523-524 if (lua_isfunction(L, -1)) { out.props.emplace(std::string(propName), registerUiHandler(L, read, path, propName)); } ``` and dispatched through `@ui_handlers` (`luau_host.cpp:3084-3090`, `ui_handler_table.h:18-19`). That deletes the numbered handlers, the caps-to-handler-count coupling, the generator and `getfenv` in one move — the entire problem T1 was assigned. The installed shell is v5.0.0 and supports API 4. Out of scope for this PR, but it is where this should go, and it is worth knowing before this pattern gets copied into `bar.luau`, whose timer runs far hotter than the panel's. ### 2. `HANDLERS` is a one-consumer data structure — P3 `timew/panel.luau:931-951` Ruling on the weight question, since the task asked for one explicitly: the four-entry table is heavier than what it does. It is read in exactly one place, 16 lines below its own definition, and it buys nothing a helper does not. Adding a family costs one line either way. It also costs a `family.prefix` / `family.count` / `family.fn` hash lookup where a helper has upvalues, including `family.fn` on every click. 12 code lines become 10, one nesting level disappears, and the three field names per entry stop being written out: ```lua -- One call per numbered family. The `count` argument is the cap that bounds -- the `onClick` strings `render` can build, so no click can name a handler -- that is absent. Each `fn` is resolved now, so it must already be assigned -- above this point. local globals = getfenv(0) local function generate(prefix, count, fn) for i = 1, count do globals[prefix .. i] = function() fn(i) end end end generate("onTag", RECENT_MAX, startRecent) generate("onDay", DAYS, loadDay) -- ROWS_MAX, not the 20 the literals used to reach: `activityRows` stops -- filling `shown` at ROWS_MAX and numbers the buttons over `ipairs(shown)`, -- so an index past ROWS_MAX is a string the panel cannot produce. generate("onRow", ROWS_MAX, filterFromRow) -- `pageBase` is read when the click arrives, not when the handler is made: -- the strip slides, and a captured base would page to a stale window. generate("onPage", PAGE_BUTTONS, function(i) gotoPage(pageBase + i - 1) end) ``` Moot if finding 1 is resolved by (a). ### 3. `HANDLERS` snapshots `fn` by value, adding a silent ordering constraint — P3 `timew/panel.luau:933` `loadDay` is a forward-declared local (`:337`) assigned at `:756`. Line 933 reads its **value**, so the table holds a snapshot. The literal it replaced — `function onDay1() loadDay(1) end` — held `loadDay` as an upvalue and resolved it at call time, so the order of the two blocks did not matter. Now it does. Correct today, because 756 precedes 931. But the constraint is new, unwritten, and unenforced by lint or tests. Move the `HANDLERS` block up beside the other handlers around `:880` — exactly where a tidy-up would put it — or move `loadDay` down, and `fn` is `nil`: every `onDayN` click raises `attempt to call a nil value`, with nothing catching it. Wave 2 has concurrent tasks editing this file. Same file, only the order of two blocks differing: ``` loadDay assigned BEFORE table (today) new=loaded 1 old=loaded 1 loadDay assigned AFTER table new=attempt to call a nil value old=loaded 1 ``` One comment line is a proportionate fix — the `generate()` form in finding 2 has the same property, so the note is worth having either way: ```lua -- `fn` is captured by value here, so every function named below must already -- be assigned above this point: `loadDay` is the forward-declared local from -- the top of the file, assigned at `function loadDay`. ``` ## Verified Both gates re-run in the author's worktree at `cc0178f`: ``` $ nix shell nixpkgs#luau -c luau test.luau ok exit=0 $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings exit=0 ``` Diff scope, and the excluded files: ``` $ git diff --name-status main...refactor/t1-handlers M timew/panel.luau $ git diff --stat main...refactor/t1-handlers timew/panel.luau | 72 ++++++++++++++++++++------------------------------------ 1 file changed, 25 insertions(+), 47 deletions(-) $ git ls-files | grep -E '\.md$|secretspec.toml|forge.sh' (no output) ``` `.git/info/exclude` carries `*.md`, `secretspec.toml`, `forge.sh`, `.worktrees/`; none are tracked and none appear in the diff. One file changed, one commit. **Sandbox behaviour, probed rather than assumed.** Both halves of the `getfenv(0)` claim hold: ``` getfenv exists: function setfenv exists: function _G is g0: false plain `function` stmt lands in getfenv(0): true _G write: false attempt to modify a readonly table getfenv(0) write: true readable as a plain global name: function 42 rawget(g0,'probeVia0'): function ``` `_G` is not the thread's globals table and rejects writes; `getfenv(0)` is, accepts them, and is where a plain `function` statement lands. Confirmed against the host: `plugin.toml` declares `entry = "panel.luau"` under `[[panel]]`, and an entry is loaded and run *on `m_T`* — `luau_load(m_T, ...)` (`luau_host.cpp:3063`), `lua_pcall(m_T, ...)` (`:2873`), with no `lua_setfenv` on that path. So `getfenv(0)` inside the chunk *is* `m_T->gt`, the table `lua_getglobal(m_T, name)` reads at `:3079`. The names are also plain, so they take that branch rather than the `@ui:` one (`ui_handler_table.h:19`). **Handler coverage, derived independently from the four producers.** Caps read out of the source, not the PR body: ``` $ sed -n 's/^local $RECENT_MAX\|ROWS_MAX\|DAYS\|PAGE_BUTTONS$ = $[0-9]*$.*/\1=\2/p' timew/panel.luau RECENT_MAX=4 ROWS_MAX=8 DAYS=14 PAGE_BUTTONS=5 ``` | family | producer | `onClick` site | index expression | what bounds it | max | generated | verdict | |---|---|---|---|---|---|---|---| | `onTag` | `recentButtons` | `:998` | `"onTag" .. i` over `ipairs(recent)` | `fetchTags` guards `#recent < RECENT_MAX` (`:719`); sole writer of `recent` | 4 | 1..4 | covered | | `onDay` | `dayColumns` | `:1054`, `:1061` | `"onDay" .. i` | loop header clamps: `for i = math.min(DAYS, #days), 1, -1` (`:1031`) | 14 | 1..14 | covered | | `onRow` | `activityRows` | `:1112`, `:1126` | `"onRow" .. i` over `ipairs(shown)` | `local cap = ROWS_MAX` (`:1086`) + `#shown < cap` (`:1091`) | 8 | 1..8 | covered | | `onPage` | pager | `:1330` | `"onPage" .. (p - firstShown + 1)` for `p = firstShown, lastShown` | `lastShown = math.min(pages, firstShown + PAGE_BUTTONS - 1)` (`:1309`) | 5 | 1..5 | covered | Non-numbered click strings — `onPagePrev` (`:1318`), `onPageNext` (`:1343`), `onClearFilter` (`:1225`), `onScopeDay`/`onScopeAll` (via `scopeButton`, `:1244`), `onStop`/`onStartDraft` (`:1203`) — plus the non-click callback props `onDraftChange`/`onDraftSubmit` (`:1178-1179`) and `onSearchChange`/`onSearchSubmit` (`:1290-1291`). All 13 hand-written handlers survive. Checked in both directions, against the loaded module's own globals table: ``` caps derived from source: onTag=4 onDay=14 onRow=8 onPage=5 names a producer can emit that are NOT functions: none one past each cap still present: none live numbered handlers: 31 orphan numbered handlers: none hand-written handlers missing: none ``` 31 = 4 + 14 + 8 + 5. No producible name is missing, nothing survives past its cap, and no numbered global exists outside the four families. **The pager, swept rather than argued.** `firstShown`/`lastShown`/`onClick` replicated verbatim from `:1301-1330` and `:1355` over every reachable `(pages, current, p)`: ``` (pages,current,p) triples checked: 400979 onPage index range emitted: 1..5 PAGE_BUTTONS = 5 index stays within 1..PAGE_BUTTONS: true handler targets the page printed on the button: always ``` The last-page clamp is the case worth naming: at `current == pages` (for `pages >= 5`), `firstShown = max(1, min(pages-2, pages-4)) = pages-4`, `lastShown = min(pages, pages) = pages`, so the final button emits `pages - (pages-4) + 1 = 5`. Exactly `PAGE_BUTTONS`, never past it. **`pageBase` read at click time.** The generated body is `function(i) gotoPage(pageBase + i - 1) end`; `pageBase` is the module-level local from `:228`, so it is an upvalue, and only `i` is captured. Luau gives each numeric-`for` iteration a fresh `i`, so the indices do not collide. Demonstrated on the same closure shape, reassigning the base the way `render` does at `:1355`: ``` pageBase=1 -> onPage1..5 target: 1,2,3,4,5 pageBase=12 -> onPage1..5 target: 12,13,14,15,16 upvalue re-read at click time: true frozen-at-generation would have given: 1,2,3,4,5 ``` ## Answers **2. The `onRow` narrowing from 20 to 8 — safe. No objection.** Verified three ways, independently of the PR body. Statically, `activityRows` (`:1082-1094`) fills `shown` under `local cap = ROWS_MAX` with `#shown < cap`, then numbers buttons over `ipairs(shown)`, so the largest emitted index is `#shown <= 8`. Dynamically, the enumeration above shows `onRow9` absent and no producer able to name it. And on `main` the extra twelve were already unreachable: `main` defines `onRow1..onRow20` but `ROWS_MAX` is already 8 there, so `onRow9..20` were dead weight, not live surface. The narrowing removes handlers that nothing could ever call. On the IPC path specifically — `noctalia msg plugin yuris/timew:panel all row 12` is unaffected, because `onIpc` does not route through the globals at all: ```lua -- panel.luau:1418 elseif event == "row" then filterFromRow(tonumber(payload) or 1) ``` It calls the local directly. `filterFromRow(12)` reads `shown[12]`, gets `nil`, and its `if row then` guard makes it a no-op. Identical before and after: ``` branch: onRow8 present: function onRow9 present: nil branch: onIpc('row','12'): true main: onRow12 present: function main: onIpc('row','12'): true ``` `Query.window`'s `cap` argument is not a leak either. It honours whatever cap it is given, and `test.luau:94-95` already pins that (`assert(#win == 3 and count == 10, ...)`), so a future rewire of `activityRows` onto it stays bounded as long as the caller passes `ROWS_MAX` — the same invariant, already covered. One consequence worth stating plainly: the patch removes twelve handlers of slack, so `#shown <= ROWS_MAX` is now load-bearing for handler existence with zero margin. That is the right design — the count should track the constant — but it is a good reason to want the `test.luau` assertion from finding 1(a), which pins it mechanically instead of by reading two functions. **3. `getfenv(0)` — correct, and the wrong mechanism to ship.** Both halves of the correctness claim hold, and I verified them rather than taking them: the write succeeds at chunk load, and it lands in the same table `lua_getglobal(m_T, name)` reads, because an entry chunk is loaded and executed on `m_T` with no `lua_setfenv` in the way. The author's `luau_host.cpp` citations are accurate. `_G` genuinely loses. The deprecation risk is the smaller half of the problem and I would not block on it: `getfenv` is deprecated in Luau but present, the host strips nothing (the only `lua_setfenv` uses are in the `require` path, which entries do not take), and if a future shell did remove it the plugin would fail loudly at load rather than silently — 31 dead handlers is not a subtle symptom. What I object to is the cost, which is finding 1: the call permanently deoptimises the module, and the PR body's analysis of `safeenv` addresses the writes rather than the call, so the trade was never actually priced. What I would do instead: revert to literal statements and move the cap invariant into `test.luau` (1a), which keeps every stated goal of the task — the caps stop being spelled twice, drift fails a gate — at zero runtime cost. Longer term, API 4 (1b) deletes the numbered-handler scheme outright, and that is the change actually worth making. **5. `HANDLERS` — heavier than it needs to be.** Ruling: four small `generate(...)` calls beat the four-entry table. The table has exactly one consumer sixteen lines below it, adding a family costs one line either way, and `{ prefix = , count = , fn = }` spells out three field names per entry that positional arguments make free — plus a `family.fn` lookup on every click. See finding 2. It is a P3: the direction of the PR is right and 25-replacing-47 is a real improvement, the specific shape is just two lines and one indirection heavier than the simplest thing that works. **6. Diff scope and the API-3 comment — both good.** One file, one commit, no excluded file tracked or committed. The API-3 comment survives at `:873-875` and the extension is accurate: handlers still cannot carry a value at this API, and `HANDLERS` really is the generation site. The retained clause *"which also caps every list"* now reads slightly against the grain — the handler block used to set the cap and now follows it — but the appended sentence *"so a cap is data, not 43 lines"* says exactly that, so the pair is not misleading. No change needed. Finally, credit where it is due: the static proof in the description is sound. I rebuilt it from scratch, in both directions and with the caps read out of the source rather than the prose, and it came back clean. The functional half of this PR is well argued; it is the mechanism's price tag that needs revisiting.

yuris referenced this pull request from a commit

2026-08-31 17:35:59 +00:00

fixup(panel): drop the getfenv generator, keep the onRow narrowing

yuris added 1 commit

2026-08-31 17:35:59 +00:00

fixup(panel): drop the getfenv generator, keep the onRow narrowing f5b248d700

Review on #4 blocked the mechanism, not the result: getfenv(0) calls
lua_setsafeenv(L, -1, false) unconditionally (lbaselib.cpp:131), which
permanently disables safeenv for this module - no GETIMPORT, no builtin
fastcall - against a hard 25ms per-handler budget (luau_host.cpp:2113).

The four numbered families are literal `function onTagN()` statements
again, which compile to SETGLOBAL and never materialise the globals
table. Kept from the PR: onRow is 8 names (ROWS_MAX), not 20, and the
API-3 comment, extended with the cap-regeneration rule, the safeenv
finding and the plugin_api = 4 exit.

yuris commented

2026-08-31 17:36:34 +00:00

Author

Owner

Fix pushed as f5b248d. The block stands, and the review is right about where the cost was hiding: the PR body priced the writes against safeenv and never priced the call.

Accepted: finding 1, option (a)

The generator and getfenv(0) are gone. The four numbered families are literal function onTagN() ... end statements again, which compile to SETGLOBAL and never materialise the globals table, so the lua_setsafeenv(L, -1, false) at lbaselib.cpp:131 is never reached and safeenv stays on for the module's life. GETIMPORT and builtin fastcall keep working through render, activityRows, dayColumns and the pager, which is what the 25 ms budget at luau_host.cpp:2113 is spent on.

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

Zero, not "zero calls" — the word does not appear in the file at all, including in prose, so the gate is a plain grep with nothing to whitelist. The comment cites the two lbaselib.cpp lines as "the two *fenv env-reflection builtins" for exactly that reason.

Kept: the `onRow` narrowing (your answer 2)

onRow1..8, onRow9..onRow20 deleted. This is now the only behavioural change in the PR, so I re-derived the bound in one pass rather than leaning on either the PR body or the review:

shown has exactly two writers, both in activityRows: shown = {} (:1083 pre-fix) and the single table.insert(shown, row) guarded by matching > listOffset and #shown < cap under local cap = ROWS_MAX. So #shown <= 8 unconditionally.
The only onClick = "onRow" .. i sites are the dot box and the ghost tag button, both inside for i, row in ipairs(shown), so the largest producible index is #shown.
The IPC path does not go through the globals: onIpc does elseif event == "row" then filterFromRow(tonumber(payload) or 1), calling the local directly. filterFromRow(12) reads shown[12], gets nil, and the if row then guard makes it a no-op. noctalia msg ... row 12 behaves identically before and after.

The narrowing removes handlers nothing could call: ROWS_MAX was already 8 on main, so onRow9..20 were dead there too.

Kept: the documentation

The API-3 comment survives and carries the reasoning that was scattered across the PR description, so the next person does not redo this:

handler count is bounded by RECENT_MAX / DAYS / ROWS_MAX / PAGE_BUTTONS, and the matching block must be extended by hand if a constant moves, or render emits an onClick naming nothing and the click is silently dead;
the generator was tried and rejected, with the lbaselib.cpp:131 / :140 citation, the ~2.4x measurement, and the 25 ms budget at luau_host.cpp:2113;
plugin_api = 4 deletes the scheme outright via function-valued onClick.

The onRow block keeps its own comment naming the ROWS_MAX bound and the IPC no-op, since that is the pair someone will re-check first. The pageBase-read-at-click-time note moved onto the onPage literals where it belongs.

Moot: findings 2 and 3

Both were properties of the generator, so they died with it, and I would rather say that than pretend to fix them:

2 (HANDLERS is a one-consumer data structure) — there is no table and no generate() helper. Ruling noted for the next time this shape is proposed.
3 (fn captured by value, silent ordering constraint) — gone by construction. function onDay1() loadDay(1) end holds loadDay as an upvalue and resolves it at call time, so the order of the handler block and loadDay's assignment is once again irrelevant. Given Wave 2 has concurrent tasks in this file, losing that constraint entirely beats documenting it.

Follow-up, not this PR

Your 1(a) suggestion to pin the caps in test.luau is the right call and is not dropped — it is scheduled for after #5 merges, which owns test.luau and is still open. The invariant to pin is the one this PR made load-bearing with zero margin: type(g[prefix .. i]) == "function" for i <= cap and nil at cap + 1, for onTag/onDay/onRow/onPage. getfenv in the test process is free; the ban is only on the shipped entry file.

Coverage proof

Throwaway script, not committed. It requires the module and reaches its globals through the env of an exported function — the same table the function statements wrote to — and checks one past each family's end. getfenv is in the harness, which is the distinction your 1(a) draws:

onTag1=function  onTag2=function  onTag3=function  onTag4=function  onTag5=nil
onDay1=function  onDay2=function  onDay3=function  onDay4=function  onDay5=function  onDay6=function  onDay7=function  onDay8=function  onDay9=function  onDay10=function  onDay11=function  onDay12=function  onDay13=function  onDay14=function  onDay15=nil
onRow1=function  onRow2=function  onRow3=function  onRow4=function  onRow5=function  onRow6=function  onRow7=function  onRow8=function  onRow9=nil
onPage1=function  onPage2=function  onPage3=function  onPage4=function  onPage5=function  onPage6=nil
live numbered handlers:  31
hand-written present:    13/13  (none missing)
orphan numbered globals: none
exit=0

31 = 4 + 14 + 8 + 5, nil one past every cap, no numbered global outside the four families, and all 13 hand-written handlers (onClearFilter, onScopeDay, onScopeAll, onPagePrev, onPageNext, onSearchChange, onSearchSubmit, onDraftChange, onDraftSubmit, onStop, onStartDraft, onOpen, onIpc) intact.

Gate

$ nix shell nixpkgs#luau -c luau test.luau
ok
exit=0

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings
exit=0

$ grep -c getfenv timew/panel.luau
0

$ grep -c '^function on' timew/panel.luau
44

$ git diff --name-status main...HEAD
M	timew/panel.luau

$ git diff --stat main...HEAD
 timew/panel.luau | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

44 = 13 hand-written + 31 numbered. test.luau untouched (#5 owns it). The net diff against main is now the comment expansion plus the twelve deleted onRow lines — the PR no longer removes 43 handler lines, because keeping them literal was the point of the fix. Runtime verification (reload, panel, IPC click paths) is the integrator's.

Fix pushed as `f5b248d`. The block stands, and the review is right about where the cost was hiding: the PR body priced the *writes* against `safeenv` and never priced the *call*. ## Accepted: finding 1, option (a) The generator and `getfenv(0)` are gone. The four numbered families are literal `function onTagN() ... end` statements again, which compile to `SETGLOBAL` and never materialise the globals table, so the `lua_setsafeenv(L, -1, false)` at `lbaselib.cpp:131` is never reached and `safeenv` stays on for the module's life. `GETIMPORT` and builtin fastcall keep working through `render`, `activityRows`, `dayColumns` and the pager, which is what the 25 ms budget at `luau_host.cpp:2113` is spent on. ``` $ grep -c getfenv timew/panel.luau 0 $ grep -c setfenv timew/panel.luau 0 ``` Zero, not "zero calls" — the word does not appear in the file at all, including in prose, so the gate is a plain grep with nothing to whitelist. The comment cites the two `lbaselib.cpp` lines as "the two `*fenv` env-reflection builtins" for exactly that reason. ## Kept: the `onRow` narrowing (your answer 2) `onRow1..8`, `onRow9..onRow20` deleted. This is now the only behavioural change in the PR, so I re-derived the bound in one pass rather than leaning on either the PR body or the review: - `shown` has exactly two writers, both in `activityRows`: `shown = {}` (`:1083` pre-fix) and the single `table.insert(shown, row)` guarded by `matching > listOffset and #shown < cap` under `local cap = ROWS_MAX`. So `#shown <= 8` unconditionally. - The only `onClick = "onRow" .. i` sites are the dot box and the ghost tag button, both inside `for i, row in ipairs(shown)`, so the largest producible index is `#shown`. - The IPC path does not go through the globals: `onIpc` does `elseif event == "row" then filterFromRow(tonumber(payload) or 1)`, calling the local directly. `filterFromRow(12)` reads `shown[12]`, gets `nil`, and the `if row then` guard makes it a no-op. `noctalia msg ... row 12` behaves identically before and after. The narrowing removes handlers nothing could call: `ROWS_MAX` was already 8 on `main`, so `onRow9..20` were dead there too. ## Kept: the documentation The API-3 comment survives and carries the reasoning that was scattered across the PR description, so the next person does not redo this: - handler count is bounded by `RECENT_MAX` / `DAYS` / `ROWS_MAX` / `PAGE_BUTTONS`, and the matching block must be extended **by hand** if a constant moves, or `render` emits an `onClick` naming nothing and the click is silently dead; - the generator was tried and rejected, with the `lbaselib.cpp:131` / `:140` citation, the ~2.4x measurement, and the 25 ms budget at `luau_host.cpp:2113`; - `plugin_api = 4` deletes the scheme outright via function-valued `onClick`. The `onRow` block keeps its own comment naming the `ROWS_MAX` bound *and* the IPC no-op, since that is the pair someone will re-check first. The `pageBase`-read-at-click-time note moved onto the `onPage` literals where it belongs. ## Moot: findings 2 and 3 Both were properties of the generator, so they died with it, and I would rather say that than pretend to fix them: - **2 (`HANDLERS` is a one-consumer data structure)** — there is no table and no `generate()` helper. Ruling noted for the next time this shape is proposed. - **3 (`fn` captured by value, silent ordering constraint)** — gone by construction. `function onDay1() loadDay(1) end` holds `loadDay` as an upvalue and resolves it at call time, so the order of the handler block and `loadDay`'s assignment is once again irrelevant. Given Wave 2 has concurrent tasks in this file, losing that constraint entirely beats documenting it. ## Follow-up, not this PR Your 1(a) suggestion to pin the caps in `test.luau` is the right call and is **not** dropped — it is scheduled for after #5 merges, which owns `test.luau` and is still open. The invariant to pin is the one this PR made load-bearing with zero margin: `type(g[prefix .. i]) == "function"` for `i <= cap` and `nil` at `cap + 1`, for `onTag`/`onDay`/`onRow`/`onPage`. `getfenv` in the test process is free; the ban is only on the shipped entry file. ## Coverage proof Throwaway script, not committed. It `require`s the module and reaches its globals through the env of an exported function — the same table the `function` statements wrote to — and checks one past each family's end. `getfenv` is in the **harness**, which is the distinction your 1(a) draws: ``` onTag1=function onTag2=function onTag3=function onTag4=function onTag5=nil onDay1=function onDay2=function onDay3=function onDay4=function onDay5=function onDay6=function onDay7=function onDay8=function onDay9=function onDay10=function onDay11=function onDay12=function onDay13=function onDay14=function onDay15=nil onRow1=function onRow2=function onRow3=function onRow4=function onRow5=function onRow6=function onRow7=function onRow8=function onRow9=nil onPage1=function onPage2=function onPage3=function onPage4=function onPage5=function onPage6=nil live numbered handlers: 31 hand-written present: 13/13 (none missing) orphan numbered globals: none exit=0 ``` 31 = 4 + 14 + 8 + 5, `nil` one past every cap, no numbered global outside the four families, and all 13 hand-written handlers (`onClearFilter`, `onScopeDay`, `onScopeAll`, `onPagePrev`, `onPageNext`, `onSearchChange`, `onSearchSubmit`, `onDraftChange`, `onDraftSubmit`, `onStop`, `onStartDraft`, `onOpen`, `onIpc`) intact. ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau ok exit=0 $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings exit=0 $ grep -c getfenv timew/panel.luau 0 $ grep -c '^function on' timew/panel.luau 44 $ git diff --name-status main...HEAD M timew/panel.luau $ git diff --stat main...HEAD timew/panel.luau | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) ``` 44 = 13 hand-written + 31 numbered. `test.luau` untouched (#5 owns it). The net diff against `main` is now the comment expansion plus the twelve deleted `onRow` lines — the PR no longer removes 43 handler lines, because keeping them literal was the point of the fix. Runtime verification (reload, panel, IPC click paths) is the integrator's.

yuris merged commit bfefba4068 into main

2026-08-31 17:36:38 +00:00

yuris deleted branch refactor/t1-handlers

2026-08-31 17:36:38 +00:00

yuris referenced this pull request from a commit

2026-08-31 17:36:38 +00:00

refactor(panel): generate the numbered click handlers from a table (#4)

No reviewers

No labels

No milestone

No project

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!4

No description provided.

Rows
Columns

refactor(panel): generate the numbered click handlers from a table #4

What changed

_G does not work here, getfenv(0) does

onRow: 8 generated, not 20

pageBase is still read at click time

Comments

Static proof

Gate

Generated handler names, for spot-checking against the onClick strings

Findings

1. getfenv(0) disables safeenv for the plugin's whole lifetime — P1

2. HANDLERS is a one-consumer data structure — P3

3. HANDLERS snapshots fn by value, adding a silent ordering constraint — P3

Verified

Answers

Accepted: finding 1, option (a)

Kept: the onRow narrowing (your answer 2)

Kept: the documentation

Moot: findings 2 and 3

Follow-up, not this PR

Coverage proof

Gate

`_G` does not work here, `getfenv(0)` does

`onRow`: 8 generated, not 20

`pageBase` is still read at click time

Generated handler names, for spot-checking against the `onClick` strings

1. `getfenv(0)` disables `safeenv` for the plugin's whole lifetime — P1

2. `HANDLERS` is a one-consumer data structure — P3

3. `HANDLERS` snapshots `fn` by value, adding a silent ordering constraint — P3

Kept: the `onRow` narrowing (your answer 2)