fix(web): de-race directory browsing in the files panel and workspace picker (#222)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Yaowei Zheng
2026-08-06 20:08:27 +08:00
committed by GitHub
parent 8fc26cc44f
commit c98749fbff
8 changed files with 182 additions and 55 deletions
+29 -2
View File
@@ -201,12 +201,39 @@ test("draft: pick model/approval -> reload restores them -> send creates the ses
await expect(page.getByRole("textbox", { name: "Workspace" })).toHaveValue(
new RegExp(`${wsLabel}$`),
);
await page.getByRole("button", { name: "上级目录" }).click();
// Regression (workspace picker race): while a /dirs request is in flight the picker's rows
// are disabled, so a rapid double-click on "parent dir" must issue exactly ONE request and
// ascend exactly one level — previously both clicks fired an un-sequenced load and could
// relocate the browsing position. The response is gated on an explicit release (not a
// timeout) so the second click deterministically lands inside the loading window.
let releaseDirs;
const dirsGate = new Promise((resolve) => {
releaseDirs = resolve;
});
let dirsRequests = 0;
const dirsRoute = (url) => url.pathname.endsWith("/dirs");
const gateDirs = async (route) => {
dirsRequests += 1;
await dirsGate;
await route.continue();
};
await page.route(dirsRoute, gateDirs);
const upRow = page.getByRole("button", { name: "上级目录" });
await upRow.click();
// force: the row is disabled while loading, and a plain click would stall on Playwright's
// actionability wait instead of exercising the double-click; the disabled button swallows it.
await upRow.click({ force: true });
releaseDirs();
const parentLabel = basename(dirname(namedWs));
await expect(page.getByRole("textbox", { name: "Workspace" })).toHaveValue(
new RegExp(`${parentLabel}$`),
);
await expect(page.getByRole("textbox", { name: "Workspace" })).not.toHaveValue(
new RegExp(`${wsLabel}$`),
);
expect(dirsRequests, "double-click while loading fires a single /dirs request").toBe(1);
await page.unroute(dirsRoute, gateDirs);
await page.getByRole("button", { name: "使用此目录" }).click();
const parentLabel = basename(dirname(namedWs));
await expect(page.getByLabel("Workspace")).toContainText(parentLabel);
await page.reload();
await expect(page.getByLabel("Workspace")).toContainText(parentLabel);
@@ -10,6 +10,7 @@ import type { Components } from "react-markdown";
import remarkGfm from "remark-gfm";
import * as api from "../../api/endpoints";
import { apiErrorText } from "../../lib/api-error";
import { joinWorkspacePath } from "../../lib/file-path";
import { formatBytes } from "../../lib/format";
import { S } from "../../lib/strings";
import { SkeletonList } from "../../components/ui/skeleton";
@@ -77,10 +78,6 @@ function extOf(name: string): string {
return index >= 0 ? name.slice(index + 1).toLowerCase() : name.toLowerCase();
}
function joinPath(dir: string, name: string): string {
return dir === "" ? name : `${dir}/${name}`;
}
function dirOf(filePath: string): string {
return filePath.includes("/") ? filePath.slice(0, filePath.lastIndexOf("/")) : "";
}
@@ -136,7 +133,12 @@ function MaterialGroup({
}: MaterialGroupProps) {
const [open, setOpen] = useState(defaultOpen);
const [path, setPath] = useState("");
const [listing, setListing] = useState<WorkspaceFilesResponse | null>(null);
/** Bound to the path it was fetched for: entry targets join against `base`, so a click on a
* row that is momentarily stale (the fetch effect nulls the listing, but the state update
* commits one frame later) cannot compound segments onto an already-advanced `path`. */
const [listing, setListing] = useState<{ base: string; res: WorkspaceFilesResponse } | null>(
null,
);
const [listError, setListError] = useState<string | null>(null);
const initialReadmeOpened = useRef(false);
@@ -149,7 +151,7 @@ function MaterialGroup({
.listBenchmarkCaseFiles(projectId, agentId, benchmarkId, caseSummary.id, path, material)
.then((data) => {
if (cancelled) return;
setListing(data);
setListing({ base: path, res: data });
if (path === "" && !initialReadmeOpened.current) {
initialReadmeOpened.current = true;
const readme = data.entries.find(
@@ -169,11 +171,13 @@ function MaterialGroup({
const crumbs = path === "" ? [] : path.split("/");
const openEntry = (entry: WorkspaceFileEntry) => {
if (listing === null) return; // rows only render out of a loaded listing
const target = joinWorkspacePath(listing.base, entry.name);
if (entry.kind === "dir") {
setPath(joinPath(path, entry.name));
setPath(target);
return;
}
onPreview(material, joinPath(path, entry.name));
onPreview(material, target);
};
return (
@@ -219,10 +223,10 @@ function MaterialGroup({
)}
{listError && <p className="px-6 py-2 text-xs text-red-500">{listError}</p>}
{!listing && !listError && <SkeletonList rows={3} />}
{listing?.entries.length === 0 && (
{listing?.res.entries.length === 0 && (
<p className="px-6 py-2 text-xs text-gray-400">{S.files.empty}</p>
)}
{listing?.entries.map((entry) => (
{listing?.res.entries.map((entry) => (
<button
key={`${entry.kind}/${entry.name}`}
type="button"
@@ -22,6 +22,7 @@ import { ApiError } from "../../api/client";
import { useAuth } from "../../state/auth";
import { S } from "../../lib/strings";
import { apiErrorText } from "../../lib/api-error";
import { joinWorkspacePath } from "../../lib/file-path";
import { formatBytes, formatDateTime } from "../../lib/format";
import { Button } from "../../components/ui/button";
import { ConfirmModal } from "../../components/ui/confirm-modal";
@@ -120,10 +121,6 @@ function extOf(name: string): string {
return i >= 0 ? name.slice(i + 1).toLowerCase() : name.toLowerCase();
}
function joinPath(dir: string, name: string): string {
return dir === "" ? name : `${dir}/${name}`;
}
function dirOf(filePath: string): string {
return filePath.includes("/") ? filePath.slice(0, filePath.lastIndexOf("/")) : "";
}
@@ -207,7 +204,15 @@ export function WorkspaceBrowser({
// silently in the page) and the in-app rendered view to the srcDoc fallback.
const { previewIsolated } = useAuth();
const [path, setPath] = useState("");
const [data, setData] = useState<WorkspaceFilesResponse | null>(null);
/**
* The loaded listing, bound to the path it was fetched for. Entry-row targets (descend /
* preview / download) are joined against `base` — the generation the rendered rows came
* from — never against the live `path` state: the stale rows stay on screen while a
* navigation fetch is in flight, and joining onto the already-advanced path would compound
* segments on a double-click ("home" → "home/home", a directory that does not exist).
* Base-bound targets make a repeated click recompute the same target, i.e. a no-op.
*/
const [data, setData] = useState<{ base: string; res: WorkspaceFilesResponse } | null>(null);
const [error, setError] = useState<string | null>(null);
const [preview, setPreview] = useState<Preview | null>(null);
const [uploading, setUploading] = useState(false);
@@ -234,7 +239,7 @@ export function WorkspaceBrowser({
api
.listWorkspaceFiles(session.sessionId, path)
.then((res) => {
if (!cancelled) setData(res);
if (!cancelled) setData({ base: path, res });
})
.catch((e: unknown) => {
if (!cancelled) setError(e instanceof ApiError ? e.message : S.files.loadFailed);
@@ -403,10 +408,6 @@ export function WorkspaceBrowser({
void previewPath(target);
}, [openRequest, previewPath]);
const openEntry = (name: string) => {
void previewPath(joinPath(path, name));
};
const doUpload = (files: File[]) => {
setUploading(true);
setError(null);
@@ -422,7 +423,7 @@ export function WorkspaceBrowser({
reader.onerror = () => reject(new Error("read failed"));
reader.readAsDataURL(file);
});
await api.uploadWorkspaceFile(session.sessionId, joinPath(path, file.name), b64);
await api.uploadWorkspaceFile(session.sessionId, joinWorkspacePath(path, file.name), b64);
}
toastSuccess(S.files.uploaded);
setReloadTick((t) => t + 1);
@@ -440,13 +441,22 @@ export function WorkspaceBrowser({
if (files.length === 0) return;
// Uploads overwrite same-name files: names already present in the loaded listing
// confirm first (the picker is stashed — confirm continues, cancel drops it).
const existing = new Set((data?.entries ?? []).map((entry) => entry.name));
const existing = new Set((data?.res.entries ?? []).map((entry) => entry.name));
const clashes = files.filter((f) => existing.has(f.name)).map((f) => f.name);
if (clashes.length > 0) setPendingUpload({ files, clashes });
else doUpload(files);
};
const crumbs = path === "" ? [] : path.split("/");
/**
* A navigation fetch is in flight: the rendered rows belong to a different directory than
* the one being loaded. Entry rows and breadcrumbs are disabled for the duration — the
* base-bound targets above are what make clicks safe regardless of timing; this is the
* user-visible feedback. Derived, not stored: initial load and same-path refreshes
* (reloadTick) don't count, and a failed navigation renders `error` instead of rows, so
* the disabled state can never outlive the fetch that justified it.
*/
const busy = error === null && data !== null && data.base !== path;
if (preview !== null) {
return (
@@ -699,10 +709,11 @@ export function WorkspaceBrowser({
<div className="flex shrink-0 flex-wrap items-center gap-1 border-b border-gray-200 px-3 py-2 dark:border-gray-800">
<button
type="button"
disabled={busy}
onClick={() => {
setPath("");
}}
className="rounded px-1.5 py-0.5 text-sm text-gray-600 transition-colors duration-150 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800"
className="rounded px-1.5 py-0.5 text-sm text-gray-600 transition-colors duration-150 hover:bg-gray-100 disabled:opacity-50 dark:text-gray-300 dark:hover:bg-gray-800"
>
{S.files.root}
</button>
@@ -711,8 +722,9 @@ export function WorkspaceBrowser({
<span className="text-gray-300 dark:text-gray-700">/</span>
<button
type="button"
disabled={busy}
onClick={() => setPath(crumbs.slice(0, i + 1).join("/"))}
className="max-w-32 truncate rounded px-1 py-0.5 text-sm text-gray-600 transition-colors duration-150 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800"
className="max-w-32 truncate rounded px-1 py-0.5 text-sm text-gray-600 transition-colors duration-150 hover:bg-gray-100 disabled:opacity-50 dark:text-gray-300 dark:hover:bg-gray-800"
>
{seg}
</button>
@@ -756,21 +768,28 @@ export function WorkspaceBrowser({
<p className="px-3 py-3 text-sm text-red-600 dark:text-red-400">{error}</p>
) : data === null ? (
<SkeletonList rows={6} />
) : data.entries.length === 0 ? (
) : data.res.entries.length === 0 ? (
<p className="px-3 py-3 text-sm text-gray-400">{S.files.empty}</p>
) : (
// No "up a level" row: going up a level is done via the toolbar breadcrumbs (root / any segment is clickable).
<ul className="divide-y divide-gray-100 dark:divide-gray-800/60">
{data.entries.map((entry) => (
// While a navigation is in flight (busy) the stale rows stay visible but dimmed and inert.
<ul
aria-busy={busy}
className={`divide-y divide-gray-100 dark:divide-gray-800/60 ${busy ? "opacity-60" : ""}`}
>
{data.res.entries.map((entry) => (
<li key={entry.name}>
<div className="group flex items-center gap-2 px-3 py-1.5 transition-colors duration-150 hover:bg-gray-50 dark:hover:bg-gray-800/50">
<button
type="button"
onClick={() =>
entry.kind === "dir"
? setPath(joinPath(path, entry.name))
: openEntry(entry.name)
}
disabled={busy}
onClick={() => {
// Joined against the listing's own base (see the data state comment): a second
// click on the same stale row resolves to the same target, not a deeper one.
const target = joinWorkspacePath(data.base, entry.name);
if (entry.kind === "dir") setPath(target);
else void previewPath(target);
}}
className="flex min-w-0 flex-1 items-center gap-2 text-left"
title={entry.name}
>
@@ -811,7 +830,7 @@ export function WorkspaceBrowser({
<a
href={api.workspaceFileUrl(
session.sessionId,
joinPath(path, entry.name),
joinWorkspacePath(data.base, entry.name),
true,
)}
download={entry.name}
@@ -72,16 +72,39 @@ export function WorkspaceSelect({
setPathDraft(dir?.path ?? "");
}, [dir]);
/** Browses level by level (clicking a directory/parent); an empty string means the server's home directory (the default starting point). */
/**
* Monotonic id of the newest loadDir request. Only the newest request may publish its
* result: navigations race (rows stay visible while a fetch is in flight, and the path row
* accepts commits at any time), and without the guard a slow older response would overwrite
* a newer one — silently relocating the browsing position — or clear `loading` while the
* newer request is still in flight.
*/
const loadSeq = useRef(0);
/**
* Browses level by level (clicking a directory/parent); an empty string means the server's
* home directory (the default starting point). `onError` overrides the default error-row
* handling (the path-edit commit toasts and reverts instead); it only ever fires for the
* newest request, like every other outcome.
*/
const loadDir = useCallback(
(abs: string) => {
(abs: string, opts?: { onError?: () => void }) => {
const seq = ++loadSeq.current;
setLoading(true);
setError(null);
api
.listDirs(projectId, abs)
.then(setDir)
.catch((e: unknown) => setError(apiErrorText(e)))
.finally(() => setLoading(false));
.then((res) => {
if (seq === loadSeq.current) setDir(res);
})
.catch((e: unknown) => {
if (seq !== loadSeq.current) return;
if (opts?.onError) opts.onError();
else setError(apiErrorText(e));
})
.finally(() => {
if (seq === loadSeq.current) setLoading(false);
});
},
[projectId],
);
@@ -113,19 +136,24 @@ export function WorkspaceSelect({
}
};
/** Commits the edited path: navigates to it if it exists, otherwise toasts and reverts to the current browsing position. */
const commitPathEdit = async () => {
/**
* Commits the edited path: navigates to it if it exists, otherwise toasts and reverts to
* the current browsing position. Routed through loadDir so the commit shows the loading
* row and participates in the same request sequencing as row clicks — a raw listDirs here
* used to race them and could land after (and clobber) a newer navigation.
*/
const commitPathEdit = () => {
const p = pathDraft.trim();
if (!p || p === dir?.path) {
setPathDraft(dir?.path ?? "");
return;
}
try {
setDir(await api.listDirs(projectId, p));
} catch {
toastError(S.chat.workspaceDirInvalid);
setPathDraft(dir?.path ?? "");
}
loadDir(p, {
onError: () => {
toastError(S.chat.workspaceDirInvalid);
setPathDraft(dir?.path ?? "");
},
});
};
const trimmed = workspace.trim();
@@ -208,11 +236,11 @@ export function WorkspaceSelect({
aria-label={S.chat.workspace}
{...noAutofill}
onChange={(e) => setPathDraft(e.target.value)}
onBlur={() => void commitPathEdit()}
onBlur={commitPathEdit}
onKeyDown={(e) => {
if (e.key === "Enter" && !e.nativeEvent.isComposing) {
e.preventDefault();
void commitPathEdit();
commitPathEdit();
} else if (e.key === "Escape") {
// Discard the edit: only reverts the draft; Escape bubbles up to Dropdown, which closes the menu.
setPathDraft(dir?.path ?? "");
@@ -235,12 +263,16 @@ export function WorkspaceSelect({
</div>
{/* Directory list (excludes hidden directories) */}
<ul className="max-h-40 overflow-y-auto py-1">
{/* Rows are disabled while a load is in flight: they still show the PREVIOUS
directory until the response lands, so clicks during the window would resend
stale targets (N clicks on "parent" all resending the same dir.parent). */}
{parentPath !== null && (
<li>
<button
type="button"
disabled={loading}
onClick={() => loadDir(parentPath)}
className="flex w-full items-center gap-2 px-2.5 py-1.5 text-left font-mono text-xs text-gray-500 transition-colors duration-150 hover:bg-gray-100 dark:text-gray-400 dark:hover:bg-gray-800"
className="flex w-full items-center gap-2 px-2.5 py-1.5 text-left font-mono text-xs text-gray-500 transition-colors duration-150 hover:bg-gray-100 disabled:opacity-50 dark:text-gray-400 dark:hover:bg-gray-800"
>
↰ {S.chat.workspaceUp}
</button>
@@ -250,8 +282,9 @@ export function WorkspaceSelect({
<li key={entry.path}>
<button
type="button"
disabled={loading}
onClick={() => loadDir(entry.path)}
className="flex w-full items-center gap-2 px-2.5 py-1.5 text-left font-mono text-xs text-gray-700 transition-colors duration-150 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800"
className="flex w-full items-center gap-2 px-2.5 py-1.5 text-left font-mono text-xs text-gray-700 transition-colors duration-150 hover:bg-gray-100 disabled:opacity-50 dark:text-gray-300 dark:hover:bg-gray-800"
>
<svg
width="13"
@@ -284,8 +317,9 @@ export function WorkspaceSelect({
</span>
<button
type="button"
disabled={loading}
onClick={() => loadDir("")}
className="shrink-0 rounded border border-gray-300 px-1.5 py-0.5 text-xs text-gray-700 transition-colors duration-150 hover:bg-gray-100 dark:border-gray-700 dark:text-gray-300 dark:hover:bg-gray-800"
className="shrink-0 rounded border border-gray-300 px-1.5 py-0.5 text-xs text-gray-700 transition-colors duration-150 hover:bg-gray-100 disabled:opacity-50 dark:border-gray-700 dark:text-gray-300 dark:hover:bg-gray-800"
>
{S.common.retry}
</button>
+16
View File
@@ -79,6 +79,22 @@ export function isFilePathLike(text: string): boolean {
return KNOWN_EXTENSIONS.has(ext);
}
/**
* Join a directory base path with a child entry name (Workspace-relative
* convention: "" is the Workspace root).
*
* Navigation targets must be computed from the base of the **listing the entry
* row came from**, never from the browser's current path state: stale rows stay
* rendered while a navigation fetch is in flight, and joining onto the
* already-advanced path turns a double-click into a compounded descent
* ("home" → "home/home" → "home/home/home", a path that does not exist).
* Keyed to the listing's own base, repeated clicks on the same row recompute
* the same target, so navigation is idempotent regardless of fetch timing.
*/
export function joinWorkspacePath(base: string, name: string): string {
return base === "" ? name : `${base}/${name}`;
}
/** Max length for a single files/stat path (matches server-side validation). */
const MAX_PATH_LEN = 512;
+1
View File
@@ -1155,6 +1155,7 @@ Scenarios:
too_many_files: "Too many files attached to one message.",
payload_too_large: "The request is too large.",
dir_not_absolute: "The directory must be an absolute path.",
dir_not_found: "That directory does not exist or is inaccessible.",
not_a_dir: "That path is not a directory.",
path_not_found: "That path does not exist.",
workspace_missing: "This Session's Workspace no longer exists.",
+1
View File
@@ -1129,6 +1129,7 @@ Benchmark:
too_many_files: "一条消息附加的文件过多。",
payload_too_large: "请求体过大。",
dir_not_absolute: "目录必须是绝对路径。",
dir_not_found: "该目录不存在或不可访问。",
not_a_dir: "该路径不是目录。",
path_not_found: "该路径不存在。",
workspace_missing: "该 Session 的 Workspace 已不存在。",
+27 -2
View File
@@ -1,9 +1,10 @@
/**
* file-path.ts unit tests: whether inline code in message text looks like a file path,
* and all branches of normalizing body paths to a Workspace-relative path (toWorkspaceRelative).
* all branches of normalizing body paths to a Workspace-relative path (toWorkspaceRelative),
* and the directory-browser navigation join (joinWorkspacePath).
*/
import { describe, expect, it } from "vitest";
import { isFilePathLike, toWorkspaceRelative } from "../src/lib/file-path";
import { isFilePathLike, joinWorkspacePath, toWorkspaceRelative } from "../src/lib/file-path";
describe("isFilePathLike", () => {
it("relative path + image extension → match", () => {
@@ -133,3 +134,27 @@ describe("toWorkspaceRelative", () => {
expect(toWorkspaceRelative(` ${WS}/a.txt `, WS)).toBe("a.txt");
});
});
describe("joinWorkspacePath", () => {
it("joins an entry name onto the Workspace root (empty base)", () => {
expect(joinWorkspacePath("", "home")).toBe("home");
});
it("joins an entry name onto a nested base", () => {
expect(joinWorkspacePath("home/user", "docs")).toBe("home/user/docs");
});
it("two clicks on the same row from the same listing generation are idempotent (regression: compounded descent)", () => {
// The files-panel race: while the navigation fetch is in flight, the listing for `base`
// stays rendered but the path state has already advanced. Click #2 on the same stale row
// must recompute the SAME target from the listing's own base…
const base = "";
const first = joinWorkspacePath(base, "home");
const pathStateAfterFirstClick = first;
const second = joinWorkspacePath(base, "home");
expect(second).toBe(first);
// …whereas joining against the advanced path state — the old behavior — compounds the
// segment into a directory that does not exist ("home/home", then "home/home/home", …).
expect(joinWorkspacePath(pathStateAfterFirstClick, "home")).toBe("home/home");
});
});