diff --git a/agent/reviewer_groups.py b/agent/reviewer_groups.py index 36e4c3dc..d5923eb0 100644 --- a/agent/reviewer_groups.py +++ b/agent/reviewer_groups.py @@ -52,10 +52,9 @@ class _DiffGroupModel(BaseModel): ) summary: str = Field( description=( - "GitHub-flavored markdown walkthrough of this group: a few short " - "sentences or bullets, code identifiers in `backticks`, optionally one " - "short fenced code block of the key snippet, and concrete locations " - "linked as [path:start-end](#loc=path:start-end)." + "A short, plain explanation of what this group changes and why, in a " + "few sentences a reviewer can skim. Wrap code identifiers, symbols, " + "types, flags, and paths in `backticks`. No code blocks or links." ) ) files: list[str] = Field( @@ -82,24 +81,18 @@ Rules: - Use at most {max_groups} groups. Prefer fewer, larger groups over many tiny ones. - title: a short headline naming the change, roughly 4-10 words, no trailing \ punctuation. Wrap code identifiers (symbols, flags, file names) in `backticks`. -- summary: GitHub-flavored markdown explaining what the group changes and why, \ -written as a short walkthrough a reviewer can skim: - - Keep it focused — a few short sentences or bullets. Do not restate the diff line by line. +- summary: a short, plain explanation of what the group changes and why, that a \ +reviewer can skim: + - Keep it focused — a few short sentences. Do not restate the diff line by line. - Wrap every code identifier, symbol, type, flag, and path in `backticks`. - - When one change is central, include at most ONE short fenced code block \ -(```lang, <= ~8 lines) of the key snippet — not the whole hunk. - - Reference concrete locations as markdown links of the EXACT form \ -[path:start-end](#loc=path:start-end), where path is the verbatim changed-file \ -path and start/end are line numbers from the "lines X-Y" annotations below \ -(use [path:line](#loc=path:line) when start == end). Prefer these links over \ -describing locations in prose — they let the reader jump straight to the hunk. + - Plain prose only — no code blocks and no links. - files: the exact file paths (copied verbatim from the list below) in this \ group, in the order a reviewer should read them. Changed files: {file_list} -Diffs (each hunk is annotated with its line range in the new file): +Diffs: {diffs} """ @@ -122,9 +115,7 @@ def _build_prompt(diff_text: str, files: list[str]) -> str: body = hunk.body if len(body) > MAX_FILE_HUNK_CHARS: body = body[:MAX_FILE_HUNK_CHARS] + "\n... (truncated)" - # Surface the new-file line range so the model can cite accurate - # locations in its summary links. - segments.append(f"lines {hunk.new_start}-{hunk.new_end}:\n```diff\n{body}\n```") + segments.append(f"```diff\n{body}\n```") block = f"### {file_diff.file}\n" + "\n".join(segments) + "\n" if budget - len(block) < 0: parts.append(f"### {file_diff.file}\n(diff omitted — prompt budget reached)\n") diff --git a/tests/test_reviewer_groups.py b/tests/test_reviewer_groups.py index 9051b645..222cfa22 100644 --- a/tests/test_reviewer_groups.py +++ b/tests/test_reviewer_groups.py @@ -97,14 +97,13 @@ async def test_generate_diff_groups_llm_failure_returns_none() -> None: assert groups is None -def test_build_prompt_annotates_hunk_line_ranges() -> None: +def test_build_prompt_includes_files_and_diffs() -> None: prompt = _build_prompt(_DIFF, ["foo.py", "bar.py"]) assert "### foo.py" in prompt assert "### bar.py" in prompt - # New-file line ranges from the @@ headers are surfaced so the model can - # cite accurate locations: foo.py @@ -1,2 +1,3 @@ and bar.py @@ -1 +1,2 @@. - assert "lines 1-3:" in prompt - assert "lines 1-2:" in prompt + # Hunks are rendered as plain diff fences (no line-range annotations). + assert "lines 1-3:" not in prompt + assert "```diff" in prompt # The verbatim file list is included so every file can be assigned. assert "- foo.py" in prompt assert "- bar.py" in prompt diff --git a/ui/src/components/agents/ReviewSidebar.tsx b/ui/src/components/agents/ReviewSidebar.tsx index 079f2c06..60ea95b6 100644 --- a/ui/src/components/agents/ReviewSidebar.tsx +++ b/ui/src/components/agents/ReviewSidebar.tsx @@ -51,7 +51,6 @@ export interface ReviewSidebarData { view: ReviewSidebarView onViewChange: (view: ReviewSidebarView) => void onSelectGroup: (index: number) => void - onLocationClick?: (file: string, startLine: number, endLine: number) => void } const ReviewSidebarContext = createContext<{ @@ -105,7 +104,6 @@ export function ReviewSidebarPanel({ data }: { data: ReviewSidebarData }) { groups={data.groups ?? []} onSelectGroup={data.onSelectGroup} onSelectFile={data.onSelect} - onLocationClick={data.onLocationClick} /> ) : !data.files ? (
@@ -183,12 +181,10 @@ function ReviewGroupList({ groups, onSelectGroup, onSelectFile, - onLocationClick, }: { groups: Array onSelectGroup: (index: number) => void onSelectFile: (path: string) => void - onLocationClick?: (file: string, startLine: number, endLine: number) => void }) { return (
@@ -198,7 +194,6 @@ function ReviewGroupList({ group={group} onSelect={() => onSelectGroup(group.index)} onSelectFile={onSelectFile} - onLocationClick={onLocationClick} /> ))}
@@ -211,6 +206,12 @@ function splitPath(path: string): { dir: string; base: string } { return { dir: path.slice(0, idx), base: path.slice(idx + 1) } } +// Older stored summaries embed `[label](#loc=path:line)` diff links. Render the +// label as inline code instead so no stale jump-links leak into the explanation. +function stripLocationLinks(summary: string): string { + return summary.replace(/\[([^\]]+)\]\(#loc=[^)]*\)/g, "`$1`") +} + // Render a title with `backtick`-delimited spans as inline code chips, matching // the Markdown component's inline-code styling, without pulling in the full // block renderer for a single line. @@ -234,12 +235,10 @@ function ReviewGroupRow({ group, onSelect, onSelectFile, - onLocationClick, }: { group: ReviewSidebarGroup onSelect: () => void onSelectFile: (path: string) => void - onLocationClick?: (file: string, startLine: number, endLine: number) => void }) { const [expanded, setExpanded] = useState(false) return ( @@ -297,7 +296,7 @@ function ReviewGroupRow({ )} {group.summary && ( -
+
{expanded && (
- +
)}
diff --git a/ui/src/components/agents/ported/Markdown.tsx b/ui/src/components/agents/ported/Markdown.tsx index e258cf04..a4e37c44 100644 --- a/ui/src/components/agents/ported/Markdown.tsx +++ b/ui/src/components/agents/ported/Markdown.tsx @@ -1,6 +1,5 @@ import { memo, useMemo } from "react"; import { Streamdown } from "streamdown"; -import { parseLocationHref } from "./markdownLocation"; import type { ReactNode } from "react"; import "streamdown/styles.css"; @@ -8,12 +7,6 @@ interface MarkdownProps { content: string; /** When true, keep Streamdown in streaming mode for the duration of the run. */ isLive?: boolean; - /** - * When set, `#loc=path:start-end` links render as in-page buttons that invoke - * this callback instead of navigating. Used by the review view to jump the - * diff to a referenced hunk. - */ - onLocationClick?: (file: string, startLine: number, endLine: number) => void; } /** @@ -86,37 +79,22 @@ const SHIKI_THEME: ["github-light", "github-dark"] = ["github-light", "github-da export const Markdown = memo(function Markdown({ content, isLive = false, - onLocationClick, }: MarkdownProps) { const components = useMemo( () => ({ ...STREAMDOWN_COMPONENTS, - a: ({ href, children }: { href?: string; children?: ReactNode }) => { - const loc = onLocationClick ? parseLocationHref(href) : null; - if (loc && onLocationClick) { - return ( - - ); - } - return ( - - {children} - - ); - }, + a: ({ href, children }: { href?: string; children?: ReactNode }) => ( + + {children} + + ), }), - [onLocationClick] + [] ); return ( diff --git a/ui/src/components/agents/ported/markdownLocation.test.ts b/ui/src/components/agents/ported/markdownLocation.test.ts deleted file mode 100644 index 0e2aa28c..00000000 --- a/ui/src/components/agents/ported/markdownLocation.test.ts +++ /dev/null @@ -1,53 +0,0 @@ -import { describe, expect, it } from "vitest"; - -import { parseLocationHref } from "./markdownLocation"; - -describe("parseLocationHref", () => { - it("parses a path with a start-end range", () => { - expect(parseLocationHref("#loc=src/foo.rs:231-232")).toEqual({ - file: "src/foo.rs", - startLine: 231, - endLine: 232, - }); - }); - - it("parses a single-line ref (no end)", () => { - expect(parseLocationHref("#loc=mod.rs:42")).toEqual({ - file: "mod.rs", - startLine: 42, - endLine: 42, - }); - }); - - it("tolerates an origin prefix the URL transform may prepend", () => { - expect(parseLocationHref("http://localhost/#loc=a/b.ts:5-9")).toEqual({ - file: "a/b.ts", - startLine: 5, - endLine: 9, - }); - }); - - it("decodes percent-encoded hrefs", () => { - expect(parseLocationHref("#loc=a%20b/c.ts:1-2")).toEqual({ - file: "a b/c.ts", - startLine: 1, - endLine: 2, - }); - }); - - it("clamps an end below the start to a single line", () => { - expect(parseLocationHref("#loc=x.ts:10-3")).toEqual({ - file: "x.ts", - startLine: 10, - endLine: 10, - }); - }); - - it("returns null for non-location and malformed hrefs", () => { - expect(parseLocationHref(undefined)).toBeNull(); - expect(parseLocationHref("https://example.com")).toBeNull(); - expect(parseLocationHref("#loc=noline")).toBeNull(); - expect(parseLocationHref("#loc=:5")).toBeNull(); - expect(parseLocationHref("#loc=x.ts:abc")).toBeNull(); - }); -}); diff --git a/ui/src/components/agents/ported/markdownLocation.ts b/ui/src/components/agents/ported/markdownLocation.ts deleted file mode 100644 index 30e85d24..00000000 --- a/ui/src/components/agents/ported/markdownLocation.ts +++ /dev/null @@ -1,35 +0,0 @@ -export interface MarkdownLocation { - file: string; - startLine: number; - endLine: number; -} - -/** - * Parse a `#loc=:[-]` sentinel href emitted by the reviewer - * diff-grouping pass into a file + line range. Tolerant of an origin prefix the - * markdown URL transform may prepend, and of percent-encoding. Returns null for - * any href that isn't a location link. - */ -export function parseLocationHref(href?: string): MarkdownLocation | null { - if (!href) return null; - const marker = href.indexOf("#loc="); - if (marker === -1) return null; - let raw = href.slice(marker + "#loc=".length); - try { - raw = decodeURIComponent(raw); - } catch { - // Keep raw as-is if it isn't valid percent-encoding. - } - const colon = raw.lastIndexOf(":"); - if (colon <= 0) return null; - const file = raw.slice(0, colon); - const [startStr, endStr] = raw.slice(colon + 1).split("-"); - const start = Number.parseInt(startStr ?? "", 10); - if (!Number.isFinite(start) || start <= 0) return null; - const end = endStr !== undefined ? Number.parseInt(endStr, 10) : start; - return { - file, - startLine: start, - endLine: Number.isFinite(end) && end >= start ? end : start, - }; -} diff --git a/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx b/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx index 0024204d..1ef9d472 100644 --- a/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx +++ b/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx @@ -222,11 +222,6 @@ function ReviewBody({ const [anchorEl, setAnchorEl] = useState(null) const scrollRef = useRef(null) const groupRefs = useRef>({}) - const [selectedRange, setSelectedRange] = useState<{ - file: string - start: number - end: number - } | null>(null) useEffect(() => { void warmDiffHighlighter() @@ -405,21 +400,6 @@ function ReviewBody({ }) }, []) - const scrollToLineRange = useCallback( - (file: string, start: number, end: number) => { - setSelectedFile(file) - setSelectedRange({ file, start, end }) - setExpandedFiles((prev) => ({ ...prev, [file]: true })) - requestAnimationFrame(() => { - fileRefs.current[file]?.scrollIntoView({ - block: "start", - behavior: "smooth", - }) - }) - }, - [] - ) - const openFinding = useCallback( (finding: ReviewFinding) => { markRead(finding.id) @@ -447,7 +427,6 @@ function ReviewBody({ file={file} findings={findingsByFile.get(file.path) ?? []} focused={focused} - selectedRange={selectedRange} viewed={viewed.has(file.path)} onToggleViewed={() => { const becomingViewed = !viewed.has(file.path) @@ -485,7 +464,6 @@ function ReviewBody({ view, onViewChange: setView, onSelectGroup: scrollToGroup, - onLocationClick: scrollToLineRange, }), [ detail.number, @@ -497,7 +475,6 @@ function ReviewBody({ view, setView, scrollToGroup, - scrollToLineRange, ] ) useRegisterReviewSidebar(sidebarData) @@ -689,7 +666,6 @@ function FileDiffCard({ file, findings, focused, - selectedRange, viewed, onToggleViewed, expanded, @@ -701,7 +677,6 @@ function FileDiffCard({ file: ReviewDiffFile findings: Array focused: ReviewFinding | null - selectedRange: { file: string; start: number; end: number } | null viewed: boolean onToggleViewed: () => void expanded: boolean @@ -728,16 +703,8 @@ function FileDiffCard({ if (focused?.file === file.path && isAnchored(focused)) { return findingSelectedRange(focused) } - if (selectedRange?.file === file.path) { - return { - start: selectedRange.start, - end: selectedRange.end, - side: "additions", - endSide: "additions", - } - } return null - }, [focused, selectedRange, file.path]) + }, [focused, file.path]) return (