Review mode: draft comments on files, sent to agent sessions - #172
Merged
Conversation
Olga Lavrichenko (OLavrik)
requested review from
danyaberezun and
Rinat S (rsolmano)
as code owners
August 4, 2026 11:59
![jetbrains-air[bot]](https://avatars.githubusercontent.com/in/3314082?s=60&v=4){kind=small}
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
from
August 4, 2026 14:56
dd8c34d to
b0f6f1b
Compare
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
from
August 4, 2026 20:24
b0f6f1b to
4d0f66a
Compare
danyaberezun
marked this pull request as draft
August 4, 2026 22:39
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
3 times, most recently
from
August 6, 2026 11:31
514b467 to
2762f0e
Compare
Olga Lavrichenko (OLavrik)
marked this pull request as ready for review
August 6, 2026 14:12
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
from
August 6, 2026 16:24
2762f0e to
5589f31
Compare
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
2 times, most recently
from
August 6, 2026 19:57
630fa79 to
fb77e89
Compare
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
2 times, most recently
from
August 7, 2026 13:08
a01ad96 to
d73d362
Compare
…ance-aware glyphs, status-dependent row navigation, per-file Send review, self-announcing tab flags, and an explicit Done that finishes a fully-resolved file
…aft delete, anchoring drift, both diff sides, preview commenting, panel sections and navigation, and the file Done flow
…ew.changed pushes, a cold reload re-hydrates the review whole, and a file's Done is undone by a fresh remark
markCommentsSent runs synchronously before the detached prompt (so the session pin exists inside the review lock), so a pre-turn rejection (expired/missing key, bad model) stranded the comments as sent — the UI dropped their send/edit/delete actions and the review could never be delivered without recreating every remark. fireReviewPrompt now uses ackSend to tell accept from reject: on a pre-turn rejection it rolls the comments back to draft (new rollbackSend) and unpins a chat spun up solely for the failed send, while a fault after acceptance rides the event stream and leaves sent correct.
…er close can't resurrect the review The rollback fires detached, after the send's review lock is released, so a review.close or workspace archive can land first. ensureSnapshot would then write a fresh empty open review over the closed one. Read with load and no-op unless the review is still open — matching reanchorWorkspace and resolveCommentFromAgent, the other unlocked, fully-synchronous mutations.
- web: userText moves to the lib leaf, restoring the declared type-only store→chat edge (spec: apps/web/SPEC.md #62); lib SPEC gains it - web: panels import toast through the store barrel, not store/appStore - server: review.commentUpdate may only land resolved/dismissed from draft/sent — draft↔sent stays owned by markCommentsSent/rollbackSend, so a client can't un-send a comment whose id a chat already quotes - server: ensureSessionAttached scopes a LIVE session to the requested workspace (like getSessionMessages), so a review send can't route a package into another workspace's chat e2e reaches sent-hood via the persisted review file now that the wire shortcut is forbidden; unit test pins the new lifecycle guard.
…(ResizeObserver) Monaco keeps an off-viewport zone's node at display:none, so a card below the fold at setThreads time — exactly how the markdown tab's rendered→source switch mounts — measured 0, its zone stayed at the placeholder height, and the card painted OVER the following lines once scrolled in. The one-shot next-frame measure becomes a ResizeObserver on each card: the zone grows the moment the card gains real geometry, and follows later growth too. e2e reproduces the exact flow (drafts made in the preview, far below the source view's initial viewport, toggled to source and back) and pins that every measurable card's zone reserves at least its height.
… + files-level Send all The panel's FILE level carries the same drafts-only 'Send review (N)' as the pane toolbars (review-panel-send; path null covers the whole-change-set bucket), and the FILES level grows a 'Send all (N)' across every file (review-send-all, over the new allDraftIds; the send passes no ids — the host's all-drafts IS the batch, so the count can't race a concurrent edit). Both ride the shared reviewSend batch path and the shared drafts-only gate (nothing sendable → no button). fileDraftIds now accepts the null bucket key. e2e pins the counts at both levels and the gate's absence case; unit tests pin the draft-id derivations.
…+»'s twin The floating «+» stays; Monaco surfaces also get the same action in the right-click context menu (after Copy, Cmd/Ctrl+Shift+M, editorHasSelection precondition) — one action pair into one composer. The action id is suffixed with the editor's own id: addAction registers a GLOBAL command, and a diff attaches the flow to both inner editors, so a shared id would route one editor's menu click to the other's (empty) selection. attachReviewCommenting now takes IStandaloneCodeEditor (addAction lives only there; every caller holds one). The rendered preview's context menu is the browser's own and stays unextended. e2e drives the menu path end-to-end (retrying the gesture: Monaco arms the menu's mouseup listener ~100ms after open, and a live diff re-read can remount the editor under it).
Monaco's standalone menu is label-only (action.class icons are a workbench feature addAction can't reach), so monacoMenuIcons.ts decorates the OPEN menu in place: every row gets a fixed-width .tr-menu-icon slot (labels stay aligned) and known English labels get the same lucide glyph the rest of the UI uses (Comment on selection, Copy, Go to Definition/References/Symbol, Peek, Command Palette; Cut/Paste ride along for future editable surfaces). The menu mounts inside an OPEN shadow root (.shadow-root-host) that page stylesheets can't reach, so the slot's one rule (currentColor — no colors of its own) is injected into whichever root the menu renders in. Installed on every Monaco surface, review or not; unmapped rows stay label-only, so a Monaco bump can only lose icons, never break the menu. Submenu popups stay undecorated. e2e asserts the glyphs on the Comment/Copy rows in the live menu.
…omments in place and opens the file Replaces the two-level files/file navigation: each file row toggles an in-place section with the file's comments (In progress / Drafts / Resolved, same plan atoms); unfolding also opens the file's own surface, folding navigates nowhere. The active reviewed tab's section auto-unfolds (seeded at mount too — RightPanel often mounts the panel ON such an activation) and an expansion never auto-collapses: folding is the user's gesture alone, so a send opening its chat tab can't fold the section being read. The per-file Send review (N) and the Done finisher move into the unfolded section's strip; Send all stays in the panel header. The back arrow and the pinned file-level screen are gone. Also renames the Monaco menu icon slot class to editor-menu-icon — the tr-* namespace belongs to generated typography classes (typographyUsage gate).
…e section strip A fully-resolved file's row grows the check glyph right after its counts — visible folded or unfolded; the strip below held one lone glyph and read as stray space. The row becomes a flex pair (toggle button + Done button) so the action never nests inside the toggle; the section strip now exists only while drafts do (for Send review (N)).
…t body edits ReviewThreadCard initialized its textarea from the snapshot once, so a review.changed push carrying another client's body edit never reached an already-mounted card — stale text, and the next blur would overwrite the newer body. The field now reconciles on every push: a non-dirty field adopts the new body (render-time adjustment keyed on the last synced body); an unsaved local edit in flight is kept — the user's newest intent, saved as the usual last-writer update. The textarea re-measures on programmatic body changes too (a push never fires its change handler). e2e extends the preview test: a wire body edit lands in the open card.
…→ comment + fragment 'Sent 3 review comments' answered nothing about WHAT was sent. The package user message now renders as a foldable card: collapsed to the one-line summary, unfolding to a per-file list, each file to its comments — the remark's text, its line ref, and the quoted <fragment> verbatim (monospace, height-capped). Everything is parsed from the message itself — never the review snapshot, which the next review replaces — so any transcript answers 'what was sent' forever, on any client; parseReviewPackage grows the item/ group halves (pinned against packageRender's verbatim output) and both fold levels ride the shared fold cache, surviving virtualization. New @agent spec drives the real flow: Send now → collapsed card → unfold asserts text + fragment — then closes the chat and reopens it FROM DISK and unfolds again (the toggles retry as blocks: hydration mints fresh row ids and can remount a node mid-click).
…h comment folds on its own Per review of the first cut: the summary line no longer hides the files — 'Sent N review comments' sits above always-visible file rows; a file unfolds to one-line comment rows (▸ L2 · the remark…), and each comment unfolds to its full text + quoted fragment. Three testable levels, all riding the shared fold cache. The @agent spec pins the journey and that a reopen from history keeps both the card (re-rendered from the transcript message) and the user's unfolds (same runtime → same row ids → fold cache holds).
Summary line leads with the send glyph (the same lucide Send every send affordance wears) in default text; file rows get the FileText glyph, and both toggle levels take the shared row treatment (radius, px-xs, hover:bg-control-bg-hovered, transition) matching the Review panel's rows; the fragment block gains the muted border and deeper indent so the three levels read as one nested structure. Tokens only — no raw colors.
…e floating icon goes fully imperative Two things behind 'selecting a piece of a bullet highlights everything': - .review-region painted the whole target block with the hover surface, so picking three words washed the entire bullet wall-to-wall — reading as a broken text selection. It is now a thin left rail (the gutter-rail half of Monaco's decoration; inset shadow, zero layout shift): the block is marked without impersonating the selection. e2e pins the rail-not-wash contract (fails against the old CSS) plus that a dragged piece stays exactly a piece. - The icon-follows-selection machinery kept its position in React state; a mid-drag state flip re-renders the per-render-typed markdown tree and can replace the text nodes under the LIVE selection (Chrome then 'restores' it by flooding whole blocks). Position/visibility are now imperative DOM on an always-mounted body portal (custom properties + data-visible), so a drag never triggers a render at all.
… file level is dropped A send is one message per file (review.sendBatch groups by file and fires each group as its own message), so the card's file row always held exactly one entry naming the file the summary already names. The comment rows now sit right under the summary; each still unfolds to its full text + quoted fragment. groupPackageItems/ReviewPackageFile leave with their only consumer; the @agent spec asserts the flat shape.
…ed doc comment
Re-reading the whole series as a reviewer: chat/SPEC.md still described the
dropped file-level fold ('both fold levels'); ReviewThreadCard's grow()
helper had landed between the card's doc comment and the component,
hijacking it; SendReviewButton/reviewModel docs still said 'FILE level'
after the accordion; a stray blank line split the turns.tsx import block.
No behavior changes.
…reviews dir Two layers, per review: the handler tears down (spec cache, review file, watcher, PTYs, worktree reclaim) only after forgetWorkspace actually found the workspace, keyed by the canonical ws.id — an unknown wire id is an idempotent ack that touches nothing. And the review persistence helper refuses ids with path segments outright (/^[\w-]+$/ — real ids are UUIDs): the id becomes a filename, so a wire-supplied '../config' would otherwise resolve every read/write/unlink in the module onto the data dir's own files. Unit test pins the refusal across removeWorkspaceReviews and the read path.
Olga Lavrichenko (OLavrik)
force-pushed
the
olga.lavrichenko/review
branch
from
August 10, 2026 12:19
bf0a8b8 to
2f38497
Compare
…draft survives a sibling push setThreads tore down and recreated every Monaco view zone on each review snapshot, so a review.changed push (another client's comment, a re-anchor/resolve elsewhere) replaced the textarea a user was editing with the last-persisted body — silently dropping the unsaved edit. Reconcile by comment id instead: a card whose rendered content is unchanged (status/anchorState/line-range/body signature) keeps its exact DOM, so a draft mid-edit survives with its textarea value, focus and selection; only changed cards rebuild, gone ones drop, new ones add.
…e anchor's own baseRef openDiffInTab re-resolves a branch/uncommitted scope against the current fork point/HEAD, so after the worktree committed or the review target was re-pointed, sidebar navigation to a base-side comment opened a different original blob and mounted the card at stale line numbers. Add GitDiffScope.kind: "pinned" (worktree vs one immutable commit; wire v30), resolved server-side like a commit scope but with the worktree as the modified side. commentSurface now reopens a base-side anchor through a pinned scope on its own baseRef (captured scope stays the fallback for comments saved before baseRef was stamped), so navigation always shows the exact blob the remark quotes. Covers changesModel scope handling, the server resolver + its test, an e2e original-side check, and the specs.
danyaberezun
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #171
Adds a GitHub-style review flow on top of the worktree: select code in any file
surface — Monaco editor, either diff side, or the rendered markdown preview — and
leave a comment. Drafts are server-persisted per workspace, re-anchored on file
changes (line-range + text-quote fallback), and rendered as editable in-flow cards
right in the document.
Comments are sent to the agent: a single comment opens its own chat, a batch opens
one review chat — the agent receives a structured context package with stable
comment ids and resolves items via a
resolve_commenttool (manual override andgit.revertFilereject stay available). A per-file Review panel (TODO-plan styling:In progress / Drafts / Resolved, glance-aware glyphs) tracks the flow; reviewed
files get a tab flag and a pane-toolbar "Send review (N)" button.
Wire:
review.*methods +review.changedpush,git.revertFile(protocol v23).Verified: full e2e 156/156 (incl. new
review.spec.ts), unit, lint, typecheck;migrated to the new semantic color tokens (#161).