fix(dashboard): Isolate PageContext in asset picker dialog#4439
fix(dashboard): Isolate PageContext in asset picker dialog#4439grolmus wants to merge 1 commit intovendurehq:masterfrom
Conversation
Wraps AssetPickerDialog content in an isolated PageContext.Provider to prevent parent page context (pageId, entity, form) from leaking into the dialog. This fixes an issue where custom action bar items registered for detail pages were incorrectly appearing inside the asset picker dialog. Fixes OSS-406
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe change involves wrapping the dialog content of an asset picker component with a PageContext provider. An import for PageContext was added, and the JSX structure was reorganized to place the DialogContent, DialogHeader, AssetGallery, and DialogFooter elements inside the new PageContext.Provider wrapper. The provider is initialized with an empty value object. The component's public API and props remain unchanged. This is a structural refactoring focused on the internal rendering context. 🚥 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/src/lib/components/shared/asset/asset-picker-dialog.tsx (1)
82-82: Consider using a stable reference for the empty context value.Passing
value={{}}creates a new object on every render, which could cause unnecessary re-renders of any memoized context consumers inside the dialog. A stable reference would be more efficient:♻️ Suggested improvement
Define a constant outside the component:
const EMPTY_PAGE_CONTEXT = {};Then use it:
- <PageContext.Provider value={{}}> + <PageContext.Provider value={EMPTY_PAGE_CONTEXT}>The empty object is type-compatible with
PageContextValuesince all properties are optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dashboard/src/lib/components/shared/asset/asset-picker-dialog.tsx` at line 82, PageContext.Provider is being passed a new empty object literal (value={{}}) on every render which can cause needless re-renders; define a stable constant (e.g. EMPTY_PAGE_CONTEXT) outside the component with the same empty-object shape and replace value={{}} with value={EMPTY_PAGE_CONTEXT} so consumers receive a stable reference (ensure the constant is typed to PageContextValue if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/dashboard/src/lib/components/shared/asset/asset-picker-dialog.tsx`:
- Line 82: PageContext.Provider is being passed a new empty object literal
(value={{}}) on every render which can cause needless re-renders; define a
stable constant (e.g. EMPTY_PAGE_CONTEXT) outside the component with the same
empty-object shape and replace value={{}} with value={EMPTY_PAGE_CONTEXT} so
consumers receive a stable reference (ensure the constant is typed to
PageContextValue if needed).
biggamesmallworld
left a comment
biggamesmallworld
left a comment
There was a problem hiding this comment.
Review — one suggestion
Clean fix for the Radix portal context propagation issue. The approach is correct and minimal.
💡 Suggestion: Add a comment explaining the PageContext.Provider isolation
The wrapper at line 82 exists purely to prevent parent page extension action bar items from leaking into the dialog via Radix portal context propagation. Without a comment, a future developer might remove it while "cleaning up" and reintroduce the bug. Something like:
{/* Isolate from parent PageContext to prevent extension action bar items
registered for the parent page from rendering inside this dialog.
See OSS-406. */}
<PageContext.Provider value={{}}>Optionally, hoisting to const EMPTY_PAGE_CONTEXT = {}; at module scope would avoid a new object reference per render (minor, since this is a short-lived modal).
LGTM otherwise 👍
2 participants
Summary
AssetPickerDialogcontent in isolatedPageContext.Providerto prevent parent page context leakageRoot Cause
React Context propagates through Radix Dialog portals. When
AssetPickerDialogopened on a detail page, it inherited the parent'sPageContext(includingpageId). Extension action bar items registered for thatpageIdwere then incorrectly rendered inside the dialog.Test plan
Fixes OSS-406