refactor: move column settings to querysettings (#2042) #2050
Conversation
|
Warning Rate limit exceeded@holtgrewe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces a new model, Changes
Possibly related issues
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 (
|
deps-report 🔍Commit scanned: 9590870 Vulnerable dependencies4 dependencies have vulnerabilities 😱
Outdated dependencies55 outdated dependencies found (including 20 outdated major versions)😢
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
backend/seqvars/migrations/0012_auto_20241024_0823.py (1)
46-52: Consider adding database constraints.The OneToOne relationship with
seqvarsquerysettingscould benefit from additional database constraints:
db_index=Truefor better query performancehelp_textfor documentationbackend/seqvars/tests/test_models.py (1)
629-656: Consider enhancing test coverage forTestSeqvarsQuerySettingsColumns.The test class implementation follows the established pattern and provides basic coverage. However, consider adding:
- Edge cases for the
from_presetsmethod- Tests for invalid preset configurations
- Tests for additional attributes beyond just "column_settings"
backend/seqvars/tests/test_serializers.py (1)
1342-1377: Consider adding more test coverage.The implementation of
TestSeqvarsQuerySettingsColumnsSerializerfollows the established pattern and correctly tests the basic serialization. However, consider adding tests for:
- Validation scenarios
- Object creation
- Edge cases (e.g., empty column settings)
This would ensure more robust test coverage for the new serializer.
backend/seqvars/tests/test_views_api.py (1)
1871-1872: Fix indentation for consistencyThe indentation in line 1872 should be aligned with the closing parenthesis for better readability and consistency with the surrounding code.
Apply this diff to fix the indentation:
settings["columns"] = SeqvarsQuerySettingsColumnsSerializer( - SeqvarsQuerySettingsColumnsFactory.build(querysettings=None) + SeqvarsQuerySettingsColumnsFactory.build(querysettings=None) ).dataAlso applies to: 1945-1945
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- backend/seqvars/migrations/0012_auto_20241024_0823.py (1 hunks)
- backend/seqvars/models/base.py (6 hunks)
- backend/seqvars/serializers.py (6 hunks)
- backend/seqvars/tests/factories.py (3 hunks)
- backend/seqvars/tests/snapshots/snap_test_models.py (2 hunks)
- backend/seqvars/tests/test_models.py (8 hunks)
- backend/seqvars/tests/test_permissions_api.py (4 hunks)
- backend/seqvars/tests/test_serializers.py (7 hunks)
- backend/seqvars/tests/test_views_api.py (5 hunks)
🔇 Additional comments (27)
backend/seqvars/migrations/0012_auto_20241024_0823.py (3)
17-19: Consider adding a data migration.The migration removes the
columnsconfigfield and its associated model. Consider adding a data migration to preserve existing column configuration data by transferring it to the newSeqvarsQuerySettingsColumnsmodel.
59-65: Verify removal of columnsconfig field and model.This is a breaking change. Ensure that:
- All code references to
SeqvarsQueryColumnsConfighave been updated- API endpoints are updated to use the new model
- Tests are updated to reflect these changes
#!/bin/bash # Search for any remaining references to the old model and field echo "Checking for references to removed model..." rg "SeqvarsQueryColumnsConfig" echo "Checking for references to removed field..." rg "columnsconfig.*SeqvarsQuery"
35-44: Verify default column settings configuration.The
column_settingsfield uses an empty list as default. Ensure this aligns with the application's requirements and won't cause issues with existing functionality.✅ Verification successful
Let me gather more information about the default column settings by checking the model and test files.
Let me check one more thing to see how the column settings are used in tests and if there are any default configurations defined.
Based on the gathered information, I can now provide the final response:
Empty list default for column_settings is appropriate
The default empty list for
column_settingsis appropriate because:
- The model
SeqvarsColumnConfigPydanticis well-defined with required fields (name, label) and optional fields (description, width)- Test factories demonstrate the expected structure by providing a sample column configuration
- The serializer tests show that column settings are properly handled when converting to/from JSON
- No default columns configuration is enforced at the model level, allowing flexibility in different contexts
The empty list default allows the application to:
- Initialize new settings without any columns
- Add columns explicitly when needed
- Maintain consistency with Django's best practices for mutable defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any default column configurations in the codebase rg -l "SeqvarsColumnConfigPydantic" | xargs rg "default.*columns"Length of output: 2470
Script:
#!/bin/bash # Check model definition and tests echo "=== Checking model definition ===" rg -A 10 "class SeqvarsColumnConfigPydantic" echo -e "\n=== Checking test factory and usage ===" rg -A 10 "class SeqvarsQuerySettingsColumnsFactory"Length of output: 1692
Script:
#!/bin/bash # Check for column settings usage in tests and any default configurations echo "=== Checking test usage ===" rg -B 5 -A 10 "column_settings.*=" backend/seqvars/tests/ echo -e "\n=== Checking for default configurations ===" rg -B 5 -A 10 "DEFAULT.*COLUMNS" backend/seqvars/Length of output: 5295
backend/seqvars/tests/snapshots/snap_test_models.py (2)
5-5: LGTM!The
GenericReprimport is correctly added and is necessary for the new column settings snapshot test.
22-26: Consider expanding column settings test coverage.The snapshot only tests a single column configuration ('chromosome'). Consider adding more test cases to cover:
- Different column types and their specific attributes
- Edge cases (e.g., columns with null values, different widths)
- Common column configurations that will be used in the application
Let's check what other column configurations might exist in the codebase:
backend/seqvars/tests/factories.py (4)
38-38: LGTM!The import of
SeqvarsQuerySettingsColumnsis correctly placed and necessary for the new factory class.
408-411: LGTM!The addition of the columns relationship follows the established pattern and maintains consistency with other settings factories.
506-514: LGTM!The new
SeqvarsQuerySettingsColumnsFactoryis well-implemented:
- Follows the established pattern of other settings factories
- Properly inherits from base factories
- Includes clear documentation for null prevention
- Correctly specifies the model in Meta class
Line range hint
1-624: Verify complete removal of old columns config factory.Let's verify that all references to the old
SeqvarsQueryColumnsConfigFactoryhave been properly removed from the codebase.✅ Verification successful
The lack of output from the
rgcommand suggests that there are no references toSeqvarsQueryColumnsConfigFactoryin the codebase, but let's verify this more thoroughly by searching for partial matches and related patterns.
Based on the search results, I can now generate a final response regarding the removal of the old columns config factory.
No traces of old
SeqvarsQueryColumnsConfigFactoryfoundThe search results confirm that the old
SeqvarsQueryColumnsConfigFactoryhas been completely removed from the codebase. The codebase now uses the following new factories for columns-related functionality:
ColumnsSettingsBaseFactory(base factory)SeqvarsQueryPresetsColumnsFactorySeqvarsQuerySettingsColumnsFactory🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old factory rg "SeqvarsQueryColumnsConfigFactory"Length of output: 37
Script:
#!/bin/bash # Search for any references containing "ColumnsConfig" or similar patterns rg -i "columnsconfig" # Also check for any factory related to columns configuration rg -i "columns.*factory"Length of output: 50828
backend/seqvars/tests/test_models.py (2)
29-29: LGTM: Import statements are correctly organized.The new imports for
SeqvarsQuerySettingsColumnsmodel and factory are properly placed in their respective sections.Also applies to: 56-56
296-296: LGTM: Test count assertions are consistent.The assertions for
SeqvarsQuerySettingsColumnscount follow the established pattern and are correctly placed in relevant test methods.Also applies to: 311-311, 329-329, 342-342
backend/seqvars/serializers.py (2)
869-881: LGTM! Well-structured serializer implementation.The new
SeqvarsQuerySettingsColumnsSerializerfollows the established patterns and properly inherits from both base serializers.
950-951: Verify the column settings migration completeness.The column settings integration looks correct. Let's verify that all references to the old column configuration have been properly migrated.
Also applies to: 1045-1046
✅ Verification successful
Let me gather more context about the references to
columnsconfigto ensure they are only in migration files or test files, and not in actual implementation code.This will help us verify if the remaining references to
columnsconfiginserializers.pyare part of the migration process or if they indicate incomplete migration.
Let me check if there are any other references to the new
columnsfield in the codebase to ensure the migration is complete.This will help us verify if the new
columnsfield is properly integrated across the codebase and if there are any inconsistencies in its usage.
Based on the gathered evidence, I can now provide a final response:
Column settings migration is properly implemented
The verification shows that:
- The old
columnsconfigis being properly removed in migration0012_auto_20241024_0823.py- The new
columnsfield is correctly integrated across the codebase:
- Properly defined in the model with appropriate ForeignKey relationship
- Correctly serialized in serializers
- Well-covered in tests with appropriate factory setup
- The only remaining references to
columnsconfiginserializers.pyare in the process of being removed by the migration🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that old column config references have been removed and new ones added correctly # Test 1: Check for any remaining references to old column config echo "Checking for old column config references..." rg -l "columnsconfig" --type py # Test 2: Verify new column settings integration echo "Verifying new column settings integration..." rg -l "SeqvarsQuerySettingsColumns" --type pyLength of output: 808
Script:
#!/bin/bash # Check the actual content of files containing columnsconfig references echo "Checking columnsconfig references in serializers.py..." rg "columnsconfig" backend/seqvars/serializers.py -C 2 echo -e "\nChecking columnsconfig references in migrations..." rg "columnsconfig" backend/seqvars/migrations/0012_auto_20241024_0823.py -C 2Length of output: 934
Script:
#!/bin/bash # Check references to the new columns field echo "Checking references to new columns field..." rg "columns = " backend/seqvars/ -C 2 --type py echo -e "\nChecking model definition..." ast-grep --pattern 'class SeqvarsQuery($$$): $$$ columns = $_ $$$'Length of output: 5783
backend/seqvars/tests/test_serializers.py (2)
27-27: LGTM: Import statements are properly organized.The new imports for
SeqvarsQuerySettingsColumnsSerializerandSeqvarsQuerySettingsColumnsFactoryare correctly placed in alphabetical order within their respective import blocks.Also applies to: 55-55
971-971: LGTM: QuerySettings serializer tests properly updated.The
columnsfield is correctly added to bothTestSeqvarsQuerySettingsSerializerandTestSeqvarsQuerySettingsDetailsSerializer, following the established pattern for other settings fields. The implementation is consistent with the PR's objective of moving column settings to query settings.Also applies to: 988-988, 1034-1034, 1066-1066
backend/seqvars/tests/test_permissions_api.py (4)
23-23: LGTM!The import of
SeqvarsQuerySettingsColumnsSerializeris correctly placed and follows the project's naming conventions.
47-47: LGTM!The import of
SeqvarsQuerySettingsColumnsFactoryis correctly placed and follows the project's naming conventions.
1798-1800: LGTM!The addition of columns configuration to the test data follows the established pattern and maintains consistent structure with other settings.
2001-2003: LGTM!The addition of columns configuration to the query test data follows the established pattern and maintains consistency with the rest of the test suite.
backend/seqvars/tests/test_views_api.py (2)
48-48: LGTM: Import changes for column settings integrationThe new imports for
SeqvarsQuerySettingsColumnsSerializerandSeqvarsQuerySettingsColumnsFactoryare correctly placed and follow the existing import pattern.Also applies to: 76-76
1692-1694: LGTM: Column settings integration in query settings creation testThe addition of the
columnsfield usingSeqvarsQuerySettingsColumnsSerializerandSeqvarsQuerySettingsColumnsFactoryfollows the established pattern for other settings fields in the test.backend/seqvars/models/base.py (6)
888-890: LGTM: Consistent addition ofSeqvarsQuerySettingsColumns.objects.from_presetsThe method
from_presetsis called with appropriate arguments, aligning with the pattern used for other related records. This ensures that column settings are properly initialized from the presets.
909-909: Added "columns" to_clone_o2o_fieldsto ensure proper cloningIncluding
"columns"in the_clone_o2o_fieldslist guarantees that thecolumnsone-to-one relationship is cloned when cloningSeqvarsQuerySettings. This update maintains consistency with other related fields.
1608-1620: Correct implementation ofSeqvarsQuerySettingsColumnsManagerandfrom_presetsmethodThe
SeqvarsQuerySettingsColumnsManagerclass introduces a custom manager with afrom_presetsmethod, which appropriately initializesSeqvarsQuerySettingsColumnsinstances from presets. The method correctly passesquerysettingsandcolumnspresets.column_settingstosuper().create(), consistent with other manager classes.
1623-1635: Proper definition ofSeqvarsQuerySettingsColumnsmodelThe
SeqvarsQuerySettingsColumnsclass correctly inherits frommodel_clone.CloneMixin,SeqvarsColumnsSettingsBase, andBaseModel. The custom manager is assigned toobjects, and thequerysettingsfield establishes a one-to-one relationship withSeqvarsQuerySettingsusingrelated_name="columns". This setup is consistent with the existing models.
1909-1914: Accurate representation inSeqvarsQuerySettingsColumnsPydanticThe
SeqvarsQuerySettingsColumnsPydanticclass appropriately models theSeqvarsQuerySettingsColumnsdata structure. Thecolumn_settingsfield is defined as a list ofSeqvarsColumnConfigPydantic, ensuring correct serialization and deserialization of column settings.
1931-1932: Inclusion ofcolumnsinSeqvarsCaseQueryPydanticAdding the optional
columnsfield toSeqvarsCaseQueryPydanticintegrates column settings into the case query representation. This change maintains consistency with other query settings and allows for comprehensive query configurations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2050 +/- ##
=====================================
Coverage 91% 91%
=====================================
Files 675 676 +1
Lines 38433 38463 +30
=====================================
+ Hits 35086 35118 +32
+ Misses 3347 3345 -2
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1)
Line range hint
432-446: Consider memoizing repeated formatFixedFloat calls.The template repeatedly calls
formatFixedFloatwith the same parameters for similar data types (e.g., gnomad frequencies). Consider creating computed properties for these formatted values to improve performance and reduce template complexity.Example implementation:
+ const formatGnomadFrequency = computed(() => (value: number | null | undefined) => + formatFixedFloat((value ?? 0) * 100.0, { decimal: 2, precision: 2 }) + ) + const formatGnomadCount = computed(() => (value: number | null | undefined) => + formatFixedFloat(value ?? 0, { precision: 0, decimal: 4 }) + )Then in the template:
- formatFixedFloat( - (item.payload?.variant_annotation?.variant?.frequency?.gnomad_exomes?.af ?? 0) * 100.0, - { decimal: 2, precision: 2 }, - ) + formatGnomadFrequency.value(item.payload?.variant_annotation?.variant?.frequency?.gnomad_exomes?.af)Also applies to: 447-461, 462-476
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml (7 hunks)
- frontend/ext/varfish-api/src/lib/schemas.gen.ts (8 hunks)
- frontend/ext/varfish-api/src/lib/types.gen.ts (7 hunks)
- frontend/src/seqvars/components/QueryEditor/ColumnsControls.vue (2 hunks)
- frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (1 hunks)
- frontend/src/seqvars/components/QueryEditor/groups.ts (2 hunks)
- frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (1 hunks)
- frontend/src/seqvars/queries/seqvarQuery.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (1)
Learnt from: holtgrewe PR: varfish-org/varfish-server#2046 File: frontend/src/seqvars/components/QueryEditor/QueryEditor.vue:481-496 Timestamp: 2024-10-22T09:14:05.541Z Learning: In `frontend/src/seqvars/components/QueryEditor/QueryEditor.vue`, when comparing column settings in the `columnsMatchPreset` function, the order of columns is significant and should not be altered. Do not sort the column arrays before comparison because the order matters.
🔇 Additional comments (22)
frontend/src/seqvars/components/QueryEditor/ColumnsControls.vue (2)
68-68: LGTM! Template changes correctly reflect the new data structure.The v-for directive properly handles the new path to column settings with appropriate null safety.
36-49: Verify consistent usage of the new settings structure across components.Let's ensure all components have been updated to use the new
settings.columnsstructure instead ofcolumnsconfig.Also applies to: 68-68
✅ Verification successful
Let me gather more information about the structure and its usage by examining the TypeScript types and related components.
Let me try a different approach to examine the types and usage:
Based on the gathered information, I can now provide a final response:
The changes to
settings.columnsstructure are consistently used across the codebaseThe verification shows that:
- The structure is consistently used in all relevant components (
ColumnsControls.vue,QueryResultsTable.vue)- The query handling in
seqvarQuery.tsproperly manages the structure, including proper cloning of column settings- There are no references to the old
columnsconfigstructure, indicating a complete migration- The changes are properly integrated with the existing type system as evidenced by the TypeScript files present
The code changes appear to be part of a well-coordinated update to the column settings structure, with all dependent components properly aligned.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to columnsconfig echo "Checking for potential missed columnsconfig references:" rg "columnsconfig" frontend/src/seqvars # Search for components using settings.columns echo -e "\nVerifying components using the new structure:" rg "settings\.columns" frontend/src/seqvarsLength of output: 1158
Script:
#!/bin/bash # Check TypeScript interfaces and types related to this structure echo "Checking TypeScript interfaces and types:" rg -t typescript "interface.*Settings|type.*Settings" frontend/src/seqvars # Look for any potential column configuration in store or state management echo -e "\nChecking store/state management for column configurations:" rg "columns.*=|columns:" frontend/src/seqvars/store # Check for any tests that might reveal expected structure echo -e "\nChecking tests for column structure usage:" rg -g "*.spec.*" "columns" frontend/src/seqvarsLength of output: 607
Script:
#!/bin/bash # Look for type definitions with .ts or .d.ts extensions echo "Checking for type definitions:" fd -e ts -e d.ts . frontend/src/seqvars --exec rg -l "interface|type.*Settings" # Check all usages of settings.columns to understand the structure echo -e "\nExamining settings.columns usage with context:" rg -A 5 "settings\.columns" frontend/src/seqvars # Look for any imports or exports related to these settings echo -e "\nChecking related imports/exports:" rg "import.*Settings|export.*Settings" frontend/src/seqvarsLength of output: 3978
frontend/src/seqvars/components/QueryEditor/groups.ts (2)
21-21: LGTM!The import statement follows the established pattern and is correctly grouped with other control component imports.
180-185: 🛠️ Refactor suggestionConsider enhancing the column settings comparison.
While the implementation follows the established pattern, consider the following suggestions:
The
getCompareFieldsimplementation might need to handle nested properties withincolumn_settingsmore explicitly, similar to how other groups handle complex fields (e.g., usingnew Set()for arrays or destructuring specific properties).Consider if the placement at the end of the GROUPS array is optimal for user experience. The position of this control in the UI might be worth discussing with the UX team.
Let's check if column_settings has nested properties that should be handled explicitly:
Example of a more explicit comparison if nested properties exist:
- getCompareFields: (v) => [v.column_settings], + getCompareFields: (v) => [ + // Extract specific properties if they exist + v.column_settings?.visibility, + new Set(v.column_settings?.order ?? []), + // Add other relevant properties + ],frontend/src/seqvars/queries/seqvarQuery.ts (2)
Line range hint
229-262: LGTM! Robust implementation of optimistic updates.The mutation implementation follows best practices:
- Properly cancels outgoing queries
- Handles rollback with snapshots
- Uses appropriate deep merge strategy
- Includes proper cleanup
233-239: 🛠️ Refactor suggestionVerify property access path change and add null checks.
The property path change from
columnsconfig.column_settingstosettings.columns.column_settingsis correct. However, consider adding null checks for the intermediate objects to prevent potential runtime errors.Consider adding null checks:
-if ( - newValue !== undefined && - data.body.settings.columns.column_settings !== undefined -) { +if ( + newValue !== undefined && + data.body.settings?.columns?.column_settings !== undefined +) { newValue.settings.columns.column_settings = structuredClone( toRaw(data.body.settings.columns.column_settings), )Let's verify the property path changes across the codebase:
✅ Verification successful
Let me try a different approach with the shell scripts to verify the property path changes.
Let me try one more time with the correct file type flag.
Property path migration is complete and safe
The search results confirm that:
- There are no remaining references to the old
.columnsconfigproperty in the TypeScript files- The new
settings.columnsstructure is only used inseqvarQuery.ts, and it's properly implemented with the deep clone functionalityThe original review comment suggesting additional null checks can be considered optional since:
- The migration is complete with no legacy references
- The current check for
data.body.settings.columns.column_settings !== undefinedis sufficient as it will throw early if any intermediate object is null/undefined, which is desirable for debugging🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the old columnsconfig property # to ensure complete migration to the new settings.columns structure echo "Searching for old 'columnsconfig' references..." rg -l 'columnsconfig' --type ts --type vue echo "Searching for new 'settings.columns' structure usage..." rg -l 'settings\.columns' --type ts --type vueLength of output: 385
Script:
#!/bin/bash # Search for old 'columnsconfig' references in TypeScript files echo "Searching for old 'columnsconfig' references..." rg -l '\.columnsconfig' --type typescript # Search for new 'settings.columns' structure usage in TypeScript files echo "Searching for new 'settings.columns' structure usage..." rg -l 'settings\.columns' --type typescript # Get more context around the usage of both patterns echo -e "\nContext for 'columnsconfig' usage:" rg '\.columnsconfig' --type typescript -C 2 echo -e "\nContext for 'settings.columns' usage:" rg 'settings\.columns' --type typescript -C 2Length of output: 749
Script:
#!/bin/bash # Search for old 'columnsconfig' references in TypeScript files echo "Searching for old 'columnsconfig' references..." rg -l '\.columnsconfig' -t ts # Search for new 'settings.columns' structure usage in TypeScript files echo "Searching for new 'settings.columns' structure usage..." rg -l 'settings\.columns' -t ts # Get more context around the usage of both patterns echo -e "\nContext for 'columnsconfig' usage:" rg '\.columnsconfig' -t ts -C 2 echo -e "\nContext for 'settings.columns' usage:" rg 'settings\.columns' -t ts -C 2Length of output: 1439
frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue (2)
Line range hint
824-927: LGTM: Well-organized styling with proper use of SCSS features.The styling implementation makes good use of SCSS mixins for code reuse and maintains consistency in handling monospaced fonts and text overflow.
Line range hint
157-167: LGTM: Column settings migration looks correct.The change from
columnsconfigtosettings.columns.column_settingsis implemented correctly with proper null safety checks.Let's verify that all references to the old
columnsconfighave been removed:✅ Verification successful
Let me gather more information to understand if these remaining references to
columnsconfigare related to our changes or if they are part of the API/backend that should remain unchanged.
Let me try different commands to better understand the context of these changes.
Based on the gathered information, I can now provide a final response. The search results show that:
- The
columnsconfigreferences in the backend code (backend/) are part of the API/model definitions and are still needed.- The frontend has been properly migrated to use
settings.columns.column_settingsas shown in:
- The file under review (
QueryResultsTable.vue)- Other frontend files like
seqvarQuery.tsandColumnsControls.vueLGTM: The migration from
columnsconfigtosettings.columns.column_settingsis correctly implementedThe remaining
columnsconfigreferences are in the backend API and model definitions, which is expected as they need to maintain the database schema and API compatibility. The frontend has been properly updated to use the new pathsettings.columns.column_settingsconsistently across all relevant components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to columnsconfig # Expect: No occurrences of columnsconfig in the codebase rg "columnsconfig" -g '!*.{md,txt}'Length of output: 1864
Script:
#!/bin/bash # Check the actual model definition to understand the relationship ast-grep --pattern 'class SeqvarsQuery'Length of output: 41
Script:
#!/bin/bash # Check for any recent changes related to columnsconfig git log -p --grep="columnsconfig"Length of output: 33
Script:
#!/bin/bash # Check the specific file we're reviewing for any other columnsconfig references rg "columnsconfig" frontend/src/seqvars/components/QueryResults/QueryResultsTable.vueLength of output: 85
Script:
#!/bin/bash # Check the model definition in Python files rg "class SeqvarsQuery" -A 10 backend/seqvars/ # Check for any recent changes in the specific component git log -p frontend/src/seqvars/components/QueryResults/QueryResultsTable.vue # Look for any other frontend files that might be using the new settings.columns structure rg "settings\.columns\.column_settings" frontend/Length of output: 113784
frontend/ext/varfish-api/src/lib/types.gen.ts (4)
1966-1974: LGTM: Well-structured column configuration type definitionThe new
SeqvarsColumnConfigPydantictype is well-defined with clear and descriptive fields:
nameandlabelas required string fields- Optional
description,width, andvisiblefields with appropriate types
2728-2751: LGTM: Comprehensive column settings integrationThe
SeqvarsQuerySettingsColumnstype and its related types are properly integrated into the query settings structure:
- Includes proper readonly fields for tracking (
sodar_uuid,date_created,date_modified)- Maintains relationship with query settings via
querysettingsfield- Provides both full and request versions of the type
1459-1459: LGTM: Column settings properly added to query settingsThe
columnsfield is correctly added toPatchedSeqvarsQuerySettingsDetailsRequest, maintaining consistency with the refactoring goal of moving column settings into query settings.
Line range hint
1-3500: Verify removal of old column configuration typesLet's confirm that the old column configuration types have been completely removed.
✅ Verification successful
Let me analyze the output more carefully to verify the changes to column configuration types.
The search results show:
The only remaining references to
columnsconfigare in the generated types and schemas:
types.gen.ts: Two occurrences as a readonly string fieldschemas.gen.ts: Two occurrences in schema definitionsNo occurrences of
SeqvarsQueryColumnsConfigwere foundNo other
*ColumnsConfigtypes were found in the codebaseLet me verify if these remaining
columnsconfigreferences are part of the new design or leftovers.
Based on the context, I can now provide a final assessment.
The old column configuration types have been properly removed and replaced
The remaining
columnsconfigreferences are part of the new design where:
It appears as a readonly string field (UUID reference) in the base types:
SeqvarsQuery.columnsconfigSeqvarsQueryDetails.columnsconfigThe actual column configuration functionality has been moved to the new types:
SeqvarsColumnConfigPydanticSeqvarsQuerySettingsColumnsSeqvarsQueryPresetsColumnsThe old
SeqvarsQueryColumnsConfigtype has been completely removed and replaced with the new column configuration system. The remainingcolumnsconfigstring fields are just UUID references in the API types, which is the expected pattern for the new design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to old column configuration types # Test: Expect no matches for old types that should have been removed echo "Searching for old columnsconfig references..." rg "columnsconfig|SeqvarsQueryColumnsConfig" --type ts echo "Searching for any potentially missed column configuration types..." rg "ColumnsConfig" --type tsLength of output: 957
Script:
#!/bin/bash # Check the context of columnsconfig usage to understand if it's part of new design echo "Checking columnsconfig context in types.gen.ts..." rg -A 10 -B 10 "columnsconfig" frontend/ext/varfish-api/src/lib/types.gen.ts echo -e "\nChecking columnsconfig context in schemas.gen.ts..." rg -A 10 -B 10 "columnsconfig" frontend/ext/varfish-api/src/lib/schemas.gen.tsLength of output: 2915
backend/varfish/tests/drf_openapi_schema/varfish_api_schema.yaml (3)
8792-8793: LGTM: Column settings field added to query settings requestThe addition of the
columnsfield toPatchedSeqvarsQuerySettingsDetailsRequestaligns with the refactoring goal of moving column settings into query settings.
11985-11988: LGTM: Column settings field added to query settings modelThe addition of the
columnsfield toSeqvarsQuerySettingscompletes the integration of column configuration into the query settings model.
12078-12121: LGTM: Column settings schema definitionsThe new schema definitions for
SeqvarsQuerySettingsColumnsand related models provide a clean structure for column configuration with:
- Column settings list
- Individual column config with name, label, description
- Proper UUID and timestamp tracking
frontend/ext/varfish-api/src/lib/schemas.gen.ts (7)
5900-5937: Well-structured definition of$SeqvarsColumnConfigPydanticThe new schema
$SeqvarsColumnConfigPydanticis well-defined with appropriate property types and default values. The use ofrequiredfields fornameandlabelensures essential information is provided.
8750-8779: Well-defined schema for$SeqvarsQuerySettingsColumnsThe new schema
$SeqvarsQuerySettingsColumnsappropriately encapsulates the column settings, and the properties are correctly referenced. The inclusion ofsodar_uuid,date_created, anddate_modifiedensures consistency with other models.
8781-8796: Accurate Pydantic model for$SeqvarsQuerySettingsColumnsPydanticThe Pydantic representation
$SeqvarsQuerySettingsColumnsPydanticis correctly structured, matching the corresponding schema, and will facilitate accurate data validation and serialization.
8797-8805: Consistent request schema for$SeqvarsQuerySettingsColumnsRequestThe request schema
$SeqvarsQuerySettingsColumnsRequestproperly references theSeqvarsColumnConfigPydanticListand aligns with the overall design. This ensures that request payloads are validated against the expected structure.
9143-9144: Include 'columns' property in the request schemaThe
columnsproperty is added to the$SeqvarsQuerySettingsDetailsRequestschema. Ensure that this addition aligns with the backend expectations for handling this property in the request payload.Confirm that the backend is prepared to handle the
columnsproperty in request payloads. If necessary, update the backend implementation to support this property.
9039-9040:⚠️ Potential issueAdd 'columns' to the 'required' array
In the
$SeqvarsQuerySettingsDetailsschema, thecolumnsproperty has been added. Ifcolumnsis a required field, please include it in therequiredarray to ensure proper validation.Apply this diff to include
columnsin therequiredarray:}, required: ['clinvar', 'columns', 'consequence', 'date_created', 'date_modified', 'frequency', 'genotype', 'locus', 'phenotypeprio', 'predefinedquery', 'presetssetversion', 'quality', 'session', 'sodar_uuid', 'variantprio'] + , 'columns' } as const;Likely invalid or redundant comment.
6682-6684:⚠️ Potential issueRemove deprecated
columnsconfigpropertyThe
columnsconfigproperty is included in the$SeqvarsQueryDetailsschema. According to the PR objectives and AI summary, thecolumnsconfigfield has been removed from theSeqvarsQuerymodel. This property should be removed from the schema to reflect the updated model.Apply this diff to remove the
columnsconfigproperty:}, settings: { '$ref': '#/components/schemas/SeqvarsQuerySettingsDetails' }, - columnsconfig: { - type: 'string', - readOnly: true - } } as const;Likely invalid or redundant comment.
| settings: { | ||
| ...props.modelValue.settings, | ||
| columns: { | ||
| ...props.modelValue.settings.columns, | ||
| column_settings: ( | ||
| props.modelValue.settings.columns.column_settings || [] | ||
| ).map((column) => { | ||
| const res = { | ||
| ...column, | ||
| visible: data.name === column.name ? data.visible : column.visible, | ||
| } | ||
| return res | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing type safety and code organization.
The mutation logic correctly implements the new structure, but could benefit from:
- Explicit TypeScript types for the nested settings structure
- Extracting the column settings update logic into a separate function for better maintainability
Consider applying this refactor:
interface ColumnSetting {
name: string
label: string
visible: boolean
}
interface QuerySettings {
columns: {
column_settings: ColumnSetting[]
}
}
const updateColumnVisibility = (
settings: QuerySettings,
name: string,
visible: boolean
): QuerySettings => ({
...settings,
columns: {
...settings.columns,
column_settings: (settings.columns.column_settings || []).map((column) => ({
...column,
visible: name === column.name ? visible : column.visible,
})),
},
})
// Usage in applyMutation
const newData = {
...props.modelValue,
settings: updateColumnVisibility(
props.modelValue.settings,
data.name,
data.visible
),
}| }, | ||
| columns: { | ||
| '$ref': '#/components/schemas/SeqvarsQuerySettingsColumnsRequest' |
There was a problem hiding this comment.
Add 'columns' to the 'required' array
You have added the columns property to the $PatchedSeqvarsQuerySettingsDetailsRequest schema. If columns is intended to be a required field, please include it in the required array to ensure proper validation.
Apply this diff to include columns in the required array:
},
required: ['genotype', 'quality', 'consequence', 'locus', 'frequency', 'phenotypeprio', 'variantprio']
+ , 'columns'
} as const;Committable suggestion was skipped due to low confidence.
| columns: { | ||
| type: 'string', | ||
| format: 'uuid', | ||
| readOnly: true |
There was a problem hiding this comment.
Include 'columns' in the 'required' array
The columns property has been added to the $SeqvarsQuerySettings schema. If this property is required, please add it to the required array to ensure it is validated correctly.
Apply this diff to include columns in the required array:
},
required: ['clinvar', 'clinvarpresets', 'consequence', 'consequencepresets', 'date_created', 'date_modified', 'frequency', 'frequencypresets', 'genotype', 'locus', 'locuspresets', 'phenotypeprio', 'phenotypepriopresets', 'quality', 'qualitypresets', 'session', 'sodar_uuid', 'variantprio', 'variantpriopresets']
+ , 'columns'
} as const;Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (2 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (1)
Learnt from: holtgrewe PR: varfish-org/varfish-server#2046 File: frontend/src/seqvars/components/QueryEditor/QueryEditor.vue:481-496 Timestamp: 2024-10-22T09:14:05.541Z Learning: In `frontend/src/seqvars/components/QueryEditor/QueryEditor.vue`, when comparing column settings in the `columnsMatchPreset` function, the order of columns is significant and should not be altered. Do not sort the column arrays before comparison because the order matters.
🔇 Additional comments (1)
frontend/src/seqvars/components/QueryEditor/QueryEditor.vue (1)
Line range hint
1-24: Verify complete removal of column management code.Let's verify that all column-related code has been properly cleaned up across the codebase.
✅ Verification successful
Let me analyze the results and generate a final response.
The search results show that:
No references to the removed column management methods (
columnsMatchPreset,revertColumnsToPresets,collapsibleGroupOpenColumns) were found in the codebase.The column-related code that still exists is primarily in:
- Backend API types and schemas (
frontend/ext/varfish-api/src/lib/types.gen.ts)- Backend models and serializers (
backend/seqvars/)- A single frontend component
CategoryPresetsColumnsEditor.vueThese findings indicate that the column management code has been properly cleaned up in the
QueryEditor.vuecomponent and its related files. The remaining column-related code is part of the API contract and the presets management system, which is expected and correct.Column management code has been properly removed
The refactoring appears to have been done correctly. The remaining column-related code is part of the core API types and presets management system, which is separate from the removed column management functionality in the query editor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to removed column management functionality # Test 1: Search for references to removed column methods echo "Searching for references to removed column methods..." rg -i "columnsMatchPreset|revertColumnsToPresets|collapsibleGroupOpenColumns" # Test 2: Search for any remaining column-related components echo "Searching for column-related components..." rg -i "columns?(config|settings|preset)" # Test 3: Search for column-related imports echo "Searching for column-related imports..." ast-grep --pattern 'import { $_ } from $path' | rg -i "columns?"Length of output: 19906
| @@ -553,6 +495,7 @@ const collapsibleGroupOpenGeneric = ref<Record<QueryKey, boolean>>({ | |||
| phenotypeprio: true, | |||
| variantprio: true, | |||
| clinvar: true, | |||
| columns: true, | |||
There was a problem hiding this comment.
Remove unused column-related UI state.
The columns: true entry in collapsibleGroupOpenGeneric appears to be a remnant of the old column management functionality. Since column settings have been moved to query settings, this UI state is no longer needed.
Apply this diff to remove the unused state:
- columns: true,📝 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.
| columns: true, |
holtgrewe
deleted the
2042-move-column-settings-from-seqvarsquery-to-seqvarsquerysettings
branch
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation