Skip to content

feat(content-preview): support externally-managed preview instance #4693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zhirongwang wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(content-preview): support externally-managed preview instance #4693

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/elements/content-preview/ContentPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,13 @@ type Props = {
previewExperiences?: {
[name: string]: TargetingApi,
},
/**
* Optional externally-managed Box.Preview instance. When provided,
* ContentPreview reuses it instead of constructing its own, and on
* cleanup it only detaches its listeners rather than destroying the
* instance — the host application retains ownership of its lifecycle.
*/
previewInstance?: Object,
previewLibraryVersion: string,
previewMode?: 'default' | 'shared_file' | 'shared_folder' | 'editable_shared_file' | 'inline_feed',
requestInterceptor?: Function,
Expand Down Expand Up @@ -426,8 +433,15 @@ class ContentPreview extends React.PureComponent<Props, State> {
this.clearLoadingIndicatorDelayTimeout();
const { onPreviewDestroy } = this.props;
if (this.preview) {
this.preview.destroy();
this.preview.removeAllListeners();
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();
}
Comment on lines +436 to +444

@coderabbitai coderabbitai Bot Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.js

Repository: 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.js

Repository: 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.js

Repository: 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.

@JChan106 JChan106 Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, why do we not use removeAllListeners?

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.preview = undefined;

onPreviewDestroy(shouldReset);
Expand Down Expand Up @@ -1008,7 +1022,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
useHotkeys: false,
};
const { Preview } = global.Box;
this.preview = new Preview();
this.preview = this.props.previewInstance || new Preview();

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.js

Repository: 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.js

Repository: 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.js

Repository: 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 -n

Repository: 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.

this.preview.addListener('load', this.onPreviewLoad);
this.preview.addListener('preload', this.endLoadingSession);

Expand Down
Loading