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

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.

• Bug Fixes

  • Improved handling of consecutive file loads and error/finish paths to maintain consistent loading behavior.

• Tests

  • Expanded tests for loading defer timing, session behavior, and error handling.

[coderabbitai]

Walkthrough

ContentPreview gains 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, and propagated to PreviewMask; tests added for defer and session behavior.

Changes

Cohort / File(s)	Summary
Deferred loading core src/elements/content-preview/ContentPreview.js	Adds loadingIndicatorDelayMs prop, isDeferringLoading state, session fields (loadingIndicatorShownThisSession, loadingIndicatorDelayTimeoutId), helper methods (getLoadingIndicatorDelayMs, clearLoadingIndicatorDelayTimeout, endLoadingSession), and lifecycle logic to defer/clear the loading spinner across mount/unmount, file reloads, success/error paths.
Mask rendering src/elements/content-preview/PreviewMask.tsx	Introduces isDeferringLoading prop and updates rendering branches: error -> PreviewError, deferring -> empty wrapper, loading -> PreviewLoading, otherwise null. Adjusts prop destructuring and default.
Tests src/elements/content-preview/__tests__/ContentPreview.test.js	Adds tests covering defer timeout setup/cleanup, fast-load behavior, negative/invalid delay bounds, session-aware spinner behavior, and onPreviewError session termination; updates existing loading-state expectations to match new session semantics.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ContentPreview
  participant PreviewMask
  participant PreviewInstance

  Client->>ContentPreview: mount / open file (with loadingIndicatorDelayMs>0)
  ContentPreview-->>ContentPreview: set isDeferringLoading=true, start timeout
  ContentPreview->>PreviewInstance: init (include delay)
  Note over ContentPreview,PreviewMask: while deferring, isLoading=false
  alt timeout fires
    ContentPreview-->>ContentPreview: clear timeout, set isDeferringLoading=false, set isLoading=true
    ContentPreview->>PreviewMask: render loading
    PreviewMask->>Client: show spinner
  else file loads quickly / error occurs 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

Poem

🐇 I hid the spinner in a burrow of time,

A patient little twirl postponed by design.

When timeout blooms, I hop and spin with glee—

But if the file finishes fast, I nap quietly. 🥕✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is incomplete; it lacks required template sections and instead only contains implementation details without addressing the merge queue and approval process outlined in the template.	Replace the current description with the required template structure, ensuring all merge queue procedures and approval requirements are clearly documented.
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 (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding deferred loading indicator functionality to avoid brief spinner flicker.

_{✏️ 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

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

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

1004-1014: fetchFileSuccessCallback deferred-loading logic is correct but could benefit from a brief inline clarification.

After the initialState spread (which sets isDeferringLoading: false), the running timeout is intentionally left active. If it fires later, it will show the spinner — this is correct per the session-based design. However, a reader might wonder why isDeferringLoading isn't restored to true while the timeout is still pending.

Consider adding a short comment clarifying that isDeferringLoading is intentionally left false here because the file content area is now rendering (since file is set), so the "deferring" placeholder in PreviewMask is no longer needed — only isLoading matters from this point.

---

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