feat(panel): edit a row's time and tag inline #21

Merged
yuris merged 1 commit from feat/inline-edit into main 2026-09-04 06:36:40 +00:00
Owner

Right-click a row → Edit time and tag… turns that row into an editor in
place. Start time and duration on one line with confirm/cancel, tag on the line
under them. Only a field you actually change sends a command; an untouched
submit runs nothing and does not even refetch.

Three verbs, and deliberately not modify

Field Command
start timew move @id <the row's own date>THH:MM
duration timew lengthen|shorten @id PT<n>S
tag timew retag @id <text>

timew modify start|end is the obvious instrument and it is the wrong one. It
sets an absolute end, so the panel would have to compose one from a
time-of-day plus a date — and that is where timew surprises you. Verified
against 1.9.1:

interval @3 dated 2026-09-01, 14:00–15:00
$ timew modify end @3 15:30
→ end becomes 2026-09-02T15:30   … a 25-hour interval, silently

A bare time is not a parse error to timew, it is a valid timestamp on today's
date
. move carries the duration with it and lengthen/shorten take a
relative delta, so no end timestamp is ever built and that class of bug
cannot be written here. All three verbs also take the id in the same position,
which is why Timew.cmd.byStart needed one trailing argument (${4:+"$4"})
rather than a word before the id as well.

move runs last. Every command resolves the interval by its start — the
same identity the context menu already uses — and move is the only one that
changes that start. Sent first it would leave retag and the resize looking up
a start that no longer exists and reporting the row as gone.

What is validated here, and what is left to timew

Only the two things timew answers badly: an unparseable time or duration,
where it replies Cannot modify interval @N where start is after end and names
the wrong problem.

Everything timew reports accurately reaches the user verbatim through
Parse.toast — You cannot overlap intervals… and Cannot lengthen open interval @1. So the running interval's start is editable and its duration
is not, and timew is what says so. A refusal stops the chain and leaves the
editor open
with what was typed, so an overlap is corrected rather than
retyped.

The baseline has two jobs, and they are not one job

This is where the review round landed, and the first version of this PR ran them
together.

Whether a field changed is decided against the text the field was seeded
with. Fmt.span floors to minutes above a minute, so a 15m32s interval displays
0:15; comparing the parsed display back would have shortened it by 32s on an
unchanged submit, and on the active row — whose duration ticks every second —
an untouched submit would have sent a resize nobody asked for. Parse.interval
carries seconds beside duration and beginEdit captures both, so an
untouched field sends nothing however the row moves underneath.

By how much the duration changed is decided against the row as it now
stands
, which the first version got wrong. Nothing closes this editor but its
own done(ok): run refetches from start, stop, startTag, startRecent,
continue, delete and undo, and none of them touches state.editing. So the
interval underneath can change length while the editor is open — right-click the
running row, click Stop, ask for 1:00 on a row that is now 0:45 — and
the frozen number sent lengthen PT1800S, leaving the interval at 1:15 with
nothing to toast, because timew had done exactly what it was told. submitEdit
now finds the live row by base.start and hands that to Timew.cmd.edits;
start and date are the same value either way, so only seconds moves, and a
row that has gone falls back to the baseline, which is what the miss message is
already there for.

The editor also closes with the panel now. onOpen cleared the card's draft
and not the editor's four fields or its baseline, and there is no onClose in
the plugin API to catch them anywhere else — so a reopened panel came back with
a row already in edit mode, holding text from a previous session and a baseline
of unbounded age.

Two more subtleties

A multi-tag row's tag is refused rather than mangled. timew quotes a tag
containing a space and leaves a bare one alone, so a two-tag set arrives as
admin "deep work" and the parser's whole-string unquote cannot take those
quotes. Handing that back to retag does not flatten the set: 1.9.1 answers
Note: '"admin \"deep work\""' is a new tag and the interval keeps a tag by
that name, in timew tags, for good. beginEdit recognises the shape — a quote
still in a value the parser already unquoted once — and the tag is left out of
that row's submit. The other two fields still work.

Two lines, and it is measured rather than chosen. A ui.input reserves
30 px of inset flat — Style::spaceMd + kTextInnerInset, both sides, and
nothing in kInput reaches setHorizontalPadding. Three fields plus two
buttons on one 283 px row left the time fields 22 px of viewport and 08:00
rendered as 08:. No plugin API measures text, so the suite could not see it —
a screenshot caught it. The time fields are budgeted for six glyphs, not
five: Fmt.span's hour field has no bound, so a hundred-hour interval seeds
100:00, and timew holds those happily (lengthen @1 PT359996400S runs the
interval out to 2038). 76 px each, which the top line's 283 absorbs with 67 px
to spare.

Verification

Driven with real pointer and keyboard events against a real database (backed up
first; my probe intervals removed, the six real ones untouched):

Path Result
menu item placement beside Continue, above the separator; Undo correctly greyed
full three-field edit 08:00 1:00 edit-probe → 10:30 2:15 edit-probe-2, one confirm, exact
list refresh editor closed, row redrawn, dot recoloured for the new tag
overlapping duration refused, db unchanged, editor kept 4:00, timew's own message toasted
malformed time this plugin's message toasted, no command sent, editor stayed open
cancel typed values discarded, row restored

The byStart shell program was also run directly against a scratch database:
retag with deep work stores one tag with a space, move preserves duration,
lengthen PT600S adds exactly ten minutes, the menu's verbs still work with no
$4 at all, and a hostile tag arrives byte-intact with $HOME unexpanded and
backticks unexecuted.

Gates: nix shell nixpkgs#luau -c luau test.luau → 13 states, 1870 field comparisons, ok; noctalia plugins lint timew → 0 errors, 0 warnings.

What that 1870 does not cover, said plainly. main at 2029e99 prints the
same line byte for byte: the state matrix gained zero comparisons here, and it
cannot have — all 13 check() calls run before the editor section, so
state.editing is "" and editKey is 0 in every one of them, and compare
never reads a row's isEditing or the four new model fields. The figure is not
a gate for this feature and is no longer quoted as one.

What does cover it is the editor section driving the tree — 82 fire( calls
against 13 on main, 658 asserts against 479 — plus a new two-way
MODEL_FIELDS census beside the existing state one, so Viewmodel.build
growing a junk top-level field, or a junk field on every row or every bar, now
fails where it used to pass.

58 source mutations run this round; 3 survive, and all three are named. The
author's original 31 all still die, including the eleven both reviewers found
alive: EDIT_H 24→20 and a dropped height, focus = true on the editor
fields, either gap zeroed, confirm's variant matching cancel's, cancelEdit
leaving editBase populated, submitEdit without its base.start guard,
runChain firing done(ok) after the poke, onSubmit as a no-op / as
cancelEdit / on one field of three, one key shared by all three inputs, move
composing today's date when the start moves forward, and the two directions of
the baseline confusion (replacing base wholesale, and measuring the delta off
the frozen seconds).

One mutation is retired rather than killed: "comparing against the live row
instead of the baseline" was on the original list, and half of it is now the
correct behaviour. The suite pins both halves separately — an untouched submit
on a row that grew underneath sends nothing, and a touched one measures its
delta off the live seconds.

The three survivors, with why each is left:

  • EDIT_TIME_W 76 → 71 — 41 px of viewport is exactly the floor the suite
    asserts; the remaining 5 px is the caret's, which is judgment and not a
    measurement.
  • the break in the live-row lookup — two intervals cannot share a start
    (timew forbids overlap), so the last match is the first match.
  • onOpen also resetting editKey to 0 — nothing observes it, because a
    closed panel's tree is gone. The counter is left monotonic on purpose, so a
    fresh editor cannot reuse a key an earlier one used.

Known limits, documented not worked around

  • Clearing the tag field does nothing — retag cannot empty a tag set.
  • A multi-tag interval whose tags timew quotes cannot have its tag edited here
    at all (above). A set of bare tags — admin focus — carries no quote to
    recognise and still flattens into one tag if you edit the field; leave it
    alone and the set is untouched.
  • The editor is 24-hour whatever the shell's time_format is, so on a 12-hour
    shell the row reads 5:48 PM above a field reading 17:48.
  • The start field sets a time on the row's own date, always. On a 23:50
    row, typing 00:10 moves the interval 23h40m backwards within that day —
    verified on 1.9.1, Moved @2 to 2026-09-01T00:10:00.
  • The editor is 32 px taller than the row it replaces and kScroll exposes no
    scroll offset, so opening one on the last visible row pushes part of it below
    the fold. Scroll down by hand.
  • Undo pops one journal entry, so a three-command edit needs three undos —
    timew has no transaction to group them with.
Right-click a row → **Edit time and tag…** turns that row into an editor in place. Start time and duration on one line with confirm/cancel, tag on the line under them. Only a field you actually change sends a command; an untouched submit runs nothing and does not even refetch. ## Three verbs, and deliberately not `modify` | Field | Command | |---|---| | start | `timew move @id <the row's own date>THH:MM` | | duration | `timew lengthen\|shorten @id PT<n>S` | | tag | `timew retag @id <text>` | `timew modify start|end` is the obvious instrument and it is the wrong one. It sets an **absolute** end, so the panel would have to compose one from a time-of-day plus a date — and that is where timew surprises you. Verified against 1.9.1: ``` interval @3 dated 2026-09-01, 14:00–15:00 $ timew modify end @3 15:30 → end becomes 2026-09-02T15:30 … a 25-hour interval, silently ``` A bare time is not a parse error to timew, it is a valid timestamp *on today's date*. `move` carries the duration with it and `lengthen`/`shorten` take a relative delta, so **no end timestamp is ever built** and that class of bug cannot be written here. All three verbs also take the id in the same position, which is why `Timew.cmd.byStart` needed one trailing argument (`${4:+"$4"}`) rather than a word before the id as well. **`move` runs last.** Every command resolves the interval by its start — the same identity the context menu already uses — and `move` is the only one that changes that start. Sent first it would leave `retag` and the resize looking up a start that no longer exists and reporting the row as gone. ## What is validated here, and what is left to timew Only the two things timew answers *badly*: an unparseable time or duration, where it replies `Cannot modify interval @N where start is after end` and names the wrong problem. Everything timew reports accurately reaches the user verbatim through `Parse.toast` — `You cannot overlap intervals…` and `Cannot lengthen open interval @1`. So the **running** interval's start is editable and its duration is not, and timew is what says so. A refusal stops the chain and **leaves the editor open** with what was typed, so an overlap is corrected rather than retyped. ## The baseline has two jobs, and they are not one job This is where the review round landed, and the first version of this PR ran them together. **Whether** a field changed is decided against the text the field was seeded with. `Fmt.span` floors to minutes above a minute, so a 15m32s interval displays `0:15`; comparing the parsed display back would have shortened it by 32s on an unchanged submit, and on the *active* row — whose duration ticks every second — an untouched submit would have sent a resize nobody asked for. `Parse.interval` carries `seconds` beside `duration` and `beginEdit` captures both, so an untouched field sends nothing however the row moves underneath. **By how much** the duration changed is decided against the row *as it now stands*, which the first version got wrong. Nothing closes this editor but its own `done(ok)`: `run` refetches from start, stop, `startTag`, `startRecent`, continue, delete and undo, and none of them touches `state.editing`. So the interval underneath can change length while the editor is open — right-click the running row, click **Stop**, ask for `1:00` on a row that is now `0:45` — and the frozen number sent `lengthen PT1800S`, leaving the interval at **1:15** with nothing to toast, because timew had done exactly what it was told. `submitEdit` now finds the live row by `base.start` and hands *that* to `Timew.cmd.edits`; `start` and `date` are the same value either way, so only `seconds` moves, and a row that has gone falls back to the baseline, which is what the miss message is already there for. The editor also closes with the panel now. `onOpen` cleared the card's `draft` and not the editor's four fields or its baseline, and there is no `onClose` in the plugin API to catch them anywhere else — so a reopened panel came back with a row already in edit mode, holding text from a previous session and a baseline of unbounded age. ## Two more subtleties **A multi-tag row's tag is refused rather than mangled.** timew quotes a tag containing a space and leaves a bare one alone, so a two-tag set arrives as `admin "deep work"` and the parser's whole-string unquote cannot take those quotes. Handing that back to `retag` does not flatten the set: 1.9.1 answers `Note: '"admin \"deep work\""' is a new tag` and the interval keeps a tag by that name, in `timew tags`, for good. `beginEdit` recognises the shape — a quote still in a value the parser already unquoted once — and the tag is left out of that row's submit. The other two fields still work. **Two lines, and it is measured rather than chosen.** A `ui.input` reserves 30 px of inset flat — `Style::spaceMd` + `kTextInnerInset`, both sides, and nothing in `kInput` reaches `setHorizontalPadding`. Three fields plus two buttons on one 283 px row left the time fields 22 px of viewport and `08:00` rendered as `08:`. No plugin API measures text, so the suite could not see it — a screenshot caught it. The time fields are budgeted for **six** glyphs, not five: `Fmt.span`'s hour field has no bound, so a hundred-hour interval seeds `100:00`, and timew holds those happily (`lengthen @1 PT359996400S` runs the interval out to 2038). 76 px each, which the top line's 283 absorbs with 67 px to spare. ## Verification Driven with real pointer and keyboard events against a real database (backed up first; my probe intervals removed, the six real ones untouched): | Path | Result | |---|---| | menu item placement | beside Continue, above the separator; Undo correctly greyed | | full three-field edit | `08:00 1:00 edit-probe` → `10:30 2:15 edit-probe-2`, one confirm, exact | | list refresh | editor closed, row redrawn, dot recoloured for the new tag | | overlapping duration | refused, **db unchanged**, editor kept `4:00`, timew's own message toasted | | malformed time | this plugin's message toasted, **no command sent**, editor stayed open | | cancel | typed values discarded, row restored | The `byStart` shell program was also run directly against a scratch database: `retag` with `deep work` stores one tag with a space, `move` preserves duration, `lengthen PT600S` adds exactly ten minutes, the menu's verbs still work with no `$4` at all, and a hostile tag arrives byte-intact with `$HOME` unexpanded and backticks unexecuted. Gates: `nix shell nixpkgs#luau -c luau test.luau` → `13 states, 1870 field comparisons`, `ok`; `noctalia plugins lint timew` → `0 errors, 0 warnings`. **What that 1870 does not cover, said plainly.** `main` at `2029e99` prints the same line byte for byte: the state matrix gained *zero* comparisons here, and it cannot have — all 13 `check()` calls run before the editor section, so `state.editing` is `""` and `editKey` is `0` in every one of them, and `compare` never reads a row's `isEditing` or the four new model fields. The figure is not a gate for this feature and is no longer quoted as one. What does cover it is the editor section driving the tree — 82 `fire(` calls against 13 on `main`, 658 asserts against 479 — plus a new two-way `MODEL_FIELDS` census beside the existing state one, so `Viewmodel.build` growing a junk top-level field, or a junk field on every row or every bar, now fails where it used to pass. **58 source mutations run this round; 3 survive, and all three are named.** The author's original 31 all still die, including the eleven both reviewers found alive: `EDIT_H` 24→20 and a dropped `height`, `focus = true` on the editor fields, either `gap` zeroed, confirm's `variant` matching cancel's, `cancelEdit` leaving `editBase` populated, `submitEdit` without its `base.start` guard, `runChain` firing `done(ok)` after the poke, `onSubmit` as a no-op / as `cancelEdit` / on one field of three, one key shared by all three inputs, `move` composing today's date when the start moves forward, and the two directions of the baseline confusion (replacing `base` wholesale, and measuring the delta off the frozen `seconds`). One mutation is **retired** rather than killed: "comparing against the live row instead of the baseline" was on the original list, and half of it is now the correct behaviour. The suite pins both halves separately — an untouched submit on a row that grew underneath sends nothing, and a touched one measures its delta off the live seconds. The three survivors, with why each is left: - `EDIT_TIME_W` 76 → 71 — 41 px of viewport is exactly the floor the suite asserts; the remaining 5 px is the caret's, which is judgment and not a measurement. - the `break` in the live-row lookup — two intervals cannot share a start (timew forbids overlap), so the last match is the first match. - `onOpen` also resetting `editKey` to 0 — nothing observes it, because a closed panel's tree is gone. The counter is left monotonic on purpose, so a fresh editor cannot reuse a key an earlier one used. ## Known limits, documented not worked around - Clearing the tag field does nothing — `retag` cannot empty a tag set. - A multi-tag interval whose tags timew quotes cannot have its tag edited here at all (above). A set of bare tags — `admin focus` — carries no quote to recognise and still **flattens** into one tag if you edit the field; leave it alone and the set is untouched. - The editor is 24-hour whatever the shell's `time_format` is, so on a 12-hour shell the row reads `5:48 PM` above a field reading `17:48`. - The start field sets a time on the row's **own** date, always. On a `23:50` row, typing `00:10` moves the interval 23h40m *backwards* within that day — verified on 1.9.1, `Moved @2 to 2026-09-01T00:10:00`. - The editor is 32 px taller than the row it replaces and `kScroll` exposes no scroll offset, so opening one on the last visible row pushes part of it below the fold. Scroll down by hand. - Undo pops one journal entry, so a three-command edit needs three undos — timew has no transaction to group them with.
Right-click a row -> "Edit time and tag..." turns that row into an editor
in place: start time and duration on one line with confirm/cancel, tag on
the line under them. Only a field you actually change sends a command; an
untouched submit runs nothing and does not refetch.

Three verbs, and deliberately not `modify`:

  start     timew move @id <the row's own date>THH:MM
  duration  timew lengthen|shorten @id PT<n>S
  tag       timew retag @id <text>

`modify start|end` sets an absolute end, so the panel would have to build
one from a time-of-day plus a date -- and that is where timew surprises
you. Verified against 1.9.1: `timew modify end @3 15:30` on a row dated
the day before moved its end to *today* and silently produced a 25-hour
interval, because a bare time is a valid timestamp on today's date. `move`
carries the duration with it and lengthen/shorten take a relative delta,
so no end timestamp is ever composed and that bug cannot be written here.
All three also take the id in the same position, so Timew.cmd.byStart
needed one trailing argument (`${4:+"$4"}`) rather than a word before the
id as well.

`move` runs last: every command resolves the interval by its start, and
`move` is the only one that changes it. Sent first it would leave retag
and the resize looking up a start that no longer exists.

Validation covers only what timew answers badly -- an unparseable time or
duration, where it replies "start is after end" and names the wrong
problem. Overlap and "Cannot lengthen open interval" are accurate, so they
reach the user verbatim through Parse.toast; the running interval's start
is editable and its duration is not, and timew is what says so. A refusal
stops the chain and leaves the editor open with what was typed.

The duration delta is measured against the interval's raw seconds, not the
string on screen: Fmt.span floors to minutes, so a 15m32s row displays
0:15 and an unchanged submit would otherwise have shortened it by 32s.

Two lines, not one, and measured rather than chosen: a ui.input reserves
30px of inset flat (Style::spaceMd + kTextInnerInset, both sides, no
plugin prop reaches it), so three fields plus two buttons on one 283px row
left the time fields 22px of viewport and "08:00" rendered as "08:". No
plugin API measures text, so the suite could not see it -- a screenshot
caught it.

Verified live with real pointer and keyboard events: a full three-field
edit landed exactly (08:00 1:00 -> 10:30 2:15, tag retagged, one confirm);
an overlapping duration was refused with timew's own message and the
editor kept the typed values; a malformed time toasted this plugin's
message and sent nothing; cancel discarded. 31 source mutations, no
survivors.
Author
Owner

Review — source half (parse.luau, timew.luau, panel.luau, viewmodel.luau, en.json)

REQUEST_CHANGES. One reachable silent wrong write, plus the state leak that
widens it. Everything else is advisory. test.luau is the sibling reviewer's;
I only read it.

Gates on the branch as shipped: 13 states, 1870 field comparisons, ok;
0 errors, 0 warnings. Everything below was run against my own
TIMEWARRIORDB=/tmp/rs2-timewdb and a /tmp copy of the tree.


The claims hold. Checked one at a time, not taken on trust.

The 25-hour bug is real and worse than the PR states. Today is 2026-09-04:

$ timew get dom.tracked.1.start dom.tracked.1.end
2026-09-01T14:00:00 2026-09-01T15:00:00
$ timew modify end @1 15:30        # no output, exit 0
2026-09-01T14:00:00 2026-09-04T15:30:00

73.5 hours, silently. The interval's own date never enters it — the bare time
resolves on today, so the error grows with the row's age.

And the three verbs cannot write it. move preserves duration
(14:00–15:00 → move @1 …T09:00 → 09:00–10:00); lengthen/shorten take a
relative delta and build no timestamp at all. The one absolute value the panel
composes is base.date .. "T" .. want.at (timew.luau:415), and date is a
substring of start — one regex, nested capture, parse.luau:161-162 — so
the two cannot disagree and the day can only ever be the row's own. I looked
specifically for a wrong-day escape near midnight (23:30 start running into the
next day) and on an interval longer than 24 h: there is none. want.at has been
through Parse.timeOfDay, so it is HH:MM and carries no date of its own.

Command order: proven necessary, and the shipped one is right. Real
byStart script, real database, three intervals, editing the middle one.

shipped  retag → lengthen → move :  Retagged @2 as "deep work"
                                    Lengthened @2 by 0:10:00
                                    Moved @2 to 2026-09-01T09:30:00
                                    → 09:30:00 10:40:00 "deep work"   ✓ all three

reversed move → lengthen → retag :  Moved @2 to 2026-09-01T09:30:00
                                    GONE  (exit 1)
                                    GONE  (exit 1)
                                    → 09:30:00 10:30:00 beta          ✗ two lost

${4:+"$4"} behaves. Invoked as the host does — /bin/sh -c "$SCRIPT" sh <verb> <start> <missing> [<arg>], with a timew shim printing its argv:

call argv reaching timew
three args (continue/delete) <continue> <@2> — 2 elements
four args, arg = "" <continue> <@2> — 2 elements, nothing added
de"ep $HOME \id` * work␊second line` <retag> <@2> <de"ep $HOME \id` * work␊second line>`
* <retag> <@2> <*> — not globbed

One element, byte-intact, $HOME unexpanded, backticks unexecuted, no field
splitting. Identical under dash, so the idiom is not a bash accident.

run is behaviourally unchanged. For #cmds == 1: success → apply →
pokeBar → refresh; failure → pokeBar → refresh with no apply. Same
calls in the same order as the body it replaced, and undoable ~= false still
defaults true and is still only set after a command that landed.

Layout arithmetic checks out, and the 30 px is exact. Style::spaceMd = 12.0F (style.h:25) + kTextInnerInset = 3.0F (input.cpp:54), applied both
sides at input.cpp:878-880 — width - textInset - rightInset. LIST_W = 305
− 8 − 14 = 283; line one spends 68+68+24+24 plus four 4 px gaps = 200, leaving
the spacer 83. Nothing on that line can clip.

Cited lines, spot-checked against the tree — input.h:231 is
m_horizontalPadding = Style::spaceMd, input.cpp:54 is kTextInnerInset,
input.cpp:878-880 is the three-line viewport computation,
plugin_bindings.cpp:454-459 is uiNodePath, plugin_panel.cpp:419-422 is id
then context. All five exact. One is not; see A3.


Blocking

B1 — editBase never reconciles with the row, and Stop turns that into a wrong write

panel.luau:905 · panel.luau:833 · timew.luau:398

The baseline is written once in beginEdit and cleared only by cancel or a
successful submit. Nothing in between re-seeds it — and the editor survives
every refresh() the panel issues, because run → pokeBar → refresh never
touches state.editing. The comment at panel.luau:1288 believes otherwise;
see A4.

The reachable gesture is four steps on the row the PR itself calls the one most
likely to be edited:

  1. right-click the running interval → Edit time and tag…
  2. click Stop (or stop from the bar widget, or from a terminal)
  3. type a duration
  4. confirm

Driven through the panel's own nodes:

[probe] tracking = true  editor open = true  base.seconds = 1800  field = "0:30"
[probe] after Stop: editor still open = true  row is now "0:45"
                    base.seconds still 1800   field still "0:30"
[probe] emitted: timew lengthen @id ARG=PT1800S

The row is 2700 s, the user asked for 3600 s, the correct delta is PT900S. The
panel sends PT1800S and the interval becomes 1:15. No toast — timew did
exactly what it was told.

lengthen/shorten on an open interval are refused by name ("Cannot lengthen
open interval @1", verified), which is what masks this while tracking. Stopping
lifts the refusal and leaves the stale number in place. Any refetch that changes
the edited row's duration without changing its start does the same; the active
row is simply the one that does it every second.

The fix keeps the baseline for the decision and moves only the arithmetic. The
baseline is what makes an untouched field send nothing — that must not change —
but the size of the delta belongs to the row as it now stands:

	-- The baseline decides what changed; the row as it stands now decides by how
	-- much. `start` and `date` are the same value either way -- this is found by
	-- `base.start` -- so only `seconds` moves, and a row that has gone falls back
	-- to the baseline, which is the argument the miss message is already for.
	local live = base
	for _, row in ipairs(state.shown) do
		if row.start == base.start then
			live = row
			break
		end
	end
	runChain(Timew.cmd.edits(live, want, noctalia.tr("panel.menu_gone")), true, function(ok)

B2 — onOpen resets the card's field and not the editor's three

panel.luau:1523

apply({ draft = "", draftKey = state.draftKey + 1, selected = 1, mode = "day", focusOnce = true })

draft is cleared here because a half-typed tag must not survive a close. The
editor's four fields and its baseline are not, and there is no onClose in the
plugin API to catch them elsewhere. Reopening the panel:

[probe] editor open, typed: 09:15  2:00  half-typed     editBase.seconds = 1800
[probe] AFTER REOPEN: state.editing  = "2026-08-31T12:00:00"
[probe] AFTER REOPEN: editBase.start = "2026-08-31T12:00:00"
[probe] AFTER REOPEN: editor IS drawn, fields = "09:15"|"2:00"|"half-typed"
[probe] contrast: state.draft = ""

A row comes back already in edit mode holding text from a previous session, with
a confirm button beside it and a baseline of unbounded age. Everything B1
describes applies, with no bound on the staleness.

	apply({
		draft = "",
		draftKey = state.draftKey + 1,
		selected = 1,
		mode = "day",
		focusOnce = true,
		-- The editor closes with the panel, for the reason `draft` clears above:
		-- a half-typed edit is not state an open is entitled to inherit, and its
		-- baseline describes a row nobody has looked at since.
		editing = "",
		editBase = {},
		editAt = "",
		editDuration = "",
		editTag = "",
	})

Advisory

A1 — a multi-tag row seeds literal quote characters into retag

panel.luau:817 · parse.luau:180

Parse.unquote strips only a string that both starts and ends with ". A
two-tag interval where one tag has a space does neither:

$ timew get dom.tracked.1.tags
"deep work" review
> Parse.interval(...).tag  ==  "\"deep work\" review"    -- quotes are in the value

That is what the tag field opens with and what retag receives if the user
touches it:

$ timew retag @2 '"deep work" review'
Note: '"\"deep work\" review"' is a new tag.
$ timew get dom.tracked.2.tags
"\"deep work\" review"

The README's limit says a multi-tag interval "flattens". It also permanently
gains a tag whose name contains escaped quotes, which is a different and worse
thing. Either say so, or refuse the retag when row.tag is not a single tag —
Parse.unquote(row.tag) == row.tag and string.find(row.tag, '"', 1, true) is the
test, and it costs one branch in beginEdit.

A2 — on a 12-hour shell the field disagrees with the row above it

panel.luau:820

state.shown is the model's raw matchedRows, so row.at is the 24-hour
17:48 while the row three pixels away renders 5:48 PM through Fmt.hhmm.
Parse.timeOfDay accepts only HH:MM, so a 12-hour user typing what they see
is rejected. The toast does name the format, so this is a wart rather than a
trap — but the two surfaces are one glance apart.

A3 — script_runtime_types.h:39-42 names the wrong lines

panel.luau:801 (and the same citation at :753)

The comment reads "Action, Header, Separator (script_runtime_types.h:39-42)".
Those three enumerators are at 33-35, inside ScriptContextMenuItemKind
(32-36); 39-42 is ScriptContextMenuItem's four fields — kind, id, label,
enabled. The argument is sound either way, and 38-43 does support the stronger
half of it ("an item carries these four things and no others, so it cannot host
an input"). The parenthetical should be 32-36, or 38-43 if the item's shape
is what is meant.

A4 — "a command that does refetch closes the editor on success" is false

panel.luau:1288-1289

run is reached by start, stop, startTag, startRecent, continue, delete and
undo. Every one of them refetches and none closes the editor — the B1 probe
shows it surviving a full refresh() after Stop. Only submitEdit's own
done(ok) closes it. The paragraph's conclusion still stands (typed text is
re-supplied from the model, only the caret moves), but the reason given is not
the mechanism. Worth correcting, because it is the sentence that made the
staleness in B1 look impossible.

A5 — a duration past 100 hours outgrows the field

panel.luau:129

EDIT_TIME_W = 68 leaves 38 px of viewport. The comment budgets for 26:00,
five glyphs. Fmt.span emits 100:00 — six — for any interval of 100 h or
more, which timew will happily hold (lengthen @1 PT359996400S was accepted and
ran to 2038-01-28). The field is the one the user is about to correct, so it
clipping is the same failure the 52 px screenshot caught, one order of magnitude
out.

A6 — the editor cannot scroll itself into view

panel.luau:1329-1330

The editor is 24 + 4 + 24 = 52 px where the row was 20, so opening one on the
last visible row pushes 32 px past the fold. kScroll exposes stickToBottom
and scrollToBottomRev and no scroll offset at all
(ui_tree_reconciler.cpp:496-499), so there is nothing to fix it with — this is
a limit to name in the README beside the other three, not a bug to work around.


Mutation survivors

The author's 31 all die. I found eight more that live, and they cluster in
one place: the editor's geometry and its lifecycle guards. Every mutation was
applied to a fresh copy of the tree and judged on luau test.luau alone.

# mutation why it matters
M1 EDIT_H 24 → 20 the one layout number whose comment argues hardest for itself — "20 px clips its text" — is the one not pinned. EDIT_TIME_W and EDIT_BTN both die.
M2 focus = true added to all three editor fields the "No focus request" paragraph at :1298-1302 argues a real property; three nodes plus the card's focusOnce can fight for the caret and the suite is silent.
M3 editor column gap 4 → 0 the two-line split is the feature; the gap between the lines is unpinned.
M4 editor row gap 4 → 0 same, on the line the width arithmetic is computed for.
M5 confirm button variant primary → ghost confirm and cancel become indistinguishable.
M6 cancelEdit leaves editBase populated inert today only because editing == "" is checked first.
M7 submitEdit drops the base.start == nil guard the other half of the same pair.
M8 runChain fires done(ok) after pokeBar/refresh the ordering the function's comment is built around.

M1 is the one worth acting on: it is the number the PR body defends at length
and the only one of the three the tree does not hold to account. M2 is the one
with the most behaviour behind it.

Everything else I threw held: timeOfDay without its m > 59 bound, without
its $ anchor, with h > 24, without the %02d normalisation; spanSeconds
with an optional s, with the 1:0203 pattern the comment warns about;
cmd.edits with PT<n>M, with a zero-delta command, with a space for the T;
byStart with arg un-defaulted and with $4 unquoted inside the expansion;
runChain refetching on an empty list, marking undoable after a failure,
closing the editor on failure; beginEdit with a zeroed baseline, a dropped
date, a frozen editKey; buildList drawing both the editor and the row;
isEditing pinned false; onIpc defaulting to row 0; the dropped placeholder,
the dropped spacer, the dropped onSubmit.

One more thing, not a finding: 0:00 in the duration field sends
shorten @id PT<base.seconds>S, and timew accepts it — the interval becomes
zero-length rather than being refused. It is what the user typed and one undo
away, so I would leave it; it is worth a line in the limits list beside the
cleared tag field.


Both corrections above were applied to a scratch copy and run, rather than
proposed on paper:

[B1 fixed]      timew lengthen ARG=PT900S            (was PT1800S)
[B1 regression] untouched submit on a row that ticked: 0 commands
[B2 fixed]      after reopen: editing = ""  editor drawn = false  editAt = ""
                                                    13 states, 1870 field comparisons — ok

The regression line is the one that matters: the baseline still decides that an
untouched field on a row whose duration moved under it sends nothing, which is
the property editBase exists for. Only the arithmetic moved.

## Review — source half (`parse.luau`, `timew.luau`, `panel.luau`, `viewmodel.luau`, `en.json`) **REQUEST_CHANGES.** One reachable silent wrong write, plus the state leak that widens it. Everything else is advisory. `test.luau` is the sibling reviewer's; I only read it. Gates on the branch as shipped: `13 states, 1870 field comparisons`, `ok`; `0 errors, 0 warnings`. Everything below was run against my own `TIMEWARRIORDB=/tmp/rs2-timewdb` and a `/tmp` copy of the tree. --- ### The claims hold. Checked one at a time, not taken on trust. **The 25-hour bug is real and worse than the PR states.** Today is 2026-09-04: ``` $ timew get dom.tracked.1.start dom.tracked.1.end 2026-09-01T14:00:00 2026-09-01T15:00:00 $ timew modify end @1 15:30 # no output, exit 0 2026-09-01T14:00:00 2026-09-04T15:30:00 ``` 73.5 hours, silently. The interval's own date never enters it — the bare time resolves on *today*, so the error grows with the row's age. **And the three verbs cannot write it.** `move` preserves duration (`14:00–15:00` → `move @1 …T09:00` → `09:00–10:00`); `lengthen`/`shorten` take a relative delta and build no timestamp at all. The one absolute value the panel composes is `base.date .. "T" .. want.at` (`timew.luau:415`), and `date` is a *substring* of `start` — one regex, nested capture, `parse.luau:161-162` — so the two cannot disagree and the day can only ever be the row's own. I looked specifically for a wrong-day escape near midnight (23:30 start running into the next day) and on an interval longer than 24 h: there is none. `want.at` has been through `Parse.timeOfDay`, so it is `HH:MM` and carries no date of its own. **Command order: proven necessary, and the shipped one is right.** Real `byStart` script, real database, three intervals, editing the middle one. ``` shipped retag → lengthen → move : Retagged @2 as "deep work" Lengthened @2 by 0:10:00 Moved @2 to 2026-09-01T09:30:00 → 09:30:00 10:40:00 "deep work" ✓ all three reversed move → lengthen → retag : Moved @2 to 2026-09-01T09:30:00 GONE (exit 1) GONE (exit 1) → 09:30:00 10:30:00 beta ✗ two lost ``` **`${4:+"$4"}` behaves.** Invoked as the host does — `/bin/sh -c "$SCRIPT" sh <verb> <start> <missing> [<arg>]`, with a `timew` shim printing its argv: | call | argv reaching timew | |---|---| | three args (`continue`/`delete`) | `<continue> <@2>` — 2 elements | | four args, `arg` = `""` | `<continue> <@2>` — 2 elements, nothing added | | `de"ep $HOME \`id\` * work␊second line` | `<retag> <@2> <de"ep $HOME \`id\` * work␊second line>` | | `*` | `<retag> <@2> <*>` — not globbed | One element, byte-intact, `$HOME` unexpanded, backticks unexecuted, no field splitting. Identical under `dash`, so the idiom is not a bash accident. **`run` is behaviourally unchanged.** For `#cmds == 1`: success → `apply` → `pokeBar` → `refresh`; failure → `pokeBar` → `refresh` with no `apply`. Same calls in the same order as the body it replaced, and `undoable ~= false` still defaults true and is still only set after a command that landed. **Layout arithmetic checks out, and the 30 px is exact.** `Style::spaceMd = 12.0F` (`style.h:25`) + `kTextInnerInset = 3.0F` (`input.cpp:54`), applied both sides at `input.cpp:878-880` — `width - textInset - rightInset`. `LIST_W` = 305 − 8 − 14 = 283; line one spends 68+68+24+24 plus four 4 px gaps = 200, leaving the spacer 83. Nothing on that line can clip. **Cited lines, spot-checked against the tree** — `input.h:231` is `m_horizontalPadding = Style::spaceMd`, `input.cpp:54` is `kTextInnerInset`, `input.cpp:878-880` is the three-line viewport computation, `plugin_bindings.cpp:454-459` is `uiNodePath`, `plugin_panel.cpp:419-422` is id then context. All five exact. One is not; see A3. --- ## Blocking ### B1 — `editBase` never reconciles with the row, and Stop turns that into a wrong write `panel.luau:905` · `panel.luau:833` · `timew.luau:398` The baseline is written once in `beginEdit` and cleared only by cancel or a successful submit. Nothing in between re-seeds it — and the editor survives every `refresh()` the panel issues, because `run` → `pokeBar` → `refresh` never touches `state.editing`. The comment at `panel.luau:1288` believes otherwise; see A4. The reachable gesture is four steps on the row the PR itself calls the one most likely to be edited: 1. right-click the running interval → **Edit time and tag…** 2. click **Stop** (or stop from the bar widget, or from a terminal) 3. type a duration 4. confirm Driven through the panel's own nodes: ``` [probe] tracking = true editor open = true base.seconds = 1800 field = "0:30" [probe] after Stop: editor still open = true row is now "0:45" base.seconds still 1800 field still "0:30" [probe] emitted: timew lengthen @id ARG=PT1800S ``` The row is 2700 s, the user asked for 3600 s, the correct delta is `PT900S`. The panel sends `PT1800S` and the interval becomes **1:15**. No toast — timew did exactly what it was told. `lengthen`/`shorten` on an open interval are refused by name ("Cannot lengthen open interval @1", verified), which is what masks this while tracking. Stopping lifts the refusal and leaves the stale number in place. Any refetch that changes the edited row's duration without changing its start does the same; the active row is simply the one that does it every second. The fix keeps the baseline for the decision and moves only the arithmetic. The baseline is what makes an untouched field send nothing — that must not change — but the *size* of the delta belongs to the row as it now stands: ```suggestion -- The baseline decides what changed; the row as it stands now decides by how -- much. `start` and `date` are the same value either way -- this is found by -- `base.start` -- so only `seconds` moves, and a row that has gone falls back -- to the baseline, which is the argument the miss message is already for. local live = base for _, row in ipairs(state.shown) do if row.start == base.start then live = row break end end runChain(Timew.cmd.edits(live, want, noctalia.tr("panel.menu_gone")), true, function(ok) ``` ### B2 — `onOpen` resets the card's field and not the editor's three `panel.luau:1523` ```lua apply({ draft = "", draftKey = state.draftKey + 1, selected = 1, mode = "day", focusOnce = true }) ``` `draft` is cleared here because a half-typed tag must not survive a close. The editor's four fields and its baseline are not, and there is no `onClose` in the plugin API to catch them elsewhere. Reopening the panel: ``` [probe] editor open, typed: 09:15 2:00 half-typed editBase.seconds = 1800 [probe] AFTER REOPEN: state.editing = "2026-08-31T12:00:00" [probe] AFTER REOPEN: editBase.start = "2026-08-31T12:00:00" [probe] AFTER REOPEN: editor IS drawn, fields = "09:15"|"2:00"|"half-typed" [probe] contrast: state.draft = "" ``` A row comes back already in edit mode holding text from a previous session, with a confirm button beside it and a baseline of unbounded age. Everything B1 describes applies, with no bound on the staleness. ```suggestion apply({ draft = "", draftKey = state.draftKey + 1, selected = 1, mode = "day", focusOnce = true, -- The editor closes with the panel, for the reason `draft` clears above: -- a half-typed edit is not state an open is entitled to inherit, and its -- baseline describes a row nobody has looked at since. editing = "", editBase = {}, editAt = "", editDuration = "", editTag = "", }) ``` --- ## Advisory ### A1 — a multi-tag row seeds literal quote characters into `retag` `panel.luau:817` · `parse.luau:180` `Parse.unquote` strips only a string that both starts and ends with `"`. A two-tag interval where one tag has a space does neither: ``` $ timew get dom.tracked.1.tags "deep work" review > Parse.interval(...).tag == "\"deep work\" review" -- quotes are in the value ``` That is what the tag field opens with and what `retag` receives if the user touches it: ``` $ timew retag @2 '"deep work" review' Note: '"\"deep work\" review"' is a new tag. $ timew get dom.tracked.2.tags "\"deep work\" review" ``` The README's limit says a multi-tag interval "flattens". It also permanently gains a tag whose *name* contains escaped quotes, which is a different and worse thing. Either say so, or refuse the `retag` when `row.tag` is not a single tag — `Parse.unquote(row.tag) == row.tag and string.find(row.tag, '"', 1, true)` is the test, and it costs one branch in `beginEdit`. ### A2 — on a 12-hour shell the field disagrees with the row above it `panel.luau:820` `state.shown` is the model's raw `matchedRows`, so `row.at` is the 24-hour `17:48` while the row three pixels away renders `5:48 PM` through `Fmt.hhmm`. `Parse.timeOfDay` accepts only `HH:MM`, so a 12-hour user typing what they see is rejected. The toast does name the format, so this is a wart rather than a trap — but the two surfaces are one glance apart. ### A3 — `script_runtime_types.h:39-42` names the wrong lines `panel.luau:801` (and the same citation at `:753`) The comment reads "Action, Header, Separator (script_runtime_types.h:39-42)". Those three enumerators are at **33-35**, inside `ScriptContextMenuItemKind` (32-36); 39-42 is `ScriptContextMenuItem`'s four fields — `kind`, `id`, `label`, `enabled`. The argument is sound either way, and 38-43 does support the stronger half of it ("an item carries these four things and no others, so it cannot host an input"). The parenthetical should be `32-36`, or `38-43` if the item's shape is what is meant. ### A4 — "a command that does refetch closes the editor on success" is false `panel.luau:1288-1289` `run` is reached by start, stop, `startTag`, `startRecent`, continue, delete and undo. Every one of them refetches and none closes the editor — the B1 probe shows it surviving a full `refresh()` after Stop. Only `submitEdit`'s own `done(ok)` closes it. The paragraph's conclusion still stands (typed text is re-supplied from the model, only the caret moves), but the reason given is not the mechanism. Worth correcting, because it is the sentence that made the staleness in B1 look impossible. ### A5 — a duration past 100 hours outgrows the field `panel.luau:129` `EDIT_TIME_W = 68` leaves 38 px of viewport. The comment budgets for `26:00`, five glyphs. `Fmt.span` emits `100:00` — six — for any interval of 100 h or more, which timew will happily hold (`lengthen @1 PT359996400S` was accepted and ran to 2038-01-28). The field is the one the user is about to correct, so it clipping is the same failure the 52 px screenshot caught, one order of magnitude out. ### A6 — the editor cannot scroll itself into view `panel.luau:1329-1330` The editor is 24 + 4 + 24 = 52 px where the row was 20, so opening one on the last visible row pushes 32 px past the fold. `kScroll` exposes `stickToBottom` and `scrollToBottomRev` and no scroll offset at all (`ui_tree_reconciler.cpp:496-499`), so there is nothing to fix it with — this is a limit to name in the README beside the other three, not a bug to work around. --- ## Mutation survivors The author's 31 all die. I found **eight more that live**, and they cluster in one place: the editor's geometry and its lifecycle guards. Every mutation was applied to a fresh copy of the tree and judged on `luau test.luau` alone. | # | mutation | why it matters | |---|---|---| | M1 | `EDIT_H` 24 → **20** | the one layout number whose comment argues hardest for itself — "20 px clips its text" — is the one not pinned. `EDIT_TIME_W` and `EDIT_BTN` both die. | | M2 | `focus = true` added to all three editor fields | the "No focus request" paragraph at `:1298-1302` argues a real property; three nodes plus the card's `focusOnce` can fight for the caret and the suite is silent. | | M3 | editor column `gap` 4 → 0 | the two-line split is the feature; the gap between the lines is unpinned. | | M4 | editor row `gap` 4 → 0 | same, on the line the width arithmetic is computed for. | | M5 | confirm button `variant` primary → ghost | confirm and cancel become indistinguishable. | | M6 | `cancelEdit` leaves `editBase` populated | inert today only because `editing == ""` is checked first. | | M7 | `submitEdit` drops the `base.start == nil` guard | the other half of the same pair. | | M8 | `runChain` fires `done(ok)` after `pokeBar`/`refresh` | the ordering the function's comment is built around. | M1 is the one worth acting on: it is the number the PR body defends at length and the only one of the three the tree does not hold to account. M2 is the one with the most behaviour behind it. Everything else I threw held: `timeOfDay` without its `m > 59` bound, without its `$` anchor, with `h > 24`, without the `%02d` normalisation; `spanSeconds` with an optional `s`, with the `1:0203` pattern the comment warns about; `cmd.edits` with `PT<n>M`, with a zero-delta command, with a space for the `T`; `byStart` with `arg` un-defaulted and with `$4` unquoted inside the expansion; `runChain` refetching on an empty list, marking `undoable` after a failure, closing the editor on failure; `beginEdit` with a zeroed baseline, a dropped date, a frozen `editKey`; `buildList` drawing both the editor and the row; `isEditing` pinned false; `onIpc` defaulting to row 0; the dropped placeholder, the dropped spacer, the dropped `onSubmit`. One more thing, not a finding: `0:00` in the duration field sends `shorten @id PT<base.seconds>S`, and timew accepts it — the interval becomes zero-length rather than being refused. It is what the user typed and one `undo` away, so I would leave it; it is worth a line in the limits list beside the cleared tag field. --- Both corrections above were applied to a scratch copy and run, rather than proposed on paper: ``` [B1 fixed] timew lengthen ARG=PT900S (was PT1800S) [B1 regression] untouched submit on a row that ticked: 0 commands [B2 fixed] after reopen: editing = "" editor drawn = false editAt = "" 13 states, 1870 field comparisons — ok ``` The regression line is the one that matters: the baseline still decides that an untouched field on a row whose duration moved under it sends nothing, which is the property `editBase` exists for. Only the arithmetic moved.
Author
Owner

Review — the test half (test.luau), PR #21

REQUEST_CHANGES. The suite is strong on the command builder and on the
chain's control flow, and I could not break either. It is weak in one place that
matters: the subtlety the PR body leads with — "the editor keeps a baseline
captured at open" — has no assertion that can tell the baseline from the live
row.
Two more user-facing paths are asserted to exist and never exercised.

Method

94 source mutations applied to a throwaway copy, each against a pristine tree
with the baseline re-verified before every run. 27 survived, 26 of them distinct.
Three are equivalent mutants and one is pre-existing coverage, leaving 22 that
change observable behaviour and still print ok.
They cluster into three
blocking gaps and five advisories. Timew claims are checked against a real 1.9.1 database in a
scratch TIMEWARRIORDB, never the real one.

Every fix below is written out; I ran the strengthened suite and it passes on
the current source, and it kills all ten of the survivors I quote.


Blocking

1. The baseline is never distinguished from the live row

test.luau:3803-3820 — "The drift the baseline exists to prevent". Its comment
makes two claims and the code only checks one. It checks the flooring: the
field shows 0:01 for a 62-second interval, and an unchanged submit must not
shorten it by two seconds. It does not check the staleness — that
submitEdit reads state.editBase rather than the row as it stands now.

It cannot, as written. In that block state.editBase.seconds and
state.shown[3].seconds are both 62. Every other editor test has the same
property: the list under an open editor never changes duration. Even the refetch
in "Identity, not position" (test.luau:3764-3774) replays the same PT30M and
PT1M2S.

So this mutation passes the whole suite, 13 states, 1870 field comparisons,
ok:

-- timew/panel.luau, submitEdit
local base = state.editBase
for _, r in ipairs(state.shown) do if r.start == state.editing then base = r end end

Driving the mutated panel through its own nodes: open the editor on the 12:00
row, land a refetch that reports it as PT45M, then submit untouched. The
real source sends nothing. The mutant sends shorten … PT900S — a resize
nobody asked for, on data the user did not touch. That is the exact failure the
PR body describes.

It is not a hypothetical path. Against timewarrior 1.9.1 the running interval is
in the same walk Timew.cmd.intervalsForDay reads, and its duration ticks:

2026-09-04T12:50:20 PT0S "live probe"     ← first read
2026-09-04T12:50:20 PT1S "live probe"     ← one second later

assert(panel.state.editBase.seconds == 62) at test.luau:3812 pins that the
baseline is captured. Nothing pins that it is used.

	-- And the baseline is the one captured at open, not whatever the row says
	-- now. A refetch while the editor is up can change the interval underneath it
	-- -- the active row's duration does it every second -- and an untouched
	-- submit must still send nothing.
	open({ tags = TAGS5, days = D3, dayRows = DAY3 })
	panelEnv.onActivate("edit", START2)
	panelEnv.onIpc("day", 1)
	takeJob(ANY_DAY).cb({
		stdout = "2026-08-31T17:48:53 PT2H focus\n2026-08-31T12:00:00 PT45M admin\n2026-08-31T09:00:00 PT1M2S focus\n",
	})
	assert(panel.state.shown[2].seconds == 2700, "the row grew underneath the open editor")
	assert(panel.state.editBase.seconds == 1800, "and the baseline did not follow it")
	panelJobs = {}
	fire((buttons()), "onClick", "confirm an untouched editor whose row grew underneath")
	assert(#panelJobs == 0, "an untouched submit measures against the baseline, not the row: got " .. #panelJobs)
	assert(panel.state.editing == "", "and it closes, having sent nothing")

2. onSubmit is asserted to exist and never fired

test.luau:3540:

assert(type(node.props.onSubmit) == "function", "field " .. i .. " has no onSubmit, so Enter does nothing")

The message names the failure the assert cannot see. Three mutations survive:

mutation what a user gets
onSubmit = function() end Enter does nothing
onSubmit = cancelEdit Enter discards the edit
onSubmit on the start field only Enter works in one field of three

The file already has the right pattern — enterSearch() at test.luau:1593 and
enterDraft() at test.luau:1601 both fire onSubmit, and test.luau:2983
asserts what it did. The editor section is the one place that stops at the type.

Compare against the confirm button's own handler, and fire it once:

	local okBtn, cancelBtn = buttons()
	-- Every field is wired both ways, and the two buttons carry their handlers.
	for i, node in ipairs({ atField, durField, tagField }) do
		assert(type(node.props.onChange) == "function", "field " .. i .. " has no onChange, so typing in it goes nowhere")
		assert(node.props.onSubmit == okBtn.props.onClick, "field " .. i .. "'s Enter is not the confirm button's handler")
		assert(type(node.props.key) == "string" and node.props.key ~= "", "field " .. i .. " is unkeyed, so a tick rebuilds the caret")
	end

plus, on the live path:

	-- Enter, fired rather than merely typed.
	panelEnv.onActivate("edit", START2)
	fire(select(3, fields()), "onChange", "the tag field", "deep work")
	panelJobs = {}
	fire((fields()), "onSubmit", "Enter in the start field")
	assert(#panelJobs == 1, "Enter submits the editor, got " .. #panelJobs)
	takeJob(T.cmd.byStart("retag", START2, GONE, "deep work")).cb({ exitCode = 0, stdout = "", stderr = "" })
	takeJob(T.cmd.activeInterval()).cb({ stdout = "" })
	takeJob(T.cmd.tags()).cb({ stdout = TAGS5 })
	takeJob(daysJob()).cb({ stdout = D3 })
	takeJob(ANY_DAY).cb({ stdout = DAY3 })
	panelJobs = {}

3. move is only ever composed for a time earlier than the row's own

Both move assertions use 09:15 against a 12:00 row — the unit one at
test.luau:805-812 and the live one at test.luau:3654-3658. So a date bug that
only bites when the start is moved forward survives:

-- timew/timew.luau, Timew.cmd.edits
missing, (if want.at > base.at then "1970-01-01" else base.date) .. "T" .. want.at

ok. This is the same class as the 25-hour bug the whole verb set was chosen to
foreclose, and the suite is currently blind to half of it. One line, beside the
existing case:

argvIs(
	editCmds({ at = "18:45" })[1],
	T.cmd.byStart("move", EDIT_BASE.start, "gone", "2026-08-31T18:45"),
	"a start moved later takes the row's own date too, which the earlier case cannot show"
)

While you are there: a related behaviour is undecided rather than merely
untested. Driving the real editor on a 2026-08-31T23:50 row and typing 00:10
produces move @id 2026-08-31T00:10, and timew obliges — verified on 1.9.1,
Moved @3 to 2026-08-30T00:10:00, the interval jumps 23h40m backwards. The
row's own date is always right by construction, which is the point; whether it
is right by intent at that boundary is a question the suite does not ask. Worth
a test either way, so the answer is written down. (Raised with the source
reviewer as well.)


Advisory

4. EDIT_H is unasserted, and the height it exists to avoid survives

timew/panel.luau:105-128 says plainly that 20 px clips its text and that 24
is the control tier's minimum. The suite asserts four widths and no height:
EDIT_H = 24 → 20 survives, → 1 survives, and deleting height from
buildEditRow's field() survives. The width asserts show the shape of the
right fix; the height just needs one of the same.

	for i, f in ipairs({ atField, durField, tagField }) do
		assert(f.props.height >= 24, "field " .. i .. " is " .. tostring(f.props.height) .. " px tall, which clips its text")
	end

5. The time-field floor is set below the width the PR itself computes

test.luau:3572-3577 allows any field with 25 px of viewport. The comment two
lines above, and timew/panel.luau:113-127, both put "08:00" at about 34 px at
fontSize 12. So EDIT_TIME_W = 56 and = 55 both survive — widths the PR's own
arithmetic calls clipping. The guard catches the 52 that the screenshot caught
and nothing between. Raising the floor to the number already written down costs
nothing and closes the gap (68 still passes, 63 does not):

	for _, f in ipairs({ atField, durField }) do
		assert(
			f.props.width - INPUT_INSET >= 34,
			"a time field has " .. (f.props.width - INPUT_INSET) .. " px of viewport, under the 34 px \"08:00\" needs"
		)
	end

The budget assert at test.luau:3564 is fine — it is not the tautology it looks
like, it fails at EDIT_TIME_W = 130 — but it carries 83 px of slack, so
EDIT_BTN = 48, a zero gap and a spacer with no flexGrow all pass it. Those
are cosmetic and I would not hold the PR for them.

6. The three input keys are never asserted distinct

test.luau:3541 asserts each key is a non-empty string; 3889-3910 assert they
are stable across ticks and fresh on reopen. None asserts they differ from each
other, so key = "edit-" .. m.editKey for all three survives. Two of them are
siblings in the same row, and a handler name is the node's path
(plugin_bindings.cpp:454-459) — which is the scheme this file polices in
sweep and in the closure-identity section below the editor. One line:

	assert(
		atField.props.key ~= durField.props.key
			and durField.props.key ~= tagField.props.key
			and atField.props.key ~= tagField.props.key,
		"two editor fields share a key, so their generated handler paths collide"
	)

7. 1870 field comparisons means nothing for this feature

Worth saying plainly, because the PR body quotes the figure as a gate:

  • main at 2029e99 reports 13 states, 1870 field comparisons — byte for
    byte the same line. The matrix gained zero comparisons here. same(
    appears 65 times before and 65 times after.
  • The string isEditing does not appear anywhere in test.luau. All 13
    check() calls run at lines 2419-2703, before the editor section at 3457, so
    state.editing is "", editKey is 0 and editAt/editDuration/editTag
    are "" in every one of them. compare() never reads a row's isEditing and
    never reads the four new top-level model fields.
  • There is no model census. The state census at test.luau:4072-4079 is
    two-way and does bite — adding an undeclared field to state fails at
    "a patch grew state a field no declaration has". The model has no
    counterpart: Viewmodel.build growing a junk top-level field, or a junk
    per-row field, passes.

The feature is covered, but entirely by the editor section driving the tree —
which is why dropping editAt from the model fails at
"the start field opens on the row's own time" rather than at any census. That
is fine; the figure just should not be read as covering it. A MODEL_FIELDS
census mirroring FIELDS would be the cheap way to make it mean something.

8. Untested behaviour worth naming

Checked by driving the real panel, all correct today, none asserted:

  • A multi-tag row. 2026-08-31T12:00:00 PT30M "deep work" admin parses to
    the tag "deep work" admin — quote characters and all — and that is the
    string the field is seeded with. Unchanged, it submits nothing (right).
    Changed, it retags to one flat tag, which the README documents. The raw quotes
    in the field are not documented anywhere and nothing pins either half.
  • The running row is never edited. No test opens an editor on an open
    interval, so neither "its start is editable" nor "its duration is refused"
    (Cannot lengthen open interval @1, confirmed on 1.9.1) is exercised. Same
    fixture gap as #1.
  • All-time scope. Works — editBase.date is the row's own 2026-08-28, so
    move composes correctly across days — but nothing asserts it.
  • onConfigChanged with an editor open. commit() leaves editing and
    editKey alone, so the caret survives; unasserted.
  • Two editors at once is impossible by construction, but editRow()
    (test.luau:3477) returns the first column and #scrollNode().children == 3
    would still hold if a second row drew one. An assert on the count of columns
    inside editRow would close it for free.

What I tried to break and could not

Stated so the record is symmetric — these are the questions I went in expecting
to find something and did not:

  • Command order. A true move,retag,lengthen reversal fails at
    test.luau:833. Swapping retag with the resize fails too.
  • PT<n>S arithmetic. Measuring absolutely, dropping math.abs, comparing
    against 0 instead of base.seconds, and sending a zero delta are all killed.
    lengthen/shorten swapped is killed.
  • byStart's $4. Before the id, unquoted, and arg without the or ""
    are all killed.
  • The layout equality asserts at test.luau:3578 and 3582 compare tree
    values against tree values but are not tautologies: changing the editor
    column's width alone, or the tag field's alone, fails them.
  • Key stability. Bumping the key on every render fails at test.luau:3896.
  • paintsFor. An extra commit on open fails; a commit per keystroke fails.
  • Harness honesty. editRow() == nil is a real assertion everywhere it
    appears; fields() asserts non-nil at 3489; buttons() raises on a nil
    editor rather than passing. No fields() read follows an apply without an
    intervening repaint — the two that could have are preceded by
    panelEnv.update().
  • Queue discipline. Every #panelJobs == N in the new section is preceded by
    a panelJobs = {} on the triggering line, so none passes on a leftover.
    takeJob raises on a miss, so draining a refetch by needle does catch a
    panel that stopped refetching — a mutation removing the post-edit refresh fails
    at takeJob's own error. An extra command leaked only on the editor's success
    path is caught too. Three of the five drains do not seal with
    assert(#panelJobs == 0) the way 3713 and 3835 do; harmless as it stands,
    but sealing them is free.
  • The restructuring is clean. No assertion was dropped: the eight message
    differences against main are all the menu renumbering (items[3..5] →
    [4..6]), the byStart message gaining and no $4, the scroll-children
    rewording, and "the two that are always available" → "the three".
    assert( goes 479 → 607, fire( 13 → 53. One shown and one HOSTILE
    declaration each; shown moving to line 153 only widens its scope, and both
    the do block at 752 and the IIFE at 3457 execute — mutations die inside each.
  • submitEdit's state.editing == "" or base.start == nil guard is never
    exercised and removing it survives, but a stale handler resolves to nil in the
    shell (luau_host.cpp:3082-3083), so the branch is unreachable. Not a gap.
    Same for comparing editDuration untrimmed: Parse.spanSeconds trims and the
    zero-delta check absorbs it, so that mutant is equivalent.

Gates on the branch as it stands: nix shell nixpkgs#luau -c luau test.luau →
ok; noctalia plugins lint timew → 0 errors, 0 warnings.

## Review — the test half (`test.luau`), PR #21 **REQUEST_CHANGES.** The suite is strong on the command builder and on the chain's control flow, and I could not break either. It is weak in one place that matters: **the subtlety the PR body leads with — "the editor keeps a baseline captured at open" — has no assertion that can tell the baseline from the live row.** Two more user-facing paths are asserted to exist and never exercised. ### Method 94 source mutations applied to a throwaway copy, each against a pristine tree with the baseline re-verified before every run. **27 survived**, 26 of them distinct. Three are equivalent mutants and one is pre-existing coverage, leaving **22 that change observable behaviour and still print `ok`.** They cluster into three blocking gaps and five advisories. Timew claims are checked against a real 1.9.1 database in a scratch `TIMEWARRIORDB`, never the real one. Every fix below is written out; I ran the strengthened suite and it passes on the current source, and it kills all ten of the survivors I quote. --- ## Blocking ### 1. The baseline is never distinguished from the live row `test.luau:3803-3820` — "The drift the baseline exists to prevent". Its comment makes two claims and the code only checks one. It checks the **flooring**: the field shows `0:01` for a 62-second interval, and an unchanged submit must not shorten it by two seconds. It does not check the **staleness** — that `submitEdit` reads `state.editBase` rather than the row as it stands now. It cannot, as written. In that block `state.editBase.seconds` and `state.shown[3].seconds` are both `62`. Every other editor test has the same property: the list under an open editor never changes duration. Even the refetch in "Identity, not position" (`test.luau:3764-3774`) replays the same `PT30M` and `PT1M2S`. So this mutation passes the whole suite, `13 states, 1870 field comparisons`, `ok`: ```lua -- timew/panel.luau, submitEdit local base = state.editBase for _, r in ipairs(state.shown) do if r.start == state.editing then base = r end end ``` Driving the mutated panel through its own nodes: open the editor on the 12:00 row, land a refetch that reports it as `PT45M`, then submit **untouched**. The real source sends nothing. The mutant sends `shorten … PT900S` — a resize nobody asked for, on data the user did not touch. That is the exact failure the PR body describes. It is not a hypothetical path. Against timewarrior 1.9.1 the running interval is in the same walk `Timew.cmd.intervalsForDay` reads, and its duration ticks: ``` 2026-09-04T12:50:20 PT0S "live probe" ← first read 2026-09-04T12:50:20 PT1S "live probe" ← one second later ``` `assert(panel.state.editBase.seconds == 62)` at `test.luau:3812` pins that the baseline is *captured*. Nothing pins that it is *used*. ```suggestion -- And the baseline is the one captured at open, not whatever the row says -- now. A refetch while the editor is up can change the interval underneath it -- -- the active row's duration does it every second -- and an untouched -- submit must still send nothing. open({ tags = TAGS5, days = D3, dayRows = DAY3 }) panelEnv.onActivate("edit", START2) panelEnv.onIpc("day", 1) takeJob(ANY_DAY).cb({ stdout = "2026-08-31T17:48:53 PT2H focus\n2026-08-31T12:00:00 PT45M admin\n2026-08-31T09:00:00 PT1M2S focus\n", }) assert(panel.state.shown[2].seconds == 2700, "the row grew underneath the open editor") assert(panel.state.editBase.seconds == 1800, "and the baseline did not follow it") panelJobs = {} fire((buttons()), "onClick", "confirm an untouched editor whose row grew underneath") assert(#panelJobs == 0, "an untouched submit measures against the baseline, not the row: got " .. #panelJobs) assert(panel.state.editing == "", "and it closes, having sent nothing") ``` ### 2. `onSubmit` is asserted to exist and never fired `test.luau:3540`: ```lua assert(type(node.props.onSubmit) == "function", "field " .. i .. " has no onSubmit, so Enter does nothing") ``` The message names the failure the assert cannot see. Three mutations survive: | mutation | what a user gets | |---|---| | `onSubmit = function() end` | Enter does nothing | | `onSubmit = cancelEdit` | **Enter discards the edit** | | `onSubmit` on the start field only | Enter works in one field of three | The file already has the right pattern — `enterSearch()` at `test.luau:1593` and `enterDraft()` at `test.luau:1601` both fire `onSubmit`, and `test.luau:2983` asserts what it did. The editor section is the one place that stops at the type. Compare against the confirm button's own handler, and fire it once: ```suggestion local okBtn, cancelBtn = buttons() -- Every field is wired both ways, and the two buttons carry their handlers. for i, node in ipairs({ atField, durField, tagField }) do assert(type(node.props.onChange) == "function", "field " .. i .. " has no onChange, so typing in it goes nowhere") assert(node.props.onSubmit == okBtn.props.onClick, "field " .. i .. "'s Enter is not the confirm button's handler") assert(type(node.props.key) == "string" and node.props.key ~= "", "field " .. i .. " is unkeyed, so a tick rebuilds the caret") end ``` plus, on the live path: ```suggestion -- Enter, fired rather than merely typed. panelEnv.onActivate("edit", START2) fire(select(3, fields()), "onChange", "the tag field", "deep work") panelJobs = {} fire((fields()), "onSubmit", "Enter in the start field") assert(#panelJobs == 1, "Enter submits the editor, got " .. #panelJobs) takeJob(T.cmd.byStart("retag", START2, GONE, "deep work")).cb({ exitCode = 0, stdout = "", stderr = "" }) takeJob(T.cmd.activeInterval()).cb({ stdout = "" }) takeJob(T.cmd.tags()).cb({ stdout = TAGS5 }) takeJob(daysJob()).cb({ stdout = D3 }) takeJob(ANY_DAY).cb({ stdout = DAY3 }) panelJobs = {} ``` ### 3. `move` is only ever composed for a time *earlier* than the row's own Both `move` assertions use `09:15` against a `12:00` row — the unit one at `test.luau:805-812` and the live one at `test.luau:3654-3658`. So a date bug that only bites when the start is moved **forward** survives: ```lua -- timew/timew.luau, Timew.cmd.edits missing, (if want.at > base.at then "1970-01-01" else base.date) .. "T" .. want.at ``` `ok`. This is the same class as the 25-hour bug the whole verb set was chosen to foreclose, and the suite is currently blind to half of it. One line, beside the existing case: ```suggestion argvIs( editCmds({ at = "18:45" })[1], T.cmd.byStart("move", EDIT_BASE.start, "gone", "2026-08-31T18:45"), "a start moved later takes the row's own date too, which the earlier case cannot show" ) ``` While you are there: a related behaviour is undecided rather than merely untested. Driving the real editor on a `2026-08-31T23:50` row and typing `00:10` produces `move @id 2026-08-31T00:10`, and timew obliges — verified on 1.9.1, `Moved @3 to 2026-08-30T00:10:00`, the interval jumps 23h40m **backwards**. The row's own date is always right by construction, which is the point; whether it is right by intent at that boundary is a question the suite does not ask. Worth a test either way, so the answer is written down. (Raised with the source reviewer as well.) --- ## Advisory ### 4. `EDIT_H` is unasserted, and the height it exists to avoid survives `timew/panel.luau:105-128` says plainly that `20 px clips its text` and that 24 is the control tier's minimum. The suite asserts four widths and no height: `EDIT_H = 24 → 20` survives, `→ 1` survives, and deleting `height` from `buildEditRow`'s `field()` survives. The width asserts show the shape of the right fix; the height just needs one of the same. ```suggestion for i, f in ipairs({ atField, durField, tagField }) do assert(f.props.height >= 24, "field " .. i .. " is " .. tostring(f.props.height) .. " px tall, which clips its text") end ``` ### 5. The time-field floor is set below the width the PR itself computes `test.luau:3572-3577` allows any field with 25 px of viewport. The comment two lines above, and `timew/panel.luau:113-127`, both put `"08:00"` at about 34 px at fontSize 12. So `EDIT_TIME_W = 56` and `= 55` both survive — widths the PR's own arithmetic calls clipping. The guard catches the 52 that the screenshot caught and nothing between. Raising the floor to the number already written down costs nothing and closes the gap (68 still passes, 63 does not): ```suggestion for _, f in ipairs({ atField, durField }) do assert( f.props.width - INPUT_INSET >= 34, "a time field has " .. (f.props.width - INPUT_INSET) .. " px of viewport, under the 34 px \"08:00\" needs" ) end ``` The budget assert at `test.luau:3564` is fine — it is not the tautology it looks like, it fails at `EDIT_TIME_W = 130` — but it carries 83 px of slack, so `EDIT_BTN = 48`, a zero gap and a spacer with no `flexGrow` all pass it. Those are cosmetic and I would not hold the PR for them. ### 6. The three input keys are never asserted distinct `test.luau:3541` asserts each key is a non-empty string; `3889-3910` assert they are stable across ticks and fresh on reopen. None asserts they differ from each other, so `key = "edit-" .. m.editKey` for all three survives. Two of them are siblings in the same row, and a handler name is the node's path (`plugin_bindings.cpp:454-459`) — which is the scheme this file polices in `sweep` and in the closure-identity section below the editor. One line: ```suggestion assert( atField.props.key ~= durField.props.key and durField.props.key ~= tagField.props.key and atField.props.key ~= tagField.props.key, "two editor fields share a key, so their generated handler paths collide" ) ``` ### 7. `1870 field comparisons` means nothing for this feature Worth saying plainly, because the PR body quotes the figure as a gate: - `main` at `2029e99` reports **`13 states, 1870 field comparisons`** — byte for byte the same line. The matrix gained **zero** comparisons here. `same(` appears 65 times before and 65 times after. - The string `isEditing` **does not appear anywhere in `test.luau`.** All 13 `check()` calls run at lines 2419-2703, before the editor section at 3457, so `state.editing` is `""`, `editKey` is `0` and `editAt`/`editDuration`/`editTag` are `""` in every one of them. `compare()` never reads a row's `isEditing` and never reads the four new top-level model fields. - There is **no model census.** The state census at `test.luau:4072-4079` is two-way and does bite — adding an undeclared field to `state` fails at `"a patch grew state a field no declaration has"`. The model has no counterpart: `Viewmodel.build` growing a junk top-level field, or a junk per-row field, passes. The feature *is* covered, but entirely by the editor section driving the tree — which is why dropping `editAt` from the model fails at `"the start field opens on the row's own time"` rather than at any census. That is fine; the figure just should not be read as covering it. A `MODEL_FIELDS` census mirroring `FIELDS` would be the cheap way to make it mean something. ### 8. Untested behaviour worth naming Checked by driving the real panel, all correct today, none asserted: - **A multi-tag row.** `2026-08-31T12:00:00 PT30M "deep work" admin` parses to the tag `"deep work" admin` — quote characters and all — and that is the string the field is seeded with. Unchanged, it submits nothing (right). Changed, it retags to one flat tag, which the README documents. The raw quotes in the field are not documented anywhere and nothing pins either half. - **The running row is never edited.** No test opens an editor on an open interval, so neither "its start is editable" nor "its duration is refused" (`Cannot lengthen open interval @1`, confirmed on 1.9.1) is exercised. Same fixture gap as #1. - **All-time scope.** Works — `editBase.date` is the row's own `2026-08-28`, so `move` composes correctly across days — but nothing asserts it. - **`onConfigChanged` with an editor open.** `commit()` leaves `editing` and `editKey` alone, so the caret survives; unasserted. - **Two editors at once** is impossible by construction, but `editRow()` (`test.luau:3477`) returns the *first* column and `#scrollNode().children == 3` would still hold if a second row drew one. An `assert` on the count of columns inside `editRow` would close it for free. --- ## What I tried to break and could not Stated so the record is symmetric — these are the questions I went in expecting to find something and did not: - **Command order.** A true `move,retag,lengthen` reversal fails at `test.luau:833`. Swapping `retag` with the resize fails too. - **`PT<n>S` arithmetic.** Measuring absolutely, dropping `math.abs`, comparing against `0` instead of `base.seconds`, and sending a zero delta are all killed. `lengthen`/`shorten` swapped is killed. - **`byStart`'s `$4`.** Before the id, unquoted, and `arg` without the `or ""` are all killed. - **The layout equality asserts** at `test.luau:3578` and `3582` compare tree values against tree values but are not tautologies: changing the editor column's width alone, or the tag field's alone, fails them. - **Key stability.** Bumping the key on every render fails at `test.luau:3896`. - **`paintsFor`.** An extra commit on open fails; a commit per keystroke fails. - **Harness honesty.** `editRow() == nil` is a real assertion everywhere it appears; `fields()` asserts non-nil at `3489`; `buttons()` raises on a nil editor rather than passing. No `fields()` read follows an `apply` without an intervening repaint — the two that could have are preceded by `panelEnv.update()`. - **Queue discipline.** Every `#panelJobs == N` in the new section is preceded by a `panelJobs = {}` on the triggering line, so none passes on a leftover. `takeJob` raises on a miss, so draining a refetch by needle *does* catch a panel that stopped refetching — a mutation removing the post-edit refresh fails at `takeJob`'s own error. An extra command leaked only on the editor's success path is caught too. Three of the five drains do not seal with `assert(#panelJobs == 0)` the way `3713` and `3835` do; harmless as it stands, but sealing them is free. - **The restructuring is clean.** No assertion was dropped: the eight message differences against `main` are all the menu renumbering (`items[3..5]` → `[4..6]`), the `byStart` message gaining `and no $4`, the scroll-children rewording, and `"the two that are always available"` → `"the three"`. `assert(` goes 479 → 607, `fire(` 13 → 53. One `shown` and one `HOSTILE` declaration each; `shown` moving to line 153 only widens its scope, and both the `do` block at 752 and the IIFE at 3457 execute — mutations die inside each. - **`submitEdit`'s `state.editing == "" or base.start == nil` guard** is never exercised and removing it survives, but a stale handler resolves to nil in the shell (`luau_host.cpp:3082-3083`), so the branch is unreachable. Not a gap. Same for comparing `editDuration` untrimmed: `Parse.spanSeconds` trims and the zero-delta check absorbs it, so that mutant is equivalent. Gates on the branch as it stands: `nix shell nixpkgs#luau -c luau test.luau` → `ok`; `noctalia plugins lint timew` → `0 errors, 0 warnings`.
yuris force-pushed feat/inline-edit from cfc2e8ec91 to 03490e273c 2026-09-04 06:20:53 +00:00 Compare
Author
Owner

Both reviews actioned — every finding, and what happened to it

Branch force-pushed as one commit (cfc2e8e → 2431b81); body rewritten. Gates
in the checkout after the last edit:

$ nix shell nixpkgs#luau -c luau test.luau
viewModel: 13 states, 1870 field comparisons
ok
$ noctalia plugins lint timew
  ok
0 errors, 0 warnings

The one decision both reviews got half-right

The two reviews proposed opposite fixes for the same line, because state.editBase
was doing two jobs. Both are now separate and both are pinned:

  • Whether a field changed → the captured baseline text (base.at,
    base.duration, base.tag). This is what keeps an untouched submit silent on a
    row whose duration ticks or gets refetched. Replacing base wholesale (the
    sibling review's mutation) now fails.
  • By how much the duration changed → the live row, found in state.shown by
    base.start, falling back to the baseline when the row is gone. The frozen
    value was the reachable wrong write: Stop the running row, ask for 1:00 on a
    row that is now 0:45, and the shipped code emitted lengthen PT1800S. That
    now fails too.

Blocking

# Finding Resolution
B1 editBase never reconciles; Stop turns it into a silent wrong write Applied as suggested — live found by base.start, Timew.cmd.edits(live, …). Both guarding tests added: an untouched submit after a refetch that grows the row sends nothing; a touched one sends PT900S and not PT1800S. Each review found one half; both mutations now die.
B2 onOpen resets the card's field and not the editor's Applied. editing, editAt, editDuration, editTag, editBase all cleared, with the reason beside draft's. Test: a half-typed editor plus a reopen leaves no editor drawn, no baseline, no typed text. editKey deliberately not reset — see survivors below.
A4 "a command that does refetch closes the editor on success" is false Applied. Rewritten to state what holds: run refetches from start, stop, startTag, startRecent, continue, delete and undo and touches none of state.editing; only submitEdit's own done(ok) closes the editor; a reorder moves the editor with its interval because it is drawn where row.start == state.editing. The surrounding argument (typed text survives a rebuild because onChange applies to state, so only the caret moves) is kept and still reads correctly.
A1 A multi-tag row seeds literal quote characters into retag Applied, the refusing half. beginEdit recognises the shape and submitEdit leaves the tag out; the other two fields still work. Reproduced first: timew retag @1 'admin "deep work"' → Note: '"admin \"deep work\""' is a new tag, and timew tags then lists admin "deep work" for good. Tests: the raw-quote seed, the untouched submit, a typed-over submit that sends the move alone, and a single spaced tag ("deep work") proving it is not this case. README limit corrected — it said "flattens", which understated it; a bare admin focus set is the case that still flattens, and that is now said separately.
S2 onSubmit asserted to exist and never fired Applied. The prop is compared against the confirm button's own onClick (which kills the no-op, the cancelEdit swap and the one-field-of-three wiring), and Enter is then fired for real on the live path, asserting the retag it queued and the close when it lands.
S3 move only ever composed for a start moved earlier Applied at both levels: editCmds({ at = "18:45" }) at the unit level and an 18:45 confirm on the live path. The if want.at > base.at then "1970-01-01" mutation now dies at the unit assert.

Advisory, all applied

# Finding Resolution
A5 EDIT_TIME_W budgeted for five glyphs; Fmt.span emits 100:00 68 → 76. Verified Fmt.span(100*3600) == "100:00". The comment's arithmetic is re-measured: 34 px for 08:00, ~7 px for a sixth glyph → 41 wanted, 71 + caret = 76. Top line spends 216 of LIST_W's 283, spacer 67, so nothing is pushed off.
S5 Time-field viewport floor >= 5 * 5 admits 53–67 Raised to 41, not the 34 the review asked for — item A5 re-budgets the field for six glyphs, so 34 would have left 68 passing while 100:00 clips. 68 and 56 both die now.
S4 EDIT_H unasserted f.props.height >= 24 on all three fields. 24 → 20 and a dropped height both die.
S6 The three input keys never asserted distinct Pairwise inequality asserted; one shared key dies.
A3 script_runtime_types.h:39-42 names the wrong lines Corrected to 32-36 at :801. The citation at :753 is about the item's field defaults and is correct — left alone.
M1–M8 Eight survivors in the editor's geometry and lifecycle guards All eight killed: no focus on the fields, both gaps non-zero, confirm's variant asserted different from cancel's, cancelEdit (and a landed submit) leaving editBase empty, the base.start == nil guard exercised by clearing the baseline under a drawn editor, and runChain's done(ok) proven to fire before pokeBar by instrumenting noctalia.state.set and reading state.editing at the poke.
S8a editRow() returns the first ui.column It counts them now and asserts <= 1; a buildList that draws a second editor dies on that assert with the child count still correct.
S7 1870 field comparisons covers none of this feature PR body corrected (see below) and a two-way MODEL_FIELDS census added beside the state one, over the model, every list row and every chart bar. A junk top-level, per-row or per-bar field now fails, as does a dropped one.
S8b Fixture gaps Added: an editor on the running interval (start editable; duration refused with timew's own Cannot lengthen open interval @1, re-verified on 1.9.1, chain stopped, editor left open), an editor in all-time scope (base.date is the row's own 2026-08-28, four days off the day scope, and move composes on it), and onConfigChanged with an editor open (stays open, keeps the typed text, keeps its key).

README

Limits list now carries, beside the three that were there: the 24-hour editor
under a 12-hour row (A2 — documented rather than reformatted, see below); the
32 px growth and kScroll's absent scroll-into-view (A6); the own-date rule and
its edge, re-verified here (23:50 row + 00:10 → Moved @2 to 2026-09-01T00:10:00, 23h40m backwards); and the corrected multi-tag entry.

PR body

The 13 states, 1870 field comparisons quote is gone as a gate, with the reason
stated: main prints that line byte for byte, all 13 check() calls run before
the editor section, and compare never reads isEditing or the four model
fields. What does cover the feature is named instead — the editor section driving
the tree (82 fire( against main's 13; 658 asserts against 479) plus the new
census — and the mutation count is updated.

Mutations, before and after

before after
ReviewSource M1–M8 (geometry and lifecycle) 8 survived 0
ReviewSuite blocking (baseline↔live, onSubmit ×3, move forward) 5 survived 0
the wrong write itself (delta off the frozen seconds) shipped dies
author's original 31 0 survived 0 — the twelve the body names that still apply were re-run directly and all die
this round, total applied — 58 applied, 55 die, 3 survive

The three survivors, deliberately:

  • EDIT_TIME_W 76 → 71. 41 px of viewport is exactly the asserted floor; the
    remaining 5 px is the caret's, which is judgment, not measurement. Pinning it
    would freeze a number no API can check.
  • the break in the live-row lookup. Two intervals cannot share a start — timew
    forbids overlap — so last-match equals first-match. Equivalent mutant.
  • onOpen also resetting editKey = 0 (which the source review's snippet
    included). Unobservable: a closed panel's tree is gone. Left monotonic on
    purpose, so a fresh editor cannot reuse a key an earlier one used — which is
    why it is not in the five fields onOpen clears.

Considered and declined

  • A2, seeding/parsing the start field through Fmt.hhmm. Documented as a
    limit instead. The inverse of Fmt.hhmm does not exist, Parse.timeOfDay
    would have to accept 5:48 PM in every locale's spelling of the meridiem, and
    the toast already names the format it wants. A wart, one glance wide, on the
    shells where hour12 is true.
  • A toast when a multi-tag row's tag is dropped. The submit's two other
    inexpressible cases (a cleared tag, a zero delta) are silent for the same
    reason, and a blocking toast would refuse the other fields with it. Left
    silent and documented. This is the one I would most expect a second reviewer to
    push back on.
  • The predicate's first clause. The review's Parse.unquote(row.tag) == row.tag and string.find(row.tag, '"') was written against the raw DOM string;
    row.tag has already been through Parse.unquote once, so the first half can
    only fail on a value still quoted end to end after a strip, which timew never
    emits (it escapes an inner quote instead of doubling the outer pair). Dropping
    it survived the suite as dead code, so it is gone and the comment says why.
  • 0:00 in the duration field shortening an interval to zero length. Real,
    accepted by timew, one undo away, and outside this round's scope — worth a
    README line if you want it, say so and I will add it.
  • Sealing three of the five job drains with assert(#panelJobs == 0). The new
    drains follow the existing pattern (panelJobs = {} on the triggering line, so
    none passes on a leftover); two of my new sections do seal. Not tightened
    further.
## Both reviews actioned — every finding, and what happened to it Branch force-pushed as one commit (`cfc2e8e` → `2431b81`); body rewritten. Gates in the checkout after the last edit: ``` $ nix shell nixpkgs#luau -c luau test.luau viewModel: 13 states, 1870 field comparisons ok $ noctalia plugins lint timew ok 0 errors, 0 warnings ``` ### The one decision both reviews got half-right The two reviews proposed opposite fixes for the same line, because `state.editBase` was doing two jobs. Both are now separate and both are pinned: - **Whether** a field changed → the captured baseline text (`base.at`, `base.duration`, `base.tag`). This is what keeps an untouched submit silent on a row whose duration ticks or gets refetched. Replacing `base` wholesale (the sibling review's mutation) now fails. - **By how much** the duration changed → the live row, found in `state.shown` by `base.start`, falling back to the baseline when the row is gone. The frozen value was the reachable wrong write: Stop the running row, ask for `1:00` on a row that is now `0:45`, and the shipped code emitted `lengthen PT1800S`. That now fails too. ### Blocking | # | Finding | Resolution | |---|---|---| | B1 | `editBase` never reconciles; Stop turns it into a silent wrong write | **Applied** as suggested — `live` found by `base.start`, `Timew.cmd.edits(live, …)`. Both guarding tests added: an untouched submit after a refetch that grows the row sends **nothing**; a *touched* one sends `PT900S` and not `PT1800S`. Each review found one half; both mutations now die. | | B2 | `onOpen` resets the card's field and not the editor's | **Applied.** `editing`, `editAt`, `editDuration`, `editTag`, `editBase` all cleared, with the reason beside `draft`'s. Test: a half-typed editor plus a reopen leaves no editor drawn, no baseline, no typed text. `editKey` deliberately **not** reset — see survivors below. | | A4 | "a command that does refetch closes the editor on success" is false | **Applied.** Rewritten to state what holds: `run` refetches from start, stop, `startTag`, `startRecent`, continue, delete and undo and touches none of `state.editing`; only `submitEdit`'s own `done(ok)` closes the editor; a reorder moves the editor with its interval because it is drawn where `row.start == state.editing`. The surrounding argument (typed text survives a rebuild because `onChange` applies to state, so only the caret moves) is kept and still reads correctly. | | A1 | A multi-tag row seeds literal quote characters into `retag` | **Applied**, the refusing half. `beginEdit` recognises the shape and `submitEdit` leaves the tag out; the other two fields still work. Reproduced first: `timew retag @1 'admin "deep work"'` → `Note: '"admin \"deep work\""' is a new tag`, and `timew tags` then lists `admin "deep work"` for good. Tests: the raw-quote seed, the untouched submit, a typed-over submit that sends the `move` **alone**, and a single spaced tag (`"deep work"`) proving it is *not* this case. README limit corrected — it said "flattens", which understated it; a bare `admin focus` set is the case that still flattens, and that is now said separately. | | S2 | `onSubmit` asserted to exist and never fired | **Applied.** The prop is compared against the confirm button's own `onClick` (which kills the no-op, the `cancelEdit` swap *and* the one-field-of-three wiring), and Enter is then fired for real on the live path, asserting the `retag` it queued and the close when it lands. | | S3 | `move` only ever composed for a start moved *earlier* | **Applied** at both levels: `editCmds({ at = "18:45" })` at the unit level and an 18:45 confirm on the live path. The `if want.at > base.at then "1970-01-01"` mutation now dies at the unit assert. | ### Advisory, all applied | # | Finding | Resolution | |---|---|---| | A5 | `EDIT_TIME_W` budgeted for five glyphs; `Fmt.span` emits `100:00` | `68 → 76`. Verified `Fmt.span(100*3600) == "100:00"`. The comment's arithmetic is re-measured: 34 px for `08:00`, ~7 px for a sixth glyph → 41 wanted, 71 + caret = 76. Top line spends 216 of `LIST_W`'s 283, spacer 67, so nothing is pushed off. | | S5 | Time-field viewport floor `>= 5 * 5` admits 53–67 | Raised to **41**, not the 34 the review asked for — item A5 re-budgets the field for six glyphs, so 34 would have left `68` passing while `100:00` clips. `68` and `56` both die now. | | S4 | `EDIT_H` unasserted | `f.props.height >= 24` on all three fields. `24 → 20` and a dropped `height` both die. | | S6 | The three input keys never asserted distinct | Pairwise inequality asserted; one shared key dies. | | A3 | `script_runtime_types.h:39-42` names the wrong lines | Corrected to `32-36` at `:801`. The citation at `:753` is about the item's field defaults and is correct — left alone. | | M1–M8 | Eight survivors in the editor's geometry and lifecycle guards | All eight killed: no `focus` on the fields, both `gap`s non-zero, confirm's `variant` asserted **different from** cancel's, `cancelEdit` (and a landed submit) leaving `editBase` empty, the `base.start == nil` guard exercised by clearing the baseline under a drawn editor, and `runChain`'s `done(ok)` proven to fire *before* `pokeBar` by instrumenting `noctalia.state.set` and reading `state.editing` at the poke. | | S8a | `editRow()` returns the first `ui.column` | It counts them now and asserts `<= 1`; a `buildList` that draws a second editor dies on that assert with the child count still correct. | | S7 | `1870 field comparisons` covers none of this feature | PR body corrected (see below) **and** a two-way `MODEL_FIELDS` census added beside the state one, over the model, every list row and every chart bar. A junk top-level, per-row or per-bar field now fails, as does a dropped one. | | S8b | Fixture gaps | Added: an editor on the **running** interval (start editable; duration refused with timew's own `Cannot lengthen open interval @1`, re-verified on 1.9.1, chain stopped, editor left open), an editor in **all-time** scope (`base.date` is the row's own `2026-08-28`, four days off the day scope, and `move` composes on it), and `onConfigChanged` with an editor open (stays open, keeps the typed text, keeps its key). | ### README Limits list now carries, beside the three that were there: the 24-hour editor under a 12-hour row (A2 — documented rather than reformatted, see below); the 32 px growth and `kScroll`'s absent scroll-into-view (A6); the own-date rule and its edge, re-verified here (`23:50` row + `00:10` → `Moved @2 to 2026-09-01T00:10:00`, 23h40m backwards); and the corrected multi-tag entry. ### PR body The `13 states, 1870 field comparisons` quote is gone as a gate, with the reason stated: `main` prints that line byte for byte, all 13 `check()` calls run before the editor section, and `compare` never reads `isEditing` or the four model fields. What does cover the feature is named instead — the editor section driving the tree (82 `fire(` against `main`'s 13; 658 asserts against 479) plus the new census — and the mutation count is updated. ### Mutations, before and after | | before | after | |---|---|---| | ReviewSource M1–M8 (geometry and lifecycle) | 8 survived | 0 | | ReviewSuite blocking (baseline↔live, `onSubmit` ×3, `move` forward) | 5 survived | 0 | | the wrong write itself (delta off the frozen `seconds`) | shipped | dies | | author's original 31 | 0 survived | 0 — the twelve the body names that still apply were re-run directly and all die | | this round, total applied | — | **58 applied, 55 die, 3 survive** | The three survivors, deliberately: - `EDIT_TIME_W` 76 → **71**. 41 px of viewport is exactly the asserted floor; the remaining 5 px is the caret's, which is judgment, not measurement. Pinning it would freeze a number no API can check. - the `break` in the live-row lookup. Two intervals cannot share a start — timew forbids overlap — so last-match equals first-match. Equivalent mutant. - `onOpen` also resetting `editKey = 0` (which the source review's snippet included). Unobservable: a closed panel's tree is gone. Left monotonic on purpose, so a fresh editor cannot reuse a key an earlier one used — which is why it is not in the five fields `onOpen` clears. ### Considered and declined - **A2, seeding/parsing the start field through `Fmt.hhmm`.** Documented as a limit instead. The inverse of `Fmt.hhmm` does not exist, `Parse.timeOfDay` would have to accept `5:48 PM` in every locale's spelling of the meridiem, and the toast already names the format it wants. A wart, one glance wide, on the shells where `hour12` is true. - **A toast when a multi-tag row's tag is dropped.** The submit's two other inexpressible cases (a cleared tag, a zero delta) are silent for the same reason, and a blocking toast would refuse the *other* fields with it. Left silent and documented. This is the one I would most expect a second reviewer to push back on. - **The predicate's first clause.** The review's `Parse.unquote(row.tag) == row.tag and string.find(row.tag, '"')` was written against the raw DOM string; `row.tag` has already been through `Parse.unquote` once, so the first half can only fail on a value still quoted end to end after a strip, which timew never emits (it escapes an inner quote instead of doubling the outer pair). Dropping it survived the suite as dead code, so it is gone and the comment says why. - **`0:00` in the duration field** shortening an interval to zero length. Real, accepted by timew, one `undo` away, and outside this round's scope — worth a README line if you want it, say so and I will add it. - **Sealing three of the five job drains with `assert(#panelJobs == 0)`.** The new drains follow the existing pattern (`panelJobs = {}` on the triggering line, so none passes on a leftover); two of my new sections do seal. Not tightened further.
yuris force-pushed feat/inline-edit from 03490e273c to 2431b81c65 2026-09-04 06:23:29 +00:00 Compare
yuris force-pushed feat/inline-edit from 2431b81c65 to 512c57f9ab 2026-09-04 06:35:17 +00:00 Compare
Author
Owner

Both reviews actioned, plus one item they left open. Amended to 512c57f.

The two reviews proposed opposite fixes for the same line

ReviewSource proved the frozen editBase.seconds produces a wrong delta;
ReviewSuite proved that reading the live row instead submits a resize nobody
asked for. Both are right, because editBase was doing two jobs on one line:

  • whether a field changed → the captured text decides (keeps an
    untouched submit silent on a row whose duration ticks or gets refetched)
  • by how much the duration changed → the live row decides (lands the
    resize where the user asked)

submitEdit now keeps base for the change detection and passes a live row
into Timew.cmd.edits. Both halves are separately pinned, and I re-ran both
reviewers' mutations myself: reverting to edits(base, …) dies, and
ReviewSuite's wholesale base = r dies.

One item beyond both reviews

ReviewSuite's last note wondered whether a prop existed to make the inert
multi-tag field honest. It does — enabled is in kInput
(ui_tree_reconciler.cpp:539-545). That field is now rendered disabled rather
than accepting keystrokes the submit discards, which was the one place this
panel took input and did nothing with it. The two time fields stay live.

Verified independently of the fixer's report

24 mutations re-run by hand in a /tmp copy, all dead — both blockers in both
directions, all eight of ReviewSource's geometry/lifecycle survivors (EDIT_H
24→20, focus added, both gaps, confirm's variant, cancelEdit leaving
editBase, the base.start == nil guard, done(ok) ordering), and all of
ReviewSuite's (onSubmit no-op and swap, EDIT_TIME_W 76→68, shared keys, two
editors drawn, junk model field). Three more for the enabled change: dropping
it, disabling every field, and pinning the flag false all die.

The new model census earned its place immediately — it failed on
editTagLocked before I declared it.

Gates in the live checkout: 13 states, 1870 field comparisons / ok, and
0 errors, 0 warnings.

Accepted survivors

Three, all argued rather than hidden: EDIT_TIME_W 76→71 (the remaining 5 px is
caret slack — a judgment number no API can measure), the break in the live-row
lookup (starts are unique because timew forbids overlap, so it is an equivalent
mutant), and onOpen also zeroing editKey (unobservable across a panel close;
left monotonic so a fresh editor cannot reuse a key the shell may remember).

Declined, with reasons

  • No toast when a multi-tag tag edit is dropped. The enabled = false above
    supersedes this: the field now refuses the keystroke instead of needing to
    apologise for it afterwards.
  • 12-hour editor. Documented rather than fixed. Fmt.hhmm has no inverse and
    Parse.timeOfDay would have to accept every locale's meridiem spelling; the
    toast already names the required format.

What a second reviewer should still challenge

  • EDIT_TIME_W = 76 and the 41 px viewport floor are claims about a font, not
    measurements — the same class of claim a screenshot had to falsify at 52 px.
    I could not re-verify this one visually: niri msg --json outputs returns
    {}, so the compositor currently has no output and grim cannot capture. The
    arithmetic checks out (top line spends 216 of LIST_W 283; 46 px of viewport
    against ~42 for 100:00, and Fmt.span(100*3600) is "100:00") but it is
    arithmetic, not a picture. The earlier live verification in this PR was done
    before the width changed from 68.
  • The multi-tag predicate is single-clause and depends on Parse.interval
    always unquoting once. It fails safe (refuses the tag rather than corrupting
    it), but the coupling is real.
  • One test writes panel.state.editBase = {} directly to reach a branch that is
    unreachable through the shell — the only place the suite pokes state instead
    of driving a handler.
Both reviews actioned, plus one item they left open. Amended to `512c57f`. ## The two reviews proposed opposite fixes for the same line ReviewSource proved the frozen `editBase.seconds` produces a wrong delta; ReviewSuite proved that reading the live row instead submits a resize nobody asked for. Both are right, because `editBase` was doing two jobs on one line: - **whether** a field changed → the **captured text** decides (keeps an untouched submit silent on a row whose duration ticks or gets refetched) - **by how much** the duration changed → the **live row** decides (lands the resize where the user asked) `submitEdit` now keeps `base` for the change detection and passes a `live` row into `Timew.cmd.edits`. Both halves are separately pinned, and I re-ran both reviewers' mutations myself: reverting to `edits(base, …)` dies, and ReviewSuite's wholesale `base = r` dies. ## One item beyond both reviews ReviewSuite's last note wondered whether a prop existed to make the inert multi-tag field honest. It does — **`enabled` is in `kInput`** (`ui_tree_reconciler.cpp:539-545`). That field is now rendered disabled rather than accepting keystrokes the submit discards, which was the one place this panel took input and did nothing with it. The two time fields stay live. ## Verified independently of the fixer's report 24 mutations re-run by hand in a `/tmp` copy, all dead — both blockers in both directions, all eight of ReviewSource's geometry/lifecycle survivors (`EDIT_H` 24→20, `focus` added, both gaps, confirm's `variant`, `cancelEdit` leaving `editBase`, the `base.start == nil` guard, `done(ok)` ordering), and all of ReviewSuite's (`onSubmit` no-op and swap, `EDIT_TIME_W` 76→68, shared keys, two editors drawn, junk model field). Three more for the `enabled` change: dropping it, disabling every field, and pinning the flag false all die. The new model census earned its place immediately — it failed on `editTagLocked` before I declared it. Gates in the live checkout: `13 states, 1870 field comparisons` / `ok`, and `0 errors, 0 warnings`. ## Accepted survivors Three, all argued rather than hidden: `EDIT_TIME_W` 76→71 (the remaining 5 px is caret slack — a judgment number no API can measure), the `break` in the live-row lookup (starts are unique because timew forbids overlap, so it is an equivalent mutant), and `onOpen` also zeroing `editKey` (unobservable across a panel close; left monotonic so a fresh editor cannot reuse a key the shell may remember). ## Declined, with reasons - **No toast when a multi-tag tag edit is dropped.** The `enabled = false` above supersedes this: the field now refuses the keystroke instead of needing to apologise for it afterwards. - **12-hour editor.** Documented rather than fixed. `Fmt.hhmm` has no inverse and `Parse.timeOfDay` would have to accept every locale's meridiem spelling; the toast already names the required format. ## What a second reviewer should still challenge - `EDIT_TIME_W = 76` and the 41 px viewport floor are claims about a font, not measurements — the same class of claim a screenshot had to falsify at 52 px. **I could not re-verify this one visually:** `niri msg --json outputs` returns `{}`, so the compositor currently has no output and `grim` cannot capture. The arithmetic checks out (top line spends 216 of `LIST_W` 283; 46 px of viewport against ~42 for `100:00`, and `Fmt.span(100*3600)` is `"100:00"`) but it is arithmetic, not a picture. The earlier live verification in this PR was done before the width changed from 68. - The multi-tag predicate is single-clause and depends on `Parse.interval` always unquoting once. It fails safe (refuses the tag rather than corrupting it), but the coupling is real. - One test writes `panel.state.editBase = {}` directly to reach a branch that is unreachable through the shell — the only place the suite pokes state instead of driving a handler.
yuris merged commit ae619fd0a3 into main 2026-09-04 06:36:40 +00:00
yuris deleted branch feat/inline-edit 2026-09-04 06:36:40 +00:00
Sign in to join this conversation.
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!21
No description provided.