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.

Sign up for GitHub

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 infinite reload of sample lists #4641

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Fix infinite reload of sample lists #4641

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 13 additions & 31 deletions src/shared/components/query/QueryStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
observable,
ObservableMap,
reaction,
runInAction,
toJS,
} from 'mobx';
import {
Expand Down Expand Up @@ -296,7 +297,10 @@ export class QueryStore {
}

set selectableSelectedStudyIds(val: string[]) {
this._allSelectedStudyIds = observable.map(stringListToSet(val));
runInAction(() => {
this.selectedSampleListId = undefined;
this._allSelectedStudyIds = observable.map(stringListToSet(val));
});
}

@action
Expand Down Expand Up @@ -474,9 +478,12 @@ export class QueryStore {
@observable private _selectedSampleListId?: string = undefined; // user selection
@computed
public get selectedSampleListId() {
if (this._selectedSampleListId !== undefined)
return this._selectedSampleListId;
return this.defaultSelectedSampleListId;
const matchesSelectedStudy = this.sampleListInSelectedStudies.result.some(
sampleList => sampleList.sampleListId === this._selectedSampleListId
);
return matchesSelectedStudy
? this._selectedSampleListId
: this.defaultSelectedSampleListId;
}

public set selectedSampleListId(value) {
Expand Down Expand Up @@ -1004,14 +1011,6 @@ export class QueryStore {
);
},
default: [],
onResult: () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BasLee this is the offending onResult. It depends upon this._selectedSampleListId, so when it mutates that value it re-fires invoke and is circular.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe that it was necessary in order to clear the selection when user selected a different study. This can/should be handled on read by checking to see if the _selectedSampleListId matches selectedStudies

Copy link

Choose a reason for hiding this comment

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

the weird thing was, that the infinite loop even existed without any code in the onResult: it seemed like the presence of onResult itself was causing the loop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interesting. i will test that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i see that you are right. in my testing, the problem is that mobxpromise's onResult references this.result in order to pass it to the callback:

image

when i use result instead of this.result, the problem goes away. will have to make this change to the library.

nevertheless, i think getting rid of onResult in general is a good idea as it is almost never actually necessary.

if (
!this.initiallySelected.sampleListId ||
this.studiesHaveChangedSinceInitialization
) {
this._selectedSampleListId = undefined;
}
},
});

readonly validProfileIdSetForSelectedStudies = remoteData({
Expand Down Expand Up @@ -1075,7 +1074,7 @@ export class QueryStore {
return _.sumBy(this.selectableSelectedStudies, s => s.allSampleCount);
}

readonly sampleLists = remoteData({
readonly sampleLists = remoteData<SampleList[]>({
invoke: async () => {
if (!this.isSingleNonVirtualStudySelected) {
return [];
Expand All @@ -1088,14 +1087,6 @@ export class QueryStore {
return _.sortBy(sampleLists, sampleList => sampleList.name);
},
default: [],
onResult: () => {
if (
!this.initiallySelected.sampleListId ||
this.studiesHaveChangedSinceInitialization
) {
this._selectedSampleListId = undefined;
}
},
});

readonly mutSigForSingleStudy = remoteData({
Expand Down Expand Up @@ -2295,16 +2286,7 @@ export class QueryStore {

let urlParams = this.asyncUrlParams.result;

if (this.forDownloadTab) {
formSubmit(
buildCBioPortalPageUrl('data_download'),
urlParams.query,
undefined,
'smart'
);
} else {
this.singlePageAppSubmitRoutine(urlParams.query, currentTab);
}
this.singlePageAppSubmitRoutine(urlParams.query, currentTab);
Copy link

Choose a reason for hiding this comment

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

this is an unrelated improvement/bug fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, download mode doesn't exist anymore so I've been trying to clean this stuff out, but i should probably do it in a seperate PR


return true;
}
Expand Down