Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(APW): Refresh content review - pull data on every 10s #3506

Merged
merged 14 commits into from
Oct 23, 2023

Conversation

mihajlovco
Copy link
Contributor

@mihajlovco mihajlovco commented Aug 30, 2023

With this PR we introducing a near-realtime experience for APW UI. Content will be refreshing every 10s.
What kind of updates user can see on the UI:

  • Content review list page: Refresh of the list of content reviews
  • Content review details page: Refresh the comments, change requests, and content review state (stapes and signoff).

Changes

Comments list

  • We send requests when the user clicks on the change request list item.
  • The list will be updated every 10 seconds when the user stays on the same change request list item.
  • An API request will not be sent if the user clicks on the workflow step and the comment section is not visible.

Test videos: Use case 1 and Use case 2

Change request list

  • The change request list will be updated every 10 seconds.
  • Test videos: Use case 3 and Use case 4

Content review list and details

  • The Content review will be updated every 10 seconds.
  • Test video: Use case 5

Technical changes

  • New useFetchInterval hook. This hook uses a recursive function that will not execute until the API call is finished + the delay.
    - Fetch interval test screenshot: Use case 6

Exposed refetch method from useQuery that allows the user to fetch the content in the following hooks:

  • useCommentsList hook
  • useChangeRequestsList hook
  • useContentReview
  • useContentReviewList

The useCurrentStep hook is implementing the useFetchInterval hook because it imports the useContentReviewhook that already exposes the refetch method.

Components where a 10s refresh interval is implemented.

  • Comments component - updates comments
  • CenterPanel component - updates the change requests and content review step states
  • ContentReviewDataList component - updates the list of content reviews

How Has This Been Tested?

Manually. Check the videos in the testing section.

Testing

Use case 1: Users can see the updates of the new messages

  • Refresh the comments section when the user clicks on another change request item in the list
    The user waiting for the new comment to appear on the same change request
apw-messeges-between-users.mov

Use case 2: Comments time interval is destroyed when the comment section is no longer visible.

apw-destroed-time-interval-instance.mov

Use case 3: Change request list updates every 10s.

apw-requests-sync.mov

Use case 4: Change request status update.

apw-request-change-status-update.mov

Use case 5: Content review list and single content review updates.

apw-content-review-list-and-details.mov

Use case 6: Fetch interval tests.
fetch-interval-tests

@mihajlovco mihajlovco added this to the 5.35.8 milestone Aug 30, 2023
@mihajlovco mihajlovco self-assigned this Aug 30, 2023
@brunozoric
Copy link
Contributor

brunozoric commented Aug 30, 2023

@mihajlovco
As I mentioned on a call we had, I really dislike using the setInterval to call something periodically. Especially if interval is small. Why?
Let's say you have a data refresh running every 5 seconds:

  • tick 1 takes 1 second
  • next tick will happen 4 seconds after previous call
  • tick 2 takes 4 seconds
  • next tick will happen immediately after previous call
  • tick 3 takes 6 seconds
  • next tick will happen even before the previous call was done

Yes, I know this is an edge case, but its quite possible (internet gets slow, system is overwhelmed with requests, etc...).

I would recommend using setTimeout and when callback is done, start a new timeout.

@Pavel910
Copy link
Collaborator

You can either look for an npm package which does this kind of stuff (I don't know of any, so can't recommend).
But a very simple solution which solves all the problems @brunozoric outlined, is this:

while(true) {
    // Issue api call...
    
    // Wait for 5 seconds before doing another loop.
    await new Promise(resolve => setTimeout(resolve, 5000));
}

Make sure you test this, and make it "abortable", so the process stops if you unmount a component (if such a scenario is needed).

@mihajlovco
Copy link
Contributor Author

mihajlovco commented Aug 30, 2023

@mihajlovco As I mentioned on a call we had, I really dislike using the setInterval to call something periodically. Especially if interval is small. Why? Let's say you have a data refresh running every 5 seconds:

  • tick 1 takes 1 second
  • next tick will happen 4 seconds after previous call
  • tick 2 takes 4 seconds
  • next tick will happen immediately after previous call
  • tick 3 takes 6 seconds
  • next tick will happen even before the previous call was done

Yes, I know this is an edge case, but its quite possible (internet gets slow, system is overwhelmed with requests, etc...).

I would recommend using setTimeout and when the callback is done, start a new timeout.

Thanks for the fast feedback, I will change that as you suggested.
I was thinking of not using only the Appolo client pollInterval, and use the js native setInterval. Thanks for the reminder.

@mihajlovco mihajlovco changed the title feat(APW): refresh reviews on every 10s feat(APW): Refresh content reviews comments and change requests on every 10s Aug 31, 2023
@mihajlovco mihajlovco changed the title feat(APW): Refresh content reviews comments and change requests on every 10s feat(APW): Refresh content review - comments and change requests on every 10s Aug 31, 2023
@brunozoric
Copy link
Contributor

How will this code:
https://github.com/webiny/webiny-js/pull/3506/files#diff-bcd435433b2e02d7ff474d3022f6e36f1fae3df4f58eb566856233df26ae8034R85

prevent the issue I wrote about in my first comment?

@mihajlovco mihajlovco modified the milestones: 5.35.8, 5.38.0 Sep 2, 2023
@mihajlovco mihajlovco marked this pull request as ready for review September 13, 2023 05:23
@mihajlovco mihajlovco changed the title feat(APW): Refresh content review - comments and change requests on every 10s feat(APW): Refresh content review - pull data on every 10s Sep 13, 2023
@Pavel910 Pavel910 merged commit 742aff2 into next Oct 23, 2023
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants