fix: invalidate ValidatedPopover in-flight submits on close
If the user clicked Submit and dismissed the popover before the request settled, the next time they re-opened it would inherit `loading=true` (the disabled-while-loading submit button was stuck) and a late rejection from the previous session could repopulate `error` in the new one. Track the submit via a `useRef` token, bump it on close, and ignore any result/rejection whose token no longer matches. Also reset `loading` and `error` on close so the next open starts clean. Adds a regression test covering both the late-resolution and late-rejection paths. Addresses CodeRabbit review on PR #759.
This commit is contained in:
@@ -0,0 +1,130 @@
|
|||||||
|
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||||
|
import { render, screen, fireEvent, act, waitFor, cleanup } from "@testing-library/react";
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
cleanup();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Stub the Radix-backed Popover/Tooltip primitives so children render inline
|
||||||
|
// (no portal, no measurement gymnastics) and the popover's open/close is
|
||||||
|
// driven by ValidatedPopover's own onOpenChange callback that we surface via
|
||||||
|
// a parent-controlled close button.
|
||||||
|
vi.mock("@kandev/ui/popover", () => {
|
||||||
|
return {
|
||||||
|
Popover: ({
|
||||||
|
open,
|
||||||
|
onOpenChange,
|
||||||
|
children,
|
||||||
|
}: {
|
||||||
|
open: boolean;
|
||||||
|
onOpenChange: (next: boolean) => void;
|
||||||
|
children: React.ReactNode;
|
||||||
|
}) => (
|
||||||
|
<div data-testid="popover" data-open={open}>
|
||||||
|
{children}
|
||||||
|
<button data-testid="popover-close" type="button" onClick={() => onOpenChange(false)}>
|
||||||
|
close
|
||||||
|
</button>
|
||||||
|
</div>
|
||||||
|
),
|
||||||
|
PopoverTrigger: ({ children }: { children: React.ReactNode }) => <>{children}</>,
|
||||||
|
PopoverContent: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
vi.mock("@kandev/ui/tooltip", () => ({
|
||||||
|
Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}</>,
|
||||||
|
TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children}</>,
|
||||||
|
TooltipContent: ({ children }: { children: React.ReactNode }) => <>{children}</>,
|
||||||
|
}));
|
||||||
|
|
||||||
|
import { ValidatedPopover } from "./validated-popover";
|
||||||
|
|
||||||
|
function deferred<T>() {
|
||||||
|
let resolve!: (v: T) => void;
|
||||||
|
let reject!: (e: unknown) => void;
|
||||||
|
const promise = new Promise<T>((res, rej) => {
|
||||||
|
resolve = res;
|
||||||
|
reject = rej;
|
||||||
|
});
|
||||||
|
return { promise, resolve, reject };
|
||||||
|
}
|
||||||
|
|
||||||
|
function renderHarness(overrides?: { fetch?: (key: string) => Promise<string> }) {
|
||||||
|
const onSuccess = vi.fn();
|
||||||
|
const result = render(
|
||||||
|
<ValidatedPopover
|
||||||
|
triggerStyle="ghost-icon"
|
||||||
|
triggerIcon={<span>icon</span>}
|
||||||
|
triggerAriaLabel="trigger"
|
||||||
|
tooltip="tip"
|
||||||
|
headline="Headline"
|
||||||
|
placeholder="paste here"
|
||||||
|
extractKey={(raw) => (raw.trim() ? raw.trim() : null)}
|
||||||
|
validationHint="hint"
|
||||||
|
fetch={overrides?.fetch ?? (async (k: string) => k)}
|
||||||
|
onSuccess={onSuccess}
|
||||||
|
submitLabel="Submit"
|
||||||
|
submittingLabel="Submitting..."
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
return { ...result, onSuccess };
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("ValidatedPopover", () => {
|
||||||
|
it("close-while-loading resets loading state and ignores the late resolution", async () => {
|
||||||
|
const d = deferred<string>();
|
||||||
|
const fetchSpy = vi.fn(async () => d.promise);
|
||||||
|
const { onSuccess } = renderHarness({ fetch: fetchSpy });
|
||||||
|
|
||||||
|
// Type a key + submit
|
||||||
|
fireEvent.change(screen.getByPlaceholderText("paste here"), { target: { value: "KEY-1" } });
|
||||||
|
fireEvent.click(screen.getByRole("button", { name: "Submit" }));
|
||||||
|
|
||||||
|
// Mid-flight: submitting label visible, button disabled.
|
||||||
|
await waitFor(() => {
|
||||||
|
const submitting = screen.getByRole("button", { name: "Submitting..." }) as HTMLButtonElement;
|
||||||
|
expect(submitting.disabled).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
// User dismisses the popover before fetch resolves.
|
||||||
|
fireEvent.click(screen.getByTestId("popover-close"));
|
||||||
|
|
||||||
|
// Loading must reset on close so the next open starts clean.
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(screen.queryByRole("button", { name: "Submitting..." })).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Late resolution must not call onSuccess (the submission token was
|
||||||
|
// invalidated when we closed) and must not bring the popover back.
|
||||||
|
await act(async () => {
|
||||||
|
d.resolve("late");
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
expect(onSuccess).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("close-while-loading suppresses a late rejection rather than re-populating error", async () => {
|
||||||
|
const d = deferred<string>();
|
||||||
|
const fetchSpy = vi.fn(async () => d.promise);
|
||||||
|
renderHarness({ fetch: fetchSpy });
|
||||||
|
|
||||||
|
fireEvent.change(screen.getByPlaceholderText("paste here"), { target: { value: "KEY-1" } });
|
||||||
|
fireEvent.click(screen.getByRole("button", { name: "Submit" }));
|
||||||
|
await waitFor(() => {
|
||||||
|
const submitting = screen.getByRole("button", { name: "Submitting..." }) as HTMLButtonElement;
|
||||||
|
expect(submitting.disabled).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
fireEvent.click(screen.getByTestId("popover-close"));
|
||||||
|
|
||||||
|
// Reject after close — the in-flight error must not leak into UI state.
|
||||||
|
await act(async () => {
|
||||||
|
d.reject(new Error("boom"));
|
||||||
|
await Promise.resolve().catch(() => {});
|
||||||
|
});
|
||||||
|
|
||||||
|
// role=alert is the inline error <p> in ValidatedPopover; should be absent.
|
||||||
|
expect(screen.queryByRole("alert")).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1,6 +1,6 @@
|
|||||||
"use client";
|
"use client";
|
||||||
|
|
||||||
import { useCallback, useState, type ReactNode } from "react";
|
import { useCallback, useRef, useState, type ReactNode } from "react";
|
||||||
import { Button } from "@kandev/ui/button";
|
import { Button } from "@kandev/ui/button";
|
||||||
import { Input } from "@kandev/ui/input";
|
import { Input } from "@kandev/ui/input";
|
||||||
import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover";
|
import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover";
|
||||||
@@ -105,6 +105,10 @@ export function ValidatedPopover<T>(props: ValidatedPopoverProps<T>) {
|
|||||||
const [value, setValue] = useState("");
|
const [value, setValue] = useState("");
|
||||||
const [loading, setLoading] = useState(false);
|
const [loading, setLoading] = useState(false);
|
||||||
const [error, setError] = useState<string | null>(null);
|
const [error, setError] = useState<string | null>(null);
|
||||||
|
// Submission token: bumped on every submit and on close, so an in-flight
|
||||||
|
// request whose promise resolves after the popover was closed (or
|
||||||
|
// re-opened) cannot leak loading/error state into the next session.
|
||||||
|
const submissionRef = useRef(0);
|
||||||
|
|
||||||
const submit = useCallback(async () => {
|
const submit = useCallback(async () => {
|
||||||
const key = extractKey(value.trim());
|
const key = extractKey(value.trim());
|
||||||
@@ -112,25 +116,33 @@ export function ValidatedPopover<T>(props: ValidatedPopoverProps<T>) {
|
|||||||
setError(validationHint);
|
setError(validationHint);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
const submission = ++submissionRef.current;
|
||||||
setLoading(true);
|
setLoading(true);
|
||||||
setError(null);
|
setError(null);
|
||||||
try {
|
try {
|
||||||
const result = await fetch(key);
|
const result = await fetch(key);
|
||||||
|
if (submission !== submissionRef.current) return;
|
||||||
onSuccess(key, result);
|
onSuccess(key, result);
|
||||||
setOpen(false);
|
setOpen(false);
|
||||||
setValue("");
|
setValue("");
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
|
if (submission !== submissionRef.current) return;
|
||||||
setError(err instanceof Error ? err.message : String(err));
|
setError(err instanceof Error ? err.message : String(err));
|
||||||
} finally {
|
} finally {
|
||||||
setLoading(false);
|
if (submission === submissionRef.current) setLoading(false);
|
||||||
}
|
}
|
||||||
}, [value, extractKey, validationHint, fetch, onSuccess]);
|
}, [value, extractKey, validationHint, fetch, onSuccess]);
|
||||||
|
|
||||||
// Closing discards any stale validation/error so the next open starts
|
// Closing invalidates any in-flight submit (so a late rejection can't
|
||||||
// clean rather than rehydrating yesterday's failure.
|
// re-populate `error` after the user dismissed the popover) and clears
|
||||||
|
// local submit state so the next open starts clean.
|
||||||
const handleOpenChange = (next: boolean) => {
|
const handleOpenChange = (next: boolean) => {
|
||||||
setOpen(next);
|
setOpen(next);
|
||||||
if (!next) setError(null);
|
if (!next) {
|
||||||
|
submissionRef.current += 1;
|
||||||
|
setLoading(false);
|
||||||
|
setError(null);
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
return (
|
return (
|
||||||
|
|||||||
Reference in New Issue
Block a user