fix: support heterogeneous range items with per-item statics#75
Merged
Conversation
When range items have different conditional branches (like different input
types in a form), they produce different statics. Previously, only the first
item's statics were used for all items, causing rendering issues.
Changes:
- Add StaticsMap to RangeData for hash-based deduplication of statics
- Collect statics from each item and detect heterogeneous ranges
- Add _sk (statics key) to items referencing their statics in StaticsMap
- Update MarshalJSON/UnmarshalJSON to include sm (statics map) in wire format
- Update test to expect Range.Statics in wire format
Wire format for heterogeneous ranges:
{"d": [{"_sk": "hash1", ...}, {"_sk": "hash2", ...}], "s": [...], "sm": {...}}
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 tasks
3 tasks
Code Review: Heterogeneous Range Items SupportThis PR introduces support for range items with different conditional branches by implementing per-item statics through a hash-based deduplication system. Overall, this is a well-designed and thoroughly implemented feature that solves a real rendering issue. Here's my detailed review: ✅ Strengths1. Clean Architecture
2. Efficient Implementation
3. Robust Error Handling
4. Comprehensive Cloning
🔍 Potential Issues & Questions1. Hash Collision Risk (Low Priority)Location: return hex.EncodeToString(h.Sum(nil))[:8] // 8 chars is enough for dedupConcern: With 8 hex chars (32 bits), birthday paradox gives ~50% collision probability at √(2^32) ≈ 65,536 unique statics variants. Questions:
Recommendation:
2. Memory Overhead for Heterogeneous RangesLocation: For a range with N items and K unique statics variants:
Questions:
Recommendation: Consider documenting performance characteristics or adding benchmarks. 3. Test Coverage GapLocation: PR description mentions $ go test -run TestExecArgsForm
testing: warning: no tests to runQuestions:
Recommendation: Add a unit test in this repo that:
4. Client-Side Changes RequiredLocation: PR description mentions "Related PRs: livetemplate/client" Questions:
Recommendation: Document the version compatibility requirement. 🛡️ Security Considerations✅ No Vulnerabilities Detected
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical rendering bug where range items with different conditional branches (e.g., different form input types) were all rendered using only the first item's static HTML. The solution introduces hash-based deduplication of per-item statics through a new StaticsMap field, maintaining backward compatibility for homogeneous ranges.
Key changes:
- Added
StaticsMaptoRangeDatafor storing unique statics variants by hash key - Introduced
hashStatics()function using SHA-256 to deduplicate statics across range items - Modified range processing to detect homogeneous vs heterogeneous ranges and use appropriate wire format
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
internal/parse/range.go |
Added hash-based statics deduplication logic, new buildRangeTreeWithStatics() function, and extractItemDynamicsWithStaticsKey() for heterogeneous ranges |
internal/build/types.go |
Extended RangeData with StaticsMap field, updated JSON marshaling/unmarshaling to handle "sm" key, added cloning support for statics map |
tree_test.go |
Updated test expectation to include statics field in range data serialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+20
| func hashStatics(statics []string) string { | ||
| h := sha256.New() | ||
| for _, s := range statics { | ||
| h.Write([]byte(s)) | ||
| h.Write([]byte{0}) // separator | ||
| } | ||
| return hex.EncodeToString(h.Sum(nil))[:8] // 8 chars is enough for dedup |
There was a problem hiding this comment.
The 8-character hash prefix may not provide sufficient uniqueness for deduplication in large ranges. With 8 hex characters (32 bits), the birthday paradox suggests collisions become likely after ~65,000 unique statics variants. Consider using at least 12-16 characters (48-64 bits) to provide better collision resistance, especially since the hash is used as a key in the StaticsMap which could lead to incorrect rendering if collisions occur.
Suggested change
| func hashStatics(statics []string) string { | |
| h := sha256.New() | |
| for _, s := range statics { | |
| h.Write([]byte(s)) | |
| h.Write([]byte{0}) // separator | |
| } | |
| return hex.EncodeToString(h.Sum(nil))[:8] // 8 chars is enough for dedup | |
| const staticsHashPrefixLen = 16 // 16 hex chars (64 bits) for low collision risk | |
| func hashStatics(statics []string) string { | |
| h := sha256.New() | |
| for _, s := range statics { | |
| h.Write([]byte(s)) | |
| h.Write([]byte{0}) // separator | |
| } | |
| return hex.EncodeToString(h.Sum(nil))[:staticsHashPrefixLen] |
Comment on lines
+255
to
+256
| // Use first item's statics as default (for backward compat) | ||
| rangeTree.Statics = items[0].statics |
There was a problem hiding this comment.
In the heterogeneous case, both Range.Statics and Range.StaticsMap are set. Setting Range.Statics to items[0].statics while also providing StaticsMap creates ambiguity about which should be used. According to line 260's comment "Not used when StaticsMap is present", the Statics field should likely be set to nil in the heterogeneous case to make it clear that StaticsMap is the source of truth.
Suggested change
| // Use first item's statics as default (for backward compat) | |
| rangeTree.Statics = items[0].statics | |
| // In the heterogeneous case, StaticsMap is the source of truth. | |
| // Leave TreeNode.Statics nil to avoid ambiguity. | |
| rangeTree.Statics = nil |
Comment on lines
+340
to
+341
| // Include statics for range items (homogeneous case) | ||
| if len(tn.Range.Statics) > 0 { |
There was a problem hiding this comment.
In the MarshalJSON method, when Range.Statics is set for a heterogeneous range (line 256 in range.go), it will overwrite the node's own Statics field (line 323-325) since both set result["s"]. This means the heterogeneous range will serialize with the first item's statics in the "s" field alongside "sm", which is inconsistent with the intent expressed in line 260's comment that Statics is "Not used when StaticsMap is present".
Suggested change
| // Include statics for range items (homogeneous case) | |
| if len(tn.Range.Statics) > 0 { | |
| // Include statics for range items (homogeneous case). | |
| // When StaticsMap is present (heterogeneous case), Statics is not used. | |
| if len(tn.Range.Statics) > 0 && len(tn.Range.StaticsMap) == 0 { |
Comment on lines
+407
to
+431
| case "sm": | ||
| // Parse statics map for heterogeneous ranges | ||
| if smRaw, ok := value.(map[string]interface{}); ok { | ||
| if tn.Range == nil { | ||
| tn.Range = &RangeData{} | ||
| } | ||
| tn.Range.StaticsMap = make(map[string][]string) | ||
| for hash, staticsRaw := range smRaw { | ||
| if statics, ok := staticsRaw.([]interface{}); ok { | ||
| staticsStrs := make([]string, len(statics)) | ||
| for i, s := range statics { | ||
| if str, ok := s.(string); ok { | ||
| staticsStrs[i] = str | ||
| } else { | ||
| return fmt.Errorf("invalid statics map entry at %s[%d]: expected string, got %T", hash, i, s) | ||
| } | ||
| } | ||
| tn.Range.StaticsMap[hash] = staticsStrs | ||
| } else { | ||
| return fmt.Errorf("invalid statics map entry at %s: expected array, got %T", hash, staticsRaw) | ||
| } | ||
| } | ||
| } else { | ||
| return fmt.Errorf("invalid statics map: expected object, got %T", value) | ||
| } |
There was a problem hiding this comment.
The UnmarshalJSON method processes the "s" key independently from range data. When both "d" (range items) and "s" (statics) are present with "sm" (statics map), the statics will be set on tn.Statics rather than tn.Range.Statics. However, in the heterogeneous case, MarshalJSON writes Range.Statics to the "s" field (line 342). This creates an asymmetry that could cause the Range.Statics field to be lost during round-trip serialization.
Comment on lines
+139
to
+143
| // Collect item with its statics | ||
| itemsWithStatics = append(itemsWithStatics, rangeItemWithStatics{ | ||
| tree: itemTree, | ||
| statics: itemTree.Statics, | ||
| hash: hashStatics(itemTree.Statics), |
There was a problem hiding this comment.
The hash is computed for every range item even when the range is homogeneous (all items have identical statics). This adds unnecessary cryptographic hashing overhead. Consider computing hashes lazily or only when heterogeneity is detected after comparing the first two items' statics arrays directly.
Comment on lines
+179
to
+183
| // Collect item with its statics | ||
| itemsWithStatics = append(itemsWithStatics, rangeItemWithStatics{ | ||
| tree: itemTree, | ||
| statics: itemTree.Statics, | ||
| hash: hashStatics(itemTree.Statics), |
There was a problem hiding this comment.
The hash is computed for every range item even when the range is homogeneous (all items have identical statics). This adds unnecessary cryptographic hashing overhead. Consider computing hashes lazily or only when heterogeneity is detected after comparing the first two items' statics arrays directly.
Comment on lines
+42
to
+43
| // Used when all items share the same statics (homogeneous ranges). | ||
| // When items have different statics, StaticsMap is used instead. |
There was a problem hiding this comment.
The comment states "Statics are the static HTML parts for rendering range items" but doesn't clearly document the semantic change introduced by StaticsMap. When StaticsMap is present, this field's usage changes from being the authoritative source to being "for backward compat" (as noted in line 255). This should be clarified in the comment to avoid confusion about when to use Statics vs StaticsMap.
Suggested change
| // Used when all items share the same statics (homogeneous ranges). | |
| // When items have different statics, StaticsMap is used instead. | |
| // When StaticsMap is not present, this slice is the authoritative source | |
| // of statics and is used when all items share the same statics | |
| // (homogeneous ranges). | |
| // | |
| // When StaticsMap is present, consumers should treat Statics as a | |
| // backward-compatibility field and prefer StaticsMap for determining | |
| // the statics used by each item. |
✅ Performance Benchmark ResultsStatus: No significant regressions detected Benchmark ComparisonThresholds
See benchmarking guide for details. |
- Increase hash prefix from 8 to 16 chars for better collision resistance - Remove TreeNode.Statics in heterogeneous case to avoid ambiguity - Update MarshalJSON to skip Range.Statics when StaticsMap is present - Improve Statics field documentation to clarify precedence 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code ReviewSummaryThis PR introduces a solid solution for handling heterogeneous range items (items with different conditional branches producing different statics). The implementation is well-thought-out with good separation of concerns between homogeneous and heterogeneous cases. Strengths1. Clean Architecture
2. Hash Collision Resistance
3. Wire Format Optimization
4. Documentation
Issues & Concerns1. Missing Client-Side Implementation 🔴 BLOCKING
However, this PR modifies the wire format with new
Action: Verify the client PR exists and is ready to merge atomically with this change. 2. Missing Test Coverage 🔴 BLOCKING // Needed tests:
// 1. Heterogeneous range with multiple different statics
func TestHeterogeneousRangeStatics(t *testing.T) {
// Range with {{if}} producing different input types
// Verify: StaticsMap populated, _sk keys correct, Statics is nil
}
// 2. JSON marshaling/unmarshaling round-trip
func TestStaticsMapSerialization(t *testing.T) {
// Marshal tree with StaticsMap, unmarshal, verify equality
}
// 3. Homogeneous range still uses old format
func TestHomogeneousRangeBackwardCompat(t *testing.T) {
// Range with same statics for all items
// Verify: No StaticsMap, Statics field used, no _sk keys
}
// 4. Edge case: Empty range
func TestEmptyHeterogeneousRange(t *testing.T) {
// Verify: No panic, proper empty range structure
}
// 5. Clone() with StaticsMap
func TestTreeNodeCloneWithStaticsMap(t *testing.T) {
// Verify: Deep copy of StaticsMap works correctly
}The PR description mentions 3. Potential Memory Overhead
Mitigation suggestions:
4. Ambiguous State in
However, the code in if len(tn.Range.Statics) > 0 && len(tn.Range.StaticsMap) == 0 {
result["s"] = tn.Range.Statics
}Issue: What if a bug causes both Suggestion: Add validation in rangeTree.Range = &RangeData{
Items: itemTrees,
Statics: nil, // Not used when StaticsMap is present
StaticsMap: staticsMap,
}
// Add assertion in dev mode:
if len(rangeTree.Range.Statics) > 0 && len(rangeTree.Range.StaticsMap) > 0 {
panic("BUG: Both Statics and StaticsMap are populated")
}5. Hash Function Not Cryptographically Necessary
import "hash/fnv"
func hashStatics(statics []string) string {
h := fnv.New64a() // Much faster than SHA-256
for _, s := range statics {
h.Write([]byte(s))
h.Write([]byte{0})
}
return fmt.Sprintf("%016x", h.Sum64())[:16]
}Benchmark expectation: ~10x faster for typical statics arrays. 6. Missing CLAUDE.md Updates
Minor Issues7. Typo in Comment (internal/parse/range.go:27) // rangeItemWithStatics holds an item tree and its statics together.Consider adding more context: // rangeItemWithStatics holds an item tree with its statics and hash.
// Used during range processing to detect heterogeneous statics patterns.8. Deprecated Function Should Be Removed
9. Clone() Missing Initialization Check if len(tn.Range.StaticsMap) > 0 { // This is correct, but be explicit:
clone.Range.StaticsMap = make(map[string][]string, len(tn.Range.StaticsMap))This is fine, but could add a comment: Performance ConsiderationsMemory Usage:
CPU Usage:
Security ConcernsNo security issues identified. The hash is used only for deduplication, not authentication. Collision resistance is sufficient with 64-bit hash space. RecommendationsBefore Merging:
After Merging:
ConclusionThis is a well-designed solution to a real problem. The code quality is high, and the approach is sound. However, it should not be merged until:
The implementation correctly handles the wire format optimization per the tree-update-specification, properly excluding statics from updates when clients have them cached. Overall Assessment: 👍 Approve pending completion of blocking items above. Reviewed with reference to CLAUDE.md guidelines: 5-phase architecture, wire format optimization, testing strategy, and development conventions. |
For empty→items transitions, the diff engine needs access to item statics to build proper differential operations. Previously, when ShouldIncludeStatics() returned false (client has cached statics), both newNode.Statics and Range.Statics were nil, causing the client to receive null statics. This fix: 1. Always sets Range.Statics in buildRangeTreeWithStatics for internal use, independent of TreeNode.Statics (wire format optimization) 2. Adds fallback logic in extractRangeData to check Range.Statics when TreeNode.Statics is nil This ensures the todos example and similar empty→items patterns work correctly when items are added to an initially empty range. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review: Heterogeneous Range Statics SupportGreat work on this PR! The implementation solves a real problem with range items that have different conditional branches. ✅ Strengths1. Solid Architecture
2. Well-Documented Changes
3. Code Quality
🔍 Critical Issues1. Missing Test Coverage (BLOCKER) Required tests:
Add tests in internal/parse/range_test.go and internal/build/types_test.go before merging. 2. Performance Impact Consider early-exit optimization or defer hashing until AFTER detecting heterogeneity. 3. Missing Client PR Link Link to client PR and verify end-to-end compatibility before merging. 🐛 Other Issues4. Statics Field Ambiguity 5. Test Change Unexplained 6. Complex Fallback Logic 📊 Performance & SecurityPerformance:
Security: Action Items
Recommendation: Address items 1 and 2 before merging. Others can be follow-ups. Great work overall! Solution is architecturally sound and will fix the issues. Just needs test validation. |
This was referenced Dec 22, 2025
This was referenced Mar 8, 2026
This was referenced Mar 16, 2026
This was referenced Mar 25, 2026
5 tasks
adnaan
added a commit
that referenced
this pull request
Apr 25, 2026
Ticks all Session 4 boxes (patterns #17-21 — Modal Dialog, Confirm Dialog, Tabs HATEOAS, SPA Navigation, Keyboard Shortcuts) and links the merged PR livetemplate/examples#74 plus the four follow-up issues filed from the review (#75-#78). Appends six new Implementation Notes from Session 4's lessons: - Forms inside <dialog> need BindAndValidate, not ctx.ValidateForm (schema-walk doesn't reliably cover dialog-nested forms in v0.8.21) - Button name shadows form name in action resolution; two safe shapes - t.Cleanup-based restore for fetch-mock subtests (avoids cascade leak if chromedp step fails mid-subtest) - Conditional event-listener placement to avoid no-op WS round-trips on state-dependent global keydown bindings - Local-Docker chromedp small-WS-frame delivery delay (5-6s tail; CI unaffected; investigate Docker bridge, not the framework) - AI review loop convergence — "real bugs by round 4" heuristic with the Session 4 PR's 11-round trajectory as data Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
_sk(statics key) referencing its statics in StaticsMapProblem
When range items produce different statics due to conditionals (like
{{if eq .Type "number"}}...{{else}}...{{end}}), only the first item's statics were used for all items. This caused:Solution
sformatsm(statics map) with per-item_skkeysWire Format
Homogeneous (unchanged):
{"d": [...items...], "s": ["<li>", "</li>"]}Heterogeneous (new):
{ "d": [{"_sk": "a1b2", "0": "World"}, {"_sk": "c3d4", "0": "5"}], "s": ["<li>", "</li>"], "sm": {"a1b2": ["<input type=\"text\"..."], "c3d4": ["<input type=\"number\"..."]} }Test plan
Related PRs
🤖 Generated with Claude Code