[WEB-2628] fix: Sorting by estimates #5988
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the handling of estimation points within various components of the application. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
- Fix a bug with with sorting on special fields on spreadsheet layout - Cleanup logging
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
web/core/local-db/utils/load-workspace.ts (1)
107-109: Consider adding error logging for database initialization failuresWhile the null check prevents runtime errors, silently returning might hide database initialization issues. Consider logging the error state.
if (!persistence.db || !persistence.db.exec) { + log("Database initialization failed - database or exec method is undefined"); return; }web/core/constants/spreadsheet.ts (1)
82-84: Consider adding a comment explaining the estimate point structure.To improve maintainability, consider adding a comment explaining that
estimate_point__keyrefers to a nested key property within the estimate point structure.estimate: { title: "Estimate", + // estimate_point is an object with a key property used for sorting ascendingOrderKey: "estimate_point__key", ascendingOrderTitle: "Low", descendingOrderKey: "-estimate_point__key", descendingOrderTitle: "High",web/core/local-db/utils/query-constructor.ts (2)
79-82: Consider adding error handling for invalid estimate pointsWhile the SQL construction is correct, consider adding error handling for cases where estimate points might be invalid or null. This would improve the robustness of the sorting functionality.
} else if (order_by.includes("estimate")) { - sql += ` s.key as ${name} `; + sql += ` COALESCE(s.key, 0) as ${name} `; } else {
24-24: Consider adding documentation for special ordering logicThe SQL construction for special ordering cases (including the new estimate point handling) is complex. Consider adding documentation explaining the different cases and their implications.
web/core/local-db/utils/query.utils.ts (1)
339-341: LGTM! Consider adding tests for estimate point sorting.The code correctly includes the estimate_point field when sorting by estimate_point__key, which is essential for the sorting functionality to work properly.
Consider adding test cases to verify:
- Sorting by estimate_point__key includes the estimate_point field
- The sorting works correctly in both ascending and descending order
web/core/store/issue/helpers/base-issues.store.ts (1)
289-297: Add a TODO for the temporary codeThe comment
// Temporary code to fix no load order byindicates a temporary fix. Consider adding aTODOcomment or creating an issue to ensure this code is revisited and properly addressed in the future.Would you like me to create a GitHub issue to track this temporary code for future improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/types/src/view-props.d.ts(1 hunks)web/core/constants/spreadsheet.ts(1 hunks)web/core/local-db/storage.sqlite.ts(1 hunks)web/core/local-db/utils/load-workspace.ts(4 hunks)web/core/local-db/utils/query-constructor.ts(2 hunks)web/core/local-db/utils/query.utils.ts(2 hunks)web/core/store/issue/helpers/base-issues.store.ts(15 hunks)
🔇 Additional comments (12)
web/core/local-db/utils/load-workspace.ts (2)
90-94: LGTM: Array handling improvements
The changes improve the clarity of array manipulation by using explicit reassignment instead of mutation. The null check for estimates is properly maintained.
118-118: LGTM: Variable name typo fix
The correction of the variable name from 'memebers' to 'members' improves code clarity.
packages/types/src/view-props.d.ts (1)
43-44: LGTM! Verify estimate key changes across the codebase.
The addition of estimate_point__key options aligns with the PR objectives to fix sorting by estimates.
Let's verify the consistency of these changes across the codebase:
web/core/constants/spreadsheet.ts (1)
82-84: LGTM! Verify the estimate point structure.
The change from estimate_point to estimate_point__key looks correct for fixing the sorting functionality. This suggests that estimate points are now structured objects with a key property.
Let's verify the estimate point structure in the API response:
✅ Verification successful
Estimate point structure correctly updated.
The estimate_point__key structure is consistent across the codebase and properly handled by the SpreadsheetEstimateColumn component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for estimate point structure in API-related files
rg -g '*.ts' -g '*.tsx' -A 5 'estimate_point'
# Search for the EstimateColumn component to verify handling
ast-grep --pattern 'SpreadsheetEstimateColumn'
Length of output: 41698
web/core/local-db/utils/query-constructor.ts (2)
21-22: LGTM: Consistent mapping for estimate point keys
The new mappings correctly handle both ascending and descending sort orders for estimate points while maintaining consistency with the existing pattern.
90-92: Verify index existence on estimate_points table
The LEFT JOIN with estimate_points table is correctly implemented. However, to ensure optimal query performance, verify that appropriate indexes exist.
✅ Verification successful
Index on estimate_points table exists
The necessary index on the estimate_points table is present, ensuring optimal query performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for indexes on estimate_points table
# Note: This assumes SQLite schema is available in the repository
# Search for CREATE TABLE or CREATE INDEX statements related to estimate_points
rg -A 5 "CREATE (TABLE|INDEX).*estimate_points"
Length of output: 523
web/core/local-db/utils/query.utils.ts (2)
Line range hint 1-385: Verify end-to-end sorting functionality for estimates.
The changes look good overall. Let's verify the complete sorting functionality:
#!/bin/bash
# Description: Verify estimate point sorting implementation
# Expected: Find all related estimate point handling code
# Check for estimate point related SQL queries
rg "estimate_point" --type ts
# Check for order_by handling with estimate points
rg "order_by.*estimate_point" --type ts -A 2
# Check for any potential edge cases in estimate point handling
rg "estimate_point__key|estimate_point" --type test48-48: LGTM! Verify the order_by behavior across layout switches.
The condition correctly handles order_by when switching from spreadsheet layout to other layouts.
Let's verify the SPECIAL_ORDER_BY keys and their usage:
✅ Verification successful
Verified: The order_by handling for non-spreadsheet layouts is correctly implemented and SPECIAL_ORDER_BY is properly defined and utilized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SPECIAL_ORDER_BY usage and layout-specific ordering
# Expected: Find SPECIAL_ORDER_BY definition and any layout-specific order_by handling
# Check for SPECIAL_ORDER_BY definition
rg "SPECIAL_ORDER_BY.*=.*{" -A 5
# Check for other layout-specific order_by handling
rg "layout.*[!=]=.*[\"']spreadsheet[\"'].*order_by" -A 2
Length of output: 13351
Script:
#!/bin/bash
# Description: Check SPECIAL_ORDER_BY usage and layout-specific order_by handling with corrected file type
# Expected: Find SPECIAL_ORDER_BY definition and any layout-specific order_by handling
# Check for SPECIAL_ORDER_BY definition
rg "SPECIAL_ORDER_BY\s*=\s*{" --type ts -A 5
# Check for other layout-specific order_by handling
rg "layout\s*!=\s*['\"]spreadsheet['\"]\s*&&\s*order_by" --type ts -A 2
Length of output: 236
Script:
#!/bin/bash
# Description: Check SPECIAL_ORDER_BY usage and layout-specific order_by handling with corrected regex
# Expected: Find SPECIAL_ORDER_BY definition and any layout-specific order_by handling
# Check for SPECIAL_ORDER_BY definition
rg "SPECIAL_ORDER_BY\\s*=\\s*\\{" --type ts -A 5
# Check for other layout-specific order_by handling
rg "layout\\s*!=\\s*[\"']spreadsheet[\"']\\s*&&\\s*order_by" --type ts -A 2
Length of output: 600
web/core/local-db/storage.sqlite.ts (1)
302-304: Verify query constructor compatibility with new estimate key.
Since the PR changes the estimate key from estimate_point to estimate_point__key, ensure that the issueFilterQueryConstructor and related query utilities properly handle the new key format.
Let's verify the query construction implementation:
#!/bin/bash
# Description: Check query constructor implementation for estimate key handling
# Search for estimate-related query construction
rg -A 5 'estimate_point' web/core/local-db/utils/query-constructor.ts
# Check for any direct references to the old key
rg -l 'estimate_point[^_]' web/core/store/issue/helpers/base-issues.store.ts (3)
39-39: Importing SPECIAL_ORDER_BY
The import statement correctly adds SPECIAL_ORDER_BY from "@/local-db/utils/query-constructor", which is necessary for the updated sorting logic.
168-169: Update ISSUE_ORDERBY_KEY with estimate point keys
The mapping correctly adds estimate_point__key and -estimate_point__key to ISSUE_ORDERBY_KEY, aligning with the new keys used for estimate points in sorting.
1718-1721: Ensure all calls to populateIssueDataForSorting include projectId
The function populateIssueDataForSorting now includes projectId as a parameter. Please verify that all calls to this function have been updated to include the projectId argument to prevent potential runtime errors.
Run the following script to find any calls to populateIssueDataForSorting missing the projectId parameter:
| let objects: IEstimatePoint[] = []; | ||
| (estimates || []).forEach((estimate: IEstimate) => { | ||
| if (estimate?.points) { | ||
| objects.concat(estimate.points); | ||
| objects = objects.concat(estimate.points); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Schema does not support the new estimate point key structure
The estimatePointSchema does not include estimate_point__key. Please update the schema to support the new key structure.
🔗 Analysis chain
Verify schema compatibility with estimate point key changes
Given that the PR changes the estimate point key from estimate_point to estimate_point__key, we should verify that the database schema and related components support this change.
Also applies to: 127-127
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the estimate point schema and related components support the new key structure
# Check the estimate point schema definition
rg -A 10 "estimatePointSchema.*="
# Check for any references to the old and new estimate point keys
rg "estimate_point[^_]"
rg "estimate_point__key"
Length of output: 35316
| @@ -300,6 +300,7 @@ export class Storage { | |||
| const { cursor, group_by, sub_group_by } = queries; | |||
|
|
|||
| const query = issueFilterQueryConstructor(this.workspaceSlug, projectId, queries); | |||
| log("#### Query", query); | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent logging patterns detected in query-related operations.
The added logging statement is not consistent with existing logging practices across other query-related methods. Please ensure that logging is applied uniformly to facilitate debugging and maintenance.
web/core/local-db/storage.sqlite.ts(Line 303)
🔗 Analysis chain
LGTM! Helpful debug logging addition.
The added logging statement for the constructed query is well-placed and consistent with the existing logging pattern in the file. This will be particularly useful for debugging the sorting functionality changes related to estimate_point__key.
Let's verify the logging implementation is consistent across query-related operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent logging patterns across query-related operations
# Search for logging patterns in query-related code
rg -A 2 'log\(' web/core/local-db/storage.sqlite.ts
Length of output: 3677
| case "estimate_point": { | ||
| // return if project Id does not exist | ||
| if (!projectId) break; | ||
| // get the estimate ID for the current Project | ||
| const currentProjectEstimateId = | ||
| this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId); | ||
| // return if current Estimate Id for the project is not available | ||
| if (!currentProjectEstimateId) break; | ||
| // get Estimate based on Id | ||
| const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId); | ||
| // If Estimate is not available, then return | ||
| if (!estimate) break; | ||
| // Get Estimate Value | ||
| const estimateKey = estimate?.estimatePointById(dataIds as string)?.key; | ||
|
|
||
| // If Value string i not available or empty then return | ||
| if (estimateKey === undefined) break; | ||
|
|
||
| dataValues.push(estimateKey); | ||
| } |
There was a problem hiding this comment.
Handle dataIds type correctly in estimate_point case
In the "estimate_point" case, dataIds is cast to string with dataIds as string, but dataIds can be string | string[] | null | undefined. Casting without checking can cause runtime errors if dataIds is an array or null.
Consider modifying the code to handle both string and string[] types for dataIds. Apply the following diff to fix the issue:
case "estimate_point": {
if (!projectId) break;
const currentProjectEstimateId =
this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId);
if (!currentProjectEstimateId) break;
const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId);
if (!estimate) break;
- const estimateKey = estimate?.estimatePointById(dataIds as string)?.key;
- if (estimateKey === undefined) break;
- dataValues.push(estimateKey);
+ const dataIdsArray = Array.isArray(dataIds) ? dataIds : [dataIds];
+ for (const id of dataIdsArray) {
+ if (!id) continue;
+ const estimateKey = estimate.estimatePointById(id)?.key;
+ if (estimateKey !== undefined) {
+ dataValues.push(estimateKey);
+ }
+ }
}This change ensures that both single and multiple dataIds are handled correctly, preventing potential runtime errors.
📝 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.
| case "estimate_point": { | |
| // return if project Id does not exist | |
| if (!projectId) break; | |
| // get the estimate ID for the current Project | |
| const currentProjectEstimateId = | |
| this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId); | |
| // return if current Estimate Id for the project is not available | |
| if (!currentProjectEstimateId) break; | |
| // get Estimate based on Id | |
| const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId); | |
| // If Estimate is not available, then return | |
| if (!estimate) break; | |
| // Get Estimate Value | |
| const estimateKey = estimate?.estimatePointById(dataIds as string)?.key; | |
| // If Value string i not available or empty then return | |
| if (estimateKey === undefined) break; | |
| dataValues.push(estimateKey); | |
| } | |
| case "estimate_point": { | |
| // return if project Id does not exist | |
| if (!projectId) break; | |
| // get the estimate ID for the current Project | |
| const currentProjectEstimateId = | |
| this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId); | |
| // return if current Estimate Id for the project is not available | |
| if (!currentProjectEstimateId) break; | |
| // get Estimate based on Id | |
| const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId); | |
| // If Estimate is not available, then return | |
| if (!estimate) break; | |
| // Get Estimate Value | |
| const dataIdsArray = Array.isArray(dataIds) ? dataIds : [dataIds]; | |
| for (const id of dataIdsArray) { | |
| if (!id) continue; | |
| const estimateKey = estimate.estimatePointById(id)?.key; | |
| if (estimateKey !== undefined) { | |
| dataValues.push(estimateKey); | |
| } | |
| } | |
| } |
This PR makes changes to fix sorting of estimates. This fixes the issue in All the places required.
This PR also changes the Key for estimates from
estimate_pointtoestimate_point__key.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates aim to enhance user experience by improving data accuracy and retrieval efficiency within the application.