fix: update default query of error page #4634
Conversation
|
WalkthroughThis pull request introduces a new function, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/backend-mock/api/table/list.ts (2 hunks)
- packages/effects/plugins/src/vxe-table/extends.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/backend-mock/api/table/list.ts (1)
62-66: Integration offilterMockDatafunction looks good.The event handler correctly captures query parameters and applies the filtering function to the mock data.
| @@ -38,6 +38,18 @@ function extendProxyOption( | |||
| const wrapperFn = async (params: any, _formValues: any, ...args: any[]) => { | |||
| const formValues = getFormValues(); | |||
| const data = await configFn(params, formValues, ...args); | |||
|
|
|||
| // 假如数据变少，导致总页数变少并小于当前选中页码，通过getPaginationRef获取到的页码是不正确的，需获取正确的页码再次执行 | |||
There was a problem hiding this comment.
Translate comments to English for consistency
The comment on line 42 is in Chinese. For consistency and maintainability, please translate comments to English so that all contributors can understand the code.
| @@ -38,6 +38,18 @@ function extendProxyOption( | |||
| const wrapperFn = async (params: any, _formValues: any, ...args: any[]) => { | |||
| const formValues = getFormValues(); | |||
| const data = await configFn(params, formValues, ...args); | |||
|
|
|||
| // 假如数据变少，导致总页数变少并小于当前选中页码，通过getPaginationRef获取到的页码是不正确的，需获取正确的页码再次执行 | |||
| const { total } = proxyConfig?.response as any; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using as any; define proper typings for proxyConfig.response
Using as any bypasses type checking and may introduce unexpected errors. Consider defining proper types or interfaces for proxyConfig.response to ensure type safety and improve maintainability.
| const currentTotalPage = Math.ceil(resultTotal / pageSize); | ||
| if (currentPage > currentTotalPage) { | ||
| params.page.currentPage = currentTotalPage; |
There was a problem hiding this comment.
Handle the case when currentTotalPage is zero to prevent invalid currentPage
When resultTotal is zero, currentTotalPage becomes zero due to Math.ceil(0 / pageSize). Pagination typically starts from page 1, so setting currentPage to zero may cause issues. Consider ensuring that currentTotalPage is at least 1 to avoid setting an invalid currentPage.
Apply this diff to adjust the calculation:
- const currentTotalPage = Math.ceil(resultTotal / pageSize);
+ const currentTotalPage = Math.max(1, Math.ceil(resultTotal / pageSize));📝 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.
| const currentTotalPage = Math.ceil(resultTotal / pageSize); | |
| if (currentPage > currentTotalPage) { | |
| params.page.currentPage = currentTotalPage; | |
| const currentTotalPage = Math.max(1, Math.ceil(resultTotal / pageSize)); | |
| if (currentPage > currentTotalPage) { | |
| params.page.currentPage = currentTotalPage; |
apps/backend-mock/api/table/list.ts
Outdated
| @@ -35,6 +35,22 @@ function generateMockDataList(count: number) { | |||
|
|
|||
| const mockData = generateMockDataList(100); | |||
|
|
|||
| function filterMockData(items = [], filters = {}) { | |||
| if (!Object.keys(filters)) return items; | |||
There was a problem hiding this comment.
Fix the condition to check if the filters object is empty.
The condition if (!Object.keys(filters)) will always evaluate to false because Object.keys(filters) returns an array, which is truthy even when empty. To properly check if the filters object is empty, you should check the length of the keys array.
Apply this diff to fix the condition:
-if (!Object.keys(filters)) return items;
+if (Object.keys(filters).length === 0) return items;📝 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.
| if (!Object.keys(filters)) return items; | |
| if (Object.keys(filters).length === 0) return items; |
|
ai有些建议。 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/effects/plugins/src/vxe-table/extends.ts (1)
41-52: Excellent implementation to address pagination issuesThe added logic effectively solves the problem described in the PR objectives. It correctly handles cases where the total number of items is less than the current selected page, ensuring that users always see valid data.
Some notable improvements:
- The use of
Math.max(1, Math.ceil(resultTotal / pageSize))prevents issues with zero pages.- The recursive call to
wrapperFnensures that the correct data is fetched after adjusting the page number.For improved readability, consider extracting the page calculation logic into a separate function:
function calculateCurrentPage(resultTotal: number, pageSize: number, currentPage: number): number { const currentTotalPage = Math.max(1, Math.ceil(resultTotal / pageSize)); return Math.min(currentPage, currentTotalPage); } // Then in the main function: if (Number(resultTotal)) { const newCurrentPage = calculateCurrentPage(resultTotal, pageSize, currentPage); if (newCurrentPage !== currentPage) { params.page.currentPage = newCurrentPage; return await wrapperFn(params, _formValues, ...args); } }This refactoring would make the logic more modular and easier to test.
apps/backend-mock/api/table/list.ts (2)
66-70: Consider separating filter application from pagination.The current implementation applies filters on every page change, which aligns with the PR objectives of fixing the empty result issue. However, it doesn't fully address the concern raised in the comments about applying filters only when the search button is clicked.
To better align with the suggested behavior:
- Consider storing the last applied filters separately from the current input values.
- Only update these stored filters when the search button is clicked.
- Use the stored filters for pagination, rather than the current query parameters.
This approach would prevent unexpected filter applications during pagination and ensure that the user's navigation experience matches their expectations.
Would you like assistance in implementing this separation of concerns?
Line range hint
38-70: Summary: Good progress, but further refinement neededThe changes in this PR make significant progress towards addressing the issue of empty results when filtering and paginating. The
filterMockDatafunction is well-implemented and provides robust filtering capabilities.However, to fully align with the PR objectives and the suggestions in the comments:
- Consider implementing a state management solution to store the last applied filters separately from the current input values.
- Modify the event handler to only apply new filters when a "search" or "apply filters" action is triggered.
- Use the stored (last applied) filters for pagination operations.
These additional changes would ensure that the pagination behavior matches user expectations and prevents unexpected filter applications during navigation.
Would you like assistance in outlining a more comprehensive solution that addresses these points?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/backend-mock/api/table/list.ts (2 hunks)
- packages/effects/plugins/src/vxe-table/extends.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/effects/plugins/src/vxe-table/extends.ts (1)
Line range hint
1-85: Summary of the reviewThe changes implemented in this file effectively address the pagination issues described in the PR objectives. The new logic handles cases where the total number of items is less than the current selected page, ensuring that users always see valid data.
Key points:
- The implementation aligns with the suggested approach in the PR comments.
- Previous concerns about handling zero pages and translating comments have been addressed.
- Minor suggestions for improvements have been provided, including:
- Extracting the page calculation logic into a separate function for better readability and testability.
- Improving type safety by defining proper types for
proxyConfig.response.Overall, the changes are well-implemented and solve the reported issue. Addressing the minor suggestions would further enhance the code quality.
apps/backend-mock/api/table/list.ts (1)
38-56: LGTM! Implementation addresses previous concerns.The
filterMockDatafunction looks well-implemented. It correctly handles the filtering of mock data based on the provided criteria. The function addresses the previous concerns:
- The empty object check now uses
Object.keys(filters).length === 0, which is the correct way to check for an empty object.- Falsy values are properly handled by removing null, undefined, and empty string values from the filters object before processing.
The filtering logic is robust, handling both direct value comparisons and array inclusions. This implementation should work well with the vxe-table component mentioned in the PR objectives.
|
沿用v2版本的逻辑，感觉也更符合用户的预期，用户输入筛选值点击翻页自然是想查询，俺家产品倾向这种结果😄 |
| // If the page is incorrect, make corrections and request again | ||
| const total = proxyConfig?.response?.total as string; | ||
| const { pageSize, currentPage } = params.page; | ||
| const resultTotal: number = data[total]; |
There was a problem hiding this comment.
这里会有问题，total会有嵌套，比如 result.total, 这个时候这个写法有问题
我提这个问题也是倾向于未点击查询，筛选参数不应生效，我看了下antd的protable也是这种逻辑 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/effects/plugins/src/vxe-table/extends.ts (1)
46-56: LGTM: Improved pagination handlingThe new pagination logic effectively handles cases where the current page might become invalid due to changes in the total number of items. The recursive approach to refetch data with the corrected page number is elegant.
For improved clarity, consider adding a comment explaining the recursive call:
if (currentPage > currentTotalPage) { params.page.currentPage = currentTotalPage; // Refetch data with the corrected page number return await wrapperFn(params, _formValues, ...args); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/effects/plugins/src/vxe-table/extends.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/effects/plugins/src/vxe-table/extends.ts (4)
5-5: LGTM: New utility functions importedThe addition of
getNestedValueimport is appropriate for the new logic implemented. TheisFunctionimport consolidation improves code organization.
42-42: Translate comment to English for consistencyThe comment on line 42 is in Chinese. For consistency and maintainability, please translate it to English so that all contributors can understand the code.
47-49: LGTM: Flexible total calculation addressing nested structuresThe updated
resultTotalcalculation effectively handles various scenarios, including function-based totals and nested object structures. The use ofgetNestedValueaddresses the concern raised in the previous comment about handling nested totals likeresult.total.This implementation provides the flexibility needed to work with different API response structures.
41-56: Summary: Improved pagination handling aligns with PR objectivesThe implemented changes effectively address the pagination issues mentioned in the PR objectives. By recalculating the current page based on the total number of results, the code now handles cases where filter conditions might lead to incorrect query results. This solution ensures that users will always see the correct data, even when the total number of items changes due to filtering.
Key improvements:
- Flexible total extraction from various response structures
- Proper handling of cases where the current page becomes invalid
- Automatic correction and refetching of data when necessary
These changes significantly enhance the robustness of the vxe-table component's pagination functionality.
| const response = proxyConfig?.response || {}; | ||
| const total = response?.total; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for response handling
The response handling logic looks good, but we can improve type safety. Consider defining a proper type for proxyConfig.response instead of using optional chaining.
Here's a suggestion to improve the typing:
interface ProxyConfigResponse {
total?: string | number | ((params: { data: any; $grid: any }) => number);
}
const response = (proxyConfig?.response || {}) as ProxyConfigResponse;
const total = response.total;This change will provide better type checking and autocompletion for the response object properties.
那不如两种都支持 做成配置项 |
|
这个有时间我再看下，先放着 |
|
还是决定选择保持未提交的表单，不应该被携带的原则，已在 #4847 修复。 |
Description
fixed #4617
组件：vxe-table封装
问题：添加筛选条件并点击翻页，会导致页面查询结果可能不正确(页面返回了第一页，但没有数据)
解决：根据查询返回的总数量判断当前页码是否正确，不正确则重新请求
问题复现：先把筛选条件清空查询，复制从第一页列表已有的值输入Category输入框中，点击页码3查询结果不应该为空；
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
Improvements