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

refactor(bar): read settings through a cached Config table #1

Merged
yuris merged 2 commits from refactor/t2a-config into main 2026-08-31 16:44:39 +00:00
Conversation 3 Commits 2 Files changed 1 +78 -5
yuris commented 2026-08-31 16:12:02 +00:00
Owner
Copy link

What

timew/bar.luau gains a small Config table that is now the single read point
for plugin settings. The two ad hoc reads (icon_size in render(), tag in
onClick()) go through it. No behaviour change for a normally-configured user.

API added:

Config.DEFAULTS = { tag = "focus", icon_size = 17 }  -- mirrors plugin.toml
Config.get(key)         -- coerced, clamped, cached value (never nil)
Config.invalidate()     -- drop the cache
onConfigChanged()       -- global: invalidate + render()

Config is also added to the file's trailing export table, next to
formatElapsed, so a later wave can assert on it from test.luau.

Why

  • Defaults duplicated from the manifest. The host returns nil for a key the
    user has never touched — a manifest default is what the settings UI prefills,
    not something getConfig hands back. So render() was previously passing nil
    as a glyph size on a fresh install and relying on the host's own fallback.
  • Cache. render() runs at 1 Hz and a settings read is a host round-trip.
    The invalidation point is onConfigChanged(), which the shell dispatches
    generically to every loaded script (script_runtime.cpp:601), bar widgets
    included, so the cache cannot go stale.
  • Coercion + clamp. Config.get coerces to the type of the default
    (tonumber / tostring), falls back to the default on nil/empty/uncoercible,
    and clamps numbers to the manifest's min/max via a per-key bounds table. The
    settings UI enforces 10-28 for icon_size; a hand-edited config file does not.

plugin.toml is untouched: the declared defaults (tag = "focus",
icon_size = 17, min 10, max 28) match Config.DEFAULTS and CONFIG_BOUNDS
exactly. No mismatch found.

Verified

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

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

$ grep -c 'noctalia\.getConfig' timew/bar.luau
1

Plus a throwaway harness (not committed — it needs getfenv to stub the
noctalia global, since the luau CLI gives each required module its own global
table, and test.luau is off-limits for this PR per the wave contract):

nil            -> "focus" / 17          (manifest default)
"" / ""        -> "focus" / 17          (empty means no opinion)
icon_size 99   -> 28                    (clamp high)
icon_size 1    -> 10                    (clamp low)
icon_size "20" -> 20 (number)           (coerce)
tag 5          -> "5"                   (coerce)
icon_size "big"-> 17                    (uncoercible -> default)
cached until invalidate(), re-read after
=> config ok

Worth challenging

  1. Config in the export table. It widens a return value whose comment says
    it exists only for test.luau's pure helpers — and Config.get is not pure,
    it touches the host. I added it so wave 2 can test the coercion/clamp logic
    for real instead of me shipping verified-once-then-thrown-away output. Say the
    word and I drop it back to { formatElapsed = formatElapsed }.
  2. icon_size is declared int but Config.get does not floor. "17.5"
    would survive as 17.5. The settings UI cannot produce it; only a hand-edited
    config can, and a fractional glyph size is harmless. One math.floor if you
    disagree.
  3. onConfigChanged calls render() unconditionally, including while
    tracking, where the label ignores icon_size. One wasted repaint on a settings
    change; guarding it costs more code than it saves.
## What `timew/bar.luau` gains a small `Config` table that is now the single read point for plugin settings. The two ad hoc reads (`icon_size` in `render()`, `tag` in `onClick()`) go through it. No behaviour change for a normally-configured user. API added: ```lua Config.DEFAULTS = { tag = "focus", icon_size = 17 } -- mirrors plugin.toml Config.get(key) -- coerced, clamped, cached value (never nil) Config.invalidate() -- drop the cache onConfigChanged() -- global: invalidate + render() ``` `Config` is also added to the file's trailing export table, next to `formatElapsed`, so a later wave can assert on it from `test.luau`. ## Why - **Defaults duplicated from the manifest.** The host returns `nil` for a key the user has never touched — a manifest `default` is what the settings UI prefills, not something `getConfig` hands back. So `render()` was previously passing `nil` as a glyph size on a fresh install and relying on the host's own fallback. - **Cache.** `render()` runs at 1 Hz and a settings read is a host round-trip. The invalidation point is `onConfigChanged()`, which the shell dispatches generically to every loaded script (`script_runtime.cpp:601`), bar widgets included, so the cache cannot go stale. - **Coercion + clamp.** `Config.get` coerces to the type of the default (`tonumber` / `tostring`), falls back to the default on nil/empty/uncoercible, and clamps numbers to the manifest's min/max via a per-key bounds table. The settings UI enforces 10-28 for `icon_size`; a hand-edited config file does not. `plugin.toml` is untouched: the declared defaults (`tag = "focus"`, `icon_size = 17`, min 10, max 28) match `Config.DEFAULTS` and `CONFIG_BOUNDS` exactly. No mismatch found. ## Verified ``` $ nix shell nixpkgs#luau -c luau test.luau ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ grep -c 'noctalia\.getConfig' timew/bar.luau 1 ``` Plus a throwaway harness (not committed — it needs `getfenv` to stub the `noctalia` global, since the luau CLI gives each required module its own global table, and `test.luau` is off-limits for this PR per the wave contract): ``` nil -> "focus" / 17 (manifest default) "" / "" -> "focus" / 17 (empty means no opinion) icon_size 99 -> 28 (clamp high) icon_size 1 -> 10 (clamp low) icon_size "20" -> 20 (number) (coerce) tag 5 -> "5" (coerce) icon_size "big"-> 17 (uncoercible -> default) cached until invalidate(), re-read after => config ok ``` ## Worth challenging 1. **`Config` in the export table.** It widens a return value whose comment says it exists only for `test.luau`'s pure helpers — and `Config.get` is not pure, it touches the host. I added it so wave 2 can test the coercion/clamp logic for real instead of me shipping verified-once-then-thrown-away output. Say the word and I drop it back to `{ formatElapsed = formatElapsed }`. 2. **`icon_size` is declared `int` but `Config.get` does not floor.** `"17.5"` would survive as `17.5`. The settings UI cannot produce it; only a hand-edited config can, and a fractional glyph size is harmless. One `math.floor` if you disagree. 3. **`onConfigChanged` calls `render()` unconditionally**, including while tracking, where the label ignores `icon_size`. One wasted repaint on a settings change; guarding it costs more code than it saves.
yuris reviewed 2026-08-31 16:26:31 +00:00
yuris left a comment
Copy link

Merge it. Every claim in the description holds up under independent check: Config.DEFAULTS and CONFIG_BOUNDS match plugin.toml exactly, exactly one noctalia.getConfig call site survives (bar.luau:43), the diff touches only timew/bar.luau, and the whole coercion/clamp/cache matrix reproduces on a harness I wrote myself rather than reading yours. I also went past what you claimed and disassembled the shell instead of trusting script_runtime.cpp:601: the dispatch is real, it is generic, and — better than you argued — ScriptRuntime::State::processEvent installs the new settings map before it calls onConfigChanged, so the cache cannot serve a stale value even in principle. Your premise about nil is correct too: luau_getConfig has no manifest-default fallback at all. Nothing here is a defect and nothing changes what the bar renders. Three findings, two of them comments that the patch made untrue; the only one with future bite is Config.get returning nil for a key you forget to add to DEFAULTS.

Findings

1. Config.get returns nil, uncached, for a key missing from DEFAULTS — timew/bar.luau:42-43

The description promises Config.get(key) is a "coerced, clamped, cached value (never nil)". That holds only for keys present in Config.DEFAULTS. For anything else default is nil, so type(default) == "number" is false, the string branch produces value = nil when the host has nothing either, and configCache[key] = nil stores nothing — every call is a fresh host round-trip returning nil. Measured on the harness below:

undeclared key   1st=nil   2nd=nil   hostCalls=1->2

No call site reaches this today, so it is not a live bug. It matters because T2b routes more reads through this seam and REFACTOR.md sells it as "a configuration seam that makes future settings additive": adding a key to plugin.toml and calling Config.get("new_key") without touching DEFAULTS silently reintroduces the exact nil-into-ui.glyph bug this table exists to kill, and pays a host round-trip per call forever. The host already treats this class of mistake as loud — luau_getConfig logs plugin {} read undeclared setting '{}' before pushing nil — so matching it is three lines:

	local default = Config.DEFAULTS[key]
	if default == nil then
		error("Config.get: no default for '" .. key .. "'", 2)
	end
	local raw = noctalia.getConfig(key)

2. The cache's stated reason is not true — timew/bar.luau:23-26

the value is cached, because render() runs once a second and a settings read is a host round-trip

render() reads a setting only on the idle branch (size = Config.get("icon_size"), line 134). While idle, update() never calls render() — line 193 is if active then render() end — so idle repaints come only from sync(), once every RESYNC_TICKS * TICK_MS = 10 s. While tracking, render() does run at 1 Hz, but it takes the label branch and never calls Config.get at all. Uncached cost is one host call per 10 s on the idle path, never one per second.

Keep the cache — the real justification is stronger than the one you wrote. With no [widget.timew] block both keys are unset, which is the case on this machine (~/.config/noctalia/config.toml contains only [widget.cat]), and every uncached read makes the host log plugin yuris/timew read undeclared setting 'icon_size'. The cache turns a log line every 10 s into one per config change. Since comments here are load-bearing, put that in the comment instead:

-- And the value is cached, because both keys are unset until the user opens the
-- settings UI, and an unset key costs a host round-trip plus a `read undeclared
-- setting` log line on every read. The shell dispatches `onConfigChanged`
-- generically to every loaded script (script_runtime.cpp:601), bar widgets
-- included, and installs the new settings map before the call, so the cache has
-- a reliable invalidation point and cannot go stale.

3. Exporting Config falsifies the comment 200 lines above it — timew/bar.luau:11-12

Keep the export, fix the comment. Line 11 says the trailing return "exists only so test.luau can drive the two pure helpers", and Config.get touches the host — you flagged this yourself. The export costs nothing (the host ignores an entry's return value, and nothing new runs at load), and T7 plans a config matrix that needs it, so dropping it just means re-adding it next wave. But leaving the comment as-is is the one thing the wave contract explicitly forbids: a comment that no longer describes the code.

-- no `require`. The trailing return exists only for test.luau — the pure helpers
-- plus `Config`, whose coercion and clamping T7 asserts on; the host ignores an
-- entry's return value.

Verified

Both gate commands, run by me inside .worktrees/t2a-config:

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

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings
--- exit=0 ---

Scope — one commit, one file, no excluded file in the tree:

$ git log --oneline main..refactor/t2a-config
6db4b75 refactor(bar): read settings through a cached Config table

$ git diff --name-only main...refactor/t2a-config
timew/bar.luau

$ git ls-tree -r --name-only refactor/t2a-config | grep -c '\.md$\|secretspec\.toml\|forge\.sh'
0

One getConfig call site, and it is inside Config.get:

$ grep -rn 'noctalia\.getConfig' timew/
timew/bar.luau:43:	local raw = noctalia.getConfig(key)

Coercion, clamp and cache, on my own harness (copy of your bar.luau under /tmp, noctalia stubbed through getfenv(Config.get); barWidget/ui stubbed so render() is observable):

== DEFAULTS vs plugin.toml ==
  DEFAULTS         tag=focus <string>   icon_size=17 <number>
== coercion ==
  unset/nil        tag=focus <string>   icon_size=17 <number>
  empty string     tag=focus <string>   icon_size=17 <number>
  normal           tag=deep work <string>   icon_size=22 <number>
  clamp high       icon_size=28 <number>
  clamp low        icon_size=10 <number>
  numeric string   icon_size=20 <number>
  uncoercible      icon_size=17 <number>
  fractional str   icon_size=17.5 <number>
  nan              icon_size=28 <number>
  inf              icon_size=28 <number>
  numeric tag      tag=5 <string>
  bool tag         tag=true <string>
== key not in DEFAULTS ==
  undeclared key   1st=nil   2nd=nil   hostCalls=1->2
== cache + invalidate ==
  cache            first=12   2ndGetHostCalls=1   afterEdit=12   afterInvalidate=26
== onConfigChanged ==
  global type      function
  after callback   renders=1   glyphSize=26 <number>   node=glyph

DEFAULTS/CONFIG_BOUNDS against the manifest — tag = "focus", icon_size = 17, min = 10, max = 28. Identical, no drift.

Rather than take script_runtime.cpp:601 on trust I disassembled the installed shell (noctalia-5.0.0). The callback name lives in the script-runtime string table next to the other script callbacks:

 124e0a0 65007363 72697074 2d72756e 74696d65  e.script-runtime
 124e0b0 006f6e49 7063006f 6e436f6e 66696743  .onIpc.onConfigC
 124e0c0 68616e67 6564006f 6e536372 6f6c6c00  hanged.onScroll.

ScriptRuntime::State::processLoad only probes it — hasGlobal, result stored as a capability flag — so defining the global does not cause a render at load, and your "No render() at load" invariant survives:

81f1b7: lea 0xa2eef9(%rip),%rsi   # 124e0b7 (onConfigChanged)
81f1be: call 78fa50 <LuauHost::hasGlobal(char const*)>

ScriptRuntime::State::processEvent does the dispatch, and the ordering is the part worth knowing: it assigns the new settings hashtable first, then gates on hasGlobal alone — no widget-kind filter — then calls it:

81f9ec: call <std::_Hashtable<string, variant<bool,long,double,string,vector<string>>>::_M_assign...>
81fa06: lea  # 124e0b7 (onConfigChanged)
81fa1b: call <LuauHost::hasGlobal(char const*)>
81fa57: call <scripting::PluginBindingContext::beginCall(ScriptSnapshot)>
81fa8b: call <LuauHost::callGlobalWithBudget(char const*, duration)>

So the invalidation point is sound: fresh map, then callback, and generic to every loaded script. Your nil premise checks out too — scripting::luau_getConfig calls luaL_checklstring, getContext, one hashtable lookup, and on a miss std::vformat + logMessage + lua_pushnil. There is no findWidgetSettingSpec call and no manifest-default lookup anywhere in the function, and the format string is plugin {} read undeclared setting '{}'. A declared-but-unset key genuinely comes back nil, so Config.DEFAULTS is load-bearing, not duplication for its own sake.

Behaviour drift: none for a configured user. The only path that moves is a fresh install, where main passed nil into ui.glyph and this branch passes 17 — which is what README.md and the line-131 comment already say the user should see. Wave 1 stays invisible.

I did not run gate step 3 (reload + journalctl); reloading your running shell was out of bounds for me. The static evidence above is strictly stronger than that probe would have been, but see the answer to Q3.

Answers

Q1 — Config in the export table. Keep it. The host ignores the return value, nothing extra executes at load, and test.luau requiring the file is unaffected — I proved the last one by driving Config from outside the module. Reverting to { formatElapsed = formatElapsed } only means T7 re-adds it, and shipping verified-once-then-discarded output is the thing this refactor is supposed to stop. The comment on line 11 has to move with it, though: see finding 3.

Q2 — no math.floor, and don't add a guard for the other one either. Agreed, it is speculative. My probe confirms "17.5" survives as 17.5, the settings UI cannot produce it, and a fractional glyph size is harmless. While checking I did find the one input where the clamp is odd: tonumber("nan") returns NaN, and math.min(28, nan) is 28 because every NaN comparison is false, so icon_size = nan (valid TOML) lands on the maximum size rather than the default — same for inf. I am explicitly not asking you to fix it: 28 is bounded and harmless, the manifest type is int, and nothing else in this codebase guards NaN. Recording it so the outcome is a decision rather than an accident.

Q3 — unconditional render() in onConfigChanged. Leave it. The disassembly confirms your instinct: the gate is hasGlobal and nothing else, so the callback fires on any config change, including settings that have nothing to do with this widget. The cost is still one barWidget.render(), and while tracking that emits the same ui.label the 1 Hz tick already emits. Guarding it would cost a branch plus a "did the value actually change" question, to save a repaint you cannot perceive. One thing to carry forward instead: T0 parked the runtime confirmation of onConfigChanged in this task, and this PR ships a throwaway harness rather than that. I have covered the mechanism statically, so it does not block the merge — but run gate step 3 once at wave-1 integration, since it is the plan's own gate and it is cheaper to do with all three branches landed.

Merge it. Every claim in the description holds up under independent check: `Config.DEFAULTS` and `CONFIG_BOUNDS` match `plugin.toml` exactly, exactly one `noctalia.getConfig` call site survives (`bar.luau:43`), the diff touches only `timew/bar.luau`, and the whole coercion/clamp/cache matrix reproduces on a harness I wrote myself rather than reading yours. I also went past what you claimed and disassembled the shell instead of trusting `script_runtime.cpp:601`: the dispatch is real, it is generic, and — better than you argued — `ScriptRuntime::State::processEvent` installs the new settings map *before* it calls `onConfigChanged`, so the cache cannot serve a stale value even in principle. Your premise about `nil` is correct too: `luau_getConfig` has no manifest-default fallback at all. Nothing here is a defect and nothing changes what the bar renders. Three findings, two of them comments that the patch made untrue; the only one with future bite is `Config.get` returning `nil` for a key you forget to add to `DEFAULTS`. ## Findings ### 1. `Config.get` returns `nil`, uncached, for a key missing from `DEFAULTS` — `timew/bar.luau:42-43` The description promises `Config.get(key)` is a "coerced, clamped, cached value (never nil)". That holds only for keys present in `Config.DEFAULTS`. For anything else `default` is `nil`, so `type(default) == "number"` is false, the string branch produces `value = nil` when the host has nothing either, and `configCache[key] = nil` stores nothing — every call is a fresh host round-trip returning `nil`. Measured on the harness below: ``` undeclared key 1st=nil 2nd=nil hostCalls=1->2 ``` No call site reaches this today, so it is not a live bug. It matters because T2b routes more reads through this seam and REFACTOR.md sells it as "a configuration seam that makes future settings additive": adding a key to `plugin.toml` and calling `Config.get("new_key")` without touching `DEFAULTS` silently reintroduces the exact `nil`-into-`ui.glyph` bug this table exists to kill, and pays a host round-trip per call forever. The host already treats this class of mistake as loud — `luau_getConfig` logs `plugin {} read undeclared setting '{}'` before pushing nil — so matching it is three lines: ```suggestion local default = Config.DEFAULTS[key] if default == nil then error("Config.get: no default for '" .. key .. "'", 2) end local raw = noctalia.getConfig(key) ``` ### 2. The cache's stated reason is not true — `timew/bar.luau:23-26` > the value is cached, because render() runs once a second and a settings read is a host round-trip `render()` reads a setting only on the idle branch (`size = Config.get("icon_size")`, line 134). While idle, `update()` never calls `render()` — line 193 is `if active then render() end` — so idle repaints come only from `sync()`, once every `RESYNC_TICKS * TICK_MS` = 10 s. While tracking, `render()` does run at 1 Hz, but it takes the label branch and never calls `Config.get` at all. Uncached cost is one host call per 10 s on the idle path, never one per second. Keep the cache — the real justification is stronger than the one you wrote. With no `[widget.timew]` block both keys are unset, which is the case on this machine (`~/.config/noctalia/config.toml` contains only `[widget.cat]`), and every uncached read makes the host log `plugin yuris/timew read undeclared setting 'icon_size'`. The cache turns a log line every 10 s into one per config change. Since comments here are load-bearing, put that in the comment instead: ```suggestion -- And the value is cached, because both keys are unset until the user opens the -- settings UI, and an unset key costs a host round-trip plus a `read undeclared -- setting` log line on every read. The shell dispatches `onConfigChanged` -- generically to every loaded script (script_runtime.cpp:601), bar widgets -- included, and installs the new settings map before the call, so the cache has -- a reliable invalidation point and cannot go stale. ``` ### 3. Exporting `Config` falsifies the comment 200 lines above it — `timew/bar.luau:11-12` Keep the export, fix the comment. Line 11 says the trailing return "exists only so test.luau can drive the two pure helpers", and `Config.get` touches the host — you flagged this yourself. The export costs nothing (the host ignores an entry's return value, and nothing new runs at load), and T7 plans a config matrix that needs it, so dropping it just means re-adding it next wave. But leaving the comment as-is is the one thing the wave contract explicitly forbids: a comment that no longer describes the code. ```suggestion -- no `require`. The trailing return exists only for test.luau — the pure helpers -- plus `Config`, whose coercion and clamping T7 asserts on; the host ignores an -- entry's return value. ``` ## Verified Both gate commands, run by me inside `.worktrees/t2a-config`: ``` $ nix shell nixpkgs#luau -c luau test.luau ok --- exit=0 --- $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings --- exit=0 --- ``` Scope — one commit, one file, no excluded file in the tree: ``` $ git log --oneline main..refactor/t2a-config 6db4b75 refactor(bar): read settings through a cached Config table $ git diff --name-only main...refactor/t2a-config timew/bar.luau $ git ls-tree -r --name-only refactor/t2a-config | grep -c '\.md$\|secretspec\.toml\|forge\.sh' 0 ``` One `getConfig` call site, and it is inside `Config.get`: ``` $ grep -rn 'noctalia\.getConfig' timew/ timew/bar.luau:43: local raw = noctalia.getConfig(key) ``` Coercion, clamp and cache, on my own harness (copy of your `bar.luau` under `/tmp`, `noctalia` stubbed through `getfenv(Config.get)`; `barWidget`/`ui` stubbed so `render()` is observable): ``` == DEFAULTS vs plugin.toml == DEFAULTS tag=focus <string> icon_size=17 <number> == coercion == unset/nil tag=focus <string> icon_size=17 <number> empty string tag=focus <string> icon_size=17 <number> normal tag=deep work <string> icon_size=22 <number> clamp high icon_size=28 <number> clamp low icon_size=10 <number> numeric string icon_size=20 <number> uncoercible icon_size=17 <number> fractional str icon_size=17.5 <number> nan icon_size=28 <number> inf icon_size=28 <number> numeric tag tag=5 <string> bool tag tag=true <string> == key not in DEFAULTS == undeclared key 1st=nil 2nd=nil hostCalls=1->2 == cache + invalidate == cache first=12 2ndGetHostCalls=1 afterEdit=12 afterInvalidate=26 == onConfigChanged == global type function after callback renders=1 glyphSize=26 <number> node=glyph ``` `DEFAULTS`/`CONFIG_BOUNDS` against the manifest — `tag = "focus"`, `icon_size = 17`, `min = 10`, `max = 28`. Identical, no drift. Rather than take `script_runtime.cpp:601` on trust I disassembled the installed shell (`noctalia-5.0.0`). The callback name lives in the script-runtime string table next to the other script callbacks: ``` 124e0a0 65007363 72697074 2d72756e 74696d65 e.script-runtime 124e0b0 006f6e49 7063006f 6e436f6e 66696743 .onIpc.onConfigC 124e0c0 68616e67 6564006f 6e536372 6f6c6c00 hanged.onScroll. ``` `ScriptRuntime::State::processLoad` only probes it — `hasGlobal`, result stored as a capability flag — so defining the global does **not** cause a render at load, and your "No render() at load" invariant survives: ``` 81f1b7: lea 0xa2eef9(%rip),%rsi # 124e0b7 (onConfigChanged) 81f1be: call 78fa50 <LuauHost::hasGlobal(char const*)> ``` `ScriptRuntime::State::processEvent` does the dispatch, and the ordering is the part worth knowing: it assigns the new settings hashtable *first*, then gates on `hasGlobal` alone — no widget-kind filter — then calls it: ``` 81f9ec: call <std::_Hashtable<string, variant<bool,long,double,string,vector<string>>>::_M_assign...> 81fa06: lea # 124e0b7 (onConfigChanged) 81fa1b: call <LuauHost::hasGlobal(char const*)> 81fa57: call <scripting::PluginBindingContext::beginCall(ScriptSnapshot)> 81fa8b: call <LuauHost::callGlobalWithBudget(char const*, duration)> ``` So the invalidation point is sound: fresh map, then callback, and generic to every loaded script. Your `nil` premise checks out too — `scripting::luau_getConfig` calls `luaL_checklstring`, `getContext`, one hashtable lookup, and on a miss `std::vformat` + `logMessage` + `lua_pushnil`. There is no `findWidgetSettingSpec` call and no manifest-default lookup anywhere in the function, and the format string is `plugin {} read undeclared setting '{}'`. A declared-but-unset key genuinely comes back `nil`, so `Config.DEFAULTS` is load-bearing, not duplication for its own sake. Behaviour drift: none for a configured user. The only path that moves is a fresh install, where `main` passed `nil` into `ui.glyph` and this branch passes 17 — which is what `README.md` and the line-131 comment already say the user should see. Wave 1 stays invisible. I did not run gate step 3 (reload + `journalctl`); reloading your running shell was out of bounds for me. The static evidence above is strictly stronger than that probe would have been, but see the answer to Q3. ## Answers **Q1 — `Config` in the export table.** Keep it. The host ignores the return value, nothing extra executes at load, and `test.luau` requiring the file is unaffected — I proved the last one by driving `Config` from outside the module. Reverting to `{ formatElapsed = formatElapsed }` only means T7 re-adds it, and shipping verified-once-then-discarded output is the thing this refactor is supposed to stop. The comment on line 11 has to move with it, though: see finding 3. **Q2 — no `math.floor`, and don't add a guard for the other one either.** Agreed, it is speculative. My probe confirms `"17.5"` survives as `17.5`, the settings UI cannot produce it, and a fractional glyph size is harmless. While checking I did find the one input where the clamp is odd: `tonumber("nan")` returns NaN, and `math.min(28, nan)` is `28` because every NaN comparison is false, so `icon_size = nan` (valid TOML) lands on the *maximum* size rather than the default — same for `inf`. I am explicitly not asking you to fix it: 28 is bounded and harmless, the manifest type is `int`, and nothing else in this codebase guards NaN. Recording it so the outcome is a decision rather than an accident. **Q3 — unconditional `render()` in `onConfigChanged`.** Leave it. The disassembly confirms your instinct: the gate is `hasGlobal` and nothing else, so the callback fires on *any* config change, including settings that have nothing to do with this widget. The cost is still one `barWidget.render()`, and while tracking that emits the same `ui.label` the 1 Hz tick already emits. Guarding it would cost a branch plus a "did the value actually change" question, to save a repaint you cannot perceive. One thing to carry forward instead: T0 parked the *runtime* confirmation of `onConfigChanged` in this task, and this PR ships a throwaway harness rather than that. I have covered the mechanism statically, so it does not block the merge — but run gate step 3 once at wave-1 integration, since it is the plan's own gate and it is cheaper to do with all three branches landed.
yuris reviewed 2026-08-31 16:27:57 +00:00
yuris left a comment
Copy link

Verdict: APPROVE. Filed under a COMMENT event only because the forge refuses APPROVED on a self-authored PR (422 approve your own pull is not allowed — the token and the PR poster are both yuris). Treat this as an approval: mergeable as is, the three findings below are yours to take or leave.

Merge it. Every claim in the description holds up under independent check: Config.DEFAULTS and CONFIG_BOUNDS match plugin.toml exactly, exactly one noctalia.getConfig call site survives (bar.luau:43), the diff touches only timew/bar.luau, and the whole coercion/clamp/cache matrix reproduces on a harness I wrote myself rather than reading yours. I also went past what you claimed and disassembled the shell instead of trusting script_runtime.cpp:601: the dispatch is real, it is generic, and — better than you argued — ScriptRuntime::State::processEvent installs the new settings map before it calls onConfigChanged, so the cache cannot serve a stale value even in principle. Your premise about nil is correct too: luau_getConfig has no manifest-default fallback at all. Nothing here is a defect and nothing changes what the bar renders. Three findings, two of them comments that the patch made untrue; the only one with future bite is Config.get returning nil for a key you forget to add to DEFAULTS.

Findings

1. Config.get returns nil, uncached, for a key missing from DEFAULTS — timew/bar.luau:42-43

The description promises Config.get(key) is a "coerced, clamped, cached value (never nil)". That holds only for keys present in Config.DEFAULTS. For anything else default is nil, so type(default) == "number" is false, the string branch produces value = nil when the host has nothing either, and configCache[key] = nil stores nothing — every call is a fresh host round-trip returning nil. Measured on the harness below:

undeclared key   1st=nil   2nd=nil   hostCalls=1->2

No call site reaches this today, so it is not a live bug. It matters because T2b routes more reads through this seam and REFACTOR.md sells it as "a configuration seam that makes future settings additive": adding a key to plugin.toml and calling Config.get("new_key") without touching DEFAULTS silently reintroduces the exact nil-into-ui.glyph bug this table exists to kill, and pays a host round-trip per call forever. The host already treats this class of mistake as loud — luau_getConfig logs plugin {} read undeclared setting '{}' before pushing nil — so matching it is three lines:

	local default = Config.DEFAULTS[key]
	if default == nil then
		error("Config.get: no default for '" .. key .. "'", 2)
	end
	local raw = noctalia.getConfig(key)

2. The cache's stated reason is not true — timew/bar.luau:23-26

the value is cached, because render() runs once a second and a settings read is a host round-trip

render() reads a setting only on the idle branch (size = Config.get("icon_size"), line 134). While idle, update() never calls render() — line 193 is if active then render() end — so idle repaints come only from sync(), once every RESYNC_TICKS * TICK_MS = 10 s. While tracking, render() does run at 1 Hz, but it takes the label branch and never calls Config.get at all. Uncached cost is one host call per 10 s on the idle path, never one per second.

Keep the cache — the real justification is stronger than the one you wrote. With no [widget.timew] block both keys are unset, which is the case on this machine (~/.config/noctalia/config.toml contains only [widget.cat]), and every uncached read makes the host log plugin yuris/timew read undeclared setting 'icon_size'. The cache turns a log line every 10 s into one per config change. Since comments here are load-bearing, put that in the comment instead:

-- And the value is cached, because both keys are unset until the user opens the
-- settings UI, and an unset key costs a host round-trip plus a `read undeclared
-- setting` log line on every read. The shell dispatches `onConfigChanged`
-- generically to every loaded script (script_runtime.cpp:601), bar widgets
-- included, and installs the new settings map before the call, so the cache has
-- a reliable invalidation point and cannot go stale.

3. Exporting Config falsifies the comment 200 lines above it — timew/bar.luau:11-12

Keep the export, fix the comment. Line 11 says the trailing return "exists only so test.luau can drive the two pure helpers", and Config.get touches the host — you flagged this yourself. The export costs nothing (the host ignores an entry's return value, and nothing new runs at load), and T7 plans a config matrix that needs it, so dropping it just means re-adding it next wave. But leaving the comment as-is is the one thing the wave contract explicitly forbids: a comment that no longer describes the code.

-- no `require`. The trailing return exists only for test.luau — the pure helpers
-- plus `Config`, whose coercion and clamping T7 asserts on; the host ignores an
-- entry's return value.

Verified

Both gate commands, run by me inside .worktrees/t2a-config:

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

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings
--- exit=0 ---

Scope — one commit, one file, no excluded file in the tree:

$ git log --oneline main..refactor/t2a-config
6db4b75 refactor(bar): read settings through a cached Config table

$ git diff --name-only main...refactor/t2a-config
timew/bar.luau

$ git ls-tree -r --name-only refactor/t2a-config | grep -c '\.md$\|secretspec\.toml\|forge\.sh'
0

One getConfig call site, and it is inside Config.get:

$ grep -rn 'noctalia\.getConfig' timew/
timew/bar.luau:43:	local raw = noctalia.getConfig(key)

Coercion, clamp and cache, on my own harness (copy of your bar.luau under /tmp, noctalia stubbed through getfenv(Config.get); barWidget/ui stubbed so render() is observable):

== DEFAULTS vs plugin.toml ==
  DEFAULTS         tag=focus <string>   icon_size=17 <number>
== coercion ==
  unset/nil        tag=focus <string>   icon_size=17 <number>
  empty string     tag=focus <string>   icon_size=17 <number>
  normal           tag=deep work <string>   icon_size=22 <number>
  clamp high       icon_size=28 <number>
  clamp low        icon_size=10 <number>
  numeric string   icon_size=20 <number>
  uncoercible      icon_size=17 <number>
  fractional str   icon_size=17.5 <number>
  nan              icon_size=28 <number>
  inf              icon_size=28 <number>
  numeric tag      tag=5 <string>
  bool tag         tag=true <string>
== key not in DEFAULTS ==
  undeclared key   1st=nil   2nd=nil   hostCalls=1->2
== cache + invalidate ==
  cache            first=12   2ndGetHostCalls=1   afterEdit=12   afterInvalidate=26
== onConfigChanged ==
  global type      function
  after callback   renders=1   glyphSize=26 <number>   node=glyph

DEFAULTS/CONFIG_BOUNDS against the manifest — tag = "focus", icon_size = 17, min = 10, max = 28. Identical, no drift.

Rather than take script_runtime.cpp:601 on trust I disassembled the installed shell (noctalia-5.0.0). The callback name lives in the script-runtime string table next to the other script callbacks:

 124e0a0 65007363 72697074 2d72756e 74696d65  e.script-runtime
 124e0b0 006f6e49 7063006f 6e436f6e 66696743  .onIpc.onConfigC
 124e0c0 68616e67 6564006f 6e536372 6f6c6c00  hanged.onScroll.

ScriptRuntime::State::processLoad only probes it — hasGlobal, result stored as a capability flag — so defining the global does not cause a render at load, and your "No render() at load" invariant survives:

81f1b7: lea 0xa2eef9(%rip),%rsi   # 124e0b7 (onConfigChanged)
81f1be: call 78fa50 <LuauHost::hasGlobal(char const*)>

ScriptRuntime::State::processEvent does the dispatch, and the ordering is the part worth knowing: it assigns the new settings hashtable first, then gates on hasGlobal alone — no widget-kind filter — then calls it:

81f9ec: call <std::_Hashtable<string, variant<bool,long,double,string,vector<string>>>::_M_assign...>
81fa06: lea  # 124e0b7 (onConfigChanged)
81fa1b: call <LuauHost::hasGlobal(char const*)>
81fa57: call <scripting::PluginBindingContext::beginCall(ScriptSnapshot)>
81fa8b: call <LuauHost::callGlobalWithBudget(char const*, duration)>

So the invalidation point is sound: fresh map, then callback, and generic to every loaded script. Your nil premise checks out too — scripting::luau_getConfig calls luaL_checklstring, getContext, one hashtable lookup, and on a miss std::vformat + logMessage + lua_pushnil. There is no findWidgetSettingSpec call and no manifest-default lookup anywhere in the function, and the format string is plugin {} read undeclared setting '{}'. A declared-but-unset key genuinely comes back nil, so Config.DEFAULTS is load-bearing, not duplication for its own sake.

Behaviour drift: none for a configured user. The only path that moves is a fresh install, where main passed nil into ui.glyph and this branch passes 17 — which is what README.md and the line-131 comment already say the user should see. Wave 1 stays invisible.

I did not run gate step 3 (reload + journalctl); reloading your running shell was out of bounds for me. The static evidence above is strictly stronger than that probe would have been, but see the answer to Q3.

Answers

Q1 — Config in the export table. Keep it. The host ignores the return value, nothing extra executes at load, and test.luau requiring the file is unaffected — I proved the last one by driving Config from outside the module. Reverting to { formatElapsed = formatElapsed } only means T7 re-adds it, and shipping verified-once-then-discarded output is the thing this refactor is supposed to stop. The comment on line 11 has to move with it, though: see finding 3.

Q2 — no math.floor, and don't add a guard for the other one either. Agreed, it is speculative. My probe confirms "17.5" survives as 17.5, the settings UI cannot produce it, and a fractional glyph size is harmless. While checking I did find the one input where the clamp is odd: tonumber("nan") returns NaN, and math.min(28, nan) is 28 because every NaN comparison is false, so icon_size = nan (valid TOML) lands on the maximum size rather than the default — same for inf. I am explicitly not asking you to fix it: 28 is bounded and harmless, the manifest type is int, and nothing else in this codebase guards NaN. Recording it so the outcome is a decision rather than an accident.

Q3 — unconditional render() in onConfigChanged. Leave it. The disassembly confirms your instinct: the gate is hasGlobal and nothing else, so the callback fires on any config change, including settings that have nothing to do with this widget. The cost is still one barWidget.render(), and while tracking that emits the same ui.label the 1 Hz tick already emits. Guarding it would cost a branch plus a "did the value actually change" question, to save a repaint you cannot perceive. One thing to carry forward instead: T0 parked the runtime confirmation of onConfigChanged in this task, and this PR ships a throwaway harness rather than that. I have covered the mechanism statically, so it does not block the merge — but run gate step 3 once at wave-1 integration, since it is the plan's own gate and it is cheaper to do with all three branches landed.

**Verdict: APPROVE.** Filed under a `COMMENT` event only because the forge refuses `APPROVED` on a self-authored PR (`422 approve your own pull is not allowed` — the token and the PR poster are both `yuris`). Treat this as an approval: mergeable as is, the three findings below are yours to take or leave. Merge it. Every claim in the description holds up under independent check: `Config.DEFAULTS` and `CONFIG_BOUNDS` match `plugin.toml` exactly, exactly one `noctalia.getConfig` call site survives (`bar.luau:43`), the diff touches only `timew/bar.luau`, and the whole coercion/clamp/cache matrix reproduces on a harness I wrote myself rather than reading yours. I also went past what you claimed and disassembled the shell instead of trusting `script_runtime.cpp:601`: the dispatch is real, it is generic, and — better than you argued — `ScriptRuntime::State::processEvent` installs the new settings map *before* it calls `onConfigChanged`, so the cache cannot serve a stale value even in principle. Your premise about `nil` is correct too: `luau_getConfig` has no manifest-default fallback at all. Nothing here is a defect and nothing changes what the bar renders. Three findings, two of them comments that the patch made untrue; the only one with future bite is `Config.get` returning `nil` for a key you forget to add to `DEFAULTS`. ## Findings ### 1. `Config.get` returns `nil`, uncached, for a key missing from `DEFAULTS` — `timew/bar.luau:42-43` The description promises `Config.get(key)` is a "coerced, clamped, cached value (never nil)". That holds only for keys present in `Config.DEFAULTS`. For anything else `default` is `nil`, so `type(default) == "number"` is false, the string branch produces `value = nil` when the host has nothing either, and `configCache[key] = nil` stores nothing — every call is a fresh host round-trip returning `nil`. Measured on the harness below: ``` undeclared key 1st=nil 2nd=nil hostCalls=1->2 ``` No call site reaches this today, so it is not a live bug. It matters because T2b routes more reads through this seam and REFACTOR.md sells it as "a configuration seam that makes future settings additive": adding a key to `plugin.toml` and calling `Config.get("new_key")` without touching `DEFAULTS` silently reintroduces the exact `nil`-into-`ui.glyph` bug this table exists to kill, and pays a host round-trip per call forever. The host already treats this class of mistake as loud — `luau_getConfig` logs `plugin {} read undeclared setting '{}'` before pushing nil — so matching it is three lines: ```suggestion local default = Config.DEFAULTS[key] if default == nil then error("Config.get: no default for '" .. key .. "'", 2) end local raw = noctalia.getConfig(key) ``` ### 2. The cache's stated reason is not true — `timew/bar.luau:23-26` > the value is cached, because render() runs once a second and a settings read is a host round-trip `render()` reads a setting only on the idle branch (`size = Config.get("icon_size")`, line 134). While idle, `update()` never calls `render()` — line 193 is `if active then render() end` — so idle repaints come only from `sync()`, once every `RESYNC_TICKS * TICK_MS` = 10 s. While tracking, `render()` does run at 1 Hz, but it takes the label branch and never calls `Config.get` at all. Uncached cost is one host call per 10 s on the idle path, never one per second. Keep the cache — the real justification is stronger than the one you wrote. With no `[widget.timew]` block both keys are unset, which is the case on this machine (`~/.config/noctalia/config.toml` contains only `[widget.cat]`), and every uncached read makes the host log `plugin yuris/timew read undeclared setting 'icon_size'`. The cache turns a log line every 10 s into one per config change. Since comments here are load-bearing, put that in the comment instead: ```suggestion -- And the value is cached, because both keys are unset until the user opens the -- settings UI, and an unset key costs a host round-trip plus a `read undeclared -- setting` log line on every read. The shell dispatches `onConfigChanged` -- generically to every loaded script (script_runtime.cpp:601), bar widgets -- included, and installs the new settings map before the call, so the cache has -- a reliable invalidation point and cannot go stale. ``` ### 3. Exporting `Config` falsifies the comment 200 lines above it — `timew/bar.luau:11-12` Keep the export, fix the comment. Line 11 says the trailing return "exists only so test.luau can drive the two pure helpers", and `Config.get` touches the host — you flagged this yourself. The export costs nothing (the host ignores an entry's return value, and nothing new runs at load), and T7 plans a config matrix that needs it, so dropping it just means re-adding it next wave. But leaving the comment as-is is the one thing the wave contract explicitly forbids: a comment that no longer describes the code. ```suggestion -- no `require`. The trailing return exists only for test.luau — the pure helpers -- plus `Config`, whose coercion and clamping T7 asserts on; the host ignores an -- entry's return value. ``` ## Verified Both gate commands, run by me inside `.worktrees/t2a-config`: ``` $ nix shell nixpkgs#luau -c luau test.luau ok --- exit=0 --- $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings --- exit=0 --- ``` Scope — one commit, one file, no excluded file in the tree: ``` $ git log --oneline main..refactor/t2a-config 6db4b75 refactor(bar): read settings through a cached Config table $ git diff --name-only main...refactor/t2a-config timew/bar.luau $ git ls-tree -r --name-only refactor/t2a-config | grep -c '\.md$\|secretspec\.toml\|forge\.sh' 0 ``` One `getConfig` call site, and it is inside `Config.get`: ``` $ grep -rn 'noctalia\.getConfig' timew/ timew/bar.luau:43: local raw = noctalia.getConfig(key) ``` Coercion, clamp and cache, on my own harness (copy of your `bar.luau` under `/tmp`, `noctalia` stubbed through `getfenv(Config.get)`; `barWidget`/`ui` stubbed so `render()` is observable): ``` == DEFAULTS vs plugin.toml == DEFAULTS tag=focus <string> icon_size=17 <number> == coercion == unset/nil tag=focus <string> icon_size=17 <number> empty string tag=focus <string> icon_size=17 <number> normal tag=deep work <string> icon_size=22 <number> clamp high icon_size=28 <number> clamp low icon_size=10 <number> numeric string icon_size=20 <number> uncoercible icon_size=17 <number> fractional str icon_size=17.5 <number> nan icon_size=28 <number> inf icon_size=28 <number> numeric tag tag=5 <string> bool tag tag=true <string> == key not in DEFAULTS == undeclared key 1st=nil 2nd=nil hostCalls=1->2 == cache + invalidate == cache first=12 2ndGetHostCalls=1 afterEdit=12 afterInvalidate=26 == onConfigChanged == global type function after callback renders=1 glyphSize=26 <number> node=glyph ``` `DEFAULTS`/`CONFIG_BOUNDS` against the manifest — `tag = "focus"`, `icon_size = 17`, `min = 10`, `max = 28`. Identical, no drift. Rather than take `script_runtime.cpp:601` on trust I disassembled the installed shell (`noctalia-5.0.0`). The callback name lives in the script-runtime string table next to the other script callbacks: ``` 124e0a0 65007363 72697074 2d72756e 74696d65 e.script-runtime 124e0b0 006f6e49 7063006f 6e436f6e 66696743 .onIpc.onConfigC 124e0c0 68616e67 6564006f 6e536372 6f6c6c00 hanged.onScroll. ``` `ScriptRuntime::State::processLoad` only probes it — `hasGlobal`, result stored as a capability flag — so defining the global does **not** cause a render at load, and your "No render() at load" invariant survives: ``` 81f1b7: lea 0xa2eef9(%rip),%rsi # 124e0b7 (onConfigChanged) 81f1be: call 78fa50 <LuauHost::hasGlobal(char const*)> ``` `ScriptRuntime::State::processEvent` does the dispatch, and the ordering is the part worth knowing: it assigns the new settings hashtable *first*, then gates on `hasGlobal` alone — no widget-kind filter — then calls it: ``` 81f9ec: call <std::_Hashtable<string, variant<bool,long,double,string,vector<string>>>::_M_assign...> 81fa06: lea # 124e0b7 (onConfigChanged) 81fa1b: call <LuauHost::hasGlobal(char const*)> 81fa57: call <scripting::PluginBindingContext::beginCall(ScriptSnapshot)> 81fa8b: call <LuauHost::callGlobalWithBudget(char const*, duration)> ``` So the invalidation point is sound: fresh map, then callback, and generic to every loaded script. Your `nil` premise checks out too — `scripting::luau_getConfig` calls `luaL_checklstring`, `getContext`, one hashtable lookup, and on a miss `std::vformat` + `logMessage` + `lua_pushnil`. There is no `findWidgetSettingSpec` call and no manifest-default lookup anywhere in the function, and the format string is `plugin {} read undeclared setting '{}'`. A declared-but-unset key genuinely comes back `nil`, so `Config.DEFAULTS` is load-bearing, not duplication for its own sake. Behaviour drift: none for a configured user. The only path that moves is a fresh install, where `main` passed `nil` into `ui.glyph` and this branch passes 17 — which is what `README.md` and the line-131 comment already say the user should see. Wave 1 stays invisible. I did not run gate step 3 (reload + `journalctl`); reloading your running shell was out of bounds for me. The static evidence above is strictly stronger than that probe would have been, but see the answer to Q3. ## Answers **Q1 — `Config` in the export table.** Keep it. The host ignores the return value, nothing extra executes at load, and `test.luau` requiring the file is unaffected — I proved the last one by driving `Config` from outside the module. Reverting to `{ formatElapsed = formatElapsed }` only means T7 re-adds it, and shipping verified-once-then-discarded output is the thing this refactor is supposed to stop. The comment on line 11 has to move with it, though: see finding 3. **Q2 — no `math.floor`, and don't add a guard for the other one either.** Agreed, it is speculative. My probe confirms `"17.5"` survives as `17.5`, the settings UI cannot produce it, and a fractional glyph size is harmless. While checking I did find the one input where the clamp is odd: `tonumber("nan")` returns NaN, and `math.min(28, nan)` is `28` because every NaN comparison is false, so `icon_size = nan` (valid TOML) lands on the *maximum* size rather than the default — same for `inf`. I am explicitly not asking you to fix it: 28 is bounded and harmless, the manifest type is `int`, and nothing else in this codebase guards NaN. Recording it so the outcome is a decision rather than an accident. **Q3 — unconditional `render()` in `onConfigChanged`.** Leave it. The disassembly confirms your instinct: the gate is `hasGlobal` and nothing else, so the callback fires on *any* config change, including settings that have nothing to do with this widget. The cost is still one `barWidget.render()`, and while tracking that emits the same `ui.label` the 1 Hz tick already emits. Guarding it would cost a branch plus a "did the value actually change" question, to save a repaint you cannot perceive. One thing to carry forward instead: T0 parked the *runtime* confirmation of `onConfigChanged` in this task, and this PR ships a throwaway harness rather than that. I have covered the mechanism statically, so it does not block the merge — but run gate step 3 once at wave-1 integration, since it is the plan's own gate and it is cheaper to do with all three branches landed. 
Add the loud guard for a key missing from Config.DEFAULTS, and correct two
comments that no longer described the code.

The guard: Config.get was silent for an undeclared key -- it returned nil and
did not cache it, so every read repeated the host round-trip (measured
1st=nil 2nd=nil hostCalls=0->2). It now raises at level 2, before the round
trip, so the traceback points at the caller.

The comments: the cache rationale claimed render() reads a setting at 1 Hz.
It does not. update() calls render() only while tracking, and the tracking
branch renders a label with no setting read; the idle branch, the only one
that reads icon_size, repaints from sync() every RESYNC_TICKS * TICK_MS.
The defaults rationale claimed the host returns nil for an untouched key.
It does not: seedEntrySettings seeds every declared entry setting with its
manifest default, so DEFAULTS is a fallback for an unusable stored value,
not for an absent one. Both are rewritten to what the code actually does,
including why the runtime guard is now the only cross-check left.

The export header said the trailing return exists for "the two pure helpers".
It exports one pure helper and Config, which is not pure. Reworded.
yuris commented 2026-08-31 16:44:34 +00:00
Author
Owner
Copy link

Pushed 546de14. Two of the three findings applied as specified; the third was
right that the comment is wrong but wrong about what the truth is, so I wrote a
different replacement. Evidence below, since I am contradicting both the review
and my own PR description.

1. Config.get guard — applied as specified

Added between the DEFAULTS lookup and the getConfig call, with the message
and level 2 unchanged:

if default == nil then
    error("Config.get: no default for '" .. key .. "'", 2)
end

Reproduced the pre-guard behaviour you measured, against 6db4b75:

PRE-GUARD undeclared key: 1st=nil 2nd=nil hostCalls=0->2

Same numbers you got. nil is never written to configCache, so every read
repeated the round-trip. After the guard:

undeclared key raises: Config.get: no default for 'icon_siz'
host calls during undeclared read: 0    (was 1 per read, uncached, before)
level-2 attribution: ./combined.luau:278: Config.get: no default for 'nope'
guard ok

combined.luau:278 is the Config.get("nope") call site in the harness, not
the error() line in bar.luau, so level 2 is doing what it should.

The guard matters more than either of us said, and it is worth recording why.
The refactor replaced two literal getConfig("...") calls with one
getConfig(key), and plugin_lint only cross-checks literal keys: a computed
key sets dynamicCount and suppresses the declared-but-unread check
(plugin_lint.cpp:38,331), while ReadUndeclared is emitted per literal
read (:317) and so can no longer fire at all. Measured on two throwaway
copies of the plugin:

=== A: literal typo'd key (main style)
  error  bar.luau:85  reads undeclared setting 'icon_siz' (did you mean 'icon_size'?)
  warn   bar.luau  declared setting 'icon_size' (entry 'bar') is never read
1 error, 1 warning

=== B: computed key, DEFAULTS typo'd (branch style)
  ok
0 errors, 0 warnings

Case B is the same typo, and lint says ok. The runtime guard is what closes
that hole — the same B plugin under the harness:

DEFAULTS-typo plugin (lint said 'ok'): raised=true err=Config.get: no default for 'icon_size'

So this PR traded a build-time check for a runtime one; the guard is what makes
that trade honest rather than a silent regression.

2. Cache rationale comment — comment was wrong, replacement was too

Your trace of the flow is correct, and I confirmed it before touching anything:
update() calls render() only under if active then, the tracking branch
renders ui.label and reads no setting, and the idle branch — the only reader
of icon_size — repaints from sync()'s callback every
RESYNC_TICKS * TICK_MS. So "render() runs once a second" was not the reason
for the cache. My comment was wrong.

But the replacement rationale is also wrong. There is no log line to save.
The host seeds every declared entry setting with its manifest default before
the script runs, so getConfig("icon_size") does not miss and does not warn:

  • seedEntrySettings (plugin_manifest.cpp:422-434) walks entry.settings and
    inserts field.defaultValue() for every key the user has not overridden.
  • widget_factory.cpp:404-415 calls exactly that, then mergePluginSettings,
    and moves the result into the widget's PluginRuntimeContext::settings.
  • luau_getConfig warns only on settings->find(key) == end()
    (plugin_bindings.cpp:580-585), which a seeded key never hits.

Empirically, over 30 days of this shell's journal:

     44 plugin yuris/timew:panel read undeclared setting 'compact_padding'
      9 plugin yuris/timew:panel read undeclared setting 'input_padding'
      1 plugin yuris/timew:bar read undeclared setting 'icon_size'

The panel's two keys are genuinely undeclared in plugin.toml and warn on
every read — that is the mechanism working. icon_size appears once, at
Aug 31 18:09:16, and plugin.toml was first committed at 22:56:47 the same
day, so that single line predates the manifest declaring the key by nearly five
hours. Since then the idle branch has read icon_size every 10 s across hours
of uptime with zero warnings; noctalia.log contains no occurrence at all.
(The message also renders as plugin yuris/timew:bar ... — ownerId is the
entry's full id, not the plugin id.)

The same evidence kills my own justification in the PR description: "the host
returns nil for a key the user has never touched" is false. It returns the
manifest default. DEFAULTS is not a nil-guard; it is the fallback for a
stored value that is unusable rather than absent, which is what the coercion
and clamp already handle. I rewrote both halves of the comment to that, and
noted that the two lists must now stay in step by hand because lint no longer
cross-checks them.

I left the cache and DEFAULTS in place: the coercion and clamp still earn
their keep against a hand-edited config, and rewiring approved code is not
wave-1's job. Worth flagging for wave 2 though: with the nil premise gone, the
cache's remaining value is one saved round-trip per 10 s idle plus one per
click, which is thin. If wave 2 wants to drop it, onConfigChanged is public
surface and would have to stay.

3. Export header comment — applied

It claimed the return exists for "the two pure helpers"; it exports one pure
helper and Config, which is not pure. (main exported one helper and already
said "two", so this predates the PR.) Reworded to name both exports and what
each is for. Config stays in the export table per your ruling.

math.floor — accepted, not added

Your ruling stands. For the record it is even narrower than you put it: the
manifest declares int, and ManifestField::defaultValue() casts to
std::int64_t (plugin_manifest.cpp:398), so the seeded value is always
integral and only a hand-edited float can reach the script fractional. Not
worth a line of code.

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/bar.luau | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 3 deletions(-)

Harness not committed, same reason as before.

Pushed `546de14`. Two of the three findings applied as specified; the third was right that the comment is wrong but wrong about what the truth is, so I wrote a different replacement. Evidence below, since I am contradicting both the review and my own PR description. ## 1. `Config.get` guard — applied as specified Added between the `DEFAULTS` lookup and the `getConfig` call, with the message and level 2 unchanged: ```lua if default == nil then error("Config.get: no default for '" .. key .. "'", 2) end ``` Reproduced the pre-guard behaviour you measured, against `6db4b75`: ``` PRE-GUARD undeclared key: 1st=nil 2nd=nil hostCalls=0->2 ``` Same numbers you got. `nil` is never written to `configCache`, so every read repeated the round-trip. After the guard: ``` undeclared key raises: Config.get: no default for 'icon_siz' host calls during undeclared read: 0 (was 1 per read, uncached, before) level-2 attribution: ./combined.luau:278: Config.get: no default for 'nope' guard ok ``` `combined.luau:278` is the `Config.get("nope")` call site in the harness, not the `error()` line in `bar.luau`, so level 2 is doing what it should. The guard matters more than either of us said, and it is worth recording why. The refactor replaced two literal `getConfig("...")` calls with one `getConfig(key)`, and `plugin_lint` only cross-checks literal keys: a computed key sets `dynamicCount` and suppresses the declared-but-unread check (`plugin_lint.cpp:38,331`), while `ReadUndeclared` is emitted per *literal* read (`:317`) and so can no longer fire at all. Measured on two throwaway copies of the plugin: ``` === A: literal typo'd key (main style) error bar.luau:85 reads undeclared setting 'icon_siz' (did you mean 'icon_size'?) warn bar.luau declared setting 'icon_size' (entry 'bar') is never read 1 error, 1 warning === B: computed key, DEFAULTS typo'd (branch style) ok 0 errors, 0 warnings ``` Case B is the same typo, and lint says `ok`. The runtime guard is what closes that hole — the same B plugin under the harness: ``` DEFAULTS-typo plugin (lint said 'ok'): raised=true err=Config.get: no default for 'icon_size' ``` So this PR traded a build-time check for a runtime one; the guard is what makes that trade honest rather than a silent regression. ## 2. Cache rationale comment — comment was wrong, replacement was too Your trace of the flow is correct, and I confirmed it before touching anything: `update()` calls `render()` only under `if active then`, the tracking branch renders `ui.label` and reads no setting, and the idle branch — the only reader of `icon_size` — repaints from `sync()`'s callback every `RESYNC_TICKS * TICK_MS`. So "render() runs once a second" was not the reason for the cache. My comment was wrong. **But the replacement rationale is also wrong.** There is no log line to save. The host seeds every declared entry setting with its manifest default before the script runs, so `getConfig("icon_size")` does not miss and does not warn: - `seedEntrySettings` (`plugin_manifest.cpp:422-434`) walks `entry.settings` and inserts `field.defaultValue()` for every key the user has not overridden. - `widget_factory.cpp:404-415` calls exactly that, then `mergePluginSettings`, and moves the result into the widget's `PluginRuntimeContext::settings`. - `luau_getConfig` warns only on `settings->find(key) == end()` (`plugin_bindings.cpp:580-585`), which a seeded key never hits. Empirically, over 30 days of this shell's journal: ``` 44 plugin yuris/timew:panel read undeclared setting 'compact_padding' 9 plugin yuris/timew:panel read undeclared setting 'input_padding' 1 plugin yuris/timew:bar read undeclared setting 'icon_size' ``` The panel's two keys are genuinely undeclared in `plugin.toml` and warn on every read — that is the mechanism working. `icon_size` appears once, at `Aug 31 18:09:16`, and `plugin.toml` was first committed at `22:56:47` the same day, so that single line predates the manifest declaring the key by nearly five hours. Since then the idle branch has read `icon_size` every 10 s across hours of uptime with zero warnings; `noctalia.log` contains no occurrence at all. (The message also renders as `plugin yuris/timew:bar ...` — `ownerId` is the entry's full id, not the plugin id.) The same evidence kills my own justification in the PR description: "the host returns `nil` for a key the user has never touched" is false. It returns the manifest default. `DEFAULTS` is not a nil-guard; it is the fallback for a *stored* value that is unusable rather than absent, which is what the coercion and clamp already handle. I rewrote both halves of the comment to that, and noted that the two lists must now stay in step by hand because lint no longer cross-checks them. I left the cache and `DEFAULTS` in place: the coercion and clamp still earn their keep against a hand-edited config, and rewiring approved code is not wave-1's job. Worth flagging for wave 2 though: with the nil premise gone, the cache's remaining value is one saved round-trip per 10 s idle plus one per click, which is thin. If wave 2 wants to drop it, `onConfigChanged` is public surface and would have to stay. ## 3. Export header comment — applied It claimed the return exists for "the two pure helpers"; it exports one pure helper and `Config`, which is not pure. (`main` exported one helper and already said "two", so this predates the PR.) Reworded to name both exports and what each is for. `Config` stays in the export table per your ruling. ## `math.floor` — accepted, not added Your ruling stands. For the record it is even narrower than you put it: the manifest declares `int`, and `ManifestField::defaultValue()` casts to `std::int64_t` (`plugin_manifest.cpp:398`), so the seeded value is always integral and only a hand-edited float can reach the script fractional. Not worth a line of code. ## 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/bar.luau | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 60 insertions(+), 3 deletions(-) ``` Harness not committed, same reason as before.
yuris merged commit e3d69aaa50 into main 2026-08-31 16:44:39 +00:00
yuris deleted branch refactor/t2a-config 2026-08-31 16:44:39 +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!1
No description provided.