mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-02 21:03:34 +03:00
fix(desktop): key endpoint availability by the endpoint set
Review caught a real fail-open regression. The multiple-endpoint query keyed on a constant, which asserts the value is caller-independent, but on two paths it is not: the self-hosted-offline check and the legacy ?endpoints= fallback both resolve only the endpoints they were handed. Consumers pass disjoint sets, so whichever mounted first froze a partial map into the shared entry, and a later consumer projected endpoints nobody had checked. Those read as available on exactly the paths where desktop means to fail closed, and being mount-order dependent it would not have reproduced reliably. Keying by the endpoint set restores the old per-consumer semantics and removes the mount-order dependence. Prefix invalidation still covers every set, so the reconnect swap is unaffected. This costs the cross-consumer dedup the PR claimed: that saving only held while the whole-map response made the shared entry complete, which those two paths break. Also stops the readiness effect invalidating on mount, which was forcing a redundant second fetch behind the first.
This commit is contained in:
@@ -9,6 +9,8 @@ import {
|
|||||||
} from "vitest";
|
} from "vitest";
|
||||||
import { renderHook, waitFor, act } from "@testing-library/react";
|
import { renderHook, waitFor, act } from "@testing-library/react";
|
||||||
import { TestQueryProvider } from "@app/tests/utils/TestQueryProvider";
|
import { TestQueryProvider } from "@app/tests/utils/TestQueryProvider";
|
||||||
|
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||||
|
import type { ReactNode } from "react";
|
||||||
import {
|
import {
|
||||||
useEndpointEnabled,
|
useEndpointEnabled,
|
||||||
useMultipleEndpointsEnabled,
|
useMultipleEndpointsEnabled,
|
||||||
@@ -113,6 +115,8 @@ vi.mock("@app/services/endpointAvailabilityService", () => ({
|
|||||||
}));
|
}));
|
||||||
|
|
||||||
const NONE: string[] = [];
|
const NONE: string[] = [];
|
||||||
|
const NARROW = ["merge"];
|
||||||
|
const WIDE = ["merge", "ocr", "compress"];
|
||||||
|
|
||||||
function appConfig(dependenciesReady: boolean) {
|
function appConfig(dependenciesReady: boolean) {
|
||||||
return { data: { dependenciesReady } };
|
return { data: { dependenciesReady } };
|
||||||
@@ -152,6 +156,11 @@ describe("desktop useMultipleEndpointsEnabled", () => {
|
|||||||
|
|
||||||
await waitFor(() => expect(result.current.loading).toBe(false));
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
expect(result.current.endpointStatus).toEqual({ merge: true, ocr: false });
|
expect(result.current.endpointStatus).toEqual({ merge: true, ocr: false });
|
||||||
|
// Once, not twice: the readiness effect must not invalidate on mount.
|
||||||
|
const availabilityCalls = mockGet.mock.calls.filter((c) =>
|
||||||
|
String(c[0]).includes("endpoints-availability"),
|
||||||
|
);
|
||||||
|
expect(availabilityCalls).toHaveLength(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("marks locally-disabled endpoints available in SaaS mode", async () => {
|
it("marks locally-disabled endpoints available in SaaS mode", async () => {
|
||||||
@@ -308,6 +317,46 @@ describe("desktop useMultipleEndpointsEnabled", () => {
|
|||||||
await waitFor(() => expect(result.current.endpointStatus.merge).toBe(true));
|
await waitFor(() => expect(result.current.endpointStatus.merge).toBe(true));
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("resolves each consumer's own endpoints when they ask for disjoint sets", async () => {
|
||||||
|
// The offline path resolves only the endpoints it is handed. If consumers
|
||||||
|
// shared one cache entry, the second would project endpoints nobody
|
||||||
|
// checked and read them as available.
|
||||||
|
mode = "selfhosted";
|
||||||
|
selfHostedStatus = "offline";
|
||||||
|
mockLocalSupport.mockImplementation((endpoint: string) =>
|
||||||
|
Promise.resolve(endpoint === "merge"),
|
||||||
|
);
|
||||||
|
|
||||||
|
const client = new QueryClient({
|
||||||
|
defaultOptions: { queries: { retry: false, gcTime: Infinity } },
|
||||||
|
});
|
||||||
|
const wrapper = ({ children }: { children: ReactNode }) => (
|
||||||
|
<QueryClientProvider client={client}>{children}</QueryClientProvider>
|
||||||
|
);
|
||||||
|
|
||||||
|
const { result } = renderHook(
|
||||||
|
() => ({
|
||||||
|
narrow: useMultipleEndpointsEnabled(NARROW),
|
||||||
|
wide: useMultipleEndpointsEnabled(WIDE),
|
||||||
|
}),
|
||||||
|
{ wrapper },
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.wide.loading).toBe(false));
|
||||||
|
|
||||||
|
// ocr and compress were genuinely probed and are unsupported locally.
|
||||||
|
expect(result.current.wide.endpointStatus).toEqual({
|
||||||
|
merge: true,
|
||||||
|
ocr: false,
|
||||||
|
compress: false,
|
||||||
|
});
|
||||||
|
expect(mockLocalSupport).toHaveBeenCalledWith("ocr", expect.anything());
|
||||||
|
expect(mockLocalSupport).toHaveBeenCalledWith(
|
||||||
|
"compress",
|
||||||
|
expect.anything(),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("reports nothing loading when asked for no endpoints", async () => {
|
it("reports nothing loading when asked for no endpoints", async () => {
|
||||||
// Stable reference: the current hook keys effects on the array identity.
|
// Stable reference: the current hook keys effects on the array identity.
|
||||||
const { result } = renderHook(() => useMultipleEndpointsEnabled(NONE), {
|
const { result } = renderHook(() => useMultipleEndpointsEnabled(NONE), {
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import {
|
|||||||
useCallback,
|
useCallback,
|
||||||
useEffect,
|
useEffect,
|
||||||
useMemo,
|
useMemo,
|
||||||
|
useRef,
|
||||||
useState,
|
useState,
|
||||||
useSyncExternalStore,
|
useSyncExternalStore,
|
||||||
} from "react";
|
} from "react";
|
||||||
@@ -77,11 +78,15 @@ export function useEndpointEnabled(endpoint: string): {
|
|||||||
retryDelay: RETRY_DELAY_MS,
|
retryDelay: RETRY_DELAY_MS,
|
||||||
});
|
});
|
||||||
|
|
||||||
// Re-run only when readiness or the endpoint changes; queryClient/queryKey
|
// Skipped on mount for the same reason as above: the query is already running.
|
||||||
// are stable and deliberately excluded.
|
const seenReadiness = useRef<string | null>(null);
|
||||||
|
const readinessMark = `${backendOnline}:${offline}:${endpoint}`;
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (ready) void queryClient.invalidateQueries({ queryKey });
|
const first = seenReadiness.current === null;
|
||||||
}, [backendOnline, offline, endpoint]);
|
seenReadiness.current = readinessMark;
|
||||||
|
if (first || !ready) return;
|
||||||
|
void queryClient.invalidateQueries({ queryKey });
|
||||||
|
}, [readinessMark]);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
enabled: endpoint ? (data ?? true) : null,
|
enabled: endpoint ? (data ?? true) : null,
|
||||||
@@ -105,12 +110,16 @@ export function useMultipleEndpointsEnabled(endpoints: string[]): {
|
|||||||
const { ready, backendOnline, offline } = useBackendReadiness();
|
const { ready, backendOnline, offline } = useBackendReadiness();
|
||||||
const wanted = endpoints ?? [];
|
const wanted = endpoints ?? [];
|
||||||
const key = wanted.join(",");
|
const key = wanted.join(",");
|
||||||
const queryKey = qk.endpointsAvailability();
|
// Keyed by the endpoint set, not shared across consumers. A constant key
|
||||||
|
// would assert the value is caller-independent, and on two paths it is not:
|
||||||
|
// the self-hosted-offline check and the legacy ?endpoints= fallback both
|
||||||
|
// resolve only the endpoints they were handed. Consumers pass disjoint sets,
|
||||||
|
// so a shared entry would leave the second consumer's endpoints unresolved
|
||||||
|
// and reading as available. Prefix invalidation below still covers every set.
|
||||||
|
const queryKey = [...qk.endpointsAvailability(), key];
|
||||||
|
|
||||||
const { data, isPending, refetch } = useQuery({
|
const { data, isPending, refetch } = useQuery({
|
||||||
queryKey,
|
queryKey,
|
||||||
// key drives the legacy fallback param and the offline per-endpoint checks;
|
|
||||||
// the shared cache entry is still the whole map, projected per consumer.
|
|
||||||
queryFn: () => resolveEndpointsAvailability(key ? key.split(",") : []),
|
queryFn: () => resolveEndpointsAvailability(key ? key.split(",") : []),
|
||||||
enabled: wanted.length > 0 && ready,
|
enabled: wanted.length > 0 && ready,
|
||||||
staleTime: CONFIG_STALE_TIME,
|
staleTime: CONFIG_STALE_TIME,
|
||||||
@@ -118,16 +127,26 @@ export function useMultipleEndpointsEnabled(endpoints: string[]): {
|
|||||||
retryDelay: RETRY_DELAY_MS,
|
retryDelay: RETRY_DELAY_MS,
|
||||||
});
|
});
|
||||||
|
|
||||||
// A reconnect (readiness flips) forces the swap from offline to remote data.
|
// A reconnect forces the swap from offline to remote data. Skipped on mount:
|
||||||
|
// the query is already fetching, and invalidating would double the request.
|
||||||
|
const seenReadiness = useRef<string | null>(null);
|
||||||
|
const readinessMark = `${backendOnline}:${offline}`;
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (ready) void queryClient.invalidateQueries({ queryKey });
|
const first = seenReadiness.current === null;
|
||||||
}, [backendOnline, offline]);
|
seenReadiness.current = readinessMark;
|
||||||
|
if (first || !ready) return;
|
||||||
|
void queryClient.invalidateQueries({
|
||||||
|
queryKey: qk.endpointsAvailability(),
|
||||||
|
});
|
||||||
|
}, [readinessMark]);
|
||||||
|
|
||||||
const projected = useMemo(() => {
|
const projected = useMemo(() => {
|
||||||
const status: Record<string, boolean> = {};
|
const status: Record<string, boolean> = {};
|
||||||
const details: Record<string, EndpointAvailabilityDetails> = {};
|
const details: Record<string, EndpointAvailabilityDetails> = {};
|
||||||
if (!data) return { status, details };
|
if (!data) return { status, details };
|
||||||
for (const endpoint of key ? key.split(",") : []) {
|
for (const endpoint of key ? key.split(",") : []) {
|
||||||
|
// Safe because the entry is per-set: a miss here means the server does
|
||||||
|
// not know the endpoint, which the old code also treated as available.
|
||||||
const detail = data[endpoint] ?? OPTIMISTIC;
|
const detail = data[endpoint] ?? OPTIMISTIC;
|
||||||
status[endpoint] = detail.enabled;
|
status[endpoint] = detail.enabled;
|
||||||
details[endpoint] = detail;
|
details[endpoint] = detail;
|
||||||
|
|||||||
Reference in New Issue
Block a user