From b281447da8a58c37d8163439958629dbc309b285 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 1 Nov 2026 09:55:43 +0800 Subject: [PATCH] fix(search): mark outdated find results and block stale replacements (#630) While a new query was debouncing or validating, Find and Replace in Files kept showing the previous query's rows, count and highlights, and Replace stayed enabled. Replace, Replace All in This File and Replace All then sent the new pattern with the old positions, so `order` changed to `ord` could rewrite `order` as `Xer`. From the moment query, flags, mask, scope or context change, old rows are dimmed and lose their highlights, replacement preview and count until the new search's first results arrive. All replace buttons and keyboard paths are blocked meanwhile, and a pending Replace All confirmation rechecks that its results are still current. Opening a retained row still works. Refs #623 --- .../search/__tests__/findPopup.test.tsx | 180 +++++++++++++++++- .../search/__tests__/findSession.test.ts | 53 ++++++ .../search/find/FindInFiles.module.css | 6 + src/features/search/find/FindInFiles.tsx | 59 ++++-- src/features/search/find/session.ts | 39 +++- src/platform/locales/text/zh.json | 1 + 6 files changed, 308 insertions(+), 30 deletions(-) diff --git a/src/features/search/__tests__/findPopup.test.tsx b/src/features/search/__tests__/findPopup.test.tsx index 0c6593ea3..9d10f9f6b 100644 --- a/src/features/search/__tests__/findPopup.test.tsx +++ b/src/features/search/__tests__/findPopup.test.tsx @@ -1,15 +1,22 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { act, fireEvent, render, screen, within } from "@testing-library/react"; +import type { FileMatches, SearchSummary } from "@/platform/ipc"; import type { ReplaceRequest } from "../find/replaceFlow"; const replaced: ReplaceRequest[] = []; const confirms: unknown[] = []; +let validation: Promise | null = null; +let search: ((onFile: (file: FileMatches) => void) => Promise) | null = null; +let confirmation: Promise | null = null; vi.mock("../find/windowStore", async (orig) => ({ ...(await orig()), ipcFindDeps: () => ({ - validate: async () => {}, - start: async (_id: number, _q: unknown, onFile: (f: unknown) => void) => { + validate: async () => { + await validation; + }, + start: async (_id: number, _q: unknown, onFile: (f: FileMatches) => void) => { + if (search) return search(onFile); onFile({ path: "/p/a.kt", relPath: "a.kt", @@ -44,7 +51,7 @@ vi.mock("@/workbench/messages", async (orig) => ({ ...(await orig()), confirm: async (opts: unknown) => { confirms.push(opts); - return true; + return confirmation ?? true; }, })); @@ -63,7 +70,22 @@ beforeEach(() => { replaced.length = 0; confirms.length = 0; localStorage.clear(); - useFindPopup.setState({ pinned: false, maskEnabled: false, preserveCase: false, context: "any", replacement: "" }); + validation = null; + search = null; + confirmation = null; + useFindPopup.setState({ + pattern: "", + caseSensitive: false, + wholeWord: false, + isRegex: false, + scopeTab: "project", + mask: "", + pinned: false, + maskEnabled: false, + preserveCase: false, + context: "any", + replacement: "", + }); }); afterEach(() => { @@ -163,3 +185,153 @@ describe("Find in Files popup", () => { expect(findHistory("find")[0]).toBe("order"); }); }); + +describe("outdated popup results", () => { + const results = () => screen.getByRole("listbox", { name: "Matches" }).parentElement!; + const rowText = () => + within(screen.getByRole("listbox", { name: "Matches" })) + .getAllByRole("option") + .map((r) => r.textContent); + + it.each([false, true])( + "marks retained rows immediately in replace=%s and waits for the first new file", + async (replace) => { + render(); + act(() => showFindPopup(replace, { query: "order" })); + await settle(); + if (replace) fireEvent.change(screen.getByLabelText("Replace with"), { target: { value: "item" } }); + const oldRows = within(screen.getByRole("listbox", { name: "Matches" })).getAllByRole("option"); + let validate!: () => void; + validation = new Promise((r) => { + validate = r; + }); + let onFile!: (file: FileMatches) => void; + let finish!: (summary: SearchSummary | null) => void; + search = (callback) => { + onFile = callback; + return new Promise((r) => { + finish = r; + }); + }; + fireEvent.change(field(), { target: { value: "fresh" } }); + expect(results()).toHaveAttribute("data-stale", "true"); + expect(screen.getByText("Updating results…")).toBeInTheDocument(); + expect(within(results()).getAllByRole("option")).toEqual(oldRows); + expect(results().querySelector("mark, del, ins")).toBeNull(); + expect(screen.queryByText("3 matches in 2 files")).not.toBeInTheDocument(); + await settle(); + expect(results()).toHaveAttribute("data-stale", "true"); + expect(rowText()).toHaveLength(3); + await act(async () => validate()); + expect(results()).toHaveAttribute("data-stale", "true"); + expect(rowText()).toHaveLength(3); + if (replace) expect(screen.getByRole("button", { name: "Replace" })).toBeDisabled(); + await act(async () => + onFile({ + path: "/p/fresh.kt", + relPath: "fresh.kt", + matches: [{ line: 0, text: "fresh", ranges: [[0, 5]], textOffset: 0 }], + }), + ); + expect(results()).not.toHaveAttribute("data-stale"); + expect(rowText()).toHaveLength(1); + expect(screen.getByText("1 match in 1 file")).toBeInTheDocument(); + expect(results().querySelector(replace ? "ins" : "mark")).not.toBeNull(); + if (replace) expect(screen.getByRole("button", { name: "Replace" })).toBeEnabled(); + await act(async () => finish(null)); + }, + ); + + it.each(["Replace", "Replace All in This File", "Replace All", "Enter", "All shortcut", "All mnemonic"])( + "blocks %s during debounce and validation", + async (action) => { + render(); + act(() => showFindPopup(true, { query: "order" })); + await settle(); + validation = new Promise(() => {}); + fireEvent.change(field(), { target: { value: "ord" } }); + const invoke = () => { + if (action === "Enter") fireEvent.keyDown(field(), { key: "Enter" }); + else if (action === "All shortcut") fireEvent.keyDown(field(), { key: "Enter", altKey: true, shiftKey: true }); + else if (action === "All mnemonic") fireEvent.keyDown(field(), { key: "a", code: "KeyA", altKey: true }); + else { + const button = screen.getByRole("button", { name: action }); + expect(button).toBeDisabled(); + fireEvent.click(button); + } + }; + invoke(); + await settle(); + invoke(); + await act(async () => {}); + expect(replaced).toHaveLength(0); + expect(confirms).toHaveLength(0); + }, + ); + + it.each([ + { caseSensitive: true }, + { wholeWord: true }, + { isRegex: true }, + { maskEnabled: true, mask: "*.ts" }, + { scopeTab: "directory" as const, directory: "/other" }, + { context: "inComments" as const }, + ])("invalidates when search options change: %j", async (patch) => { + render(); + act(() => showFindPopup(true, { query: "order" })); + await settle(); + act(() => useFindPopup.setState(patch)); + expect(results()).toHaveAttribute("data-stale", "true"); + expect(results().querySelector("mark, del, ins")).toBeNull(); + expect(screen.getByRole("button", { name: "Replace" })).toBeDisabled(); + }); + + it("clears retained rows and the count when the new search finds nothing", async () => { + render(); + act(() => showFindPopup(true, { query: "order" })); + await settle(); + let finish!: (summary: SearchSummary | null) => void; + search = () => + new Promise((r) => { + finish = r; + }); + fireEvent.change(field(), { target: { value: "absent" } }); + await settle(); + expect(results()).toHaveAttribute("data-stale", "true"); + expect(rowText()).toHaveLength(3); + const box = results(); + await act(async () => finish(null)); + expect(box).not.toHaveAttribute("data-stale"); + expect(screen.getByText("No matches")).toBeInTheDocument(); + expect(screen.getByText("Nothing found")).toBeInTheDocument(); + expect(within(box).queryAllByRole("option")).toHaveLength(0); + expect(screen.getByRole("button", { name: "Replace" })).toBeDisabled(); + }); + + it.each([false, true])("keeps opening an outdated result available in replace=%s", async (replace) => { + render(); + act(() => showFindPopup(replace, { query: "order" })); + await settle(); + fireEvent.change(field(), { target: { value: "absent" } }); + if (replace) act(() => void runFindPopupCommand("openResult")); + else fireEvent.keyDown(field(), { key: "Enter" }); + expect(useFindPopup.getState().open).toBe(false); + expect(replaced).toHaveLength(0); + }); + + it("rejects Replace All if the query changes while confirmation is open", async () => { + render(); + act(() => showFindPopup(true, { query: "order" })); + await settle(); + let confirm!: (ok: boolean) => void; + confirmation = new Promise((r) => { + confirm = r; + }); + fireEvent.click(screen.getByRole("button", { name: "Replace All" })); + expect(confirms).toHaveLength(1); + fireEvent.change(field(), { target: { value: "ord" } }); + await settle(); + await act(async () => confirm(true)); + expect(replaced).toHaveLength(0); + }); +}); diff --git a/src/features/search/__tests__/findSession.test.ts b/src/features/search/__tests__/findSession.test.ts index bc8c8f6a2..58b74eb79 100644 --- a/src/features/search/__tests__/findSession.test.ts +++ b/src/features/search/__tests__/findSession.test.ts @@ -96,6 +96,59 @@ describe("FindSession", () => { expect(last().status).toBe("searching"); }); + it("invalidates before debounce, drops old events, and keeps rows until the first new file", async () => { + const { session, deps, running, last } = setup(); + void session.search(q("foo")); + await flush(); + running[0]!.onFile(file("old.kt", [1, 2])); + await flush(); + const oldFiles = last().files; + session.invalidate(); + expect(deps.cancel).toHaveBeenCalledWith(1); + expect(last()).toMatchObject({ stale: true, matchCount: 0, summary: null }); + expect(last().files).toBe(oldFiles); + running[0]!.onFile(file("late.kt", [3])); + running[0]!.finish(summary(3)); + await flush(); + expect(last().files).toBe(oldFiles); + let validate!: () => void; + vi.mocked(deps.validate).mockImplementationOnce( + () => + new Promise((r) => { + validate = r; + }), + ); + void session.search(q("bar")); + expect(last().stale).toBe(true); + expect(last().files).toBe(oldFiles); + validate(); + await flush(); + expect(last().status).toBe("searching"); + expect(last().stale).toBe(true); + expect(last().files).toBe(oldFiles); + running[1]!.onFile(file("new.kt", [4])); + await flush(); + expect(last().stale).toBe(false); + expect(last().files.map((f) => f.relPath)).toEqual(["new.kt"]); + expect(last().matchCount).toBe(1); + }); + + it("clears retained rows on completion without results", async () => { + const { session, running, last } = setup(); + void session.search(q("foo")); + await flush(); + running[0]!.onFile(file("old.kt", [1])); + running[0]!.finish(summary(1)); + await flush(); + void session.search(q("absent")); + await flush(); + expect(last().stale).toBe(true); + expect(last().files).toHaveLength(1); + running[1]!.finish({ ...summary(0), files: 0 }); + await flush(); + expect(last()).toMatchObject({ status: "done", stale: false, files: [], matchCount: 0 }); + }); + it("reports invalid patterns without starting a search", async () => { const { session, deps, last } = setup(); await session.search(q("(", { isRegex: true })); diff --git a/src/features/search/find/FindInFiles.module.css b/src/features/search/find/FindInFiles.module.css index 76c4ed1de..da28768ad 100644 --- a/src/features/search/find/FindInFiles.module.css +++ b/src/features/search/find/FindInFiles.module.css @@ -120,6 +120,12 @@ .results > * { flex: 1; } +.outdated .rowText, +.outdated .rowText *, +.outdated .rowFile, +.outdated .rowFile * { + color: var(--fg-faint); +} .rowText { flex: 1; min-width: 0; diff --git a/src/features/search/find/FindInFiles.tsx b/src/features/search/find/FindInFiles.tsx index 957b8bba1..f265465fb 100644 --- a/src/features/search/find/FindInFiles.tsx +++ b/src/features/search/find/FindInFiles.tsx @@ -193,8 +193,12 @@ function FindInFilesDialog() { ); const queryKey = JSON.stringify(query); + const stale = state.stale || (state.query ? JSON.stringify(state.query) !== queryKey : !!s.pattern); + const currentQuery = useRef(queryKey); const [rerun, setRerun] = useState(0); - useEffect(() => { + useLayoutEffect(() => { + currentQuery.current = queryKey; + session.invalidate(); const t = setTimeout(() => { void session.search(query); setIndex(0); @@ -209,13 +213,13 @@ function FindInFilesDialog() { const selected = active >= 0 ? rows[active] : undefined; const chosen = useMemo(() => selectedRows(rows, sel), [rows, sel]); const fileHighlights = useMemo(() => { - if (!selected) return []; + if (stale || !selected) return []; const f = state.files.find((x) => x.path === selected.path); return (f?.matches ?? []).map((m) => ({ line: m.line, ranges: m.ranges.map(([a, b]) => [a + m.textOffset, b + m.textOffset] as [number, number]), })); - }, [selected, state.files]); + }, [stale, selected, state.files]); const close = () => { rememberFindString("find", s.pattern); @@ -243,8 +247,13 @@ function FindInFilesDialog() { openInFindWindow(pinned, describeQuery(pinned, root), s.replace ? s.replacement : null, undefined, s.preserveCase); }; + // Check again after a confirmation: its captured rows may have been superseded. + const canReplace = () => + !stale && !session.current.stale && currentQuery.current === queryKey && session.current.files === state.files; + /** IDEA asks before Replace All only when the results span more than one file. */ const confirmReplaceAll = async () => { + if (!canReplace()) return; const usages = pendingUsages(rows, marks); if (!usages.length) return; const counts = countUsages(usages); @@ -278,7 +287,7 @@ function FindInFilesDialog() { /** Replaces `usages` in the backend, then marks them done (IDEA's Replace / Replace All). */ const replaceUsages = async (usages: Usage[]): Promise => { - if (!usages.length || running) return null; + if (!canReplace() || !usages.length || running) return null; const run = startReplace( { query, replacement: s.replacement, preserveCase: s.preserveCase, targets: replaceTargets(usages) }, setProgress, @@ -349,10 +358,18 @@ function FindInFilesDialog() { if (done) close(); }; + const diffPreviewKey = JSON.stringify([ + selected?.key, + queryKey, + s.replacement, + s.preserveCase, + rerun, + previewVersion, + ]); useEffect(() => { - if (!previewMode || !s.replace || !selected) return; + if (stale || !previewMode || !s.replace || !selected) return; let alive = true; - const key = selected.key; + const key = diffPreviewKey; const usages = pendingUsages([selected], marks); const t = setTimeout(() => { loadReplacePreview(query, s.replacement, s.preserveCase, selected.path, usages).then( @@ -365,7 +382,7 @@ function FindInFilesDialog() { clearTimeout(t); }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [previewMode, s.replace, selected?.key, s.replacement, s.preserveCase, queryKey, marks, previewVersion]); + }, [stale, previewMode, s.replace, diffPreviewKey, marks]); const moveTo = (next: number, extend: boolean) => setSel((cur) => selectRow(rows, cur, next, extend ? "range" : "single")); @@ -443,7 +460,7 @@ function FindInFilesDialog() { return () => setFindPopupCommandHandler(null); }, []); - const summary = statusText(state); + const summary = stale ? (s.pattern ? tx("Updating results…") : "") : statusText(state); const maskItems: MenuItem[] = recentMasks().map((m) => ({ label: m, checked: m === s.mask ? true : undefined, @@ -467,7 +484,7 @@ function FindInFilesDialog() { checked: c.value === s.context ? true : undefined, run: () => set({ context: c.value }), })); - const pendingSelected = s.replace ? pendingUsages(chosen, marks).length : 0; + const pendingSelected = s.replace && !stale ? pendingUsages(chosen, marks).length : 0; return ( {s.replace ? tx("Replace in Files") : tx("Find in Files")} - {state.status === "searching" || state.status === "validating" ? : null} + {stale || state.status === "searching" || state.status === "validating" ? : null} {summary} @@ -664,7 +681,11 @@ function FindInFilesDialog() { )} -
+
items={rows} getKey={(r, i) => `${r.key}:${i}`} @@ -691,10 +712,10 @@ function FindInFilesDialog() { > @@ -725,9 +746,9 @@ function FindInFilesDialog() { )}
- {previewMode && s.replace && selected ? ( + {!stale && previewMode && s.replace && selected ? ( ) : ( @@ -760,19 +781,19 @@ function FindInFilesDialog() { {s.replace && ( <> - - -