diff --git a/AGENTS.md b/AGENTS.md index 7da32d029..ab6f10b4f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,6 +178,34 @@ and collapses. Fix: make the wrapper `flex flex-col` (or add `block`/`w-full` to anchor). The GitHub button (`.github-signin-btn`) sets `display: flex` explicitly and is not affected. (Hit while building the "Last used" sign-in badge, #1316.) +## A mocked hook whose value lands in a dependency array must keep a stable identity + +If you `vi.mock` a hook and the code under test puts its return value in a `useEffect` dependency +array, returning a fresh object per call re-fires that effect on **every render** — and any +assertion counting effect side-effects then passes for the wrong reason. `vi.mock('@tanstack/ +react-router', () => ({ useRouter: () => ({ ... }) }))` did exactly that to +[`datadog.test.tsx`](src/integrations/datadog/datadog.test.tsx): the test that claimed to prove +"one view per navigation" would have passed with `location.href` removed from the deps entirely. +The real `useRouter` returns a stable reference, so the mock was also lying about production. + +Instantiate once in the factory and expose changing state through a getter: + +```ts +vi.mock('@tanstack/react-router', () => { + const router = { + get state() { + return { location: { href: routerState.href } }; + }, + }; + return { useRouter: () => router }; +}); +``` + +The general check, worth running on any effect-counting test: **re-render without changing +anything and assert nothing happened.** That assertion is what distinguishes "the effect re-ran +because its input changed" from "the effect re-runs constantly"; it fails immediately against an +unstable mock. + ## Testing Radix menus in jsdom This repo has NO `@testing-library/user-event` — only `@testing-library/react` + @@ -383,6 +411,47 @@ Only fields on the SDK's modifiable-field allowlist can be mutated here. `view.n `error.fingerprint`; resource events add `resource.url` (plus GraphQL/header/websocket fields). Grep the installed bundle for `"view.referrer":"string"` to re-check after an SDK bump. +## Datadog RUM — exactly one `startView` per page load, or Core Web Vitals vanish + +[`datadog.ts`](src/integrations/datadog/datadog.ts) sets `trackViewsManually: true`, and the SDK's +contract for that flag is narrower than it looks. It stays stopped until the **first** `startView`, +adopts that call's options as its single `initial_load` view, then turns every later call into a +`route_change` view (`preStartRum.ts` `tryStartRum`, `trackViews.ts` `startView`). Only an +`initial_load` view runs `trackInitialViewMetrics`, so it is the only view that can ever carry LCP +or FCP. + +A second `startView` on boot therefore ends the one view that collects paint metrics. That is what +zeroed Studio's vitals for a month (#1570): `useDatadog` and `useOnRouteLoadTracker` both called +it, 0.3ms apart, and every `initial_load` event shipped with `dom_complete` but no `lcp` and no +`fcp`. Ownership of that first call now lives in `useOnRouteLoadTracker` alone — it mounts on the +root route (`rootRoute.ts`), so it runs on every cloud route. + +Two traps when reading this from RUM data: + +- `view.time_spent` on an `initial_load` view is measured from `clocksOrigin()` — page origin, not + SDK start — so a ~1s `time_spent` does **not** mean the view was alive and observing for a + second. It says nothing about the observation window. +- Never name a view from `window.location.pathname`. Studio uses hash routing, so that is + permanently `/` — which is why all 880 `initial_load` views in one week were named `/` whatever + route actually loaded, and why #1405's "`/` view regression" was really every deep-link entry + conflated into one bucket. + +A boot-time redirect does **not** cost you that first view, and the deciding factor is whether +`getAllConnections()` can answer synchronously. It synthesizes `{ user: null, isLoading: false }` +for `OverallAppSignIn` only when the `Studio:PotentiallyAuthenticated` localStorage record has no +entry for it; with an entry it returns the record untouched, so the key is simply **absent**. +`dashboardLayout.beforeLoad` guards on `auth && !auth.isLoading && !auth.user`, so those are two +different outcomes: a signed-out deep link redirects _during_ the router's initial load and the root +component never commits the deep-link location — one `startView`, named `/sign-in/` — while an +expired session short-circuits on the missing key, renders the deep link, and only redirects once +auth resolves and `AppRouted` calls `router.invalidate()`. Two views there, a network round trip +apart, the second a genuine navigation. + +Verifying a change here needs a **visible** browser on a production build: a headless or background +tab reports `visibilityState: 'hidden'`, emits zero paint and LCP entries, and `trackFirstHidden` +discards them anyway — so vitals always read as absent, and a broken fix looks identical to a +working one. + ## Never put an address or a credential in a URL Studio uses **hash routing**, so a query param lives in the fragment: for diff --git a/src/integrations/datadog/datadog.test.tsx b/src/integrations/datadog/datadog.test.tsx new file mode 100644 index 000000000..12b9ab0eb --- /dev/null +++ b/src/integrations/datadog/datadog.test.tsx @@ -0,0 +1,123 @@ +/** + * @vitest-environment jsdom + */ +import { cleanup, render } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { rum, routerState } = vi.hoisted(() => ({ + rum: { + init: vi.fn(), + startView: vi.fn(), + onReady: vi.fn((callback: () => void) => callback()), + setUser: vi.fn(), + clearUser: vi.fn(), + addAction: vi.fn(), + }, + routerState: { href: '/org-1/clu-2/apps', params: [{ organizationId: 'org-1', clusterId: 'clu-2' }] }, +})); + +vi.mock('@datadog/browser-rum', () => ({ datadogRum: rum })); +vi.mock('@datadog/browser-rum-react', () => ({ reactPlugin: () => ({ name: 'react' }) })); +vi.mock('@/config/constants', () => ({ isLocalStudio: false })); +vi.mock('@/hooks/useAuth', () => ({ useOverallAuth: () => ({ user: undefined }) })); +// One stable `router` identity, as the real `useRouter` returns — the tracker's effect lists +// `router` in its deps, so a fresh object per render would re-fire it on every render and the +// navigation assertions below would hold even if `location.href` were not a dependency. +vi.mock('@tanstack/react-router', () => { + const router = { + get state() { + return { location: { href: routerState.href } }; + }, + matchRoutes: () => routerState.params.map((params) => ({ params })), + }; + return { + useLocation: () => ({ href: routerState.href }), + useRouter: () => router, + }; +}); + +async function loadDatadogModule() { + vi.resetModules(); + // `enabled` is a module-scope const, so DEV has to be false before the import. + vi.stubEnv('DEV', false); + return import('./datadog'); +} + +beforeEach(() => { + routerState.href = '/org-1/clu-2/apps'; + routerState.params = [{ organizationId: 'org-1', clusterId: 'clu-2' }]; +}); + +afterEach(() => { + cleanup(); + vi.unstubAllEnvs(); + vi.clearAllMocks(); +}); + +describe('Datadog view tracking', () => { + it('starts exactly one view when the whole tree boots', async () => { + const { useDatadog, useOnRouteLoadTracker } = await loadDatadogModule(); + function CloudRoot() { + useOnRouteLoadTracker(); + return null; + } + function App() { + useDatadog(); + return ; + } + + render(); + + expect(rum.init).toHaveBeenCalledTimes(1); + expect(rum.startView).toHaveBeenCalledTimes(1); + }); + + it("names that view by route, not by the hash router's pathname", async () => { + const { useDatadog, useOnRouteLoadTracker } = await loadDatadogModule(); + function CloudRoot() { + useOnRouteLoadTracker(); + return null; + } + function App() { + useDatadog(); + return ; + } + + render(); + + expect(rum.startView).toHaveBeenCalledWith( + expect.objectContaining({ name: '/$organizationId/$clusterId/apps/' }), + ); + }); + + it('does not start a view from useDatadog', async () => { + const { useDatadog } = await loadDatadogModule(); + function Harness() { + useDatadog(); + return null; + } + + render(); + + expect(rum.startView).not.toHaveBeenCalled(); + }); + + it('starts a further view on each subsequent navigation', async () => { + const { useOnRouteLoadTracker } = await loadDatadogModule(); + function CloudRoot() { + useOnRouteLoadTracker(); + return null; + } + + const { rerender } = render(); + rerender(); + routerState.href = '/org-1/clu-2/config'; + rerender(); + + // The middle re-render changed nothing, so it must not have produced a view. + expect(rum.startView.mock.calls.map(([options]) => options.name)).toEqual([ + '/$organizationId/$clusterId/apps/', + '/$organizationId/$clusterId/config/', + ]); + }); +}); diff --git a/src/integrations/datadog/datadog.ts b/src/integrations/datadog/datadog.ts index 8dd2e87df..ec67e6863 100644 --- a/src/integrations/datadog/datadog.ts +++ b/src/integrations/datadog/datadog.ts @@ -42,13 +42,8 @@ export function useDatadog() { plugins: [reactPlugin()], }); - datadogRum.onReady(() => { - datadogRum.startView({ - service: 'studio', - version: import.meta.env.VITE_STUDIO_VERSION, - name: window.location.pathname || 'initial', - }); - }); + // No `startView` here: `useOnRouteLoadTracker` must own the sole boot view, because + // only the first one is an `initial_load` view and only that view collects LCP/FCP. } }, []); } diff --git a/src/integrations/datadog/datadogBootView.test.tsx b/src/integrations/datadog/datadogBootView.test.tsx new file mode 100644 index 000000000..129fee457 --- /dev/null +++ b/src/integrations/datadog/datadogBootView.test.tsx @@ -0,0 +1,73 @@ +/** + * @vitest-environment jsdom + */ +import { QueryClient } from '@tanstack/react-query'; +import { render, waitFor } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const { rum } = vi.hoisted(() => ({ + rum: { + init: vi.fn(), + startView: vi.fn(), + onReady: vi.fn((callback: () => void) => callback()), + setUser: vi.fn(), + clearUser: vi.fn(), + addAction: vi.fn(), + }, +})); + +vi.mock('@datadog/browser-rum', () => ({ datadogRum: rum })); +vi.mock('@datadog/browser-rum-react', () => ({ reactPlugin: () => ({ name: 'react' }) })); +vi.mock('@/features/notifications/NotificationsSubscriptionManager', () => ({ + NotificationsSubscriptionManager: () => null, +})); +vi.mock('@/components/NotificationBanner', () => ({ NotificationBanner: () => null })); + +// Real `rootRoute` and real `dashboardLayout`, stub leaves: mocking either would stop this from +// guarding the one thing it exists for — that the tracker is still mounted at the root. +async function bootDeepLink() { + vi.resetModules(); + vi.stubEnv('DEV', false); + const router = await import('@tanstack/react-router'); + const { rootRoute } = await import('@/router/rootRoute'); + const { dashboardLayout } = await import('@/router/dashboardRoute'); + const { OverallAppSignIn } = await import('@/features/auth/store/authStore'); + + const deepRoute = router.createRoute({ + getParentRoute: () => dashboardLayout, + path: '/$organizationId/$clusterId/apps', + component: () => null, + }); + const signInRoute = router.createRoute({ + getParentRoute: () => rootRoute, + path: '/sign-in', + component: () => null, + }); + const instance = router.createRouter({ + routeTree: rootRoute.addChildren([signInRoute, dashboardLayout.addChildren([deepRoute])]), + history: router.createMemoryHistory({ initialEntries: ['/org-1/clu-2/apps'] }), + context: { + queryClient: new QueryClient(), + authentication: { [OverallAppSignIn]: { isLoading: false, user: null } }, + }, + }); + + render(); + return instance; +} + +afterEach(() => { + vi.unstubAllEnvs(); + vi.clearAllMocks(); +}); + +describe('Datadog boot view against the real router', () => { + it('starts exactly one view when a signed-out deep link redirects to sign-in', async () => { + const instance = await bootDeepLink(); + + await waitFor(() => expect(instance.state.location.pathname).toBe('/sign-in')); + + expect(rum.startView).toHaveBeenCalledTimes(1); + expect(rum.startView).toHaveBeenCalledWith(expect.objectContaining({ name: '/sign-in/' })); + }); +});