Skip to content

Refactor portal hooks#1981

Open
jacbn wants to merge 7 commits intomainfrom
improvement/refactor-portals
Open

Refactor portal hooks#1981
jacbn wants to merge 7 commits intomainfrom
improvement/refactor-portals

Conversation

@jacbn
Copy link
Contributor

@jacbn jacbn commented Feb 11, 2026

Refactors the main React portal wrapper to use better use hooks and dependencies in order to:

  • Prevent portal code running multiple times unnecessarily;
  • Better link the two halves of our portal hooks (modifying the DOM / placing portals) such that portals always link to an extant part of the DOM. This, I believe, forcibly prevents race conditions between portals appearing and the DOM they were meant to point to updating, meaning all the past issues regarding “x portal component not appearing”, “empty div in DOM with no content” should be fixed!!

@codecov
Copy link

codecov bot commented Feb 11, 2026

Codecov Report

❌ Patch coverage is 46.77419% with 99 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.29%. Comparing base (fcb545a) to head (4c46462).

Files with missing lines Patch % Lines
...mponents/elements/markup/portals/GlossaryTerms.tsx 34.09% 28 Missing and 1 partial ⚠️
...elements/markup/portals/renderClozeDropRegions.tsx 23.68% 26 Missing and 3 partials ⚠️
.../app/components/elements/markup/portals/Tables.tsx 44.73% 19 Missing and 2 partials ⚠️
.../elements/markup/portals/renderInlineEntryZone.tsx 36.00% 14 Missing and 2 partials ⚠️
src/app/components/elements/markup/index.tsx 82.60% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1981      +/-   ##
==========================================
+ Coverage   43.17%   43.29%   +0.11%     
==========================================
  Files         575      575              
  Lines       24620    24688      +68     
  Branches     8128     8155      +27     
==========================================
+ Hits        10630    10688      +58     
- Misses      13322    13329       +7     
- Partials      668      671       +3     

☔ 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.

Copy link
Contributor

@sjd210 sjd210 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Took me some time to figure out how portals actually work in the codebase in the first place, even with the new comments. In fact, we could perhaps do with even more comments, but inside the individual portal hook implementations like Table.tsx has. That's outside of this PR's scope however, so I'll make a separate card.

As for the main changes, I can see how the race condition would come up and it makes sense to me that this would fix it, BUT owing to the inconsistency of reproducing theses portal issues in the first place it's hard to be 100% sure that this was the only/entire problem. I've tested as best I can across each of the different portal types and seen no issues, and I don't see how this could possibly make things worse, so I think at this point we should just merge and wait for issues to come in again or not (🤞)

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