[MWPW-190483] Fixing error observed in analytics + console errors on clicking cancel#737
[MWPW-190483] Fixing error observed in analytics + console errors on clicking cancel#737arugupta1992 merged 5 commits intostagefrom
Conversation
| } | ||
| return await this.afterFetchFromService(url, response); | ||
| } catch (error) { | ||
| if (error) error.message += `, Max retry delay exceeded for URL: ${url}`; |
There was a problem hiding this comment.
Reason : The catch block unconditionally appends "Max retry delay exceeded" to the error message regardless of whether the polling ever started. If the request failed on the first attempt with no retries then the message "Max retry delay exceeded" is wrong. This makes the analytics log misleading. The fix distinguish between a polling timeout and a first-attempt fetch failure:
| } | ||
|
|
||
| updateProgressBar(layer, percentage) { | ||
| if (!layer) return; |
There was a problem hiding this comment.
This is to guard for the race scenarios where the upload process begins before splashfragment loads.
Trade-off:
Now if the layer (i.e. splashscreen) is null, the workflow will progress to completion but user sees the page normally until the redirect fires as splashscreen was never loaded. This should be acceptable behaviour, since current behaviour blocks the user flow and throws error in this scenario.
There was a problem hiding this comment.
This is really interesting. I would agree that instead of a blocking flow, would be better to still let user be redirected to product.
How common of a case has this been?
There was a problem hiding this comment.
This is third top error for most of the verbs. First being 401 which is user error so we can call this out the second most impacting error.
| const { failedChunks, attemptMap } = result; | ||
| const totalChunks = Math.ceil(file.size / blockSize); | ||
| if (failedChunks.size > 0) { | ||
| if (failedChunks.size > 0 && !signal?.aborted) { |
There was a problem hiding this comment.
This is to prevent sending this analytics if workflow was cancelled by user.
| try { | ||
| response = await fetch(storageUrl, uploadOptions); | ||
| } catch (e) { | ||
| if (e instanceof TypeError) { |
There was a problem hiding this comment.
This is to account for correct error message if it is a network failure.
| } | ||
| const { default: TransitionScreen } = await import(`${getUnityLibs()}/scripts/transition-screen.js`); | ||
| this.transitionScreen = new TransitionScreen(this.transitionScreen.splashScreenEl, this.initActionListeners, this.LOADER_LIMIT, this.workflowCfg, this.desktop); | ||
| if (!this.transitionScreen) { |
There was a problem hiding this comment.
Changed all TransitionScreen instantiation sites to be lazy — only creates a new TransitionScreen instance if one doesn't already exist (if (!this.transitionScreen)). Also fixes a bug where this.transitionScreen.splashScreenEl was used (referencing the old object) instead of this.splashScreenEl, and updates LOADER_LIMIT on the existing instance rather than re-constructing.
arugupta1992
requested review from
Ruchika4,
sanjayms01 and
vipu0303
and removed request for
sanjayms01
| if (!this.transitionScreen) { | ||
| const { default: TransitionScreen } = await import(`${getUnityLibs()}/scripts/transition-screen.js`); | ||
| this.transitionScreen = new TransitionScreen(this.splashScreenEl, this.initActionListeners, this.LOADER_LIMIT, this.workflowCfg, this.desktop); | ||
| } |
There was a problem hiding this comment.
Thoughts on breaking this out to a separate function to remove the redundant lines of code?
There was a problem hiding this comment.
Yeah, good point. We already have loadTransitionScreen function which does the same. I am not sure why that wasn't being used throughout. I can take this up in my next PR.
|
Looks fine in branch URL. Test results are in the ticket :MWPW-190483 |
5 participants
Resolves: MWPW-190483
Test URLs: