diff --git a/src/features/instance/databases/DESIGN.md b/src/features/instance/databases/DESIGN.md index f24e94d16..f65c4b728 100644 --- a/src/features/instance/databases/DESIGN.md +++ b/src/features/instance/databases/DESIGN.md @@ -90,3 +90,27 @@ 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 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/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..ba95efac7 100644 --- a/src/integrations/api/instance/database/importData.test.tsx +++ b/src/integrations/api/instance/database/importData.test.tsx @@ -1,10 +1,22 @@ // @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 { getSearchByIdOptions } from './getSearchById'; +import { getSearchByValueOptions } from './getSearchByValue'; +import { getTableRecordCountQueryOptions } from './getTableRecordCount'; +import { + awaitDatabaseMapRefresh, + importData, + type ImportDataParams, + type ImportProgressListener, + useImportDataMutation, +} from './importData'; function progressListener() { return { @@ -14,7 +26,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 +34,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 +125,213 @@ 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, + record: getSearchByIdOptions({ ...browse, enabled: true, databaseName: 'data', tableName, ids: ['rex'] }) + .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 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, ...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); + } + 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; + } + + 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); + + 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(client.getQueryState([...keys('dog').record])?.isInvalidated).toBe(true); + expect(reads.otherPage).toHaveBeenCalledTimes(1); + }); + + 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], + [ + 'Harper refused the records for a table the import had just created', + createsTableThenRefuses, + { 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(failures)('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('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, pollsStopAnswering()); + await vi.advanceTimersByTimeAsync(10_000); + + expect(modal.result.current.isError).toBe(true); + 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 () => { + 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..28ae98e7a 100644 --- a/src/integrations/api/instance/database/importData.ts +++ b/src/integrations/api/instance/database/importData.ts @@ -1,9 +1,10 @@ -import { InstanceClientConfig } from '@/config/instanceClientConfig'; +import { InstanceClientIdConfig } from '@/config/instanceClientConfig'; import { errorText } from '@/lib/errorText'; -import { useMutation } from '@tanstack/react-query'; +import { type QueryClient, useMutation, useQueryClient } from '@tanstack/react-query'; 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 = @@ -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,47 @@ 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); } } +function refreshImportedTable( + queryClient: QueryClient, + { entityId, database, table }: Pick, +) { + void queryClient.invalidateQueries({ queryKey: [entityId, database, table] }); + void queryClient.invalidateQueries({ queryKey: searchByIdInvalidationKey(entityId, database, table) }); + void queryClient.invalidateQueries({ queryKey: [entityId, 'describe_all'], refetchType: 'all' }); +} + +/** + * 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 }, + ); +} + 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()), + // 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)), }); }