fix(ui): navigate comments by stream position

[benvinegar]

Summary

• resolves annotated/comment navigation relative to the full review stream when the current hunk is not annotated
• updates the React review controller and session navigation path to pass full hunk cursors into the navigation helper
• adds unit and PTY regressions for resuming } navigation from an unannotated hunk

Testing

bun test src/ui/lib/hunks.test.ts
tmp=$(mktemp -d); XDG_CONFIG_HOME="$tmp" bun test test/pty/ui-integration.test.ts -t "comment navigation resumes"; rc=$?; rm -rf "$tmp"; exit $rc
bun run typecheck
bun run format:check

This PR description was generated by Pi using OpenAI GPT-5

[greptile-apps]

Greptile Summary

Adds a PTY regression fixture and integration test to verify that navigating between agent-context notes using } across multiple files does not trigger the prior "Maximum update depth exceeded" crash.

• harness.ts: New createAgentNavigationRepoFixture builds a three-file git repo (alpha/beta/gamma, 80/20/80 lines) with annotated notes on alpha and gamma at line 60 and no annotation on beta, specifically to exercise the "skip unannotated file" navigation path.
• ui-integration.test.ts: New test launches hunk with --agent-notes, presses } twice, and asserts both that the correct note text appears and that no crash string is present after each navigation step.

Confidence Score: 4/5

Safe to merge — only test infrastructure is touched and the fixture logic is straightforward.

Both changed files are test-only. The fixture correctly mirrors existing harness patterns and the CLI flag it uses (--agent-notes) is confirmed in the source. The one minor inconsistency is that alphaNote gets only a crash-guard assertion while gammaNote also has a positive content assertion.

No production files are changed; ui-integration.test.ts has a small asymmetry in the alphaNote assertions worth reviewing.

Important Files Changed

Filename	Overview
test/pty/harness.ts	Adds createAgentNavigationRepoFixture — a three-file git repo fixture (alpha/beta/gamma) with agent-context annotations on alpha and gamma at line 60, deliberately leaving beta unannotated to exercise cross-file skipping. The line-number arithmetic, padStart formatting, and JSON structure all match existing patterns correctly.
test/pty/ui-integration.test.ts	Adds a PTY regression test that launches hunk with --agent-notes, presses } twice to navigate between annotated notes, and asserts no "Maximum update depth exceeded" crash occurs. Minor: alpha note snapshot lacks the symmetric positive toContain assertion that the gamma note has.

Sequence Diagram

sequenceDiagram
    participant Test as PTY Test
    participant Session as Hunk PTY Session
    participant UI as Hunk UI (split mode)

    Test->>Session: launchHunk(--agent-notes, --agent-context)
    Session->>UI: start with notes visible
    UI-->>Test: waitForText(menu bar) → initial snapshot

    Test->>Session: "press("}")"
    Session->>UI: navigate to next agent note
    UI-->>Test: waitForSnapshot("Alpha note for navigation.")
    Note over Test: assert no crash string

    Test->>Session: "press("}")"
    Session->>UI: skip beta.ts (no annotation), navigate to gamma
    UI-->>Test: waitForSnapshot("Gamma note for navigation.")
    Note over Test: assert text present + no crash string

    Test->>Session: close()

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
test/pty/ui-integration.test.ts:116
The `alphaNote` snapshot is validated only by the crash-guard assertion, while `gammaNote` gets both a positive content assertion and the crash guard. Since `waitForSnapshot` guarantees the predicate holds before returning, the positive assertion would be redundant *functionally*, but the inconsistency makes the intent less obvious to future readers and could hide a scenario where the predicate logic is accidentally loosened.

```suggestion
      expect(alphaNote).toContain("Alpha note for navigation.");
      expect(alphaNote).not.toContain("Maximum update depth exceeded");
```

_{Reviews (1): Last reviewed commit: "test(pty): cover agent comment navigatio..." | Re-trigger Greptile}

[greptile-apps]

The alphaNote snapshot is validated only by the crash-guard assertion, while gammaNote gets both a positive content assertion and the crash guard. Since waitForSnapshot guarantees the predicate holds before returning, the positive assertion would be redundant functionally, but the inconsistency makes the intent less obvious to future readers and could hide a scenario where the predicate logic is accidentally loosened.

Suggested change

	expect(alphaNote).not.toContain("Maximum update depth exceeded");
	expect(alphaNote).toContain("Alpha note for navigation.");
	expect(alphaNote).not.toContain("Maximum update depth exceeded");

Prompt To Fix With AI

This is a comment left during a code review.
Path: test/pty/ui-integration.test.ts
Line: 116

Comment:
The `alphaNote` snapshot is validated only by the crash-guard assertion, while `gammaNote` gets both a positive content assertion and the crash guard. Since `waitForSnapshot` guarantees the predicate holds before returning, the positive assertion would be redundant *functionally*, but the inconsistency makes the intent less obvious to future readers and could hide a scenario where the predicate logic is accidentally loosened.

```suggestion
      expect(alphaNote).toContain("Alpha note for navigation.");
      expect(alphaNote).not.toContain("Maximum update depth exceeded");
```

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[benvinegar]

Good call — added the explicit positive assertion for the alpha snapshot so it matches the gamma assertion pattern.

_{Responded by Pi using OpenAI GPT-5.}