[Guided onboarding] Fix multiple API requests when fetching guide state#144160
Conversation
yuliacech
added
release_note:skip
…est fails and when the subscription unsubscribes
|
Pinging @elastic/platform-onboarding (Team:Journey/Onboarding) |
…not getting the new state value
…-ref HEAD~1..HEAD --fix'
alisonelizabeth
left a comment
alisonelizabeth
left a comment
There was a problem hiding this comment.
Thanks so much for taking this on! I found one issue with the "complete" state not updating. I left a suggestion that I think could resolve it.
| this.isGuideStateLoading = false; | ||
| // There should only be 1 active guide | ||
| const hasState = response.state.length === 1; | ||
| if (hasState) { |
There was a problem hiding this comment.
I think this will break the "complete" state; we need to update I think this will be irrelevant if you follow my suggestion below.this.onboardingGuideState$ once the user completes a guide so we know to hide the setup guide button.
| }) | ||
| ) | ||
| : of(state) | ||
| !state && !this.isGuideStateLoading ? this.createGetStateObservable() : of(state) |
There was a problem hiding this comment.
Related to my comment above. I think we need a way to differentiate between state that is undefined because it hasn't been fetched yet vs. state that is undefined bc the active guide has been completed.
Maybe we should rethink this line in updateGuideState and maintain the state:
// If the guide has been deactivated, we return undefined
this.onboardingGuideState$.next(newState.isActive ? newState : undefined);
Change to:
this.onboardingGuideState$.next(newState);
Then, I think we'd need to update guide_panel.tx when checking for the config:
const guideConfig = guideState?.isActive && getGuideConfig(guideState?.guideId);
| await pageObjects.infraHome.ensureTourStepIsClosed('guidedSetupStep'); | ||
| }); | ||
| // only run these tests on Cloud | ||
| if (isCloud) { |
There was a problem hiding this comment.
Thanks for fixing this!
|
Thanks a lot for your review, @alisonelizabeth! Great catch with the completed state not updating, I used your suggestions and added a couple of tests. I might update the guide state observable even further when I'll work on the button logic. We probably do need an indicator of an overall guided onboarding state. |
💚 Build Succeeded
Metrics [docs]Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
alisonelizabeth
left a comment
alisonelizabeth
left a comment
There was a problem hiding this comment.
Latest LGTM. Great work!
|
Apologies, I needed to revert this as it hit a type error when merged with main: 02a2a6a |
|
Thanks for the heads up @tylersmalley! Yes, there was another PR that got merged shortly before this one and caused a types error. I'll fix this and open a new PR. |
…tching guide state (elastic#144160)"" This reverts commit a4445f9.
## Summary Removes the feature flag for the guided onboarding tour in security To be merged after #144160. Make sure we run the Cypress tests after #144160 is merged before merging this Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Yulia Čech <6585477+yuliacech@users.noreply.github.com>
7 participants
Summary
This PR fixes a bug that guided onboarding is spamming API requests when loading guide state (for example to check if a step is active). The issue was caused by the code not checking if a request was already in flight before sending the next one. The assumption was that one request would complete before any other subscriptions would be made to the observable. However, when a component is being re-rendered several times in fast succession, there were several subscriptions and the requests were not able to complete.
I introduced a check for the request in flight and a handle to abort the request if the subscription is unsubscribed.