diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fcee99..db4e692 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,32 @@ pre-1.0). ## [Unreleased] +### Added + ### Fixed +- `rift diff --watch` live-reloads the browser as the working tree changes — + built for reviewing while an agent edits in another pane. The file list, + stats, and diffs refresh in place (the selection survives), the tests lens + re-collects, and content-keyed reviewed marks reset as files change, so the + unreviewed filter always shows what still needs eyes. A `watching` tag in + the header shows it's live, and `w` toggles it without restarting — + enabling catches up on anything that changed while it was off. +- `rift skill` prints a built-in agent skill that teaches an AI agent rift's + composable review workflow (`--json` changeset inspection, structural + `--print` diffs, the `--tests` lens, review state). `rift skill path` writes + it to `~/.local/share/rift/skills/rift-review/SKILL.md` and prints the path, + so you can tell an agent "run `rift skill path` and follow that skill". +- Committed-range and base-ref diffs now detect renames: a moved file lists as + one `R` record showing only its real content delta (with an `old → new` + preview title and `old_path` in `--json`) instead of a full delete plus add. + The tests lens reads a renamed file's old side from its old path, so its + specs no longer all report as newly added. Working-tree and staged listings + keep git-status semantics (the stage screen operates on real index paths). + +- In a base-ref diff (`rift diff main`), returning from the editor (`o`) no + longer replaces the file list with the plain worktree-vs-index set — the + reload now honors the base ref. - `install.sh` no longer fails checksum verification on macOS with a matching hash: it now compares hash strings directly instead of relying on `-c` check mode, whose behavior differs between GNU `sha256sum` and macOS's `shasum`. @@ -184,7 +208,7 @@ pre-1.0). - `rift diff` shows the file list and toggles to the tests view live with `t` (the header reads `rift ❯ diff ❯ tests` to show it's a lens); `--tests` just opens straight into it. Works on the working tree, `--staged`, or a ` - ` range, with `--print`/`--json` for scripting. The diff preview +` range, with `--print`/`--json` for scripting. The diff preview scrolls to the selected test and labels it in the panel's border, so a body-only change stays named even when its declaration isn't in the diff; `o` opens it in `$EDITOR` at its line. @@ -247,7 +271,7 @@ pre-1.0). ## [0.2.0] - 2026-05-31 -A big step toward being a daily driver for *reading* git state: the diff and log +A big step toward being a daily driver for _reading_ git state: the diff and log browsers got a top-to-bottom presentation overhaul, and the tool's scope tightened around what it does best. @@ -275,7 +299,7 @@ tightened around what it does best. quitting; quit stays on `q` / `ctrl+c`. - Upgraded to the Charm v2 ecosystem (bubbletea / lipgloss / bubbles) and extracted a reusable split-list component shared across screens. -- Repositioned as *worktree-aware* — every command reads correctly inside +- Repositioned as _worktree-aware_ — every command reads correctly inside bare-repo and linked-worktree layouts — rather than worktree-managing. Pair with [worktrunk](https://github.com/max-sixty/worktrunk) for managing worktrees and branches. diff --git a/CLAUDE.md b/CLAUDE.md index a8007bd..4da817e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,6 +38,7 @@ internal/ # private packages review/ # review state (reviewed marks) + test-case extraction tooling/ # external tool management (difftastic download/detect) output/ # --print / --json composable output + skill/ # embedded agent skill (SKILL.md) + materialization main.go # entrypoint ``` @@ -50,6 +51,9 @@ design intent only — do not assume the packages or tools exist: - **Syntax-aware merge / conflict resolution:** mergiraf (`internal/merge`) - **Checkpoints:** shadow-commit snapshots on hidden refs (`internal/checkpoint`) - **Risk classification** in review — today `internal/review` only tracks reviewed marks and extracts test cases +- **Inline agent annotations:** `rift annotate` writing to a JSON store next to the reviewed marks (`.git/rift/annotations.json`), rendered in the diff preview and picked up by `--watch`. File-based on purpose — no daemon/socket (see the design doc's Future Considerations) +- **Mouse support:** wheel scroll + click-to-select via bubbletea mouse events; must keep terminal text selection workable (see design doc) +- **Moved-line diff view:** a config option swapping the git engine's flags to `--color-moved=zebra --color-moved-ws=allow-indentation-change` (mutually exclusive with the default `--word-diff=color` — git silently ignores `--color-moved` under word-diff, so it can't be a combined default; see `displayFlags` in `internal/diff`) ## Workflow diff --git a/README.md b/README.md index 0a18770..1869df2 100644 --- a/README.md +++ b/README.md @@ -10,12 +10,14 @@ It wraps the git workflows where UX is the bottleneck — diffing, staging, log rift # contextual launchpad rift diff # syntax-aware diff browser rift diff --tests # review a change by the tests it touches +rift diff --watch # live-reloading review while an agent edits rift stage # interactive staging with hunk granularity rift log # structural commit explorer rift stash # stash manager with diff preview +rift skill # built-in skill that teaches agents the review workflow ``` -rift is **worktree-aware** — every command reads correctly inside bare-repo and linked-worktree layouts — but it does not *manage* worktrees or branches. For creating, switching, and pruning worktrees, pair it with [worktrunk](https://github.com/max-sixty/worktrunk). +rift is **worktree-aware** — every command reads correctly inside bare-repo and linked-worktree layouts — but it does not _manage_ worktrees or branches. For creating, switching, and pruning worktrees, pair it with [worktrunk](https://github.com/max-sixty/worktrunk).

rift diff — structural diff browser with a one-row peek and a per-file legend @@ -29,13 +31,35 @@ rift is **worktree-aware** — every command reads correctly inside bare-repo an rift occupies the space between them: **transient** (invoke, act, return to shell), **structural** (diffs understand your code's syntax), and **composable** (every command has `--print` and `--json` modes). -The bet behind it: as agents write more of the code, the scarce human activity becomes *reading* it — so a git tool should be built for review first, and none of the above are. +The bet behind it: as agents write more of the code, the scarce human activity becomes _reading_ it — so a git tool should be built for review first, and none of the above are. + +### How it compares + +A new cohort of review-first diff viewers — [hunk](https://github.com/modem-dev/hunk) most notably — shares that bet. rift's angle within it: structural diffs, review that goes deeper than viewing (tracking, test impact, staging, history), and composable output on everything. + +| Capability | rift | [hunk](https://github.com/modem-dev/hunk) | [lazygit](https://github.com/jesseduffield/lazygit) | [forgit](https://github.com/wfxr/forgit) | [delta](https://github.com/dandavison/delta) | [difftastic](https://difftastic.wilfred.me.uk/) | +| ---------------------------------------- | :--: | :---------------------------------------: | :-------------------------------------------------: | :--------------------------------------: | :------------------------------------------: | :---------------------------------------------: | +| Structural (syntax-aware) diffs | ✅ | ❌ | ❌ | ❌ | ❌ | ✅ | +| Interactive multi-file review | ✅ | ✅ | ✅ | ✅ | ❌ | ❌ | +| Review tracking (content-keyed marks) | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | +| Test-impact lens (`--tests`) | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | +| Interactive hunk staging | ✅ | ❌ | ✅ | ✅ | ❌ | ❌ | +| Log & stash browsing | ✅ | ❌ | ✅ | ✅ | ❌ | ❌ | +| `--print` / `--json` on every command | ✅ | ❌ | ❌ | ❌ | — | ❌ | +| Watch mode (live reload) | ✅ | ✅ | ✅ | ❌ | — | ❌ | +| Moved-line detection | 🔜 | ✅ | ❌ | ❌ | ❌ | ❌ | +| Transient — invoke, act, return to shell | ✅ | ✅ | ❌ | ✅ | — | — | +| Single static binary | ✅ | ❌ | ✅ | ❌ | ✅ | ✅ | +| Inline agent annotations | 🔜 | ✅ | ❌ | ❌ | ❌ | ❌ | +| Mouse support | 🔜 | ✅ | ✅ | ❌ | — | — | + +✅ yes · ❌ no · — not applicable · 🔜 planned. delta and difftastic are diff renderers/pagers rather than browsers, so several rows don't apply — rift uses difftastic as its diff engine and falls back to a built-in line diff without it. ## Key Features ### Structural diffs -Powered by difftastic: reformatting noise disappears and you see what actually changed at the expression level, with syntax highlighting and intra-line emphasis. When difftastic isn't on your `$PATH`, rift falls back to git's own diff (with word-level and whitespace highlighting) — toggle between the two engines with `e`. +Powered by difftastic: reformatting noise disappears and you see what actually changed at the expression level, with syntax highlighting and intra-line emphasis. When difftastic isn't on your `$PATH`, rift falls back to git's own diff (with word-level and whitespace highlighting) — toggle between the two engines with `e`. In commit-range and base-ref diffs, a renamed file lists as a single `old → new` entry showing only its real content delta, not a delete plus an add. ![A structural diff of a refactor](docs/images/diff.png) @@ -51,7 +75,7 @@ Powered by difftastic: reformatting noise disappears and you see what actually c ### Test lens -Add `--tests` to `rift diff` or `rift log` to read a change by the *tests* it touches instead of its files — handy for reviewing agent-generated code. Each spec is marked by how the diff changed it: `+` added, `→` renamed, `~` modified (the `~` cases, where an assertion can be quietly weakened under an unchanged name, are the ones worth a close read). The preview scrolls to the selected test and names it in the border. Toggle live between the file view and the tests view with `t`; works on the working tree, `--staged`, or a commit range, and supports `--print` / `--json`. Tests are extracted across Go, JavaScript/TypeScript, Ruby, Python, and Rust — including `t.Run` subtests and table-driven cases. +Add `--tests` to `rift diff` or `rift log` to read a change by the _tests_ it touches instead of its files — handy for reviewing agent-generated code. Each spec is marked by how the diff changed it: `+` added, `→` renamed, `~` modified (the `~` cases, where an assertion can be quietly weakened under an unchanged name, are the ones worth a close read). The preview scrolls to the selected test and names it in the border. Toggle live between the file view and the tests view with `t`; works on the working tree, `--staged`, or a commit range, and supports `--print` / `--json`. Tests are extracted across Go, JavaScript/TypeScript, Ruby, Python, and Rust — including `t.Run` subtests and table-driven cases. ![rift diff --tests — the tests a change added, renamed, or modified](docs/images/tests.gif) @@ -61,6 +85,10 @@ In `rift diff`, press `r` to mark the selected file **reviewed** (a `✓` replac ![rift diff — marking changes reviewed and filtering to what's left](docs/images/reviewed.gif) +### Watch mode + +`rift diff --watch` keeps the browser live while something else edits the working tree — typically an agent writing code in another pane. The file list, stats, and diffs refresh in place (your selection survives), the tests lens re-collects, and because reviewed marks are content-keyed they reset the moment a file changes again — so the unreviewed filter always shows exactly what still needs eyes. A `watching` tag in the header shows it's live. Works with any working-tree scope, including a base ref (`rift diff main --watch`) and the staged side — and toggles live with `w`, so you can turn it on the moment an agent starts working without losing your place (enabling catches up on anything that changed while it was off). + ### Interactive staging `rift stage` replaces `git add -p` with a two-panel TUI: a file list with structural diff preview and hunk-level staging, the active hunk clearly marked in the gutter. @@ -112,30 +140,30 @@ Every work screen (`diff`, `log`, `stash`, `stage`, and the tests lens) shares t **Shared keys** -| Key | Action | -| --- | --- | -| `j`/`k`, `↑`/`↓` | move selection / scroll the preview | -| `J`/`K`, `⇧↑`/`⇧↓`, `]`/`[` | step to the next / previous item (even while reading) | -| `gg`/`G` | jump to first / last | -| `{`/`}` | previous / next diff section (previous / next hunk in `stage`) | -| `ctrl-d`/`ctrl-u` | half-page down / up | -| `ctrl-f`/`ctrl-b` | page down / up | -| `⇥` | switch between the list and preview panes | -| `/` | fuzzy-filter (`⏎` accept, `esc` clear) | -| `y` | yank the selection (path, commit hash, or stash ref) | -| `?` | keybinding overlay | -| `q`, `ctrl-c` | quit | -| `esc` | leave the current mode (filter, help, drilldown) — never quits a work screen | +| Key | Action | +| --------------------------- | ---------------------------------------------------------------------------- | +| `j`/`k`, `↑`/`↓` | move selection / scroll the preview | +| `J`/`K`, `⇧↑`/`⇧↓`, `]`/`[` | step to the next / previous item (even while reading) | +| `gg`/`G` | jump to first / last | +| `{`/`}` | previous / next diff section (previous / next hunk in `stage`) | +| `ctrl-d`/`ctrl-u` | half-page down / up | +| `ctrl-f`/`ctrl-b` | page down / up | +| `⇥` | switch between the list and preview panes | +| `/` | fuzzy-filter (`⏎` accept, `esc` clear) | +| `y` | yank the selection (path, commit hash, or stash ref) | +| `?` | keybinding overlay | +| `q`, `ctrl-c` | quit | +| `esc` | leave the current mode (filter, help, drilldown) — never quits a work screen | **Per-screen keys** -| Screen | Keys | -| --- | --- | -| `diff` | `s` toggle staged · `t` tests lens · `\` diff layout · `e` engine · `o` open in `$EDITOR` · `r` mark reviewed · `U` show only unreviewed | -| `log` | `⏎` drill into the commit's files (or tests) · `\` layout · `e` engine · `c` cherry-pick · `r` revert | -| `stash` | `a` apply · `p` pop · `x` drop · `\` layout · `e` engine | -| `stage` | `s` stage · `u` unstage · `a` stage all · `o` open in `$EDITOR` · `e` engine (fixed layout, no `\`) | -| tests lens | `t` back to files · `s` toggle staged · `\` layout · `e` engine · `o` open the test | +| Screen | Keys | +| ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `diff` | `s` toggle staged · `w` toggle watch · `t` tests lens · `\` diff layout · `e` engine · `o` open in `$EDITOR` · `r` mark reviewed · `U` show only unreviewed | +| `log` | `⏎` drill into the commit's files (or tests) · `\` layout · `e` engine · `c` cherry-pick · `r` revert | +| `stash` | `a` apply · `p` pop · `x` drop · `\` layout · `e` engine | +| `stage` | `s` stage · `u` unstage · `a` stage all · `o` open in `$EDITOR` · `e` engine (fixed layout, no `\`) | +| tests lens | `t` back to files · `s` toggle staged · `w` toggle watch · `\` layout · `e` engine · `o` open the test | The `menu` launchpad is a plain list: `⏎` selects, `/` filters, and `esc`/`q` quit. Destructive actions (`stash` pop/drop, `log` cherry-pick/revert) confirm inline with `y`/`n`. @@ -162,7 +190,7 @@ curl -fsSL https://raw.githubusercontent.com/madhermit/rift/main/install.sh | ba Prebuilt binaries for Linux and macOS (amd64 and arm64), plus the `SHA256SUMS` file, are attached to every [Release](https://github.com/madhermit/rift/releases) if you'd rather download and verify by hand. -> **Installing with `go install`?** `go install github.com/madhermit/rift@latest` works, but it builds *without* the `grammar_subset*` tags that trim gotreesitter's embedded tree-sitter grammars — so the binary is much larger (~43 MB vs ~21 MB for a release build), and `rift version` reports the module version rather than a release tag. To match the release build, clone the repo and run `mise run install`, or build with the tags yourself: +> **Installing with `go install`?** `go install github.com/madhermit/rift@latest` works, but it builds _without_ the `grammar_subset*` tags that trim gotreesitter's embedded tree-sitter grammars — so the binary is much larger (~43 MB vs ~21 MB for a release build), and `rift version` reports the module version rather than a release tag. To match the release build, clone the repo and run `mise run install`, or build with the tags yourself: > > ```bash > go build -tags "$GRAMMAR_TAGS" . # GRAMMAR_TAGS as defined in mise.toml @@ -178,17 +206,32 @@ The `--json` output on every command gives agents structural understanding that ```bash # inspect changes as structured data -rift diff --json | jq '.[] | select(.status == "modified")' +rift diff --json | jq '.[] | select(.status == "Modified")' + +# the tests a change added, renamed, or modified +rift diff --tests --json + +# the files a human hasn't (re-)reviewed yet +rift diff --unreviewed --name-only # list recent commits rift log --json -n 10 | jq '.[].hash' ``` +rift also ships a built-in [skill](internal/skill/SKILL.md) that teaches an agent the whole review workflow — inspecting a changeset, reading structural diffs, checking test impact, and respecting review state: + +```bash +rift skill # print it +rift skill path # write it to a stable path and print the location +``` + +Tell your agent: _"Run `rift skill path` and follow that skill to review this change."_ Pair it with `rift diff --watch` in your own terminal and the review loop closes — you watch files tick off (and re-surface) as the agent works. + ## Status -The core consumption commands — `diff`, `log`, `stash`, `stage` — are implemented and meant to be a daily driver (latest tag **v0.6.0**), with ongoing work on the diff/log reading experience: colored file icons, scrollbars, sticky file headers, and a structural fallback when difftastic is absent. Config and local code review are planned next. +The core consumption commands — `diff`, `log`, `stash`, `stage` — are implemented and meant to be a daily driver (latest tag **v0.6.0**), with ongoing work on the diff/log reading experience: colored file icons, scrollbars, sticky file headers, and a structural fallback when difftastic is absent. Planned next: config, local code review, inline agent annotations (file-based, in the same store as reviewed marks — no daemon), and mouse support. -Worktree and branch *management* are intentionally out of scope — rift stays worktree-aware and pairs with [worktrunk](https://github.com/max-sixty/worktrunk). +Worktree and branch _management_ are intentionally out of scope — rift stays worktree-aware and pairs with [worktrunk](https://github.com/max-sixty/worktrunk). See [CHANGELOG.md](CHANGELOG.md) for release notes. diff --git a/cmd/diff.go b/cmd/diff.go index 1608cce..0fab776 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -26,6 +26,7 @@ func init() { diffCmd.Flags().Bool("name-only", false, "Only show changed file names") diffCmd.Flags().Bool("tests", false, "Show the test cases the diff touches instead of files") diffCmd.Flags().Bool("unreviewed", false, "Only show files not yet marked reviewed") + diffCmd.Flags().Bool("watch", false, "Reload automatically as the working tree changes") rootCmd.AddCommand(diffCmd) } @@ -61,6 +62,16 @@ func runDiff(cmd *cobra.Command, args []string) error { return fmt.Errorf("--staged cannot be combined with a commit argument") } + // --watch reloads the TUI as the working tree changes: a committed range is + // immutable, and a non-interactive listing has nothing to reload into. + watch, _ := cmd.Flags().GetBool("watch") + if watch && target != "" { + return fmt.Errorf("--watch requires a working-tree diff, not a commit range") + } + if watch && (mode != output.Interactive || nameOnly) { + return fmt.Errorf("--watch requires the interactive TUI") + } + // Committed() keys on Target, so a working-tree scope ignores Base and a // committed range ignores Staged — one literal covers both. scope := review.DiffScope{Staged: staged, Base: base, Target: target, Paths: pathArgs} @@ -74,11 +85,10 @@ func runDiff(cmd *cobra.Command, args []string) error { return runTestsLens(mode, repo, scope) } - files, err := listChangedFiles(repo, staged, base, target) + files, err := repo.ListChanged(staged, base, target, pathArgs...) if err != nil { return err } - files = git.FilterByPaths(files, pathArgs) // --unreviewed narrows any non-interactive listing to files not yet marked // reviewed (a working-tree concept). --name-only prints a plain listing even @@ -102,39 +112,12 @@ func runDiff(cmd *cobra.Command, args []string) error { case output.Print: return printDiffs(engine, repo, files, staged, base, target) default: - m := lensui.New(repo, engine, files, scope, tests, unrev) + m := lensui.New(repo, engine, files, scope, tests, unrev, watch) _, err := tui.NewProgram(m).Run() return err } } -func listChangedFiles(repo *git.Repo, staged bool, base, target string) ([]git.ChangedFile, error) { - var ( - files []git.ChangedFile - err error - ) - switch { - case target != "": - // Committed range: base..target, tree to tree. - files, err = repo.DiffBetweenCommits(base, target) - case base != "": - // Single ref: working tree vs base, so committed changes since the ref - // show up too (not just the worktree-vs-HEAD delta), matching the - // per-file preview which also diffs against base. - files, err = repo.DiffAgainstRef(base) - default: - files, err = repo.ChangedFiles(staged) - } - if err != nil { - return nil, err - } - if files == nil { - files = []git.ChangedFile{} - } - git.SortByPath(files) - return files, nil -} - // filterUnreviewed drops files whose current content is marked reviewed. func filterUnreviewed(repo *git.Repo, files []git.ChangedFile) []git.ChangedFile { return review.LoadReviewed(repo).Unreviewed(files, review.ContentHashes(repo, files)) @@ -170,7 +153,7 @@ func fileDiff(ctx context.Context, engine diff.Engine, repo *git.Repo, f git.Cha if f.Status == "Untracked" { return diff.RawNewFileDiff(repo.Root(), f.Path) } - return engine.Diff(ctx, repo.Root(), f.Path, diff.DiffOpts{ + return engine.Diff(ctx, repo.Root(), diff.File{Path: f.Path, OldPath: f.OldPath}, diff.DiffOpts{ Staged: staged, Base: base, Target: target, diff --git a/cmd/skill.go b/cmd/skill.go new file mode 100644 index 0000000..f1d4934 --- /dev/null +++ b/cmd/skill.go @@ -0,0 +1,50 @@ +package cmd + +import ( + "fmt" + + "github.com/madhermit/rift/internal/output" + "github.com/madhermit/rift/internal/skill" + "github.com/spf13/cobra" +) + +var skillCmd = &cobra.Command{ + Use: "skill", + Short: "Print the agent skill that teaches rift's review workflow", + Long: "Print the built-in agent skill: a document that teaches an AI agent how to review " + + "changes with rift's composable --print/--json output. Point an agent at it with " + + "`rift skill path`.", + RunE: func(cmd *cobra.Command, args []string) error { + if output.Detect(cmd) == output.JSON { + return output.WriteJSON(cmd.OutOrStdout(), skillInfo{Content: skill.Content()}) + } + fmt.Fprint(cmd.OutOrStdout(), skill.Content()) + return nil + }, +} + +var skillPathCmd = &cobra.Command{ + Use: "path", + Short: "Write the skill to its managed location and print the path", + RunE: func(cmd *cobra.Command, args []string) error { + p, err := skill.Path() + if err != nil { + return err + } + if output.Detect(cmd) == output.JSON { + return output.WriteJSON(cmd.OutOrStdout(), skillInfo{Path: p}) + } + fmt.Fprintln(cmd.OutOrStdout(), p) + return nil + }, +} + +type skillInfo struct { + Path string `json:"path,omitempty"` + Content string `json:"content,omitempty"` +} + +func init() { + skillCmd.AddCommand(skillPathCmd) + rootCmd.AddCommand(skillCmd) +} diff --git a/git-flux-design.md b/git-flux-design.md index 70c394c..60dec49 100644 --- a/git-flux-design.md +++ b/git-flux-design.md @@ -1275,6 +1275,12 @@ git-flux follows a "degrade gracefully, never crash, always explain" approach: The following are recorded for future exploration, not V1 scope. They become interesting once the composable foundation is solid and real usage patterns emerge. +**Inline agent annotations (`rift annotate`).** Let an agent leave review notes anchored to a file and line (or hunk), rendered inline in the diff preview — the agent explaining *why* it made a change, or a reviewing agent flagging a concern, right where the human is already reading. The design follows the reviewed-marks precedent, not a daemon: annotations live in a JSON store next to the marks (`.git/rift/annotations.json` — per worktree, uncommitted, worktree-aware for free), written by a plain composable command (`rift annotate --line N -m "..."`, plus `--json` listing and a clear operation) and picked up live by `--watch`. Agent and human notes stay separate namespaces: an agent can list and clear its own notes but never edit a human's. Prior art: hunk drives the same feature through a loopback HTTP session daemon (`hunk session comment add`); rift deliberately does it with files and polling to stay transient, stateless, and scriptable — an agent that can run a CLI needs no protocol. + +**Moved-line diff view (config).** git's `--color-moved=zebra` classifies relocated code and renders it in distinct colors — answering "moved or rewritten?" during refactor review. It cannot ship as part of the default git view because git silently ignores `--color-moved` when `--word-diff` is active (verified empirically), and word-level emphasis is the better everyday default. Ship it as a config option (e.g. `diff.git_view = "words" | "moved"` in config.toml) rather than a UI toggle — one best-effort default, configurable when someone asks. + +**Mouse support.** Click to select a row, click a pane to focus it, wheel to scroll the preview — bubbletea already delivers the events. The care point is that enabling terminal mouse tracking captures the pointer, breaking native text selection (shift-click escape hatches vary by terminal emulator), and copy-from-the-diff matters in a review tool. So: wheel scrolling and click-to-select on by default, everything else conservative, and a config/env switch to disable tracking entirely. + **Agent orchestration (`git flux agent`).** A dedicated subcommand for spawning agent worktrees with task metadata, monitoring progress across a dashboard, and managing the review→merge lifecycle. This would include `.git-flux-agent.toml` files for provenance, agent-aware status bars, and pre-flight merge checks. Worth building once the parallel-agent-worktree pattern stabilizes and we understand what orchestration agents actually need vs. what they handle themselves. **MCP server mode (`git flux mcp`).** Exposing git-flux as a Model Context Protocol server (stdio + HTTP transport) so agents can call tools like `flux_diff`, `flux_review_state`, and `flux_conflict_check` via structured protocol instead of shelling out. This is compelling but the composable `--json` CLI already serves most agent needs. MCP becomes valuable if agents need live subscriptions (e.g., "notify me when review state changes") or if the ecosystem converges on MCP as the standard agent↔tool protocol. diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 13f6ff9..06151f9 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -101,8 +101,17 @@ type DiffOpts struct { Display Display } +// File identifies the file a diff is for. OldPath is set when the file was +// renamed in the scope — it rides with the file (not DiffOpts) so a caller +// passing shared opts per file can't forget it and silently render a rename +// as a whole-file addition. +type File struct { + Path string + OldPath string +} + type Engine interface { - Diff(ctx context.Context, repoRoot, file string, opts DiffOpts) (string, error) + Diff(ctx context.Context, repoRoot string, file File, opts DiffOpts) (string, error) DiffHunks(ctx context.Context, hunks []Hunk, filename, baseContent string, color bool, width int) []string Name() string } @@ -121,10 +130,15 @@ func NewPlainEngine() Engine { return &fallbackEngine{} } -// displayFlags add intra-line word coloring and whitespace-error highlighting to -// the plain git-diff engine's output. They apply only in color mode and only to -// the fallback engine — difftastic supplies its own structural intra-line -// highlighting, and the raw-diff path used for hunk parsing must stay vanilla. +// displayFlags are the git-diff engine's readability flags, color mode only — +// difftastic supplies its own structural intra-line highlighting, and the +// raw-diff path used for hunk parsing must stay vanilla. +// +// Word emphasis is the best-effort default. A moved-line view +// (--color-moved=zebra --color-moved-ws=allow-indentation-change) is planned +// as a config option instead of a second default: git silently ignores +// --color-moved under --word-diff (verified empirically), so the two views +// are mutually exclusive per invocation and can't be combined here. func displayFlags(color bool) []string { if !color { return nil @@ -132,19 +146,17 @@ func displayFlags(color bool) []string { return []string{"--word-diff=color", "--ws-error-highlight=all"} } -// buildGitDiffArgs builds `git diff` arguments. display adds the fallback -// engine's readability flags; the difftastic-via-git path passes false so its -// external-diff driver sees a plain diff. -func buildGitDiffArgs(opts DiffOpts, file string, display bool) []string { +// buildGitDiffArgs builds `git diff` arguments with the given extra display +// flags (nil for the difftastic-via-git path, whose external-diff driver must +// see a plain diff). +func buildGitDiffArgs(opts DiffOpts, file File, display []string) []string { args := []string{"diff"} if opts.Color { args = append(args, "--color=always") } else { args = append(args, "--color=never") } - if display { - args = append(args, displayFlags(opts.Color)...) - } + args = append(args, display...) if opts.Staged { args = append(args, "--staged") } else if opts.Base != "" && opts.Target != "" { @@ -152,8 +164,13 @@ func buildGitDiffArgs(opts DiffOpts, file string, display bool) []string { } else if opts.Base != "" { args = append(args, opts.Base) } - if file != "" { - args = append(args, "--", file) + if file.Path != "" { + if file.OldPath != "" && file.OldPath != file.Path { + // A renamed file needs both sides in the pathspec for git to pair + // them, and --find-renames guards against diff.renames=false config. + return append(args, "--find-renames", "--", file.Path, file.OldPath) + } + args = append(args, "--", file.Path) } return args } diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go index 04f804d..7b69371 100644 --- a/internal/diff/diff_test.go +++ b/internal/diff/diff_test.go @@ -37,70 +37,76 @@ func TestBuildGitDiffArgs(t *testing.T) { tests := []struct { name string opts DiffOpts - file string - display bool + file File + display []string want []string }{ { name: "staged with color (no display flags)", opts: DiffOpts{Staged: true, Color: true}, - file: "main.go", + file: File{Path: "main.go"}, want: []string{"diff", "--color=always", "--staged", "--", "main.go"}, }, { name: "staged no color", opts: DiffOpts{Staged: true, Color: false}, - file: "main.go", + file: File{Path: "main.go"}, want: []string{"diff", "--color=never", "--staged", "--", "main.go"}, }, { name: "base only", opts: DiffOpts{Base: "HEAD~1"}, - file: "main.go", + file: File{Path: "main.go"}, want: []string{"diff", "--color=never", "HEAD~1", "--", "main.go"}, }, { name: "base and target", opts: DiffOpts{Base: "abc123", Target: "def456"}, - file: "main.go", + file: File{Path: "main.go"}, want: []string{"diff", "--color=never", "abc123", "def456", "--", "main.go"}, }, { name: "no opts (working tree)", opts: DiffOpts{}, - file: "main.go", + file: File{Path: "main.go"}, want: []string{"diff", "--color=never", "--", "main.go"}, }, + { + name: "rename pathspec covers both sides", + opts: DiffOpts{Base: "a", Target: "b"}, + file: File{Path: "new.go", OldPath: "old.go"}, + want: []string{"diff", "--color=never", "a", "b", "--find-renames", "--", "new.go", "old.go"}, + }, { name: "color with base and target", opts: DiffOpts{Base: "a", Target: "b", Color: true}, - file: "f.go", + file: File{Path: "f.go"}, want: []string{"diff", "--color=always", "a", "b", "--", "f.go"}, }, { name: "empty file omits separator", opts: DiffOpts{Color: true}, - file: "", + file: File{}, want: []string{"diff", "--color=always"}, }, { name: "empty file with staged", opts: DiffOpts{Staged: true}, - file: "", + file: File{}, want: []string{"diff", "--color=never", "--staged"}, }, { name: "display with color adds word-diff and ws flags", opts: DiffOpts{Staged: true, Color: true}, - file: "main.go", - display: true, + file: File{Path: "main.go"}, + display: displayFlags(true), want: []string{"diff", "--color=always", "--word-diff=color", "--ws-error-highlight=all", "--staged", "--", "main.go"}, }, { name: "display without color stays vanilla", opts: DiffOpts{Staged: true, Color: false}, - file: "main.go", - display: true, + file: File{Path: "main.go"}, + display: displayFlags(false), want: []string{"diff", "--color=never", "--staged", "--", "main.go"}, }, } @@ -149,7 +155,7 @@ func TestFallbackEngineUntracked(t *testing.T) { } e := &fallbackEngine{} - out, err := e.Diff(context.Background(), dir, "new.txt", DiffOpts{}) + out, err := e.Diff(context.Background(), dir, File{Path: "new.txt"}, DiffOpts{}) if err != nil { t.Fatalf("untracked diff: %v", err) } @@ -157,7 +163,7 @@ func TestFallbackEngineUntracked(t *testing.T) { t.Errorf("untracked file should render as a new-file diff, got %q", out) } - out, err = e.Diff(context.Background(), dir, "tracked.txt", DiffOpts{}) + out, err = e.Diff(context.Background(), dir, File{Path: "tracked.txt"}, DiffOpts{}) if err != nil { t.Fatalf("tracked diff: %v", err) } diff --git a/internal/diff/difftastic.go b/internal/diff/difftastic.go index d75257c..8050974 100644 --- a/internal/diff/difftastic.go +++ b/internal/diff/difftastic.go @@ -17,15 +17,15 @@ type difftasticEngine struct { func (d *difftasticEngine) Name() string { return "difftastic" } -func (d *difftasticEngine) Diff(ctx context.Context, repoRoot, file string, opts DiffOpts) (string, error) { +func (d *difftasticEngine) Diff(ctx context.Context, repoRoot string, file File, opts DiffOpts) (string, error) { if opts.Width <= 0 { return d.diffViaGit(ctx, repoRoot, file, opts) } return d.diffDirect(ctx, repoRoot, file, opts) } -func (d *difftasticEngine) diffViaGit(ctx context.Context, repoRoot, file string, opts DiffOpts) (string, error) { - args := buildGitDiffArgs(opts, file, false) +func (d *difftasticEngine) diffViaGit(ctx context.Context, repoRoot string, file File, opts DiffOpts) (string, error) { + args := buildGitDiffArgs(opts, file, nil) cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = repoRoot colorEnv := "DFT_COLOR=never" @@ -36,7 +36,7 @@ func (d *difftasticEngine) diffViaGit(ctx context.Context, repoRoot, file string return runGitDiff(cmd, "difftastic") } -func (d *difftasticEngine) diffDirect(ctx context.Context, repoRoot, file string, opts DiffOpts) (string, error) { +func (d *difftasticEngine) diffDirect(ctx context.Context, repoRoot string, file File, opts DiffOpts) (string, error) { tmpDir, err := os.MkdirTemp("", "rift-diff-*") if err != nil { return "", fmt.Errorf("create temp dir: %w", err) @@ -50,18 +50,26 @@ func (d *difftasticEngine) diffDirect(ctx context.Context, repoRoot, file string switch { case opts.Base != "" && opts.Target != "": oldRef = opts.Base - newPath = showOrNull(ctx, repoRoot, opts.Target, file, filepath.Join(tmpDir, "b", file)) + newPath = showOrNull(ctx, repoRoot, opts.Target, file.Path, filepath.Join(tmpDir, "b", file.Path)) case opts.Staged: oldRef = "HEAD" - newPath = showOrNull(ctx, repoRoot, "", file, filepath.Join(tmpDir, "b", file)) + newPath = showOrNull(ctx, repoRoot, "", file.Path, filepath.Join(tmpDir, "b", file.Path)) case opts.Base != "": oldRef = opts.Base - newPath = worktreeOrNull(filepath.Join(repoRoot, file)) + newPath = worktreeOrNull(filepath.Join(repoRoot, file.Path)) default: - newPath = worktreeOrNull(filepath.Join(repoRoot, file)) + newPath = worktreeOrNull(filepath.Join(repoRoot, file.Path)) } - oldPath := showOrNull(ctx, repoRoot, oldRef, file, filepath.Join(tmpDir, "a", file)) + // A renamed file's pre-image lives at its old path in the old ref. It's + // still extracted under the NEW path (destPath) so both sides share a + // relative path — difftastic then labels the diff with that clean path + // instead of the temp file's; rift's own preview title carries old → new. + oldFile := file.Path + if file.OldPath != "" { + oldFile = file.OldPath + } + oldPath := showOrNull(ctx, repoRoot, oldRef, oldFile, filepath.Join(tmpDir, "a", file.Path)) return d.diffFiles(ctx, oldPath, newPath, opts.Color, opts.Width, opts.Display) } diff --git a/internal/diff/fallback.go b/internal/diff/fallback.go index 016354e..d9aaab6 100644 --- a/internal/diff/fallback.go +++ b/internal/diff/fallback.go @@ -17,8 +17,9 @@ func (f *fallbackEngine) Name() string { return "git-diff" } // re-checked: if the file is untracked, it's diffed against the null device // instead so its content renders as a new-file diff — matching the difftastic // engine, which reaches the same result via its failed `git show :file`. -func (f *fallbackEngine) Diff(ctx context.Context, repoRoot, file string, opts DiffOpts) (string, error) { - args := buildGitDiffArgs(opts, file, true) +func (f *fallbackEngine) Diff(ctx context.Context, repoRoot string, file File, opts DiffOpts) (string, error) { + display := displayFlags(opts.Color) + args := buildGitDiffArgs(opts, file, display) cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = repoRoot out, err := runGitDiff(cmd, "git diff") @@ -26,10 +27,10 @@ func (f *fallbackEngine) Diff(ctx context.Context, repoRoot, file string, opts D return out, err } worktreeScope := !opts.Staged && opts.Base == "" && opts.Target == "" - if !worktreeScope || !isUntracked(ctx, repoRoot, file) { + if !worktreeScope || !isUntracked(ctx, repoRoot, file.Path) { return out, nil } - args = append(buildGitDiffArgs(opts, "", true), "--no-index", "--", os.DevNull, file) + args = append(buildGitDiffArgs(opts, File{}, display), "--no-index", "--", os.DevNull, file.Path) cmd = exec.CommandContext(ctx, "git", args...) cmd.Dir = repoRoot return runGitDiff(cmd, "git diff untracked") diff --git a/internal/diff/hunk.go b/internal/diff/hunk.go index 43e86af..e16b702 100644 --- a/internal/diff/hunk.go +++ b/internal/diff/hunk.go @@ -298,11 +298,12 @@ func ShowFile(repoRoot, ref, file string) ([]byte, error) { } // RawRangeDiffAll returns the whole-repo unified diff between two commits — no -// pathspec, so git's rename detection stays on (a per-file pathspec would defeat -// it, rendering a renamed file as 100% added). Callers parse it once and index by -// path. +// pathspec, and --find-renames forced, so rename detection is on even under +// diff.renames=false config (a per-file pathspec or a config-disabled detection +// would render a renamed file as 100% added, disagreeing with the listing). +// Callers parse it once and index by path. func RawRangeDiffAll(repoRoot, base, target string) (string, error) { - cmd := exec.Command("git", "diff", "--no-color", base, target) + cmd := exec.Command("git", "diff", "--no-color", "--find-renames", base, target) cmd.Dir = repoRoot return runGitDiff(cmd, "git diff range") } @@ -337,11 +338,12 @@ func RawUnifiedDiff(repoRoot string, staged bool, file string) (string, error) { } // RawWorktreeDiff returns the whole working-tree (or staged) unified diff — no -// pathspec, so git's rename detection stays on and one subprocess covers every -// file. Callers parse it once and index by path. Untracked files aren't included -// (git diff omits them); those are handled separately. +// pathspec, and --find-renames forced (see RawRangeDiffAll), so rename +// detection stays on and one subprocess covers every file. Callers parse it +// once and index by path. Untracked files aren't included (git diff omits +// them); those are handled separately. func RawWorktreeDiff(repoRoot string, staged bool) (string, error) { - args := []string{"diff", "--no-color"} + args := []string{"diff", "--no-color", "--find-renames"} if staged { args = append(args, "--staged") } diff --git a/internal/git/diff.go b/internal/git/diff.go index d29cbca..2171e1e 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -21,11 +21,21 @@ const EmptyTree = "4b825dc642cb6eb9a060e54bf899d69f82cf7207" type ChangedFile struct { Path string `json:"path"` + OldPath string `json:"old_path,omitempty"` // set for a rename/copy: the pre-image path Status string `json:"status"` Added int `json:"added"` Deleted int `json:"deleted"` } +// DisplayPath is the human-facing label for the file: its path, or +// "old → new" for a rename. +func (f ChangedFile) DisplayPath() string { + if f.OldPath != "" { + return f.OldPath + " → " + f.Path + } + return f.Path +} + // Paths returns just the paths of the changed files. func Paths(files []ChangedFile) []string { paths := make([]string, len(files)) @@ -93,42 +103,61 @@ func applyNumstat(files []ChangedFile, stats map[string][2]int) { } } -// parseNumstatZ parses `git diff --numstat -z --no-renames` output: NUL- -// terminated "added\tdeleted\tpath" records. Binary files report "-" for the -// counts, which Atoi maps to 0. -z keeps the path verbatim (no C-quoting). +// parseNumstatZ parses `git diff --numstat -z` output: NUL-terminated +// "added\tdeleted\tpath" records. A rename/copy record leaves the inline path +// empty and appends the two paths as separate NUL fields (old, then new); the +// stats are keyed by the new path, matching parseNameStatusZ. Binary files +// report "-" for the counts, which Atoi maps to 0. -z keeps paths verbatim +// (no C-quoting). func parseNumstatZ(out string) map[string][2]int { stats := make(map[string][2]int) - for _, rec := range strings.Split(out, "\x00") { - if rec == "" { - continue - } - parts := strings.SplitN(rec, "\t", 3) + fields := strings.Split(out, "\x00") + for i := 0; i < len(fields); i++ { + parts := strings.SplitN(fields[i], "\t", 3) if len(parts) != 3 { continue } added, _ := strconv.Atoi(parts[0]) // "-" (binary) → 0 deleted, _ := strconv.Atoi(parts[1]) // "-" (binary) → 0 - stats[parts[2]] = [2]int{added, deleted} + path := parts[2] + if path == "" && i+2 < len(fields) { // rename/copy: paths follow as two fields + path = fields[i+2] + i += 2 + } + if path != "" { + stats[path] = [2]int{added, deleted} + } } return stats } -// parseNameStatusZ parses `git diff --name-status -z --no-renames` (and the -// equivalent diff-tree) output: NUL-separated fields alternating STATUS, PATH. -// With renames disabled there are no three-field rename records, so every entry -// is a clean pair and non-ASCII paths come through verbatim (no C-quoting). +// parseNameStatusZ parses `git diff --name-status -z --find-renames` (and the +// equivalent diff-tree) output: NUL-separated fields, normally STATUS, PATH +// pairs. A rename/copy record (R/C) carries a third field — the +// record is STATUS, OLD, NEW. -z keeps non-ASCII paths verbatim (no C-quoting). func parseNameStatusZ(out string) []ChangedFile { fields := strings.Split(out, "\x00") var files []ChangedFile - for i := 0; i+1 < len(fields); i += 2 { + for i := 0; i+1 < len(fields); { status, path := fields[i], fields[i+1] if status == "" || path == "" { + i += 2 + continue + } + if (status[0] == 'R' || status[0] == 'C') && i+2 < len(fields) && fields[i+2] != "" { + files = append(files, ChangedFile{ + Path: fields[i+2], + OldPath: path, + Status: nameStatusCode(status), + }) + i += 3 continue } files = append(files, ChangedFile{ Path: path, Status: nameStatusCode(status), }) + i += 2 } return files } @@ -143,6 +172,43 @@ func nameStatusCode(code string) string { return statusWord(code[0]) } +// ListChanged lists the changed files for a diff scope, pathspec-filtered, +// sorted by path, and never nil. The scope picks the comparison: +// - target set: the committed range base..target, tree to tree +// - staged set: the index against HEAD. Staged wins over base to match the +// per-file preview (buildGitDiffArgs), which puts --staged first — the CLI +// rejects the combination, but the TUI's `s` toggle can produce it, and the +// list and previews must agree on what it means +// - base set: the working tree against base, so committed changes since the +// ref show up too (not just the worktree-vs-HEAD delta), matching a +// per-file preview that also diffs against base +// - none: the working tree against the index +func (r *Repo) ListChanged(staged bool, base, target string, paths ...string) ([]ChangedFile, error) { + var ( + files []ChangedFile + err error + ) + switch { + case target != "": + files, err = r.DiffBetweenCommits(base, target) + case staged: + files, err = r.ChangedFiles(true) + case base != "": + files, err = r.DiffAgainstRef(base) + default: + files, err = r.ChangedFiles(false) + } + if err != nil { + return nil, err + } + files = FilterByPaths(files, paths) + if files == nil { + files = []ChangedFile{} + } + SortByPath(files) + return files, nil +} + func DiffTargets(args []string) (base, target string, err error) { switch len(args) { case 0: @@ -174,10 +240,12 @@ func DiffTargets(args []string) (base, target string, err error) { // real empty-tree object (resolved from git, since its hash is object-format // dependent) instead. // -// --no-renames keeps a rename as a delete + an add (like statusFiles), so a -// renamed file in a commit drilldown renders correctly instead of as a whole -// addition — the new path doesn't exist at the base, so the diff engine would -// extract a /dev/null old side for it. +// --find-renames reports a rename as a single record with both paths (even if +// the user's config sets diff.renames=false), so a moved file reads as its +// small content delta instead of a full delete + add. The old path travels on +// ChangedFile.OldPath, which the diff engines use to extract the pre-image — +// worktree/staged listings (statusFiles) intentionally stay rename-free, since +// the stage screen operates on the real index paths. func (r *Repo) DiffBetweenCommits(baseRef, targetRef string) ([]ChangedFile, error) { base, useRoot := baseRef, false if baseRef == EmptyTree { @@ -193,9 +261,9 @@ func (r *Repo) DiffBetweenCommits(baseRef, targetRef string) ([]ChangedFile, err } return changedFilesWithStats(func(format string) (string, error) { if useRoot { - return r.runGit("diff-tree", "--root", "--no-commit-id", format, "--no-renames", "-z", "-r", targetRef) + return r.runGit("diff-tree", "--root", "--no-commit-id", format, "--find-renames", "-z", "-r", targetRef) } - return r.runGit("diff", format, "--no-renames", "-z", base, targetRef) + return r.runGit("diff", format, "--find-renames", "-z", base, targetRef) }) } @@ -245,9 +313,10 @@ func (r *Repo) emptyTreeHash() (string, error) { // scope: it surfaces both committed and uncommitted changes since baseRef, so the // listing matches the per-file preview, which also diffs the working tree against // baseRef. Untracked files are excluded, matching `git diff ` semantics. +// Renames are reported as single records (see DiffBetweenCommits). func (r *Repo) DiffAgainstRef(baseRef string) ([]ChangedFile, error) { return changedFilesWithStats(func(format string) (string, error) { - return r.runGit("diff", format, "--no-renames", "-z", baseRef) + return r.runGit("diff", format, "--find-renames", "-z", baseRef) }) } @@ -260,13 +329,16 @@ func matchPath(file string, paths []string) bool { return false } +// FilterByPaths keeps the files under any of the given paths. A rename matches +// on either side, so a file moved out of (or into) the scoped directory still +// lists rather than silently vanishing from the changeset. func FilterByPaths(files []ChangedFile, paths []string) []ChangedFile { if len(paths) == 0 { return files } filtered := []ChangedFile{} for _, f := range files { - if matchPath(f.Path, paths) { + if matchPath(f.Path, paths) || (f.OldPath != "" && matchPath(f.OldPath, paths)) { filtered = append(filtered, f) } } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index dc2f99b..50ea594 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -266,3 +266,157 @@ func TestParseNumstatZ(t *testing.T) { } } } + +func TestParseNameStatusZ_Rename(t *testing.T) { + // A rename record carries three fields: R\0OLD\0NEW. Following + // records parse normally. + out := "R087\x00old dir/a.go\x00new dir/b.go\x00M\x00c.go\x00" + got := parseNameStatusZ(out) + want := []ChangedFile{ + {Path: "new dir/b.go", OldPath: "old dir/a.go", Status: "Renamed"}, + {Path: "c.go", Status: "Modified"}, + } + if len(got) != len(want) { + t.Fatalf("got %d files, want %d: %+v", len(got), len(want), got) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("file %d = %+v, want %+v", i, got[i], want[i]) + } + } +} + +func TestParseNumstatZ_Rename(t *testing.T) { + // A rename record leaves the inline path empty and appends OLD and NEW as + // separate NUL fields; stats key on the new path. + out := "3\t1\t\x00old/a.go\x00new/b.go\x005\t0\tc.go\x00" + got := parseNumstatZ(out) + want := map[string][2]int{ + "new/b.go": {3, 1}, + "c.go": {5, 0}, + } + if len(got) != len(want) { + t.Fatalf("got %d entries, want %d: %+v", len(got), len(want), got) + } + for path, w := range want { + if got[path] != w { + t.Errorf("%q = %v, want %v", path, got[path], w) + } + } +} + +// TestDiffBetweenCommits_Rename covers rename detection end to end: a renamed +// file with a small edit reports one record with both paths and the edit's +// stats, not a full delete + add. +func TestDiffBetweenCommits_Rename(t *testing.T) { + repo := setupTestRepo(t) + + wt, err := repo.repo.Worktree() + if err != nil { + t.Fatalf("get worktree: %v", err) + } + + content := "package main\n\nfunc a() {}\nfunc b() {}\nfunc c() {}\nfunc d() {}\nfunc e() {}\nfunc f() {}\nfunc g() {}\nfunc h() {}\n" + writeFile(t, repo.root, "old_name.go", content) + if _, err := wt.Add("old_name.go"); err != nil { + t.Fatalf("git add: %v", err) + } + base := testCommit(t, wt, "add old_name") + + // Rename with a one-line tweak: similar enough for -M to pair the paths. + writeFile(t, repo.root, "new_name.go", strings.Replace(content, "func h() {}", "func i() {}", 1)) + if _, err := wt.Remove("old_name.go"); err != nil { + t.Fatalf("git rm: %v", err) + } + if _, err := wt.Add("new_name.go"); err != nil { + t.Fatalf("git add: %v", err) + } + target := testCommit(t, wt, "rename old_name to new_name") + + files, err := repo.DiffBetweenCommits(base.String(), target.String()) + if err != nil { + t.Fatalf("DiffBetweenCommits() error = %v", err) + } + if len(files) != 1 { + t.Fatalf("expected 1 rename record, got %d: %+v", len(files), files) + } + got := files[0] + want := ChangedFile{Path: "new_name.go", OldPath: "old_name.go", Status: "Renamed", Added: 1, Deleted: 1} + if got != want { + t.Errorf("rename = %+v, want %+v", got, want) + } +} + +// TestListChanged_StagedWinsOverBase pins the precedence contract: staged +// beats base, matching the per-file preview (buildGitDiffArgs), so the TUI's +// `s` toggle in a base-ref scope lists the same side the previews render. +func TestListChanged_StagedWinsOverBase(t *testing.T) { + repo := setupTestRepo(t) + + wt, err := repo.repo.Worktree() + if err != nil { + t.Fatalf("get worktree: %v", err) + } + head, err := repo.repo.Head() + if err != nil { + t.Fatalf("get head: %v", err) + } + base := head.Hash().String() + + // One committed-since-base change and one staged change. + writeFile(t, repo.root, "committed.go", "package main\n") + if _, err := wt.Add("committed.go"); err != nil { + t.Fatalf("git add: %v", err) + } + testCommit(t, wt, "committed change") + writeFile(t, repo.root, "staged.go", "package main\n") + if _, err := wt.Add("staged.go"); err != nil { + t.Fatalf("git add: %v", err) + } + + files, err := repo.ListChanged(true, base, "") + if err != nil { + t.Fatalf("ListChanged() error = %v", err) + } + if len(files) != 1 || files[0].Path != "staged.go" { + t.Errorf("staged+base should list the staged side only, got %+v", files) + } +} + +func TestStagedBlobHashes(t *testing.T) { + repo := setupTestRepo(t) + + wt, err := repo.repo.Worktree() + if err != nil { + t.Fatalf("get worktree: %v", err) + } + writeFile(t, repo.root, "f.go", "package main\n") + if _, err := wt.Add("f.go"); err != nil { + t.Fatalf("git add: %v", err) + } + + staged := repo.StagedBlobHashes([]string{"f.go"}) + if staged["f.go"] == "" { + t.Fatalf("expected an index hash for f.go, got %v", staged) + } + + // Editing the worktree without re-adding must not change the index hash — + // that's the property the staged watch fingerprint relies on. + writeFile(t, repo.root, "f.go", "package main\n\nfunc edited() {}\n") + after := repo.StagedBlobHashes([]string{"f.go"}) + if after["f.go"] != staged["f.go"] { + t.Errorf("index hash changed on a worktree-only edit: %q -> %q", staged["f.go"], after["f.go"]) + } + if wtHash := repo.BlobHashes([]string{"f.go"})["f.go"]; wtHash == after["f.go"] { + t.Errorf("worktree and index hashes should differ after the edit, both %q", wtHash) + } + + // Re-staging picks up the new content. + if _, err := wt.Add("f.go"); err != nil { + t.Fatalf("git add: %v", err) + } + restaged := repo.StagedBlobHashes([]string{"f.go"}) + if restaged["f.go"] == staged["f.go"] { + t.Errorf("index hash should change after re-staging, still %q", staged["f.go"]) + } +} diff --git a/internal/git/state.go b/internal/git/state.go index 0f9eed1..819ef88 100644 --- a/internal/git/state.go +++ b/internal/git/state.go @@ -22,6 +22,34 @@ func (r *Repo) GitPath(rel string) (string, error) { return p, nil } +// StagedBlobHashes returns the blob hash of each path's content in the index, +// in one subprocess (`git ls-files --stage`). Paths absent from the index are +// omitted. This is the staged-side counterpart of BlobHashes: the index +// already stores blob OIDs, so nothing is hashed. +func (r *Repo) StagedBlobHashes(paths []string) map[string]string { + hashes := map[string]string{} + if len(paths) == 0 { + return hashes + } + args := append([]string{"ls-files", "--stage", "-z", "--"}, paths...) + out, err := r.runGit(args...) + if err != nil { + return hashes + } + // Records: " \t", NUL-terminated. + for _, rec := range strings.Split(out, "\x00") { + meta, path, ok := strings.Cut(rec, "\t") + if !ok { + continue + } + fields := strings.Fields(meta) + if len(fields) == 3 { + hashes[path] = fields[1] + } + } + return hashes +} + // BlobHashes returns the git blob hash of each path's current working-tree // content, in one subprocess. Paths that don't exist on disk (a deleted file, a // directory) are omitted — there's no content to hash. diff --git a/internal/review/scope.go b/internal/review/scope.go index 478007f..30c126a 100644 --- a/internal/review/scope.go +++ b/internal/review/scope.go @@ -14,6 +14,7 @@ import ( // the set of line numbers the diff added or modified. type fileScope struct { Path string + OldPath string // pre-image path when the file was renamed in scope Ext Extractor Content []byte Old []byte @@ -83,7 +84,13 @@ func gatherRange(repo *git.Repo, base, target string, paths []string) ([]fileSco if added == nil { added = map[int]bool{} } - old, _ := diff.ShowFile(root, base, f.Path) // nil old side for a file added in the range + // A renamed file's old side lives at its old path in the base — without + // it every spec in the file would read as added rather than renamed. + oldPath := f.Path + if f.OldPath != "" { + oldPath = f.OldPath + } + old, _ := diff.ShowFile(root, base, oldPath) // nil old side for a file added in the range return content, old, added, true }), nil } @@ -103,7 +110,7 @@ func collect(files []git.ChangedFile, resolve func(git.ChangedFile) (content, ol if !ok { continue } - scopes = append(scopes, fileScope{Path: f.Path, Ext: ext, Content: content, Old: old, Added: added}) + scopes = append(scopes, fileScope{Path: f.Path, OldPath: f.OldPath, Ext: ext, Content: content, Old: old, Added: added}) } return scopes } diff --git a/internal/review/testspec.go b/internal/review/testspec.go index 4144a1d..66029df 100644 --- a/internal/review/testspec.go +++ b/internal/review/testspec.go @@ -12,6 +12,7 @@ import ( // describe/context path that contains it, and how the diff changed it. type Spec struct { File string `json:"file"` + OldPath string `json:"old_path,omitempty"` // pre-image path when the file was renamed in scope Language string `json:"language"` Path []string `json:"path,omitempty"` Name string `json:"name"` @@ -81,6 +82,7 @@ func Collect(repo *git.Repo, scope DiffScope) ([]Spec, error) { } specs = append(specs, Spec{ File: sc.Path, + OldPath: sc.OldPath, Language: sc.Ext.Language(), Path: r.Path, Name: r.Name, diff --git a/internal/skill/SKILL.md b/internal/skill/SKILL.md new file mode 100644 index 0000000..3f5aa46 --- /dev/null +++ b/internal/skill/SKILL.md @@ -0,0 +1,102 @@ +--- +name: rift-review +description: Review git changes with rift — structural diffs, test-impact analysis, and review tracking through composable --print/--json output. Use when reviewing a diff or commit, checking which tests a change touches, or coordinating a review with a human in a repo where rift is installed. +--- + +# Reviewing changes with rift + +rift is a git tool built for reading code: syntax-aware diffs (via difftastic), +a test-impact lens, and per-file review tracking. Every command works two ways — +an interactive TUI for humans, and `--print` / `--json` output for you. This +skill covers the composable side; never try to drive the TUI. + +Ground rules: + +- Always pass `--print` or `--json` explicitly. (When stdout is not a TTY rift + falls back to `--print` on its own, so it will never hang you, but being + explicit keeps intent clear.) +- rift only reads and presents git state — it never mutates the working tree, + and the only writes it can make happen through human keypresses in the TUI + (stage, stash apply, cherry-pick). Running the commands below is always safe. +- All commands accept a trailing `-- ...` to scope to files or + directories. + +## Inspect a changeset + +```bash +rift diff --json # unstaged changes: [{path, status, added, deleted}, ...] +rift diff --staged --json # staged changes +rift diff main --json # working tree vs a ref (committed + uncommitted) +rift diff HEAD~3 HEAD --json # a committed range +rift diff --name-only # just the paths, one per line +``` + +`status` is `Modified`, `Added`, `Deleted`, `Renamed`, `Copied`, or `Untracked`. +In a commit-range or base-ref scope, `Renamed` entries also carry `old_path`. + +## Read the diffs + +```bash +rift diff --print # every file's diff, structural when difftastic is available +rift diff --print -- cmd/ # scoped to a directory +``` + +Structural diffs elide reformatting noise and show expression-level changes — +usually much less text than `git diff` for the same change. + +## Test impact + +```bash +rift diff --tests --json # the test cases the diff touches +rift diff HEAD~5 HEAD --tests --json # same, for a commit range +``` + +Output: `[{file, language, name, line, status, path?, old_path?, ticket?}, ...]` where +`status` is `added`, `renamed`, or `modified`. Pay closest attention to +`modified`: an existing test whose body changed can mean a quietly weakened +assertion. A change that touches no tests is also a finding worth reporting. +Extraction covers Go, JavaScript/TypeScript, Ruby, Python, and Rust, including +subtests and table-driven cases. + +## Review state + +Humans mark files reviewed in the rift TUI (`r`). A mark is keyed to the file's +content hash, so it resets the moment the file changes again. You can read that +state to know what still needs attention: + +```bash +rift diff --unreviewed --name-only # files the human has not (re-)reviewed +rift diff --unreviewed --json +``` + +If you edit a file that was already marked reviewed, its mark resets — that is +expected and correct; don't try to avoid it. + +## History + +```bash +rift log --json -n 20 # [{hash, author, date, message, body?}, ...] +rift log main..HEAD --json # commits on this branch only +rift log --json -- internal/git # commits touching a path +``` + +## Reviewing alongside a human + +The human may have `rift diff --watch` open in another terminal: it live-reloads +as you edit, and their reviewed marks tick off (or reset) as files change. You +don't need to signal anything — just edit; rift picks it up. A useful summary +when you finish a change: + +```bash +rift diff --json # what changed +rift diff --tests --json # which tests the change touches +rift diff --unreviewed --name-only # what the human hasn't looked at yet +``` + +## A suggested review pass + +1. `rift diff --json` — map the changeset; note sizes and statuses. +2. `rift diff --tests --json` — check the test story: are there + `modified` tests to scrutinize? Is new code untested? +3. `rift diff --print` — read the structural diffs, largest files first. +4. Report findings referencing `path:line` from the JSON. diff --git a/internal/skill/skill.go b/internal/skill/skill.go new file mode 100644 index 0000000..4282ea9 --- /dev/null +++ b/internal/skill/skill.go @@ -0,0 +1,41 @@ +// Package skill embeds the agent skill that teaches rift's composable review +// workflow, and materializes it to a stable path agents can read. +package skill + +import ( + _ "embed" + "fmt" + "os" + "path/filepath" + + "github.com/madhermit/rift/internal/tooling" +) + +//go:embed SKILL.md +var content string + +// Content returns the skill document. +func Content() string { return content } + +// Path materializes the skill to its managed location (under the same data dir +// as the difftastic binary) and returns the absolute path. The file is +// rewritten when its content is stale, so an upgraded binary refreshes it — +// via a temp file and atomic rename, so an agent reading the skill while +// another rift invocation refreshes it never sees a truncated file. +func Path() (string, error) { + dir, err := tooling.DataDir() + if err != nil { + return "", fmt.Errorf("resolve data directory: %w", err) + } + p := filepath.Join(dir, "skills", "rift-review", "SKILL.md") + if existing, err := os.ReadFile(p); err == nil && string(existing) == content { + return p, nil + } + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + return "", fmt.Errorf("create skill directory: %w", err) + } + if err := tooling.WriteFileAtomic(p, []byte(content), 0o644); err != nil { + return "", fmt.Errorf("write skill file: %w", err) + } + return p, nil +} diff --git a/internal/skill/skill_test.go b/internal/skill/skill_test.go new file mode 100644 index 0000000..4afeab0 --- /dev/null +++ b/internal/skill/skill_test.go @@ -0,0 +1,45 @@ +package skill + +import ( + "os" + "strings" + "testing" +) + +func TestContent(t *testing.T) { + c := Content() + if !strings.HasPrefix(c, "---\nname: rift-review\n") { + t.Errorf("skill content missing frontmatter header, starts with %q", c[:40]) + } + if !strings.Contains(c, "rift diff --tests --json") { + t.Error("skill content missing the tests-lens workflow") + } +} + +func TestPathMaterializes(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + p, err := Path() + if err != nil { + t.Fatalf("Path() error: %v", err) + } + data, err := os.ReadFile(p) + if err != nil { + t.Fatalf("skill file not written: %v", err) + } + if string(data) != Content() { + t.Error("materialized skill differs from embedded content") + } + + // A stale file (e.g. from an older binary) is refreshed. + if err := os.WriteFile(p, []byte("old"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := Path(); err != nil { + t.Fatalf("Path() on stale file: %v", err) + } + data, _ = os.ReadFile(p) + if string(data) != Content() { + t.Error("stale skill file was not refreshed") + } +} diff --git a/internal/tooling/difft.go b/internal/tooling/difft.go index 32a5eb3..a844f12 100644 --- a/internal/tooling/difft.go +++ b/internal/tooling/difft.go @@ -47,12 +47,46 @@ const downloadTimeout = 30 * time.Second // network doesn't stall every rift invocation retrying synchronously. const failureBackoff = time.Hour -func managedPath() (string, error) { +// DataDir is rift's per-user data directory (managed binaries, the agent +// skill). Kept in one place so a future XDG_DATA_HOME override lands +// everywhere at once. +func DataDir() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", err } - return filepath.Join(home, ".local", "share", "rift", "bin", "difft"), nil + return filepath.Join(home, ".local", "share", "rift"), nil +} + +// WriteFileAtomic writes data to path via a temp file and rename, so a +// concurrent reader never sees a truncated file. The parent directory must +// already exist. +func WriteFileAtomic(path string, data []byte, perm os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+"-*") + if err != nil { + return err + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // no-op once renamed away + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chmod(tmpName, perm); err != nil { + return err + } + return os.Rename(tmpName, path) +} + +func managedPath() (string, error) { + dir, err := DataDir() + if err != nil { + return "", err + } + return filepath.Join(dir, "bin", "difft"), nil } func FindOrInstallDifft() (string, error) { diff --git a/internal/tui/diff/model.go b/internal/tui/diff/model.go index 44ce8fa..53fc02c 100644 --- a/internal/tui/diff/model.go +++ b/internal/tui/diff/model.go @@ -14,6 +14,17 @@ import ( var reviewedGlyph = lipgloss.NewStyle().Foreground(tui.Green) +// FilesChangedMsg tells the lens its changed-file set was re-listed externally +// (the watch poller): Files is the new set, already pathspec-filtered and +// sorted, and Hashes its worktree content hashes (computed off the event loop +// by the poll; nil means compute them here). The lens applies the set in +// place, keeping the selection and — for files whose content didn't change — +// the cached previews and the reader's scroll position. +type FilesChangedMsg struct { + Files []git.ChangedFile + Hashes map[string]string +} + // reviewMarks is the reviewed-state lookup the row renderer reads: the persisted // marks plus each file's current blob hash. Both are mutated in place — Toggle on // the store, a fresh hash map after an edit — so the row closure that captures @@ -43,6 +54,7 @@ type Model struct { paths []string // pathspec scope (from `-- path...`), re-applied on reload marks *reviewMarks // nil for a commit diff — historical files don't get reviewed unreviewedOnly bool + watching bool // show the live-watch indicator in the header context } // prependAllEntry adds a synthetic "All" row (carrying the totals) that previews @@ -115,7 +127,7 @@ func New(repo *git.Repo, engine diff.Engine, files []git.ChangedFile, staged boo hints = append(hints, [2]string{"t", "tests"}) } if !commitDiff { - hints = append(hints, [2]string{"s", "staged"}) + hints = append(hints, [2]string{"s", "staged"}, [2]string{"w", "watch"}) } hints = append(hints, [2]string{"\\", "layout"}) if engines.CanToggle() { @@ -132,7 +144,7 @@ func New(repo *git.Repo, engine diff.Engine, files []git.ChangedFile, staged boo Screen: "diff", ListTitle: m.listTitle(), Branch: repo.CurrentBranch(), - Context: tui.ContextLabel(target, engine.Name()), + Context: tui.ContextLabel(target, engine.Name(), false), NavFraction: 30, EmptyStatus: m.emptyStatus(), Hints: hints, @@ -141,7 +153,7 @@ func New(repo *git.Repo, engine diff.Engine, files []git.ChangedFile, staged boo if f.Path == "" { return "all changes" } - return f.Path + return f.DisplayPath() }, // Cache per file; the All entry depends on the filtered set, so skip it. // Staged/display changes clear the cache (SetItems / ClearCacheAndReload). @@ -184,6 +196,19 @@ func (m Model) displayFiles() []git.ChangedFile { return prependAllEntry(m.files) } +// SetWatching sets the "watching" tag in the header context, marking the lens +// as live-reloading. The flag persists on the model because the engine toggle +// rebuilds the label. +func (m Model) SetWatching(on bool) Model { + m.watching = on + m.list = m.list.SetContext(m.contextLabel()) + return m +} + +func (m Model) contextLabel() string { + return tui.ContextLabel(m.target, m.engines.Name(), m.watching) +} + // SetEmptyStatus overrides the footer text shown when the list is empty (e.g. a // commit whose diff failed to load, distinct from "No changes found"). func (m Model) SetEmptyStatus(s string) Model { @@ -258,6 +283,11 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // can flip its reviewed state — so reload the file list (recomputing hashes), // preserving the selection, rather than leaving stale rows and +/- stats. return m.reloadFiles() + case FilesChangedMsg: + if m.commitDiff { + return m, nil // historical set; nothing external can change it + } + return m.applyFiles(msg.Files, msg.Hashes) case tea.KeyPressMsg: if m.list.Filtering() || m.list.ShowingHelp() { break @@ -271,7 +301,7 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { break // only one engine available; nothing to toggle } m.engines = m.engines.Toggle() - m.list = m.list.SetContext(tui.ContextLabel(m.target, m.engines.Name())) + m.list = m.list.SetContext(m.contextLabel()) return m.reloadFresh() case "o": // Disabled for a commit diff: the files are historical, so editing the @@ -336,30 +366,74 @@ func (m Model) reloadFresh() (tea.Model, tea.Cmd) { return m, cmd } -// reloadFiles re-reads the changed-file list for the working-tree scope (staged -// side and pathspec), refreshes reviewed hashes, and repopulates the list while -// keeping the selection on the same file when it survives. A commit diff shows a -// fixed historical set, so it only reloads the current preview. +// reloadFiles re-reads the changed-file list for the current scope (staged +// side, base ref, and pathspec) and applies it. A commit diff shows a fixed +// historical set, so it only reloads the current preview. func (m Model) reloadFiles() (tea.Model, tea.Cmd) { if m.commitDiff { return m.reloadFresh() } - files, err := m.repo.ChangedFiles(m.staged) + files, err := m.repo.ListChanged(m.staged, m.base, "", m.paths...) if err != nil { return m, nil } - git.SortByPath(files) - m.files = git.FilterByPaths(files, m.paths) + return m.applyFiles(files, nil) +} + +// applyFiles replaces the changed-file set (hashes may carry precomputed +// worktree content hashes; nil means compute them) and repopulates the list, +// keeping the selection on the same file when it survives. Only files whose +// record or content actually changed lose their cached preview — so a watch +// refresh triggered by some other file leaves the diff being read untouched. +func (m Model) applyFiles(files []git.ChangedFile, hashes map[string]string) (tea.Model, tea.Cmd) { + if hashes == nil { + hashes = review.ContentHashes(m.repo, files) + } + stale := staleKeys(m.files, files, m.hashes(), hashes) + m.files = files if m.marks != nil { - m.marks.hashes = review.ContentHashes(m.repo, m.files) + m.marks.hashes = hashes } prevKey := m.list.SelectedKey() m.list = m.list.SetListTitle(m.listTitle()) var cmd tea.Cmd - m.list, cmd = m.list.SetItemsSelecting(m.displayFiles(), prevKey) + m.list, cmd = m.list.SetItemsSelectingStale(m.displayFiles(), prevKey, stale) return m, cmd } +// hashes is the current content-hash map (empty when reviewing is off). +func (m Model) hashes() map[string]string { + if m.marks == nil { + return nil + } + return m.marks.hashes +} + +// staleKeys is the set of preview cache keys invalidated by a file-set change: +// files whose record (status/stats) or content hash differs, plus files that +// disappeared, plus the synthetic "All changes" entry (keyed ""), whose +// preview aggregates everything. +func staleKeys(old, new []git.ChangedFile, oldHashes, newHashes map[string]string) map[string]bool { + stale := map[string]bool{"": true} + prev := make(map[string]git.ChangedFile, len(old)) + for _, f := range old { + prev[f.Path] = f + } + seen := make(map[string]bool, len(new)) + for _, f := range new { + seen[f.Path] = true + if p, ok := prev[f.Path]; !ok || p != f || oldHashes[f.Path] != newHashes[f.Path] { + stale[f.Path] = true + } + } + for path := range prev { + if !seen[path] { + stale[path] = true + } + } + return stale +} + func (m Model) View() tea.View { return m.list.TeaView() } // Filtering and ShowingHelp expose the embedded list's modal state, so a parent @@ -404,14 +478,14 @@ func (m Model) previewOpts() diff.DiffOpts { // previewFiles is the list of files a selection previews: just the selected file, // or every changed file for the synthetic "All changes" entry (empty path). -func previewFiles(sel git.ChangedFile, visible []git.ChangedFile) []string { +func previewFiles(sel git.ChangedFile, visible []git.ChangedFile) []git.ChangedFile { if sel.Path != "" { - return []string{sel.Path} + return []git.ChangedFile{sel} } - var files []string + var files []git.ChangedFile for _, f := range visible { if f.Path != "" { - files = append(files, f.Path) + files = append(files, f) } } return files diff --git a/internal/tui/diff/model_test.go b/internal/tui/diff/model_test.go index a4977fa..e19fa39 100644 --- a/internal/tui/diff/model_test.go +++ b/internal/tui/diff/model_test.go @@ -11,7 +11,7 @@ import ( type stubEngine struct{} func (stubEngine) Name() string { return "difftastic" } -func (stubEngine) Diff(context.Context, string, string, diff.DiffOpts) (string, error) { +func (stubEngine) Diff(context.Context, string, diff.File, diff.DiffOpts) (string, error) { return "", nil } func (stubEngine) DiffHunks(context.Context, []diff.Hunk, string, string, bool, int) []string { @@ -38,10 +38,10 @@ func TestDisplayFilesEmpty(t *testing.T) { // changed file for the synthetic "All changes" entry. func TestPreviewFiles(t *testing.T) { visible := []git.ChangedFile{{Path: ""}, {Path: "a"}, {Path: "b"}} - if got := previewFiles(git.ChangedFile{Path: "a"}, visible); len(got) != 1 || got[0] != "a" { + if got := previewFiles(git.ChangedFile{Path: "a"}, visible); len(got) != 1 || got[0].Path != "a" { t.Errorf("single file: got %v", got) } - if got := previewFiles(git.ChangedFile{Path: ""}, visible); len(got) != 2 || got[0] != "a" || got[1] != "b" { + if got := previewFiles(git.ChangedFile{Path: ""}, visible); len(got) != 2 || got[0].Path != "a" || got[1].Path != "b" { t.Errorf("all changes: got %v (should skip the empty All entry)", got) } } diff --git a/internal/tui/engine.go b/internal/tui/engine.go index b8ea71e..aad0baf 100644 --- a/internal/tui/engine.go +++ b/internal/tui/engine.go @@ -2,37 +2,41 @@ package tui import "github.com/madhermit/rift/internal/diff" -// EngineToggle holds the active diff engine alongside its alternate (the plain -// line engine), letting a screen flip between them with `e`. When only the plain -// engine is available the two sides match and the flip is a no-op. +// EngineToggle pairs the primary diff engine (difftastic when available) with +// git's own diff as the alternate, cycled with `e`. When the primary already +// is the git engine (difftastic unavailable) there is nothing to toggle. type EngineToggle struct { - active diff.Engine - alt diff.Engine - canToggle bool // the pair is fixed, so toggle-ability is computed once + engines []diff.Engine + idx int } -// NewEngineToggle pairs the given engine with the plain line engine as its -// alternate. +// NewEngineToggle builds the engine ring starting from the given primary +// engine. An alternate matching the primary isn't added twice, so a ring +// seeded with an already-cycled engine (a log drilldown passes the active +// engine) stays duplicate-free. func NewEngineToggle(engine diff.Engine) EngineToggle { - alt := diff.NewPlainEngine() - return EngineToggle{active: engine, alt: alt, canToggle: engine.Name() != alt.Name()} + engines := []diff.Engine{engine} + if alt := diff.NewPlainEngine(); alt.Name() != engine.Name() { + engines = append(engines, alt) + } + return EngineToggle{engines: engines} } // Engine is the active engine — what previews are rendered with. -func (e EngineToggle) Engine() diff.Engine { return e.active } +func (e EngineToggle) Engine() diff.Engine { return e.engines[e.idx] } // Name is the active engine's name, used as the header context label. -func (e EngineToggle) Name() string { return e.active.Name() } +func (e EngineToggle) Name() string { return e.Engine().Name() } -// CanToggle reports whether the two engines differ (false when only the plain -// engine is available). -func (e EngineToggle) CanToggle() bool { return e.canToggle } +// CanToggle reports whether there is more than one engine to cycle through +// (false without difftastic — the hint is then hidden). +func (e EngineToggle) CanToggle() bool { return len(e.engines) > 1 } -// Toggle swaps the active and alternate engines, returning the new state; a -// no-op when only one engine is available. +// Toggle advances to the next engine in the ring, returning the new state; a +// no-op when there's nothing to cycle to. func (e EngineToggle) Toggle() EngineToggle { - if e.canToggle { - e.active, e.alt = e.alt, e.active + if len(e.engines) > 1 { + e.idx = (e.idx + 1) % len(e.engines) } return e } diff --git a/internal/tui/lens/model.go b/internal/tui/lens/model.go index e015285..68b3fb1 100644 --- a/internal/tui/lens/model.go +++ b/internal/tui/lens/model.go @@ -32,14 +32,18 @@ type Model struct { filesLens tui.PreviewChild // built lazily / at New; nil until first shown testsLens tui.PreviewChild showTests bool - unreviewed bool // open the file lens with the unreviewed-only filter on + unreviewed bool // open the file lens with the unreviewed-only filter on + watch bool // poll the working tree and refresh on change (see watch.go) + watchFP string // last polled fingerprint; "" means the next poll baselines without refreshing + watchGen int // invalidates in-flight ticks/polls when the chain or scope changes + collectGen int // gen of the in-flight tests collect; pending iff == gen (-1 when none) gen int width int height int } -func New(repo *git.Repo, engine diff.Engine, files []git.ChangedFile, scope review.DiffScope, showTests, unreviewed bool) Model { - m := Model{repo: repo, engine: engine, scope: scope, files: files, showTests: showTests, unreviewed: unreviewed} +func New(repo *git.Repo, engine diff.Engine, files []git.ChangedFile, scope review.DiffScope, showTests, unreviewed, watch bool) Model { + m := Model{repo: repo, engine: engine, scope: scope, files: files, showTests: showTests, unreviewed: unreviewed, watch: watch, collectGen: -1} if showTests { m.testsLens = m.buildTests() // sync at startup is fine } else { @@ -49,29 +53,64 @@ func New(repo *git.Repo, engine diff.Engine, files []git.ChangedFile, scope revi } func (m Model) buildFiles() tui.PreviewChild { - return diffui.New(m.repo, m.engine, m.files, m.scope.Staged, m.scope.Base, m.scope.Target, true, m.scope.Paths, m.unreviewed) + lens := diffui.New(m.repo, m.engine, m.files, m.scope.Staged, m.scope.Base, m.scope.Target, true, m.scope.Paths, m.unreviewed) + return lens.SetWatching(m.watch) } func (m Model) buildTests() tui.PreviewChild { specs, _ := review.Collect(m.repo, m.scope) - return reviewui.New(m.repo, m.engine, specs, m.scope, true) + return m.newTests(specs) } -func (m Model) Init() tea.Cmd { return tui.ThemeInit() } +// newTests builds the tests lens over already-collected specs, tagged with the +// current watch state. +func (m Model) newTests(specs []review.Spec) tui.PreviewChild { + return reviewui.New(m.repo, m.engine, specs, m.scope, true).SetWatching(m.watch) +} + +func (m Model) Init() tea.Cmd { + if m.watch { + // Poll immediately: the first result baselines the fingerprint (watchFP + // is ""), off the event loop, without refreshing the just-built lens. + return tea.Batch(tui.ThemeInit(), m.pollWatch()) + } + return tui.ThemeInit() +} func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { + case watchTickMsg, watchStateMsg: + return m.watchUpdate(msg) case lensMsg: if msg.gen != m.gen { - return m, nil // a stale preview message from a lens we toggled away from + // A stale message from a superseded generation. A stream that had just + // started never got its cancel registered — invoke it here, or its + // difftastic subprocesses would run to completion unmanaged. + if sr, ok := msg.msg.(tui.StreamReadyMsg); ok && sr.Cancel != nil { + sr.Cancel() + } + return m, nil } return m.delegate(msg.msg) case lensCollectedMsg: if msg.gen != m.gen { return m, nil // a superseded tests build (toggled again before it landed) } - m.testsLens = reviewui.New(m.repo, m.engine, msg.specs, m.scope, true) + m.collectGen = -1 m = m.clearCollecting() // the specs landed; drop the "collecting…" note + if msg.apply && m.showTests { + // A watch refresh: apply the fresh specs to the shown lens in place, + // preserving its engine/layout/filter and the reviewer's selection. + if _, ok := m.testsLens.(reviewui.Model); ok { + return m.delegate(reviewui.SpecsChangedMsg{Specs: msg.specs}) + } + } + if m.showTests { + // Replacing a visible stale lens: kill any stream it started during + // the collect, or its difftastic runs outlive the discarded model. + m = m.cancelShown() + } + m.testsLens = m.newTests(msg.specs) return m.show(true) case tea.WindowSizeMsg: m.width, m.height = msg.Width, msg.Height @@ -87,6 +126,10 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if !m.scope.Committed() { return m.toggleStaged() } + case "w": + if !m.scope.Committed() { + return m.toggleWatch() + } } } return m.delegate(msg) @@ -120,7 +163,8 @@ func (m Model) toggle() (tea.Model, tea.Cmd) { // the user the collect is running. m.gen++ m = m.noteCollecting() - return m, m.collectTests(m.gen) + m.collectGen = m.gen + return m, m.collectTests(m.gen, false) } return m.show(true) } @@ -157,19 +201,41 @@ func (m Model) clearCollecting() Model { // toggle. The file lens is cheap to rebuild now; the tests lens re-parses off the // event loop (like the first switch to tests) so `s` doesn't block the UI. func (m Model) toggleStaged() (tea.Model, tea.Cmd) { - m = m.cancelShown() m.scope.Staged = !m.scope.Staged - m.gen++ - m.files = worktreeFiles(m.repo, m.scope.Staged, m.scope.Paths) + m.files, _ = scopeFiles(m.repo, m.scope) // best-effort, like the reload paths + var rearm tea.Cmd + if m.watch { + // A poll in flight captured the old scope: orphan it (watchGen bump) and + // start a fresh chain with an immediate poll — watchFP resets so that + // poll baselines the new scope off the event loop instead of reading the + // flip itself as a tree change. + m.watchGen++ + m.watchFP = "" + rearm = m.pollWatch() + } if m.showTests { - // Keep showing the now-stale tests until the re-collect lands, with a footer - // note that the collect is running. - m = m.noteCollecting() - m.filesLens = nil - return m, m.collectTests(m.gen) + model, cmd := m.recollect(false) + return model, tea.Batch(cmd, rearm) } + m = m.cancelShown() + m.gen++ // also invalidates a pending `t` collect, which targeted the old scope m.filesLens, m.testsLens = m.buildFiles(), nil - return m.delegate(tea.WindowSizeMsg{Width: m.width, Height: m.height}) + model, cmd := m.delegate(tea.WindowSizeMsg{Width: m.width, Height: m.height}) + return model, tea.Batch(cmd, rearm) +} + +// recollect invalidates the current specs and re-collects for the tests view: +// the stale tests lens stays visible (with a "collecting" note) until the +// fresh specs land. apply=true applies them to the shown lens in place (a +// watch refresh — the reviewer's view state survives); apply=false rebuilds +// the lens (a scope flip). +func (m Model) recollect(apply bool) (tea.Model, tea.Cmd) { + m = m.cancelShown() + m.gen++ + m = m.noteCollecting() + m.filesLens = nil + m.collectGen = m.gen + return m, m.collectTests(m.gen, apply) } // show switches the displayed lens and re-syncs its layout. It cancels the lens @@ -177,6 +243,13 @@ func (m Model) toggleStaged() (tea.Model, tea.Cmd) { // that lens's in-flight preview messages. func (m Model) show(tests bool) (tea.Model, tea.Cmd) { m = m.cancelShown() + if !tests && m.collectGen == m.gen { + // The gen bump below drops the pending collect, and its fingerprint has + // already been consumed — the retained tests lens predates the tree that + // collect targeted, so drop it too; the next `t` re-collects instead of + // showing stale specs indefinitely. + m.testsLens = nil + } m.showTests = tests m.gen++ return m.delegate(tea.WindowSizeMsg{Width: m.width, Height: m.height}) @@ -199,30 +272,27 @@ func (m Model) delegate(msg tea.Msg) (tea.Model, tea.Cmd) { func (m Model) View() tea.View { return m.shown().View() } -func (m Model) collectTests(gen int) tea.Cmd { +func (m Model) collectTests(gen int, apply bool) tea.Cmd { repo, scope := m.repo, m.scope return func() tea.Msg { specs, _ := review.Collect(repo, scope) - return lensCollectedMsg{gen: gen, specs: specs} + return lensCollectedMsg{gen: gen, apply: apply, specs: specs} } } -// worktreeFiles lists the changed files for a working-tree staged toggle, sorted -// and pathspec-filtered like the diff command's own listing — so the file lens -// keeps the `-- path` scope that the tests lens (via review.Collect) already does. -func worktreeFiles(repo *git.Repo, staged bool, paths []string) []git.ChangedFile { - files, err := repo.ChangedFiles(staged) - if err != nil { - return nil - } - git.SortByPath(files) - return git.FilterByPaths(files, paths) +// scopeFiles lists the changed files for a working-tree scope (staged side, +// base ref, pathspec), sorted and filtered like the diff command's own listing +// — so the file lens keeps the `-- path` scope that the tests lens (via +// review.Collect) already does. +func scopeFiles(repo *git.Repo, scope review.DiffScope) ([]git.ChangedFile, error) { + return repo.ListChanged(scope.Staged, scope.Base, "", scope.Paths...) } // lensCollectedMsg delivers the parsed tests for the first switch to the tests // lens, tagged with the generation that requested it. type lensCollectedMsg struct { gen int + apply bool // apply the specs to the shown lens in place (a watch refresh) specs []review.Spec } diff --git a/internal/tui/lens/watch.go b/internal/tui/lens/watch.go new file mode 100644 index 0000000..ed5adc4 --- /dev/null +++ b/internal/tui/lens/watch.go @@ -0,0 +1,160 @@ +package lensui + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/madhermit/rift/internal/git" + "github.com/madhermit/rift/internal/review" + diffui "github.com/madhermit/rift/internal/tui/diff" + reviewui "github.com/madhermit/rift/internal/tui/review" +) + +// Watch mode (`rift diff --watch`) keeps the lens current while something else +// edits the working tree — typically an agent writing code in another pane. It +// polls rather than watching the filesystem: the poll is a handful of fast git +// subprocesses, and polling can't hit inotify watch limits or miss events on +// network filesystems. Each poll fingerprints the scope's changed files; a +// changed fingerprint refreshes the shown lens in place, invalidating only the +// previews whose content actually changed — so the selection and the reader's +// scroll position survive. + +const watchInterval = time.Second + +// watchTickMsg schedules the next poll; watchStateMsg carries a poll's result. +// Both carry the watch generation they were started under: the `w` toggle and +// the `s` scope flip bump it, so a tick or poll left in flight is dropped +// instead of applying a stale scope or running a second, parallel polling +// chain. +type watchTickMsg struct{ gen int } + +type watchStateMsg struct { + gen int + failed bool // the listing errored; skip this poll rather than treat it as empty + fingerprint string + files []git.ChangedFile + hashes map[string]string +} + +func watchTick(gen int) tea.Cmd { + return tea.Tick(watchInterval, func(time.Time) tea.Msg { return watchTickMsg{gen: gen} }) +} + +// pollWatch re-lists the scope's changed files off the event loop and +// fingerprints them, so Update can tell whether anything the lens shows has +// changed. A listing error is reported as failed — never as an empty set, +// which would wipe the lens. The message always carries worktree content +// hashes (the reviewed-marks identity, forwarded to the file lens); the +// fingerprint side additionally uses index hashes for the staged scope, where +// a re-stage must register even when the worktree and the stat line don't +// change. +func (m Model) pollWatch() tea.Cmd { + repo, scope, gen := m.repo, m.scope, m.watchGen + return func() tea.Msg { + files, err := scopeFiles(repo, scope) + if err != nil { + return watchStateMsg{gen: gen, failed: true} + } + hashes := review.ContentHashes(repo, files) + fpHashes := hashes + if scope.Staged { + fpHashes = repo.StagedBlobHashes(git.Paths(files)) + } + return watchStateMsg{gen: gen, fingerprint: fingerprint(files, fpHashes), files: files, hashes: hashes} + } +} + +// fingerprint reduces a changed-file set to a comparable identity: the listing +// itself plus each file's content hash, so an edit that leaves the stat line +// unchanged (`x := 1` → `x := 2`) still registers. +func fingerprint(files []git.ChangedFile, hashes map[string]string) string { + h := sha256.New() + for _, f := range files { + fmt.Fprintf(h, "%s\x00%s\x00%s\x00%d\x00%d\x00%s\x00", f.Path, f.OldPath, f.Status, f.Added, f.Deleted, hashes[f.Path]) + } + return hex.EncodeToString(h.Sum(nil)) +} + +// watchUpdate handles the watch loop's messages: a tick polls, and a poll +// result either re-arms the tick (nothing changed, or the listing failed) or +// refreshes the shown lens before re-arming. +func (m Model) watchUpdate(msg tea.Msg) (tea.Model, tea.Cmd) { + switch msg := msg.(type) { + case watchTickMsg: + if !m.watch || msg.gen != m.watchGen { + return m, nil // watch toggled off, or a superseded chain + } + return m, m.pollWatch() + case watchStateMsg: + if !m.watch || msg.gen != m.watchGen { + return m, nil + } + if msg.failed || msg.fingerprint == m.watchFP { + return m, watchTick(m.watchGen) + } + if m.watchFP == "" { + // Baseline poll (startup or a scope flip): the lens was just built + // from this state, so record the fingerprint without refreshing. + m.watchFP = msg.fingerprint + return m, watchTick(m.watchGen) + } + m.watchFP = msg.fingerprint + model, cmd := m.refreshChanged(msg.files, msg.hashes) + return model, tea.Batch(cmd, watchTick(m.watchGen)) + } + return m, nil +} + +// toggleWatch flips watch mode live (`w`). Enabling polls immediately, off the +// event loop: watchFP survives from before the last disable, so re-enabling +// over an unchanged tree refreshes nothing, while any drift that accumulated +// while watch was off is caught by that first poll. +func (m Model) toggleWatch() (tea.Model, tea.Cmd) { + m.watch = !m.watch + m.watchGen++ // orphan any tick/poll left in flight by the previous chain + m = m.setWatching(m.watch) + if !m.watch { + return m, nil + } + return m, m.pollWatch() +} + +// setWatching re-tags both built lenses' header context with the current watch +// state; buildFiles/newTests tag lenses built later. +func (m Model) setWatching(on bool) Model { + if d, ok := m.filesLens.(diffui.Model); ok { + m.filesLens = d.SetWatching(on) + } + if r, ok := m.testsLens.(reviewui.Model); ok { + m.testsLens = r.SetWatching(on) + } + return m +} + +// refreshChanged applies an externally observed working-tree change. The shown +// lens refreshes in place — the file lens keeps its selection and the cached +// previews of untouched files, the tests lens re-collects and applies the +// specs in place — and the hidden lens is dropped so its next toggle rebuilds +// against the new state. +func (m Model) refreshChanged(files []git.ChangedFile, hashes map[string]string) (tea.Model, tea.Cmd) { + m.files = files + if m.showTests { + return m.recollect(true) + } + m.testsLens = nil + // Invalidate in-flight messages for the pre-change tree. A `t` press racing + // this refresh had its collect dropped by the bump, so relaunch it against + // the new tree — the switch to tests then happens when the fresh specs land. + pending := m.collectGen == m.gen + m.gen++ + var relaunch tea.Cmd + if pending { + m.collectGen = m.gen + relaunch = m.collectTests(m.gen, false) + } + model, cmd := m.delegate(diffui.FilesChangedMsg{Files: files, Hashes: hashes}) + return model, tea.Batch(cmd, relaunch) +} diff --git a/internal/tui/lens/watch_test.go b/internal/tui/lens/watch_test.go new file mode 100644 index 0000000..ee7c6a6 --- /dev/null +++ b/internal/tui/lens/watch_test.go @@ -0,0 +1,66 @@ +package lensui + +import ( + "testing" + + "github.com/madhermit/rift/internal/git" +) + +func TestFingerprint(t *testing.T) { + files := []git.ChangedFile{ + {Path: "a.go", Status: "Modified", Added: 3, Deleted: 1}, + {Path: "b.go", Status: "Untracked", Added: 10}, + } + hashes := map[string]string{"a.go": "h1", "b.go": "h2"} + base := fingerprint(files, hashes) + + if fingerprint(files, hashes) != base { + t.Error("fingerprint not stable for identical input") + } + + tests := []struct { + name string + files []git.ChangedFile + hashes map[string]string + }{ + { + // An edit that leaves the stat line unchanged must still register. + name: "content hash changed", + files: files, + hashes: map[string]string{"a.go": "h1-edited", "b.go": "h2"}, + }, + { + name: "stat changed", + files: []git.ChangedFile{{Path: "a.go", Status: "Modified", Added: 4, Deleted: 1}, files[1]}, + hashes: hashes, + }, + { + name: "file added", + files: append(append([]git.ChangedFile{}, files...), git.ChangedFile{Path: "c.go", Status: "Added"}), + hashes: hashes, + }, + { + name: "file removed", + files: files[:1], + hashes: hashes, + }, + { + // A rename record with the same stats must register (OldPath is hashed). + name: "rename recorded", + files: []git.ChangedFile{{Path: "a.go", OldPath: "z.go", Status: "Modified", Added: 3, Deleted: 1}, files[1]}, + hashes: hashes, + }, + { + name: "empty set", + files: nil, + hashes: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if fingerprint(tt.files, tt.hashes) == base { + t.Error("fingerprint did not change") + } + }) + } +} diff --git a/internal/tui/log/model.go b/internal/tui/log/model.go index a31d76e..eb28d83 100644 --- a/internal/tui/log/model.go +++ b/internal/tui/log/model.go @@ -365,7 +365,7 @@ func (m Model) previewCmd(reqID int) tea.Cmd { return tui.StreamReadyMsg{ReqID: reqID, Header: header} } opts.Base, opts.Target = base, commit.Hash - ch, cancel := tui.StreamFiles(engine, repo.Root(), git.Paths(files), opts) + ch, cancel := tui.StreamFiles(engine, repo.Root(), files, opts) return tui.StreamReadyMsg{ReqID: reqID, Header: header, Ch: ch, Cancel: cancel} } } diff --git a/internal/tui/log/model_smoke_test.go b/internal/tui/log/model_smoke_test.go index 3c5a8d2..49643d7 100644 --- a/internal/tui/log/model_smoke_test.go +++ b/internal/tui/log/model_smoke_test.go @@ -13,7 +13,7 @@ import ( type stubEngine struct{} func (stubEngine) Name() string { return "difftastic" } -func (stubEngine) Diff(context.Context, string, string, diff.DiffOpts) (string, error) { +func (stubEngine) Diff(context.Context, string, diff.File, diff.DiffOpts) (string, error) { return "", nil } func (stubEngine) DiffHunks(context.Context, []diff.Hunk, string, string, bool, int) []string { diff --git a/internal/tui/review/model.go b/internal/tui/review/model.go index 7be8fff..b54bd37 100644 --- a/internal/tui/review/model.go +++ b/internal/tui/review/model.go @@ -28,9 +28,10 @@ type Model struct { engines tui.EngineToggle list tui.SplitList[review.Spec] - scope review.DiffScope - display diff.Display - stream *tui.PreviewStream // active spec-diff stream; nil otherwise + scope review.DiffScope + display diff.Display + stream *tui.PreviewStream // active spec-diff stream; nil otherwise + watching bool // show the live-watch indicator in the header context } // New builds the tests lens for a diff. The scope decides whether it reads the @@ -44,7 +45,7 @@ func New(repo *git.Repo, engine diff.Engine, specs []review.Spec, scope review.D hints = append(hints, [2]string{"t", "files"}) } if !scope.Committed() { - hints = append(hints, [2]string{"s", "staged"}) + hints = append(hints, [2]string{"s", "staged"}, [2]string{"w", "watch"}) } hints = append(hints, [2]string{"\\", "layout"}) if engines.CanToggle() { @@ -60,7 +61,7 @@ func New(repo *git.Repo, engine diff.Engine, specs []review.Spec, scope review.D Screen: tui.Breadcrumb("diff", "tests"), ListTitle: listTitle(scope), Branch: repo.CurrentBranch(), - Context: tui.ContextLabel(scope.Target, engine.Name()), + Context: tui.ContextLabel(scope.Target, engine.Name(), false), NavFraction: 40, EmptyStatus: "No test changes in scope", Hints: hints, @@ -153,6 +154,8 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, cmd case tui.EditorClosedMsg: return m.reloadFresh() // the file may have changed + case SpecsChangedMsg: + return m.applySpecs(msg.Specs) case tea.KeyPressMsg: if m.list.Filtering() || m.list.ShowingHelp() { break @@ -166,7 +169,7 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { break } m.engines = m.engines.Toggle() - m.list = m.list.SetContext(tui.ContextLabel(m.scope.Target, m.engines.Name())) + m.list = m.list.SetContext(m.contextLabel()) return m.reloadFresh() case "o": if sel, ok := m.list.Selected(); ok { @@ -186,6 +189,36 @@ func (m Model) View() tea.View { return m.list.TeaView() } func (m Model) Filtering() bool { return m.list.Filtering() } func (m Model) ShowingHelp() bool { return m.list.ShowingHelp() } +// SpecsChangedMsg tells the tests lens its specs were re-collected externally +// (the watch poller). The lens applies them in place — unlike a rebuild, the +// engine cycle, layout, filter, and selection survive. +type SpecsChangedMsg struct { + Specs []review.Spec +} + +// applySpecs replaces the spec set, keeping the selection on the same spec +// when it survives. Spec previews depend on file content that just changed, +// so the cache is dropped and the selected spec re-renders. +func (m Model) applySpecs(specs []review.Spec) (tea.Model, tea.Cmd) { + prevKey := m.list.SelectedKey() + var cmd tea.Cmd + m.list, cmd = m.list.SetItemsSelecting(specs, prevKey) + return m, cmd +} + +// SetWatching sets the "watching" tag in the header context, marking the lens +// as live-reloading. The flag persists on the model because the engine toggle +// rebuilds the label. +func (m Model) SetWatching(on bool) Model { + m.watching = on + m.list = m.list.SetContext(m.contextLabel()) + return m +} + +func (m Model) contextLabel() string { + return tui.ContextLabel(m.scope.Target, m.engines.Name(), m.watching) +} + // SetBreadcrumb marks this view as an embedded drilldown: it sets the header // breadcrumb (e.g. "log ❯ a1b2c3d ❯ tests") and adds an "esc back" footer hint. func (m Model) SetBreadcrumb(screen string) Model { @@ -225,7 +258,7 @@ func (m Model) previewCmd(reqID int) tea.Cmd { opts.Staged, opts.Base, opts.Target = m.scope.Staged, m.scope.Base, m.scope.Target root, engine := m.repo.Root(), m.engines.Engine() return func() tea.Msg { - ch, cancel := tui.StreamFiles(engine, root, []string{sel.File}, opts) + ch, cancel := tui.StreamFiles(engine, root, []git.ChangedFile{{Path: sel.File, OldPath: sel.OldPath}}, opts) return tui.StreamReadyMsg{ReqID: reqID, Ch: ch, Cancel: cancel} } } diff --git a/internal/tui/splitlist.go b/internal/tui/splitlist.go index c38b70f..cb83c96 100644 --- a/internal/tui/splitlist.go +++ b/internal/tui/splitlist.go @@ -297,19 +297,58 @@ func (m SplitList[T]) SetItems(items []T) (SplitList[T], tea.Cmd) { // value equals key (when it survives the new set/filter), instead of resetting to // the top. Used to preserve the user's position across a file-list reload. func (m SplitList[T]) SetItemsSelecting(items []T, key string) (SplitList[T], tea.Cmd) { + return m.setItems(items, key, nil) +} + +// SetItemsSelectingStale is SetItemsSelecting for an in-place refresh (the +// watch poller): instead of dropping the whole cache, it evicts only the +// previews whose CacheKey is in stale — and when the selection survives on an +// item that isn't stale, it leaves the shown preview entirely untouched (no +// re-request, no viewport reset), so a change elsewhere in the tree doesn't +// cost the reader their place in the diff they're reading. +func (m SplitList[T]) SetItemsSelectingStale(items []T, key string, stale map[string]bool) (SplitList[T], tea.Cmd) { + return m.setItems(items, key, stale) +} + +// setItems is the shared core of the SetItems* family. nil stale means the +// item set reflects a different state entirely: the whole cache drops and the +// selection's preview is re-requested. A non-nil stale set (of raw CacheKey +// values — stored cache keys carry a width qualifier that is stripped for the +// comparison) evicts only those previews, and a surviving, non-stale selection +// keeps its shown preview untouched. +func (m SplitList[T]) setItems(items []T, key string, stale map[string]bool) (SplitList[T], tea.Cmd) { m.items = items - m.cache = map[string]string{} + if stale == nil { + m.cache = map[string]string{} + } else { + for k := range m.cache { + if raw, _, _ := strings.Cut(k, "\x00"); stale[raw] { + delete(m.cache, k) + } + } + } m.filtered = FuzzyFilter(m.items, m.filter.Value(), m.cfg.Match) m.selected = 0 + kept := false if key != "" && m.cfg.Match != nil { for i, it := range m.filtered { if m.cfg.Match(it) == key { m.selected = i + kept = true break } } } m.resizeViewport() + if it, ok := m.Selected(); stale != nil && kept && ok && !m.loading && m.prevErr == nil { + raw := "" + if m.cfg.CacheKey != nil { + raw = m.cfg.CacheKey(it) + } + if raw != "" && !stale[raw] { + return m, nil // shown preview is still current; don't move the reader + } + } return m.requestPreview() } diff --git a/internal/tui/stash/model.go b/internal/tui/stash/model.go index 06dcd60..ff1f72b 100644 --- a/internal/tui/stash/model.go +++ b/internal/tui/stash/model.go @@ -205,7 +205,7 @@ func (m Model) previewCmd(reqID int) tea.Cmd { if len(files) == 0 { return tui.StreamReadyMsg{ReqID: reqID} } - ch, cancel := tui.StreamFiles(engine, repo.Root(), git.Paths(files), opts) + ch, cancel := tui.StreamFiles(engine, repo.Root(), files, opts) return tui.StreamReadyMsg{ReqID: reqID, Ch: ch, Cancel: cancel} } } diff --git a/internal/tui/stream.go b/internal/tui/stream.go index c88cb91..5864546 100644 --- a/internal/tui/stream.go +++ b/internal/tui/stream.go @@ -6,6 +6,7 @@ import ( tea "charm.land/bubbletea/v2" "github.com/madhermit/rift/internal/diff" + "github.com/madhermit/rift/internal/git" ) // ChunkMsg carries one item's rendered preview from a progressive stream. More @@ -34,41 +35,43 @@ func PreviewDiffOpts(width int, display diff.Display) diff.DiffOpts { return diff.DiffOpts{Color: ColorEnabled(), Width: width, Display: display} } -// StreamFiles diffs paths concurrently and returns a channel yielding each file's -// rendered diff in order (for a progressive preview), plus a cancel that stops -// the work — killing the difftastic subprocesses of a stream the user navigated -// away from. Shared by the diff, log, and stash previews. -func StreamFiles(engine diff.Engine, root string, paths []string, opts diff.DiffOpts) (<-chan string, func()) { +// StreamFiles diffs files concurrently and returns a channel yielding each +// file's rendered diff in order (for a progressive preview), plus a cancel that +// stops the work — killing the difftastic subprocesses of a stream the user +// navigated away from. Shared by the diff, log, and stash previews. Each +// file's OldPath (when renamed) rides into the engine as part of its identity. +func StreamFiles(engine diff.Engine, root string, files []git.ChangedFile, opts diff.DiffOpts) (<-chan string, func()) { ctx, cancel := context.WithCancel(context.Background()) - ch := diff.ParallelStream(len(paths), func(i int) string { - return renderFileDiff(ctx, engine, root, paths[i], opts) + ch := diff.ParallelStream(len(files), func(i int) string { + return renderFileDiff(ctx, engine, root, files[i], opts) }) return ch, cancel } // renderFileDiff diffs one file, prefixing a section banner. The banner is a // hidden marker — the preview strips it from the body (the diff's own header is -// the visible boundary) and pins its path in the legend as you scroll into the -// file; see VimNav.SetContent. A file whose diff fails renders a visible marker -// rather than vanishing silently; a cancelled diff (navigated away) renders -// nothing, since it's discarded. -func renderFileDiff(ctx context.Context, engine diff.Engine, root, file string, opts diff.DiffOpts) string { - content, err := engine.Diff(ctx, root, file, opts) +// the visible boundary) and pins its label in the legend as you scroll into the +// file (a rename reads "old → new"); see VimNav.SetContent. A file whose diff +// fails renders a visible marker rather than vanishing silently; a cancelled +// diff (navigated away) renders nothing, since it's discarded. +func renderFileDiff(ctx context.Context, engine diff.Engine, root string, f git.ChangedFile, opts diff.DiffOpts) string { + content, err := engine.Diff(ctx, root, diff.File{Path: f.Path, OldPath: f.OldPath}, opts) + label := f.DisplayPath() switch { case ctx.Err() != nil: return "" case err != nil: - return diffErrorMarker(file, opts, err) + return diffErrorMarker(label, opts, err) case content == "": return "" default: - return SectionBanner(file, opts.Width) + "\n" + content + "\n" + return SectionBanner(label, opts.Width) + "\n" + content + "\n" } } -func diffErrorMarker(file string, opts diff.DiffOpts, err error) string { +func diffErrorMarker(label string, opts diff.DiffOpts, err error) string { line := strings.SplitN(err.Error(), "\n", 2)[0] - return SectionBanner(file, opts.Width) + "\n ⚠ diff unavailable: " + line + "\n" + return SectionBanner(label, opts.Width) + "\n ⚠ diff unavailable: " + line + "\n" } // PreviewStream tracks an in-flight progressive preview feeding a SplitList: a diff --git a/internal/tui/theme.go b/internal/tui/theme.go index 5c1919b..9a650ba 100644 --- a/internal/tui/theme.go +++ b/internal/tui/theme.go @@ -258,12 +258,16 @@ func HeaderContext(branch, rest string) string { // ContextLabel is the header right-context for a diff-backed screen: the engine // name, prefixed with the target ref when viewing a committed range (target is -// "" for a working-tree/staged diff). -func ContextLabel(target, engineName string) string { - if target == "" { - return engineName +// "" for a working-tree/staged diff) and tagged when the screen live-reloads. +func ContextLabel(target, engineName string, watching bool) string { + label := engineName + if target != "" { + label = target + " · " + engineName } - return target + " · " + engineName + if watching { + label += " · watching" + } + return label } // Footer renders the two-row bottom bar: a faint rule then a status segment