feat(content-preview): support externally-managed preview instance#4693
feat(content-preview): support externally-managed preview instance#4693zhirongwang wants to merge 1 commit into
Conversation
Walkthrough
ChangesContentPreview instance reuse
Estimated code review effort: 2 (Simple) | ~10 minutes 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 18644 characters] ... nly feature. Convert your file to a TypeScript file or remove the syntax.; Line 1607: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1619: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1626: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 1658: 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 |
zhirongwang
force-pushed
the
feat/content-preview-external-preview-instance
branch
from
913cd74 to
3d58d73
Compare
|
zhirongwang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 1025: Update ContentPreview’s componentDidUpdate lifecycle to detect
changes to props.previewInstance, destroy the previous preview instance and its
listeners, then assign the new instance or create the default Preview and rebind
the required listeners. Ensure clearing or swapping the prop immediately leaves
the component wired to the current preview instance, while preserving the
existing behavior when the instance is unchanged.
- Around line 436-444: Update the previewInstance cleanup branch in
ContentPreview to remove every listener registered by ContentPreview, including
thumbnailsOpen, thumbnailsClose, annotator_create, and advanced_insights_report,
in addition to the existing handlers. Keep the shared external preview instance
intact and ensure no ContentPreview listeners remain attached after cleanup.
🪄 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: 4e463323-196a-4a2e-bbf0-667d09c2b119
📒 Files selected for processing (1)
src/elements/content-preview/ContentPreview.js
| if (this.props.previewInstance) { | ||
| this.preview.removeListener('load', this.onPreviewLoad); | ||
| this.preview.removeListener('preload', this.endLoadingSession); | ||
| this.preview.removeListener('preview_error', this.onPreviewError); | ||
| this.preview.removeListener('preview_metric', this.onPreviewMetric); | ||
| } else { | ||
| this.preview.destroy(); | ||
| this.preview.removeAllListeners(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all listener attachments on this.preview in ContentPreview.js
rg -n 'this\.preview\.(addListener|on|addListener)\(' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant sections around listener registration and teardown.
sed -n '1000,1060p' src/elements/content-preview/ContentPreview.js
printf '\n---\n'
sed -n '420,460p' src/elements/content-preview/ContentPreview.js
printf '\n---\n'
rg -n 'componentDidUpdate|previewInstance|destroyPreview|removeAllListeners|removeListener' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 4732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect update handling around previewInstance and destroyPreview calls.
sed -n '520,560p' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 2070
Detach all listeners when previewInstance is supplied
src/elements/content-preview/ContentPreview.js:436-444 only removes 4 handlers for external instances, but ContentPreview also registers thumbnailsOpen, thumbnailsClose, annotator_create, and advanced_insights_report. Those stay attached to the shared preview instance after unmount/reload.
🤖 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 436 - 444,
Update the previewInstance cleanup branch in ContentPreview to remove every
listener registered by ContentPreview, including thumbnailsOpen,
thumbnailsClose, annotator_create, and advanced_insights_report, in addition to
the existing handlers. Keep the shared external preview instance intact and
ensure no ContentPreview listeners remain attached after cleanup.
There was a problem hiding this comment.
+1, why do we not use removeAllListeners?
There was a problem hiding this comment.
Good question, @JChan106 — since previewInstance is externally owned by the host application when it's supplied, removeAllListeners() would strip out any listeners the host app itself registered on that shared instance (and any listeners other consumers might have attached), not just the ones ContentPreview added. That would be a destructive side effect on an object the component doesn't own.
So the intent is correct: only remove the specific listeners ContentPreview itself attached, leaving everything else on the shared instance intact for the host to manage.
That said, the current cleanup only detaches 4 of the listeners registered in loadPreview — it's missing thumbnailsOpen, thumbnailsClose, annotator_create (when showAnnotationsControls is set), and advanced_insights_report (when advancedContentInsights is set). To avoid leaking those onto the shared instance, it'd be worth removing all of them symmetrically in destroyPreview, e.g.:
if (this.props.previewInstance) {
this.preview.removeListener('load', this.onPreviewLoad);
this.preview.removeListener('preload', this.endLoadingSession);
this.preview.removeListener('preview_error', this.onPreviewError);
this.preview.removeListener('preview_metric', this.onPreviewMetric);
this.preview.removeListener('thumbnailsOpen', this.onThumbnailsOpen);
this.preview.removeListener('thumbnailsClose', this.onThumbnailsClose);
this.preview.removeListener('annotator_create', onAnnotator);
this.preview.removeListener('advanced_insights_report', onContentInsightsEventReport);
} else {
...
}Note that the thumbnailsOpen/thumbnailsClose listeners and annotator_create/advanced_insights_report are currently registered as inline anonymous functions or closures in loadPreview, so they'd need to be hoisted to stable instance methods/references to be removable later.
| }; | ||
| const { Preview } = global.Box; | ||
| this.preview = new Preview(); | ||
| this.preview = this.props.previewInstance || new Preview(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check componentDidUpdate and related lifecycle for previewInstance handling
rg -n 'componentDidUpdate|previewInstance' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/elements/content-preview/ContentPreview.js --view expanded
sed -n '480,580p' src/elements/content-preview/ContentPreview.js
sed -n '990,1060p' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 7054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '580,760p' src/elements/content-preview/ContentPreview.js
rg -n 'previewInstance|destroyPreview|componentWillUnmount|removeListener|destroy\(' src/elements/content-preview/ContentPreview.jsRepository: box/box-ui-elements
Length of output: 6664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '420,450p' src/elements/content-preview/ContentPreview.js | cat -nRepository: box/box-ui-elements
Length of output: 1393
Handle previewInstance swaps in componentDidUpdate. destroyPreview() keys cleanup off the current prop, but there’s no branch that tears down the old instance and rebinds listeners when previewInstance changes. If the host swaps or clears the instance, the old listeners can stick around and the new instance won’t be wired up until the next load.
🤖 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` at line 1025, Update
ContentPreview’s componentDidUpdate lifecycle to detect changes to
props.previewInstance, destroy the previous preview instance and its listeners,
then assign the new instance or create the default Preview and rebind the
required listeners. Ensure clearing or swapping the prop immediately leaves the
component wired to the current preview instance, while preserving the existing
behavior when the instance is unchanged.
3 participants
Summary
previewInstanceprop toContentPreviewthat lets host applications pass in an existingBox.Previewinstance instead of having the component construct its own.ContentPreviewdetaches only the listeners it attached (load,preload,preview_error,preview_metric) rather than callingdestroy()andremoveAllListeners(), so the host application retains ownership of the instance lifecycle.ContentPreviewcreates and destroys its own instance as before.Test Plan
yarn flow checkpasses (0 errors)previewInstanceis not providedPreviewinstance is reused and not destroyed on unmountMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes