feat(data-table): add custom sorting functions and enhance query config #487
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request introduces custom sorting functions to the data table component and enhances the query configuration for the tables overview page. The custom sorting functions allow users to sort columns based on specific logic, while the enhanced query configuration provides additional metrics related to data compression and part sizes. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a validation step to ensure that the
sortingFnNameprovided in the config actually exists, to prevent runtime errors. - The
console.logstatement should be removed before merging.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| // Add the sorting function if specified | ||
| const sortingFnName = config.sortingFns?.[name] | ||
| console.log(`${name} => sortingFnName: ${sortingFnName}`) |
There was a problem hiding this comment.
suggestion: Remove or disable debug logging in production code
The console.log appears intended for debugging but may generate unwanted noise or leak information in production. Consider removing it or gating it behind a debug flag.
Suggested change
| console.log(`${name} => sortingFnName: ${sortingFnName}`) | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.log(`${name} => sortingFnName: ${sortingFnName}`) | |
| } |
| @@ -66,7 +74,8 @@ export const getColumnDefs = < | |||
| columnFormat = format as ColumnFormat | |||
| } | |||
There was a problem hiding this comment.
issue (complexity): Consider using a unified lookup mapping for custom and built-in sorting functions to reduce nested conditionals and improve code readability and maintainability.
You can reduce the nested conditionals by creating a unified lookup mapping for both custom and built-in sorting functions. For example:
// Define your built-in sorting functions mapping
const builtInSortingFns: Record<string, BuiltInSortingFn> = {
alphanumeric: 'alphanumeric',
alphanumericCaseSensitive: 'alphanumericCaseSensitive',
text: 'text',
textCaseSensitive: 'textCaseSensitive',
datetime: 'datetime',
basic: 'basic',
}
// Merge custom and built-in functions
const customSortingFns = getCustomSortingFns<TData>()
const sortingFnMap = {
...customSortingFns,
...builtInSortingFns,
}
// Then assign sortingFn if found:
const sortingFnName = config.sortingFns?.[name]
if (sortingFnName && sortingFnMap[sortingFnName]) {
columnDef.sortingFn = sortingFnMap[sortingFnName]
console.log(`${name} => sortingFnName: ${sortingFnName}`)
}Steps:
- Define a mapping for built-in functions.
- Merge your custom functions with the built-in ones.
- Replace the nested if-else with a simple lookup against the combined
sortingFnMap.
This approach minimizes control flow complexity while keeping all functionality intact.
| * @param <TData> - The type of the data in the table. | ||
| * @returns - The sorting functions for the table. | ||
| */ | ||
| export const getCustomSortingFns = <TData extends RowData>() => { |
There was a problem hiding this comment.
issue (complexity): Consider exporting a constant object instead of a factory function for the sorting functions to simplify the code if the sorting functions are static and don't need to be recreated per call, potentially relaxing the generic parameter or defining the sorting function separately to maintain type safety if needed .
If the sorting functions are static and don’t need to be recreated per call, you can simplify by exporting a constant object instead of a factory. For example, if you can relax the generic parameter to a less strict type, you might refactor as follows:
export const customSortingFns: Record<string, SortingFn<unknown>> = {
sort_column_using_actual_value: (rowA, rowB, columnId) => {
const colName = columnId.replace('readable_', '').replace('pct_', '');
const valueA = rowA.original[colName];
const valueB = rowB.original[colName];
if (typeof valueA === 'number' && typeof valueB === 'number') {
return valueA - valueB;
}
return 0;
},
};If type-safety for TData is important, you could define the sorting function separately and then include it in a constant:
const sortColumnUsingActualValue = <TData extends RowData>(
rowA: Row<TData>,
rowB: Row<TData>,
columnId: string
): number => {
const colName = columnId.replace('readable_', '').replace('pct_', '');
const valueA = rowA.original[colName as keyof TData];
const valueB = rowB.original[colName as keyof TData];
if (typeof valueA === 'number' && typeof valueB === 'number') {
return valueA - valueB;
}
return 0;
};
export const customSortingFns = {
sort_column_using_actual_value: sortColumnUsingActualValue,
} as Record<string, SortingFn<any>>;These changes remove the extra abstraction layer while preserving functionality.
Bundle ReportChanges will increase total bundle size by 2.91kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: clickhouse-monitoring-bundle-client-array-pushAssets Changed:
view changes for bundle: clickhouse-monitoring-bundle-server-cjsAssets Changed:
Files in
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
==========================================
- Coverage 77.16% 76.82% -0.34%
==========================================
Files 5 5
Lines 162 164 +2
Branches 60 60
==========================================
+ Hits 125 126 +1
- Misses 34 35 +1
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enhance data table functionality by adding custom sorting functions and extending query configuration to support flexible column sorting
New Features:
Enhancements:
Chores: