Skip to content

fix(diffviewer): wrap long lines through delta in side-by-side, mark clipped lines in unified (closes #99)#133

Open
Booyaka101 wants to merge 3 commits into
dlvhdr:mainfrom
Booyaka101:fix/long-line-wrap-99
Open

fix(diffviewer): wrap long lines through delta in side-by-side, mark clipped lines in unified (closes #99)#133
Booyaka101 wants to merge 3 commits into
dlvhdr:mainfrom
Booyaka101:fix/long-line-wrap-99

Conversation

@Booyaka101
Copy link
Copy Markdown
Contributor

@Booyaka101 Booyaka101 commented May 9, 2026

Summary

Closes #99.

diffFile and diffDir invoke delta with --max-line-length=<width>, which is the truncation flag — not a wrap hint. Combined with delta's default --wrap-max-lines=2, any source line longer than roughly two viewport widths got silently dropped in side-by-side mode. In unified mode (where delta does not wrap at all) the long line then hit the post-processing truncation in the diffContentMsg handler, which used an empty tail ("") and clipped without any visible signal.

The reporter pointed at both spots in the original issue — both the delta args and the post-processing pass.

Fix

Two changes in pkg/ui/panes/diffviewer/diffviewer.go, plus horizontal scrolling added per review feedback:

  1. diffFile and diffDir — replace --max-line-length=<width> with --max-line-length=0 (disable truncation) and add --wrap-max-lines=unlimited. Side-by-side mode now wraps long lines all the way through instead of stopping at the second wrap.

  2. diffContentMsg handler — store the raw delta output, render the visible window on every change. The cache now holds raw text, so cache hits re-render through the same path.

  3. Horizontal scrolling (added in response to fix(diffviewer): wrap long lines through delta in side-by-side, mark clipped lines in unified (closes #99) #133 (comment)):

    • Left / right arrow keys scroll the diff viewport horizontally at half-viewport step. h/l are taken by file-tree expand/collapse, so arrows are the only sensible binding.
    • xOffset resets to 0 whenever a new file or dir is loaded.
    • The markers are conditional: right when lineWidth > xOffset + viewportWidth, left when xOffset > 0 && lineWidth > xOffset. A line shorter than the viewport never shows either marker.

Verification

Local repro with the canonical "very long line" diff at -w=80:

Mode Pre-fix Post-fix
Side-by-side clipped at "what flags delta is invoked with" (≈2 wraps) reaches "to see what happens here" (full content)
Unified line silently cut at viewport width with no marker line cut with tail; right-arrow scrolls to reveal the rest, appears on the left once scrolled

Verified delta args directly:

$ delta --paging=never -w=80 --max-line-length=0 --wrap-max-lines=unlimited --side-by-side < long.diff
…
│    │                                  │  2 │const Long = "this is a very long↵
│    │                                  │    │ line that should be wrapped or t↵
│    │                                  │    │runcated depending on what flags ↵
│    │                                  │    │delta is invoked with and we want↵
│    │                                  │    │ to see what happens here"
…

Existing pkg/ui/panes/diffviewer test suite passes:

ok  	github.com/dlvhdr/diffnav/pkg/ui/panes/diffviewer	0.366s

The pre-existing TestSearchResultsRenderWhenFileTreeIsHidden, TestBuildFullFileTree, and TestCollapseTree failures on main are unrelated lipgloss-styling drift and are not touched by this PR.

AI usage disclosure (per AI_POLICY.md)

Tool: Claude Code (Opus 4.7).
Extent: pattern analysis (delta flag semantics, charmbracelet ansi.Cut usage), code drafting for the offset / render-window logic, and commit-message wording. I reviewed every change manually, ran go build, go vet, go test ./pkg/ui/panes/diffviewer/..., and confirmed the long-line repro behaviour locally before pushing. Happy to walk through any line on request.

Credit to @fgrehm for the report and the precise line references in the issue.

…clipped lines in unified (closes dlvhdr#99)

`diffFile` and `diffDir` were invoking delta with `--max-line-length=<width>`,
which is the truncation flag rather than a wrap hint. Combined with delta's
default `--wrap-max-lines=2`, that capped any source line at roughly two
viewport widths in side-by-side mode and silently dropped the rest. In
unified mode, where delta does not wrap, the long line then hit the post-
processing truncation in the `diffContentMsg` handler, which used an empty
tail and clipped without any visible signal.

This commit:

- Replaces `--max-line-length=<width>` with `--max-line-length=0` and adds
  `--wrap-max-lines=unlimited` for both invocations. Side-by-side now wraps
  long lines all the way to the end instead of truncating after two wraps.
  Verified locally: a 162-char line at `-w=80` previously stopped at
  "what flags delta is invoked with"; it now reaches "to see what happens
  here".
- Replaces the empty truncation tail in the `diffContentMsg` handler with
  `…`, so unified-mode lines that exceed the viewport at least signal the
  cut-off rather than vanishing silently.

The existing `diffviewer` test suite still passes. Pre-existing
`TestSearchResultsRenderWhenFileTreeIsHidden`, `TestBuildFullFileTree`, and
`TestCollapseTree` failures on `main` are unrelated to this change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread pkg/ui/panes/diffviewer/diffviewer.go Outdated
for i, line := range lines {
if lipgloss.Width(line) > m.vp.Width() && m.vp.Width() > 0 {
lines[i] = ansi.Truncate(line, m.vp.Width(), "")
lines[i] = ansi.Truncate(line, m.vp.Width(), "")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would like to support horizontal scrolling. I think the ... is a nice addition to indicate you can scroll right but I would like to extend it in 2 ways:

  1. Only show it on the right when the line's visible length is greater than the viewport's width
  2. Do the same behavior for the left side

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Plan: add an xOffset to the diff viewport, bind left/right arrows at half-viewport step, and gate the markers on actual content past each edge (right when lineWidth > xOffset + viewportWidth, left when xOffset > 0). Will note AI assistance in the PR description per AI_POLICY.

…psis markers

Per dlvhdr#133 review: maintainer asked for horizontal scrolling instead of
silent clipping, with the ellipsis markers gated on actual visible-vs-
viewport content (and mirrored on the left side).

Changes:
- pkg/ui/keys.go: add ScrollLeft/ScrollRight bindings on left/right
  arrows (h/l are taken by file-tree expand/collapse).
- pkg/ui/tui.go: forward arrow keys to diffViewer when the diff pane
  is active.
- pkg/ui/panes/diffviewer: store the raw delta output and re-render
  the visible window on offset change. Cache now holds raw text so
  cache hits re-render through the same path. xOffset resets when a
  new file or dir is loaded.

Ellipsis behaviour:
- Right marker: only when lineWidth > xOffset + viewportWidth.
- Left marker: only when xOffset > 0 AND lineWidth > xOffset.
- Slice budget is reduced by 1 cell per active marker so the visible
  content + markers fit the viewport.
@dlvhdr
Copy link
Copy Markdown
Owner

dlvhdr commented May 16, 2026

@Booyaka101 charm.land/bubbles/v2/viewport already implements a ScrollRight and ScrollLeft methods so let's use those. I'm not sure if it supports showing overflow characters like you did with the ....

There's also a library I intended to check out github.com/robinovitch61/viewport.
It could be better to just use it but I haven't fully researched it.

Comment thread pkg/ui/keys.go
Comment on lines +76 to +83
ScrollLeft: key.NewBinding(
key.WithKeys("left"),
key.WithHelp("←", "scroll left"),
),
ScrollRight: key.NewBinding(
key.WithKeys("right"),
key.WithHelp("→", "scroll right"),
),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think these should scroll just 1 column at a time, like how j and k work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Step is one column per keypress now, matching how j / k step one line. The diffviewer's ScrollLeft / ScrollRight delegate straight to m.vp.ScrollLeft(1) / m.vp.ScrollRight(1).

Per dlvhdr#133 review: don't reimplement what the viewport already provides.

bubbles/v2/viewport already exposes ScrollLeft(n) / ScrollRight(n) and
manages xOffset / maxXOffset internally, so the local field and
renderViewport pass were duplicating that work. ScrollLeft / ScrollRight
on the diffviewer are now thin wrappers around the viewport, stepping
one column per call (matches the j/k vertical-step convention in this
codebase).

The conditional ellipsis markers from the previous iteration are
dropped: bubbles' viewport handles horizontal scroll by shifting the
visible window and doesn't expose a per-visible-line render hook, so
keeping the markers would mean reimplementing the slice path the review
asked to remove. Happy to add them back in a follow-up via a View()
wrapper, or by trying github.com/robinovitch61/viewport which the
maintainer flagged as an alternative.

The underlying bug fixes for dlvhdr#99 are unchanged:
  - delta is now invoked with --max-line-length=0 and
    --wrap-max-lines=unlimited in both diffFile and diffDir, so
    side-by-side carries the full line through.
  - the diffContentMsg handler no longer post-truncates the unified-
    mode output with an empty tail; SetContent gets the raw delta text
    and the viewport renders it directly.
@Booyaka101
Copy link
Copy Markdown
Contributor Author

Done in d1bb830. ScrollLeft / ScrollRight on the diffviewer are now thin wrappers around m.vp.ScrollLeft(1) / m.vp.ScrollRight(1) — the local xOffset field and the renderViewport pass are gone (they were duplicating what the viewport already manages).

The conditional markers came off with that — bubbles' viewport handles horizontal scroll by shifting the visible window and doesn't expose a per-visible-line render hook, so keeping the markers would mean reimplementing the slice path you asked to drop. Happy to add them back in a follow-up via a View() wrapper, or to look at github.com/robinovitch61/viewport if you want to switch the underlying viewport implementation entirely. Let me know which way you'd prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Long lines get silently truncated in the diff viewer

2 participants