diff --git a/src/elements/content-preview/ContentPreview.js b/src/elements/content-preview/ContentPreview.js index 8eeb9b6eed..c1132fc855 100644 --- a/src/elements/content-preview/ContentPreview.js +++ b/src/elements/content-preview/ContentPreview.js @@ -110,6 +110,7 @@ type Props = { isLarge: boolean, isVeryLarge?: boolean, language: string, + loadingIndicatorDelayMs?: number, logoUrl?: string, measureRef: Function, messages?: StringMap, @@ -148,6 +149,7 @@ type State = { error?: ErrorType, file?: BoxItem, isLoading: boolean, + isDeferringLoading?: boolean, isReloadNotificationVisible: boolean, isThumbnailSidebarOpen: boolean, prevFileIdProp?: string, // the previous value of the "fileId" prop. Needed to implement getDerivedStateFromProps @@ -238,6 +240,7 @@ class ContentPreview extends React.PureComponent { canPrint: false, error: undefined, isLoading: true, + isDeferringLoading: false, isReloadNotificationVisible: false, isThumbnailSidebarOpen: false, }; @@ -255,6 +258,7 @@ class ContentPreview extends React.PureComponent { enableThumbnailsSidebar: false, hasHeader: false, language: DEFAULT_LOCALE, + loadingIndicatorDelayMs: 0, onAnnotator: noop, onAnnotatorEvent: noop, onContentInsightsEventReport: noop, @@ -281,6 +285,10 @@ class ContentPreview extends React.PureComponent { */ fetchFileStartTime: ?number; + loadingIndicatorShownThisSession: boolean; + + loadingIndicatorDelayTimeoutId: TimeoutID | void; + /** * [constructor] * @@ -301,6 +309,7 @@ class ContentPreview extends React.PureComponent { } = props; this.id = uniqueid('bcpr_'); + this.loadingIndicatorShownThisSession = false; this.api = new API({ apiHost, cache, @@ -313,8 +322,11 @@ class ContentPreview extends React.PureComponent { token, version: CLIENT_VERSION, }); + const delayMs = this.getLoadingIndicatorDelayMs(); + // When delay is used, start in defer state so the spinner is not shown until after delayMs this.state = { ...this.initialState, + ...(delayMs > 0 ? { isLoading: false, isDeferringLoading: true } : {}), currentFileId: fileId, // eslint-disable-next-line react/no-unused-state prevFileIdProp: fileId, @@ -331,15 +343,21 @@ class ContentPreview extends React.PureComponent { * @return {void} */ componentWillUnmount(): void { + this.clearLoadingIndicatorDelayTimeout(); // Don't destroy the cache while unmounting this.api.destroy(false); this.destroyPreview(); } /** - * Cleans up the preview instance + * Cleans up the preview instance. + * When shouldReset is true (e.g. file changed), clears the loading-indicator delay timeout. + * When false (e.g. loading preview for same file), leaves the timeout so one defer per session. */ destroyPreview(shouldReset: boolean = true) { + if (shouldReset) { + this.clearLoadingIndicatorDelayTimeout(); + } const { onPreviewDestroy } = this.props; if (this.preview) { this.preview.destroy(); @@ -350,6 +368,27 @@ class ContentPreview extends React.PureComponent { } } + clearLoadingIndicatorDelayTimeout(): void { + if (this.loadingIndicatorDelayTimeoutId) { + clearTimeout(this.loadingIndicatorDelayTimeoutId); + this.loadingIndicatorDelayTimeoutId = undefined; + } + } + + getLoadingIndicatorDelayMs(): number { + return Math.max(0, Number(this.props.loadingIndicatorDelayMs) || 0); + } + + /** + * Ends the current loading session: clear defer timer, reset session flag, hide loading state. + * Call when preview has loaded, errored, or file fetch failed. + */ + endLoadingSession(): void { + this.clearLoadingIndicatorDelayTimeout(); + this.loadingIndicatorShownThisSession = false; + this.setState({ isLoading: false, isDeferringLoading: false }); + } + /** * Destroys api instances with caches * @@ -362,6 +401,7 @@ class ContentPreview extends React.PureComponent { /** * Once the component mounts, load Preview assets and fetch file info. + * When loadingIndicatorDelayMs > 0, defer showing the loading spinner. * * @return {void} */ @@ -369,7 +409,22 @@ class ContentPreview extends React.PureComponent { this.loadStylesheet(); this.loadScript(); - this.fetchFile(this.state.currentFileId); + const { currentFileId } = this.state; + if (!currentFileId) { + this.focusPreview(); + return; + } + + const delayMs = this.getLoadingIndicatorDelayMs(); + if (delayMs > 0) { + this.loadingIndicatorDelayTimeoutId = setTimeout(() => { + this.loadingIndicatorDelayTimeoutId = undefined; + this.loadingIndicatorShownThisSession = true; + this.setState({ isLoading: true, isDeferringLoading: false }); + }, delayMs); + } + + this.fetchFile(currentFileId); this.focusPreview(); } @@ -402,14 +457,26 @@ class ContentPreview extends React.PureComponent { features?.advancedContentInsights, ); const haveExperiencesChanged = prevPreviewExperiences !== previewExperiences; + const delayMs = this.getLoadingIndicatorDelayMs(); if (hasFileIdChanged) { this.destroyPreview(); - this.setState({ isLoading: true, selectedVersion: undefined }); + this.loadingIndicatorShownThisSession = false; + if (!currentFileId) { + this.endLoadingSession(); + } else if (delayMs > 0) { + this.setState({ isLoading: false, isDeferringLoading: true, selectedVersion: undefined }); + this.loadingIndicatorDelayTimeoutId = setTimeout(() => { + this.loadingIndicatorDelayTimeoutId = undefined; + this.loadingIndicatorShownThisSession = true; + this.setState({ isLoading: true, isDeferringLoading: false }); + }, delayMs); + } else { + this.setState({ isLoading: true, selectedVersion: undefined }); + } this.fetchFile(currentFileId); } else if (this.shouldLoadPreview(prevState)) { this.destroyPreview(false); - this.setState({ isLoading: true }); this.loadPreview(); } else if (hasTokenChanged) { this.updatePreviewToken(); @@ -629,8 +696,8 @@ class ContentPreview extends React.PureComponent { const { code = ERROR_CODE_UNKNOWN } = error; const { onError } = this.props; - // In case of error, there should be no thumbnail sidebar to account for - this.setState({ isLoading: false, isThumbnailSidebarOpen: false }); + this.endLoadingSession(); + this.setState({ isThumbnailSidebarOpen: false }); onError( error, @@ -739,7 +806,7 @@ class ContentPreview extends React.PureComponent { onLoad(loadData); - this.setState({ isLoading: false }); + this.endLoadingSession(); this.focusPreview(); if (this.preview && filesToPrefetch.length) { @@ -854,6 +921,7 @@ class ContentPreview extends React.PureComponent { header: 'none', headerElement: `#${this.id} .bcpr-PreviewHeader`, experiences: previewExperiences, + loadingIndicatorDelayMs: this.getLoadingIndicatorDelayMs(), showAnnotations: this.canViewAnnotations(), showAnnotationsControls, showDownload: this.canDownload(), @@ -934,7 +1002,16 @@ class ContentPreview extends React.PureComponent { // If the file is watermarked or if its a new file, then update the state // In this case preview should reload without prompting the user if (isWatermarked || !isExistingFile) { - this.setState({ ...this.initialState, file }); + const delayMs = this.getLoadingIndicatorDelayMs(); + const useDeferredLoading = delayMs > 0; + // Set isLoading to false only when we never showed the spinner this session. + // If we already showed it, leave isLoading true so the spinner stays until preview loads or errors (no flicker). + const shouldHideLoading = useDeferredLoading && !this.loadingIndicatorShownThisSession; + this.setState({ + ...this.initialState, + file, + ...(shouldHideLoading ? { isLoading: false } : {}), + }); // $FlowFixMe file version and sha1 should exist at this point } else if (currentFile.file_version.sha1 !== file.file_version.sha1) { // If we are already prevewing the file that got updated then show the @@ -959,7 +1036,8 @@ class ContentPreview extends React.PureComponent { code: errorCode, message: fileError.message, }; - this.setState({ error, file: undefined, isLoading: false }); + this.endLoadingSession(); + this.setState({ error, file: undefined }); onError(fileError, errorCode, { error: fileError, }); @@ -1369,6 +1447,7 @@ class ContentPreview extends React.PureComponent { error, file, isLoading, + isDeferringLoading, isReloadNotificationVisible, isThumbnailSidebarOpen, selectedVersion, @@ -1439,6 +1518,7 @@ class ContentPreview extends React.PureComponent { + + + ); } - return ( -
- {errorCode ? : isLoading && } -
- ); + if (isDeferringLoading) { + return
; + } + + if (isLoading) { + return ( +
+ +
+ ); + } + + return null; } diff --git a/src/elements/content-preview/__tests__/ContentPreview.test.js b/src/elements/content-preview/__tests__/ContentPreview.test.js index 4baf2de9d4..a50443db30 100644 --- a/src/elements/content-preview/__tests__/ContentPreview.test.js +++ b/src/elements/content-preview/__tests__/ContentPreview.test.js @@ -661,6 +661,101 @@ describe('elements/content-preview/ContentPreview', () => { expect(instance.state.error).toBeUndefined(); expect(instance.state.isReloadNotificationVisible).toBeTruthy(); }); + + test('should keep isLoading true when loadingIndicatorShownThisSession is true and new file arrives', () => { + const wrapper = getWrapper({ ...props, loadingIndicatorDelayMs: 300 }); + const inst = wrapper.instance(); + inst.setState({ file: undefined, isLoading: true }); + inst.loadingIndicatorShownThisSession = true; + const newFile = { id: '456', file_version: { sha1: 'sha' } }; + inst.fetchFileSuccessCallback(newFile); + + expect(inst.state.file).toEqual(newFile); + expect(inst.state.isLoading).toBe(true); + }); + }); + + describe('loading indicator defer and timeout', () => { + test('componentDidMount sets up defer timer when loadingIndicatorDelayMs > 0', () => { + jest.useFakeTimers(); + const wrapper = getWrapper({ + token: 'token', + fileId: '123', + loadingIndicatorDelayMs: 200, + }); + const instance = wrapper.instance(); + + expect(typeof instance.loadingIndicatorDelayTimeoutId).toBe('number'); + expect(wrapper.state('isDeferringLoading')).toBe(true); + expect(wrapper.state('isLoading')).toBe(false); + + jest.useRealTimers(); + }); + + test('timeout firing transitions from isDeferringLoading to isLoading', () => { + jest.useFakeTimers(); + const wrapper = getWrapper({ + token: 'token', + fileId: '123', + loadingIndicatorDelayMs: 200, + }); + const instance = wrapper.instance(); + + expect(wrapper.state('isDeferringLoading')).toBe(true); + expect(instance.loadingIndicatorShownThisSession).toBe(false); + + jest.advanceTimersByTime(200); + + expect(wrapper.state('isDeferringLoading')).toBe(false); + expect(wrapper.state('isLoading')).toBe(true); + expect(instance.loadingIndicatorShownThisSession).toBe(true); + expect(instance.loadingIndicatorDelayTimeoutId).toBeUndefined(); + + jest.useRealTimers(); + }); + + test('endLoadingSession clears timeout before it fires (fast-load scenario)', () => { + jest.useFakeTimers(); + const wrapper = getWrapper({ + token: 'token', + fileId: '123', + loadingIndicatorDelayMs: 200, + }); + const instance = wrapper.instance(); + + instance.endLoadingSession(); + + jest.advanceTimersByTime(200); + + expect(wrapper.state('isLoading')).toBe(false); + expect(wrapper.state('isDeferringLoading')).toBe(false); + expect(instance.loadingIndicatorDelayTimeoutId).toBeUndefined(); + + jest.useRealTimers(); + }); + + test('getLoadingIndicatorDelayMs returns 0 for negative or non-numeric values', () => { + const wrapperNegative = getWrapper({ + token: 'token', + fileId: '123', + loadingIndicatorDelayMs: -100, + }); + expect(wrapperNegative.instance().getLoadingIndicatorDelayMs()).toBe(0); + + const wrapperNonNumeric = getWrapper({ + token: 'token', + fileId: '123', + loadingIndicatorDelayMs: 'not-a-number', + }); + expect(wrapperNonNumeric.instance().getLoadingIndicatorDelayMs()).toBe(0); + + const wrapperValid = getWrapper({ + token: 'token', + fileId: '123', + loadingIndicatorDelayMs: 150, + }); + expect(wrapperValid.instance().getLoadingIndicatorDelayMs()).toBe(150); + }); }); describe('fetchFileErrorCallback()', () => { @@ -873,6 +968,24 @@ describe('elements/content-preview/ContentPreview', () => { }); }); + describe('onPreviewError()', () => { + test('should end loading session and call onError', () => { + const onError = jest.fn(); + const wrapper = getWrapper({ ...props, onError }); + const inst = wrapper.instance(); + inst.setState({ isLoading: true }); + const errorPayload = { error: { code: 'some_code', message: 'msg' } }; + inst.onPreviewError(errorPayload); + expect(inst.state.isLoading).toBe(false); + expect(onError).toHaveBeenCalledWith( + errorPayload.error, + 'some_code', + expect.objectContaining({ error: errorPayload.error }), + expect.any(String), + ); + }); + }); + describe('onPreviewMetric()', () => { let wrapper; let instance; @@ -1060,12 +1173,14 @@ describe('elements/content-preview/ContentPreview', () => { expect(instance.loadPreview).toBeCalledTimes(1); }); - test("should update the loading state if fileId hasn't changed and shouldLoadPreview returns true", () => { + test('should not update loading state when shouldLoadPreview returns true (same session)', () => { instance.shouldLoadPreview = jest.fn().mockReturnValue(true); - wrapper.setState({ isLoading: false }); // Simulate existing preview - wrapper.setProps({ fileId: 'bar' }); + wrapper.setState({ isLoading: false }); + wrapper.setProps({ foo: 'bar' }); - expect(wrapper.state('isLoading')).toBe(true); + expect(instance.loadPreview).toHaveBeenCalled(); + expect(instance.destroyPreview).toHaveBeenCalledWith(false); + expect(wrapper.state('isLoading')).toBe(false); }); test('should update the preview with the new token if it changes', () => {