diff --git a/console/web/src/components/chat/ChatView.tsx b/console/web/src/components/chat/ChatView.tsx index 67ba83282..840484ede 100644 --- a/console/web/src/components/chat/ChatView.tsx +++ b/console/web/src/components/chat/ChatView.tsx @@ -21,6 +21,7 @@ import { isHarnessAvailable, } from '@/hooks/use-harness-status' import { useLiveAnnouncer } from '@/hooks/use-live-announcer' +import { DESKTOP_POINTER_QUERY, useMediaQuery } from '@/hooks/use-media-query' import { useWorktreeBinding } from '@/hooks/use-worktree-binding' import { useWorktreeEvents } from '@/hooks/use-worktree-events' import { expandAttachments, hasExpandableAttachments } from '@/lib/attachments' @@ -212,6 +213,9 @@ export function ChatView({ : false const harnessBlockedRef = useRef(harnessBlocked) harnessBlockedRef.current = harnessBlocked + // This view is keyed by conversation, so mounting IS opening a session: + // the caret belongs in the composer, on the devices where that is free. + const focusComposerOnOpen = useMediaQuery(DESKTOP_POINTER_QUERY) /* What the model on the other end can do with a picture, read at send time rather than closed over: the send and edit-queued callbacks are built @@ -2082,6 +2086,7 @@ export function ChatView({ isStreaming={streamingIndicator} queueWhileStreaming={!!backend.queueMessage} blocked={harnessBlocked} + autoFocus={focusComposerOnOpen && !harnessBlocked} blockedPlaceholder={ conversationsCtx ? harnessComposerPlaceholder(conversationsCtx.harnessStatus) diff --git a/console/web/src/components/chat/Composer.tsx b/console/web/src/components/chat/Composer.tsx index e7e9dd2e9..f3ce67c20 100644 --- a/console/web/src/components/chat/Composer.tsx +++ b/console/web/src/components/chat/Composer.tsx @@ -110,6 +110,12 @@ interface ComposerProps { blocked?: boolean /** Placeholder while `blocked` is true. */ blockedPlaceholder?: string + /** + * Put the caret in the editor on mount. The caller decides, because only it + * knows whether focus is welcome: on a touch device it raises the on-screen + * keyboard over the conversation, which is worse than aiming once. + */ + autoFocus?: boolean /** Initial editor content (applied once on mount). */ initialContent?: (editor: LexicalEditor) => void /** @@ -177,6 +183,7 @@ export function Composer({ queueWhileStreaming, blocked, blockedPlaceholder = 'chat unavailable…', + autoFocus, initialContent, initialText, onTextChange, @@ -426,6 +433,7 @@ export function Composer({ : 'send a message…' } disabled={inputDisabled} + autoFocus={autoFocus} initialContent={resolvedInitialContent} functionEntries={functionEntries} workingDir={workingDir} diff --git a/console/web/src/components/chat/LexicalShell.tsx b/console/web/src/components/chat/LexicalShell.tsx index 862d4c959..e621f222f 100644 --- a/console/web/src/components/chat/LexicalShell.tsx +++ b/console/web/src/components/chat/LexicalShell.tsx @@ -1,3 +1,4 @@ +import { AutoFocusPlugin } from '@lexical/react/LexicalAutoFocusPlugin' import { ClearEditorPlugin } from '@lexical/react/LexicalClearEditorPlugin' import { LexicalComposer } from '@lexical/react/LexicalComposer' import { useLexicalComposerContext } from '@lexical/react/LexicalComposerContext' @@ -18,7 +19,7 @@ import { type LexicalEditor, } from 'lexical' import { useEffect, useMemo, useRef } from 'react' -import { onComposerInsert } from '@/lib/composer-insert' +import { onComposerFocusRequest, onComposerInsert } from '@/lib/composer-insert' import type { FunctionEntry } from '@/lib/functions' import { FileMentionNode } from './lexical/FileMentionNode' import { FileMentionsPlugin } from './lexical/FileMentionsPlugin' @@ -33,6 +34,8 @@ interface LexicalShellProps { onSubmit: () => void placeholder?: string disabled?: boolean + /** Put the caret in the editor on mount. Off by default. */ + autoFocus?: boolean } const baseConfig = { @@ -196,6 +199,19 @@ function ExternalInsertPlugin() { return null } +/** + * Take the caret when a surface asks for it, e.g. a "new chat" that reused + * the untouched one already open, where nothing remounts to focus itself. + */ +function FocusOnRequestPlugin({ enabled }: { enabled: boolean }) { + const [editor] = useLexicalComposerContext() + useEffect(() => { + if (!enabled) return + return onComposerFocusRequest(() => editor.focus()) + }, [editor, enabled]) + return null +} + /** * Toggle the editor's editable state when `disabled` flips. */ @@ -227,6 +243,7 @@ export function LexicalShell({ onSubmit, placeholder = 'send a message…', disabled, + autoFocus, clearToken, initialContent, functionEntries, @@ -272,6 +289,12 @@ export function LexicalShell({ + {/* Opening a session is a request to write in it, so the first + keystroke should land in the message rather than be spent aiming. + Lexical's own plugin waits for the editable node, which a bare + focus() call on mount does not. */} + {autoFocus === true && disabled !== true ? : null} + { expect(next.systemPrompt?.strategy).toBe('enrich') }) }) + +describe('isUntouchedDraft', () => { + it('recognises the chat nobody has written in yet', () => { + expect(isUntouchedDraft(conversation({ draft: true, messages: [] }))).toBe( + true, + ) + }) + + it('refuses a draft that already carries work', () => { + expect( + isUntouchedDraft( + conversation({ + draft: true, + messages: [], + draftText: 'half a thought', + }), + ), + ).toBe(false) + expect( + isUntouchedDraft( + conversation({ + draft: true, + messages: [ + { id: 'm1', role: 'user', content: 'sent', createdAt: 1 }, + ] as Conversation['messages'], + }), + ), + ).toBe(false) + }) + + it('refuses a real session, which is never interchangeable', () => { + expect(isUntouchedDraft(conversation({ draft: false, messages: [] }))).toBe( + false, + ) + }) +}) diff --git a/console/web/src/hooks/use-conversations.ts b/console/web/src/hooks/use-conversations.ts index 88a483d10..f80d750db 100644 --- a/console/web/src/hooks/use-conversations.ts +++ b/console/web/src/hooks/use-conversations.ts @@ -31,6 +31,7 @@ import { type SystemPromptAddon, type SystemPromptState, } from '@/components/chat/system-prompt-selection' +import { requestComposerFocus } from '@/lib/composer-insert' import { getIiiClient } from '@/lib/iii-client' import { newSessionId } from '@/lib/session-id' import { @@ -105,6 +106,16 @@ function emptyConversation(defaultModel: ModelId | null): Conversation { } } +/** A chat nobody has written in yet: still local, no transcript, no draft + text. Two of these are the same chat as far as anyone can tell. */ +export function isUntouchedDraft(conversation: Conversation): boolean { + return ( + conversation.draft === true && + conversation.messages.length === 0 && + (conversation.draftText ?? '') === '' + ) +} + function isMode(v: unknown): v is Mode { return v === 'ask' || v === 'agent' } @@ -812,11 +823,22 @@ export function useConversations( ) const createNew = useCallback(() => { + // Asking for a new chat while an untouched one is already open reads as + // "nothing happened": the second empty draft is indistinguishable from + // the first, and they pile up in the list. Hand back the one in front of + // you instead, and put the caret in it. + const current = conversations.find( + (conversation) => conversation.id === activeId, + ) + if (current && isUntouchedDraft(current)) { + requestComposerFocus() + return current.id + } const next = emptyConversation(loadLastModel()) setConversations((list) => [next, ...list]) setActiveId(next.id) return next.id - }, []) + }, [conversations, activeId]) const select = useCallback((id: string) => setActiveId(id), []) diff --git a/console/web/src/hooks/use-keybindings.test.ts b/console/web/src/hooks/use-keybindings.test.ts new file mode 100644 index 000000000..7949f559f --- /dev/null +++ b/console/web/src/hooks/use-keybindings.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest' +import { allowsWhileTyping } from './use-keybindings' + +/** The guard reads one dataset entry, so a field stands in for the element. */ +function field(allow?: string): EventTarget { + return { + dataset: allow === undefined ? {} : { keybindingsAllow: allow }, + } as unknown as EventTarget +} + +describe('allowsWhileTyping', () => { + it('hands back only the actions the field names', () => { + const input = field('workspace.selectByIndex panel.split') + + expect(allowsWhileTyping(input, 'workspace.selectByIndex')).toBe(true) + expect(allowsWhileTyping(input, 'panel.split')).toBe(true) + // The workspace key is a letter, so this box still spells its own query. + expect(allowsWhileTyping(input, 'workspace.create')).toBe(false) + }) + + it('accepts a comma-separated list as readily as a spaced one', () => { + expect( + allowsWhileTyping(field('panel.split,app.settings'), 'app.settings'), + ).toBe(true) + }) + + it('keeps every key for a field that opts into nothing', () => { + expect(allowsWhileTyping(field(), 'panel.split')).toBe(false) + expect(allowsWhileTyping(field(''), 'panel.split')).toBe(false) + expect(allowsWhileTyping(null, 'panel.split')).toBe(false) + }) + + it('does not match an action whose id merely starts the same way', () => { + expect(allowsWhileTyping(field('panel.splitter'), 'panel.split')).toBe( + false, + ) + }) +}) diff --git a/console/web/src/hooks/use-keybindings.ts b/console/web/src/hooks/use-keybindings.ts index d3e6a5c3b..8c1d5230d 100644 --- a/console/web/src/hooks/use-keybindings.ts +++ b/console/web/src/hooks/use-keybindings.ts @@ -30,6 +30,25 @@ function isTyping(target: EventTarget | null): boolean { return tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT' } +/** + * A field may hand specific shortcuts back with + * `data-keybindings-allow="workspace.selectByIndex panel.split"`. A search box + * that opens focused would otherwise swallow the navigation keys for as long + * as it holds the caret: after `t` opens a workspace, its page search has the + * focus, so `\` and the workspace digits typed characters instead of moving. + * Opt in per field and per action, never wholesale, so a field keeps every + * key it actually needs to spell its own query. + */ +export function allowsWhileTyping( + target: EventTarget | null, + actionId: KeybindingActionId, +): boolean { + const element = target as HTMLElement | null + const allow = element?.dataset?.keybindingsAllow + if (!allow) return false + return allow.split(/[\s,]+/).includes(actionId) +} + export function useKeybindings(handlers: KeybindingHandlers): void { // Read through a ref so the listener binds once: handlers are rebuilt every // render by every caller that passes inline arrows. @@ -46,7 +65,12 @@ export function useKeybindings(handlers: KeybindingHandlers): void { const typing = isTyping(event.target) for (const definition of KEYBINDINGS) { if (definition.scope !== 'global') continue - if (typing && !definition.firesWhileTyping) continue + if ( + typing && + !definition.firesWhileTyping && + !allowsWhileTyping(event.target, definition.id) + ) + continue const run = handlersRef.current[definition.id] if (!run) continue if (definition.digitIndex) { diff --git a/console/web/src/hooks/use-media-query.ts b/console/web/src/hooks/use-media-query.ts index c3d7996bf..34c19a32d 100644 --- a/console/web/src/hooks/use-media-query.ts +++ b/console/web/src/hooks/use-media-query.ts @@ -1,5 +1,12 @@ import { useEffect, useState } from 'react' +/** + * A pointer you aim, on a screen with room for a keyboard that is already + * there. Width alone would call a tablet in landscape a desktop, and taking + * focus there raises the on-screen keyboard over whatever you were reading. + */ +export const DESKTOP_POINTER_QUERY = '(hover: hover) and (pointer: fine)' + export function useMediaQuery(query: string): boolean { const [matches, setMatches] = useState(() => typeof window === 'undefined' || typeof window.matchMedia !== 'function' diff --git a/console/web/src/lib/composer-insert.ts b/console/web/src/lib/composer-insert.ts index 891f25f05..2bfe4d3d4 100644 --- a/console/web/src/lib/composer-insert.ts +++ b/console/web/src/lib/composer-insert.ts @@ -19,6 +19,25 @@ export function insertIntoComposer(text: string): void { for (const listener of listeners) listener(text) } +type ComposerFocusListener = () => void + +const focusListeners = new Set() + +/** Ask the mounted composer for the caret. Unlike an insert this is not + buffered: a focus nobody is around to take is a focus nobody wanted. */ +export function requestComposerFocus(): void { + for (const listener of focusListeners) listener() +} + +export function onComposerFocusRequest( + listener: ComposerFocusListener, +): () => void { + focusListeners.add(listener) + return () => { + focusListeners.delete(listener) + } +} + export function onComposerInsert(listener: ComposerInsertListener): () => void { listeners.add(listener) if (pending.length > 0) { diff --git a/console/web/src/lib/workspace-tabs.test.ts b/console/web/src/lib/workspace-tabs.test.ts index 62bf53a75..d4e298f90 100644 --- a/console/web/src/lib/workspace-tabs.test.ts +++ b/console/web/src/lib/workspace-tabs.test.ts @@ -310,6 +310,23 @@ describe('withWorkspaceScreenOpened', () => { }) }) + it('stays put when the workspace you are on already shows that screen', () => { + const tabs: WorkspaceTab[] = [ + { id: 'first-chat', screens: [CHAT_SCREEN] }, + { id: 'chat-and-shell', screens: [CHAT_SCREEN, 'ext:shell'] }, + ] + // Opening chat from the second tab must not send you to the first one + // just because it was created earlier. + expect( + withWorkspaceScreenOpened( + tabs, + 'chat-and-shell', + CHAT_SCREEN, + () => 'new', + ), + ).toEqual({ tabs, activeTabId: 'chat-and-shell' }) + }) + it('places beside chat, then creates a safe split when the active tab is full', () => { const open = withWorkspaceScreenOpened( [{ id: 'chat', columns: 2, screens: [CHAT_SCREEN, 'traces'] }], diff --git a/console/web/src/lib/workspace-tabs.ts b/console/web/src/lib/workspace-tabs.ts index 7f60aebd2..6c408823f 100644 --- a/console/web/src/lib/workspace-tabs.ts +++ b/console/web/src/lib/workspace-tabs.ts @@ -381,10 +381,17 @@ export function withWorkspaceScreenOpened( screen: TabScreen, makeTabId: () => string = newTabId, ): OpenWorkspaceScreenResult { + const active = tabs.find((tab) => tab.id === activeTabId) ?? tabs[0] + // Where you already are beats where it happens to be mounted: opening chat + // from a workspace that shows chat should stay put, not send you to + // whichever other tab was created first. + if (active?.screens.includes(screen)) { + return { tabs, activeTabId: active.id } + } + const existing = tabs.find((tab) => tab.screens.includes(screen)) if (existing) return { tabs, activeTabId: existing.id } - const active = tabs.find((tab) => tab.id === activeTabId) ?? tabs[0] if (active) { const placed = withScreenOpenedBeside(active, screen) if (placed) { diff --git a/shell/ui/src/page/ReviewPane.tsx b/shell/ui/src/page/ReviewPane.tsx index eca0fb139..2525b7212 100644 --- a/shell/ui/src/page/ReviewPane.tsx +++ b/shell/ui/src/page/ReviewPane.tsx @@ -20,7 +20,7 @@ import { type ReadFileResponse, } from './coder' import { diffLines, diffTotals } from './diff' -import { gitReadSource, gitShowHead } from './git' +import { gitHeadBaseline, gitReadSource, gitShowHead } from './git' import type { ReviewEntry } from './review' export interface ReviewOptions { @@ -72,13 +72,7 @@ type FileState = | { phase: 'idle' } | { phase: 'loading' } | { phase: 'error'; message: string } - | { - phase: 'ready' - oldContents: string - newContents: string - worktreeRevision?: string - mode?: number | null - } + | ({ phase: 'ready' } & ReviewContents) interface FileLoadDescriptor { host: Host @@ -338,16 +332,61 @@ export function exactCoderText(out: ReadFileResponse, path: string): string { return out.content ?? '' } -async function loadReviewContents( - host: Host, +/** Split a root-relative path into the directory to run Git in and the name + to ask it about. Running in the file's own directory is what lets Git + discover a repository nested under a non-repository workspace root. */ +export function gitLookupTarget( root: string, - entry: ReviewEntry, -): Promise<{ + path: string, +): { cwd: string; name: string } { + const slash = path.lastIndexOf('/') + return slash === -1 + ? { cwd: root, name: path } + : { cwd: joinPath(root, path.slice(0, slash)), name: path.slice(slash + 1) } +} + +export interface ReviewContents { oldContents: string newContents: string worktreeRevision?: string mode?: number | null -}> { + /** Set when the old side is the last commit rather than this turn's + pre-turn snapshot. */ + baselineSource?: 'committed' +} + +/** Last-resort baseline for a row whose pre-turn body was never captured: + the file's committed body. It is a weaker claim than the snapshot — any + edit already uncommitted before the turn is attributed to this turn — so + the caller labels the diff instead of presenting it as a turn diff. */ +async function loadCommittedFallback( + host: Host, + root: string, + entry: ReviewEntry, +): Promise { + const { change } = entry + const oldPath = change.from ?? change.path + const target = gitLookupTarget(root, oldPath) + const oldContents = await gitHeadBaseline(host, target.cwd, target.name) + if (oldContents === null) return null + if (change.status === 'deleted') { + return { oldContents, newContents: '', baselineSource: 'committed' } + } + const out = await coderReadFile(host, joinPath(root, change.path)) + return { + oldContents, + newContents: exactCoderText(out, change.path), + worktreeRevision: out.revision ?? undefined, + mode: out.mode, + baselineSource: 'committed', + } +} + +export async function loadReviewContents( + host: Host, + root: string, + entry: ReviewEntry, +): Promise { if (entry.before !== undefined && entry.after !== undefined) { const oldSide = gitReadSource(host, root, entry.before) if (entry.after.kind === 'worktree') { @@ -369,7 +408,11 @@ async function loadReviewContents( return { oldContents, newContents } } if (entry.baseline === null) { - throw new Error('earlier content was not captured for this turn') + const committed = await loadCommittedFallback(host, root, entry) + if (committed !== null) return committed + throw new Error( + 'earlier content was not captured for this turn, and this file has no committed version to compare against', + ) } const { change } = entry const oldPath = change.from ?? change.path @@ -1114,6 +1157,15 @@ function ReviewFile({ {editStatus.message} ) : null} + {!collapsed && + renderBody && + state.phase === 'ready' && + state.baselineSource === 'committed' ? ( +
+ compared against the last commit — this turn's earlier content was + not captured, so edits made before it are included +
+ ) : null} {collapsed ? null : !renderBody ? (
scroll to load diff…
) : state.phase === 'idle' || state.phase === 'loading' ? ( diff --git a/shell/ui/src/page/__tests__/ReviewPane.test.ts b/shell/ui/src/page/__tests__/ReviewPane.test.ts index ea4ff9ae9..8762421ba 100644 --- a/shell/ui/src/page/__tests__/ReviewPane.test.ts +++ b/shell/ui/src/page/__tests__/ReviewPane.test.ts @@ -36,6 +36,8 @@ import { defaultCollapsedReviewPaths, desiredReviewEntries, exactCoderText, + gitLookupTarget, + loadReviewContents, expandedReviewPaths, LARGE_REVIEW_EAGER_FILE_COUNT, LARGE_REVIEW_THRESHOLD, @@ -410,3 +412,111 @@ describe('orderedReviewSummaries', () => { ]) }) }) + +describe('gitLookupTarget', () => { + it('runs Git in the file own directory so a nested repository answers', () => { + expect(gitLookupTarget('/root', 'nested/repo/src/app.ts')).toEqual({ + cwd: '/root/nested/repo/src', + name: 'app.ts', + }) + expect(gitLookupTarget('/root', 'top.ts')).toEqual({ + cwd: '/root', + name: 'top.ts', + }) + }) +}) + +function execHost(replies: Record) { + const trigger = vi.fn(async (functionId: string, input: unknown) => { + const reply = replies[functionId] + if (reply === undefined) throw new Error(`unexpected function ${functionId}`) + return typeof reply === 'function' + ? (reply as (value: unknown) => unknown)(input) + : reply + }) + return { + host: { iii: { trigger } } as unknown as Parameters[0], + trigger, + } +} + +describe('loadReviewContents without a captured baseline', () => { + const uncaptured: ReviewEntry = { + path: 'nested/repo/src/app.ts', + change: { path: 'nested/repo/src/app.ts', status: 'modified', staged: false }, + baseline: null, + } + + it('falls back to the committed body and labels it', async () => { + const { host, trigger } = execHost({ + 'shell::exec': { + exit_code: 0, + stdout: 'committed\n', + stderr: '', + timed_out: false, + stdout_truncated: false, + stderr_truncated: false, + }, + 'coder::read-file': { + content: 'current\n', + is_utf8: true, + more_lines: false, + revision: 'r1', + mode: 420, + }, + }) + + await expect(loadReviewContents(host, '/root', uncaptured)).resolves.toEqual({ + oldContents: 'committed\n', + newContents: 'current\n', + worktreeRevision: 'r1', + mode: 420, + baselineSource: 'committed', + }) + expect(trigger).toHaveBeenCalledWith( + 'shell::exec', + expect.objectContaining({ cwd: '/root/nested/repo/src' }), + ) + }) + + it('keeps failing closed when there is no committed body either', async () => { + const { host } = execHost({ + 'shell::exec': { + exit_code: 128, + stdout: '', + stderr: 'fatal: not a git repository', + timed_out: false, + stdout_truncated: false, + stderr_truncated: false, + }, + }) + + await expect(loadReviewContents(host, '/root', uncaptured)).rejects.toThrow( + 'earlier content was not captured for this turn', + ) + }) + + it('compares a deleted file against its committed body', async () => { + const { host } = execHost({ + 'shell::exec': { + exit_code: 0, + stdout: 'committed\n', + stderr: '', + timed_out: false, + stdout_truncated: false, + stderr_truncated: false, + }, + }) + + await expect( + loadReviewContents(host, '/root', { + ...uncaptured, + change: { ...uncaptured.change, status: 'deleted' }, + }), + ).resolves.toEqual({ + oldContents: 'committed\n', + newContents: '', + baselineSource: 'committed', + }) + }) +}) diff --git a/shell/ui/src/page/__tests__/baseline.test.ts b/shell/ui/src/page/__tests__/baseline.test.ts index e1e8e1260..231391ece 100644 --- a/shell/ui/src/page/__tests__/baseline.test.ts +++ b/shell/ui/src/page/__tests__/baseline.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import { classifyWorkspaceBaselinePath, captureWorkspaceBaseline, + prioritizedBaselineCandidates, } from '../baseline' import type { TreeNode } from '../coder' import { normalizeLiveReviewEvent } from '../live-review' @@ -33,16 +34,17 @@ function hostFor(root: TreeNode) { const trigger = vi.fn(async (functionId: string, _input: unknown) => { if (functionId === 'coder::tree') return { path: '/repo', root } if (functionId === 'coder::read-file') { + // One result per requested path, so a test can count what the capture + // actually asked for rather than a fixture's single canned row. + const paths = (_input as { paths?: string[] }).paths ?? [] return { - results: [ - { - path: '/repo/visible.ts', - success: true, - content: 'before\n', - is_utf8: true, - more_lines: false, - }, - ], + results: paths.map((path) => ({ + path, + success: true, + content: 'before\n', + is_utf8: true, + more_lines: false, + })), } } throw new Error(`unexpected function ${functionId}`) @@ -307,3 +309,71 @@ describe('captureWorkspaceBaseline', () => { }) }) }) + +describe('prioritizedBaselineCandidates', () => { + function aged(name: string, mtime: number): TreeNode { + return { name, kind: 'file', size: 7, mtime } + } + + it('spends the body budget on the most recently modified files first', () => { + const root = workspace([ + aged('stale.ts', 10), + { + name: 'src', + kind: 'dir', + size: 0, + mtime: 5, + children: [aged('fresh.ts', 99), aged('older.ts', 20)], + }, + ]) + + expect(prioritizedBaselineCandidates(root, () => true)).toEqual([ + 'src/fresh.ts', + 'src/older.ts', + 'stale.ts', + ]) + }) + + it('keeps tree order for equal mtimes and honours the review predicate', () => { + const root = workspace([aged('a.ts', 7), aged('b.ts', 7), aged('skip.log', 90)]) + + expect( + prioritizedBaselineCandidates(root, (path) => !path.endsWith('.log')), + ).toEqual(['a.ts', 'b.ts']) + }) +}) + +describe('baseline coverage', () => { + it('reports a capped body snapshot without disturbing inventory completeness', async () => { + const children = Array.from({ length: 501 }, (_, index) => + file(`file-${index}.ts`), + ) + const baseline = await captureWorkspaceBaseline( + hostFor(workspace(children)).host, + '/repo', + () => true, + ) + + expect(baseline.coverage).toEqual({ + candidates: 501, + captured: 500, + capped: true, + }) + expect(baseline.contents.size).toBe(500) + expect(baseline.complete).toBe(true) + }) + + it('reports full coverage when every candidate fits', async () => { + const baseline = await captureWorkspaceBaseline( + hostFor(workspace([file('visible.ts')])).host, + '/repo', + () => true, + ) + + expect(baseline.coverage).toEqual({ + candidates: 1, + captured: 1, + capped: false, + }) + }) +}) diff --git a/shell/ui/src/page/__tests__/git.test.ts b/shell/ui/src/page/__tests__/git.test.ts index 767faf0bb..c90bb363b 100644 --- a/shell/ui/src/page/__tests__/git.test.ts +++ b/shell/ui/src/page/__tests__/git.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import { gitBranchComparison, gitChanges, + gitHeadBaseline, gitCommitComparison, gitComparison, gitReadSource, @@ -881,3 +882,45 @@ describe('git metadata', () => { }) }) }) + +describe('gitHeadBaseline', () => { + it("reads the committed body from the file's own directory", async () => { + const { host, trigger } = mockedHost(reply({ stdout: 'committed\n' })) + + await expect( + gitHeadBaseline(host, '/root/nested/src', 'app.ts'), + ).resolves.toBe('committed\n') + expect(trigger).toHaveBeenCalledWith('shell::exec', { + command: 'git', + args: ['show', 'HEAD:./app.ts'], + cwd: '/root/nested/src', + timeout_ms: 15_000, + }) + }) + + it('reports absence instead of an empty body', async () => { + const untracked = mockedHost( + reply({ exit_code: 128, stderr: "fatal: path 'app.ts' does not exist" }), + ) + await expect( + gitHeadBaseline(untracked.host, '/root', 'app.ts'), + ).resolves.toBeNull() + + const truncated = mockedHost( + reply({ stdout: 'partial', stdout_truncated: true }), + ) + await expect( + gitHeadBaseline(truncated.host, '/root', 'app.ts'), + ).resolves.toBeNull() + + const binary = mockedHost(reply({ stdout: 'PNG\0data' })) + await expect( + gitHeadBaseline(binary.host, '/root', 'logo.png'), + ).resolves.toBeNull() + + const failed = mockedHost(new Error('exec unavailable')) + await expect( + gitHeadBaseline(failed.host, '/root', 'app.ts'), + ).resolves.toBeNull() + }) +}) diff --git a/shell/ui/src/page/__tests__/live-review.test.ts b/shell/ui/src/page/__tests__/live-review.test.ts index 595aedbd7..cf4c19518 100644 --- a/shell/ui/src/page/__tests__/live-review.test.ts +++ b/shell/ui/src/page/__tests__/live-review.test.ts @@ -118,4 +118,56 @@ describe('normalizeLiveReviewEvent', () => { baseline: 'original\n', }) }) + + it('believes a witnessed creation over an inventory that only guessed', () => { + // A truncated inventory answers "file" for every path it never listed, so + // a file the turn created would otherwise read as modified with nothing + // to compare against. + expect( + normalizeLiveReviewEvent({ + path: 'ReachAI/Dockerfile', + rawKind: 'created', + priorKind: 'file', + priorKindExact: false, + existsNow: true, + }), + ).toEqual({ + action: 'created', + path: 'ReachAI/Dockerfile', + baseline: '', + }) + }) + + it('keeps a proven existing file a modification, however it was written', () => { + expect( + normalizeLiveReviewEvent({ + path: 'src/atomic.ts', + rawKind: 'created', + priorKind: 'file', + priorKindExact: true, + existsNow: true, + }), + ).toEqual({ + action: 'modified', + path: 'src/atomic.ts', + baseline: undefined, + }) + }) + + it('keeps a captured body even when the classification was a guess', () => { + expect( + normalizeLiveReviewEvent({ + path: 'src/guessed.ts', + rawKind: 'created', + priorKind: 'file', + priorKindExact: false, + priorBaseline: 'before\n', + existsNow: true, + }), + ).toEqual({ + action: 'modified', + path: 'src/guessed.ts', + baseline: 'before\n', + }) + }) }) diff --git a/shell/ui/src/page/baseline.ts b/shell/ui/src/page/baseline.ts index 7d774e487..4464af909 100644 --- a/shell/ui/src/page/baseline.ts +++ b/shell/ui/src/page/baseline.ts @@ -18,6 +18,19 @@ export interface WorkspaceBaseline { contents: ReadonlyMap /** False when coder::tree may have omitted reviewable descendants. */ complete: boolean + /** How much of the reviewable inventory the body snapshot could hold. A + capped snapshot still classifies every path — only bodies are missing — + so this stays separate from `complete`, which drives new-vs-existing. */ + coverage: WorkspaceBaselineCoverage +} + +export interface WorkspaceBaselineCoverage { + /** Reviewable files the inventory offered. */ + candidates: number + /** Files whose body the snapshot actually holds. */ + captured: number + /** True when the candidate count exceeded the per-turn body budget. */ + capped: boolean } export interface WorkspaceBaselinePathState { @@ -76,6 +89,34 @@ export function classifyWorkspaceBaselinePath( : { priorKind: 'file', exact: false } } +/** Reviewable files in the order the body budget should spend itself: most + recently modified first. A turn edits the working set, not the + alphabetically first 500 paths, so recency buys far more coverage than + tree order on a large or shared root. Equal mtimes keep tree order. */ +export function prioritizedBaselineCandidates( + root: TreeNode, + includePath: (path: string) => boolean, +): string[] { + const candidates: { path: string; mtime: number; order: number }[] = [] + const walk = (node: TreeNode, prefix: string) => { + for (const child of node.children ?? []) { + const path = prefix === '' ? child.name : `${prefix}/${child.name}` + if (child.kind === 'file' && includePath(path)) { + candidates.push({ path, mtime: child.mtime, order: candidates.length }) + } + walk(child, path) + } + } + walk(root, '') + return candidates + .sort((left, right) => + left.mtime === right.mtime + ? left.order - right.order + : right.mtime - left.mtime, + ) + .map((candidate) => candidate.path) +} + /** * Capture a turn baseline at Harness's awaited pre-turn boundary. The result * is built locally and published atomically, so tree refreshes cannot cancel a @@ -90,10 +131,8 @@ export async function captureWorkspaceBaseline( ): Promise { const treeResponse = await baselineTree(host, root) const tree = flattenTree(treeResponse.root) - const relPaths = [...tree.kinds] - .filter(([path, kind]) => kind === 'file' && includePath(path)) - .map(([path]) => path) - .slice(0, SNAPSHOT_MAX_FILES) + const candidates = prioritizedBaselineCandidates(treeResponse.root, includePath) + const relPaths = candidates.slice(0, SNAPSHOT_MAX_FILES) const contents = new Map() for (let start = 0; start < relPaths.length; start += SNAPSHOT_BATCH_SIZE) { @@ -115,5 +154,10 @@ export async function captureWorkspaceBaseline( // rejects that subtree. Capacity, depth, I/O, and reviewable default // excludes remain fail-closed. complete: inventoryCompleteForReview(treeResponse.root, includePath), + coverage: { + candidates: candidates.length, + captured: contents.size, + capped: candidates.length > SNAPSHOT_MAX_FILES, + }, } } diff --git a/shell/ui/src/page/git.ts b/shell/ui/src/page/git.ts index f81c6b3dd..d57ec6162 100644 --- a/shell/ui/src/page/git.ts +++ b/shell/ui/src/page/git.ts @@ -1125,11 +1125,32 @@ export async function nestedGitStatus( return status === 'renamed' ? 'modified' : status } +/** The committed body of one path, resolved from the directory it lives in so + a repository nested under a non-repository root still answers. Null means + there is no usable committed body — no repository, path untracked, binary, + truncated, or a failed exec — never an empty string, which a caller would + read as a real empty file. */ +export async function gitHeadBaseline( + host: Host, + cwd: string, + path: string, +): Promise { + try { + const out = await git(host, cwd, ['show', `HEAD:./${path}`]) + if (execFailure(out, 'git show HEAD') !== null) return null + if (out.stdout.includes('\0') || out.stdout.includes('�')) return null + return out.stdout + } catch { + return null + } +} + +/** Committed body or empty, for the callers that already treat a missing + HEAD side as an addition. */ export async function gitShowHead( host: Host, root: string, path: string, ): Promise { - const out = await git(host, root, ['show', `HEAD:./${path}`]) - return out.exit_code === 0 ? out.stdout : '' + return (await gitHeadBaseline(host, root, path)) ?? '' } diff --git a/shell/ui/src/page/index.tsx b/shell/ui/src/page/index.tsx index 5c141b8ee..662833c6a 100644 --- a/shell/ui/src/page/index.tsx +++ b/shell/ui/src/page/index.tsx @@ -50,6 +50,7 @@ import { errorMessage } from '../lib/format' import { captureWorkspaceBaseline, classifyWorkspaceBaselinePath, + type WorkspaceBaselineCoverage, } from './baseline' import { ChangeDiffPane } from './ChangeDiffPane' import { @@ -444,6 +445,9 @@ export function ShellExplorerPage({ const baselineCompleteRef = useRef(false) const baselineCapturedRef = useRef(false) const baselineReadyRef = useRef>(Promise.resolve()) + // A capped snapshot degrades quietly per row, so the toolbar says so once. + const [baselineCoverage, setBaselineCoverage] = + useState(null) const preparedTurnRef = useRef(null) const lastReviewKeyRef = useRef(observedReviewKey ?? null) const reviewEpochRef = useRef(0) @@ -610,6 +614,7 @@ export function ShellExplorerPage({ baselineCompleteRef.current = false baselineCapturedRef.current = false baselineReadyRef.current = Promise.resolve() + setBaselineCoverage(null) reviewEntriesRef.current = new Map() setReviewEntries(new Map()) scopeEntriesRef.current = new Map() @@ -650,7 +655,7 @@ export function ShellExplorerPage({ baselineCompleteRef.current = false baselineCapturedRef.current = false const snapshot = captureWorkspaceBaseline(host, currentRoot, reviewablePath) - .then(({ contents, kinds, complete }) => { + .then(({ contents, kinds, complete, coverage }) => { if ( rootGenerationRef.current !== generation || reviewEpochRef.current !== epoch || @@ -662,6 +667,7 @@ export function ShellExplorerPage({ baselineKindsRef.current = kinds baselineCompleteRef.current = complete baselineCapturedRef.current = true + setBaselineCoverage(coverage) }) .catch(() => { // Git can still provide HEAD; non-Git rows fail closed with a clear @@ -1506,6 +1512,7 @@ export function ShellExplorerPage({ path: rel, rawKind, priorKind, + priorKindExact: baselinePath?.exact, priorBaseline: baseline, existsNow: results === null @@ -1799,6 +1806,7 @@ export function ShellExplorerPage({ baselineCompleteRef.current = false baselineCapturedRef.current = false baselineReadyRef.current = Promise.resolve() + setBaselineCoverage(null) preparedTurnRef.current = null reviewEntriesRef.current = new Map() reviewEditBackupsRef.current.clear() @@ -2546,6 +2554,15 @@ export function ShellExplorerPage({ unavailable ) : null} + {reviewScope.kind === 'last-turn' && baselineCoverage?.capped ? ( + + snapshot {baselineCoverage.captured}/ + {baselineCoverage.candidates} + + ) : null} {reviewTotals.ready > 0 ? ( <> diff --git a/shell/ui/src/page/live-review.ts b/shell/ui/src/page/live-review.ts index 60e054a81..d3f059863 100644 --- a/shell/ui/src/page/live-review.ts +++ b/shell/ui/src/page/live-review.ts @@ -5,6 +5,13 @@ export interface LiveReviewEventInput { rawKind: string /** `null` means known missing; `undefined` means no tree snapshot. */ priorKind: PriorFilesystemKind + /** + * Whether the inventory PROVED `priorKind` rather than inferring it. A + * truncated inventory cannot tell an omitted file from a new one, so it + * guesses `file`; that guess must not outrank a creation the watcher + * actually saw during this turn. + */ + priorKindExact?: boolean /** Undefined means uncaptured. An empty string is a real baseline. */ priorBaseline?: string /** Whether the changed path is a readable file after the event burst. */ @@ -26,6 +33,16 @@ export function normalizeLiveReviewEvent(input: LiveReviewEventInput): LiveRevie return { action: 'ignore-directory', path: input.path } } + // A guessed "it existed" loses to a witnessed creation with no captured + // body: the watcher saw this path appear, the inventory never listed it. + const guessedExisting = + input.priorKind === 'file' && + input.priorKindExact === false && + input.priorBaseline === undefined + if (guessedExisting && input.rawKind === 'created' && input.existsNow) { + return { action: 'created', path: input.path, baseline: '' } + } + const existedBefore = input.priorKind === 'file' || (input.priorKind === undefined && input.priorBaseline !== undefined)