fix(web): keep file tree and terminal waiting through long prepare (#564)
This commit is contained in:
@@ -210,8 +210,13 @@ export function PrepareProgress({ sessionId }: PrepareProgressProps) {
|
||||
return (
|
||||
<div className="px-3 py-2 space-y-1.5 border-b border-border/50">
|
||||
<div className="flex items-center gap-1.5 text-xs font-medium text-muted-foreground">
|
||||
{status === "preparing" && <IconLoader2 className="h-3 w-3 animate-spin" />}
|
||||
{status === "failed" && <IconX className="h-3 w-3 text-destructive" />}
|
||||
{status === "preparing" && (
|
||||
<IconLoader2
|
||||
data-testid="prepare-progress-header-spinner"
|
||||
className="h-3.5 w-3.5 animate-spin"
|
||||
/>
|
||||
)}
|
||||
{status === "failed" && <IconX className="h-3.5 w-3.5 text-destructive" />}
|
||||
<span>
|
||||
{status === "preparing" && "Preparing environment..."}
|
||||
{status === "failed" && (prepareState.errorMessage ?? "Environment preparation failed")}
|
||||
|
||||
@@ -277,6 +277,36 @@ function useTreeLoader(ctx: TreeLoaderContext) {
|
||||
return loadTree;
|
||||
}
|
||||
|
||||
function useFileChangeSubscription({
|
||||
sessionIdRef,
|
||||
expandedPaths,
|
||||
setTree,
|
||||
setLoadState,
|
||||
}: {
|
||||
sessionIdRef: React.MutableRefObject<string>;
|
||||
expandedPaths: Set<string>;
|
||||
setTree: React.Dispatch<React.SetStateAction<FileTreeNode | null>>;
|
||||
setLoadState: React.Dispatch<React.SetStateAction<LoadState>>;
|
||||
}) {
|
||||
useEffect(() => {
|
||||
const client = getWebSocketClient();
|
||||
if (!client) return;
|
||||
return client.on("session.workspace.file.changes", (msg) => {
|
||||
const changes = msg.payload?.changes;
|
||||
if (!changes || changes.length === 0) return;
|
||||
applyFileChanges({
|
||||
client,
|
||||
sessionId: sessionIdRef.current,
|
||||
expandedPaths,
|
||||
changes,
|
||||
setTree,
|
||||
setLoadState,
|
||||
});
|
||||
});
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [expandedPaths]);
|
||||
}
|
||||
|
||||
/**
|
||||
* Hook for tree loading with retry logic, file-change subscription, and expanded state.
|
||||
* @param sessionId - Session ID for API calls (agentctl routing).
|
||||
@@ -317,11 +347,18 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) {
|
||||
setLoadError,
|
||||
});
|
||||
|
||||
// Full reset: clear tree + loading state. Only fires when the environment changes.
|
||||
// Refs for effects below to read without adding deps — avoids infinite
|
||||
// retry loops and wipe-on-reconnect when `isReady`/`loadState`/`tree` tick.
|
||||
const agentctlIsReadyRef = useRef(agentctlStatus.isReady);
|
||||
const loadStateRef = useRef(loadState);
|
||||
const treeRef = useRef(tree);
|
||||
agentctlIsReadyRef.current = agentctlStatus.isReady;
|
||||
loadStateRef.current = loadState;
|
||||
treeRef.current = tree;
|
||||
useEffect(() => {
|
||||
setTree(null);
|
||||
setIsLoadingTree(true);
|
||||
setLoadState("loading");
|
||||
setLoadState(agentctlIsReadyRef.current ? "loading" : "waiting");
|
||||
setLoadError(null);
|
||||
retryAttemptRef.current = 0;
|
||||
clearRetryTimer();
|
||||
@@ -329,16 +366,26 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) {
|
||||
const savedPaths = getFilesPanelExpandedPaths(effectiveResetKey);
|
||||
setExpandedPaths(savedPaths.length > 0 ? new Set(savedPaths) : new Set());
|
||||
if (savedPaths.length > 0) hasInitializedExpandedRef.current = effectiveResetKey;
|
||||
void loadTree({ resetRetry: true });
|
||||
// Only load now if agentctl is ready; otherwise the sibling effect fires
|
||||
// on the ready flip. Prevents burning the retry budget during slow prepare.
|
||||
if (agentctlIsReadyRef.current) void loadTree({ resetRetry: true });
|
||||
else setIsLoadingTree(false);
|
||||
return () => {
|
||||
clearRetryTimer();
|
||||
};
|
||||
}, [clearRetryTimer, loadTree, effectiveResetKey]);
|
||||
|
||||
// Fire the initial load on the waiting → ready transition. `loadState` is
|
||||
// read via a ref so a failed load (which sets state back to "waiting") does
|
||||
// not re-fire this effect and cancel the retry timer via `resetRetry: true`.
|
||||
// "loaded" only counts when a tree is actually present — `applyFileChanges`
|
||||
// can flip state to "loaded" while the tree is still null.
|
||||
useEffect(() => {
|
||||
if (!agentctlStatus.isReady || loadState === "loaded") return;
|
||||
if (!agentctlStatus.isReady) return;
|
||||
if (loadStateRef.current === "loading") return;
|
||||
if (loadStateRef.current === "loaded" && treeRef.current) return;
|
||||
void loadTree({ resetRetry: true });
|
||||
}, [agentctlStatus.isReady, loadState, loadTree]);
|
||||
}, [agentctlStatus.isReady, loadTree]);
|
||||
|
||||
useEffect(() => {
|
||||
if (!tree || isLoadingTree || hasInitializedExpandedRef.current === effectiveResetKey) return;
|
||||
@@ -353,22 +400,7 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) {
|
||||
}
|
||||
}, [expandedPaths, effectiveResetKey]);
|
||||
|
||||
useEffect(() => {
|
||||
const client = getWebSocketClient();
|
||||
if (!client) return;
|
||||
return client.on("session.workspace.file.changes", (msg) => {
|
||||
const changes = msg.payload?.changes;
|
||||
if (!changes || changes.length === 0) return;
|
||||
applyFileChanges({
|
||||
client,
|
||||
sessionId: sessionIdRef.current,
|
||||
expandedPaths,
|
||||
changes,
|
||||
setTree,
|
||||
setLoadState,
|
||||
});
|
||||
});
|
||||
}, [expandedPaths]);
|
||||
useFileChangeSubscription({ sessionIdRef, expandedPaths, setTree, setLoadState });
|
||||
|
||||
const collapseAll = useCallback(() => {
|
||||
setExpandedPaths(new Set());
|
||||
|
||||
@@ -35,7 +35,10 @@ export function renderSessionOrLoadState({
|
||||
}
|
||||
if (loadState === "waiting") {
|
||||
return (
|
||||
<div className="p-4 text-sm text-muted-foreground flex items-center gap-2">
|
||||
<div
|
||||
data-testid="file-tree-waiting"
|
||||
className="p-4 text-sm text-muted-foreground flex items-center gap-2"
|
||||
>
|
||||
<IconLoader2 className="h-3.5 w-3.5 animate-spin" />
|
||||
Preparing workspace...
|
||||
</div>
|
||||
@@ -43,7 +46,7 @@ export function renderSessionOrLoadState({
|
||||
}
|
||||
if (loadState === "manual") {
|
||||
return (
|
||||
<div className="p-4 text-sm text-muted-foreground space-y-2">
|
||||
<div data-testid="file-tree-manual" className="p-4 text-sm text-muted-foreground space-y-2">
|
||||
<div>{loadError ?? "Workspace is still starting."}</div>
|
||||
<button
|
||||
type="button"
|
||||
|
||||
@@ -79,9 +79,12 @@ export function PassthroughTerminal(props: PassthroughTerminalProps) {
|
||||
const sessionId = propSessionId ?? storeSessionId;
|
||||
|
||||
const { session, isActive } = useSession(sessionId);
|
||||
useSessionAgentctl(sessionId);
|
||||
const agentctlStatus = useSessionAgentctl(sessionId);
|
||||
const taskId = session?.task_id ?? null;
|
||||
const canConnect = Boolean(sessionId && isActive);
|
||||
// Gate WS connection on agentctl readiness. During a long prepare script
|
||||
// the backend terminal endpoint isn't accepting connections yet, and
|
||||
// spamming reconnects burns cycles and confuses the loading state.
|
||||
const canConnect = Boolean(sessionId && isActive && agentctlStatus.isReady);
|
||||
const wsBaseUrl = useWsBaseUrl();
|
||||
|
||||
const [isTerminalReady, setIsTerminalReady] = useState(false);
|
||||
|
||||
@@ -181,8 +181,44 @@ export const backendFixture = base.extend<object, { backend: BackendContext }>({
|
||||
const agentctlPortBase = 30001 + workerInfo.workerIndex * 50;
|
||||
const agentctlPortMax = agentctlPortBase + 49;
|
||||
|
||||
// Install a `git` shim that can sleep on `fetch`/`pull` before execing
|
||||
// the real git binary. Tests that need to simulate slow network git
|
||||
// operations write a millisecond value to `${tmpDir}/git-delay-ms`; the
|
||||
// shim reads it on every invocation and sleeps the matching duration.
|
||||
// When the file is absent the shim is a transparent passthrough, so
|
||||
// other tests in the same worker are unaffected.
|
||||
const shimDir = path.join(tmpDir, "bin");
|
||||
const shimPath = path.join(shimDir, "git");
|
||||
const shimDelayFile = path.join(tmpDir, "git-delay-ms");
|
||||
const originalPath = process.env.PATH ?? "";
|
||||
fs.mkdirSync(shimDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
shimPath,
|
||||
`#!/bin/sh
|
||||
if [ -f "$KANDEV_E2E_GIT_DELAY_FILE" ] && { [ "$1" = "fetch" ] || [ "$1" = "pull" ]; }; then
|
||||
delay_ms=$(cat "$KANDEV_E2E_GIT_DELAY_FILE" 2>/dev/null)
|
||||
case "$delay_ms" in
|
||||
''|*[!0-9]*) ;;
|
||||
*)
|
||||
if [ "$delay_ms" -gt 0 ]; then
|
||||
sleep_secs=$((delay_ms / 1000))
|
||||
[ "$sleep_secs" -lt 1 ] && sleep_secs=1
|
||||
sleep "$sleep_secs"
|
||||
fi
|
||||
;;
|
||||
esac
|
||||
fi
|
||||
export PATH="$KANDEV_E2E_ORIGINAL_PATH"
|
||||
exec git "$@"
|
||||
`,
|
||||
{ mode: 0o755 },
|
||||
);
|
||||
|
||||
const backendEnv = {
|
||||
...stripGitHubTokens(process.env as Record<string, string>),
|
||||
PATH: `${shimDir}:${originalPath}`,
|
||||
KANDEV_E2E_ORIGINAL_PATH: originalPath,
|
||||
KANDEV_E2E_GIT_DELAY_FILE: shimDelayFile,
|
||||
HOME: tmpDir,
|
||||
KANDEV_DATA_DIR: dataDir,
|
||||
KANDEV_SERVER_PORT: String(backendPort),
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
/**
|
||||
* Regression: when workspace preparation takes longer than the file-tree
|
||||
* retry budget (~18s), the right-sidebar file tree and terminal must keep
|
||||
* waiting instead of giving up. We simulate a slow workspace prepare by
|
||||
* writing a delay file that the backend fixture's `git` shim reads — so
|
||||
* `git fetch` inside the worktree preparer blocks for N seconds, which
|
||||
* serialises agentctl readiness behind it (same ordering that a real slow
|
||||
* git pull on a big monorepo would produce).
|
||||
*/
|
||||
test.describe("Long prepare (slow git fetch)", () => {
|
||||
test("file tree and terminal keep waiting and recover when fetch completes", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
backend,
|
||||
}) => {
|
||||
test.setTimeout(120_000);
|
||||
|
||||
// Sleep 25s on every `git fetch`/`git pull` run by the backend. The
|
||||
// worktree fetch timeout is 90s (see worktree/manager.go) so 25s fits
|
||||
// comfortably under it. 25s is well past the file-tree retry budget
|
||||
// (~18s), which is what we want to exercise.
|
||||
const delayFile = path.join(backend.tmpDir, "git-delay-ms");
|
||||
fs.writeFileSync(delayFile, "25000");
|
||||
|
||||
try {
|
||||
// Force the worktree executor path; otherwise the task resolves with an
|
||||
// empty executor_type and runEnvironmentPreparer short-circuits, meaning
|
||||
// no fetch is ever invoked and the shim sleep never runs.
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Slow Git Fetch Task",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:simple-message",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
executor_profile_id: seedData.worktreeExecutorProfileId,
|
||||
},
|
||||
);
|
||||
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
|
||||
// While git fetch is sleeping, agentctl cannot become ready. The file
|
||||
// tree must sit in the "waiting" state (the fix: useFileBrowserTree
|
||||
// gates its initial load on agentctlStatus.isReady instead of racing
|
||||
// the 18s retry budget).
|
||||
const fileTreeWaiting = testPage.getByTestId("file-tree-waiting");
|
||||
const fileTreeManual = testPage.getByTestId("file-tree-manual");
|
||||
await expect(fileTreeWaiting).toBeVisible({ timeout: 15_000 });
|
||||
await expect(fileTreeManual).toHaveCount(0);
|
||||
|
||||
// Wait past the pre-fix retry budget (1+2+5+10 = 18s). The file tree
|
||||
// must NOT have transitioned to the "manual" (Load Files) state —
|
||||
// that's the regression.
|
||||
await testPage.waitForTimeout(19_000);
|
||||
await expect(fileTreeManual).toHaveCount(0);
|
||||
await expect(fileTreeWaiting).toBeVisible();
|
||||
|
||||
// Fetch eventually returns, worktree creation proceeds, agentctl
|
||||
// becomes ready. File tree leaves the waiting state automatically.
|
||||
await expect(fileTreeWaiting).toBeHidden({ timeout: 30_000 });
|
||||
await expect(fileTreeManual).toHaveCount(0);
|
||||
} finally {
|
||||
// Restore fast git for any subsequent tests that share this worker.
|
||||
if (fs.existsSync(delayFile)) fs.unlinkSync(delayFile);
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user