feat(ContentPreview): add customPreviewContent prop for extensibility #4439
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add customPreviewContent prop to ContentPreview component to allow rendering custom preview implementations while preserving all ContentPreview features (sidebar, navigation, header, etc). Changes: - Add customPreviewContent prop to Props type - Skip Box.Preview library loading when custom content provided - Skip keyboard hotkey handling when custom content present - Render custom component inside .bcpr-content div - Set loading state false before onLoad callback to prevent blocking - Add comprehensive test coverage for custom preview functionality This enables use cases like inline markdown editing where the preview area is replaced with a custom renderer while maintaining the full ContentPreview layout and functionality. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
WalkthroughContentPreview gained an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentPreview
participant CustomPreview
participant BoxPreviewLib
participant Logger
User->>ContentPreview: mount with props (fileId, token, customPreviewContent?)
ContentPreview->>ContentPreview: validate customPreviewContent
alt valid customPreviewContent
ContentPreview->>CustomPreview: render with (fileId, token, apiHost, file, onError, onLoad)
CustomPreview->>ContentPreview: onLoad / onError callbacks
ContentPreview->>Logger: log errors from onLoad/onError (try/catch)
else no customPreviewContent
ContentPreview->>BoxPreviewLib: load styles/scripts
BoxPreviewLib->>ContentPreview: initialize preview
ContentPreview->>Logger: log init/load errors
end
User->>ContentPreview: keyboard events
alt customPreviewContent present
ContentPreview->>CustomPreview: delegate keyboard handling
else
ContentPreview->>ContentPreview: handle internal hotkeys
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/elements/content-preview/__tests__/ContentPreview.test.js`:
- Around line 1736-1738: The test currently compares
customPreviewInstance.prop('onError') to instance.onError which is undefined;
update the assertion to validate the real error handler is wired by comparing
customPreviewInstance.prop('onError') to the actual instance method or prop (for
example instance.onPreviewError if the class defines onPreviewError, or
instance.props.onError if the handler comes from props) while keeping the
existing onLoad assertion (customPreviewInstance.prop('onLoad')
toBe(instance.onPreviewLoad) ) so the spec verifies the error callback is truly
connected; locate symbols customPreviewInstance, instance, onError,
onPreviewError, onPreviewLoad and adjust the expectation accordingly.
- Around line 1741-1757: Split the current conditional test into two
deterministic tests: (1) "should not render Measure when file is not loaded" —
use getWrapper(props) without setting file state and assert
measureComponent.exists() is false unconditionally; (2) "should not render
custom preview content when file is loaded" — set the component to a loaded-file
state (via wrapper.setState or appropriate props), assert
measureComponent.exists() is true, grab the renderProp children from Measure and
shallow-render it, then assert that customPreviewContent (CustomPreview) is not
present; update test names accordingly and remove the if/else conditional so
each case has a single clear assertion path.
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 754-762: The onLoad call currently swallows all errors and uses
console.warn; change it to use the component logger (this.logger.warn) and only
swallow errors for the custom preview path—use the same custom-preview detection
this component already uses (e.g., the renderCustomPreview branch or
props.customPreview flag); for non-custom previews, catch, log via
this.logger.warn and re-throw so existing Box.Preview consumers still see
errors. Ensure you update the block around onLoad(loadData) in the
ContentPreview class to implement this conditional try-catch behavior and
replace console.warn with this.logger.warn.
- Around line 1470-1478: The JSX passes this.onError (which doesn't exist) to
CustomPreview causing undefined to be sent; replace this.onError with the class
method this.onPreviewError (or, if you intend to forward the external prop, use
this.props.onError) so CustomPreview receives a valid callback—update the
CustomPreview prop from onError={this.onError} to onError={this.onPreviewError}
(or onError={this.props.onError}) and ensure onPreviewError is correctly bound
or defined as an arrow method.
Comment on lines
+1741
to
+1757
| test('should not render custom preview content when file is not loaded', () => { | ||
| const wrapper = getWrapper(props); | ||
| // Don't set file state - file should be undefined | ||
|
|
||
| // Check if Measure component exists (it may not render without file) | ||
| const measureComponent = wrapper.find('Measure'); | ||
| if (measureComponent.exists()) { | ||
| const renderProp = measureComponent.prop('children'); | ||
| const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>); | ||
|
|
||
| const CustomPreview = customPreviewContent; | ||
| expect(measureContent.find(CustomPreview).exists()).toBe(false); | ||
| } else { | ||
| // If Measure doesn't exist, custom preview definitely isn't rendered | ||
| expect(measureComponent.exists()).toBe(false); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid conditional logic in tests — it weakens the assertion.
The if/else on measureComponent.exists() means this test can pass without actually asserting custom preview absence. Split into two deterministic tests or assert the expected state unconditionally.
♻️ Suggested: assert deterministically
test('should not render custom preview content when file is not loaded', () => {
const wrapper = getWrapper(props);
// Don't set file state - file should be undefined
- // Check if Measure component exists (it may not render without file)
- const measureComponent = wrapper.find('Measure');
- if (measureComponent.exists()) {
- const renderProp = measureComponent.prop('children');
- const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>);
-
- const CustomPreview = customPreviewContent;
- expect(measureContent.find(CustomPreview).exists()).toBe(false);
- } else {
- // If Measure doesn't exist, custom preview definitely isn't rendered
- expect(measureComponent.exists()).toBe(false);
- }
+ // Without file state set, the Measure component should not render
+ // (it's guarded by `{file && <Measure ...>}` in the source)
+ const measureComponent = wrapper.find('Measure');
+ expect(measureComponent.exists()).toBe(false);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('should not render custom preview content when file is not loaded', () => { | |
| const wrapper = getWrapper(props); | |
| // Don't set file state - file should be undefined | |
| // Check if Measure component exists (it may not render without file) | |
| const measureComponent = wrapper.find('Measure'); | |
| if (measureComponent.exists()) { | |
| const renderProp = measureComponent.prop('children'); | |
| const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>); | |
| const CustomPreview = customPreviewContent; | |
| expect(measureContent.find(CustomPreview).exists()).toBe(false); | |
| } else { | |
| // If Measure doesn't exist, custom preview definitely isn't rendered | |
| expect(measureComponent.exists()).toBe(false); | |
| } | |
| }); | |
| test('should not render custom preview content when file is not loaded', () => { | |
| const wrapper = getWrapper(props); | |
| // Don't set file state - file should be undefined | |
| // Without file state set, the Measure component should not render | |
| // (it's guarded by `{file && <Measure ...>}` in the source) | |
| const measureComponent = wrapper.find('Measure'); | |
| expect(measureComponent.exists()).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
In `@src/elements/content-preview/__tests__/ContentPreview.test.js` around lines
1741 - 1757, Split the current conditional test into two deterministic tests:
(1) "should not render Measure when file is not loaded" — use getWrapper(props)
without setting file state and assert measureComponent.exists() is false
unconditionally; (2) "should not render custom preview content when file is
loaded" — set the component to a loaded-file state (via wrapper.setState or
appropriate props), assert measureComponent.exists() is true, grab the
renderProp children from Measure and shallow-render it, then assert that
customPreviewContent (CustomPreview) is not present; update test names
accordingly and remove the if/else conditional so each case has a single clear
assertion path.
Comment on lines
754
to
762
| // Set loading to false before calling onLoad, so errors in metrics don't prevent UI update | ||
| this.setState({ isLoading: false }); | ||
|
|
||
| try { | ||
| onLoad(loadData); | ||
| } catch (error) { | ||
| // Log but don't throw - metrics errors shouldn't break preview | ||
| console.warn('ContentPreview: onLoad callback threw error:', error); | ||
| } |
Contributor
There was a problem hiding this comment.
try-catch silently swallows onLoad errors for all callers, not just custom preview.
This behavioral change affects the existing Box.Preview path as well. Silently catching and only console.warn-ing errors from onLoad callbacks could mask bugs in production. Consider scoping this guard to the custom preview path only, or at minimum re-throwing after logging so existing consumers aren't silently broken.
Also, console.warn should use the component's logger for consistency with the rest of the file.
♻️ Suggested: scope try-catch to custom preview only
// Set loading to false before calling onLoad, so errors in metrics don't prevent UI update
this.setState({ isLoading: false });
- try {
- onLoad(loadData);
- } catch (error) {
- // Log but don't throw - metrics errors shouldn't break preview
- console.warn('ContentPreview: onLoad callback threw error:', error);
- }
+ if (this.props.customPreviewContent) {
+ try {
+ onLoad(loadData);
+ } catch (error) {
+ // Log but don't throw - custom content metrics errors shouldn't break preview
+ console.warn('ContentPreview: onLoad callback threw error:', error);
+ }
+ } else {
+ onLoad(loadData);
+ }🤖 Prompt for AI Agents
In `@src/elements/content-preview/ContentPreview.js` around lines 754 - 762, The
onLoad call currently swallows all errors and uses console.warn; change it to
use the component logger (this.logger.warn) and only swallow errors for the
custom preview path—use the same custom-preview detection this component already
uses (e.g., the renderCustomPreview branch or props.customPreview flag); for
non-custom previews, catch, log via this.logger.warn and re-throw so existing
Box.Preview consumers still see errors. Ensure you update the block around
onLoad(loadData) in the ContentPreview class to implement this conditional
try-catch behavior and replace console.warn with this.logger.warn.
…ntent
Critical fixes:
- Fix bug: change onError={this.onError} to onError={this.onPreviewError}
- Add specific callback types (CustomPreviewOnError, CustomPreviewOnLoad)
- Replace console.warn with logger.logError in onPreviewLoad
- Add ErrorBoundary wrapper around CustomPreview component
- Add prop validation before rendering CustomPreview
High priority fixes:
- Add runtime type validation for customPreviewContent
- Enhance JSDoc with comprehensive documentation
- Add validation for required props (fileId, token, apiHost)
Additional improvements:
- Improved error handling and logging throughout
- Added comprehensive test coverage for error scenarios
- Better comments explaining tradeoffs and behavior
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix test to expect onPreviewError instead of onError - Add logError to logger mocks in tests - Ensure all logger mocks have required methods Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/elements/content-preview/__tests__/ContentPreview.test.js`:
- Around line 1932-1946: The test "should wrap CustomPreview in ErrorBoundary"
currently only asserts ErrorBoundary when customPreviewContent exists due to the
conditional if (measureContent.find(customPreviewContent).exists()); remove that
conditional and make the assertion unconditional: after rendering measureContent
from the Measure render prop (using wrapper, getWrapper, file, Measure, and
measureRef), directly assert that measureContent.find('ErrorBoundary').exists()
is true so the test always verifies ErrorBoundary wraps CustomPreview.
- Around line 1948-1971: The test currently guards the key assertion with if
(customPreviewInstance.exists()), which can skip the assertion; remove that
conditional and make two unconditional expectations: first assert that
customPreviewInstance.exists() is true (to guarantee CustomPreview rendered
after wrapper.setState({ file })), then assert
customPreviewInstance.prop('onError') === instance.onPreviewError; update the
test around the Measure render-prop extraction (measureContent and
customPreviewInstance) to perform these two expects without an if so the wiring
is always verified.
- Around line 1908-1930: The test is asserting a trivially true condition by
checking that mockLogger.logError is defined instead of verifying it was called;
update the test that uses getWrapper, CustomPreview, wrapper.setState and
instance.onPreviewError to actually trigger the missing-props validation path
(for example render with token: undefined and set currentFileId so validation
runs) and assert mockLogger.logError was called (e.g.,
expect(mockLogger.logError).toHaveBeenCalled()/toHaveBeenCalledWith(...)) rather
than toBeDefined(); ensure the wrapper.update()/instance.onPreviewError
invocation triggers the validation before the assertion.
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 1553-1558: The spread of validationError loses non-enumerable
properties like message/stack, so when calling onPreviewError you should
preserve those by creating or augmenting an Error instance instead of
{...validationError, code: ...}; for example, build a new Error using
validationError.message (or clone by assigning message/name/stack onto a new
Error) and set err.code = 'CUSTOM_PREVIEW_INVALID_PROPS' before passing it to
this.onPreviewError so handlers receive a real Error with message and stack
intact (refer to this.onPreviewError and validationError in ContentPreview.js).
- Around line 416-428: The current early return in the customPreviewContent
validation block prevents fetchFile and focusPreview from running and never sets
the component error state, so PreviewMask cannot show the error; modify the
validation branch in ContentPreview (the block referencing customPreviewContent,
logger.logError and onError) to set the component's error state (e.g., via
this.setState({ error: error })) before returning so PreviewMask will render the
error UI, or alternatively allow execution to continue to the normal flow (so
fetchFile and focusPreview still run) while ensuring onError/logger still record
the issue; ensure ORIGIN_CONTENT_PREVIEW is passed unchanged and that no
duplicate error reports occur when choosing the continue-vs-return approach.
- Around line 1565-1577: The current onError handler creates a new Error with
elementsError.message which discards the original stack; instead pass the
original error through to logger.logError (or wrap it as the cause) so the
original stack and metadata are preserved—update the ErrorBoundary onError
callback (the function that receives elementsError) to log elementsError
directly to logger.logError (keeping the same context object with
ORIGIN_CONTENT_PREVIEW, currentFileId, file.name and elementsError.code) or, if
you must create a new Error, include elementsError as the cause so the original
stack is retained.
🧹 Nitpick comments (1)
src/elements/content-preview/ContentPreview.js (1)
1533-1589: IIFE inside JSX is hard to read — extract to a method or simple conditional.The inline IIFE
(() => { ... })()adds cognitive complexity. A helper method (e.g.,renderCustomPreview()) or early-return guard with a plain conditional would be clearer and easier to test in isolation.Sketch
- {CustomPreview && - (() => { - // ... 30+ lines of validation and rendering - })()} + {CustomPreview && this.renderCustomPreview(CustomPreview, currentFileId, token, apiHost, file, logger)}Then define:
renderCustomPreview(CustomPreview, currentFileId, token, apiHost, file, logger) { if (!currentFileId || !token || !apiHost) { // validation + logging + return null } return ( <ErrorBoundary ...> <CustomPreview ... /> </ErrorBoundary> ); }
Comment on lines
+1908
to
+1930
| test('should validate required props before rendering CustomPreview', () => { | ||
| const mockLogger = { logError: jest.fn(), onReadyMetric: jest.fn() }; | ||
| const CustomPreview = jest.fn(() => <div>Custom</div>); | ||
|
|
||
| // Create wrapper without setting currentFileId in state | ||
| const wrapper = getWrapper({ | ||
| ...props, | ||
| customPreviewContent: CustomPreview, | ||
| logger: mockLogger, | ||
| }); | ||
| wrapper.setState({ file }); // Set file but currentFileId will be undefined in some scenario | ||
|
|
||
| // Get instance and manually call onPreviewError to set up spy | ||
| const instance = wrapper.instance(); | ||
| instance.onPreviewError = jest.fn(); | ||
|
|
||
| // Force a re-render to trigger validation | ||
| wrapper.update(); | ||
|
|
||
| // In a real scenario where props are missing, logError would be called | ||
| // This test documents the expected behavior | ||
| expect(mockLogger.logError).toBeDefined(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test asserts a trivially true condition — mockLogger.logError is always defined.
expect(mockLogger.logError).toBeDefined() passes regardless of whether validation actually fired, because logError is jest.fn(). This test doesn't exercise the missing-props validation path. Either remove it or restructure to actually trigger the scenario (e.g., render with token: undefined and currentFileId set, then check logError was called).
Suggested fix
test('should validate required props before rendering CustomPreview', () => {
const mockLogger = { logError: jest.fn(), onReadyMetric: jest.fn() };
const CustomPreview = jest.fn(() => <div>Custom</div>);
const wrapper = getWrapper({
...props,
customPreviewContent: CustomPreview,
logger: mockLogger,
+ token: undefined, // Missing required prop to trigger validation
});
- wrapper.setState({ file });
-
- const instance = wrapper.instance();
- instance.onPreviewError = jest.fn();
-
- wrapper.update();
+ wrapper.setState({ file, currentFileId: file.id });
- expect(mockLogger.logError).toBeDefined();
+ // Trigger the render prop to exercise validation
+ const measureComponent = wrapper.find('Measure');
+ const renderProp = measureComponent.prop('children');
+ shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>);
+
+ expect(mockLogger.logError).toHaveBeenCalledWith(
+ expect.any(Error),
+ 'CUSTOM_PREVIEW_INVALID_PROPS',
+ expect.objectContaining({ missingProps: expect.arrayContaining(['token']) }),
+ );
});🤖 Prompt for AI Agents
In `@src/elements/content-preview/__tests__/ContentPreview.test.js` around lines
1908 - 1930, The test is asserting a trivially true condition by checking that
mockLogger.logError is defined instead of verifying it was called; update the
test that uses getWrapper, CustomPreview, wrapper.setState and
instance.onPreviewError to actually trigger the missing-props validation path
(for example render with token: undefined and set currentFileId so validation
runs) and assert mockLogger.logError was called (e.g.,
expect(mockLogger.logError).toHaveBeenCalled()/toHaveBeenCalledWith(...)) rather
than toBeDefined(); ensure the wrapper.update()/instance.onPreviewError
invocation triggers the validation before the assertion.
Comment on lines
+1932
to
+1946
| test('should wrap CustomPreview in ErrorBoundary', () => { | ||
| const wrapper = getWrapper(props); | ||
| wrapper.setState({ file }); | ||
|
|
||
| // Find the Measure component and extract its render prop | ||
| const measureComponent = wrapper.find('Measure'); | ||
| const renderProp = measureComponent.prop('children'); | ||
| const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>); | ||
|
|
||
| // Verify ErrorBoundary is present when CustomPreview is rendered | ||
| const errorBoundary = measureContent.find('ErrorBoundary'); | ||
| if (measureContent.find(customPreviewContent).exists()) { | ||
| expect(errorBoundary.exists()).toBe(true); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Conditional assertion weakens ErrorBoundary test.
Line 1943's if (measureContent.find(customPreviewContent).exists()) means the assertion only runs when CustomPreview is found. Since file is set and all props are present, CustomPreview should always render — assert unconditionally.
Suggested fix
- const errorBoundary = measureContent.find('ErrorBoundary');
- if (measureContent.find(customPreviewContent).exists()) {
- expect(errorBoundary.exists()).toBe(true);
- }
+ expect(measureContent.find(customPreviewContent).exists()).toBe(true);
+ expect(measureContent.find('ErrorBoundary').exists()).toBe(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('should wrap CustomPreview in ErrorBoundary', () => { | |
| const wrapper = getWrapper(props); | |
| wrapper.setState({ file }); | |
| // Find the Measure component and extract its render prop | |
| const measureComponent = wrapper.find('Measure'); | |
| const renderProp = measureComponent.prop('children'); | |
| const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>); | |
| // Verify ErrorBoundary is present when CustomPreview is rendered | |
| const errorBoundary = measureContent.find('ErrorBoundary'); | |
| if (measureContent.find(customPreviewContent).exists()) { | |
| expect(errorBoundary.exists()).toBe(true); | |
| } | |
| }); | |
| test('should wrap CustomPreview in ErrorBoundary', () => { | |
| const wrapper = getWrapper(props); | |
| wrapper.setState({ file }); | |
| // Find the Measure component and extract its render prop | |
| const measureComponent = wrapper.find('Measure'); | |
| const renderProp = measureComponent.prop('children'); | |
| const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>); | |
| // Verify ErrorBoundary is present when CustomPreview is rendered | |
| expect(measureContent.find(customPreviewContent).exists()).toBe(true); | |
| expect(measureContent.find('ErrorBoundary').exists()).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
In `@src/elements/content-preview/__tests__/ContentPreview.test.js` around lines
1932 - 1946, The test "should wrap CustomPreview in ErrorBoundary" currently
only asserts ErrorBoundary when customPreviewContent exists due to the
conditional if (measureContent.find(customPreviewContent).exists()); remove that
conditional and make the assertion unconditional: after rendering measureContent
from the Measure render prop (using wrapper, getWrapper, file, Measure, and
measureRef), directly assert that measureContent.find('ErrorBoundary').exists()
is true so the test always verifies ErrorBoundary wraps CustomPreview.
Comment on lines
+1948
to
+1971
| test('should call onPreviewError not onError in CustomPreview', () => { | ||
| const wrapper = getWrapper(props); | ||
| wrapper.setState({ file }); | ||
| const instance = wrapper.instance(); | ||
|
|
||
| // Verify the instance has onPreviewError method | ||
| expect(typeof instance.onPreviewError).toBe('function'); | ||
|
|
||
| // Verify onError doesn't exist as an instance method | ||
| expect(instance.onError).toBeUndefined(); | ||
|
|
||
| // Find CustomPreview in render output | ||
| const measureComponent = wrapper.find('Measure'); | ||
| const renderProp = measureComponent.prop('children'); | ||
| const measureContent = shallow(<div>{renderProp({ measureRef: jest.fn() })}</div>); | ||
|
|
||
| const CustomPreview = customPreviewContent; | ||
| const customPreviewInstance = measureContent.find(CustomPreview); | ||
|
|
||
| if (customPreviewInstance.exists()) { | ||
| // Verify it receives onPreviewError, not onError | ||
| expect(customPreviewInstance.prop('onError')).toBe(instance.onPreviewError); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Same conditional-assertion pattern weakens the onPreviewError wiring test.
The if (customPreviewInstance.exists()) guard at line 1967 means the key assertion at line 1969 might be skipped. Since this test sets file in state, CustomPreview should always render — assert unconditionally.
Suggested fix
const customPreviewInstance = measureContent.find(CustomPreview);
-
- if (customPreviewInstance.exists()) {
- // Verify it receives onPreviewError, not onError
- expect(customPreviewInstance.prop('onError')).toBe(instance.onPreviewError);
- }
+ expect(customPreviewInstance.exists()).toBe(true);
+ expect(customPreviewInstance.prop('onError')).toBe(instance.onPreviewError);🤖 Prompt for AI Agents
In `@src/elements/content-preview/__tests__/ContentPreview.test.js` around lines
1948 - 1971, The test currently guards the key assertion with if
(customPreviewInstance.exists()), which can skip the assertion; remove that
conditional and make two unconditional expectations: first assert that
customPreviewInstance.exists() is true (to guarantee CustomPreview rendered
after wrapper.setState({ file })), then assert
customPreviewInstance.prop('onError') === instance.onPreviewError; update the
test around the Measure render-prop extraction (measureContent and
customPreviewInstance) to perform these two expects without an if so the wiring
is always verified.
Comment on lines
+416
to
+428
| const { customPreviewContent, logger, onError } = this.props; | ||
|
|
||
| // Validate customPreviewContent type at runtime | ||
| if (customPreviewContent && typeof customPreviewContent !== 'function') { | ||
| const error = new Error('customPreviewContent must be a React component (function or class)'); | ||
| logger.logError(error, 'INVALID_CUSTOM_PREVIEW_TYPE', { | ||
| receivedType: typeof customPreviewContent, | ||
| receivedValue: String(customPreviewContent), | ||
| }); | ||
| onError(error, 'INVALID_PROP', { error }, ORIGIN_CONTENT_PREVIEW); | ||
| // Don't proceed with custom content | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Early return on invalid customPreviewContent skips fetchFile and focusPreview entirely.
When validation fails, the component won't fetch file metadata or set focus. The user sees a blank component with no error UI (no error state is set, so PreviewMask won't show an error either). Consider setting error state before returning so the mask renders an error, or falling through to the normal Box.Preview path as a degraded experience.
Proposed fix — set error state so PreviewMask renders
onError(error, 'INVALID_PROP', { error }, ORIGIN_CONTENT_PREVIEW);
- // Don't proceed with custom content
- return;
+ // Set error state so PreviewMask shows the error, then skip custom content loading
+ this.setState({ error: { code: 'INVALID_PROP', message: error.message }, isLoading: false });
+ // Fall through to still fetch file and focus, but custom content won't render due to error state
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { customPreviewContent, logger, onError } = this.props; | |
| // Validate customPreviewContent type at runtime | |
| if (customPreviewContent && typeof customPreviewContent !== 'function') { | |
| const error = new Error('customPreviewContent must be a React component (function or class)'); | |
| logger.logError(error, 'INVALID_CUSTOM_PREVIEW_TYPE', { | |
| receivedType: typeof customPreviewContent, | |
| receivedValue: String(customPreviewContent), | |
| }); | |
| onError(error, 'INVALID_PROP', { error }, ORIGIN_CONTENT_PREVIEW); | |
| // Don't proceed with custom content | |
| return; | |
| } | |
| const { customPreviewContent, logger, onError } = this.props; | |
| // Validate customPreviewContent type at runtime | |
| if (customPreviewContent && typeof customPreviewContent !== 'function') { | |
| const error = new Error('customPreviewContent must be a React component (function or class)'); | |
| logger.logError(error, 'INVALID_CUSTOM_PREVIEW_TYPE', { | |
| receivedType: typeof customPreviewContent, | |
| receivedValue: String(customPreviewContent), | |
| }); | |
| onError(error, 'INVALID_PROP', { error }, ORIGIN_CONTENT_PREVIEW); | |
| // Set error state so PreviewMask shows the error, then skip custom content loading | |
| this.setState({ error: { code: 'INVALID_PROP', message: error.message }, isLoading: false }); | |
| // Fall through to still fetch file and focus, but custom content won't render due to error state | |
| } |
🤖 Prompt for AI Agents
In `@src/elements/content-preview/ContentPreview.js` around lines 416 - 428, The
current early return in the customPreviewContent validation block prevents
fetchFile and focusPreview from running and never sets the component error
state, so PreviewMask cannot show the error; modify the validation branch in
ContentPreview (the block referencing customPreviewContent, logger.logError and
onError) to set the component's error state (e.g., via this.setState({ error:
error })) before returning so PreviewMask will render the error UI, or
alternatively allow execution to continue to the normal flow (so fetchFile and
focusPreview still run) while ensuring onError/logger still record the issue;
ensure ORIGIN_CONTENT_PREVIEW is passed unchanged and that no duplicate error
reports occur when choosing the continue-vs-return approach.
Comment on lines
+1553
to
+1558
| this.onPreviewError({ | ||
| error: { | ||
| ...validationError, | ||
| code: 'CUSTOM_PREVIEW_INVALID_PROPS', | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Spreading an Error object produces an empty object — message and stack are lost.
Error properties (message, name, stack) are non-enumerable, so {...validationError, code: '...'} yields { code: 'CUSTOM_PREVIEW_INVALID_PROPS' } with no message. Downstream, onPreviewError and any error handler relying on error.message will receive undefined.
Proposed fix
this.onPreviewError({
error: {
- ...validationError,
+ message: validationError.message,
code: 'CUSTOM_PREVIEW_INVALID_PROPS',
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.onPreviewError({ | |
| error: { | |
| ...validationError, | |
| code: 'CUSTOM_PREVIEW_INVALID_PROPS', | |
| }, | |
| }); | |
| this.onPreviewError({ | |
| error: { | |
| message: validationError.message, | |
| code: 'CUSTOM_PREVIEW_INVALID_PROPS', | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@src/elements/content-preview/ContentPreview.js` around lines 1553 - 1558, The
spread of validationError loses non-enumerable properties like message/stack, so
when calling onPreviewError you should preserve those by creating or augmenting
an Error instance instead of {...validationError, code: ...}; for example, build
a new Error using validationError.message (or clone by assigning
message/name/stack onto a new Error) and set err.code =
'CUSTOM_PREVIEW_INVALID_PROPS' before passing it to this.onPreviewError so
handlers receive a real Error with message and stack intact (refer to
this.onPreviewError and validationError in ContentPreview.js).
Comment on lines
+1565
to
+1577
| <ErrorBoundary | ||
| errorOrigin={ORIGIN_CONTENT_PREVIEW} | ||
| onError={elementsError => { | ||
| logger.logError( | ||
| new Error(elementsError.message), | ||
| 'CUSTOM_PREVIEW_RENDER_ERROR', | ||
| { | ||
| fileId: currentFileId, | ||
| fileName: file.name, | ||
| errorCode: elementsError.code, | ||
| }, | ||
| ); | ||
| }} |
Contributor
There was a problem hiding this comment.
ErrorBoundary onError discards the original stack trace.
new Error(elementsError.message) creates a new error with a fresh stack pointing at this callback, not at the actual crash site. Consider logging elementsError directly (or at least preserving the original error as a cause).
Suggested fix
<ErrorBoundary
errorOrigin={ORIGIN_CONTENT_PREVIEW}
onError={elementsError => {
logger.logError(
- new Error(elementsError.message),
+ elementsError,
'CUSTOM_PREVIEW_RENDER_ERROR',
{
fileId: currentFileId,
fileName: file.name,
- errorCode: elementsError.code,
},
);
}}
>🤖 Prompt for AI Agents
In `@src/elements/content-preview/ContentPreview.js` around lines 1565 - 1577, The
current onError handler creates a new Error with elementsError.message which
discards the original stack; instead pass the original error through to
logger.logError (or wrap it as the cause) so the original stack and metadata are
preserved—update the ErrorBoundary onError callback (the function that receives
elementsError) to log elementsError directly to logger.logError (keeping the
same context object with ORIGIN_CONTENT_PREVIEW, currentFileId, file.name and
elementsError.code) or, if you must create a new Error, include elementsError as
the cause so the original stack is retained.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new
customPreviewContentprop to the ContentPreview component that enables rendering custom content instead of the Box.Preview library. This provides an extensible pattern for implementing custom renderers for specific filetypes while maintaining all ContentPreview features (navigation, header, sidebar, error handling).
Related Issues
Changes
ContentPreview.js
customPreviewContentprop - Optional React component for custom rendering.bcpr-contentwhen providedTechnical Details
When
customPreviewContentis provided:fileId,token,apiHost,file,onError,onLoadFuture Work
This pattern enables:
Summary by CodeRabbit
New Features
Bug Fixes
Tests