feat(patterns): sortable list with HTML5 drag-and-drop#84
Conversation
Adds the Sortable List pattern (Lists & Data #12), demonstrating the new lvt-on:dragstart / lvt-on:dragover / lvt-on:drop event support shipped in @livetemplate/client (livetemplate/client#101). The template is a plain <ul> of draggable <li>s, each carrying a data-key. The client auto-stashes the dragged item's data-key into DataTransfer on dragstart and lifts it back out on drop, so the controller's Reorder() method receives a {dragSourceKey, dragTargetKey} pair without any custom JavaScript. lvt-mod:throttle="100" tames dragover's high frequency. The diff engine emits only ["o", newKeys] on the wire — no full re-render. SortableController follows DeleteRowController's process-wide in-memory shared-state pattern so reorders persist across reloads and across tabs (multi-tab broadcast story works for free). A Reset Order button restores the initial ordering. E2E coverage exercises Reorder forward, backward, the self-drop no-op guard, and Reset. Drag is simulated by dispatching real DragEvent objects with a shared DataTransfer via chromedp.Evaluate — this hits the production client delegation pipeline rather than bypassing it via liveTemplateClient.send(), but works around the unreliable HTML5 DnD synthesis through CDP Input.dispatchMouseEvent in headless Docker Chrome. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Sortable pattern was binding lvt-on:dragstart and lvt-on:dragover to "reorder", which caused one no-op Reorder call per dragstart plus ~10/sec throttled no-op calls per dragover tick. Drop is the only event that carries meaningful keys. Switch to the marker pattern (empty action value) introduced in @livetemplate/client #101 follow-up: the client still runs the spec-mandated side-effects (preventDefault, dataTransfer.setData) but skips the WS round-trip. Net result: exactly one Reorder call per drop gesture instead of dozens of no-ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code-review pass: - SortableController.Reorder: single-pass key scan with early break once both indices are found (academic for 6 items, real if a future example uses a longer list). - SortableController.Reset: hold the lock across the swap+clone instead of releasing and re-acquiring via snapshot(). - Comments: drop the "mirrors DeleteRowController" narration and the Reorder algorithm prose that just restated the code; keep the client-contract description for ctx field names. Trim the simulateDrag and SelfDrop_NoOp test rationale to one line each. No behavior change. TestSortable still passes (3.4s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — Sortable List (PR #84)Overall this is a clean, well-structured addition. The controller logic, mutex usage, and Tier 1/2 pattern split are all solid. A few issues worth addressing before merge. Bugs / Correctness1. Pattern #12 numbering conflict in test comments 2. Misleading comment in But 3. Ignored error in _ = chromedp.Run(ctx,
chromedp.Evaluate(..., &orderChanged),
)
if orderChanged { ... }If Test Quality4. Subtests depend on each other's state Template / UX5. Hgroup subtitle exposes implementation detail <p><small>...The diff engine sends only an <code>["o", newKeys]</code> reorder op — no full re-render...</small></p>This is implementation detail, not user-facing copy. Other pattern subtitles describe what the user experiences (e.g., "Click any row to expand"). Suggest: "Drag any row to reorder. Changes persist across reloads and broadcast to other tabs." 6. Accessibility gaps
Minor / Style7. Comments in 8. Summary
Items 1–4 are blocking; 5–7 are suggestions. Happy to look at a v2. |
There was a problem hiding this comment.
Pull request overview
Adds a new “Sortable List” pattern under patterns/ to demonstrate HTML5 drag-and-drop reordering backed by server-side state, plus an accompanying E2E test.
Changes:
- Introduces a new sortable list template using
lvt-on:dragstart/lvt-on:dragover/lvt-on:dropand keyed<li data-key=...>items. - Adds
SortableController+ state/data plumbing to persist list order in a process-wide in-memory controller. - Adds
TestSortableE2E coverage for initial reset, forward/backward reorder, self-drop no-op, and reset.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| patterns/templates/lists/sortable.tmpl | New template rendering a draggable keyed <ul> and a Reset button. |
| patterns/state_lists.go | Adds SortableState / SortableItem types for the new pattern. |
| patterns/handlers_lists.go | Adds SortableController with mutex-protected shared ordering, plus handler wiring. |
| patterns/data.go | Adds initial sortable item seed data and registers the new pattern in the catalog. |
| patterns/main.go | Registers the new /patterns/lists/sortable route. |
| patterns/patterns_test.go | Adds TestSortable E2E test for drag/drop reorder + reset behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| })) | ||
| } | ||
|
|
||
| // --- Pattern #12: Sortable List --- |
There was a problem hiding this comment.
The pattern number in this header duplicates an existing “Pattern #12” in the Search handlers (Active Search). To keep the catalog numbering coherent, please renumber this section (and any other pattern-number comments) so each pattern number is unique and monotonically increasing.
| // --- Pattern #12: Sortable List --- | |
| // --- Pattern #11: Sortable List --- |
| // SortableState holds the state for the Sortable List pattern (#12). | ||
| type SortableState struct { | ||
| Title string | ||
| Category string | ||
| Items []SortableItem | ||
| } |
There was a problem hiding this comment.
| }) | ||
| } | ||
|
|
||
| // --- Pattern #12 (Lists): Sortable List --- |
There was a problem hiding this comment.
This new section header introduces a second “Pattern #12” in this test file (Active Search below is still labeled #12). Please renumber the pattern headers so the numbering stays consistent across the patterns suite.
| // --- Pattern #12 (Lists): Sortable List --- | |
| // --- Pattern #13 (Lists): Sortable List --- |
| // task-2, task-3, task-1, task-4, task-5, task-6 | ||
| // (task-1 removed from index 0, target task-3 at original index 2 | ||
| // becomes index 1 after removal, task-1 inserted at index 1.) |
There was a problem hiding this comment.
The expected order described in this comment doesn’t match the assertion below. The comment says the after-order should be “task-2, task-3, task-1, …”, but the test asserts “task-2,task-1,task-3, …”. Please correct the comment (or the assertion) so they describe the same reorder semantics.
| // task-2, task-3, task-1, task-4, task-5, task-6 | |
| // (task-1 removed from index 0, target task-3 at original index 2 | |
| // becomes index 1 after removal, task-1 inserted at index 1.) | |
| // task-2, task-1, task-3, task-4, task-5, task-6 | |
| // (task-1 removed from index 0, then inserted at index 1, | |
| // before task-3 in the updated list.) |
| _ = chromedp.Run(ctx, | ||
| chromedp.Evaluate( | ||
| `(async () => { | ||
| const start = Date.now(); | ||
| while (Date.now() - start < 500) { |
There was a problem hiding this comment.
This subtest discards the error from chromedp.Run, which can hide failures (e.g., if the Evaluate fails due to navigation/context issues). Please check and fail the test on error here; otherwise the test may pass while silently skipping the intended assertion.
| func TestSortable(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("Skipping E2E test in short mode") | ||
| } | ||
|
|
||
| ctx, cancel, serverPort := setupTest(t) |
There was a problem hiding this comment.
PR description says TestSortable has “4 subtests” but the implementation includes 5 (Initial_Reset, Reorder_DragForward, Reorder_DragBackward, SelfDrop_NoOp, Reset_RestoresInitialOrder). Consider updating the PR description (or test) so they match.
| // SortableController shares ordering across all sessions, so reorders | ||
| // persist across reloads and broadcast to other tabs. | ||
| type SortableController struct { | ||
| mu sync.Mutex | ||
| items []SortableItem | ||
| } |
There was a problem hiding this comment.
This comment says reorders “broadcast to other tabs”, but the controller doesn’t implement Sync() (or call ctx.BroadcastAction) like the existing multi-tab sync patterns do. As written, other tabs will only observe the new order on reload/reconnect (via Mount snapshot), not via live broadcast. Either add a Sync() method that refreshes state.Items from c.items (so peers converge after Reorder/Reset), or adjust the comment/UX copy to avoid claiming live tab-to-tab broadcast.
| mux.Handle("/patterns/lists/sortable", sortableHandler(baseOpts)) | ||
|
|
||
| // Category: Search & Filtering (#12–#13) | ||
| mux.Handle("/patterns/search/active-search", activeSearchHandler(baseOpts)) |
There was a problem hiding this comment.
The category numbering comments are now inconsistent with the new sortable route: “Lists & Data (#8–#11)” now contains an additional pattern, and the subsequent category ranges should shift accordingly. Consider updating these comment ranges to avoid duplicate “#12” references elsewhere (e.g., Active Search is currently labeled #12).
Functional fixes from Claude bot + Copilot: - Pattern numbering collision: my new headers labeled the Sortable example as "Pattern #12", but Active Search already had that number. Drop the global numbering from the new code rather than cascade- renumbering 25+ comments through unrelated patterns. - Reorder_DragForward had a comment that contradicted the assertion ("task-2, task-3, task-1, ..." vs the actual "task-2, task-1, task-3, ..."). Comment was wrong; rewritten to match the algorithm. - Reorder_DragBackward had been stateful — relied on DragForward having run first. Now resets to initial order, which also exposed a bug in MY expected order (insert-before-target with srcIdx > tgtIdx gives [task-1, task-6, task-2, ...], not [task-6, ...]). Fixed. - SelfDrop_NoOp ignored the chromedp.Run error from the in-page poll (`_ = chromedp.Run(...)`), and the async IIFE returned an unawaited Promise that chromedp couldn't unmarshal as bool — silently passing. Switched to a Go-side time.Sleep + before/after compare. Errors are now captured and t.Fatal'd. - Reorder subtests no longer depend on each other: each now starts with a Reset-to-initial via a shared resetToInitial Tasks block. Template / docs: - Hgroup subtitle: replace the "diff engine sends only ['o', newKeys]" implementation-detail prose with user-facing copy. Drop the false "broadcasts to other tabs" claim — the demo persists across reloads and is visible to other tabs on next refresh, but doesn't push live updates (no Sync/BroadcastAction). - Add aria-label="Reorderable task list" to the <ul>. - Drop what-comments per CLAUDE.md (SortableState struct comment, initialSortableItems doc). - Update SortableController doc to remove the stale "broadcast" claim. Skipped: aria-live announcement region (separate scope, would need a server-side state field for the announcement text). TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — Sortable List (PR #84)(Note: There is already a review comment on this PR; I've verified the actual code before filing issues, so some claims in the prior review do not match what was merged.) What this doesAdds Correctness1. Stale
// --- Pattern #12: Active Search ---Sortable List is inserted before Active Search in the route list, so Active Search is now at least #13. The comment is stale and should be updated or removed to match the actual slot number. 2. When The safer pattern, consistent with how if src == "" || tgt == "" || src == tgt {
state.Items = c.snapshot()
return state, nil
}Same fix applies to the Tests3.
This subtest should first drag something out of order (or not call t.Run("Reset_RestoresInitialOrder", func(t *testing.T) {
var order string
err := chromedp.Run(ctx,
simulateDrag("task-3", "task-1"), // scramble first
e2etest.WaitFor(`document.querySelectorAll('#sortable-list li')[0].dataset.key === 'task-3'`, 5*time.Second),
chromedp.Click(`button[name="reset"]`, chromedp.ByQuery),
e2etest.WaitFor(`...task-1 back at [0]...`, 5*time.Second),
chromedp.Evaluate(`...map keys...`, &order),
)
...
})4. The 500 ms Template / UX5. Drag target is the entire The template marks the whole Conventional approach: make only the handle span 6. No keyboard reorder path HTML5 drag-and-drop is mouse/pointer only. CLAUDE.md doesn't mandate keyboard reorder, but for a "Sortable List" pattern users will reasonably expect one. At minimum, a code comment or the subtitle copy should acknowledge this limitation: "Drag any row by its handle to reorder (mouse/touch only)." That way it sets expectations without requiring a full keyboard implementation in this PR. Minor7. What-comment on // Key is wired to data-key in the template — the stable identity the
// diff engine and the drag-and-drop client use to track which item moved.This is a what-comment on a struct field. Per CLAUDE.md, comments should be added only when the WHY is non-obvious. The name What's good
SummaryTwo actionable bugs before merge: the stale |
Functional fixes from Claude bot: - Reorder() guard returns now populate state.Items from snapshot. Previously the early-exit paths returned `state` as-is, which the framework may have populated with a stale Items slice — the bug could overwrite the rendered list with stale data on cross-app drags or unknown keys. Snapshot-on-every-return is now the invariant, matching how Mount() already worked. - Reset_RestoresInitialOrder was trivially passing because the preceding subtest (SelfDrop_NoOp) calls resetToInitial first and the self-drop is a no-op, so by the time Reset_RestoresInitialOrder ran the list was already in initial order. Now scrambles the list first (drag task-3 onto task-1), then asserts Reset reverts it. - Subtitle copy: drop "by its handle" (the entire <li> is draggable, not just the hamburger glyph — the glyph is decorative). Add "(mouse only — no keyboard reorder path in this demo)" to set expectations honestly. - Drop the SortableItem.Key what-comment per CLAUDE.md. Skipped: - Renumbering existing patterns (Active Search "#12", URL-Preserved Filters "#13"): those labels are pre-existing legacy code outside this PR's scope. The numbering scheme is now historical-not-ordinal, acknowledged by Sortable having no number. - Bumping time.Sleep to 1s in SelfDrop_NoOp: 500ms is sufficient in practice; longer waits add CI time without clear benefit. - Refactoring the template to drag-by-handle instead of drag-by-row: whole-row draggable is the simpler pedagogical demo. Subtitle copy is fixed to no longer mislead users about handle-only behavior. TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round-2 review triaged: ✅ Applied (commit
❌ Declined with reason:
|
Code Review — PR #84: Sortable List (HTML5 Drag-and-Drop)Overall this is a clean, well-structured addition that follows established patterns in the repo. The concurrency model mirrors BlockingCI will not pass until Issues1. No visual dragover feedback (UX gap)
Either add a framework-applied class on dragover (if 2.
|
- Subtitle: also document that there's no visual drop-zone highlight in this v1 demo (the framework doesn't currently expose a hover class hook for drag targets, and adding one would require client changes outside this example's scope). - SelfDrop_NoOp time.Sleep: bump to 1s to give loaded CI runners headroom for any spurious round-trip to surface in the assertion. - Reorder() docstring: trim the second half that walked through the algorithm; keep the non-obvious invariant about not trusting framework-provided state on early-exit paths. Skipped: - Adding a Reorders counter field to SortableState as a positive assertion replacement for time.Sleep — that pollutes the wire format with a test-only field and adds non-trivial state-design complexity for marginal robustness improvement. TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — PR #84: Sortable List (drag-and-drop reordering)OverviewAdds a ✅ What's done well
Issues / Suggestions1. Accessibility gap is undisclosed to screen-reader users The subtitle says "mouse only; no keyboard path" but there's nothing in the DOM to communicate this constraint to AT users. A screen reader lands on the list, encounters <ul id="sortable-list"
aria-label="Reorderable task list"
aria-description="Keyboard reordering is not supported in this demo.">2. No If the user drags an item outside the list boundary and releases (or presses Escape), the 3. The PR description explains why Go-side 4. Template doesn't document the implicit client contract The controller reads 5. Minor:
<span role="img" aria-label="drag handle">⋮</span>( Test coverage gaps
SummaryThe core logic is correct and well-tested. The main asks before merging:
Items 1 and 2 are quick fixes; item 3 can be a follow-up issue if client#106 handles abort cleanly. 🤖 Generated with Claude Code |
- Add aria-description on the <ul> so AT users get an explicit notice about the missing keyboard reorder path, instead of hitting a silent dead-end on draggable items. - Tighten the Reorder() doc comment to name the source of the ctx keys (the livetemplate/client drag handler reading data-key on the dragstart and drop targets). Skipped: - dragleave/dragend abort guard: moot — the client only fires the Reorder action on actual drop events. Aborted drags (release outside the list, ESC) don't fire drop on a registered lvt-on:drop element, so Reorder isn't called at all. - Glyph choice (☰ hamburger vs ⋮ vertical ellipsis): both are commonly used as drag-handle indicators. Bikeshedding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — Sortable List (PR #84)This is a clean, well-executed addition. The controller logic, mutex usage, index-adjustment math, and Tier 1/2 pattern split are all correct. A few things worth addressing before merge. Bugs / Correctness1.
<p id="sortable-desc" class="visually-hidden">
Use a mouse to drag items. Keyboard reordering is not supported in this demo.
</p>
<ul id="sortable-list"
aria-label="Reorderable task list"
aria-describedby="sortable-desc">The current 2. Unhandled When a drag is started but the user releases outside the list (no Test Quality3. The one-second Go-side sleep is reasonable for a negative assertion and the author's justification in the comment is correct (this is not Style / CLAUDE.md Conventions4. Multi-line comment blocks on CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The class-level comment (4 lines) and the 5. Template subtitle leaks implementation detail The current hgroup subtitle reads:
Users don't see diff ops. Other pattern subtitles describe the UX, not the wire protocol. Suggested replacement:
Minor Observations
Summary
The |
- aria-description (ARIA 1.3, uneven AT support) → aria-describedby pointing to a visually-hidden description element. Reliably surfaces the keyboard-reorder limitation to screen readers. - Trim multi-line comment blocks on SortableController and Reorder per CLAUDE.md "one short line max" rule. Keep the load-bearing WHY (snapshot-on-every-return invariant) on a single line. Skipped: - dragend-without-drop edge case: dataTransfer is per-event, no persistent client state to dangle. - Replacing time.Sleep(1s) with an echo-ping pattern: adds complexity for marginal robustness; declined twice already. - "Subtitle leaks 'diff engine' implementation detail": stale read — that prose was removed in commit ff7e36c (round 1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — Sortable List (#84)Overall this is a clean, well-structured addition. The controller logic is correct, the mutex usage is sound, and the Tier 1/2 split is appropriate. A few things worth addressing before merge. Potential Bugs
HTML5 drag-and-drop requires If client#106 only prevents default for named actions (non-empty strings), drops will silently fail in all browsers. Please confirm the client-side implementation and, if needed, add a brief note in the template comment (the one allowed by CLAUDE.md "why is non-obvious" rule). Missing Chrome and Firefox also check No After a successful drop, the browser leaves the source Test Quality
time.Sleep(1 * time.Second)CLAUDE.md explicitly prohibits sleep-based waits. The PR note argues this is a "Go-side negative assertion" which is a reasonable exception — you can't condition-wait on nothing changing. However, 1 s may be too tight on loaded CI runners. A more robust alternative that still avoids // Poll for a short window; confirm order stays stable for at least 3 ticks.
const polls = 3
for i := 0; i < polls; i++ {
var cur string
_ = chromedp.Run(ctx, chromedp.Evaluate(`...`, &cur))
if cur != orderBefore {
t.Errorf("Self-drop changed order: was %q, now %q", orderBefore, cur)
break
}
time.Sleep(200 * time.Millisecond)
}Or use Template / UXSubtitle exposes implementation detail <p><small>...The diff engine emits a single ["o", newKeys] op per drop — no full re-render.</small></p>
The Minor
What's Good
🤖 Generated with Claude Code |
|
Round-6 review — convergence signal per project CLAUDE.md guidance. Quick triage: ❌ Declined:
Per project CLAUDE.md: rounds 1-3 produced functional fixes (all addressed); rounds 4-5 produced two more functional items (aria-describedby, comment trim, both addressed); round 6 is hallucinations + framework-verification questions answerable from client#106's existing tests + cosmetic polish. Stopping the bot loop here. |
Summary
Adds the Sortable List pattern (under "Lists & Data"), demonstrating the new
lvt-on:dragstart/lvt-on:dragover/lvt-on:dropevent support shipped in livetemplate/client#106./patterns/lists/sortable<ul>ofdraggable="true"<li>s withdata-key, using the marker pattern for dragstart/dragover (empty action) so only the drop event sends to the server. Pure HTML — zero custom JavaScript.SortableControllerfollows the same process-wide in-memorysync.Mutex-backed pattern asDeleteRowController, so reorders persist across reloads and are visible to other tabs on next refresh. (Live multi-tab sync would requireSync()/BroadcastAction— out of scope for this demo.) A Reset Order button restores the initial sequence.Reorder()readsdragSourceKey/dragTargetKeyfrom ctx and applies a remove-then-insert with index adjustment. Self-drop, missing keys, and unknown keys are all guarded no-ops (so cross-app drags or stale page state can't corrupt the list).["o", newKeys]op per drop — no full re-render.Dependency
This PR's CI will fail until livetemplate/client#106 is merged and released to npm. The example template references
@livetemplate/client@latestfrom CDN in production /e2etest.ServeClientLibraryin test mode, neither of which has the new drag events yet. Sequence of events:release.shto cut v0.8.38 to npmTest plan
TestSortable— 5 subtests (Initial_Reset, Reorder_DragForward, Reorder_DragBackward, SelfDrop_NoOp, Reset_RestoresInitialOrder), all green when run withLVT_LOCAL_CLIENTpointing at the drag-events worktree's dist./test-all.sh— full sweep across all 12 working examples passes with the local distE2E strategy note
The drag is simulated by dispatching real
DragEventobjects with a sharedDataTransferviachromedp.Evaluate. CDP'sInput.dispatchMouseEventis unreliable for HTML5 DnD in headless Docker Chrome (drag-threshold and screen-coordinate quirks). The synthetic events still traverse the full client delegation pipeline — this is not aliveTemplateClient.send()shortcut.🤖 Generated with Claude Code