Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 11 additions & 23 deletions src/features/instance/status/analytics/StatusTabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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' },
Expand All @@ -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);
Expand Down Expand Up @@ -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<string, unknown>);

// 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
Expand Down Expand Up @@ -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<AnalyticsContextValue>(() => {
const preset = getPreset(presetId);
const endTime = Date.now();
Expand Down Expand Up @@ -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];
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand All @@ -134,8 +137,6 @@ describe('StatusTabs URL sync', () => {
});
navigateMock.mockClear();
unmount();
expect(navigateMock).toHaveBeenCalledWith(
expect.objectContaining({ search: undefined, replace: true }),
);
expect(navigateMock).not.toHaveBeenCalled();
});
});
9 changes: 8 additions & 1 deletion src/features/instance/status/routes.ts
Original file line number Diff line number Diff line change
@@ -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<typeof createInstanceLayoutRoute>) {
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'),
});
Expand Down
35 changes: 35 additions & 0 deletions src/features/instance/status/statusSearch.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
49 changes: 49 additions & 0 deletions src/features/instance/status/statusSearch.ts
Original file line number Diff line number Diff line change
@@ -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<string, unknown>): 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,
};
}
66 changes: 66 additions & 0 deletions src/router/__tests__/statusSearchScope.test.ts
Original file line number Diff line number Diff line change
@@ -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({});
});
});