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

Jump to bottom

Enable search for new samples query #163

Merged
merged 22 commits into from
Sep 27, 2024
Merged

Enable search for new samples query #163

merged 22 commits into from
Sep 27, 2024

Conversation

qu8n
Copy link
Collaborator

@qu8n qu8n commented Sep 26, 2024 •
edited
Loading

This PR includes the work for cards #1224 and #1281, with most changes for the former.

This enables search for the new samples query written in #162.

High-level details of the new flow:

  • User inputs some value(s) into the search bar and press Enter or click "Search"
  • The React frontend calls a new GraphQL query called DashboardSamples
  • In the Apollo server, the resolvers for this query run Cypher queries and return the results

TODOs in the next PR:

  • Re-enable editing
  • Re-enable users' ability to download >500 and <5,000 samples. Right now, users can either download up to 500 or not at all
  • Resolve the duplicate tableElements.tsx files
  • Consider implementing or using a 3rd party package for a Cypher query builder
  • Fix exported file to show only the non-hidden columns

@qu8n
Update configs to work with new Neo4j version
d553c44
@qu8n
Move nested fields returned in the new Samples query to the root level
3dd62a4
@qu8n
Create a placeholder custom query and type for the new Samples query
b4c6725
@qu8n
Populate cancerType and cancerTypeDetailed fields with the new Sample…
3aeb9c9 
…s query
@qu8n
Enable searching in the all-samples view of Samples page
b8137e7
@qu8n
Populate recipe with the new Samples query
8d121db
@qu8n
Add better error handling when getting oncotree data from cache
7b3ff5c
@qu8n
Search ct and ctDetailed fields more efficiently
2bcf611
@qu8n
Create a separate custom schema for dashboard samples
89ce5c0
@qu8n
Add a context arg to the new samples query & enable WES view
3c5d0f5
@qu8n
Use the new samples query in the Request Samples view
d71c2b4
@qu8n
Use the new samples query in the Patient Samples view
affcd77
@qu8n
Use the new samples query in the Cohort Samples view
c433e62
@qu8n
Resolve sample count using the new samples query
c7a0dbb
@qu8n
Clean up the old sample query's WHERE variables
f9a2177
@qu8n
Remove unnecessary React state 'parsedSearchVals' in Samples view
de8fe9f
@qu8n
Remove dataloader package
194efd4 
We previously used dataloader to make a single query for samples via
the OGM. Otherwise, the `samples` and `samplesConnection` sub-queries
would be call it twice.

With the new approach, we use two slightly different queries to get
samples data and the count of samples. We could use dataloader to access
the oncotree cache, but this operation is very fast (< 0.05ms).
@qu8n
Trigger search when search form is cleared
3466ed4
@qu8n
Trigger search when search form is cleared
040232f
@qu8n
Temporarily handle table element diffs between SamplesList and Record…
c367406 
…sList components
@qu8n
Add Investigator Sample ID column to WES Samples view
b762ca1
qu8n
qu8n commented Sep 27, 2024
View reviewed changes
frontend/src/components/SamplesList.tsx Show resolved Hide resolved
frontend/src/shared/tableElements-records.tsx Outdated Show resolved Hide resolved
graphql-server/src/schemas/custom.ts Outdated Show resolved Hide resolved
@qu8n qu8n requested a review from ao508 September 27, 2024 13:17
@qu8n qu8n marked this pull request as ready for review September 27, 2024 13:17
ao508

This comment was marked as resolved.

Sign in to view

@qu8n

This comment was marked as resolved.

Sign in to view
@qu8n

This comment was marked as outdated.

Sign in to view
ao508

This comment was marked as resolved.

Sign in to view

ao508
ao508 reviewed Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@ao508 ao508 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One thing I've noticed while testing is that all columns get downloaded on the samples page even when there are columns hidden. Is this a TODO for a later card?

Related to this: there still seems to be a cap of 500 samples here when downloading from the samples page

To replicate: I used search term "BRCNOS" which should return 804 samples (dev server)

@qu8n
Copy link
Collaborator Author

qu8n commented Sep 27, 2024 •
edited
Loading

One thing I've noticed while testing is that all columns get downloaded on the samples page even when there are columns hidden. Is this a TODO for a later card?

@ao508 Thanks for catching that. Adding it to the next PR's todos now.

Related to this: there still seems to be a cap of 500 samples here when downloading from the samples page

To replicate: I used search term "BRCNOS" which should return 804 samples (dev server)

This is one of my TODOs for the next PR. You can find this list in this PR's description here for now. I'll add this list to the next Zenhub card after we finish with this PR.

ao508
ao508 reviewed Sep 27, 2024
View reviewed changes
frontend/src/components/SamplesList.tsx Outdated
import styles from "./records.module.scss";
import { getUserEmail } from "../utils/getUserEmail";
import { openLoginPopup } from "../utils/openLoginPopup";
import { Title } from "../shared/components/Title";
import { BreadCrumb } from "../shared/components/BreadCrumb";
import { useParams } from "react-router-dom";
import { DataName } from "../shared/types";
import { parseUserSearchVal } from "../utils/parseSearchQueries";

const POLLING_INTERVAL = 2000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind increasing this to 2700 (45 seconds)? I'm hoping that the extra interval of time will result in less "blinking" when viewing and updating samples for a request/patient/cohort

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is in milliseconds, so this is currently at 2 seconds. I just bumped it up to 5 seconds in the dev dashboard. Let me know if you're still seeing it. If you're able to record a clip of this "blinking", I'm happy to take a look as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(If this blinking is a big problem, we can consider using websocket to display real-time sample updates instead. More optimal as we don't continuously refetch for no reason.)

ao508
ao508 approved these changes Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@ao508 ao508 left a comment

Choose a reason for hiding this comment

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

Looking good, thank you!

@ao508 ao508 merged commit b09494b into mskcc:samples-query-optimization Sep 27, 2024
2 checks passed
@qu8n qu8n deleted the enable-search-for-new-samples-query branch November 14, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ao508 ao508 ao508 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qu8n @ao508