Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

fix: LEAP-740: Don't use destroy = null #1706

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Conversation

hlomzik
Copy link
Collaborator

@hlomzik hlomzik commented Feb 23, 2024

There are external places that can call destroy() unconditionally, so having it uninitialized is dangerous and leads to app crash.

Note: 4 e2e tests are disabled waiting for #1708

PR fulfills these requirements

  • Tests for the changes have been added/updated
  • Docs have been added/updated
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance

Describe the reason for change

App was crashing in Config Preview (editor) when you use config with Image tag and change its value resulting in error f.current.destroy is not a function

What feature flags were used to cover this change?

That only happens with fflag_feat_front_lsdv_4583_6_images_preloading_short ON
And despite the change was introduced in #1640 it happens even with fflag_fix_front_leap_443_select_annotation_once OFF

What alternative approaches were there?

Check what's going on with image preloading that forces Preview to destroy LSF in some different way. Will do this later.

This change affects (describe how if yes)

  • Performance
  • Security
  • UX

Does this PR introduce a breaking change?

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Fixes breaking change

What level of testing was included in the change?

  • e2e (codecept)
  • integration (cypress)
  • unit (jest)

There are external places that can call destroy unconditionally,
so having it uninitialized is dangerous and leads to app crash.
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.41%. Comparing base (313caaa) to head (2a5f7f6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1706      +/-   ##
==========================================
- Coverage   68.83%   68.41%   -0.42%     
==========================================
  Files         441      441              
  Lines       28757    28757              
  Branches     7639     7639              
==========================================
- Hits        19794    19674     -120     
- Misses       7715     7815     +100     
- Partials     1248     1268      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hlomzik hlomzik merged commit 2383318 into master Feb 26, 2024
15 checks passed
@hlomzik hlomzik deleted the fb-leap-740/proper-destroy branch February 26, 2024 10:52
MasherJames pushed a commit to HelloPareto/label-studio-frontend that referenced this pull request Feb 29, 2024
* fix: LEAP-740: Don't use destroy = null

There are external places that can call destroy unconditionally,
so having it uninitialized is dangerous and leads to app crash.

* Fail E2E test if Image is not loaded

* Revert destroy() behavior to original one

* Trigger CI

* Skip failing tests for now (until HumanSignal#1708)
@hlomzik hlomzik mentioned this pull request Mar 4, 2024
7 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants