Skip to content

feat: add session user review notes#281

Merged
benvinegar merged 22 commits into
mainfrom
feat/user-review-notes
May 16, 2026
Merged

feat: add session user review notes#281
benvinegar merged 22 commits into
mainfrom
feat/user-review-notes

Conversation

@benvinegar
Copy link
Copy Markdown
Member

@benvinegar benvinegar commented May 10, 2026

Summary

  • Adds session-persistent, human-authored inline review notes that render in the main diff stream alongside AI/agent notes.
  • Adds an inline draft composer with save/cancel actions, focus-aware shortcuts, soft-wrap growth, blur behavior that preserves drafts, and polished note card labels/layout.
  • Adds a hover-only add-note affordance ([+]) for diff rows, including idle/blur hiding and c shortcut routing to the active hovered line when present.
  • Unifies note/comment readback under hunk session comment list --type <live|all|ai|agent|user> while keeping hunk session review --include-notes for full review exports.
  • Plumbs review-note projections through the session broker without duplicate session reads and keeps review-note counts consistent with the broker snapshot.
  • Stabilizes rebased test coverage around missing jj, session comment listing, note projections, draft notes, and inline-note rendering.

Verification

  • bun run format:check
  • bun run lint
  • bun run typecheck
  • bun test
  • Targeted tmux/manual verification for inline add-note hover behavior and draft note save/readback.

Updated by Pi using GPT-5 (2026-05-16)

@benvinegar benvinegar force-pushed the feat/user-review-notes branch 8 times, most recently from 4fb6c4b to c29c12a Compare May 11, 2026 04:35
@benvinegar benvinegar marked this pull request as ready for review May 16, 2026 13:49
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 16, 2026

Greptile Summary

This PR adds session-persistent, human-authored inline review notes: users can draft, save, edit, and remove notes directly in the diff stream, and those notes are broadcast through the daemon snapshot so agents can read them via hunk session comment list --type user|all|ai|agent.

  • UI layer (useReviewController, AgentInlineNote, DiffPane): full note lifecycle (start/save/cancel/edit/remove draft), merged into the annotation display alongside live agent comments, and serialised into reviewNoteSummaries for snapshot broadcast.
  • Session protocol (protocol.ts, brokerServer.ts, cli.ts): adds note-list, note-get, and note-rm server actions along with the required request/response shapes and HunkSessionCliClient method implementations.
  • CLI surface (cli.ts, commands.ts): note listing is accessible via comment list --type user|all|ai|agent; note-get and note-rm are fully implemented server-side but have no corresponding CLI subcommands or runSessionCommand routing, leaving those two endpoints unreachable from the command line.

Confidence Score: 3/5

The UI-side note creation and snapshot broadcast work end-to-end, but two of the three advertised agent-facing CLI commands (note get and note rm) are entirely missing from the CLI parser and command router, making those server endpoints dead code today.

The review note lifecycle is solid on the UI and daemon sides. However, hunk session note get and hunk session note rm — mentioned explicitly in the PR description as agent-facing entry points — have no CLI subcommand parser, no runSessionCommand case, and no entry in SessionCommandInput. Agents who attempt either command will hit an "Unknown session command" error. The note-get/note-rm code in HunkSessionCliClient and brokerServer.ts is fully unreachable from the command line. The double state.getSession call in the review handler is a secondary consistency concern.

src/core/cli.ts and src/session/commands.ts are missing the note subcommand branch and its routing; src/core/types.ts has note command input types that are excluded from the SessionCommandInput union.

Important Files Changed

Filename Overview
src/core/cli.ts Adds --type filtering to comment list and --include-notes to review; missing note subcommand parser for note-get and note-rm actions that are fully implemented server-side
src/core/types.ts Adds ReviewNoteSource, ReviewNote, SessionCommentListType, and three SessionNoteXxxCommandInput types; note types are not included in SessionCommandInput union making them dead from the command router's perspective
src/session-broker/brokerServer.ts Adds note-list, note-get, and note-rm server actions; the review case with includeNotes performs a second getSession call to patch notes instead of reusing buildHunkSessionReview's native includeNotes support
src/session/commands.ts Adds note-list action routing through the comment-list case when --type is not live; correctly guards with daemon capability check; no cases for note-get or note-rm
src/ui/hooks/useReviewController.ts Adds full user-note lifecycle: startUserNote, saveDraftNote, cancelDraftNote, editUserNote, removeUserNote; serializes both agent and user notes into reviewNoteSummaries for snapshot broadcast
src/hunk-session/projections.ts Adds listHunkSessionNotes and getHunkSessionNote helpers; buildHunkSessionReview gains includeNotes option that correctly reads from snapshot state
src/hunk-session/types.ts Adds SessionReviewNoteSummary, RemoveUserNoteToolInput, RemovedUserNoteResult, and supporting fields; clean shape that mirrors ReviewNote from core
src/ui/hooks/useHunkSessionBridge.ts Pipes reviewNoteCount and reviewNoteSummaries into the daemon snapshot; wires removeUserNote into the bridge for daemon-driven note removal
src/hunk-session/cli.ts Adds listNotes, getNote, removeNote methods to HttpHunkSessionCliClient and corresponding format helpers; optional methods correctly typed
src/session/protocol.ts Adds note-list, note-get, note-rm to SessionDaemonAction and corresponding request/response shapes; well-structured additions consistent with existing comment protocol

Sequence Diagram

sequenceDiagram
    participant User as User (keyboard)
    participant UI as useReviewController
    participant Bridge as useHunkSessionBridge
    participant Daemon as brokerServer (HTTP)
    participant CLI as hunk session CLI

    User->>UI: press `c` → startUserNote()
    User->>UI: type body → updateDraftNote()
    User->>UI: Ctrl+S → saveDraftNote()
    UI->>UI: store in userNotesByFileId
    UI->>UI: recompute reviewNoteSummaries
    UI->>Bridge: reviewNoteCount + reviewNoteSummaries
    Bridge->>Daemon: updateSnapshot(state with reviewNotes)

    CLI->>Daemon: "POST /session-api {action:"note-list"}"
    Daemon->>Daemon: listHunkSessionNotes(session, filter)
    Daemon-->>CLI: "{notes:[...]}"

    Note over CLI,Daemon: note-get and note-rm handled server-side but no CLI subcommand routes to them yet

    CLI->>Daemon: "POST /session-api {action:"note-rm"}"
    Daemon->>UI: dispatchCommand remove_user_note
    UI->>UI: removeUserNote(noteId)
    UI-->>Daemon: RemovedUserNoteResult
Loading

Comments Outside Diff (2)

  1. src/core/cli.ts, line 1183 (link)

    P1 note get and note rm CLI subcommands never implemented

    The broker server defines and handles note-get and note-rm actions, HunkSessionCliClient implements getNote and removeNote, and SessionNoteGetCommandInput / SessionNoteRemoveCommandInput types exist in core/types.ts — but parseSessionCommand ends with throw new Error(\Unknown session command: ${subcommand}`)with nonotebranch.runSessionCommandlikewise has no cases for those actions. Any call tohunk session note get orhunk session note rm will throw at the CLI parser. The PR description's claim that notes are exposed throughhunk session note list/get/rmis only true for thelistpath (tunnelled viacomment list --type); getandrm` are completely unreachable from the CLI.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/core/cli.ts
    Line: 1183
    
    Comment:
    **`note get` and `note rm` CLI subcommands never implemented**
    
    The broker server defines and handles `note-get` and `note-rm` actions, `HunkSessionCliClient` implements `getNote` and `removeNote`, and `SessionNoteGetCommandInput` / `SessionNoteRemoveCommandInput` types exist in `core/types.ts` — but `parseSessionCommand` ends with `throw new Error(\`Unknown session command: ${subcommand}\`)` with no `note` branch. `runSessionCommand` likewise has no cases for those actions. Any call to `hunk session note get <id>` or `hunk session note rm <id>` will throw at the CLI parser. The PR description's claim that notes are exposed through `hunk session note list/get/rm` is only true for the `list` path (tunnelled via `comment list --type`); `get` and `rm` are completely unreachable from the CLI.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. src/core/types.ts, line 226-261 (link)

    P2 SessionNoteGetCommandInput and SessionNoteRemoveCommandInput not included in SessionCommandInput

    SessionNoteListCommandInput, SessionNoteGetCommandInput, and SessionNoteRemoveCommandInput are defined (lines 226–249) but the SessionCommandInput union (lines 251–261) does not include them. Since REQUIRED_ACTION_BY_COMMAND in commands.ts is keyed by SessionCommandInput["action"], and runSessionCommand is typed to accept SessionCommandInput, these note action types are entirely outside the command routing layer. This is consistent with the fact that no CLI subcommands exist for them yet, but leaves dead type definitions that will confuse future contributors expecting them to be routable.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/core/types.ts
    Line: 226-261
    
    Comment:
    **`SessionNoteGetCommandInput` and `SessionNoteRemoveCommandInput` not included in `SessionCommandInput`**
    
    `SessionNoteListCommandInput`, `SessionNoteGetCommandInput`, and `SessionNoteRemoveCommandInput` are defined (lines 226–249) but the `SessionCommandInput` union (lines 251–261) does not include them. Since `REQUIRED_ACTION_BY_COMMAND` in `commands.ts` is keyed by `SessionCommandInput["action"]`, and `runSessionCommand` is typed to accept `SessionCommandInput`, these note action types are entirely outside the command routing layer. This is consistent with the fact that no CLI subcommands exist for them yet, but leaves dead type definitions that will confuse future contributors expecting them to be routable.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/core/cli.ts:1183
**`note get` and `note rm` CLI subcommands never implemented**

The broker server defines and handles `note-get` and `note-rm` actions, `HunkSessionCliClient` implements `getNote` and `removeNote`, and `SessionNoteGetCommandInput` / `SessionNoteRemoveCommandInput` types exist in `core/types.ts` — but `parseSessionCommand` ends with `throw new Error(\`Unknown session command: ${subcommand}\`)` with no `note` branch. `runSessionCommand` likewise has no cases for those actions. Any call to `hunk session note get <id>` or `hunk session note rm <id>` will throw at the CLI parser. The PR description's claim that notes are exposed through `hunk session note list/get/rm` is only true for the `list` path (tunnelled via `comment list --type`); `get` and `rm` are completely unreachable from the CLI.

### Issue 2 of 3
src/session-broker/brokerServer.ts:123-131
**Double `getSession` call when `includeNotes` is true**

`state.getSessionReview` already uses `buildHunkSessionReview` which accepts an `includeNotes` option. But because the generic `SessionBrokerState` adapter doesn't forward that option, the code fetches the review first and then calls `state.getSession(input.selector)` a second time to manually patch `reviewNotes` and `reviewNoteCount`. If the snapshot updates between the two calls (concurrent snapshot write from the UI), `reviewNoteCount` in the returned `review` will reflect the second snapshot while the rest of the review fields reflect the first. Additionally, the patch unconditionally uses `reviewNotes.length` for `reviewNoteCount`, bypassing the `reviewNoteCount ?? reviewNotes.length` fallback logic in `buildHunkSessionReview`. Consider plumbing `includeNotes` through `getSessionReview` in the broker adapter, or performing a single `getSession` call and calling `buildHunkSessionReview` directly here.

### Issue 3 of 3
src/core/types.ts:226-261
**`SessionNoteGetCommandInput` and `SessionNoteRemoveCommandInput` not included in `SessionCommandInput`**

`SessionNoteListCommandInput`, `SessionNoteGetCommandInput`, and `SessionNoteRemoveCommandInput` are defined (lines 226–249) but the `SessionCommandInput` union (lines 251–261) does not include them. Since `REQUIRED_ACTION_BY_COMMAND` in `commands.ts` is keyed by `SessionCommandInput["action"]`, and `runSessionCommand` is typed to accept `SessionCommandInput`, these note action types are entirely outside the command routing layer. This is consistent with the fact that no CLI subcommands exist for them yet, but leaves dead type definitions that will confuse future contributors expecting them to be routable.

Reviews (1): Last reviewed commit: "fix(ui): restore shortcuts after note bl..." | Re-trigger Greptile

Comment on lines +123 to 131
case "review": {
const review = state.getSessionReview(input.selector, { includePatch: input.includePatch });
if (input.includeNotes) {
const session = state.getSession(input.selector);
review.reviewNotes = session.snapshot.state.reviewNotes ?? [];
review.reviewNoteCount = review.reviewNotes.length;
}
response = { review };
break;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Double getSession call when includeNotes is true

state.getSessionReview already uses buildHunkSessionReview which accepts an includeNotes option. But because the generic SessionBrokerState adapter doesn't forward that option, the code fetches the review first and then calls state.getSession(input.selector) a second time to manually patch reviewNotes and reviewNoteCount. If the snapshot updates between the two calls (concurrent snapshot write from the UI), reviewNoteCount in the returned review will reflect the second snapshot while the rest of the review fields reflect the first. Additionally, the patch unconditionally uses reviewNotes.length for reviewNoteCount, bypassing the reviewNoteCount ?? reviewNotes.length fallback logic in buildHunkSessionReview. Consider plumbing includeNotes through getSessionReview in the broker adapter, or performing a single getSession call and calling buildHunkSessionReview directly here.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session-broker/brokerServer.ts
Line: 123-131

Comment:
**Double `getSession` call when `includeNotes` is true**

`state.getSessionReview` already uses `buildHunkSessionReview` which accepts an `includeNotes` option. But because the generic `SessionBrokerState` adapter doesn't forward that option, the code fetches the review first and then calls `state.getSession(input.selector)` a second time to manually patch `reviewNotes` and `reviewNoteCount`. If the snapshot updates between the two calls (concurrent snapshot write from the UI), `reviewNoteCount` in the returned `review` will reflect the second snapshot while the rest of the review fields reflect the first. Additionally, the patch unconditionally uses `reviewNotes.length` for `reviewNoteCount`, bypassing the `reviewNoteCount ?? reviewNotes.length` fallback logic in `buildHunkSessionReview`. Consider plumbing `includeNotes` through `getSessionReview` in the broker adapter, or performing a single `getSession` call and calling `buildHunkSessionReview` directly here.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fe87e05: the review endpoint now forwards includeNotes into SessionBrokerState/buildSessionReview, so review notes and counts come from the same broker snapshot and preserve the projection fallback logic. I also routed hunk session comment list --type ... through the existing comment-list API instead of the note-list adapter so typed comment readback stays unified.

Responded by pi-coding-agent using GPT-5.

@benvinegar benvinegar force-pushed the feat/user-review-notes branch from dab8cb9 to fe87e05 Compare May 16, 2026 14:59
@benvinegar benvinegar changed the title [draft] feat: add session user review notes eat: add session user review notes May 16, 2026
@benvinegar benvinegar changed the title eat: add session user review notes feat: add session user review notes May 16, 2026
@benvinegar benvinegar merged commit 2b7e82a into main May 16, 2026
5 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.

1 participant