This repository has been archived by the owner on Apr 18, 2024. It is now read-only.
fix: OPTIC-353: Fix null errors when switching from task detail to settings #1665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1665 +/- ##
==========================================
- Coverage 68.69% 64.58% -4.11%
==========================================
Files 443 443
Lines 28721 28723 +2
Branches 7636 7523 -113
==========================================
- Hits 19729 18552 -1177
- Misses 7748 10171 +2423
+ Partials 1244 0 -1244 ☔ View full report in Codecov by Sentry. |
hlomzik
reviewed
Jan 11, 2024
hlomzik
reviewed
Jan 11, 2024
Co-authored-by: hlomzik <[email protected]>
…hub.com:heartexlabs/label-studio-frontend into fb-optic-353/task-detail-to-settings-null-error
bmartel
approved these changes
Jan 12, 2024
Gondragos
approved these changes
Jan 15, 2024
hlomzik
approved these changes
Jan 15, 2024
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…hub.com:heartexlabs/label-studio-frontend into fb-optic-353/task-detail-to-settings-null-error
Travis1282
force-pushed
the
fb-optic-353/task-detail-to-settings-null-error
branch
from
532b5b0 to
240284b
Compare
Travis1282
force-pushed
the
fb-optic-353/task-detail-to-settings-null-error
branch
from
240284b to
02a1917
Compare
Travis1282
deleted the
fb-optic-353/task-detail-to-settings-null-error
branch
juliosgarbi
pushed a commit
that referenced
this pull request
Jan 24, 2024
…ttings (#1665) * fix: optic-353: fix null errors when switching from task detail to settings * null block children before state tree destroy * Update src/LabelStudio.js Co-authored-by: hlomzik <[email protected]> * fix null block logic for children * commiting console to understand the difference between local and deployed * destroy annotaions first * use getRoot to find the selected annotation for video region * revert image selection * Update src/stores/AppStore.js Co-authored-by: Sergey <[email protected]> * check if annotation store exists from root * allow no conditional assignment for while loop --------- Co-authored-by: hlomzik <[email protected]> Co-authored-by: Sergey <[email protected]>
juliosgarbi
added a commit
that referenced
this pull request
Jan 24, 2024
fix: OPTIC-353: Fix null errors when switching from task detail to settings (#1665) * fix: optic-353: fix null errors when switching from task detail to settings * null block children before state tree destroy * Update src/LabelStudio.js * fix null block logic for children * commiting console to understand the difference between local and deployed * destroy annotaions first * use getRoot to find the selected annotation for video region * revert image selection * Update src/stores/AppStore.js * check if annotation store exists from root * allow no conditional assignment for while loop --------- Co-authored-by: Travis Clark <[email protected]> Co-authored-by: hlomzik <[email protected]> Co-authored-by: Sergey <[email protected]>
hlomzik
added a commit
that referenced
this pull request
Feb 20, 2024
### Main change **Only one annotation is selected during task opening.** We have the concept of selecting annotation, when we not just assign it as current one, but also update data in tags, trigger external event, load annotation history, setup hotkeys, set initial values. Previosly it was required to select every annotation to do some extra work in regions/tags inside it because of some quirky legacy code. I fixed what I found so far and ran all possible tests — they all are green, so consider this change mostly safe. Benefits: - improved performance as only one annotation is selected, reducing number of calculations and renders - no excess network requests to retrieve annotation history for every of these annotations - more clean code - one more step to remove one of the oldest legacy concept of `states`: it's not used in init anymore Things changed: - `STORE_INIT_OK` global var added to catch tags that can't properly work with new system (edge cases we missed) - apparently we have only one unit test for tags — Ranker; it's securely mocked Concerns: - `states` in Video tag select only *Labels controls, but changed code is in `fixBrokenAnnotation` which will do nothing to Video anyway - in potential missed edge case app will just crash - we could potentially check that we are inside init process by some flag, right? ### Other changes - removed Predictions panel and related code like showAllPredictions - removed panels from App and down the stream — we don't use them anywhere (3rd party LSF can be affected, but not sure anyone using panels there) - converted `LabelStudio.js` to TypeScript - fixed bunch of annoying react warnings (non-html props, wrong values, missed refs) - started to reorganize our "envs" to get rid of them at some point; for now it's `getRoot()` removed from there - removed `SidebarPage` as it was useless, plus removed multi-tabs support from `SidebarTabs` as `panels` were not used; plus that's all only for the old interface which is about to be removed - removed `hydrated` as it's not used anymore after #1166 - changed order of annotations to original one without reversing; thus order will always be the same, fixing the issue when list was reversed again after task reload; visual order is defined in `AnnotationsCarousel` anyway, so no changes in UX - fixed annotations order in View All to be the same as in annotation tabs * Remove View All Predictions and anything related It's not used anymore for a while * Move LabelStudio to TS, improve a little - Type drafts for main LSF class - getRootElement moved from env - Fixed methods signatures - Fixed typo in method name Most of the imports have TS errors because they are not typed; let it be like this for now. * Simplify SidebarTabs and remove unused panels param `panels` were not used anywhere, and this is part of old interface, that will be removed soon. + fix global Htx typing + fix conditions in App * Remove `hydrated`, we don't need it after #1166 * Fix some annoying react warnings * Don't select every annotation on init Also don't call any unneccessary methods for render. Just init regions. * Add flags to test correct behavior of init * Fix Relation init * fix our fixBrokenAnnotation() method Don't use states(), we already have simple toNames * Some other small fixes React warnings, simplier code * Fix merged results - they can also use toNames This is needed only for Video rectangles * Temporary fix for unit tests We don't have a real init, so overpass it * Fix history reinit for some cases Should fix failed e2e with TimeSeries, Audio and Video * Fix annotation selected by default: should be 1st one Annotations added to the start of the list, so the last one added is the first one in the list and is selected by default * Remove unused Annotation#update model prop * Add feature flag to cover annotation select * Fix FF name * Add a description for this FF * Add doc with lot of investigations * Tiny fix for panel classnames no extra whitespaces * Fix from #1665 * Fix typos in LSF.init.md * Trigger LS build * Fix typo to trigger LS build again * Move simpleInit to volatile This allows to check FF indirectly in other apps * Add more info about TopBar * Add couple of deprecation messages * Stop reversing annotations list Reversing the list caused problems when task is reloaded and list is reversed again. The visual order is defined in AnnotationsCarousel, so technical order is not that important. View All used technical order which can cause discomfort when you see different order in different places. So that was also fixed. Everything is behind the same FF, without it the order is an old one, reversed. * Move annotations ordering to utilities; sort imports * Fix annotation selected after View All --------- Co-authored-by: hlomzik <[email protected]>
MasherJames
pushed a commit
to HelloPareto/label-studio-frontend
that referenced
this pull request
Feb 29, 2024
…ttings (HumanSignal#1665) * fix: optic-353: fix null errors when switching from task detail to settings * null block children before state tree destroy * Update src/LabelStudio.js Co-authored-by: hlomzik <[email protected]> * fix null block logic for children * commiting console to understand the difference between local and deployed * destroy annotaions first * use getRoot to find the selected annotation for video region * revert image selection * Update src/stores/AppStore.js Co-authored-by: Sergey <[email protected]> * check if annotation store exists from root * allow no conditional assignment for while loop --------- Co-authored-by: hlomzik <[email protected]> Co-authored-by: Sergey <[email protected]>
MasherJames
pushed a commit
to HelloPareto/label-studio-frontend
that referenced
this pull request
Feb 29, 2024
### Main change **Only one annotation is selected during task opening.** We have the concept of selecting annotation, when we not just assign it as current one, but also update data in tags, trigger external event, load annotation history, setup hotkeys, set initial values. Previosly it was required to select every annotation to do some extra work in regions/tags inside it because of some quirky legacy code. I fixed what I found so far and ran all possible tests — they all are green, so consider this change mostly safe. Benefits: - improved performance as only one annotation is selected, reducing number of calculations and renders - no excess network requests to retrieve annotation history for every of these annotations - more clean code - one more step to remove one of the oldest legacy concept of `states`: it's not used in init anymore Things changed: - `STORE_INIT_OK` global var added to catch tags that can't properly work with new system (edge cases we missed) - apparently we have only one unit test for tags — Ranker; it's securely mocked Concerns: - `states` in Video tag select only *Labels controls, but changed code is in `fixBrokenAnnotation` which will do nothing to Video anyway - in potential missed edge case app will just crash - we could potentially check that we are inside init process by some flag, right? ### Other changes - removed Predictions panel and related code like showAllPredictions - removed panels from App and down the stream — we don't use them anywhere (3rd party LSF can be affected, but not sure anyone using panels there) - converted `LabelStudio.js` to TypeScript - fixed bunch of annoying react warnings (non-html props, wrong values, missed refs) - started to reorganize our "envs" to get rid of them at some point; for now it's `getRoot()` removed from there - removed `SidebarPage` as it was useless, plus removed multi-tabs support from `SidebarTabs` as `panels` were not used; plus that's all only for the old interface which is about to be removed - removed `hydrated` as it's not used anymore after HumanSignal#1166 - changed order of annotations to original one without reversing; thus order will always be the same, fixing the issue when list was reversed again after task reload; visual order is defined in `AnnotationsCarousel` anyway, so no changes in UX - fixed annotations order in View All to be the same as in annotation tabs * Remove View All Predictions and anything related It's not used anymore for a while * Move LabelStudio to TS, improve a little - Type drafts for main LSF class - getRootElement moved from env - Fixed methods signatures - Fixed typo in method name Most of the imports have TS errors because they are not typed; let it be like this for now. * Simplify SidebarTabs and remove unused panels param `panels` were not used anywhere, and this is part of old interface, that will be removed soon. + fix global Htx typing + fix conditions in App * Remove `hydrated`, we don't need it after HumanSignal#1166 * Fix some annoying react warnings * Don't select every annotation on init Also don't call any unneccessary methods for render. Just init regions. * Add flags to test correct behavior of init * Fix Relation init * fix our fixBrokenAnnotation() method Don't use states(), we already have simple toNames * Some other small fixes React warnings, simplier code * Fix merged results - they can also use toNames This is needed only for Video rectangles * Temporary fix for unit tests We don't have a real init, so overpass it * Fix history reinit for some cases Should fix failed e2e with TimeSeries, Audio and Video * Fix annotation selected by default: should be 1st one Annotations added to the start of the list, so the last one added is the first one in the list and is selected by default * Remove unused Annotation#update model prop * Add feature flag to cover annotation select * Fix FF name * Add a description for this FF * Add doc with lot of investigations * Tiny fix for panel classnames no extra whitespaces * Fix from HumanSignal#1665 * Fix typos in LSF.init.md * Trigger LS build * Fix typo to trigger LS build again * Move simpleInit to volatile This allows to check FF indirectly in other apps * Add more info about TopBar * Add couple of deprecation messages * Stop reversing annotations list Reversing the list caused problems when task is reloaded and list is reversed again. The visual order is defined in AnnotationsCarousel, so technical order is not that important. View All used technical order which can cause discomfort when you see different order in different places. So that was also fixed. Everything is behind the same FF, without it the order is an old one, reversed. * Move annotations ordering to utilities; sort imports * Fix annotation selected after View All --------- Co-authored-by: hlomzik <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OPTIC-353
Error when switching from task detail to settings from a video annotation with the region or intermittently images with regions fixed with a null block of child nodes before they are used as an arg in clearRenderedApp. Note this bug is connected to the effort to reduce the number of memory leaks and can be also resolved by turning off the feature flag: fflag_fix_front_lsdv_4620_memory_leaks_100723_short