From 7960789d37f7374d6658abbf99086c5824017fc5 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 9 Jun 2026 22:44:30 -0700 Subject: [PATCH] FN-6167: fix stale mobile modal navigation history Keep modal history entries in sync so mobile swipe-back reliably closes reopened task detail views. - add removeNav support to navigation history and ignore self-triggered popstate callbacks - route modal and overlay programmatic close handlers through navigation-aware wrappers - add regression coverage for close-and-reopen mobile detail flows and document the stale stack bug Files changed: .../navigation-history-stale-modal-stack.md | 85 +++++++++++++ packages/dashboard/app/App.tsx | 35 ++++-- packages/dashboard/app/components/AppModals.tsx | 137 +++++++++++++++++---- .../app/components/__tests__/AppModals.test.tsx | 5 +- .../__tests__/navigation-history.test.tsx | 113 ++++++++++++++++- .../hooks/__tests__/useNavigationHistory.test.ts | 98 +++++++++++++++ .../dashboard/app/hooks/useNavigationHistory.ts | 55 ++++++++- 7 files changed, 494 insertions(+), 34 deletions(-) Fusion-Task-Id: FN-6167 Fusion-Task-Lineage: 086d60ba-7e3d-4e83-b9b4-37ec49b06393 --- .../navigation-history-stale-modal-stack.md | 85 +++++++++++ packages/dashboard/app/App.tsx | 35 +++-- .../dashboard/app/components/AppModals.tsx | 137 +++++++++++++++--- .../components/__tests__/AppModals.test.tsx | 5 +- .../__tests__/navigation-history.test.tsx | 113 ++++++++++++++- .../__tests__/useNavigationHistory.test.ts | 98 +++++++++++++ .../app/hooks/useNavigationHistory.ts | 55 ++++++- 7 files changed, 494 insertions(+), 34 deletions(-) create mode 100644 docs/solutions/ui-bugs/navigation-history-stale-modal-stack.md diff --git a/docs/solutions/ui-bugs/navigation-history-stale-modal-stack.md b/docs/solutions/ui-bugs/navigation-history-stale-modal-stack.md new file mode 100644 index 0000000000..09c6a9a2ef --- /dev/null +++ b/docs/solutions/ui-bugs/navigation-history-stale-modal-stack.md @@ -0,0 +1,85 @@ +--- +title: "Navigation history stale modal stack" +date: 2026-06-09 +category: ui-bugs +module: packages/dashboard/app/hooks/useNavigationHistory +problem_type: ui_bug +component: frontend_navigation +symptoms: + - "Mobile browser swipe-back sometimes does not close a reopened task detail modal" + - "Browser back works after a fresh modal open but becomes inconsistent after closing with the X button" +root_cause: state_desync +resolution_type: code_fix +severity: medium +related_components: + - packages/dashboard/app/components/AppModals.tsx + - packages/dashboard/app/App.tsx + - packages/dashboard/app/hooks/useModalManager.ts +tags: + - history-api + - popstate + - modal-navigation + - mobile-safari + - stale-stack +--- + +# Navigation history stale modal stack + +## Problem + +`useNavigationHistory` mirrors modal/view opens into `window.history.pushState()` so browser back and mobile swipe-back can close the top modal or revert an overlay view. The internal stack must stay aligned with browser history entries. + +A task detail modal could be opened, closed with the rendered close affordance, opened again, and then fail to close on mobile swipe-back. The close affordance updated modal state but did not remove the corresponding navigation entry, leaving a stale callback on the hook's stack. + +## Symptoms + +- Fresh open → browser back/swipe-back closes the modal. +- Open → close with X/mobile back button → reopen → swipe-back can no-op. +- The `popstate` handler sees a target `navIndex` that no longer matches the stale internal stack and returns without invoking the close callback. +- The issue is most visible on mobile because the fullscreen task detail modal has no overlay tap target; the browser gesture is the primary touch dismiss path. + +## Root cause + +`pushNav({ type: "modal", close })` added both: + +1. an internal `NavEntry` in `useNavigationHistory`, and +2. a browser history entry via `history.pushState({ navIndex })`. + +When a modal closed programmatically (X button, mobile header back button, state-driven close), only React/modal state changed. The hook stack retained the old `NavEntry`, so future opens produced duplicate or misindexed stack state. Browser history still moved independently, and `popstate` index arithmetic could silently bail out. + +## Solution + +Add `removeNav(closeOrRevert)` to `useNavigationHistory` and call it from programmatic close paths before the actual close callback: + +```tsx +const closeDetailWithNav = useCallback(() => { + removeNav(modalManager.closeDetailTask); + deepLink.handleDetailClose(); +}, [deepLink, modalManager.closeDetailTask, removeNav]); +``` + +`removeNav` is the inverse of `pushNav` for programmatic dismissals: + +1. Search the stack from top to bottom for the matching `close` or `revert` callback. +2. Remove the matching entry from the internal stack. +3. Call `window.history.back()` to consume the browser history entry. +4. Mark the resulting `popstate` as self-triggered so the handler does not call the close/revert callback a second time. + +The normal browser back/swipe path must **not** call `removeNav`; `popstate` already pops the stack and invokes the entry callback. + +## Prevention + +- Any modal or overlay view opened with `pushNav` must have its programmatic close path wired to `removeNav` with the same callback reference used by `pushNav`. +- Keep pushed callbacks stable (`useCallback` or modal-manager callbacks). Anonymous `revert: () => ...` callbacks cannot be removed later unless they are stored in a stable variable. +- For modal-to-modal transitions that reuse the same browser history slot, prefer `replaceCurrent` rather than `removeNav` + `pushNav`. +- Regression coverage should include both paths: + - normal open → popstate closes modal + - open → programmatic close → reopen → popstate closes modal + +## Related files + +- `packages/dashboard/app/hooks/useNavigationHistory.ts` +- `packages/dashboard/app/hooks/__tests__/useNavigationHistory.test.ts` +- `packages/dashboard/app/components/__tests__/navigation-history.test.tsx` +- `packages/dashboard/app/components/AppModals.tsx` +- `packages/dashboard/app/App.tsx` diff --git a/packages/dashboard/app/App.tsx b/packages/dashboard/app/App.tsx index 3703cbfc8e..de03100020 100644 --- a/packages/dashboard/app/App.tsx +++ b/packages/dashboard/app/App.tsx @@ -381,7 +381,7 @@ function AppInner() { const isMobile = viewportMode === "mobile"; // Navigation history for browser back button (desktop + mobile). - const { pushNav, replaceCurrent } = useNavigationHistory({ enabled: true }); + const { pushNav, replaceCurrent, removeNav } = useNavigationHistory({ enabled: true }); // View state must be defined before useTasks since useTasks depends on taskView for SSE gating const { viewMode, setViewMode, taskView, handleChangeTaskView } = useViewState({ @@ -755,6 +755,9 @@ function AppInner() { // Nodes management is an overlay view (not a modal), so it stays local to App. const [nodesOpen, setNodesOpen] = useState(false); + const closeNodes = useCallback(() => { + setNodesOpen(false); + }, []); const [retryingProjects, setRetryingProjects] = useState(false); const [missionResumeSessionId, setMissionResumeSessionId] = useState(undefined); const [missionTargetId, setMissionTargetId] = useState(undefined); @@ -953,6 +956,11 @@ function AppInner() { void refreshAppSettings(); }, [modalManager, refreshAppSettings]); + const handleSettingsCloseWithNav = useCallback(() => { + removeNav(handleSettingsClose); + handleSettingsClose(); + }, [handleSettingsClose, removeNav]); + // Redirect to board if feature-gated views are disabled. useEffect(() => { if (!settingsLoaded) return; @@ -990,9 +998,10 @@ function AppInner() { // Auto-close nodes overlay if feature flag is toggled off while overlay is open useEffect(() => { if (nodesOpen && !nodesEnabled) { - setNodesOpen(false); + removeNav(closeNodes); + closeNodes(); } - }, [nodesOpen, nodesEnabled]); + }, [closeNodes, nodesOpen, nodesEnabled, removeNav]); const { availableModels, favoriteProviders, @@ -1133,16 +1142,21 @@ function AppInner() { setNodesOpen((prev) => !prev); }, [nodesEnabled]); + const closeNodesWithNav = useCallback(() => { + removeNav(closeNodes); + closeNodes(); + }, [closeNodes, removeNav]); + // History-aware nodes toggle — pushes nav entry only when opening const handleOpenNodesWithNav = useCallback(() => { if (!nodesEnabled) return; if (!nodesOpen) { setNodesOpen(true); - pushNav({ type: "view", revert: () => setNodesOpen(false) }); + pushNav({ type: "view", revert: closeNodes }); } else { - setNodesOpen(false); + closeNodesWithNav(); } - }, [nodesEnabled, nodesOpen, pushNav]); + }, [closeNodes, closeNodesWithNav, nodesEnabled, nodesOpen, pushNav]); // History-aware modal open handlers — push nav entries for back-navigation. const openDetailTask = useCallback((task: Task | TaskDetail, tab?: Parameters[1], opts?: { origin?: DetailTaskOrigin }) => { @@ -1195,9 +1209,10 @@ function AppInner() { modalManager.toggleTerminal(); pushNav({ type: "modal", close: modalManager.closeTerminal }); } else { + removeNav(modalManager.closeTerminal); modalManager.toggleTerminal(); } - }, [modalManager, pushNav]); + }, [modalManager, pushNav, removeNav]); const openFilesWithNav = useCallback((workspace?: string, initialFile?: string | null) => { modalManager.openFiles(workspace, initialFile); @@ -1408,7 +1423,7 @@ function AppInner() {
- setNodesOpen(false)} /> +
@@ -1804,7 +1819,7 @@ function AppInner() { const isRevalidating = projectsLoading || currentProjectLoading || isStale; return ( - + {isFirstEverBoot ? ( @@ -2108,7 +2123,7 @@ function AppInner() { taskOperations={{ moveTask, deleteTask, mergeTask, archiveTask, retryTask, resetTask, duplicateTask }} deepLink={{ handleDetailClose }} settings={{ prAuthAvailable, themeMode, colorTheme, dashboardFontScalePct, setThemeMode, setColorTheme, setDashboardFontScalePct }} - onSettingsClose={handleSettingsClose} + onSettingsClose={handleSettingsCloseWithNav} onReopenOnboarding={reopenOnboardingWithNav} onOpenApprovals={(_approvalId) => handleTaskViewChange("mailbox")} /> diff --git a/packages/dashboard/app/components/AppModals.tsx b/packages/dashboard/app/components/AppModals.tsx index 61805592a1..5bb9b13458 100644 --- a/packages/dashboard/app/components/AppModals.tsx +++ b/packages/dashboard/app/components/AppModals.tsx @@ -106,7 +106,7 @@ export function AppModals({ onReopenOnboarding, onOpenApprovals, }: AppModalsProps) { - const { pushNav } = useNavigationHistoryContext(); + const { pushNav, removeNav } = useNavigationHistoryContext(); const [firstCreatedTask, setFirstCreatedTask] = useState(null); const detailTask = modalManager.detailTask ? (() => { @@ -131,6 +131,101 @@ export function AppModals({ // Use the override handler if provided, otherwise fall back to modalManager.closeSettings const handleSettingsClose = onSettingsClose ?? modalManager.closeSettings; + const closeDetailWithNav = useCallback(() => { + removeNav(modalManager.closeDetailTask); + deepLink.handleDetailClose(); + }, [deepLink, modalManager.closeDetailTask, removeNav]); + + const closeGroupWithNav = useCallback(() => { + removeNav(modalManager.closeGroupModal); + modalManager.closeGroupModal(); + }, [modalManager.closeGroupModal, removeNav]); + + const closeSettingsWithNav = useCallback(() => { + removeNav(handleSettingsClose); + handleSettingsClose(); + }, [handleSettingsClose, removeNav]); + + const closeGitHubImportWithNav = useCallback(() => { + removeNav(modalManager.closeGitHubImport); + modalManager.closeGitHubImport(); + }, [modalManager.closeGitHubImport, removeNav]); + + const closePlanningWithNav = useCallback(() => { + removeNav(modalManager.closePlanning); + modalManager.closePlanning(); + }, [modalManager.closePlanning, removeNav]); + + const closeSubtaskWithNav = useCallback(() => { + removeNav(modalManager.closeSubtask); + modalManager.closeSubtask(); + }, [modalManager.closeSubtask, removeNav]); + + const closeTerminalWithNav = useCallback(() => { + removeNav(modalManager.closeTerminal); + modalManager.closeTerminal(); + }, [modalManager.closeTerminal, removeNav]); + + const closeScriptsWithNav = useCallback(() => { + removeNav(modalManager.closeScripts); + modalManager.closeScripts(); + }, [modalManager.closeScripts, removeNav]); + + const closeFilesWithNav = useCallback(() => { + removeNav(modalManager.closeFiles); + modalManager.closeFiles(); + }, [modalManager.closeFiles, removeNav]); + + const closeTodosWithNav = useCallback(() => { + removeNav(modalManager.closeTodos); + modalManager.closeTodos(); + }, [modalManager.closeTodos, removeNav]); + + const closeUsageWithNav = useCallback(() => { + removeNav(modalManager.closeUsage); + modalManager.closeUsage(); + }, [modalManager.closeUsage, removeNav]); + + const closeSystemStatsWithNav = useCallback(() => { + removeNav(modalManager.closeSystemStats); + modalManager.closeSystemStats(); + }, [modalManager.closeSystemStats, removeNav]); + + const closeSchedulesWithNav = useCallback(() => { + removeNav(modalManager.closeSchedules); + modalManager.closeSchedules(); + }, [modalManager.closeSchedules, removeNav]); + + const closeNewTaskWithNav = useCallback(() => { + removeNav(modalManager.closeNewTask); + modalManager.closeNewTask(); + }, [modalManager.closeNewTask, removeNav]); + + const closeActivityLogWithNav = useCallback(() => { + removeNav(modalManager.closeActivityLog); + modalManager.closeActivityLog(); + }, [modalManager.closeActivityLog, removeNav]); + + const closeGitManagerWithNav = useCallback(() => { + removeNav(modalManager.closeGitManager); + modalManager.closeGitManager(); + }, [modalManager.closeGitManager, removeNav]); + + const closeWorkflowEditorWithNav = useCallback(() => { + removeNav(modalManager.closeWorkflowEditor); + modalManager.closeWorkflowEditor(); + }, [modalManager.closeWorkflowEditor, removeNav]); + + const closeAgentsWithNav = useCallback(() => { + removeNav(modalManager.closeAgents); + modalManager.closeAgents(); + }, [modalManager.closeAgents, removeNav]); + + const closeSetupWizardWithNav = useCallback(() => { + removeNav(modalManager.closeSetupWizard); + modalManager.closeSetupWizard(); + }, [modalManager.closeSetupWizard, removeNav]); + const handleOpenNewTask = useCallback(() => { modalManager.openNewTask(); }, [modalManager]); @@ -190,7 +285,7 @@ export function AppModals({ task={detailTask} projectId={projectId} tasks={tasks} - onClose={deepLink.handleDetailClose} + onClose={closeDetailWithNav} onOpenDetail={openDetailTaskWithNav} mobileHeaderMode={modalManager.detailTaskOrigin === "list-mobile" ? "back" : "close"} onMoveTask={taskOperations.moveTask} @@ -213,7 +308,7 @@ export function AppModals({ { @@ -230,7 +325,7 @@ export function AppModals({ { - handleSettingsClose(); + closeSettingsWithNav(); modalManager.openWorkflowEditor("settings"); }} /> @@ -253,7 +348,7 @@ export function AppModals({ @@ -313,7 +408,7 @@ export function AppModals({ {modalManager.todosOpen && ( {modalManager.schedulesOpen && ( @@ -344,7 +439,7 @@ export function AppModals({ @@ -404,7 +499,7 @@ export function AppModals({ )} diff --git a/packages/dashboard/app/components/__tests__/AppModals.test.tsx b/packages/dashboard/app/components/__tests__/AppModals.test.tsx index bb4c71c44c..dcfe4ba272 100644 --- a/packages/dashboard/app/components/__tests__/AppModals.test.tsx +++ b/packages/dashboard/app/components/__tests__/AppModals.test.tsx @@ -558,10 +558,13 @@ describe("AppModals", () => { expect(mockSystemStatsModalProps).toHaveBeenCalledWith( expect.objectContaining({ isOpen: true, - onClose: closeSystemStats, + onClose: expect.any(Function), projectId: "proj-system", }), ); + + mockSystemStatsModalProps.mock.calls[0][0].onClose(); + expect(closeSystemStats).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/dashboard/app/components/__tests__/navigation-history.test.tsx b/packages/dashboard/app/components/__tests__/navigation-history.test.tsx index d580fe1a64..5369b8bbd3 100644 --- a/packages/dashboard/app/components/__tests__/navigation-history.test.tsx +++ b/packages/dashboard/app/components/__tests__/navigation-history.test.tsx @@ -9,7 +9,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen, fireEvent, waitFor, act } from "@testing-library/react"; -import type { Settings } from "@fusion/core"; +import type { Settings, Task } from "@fusion/core"; import type { ProjectInfo } from "../../api"; import { scopedKey } from "../../utils/projectStorage"; @@ -144,6 +144,18 @@ vi.mock("../../components/model-onboarding-state", () => ({ ONBOARDING_FLOW_STEPS: ["ai-setup", "github", "project-setup", "first-task"], })); +vi.mock("../../components/Board", () => ({ + Board: ({ tasks, onOpenDetail }: { tasks: Task[]; onOpenDetail: (task: Task) => void }) => ( +
+ {tasks.map((task) => ( + + ))} +
+ ), +})); + vi.mock("../../components/TaskDetailModal", () => ({ TaskDetailModal: ({ task, onClose }: { task: { id: string; title?: string }; onClose: () => void }) => (
@@ -305,6 +317,18 @@ vi.mock("../../hooks/useMobileKeyboard", () => ({ import { App } from "../../App"; +function makeTask(id: string, title: string): Task { + return { + id, + title, + description: "Test task description", + column: "todo", + status: "todo", + createdAt: new Date(0).toISOString(), + updatedAt: new Date(0).toISOString(), + } as Task; +} + function dispatchPopState(state: Record | null) { act(() => { window.dispatchEvent(new PopStateEvent("popstate", { state })); @@ -380,6 +404,14 @@ describe("Navigation history integration", () => { return result; } + async function renderMobileAppAndWait() { + const result = render(); + await waitFor(() => { + expect(screen.getByTestId("board-view")).toBeTruthy(); + }); + return result; + } + // 1. Desktop: opening Settings pushes a history entry it("pushes history entry when opening Settings modal on desktop", async () => { await renderAppAndWait(); @@ -481,6 +513,85 @@ describe("Navigation history integration", () => { }); }); + it("dismisses task detail on mobile popstate after a normal open", async () => { + mockUseViewportMode.mockReturnValue("mobile"); + const task = makeTask("FN-1", "Mobile Swipe Detail"); + mockUseTasks.mockImplementation(() => ({ + tasks: [task], + createTask: mockCreateTask, + moveTask: vi.fn(), + deleteTask: vi.fn(), + mergeTask: vi.fn(), + retryTask: vi.fn(), + updateTask: vi.fn(), + duplicateTask: vi.fn(), + archiveTask: vi.fn(), + unarchiveTask: vi.fn(), + archiveAllDone: vi.fn(), + })); + + await renderMobileAppAndWait(); + + fireEvent.click(screen.getByTestId("open-task-FN-1")); + + await waitFor(() => { + expect(screen.getByTestId("task-detail-modal")).toBeTruthy(); + }); + + dispatchPopState({ navIndex: 0 }); + + await waitFor(() => { + expect(screen.queryByTestId("task-detail-modal")).toBeNull(); + }); + }); + + it("dismisses task detail on mobile popstate after close and reopen", async () => { + mockUseViewportMode.mockReturnValue("mobile"); + const task = makeTask("FN-1", "Mobile Swipe Detail"); + mockUseTasks.mockImplementation(() => ({ + tasks: [task], + createTask: mockCreateTask, + moveTask: vi.fn(), + deleteTask: vi.fn(), + mergeTask: vi.fn(), + retryTask: vi.fn(), + updateTask: vi.fn(), + duplicateTask: vi.fn(), + archiveTask: vi.fn(), + unarchiveTask: vi.fn(), + archiveAllDone: vi.fn(), + })); + + await renderMobileAppAndWait(); + + fireEvent.click(screen.getByTestId("open-task-FN-1")); + + await waitFor(() => { + expect(screen.getByTestId("task-detail-modal")).toBeTruthy(); + }); + + fireEvent.click(screen.getByRole("button", { name: "Close" })); + // removeNav drives history.back(); consume the self-triggered popstate + // before reopening so the next popstate represents the user's swipe-back. + dispatchPopState({ navIndex: 0 }); + + await waitFor(() => { + expect(screen.queryByTestId("task-detail-modal")).toBeNull(); + }); + + fireEvent.click(screen.getByTestId("open-task-FN-1")); + + await waitFor(() => { + expect(screen.getByTestId("task-detail-modal")).toBeTruthy(); + }); + + dispatchPopState({ navIndex: 0 }); + + await waitFor(() => { + expect(screen.queryByTestId("task-detail-modal")).toBeNull(); + }); + }); + // 5. Verify useNavigationHistory is called with enabled=true on mobile it("calls useNavigationHistory with enabled=true on mobile", async () => { mockUseViewportMode.mockReturnValue("mobile"); diff --git a/packages/dashboard/app/hooks/__tests__/useNavigationHistory.test.ts b/packages/dashboard/app/hooks/__tests__/useNavigationHistory.test.ts index ff055e92c1..bfe086500a 100644 --- a/packages/dashboard/app/hooks/__tests__/useNavigationHistory.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useNavigationHistory.test.ts @@ -11,25 +11,30 @@ import { describe("useNavigationHistory", () => { const originalPushState = window.history.pushState; const originalReplaceState = window.history.replaceState; + const originalBack = window.history.back; let pushStateSpy: ReturnType; let replaceStateSpy: ReturnType; + let backSpy: ReturnType; beforeEach(() => { vi.clearAllMocks(); pushStateSpy = vi.fn(); replaceStateSpy = vi.fn(); + backSpy = vi.fn(); // Use real replaceState for setup so history.state is actually set, // then install spies for assertions. window.history.replaceState = originalReplaceState; window.history.replaceState({}, ""); window.history.pushState = pushStateSpy; window.history.replaceState = replaceStateSpy; + window.history.back = backSpy; }); afterEach(() => { window.history.pushState = originalPushState; window.history.replaceState = originalReplaceState; + window.history.back = originalBack; }); function renderHookWithHistory(enabled = true) { @@ -128,6 +133,98 @@ describe("useNavigationHistory", () => { expect(close1).not.toHaveBeenCalled(); }); + it("removeNav removes a matching entry and calls history.back", () => { + const close = vi.fn(); + const { result } = renderHookWithHistory(); + + act(() => { + result.current.pushNav({ type: "modal", close }); + }); + + act(() => { + result.current.removeNav(close); + }); + + expect(backSpy).toHaveBeenCalledTimes(1); + + dispatchPopState({ navIndex: 0 }); + + expect(close).not.toHaveBeenCalled(); + }); + + it("consumes the self-triggered popstate after removeNav without invoking callbacks", () => { + const close = vi.fn(); + const revert = vi.fn(); + const { result } = renderHookWithHistory(); + + act(() => { + result.current.pushNav({ type: "modal", close }); + result.current.pushNav({ type: "view", revert }); + result.current.removeNav(revert); + }); + + dispatchPopState({ navIndex: 1 }); + + expect(revert).not.toHaveBeenCalled(); + expect(close).not.toHaveBeenCalled(); + }); + + it("allows reopening with the same callback after removeNav", () => { + const close = vi.fn(); + const { result } = renderHookWithHistory(); + + act(() => { + result.current.pushNav({ type: "modal", close }); + result.current.removeNav(close); + }); + + dispatchPopState({ navIndex: 0 }); + + act(() => { + result.current.pushNav({ type: "modal", close }); + }); + + expect(pushStateSpy).toHaveBeenCalledTimes(2); + }); + + it("keeps stack order consistent after removing the top entry", () => { + const close1 = vi.fn(); + const close2 = vi.fn(); + const close3 = vi.fn(); + const { result } = renderHookWithHistory(); + + act(() => { + result.current.pushNav({ type: "modal", close: close1 }); + result.current.pushNav({ type: "modal", close: close2 }); + result.current.pushNav({ type: "modal", close: close3 }); + result.current.removeNav(close3); + }); + + dispatchPopState({ navIndex: 2 }); + dispatchPopState({ navIndex: 1 }); + + expect(close3).not.toHaveBeenCalled(); + expect(close2).toHaveBeenCalledTimes(1); + expect(close1).not.toHaveBeenCalled(); + }); + + it("removeNav is a no-op when the callback is not on the stack", () => { + const close = vi.fn(); + const absent = vi.fn(); + const { result } = renderHookWithHistory(); + + act(() => { + result.current.pushNav({ type: "modal", close }); + result.current.removeNav(absent); + }); + + expect(backSpy).not.toHaveBeenCalled(); + + dispatchPopState({ navIndex: 0 }); + + expect(close).toHaveBeenCalledTimes(1); + }); + // 5. replaceCurrent updates the top entry it("replaceCurrent updates the top entry", () => { const closeA = vi.fn(); @@ -325,6 +422,7 @@ describe("useNavigationHistory", () => { const value: UseNavigationHistoryResult = { pushNav: vi.fn(), replaceCurrent: vi.fn(), + removeNav: vi.fn(), }; const wrapper = ({ children }: { children: ReactNode }) => diff --git a/packages/dashboard/app/hooks/useNavigationHistory.ts b/packages/dashboard/app/hooks/useNavigationHistory.ts index 40851a2b9d..0524aff946 100644 --- a/packages/dashboard/app/hooks/useNavigationHistory.ts +++ b/packages/dashboard/app/hooks/useNavigationHistory.ts @@ -33,8 +33,15 @@ export interface UseNavigationHistoryResult { pushNav: (entry: NavEntry) => void; /** Replace the top-of-stack entry and call history.replaceState. */ replaceCurrent: (entry: NavEntry) => void; + /** + * Remove a programmatically-dismissed entry and call history.back so the + * browser history entry created by pushNav is consumed as well. + */ + removeNav: (closeOrRevert: () => void) => void; } +const SELF_POP_FALLBACK_CLEAR_MS = 1_000; + export const NavigationHistoryContext = createContext(null); export function NavigationHistoryProvider({ @@ -76,6 +83,12 @@ export function useNavigationHistory( // history entry. pushNav checks this flag and skips if true. const isPoppingRef = useRef(false); + // Guard flag: removeNav calls history.back() to consume the matching browser + // history entry after the UI has already closed. The resulting popstate must + // be ignored so callbacks are not invoked twice. + const selfPopRef = useRef(false); + const selfPopClearTimerRef = useRef(null); + // Keep enabled in a ref so the popstate handler can read the current value // without needing to be re-registered on every change. const enabledRef = useRef(enabled); @@ -128,6 +141,33 @@ export function useNavigationHistory( [], // stable — reads from refs ); + const removeNav = useCallback( + (closeOrRevert: () => void) => { + if (!enabledRef.current) return; + + for (let i = stackRef.current.length - 1; i >= 0; i -= 1) { + const entry = stackRef.current[i]; + const callback = entry.type === "modal" ? entry.close : entry.revert; + if (callback !== closeOrRevert) continue; + + stackRef.current.splice(i, 1); + selfPopRef.current = true; + + if (selfPopClearTimerRef.current !== null) { + window.clearTimeout(selfPopClearTimerRef.current); + } + selfPopClearTimerRef.current = window.setTimeout(() => { + selfPopRef.current = false; + selfPopClearTimerRef.current = null; + }, SELF_POP_FALLBACK_CLEAR_MS); + + window.history.back(); + return; + } + }, + [], // stable — reads from refs + ); + // Register popstate listener. Always registers in browser environments but // the handler checks enabledRef.current to skip when disabled (desktop). useEffect(() => { @@ -136,6 +176,15 @@ export function useNavigationHistory( const handlePopState = (event: PopStateEvent) => { if (!enabledRef.current) return; + if (selfPopRef.current) { + selfPopRef.current = false; + if (selfPopClearTimerRef.current !== null) { + window.clearTimeout(selfPopClearTimerRef.current); + selfPopClearTimerRef.current = null; + } + return; + } + const targetIndex = event.state?.navIndex ?? 0; const currentLength = stackRef.current.length; @@ -168,8 +217,12 @@ export function useNavigationHistory( window.addEventListener("popstate", handlePopState); return () => { window.removeEventListener("popstate", handlePopState); + if (selfPopClearTimerRef.current !== null) { + window.clearTimeout(selfPopClearTimerRef.current); + selfPopClearTimerRef.current = null; + } }; }, []); - return { pushNav, replaceCurrent }; + return { pushNav, replaceCurrent, removeNav }; }