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

[sanilsalvi]

Description:

• Adds optional loadingIndicatorDelayMs prop: when set, the loading spinner is shown only after that delay; if the preview loads first, the spinner is never shown.
• Fixes spinner disappearing and reappearing mid-session: once the spinner is shown, it stays until the preview has loaded or errored (session-based, no flicker).
• One defer timer per load session (per file); session ends on load, preview error, or file fetch error.
• PreviewMask simplified to error / defer / loading states. Unit tests updated and extended for delay and session behavior.
• Backward compatible: default is immediate spinner when the prop is omitted or 0

Loading Indicator experience in local env

Local.Delay.Loading.Indicator.mov

Summary by CodeRabbit

• New Features

  • Configurable delay for the loading indicator so the spinner appears only after a specified duration.
  • Deferred loading state to reduce spinner flicker and improve preview responsiveness.
  • Preview mask now respects the deferred state to avoid showing UI chrome during deferral.

• Bug Fixes

  • Improved handling of consecutive file loads, errors, and finish paths to maintain consistent loading behavior and clear timers.

• Tests

  • Expanded tests for defer timing, session behavior, error handling, and fast-load scenarios.

[coderabbitai]

Walkthrough

ContentPreview adds a deferred loading spinner controlled by loadingIndicatorDelayMs, tracked via isDeferringLoading and session flags. Timeout lifecycle and state transitions are managed across mount, unmount, file changes, success/error paths, propagated to PreviewMask; tests for defer/session behavior were added.

Changes

Cohort / File(s)	Summary
Deferred loading core src/elements/content-preview/ContentPreview.js	Adds loadingIndicatorDelayMs prop and default, isDeferringLoading state and initial value, session fields (loadingIndicatorShownThisSession, loadingIndicatorDelayTimeoutId), helper methods (getLoadingIndicatorDelayMs, clearLoadingIndicatorDelayTimeout, endLoadingSession), and lifecycle logic to start/clear defer timers and maintain consistent loading state across mount/unmount, file reloads, success/error paths.
Mask rendering src/elements/content-preview/PreviewMask.tsx	Adds isDeferringLoading prop (default false) and updates rendering branches: explicit handling for error, deferring (renders empty wrapper), loading (renders PreviewLoading), otherwise null.
Tests src/elements/content-preview/__tests__/ContentPreview.test.js	Adds tests for defer timer setup/timeout transition, fast-load clearing of timeout, normalization of negative/non-numeric delays, session-aware spinner behavior on subsequent files, and onPreviewError ending the loading session; updates existing loading-state expectations to reflect session semantics.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ContentPreview
  participant PreviewInstance
  participant PreviewMask

  Client->>ContentPreview: mount / open file (with loadingIndicatorDelayMs > 0)
  ContentPreview-->>ContentPreview: set isDeferringLoading = true, isLoading = false, start timeout
  ContentPreview->>PreviewInstance: init preview (include delay)
  Note over ContentPreview,PreviewMask: while deferring, PreviewMask sees isDeferringLoading
  alt timeout fires
    ContentPreview-->>ContentPreview: clear timeout, set isDeferringLoading = false, set isLoading = true
    ContentPreview->>PreviewMask: render loading
    PreviewMask->>Client: show spinner
  else file loads or errors before timeout
    ContentPreview-->>ContentPreview: clear timeout, endLoadingSession()
    ContentPreview->>PreviewMask: render error or no-loading
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

ready-to-merge

Suggested reviewers

• jmcbgaston
• JChan106
• tjuanitas

Poem

🐇 I hid the spinner for just a small while,
A patient hop, a timeout, a pause with a smile.
If the file is quick, I tuck it away—
If not, I spin gently and greet the display. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: introducing deferred loading indicator to prevent brief flicker.
Description check	✅ Passed	The pull request provides a comprehensive description that clearly explains the feature, implementation approach, and backward compatibility, with a demonstration video.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/elements/content-preview/ContentPreview.js (1)

306-329: 🛠️ Refactor suggestion | 🟠 Major

Undeclared instance properties and redundant defer-state setup in constructor.

Two observations:

1. loadingIndicatorShownThisSession and loadingIndicatorDelayTimeoutId are used as instance properties throughout the class but are never declared in the class body (unlike preview, api, fetchFileEndTime, etc.). Declare them for consistency and Flow typing.

2. The constructor (Line 325) and componentDidMount (Lines 416–422) both set { isLoading: false, isDeferringLoading: true } when delayMs > 0. The constructor state is immediately overwritten by componentDidMount, making the constructor spread redundant. Consider removing the defer logic from the constructor and keeping it only in componentDidMount, which is where the timeout actually starts.

Proposed class-body declarations

     previewLibraryLoaded: boolean = false;
 
     updateVersionToCurrent: ?() => void;
 
     dynamicOnPreviewLoadAction: ?() => void;
+
+    loadingIndicatorShownThisSession: boolean = false;
+
+    loadingIndicatorDelayTimeoutId: ?TimeoutID;

🤖 Fix all issues with AI agents

In `@src/elements/content-preview/ContentPreview.js`:
- Around line 457-472: When handling a file-change in componentDidUpdate, avoid
starting the deferred loading timeout when there is no new file to load: before
creating loadingIndicatorDelayTimeoutId or calling setTimeout in the
hasFileIdChanged branch, check that currentFileId is truthy (or alternatively
call endLoadingSession immediately when fetchFile would short-circuit); update
componentDidUpdate to either guard the timeout setup with a currentFileId
truthiness check or to call endLoadingSession/destroyPreview to clear any
pending timeout if fetchFile returns early, ensuring
loadingIndicatorShownThisSession and isLoading are not toggled for an absent
file.
- Line 152: Remove the development-only inline comment on the isDeferringLoading
prop in the ContentPreview component: locate the prop declaration
(isDeferringLoading?: boolean) in ContentPreview (ContentPreview.js) and delete
the trailing "// DEMO: deferred spinner cues – remove for production" comment so
the prop line contains only the type/signature.

🧹 Nitpick comments (1)

src/elements/content-preview/__tests__/ContentPreview.test.js (1)

888-904: Consider adding tests for core defer/timeout behavior.

The new tests cover error and session-flag scenarios well, but there's no test coverage for:

• componentDidMount setting up the defer timer when loadingIndicatorDelayMs > 0
• The timeout firing and transitioning from isDeferringLoading: true → isLoading: true
• endLoadingSession clearing the timeout before it fires (fast-load scenario)
• getLoadingIndicatorDelayMs handling negative or non-numeric values

These would strengthen confidence in the timer lifecycle, especially around edge cases. You can use jest.useFakeTimers() / jest.advanceTimersByTime() to test timeout behavior deterministically.

Also applies to: 664-675