PR Review: Fix form element sync and simplify sort options

Overview

This PR addresses two UX issues in generated LiveTemplate apps:

1. Form element sync issues - Prevents morphdom from reverting select dropdowns and search inputs
2. Sort options simplification - Shows only the first string field for sorting instead of all string fields

Code Quality & Best Practices

✅ Strengths

1. Well-documented approach: The code includes clear comments explaining the morphdom reversion detection logic
2. Consistent implementation: Changes are applied uniformly across all three kit templates (multi, single, system)
3. Dual protection strategy: Combines server-side timing checks with client-side DOM synchronization
4. Non-breaking: Uses lvt:"transient" tags properly to exclude tracking fields from state serialization
5. Good PR description: Clear explanation of the problem and solution with before/after examples

⚠️ Concerns & Suggestions

1. Magic Number - 500ms Threshold (handler.go.tmpl:309)

if input.SortBy == state.PrevSortBy && elapsedMs < 500 {

Issue: The 500ms threshold is hardcoded without explanation. This could be:

• Too generous (allowing actual duplicate submissions within 500ms to be ignored)
• Too strict (blocking legitimate rapid changes on fast networks)

Recommendation:

• Document why 500ms was chosen (testing? research?)
• Consider making it configurable or add a comment explaining the reasoning
• Typical debounce values are 250-300ms for UX, but reversion detection might need different timing

2. Client-Side Sync Script Performance (template.tmpl.tmpl:335-352)

The MutationObserver watches attributes: true on the entire document.body, which could be expensive for large DOMs.

Recommendation:

// More targeted observation
var observer = new MutationObserver(function(mutations) {
  mutations.forEach(function(mutation) {
    if (mutation.type === 'attributes' && mutation.target.tagName === 'SELECT') {
      syncSelectValues();
    }
  });
});
// Observe only the forms/toolbar, not entire body
var toolbar = document.querySelector('[name="sort_by"]').closest('form') || document.body;
observer.observe(toolbar, { attributes: true, subtree: true, attributeFilter: ['data-expected-value'] });

3. Template Range Variable Scoping (sort.tmpl:17-18)

[[- range $i, $f := .Fields]]
[[- if and (eq $i 0) (eq $f.GoType "string")]]
        <option value="[[.Name]]_asc"...

The code correctly uses $i and $f for indexed iteration, but then references .Name instead of $f.Name. This works due to Go template's implicit context, but could be more explicit for consistency. Not a bug, just a style observation.

4. LoadedCount Reset Removal (handler.go.tmpl:319)

// Note: Don't reset LoadedCount when sorting - keep all loaded items visible
// Just re-sort the existing items for better UX

Observation: This changes behavior for infinite scroll mode. Previously, sorting would reset to showing only PageSize items. Now it keeps all loaded items.

Recommendation: Add a test case to verify:

- [ ] Verify infinite scroll + sort keeps loaded items (doesn't reset to PageSize)

Potential Bugs

⚠️ Race Condition in Sort Action

The Sort handler uses time.Now().UnixNano() for timing checks without synchronization. If two rapid sort requests arrive concurrently (unlikely but possible in WebSocket scenarios), both could pass the check and update LastSortTime incorrectly.

Impact: Low probability in practice since LiveTemplate's handler is typically request-scoped.

Recommendation: Document that state mutations are expected to be serial per session.

⚠️ Client-Side Script Timing

The inline script at the bottom of the body might miss elements if morphdom is still processing. Consider wrapping in DOMContentLoaded:

document.addEventListener('DOMContentLoaded', function() {
  syncSelectValues();
  // ... rest of observer setup
});

Performance Considerations

✅ Generally Good

1. Minimal overhead: The timing check is O(1) and very fast
2. Early exit: Returns immediately if spurious event detected
3. Transient fields: Properly marked to avoid JSON serialization overhead

⚠️ Minor Optimizations

Repeated querySelectorAll: On every mutation, querySelectorAll rescans the DOM. Consider caching:

var selects = document.querySelectorAll('select[data-expected-value]');
function syncSelectValues() {
  selects.forEach(function(select) {
    var expected = select.getAttribute('data-expected-value');
    if (select.value !== expected) {
      select.value = expected;
    }
  });
}

Security Concerns

✅ No Security Issues Found

• No XSS vulnerabilities introduced
• No SQL injection risks (uses existing parameterized queries)
• No authentication/authorization changes
• data-expected-value is server-controlled, safe from injection

Test Coverage

⚠️ Incomplete Test Plan

Missing automated tests:

1. No test for the morphdom sync fix (manual testing only)
2. No test for the 500ms timing window behavior
3. No test for infinite scroll + sort interaction
4. The golden test file is updated but no specific test validates the new behavior

Recommendations:

1. Add a WebSocket test that simulates rapid sort changes to verify the 500ms debouncing
2. Add E2E test using chromedp to verify select dropdown persistence
3. Document manual testing results for the unchecked items in PR description

Architecture & Design

✅ Solid Approach

The dual-protection strategy is clever:

1. Server-side: Prevents spurious events from causing state changes
2. Client-side: Ensures DOM stays in sync even if morphdom misses updates

This defense-in-depth approach is good engineering.

🤔 Alternative Approaches (for future consideration)

morphdom callbacks might be cleaner:

morphdom(oldNode, newNode, {
  onBeforeElUpdated: function(fromEl, toEl) {
    if (fromEl.tagName === 'SELECT' || fromEl.tagName === 'INPUT') {
      // Preserve the value
      return true;
    }
  }
});

Not a criticism of the current approach—just an alternative to consider.

Sort Simplification Logic

✅ Clean Implementation

[[- range $i, $f := .Fields]]
[[- if and (eq $i 0) (eq $f.GoType "string")]]

Simple and effective. Correctly handles edge cases (no string fields).

🤔 Edge Case

User expectations: Is the "first field" always the most relevant for sorting? For most cases (name, title being first), yes. Might be surprising if first field is something like "sku" instead of "name", but this is a reasonable default.

Summary & Recommendations

✅ Approve with Suggestions

This PR solves real UX problems with a reasonable approach. The code is generally well-written and consistent.

Before merging:

1. ✅ Address or document the 500ms threshold choice
2. ✅ Complete manual testing checklist in PR description
3. ⚠️ Consider adding automated tests for the new behavior (nice-to-have)
4. ⚠️ Consider scoping MutationObserver more narrowly (performance optimization)
5. ⚠️ Document infinite scroll + sort behavior change in test plan

Priority fixes: None critical, but documenting the timing threshold would improve maintainability

Overall Assessment

• Code Quality: 8/10 - Clean, consistent, well-documented
• Test Coverage: 6/10 - Manual testing planned, but limited automated coverage
• Performance: 8/10 - Minimal overhead, minor optimization opportunities
• Security: 10/10 - No concerns
• Architecture: 9/10 - Good design, addresses root cause appropriately

Great work on identifying and fixing these UX issues! The morphdom interaction is tricky, and this solution handles it pragmatically. 🚀

Pull Request Review: Form Element Sync and Sort Options Simplification

Summary

This PR addresses two UX issues in generated LiveTemplate applications: morphdom-related form element synchronization problems and overly verbose sort dropdown options. The approach combines client-side value syncing with server-side reversion protection.

Code Quality ✅

Strengths

1. Well-documented workaround: The inline comments clearly explain the morphdom issue and the 500ms heuristic
2. Consistent implementation: Changes are applied uniformly across all three kits (multi, single, generator)
3. Minimal invasiveness: The client-side script is isolated and self-contained
4. Updated golden files: Test fixtures are properly updated

Areas for Improvement

1. Magic Number - 500ms Threshold

Location: handler.go.tmpl:309

The 500ms threshold for detecting spurious events is hardcoded:

if input.SortBy == state.PrevSortBy && elapsedMs < 500 {

Concern: This is a heuristic that may not work reliably across:

• Slow network connections (legitimate user actions might take >500ms)
• Fast automated testing scenarios
• Different browsers with varying morphdom timing

Recommendation: Consider making this configurable or add a comment explaining the rationale for 500ms specifically.

2. Server-Side State Bloat

Location: handler.go.tmpl:88-90

Adding PrevSortBy and LastSortTime to every generated resource state:

PrevSortBy   string  `json:"prev_sort_by" lvt:"transient"`
LastSortTime int64   `json:"last_sort_time" lvt:"transient"`

Concern: This workaround pollutes the state structure for all generated resources. While marked as transient, this is addressing a client-side issue with server-side state.

Recommendation: Consider if there's a cleaner client-side-only solution, such as:

• Debouncing the change event
• Tracking the last programmatic update timestamp in the DOM
• Using a data attribute to mark server-initiated updates

Potential Bugs 🐛

1. Race Condition in Time-Based Check

Location: handler.go.tmpl:283-288

if state.LastSortTime > 0 {
    elapsed := now - state.LastSortTime
    elapsedMs := elapsed / 1_000_000
    if input.SortBy == state.PrevSortBy && elapsedMs < 500 {
        return state, nil
    }
}

Issue: If a user rapidly:

1. Changes sort to "title_asc" (sets LastSortTime=T1)
2. Immediately changes back to "" within 500ms
3. This would be incorrectly ignored as a spurious event

Impact: Medium - could prevent legitimate rapid sort changes

Recommendation: Also check that we're reverting to the previous value, not just any duplicate value:

if input.SortBy == state.PrevSortBy && input.SortBy != state.SortBy && elapsedMs < 500 {

2. Client-Side Observer Performance

Location: template.tmpl.tmpl:354-357

var observer = new MutationObserver(function(mutations) {
  syncFormValues();
});
observer.observe(document.body, { attributes: true, subtree: true, attributeFilter: ['data-expected-value'] });

Issue: This observes the entire document body with subtree: true, which could fire frequently on pages with many DOM updates.

Impact: Low to Medium - may cause performance issues on complex pages with many form elements

Recommendation:

• Limit observation to specific containers (e.g., data-lvt-id elements)
• Or use event delegation instead of MutationObserver
• Consider disconnecting the observer after initial sync if data-expected-value attributes don't change frequently

Performance Considerations ⚡

1. Redundant Server Calls Prevented ✅

The time-based check successfully prevents spurious server round-trips, which is good for performance.

2. Client-Side Overhead

• MutationObserver: Runs on every attribute change in the entire body
• querySelectorAll: Runs on every mutation that matches the filter

Recommendation: Profile this on a page with many form elements to ensure it doesn't cause jank.

3. Template Range Iteration Change

Location: sort.tmpl:15-18

[[- range ,  := .Fields]]
[[- if and (eq  0) (eq .GoType "string")]]

Impact: Positive - reduces template size and rendering time by only showing relevant sort options.

Security Concerns 🔒

No Issues Found ✅

• No XSS vectors introduced
• No SQL injection risks
• Server-side validation remains intact
• Client-side script is properly isolated in IIFE

Test Coverage 📋

Current Status

• ✅ Golden files updated (resource_handler.go.golden)
• ❌ No automated tests for the morphdom sync behavior
• ❌ No tests for the 500ms reversion detection logic
• ❌ Manual testing mentioned in PR description but not automated

Recommended Tests

1. Unit Test for Sort Reversion Detection

func TestSortController_IgnoresSpuriousReversions(t *testing.T) {
    // Test that rapid duplicate sorts within 500ms are ignored
    // Test that legitimate sort changes are not ignored
    // Test edge cases (LastSortTime=0, exactly 500ms, etc.)
}

2. E2E Browser Test for Form Sync

Add to e2e/rendering_test.go:

func TestRendering_SelectPersistence(t *testing.T) {
    // 1. Select a sort option
    // 2. Verify it persists after morphdom update
    // 3. Verify it doesn't revert to previous value
}

3. Test Sort Options Filtering

func TestSortDropdown_OnlyShowsFirstStringField(t *testing.T) {
    // Verify sort options only include first string field
}

Architecture & Design 🏗️

Concerns

1. Root Cause Not Addressed

This PR treats symptoms (morphdom reverting values) rather than the root cause. The real issue is likely:

• Morphdom's form element diffing algorithm
• Missing onBeforeElUpdated callback configuration
• State synchronization timing

Question: Has the LiveTemplate client library's morphdom configuration been investigated? The library might need a fix rather than generated code.

2. Dual-Layer Defense

The PR implements both:

• Client-side value syncing (MutationObserver)
• Server-side reversion detection (time-based)

Analysis: This is defense-in-depth, but suggests uncertainty about which layer solves the problem. Consider:

• Testing each fix independently
• Documenting which fix handles which scenario
• Potentially removing one if redundant

3. Scalability

Adding state fields to every generated resource for this specific issue doesn't scale well if other similar issues arise.

Recommendation: Consider a more general solution:

• Framework-level form state management
• Morphdom configuration at the client library level
• Generic "last action timestamp" in base state

Sort Options Simplification ✅

This Change is Good

[[- range ,  := .Fields]]
[[- if and (eq  0) (eq .GoType "string")]]

Benefits:

• Reduces cognitive load (fewer irrelevant options)
• Cleaner UI for resources with many string fields
• Focuses on the primary display field

Edge Case: What if the first field isn't the most relevant for sorting? Consider:

• Using a tagged field (e.g., lvt:"primary")
• Or a naming convention (fields named "title", "name" get priority)

Documentation 📚

Missing Documentation

1. No upgrade guide: Existing apps won't get the fix automatically
2. No comment in generated code: Users won't understand the morphdom workaround
3. No architecture decision record: Future maintainers won't know why this approach was chosen

Recommendations

1. Add inline comments in generated templates explaining the morphdom issue
2. Document the 500ms threshold rationale
3. Add to CHANGELOG.md with upgrade instructions
4. Consider a migration guide for existing apps

Recommendations Summary

Critical (Fix Before Merge)

1. ❗ Fix the race condition in sort reversion detection (input.SortBy != state.SortBy check)
2. ❗ Add unit tests for the sort reversion logic
3. ❗ Document the 500ms threshold rationale

High Priority (Should Address)

1. 🔴 Investigate if client library morphdom config can fix this instead
2. 🔴 Add E2E browser tests for form value persistence
3. 🔴 Optimize MutationObserver scope to reduce performance impact

Medium Priority (Consider)

1. 🟡 Make the 500ms threshold configurable
2. 🟡 Add architecture decision record
3. 🟡 Provide upgrade path for existing generated apps

Nice to Have

1. 🟢 Add field tagging for sort priority instead of index-based
2. 🟢 Performance profiling on complex pages
3. 🟢 Investigate morphdom onBeforeElUpdated callbacks

Verdict

Approve with Changes Required ⚠️

This PR addresses real UX issues and the implementation is generally sound, but:

1. The race condition bug should be fixed
2. Test coverage is insufficient for such a critical behavior change
3. The architecture decision (client-side observation + server-side heuristics) needs validation

The sort simplification is excellent and ready to merge.

Estimated Risk: Medium

• User-facing behavior change
• No automated test coverage
• Time-based heuristics can be fragile
• Performance impact of MutationObserver unknown

Great work identifying and fixing these UX issues! The dual-approach defense shows good engineering judgment, but let's validate it with tests and address the race condition before merging.

cc: @adnaan

Code Review - PR #41

This PR addresses two UX improvements for generated LiveTemplate apps. Overall, the changes are well-structured and solve real morphdom sync issues. Below are my findings:

✅ Strengths

1. Well-identified problem: The morphdom DOM diffing causing select dropdowns to revert is a legitimate UX issue
2. Dual approach: Both client-side (syncFormValues) and server-side (PrevSortBy/LastSortTime) protection is thorough
3. Good UX improvement: Simplifying sort options to only show the first string field reduces UI clutter
4. Consistent application: Changes applied across all kit variants (single, multi, system)
5. Template variable fix: Correctly addresses the range variable scoping issue (using dollar-f.Name instead of .Name)

🔍 Code Quality & Best Practices

Template Changes

File: internal/kits/system/multi/components/sort.tmpl:15-19

The range variable fix is correct - properly uses dollar-f.Name for the field variable and dollar-.SortBy for parent scope

Handler Logic

File: internal/kits/system/multi/templates/resource/handler.go.tmpl:294-312

The debounce logic looks solid:

• 500ms window is reasonable for detecting spurious events
• Proper overflow-safe time calculation

⚠️ Potential Issues & Concerns

1. Search Input Missing data-expected-value (Medium Priority)

The PR description mentions fixing the search input clear button, but the search input does not have the data-expected-value attribute. Add data-expected-value attribute to the search input for consistency with the stated goal.

2. JavaScript Performance - MutationObserver Inefficiency (Low Priority)

The observer fires on every attribute change in the subtree, even when data-expected-value does not change on form elements. Consider checking if mutations actually affect the elements we care about, or use a more targeted approach.

3. Race Condition Edge Case (Low Priority)

If a user rapidly clicks between sort options A to B to A within 500ms, the second change would incorrectly be ignored. Consider tracking both current and previous values differently, or reset the protection window after successful changes.

4. Missing Tests (High Priority)

There are no automated tests for:

• The morphdom sync behavior
• The server-side debounce logic
• The simplified sort options generation
• The syncFormValues JavaScript function

Add E2E tests to verify:

1. Select dropdowns maintain their value after morphdom updates
2. Search input clears properly
3. Rapid sort changes do not get incorrectly debounced
4. Only first string field appears in sort options

🔒 Security Concerns

None identified. The changes do not introduce XSS risks or other security vulnerabilities. The data-expected-value attributes are properly escaped in the templates.

⚡ Performance Considerations

1. MutationObserver overhead: Could be optimized but unlikely to cause issues in practice
2. Sort action optimization: The change to NOT reset LoadedCount when sorting is a good performance improvement

🧪 Test Coverage

Current status: ⚠️ Insufficient

The test plan in the PR description has unchecked items. Convert these manual tests into E2E tests in the e2e/ directory before merging.

📋 Recommendations Summary

Before Merging:

1. Add data-expected-value to search input to match PR description
2. Add E2E tests for the new behavior
3. Consider the race condition in rapid sort changes

Future Improvements:

1. Optimize MutationObserver to reduce unnecessary callbacks
2. Consider extracting the morphdom sync fix into the core client library

✅ Approval Status

LGTM with minor changes requested. The core functionality is solid, but please address:

1. Search input missing attribute
2. Test coverage

Great work on identifying and solving this UX issue! The dual client/server approach shows good defensive programming.

🤖 Generated with Claude Code

PR Review: Fix Form Element Sync and Simplify Sort Options

Summary

This PR addresses two UX issues in generated LiveTemplate apps:

1. Morphdom form element sync issues causing dropdowns and inputs to revert
2. Simplifying sort dropdown options to show only relevant fields

Code Quality ✅

Strengths:

• Well-structured solution with both client-side and server-side fixes
• Clear, descriptive comments explaining the reasoning
• Consistent changes across all kit variants (multi, single, simple)
• Updated golden files to match changes

Areas for Consideration:

1. Magic Number - 500ms Timeout

Location: handler.go.tmpl:310

if input.SortBy == state.PrevSortBy && elapsedMs < 500 {

Issue: The 500ms threshold is hardcoded. Consider:

• Making it a constant with a descriptive name: const morphdomRaceWindowMs = 500
• Adding a comment explaining why 500ms was chosen
• Consider if this should be configurable

2. Potential Race Condition

Location: handler.go.tmpl:302-318

The server-side deduplication logic assumes sequential processing. If multiple sort requests arrive concurrently (e.g., user rapidly clicking), the timestamp-based detection might not work reliably since:

• state.LastSortTime could be updated by concurrent requests
• state.PrevSortBy might not reflect the true previous value

Recommendation: Document that this assumes single-threaded request processing per session, or add proper synchronization if concurrent requests are possible.

3. JavaScript Observer Performance

Location: layout.tmpl:156-180

observer.observe(document.body, { attributes: true, subtree: true, attributeFilter: ['data-expected-value'] });

Issue: Observing the entire document.body with subtree: true could have performance implications on large DOMs.

Recommendation:

• Consider observing only the form container instead of document.body
• Add a comment about performance considerations
• Consider debouncing the syncFormValues callback if performance becomes an issue

Potential Bugs ⚠️

1. Input Element Sync Not Complete

Location: toolbar.tmpl and sort.tmpl

The templates add data-expected-value to select elements but the search input (if it exists) might not have this attribute. The PR description mentions fixing "search input not clearing when X button is clicked" but I don't see data-expected-value added to search inputs in the diff.

Verify: Are there search input elements that also need the data-expected-value attribute?

2. Template Variable Scope Issue - Resolved ✅

Location: sort.tmpl:50-52

Good catch in commit 2473ab7! The fix to use $.SortBy instead of .SortBy within the range is correct. However, there's an inconsistency:

<select ... data-expected-value="{{.SortBy}}">  // Line 44 - uses .SortBy
  ...
  <option value="[[$f.Name]]_asc" {{if eq $.SortBy "[[$f.Name]]_asc"}}selected{{end}}>  // Uses $.SortBy

The select's data-expected-value uses {{.SortBy}} (runtime Go template), while the option comparison uses $.SortBy (code generation template). This is actually correct since they're different template evaluation contexts, but it's worth verifying this works as expected.

Performance Considerations 📊

1. Client-Side Performance

• MutationObserver: Runs on every attribute change in the DOM tree. On large forms or frequent updates, this could add overhead.
• QuerySelectorAll: Called on every mutation. Consider caching selectors or using event delegation.

Recommendation:

// Consider debouncing
let syncTimeout;
var observer = new MutationObserver(function(mutations) {
  clearTimeout(syncTimeout);
  syncTimeout = setTimeout(syncFormValues, 10);
});

2. Server-Side Performance

The server-side changes are minimal and should have negligible performance impact. The timestamp comparison is efficient.

Security Concerns 🔒

1. No XSS Issues ✅

The data-expected-value attribute values come from server-side template rendering and are properly escaped by Go's template engine.

2. Client-Side Tampering

Minor Issue: A malicious user could modify data-expected-value attributes in the browser, but this is mitigated by:

• Server-side validation still occurs
• The sync only affects UI state, not data integrity

Status: Not a real concern, but worth noting.

Test Coverage 📝

Missing:

• No automated tests for the morphdom sync behavior
• No tests for the server-side deduplication logic
• No tests verifying only the first string field appears in sort options

Recommendations:

1. Add unit tests for the Sort handler's deduplication logic
2. Add E2E tests simulating the morphdom race condition
3. Add golden file verification tests (already done ✅)
4. Test the range iteration change to ensure correct field filtering

Manual Testing Required:
As noted in the PR description:

• Verify sort dropdown only shows first string field options
• Verify select dropdown persists value after selection
• Verify search clear button clears the input

Architecture & Design 🏗️

1. Dual-Layer Defense ✅

The approach of fixing the issue at both client (syncFormValues) and server (deduplication) levels is good defensive programming. Even if one layer fails, the other provides protection.

2. Simplification is Good ✅

Reducing sort options to only the first string field is a sensible UX improvement. Users can always customize if they need more options.

3. Template Inheritance

The fix is applied consistently across:

• Template generators
• Kit components (multi, single, simple)
• Golden test files

This consistency is excellent.

Breaking Changes 🚨

Minor Breaking Change: Users who relied on sorting by all string fields will now only see the first string field in the dropdown. While this is a UX improvement, it changes behavior.

Mitigation: This only affects newly generated resources. Existing apps won't be affected unless regenerated.

Recommendations 📋

High Priority

1. Add a constant for the 500ms timeout value
2. Add tests for the Sort deduplication logic
3. Verify search input has data-expected-value if needed

Medium Priority

4. Document concurrent request behavior in comments
5. Consider debouncing the MutationObserver callback
6. Add E2E tests for the morphdom race condition

Low Priority

7. Consider making the timeout threshold configurable
8. Add performance benchmarks for large forms

Code Suggestions

Suggestion 1: Extract constant

// handler.go.tmpl
const (
    // morphdomEventWindowMs is the time window within which duplicate sort events
    // are considered spurious morphdom-triggered events and should be ignored
    morphdomEventWindowMs = 500
)

// In Sort method:
if input.SortBy == state.PrevSortBy && elapsedMs < morphdomEventWindowMs {

Suggestion 2: Add debouncing

// layout.tmpl
<script>
  (function() {
    let syncTimeout;
    function syncFormValues() {
      document.querySelectorAll('select[data-expected-value]').forEach(function(select) {
        var expected = select.getAttribute('data-expected-value');
        if (select.value !== expected) {
          select.value = expected;
        }
      });
      document.querySelectorAll('input[data-expected-value]').forEach(function(input) {
        var expected = input.getAttribute('data-expected-value');
        if (input.value !== expected) {
          input.value = expected;
        }
      });
    }
    syncFormValues();
    var observer = new MutationObserver(function(mutations) {
      clearTimeout(syncTimeout);
      syncTimeout = setTimeout(syncFormValues, 10);
    });
    observer.observe(document.body, { attributes: true, subtree: true, attributeFilter: ['data-expected-value'] });
  })();
</script>

Conclusion

This is a solid PR that addresses real UX issues with a thoughtful dual-layer approach. The code quality is good, with clear comments and consistent implementation across all kits.

Status: Approve with minor suggestions ✅

The suggestions above are mostly optimizations and best practices. The core functionality is sound. I'd recommend addressing the high-priority items (constant extraction and testing) before merging, but the medium and low priority items can be addressed in follow-up PRs.

Great work on identifying and fixing these morphdom edge cases!

Review generated with Claude Code