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
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().
Walkthrough
ChangesNPM preview loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ContentPreview
participant box_content_preview
participant Preview
ContentPreview->>ContentPreview: Check useNpmBoxContentPreview
ContentPreview->>box_content_preview: Dynamically import package and styles
box_content_preview-->>ContentPreview: Return Preview export
ContentPreview->>Preview: Call preview.show with location and pdfjs options
Possibly related PRs
Suggested labels: Suggested reviewers: 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 17470 characters] ... remove the syntax.; Line 1697: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1704: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1723: Illegal return statement outside of a function; Line 1736: 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
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
New Features
Bug Fixes