ref(issues): Extract modal into hook, allow local dismissal #89741
Conversation
github-actions
bot
added
the
Scope: Frontend
| ), | ||
| { | ||
| modalCss: IssueDetailsTourModalCss, | ||
| onClose: handleEndTour, |
There was a problem hiding this comment.
Do we need to add if (reason) here so that this only occurs when the modal is dismissed by the user? I think that this will get called from props.closeModal() otherwise which I don't think we want (since that happens if they click to start the tour)
There was a problem hiding this comment.
Good call, that makes sense 👍
| isCompleted: isTourCompleted, | ||
| } = useIssueDetailsTour(); | ||
|
|
||
| const [localTourState, setLocalTourState] = useLocalStorageState( |
There was a problem hiding this comment.
If localstorage is actually necessary it seems like we would want to use it as a backup everywhere we currently use /assistant/ - do you think that's the case?
There was a problem hiding this comment.
I think maybe, though there are a bunch of other places we use the assistant that would need addressing so it's probably out of scope for now. If we have time we can look into the cause for this user's bad assistant state but I think it's a little early for a broad change (though it's worth noting)
If reports keep coming in we should look into it further, this page is just extremely high traffic so I'm being cautious.
There was a problem hiding this comment.
Also something we should note for building a TourStore to resolve the concurrent tour issues (related to #89733)!
For whatever reason, it's possible that users do not create assistant records, which will cause annoying behaviour like repeatedly popping the tour modal. I'm guessing this has something to do with an ad blocker, or `/assistant/` being flagged on their network or something oddly specific. Either way, to make it a bit more resilient, we can add a back up check with local storage. This should prevent any further issues of repeated modals since only super users can ever reset this local storage value in-app. The component was also getting pretty confusing, so pulled it all into its own hook.
For whatever reason, it's possible that users do not create assistant records, which will cause annoying behaviour like repeatedly popping the tour modal. I'm guessing this has something to do with an ad blocker, or
/assistant/being flagged on their network or something oddly specific.Either way, to make it a bit more resilient, we can add a back up check with local storage. This should prevent any further issues of repeated modals since only super users can ever reset this local storage value in-app.
The component was also getting pretty confusing, so pulled it all into its own hook.