From 561a424393ab7bb7aa0b617e6f86f845a562d7df Mon Sep 17 00:00:00 2001 From: David Hahn Date: Mon, 5 Oct 2026 14:54:44 -0700 Subject: [PATCH] MWPW-206368 - Fix cancel button stuck screen, stray redirects and extra assets Fix browser test isolation and add transition-screen error coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflow-acrobat/action-binder.test.js | 136 +++++++++++++++++- .../workflow-acrobat/upload-handler.test.js | 89 ++++++++++++ .../scripts/transition-screen.test.js | 41 +++--- .../workflow-acrobat/action-binder.js | 13 +- .../workflow-acrobat/upload-handler.js | 5 + 5 files changed, 259 insertions(+), 25 deletions(-) diff --git a/test/core/workflow/workflow-acrobat/action-binder.test.js b/test/core/workflow/workflow-acrobat/action-binder.test.js index 20b68ab56..4439638e9 100644 --- a/test/core/workflow/workflow-acrobat/action-binder.test.js +++ b/test/core/workflow/workflow-acrobat/action-binder.test.js @@ -1,6 +1,7 @@ import { expect } from '@esm-bundle/chai'; import sinon from 'sinon'; import ActionBinder from '../../../../unitylibs/core/workflow/workflow-acrobat/action-binder.js'; +import { getUnityLibs, setUnityLibs } from '../../../../unitylibs/scripts/utils.js'; const mockUpdateProgressBar = null; @@ -1557,11 +1558,7 @@ describe('ActionBinder', () => { }); describe('continueInApp', () => { - let locationSpy; - beforeEach(() => { - locationSpy = sinon.spy(); - actionBinder.redirectUrl = 'https://test.com?param=value'; actionBinder.operations = ['test-operation']; actionBinder.redirectWithoutUpload = false; @@ -1570,6 +1567,7 @@ describe('ActionBinder', () => { actionBinder.multiFileFailure = null; actionBinder.showTransitionScreen = sinon.stub().resolves(); actionBinder.transitionScreen = { + clearProgressBarHandler: sinon.stub(), updateProgressBar: sinon.stub(), showSplashScreen: sinon.stub().resolves(), }; @@ -1581,14 +1579,16 @@ describe('ActionBinder', () => { actionBinder.redirectUrl = ''; await actionBinder.continueInApp(); expect(actionBinder.showTransitionScreen.called).to.be.false; - expect(locationSpy.called).to.be.false; + expect(actionBinder.transitionScreen.updateProgressBar.called).to.be.false; + expect(actionBinder.delay.called).to.be.false; }); it('should not proceed if no operations and not redirectWithoutUpload', async () => { actionBinder.operations = []; await actionBinder.continueInApp(); expect(actionBinder.showTransitionScreen.called).to.be.false; - expect(locationSpy.called).to.be.false; + expect(actionBinder.transitionScreen.updateProgressBar.called).to.be.false; + expect(actionBinder.delay.called).to.be.false; }); it('should log to splunk and continue when direct upload verb progress bar update throws', async () => { @@ -1635,6 +1635,8 @@ describe('ActionBinder', () => { showSplashScreen: sinon.stub().resolves(), }; actionBinder.transitionScreen = existingTransitionScreen; + // Stop after the progress update; this test does not exercise navigation. + actionBinder.delay.callsFake(async () => { actionBinder.redirectUrl = ''; }); await actionBinder.continueInApp(); expect(actionBinder.transitionScreen).to.equal(existingTransitionScreen); expect(actionBinder.LOADER_LIMIT).to.equal(100); @@ -1642,6 +1644,104 @@ describe('ActionBinder', () => { expect(existingTransitionScreen.clearProgressBarHandler.calledOnce).to.be.true; expect(existingTransitionScreen.updateProgressBar.calledOnceWith(splashLayer, 100)).to.be.true; }); + + it('should restore the splash and report an error when the final redirect delay fails', async () => { + const error = new Error('redirect delay failed'); + actionBinder.delay.rejects(error); + await actionBinder.continueInApp(); + expect(actionBinder.transitionScreen.showSplashScreen.calledOnce).to.be.true; + expect(actionBinder.dispatchErrorToast.calledOnceWith( + 'error_generic', + 500, + 'Exception thrown when redirecting to product; redirect delay failed', + false, + undefined, + { code: 'upload_error_redirect_to_app', subCode: undefined, desc: error.message }, + )).to.be.true; + }); + + it('should not navigate when cancel clears the redirect URL during the final progress delay', async () => { + actionBinder.transitionScreen = { + splashScreenEl: document.createElement('div'), + clearProgressBarHandler: sinon.stub(), + updateProgressBar: sinon.stub(), + showSplashScreen: sinon.stub().resolves(), + }; + actionBinder.delay = sinon.stub().callsFake(async () => { actionBinder.redirectUrl = ''; }); + // multiFileFailure is only read when building the navigation URL. Throwing here makes a + // regression fail via the catch/toast path instead of actually navigating the test page. + Object.defineProperty(actionBinder, 'multiFileFailure', { + get() { throw new Error('navigation attempted after cancel'); }, + configurable: true, + }); + try { + await actionBinder.continueInApp(); + expect(actionBinder.dispatchErrorToast.called).to.be.false; + expect(actionBinder.transitionScreen.showSplashScreen.called).to.be.false; + } finally { + delete actionBinder.multiFileFailure; + } + }); + }); + + describe('showTransitionScreen', () => { + it('should clear the previous transition screen progress bar timer before loading a new one', async () => { + setUnityLibs('/unitylibs'); + const splashLayer = document.createElement('div'); + const existingTransitionScreen = { + splashScreenEl: splashLayer, + clearProgressBarHandler: sinon.stub(), + }; + actionBinder.transitionScreen = existingTransitionScreen; + const pending = actionBinder.showTransitionScreen(); + expect(existingTransitionScreen.clearProgressBarHandler.calledOnce).to.be.true; + await pending; + expect(actionBinder.transitionScreen).to.not.equal(existingTransitionScreen); + expect(actionBinder.transitionScreen.splashScreenEl).to.equal(splashLayer); + expect(actionBinder.transitionScreen.LOADER_LIMIT).to.equal(actionBinder.LOADER_LIMIT); + expect(actionBinder.transitionScreen.showSplashScreen).to.be.a('function'); + }); + }); + + describe('loadTransitionScreen', () => { + beforeEach(() => { + setUnityLibs('/unitylibs'); + actionBinder.loadTransitionScreen.restore(); + }); + + it('should load the transition screen only once and schedule its splash loader', async () => { + const { default: TransitionScreen } = await import(`${getUnityLibs()}/scripts/transition-screen.js`); + const loader = sinon.stub(TransitionScreen.prototype, 'delayedSplashLoader').resolves(); + await actionBinder.loadTransitionScreen(); + const { transitionScreen } = actionBinder; + await actionBinder.loadTransitionScreen(); + expect(transitionScreen).to.be.instanceOf(TransitionScreen); + expect(transitionScreen.workflowCfg).to.equal(mockWorkflowCfg); + expect(actionBinder.transitionScreen).to.equal(transitionScreen); + expect(loader.calledOnce).to.be.true; + }); + + it('should report and propagate a splash loader failure', async () => { + const { default: TransitionScreen } = await import(`${getUnityLibs()}/scripts/transition-screen.js`); + const error = new Error('splash loader failed'); + sinon.stub(TransitionScreen.prototype, 'delayedSplashLoader').rejects(error); + const toast = sinon.stub(actionBinder, 'dispatchErrorToast').resolves(); + let failure; + try { + await actionBinder.loadTransitionScreen(); + } catch (caught) { + failure = caught; + } + expect(failure).to.equal(error); + expect(toast.calledOnceWith( + 'pre_upload_error_transition_screen', + null, + `Error loading transition screen, Error: ${error}`, + false, + true, + { code: 'pre_upload_error_transition_screen' }, + )).to.be.true; + }); }); describe('cancelAcrobatOperation', () => { @@ -1703,6 +1803,12 @@ describe('ActionBinder', () => { await actionBinder.cancelAcrobatOperation(); expect(actionBinder.filesData.workflowStep).to.equal('preuploading'); }); + + it('should clear recorded operations so the next upload cannot redirect with stale state', async () => { + actionBinder.operations = ['asset-cancelled']; + await actionBinder.cancelAcrobatOperation(); + expect(actionBinder.operations).to.deep.equal([]); + }); }); describe('initActionListeners', () => { @@ -2038,6 +2144,24 @@ describe('ActionBinder', () => { spy.restore(); }); + it('should register the RedirectReady listener only once across repeated actions', async () => { + actionBinder.transitionScreen = { test: 'existing' }; + actionBinder.handlePreloads = sinon.stub().resolves(); + const cancelStub = sinon.stub(actionBinder, 'cancelAcrobatOperation').resolves(); + const addSpy = sinon.spy(window, 'addEventListener'); + delete actionBinder.redirectReadyBound; + try { + await actionBinder.acrobatActionMaps('interrupt'); + await actionBinder.acrobatActionMaps('interrupt'); + await actionBinder.acrobatActionMaps('interrupt'); + const registrations = addSpy.getCalls().filter((c) => c.args[0] === 'DCUnity:RedirectReady'); + expect(registrations).to.have.lengthOf(1); + } finally { + addSpy.restore(); + cancelStub.restore(); + } + }); + describe('enabledFeatures validation', () => { it('should dispatch error when enabledFeatures is null', async () => { // Mock transition screen to avoid early return diff --git a/test/core/workflow/workflow-acrobat/upload-handler.test.js b/test/core/workflow/workflow-acrobat/upload-handler.test.js index cbb2548dd..fdaa401d3 100644 --- a/test/core/workflow/workflow-acrobat/upload-handler.test.js +++ b/test/core/workflow/workflow-acrobat/upload-handler.test.js @@ -591,6 +591,95 @@ describe('UploadHandler', () => { }); }); + describe('directUploadSingleFile', () => { + let file; + let fileData; + + beforeEach(() => { + file = new File(['test content'], 'test.pdf', { type: 'application/pdf' }); + fileData = { type: 'application/pdf', size: 1000, count: 1, uploadType: 'sfu' }; + }); + + it('should show the error splash screen and return false for a genuine upload failure', async () => { + uploadHandler.directUploadAsset = sinon.stub().rejects(new Error('network fail')); + + const result = await uploadHandler.directUploadSingleFile(file, fileData); + + expect(result).to.be.false; + expect(uploadHandler.initSplashScreen.called).to.be.true; + expect(mockTransitionScreen.showSplashScreen.called).to.be.true; + expect(mockActionBinder.dispatchErrorToast.called).to.be.true; + }); + + it('should redirect and dispatch the uploaded event when the upload completes normally', async () => { + mockActionBinder.isUploading = true; + uploadHandler.directUploadAsset = sinon.stub().resolves({ id: 'asset-999' }); + + const result = await uploadHandler.directUploadSingleFile(file, fileData); + + expect(result).to.be.true; + expect(mockActionBinder.handleRedirect.calledOnce).to.be.true; + expect(mockActionBinder.operations).to.include('asset-999'); + expect(mockActionBinder.dispatchAnalyticsEvent.calledWith('uploaded')).to.be.true; + }); + + it('should skip the redirect when cancel wins the race against a completing upload', async () => { + // Upload can still resolve after Cancel already ran. + mockActionBinder.isUploading = false; + uploadHandler.directUploadAsset = sinon.stub().resolves({ id: 'asset-999' }); + + const result = await uploadHandler.directUploadSingleFile(file, fileData); + + expect(result).to.be.true; + expect(mockActionBinder.handleRedirect.called).to.be.false; + expect(mockActionBinder.operations).to.not.include('asset-999'); + expect(mockActionBinder.dispatchAnalyticsEvent.calledWith('uploaded')).to.be.false; + }); + + it('should stay silent and skip the chunked fallback when cancel aborts the upload', async () => { + const abortError = new Error('Request aborted by user.'); + abortError.name = 'AbortError'; + uploadHandler.directUploadAsset = sinon.stub().rejects(abortError); + + const result = await uploadHandler.directUploadSingleFile(file, fileData); + + expect(result).to.be.true; + expect(uploadHandler.initSplashScreen.called).to.be.false; + expect(mockActionBinder.dispatchErrorToast.called).to.be.false; + }); + + it('should still show the error splash screen for a request timeout (not a user cancel)', async () => { + const timeoutError = new Error('Request timed out after 60000ms'); + timeoutError.name = 'TimeoutError'; + uploadHandler.directUploadAsset = sinon.stub().rejects(timeoutError); + + const result = await uploadHandler.directUploadSingleFile(file, fileData); + + expect(result).to.be.false; + expect(uploadHandler.initSplashScreen.called).to.be.true; + expect(mockActionBinder.dispatchErrorToast.called).to.be.true; + }); + + it('should not record a cancelled upload whose redirect URL returns after a new upload started', async () => { + const signal = { aborted: false }; + mockActionBinder.getAbortSignal = sinon.stub().returns(signal); + mockActionBinder.isUploading = true; + uploadHandler.directUploadAsset = sinon.stub().resolves({ id: 'asset-cancelled' }); + mockActionBinder.handleRedirect = sinon.stub().callsFake(async () => { + // Cancel aborts this upload, then the next upload sets isUploading back to true. + signal.aborted = true; + mockActionBinder.isUploading = true; + return true; + }); + + const result = await uploadHandler.directUploadSingleFile(file, fileData); + + expect(result).to.be.true; + expect(mockActionBinder.operations).to.not.include('asset-cancelled'); + expect(mockActionBinder.dispatchAnalyticsEvent.calledWith('uploaded')).to.be.false; + }); + }); + describe('uploadMultiFile', () => { let files; let filesData; diff --git a/test/unitylibs/scripts/transition-screen.test.js b/test/unitylibs/scripts/transition-screen.test.js index 3c47bbc74..423cedee3 100644 --- a/test/unitylibs/scripts/transition-screen.test.js +++ b/test/unitylibs/scripts/transition-screen.test.js @@ -8,12 +8,19 @@ describe('TransitionScreen', () => { let workflowCfg; beforeEach(() => { + document.body.innerHTML = '
'; splashScreenEl = document.createElement('div'); workflowCfg = { targetCfg: { showSplashScreen: true }, productName: 'acrobat' }; screen = new TransitionScreen(splashScreenEl, sinon.stub(), 95, workflowCfg); screen.splashScreenEl = splashScreenEl; }); + afterEach(() => { + screen.clearProgressBarHandler(); + sinon.restore(); + document.body.innerHTML = ''; + }); + describe('updateProgressBar', () => { it('should update progress bar attributes and text', () => { splashScreenEl.innerHTML = ` @@ -84,33 +91,36 @@ describe('TransitionScreen', () => { const spy = sinon.spy(screen, 'updateProgressBar'); const clock = sinon.useFakeTimers(); screen.progressBarHandler(splashScreenEl, 10, 10, true); - clock.tick(20); - expect(spy.called).to.be.true; + expect(spy.calledOnceWith(splashScreenEl, 0)).to.be.true; + clock.tick(110); + expect(spy.calledTwice).to.be.true; + expect(spy.calledWith(splashScreenEl, 5)).to.be.true; + clock.tick(210); + expect(spy.calledThrice).to.be.true; + expect(spy.calledWith(splashScreenEl, 10)).to.be.true; spy.restore(); - clock.restore(); }); it('should ignore stale callbacks after clearProgressBarHandler', () => { const clock = sinon.useFakeTimers(); screen.progressBarHandler(splashScreenEl, 10, 10, true); - clock.tick(15); + clock.tick(110); screen.clearProgressBarHandler(); const valueBeforeStaleTick = splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value'); - clock.tick(100); + expect(clock.countTimers()).to.equal(0); + clock.tick(210); expect(splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value')).to.equal(valueBeforeStaleTick); - clock.restore(); }); it('should restart cleanly when initialized after cancel', () => { const clock = sinon.useFakeTimers(); screen.progressBarHandler(splashScreenEl, 10, 10, true); - clock.tick(15); + clock.tick(110); screen.clearProgressBarHandler(); screen.progressBarHandler(splashScreenEl, 10, 10, true); expect(splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value')).to.equal('0'); - clock.tick(15); + clock.tick(110); expect(parseInt(splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value'), 10)).to.be.greaterThan(0); - clock.restore(); }); }); @@ -124,12 +134,12 @@ describe('TransitionScreen', () => { `; const clock = sinon.useFakeTimers(); screen.progressBarHandler(splashScreenEl, 10, 10, true); - clock.tick(15); + clock.tick(110); screen.updateProgressBar(splashScreenEl, 100); expect(splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value')).to.equal('100'); - clock.tick(100); + expect(clock.countTimers()).to.equal(0); + clock.tick(210); expect(splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value')).to.equal('100'); - clock.restore(); }); }); @@ -164,7 +174,6 @@ describe('TransitionScreen', () => { beforeEach(() => { const parent = document.createElement('div'); parent.appendChild(splashScreenEl); - document.body.innerHTML = '
'; }); it('should hide splash and reset LOADER_LIMIT when displayOn is false', () => { const clock = sinon.useFakeTimers(); @@ -175,15 +184,15 @@ describe('TransitionScreen', () => {
`; screen.progressBarHandler(splashScreenEl, 10, 10, true); - clock.tick(15); + clock.tick(110); screen.splashVisibilityController(false); const valueAtCancel = splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value'); - clock.tick(100); + expect(clock.countTimers()).to.equal(0); + clock.tick(210); expect(screen.LOADER_LIMIT).to.equal(95); expect(splashScreenEl.classList.contains('show')).to.be.false; expect(splashScreenEl.parentElement.classList.contains('hide-splash-overflow')).to.be.false; expect(splashScreenEl.querySelector('.spectrum-ProgressBar').getAttribute('value')).to.equal(valueAtCancel); - clock.restore(); }); it('should show splash and set aria-hidden when displayOn is true', () => { const stub = sinon.stub(screen, 'progressBarHandler'); diff --git a/unitylibs/core/workflow/workflow-acrobat/action-binder.js b/unitylibs/core/workflow/workflow-acrobat/action-binder.js index e1375c2fb..e787ddd7d 100644 --- a/unitylibs/core/workflow/workflow-acrobat/action-binder.js +++ b/unitylibs/core/workflow/workflow-acrobat/action-binder.js @@ -489,6 +489,7 @@ export default class ActionBinder { } async showTransitionScreen() { + this.transitionScreen?.clearProgressBarHandler(); const { default: TransitionScreen } = await import(`${getUnityLibs()}/scripts/transition-screen.js`); this.transitionScreen = new TransitionScreen(this.transitionScreen.splashScreenEl, this.initActionListeners, this.LOADER_LIMIT, this.workflowCfg); await this.transitionScreen.showSplashScreen(); @@ -808,6 +809,8 @@ export default class ActionBinder { else this.transitionScreen.updateProgressBar(splashLayer, 100); try { await this.delay(500); + // Cancel may clear redirectUrl during the final delay; navigating now would reload the page with "&undefined". + if (!this.redirectUrl) return; const [baseUrl, queryString] = this.redirectUrl.split('?'); if (getMatchedDomain(this.workflowCfg.targetCfg.domainMap) === 'acrobat') { document.cookie = `dc_fl=1;domain=.adobe.com;path=/;expires=${new Date(Date.now() + 30 * 1000).toUTCString()}`; @@ -828,6 +831,7 @@ export default class ActionBinder { async cancelAcrobatOperation() { await this.showTransitionScreen(); this.redirectUrl = ''; + this.operations = []; this.filesData = this.filesData || {}; this.filesData.workflowStep = this.isUploading ? 'uploading' : 'preuploading'; this.dispatchAnalyticsEvent('cancel', this.filesData); @@ -867,9 +871,12 @@ export default class ActionBinder { return; } } - window.addEventListener('DCUnity:RedirectReady', async () => { - await this.continueInApp(); - }); + if (!this.redirectReadyBound) { + this.redirectReadyBound = true; + window.addEventListener('DCUnity:RedirectReady', async () => { + await this.continueInApp(); + }); + } if (!this.workflowCfg.enabledFeatures?.length || !ActionBinder.LIMITS_MAP[this.workflowCfg.enabledFeatures[0]]) { await this.dispatchErrorToast('error_generic', 500, 'Invalid or missing verb configuration on Unity', false, true, { code: 'pre_upload_error_missing_verb_config' }); return; diff --git a/unitylibs/core/workflow/workflow-acrobat/upload-handler.js b/unitylibs/core/workflow/workflow-acrobat/upload-handler.js index 6a4109344..3cd777b87 100644 --- a/unitylibs/core/workflow/workflow-acrobat/upload-handler.js +++ b/unitylibs/core/workflow/workflow-acrobat/upload-handler.js @@ -57,11 +57,14 @@ export default class UploadHandler { try { assetData = await this.directUploadAsset(file, abortSignal); } catch (error) { + // A cancel-triggered abort is not a failure: skip the error splash and the chunked-upload fallback. + if (abortSignal.aborted || error?.name === 'AbortError') return true; this.initSplashScreen(); await this.transitionScreen.showSplashScreen(); this.handleUploadError(error, 'pre_upload_error_direct_upload'); return false; } + if (abortSignal.aborted || !this.actionBinder.isUploading) return true; fileData.assetId = assetData.id; this.actionBinder.setAssetId(assetData.id); const effectiveFileType = await this.getEffectiveFileType(file); @@ -77,6 +80,8 @@ export default class UploadHandler { }, }; const redirectSuccess = await this.actionBinder.handleRedirect(cOpts, fileData); + // Only the signal: a newer upload may have already set isUploading back to true. + if (abortSignal.aborted) return true; if (!redirectSuccess) return false; this.actionBinder.operations.push(assetData.id);