Skip to content

Conversation

@sanilsalvi
Copy link

@sanilsalvi sanilsalvi commented Jan 6, 2026

…optimization

Summary

Adds preloadUrlMap prop to ContentPreview and passes it to the preview SDK.

This enables the preview SDK to use prefetched image URLs from the backend, avoiding redundant API calls for document first pages that were already prefetched. The prop is optional and backward compatible.

Summary by CodeRabbit

  • Refactor
    • Improved internal code organization and clarity in the content preview functionality.

✏️ Tip: You can customize this high-level summary in your review settings.

@sanilsalvi sanilsalvi requested review from a team as code owners January 6, 2026 09:11
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

Walkthrough

The ContentPreview.js file is refactored to explicitly destructure state variables (startAt, file, selectedVersion) into local variables and to ensure Preview is retrieved from the global namespace before instantiation.

Changes

Cohort / File(s) Summary
State Destructuring and Preview Initialization
src/elements/content-preview/ContentPreview.js
Added explicit destructuring of startAt, file, and selectedVersion from this.state in the loadPreview method for direct variable usage. Introduced explicit retrieval of Preview from global.Box before object instantiation. No behavioral changes to preview logic or control flow.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A dash of destructuring, clean and bright,
State variables dancing into light,
Global Preview sourced with care,
Refactored code, beyond compare!
—Your friendly rabbit reviewer 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main feature: adding preloadUrlMap prop to enable prefetched image optimization in ContentPreview.
Description check ✅ Passed The description provides a clear summary of changes and explains the purpose and backward compatibility, though it lacks detailed implementation specifics and rationale.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e752826 and 4ffdd44.

📒 Files selected for processing (1)
  • src/elements/content-preview/ContentPreview.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: lint_test_build
  • GitHub Check: semgrep-cloud-platform/scan
  • GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-preview/ContentPreview.js (1)

819-820: LGTM - Safe refactoring changes.

The explicit destructuring of state variables at the beginning of loadPreview and the explicit retrieval of Preview from the global namespace are both safe refactoring changes. The state destructuring at line 820 is used consistently throughout the function, and the guard check at line 823 ensures global.Box exists before the destructuring at line 867.

These changes don't alter functionality and make the code slightly more explicit.

Also applies to: 866-867


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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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

128-128: LGTM! Consider more specific typing as a future enhancement.

The optional preloadUrlMap prop is correctly typed and follows the established pattern. The generic Object type is acceptable and consistent with similar props in this file (e.g., boxAnnotations, fileOptions).

Optional: More specific type definition

If the structure of preloadUrlMap is known, consider defining a more specific Flow type:

type PreloadUrlMap = {
  [format: string]: { [pageNumber: number]: string }
};

Then update the prop:

-    preloadUrlMap?: Object,
+    preloadUrlMap?: PreloadUrlMap,

This would provide better type safety and documentation, but is not required for the current implementation.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69be418 and e752826.

📒 Files selected for processing (2)
  • src/elements/content-preview/ContentPreview.js
  • src/elements/content-preview/__tests__/ContentPreview.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-preview/__tests__/ContentPreview.test.js (1)
src/elements/content-preview/ContentPreview.js (3)
  • props (292-302)
  • props (323-323)
  • props (378-378)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: lint_test_build
  • GitHub Check: semgrep-cloud-platform/scan
  • GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-preview/ContentPreview.js (2)

815-815: LGTM!

The prop is correctly destructured from this.props and follows the established pattern in this method.


859-859: LGTM!

The preloadUrlMap is correctly added to the preview options object and will be passed to the preview SDK. This follows the established pattern for optional configuration props.

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

448-468: LGTM! Well-structured test case.

The test correctly verifies that preloadUrlMap is passed through to preview.show() when provided. The test follows the established pattern in this file and provides a clear example of the expected data structure.

[name: string]: TargetingApi,
},
previewLibraryVersion: string,
preloadUrlMap?: Object,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is how we want to do this. Take a look at the comments on the corresponding PR in EUA.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to close this PR as we won't need anything to be done here. The current approach is to pass the generated token as part of the preview metadata in file cache which is already supported here.

@sanilsalvi
Copy link
Author

The current approach is to pass the generated token as part of the preview metadata in file cache which is already supported here.

@sanilsalvi sanilsalvi closed this Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants