diff --git a/DESIGN.md b/DESIGN.md index a66b1bd9d..9a656f28f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -12,6 +12,7 @@ This file collects architecture and design notes for studio that aren't otherwis - [`src/features/instance/databases/DESIGN.md`](src/features/instance/databases/DESIGN.md) — when the browse grid may show its spinner, and why a table without a primary key can't be listed. +- [`src/features/instance/applications/DESIGN.md`](src/features/instance/applications/DESIGN.md) — when a rolling deploy is done: following its restart job, and why a job Studio can't follow is never a success; and when a live deploy may be sent again. - [`src/features/instance/config/roles/DESIGN.md`](src/features/instance/config/roles/DESIGN.md) — why the role editor's JSON text is the only source of truth, what forces the JSON view, and what Save changes on purpose. - [`src/router/DESIGN.md`](src/router/DESIGN.md) — why a failed route load is reported and shown only by its route, and why a preload still toasts. @@ -75,6 +76,7 @@ Central manager only ever sends the boolean, never the other organizations' ids; again on the dead session. The guard redirects every signed-out dashboard load, so the spinner only covers the moment before it does (`endedSessionRouteFailures.test.tsx`, HarperFast/studio#1546). - **Data fetching**: TanStack React Query 5. Query keys must be instance-scoped when the request targets a specific instance. +- **Polling**: every `refetchInterval` on a query that can reject against an instance or a cluster goes through [`pollUnlessForbidden`](src/react-query/pollUnlessForbidden.ts), which stops the timer on a 403 and backs off a sustained 400 or 5xx; a raw number polls a failing endpoint at full rate for as long as the page is open. When the interval depends on the data, compute it and hand it over per tick: `(query) => pollUnlessForbidden(base(query))(query)` (the back-off state is per query, not per wrapper). - **Forms**: react-hook-form with a zod `zodResolver`. A form whose submit button waits on `formState.isValid` must not use the default `onSubmit` mode, which keeps `isValid` current but shows errors only after a submit the disabled button never allows, so the user is blocked with no reason given (HarperFast/studio#1550). Use `mode: 'onTouched'`: a field's error appears once it is first left and then follows each change, while `isValid` still tracks every keystroke (`onBlur` recomputes `isValid` only on blur, so the button lags the text). Enter neither submits past a disabled button nor leaves the field, so give the `
` `onKeyDown={revealFieldErrorOnEnter(form)}`, which treats Enter in a text input as leaving it. A refine that compares two fields reports on one of them; re-check that field from the other's `onChange` once it has been touched (`getFieldState(name).isTouched`, then `trigger(name)`), because `rules={{ deps }}` fires on the other field's first blur, before the user reaches it. react-hook-form only updates the `formState` fields a render has read, and `disabled={!isDirty || !isValid}` skips `isValid` while the form is pristine, so where one change can complete the form (a single field, a paste, a checkbox) the button stays disabled on a valid form. Read the fields before the JSX (`const { isDirty, isValid } = form.formState;`) and pin it with a test that makes one change and checks the button. - **Cloud permission gates** ([`usePermissions.ts`](src/hooks/usePermissions.ts)) must mirror the check the central manager endpoint runs, not the nearest role flag. An org admin appears in `/User/current` as `roles[orgId].role === 'admin'`; central manager builds every org role with `permission.super_user: false`, so the member helpers' `super_user` short-circuits never identify one — an admin gets full access there from its all-true flags. Container lifecycle ops are where the two diverge: their endpoints admit only org admins and staff holding `instance:update`, so gate them on `useContainerOpsPermission`, never on cluster `update`. - **Unit tests never read `.env` files and never open a real WebSocket.** `vitest.config.ts` sets diff --git a/src/features/cluster/InstanceActionsMenu.test.tsx b/src/features/cluster/InstanceActionsMenu.test.tsx index 7aee1f7ae..1314e6310 100644 --- a/src/features/cluster/InstanceActionsMenu.test.tsx +++ b/src/features/cluster/InstanceActionsMenu.test.tsx @@ -7,7 +7,12 @@ import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { InstanceActionsMenu } from './InstanceActionsMenu'; const ORG = 'org-a'; -const state = vi.hoisted(() => ({ user: null as unknown, run: vi.fn() })); +const state = vi.hoisted(() => ({ + user: null as unknown, + run: vi.fn(), + statusEnabled: [] as (boolean | undefined)[], + availability: undefined as string | undefined, +})); vi.mock('@tanstack/react-router', () => ({ useParams: () => ({ organizationId: 'org-a', clusterId: 'clu-b' }), Link: ({ children, to }: { children: ReactNode; to: string }) => {children}, @@ -25,9 +30,13 @@ vi.mock('@/config/useInstanceClient', () => ({ useInstanceClient: () => ({}), useInstanceClientIdParams: () => ({}), })); -vi.mock('@/integrations/api/instance/status/getStatus', () => ({ - getStatusQueryOptions: () => ({ queryKey: ['status'], queryFn: () => null, enabled: false }), - getSystemStatusById: () => undefined, +vi.mock('@/integrations/api/instance/status/getStatus', async importOriginal => ({ + ...await importOriginal(), + getStatusQueryOptions: (_params: unknown, enabled?: boolean) => { + state.statusEnabled.push(enabled); + return { queryKey: ['status'], queryFn: () => null, enabled: false }; + }, + getSystemStatusById: () => state.availability, })); vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); vi.mock('@/hooks/useInstanceContainerOps', () => ({ useInstanceContainerOps: () => ({ run: state.run }) })); @@ -46,9 +55,17 @@ beforeAll(() => { afterEach(() => { cleanup(); state.run.mockClear(); + state.statusEnabled = []; + state.availability = undefined; }); -const instance = { id: 'ins-c', name: 'primary', status: 'RUNNING', instanceFqdn: 'ins-c.example.com' } as Instance; +const instance = { + id: 'ins-c', + name: 'primary', + status: 'RUNNING', + instanceFqdn: 'ins-c.example.com', + version: '4.7.3', +} as Instance; const member = (role: string) => ({ fabricRole: 'least_privileged', @@ -65,10 +82,10 @@ const member = (role: string) => }, }) as unknown as User; -function openMenu() { +function openMenu(target: Instance = instance) { render( - + , ); fireEvent.pointerDown(screen.getByRole('button', { name: 'Instance options' }), { button: 0, ctrlKey: false }); @@ -94,3 +111,31 @@ describe('InstanceActionsMenu container ops', () => { expect(state.run).toHaveBeenCalledWith('stop'); }); }); + +describe('InstanceActionsMenu availability status', () => { + it('asks for it while the menu is open', () => { + state.user = member('admin'); + openMenu(); + expect(state.statusEnabled.at(-1)).toBe(true); + }); + + it('never asks a Harper older than 4.6, which has no get_status', () => { + state.user = member('admin'); + openMenu({ ...instance, version: '4.5.12' }); + expect(state.statusEnabled.at(-1)).toBe(false); + }); + + it('offers rotation from a known availability', () => { + state.user = member('admin'); + state.availability = 'Available'; + openMenu(); + expect(screen.getByRole('menuitem', { name: 'Bring out of rotation' })).toBeTruthy(); + }); + + it('never offers rotation on a Harper older than 4.6, even from a cached availability', () => { + state.user = member('admin'); + state.availability = 'Available'; + openMenu({ ...instance, version: '4.5.12' }); + expect(screen.queryByRole('menuitem', { name: 'Bring out of rotation' })).toBeNull(); + }); +}); diff --git a/src/features/cluster/InstanceStatusCell.polling.test.tsx b/src/features/cluster/InstanceStatusCell.polling.test.tsx new file mode 100644 index 000000000..6542d7b71 --- /dev/null +++ b/src/features/cluster/InstanceStatusCell.polling.test.tsx @@ -0,0 +1,92 @@ +/** @vitest-environment jsdom */ +import type { Instance } from '@/integrations/api/api.patch'; +import { FakeIntersectionObserver } from '@/test/fakeIntersectionObserver'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, cleanup, render } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { InstanceStatusCell } from './InstanceStatusCell'; + +/** Real `getStatusQueryOptions` and a real QueryClient: only the transport is faked, so these + * count the requests the instances list actually sends. */ +const state = vi.hoisted(() => ({ requests: [] as string[] })); +vi.mock('@/hooks/usePermissions', () => ({ useOrganizationClusterInstancePermissions: () => ({ update: true }) })); +vi.mock('@/config/useInstanceClient', () => ({ + useInstanceClientIdParams: ({ instanceId }: { instanceId: string }) => ({ + entityId: instanceId, + instanceClient: { + post: async () => { + state.requests.push(instanceId); + return { data: { systemStatus: [{ id: 'availability', status: 'Available' }], componentStatus: [] } }; + }, + }, + }), +})); +vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); + +const ids = ['ins-0', 'ins-1', 'ins-2']; + +beforeEach(() => { + vi.useFakeTimers(); + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + state.requests = []; +}); +afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + FakeIntersectionObserver.reset(); +}); + +function renderList() { + const queryClient = new QueryClient(); + render( + + {ids.map((id, index) => ( +
+ +
+ ))} +
, + ); + return queryClient; +} + +function rowIs(id: string) { + return (target: Element) => target.closest('[data-row]')?.getAttribute('data-row') === id; +} + +function requestsFor(id: string) { + return state.requests.filter(r => r === id).length; +} + +describe('instances list get_status polling', () => { + it('polls only the rows on screen', async () => { + const queryClient = renderList(); + // Past the per-row stagger; React flushes the `ready` update only when an act() ends. + await act(() => vi.advanceTimersByTimeAsync(2_000)); + act(() => FakeIntersectionObserver.setIntersecting(true, rowIs('ins-0'))); + await act(() => vi.advanceTimersByTimeAsync(60_000)); + + expect(requestsFor('ins-0')).toBe(7); + expect(requestsFor('ins-1')).toBe(0); + expect(requestsFor('ins-2')).toBe(0); + queryClient.clear(); + }); + + it('fetches a row as soon as it scrolls into view, then keeps its 10s cadence', async () => { + const queryClient = renderList(); + await act(() => vi.advanceTimersByTimeAsync(30_000)); + expect(state.requests).toHaveLength(0); + + act(() => FakeIntersectionObserver.setIntersecting(true, rowIs('ins-1'))); + await act(() => vi.advanceTimersByTimeAsync(0)); + expect(requestsFor('ins-1')).toBe(1); + + await act(() => vi.advanceTimersByTimeAsync(30_000)); + expect(requestsFor('ins-1')).toBe(4); + queryClient.clear(); + }); +}); diff --git a/src/features/cluster/InstanceStatusCell.test.tsx b/src/features/cluster/InstanceStatusCell.test.tsx index ff484e8d1..e5ab738d0 100644 --- a/src/features/cluster/InstanceStatusCell.test.tsx +++ b/src/features/cluster/InstanceStatusCell.test.tsx @@ -1,5 +1,6 @@ /** @vitest-environment jsdom */ import type { Instance } from '@/integrations/api/api.patch'; +import { FakeIntersectionObserver } from '@/test/fakeIntersectionObserver'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, cleanup, render, screen } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -9,7 +10,8 @@ const state = vi.hoisted(() => ({ enabled: [] as (boolean | undefined)[] })); vi.mock('@/hooks/usePermissions', () => ({ useOrganizationClusterInstancePermissions: () => ({ update: true }) })); vi.mock('@/config/useInstanceClient', () => ({ useInstanceClientIdParams: () => ({}) })); vi.mock('@/integrations/api/instance/status/setStatus', () => ({ useSetStatus: () => ({ mutate: vi.fn() }) })); -vi.mock('@/integrations/api/instance/status/getStatus', () => ({ +vi.mock('@/integrations/api/instance/status/getStatus', async importOriginal => ({ + ...await importOriginal(), getStatusQueryOptions: (_params: unknown, enabled?: boolean) => { state.enabled.push(enabled); return { queryKey: ['status'], queryFn: () => null, enabled: false }; @@ -24,19 +26,24 @@ beforeEach(() => { afterEach(() => { cleanup(); vi.useRealTimers(); + vi.unstubAllGlobals(); + FakeIntersectionObserver.reset(); }); -async function renderCell(status: string) { +async function renderCell(status: string, version: string | undefined = '4.7.3') { render( - + , ); await act(() => vi.advanceTimersByTimeAsync(1_000)); } -async function pollEnabledFor(status: string) { - await renderCell(status); +async function pollEnabledFor(status: string, version?: string) { + await renderCell(status, version); return state.enabled.at(-1); } @@ -57,4 +64,57 @@ describe('InstanceStatusCell get_status poll', () => { expect(screen.getByLabelText('Status unavailable')).toBeTruthy(); expect(screen.queryByRole('button')).toBeNull(); }); + + it.each(['4.6.0', '4.7.3', '5.0.2'])('polls a Harper %s instance', async version => { + expect(await pollEnabledFor('RUNNING', version)).toBe(true); + }); + + it.each(['4.4.24', '4.5.12'])( + 'does not poll a Harper %s instance, which rejects get_status with a 400 on every tick', + async version => { + expect(await pollEnabledFor('RUNNING', version)).toBe(false); + }, + ); + + it.each(['', 'custom-build'])( + 'does not poll an instance whose Harper version is %j, nor blame the version', + async version => { + expect(await pollEnabledFor('RUNNING', version)).toBe(false); + expect(screen.getByLabelText('Status unavailable')).toBeTruthy(); + }, + ); + + it('says why a pre-4.6 instance has no availability, instead of spinning or offering rotation', async () => { + await renderCell('RUNNING', '4.5.12'); + expect(screen.getByLabelText('Status needs Harper 4.6 or later')).toBeTruthy(); + expect(screen.queryByRole('button')).toBeNull(); + }); +}); + +describe('InstanceStatusCell off-screen gating', () => { + beforeEach(() => { + vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver); + }); + + it('does not poll a row until it scrolls into view', async () => { + await renderCell('RUNNING'); + expect(state.enabled.at(-1)).toBe(false); + + act(() => FakeIntersectionObserver.setIntersecting(true)); + expect(state.enabled.at(-1)).toBe(true); + }); + + it('stops polling a row once it scrolls out of view', async () => { + await renderCell('RUNNING'); + act(() => FakeIntersectionObserver.setIntersecting(true)); + act(() => FakeIntersectionObserver.setIntersecting(false)); + expect(state.enabled.at(-1)).toBe(false); + }); + + it('shows a spinner rather than an unknown status for a row that has not loaded yet', async () => { + await renderCell('RUNNING'); + expect(screen.queryByText('Offline')).toBeNull(); + expect(document.querySelector('.animate-spin')?.getAttribute('aria-hidden')).toBe('true'); + expect(screen.getByText('Loading status')).toBeTruthy(); + }); }); diff --git a/src/features/cluster/InstanceStatusCell.tsx b/src/features/cluster/InstanceStatusCell.tsx index 109aa7b36..415d2ec25 100644 --- a/src/features/cluster/InstanceStatusCell.tsx +++ b/src/features/cluster/InstanceStatusCell.tsx @@ -3,9 +3,15 @@ import { Button } from '@/components/ui/button'; import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'; import { isOperationsProxyRefused, isStoppedOrTransitioning } from '@/components/ui/utils/badgeStatus'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { useIsOnScreen } from '@/hooks/useIsOnScreen'; import { useOrganizationClusterInstancePermissions } from '@/hooks/usePermissions'; import { Instance } from '@/integrations/api/api.patch'; -import { getStatusQueryOptions, getSystemStatusById } from '@/integrations/api/instance/status/getStatus'; +import { + getStatusQueryOptions, + getSystemStatusById, + predatesGetStatus, + supportsGetStatus, +} from '@/integrations/api/instance/status/getStatus'; import { useSetStatus } from '@/integrations/api/instance/status/setStatus'; import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; import { useQuery } from '@tanstack/react-query'; @@ -25,6 +31,10 @@ export function InstanceStatusCell( const { mutate: setStatus, isPending: isSettingStatus } = useSetStatus(); const [ready, setReady] = useState(false); + const [cellElement, setCellElement] = useState(null); + // A row scrolled out of view stops polling, so a large cluster costs what is on screen rather than + // one request per instance every 10s. The margin starts a row's fetch just before it scrolls in. + const onScreen = useIsOnScreen(cellElement, '200px'); // Spread the status requests out. useEffect(() => { @@ -33,7 +43,8 @@ export function InstanceStatusCell( }, [index]); const notProxied = isOperationsProxyRefused(instance.status); - const statusPollEnabled = ready && canManage && !notProxied; + const statusSupported = supportsGetStatus(instance.version); + const statusPollEnabled = ready && onScreen && canManage && !notProxied && statusSupported; const { data: statusResponse, isLoading, isFetching } = useQuery( getStatusQueryOptions(instanceParams, statusPollEnabled), ); @@ -46,10 +57,16 @@ export function InstanceStatusCell( return null; } - // Availability and rotation are N/A while central manager refuses the instance's operations. Show - // a muted dot rather than a stale green "Available" (cached from before) or an endless spinner. - if (notProxied) { - const label = isStoppedOrTransitioning(instance.status) ? 'Not running' : 'Status unavailable'; + // Availability and rotation are N/A while central manager refuses the instance's operations, or on + // a Harper too old to have them. Show a muted dot rather than a stale green "Available" (cached + // from before) or an endless spinner. + if (notProxied || !statusSupported) { + let label = 'Status unavailable'; + if (notProxied && isStoppedOrTransitioning(instance.status)) { + label = 'Not running'; + } else if (!notProxied && predatesGetStatus(instance.version)) { + label = 'Status needs Harper 4.6 or later'; + } return (
@@ -63,12 +80,17 @@ export function InstanceStatusCell( } return ( -
+
- {isLoading || !ready || (isFetching && !statusResponse) - ? + {isLoading || !ready || ((isFetching || !onScreen) && !statusResponse) + ? ( + <> + + Loading status + + ) : (