Skip to content

M0 Phase 0: upstream bump to livetemplate v0.19.1 + client 0.18.2 - #296

Merged
adnaan merged 2 commits into
mainfrom
m0/upstream-bump
Jul 19, 2026
Merged

M0 Phase 0: upstream bump to livetemplate v0.19.1 + client 0.18.2#296
adnaan merged 2 commits into
mainfrom
m0/upstream-bump

Conversation

@adnaan

@adnaan adnaan commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

First phase of the ephemeral-UI reframe plan (M0 Phase 0).

Tinkerdown was pinned to livetemplate v0.10.0 while upstream shipped nine minor releases. This adopts the current latest across the stack.

Dependency Was Now
github.com/livetemplate/livetemplate v0.10.0 v0.19.1
github.com/livetemplate/lvt/components pseudo-version 2026-02-28 v0.2.0
@livetemplate/client 0.14.3 (lockfile); node_modules had drifted to 0.11.9 0.18.2

No source changes were required. Tinkerdown's livetemplate API surface is 10 symbols, all stable across the range, and the one API removed upstream (WithStore, v0.19.0) was never used here.

The client version is a wire contract, not a preference: livetemplate v0.18.0 added the ClientVersion constant precisely because there is no runtime server↔client version handshake, and v0.19.0 declares 0.18.2 as the compatible pair for this server release.

Also fixes #295

make build ran npm run build with no install step, so it bundled whatever happened to be in client/node_modules rather than what the lockfile pins. On a drifted checkout this silently regenerated the committed bundle from a stale dependency and reverted shipped fixes — including the data-lvt-force-update handling the checkbox e2e tests depend on. Now runs npm ci first.

This is folded in here deliberately: without it, the next make build would silently revert the bundle this PR commits.

Verification

All under GOWORK=off — a go.work one directory up redirects livetemplate/lvt to local checkouts that sit ahead of the published tags, so without it the pinned dependency is not what gets tested.

  • go build ./... clean; full go test ./... green, including the root package holding all 32 //go:build !ci e2e files (827s)
  • Discriminating e2e set 14/14: TestAutoTables_*, TestAutoTasks_* (checkbox toggle), TestExecToolbar*, TestLvtSourceMarkdownToggle*
  • Bundle provenance, directionally: ctrlKey occurrences 1 → 5, matching v0.18.2's client fix for bare-key lvt-on:keydown firing while Ctrl/Meta/Alt is held. A size delta alone would not distinguish an upgrade from a downgrade — make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests #295's bad bundle also differed from HEAD.
  • CSS half verified separately: browser.css is byte-identical for a good reason (the bump doesn't touch tinkerdown's own CSS sources); the client's livetemplate.css is deliberately not bundled, as it only configures lvt-fx:*, unused here.
  • Manual browser pass: served examples/action-buttons, full-page screenshot — styling, auto-table, form controls, button variants and syntax highlighting all correct; console clean, no JS exceptions.

Plan tracker updated in the same commit (convention 4)

Four findings recorded, three of which outlive this phase:

  1. WithEphemeralSweepTTL has no call site — it's a HandleOption and tinkerdown never calls Handle(). The plan's Implementation item assumed otherwise; struck through with the reason so M1 doesn't re-propose it.
  2. go test -tags=browser ./... was fiction — no such tag exists. E2E is opt-out (!ci), and CI excludes it twice over (-tags=ci and -skip='E2E|e2e'), so CI structurally cannot catch an e2e regression; the local run is the only gate.
  3. The go.work redirect — local builds were never testing the published dependency. GOWORK=off is now written into § Verification as the operational form of session-guide convention 11.
  4. websocket.go:467 still writes each inline block to /tmp, contradicting the reframe's "disk-free ephemeral path" claim now that v0.17.0 ships WithParseFS. Deferred to M1 Phase 3 rather than refactored here — a WS hot-path rewrite inside a version-bump phase would destroy the phase's isolability.

The plan's "multi-minor upstream bump may ripple" risk is retired: it was aimed at the Go boundary, and the actual hazard was committed-artifact provenance, now recorded as a standing risk.

🤖 Generated with Claude Code

https://claude.ai/code/session_018M9pJSPmG6i1D8s6rpEV4h

Tinkerdown was pinned to livetemplate v0.10.0 while upstream shipped nine
minor releases. Adopt the current latest across the stack:

  livetemplate          v0.10.0  -> v0.19.1
  lvt/components        pseudo   -> v0.2.0
  @livetemplate/client  0.14.3   -> 0.18.2

No source changes were required. Tinkerdown's livetemplate API surface is
10 symbols, all stable across the range, and the one API removed upstream
(WithStore, v0.19.0) was never used here.

The client version is a wire contract, not a preference: livetemplate
v0.18.0 added the ClientVersion constant precisely because there is no
runtime server<->client version handshake, and v0.19.0 declares 0.18.2 as
the compatible pair for this server release.

Also fix `make build` to run `npm ci` before `npm run build` (#295). It
previously bundled whatever happened to be in client/node_modules rather
than what the lockfile pins -- on this checkout node_modules had drifted
to 0.11.9, so any rebuild silently reverted shipped fixes including the
data-lvt-force-update handling the checkbox e2e tests depend on. Without
this fix the next `make build` would revert the bundle committed here.

Verification (all under GOWORK=off, since a go.work one directory up
redirects to local upstream checkouts that are ahead of the published
tags -- so without it the pinned dependency is not what gets tested):
  - go build ./... clean
  - full `go test ./...` green, including the root package that holds all
    32 //go:build !ci e2e files (827s)
  - the discriminating e2e set 14/14: TestAutoTables_*, TestAutoTasks_*
    (checkbox toggle), TestExecToolbar*, TestLvtSourceMarkdownToggle*
  - bundle provenance confirmed directionally: ctrlKey occurrences 1 -> 5,
    matching the v0.18.2 client fix for bare-key lvt-on:keydown firing
    while Ctrl/Meta/Alt is held

Plan tracker updated in the same commit (convention 4), recording four
findings: WithEphemeralSweepTTL has no call site in tinkerdown (it is a
HandleOption; tinkerdown never calls Handle()); `go test -tags=browser`
was fiction (no such tag -- e2e is opt-out via !ci, not opt-in); the
go.work redirect; and that websocket.go still writes each inline block to
/tmp, contradicting the reframe's disk-free claim now that v0.17.0 ships
WithParseFS -- deferred to M1 Phase 3 rather than refactored here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018M9pJSPmG6i1D8s6rpEV4h
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review

This is a clean, well-scoped dependency bump. Verified several of the PR's claims directly against the merged tree:

  • go.mod pins match the description (livetemplate v0.19.1, lvt/components v0.2.0); the tdewolff/* indirect deps are fully removed from both go.mod and go.sum with no dangling references.
  • client/package.json / lockfile correctly bumped to ^0.18.2.
  • Makefile:24 now runs npm ci && npm run build — confirms the make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests #295 fix is in place.
  • The CI gating claim checks out: .github/workflows/ci.yml runs -tags=ci -skip='E2E|e2e', and there are indeed 32 //go:build !ci files, so the e2e suite genuinely only runs locally. Good catch, and good that it's now documented in the plan tracker rather than left implicit.
  • internal/server/websocket.go:467 still writes to /tmp/lvt-<blockID>.tmpl with the stale "livetemplate.New() requires template files" comment — confirmed this is real and correctly deferred to M1 Phase 3 rather than scope-crept into this PR. Reasonable call given the "isolability" argument in the Learn section — a WS hot-path rewrite doesn't belong in a version-bump commit.
  • WithParseFS, WithStore, ClientVersion are all unreferenced in source, consistent with the "no source changes required" / "WithStore never used" claims.

Code quality: No notes — there's no hand-written logic in this diff to critique beyond the one-line Makefile fix, which is correct and minimal.

Bugs/correctness: None found. The one item worth flagging for awareness rather than action: the bundle provenance check (ctrlKey 1→5) is a good directional signal, but it's inherently a proxy — it confirms a client fix landed, not that the entire 0.14.3→0.18.2 diff is correctly bundled. Given this is a generated+committed artifact (the exact failure class #295 already burned you on), it might be worth a comment near the asset directory or in CONTRIBUTING noting "regenerate via make build-client, never hand-edit, and diff against a fresh npm ci build if in doubt" — but this is a nice-to-have, not a blocker, and the CHANGELOG already documents provenance well.

Performance: N/A — no runtime logic changed.

Security: No concerns. npm ci is the right call for reproducible installs (fails on lockfile drift rather than silently resolving), and no new dependencies were introduced beyond the version bumps.

Test coverage: The verification story here is unusually thorough for a dependency bump (14/14 discriminating e2e set, directional bundle check, manual browser pass, CSS parity check). One gap acknowledged in the plan tracker itself: CI structurally cannot catch an e2e regression on this bundle going forward, since the tests that would are !ci-only. That's an existing repo-level gap, not something this PR needs to fix, but worth keeping in mind for future bundle-touching PRs.

Also confirmed per CLAUDE.md guidance: this PR doesn't introduce or reimplement any lvt-* attributes — it's a pure upstream consumption bump, so that check doesn't apply here.

Nice work tying the dependency bump, the #295 build fix, and the plan-tracker update into one coherent, well-documented change.

Addresses the review-bot suggestion on #296: this asset is generated yet
tracked, so it can silently disagree with the lockfile that supposedly
produced it -- the exact failure class recorded in #295.

Documents the three things that are non-obvious about it: regenerate via
`make build-client` (never a bare `npm run build`, which bundles whatever
is in node_modules); CI cannot catch a regression here because the e2e
tests that would are !ci-only, so a local run is the only gate; and check
direction rather than mere difference, since a downgrade also differs from
HEAD. Also notes that the client version is a wire contract pinned by
livetemplate's ClientVersion constant, not whatever npm calls latest.

Lands beside the files rather than in CONTRIBUTING so it is visible to
whoever is about to touch them. Never served over HTTP -- client assets go
out through the named GetClientJS/GetClientCSS accessors, not the embedded
FS wholesale -- so this only adds ~2KB to the binary.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018M9pJSPmG6i1D8s6rpEV4h
@adnaan

adnaan commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — took the bundle-provenance suggestion. Added internal/assets/client/README.md (6231478) beside the generated files rather than in CONTRIBUTING, so it's visible to whoever is about to touch them.

It documents the three non-obvious things: regenerate via make build-client and never a bare npm run build; CI cannot catch a regression here because the tests that would are !ci-only, so the local run is the only gate; and check direction rather than mere difference, since a downgrade also differs from HEAD.

You're right that ctrlKey 1→5 is a proxy and not full-diff verification — I've written that limitation into the doc as the reason to diff against a clean npm ci build when in doubt, rather than leaning on a single marker.

No behavior change; ClientFS() is only read by a test, and client assets are served through the named GetClientJS/GetClientCSS accessors rather than the embedded FS wholesale, so the README is never served over HTTP.

Leaving the websocket.go:467 /tmp write and the repo-level e2e/CI gap as you framed them — deferred to M1 Phase 3 and tracked in the plan respectively.

@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review: M0 Phase 0 upstream bump

Reviewed the diff (CHANGELOG, Makefile, go.mod/go.sum, client package files, plan tracker, generated client bundle). Overall this is a clean, well-documented dependency bump with no hand-written source changes — solid work. A few notes:

Strengths

  • make build-client fix (npm ci instead of npm run build) is the right root-cause fix for make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests #295 — it makes the build fail loudly on lockfile drift instead of silently reverting the committed bundle. Good catch that this needed to be folded into the same PR (a later make build would otherwise revert this PR's own bundle).
  • Directional bundle verification (ctrlKey 1→5 tied to the v0.18.2 keydown-modifier fix, plus the data-lvt-force-update canary) is a meaningfully stronger check than "bundle bytes changed" — nice job catching that a plain diff can't distinguish an upgrade from a regression, which is exactly the failure mode make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests #295 hit.
  • Pruning the now-unused tdewolff/minify and tdewolff/parse indirect deps from go.mod/go.sum is a good side-effect of go mod tidy — smaller dependency surface.
  • internal/assets/client/README.md is a good addition — it documents the "generated but tracked" hazard in the one place someone regenerating the bundle will actually look.
  • Per repo convention, confirmed there's no hand-authored lvt-* attribute logic touched by this change (it's a pure dependency/version bump — internal/assets/client/*.js is upstream-generated output, not reimplemented client behavior), so the CLAUDE.md guidance about checking livetemplate/client before reimplementing an lvt-* attribute doesn't apply here.

Minor observations (non-blocking)

  • CI still can't catch a client-bundle regression (noted honestly in the PR description and plan doc) since the e2e suite is //go:build !ci and CI runs -tags=ci -skip='E2E|e2e'. That's an existing, pre-PR gap rather than something this PR needs to fix, but since this PR's own root cause (make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests #295) was exactly "a bad bundle slipped through because nothing caught it," it might be worth a fast-follow issue to run at least a small e2e/browser smoke subset in CI (even without full chromedp coverage) so this class of bug isn't solely dependent on a human running go test ./... locally before commit.
  • WithStore removal (v0.19.0) is called out as unused — worth double-checking there's no reflection-based or interface-satisfaction usage that a simple grep WithStore might miss, but given go build ./... and full go test ./... are reported green, this is almost certainly fine.
  • The plan-tracker rewrite (docs/plans/2026-07-09-ephemeral-ui-reframe.md) is large (78 lines changed) and mixed into a dependency-bump PR. It's useful context and the PR body explains why ("convention 4"), but it does make the diff larger to review than the code change itself — no action needed, just noting for anyone skimming the diff stat expecting a small PR.
  • No new automated test coverage was added (nor really needed) since this is a dependency bump with zero source changes — the manual/e2e verification described in the PR body is the appropriate substitute here, just flagging that test coverage for the actual code is unchanged by this PR (as expected).

Security

No security concerns — this only bumps first-party (livetemplate/lvt) dependencies to newer tagged releases and removes now-unused indirect deps. No new attack surface, no secrets, no injection vectors introduced.

Nice, thorough verification writeup in the PR description — the directional bundle check and the GOWORK=off callout are exactly the kind of detail that prevents this class of bug from recurring.

@adnaan

adnaan commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Second-pass notes addressed:

WithStore — resolved conclusively, not by grep. Fair challenge, so I proved it rather than asserting it: livetemplate.WithStore is undefined at v0.19.1, and a probe file referencing it fails with undefined: livetemplate.WithStore. Since our build passes, we demonstrably never referenced it — a removed exported function is a compile error, not a silent failure. Go also has no way to reach a package-level func by string name, so reflection isn't a real category here; the compiler's check is exhaustive.

CI e2e smoke subset — agreed, and it's the sharpest observation in the review. You're right that this PR's own root cause was 'a bad bundle slipped through because nothing caught it,' and the mitigation still bottoms out in a human remembering to run go test ./.... That's a real fast-follow; proposing it to the maintainer as its own issue rather than expanding this PR.

Plan-tracker size — noted and accepted. Convention 4 requires the drift land in the same commit as the code, so the ratio is by design, but it does make the diff stat misleading to a skimmer.

No new test coverage — correct and expected; zero source changes to cover.

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.

make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests

1 participant