fix(metadataeditor): portal dropdown to preview container #4273
Conversation
WalkthroughListen up! Adds a new PreviewContext exposing a previewBodyRef, provides it from ContentPreview (attached to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CP as ContentPreview
participant PCtx as PreviewContext.Provider
participant MIE as MetadataInstanceEditor
participant MIF as MetadataInstanceForm
U->>CP: Mount ContentPreview
CP->>PCtx: Provide { previewBodyRef } (ref object)
Note right of CP #D3E4CD: `bcpr-body` attached to previewBodyRef
PCtx-->>MIE: useContext() -> previewBodyRef (ref object)
MIE->>MIF: Render with customRef = previewBodyRef?.current
MIF->>CP: Portal dropdowns / anchors to preview body element as needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/elements/content-preview/ContentPreview.js (2)
🪛 Biome (2.1.2)src/elements/content-preview/ContentPreview.js[error] 217-217: Parenthesized expression didnt contain anything Expected an expression here (parse) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
✨ Finishing touches
🧪 Generate unit tests
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.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/elements/content-preview/PreviewContext.ts (1)
3-9: Pass a stable ref object through context, fool!Passing the element value risks stale/null reads and extra re-renders. Share the RefObject instead so consumers always read the latest via
.current. I pity the stale ref!Apply:
-export interface PreviewContextType { - previewBodyRef: HTMLDivElement | null; -} +export interface PreviewContextType { + previewBodyRef: React.RefObject<HTMLDivElement>; +} -const PreviewContext = React.createContext<PreviewContextType | null>(null); +const PreviewContext = React.createContext<PreviewContextType | null>(null); +PreviewContext.displayName = 'PreviewContext';src/elements/content-sidebar/MetadataInstanceEditor.tsx (1)
55-57: Use.currentif context provides a RefObjectIf PreviewContext switches to provide a RefObject, grab the element via
.currentbefore passing toMetadataInstanceForm. Don’t make your portals homeless, sucker!Please confirm
MetadataInstanceForm’scustomRefprop expectsHTMLElement | null.-const previewContext: PreviewContextType | null = useContext(PreviewContext); -const customRef = previewContext?.previewBodyRef; +const previewContext: PreviewContextType | null = useContext(PreviewContext); +const customRef = previewContext?.previewBodyRef?.current; ... - customRef={customRef} + customRef={customRef}Also applies to: 78-79
src/elements/content-preview/ContentPreview.js (2)
29-30: Flow/TS interop nit: add suppression for TS importThis file is Flow;
PreviewContext.tsis TS. For consistency with nearby imports, add a Flow suppression to avoid type tooling grief.-// $FlowFixMe TypeScript file -import ThemingStyles from '../common/theming'; +// $FlowFixMe TypeScript file +import ThemingStyles from '../common/theming'; +// $FlowFixMe TypeScript file +import PreviewContext from './PreviewContext';
1334-1336: Provide the RefObject via context, not.currentPassing
.currentlocks the value to whatever it was at render time and forces extra updates. Pass the ref object; consumers read.currentwhen needed. That’s how you keep it fresh, fool.- <PreviewContext.Provider value={{ previewBodyRef: this.previewBodyRef.current }}> + <PreviewContext.Provider value={{ previewBodyRef: this.previewBodyRef }}>Optional perf nit: cache the provider value on
thisto avoid a new object each render.src/elements/content-preview/stories/tests/ContentPreview-metadata-visual.stories.js (1)
1-3: Add an assertion that the dropdown is portaled inside preview bodyProve the fix by asserting the listbox lives under
.bcpr-body. Don’t just click — verify, sucka!-import { userEvent, within } from 'storybook/test'; +import { userEvent, within, expect } from 'storybook/test'; ... play: async ({ canvasElement }) => { const canvas = within(canvasElement); const editButton = await canvas.findByRole('button', { name: 'Edit Long Dropdown Test Template' }); await userEvent.click(editButton); const dropdownField = await canvas.findByRole('combobox', { name: 'Department Selection' }); await userEvent.click(dropdownField); - await canvas.findByRole('option', { + const option = await canvas.findByRole('option', { name: 'Engineering - Software Development - Frontend React TypeScript Team Alpha Division', }); + const listbox = option.closest('[role="listbox"]'); + expect(listbox).not.toBeNull(); + expect(listbox?.closest('.bcpr-body')).not.toBeNull(); },Also applies to: 141-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/content-preview/ContentPreview.js(3 hunks)src/elements/content-preview/PreviewContext.ts(1 hunks)src/elements/content-preview/stories/tests/ContentPreview-metadata-visual.stories.js(1 hunks)src/elements/content-sidebar/MetadataInstanceEditor.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/elements/content-sidebar/MetadataInstanceEditor.tsx (1)
src/elements/content-preview/PreviewContext.ts (1)
PreviewContextType(3-5)
src/elements/content-preview/stories/tests/ContentPreview-metadata-visual.stories.js (1)
src/elements/common/__mocks__/mockRequests.ts (2)
mockUserRequest(10-30)mockEventRequest(5-8)
src/elements/content-preview/ContentPreview.js (3)
src/features/message-preview-content/MessagePreviewContent.js (1)
previewRef(40-40)src/elements/content-preview/PreviewMask.tsx (1)
PreviewMask(13-23)src/elements/content-preview/PreviewNavigation.js (1)
PreviewNavigation(153-161)
🪛 Biome (2.1.2)
src/elements/content-preview/ContentPreview.js
[error] 216-216: return types can only be used in TypeScript files
remove this type annotation
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
src/elements/content-preview/stories/tests/ContentPreview-metadata-visual.stories.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-preview/ContentPreview.js (1)
217-218: Fix TS generic in Flow/JS file to resolve Biome parse errorBiome chokes here (“Parenthesized expression didn’t contain anything”). Drop the TypeScript generic; keep it plain JS. I pity the fool who lets the build fail!
Apply this diff:
- previewBodyRef = React.createRef<HTMLDivElement>(); + previewBodyRef = React.createRef();
🧹 Nitpick comments (1)
src/elements/content-preview/ContentPreview.js (1)
1335-1337: Stabilize PreviewContext value to avoid unnecessary re-rendersYou’re creating a new object every render. Memoize the provider value so consumers don’t re-render for no reason, fool.
Apply this diff here:
- <PreviewContext.Provider value={{ previewBodyRef: this.previewBodyRef }}> + <PreviewContext.Provider value={this.previewContextValue}>And add this class field (right after the previewBodyRef field) so the identity is stable:
previewContextValue = { previewBodyRef: this.previewBodyRef };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-preview/ContentPreview.js(3 hunks)src/elements/content-preview/PreviewContext.ts(1 hunks)src/elements/content-sidebar/MetadataInstanceEditor.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-preview/PreviewContext.ts
- src/elements/content-sidebar/MetadataInstanceEditor.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-preview/ContentPreview.js (1)
src/elements/content-preview/PreviewMask.tsx (1)
PreviewMask(13-23)
🪛 Biome (2.1.2)
src/elements/content-preview/ContentPreview.js
[error] 217-217: Parenthesized expression didnt contain anything
Expected an expression here
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-preview/ContentPreview.js (4)
29-31: LGTM: PreviewContext import is cleanImport looks good with the Flow suppression. Rock solid, fool.
1361-1373: LGTM: Ref wiring and Measure usageHooking bcpr-body to the ref and wrapping bcpr-content with Measure looks right. Smooth move, sucka.
1387-1405: Sidebar relocation: verify stacking and hover behaviorMoving LoadableSidebar outside bcpr-container may tweak stacking/hover regions. Please sanity-check:
- z-index over preview content and dropdowns
- nav arrow visibility logic when hovering sidebar (onMouseMove is on bcpr-container)
Don’t ship surprises, fool.
1409-1412: ReloadNotification placement change: verify layeringAfter moving it below bcpr-body, confirm it still overlays preview and is clickable above masks/sidebars.
bfoxx1906
left a comment
bfoxx1906
left a comment
There was a problem hiding this comment.
looks straight forward.
3 participants
Portal metadata editor onto preview body so dropdown options don't get cut off
Before:
After:
Summary by CodeRabbit