feat(preview): support loading the Preview library from npm#4695
feat(preview): support loading the Preview library from npm#4695jackiejou wants to merge 1 commit into
Conversation
Walkthrough
ChangesNPM preview integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ContentPreview
participant box_content_preview
participant Preview
User->>ContentPreview: Request content preview
ContentPreview->>box_content_preview: Dynamically import package and styles
box_content_preview-->>ContentPreview: Return Preview export
ContentPreview->>Preview: Call show with location and pdfjs options
Preview-->>User: Display preview
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/elements/content-preview/ContentPreview.js (1)
510-538: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
loadNpmPreview()isn't reentrant-safe, and there's no visible unmount guard.The guard
if (this.npmPreviewModule) return;only checks the final cached result, not whether a load is already in flight. If this method is ever invoked a second time before the firstPromise.allresolves (e.g. a future retry path, or as seen in the test atContentPreview.test.jslines 2555-2565 wherecomponentDidMount's fire-and-forget call races an explicitawait instance.loadNpmPreview()), both calls will independently import the module, assignnpmPreviewModule, and callthis.loadPreview()— duplicatingnew Preview()/.show()invocations. Separately, nothing here appears to check whether the component is still mounted before the post-awaitsetState/loadPreview()/error-path calls.♻️ Proposed fix: memoize the in-flight promise
- loadNpmPreview = async (): Promise<void> => { - if (this.npmPreviewModule) { - return; - } - - let previewModule; - try { - [previewModule] = await Promise.all([ - import(/* webpackChunkName: "box-content-preview" */ 'box-content-preview'), - import(/* webpackChunkName: "box-content-preview" */ 'box-content-preview/styles.css'), - ]); - } catch { - this.onNpmPreviewLoadError('Failed to load the box-content-preview module'); - return; - } - - if (!previewModule.Preview) { - this.onNpmPreviewLoadError('box-content-preview module has no Preview export'); - return; - } - - this.npmPreviewModule = previewModule; - this.loadPreview(); - }; + loadNpmPreview = (): Promise<void> => { + if (this.npmPreviewModule) { + return Promise.resolve(); + } + if (this.npmPreviewLoadPromise) { + return this.npmPreviewLoadPromise; + } + + this.npmPreviewLoadPromise = (async () => { + let previewModule; + try { + [previewModule] = await Promise.all([ + import(/* webpackChunkName: "box-content-preview" */ 'box-content-preview'), + import(/* webpackChunkName: "box-content-preview" */ 'box-content-preview/styles.css'), + ]); + } catch { + this.onNpmPreviewLoadError('Failed to load the box-content-preview module'); + return; + } + + if (!previewModule.Preview) { + this.onNpmPreviewLoadError('box-content-preview module has no Preview export'); + return; + } + + this.npmPreviewModule = previewModule; + this.loadPreview(); + })(); + + return this.npmPreviewLoadPromise; + };Please also confirm whether
setState/loadPreview()calls after theawaitare already guarded against post-unmount execution elsewhere in this class (e.g. incomponentWillUnmount); if not, this same pattern would benefit from a mount-check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-preview/ContentPreview.js` around lines 510 - 538, Update loadNpmPreview to memoize and reuse an in-flight loading promise, so concurrent callers perform only one import and one subsequent loadPreview invocation. Ensure the promise reference is cleared appropriately after completion, preserve existing module and error handling, and verify the class’s componentWillUnmount or equivalent lifecycle state prevents post-await error handling or loadPreview execution after unmount; add the necessary mounted guard if none exists.src/elements/content-preview/__tests__/ContentPreview.test.js (1)
2620-2623: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
jest.dontMockmay not safely restore the original virtual mock for later tests.
jest.dontMock('box-content-preview')tells Jest to resolve the real module on nextrequire(), which is a different operation from re-registering the original hoistedjest.mock(...)factory. Sincebox-content-previewisn't installed in CI, if any test added after this block ever re-importsbox-content-previewrelying on the top-of-file virtual mock, it could fail to resolve. It works today only because this appears to be the last describe block in the file.♻️ More robust cleanup: re-register the healthy mock instead of `dontMock`
afterEach(() => { - jest.dontMock('box-content-preview'); jest.resetModules(); + jest.doMock( + 'box-content-preview', + () => ({ + Preview: function Preview() { + this.addListener = jest.fn(); + this.destroy = jest.fn(); + this.removeAllListeners = jest.fn(); + this.show = jest.fn(); + this.updateFileCache = jest.fn(); + }, + }), + { virtual: true }, + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-preview/__tests__/ContentPreview.test.js` around lines 2620 - 2623, Update the afterEach cleanup in the ContentPreview test block to re-register the original virtual mock for box-content-preview instead of calling jest.dontMock. Preserve jest.resetModules so later tests resolve the same hoisted mock factory safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/elements/content-preview/__tests__/ContentPreview.test.js`:
- Around line 2620-2623: Update the afterEach cleanup in the ContentPreview test
block to re-register the original virtual mock for box-content-preview instead
of calling jest.dontMock. Preserve jest.resetModules so later tests resolve the
same hoisted mock factory safely.
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 510-538: Update loadNpmPreview to memoize and reuse an in-flight
loading promise, so concurrent callers perform only one import and one
subsequent loadPreview invocation. Ensure the promise reference is cleared
appropriately after completion, preserve existing module and error handling, and
verify the class’s componentWillUnmount or equivalent lifecycle state prevents
post-await error handling or loadPreview execution after unmount; add the
necessary mounted guard if none exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 054ec505-e485-4dfd-b492-8d516308a5cf
📒 Files selected for processing (5)
.flowconfigflow/BoxContentPreviewStub.js.flowpackage.jsonsrc/elements/content-preview/ContentPreview.jssrc/elements/content-preview/__tests__/ContentPreview.test.js
0c91689 to
4c3af68
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 515-538: Update the catch block in loadNpmPreview to capture the
thrown import error and pass its details to onNpmPreviewLoadError instead of
discarding it. Preserve the existing early return and distinguish
module-versus-stylesheet failures when possible so downstream onError/logging
reflects the original cause.
- Around line 544-552: Normalize staticPath at the URL join points in
getNpmPreviewLocation() and getBasePath(), removing leading and trailing slashes
before concatenation so preview asset URLs contain exactly one separator. Apply
the same behavior consistently in both methods without changing the surrounding
host, locale, or version handling.
- Around line 1092-1102: Guard the npm preview rendering flow so
componentDidUpdate/loadPreview cannot instantiate a Preview before
loadNpmPreview has completed. In the Preview selection and construction path,
require a loaded npm preview module when npmPreviewModule is enabled; otherwise
defer or return until it is available, while preserving the existing
global.Box.Preview fallback for non-npm previews.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5018b90-385b-4ff3-9f4e-95525969d71d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.storybook/main.tspackage.jsonscripts/webpack.config.jssrc/elements/content-preview/ContentPreview.jssrc/elements/content-preview/__tests__/ContentPreview.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/elements/content-preview/tests/ContentPreview.test.js
4c3af68 to
dfed52c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/elements/content-preview/ContentPreview.js (1)
546-554: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
staticPathstill isn't normalized before concatenation.
trailingSlashonly guards the join betweenstaticHostandstaticPath; it doesn't strip a leading/trailing slash already present onstaticPathitself. A caller-suppliedstaticPathof/foo/orfoo/produces a double slash instaticBaseURI, which can break asset resolution in the npm-loaded viewer. This was flagged previously and remains unresolved (no "Addressed" marker, unlike the two other prior findings on this file).🩹 Proposed fix to normalize staticPath
getNpmPreviewLocation(): { locale: string, staticBaseURI: string, version: string } { const { language, previewLibraryVersion, staticHost, staticPath } = this.props; - const trailingSlash = staticHost.endsWith('/') ? '' : '/'; + const normalizedHost = staticHost.replace(/\/+$/, ''); + const normalizedPath = staticPath.replace(/^\/+|\/+$/g, ''); return { locale: language, - staticBaseURI: `${staticHost}${trailingSlash}${staticPath}/`, + staticBaseURI: `${normalizedHost}/${normalizedPath}/`, version: previewLibraryVersion, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-preview/ContentPreview.js` around lines 546 - 554, Update getNpmPreviewLocation to normalize staticPath before constructing staticBaseURI: remove leading and trailing slashes, then concatenate the normalized value with staticHost and the existing separator so inputs such as “/foo/” and “foo/” produce a single-slash path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 546-554: Update getNpmPreviewLocation to normalize staticPath
before constructing staticBaseURI: remove leading and trailing slashes, then
concatenate the normalized value with staticHost and the existing separator so
inputs such as “/foo/” and “foo/” produce a single-slash path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af909572-a4c9-45e8-aff2-274d95205386
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.storybook/main.tspackage.jsonscripts/webpack.config.jssrc/elements/content-preview/ContentPreview.jssrc/elements/content-preview/__tests__/ContentPreview.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- scripts/webpack.config.js
- src/elements/content-preview/tests/ContentPreview.test.js
Add optional box-content-preview peer dep and a useNpmBoxContentPreview-gated load path that dynamically imports the package, forwarding pdfjs workerSrc and location (staticBaseURI/version/locale) to Preview.show().
dfed52c to
c5a2831
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 515-531: Guard the asynchronous loadNpmPreview flow with the
component’s mounted/destroyed state after the dynamic imports resolve or reject,
before invoking onNpmPreviewLoadError, loadPreview, or any setState-triggering
logic. Update componentWillUnmount to mark the instance destroyed, and ensure
both success and failure paths return without acting once unmounted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e2de9b7-718c-4542-8c78-fcc3610e1122
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.storybook/main.tspackage.jsonscripts/webpack.config.jssrc/elements/content-preview/ContentPreview.jssrc/elements/content-preview/__tests__/ContentPreview.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- scripts/webpack.config.js
- .storybook/main.ts
- src/elements/content-preview/tests/ContentPreview.test.js
| "@hapi/address": "^2.1.4", | ||
| "@tanstack/react-virtual": "^3.13.12", | ||
| "axios": "^0.32.0", | ||
| "box-content-preview": "^3.62.0", |
There was a problem hiding this comment.
[question] Why is this version different from the one above?
There was a problem hiding this comment.
whoops good catch
There was a problem hiding this comment.
but the ^ caret would allow us to get 3.62.1 also
| if (this.shouldUseNpmPreview()) { | ||
| this.loadNpmPreview(); | ||
| } else { | ||
| this.loadStylesheet(); | ||
| this.loadScript(); |
There was a problem hiding this comment.
[question] Eventually shouldUseNpmPreview feature related code will be cleaned up I assume? In that case, will other consumer that has been leveraging the CDN script also be forced to switch to using npm installed version only?
| */ | ||
| componentDidMount(): void { | ||
| // Always load Box.Preview library assets | ||
| // Always load preview library assets (npm module or CDN script) |
There was a problem hiding this comment.
[question] This mean when shouldUseNpmPreview is true, you will always have to rely on the BUIE installed box-content-preview package version since that will be the npm version and never able to leverage the older CDN version. Is this intended?
| // box-content-preview (devDependency) imports its box-ui-elements peer via | ||
| // es/ paths; inside this repo those resolve to the src/ they are built from. | ||
| 'box-ui-elements/es': path.resolve('src'), |
There was a problem hiding this comment.
[question] was the storybook breaking without this change?
| // box-content-preview (devDependency) imports its box-ui-elements peer via | ||
| // es/ paths; inside this repo those resolve to the src/ they are built from. | ||
| 'box-ui-elements/es': path.resolve('src'), |
There was a problem hiding this comment.
[question] Just curious, how were you testing that the bundle size is not doubled and installed both version of BUIE
| are-we-there-yet@^2.0.0: | ||
| version "2.0.0" | ||
| resolved "https://registry.yarnpkg.com/are-we-there-yet/-/are-we-there-yet-2.0.0.tgz#372e0e7bd279d8e94c653aaa1f67200884bf3e1c" | ||
| integrity sha512-Ci/qENmwHnsYo9xKIcUJN5LeDKdJ6R1Z1j9V/J5wyq8nh/mYPEpIKJbBZXtZjG04HiK7zV/p6Vs9952MrMeUIw== | ||
| dependencies: | ||
| delegates "^1.0.0" | ||
| readable-stream "^3.6.0" |
There was a problem hiding this comment.
Just want to call out that this package is no longer supported. Meaning in the future if there's any security vulnerability, we cannot do much to resolve it. Do you have any plan to deprecate this package sometime soon?
reneshen0328
left a comment
reneshen0328
left a comment
There was a problem hiding this comment.
Left some questions but nothing is blocking this PR
| resolved "https://registry.yarnpkg.com/@leichtgewicht/ip-codec/-/ip-codec-2.0.5.tgz#4fc56c15c580b9adb7dc3c333a134e540b44bfb1" | ||
| integrity sha512-Vo+PSpZG2/fmgmiNzYK9qWRh8h/CHrwD0mo1h1DzL4yzHNSfWYujGTYsWGreD000gcgmZ7K4Ys6Tx9TxtsKdDw== | ||
|
|
||
| "@mapbox/node-pre-gyp@^1.0.0": |
There was a problem hiding this comment.
Just want to call out that this package is no longer supported. Meaning in the future if there's any security vulnerability, we cannot do much to resolve it. Do you have any plan to deprecate this package sometime soon?
| axios@^0.24.0: | ||
| version "0.24.0" | ||
| resolved "https://registry.yarnpkg.com/axios/-/axios-0.24.0.tgz#804e6fa1e4b9c5288501dd9dff56a7a0940d20d6" | ||
| integrity sha512-Q6cWsys88HoPgAaFAVUb0WpPk0O8iTeisR9IMqy9G8AbO4NlpVknrnQS03zzF9PGAWgO3cgletO3VjV/P7VztA== | ||
| dependencies: | ||
| follow-redirects "^1.14.4" |
There was a problem hiding this comment.
This should be deduplicated with the existing version 0.32.0 installed.
| resolved "https://registry.yarnpkg.com/functions-have-names/-/functions-have-names-1.2.3.tgz#0404fe4ee2ba2f607f0e0ec3c80bae994133b834" | ||
| integrity sha512-xckBUXyTIqT97tq2x2AMb+g163b5JFysYk0x4qxNFwbfQkmNZoiRHb6sPzI9/QV33WeuvVYBUIiD4NzNIyqaRQ== | ||
|
|
||
| gauge@^3.0.0: |
There was a problem hiding this comment.
Just want to call out that this package is no longer supported. Meaning in the future if there's any security vulnerability, we cannot do much to resolve it. Do you have any plan to deprecate this package sometime soon?
| which "^5.0.0" | ||
| write-file-atomic "^6.0.0" | ||
|
|
||
| npmlog@^5.0.1: |
There was a problem hiding this comment.
Just want to call out that this package is no longer supported. Meaning in the future if there's any security vulnerability, we cannot do much to resolve it. Do you have any plan to deprecate this package sometime soon?
Dismiss until box-content-preview fixes all critical and high security vuln
3 participants
Summary
Adds an opt-in path for loading the Preview library from the
box-content-previewnpm package instead of injecting CDN script/stylesheet tags, gated on theuseNpmBoxContentPreviewfeature flag.preview.jsandpreview.cssare loaded from the CDN andglobal.Box.Previewis used.box-content-previewand itsstyles.cssare loaded via dynamic import, and the namedPreviewexport is used. Two option groups are forwarded toPreview.show()that the npm build cannot self-derive from a CDN script tag:pdfjs: { workerSrc }from the new optionalpdfjsWorkerSrcprop, so document viewers can boot the pdfjs worker the consumer bundledlocation: { staticBaseURI, version, locale }derived from the existingstaticHost/staticPath/previewLibraryVersion/languageprops, so viewers that build asset URLs fromstaticBaseURIkeep workingRequires
box-content-preview>= 3.62.0 (box/box-content-preview#1704), declared as an optional peer dependency; consumers on the CDN path do not need to install it. Consumers on the npm path must also bundle a pdfjs-dist worker matching the version pinned by their installedbox-content-preview.Details
Previewexport) surfaces through the standard error path: loading state ends, error state renders, and the hostonErrorcallback fires with the underlying cause.box-content-preview@3.62.1is added as a devDependency so Flow, jest, and the webpack builds resolve the real package. Two build-config accommodations exist only because this repo is itself thebox-ui-elementspeer that box-content-preview imports: abox-ui-elements/es->src/alias in the webpack/Storybook configs (the publishedes/output does not exist in the working tree), and a babel-loader exclude for box-content-preview's pre-bundled dist (it ships modern syntax and needs no transpilation). Neither affects consumers.Testing
show()option shapes includingstaticHosttrailing-slash handling),pdfjsomitted whenpdfjsWorkerSrcis absent, and both import-failure paths.yarn flow check,eslint, and the full ContentPreview test suite (158 tests) pass. The production webpack build and Storybook build compile with the npm package resolved.