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

Merged
merged 3 commits into from
Feb 7, 2025

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 535 tests ±0  1 527 ✅ ±0  8 💤 ±0  0 ❌ ±0 
1 537 runs  ±0  1 529 ✅ ±0  8 💤 ±0  0 ❌ ±0 

Results for commit 3a4db22. ± Comparison against base commit 5a35a7c.

♻️ 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).

@JohnC-80 JohnC-80 requested a review from a team as a code owner February 6, 2025 21:43
@JohnC-80
Copy link
Contributor Author

JohnC-80 commented Feb 6, 2025

The anointed React best-practice tenant is applicable here - ensure mount/setState function can run twice w/o causing issue. Deduplication as implemented here seems like the best way. Other techniques such as avoiding the setState callbacks and pushing the calls into the setState function itself still resulted in duplicate items in state passed to code that would actually calculate the necessary style updates. This way, that never happens.

@zburke zburke self-requested a review February 7, 2025 01:41
@JohnC-80 JohnC-80 merged commit eafab8c into master Feb 7, 2025
15 checks passed
@JohnC-80 JohnC-80 deleted the STCOM-1386 branch February 7, 2025 20:00
github-actions bot pushed a commit that referenced this pull request Feb 7, 2025
)

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