Add interactive terminal companions - #887
Conversation
7bf583f to
da2768d
Compare
WalkthroughThis change adds terminal pets with persisted selection, remote catalog and installation support, animation metadata, Kitty and Sixel rendering, synchronized output handling, and TUI picker, preview, dragging, layout, and playback integration. ChangesTerminal pet feature
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (18)
internal/tui/run.go (1)
83-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a table test for
terminalPetFrameCache.
internal/tui/run_test.goexists but the diff adds no coverage for this function. Cover an absoluteUserConfigPath, a relativeUserConfigPath, a whitespace-onlyUserConfigPath, and theos.UserConfigDir()fallback. Compare results withfilepath.Joinrather than hardcoded separators so the test passes on Windows.The coding guidelines require a regression test for every behavior change, and require canonicalizing paths before comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/run.go` around lines 83 - 94, Add a table-driven test for terminalPetFrameCache in run_test.go covering absolute, relative, and whitespace-only UserConfigPath values plus the os.UserConfigDir fallback. Build expected paths with filepath.Join and canonicalize paths before comparison, while preserving platform-independent assertions.Source: Coding guidelines
internal/terminalpet/image_renderer.go (4)
368-380: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the aspect-ratio division against a zero-height bound.
bounds.Dy()is the divisor at Line 376.frame.Bounds().Empty()at Line 369 returns true when either dimension is zero, so the current call path is safe. The guard depends on that coupling. State it, or compute the ratio only whenbounds.Dy() > 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/image_renderer.go` around lines 368 - 380, Make the zero-height precondition explicit in renderSixel before calculating widthPixels: derive bounds and validate bounds.Dy() > 0 before using it as the divisor, while preserving the existing invalid-frame error and scaling behavior.
338-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
len(parts) == 0is dead code.
strings.Splitalways returns at least one element. Thelen(parts) == 0test at Line 340 never evaluates true. Keep only the upper bound.🧹 Proposed cleanup
- if len(parts) == 0 || len(parts) > 3 { + if len(parts) > 3 { return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/image_renderer.go` around lines 338 - 366, Remove the unreachable len(parts) == 0 condition from the validation in dottedVersionAtLeast, keeping only the len(parts) > 3 upper-bound check and preserving the remaining version parsing behavior.
314-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe frame cache grows without bound.
cachePNGFramewrites one file per unique frame digest and never removes anything. Each installed pet, each animation state, and each phase adds a file under~/.config/zero/pets/frame-cache. The directory only grows across sessions. Add a size or age budget, or prune the directory at startup.This applies only to the iTerm2 local-file protocol path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/image_renderer.go` around lines 314 - 336, Update cachePNGFrame to enforce a bounded frame-cache policy for the iTerm2 local-file protocol path, pruning cached PNGs by age or total size before or after writing the new digest file. Preserve current digest-based reuse and error propagation, and limit cleanup to the configured terminal pet frame-cache directory.
189-216: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRestore the cursor when the protocol switch takes the default branch.
Line 189 writes
\x1b[sand a cursor-position sequence before the switch. Thedefaultbranch returns at Line 215 without writing\x1b[u. The saved cursor is then never restored, and the cursor stays at the pet position. The branch is unreachable today becauseSupported()only admits the three handled protocols. A new protocol value would make it reachable.🛠️ Proposed fix
default: + if _, err := io.WriteString(writer, "\x1b[u"); err != nil { + return err + } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/image_renderer.go` around lines 189 - 216, Update the default branch of the protocol switch in the renderer’s frame-writing method to restore the previously saved cursor with the matching restore sequence before returning. Leave the existing handling for ImageProtocolKitty, ImageProtocolKittyLocalFile, and ImageProtocolSixel unchanged.internal/tui/pet_output_test.go (2)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
Seekcall beforeos.ReadFilehas no effect.Line 44 moves the file offset. Line 47 calls
os.ReadFile(file.Name()), which opens a separate handle and ignores that offset. Remove theSeek.Elsewhere in this file, Lines 150, 197, 236, and 308 use
file.Seek(0, 1)to capture the current offset. That use is correct, butio.SeekCurrentreads better than the literal1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/pet_output_test.go` around lines 44 - 50, Remove the redundant file.Seek call immediately before os.ReadFile in the affected test. In the other offset-capture calls in this file, replace the literal whence value 1 with io.SeekCurrent while preserving their existing behavior.
17-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a concurrency test for
petImageOutput.
petImageOutputholdsmuto serializeWriteandclearImage. No test drives both concurrently, so the race detector never exercises that guard. Bubble Tea writes frames from its render goroutine whileinternal/tui/run.goLine 69 callsclearImageon the main goroutine.Add a test that runs concurrent
Writecalls against aclearImagecall and run it with-race.The coding guidelines require running affected concurrent code under the race detector.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/pet_output_test.go` around lines 17 - 27, Extend the tests around petImageOutput with a concurrency case that executes concurrent Write calls while clearImage runs, exercising the mu synchronization between them. Ensure the test is race-detector compatible and validate the affected package with go test -race.Source: Coding guidelines
internal/terminalpet/image_renderer_test.go (1)
142-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd failure-path coverage for the local-file cache.
cachePNGFramereturns "terminal pet image cache is unavailable" whencacheDiris empty.internal/tui/run.goLine 93 can return an empty cache path when no config root resolves, andRunthen constructs the renderer with that empty path. On iTerm2 that combination makes everyRendercall return an error, whichpetImageOutput.Writepropagates to Bubble Tea as a write failure.Add a test that builds
NewImageRenderer(ImageSupport{Protocol: ImageProtocolKittyLocalFile})with no cache and asserts the error. Also add aZellijdetection case and a malformedTERM_PROGRAM_VERSIONcase.The coding guidelines require a regression test for every behavior or security-boundary change, including failure paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/image_renderer_test.go` around lines 142 - 167, Extend the image renderer tests with a local-file failure case using NewImageRenderer and ImageProtocolKittyLocalFile without a cache directory, asserting Render returns the “terminal pet image cache is unavailable” error. Also add coverage for Zellij detection and malformed TERM_PROGRAM_VERSION handling in the relevant terminal-detection tests, preserving existing behavior for valid detection inputs.Source: Coding guidelines
internal/terminalpet/sixel.go (2)
12-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSixel frames are re-quantized and re-encoded on every render.
The Kitty path caches PNG bytes in
Animation.pngCache(seeinternal/terminalpet/model.goLines 56-63). The Sixel path has no equivalent.renderSixelininternal/terminalpet/image_renderer.goLines 368-380 runs a Catmull-Rom rescale, andencodeSixelthen walks every pixel twice and allocates a[]int16buffer plus a map, on every animation tick.Add a cache keyed by
frameCacheKeyplus the target height, in the same style aspngCache.The static analysis narrowing warnings on Lines 29, 30, 56, and 88 are false positives.
red>>13andgreen>>13yield 0-7, andblue>>14yields 0-3, so the packed value always fits inuint8, and the palette index always fits inint16.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/sixel.go` around lines 12 - 48, Add Sixel frame caching keyed by frameCacheKey and target height, matching the existing Animation.pngCache pattern used by the Kitty renderer. Update renderSixel and the Animation cache state so repeated renders reuse encoded Sixel bytes instead of rescaling and calling encodeSixel each tick; retain encodeSixel’s current quantization and ignore the noted narrowing warnings.Source: Linters/SAST tools
49-75: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNo run-length encoding makes each Sixel frame large.
The band loop emits one byte per pixel column per active color. For a 100-pixel-wide frame with 40 active colors in a band, one band costs about 4 KB, and a 13-band frame costs about 52 KB. The encoder writes that on every animation tick to the terminal.
Sixel supports the
!<count>repeat introducer. Collapse runs of identical mask bytes to cut the payload by a large factor.⚡ Sketch of the repeat introducer
- for x := 0; x < width; x++ { - mask := byte(0) - for bit := 0; bit < sixelBandHeight && bandTop+bit < height; bit++ { - if pixels[(bandTop+bit)*width+x] == int16(colorIndex) { - mask |= 1 << bit - } - } - output.WriteByte('?' + mask) - } + run, runLength := byte(0), 0 + flush := func() { + switch { + case runLength == 0: + case runLength < 4: + output.WriteString(strings.Repeat(string(rune('?'+run)), runLength)) + default: + fmt.Fprintf(&output, "!%d%c", runLength, '?'+run) + } + } + for x := 0; x < width; x++ { + mask := byte(0) + for bit := 0; bit < sixelBandHeight && bandTop+bit < height; bit++ { + if pixels[(bandTop+bit)*width+x] == int16(colorIndex) { + mask |= 1 << bit + } + } + if runLength > 0 && mask == run { + runLength++ + continue + } + flush() + run, runLength = mask, 1 + } + flush()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/sixel.go` around lines 49 - 75, Update the Sixel emission loop in the band encoder to run-length encode consecutive identical mask bytes using Sixel’s !<count> repeat introducer, while preserving the existing color and band delimiters. Ensure each run is emitted with the correct count and mask byte, including single-byte runs, and retain the current output behavior when masks differ.internal/terminalpet/client.go (4)
487-493: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe
valueclosure silently reads column 0 for any field absent fromwanted.
index := wanted[field]returns 0 whenfieldis not a manifest key. The closure then decodesrow[0], which is the slug column, into the target.This is safe today. Every field passed to
valueappears inrequiredand is checked at lines 473-477. The hazard is future edits: adding an optional field read throughvaluewould silently decode the slug into it, with no error.Make the lookup explicit.
🛡️ Proposed guard
value := func(field string, target any) error { - index := wanted[field] - if index >= len(row) { + index, ok := wanted[field] + if !ok { + return fmt.Errorf("pet catalog has no %q field", field) + } + if index >= len(row) { return fmt.Errorf("pet catalog row is missing %q", field) } return json.Unmarshal(row[index], target) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/client.go` around lines 487 - 493, Update the value closure’s wanted lookup to explicitly check whether field exists in wanted before indexing the row. Return the existing missing-column error when the manifest key is absent, while preserving the bounds check and JSON unmarshalling for valid fields.
319-332: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
LoadInstalledskips the sprite-version validation thatInstallapplies.
Installvalidates the metadata sprite version at lines 247-252 and rejects anything other than 1 or 2.LoadInstalledat lines 323-325 assignsdocument.SpriteVersionwith no equivalent check.The failure is contained:
atlasAnimationrejects unknown versions atinternal/terminalpet/model.goline 261. So a tampered installedpet.jsonfails closed. The user sees "unsupported sprite version" instead of the more specific "invalid pet metadata" message.Align the two paths so the validation rule lives in one place.
♻️ Proposed alignment
if document.SpriteVersion != 0 { + if document.SpriteVersion != 1 && document.SpriteVersion != 2 { + return nil, fmt.Errorf("read installed pet metadata: unsupported sprite version %d", document.SpriteVersion) + } entry.SpriteVersion = document.SpriteVersion }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/client.go` around lines 319 - 332, Centralize the sprite-version validation currently applied by Install and reuse it from LoadInstalled before assigning document.SpriteVersion or calling document.atlasTracks. Preserve the rule that only versions 1 and 2 are valid, and return the existing invalid-pet-metadata error for unsupported versions instead of relying on atlasAnimation to reject them.
771-782: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWindows reserved device names pass
validateSlugand become directory paths.The character class allows
[a-z0-9]plus-and_. Names such ascon,prn,aux,nul,com1throughcom9, andlpt1throughlpt9all pass. Line 293 and line 304 pass the slug tofilepath.Joinunder the installed directory.On Windows those names resolve to devices, not files.
os.MkdirAllfails, soInstallreturns an error rather than corrupting state. The behavior is fail-closed but the error text is confusing, and a remote catalog controls the slug.The coding guidelines require code to pass on Linux, macOS, and Windows. Add a reserved-name rejection and a test case.
🛡️ Proposed guard and test
+// Windows resolves these names to devices regardless of directory, so they can +// never back an installed pet directory. Rejected on every platform to keep the +// installed layout identical across operating systems. +var reservedSlugs = map[string]bool{ + "con": true, "prn": true, "aux": true, "nul": true, + "com1": true, "com2": true, "com3": true, "com4": true, "com5": true, + "com6": true, "com7": true, "com8": true, "com9": true, + "lpt1": true, "lpt2": true, "lpt3": true, "lpt4": true, "lpt5": true, + "lpt6": true, "lpt7": true, "lpt8": true, "lpt9": true, +} + func validateSlug(slug string) error { if slug == "" || len(slug) > 128 { return fmt.Errorf("invalid pet slug %q", slug) } + if reservedSlugs[slug] { + return fmt.Errorf("invalid pet slug %q", slug) + } for index, value := range slug {Extend the existing table in
internal/terminalpet/client_test.goline 298:- for _, slug := range []string{"../boba", "/boba", "Boba", "boba/other", ""} { + for _, slug := range []string{"../boba", "/boba", "Boba", "boba/other", "", "con", "nul", "lpt1"} {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/client.go` around lines 771 - 782, Update validateSlug to reject Windows reserved device names, including con, prn, aux, nul, com1–com9, and lpt1–lpt9, case-insensitively and when followed by a slug extension or suffix as applicable to Windows path semantics. Preserve existing character and length validation, and extend the validateSlug tests in client_test.go with representative reserved-name cases.Source: Coding guidelines
745-750: 🚀 Performance & Scalability | 🔵 TrivialThe preview cache grows without bound.
cacheFilewrites intopreviewDir()with one entry per(slug, spritesheet URL)pair, keyed by the hash at line 726. Nothing ever removes those files. When an upstream pet republishes a new spritesheet URL, a new cache file appears and the old one stays.Each file is capped at
maxPreviewBytes(2 MiB). The manifest holds up to 60 ranked pets per fetch. Growth is slow but monotonic in a user config directory.Consider a size or age sweep on the preview directory at startup.
[operational_advice]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/client.go` around lines 745 - 750, Implement bounded preview-cache cleanup during startup by sweeping the directory returned by previewDir(), removing stale or excess cache files while preserving current entries needed by the application. Update the startup initialization path and reuse existing cache-size or age constants where available; keep cacheFile focused on atomically writing files.internal/terminalpet/model.go (1)
365-375: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
cropImage.Atdoes not clamp tobounds.
Atadds the offset and reads the source directly. Theimage.Imagecontract expects coordinates outsideBoundsto return the zero color. Here they sample the neighboring sprite cell instead.No current consumer reads outside
Bounds.png.Encodeiterates exactly overBounds. The gap is a latent trap for any future consumer that pads or over-reads.♻️ Proposed clamp
func (c cropImage) At(x, y int) color.Color { + if !image.Pt(x, y).In(c.bounds) { + return c.source.ColorModel().Convert(color.Alpha{}) + } return c.source.At(x+c.offset.X, y+c.offset.Y) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/model.go` around lines 365 - 375, Update cropImage.At to return the image package’s zero color whenever the requested coordinates fall outside c.bounds; only apply c.offset and sample c.source for coordinates within bounds. Preserve the existing ColorModel and Bounds behavior.internal/terminalpet/model_test.go (1)
9-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the looping-state case and the
ClickAnimationcycling case.Both tests are correct and the arithmetic checks out. Two documented contracts have no coverage:
PrimaryDurationreturns 0 whenloopStart == 0. The doc comment atinternal/terminalpet/model.golines 103-105 states this explicitly. Only the-1branch is tested.ClickAnimationcycles withindex % len(clickAnimations)and folds negative indices.internal/terminalpet/client_test.goline 131 checks index 0 only, so the wrap is untested.🧪 Proposed additional tests
func TestPrimaryDurationIsZeroForLoopingState(t *testing.T) { looping := State("looping-action") animation := &Animation{ durations: map[State][]time.Duration{looping: {100 * time.Millisecond, 200 * time.Millisecond}}, loopStarts: map[State]int{looping: 0}, } if got := animation.PrimaryDuration(looping); got != 0 { t.Fatalf("looping duration = %s, want 0", got) } if got := animation.PrimaryDuration(State("absent")); got != 0 { t.Fatalf("absent-state duration = %s, want 0", got) } } func TestClickAnimationCyclesAndFoldsNegativeIndex(t *testing.T) { first, second := State("first"), State("second") animation := &Animation{clickAnimations: []State{first, second}} for index, want := range map[int]State{0: first, 1: second, 2: first, 3: second, -1: second} { got, ok := animation.ClickAnimation(index) if !ok || got != want { t.Errorf("ClickAnimation(%d) = %q, %t; want %q, true", index, got, ok, want) } } empty := &Animation{} if got, ok := empty.ClickAnimation(0); ok || got != "" { t.Fatalf("empty ClickAnimation = %q, %t; want \"\", false", got, ok) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/model_test.go` around lines 9 - 38, Add coverage for the documented looping and cycling contracts: in the model tests, verify PrimaryDuration returns zero for a state with loopStarts set to 0 and for an absent state; add ClickAnimation tests covering repeated indices, negative-index folding, and an empty clickAnimations list returning no result. Use the existing Animation, PrimaryDuration, and ClickAnimation symbols without changing implementation behavior.internal/terminalpet/client_test.go (1)
248-278: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
rankingFailsis shared between the test goroutine and the server handler goroutine.Line 276 writes
rankingFailsfrom the test goroutine. Line 256 reads it from the handler goroutine. The write is ordered before the secondCatalogcall, so the common path is safe.One window is not ordered.
Cataloggives the ranking fetch a 1500 ms sub-context and callscancel()atinternal/terminalpet/client.goline 108. If that fetch times out, the handler goroutine from the firstCatalogcall can still be running when line 276 writes. The race detector can then report the access.The coding guidelines require affected concurrent code to run under the race detector. Use an atomic to remove the window.
♻️ Proposed fix
- var rankingFails bool + var rankingFails atomic.Bool var server *httptest.Server server = httptest.NewTLSServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { switch request.URL.Path { case "/manifest": manifest.AssetBase = server.URL _ = json.NewEncoder(writer).Encode(manifest) case "/ranking": - if rankingFails { + if rankingFails.Load() { http.Error(writer, "unavailable", http.StatusServiceUnavailable) return }- rankingFails = true + rankingFails.Store(true)Add
"sync/atomic"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminalpet/client_test.go` around lines 248 - 278, Replace the shared rankingFails boolean with a sync/atomic boolean in this test. Update the /ranking handler to atomically load its value and the test’s failure setup to atomically store it, eliminating the race between Catalog cancellation and the handler goroutine.Source: Coding guidelines
internal/config/writer.go (1)
594-647: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
SetPetround-trips the config throughmap[string]json.RawMessage, which both preserves unknown members and discards key order. The map choice is what makesSetPetsafer than its siblings and what makes it rewrite the whole file layout. Treat the two effects together when deciding the final shape.
internal/config/writer.go#L594-L647: extract the raw read-modify-write into a shared preference setter soSetTheme(line 587) andSetRecapsEnabled(line 565) stop dropping config members the Go structs do not model. Keep the extraction out of this PR if it widens the approved scope.internal/config/writer.go#L639-L642: decide whether alphabetical key reordering is acceptable. If it is, add a test that documents it. If it is not, replace the map with an ordered representation, which also removes the need for the sibling helper to reorder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/writer.go` around lines 594 - 647, Update internal/config/writer.go lines 594-647 by extracting SetPet’s raw read-modify-write logic into a shared preference setter used by SetTheme and SetRecapsEnabled, preserving unknown config members; if that exceeds the approved scope, leave the extraction out of this PR. At internal/config/writer.go lines 639-642, either document the existing alphabetical key reordering with a test or replace the map-based representation with an ordered one to preserve key order and eliminate sibling reordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/pet_writer_test.go`:
- Around line 10-93: Add regression tests covering all untested SetPet branches:
reject a blank path, reject malformed JSON, create and persist a config when the
file is missing (including nested directories), and verify a blank pet removes
the pet member and deletes an empty preferences object while preserving
unrelated fields.
In `@internal/config/writer.go`:
- Around line 639-642: Update the config serialization flow around
json.MarshalIndent in SetPet to preserve the original top-level key order
instead of encoding raw as a map that alphabetically sorts keys. Decode the
document into an ordered representation, update the pet member in place, and
retain existing formatting and error behavior.
In `@internal/terminalpet/client_test.go`:
- Around line 287-318: Add regression tests for the three untrusted-input
guards: verify decodeImage rejects dimensions beyond maxImageSide or
maxImagePixels, fetch/fetchLimit rejects response bodies exceeding the requested
byte limit, and resolveAssetURL rejects references outside allowed catalog
roots. Reuse existing test helpers and symbols such as encodedPNG, testClient,
maxImageSide, and maxImagePixels where available, and assert each failure path
returns an error.
In `@internal/tui/pet_output_test.go`:
- Around line 35-39: Check and fail the test immediately when
terminalpet.ThumbnailAnimation returns an error instead of discarding it. In
internal/tui/pet_output_test.go, apply this to the calls at lines 37, 71, 100,
119, 142, 189, 229, 264, 298, and 338; in
internal/terminalpet/image_renderer_test.go, apply it at lines 126 and 145,
using t.Fatal with the error details before passing animation to the renderer.
In `@internal/tui/pet_output.go`:
- Around line 96-104: Update the unsynchronized write path around
o.output.Write(value) to detect when written is less than len(value), even if
writeErr is nil, and convert that condition to io.ErrShortWrite before writing
imageUpdate. Match the short-write handling already used in the synchronized
path while preserving the existing error-return ordering.
- Around line 93-104: Update the write path around the existing
terminalSyncStart/terminalSyncEnd handling so values containing an unmatched
terminalSyncStart are treated as already inside a synchronized block: write the
value and append imageUpdate.Bytes() without emitting another terminalSyncStart
or terminalSyncEnd. Preserve the current marker-wrapping behavior for values
without an unmatched start.
In `@internal/tui/pets_test.go`:
- Around line 975-998: Increase the timeout in petCommandIncludesRaw to a
reliable budget below petFrameDelay (180 ms), so immediate raw commands are not
rejected under race-enabled or busy CI scheduling while genuinely delayed
animation ticks still time out.
In `@internal/tui/pets.go`:
- Around line 161-171: Guard m.picker before invoking picker.current() in
schedulePetPreview. If the picker is nil, clear the preview loading state and
slug and return without scheduling a command, matching the existing nil-picker
handling in startPetPreview and pickerMoved.
In `@internal/tui/run.go`:
- Around line 76-79: Update the cleanup error branch in the run flow around
petOutput.clearImage and the clearErr check so it still prints the diagnostic
but returns exit code 0, preserving successful command status after the chat
session completes.
- Around line 83-94: Update terminalPetFrameCache to prevent relative
UserConfigPath values from producing cache paths under the current working
directory. Resolve configPath to an absolute path before taking its directory,
or ignore non-absolute values and fall back to os.UserConfigDir(); preserve the
existing pets/frame-cache layout and empty-root behavior.
- Around line 45-46: Update terminalPetFrameCache to always resolve a usable
non-empty cache root when configured and user cache paths are unavailable, then
adjust petImageOutput.Write to swallow renderer errors and preserve the terminal
output flow so decorative pet rendering cannot fail the TUI.
---
Nitpick comments:
In `@internal/config/writer.go`:
- Around line 594-647: Update internal/config/writer.go lines 594-647 by
extracting SetPet’s raw read-modify-write logic into a shared preference setter
used by SetTheme and SetRecapsEnabled, preserving unknown config members; if
that exceeds the approved scope, leave the extraction out of this PR. At
internal/config/writer.go lines 639-642, either document the existing
alphabetical key reordering with a test or replace the map-based representation
with an ordered one to preserve key order and eliminate sibling reordering.
In `@internal/terminalpet/client_test.go`:
- Around line 248-278: Replace the shared rankingFails boolean with a
sync/atomic boolean in this test. Update the /ranking handler to atomically load
its value and the test’s failure setup to atomically store it, eliminating the
race between Catalog cancellation and the handler goroutine.
In `@internal/terminalpet/client.go`:
- Around line 487-493: Update the value closure’s wanted lookup to explicitly
check whether field exists in wanted before indexing the row. Return the
existing missing-column error when the manifest key is absent, while preserving
the bounds check and JSON unmarshalling for valid fields.
- Around line 319-332: Centralize the sprite-version validation currently
applied by Install and reuse it from LoadInstalled before assigning
document.SpriteVersion or calling document.atlasTracks. Preserve the rule that
only versions 1 and 2 are valid, and return the existing invalid-pet-metadata
error for unsupported versions instead of relying on atlasAnimation to reject
them.
- Around line 771-782: Update validateSlug to reject Windows reserved device
names, including con, prn, aux, nul, com1–com9, and lpt1–lpt9,
case-insensitively and when followed by a slug extension or suffix as applicable
to Windows path semantics. Preserve existing character and length validation,
and extend the validateSlug tests in client_test.go with representative
reserved-name cases.
- Around line 745-750: Implement bounded preview-cache cleanup during startup by
sweeping the directory returned by previewDir(), removing stale or excess cache
files while preserving current entries needed by the application. Update the
startup initialization path and reuse existing cache-size or age constants where
available; keep cacheFile focused on atomically writing files.
In `@internal/terminalpet/image_renderer_test.go`:
- Around line 142-167: Extend the image renderer tests with a local-file failure
case using NewImageRenderer and ImageProtocolKittyLocalFile without a cache
directory, asserting Render returns the “terminal pet image cache is
unavailable” error. Also add coverage for Zellij detection and malformed
TERM_PROGRAM_VERSION handling in the relevant terminal-detection tests,
preserving existing behavior for valid detection inputs.
In `@internal/terminalpet/image_renderer.go`:
- Around line 368-380: Make the zero-height precondition explicit in renderSixel
before calculating widthPixels: derive bounds and validate bounds.Dy() > 0
before using it as the divisor, while preserving the existing invalid-frame
error and scaling behavior.
- Around line 338-366: Remove the unreachable len(parts) == 0 condition from the
validation in dottedVersionAtLeast, keeping only the len(parts) > 3 upper-bound
check and preserving the remaining version parsing behavior.
- Around line 314-336: Update cachePNGFrame to enforce a bounded frame-cache
policy for the iTerm2 local-file protocol path, pruning cached PNGs by age or
total size before or after writing the new digest file. Preserve current
digest-based reuse and error propagation, and limit cleanup to the configured
terminal pet frame-cache directory.
- Around line 189-216: Update the default branch of the protocol switch in the
renderer’s frame-writing method to restore the previously saved cursor with the
matching restore sequence before returning. Leave the existing handling for
ImageProtocolKitty, ImageProtocolKittyLocalFile, and ImageProtocolSixel
unchanged.
In `@internal/terminalpet/model_test.go`:
- Around line 9-38: Add coverage for the documented looping and cycling
contracts: in the model tests, verify PrimaryDuration returns zero for a state
with loopStarts set to 0 and for an absent state; add ClickAnimation tests
covering repeated indices, negative-index folding, and an empty clickAnimations
list returning no result. Use the existing Animation, PrimaryDuration, and
ClickAnimation symbols without changing implementation behavior.
In `@internal/terminalpet/model.go`:
- Around line 365-375: Update cropImage.At to return the image package’s zero
color whenever the requested coordinates fall outside c.bounds; only apply
c.offset and sample c.source for coordinates within bounds. Preserve the
existing ColorModel and Bounds behavior.
In `@internal/terminalpet/sixel.go`:
- Around line 12-48: Add Sixel frame caching keyed by frameCacheKey and target
height, matching the existing Animation.pngCache pattern used by the Kitty
renderer. Update renderSixel and the Animation cache state so repeated renders
reuse encoded Sixel bytes instead of rescaling and calling encodeSixel each
tick; retain encodeSixel’s current quantization and ignore the noted narrowing
warnings.
- Around line 49-75: Update the Sixel emission loop in the band encoder to
run-length encode consecutive identical mask bytes using Sixel’s !<count> repeat
introducer, while preserving the existing color and band delimiters. Ensure each
run is emitted with the correct count and mask byte, including single-byte runs,
and retain the current output behavior when masks differ.
In `@internal/tui/pet_output_test.go`:
- Around line 44-50: Remove the redundant file.Seek call immediately before
os.ReadFile in the affected test. In the other offset-capture calls in this
file, replace the literal whence value 1 with io.SeekCurrent while preserving
their existing behavior.
- Around line 17-27: Extend the tests around petImageOutput with a concurrency
case that executes concurrent Write calls while clearImage runs, exercising the
mu synchronization between them. Ensure the test is race-detector compatible and
validate the affected package with go test -race.
In `@internal/tui/run.go`:
- Around line 83-94: Add a table-driven test for terminalPetFrameCache in
run_test.go covering absolute, relative, and whitespace-only UserConfigPath
values plus the os.UserConfigDir fallback. Build expected paths with
filepath.Join and canonicalize paths before comparison, while preserving
platform-independent assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f5aa8d8-aa18-42ff-b717-872a4f5b3796
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
go.modinternal/cli/app.gointernal/config/pet_writer_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/config/writer.gointernal/terminalpet/client.gointernal/terminalpet/client_test.gointernal/terminalpet/image_renderer.gointernal/terminalpet/image_renderer_test.gointernal/terminalpet/model.gointernal/terminalpet/model_test.gointernal/terminalpet/sixel.gointernal/tui/clipboard.gointernal/tui/commands.gointernal/tui/modal_selection.gointernal/tui/model.gointernal/tui/mouse.gointernal/tui/mouse_filter.gointernal/tui/mouse_filter_test.gointernal/tui/options.gointernal/tui/pet_output.gointernal/tui/pet_output_test.gointernal/tui/pets.gointernal/tui/pets_test.gointernal/tui/picker.gointernal/tui/run.gointernal/tui/sidebar_test.gointernal/tui/view.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/pet_writer_test.go`:
- Around line 67-73: Strengthen the persistence assertions in the relevant test
by unmarshalling root["preferences"] and root["future"] into structured values,
then compare the specific pet and enabled fields for exact equality. Replace
substring checks that can match values such as "boba-extra" or nested enabled
fields, while preserving the existing failure reporting and regression coverage.
In `@internal/tui/run_test.go`:
- Around line 28-53: Add regression cases around terminalPetFrameCacheWith for
providers returning an empty string with nil error: verify a blank userConfigDir
falls back to cacheRoot, and verify a blank userCacheDir produces an empty
result. Keep the existing provider-error cases unchanged and use the same
canonical path comparisons and failure assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ede0a90c-2972-44a1-92fb-92159611a193
📒 Files selected for processing (9)
internal/config/pet_writer_test.gointernal/terminalpet/client_test.gointernal/terminalpet/image_renderer_test.gointernal/tui/pet_output.gointernal/tui/pet_output_test.gointernal/tui/pets.gointernal/tui/pets_test.gointernal/tui/run.gointernal/tui/run_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tui/pet_output.go
- internal/tui/run.go
- internal/tui/pets.go
- internal/terminalpet/client_test.go
|
@coderabbitai re-review the latest commit and approve if everything looks good. All requested changes are addressed and checks are green. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Pull request metadata =='
gh pr view 887 --repo Gitlawb/zero --json number,headRefOid,baseRefOid,latestReviews,statusCheckRollup --jq '{
number,
headRefOid,
baseRefOid,
latestReviews: [.latestReviews[] | {author: .author.login, state, submittedAt, body}],
checks: [.statusCheckRollup[] | {name: .name, status: .status, conclusion: .conclusion}]
}'
printf '%s\n' '== Latest commit =='
git show --no-ext-diff --format='commit %H%nparent %P%nsubject %s%n' --stat HEAD
printf '%s\n' '== Latest commit file list =='
git diff-tree --no-commit-id --name-status -r HEAD
printf '%s\n' '== CodeRabbit review comments and unresolved threads =='
gh api graphql -F owner='Gitlawb' -F name='zero' -F number=887 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 20) {
nodes {
author { login }
body
createdAt
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]?; .author.login == "coderabbitai"))
| {isResolved, path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body, createdAt}]}'
printf '%s\n' '== AST outline of latest changed Go files =='
git diff-tree --no-commit-id --name-only -r HEAD -- '*.go' |
while IFS= read -r f; do
[ -n "$f" ] || continue
printf '\n-- %s --\n' "$f"
ast-grep outline "$f" --items all || true
done
printf '%s\n' '== Latest patch =='
git diff --no-ext-diff --unified=60 HEAD^ HEAD -- '*.go' 'go.mod' 'go.sum'Length of output: 56441 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Writer and cache-helper outlines =='
ast-grep outline internal/config/writer.go --items all
ast-grep outline internal/tui/run.go --items all
printf '%s\n' '== SetPet implementation and surrounding write helpers =='
sed -n '560,680p' internal/config/writer.go
printf '%s\n' '== Frame-cache implementation =='
sed -n '60,165p' internal/tui/run.go
printf '%s\n' '== Complete latest test functions =='
sed -n '1,175p' internal/config/pet_writer_test.go
sed -n '1,110p' internal/tui/run_test.go
printf '%s\n' '== SetPet call sites and config serialization conventions =='
rg -n -C 4 '\bSetPet\s*\(' --type go .
rg -n -C 3 'json\.MarshalIndent\(' --type go internal
printf '%s\n' '== Full PR changes to config writer and associated tests =='
git diff --no-ext-diff --unified=35 2458e0c67b49a2e959e4ec068f1f2ad382cb2406 HEAD -- internal/config/writer.go internal/config/pet_writer_test.go internal/tui/run.go internal/tui/run_test.goLength of output: 67466
I cannot approve yet. One valid CodeRabbit thread remains unresolved.
Please preserve the original JSON member order, or explicitly document and test the normalization behavior. You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/json_object_edit.go`:
- Around line 261-265: Update insertJSONMember’s empty-object branch to preserve
the existing trailing newline and indentation when object contains multiline
whitespace, keeping the closing brace on its original line while inserting the
entry. Add a byte-for-byte regression test through SetPet using a multiline
empty preferences object and verify the resulting JSON formatting.
- Around line 46-48: Update the null-handling branch in SetPet to check for an
earlier duplicate preferences member using the same protection logic as the
existing lines 90-94 before calling removeJSONMember; retain the final
preferences member when such a duplicate exists. Add a regression test that runs
SetPet(path, "") on duplicate preferences input, reloads the file, and verifies
the earlier pet value is not reactivated.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b3b3e59c-2c76-420a-b861-df68e7de5394
📒 Files selected for processing (3)
internal/config/json_object_edit.gointernal/config/pet_writer_test.gointernal/config/writer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/writer.go
|
@coderabbitai all requested changes are resolved, all review threads are closed, and every check is green. Please review the latest commit and update your formal review to approve |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Gitlawb/zero"
pr="887"
printf '%s\n' '== Pull request state and checks =='
gh pr view "$pr" --repo "$repo" \
--json number,headRefOid,baseRefOid,latestReviews,statusCheckRollup \
--jq '{
number,
headRefOid,
baseRefOid,
latestReviews: [.latestReviews[] | {author: .author.login, state, submittedAt}],
checks: [.statusCheckRollup[] | {name: .name, status: .status, conclusion: .conclusion}]
}'
printf '%s\n' '== CodeRabbit review threads =='
gh api graphql -F owner='Gitlawb' -F name='zero' -F number="$pr" -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 20) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]?; .author.login == "coderabbitai"))
| {isResolved, path, line, originalLine,
comments: [.comments.nodes[] | {author: .author.login, body, createdAt}]}'
printf '%s\n' '== Latest commit =='
git show --no-ext-diff --format='commit %H%nparent %P%nsubject %s%n' --stat HEAD
printf '%s\n' '== Files in latest commit =='
git diff-tree --no-commit-id --name-status -r HEAD
printf '%s\n' '== Relevant source outlines =='
ast-grep outline internal/config/writer.go --items all
ast-grep outline internal/config/json_object_edit.go --items all
ast-grep outline internal/config/pet_writer_test.go --items all
printf '%s\n' '== SetPet and JSON-preserving editor =='
rg -n -C 8 '\bSetPet\s*\(|\bupdatePreferencesPet\s*\(|\bedit.*[Pp]et\b' \
internal/config/writer.go internal/config/json_object_edit.go
printf '%s\n' '== JSON editor implementation =='
cat -n internal/config/json_object_edit.go
printf '%s\n' '== Pet writer tests =='
cat -n internal/config/pet_writer_test.go
printf '%s\n' '== Latest patch for the fix =='
git diff --no-ext-diff --unified=45 HEAD^ HEAD -- \
internal/config/writer.go internal/config/json_object_edit.go internal/config/pet_writer_test.goLength of output: 85069 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Run flow and cache-root implementation =='
sed -n '1,175p' internal/tui/run.go
printf '%s\n' '== Pet image output implementation =='
ast-grep outline internal/tui/pet_output.go --items all
cat -n internal/tui/pet_output.go
printf '%s\n' '== Cache-root and render-failure tests =='
rg -n -C 12 \
'terminalPetFrameCache|terminalPetFrameCacheWith|renderer.*[Ff]ail|Render.*[Ee]rror|KeepsTextAlive|cache is unavailable' \
internal/tui/run_test.go internal/tui/pet_output_test.go internal/terminalpet/image_renderer.go
printf '%s\n' '== Renderer cache behavior =='
sed -n '285,345p' internal/terminalpet/image_renderer.go
printf '%s\n' '== Relevant changes since PR base =='
git diff --no-ext-diff --unified=30 \
2458e0c67b49a2e959e4ec068f1f2ad382cb2406 HEAD -- \
internal/tui/run.go internal/tui/pet_output.go internal/tui/run_test.go internal/tui/pet_output_test.goLength of output: 64434
The remaining unresolved rendering thread is no longer valid. All named checks succeeded. I approve this pull request. [approve] You are interacting with an AI system. |
On Windows Terminal the companion draws a filled black rectangle over the interface, and dragging it mangles text under and around it. It looks like a platform quirk. It is an untested code path that only Windows reaches. Sixel is the only protocol here that erases by PAINTING CHARACTERS. Kitty hands the terminal an image id and the placement goes away. A sixel is already in the cell grid, so clearRenderedImage writes literal spaces over the cells it occupied. Those spaces are ordinary characters and carry whatever SGR the TUI last set, and nothing normalised it, so every erase stamped a filled block in the ambient background. A drag erases and repaints on every pixel, so the block was continuously refreshed over the UI. It stayed invisible everywhere else because ghostty, kitty, WezTerm and iTerm all resolve to the Kitty protocol and delete by id, never reaching this code. Windows Terminal is the only mainstream terminal that lands on the sixel branch, which is why testing on Linux and macOS could not have caught it. The erase now resets SGR before painting, and uses DECSC/DECRC rather than CSI s/u because only DECSC saves the attributes this clobbers. Restoring them matters: the erase is emitted mid-frame, between styled writes the TUI has already begun. Measured before and after rather than reasoned about. Before: \x1b[s\x1b[3;41H \x1b[4;41H \x1b[5;41H \x1b[u no reset anywhere. After, the reset leads and DECRC closes. Tests assert the bytes, since bytes are what the terminal sees: SGR is reset BEFORE the first row is painted rather than merely present somewhere, the erase still covers every claimed cell so the fix cannot be satisfied by erasing nothing, and the Kitty path still deletes by id rather than acquiring the character-painting erase. Verified by mutation: restoring the old sequence fails the first of those with the exact reported symptom. Deliberately not fixed here, since both are design calls rather than defects: the erase rectangle is computed from the app's own cell maths rather than the terminal's real footprint, and the sixel path has no in-place move, so every drag pixel is a full erase and repaint where Kitty gets a cheap reposition.
|
@anandh8x tested this on Windows Terminal 1.24 and hit the black rectangle plus mangled text under the companion. Pushed a fix for the cause, It is not a Windows quirk. Sixel is the only protocol here that erases by painting characters. Kitty hands the terminal an image id and the placement goes away; a sixel is already in the cell grid, so That is also why Linux and macOS came back clean. ghostty, kitty, WezTerm and iTerm all resolve to Measured rather than reasoned about. Before: No reset anywhere. The fix leads with one and switches to DECSC/DECRC, because only DECSC saves the attributes this now clobbers, and the erase is emitted mid-frame between styled writes the TUI has already started. Tests assert the bytes, since bytes are what the terminal sees: that SGR is reset BEFORE the first row is painted rather than merely present somewhere, that the erase still covers every claimed cell so it cannot be satisfied by erasing nothing, and that the Kitty path keeps deleting by id rather than inheriting the character-painting erase. Verified by mutation, restoring the old sequence fails the first with the exact reported symptom. I renamed my test file mid-write, worth mentioning because the repo has a guard for it now: I first called it Two more I found and deliberately did NOT fixBoth are design calls rather than defects, so they are yours. The erase rectangle is the app's belief, not the terminal's. It erases Sixel has no in-place move. One unrelated thing while I was in here. Also worth a word from you before this lands: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/terminalpet/image_renderer.go`:
- Around line 305-313: In the function containing the DECSC/SGR write and
row-clearing loop, defer the DECRC restore immediately after the initial write
succeeds so every subsequent error path restores terminal state. Preserve any
primary row-write error, but return the deferred restore error when no earlier
error occurred.
In `@internal/terminalpet/sixel_erase_test.go`:
- Around line 57-61: Update the row-validation loop in the sixel erase test to
compute the expected cursor sequence using key.y + row + 1 and key.x + 1 for
each row, then assert that sequence is present in got. Remove the row == 0-only
condition while preserving the existing failure diagnostics and row coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dff45c7d-d3e5-4d2b-b254-47ef6aeaf30c
📒 Files selected for processing (2)
internal/terminalpet/image_renderer.gointernal/terminalpet/sixel_erase_test.go
…painted The companion blanks a row and a column of live interface on Windows Terminal. The cause is one predicate. petPixelProtocolSupported gates two unrelated questions that only looked like one: whether pixel-precise dragging is available, and whether to ask the terminal for its cell size with CSI 16 t. It answers true for Kitty only, so on a sixel terminal the request was never sent, the reply never arrived, and petCellPixelWidth/Height stayed zero for the whole session. Everything downstream then took its fallback. petImageHeightPixels returned its blind preferredHeight of 75 pixels instead of the cell-aware branch below it, and the erase in clearRenderedImage used petImageColumns and petImageRows. Those constants describe the RESERVED area, not what was painted. Windows Terminal reports a 20x10 cell, so a 75-pixel sprite covers 4 rows and about 8 columns while the erase blanked 5 by 9, taking a row and a column of interface with it on every move. A drag repaints per pixel, so it did that continuously. Windows Terminal answers CSI 16 t, confirmed by hand: it replies ESC [6;20;10t. This was never a question of terminal support. Nothing asked. So the request now goes out for every image protocol, via a predicate separate from the dragging one, and the sixel footprint is computed from the rendered pixel height and the reported cell size rather than from the constants. It is still clamped to the reserved area, because the layout keeps that many cells clear and the drag clamps to it, so a larger computed footprint would mean erasing into live interface. Deliberately NOT widening petPixelProtocolSupported itself: that would enable pixel-precise dragging on sixel as a side effect, which is a separate feature decision rather than part of this defect. Kitty is untouched and a test pins that. There Columns and Rows are the placement REQUEST handed to the terminal, which owns the region and scales into it, so recomputing them would resize the image rather than fix an erase. Tests use the real 20x10 measurement: the sixel footprint is 4 rows not 5, Kitty keeps its requested placement, the fallback without metrics stays the reserved area rather than collapsing to zero and erasing nothing, and every image protocol asks for the measurement.
|
@anandh8x follow-up on the Windows report. I pushed two fixes that are correct and did not solve it, and instrumenting a build finally showed why. The remaining cause is a layer below both, and it is a design call rather than a defect, so I am leaving it with you. What the two commits did fix
Both confirmed live. Trace from a real Windows Terminal session: Metrics arrive, footprint corrects from 9x5 to 7x4, and the raster header proves it exact: 69px wide over a 10px cell is 6.9 so 7 columns, 75px tall over a 20px cell is 3.75 so 4 rows. Windows Terminal answers What is actually breaking the UIThe geometry is now provably right and the damage is unchanged. So it was never the geometry. The sixel is written out of band, straight into the cell grid, behind Bubble Tea's differential renderer. Bubble Tea keeps a model of what every cell holds and rewrites only the cells it believes changed. The image destroys text in cells it still believes contain that text, so it never repaints them and the characters do not come back. That matches the symptom precisely. The reports are missing characters and broken borders, not mis-coloured ones: Three things corroborate it:
Your callTwo routes and I do not think it is mine to pick:
Two smaller things from the same trace, yours to weigh: The first ~14 draws run at Sixel has no in-place move. I deliberately did not widen |
|
@Vasanthdev2004 I pushed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/terminalpet/sixel_erase_test.go`:
- Around line 60-63: Strengthen the erase validation in the test loop around the
position and erase values: verify that each cursor sequence is followed by
exactly key.columns spaces, rather than using strings.Contains, so longer erase
runs are rejected while the expected clearRenderedImage output remains accepted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bd80219-98ee-47ad-9db7-5bc04a35f991
📒 Files selected for processing (4)
internal/terminalpet/image_renderer.gointernal/terminalpet/sixel_erase_test.gointernal/tui/pets.gointernal/tui/pets_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/terminalpet/image_renderer.go
- internal/tui/pets.go
- internal/tui/pets_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving d195972d. Tested on Windows Terminal 1.24 throughout, which is the platform this had trouble on.
The dock is the right call. A sixel lives in the cell grid, so wherever it is dragged it destroys text Bubble Tea believes is still present and will not repaint. Kitty images are terminal-owned overlays, so the grid is never touched and free dragging costs nothing. Pinning sixel to a reserved dock removes the overlap rather than trying to repair it afterwards, and gating on petSupportsAlphaOverlay keeps the full behaviour where it is actually safe. The alternative, repainting the damaged region after every move, would have meant a repaint per drag pixel and visible flicker for a worse result.
What I checked rather than assumed, since this fetches and decodes third-party images:
- Host allowlisting is two-tier and the narrower one is the right way round: catalog redirects may land on
petdex.dev, while image and metadata downloads are pinned to the asset host. - Redirects are handled properly on both sides.
CheckRedirectvalidates every hop against the appropriate allowlist with a hop cap, and the finalresponse.Request.URLis re-validated afterwards. That second check is what stops a redirect chain ending somewhere the per-hop check let through, and it is a mistake I have seen made in this repo before. - Downloads are bounded: 2 MB manifest, 16 MB sprite, enforced with
io.LimitReader(body, limit+1)rather than trusting Content-Length, plus a 20 second client timeout. - CI is 8/8 including CodeQL and
Analyze (actions).
Two things for @kevincodex1 rather than blockers.
go.mod gains golang.org/x/image as a new direct dependency and promotes ultraviolet from indirect. CONTRIBUTING asks for prior justification on dependency changes. The choice looks right to me, it is the Go team's own webp decoder and there is no reasonable alternative for decoding sprite sheets, but the policy call is yours.
Windows users now get a stationary companion where Kitty terminals get a draggable one. That follows from the protocol rather than from anything avoidable, and I would rather ship it docked than shipping it breaking the interface, but it is a platform-parity decision worth making knowingly on a PR titled "interactive terminal companions".
Scope of this approval. I reviewed the rendering and network paths closely and verified the Windows behaviour by hand. I did not re-read all 31 files line by line. The two fixes I pushed earlier, 0c4a1f2c for the erase painting in the caller's background and cbb4a64f for the cell-size request being gated to Kitty only, are both still present and unaffected by the dock change.
Good work chasing this down across three platforms with only one of them able to reproduce it.
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
3 participants
Summary
/petscompanion discovery, preview, installation, selection, and disable controlsWhy
Terminal companions add an optional, expressive layer to the Zero interface without affecting users who leave the feature disabled. The implementation supports terminal-native image protocols, persists the selected companion, and keeps the experience usable across compact layouts and active agent workflows.
Validation
make fmt-checkgo vet ./...go test -race ./internal/config ./internal/terminalpet ./internal/tuigo test ./...GOFLAGS=-buildvcs=false go run ./cmd/zero-release buildGOFLAGS=-buildvcs=false go run ./cmd/zero-release smokemake lint-staticmake vulncheckgit diff HEAD --checkSummary by CodeRabbit
New Features
/petsand/petcommands for selecting or hiding companions.Bug Fixes