mirror of
https://github.com/getcompanion-ai/co-mono.git
synced 2026-04-21 01:01:42 +00:00
fix(tui): Container change detection for proper differential rendering
Fixed rendering artifact where duplicate bottom borders appeared when components dynamically shifted positions (e.g., Ctrl+C in agent clearing status container). Root cause: Container wasn't reporting as "changed" when cleared (0 children), causing differential renderer to skip re-rendering that area. Solution: Container now tracks previousChildCount and reports changed when child count changes, ensuring proper re-rendering when containers are cleared. - Added comprehensive test reproducing the layout shift artifact - Fixed Container to track and report child count changes - All tests pass including new layout shift artifact test
This commit is contained in:
parent
2ec8a27222
commit
192d8d2600
24 changed files with 356 additions and 2910 deletions
|
|
@ -0,0 +1,82 @@
|
|||
## Analysis of TUI Differential Rendering and Layout Shift Artifacts
|
||||
|
||||
### Key Findings
|
||||
|
||||
**1. The Surgical Differential Rendering Implementation**
|
||||
|
||||
The TUI uses a three-strategy rendering system in `renderDifferentialSurgical` (lines 331-513 in `/Users/badlogic/workspaces/pi-mono/packages/tui/src/tui.ts`):
|
||||
|
||||
- **SURGICAL**: Updates only changed lines with same line counts
|
||||
- **PARTIAL**: Re-renders from first change when structure/line counts shift
|
||||
- **FULL**: Clears scrollback when changes are above viewport
|
||||
|
||||
**2. The Critical Gap: Cursor Positioning in SURGICAL Strategy**
|
||||
|
||||
The artifact issue lies in the SURGICAL strategy's cursor positioning logic (lines 447-493). When components are added and removed dynamically, the cursor positioning calculations become incorrect, leading to incomplete clearing of old content.
|
||||
|
||||
**Specific Problem Areas:**
|
||||
|
||||
```typescript
|
||||
// Lines 484-492: Cursor repositioning after surgical updates
|
||||
const lastContentLine = totalNewLines - 1;
|
||||
const linesToMove = lastContentLine - currentCursorLine;
|
||||
if (linesToMove > 0) {
|
||||
output += `\x1b[${linesToMove}B`;
|
||||
} else if (linesToMove < 0) {
|
||||
output += `\x1b[${-linesToMove}A`;
|
||||
}
|
||||
// Now add final newline to position cursor on next line
|
||||
output += "\r\n";
|
||||
```
|
||||
|
||||
**3. Component Change Detection Issues**
|
||||
|
||||
The system determines changes by comparing:
|
||||
- Component IDs (structural changes)
|
||||
- Line counts (hasLineCountChange)
|
||||
- Content with same line counts (changedLines array)
|
||||
|
||||
However, when a status message is added temporarily then removed, the detection logic may not properly identify all affected lines that need clearing.
|
||||
|
||||
**4. Missing Test Coverage**
|
||||
|
||||
Current tests in `/Users/badlogic/workspaces/pi-mono/packages/tui/test/` don't cover the specific scenario of:
|
||||
- Dynamic addition of components that cause layout shifts
|
||||
- Temporary status messages that appear and disappear
|
||||
- Components moving back to original positions after removals
|
||||
|
||||
### The Agent Scenario Analysis
|
||||
|
||||
The agent likely does this sequence:
|
||||
1. Has header, chat container, text editor in vertical layout
|
||||
2. Adds a status message component between chat and editor
|
||||
3. Editor shifts down (differential render uses PARTIAL strategy)
|
||||
4. After delay, removes status message
|
||||
5. Editor shifts back up (this is where artifacts remain)
|
||||
|
||||
The issue is that when the editor moves back up, the SURGICAL strategy is chosen (same component structure, just content changes), but it doesn't properly clear the old border lines that were drawn when the editor was in the lower position.
|
||||
|
||||
### Root Cause
|
||||
|
||||
The differential rendering assumes that when using SURGICAL updates, only content within existing component boundaries changes. However, when components shift positions due to additions/removals, old rendered content at previous positions isn't being cleared properly.
|
||||
|
||||
**Specific gap:** The SURGICAL strategy clears individual lines with `\x1b[2K` but doesn't account for situations where component positions have changed, leaving artifacts from the previous render at the old positions.
|
||||
|
||||
### Test Creation Recommendation
|
||||
|
||||
A test reproducing this would:
|
||||
|
||||
```typescript
|
||||
test("clears artifacts when components shift positions dynamically", async () => {
|
||||
// 1. Setup: header, container, editor
|
||||
// 2. Add status message (causes editor to shift down)
|
||||
// 3. Remove status message (editor shifts back up)
|
||||
// 4. Verify no border artifacts remain at old editor position
|
||||
});
|
||||
```
|
||||
|
||||
The test should specifically check that after the removal, there are no stray border characters (`╭`, `╮`, `│`, `╰`, `╯`) left at the position where the editor was temporarily located.
|
||||
|
||||
### Proposed Fix Direction
|
||||
|
||||
The PARTIAL strategy should be used more aggressively when components are added/removed, even if the final structure looks identical, to ensure complete clearing of old content. Alternatively, the SURGICAL strategy needs enhanced logic to detect and clear content at previous component positions.
|
||||
|
|
@ -0,0 +1,36 @@
|
|||
# Agent/TUI: Ctrl+C Display Artifact
|
||||
**Status:** Done
|
||||
**Agent PID:** 36116
|
||||
|
||||
## Original Todo
|
||||
agent/tui: when pressing ctrl + c, the editor gets pushed down by one line, after a second it gets pushed up again, leaving an artifact (duplicate bottom border). Should replicate this in a test:
|
||||
Press Ctrl+C again to exit
|
||||
╭────────────────────────────────────────────────────────────────────────────────────────────────────────╮
|
||||
│ > │
|
||||
╰────────────────────────────────────────────────────────────────────────────────────────────────────────╯
|
||||
╰────────────────────────────────────────────────────────────────────────────────────────────────────────╯
|
||||
↑967 ↓12 ⚒ 4
|
||||
|
||||
## Description
|
||||
Create a test in the TUI package that reproduces the rendering artifact issue when components dynamically shift positions (like when a status message appears and disappears). The test will verify that when components move back to their original positions after a temporary layout change, no visual artifacts (duplicate borders) remain. If the test reveals a bug in the TUI's differential rendering, fix it.
|
||||
|
||||
*Read [analysis.md](./analysis.md) in full for detailed codebase research and context*
|
||||
|
||||
## Implementation Plan
|
||||
Create a test that reproduces the layout shift artifact issue in the TUI differential rendering, then fix the rendering logic if needed to properly clear old content when components shift positions.
|
||||
|
||||
- [x] Create test file `packages/tui/test/layout-shift-artifacts.test.ts` that reproduces the issue
|
||||
- [x] Test should create components in vertical layout, add a temporary component causing shifts, remove it, and verify no artifacts
|
||||
- [x] Run test to confirm it reproduces the artifact issue
|
||||
- [x] Fix the differential rendering logic in `packages/tui/src/tui.ts` to properly clear content when components shift
|
||||
- [x] Verify all tests pass (including the new one) after fix
|
||||
- [x] Run `npm run check` to ensure code quality
|
||||
|
||||
## Notes
|
||||
The issue was NOT in the differential rendering strategy as initially thought. The real bug was in the Container component:
|
||||
|
||||
When a Container is cleared (has 0 children), it wasn't reporting as "changed" because the render method only checked if any children reported changes. Since there were no children after clearing, `changed` remained false, and the differential renderer didn't know to re-render that area.
|
||||
|
||||
The fix: Container now tracks `previousChildCount` and reports as changed when the number of children changes (especially important for going from N children to 0).
|
||||
|
||||
This ensures that when statusContainer.clear() is called in the agent, the differential renderer properly clears and re-renders that section of the screen.
|
||||
|
|
@ -1,7 +1,3 @@
|
|||
- agent/tui: broken rendering of resumed session messages
|
||||
- start session, "read all README.md files except in node_modules
|
||||
- stop session
|
||||
- resume session, messages are cut off?
|
||||
|
||||
- pods: pi start outputs all models that can be run on the pod. however, it doesn't check the vllm version. e.g. gpt-oss can only run via vllm+gpt-oss. glm4.5 can only run on vllm nightly.
|
||||
|
||||
|
|
@ -41,8 +37,6 @@
|
|||
|
||||
- agent: test for basic functionality, including thinking, completions & responses API support for all the known providers and their endpoints.
|
||||
|
||||
- agent: token usage gets overwritten with each message that has usage data. however, if the latest data doesn't have a specific usage field, we record undefined i think? also, {"type":"token_usage" "inputTokens":240,"outputTokens":35,"totalTokens":275,"cacheReadTokens":0,"cacheWriteTokens":0} doesn't contain reasoningToken? do we lack initialization? See case "token_usage": in renderers. probably need to check if lastXXX > current and use lastXXX.
|
||||
|
||||
- agent: groq responses api throws on second message
|
||||
```
|
||||
➜ pi-mono git:(main) ✗ npx tsx packages/agent/src/cli.ts --base-url https://api.groq.com/openai/v1 --api-key $GROQ_API_KEY --model openai/gpt-oss-120b --api responses
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue