fix(CQ-WEB-3): Fix race condition in LinkAutocomplete
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Add AbortController to cancel in-flight search requests when a new search fires, preventing stale results from overwriting newer ones. The controller is also aborted on component unmount for cleanup. Switched from apiGet to apiRequest to support passing AbortSignal. Added 3 new tests verifying signal passing, abort on new search, and abort on unmount. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
"use client";
|
"use client";
|
||||||
|
|
||||||
import React, { useState, useEffect, useRef, useCallback } from "react";
|
import React, { useState, useEffect, useRef, useCallback } from "react";
|
||||||
import { apiGet } from "@/lib/api/client";
|
import { apiRequest } from "@/lib/api/client";
|
||||||
import type { KnowledgeEntryWithTags } from "@mosaic/shared";
|
import type { KnowledgeEntryWithTags } from "@mosaic/shared";
|
||||||
|
|
||||||
interface LinkAutocompleteProps {
|
interface LinkAutocompleteProps {
|
||||||
@@ -51,11 +51,14 @@ export function LinkAutocomplete({
|
|||||||
const [isLoading, setIsLoading] = useState<boolean>(false);
|
const [isLoading, setIsLoading] = useState<boolean>(false);
|
||||||
const dropdownRef = useRef<HTMLDivElement>(null);
|
const dropdownRef = useRef<HTMLDivElement>(null);
|
||||||
const searchTimeoutRef = useRef<NodeJS.Timeout | null>(null);
|
const searchTimeoutRef = useRef<NodeJS.Timeout | null>(null);
|
||||||
|
const abortControllerRef = useRef<AbortController | null>(null);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Search for knowledge entries matching the query
|
* Search for knowledge entries matching the query.
|
||||||
|
* Accepts an AbortSignal to allow cancellation of in-flight requests,
|
||||||
|
* preventing stale results from overwriting newer ones.
|
||||||
*/
|
*/
|
||||||
const searchEntries = useCallback(async (query: string): Promise<void> => {
|
const searchEntries = useCallback(async (query: string, signal: AbortSignal): Promise<void> => {
|
||||||
if (!query.trim()) {
|
if (!query.trim()) {
|
||||||
setResults([]);
|
setResults([]);
|
||||||
return;
|
return;
|
||||||
@@ -63,7 +66,7 @@ export function LinkAutocomplete({
|
|||||||
|
|
||||||
setIsLoading(true);
|
setIsLoading(true);
|
||||||
try {
|
try {
|
||||||
const response = await apiGet<{
|
const response = await apiRequest<{
|
||||||
data: KnowledgeEntryWithTags[];
|
data: KnowledgeEntryWithTags[];
|
||||||
meta: {
|
meta: {
|
||||||
total: number;
|
total: number;
|
||||||
@@ -71,7 +74,10 @@ export function LinkAutocomplete({
|
|||||||
limit: number;
|
limit: number;
|
||||||
totalPages: number;
|
totalPages: number;
|
||||||
};
|
};
|
||||||
}>(`/api/knowledge/search?q=${encodeURIComponent(query)}&limit=10`);
|
}>(`/api/knowledge/search?q=${encodeURIComponent(query)}&limit=10`, {
|
||||||
|
method: "GET",
|
||||||
|
signal,
|
||||||
|
});
|
||||||
|
|
||||||
const searchResults: SearchResult[] = response.data.map((entry) => ({
|
const searchResults: SearchResult[] = response.data.map((entry) => ({
|
||||||
id: entry.id,
|
id: entry.id,
|
||||||
@@ -83,15 +89,23 @@ export function LinkAutocomplete({
|
|||||||
setResults(searchResults);
|
setResults(searchResults);
|
||||||
setSelectedIndex(0);
|
setSelectedIndex(0);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
// Ignore aborted requests - a newer search has superseded this one
|
||||||
|
if (error instanceof DOMException && error.name === "AbortError") {
|
||||||
|
return;
|
||||||
|
}
|
||||||
console.error("Failed to search entries:", error);
|
console.error("Failed to search entries:", error);
|
||||||
setResults([]);
|
setResults([]);
|
||||||
} finally {
|
} finally {
|
||||||
setIsLoading(false);
|
if (!signal.aborted) {
|
||||||
|
setIsLoading(false);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Debounced search - waits 300ms after user stops typing
|
* Debounced search - waits 300ms after user stops typing.
|
||||||
|
* Cancels any in-flight request via AbortController before firing a new one,
|
||||||
|
* preventing race conditions where older results overwrite newer ones.
|
||||||
*/
|
*/
|
||||||
const debouncedSearch = useCallback(
|
const debouncedSearch = useCallback(
|
||||||
(query: string): void => {
|
(query: string): void => {
|
||||||
@@ -99,8 +113,16 @@ export function LinkAutocomplete({
|
|||||||
clearTimeout(searchTimeoutRef.current);
|
clearTimeout(searchTimeoutRef.current);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Abort any in-flight request from a previous search
|
||||||
|
if (abortControllerRef.current) {
|
||||||
|
abortControllerRef.current.abort();
|
||||||
|
}
|
||||||
|
|
||||||
searchTimeoutRef.current = setTimeout(() => {
|
searchTimeoutRef.current = setTimeout(() => {
|
||||||
void searchEntries(query);
|
// Create a new AbortController for this search request
|
||||||
|
const controller = new AbortController();
|
||||||
|
abortControllerRef.current = controller;
|
||||||
|
void searchEntries(query, controller.signal);
|
||||||
}, 300);
|
}, 300);
|
||||||
},
|
},
|
||||||
[searchEntries]
|
[searchEntries]
|
||||||
@@ -321,13 +343,16 @@ export function LinkAutocomplete({
|
|||||||
}, [textareaRef, handleInput, handleKeyDown]);
|
}, [textareaRef, handleInput, handleKeyDown]);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Cleanup timeout on unmount
|
* Cleanup timeout and abort in-flight requests on unmount
|
||||||
*/
|
*/
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
return (): void => {
|
return (): void => {
|
||||||
if (searchTimeoutRef.current) {
|
if (searchTimeoutRef.current) {
|
||||||
clearTimeout(searchTimeoutRef.current);
|
clearTimeout(searchTimeoutRef.current);
|
||||||
}
|
}
|
||||||
|
if (abortControllerRef.current) {
|
||||||
|
abortControllerRef.current.abort();
|
||||||
|
}
|
||||||
};
|
};
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
|
|||||||
@@ -8,10 +8,10 @@ import * as apiClient from "@/lib/api/client";
|
|||||||
|
|
||||||
// Mock the API client
|
// Mock the API client
|
||||||
vi.mock("@/lib/api/client", () => ({
|
vi.mock("@/lib/api/client", () => ({
|
||||||
apiGet: vi.fn(),
|
apiRequest: vi.fn(),
|
||||||
}));
|
}));
|
||||||
|
|
||||||
const mockApiGet = apiClient.apiGet as ReturnType<typeof vi.fn>;
|
const mockApiRequest = apiClient.apiRequest as ReturnType<typeof vi.fn>;
|
||||||
|
|
||||||
describe("LinkAutocomplete", (): void => {
|
describe("LinkAutocomplete", (): void => {
|
||||||
let textareaRef: React.RefObject<HTMLTextAreaElement>;
|
let textareaRef: React.RefObject<HTMLTextAreaElement>;
|
||||||
@@ -29,7 +29,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
|
|
||||||
// Reset mocks
|
// Reset mocks
|
||||||
vi.clearAllMocks();
|
vi.clearAllMocks();
|
||||||
mockApiGet.mockResolvedValue({
|
mockApiRequest.mockResolvedValue({
|
||||||
data: [],
|
data: [],
|
||||||
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
||||||
});
|
});
|
||||||
@@ -67,6 +67,146 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should pass an AbortSignal to apiRequest for cancellation", async (): Promise<void> => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
|
||||||
|
mockApiRequest.mockResolvedValue({
|
||||||
|
data: [],
|
||||||
|
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
||||||
|
});
|
||||||
|
|
||||||
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
|
const textarea = textareaRef.current;
|
||||||
|
if (!textarea) throw new Error("Textarea not found");
|
||||||
|
|
||||||
|
// Simulate typing [[abc
|
||||||
|
act(() => {
|
||||||
|
textarea.value = "[[abc";
|
||||||
|
textarea.setSelectionRange(5, 5);
|
||||||
|
fireEvent.input(textarea);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Advance past debounce to fire the search
|
||||||
|
await act(async () => {
|
||||||
|
await vi.advanceTimersByTimeAsync(300);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Verify apiRequest was called with a signal
|
||||||
|
expect(mockApiRequest).toHaveBeenCalledTimes(1);
|
||||||
|
const callArgs = mockApiRequest.mock.calls[0] as [
|
||||||
|
string,
|
||||||
|
{ method: string; signal: AbortSignal },
|
||||||
|
];
|
||||||
|
expect(callArgs[1]).toHaveProperty("signal");
|
||||||
|
expect(callArgs[1].signal).toBeInstanceOf(AbortSignal);
|
||||||
|
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should abort previous in-flight request when a new search fires", async (): Promise<void> => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
|
||||||
|
mockApiRequest.mockResolvedValue({
|
||||||
|
data: [],
|
||||||
|
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
||||||
|
});
|
||||||
|
|
||||||
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
|
const textarea = textareaRef.current;
|
||||||
|
if (!textarea) throw new Error("Textarea not found");
|
||||||
|
|
||||||
|
// First search: type [[foo
|
||||||
|
act(() => {
|
||||||
|
textarea.value = "[[foo";
|
||||||
|
textarea.setSelectionRange(5, 5);
|
||||||
|
fireEvent.input(textarea);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Advance past debounce to fire the first search
|
||||||
|
await act(async () => {
|
||||||
|
await vi.advanceTimersByTimeAsync(300);
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockApiRequest).toHaveBeenCalledTimes(1);
|
||||||
|
const firstCallArgs = mockApiRequest.mock.calls[0] as [
|
||||||
|
string,
|
||||||
|
{ method: string; signal: AbortSignal },
|
||||||
|
];
|
||||||
|
const firstSignal = firstCallArgs[1].signal;
|
||||||
|
expect(firstSignal.aborted).toBe(false);
|
||||||
|
|
||||||
|
// Second search: type [[foobar (user continues typing)
|
||||||
|
act(() => {
|
||||||
|
textarea.value = "[[foobar";
|
||||||
|
textarea.setSelectionRange(8, 8);
|
||||||
|
fireEvent.input(textarea);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The first signal should be aborted immediately when debouncedSearch fires again
|
||||||
|
// (abort happens before the timeout, in the debounce function itself)
|
||||||
|
expect(firstSignal.aborted).toBe(true);
|
||||||
|
|
||||||
|
// Advance past debounce to fire the second search
|
||||||
|
await act(async () => {
|
||||||
|
await vi.advanceTimersByTimeAsync(300);
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockApiRequest).toHaveBeenCalledTimes(2);
|
||||||
|
const secondCallArgs = mockApiRequest.mock.calls[1] as [
|
||||||
|
string,
|
||||||
|
{ method: string; signal: AbortSignal },
|
||||||
|
];
|
||||||
|
const secondSignal = secondCallArgs[1].signal;
|
||||||
|
expect(secondSignal.aborted).toBe(false);
|
||||||
|
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should abort in-flight request on unmount", async (): Promise<void> => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
|
||||||
|
mockApiRequest.mockResolvedValue({
|
||||||
|
data: [],
|
||||||
|
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
||||||
|
});
|
||||||
|
|
||||||
|
const { unmount } = render(
|
||||||
|
<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />
|
||||||
|
);
|
||||||
|
|
||||||
|
const textarea = textareaRef.current;
|
||||||
|
if (!textarea) throw new Error("Textarea not found");
|
||||||
|
|
||||||
|
// Trigger a search
|
||||||
|
act(() => {
|
||||||
|
textarea.value = "[[test";
|
||||||
|
textarea.setSelectionRange(6, 6);
|
||||||
|
fireEvent.input(textarea);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Advance past debounce
|
||||||
|
await act(async () => {
|
||||||
|
await vi.advanceTimersByTimeAsync(300);
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockApiRequest).toHaveBeenCalledTimes(1);
|
||||||
|
const callArgs = mockApiRequest.mock.calls[0] as [
|
||||||
|
string,
|
||||||
|
{ method: string; signal: AbortSignal },
|
||||||
|
];
|
||||||
|
const signal = callArgs[1].signal;
|
||||||
|
expect(signal.aborted).toBe(false);
|
||||||
|
|
||||||
|
// Unmount the component - should abort in-flight request
|
||||||
|
unmount();
|
||||||
|
|
||||||
|
expect(signal.aborted).toBe(true);
|
||||||
|
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
|
||||||
// TODO: Fix async/timer interaction - component works but test has timing issues with fake timers
|
// TODO: Fix async/timer interaction - component works but test has timing issues with fake timers
|
||||||
it.skip("should perform debounced search when typing query", async (): Promise<void> => {
|
it.skip("should perform debounced search when typing query", async (): Promise<void> => {
|
||||||
vi.useFakeTimers();
|
vi.useFakeTimers();
|
||||||
@@ -93,7 +233,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
||||||
};
|
};
|
||||||
|
|
||||||
mockApiGet.mockResolvedValue(mockResults);
|
mockApiRequest.mockResolvedValue(mockResults);
|
||||||
|
|
||||||
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
@@ -108,7 +248,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Should not call API immediately
|
// Should not call API immediately
|
||||||
expect(mockApiGet).not.toHaveBeenCalled();
|
expect(mockApiRequest).not.toHaveBeenCalled();
|
||||||
|
|
||||||
// Fast-forward 300ms and let promises resolve
|
// Fast-forward 300ms and let promises resolve
|
||||||
await act(async () => {
|
await act(async () => {
|
||||||
@@ -116,7 +256,11 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
expect(mockApiGet).toHaveBeenCalledWith("/api/knowledge/search?q=test&limit=10");
|
expect(mockApiRequest).toHaveBeenCalledWith(
|
||||||
|
"/api/knowledge/search?q=test&limit=10",
|
||||||
|
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
|
||||||
|
expect.objectContaining({ method: "GET", signal: expect.any(AbortSignal) })
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
@@ -168,7 +312,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
meta: { total: 2, page: 1, limit: 10, totalPages: 1 },
|
meta: { total: 2, page: 1, limit: 10, totalPages: 1 },
|
||||||
};
|
};
|
||||||
|
|
||||||
mockApiGet.mockResolvedValue(mockResults);
|
mockApiRequest.mockResolvedValue(mockResults);
|
||||||
|
|
||||||
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
@@ -241,7 +385,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
||||||
};
|
};
|
||||||
|
|
||||||
mockApiGet.mockResolvedValue(mockResults);
|
mockApiRequest.mockResolvedValue(mockResults);
|
||||||
|
|
||||||
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
@@ -299,7 +443,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
||||||
};
|
};
|
||||||
|
|
||||||
mockApiGet.mockResolvedValue(mockResults);
|
mockApiRequest.mockResolvedValue(mockResults);
|
||||||
|
|
||||||
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
@@ -407,7 +551,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
it.skip("should show 'No entries found' when search returns no results", async (): Promise<void> => {
|
it.skip("should show 'No entries found' when search returns no results", async (): Promise<void> => {
|
||||||
vi.useFakeTimers();
|
vi.useFakeTimers();
|
||||||
|
|
||||||
mockApiGet.mockResolvedValue({
|
mockApiRequest.mockResolvedValue({
|
||||||
data: [],
|
data: [],
|
||||||
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
meta: { total: 0, page: 1, limit: 10, totalPages: 0 },
|
||||||
});
|
});
|
||||||
@@ -444,7 +588,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
const searchPromise = new Promise((resolve) => {
|
const searchPromise = new Promise((resolve) => {
|
||||||
resolveSearch = resolve;
|
resolveSearch = resolve;
|
||||||
});
|
});
|
||||||
mockApiGet.mockReturnValue(
|
mockApiRequest.mockReturnValue(
|
||||||
searchPromise as Promise<{
|
searchPromise as Promise<{
|
||||||
data: unknown[];
|
data: unknown[];
|
||||||
meta: { total: number; page: number; limit: number; totalPages: number };
|
meta: { total: number; page: number; limit: number; totalPages: number };
|
||||||
@@ -510,7 +654,7 @@ describe("LinkAutocomplete", (): void => {
|
|||||||
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
meta: { total: 1, page: 1, limit: 10, totalPages: 1 },
|
||||||
};
|
};
|
||||||
|
|
||||||
mockApiGet.mockResolvedValue(mockResults);
|
mockApiRequest.mockResolvedValue(mockResults);
|
||||||
|
|
||||||
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
render(<LinkAutocomplete textareaRef={textareaRef} onInsert={onInsertMock} />);
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user