feat(ui): Ctrl+E editor handoff for multi-line annotations#115
feat(ui): Ctrl+E editor handoff for multi-line annotations#115
Conversation
Plan for adding Ctrl+E / $EDITOR integration to annotation input so users can write multi-line annotations. Covers multi-line rendering, editor invocation via tea.ExecProcess, save-on-exit flow, output format compatibility, and documentation sync. Related to #111
Introduce app/editor package with ExternalEditor interface and temp-file lifecycle management for launching external editors.
Consolidated review fixes from multiple iterations: - address code review findings on annotation store, editor, and UI - address iter-2 review findings on annotate flow and docs - address codex review findings on editor and annotate - name receiver on annotationContinuationIndent - don't save seed content on editor launch failure - document edge case in handleEditorFinished seed comparison
Captures three principles enforced via course-correction this branch: decouple OS/external concerns from UI, minimize exported surface, consumer-side interfaces for external deps.
Deploying revdiff with
|
| Latest commit: |
b3fb9e0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eb39a82d.revdiff.pages.dev |
| Branch Preview URL: | https://external-editor-annotations.revdiff.pages.dev |
There was a problem hiding this comment.
Pull request overview
Adds an external-editor handoff to the annotation workflow so users can author and preserve multi-line annotations (including rendering, listing, and export), without replacing the existing single-line textinput fast path.
Changes:
- Introduces
Ctrl+Eduring annotation input to open$EDITOR(via newapp/editorpackage +ui.ExternalEditorinjection) and save multi-line content on editor exit. - Updates UI rendering/counting to be
\n-aware for inline annotations, and flattens newlines in the annotation list overlay. - Updates structured output to prevent multi-line bodies starting with
##from being misparsed as new records; syncs docs/tests accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| site/docs.html | Documents Ctrl+E editor handoff + ## escaping behavior in output format docs. |
| README.md | Mirrors the new Ctrl+E flow and output-format nuance in primary docs. |
| .claude-plugin/skills/revdiff/references/usage.md | Mirrors updated usage docs for the Claude plugin. |
| plugins/codex/skills/revdiff/references/usage.md | Mirrors updated usage docs for the Codex plugin. |
| docs/ARCHITECTURE.md | Adds app/editor to architecture + describes multi-line annotation flow and output escaping. |
| docs/plans/completed/20260416-external-editor-annotations.md | Captures the completed implementation plan and rationale. |
| app/editor/editor.go | New editor driver: resolves $EDITOR/$VISUAL, manages temp file lifecycle, returns *exec.Cmd + completion reader. |
| app/editor/editor_test.go | Unit tests for editor resolution/tokenization + tempfile/readResult behaviors. |
| app/ui/editor.go | Consumer-side ExternalEditor interface + openEditor and editorFinishedMsg routing. |
| app/ui/editor_test.go | UI-level tests driving openEditor via a fake editor implementation. |
| app/ui/model.go | Wires ExternalEditor into Model/ModelConfig, defaults to editor.Editor{}, handles editorFinishedMsg in Update. |
| app/ui/annotate.go | Adds Ctrl+E handling during annotation input; stashes existing multi-line annotations to avoid textinput sanitization; refactors save logic into saveComment. |
| app/ui/annotate_test.go | Extensive coverage for multi-line stashing/editing, editor-finished routing, and retry/cancel semantics. |
| app/ui/diffview.go | Splits/wraps annotations by logical lines (\n) and aligns continuation indentation. |
| app/ui/diffview_test.go | Tests multi-line wrapped annotation rendering + indent calculation. |
| app/ui/overlay/annotlist.go | Flattens \n to ⏎ for single-row display in the annotation list overlay. |
| app/ui/overlay/annotlist_test.go | Tests multi-line flattening, selected rendering, and truncation in overlay rows. |
| app/annotation/store.go | Escapes body lines that start with ## when formatting structured output. |
| app/annotation/store_test.go | Adds tests for multi-line output preservation and delimiter integrity. |
| app/ui/doc.go | Updates package doc to mention editor.go and ExternalEditor. |
| CLAUDE.md | Documents new app/editor package and tea.ExecProcess capture/cleanup gotchas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (m Model) renderWrappedAnnotation(b *strings.Builder, cursor, text string) { | ||
| wrapWidth := m.diffContentWidth() - 1 // 1 for cursor column | ||
| paneBg := m.resolver.Color(style.ColorKeyDiffPaneBg) | ||
|
|
||
| if wrapWidth > 10 && lipgloss.Width(text) > wrapWidth { | ||
| lines := m.wrapContent(text, wrapWidth) | ||
| for i, line := range lines { | ||
| c := " " // continuation lines get space instead of cursor | ||
| if i == 0 { | ||
| logical := strings.Split(text, "\n") | ||
| indent := m.annotationContinuationIndent(logical[0]) | ||
|
|
||
| first := true | ||
| for i, segment := range logical { | ||
| if i > 0 { | ||
| segment = indent + segment | ||
| } | ||
| var lines []string | ||
| if wrapWidth > 10 && lipgloss.Width(segment) > wrapWidth { | ||
| lines = m.wrapContent(segment, wrapWidth) | ||
| } else { | ||
| lines = []string{segment} | ||
| } | ||
| for _, line := range lines { | ||
| c := " " | ||
| if first { | ||
| c = cursor | ||
| first = false | ||
| } | ||
| b.WriteString(c + m.renderer.AnnotationInline(line) + "\n") | ||
| styled := c + m.renderer.AnnotationInline(line) | ||
| b.WriteString(m.extendLineBg(styled, paneBg) + "\n") | ||
| } |
There was a problem hiding this comment.
renderWrappedAnnotation pads annotation rows via extendLineBg, but extendLineBg subtracts gutterExtra (line numbers/blame). Since annotation rows don’t render the gutters separately, enabling gutters will make these rows under-pad and leave a strip of unstyled background on the right. Consider either (a) including the same gutter blanks in the annotation row before padding, or (b) using a padding helper that targets the full diff pane width for gutter-less rows.
There was a problem hiding this comment.
pushing back on this one. the same extendLineBg pattern is used by the pre-existing single-line annotation input at diffview.go:319, so if this actually under-padded with gutters enabled it'd have been visible on master long before this PR. either the gutter subtraction is intentional (annotation body renders past the gutter columns, which stay blank) or it's a cosmetic tradeoff that has been in place for a while. not specific to multi-line annotations.
addressed the other three findings in b3fb9e0:
tokenize: narrowed backslash escape to POSIX shell-meta chars only soC:\foosurvivesNewModel: addedisNilValueguard so a typed-nilExternalEditorfalls back toeditor.Editor{}ARCHITECTURE.md: fixed the##vs##mismatch
happy to revisit the gutter padding as a separate issue if it turns out to be visibly broken.
| ed := cfg.Editor | ||
| if ed == nil { | ||
| ed = editor.Editor{} | ||
| } |
There was a problem hiding this comment.
NewModel only checks cfg.Editor == nil before defaulting to editor.Editor{}. If a caller passes a typed-nil ExternalEditor (e.g. (*myEditor)(nil)), the interface will be non-nil and later calls to m.editor.Command will panic. Consider adding a typed-nil guard (similar to the typed-nil TOC collapse pattern) so “nil means default editor” is robust.
| case r == '\\' && i+1 < len(runes): | ||
| cur.WriteRune(runes[i+1]) | ||
| hasToken = true | ||
| i++ | ||
| case r == ' ' || r == '\t' || r == '\n': |
There was a problem hiding this comment.
Editor.tokenize treats backslash as an escape outside quotes (it drops the backslash and keeps the next rune). On Windows this will corrupt common executable paths like C:\Program Files\... (backslashes are path separators, not escapes), making $EDITOR unusable. Consider making backslash-escape handling OS-aware (disable on windows), or only treating backslash as an escape when it precedes whitespace/quotes (POSIX-style) rather than unconditionally.
| ### app/annotation/ — annotation store | ||
|
|
||
| In-memory store for annotations. Each `Annotation` has file, line, text, and optional `EndLine` for hunk range headers (triggered when comment contains "hunk" keyword). Structured output formatting for export. | ||
| In-memory store for annotations. Each `Annotation` has file, line, text, and optional `EndLine` for hunk range headers (triggered when comment contains "hunk" keyword). Structured output formatting for export. `FormatOutput` escapes body lines that start with `##` by prefixing a single space so downstream parsers cannot confuse a comment line for a new record header. |
There was a problem hiding this comment.
ARCHITECTURE.md says FormatOutput escapes body lines starting with ## (no space), but the implementation only escapes the record-header form ## (with a trailing space). To avoid misleading readers, update this to ## (and/or explicitly call out that ##foo and ### are not escaped).
| In-memory store for annotations. Each `Annotation` has file, line, text, and optional `EndLine` for hunk range headers (triggered when comment contains "hunk" keyword). Structured output formatting for export. `FormatOutput` escapes body lines that start with `##` by prefixing a single space so downstream parsers cannot confuse a comment line for a new record header. | |
| In-memory store for annotations. Each `Annotation` has file, line, text, and optional `EndLine` for hunk range headers (triggered when comment contains "hunk" keyword). Structured output formatting for export. `FormatOutput` escapes body lines that start with `## ` by prefixing a single space so downstream parsers cannot confuse a comment line for a new record header. Lines starting with `##foo` or `###` are not escaped. |
- tokenize: narrow backslash escape to shell-meta chars only (POSIX-style);
preserves Windows-style `C:\foo` paths with backslash intact
- NewModel: guard against typed-nil ExternalEditor via isNilValue helper so
`(*T)(nil)` defaults to editor.Editor{} instead of panicking on Command
- ARCHITECTURE.md: correct `##` escape description to `## ` (with space,
matching the narrowed check landed earlier)
Adds
Ctrl+Ewhile annotating to hand off the current text to$EDITOR, so users can write multi-line annotations (numbered lists, pasted data slices, longer explanations) without swapping the single-linetextinputwidget for a full textarea.Related to #111. The 500→8000
CharLimitbump addressed length; this change addresses structure.How it works
a/A/Enterstill start the single-line input for quick notesCtrl+Ecaptures the current input value, writes it to a.mdtemp file, suspends bubbletea viatea.ExecProcess, opens the editor, and on exit saves the content directly (bypassing the single-line widget so newlines survive)$EDITOR→$VISUAL→vi. Quoted paths/args respected (e.g.EDITOR="/Applications/My Editor.app/..." --wait)cancelAnnotation(preserves any pre-existing annotation on that line — accidentally quitting vim doesn't wipe your note)renderWrappedAnnotationsplits on\n, aligns continuation lines to the emoji prefix width, wraps each logical line independently@) flattens\nto⏎so rows stay single-lineFormatOutputemits newlines verbatim; body lines starting with##get a single leading space so parsers splitting on##record headers don't misparse multi-line bodies as new recordsNew package
app/editor/owns the editor spawn/tempfile lifecycle; statelessEditorstruct with one public methodCommand(content)returning(*exec.Cmd, complete func(error) (string, error), error)app/ui/definesExternalEditorinterface (consumer-side, injected viaModelConfig.Editor, defaults toeditor.Editor{}). Tests injectfakeExternalEditorstubTarget capture
openEditorfreezesfileName,fileLevel,line,changeType, and the seed content at spawn time. Since bubbletea is suspended duringExecProcess, the user can't navigate during an editor session, but the captures still protect against any queued state mutations resuming after editor exit.Error handling
tea.ExecProcesscollapses launch-time failures,cmd.Runerrors, andRestoreTerminalfailures into one callback. handler distinguishes by comparing content vs seed:One documented edge case: if user saves content identical to seed and
RestoreTerminalfails, the save is dropped (false negative). Comment inhandleEditorFinishedexplains the tradeoff.Multi-line re-edit
Re-annotating a saved multi-line comment would flatten through
textinput.SetValue's sanitizer. Mitigation: detect\nin the pre-existing comment, stash inannot.existingMultilineinstead of SetValue, show placeholder[existing multi-line — Ctrl+E to edit].Ctrl+Eseeds from the stash; Enter on empty input preserves existing unchanged; typing a new value explicitly overwrites.Docs
site/docs.html, pluginusage.mdmirrors (.claude-plugin+plugins/codex) all syncedCLAUDE.md: newapp/editor/entry + gotcha ontea.ExecProcessstate capturedocs/ARCHITECTURE.md: package block, file table row, interface entry, annotation flow diagram updatedapp/ui/doc.go: editor.go bullet +ExternalEditorlistedKnown deferred
strings.TrimRightonly strips\n, leaves stray\r. Cosmetic on macOS-primary tool