Skip to content

Commit b7dc6f9

Browse files
Copilothotlong
andcommitted
fix: memoize detailNavigation and onNavigate to prevent stale closure 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>
1 parent 9f9387d commit b7dc6f9

3 files changed

Lines changed: 93 additions & 5 deletions

File tree

ROADMAP.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1129,6 +1129,17 @@ The `FlowDesigner` is a canvas-based flow editor that bridges the gap between th
11291129

11301130
## 🐛 Bug Fixes
11311131

1132+
### Default Navigation Mode (Page) Clicks Have No Effect — Stale Closure (February 2026)
1133+
1134+
**Root Cause:** Three compounding issues created a stale closure chain in `ObjectView.tsx`:
1135+
1. `{ mode: 'page' }` fallback created a new object literal every render, causing `useNavigationOverlay`'s `handleClick` `useCallback` to be recreated with potentially stale deps.
1136+
2. The `onNavigate` callback was an inline arrow function with unstable identity, so React could batch-skip the render where `handleClick` picks up the fresh closure.
1137+
3. Cascade instability: `navOverlay` (from `useMemo`) got a new reference every render because its deps (`handleClick`, etc.) changed, propagating stale closures through `renderListView`.
1138+
1139+
**Fix:** Memoized `detailNavigation` with `useMemo` (stable reference for the `{ mode: 'page' }` fallback) and extracted `onNavigate` into a `useCallback` (`handleNavOverlayNavigate`) with `[navigate, viewId]` deps. This ensures stable identities for both inputs to `useNavigationOverlay`, preventing stale closures.
1140+
1141+
**Tests:** Added 2 stale closure prevention tests in `useNavigationOverlay.test.ts`: (1) verify `handleClick` uses latest `onNavigate` after re-render with new callback, (2) verify navigation works after config changes from undefined to explicit page mode. All 31 useNavigationOverlay tests and 739 console tests pass.
1142+
11321143
### ListView Grouping Config Not Taking Effect (February 2026)
11331144

11341145
**Root Cause:** `viewComponentSchema` `useMemo` in `ListView.tsx` was missing `groupingConfig`, `rowColorConfig`, and `navigation.handleClick` in its dependency array. When users toggled grouping fields via the toolbar popover, the state changed but the memoized schema was not recomputed, so the child grid/kanban/gallery never received the updated grouping config.

apps/console/src/components/ObjectView.tsx

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -420,12 +420,15 @@ export function ObjectView({ dataSource, objects, onEdit, onRowClick }: any) {
420420

421421
// Navigation overlay for record detail (supports drawer/modal/split/popover via config)
422422
// Priority: activeView.navigation > objectDef.navigation > default page
423-
const detailNavigation: ViewNavigationConfig = activeView?.navigation ?? objectDef.navigation ?? { mode: 'page' };
423+
// Memoize to avoid unstable object identity on every render (stale closure prevention)
424+
const detailNavigation: ViewNavigationConfig = useMemo(
425+
() => activeView?.navigation ?? objectDef.navigation ?? { mode: 'page' },
426+
[activeView?.navigation, objectDef.navigation]
427+
);
424428
const drawerRecordId = searchParams.get('recordId');
425-
const navOverlay = useNavigationOverlay({
426-
navigation: detailNavigation,
427-
objectName: objectDef.name,
428-
onNavigate: (recordId: string | number, action?: string) => {
429+
// Memoize onNavigate to prevent stale closure in useNavigationOverlay's handleClick
430+
const handleNavOverlayNavigate = useCallback(
431+
(recordId: string | number, action?: string) => {
429432
if (action === 'new_window') {
430433
// Open record detail in a new browser tab with Console-correct URL
431434
const basePath = window.location.pathname.replace(/\/view\/.*$/, '');
@@ -443,6 +446,12 @@ export function ObjectView({ dataSource, objects, onEdit, onRowClick }: any) {
443446
}
444447
}
445448
},
449+
[navigate, viewId]
450+
);
451+
const navOverlay = useNavigationOverlay({
452+
navigation: detailNavigation,
453+
objectName: objectDef.name,
454+
onNavigate: handleNavOverlayNavigate,
446455
});
447456
const handleDrawerClose = () => {
448457
navOverlay.close();

packages/react/src/__tests__/useNavigationOverlay.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,4 +507,72 @@ describe('useNavigationOverlay', () => {
507507
expect(result.current.view).toBeUndefined();
508508
});
509509
});
510+
511+
// ============================================================
512+
// Stale closure prevention
513+
// ============================================================
514+
515+
describe('stale closure prevention', () => {
516+
it('should use latest onNavigate after re-render with new callback', () => {
517+
const onNavigateV1 = vi.fn();
518+
const onNavigateV2 = vi.fn();
519+
520+
const { result, rerender } = renderHook(
521+
({ onNavigate }) =>
522+
useNavigationOverlay({
523+
navigation: { mode: 'page' },
524+
objectName: 'contacts',
525+
onNavigate,
526+
}),
527+
{ initialProps: { onNavigate: onNavigateV1 } }
528+
);
529+
530+
// First click uses v1
531+
act(() => {
532+
result.current.handleClick({ _id: '1' });
533+
});
534+
expect(onNavigateV1).toHaveBeenCalledWith('1', 'view');
535+
expect(onNavigateV2).not.toHaveBeenCalled();
536+
537+
// Re-render with new onNavigate
538+
rerender({ onNavigate: onNavigateV2 });
539+
540+
// Second click must use v2 (not stale v1)
541+
act(() => {
542+
result.current.handleClick({ _id: '2' });
543+
});
544+
expect(onNavigateV2).toHaveBeenCalledWith('2', 'view');
545+
expect(onNavigateV1).toHaveBeenCalledTimes(1); // v1 was not called again
546+
});
547+
548+
it('should use latest onNavigate after navigation config changes from undefined to page', () => {
549+
const onNavigate = vi.fn();
550+
551+
const { result, rerender } = renderHook(
552+
({ navigation }) =>
553+
useNavigationOverlay({
554+
navigation,
555+
objectName: 'contacts',
556+
onNavigate,
557+
}),
558+
{ initialProps: { navigation: undefined as NavigationConfig | undefined } }
559+
);
560+
561+
// Click with no config — should still call onNavigate (default page behavior)
562+
act(() => {
563+
result.current.handleClick({ _id: '1' });
564+
});
565+
expect(onNavigate).toHaveBeenCalledWith('1', 'view');
566+
567+
// Re-render with explicit page config
568+
rerender({ navigation: { mode: 'page' } });
569+
570+
// Click should still work (not stale)
571+
act(() => {
572+
result.current.handleClick({ _id: '2' });
573+
});
574+
expect(onNavigate).toHaveBeenCalledWith('2', 'view');
575+
expect(onNavigate).toHaveBeenCalledTimes(2);
576+
});
577+
});
510578
});

0 commit comments

Comments
 (0)