Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/ui/hooks/useReviewController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,14 +225,15 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon
selectedFile?.id,
selectedHunkIndex,
delta,
hunkCursors,
);
if (!nextCursor) {
return;
}

selectHunk(nextCursor.fileId, nextCursor.hunkIndex, { scrollToNote: true });
},
[annotatedHunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex],
[annotatedHunkCursors, hunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex],
);

/** Cycle through only the currently visible files that carry annotations. */
Expand Down
37 changes: 37 additions & 0 deletions src/ui/lib/hunks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,41 @@ describe("annotated hunk navigation", () => {
hunkIndex: 1,
});
});

test("uses full stream position when annotated navigation starts on an unannotated hunk", () => {
const streamCursors: HunkCursor[] = [
{ fileId: "alpha", hunkIndex: 0 },
{ fileId: "alpha", hunkIndex: 1 },
{ fileId: "beta", hunkIndex: 0 },
{ fileId: "gamma", hunkIndex: 0 },
{ fileId: "gamma", hunkIndex: 1 },
{ fileId: "omega", hunkIndex: 0 },
];
const annotatedCursors: HunkCursor[] = [
{ fileId: "alpha", hunkIndex: 1 },
{ fileId: "gamma", hunkIndex: 0 },
{ fileId: "gamma", hunkIndex: 1 },
];

expect(findNextHunkCursor(annotatedCursors, "beta", 0, 1, streamCursors)).toEqual({
fileId: "gamma",
hunkIndex: 0,
});
expect(findNextHunkCursor(annotatedCursors, "beta", 0, -1, streamCursors)).toEqual({
fileId: "alpha",
hunkIndex: 1,
});
expect(findNextHunkCursor(annotatedCursors, "alpha", 0, 1, streamCursors)).toEqual({
fileId: "alpha",
hunkIndex: 1,
});
expect(findNextHunkCursor(annotatedCursors, "gamma", 1, 1, streamCursors)).toEqual({
fileId: "gamma",
hunkIndex: 1,
});
expect(findNextHunkCursor(annotatedCursors, "omega", 0, 1, streamCursors)).toEqual({
fileId: "gamma",
hunkIndex: 1,
});
});
});
54 changes: 51 additions & 3 deletions src/ui/lib/hunks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export function findNextHunkCursor(
currentFileId: string | undefined,
currentHunkIndex: number,
delta: number,
streamCursors: HunkCursor[] = cursors,
): HunkCursor | null {
if (cursors.length === 0) {
return null;
Expand All @@ -40,9 +41,56 @@ export function findNextHunkCursor(
const nextIndex =
currentIndex >= 0
? Math.min(Math.max(currentIndex + delta, 0), cursors.length - 1)
: delta >= 0
? 0
: cursors.length - 1;
: findNearestCursorIndex(cursors, streamCursors, currentFileId, currentHunkIndex, delta);

return cursors[nextIndex] ?? null;
}

/** Resolve relative movement when the current hunk is not in the target cursor subset. */
function findNearestCursorIndex(
cursors: HunkCursor[],
streamCursors: HunkCursor[],
currentFileId: string | undefined,
currentHunkIndex: number,
delta: number,
) {
if (!currentFileId) {
return delta >= 0 ? 0 : cursors.length - 1;
}

const currentStreamIndex = streamCursors.findIndex(
(cursor) => cursor.fileId === currentFileId && cursor.hunkIndex === currentHunkIndex,
);
if (currentStreamIndex < 0) {
return delta >= 0 ? 0 : cursors.length - 1;
}

const streamIndexByCursor = new Map(
streamCursors.map((cursor, index) => [`${cursor.fileId}\0${cursor.hunkIndex}`, index] as const),
);
const cursorStreamIndex = (cursor: HunkCursor) =>
streamIndexByCursor.get(`${cursor.fileId}\0${cursor.hunkIndex}`) ?? -1;
const indexedCursors = cursors
.map((cursor, index) => ({ index, streamIndex: cursorStreamIndex(cursor) }))
.filter(({ streamIndex }) => streamIndex >= 0);

if (indexedCursors.length === 0) {
return delta >= 0 ? 0 : cursors.length - 1;
}

// Comment navigation is non-cyclic like normal hunk navigation, so positions outside
// the annotated span clamp to the nearest annotated edge instead of wrapping.
if (delta >= 0) {
const nextCursor = indexedCursors.find(({ streamIndex }) => streamIndex > currentStreamIndex);
return nextCursor?.index ?? indexedCursors[indexedCursors.length - 1]!.index;
}

for (let index = indexedCursors.length - 1; index >= 0; index -= 1) {
const indexedCursor = indexedCursors[index]!;
if (indexedCursor.streamIndex < currentStreamIndex) {
return indexedCursor.index;
}
}

return indexedCursors[0]!.index;
}
9 changes: 8 additions & 1 deletion src/ui/lib/reviewState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,15 @@ export function resolveReviewNavigationTarget({
}): ReviewNavigationTarget {
if (input.commentDirection) {
const delta = input.commentDirection === "next" ? 1 : -1;
const hunkCursors = buildHunkCursors(visibleFiles);
const annotatedCursors = buildAnnotatedHunkCursors(visibleFiles);
const nextCursor = findNextHunkCursor(annotatedCursors, currentFileId, currentHunkIndex, delta);
const nextCursor = findNextHunkCursor(
annotatedCursors,
currentFileId,
currentHunkIndex,
delta,
hunkCursors,
);

if (!nextCursor) {
throw new Error("No annotated hunks found in the current review.");
Expand Down
68 changes: 68 additions & 0 deletions test/pty/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,73 @@ export function createPtyHarness() {
return { dir, before, after, agentContext };
}

function createAgentNavigationRepoFixture() {
const alphaBeforeLines = createNumberedExportLines(1, 80).split("\n");
const alphaAfterLines = [...alphaBeforeLines];
alphaAfterLines[0] = "export const line01 = 1001;";
alphaAfterLines[59] = "export const line60 = 6000;";

const betaBeforeLines = createNumberedExportLines(81, 20).split("\n");
const betaAfterLines = [...betaBeforeLines];
betaAfterLines[0] = "export const line81 = 8100;";

const gammaBeforeLines = createNumberedExportLines(101, 80).split("\n");
const gammaAfterLines = [...gammaBeforeLines];
gammaAfterLines[0] = "export const line101 = 10100;";
gammaAfterLines[59] = "export const line160 = 16000;";

const fixture = createGitRepoFixture([
{
path: "alpha.ts",
before: `${alphaBeforeLines.join("\n")}\n`,
after: `${alphaAfterLines.join("\n")}\n`,
},
{
path: "beta.ts",
before: `${betaBeforeLines.join("\n")}\n`,
after: `${betaAfterLines.join("\n")}\n`,
},
{
path: "gamma.ts",
before: `${gammaBeforeLines.join("\n")}\n`,
after: `${gammaAfterLines.join("\n")}\n`,
},
]);
const agentContext = join(fixture.dir, "agent-context.json");

writeText(
agentContext,
JSON.stringify({
version: 1,
summary: "Agent navigation notes",
files: [
{
path: "alpha.ts",
annotations: [
{
newRange: [60, 60],
summary: "Alpha note for navigation.",
rationale: "Used to prove comment navigation can leave an earlier note.",
},
],
},
{
path: "gamma.ts",
annotations: [
{
newRange: [60, 60],
summary: "Gamma note for navigation.",
rationale: "Used to prove comment navigation resumes after an unannotated hunk.",
},
],
},
],
}),
);

return { ...fixture, agentContext };
}

function createMultiHunkFilePair() {
const dir = makeTempDir("hunk-tuistory-hunks-");
const before = join(dir, "before.ts");
Expand Down Expand Up @@ -515,6 +582,7 @@ export function createPtyHarness() {
cleanup,
countMatches,
createAgentFilePair,
createAgentNavigationRepoFixture,
createBottomClampedRepoFixture,
createCollapsedTopRepoFixture,
createCrossFileHunkNavigationRepoFixture,
Expand Down
42 changes: 42 additions & 0 deletions test/pty/ui-integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,48 @@ describe("live UI integration", () => {
}
});

test("comment navigation resumes from an unannotated hunk in stream order", async () => {
const fixture = harness.createAgentNavigationRepoFixture();
const session = await harness.launchHunk({
args: ["diff", "--mode", "split", "--agent-context", fixture.agentContext, "--agent-notes"],
cwd: fixture.dir,
cols: 160,
rows: 14,
});

try {
const initial = await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, {
timeout: 15_000,
});
expect(initial).not.toContain("Maximum update depth exceeded");

await session.press("}");
const alphaNote = await harness.waitForSnapshot(
session,
(text) => text.includes("Alpha note for navigation."),
5_000,
);
expect(alphaNote).toContain("Alpha note for navigation.");
expect(alphaNote).not.toContain("Maximum update depth exceeded");
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 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!

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.

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.


await session.press(".");
await harness.waitForSnapshot(session, (text) => text.includes("line101 = 10100"), 5_000);

await session.press("}");
const gammaNote = await harness.waitForSnapshot(
session,
(text) => text.includes("Gamma note for navigation."),
5_000,
);

expect(gammaNote).toContain("Gamma note for navigation.");
expect(gammaNote).not.toContain("Alpha note for navigation.");
expect(gammaNote).not.toContain("Maximum update depth exceeded");
} finally {
session.close();
}
});

test("real hunk navigation jumps to later hunks in the review stream", async () => {
const fixture = harness.createMultiHunkFilePair();
const session = await harness.launchHunk({
Expand Down
Loading