Set default enableSorting and enableHiding to false#3070
Conversation
📝 WalkthroughWalkthroughMultiple admin table pages had sorting disabled on several columns; core DataView components now normalize fields to set Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/components/dataviews/DataViewTable.tsx:
- Around line 74-81: The code sets column defaults after spreading each field so
per-field settings get overwritten; update the normalizedFields mapping to apply
defaults first and then spread field properties (i.e., { enableSorting: false,
enableHiding: false, ...field }) so any explicit field.enableSorting or
field.enableHiding values take precedence (refer to the normalizedFields mapping
where fields.map is used).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/admin/dashboard/pages/reverse-withdrawal/ReverseWithdrawalTransaction.tsxsrc/admin/dashboard/pages/reverse-withdrawal/index.tsxsrc/admin/dashboard/pages/vendors.tsxsrc/admin/dashboard/pages/withdraw/index.tsxsrc/components/dataviews/AdminDataViewTable.tsxsrc/components/dataviews/DataViewTable.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/dataviews/DataViewTable.tsx (1)
src/vendor-dashboard/reports/analytics/components/report-chart/index.js (1)
fields(351-351)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
🔇 Additional comments (7)
src/admin/dashboard/pages/withdraw/index.tsx (1)
108-218: LGTM! Explicit sorting disable aligns with PR objectives.The explicit
enableSorting: falseon Vendor, Amount, Charge, Payable, and Date fields correctly addresses the UI issues. WhileAdminDataViewTablenow applies global normalization withenableSorting: falseby default, these explicit declarations serve as clear documentation of intent and provide safety if the global behavior changes in the future.src/admin/dashboard/pages/reverse-withdrawal/ReverseWithdrawalTransaction.tsx (1)
65-76: LGTM!Disabling sorting on the Date field addresses the reported UI issue where sorting controls were present but non-functional.
src/components/dataviews/AdminDataViewTable.tsx (2)
95-102: LGTM! Correct spread order for default normalization.The normalization correctly places defaults before the field spread, allowing individual fields to override these defaults if needed in the future. This is the right pattern for maintainability and extensibility.
114-119: Correctly applies normalized fields to the filter pipeline.src/admin/dashboard/pages/reverse-withdrawal/index.tsx (1)
67-88: LGTM!Sorting disabled on Amount and Date fields, consistent with the broader PR objective to disable non-functional sorting controls.
src/components/dataviews/DataViewTable.tsx (1)
113-118: Correctly applies normalized fields to the filter pipeline.src/admin/dashboard/pages/vendors.tsx (1)
326-326: Change aligns with PR objectives.Disabling sorting for the 'registered' column is consistent with the fix for broken sorting UI. However, note that:
- The initial view state (line 35) still sets
sort: { field: 'registered', direction: 'desc' }- Lines 159-166 contain sorting logic specifically handling the 'registered' field
This means data will load pre-sorted by registration date, but users cannot change it via UI—which may be intentional. If the goal is to restore sorting functionality later, the backend logic is preserved. Otherwise, consider adding a comment clarifying that the default sort order is intentional while user-controlled sorting is disabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/admin/dashboard/pages/withdraw/index.tsx (1)
694-749: Pre-existing: Polling logic has a minor issue with the initial call.This is pre-existing code, but worth noting: on line 748,
checkStatus()is called without theresolveparameter. InsidecheckStatus, whenresolveis undefined, the function still completes but the outerawaitmay not behave as expected during the recursive polling. The current implementation works becauseresolve?.()is safely guarded, but this pattern could be simplified.♻️ Consider a cleaner polling approach
- const pollExportStatus = async ( exportId ) => { - const maxAttempts = 60; // Maximum 5 minutes (60 * 5 seconds) - let attempts = 0; - - const checkStatus = async ( resolve ) => { - try { - const statusResponse = await apiFetch( { - path: `/dokan/v1/reports/withdraws/export/${ exportId }/status`, - method: 'GET', - } ); - - console.log( 'Export status:', statusResponse ); - - if ( statusResponse.percent_complete === 100 ) { - // Export is complete, download the file - if ( statusResponse.download_url ) { - // Create a temporary link to download the file - const link = document.createElement( 'a' ); - link.href = statusResponse.download_url; - link.download = ''; // Let the browser determine the filename - document.body.appendChild( link ); - link.click(); - document.body.removeChild( link ); - - console.log( 'Export completed and downloaded' ); - } else { - throw new Error( 'Download URL not available' ); - } - } else { - // Still processing, check again - attempts++; - if ( attempts < maxAttempts ) { - await new Promise( - ( res ) => - setTimeout( - async () => await checkStatus( res ), - 5000 - ) // Check again in 5 seconds - ); - } else { - throw new Error( 'Export timeout - please try again' ); - } - } - } catch ( error ) { - console.error( 'Error checking export status:', error ); - alert( __( 'Export failed. Please try again.', 'dokan-lite' ) ); - } - - if ( resolve ) { - resolve(); - } - }; - - // Start status checking - await checkStatus(); - }; + const pollExportStatus = async ( exportId ) => { + const maxAttempts = 60; + const delayMs = 5000; + + for ( let attempt = 0; attempt < maxAttempts; attempt++ ) { + try { + const statusResponse = await apiFetch( { + path: `/dokan/v1/reports/withdraws/export/${ exportId }/status`, + method: 'GET', + } ); + + if ( statusResponse.percent_complete === 100 ) { + if ( ! statusResponse.download_url ) { + throw new Error( 'Download URL not available' ); + } + const link = document.createElement( 'a' ); + link.href = statusResponse.download_url; + link.download = ''; + document.body.appendChild( link ); + link.click(); + document.body.removeChild( link ); + return; + } + + await new Promise( ( r ) => setTimeout( r, delayMs ) ); + } catch ( error ) { + console.error( 'Error checking export status:', error ); + alert( __( 'Export failed. Please try again.', 'dokan-lite' ) ); + return; + } + } + + alert( __( 'Export timeout - please try again.', 'dokan-lite' ) ); + };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/admin/dashboard/pages/withdraw/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (2)
src/admin/dashboard/pages/withdraw/index.tsx (2)
108-218: Sorting disabled on selected columns aligns with PR objective.The
enableSorting: falseadditions to vendor, amount, charge, payable, and date fields address the reported UI issues where sorting controls weren't functioning.Note that
status,method,details, andnotefields don't explicitly setenableSorting. Per the PR title, if the default is nowfalseat the DataView component level, this is consistent. However, if any of these fields should remain sortable (e.g.,statusordate), verify that the backend API actually supports sorting on those fields before re-enabling.
515-524: Explicit fields array correctly excludesvendorsince it's used astitleField.The hardcoded fields list intentionally omits
vendorbecause it's specified astitleFieldon line 512, meaning it's rendered as the primary row identifier rather than a regular column. This is consistent with DataView patterns where the title field receives special treatment.
…) at vendors page
|
Note: this PR partially solve the issue, not close the issue |
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Default table interactions have been refined
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.