Add Claude Code GitHub Workflow#2
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review - PR #2: Add Claude Code GitHub WorkflowOverviewThis PR adds GitHub Actions workflows for Claude Code integration, enabling AI-assisted development through the @claude mention system. The implementation includes two workflows: one for interactive @claude mentions and another for automated code reviews on PRs. Code Quality & Best Practices ✅Strengths:
Suggestions:
Potential Issues & Bugs
|
4 tasks
adnaan
added a commit
that referenced
this pull request
May 2, 2026
Bug fixes from Claude + Copilot review on PR #108: 1. **findContainer no longer falls back to unscoped wrapper walk** (Claude #1) Removed `wrapper.querySelector('[data-key]')` fallback that could silently return a container belonging to a *different* keyed range when the wrapper has more than one. Returns null instead so the caller falls back to full rebuild. 2. **applyUpdateRow no-morphdom path now fires lifecycle hooks + notifies host** (Claude #2) The `row.replaceWith(newRow)` fallback never fired lvt-mounted/lvt-destroyed and didn't increment nodesAddedThisRender, so post-render directive scans were incorrectly skipped on rows containing newly-revealed children. 3. **Override childrenOnly:false for per-row morph** (Copilot #1) The main morphdom call uses childrenOnly:true (correct for the wrapper). For a single-row morph we must diff the row root too — class/style/aria attributes produced by statics+dynamics may have changed. 4. **Robust handling of failed targeted ops** (Copilot #2) When apply() returns null, the treeState was updated but the live DOM wasn't. Previously we'd still convert placeholders → either tell morphdom to skip (stale DOM) or leave an empty container (10k rows destroyed). Now we re-render full HTML from treeState (new `TreeRenderer.renderState()`), strip any partial success markers, and let morphdom sync. 5. **lvt-ignore check now includes wrapper itself** (Copilot #3) The walk previously stopped *before* checking the wrapper, allowing targeted-apply to mutate DOM inside an lvt-ignore'd wrapper. 6. **applyReorder logs a warning when newKeyOrder is shorter than children** (Claude #3) Documents the protocol assumption (server always sends full new order) and surfaces silent data-loss if the protocol changes. 7. **CSS escape fallback warns on unsupported chars** (Claude #4) Polyfill only handles `"` and `\`; warn (not silently miss) when keys contain `[]:.()#>~+*=^$|! ` whitespace etc. Tests: - 4 new range-dom-applier tests covering (1), (2), (3), (5) - Fixture pre-populates container cache via canApplyTargeted-equivalent call so a/p ops can locate the container (mirrors production flow now that the unscoped fallback is gone) - 510 unit tests pass (was 507 — net +3) - TestLargeTable + TestLargeTable_DeleteLatency_10k green at 1958 ms (ceiling 2500 ms), targeted-apply hits=1 - TestTodosE2E full suite green
adnaan
added a commit
that referenced
this pull request
May 2, 2026
* feat: per-op targeted DOM mutation for range diff ops (#107) Bypasses full tree HTML reconstruction + morphdom diff for keyed-range diff ops (r/u/i/a/p/o). The targeted-apply path mutates the live DOM directly via [data-key="..."] queries, while a sentinel comment + data-lvt-targeted-skip marker tells morphdom to short-circuit the subtree on coexisting sibling updates. Pre-fix, single-row delete on a 10k-row table took 6-8 seconds in Chrome desktop because the client deep-cloned 10k items, rebuilt 5MB of HTML, and ran morphdom over the entire range. Post-fix, the same op completes in ~1.7s wall-clock (server compute + WS + targeted DOM mutation + post-render scans). The targeted-apply portion itself is sub-100ms; the residual cost is in post-morphdom side-effect rescans (handleScrollDirectives, changeAutoWirer.wireElements) which still walk the wrapper at O(N) — that's a follow-up. Architecture: - New state/range-dom-applier.ts: per-op DOM mutation module with findContainer + canApplyTargeted + apply (r/u/i/a/p/o) + cleanupMarkers - state/tree-renderer.ts: extracted renderRangeItem helper; applyUpdate now takes optional canApplyTargeted callback, mutates treeState in place for eligible keys, returns targetedOps + emits comment placeholder in result.html for skipped ranges - livetemplate-client.ts updateDOM: dispatches targeted ops before morphdom; post-processes placeholder comments; morphdom's onBeforeElUpdated returns false for marked subtrees; cleanup in try/finally - types.ts: TargetedRangeOp interface + optional UpdateResult.targetedOps field (additive, no breakage for downstream consumers) Includes 23 jsdom unit tests in tests/range-dom-applier.test.ts (per-op coverage, focus preservation, lifecycle hooks, skip mechanism with morphdom). Closes #107 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: add window.__lvtTargetedHits observability hook for tests (#107) E2E tests assert the targeted-apply path was actually taken (vs silently hitting the fallback). Cost: one integer increment per applied op. * perf(client): skip wrapper-wide directive scans when no nodes added (#107) Profile of a 10k-row delete revealed ~360ms wasted in post-render directive scans (setupFxDOMEventTriggers, setupDOMEventTriggerDelegation, handleScrollDirectives, etc.) — each does qsa("*") on the wrapper, which contains ~80k descendants at N=10k. For a delete-only render, no new elements need wiring, so all wrapper-wide scans become pure waste. Track DOM additions via: - Existing morphdom onNodeAdded callback (now also increments counter) - New onNodeAdded callback in RangeDomApplier context, fired by i/a/p ops When `nodesAddedThisRender === 0` after both phases, skip the scans. changeAutoWirer.wireElements still runs (it has its own eviction loop for stale wirings on removed elements). Measured impact (10k-row LargeTable delete in headless Chrome): - updateDOM: 692ms → 447ms (-35%) - Browser-side wall-clock: ~1500ms → ~1170ms (-22%) Limitation: if the server adds new directive attributes to an existing element via attribute morph (e.g. adds lvt-fx:keydown to a button that didn't have it), the listener won't be wired until a render that DOES add a node fires the next scan. Rare in practice; opt out by adding data-lvt-force-update on the affected element. * fix: address bot review comments (#107) Bug fixes from Claude + Copilot review on PR #108: 1. **findContainer no longer falls back to unscoped wrapper walk** (Claude #1) Removed `wrapper.querySelector('[data-key]')` fallback that could silently return a container belonging to a *different* keyed range when the wrapper has more than one. Returns null instead so the caller falls back to full rebuild. 2. **applyUpdateRow no-morphdom path now fires lifecycle hooks + notifies host** (Claude #2) The `row.replaceWith(newRow)` fallback never fired lvt-mounted/lvt-destroyed and didn't increment nodesAddedThisRender, so post-render directive scans were incorrectly skipped on rows containing newly-revealed children. 3. **Override childrenOnly:false for per-row morph** (Copilot #1) The main morphdom call uses childrenOnly:true (correct for the wrapper). For a single-row morph we must diff the row root too — class/style/aria attributes produced by statics+dynamics may have changed. 4. **Robust handling of failed targeted ops** (Copilot #2) When apply() returns null, the treeState was updated but the live DOM wasn't. Previously we'd still convert placeholders → either tell morphdom to skip (stale DOM) or leave an empty container (10k rows destroyed). Now we re-render full HTML from treeState (new `TreeRenderer.renderState()`), strip any partial success markers, and let morphdom sync. 5. **lvt-ignore check now includes wrapper itself** (Copilot #3) The walk previously stopped *before* checking the wrapper, allowing targeted-apply to mutate DOM inside an lvt-ignore'd wrapper. 6. **applyReorder logs a warning when newKeyOrder is shorter than children** (Claude #3) Documents the protocol assumption (server always sends full new order) and surfaces silent data-loss if the protocol changes. 7. **CSS escape fallback warns on unsupported chars** (Claude #4) Polyfill only handles `"` and `\`; warn (not silently miss) when keys contain `[]:.()#>~+*=^$|! ` whitespace etc. Tests: - 4 new range-dom-applier tests covering (1), (2), (3), (5) - Fixture pre-populates container cache via canApplyTargeted-equivalent call so a/p ops can locate the container (mirrors production flow now that the unscoped fallback is gone) - 510 unit tests pass (was 507 — net +3) - TestLargeTable + TestLargeTable_DeleteLatency_10k green at 1958 ms (ceiling 2500 ms), targeted-apply hits=1 - TestTodosE2E full suite green * fix: silent per-op skips now propagate to apply() fallback (#107) Round-2 bot review (Claude) caught a correctness gap in the round-1 fixes: per-op methods (applyUpdateRow, applyInsertAfter, applyAppend, applyPrepend, applyReorder) had several early-return paths for stale state (row not found, item state unavailable, anchor missing, render failed). These returned silently and apply() still reported the container as a success — so updateDOM marked it with TARGETED_APPLIED_ATTR, morphdom skipped the subtree, and the live DOM stayed out of sync with treeState forever. Changes: 1. Each per-op method now returns boolean. apply() collects the result and returns null if any op silently no-op'd, triggering the updateDOM full-rebuild fallback (added in round 1). 2. applyRemove still returns true on missing row (idempotent — common when the same row got removed by an earlier op or previous render). 3. itemLookup moved from `wireItemLookup()` post-construction call into the required RangeDomApplierContext field. Eliminates the silent footgun where forgetting to wire it would silently no-op every `u` op (treeState updated, DOM stale). 4. Added 3 jsdom unit tests covering the new contract: - apply() returns null when u op finds row in DOM but item state missing - apply() returns null when i op anchor is missing - r op apply() succeeds even when row already gone (idempotency) Tests: 513 unit tests pass (+3 new). TestLargeTable + TestLargeTable DeleteLatency_10k green at 1523ms with targeted-apply hits=1. Notes for follow-up (not addressed in this PR): - Claude flagged that data-lvt-force-update (the escape hatch for attribute-only changes that skip directive scans when nodesAddedThisRender===0) isn't in user-facing docs. The inline comment in updateDOM documents it but the public docs in livetemplate/docs/references/ should be updated. Out of client repo scope; deferred. * fix: address round-3 bot review (#107) Three Claude follow-up findings on commit 3c99b99: 1. **applyReorder fires lvt-destroyed on dropped children** When `newKeyOrder` is shorter than the existing children set, dropped children were silently discarded by `replaceChildren(fragment)`. Any user teardown registered via lvt-destroyed (timer cancellation, observer disconnect, etc.) never ran. Now: walk byKey for entries missing from the new order set, fire lvt-destroyed on each subtree before the replaceChildren. 2. **Directive-touched signal complements nodes-added signal** The `nodesAddedThisRender > 0` skip-scan optimization missed the case where the server adds a directive attribute to an existing element via attribute morph (e.g. `lvt-fx:keydown` on a button that didn't have it). Without scanning, the listener never wires and the user has to know about `data-lvt-force-update` as the escape hatch — which isn't publicly documented. Now morphdom.onBeforeElUpdated checks toEl for any `lvt-*` attribute and sets `directiveTouchedThisRender = true`. The scan gate becomes `nodesAdded > 0 || directiveTouched`, preserving the delete-only fast path while wiring listeners on attribute-only morphs that introduce new directives. Cost: per onBeforeElUpdated call, scan toEl.attributes for any name starting with "lvt-" (4-char prefix check via charCodeAt to avoid String.startsWith allocation overhead). At 80k descendants that's ~10ms — well below the post-render scans we'd otherwise run unnecessarily. 3. **__lvtTargetedHits gated behind explicit init** Previously incremented `window.__lvtTargetedHits` unconditionally, which polluted the global object in production. Now: increments only when the property is already present on `window` — tests initialize it via `window.__lvtTargetedHits = 0` before measuring; production never sets it so the increment branch is skipped. 4. **itemLookup O(N) acknowledged with comment** (Claude #3, low pri) Linear scan is bounded; building a Map<key,item> per render would amortize multi-u-op renders but adds caching complexity. Documented the trade-off in the lookup callback; revisit if profiling shows it on the hot path. Tests: 514 unit pass (+1 new applyReorder lvt-destroyed test). TestLargeTable + TestLargeTable_DeleteLatency_10k green at 1798ms, targeted-apply hits=1. * fix: address round-4 bot review (#107) Three Claude follow-up findings on commit 02c1755: 1. **Unknown op type now triggers fallback** (forward-compat) The switch's `default:` case left `opOK = true`, so any future unrecognised op type was silently treated as a success and the targeted-skip marker would have left the live DOM stale. Now sets `opOK = false` (and warns), forcing a full morphdom rebuild. 2. **Atomic insert: applyAppend/Prepend/InsertAfter bail before any DOM mutation on partial render failure** Previously, if `renderAndParse` failed for the second of two items, the first was already inserted with `lvt-mounted` fired. The fallback then ran a full morphdom rebuild — adding the same row again and firing `lvt-mounted` a second time. Refactored all three insert paths to render into a scratch DocumentFragment first, only splicing into the live DOM (and firing onNodeAdded + lvt-mounted) when ALL items rendered successfully. Extracted the common path into `renderItemsAtomic`. 3. **staticsContainKeyAttribute boundary check** (precision) `s.includes('data-key=')` had two false-positive classes: longer attribute names like `data-keystone=`, and `data-key=` appearing in text/attribute values. Tightened to a regex requiring the attr name be preceded by whitespace or `<` and followed by `\s*=` — eliminates the longer-name class. Quoted-value class is acknowledged as a known limitation in a code comment (false positives are SAFE: they just trigger the full-rebuild fallback without corrupting state). (4 minor: __lvtTargetedHits Symbol-key — declined; the existence-check guard is sufficient for the test-only use case and Symbol keys significantly complicate test setup. Documented in the existing comment.) Tests: 519 unit pass (+5 new). TestLargeTable + DeleteLatency_10k green at 1733ms with targeted-apply hits=1. * chore: address round-5 cleanup nits (#107) Three small Claude follow-up items on commit c26c003: 1. **applyReorder JSDoc**: said dropped rows fire no lvt-destroyed, but commit 02c1755 already fixed that to fire them. Comment was stale; updated to describe the correct (current) behavior. 2. **renderItemsAtomic dead `container` parameter**: leftover from the inline-helper refactor in c26c003. The splice callback closes over its container, so renderItemsAtomic doesn't need the param. Removed from signature + all 3 call sites. 3. **Pre-compile staticsContainKeyAttribute regexes**: hoisted the per-attr RegExp construction to module-level constants (KEY_ATTR_REGEXES). Previously rebuilt 2 RegExps per static segment per call. At initial render that's ~hundreds of allocations; now amortized to 2 at module load. No behavior change. 519 unit tests pass. TestLargeTable_DeleteLatency_10k: 1534ms wall-clock with hits=1. * fix: address round-6 bot review (#107) Three Claude follow-up items on commit ff715fd: 1. **directiveTouchedThisRender now requires NEW lvt-* attribute** The previous check fired the flag whenever onBeforeElUpdated saw any lvt-* attribute on toEl. For high-frequency `u`-op renders on rows that ALREADY carry a directive (e.g. Todos rows with lvt-fx:animate persisting across renders), this triggered a wrapper-wide directive scan on every render — negating the scan-avoidance optimization. Now we only flag when the attribute is NEW: `toAttrs.includes(n) && !fromEl.hasAttribute(n)`. Catches the originally-intended case (server adds a new directive via attribute morph) without thrashing on stable directives. 2. **Comment on no-rollback safety in apply() failure path** When `allOpsSucceeded = false`, earlier ops that ran are NOT rolled back. The fallback rebuild from `treeRenderer.renderState()` produces the authoritative HTML and morphdom reconciles whatever partial state the live DOM is in. Documented in the early-return block. 3. **firstKnownKey now extracts from a/p item._k as a fallback** For batches containing only a/p ops on a cold cache (rare; canApplyTargeted normally warms it), apply would have failed container lookup. Now firstKnownKey samples the new items' `_k` so apply can find the container in the cold-cache case. (4 minor: TargetedRangeOp.ops discriminated union — declined; would touch the public type API. The existing `any[]` is permissive but the runtime switch + per-op return-bool already enforces correctness. Documented as a TODO in a future cleanup.) No behavior change for the LargeTable hot path. 519 unit tests pass; TestLargeTable_DeleteLatency_10k green at 1705ms with targeted-apply hits=1. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Installing Claude Code GitHub App
This PR adds a GitHub Actions workflow that enables Claude Code integration in our repository.
What is Claude Code?
Claude Code is an AI coding agent that can help with:
How it works
Once this PR is merged, we'll be able to interact with Claude by mentioning @claude in a pull request or issue comment.
Once the workflow is triggered, Claude will analyze the comment and surrounding context, and execute on the request in a GitHub action.
Important Notes
Security
There's more information in the Claude Code action repo.
After merging this PR, let's try mentioning @claude in a comment on any PR to get started!