Fix PluginObjectView split/popover navigation modes falling back to drawer#891
Fix PluginObjectView split/popover navigation modes falling back to drawer#891
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
… split/popover support Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 仔细评估是否已经完成了任务中的所有要求 |
已逐项核对 issue 中的全部检查清单,确认所有 6 种导航模式均已修复并有测试覆盖:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes record-row navigation behavior so split and popover modes render the intended NavigationOverlay UI instead of silently falling back to drawer behavior, and tightens Console onNavigate handling for page navigation actions.
Changes:
- Update PluginObjectView to map
split/popovernavigation modes and renderNavigationOverlayfor those modes. - Adjust Console ObjectView
onNavigatehandling to explicitly treataction === 'view'as page navigation. - Update/add tests around split/popover rendering behaviors and split panel close behavior; document the fix in
ROADMAP.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/plugin-view/src/ObjectView.tsx | Adds NavigationOverlay rendering paths for split and popover navigation modes. |
| packages/plugin-view/src/tests/ObjectView.test.tsx | Updates split/popover expectations and adds a split-close regression test. |
| apps/console/src/components/ObjectView.tsx | Refines onNavigate logic for page mode vs new-window behavior. |
| ROADMAP.md | Documents root cause, fix approach, and test coverage summary for the navigation mode bug. |
| // Handles action === 'view' (from useNavigationOverlay page mode) and | ||
| // default fallthrough for any unrecognised action | ||
| if (action === 'view' || !action || action === 'page') { | ||
| if (viewId) { | ||
| navigate(`../../record/${String(recordId)}`, { relative: 'path' }); | ||
| } else { | ||
| navigate(`record/${String(recordId)}`); | ||
| } |
There was a problem hiding this comment.
useNavigationOverlay calls onNavigate(recordId, view ?? 'view') for page mode, where view comes from navigation.view (a user-configurable target view name). With the new guard, any configured navigation.view value other than 'view' will prevent navigation entirely. Consider treating any action other than 'new_window' as page navigation (or explicitly handling navigation.view by incorporating it into the route/query) so page mode keeps working when a view name is set.
| {isFormOpen && selectedRecord ? ( | ||
| <NavigationOverlay | ||
| isOpen={isFormOpen} | ||
| selectedRecord={selectedRecord} | ||
| mode="split" |
There was a problem hiding this comment.
In split navigation mode, the overlay is only rendered when isFormOpen && selectedRecord. This means create-mode (where selectedRecord is intentionally null) will not render any form at all, and edit/view behavior becomes dependent on selectedRecord being set. Consider using split/popup overlays only for record-detail view (e.g., formMode === 'view'), or falling back to the existing drawer/modal form container for create/edit so those flows keep working when navigation.mode is split.
| {/* Popover mode — uses NavigationOverlay Dialog fallback (no popoverTrigger) */} | ||
| {formLayout === 'popover' && isFormOpen && selectedRecord && ( | ||
| <NavigationOverlay | ||
| isOpen={isFormOpen} | ||
| selectedRecord={selectedRecord} | ||
| mode="popover" |
There was a problem hiding this comment.
Popover mode has the same isFormOpen && selectedRecord gate. As a result, create-mode (no selectedRecord) cannot open a form when navigation.mode is popover because neither drawer/modal nor NavigationOverlay will render. Recommend ensuring create/edit continue to render via the normal form container, and reserving the popover NavigationOverlay for record-detail view only.
| : layout; | ||
|
|
||
| // Build the record detail content for NavigationOverlay (split/popover modes) | ||
| const renderOverlayDetail = (record: Record<string, unknown>) => ( |
There was a problem hiding this comment.
renderOverlayDetail receives a record argument from NavigationOverlay, but it is unused (the form schema is instead derived from selectedRecord in closure). Rename the parameter to _record to avoid unused-var noise, or refactor buildFormSchema to accept the record so the overlay content is purely driven by the provided record.
| const renderOverlayDetail = (record: Record<string, unknown>) => ( | |
| const renderOverlayDetail = (_record: Record<string, unknown>) => ( |
| <div className={cn('flex flex-col h-full min-w-0 overflow-hidden', className)}> | ||
| {(schema.title || schema.description) && ( | ||
| <div className="mb-4 shrink-0"> | ||
| {schema.title && <h2 className="text-2xl font-bold tracking-tight">{schema.title}</h2>} | ||
| {schema.description && <p className="text-muted-foreground mt-1">{schema.description}</p>} |
There was a problem hiding this comment.
The split-mode branch duplicates the outer layout structure (title/description/toolbar wrappers) from the main return path. This increases the risk of future UI divergence between split and non-split modes. Consider extracting a shared wrapper/layout component/function so split mode only changes the content region wiring.
| // Split mode renders NavigationOverlay with split panels including a close button | ||
| expect(screen.getByTestId('object-form')).toBeDefined(); | ||
| expect(screen.getByLabelText('Close panel')).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
The new assertions use toBeDefined(), which is redundant with getBy* queries (they already throw if not found) and is inconsistent with other tests in this file that use toBeInTheDocument(). Prefer expect(...).toBeInTheDocument() (or drop the matcher entirely) for clearer intent.
Only drawer/modal navigation modes worked when clicking ListView rows. Split and popover modes silently fell back to rendering a drawer form because
formLayoutdidn't map them, and noNavigationOverlaywas rendered for those modes.PluginObjectView (
packages/plugin-view/src/ObjectView.tsx)formLayoutnow mapssplitandpopover— previously both fell through tolayout(default'drawer')NavigationOverlaywithmainContentwrapping the grid in a resizable side-by-side panelNavigationOverlayin popover mode (Dialog fallback when nopopoverTrigger)handleOverlayOpenChangecallback to avoid inline duplicationConsole ObjectView (
apps/console/src/components/ObjectView.tsx)onNavigatenow explicitly checksaction === 'view'for page mode instead of relying on implicit fallthrough after the'new_window'guardTests
Close panelbutton fromNavigationOverlaydialogrole from the Dialog fallbackROADMAP
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.