Fix race condition for woocommerce_admin_reports_list filter
#59897
Conversation
Addresses the race condition issue where reports registered via the woocommerce_admin_reports_list filter after initial application load were not being recognized. This follows the same pattern used for the woocommerce_admin_pages_list filter fix. Changes: - Rename get-reports.js to use-reports.js for clearer purpose - Add useReports hook with race condition mitigation - Update controller.js to use useReports in usePages - Add withReports HOC to report/index.js for class component compatibility - Maintain backward compatibility with existing getReports function 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…mmerce, woocommerce/client/admin
📝 WalkthroughWalkthroughThe changes refactor how analytics reports are sourced and consumed in the WooCommerce admin client. They replace a static function-based approach with a React hook ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Report Component
participant HOC as withReports HOC
participant Hook as useReports Hook
participant Data as Reports Data Source
UI->>HOC: Render (wrapped by withReports)
HOC->>Hook: Call useReports()
Hook->>Data: Fetch reports (getReports)
Data-->>Hook: Return current reports array
Hook-->>HOC: Provide reports array
HOC-->>UI: Inject reports as prop
UI->>UI: Use reports prop for rendering
Note over Hook,Data: On new report hooks, useReports updates reports and triggers re-render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
|
Size Change: +2.72 kB (+0.05%) Total Size: 5.91 MB
|
woocommerce_admin_reports_list filter
Testing GuidelinesApart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
| useEffect( () => { | ||
| const handleHookAdded = ( hookName ) => { | ||
| if ( | ||
| hookName === REPORTS_FILTER && |
There was a problem hiding this comment.
This is a neat workaround! It does make me wonder, especially since we're using this pattern multiple times, if we should turn this into a hook wrapper when we want to re-render content.
const applyFiltersHook = ( hookName, ...args ) => {
const [ hookValue, setHookValue ] = useState( null );
if (
hookName === hookName &&
didFilter( hookName ) > 0
) {
// Recalculate hook value here.
setHookValue( value );
}
return hookValue;
}I'm not sure if there are issues with this though. I'm really surprised there's not an upstream fix for this since it's such a common pattern.
There was a problem hiding this comment.
Great suggestion! I've implemented the reusable hook wrapper approach you suggested in 73bb853.
…e condition mitigation - Simplified useReports and usePages hooks to leverage the new useFilterHook utility, improving race condition handling for dynamically added reports and pages. - Removed redundant state management and effects in favor of a more generic solution. - Introduced useFilterHook to encapsulate filter logic and ensure components re-render correctly when new hooks are added.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/woocommerce/client/admin/client/utils/use-filter-hook.ts (1)
50-51: Add unit tests and improve robustnessThe hook needs unit tests as required by the coding guidelines, and should be more defensive about the WordPress hooks API availability.
+// Add type guard for better TypeScript support +if (typeof addAction === 'undefined' || typeof removeAction === 'undefined') { + console.warn('useFilterHook: WordPress hooks API not available'); + return getterFn(); +} + return value;Do you want me to generate comprehensive unit tests for this hook? The tests should cover:
- Normal operation with filter hooks
- Race condition scenarios
- Error handling in getter functions
- Cleanup behavior
- Edge cases with invalid inputs
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/woocommerce/client/admin/client/analytics/report/use-reports.js(3 hunks)plugins/woocommerce/client/admin/client/layout/controller.js(5 hunks)plugins/woocommerce/client/admin/client/utils/use-filter-hook.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/woocommerce/client/admin/client/analytics/report/use-reports.js
- plugins/woocommerce/client/admin/client/layout/controller.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{php,js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/code-quality.mdc)
**/*.{php,js,jsx,ts,tsx}: Guard against unexpected inputs
Sanitize and validate any potentially dangerous inputs
Ensure code is backwards compatible
Write code that is readable and intuitive
Ensure code has unit or E2E tests where applicable
Files:
plugins/woocommerce/client/admin/client/utils/use-filter-hook.ts
**/*.{php,js,ts,jsx,tsx}
⚙️ CodeRabbit Configuration File
**/*.{php,js,ts,jsx,tsx}: Don't trust that extension developers will follow the best practices, make sure the code:
- Guards against unexpected inputs.
- Sanitizes and validates any potentially dangerous inputs.
- Is backwards compatible.
- Is readable and intuitive.
- Has unit or E2E tests where applicable.
Files:
plugins/woocommerce/client/admin/client/utils/use-filter-hook.ts
⏰ 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). (11)
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: JavaScript - @woocommerce/admin-library [unit]
- GitHub Check: Lint - @woocommerce/admin-library
- GitHub Check: build
- GitHub Check: Check Asset Sizes
🔇 Additional comments (1)
plugins/woocommerce/client/admin/client/utils/use-filter-hook.ts (1)
4-5: LGTM - Imports are appropriateThe imports correctly bring in the necessary React and WordPress hooks APIs for the functionality.
| export function useFilterHook< T >( | ||
| filterName: string, | ||
| getterFn: () => T, | ||
| dependencies: React.DependencyList = [] | ||
| ): T { | ||
| const [ value, setValue ] = useState< T >( getterFn ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to guard against unexpected inputs
The function lacks validation for its parameters, which could lead to runtime errors:
filterNameshould be validated as a non-empty stringgetterFnshould be validated as a function- Consider error handling if
getterFn()throws an exception during initial state setup
export function useFilterHook< T >(
filterName: string,
getterFn: () => T,
dependencies: React.DependencyList = []
): T {
+ if (!filterName || typeof filterName !== 'string') {
+ throw new Error('useFilterHook: filterName must be a non-empty string');
+ }
+ if (typeof getterFn !== 'function') {
+ throw new Error('useFilterHook: getterFn must be a function');
+ }
+
- const [ value, setValue ] = useState< T >( getterFn );
+ const [ value, setValue ] = useState< T >( () => {
+ try {
+ return getterFn();
+ } catch (error) {
+ console.error(`useFilterHook: Error in getterFn for filter "${filterName}":`, error);
+ throw error;
+ }
+ });📝 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.
| export function useFilterHook< T >( | |
| filterName: string, | |
| getterFn: () => T, | |
| dependencies: React.DependencyList = [] | |
| ): T { | |
| const [ value, setValue ] = useState< T >( getterFn ); | |
| export function useFilterHook< T >( | |
| filterName: string, | |
| getterFn: () => T, | |
| dependencies: React.DependencyList = [] | |
| ): T { | |
| if (!filterName || typeof filterName !== 'string') { | |
| throw new Error('useFilterHook: filterName must be a non-empty string'); | |
| } | |
| if (typeof getterFn !== 'function') { | |
| throw new Error('useFilterHook: getterFn must be a function'); | |
| } | |
| const [ value, setValue ] = useState< T >( () => { | |
| try { | |
| return getterFn(); | |
| } catch (error) { | |
| console.error( | |
| `useFilterHook: Error in getterFn for filter "${filterName}":`, | |
| error | |
| ); | |
| throw error; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In plugins/woocommerce/client/admin/client/utils/use-filter-hook.ts around lines
17 to 22, add input validation to ensure filterName is a non-empty string and
getterFn is a function before using them. Also, wrap the call to getterFn() in a
try-catch block to handle any exceptions during initial state setup, providing
fallback behavior or error logging as needed to prevent runtime errors.
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR addresses a race condition issue with the
woocommerce_admin_reports_listfilter, similar to the one that was previously fixed for thewoocommerce_admin_pages_listfilter in #47696 and #58834.Problem: Reports registered via the
woocommerce_admin_reports_listfilter after the initial application load were not being recognized by the routing system and breadcrumb generation.Solution: Implement the same race condition mitigation pattern used for pages, providing reactive updates when new reports are added to the filter.
Changes made:
get-reports.jstouse-reports.jsto better reflect its purposeuseReportshook with race condition mitigation that watches for new hookscontroller.jsto useuseReportsin theusePageshookwithReportsHOC toreport/index.jsfor class component compatibilitygetReportsfunctionHow to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
test-reports-race-condition.php.zip plugin that registers reports via the
woocommerce_admin_reports_listfilter with delayed registration./wp-admin/admin.php?page=wc-admin&path=%2Fanalytics%2Ftest-report-delayed-3sSorry, you are not allowed to access this page.message is displayedTest Report (3s Delay)is displayedChangelog entry
Changelog Entry Details
Significance
Type
Message
Fix race condition for woocommerce_admin_reports_list filter, preventing dynamically registered reports from being recognized
Changelog Entry Comment
Comment