diff --git a/README.md b/README.md index 554aba6..d1eaac1 100644 --- a/README.md +++ b/README.md @@ -12,11 +12,14 @@ A local PR‑style review UI for LLM workflows. Run the `meatcheck` CLI with a s - File tree + code view UI similar to GitHub PR reviews - Click to select a line, shift‑click for a range - Inline comment threads under the referenced line +- Unified and side‑by‑side diff views (toggle via toolbar button) +- Comment on both added and deleted lines in diff mode - Markdown rendering for comments (toggle raw/rendered) - Syntax highlighting for code (toggle raw/rendered) - Grouped review mode — organize files into named groups via `--groups` - Per‑file viewed/commented indicators in the tree sidebar - "Mark as viewed" advances to the next unviewed file +- Preferences (diff format, sidebar width) persist across sessions via XDG config - Outputs TOON format to stdout on Finish ## Install / Build @@ -66,6 +69,13 @@ The `--groups` flag takes a path to a JSON file that defines an ordered list of - `Ctrl+Enter` / `Cmd+Enter`: submit the inline comment +## Preferences + +Meatcheck stores user preferences at `$XDG_CONFIG_HOME/meatcheck/preferences.json` (typically `~/.config/meatcheck/preferences.json`). Currently persisted settings: + +- **Diff format** — unified or side‑by‑side +- **Sidebar width** — drag‑resized column width + ## Output On “Finish Review”, the app prints TOON to stdout and exits. diff --git a/docs/plans/diff-view-options.md b/docs/plans/diff-view-options.md new file mode 100644 index 0000000..0f3b486 --- /dev/null +++ b/docs/plans/diff-view-options.md @@ -0,0 +1,264 @@ +# Implementation Plan: Diff View Options (Unified/Side-by-Side, Preferences, Background) + +Add switchable unified/side-by-side diff view with localStorage persistence, enable commenting on old (deleted) lines, and change the diff area background to black. + +## Context + +**Research Document**: `docs/research/2026-03-09-diff-view-options.md` + +**Key Files**: +- `internal/app/model.go` - Data types: `ReviewModel`, `ViewDiffLine`, `ViewDiffHunk`, `ViewDiffFile`, `Comment` +- `internal/app/view.go` - View builders: `buildViewDiff()`, `updateDiffView()`, `projectLineComments()`, `diffLineExists()` +- `internal/app/app.go` - Event handlers: `toggle-file-render`, `select-line`, `add-comment`, `cancel-comment` +- `internal/app/diff.go` - Diff parser: `parseUnifiedDiff()`, `DiffLine`, `DiffHunk` +- `internal/ui/template.html` - HTML template with JS hooks for line selection and localStorage +- `internal/ui/styles.css` - CSS with theme variables and diff styling + +**Architectural Notes**: +- Server-driven reactive UI via `jfyne/live` — events go client→server, server mutates model, full re-render diffs to client +- All UI state lives in `ReviewModel` struct; only sidebar width persists in localStorage +- Template uses Go `html/template` with `{{if}}` conditionals for mode branching +- CSS uses custom properties (`:root` variables), embedded in binary via `//go:embed` + +**Functional Requirements** (EARS notation): +- When the user clicks the diff format toggle, the system shall switch between unified and side-by-side diff views +- While in side-by-side mode, the system shall display old lines on the left and new lines on the right, paired by position within each hunk +- When the user selects a diff format, the system shall persist the choice to localStorage +- When the application loads, the system shall restore the diff format from localStorage +- While viewing a diff in either format, the system shall allow clicking on both old (deleted) and new (added) lines to create comments +- The diff code area background shall be black (#000000) +- If the user is not in diff mode, then the diff format toggle shall be hidden + +## Execution Stages + +### Stage 1: Data Model and Background Color + +#### Test Creation Phase (parallel) +- T-test-1A: Write tests for `DiffFormat` model types and `diffOldLineExists` (hmm-test-writer) + - New feature tests (RED): Scenarios 1, 2 +- T-test-1B: Write tests for side-aware `projectLineComments` (hmm-test-writer) + - Regression tests: existing `projectLineComments` behavior with empty side + - New feature tests (RED): Scenarios 5, 6 + +#### Implementation Phase (parallel, depends on Test Creation Phase) +- T-impl-1A: Add `DiffFormat` type, `Comment.Side`, `SelectionSide`, `ViewDiffRow` types to model.go (hmm-implement-worker, TDD mode) + - Files: `internal/app/model.go` (modifies) + - Make RED tests pass (GREEN) +- T-impl-1B: Update `projectLineComments` for side-aware matching, add `diffOldLineExists` (hmm-implement-worker, TDD mode) + - Files: `internal/app/view.go` (modifies) + - Make RED tests pass (GREEN) +- T-impl-1C: Change `.diff` background to black (hmm-implement-worker, TDD mode) + - Files: `internal/ui/styles.css` (modifies) + +### Stage 2: Event Handlers and Unified View Old-Line Support (depends on Stage 1) + +#### Test Creation Phase (parallel) +- T-test-2A: Write tests for `buildViewDiff` with old-line selection and commenting (hmm-test-writer) + - Regression tests: existing `buildViewDiff` behavior + - New feature tests (RED): Scenarios 3, 4 + +#### Implementation Phase (sequential, depends on Test Creation Phase) +- T-impl-2A: Update all app.go event handlers — add `toggle-diff-format`, `init-diff-format`; update `select-line`, `add-comment`, `cancel-comment` for old-line support; initialize `DiffFormat` in `Run()` (hmm-implement-worker, TDD mode) + - Files: `internal/app/app.go` (modifies) +- T-impl-2B: Update `buildViewDiff` and `updateDiffView` for old-line selection/commenting and DiffFormat dispatch (hmm-implement-worker, TDD mode) + - Files: `internal/app/view.go` (modifies) + - Make RED tests pass (GREEN) + +### Stage 3: Side-by-Side View Builder (depends on Stage 2) + +#### Test Creation Phase (parallel) +- T-test-3A: Write tests for `buildViewDiffSplit` pairing algorithm (hmm-test-writer) + - New feature tests (RED): Scenarios 7, 8, 9 + +#### Implementation Phase (parallel, depends on Test Creation Phase) +- T-impl-3A: Implement `buildViewDiffSplit` function (hmm-implement-worker, TDD mode) + - Files: `internal/app/view.go` (modifies) + - Make RED tests pass (GREEN) + +### Stage 4: Template, CSS, and JavaScript (depends on Stage 3) + +#### Test Creation Phase +- T-test-4A: Write render tests for toggle visibility and split template output (hmm-test-writer) + - New feature tests (RED): Scenario 10 + +#### Implementation Phase (sequential, depends on Test Creation Phase) +- T-impl-4A: Update template.html — add toggle button, `data-old-line` attributes, side-by-side template block, old-line comment form conditional, JS click handler for old-line clicks, localStorage persistence (hmm-implement-worker, TDD mode) + - Files: `internal/ui/template.html` (modifies) +- T-impl-4B: Add side-by-side CSS layout (hmm-implement-worker, TDD mode) + - Files: `internal/ui/styles.css` (modifies) + +## Task List + +### Data Model + +- [x] Add `DiffFormat` type and constants [Stage 1] + - Files: `internal/app/model.go` (modifies) + - Add `type DiffFormat string` with `DiffFormatUnified DiffFormat = "unified"` and `DiffFormatSplit DiffFormat = "split"`. + - Add `DiffFormat DiffFormat` field to `ReviewModel` struct. + +- [x] Add `Side` field to `Comment` struct [Stage 1] + - Files: `internal/app/model.go` (modifies) + - Add `Side string \`json:"side,omitempty"\`` after `EndLine`. Values: `""` (new, default), `"old"`. + - `omitempty` ensures backward compatibility in TOON output. + +- [x] Add `SelectionSide` field to `ReviewModel` [Stage 1] + - Files: `internal/app/model.go` (modifies) + - Add `SelectionSide string` near `SelectionStart`/`SelectionEnd`. + +- [x] Add `ViewDiffRow` and `ViewDiffSide` types for side-by-side view [Stage 1] + - Files: `internal/app/model.go` (modifies) + - `ViewDiffSide`: `Line int`, `Kind DiffLineKind`, `Text string`, `HTML template.HTML`, `Empty bool`, `Selected bool`, `Commented bool`, `Comments []ViewComment` + - `ViewDiffRow`: `Left ViewDiffSide`, `Right ViewDiffSide` + - `ViewDiffSplitHunk`: `Header string`, `Rows []ViewDiffRow` + - Add `ViewDiffSplit []ViewDiffSplitHunk` field to `ReviewModel`. + +### View Building + +- [x] Update `projectLineComments` for side-aware matching [Stage 1] + - Files: `internal/app/view.go` (modifies) + - Add `side string` parameter. Filter `c.Side` against the provided side. Treat `c.Side == ""` as `"new"` for backward compat. + - Update all call sites: `buildViewDiff` (2 calls), `buildSingleViewLine` (1 call). File-mode calls pass `""`. + +- [x] Add `diffOldLineExists` function [Stage 1] + - Files: `internal/app/view.go` (modifies) + - Same structure as `diffLineExists` but checks `dl.OldLine == oldLine && dl.Kind != DiffAdd`. + +- [x] Update `buildViewDiff` for old-line selection and commenting [Stage 2] + - Files: `internal/app/view.go` (modifies) + - Accept `selectionSide string` parameter. + - Remove the `selectable` guard that blocks deleted lines. All lines with OldLine > 0 or NewLine > 0 are selectable. + - When `selectionSide == "old"`, check selection against `dl.OldLine`; otherwise `dl.NewLine`. + - Project comments: call `projectLineComments` with side `""` on `dl.NewLine` and side `"old"` on `dl.OldLine`, merge results. + +- [x] Update `updateDiffView` to dispatch on DiffFormat [Stage 2] + - Files: `internal/app/view.go` (modifies) + - When `DiffFormatSplit`, call `buildViewDiffSplit` and populate `model.ViewDiffSplit`. + - When `DiffFormatUnified` (or default), use existing `buildViewDiff`. + - Clear both `model.ViewDiff` and `model.ViewDiffSplit` at the start. + - Pass `model.SelectionSide` to `buildViewDiff` (and later `buildViewDiffSplit`) for correct selection highlighting. + +- [x] Implement `buildViewDiffSplit` pairing algorithm [Stage 3] + - Files: `internal/app/view.go` (modifies) + - For each hunk, pair lines into rows: + - Context lines: both Left and Right populated with same text + - Del/Add blocks: collect consecutive dels then adds, zip 1:1. Unpaired lines get `Empty: true` opposite side. + - Apply selection/comment projection to each side independently. + - Render syntax highlighting for old and new lines separately. + +### Event Handlers + +- [x] Update all app.go handlers and initialization for diff format and old-line support [Stage 2] + - Files: `internal/app/app.go` (modifies) + - **New handlers**: + - `toggle-diff-format`: toggle `model.DiffFormat` between `DiffFormatUnified` and `DiffFormatSplit`. Clear selection state (`SelectionStart`, `SelectionEnd`, `SelectionSide`). Call `updateView(model)`. + - `init-diff-format`: read `p.String("format")`, validate (`"unified"` or `"split"`), set `model.DiffFormat`. Call `updateView(model)`. + - **Modified handlers**: + - `select-line`: read `old_line` param. When `old_line > 0`, validate via `diffOldLineExists`, set `model.SelectionSide = "old"`. When selecting new lines, set `model.SelectionSide = ""`. + - `add-comment`: set `Side: model.SelectionSide` on new `Comment`. Clear `model.SelectionSide` after adding. + - `cancel-comment`: add `model.SelectionSide = ""` alongside existing selection clearing. + - **Initialization**: set `DiffFormat: DiffFormatUnified` in `ReviewModel` struct literal in `Run()`. + +### Template and UI + +- [x] Update template.html for diff format toggle, side-by-side view, old-line support, and localStorage [Stage 4] + - Files: `internal/ui/template.html` (modifies) + - **Toggle button**: Add diff format toggle in `.column-header`, only visible when `$root.Mode == "diff"`. Uses `live-click="toggle-diff-format"`. SVG icon of two rectangles side by side. `active` class when `$root.DiffFormat == "split"`. + - **Unified view old-line support**: Add `data-old-line="{{.OldLine}}"` to `.diff-line` div (line 123). Show comment threads on deleted lines — update template conditional so comments render when OldLine > 0 and the comment has side `"old"`. Update the inline-comment form conditional (template line 136) to also trigger when `$root.SelectionSide == "old"` and `.OldLine == $root.SelectionEnd`. + - **Side-by-side template block**: Add `{{if eq $root.DiffFormat "split"}}` conditional inside the diff mode block. Each row renders as `.diff-row-split` with two `.diff-cell` halves (left=old with `data-old-line`, right=new with `data-line`). Empty sides render placeholder cells. Comment threads render full-width below rows. Inline comment form spans full width after selection end row. + - **JS click handler**: If `data-line` (NewLine) is 0, fall back to `data-old-line` (OldLine). Send `old_line: "1"` flag to server when clicking an old line. + - **localStorage persistence**: On mount, read `localStorage.getItem("meatcheck-diff-format")` and send `init-diff-format` event (deferred until `window.Live` is available via polling). On toggle, save via `handleEvent("diff-format-changed", ...)`. + +### CSS + +- [x] Change `.diff` background to black [Stage 1] + - Files: `internal/ui/styles.css` (modifies) + - Change line 777 from `background: var(--panel)` to `background: #000000`. + +- [x] Add side-by-side CSS layout [Stage 4] + - Files: `internal/ui/styles.css` (modifies) + - `.diff-row-split`: `display: grid; grid-template-columns: 1fr 1fr` + - `.diff-cell`: `display: grid; grid-template-columns: 4ch 1ch max-content; gap: 12px` + - Kind-based backgrounds on `.diff-cell[data-kind="add"]` and `[data-kind="del"]` + - Hover, selected, commented states on `.diff-cell` + - `.diff-cell .code-text` rules mirroring existing `.diff-line .code-text` + - Border separator between left and right cells + +## Acceptance Criteria + +~~~gherkin +Feature: Diff view options + + Scenario: Toggle between unified and side-by-side diff view + Given the user is viewing a diff + When the user clicks the diff format toggle button + Then the view switches from unified to side-by-side (or vice versa) + And the toggle button shows an active state when in side-by-side mode + + Scenario: Diff format preference persists across sessions + Given the user has selected side-by-side diff format + When the user closes and reopens the tool + Then the diff view loads in side-by-side format + + Scenario: Side-by-side view pairs old and new lines correctly + Given a diff hunk with 2 deleted lines followed by 3 added lines + When the view is in side-by-side mode + Then the first 2 rows show del on the left and add on the right + And the 3rd row shows an empty left cell and add on the right + + Scenario: Context lines appear on both sides + Given a diff hunk with context lines + When the view is in side-by-side mode + Then context lines appear on both the left and right sides with matching line numbers + + Scenario: Comment on a deleted line in unified view + Given the user is viewing a diff in unified mode + When the user clicks on a deleted line + Then the line is selected and a comment form appears + And the comment is anchored to the old-side line number + + Scenario: Comment on a deleted line in side-by-side view + Given the user is viewing a diff in side-by-side mode + When the user clicks on a deleted line in the left column + Then the line is selected and a comment form appears + And the comment is anchored to the old-side line number + + Scenario: Side-by-side comments on old vs new lines with same number + Given a comment exists on old line 5 (deleted) and another on new line 5 (added) + When viewing in side-by-side mode + Then the old-line comment appears in the left column + And the new-line comment appears in the right column + And they do not cross-contaminate + + Scenario: Side-by-side view renders hunk headers full-width + Given a diff with multiple hunks + When the view is in side-by-side mode + Then hunk headers span the full width of both columns + + Scenario: Side-by-side view with syntax highlighting + Given syntax highlighting is enabled + When the view is in side-by-side mode + Then both left and right columns render syntax-highlighted code + + Scenario: Diff format toggle hidden in file mode + Given the user is viewing a plain file (not a diff) + Then the diff format toggle button is not visible + + Scenario: Diff area background is black + Given the user is viewing a diff + Then the diff code area background is black (#000000) + And the sidebar and other panels retain their original background +~~~ + +**Source**: Generated from plan context + +## Implementation Notes + +- **Backward compatibility**: `Comment.Side` uses `omitempty` so existing TOON output is unchanged. Empty `Side` is treated as `"new"`. +- **Shift-click range selection**: Old-line selection does not support shift-click ranges initially. Shift-clicking resets to single-line selection for simplicity. +- **Syntax highlighting in split view**: Old-side and new-side lines are rendered separately through `codeRenderer.RenderLines` for accurate highlighting. +- **Performance**: Side-by-side doubles DOM elements per row. Acceptable for typical diff sizes. +- **localStorage init timing**: Uses polling interval (50ms) to wait for `window.Live` to be available before sending `init-diff-format`. + +## Refs + +- `docs/research/2026-03-09-diff-view-options.md` diff --git a/docs/research/2026-03-09-diff-view-options.md b/docs/research/2026-03-09-diff-view-options.md new file mode 100644 index 0000000..442d947 --- /dev/null +++ b/docs/research/2026-03-09-diff-view-options.md @@ -0,0 +1,159 @@ +--- +date: 2026-03-09T00:00:00+00:00 +researcher: josh +topic: "Diff view mode switching (unified/side-by-side), preference persistence, and code background color" +tags: [research, codebase, diff, ui, preferences] +last_updated: 2026-03-09 +last_updated_by: josh +--- + +# Research: Diff View Options + +## Research Question + +How does the current diff view work, and what would be involved in: +1. Making the diff view switchable between unified and side-by-side modes +2. Persisting the user's choice across sessions +3. Changing the code view background from red to black + +## Summary + +Meatcheck currently renders diffs in a single **unified diff format** only. The diff pipeline flows through three layers: parsing (`diff.go`), view model building (`view.go`), and HTML rendering (`template.html`). The code view area background uses `var(--panel)` which resolves to `#161014` — a very dark reddish-brown. There is no side-by-side diff view and no mechanism for persisting view preferences beyond sidebar width (stored in `localStorage`). + +## Detailed Findings + +### Diff Parsing + +The `parseUnifiedDiff()` function in `internal/app/diff.go` parses standard Git unified diff output into structured data: + +- `DiffLineKind` enum: `DiffContext`, `DiffAdd`, `DiffDel` +- `DiffLine`: stores kind, old line number, new line number, and text +- `DiffHunk`: groups lines with old/new start positions and counts +- `DiffFile`: contains file path and an array of hunks + +The parser is a line-by-line state machine that classifies lines by their prefix character (`+`, `-`, ` `). Deleted lines have `NewLine=0`, added lines have `OldLine=0`, context lines have both. + +### View Model Building + +`buildViewDiff()` in `internal/app/view.go:119` converts parsed `DiffFile` data into `ViewDiffFile` for template rendering: + +- Iterates hunks, building hunk header strings (`@@ -oldStart,oldCount +newStart,newCount @@`) +- For each line: maps kind, line numbers, raw text, optional syntax-highlighted HTML +- Selection is only allowed on new-file lines (not deletions): `selectable := dl.NewLine > 0 && dl.Kind != DiffDel` +- Comments can only anchor to new file lines via `projectLineComments()` + +When `side-by-side` mode is introduced, this function will need a parallel variant that pairs old/new lines together rather than interleaving them sequentially. + +### HTML Template (Unified View) + +The diff section in `internal/ui/template.html:118-151` renders a unified view: + +```html +
+ {{range .ViewDiff.Hunks}} +
{{.Header}}
+ {{range .Lines}} +
+ {{.OldLine}} + {{.NewLine}} + +/-/ +
{{.HTML or .Text}}
+
+ {{end}} + {{end}} +
+``` + +The grid layout uses 4 columns: `4ch 4ch 1ch max-content` (old line#, new line#, sign, content). + +For side-by-side mode, the template will need a conditional branch (`{{if eq .DiffFormat "split"}}`) with a different HTML structure — likely two columns, each with its own line number, sign, and code content. + +### CSS Styling + +**Theme variables** (`internal/ui/styles.css:1-13`): + +| Variable | Value | Description | +|----------|-------|-------------| +| `--bg` | `#0f0b0c` | Page background | +| `--panel` | `#161014` | Panel/code area background — **dark reddish-brown** | +| `--ink` | `#f3e9ea` | Primary text color | +| `--muted` | `#b7a7aa` | Secondary text color | +| `--accent` | `#c00000` | Accent color (red) | +| `--border` | `#2a1b1e` | Border color (dark red-brown) | +| `--line-hover` | `#241317` | Line hover background | +| `--line-selected` | `#3a1515` | Selected line background | + +The `.diff` container uses `background: var(--panel)` (`styles.css:777`), which is `#161014`. This is the "red" background the user is referring to — it's a dark maroon/reddish-brown rather than pure black. + +**Diff line coloring** (`styles.css:828-842`): +- Added lines: `background: rgba(46, 160, 67, 0.12)` with green sign `#2ea043` +- Deleted lines: `background: rgba(248, 81, 73, 0.16)` with red sign `#f85149` +- Context lines: no background color + +### Preference Persistence + +**Current state**: Only sidebar width is persisted via `localStorage.getItem/setItem("meatcheck-sidebar-width")` in the JS hook (`template.html:223-228`). + +**All other state** (render toggles, viewed files, comments) lives in the server-side `ReviewModel` struct and is lost when the session ends. + +**Pattern for adding diff format persistence**: Following the existing sidebar-width pattern: +1. Store in `localStorage` with key like `"meatcheck-diff-format"` +2. On mount, read from localStorage and send initial value to server via `Live.send()` +3. Server stores the preference in `ReviewModel.DiffFormat` field +4. Toggle button sends event to server, server updates model, JS also persists to localStorage + +### Event Handling Pattern + +Event handlers are registered in `buildLiveHandler()` (`internal/app/app.go:190`). Existing toggle patterns: + +- `toggle-file-render`: flips `model.RenderFile` boolean, calls `updateView(model)` +- `toggle-comment-render`: flips `model.RenderComments` boolean +- `toggle-sidebar`: flips `model.SidebarCollapsed` boolean + +A `toggle-diff-format` event would follow the same pattern: flip a `DiffFormat` field on `ReviewModel` and call `updateView(model)`. + +### Toggle UI Pattern + +Existing toggle buttons in the column header (`template.html:65-80`) use this pattern: + +```html + +``` + +A diff format toggle would follow this same pattern, placed alongside the existing toggles. + +## Code References + +- `internal/app/diff.go` — Unified diff parser (`parseUnifiedDiff()`, `DiffLine`, `DiffFile`) +- `internal/app/model.go:66-71` — `ViewMode` enum (`ModeFile`, `ModeDiff`) +- `internal/app/model.go:73-92` — `ViewDiffLine`, `ViewDiffHunk`, `ViewDiffFile` structs +- `internal/app/model.go:105-132` — `ReviewModel` struct (all session state) +- `internal/app/view.go:11-18` — `updateView()` dispatch by mode +- `internal/app/view.go:119-157` — `buildViewDiff()` — builds view model for unified diff +- `internal/app/app.go:190-232` — `buildLiveHandler()` — event handler registration, render function +- `internal/ui/template.html:118-151` — Unified diff HTML template +- `internal/ui/template.html:216-324` — JavaScript hooks (localStorage, click handlers) +- `internal/ui/styles.css:1-13` — CSS theme variables (including `--panel: #161014`) +- `internal/ui/styles.css:776-867` — Diff-specific CSS (`.diff`, `.diff-line`, line kind colors) + +## Architecture Documentation + +The application follows a server-driven reactive pattern using `jfyne/live`: + +1. **Server-side state**: All UI state lives in `ReviewModel` struct, shared across WebSocket connections +2. **Event-driven updates**: Client sends events via WebSocket → Go handler mutates model → full template re-render → diff patched to client +3. **Client-side persistence**: Only sidebar width uses `localStorage`; everything else is ephemeral per session +4. **Template pattern**: Single `template.html` with conditional blocks for diff vs file mode +5. **CSS pattern**: CSS variables for theming, embedded in binary via `//go:embed` + +## Design Decisions + +1. **Background color**: Change only the `.diff` container background to black (`#000000`). Leave sidebar, hunk headers, and other panels using `--panel` (`#161014`) unchanged. +2. **Comment anchoring**: Comments should be attachable to **both old and new lines** — in both unified and side-by-side modes. This is a change from the current behavior where comments can only anchor to new-file lines. The `selectable` guard in `buildViewDiff()` (`view.go:142`) and the template's line-click handler need to be updated to allow selection of deleted/old lines. +3. **Toggle visibility**: The unified/side-by-side toggle button should only be visible in diff mode. Hide it when viewing plain files. + +## Open Questions + +None — all resolved. diff --git a/go.mod b/go.mod index 2794eca..96200be 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( ) require ( + github.com/adrg/xdg v0.5.3 // indirect github.com/coder/websocket v1.8.14 // indirect github.com/dlclark/regexp2 v1.11.0 // indirect github.com/google/go-cmp v0.7.0 // indirect diff --git a/go.sum b/go.sum index ab77a24..e922084 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +github.com/adrg/xdg v0.5.3 h1:xRnxJXne7+oWDatRhR1JLnvuccuIeCoBu2rtuLqQB78= +github.com/adrg/xdg v0.5.3/go.mod h1:nlTsY+NNiCBGCK2tpm09vRqfVzrc2fLmXGpBLF0zlTQ= github.com/alecthomas/assert/v2 v2.6.0 h1:o3WJwILtexrEUk3cUVal3oiQY2tfgr/FHWiz/v2n4FU= github.com/alecthomas/assert/v2 v2.6.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k= github.com/alecthomas/chroma/v2 v2.13.0 h1:VP72+99Fb2zEcYM0MeaWJmV+xQvz5v5cxRHd+ooU1lI= diff --git a/internal/app/app.go b/internal/app/app.go index d4826bf..ca5a302 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -84,6 +84,8 @@ func Run(ctx context.Context, cfg Config) error { SelectedPath: "", SelectedLabel: "", Mode: mode, + DiffFormat: preferredDiffFormat(), + SidebarWidth: loadPreferences().SidebarWidth, RenderFile: true, RenderComments: true, Prompt: cfg.Prompt, @@ -280,18 +282,29 @@ func buildLiveHandler(rs *ReviewServer) *live.Handler { model := getModel(s, rs.Model) line := p.Int("line") lineEnd := p.Int("line_end") + oldLine := p.Int("old_line") shift := p.String("shift") == "1" - if line <= 0 { - return model, nil + if model.Mode == ModeDiff && oldLine > 0 { + if !diffOldLineExists(model.DiffFiles, model.SelectedPath, oldLine) { + return model, nil + } + line = oldLine + lineEnd = oldLine + model.SelectionSide = "old" + } else { + if line <= 0 { + return model, nil + } + if model.Mode == ModeDiff { + if !diffLineExists(model.DiffFiles, model.SelectedPath, line) { + return model, nil + } + } + model.SelectionSide = "" } if lineEnd < line { lineEnd = line } - if model.Mode == ModeDiff { - if !diffLineExists(model.DiffFiles, model.SelectedPath, line) { - return model, nil - } - } if shift && model.SelectionStart > 0 { start := model.SelectionStart end := lineEnd @@ -326,12 +339,14 @@ func buildLiveHandler(rs *ReviewServer) *live.Handler { Path: model.SelectedPath, StartLine: model.SelectionStart, EndLine: model.SelectionEnd, + Side: model.SelectionSide, Text: text, }) model.CommentDraft = "" model.Error = "" model.SelectionStart = 0 model.SelectionEnd = 0 + model.SelectionSide = "" rebuildTree(model) updateView(model) return model, nil @@ -343,6 +358,7 @@ func buildLiveHandler(rs *ReviewServer) *live.Handler { model.Error = "" model.SelectionStart = 0 model.SelectionEnd = 0 + model.SelectionSide = "" updateView(model) return model, nil }) @@ -414,6 +430,31 @@ func buildLiveHandler(rs *ReviewServer) *live.Handler { return model, nil }) + h.HandleEvent("toggle-diff-format", func(ctx context.Context, s *live.Socket, p live.Params) (any, error) { + model := getModel(s, rs.Model) + if model.DiffFormat == DiffFormatSplit { + model.DiffFormat = DiffFormatUnified + } else { + model.DiffFormat = DiffFormatSplit + } + model.SelectionStart = 0 + model.SelectionEnd = 0 + model.SelectionSide = "" + savePreference(func(p *Preferences) { p.DiffFormat = model.DiffFormat }) + updateView(model) + return model, nil + }) + + h.HandleEvent("save-sidebar-width", func(ctx context.Context, s *live.Socket, p live.Params) (any, error) { + model := getModel(s, rs.Model) + w := p.String("width") + if w != "" { + model.SidebarWidth = w + savePreference(func(p *Preferences) { p.SidebarWidth = w }) + } + return model, nil + }) + h.HandleEvent("finish", func(ctx context.Context, s *live.Socket, p live.Params) (any, error) { model := getModel(s, rs.Model) if s != nil { diff --git a/internal/app/comment_test.go b/internal/app/comment_test.go index b42f5e4..d7e8e8c 100644 --- a/internal/app/comment_test.go +++ b/internal/app/comment_test.go @@ -61,7 +61,7 @@ func TestProjectLineCommentsEditing(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - _, viewComments := projectLineComments("a.go", 5, comments, tc.editingID) + _, viewComments := projectLineComments("a.go", 5, comments, tc.editingID, "") if len(viewComments) != 2 { t.Fatalf("expected 2 view comments, got %d", len(viewComments)) } diff --git a/internal/app/diff_format_test.go b/internal/app/diff_format_test.go new file mode 100644 index 0000000..5e0ea21 --- /dev/null +++ b/internal/app/diff_format_test.go @@ -0,0 +1,106 @@ +package app + +import "testing" + +// TestDiffFormatConstants verifies DiffFormatUnified and DiffFormatSplit have +// the correct string values. +func TestDiffFormatConstants(t *testing.T) { + if DiffFormatUnified != DiffFormat("unified") { + t.Errorf("expected DiffFormatUnified == \"unified\", got %q", DiffFormatUnified) + } + if DiffFormatSplit != DiffFormat("split") { + t.Errorf("expected DiffFormatSplit == \"split\", got %q", DiffFormatSplit) + } + // Ensure the two constants are distinct. + if DiffFormatUnified == DiffFormatSplit { + t.Error("DiffFormatUnified and DiffFormatSplit must be distinct") + } +} + +// TestDiffOldLineExists is a table-driven test for diffOldLineExists. +func TestDiffOldLineExists(t *testing.T) { + files := []DiffFile{ + { + Path: "a.go", + Hunks: []DiffHunk{ + { + OldStart: 1, OldCount: 3, NewStart: 1, NewCount: 3, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 1, NewLine: 1, Text: "ctx"}, + {Kind: DiffDel, OldLine: 2, NewLine: 0, Text: "removed"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 2, Text: "added"}, + {Kind: DiffContext, OldLine: 3, NewLine: 3, Text: "ctx2"}, + }, + }, + }, + }, + } + + tests := []struct { + name string + path string + oldLine int + expected bool + }{ + { + name: "old line on a del line returns true", + path: "a.go", + oldLine: 2, + expected: true, + }, + { + name: "old line on a context line returns true", + path: "a.go", + oldLine: 1, + expected: true, + }, + { + name: "old line 0 on add line returns false", + path: "a.go", + oldLine: 0, + expected: false, + }, + { + name: "line number not in any hunk returns false", + path: "a.go", + oldLine: 99, + expected: false, + }, + { + name: "file not found returns false", + path: "notexist.go", + oldLine: 1, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := diffOldLineExists(files, tc.path, tc.oldLine) + if got != tc.expected { + t.Errorf("diffOldLineExists(%q, %d) = %v, want %v", tc.path, tc.oldLine, got, tc.expected) + } + }) + } +} + +// TestReviewModelHasDiffFormat verifies that ReviewModel carries a DiffFormat +// field and that it can be set to the known constants. +func TestReviewModelHasDiffFormat(t *testing.T) { + m := ReviewModel{} + + // Zero value — DiffFormat field must exist (compiler enforces this). + if m.DiffFormat != DiffFormat("") { + t.Errorf("zero value of DiffFormat should be empty string, got %q", m.DiffFormat) + } + + m.DiffFormat = DiffFormatUnified + if m.DiffFormat != DiffFormatUnified { + t.Errorf("expected DiffFormatUnified after assignment, got %q", m.DiffFormat) + } + + m.DiffFormat = DiffFormatSplit + if m.DiffFormat != DiffFormatSplit { + t.Errorf("expected DiffFormatSplit after assignment, got %q", m.DiffFormat) + } +} diff --git a/internal/app/diff_format_view_test.go b/internal/app/diff_format_view_test.go new file mode 100644 index 0000000..5e82c1f --- /dev/null +++ b/internal/app/diff_format_view_test.go @@ -0,0 +1,306 @@ +package app + +import ( + "strings" + "testing" +) + +// TestUpdateDiffViewUnifiedFormat verifies that updateDiffView populates +// ViewDiff (not ViewDiffSplit) when DiffFormat is DiffFormatUnified. +// +// Scenario: Toggle between unified and side-by-side diff view (unified path) +func TestUpdateDiffViewUnifiedFormat(t *testing.T) { + model := &ReviewModel{ + Mode: ModeDiff, + DiffFormat: DiffFormatUnified, + SelectedPath: "a.go", + DiffFiles: []DiffFile{{ + Path: "a.go", + Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 1, + NewStart: 1, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 1, NewLine: 1, Text: "package main"}, + }, + }}, + }}, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + + updateDiffView(model) + + // Unified format: ViewDiff.Hunks must be populated. + if len(model.ViewDiff.Hunks) == 0 { + t.Errorf("expected ViewDiff.Hunks to be non-empty for DiffFormatUnified, got 0 hunks") + } + // Specific content check: the hunk must carry the context line. + if len(model.ViewDiff.Hunks[0].Lines) != 1 { + t.Errorf("expected 1 line in hunk, got %d", len(model.ViewDiff.Hunks[0].Lines)) + } + if model.ViewDiff.Hunks[0].Lines[0].Text != "package main" { + t.Errorf("expected line text %q, got %q", "package main", model.ViewDiff.Hunks[0].Lines[0].Text) + } + // Split view must not be populated. + if model.ViewDiffSplit != nil { + t.Errorf("expected ViewDiffSplit to be nil for DiffFormatUnified, got %v", model.ViewDiffSplit) + } +} + +// TestUpdateDiffViewSplitFormat verifies that updateDiffView populates +// ViewDiffSplit (not ViewDiff) when DiffFormat is DiffFormatSplit. +// +// Scenario: Toggle between unified and side-by-side diff view (split path) +func TestUpdateDiffViewSplitFormat(t *testing.T) { + model := &ReviewModel{ + Mode: ModeDiff, + DiffFormat: DiffFormatSplit, + SelectedPath: "b.go", + DiffFiles: []DiffFile{{ + Path: "b.go", + Hunks: []DiffHunk{{ + OldStart: 3, + OldCount: 2, + NewStart: 3, + NewCount: 2, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 3, NewLine: 0, Text: "old line"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 3, Text: "new line"}, + }, + }}, + }}, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + + updateDiffView(model) + + // Split format: ViewDiffSplit must be populated. + if len(model.ViewDiffSplit) == 0 { + t.Errorf("expected ViewDiffSplit to be non-empty for DiffFormatSplit, got 0 hunks") + } + // Specific content check: first row left side must carry the del line. + rows := model.ViewDiffSplit[0].Rows + if len(rows) == 0 { + t.Fatalf("expected at least 1 row in ViewDiffSplit hunk, got 0") + } + if rows[0].Left.Text != "old line" { + t.Errorf("expected split row Left.Text %q, got %q", "old line", rows[0].Left.Text) + } + if rows[0].Right.Text != "new line" { + t.Errorf("expected split row Right.Text %q, got %q", "new line", rows[0].Right.Text) + } + // Unified view must not be populated (hunks must be empty). + if len(model.ViewDiff.Hunks) != 0 { + t.Errorf("expected ViewDiff.Hunks to be empty for DiffFormatSplit, got %d hunks", len(model.ViewDiff.Hunks)) + } +} + +// TestToggleDiffFormatClearsSelection verifies that when the diff format is +// toggled the selection state is cleared, matching what the toggle-diff-format +// handler does. +// +// Scenario: Toggle between unified and side-by-side diff view (selection cleared) +func TestToggleDiffFormatClearsSelection(t *testing.T) { + model := &ReviewModel{ + Mode: ModeDiff, + DiffFormat: DiffFormatUnified, + SelectedPath: "c.go", + SelectionStart: 5, + SelectionEnd: 10, + SelectionSide: "old", + DiffFiles: []DiffFile{{ + Path: "c.go", + Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 1, + NewStart: 1, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 5, NewLine: 5, Text: "ctx"}, + }, + }}, + }}, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + + // Simulate what toggle-diff-format handler does: swap format, clear selection. + if model.DiffFormat == DiffFormatUnified { + model.DiffFormat = DiffFormatSplit + } else { + model.DiffFormat = DiffFormatUnified + } + model.SelectionStart = 0 + model.SelectionEnd = 0 + model.SelectionSide = "" + updateDiffView(model) + + if model.DiffFormat != DiffFormatSplit { + t.Errorf("expected DiffFormat to be %q after toggle, got %q", DiffFormatSplit, model.DiffFormat) + } + if model.SelectionStart != 0 { + t.Errorf("expected SelectionStart = 0 after toggle, got %d", model.SelectionStart) + } + if model.SelectionEnd != 0 { + t.Errorf("expected SelectionEnd = 0 after toggle, got %d", model.SelectionEnd) + } + if model.SelectionSide != "" { + t.Errorf("expected SelectionSide = %q after toggle, got %q", "", model.SelectionSide) + } +} + +// TestHTTPRenderDiffFormatToggleHiddenInFileMode verifies that the diff format +// toggle button is NOT rendered when Mode == ModeFile (file mode). +// +// Scenario: Diff format toggle hidden in file mode +func TestHTTPRenderDiffFormatToggleHiddenInFileMode(t *testing.T) { + model := &ReviewModel{ + Files: []File{{ + Path: "a.go", + PathSlash: "a.go", + Lines: []string{"package main"}, + }}, + SelectedPath: "a.go", + Mode: ModeFile, + RenderFile: true, + RenderComments: true, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + model.Tree = buildTree(model.Files, model.SelectedPath, nil, nil) + + html := renderReviewHTML(t, model) + + // The toggle button element must not appear in file mode. + if strings.Contains(html, `live-click="toggle-diff-format"`) { + t.Errorf("expected diff format toggle button to be absent in file mode, but found it in rendered HTML") + } +} + +// TestHTTPRenderDiffFormatToggleVisibleInDiffMode verifies that the diff format +// toggle button IS rendered when Mode == ModeDiff. +// +// Scenario: Diff format toggle hidden in file mode (inverse: visible in diff mode) +func TestHTTPRenderDiffFormatToggleVisibleInDiffMode(t *testing.T) { + model := &ReviewModel{ + DiffFiles: []DiffFile{{ + Path: "a.go", + Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 1, + NewStart: 1, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 1, NewLine: 1, Text: "package main"}, + }, + }}, + }}, + SelectedPath: "a.go", + Mode: ModeDiff, + DiffFormat: DiffFormatUnified, + RenderFile: true, + RenderComments: true, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + model.Tree = buildTree(diffFilesAsFiles(model.DiffFiles), model.SelectedPath, nil, nil) + + html := renderReviewHTML(t, model) + + // The toggle button must be present in diff mode. + if !strings.Contains(html, `toggle-diff-format`) { + t.Errorf("expected diff format toggle (toggle-diff-format) to be present in diff mode, but not found in rendered HTML") + } +} + +// TestHTTPRenderSplitDiffBlock verifies that when DiffFormat == DiffFormatSplit +// the rendered HTML contains the side-by-side split block (diff-row-split class). +// +// Scenario: Side-by-side view pairs old and new lines correctly (template output) +func TestHTTPRenderSplitDiffBlock(t *testing.T) { + model := &ReviewModel{ + DiffFiles: []DiffFile{{ + Path: "a.go", + Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 1, + NewStart: 1, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 1, NewLine: 0, Text: "old line"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 1, Text: "new line"}, + }, + }}, + }}, + SelectedPath: "a.go", + Mode: ModeDiff, + DiffFormat: DiffFormatSplit, + RenderFile: true, + RenderComments: true, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + model.Tree = buildTree(diffFilesAsFiles(model.DiffFiles), model.SelectedPath, nil, nil) + + html := renderReviewHTML(t, model) + + // The split template block must render the diff-row-split element. + if !strings.Contains(html, `diff-row-split`) { + t.Errorf("expected diff-row-split class in rendered HTML for DiffFormatSplit, got: %q", html) + } + // Both left (del) and right (add) cells must be rendered. + if !strings.Contains(html, `diff-cell`) { + t.Errorf("expected diff-cell class in rendered HTML for DiffFormatSplit, got: %q", html) + } +} + +// TestHTTPRenderUnifiedDiffNotSplit verifies that when DiffFormat == DiffFormatUnified +// the rendered HTML uses the unified diff block (diff-line class) and NOT the +// split block (diff-row-split). +// +// Scenario: Toggle between unified and side-by-side diff view (template unified path) +func TestHTTPRenderUnifiedDiffNotSplit(t *testing.T) { + model := &ReviewModel{ + DiffFiles: []DiffFile{{ + Path: "a.go", + Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 1, + NewStart: 1, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 1, NewLine: 1, Text: "package main"}, + }, + }}, + }}, + SelectedPath: "a.go", + Mode: ModeDiff, + DiffFormat: DiffFormatUnified, + RenderFile: true, + RenderComments: true, + Viewed: make(map[string]bool), + Ranges: map[string][]LineRange{}, + MarkdownRenderByPath: map[string]bool{}, + } + model.Tree = buildTree(diffFilesAsFiles(model.DiffFiles), model.SelectedPath, nil, nil) + + html := renderReviewHTML(t, model) + + // Unified format must use diff-line elements. + if !strings.Contains(html, `diff-line`) { + t.Errorf("expected diff-line class in rendered HTML for DiffFormatUnified, got: %q", html) + } + // Split view HTML elements must be absent (check for the element class, not CSS selectors). + if strings.Contains(html, `class="diff-row-split"`) { + t.Errorf("expected diff-row-split elements to be absent in rendered HTML for DiffFormatUnified") + } +} diff --git a/internal/app/model.go b/internal/app/model.go index fe785f1..4bf88da 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -10,6 +10,7 @@ type Comment struct { Path string `json:"path"` StartLine int `json:"start_line"` EndLine int `json:"end_line"` + Side string `json:"side,omitempty"` Text string `json:"text"` } @@ -70,6 +71,13 @@ const ( ModeDiff ViewMode = "diff" ) +type DiffFormat string + +const ( + DiffFormatUnified DiffFormat = "unified" + DiffFormatSplit DiffFormat = "split" +) + type ViewDiffLine struct { Kind DiffLineKind OldLine int @@ -91,6 +99,27 @@ type ViewDiffFile struct { Hunks []ViewDiffHunk } +type ViewDiffSide struct { + Line int + Kind DiffLineKind + Text string + HTML template.HTML + Empty bool + Selected bool + Commented bool + Comments []ViewComment +} + +type ViewDiffRow struct { + Left ViewDiffSide + Right ViewDiffSide +} + +type ViewDiffSplitHunk struct { + Header string + Rows []ViewDiffRow +} + type ViewComment struct { Comment Rendered template.HTML @@ -120,6 +149,7 @@ type ReviewModel struct { PromptHTML template.HTML SelectionStart int SelectionEnd int + SelectionSide string CommentDraft string Comments []Comment NextCommentID int @@ -128,9 +158,30 @@ type ReviewModel struct { MarkdownRenderByPath map[string]bool ViewFile ViewFile ViewDiff ViewDiffFile + ViewDiffSplit []ViewDiffSplitHunk + DiffFormat DiffFormat + SidebarWidth string Error string } +// diffOldLineExists reports whether the given old-side line number exists in +// the diff hunks for the specified file path. Add lines are excluded because +// they have no old-side representation. +func diffOldLineExists(files []DiffFile, path string, oldLine int) bool { + file := findDiffFile(files, path) + if file == nil { + return false + } + for _, h := range file.Hunks { + for _, dl := range h.Lines { + if dl.OldLine == oldLine && dl.Kind != DiffAdd { + return true + } + } + } + return false +} + type ReviewServer struct { Model *ReviewModel DoneCh chan struct{} diff --git a/internal/app/preferences.go b/internal/app/preferences.go new file mode 100644 index 0000000..9294822 --- /dev/null +++ b/internal/app/preferences.go @@ -0,0 +1,48 @@ +package app + +import ( + "encoding/json" + "os" + "path/filepath" + + "github.com/adrg/xdg" +) + +type Preferences struct { + DiffFormat DiffFormat `json:"diff_format,omitempty"` + SidebarWidth string `json:"sidebar_width,omitempty"` +} + +func preferencesPath() string { + return filepath.Join(xdg.ConfigHome, "meatcheck", "preferences.json") +} + +func preferredDiffFormat() DiffFormat { + p := loadPreferences() + if p.DiffFormat == DiffFormatSplit || p.DiffFormat == DiffFormatUnified { + return p.DiffFormat + } + return DiffFormatUnified +} + +func loadPreferences() Preferences { + data, err := os.ReadFile(preferencesPath()) + if err != nil { + return Preferences{} + } + var p Preferences + _ = json.Unmarshal(data, &p) + return p +} + +func savePreference(fn func(*Preferences)) { + p := loadPreferences() + fn(&p) + path := preferencesPath() + _ = os.MkdirAll(filepath.Dir(path), 0o755) + data, err := json.Marshal(p) + if err != nil { + return + } + _ = os.WriteFile(path, data, 0o644) +} diff --git a/internal/app/side_comment_test.go b/internal/app/side_comment_test.go new file mode 100644 index 0000000..79a8634 --- /dev/null +++ b/internal/app/side_comment_test.go @@ -0,0 +1,216 @@ +package app + +import ( + "encoding/json" + "strings" + "testing" +) + +// TestProjectLineCommentsSideAware verifies that projectLineComments, once it +// gains a side parameter, filters comments by their Side field. +// +// Scenario: side="" matches only comments with c.Side=="" (backward compat) +// Scenario: side="" does NOT match comments with c.Side="old" +// Scenario: side="old" matches comments with c.Side="old" +// Scenario: side="old" does NOT match comments with c.Side=="" +func TestProjectLineCommentsSideAware(t *testing.T) { + newSideComment := Comment{ + ID: 1, + Path: "file.go", + StartLine: 5, + EndLine: 5, + Text: "new-side comment", + Side: "", + } + oldSideComment := Comment{ + ID: 2, + Path: "file.go", + StartLine: 5, + EndLine: 5, + Text: "old-side comment", + Side: "old", + } + allComments := []Comment{newSideComment, oldSideComment} + + tests := []struct { + name string + side string + wantCommented bool + wantCount int + wantCommentText string + }{ + { + name: "side='' matches new-side comment only", + side: "", + wantCommented: true, + wantCount: 1, + wantCommentText: "new-side comment", + }, + { + name: "side='' does not match old-side comment", + side: "", + wantCommented: true, + wantCount: 1, + // Verified by checking the single result is NOT the old-side one. + wantCommentText: "new-side comment", + }, + { + name: "side='old' matches old-side comment only", + side: "old", + wantCommented: true, + wantCount: 1, + wantCommentText: "old-side comment", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + commented, viewComments := projectLineComments("file.go", 5, allComments, 0, tc.side) + if commented != tc.wantCommented { + t.Errorf("commented: got %v, want %v", commented, tc.wantCommented) + } + if len(viewComments) != tc.wantCount { + t.Fatalf("len(viewComments): got %d, want %d", len(viewComments), tc.wantCount) + } + if viewComments[0].Text != tc.wantCommentText { + t.Errorf("comment text: got %q, want %q", viewComments[0].Text, tc.wantCommentText) + } + }) + } + + // Explicit sub-test: side="old" must NOT return the new-side comment. + t.Run("side='old' does not match new-side comment", func(t *testing.T) { + _, viewComments := projectLineComments("file.go", 5, allComments, 0, "old") + for _, vc := range viewComments { + if vc.Side != "old" { + t.Errorf("unexpected comment with Side=%q returned for side='old' query", vc.Side) + } + } + }) + + // Explicit sub-test: side="" must NOT return the old-side comment. + t.Run("side='' does not match old-side comment", func(t *testing.T) { + _, viewComments := projectLineComments("file.go", 5, allComments, 0, "") + for _, vc := range viewComments { + if vc.Side != "" { + t.Errorf("unexpected comment with Side=%q returned for side='' query", vc.Side) + } + } + }) +} + +// TestProjectLineCommentsSideAwareNotCommented verifies that when side="old" is +// requested but only a new-side comment exists on the line, commented is false. +// +// Scenario: side="old" does NOT set commented when only new-side comment exists +func TestProjectLineCommentsSideAwareNotCommented(t *testing.T) { + comments := []Comment{ + {ID: 1, Path: "file.go", StartLine: 3, EndLine: 3, Text: "new only", Side: ""}, + } + + commented, viewComments := projectLineComments("file.go", 3, comments, 0, "old") + if commented { + t.Error("commented should be false when no old-side comment exists on the line") + } + if len(viewComments) != 0 { + t.Errorf("viewComments: got %d, want 0", len(viewComments)) + } +} + +// TestProjectLineCommentsSideAwareDifferentInputs verifies different line/path +// combos to prevent the implementation from returning a hardcoded value. +func TestProjectLineCommentsSideAwareDifferentInputs(t *testing.T) { + comments := []Comment{ + {ID: 10, Path: "alpha.go", StartLine: 1, EndLine: 1, Text: "alpha line1 new", Side: ""}, + {ID: 11, Path: "alpha.go", StartLine: 2, EndLine: 2, Text: "alpha line2 old", Side: "old"}, + {ID: 12, Path: "beta.go", StartLine: 1, EndLine: 1, Text: "beta line1 old", Side: "old"}, + } + + // alpha.go line 1 new-side: expects 1 comment with ID 10 + _, got := projectLineComments("alpha.go", 1, comments, 0, "") + if len(got) != 1 || got[0].ID != 10 { + t.Errorf("alpha.go line 1 side='': got %v, want [{ID:10}]", got) + } + + // alpha.go line 2 old-side: expects 1 comment with ID 11 + _, got = projectLineComments("alpha.go", 2, comments, 0, "old") + if len(got) != 1 || got[0].ID != 11 { + t.Errorf("alpha.go line 2 side='old': got %v, want [{ID:11}]", got) + } + + // beta.go line 1 new-side: expects 0 comments (only old-side comment exists) + _, got = projectLineComments("beta.go", 1, comments, 0, "") + if len(got) != 0 { + t.Errorf("beta.go line 1 side='': got %d comments, want 0", len(got)) + } + + // beta.go line 1 old-side: expects 1 comment with ID 12 + _, got = projectLineComments("beta.go", 1, comments, 0, "old") + if len(got) != 1 || got[0].ID != 12 { + t.Errorf("beta.go line 1 side='old': got %v, want [{ID:12}]", got) + } +} + +// TestCommentSideFieldOmitEmpty verifies that a Comment with Side="" marshals +// to JSON without the "side" key (omitempty behavior). +// +// Scenario: Comment with Side="" marshals without "side" key in JSON +func TestCommentSideFieldOmitEmpty(t *testing.T) { + c := Comment{ + ID: 1, + Path: "a.go", + StartLine: 1, + EndLine: 1, + Text: "hello", + Side: "", + } + + data, err := json.Marshal(c) + if err != nil { + t.Fatalf("json.Marshal error: %v", err) + } + + got := string(data) + if strings.Contains(got, `"side"`) { + t.Errorf("JSON should not contain 'side' key when Side is empty, got: %s", got) + } + // Verify other fields are present. + if !strings.Contains(got, `"id":1`) { + t.Errorf("JSON missing 'id' field, got: %s", got) + } + if !strings.Contains(got, `"path":"a.go"`) { + t.Errorf("JSON missing 'path' field, got: %s", got) + } +} + +// TestCommentSideFieldPresent verifies that a Comment with Side="old" marshals +// to JSON with "side":"old". +// +// Scenario: Comment with Side="old" marshals with "side":"old" in JSON +func TestCommentSideFieldPresent(t *testing.T) { + c := Comment{ + ID: 2, + Path: "b.go", + StartLine: 5, + EndLine: 5, + Text: "old side comment", + Side: "old", + } + + data, err := json.Marshal(c) + if err != nil { + t.Fatalf("json.Marshal error: %v", err) + } + + got := string(data) + if !strings.Contains(got, `"side":"old"`) { + t.Errorf("JSON should contain '\"side\":\"old\"', got: %s", got) + } + // Verify other fields are present. + if !strings.Contains(got, `"id":2`) { + t.Errorf("JSON missing 'id' field, got: %s", got) + } + if !strings.Contains(got, `"path":"b.go"`) { + t.Errorf("JSON missing 'path' field, got: %s", got) + } +} diff --git a/internal/app/view.go b/internal/app/view.go index 827919a..098acb0 100644 --- a/internal/app/view.go +++ b/internal/app/view.go @@ -59,15 +59,141 @@ func updateFileView(model *ReviewModel) { } func updateDiffView(model *ReviewModel) { + model.ViewDiff = ViewDiffFile{} + model.ViewDiffSplit = nil + diffFile := findDiffFile(model.DiffFiles, model.SelectedPath) - viewDiff := ViewDiffFile{Path: model.SelectedPath} if diffFile != nil { - viewDiff = buildViewDiff(diffFile, model.Comments, model.SelectionStart, model.SelectionEnd, model.RenderFile, model.EditingCommentID) + switch model.DiffFormat { + case DiffFormatSplit: + model.ViewDiffSplit = buildViewDiffSplit(diffFile, model.Comments, model.SelectionStart, model.SelectionEnd, model.RenderFile, model.EditingCommentID, model.SelectionSide) + default: + model.ViewDiff = buildViewDiff(diffFile, model.Comments, model.SelectionStart, model.SelectionEnd, model.RenderFile, model.EditingCommentID, model.SelectionSide) + } } - model.ViewDiff = viewDiff model.SelectedLabel = model.SelectedPath } +func buildViewDiffSplit(file *DiffFile, comments []Comment, start, end int, render bool, editingID int, selectionSide string) []ViewDiffSplitHunk { + hunks := make([]ViewDiffSplitHunk, 0, len(file.Hunks)) + for _, h := range file.Hunks { + hdr := fmt.Sprintf("@@ -%d,%d +%d,%d @@", h.OldStart, h.OldCount, h.NewStart, h.NewCount) + vh := ViewDiffSplitHunk{Header: hdr} + + // Render syntax highlighting for all lines in the hunk if requested. + var rendered []template.HTML + if render { + texts := make([]string, 0, len(h.Lines)) + for _, dl := range h.Lines { + texts = append(texts, dl.Text) + } + rendered = codeRenderer.RenderLines(file.Path, texts) + } + + // Process lines: walk sequentially, grouping del/add blocks together. + lines := h.Lines + i := 0 + for i < len(lines) { + dl := lines[i] + if dl.Kind == DiffContext { + // Context line: both sides populated. + row := ViewDiffRow{ + Left: ViewDiffSide{ + Line: dl.OldLine, + Kind: DiffContext, + Text: dl.Text, + }, + Right: ViewDiffSide{ + Line: dl.NewLine, + Kind: DiffContext, + Text: dl.Text, + }, + } + if len(rendered) > i { + row.Left.HTML = rendered[i] + row.Right.HTML = rendered[i] + } + vh.Rows = append(vh.Rows, row) + i++ + } else { + // Collect consecutive del lines, then consecutive add lines. + var dels []int // indices into lines + var adds []int + for i < len(lines) && lines[i].Kind == DiffDel { + dels = append(dels, i) + i++ + } + for i < len(lines) && lines[i].Kind == DiffAdd { + adds = append(adds, i) + i++ + } + + // Zip dels and adds 1:1. + maxLen := max(len(dels), len(adds)) + for j := range maxLen { + row := ViewDiffRow{} + if j < len(dels) { + idx := dels[j] + row.Left = ViewDiffSide{ + Line: lines[idx].OldLine, + Kind: DiffDel, + Text: lines[idx].Text, + } + if len(rendered) > idx { + row.Left.HTML = rendered[idx] + } + } else { + row.Left = ViewDiffSide{Empty: true} + } + if j < len(adds) { + idx := adds[j] + row.Right = ViewDiffSide{ + Line: lines[idx].NewLine, + Kind: DiffAdd, + Text: lines[idx].Text, + } + if len(rendered) > idx { + row.Right.HTML = rendered[idx] + } + } else { + row.Right = ViewDiffSide{Empty: true} + } + vh.Rows = append(vh.Rows, row) + } + } + } + + // Apply selection and comment projection to each row. + for ri := range vh.Rows { + row := &vh.Rows[ri] + + // Selection projection. + if start > 0 && end > 0 { + if selectionSide == "old" { + if !row.Left.Empty && row.Left.Line > 0 && row.Left.Line >= start && row.Left.Line <= end { + row.Left.Selected = true + } + } else { + if !row.Right.Empty && row.Right.Line > 0 && row.Right.Line >= start && row.Right.Line <= end { + row.Right.Selected = true + } + } + } + + // Comment projection: left side uses "old", right side uses "". + if !row.Left.Empty && row.Left.Line > 0 { + row.Left.Commented, row.Left.Comments = projectLineComments(file.Path, row.Left.Line, comments, editingID, "old") + } + if !row.Right.Empty && row.Right.Line > 0 { + row.Right.Commented, row.Right.Comments = projectLineComments(file.Path, row.Right.Line, comments, editingID, "") + } + } + + hunks = append(hunks, vh) + } + return hunks +} + func buildViewLinesWithRanges(file *File, comments []Comment, start, end int, rendered []template.HTML, ranges []LineRange, editingID int) []ViewLine { if len(ranges) == 0 { return buildViewLines(file, comments, start, end, rendered, editingID) @@ -92,7 +218,7 @@ func buildSingleViewLine(file *File, comments []Comment, start, end int, rendere lineNum := idx + 1 raw := file.Lines[idx] selected := start > 0 && end > 0 && lineNum >= start && lineNum <= end - commented, lineComments := projectLineComments(file.Path, lineNum, comments, editingID) + commented, lineComments := projectLineComments(file.Path, lineNum, comments, editingID, "") lineHTML := template.HTML("") if len(rendered) > idx { @@ -116,7 +242,7 @@ func buildViewLines(file *File, comments []Comment, start, end int, rendered []t return lines } -func buildViewDiff(file *DiffFile, comments []Comment, start, end int, render bool, editingID int) ViewDiffFile { +func buildViewDiff(file *DiffFile, comments []Comment, start, end int, render bool, editingID int, selectionSide string) ViewDiffFile { view := ViewDiffFile{Path: file.Path} for _, h := range file.Hunks { hdr := fmt.Sprintf("@@ -%d,%d +%d,%d @@", h.OldStart, h.OldCount, h.NewStart, h.NewCount) @@ -139,15 +265,28 @@ func buildViewDiff(file *DiffFile, comments []Comment, start, end int, render bo if len(rendered) > i { line.HTML = rendered[i] } - selectable := dl.NewLine > 0 && dl.Kind != DiffDel - if selectable && start > 0 && end > 0 && dl.NewLine >= start && dl.NewLine <= end { - line.Selected = true + // Selection: when selectionSide == "old", check OldLine; otherwise check NewLine + if start > 0 && end > 0 { + if selectionSide == "old" { + if dl.OldLine > 0 && dl.OldLine >= start && dl.OldLine <= end { + line.Selected = true + } + } else { + if dl.NewLine > 0 && dl.NewLine >= start && dl.NewLine <= end { + line.Selected = true + } + } } + // Project comments from both sides, merge results if dl.NewLine > 0 { - line.Commented, line.Comments = projectLineComments(file.Path, dl.NewLine, comments, editingID) + line.Commented, line.Comments = projectLineComments(file.Path, dl.NewLine, comments, editingID, "") } - if !selectable { - line.Selected = false + if dl.OldLine > 0 { + oldCommented, oldComments := projectLineComments(file.Path, dl.OldLine, comments, editingID, "old") + if oldCommented { + line.Commented = true + } + line.Comments = append(line.Comments, oldComments...) } vh.Lines = append(vh.Lines, line) } @@ -156,13 +295,23 @@ func buildViewDiff(file *DiffFile, comments []Comment, start, end int, render bo return view } -func projectLineComments(path string, lineNum int, comments []Comment, editingID int) (bool, []ViewComment) { +func projectLineComments(path string, lineNum int, comments []Comment, editingID int, side string) (bool, []ViewComment) { commented := false lineComments := make([]ViewComment, 0) for _, c := range comments { if c.Path != path { continue } + // Side-aware filtering: "" matches new-side (c.Side == ""), "old" matches old-side (c.Side == "old") + if side == "" { + if c.Side != "" { + continue + } + } else { + if c.Side != side { + continue + } + } if lineNum >= c.StartLine && lineNum <= c.EndLine { commented = true } diff --git a/internal/app/view_diff_split_test.go b/internal/app/view_diff_split_test.go new file mode 100644 index 0000000..5575c83 --- /dev/null +++ b/internal/app/view_diff_split_test.go @@ -0,0 +1,374 @@ +package app + +import ( + "fmt" + "testing" +) + +// TestBuildViewDiffSplitContextLines verifies that context lines appear on +// both the left and right sides of a split-diff row. +// +// Scenario: Context lines appear on both sides +func TestBuildViewDiffSplitContextLines(t *testing.T) { + df := &DiffFile{Path: "ctx.go", Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 2, + NewStart: 1, + NewCount: 2, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 1, NewLine: 1, Text: "line one"}, + {Kind: DiffContext, OldLine: 2, NewLine: 2, Text: "line two"}, + }, + }}} + + hunks := buildViewDiffSplit(df, nil, 0, 0, false, 0, "") + + if len(hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(hunks)) + } + rows := hunks[0].Rows + if len(rows) != 2 { + t.Fatalf("expected 2 rows, got %d", len(rows)) + } + + // Row 0: context line 1 + r0 := rows[0] + if r0.Left.Line != 1 { + t.Errorf("row 0 Left.Line: got %d, want 1", r0.Left.Line) + } + if r0.Right.Line != 1 { + t.Errorf("row 0 Right.Line: got %d, want 1", r0.Right.Line) + } + if r0.Left.Text != "line one" { + t.Errorf("row 0 Left.Text: got %q, want %q", r0.Left.Text, "line one") + } + if r0.Right.Text != "line one" { + t.Errorf("row 0 Right.Text: got %q, want %q", r0.Right.Text, "line one") + } + if r0.Left.Kind != DiffContext { + t.Errorf("row 0 Left.Kind: got %q, want %q", r0.Left.Kind, DiffContext) + } + if r0.Right.Kind != DiffContext { + t.Errorf("row 0 Right.Kind: got %q, want %q", r0.Right.Kind, DiffContext) + } + if r0.Left.Empty { + t.Errorf("row 0 Left.Empty: got true, want false") + } + if r0.Right.Empty { + t.Errorf("row 0 Right.Empty: got true, want false") + } + + // Row 1: context line 2 + r1 := rows[1] + if r1.Left.Line != 2 { + t.Errorf("row 1 Left.Line: got %d, want 2", r1.Left.Line) + } + if r1.Right.Line != 2 { + t.Errorf("row 1 Right.Line: got %d, want 2", r1.Right.Line) + } + if r1.Left.Text != "line two" { + t.Errorf("row 1 Left.Text: got %q, want %q", r1.Left.Text, "line two") + } + if r1.Right.Text != "line two" { + t.Errorf("row 1 Right.Text: got %q, want %q", r1.Right.Text, "line two") + } +} + +// TestBuildViewDiffSplitDelAddPairing verifies that a del/add block is paired +// 1:1, with unpaired add lines getting an Empty left side. +// +// Scenario: Del/Add blocks: collect consecutive dels then adds, zip 1:1; +// unpaired lines get Empty: true opposite side (more adds than dels case). +func TestBuildViewDiffSplitDelAddPairing(t *testing.T) { + df := &DiffFile{Path: "pair.go", Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 2, + NewStart: 1, + NewCount: 3, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 1, NewLine: 0, Text: "old1"}, + {Kind: DiffDel, OldLine: 2, NewLine: 0, Text: "old2"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 1, Text: "new1"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 2, Text: "new2"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 3, Text: "new3"}, + }, + }}} + + hunks := buildViewDiffSplit(df, nil, 0, 0, false, 0, "") + + if len(hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(hunks)) + } + rows := hunks[0].Rows + if len(rows) != 3 { + t.Fatalf("expected 3 rows, got %d", len(rows)) + } + + // Row 0: del old1 paired with add new1 + r0 := rows[0] + if r0.Left.Line != 1 { + t.Errorf("row 0 Left.Line: got %d, want 1", r0.Left.Line) + } + if r0.Left.Kind != DiffDel { + t.Errorf("row 0 Left.Kind: got %q, want %q", r0.Left.Kind, DiffDel) + } + if r0.Left.Text != "old1" { + t.Errorf("row 0 Left.Text: got %q, want %q", r0.Left.Text, "old1") + } + if r0.Left.Empty { + t.Errorf("row 0 Left.Empty: got true, want false") + } + if r0.Right.Line != 1 { + t.Errorf("row 0 Right.Line: got %d, want 1", r0.Right.Line) + } + if r0.Right.Kind != DiffAdd { + t.Errorf("row 0 Right.Kind: got %q, want %q", r0.Right.Kind, DiffAdd) + } + if r0.Right.Text != "new1" { + t.Errorf("row 0 Right.Text: got %q, want %q", r0.Right.Text, "new1") + } + if r0.Right.Empty { + t.Errorf("row 0 Right.Empty: got true, want false") + } + + // Row 1: del old2 paired with add new2 + r1 := rows[1] + if r1.Left.Line != 2 { + t.Errorf("row 1 Left.Line: got %d, want 2", r1.Left.Line) + } + if r1.Left.Kind != DiffDel { + t.Errorf("row 1 Left.Kind: got %q, want %q", r1.Left.Kind, DiffDel) + } + if r1.Left.Text != "old2" { + t.Errorf("row 1 Left.Text: got %q, want %q", r1.Left.Text, "old2") + } + if r1.Right.Line != 2 { + t.Errorf("row 1 Right.Line: got %d, want 2", r1.Right.Line) + } + if r1.Right.Kind != DiffAdd { + t.Errorf("row 1 Right.Kind: got %q, want %q", r1.Right.Kind, DiffAdd) + } + if r1.Right.Text != "new2" { + t.Errorf("row 1 Right.Text: got %q, want %q", r1.Right.Text, "new2") + } + + // Row 2: no del to pair — left side must be empty + r2 := rows[2] + if !r2.Left.Empty { + t.Errorf("row 2 Left.Empty: got false, want true (unpaired add)") + } + if r2.Right.Line != 3 { + t.Errorf("row 2 Right.Line: got %d, want 3", r2.Right.Line) + } + if r2.Right.Kind != DiffAdd { + t.Errorf("row 2 Right.Kind: got %q, want %q", r2.Right.Kind, DiffAdd) + } + if r2.Right.Text != "new3" { + t.Errorf("row 2 Right.Text: got %q, want %q", r2.Right.Text, "new3") + } + if r2.Right.Empty { + t.Errorf("row 2 Right.Empty: got true, want false") + } +} + +// TestBuildViewDiffSplitMoreDelsThanAdds verifies that when there are more del +// lines than add lines, the unpaired del rows get an Empty right side. +// +// Scenario: Del/Add blocks: unpaired lines get Empty: true opposite side +// (more dels than adds case). +func TestBuildViewDiffSplitMoreDelsThanAdds(t *testing.T) { + df := &DiffFile{Path: "moredel.go", Hunks: []DiffHunk{{ + OldStart: 1, + OldCount: 3, + NewStart: 1, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 1, NewLine: 0, Text: "del1"}, + {Kind: DiffDel, OldLine: 2, NewLine: 0, Text: "del2"}, + {Kind: DiffDel, OldLine: 3, NewLine: 0, Text: "del3"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 1, Text: "add1"}, + }, + }}} + + hunks := buildViewDiffSplit(df, nil, 0, 0, false, 0, "") + + if len(hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(hunks)) + } + rows := hunks[0].Rows + if len(rows) != 3 { + t.Fatalf("expected 3 rows, got %d", len(rows)) + } + + // Row 0: del1 paired with add1 + r0 := rows[0] + if r0.Left.Kind != DiffDel { + t.Errorf("row 0 Left.Kind: got %q, want %q", r0.Left.Kind, DiffDel) + } + if r0.Left.Text != "del1" { + t.Errorf("row 0 Left.Text: got %q, want %q", r0.Left.Text, "del1") + } + if r0.Left.Empty { + t.Errorf("row 0 Left.Empty: got true, want false") + } + if r0.Right.Kind != DiffAdd { + t.Errorf("row 0 Right.Kind: got %q, want %q", r0.Right.Kind, DiffAdd) + } + if r0.Right.Text != "add1" { + t.Errorf("row 0 Right.Text: got %q, want %q", r0.Right.Text, "add1") + } + if r0.Right.Empty { + t.Errorf("row 0 Right.Empty: got true, want false") + } + + // Row 1: del2 unpaired — right side must be empty + r1 := rows[1] + if r1.Left.Kind != DiffDel { + t.Errorf("row 1 Left.Kind: got %q, want %q", r1.Left.Kind, DiffDel) + } + if r1.Left.Text != "del2" { + t.Errorf("row 1 Left.Text: got %q, want %q", r1.Left.Text, "del2") + } + if r1.Left.Empty { + t.Errorf("row 1 Left.Empty: got true, want false") + } + if !r1.Right.Empty { + t.Errorf("row 1 Right.Empty: got false, want true (unpaired del)") + } + + // Row 2: del3 unpaired — right side must be empty + r2 := rows[2] + if r2.Left.Kind != DiffDel { + t.Errorf("row 2 Left.Kind: got %q, want %q", r2.Left.Kind, DiffDel) + } + if r2.Left.Text != "del3" { + t.Errorf("row 2 Left.Text: got %q, want %q", r2.Left.Text, "del3") + } + if r2.Left.Empty { + t.Errorf("row 2 Left.Empty: got true, want false") + } + if !r2.Right.Empty { + t.Errorf("row 2 Right.Empty: got false, want true (unpaired del)") + } +} + +// TestBuildViewDiffSplitSelection verifies that selection is applied +// independently to each side: selectionSide="old" selects only the left (del) +// side when OldLine is in range, and the right (add) side is not selected. +// +// Scenario: Selection works on each side independently +func TestBuildViewDiffSplitSelection(t *testing.T) { + df := &DiffFile{Path: "sel.go", Hunks: []DiffHunk{{ + OldStart: 5, + OldCount: 1, + NewStart: 5, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 5, NewLine: 0, Text: "old text"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 5, Text: "new text"}, + }, + }}} + + // selectionSide="old", range [5,5]: only the del line (OldLine=5) should be selected. + hunks := buildViewDiffSplit(df, nil, 5, 5, false, 0, "old") + + if len(hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(hunks)) + } + rows := hunks[0].Rows + if len(rows) != 1 { + t.Fatalf("expected 1 row, got %d", len(rows)) + } + + r := rows[0] + // Left side (del, OldLine=5) must be selected. + if !r.Left.Selected { + t.Errorf("Left (del, OldLine=5) should be Selected when selectionSide='old' and range=[5,5]") + } + // Right side (add, OldLine=0) must NOT be selected. + if r.Right.Selected { + t.Errorf("Right (add) should NOT be selected when selectionSide='old'") + } +} + +// TestBuildViewDiffSplitComments verifies that comments are projected to the +// correct side: old-side comments project to the left, new-side comments to +// the right. +// +// Scenario: Comments project to correct side +func TestBuildViewDiffSplitComments(t *testing.T) { + df := &DiffFile{Path: "cmt.go", Hunks: []DiffHunk{{ + OldStart: 3, + OldCount: 1, + NewStart: 4, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 3, NewLine: 4, Text: "context text"}, + }, + }}} + + oldComment := Comment{ID: 10, Path: "cmt.go", StartLine: 3, EndLine: 3, Text: "old comment", Side: "old"} + newComment := Comment{ID: 11, Path: "cmt.go", StartLine: 4, EndLine: 4, Text: "new comment", Side: ""} + comments := []Comment{oldComment, newComment} + + hunks := buildViewDiffSplit(df, comments, 0, 0, false, 0, "") + + if len(hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(hunks)) + } + rows := hunks[0].Rows + if len(rows) != 1 { + t.Fatalf("expected 1 row, got %d", len(rows)) + } + + r := rows[0] + + // Left side uses "old" side for comments. + if !r.Left.Commented { + t.Errorf("Left side (OldLine=3) should be Commented by the old-side comment") + } + if len(r.Left.Comments) != 1 { + t.Errorf("Left side should have 1 comment, got %d", len(r.Left.Comments)) + } else if r.Left.Comments[0].ID != 10 { + t.Errorf("Left side comment ID: got %d, want 10", r.Left.Comments[0].ID) + } + + // Right side uses "" (new) side for comments. + if !r.Right.Commented { + t.Errorf("Right side (NewLine=4) should be Commented by the new-side comment") + } + if len(r.Right.Comments) != 1 { + t.Errorf("Right side should have 1 comment, got %d", len(r.Right.Comments)) + } else if r.Right.Comments[0].ID != 11 { + t.Errorf("Right side comment ID: got %d, want 11", r.Right.Comments[0].ID) + } +} + +// TestBuildViewDiffSplitHunkHeader verifies that the Header field of a +// returned ViewDiffSplitHunk matches the standard unified-diff format. +// +// Scenario: Hunk header is set correctly +func TestBuildViewDiffSplitHunkHeader(t *testing.T) { + df := &DiffFile{Path: "hdr.go", Hunks: []DiffHunk{{ + OldStart: 10, + OldCount: 3, + NewStart: 12, + NewCount: 4, + Lines: []DiffLine{ + {Kind: DiffContext, OldLine: 10, NewLine: 12, Text: "a"}, + {Kind: DiffContext, OldLine: 11, NewLine: 13, Text: "b"}, + {Kind: DiffContext, OldLine: 12, NewLine: 14, Text: "c"}, + }, + }}} + + hunks := buildViewDiffSplit(df, nil, 0, 0, false, 0, "") + + if len(hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(hunks)) + } + + want := fmt.Sprintf("@@ -%d,%d +%d,%d @@", 10, 3, 12, 4) + if hunks[0].Header != want { + t.Errorf("hunk Header: got %q, want %q", hunks[0].Header, want) + } +} diff --git a/internal/app/view_diff_test.go b/internal/app/view_diff_test.go index 90cb91a..d5f5db4 100644 --- a/internal/app/view_diff_test.go +++ b/internal/app/view_diff_test.go @@ -14,7 +14,7 @@ func TestBuildViewDiffCommentsNewLinesOnly(t *testing.T) { }, }}} comments := []Comment{{ID: 1, Path: "x.go", StartLine: 1, EndLine: 1, Text: "hi"}} - view := buildViewDiff(df, comments, 1, 1, false, 0) + view := buildViewDiff(df, comments, 1, 1, false, 0, "") if len(view.Hunks) != 1 { t.Fatalf("expected 1 hunk") } @@ -32,3 +32,163 @@ func TestBuildViewDiffCommentsNewLinesOnly(t *testing.T) { t.Fatalf("expected 1 comment on added line") } } + +// TestBuildViewDiffOldLineSelection verifies that when selectionSide == "old", +// a deleted line whose OldLine falls in [start, end] is marked Selected. +// +// Scenario: selectionSide "old" selects deleted line by OldLine number +func TestBuildViewDiffOldLineSelection(t *testing.T) { + df := &DiffFile{Path: "f.go", Hunks: []DiffHunk{{ + OldStart: 5, + OldCount: 1, + NewStart: 5, + NewCount: 1, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 5, NewLine: 0, Text: "deleted text"}, + {Kind: DiffAdd, OldLine: 0, NewLine: 5, Text: "added text"}, + }, + }}} + + // selectionSide="old", select range [5,5]: only the del line (OldLine==5) should be selected. + view := buildViewDiff(df, nil, 5, 5, false, 0, "old") + + if len(view.Hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(view.Hunks)) + } + lines := view.Hunks[0].Lines + if len(lines) != 2 { + t.Fatalf("expected 2 lines, got %d", len(lines)) + } + + delLine := lines[0] + if delLine.Kind != DiffDel { + t.Fatalf("lines[0] should be DiffDel, got %q", delLine.Kind) + } + if !delLine.Selected { + t.Errorf("deleted line with OldLine=5 should be Selected when selectionSide='old' and range=[5,5]") + } + + addLine := lines[1] + if addLine.Kind != DiffAdd { + t.Fatalf("lines[1] should be DiffAdd, got %q", addLine.Kind) + } + // The add line has OldLine=0, so with selectionSide="old" it must NOT be selected. + if addLine.Selected { + t.Errorf("added line (OldLine=0) should NOT be selected when selectionSide='old'") + } +} + +// TestBuildViewDiffOldLineComments verifies that comments with Side "old" appear +// on deleted lines and comments with Side "" appear on context/new lines. +// +// Scenario: old-side comment projects onto deleted line via OldLine +// Scenario: new-side comment projects onto context line via NewLine +func TestBuildViewDiffOldLineComments(t *testing.T) { + // del line: OldLine=3, NewLine=0 + // context line: OldLine=3, NewLine=4 (same old number as del, different new number) + df := &DiffFile{Path: "g.go", Hunks: []DiffHunk{{ + OldStart: 3, + OldCount: 2, + NewStart: 3, + NewCount: 2, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 3, NewLine: 0, Text: "removed line"}, + {Kind: DiffContext, OldLine: 4, NewLine: 4, Text: "context line"}, + }, + }}} + + oldSideComment := Comment{ID: 10, Path: "g.go", StartLine: 3, EndLine: 3, Text: "old comment", Side: "old"} + newSideComment := Comment{ID: 11, Path: "g.go", StartLine: 4, EndLine: 4, Text: "new comment", Side: ""} + comments := []Comment{oldSideComment, newSideComment} + + view := buildViewDiff(df, comments, 0, 0, false, 0, "") + + if len(view.Hunks) != 1 { + t.Fatalf("expected 1 hunk, got %d", len(view.Hunks)) + } + lines := view.Hunks[0].Lines + if len(lines) != 2 { + t.Fatalf("expected 2 lines, got %d", len(lines)) + } + + delLine := lines[0] + if delLine.Kind != DiffDel { + t.Fatalf("lines[0] should be DiffDel") + } + // Deleted line should show the old-side comment (Side="old", OldLine=3). + if !delLine.Commented { + t.Errorf("deleted line (OldLine=3) should be Commented via the old-side comment") + } + if len(delLine.Comments) != 1 { + t.Errorf("deleted line should have 1 comment, got %d", len(delLine.Comments)) + } else if delLine.Comments[0].ID != 10 { + t.Errorf("deleted line comment should have ID=10, got %d", delLine.Comments[0].ID) + } + + ctxLine := lines[1] + if ctxLine.Kind != DiffContext { + t.Fatalf("lines[1] should be DiffContext") + } + // Context line should show the new-side comment (Side="", NewLine=4). + if !ctxLine.Commented { + t.Errorf("context line (NewLine=4) should be Commented via the new-side comment") + } + if len(ctxLine.Comments) != 1 { + t.Errorf("context line should have 1 comment, got %d", len(ctxLine.Comments)) + } else if ctxLine.Comments[0].ID != 11 { + t.Errorf("context line comment should have ID=11, got %d", ctxLine.Comments[0].ID) + } +} + +// TestBuildViewDiffDeletedLinesSelectable verifies that deleted lines are no +// longer blocked by the selectable guard and can be selected when +// selectionSide == "old". +// +// Scenario: deleted lines not filtered out — always included in output +// Scenario: deleted line selected when selectionSide="old" and OldLine in range +func TestBuildViewDiffDeletedLinesSelectable(t *testing.T) { + df := &DiffFile{Path: "h.go", Hunks: []DiffHunk{{ + OldStart: 10, + OldCount: 1, + NewStart: 11, + NewCount: 0, + Lines: []DiffLine{ + {Kind: DiffDel, OldLine: 10, NewLine: 0, Text: "removed"}, + }, + }}} + + // First call: selectionSide="" with no selection range — line must appear in output. + view := buildViewDiff(df, nil, 0, 0, false, 0, "") + + if len(view.Hunks) != 1 { + t.Fatalf("case 1: expected 1 hunk, got %d", len(view.Hunks)) + } + lines := view.Hunks[0].Lines + if len(lines) != 1 { + t.Fatalf("case 1: expected 1 line (del line must not be filtered), got %d", len(lines)) + } + if lines[0].Kind != DiffDel { + t.Fatalf("case 1: expected DiffDel line, got %q", lines[0].Kind) + } + if lines[0].OldLine != 10 { + t.Errorf("case 1: expected OldLine=10, got %d", lines[0].OldLine) + } + // With no selection range the line must NOT be selected. + if lines[0].Selected { + t.Errorf("case 1: del line should not be selected when start=0, end=0") + } + + // Second call: selectionSide="old", range covers OldLine=10 — del line must be selected. + view2 := buildViewDiff(df, nil, 10, 10, false, 0, "old") + + if len(view2.Hunks) != 1 { + t.Fatalf("case 2: expected 1 hunk, got %d", len(view2.Hunks)) + } + lines2 := view2.Hunks[0].Lines + if len(lines2) != 1 { + t.Fatalf("case 2: expected 1 line, got %d", len(lines2)) + } + if !lines2[0].Selected { + t.Errorf("case 2: del line (OldLine=10) should be Selected when selectionSide='old' and range=[10,10]") + } +} diff --git a/internal/ui/styles.css b/internal/ui/styles.css index a608c5c..f65f068 100644 --- a/internal/ui/styles.css +++ b/internal/ui/styles.css @@ -774,7 +774,7 @@ textarea { } .diff { - background: var(--panel); + background: #000000; border: 1px solid var(--border); border-radius: 0; padding: 0; @@ -795,9 +795,13 @@ textarea { border-bottom: 1px solid var(--border); } +.diff-inner { + display: block; +} + .diff-line { display: grid; - grid-template-columns: 4ch 4ch 1ch max-content; + grid-template-columns: 4ch 4ch 1ch 1fr; gap: 12px; padding: 0 20px; align-items: baseline; @@ -826,11 +830,11 @@ textarea { } .diff-line[data-kind="add"] { - background: rgba(46, 160, 67, 0.12); + background: rgba(46, 160, 67, 0.25); } .diff-line[data-kind="del"] { - background: rgba(248, 81, 73, 0.16); + background: rgba(248, 81, 73, 0.25); } .diff-sign.add { @@ -851,7 +855,9 @@ textarea { } .diff-line .code-text { - white-space: pre; + white-space: pre-wrap; + overflow-wrap: break-word; + min-width: 0; font-family: Menlo, Consolas, Monaco, Adwaita Mono, Liberation Mono, Lucida Console, monospace; letter-spacing: 0; word-spacing: 0; @@ -860,12 +866,17 @@ textarea { .diff-line .code-text * { font-family: Menlo, Consolas, Monaco, Adwaita Mono, Liberation Mono, Lucida Console, monospace; - white-space: pre; + white-space: pre-wrap; + overflow-wrap: break-word; letter-spacing: 0; word-spacing: 0; font-variant-ligatures: none; } +.chroma { + background-color: transparent !important; +} + .diff-line:hover { background: var(--line-hover); } @@ -906,3 +917,99 @@ textarea { .edit-comment-form textarea { min-height: 70px; } + +/* Side-by-side diff layout */ +.diff-split { + overflow-x: hidden; +} + +.diff-row-split { + display: grid; + grid-template-columns: 1fr 1fr; +} + +.diff-cell { + display: grid; + grid-template-columns: 4ch 1ch 1fr; + gap: 12px; + padding: 0 12px; + align-items: baseline; + cursor: pointer; + overflow: hidden; + min-width: 0; +} + +.diff-cell.empty { + cursor: default; +} + +.diff-cell[data-kind="add"] { + background: rgba(46, 160, 67, 0.25); +} + +.diff-cell[data-kind="del"] { + background: rgba(248, 81, 73, 0.25); +} + +.diff-cell:hover:not(.empty) { + background: var(--line-hover); +} + +.diff-cell.selected { + background: var(--line-selected); +} + +.diff-cell.commented .ln { + color: var(--accent); + font-weight: 700; +} + +.diff-cell .ln { + color: var(--muted); + text-align: right; + padding-right: 8px; + user-select: none; + cursor: pointer; + font-variant-numeric: tabular-nums; + font-feature-settings: "tnum"; + font-family: Menlo, Consolas, Monaco, Adwaita Mono, Liberation Mono, Lucida Console, monospace; + width: 4ch; +} + +.diff-cell .diff-sign { + color: var(--muted); + text-align: center; + width: 1ch; +} + +.diff-cell .diff-sign.add { + color: #2ea043; +} + +.diff-cell .diff-sign.del { + color: #f85149; +} + +.diff-cell .code-text { + white-space: pre-wrap; + overflow-wrap: break-word; + min-width: 0; + font-family: Menlo, Consolas, Monaco, Adwaita Mono, Liberation Mono, Lucida Console, monospace; + letter-spacing: 0; + word-spacing: 0; + font-variant-ligatures: none; +} + +.diff-cell .code-text * { + font-family: Menlo, Consolas, Monaco, Adwaita Mono, Liberation Mono, Lucida Console, monospace; + white-space: pre-wrap; + overflow-wrap: break-word; + letter-spacing: 0; + word-spacing: 0; + font-variant-ligatures: none; +} + +/* Border separator between left and right cells */ +.diff-cell:first-child { + border-right: 1px solid var(--border); +} diff --git a/internal/ui/template.html b/internal/ui/template.html index 4a068bb..49c0d55 100644 --- a/internal/ui/template.html +++ b/internal/ui/template.html @@ -62,6 +62,14 @@ {{end}}
+ {{if eq .Mode "diff"}} + + {{end}}
-
+
{{if eq $root.Mode "diff"}} + {{if eq $root.DiffFormat "split"}} +
+
+ {{range .ViewDiffSplit}} +
{{.Header}}
+ {{range .Rows}} +
+ {{if .Left.Empty}} +
+ {{else}} +
+ {{if gt .Left.Line 0}}{{.Left.Line}}{{end}} + {{if eq .Left.Kind "add"}}+{{else if eq .Left.Kind "del"}}-{{else}} {{end}} +
+ {{if $root.RenderFile}}{{.Left.HTML}}{{else}}{{.Left.Text}}{{end}} +
+
+ {{end}} + {{if .Right.Empty}} +
+ {{else}} +
+ {{if gt .Right.Line 0}}{{.Right.Line}}{{end}} + {{if eq .Right.Kind "add"}}+{{else if eq .Right.Kind "del"}}-{{else}} {{end}} +
+ {{if $root.RenderFile}}{{.Right.HTML}}{{else}}{{.Right.Text}}{{end}} +
+
+ {{end}} +
+ {{if or .Left.Comments .Right.Comments}} +
+ {{if .Left.Comments}} + {{template "commentThread" (commentThreadData $root .Left.Comments $.Logo)}} + {{end}} + {{if .Right.Comments}} + {{template "commentThread" (commentThreadData $root .Right.Comments $.Logo)}} + {{end}} +
+ {{end}} + {{if and (gt $root.SelectionEnd 0) (or (and (ne $root.SelectionSide "old") (not .Right.Empty) (eq .Right.Line $root.SelectionEnd)) (and (eq $root.SelectionSide "old") (not .Left.Empty) (eq .Left.Line $root.SelectionEnd)))}} +
+
+
Selected: {{$root.SelectionStart}}-{{$root.SelectionEnd}}
+ + {{if $root.Error}}
{{$root.Error}}
{{end}} +
+ + +
+
+
+ {{end}} + {{end}} + {{end}} +
+
+ {{else}}
+
{{range .ViewDiff.Hunks}}
{{.Header}}
{{range .Lines}} -
+
{{if gt .OldLine 0}}{{.OldLine}}{{end}} {{if gt .NewLine 0}}{{.NewLine}}{{end}} {{if eq .Kind "add"}}+{{else if eq .Kind "del"}}-{{else}} {{end}} @@ -133,7 +200,7 @@ {{template "commentThread" (commentThreadData $root .Comments $.Logo)}}
{{end}} - {{if and (gt $root.SelectionEnd 0) (eq .NewLine $root.SelectionEnd) (ne .Kind "del")}} + {{if and (gt $root.SelectionEnd 0) (or (and (eq .NewLine $root.SelectionEnd) (ne .Kind "del") (ne $root.SelectionSide "old")) (and (eq $root.SelectionSide "old") (eq .OldLine $root.SelectionEnd)))}}
Selected: {{$root.SelectionStart}}-{{$root.SelectionEnd}}
@@ -148,7 +215,9 @@ {{end}} {{end}} {{end}} +
+ {{end}} {{else}} {{if and .ViewFile.MarkdownFile .ViewFile.MarkdownRendered}}
@@ -219,15 +288,6 @@ mounted: function () { const root = this.el; - // Restore sidebar width from localStorage - (function () { - var saved = null; - try { saved = localStorage.getItem("meatcheck-sidebar-width"); } catch (e) {} - if (saved) { - document.documentElement.style.setProperty("--sidebar-width", saved); - } - })(); - // Drag-to-resize sidebar (function () { var handle = root.querySelector(".sidebar-resize-handle"); @@ -251,14 +311,18 @@ var delta = ev.clientX - startX; var newWidth = Math.max(140, Math.min(600, startWidth + delta)); var val = newWidth + "px"; - document.documentElement.style.setProperty("--sidebar-width", val); - try { localStorage.setItem("meatcheck-sidebar-width", val); } catch (e) {} + root.querySelector(".workspace").style.setProperty("--sidebar-width", val); }); document.addEventListener("mouseup", function () { if (!dragging) return; dragging = false; document.body.style.cursor = ""; document.body.style.userSelect = ""; + var sidebar = root.querySelector(".sidebar"); + if (sidebar && window.Live && typeof window.Live.send === "function") { + var w = Math.round(sidebar.getBoundingClientRect().width) + "px"; + window.Live.send("save-sidebar-width", { width: w }); + } }); })(); @@ -275,12 +339,20 @@ const target = ev.target; if (!target || !(target instanceof Element)) return; if (target.closest(".inline-comment, .line-comment-thread, .comment-form")) return; - const clickable = target.closest("[data-line]"); + const clickable = target.closest("[data-line], [data-old-line]"); if (!clickable) return; - const line = Number(clickable.dataset.line || 0); - if (!line) return; + var line = Number(clickable.dataset.line || 0); const lineEnd = Number(clickable.dataset.lineEnd || 0); + const oldLine = Number(clickable.dataset.oldLine || 0); const shift = ev.shiftKey ? "1" : ""; + // If NewLine is 0, fall back to OldLine + if (!line && oldLine) { + if (window.Live && typeof window.Live.send === "function") { + window.Live.send("select-line", { line: "0", old_line: String(oldLine), shift: shift }); + } + return; + } + if (!line) return; if (window.Live && typeof window.Live.send === "function") { window.Live.send("select-line", { line: line, line_end: lineEnd || line, shift: shift }); }