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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)src/elements/content-preview/ContentPreview.jsFile contains syntax errors that prevent linting: Line 21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 36: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 65: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 66: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 67: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 68: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 69: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 70: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 71: ' ... [truncated 17471 characters] ... remove the syntax.; Line 1699: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1706: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1725: Illegal return statement outside of a function; Line 1738: expected 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
jackiejou
force-pushed
the
npm-content-preview
branch
from
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
| getNpmPreviewLocation(): { locale: string, staticBaseURI: string, version: string } { | ||
| const { language, previewLibraryVersion, staticHost, staticPath } = this.props; | ||
| const trailingSlash = staticHost.endsWith('/') ? '' : '/'; | ||
| return { | ||
| locale: language, | ||
| staticBaseURI: `${staticHost}${trailingSlash}${staticPath}/`, | ||
| version: previewLibraryVersion, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how staticPath/staticHost are combined elsewhere in this file (e.g. CDN script/style URL builders)
# to confirm the slash convention getNpmPreviewLocation should match.
rg -n -B2 -A5 'staticPath' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 1538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the defaults, prop types, and nearby URL-building logic for staticPath.
sed -n '1,220p' src/elements/content-preview/ContentPreview.js
printf '\n---\n'
sed -n '330,410p' src/elements/content-preview/ContentPreview.js
printf '\n---\n'
sed -n '680,710p' src/elements/content-preview/ContentPreview.js
printf '\n---\n'
rg -n "DEFAULT_PATH_STATIC_PREVIEW|DEFAULT_HOSTNAME_STATIC|staticPath" src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 12492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the default static path value and any other callers that construct it.
rg -n "DEFAULT_PATH_STATIC_PREVIEW|DEFAULT_HOSTNAME_STATIC|staticPath:" srcRepository: box/box-ui-elements
Length of output: 1242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/elements/content-preview/ContentPreview.js | nl -ba | sed -n '1,120p'
printf '\n---\n'
rg -n "DEFAULT_PATH_STATIC_PREVIEW|DEFAULT_HOSTNAME_STATIC" src/elements/content-preview/ContentPreview.js srcRepository: box/box-ui-elements
Length of output: 197
Normalize staticPath before building preview URLs getNpmPreviewLocation() and getBasePath() concatenate staticPath directly, so a leading or trailing slash will produce malformed asset URLs. Normalize it at the join point or enforce a slash-free prop contract.
🧰 Tools
🪛 Biome (2.5.3)
[error] 544-544: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 544-544: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 547-551: Illegal return statement outside of a function
(parse)
🤖 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 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.
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().
jackiejou
force-pushed
the
npm-content-preview
branch
from
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
| 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 (error) { | ||
| this.onNpmPreviewLoadError( | ||
| `Failed to load the box-content-preview module: ${error?.message || String(error)}`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'isUnmounted|componentWillUnmount|_isMounted' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=src/elements/content-preview/ContentPreview.js
echo '--- componentWillUnmount and nearby methods ---'
sed -n '390,560p' "$FILE" | cat -n
echo
echo '--- search for async state guards and load helpers ---'
rg -n 'onNpmPreviewLoadError|loadPreview|loadScript|loadStylesheet|setState|componentWillUnmount|_isMounted|isUnmounted' "$FILE"Repository: box/box-ui-elements
Length of output: 9180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=src/elements/content-preview/ContentPreview.js
echo '--- onNpmPreviewLoadError and loadPreview ---'
sed -n '556,1065p' "$FILE" | cat -n
echo
echo '--- any mount/destroy flags in the whole file ---'
rg -n 'mounted|destroyed|isUnmounted|_isMounted|componentWillUnmount|setState\(' "$FILE"Repository: box/box-ui-elements
Length of output: 22304
Guard loadNpmPreview against unmounts src/elements/content-preview/ContentPreview.js:126-150
componentWillUnmount() only destroys the preview/API; it doesn’t stop the pending dynamic import from later calling onNpmPreviewLoadError() or loadPreview(). Add a mounted/destroyed guard before any setState in this path.
🧰 Tools
🪛 Biome (2.5.3)
[error] 515-515: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
🤖 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 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.
2 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.flow/BoxContentPreviewStub.js.flow+ a.flowconfigname mapper let Flow resolve the optional peer without it being installed. Jest tests mock the package as a virtual module for the same reason.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.Summary by CodeRabbit