diff --git a/src/features/instance/status/analytics/StatusTabs.tsx b/src/features/instance/status/analytics/StatusTabs.tsx index 931a1efec..543be5995 100644 --- a/src/features/instance/status/analytics/StatusTabs.tsx +++ b/src/features/instance/status/analytics/StatusTabs.tsx @@ -6,10 +6,11 @@ import { ANALYTICS_QUERY_KEY_PREFIX } from '@/integrations/api/instance/status/g import { useQueryClient } from '@tanstack/react-query'; import { useNavigate, useSearch } from '@tanstack/react-router'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { type StatusTabId, validateStatusSearch } from '../statusSearch.ts'; import { AnalyticsOnboardingHint } from './components/AnalyticsOnboardingHint.tsx'; import { TimeRangePicker } from './components/TimeRangePicker.tsx'; import { type AnalyticsContextValue, AnalyticsProvider } from './context/AnalyticsContext.tsx'; -import { DEFAULT_PRESET_ID, DEFAULT_REFRESH_MS, getPreset, type TimePresetId } from './context/timePresets.ts'; +import { getPreset, type TimePresetId } from './context/timePresets.ts'; import { useAnalyticsCapability } from './hooks/useAnalyticsCapability.ts'; import { DatabaseTab } from './tabs/DatabaseTab.tsx'; import { HealthTab } from './tabs/HealthTab.tsx'; @@ -24,6 +25,8 @@ interface Props { isLocalStudio: boolean; } +// Labels for the tab ids the route's search schema defines; `satisfies` +// keeps this list and STATUS_TAB_IDS from drifting apart. const TAB_DEFS = [ { id: 'health', label: 'Health' }, { id: 'traffic', label: 'Traffic' }, @@ -32,9 +35,9 @@ const TAB_DEFS = [ { id: 'replication', label: 'Replication' }, { id: 'storage', label: 'Storage' }, { id: 'overview', label: 'Overview' }, -] as const; +] as const satisfies readonly { id: StatusTabId; label: string }[]; -type TabId = (typeof TAB_DEFS)[number]['id']; +type TabId = StatusTabId; export function StatusTabs({ instanceParams, isLocalStudio }: Props) { const capability = useAnalyticsCapability(instanceParams); @@ -69,14 +72,11 @@ export function StatusTabs({ instanceParams, isLocalStudio }: Props) { function StatusTabsInner({ instanceParams, isLocalStudio }: Props) { const navigate = useNavigate(); - const raw: { tab?: string; range?: string; refresh?: string | number } = useSearch({ strict: false }); - const tab: TabId = TAB_DEFS.some((t) => t.id === raw.tab) ? (raw.tab as TabId) : 'health'; - const presetId: TimePresetId = raw.range && VALID_PRESETS.includes(raw.range) - ? (raw.range as TimePresetId) - : DEFAULT_PRESET_ID; - const refreshMs: number = raw.refresh !== undefined && VALID_REFRESH.includes(Number(raw.refresh)) - ? Number(raw.refresh) - : DEFAULT_REFRESH_MS; + // The route's validateSearch already normalized these; re-validating here + // only narrows the `strict: false` typing to the same single source of + // truth (statusSearch.ts) instead of hand-rolled casts. + const raw = useSearch({ strict: false }); + const { tab, range: presetId, refresh: refreshMs } = validateStatusSearch(raw as Record); // The analytics window is a fixed [start, end] snapshot baked into every // panel's query key, so "refresh" means sliding the window forward to a @@ -136,15 +136,6 @@ function StatusTabsInner({ instanceParams, isLocalStudio }: Props) { void navigate({ to: '.', search: { tab, range: presetId, refresh: ms } }); }, [navigate, tab, presetId]); - // Strip our query params on unmount so they don't bleed into sibling - // routes (e.g. navigating from /status to /databases shouldn't carry - // tab/range/refresh forward). - useEffect(() => { - return () => { - void navigate({ search: undefined, replace: true }); - }; - }, [navigate]); - const ctxValue = useMemo(() => { const preset = getPreset(presetId); const endTime = Date.now(); @@ -261,6 +252,3 @@ function TabBody({ picker, children }: { picker: React.ReactNode; children: Reac ); } - -const VALID_PRESETS: readonly string[] = ['1h', '6h', '24h', '7d', '30d']; -const VALID_REFRESH: readonly number[] = [0, 30_000, 60_000, 300_000]; diff --git a/src/features/instance/status/analytics/__tests__/StatusTabs.url-sync.test.tsx b/src/features/instance/status/analytics/__tests__/StatusTabs.url-sync.test.tsx index 8df84585a..bb5b6ecfe 100644 --- a/src/features/instance/status/analytics/__tests__/StatusTabs.url-sync.test.tsx +++ b/src/features/instance/status/analytics/__tests__/StatusTabs.url-sync.test.tsx @@ -122,7 +122,10 @@ describe('StatusTabs URL sync', () => { }, { timeout: 2000 }); }); - it('clears search params on unmount so sibling routes do not inherit them', async () => { + it('does not navigate on unmount (params are route-scoped, #1440)', async () => { + // The old navigate-on-unmount cleanup fired after the destination route + // committed and could clobber its search params; scoping now lives on + // the route via validateSearch + stripSearchParams. currentSearch = { tab: 'requests', range: '24h' }; const { unmount } = mount(); await waitFor(() => { @@ -134,8 +137,6 @@ describe('StatusTabs URL sync', () => { }); navigateMock.mockClear(); unmount(); - expect(navigateMock).toHaveBeenCalledWith( - expect.objectContaining({ search: undefined, replace: true }), - ); + expect(navigateMock).not.toHaveBeenCalled(); }); }); diff --git a/src/features/instance/status/routes.ts b/src/features/instance/status/routes.ts index aa08eb859..d3d208472 100644 --- a/src/features/instance/status/routes.ts +++ b/src/features/instance/status/routes.ts @@ -1,10 +1,17 @@ import { createInstanceLayoutRoute } from '@/features/instance/instanceLayoutRoute'; -import { createRoute, lazyRouteComponent } from '@tanstack/react-router'; +import { STATUS_SEARCH_DEFAULTS, validateStatusSearch } from '@/features/instance/status/statusSearch.ts'; +import { createRoute, lazyRouteComponent, stripSearchParams } from '@tanstack/react-router'; export function createStatusRouteTree(instanceLayoutRoute: ReturnType) { const instanceConfigRoute = createRoute({ getParentRoute: () => instanceLayoutRoute, path: 'status', + // tab/range/refresh are validated (and defaulted) here so they are + // scoped to this route declaratively — sibling routes never see them, + // and StatusTabs needs no imperative cleanup on unmount. The strip + // middleware keeps default values out of the URL. + validateSearch: validateStatusSearch, + search: { middlewares: [stripSearchParams(STATUS_SEARCH_DEFAULTS)] }, head: () => ({ meta: [{ title: 'Status — Harper Fabric' }] }), component: lazyRouteComponent(async () => import('@/features/instance/status/index'), 'StatusIndex'), }); diff --git a/src/features/instance/status/statusSearch.test.ts b/src/features/instance/status/statusSearch.test.ts new file mode 100644 index 000000000..69bcffe9e --- /dev/null +++ b/src/features/instance/status/statusSearch.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vitest'; +import { STATUS_SEARCH_DEFAULTS, validateStatusSearch } from './statusSearch.ts'; + +describe('validateStatusSearch', () => { + it('passes through valid values', () => { + expect(validateStatusSearch({ tab: 'traffic', range: '6h', refresh: 30_000 })).toEqual({ + tab: 'traffic', + range: '6h', + refresh: 30_000, + }); + }); + + it('falls back to defaults for missing values', () => { + expect(validateStatusSearch({})).toEqual(STATUS_SEARCH_DEFAULTS); + }); + + it('falls back to defaults for invalid values', () => { + expect(validateStatusSearch({ tab: 'garbage', range: '999y', refresh: 1234 })).toEqual( + STATUS_SEARCH_DEFAULTS, + ); + }); + + it('coerces a string refresh (URL-sourced) to its numeric option', () => { + expect(validateStatusSearch({ refresh: '30000' }).refresh).toBe(30_000); + }); + + it('treats an empty-string refresh as missing, not as 0/Off', () => { + expect(validateStatusSearch({ refresh: '' }).refresh).toBe(STATUS_SEARCH_DEFAULTS.refresh); + }); + + it('keeps refresh=0 (auto-refresh off) instead of defaulting it', () => { + expect(validateStatusSearch({ refresh: 0 }).refresh).toBe(0); + expect(validateStatusSearch({ refresh: '0' }).refresh).toBe(0); + }); +}); diff --git a/src/features/instance/status/statusSearch.ts b/src/features/instance/status/statusSearch.ts new file mode 100644 index 000000000..dcb6d010c --- /dev/null +++ b/src/features/instance/status/statusSearch.ts @@ -0,0 +1,49 @@ +import { + DEFAULT_PRESET_ID, + DEFAULT_REFRESH_MS, + REFRESH_OPTIONS, + TIME_PRESETS, + type TimePresetId, +} from './analytics/context/timePresets.ts'; + +/** Search-param schema for the instance Status route (`?tab&range&refresh`). + * Lives outside analytics/ so the route definition can validate search + * without pulling the lazy-loaded analytics bundle into the main chunk. */ + +export const STATUS_TAB_IDS = [ + 'health', + 'traffic', + 'requests', + 'database', + 'replication', + 'storage', + 'overview', +] as const; +export type StatusTabId = (typeof STATUS_TAB_IDS)[number]; + +export interface StatusSearch { + tab: StatusTabId; + range: TimePresetId; + refresh: number; +} + +export const STATUS_SEARCH_DEFAULTS: StatusSearch = { + tab: 'health', + range: DEFAULT_PRESET_ID, + refresh: DEFAULT_REFRESH_MS, +}; + +/** Normalizes raw search params to a valid `StatusSearch`; missing or invalid + * values fall back to defaults rather than erroring, so hand-edited or stale + * deep links degrade gracefully. */ +export function validateStatusSearch(search: Record): StatusSearch { + // `Number('')` is 0, which would silently match the "Off" refresh option — + // treat an empty string like a missing param instead. + const refreshValue = search.refresh === '' ? Number.NaN : Number(search.refresh); + return { + tab: STATUS_TAB_IDS.find((t) => t === search.tab) ?? STATUS_SEARCH_DEFAULTS.tab, + range: TIME_PRESETS.find((p) => p.id === search.range)?.id ?? STATUS_SEARCH_DEFAULTS.range, + refresh: REFRESH_OPTIONS.find((o) => o.value === refreshValue)?.value + ?? STATUS_SEARCH_DEFAULTS.refresh, + }; +} diff --git a/src/router/__tests__/statusSearchScope.test.ts b/src/router/__tests__/statusSearchScope.test.ts new file mode 100644 index 000000000..b6a6bda5c --- /dev/null +++ b/src/router/__tests__/statusSearchScope.test.ts @@ -0,0 +1,66 @@ +// @vitest-environment jsdom +import { rootRouteTree } from '@/router/rootRouteTree'; +import { QueryClient } from '@tanstack/react-query'; +import { createMemoryHistory, createRouter } from '@tanstack/react-router'; +import { describe, expect, it } from 'vitest'; + +// Regression coverage for #1440: the status tab/range/refresh search params +// used to be validated by hand inside StatusTabs and cleaned up with a +// navigate-on-unmount effect that could clobber the destination route's own +// search params. They are now declared on the status route (validateSearch + +// stripSearchParams), so the router scopes and normalizes them declaratively. +// Like orgUndefinedRepro.test.ts, these tests use matchRoutes/buildLocation — +// no loaders, no network. + +const INSTANCE_BASE = '/org-qpz5akmyrp1d0opj/clu-tc9pqw20vrks2zik/instance/ins-abc123'; +const STATUS_PATH = `${INSTANCE_BASE}/status`; + +function makeRouter(initialEntry: string) { + return createRouter({ + routeTree: rootRouteTree, + history: createMemoryHistory({ initialEntries: [initialEntry] }), + context: { queryClient: new QueryClient(), authentication: {} }, + }); +} + +describe('status route search-param scoping', () => { + it('validates deep-linked search params on the status route', () => { + const router = makeRouter(`${STATUS_PATH}?tab=traffic&range=6h`); + const leaf = router.matchRoutes(router.state.location).at(-1)!; + expect(leaf.routeId).toContain('/status'); + expect(leaf.search).toEqual({ tab: 'traffic', range: '6h', refresh: 60_000 }); + }); + + it('normalizes invalid deep-link values to defaults instead of erroring', () => { + const router = makeRouter(`${STATUS_PATH}?tab=garbage&range=999y&refresh=17`); + const leaf = router.matchRoutes(router.state.location).at(-1)!; + expect(leaf.search).toEqual({ tab: 'health', range: '1h', refresh: 60_000 }); + }); + + it('strips default values from status URLs', () => { + const router = makeRouter(STATUS_PATH); + const loc = router.buildLocation({ + to: STATUS_PATH, + search: { tab: 'health', range: '1h', refresh: 60_000 }, + }); + expect(loc.searchStr).toBe(''); + }); + + it('keeps only non-default values in status URLs', () => { + const router = makeRouter(STATUS_PATH); + const loc = router.buildLocation({ + to: STATUS_PATH, + search: { tab: 'traffic', range: '1h', refresh: 60_000 }, + }); + expect(loc.searchStr).toBe('?tab=traffic'); + }); + + it('does not carry status params onto sibling routes', () => { + // From a status location with non-default params, a plain nav-bar-style + // navigation (no `search`) must land on a clean sibling URL. + const router = makeRouter(`${STATUS_PATH}?tab=traffic&range=6h`); + const loc = router.buildLocation({ to: `${INSTANCE_BASE}/logs` }); + expect(loc.searchStr).toBe(''); + expect(loc.search).toEqual({}); + }); +});