From 9be451c0d524bdab7b3740ca868515bea5215578 Mon Sep 17 00:00:00 2001 From: Stephen Hellicar Date: Tue, 7 Apr 2026 00:44:49 +1000 Subject: [PATCH] Extract buildSubmitText and inline #buildStatusLine (step 5e) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildSubmitText(text, attachments) is the last non-trivial logic that lived inline in AppLayout with no tests. The ctrl+enter handler had 25 lines building [attachment #N] blocks from the editor text and attachment store. As a pure function it tests easily and its format is contractual (the system prompt tells the agent how these blocks are structured). #buildStatusLine was a one-line delegation wrapper with no logic. Inlining it (renderStatus(this.#statusState, cols)) removes indirection without losing any readability — the call site already names both the function and its inputs. AppLayout now contains only coordination glue: screen management, mode flag, five state objects, two promise holders, render/flush/debounce plumbing, handleKey routing, and the async I/O for clipboard/file attachment keys. 19 new tests in buildSubmitText.spec.ts. Total: 338 tests across 14 files (up from 319). Refactor series complete. --- .claude/sessions/2026-04-07.md | 42 ++++ CLAUDE.md | 4 +- apps/claude-sdk-cli/src/AppLayout.ts | 37 +--- apps/claude-sdk-cli/src/buildSubmitText.ts | 44 ++++ .../test/buildSubmitText.spec.ts | 198 ++++++++++++++++++ 5 files changed, 289 insertions(+), 36 deletions(-) create mode 100644 apps/claude-sdk-cli/src/buildSubmitText.ts create mode 100644 apps/claude-sdk-cli/test/buildSubmitText.spec.ts diff --git a/.claude/sessions/2026-04-07.md b/.claude/sessions/2026-04-07.md index 0de07ec..823d0d2 100644 --- a/.claude/sessions/2026-04-07.md +++ b/.claude/sessions/2026-04-07.md @@ -137,3 +137,45 @@ Continuation of the same session after context compaction. **Preview gate moved inside renderer:** In AppLayout, `buildPreviewRows` was only called when `previewMode && commandMode`. Moving that guard inside `renderCommandMode` makes the function self-contained — callers don't need to know the precondition. **`isLikelyPath` and async clipboard/stat code stay in AppLayout:** These are I/O operations, not state. They belong with `#handleCommandKey` which stays in AppLayout as part of the event dispatch layer. Step 5e will decide whether this warrants further extraction. + + +--- + +## Step 5e — ScreenCoordinator cleanup (PR #199) + +Final step in the AppLayout refactor series. + +**New `buildSubmitText.ts`** — pure function, no I/O: +- `buildSubmitText(text: string, attachments: readonly Attachment[] | null): string` +- Assembles final message string from editor text + optional attachment blocks +- Each attachment becomes an `[attachment #N]...[/attachment]` block appended after the main text +- Returns `text` unchanged when no attachments (null or empty), so callers don't special-case the zero-attachment path +- Extracted from the ctrl+enter handler in AppLayout — was ~25 inline lines with no tests + +**`AppLayout.ts` changes:** +- Inline `#buildStatusLine`: was a one-liner delegation to `renderStatus()`. Removed the method, call site now reads `renderStatus(this.#statusState, cols)` directly. +- ctrl+enter handler: 28 lines of attachment serialization → `buildSubmitText(text, attachments)` (3 lines) +- Net: AppLayout loses 34 lines; all remaining code is coordination glue (screen, event loop, state wiring) + +**Tests:** 19 new in `buildSubmitText.spec.ts`. Total: 338 (up from 319). + +## What AppLayout now contains + +- Screen management: `#screen`, enter/exit, resize listener +- Mode flag (`editor` / `streaming`) +- Five state objects: `#conversationState`, `#editorState`, `#statusState`, `#toolApprovalState`, `#commandModeState` +- Two promise holders: `#editorResolve`, `#cancelFn` +- Render debounce: `#renderPending` + `#scheduleRender()` +- `render()`: orchestrates all five renderers + screen write +- `#flushToScroll()`: exits alt buffer, writes sealed blocks, re-enters +- `handleKey()`: routes keys to the right state objects +- `#handleCommandKey()`: async I/O (clipboard reads, stat) for the `t`/`f` command mode keys +- `isLikelyPath()`: pure helper for path detection in `#handleCommandKey` + +## Decisions + +**Inline `#buildStatusLine`**: It was a one-liner with no logic of its own. The only reason to have a private method is to give a name to something complex enough to warrant one. `renderStatus(this.#statusState, cols)` is already self-explanatory at the call site. + +**`isLikelyPath` stays in AppLayout**: Used only in `#handleCommandKey`. Moving it to clipboard.ts or a utils file would create a dependency for 8 lines that only ever live in one context. The benefit doesn’t justify the churn. + +**togglePreview invariant not addressed**: `CommandModeState.togglePreview()` can set `previewMode = true` when `commandMode = false`. The renderer handles this correctly (returns empty previewRows), the original code had the same behaviour, and fixing it would change the state contract in a way that breaks existing tests. Reviewer noted this as non-blocking; leaving as-is. diff --git a/CLAUDE.md b/CLAUDE.md index 3e9deb6..8255c78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,9 +53,9 @@ Refactoring `AppLayout.ts` into focused, testable units (milestone 1.0 prerequis | 5b ConversationState + renderConversation | ✅ Done | #196 | | 5c ToolApprovalState + renderToolApproval | ✅ Done | #197 | | 5d CommandModeState + renderCommandMode | ✅ Done | #198 | -| 5e ScreenCoordinator cleanup | ⏳ Next | — | +| 5e ScreenCoordinator cleanup | ✅ Done | #199 | -Test count: 319 across 13 spec files. +Test count: 338 across 14 spec files. Refactor series complete. ## Recent Decisions diff --git a/apps/claude-sdk-cli/src/AppLayout.ts b/apps/claude-sdk-cli/src/AppLayout.ts index 104d796..bb0079c 100644 --- a/apps/claude-sdk-cli/src/AppLayout.ts +++ b/apps/claude-sdk-cli/src/AppLayout.ts @@ -6,6 +6,7 @@ import { sanitiseLoneSurrogates } from '@shellicar/claude-core/sanitise'; import type { Screen } from '@shellicar/claude-core/screen'; import { StdoutScreen } from '@shellicar/claude-core/screen'; import type { SdkMessageUsage } from '@shellicar/claude-sdk'; +import { buildSubmitText } from './buildSubmitText.js'; import { CommandModeState } from './CommandModeState.js'; import type { Block, BlockType } from './ConversationState.js'; import { ConversationState } from './ConversationState.js'; @@ -280,37 +281,9 @@ export class AppLayout implements Disposable { return; } const attachments = this.#commandModeState.takeAttachments(); - const parts: string[] = [text]; - if (attachments) { - for (let n = 0; n < attachments.length; n++) { - const att = attachments[n]; - if (!att) { - continue; - } - if (att.kind === 'text') { - const showSize = att.sizeBytes >= 1024 ? `${(att.sizeBytes / 1024).toFixed(1)}KB` : `${att.sizeBytes}B`; - const fullSize = att.fullSizeBytes >= 1024 ? `${(att.fullSizeBytes / 1024).toFixed(1)}KB` : `${att.fullSizeBytes}B`; - const truncPrefix = att.truncated ? `// showing ${showSize} of ${fullSize} (truncated)\n` : ''; - parts.push(`\n\n[attachment #${n + 1}]\n${truncPrefix}${att.text}\n[/attachment]`); - } else { - const lines: string[] = [`path: ${att.path}`]; - if (att.fileType === 'missing') { - lines.push('// not found'); - } else { - lines.push(`type: ${att.fileType}`); - if (att.fileType === 'file' && att.sizeBytes !== undefined) { - const sz = att.sizeBytes; - const sizeStr = sz >= 1024 ? `${(sz / 1024).toFixed(1)}KB` : `${sz}B`; - lines.push(`size: ${sizeStr}`); - } - } - parts.push(`\n\n[attachment #${n + 1}]\n${lines.join('\n')}\n[/attachment]`); - } - } - } const resolveInput = this.#editorResolve; this.#editorResolve = null; - resolveInput(parts.join('')); + resolveInput(buildSubmitText(text, attachments)); } #flushToScroll(): void { @@ -351,7 +324,7 @@ export class AppLayout implements Disposable { const visibleRows = overflow > 0 ? allContent.slice(overflow) : [...new Array(contentRows - allContent.length).fill(''), ...allContent]; const separator = buildDivider(null, cols); - const statusLine = this.#buildStatusLine(cols); + const statusLine = renderStatus(this.#statusState, cols); const allRows = [...visibleRows, separator, statusLine, approvalRow, commandRow, ...expandedRows]; let out = syncStart + hideCursor; @@ -439,8 +412,4 @@ export class AppLayout implements Disposable { } // All other keys silently consumed } - - #buildStatusLine(cols: number): string { - return renderStatus(this.#statusState, cols); - } } diff --git a/apps/claude-sdk-cli/src/buildSubmitText.ts b/apps/claude-sdk-cli/src/buildSubmitText.ts new file mode 100644 index 0000000..8110557 --- /dev/null +++ b/apps/claude-sdk-cli/src/buildSubmitText.ts @@ -0,0 +1,44 @@ +import type { Attachment } from './AttachmentStore.js'; + +/** + * Assemble the final message string from editor text and optional attachments. + * + * Each attachment becomes an [attachment #N] block appended after the main text. + * The format is part of the system prompt contract — the agent is told how these + * blocks are structured so it can reference attached content correctly. + * + * Returns plain text unchanged when there are no attachments, so callers do not + * need to handle the zero-attachment case specially. + */ +export function buildSubmitText(text: string, attachments: readonly Attachment[] | null): string { + if (!attachments || attachments.length === 0) { + return text; + } + const parts: string[] = [text]; + for (let n = 0; n < attachments.length; n++) { + const att = attachments[n]; + if (!att) { + continue; + } + if (att.kind === 'text') { + const showSize = att.sizeBytes >= 1024 ? `${(att.sizeBytes / 1024).toFixed(1)}KB` : `${att.sizeBytes}B`; + const fullSize = att.fullSizeBytes >= 1024 ? `${(att.fullSizeBytes / 1024).toFixed(1)}KB` : `${att.fullSizeBytes}B`; + const truncPrefix = att.truncated ? `// showing ${showSize} of ${fullSize} (truncated)\n` : ''; + parts.push(`\n\n[attachment #${n + 1}]\n${truncPrefix}${att.text}\n[/attachment]`); + } else { + const lines: string[] = [`path: ${att.path}`]; + if (att.fileType === 'missing') { + lines.push('// not found'); + } else { + lines.push(`type: ${att.fileType}`); + if (att.fileType === 'file' && att.sizeBytes !== undefined) { + const sz = att.sizeBytes; + const sizeStr = sz >= 1024 ? `${(sz / 1024).toFixed(1)}KB` : `${sz}B`; + lines.push(`size: ${sizeStr}`); + } + } + parts.push(`\n\n[attachment #${n + 1}]\n${lines.join('\n')}\n[/attachment]`); + } + } + return parts.join(''); +} diff --git a/apps/claude-sdk-cli/test/buildSubmitText.spec.ts b/apps/claude-sdk-cli/test/buildSubmitText.spec.ts new file mode 100644 index 0000000..27499bd --- /dev/null +++ b/apps/claude-sdk-cli/test/buildSubmitText.spec.ts @@ -0,0 +1,198 @@ +import { describe, expect, it } from 'vitest'; +import type { Attachment } from '../src/AttachmentStore.js'; +import { buildSubmitText } from '../src/buildSubmitText.js'; + +// --------------------------------------------------------------------------- +// No attachments +// --------------------------------------------------------------------------- + +describe('buildSubmitText — no attachments', () => { + it('returns text unchanged when attachments is null', () => { + const expected = 'hello world'; + const actual = buildSubmitText('hello world', null); + expect(actual).toBe(expected); + }); + + it('returns text unchanged when attachments is empty', () => { + const expected = 'hello world'; + const actual = buildSubmitText('hello world', []); + expect(actual).toBe(expected); + }); + + it('returns empty string unchanged when no attachments', () => { + const expected = ''; + const actual = buildSubmitText('', null); + expect(actual).toBe(expected); + }); +}); + +// --------------------------------------------------------------------------- +// Text attachments +// --------------------------------------------------------------------------- + +describe('buildSubmitText — text attachment', () => { + const textAtt: Attachment = { + kind: 'text', + hash: 'abc123', + text: 'some content', + sizeBytes: 12, + fullSizeBytes: 12, + truncated: false, + }; + + it('appends [attachment #1] block', () => { + const result = buildSubmitText('prompt', [textAtt]); + const expected = true; + const actual = result.includes('[attachment #1]'); + expect(actual).toBe(expected); + }); + + it('includes the attachment text content', () => { + const result = buildSubmitText('prompt', [textAtt]); + const expected = true; + const actual = result.includes('some content'); + expect(actual).toBe(expected); + }); + + it('includes [/attachment] closing tag', () => { + const result = buildSubmitText('prompt', [textAtt]); + const expected = true; + const actual = result.includes('[/attachment]'); + expect(actual).toBe(expected); + }); + + it('starts with the main prompt text', () => { + const result = buildSubmitText('my prompt', [textAtt]); + const expected = true; + const actual = result.startsWith('my prompt'); + expect(actual).toBe(expected); + }); + + it('does not include truncation notice when not truncated', () => { + const result = buildSubmitText('prompt', [textAtt]); + const expected = false; + const actual = result.includes('truncated'); + expect(actual).toBe(expected); + }); +}); + +// --------------------------------------------------------------------------- +// Truncated text attachment +// --------------------------------------------------------------------------- + +describe('buildSubmitText — truncated text attachment', () => { + const truncAtt: Attachment = { + kind: 'text', + hash: 'xyz', + text: 'stored text', + sizeBytes: 1024, + fullSizeBytes: 2048, + truncated: true, + }; + + it('includes truncation notice', () => { + const result = buildSubmitText('prompt', [truncAtt]); + const expected = true; + const actual = result.includes('truncated'); + expect(actual).toBe(expected); + }); + + it('includes showing size in truncation notice', () => { + const result = buildSubmitText('prompt', [truncAtt]); + const expected = true; + const actual = result.includes('showing'); + expect(actual).toBe(expected); + }); + + it('formats size in KB when >= 1024 bytes', () => { + const result = buildSubmitText('prompt', [truncAtt]); + const expected = true; + const actual = result.includes('KB'); + expect(actual).toBe(expected); + }); + + it('formats size in bytes when < 1024', () => { + const smallTrunc: Attachment = { + kind: 'text', + hash: 'small', + text: 'x', + sizeBytes: 500, + fullSizeBytes: 800, + truncated: true, + }; + const result = buildSubmitText('prompt', [smallTrunc]); + const expected = true; + const actual = result.includes('500B'); + expect(actual).toBe(expected); + }); +}); + +// --------------------------------------------------------------------------- +// File attachments +// --------------------------------------------------------------------------- + +describe('buildSubmitText — file attachment', () => { + it('includes path for file attachment', () => { + const att: Attachment = { kind: 'file', path: '/tmp/report.txt', fileType: 'file', sizeBytes: 2048 }; + const result = buildSubmitText('prompt', [att]); + const expected = true; + const actual = result.includes('path: /tmp/report.txt'); + expect(actual).toBe(expected); + }); + + it('includes type and size for file attachment', () => { + const att: Attachment = { kind: 'file', path: '/tmp/report.txt', fileType: 'file', sizeBytes: 2048 }; + const result = buildSubmitText('prompt', [att]); + const expected = true; + const actual = result.includes('type: file') && result.includes('size:'); + expect(actual).toBe(expected); + }); + + it('includes type: dir for directory attachment', () => { + const att: Attachment = { kind: 'file', path: '/tmp/mydir', fileType: 'dir' }; + const result = buildSubmitText('prompt', [att]); + const expected = true; + const actual = result.includes('type: dir'); + expect(actual).toBe(expected); + }); + + it('includes // not found for missing file', () => { + const att: Attachment = { kind: 'file', path: '/tmp/gone.txt', fileType: 'missing' }; + const result = buildSubmitText('prompt', [att]); + const expected = true; + const actual = result.includes('// not found'); + expect(actual).toBe(expected); + }); +}); + +// --------------------------------------------------------------------------- +// Multiple attachments +// --------------------------------------------------------------------------- + +describe('buildSubmitText — multiple attachments', () => { + const att1: Attachment = { kind: 'text', hash: 'h1', text: 'first', sizeBytes: 5, fullSizeBytes: 5, truncated: false }; + const att2: Attachment = { kind: 'file', path: '/tmp/second.txt', fileType: 'file', sizeBytes: 100 }; + + it('numbers attachments starting at #1', () => { + const result = buildSubmitText('prompt', [att1, att2]); + const expected = true; + const actual = result.includes('[attachment #1]'); + expect(actual).toBe(expected); + }); + + it('numbers second attachment #2', () => { + const result = buildSubmitText('prompt', [att1, att2]); + const expected = true; + const actual = result.includes('[attachment #2]'); + expect(actual).toBe(expected); + }); + + it('both attachments appear in order', () => { + const result = buildSubmitText('prompt', [att1, att2]); + const pos1 = result.indexOf('[attachment #1]'); + const pos2 = result.indexOf('[attachment #2]'); + const expected = true; + const actual = pos1 < pos2; + expect(actual).toBe(expected); + }); +});