Repository navigation
feat(review): draw GitHub PR comments inline in the code - #49
Merged
Merged
Conversation
The Checks pane listed a release request's review threads, but reading one and acting on it meant leaving Orca: opening the file showed no trace of the comment, and replying or resolving meant a trip to github.com. Review threads now render on their commented line — in the plain file editor and in diff tabs — as a card that sits between lines and pushes the code below it down. The card carries the thread's replies and its reply composer, so a reply can be written where the comment is, and a Resolve button that collapses the thread to a marker in the gutter. Clicking that marker brings the thread back; resolving is treated as "done with this", so a resolved thread starts collapsed and stays one click away. Every surface reads the same store caches the Checks pane writes, so a reply made inline appears in the sidebar and vice versa without either telling the other. The Comments section header gained a toggle for the whole feature (on by default, also in Settings), and a comment's file:line badge in the sidebar is now a link into the code that opens the file and reveals the thread. A comment's line number belongs to the pull request's head commit, so inline placement is gated on the checkout being that commit. When it is not, nothing is drawn and the Comments header says why, with both short SHAs; the reader can still click through from the sidebar, which then falls back to the provider. Rendering a comment against code it was never about would read as a claim about that code, which is worse than explaining the absence. The inline machinery is the one the local review notes already used: review threads join that Monaco view-zone pipeline rather than getting a second one, which is why a thread and a note can share a file without either leaking into the other's controls. Two adjacent editor settings moved into their own components, following the four settings already extracted in that file — the file was at its line limit before this change.
Three corrections to the inline-comment gate, all found by checking the assumptions the first commit rested on rather than by a failing test. The gate compared the checkout only against the commit the cached comments were resolved for, never against the pull request's own head. A worktree left on an older revision therefore presented that revision's review as the current one. Both comparisons are now required, and the notice under the Comments header names each short SHA for what it is — the checkout, and the commit the comments' line numbers belong to — instead of asserting which one the pull request points at, which was only true for one of the two ways the gate can fail. A comment fetch that recorded no head inherited the previous entry's, so fresh comments could carry a stale head and render on the wrong lines. An absent head now records none, which withholds inline placement instead. The gutter marker's whole design rested on monaco-editor defaulting the glyph margin to false. It does — but not for the reason the source suggests: the option is registered with a default of true, and the standalone entry point the package ships (esm/vs/editor/editor.api2.js) overrides it to false. Reading only the registry would have inverted the hook and stripped the gutter column from every editor. That override is now asserted against the library in a test, along with the marker's class, its enable/release cycle, and the gutter hit test.
…apter structured-agent-session-adapter.ts is a type and error-class module; three runtime helpers at its end pushed it past the 300-line budget, which fails the whole-repo lint gate that every PR in this fork runs. They move to structured-agent-session-provider-exit-proof.ts and the six call sites import them from there, with bodies unchanged. This is the same split upstream made in the same file, so the next sync brings no conflict here.
…odule register-worktree-catalog-handlers.ts carried a concurrency constant and two list-batching helpers that its own handler was the only caller of, which put the file five lines over the 300-line budget the whole-repo lint gate enforces. They move to worktree-catalog-list-all.ts and the handler imports them.
…r own file worktrees-local-create-flow.test.ts was 34 counted lines past the 800-line budget. The five base-ref probe cases move into worktrees-local-create-base-prefetch.test.ts, which mirrors the existing worktrees-ssh-create-base-prefetch.test.ts, taking the mock preamble with them (Vitest hoists vi.mock per file, so it cannot be shared). Both files pass: 20 tests, five of which moved.
FileExplorer.tsx was 20 counted lines past the 400-line budget for a .tsx file, which fails the whole-repo lint gate. The no-workspace notice and the search half of the shared body slot become FileExplorerWorkspacePlaceholder and FileExplorerSearchOverlay, both self-contained and neither needing new state. 2248 renderer tests pass.
…n module protocol-version.ts was four counted lines past the 300-line budget the whole-repo lint gate enforces, and it is the last file over it. The ORCHESTRATION_* capability constants and their nine registry entries move to orchestration-runtime-capabilities.ts, which also exports them as one array the registry spreads. protocol-version.ts re-exports the module, so the wire surface is unchanged: all 77 exported names survive, the capability string values are byte-identical, and the registry expands to the same list in the same order. Verified against those invariants, plus the shared suite (10466 tests), the orchestration consumers (97 tests), and the RPC params catalog check.
… render react-doctor's no-ref-current-in-render rule failed the static-analysis job on six writes this branch added. Each one assigned a ref during render, which React may replay or discard, so the value can leak from UI that never commits. The three hooks now stash their callbacks from an effect. useDiffCommentDecorator uses an effect with no dependency list because callers pass inline arrows, so a dependency array would be rebuilt on every render.
…orca package (windows) failed on main's codex managed-hook install suite: the test built its expected command from a literal ".orca" while the fork's home directory is ".orca-np", so the assertion never matched on a Windows runner. Same change as the one carried by nplez1/Agent-Dash-Improvements, kept byte-identical so whichever branch lands second is a no-op.
nplez1
added a commit
that referenced
this pull request
Oct 6, 2026
* feat(review): draw GitHub PR comments inline in the code The Checks pane listed a release request's review threads, but reading one and acting on it meant leaving Orca: opening the file showed no trace of the comment, and replying or resolving meant a trip to github.com. Review threads now render on their commented line — in the plain file editor and in diff tabs — as a card that sits between lines and pushes the code below it down. The card carries the thread's replies and its reply composer, so a reply can be written where the comment is, and a Resolve button that collapses the thread to a marker in the gutter. Clicking that marker brings the thread back; resolving is treated as "done with this", so a resolved thread starts collapsed and stays one click away. Every surface reads the same store caches the Checks pane writes, so a reply made inline appears in the sidebar and vice versa without either telling the other. The Comments section header gained a toggle for the whole feature (on by default, also in Settings), and a comment's file:line badge in the sidebar is now a link into the code that opens the file and reveals the thread. A comment's line number belongs to the pull request's head commit, so inline placement is gated on the checkout being that commit. When it is not, nothing is drawn and the Comments header says why, with both short SHAs; the reader can still click through from the sidebar, which then falls back to the provider. Rendering a comment against code it was never about would read as a claim about that code, which is worse than explaining the absence. The inline machinery is the one the local review notes already used: review threads join that Monaco view-zone pipeline rather than getting a second one, which is why a thread and a note can share a file without either leaking into the other's controls. Two adjacent editor settings moved into their own components, following the four settings already extracted in that file — the file was at its line limit before this change. * fix(review): only place inline comments on the revision they describe Three corrections to the inline-comment gate, all found by checking the assumptions the first commit rested on rather than by a failing test. The gate compared the checkout only against the commit the cached comments were resolved for, never against the pull request's own head. A worktree left on an older revision therefore presented that revision's review as the current one. Both comparisons are now required, and the notice under the Comments header names each short SHA for what it is — the checkout, and the commit the comments' line numbers belong to — instead of asserting which one the pull request points at, which was only true for one of the two ways the gate can fail. A comment fetch that recorded no head inherited the previous entry's, so fresh comments could carry a stale head and render on the wrong lines. An absent head now records none, which withholds inline placement instead. The gutter marker's whole design rested on monaco-editor defaulting the glyph margin to false. It does — but not for the reason the source suggests: the option is registered with a default of true, and the standalone entry point the package ships (esm/vs/editor/editor.api2.js) overrides it to false. Reading only the registry would have inverted the hook and stripped the gutter column from every editor. That override is now asserted against the library in a test, along with the marker's class, its enable/release cycle, and the gutter hit test. * fix(lint): move the provider exit-proof helpers out of the session adapter structured-agent-session-adapter.ts is a type and error-class module; three runtime helpers at its end pushed it past the 300-line budget, which fails the whole-repo lint gate that every PR in this fork runs. They move to structured-agent-session-provider-exit-proof.ts and the six call sites import them from there, with bodies unchanged. This is the same split upstream made in the same file, so the next sync brings no conflict here. * fix(lint): move the worktree catalog's batch helpers into their own module register-worktree-catalog-handlers.ts carried a concurrency constant and two list-batching helpers that its own handler was the only caller of, which put the file five lines over the 300-line budget the whole-repo lint gate enforces. They move to worktree-catalog-list-all.ts and the handler imports them. * test(worktrees): split the local create base-prefetch cases into their own file worktrees-local-create-flow.test.ts was 34 counted lines past the 800-line budget. The five base-ref probe cases move into worktrees-local-create-base-prefetch.test.ts, which mirrors the existing worktrees-ssh-create-base-prefetch.test.ts, taking the mock preamble with them (Vitest hoists vi.mock per file, so it cannot be shared). Both files pass: 20 tests, five of which moved. * refactor(explorer): lift two presentational blocks out of FileExplorer FileExplorer.tsx was 20 counted lines past the 400-line budget for a .tsx file, which fails the whole-repo lint gate. The no-workspace notice and the search half of the shared body slot become FileExplorerWorkspacePlaceholder and FileExplorerSearchOverlay, both self-contained and neither needing new state. 2248 renderer tests pass. * refactor(protocol): lift the orchestration capabilities into their own module protocol-version.ts was four counted lines past the 300-line budget the whole-repo lint gate enforces, and it is the last file over it. The ORCHESTRATION_* capability constants and their nine registry entries move to orchestration-runtime-capabilities.ts, which also exports them as one array the registry spreads. protocol-version.ts re-exports the module, so the wire surface is unchanged: all 77 exported names survive, the capability string values are byte-identical, and the registry expands to the same list in the same order. Verified against those invariants, plus the shared suite (10466 tests), the orchestration consumers (97 tests), and the RPC params catalog check. * fix(react): stash the latest comment callbacks in effects, not during render react-doctor's no-ref-current-in-render rule failed the static-analysis job on six writes this branch added. Each one assigned a ref during render, which React may replay or discard, so the value can leak from UI that never commits. The three hooks now stash their callbacks from an effect. useDiffCommentDecorator uses an effect with no dependency list because callers pass inline arrows, so a dependency array would be rebuilt on every render. * fix(test): read the fork's home directory name instead of hardcoded .orca package (windows) failed on main's codex managed-hook install suite: the test built its expected command from a literal ".orca" while the fork's home directory is ".orca-np", so the assertion never matched on a Windows runner. Same change as the one carried by nplez1/Agent-Dash-Improvements, kept byte-identical so whichever branch lands second is a no-op.
nplez1
pushed a commit
that referenced
this pull request
Oct 19, 2026
…iew off the deprecated owner resolver (stablyai#30562) * refactor(renderer): fold browser creation, file actions and doc-preview callers off the deprecated owner resolver The creation-action policy and browser creation (#3, #48) fold together, so the provider the policy offers is the one creation uses. Browser panes, page lifecycle and conversion, tab duplication, the explorer tree pane, doc-preview actions and editor path moves (#21, #43, #45-#47, #49, #50) read runtimeEnvironmentIdWithLegacyFallbackUnlessTwin. File-explorer writes on a folder (#51) take the owner from the rows alone, never from the focused server. Deprecated importers 16 -> 6. * refactor(renderer): fold file preview off the deprecated owner resolver; Open in Orca Browser refuses a twin A project two hosts publish reads as no server for the doc-preview plan, but the file:// path now refuses it with its own message instead of opening this machine's copy (#7). Deprecated importers 6 -> 5. * fix(renderer): a twin's file preview and open-externally refuse instead of opening this machine's copy Codex P2 on the F7 folds: a folder twin read as no server, so the preview plan built a local file:// tab and open-externally handed the path to this machine's OS. Both now check the owner for ambiguity first. Also fix two test fixtures that broke the renderer typecheck. * refactor(renderer): move the file:// open target out of file-preview.ts file-preview.ts crossed its line limit after main grew it; the file:// target, its type and its two refusal messages move to workspace-file-browser-open-target.ts, which they never depended on file-preview for. * test: mock the replacement resolver in the quick-action availability suite (Codex P2)
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
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.
ELI5
When someone reviews your pull request on GitHub, their comments live on the GitHub website. This change puts each one directly underneath the line of code it's about, inside Orca. You can read it there, type a reply there, and mark it resolved there, without opening a browser.
What Changed
Before. If you wanted to read a review comment, you opened the Checks pane on the right, found the comment in a list, and clicked through to github.com. The file you were looking at in Orca showed no sign that anyone had commented on it. Replying and resolving also happened on the website.
After. Open any file that a pull request has comments on and each thread appears on its own line, as a card that pushes the lines below it down so nothing is hidden. The card shows the thread's replies, has a box to type a reply, and has a Resolve button. Resolving collapses the thread to a small bubble in the left gutter; clicking the bubble brings it back. A button in the Comments header of the Checks pane turns the whole thing off if you find it distracting — the gutter bubbles stay, so nothing is lost.
Clicking a comment's
file:linebadge in the Checks pane now takes you into the file and reveals that thread, instead of opening github.com.This works in the plain file editor, in worktree diff tabs, and in the PR dialog's Files tab.
The mechanism. Orca already had the pieces, unused for this: the local review notes ("Add note for the AI") draw Monaco view zones on lines, and PR comments were already being mapped into that same shape for the PR diff viewer — read-only, with no reply or resolve. This change makes that card editable and adds reply, resolve and collapse to it, then mounts it on the surfaces that were missing it (the plain file editor in particular).
Two details worth naming:
The one rule worth reviewing. A comment's line number is a line in the pull request's head commit. If the commit checked out in your worktree is a different commit, that line number may point at unrelated code, and drawing the comment there would state something false about the code. So inline comments are drawn only when the checkout matches both the pull request's head and the commit the comments' line numbers were resolved for. When it matches neither, nothing is drawn and the Comments header explains why with both short commit hashes; clicking a comment in the sidebar still works, and falls back to opening it on GitHub. This required recording which commit the cached comments belong to — the cache is keyed by repo and PR number, not by commit, so that fact had to travel with the comments.
Why
Why not put the comment in a popover or a separate panel? The request was specifically for the comment to sit on its line and push content down, which is what a view zone does. That also means the code the comment is about stays visible while you read it.
Why reuse the local-review-note machinery rather than build a parallel one? The view-zone lifecycle (creating zones, tracking a fixed height Monaco never re-measures, patching a card in place, tearing zones down on model swap) is subtle and already covered by 114 tests. Review threads run through that same untested-nothing path rather than a second implementation of it.
Why one shared card surface? A review thread card and a local note card appear on the same lines in the same file. If they were styled differently a reader would assume they meant different things. They use the same CSS, so the difference is content, not appearance.
Why the head rule rather than showing comments anyway? A comment rendered against the wrong code reads as a claim about that code. Explaining the absence is cheaper than a reviewer trusting a wrong line.
Linked Issue
No tracked issue — this was requested directly and implemented on the fork. The
nplez1/mainbranch tracker (BRANCHES.md) is the record.Visual Proof
Not attached, and that is a real gap in this PR. The change builds and its logic is covered by unit tests, but I did not run it in Electron, so there is no screenshot of a rendered review thread or of the gutter marker. Everything about the glyph-margin gutter was verified against Monaco's source and option registry rather than by looking at it — which did catch a genuine bug (see below), but is not the same as having seen it. A reviewer should treat the rendered result as unconfirmed until someone runs it.
Worth knowing before reviewing: the gutter bubble makes Monaco reserve a gutter column in files that have a collapsed thread. Files with no review comments are laid out exactly as before.
Testing
pnpm tc— clean.pnpm teston the touched trees (editor, right-sidebar, settings, diff-comments, pr-comment-inline, github-item-dialog, store, lib): 15,224 passing. 51 new tests cover thread derivation, the head gate, the collapse/reveal state machine, and the gutter decorations.pnpm run check:code-quality:changed— 0 findings, including the design-system gate.verify:localization-coverage/-extraction/-catalogs), max-lines ratchet, dead-classes.Two things a reviewer should know about how this was checked. The head-gate comparison and the gutter option were both driven by reading Monaco and Orca source, and one of my assumptions was wrong in a way that would have shipped a regression:
monaco-editorregisters the glyph-margin option with a default oftrueand then overrides it tofalsein the standalone entry point the package actually ships. Reading only the registry led me to "fix" the option handling into a form that would have stripped the gutter column out of every editor. That override is now asserted against the library in a test, so a dependency bump that drops it fails loudly instead of quietly changing every editor's layout.Notes
Cross-platform. No path, shell or shortcut handling.
pathcomparisons normalise separators when matching a GitHub path against an editor path, since GitHub always uses/.SSH / remote. The review identity is derived from the same repo/PR caches the Checks pane already uses, which carry the execution-host scope, so a remote worktree resolves its own pull request rather than a local one. The comment cache key picks up connection and execution-host scope from the same helper as before.
Mobile. Untouched; this is a desktop renderer surface.
Performance. The derivation memoizes on the store slice identities it reads, so an unrelated store write does not re-derive. Only files that actually have threads allocate threads.
Backwards compatibility. One new setting,
prCommentsInlineEnabled, defaults to on, and absent reads as on so an existing profile does not silently lose the feature. The comment cache gained an optional field; an entry written before this change has no recorded commit, which withholds inline placement rather than guessing.Known follow-ups, not addressed here:
pull-request-page/files/combined-diff-viewer.tsxstill renders PR comments read-only through the older mapping helper. It is a fourth surface outside the three this change targeted, and it needs the same treatment the PR dialog got.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Checklist
N/Awith reason — not attached; see Visual Proofpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)CI status
static analysiswas red on this PR for a reason unrelated to it, and that is now repaired. CI runspnpm exec oxlintover the whole repository (pr.yml:197), andnplez1/mainalready carried fivemax-linesviolations — every file byte-identical to the base, with this PR adding zero linterrors. The
verifygate then failed only because it requiresstatic_analysisto succeed, whichalso skipped
test/package.Where they came from:
src/main/native-chat/agent-session-wire/structured-agent-session-adapter.tsstructured-agent-session-provider-exit-proof.ts— the same split upstream made, so the next sync is cleansrc/main/ipc/worktrees/listing/register-worktree-catalog-handlers.tsworktree-catalog-list-all.tssrc/shared/protocol-version.tsORCHESTRATION_*capabilities move toorchestration-runtime-capabilities.tsand the registry spreads themsrc/renderer/src/components/right-sidebar/FileExplorer.tsxsrc/main/ipc/worktrees-local-create-flow.test.tsworktrees-local-create-base-prefetch.test.ts, mirroring the existing SSH counterpartpnpm exec oxlintover the whole repository now exits 0. Bodies moved unchanged; the containerchanges only.
The one file worth a second look is
protocol-version.ts, a wire contract. Its exported surface isunchanged — all 77 exported names survive, the capability string values are byte-identical, and the
registry expands to the same list in the same order. That was verified explicitly, along with the
shared suite (10,466 tests), the orchestration consumers (97), and the RPC params catalog check.
Separately,
nplez1/mainalready fails 17 tests insrc/main/native-chat/agent-session-wire/(
structured-agent-session-acquisition-options,-settled-attach-retry,-crash-mid-start). Iconfirmed this by stashing my edits and re-running: identical failures, identical count, on pristine
sources. Nothing in this PR touches that behavior, and it is not addressed here.