Skip to content

Conversation

@ukutaht
Copy link
Contributor

@ukutaht ukutaht commented Nov 25, 2025

Summary

This PR adds backend control capabilities for opening and closing modals via push_event, enabling server-side components to control modal visibility without relying on client-side JavaScript commands.

Key Features

  • Backend Open/Close: New Prima.Modal.push_open/2 and Prima.Modal.push_close/2 functions allow backend to control modals
  • Multi-Modal Targeting: Optional id parameter enables precise control when multiple modals are present on a page
  • Backward Compatible: Empty payload continues to work (targets all modals)

Usage

Single modal (no ID needed):

def handle_event("close_modal", _params, socket) do
  {:noreply, Prima.Modal.push_close(socket)}
end

Multiple modals (specify ID):

def handle_event("open_settings", _params, socket) do
  {:noreply, Prima.Modal.push_open(socket, "settings-modal")}
end

Implementation Details

  • Modal hook listens for prima:modal:open and prima:modal:close push events
  • Hook checks payload ID and only responds if it matches (or if no ID specified)
  • Updated documentation with clear frontend vs async modal patterns
  • Comprehensive Wallaby test coverage for single and multi-modal scenarios

Files Changed

  • lib/prima/modal.ex - Added documentation and push_open/push_close functions
  • assets/js/hooks/modal.js - Implemented push_event handlers with ID targeting
  • demo/ - Added fixtures and tests demonstrating the feature

Testing

  • 26 Wallaby features + 1 test passing
  • Coverage for both single modal and multi-modal targeting scenarios
  • Tests verify modals open/close correctly from backend push_event

ukutaht and others added 13 commits November 25, 2025 13:50
Extend modal component to support closing and opening via push_event
from the backend in frontend (non-async) mode.

- Add handleEvent registration in modal hook for prima:modal:close
  and prima:modal:open events
- Integrate push event listener setup/cleanup into initialize/cleanup
  lifecycle methods
- Add fixture and Wallaby test demonstrating backend-initiated close
- Backend can now use push_event(socket, "prima:modal:close", %{})
  to close frontend modals

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Rename for clarity to distinguish between DOM event listeners
(addEventListener) and LiveView push event handlers (handleEvent).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Add test to verify modal can be opened from backend using push_event,
in addition to the existing close functionality.

- Add test for prima:modal:open push_event
- Add backend handler for open-frontend-modal event
- Add backend-open-button to fixture

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Allow backend to target specific modals via push_event payload,
enabling precise control when multiple modals are present.

- Add optional `id` field in push_event payload
- Hook checks payload and only responds if ID matches or is unspecified
- Maintains backward compatibility (empty payload targets all modals)
- Add fixture and tests demonstrating multiple modal targeting

Usage:
  push_event(socket, "prima:modal:close", %{id: "user-modal"})
  push_event(socket, "prima:modal:open", %{id: "settings-modal"})

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Consolidate frontend_modal_push_event and multiple_modals_push_event
fixtures and tests into a unified modal_push_event fixture that
demonstrates both single modal (no ID) and multi-modal (with ID)
scenarios.

- Merge two fixtures into modal_push_event_fixture.html.heex
- Merge two test files into modal_push_event_test.exs
- Update router and template to use single fixture route
- All 26 features + 1 test still pass

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@claude
Copy link

claude bot commented Nov 25, 2025

PR Review: Backend Control for Modal Open/Close

This is a solid feature addition that enables server-side control of modals via push_event. The implementation is well-thought-out and follows good practices.

✅ Strengths

  1. Excellent Documentation - The updated moduledoc in lib/prima/modal.ex clearly distinguishes between Frontend Modals and Async Modals patterns with clear examples for both single-modal and multi-modal scenarios

  2. Clean API Design - The Prima.Modal.JS namespace separation is excellent, keeping client-side and server-side APIs distinct. The optional id parameter elegantly handles both single-modal and multi-modal scenarios

  3. Robust JavaScript Implementation - Proper cleanup of push event handlers in cleanup() method (lines 85-90 in modal.js), correct ID matching logic, and good separation of DOM and push event listeners

  4. Comprehensive Test Coverage - 5 Wallaby features testing single modal, multi-modal, and independent targeting scenarios with good use of visit_fixture helper

  5. Consistent Refactoring - All existing modal usages updated to use Prima.Modal.JS.open/close (10 fixture files), maintaining backward compatibility

🔍 Issues and Concerns

1. Potential Memory Leak in Event Handler Registration ⚠️

In modal.js:27-40, setupPushEventListeners() is called on every initialize() (which happens on mounted, updated, and reconnected). The cleanup relies on this.pushEventRefs being truthy.

Recommendation: Initialize pushEventRefs as an empty array in setupPushEventListeners() before creating refs to ensure cleanup always works.

2. Missing Error Handling

Push event handlers in modal.js:29-38 don't validate payload structure. Consider adding basic validation for malformed data, though this might be overkill for an internal API.

3. Element Reference Cleanup

The PR correctly removes this.modalEl and replaces all references with this.el. The old code was incorrectly trying to find the modal element by removing -portal from the ID. This removal fixes a bug.

🎯 Minor Suggestions

  1. Consider adding @SPEC annotations to push_open/2 and push_close/2 for better type documentation
  2. CLAUDE.md update about sleep/setTimeout in tests is good but unrelated to this PR
  3. Fly.io config changes seem unrelated and should probably be in a separate PR
  4. Test coverage is solid but could add edge cases: nonexistent modal IDs, push_open on already-open modal

🔒 Security and Performance

✅ No security concerns - ID parameter uses string comparison not DOM query construction, no XSS risk

✅ Performance looks good - minimal event handlers (2 per modal) with proper cleanup

📝 Summary

This is a high-quality PR with valuable functionality, excellent documentation, and solid test coverage. Main concern is the potential event handler accumulation (item 1) which should be verified or fixed. Other issues are minor.

Recommendation: ✅ Approve with minor suggestions

Great work! The separation between Prima.Modal.JS and Prima.Modal.push_* functions is particularly elegant.

@ukutaht ukutaht merged commit 6d8d0b1 into main Nov 25, 2025
0 of 3 checks passed
@ukutaht ukutaht deleted the modal-backend-open-close branch November 25, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants