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

NickAkhmetov/CAT-634 Fix selection of samples on organ page when more than 500 are present #3595

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

NickAkhmetov
Copy link
Collaborator

Summary

The Kidney page on prod currently fails to select all results when using the "select all" checkbox of the samples table. This issue was caused by the useAllSearchIds hook being limited to only fetching the first 500 results. Lifting this limit/removing the "number of pages to fetch" from the ID lookup resolved this issue by enabling lookup of all 634 results in the table.

Design Documentation/Original Tickets

https://hms-dbmi.atlassian.net/browse/CAT-634

Testing

Tested on the kidney page as well as other organ pages.

Screenshots/Video

Screen.Recording.2024-11-04.104133.mp4

Checklist

  • Code follows the project's coding standards
    • Lint checks pass locally
    • New CHANGELOG-your-feature-name-here.md is present in the root directory, describing the change(s) in full sentences.
  • Unit tests covering the new feature have been added
  • All existing tests pass
  • Any relevant documentation in JIRA/Confluence has been updated to reflect the new feature
  • Any new functionalities have appropriate analytics functionalities added

Additional Notes

I'm not sure if this PR overlaps with the changes in the searchkit PR - it can definitely wait if so.

@@ -243,21 +243,14 @@ export function useAllSearchIDs(
);

const totalHitsCount = getTotalHitsCount(searchData);
const numberOfPagesToRequest = totalHitsCount ? Math.ceil(10000 / totalHitsCount) : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this continue to handle > 10k results?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is definitely worth testing to be safe in the long run - I can test it out with a query for all samples without any filters and confirm whether it works as expected.

The 10k results limitation is definitely worth keeping in mind in general, but I am curious - per #3593, it seems like we aren't as close to the 10k result threshold as we thought once we filter samples without descendant datasets, so maybe this could be a premature optimization?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would slightly prefer to keep the logic to handle this in place rather than have to re-visit suddenly it in the future if something changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have ~7k datasets with HuBMAP Read.

@NickAkhmetov
Copy link
Collaborator Author

Following John's feedback, I've made some improvements to the fetchAllIds functionality, using an async generator to sequentially request all the IDs for a given search request's results.

On login with HuBMAP Read, there are >20,000 kidney samples, so I was able to confirm the fix:

Screen.Recording.2024-11-05.122353.mp4

It is worth noting that adding the selected to a list caused the page to freeze. This might become a bigger issue if we start persisting lists in a key-value store.

Copy link
Collaborator

@john-conroy john-conroy left a 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! We could likely avoid the additional forEach call, but not a big deal. I wasn't aware of the _ convention in numbers for readability so thanks for teaching me that!

@NickAkhmetov NickAkhmetov merged commit 4a8111c into main Nov 5, 2024
8 checks passed
@NickAkhmetov NickAkhmetov deleted the nickakhmetov/cat-634-select-all-organ-samples branch November 5, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants