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

Fix embedded crash when opening dialogs #2469

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Conversation

garrettjstevens
Copy link
Collaborator

Fixes #2466

This doesn't update the babel configuration (I tried that without any luck), but changes the code to avoid what is probably a weird bug in babel. It makes the volatile queueOfDialogs array not observable, and re-assigns the variable each time it's altered so the volatile's top-level observability is sufficient.

Came up with this with @cmdcolin during a pairing.

Co-authored-by: Colin Diesh <colin.diesh@gmail.com>
@garrettjstevens garrettjstevens self-assigned this Oct 28, 2021
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 28, 2021
@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #2469 (140be8a) into main (473b37b) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2469      +/-   ##
==========================================
+ Coverage   61.15%   61.18%   +0.03%     
==========================================
  Files         539      539              
  Lines       25072    25076       +4     
  Branches     5858     5858              
==========================================
+ Hits        15332    15343      +11     
+ Misses       9422     9415       -7     
  Partials      318      318              
Impacted Files Coverage Δ
...-genome-view/src/createModel/createSessionModel.ts 11.39% <0.00%> (-0.15%) ⬇️
...-genome-view/src/createModel/createSessionModel.ts 18.75% <0.00%> (-0.24%) ⬇️
packages/core/TextSearch/TextSearchManager.ts 100.00% <0.00%> (+3.44%) ⬆️
...inearGenomeView/components/RefNameAutocomplete.tsx 86.02% <0.00%> (+10.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 473b37b...140be8a. Read the comment docs.

@rbuels rbuels added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Oct 29, 2021
@cmdcolin cmdcolin merged commit 10326b8 into main Oct 29, 2021
@cmdcolin cmdcolin deleted the 2466_embedded_crash_fix branch October 29, 2021 01:33
cmdcolin added a commit that referenced this pull request Nov 1, 2021
…from embedded apps (#2469)


Co-authored-by: Colin Diesh <colin.diesh@gmail.com>
@rbuels
Copy link
Contributor

rbuels commented Apr 27, 2023

Are the storybooks sufficient to test if this is still happening? Trying to figure out if I need to include this in #3661

@cmdcolin
Copy link
Collaborator

I would consider observable arrays in a volatile to be overcomplicating things so I would keep it as is, and it may not be caught by tests (would have to be a component_tests/lgv probably to test the built products)

@rbuels
Copy link
Contributor

rbuels commented Apr 27, 2023 via email

@cmdcolin
Copy link
Collaborator

cmdcolin commented Apr 27, 2023

ya should be fine to unify across products (either nonobservable.array or observer, nonobserver should be fine though and we can audit usages of observer if it is helpful)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedded circular and linear views crash when opening session dialog component
3 participants