fix: vxeTable default sort data no effect in first query #5139
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/backend-mock/api/table/list.tsOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![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 (1)
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1)
163-172: Consider adding monitoring for this temporary fixSince this is a temporary fix pending an upstream bug fix in VxeTable:
- Consider adding a TODO comment with the relevant VxeTable issue number
- Add monitoring or logging to track if this workaround causes any side effects
- Create a follow-up ticket to remove this code once VxeTable fixes the bug
Would you like me to help create a follow-up issue to track the removal of this workaround once VxeTable fixes the bug?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue(4 hunks)
🔇 Additional comments (3)
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (3)
144-145: LGTM: Clear and purposeful state management
The addition of isLoaded ref provides a clean way to track the grid's loading state.
Line range hint 255-261: LGTM: Proper initialization handling
The implementation correctly handles the autoLoad behavior while preventing form default values from interfering with the initial load.
Let's verify the autoLoad behavior across the codebase:
✅ Verification successful
LGTM: Proper autoLoad implementation verified
The implementation is correct and consistent with the codebase patterns. The search results show that:
- The default
autoLoadis set totruein various adapter configurations (playground, docs, and apps) - In
use-vxe-grid.vue, the component explicitly setsautoLoadtofalseand takes control of the loading behavior - The reviewed code properly checks both
autoLoadfrom default options andenableProxyConfigbefore triggering the reload
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for autoLoad usage patterns
ast-grep --pattern 'proxyConfig: {
$$$
autoLoad: $_
$$$
}'
Length of output: 66
Script:
#!/bin/bash
# Let's try a different approach to search for autoLoad configurations
rg -U "autoLoad.*=|autoLoad.*:" -A 3 -B 3
# Also search for proxyConfig patterns
rg -U "proxyConfig.*=" -A 3 -B 3
Length of output: 5848
163-172: Review the setTimeout workaround implementation
While the fix addresses the immediate issue with sorting parameters, the implementation has some concerns:
- Using
setTimeoutwith 0 delay is a workaround that might be fragile - The comment indicates this is temporary until VxeTable fixes the bug
- The approach might interfere with other grid configurations or custom query handlers
Let's verify if this is the minimal required fix:
Consider these alternatives:
if (mergedOptions.proxyConfig.ajax?.query && !isLoaded.value) {
mergedOptions.proxyConfig.ajax.query = async () => {
isLoaded.value = true;
- await nextTick();
- setTimeout(() => {
- props.api.query();
- }, 0);
+ // Option 1: Use Vue's nextTick more explicitly
+ await nextTick();
+ return props.api.query();
+
+ // Option 2: Handle the initialization in a more controlled way
+ // return props.api.query({
+ // ...props.api.getTableState(),
+ // sortConfig: mergedOptions.sortConfig
+ // });
};
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/backend-mock/api/table/list.ts (1)
48-70: Consider enhancing the sorting implementationWhile the current implementation works, consider these improvements:
- Use
localeComparefor string comparisons to handle special characters and different locales properly- Add validation for sortOrder values
Here's a suggested improvement:
if (sortBy && Reflect.has(listData[0], sortBy as string)) { + const validSortOrders = ['asc', 'desc']; + if (!validSortOrders.includes(sortOrder as string)) { + sortOrder = 'asc'; // default fallback + } listData.sort((a, b) => { if (sortOrder === 'asc') { if (sortBy === 'price') { return ( Number.parseFloat(a[sortBy as string]) - Number.parseFloat(b[sortBy as string]) ); } else { - return a[sortBy as string] > b[sortBy as string] ? 1 : -1; + return (a[sortBy as string] as string).localeCompare(b[sortBy as string] as string); } } else { if (sortBy === 'price') { return ( Number.parseFloat(b[sortBy as string]) - Number.parseFloat(a[sortBy as string]) ); } else { - return a[sortBy as string] < b[sortBy as string] ? 1 : -1; + return (b[sortBy as string] as string).localeCompare(a[sortBy as string] as string); } } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backend-mock/api/table/list.ts(1 hunks)playground/src/views/examples/vxe-table/remote.vue(1 hunks)
🔇 Additional comments (5)
playground/src/views/examples/vxe-table/remote.vue (4)
28-31: LGTM: Column sorting configuration is appropriate
The addition of sortable: true to category, productName, and price columns is well-chosen as these are common fields users would want to sort by.
39-46: LGTM: Query function properly handles sort parameters
The query function now correctly passes sorting information to the API, which is essential for server-side sorting functionality.
58-58: LGTM: Refresh configuration uses proper code-based approach
The update to use { code: 'query' } instead of a boolean provides more explicit refresh behavior control.
48-52: LGTM: Sort configuration properly implements default sorting
The configuration correctly enables remote sorting and sets a default sort on the category field. This addresses the core issue mentioned in the PR.
✅ Verification successful
Based on the search results, we can see that:
- The default sort configuration exists only in
playground/src/views/examples/vxe-table/remote.vue - No other conflicting sort configurations with the category field were found
Since this is the only instance of the sort configuration for the category field in the codebase, and it's properly implemented with remote sorting enabled, I can generate the final response:
✅ Sort configuration is correctly implemented and unique
The default sort configuration for the category field is only present in this file and properly implements remote sorting, which aligns with the PR's objective without causing any conflicts in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that this is the only instance of sortConfig to avoid conflicts
rg -l 'sortConfig.*defaultSort.*category' --type vue
Length of output: 82
Script:
#!/bin/bash
# Search for sortConfig patterns without file type restriction
rg -l 'sortConfig.*defaultSort.*category'
Length of output: 43
Script:
#!/bin/bash
# Search for any other default sort configurations that might conflict
rg 'defaultSort.*category'
Length of output: 134
Script:
#!/bin/bash
# List all vue files to ensure we're searching in the right place
fd '.*\.vue$'
Length of output: 29749
apps/backend-mock/api/table/list.ts (1)
46-47: LGTM: Query parameter handling is well-implemented
Good use of structuredClone to create a copy of the data before sorting, preventing modifications to the original dataset.
|
已找到更好的解决方案 |
Description
修复VxeGrid在设置了默认排序数据时，首次加载数据时，proxyConfig.ajax.query函数的参数缺少排序数据的问题。我认为这是一个VxeGrid的Bug，提交了一个Issue：https://gitee.com/x-extends/vxe-table/issues/IBBBJ2
这个PR是一个针对性的修复方案，在上述疑似BUG被修复后，这个PR的改动应当被撤销。已找到更优的方法解决这个问题：#5141
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
formConfig, recommending the use offormOptionsinstead.