Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
… in page navigation Memoize the fallback navigation config with useMemo and extract the onNavigate callback into useCallback to ensure stable references. This prevents the stale closure chain that caused default page-mode row clicks to have no effect. Add 2 stale-closure prevention tests to useNavigationOverlay.test.ts. Update ROADMAP.md with root cause analysis and fix documentation. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Console row-click navigation when navigation.mode falls back to the default ('page') by stabilizing references passed into useNavigationOverlay, preventing stale/incorrect handler closures during rerenders.
Changes:
- Memoize
detailNavigationand extractonNavigateinto auseCallbackinapps/console/src/components/ObjectView.tsx. - Add two regression tests around rerender/option transitions in
packages/react/src/__tests__/useNavigationOverlay.test.ts. - Document the root cause and fix in
ROADMAP.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/console/src/components/ObjectView.tsx | Stabilizes navigation config + onNavigate callback identities passed to useNavigationOverlay. |
| packages/react/src/tests/useNavigationOverlay.test.ts | Adds rerender-focused tests intended to guard against stale-closure regressions. |
| ROADMAP.md | Adds a bug-fix entry describing the stale-closure chain and the mitigation. |
| const { result, rerender } = renderHook( | ||
| ({ onNavigate }) => | ||
| useNavigationOverlay({ | ||
| navigation: { mode: 'page' }, | ||
| objectName: 'contacts', | ||
| onNavigate, | ||
| }), |
There was a problem hiding this comment.
In the rerender test, navigation: { mode: 'page' } is created inline inside the hook callback, so the navigation object reference changes on every rerender. That means this test will still pass even if handleClick only updates because navigation changed, so it doesn’t reliably prove that onNavigate updates are picked up independently. Consider hoisting the navigation object to a stable const (or passing it via props) so the rerender only changes onNavigate.
| it('should use latest onNavigate after navigation config changes from undefined to page', () => { | ||
| const onNavigate = vi.fn(); | ||
|
|
||
| const { result, rerender } = renderHook( | ||
| ({ navigation }) => | ||
| useNavigationOverlay({ | ||
| navigation, | ||
| objectName: 'contacts', | ||
| onNavigate, | ||
| }), | ||
| { initialProps: { navigation: undefined as NavigationConfig | undefined } } | ||
| ); |
There was a problem hiding this comment.
The test name mentions "use latest onNavigate" but onNavigate never changes in this test; it’s validating behavior across a navigation prop transition (undefined → page). Renaming the test (or adjusting it to actually vary onNavigate) would make the intent clearer and help prevent confusion when maintaining these tests.
| // page / view mode — navigate to record detail page | ||
| // 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, navigation.view ?? 'view') for page mode. If a view is configured (e.g. 'detail_view'), action will be that string, but this handler only navigates for 'view' | 'page' | undefined, so clicks will silently do nothing. Consider treating any non-new_window action as a page navigation (or explicitly handling the view value).
Row clicks with default
navigation.mode: 'page'silently do nothing due to a stale closure chain inObjectView.tsx. Three unstable references compound: the{ mode: 'page' }fallback object literal, the inlineonNavigatearrow, and the resultingnavOverlay— all recreated every render, causinguseNavigationOverlay'shandleClickto capture stale/undefinedonNavigate.Changes
apps/console/src/components/ObjectView.tsx— WrapdetailNavigationinuseMemoand extractonNavigateintouseCallbackwith[navigate, viewId]deps:packages/react/src/__tests__/useNavigationOverlay.test.ts— Two new tests: verifyhandleClickpicks up a replacedonNavigateafter rerender, and verify navigation survives config transition fromundefined→{ mode: 'page' }.ROADMAP.md— Documented root cause and fix.Original prompt
This section details on the original issue you should resolve
<issue_title>Bug: Default Navigation Mode (Page) Clicks Have No Effect — Stale Closure Root Cause</issue_title>
<issue_description>
Summary
Clicking a row in the grid/list when the default navigation mode is
Pagedoes nothing. This has been reported and patched multiple times but the underlying root cause was never addressed. The bug is a stale closure chain caused by unmemoized objects/callbacks passed intouseNavigationOverlay.Screenshot
Root Cause Analysis (3 Compounding Issues)
Issue 1:
{ mode: 'page' }fallback creates a new object every renderIn
apps/console/src/components/ObjectView.tsxline 423:{ mode: 'page' }is a new object literal on every render. This causes thenavigationoption passed touseNavigationOverlayto change reference every render, which in turn causes its internalhandleClickuseCallbackto be recreated every render.Issue 2:
onNavigatecallback is not memoizedIn
apps/console/src/components/ObjectView.tsxlines 428-445, theonNavigateis an inline arrow function:Because both
navigationANDonNavigatehave unstable identities, React may batch-skip the render wherehandleClickpicks up the freshonNavigate, resulting in a stale (or initial undefined) closure being called.Issue 3: Cascade instability through
renderListViewnavOverlay(returned fromuseMemo) gets a new reference every render because its deps (handleClick, etc.) changed. This propagates throughrenderListView'suseCallbackdeps, causing the<ListView>to re-render with potentially stale intermediate closures during React's batched updates.Call Chain Trace
Proposed Fix
Memoize the navigation config and
onNavigatecallback inapps/console/src/components/ObjectView.tsx:Files Affected
apps/console/src/components/ObjectView.tsxdetailNavigationwithuseMemo, extractonNavigateintouseCallbackpackages/react/src/__tests__/useNavigationOverlay.test.tsROADMAP.mdAcceptance Criteria
navigation.mode: 'page'(default) navigates to record detail pagenavigation.mode: 'drawer'opens drawer overlaynavigation.mode: 'modal'opens modal overlaynavigation.mode: 'none'does nothinguseNavigationOverlaytests pass (29 tests)ObjectViewtests pass (32 tests)handleClickuses latestonNavigateafter re-renderROADMAP.mdwith root cause and fixLabels
bug,P0,navigation</issue_description>Comments on the Issue (you are @copilot in this section)
<comments...
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.