diff --git a/src/contexts/workspace-context.test.tsx b/src/contexts/workspace-context.test.tsx index a2f715a364..672ae90f35 100644 --- a/src/contexts/workspace-context.test.tsx +++ b/src/contexts/workspace-context.test.tsx @@ -116,6 +116,7 @@ beforeEach(() => { vi.mock("@/lib/api", () => ({ getHomeDirectory: vi.fn(), + listDirectoryWithFiles: vi.fn(), readFileForEdit: vi.fn(), readFileBase64: vi.fn(), readFilePreview: vi.fn(), @@ -260,6 +261,7 @@ function WorkspaceProbe() { activeFileTabId, filesMaximized, openSessionFileDiff, + openFilePreview, closeFileTab, closeAllFileTabs, toggleFilesMaximized, @@ -268,6 +270,9 @@ function WorkspaceProbe() { return (
+ {mode} {fileTabs.length} {activePane} @@ -2009,6 +2014,19 @@ describe("WorkspaceProvider office auto-preview", () => { workspaceStoreMock.reset() // Preference defaults ON; drop any "false" a prior test left behind. localStorage.removeItem("workspace:office-auto-preview") + // Reset first: call history is what the "one listing per parent" tests + // assert on, and an unconsumed `…Once` from a prior test would otherwise + // answer the next one's first lookup. + vi.mocked(api.listDirectoryWithFiles).mockReset() + vi.mocked(api.listDirectoryWithFiles).mockImplementation(async (root) => + ["report.pptx", "deck.pptx", "report.docx"].map((name) => ({ + name, + path: `${root}/${name}`, + isDir: false, + hasChildren: false, + size: 100, + })) + ) }) it("auto-opens an office file's preview when the watcher reports it, with no aux panel involved", async () => { @@ -2126,6 +2144,157 @@ describe("WorkspaceProvider office auto-preview", () => { expect(screen.getByTestId("file-tab-count")).toHaveTextContent("1") }) + it("does not open a removed document from a changed_paths envelope", async () => { + vi.mocked(api.listDirectoryWithFiles).mockResolvedValue([]) + renderWorkspace() + + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + expect(screen.getByTestId("active-pane")).toHaveTextContent("conversation") + }) + + it("does not open documents from a removed parent directory", async () => { + vi.mocked(api.listDirectoryWithFiles).mockRejectedValue( + new Error("Path is not a directory") + ) + renderWorkspace() + + await act(async () => { + workspaceStoreMock.emitEnvelope(["scratch/report.docx"]) + }) + + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + }) + + it("keeps a dismissed preview closed after switching folders and back", async () => { + renderWorkspace() + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + act(() => screen.getByRole("button", { name: "Close active" }).click()) + act(() => foldersMock.setActiveFolderId(2)) + act(() => foldersMock.setActiveFolderId(1)) + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + }) + + it("can open a document recreated after an earlier removal event", async () => { + vi.mocked(api.listDirectoryWithFiles).mockResolvedValueOnce([]) + renderWorkspace() + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("1") + }) + + it("does not mistake a directory with an Office suffix for a document", async () => { + vi.mocked(api.listDirectoryWithFiles).mockResolvedValue([ + { + name: "report.docx", + path: "/repo/report.docx", + isDir: true, + hasChildren: true, + size: null, + }, + ]) + renderWorkspace() + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + }) + + it("discards a pending preview when its workspace is no longer active", async () => { + let finish!: ( + entries: Awaited> + ) => void + vi.mocked(api.listDirectoryWithFiles).mockReturnValueOnce( + new Promise((resolve) => { + finish = resolve + }) + ) + renderWorkspace() + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + act(() => foldersMock.setActiveFolderId(2)) + await act(async () => { + finish([ + { + name: "report.docx", + path: "/repo/report.docx", + isDir: false, + hasChildren: false, + size: 100, + }, + ]) + }) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + }) + + it("lists a parent once per burst and never re-lists a decided path", async () => { + // The existence probe sits on the watcher's hot path, and the backing + // command stats every sibling and ships the whole listing to the renderer + // (over HTTP in server/remote mode). Both bounds below are what keep that + // affordable: one lookup per parent per envelope, and none at all once a + // path has been decided. + renderWorkspace() + + await act(async () => { + workspaceStoreMock.emitEnvelope([ + "report.docx", + "deck.pptx", + "report.pptx", + ]) + }) + + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("3") + expect(api.listDirectoryWithFiles).toHaveBeenCalledTimes(1) + expect(api.listDirectoryWithFiles).toHaveBeenCalledWith("/repo") + + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx", "deck.pptx"]) + }) + + expect(api.listDirectoryWithFiles).toHaveBeenCalledTimes(1) + }) + + it("does not resurface a document the user opened by hand and closed", async () => { + renderWorkspace() + + await act(async () => { + screen.getByRole("button", { name: "Open office by hand" }).click() + }) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("1") + + // The agent writes while the hand-opened tab is up: nothing to open, and + // no existence probe either — the open tab already answers the question. + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + expect(api.listDirectoryWithFiles).not.toHaveBeenCalled() + + act(() => screen.getByRole("button", { name: "Close active" }).click()) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + + // A document the user has already seen and dismissed stays dismissed. + await act(async () => { + workspaceStoreMock.emitEnvelope(["report.docx"]) + }) + expect(screen.getByTestId("file-tab-count")).toHaveTextContent("0") + }) + it("does not auto-open when the preference is disabled", async () => { localStorage.setItem("workspace:office-auto-preview", "false") renderWorkspace() diff --git a/src/contexts/workspace-context.tsx b/src/contexts/workspace-context.tsx index 954189b6c0..cd4d2d256a 100644 --- a/src/contexts/workspace-context.tsx +++ b/src/contexts/workspace-context.tsx @@ -20,6 +20,7 @@ import { gitIsTracked, gitShowDiff, gitShowFile, + listDirectoryWithFiles, readFileBase64, readFileForEdit, readFilePreview, @@ -544,6 +545,9 @@ export function WorkspaceProvider({ children }: WorkspaceProviderProps) { new Map() ) const fileTabsRef = useRef([]) + // Keep dismissals across folder changes and effect re-subscriptions. A + // watcher event must not reopen a preview the user already closed. + const autoOpenedOfficePathsRef = useRef(new Set()) // Latest-state mirrors for the stable action callbacks. Actions live in a // context value that must NOT change identity when tabs/folder change, so // they read these refs instead of capturing render-scoped state. The refs @@ -1849,10 +1853,16 @@ export function WorkspaceProvider({ children }: WorkspaceProviderProps) { // Leading-edge with dedup: an agent building a doc fires a burst of writes, // so we open on first sighting and remember it in `autoOpened` (which also // keeps a tab the user has since closed from popping back open). - const autoOpened = new Set() + const autoOpened = autoOpenedOfficePathsRef.current + const pending = new Set() + let cancelled = false const streamRoot = folderPath const unsubscribe = subscribeOfficeEnvelopes(({ changed_paths }) => { if (!changed_paths || changed_paths.length === 0) return + const directories = new Map< + string, + ReturnType + >() // Tab identity is the absolute path, so joining the stream root onto // the changed relative path compares exactly — an identically-named // doc in another folder has a different absolute path and never @@ -1877,12 +1887,53 @@ export function WorkspaceProvider({ children }: WorkspaceProviderProps) { // at once — which arrived here as a dozen unreadable previews. if (isOfficeOwnerFile(changed)) continue const abs = joinRootRel(streamRoot, changed) - if (autoOpened.has(abs) || openPaths.has(abs)) continue - autoOpened.add(abs) - void openFilePreview(abs) + if (autoOpened.has(abs) || pending.has(abs)) continue + // An already-open tab counts as a sighting, not just a skip: this + // feature exists to surface documents the user has NOT seen, so a tab + // they opened by hand must not become a fresh auto-open the moment + // they close it and the agent writes again. + if (openPaths.has(abs)) { + autoOpened.add(abs) + continue + } + const io = splitAbsPath(abs) + if (!io) continue + pending.add(abs) + // changed_paths includes removals, including removed worktree copies. + // Inspect directory metadata before opening a tab, without reading or + // locking the Office document. Share one listing per parent per burst. + let listing = directories.get(io.rootPath) + if (!listing) { + listing = listDirectoryWithFiles(io.rootPath) + directories.set(io.rootPath, listing) + } + void listing + .then((entries) => { + if (cancelled || autoOpened.has(abs)) return + const exists = entries.some( + (entry) => + !entry.isDir && + entry.size != null && + normalizeAbsPath(entry.path) === abs + ) + if (!exists) return + autoOpened.add(abs) + // A manual open during the lookup already handled this file. + if (fileTabsRef.current.some((tab) => tab.path === abs)) return + return openFilePreview(abs) + }) + .catch(() => { + // Covers both halves of the chain: a removed/unreadable parent is + // not a document to preview, and `openFilePreview` already reports + // its own failures on the tab it seeded. + }) + .finally(() => pending.delete(abs)) } }) - return unsubscribe + return () => { + cancelled = true + unsubscribe() + } }, [ folderPath, activeFolderIdForOffice,