refactor(panel): route every fetch through Timew #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refactor/t3b-rewire"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Wave 3, first of two: the panel's fetch layer now goes through
Timew, and theduplication that was deliberate for one commit is gone.
The invariant this PR establishes
All 8 query paths originate in
Timew. The two mutating strings stay at theircall sites, because
bar.luauduplicates them and cannotrequireacross pluginentries.
Exactly three matches, and no more:
startTagbuilds the one interpolated string (its ownshQuote);onStopandonIpcshare the constant one. All three go throughrun->Timew.exec, sothe execution is Timew's; only the verb is not.
Timew.exec's comment (wave 1)is why: a typed
Timew.start/Timew.stoppair would put the tag's quoting in atable
bar.luau:215cannot reach, serving one caller of two and leaving twoidioms for one operation. I updated that comment's last sentence to record that
the rewire left them in place, rather than reading it as advice to a wave that
has now happened.
8 calls, every one inside
Timew(activeInterval,todayTotal,tags,dayTotals,intervalsForDay,allIntervals,rangeTotal,exec), plus threeprose mentions in comments. No
getfenv/setfenv.What changed
The eight fetch functions keep their names, their side effects (they own the
module state and call
render()) and their call sites. Their bodies shrank to aTimewcall and an assignment.fetchActiveTimew.activeIntervalfetchTotalTimew.todayTotalfetchTagsTimew.tagsfetchDaysTimew.dayTotals(DAYS, filter, …)loadDayTimew.intervalsForDay(day.date, DAY_WALK_MAX, …)fetchAllTimew.allIntervals(ALL_WALK_MAX, filter, …)fetchAllTimew.rangeTotal(filter, …)runTimew.execTimeouts are not asserted by prose: the harness below compares the timeout
argument of every issued command, old against new.
RECENT_MAXmoved to the caller.Timew.parseTagsreturns every tag on purposeand
test.luau:177pins that, sofetchTagscaps:recent = table.move(list, 1, math.min(#list, RECENT_MAX), 1, {}). The no-capassert was not touched or weakened.
Quoting:
filterandday.datereachTimewbare, andTimew.tagArg/Timew.cmd.intervalsForDayare the only things that quote them. Nothingdouble-quotes: the harness drives
"x'; rm -rf /; #"and"it's"throughfetchDaysandfetchAlland compares the resulting command strings characterfor character against the old code's.
Deletions, each with its grep
grep -nfor remaining callers before removing (call-site line numbers are thepre-change ones):
unquote— callers wereparseInterval:332andfetchActive:690. Both gone.Parse.unquote(105) is the surviving copy, and whatTimew.parseActiveandParse.intervaluse. Deleted.parseInterval— callers wereloadDay:789andfetchAll:812, both nowTimew.parseIntervals. Deleted.parseClock— callers werefetchDays:744andfetchAll:823.Parse.clockcovers both, throughTimew.parseDayTotalsandTimew.parseRangeTotal. Deleted.parseDuration— only caller wasparseInterval:331. Deleted with it.matchesFilter— callers werefetchAll:813(gone),activityRows:1127, and the row button'sselected:1163. The two survivors now readTimew.tagMatches(row.tag, filter), the disposition wave 1 recorded aboveTimew.tagMatches. Deleted.trim(startTag,setFilter),shQuote(Timew.tagArg,Timew.cmd.intervalsForDay,startTag),formatSpan(render),formatElapsed(render),tagColor(activityRows),elapsed(render). These go with theFmt/Parsecall-site move, which is the next PR.test.luaufollows the two exports that no longer exist:panel.parseDurationand
panel.parseClockbecamepanel.Parse.duration/panel.Parse.clock, thesame assert list against the copy that survived. The bar/panel drift comment no
longer claims panel has a private
unquote, and the frozen-literals comment nolonger says the fetch functions are still there to be diffed against.
New coverage: bar's stop command
The suite pinned every query builder and bar's start command, but nothing
pinned
"timew stop"against a real call site — the stop branch ofbar.onClickis reachable only oncesynchas seen a live interval, whichnothing in the suite had done. Added, at the end of the drift section:
bar's start assert now composes
Timew.cmd.execas well, so the2>&1redirectis pinned against the builder instead of a second inline literal.
Why no
Timew.cmd.start/Timew.cmd.stop. They were considered astest-only frozen literals, and skipped: no call site may use them (see the
invariant above), so they would be two functions with zero callers, and the
asserts pinning them would compare a builder against its own literal — a
tautology that cannot fail when
startTag,onStoporonIpcchanges.test.luau:274already freezes"timew stop"inside thecmd.execassert, andtest.luau:408freezes"timew start"plus its quoting against bar's actualcommand. The one real gap was bar's stop branch, and the assert above closes it
without adding dead code to a module on a 25 ms per-handler budget. panel's own
three call sites stay unreachable from
test.luau— the luau CLI gives eachrequired module its own global table, so no stub of panel's
runAsyncisvisible inside it. That gap is the no-
requirerule's, and it closes at pluginAPI 4.
Equivalence evidence
This PR changes live behaviour, so here is the proof that it does not. Throwaway
harness, not committed (
/tmp/t3b/):git show main:timew/panel.luauand thenew file are each wrapped with a
noctalia.runAsyncstub and an epilogue thatexposes the module state, then both are driven through the same 21 scenarios —
all eight paths, including idle/live/untagged/quoted active intervals, a prose
day total, six tags against the cap,
loadDaywithkeepScope, without it andout of range, both
fetchAllcommands, the shell injection strings, andrunonexit 0 and exit 1 with the whole
refreshcascade behind it.Compared per scenario, field by field: every issued command string, every
timeout, the
notifyErrortext, everynoctalia.state.setwrite, andactive,startEpoch,tags,total,recent,days,dayRows,allRows,allTotal,mode,selected,listOffset,filter.Negative controls, so that count means something: changing the
dayTotalstimeout to 14000 reports 5 differences; dropping the
RECENT_MAXcap reportstags/six.state.recent.5and.6.Gate
For the reviewer to challenge
fetchDayspassesDAYS,loadDaypassesDAY_WALK_MAX,fetchAllpassesALL_WALK_MAX, rather than relying onTimew's defaults. Deliberate: the command strings stay byte-identical to the old ones (the harness compares them), and the bound reads next to the caller that owns it.test.luauseparately pins that an explicit value equals the default.runignoresTimew.exec'sokargument. That matches the old behaviour: the panel refetches after a failure too, since a failed start otherwise leaves it claiming the old state.loadDay's state logic (pager rewind, scope reset, thekeepScopepath) is copied unchanged; only its tail is new. Worth a read againstmain— the harness covers all three branches.Verdict: REQUEST_CHANGES
I rebuilt the equivalence check from scratch rather than take the deleted harness on faith:
git show main:timew/panel.luauand the branch file, each wrapped in the samerunAsync/ui/panelstub with a probe spliced into the return table, driven through 45 scenarios / 2521 field comparisons — 0 differences, cascades included. The rewire is faithful, the deletions are clean, the quoting is single-layered on every path, andloadDay's state logic is byte-identical. Two things do not hold up:fetchAllmoved the tag test from callback time to issue time, which is a real behaviour change under a mid-flightsetFilter, and theRECENT_MAXcap now has no guard at all — I deleted it in the worktree and the suite still printedok, which is exactly what two comments in this diff's neighbourhood promise cannot happen.Findings
1. Tag the all-time rows at callback time, not at issue time —
timew/panel.luau:696-705(P2)mainfiltered inside the callback (matchesFilter(row.tag)reads the module'sfilter), so a walk that outlived its tag still produced rows for the tag that is current now. The branch passesfilterintoTimew.allIntervals, which closes over it and applies it inTimew.parseIntervals(result.stdout, tag)— the issue-time tag.The two differ whenever
filterchanges while a walk is in flight, and that is a one-click path: in all-time scope,onRow1..8→filterFromRow→setFilter, or a second Enter in the search box (onSearchSubmit:874), both fire while the previous 200-id DOM walk is still spawningtimew getsubprocesses under a 20 s timeout. If the older walk lands last,allRowsis overwritten with the previous tag's rows while the chip shows the new one, and it sticks:update():1308only re-renders, so nothing refetches until the user acts again.activityRows:1002short-circuits onmode == "all"and does not re-filter at render, so the stale rows are displayed as-is.Reproduced against both files with the same stub — issue
fetchAll()underfilter = "focus", change the filter to"email", then land the walk:A stale-drop guard fixes it and also stops
allTotalgoing stale, whichmaingot wrong in the other direction (its rows followed the new tag while its total was still the old tag's):2. The
RECENT_MAXcap is unguarded, and two comments say it is guarded —timew/panel.luau:647-655(P2)I did the mutation the comments invite. In the worktree,
recent = table.move(list, 1, math.min(#list, RECENT_MAX), 1, {})→recent = list:(File restored byte-identically afterwards;
cmpclean,git status --porcelainempty.)Nothing fails, because the cap moved from
parseTags— reachable through the export table — tofetchTags, a module local that norequirecan see at plugin API 3. My own harness catches it (18 differing lines,tags/six/state.recent.5 = admin,.6 = reading), but the repo's suite cannot, and that is the suite that gates the merge.Two load-bearing comments now say otherwise, and this PR is the event that made them false:
timew/panel.luau:363-366— "test.luau asserts the absence of a cap rather than trusting the claim: a rewire that drops the cap has to fail the suite". The rewire has happened; dropping the cap does not fail the suite.test.luau:175-179— "this is what fails if a rewire forgets it — the panel cannot clip, so an uncapped chip row overflows it silently".assert(#many == 6)pinsparseTags' no-cap contract, which is untouched by forgetting the caller-side cap. It fails only if someone adds a cap toparseTags, the opposite hazard.The assert is still worth having; the claim around it is not. Concretely, at
timew/panel.luau:360-366:and the matching sentence at
test.luau:175-179should say the same thing, rather than claiming this assert catches a forgotten cap.3. The
or DEFAULTfallbacks inTimew.cmd.*are now provably dead —timew/panel.luau:659-666(P3)Answering your first challenge item with a deletion rather than a preference. Passing
DAYS,DAY_WALK_MAXandALL_WALK_MAXexplicitly is the right half: the bound reads next to the caller that owns it, and it costs one argument. The default is the redundant half. Before this PRTimewhad no callers at all, so(days or DAYS),(walkMax or DAY_WALK_MAX)and(walkMax or ALL_WALK_MAX)were plausibly for a future one. This PR is that future, and all three call sites pass the value — so the fallback branch is now reachable only from the three asserts that exist to pin it:That is the shape you rejected for
Timew.cmd.start/stop: a code path with no call site, plus an assert comparing a builder against another spelling of itself. Verified harmless — omitting all three arguments at the call sites reproducesmainbyte for byte (0 differing lines over the same 2521 comparisons) — so this is weight, not risk. Dropor DAYS/or DAY_WALK_MAX/or ALL_WALK_MAXatpanel.luau:449,:464,:481, make the parameters required, and delete those three asserts: a future caller that forgets the bound then fails loudly onnilarithmetic instead of silently getting 14. Net −6 lines. Not a merge blocker.4. The new stop assert leaves bar tracking —
test.luau:461-463(P3)syncCb({ stdout = "1000 focus\n" })makes bar active andonClick()does not flip that back —sync()runs only in the callback, which the assert never fires. So the file now ends with bar's module state tracking.test.luau:399-402exists precisely to catch that ("pin it, so a later assert that leaves bar tracking fails here"), but it runs earlier, so it cannot: anything appended after line 463 silently takes the stop branch. You restorebarEnv.osone line later for exactly this reason; restore the state too.Verified
Gate, in the worktree.
My own equivalence run. Both
git show main:timew/panel.luauand the branch file wrapped with the same stub (noctalia.runAsyncqueues instead of running;noctalia.state.*,noctalia.tr,noctalia.notifyError,panel.render,panel.setWantsSecondTicks,ui.*andos.timeall recorded or frozen), with a probe spliced into the return table exposing the module state and the eight fetch entry points. Each scenario drains the whole queue in order — nested calls included — and compares every issued command string, every timeout, everystate.setwrite, everynotifyErrortext, the full module state, and the rendered tree.Covered per path:
fetchActiveidle/live/untagged/quoted/garbage;fetchTotalnormal/empty;fetchTags0/1/4/6/whitespace-and-blank/raw-awk-output;fetchDaysplain/filtered/it's/x'; rm -rf /; #, plus the callback re-enteringloadDay(selected, true)both whendays[selected]exists and when it does not;loadDaysame index / other index /keepScope/keepScopeon the same index / out of range with and withoutkeepScope/ a date containing a quote / with a tag filter set;fetchAllunfiltered / filtered / multi-word / prose total / injection / apostrophe /(untagged);runon exit 0 and exit 1 with the wholerefreshcascade behind it;startTagplain / injection / whitespace-only;setFilterin day scope, all scope and clearing;refreshin both scopes.Negative controls, so that 0 means something.
Every timeout is compared on every path, in both directions; a 1000 ms change is detectable, and none of 3000/3000/3000/15000/10000/20000/10000/3000 moved.
The cap, directly. 0 tags ->
{}; 1 -> 1; 4 -> 4; 6 -> the first 4 in order (focus, deep work, email, review), identical tomain's in-loop#recent < RECENT_MAX.table.move(list, 1, 0, 1, {})is a no-op that returns the fresh table, so the empty case is safe, and the destination is always new —recentnever aliases whatparseTagsreturned. The mutation result is finding 2.Quoting, through a real
/bin/shwith a faketimewonPATHlogging its argv. The commands were taken verbatim out of the branch's own output, not retyped:One argument each, nothing executed, the canary file untouched.
loadDay's date reaches the shell asd='2026-08-31'\''x';for a date carrying a quote. Exactly one layer of quoting everywhere:filterandday.datego intoTimewbare,Timew.tagArg/Timew.cmd.intervalsForDayare the only quoters, and the composed strings are byte-identical tomain's.Deletions. No surviving caller for any of the five, over
timew/*.luauandtest.luau:unquote(bar's private copy atbar.luau:94is bar's own, andParse.unquoteis what the compare-against assert now names),parseInterval(zero hits),parseClock/parseDuration(a comment attest.luau:6only),matchesFilter(a comment atpanel.luau:325only).panel.parseDuration/panel.parseClockare gone from the return table andtest.luauno longer names them. Every helper kept still has a live caller —trim(730, 738),shQuote(417, 466, 734),tagColor(1028),formatSpan(1067),formatElapsedandelapsed(1076) — so nothing was orphaned by the deletions.Invariants.
runAsyncappears 8 times as a call, all insideTimew(508, 515, 522, 529, 536, 545, 552, 570), plus 3 in prose;run("timewis exactly 3 (734, 897, 1328). Nogetfenv/setfenvanywhere intimew/.git diff --name-status main...HEADisM test.luauandM timew/panel.luauand nothing else — no excluded file in the diff.test.luau's no-cap assert is untouched: the hunks are at 5, 211, 376, 405 and 451, and the assert sits at 177 (main) / 179 (branch), outside all of them.loadDayline by line. The pager/scope block,function loadDaythroughlocal day = days[index], diffs empty againstmain— 15 lines, byte for byte,listOffsetrewind andkeepScopeincluded. All six branch combinations are in the run above and match, including the out-of-range early return issuing no command while still having movedselectedandmode.Answers
DAYS/DAY_WALK_MAX/ALL_WALK_MAXpassed explicitly rather than defaulted. The explicit argument is right; keep it. The default it overrides is what should go — see finding 3. There is no drift risk either way (both spellings read the same constant, and omitting all three reproducesmainexactly), so this is weight rather than a bug: threeorbranches and three asserts kept alive for a call site that does not exist.runignoringTimew.exec'sok. Correct, and it matchesmainexactly. The ordering is preserved too —notifyErrorfires insideTimew.execbeforecb(ok)reachespokeBar()andrefresh(), which is the ordermainhad inline. Therun/failscenario compares the notify text, the poke write and the entire five-command refresh cascade behind it: no differences. The comment you added above it is accurate.loadDay's state logic copied unchanged. Verified textually and behaviourally, both above. Nothing shifted. One note that is not an objection:fetchDays' callback re-entersloadDay(selected, true)on every refresh, so theindex ~= selectedguard is the only thing keeping a reader on page 3, and it is unchanged and covered (days/reenter,loadDay/keepscope_same).No
Timew.cmd.start/Timew.cmd.stop. Agreed, and the reasoning holds up under checking rather than just reading well.bar.luau:215builds"timew stop" .. " 2>&1"from its own literal, so the new assert attest.luau:463compares two independently-built strings and is not a tautology — and it is anchored, becausetest.luau:274freezesT.cmd.exec("timew stop") == "timew stop 2>&1"against a literal and:280-285freezestagArg. So rewriting the bar start assert topanel.Timew.cmd.exec("timew start" .. panel.Timew.tagArg(tag))did not weaken it either: both halves it now leans on are pinned to literals elsewhere in the same file. Adding the builders would have added two callerless functions to a module on a 25 ms budget to buy asserts that cannot fail. The one thing to fix in that new block is finding 4.Both blockers fixed in
8aa25e7, and the cap is now guarded rather than promised. One finding pushed back with evidence: theor DEFAULTfallbacks are the suite's only reach into three constants, so dropping them repeats finding 2 rather than resolving it.1. Stale-filter race in
fetchAll— fixed, your guard appliedReproduced first, on
0ee6121, with a throwaway harness (not committed): host stubbed in panel's own global table,runAsyncqueues, and the rows read back off the rendered tree so nothing has to be exported. Driven entirely throughonIpc, which is the pathonRowNand the search box take. All-time scope underfocus,setFilter("email")mid-flight, then land the walks in the order that matters — the newer one first, the older one last:update()is in there because it is what makes it stick: it only re-renders. The total goes with the rows, as you said —allTotalwas landing at the old tag's9:11too.The guard is yours verbatim:
local wanted = filterbefore both calls,wantedintoTimew.allIntervalsandTimew.rangeTotal, andif wanted ~= filter then return endopening each callback. It also does not over-drop: clearing the tag (wanted == "") relists everything, which is asserted.This is pinned in the suite now, per callback:
2.
RECENT_MAX— comments corrected, and the cap is now actually guardedThe honesty problem went first.
timew/panel.luau:354-363andtest.luau:173-179now say thatassert(#many == 6)pins the parser's absence of a cap and fails only if one is put back — the opposite hazard — and that the caller's cap is elsewhere.assert(#many == 6)is untouched.Then the cap itself, because it turns out to be reachable without the two things that were off the table. No new export from
timew/panel.luau, and no*fenvintimew/(grep -c 'getfenv\|setfenv' timew/*.luau->bar.luau:0,panel.luau:0). The route is the onetest.luaualready uses forbar.luau:getfenv(panel.formatElapsed)hands back panel's own global table, so the host stubs in,onOpen()issues the realrefresh(), the tags callback is picked out byjob.cmd == T.cmd.tags(), and the chip row is counted off the rendered tree by itsonTagNhandlers. Feed it six tags, get four chips, in timew's order.Your mutation, which used to print
ok:(Restored byte-identically after each run;
cmpclean.)So the comment at
panel.luau:354-363says the cap is checked, because it is, and names how. The section sits at the end oftest.luaubecause it leaves the panel holding stubs.3.
or DAYS/or DAY_WALK_MAX/or ALL_WALK_MAX— keeping both halves, and here is whyPushing back on this one. The premise is that the fallback branch is reachable only from three asserts; the measurement says the opposite — the fallback branch is the suite's only reach into the values of
DAYS,DAY_WALK_MAXandALL_WALK_MAX. They are module locals, absent from the export table: exactly the shape finding 2 is about.T.cmd.dayTotals()is what pinsDAYSat 14, by way ofseq 0 13.Applying finding 3 exactly as specified — drop the three
orbranches, make the parameters required, delete the three explicit-argument asserts, and pass 14/60/200 in the frozen-literal asserts — then drifting one constant:Lines 3 and 5 are the cost.
DAYSis not just a bound:renderwalksmath.min(DAYS, #days)bars,RIGHT_WisBAR_W * DAYS + BAR_GAP * (DAYS - 1), and theonDay1..onDay14block must be exactlyDAYSlong or every bar past the fourteenth emits anonClicknaming a function that does not exist and clicks nothing — the hazardpanel.luau:768-775is written for. Today that drift fails the suite; with the defaults gone it is silent, and there is no way to pin the constant directly without exporting it.The three explicit-argument asserts stay for the mirror-image reason: they are the only reach into the branch the panel's own call sites actually take. Cheap to keep, and a bound with neither half checked is a bound that is merely claimed. The argument is recorded at
test.luau:243-249so this does not get re-filed as weight.4. Bar left tracking at the end of the file — fixed
Applied verbatim:
syncCb({ stdout = "\n" })after the stop assert, with the reason next to it. This one earned itself immediately — the new panel section is appended below, so thetest.luau:409-412pin had to be true again first.Gate, in the worktree
No
requireadded between entries, no new export, no*fenvintimew/, no formatter run, and notimewdata touched.