Skip to content

Extract ConversationState and renderConversation from AppLayout (step 5b)#196

Merged
shellicar merged 2 commits intomainfrom
feature/conversation-state
Apr 6, 2026
Merged

Extract ConversationState and renderConversation from AppLayout (step 5b)#196
shellicar merged 2 commits intomainfrom
feature/conversation-state

Conversation

@shellicar
Copy link
Copy Markdown
Owner

What

Extracts the conversation block state and rendering out of AppLayout into two focused, independently testable files.

ConversationState.ts — pure state

  • #sealedBlocks, #flushedCount, #activeBlock move here
  • transitionBlock returns a result struct ({noop, from, sealed}) so AppLayout can log without re-reading private state
  • appendToLastSealed returns 'active', a sealed index, or 'miss' so the caller can log the exact target
  • addBlocks, appendToActive, completeActive, advanceFlushedCount cover the remaining mutation surface

renderConversation.ts — pure rendering

  • All block rendering helpers move here: renderBlockContent, buildDivider, BLOCK_EMOJI, BLOCK_PLAIN
  • renderConversation(state, cols) — sealed + active blocks for the alt-buffer viewport
  • renderBlocksToString(blocks, startIndex, cols) — slice-to-string for the flush-to-scroll path; uses full array for continuation checks so headers suppress correctly even for already-flushed blocks
  • buildDivider(null, cols) replaces the inline DIM + FILL.repeat(cols) + RESET in render()

AppLayout after

  • Holds #conversationState = new ConversationState()
  • All block mutations delegate to it
  • render() calls renderConversation(this.#conversationState, cols) then appends editor lines
  • #flushToScroll() calls renderBlocksToString(...) and advanceFlushedCount()
  • FILL constant removed; BLOCK_EMOJI/PLAIN removed; renderBlockContent/buildDivider removed

Tests

41 new tests (231 total, up from 190):

  • ConversationState.spec.ts (27): initial state, addBlocks, transitionBlock (noop/seal/discard/from), appendToActive, completeActive, appendToLastSealed (active/sealed/miss/most-recent), advanceFlushedCount
  • renderConversation.spec.ts (14): empty state, single block structure, continuation suppression, active block rendering, buildDivider variants

Risk

Medium. The flush-to-scroll boundary (#flushedCount) is the dangerous part — blocks flushed to scroll are permanently written and cannot be un-written. This is addressed by keeping the flush logic in AppLayout and only delegating state reads/writes to ConversationState. The continuation-check references the full sealed block array (not just the unflushed slice) so headers suppress correctly for already-flushed blocks.

… 5b)

Moves block state and rendering out of AppLayout into two focused files:

- ConversationState.ts: sealed blocks, active block, flush boundary. Pure
  state machine — no I/O, no rendering. transitionBlock returns a result
  struct so the caller can log without needing to re-read private state.
  appendToLastSealed returns 'active', a sealed index, or 'miss' so the
  caller can log the exact target without duplicating the search logic.

- renderConversation.ts: all block rendering helpers (renderBlockContent,
  buildDivider, BLOCK_EMOJI/PLAIN constants) plus three exports:
    renderConversation(state, cols) — sealed + active blocks for the
      alt-buffer viewport
    renderBlocksToString(blocks, startIndex, cols) — slice-to-string for
      the flush-to-scroll path; uses full array for continuation checks
    buildDivider(label, cols) — used by AppLayout for the prompt divider
      and separator (buildDivider(null, cols))

AppLayout after: holds #conversationState, delegates all block mutations
to it, calls renderConversation in render(), calls renderBlocksToString
in #flushToScroll(). The separator DIM+FILL+RESET is now buildDivider(null,
cols) — same output, no separate FILL constant needed in AppLayout.

Tests: 41 new (27 ConversationState, 14 renderConversation). 231 total.
@shellicar shellicar added this to the 1.0 milestone Apr 6, 2026
@shellicar shellicar added the enhancement New feature or request label Apr 6, 2026
@shellicar shellicar self-assigned this Apr 6, 2026
@shellicar shellicar requested a review from bananabot9000 April 6, 2026 13:23
@shellicar shellicar enabled auto-merge (squash) April 6, 2026 13:23
Copy link
Copy Markdown
Collaborator

@bananabot9000 bananabot9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean extraction. Return-value APIs (TransitionResult, appendToLastSealed discriminant) are the highlight -- opaque state with expressive caller interfaces. renderBlocksToString taking the full array is the subtle-but-correct call for flush boundary continuations. One suggestion: add a test for renderBlocksToString covering continuation suppression across a flush boundary, since the PR itself identifies this as the highest-risk area. 41 tests, 706 total. Approve. 🍌

@shellicar shellicar merged commit 2fa8fa6 into main Apr 6, 2026
4 checks passed
@shellicar shellicar deleted the feature/conversation-state branch April 6, 2026 13:53
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants