Conversation
Greptile SummaryThis PR adds Key changes:
Confidence Score: 5/5Safe to merge — the only finding is a P2 edge case in the "prefer new" heuristic inside firstCommentTargetForHunk. All changes are well-scoped: protocol types, CLI parsing, server-side validation, and UI bridge are updated consistently. The 1-based→0-based index conversion is correctly applied and tested at the server boundary. The only concern is that firstCommentTargetForHunk's "prefer new" bias can silently fall back to "old" when pure-deletion chunks appear before pure-addition chunks in hunkContent, but this does not break functionality — it only affects which anchor line is chosen. All remaining findings are P2. src/core/liveComments.ts — specifically the firstCommentTargetForHunk loop logic Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant CLI as hunk session CLI
participant Server as MCP Daemon (server.ts)
participant Bridge as useHunkSessionBridge
participant Resolver as resolveCommentTarget
Agent->>CLI: comment add --hunk 2 --file README.md --summary "..."
CLI->>CLI: parse --hunk 2 → hunkNumber: 2
CLI->>Server: POST /session-api {action:"comment-add", hunkNumber:2}
Server->>Server: validate (hunkNumber OR side+line present)
Server->>Server: convert hunkNumber 2 → hunkIndex 1
Server->>Bridge: sendComment({hunkIndex:1, filePath, summary})
Bridge->>Resolver: resolveCommentTarget(file, {hunkIndex:1})
Resolver->>Resolver: file.metadata.hunks[1] → firstCommentTargetForHunk(hunk)
Resolver-->>Bridge: {hunkIndex:1, side:"new", line:13}
Bridge->>Bridge: buildLiveComment({side:"new", line:13}, ...)
Bridge-->>Server: AppliedCommentResult {hunkIndex:1, side:"new", line:13}
Server-->>Agent: 200 {result: {commentId, hunkIndex:1, side:"new", line:13}}
Reviews (1): Last reviewed commit: "Support hunk-targeted live comments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function firstCommentTargetForHunk(hunk: Hunk): Omit<ResolvedCommentTarget, "hunkIndex"> { | ||
| let deletionLineNumber = hunk.deletionStart; | ||
| let additionLineNumber = hunk.additionStart; | ||
|
|
||
| for (const content of hunk.hunkContent) { | ||
| if (content.type === "context") { | ||
| deletionLineNumber += content.lines; | ||
| additionLineNumber += content.lines; | ||
| continue; | ||
| } | ||
|
|
||
| if (content.additions > 0) { | ||
| return { | ||
| side: "new", | ||
| line: additionLineNumber, | ||
| }; | ||
| } | ||
|
|
||
| if (content.deletions > 0) { | ||
| return { | ||
| side: "old", | ||
| line: deletionLineNumber, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const fallbackRange = hunkLineRange(hunk); | ||
| return hunk.additionLines > 0 | ||
| ? { side: "new", line: fallbackRange.newRange[0] } | ||
| : { side: "old", line: fallbackRange.oldRange[0] }; | ||
| } |
There was a problem hiding this comment.
The function's stated goal is to prefer the new (addition) side when present, but that preference only applies when the first non-context chunk already contains additions. If hunkContent has a pure-deletion block first followed by a pure-addition block later, the loop returns "old" at the deletion block and never reaches the addition block.
// example hunkContent sequence that triggers the issue:
// { type: "change", deletions: 2, additions: 0 } ← loop returns "old" here
// { type: "change", deletions: 0, additions: 3 } ← never checked
The fallback below the loop correctly inspects hunk.additionLines > 0 to prefer "new", but that fallback is only reachable when all chunks have zero additions and zero deletions — essentially impossible for a real diff hunk, so it will never fire in this situation.
A simple fix is to do a two-pass scan: first sweep for the earliest addition to get the "new" preference, then fall back to the earliest deletion, before reaching the hunkLineRange fallback.
There was a problem hiding this comment.
Good catch. I changed firstCommentTargetForHunk() to keep scanning after deletion-only chunks, remember the first deletion as a fallback, and still prefer the earliest addition when one appears later in the same hunk. I also added a regression test for that mixed deletion/addition shape.
This comment was generated by Pi using OpenAI o3
1 participant
Summary
hunk session comment addto target a hunk directly with--hunk <n>Testing
bun run typecheckbun testPart of #158.
This PR description was generated by Pi using OpenAI o3