feat(ui): Open focused source in editor by abhinav · Pull Request #233 · umputun/revdiff

abhinav · Jun 20, 2026

Add an open_file_in_editor action for the diff pane so reviewers can jump from the focused row to the current worktree file in their configured editor.

The default binding is e. Ctrl+E keeps the existing annotation-editor behavior.

Line navigation is best effort for worktree-backed reviews. Added and context rows request their current line, removed rows use the nearest current source line, and staged or ref reviews still request a line without reloading the displayed diff after a clean editor exit.

Demos:

Opening an added line

Opening a deleted line opens the nearest available line

Opening a collapsed deleted line does not work

Add an `open_file_in_editor` action for the diff pane so reviewers can jump from the focused row to the current worktree file in their configured editor. The default binding is `e`. `Ctrl+E` keeps the existing annotation-editor behavior. Line navigation is best effort for worktree-backed reviews. Added and context rows request their current line, removed rows use the nearest current source line, and staged or ref reviews still request a line without reloading the displayed diff after a clean editor exit. The editor package owns command construction and source-file checks, so the UI only maps diff state to an editor target and status hint. Documentation and plugin references describe the new action, resolver order, line syntax, and reload behavior.

Copilot

Pull request overview

Adds a new diff-pane action to open the currently focused worktree file (optionally at the best-effort resolved line) in the user’s configured external editor, enabling quick “jump to source” from within revdiff.

Changes:

Introduces a new keymap action open_file_in_editor with default binding e (while keeping Ctrl+E for annotation editing).
Implements source-file editor launching via ExternalEditor.SourceCommand(...), including best-effort line resolution for add/context/remove rows and refresh-on-return behavior for worktree reviews.
Updates documentation/reference materials and expands unit test coverage for the new editor-opening flow.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
site/docs.html	Documents `e` binding and `open_file_in_editor` action in the site docs.
README.md	Updates README keybindings/actions and describes editor line-navigation behavior.
plugins/codex/skills/revdiff/references/usage.md	Updates Codex skill reference docs for the new `e` behavior.
plugins/codex/skills/revdiff/references/config.md	Updates available-actions reference list to include `open_file_in_editor` (and related actions).
docs/ARCHITECTURE.md	Extends architecture docs to describe the new “source editor” flow and Editor API.
app/ui/view.go	Adds editor hint state into transient hint priority ordering.
app/ui/mouse.go	Clears editor transient hint on mouse interactions (parity with key handling).
app/ui/model.go	Adds `editorState` to model state and routes `sourceEditorFinishedMsg` in Update loop.
app/ui/mocks/external_editor.go	Extends UI editor mock to support `SourceCommand`.
app/ui/editor.go	Implements `openSourceEditor`, target/line resolution, and completion handling with refresh policy.
app/ui/editor_test.go	Adds unit tests covering source-target selection/line resolution and dispatch.
app/ui/doc.go	Updates package docs to reflect both annotation editing and source-file editor handoff.
app/ui/diffnav.go	Wires `ActionOpenFileInEditor` into diff-pane action dispatch.
app/keymap/keymap.go	Defines `open_file_in_editor`, adds validation/help entry, and binds default `e`.
app/keymap/keymap_test.go	Adds tests for action validity, default binding, and help entry.
app/editor/editor.go	Adds `SourceTarget` + `SourceCommand` with editor-specific line-navigation arg support.
app/editor/editor_test.go	Adds tests for source command line-arg syntax, basename recognition, and validation.
.claude-plugin/skills/revdiff/references/usage.md	Mirrors usage doc updates for the Claude plugin reference.
.claude-plugin/skills/revdiff/references/config.md	Mirrors config action list updates for the Claude plugin reference.

Files not reviewed (1)

app/ui/mocks/external_editor.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Collapsed delete-only placeholders are rendered as the first removed row from the underlying diff. Opening the source editor from that row previously followed the normal removed-line fallback and could jump to a nearby current worktree line. Treat collapsed removed rows that are placeholders or hidden rows as selection errors instead. Expanded removed rows keep the existing best-effort behavior and still open the nearest current worktree line. Regression coverage exercises the placeholder rejection and the expanded removed-row fallback.

abhinav · Jun 20, 2026

My bad on the lint failure. On it

Copilot

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

app/ui/mocks/external_editor.go: Generated file

@@ -154,6 +154,7 @@ func (m Model) handleMouse(msg tea.MouseMsg) (tea.Model, tea.Cmd) {
 	// that reaches this point dismisses the last hint, mirroring handleKey.
 	m.reload.hint = ""
 	m.compact.hint = ""
+	m.editorState.hint = ""


umputun

couple issues to fix before merge:

open_file_in_editor should refuse to launch when the current file has line-level annotations. After the editor exits and the file reloads, annotations keyed by (file,line,type) can become orphaned or attach to the wrong line if the user deletes or moves annotated content. Since there is no stable anchor/rebase mechanism, blocking source editing for line-annotated files is safer than silently corrupting review comments. File-level annotations can still be allowed.
sourceEditorFinishedMsg should still capture the opened file. Completion currently calls m.reloadCurrentFile(), so it reloads whatever m.file.name is when the editor exits. If a queued file-load message changes the displayed file while the TUI is suspended, the file just edited may not be reloaded and another file may refresh instead. Capture the opened file and reload only that target, or reload only if it is still displayed.
workDir != "" is not a safe capability gate for this feature. --stdin is blocked, but compare mode, --all-files, standalone --only, and staged/ref reviews all have different source and reload semantics. Pls wire an explicit source-editor policy from the composition root instead of deriving support from workDir inside the UI. At minimum, document and test the supported modes: working tree reloads, staged/ref is best effort with no reload, compare opens --compare-new, stdin is unavailable.
pls reject source paths that escape the worktree, both lexically and after symlink resolution. sourceEditorTarget joins workDir + file, and SourceCommand uses os.Stat, which follows symlinks. A renderer path with .. or a symlinked repo file can point outside the worktree and e would open that target in the editor.

minor:

I'd avoid exposing editor.SourceTarget through the UI-side ExternalEditor interface. SourceCommand(path string, line int) or a UI-local request keeps the consumer-side interface cleaner.
app/ui/mouse.go:153 says mouse hint clearing mirrors handleKey, but mouse only clears reload/compact/editor hints while handleKey also clears key-chord and vim-motion hints. Either narrow the comment or match the behavior.

abhinav · Jun 20, 2026

open_file_in_editor should refuse to launch when the current file has line-level annotations.

Argh. I see the concern and I agree.
But that also has a slightly degraded experience: you're reviewing the file and then see a minor thing that's not worth round-tripping with the agent for, but can't edit.
But I agree that's still better than data loss.

If you're amenable to it, I'll start with a version where it's forbidden for annotated files but only if we're reviewing dirty changes. For staged, or for older commits or stdin, the diff output is stable and won't change. WDYT?

Will work on the others comments as well.

Source editor targets should reject repository-relative paths that leave the worktree before launching the editor. Reject lexical escapes with filepath.IsLocal. Reject symlink escapes with os.Root. Per os.Root docs: > Methods on Root will follow symbolic links, > but symbolic links may not reference a location outside the root. > Symbolic links must not be absolute.

Opening a worktree file in the source editor can reload the displayed diff on a clean editor exit. That reload can invalidate line-level annotation coordinates that were made against the pre-edit diff. Reject source-editor targets for worktree-change reviews when the current file has any line-level annotation. File-level annotations do not depend on line coordinates and still allow editing. Staged and ref reviews keep allowing source-editor launches even with line-level annotations because those modes return to the existing diff instead of reloading the displayed file after a clean editor exit.

A source-editor session can finish after the user has moved the UI to a different file. The worktree refresh path must reload the file that launched the editor, not whichever file is displayed when the editor process returns. Capture the displayed diff file name at launch time and carry it through `sourceEditorFinishedMsg`.

Source editor availability now comes from a policy selected at the composition root instead of UI checks against workDir, staged, and ref. This keeps stdin disabled, lets staged and ref reviews open files without reloading, and lets worktree reviews reload only the same displayed file after a clean editor exit. Compare file-vs-file reviews now carry the exact --compare-new path into the source editor policy, so opening the displayed file targets the new side even though the diff entry is shown by basename. Unit tests cover the mode policy and the UI target resolution contracts.

Source editor opening only needs a path and optional one-based line at the call boundary. Passing those values directly keeps the UI-owned ExternalEditor interface independent of the app/editor request shape while preserving app/editor ownership of path validation and editor-specific line arguments.

abhinav · Jun 20, 2026

All comments addressed. Kept everything as separate commits for (hopefully) ease of review.

umputun

one remaining thing: pls sync the source-editor docs in the agent/plugin reference files with the updated README/site wording.

These still describe the old behavior and miss the current mode/annotation constraints:

.claude-plugin/skills/revdiff/references/usage.md
plugins/codex/skills/revdiff/references/usage.md

They should mention the same points as README.md: stable source path only, compare mode opens --compare-new, stdin is unavailable, and working-tree files with line annotations cannot be opened for editing.

The plugin reference docs should describe the same source-file opening behavior as the README so reviewers see one contract for `e`. The docs now state that `e` requires a stable source path, that known editors receive line-navigation arguments while unknown editors receive only the path, and that line lookup is best effort. They also document the reload behavior for working tree changes, the no-reload behavior for `--staged` and refs, `--compare-new` selection in compare mode, and the `--stdin` and annotation limits.

abhinav · Jun 21, 2026

@umputun updated as suggested.

Semi-related FYI: If you'd like to always keep some sections of these files in sync, I have a small tool I wrote some time ago to stitch larger .md files together from smaller ones. The idea is that sections get their own files, and then a separate file specifies the order to combine them in (similarly to mdbook's SUMMARY.md files). It just might fit into your setup.

umputun

round 2 looks good. All the round-1 items are addressed (line-annotation block, captured reload target, composition-root policy, path-escape guard, primitive interface, mouse comment), and the doc sync is done.

two minor things, neither blocking:

docs/ARCHITECTURE.md still references a type that no longer exists. Lines ~212/238/343 show SourceCommand(target editor.SourceTarget) / SourceCommand(target), but the actual signature is SourceCommand(path string, line int) and editor.SourceTarget was removed in the interface flatten. Pls sync the doc.
optional hardening, not a blocker. The worktree-escape guard in sourceEditorTarget only runs for relative paths (inside if !filepath.IsAbs), so an absolute target skips both filepath.IsLocal and the os.OpenRoot/root.Stat check. Right now that's only reachable through user-named --only paths (VCS listings stay relative and do get validated), so it's not an escape of the untrusted surface. Just flagging that absolute targets aren't confined, in case you want symmetric validation later.

The architecture document still described source-editor opening through a UI-side `SourceTarget` value. The current editor boundary passes the source path and worktree line directly as `SourceCommand(path string, line int)`. Keep the documented ownership unchanged: `editor.Editor` prepares the source-file command, validates that the path exists and is a regular file, and applies line-navigation arguments for known editors.

abhinav · Jun 22, 2026

For absolute path escape—I think yeah right now that's working off of assumptions of current use.
Maybe, based on flags, we decide at entry point whether paths are allowed to escape root (if root is set)?
e.g. --only=/tmp=foo is good, but not revdiff commita commitb getting a malicious diff output.

abhinav · Jun 22, 2026

Update: I'm going to hold off on folding that into this change unless you prefer that be handled here as well.

umputun

the ARCHITECTURE.md signature and the collapsed-delete rejection are both addressed correctly.

on the abs-path escape, agreed - let's hold it for this PR. The relative/untrusted-listing path is fully guarded, and an absolute target is only reachable through your own --only path, so symmetric validation can come later if at all.

one new thing, in the same area as the round-1 reload-target fix. handleSourceEditorFinished skips reload on msg.fileName != m.file.name, but m.file.name lags m.tree.SelectedFile() during a queued cross-file load. With --cross-file-hunks, a boundary ]/[ calls m.tree.StepFile() synchronously and queues the next-file load while m.file.name is still the old file. Press e in that window and the editor opens the old file; if that queued load is still in flight when the editor exits, reloadCurrentFile() bumps loadSeq and the next-file load is dropped as stale, so the displayed file and tree selection disagree. Narrow window and a non-default flag, no data loss, but it's the residual clobber the captured-target fix was meant to close. Anchoring the guard on m.tree.SelectedFile() (and clearing pendingHunkJump on mismatch) plus a regression test should cover it.

couple optional nits, take or leave:

sourceEditorTargetResult is package-private but has exported fields with godoc-style comments - editorFinishedMsg right above it is unexported and lowercase, could match that.
SourceCommand's line-syntax switch has no default arm - a future mode constant added without an arm would build a command with no path. Latent, not a live bug.

also needs a rebase - it conflicts in model.go + editor_test.go now that #235 merged.

The branch is kept as a merge because replaying the stack over current trunk repeatedly conflicts in the editor and model tests added by recent trunk work. The merge keeps the source editor behavior and the mouse tracking restoration behavior together at the integration boundary.

Cross-file hunk navigation can select the next file before that file finishes loading. If a source editor opened for the previous file exits in that window, reloading the previous file can stale-drop the queued load. Compare source-editor completion with the selected tree file before reloading the displayed file. When they differ, clear the pending cross-file hunk jump and leave the queued file load in control. The regression test failed before this fix because the handler returned a reload command, bumped the file load sequence, and left the pending jump armed.

`sourceEditorTargetResult` is package-private state used only inside the UI package. Keeping its fields private matches the surrounding editor completion structs and avoids suggesting a package boundary that does not exist. The field comments remain because the result carries captured file, path, line, and reload state whose meanings are easier to audit when named explicitly.

`editorLineMode` is produced by private code in `app/editor`. If a future mode is added without a matching `SourceCommand` arm, continuing with a partially built command would hide an internal bug. Panic on unrecognized modes so the failure points at the missing switch arm instead of launching an editor command without the source path.

abhinav · Jun 23, 2026

Fixed

sourceEditorTargetResult is package-private but has exported fields

That was intentional but happy to unexport. Changed.

SourceCommand's line-syntax switch has no default arm

added

umputun

round 3 items all addressed. Cross-file reload race now anchors the guard on the tree selection and clears the pending hunk jump, with a regression test covering the window. Unexport and default-arm nits done, rebase is clean.

lgtm, thx

Copilot AI review requested due to automatic review settings June 20, 2026 14:18

abhinav requested a review from umputun as a code owner June 20, 2026 14:18

Copilot started reviewing on behalf of abhinav June 20, 2026 14:19 View session

Copilot AI reviewed Jun 20, 2026

View reviewed changes

Comment thread app/ui/editor.go

umputun requested a review from Copilot June 20, 2026 17:31

Copilot started reviewing on behalf of umputun June 20, 2026 17:32 View session

Copilot AI reviewed Jun 20, 2026

View reviewed changes

umputun requested changes Jun 20, 2026

View reviewed changes

golangci-lint fixes

7c4667a

abhinav added 5 commits June 20, 2026 11:50

umputun reviewed Jun 21, 2026

View reviewed changes

abhinav added 2 commits June 21, 2026 15:43

Document source editor path trust

469e6a1

umputun requested changes Jun 22, 2026

View reviewed changes

abhinav added 2 commits June 22, 2026 18:25

abhinav added 2 commits June 22, 2026 19:07

umputun approved these changes Jun 23, 2026

View reviewed changes

umputun merged commit 43df140 into umputun:master Jun 23, 2026
2 checks passed

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

abhinav commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

abhinav commented Jun 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

abhinav commented Jun 20, 2026

Uh oh!

abhinav commented Jun 20, 2026

Uh oh!

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

abhinav commented Jun 21, 2026

Uh oh!

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

abhinav commented Jun 22, 2026

Uh oh!

abhinav commented Jun 22, 2026

Uh oh!

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

abhinav commented Jun 23, 2026

Uh oh!

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abhinav commented Jun 20, 2026 •

edited

Loading