Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STCOM-1386 Paneset: check for existing id before registering pane #2395

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JohnC-80
Copy link
Contributor

@JohnC-80 JohnC-80 commented Nov 22, 2024

Panes can exhibit strange behavior in the dev environment that just doesn't happen in production. I believe this is caused by pane registration misbehaving in React StrictMode (where mounting happens twice). This PR dedupes the panes by checking if a pane id already exists in the paneset - and if it does, the pane is not added.

Problematic behavior can be seen specifically with panes that have defaultWidth: "fill" - they'll be registered twice, and their width calculation (which divides the remaining width among other fill panes) will leave them being half the size they should be. Hiding a search/filter pane can leave the results view at only 50% of its width, when it should be the full width of the view when it's the only pane.

Copy link

github-actions bot commented Nov 22, 2024

Bigtest Unit Test Results

    1 files  ±0      1 suites  ±0   15s ⏱️ ±0s
1 533 tests ±0  1 525 ✅ ±0  8 💤 ±0  0 ❌ ±0 
1 535 runs  ±0  1 527 ✅ ±0  8 💤 ±0  0 ❌ ±0 

Results for commit 2e61b74. ± Comparison against base commit a3e41cd.

♻️ This comment has been updated with latest results.

@JohnC-80 JohnC-80 marked this pull request as ready for review November 25, 2024 14:46
@JohnC-80 JohnC-80 requested a review from zburke November 25, 2024 14:47
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Your description of the symptom sounds correct. Bravo! I have seen this and been totally mystified by it.

Are you confident this the Most Correct fix? I worry a little bit that we are treating the symptom here (double registration) instead of a root problem (missing cleanup function).

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