fix(#1073): Tech debt fix

[Simpaqt]

Should clear the issues in 1073

Summary by CodeRabbit

• Bug Fixes

  • Image previews no longer fail due to a strict dimension pre-check; rendering and fallback behavior unchanged.

• Chores

  • Fixed a typo in a debug log and adjusted comments/formatting for clarity.

• Tests

  • Internal test utilities simplified and many tests updated to use the streamlined test helper.

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

Removed an early positive-dimension pre-check from ImagePreviewWithRenderer; minor comment/formatting edits in kitty helpers; fixed a tmux testsuite log typo; simplified test utilities by changing TeaUpdate signature and replacing TeaUpdateWithErrCheck call sites across many tests. No exported API changes.

Changes

Cohort / File(s)	Summary
Image preview flow src/pkg/file_preview/image_preview.go	Removed the early maxWidth > 0 / maxHeight > 0 validation in ImagePreviewWithRenderer; remaining stat/read/prepare and Kitty/ANSI rendering (with fallback) unchanged.
Kitty helper edits src/pkg/file_preview/kitty.go	Minor comment change in generateKittyClearCommands and reflow of renderWithKittyUsingTermCap parameter list; no behavioral or signature changes.
Tmux testsuite logging fix testsuite/core/tmux_manager.py	Fixed debug log message typo in TmuxSPFManager._send_key ("sendind" → "sending").
Test utilities refactor src/internal/test_utils.go	TeaUpdate signature changed from (tea.Cmd, error) → tea.Cmd; removed TeaUpdateWithErrCheck. Call sites updated to use new TeaUpdate.
Unit tests (call-site updates) src/internal/handle_file_operation_test.go, src/internal/model_file_operations_test.go, src/internal/model_navigation_test.go, src/internal/model_prompt_test.go, src/internal/model_test.go, ...	Replaced TeaUpdateWithErrCheck(...) calls with TeaUpdate(...) across many tests; call semantics adjusted to single-return tea.Cmd (error checking removed).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant ImagePreview as ImagePreviewWithRenderer
  participant FS as FileSystem
  participant Renderer

  Caller->>ImagePreview: Render(path, maxWidth, maxHeight, renderer)
  Note over ImagePreview: Early positive-dimension validation removed
  ImagePreview->>FS: stat(path)
  FS-->>ImagePreview: file info
  ImagePreview->>FS: read file bytes
  FS-->>ImagePreview: bytes
  ImagePreview->>ImagePreview: prepare/validate image size & decode
  alt Terminal supports Kitty/ANSI
    ImagePreview->>Renderer: render via Kitty/ANSI (with fallback)
    Renderer-->>ImagePreview: render result
  else Fallback
    ImagePreview->>Renderer: fallback render
    Renderer-->>ImagePreview: result
  end
  ImagePreview-->>Caller: status/result

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Code tech debt and existing code issues #1073 — Matches changes to image preview, kitty helper, tmux log, and test-utils refactor described in this PR.

Possibly related PRs

• Model unit tests #803 — Changes the test utilities and call sites (TeaUpdate / TeaUpdateWithErrCheck) similar to this PR.
• feat: File operation via tea cmd #963 — Modifies src/internal/test_utils.go signature and removes TeaUpdateWithErrCheck, matching this PR.
• feat: enable lll and recvcheck linter, fix tests, more refactors #977 — Edits ImagePreviewWithRenderer (dimension-check related) overlapping with this PR.

Suggested labels

pr needs work

Suggested reviewers

• yorukot

Poem

I nibbled at checks and let pixels roam,
Tweaked a comment and fixed a log at home.
Tests now call simply, no extra fuss,
Little rabbit hops — previews for us. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title Check	❓ Inconclusive	The PR title "fix(#1073): Tech debt fix" is too generic and does not convey the primary change; the diff removes an early image-dimension validation in src/pkg/file_preview/image_preview.go and also includes minor formatting/comment and logging-typo fixes, so a reviewer cannot tell the main intent from the title alone.	Please update the title to clearly state the main change, for example: "Remove early image-dimension validation in image preview (fix #1073)" or "Fix image-preview validation and minor formatting/typo fixes (fix #1073)".

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e2cb86 and a4c9b1f.

📒 Files selected for processing (6)

• src/internal/handle_file_operation_test.go (1 hunks)
• src/internal/model_file_operations_test.go (1 hunks)
• src/internal/model_navigation_test.go (2 hunks)
• src/internal/model_prompt_test.go (6 hunks)
• src/internal/model_test.go (7 hunks)
• src/internal/test_utils.go (4 hunks)

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[lazysegtree]

Can you also apply do go fmt ./... to solve these build errors.

[Simpaqt]

Can you also apply do go fmt ./... to solve these build errors.

Yeah I'll do that 👍

[lazysegtree]

I will check the lint failures and merge it this weekend

[Simpaqt]

Alright 👍

[codescene-delta-analysis]

Code Health Improved (1 files improve in Code Health)

Gates Passed
3 Quality Gates Passed

See analysis details in CodeScene

View Improvements

File	Code Health Impact	Categories Improved
image_preview.go	9.10 → 9.39	Complex Method

Quality Gate Profile: The Bare Minimum
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/pkg/file_preview/kitty.go (1)

24-41: Update terminal-capability heuristic — remove iTerm2; mark Konsole partial; keep kitty/WezTerm/Warp with caveats

• Remove "iTerm2" from knownTerminals — iTerm2 uses OSC 1337/File, not the Kitty graphics protocol (incompatible).
• Treat "Konsole" as partial support — it implements a subset of Kitty graphics; enable only via runtime probe or explicit opt-in.
• Keep "kitty" and "xterm-kitty" (reference). Retain "WezTerm" (substantial but not identical support) and "WarpTerminal" (full support added May 14, 2025) but document caveats.
• Action: either restrict the heuristic to full/native implementers or add a runtime capability probe / user opt-in. Location: src/pkg/file_preview/kitty.go:24-41.

🧹 Nitpick comments (3)

src/pkg/file_preview/kitty.go (3)

114-122: Guard against zero cell size to avoid div-by-zero/NaN sizing.

If PixelsPerRow/Column come back 0 (cap not detected), the ratio math can produce Inf/NaN and 0/huge dims later.

Apply this minimal guard:

   cellSize := p.terminalCap.GetTerminalCellSize()
-  pixelsPerColumn := cellSize.PixelsPerColumn
-  pixelsPerRow := cellSize.PixelsPerRow
+  pixelsPerColumn := cellSize.PixelsPerColumn
+  pixelsPerRow := cellSize.PixelsPerRow
+  if pixelsPerColumn <= 0 || pixelsPerRow <= 0 {
+    slog.Debug("invalid terminal cell size; falling back to 1x1",
+      "pixelsPerColumn", pixelsPerColumn, "pixelsPerRow", pixelsPerRow)
+    if pixelsPerColumn <= 0 {
+      pixelsPerColumn = 1
+    }
+    if pixelsPerRow <= 0 {
+      pixelsPerRow = 1
+    }
+  }

---

125-135: Clamp computed rows/cols to at least 1.

Extreme aspect ratios can round down to 0 and yield empty output.

   if imgRatio > termRatio {
     dstCols := maxWidth
     dstRows := int(float64(dstCols*pixelsPerColumn) / imgRatio / float64(pixelsPerRow))
+    if dstCols < 1 { dstCols = 1 }
+    if dstRows < 1 { dstRows = 1 }
     opts.DstCols = uint32(dstCols)
     opts.DstRows = uint32(dstRows)
   } else {
     dstRows := maxHeight
     dstCols := int(float64(dstRows*pixelsPerRow) * imgRatio / float64(pixelsPerColumn))
+    if dstRows < 1 { dstRows = 1 }
+    if dstCols < 1 { dstCols = 1 }
     opts.DstRows = uint32(dstRows)
     opts.DstCols = uint32(dstCols)
   }

---

55-62: Deduplicate ClearKittyImages implementations.

The method repeats the capability check/logic; call the package func instead.

 func (p *ImagePreviewer) ClearKittyImages() string {
-  if !p.IsKittyCapable() {
-    return "" // No need to clear if terminal doesn't support Kitty protocol
-  }
-
-  return generateKittyClearCommands()
+  return ClearKittyImages()
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8b9e6d and 6e2cb86.

📒 Files selected for processing (2)

• src/pkg/file_preview/image_preview.go (0 hunks)
• src/pkg/file_preview/kitty.go (2 hunks)

💤 Files with no reviewable changes (1)

• src/pkg/file_preview/image_preview.go

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.

Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.

Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.

Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.

Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.

Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.

Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.

📚 Learning: 2025-09-14T05:34:28.184Z

Learnt from: lazysegtree
PR: yorukot/superfile#841
File: src/pkg/file_preview/kitty.go:78-79
Timestamp: 2025-09-14T05:34:28.184Z
Learning: lazysegtree correctly identified that `\x1b[0m` is an ANSI reset sequence that resets text formatting to default, not a delay command as incorrectly commented in the Kitty graphics protocol clearing code in src/pkg/file_preview/kitty.go.

Applied to files:

• src/pkg/file_preview/kitty.go

🔇 Additional comments (2)

src/pkg/file_preview/kitty.go (2)

76-77: Correct comment: ESC[0m resets formatting.

Nice fix; this aligns with prior feedback that it’s not a delay escape.

---

97-98: Signature reflow + trailing comma are fine.

No behavior change; this is gofmt‑friendly for multi‑line params.