diff --git a/apps/web/src/App.tsx b/apps/web/src/App.tsx index 6f8afc46..31be9393 100644 --- a/apps/web/src/App.tsx +++ b/apps/web/src/App.tsx @@ -30,6 +30,8 @@ import { isTerminalRunStatus, removeTrackedBatch, removeTrackedRun, + selectStaleTrackedRuns, + STALE_TRACKED_RUN_MS, subscribeRunTracker, summarizeBatchStatuses, } from './lib/run-tracker-store.js' @@ -223,6 +225,40 @@ export function RunNotificationObserver() { removeTrackedRun(run.id) } + // Give up on tracked runs that have aged out of the capped GET /runs window + // (see STALE_TRACKED_RUN_MS). Such a run never reappears, so the loop above + // never sees it reach a terminal status and its trigger button would wedge + // on "Syncing…"/"Running…" forever. Only reconcile once /runs has actually + // loaded (`data` defined), so a not-yet-loaded list is never misread as + // "absent"; the TTL additionally protects a just-queued run that hasn't hit + // the list yet. Refresh the kind's queries with whatever is current, then + // drop the tracked run so its button re-enables. + if (runsQuery.data !== undefined) { + const presentRunIds = new Set(runs.map((r) => r.id)) + const staleRuns = selectStaleTrackedRuns( + Object.values(trackedState.runs), + presentRunIds, + Date.now(), + STALE_TRACKED_RUN_MS, + ) + for (const trackedRun of staleRuns) { + const project = projects.find((candidate) => candidate.id === trackedRun.projectId) + if (project) { + invalidateQueriesForRunKind(queryClient, trackedRun.kind, project.name) + } + if (trackedRun.sourceAction !== 'run-all') { + addToast({ + title: `${formatTrackedRunKind(trackedRun.kind)} status unavailable`, + detail: resolveProjectLabel(trackedRun.projectId, trackedRun.projectLabel, projects), + tone: 'neutral', + dedupeKey: `run-stale:${trackedRun.runId}`, + dedupeMode: 'replace', + }) + } + removeTrackedRun(trackedRun.runId) + } + } + for (const batch of Object.values(trackedState.batches)) { const summary = summarizeBatchStatuses(batch.runIds, runsById) if (!summary.finished) continue diff --git a/apps/web/src/components/project/GbpSection.tsx b/apps/web/src/components/project/GbpSection.tsx index 8e9e38d7..b8e10920 100644 --- a/apps/web/src/components/project/GbpSection.tsx +++ b/apps/web/src/components/project/GbpSection.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react' +import { useEffect, useState, useSyncExternalStore } from 'react' import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query' import { getApiV1ProjectsByNameGoogleConnectionsOptions, @@ -15,6 +15,7 @@ import { classifyGbpMetric, GBP_CONVERSION_METRICS, GBP_REACH_METRICS, + RunKinds, } from '@ainyc/canonry-contracts' import { Button } from '../ui/button.js' @@ -43,8 +44,9 @@ import { } from '../shared/ChartPrimitives.js' import { fetchInsights, heyClient } from '../../api.js' import { addToast } from '../../lib/toast-store.js' +import { getRunTrackerState, subscribeRunTracker, trackRun } from '../../lib/run-tracker-store.js' -export function GbpSection({ projectName }: { projectName: string }) { +export function GbpSection({ projectName, projectId }: { projectName: string; projectId: string }) { const queryClient = useQueryClient() // `null` = all tracked locations (aggregate). A locationName scopes every read // to one location. A single tracked location reads 1:1 with no selector. @@ -92,15 +94,43 @@ export function GbpSection({ projectName }: { projectName: string }) { }) } - // Errors surface through the global error toast (no skipGlobalErrorToast meta). + // GBP sync is an async background run: POST /gbp/sync queues a `gbp-sync` run + // and returns immediately. Hand the run to the global RunNotificationObserver + // (via trackRun) so it polls to completion, invalidates the GBP queries once + // the data is actually refreshed, and emits the completion toast — rather than + // invalidating eagerly here (which refetches the same stale data while the run + // is still in flight) and re-enabling the button the instant the POST returns. + // Errors on the POST itself still surface through the global error toast. const syncMutation = useMutation({ ...postApiV1ProjectsByNameGbpSyncMutation(), - onSuccess: () => { - addToast('Google Business Profile sync started.', 'positive') - invalidateGbp() + onSuccess: (data) => { + trackRun({ + id: data.runId, + projectId, + kind: RunKinds['gbp-sync'], + projectLabel: projectName, + sourceAction: 'gbp-sync', + }) + addToast({ + title: 'Business Profile sync started', + detail: `${projectName} will refresh when the sync completes.`, + tone: 'neutral', + dedupeKey: `gbp-sync:${projectName}`, + dedupeMode: 'replace', + }) }, }) + // Keep the Sync button disabled until the queued run reaches a terminal + // status. `syncMutation.isPending` only covers the brief POST; the tracked + // run (cleared by RunNotificationObserver on completion) covers the + // background sync that follows. + const trackerState = useSyncExternalStore(subscribeRunTracker, getRunTrackerState, getRunTrackerState) + const gbpSyncInFlight = Object.values(trackerState.runs).some( + (run) => run.kind === RunKinds['gbp-sync'] && run.projectId === projectId, + ) + const isSyncing = syncMutation.isPending || gbpSyncInFlight + const selectionMutation = useMutation({ ...putApiV1ProjectsByNameGbpLocationsByLocationNameSelectionMutation(), onSuccess: () => invalidateGbp(), @@ -176,10 +206,10 @@ export function GbpSection({ projectName }: { projectName: string }) { type="button" variant="outline" size="sm" - disabled={syncMutation.isPending} + disabled={isSyncing} onClick={() => syncMutation.mutate({ client: heyClient, path: { name: projectName }, body: {} })} > - {syncMutation.isPending ? 'Syncing…' : 'Sync'} + {isSyncing ? 'Syncing…' : 'Sync'} diff --git a/apps/web/src/lib/run-tracker-store.ts b/apps/web/src/lib/run-tracker-store.ts index a448b463..03313321 100644 --- a/apps/web/src/lib/run-tracker-store.ts +++ b/apps/web/src/lib/run-tracker-store.ts @@ -5,6 +5,7 @@ export type TrackedRunSourceAction = | 'setup-launch' | 'run-all' | 'gsc-sync' + | 'gbp-sync' | 'discover-sitemaps' | 'inspect-sitemap' @@ -12,9 +13,13 @@ export interface TrackedRun { runId: string projectId: string projectLabel?: string - kind: string + kind: ApiRun['kind'] sourceAction: TrackedRunSourceAction lastAnnouncedStatus: string + /** Epoch ms when the run was first tracked. Used to give up on a run that has + * aged out of the capped /runs window (see `selectStaleTrackedRuns`). + * Optional: rows persisted before this field existed hydrate without it. */ + trackedAt?: number } export interface TrackedBatch { @@ -96,6 +101,8 @@ export function trackRun(run: Pick & { projectLabel?: string sourceAction: TrackedRunSourceAction lastAnnouncedStatus?: string + /** Override the tracked-at stamp (tests); defaults to now. */ + trackedAt?: number }) { const snapshot = getRunTrackerState() updateState({ @@ -109,6 +116,7 @@ export function trackRun(run: Pick & { kind: run.kind, sourceAction: run.sourceAction, lastAnnouncedStatus: run.lastAnnouncedStatus ?? 'queued', + trackedAt: run.trackedAt ?? Date.now(), }, }, }) @@ -172,6 +180,36 @@ export function isTerminalRunStatus(status: string) { return status === 'completed' || status === 'partial' || status === 'failed' || status === 'cancelled' } +/** + * How long a tracked run may be ABSENT from the GET /runs response before the + * observer gives up on it. GET /runs is capped (default 500 rows / a since + * window), so a tracked run can age out of that window — cron syncs alone can + * fill it in under an hour — after which the observer never sees it reach a + * terminal status and its trigger button would wedge on "Syncing…"/"Running…" + * forever. Absence is only ever the aged-out case (an in-flight run is still in + * the window), so this is purely a backstop; a page reload clears it sooner. + */ +export const STALE_TRACKED_RUN_MS = 10 * 60_000 + +/** + * Pure give-up rule: return the tracked runs that have aged out of the /runs + * window and should be cleared. A run still PRESENT in `presentRunIds` is + * never stale (the observer is still watching it); a run ABSENT for at least + * `staleMs` (relative to `now`) has aged out. A run with no `trackedAt` (a row + * persisted before this field existed) is treated as infinitely old, so any + * pre-existing wedged run is cleared on the next poll. + */ +export function selectStaleTrackedRuns( + trackedRuns: TrackedRun[], + presentRunIds: Set, + now: number, + staleMs: number, +): TrackedRun[] { + return trackedRuns.filter( + (run) => !presentRunIds.has(run.runId) && now - (run.trackedAt ?? 0) >= staleMs, + ) +} + export function summarizeBatchStatuses(runIds: string[], runsById: Record) { let completed = 0 let partial = 0 diff --git a/apps/web/src/pages/ProjectPage.tsx b/apps/web/src/pages/ProjectPage.tsx index 50ab3118..110dee31 100644 --- a/apps/web/src/pages/ProjectPage.tsx +++ b/apps/web/src/pages/ProjectPage.tsx @@ -2227,7 +2227,7 @@ function ProjectPageContent({ ) : tab === 'local' ? ( // Local presence (Google Business Profile + Places). GbpSection // self-gates on the connection and renders its own empty state. - + ) : ( )} diff --git a/apps/web/test/gbp-section.test.tsx b/apps/web/test/gbp-section.test.tsx index 40cbbd65..7125b5cf 100644 --- a/apps/web/test/gbp-section.test.tsx +++ b/apps/web/test/gbp-section.test.tsx @@ -1,6 +1,6 @@ import React from 'react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' -import { afterEach, expect, onTestFinished, test, vi } from 'vitest' +import { afterEach, beforeEach, expect, onTestFinished, test, vi } from 'vitest' import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' // Recharts needs a sized container (jsdom has none); stub it like the other @@ -21,16 +21,31 @@ vi.mock('recharts', () => { } }) +import { act } from '@testing-library/react' + import { GbpSection } from '../src/components/project/GbpSection.js' +import { getRunTrackerState, removeTrackedRun, resetRunTracker } from '../src/lib/run-tracker-store.js' import { mockFetch, jsonResponse } from './mock-fetch.js' -afterEach(cleanup) +// The run tracker is a module-global store backed by sessionStorage; reset it +// around every test so a queued gbp-sync run from one test never leaks into the +// button state of the next. +beforeEach(() => { + resetRunTracker() + window.sessionStorage.clear() +}) + +afterEach(() => { + cleanup() + resetRunTracker() + window.sessionStorage.clear() +}) function renderGbpSection() { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) return render( - + , ) } @@ -299,3 +314,57 @@ test('shows a connect empty-state when the project has no GBP connection', async await waitFor(() => expect(screen.getByText(/No Google Business Profile is connected/)).toBeTruthy()) expect(screen.getByText('Google Business Profile')).toBeTruthy() }) + +test('Sync queues a gbp-sync run, stays disabled, and hands off to the run tracker', async () => { + const restoreFetch = mockFetch((url, init) => { + const urlPath = url.split('?')[0]! + if (urlPath.endsWith('/projects/test-project/gbp/sync') && init?.method === 'POST') { + // POST /gbp/sync queues an async run and returns immediately. + return jsonResponse({ runId: 'gbp-run-1', status: 'running' }) + } + if (urlPath.endsWith('/projects/test-project/google/connections')) return jsonResponse([gbpConnection]) + if (urlPath.endsWith('/projects/test-project/gbp/summary')) return jsonResponse(emptySummary()) + if (urlPath.endsWith('/projects/test-project/gbp/locations')) { + return jsonResponse({ locations: [makeLocation({})], totalDiscovered: 1, totalSelected: 1 }) + } + if (urlPath.endsWith('/projects/test-project/gbp/keywords')) return jsonResponse({ keywords: [], total: 0, thresholdedPct: 0 }) + if (urlPath.endsWith('/projects/test-project/gbp/places')) return jsonResponse({ places: [], total: 0 }) + if (urlPath.endsWith('/projects/test-project/insights')) return jsonResponse([]) + throw new Error(`Unexpected fetch: ${url} ${init?.method ?? ''}`) + }) + onTestFinished(restoreFetch) + + renderGbpSection() + + // The Sync action is enabled once the connection resolves. + const syncButton = await screen.findByRole('button', { name: 'Sync' }) + expect(syncButton.hasAttribute('disabled')).toBe(false) + + fireEvent.click(syncButton) + + // The button stays disabled and reads "Syncing…" while the queued run is in + // flight — it does not snap back to "Sync" the instant the POST returns. (The + // global RunNotificationObserver, not mounted here, clears the tracked run on + // completion, which is what re-enables it in the app.) + await waitFor(() => expect(screen.getByRole('button', { name: /Syncing/ })).toBeTruthy()) + expect(screen.getByRole('button', { name: /Syncing/ }).hasAttribute('disabled')).toBe(true) + + // The async run is handed to the global run tracker (which polls it to + // completion and then invalidates the GBP queries) rather than invalidating + // eagerly here while the data is still stale. + const tracked = Object.values(getRunTrackerState().runs) + expect(tracked).toHaveLength(1) + expect(tracked[0]).toMatchObject({ + runId: 'gbp-run-1', + projectId: 'p1', + kind: 'gbp-sync', + sourceAction: 'gbp-sync', + }) + + // When the run reaches a terminal status the observer drops it from the + // tracker (RunNotificationObserver → removeTrackedRun). Simulate that and + // assert the Sync button re-enables — i.e. it stays disabled *until the sync + // is complete*, not just for the POST. + act(() => removeTrackedRun('gbp-run-1')) + await waitFor(() => expect(screen.getByRole('button', { name: 'Sync' }).hasAttribute('disabled')).toBe(false)) +}) diff --git a/apps/web/test/run-invalidations.test.ts b/apps/web/test/run-invalidations.test.ts index 630e55ab..f58a8428 100644 --- a/apps/web/test/run-invalidations.test.ts +++ b/apps/web/test/run-invalidations.test.ts @@ -80,9 +80,16 @@ test('invalidates GA operations for ga-sync runs', () => { expect(predicateMatches('getApiV1ProjectsByNameGaStatus')).toBe(true) }) +test('invalidates GBP operations for gbp-sync runs', () => { + invalidateQueriesForRunKind(queryClient, RunKinds['gbp-sync'], 'demo') + expect(predicateMatches('getApiV1ProjectsByNameGbpSummary')).toBe(true) + expect(predicateMatches('getApiV1ProjectsByNameGbpKeywords')).toBe(true) +}) + test('does not invalidate domain-scoped operations for answer-visibility runs', () => { invalidateQueriesForRunKind(queryClient, RunKinds['answer-visibility'], 'demo') expect(predicateMatches('getApiV1ProjectsByNameGoogleGscCoverage')).toBe(false) expect(predicateMatches('getApiV1ProjectsByNameBingCoverage')).toBe(false) expect(predicateMatches('getApiV1ProjectsByNameGaTraffic')).toBe(false) + expect(predicateMatches('getApiV1ProjectsByNameGbpSummary')).toBe(false) }) diff --git a/apps/web/test/run-notifications.test.tsx b/apps/web/test/run-notifications.test.tsx index dd2dc905..4b5aa3b3 100644 --- a/apps/web/test/run-notifications.test.tsx +++ b/apps/web/test/run-notifications.test.tsx @@ -480,3 +480,41 @@ test('maps RUN_IN_PROGRESS errors to one caution toast with an extended timer', expect(runInProgressToasts[0]?.detail).toContain('Citypoint Dental NYC already has an active run') }) }) + +test('clears a tracked sync run that has aged out of the /runs window so its button cannot wedge', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = input instanceof Request ? input.url : String(input) + // The tracked run has aged out of the capped window: GET /runs no longer returns it. + if (url.includes('/api/v1/runs')) return jsonResponse([]) + if (url.includes('/api/v1/projects')) return jsonResponse([makeProject()]) + throw new Error(`Unexpected fetch: ${url}`) + }) + vi.stubGlobal('fetch', fetchMock) + + // A gbp-sync run tracked long ago (epoch) — absent from /runs and past the TTL. + trackRun({ + id: 'gbp_run_aged', + projectId: 'proj_1', + kind: 'gbp-sync', + projectLabel: 'Citypoint Dental NYC', + sourceAction: 'gbp-sync', + lastAnnouncedStatus: 'queued', + trackedAt: 0, + }) + expect(getRunTrackerState().runs.gbp_run_aged).toBeTruthy() + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false, staleTime: Infinity } } }) + queryClient.setQueryData(runsCacheKey, []) + queryClient.setQueryData(projectsCacheKey, [makeProject()]) + render( + + + , + ) + + // The observer gives up on the aged-out run instead of tracking it forever: it + // drops from the tracker (which re-enables the Sync button) and surfaces a + // neutral "status unavailable" toast. + await waitFor(() => expect(getRunTrackerState().runs.gbp_run_aged).toBeUndefined()) + expect(getToasts().some((toast) => toast.title === 'Business Profile sync status unavailable')).toBe(true) +}) diff --git a/apps/web/test/run-tracker-store.test.ts b/apps/web/test/run-tracker-store.test.ts new file mode 100644 index 00000000..086e2ee7 --- /dev/null +++ b/apps/web/test/run-tracker-store.test.ts @@ -0,0 +1,69 @@ +import { afterEach, beforeEach, expect, test } from 'vitest' + +import { + resetRunTracker, + selectStaleTrackedRuns, + STALE_TRACKED_RUN_MS, + type TrackedRun, +} from '../src/lib/run-tracker-store.js' + +beforeEach(() => { + resetRunTracker() + window.sessionStorage.clear() +}) + +afterEach(() => { + resetRunTracker() + window.sessionStorage.clear() +}) + +function tracked(partial: Partial & { runId: string }): TrackedRun { + return { + projectId: 'p1', + kind: 'gbp-sync', + sourceAction: 'gbp-sync', + lastAnnouncedStatus: 'queued', + trackedAt: 1_000, + ...partial, + } +} + +// `selectStaleTrackedRuns` is the give-up rule for tracked runs that have aged +// out of the capped GET /runs window: a run still PRESENT in /runs is never +// stale (the observer is still watching it), while a run ABSENT from /runs for +// longer than the TTL has aged out and must be cleared so its trigger button +// cannot wedge on "Syncing…"/"Running…" forever. + +test('a tracked run still present in /runs is never stale, even when old', () => { + const runs = [tracked({ runId: 'r1', trackedAt: 0 })] + expect(selectStaleTrackedRuns(runs, new Set(['r1']), 9_999_999, 1_000)).toEqual([]) +}) + +test('a tracked run absent from /runs past the TTL is stale', () => { + const runs = [tracked({ runId: 'r1', trackedAt: 0 })] + expect(selectStaleTrackedRuns(runs, new Set(), 1_000, 1_000).map((r) => r.runId)).toEqual(['r1']) +}) + +test('a tracked run absent but within the TTL grace is not cleared (covers the just-queued race)', () => { + const runs = [tracked({ runId: 'r1', trackedAt: 0 })] + expect(selectStaleTrackedRuns(runs, new Set(), 999, 1_000)).toEqual([]) +}) + +test('a legacy tracked run with no trackedAt, absent from /runs, is cleared', () => { + const legacy = tracked({ runId: 'r1', trackedAt: undefined }) + expect(selectStaleTrackedRuns([legacy], new Set(), 1_000, 1_000).map((r) => r.runId)).toEqual(['r1']) +}) + +test('from a mixed set, only the absent-past-TTL runs are returned', () => { + const runs = [ + tracked({ runId: 'present', trackedAt: 0 }), + tracked({ runId: 'fresh-absent', trackedAt: 950 }), + tracked({ runId: 'stale-absent', trackedAt: 0 }), + ] + const staleIds = selectStaleTrackedRuns(runs, new Set(['present']), 1_000, 1_000).map((r) => r.runId) + expect(staleIds.sort()).toEqual(['stale-absent']) +}) + +test('STALE_TRACKED_RUN_MS is a sane positive duration well above the poll interval', () => { + expect(STALE_TRACKED_RUN_MS).toBeGreaterThan(60_000) +}) diff --git a/package.json b/package.json index 3f8a8a15..577673e3 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "canonry", "private": true, - "version": "4.69.1", + "version": "4.69.2", "type": "module", "packageManager": "pnpm@10.28.2", "scripts": { diff --git a/packages/canonry/package.json b/packages/canonry/package.json index 294d09ed..a9021716 100644 --- a/packages/canonry/package.json +++ b/packages/canonry/package.json @@ -1,6 +1,6 @@ { "name": "@ainyc/canonry", - "version": "4.69.1", + "version": "4.69.2", "type": "module", "description": "Agent-first open-source AEO operating platform - track how answer engines cite your domain", "license": "FSL-1.1-ALv2",