From e59d02f37ece94fa03334db4370449b4bb7b916f Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 13 Oct 2026 10:28:20 -0400 Subject: [PATCH 1/2] fix(browse): refresh the table once an import's job completes An import into a table that is already open left the grid, its count and the database overview on the rows from before the import, until a manual Refresh. The import only refetched describe_all and navigated to the table. Navigating to the table already on screen fetches nothing, because every Browse read has a 60s staleTime. This regressed in #1471, which moved the import modal into DatabaseActionModals and dropped the table view's own refreshTable() call for the same-table case. #1880's job-following toast did not change it. useImportDataMutation now refreshes from the mutation's own onSettled, so the refresh does not depend on the modal still being mounted. It invalidates the table's queries ([entityId, database, table]: the page, a filtered search, the exact count, describe_table) and describe_all. The refresh is not awaited, so the import still settles, and the modal still closes, at the job's verdict. onImported waits for describe_all before it navigates, as it did before, but now joins the mutation's refetch (cancelRefetch: false) instead of starting another. Every outcome invalidates, because any of them can have written: a job that skipped rows, a job that ended in ERROR (csv_url_load inserts chunk by chunk, so earlier chunks stay written), a wait that timed out on a load still running, a load refused after the import's create_table. A failure with no verdict (no response, or a 502/503/504) only marks the reads stale instead of refetching them, since they would fail too and add their own error toasts. The databases DESIGN.md records where the refresh lives and why. Co-Authored-By: Claude Opus 5.5 --- src/features/instance/databases/DESIGN.md | 20 ++ .../components/DatabaseActionModals.test.tsx | 48 +++- .../components/DatabaseActionModals.tsx | 7 +- .../api/instance/database/importData.test.tsx | 223 +++++++++++++++++- .../api/instance/database/importData.ts | 75 +++++- src/integrations/api/unansweredWrite.ts | 2 +- 6 files changed, 350 insertions(+), 25 deletions(-) diff --git a/src/features/instance/databases/DESIGN.md b/src/features/instance/databases/DESIGN.md index f24e94d16..c67dba9a4 100644 --- a/src/features/instance/databases/DESIGN.md +++ b/src/features/instance/databases/DESIGN.md @@ -90,3 +90,23 @@ so it is judged by that table's attribute. Typed filters and deep-linked ones bo [`DatabaseTableView`](components/DatabaseTableView.tsx), which asks first (#1285). The request then carries `allowFullScan: true`, and so does a CSV export of that filter. A new way to apply a filter has to go through that function. + +## An import refreshes Browse from the mutation, not from the modal + +Every Browse read has a 60s `staleTime`, so an import into the open table shows nothing new until +something invalidates `[entityId, database, table]`. Navigating to a table that is already on screen +doesn't invalidate it. That refresh lives in `useImportDataMutation`'s own `onSettled` +([`importData.ts`](../../../integrations/api/instance/database/importData.ts)), because React Query +skips a `mutate` call's callbacks once the component that made it has unmounted. It used to live in +the table view, and #1471 lost it when the modal moved into `DatabaseActionModals`. + +It is not awaited. The import settles at the job's verdict, and the modal closes then. The modal's +`onImported` waits for the `describe_all` refetch before it navigates +(`awaitDatabaseMapRefresh`, which joins the refetch rather than starting another), or +`resolveDatabasesRedirect` would send a table the import just created back to the overview. If that +refetch fails, the old map stays and the redirect still happens, as it did before. + +A failed import refreshes too, because a CSV loaded from a URL that fails partway leaves its earlier +chunks written. When the failure has no verdict (no response, or a 502/503/504), the reads are only +marked stale rather than refetched: they would fail too and add their own error toasts, and the next +mount, refocus or Refresh fetches them. diff --git a/src/features/instance/databases/components/DatabaseActionModals.test.tsx b/src/features/instance/databases/components/DatabaseActionModals.test.tsx index 1f080431d..d0f9f8f22 100644 --- a/src/features/instance/databases/components/DatabaseActionModals.test.tsx +++ b/src/features/instance/databases/components/DatabaseActionModals.test.tsx @@ -3,14 +3,14 @@ */ import { InstanceDatabaseMap, InstanceTable } from '@/integrations/api/api.patch'; import { setWatchedValue, useWatchedValue } from '@/lib/events/watcher'; -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { QueryClient, QueryClientProvider, useQuery } from '@tanstack/react-query'; import { act, cleanup, render, screen } from '@testing-library/react'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { DatabaseActionModals } from './DatabaseActionModals'; +const navigate = vi.hoisted(() => vi.fn()); vi.mock('@tanstack/react-router', () => { const params = {}; - const navigate = () => undefined; const router = { invalidate: async () => undefined }; return { useParams: () => params, useNavigate: () => navigate, useRouter: () => router }; }); @@ -43,7 +43,15 @@ vi.mock('@/features/instance/databases/modals/AddTableRowModal', () => ({ ), })); vi.mock('@/features/instance/databases/modals/CreateNewTableModal', () => ({ CreateNewTableModal: () => null })); -vi.mock('@/features/instance/databases/modals/ImportDataModal', () => ({ ImportDataModal: () => null })); +const importModal = vi.hoisted(() => ({ + onImported: undefined as undefined | ((databaseName: string, tableName: string) => Promise), +})); +vi.mock('@/features/instance/databases/modals/ImportDataModal', () => ({ + ImportDataModal: ({ onImported }: { onImported: typeof importModal.onImported }) => { + importModal.onImported = onImported; + return null; + }, +})); vi.mock('@/features/instance/databases/modals/DeleteTableModal', () => ({ DeleteTableModal: () => null })); vi.mock('@/features/instance/databases/modals/DeleteDatabaseModal', () => ({ DeleteDatabaseModal: () => null })); @@ -162,3 +170,37 @@ describe('DatabaseActionModals opening Add Records', () => { expect(screen.getByRole('status', { name: 'add target' }).textContent).toContain('"Dog"'); }); }); + +describe('DatabaseActionModals after an import', () => { + afterEach(() => navigate.mockClear()); + + it('goes to the imported table only once describe_all is back, so a table the import created is in the map', async () => { + const refetched = deferred(); + const tree = vi.fn<() => Promise>().mockResolvedValueOnce(instanceDatabaseMap).mockReturnValueOnce( + refetched.promise, + ); + const treeKey = ['instance-1', 'describe_all', true, true]; + function DatabasesPage() { + useQuery({ queryKey: treeKey, queryFn: tree, staleTime: 60_000 }); + return null; + } + const client = new QueryClient(); + render( + + + + , + ); + await vi.waitFor(() => expect(client.getQueryData(treeKey)).toBe(instanceDatabaseMap)); + + const imported = importModal.onImported!('data', 'Cat'); + await vi.waitFor(() => expect(tree).toHaveBeenCalledTimes(2)); + expect(navigate).not.toHaveBeenCalled(); + + await act(async () => { + refetched.resolve({ data: { ...instanceDatabaseMap.data, Cat: table('Cat', ['id']) } }); + await imported; + }); + expect(navigate).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/features/instance/databases/components/DatabaseActionModals.tsx b/src/features/instance/databases/components/DatabaseActionModals.tsx index 2327c67c4..deb672de4 100644 --- a/src/features/instance/databases/components/DatabaseActionModals.tsx +++ b/src/features/instance/databases/components/DatabaseActionModals.tsx @@ -7,6 +7,7 @@ import { DeleteTableModal } from '@/features/instance/databases/modals/DeleteTab import { ImportDataModal } from '@/features/instance/databases/modals/ImportDataModal'; import { InstanceDatabaseMap, InstanceTable } from '@/integrations/api/api.patch'; import { getDescribeTable } from '@/integrations/api/instance/database/getDescribeTable'; +import { awaitDatabaseMapRefresh } from '@/integrations/api/instance/database/importData'; import { setWatchedValue, useWatchedValue } from '@/lib/events/watcher'; import { WatchedValuesTypeMap } from '@/lib/storage/watchedValueKeys'; import { buildAbsoluteLinkToDatabasePage } from '@/lib/urls/buildAbsoluteLinkToDatabasePage'; @@ -91,11 +92,7 @@ export function DatabaseActionModals({ instanceDatabaseMap }: { instanceDatabase }, [addTarget, addTableFromMap, instanceParams]); const onImported = useCallback(async (databaseName: string, tableName: string) => { - // The import may have created a new table (or even database), so refresh the tree too. - await queryClient.invalidateQueries({ - queryKey: [instanceParams.entityId, 'describe_all'], - refetchType: 'all', - }); + await awaitDatabaseMapRefresh(queryClient, instanceParams.entityId); goToTable(databaseName, tableName); await router.invalidate(); }, [queryClient, instanceParams.entityId, goToTable, router]); diff --git a/src/integrations/api/instance/database/importData.test.tsx b/src/integrations/api/instance/database/importData.test.tsx index 1df8fe3f3..048a0b67f 100644 --- a/src/integrations/api/instance/database/importData.test.tsx +++ b/src/integrations/api/instance/database/importData.test.tsx @@ -1,10 +1,21 @@ // @vitest-environment jsdom -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { QueryClient, QueryClientProvider, useQuery } from '@tanstack/react-query'; import { act, renderHook } from '@testing-library/react'; -import type { AxiosInstance } from 'axios'; +import { AxiosError, type AxiosInstance, type AxiosResponse } from 'axios'; import type { ReactNode } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { importData, type ImportDataParams, type ImportProgressListener, useImportDataMutation } from './importData'; +import { getDescribeAllQueryOptions } from './getDescribeAll'; +import { getDescribeTableQueryOptions } from './getDescribeTable'; +import { getSearchByConditionsOptions } from './getSearchByConditions'; +import { getSearchByValueOptions } from './getSearchByValue'; +import { getTableRecordCountQueryOptions } from './getTableRecordCount'; +import { + awaitDatabaseMapRefresh, + importData, + type ImportDataParams, + type ImportProgressListener, + useImportDataMutation, +} from './importData'; function progressListener() { return { @@ -14,7 +25,7 @@ function progressListener() { }; } -function csvClient(statuses: string[]) { +function csvClient(statuses: string[], completeMessage = 'loaded 5 of 5') { let poll = 0; const post = vi.fn(async (_url: string, body: { operation: string }) => { if (body.operation === 'csv_url_load' || body.operation === 'csv_data_load') { @@ -22,13 +33,14 @@ function csvClient(statuses: string[]) { } const status = statuses[Math.min(poll, statuses.length - 1)]; poll += 1; - return { data: [{ id: 'job-1', status, message: status === 'COMPLETE' ? 'loaded 5 of 5' : undefined }] }; + return { data: [{ id: 'job-1', status, message: status === 'COMPLETE' ? completeMessage : undefined }] }; }); return { post } as unknown as AxiosInstance; } function params(instanceClient: AxiosInstance, progress: ImportProgressListener): ImportDataParams { return { + entityId: 'instance-1', database: 'data', table: 'dog', tableExists: true, @@ -112,3 +124,204 @@ describe('useImportDataMutation', () => { expect(progress.onSuccess).not.toHaveBeenCalled(); }); }); + +describe('useImportDataMutation refreshing Browse', () => { + afterEach(() => vi.restoreAllMocks()); + + const browse = { entityId: 'instance-1', instanceClient: {} as AxiosInstance }; + const page = { sort: { attribute: 'id', descending: false }, pageIndex: 0, pageSize: 20, onlyIfCached: true }; + // The real builders, so the keys refreshed here are the keys Browse reads. + const keys = (tableName: string) => ({ + page: getSearchByValueOptions({ + ...browse, + ...page, + enabled: true, + databaseName: 'data', + tableName, + searchAttribute: 'id', + }).queryKey, + filtered: getSearchByConditionsOptions({ + ...browse, + ...page, + enabled: true, + databaseName: 'data', + tableName, + conditions: [{ search_attribute: 'name', search_type: 'equals', search_value: 'Rex' }], + }).queryKey, + count: getTableRecordCountQueryOptions({ ...browse, enabled: true, databaseName: 'data', tableName }).queryKey, + describe: getDescribeTableQueryOptions({ ...browse, databaseName: 'data', tableName }).queryKey, + }); + const treeKey = getDescribeAllQueryOptions({ ...browse, skipRecordCount: true, includeComputed: true }).queryKey; + + function wrapperFor(client: QueryClient) { + return ({ children }: { children: ReactNode }) => ( + {children} + ); + } + + async function openBrowse(client: QueryClient, tree = vi.fn(async (): Promise => ({ dog: {} }))) { + const reads = { + page: vi.fn(async () => ({ data: [] })), + describe: vi.fn(async () => ({ record_count: 0 })), + otherPage: vi.fn(async () => ({ data: [] })), + tree, + }; + // Browse's own staleTime, so within the test only an invalidation refetches. + const staleTime = 60_000; + renderHook(() => { + useQuery({ queryKey: [...keys('dog').page], queryFn: reads.page, staleTime }); + useQuery({ queryKey: [...keys('dog').describe], queryFn: reads.describe, staleTime }); + useQuery({ queryKey: [...keys('cat').page], queryFn: reads.otherPage, staleTime }); + useQuery({ queryKey: [...treeKey], queryFn: reads.tree, staleTime }); + }, { wrapper: wrapperFor(client) }); + // Cached but not on screen, so an invalidation only marks them stale. + client.setQueryData([...keys('dog').filtered], { data: [] }); + client.setQueryData([...keys('dog').count], 5); + await vi.advanceTimersByTimeAsync(0); + for (const read of Object.values(reads)) { + expect(read).toHaveBeenCalledTimes(1); + } + return reads; + } + + function startImport(client: QueryClient, instanceClient: AxiosInstance, overrides: Partial = {}) { + const modal = renderHook(() => useImportDataMutation(), { wrapper: wrapperFor(client) }); + act(() => modal.result.current.mutate({ ...params(instanceClient, progressListener()), ...overrides })); + return modal; + } + + const rejection = (status?: number) => + new AxiosError( + status ? `Request failed with status code ${status}` : 'Network Error', + status ? AxiosError.ERR_BAD_REQUEST : AxiosError.ERR_NETWORK, + undefined, + undefined, + status ? ({ status, data: 'refused' } as AxiosResponse) : undefined, + ); + + function createsTableThenRefuses() { + const post = vi.fn(async (_url: string, body: { operation: string }) => { + if (body.operation === 'create_table') { + return { data: { message: 'table created' } }; + } + throw rejection(400); + }); + return { post } as unknown as AxiosInstance; + } + + it('refetches the imported table and the tree once the job completes, rows skipped and the modal unmounted', async () => { + const client = new QueryClient(); + const reads = await openBrowse(client); + + startImport(client, csvClient(['IN_PROGRESS', 'COMPLETE'], 'successfully loaded 3 of 5 records')).unmount(); + await vi.advanceTimersByTimeAsync(5_000); + + expect(reads.page).toHaveBeenCalledTimes(2); + expect(reads.describe).toHaveBeenCalledTimes(2); + expect(reads.tree).toHaveBeenCalledTimes(2); + expect(client.getQueryState([...keys('dog').filtered])?.isInvalidated).toBe(true); + expect(client.getQueryState([...keys('dog').count])?.isInvalidated).toBe(true); + expect(reads.otherPage).toHaveBeenCalledTimes(1); + }); + + const answeredFailures: [string, () => AxiosInstance, Partial, number][] = [ + ['the job ended in ERROR', () => csvClient(['IN_PROGRESS', 'ERROR']), {}, 5_000], + ['the wait timed out on a load still running', () => csvClient(['IN_PROGRESS']), {}, 11 * 60_000], + [ + 'Harper refused the records for a table the import had just created', + createsTableThenRefuses, + { tableExists: false, source: { kind: 'json-records', records: [{ name: 'Rex' }] } }, + 5_000, + ], + ]; + it.each(answeredFailures)('refetches too when %s', async (_case, instanceClient, overrides, waitMs) => { + const client = new QueryClient(); + const reads = await openBrowse(client); + + const modal = startImport(client, instanceClient(), overrides); + await vi.advanceTimersByTimeAsync(waitMs); + + expect(modal.result.current.isError).toBe(true); + expect(reads.page).toHaveBeenCalledTimes(2); + expect(reads.tree).toHaveBeenCalledTimes(2); + }); + + it.each([ + ['the submission got no answer', async () => { + throw rejection(); + }], + ['a gateway answered 504', async () => { + throw rejection(504); + }], + ['the polls stopped answering', async (_url: string, body: { operation: string }) => { + if (body.operation === 'csv_url_load') { + return { data: { message: 'Starting job', job_id: 'job-1' } }; + } + throw rejection(); + }], + ])('only marks Browse stale when %s', async (_case, post) => { + const client = new QueryClient(); + const reads = await openBrowse(client); + + const modal = startImport(client, { post: vi.fn(post) } as unknown as AxiosInstance); + await vi.advanceTimersByTimeAsync(10_000); + + expect(modal.result.current.isError).toBe(true); + expect(reads.page).toHaveBeenCalledTimes(1); + expect(reads.tree).toHaveBeenCalledTimes(1); + expect(client.getQueryState([...keys('dog').page])?.isInvalidated).toBe(true); + expect(client.getQueryState([...treeKey])?.isInvalidated).toBe(true); + expect(client.getQueryState([...keys('cat').page])?.isInvalidated).toBe(false); + }); + + it('settles at the job verdict, and a caller waiting for the map joins the refetch already in flight', async () => { + const client = new QueryClient(); + let landRefetch!: (map: object) => void; + const tree = vi.fn<() => Promise>() + .mockResolvedValueOnce({}) + .mockImplementationOnce(() => + new Promise((resolve) => { + landRefetch = resolve; + }) + ); + await openBrowse(client, tree); + + let mapAtNavigation: unknown; + const modal = renderHook(() => useImportDataMutation(), { wrapper: wrapperFor(client) }); + act(() => + modal.result.current.mutate(params(csvClient(['COMPLETE']), progressListener()), { + onSuccess: async () => { + await awaitDatabaseMapRefresh(client, 'instance-1'); + mapAtNavigation = client.getQueryData([...treeKey]); + }, + }) + ); + await vi.advanceTimersByTimeAsync(0); + + expect(modal.result.current.isSuccess).toBe(true); + expect(mapAtNavigation).toBeUndefined(); + + landRefetch({ dog: {} }); + await vi.advanceTimersByTimeAsync(0); + + expect(mapAtNavigation).toEqual({ dog: {} }); + expect(tree).toHaveBeenCalledTimes(2); + }); + + it('keeps a completed import successful when the refresh throws', async () => { + const debug = vi.spyOn(console, 'debug').mockImplementation(() => {}); + const client = new QueryClient(); + vi.spyOn(client, 'invalidateQueries').mockImplementation(() => { + throw new Error('cache exploded'); + }); + const progress = progressListener(); + const modal = renderHook(() => useImportDataMutation(), { wrapper: wrapperFor(client) }); + + act(() => modal.result.current.mutate(params(csvClient(['COMPLETE']), progress))); + await vi.advanceTimersByTimeAsync(5_000); + + expect(modal.result.current.isSuccess).toBe(true); + expect(progress.onError).not.toHaveBeenCalled(); + expect(debug).toHaveBeenCalledWith('Refreshing the imported table failed', expect.any(Error)); + }); +}); diff --git a/src/integrations/api/instance/database/importData.ts b/src/integrations/api/instance/database/importData.ts index ff344f8f7..11ff7a27d 100644 --- a/src/integrations/api/instance/database/importData.ts +++ b/src/integrations/api/instance/database/importData.ts @@ -1,7 +1,8 @@ -import { InstanceClientConfig } from '@/config/instanceClientConfig'; +import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; +import { NO_VERDICT_STATUSES } from '@/integrations/api/unansweredWrite'; import { errorText } from '@/lib/errorText'; -import { useMutation } from '@tanstack/react-query'; -import { AxiosError } from 'axios'; +import { type QueryClient, useMutation, useQueryClient } from '@tanstack/react-query'; +import { AxiosError, isAxiosError } from 'axios'; import { onAddCSVDataSubmit } from './addCSVData'; import { type HarperJob, waitForJob } from './getJob'; import { insertTableRecords } from './insertTableRecords'; @@ -11,7 +12,7 @@ export type ImportSource = | { kind: 'csv-url'; url: string } | { kind: 'json-records'; records: object[] }; -export interface ImportDataParams extends InstanceClientConfig { +export interface ImportDataParams extends InstanceClientIdConfig { database: string; table: string; /** Skip the create_table round-trip when the caller already knows the table exists. */ @@ -46,7 +47,7 @@ function extractErrorMessage(err: unknown): string { /** * Loads data into a table, creating the table first when needed. CSV loads are async * on the Harper side (they return a job id), so this resolves only once the job - * finishes — callers can refresh the table immediately on success. + * finishes. {@link useImportDataMutation} re-reads the table when it does. */ export async function importData({ database, @@ -113,20 +114,72 @@ export async function importData({ } } -/** A throw from a mutation callback becomes the mutation's error, so a progress listener that fails - * would report an import Harper completed as failed and invite a duplicating retry. */ -function settleQuietly(settle: () => void) { +/** A throw from a mutation callback becomes the mutation's error, so a progress listener or a refresh + * that fails would report an import Harper completed as failed and invite a duplicating retry. */ +function settleQuietly(what: string, settle: () => void) { try { settle(); } catch (err) { - console.debug('Import progress listener failed', err); + console.debug(`${what} failed`, err); } } +/** describe_table sits under the prefix, and the database overview totals its counts. describe_all + * gains the table when the import created it. */ +function refreshImportedTable( + queryClient: QueryClient, + { entityId, database, table }: Pick, + refetchNow: boolean, +) { + void queryClient.invalidateQueries({ + queryKey: [entityId, database, table], + refetchType: refetchNow ? 'active' : 'none', + }); + void queryClient.invalidateQueries({ + queryKey: [entityId, 'describe_all'], + refetchType: refetchNow ? 'all' : 'none', + }); +} + +/** + * Resolves once the describe_all refetch {@link useImportDataMutation} started has settled, joining it + * rather than replacing it, so navigating to a table the import created finds it in the map instead of + * being redirected off it (`resolveDatabasesRedirect`). A refetch that fails still resolves, and leaves + * the old map. + */ +export function awaitDatabaseMapRefresh(queryClient: QueryClient, entityId: ImportDataParams['entityId']) { + return queryClient.invalidateQueries( + { queryKey: [entityId, 'describe_all'], refetchType: 'all' }, + { cancelRefetch: false }, + ); +} + +/** + * Any outcome can have changed the table: a job that ended in `ERROR` keeps the chunks `csv_url_load` + * wrote before the failing one (harper `dataLayer/bulkLoad.ts`, `insertChunk`), and a refused load can + * follow the `create_table` that made its table. But without a verdict (no response, or a 502/503/504) + * re-reading now would fail the same way and stack its own error toasts on the import's, so the reads + * are only marked stale. + */ +function instanceAnswered(error: Error | null): boolean { + if (!isAxiosError(error)) { + return true; + } + const status = error.response?.status; + return status !== undefined && !NO_VERDICT_STATUSES.has(status); +} + export function useImportDataMutation() { + const queryClient = useQueryClient(); return useMutation({ mutationFn: importData, - onSuccess: (result, { progress }) => settleQuietly(() => progress?.onSuccess(result)), - onError: (_error, { progress }) => settleQuietly(() => progress?.onError()), + onSuccess: (result, { progress }) => settleQuietly('Import progress listener', () => progress?.onSuccess(result)), + onError: (_error, { progress }) => settleQuietly('Import progress listener', () => progress?.onError()), + // Not awaited, so a slow re-read cannot hold the modal on "Importing…". See the databases DESIGN.md. + onSettled: (_result, error, params) => + settleQuietly( + 'Refreshing the imported table', + () => refreshImportedTable(queryClient, params, instanceAnswered(error)), + ), }); } diff --git a/src/integrations/api/unansweredWrite.ts b/src/integrations/api/unansweredWrite.ts index d117b5eae..a7a37e8b2 100644 --- a/src/integrations/api/unansweredWrite.ts +++ b/src/integrations/api/unansweredWrite.ts @@ -26,7 +26,7 @@ export const NO_RESPONSE_CODES: ReadonlySet = new Set([ // A proxy answers these for an upstream that may already have the request (central manager's // operations proxy sends 504 after forwarding it), and Harper answers 503 when the worker it handed an // operation to exits or overruns. The same reasoning stops the gateway retry from replaying a write. -const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); +export const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); /** Its failure must not read as a plain failure: it may have been applied, so a retry can apply it twice. */ export function isUnansweredWrite(rawErr: unknown): rawErr is AxiosError { From 3d8f58efc10e0bf65b62d14695cfc3caaa420321 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 13 Oct 2026 12:54:45 -0400 Subject: [PATCH 2/2] fix(browse): re-read the imported table after a no-verdict failure too The import refresh only marked Browse stale, without refetching, when a failure had no verdict (no response, or a 502/503/504). But that is the case whose toast says "Outcome unknown" and asks the user to check whether the import took effect. For example, central manager can answer 504 on a long insert that Harper goes on to apply. The open grid then kept the cached rows from before the import, so the check found nothing new, and a retry would insert every record without an id a second time. Every outcome now refetches what is on screen. When the instance really is down, the re-reads fail too and add their own error toasts, which is cheaper than a grid that invites a duplicating retry. invalidateQueries does not reject on a failed refetch unless throwOnError is set, so a failed re-read is not an unhandled rejection; a test pins that. The refresh also invalidates the open record's search_by_id key, which the table prefix does not reach. Every import only inserts, so this only matters for a cached "not found" for an id the import then wrote. NO_VERDICT_STATUSES goes back to being private to unansweredWrite.ts. Co-Authored-By: Claude Opus 5.5 --- src/features/instance/databases/DESIGN.md | 12 ++-- .../api/instance/database/importData.test.tsx | 66 +++++++++++-------- .../api/instance/database/importData.ts | 43 +++--------- src/integrations/api/unansweredWrite.ts | 2 +- 4 files changed, 56 insertions(+), 67 deletions(-) diff --git a/src/features/instance/databases/DESIGN.md b/src/features/instance/databases/DESIGN.md index c67dba9a4..f65c4b728 100644 --- a/src/features/instance/databases/DESIGN.md +++ b/src/features/instance/databases/DESIGN.md @@ -106,7 +106,11 @@ It is not awaited. The import settles at the job's verdict, and the modal closes `resolveDatabasesRedirect` would send a table the import just created back to the overview. If that refetch fails, the old map stays and the redirect still happens, as it did before. -A failed import refreshes too, because a CSV loaded from a URL that fails partway leaves its earlier -chunks written. When the failure has no verdict (no response, or a 502/503/504), the reads are only -marked stale rather than refetched: they would fail too and add their own error toasts, and the next -mount, refocus or Refresh fetches them. +A failed import refreshes too, because any outcome can have written. `csv_url_load` inserts chunk by +chunk (harper `dataLayer/bulkLoad.ts`, `insertChunk`), so a load that fails partway keeps its earlier +chunks, and a refused load can follow the `create_table` that made its table. A submission with no +verdict is the strongest case: its toast says "Outcome unknown" and asks the user to check whether the +import took effect (`isUnansweredWrite`). A central-manager 504 on a long insert that Harper went on to apply is one +example, and only a re-read shows the answer. When the instance is really down, the re-reads fail +too and add their own error toasts. That noise is cheaper than a stale grid inviting a retry that +duplicates every record without an id. diff --git a/src/integrations/api/instance/database/importData.test.tsx b/src/integrations/api/instance/database/importData.test.tsx index 048a0b67f..ba95efac7 100644 --- a/src/integrations/api/instance/database/importData.test.tsx +++ b/src/integrations/api/instance/database/importData.test.tsx @@ -7,6 +7,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { getDescribeAllQueryOptions } from './getDescribeAll'; import { getDescribeTableQueryOptions } from './getDescribeTable'; import { getSearchByConditionsOptions } from './getSearchByConditions'; +import { getSearchByIdOptions } from './getSearchById'; import { getSearchByValueOptions } from './getSearchByValue'; import { getTableRecordCountQueryOptions } from './getTableRecordCount'; import { @@ -150,6 +151,8 @@ describe('useImportDataMutation refreshing Browse', () => { }).queryKey, count: getTableRecordCountQueryOptions({ ...browse, enabled: true, databaseName: 'data', tableName }).queryKey, describe: getDescribeTableQueryOptions({ ...browse, databaseName: 'data', tableName }).queryKey, + record: getSearchByIdOptions({ ...browse, enabled: true, databaseName: 'data', tableName, ids: ['rex'] }) + .queryKey, }); const treeKey = getDescribeAllQueryOptions({ ...browse, skipRecordCount: true, includeComputed: true }).queryKey; @@ -166,17 +169,18 @@ describe('useImportDataMutation refreshing Browse', () => { otherPage: vi.fn(async () => ({ data: [] })), tree, }; - // Browse's own staleTime, so within the test only an invalidation refetches. - const staleTime = 60_000; + // Browse's own options, so within the test only an invalidation refetches, and once. + const browseOptions = { staleTime: 60_000, retry: false } as const; renderHook(() => { - useQuery({ queryKey: [...keys('dog').page], queryFn: reads.page, staleTime }); - useQuery({ queryKey: [...keys('dog').describe], queryFn: reads.describe, staleTime }); - useQuery({ queryKey: [...keys('cat').page], queryFn: reads.otherPage, staleTime }); - useQuery({ queryKey: [...treeKey], queryFn: reads.tree, staleTime }); + useQuery({ queryKey: [...keys('dog').page], queryFn: reads.page, ...browseOptions }); + useQuery({ queryKey: [...keys('dog').describe], queryFn: reads.describe, ...browseOptions }); + useQuery({ queryKey: [...keys('cat').page], queryFn: reads.otherPage, ...browseOptions }); + useQuery({ queryKey: [...treeKey], queryFn: reads.tree, ...browseOptions }); }, { wrapper: wrapperFor(client) }); // Cached but not on screen, so an invalidation only marks them stale. client.setQueryData([...keys('dog').filtered], { data: [] }); client.setQueryData([...keys('dog').count], 5); + client.setQueryData([...keys('dog').record], { data: [] }); await vi.advanceTimersByTimeAsync(0); for (const read of Object.values(reads)) { expect(read).toHaveBeenCalledTimes(1); @@ -209,6 +213,16 @@ describe('useImportDataMutation refreshing Browse', () => { return { post } as unknown as AxiosInstance; } + function pollsStopAnswering() { + const post = vi.fn(async (_url: string, body: { operation: string }) => { + if (body.operation === 'csv_url_load') { + return { data: { message: 'Starting job', job_id: 'job-1' } }; + } + throw rejection(); + }); + return { post } as unknown as AxiosInstance; + } + it('refetches the imported table and the tree once the job completes, rows skipped and the modal unmounted', async () => { const client = new QueryClient(); const reads = await openBrowse(client); @@ -221,10 +235,17 @@ describe('useImportDataMutation refreshing Browse', () => { expect(reads.tree).toHaveBeenCalledTimes(2); expect(client.getQueryState([...keys('dog').filtered])?.isInvalidated).toBe(true); expect(client.getQueryState([...keys('dog').count])?.isInvalidated).toBe(true); + expect(client.getQueryState([...keys('dog').record])?.isInvalidated).toBe(true); expect(reads.otherPage).toHaveBeenCalledTimes(1); }); - const answeredFailures: [string, () => AxiosInstance, Partial, number][] = [ + const rejectAll = (status?: number) => () => + ({ + post: vi.fn(async () => { + throw rejection(status); + }), + }) as unknown as AxiosInstance; + const failures: [string, () => AxiosInstance, Partial, number][] = [ ['the job ended in ERROR', () => csvClient(['IN_PROGRESS', 'ERROR']), {}, 5_000], ['the wait timed out on a load still running', () => csvClient(['IN_PROGRESS']), {}, 11 * 60_000], [ @@ -233,8 +254,11 @@ describe('useImportDataMutation refreshing Browse', () => { { tableExists: false, source: { kind: 'json-records', records: [{ name: 'Rex' }] } }, 5_000, ], + ['the submission got no answer', rejectAll(), {}, 5_000], + ['a gateway answered 504', rejectAll(504), {}, 5_000], + ['the polls stopped answering', pollsStopAnswering, {}, 10_000], ]; - it.each(answeredFailures)('refetches too when %s', async (_case, instanceClient, overrides, waitMs) => { + it.each(failures)('refetches too when %s', async (_case, instanceClient, overrides, waitMs) => { const client = new QueryClient(); const reads = await openBrowse(client); @@ -246,32 +270,18 @@ describe('useImportDataMutation refreshing Browse', () => { expect(reads.tree).toHaveBeenCalledTimes(2); }); - it.each([ - ['the submission got no answer', async () => { - throw rejection(); - }], - ['a gateway answered 504', async () => { - throw rejection(504); - }], - ['the polls stopped answering', async (_url: string, body: { operation: string }) => { - if (body.operation === 'csv_url_load') { - return { data: { message: 'Starting job', job_id: 'job-1' } }; - } - throw rejection(); - }], - ])('only marks Browse stale when %s', async (_case, post) => { + it('keeps re-reads that fail with the instance from escaping as unhandled rejections', async () => { const client = new QueryClient(); const reads = await openBrowse(client); + reads.page.mockRejectedValue(rejection()); + reads.tree.mockRejectedValue(rejection()); - const modal = startImport(client, { post: vi.fn(post) } as unknown as AxiosInstance); + const modal = startImport(client, pollsStopAnswering()); await vi.advanceTimersByTimeAsync(10_000); expect(modal.result.current.isError).toBe(true); - expect(reads.page).toHaveBeenCalledTimes(1); - expect(reads.tree).toHaveBeenCalledTimes(1); - expect(client.getQueryState([...keys('dog').page])?.isInvalidated).toBe(true); - expect(client.getQueryState([...treeKey])?.isInvalidated).toBe(true); - expect(client.getQueryState([...keys('cat').page])?.isInvalidated).toBe(false); + expect(client.getQueryState([...keys('dog').page])?.status).toBe('error'); + expect(client.getQueryState([...treeKey])?.status).toBe('error'); }); it('settles at the job verdict, and a caller waiting for the map joins the refetch already in flight', async () => { diff --git a/src/integrations/api/instance/database/importData.ts b/src/integrations/api/instance/database/importData.ts index 11ff7a27d..28ae98e7a 100644 --- a/src/integrations/api/instance/database/importData.ts +++ b/src/integrations/api/instance/database/importData.ts @@ -1,10 +1,10 @@ import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; -import { NO_VERDICT_STATUSES } from '@/integrations/api/unansweredWrite'; import { errorText } from '@/lib/errorText'; import { type QueryClient, useMutation, useQueryClient } from '@tanstack/react-query'; -import { AxiosError, isAxiosError } from 'axios'; +import { AxiosError } from 'axios'; import { onAddCSVDataSubmit } from './addCSVData'; import { type HarperJob, waitForJob } from './getJob'; +import { searchByIdInvalidationKey } from './getSearchById'; import { insertTableRecords } from './insertTableRecords'; export type ImportSource = @@ -124,21 +124,13 @@ function settleQuietly(what: string, settle: () => void) { } } -/** describe_table sits under the prefix, and the database overview totals its counts. describe_all - * gains the table when the import created it. */ function refreshImportedTable( queryClient: QueryClient, { entityId, database, table }: Pick, - refetchNow: boolean, ) { - void queryClient.invalidateQueries({ - queryKey: [entityId, database, table], - refetchType: refetchNow ? 'active' : 'none', - }); - void queryClient.invalidateQueries({ - queryKey: [entityId, 'describe_all'], - refetchType: refetchNow ? 'all' : 'none', - }); + void queryClient.invalidateQueries({ queryKey: [entityId, database, table] }); + void queryClient.invalidateQueries({ queryKey: searchByIdInvalidationKey(entityId, database, table) }); + void queryClient.invalidateQueries({ queryKey: [entityId, 'describe_all'], refetchType: 'all' }); } /** @@ -154,32 +146,15 @@ export function awaitDatabaseMapRefresh(queryClient: QueryClient, entityId: Impo ); } -/** - * Any outcome can have changed the table: a job that ended in `ERROR` keeps the chunks `csv_url_load` - * wrote before the failing one (harper `dataLayer/bulkLoad.ts`, `insertChunk`), and a refused load can - * follow the `create_table` that made its table. But without a verdict (no response, or a 502/503/504) - * re-reading now would fail the same way and stack its own error toasts on the import's, so the reads - * are only marked stale. - */ -function instanceAnswered(error: Error | null): boolean { - if (!isAxiosError(error)) { - return true; - } - const status = error.response?.status; - return status !== undefined && !NO_VERDICT_STATUSES.has(status); -} - export function useImportDataMutation() { const queryClient = useQueryClient(); return useMutation({ mutationFn: importData, onSuccess: (result, { progress }) => settleQuietly('Import progress listener', () => progress?.onSuccess(result)), onError: (_error, { progress }) => settleQuietly('Import progress listener', () => progress?.onError()), - // Not awaited, so a slow re-read cannot hold the modal on "Importing…". See the databases DESIGN.md. - onSettled: (_result, error, params) => - settleQuietly( - 'Refreshing the imported table', - () => refreshImportedTable(queryClient, params, instanceAnswered(error)), - ), + // After a failure too: any outcome can have written (see the databases DESIGN.md). Not awaited, so a + // slow re-read cannot hold the modal on "Importing…". + onSettled: (_result, _error, params) => + settleQuietly('Refreshing the imported table', () => refreshImportedTable(queryClient, params)), }); } diff --git a/src/integrations/api/unansweredWrite.ts b/src/integrations/api/unansweredWrite.ts index a7a37e8b2..d117b5eae 100644 --- a/src/integrations/api/unansweredWrite.ts +++ b/src/integrations/api/unansweredWrite.ts @@ -26,7 +26,7 @@ export const NO_RESPONSE_CODES: ReadonlySet = new Set([ // A proxy answers these for an upstream that may already have the request (central manager's // operations proxy sends 504 after forwarding it), and Harper answers 503 when the worker it handed an // operation to exits or overruns. The same reasoning stops the gateway retry from replaying a write. -export const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); +const NO_VERDICT_STATUSES: ReadonlySet = new Set([502, 503, 504]); /** Its failure must not read as a plain failure: it may have been applied, so a retry can apply it twice. */ export function isUnansweredWrite(rawErr: unknown): rawErr is AxiosError {