Skip to content

Conversation

@bmagyarkuti
Copy link
Contributor

@bmagyarkuti bmagyarkuti commented Aug 21, 2025

This PR outlines a suggestion for solving the following problems:

  • If you open a development console on a page with a modal (dev-mode only), there are lots of warnings printed because the objects we use to represent the modals cannot be serialized, and Redux wants us to use serializable state only. This problem affects all pages that use ActiveModal. Screenshot 2025-08-21 at 10 28 51
  • I've recently introduced ActiveModalWithState, to find a better way to have stateful modals. Before that, the only way to have stateful modals was by abandoning the current component's slots interface and passing in everything as a single body component (for an example, see NotificationModal). However, @jacbn made the observation that the solution I implemented effectively broke React's rules of hooks, by passing a custom hook as a prop to a component. If hooks should never be conditionally called, then this is also against the rules, since the component that calls the custom hook has no way of knowing that the same thing is passed every time).

This PR shows a potential resolution to both problems. Using the example of AdaTeacherOnboardingModal, in this example I show a new way for creating ActiveModals. This still makes use of the Redux state, but rather than storing the modal's component parts directly on the state, the modal is registered as a single component in ActiveModals.tsx, and the state only stores an id. Because the modal is now defined as a React component rather than a POJO, we're able to add state (as well as call other hooks) just by just relying on component composition. And because the modal itself is never pushed to the state, we no longer receive the warnings.

Any thoughts? If we chose to adopt this pattern, I think it'd be good to rewrite existing modals to also work this way. I think this can be done in a single afternoon. We can also adopt this incrementally, and the PR also shows what incremental adoption might look like.

PS: please disregard the changes to Overview.test and store.ts, these just annoyed me but are irrelevant.

PS2: It would have been nice to enforce that modalRegistry can only hold IModals, where a IModal is any component that ultimately renders a Modal. Unfortunately, React components all just have the same return type (some tsx), and that's as much the type system sees, so this is not possible.

@barna-isaac barna-isaac changed the title Active modal refactor ActiveModal refactor idea Aug 21, 2025
@codecov
Copy link

codecov bot commented Aug 21, 2025

Codecov Report

❌ Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.12%. Comparing base (2d86b66) to head (855cee6).

Files with missing lines Patch % Lines
src/test/pages/TeacherOnboardingModal.cy.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           redesign-2024    #1660      +/-   ##
=================================================
- Coverage          41.13%   41.12%   -0.01%     
=================================================
  Files                525      525              
  Lines              23204    23198       -6     
  Branches            7693     7693              
=================================================
- Hits                9544     9541       -3     
+ Misses             13032    13029       -3     
  Partials             628      628              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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