feat(preview): support loading the Preview library from npm - #4695
feat(preview): support loading the Preview library from npm#4695jackiejou wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. Walkthrough
ChangesNPM preview integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds an opt-in npm loading path while preserving the default CDN behavior, with reported unit, Flow, lint, webpack, and Storybook checks passing; no actionable merge-blocking risk remains beyond normal review. 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
Suggested reviewers: 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
dfed52c to
c5a2831
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
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
| 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?
reneshen0328
left a comment
reneshen0328
left a comment
There was a problem hiding this comment.
Left some questions but nothing is blocking this PR
Dismiss until box-content-preview fixes all critical and high security vuln
c5a2831 to
4aa4b2a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Hosts opt in with useNpmBoxContentPreview and pass loadPreviewModule imported from loadBoxContentPreview.js. The optional peer stays out of the ContentPreview module graph. Preview assets default to 3.79.0.
c45e6b6 to
041e395
Compare
Visual stories load preview.js from the default library version. The extra delay lets the CDN script and pdf.js worker finish before capture.
A function token stays the write resolver for box-annotations. A string token is omitted so annotations receive the resolved read token.
Preview visual stories use the global 500ms capture delay.
reneshen0328
left a comment
reneshen0328
left a comment
There was a problem hiding this comment.
Love it, thanks for addressing all the dependencies vuln. We've discussed this already, but I just want to emphasize this once more so we keep an eye out on these dependencies: npmlog / gauge / are-we-there-yet are all deprecated by npm which could potentially be a maintanance issue.
3 participants
Summary
Adds an opt-in path that loads Box Content Preview from the
box-content-previewnpm package, gated onfeatures.useNpmBoxContentPreview. Hosts that want the npm path pass a loader:preview.js/preview.cssandglobal.Box.Preview, same as today.loadPreviewModuleloads the npmPreviewexport and its CSS.ContentPreviewdoes not importbox-content-previewitself, so CDN-only apps keep a build that does not resolve that package.loadPreviewModuleis required when the flag is on. A missing loader, a failed import, or a module withoutPreviewends loading, renders the error state, and callsonError.show()options includelocation(staticBaseURI,version,locale) from the existing static/language props, and optionalpdfjs.workerSrcfrompdfjsWorkerSrc.tokenis forwarded asannotatorTokenso box-annotations keeps the write resolver. A stringtokenis omitted so annotations use the resolved read token.DEFAULT_PREVIEW_VERSIONis3.79.0.box-content-previewis an optional peer (^3.79.0) and adevDependency(3.79.0). CDN-only consumers do not install it. npm consumers install a matching peer and bundle a pdfjs worker for that version.Why
Hosts can load Preview from their own bundle. The loader stays outside
ContentPreviewso the optional peer is not pulled into everyes/consumer (including Content Explorer via PreviewDialog).Test plan
global.Box.Previewis used.loadPreviewModule: no CDN tags; npmPreviewis used;locationandpdfjs.workerSrcmatch the props (includingstaticHostwith a trailing slash).loadPreviewModule, or a loader that rejects / has noPreviewexport: error state andonError.token:annotatorTokenis omitted. Functiontoken: the same function is forwarded.box-content-previewinstalled still builds.ContentPreview.test.jsfor the npm and CDN paths.