enhance: add dynamic filename for CSV exports based on filters #3043
Conversation
- Fixed the issue of downloads to be based on the filters and removed pagination on download. - Enhanced the withdraw download logs filename to include the filter information if available.
imehedi357
added
Needs: Testing
📝 WalkthroughWalkthroughFrontend switches to a paginated multi-step export with progress; the REST controller sets the exporter filename from active filters and returns it in the download URL; WithdrawLogExporter gains a filter-aware filename builder; hooks read an optional filename URL parameter to initialize the exporter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Frontend as Withdraw Page (React/Vue)
participant Controller as REST WithdrawController
participant Exporter as WithdrawLogExporter
participant Hooks as Withdraw Hooks
Note over Frontend,Controller: Export initiated with active filters
User->>Frontend: Click "Export" (filters applied)
Frontend->>Controller: GET /withdraw/export?page=1&per_page=100&filters...
Controller->>Exporter: set_filename_from_filters({user_id,status,payment_method,start_date,end_date})
Exporter-->>Controller: filename set
Controller->>Exporter: query & set_items(page,per_page)
Controller->>Exporter: set_total($withdraws->total)
Controller->>Exporter: generate_export()
Exporter-->>Controller: export file ready
Controller-->>Frontend: 200 { download_url: /admin/download?...&filename=THE_FILENAME }
Frontend->>User: redirect to download URL
User->>Hooks: GET download URL (includes filename)
Hooks->>Exporter: set_filename(sanitized_filename) -- when filename param present
Hooks-->>User: serve CSV download (browser saves CSV with filter-aware name)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/REST/WithdrawController.php (1)
394-394: Critical: Incorrect argument type passed toset_total_rows().Line 394 attempts to use
$statuses[ $args['status'] ]as the total row count, but:
$statusesis an array of status name strings:['pending', 'completed', 'cancelled']$args['status']is a numeric status code (0, 1, or 2)- This results in passing a string (e.g.,
'pending') toset_total_rows(), which expects an integer (line 97 usesabsint())The correct value should come from
$withdraw_count(populated on line 362), which contains the actual counts.🔎 Proposed fix
- $exporter->set_total_rows( $statuses[ $args['status'] ] ); + // Use total count from withdraws object + $exporter->set_total_rows( $withdraws->total );Note:
$withdraws->total(line 372) contains the total count for the current filter, which is the appropriate value for the exporter.
🧹 Nitpick comments (2)
includes/Admin/WithdrawLogExporter.php (2)
116-122: Add validation for vendor data to prevent empty filename components.If
dokan_get_store_info()returns an empty array or the store name is missing, andsanitize_title()produces an empty string from the store name, an empty element could be added to$parts, resulting in consecutive underscores in the filename (e.g.,dokan-withdraw-log-export__pending).🔎 Proposed fix to validate store name before adding
// Add vendor name if filtered by user_id. if ( ! empty( $filters['user_id'] ) ) { $store_info = dokan_get_store_info( absint( $filters['user_id'] ) ); $store_name = ! empty( $store_info['store_name'] ) ? sanitize_title( $store_info['store_name'] ) : 'vendor-' . absint( $filters['user_id'] ); - $parts[] = $store_name; + // Only add if store_name is not empty after sanitization + if ( ! empty( $store_name ) ) { + $parts[] = $store_name; + } }
124-132: Validate sanitized values before adding to filename parts.Similar to the vendor name,
sanitize_title()on status and payment_method could theoretically return empty strings if the input contains only special characters. This would result in consecutive underscores in the filename.🔎 Proposed fix to validate before adding
// Add status if filtered. if ( ! empty( $filters['status'] ) ) { - $parts[] = sanitize_title( $filters['status'] ); + $sanitized_status = sanitize_title( $filters['status'] ); + if ( ! empty( $sanitized_status ) ) { + $parts[] = $sanitized_status; + } } // Add payment method if filtered. if ( ! empty( $filters['payment_method'] ) ) { - $parts[] = sanitize_title( $filters['payment_method'] ); + $sanitized_method = sanitize_title( $filters['payment_method'] ); + if ( ! empty( $sanitized_method ) ) { + $parts[] = $sanitized_method; + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
includes/Admin/WithdrawLogExporter.phpincludes/REST/WithdrawController.phpincludes/Withdraw/Hooks.phpsrc/admin/dashboard/pages/withdraw/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Admin/WithdrawLogExporter.php (1)
includes/functions.php (1)
dokan_get_store_info(1239-1241)
⏰ 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 (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (5)
includes/Withdraw/Hooks.php (1)
50-54: LGTM! Filename parameter integration looks good.The filename is properly sanitized using
sanitize_file_name()and the nonce verification exemption is appropriate since the download action itself is already nonce-protected on line 40.includes/Admin/WithdrawLogExporter.php (1)
134-145: LGTM! Date range handling is robust.The date validation properly checks that both
strtotime()calls succeed before adding the date range to the filename. Usingwp_date()ensures timezone-aware formatting, andstrtolower()provides consistent formatting.includes/REST/WithdrawController.php (2)
380-389: LGTM! Filter parameters are properly extracted and passed.The filters are correctly extracted from the request and
$args, with appropriate fallbacks to empty strings. Theset_filename_from_filters()method will handle empty values gracefully by only including non-empty components.
404-410: LGTM! Filename parameter properly added to export URL.The
add_query_arg()function automatically handles URL encoding of the filename parameter, ensuring special characters are properly escaped.src/admin/dashboard/pages/withdraw/index.tsx (1)
550-556: The concern about performance timeouts with large datasets is unfounded. WordPress REST API enforces a 100-record per_page maximum for performance reasons, which will automatically cap the export request regardless of thetotalItemsvalue. If there are more than 100 withdrawal records, the export will be incomplete rather than causing a timeout. Consider implementing pagination for the export to handle large datasets properly.Likely an incorrect or invalid review comment.
mrabbani
added
Dev Review Done
and removed
Needs: Dev Review
dev-shahed
added
Type: Bug
and removed
Needs: Testing
|
@imehedi357 Items more than 100 can't be downloaded and not showing any error...We should also improve the export button while user waiting for start downloading the file also got some error on debug: |
- Fixed the issue of download over 100 not executing and Add polling and loading state to new React dashboard export - Fix backend pagination issues (remove unused $statuses, fix page/total_rows) - Fix old Vue dashboard export pagination Ensures exports work correctly for large datasets and provides better UX with progress feedback during export operations.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/admin/pages/Withdraw.vue (1)
670-679: Critical bug: Incorrect payment method filter in export.Line 676 uses
self.paymentMethods.id, butpaymentMethodsis an array (populated at lines 442-447), not an object. This will always evaluate toundefined, breaking payment method filtering during export.🔎 Proposed fix
const args = { is_export: true, per_page: 100, page: page, status: self.currentStatus, user_id: user_id, - payment_method: self.paymentMethods.id, + payment_method: self.filter.payment_method.id, start_date: self.filterTransactionDate.start_date, end_date: self.filterTransactionDate.end_date };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
includes/REST/WithdrawController.phpsrc/admin/dashboard/pages/withdraw/index.tsxsrc/admin/pages/Withdraw.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/REST/WithdrawController.php
⏰ 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: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (4)
src/admin/pages/Withdraw.vue (1)
672-673: LGTM! Pagination parameters correctly updated.The changes properly support the multi-step export flow:
- Fixed
per_page: 100provides a reasonable batch size for CSV export- Using the function's
pageparameter (instead ofcurrentPage) correctly supports recursive pagination viaresponse.step(line 700)src/admin/dashboard/pages/withdraw/index.tsx (3)
24-24: LGTM! Import addition for loading spinner.The
Loader2icon is properly imported and used in the export button's loading state (line 603).
91-92: LGTM! Proper state management for export UX.The state variables correctly track export status and progress, enabling the button to disable during export and display real-time progress feedback to users.
601-614: LGTM! Excellent user feedback during export.The conditional rendering provides clear visual feedback:
- Animated spinner indicates active export
- Real-time progress percentage when available
- Fallback to generic "Exporting..." message
- Clean normal state with icon and label
| <button | ||
| type="button" | ||
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[#575757] hover:bg-[#7047EB] hover:text-white" | ||
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[#575757] hover:bg-[#7047EB] hover:text-white disabled:opacity-50 disabled:cursor-not-allowed" | ||
| disabled={ isExporting } | ||
| onClick={ async () => { | ||
| setIsExporting( true ); | ||
| setExportProgress( 0 ); | ||
| try { | ||
| // Minimal placeholder; backend export flow may vary. | ||
| // Attempt to hit export endpoint via same query params. | ||
| const path = addQueryArgs( 'dokan/v2/withdraw', { | ||
| ...view, | ||
| is_export: true, | ||
| } ); | ||
| const res = await apiFetch( { path } ); | ||
| if ( res && res.url ) { | ||
| window.location.assign( res.url as string ); | ||
| let page = 1; | ||
| let isComplete = false; | ||
| let downloadUrl = null; | ||
|
|
||
| // Poll until export is complete | ||
| while ( ! isComplete ) { | ||
| const path = addQueryArgs( 'dokan/v2/withdraw', { | ||
| per_page: 100, | ||
| page: page, | ||
| search: view?.search ?? '', | ||
| status: view?.status === 'all' ? '' : view?.status, | ||
| ...filterArgs, | ||
| is_export: true, | ||
| } ); | ||
|
|
||
| const res = await apiFetch( { path } ); | ||
|
|
||
| // Update progress if available | ||
| if ( res?.percentage !== undefined ) { | ||
| setExportProgress( res.percentage ); | ||
| } | ||
|
|
||
| if ( res?.step === 'done' ) { | ||
| isComplete = true; | ||
| downloadUrl = res.url; | ||
| } else if ( res?.step ) { | ||
| // Continue to next page | ||
| page = res.step; | ||
| } else { | ||
| throw new Error( 'Invalid response from export' ); | ||
| } | ||
| } | ||
|
|
||
| if ( downloadUrl ) { | ||
| window.location.assign( downloadUrl as string ); | ||
| } | ||
| } catch ( e ) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( 'Export failed or not supported yet', e ); | ||
| alert( __( 'Export failed. Please try again.', 'dokan-lite' ) ); | ||
| } finally { | ||
| setIsExporting( false ); | ||
| setExportProgress( 0 ); | ||
| } | ||
| } } | ||
| > |
There was a problem hiding this comment.
Potential memory leak: Add cleanup for unmounted component.
The export loop (lines 560-586) continues making API calls until completion, but there's no mechanism to abort if the component unmounts during export. This can cause memory leaks and unnecessary API calls.
Additionally, consider adding a maximum iteration limit as a safety mechanism to prevent infinite loops if the backend returns unexpected responses.
🔎 Suggested improvements
Consider using an AbortController to cancel pending requests on unmount:
onClick={ async () => {
setIsExporting( true );
setExportProgress( 0 );
+ const abortController = new AbortController();
+
+ // Cleanup function
+ const cleanup = () => {
+ abortController.abort();
+ setIsExporting( false );
+ setExportProgress( 0 );
+ };
+
try {
let page = 1;
let isComplete = false;
let downloadUrl = null;
+ const MAX_ITERATIONS = 1000; // Safety limit
+ let iterations = 0;
// Poll until export is complete
while ( ! isComplete ) {
+ if ( iterations++ >= MAX_ITERATIONS ) {
+ throw new Error( 'Export timeout: too many pages' );
+ }
+
const path = addQueryArgs( 'dokan/v2/withdraw', {
per_page: 100,
page: page,
search: view?.search ?? '',
status: view?.status === 'all' ? '' : view?.status,
...filterArgs,
is_export: true,
} );
- const res = await apiFetch( { path } );
+ const res = await apiFetch( {
+ path,
+ signal: abortController.signal
+ } );Then use a useEffect cleanup:
useEffect(() => {
// Return cleanup function that aborts export on unmount
return () => {
if (isExporting) {
// abort logic here
}
};
}, [isExporting]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/withdraw/index.tsx around lines 547 to 600, the
export polling loop can continue after the component unmounts and may never
terminate; add cancellation and a safety iteration limit: create an
AbortController for the export flow and pass its signal to apiFetch calls, track
a mounted flag or use the controller to avoid calling setState after unmount,
enforce a maxIterations counter (e.g., 1000) to break the loop if responses are
unexpected, and add a useEffect cleanup that aborts the controller when the
component unmounts or when export is cancelled so pending requests stop and
state updates are prevented.
|
@mrabbani and @dev-shahed vai, Fixed the download limit issue and also added feedback to the user that the download is happening. |
There was a problem hiding this comment.
Code Review Summary
Overall, this is a solid implementation that adds dynamic filename generation for withdraw log exports and fixes export issues for large datasets. The code structure is clean and follows good practices. However, there are a few issues that should be addressed before merging.
✅ Positive Changes
- Dynamic filename generation - The
set_filename_from_filters()method effectively generates descriptive filenames based on applied filters - Bug fix for large exports - Fixed the pagination issue by using
$withdraws->totalinstead of hardcoded status array - User experience improvements - Added loading states, progress indicators, and proper export polling in the React component
- Code structure - Good separation of concerns and clear method naming
🔴 Critical Issues (Must Fix)
1. Date Format Ambiguity
Location: includes/Admin/WithdrawLogExporter.php:141-143
Using M-d format (e.g., "jan-15") doesn't include the year, which can be ambiguous for date ranges spanning years or when files are saved long-term.
Recommendation: Consider including the year if dates span different years:
$start_format = ( wp_date( 'Y', $start_timestamp ) !== wp_date( 'Y', $end_timestamp ) ) ? 'Y-M-d' : 'M-d';
$start = strtolower( wp_date( $start_format, $start_timestamp ) );
$end = strtolower( wp_date( 'Y-M-d', $end_timestamp ) );
$parts[] = $start . '_to_' . $end;2. Potential Long Filename Issue
Location: includes/Admin/WithdrawLogExporter.php:119
Very long store names could create extremely long filenames. Most filesystems have limits (typically 255 characters).
Recommendation: Consider truncating very long store names:
$store_name = ! empty( $store_info['store_name'] )
? sanitize_title( $store_info['store_name'] )
: 'vendor-' . absint( $filters['user_id'] );
// Truncate if too long (e.g., max 50 chars for store name part)
if ( strlen( $store_name ) > 50 ) {
$store_name = substr( $store_name, 0, 47 ) . '...';
}✅ Code Quality Observations
- Security ✅ - Proper use of
sanitize_text_field(),sanitize_file_name(),absint(), and nonce verification - Error handling ✅ - The React component has good error handling with try/catch and user feedback
- Consistency ✅ - The Vue component uses a similar approach (fixed
per_page: 100) - Documentation ✅ - Good inline documentation for the new method
🧪 Testing Recommendations
-
Test filename generation with various filter combinations:
- All filters applied
- No filters applied
- Only vendor filter
- Only date range
- Special characters in store names
-
Test edge cases:
- Very long store names
- Date ranges spanning multiple years
- Empty/null filter values
-
Verify the exported file has the correct name and opens correctly
-
Test the export flow with large datasets to ensure pagination works correctly
Once these issues are addressed, this PR should be ready to merge. Great work on improving the user experience with dynamic filenames and fixing the export pagination issue!
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
- Shorten base name from 'dokan-withdraw-log-export' to 'dokan-withdraw-log' - Truncate vendor names >50 chars using '-trunc' (replaces '...' to avoid sanitization issues) - Always include year in date format (Y-M-d) for clarity Max filename length: ~160 chars
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Admin/WithdrawLogExporter.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Admin/WithdrawLogExporter.php (1)
includes/functions.php (1)
dokan_get_store_info(1239-1241)
🪛 GitHub Check: Run PHPCS inspection
includes/Admin/WithdrawLogExporter.php
[failure] 126-126:
Whitespace found at end of line
[failure] 121-121:
Whitespace found at end of line
⏰ 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 (2)
includes/Admin/WithdrawLogExporter.php (2)
38-38: LGTM: Default filename shortened.The simplified base filename aligns well with the dynamic naming strategy introduced below.
112-153: Code is well-implemented and safely handles filename length.The implementation correctly handles all components:
- Sanitizes all user inputs with
sanitize_title()andabsint()- Validates timestamps before formatting dates
- Truncates vendor names to 50 characters
- Conditionally includes only applied filters
The worst-case filename length (with all filters applied: vendor + status + payment method + date range) totals approximately 125 characters, well within the ~160-character limit mentioned in the commit message and below the standard 255-character filesystem limit. The defined statuses (
pending,completed,cancelled) and payment methods (paypal,bank,skrill) are limited in length, and the vendor name truncation at 50 chars provides the primary safeguard.No additional changes needed.
| $store_name = ! empty( $store_info['store_name'] ) | ||
| ? sanitize_title( $store_info['store_name'] ) | ||
| : 'vendor-' . absint( $filters['user_id'] ); | ||
|
|
There was a problem hiding this comment.
Fix trailing whitespace.
Static analysis detected trailing whitespace on lines 121 and 126. Please remove to comply with coding standards.
🔎 Proposed fix
$store_name = ! empty( $store_info['store_name'] )
? sanitize_title( $store_info['store_name'] )
: 'vendor-' . absint( $filters['user_id'] );
-
+
// Truncate if too long (max 50 chars for store name part).
if ( strlen( $store_name ) > 50 ) {
$store_name = substr( $store_name, 0, 44 ) . '-trunc';
}
-
+
$parts[] = $store_name;Also applies to: 126-126
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 121-121:
Whitespace found at end of line
🤖 Prompt for AI Agents
In includes/Admin/WithdrawLogExporter.php around lines 121 and 126, there are
trailing whitespace characters at the ends of those lines; remove the trailing
spaces so the lines end immediately after the intended code/text (no extra
spaces or tabs) and save the file to comply with coding standards and static
analysis.
|
@mrabbani vai, Thanks for the feedback.
The max filename with all filters should be now within ~160 chars. |
imehedi357
added
Needs: Dev Review
All Submissions:
Changes proposed in this Pull Request:
Added
set_filename_from_filters()method inWithdrawLogExporter.php:Updated
WithdrawController.php:set_filename_from_filters()before generating the CSV fileUpdated
Hooks.php:export()Fixed export button in
index.tsx:per_page,page,search,status, andfilterArgsto ensure all filters are applied. (This was the bug that caused the filter to not working previously.)totalItemsis used to fetch all matching records in a single request to avoid pagination only when downloading.Files Changed:
includes/Admin/WithdrawLogExporter.php- Addedset_filename_from_filters()methodincludes/REST/WithdrawController.php- Callset_filename_from_filters()and pass filename in URLincludes/Withdraw/Hooks.php- Read filename from URL and set it before exportsrc/admin/dashboard/pages/withdraw/index.tsx- Fixed per_page handling and export query parametersRelated Pull Request(s)
Closes
How to test the changes in this Pull Request:
dokan-withdraw-log-export.csvdokan-withdraw-log-export_store-name.csvdokan-withdraw-log-export_cancelled.csvdokan-withdraw-log-export_dec-01_to_dec-31.csvdokan-withdraw-log-export_store-name_pending_paypal_dec-01_to_dec-31.csvChangelog entry
Enhanced Withdraw Export CSV Filename
Previously, the withdraw export CSV file was always downloaded with a generic filename
dokan-withdraw-log-export.csvregardless of any filters applied. Additionally, the export query parameters were not explicitly passed, which led to issues with the filter not working.Now, the exported CSV filename dynamically includes the applied filter information, making it easier to identify and organize downloaded files. The export also explicitly passes all filter parameters to ensure all matching records are exported in a single request, bypassing pagination. For example, exporting cancelled withdrawals for "My Store" in December will generate a filename like
dokan-withdraw-log-export_my-store_cancelled_dec-01_to_dec-31.csv.Before Changes
dokan-withdraw-log-export.csv...viewspread which may not have included all filter parametersAfter Changes
dokan-withdraw-log-export.csv(no filters)dokan-withdraw-log-export_my-store.csv(vendor filter)dokan-withdraw-log-export_cancelled.csv(status filter)dokan-withdraw-log-export_my-store_pending_paypal_dec-01_to_dec-31.csv(all filters used)per_page,page,search,status, andfilterArgsto ensure all filters are appliedFeature Video (optional)
N/A
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.