mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[W9-4] Add code-snippet viewer for revision findings (#532)
Backend: GET /git/file reads a file at a branch tip (read_file_at_branch) and slices it to a line window — explicit start/end, a line+context center, or the whole file capped at 2000 lines. _compute_file_range is the pure helper (unit-tested). Frontend: useGitFile hook + CodeSnippet (styled <pre>, line numbers, active- line highlight — matches git-diff-viewer, no shiki). Wired into FindingCard so each file:line finding shows the surrounding source. Fail-open: a missing file renders a muted hint, never breaks the card. Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -0,0 +1,97 @@
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import React from "react";
|
||||
|
||||
// Stub useGitFile so CodeSnippet is tested in isolation from TanStack Query.
|
||||
const { useGitFile } = vi.hoisted(() => ({ useGitFile: vi.fn() }));
|
||||
|
||||
vi.mock("@/hooks/use-git", () => ({ useGitFile }));
|
||||
|
||||
import { CodeSnippet } from "../code-snippet";
|
||||
|
||||
interface MockReturn {
|
||||
data?: {
|
||||
branch: string;
|
||||
path: string;
|
||||
content: string;
|
||||
start_line: number;
|
||||
total_lines: number;
|
||||
truncated: boolean;
|
||||
} | null;
|
||||
isLoading?: boolean;
|
||||
isError?: boolean;
|
||||
}
|
||||
|
||||
function mockReturn(r: MockReturn) {
|
||||
useGitFile.mockReturnValue({
|
||||
data: r.data ?? null,
|
||||
isLoading: r.isLoading ?? false,
|
||||
isError: r.isError ?? false,
|
||||
});
|
||||
}
|
||||
|
||||
describe("CodeSnippet", () => {
|
||||
it("renders line numbers and the truncated note for a loaded slice", () => {
|
||||
mockReturn({
|
||||
data: {
|
||||
branch: "feature/backend/abc",
|
||||
path: "roboco/services/task.py",
|
||||
content: "import os\n\ndef x():\n pass",
|
||||
start_line: 40,
|
||||
total_lines: 200,
|
||||
truncated: true,
|
||||
},
|
||||
});
|
||||
const { container } = render(
|
||||
<CodeSnippet branch="feature/backend/abc" file="roboco/services/task.py" activeLine={42} />,
|
||||
);
|
||||
// Line numbers start at 40 for the 4 content lines.
|
||||
expect(screen.getByText("40")).toBeInTheDocument();
|
||||
expect(screen.getByText("43")).toBeInTheDocument();
|
||||
expect(screen.getByText("def x():")).toBeInTheDocument();
|
||||
expect(
|
||||
screen.getByText(/showing lines 40–43 of 200/),
|
||||
).toBeInTheDocument();
|
||||
// Sanity: only one snippet block rendered.
|
||||
expect(container.querySelectorAll("pre")).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("highlights the active line", () => {
|
||||
mockReturn({
|
||||
data: {
|
||||
branch: "b",
|
||||
path: "p.py",
|
||||
content: "a\nb\nc",
|
||||
start_line: 1,
|
||||
total_lines: 3,
|
||||
truncated: false,
|
||||
},
|
||||
});
|
||||
render(<CodeSnippet branch="b" file="p.py" activeLine={2} />);
|
||||
// The active line's content is "b"; its row carries the highlight class.
|
||||
const row = screen.getByText("b").parentElement;
|
||||
expect(row?.className).toContain("bg-blue-500/15");
|
||||
});
|
||||
|
||||
it("renders the fail-open hint on an error", () => {
|
||||
mockReturn({ isError: true });
|
||||
render(<CodeSnippet branch="b" file="p.py" activeLine={1} />);
|
||||
expect(screen.getByText(/Couldn’t load this file/)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders a skeleton while loading", () => {
|
||||
mockReturn({ isLoading: true });
|
||||
const { container } = render(
|
||||
<CodeSnippet branch="b" file="p.py" activeLine={1} />,
|
||||
);
|
||||
expect(container.querySelector(".animate-pulse")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("renders nothing when branch is missing", () => {
|
||||
mockReturn({ data: null });
|
||||
const { container } = render(
|
||||
<CodeSnippet branch={null} file="p.py" activeLine={1} />,
|
||||
);
|
||||
expect(container.firstChild).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,84 @@
|
||||
"use client";
|
||||
|
||||
import { useGitFile } from "@/hooks/use-git";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
import { ScrollArea } from "@/components/ui/scroll-area";
|
||||
import { cn } from "@/lib/utils";
|
||||
|
||||
interface CodeSnippetProps {
|
||||
branch: string | null | undefined;
|
||||
file: string | null | undefined;
|
||||
activeLine?: number | null;
|
||||
context?: number;
|
||||
}
|
||||
|
||||
// A compact file-content window for a revision finding: shows the lines around
|
||||
// `activeLine` from `branch:file`, with the flagged line highlighted. Lazy and
|
||||
// fail-open — a missing/deleted file renders a muted hint, never breaks the
|
||||
// finding card. Styled to match git-diff-viewer's <pre> convention.
|
||||
export function CodeSnippet({
|
||||
branch,
|
||||
file,
|
||||
activeLine,
|
||||
context = 10,
|
||||
}: CodeSnippetProps) {
|
||||
const enabled = !!branch && !!file;
|
||||
const { data, isLoading, isError } = useGitFile(
|
||||
branch,
|
||||
file,
|
||||
activeLine ?? undefined,
|
||||
context,
|
||||
enabled,
|
||||
);
|
||||
|
||||
if (!enabled) return null;
|
||||
|
||||
if (isLoading) {
|
||||
return <Skeleton className="h-24 w-full rounded" />;
|
||||
}
|
||||
|
||||
if (isError || !data || !data.content) {
|
||||
return (
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Couldn’t load this file’s content from {branch}.
|
||||
</p>
|
||||
);
|
||||
}
|
||||
|
||||
const lines = data.content.split("\n");
|
||||
const active = activeLine ?? null;
|
||||
|
||||
return (
|
||||
<div className="rounded border bg-muted/30">
|
||||
<ScrollArea className="h-64">
|
||||
<pre className="p-2 font-mono text-[11px] leading-relaxed sm:text-xs">
|
||||
{lines.map((text, i) => {
|
||||
const lineNo = data.start_line + i;
|
||||
const isActive = active != null && lineNo === active;
|
||||
return (
|
||||
<div
|
||||
key={i}
|
||||
className={cn(
|
||||
"flex gap-3 px-2 -mx-2 rounded-sm",
|
||||
isActive &&
|
||||
"bg-blue-500/15 ring-1 ring-inset ring-blue-500/30",
|
||||
)}
|
||||
>
|
||||
<span className="select-none w-8 shrink-0 text-right text-muted-foreground/60">
|
||||
{lineNo}
|
||||
</span>
|
||||
<span className="whitespace-pre">{text || " "}</span>
|
||||
</div>
|
||||
);
|
||||
})}
|
||||
</pre>
|
||||
</ScrollArea>
|
||||
{data.truncated && (
|
||||
<p className="border-t px-2 py-1 text-[10px] text-muted-foreground">
|
||||
showing lines {data.start_line}–{data.start_line + lines.length - 1}{" "}
|
||||
of {data.total_lines}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -11,6 +11,12 @@ const { useTaskFindings } = vi.hoisted(() => ({ useTaskFindings: vi.fn() }));
|
||||
|
||||
vi.mock("@/hooks/use-tasks", () => ({ useTaskFindings }));
|
||||
|
||||
// CodeSnippet runs a real useQuery (needs a QueryClient); stub it so the
|
||||
// findings test stays focused on grouping/rendering, not git fetching.
|
||||
vi.mock("@/components/git/code-snippet", () => ({
|
||||
CodeSnippet: () => <div data-testid="code-snippet" />,
|
||||
}));
|
||||
|
||||
import { TabFindings } from "../tab-findings";
|
||||
|
||||
function buildTask(overrides: Partial<Task> = {}): Task {
|
||||
|
||||
@@ -7,6 +7,7 @@ import { Card, CardContent } from "@/components/ui/card";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
import { ListChecks } from "lucide-react";
|
||||
import { CodeSnippet } from "@/components/git/code-snippet";
|
||||
|
||||
interface TabFindingsProps {
|
||||
task: Task;
|
||||
@@ -37,7 +38,13 @@ const ORIGIN_LABEL: Record<string, string> = {
|
||||
ceo: "CEO",
|
||||
};
|
||||
|
||||
function FindingCard({ finding }: { finding: TaskFinding }) {
|
||||
function FindingCard({
|
||||
finding,
|
||||
branch,
|
||||
}: {
|
||||
finding: TaskFinding;
|
||||
branch: string | null;
|
||||
}) {
|
||||
return (
|
||||
<Card>
|
||||
<CardContent className="pt-4 space-y-2">
|
||||
@@ -65,6 +72,13 @@ function FindingCard({ finding }: { finding: TaskFinding }) {
|
||||
</code>
|
||||
)}
|
||||
</div>
|
||||
{finding.file && (
|
||||
<CodeSnippet
|
||||
branch={branch}
|
||||
file={finding.file}
|
||||
activeLine={finding.line}
|
||||
/>
|
||||
)}
|
||||
<div className="space-y-1 text-sm">
|
||||
<p>
|
||||
<span className="text-muted-foreground">Expected:</span>{" "}
|
||||
@@ -154,7 +168,7 @@ export function TabFindings({ task }: TabFindingsProps) {
|
||||
</div>
|
||||
<div className="space-y-3">
|
||||
{group.items.map((f) => (
|
||||
<FindingCard key={f.id} finding={f} />
|
||||
<FindingCard key={f.id} finding={f} branch={task.branch_name} />
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -11,6 +11,7 @@ import type {
|
||||
GitLogResponse,
|
||||
GitBranchListResponse,
|
||||
GitDiffResponse,
|
||||
GitFileContentResponse,
|
||||
GitCommitRequest,
|
||||
GitCommitResponse,
|
||||
GitPushRequest,
|
||||
@@ -45,6 +46,8 @@ export const gitKeys = {
|
||||
[...gitKeys.all, "branches", projectSlug, { includeRemote }] as const,
|
||||
diff: (projectSlug: string, staged?: boolean, filePath?: string) =>
|
||||
[...gitKeys.all, "diff", projectSlug, { staged, filePath }] as const,
|
||||
file: (branch: string, path: string, line?: number, context?: number) =>
|
||||
[...gitKeys.all, "file", branch, path, { line, context }] as const,
|
||||
};
|
||||
|
||||
// =============================================================================
|
||||
@@ -118,6 +121,25 @@ export function useGitDiff(
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a file at a branch tip, sliced around an optional line.
|
||||
*/
|
||||
export function useGitFile(
|
||||
branch: string | null | undefined,
|
||||
path: string | null | undefined,
|
||||
line: number | null | undefined,
|
||||
context: number = 10,
|
||||
enabled: boolean = true,
|
||||
) {
|
||||
return useQuery<GitFileContentResponse>({
|
||||
queryKey: gitKeys.file(branch ?? "", path ?? "", line ?? undefined, context),
|
||||
queryFn: () => gitApi.getFile(branch!, path!, line ?? undefined, context),
|
||||
enabled: enabled && !!branch && !!path,
|
||||
staleTime: 60000, // 1 minute — branch content is stable
|
||||
retry: false, // a 404 (file gone) shouldn't retry forever
|
||||
});
|
||||
}
|
||||
|
||||
// =============================================================================
|
||||
// Mutation Hooks
|
||||
// =============================================================================
|
||||
|
||||
@@ -11,6 +11,7 @@ import type {
|
||||
GitLogResponse,
|
||||
GitBranchListResponse,
|
||||
GitDiffResponse,
|
||||
GitFileContentResponse,
|
||||
GitCommitRequest,
|
||||
GitCommitResponse,
|
||||
GitPushRequest,
|
||||
@@ -160,6 +161,35 @@ export const gitApi = {
|
||||
return data;
|
||||
},
|
||||
|
||||
/**
|
||||
* Read a file's content at a branch tip, sliced to a line window.
|
||||
*/
|
||||
getFile: async (
|
||||
branch: string,
|
||||
path: string,
|
||||
line?: number,
|
||||
context: number = 10,
|
||||
): Promise<GitFileContentResponse> => {
|
||||
if (isMockMode()) {
|
||||
const lines = Array.from(
|
||||
{ length: 5 },
|
||||
(_, i) => `// mock line ${i + 1}`,
|
||||
);
|
||||
return {
|
||||
branch,
|
||||
path,
|
||||
content: lines.join("\n"),
|
||||
start_line: 1,
|
||||
total_lines: lines.length,
|
||||
truncated: false,
|
||||
};
|
||||
}
|
||||
const { data } = await api.get<GitFileContentResponse>("/git/file", {
|
||||
params: { branch, path, line, context },
|
||||
});
|
||||
return data;
|
||||
},
|
||||
|
||||
// ===========================================================================
|
||||
// WRITE OPERATIONS
|
||||
// ===========================================================================
|
||||
|
||||
@@ -54,6 +54,15 @@ export interface GitDiffResponse {
|
||||
files_changed: number;
|
||||
}
|
||||
|
||||
export interface GitFileContentResponse {
|
||||
branch: string;
|
||||
path: string;
|
||||
content: string;
|
||||
start_line: number;
|
||||
total_lines: number;
|
||||
truncated: boolean;
|
||||
}
|
||||
|
||||
export interface GitCommitResponse {
|
||||
commit_hash: string;
|
||||
message: string;
|
||||
|
||||
@@ -43,6 +43,7 @@ from roboco.api.schemas.git import (
|
||||
GitDiffResponse,
|
||||
GitFetchRequest,
|
||||
GitFetchResponse,
|
||||
GitFileContentResponse,
|
||||
GitLogResponse,
|
||||
GitMergePRRequest,
|
||||
GitMergePRResponse,
|
||||
@@ -86,6 +87,43 @@ _LOG_FORMAT_PARTS = 5
|
||||
# bubbling as 500 Internal Server Errors with no `detail`.
|
||||
_TranslatableError = (ServiceError, GitError)
|
||||
|
||||
# Cap an unbounded whole-file read so a huge file can't flood the panel.
|
||||
_FILE_MAX_LINES = 2000
|
||||
|
||||
|
||||
def _compute_file_range(
|
||||
*,
|
||||
total: int,
|
||||
line: int | None,
|
||||
context: int,
|
||||
start: int | None,
|
||||
end: int | None,
|
||||
) -> tuple[int, int, bool]:
|
||||
"""Resolve the (start, end, truncated) slice for a file-content read.
|
||||
|
||||
Explicit ``start``/``end`` win; else ``line`` centers a context window;
|
||||
else the whole file. The whole-file case is capped at ``_FILE_MAX_LINES``.
|
||||
Returns 1-based inclusive [start, end] and whether the slice is shorter
|
||||
than the file.
|
||||
"""
|
||||
if start is not None and end is not None:
|
||||
s, e_ = start, end
|
||||
elif line is not None:
|
||||
s = max(1, line - context)
|
||||
e_ = min(total, line + context)
|
||||
else:
|
||||
s, e_ = 1, total
|
||||
|
||||
s = max(1, min(s, total))
|
||||
e_ = max(s, min(e_, total))
|
||||
|
||||
truncated = e_ < total
|
||||
if s == 1 and e_ == total and total > _FILE_MAX_LINES:
|
||||
e_ = _FILE_MAX_LINES
|
||||
truncated = True
|
||||
return s, e_, truncated
|
||||
|
||||
|
||||
# Roles permitted to rebase branches via the /rebase endpoint.
|
||||
# Rebase is a history-rewriting operation that should be authorised only by
|
||||
# PM-level or CEO-level callers. Developers are intentionally excluded:
|
||||
@@ -344,6 +382,59 @@ async def get_git_diff(
|
||||
)
|
||||
|
||||
|
||||
@router.get("/file", response_model=GitFileContentResponse)
|
||||
async def get_git_file(
|
||||
db: DbSession,
|
||||
agent: CurrentAgentContext,
|
||||
branch: str = Query(..., description="Task branch holding the file"),
|
||||
path: str = Query(..., description="Repo-relative file path"),
|
||||
line: int | None = Query(default=None, ge=1, description="Target line"),
|
||||
context: int = Query(default=10, ge=0, le=100, description="Context around line"),
|
||||
start: int | None = Query(default=None, ge=1, description="Explicit start line"),
|
||||
end: int | None = Query(default=None, ge=1, description="Explicit end line"),
|
||||
) -> GitFileContentResponse:
|
||||
"""Return a file's content at a branch tip, optionally sliced to a range.
|
||||
|
||||
Reads straight out of the branch with ``git show`` (``read_file_at_branch``)
|
||||
so a reviewer/CEO can read a finding's source lines without a workspace
|
||||
mount. A missing file or bad ref yields 404. When `line` is given the
|
||||
slice is centered on it (`line - context` .. `line + context`); explicit
|
||||
`start`/`end` override. With none of the three, the whole file returns
|
||||
(capped at 2000 lines to bound the payload — `truncated` flags the cut).
|
||||
"""
|
||||
git_service = get_git_service(db)
|
||||
|
||||
try:
|
||||
content = await git_service.read_file_at_branch(
|
||||
branch_name=branch, path=path, actor_agent_id=agent.agent_id
|
||||
)
|
||||
except _TranslatableError as e:
|
||||
raise _translate_error(e) from e
|
||||
|
||||
if content is None:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_404_NOT_FOUND,
|
||||
detail=f"File not found at {branch}:{path}",
|
||||
)
|
||||
|
||||
all_lines = content.splitlines()
|
||||
total = len(all_lines)
|
||||
|
||||
s, e_, truncated = _compute_file_range(
|
||||
total=total, line=line, context=context, start=start, end=end
|
||||
)
|
||||
|
||||
sliced = all_lines[s - 1 : e_]
|
||||
return GitFileContentResponse(
|
||||
branch=branch,
|
||||
path=path,
|
||||
content="\n".join(sliced),
|
||||
start_line=s,
|
||||
total_lines=total,
|
||||
truncated=truncated,
|
||||
)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# WRITE ENDPOINTS
|
||||
# =============================================================================
|
||||
|
||||
@@ -118,6 +118,24 @@ class GitDiffResponse(BaseModel):
|
||||
files_changed: int = 0
|
||||
|
||||
|
||||
class GitFileContentResponse(BaseModel):
|
||||
"""File content at a branch tip, optionally sliced to a line range.
|
||||
|
||||
`content` holds the requested slice; `start_line` is the 1-based number of
|
||||
its first line. `total_lines` is the file's full line count and
|
||||
`truncated` is True when the slice is shorter than the file (so the viewer
|
||||
can show "…"). Raised by `GET /git/file` for the task-detail findings
|
||||
snippet viewer.
|
||||
"""
|
||||
|
||||
branch: str
|
||||
path: str
|
||||
content: str
|
||||
start_line: int = 1
|
||||
total_lines: int = 0
|
||||
truncated: bool = False
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# COMMIT
|
||||
# =============================================================================
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
"""Unit tests for the /git/file range computation (roboco.api.routes.git).
|
||||
|
||||
Pure logic — no DB, no git. Covers the line/context windowing, explicit
|
||||
range, whole-file cap, and truncation flag.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from roboco.api.routes.git import _FILE_MAX_LINES, _compute_file_range
|
||||
|
||||
|
||||
class TestComputeFileRange:
|
||||
def test_line_centers_context_window(self) -> None:
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=100, line=50, context=10, start=None, end=None
|
||||
)
|
||||
assert (s, e_, trunc) == (40, 60, True)
|
||||
|
||||
def test_line_window_clamps_to_file_start(self) -> None:
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=100, line=3, context=10, start=None, end=None
|
||||
)
|
||||
assert (s, e_, trunc) == (1, 13, True)
|
||||
|
||||
def test_line_window_clamps_to_file_end(self) -> None:
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=100, line=98, context=10, start=None, end=None
|
||||
)
|
||||
assert (s, e_, trunc) == (88, 100, False)
|
||||
|
||||
def test_explicit_start_end_override_line(self) -> None:
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=100, line=50, context=10, start=5, end=8
|
||||
)
|
||||
assert (s, e_, trunc) == (5, 8, True)
|
||||
|
||||
def test_whole_file_when_no_range_args(self) -> None:
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=50, line=None, context=10, start=None, end=None
|
||||
)
|
||||
assert (s, e_, trunc) == (1, 50, False)
|
||||
|
||||
def test_whole_file_capped_when_huge(self) -> None:
|
||||
total = _FILE_MAX_LINES + 500
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=total, line=None, context=10, start=None, end=None
|
||||
)
|
||||
assert (s, e_, trunc) == (1, _FILE_MAX_LINES, True)
|
||||
|
||||
def test_empty_file(self) -> None:
|
||||
s, e_, trunc = _compute_file_range(
|
||||
total=0, line=None, context=10, start=None, end=None
|
||||
)
|
||||
assert (s, e_, trunc) == (1, 1, False)
|
||||
Reference in New Issue
Block a user