-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: display seqvars query execution results in UI (#1952) #1957
feat: display seqvars query execution results in UI (#1952) #1957
Conversation
deps-report 🔍Commit scanned: c0dad7d Vulnerable dependencies4 dependencies have vulnerabilities 😱
Outdated dependencies47 outdated dependencies found (including 19 outdated major versions)😢
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1957 +/- ##
======================================
- Coverage 91% 91% -1%
======================================
Files 667 674 +7
Lines 37986 38407 +421
======================================
+ Hits 34869 35066 +197
- Misses 3117 3341 +224
|
066b0fa
to
40bc415
Compare
@coderabbitai review |
✅ Actions performedReview triggered.
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request encompass the addition of new files and updates to existing files related to genetic data handling, API modifications, and user interface components. New VCF files and a pedigree file have been introduced, along with updates to Django models and API endpoints to accommodate changes in data retrieval and pagination methods. Additionally, several Vue components have been created or modified to enhance the user experience and data presentation in the frontend. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 27
🧹 Outside diff range and nitpick comments (21)
backend/Case_1_exons.ped (1)
1-1
: Consider adding a header row for improved readability.While the current format is valid, adding a header row could enhance readability and make the file more self-documenting. This is especially helpful for collaborators who might not be familiar with the pedigree file format.
Consider adding the following header row at the beginning of the file:
+Family_ID Individual_ID Father_ID Mother_ID Sex Affected_Status FAM_Case_1_exons_index Case_1_exons_index Case_1_exons_father Case_1_exons_mother 1 2
backend/seqvars/migrations/0009_auto_20240905_1017.py (1)
25-31
: LGTM with a minor suggestion: Genome release field updateThe change to the
genome_release
field inseqvarsresultrow
is well-implemented:
- It restricts the field to valid genome releases (GRCh37 and GRCh38).
- The use of choices improves data integrity and provides clear options in forms.
- The internal values are lowercase, which is good for consistency in queries.
Consider reducing the
max_length
to 16 or even 8, as the longest option is only 6 characters. This could slightly optimize database storage:field=models.CharField( choices=[("grch37", "GRCh37"), ("grch38", "GRCh38")], max_length=8 )frontend/src/cases/queries/cases.ts (1)
Line range hint
96-115
: LGTM! Consider the following suggestions for improvement.The implementation of
useCaseRetrieveQuery
is well-structured and follows the established patterns in the file. Here are some suggestions for further improvement:
- The TSLint disable comments (
@ts-ignore
) indicate potential type issues. Consider addressing these to improve type safety:path: { case: () => toValue(caseUuid)! },You might want to use a type assertion or refine the types to avoid using non-null assertions.
- Consider adding error handling to make the query more robust. For example:
export const useCaseRetrieveQuery = ({ caseUuid, }: { caseUuid: MaybeRefOrGetter<string | undefined> }) => useQuery({ ...casesApiCaseRetrieveUpdateDestroyRetrieveOptions({ client, path: { case: () => toValue(caseUuid)! }, }), enabled: () => !!toValue(caseUuid), onError: (error) => { console.error('Error retrieving case:', error) // Handle error (e.g., show user notification) }, })
- The function documentation could be more detailed. Consider adding information about the return type and possible errors:
/** * Query for a single case details. * * @param caseUuid UUID of the case to load. * @returns Query result with case details. The result includes loading state, error state, and data. * @throws Will throw an error if the API request fails. */frontend/src/cases/components/CaseListTable/CaseListTable.vue (2)
132-134
: LGTM! Consider adding a title attribute for accessibility.The addition of the index column is a good UI enhancement, providing users with a clear reference for each row. The implementation is correct, starting the count from 1 instead of 0.
Consider adding a title attribute to improve accessibility:
<template #[`item.index`]="{ index }"> - {{ index + 1 }} + <span :title="`Row ${index + 1}`">{{ index + 1 }}</span> </template>This change would provide screen readers with additional context about the number's meaning.
Line range hint
29-29
: LGTM! Consider optimizing the index column definition.The implementation of the index column is consistent with the component's structure and maintains the existing functionality. Good job on integrating the new feature seamlessly.
Consider optimizing the index column definition in the headers array:
const headers = [ - { title: '#', key: 'index', width: 50, sortable: false }, + { title: '#', key: 'index', width: 50, sortable: false, align: 'right' }, // ... other headers ]Adding
align: 'right'
to the index column header would ensure consistent alignment with other numeric columns in the table, improving the overall visual appearance.Also applies to: 115-131
backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl (2)
Line range hint
27-41
: Fix inconsistent genome build referenceThe
genomebuild
attribute in the file section is set to "grch38", but the filename and template name suggest this is for GRCh37.Update the
genomebuild
attribute:- genomebuild: grch38 + genomebuild: grch37Consider implementing or removing commented BAM file section
There's a commented-out section for BAM files. Consider either implementing this feature or removing the commented code to keep the template clean.
Line range hint
59-134
: Fix inconsistent genome build references in relatives sectionThe
genomebuild
attribute in the file sections for both mother and father is set to "grch38", but the filename and template name suggest this is for GRCh37.Update the
genomebuild
attribute for both mother and father:- genomebuild: grch38 + genomebuild: grch37Consider implementing or removing commented BAM file sections for relatives
There are commented-out sections for BAM files for both mother and father. Consider either implementing this feature or removing the commented code to keep the template clean.
backend/ingested.vcf (3)
18-48
: LGTM: FILTER, FORMAT, and contig definitions are correct.The FILTER, FORMAT, and contig definitions are well-specified and follow the VCF standard. All human chromosomes are correctly defined with their lengths for the GRCh37 assembly.
Consider updating to the GRCh38 assembly in future iterations, as it's the current reference genome with improved annotations and fewer gaps.
56-58
: VarFish-specific metadata is informative.The inclusion of a UUID for the case and version information for both VarFish and the original caller is excellent for tracking and reproducibility.
The GATK HaplotypeCaller version used (3.7-0) is quite old. Consider updating to a more recent version of GATK for improved variant calling accuracy and performance.
59-60
: Variant data is well-formatted and informative.The header line and variant data are correctly formatted according to the VCF specification. The single variant reported is well-annotated with ClinVar and functional impact information, which is crucial for interpretation.
The variant (1:145507646 G>A) is heterozygous in the father and index, but absent in the mother. This pattern, combined with the ClinVar annotation of "Conflicting classifications of pathogenicity", suggests this variant might be clinically relevant. Ensure that your variant interpretation pipeline prioritizes such variants for further investigation.
backend/seqvars/models/executors.py (1)
160-160
: Approved: Improved logging for debugging.The addition of this print statement is a good practice for enhancing the visibility of the worker's execution process. It will be helpful for debugging and monitoring purposes.
Consider using a logging framework instead of a print statement for consistency with other parts of the codebase and better control over log levels. For example:
import logging logger = logging.getLogger(__name__) # In the run_worker method logger.debug(f"Running worker with command: {' '.join(cmd)}")This approach allows for more flexible log management and is consistent with Python best practices for logging.
backend/varfish/users/management/commands/initdev.py (1)
66-66
: LGTM! Consider enhancing the help text for clarity.The addition of the "Case_1_exons" option to the
--data-case
argument choices is a good improvement, allowing for more flexibility in data selection during development initialization.To improve clarity, consider updating the help text for this argument to briefly explain the difference between the two options. For example:
help="Name of case to use when adding data or import job. 'Case_1' for standard data, 'Case_1_exons' for exon-specific data."This will provide developers with more context about the available options directly in the command-line interface.
backend/seqvars/models/base.py (1)
Line range hint
2423-2489
: LGTM: Well-structured SeqvarsQueryExecutionBackgroundJob model and managerThe new
SeqvarsQueryExecutionBackgroundJob
model and its custom managerSeqvarsQueryExecutionBackgroundJobManager
are well-structured and provide a good implementation for managing background jobs for seqvars query executions. The model includes all necessary fields and relationships, and the manager'screate_full
method ensures that both the background job and the model instance are created atomically.The
Meta
class withordering = ["-pk"]
is appropriate, ensuring that the most recent jobs appear first when queried.Consider enhancing the error handling in the
create_full
method of the manager. While it correctly raises aValueError
if theSeqvarsQueryExecution
doesn't have an associated case, it might be helpful to catch and handle potential database-related exceptions as well. Here's a suggestion:@transaction.atomic def create_full(self, *, seqvarsqueryexecution: SeqvarsQueryExecution, user: User): if not seqvarsqueryexecution.case: raise ValueError("SeqvarsQueryExecution must have a case to create a background job.") case_name = seqvarsqueryexecution.case.name try: bg_job = BackgroundJob.objects.create( name=f"Import of case '{case_name}'", project=seqvarsqueryexecution.case.project, job_type=SeqvarsQueryExecutionBackgroundJob.spec_name, user=user, ) instance = super().create( project=seqvarsqueryexecution.case.project, bg_job=bg_job, seqvarsqueryexecution=seqvarsqueryexecution, ) return instance except Exception as e: # Log the error or handle it as appropriate for your application raise ValueError(f"Failed to create background job: {str(e)}") from eThis change would provide more robust error handling and potentially more informative error messages.
backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml (1)
Line range hint
1-8081
: Major API changes detected: Pagination method updated and new parameters added.The OpenAPI schema for the VarFish API has undergone significant changes:
Pagination method: Transitioned from cursor-based to page-based pagination across multiple endpoints. This affects endpoints related to case analysis, case import actions, query presets, and result sets.
New parameters: Added 'order_by' and 'order_dir' parameters to some endpoints, allowing for more flexible sorting of results.
Response schema updates: Included 'count' fields in paginated responses, providing the total number of available results.
Endpoint modifications: Various endpoints have been updated with new parameters, response structures, and data models.
These changes improve API consistency and querying flexibility. However, they may introduce breaking changes for existing clients. It's crucial to:
- Update client applications to use the new pagination method.
- Modify API calls to include new parameters where applicable.
- Adjust response handling to accommodate the updated schemas.
- Review and update any documentation or SDK that interfaces with this API.
To minimize the impact of these changes:
- Consider versioning the API (e.g., /v2/) to maintain backward compatibility.
- Provide migration guides for users of the old API version.
- Implement a deprecation period for the old endpoints if possible.
frontend/src/seqvars/queries/seqvarResultSet.ts (2)
33-33
: ExtractstaleTime
value to a constant for better maintainabilityThe
staleTime
value1000 * 5
is used in multiple places. Consider defining it as a constant to improve readability and facilitate future changes.Also applies to: 58-58
90-90
: Consider settingstaleTime
for consistency and cache controlIn
useSeqvarResultSetRetrieveQuery
, nostaleTime
is specified. For consistency with other query functions and to control caching behavior, consider adding astaleTime
value.frontend/src/seqvars/components/QueryResults/CellGeneFlags.vue (1)
7-7
: Remove unnecessary ESLint disable commentThe ESLint disable comment on line 7 may be unnecessary since the
props
variable is used in the template. Removing this comment can help keep the code clean.Apply this diff to remove the ESLint disable comment:
-// eslint-disable-next-line @typescript-eslint/no-unused-vars const props = withDefaults(
frontend/src/seqvars/components/QueryResults/QueryResults.vue (1)
41-42
: Unnecessary ESLint rule suppressionThe ESLint rule
@typescript-eslint/no-unused-vars
is being disabled on line 41, butemit
is used in the template. Consider adjusting the linter configuration to recognize thatemit
is used, or refactor the code to prevent suppression of the linting rule.frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (1)
89-92
: Avoid Committing Commented-Out CodeLines 89 to 92 contain commented-out code for
openQueryUuids
. Including commented-out code in the repository can clutter the codebase and lead to confusion. If this code is no longer needed, consider removing it. If you plan to use it later, consider adding a TODO comment or tracking it in your issue tracker.backend/seqvars/views_api.py (2)
14-14
: Confirm the Use ofPageNumberPagination
You have imported
PageNumberPagination
to switch from cursor-based pagination to page-number-based pagination. Ensure thatPageNumberPagination
meets the requirements of your API, especially concerning performance with large datasets and the user experience for navigating pages.
Line range hint
63-70
: Removeordering
Attribute inStandardPagination
Since
StandardPagination
now inherits fromPageNumberPagination
, theordering
attribute is not applicable, as it's specific toCursorPagination
. Having it may cause unexpected behavior or confusion.Apply this diff to remove the unnecessary
ordering
attribute:class StandardPagination(PageNumberPagination): """Standard cursor navigation for the API.""" page_size = 100 page_size_query_param = "page_size" max_page_size = 1000 - ordering = "-date_created"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
backend/varfish/users/management/commands/data/Case_1_exons.grch37.gatk_hc.vcf.gz
is excluded by!**/*.gz
📒 Files selected for processing (28)
- backend/Case_1_exons.ped (1 hunks)
- backend/external-copy-0.vcf (1 hunks)
- backend/ingested.vcf (1 hunks)
- backend/seqvars/migrations/0009_auto_20240905_1017.py (1 hunks)
- backend/seqvars/models/base.py (3 hunks)
- backend/seqvars/models/executors.py (1 hunks)
- backend/seqvars/urls.py (1 hunks)
- backend/seqvars/views_api.py (5 hunks)
- backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml (19 hunks)
- backend/varfish/users/management/commands/data/Case_1_exons.grch37.gatk_hc.vcf.gz.tbi (1 hunks)
- backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl (1 hunks)
- backend/varfish/users/management/commands/data/Case_1_exons.ped (1 hunks)
- backend/varfish/users/management/commands/initdev.py (1 hunks)
- frontend/ext/varfish-api/src/lib/@tanstack/vue-query.gen.ts (22 hunks)
- frontend/ext/varfish-api/src/lib/schemas.gen.ts (16 hunks)
- frontend/ext/varfish-api/src/lib/services.gen.ts (1 hunks)
- frontend/ext/varfish-api/src/lib/types.gen.ts (19 hunks)
- frontend/src/cases/components/CaseListTable/CaseListTable.vue (1 hunks)
- frontend/src/cases/queries/cases.ts (1 hunks)
- frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/CellGeneFlags.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/QueryResults.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/types.ts (1 hunks)
- frontend/src/seqvars/queries/seqvarQueryExecution.ts (6 hunks)
- frontend/src/seqvars/queries/seqvarResultRow.ts (1 hunks)
- frontend/src/seqvars/queries/seqvarResultSet.ts (1 hunks)
- frontend/src/seqvars/views/SeqvarsQuery/SeqvarsQuery.vue (4 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/varfish/users/management/commands/data/Case_1_exons.grch37.gatk_hc.vcf.gz.tbi
- backend/varfish/users/management/commands/data/Case_1_exons.ped
🧰 Additional context used
🔇 Additional comments (53)
frontend/src/seqvars/components/QueryResults/types.ts (1)
1-7
: LGTM: Well-structured interface for paginationThe
PageInfo
interface is well-designed with clear and consistent naming conventions. It provides a good foundation for handling pagination in the application.backend/Case_1_exons.ped (1)
1-3
: LGTM! The pedigree file structure is correct.The file follows the standard pedigree file format used in genetic studies. Each line correctly represents a family member with the required attributes: Family ID, Individual ID, Father ID, Mother ID, Sex, and Affected Status. The relationships and attributes are consistent across the three entries.
backend/seqvars/migrations/0009_auto_20240905_1017.py (4)
13-16
: LGTM: Ordering change for seqvarsqueryexecutionbackgroundjobThe change to order
seqvarsqueryexecutionbackgroundjob
by descending primary key (-pk
) is appropriate. This will ensure that the most recently created background jobs appear first in queries by default, which is often desirable for displaying job lists or in admin interfaces.
17-20
: LGTM: Logical ordering for seqvarsresultrowThe new ordering for
seqvarsresultrow
(["chrom_no", "pos", "ref_allele", "alt_allele"]) is well-chosen. This ordering will:
- Group variants by chromosome
- Sort variants by position within each chromosome
- Further sort by reference and alternate alleles
This is a standard and efficient way to organize genetic variant data, facilitating easier data analysis and visualization.
21-24
: LGTM: Chronological ordering for seqvarsresultsetThe change to order
seqvarsresultset
by descending date created (-date_created
) is appropriate. This ensures that:
- The most recent result sets appear first in queries by default.
- Users can easily access the latest results.
- It aligns with common user expectations of seeing the newest data first.
This ordering is particularly useful for displaying result sets in user interfaces or when retrieving the latest analysis results.
1-32
: LGTM: Well-structured migration improving model organization and data integrityThis migration file makes several beneficial changes:
- Improves ordering for three models, enhancing query efficiency and user experience.
- Restricts the
genome_release
field to valid options, ensuring data integrity.These changes will positively impact data organization, query performance, and prevent invalid data entry. The migration is well-structured and follows Django best practices.
backend/seqvars/urls.py (1)
84-87
: LGTM! Verify related code for consistency.The URL pattern change from
(?P<query>[0-9a-f-]+)
to(?P<queryexecution>[0-9a-f-]+)
looks good. This update improves the API's semantics by explicitly referring to a query execution rather than just a query.To ensure consistency across the codebase, please verify that corresponding changes have been made in the related view, serializer, and any other components that interact with this endpoint. Run the following script to check for potential inconsistencies:
This script will help identify any potential inconsistencies or areas that might need updating due to this URL pattern change.
frontend/src/cases/queries/cases.ts (1)
Line range hint
96-115
: Well-integrated with existing code structure.The new
useCaseRetrieveQuery
function is well-integrated with the existing code structure. It follows the same patterns as the other query functions in the file, using the same imports and similar configuration. This consistency makes the code more maintainable and easier to understand.frontend/src/cases/components/CaseListTable/CaseListTable.vue (1)
Line range hint
1-234
: Summary: Good enhancement to the CaseListTable componentThe changes in this file successfully add an index column to the case list table, enhancing the user interface and partially meeting the PR objectives. The implementation is correct, consistent with existing code, and follows Vue.js best practices.
Key points:
- The new index column provides a clear reference for each row in the table.
- The changes are minimal and don't affect the existing functionality.
- The implementation is well-integrated with the v-data-table-server component.
Overall, this is a solid improvement to the component that will enhance the user experience when viewing the case list.
backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl (4)
Line range hint
2-26
: LGTM: Family section structureThe structure of the
family
section is well-organized and consistent, providing a clear representation of the proband's information.
Line range hint
42-58
: LGTM: Diseases and metadata sectionsThe structure of the
diseases
andmetaData
sections is well-organized. The use of a YAML anchor for metadata promotes reusability across the document.
Line range hint
135-157
: LGTM: Pedigree sectionThe
pedigree
section is well-structured and uses template variables appropriately for customization. The family relationships are clearly defined.
Line range hint
158-171
: LGTM: Final files sectionThe final
files
section is well-structured and uses template variables appropriately for customization. Thegenomebuild
attribute is correctly set to "grch37", which is consistent with the filename.backend/ingested.vcf (1)
1-17
: LGTM: File format and INFO fields are well-defined.The VCF file format and INFO fields are correctly specified. The inclusion of gnomAD (exomes, genomes, mtDNA) and HelixMtDb data provides comprehensive allele frequency information. The ANN field for functional annotations follows the standard format.
backend/seqvars/models/base.py (2)
2376-2378
: LGTM: Appropriate ordering added to SeqvarsResultSetThe addition of the
Meta
class withordering = ["-date_created"]
is a good practice. This ensures that the most recent results are displayed first when querying SeqvarsResultSet objects.
2420-2422
: LGTM: Appropriate ordering added to SeqvarsResultRowThe addition of the
Meta
class withordering = ["chrom_no", "pos", "ref_allele", "alt_allele"]
is well-thought-out. This ordering ensures that the result rows are sorted in a genomically meaningful way: first by chromosome number, then by position, and finally by reference and alternative alleles.frontend/ext/varfish-api/src/lib/services.gen.ts (3)
Line range hint
1671-1681
: Verify impact on frontend componentsThe changes to both
seqvarsApiResultsetList
andseqvarsApiResultsetRetrieve
functions may have a broader impact on the frontend components that interact with result sets.To ensure a smooth transition, please run the following command to identify all files that import and use these functions:
#!/bin/bash # Find all files that import and use seqvarsApiResultsetList or seqvarsApiResultsetRetrieve rg "import.*\{.*(?:seqvarsApiResultsetList|seqvarsApiResultsetRetrieve).*\}.*from.*services\.gen" --type ts -A 10Review the output and update any component logic that may be affected by this change, such as how query executions are handled or how result sets are fetched and displayed.
Line range hint
1679-1681
: Update URL parameter fromquery
toqueryexecution
The URL path for
seqvarsApiResultsetRetrieve
has been updated to usequeryexecution
instead ofquery
. This change is consistent with the modification made toseqvarsApiResultsetList
.Please verify that all frontend code calling this function has been updated to provide the
queryexecution
parameter instead ofquery
. Run the following command to check for any remaining usage of the old parameter:✅ Verification successful
Verification Successful: No remaining
query
parameter usages found inseqvarsApiResultsetRetrieve
calls.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of 'query' parameter in seqvarsApiResultsetRetrieve calls rg "seqvarsApiResultsetRetrieve\s*\(\s*\{[^}]*query\s*:" --type tsLength of output: 7200
1671-1673
: Update URL parameter fromquery
toqueryexecution
The URL path for
seqvarsApiResultsetList
has been updated to usequeryexecution
instead ofquery
. This change aligns with the backend API modifications.Please ensure that all frontend code calling this function has been updated to provide the
queryexecution
parameter instead ofquery
. Run the following command to check for any remaining usage of the old parameter:frontend/ext/varfish-api/src/lib/types.gen.ts (5)
1022-1025
: LGTM: Addition ofcount
property toPaginatedSeqvarsPredefinedQueryList
The addition of the optional
count
property to thePaginatedSeqvarsPredefinedQueryList
type is a good improvement. This will allow clients to know the total number of items available, which is useful for pagination controls.
1029-1032
: LGTM: Addition ofcount
property toPaginatedSeqvarsQueryExecutionList
The change to
PaginatedSeqvarsQueryExecutionList
is consistent with the previous change, adding an optionalcount
property. This maintains consistency across paginated types.
1036-1039
: LGTM: Addition ofcount
property toPaginatedSeqvarsQueryList
The addition of the
count
property toPaginatedSeqvarsQueryList
follows the same pattern as the previous changes. This consistency is good for maintaining a uniform API structure.
1043-1046
: LGTM: Consistent addition ofcount
property to multiple paginated typesThe changes to the following types are consistent with the previous modifications:
PaginatedSeqvarsQueryPresetsClinvarList
PaginatedSeqvarsQueryPresetsColumnsList
PaginatedSeqvarsQueryPresetsConsequenceList
PaginatedSeqvarsQueryPresetsFrequencyList
PaginatedSeqvarsQueryPresetsLocusList
PaginatedSeqvarsQueryPresetsPhenotypePrioList
PaginatedSeqvarsQueryPresetsQualityList
PaginatedSeqvarsQueryPresetsSetList
PaginatedSeqvarsQueryPresetsSetVersionList
PaginatedSeqvarsQueryPresetsVariantPrioList
PaginatedSeqvarsQuerySettingsList
PaginatedSeqvarsResultRowList
PaginatedSeqvarsResultSetList
Each type now includes an optional
count
property. This consistent approach across all paginated types is excellent for maintaining a uniform API structure and improving the client's ability to handle pagination.Also applies to: 1050-1053, 1057-1060, 1064-1067, 1071-1074, 1078-1081, 1085-1088, 1092-1095, 1099-1102, 1106-1109, 1113-1116, 1120-1123, 1127-1130
1022-1130
: Summary: Improved pagination support across multiple typesThe changes in this file consistently add an optional
count
property to various paginated response types. This enhancement will allow clients to access the total count of items in a paginated list, which is crucial for implementing effective pagination controls and improving the overall user experience.The modifications are uniform across all affected types, maintaining consistency in the API structure. This update represents a positive improvement in the API's functionality without introducing any apparent issues or inconsistencies.
frontend/ext/varfish-api/src/lib/schemas.gen.ts (16)
2934-2937
: New property added to paginated response schemasA
count
property has been added to thePaginatedSeqvarsPredefinedQueryList
schema. This addition is consistent with the changes mentioned in the AI-generated summary. Thecount
property will provide the total number of items in the list, which is a useful feature for pagination.Also applies to: 2940-2942, 2946-2948
2962-2965
: Consistent addition of count propertyThe
count
property has also been added to thePaginatedSeqvarsQueryExecutionList
schema. This change is consistent with the previous addition and follows the same pattern.Also applies to: 2968-2970, 2974-2976
2990-2993
: Continued consistency in schema updatesThe
count
property has been added to thePaginatedSeqvarsQueryList
schema as well. This maintains consistency across the paginated response schemas.Also applies to: 2996-2998, 3002-3004
3018-3021
: Uniform updates to paginated schemasThe
PaginatedSeqvarsQueryPresetsClinvarList
schema has also been updated with thecount
property. This change continues the pattern of updates across paginated response schemas.Also applies to: 3024-3026, 3030-3032
3046-3049
: Consistent schema updatesThe
PaginatedSeqvarsQueryPresetsColumnsList
schema has been updated with thecount
property, maintaining consistency with the other changes.Also applies to: 3052-3054, 3058-3060
3074-3077
: Uniform addition of count propertyThe
PaginatedSeqvarsQueryPresetsConsequenceList
schema has been updated with thecount
property, following the same pattern as the previous changes.Also applies to: 3080-3082, 3086-3088
3102-3105
: Consistent schema updates continueThe
PaginatedSeqvarsQueryPresetsFrequencyList
schema has also been updated with thecount
property, maintaining the consistency of the changes.Also applies to: 3108-3110, 3114-3116
3130-3133
: Uniform updates to paginated schemasThe
PaginatedSeqvarsQueryPresetsLocusList
schema has been updated with thecount
property, following the same pattern as the previous changes.Also applies to: 3136-3138, 3142-3144
3158-3161
: Consistent addition of count propertyThe
PaginatedSeqvarsQueryPresetsPhenotypePrioList
schema has been updated with thecount
property, maintaining consistency with the other changes.Also applies to: 3164-3166, 3170-3172
3186-3189
: Uniform schema updatesThe
PaginatedSeqvarsQueryPresetsQualityList
schema has been updated with thecount
property, following the same pattern as the previous changes.Also applies to: 3192-3194, 3198-3200
3214-3217
: Consistent schema updates continueThe
PaginatedSeqvarsQueryPresetsSetList
schema has also been updated with thecount
property, maintaining the consistency of the changes.Also applies to: 3220-3222, 3226-3228
3242-3245
: Uniform updates to paginated schemasThe
PaginatedSeqvarsQueryPresetsSetVersionList
schema has been updated with thecount
property, following the same pattern as the previous changes.Also applies to: 3248-3250, 3254-3256
3270-3273
: Consistent addition of count propertyThe
PaginatedSeqvarsQueryPresetsVariantPrioList
schema has been updated with thecount
property, maintaining consistency with the other changes.Also applies to: 3276-3278, 3282-3284
3298-3301
: Uniform schema updatesThe
PaginatedSeqvarsQuerySettingsList
schema has been updated with thecount
property, following the same pattern as the previous changes.Also applies to: 3304-3306, 3310-3312
3326-3329
: Consistent schema updates continueThe
PaginatedSeqvarsResultRowList
schema has also been updated with thecount
property, maintaining the consistency of the changes.Also applies to: 3332-3334, 3338-3340
3354-3357
: Final uniform update to paginated schemasThe
PaginatedSeqvarsResultSetList
schema has been updated with thecount
property, following the same pattern as all the previous changes.These consistent updates across all paginated response schemas will improve the API's functionality by providing a standardized way to access the total count of items in each list. This information is crucial for implementing proper pagination in the frontend.
Also applies to: 3360-3362, 3366-3368
frontend/src/seqvars/queries/seqvarResultSet.ts (1)
85-87
: Avoid using@ts-ignore
and resolve the TypeScript issueThis issue also occurs here. Please address it as mentioned in the previous comment.
frontend/src/seqvars/queries/seqvarQueryExecution.ts (3)
8-8
: ImportinguseQuery
is appropriate.The addition of
useQuery
to the imports from@tanstack/vue-query
is necessary for the new query functions.
36-39
: Inclusion ofqueryExecution
parameter improves function flexibility.Adding the
queryExecution
parameter to theinvalidateSeqvarQueryExecutionKeys
function allows for selective invalidation of specific query execution caches. This change aligns the function's parameters with its internal usage and enhances its utility.
123-123
: Ensure consistent use ofclient
in API options.Adding
client
to theseqvarsApiQueryexecutionListOptions
ensures that the correct API client is used for the requests. This change enhances consistency and correctness in API calls.frontend/src/seqvars/views/SeqvarsQuery/SeqvarsQuery.vue (4)
16-16
: Import ofQueryResults
ComponentThe
QueryResults
component is correctly imported, enabling its usage within this file.
74-88
: Proper Management of Query Selection StateThe reactive references
selectedQueryUuidRef
andopenQueryUuids
, along with the computed propertyselectedQueryUuid
, are well-defined. The computed property effectively manages the selected query UUID and ensures that theopenQueryUuids
array is updated when a new query is selected.
197-198
: Binding Query Selection Props toQueryEditor
ComponentThe use of
v-model
to bindselectedQueryUuid
andopenQueryUuids
to theQueryEditor
component ensures two-way data binding and keeps the state synchronized between components.
211-231
: Integration and Conditional Rendering ofQueryResults
ComponentThe conditional rendering using
v-if
andv-else
correctly handles the loading state withv-skeleton-loader
and displays theQueryResults
component once the data is available. The bindings ofselectedQueryUuid
andopenQueryUuids
toQueryResults
ensure consistent state management across the application.frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1)
1-425
: Overall Implementation Looks GoodThe component is well-structured and follows Vue and TypeScript best practices. Reactive properties, computed properties, and watchers are used appropriately. The template is logically organized, and the use of slots and teleports enhances flexibility.
backend/seqvars/views_api.py (3)
8-8
: Import Statement Update is CorrectThe import of
OpenApiParameter
fromdrf_spectacular.utils
is appropriate, as it's used later for enhancing the API schema documentation.
92-96
: Project Retrieval Logic forqueryexecution
is SoundThe added condition in the
get_project
function correctly handles the retrieval of the project associated with aqueryexecution
UUID. This enhances the function's capability to determine the project context in more scenarios.
590-590
: Filtering Adjustment inSeqvarsResultSetViewSet
is AppropriateChanging the filter to use
queryexecution__sodar_uuid
aligns with the updated data model and simplifies the query. This should efficiently retrieve the relevant result sets based on the providedqueryexecution
UUID.frontend/ext/varfish-api/src/lib/@tanstack/vue-query.gen.ts (1)
68-71
: Verify the necessity of commenting out the body parameterThe
body
parameter merging logic in the infinite query options has been commented out. Since you're transitioning from cursor-based to page-based pagination, please confirm that the API endpoints function correctly without thebody
parameter and that all necessary data is passed via query parameters.
backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl
Show resolved
Hide resolved
frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue
Outdated
Show resolved
Hide resolved
82b3f47
to
cd58339
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (17)
backend/ext_gestaltmatcher/migrations/0003_auto_20241009_0639.py (1)
1-27
: Summary: Well-structured migration for updating primary key fields.This migration file makes appropriate changes to the
id
fields of bothsmallvariantquerygestaltmatcherscores
andsmallvariantquerypediascores
models, converting them toBigAutoField
. This change is beneficial for:
- Future-proofing the database schema for large datasets.
- Maintaining consistency across models.
- Following Django best practices for primary key fields.
Consider the following architectural implications:
- Ensure that any foreign key references to these models are updated accordingly.
- If you're using any custom queries or ORM operations that explicitly reference the
id
field, verify that they still work as expected withBigAutoField
.- If you have any serialization or deserialization processes (e.g., in APIs or data import/export), make sure they can handle the potentially larger
id
values.These changes lay a good foundation for scalability, but remember to test thoroughly, especially if you have a significant amount of existing data or complex relationships between models.
backend/seqvars/migrations/0010_auto_20240905_1017.py (1)
25-31
: LGTM with suggestion: Consider using an enumeration for genome releases.The change to
genome_release
field is good. Using a CharField with choices correctly restricts the values to valid genome releases (GRCh37 and GRCh38). The max_length of 32 is sufficient for current and potential future genome releases.Suggestion for future improvement: Consider using Django's
TextChoices
or a custom enumeration for the genome releases. This would provide better type safety and code organization. For example:from django.db import models class GenomeRelease(models.TextChoices): GRCH37 = "grch37", "GRCh37" GRCH38 = "grch38", "GRCh38" class SeqVarsResultRow(models.Model): genome_release = models.CharField( max_length=32, choices=GenomeRelease.choices, )This approach would make it easier to reference specific genome releases in your code and would be more maintainable if you need to add more releases in the future.
frontend/src/seqvars/queries/seqvarResultRow.ts (1)
1-123
: Overall: Well-implemented query functions with minor improvements needed.This file provides a comprehensive set of query functions for seqvar result sets using TanStack Query and Vue.js. The implementation is solid, with well-structured functions and appropriate use of hooks.
To further improve the code:
- Address the TypeScript issues throughout the file. Resolving these will enhance type safety and reduce reliance on
@ts-ignore
comments.- Consider adding more detailed error handling or logging for potential edge cases.
- If not already present elsewhere, consider adding unit tests for these functions to ensure their reliability.
Great job on implementing these complex query functions!
frontend/src/seqvars/queries/seqvarQueryExecution.ts (1)
158-163
: LGTM: Consistent changes in commented-out function.The changes in the commented-out
useSeqvarQueryExecutionRetrieveQuery
function maintain consistency with other functions in the file:
- Adding
client
toseqvarsApiQueryexecutionRetrieveOptions
is consistent with other API call handling.- The
@ts-ignore
comments are explained and acceptable based on the project's conventions.Note: Since this function is commented out and currently unused, consider removing it if it's no longer needed to keep the codebase clean.
frontend/src/seqvars/components/QueryResults/CellGeneFlags.vue (3)
1-32
: Consider removing unnecessary eslint-disable comment and add type annotationThe script setup looks good overall. However, there are two minor suggestions:
The eslint-disable comment on line 11 appears unnecessary. If it's not needed, consider removing it to maintain clean code.
Add a return type annotation to the
moiIncludes
function for better type safety and readability.Here's a suggested improvement for the
moiIncludes
function:const moiIncludes = ( item: SeqvarsResultRow, moiType: SeqvarsModeOfInheritance ): boolean => { return ( item?.payload?.variant_annotation?.gene?.phenotypes?.mode_of_inheritances ?? [] ).includes(moiType) }
35-69
: Fix typo in hint messageThere's a minor typo in the hint message for the ACMG Supplementary Findings List.
Please apply this change:
- ? 'PRECENSE of gene on the ACMG Supplementary Findings List' + ? 'PRESENCE of gene on the ACMG Supplementary Findings List'
120-161
: Consider updating chip labels for X-linked inheritanceThe hint messages correctly specify "X-LINKED DOMINANT" and "X-LINKED RECESSIVE", but the chip labels use "AD" and "AR". This might be confusing for users.
Consider updating the chip labels to "XD" and "XR" for X-linked Dominant and X-linked Recessive, respectively, to maintain consistency with the hint messages:
- AD + XD- AR + XRbackend/protos/seqvars/protos/output.proto (2)
169-185
: LGTM! Consider adding a reference to HPO in the enum description.The
ModeOfInheritance
enum is well-structured and comprehensive, covering the main modes of inheritance. The use of HPO terms in the comments is helpful.Consider adding a reference to the Human Phenotype Ontology (HPO) in the enum description for better context. For example:
- // Enumerations with modes of inheritance from HPO. + // Enumerations with modes of inheritance based on the Human Phenotype Ontology (HPO). + // For more information, see: https://hpo.jax.org/app/browse/term/HP:0000005
193-194
: LGTM! Consider enhancing the field comment.The addition of the
mode_of_inheritances
field to theGeneRelatedPhenotypes
message is well-implemented. It allows for the representation of multiple inheritance modes for a gene's phenotype, which is biologically relevant.Consider enhancing the comment for the new field to provide more context. For example:
- // Linked modes of inheritance. + // Associated modes of inheritance for the gene's phenotypes. + // Multiple modes can be present as a gene may be involved in various inheritance patterns. repeated ModeOfInheritance mode_of_inheritances = 3;frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1)
16-47
: Consider removing unnecessary eslint-disable commentThe props and emits definitions are well-structured and properly typed. However, there's an eslint-disable comment on line 44 that might be unnecessary:
// eslint-disable-next-line @typescript-eslint/no-unused-vars
If this comment is no longer needed, consider removing it to maintain clean code. If it's still required, please add a brief explanation of why it's necessary to suppress this specific eslint rule.
backend/seqvars/protos/output_pb2.py (1)
27-27
: Consider updating the equality comparison withFalse
.The static analysis tool suggests replacing the equality comparison with
False
to improve code readability and follow best practices.Consider updating the condition as follows:
-if _descriptor._USE_C_DESCRIPTORS == False: +if not _descriptor._USE_C_DESCRIPTORS:This change makes the code more idiomatic and easier to read.
🧰 Tools
🪛 Ruff
27-27: Avoid equality comparisons to
False
; useif not _descriptor._USE_C_DESCRIPTORS:
for false checksReplace with
not _descriptor._USE_C_DESCRIPTORS
(E712)
backend/seqvars/models/protobufs.py (2)
777-786
: LGTM! Consider adding a type hint for better clarity.The
MODE_OF_INHERITANCE_MAPPING
is well-structured and comprehensive. It correctly maps the protobufModeOfInheritance.ValueType
toSeqvarsModeOfInheritance
.Consider adding a type hint to improve code readability:
-MODE_OF_INHERITANCE_MAPPING: dict[ +MODE_OF_INHERITANCE_MAPPING: dict[ModeOfInheritance.ValueType, SeqvarsModeOfInheritance] = {
789-792
: LGTM! Consider adding a docstring for better documentation.The
_mode_of_inheritance_from_protobuf
function is well-implemented and correctly uses theMODE_OF_INHERITANCE_MAPPING
.Consider adding a docstring to improve code documentation:
def _mode_of_inheritance_from_protobuf( mode_of_inheritance: ModeOfInheritance.ValueType, ) -> SeqvarsModeOfInheritance: + """ + Convert a protobuf ModeOfInheritance value to a SeqvarsModeOfInheritance value. + + Args: + mode_of_inheritance (ModeOfInheritance.ValueType): The protobuf mode of inheritance value. + + Returns: + SeqvarsModeOfInheritance: The corresponding SeqvarsModeOfInheritance value. + """ return MODE_OF_INHERITANCE_MAPPING[mode_of_inheritance]backend/seqvars/tests/snapshots/snap_test_views_api.py (1)
10-10
: LGTM! Consider updating API documentation.The addition of the "count" field is a good improvement to the API response structure. It correctly reflects the number of items in the "results" array.
Please ensure that the API documentation is updated to reflect this new field in the response structure.
backend/seqvars/tests/test_permissions_api.py (1)
Line range hint
1-2357
: Summary of changes and suggestions for further reviewThe changes made to the TestResultSetViewSet class in this file are consistent and improve the representation of the API structure by updating the URL kwargs from 'query' to 'queryexecution'. These modifications better reflect the relationship between queries, their executions, and the resulting sets.
To ensure consistency across the entire codebase:
- Review other test files and API-related code for similar 'query' to 'queryexecution' updates.
- Check the TestResultRowViewSet class and other related classes for potential updates to maintain consistency with the new API structure.
- Update any documentation or comments that may reference the old URL structure.
To facilitate a broader review, consider running the following command to identify other files that may need similar updates:
#!/bin/bash # Search for files containing 'query' in URL patterns or API-related code rg --type python 'reverse\(.*"query":' -lThis will help identify other areas of the codebase that may need to be updated to maintain consistency with the new API structure.
frontend/ext/varfish-api/src/lib/schemas.gen.ts (1)
Line range hint
1-11731
: Overall assessment of schema changesThe changes to this auto-generated schema file appear to be part of a larger update to the API. New properties and types have been added, particularly around gene-related annotations, mode of inheritance, and paginated responses. These changes seem intentional and consistent with the existing code structure.
However, as this is an auto-generated file, it's crucial to:
- Verify that these changes are correctly reflected in the source that generates this file.
- Ensure that the frontend code using these schemas has been updated accordingly.
- Update any relevant documentation to reflect these new properties and types.
While the changes look good, I recommend a thorough testing of the API endpoints that use these modified schemas to ensure everything works as expected.
backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl (1)
Line range hint
17-22
: Inconsistent 'genomebuild' attribute value in 'fileAttributes'.The file is named
Case_1_exons.grch37.yaml.tpl
, indicating that the genome build is GRCh37. However, within thefileAttributes
sections, thegenomebuild
is set togrch38
. This inconsistency could lead to errors during data processing or analysis.Apply the following diff to correct the genome build version:
# In all 'fileAttributes' sections where 'genomebuild' is defined - genomebuild: grch38 + genomebuild: grch37Also applies to: 66-71, 115-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
backend/varfish/users/management/commands/data/Case_1_exons.grch37.gatk_hc.vcf.gz
is excluded by!**/*.gz
📒 Files selected for processing (37)
- backend/cases_import/tests/snapshots/snap_test_models_executor.py (1 hunks)
- backend/ext_gestaltmatcher/migrations/0003_auto_20241009_0639.py (1 hunks)
- backend/protos/seqvars/protos/output.proto (1 hunks)
- backend/seqvars/migrations/0010_auto_20240905_1017.py (1 hunks)
- backend/seqvars/models/base.py (5 hunks)
- backend/seqvars/models/protobufs.py (3 hunks)
- backend/seqvars/protos/output_pb2.py (2 hunks)
- backend/seqvars/protos/output_pb2.pyi (2 hunks)
- backend/seqvars/tasks.py (1 hunks)
- backend/seqvars/tests/snapshots/snap_test_views_api.py (1 hunks)
- backend/seqvars/tests/test_permissions_api.py (2 hunks)
- backend/seqvars/tests/test_views_api.py (21 hunks)
- backend/seqvars/urls.py (1 hunks)
- backend/seqvars/views_api.py (6 hunks)
- backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml (22 hunks)
- backend/varfish/users/management/commands/data/Case_1_exons.grch37.gatk_hc.vcf.gz.tbi (1 hunks)
- backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl (1 hunks)
- backend/varfish/users/management/commands/data/Case_1_exons.ped (1 hunks)
- backend/varfish/users/management/commands/initdev.py (1 hunks)
- frontend/ext/varfish-api/src/lib/@tanstack/vue-query.gen.ts (24 hunks)
- frontend/ext/varfish-api/src/lib/schemas.gen.ts (20 hunks)
- frontend/ext/varfish-api/src/lib/services.gen.ts (3 hunks)
- frontend/ext/varfish-api/src/lib/types.gen.ts (25 hunks)
- frontend/src/cases/components/CaseListTable/CaseListTable.vue (1 hunks)
- frontend/src/cases/queries/cases.ts (1 hunks)
- frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (3 hunks)
- frontend/src/seqvars/components/QueryResults/CellGeneFlags.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/ClingenDosage.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/QueryResults.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1 hunks)
- frontend/src/seqvars/components/QueryResults/types.ts (1 hunks)
- frontend/src/seqvars/queries/seqvarQuery.ts (1 hunks)
- frontend/src/seqvars/queries/seqvarQueryExecution.ts (6 hunks)
- frontend/src/seqvars/queries/seqvarResultRow.ts (1 hunks)
- frontend/src/seqvars/queries/seqvarResultSet.ts (1 hunks)
- frontend/src/seqvars/views/SeqvarsQuery/SeqvarsQuery.vue (4 hunks)
- utils/docker/Dockerfile (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/ext/varfish-api/src/lib/services.gen.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/seqvars/urls.py
- backend/varfish/users/management/commands/data/Case_1_exons.grch37.gatk_hc.vcf.gz.tbi
- backend/varfish/users/management/commands/data/Case_1_exons.ped
- backend/varfish/users/management/commands/initdev.py
- frontend/src/cases/components/CaseListTable/CaseListTable.vue
- frontend/src/cases/queries/cases.ts
- frontend/src/seqvars/components/QueryResults/types.ts
- frontend/src/seqvars/queries/seqvarResultSet.ts
- frontend/src/seqvars/views/SeqvarsQuery/SeqvarsQuery.vue
🧰 Additional context used
📓 Learnings (5)
backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl (1)
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: backend/varfish/users/management/commands/data/Case_1_exons.grch37.yaml.tpl:1-1 Timestamp: 2024-10-09T06:43:00.914Z Learning: In the VarFish server repository, files that are symlinks committed into git may appear in the diff with their symlink target as added content, which is not part of the actual file content.
frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (2)
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/components/QueryEditor/QueryEditor.vue:88-88 Timestamp: 2024-10-09T06:55:01.823Z Learning: In Vue.js components, the `defineModel` macro is a compiler macro and does not need to be imported from 'vue'.
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/views/SeqvarsQuery/SeqvarsQuery.vue:74-87 Timestamp: 2024-10-10T10:50:55.834Z Learning: Currently, it's not necessary to implement removal logic for `openQueryUuids` in `frontend/src/seqvars/views/SeqvarsQuery/SeqvarsQuery.vue`.
frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1)
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue:319-329 Timestamp: 2024-10-10T10:46:20.777Z Learning: In `frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue`, when watching `resultRowListRes.isError`, it's correct to use `(value: Ref<boolean>)` as the callback parameter type in the `watch` function.
frontend/src/seqvars/queries/seqvarQueryExecution.ts (2)
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/queries/seqvarQueryExecution.ts:84-107 Timestamp: 2024-10-09T06:55:05.311Z Learning: In the function `useSeqvarQueryExecutionListQuery` within `frontend/src/seqvars/queries/seqvarQueryExecution.ts`, the `@ts-ignore` comment is necessary because the composable function might be used in different contexts, and removing it may not be appropriate.
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/queries/seqvarResultSet.ts:28-30 Timestamp: 2024-10-09T06:43:43.012Z Learning: In TypeScript files, when `@ts-ignore` is used due to dependencies on upstream changes, and a comment is included explaining this, it's acceptable.
frontend/src/seqvars/queries/seqvarResultRow.ts (1)
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/queries/seqvarResultRow.ts:110-112 Timestamp: 2024-10-09T06:48:09.413Z Learning: In `frontend/src/seqvars/queries/seqvarResultRow.ts`, within the `useSeqvarResultRowRetrieveQueries` function, using `toValue(resultRowUuids)?.length` in the `enabled` function is necessary for transparent access into refs or getters.
🪛 Ruff
backend/seqvars/protos/output_pb2.py
27-27: Avoid equality comparisons to
False
; useif not _descriptor._USE_C_DESCRIPTORS:
for false checksReplace with
not _descriptor._USE_C_DESCRIPTORS
(E712)
🪛 Biome
frontend/ext/varfish-api/src/lib/types.gen.ts
[error] 3476-3476: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (97)
backend/seqvars/tasks.py (1)
2-10
: LGTM! Approve the refactoring changes.The changes look good and improve the code organization:
- The import statement now correctly reflects the new location of the
run_seqvarsqueryexecutionbackgroundjob
function inseqvars.models.executors
, which is more appropriate for sequence variant related functionality.- The task function maintains its signature, ensuring backward compatibility.
- The function body now delegates the execution to the imported module, which is a good practice for separation of concerns.
To ensure a smooth transition, let's verify that there are no remaining references to the old import path:
✅ Verification successful
Import path update verified successfully.
- No remaining references to
cases_import.models.run_seqvarsqueryexecutionbackgroundjob
.- All references now use
seqvars.models.executors.run_seqvarsqueryexecutionbackgroundjob
inbackend/seqvars/tasks.py
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old import path # Test: Search for references to 'cases_import.models' related to 'run_seqvarsqueryexecutionbackgroundjob' # Expect: No results, as all references should have been updated rg --type python 'from cases_import.models import.*run_seqvarsqueryexecutionbackgroundjob' rg --type python 'cases_import.models.*run_seqvarsqueryexecutionbackgroundjob' # Test: Confirm the new import is used consistently # Expect: References to the new import path rg --type python 'from seqvars.models import executors' rg --type python 'executors.run_seqvarsqueryexecutionbackgroundjob'Length of output: 423
Script:
#!/bin/bash # Description: Check for any remaining references to the old import path with corrected file type # Test: Search for references to 'cases_import.models' related to 'run_seqvarsqueryexecutionbackgroundjob' # Expect: No results, as all references should have been updated rg --type py 'from cases_import.models import.*run_seqvarsqueryexecutionbackgroundjob' rg --type py 'cases_import.models.*run_seqvarsqueryexecutionbackgroundjob' # Test: Confirm the new import is used consistently # Expect: References to the new import path rg --type py 'from seqvars.models import executors' rg --type py 'executors.run_seqvarsqueryexecutionbackgroundjob'Length of output: 431
backend/ext_gestaltmatcher/migrations/0003_auto_20241009_0639.py (3)
1-10
: LGTM: Migration class and dependencies are correctly defined.The migration file is properly generated by Django 3.2.25, and the dependency on the previous migration "0002_smallvariantquerypediascores" is correctly specified. This ensures that migrations are applied in the correct order, maintaining database schema consistency.
20-26
: LGTM: Consistent change toBigAutoField
forsmallvariantquerypediascores
.The modification of the
id
field toBigAutoField
forsmallvariantquerypediascores
is consistent with the previous change and follows good practices for handling potentially large datasets.Please verify that this change doesn't affect existing data. Run the following script to check for any potential issues:
This script will help you ensure that the change to
BigAutoField
doesn't cause any conflicts with existing data.
13-19
: LGTM: Appropriate change toBigAutoField
forsmallvariantquerygestaltmatcherscores
.The modification of the
id
field toBigAutoField
is a good practice, especially if you anticipate a large number of records. This change ensures that you won't run out of unique identifiers in the future.Please verify that this change doesn't affect existing data. Run the following script to check for any potential issues:
This script will help you ensure that the change to
BigAutoField
doesn't cause any conflicts with existing data.backend/seqvars/migrations/0010_auto_20240905_1017.py (4)
13-16
: LGTM: Appropriate ordering for background jobs.The change to order
seqvarsqueryexecutionbackgroundjob
by-pk
is a good practice. It ensures that the most recent jobs (assuming pk correlates with creation time) appear first in querysets, which is typically desired for background job models.
17-20
: Excellent: Logical ordering for genetic variant data.The ordering ["chrom_no", "pos", "ref_allele", "alt_allele"] for
seqvarsresultrow
is ideal for genetic variant data. It ensures that results are consistently sorted by chromosome number, position, and then alleles, which is crucial for accurate analysis and presentation of genetic data.
21-24
: LGTM: Appropriate ordering for result sets.Ordering
seqvarsresultset
by-date_created
is a good choice. It ensures that the most recent result sets appear first in querysets, which is typically what users expect when viewing sets of results.
1-32
: Overall: Well-structured migration with logical changes.This migration file makes several important improvements to the model structure:
- Appropriate ordering for background jobs, result rows, and result sets.
- Proper definition of the genome_release field with valid choices.
These changes will enhance data organization and query efficiency. The migration is well-structured and follows Django best practices. Consider the suggestion about using enumerations for genome releases in future development to further improve code quality and maintainability.
frontend/src/seqvars/queries/seqvarResultRow.ts (3)
1-14
: LGTM: Imports and type definition are appropriate.The imports from '@tanstack/vue-query' and '@varfish-org/varfish-api' are correctly used, and the custom type 'OrderDir' is well-defined for its purpose.
86-123
: LGTM: Function implementation is correct.The
useSeqvarResultRowRetrieveQueries
function is well-implemented:
- The use of
useQueries
is appropriate for handling multiple queries.- The
enabled
condition correctly usestoValue(resultRowUuids)?.length
for transparent access into refs or getters.- The
combine
function effectively filters out undefined data and tracks the pending state.Good job on implementing this complex query function!
56-84
:⚠️ Potential issueAddress TypeScript issues and correct parameter description.
The
@ts-ignore
comments indicate underlying type issues. Consider resolving these to maintain type safety:
- Lines 76-79: Investigate why TypeScript is flagging errors for the
resultset
andseqvarresultrow
paths.In the function's JSDoc comment, correct the parameter description for
resultRowUuid
:
- Current: "UUID of the seqvar result row to load."
- Suggested: "UUID of the specific seqvar result row to retrieve."
This change will provide a more precise description of the parameter's purpose.
frontend/src/seqvars/queries/seqvarQueryExecution.ts (4)
8-8
: LGTM: New import added for enhanced functionality.The addition of
useQuery
import from '@tanstack/vue-query' is appropriate for the newuseSeqvarQueryExecutionListQuery
function introduced in this file.
36-39
: LGTM: Improved naming consistency.The parameter name change from
queryexecution
toqueryExecution
and the corresponding type definition update enhance code consistency and adhere to camelCase naming conventions.
84-107
: LGTM: New query function implemented correctly.The new
useSeqvarQueryExecutionListQuery
function is well-implemented:
- It correctly uses
useQuery
for fetching seqvar query executions.- The
enabled
option ensures the query only runs whenseqvarQueryUuid
is available.- The
staleTime
setting helps in managing cache invalidation.Note: The
@ts-ignore
comment is present and necessary due to the composable function potentially being used in different contexts, as per the previous discussion.
Line range hint
111-123
: LGTM: Improved documentation and consistent client usage.The changes in
useSeqvarQueryExecutionListQueries
function are appropriate:
- The updated documentation provides better clarity on the function's purpose and parameters.
- Adding
client
toseqvarsApiQueryexecutionListOptions
ensures consistent API call handling across the module.backend/cases_import/tests/snapshots/snap_test_models_executor.py (4)
36-65
: Well-structured snapshot for external files in sequence variant tests.The snapshot for
ImportCreateWithSeqvarsVcfTest::test_run external files
is well-organized and contains all necessary information for testing. It correctly includes both the VCF file and its index, with appropriate file attributes, checksums, and identifier mappings. The use of thefile://
scheme for paths is suitable for local file testing.
100-129
: Consistent snapshot structure for structural variant external files.The snapshot for
ImportCreateWithStrucvarsVcfTest::test_run external files
maintains the same well-organized structure as the sequence variant snapshot. The only difference is the correct use of"variant_type": "strucvars"
, which appropriately distinguishes it from the sequence variant test. This consistency in structure between different variant types is a good practice and suggests effective code reuse.
131-162
: Consistent snapshot structure for structural variant internal files.The snapshot for
ImportCreateWithStrucvarsVcfTest::test_run internal files
maintains the same well-organized structure as the sequence variant internal files snapshot. The correct use of"variant_type": "strucvars"
appropriately distinguishes it from the sequence variant test.As mentioned in the previous comment for sequence variants, please verify:
- The intentional use of
None
for checksums.- The expected empty
file_attributes
andidentifier_map
for ingested files.This consistency in structure between different variant types is a good practice and suggests effective code reuse.
35-162
: Summary: Well-structured and consistent snapshots for variant import testsThe new snapshots for both
ImportCreateWithSeqvarsVcfTest
andImportCreateWithStrucvarsVcfTest
are well-structured and consistent. They appropriately capture the expected state for external and internal files in the import process. The use of UUIDs for internal file paths and the consistent structure between sequence and structural variant tests are commendable.Key points:
- Excellent consistency between sequence and structural variant test snapshots.
- Appropriate use of
variant_type
to distinguish between test cases.- Well-organized file attributes and paths for both external and internal files.
As mentioned in previous comments, please verify the intentional use of
None
for checksums and empty attributes for ingested files in both test cases.Overall, these snapshots provide a solid foundation for testing the variant import functionality.
frontend/src/seqvars/components/QueryResults/CellGeneFlags.vue (1)
1-228
: Overall, well-implemented component with minor improvements suggestedThe
CellGeneFlags
component is well-structured and effectively displays genetic variant information. It makes good use of Vue 3 features and follows best practices for conditional rendering and prop management. The use ofAbbrHint
components for tooltips enhances user experience.A few minor improvements have been suggested in previous comments, including:
- Adding a type annotation to the
moiIncludes
function- Fixing typos in hint messages
- Updating chip labels for consistency
- Considering an alternative approach for fallback rendering
Implementing these suggestions will further improve the component's quality and maintainability.
backend/protos/seqvars/protos/output.proto (1)
Line range hint
1-194
: Overall, the changes enhance the protocol buffer definition.The addition of the
ModeOfInheritance
enum and themode_of_inheritances
field to theGeneRelatedPhenotypes
message are well-implemented. These changes improve the representation of genetic inheritance patterns in the protocol buffer, which is crucial for accurate genetic data handling.The suggestions provided are minor and focus on improving documentation. Implementation of these suggestions would further enhance the clarity and maintainability of the code.
frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (3)
1-15
: LGTM: Imports and component setupThe import statements and component setup are well-organized and appropriate for the component's functionality. The use of TypeScript and the Composition API is a good choice for maintaining type safety and code organization.
318-328
: Correct implementation of watcher callbackThe watcher for error handling is correctly implemented. As per the previous learning, the use of
(value: Ref<boolean>)
as the callback parameter type in thewatch
function is correct for this specific use case.watch( () => resultRowListRes.isError, (value: Ref<boolean>) => { if (value.value) { // ... error handling logic } }, )This implementation aligns with the expected behavior and maintains type safety.
1-430
: Overall: Well-implemented and feature-rich componentThis
QueryResultsTable
component is well-structured and effectively implements a complex data table for displaying query results. Key strengths include:
- Proper use of TypeScript and the Composition API for type safety and code organization.
- Dynamic header generation based on the case object, providing flexibility.
- Efficient data fetching with pagination, sorting, and debounced updates.
- Custom cell renderings for complex data presentation.
- Flexible pagination controls with teleport functionality.
While there are a few minor areas for potential improvement (as noted in previous comments), the overall implementation is solid and should serve its purpose well.
backend/seqvars/protos/output_pb2.py (3)
Line range hint
21-98
: Changes in serialized positions are consistent with the summary.The updates to the serialized start and end positions for various message types and enums are consistent with the AI-generated summary. These changes reflect a reorganization of the generated code structure, which is expected when the underlying
.proto
file is modified.
33-34
: New enumModeOfInheritance
added successfully.The new enum
ModeOfInheritance
has been properly integrated into the generated code. This addition aligns with the AI-generated summary and expands the capabilities of the protocol buffer definition.
Line range hint
1-98
: Overall, the changes in this generated file look good.The updates to the serialized positions and the addition of the new
ModeOfInheritance
enum are consistent with the expected changes. The file appears to be correctly generated based on modifications to the underlying.proto
file. The only suggestion is a minor readability improvement for the condition checking_USE_C_DESCRIPTORS
.frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (7)
62-63
: New propopenQueryUuids
addedThe addition of the
openQueryUuids
prop allows the component to track currently opened queries. This can be useful for managing the state of multiple queries across the application.Also applies to: 76-76
91-91
: Improved two-way binding forselectedQueryUuid
The change from a reactive reference to
defineModel
forselectedQueryUuid
enhances the two-way binding capability of the component. This allows for better synchronization of the selected query UUID between parent and child components.
Line range hint
171-180
: Improved label generation increateQuery
functionThe label generation mechanism in the
createQuery
function has been optimized. The new implementation prevents duplicate labels more efficiently by using a single loop and incrementing the counter outside the loop. This change improves the performance and reliability of creating new queries.
Line range hint
345-376
: RefactoredrevertQueryToPresets
function for better maintainabilityThe
revertQueryToPresets
function has been refactored to use helper functionsqueryWithGenotypePresets
andqueryWithGroupPresets
. This change improves code readability and maintainability by breaking down the logic into smaller, reusable functions. It also ensures consistent handling of different types of presets.
Line range hint
395-408
: SimplifiedupdateGenotypeToPresets
functionThe
updateGenotypeToPresets
function has been simplified and now uses thequeryWithGenotypePresets
helper function. This change improves readability and ensures consistent handling of genotype presets across the component. The function is now more focused and easier to maintain.
Line range hint
458-463
: Refined event handling in QueryList componentThe event handling for updating the selected query UUID has been improved. The new implementation uses an inline arrow function, which is more concise and easier to read. This change enhances the maintainability of the component.
Line range hint
1-738
: Overall improvement in code quality and maintainabilityThe changes made to this component demonstrate a significant improvement in code quality, maintainability, and performance. Key improvements include:
- Better state management with the use of
defineModel
forselectedQueryUuid
.- Improved query creation and preset reversion logic with the use of helper functions.
- Refined event handling in the template section.
- Addition of the
openQueryUuids
prop for better query state tracking.These changes contribute to a more robust and efficient implementation of the QueryEditor component.
backend/seqvars/models/protobufs.py (1)
794-801
: LGTM! The changes integrate well with the existing code.The update to
_phenotypes_from_protobuf
function correctly incorporates the new mode of inheritance functionality. The use ofmap()
with the new_mode_of_inheritance_from_protobuf
function is an efficient way to convert the list of mode_of_inheritances.backend/seqvars/tests/snapshots/snap_test_views_api.py (2)
Line range hint
40-359
: Unchanged snapshot looks good.The "TestQueryPresetsFactoryDefaultsViewSet::test_retrieve 1" snapshot remains unchanged, which is expected as the modification was only for the list view. The detailed structure of the preset provides comprehensive coverage for testing the retrieve endpoint.
Line range hint
1-359
: Summary: API response structure improved with count fieldThe changes in this file enhance the API response structure for the list view by adding a "count" field. This improvement likely supports pagination or provides better metadata about the response. The retrieve view snapshot remains unchanged, maintaining its comprehensive test coverage.
Key points:
- The new "count" field accurately reflects the number of items in the results.
- The change is isolated to the list view, as expected.
- The detailed structure of the retrieve view continues to provide thorough test coverage.
These changes appear to be part of a larger effort to improve the API's functionality and should integrate well with the existing codebase.
backend/seqvars/tests/test_permissions_api.py (3)
2284-2287
: LGTM: Updated URL kwargs in test_retrieve methodThe change from 'query' to 'queryexecution' in the URL kwargs for the test_retrieve method is consistent with the update in the test_list method. This ensures that the test suite accurately reflects the new API structure.
Line range hint
2314-2357
: Consider reviewing TestResultRowViewSet for consistencyWhile no changes were made to the TestResultRowViewSet class in this diff, it's worth reviewing to ensure consistency with the updates made to the TestResultSetViewSet class. Specifically, check if any URL kwargs need to be updated from 'query' to 'queryexecution' or if any other adjustments are necessary to align with the new API structure.
Let's check if there are any occurrences of 'query' in the URL kwargs for this class:
#!/bin/bash # Search for 'query' in URL kwargs within the TestResultRowViewSet class rg --type python 'class TestResultRowViewSet.*?kwargs=\{.*"query":' -U
2264-2266
: LGTM: Updated URL kwargs in test_list methodThe change from 'query' to 'queryexecution' in the URL kwargs is consistent with the updated API structure. This modification better represents the relationship between queries, their executions, and the resulting sets.
To ensure consistency across the codebase, let's check if similar changes are needed elsewhere:
✅ Verification successful
LGTM: No additional occurrences of 'query' found in URL kwargs.
All relevant URL kwargs have been updated consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of 'query' in URL kwargs that might need updating rg --type python 'kwargs=\{.*"query":' -g '!test_permissions_api.py'Length of output: 197
Script:
#!/bin/bash # Search for other occurrences of 'query' in URL kwargs that might need updating rg 'kwargs\s*=\s*\{.*"query":' -g '*.py' -g '!backend/seqvars/tests/test_permissions_api.py'Length of output: 94
backend/seqvars/models/base.py (6)
2058-2067
: New enumSeqvarsModeOfInheritance
addedThis new enum represents different modes of inheritance for gene annotations. It includes common inheritance patterns such as autosomal dominant, autosomal recessive, X-linked dominant, X-linked recessive, Y-linked, and mitochondrial.
The enum is well-defined and covers the main inheritance patterns. This addition will improve the clarity and consistency of inheritance mode representation in the codebase.
2068-2077
: UpdatedGeneRelatedPhenotypesPydantic
classThe
GeneRelatedPhenotypesPydantic
class has been updated to include a new field:mode_of_inheritances: list[SeqvarsModeOfInheritance] = []This change allows for storing multiple modes of inheritance for a gene, which is appropriate as some genes can be associated with multiple inheritance patterns. The use of the newly defined
SeqvarsModeOfInheritance
enum ensures consistency in the data representation.
2389-2391
: Added ordering toSeqvarsResultSet
modelA
Meta
class has been added to theSeqvarsResultSet
model:class Meta: ordering = ["-date_created"]This change ensures that
SeqvarsResultSet
instances will be ordered by their creation date in descending order by default. This is a good practice for maintaining a consistent order and typically showing the most recent results first.
2433-2435
: Added ordering toSeqvarsResultRow
modelA
Meta
class has been added to theSeqvarsResultRow
model:class Meta: ordering = ["chrom_no", "pos", "ref_allele", "alt_allele"]This ordering ensures that
SeqvarsResultRow
instances will be sorted by chromosome number, position, reference allele, and alternative allele. This is a logical order for genomic variants and will help in presenting the data in a consistent and meaningful way.
2501-2502
: Added ordering toSeqvarsQueryExecutionBackgroundJob
modelA
Meta
class has been added to theSeqvarsQueryExecutionBackgroundJob
model:class Meta: ordering = ["-pk"]This ordering ensures that
SeqvarsQueryExecutionBackgroundJob
instances will be sorted by their primary key in descending order. This typically means that the most recently created jobs will appear first, which is often desirable for background job management.
Line range hint
2058-2502
: Overall assessment of changesThe changes in this file enhance the structure and functionality of the seqvars models:
- The new
SeqvarsModeOfInheritance
enum provides a standardized way to represent inheritance patterns.- The
GeneRelatedPhenotypesPydantic
class now supports multiple modes of inheritance for a gene.- Ordering has been added to
SeqvarsResultSet
,SeqvarsResultRow
, andSeqvarsQueryExecutionBackgroundJob
models, ensuring consistent data presentation.These changes improve the organization of the code and add useful functionality for handling genetic inheritance patterns. The additions are well-integrated with the existing code structure.
frontend/ext/varfish-api/src/lib/schemas.gen.ts (9)
Line range hint
1-1
: LGTM: Auto-generated file headerThe comment at the top of the file correctly indicates that this is an auto-generated file. This is good practice for generated code.
2493-2500
: Verify the structure of theconstraints
propertyThe
constraints
property has also been modified. Ensure this change is intended and consistent with the overall schema design.#!/bin/bash # Check for other occurrences of GeneRelatedConstraintsPydantic ast-grep --lang typescript --pattern 'GeneRelatedConstraintsPydantic'
2473-2480
: Verify the structure of theconsequences
propertySimilar to the
identity
property, theconsequences
property has been modified. Ensure this change is intended and consistent across the codebase.#!/bin/bash # Check for other occurrences of GeneRelatedConsequencesPydantic ast-grep --lang typescript --pattern 'GeneRelatedConsequencesPydantic'
2614-2621
: New propertymode_of_inheritances
addedA new property
mode_of_inheritances
has been added to theGeneRelatedPhenotypesPydantic
type. Verify that this addition is intentional and that it's being used correctly in the rest of the codebase.#!/bin/bash # Check for usage of mode_of_inheritances ast-grep --lang typescript --pattern 'mode_of_inheritances'
2483-2490
: Verify the structure of thephenotypes
propertyThe
phenotypes
property has been modified in a similar manner. Confirm that this change is intentional and consistent with other related types.#!/bin/bash # Check for other occurrences of GeneRelatedPhenotypesPydantic ast-grep --lang typescript --pattern 'GeneRelatedPhenotypesPydantic'
6133-6138
: New enumSeqvarsModeOfInheritance
addedA new enum
SeqvarsModeOfInheritance
has been added. This seems to be related to the newmode_of_inheritances
property. Verify that this enum is being used correctly throughout the codebase.#!/bin/bash # Check for usage of SeqvarsModeOfInheritance ast-grep --lang typescript --pattern 'SeqvarsModeOfInheritance'
2970-2973
: Newcount
property added to paginated responsesA
count
property has been added to various paginated response types. This is a good addition as it provides information about the total number of items. Ensure this change is consistent across all paginated response types.#!/bin/bash # Check for consistency of count property in paginated responses ast-grep --lang typescript --pattern 'type: '\''object'\'', properties: { count: { type: '\''integer'\'', example: 123 },'Also applies to: 2976-2978, 2982-2984
9795-9797
: Default value added torecessive_mode
propertyA default value of 'disabled' has been added to the
recessive_mode
property inSeqvarsQuerySettingsGenotypePydantic
. Ensure this default value is appropriate and consistent with the backend implementation.#!/bin/bash # Check for other occurrences of recessive_mode ast-grep --lang typescript --pattern 'recessive_mode'
2463-2470
: Verify the structure of theidentity
propertyThe
identity
property in theGeneRelatedAnnotationPydantic
type has been modified. Please ensure that this change is intentional and consistent with the expected schema.backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml (11)
2399-2404
: Consistent implementation of pagination changeThe change from
cursor
topage
parameter has been consistently applied here as well. This is in line with the overall pagination mechanism update.
2642-2647
: Pagination change applied to query execution endpointThe pagination change has been correctly implemented for the query execution endpoint, maintaining consistency with other endpoints.
2735-2740
: Consistent pagination implementation across multiple endpointsThe pagination change from
cursor
topage
has been consistently applied across multiple endpoints, including:
- Query presets for ClinVar
- Query presets for columns
- Query presets for consequences
- Query presets factory defaults
- Query presets for locus
- Query presets for phenotype prioritization
- Query presets for quality
- Query presets set
- Query presets set version
- Query presets for variant prioritization
- Query settings
This consistent implementation ensures a uniform API behavior across different endpoints.
Also applies to: 2942-2947, 3149-3154, 3359-3364, 3625-3630, 3832-3837, 4039-4044, 4246-4251, 4495-4500, 4741-4746, 4948-4953
7423-7437
: Nullable fields added to GeneRelatedAnnotationPydanticThe
GeneRelatedAnnotationPydantic
schema now includes nullable fields foridentity
,consequences
,phenotypes
, andconstraints
. This change allows for more flexible data representation, accommodating cases where some of this information might be missing.
7508-7513
: Default empty list for mode_of_inheritancesA default empty list has been added for the
mode_of_inheritances
field in theGeneRelatedPhenotypesPydantic
schema. This is a good practice as it ensures that the field always has a valid value, even when no specific modes of inheritance are specified.
7775-7787
: Consistent implementation of pagination in response schemasThe pagination response schemas have been updated to include a
count
field, which provides the total number of results available. This change has been consistently applied across multiple paginated response schemas, including:
- PaginatedSeqvarsPredefinedQueryList
- PaginatedSeqvarsQueryExecutionList
- PaginatedSeqvarsQueryList
- PaginatedSeqvarsQueryPresetsClinvarList
- PaginatedSeqvarsQueryPresetsColumnsList
- PaginatedSeqvarsQueryPresetsConsequenceList
- PaginatedSeqvarsQueryPresetsFrequencyList
- PaginatedSeqvarsQueryPresetsLocusList
- PaginatedSeqvarsQueryPresetsPhenotypePrioList
- PaginatedSeqvarsQueryPresetsQualityList
- PaginatedSeqvarsQueryPresetsSetList
- PaginatedSeqvarsQueryPresetsSetVersionList
- PaginatedSeqvarsQueryPresetsVariantPrioList
- PaginatedSeqvarsQuerySettingsList
- PaginatedSeqvarsResultRowList
- PaginatedSeqvarsResultSetList
This consistent implementation improves the API's usability by providing important metadata about the query results.
Also applies to: 7795-7807, 7815-7827, 7835-7847, 7855-7867, 7875-7887, 7895-7907, 7915-7927, 7935-7947, 7955-7967, 7975-7987, 7995-8007, 8015-8027, 8035-8047, 8055-8067, 8075-8087
10116-10126
: New enumeration for SeqvarsModeOfInheritanceA new enumeration
SeqvarsModeOfInheritance
has been added to represent different modes of inheritance for gene annotations. This addition provides a standardized way to represent inheritance patterns, which is crucial for genetic analysis and interpretation.
Line range hint
1-11273
: Overall assessment of API schema changesThe changes to the VarFish API schema demonstrate a significant improvement in pagination mechanisms and data representation. Key points include:
- Consistent update from cursor-based to page-based pagination across multiple endpoints.
- Addition of a
count
field in paginated response schemas, providing valuable metadata about query results.- Introduction of new parameters (e.g.,
order_by
,order_dir
) to enhance query capabilities.- Improved flexibility in data representation with nullable fields and default values.
- New enumerations (e.g.,
SeqvarsModeOfInheritance
) for standardized representation of genetic concepts.These changes collectively enhance the API's consistency, usability, and flexibility. The modifications appear to be well-implemented and should improve the overall developer experience when working with the VarFish API.
5224-5242
: Updated parameter name in result set endpointsThe parameter name has been changed from
query
toqueryexecution
in the result set endpoints. This change appears to be more descriptive and aligns better with the endpoint's purpose.Let's verify this change has been applied consistently:
#!/bin/bash # Check for the parameter name change in result set endpoints echo "Checking for 'queryexecution' parameter in result set endpoints:" sed -n '/\/seqvars\/api\/resultset/,/responses:/p' backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml | grep -E "queryexecution"Also applies to: 5260-5284
2192-2197
: Pagination parameter updated fromcursor
topage
The pagination mechanism has been changed from cursor-based to page-based pagination. This change is consistent with the AI-generated summary and appears to be implemented correctly.
To ensure this change has been applied consistently across all relevant endpoints, let's run the following verification:
5155-5168
: New parameters added to result row endpointNew parameters
order_by
andorder_dir
have been added to the/seqvars/api/resultrow/{resultset}/
endpoint. This addition enhances the query capabilities for result rows, allowing for more flexible sorting options.Let's verify that these new parameters are properly documented:
✅ Verification successful
New parameters
order_by
andorder_dir
are properly documentedThe new parameters
order_by
andorder_dir
have been successfully documented in the/seqvars/api/resultrow/{resultset}/
endpoint withinbackend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the new parameters in the result row endpoint echo "Checking for new parameters in result row endpoint:" sed -n '/\/seqvars\/api\/resultrow\/{resultset}\/:$/,/responses:/p' backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml | grep -E "order_by|order_dir"Length of output: 539
Script:
#!/bin/bash # Corrected script to check for the new parameters in the result row endpoint echo "Checking for new parameters 'order_by' and 'order_dir' in the result row endpoint:" sed -n '/\/seqvars\/api\/resultrow\/{resultset}\/:$/,/responses:/p' backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml | grep -E "order_by|order_dir"Length of output: 399
frontend/src/seqvars/components/QueryResults/ClingenDosage.vue (1)
57-64
: Verify the scoring values for 'recessive' and 'unlikely'The scores for 'recessive' and 'unlikely' are set to 30 and 40, respectively, while other scores range from 0 to 3. This might cause confusion when displaying the scores. Please verify if these values are intentional or consider aligning them for consistency.
utils/docker/Dockerfile (1)
142-142
: Confirm compatibility withvarfish-server-worker
version 0.14.0The
COPY
instruction now referencesvarfish-server-worker
version0.14.0
. Please ensure that this version is fully compatible with the existing application and that any necessary changes have been made to accommodate updates or changes in the worker's behavior or APIs.To verify compatibility, consider reviewing the release notes or changelogs for
varfish-server-worker
0.14.0 for any breaking changes or important updates that might affect the application.frontend/src/seqvars/queries/seqvarQuery.ts (1)
88-88
:⚠️ Potential issueUse explicit typing to eliminate the
@ts-ignore
comment.The use of
@ts-ignore
suppresses TypeScript errors and can mask potential issues. Instead of ignoring the error, consider defining the correct type forlastPage
so that TypeScript recognizes thenext
property. This will enhance type safety and maintain code quality.Apply this diff to specify the type of
lastPage
:- // @ts-ignore // https://github.com/hey-api/openapi-ts/issues/653#issuecomment-2314847011 getNextPageParam: (lastPage) => lastPage.next, + getNextPageParam: (lastPage: YourResponseType) => lastPage.next,Replace
YourResponseType
with the appropriate type that includes thenext
property. This approach addresses the TypeScript error without suppressing it.⛔ Skipped due to learnings
Learnt from: holtgrewe PR: varfish-org/varfish-server#1957 File: frontend/src/seqvars/queries/seqvarQueryExecution.ts:84-107 Timestamp: 2024-10-09T06:55:05.311Z Learning: In the function `useSeqvarQueryExecutionListQuery` within `frontend/src/seqvars/queries/seqvarQueryExecution.ts`, the `@ts-ignore` comment is necessary because the composable function might be used in different contexts, and removing it may not be appropriate.
backend/seqvars/protos/output_pb2.pyi (4)
81-84
: Definition of_ModeOfInheritance
is correctThe
_ModeOfInheritance
class is properly defined withValueType
and the type aliasV
.
85-104
: Enumeration_ModeOfInheritanceEnumTypeWrapper
and constants are appropriately definedThe enum wrapper
_ModeOfInheritanceEnumTypeWrapper
and the enumeration constants for modes of inheritance are correctly implemented.
105-107
:ModeOfInheritance
class is accurately inheritedThe
ModeOfInheritance
class correctly inherits from_ModeOfInheritance
with the appropriate metaclass.
108-122
: Global constants forModeOfInheritance
are correctly declaredAll global variables representing different modes of inheritance are properly assigned and documented.
backend/seqvars/tests/test_views_api.py (16)
113-116
: Ensure pagination fields are correctly tested inTestQueryPresetsSetViewSet
The addition of
"count"
,"next"
,"previous"
, and"results"
fields in the response verification ensures that the pagination is properly validated in thetest_list
method.
270-273
: Ensure pagination fields are correctly tested inTestQueryPresetsSetVersionViewSet
Including the pagination fields in the response enhances the verification of the pagination functionality in the
test_list
method.
420-423
: Ensure pagination fields are correctly tested inTestQueryPresetsQualityViewSet
Validating the pagination fields in the response ensures comprehensive testing of the list endpoint's pagination.
557-560
: Ensure pagination fields are correctly tested inTestQueryPresetsConsequenceViewSet
Adding the pagination fields to the expected response improves the accuracy of the pagination tests in the
test_list
method.
694-697
: Ensure pagination fields are correctly tested inTestQueryPresetsFrequencyViewSet
The inclusion of pagination fields in the response verification strengthens the testing of the pagination feature.
831-834
: Ensure pagination fields are correctly tested inTestQueryPresetsLocusViewSet
Verifying the pagination fields in the response confirms that the pagination is implemented correctly in the
test_list
method.
968-971
: Ensure pagination fields are correctly tested inTestQueryPresetsPhenotypePrioViewSet
Including
"count"
,"next"
,"previous"
, and"results"
in the response enhances the validation of pagination in the list endpoint.
1105-1108
: Ensure pagination fields are correctly tested inTestQueryPresetsVariantPrioViewSet
Adding pagination fields to the response verification ensures that the pagination functionality is thoroughly tested.
1242-1245
: Ensure pagination fields are correctly tested inTestQueryPresetsColumnsViewSet
Validating the pagination fields in the response improves the test coverage for the list endpoint's pagination.
1379-1382
: Ensure pagination fields are correctly tested inTestQueryPresetsClinvarViewSet
Including pagination fields in the response ensures accurate testing of the pagination feature in the
test_list
method.
1516-1519
: Ensure pagination fields are correctly tested inPredefinedQueryViewSet
Verifying the presence of pagination fields in the response strengthens the pagination tests for the list endpoint.
1649-1652
: Ensure pagination fields are correctly tested inTestQuerySettingsViewSet
Adding the pagination fields to the expected response enhances the validation of the list endpoint's pagination.
1826-1829
: Ensure pagination fields are correctly tested inTestQueryViewSet
Including
"count"
,"next"
,"previous"
, and"results"
in the response verification confirms proper pagination implementation.
2110-2113
: Ensure pagination fields are correctly tested inTestQueryExecutionViewSet
Adding pagination fields to the response checks improves the thoroughness of the pagination tests.
2213-2216
: Ensure pagination fields are correctly tested inTestResultSetViewSet
Including the pagination fields in the response verification enhances the testing of the list endpoint's pagination functionality.
2287-2290
: Ensure pagination fields are correctly tested inTestResultRowViewSet
Validating the pagination fields in the response confirms that pagination is properly implemented in the
test_list
method.frontend/ext/varfish-api/src/lib/types.gen.ts (4)
884-887
: Ensure proper handling of nullable properties inGeneRelatedAnnotationPydantic
The properties
identity
,consequences
,phenotypes
, andconstraints
inGeneRelatedAnnotationPydantic
are now nullable. Please verify that any code consuming this type correctly handles cases where these properties may benull
to prevent potentialnull
reference errors.
916-916
: Addition ofmode_of_inheritances
property is appropriateThe new property
mode_of_inheritances
has been added toGeneRelatedPhenotypesPydantic
. This enhancement aligns with the need to represent multiple modes of inheritance and appears correctly implemented.
2036-2039
: NewSeqvarsModeOfInheritance
type enhances type safetyThe addition of the
SeqvarsModeOfInheritance
type enumerates various modes of inheritance, improving type safety and clarity in code that deals with genetic annotations.
2933-2933
: Verify handling of optionalrecessive_mode
inSeqvarsQuerySettingsGenotypePydantic
The
recessive_mode
property inSeqvarsQuerySettingsGenotypePydantic
is now optional. Please ensure that all usages of this type account forrecessive_mode
potentially beingundefined
to avoid unintended behavior.frontend/ext/varfish-api/src/lib/@tanstack/vue-query.gen.ts (2)
5-6
: Code Import and Declarations Look GoodThe updated imports and function declarations correctly reflect the necessary changes.
68-71
: Confirm the Impact of Commenting Out 'body' Parameter HandlingThe 'body' parameter merging is commented out in multiple infinite query options functions. Please verify that this change is intentional and does not affect API calls relying on the 'body' parameter.
Run the following script to check for any API calls that require the 'body' parameter in infinite query options:
backend/seqvars/views_api.py (2)
8-8
: Import OpenAPI utilities for schema enhancementsThe import of
OpenApiParameter
andextend_schema
fromdrf_spectacular.utils
is correctly added to support OpenAPI schema enhancements.
14-14
: Changed pagination strategy toPageNumberPagination
The switch from
CursorPagination
toPageNumberPagination
by importingPageNumberPagination
and updating theStandardPagination
class reflects a change in pagination strategy.Please ensure that this change does not adversely affect existing API clients that might rely on cursor-based pagination. Confirm that all endpoints and clients are compatible with page-number-based pagination.
Also applies to: 64-66
/** | ||
* Query for a list of rows within a seqvar result set. | ||
* | ||
* @param resultSetUuid UUID of the result set to load rows for. | ||
* @param page page to use. | ||
* @param pageSize Number of rows to query per page. | ||
* @param orderBy Field to order by. | ||
* @param orderDir Direction to order by. | ||
* @returns Query result with page of cases. | ||
*/ | ||
export const useResultRowListQuery = ( | ||
options: MaybeRefOrGetter<{ | ||
resultSetUuid: MaybeRefOrGetter<string | undefined> | ||
page: MaybeRefOrGetter<number | undefined> | ||
pageSize: MaybeRefOrGetter<number> | ||
orderBy: MaybeRefOrGetter<string | undefined> | ||
orderDir: MaybeRefOrGetter<OrderDir | undefined> | ||
}>, | ||
) => { | ||
const { resultSetUuid, page, pageSize, orderBy, orderDir } = toValue(options) | ||
return useQuery({ | ||
...seqvarsApiResultrowListOptions({ | ||
client, | ||
path: { | ||
// @ts-ignore // https://github.com/hey-api/openapi-ts/issues/653#issuecomment-2314847011 | ||
resultset: () => toValue(resultSetUuid)!, | ||
}, | ||
// @ts-ignore // https://github.com/hey-api/openapi-ts/issues/653#issuecomment-2314847011 | ||
query: () => ({ | ||
page: toValue(page), | ||
page_size: toValue(pageSize), | ||
order_by: toValue(orderBy) ?? 'name', | ||
order_dir: toValue(orderDir) ?? 'asc', | ||
}), | ||
}), | ||
// @ts-ignore // https://github.com/hey-api/openapi-ts/issues/653#issuecomment-2314847011 | ||
enabled: () => !!toValue(resultSetUuid), | ||
}) | ||
} |
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.
Address TypeScript issues and update parameter description.
-
The
@ts-ignore
comments indicate underlying type issues. Consider resolving these to maintain type safety:- Line 40-41: Investigate why TypeScript is flagging an error for the
resultset
path. - Line 43-44: Look into why TypeScript is raising an issue with the
query
object. - Line 51-52: Examine why TypeScript is having trouble with the
enabled
function.
- Line 40-41: Investigate why TypeScript is flagging an error for the
-
In the function's JSDoc comment, update the parameter description for
resultSetUuid
:- Current: "UUID of the result set to load rows for."
- Suggested: "UUID of the seqvar result set to load rows for."
This change will maintain consistency with the terminology used in other parts of the code.
snapshots["ImportCreateWithSeqvarsVcfTest::test_run internal files"] = [ | ||
{ | ||
"checksum": None, | ||
"designation": "variant_calls/seqvars/orig-copy", | ||
"file_attributes": { | ||
"checksum": "sha256:4042c2afa59f24a327b3852bfcd0d8d991499d9c4eb81e7a7efe8d081e66af82", | ||
"designation": "variant_calls", | ||
"genomebuild": "grch37", | ||
"mimetype": "text/plain+x-bgzip+x-variant-call-format", | ||
"variant_type": "seqvars", | ||
}, | ||
"identifier_map": {"index": "NA12878-PCRF450-1"}, | ||
"mimetype": "text/plain+x-bgzip+x-variant-call-format", | ||
"path": "case-data/7a/1d7b28-2bf8-4340-81f3-5487d86c669f/c28a70a6-1c75-40a1-8d89-216ca16cffca/seqvars/external-copy-0.vcf.gz", | ||
}, | ||
{ | ||
"checksum": None, | ||
"designation": "variant_calls/seqvars/ingested-vcf", | ||
"file_attributes": {}, | ||
"identifier_map": {}, | ||
"mimetype": "text/plain+x-bgzip+x-variant-call-format", | ||
"path": "case-data/7a/1d7b28-2bf8-4340-81f3-5487d86c669f/c28a70a6-1c75-40a1-8d89-216ca16cffca/seqvars/ingested.vcf.gz", | ||
}, | ||
{ | ||
"checksum": None, | ||
"designation": "variant_calls/seqvars/ingested-tbi", | ||
"file_attributes": {}, | ||
"identifier_map": {}, | ||
"mimetype": "application/octet-stream+x-tabix-tbi-index", | ||
"path": "case-data/7a/1d7b28-2bf8-4340-81f3-5487d86c669f/c28a70a6-1c75-40a1-8d89-216ca16cffca/seqvars/ingested.vcf.gz.tbi", | ||
}, | ||
] |
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.
💡 Codebase verification
Checksum assignments not found in the codebase.
The snapshots show that the checksum
field is None
for all entries in ImportCreateWithSeqvarsVcfTest::test_run internal files
. Additionally, the codebase does not assign values to file_attributes["checksum"]
, which may result in missing checksum validations.
Please ensure that checksum values are correctly calculated and assigned to maintain data integrity.
🔗 Analysis chain
Well-structured snapshot for internal files, with a few points to verify.
The snapshot for ImportCreateWithSeqvarsVcfTest::test_run internal files
is well-organized and includes the expected files (original copy, ingested VCF, and index). The use of UUIDs in paths is a good practice for ensuring unique storage per case.
However, please verify the following:
- The
checksum
field isNone
for all entries. Is this intentional? - The
file_attributes
andidentifier_map
are empty for ingested files. Is this expected behavior?
Re-running verification to check checksum implementations.
Further verification needed for checksum assignments.
To ensure that checksum
values are being correctly calculated and assigned, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the checksum calculation is implemented for internal files
rg --type python 'def calculate_checksum|checksum =' backend/cases_import/
Length of output: 107
Script:
#!/bin/bash
# Verify if the checksum calculation is implemented for internal files by searching for 'def calculate_checksum' or 'checksum =' in Python files
rg --glob '*.py' 'def calculate_checksum|checksum =' backend/cases_import/
Length of output: 167
Script:
#!/bin/bash
# Search for assignments to file_attributes["checksum"] in Python files
rg --glob '*.py' 'file_attributes\["checksum"\]\s*=' backend/cases_import/
Length of output: 76
<template v-else> | ||
<v-chip | ||
density="compact" | ||
variant="outlined" | ||
class="ml-1 px-1 opacity-0" | ||
rounded="lg" | ||
> | ||
AD | ||
</v-chip> | ||
<v-chip | ||
density="compact" | ||
variant="outlined" | ||
class="ml-1 px-1 opacity-0" | ||
rounded="lg" | ||
> | ||
AR | ||
</v-chip> | ||
</template> |
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.
🛠️ Refactor suggestion
Consider alternative approach for fallback rendering
The current implementation renders invisible chips to maintain layout consistency when no inheritance mode is applicable. While this works, it might not be the most efficient approach.
Consider using CSS to maintain the layout instead of rendering invisible elements. You could create a placeholder div with the same dimensions as the chips:
<template v-else>
<div class="chip-placeholder"></div>
<div class="chip-placeholder"></div>
</template>
Add the following CSS:
.chip-placeholder {
display: inline-block;
width: 28px; /* Adjust based on your chip size */
height: 24px; /* Adjust based on your chip size */
margin-left: 4px;
}
This approach achieves the same visual result without rendering unnecessary DOM elements.
const callHeaders = computed<HeaderDef[]>(() => { | ||
const result = [] | ||
// in the next line, ignore typescript at all | ||
for (const { name } of props.caseObj.pedigree_obj?.individual_set ?? []) { | ||
result.push( | ||
{ | ||
title: `Genotype ${name}`, | ||
key: `payload.variant_annotation.call.call_infos.${name}.genotype`, | ||
}, | ||
{ | ||
title: `Total Depth ${name}`, | ||
key: `payload.variant_annotation.call.call_infos.${name}.dp`, | ||
}, | ||
{ | ||
title: `Alternate Depth ${name}`, | ||
key: `payload.variant_annotation.call.call_infos.${name}.ad`, | ||
}, | ||
{ | ||
title: `Genotype Quality ${name}`, | ||
key: `payload.variant_annotation.call.call_infos.${name}.gq`, | ||
}, | ||
{ | ||
title: `Phase Set ${name}`, | ||
key: `payload.variant_annotation.call.call_infos.${name}.ps`, | ||
}, | ||
) | ||
} | ||
return result | ||
}) | ||
/** The headers to display, including from calls. */ | ||
const headers = computed<HeaderDef[]>(() => { | ||
return [...BASE_HEADERS, ...callHeaders.value] | ||
}) |
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.
🛠️ Refactor suggestion
Enhance type safety in callHeaders
computed property
The callHeaders
computed property dynamically generates headers based on the case object, which is a good approach for flexibility. However, the type assertion on line 239 can be improved for better type safety:
for (const { name } of props.caseObj.pedigree_obj?.individual_set ?? []) {
Consider using a type guard or optional chaining to ensure type safety without resorting to type assertions. For example:
const individualSet = props.caseObj.pedigree_obj?.individual_set
if (Array.isArray(individualSet)) {
for (const { name } of individualSet) {
// ... rest of the code
}
}
This approach maintains type safety while still handling the possibility of individual_set
being undefined.
# Note: will fail as execution of worker in test fails. | ||
self.assertEqual( | ||
new_seqvarqueryexecution.state, | ||
SeqvarsQueryExecution.STATE_QUEUED, | ||
SeqvarsQueryExecution.STATE_FAILED, |
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.
Handle potential test failure due to worker execution
The assertion expects new_seqvarqueryexecution.state
to be STATE_FAILED
, and the comment indicates that this will fail because the worker execution in the test fails. Consider mocking the worker execution or adjusting the test to handle this scenario to prevent unintended test failures.
}; | ||
}; | ||
|
||
export type CasesImportApiCaseImportActionRetrieveUpdateDestroyDestroyResponse = (void); | ||
export type CasesImportApiCaseImportActionDestroyResponse = (void); |
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.
Replace void
with undefined
for better type clarity
Using void
outside of function return types or type parameters can be confusing in TypeScript. Consider replacing (void)
with (undefined)
in CasesImportApiCaseImportActionDestroyResponse
for clarity and to align with TypeScript best practices.
Apply this diff to fix the issue:
-export type CasesImportApiCaseImportActionDestroyResponse = (void);
+export type CasesImportApiCaseImportActionDestroyResponse = (undefined);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export type CasesImportApiCaseImportActionDestroyResponse = (void); | |
export type CasesImportApiCaseImportActionDestroyResponse = (undefined); |
🧰 Tools
🪛 Biome
[error] 3476-3476: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
// body: { | ||
// ...queryKey[0].body as any, | ||
// ...page.body as any | ||
// }, |
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.
🛠️ Refactor suggestion
Consider Refactoring Repeated Commented-Out 'body' Parameter Handling
The pattern of commenting out the 'body' parameter merging is repeated in several infinite query options functions across multiple sections (e.g., lines 68-71, 140-143, 204-215, etc.). To improve code maintainability and reduce duplication, consider refactoring this logic into a shared helper function or utilizing a configuration option.
Also applies to: 140-143, 204-215, 1256-1265, 1384-1393, 1538-1547, 1636-1645, 1764-1773, 1892-1901, 2020-2029, 2092-2101, 2220-2229, 2348-2357, 2476-2485, 2604-2613, 2758-2767, 2912-2921, 3040-3049, 3168-3177, 3240-3249
elif "queryexecution" in kwargs: | ||
queryexecution = get_object_or_404( | ||
SeqvarsQueryExecution.objects.all(), sodar_uuid=kwargs["queryexecution"] | ||
) | ||
project = queryexecution.query.session.caseanalysis.case.project |
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.
Add error handling for nested attribute access
In the get_project
function, the nested attribute access:
project = queryexecution.query.session.caseanalysis.case.project
could potentially raise an AttributeError
if any intermediate object (queryexecution
, query
, session
, caseanalysis
, case
) is None
.
Consider adding checks or error handling to ensure that all intermediate objects are not None
before accessing their attributes. This will improve the robustness of your code.
with transaction.atomic(): | ||
queryexecution = SeqvarsQueryExecution.objects.create( | ||
state=SeqvarsQueryExecution.STATE_QUEUED, | ||
query=query, | ||
querysettings=query.settings.make_clone(), | ||
) | ||
bgjob = SeqvarsQueryExecutionBackgroundJob.objects.create_full( | ||
seqvarsqueryexecution=queryexecution, | ||
user=self.request.user, | ||
) | ||
run_seqvarsqueryexecutionbackgroundjob.delay(seqvarsqueryexecutionbackgroundjob_pk=bgjob.pk) |
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.
Remove redundant transaction block
The start
method is already decorated with @transaction.atomic()
, which wraps the entire method in a transaction. The inner with transaction.atomic():
block is redundant.
Remove the inner transaction block to avoid unnecessary nesting:
@transaction.atomic()
def start(self, *args, **kwargs):
"""Create a new query execution for the given query.
Also, start the execution of a background job.
"""
query = None
# TODO: check permissions on the source's project
query = SeqvarsQuery.objects.get(sodar_uuid=self.kwargs["query"])
- with transaction.atomic():
queryexecution = SeqvarsQueryExecution.objects.create(
state=SeqvarsQueryExecution.STATE_QUEUED,
query=query,
querysettings=query.settings.make_clone(),
)
bgjob = SeqvarsQueryExecutionBackgroundJob.objects.create_full(
seqvarsqueryexecution=queryexecution,
user=self.request.user,
)
run_seqvarsqueryexecutionbackgroundjob.delay(seqvarsqueryexecutionbackgroundjob_pk=bgjob.pk)
serializer = self.get_serializer(queryexecution)
return Response(serializer.data)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
with transaction.atomic(): | |
queryexecution = SeqvarsQueryExecution.objects.create( | |
state=SeqvarsQueryExecution.STATE_QUEUED, | |
query=query, | |
querysettings=query.settings.make_clone(), | |
) | |
bgjob = SeqvarsQueryExecutionBackgroundJob.objects.create_full( | |
seqvarsqueryexecution=queryexecution, | |
user=self.request.user, | |
) | |
run_seqvarsqueryexecutionbackgroundjob.delay(seqvarsqueryexecutionbackgroundjob_pk=bgjob.pk) | |
queryexecution = SeqvarsQueryExecution.objects.create( | |
state=SeqvarsQueryExecution.STATE_QUEUED, | |
query=query, | |
querysettings=query.settings.make_clone(), | |
) | |
bgjob = SeqvarsQueryExecutionBackgroundJob.objects.create_full( | |
seqvarsqueryexecution=queryexecution, | |
user=self.request.user, | |
) | |
run_seqvarsqueryexecutionbackgroundjob.delay(seqvarsqueryexecutionbackgroundjob_pk=bgjob.pk) |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
CellGeneFlags
, for displaying genetic variant information.useCaseRetrieveQuery
, for retrieving case details.QueryResults
, for displaying query results related to cases.QueryResultsTable
, for displaying results in a structured format.Case_1_exons.ped
, for family genetic information.Bug Fixes
api/resultset
endpoint to reflect correct parameters.Documentation
Chores