test(e2e): pin explicit-submitter round-trip via chromedp

[adnaan]

Summary

Adds TestExplicitSubmitter_E2E in e2e/livetemplate_core_test.go, covering Verification item 3 of the explicit-submitter proposal: a real-browser round-trip of the explicit submitter WS field that @livetemplate/client v0.9.0 emits.

A form with two named submit buttons (save, delete) auto-intercepts onto the WebSocket; the test asserts the WS sent frame contains "submitter":"save" then "submitter":"delete", and that the controller dispatches to the matching method (counter increments on save, resets on delete). The two opposite-direction mutations rule out a stuck dispatch (always-"save" or always-"delete" would fail).

This closes the cross-repo verification gap from livetemplate#237 Phase 2 (PR #396 server-side, client#120 client-side, released as @livetemplate/client@0.9.0).

Test plan

• GOWORK=off go test -tags browser -run '^TestExplicitSubmitter_E2E$' -v ./e2e/ passes locally (CDN-resolved @livetemplate/client@0.9.0 via unpkg/@latest)
• WS frames captured via RecordWSFrames show the explicit "submitter" JSON key on both clicks
• Server-side dispatch reaches Save() then Delete() (counter flips 0→1→0; status flips idle→saved→deleted)
• go vet -tags browser ./e2e/ clean
• No new lint findings introduced by this change

[claude]

Code Review — test(e2e): pin explicit-submitter round-trip via chromedp

Overview

Solid addition. The test covers an important cross-repo contract (server-side dispatch + client WS framing) and the "opposite-direction mutations" design is clever — a stuck dispatcher cannot produce both counter=1 and counter=0. The use of existing e2etest helpers (RecordWSFrames, WaitForMessage, SetupDockerChrome) is consistent with patterns in the rest of the file.

---

Issues

1. WaitForMessage scans from the beginning of the log — ordering is not enforced (medium)

FindMessage iterates wl.messages from index 0. When the test calls wsLog.WaitForMessage('"submitter":"delete"', 3*time.Second) after the delete click, it will match the first ever message containing that substring — not the first one after the save sequence completed. In this specific test the two submitter values are disjoint ("save" vs "delete"), so there's no false positive, but the pattern is fragile. If a future refactor adds a third button or changes the sequence, a stale message from an earlier phase could satisfy the assertion silently.

The helper already exposes GetMessagesSince(since time.Time). Capturing a timestamp after the save verification and passing it to a WaitForMessage-with-since variant would make the ordering contract explicit. Alternatively, add a Reset() / Checkpoint() method to WSMessageLogger and call it between the two phases.

2. .textContent comparisons should call .trim() (minor/robustness)

// current
document.getElementById('status').textContent === 'saved'

// safer
document.getElementById('status').textContent.trim() === 'saved'

Template rendering or browser normalisation can insert surrounding whitespace. Both WaitFor conditions in this test are susceptible. Other tests in the file already use .trim() in similar conditions.

3. Asymmetric action field assertion (minor)

After the save click the test verifies saveMsg.Parsed["action"] == "save". The equivalent check is absent after the delete click. Either add it for symmetry or drop it from the save block with a comment explaining why it's not checked (if the field is always redundant given submitter).

4. Misleading // singleton comment on SubmitterTestController

The struct is instantiated with &SubmitterTestController{} — there's no singleton enforcement. The comment likely means "stateless" or "zero-value struct"; either rephrase or drop the comment.

5. Counter: 0 is a zero-value redundancy (nit)

// current
state := &SubmitterTestState{Counter: 0, Status: "idle"}

// idiomatic
state := &SubmitterTestState{Status: "idle"}

---

Positive notes

• The two-phase, opposite-mutation design is a good correctness property — worth keeping as a comment in the test.
• Console listener (chromedp.ListenTarget) aids debugging in CI; the wsLog.PrintLast(10) on WaitForMessage failures gives focused output without flooding logs.
• Using context.WithTimeout in the deferred shutdown is correct.
• Controller methods discarding *livetemplate.Context with _ is the right idiomatic choice since the context isn't needed.

---

Summary

The test is well-structured and the coverage it adds is genuinely valuable. Items 1 and 2 are the most actionable before merge; the rest are polish.

[adnaan]

Addressed items 1, 2, 3, 5 in f174af5 (frame-log clear between phases, .trim() on textContent, symmetric action assertion, dropped zero-value Counter: 0).

Declining item 4 (singleton comment). In LiveTemplate's controller pattern, singleton is the framework term for "shared instance across sessions, never cloned" (see CLAUDE.md: Controllers hold dependencies (singleton, never cloned). State holds pure data (cloned per session)). The comment isn't claiming runtime enforcement — it's declaring the lifecycle contract, which is the same convention used by FocusTestController in the same file at line 1372 ("FocusTestController is a singleton that holds dependencies"). Changing it here would diverge from the established pattern.

[claude]

Code Review: test(e2e): pin explicit-submitter round-trip via chromedp

Overview

Adds TestExplicitSubmitter_E2E to cover Verification item 3 of the explicit-submitter proposal: a full browser round-trip confirming that @livetemplate/client v0.9.0 emits "submitter":"<buttonName>" over the WebSocket and that the server dispatches to the matching controller method. The two-direction mutation design (save → counter 1, delete → counter 0) is a nice choice — a stuck dispatcher cannot pass both assertions.

---

Issues

1. WaitForMessage does not filter by direction (potential false positive)

RecordWSFrames captures both sent (browser → server) and received (server → browser) frames. WaitForMessage matches the first frame in either direction that contains the pattern string. If the server includes "submitter":"save" in its diff/patch response (e.g. as an echo or debug field), the assertion can pass on a received frame before the sent frame is logged, masking a bug where the client never emitted the field at all.

The existing WSMessageLogger exposes GetSent(), so a WaitForSentMessage helper would be straightforward:

// in testing/websocket.go
func (wl *WSMessageLogger) WaitForSentMessage(pattern string, timeout time.Duration) (WSMessage, error) {
    deadline := time.Now().Add(timeout)
    for time.Now().Before(deadline) {
        for _, msg := range wl.GetSent() {
            if strings.Contains(msg.Data, pattern) {
                return msg, nil
            }
        }
        time.Sleep(100 * time.Millisecond)
    }
    return WSMessage{}, fmt.Errorf("timeout waiting for sent message matching '%s'", pattern)
}

Then the assertion becomes:

saveMsg, err := wsLog.WaitForSentMessage(`"submitter":"save"`, 3*time.Second)

2. The "action" field assertion may be testing a field that does not exist

The sent frame assertions check both "submitter" and "action":

if got, _ := saveMsg.Parsed["action"].(string); got != "save" {
    t.Errorf(`WS action: got %q, want "save"`, got)
}

The PR summary says @livetemplate/client v0.9.0 emits an explicit "submitter" field to replace the old empty-value "action" heuristic. If the client no longer sends an "action" field in the wire message (because the server now derives ctx.Action() from "submitter" server-side), then saveMsg.Parsed["action"] will be nil, got will be "", and this assertion will always fail with a misleading error — it looks like the action was wrong, when actually the field was never sent.

Please clarify whether @livetemplate/client v0.9.0 still emits "action" alongside "submitter" in the sent frame. If not, the "action" assertions should be dropped from the sent-frame checks (they can remain as server-state checks via the DOM, which the test already does well).

---

Minor Notes

3. "Singleton" comment is misleading

// SubmitterTestController is a singleton for the explicit-submitter E2E test.
type SubmitterTestController struct{}

SubmitterTestController is an empty struct — every &SubmitterTestController{} call produces an equivalent value. The comment "singleton" suggests there is only one instance and shared state, which could confuse a reader into thinking there are field-level concerns. A simpler comment like // SubmitterTestController handles save/delete actions for the E2E test. is clearer.

4. wsLog.Clear() before the first click is good — make it symmetric

wsLog.Clear() is called before both the save click and the delete click, which makes each assertion window explicit. This is good practice and the comment explaining it is helpful. No change needed.

5. Diagnostic output on failure is solid

wsLog.Print() on click timeout and wsLog.PrintLast(10) on WaitForMessage failure give good signal. Consistent with other tests in the file.

---

Summary

The core test design is solid and the test fills a real verification gap. Two blocking items before merge:

• Must fix: WaitForMessage direction filtering (issue 1) to avoid false positives.
• Clarify: whether the "action" field assertion is actually reachable given the v0.9.0 wire format (issue 2).

Everything else (controller/state naming convention, timeout values, server lifecycle, deferred cleanup ordering) is consistent with existing tests in the file.

[adnaan]

Round-2 fixes pushed in f27f824:

Item 1 (direction filtering) — fixed. Added WaitForSentMessage to testing/websocket.go (filters GetSent() instead of all frames), routed both assertions through it. Generic helper, applies to any future WS round-trip test.

Item 2 (action field assertion). The assertion is reachable and correct. @livetemplate/client v0.9.0 emits both action and submitter on the sent frame — the new submitter field is additive, not a replacement. See dom/event-delegation.ts lines 207–216 where action = submitter.name is set on submit, then included in the WS message alongside the new message.submitter = submitter.name at lines 427–428. The test asserts both because they should agree, which pins the wire contract on both fields simultaneously. Locally, the test passed in both rounds — empirical confirmation that action is on the frame.

Item 3 ("singleton" comment). Re-declined; this is the same suggestion as round-1 item 4, which I addressed in a prior reply. In LiveTemplate's controller pattern, singleton is the documented framework term for "shared instance across sessions, never cloned" (CLAUDE.md: Controllers hold dependencies (singleton, never cloned). State holds pure data (cloned per session)). FocusTestController in the same file uses the identical phrasing at line 1372. Diverging here would create an inconsistency for future readers.

[claude]

Code Review — test(e2e): pin explicit-submitter round-trip via chromedp

Overview

This PR adds TestExplicitSubmitter_E2E, a browser-driven round-trip test that verifies the submitter field introduced in @livetemplate/client v0.9.0 is correctly transmitted over WebSocket and correctly dispatched server-side. It also adds a direction-aware WaitForSentMessage helper to testing/websocket.go. The intent is clear, the test design is sound, and the two-directional mutation trick (save → counter+1, delete → counter 0) is a good way to rule out a stuck dispatcher.

---

testing/websocket.go — WaitForSentMessage

Correctness — looks good. The method follows the same polling pattern as WaitForMessage and delegates to GetSent(), which returns a locked copy. No data races.

Suggestion — consistency with WaitForMessage: The existing WaitForMessage calls wl.FindMessage(pattern) (a method that acquires the lock internally), while WaitForSentMessage calls wl.GetSent() and loops inline. Both are correct, but for parallel pattern consider a FindSentMessage helper analogous to FindMessage. Not a blocker, just keeps the surface area consistent if callers ever need a non-blocking variant.

Minor: The doc comment is multi-line, which is fine, but the last sentence ("direction-blind matching can otherwise pass against a server frame that happens to echo the same substring") is implementation rationale rather than usage guidance. Consider trimming it to match the terse style of the rest of the file.

---

e2e/livetemplate_core_test.go — TestExplicitSubmitter_E2E

Style inconsistencies with the existing test suite

The surrounding tests use slightly different conventions; these are minor but worth aligning:

Location	Existing style	New code
log.Printf inside goroutine	"Server error: %v"	"server error: %v"
t.Fatalf for free port	"Failed to get free port: %v"	"get free port: %v"
t.Logf for shutdown	"Server shutdown warning: %v"	consistent ✓

Potential JSON fragility

wsLog.WaitForSentMessage(`"submitter":"save"`, 3*time.Second)

This substring match assumes the client serializes JSON with no space after the colon ("submitter":"save", not "submitter": "save"). If the JS runtime ever emits canonically-spaced JSON this silently breaks. Consider asserting only on the parsed field that is already checked two lines later:

// Wait for any sent frame, then assert on Parsed — avoids serialization-format coupling
msg, err := wsLog.WaitForSentMessage(`"submitter"`, 3*time.Second)
// existing: if got, _ := msg.Parsed["submitter"].(string); got != "save" { ... }

Or alternatively encode the exact byte sequence expected as a constant comment so future maintainers know it's load-bearing.

SubmitterTestController package-level declaration

The comment says "singleton" but Go has no enforcement of that — it just means stateless. The word is slightly misleading; "stateless controller" would be more accurate. Not a functional issue.

state pointer vs. framework ownership

state := &SubmitterTestState{Status: "idle"}
mux.Handle("/", tmpl.Handle(controller, livetemplate.AsState(state)))

Save and Delete operate on a by-value copy and return the new state, which is the correct pattern. This matches how other tests in this file use AsState. No issue, just flagging it was checked.

Missing assertion: server→browser update content

The test confirms the client sent the right frame and the controller ran the right method (via DOM text). It does not assert on the received (server→browser) patch frame. That's acceptable — the DOM check is an end-to-end proxy — but if the goal is pinning the wire contract in both directions, a wsLog.WaitForReceivedMessage assertion on the state diff would round it out.

wsLog.Clear() placement — correct ✓

Clear() is called before chromedp.Click, so frames from the click are guaranteed to land after the clear. Good.

Two-click ordering — correct ✓

The WaitFor DOM check after each click ensures the server has already processed the frame before WaitForSentMessage is called, so the 3-second wait should always be a near-instant hit. This is the right ordering.

---

Summary

Area	Verdict
Correctness	✅
Race safety	✅
Style alignment	⚠️ Minor (casing in error strings)
JSON pattern matching	⚠️ Fragile substring — worth hardening
Test coverage	✅ Core contract is covered
WaitForSentMessage API	✅ Useful addition; minor consistency note

Overall this is a well-reasoned test. The two issues worth addressing before merge are the JSON substring fragility and the capitalization inconsistencies. Everything else is low-priority cleanup.

[adnaan]

Round-3 changes in d34354e:

Substring fragility (✓ fixed). WaitForSentMessage now matches on "submitter" (field key only) and the Parsed map provides value-equality. Clear() before each click guarantees this matches the click-induced frame, not handshake traffic.

Capitalization (✓ partial). Aligned t.Fatalf("Failed to parse template"), t.Fatalf("Failed to get free port"), and log.Printf("Server error") with the casing used by neighboring tests. Left domain-specific messages (save click, delete click, boot) lowercase — those are descriptive labels, not generic failure prefixes, and other tests in the file mix both (e.g. server shutdown warning lowercase).

Doc-comment trim (✓ done). WaitForSentMessage doc is now one line.

Declines:

1. FindSentMessage for symmetry. Speculative API surface — no current consumer needs the non-blocking variant. Project guidance is to add abstractions only when concrete callers require them. If a future test needs it, adding it then is a 5-line change.

2. "Singleton" comment (3rd raise). Holding the prior decline. The term is the project's documented framework terminology (CLAUDE.md: Controllers hold dependencies (singleton, never cloned)), and FocusTestController line 1372 in the same file uses the identical phrasing. Rewording "singleton" → "stateless" here would create inconsistency in the test file's controller comments without changing what the code does.

[claude]

Code Review — test(e2e): pin explicit-submitter round-trip via chromedp

Overview

This is a well-structured E2E test that closes the cross-repo verification gap for the explicit-submitter feature. The design is sound: two opposite-direction mutations (counter 0→1→0, status idle→saved→deleted) make it impossible for a stuck dispatcher to pass, and wsLog.Clear() between phases cleanly isolates frame assertions to the click that produced them. The WaitForSentMessage helper is a useful addition to the testing package.

---

Issues

1. liveTemplateClient existing does not mean the WebSocket is open (potential flakiness)

e2etest.WaitFor(`typeof window.liveTemplateClient !== 'undefined'`, 5*time.Second),

The client library object may be constructed before the WS handshake completes. If the test proceeds to Click before the socket is ready the click is swallowed silently and WaitFor(status === 'saved') eventually times out — a flaky failure with no obvious cause. Consider waiting on a more specific readiness signal, e.g. window.liveTemplateClient.connected === true or a data-lt-connected attribute the framework sets on the root element, if such a signal exists.

2. Missing action field assertion after the delete click (symmetry)

The save phase asserts both Parsed["submitter"] == "save" and Parsed["action"] == "save". The delete phase only asserts Parsed["submitter"] == "delete". Either add the symmetric check or add a comment explaining why action is only validated on one branch.

3. Misleading // singleton comment

// SubmitterTestController is a singleton for the explicit-submitter E2E test.
type SubmitterTestController struct{}

Nothing enforces a single instance — the struct is stateless, not singleton. Rephrase as "stateless" or simply drop the comment; the type declaration is self-describing.

4. Redundant zero-value field (nit)

state := &SubmitterTestState{Counter: 0, Status: "idle"}
//                            ^^^^^^^^^^ zero value, can be omitted

---

What the existing review got wrong

A previous comment flagged two issues that are already handled in this diff:

• Ordering concern: The review suggested GetMessagesSince is needed. In fact wsLog.Clear() is called before each click, so WaitForSentMessage only sees frames from that click phase. No change needed.
• Missing .trim(): The review said the WaitFor conditions lack .trim(). Both conditions in the diff already call .trim(). No change needed.

---

WaitForSentMessage (testing/websocket.go)

The implementation is correct and consistent with the existing WaitForMessage poll loop. One minor note: if a future caller needs WaitForReceivedMessage, the two functions will be near-identical. A private waitForMatchIn(getter func() []WSMessage, ...) helper could DRY this up — but that's optional cleanup, not a blocker.

---

Summary

The test logic and helper are solid. The main actionable items are (1) the WS readiness race and (2) the asymmetric action assertion. Everything else is nit-level.

[adnaan]

Round-4 fixes in ef7b52d:

Item 1 (WS readiness race) — ✓ fixed. Switched the boot wait from typeof window.liveTemplateClient !== 'undefined' to !wrapper.hasAttribute('data-lvt-loading'), mirroring TestLoadingIndicator at livetemplate_core_test.go:1238. The framework's loading attribute is removed only after the JS client confirms the WS handshake.

Items 2 & 4 — already addressed in earlier rounds. The review appears to be looking at a stale diff:

• Action symmetry on delete (item 2): present at line 2516 (delMsg.Parsed["action"] == "delete"), added in round 1 commit f174af5.
• Redundant Counter: 0 (item 4): removed in the same round-1 commit; current state declaration at line 2396 is &SubmitterTestState{Status: "idle"}.

Item 3 (singleton comment) — 4th raise, declining. Per CLAUDE.md AI Code Review Workflow: Recognize the pattern and stop pushing; address remaining cosmetic nits by reply rather than by push. The term singleton is the project's documented framework terminology (controllers shared across sessions, never cloned — see CLAUDE.md Controller+State Pattern). FocusTestController line 1372 in the same file uses the identical phrasing. This is the fourth round in which I'm declining this same nit; it is cosmetic and rewording one of the two controller comments out of alignment with the other would create inconsistency rather than resolve it.

[claude]

Code Review

Overview

This PR adds TestExplicitSubmitter_E2E to verify the explicit submitter WebSocket field introduced in @livetemplate/client v0.9.0, and adds WaitForSentMessage to the testing helper. The two-direction mutation design (save→counter+1, delete→counter=0) is a solid way to rule out a stuck dispatch — a single-direction test would be weaker. Overall the approach is sound; a few issues worth addressing below.

---

Issues

Medium: WaitForSentMessage acquires the lock twice per polling tick

The new method calls wl.GetSent() on every 100ms iteration. GetSent() acquires a read lock, copies all messages, and releases — then the caller iterates the copy. That's two O(n) passes and two lock round-trips per tick, compared to WaitForMessage → FindMessage which does one pass under one lock.

The existing pattern in this file is a Find* helper (single lock, early return) paired with a WaitFor* poller that calls it. The new method should follow the same shape:

// FindSentMessage scans browser→server frames for pattern.
func (wl *WSMessageLogger) FindSentMessage(pattern string) (WSMessage, bool) {
    wl.mu.RLock()
    defer wl.mu.RUnlock()
    for _, msg := range wl.messages {
        if msg.Direction == "sent" && strings.Contains(msg.Data, pattern) {
            return msg, true
        }
    }
    return WSMessage{}, false
}

// WaitForSentMessage is WaitForMessage restricted to browser→server frames.
func (wl *WSMessageLogger) WaitForSentMessage(pattern string, timeout time.Duration) (WSMessage, error) {
    deadline := time.Now().Add(timeout)
    for time.Now().Before(deadline) {
        if msg, found := wl.FindSentMessage(pattern); found {
            return msg, nil
        }
        time.Sleep(100 * time.Millisecond)
    }
    return WSMessage{}, fmt.Errorf("timeout waiting for sent message matching '%s'", pattern)
}

This keeps the helper consistent with FindMessage/WaitForMessage and avoids the double allocation.

---

Low: shared mutable state pointer may race if the framework clones by reference

state := &SubmitterTestState{Status: "idle"}
mux.Handle("/", tmpl.Handle(controller, livetemplate.AsState(state)))

If AsState stores the pointer and the framework mutates it in place across requests (rather than cloning per session), concurrent WS events could race on Counter and Status. In a serial E2E test this is unlikely to manifest, but it's worth a brief comment confirming the framework guarantees (or switching to a value if it doesn't):

// AsState clones state per session, so the pointer is safe here.
state := &SubmitterTestState{Status: "idle"}

---

Low: "singleton" comment is misleading

// SubmitterTestController is a singleton for the explicit-submitter E2E test.
type SubmitterTestController struct{}

Nothing enforces singleton semantics here — it's just a zero-value struct allocated once in the test. "Stateless controller" or simply dropping the qualifier would be more accurate.

---

Strengths

• Two-direction mutation is the right design choice — opposite state changes make it impossible for a always-"save" or always-"delete" dispatch to pass.
• wsLog.Clear() before each click correctly isolates frame assertions to the click under test, avoiding a dependency on ordering with handshake traffic. The comment explaining why this is necessary (vs. relying on disjoint substrings) is valuable.
• Waiting on data-lvt-loading absence rather than script-tag evaluation is the correct synchronization point and the comment explaining the typeof window.liveTemplateClient pitfall is worth keeping.
• Asserting both WS frame content and DOM state (counter value, status text) gives full round-trip coverage — not just that the frame was sent, but that the server processed and rendered the result correctly.
• Dual-field WS assertion ("submitter" + "action") cross-checks client consistency without coupling to JSON spacing, which is the right tradeoff.
• WaitForSentMessage is a useful addition to the testing package and fills a real gap (direction-filtered polling).

---

Summary

The test logic is correct and the design is careful. The main ask before merge is refactoring WaitForSentMessage to follow the FindMessage/WaitForMessage pattern for consistency and to avoid the double-lock-per-tick overhead. The other two points are low-priority style nits.

[adnaan]

Round-5 changes in 8d42d42:

FindSentMessage refactor (✓ applied). WaitForSentMessage now delegates to a new FindSentMessage helper, mirroring the existing FindMessage / WaitForMessage pair. Each poll tick acquires one read lock and iterates with early return — no GetSent() allocation per tick. On reflection the round-3 decline was wrong; the pattern symmetry is structurally cleaner regardless of the marginal perf win, and the consumer is right here.

Declines:

1. State pointer race comment (item 2). The framework guarantees AsState clones state per session — documented in CLAUDE.md Controller+State Pattern (State holds pure data (cloned per session)). Other tests in this file (FocusTestController, LoadingTestController, etc.) use the same &State{} pattern without a defensive comment. Adding the comment only here would create asymmetry; the project convention is to trust the framework guarantee.

2. "Singleton" comment (5th raise). Holding the prior decline. Per CLAUDE.md AI Code Review Workflow guidance on convergence (recognize the pattern and stop pushing; address remaining cosmetic nits by reply rather than by push), this is the loop the rule describes. "Singleton" matches CLAUDE.md's documented framework terminology and FocusTestController:1372 uses identical phrasing in the same file. No change.

[claude]

Code Review: test(e2e): pin explicit-submitter round-trip via chromedp

Overview

Solid addition. This test closes a real verification gap — having the full browser→WS→server→DOM round-trip exercised for the explicit submitter field is meaningfully different from unit-testing the dispatch logic alone. The two-button design (save/delete producing opposite mutations) is a good property test: any stuck dispatch will fail at least one assertion.

---

testing/websocket.go

WaitForSentMessage duplicates the polling loop from WaitForMessage

The two implementations are structurally identical:

// existing
func (wl *WSMessageLogger) WaitForMessage(pattern string, timeout time.Duration) (WSMessage, error) {
    deadline := time.Now().Add(timeout)
    for time.Now().Before(deadline) {
        if msg, found := wl.FindMessage(pattern); found { return msg, nil }
        time.Sleep(100 * time.Millisecond)
    }
    return WSMessage{}, fmt.Errorf("timeout waiting for message matching '%s'", pattern)
}

// new — copy-paste with FindSentMessage swapped in
func (wl *WSMessageLogger) WaitForSentMessage(pattern string, timeout time.Duration) (WSMessage, error) {
    deadline := time.Now().Add(timeout)
    for time.Now().Before(deadline) {
        if msg, found := wl.FindSentMessage(pattern); found { return msg, nil }
        time.Sleep(100 * time.Millisecond)
    }
    return WSMessage{}, fmt.Errorf("timeout waiting for sent message matching '%s'", pattern)
}

If the timeout or sleep interval ever needs to change, both places will need updating. A simple fix that avoids scope creep here would be to have WaitForSentMessage delegate to a shared internal helper, or at least leave a // TODO: share loop with WaitForMessage note. Not a blocker, but it will quietly diverge.

No unit tests for the new methods

testing/websocket.go has no websocket_test.go counterpart (only http_test.go exists in that package). The new FindSentMessage/WaitForSentMessage methods are only exercised transitively through the E2E test. If the direction string ever changes (e.g. "sent" → "browser→server"), nothing catches it at the unit level. Worth a follow-up issue if not worth doing now.

---

e2e/livetemplate_core_test.go

Parsed nil access is silently handled but could be clearer

if got, _ := saveMsg.Parsed["submitter"].(string); got != "save" {
    t.Errorf(`WS submitter: got %q, want "save"`, got)
}

If parseData() failed (malformed JSON), Parsed is nil. The map access on a nil map returns the zero value, so the type assertion succeeds with got = "", and the test fails with got "" want "save". The failure message is clear enough in practice, but an explicit if saveMsg.Parsed == nil { t.Fatalf("WS frame had no parsed JSON") } guard before the field checks would make the root cause immediately obvious rather than requiring the reader to reason through nil map semantics.

SubmitterTestController "singleton" comment

// SubmitterTestController is a singleton for the explicit-submitter E2E test.
type SubmitterTestController struct{}

The existing pattern (e.g. LoadingTestController) uses "singleton that holds dependencies", which makes more sense when there are injected deps. Here the struct is empty and per-test instantiated. The comment is consistent with the convention but worth cleaning up to avoid misleading future readers — something like "SubmitterTestController handles explicit-submitter dispatch for the E2E test" would be more accurate.

Things done well

• wsLog.Clear() before each click is the right call; the comment explaining why (isolating per-click frames, avoiding substring ambiguity between "save" and "delete" handshake frames) is exactly the non-obvious explanation that belongs in a comment.
• Waiting on data-lvt-loading clearing rather than script evaluation is correct for the reason stated in the comment — good defensive synchronization.
• wsLog.PrintLast(10) / wsLog.Print() in failure paths gives immediate diagnostics without needing a re-run; this is a meaningful QoL improvement over bare t.Fatal.
• DOM state assertions (WaitFor(status === 'saved')) before WS frame assertions avoids a window where the frame is checked before the server has processed it.

---

Summary

Severity	Finding
Minor	WaitForSentMessage duplicates polling loop from WaitForMessage — risk of silent divergence
Minor	No Parsed == nil guard before field assertions — failure message is fine but root cause less obvious
Nit	SubmitterTestController "singleton" comment slightly misleading for a zero-field struct
Suggestion	Consider a follow-up issue for websocket_test.go unit tests covering direction filtering

Approachable changes overall — the test design is sound and the implementation follows project conventions closely.

[adnaan]

Round-6 reply (no push):

Round 6 contains zero new functional issues. The three items are stylistic preferences, an explicit follow-up suggestion, and a sixth re-raise of the singleton comment. Per CLAUDE.md AI Code Review Workflow: The convergence signal is 'successive rounds aren't identifying any new functional issue' — only style, phrasing, or wording alternatives on content that was already accepted. We've crossed that line.

1. Polling-loop duplication between WaitForMessage and WaitForSentMessage — declining. The duplicated portion is six lines; CLAUDE.md's anti-DRY guidance applies: Three similar lines is better than a premature abstraction. A shared internal helper for two callers with identical bodies adds an indirection layer without removing meaningful duplication.

2. Parsed == nil guard before field assertions — declining. The current failure mode (got "" want "save") is clear enough that a maintainer would print the raw frame and discover the JSON parse failure. The proposed guard adds defensive code for a path that, in practice, never fires because the frames in question are always valid JSON.

3. "Singleton" comment (6th raise) — final decline. The bot's own round-6 framing acknowledges "the comment is consistent with the convention" while still suggesting a change. Reworking it here without also changing FocusTestController:1372 and LoadingTestController:1134 — both using the identical phrasing — would create new inconsistency, not resolve any.

4. websocket_test.go unit tests — agreed as a follow-up. The new FindSentMessage/WaitForSentMessage would benefit from direct unit coverage, parallel to where http_test.go covers the HTTP helpers. Out of scope for this PR; happy to file a tracking issue if useful.

PR is ready to merge once the Test workflow on 8d42d42 completes (currently ~6min in; Test runs typically take 10-11 min on this branch).