Skip to content

fix(dialog): keep DialogManagerProvider mounted on first render and stabilize manager identity#3104

Closed
MartinCupela wants to merge 2 commits intomasterfrom
fix/keep-dialog-manager-rendered
Closed

fix(dialog): keep DialogManagerProvider mounted on first render and stabilize manager identity#3104
MartinCupela wants to merge 2 commits intomasterfrom
fix/keep-dialog-manager-rendered

Conversation

@MartinCupela
Copy link
Copy Markdown
Contributor

🎯 Goal

This PR fixes an initial-render bug in DialogManagerProvider that could cause Chat children to be skipped on first render (notably affecting SSR), and also fixes manager identity mismatches that could break anchored dialog UIs (for example channel action context menus).

Problem

DialogManagerProvider previously initialized with null for id-based managers until useEffect ran.
That introduced two issues:

  1. First render could return null, so children were not rendered in SSR/initial pass.
  2. Manager lifecycle/lookup could diverge when multiple providers with the same id were involved, causing dialog open state and portal destination to use different manager instances.

Changes

  • Removed first-pass null gating behavior for DialogManagerProvider.
  • Added pending manager cache to ensure same-id providers can share a stable manager during first render.
  • Added mount ref counting to avoid removing a shared manager while other providers with the same id are still mounted.
  • Switched manager equality checks from id-based comparison to object identity (===) where needed to prevent stale instance reuse.

Tests

Added regression coverage in both branches to verify SSR behavior:

  • Assert children are included in server-rendered markup when DialogManagerProvider is used with an id.
  • Mock DialogPortalDestination in the SSR-specific test to keep the assertion focused on provider rendering behavior.

Validation

  • Master dialog context suite passes locally:
    • yarn test src/components/Dialog/__tests__/DialogManagerContext.test.tsx
  • Context menu behavior was rechecked in the local app repro URL and now opens after toggle.

Impact

  • Prevents initial child drop on first render.
  • Restores stable dialog/context-menu behavior for shared manager scenarios.
  • Keeps behavior aligned between master and release-v13 implementations where APIs differ.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

Size Change: +223 B (+0.04%)

Total Size: 615 kB

📦 View Changed
Filename Size Change
dist/cjs/emojis.js 2.96 kB +2 B (+0.07%)
dist/cjs/index.js 237 kB +104 B (+0.04%)
dist/cjs/WithAudioPlayback.js 43.7 kB +7 B (+0.02%)
dist/es/emojis.mjs 2.47 kB -2 B (-0.08%)
dist/es/index.mjs 235 kB +106 B (+0.05%)
dist/es/WithAudioPlayback.mjs 43.5 kB +6 B (+0.01%)
ℹ️ View Unchanged
Filename Size
dist/cjs/audioProcessing.js 1.32 kB
dist/cjs/mp3-encoder.js 1.27 kB
dist/css/emoji-replacement.css 456 B
dist/css/index.css 45.4 kB
dist/es/audioProcessing.mjs 1.31 kB
dist/es/mp3-encoder.mjs 756 B

compressed-size-action

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.18%. Comparing base (443b9a8) to head (c86caaf).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3104      +/-   ##
==========================================
- Coverage   82.21%   82.18%   -0.04%     
==========================================
  Files         418      418              
  Lines       12044    12054      +10     
  Branches     3875     3884       +9     
==========================================
+ Hits         9902     9906       +4     
- Misses       2142     2148       +6     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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