PCH Related Posts: Prevent Related Posts indefinite loading #2666
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant usePostData
participant DataStore
User->>Component: Request post data
Component->>usePostData: Call usePostData(timeoutDurationMs)
usePostData->>DataStore: Fetch post data (authors, categories, tags)
alt Data fetched before timeout
DataStore-->>usePostData: Return post data
usePostData-->>Component: Provide post data
else Timeout reached
Note right of usePostData: Set isTimeout to true
usePostData-->>Component: Provide partial post data
end
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![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, codebase verification and nitpick comments (5)
src/content-helper/editor-sidebar/related-posts/hooks.ts (5)
7-7: Ensure compliance with WordPress coding standards.The import statement for
useEffectanduseStateshould be alphabetically ordered with other imports for better readability.- import { useEffect, useState } from '@wordpress/element'; + import { useEffect, useState } from '@wordpress/element';
40-40: Add a period at the end of the comment.The comment should end with a period to adhere to documentation standards.
- * @since 3.16.2 Added a timeout to ensure the hook returns after X seconds. + * @since 3.16.2. Added a timeout to ensure the hook returns after X seconds.
44-46: Clarify the parameter description.The description for
timeoutDurationMscould be more concise and clear.- * @param {number} timeoutDurationMs The duration in milliseconds before the hook times out and - * returns the data as is. + * @param {number} timeoutDurationMs The duration in milliseconds before the hook times out and returns the data as is.
49-49: Ensure compatibility with WordPress coding standards.The function signature should include a space before the default parameter value for better readability.
- export function usePostData( timeoutDurationMs: number = 200 ): PostData { + export function usePostData( timeoutDurationMs: number = 200 ): PostData {
58-63: Ensure the JSDoc comment is accurate and complete.The JSDoc comment for the
postAttributesfunction is well-written but ensure it includes a period at the end.- * @since 3.14.4 + * @since 3.14.4.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
build/content-helper/editor-sidebar.asset.phpis excluded by!build/**build/content-helper/editor-sidebar.jsis excluded by!build/**
Files selected for processing (1)
- src/content-helper/editor-sidebar/related-posts/hooks.ts (3 hunks)
Additional context used
Path-based instructions (1)
src/content-helper/editor-sidebar/related-posts/hooks.ts (1)
Pattern
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
Additional comments not posted (5)
src/content-helper/editor-sidebar/related-posts/hooks.ts (5)
50-56: Initialize state variables correctly.The state variables
isTimeoutandpostDataare initialized correctly. Ensure that these states are managed properly throughout the function.
Line range hint
64-104:
Verify the correctness of theuseSelecthook logic.The logic within the
useSelecthook appears to be correct. Ensure that thegetEntityRecordsfunction is used correctly and that the data is fetched as expected.
106-134: Ensure theuseEffecthook handles state updates correctly.The
useEffecthook updates thepostDatastate when all data is ready or when the timeout is reached. Ensure that the dependencies are correctly specified.
136-147: Ensure the timeout logic is correctly implemented.The second
useEffecthook sets a timeout to update theisTimeoutstate. Ensure that the timeout is cleared correctly to avoid memory leaks.
149-149: Return the post data correctly.The function returns the
postDatastate correctly. Ensure that the returned data is used appropriately in the consuming components.
There was a problem hiding this comment.
Interesting way to tackle this, I like it 🙂
I haven't tested, but if we're hitting WP endpoints by issuing HTTP requests, my gut feeling tells me that 200ms might be too low (e.g. high latency or heavy sites), and that we should raise it if it could be disruptive to user experience.
|
@acicovic I agree with you, I am a bit torn here, since this timeout will prevent the filter type UI from rendering for that amount of time, if any of the HTTP requests take to long or fail. Unfortunately, since we're doing the HTTP requests using Gutenberg's Although, and I tested this by throttling the connection in Chrome, if the endpoint is taking too long to respond, when it finally returns a valid response, the component will re-render and work as expected. But I think I agree with you that 200ms might be on the short side. For how long do you suggest waiting? 500ms? |
|
If I understand correctly, this timeout will only slow down users when there's a problem (e.g blocked user endpoint). This system will not affect websites that work as expected. So, not all users will be forced to wait for these 200ms. If the above is correct, in my opinion even waiting 1 second should be fine, since there would be an infinite freeze anyway if this fix wasn't in place. What I'm afraid of is introducing re-renders in configurations that work fine currently, and then having issues regarding that. If the user is using the tool while the re-rendering takes place, it could lead to a weird UX experience. |
|
I might have found a way to tackle this using |
|
@acicovic I changed the logic for the usePostData hook, and now instead of relying on the timeout, we're using This has fixed the issue of having the UI waiting for the timeout before rendering the filter type Group Toggle, when one of the resolution fails. So it feels nearly instantaneous now :) |
acicovic
changed the title
vaurdan
changed the title
Description
This PR enhances the
usePostDatahook in the Related Posts feature by utilizinghasFinishedResolutionandisResolvingto ensure it returns data only when it's fully resolved. This update is particularly useful in scenarios where theusersREST API endpoint is disabled, preventing the PCH from retrieving a list of users.Testing indicates that the new approach ensures the hook accurately determines when the data is ready to be rendered, preventing the component from remaining indefinitely in a Loading state. The component will now attempt to render with the available data as soon as all fetching operations are resolved.
Motivation and context
Improve the reliability of the Related Posts feature, ensuring it functions correctly even in edge cases or when unexpected API data is returned. By checking if the data has been resolved, the feature can gracefully handle scenarios where certain data endpoints, such as the
usersREST API, are disabled or slow to respond. This prevents the application from hanging indefinitely and enhances the user experience by rendering available data promptly.How has this been tested?
Tested locally.
Summary by CodeRabbit