Updated hooks

[ukutaht]

Resumed from #15

Adds tests for updated() behaviour.

[claude]

PR Review: Updated hooks

Summary

This PR adds updated() lifecycle hooks to the dropdown and modal components, along with comprehensive Wallaby tests to verify components remain functional after LiveView re-renders.

---

✅ What's Good

1. Well-structured approach

• The initialize() method properly calls cleanup() first, preventing memory leaks from duplicate event listeners
• Tests cover key scenarios: basic functionality, keyboard navigation, and ARIA relationships

2. Comprehensive test coverage

• Tests verify that components remain functional after LiveView triggers re-renders
• ARIA relationship verification ensures accessibility is maintained
• Good use of visit_fixture helper for hook readiness

3. Follows existing patterns

• Implementation mirrors the existing reconnected() hook pattern
• Test structure follows the project's Wallaby testing conventions

---

⚠️ Issues and Concerns

1. Missing updated() hook in combobox

The combobox hook (assets/js/hooks/combobox.js) has an updated() method (lines 144-153) but it only does partial reinitialization:

updated() {
  this.ensureOptionIds()
  this.positionOptions()
  // ... focus handling
}

For consistency, should the combobox also call this.initialize() in its updated() hook? If not, this should be documented to explain why combobox requires different handling (e.g., due to async options streaming).

2. Performance consideration with initialize() on every update

Calling initialize() on every LiveView update could be expensive for components that update frequently. The initialize() method:

• Removes all existing event listeners
• Re-queries all DOM elements
• Re-attaches all event listeners
• Sets up ARIA relationships

Consider:

• Is there a way to detect if reinitialization is actually needed? (e.g., checking if key elements still exist in the DOM)
• Would it be more efficient to only reinitialize specific parts that may have changed?

3. Potential race condition in tests

In dropdown_rerender_trigger_test.exs (line 21), after clicking the update button:

|> click(@update_button)
|> assert_has(Query.css("#dropdown [aria-haspopup=menu]", text: "Updated Trigger"))

This looks correct, but there's a potential timing issue: the test asserts the text changed, but doesn't explicitly wait for the updated() hook to complete before clicking the dropdown button. The data-prima-ready attribute is set, but it's set synchronously in initialize() before event listeners are fully bound. Consider if there could be a race between the assertion and the hook completing.

4. Modal re-render test could be more thorough

The modal test (modal_rerender_title_test.exs) updates the title while the modal is closed. Consider adding a test case where:

• The modal is open when the title is re-rendered
• This would test that the modal remains interactive during live updates

---

🔍 Code Quality Notes

1. Fixture file structure

• Fixtures are well-organized and follow the project pattern
• Good separation of concerns between the fixture and test files

2. Minor style suggestion

In dropdown_rerender_trigger_fixture.html.heex (lines 19-21), the #outside-area div has a comment about being invisible but has no actual purpose in the tests (no tests use it for click-outside testing). Consider removing if unused:

<div id="outside-area" style="height: 50px; width: 100%;">
  <!-- Invisible area for click-outside testing -->
</div>

---

🛡️ Security & Best Practices

• No security concerns identified
• No backwards-compatibility issues
• Proper cleanup in initialize() prevents memory leaks

---

📝 Recommendations

1. Document why combobox differs: Add a comment explaining why combobox's updated() doesn't call initialize(), or consider making it consistent

2. Consider partial reinitialization: For performance-critical scenarios, investigate whether full reinitialization is always necessary

3. Add test for modal open during re-render: Strengthen the modal test coverage

4. Remove unused fixture code: Clean up the #outside-area div if not used

---

Overall Assessment

LGTM with minor suggestions ✅

The core implementation is solid and addresses an important issue - components breaking after LiveView re-renders. The tests are comprehensive and follow project conventions. The suggestions above are improvements rather than blockers.

Nice work on adding proper lifecycle handling! 🎉

[claude]

PR Review: Updated hooks

Summary

This PR implements updated() lifecycle hooks for dropdown and modal components to ensure they remain functional after LiveView re-renders. The changes are well-tested with Wallaby browser tests.

---

✅ Code Quality & Best Practices

Strengths:

• Clean, consistent pattern following the existing mounted() → initialize() approach
• Tests follow project conventions (visit_fixture, proper selectors, good use of assert_has)
• Fixture structure correctly follows the established pattern

Minor Suggestions:

1. Consistency with combobox.js: The combobox already has an updated() hook (lines 144-154), but its implementation is different—it performs targeted updates rather than full reinitialization:

   updated() {
     this.ensureOptionIds()
     this.positionOptions()
     // ... focused option preservation
   }

   The dropdown/modal approach (this.initialize()) is heavier as it tears down and rebuilds all event listeners. This is safe but may be worth considering if there are performance concerns with frequently re-rendering components. For most use cases, this should be fine.

2. Comment the "why": Consider adding a brief comment explaining why updated() is needed—e.g., "Re-initialize when LiveView patches the DOM to restore event listeners on replaced elements."

---

🔍 Potential Issues

1. Modal fixture HTML appears to be truncated:
The modal_rerender_title_fixture.html.heex file shows content that ends abruptly at line 34. Please verify the file is complete and properly closes all tags.

2. Event listener duplication safety:
The initialize() method correctly calls this.cleanup() first, which prevents event listener duplication. This is good! ✅

3. Async modal consideration:
The modal hook's updated() will call initialize() which runs setupElements() and setupEventListeners(). For async modals (where panel may not exist yet), ensure this doesn't cause issues when the panel is mounted later. Looking at the code, setupEventListeners() at line 54-58 handles this with a null check, so this should be fine.

---

⚡ Performance Considerations

The full reinitialization approach in updated() includes:

• Cleanup of all existing listeners
• Re-querying DOM elements
• Re-setting up all event listeners
• Re-setting ARIA relationships

This happens on every LiveView patch to the component. For most use cases this is acceptable, but if you anticipate high-frequency updates (e.g., real-time data in dropdown items), consider a more targeted update like combobox does.

Current approach is fine for:

• Occasional trigger label changes
• Modal title updates
• Standard CRUD operations

---

🔒 Security Concerns

No security concerns identified. The changes don't introduce any new attack vectors.

---

🧪 Test Coverage

Excellent test coverage! The tests verify:

• ✅ Component functionality after re-render (click to open/close)
• ✅ Keyboard navigation after re-render
• ✅ ARIA attributes maintained after re-render
• ✅ Event listeners restored properly

Suggestions for additional coverage (optional):

1. Consider testing that click-outside-to-close still works after re-render for dropdowns
2. Consider testing escape key behavior for modal after re-render (already partially covered)

---

📋 Overall Assessment

Recommendation: Approve with minor suggestions

This is a solid PR that addresses an important issue—ensuring component functionality persists after LiveView DOM patches. The approach is safe and correct, the tests are thorough, and the code follows project conventions.

The only action item is to verify the modal fixture file is complete (check for proper closing tags).

---

Questions for Discussion

1. Should this pattern be documented somewhere for future component authors?
2. Are there plans to add similar updated() hooks to the combobox's initialization pattern for consistency, or is the current targeted approach preferred there?

[claude]

PR Review: Updated Hooks

Summary

This PR adds updated() lifecycle hooks to the dropdown and modal components to ensure they reinitialize when LiveView re-renders their content. This is a necessary fix for maintaining component functionality after dynamic updates.

---

✅ What's Good

1. Core Implementation

• The updated() hooks correctly call initialize() which handles proper cleanup before re-initialization (both hooks call this.cleanup() at the start of initialize())
• This prevents event listener accumulation and memory leaks
• The pattern matches the existing reconnected() lifecycle hook approach

2. Test Coverage

• Comprehensive Wallaby tests covering the key scenarios:
  • Dropdown remains functional after trigger re-render
  • Keyboard navigation works after re-render
  • ARIA attributes are correctly maintained
  • Modal remains functional after title re-render
• Tests follow the project's established patterns (visit_fixture, Query helpers, etc.)

3. Fixture Implementation

• Fixtures properly follow the CLAUDE.md guidance for creating fixture tests
• Router entries, template entries, and LiveView handlers are all correctly added
• Clean separation between update buttons and component under test

---

⚠️ Potential Issues & Suggestions

1. Combobox Consistency
The combobox hook (assets/js/hooks/combobox.js) already has a custom updated() implementation (lines 144-154) that handles specific state preservation like focusedOptionBeforeUpdate. This is fine and working correctly, but it's worth noting the three components now have slightly different updated() behaviors:

• Dropdown: Full re-initialization
• Modal: Full re-initialization
• Combobox: Partial update (IDs, positioning, focus restoration, selection sync)

This inconsistency is acceptable since combobox has more complex state to preserve during updates, but it should be documented.

2. Modal updated() May Cause Issues with Open Modals
Looking at the modal's initialize() method:

initialize() {
  this.cleanup()
  this.setupElements()
  this.setupEventListeners()
  this.checkInitialShow()
  this.el.setAttribute('data-prima-ready', 'true')
}

The checkInitialShow() call at line 76-79 dispatches prima:modal:open if data-prima-show is present. If a modal is already open when a re-render occurs, this could cause:

• Duplicate handleModalOpen() calls
• storeFocusedElement() could overwrite the correctly stored element with the current focus (inside the modal)
• Double preventBodyScroll() calls (though this is idempotent)

Consider adding a guard:

checkInitialShow() {
  // Don't re-trigger open if modal is already visible
  if (Object.hasOwn(this.modalEl.dataset, 'primaShow') &&
      this.modalEl.getAttribute('aria-hidden') === 'true') {
    this.modalEl.dispatchEvent(new Event('prima:modal:open'))
  }
}

3. Test Coverage Gap - Open State Re-render
The tests verify re-render behavior when components are closed, but don't test re-renders while the component is in an open state. Consider adding:

feature "dropdown remains functional when re-rendered while open", %{session: session} do
  session
  |> visit_fixture("/fixtures/dropdown-rerender-trigger", "#dropdown")
  |> click(@dropdown_button)
  |> assert_has(@dropdown_menu |> Query.visible(true))
  # Trigger update while dropdown is open
  |> click(@update_button)
  |> assert_has(Query.css("#dropdown [aria-haspopup=menu]", text: "Updated Trigger"))
  # Verify dropdown is still open and functional
  |> assert_has(@dropdown_menu |> Query.visible(true))
  |> send_keys([:down_arrow])
  |> assert_has(Query.css("#dropdown [role=menuitem]:first-child[data-focus]"))
end

---

🔍 Performance Consideration

Both hooks call this.cleanup() followed by full re-initialization on every updated() call. For dropdown, this includes:

• Removing and re-adding 7 event listeners
• Re-querying DOM elements
• Re-setting ARIA relationships

This is acceptable for occasional updates, but if these components are inside rapidly-updating containers (like a form with real-time validation), it could cause performance issues. The current implementation is correct and safe; this is just something to be aware of for future optimization if needed.

---

📋 Verdict

Approve with minor suggestions. The implementation correctly solves the re-render problem and has good test coverage. The suggestions above are improvements that could be addressed in follow-up PRs:

1. P2: Add test for re-render while component is open
2. P3: Add guard in checkInitialShow() to prevent duplicate open events
3. P4: Document the different updated() behaviors across components

The code is well-structured, follows project conventions, and the test fixtures are properly created according to CLAUDE.md guidelines.

[claude]

PR Review: Updated hooks

Summary

This PR adds updated() lifecycle hooks to the Dropdown and Modal components to re-initialize DOM elements and event listeners when LiveView re-renders component content. The implementation includes comprehensive Wallaby tests demonstrating the fix for a real problem.

---

✅ Strengths

1. Solves a Real Problem
The issue being addressed is legitimate: when LiveView patches the DOM (e.g., updating trigger text or modal title), event listeners attached to child elements can be lost or become stale. Calling initialize() in updated() properly restores functionality.

2. Excellent Test Coverage

• Tests cover both click interactions and keyboard navigation after re-render
• ARIA relationship integrity is verified after updates
• The modal test includes a critical case: re-rendering while the modal is open (modal remains open when re-rendered while open)
• Tests follow established patterns in the codebase and use visit_fixture helper correctly

3. Clean Implementation

• The hooks already have proper cleanup() logic that removes event listeners before re-attaching
• The pattern is consistent with the existing reconnected() behavior
• No memory leaks since initialize() calls cleanup() first

---

⚠️ Concerns and Suggestions

1. Missing updated() Hook for Combobox

The Combobox hook (assets/js/hooks/combobox.js) has a different updated() implementation that doesn't call this.initialize():

updated() {
  this.ensureOptionIds()
  this.positionOptions()
  // ... focus management
}

This inconsistency might cause similar re-render issues for Combobox. Consider:

• Either adding a test to verify Combobox doesn't need the full initialize() treatment
• Or updating Combobox to follow the same pattern for consistency

2. Potential Performance Concern

Calling initialize() on every updated() callback means:

• All event listeners are removed and re-added on every LiveView patch
• ARIA relationships are recalculated
• For Dropdown: auto-positioning cleanup/setup may occur

While the cleanup/setup pattern is safe, frequent re-renders (e.g., real-time updates) could cause noticeable overhead. Consider:

updated() {
  // Only re-initialize if critical elements changed
  const currentButton = this.el.querySelector(SELECTORS.BUTTON)
  if (currentButton !== this.refs.button) {
    this.initialize()
  }
}

This optimization would avoid unnecessary work when only non-structural content changes. However, the current approach is simpler and more maintainable for now.

3. Modal State Preservation During Update

The modal's updated() calls initialize() which in turn calls checkInitialShow():

checkInitialShow() {
  if (Object.hasOwn(this.modalEl.dataset, 'primaShow')) {
    this.modalEl.dispatchEvent(new Event('prima:modal:open'))
  }
}

The test "modal remains open when re-rendered while open" passes, which suggests this isn't causing issues. However, it would be good to verify that this doesn't inadvertently trigger focus management or other side effects when the modal is already open.

4. Minor: Test Naming Consistency

The test file names include the scenario being tested (dropdown_rerender_trigger_test.exs), which is clear and descriptive.

---

🔍 Code Quality

Elixir/Phoenix Code

• Event handlers follow existing patterns
• Fixture files are properly structured
• Routes are correctly added

JavaScript Code

• Simple 4-line additions following established patterns
• Properly leverages existing initialize() and cleanup() methods

Tests

• Well-structured with clear assertions
• Good use of module attributes for selectors
• Appropriate use of Query.visible() and data attribute checks

---

📝 Suggestions for Improvement

1. Add a brief code comment explaining why updated() is needed:

   updated() {
     // Re-initialize when LiveView patches DOM to restore event listeners
     this.initialize()
   },

2. Consider testing edge cases:

   • Rapid successive updates (e.g., debounced input triggering multiple patches)
   • Updates while dropdown menu is open (currently only tested for modal)

3. Document the behavior in component docs or CLAUDE.md so future developers understand when updated() hooks are necessary

---

✅ Verdict

Approve with minor suggestions. The PR correctly addresses a real issue with LiveView component re-rendering, and the test coverage is thorough. The implementation is safe due to proper cleanup handling.

The main consideration is whether to optimize for performance (checking if re-initialization is actually needed) vs. simplicity (always re-initializing). The current simple approach is reasonable for a v1 and can be optimized later if performance issues arise.