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 duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors #6058

Merged
merged 7 commits into from Feb 28, 2022

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Feb 17, 2022

This PR fixes errors in the explorative annotation list view of the dashboard. It was possible to duplicate shown annotations by toggling the "show archived annotations" button. It was also possible to have the wrong actions rendered for an annotation.
I additionally included a fix for the downsample modal not being rendered and even causing the layer settings to crash / not being rendered anymore.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Go to the my annotations view in the dashboard
  • make sure to have at least 2 annotations
  • archive an annotation X
  • click "show archived annotations"
  • click "show unarchived annotations"
  • annotation X should not appear again
  • toggle more often between "show archived annotations" and show unarchived annotations". The should not be any duplicating of annotations.
  • Click on "show unarchived annotations" to show the archived annotations and then reload
  • The shown annotations should be the exact same as before and should only include actually archived annotations.
  • Now toggling between "show archived annotations" and "show unarchived annotations" should still not lead to an inconsistent state.
  • create volume annotation with only one magnification (for example, temporarily remove mags from a segmentation layer via the dataset settings; create volume annotation with fallback; read the mags to the dataset using the auto suggestions in the dataset settings view)
  • click the orange "downsample" button in the left sidebar
  • the downsample should open up happily 😄

Issues:


(Please delete unneeded items, merge only when none are left open)

@MichaelBuessemeyer MichaelBuessemeyer changed the title Fix duplicating annotations via toggling show archived annotations button Fix duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors Feb 17, 2022
Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philippotto Could you please review these bug fixes? 🚫 🐛 🚫

@@ -210,8 +219,8 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
}

const hasVolumeTracing = getVolumeDescriptors(tracing).length > 0;
const { typ, id } = tracing;
if (!this.state.shouldShowArchivedTracings) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was that this method did not depend on the state of the annotation but on the "show archived buttons" state. This way it is saver, that the correct actions are shown

};

componentDidMount() {
this.setState(persistence.load(this.props.history));
this.fetchNextPage(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here was that the fetchNextPage relies on this.state.shouldShowArchivedTracings being up to date, but the setState Update of line 102 is not executed before the fetchNextPage is called. This lead to one of the errors. -> When wanting to initially show the archived annotations, the view instead showed the default (the active annotations).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that you found the issue 👍 However, I think, that there is another race condition looming: fetchNextPage itself fetches annotations (depending on this.state.shouldShowArchivedTracings) and then calls setModeState which itself reads again from this.state.shouldShowArchivedTracings. If the user toggles the state after the annotations-fetch has started but before it finished, the annotations will be pushed into the wrong array. I think, it would make sense to adapt setModeState to accept the shouldShowArchivedTracings parameter directly (instead of reading it out again). Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree?

Jep 👍 , that sounds like a reasonable approach to fix this.

Comment on lines 109 to 114
if (
this.state.shouldShowArchivedTracings !== prevState.shouldShowArchivedTracings ||
!this.state.performedInitialFetch
) {
this.fetchNextPage(0);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performedInitialFetch is needed to enforce the initial fetch, as otherwise the initial fetch would not be executed when the active annotations should be shown initially. Reason: Because the default value for shouldShowArchivedTracings is false leading to the whole if to be true.
performedInitialFetch simply enforces the initial fetch. I did not take the option make shouldShowArchivedTracings initially null, because performedInitialFetch seemed easier to understand later on to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's still a bit unusual, I guess, since componentDidUpdate is not called on the initial render (that's what componentDidMount is for). So, you are relying on the component updating, anyway, so that the fetch can be done. Probably this happens most of the time because of this.setState(persistence.load(this.props.history));, but it's not really safe. For example, if the localStorage is empty, the persistence module probably doesn't load anything so the state wouldn't change then?

I think, I would prefer to use this in componentDidMount:

componentDidMount() {
    this.setState(persistence.load(this.props.history), () => this.fetchNextPage(0));
}

The callback is triggered after setState is done. Then, you also don't need performedInitialFetch if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this happens most of the time because of this.setState(persistence.load(this.props.history));, but it's not really safe.

Oh yeah, you are totally right here 👍. That's a nice suggestion. Thanks 🚀.

The callback is triggered after setState is done. Then, you also don't need performedInitialFetch if I'm not mistaken.

👍

@@ -237,7 +246,7 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
<br />
</div>
);
} else {
} else if (state === "Finished") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philippotto Do you know of any other states an annotation can be in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm interpreting

CREATE TYPE webknossos.ANNOTATION_STATE AS ENUM ('Active', 'Finished', 'Cancelled', 'Initializing');
correctly, there can also be 'Cancelled' and 'Initializing'. However, I'm not sure if this is also possible for exploratives (and I've never seen "initializing"; maybe it's not exposed to the front-end). @fm3 Could you clarify this? Afterwards, @MichaelBuessemeyer can you change the string type in api_flow_types.js to "ACTIVE" | "Finished" | ... (depending on what @fm3 replied).

Copy link
Member

@fm3 fm3 Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explorative annotations exposed to the front-end can currently only be Active or Finished

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing these bugs 🐛 I left two suggestions which hopefully strengthen the reliability further :)

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
@@ -237,7 +246,7 @@ class ExplorativeAnnotationsView extends React.PureComponent<Props, State> {
<br />
</div>
);
} else {
} else if (state === "Finished") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm interpreting

CREATE TYPE webknossos.ANNOTATION_STATE AS ENUM ('Active', 'Finished', 'Cancelled', 'Initializing');
correctly, there can also be 'Cancelled' and 'Initializing'. However, I'm not sure if this is also possible for exploratives (and I've never seen "initializing"; maybe it's not exposed to the front-end). @fm3 Could you clarify this? Afterwards, @MichaelBuessemeyer can you change the string type in api_flow_types.js to "ACTIVE" | "Finished" | ... (depending on what @fm3 replied).

};

componentDidMount() {
this.setState(persistence.load(this.props.history));
this.fetchNextPage(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that you found the issue 👍 However, I think, that there is another race condition looming: fetchNextPage itself fetches annotations (depending on this.state.shouldShowArchivedTracings) and then calls setModeState which itself reads again from this.state.shouldShowArchivedTracings. If the user toggles the state after the annotations-fetch has started but before it finished, the annotations will be pushed into the wrong array. I think, it would make sense to adapt setModeState to accept the shouldShowArchivedTracings parameter directly (instead of reading it out again). Do you agree?

Comment on lines 109 to 114
if (
this.state.shouldShowArchivedTracings !== prevState.shouldShowArchivedTracings ||
!this.state.performedInitialFetch
) {
this.fetchNextPage(0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's still a bit unusual, I guess, since componentDidUpdate is not called on the initial render (that's what componentDidMount is for). So, you are relying on the component updating, anyway, so that the fetch can be done. Probably this happens most of the time because of this.setState(persistence.load(this.props.history));, but it's not really safe. For example, if the localStorage is empty, the persistence module probably doesn't load anything so the state wouldn't change then?

I think, I would prefer to use this in componentDidMount:

componentDidMount() {
    this.setState(persistence.load(this.props.history), () => this.fetchNextPage(0));
}

The callback is triggered after setState is done. Then, you also don't need performedInitialFetch if I'm not mistaken.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet!

@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) February 28, 2022 10:17
@MichaelBuessemeyer MichaelBuessemeyer merged commit 1dd5f62 into master Feb 28, 2022
@MichaelBuessemeyer MichaelBuessemeyer deleted the fix-annotation-dashboard-view branch February 28, 2022 10:41
hotzenklotz added a commit that referenced this pull request Mar 2, 2022
* 'docs' of github.com:scalableminds/webknossos:
  Connectome Tab (#5894)
  Fix duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors (#6058)
hotzenklotz added a commit that referenced this pull request Mar 29, 2022
* docs:
  mpre pr feedback
  Updates to wK Docs (#5995)
  restored some sections in sharing docs
  added context-menu hint to shortcuts
  more pr feedback
  Connectome Tab (#5894)
  Fix duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors (#6058)
hotzenklotz added a commit that referenced this pull request Mar 29, 2022
* master:
  fix docs formatting
  Remove experimental auto brush feature (#6107)
  Fix markdown issues in docs (#6105)
  Unify long-running RPC timeouts (#6103)
  Disallow deactivating users with active tasks (#6099)
  Add remote origin headers also in error case (#6098)
  Linking layers during upload no longer restricted to public datasets (#6097)
  Fix layer headers layout (#6087)
  Fix more docs links (#6095)
  Update datasets.md (#6094)
  More PR feedback for Docs (#6091)
  Updates to wK Docs (#5995)
  Connectome Tab (#5894)
  Fix duplicating annotations via toggling show archived annotations button and fix downsample modal rendering errors (#6058)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggling to/from showing archived annotations can lead to inconsistent states
3 participants