From 4472d105d143f30c5290a055de8be10453089cae Mon Sep 17 00:00:00 2001 From: Sanil Salvi Date: Fri, 6 Feb 2026 15:13:36 -0800 Subject: [PATCH 1/2] feat(content-preview): defer loading indicator to avoid brief flicker --- .../content-preview/ContentPreview.js | 93 +++++++++++++++++-- src/elements/content-preview/PreviewMask.tsx | 34 +++++-- .../__tests__/ContentPreview.test.js | 40 +++++++- 3 files changed, 146 insertions(+), 21 deletions(-) diff --git a/src/elements/content-preview/ContentPreview.js b/src/elements/content-preview/ContentPreview.js index 8eeb9b6eed..cb51feca73 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, // DEMO: deferred spinner cues – remove for production 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, @@ -301,6 +305,7 @@ class ContentPreview extends React.PureComponent { } = props; this.id = uniqueid('bcpr_'); + this.loadingIndicatorShownThisSession = false; this.api = new API({ apiHost, cache, @@ -313,8 +318,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 +339,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 +364,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 +397,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 +405,23 @@ 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.setState({ isLoading: false, isDeferringLoading: true }); + this.loadingIndicatorDelayTimeoutId = setTimeout(() => { + this.loadingIndicatorDelayTimeoutId = undefined; + this.loadingIndicatorShownThisSession = true; + this.setState({ isLoading: true, isDeferringLoading: false }); + }, delayMs); + } + + this.fetchFile(currentFileId); this.focusPreview(); } @@ -402,14 +454,24 @@ 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 (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 +691,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 +801,7 @@ class ContentPreview extends React.PureComponent { onLoad(loadData); - this.setState({ isLoading: false }); + this.endLoadingSession(); this.focusPreview(); if (this.preview && filesToPrefetch.length) { @@ -854,6 +916,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 +997,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 +1031,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 +1442,7 @@ class ContentPreview extends React.PureComponent { error, file, isLoading, + isDeferringLoading, isReloadNotificationVisible, isThumbnailSidebarOpen, selectedVersion, @@ -1439,6 +1513,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..fb848b0946 100644 --- a/src/elements/content-preview/__tests__/ContentPreview.test.js +++ b/src/elements/content-preview/__tests__/ContentPreview.test.js @@ -661,6 +661,18 @@ 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('fetchFileErrorCallback()', () => { @@ -873,6 +885,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 +1090,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', () => { From de52cc9f45d555c28fc3eff2697414ee19476d0f Mon Sep 17 00:00:00 2001 From: Sanil Salvi Date: Sun, 8 Feb 2026 16:35:40 -0800 Subject: [PATCH 2/2] feat(content-preview): address AI feedback --- .../content-preview/ContentPreview.js | 11 ++- .../__tests__/ContentPreview.test.js | 83 +++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/elements/content-preview/ContentPreview.js b/src/elements/content-preview/ContentPreview.js index cb51feca73..c1132fc855 100644 --- a/src/elements/content-preview/ContentPreview.js +++ b/src/elements/content-preview/ContentPreview.js @@ -149,7 +149,7 @@ type State = { error?: ErrorType, file?: BoxItem, isLoading: boolean, - isDeferringLoading?: boolean, // DEMO: deferred spinner cues – remove for production + isDeferringLoading?: boolean, isReloadNotificationVisible: boolean, isThumbnailSidebarOpen: boolean, prevFileIdProp?: string, // the previous value of the "fileId" prop. Needed to implement getDerivedStateFromProps @@ -285,6 +285,10 @@ class ContentPreview extends React.PureComponent { */ fetchFileStartTime: ?number; + loadingIndicatorShownThisSession: boolean; + + loadingIndicatorDelayTimeoutId: TimeoutID | void; + /** * [constructor] * @@ -413,7 +417,6 @@ class ContentPreview extends React.PureComponent { const delayMs = this.getLoadingIndicatorDelayMs(); if (delayMs > 0) { - this.setState({ isLoading: false, isDeferringLoading: true }); this.loadingIndicatorDelayTimeoutId = setTimeout(() => { this.loadingIndicatorDelayTimeoutId = undefined; this.loadingIndicatorShownThisSession = true; @@ -459,7 +462,9 @@ class ContentPreview extends React.PureComponent { if (hasFileIdChanged) { this.destroyPreview(); this.loadingIndicatorShownThisSession = false; - if (delayMs > 0) { + if (!currentFileId) { + this.endLoadingSession(); + } else if (delayMs > 0) { this.setState({ isLoading: false, isDeferringLoading: true, selectedVersion: undefined }); this.loadingIndicatorDelayTimeoutId = setTimeout(() => { this.loadingIndicatorDelayTimeoutId = undefined; diff --git a/src/elements/content-preview/__tests__/ContentPreview.test.js b/src/elements/content-preview/__tests__/ContentPreview.test.js index fb848b0946..a50443db30 100644 --- a/src/elements/content-preview/__tests__/ContentPreview.test.js +++ b/src/elements/content-preview/__tests__/ContentPreview.test.js @@ -675,6 +675,89 @@ describe('elements/content-preview/ContentPreview', () => { }); }); + 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()', () => { let instance; let error;