Skip to content

feat: include diff context in comment text output - #455

Merged
yoshiko-pg merged 3 commits into
yoshiko-pg:mainfrom
l0stplains:feat/comment-prompt-context
Aug 9, 2026
Merged

feat: include diff context in comment text output#455
yoshiko-pg merged 3 commits into
yoshiko-pg:mainfrom
l0stplains:feat/comment-prompt-context

Conversation

@l0stplains

@l0stplains l0stplains commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Closes #454

Summary

  • Mark comments on the old side with (old) in Copy Prompt, Copy All Prompt, the text API, CLI text output, and terminal shutdown output.
  • Add the requested and resolved diff selection once at the top of Copy All Prompt using Git range notation.
  • Keep new-side and legacy comment locations unchanged, preserving the common output format.

The human-readable output remains neutral so it can be used by people and tools without prescribing what to do with the comments.

The old-side marker is part of each comment location, so it appears in every human-readable output. The comparison header remains specific to Copy All Prompt, which is the aggregate export path with access to the displayed diff metadata. Adding it elsewhere would require broader metadata plumbing and output-contract changes, which are outside the scope of this PR.

Implementation notes

  • App passes the existing DiffResponse selection metadata through the Copy All Prompt path.
  • The shared comment formatter owns the location and prompt-context serialization, keeping copied prompts and other human-readable text output consistent.
  • Direct comparisons use ..; merge-base comparisons use ....
  • Resolved hashes appear as a parenthesized range only when they differ from the requested refs.
  • The comparison header is omitted when a meaningful Git range is unavailable (working, staged, ., and stdin-backed diffs).
  • New-side comments and legacy comments without side metadata retain the previous location format.
  • Stored comments and structured JSON output are unchanged; JSON already exposes the side through position.side.

Copy Prompt

Before

docs/SUMMARY.md:L85
Explain why this was removed.

After

docs/SUMMARY.md:L85 (old)
Explain why this was removed.

Copy All Prompt

Before

docs/SUMMARY.md:L85
Explain why this was removed.
=====
docs/SUMMARY.md:L242
Should this remain grouped with the related entries?

After

diff main...feature/docs-update (abcdef1...1234567)
=====
docs/SUMMARY.md:L85 (old)
Explain why this was removed.
=====
docs/SUMMARY.md:L242
Should this remain grouped with the related entries?

Direct comparisons use the same format with ... Working-tree and stdin-backed comparisons omit the header.

Human-readable comment output

This output is shared by /api/comments-output, difit comment get --format text, and terminal shutdown output.

Before

📝 Comments from review session:
==================================================
docs/SUMMARY.md:L85
Explain why this was removed.
==================================================
Total comments: 1

After

📝 Comments from review session:
==================================================
docs/SUMMARY.md:L85 (old)
Explain why this was removed.
==================================================
Total comments: 1

Test plan

  • pnpm check, pnpm format, and pnpm knip
  • pnpm test (856 passed, 2 skipped)
  • pnpm build
  • Manually verify Copy Prompt and Copy All Prompt for old/new comments and direct/merge-base/working-tree comparisons
  • Update /api/comments-output coverage to assert the old-side marker and unchanged new-side output

@yoshiko-pg

Copy link
Copy Markdown
Owner

Implementation and tests look great. One thing to confirm: since formatCommentThreadPrompt now appends side=, the change also affects formatCommentsOutput, i.e. the terminal output on Ctrl+C and /api/comments-output, not just the copy buttons. If that is intended it seems like a nice consistency win, but it is not mentioned in the PR description, so flagging in case any downstream tooling parses that output.

@l0stplains l0stplains changed the title feat: include diff context in copied comment prompts feat: include diff context in comment text output Aug 6, 2026
@l0stplains

Copy link
Copy Markdown
Contributor Author

Thanks for the review and for catching this!

You're right, I overlooked this broader effect when describing the PR.

The current implementation also appends side=old|new in the human-readable text endpoint, CLI text output, and terminal shutdown output because they use the same shared formatter. After checking those paths, I am keeping that behavior because they have the same old/new-side ambiguity as copied prompts.

The full comparison header remains specific to Copy All Prompt because it is the aggregate export path with access to the displayed diff metadata. Adding it elsewhere would require broader metadata plumbing and output-contract changes, which are outside the scope of this PR.

Structured JSON and stored comments remain unchanged.

I updated the PR description to make this scope explicit. The existing /api/comments-output coverage now asserts both side suffixes in f15a67c.

@yoshiko-pg

Copy link
Copy Markdown
Owner

After the review I kept thinking about the notation myself, and here is the format I would like to land on.

1. Side marker: only mark the old side, with (old)

side=new on every line is noisy given new is the common case, and key=value feels a bit too machine-flavored for this output. Marking only comments on deleted lines keeps the common case byte-identical to the current release:

docs/SUMMARY.md:L85 (old)
Explain why this was removed.
=====
docs/SUMMARY.md:L242
Should this remain grouped?

No marker for new-side and legacy comments. This applies to all the text outputs (copy prompts, Ctrl+C output, /api/comments-output).

2. Header: git range notation instead of key=value

diff main...feature/docs-update (abcdef1...1234567)
  • .. for direct comparison, ... for merge-base, so the mode= field disappears and it reads naturally to git users
  • resolved hashes go in the parens, omitted when identical to the requested refs
  • omit the header entirely for working-tree comparisons (working, staged, .), since it carries no useful information there

Sorry for the back and forth after my initial comments, and thanks again for the solid implementation and tests!


レビュー後に自分でも記法をどうするか考えていて、最終的にこの形に落ち着けたいです。

1. side は old 側だけ (old) を付ける

side=new が全行に付くのはノイズが多く、key=value 形式は出力として機械っぽすぎる気がしました。削除行へのコメントだけマークすれば、大多数のケースは現行リリースと完全に同じ出力になります。new 側と side 情報のない既存コメントは無印のままです。

2. ヘッダーは git のレンジ記法に

直接比較は ..、merge-base 比較は ... にすれば mode= フィールド自体が不要になり、git ユーザーにそのまま通じます。resolved ハッシュは括弧内に置き、requested と同じ場合は省略。working tree 比較(working / staged / .)では情報量がないのでヘッダー自体を省略したいです。

初回レビュー後の追加注文になってしまってすみません。実装とテストがしっかりしているので、この変更もスムーズに入ると思っています!

@l0stplains
l0stplains force-pushed the feat/comment-prompt-context branch from f15a67c to 9d12def Compare August 9, 2026 04:01
@l0stplains

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback.

I agree that this keeps the output less noisy and more natural. I was focused on adding the extra context and missed the readability aspect :)

I've adjusted the implementation and PR description accordingly.

@yoshiko-pg

Copy link
Copy Markdown
Owner

@l0stplains Thank you!!

寿司ゆき:happy

@yoshiko-pg
yoshiko-pg merged commit 7516687 into yoshiko-pg:main Aug 9, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include diff side and comparison context in copied comment prompts

2 participants