Closed vaurdan closed 1 month ago
[!IMPORTANT]
Review skipped
Auto reviews are disabled on base/target branches other than the default branch.
Base branches to auto review (4)
* develop * add/.* * fix/.* * update/.*Please check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
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?
@coderabbitai review
@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 getEntityRecords
, we don't have control/access to the requests, and it's not possible to check if they have either returned an error code, or if they are still running.
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 hasFinishedResolution
and isResolving
, from the core-data
store. I will see if I can implement a better solution, and perhaps ditch the need for a timeout :)
@acicovic I changed the logic for the usePostData hook, and now instead of relying on the timeout, we're using hasFinishedResolution
and isResolving
to check if the data we need is actually ready to be used.
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 :)
Description
This PR enhances the
usePostData
hook in the Related Posts feature by utilizinghasFinishedResolution
andisResolving
to ensure it returns data only when it's fully resolved. This update is particularly useful in scenarios where theusers
REST 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
users
REST 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