Skip to content

fix(morphdom): make lvt-on:click checkboxes/radios server-authoritative#135

Merged
adnaan merged 2 commits into
mainfrom
fix/server-authoritative-checkbox
Jun 20, 2026
Merged

fix(morphdom): make lvt-on:click checkboxes/radios server-authoritative#135
adnaan merged 2 commits into
mainfrom
fix/server-authoritative-checkbox

Conversation

@adnaan

@adnaan adnaan commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Problem

A checkbox/radio whose toggle is routed to the server via lvt-on:click is server-authoritative: the click is dispatched and the server echoes the new checked state in its re-render. But the morphdom pass defaults to "user selection wins" — it copies the live control's checked state onto the incoming element (toEl.checked = fromEl.checked) so scan-loop refreshes don't clobber pending user input.

For an lvt-on:click control that's backwards: when the live state is stale relative to the server's authoritative value, the server's toggle never reflects in the DOM and the checkbox appears stuck. This surfaced as flaky/failing E2E tests in tinkerdown (livetemplate/tinkerdown#292), where a checkbox toggle was confirmed server-side (WS frame carried checked) yet the DOM property stayed false.

Fix

Extend the server-authoritative predicate in onBeforeElUpdated so an lvt-on:click checkbox/radio is treated like data-lvt-force-update — the server's rendered checked wins.

  • data-lvt-force-update stays the explicit one-shot override (still stripped after applying).
  • lvt-on:click is a durable handler, left in place, so the control stays authoritative on every update.

This removes the need to sprinkle data-lvt-force-update on every server-driven toggle checkbox. The ecosystem already writes this pattern without the attribute — lvt's toggle component, datatable row/select-all, tinkerdown auto-tasks — and was latently affected; this retroactively fixes them.

Tests

  • handler ⇒ server wins (the bug, now fixed)
  • handler-less ⇒ user state still preserved (default behavior unchanged — guards against regression)
  • explicit data-lvt-force-update still overrides (existing behavior)

Full suite green (736 passed, 1 skipped). tsc --noEmit clean.

Blast radius

Grepped sibling apps for lvt-on:click + checkbox/radio: every occurrence renders server state via {{if .Checked}}checked{{end}}, i.e. server-authoritative by intent. No app relies on optimistic-over-server for these controls, so this is a net fix.

🤖 Generated with Claude Code

A checkbox or radio whose toggle is routed to the server via lvt-on:click
is server-authoritative: the click is dispatched and the server echoes back
the new checked state in its re-render. The morphdom pass, however, defaults
to "user selection wins" — it copies the live control's checked state onto
the incoming element (toEl.checked = fromEl.checked) so scan-loop refreshes
don't clobber pending user input. For an lvt-on:click control that's
backwards: when the live state is stale relative to the server's authoritative
value, the server's toggle never reflects in the DOM and the box appears stuck.

Extend the server-authoritative predicate in onBeforeElUpdated to treat an
lvt-on:click checkbox/radio like data-lvt-force-update — the server's rendered
checked attribute wins. data-lvt-force-update remains the explicit one-shot
override (and is still stripped after applying); lvt-on:click is a durable
handler and is left in place, so the control stays authoritative on every update.

This removes the need to sprinkle data-lvt-force-update on every server-driven
toggle checkbox (lvt's toggle component, datatable row/select-all, tinkerdown
auto-tasks, etc. all use this pattern without it and were latently affected).

Tests: handler ⇒ server wins; handler-less ⇒ user state still preserved
(default unchanged); explicit data-lvt-force-update still overrides. Full
suite green (737).
@claude

claude Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review — fix(morphdom): lvt-on:click server-authoritative checkboxes

The fix is sound and the two regression tests cover the key paths. A few things worth flagging before merging:

Semantic coupling risk (main concern)

lvt-on:click means "route this click to the server", not "the server owns my checked state." These happen to coincide today (blast-radius grep confirms), but they're not the same thing. A future developer who adds lvt-on:click to a checkbox for a side effect (e.g. analytics, logging, conditional navigation) — without intending to cede checked-state ownership — will silently get server-authoritative behavior and no indication from the attribute name that this is happening.

Consider whether an explicit data-lvt-server-checked (or similar) attribute would be a cleaner contract, even if lvt-on:click toggles it automatically when the handler is registered. Alternatively, document this implication prominently in the lvt-on:click attribute docs so developers know what they're opting into.

Radios with lvt-on:click are untested

The serverAuthoritative path applies equally to radio inputs, but the new tests only cover checkboxes. Given the known radio-group mutual-exclusion limitation (mid-morphdom uncheck of siblings), a test or explicit note on why the radio case is safe would help future maintainers.

Minor: the first test doesn't simulate a user click

The "server wins" test calls updateDOM twice without setting cb.checked = true between them — it validates the morph behavior but doesn't replicate the actual triggering scenario (user click → stale live state → server echo). Not a blocker, just worth noting if someone asks "does this test prove the E2E scenario?"

Overall the logic is correct for the described use case and the blast-radius check is reassuring. The semantic coupling concern is the one I'd want a decision on before shipping.

…checkboxes

Addresses review feedback on PR #135:
- add a server-authoritative radio test (the predicate covers radios too)
- the checkbox test now also asserts the inverse direction (server re-affirms
  checked over a stale user-unchecked state) using a co-changing sibling slot
  so each push is a real morphdom pass, not a short-circuited no-op
@adnaan

adnaan commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed all three:

1. Semantic coupling (decision made): Shipping the lvt-on:click auto-detection deliberately. Rationale: a checkbox/radio with lvt-on:click + {{if .X}}checked{{end}} is server-owned by construction, and the scope is limited to lvt-on:click specifically — so the side-effect case you raised has a built-in escape hatch: use lvt-on:change (not auto-detected) when you want user-selection to win. Documented in the core attribute reference (livetemplate/docs/references/client-attributes.md): new "Exception — lvt-on:click controls are server-authoritative" subsection + the lvt-on:change escape hatch + updated override table.

2. Radios untested: Added server wins for a radio with an lvt-on:click handler. It sends authoritative state on all radios in the group, which sidesteps the documented mid-pass mutual-exclusion limitation (noted in the test comment).

3. Test didn't replay the click sequence: The "server wins" checkbox test now also asserts the inverse direction — user optimistically unchecks, server re-affirms checked — using a co-changing sibling slot so each push is a real morphdom pass (an identical tree short-circuits in updateDOM). This exercises server authority overriding stale local state in both directions.

Full suite green (737 passed, 1 skipped).

@claude

claude Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review

Logic is correct and well-tested. The predicate change is minimal, the comment update accurately documents the new invariant, and the three new tests cover the happy path, regression guard, and inverse direction. A few things worth considering:

Partial-handler radio groups. The test only covers the "all radios have lvt-on:click" case. If a radio group mixes handler and handler-less inputs, authority is split — server-authoritative radios clobber the live checked attribute while user-authoritative siblings have theirs preserved. Combined with the documented mid-pass mutual-exclusion issue, this can leave the group incoherent. Not a regression (undefined before too), but worth either documenting or guarding against.

indeterminate on lvt-on:click checkboxes. The indeterminate sync now also fires for lvt-on:click-bound checkboxes, but there's no test for this path. Harmless today; if a server ever emits an indeterminate checkbox with a click handler a future regression would be invisible.

Attribute check on toEl vs fromEl. Checking toEl.hasAttribute("lvt-on:click") is correct — if the server stops sending the handler, authority gracefully reverts to user state. Confirming this is the intended semantics (server decides when it hands authority back), which it appears to be.

Overall this is a clean fix for a real bug. The blast-radius grep and the handler-less regression test give good confidence it won't break existing behaviour.

@adnaan adnaan merged commit 28609e1 into main Jun 20, 2026
6 checks passed
@adnaan adnaan deleted the fix/server-authoritative-checkbox branch June 20, 2026 07:57
adnaan added a commit to livetemplate/livetemplate that referenced this pull request Jun 20, 2026
…heckboxes (#461)

A checkbox/radio with an lvt-on:click handler is now server-authoritative in
@livetemplate/client (the server's rendered checked state wins without needing
data-lvt-force-update). Document this exception in the checkbox/radio
preservation section, note lvt-on:change as the user-wins escape hatch, and
update the override table.

Pairs with livetemplate/client#135.
adnaan added a commit to livetemplate/tinkerdown that referenced this pull request Jun 20, 2026
…293)

Three browser E2E tests flaked in the cross-repo job; two distinct,
unrelated root causes (plus three more tests sharing cause #2):

1. TestActionButtons — SQLITE_BUSY. SQLite's busy_timeout defaults to 0,
   so a read that races a concurrent write fails immediately with
   "database is locked". The library now opens every SQLite connection
   with busy_timeout=5000 (internal/source: new sqliteDSN helper used by
   both SQLiteSource and the schema probe), and the test's own
   verification connection gets the same pragma — the read waits for the
   action's write to commit instead of failing.

2. Checkbox toggles never reflected the server's new state
   (TestAutoTasks_BasicToggle/_NoFullReload, TestLvtSourceMarkdownToggle/
   ToggleBack). The client's morphdom pass preserves live <input> state
   over server-rendered values; for a server-authoritative
   lvt-on:click="Toggle" checkbox that's backwards, so the server's toggle
   never reflected in the DOM. Fixed upstream in @livetemplate/client
   v0.14.3 (livetemplate/client#135): an lvt-on:click checkbox/radio is now
   server-authoritative by default. Bump @livetemplate/client 0.11.9 →
   0.14.3 and rebuild the embedded bundle; no per-template attribute is
   needed — the framework handles it generically for every server-driven
   toggle.

Also hardened the auto-task E2E interactions: switch from chromedp.Click
to JS .click() (CDP click is unreliable for delegated handlers in headless
Docker Chrome, as action_buttons_e2e_test.go already documents) and
replace fixed sleeps with waitForDOM polling for the actual expected state.

Verified locally against Docker Chrome with @livetemplate/client@0.14.3:
TestActionButtons, all six TestAutoTasks, both markdown toggle tests green
on repeat runs.


Claude-Session: https://claude.ai/code/session_01HyPY7btbvCZiLKkh9wSKJx

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant