Extract buildSubmitText and inline #buildStatusLine (step 5e)#199
Merged
Extract buildSubmitText and inline #buildStatusLine (step 5e)#199
Conversation
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.
bananabot9000
approved these changes
Apr 6, 2026
Collaborator
bananabot9000
left a comment
bananabot9000
left a comment
There was a problem hiding this comment.
The finale. buildSubmitText is a clean, pure extraction -- the last non-trivial inline logic in AppLayout. #buildStatusLine inline is the right call (one-liner wrapper adds indirection without clarity). AppLayout is now a proper thin coordinator: screen management, five state objects, key routing, async I/O. All pure logic tested in 338 tests across 14 spec files. The refactor series (3a-5e, PRs #189-199) held its State/Renderer/Coordinator pattern consistently throughout. 006 has earned his licence. Approve. 🍌🎬
shellicar
added a commit
that referenced
this pull request
Apr 7, 2026
…factor The current-state section still described the architecture refactor as active work with a step-by-step table. That series finished at PR #199. Replace it with the three open PRs (#206, #207, #211) and a pointer to the backlog plans. Recent decisions gains entries for the PreviewEdit lineEdits/textEdits redesign and the CacheTtl enum move. Remove the stale cancelFn note — that work landed in #199.
shellicar
added a commit
that referenced
this pull request
Apr 7, 2026
* Add sdk-tools and cli-features backlogs, link all GitHub issues * Update CLAUDE.md: current state reflects active PRs, not completed refactor The current-state section still described the architecture refactor as active work with a step-by-step table. That series finished at PR #199. Replace it with the three open PRs (#206, #207, #211) and a pointer to the backlog plans. Recent decisions gains entries for the PreviewEdit lineEdits/textEdits redesign and the CacheTtl enum move. Remove the stale cancelFn note — that work landed in #199.
shellicar
added a commit
that referenced
this pull request
Apr 8, 2026
CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress; the architecture refactor completed through PR #199 (step 5e) weeks ago. Config loading (#222), git delta injection (#225), and the ANSI wrap fix (#223) had also shipped without being recorded. Updated current state, added two missing recent-decisions entries, removed a duplicate closing tag. Created issue #226 for CLAUDE.md loading and added the design to cli-features.md: load order, hot reload pattern, config opt-out.
shellicar
added a commit
that referenced
this pull request
Apr 8, 2026
CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress; the architecture refactor completed through PR #199 (step 5e) weeks ago. Config loading (#222), git delta injection (#225), and the ANSI wrap fix (#223) had also shipped without being recorded. Updated current state, added two missing recent-decisions entries, removed a duplicate closing tag. Created issue #226 for CLAUDE.md loading and added the design to cli-features.md: load order, hot reload pattern, config opt-out.
shellicar
added a commit
that referenced
this pull request
Apr 9, 2026
* Add CLAUDE.md loading issue #226 and update stale harness CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress; the architecture refactor completed through PR #199 (step 5e) weeks ago. Config loading (#222), git delta injection (#225), and the ANSI wrap fix (#223) had also shipped without being recorded. Updated current state, added two missing recent-decisions entries, removed a duplicate closing tag. Created issue #226 for CLAUDE.md loading and added the design to cli-features.md: load order, hot reload pattern, config opt-out. * WIP: Add cachedReminders and CLAUDE.md loading (feature incomplete) Adds cachedReminders?: string[] to RunAgentQuery — each entry becomes a <system-reminder> block prepended to the first user message of a new conversation. Stored in history so the prefix is cached by the API on every subsequent turn. ClaudeMdLoader reads ~/.claude/CLAUDE.md, CLAUDE.md, .claude/CLAUDE.md, and CLAUDE.md.local at startup (missing files silently skipped). Content is combined under a single instruction prefix and passed as cachedReminders. Stopping here to fix a separate bug in systemReminder (injected into tool-result messages instead of only human turns) before completing this feature. Will rebase onto the fix once it lands. Closes #226 (not yet ready — resuming after systemReminder fix). * Complete CLAUDE.md loading: config flag, cachedReminders tests The WIP commit had ClaudeMdLoader, the cachedReminders SDK field, and the wiring in main.ts/runAgent.ts, but was missing two things: 1. The claudeMd.enabled config flag. Loading is on by default; setting it to false in sdk-config.json disables it entirely. Follows the same .optional().default().catch() pattern as historyReplay so invalid values silently fall back to the default rather than crashing. 2. Tests for the cachedReminders injection path in AgentRun: - injects reminder as first block when history is empty - skips injection when the conversation already has messages The second test asserts absence of a <system-reminder> block rather than string content type, because RequestBuilder converts all string content to arrays before the streamer sees it. Closes #226 * Re-inject cachedReminders after compaction The injection condition was `history.messages.length === 0`, which only covered a fresh conversation. After compaction, the history contains one message — the compaction block (assistant role) — so the condition was false and reminders were not re-injected. This is wrong. Compaction drops all content before the compaction block, including the first user message that held the cached reminders. The next human turn needs the reminders re-injected so they are present in the effective context. Fix: change the condition to check for absence of user messages rather than an empty history. After compaction only the assistant compaction block remains — no user messages — so injection correctly fires. Once the new user message (with reminders) is pushed, subsequent turns have a user message in history and injection is correctly skipped. Test added first to prove the bug: post-compaction history with only the compaction block, verifying the first user message sent to the API contains a <system-reminder> block. The test failed before the fix and passes after. * Fix Biome violations in ClaudeMdLoader files The WIP commit had two issues caught by the pre-push hook: - INSTRUCTION_PREFIX split across lines (format violation) - Braces missing on single-line if return (useBlockStatements) - Four non-null assertions in the spec (noNonNullAssertion) Replaced ! assertions with ?? '' — the tests that use the result for string operations still work correctly, and the null case would produce an empty string that fails the subsequent toContain assertions anyway. * Session log 2026-04-09 — systemReminder fix, CLAUDE.md loading * Add claudeMd to generated JSON schema * Use IFileSystem in ClaudeMdLoader: cwd/homedir from fs, read on every turn IFileSystem changed to an abstract class with cwd() added alongside homedir(), so the filesystem owns all path context. NodeFileSystem implements cwd() via process.cwd(); MemoryFileSystem takes a cwd constructor param so tests can set it alongside home. ClaudeMdLoader drops the separate cwd/home constructor params and calls fs.cwd()/fs.homedir() inside getContent(), keeping the constructor to a single IFileSystem argument. A nodeFs singleton is exported from the fs entry so callers import it directly instead of newing a NodeFileSystem. Content was read once before the loop and stored in cachedReminders, making the on-demand design pointless. It now reads inside the loop on every turn so CLAUDE.md changes are picked up without a watcher. * Fix fs imports. * Linting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Final step in the AppLayout refactor series.
What
buildSubmitText.ts— pure function extracting the attachment serialization from the ctrl+enter handler#buildStatusLineinlined (one-line delegation, no reason to be a separate method)buildSubmitText.spec.tsWhy buildSubmitText
The ctrl+enter handler had ~25 inline lines building
[attachment #N]...[/attachment]blocks. The attachment format is contractual — the system prompt tells the agent how these blocks are structured — so it deserves a named, tested function. As pure logic (no I/O, no state) it extracts cleanly and tests straightforwardly.Why inline #buildStatusLine
It was
return renderStatus(this.#statusState, cols). A private method is worth naming when it encapsulates something complex or non-obvious. This wasn’t either. The call site reads identically and names both the function and its inputs.AppLayout after the refactor series
AppLayout now contains only coordination glue: screen management, mode flag, five state objects (
ConversationState,EditorState,StatusState,ToolApprovalState,CommandModeState), two promise holders (#editorResolve,#cancelFn), render/flush/debounce plumbing,handleKeyrouting, and async I/O for clipboard/file attachment keys.All pure logic — state mutations, rendering, message assembly — lives in separate tested modules.
Test count
338 across 14 files (up from 319). Refactor series complete.