Skip to content

DEMOS-1043: Remove debug statements and fix refetchQueries #546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 26, 2025
Merged

DEMOS-1043: Remove debug statements and fix refetchQueries #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c95213d
Fix refetchQueries!
tresdonicf Nov 25, 2025
8c7cd37
remove console.debug
tresdonicf Nov 25, 2025
7234e52
Fix tests
tresdonicf Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 119 additions & 9 deletions client/src/components/dialog/document/AddDocumentDialog.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,31 @@ import { describe, expect, it, vi } from "vitest";

import { fireEvent, render, screen, waitFor } from "@testing-library/react";

import { AddDocumentDialog, tryUploadingFileToS3 } from "./AddDocumentDialog";
import {
AddDocumentDialog,
DOCUMENT_POLL_INTERVAL_MS,
tryUploadingFileToS3,
VIRUS_SCAN_MAX_ATTEMPTS,
} from "./AddDocumentDialog";

let mockMutationFn = vi.fn();
let mockLazyQueryFn = vi.fn();
let mockRefetchQueries = vi.fn();

beforeEach(() => {
mockMutationFn = vi.fn();
mockLazyQueryFn = vi.fn();
mockRefetchQueries = vi.fn();

vi.mock("@apollo/client", async () => {
const actual = await vi.importActual("@apollo/client");
return {
...actual,
useMutation: () => [mockMutationFn, { loading: false }],
useLazyQuery: () => [mockLazyQueryFn, { loading: false }],
useApolloClient: () => ({
refetchQueries: mockRefetchQueries,
}),
};
});
});
Expand Down Expand Up @@ -220,7 +230,7 @@ describe("virus scan polling", () => {

await clickPromise;
// Advance timer to allow polling to complete
await vi.advanceTimersByTimeAsync(1000);
await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS);
Copy link
Contributor Author

@tresdonicf tresdonicf Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests now reference the real values in case those change the tests will still be accurate


expect(mockLazyQueryFn).toHaveBeenCalledWith({
variables: { documentId: "test-doc-id" },
Expand Down Expand Up @@ -277,9 +287,9 @@ describe("virus scan polling", () => {

await clickPromise;
// Advance timers for each polling attempt (3 total)
await vi.advanceTimersByTimeAsync(1000); // 1st poll (fails)
await vi.advanceTimersByTimeAsync(1000); // 2nd poll (fails)
await vi.advanceTimersByTimeAsync(1000); // 3rd poll (succeeds)
await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS); // 1st poll (fails)
await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS); // 2nd poll (fails)
await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS); // 3rd poll (succeeds)

expect(mockLazyQueryFn).toHaveBeenCalledTimes(3);
expect(onDocumentUploadSucceeded).toHaveBeenCalled();
Expand Down Expand Up @@ -325,11 +335,11 @@ describe("virus scan polling", () => {
});

await clickPromise;
// Advance timers to reach max attempts (10 * 1000ms)
await vi.advanceTimersByTimeAsync(10000);
// Advance timers to reach max attempts
await vi.advanceTimersByTimeAsync(VIRUS_SCAN_MAX_ATTEMPTS * DOCUMENT_POLL_INTERVAL_MS);

// Should reach max attempts (10) and throw timeout error
expect(mockLazyQueryFn).toHaveBeenCalledTimes(10);
// Should reach max attempts and throw timeout error
expect(mockLazyQueryFn).toHaveBeenCalledTimes(VIRUS_SCAN_MAX_ATTEMPTS);
});

it("skips virus scan polling for localhost uploads", async () => {
Expand Down Expand Up @@ -376,6 +386,106 @@ describe("virus scan polling", () => {
// Should not poll for virus scan in localhost mode
expect(mockLazyQueryFn).not.toHaveBeenCalled();
});

it("refetches queries after successful upload when refetchQueries is provided", async () => {
mockMutationFn.mockResolvedValue({
data: {
uploadDocument: {
presignedURL: "https://s3.amazonaws.com/test-bucket/test-file",
documentId: "test-doc-id",
},
},
});

mockLazyQueryFn.mockResolvedValue({
data: { documentExists: true },
});

vi.mocked(globalThis.fetch).mockResolvedValue({ ok: true } as Response);

const onDocumentUploadSucceeded = vi.fn();
const refetchQueries = ["GetDocuments", "GetApplicationDocuments"];

render(
<ToastProvider>
<AddDocumentDialog
onClose={vi.fn()}
applicationId="test-app-id"
onDocumentUploadSucceeded={onDocumentUploadSucceeded}
documentTypeSubset={["General File"]}
refetchQueries={refetchQueries}
/>
</ToastProvider>
);

const file = new File(["content"], "test.pdf", { type: "application/pdf" });
fireEvent.change(screen.getByTestId(FILE_INPUT_TEST_ID), {
target: { files: [file] },
});

const uploadBtn = screen.getByTestId(UPLOAD_DOCUMENT_BUTTON_TEST_ID);
await waitFor(() => expect(uploadBtn).toBeEnabled());

const clickPromise = new Promise<void>((resolve) => {
fireEvent.click(uploadBtn);
setTimeout(() => resolve(), 0);
});

await clickPromise;
await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS);

expect(onDocumentUploadSucceeded).toHaveBeenCalled();
expect(mockRefetchQueries).toHaveBeenCalledWith({ include: refetchQueries });
});

it("does not call refetchQueries when not provided", async () => {
mockMutationFn.mockResolvedValue({
data: {
uploadDocument: {
presignedURL: "https://s3.amazonaws.com/test-bucket/test-file",
documentId: "test-doc-id",
},
},
});

mockLazyQueryFn.mockResolvedValue({
data: { documentExists: true },
});

vi.mocked(globalThis.fetch).mockResolvedValue({ ok: true } as Response);

const onDocumentUploadSucceeded = vi.fn();

render(
<ToastProvider>
<AddDocumentDialog
onClose={vi.fn()}
applicationId="test-app-id"
onDocumentUploadSucceeded={onDocumentUploadSucceeded}
documentTypeSubset={["General File"]}
/>
</ToastProvider>
);

const file = new File(["content"], "test.pdf", { type: "application/pdf" });
fireEvent.change(screen.getByTestId(FILE_INPUT_TEST_ID), {
target: { files: [file] },
});

const uploadBtn = screen.getByTestId(UPLOAD_DOCUMENT_BUTTON_TEST_ID);
await waitFor(() => expect(uploadBtn).toBeEnabled());

const clickPromise = new Promise<void>((resolve) => {
fireEvent.click(uploadBtn);
setTimeout(() => resolve(), 0);
});

await clickPromise;
await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS);

expect(onDocumentUploadSucceeded).toHaveBeenCalled();
expect(mockRefetchQueries).not.toHaveBeenCalled();
});
});

describe("tryUploadingFileToS3", () => {
Expand Down
35 changes: 11 additions & 24 deletions client/src/components/dialog/document/AddDocumentDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from "react";
import { gql, useLazyQuery, useMutation } from "@apollo/client";
import { gql, useLazyQuery, useMutation, useApolloClient } from "@apollo/client";

import { DocumentType, PhaseName, UploadDocumentInput } from "demos-server";
import { DocumentDialog, DocumentDialogFields } from "components/dialog/document/DocumentDialog";
Expand All @@ -20,8 +20,8 @@ export const DOCUMENT_EXISTS_QUERY = gql`
}
`;

const VIRUS_SCAN_MAX_ATTEMPTS = 10;
const DOCUMENT_POLL_INTERVAL_MS = 1_000;
export const VIRUS_SCAN_MAX_ATTEMPTS = 10;
export const DOCUMENT_POLL_INTERVAL_MS = 2_000;
const LOCALHOST_URL_PREFIX = "http://localhost";

/**
Expand Down Expand Up @@ -70,9 +70,8 @@ export const AddDocumentDialog: React.FC<AddDocumentDialogProps> = ({
onDocumentUploadSucceeded,
}) => {
const { showError } = useToast();
const [uploadDocumentTrigger] = useMutation(UPLOAD_DOCUMENT_QUERY, {
refetchQueries,
});
const client = useApolloClient();
const [uploadDocumentTrigger] = useMutation(UPLOAD_DOCUMENT_QUERY);

const [checkDocumentExists] = useLazyQuery(DOCUMENT_EXISTS_QUERY, {
fetchPolicy: "network-only",
Expand All @@ -89,32 +88,22 @@ export const AddDocumentDialog: React.FC<AddDocumentDialogProps> = ({
};

const waitForVirusScan = async (documentId: string): Promise<void> => {
console.debug(`[AddDocumentDialog] Starting virus scan polling for document: ${documentId}`);
for (let attempt = 0; attempt < VIRUS_SCAN_MAX_ATTEMPTS; attempt++) {
console.debug(
`[AddDocumentDialog] Polling attempt ${attempt + 1}/${VIRUS_SCAN_MAX_ATTEMPTS}`
);
const { data } = await checkDocumentExists({
variables: { documentId },
});

if (data?.documentExists === true) {
console.debug(`[AddDocumentDialog] Document exists check passed on attempt ${attempt + 1}`);
return;
}

console.debug(
`[AddDocumentDialog] Document not yet available, waiting ${DOCUMENT_POLL_INTERVAL_MS}ms before retry`
);

await new Promise((resolve) => setTimeout(resolve, DOCUMENT_POLL_INTERVAL_MS));
}

throw new Error("Waiting for virus scan timed out");
};

const handleUpload = async (dialogFields: DocumentDialogFields): Promise<void> => {
console.debug(`[AddDocumentDialog] Starting upload for file: ${dialogFields.file?.name}`);
if (!dialogFields.file) {
showError("No file selected");
return;
Expand Down Expand Up @@ -146,32 +135,30 @@ export const AddDocumentDialog: React.FC<AddDocumentDialogProps> = ({
throw new Error("Upload response from the server was empty");
}

console.debug(
`[AddDocumentDialog] Received presigned URL and documentId: ${uploadResult.documentId}`
);

// Local development mode - skip S3 upload and virus scan
if (uploadResult.presignedURL.startsWith(LOCALHOST_URL_PREFIX)) {
onDocumentUploadSucceeded?.();
if (refetchQueries) {
await client.refetchQueries({ include: refetchQueries });
}
Comment on lines +141 to +143
Copy link
Contributor

@connor-parke-icf connor-parke-icf Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels like there may be a path more intended by apollo for updating frontend data from the result of a polled query, where updates to data arent explicitly caused by mutations. This might be a good example where a direct cache invalidation may be preferred, but its hard to say. I feel like we should avoid direct calls to the apolloclient so far outside the core components, but i cant come up with any concrete reasons why or why not.

Copy link
Contributor Author

@tresdonicf tresdonicf Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed to be the most clear route from what I could find but there certainly could be something else out there! I think this pattern is pretty solid for "disjointed" systems where you don't quite know when some sort of data will be available so you just allow the user to call a refresh when they need it 🤷

return;
}

if (!uploadResult.presignedURL) {
throw new Error("Could not get presigned URL from the server");
}

console.debug(`[AddDocumentDialog] Starting S3 upload for file: ${dialogFields.file.name}`);
const response = await tryUploadingFileToS3(uploadResult.presignedURL, dialogFields.file);
if (!response.success) {
console.debug(`[AddDocumentDialog] S3 upload failed: ${response.errorMessage}`);
showError(response.errorMessage);
throw new Error(response.errorMessage);
}

console.debug("[AddDocumentDialog] S3 upload successful, starting virus scan wait");
await waitForVirusScan(uploadResult.documentId);
console.debug("[AddDocumentDialog] Upload and virus scan completed successfully");
onDocumentUploadSucceeded?.();
if (refetchQueries) {
await client.refetchQueries({ include: refetchQueries });
}
};

return (
Expand Down