Skip to content

refactor(session): canonicalize comment navigation - #896

Merged
benvinegar merged 3 commits into
mainfrom
cleanup/pr-845-comment-navigation
Aug 29, 2026
Merged

refactor(session): canonicalize comment navigation#896
benvinegar merged 3 commits into
mainfrom
cleanup/pr-845-comment-navigation

Conversation

@benvinegar

@benvinegar benvinegar commented Aug 29, 2026

Copy link
Copy Markdown
Member

Context

Follow-up cleanup for #845, now merged. This branch is rebased directly onto main.

Problem

The comment-ID navigation path resolves both a hunk hint and exact line coordinates in the daemon, then relies on the terminal bridge to discard the hunk hint. That sends conflicting target information across a boundary and gives a generic bridge command feature-specific normalization behavior.

Approach

  • resolve daemon navigation requests into one canonical live-session command input;
  • send comment-ID navigation as filePath + side + line, allowing the terminal to derive the containing hunk and reveal the exact row;
  • restore the terminal bridge to pass navigation inputs through unchanged;
  • remove the superseded bridge precedence test and make the broker assertion exact;
  • improve the incomplete-selector CLI error and add an empty maintenance Changeset.

This preserves separate CLI and daemon validation: the CLI owns user-facing errors, while the daemon continues to defend its wire boundary.

Validation

  • bun test src/app/cli.test.ts src/app/session/bridge.test.ts src/session/broker/brokerServer.helpers.test.ts — 173 passed
  • bun test test/session/cli.test.ts -t "navigate works" — 1 passed
  • bun run typecheck
  • bun run lint
  • bun run format:check
  • bun run check:docs
  • bun run check:changelog
  • bun run deps:check

This PR description was generated by Pi using GPT-5.6 Sol

@vercel

vercel Bot commented Aug 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hunk-web Ignored Ignored Preview Aug 29, 2026 9:01pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves comment-navigation canonicalization into the session broker and simplifies the application bridge to forward navigation inputs directly.

  • Resolves comment IDs to canonical file and line coordinates before dispatch.
  • Updates CLI validation wording and navigation tests.
  • Removes bridge-level preference for exact coordinates when both line and hunk targets are supplied.

Confidence Score: 4/5

The ambiguous navigation-target regression should be fixed before merging because direct session API requests can now silently navigate to a hunk instead of the requested line.

The server accepts and forwards simultaneous hunk and line targets, and removing bridge canonicalization changes the effective target from the exact line to the hunk without rejecting the ambiguous request.

Files Needing Attention: src/app/session/bridge.ts, src/session/broker/brokerServer.ts

Important Files Changed

Filename Overview
src/session/broker/brokerServer.ts Extracts canonical navigation-target resolution and drops stored hunk hints when resolving comment IDs, but continues accepting simultaneous hunk and line targets.
src/app/session/bridge.ts Simplifies navigation forwarding but removes the protection that previously gave exact line coordinates precedence over a simultaneous hunk index.
src/app/cli.ts Updates navigation validation messaging to include comment-ID mode without changing parsing behavior.
src/session/broker/brokerServer.helpers.test.ts Updates comment-ID expectations for canonical line-only dispatch but does not cover simultaneous hunk and line targets.
src/app/session/bridge.test.ts Removes the regression test asserting exact-coordinate precedence, matching the bridge behavior change.

Sequence Diagram

sequenceDiagram
    participant Client as Session API client
    participant Broker as Session broker
    participant Bridge as Application bridge
    participant UI as Terminal navigation
    Client->>Broker: navigate selector and target
    Broker->>Broker: Resolve comment ID or normalize fields
    Broker->>Bridge: navigate_to_hunk
    Bridge->>UI: Forward navigation input
    UI-->>Bridge: Navigated selection
    Bridge-->>Broker: Command result
    Broker-->>Client: JSON result
Loading
Prompt To Fix All With AI
### Issue 1
src/app/session/bridge.ts:100-101
**Exact line target loses precedence**

When a direct session API request contains both `hunkNumber` and exact `side`/`line` coordinates, the broker forwards both and this bridge now passes them through unchanged. `navigateToLocation` gives `hunkIndex` precedence, so the request silently selects the hunk instead of revealing the requested line.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "refactor(session): canonicalize comment ..." | Re-trigger Greptile

Comment thread src/app/session/bridge.ts
@benvinegar
benvinegar changed the base branch from stack/pr-845-base to main August 29, 2026 13:47
@benvinegar
benvinegar force-pushed the cleanup/pr-845-comment-navigation branch from 859e353 to 8314aca Compare August 29, 2026 13:47
@benvinegar
benvinegar merged commit 3a52e5f into main Aug 29, 2026
12 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