From 0c77f7de5e1de6f5443dd852374c12d33e008096 Mon Sep 17 00:00:00 2001 From: rsbin1178 <3029898593@qq.com> Date: Thu, 8 Oct 2026 22:07:51 +0800 Subject: [PATCH] fix(coding/tui): keep the cached tool preview read-only while rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A compact tool block renders its rows by prefixing and styling each line, and the rows it was handed were the activity's own preview slice. Writing them back meant every re-render added another branch prefix and left the previous render's colour escapes inside the text, so switching the theme during a session grew a run of └ markers in front of the same diff line. The tone lookup made it worse: it reads the leading characters of the row to tell an added line from a removed one, and a row that starts with an escape fell through to the muted tone, which is why the - and + colours stopped following the theme after the first re-render. toolActivityRows now documents that the caller owns its result and clones a preview before returning it, so the cached activity stays input. The regression test renders one patch under the dark theme and then the light one: it asserts the activity is unchanged, that exactly one branch prefix is applied per render, and that each render carries its own theme's error and added tones with none of the other theme's escapes. --- internal/coding/tui/tool_activity.go | 10 ++- .../coding/tui/tool_activity_render_test.go | 89 +++++++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 internal/coding/tui/tool_activity_render_test.go diff --git a/internal/coding/tui/tool_activity.go b/internal/coding/tui/tool_activity.go index 3f8d49c..13c33cd 100644 --- a/internal/coding/tui/tool_activity.go +++ b/internal/coding/tui/tool_activity.go @@ -1423,6 +1423,10 @@ func renderToolHeading( return ansi.Truncate(heading, max(1, width), "…") } +// toolActivityRows returns the rows a compact block renders. The caller owns the +// result: a preview line lives in the cached activity, and a renderer that prefixes +// or styles rows used to write that back, so every re-render added another branch +// prefix and kept the first render's colour escapes inside the text. func toolActivityRows( class toolActivityClass, activities []toolActivity, @@ -1434,7 +1438,7 @@ func toolActivityRows( switch class { case toolClassShell: - return activities[0].preview + return slices.Clone(activities[0].preview) case toolClassPatch: if _, parsed := parsePatchDisplayChanges(activities[0]); !parsed { if _, _, single := singlePatchChange(activities[0]); single { @@ -1445,11 +1449,11 @@ func toolActivityRows( return nil } - return activities[0].preview + return slices.Clone(activities[0].preview) case toolClassGeneric: return append([]string{activities[0].invocation}, activities[0].preview...) case toolClassSubagent, toolClassTaskList: - return activities[0].preview + return slices.Clone(activities[0].preview) case toolClassExplore, toolClassPlan: return nil default: diff --git a/internal/coding/tui/tool_activity_render_test.go b/internal/coding/tui/tool_activity_render_test.go new file mode 100644 index 0000000..994285c --- /dev/null +++ b/internal/coding/tui/tool_activity_render_test.go @@ -0,0 +1,89 @@ +package tui + +import ( + "encoding/json" + "image/color" + "slices" + "strings" + "testing" + + "charm.land/lipgloss/v2" + "github.com/rsbin1178/pips/ai" + "github.com/rsbin1178/pips/internal/coding" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestToolPreviewRenderingKeepsTheActivityReadOnly pins the renderer to treat a +// cached preview as input. It used to write its prefixed and styled rows back into +// the activity, so every re-render added another branch prefix and kept the first +// render's colour escapes inside the text: switching the theme in a session then +// showed a growing run of └ markers in front of the same diff line. +func TestToolPreviewRenderingKeepsTheActivityReadOnly(t *testing.T) { + t.Parallel() + + patch := "*** Begin Patch\n" + + "*** Update File: world_map/selfcheck_reference_points.py\n" + + "@@\n" + + "- (13, 13), (31, 61),\n" + + "+ (15, 13), (31, 61),\n" + + "*** End Patch\n" + arguments, err := json.Marshal(map[string]string{"patch": patch}) + require.NoError(t, err) + + state := coding.State{Tools: []coding.ToolState{{ + Call: coding.ToolCall{ + ID: "call-1", Name: "apply_patch", Arguments: ai.JSON(string(arguments)), + }, + Status: coding.ToolStatusCompleted, + Result: ai.ToolResultText( + "call-1", + "apply_patch", + `{"schema":"pips.coding.tool_result/v1alpha1","ok":true,"tool":"apply_patch","counts":{"files":1,"additions":1,"deletions":1}}`+ + "\n\nworld_map/selfcheck_reference_points.py\n", + ), + }}} + + blocks := projectTimeline(state) + require.Len(t, blocks, 1) + require.NotEmpty(t, blocks[0].tools[0].preview, "the fixture carries a preview to render") + + preview := slices.Clone(blocks[0].tools[0].preview) + + dark := renderTimeline(blocks, newMarkdownRenderer(8), 80, themeDark, false) + assert.Equal(t, preview, blocks[0].tools[0].preview, "the first render rewrote the preview") + + light := renderTimeline(blocks, newMarkdownRenderer(8), 80, themeLight, false) + assert.Equal(t, preview, blocks[0].tools[0].preview, "the second render rewrote the preview") + + // The branch prefix is applied once per render rather than accumulating. + assert.Equal(t, 1, strings.Count(dark, "└"), "one branch prefix for one preview") + assert.Equal(t, 1, strings.Count(light, "└"), "the prefix count grew with the render") + + // Each render reads the rows the theme it was given asks for: the removed line + // as the error tone, the added line as the added tone, and neither theme's + // escapes left inside the other's output. + for _, rendered := range []struct { + name string + text string + theme colorTheme + foreign colorTheme + }{ + {name: "dark", text: dark, theme: themeDark, foreign: themeLight}, + {name: "light", text: light, theme: themeLight, foreign: themeDark}, + } { + assert.Contains(t, rendered.text, sgrPrefix(rendered.theme.palette.error), rendered.name) + assert.Contains(t, rendered.text, sgrPrefix(rendered.theme.palette.idle), rendered.name) + assert.NotContains(t, rendered.text, sgrPrefix(rendered.foreign.palette.error), rendered.name) + assert.NotContains(t, rendered.text, sgrPrefix(rendered.foreign.palette.idle), rendered.name) + } + + again := renderTimeline(blocks, newMarkdownRenderer(8), 80, themeDark, false) + assert.Equal(t, dark, again, "re-rendering one theme is stable") +} + +// sgrPrefix is the escape a style with one foreground colour opens with, which is +// how a rendered row's tone is read back. +func sgrPrefix(value color.Color) string { + return strings.TrimSuffix(lipgloss.NewStyle().Foreground(value).Render("x"), "x\x1b[m") +}