fix(workspace): scroll/diff-race/recovery/editor/mobile polish

2026-07-11 17:59:25 -04:00
parent b00141130c
commit f89dfce3b8
6 changed files with 477 additions and 18 deletions
@@ -2,6 +2,7 @@

 import { useEffect, useMemo, useRef, useState } from 'react';
 import {
+  ArrowDown,
  Ban,
  FilePenLine,
  MessagesSquare,
@@ -16,6 +17,7 @@ import { Badge, Button, Textarea } from '@spoon/ui';

 import { DiffFileView, useDiffTheme } from './diff-file-view';
 import { parseDiffFileForPath } from './diff-utils';
+import { isNearBottom } from './workspace-helpers';

 type ActivityFilter = 'all' | 'chat' | 'activity' | 'files' | 'errors';

@@ -70,6 +72,7 @@ export const AgentThread = ({
  const [filter, setFilter] = useState<ActivityFilter>('all');
  const diffTheme = useDiffTheme();
  const scrollRef = useRef<HTMLDivElement>(null);
+  const [showJumpToLatest, setShowJumpToLatest] = useState(false);
  const chatMessages = useMemo(
    () =>
      messages.filter((message) => {
@@ -111,20 +114,35 @@ export const AgentThread = ({
      ? []
      : workspaceChanges;

+  const scrollToLatest = () => {
+    const node = scrollRef.current;
+    if (!node) return;
+    if (typeof node.scrollTo === 'function') {
+      node.scrollTo({ top: node.scrollHeight, behavior: 'smooth' });
+    } else {
+      node.scrollTop = node.scrollHeight;
+    }
+    setShowJumpToLatest(false);
+  };
+
+  // Only auto-scroll to the newest content when the user is already near the
+  // bottom. If they have scrolled up to read, don't yank them — surface a
+  // "jump to latest" affordance instead. (The agent streaming no longer forces
+  // a scroll; that was fighting the reader.)
  useEffect(() => {
    const node = scrollRef.current;
    if (!node) return;
-    const distanceFromBottom =
-      node.scrollHeight - node.scrollTop - node.clientHeight;
-    if (distanceFromBottom < 160 || agentTurnActive) {
+    if (isNearBottom(node)) {
      if (typeof node.scrollTo === 'function') {
        node.scrollTo({ top: node.scrollHeight, behavior: 'smooth' });
      } else {
        node.scrollTop = node.scrollHeight;
      }
+      setShowJumpToLatest(false);
+    } else {
+      setShowJumpToLatest(true);
    }
  }, [
-    agentTurnActive,
    events.length,
    interactions.length,
    messages.length,
@@ -227,8 +245,13 @@ export const AgentThread = ({
          </Button>
        ))}
      </div>
+      <div className='relative flex min-h-0 flex-1 flex-col'>
      <div
        ref={scrollRef}
+        onScroll={() => {
+          const node = scrollRef.current;
+          if (node && isNearBottom(node)) setShowJumpToLatest(false);
+        }}
        className='min-h-0 flex-1 space-y-3 overflow-y-auto overscroll-contain p-3'
      >
        {(filter === 'all' || filter === 'chat') && interactions.length > 0
@@ -430,6 +453,19 @@ export const AgentThread = ({
            yet.
          </p>
        ) : null}
+      </div>
+        {showJumpToLatest ? (
+          <Button
+            type='button'
+            size='sm'
+            variant='secondary'
+            className='absolute bottom-3 left-1/2 z-10 -translate-x-1/2 shadow-md'
+            onClick={scrollToLatest}
+          >
+            <ArrowDown className='size-3' />
+            Jump to latest
+          </Button>
+        ) : null}
      </div>
      <div className='border-border flex-none space-y-2 border-t p-3'>
        <Textarea
@@ -1,13 +1,14 @@
 'use client';

 import type { CSSProperties, PointerEvent as ReactPointerEvent } from 'react';
-import { useCallback, useEffect, useState } from 'react';
+import { useCallback, useEffect, useRef, useState } from 'react';
 import { useMutation, useQuery } from 'convex/react';
 import {
  FileCode,
  GitCompare,
  Loader2,
  MessagesSquare,
+  PanelLeft,
  SquareTerminal,
 } from 'lucide-react';
 import { toast } from 'sonner';
@@ -26,6 +27,10 @@ import {
  AlertDialogTrigger,
  Button,
  cn,
+  Sheet,
+  SheetContent,
+  SheetHeader,
+  SheetTitle,
  Tabs,
  TabsContent,
  TabsList,
@@ -42,6 +47,7 @@ import { FileTree } from './file-tree';
 import { JobStatusBar } from './job-status-bar';
 import { WorkspaceActions } from './workspace-actions';
 import { WorkspaceTerminal } from './workspace-terminal';
+import { createRequestSequencer, shouldShowRecovery } from './workspace-helpers';

 type WorkspaceTab = 'editor' | 'diff' | 'thread' | 'terminal';

@@ -52,6 +58,9 @@ type OpenFileState = {
  loading: boolean;
  saving: boolean;
  error?: string;
+  // Set when the agent edited this file on disk while the user had unsaved
+  // local edits, so the UI can warn instead of silently stomping either side.
+  conflicted?: boolean;
 };

 type PendingOverwrite = {
@@ -105,6 +114,39 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
    useState<WorkspaceTab>('editor');
  const [pendingOverwrite, setPendingOverwrite] = useState<PendingOverwrite>();
  const [pendingClosePath, setPendingClosePath] = useState<string>();
+  const [fileTreeOpen, setFileTreeOpen] = useState(false);
+
+  // Per-loader sequence guards: only the latest response for each loader is
+  // applied, so a slower earlier fetch can't overwrite newer tree/diff data.
+  const treeSequencerRef = useRef<
+    ReturnType<typeof createRequestSequencer> | undefined
+  >(undefined);
+  treeSequencerRef.current ??= createRequestSequencer();
+  const treeSequencer = treeSequencerRef.current;
+  const diffSequencerRef = useRef<
+    ReturnType<typeof createRequestSequencer> | undefined
+  >(undefined);
+  diffSequencerRef.current ??= createRequestSequencer();
+  const diffSequencer = diffSequencerRef.current;
+
+  // Count consecutive workspace status/health failures. A single transient
+  // 'workspace is not active' blip (worker restart) must not flash the big red
+  // recovery panel — only surface it after RECOVERY_THRESHOLD in a row.
+  const consecutiveFailuresRef = useRef(0);
+  const processedChangeSignatureRef = useRef(0);
+  const conflictToastPathsRef = useRef<Set<string>>(new Set());
+
+  const recordWorkspaceSuccess = useCallback(() => {
+    consecutiveFailuresRef.current = 0;
+    setWorkspaceError(undefined);
+  }, []);
+
+  const recordWorkspaceFailure = useCallback((message: string) => {
+    consecutiveFailuresRef.current += 1;
+    if (shouldShowRecovery(consecutiveFailuresRef.current)) {
+      setWorkspaceError(message);
+    }
+  }, []);

  const workspaceDisabled =
    !job ||
@@ -133,20 +175,24 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
    );

  const loadTree = useCallback(async () => {
+    const token = treeSequencer.next();
    const response = await fetch(`/api/agent-jobs/${jobId}/tree`);
    if (!response.ok) throw new Error(await response.text());
    const data = (await response.json()) as { tree: FileTreeNode | null };
-    setWorkspaceError(undefined);
+    if (!treeSequencer.isLatest(token)) return;
+    recordWorkspaceSuccess();
    setTree(data.tree);
-  }, [jobId]);
+  }, [jobId, recordWorkspaceSuccess, treeSequencer]);

  const loadDiff = useCallback(async () => {
+    const token = diffSequencer.next();
    const response = await fetch(`/api/agent-jobs/${jobId}/diff`);
    if (!response.ok) throw new Error(await response.text());
    const data = (await response.json()) as DiffResponse;
-    setWorkspaceError(undefined);
+    if (!diffSequencer.isLatest(token)) return;
+    recordWorkspaceSuccess();
    setDiff(data.diff);
-  }, [jobId]);
+  }, [diffSequencer, jobId, recordWorkspaceSuccess]);

  const loadAgentStatus = useCallback(async () => {
    const response = await fetch(`/api/agent-jobs/${jobId}/agent/status`);
@@ -154,14 +200,14 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
      setAgentTurnActive(false);
      const body = await response.text();
      if (body.includes('workspace is not active')) {
-        setWorkspaceError(body);
+        recordWorkspaceFailure(body);
      }
      return;
    }
    const data = (await response.json()) as { active?: boolean };
-    setWorkspaceError(undefined);
+    recordWorkspaceSuccess();
    setAgentTurnActive(Boolean(data.active));
-  }, [jobId]);
+  }, [jobId, recordWorkspaceFailure, recordWorkspaceSuccess]);

  const loadFile = useCallback(
    async (path: string) => {
@@ -194,6 +240,40 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
    [jobId],
  );

+  // Re-fetch an already-open file and replace its editor buffer. Used when the
+  // agent edits a file that is currently open: if the local buffer is clean we
+  // adopt the new on-disk content; if the user has unsaved edits we flag a
+  // conflict instead of stomping their work.
+  const refreshFileBuffer = useCallback(
+    async (path: string) => {
+      const response = await fetch(
+        `/api/agent-jobs/${jobId}/file?path=${encodeURIComponent(path)}`,
+      );
+      if (!response.ok) return;
+      const data = (await response.json()) as FileResponse;
+      setFiles((current) => {
+        const existing = current[path];
+        if (!existing) return current;
+        if (existing.content !== existing.savedContent) {
+          return { ...current, [path]: { ...existing, conflicted: true } };
+        }
+        conflictToastPathsRef.current.delete(path);
+        return {
+          ...current,
+          [path]: {
+            path,
+            content: data.content,
+            savedContent: data.content,
+            loading: false,
+            saving: false,
+            conflicted: false,
+          },
+        };
+      });
+    },
+    [jobId],
+  );
+
  const openFile = useCallback(
    (path: string) => {
      setOpenFilePaths((current) =>
@@ -220,9 +300,10 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {

  useEffect(() => {
    if (!workspaceReady) return;
+    // Recovery is driven solely by the recurring status heartbeat
+    // (loadAgentStatus) so a one-off tree/diff hiccup can't trip the panel.
    const handleError = (error: unknown) => {
      console.error(error);
-      setWorkspaceError(error instanceof Error ? error.message : String(error));
    };
    const timeout = window.setTimeout(() => {
      void loadTree().catch(handleError);
@@ -260,6 +341,11 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
    (latest, change) => Math.max(latest, change._creationTime),
    0,
  );
+  // Read the (freshly-allocated-each-render) changes array via a ref inside the
+  // signature-gated effect below, so the effect's deps stay on the stable
+  // numeric signature instead of the array identity.
+  const workspaceChangesRef = useRef(workspaceChanges);
+  workspaceChangesRef.current = workspaceChanges;
  useEffect(() => {
    if (!workspaceReady) return;
    const timeout = window.setTimeout(() => {
@@ -275,6 +361,46 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
    loadTree,
  ]);

+  // When the agent records a workspace change that touches a currently OPEN
+  // file, refresh that file's editor buffer (clean buffers adopt the new
+  // content; dirty buffers get a conflict flag + one-time toast) so the editor
+  // doesn't keep showing pre-edit content.
+  useEffect(() => {
+    if (!workspaceReady) return;
+    if (workspaceChangeSignature <= processedChangeSignatureRef.current) return;
+    const previousSignature = processedChangeSignatureRef.current;
+    processedChangeSignatureRef.current = workspaceChangeSignature;
+    const changedPaths = new Set(
+      workspaceChangesRef.current
+        .filter((change) => change._creationTime > previousSignature)
+        .map((change) => change.path),
+    );
+    for (const path of openFilePaths) {
+      if (!changedPaths.has(path)) continue;
+      const file = files[path];
+      if (!file) continue;
+      if (file.content !== file.savedContent) {
+        setFiles((current) =>
+          current[path]
+            ? { ...current, [path]: { ...current[path], conflicted: true } }
+            : current,
+        );
+        if (!conflictToastPathsRef.current.has(path)) {
+          conflictToastPathsRef.current.add(path);
+          toast.warning(`${path} changed on disk`);
+        }
+      } else {
+        void refreshFileBuffer(path).catch(() => undefined);
+      }
+    }
+  }, [
+    files,
+    openFilePaths,
+    refreshFileBuffer,
+    workspaceChangeSignature,
+    workspaceReady,
+  ]);
+
  useEffect(() => {
    if (!uiState || hydratedUiState) return;
    const timeout = window.setTimeout(() => {
@@ -393,6 +519,7 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
      }));
      throw new Error(await response.text());
    }
+    conflictToastPathsRef.current.delete(path);
    setFiles((current) => ({
      ...current,
      [path]: {
@@ -403,6 +530,7 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
        content,
        savedContent: content,
        saving: false,
+        conflicted: false,
      },
    }));
    await loadDiff();
@@ -492,7 +620,7 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
  };

  return (
-    <main className='border-border bg-muted/20 flex h-[calc(100vh-8.5rem)] min-h-[720px] flex-col overflow-hidden rounded-md border'>
+    <main className='border-border bg-muted/20 flex h-[calc(100dvh-8.5rem)] min-h-[70dvh] flex-col overflow-hidden rounded-md border lg:min-h-[720px]'>
      <JobStatusBar job={job} />
      {workspacePending && !workspaceError ? (
        <div className='border-border bg-background border-b p-4'>
@@ -574,9 +702,40 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
          </div>
        </div>
      ) : null}
-      <div className='border-border bg-background flex items-center justify-end border-b px-4 py-2'>
-        <WorkspaceActions job={job} disabled={workspaceDisabled} />
+      <div className='border-border bg-background flex items-center justify-between gap-2 border-b px-4 py-2'>
+        <Button
+          type='button'
+          variant='outline'
+          size='sm'
+          className='lg:hidden'
+          onClick={() => setFileTreeOpen(true)}
+        >
+          <PanelLeft className='size-4' />
+          Files
+        </Button>
+        <div className='flex flex-1 items-center justify-end'>
+          <WorkspaceActions job={job} disabled={workspaceDisabled} />
+        </div>
      </div>
+      <Sheet open={fileTreeOpen} onOpenChange={setFileTreeOpen}>
+        <SheetContent side='left' className='w-[300px] p-0 lg:hidden'>
+          <SheetHeader className='border-border border-b'>
+            <SheetTitle>Files</SheetTitle>
+          </SheetHeader>
+          <div className='min-h-0 flex-1 overflow-y-auto'>
+            <FileTree
+              tree={tree}
+              selectedPath={activeFilePath}
+              expandedPaths={expandedDirectoryPaths}
+              onSelect={(path) => {
+                openFile(path);
+                setFileTreeOpen(false);
+              }}
+              onToggleDirectory={toggleDirectory}
+            />
+          </div>
+        </SheetContent>
+      </Sheet>
      <div
        className='grid min-h-0 flex-1 grid-cols-1 lg:grid-cols-[280px_minmax(0,1fr)] 2xl:grid-cols-[300px_minmax(0,1fr)_6px_var(--agent-thread-width)]'
        style={
@@ -585,7 +744,7 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
          } as CSSProperties
        }
      >
-        <aside className='border-border bg-background min-h-0 border-r'>
+        <aside className='border-border bg-background hidden min-h-0 border-r lg:block'>
          <div className='border-border border-b p-3'>
            <h2 className='text-sm font-semibold'>Files</h2>
            <p className='text-muted-foreground text-xs'>Current workspace</p>
@@ -657,6 +816,7 @@ export const AgentWorkspaceShell = ({ jobId }: { jobId: Id<'agentJobs'> }) => {
                savedContent={activeFile?.savedContent ?? ''}
                readOnly={workspaceDisabled}
                vimEnabled={vimEnabled}
+                conflicted={activeFile?.conflicted}
                onSave={saveFile}
                onVimEnabledChange={setVimEnabled}
                onChange={(content) => {
@@ -24,6 +24,7 @@ const EDITOR_FONT_FAMILY =

 type MonacoEditorInstance = {
  getModel?: () => unknown;
+  dispose?: () => void;
 };

 type VimMode = {
@@ -36,6 +37,7 @@ export const CodeEditor = ({
  savedContent,
  readOnly,
  vimEnabled,
+  conflicted,
  onSave,
  onChange,
  onVimEnabledChange,
@@ -45,6 +47,7 @@ export const CodeEditor = ({
  savedContent: string;
  readOnly: boolean;
  vimEnabled: boolean;
+  conflicted?: boolean;
  onSave: (content: string) => Promise<void>;
  onChange: (content: string) => void;
  onVimEnabledChange: (enabled: boolean) => void;
@@ -56,6 +59,18 @@ export const CodeEditor = ({
  const { resolvedTheme } = useTheme();
  const editorTheme = resolvedTheme === 'light' ? SPOON_LIGHT : SPOON_DARK;

+  // Dispose the Monaco instance and clear the ref on unmount so a remount
+  // (e.g. the Phase-1 key={jobId} swap) doesn't leave a stale editor/vim
+  // binding leaked behind the new mount.
+  useEffect(() => {
+    return () => {
+      vimRef.current?.dispose();
+      vimRef.current = null;
+      editorRef.current?.dispose?.();
+      editorRef.current = null;
+    };
+  }, []);
+
  useEffect(() => {
    const editor = editorRef.current;
    if (!editor) return;
@@ -102,7 +117,11 @@ export const CodeEditor = ({
            Editor
          </p>
          <p className='truncate font-mono text-xs'>{path}</p>
-          {dirty ? (
+          {conflicted ? (
+            <p className='text-amber-500 text-xs'>
+              This file changed on disk — your unsaved edits may conflict.
+            </p>
+          ) : dirty ? (
            <p className='text-muted-foreground text-xs'>Unsaved changes</p>
          ) : null}
        </div>
@@ -0,0 +1,54 @@
+/**
+ * Small pure helpers extracted from the agent workspace so the tricky UX
+ * decisions (out-of-order response guarding, recovery thresholding, and
+ * near-bottom auto-scroll) can be unit tested in isolation.
+ */
+
+/**
+ * A monotonically increasing request sequencer. Each loader call takes a fresh
+ * token via {@link RequestSequencer.next} before firing its fetch, then checks
+ * {@link RequestSequencer.isLatest} once the response resolves. A slower earlier
+ * request whose token is no longer the latest is dropped so it can't overwrite
+ * newer data (the diff/tree load race).
+ */
+export type RequestSequencer = {
+  next: () => number;
+  isLatest: (token: number) => boolean;
+};
+
+export const createRequestSequencer = (): RequestSequencer => {
+  let current = 0;
+  return {
+    next: () => {
+      current += 1;
+      return current;
+    },
+    isLatest: (token: number) => token === current,
+  };
+};
+
+/**
+ * Number of consecutive workspace status/health failures required before the
+ * "needs recovery" panel is shown. A single transient
+ * `workspace is not active` blip (e.g. during a worker restart) must not trip
+ * it.
+ */
+export const RECOVERY_THRESHOLD = 3;
+
+export const shouldShowRecovery = (
+  consecutiveFailures: number,
+  threshold: number = RECOVERY_THRESHOLD,
+): boolean => consecutiveFailures >= threshold;
+
+/**
+ * Distance from the bottom (px) within which the thread is considered "near the
+ * bottom" and safe to auto-scroll on new content. Outside this band the user
+ * has scrolled up to read and must not be yanked.
+ */
+export const NEAR_BOTTOM_THRESHOLD = 80;
+
+export const isNearBottom = (
+  metrics: { scrollHeight: number; scrollTop: number; clientHeight: number },
+  threshold: number = NEAR_BOTTOM_THRESHOLD,
+): boolean =>
+  metrics.scrollHeight - metrics.scrollTop - metrics.clientHeight < threshold;