feat(content-preview): defer loading indicator to avoid brief flicker

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,
+    isLoadingDeferred?: 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<Props, State> {
         canPrint: false,
         error: undefined,
         isLoading: true,
+        isLoadingDeferred: false,
         isReloadNotificationVisible: false,
         isThumbnailSidebarOpen: false,
     };
@@ -255,6 +258,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
         enableThumbnailsSidebar: false,
         hasHeader: false,
         language: DEFAULT_LOCALE,
+        loadingIndicatorDelayMs: 0,
         onAnnotator: noop,
         onAnnotatorEvent: noop,
         onContentInsightsEventReport: noop,
@@ -281,6 +285,10 @@ class ContentPreview extends React.PureComponent<Props, State> {
      */
     fetchFileStartTime: ?number;
 
+    loadingWasDeferred: boolean;
+
+    loadingIndicatorDelayTimeoutId: ?TimeoutID;
+
     /**
      * [constructor]
      *
@@ -293,6 +301,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
             cache,
             fileId,
             language,
+            loadingIndicatorDelayMs,
             requestInterceptor,
             responseInterceptor,
             sharedLink,
@@ -301,6 +310,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
         } = props;
 
         this.id = uniqueid('bcpr_');
+        this.loadingWasDeferred = false;
         this.api = new API({
             apiHost,
             cache,
@@ -315,6 +325,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
         });
         this.state = {
             ...this.initialState,
+            ...(loadingIndicatorDelayMs ? { isLoadingDeferred: true } : {}),
             currentFileId: fileId,
             // eslint-disable-next-line react/no-unused-state
             prevFileIdProp: fileId,
@@ -337,9 +348,10 @@ class ContentPreview extends React.PureComponent<Props, State> {
     }
 
     /**
-     * Cleans up the preview instance
+     * Cleans up the preview instance.
      */
     destroyPreview(shouldReset: boolean = true) {
+        this.clearLoadingIndicatorDelayTimeout();
         const { onPreviewDestroy } = this.props;
         if (this.preview) {
             this.preview.destroy();
@@ -350,6 +362,23 @@ class ContentPreview extends React.PureComponent<Props, State> {
         }
     }
 
+    clearLoadingIndicatorDelayTimeout(): void {
+        if (this.loadingIndicatorDelayTimeoutId != null) {
+            clearTimeout(this.loadingIndicatorDelayTimeoutId);
+            this.loadingIndicatorDelayTimeoutId = null;
+        }
+    }
+
+    /**
+     * 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.loadingWasDeferred = false;
+        this.setState({ isLoading: false, isLoadingDeferred: false });
+    };
+
     /**
      * Destroys api instances with caches
      *
@@ -369,7 +398,18 @@ class ContentPreview extends React.PureComponent<Props, State> {
         this.loadStylesheet();
         this.loadScript();
 
-        this.fetchFile(this.state.currentFileId);
+        const { currentFileId } = this.state;
+        const { loadingIndicatorDelayMs } = this.props;
+
+        if (currentFileId && loadingIndicatorDelayMs) {
+            this.loadingIndicatorDelayTimeoutId = setTimeout(() => {
+                this.loadingIndicatorDelayTimeoutId = null;
+                this.loadingWasDeferred = true;
+                this.setState({ isLoadingDeferred: false });
+            }, loadingIndicatorDelayMs);
+        }
+
+        this.fetchFile(currentFileId);
         this.focusPreview();
     }
 
@@ -402,10 +442,21 @@ class ContentPreview extends React.PureComponent<Props, State> {
             features?.advancedContentInsights,
         );
         const haveExperiencesChanged = prevPreviewExperiences !== previewExperiences;
+        const { loadingIndicatorDelayMs } = this.props;
 
         if (hasFileIdChanged) {
             this.destroyPreview();
-            this.setState({ isLoading: true, selectedVersion: undefined });
+            this.loadingWasDeferred = false;
+            if (loadingIndicatorDelayMs) {
+                this.setState({ isLoading: true, isLoadingDeferred: true, selectedVersion: undefined });
+                this.loadingIndicatorDelayTimeoutId = setTimeout(() => {
+                    this.loadingIndicatorDelayTimeoutId = null;
+                    this.loadingWasDeferred = true;
+                    this.setState({ isLoadingDeferred: false });
+                }, loadingIndicatorDelayMs);
+            } else {
+                this.setState({ isLoading: true, selectedVersion: undefined });
+            }
             this.fetchFile(currentFileId);
         } else if (this.shouldLoadPreview(prevState)) {
             this.destroyPreview(false);
@@ -629,8 +680,8 @@ class ContentPreview extends React.PureComponent<Props, State> {
         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 +790,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
 
         onLoad(loadData);
 
-        this.setState({ isLoading: false });
+        this.endLoadingSession();
         this.focusPreview();
 
         if (this.preview && filesToPrefetch.length) {
@@ -865,6 +916,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
         const { Preview } = global.Box;
         this.preview = new Preview();
         this.preview.addListener('load', this.onPreviewLoad);
+        this.preview.addListener('preload', this.endLoadingSession);
 
         this.preview.addListener('preview_error', this.onPreviewError);
         this.preview.addListener('preview_metric', this.onPreviewMetric);
@@ -934,7 +986,12 @@ class ContentPreview extends React.PureComponent<Props, State> {
         // 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 isDeferred = this.props.loadingIndicatorDelayMs && !this.loadingWasDeferred;
+            this.setState({
+                ...this.initialState,
+                file,
+                ...(isDeferred ? { isLoadingDeferred: true } : {}),
+            });
             // $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 +1016,8 @@ class ContentPreview extends React.PureComponent<Props, State> {
             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 +1427,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
             error,
             file,
             isLoading,
+            isLoadingDeferred,
             isReloadNotificationVisible,
             isThumbnailSidebarOpen,
             selectedVersion,
@@ -1440,6 +1499,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
                                             errorCode={errorCode}
                                             extension={currentExtension}
                                             isLoading={isLoading}
+                                            isLoadingDeferred={isLoadingDeferred}
                                         />
                                         <PreviewNavigation
                                             collection={collection}

src/elements/content-preview/PreviewMask.tsx

@@ -8,16 +8,26 @@ export type Props = {
     errorCode?: string;
     extension?: string;
     isLoading?: boolean;
+    isLoadingDeferred?: boolean;
 };
 
-export default function PreviewMask({ errorCode, extension, isLoading }: Props): React.ReactElement | null {
-    if (!errorCode && !isLoading) {
-        return null;
+export default function PreviewMask({
+    errorCode,
+    extension,
+    isLoading,
+    isLoadingDeferred = false,
+}: Props): React.ReactElement | null {
+    if (errorCode) {
+        return (
+            <div className="bcpr-PreviewMask">
+                <PreviewError errorCode={errorCode} />
+            </div>
+        );
     }
 
-    return (
-        <div className="bcpr-PreviewMask">
-            {errorCode ? <PreviewError errorCode={errorCode} /> : isLoading && <PreviewLoading extension={extension} />}
-        </div>
-    );
+    if (isLoading) {
+        return <div className="bcpr-PreviewMask">{!isLoadingDeferred && <PreviewLoading extension={extension} />}</div>;
+    }
+
+    return null;
 }

src/elements/content-preview/__tests__/ContentPreview.test.js

@@ -661,6 +661,93 @@ describe('elements/content-preview/ContentPreview', () => {
             expect(instance.state.error).toBeUndefined();
             expect(instance.state.isReloadNotificationVisible).toBeTruthy();
         });
+
+        test('should keep isLoading true when loadingWasDeferred is true and new file arrives', () => {
+            const delayWrapper = getWrapper({ ...props, loadingIndicatorDelayMs: 300 });
+            const delayInstance = delayWrapper.instance();
+            delayInstance.setState({ file: undefined, isLoading: true });
+            delayInstance.loadingWasDeferred = true;
+            const newFile = { id: '456', file_version: { sha1: 'sha' } };
+            delayInstance.fetchFileSuccessCallback(newFile);
+
+            expect(delayInstance.state.file).toEqual(newFile);
+            expect(delayInstance.state.isLoading).toBe(true);
+            expect(delayInstance.state.isLoadingDeferred).toBe(false);
+        });
+
+        test('should keep isLoadingDeferred true when spinner has not been shown yet', () => {
+            const delayWrapper = getWrapper({ ...props, loadingIndicatorDelayMs: 300 });
+            const delayInstance = delayWrapper.instance();
+            delayInstance.setState({ file: undefined, isLoading: true, isLoadingDeferred: true });
+            delayInstance.loadingWasDeferred = false;
+            const newFile = { id: '456', file_version: { sha1: 'sha' } };
+            delayInstance.fetchFileSuccessCallback(newFile);
+
+            expect(delayInstance.state.file).toEqual(newFile);
+            expect(delayInstance.state.isLoading).toBe(true);
+            expect(delayInstance.state.isLoadingDeferred).toBe(true);
+        });
+    });
+
+    describe('loading indicator defer and timeout', () => {
+        test('componentDidMount sets up defer timer when loadingIndicatorDelayMs is set', () => {
+            jest.useFakeTimers();
+            const wrapper = getWrapper({
+                token: 'token',
+                fileId: '123',
+                loadingIndicatorDelayMs: 200,
+            });
+            const instance = wrapper.instance();
+
+            expect(instance.loadingIndicatorDelayTimeoutId).not.toBeNull();
+            expect(wrapper.state('isLoadingDeferred')).toBe(true);
+            expect(wrapper.state('isLoading')).toBe(true);
+
+            jest.useRealTimers();
+        });
+
+        test('timeout firing clears isLoadingDeferred so spinner becomes visible', () => {
+            jest.useFakeTimers();
+            const wrapper = getWrapper({
+                token: 'token',
+                fileId: '123',
+                loadingIndicatorDelayMs: 200,
+            });
+            const instance = wrapper.instance();
+
+            expect(wrapper.state('isLoadingDeferred')).toBe(true);
+            expect(wrapper.state('isLoading')).toBe(true);
+            expect(instance.loadingWasDeferred).toBe(false);
+
+            jest.advanceTimersByTime(200);
+
+            expect(wrapper.state('isLoadingDeferred')).toBe(false);
+            expect(wrapper.state('isLoading')).toBe(true);
+            expect(instance.loadingWasDeferred).toBe(true);
+            expect(instance.loadingIndicatorDelayTimeoutId).toBeNull();
+
+            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('isLoadingDeferred')).toBe(false);
+            expect(instance.loadingIndicatorDelayTimeoutId).toBeNull();
+
+            jest.useRealTimers();
+        });
     });
 
     describe('fetchFileErrorCallback()', () => {
@@ -873,6 +960,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 instance = wrapper.instance();
+            instance.setState({ isLoading: true });
+            const errorPayload = { error: { code: 'some_code', message: 'msg' } };
+            instance.onPreviewError(errorPayload);
+            expect(instance.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,11 +1165,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 set isLoading true when shouldLoadPreview returns true', () => {
             instance.shouldLoadPreview = jest.fn().mockReturnValue(true);
-            wrapper.setState({ isLoading: false }); // Simulate existing preview
-            wrapper.setProps({ fileId: 'bar' });
+            wrapper.setState({ isLoading: false });
+            // Trigger componentDidUpdate without changing fileId
+            wrapper.setProps({ foo: 'bar' });
 
+            expect(instance.loadPreview).toHaveBeenCalled();
+            expect(instance.destroyPreview).toHaveBeenCalledWith(false);
             expect(wrapper.state('isLoading')).toBe(true);
         });