-
Notifications
You must be signed in to change notification settings - Fork 320
fix: LEAP-31: Fix functioning of auto annotations #1605
Conversation
# Conflicts: # tests/functional/package.json # tests/functional/yarn.lock
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1605 +/- ##
==========================================
+ Coverage 67.93% 68.17% +0.24%
==========================================
Files 443 443
Lines 28653 28662 +9
Branches 7615 7615
==========================================
+ Hits 19464 19539 +75
+ Misses 7921 7866 -55
+ Partials 1268 1257 -11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, i just added 1 little suggestion for readability since its a one liner but thats optional since its fine as it is
…otation-fixes # Conflicts: # tests/functional/package.json # tests/functional/yarn.lock
Co-authored-by: yyassi-heartex <[email protected]>
return canBePartOfNotification | ||
&& region.type === type | ||
&& region.labelName === labelName | ||
&& region.results?.[0]?.to_name === self.results?.[0]?.to_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It potentially compare undefined === undefined
here. Could it lead to any unexpected issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just checked that. It seems that the only way to get undefined === undefined
is collecting connected regions in the case with config that contains two classification tags with the same to_name
. I'm not sure if we need to group them but it's anyhow is better than it was.
// Indicates that it is not temporary region created just to display data like Textarea's one | ||
// and is not a suggestion | ||
get isRealRegion() { | ||
return self.annotation?.areas?.has(self.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually smart. Love it.
@@ -252,6 +252,7 @@ export const Annotation = types | |||
draftSelected: false, | |||
autosaveDelay: 5000, | |||
isDraftSaving: false, | |||
isSuggestionsAccepting: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add a comment describing this flag? from the name alone it's not very clear what it supposed to do
* fix: LEAP-31: Fix functioning of auto annotations * Add tests * Fix eslint warnings * Update ls-test * Remove Region#revokeSuggestion * Update ls-test * Refactor Co-authored-by: yyassi-heartex <[email protected]> * Update ls-test * Add comment * Update ls-test * fix deps --------- Co-authored-by: yyassi-heartex <[email protected]> Co-authored-by: hlomzik <[email protected]>
PR fulfills these requirements
[fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made
ex.fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
Change has impacts in these area(s)
(check all that apply)
Describe the reason for change
Working with data annotations was too complicated and inconsistent in a lot of cases so this PR is trying to change that by fixing bugs and implementing a different approach for the tags that do not support suggestion by design.
What does this fix?
regionFinishedDrawing
event (they don't have to be dynamic). And it will always accept suggestions automaticaly.What libraries were added/updated?
@heartexlabs/ls-test
Does this change affect performance?
Nope
Does this change affect security?
Nope
What alternative approaches were there?
Was not detected
What feature flags were used to cover this change?
N/A
Does this PR introduce a breaking change?
(check only one)
It fixes a lot and it didn't work properly before, but it should be reviewed and tested with full attention.
What level of testing was included in the change?
(check all that apply)
Which logical domain(s) does this change affect?
Auto Annotations
,Smart tools
,Dynamic regions
,LLM
,Image
,Textarea