diff --git a/apps/studio/TANSTACK_MIGRATION.md b/apps/studio/TANSTACK_MIGRATION.md index a120561fce4..c45e3eb052e 100644 --- a/apps/studio/TANSTACK_MIGRATION.md +++ b/apps/studio/TANSTACK_MIGRATION.md @@ -455,10 +455,9 @@ inline. `router.subscribe(tsEvent, …)`. Forwards Next's `(url, { shallow })` args. Maps `routeChangeStart` / `routeChangeComplete` / `beforeHistoryChange` / `hashChangeStart` / `hashChangeComplete`. - **Known gap:** Next's throw-from-`routeChangeStart`-to-cancel pattern - isn't supportable — `subscribe` is fire-and-forget. - `usePreventNavigationOnUnsavedChanges` relies on it and needs - migrating to TanStack's `useBlocker` separately. + **Cancellation:** Next's throw-from-`routeChangeStart`-to-cancel pattern + isn't supportable because `subscribe` is fire-and-forget. + `usePreventNavigationOnUnsavedChanges` blocks through TanStack history instead. - `api.ts` — `toWebHandler(nextHandler)`. See **API routes → Shim coverage** above. - `link.tsx`, `navigation.ts`, `dynamic.tsx`, `image.tsx`, @@ -604,7 +603,7 @@ for the Vite pipeline: - Switch `routes/index.tsx` redirects from `href` to `to` — all targets now live in the TanStack tree. -- Migrate `usePreventNavigationOnUnsavedChanges` from `router.events.on('routeChangeStart', …)` (throw-to-cancel pattern) to TanStack's `useBlocker`. +- [x] Make `usePreventNavigationOnUnsavedChanges` block through TanStack history while retaining the Next implementation for the legacy runtime. - Drop the `_splat` / `routeSlug` normalisation block from `pages/org/_/[[...routeSlug]].tsx` + `pages/project/_/[[...routeSlug]].tsx` (only there to keep both runtimes mounting the same body). - Remove `RouteValidationWrapper` + `next/router` compat shim usage from `__root.tsx`. diff --git a/apps/studio/compat/next/_router-events.ts b/apps/studio/compat/next/_router-events.ts index 87c9d3ca83d..71414397e97 100644 --- a/apps/studio/compat/next/_router-events.ts +++ b/apps/studio/compat/next/_router-events.ts @@ -14,11 +14,9 @@ // { fromLocation, toLocation, pathChanged, hrefChanged, hashChanged } // — we forward `toLocation.href` as the URL arg. // -// Known gap: Next's `routeChangeStart` lets handlers throw to cancel the -// navigation. TanStack's `subscribe` is fire-and-forget; cancellation -// requires `useBlocker` instead. `usePreventNavigationOnUnsavedChanges` -// relies on the throw-to-cancel pattern and will need migrating to -// `useBlocker` separately. +// Next's `routeChangeStart` lets handlers throw to cancel navigation. TanStack's +// `subscribe` is fire-and-forget, so the shared unsaved-changes hook blocks through +// TanStack history before navigation begins instead. // eslint-disable-next-line @typescript-eslint/no-explicit-any type AnyRouter = any diff --git a/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.test.ts b/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.test.ts index 4b8da45b61f..6c5c4114382 100644 --- a/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.test.ts +++ b/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.test.ts @@ -1,32 +1,120 @@ -import { act, renderHook } from '@testing-library/react' +import { act, renderHook, waitFor } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { usePreventNavigationOnUnsavedChanges } from './usePreventNavigationOnUnsavedChanges' -const routerEvents = { - on: vi.fn(), - off: vi.fn(), -} +const mocks = vi.hoisted(() => ({ + tanStackRouter: undefined as undefined | { history: { block: ReturnType } }, + nextEvents: { on: vi.fn(), off: vi.fn() }, + nextPush: vi.fn(), +})) + +vi.mock('@tanstack/react-router', () => ({ + useRouter: () => mocks.tanStackRouter, +})) vi.mock('next/router', () => ({ - useRouter: () => ({ events: routerEvents, push: vi.fn() }), + useRouter: () => ({ events: mocks.nextEvents, push: mocks.nextPush }), })) describe('usePreventNavigationOnUnsavedChanges', () => { beforeEach(() => { - routerEvents.on.mockClear() - routerEvents.off.mockClear() + vi.clearAllMocks() + mocks.tanStackRouter = undefined }) - it('bypasses only the next intentional navigation', () => { - const { result } = renderHook(() => usePreventNavigationOnUnsavedChanges({ hasChanges: true })) - const routeChangeHandler = routerEvents.on.mock.calls.find( + it('retains the legacy Next navigation guard', () => { + renderHook(() => usePreventNavigationOnUnsavedChanges({ hasChanges: true })) + const routeChangeHandler = mocks.nextEvents.on.mock.calls.find( ([event]) => event === 'routeChangeStart' )?.[1] - act(() => result.current.bypassNavigationGuard()) - - expect(() => act(() => routeChangeHandler('/replication'))).not.toThrow() expect(() => act(() => routeChangeHandler('/settings'))).toThrow('Route change declined') }) + + it('blocks TanStack navigation before the route changes and can cancel it', async () => { + let blockerFn: (() => Promise) | undefined + const unblock = vi.fn() + mocks.tanStackRouter = { + history: { + block: vi.fn(({ blockerFn: nextBlockerFn }) => { + blockerFn = nextBlockerFn + return unblock + }), + }, + } + + const { result } = renderHook(() => usePreventNavigationOnUnsavedChanges({ hasChanges: true })) + let navigationResult: Promise + act(() => { + navigationResult = blockerFn!() + }) + + await waitFor(() => expect(result.current.shouldConfirmNavigation).toBe(true)) + act(() => result.current.handleCancelNavigation()) + + let resolvedNavigation: boolean | undefined + await act(async () => { + resolvedNavigation = await navigationResult! + }) + expect(resolvedNavigation).toBe(true) + await waitFor(() => expect(result.current.shouldConfirmNavigation).toBe(false)) + expect(mocks.nextEvents.on).not.toHaveBeenCalled() + }) + + it('allows confirmed TanStack navigation to proceed', async () => { + let blockerFn: (() => Promise) | undefined + mocks.tanStackRouter = { + history: { + block: vi.fn(({ blockerFn: nextBlockerFn }) => { + blockerFn = nextBlockerFn + return vi.fn() + }), + }, + } + + const { result } = renderHook(() => usePreventNavigationOnUnsavedChanges({ hasChanges: true })) + let navigationResult: Promise + act(() => { + navigationResult = blockerFn!() + }) + + await waitFor(() => expect(result.current.shouldConfirmNavigation).toBe(true)) + act(() => result.current.handleConfirmNavigation()) + + let resolvedNavigation: boolean | undefined + await act(async () => { + resolvedNavigation = await navigationResult! + }) + expect(resolvedNavigation).toBe(false) + }) + + it('bypasses only the next intentional TanStack navigation', async () => { + let blockerFn: (() => Promise) | undefined + mocks.tanStackRouter = { + history: { + block: vi.fn(({ blockerFn: nextBlockerFn }) => { + blockerFn = nextBlockerFn + return vi.fn() + }), + }, + } + + const { result } = renderHook(() => usePreventNavigationOnUnsavedChanges({ hasChanges: true })) + act(() => result.current.bypassNavigationGuard()) + + await expect(blockerFn!()).resolves.toBe(false) + + let secondNavigation: Promise + act(() => { + secondNavigation = blockerFn!() + }) + await waitFor(() => expect(result.current.shouldConfirmNavigation).toBe(true)) + act(() => result.current.handleCancelNavigation()) + let resolvedNavigation: boolean | undefined + await act(async () => { + resolvedNavigation = await secondNavigation! + }) + expect(resolvedNavigation).toBe(true) + }) }) diff --git a/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.ts b/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.ts index 69823950d2c..c38ef6e5d74 100644 --- a/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.ts +++ b/apps/studio/hooks/ui/usePreventNavigationOnUnsavedChanges.ts @@ -1,53 +1,72 @@ -import { useRouter } from 'next/router' +import { useRouter as useTanStackRouter } from '@tanstack/react-router' +import { useRouter as useNextRouter } from 'next/router' import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { BASE_PATH } from '@/lib/constants' interface UsePreventNavigationOnUnsavedChangesOptions { - /* - * Boolean indicating whether there are changes that would be lost if users navigate to another - * page or close the browser tab - */ hasChanges: boolean } interface UsePreventNavigationOnUnsavedChangesReturn { - /* - * Cancel the navigation and keep the changes - */ handleCancelNavigation: () => void - /* - * Confirm the navigation and lose the changes - */ handleConfirmNavigation: () => void - /* - * Skip the guard for the next intentional navigation after confirmation was handled elsewhere. - */ bypassNavigationGuard: () => void - /* - * Boolean indicating whether UI to request users confirmation for the navigation should be - * displayed - */ shouldConfirmNavigation: boolean } +interface PendingTanStackNavigation { + proceed: () => void + reset: () => void +} + /* - * Hook that prevents navigation when users could lose their changes. - * It prevents both NextJS and browser navigation (such as when closing the tab) + * Prevents in-app navigation and tab close when users could lose changes. + * + * Studio ships Next and TanStack Router side by side. TanStack navigation must be blocked through + * its history API before the route starts rendering; Next continues to use routeChangeStart. */ export const usePreventNavigationOnUnsavedChanges = ({ hasChanges, }: UsePreventNavigationOnUnsavedChangesOptions): UsePreventNavigationOnUnsavedChangesReturn => { - const router = useRouter() + const nextRouter = useNextRouter() + const tanStackRouter = useTanStackRouter({ warn: false }) const [navigateUrl, setNavigateUrl] = useState() const [confirmNavigate, setConfirmNavigate] = useState(false) + const [pendingTanStackNavigation, setPendingTanStackNavigation] = + useState() const bypassNavigationGuardRef = useRef(false) useEffect(() => { - const handleBeforeUnload = (e: BeforeUnloadEvent) => { + if (!tanStackRouter || !hasChanges) return + + return tanStackRouter.history.block({ + enableBeforeUnload: true, + blockerFn: async () => { + if (bypassNavigationGuardRef.current) { + bypassNavigationGuardRef.current = false + return false + } + + const shouldCancelNavigation = await new Promise((resolve) => { + setPendingTanStackNavigation({ + proceed: () => resolve(false), + reset: () => resolve(true), + }) + }) + setPendingTanStackNavigation(undefined) + return shouldCancelNavigation + }, + }) + }, [hasChanges, tanStackRouter]) + + useEffect(() => { + if (tanStackRouter) return + + const handleBeforeUnload = (event: BeforeUnloadEvent) => { if (hasChanges) { - e.preventDefault() - e.returnValue = '' // deprecated, but older browsers still require this + event.preventDefault() + event.returnValue = '' } } @@ -60,26 +79,31 @@ export const usePreventNavigationOnUnsavedChanges = ({ if (hasChanges && !confirmNavigate) { setNavigateUrl(url) - throw 'Route change declined' // Just to prevent the route change - return + throw 'Route change declined' } setNavigateUrl(undefined) } + window.addEventListener('beforeunload', handleBeforeUnload) - router.events.on('routeChangeStart', handleBrowseAway) + nextRouter.events.on('routeChangeStart', handleBrowseAway) return () => { window.removeEventListener('beforeunload', handleBeforeUnload) - router.events.off('routeChangeStart', handleBrowseAway) + nextRouter.events.off('routeChangeStart', handleBrowseAway) } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [confirmNavigate, hasChanges]) + }, [confirmNavigate, hasChanges, nextRouter.events, tanStackRouter]) const handleCancelNavigation = useCallback(() => { + pendingTanStackNavigation?.reset() setNavigateUrl(undefined) - }, []) + }, [pendingTanStackNavigation]) const handleConfirmNavigation = useCallback(() => { + if (pendingTanStackNavigation) { + pendingTanStackNavigation.proceed() + return + } + setConfirmNavigate(true) let urlToNavigate = navigateUrl ?? '/' if (BASE_PATH && urlToNavigate.startsWith(BASE_PATH)) { @@ -87,8 +111,8 @@ export const usePreventNavigationOnUnsavedChanges = ({ } if (!urlToNavigate.startsWith('/')) urlToNavigate = `/${urlToNavigate}` setNavigateUrl(undefined) - router.push(urlToNavigate) - }, [navigateUrl, router]) + nextRouter.push(urlToNavigate) + }, [navigateUrl, nextRouter, pendingTanStackNavigation]) const bypassNavigationGuard = useCallback(() => { bypassNavigationGuardRef.current = true @@ -99,8 +123,14 @@ export const usePreventNavigationOnUnsavedChanges = ({ handleCancelNavigation, handleConfirmNavigation, bypassNavigationGuard, - shouldConfirmNavigation: !!navigateUrl, + shouldConfirmNavigation: pendingTanStackNavigation !== undefined || navigateUrl !== undefined, }), - [navigateUrl, handleCancelNavigation, handleConfirmNavigation, bypassNavigationGuard] + [ + handleCancelNavigation, + handleConfirmNavigation, + bypassNavigationGuard, + navigateUrl, + pendingTanStackNavigation, + ] ) }