feat: Review & Commit View - Diff Viewer & Git Actions (#334)#391
feat: Review & Commit View - Diff Viewer & Git Actions (#334)#391
Conversation
Add full Review & Commit View with file tree, syntax-highlighted unified diff viewer, quality gate badges, commit workflow, and PR creation. Backend: - Add get_diff_stats, get_patch, generate_commit_message to core/git.py - Add /api/v2/review/diff, /patch, /commit-message endpoints Frontend: - 7 review components: FileTreePanel, DiffViewer, DiffNavigation, ReviewHeader, CommitPanel, ExportPatchModal, PRCreatedModal - Unified diff parser utility with hunk/line parsing - API client namespaces: reviewApi, gatesApi, gitApi, prApi - Review types added to shared types - Review page assembled at /review route - Review nav item enabled in sidebar Tests: - 9 backend tests for diff stats, patch export, commit message - 15 frontend tests for diff parser utility - AppSidebar test updated for enabled Review link
WalkthroughAdds backend git diff/patch/commit-message APIs, a new Review v2 router exposing them, comprehensive git-related tests, a full client-side Review page with diff parsing, multiple UI components (file tree, diff viewer, commit panel, modals), and client API/type support for review, gates, git, and PR flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as ReviewPage (Client)
participant API as /api/v2 (Router)
participant GitCore as Git Core (codeframe/core/git.py)
participant UI as Review UI Components
User->>Client: Open /review
Client->>API: GET /review/diff
API->>GitCore: get_diff_stats(workspace, staged?)
GitCore-->>API: DiffStats
API-->>Client: DiffStatsResponse
Client->>Client: parseDiff() -> diffFiles
Client->>UI: render FileTreePanel, DiffViewer, ReviewHeader, CommitPanel
Client->>API: POST /commit-message
API->>GitCore: generate_commit_message(...)
GitCore-->>API: message
API-->>Client: CommitMessageResponse
User->>Client: Export Patch
Client->>API: GET /review/patch
API->>GitCore: get_patch(...)
GitCore-->>API: patch text
API-->>Client: PatchResponse
Client->>UI: show ExportPatchModal
User->>Client: Commit -> Create PR
Client->>API: POST /git/commit /pr/create
API-->Client: CommitResult / PRResponse
Client->>UI: update state, show PRCreatedModal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Add Review & Commit page with diff viewer and git actions across backend APIs and web UIIntroduce backend diff stats, patch export, and commit message generation in 📍Where to StartStart with the Review API handlers at Macroscope summarized 23b743d. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
PR Review: feat: Review & Commit View - Diff Viewer & Git Actions (#334)This is a well-structured Phase 3 feature. The three-panel layout, component decomposition, and adherence to the thin-adapter pattern are all solid. Below are the issues I found, from most to least critical. Backend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/src/components/layout/AppSidebar.tsx (1)
19-24:⚠️ Potential issue | 🟡 MinorCentralize
NavIteminsrc/types/index.ts.Please move this interface into the shared types module and import it here.
🧭 Suggested update (component)
-import type { BlockerListResponse } from '@/types'; +import type { BlockerListResponse, NavItem } from '@/types'; ... -interface NavItem { - href: string; - label: string; - icon: React.ComponentType<{ className?: string }>; - enabled: boolean; -}Add the interface to
web-ui/src/types/index.ts.As per coding guidelines,
web-ui/src/**/*.{ts,tsx}: TypeScript types must be centralized in web-ui/src/types/index.ts; API client must be in web-ui/src/lib/api.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/layout/AppSidebar.tsx` around lines 19 - 24, Move the NavItem interface out of AppSidebar and into the central shared types module (types/index.ts), export it from that module, then import and use that exported NavItem type in AppSidebar (update the interface reference in AppSidebar.tsx). Ensure the imported NavItem preserves the same shape (href, label, icon: React.ComponentType<{ className?: string }>, enabled) and update any local references to the inline NavItem definition to use the centralized type instead.
🧹 Nitpick comments (7)
web-ui/src/components/review/ReviewHeader.tsx (1)
53-84: Consider handling additional gate statuses.The badge styling handles
PASSEDandFAILEDexplicitly, falling back to default styling for other statuses. If gates can have statuses likeRUNNINGorSKIPPED, consider adding explicit visual indicators for these states to improve UX clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/review/ReviewHeader.tsx` around lines 53 - 84, ReviewHeader currently only styles check.status 'PASSED' and 'FAILED' explicitly; update the checks.map rendering to also handle other known statuses (e.g., 'RUNNING', 'SKIPPED') by adding explicit branches for those values on the check.status conditional used for className, variant, and the icon selection inside the Badge (refer to gateResult, checks.map, Badge, check.status, and the existing PASSED/FAILED branches) so each status has a clear color/variant and an appropriate icon (e.g., spinner/clock for RUNNING, dash/skip icon for SKIPPED) instead of relying on the fallback empty styling.codeframe/core/git.py (1)
443-451: Clarify commit prefix heuristics for mixed changes.The current logic at line 446 checks
len(deleted) > len(modified)but doesn't compare againstlen(added). If a diff has 5 added, 3 deleted, and 2 modified files, it falls through to line 450 (prefix = "feat",action = "update"), which may not be the intended behavior for deletion-heavy diffs.🛠️ Suggested clarification
# Pick prefix based on dominant change type if len(added) > len(modified) and len(added) > len(deleted): prefix = "feat" action = "add" - elif len(deleted) > len(modified): + elif len(deleted) > len(modified) and len(deleted) > len(added): prefix = "refactor" action = "remove" else: prefix = "feat" action = "update"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/git.py` around lines 443 - 451, The heuristic for setting commit prefix/action incorrectly treats deletion-heavy diffs because the branch checks only compare deleted to modified; update the conditional logic around variables added, modified, and deleted in the block that assigns prefix and action (using the variables added, modified, deleted and the names prefix/action) so that the "refactor"/"remove" branch is taken only when len(deleted) is greater than both len(added) and len(modified); ensure the "add" branch remains when added is strictly largest and keep the final else as the fallback for ties or when modified is largest.web-ui/src/components/review/CommitPanel.tsx (1)
133-172: Consider resetting PR form after successful creation.After
onCreatePRcompletes successfully,prTitleandprBodyremain populated. If the parent component shows a success modal and the user returns to the form, stale data persists. Consider either:
- Clearing form state when
showPRFormis toggled off- Adding an
onSuccesscallback to reset state🛠️ Optional: Reset form when checkbox is unchecked
<Checkbox id="create-pr" checked={showPRForm} - onCheckedChange={(checked) => setShowPRForm(checked === true)} + onCheckedChange={(checked) => { + const isChecked = checked === true; + setShowPRForm(isChecked); + if (!isChecked) { + setPrTitle(''); + setPrBody(''); + } + }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/review/CommitPanel.tsx` around lines 133 - 172, The PR form retains stale values because prTitle and prBody aren’t cleared after a successful create; update CommitPanel to reset state by either (A) invoking setPrTitle('') and setPrBody('') after onCreatePR resolves (add an onSuccess handler or await the promise inside the onClick and clear on success) or (B) clearing state whenever showPRForm toggles off by adding a useEffect that watches showPRForm and calls setPrTitle('')/setPrBody('') when it becomes false; reference the onCreatePR call, the showPRForm flag, and the setPrTitle/setPrBody state setters to implement the fix.web-ui/src/app/review/page.tsx (4)
291-296: Consider more prominent error handling for diff load failures.When diff loading fails, the error message appears at the bottom while empty panels render above. Users might not notice the error. Consider displaying the error more prominently or conditionally hiding the main content on critical failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/review/page.tsx` around lines 291 - 296, The diff currently renders diffError in a small box near the bottom; change the render flow so that when diffError (the ApiError instance) is present the page returns a prominent, top-centered error state instead of rendering the normal panels. Concretely, in page.tsx detect diffError early (e.g., if (diffError) return <div role="alert" ...>...) and render a larger, high-contrast error panel (bigger text, centered, maybe sticky top) that includes (diffError as ApiError).detail || 'Unknown error'; ensure the normal content rendering paths (the panels/components below) are skipped when diffError is truthy.
23-23: Consider using named export for consistency.The coding guidelines prefer named exports over default exports. Consider changing to a named export for consistency with the codebase pattern.
-export default function ReviewPage() { +export function ReviewPage() {Note: If using Next.js App Router, the default export is required for page components, so this can be safely ignored in that context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/review/page.tsx` at line 23, Change the default export of the page component to a named export by exporting ReviewPage as a named export (export function ReviewPage or export const ReviewPage) and update any imports that reference this module to use the named import { ReviewPage }; ensure you replace the existing "export default function ReviewPage()" declaration with the named export declaration and adjust downstream import sites accordingly (keep default export only if the Next.js App Router requires it for this page).
218-218: Simplify the height calculation.The
calc(100vh-0px)is equivalent to100vh. If the intention was to reserve space for a header/nav that was removed, consider cleaning this up.- <main className="flex h-[calc(100vh-0px)] flex-col bg-background"> + <main className="flex h-screen flex-col bg-background">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/review/page.tsx` at line 218, The className on the main element uses an unnecessary calc expression; update the main element (the JSX element with className "flex h-[calc(100vh-0px)] flex-col bg-background") to use a simplified height value—replace h-[calc(100vh-0px)] with h-[100vh] (or simply h-full/100vh per your tailwind conventions) so the height calculation is cleaned up and simpler.
71-78: Silent error handling for auto-generation is acceptable but could be improved.The empty catch block silently swallows errors from auto-generating commit messages. While this is non-critical functionality, logging the error would aid debugging.
reviewApi .generateCommitMessage(workspacePath) .then((res) => setCommitMessage(res.message)) - .catch(() => {}); + .catch((err) => console.warn('Auto-generate commit message failed:', err));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/review/page.tsx` around lines 71 - 78, The empty catch in the useEffect auto-generation flow swallows errors from reviewApi.generateCommitMessage; update the catch to log the error (e.g., console.error) and include context so failures are visible during debugging. In the useEffect in page.tsx that calls reviewApi.generateCommitMessage(workspacePath) and then setCommitMessage, replace the silent .catch(() => {}) with a handler that logs the error and a short context message relating to commit message auto-generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/review/DiffViewer.tsx`:
- Around line 76-84: The current substring matching in the useEffect (checking
key.includes(selectedFile) || selectedFile.includes(key)) causes false positives
for similar file names; change to exact match or normalize paths and compare
equality (e.g., compare normalizedKey === normalizedSelected) when iterating
fileRefs.current in the useEffect that scrolls the selectedFile, and update the
isHighlighted logic (the function/variable used around lines where isHighlighted
is computed) to use the same exact/normalized equality check so only the precise
file path matches.
In `@web-ui/src/components/review/ExportPatchModal.tsx`:
- Around line 30-34: The handleCopy function should catch failures from
navigator.clipboard.writeText(patchContent) and provide user feedback: wrap the
writeText call in try/catch, only call setCopied(true) and the timeout when the
write succeeds, and on error set an error state or invoke an existing
toast/error handler to inform the user; optionally attempt a fallback copy
(e.g., document.execCommand('copy')) if clipboard API is unavailable. Locate
handleCopy and update its control flow so failures are handled and reported
rather than left as unhandled promise rejections.
In `@web-ui/src/components/review/PRCreatedModal.tsx`:
- Around line 15-20: Move the local PRCreatedModalProps interface from
PRCreatedModal.tsx into the centralized types module and import it: remove the
interface declaration in PRCreatedModal.tsx, add/export the interface in
src/types/index.ts as PRCreatedModalProps, and update PRCreatedModal.tsx to
import { PRCreatedModalProps } from "src/types"; ensure the component's props
annotation uses the imported PRCreatedModalProps and run type checks/linter to
confirm no unused-local-type remains.
In `@web-ui/src/lib/diffParser.ts`:
- Around line 8-33: The DiffHunkLine, DiffHunk, and DiffFile interfaces are
defined locally in diffParser but per guidelines must be centralized: move these
three interfaces into the shared types module and export them, then remove the
local interface definitions and import { DiffHunkLine, DiffHunk, DiffFile } into
the diff parser file (and any other files using them). Ensure the new exports in
the central types file match the original names/signatures (header, oldStart,
oldCount, newStart, newCount, lines, insertions, deletions, isNew, isDeleted,
isRenamed, etc.) and update any references/usages to use the imported types.
---
Outside diff comments:
In `@web-ui/src/components/layout/AppSidebar.tsx`:
- Around line 19-24: Move the NavItem interface out of AppSidebar and into the
central shared types module (types/index.ts), export it from that module, then
import and use that exported NavItem type in AppSidebar (update the interface
reference in AppSidebar.tsx). Ensure the imported NavItem preserves the same
shape (href, label, icon: React.ComponentType<{ className?: string }>, enabled)
and update any local references to the inline NavItem definition to use the
centralized type instead.
---
Nitpick comments:
In `@codeframe/core/git.py`:
- Around line 443-451: The heuristic for setting commit prefix/action
incorrectly treats deletion-heavy diffs because the branch checks only compare
deleted to modified; update the conditional logic around variables added,
modified, and deleted in the block that assigns prefix and action (using the
variables added, modified, deleted and the names prefix/action) so that the
"refactor"/"remove" branch is taken only when len(deleted) is greater than both
len(added) and len(modified); ensure the "add" branch remains when added is
strictly largest and keep the final else as the fallback for ties or when
modified is largest.
In `@web-ui/src/app/review/page.tsx`:
- Around line 291-296: The diff currently renders diffError in a small box near
the bottom; change the render flow so that when diffError (the ApiError
instance) is present the page returns a prominent, top-centered error state
instead of rendering the normal panels. Concretely, in page.tsx detect diffError
early (e.g., if (diffError) return <div role="alert" ...>...) and render a
larger, high-contrast error panel (bigger text, centered, maybe sticky top) that
includes (diffError as ApiError).detail || 'Unknown error'; ensure the normal
content rendering paths (the panels/components below) are skipped when diffError
is truthy.
- Line 23: Change the default export of the page component to a named export by
exporting ReviewPage as a named export (export function ReviewPage or export
const ReviewPage) and update any imports that reference this module to use the
named import { ReviewPage }; ensure you replace the existing "export default
function ReviewPage()" declaration with the named export declaration and adjust
downstream import sites accordingly (keep default export only if the Next.js App
Router requires it for this page).
- Line 218: The className on the main element uses an unnecessary calc
expression; update the main element (the JSX element with className "flex
h-[calc(100vh-0px)] flex-col bg-background") to use a simplified height
value—replace h-[calc(100vh-0px)] with h-[100vh] (or simply h-full/100vh per
your tailwind conventions) so the height calculation is cleaned up and simpler.
- Around line 71-78: The empty catch in the useEffect auto-generation flow
swallows errors from reviewApi.generateCommitMessage; update the catch to log
the error (e.g., console.error) and include context so failures are visible
during debugging. In the useEffect in page.tsx that calls
reviewApi.generateCommitMessage(workspacePath) and then setCommitMessage,
replace the silent .catch(() => {}) with a handler that logs the error and a
short context message relating to commit message auto-generation.
In `@web-ui/src/components/review/CommitPanel.tsx`:
- Around line 133-172: The PR form retains stale values because prTitle and
prBody aren’t cleared after a successful create; update CommitPanel to reset
state by either (A) invoking setPrTitle('') and setPrBody('') after onCreatePR
resolves (add an onSuccess handler or await the promise inside the onClick and
clear on success) or (B) clearing state whenever showPRForm toggles off by
adding a useEffect that watches showPRForm and calls
setPrTitle('')/setPrBody('') when it becomes false; reference the onCreatePR
call, the showPRForm flag, and the setPrTitle/setPrBody state setters to
implement the fix.
In `@web-ui/src/components/review/ReviewHeader.tsx`:
- Around line 53-84: ReviewHeader currently only styles check.status 'PASSED'
and 'FAILED' explicitly; update the checks.map rendering to also handle other
known statuses (e.g., 'RUNNING', 'SKIPPED') by adding explicit branches for
those values on the check.status conditional used for className, variant, and
the icon selection inside the Badge (refer to gateResult, checks.map, Badge,
check.status, and the existing PASSED/FAILED branches) so each status has a
clear color/variant and an appropriate icon (e.g., spinner/clock for RUNNING,
dash/skip icon for SKIPPED) instead of relying on the fallback empty styling.
| interface PRCreatedModalProps { | ||
| open: boolean; | ||
| onClose: () => void; | ||
| prUrl: string; | ||
| prNumber: number; | ||
| } |
There was a problem hiding this comment.
Centralize PRCreatedModalProps in src/types/index.ts.
Move the props interface to the centralized types module and import it here to comply with the project type-location rule.
🧭 Suggested update (component)
-import { useCallback } from 'react';
+import { useCallback } from 'react';
+import type { PRCreatedModalProps } from '@/types';
import { CheckmarkCircle01Icon } from '@hugeicons/react';
...
-interface PRCreatedModalProps {
- open: boolean;
- onClose: () => void;
- prUrl: string;
- prNumber: number;
-}Add this interface to web-ui/src/types/index.ts:
export interface PRCreatedModalProps {
open: boolean;
onClose: () => void;
prUrl: string;
prNumber: number;
}As per coding guidelines, web-ui/src/**/*.{ts,tsx}: TypeScript types must be centralized in web-ui/src/types/index.ts; API client must be in web-ui/src/lib/api.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/review/PRCreatedModal.tsx` around lines 15 - 20, Move
the local PRCreatedModalProps interface from PRCreatedModal.tsx into the
centralized types module and import it: remove the interface declaration in
PRCreatedModal.tsx, add/export the interface in src/types/index.ts as
PRCreatedModalProps, and update PRCreatedModal.tsx to import {
PRCreatedModalProps } from "src/types"; ensure the component's props annotation
uses the imported PRCreatedModalProps and run type checks/linter to confirm no
unused-local-type remains.
| export interface DiffHunkLine { | ||
| type: 'addition' | 'deletion' | 'context' | 'header'; | ||
| content: string; | ||
| oldLineNumber: number | null; | ||
| newLineNumber: number | null; | ||
| } | ||
|
|
||
| export interface DiffHunk { | ||
| header: string; | ||
| oldStart: number; | ||
| oldCount: number; | ||
| newStart: number; | ||
| newCount: number; | ||
| lines: DiffHunkLine[]; | ||
| } | ||
|
|
||
| export interface DiffFile { | ||
| oldPath: string; | ||
| newPath: string; | ||
| hunks: DiffHunk[]; | ||
| insertions: number; | ||
| deletions: number; | ||
| isNew: boolean; | ||
| isDeleted: boolean; | ||
| isRenamed: boolean; | ||
| } |
There was a problem hiding this comment.
Move diff parser interfaces to centralized types.
DiffHunkLine, DiffHunk, and DiffFile should live in src/types/index.ts and be imported here.
🧭 Suggested update (diffParser)
-export interface DiffHunkLine {
- type: 'addition' | 'deletion' | 'context' | 'header';
- content: string;
- oldLineNumber: number | null;
- newLineNumber: number | null;
-}
-
-export interface DiffHunk {
- header: string;
- oldStart: number;
- oldCount: number;
- newStart: number;
- newCount: number;
- lines: DiffHunkLine[];
-}
-
-export interface DiffFile {
- oldPath: string;
- newPath: string;
- hunks: DiffHunk[];
- insertions: number;
- deletions: number;
- isNew: boolean;
- isDeleted: boolean;
- isRenamed: boolean;
-}
+import type { DiffHunkLine, DiffHunk, DiffFile } from '@/types';Add these interfaces to web-ui/src/types/index.ts.
As per coding guidelines, web-ui/src/**/*.{ts,tsx}: TypeScript types must be centralized in web-ui/src/types/index.ts; API client must be in web-ui/src/lib/api.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/lib/diffParser.ts` around lines 8 - 33, The DiffHunkLine,
DiffHunk, and DiffFile interfaces are defined locally in diffParser but per
guidelines must be centralized: move these three interfaces into the shared
types module and export them, then remove the local interface definitions and
import { DiffHunkLine, DiffHunk, DiffFile } into the diff parser file (and any
other files using them). Ensure the new exports in the central types file match
the original names/signatures (header, oldStart, oldCount, newStart, newCount,
lines, insertions, deletions, isNew, isDeleted, isRenamed, etc.) and update any
references/usages to use the imported types.
- Clamp selectedFileIndex when diffFiles shrinks after commit - Use ref to prevent auto-generate from re-triggering on clear - Use exact path matching in DiffViewer file scroll - Add clipboard API fallback for non-HTTPS contexts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web-ui/src/app/review/page.tsx (2)
55-69: Consider aligning SWR key with fetcher pattern.The SWR key is a URL string but the fetcher ignores it and uses
workspacePath!directly. While functional, the conventional pattern is either:
- Use a simple key and pass params to the fetcher, or
- Have the fetcher use the key
This is a minor inconsistency that doesn't affect functionality.
♻️ Optional: align key and fetcher
const { data: diffData, error: diffError, mutate: mutateDiff, } = useSWR<DiffStatsResponse>( - workspacePath ? `/api/v2/review/diff?path=${workspacePath}` : null, - () => reviewApi.getDiff(workspacePath!) + workspacePath ? ['review-diff', workspacePath] : null, + ([, path]) => reviewApi.getDiff(path) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/review/page.tsx` around lines 55 - 69, The SWR key string and the fetcher are inconsistent: useSWR currently uses a URL string key while the fetcher ignores that key and reads workspacePath directly; update the call so the key and fetcher align — for example, make the key an array like workspacePath ? ['/api/v2/review/diff', workspacePath] : null and change the fetcher to accept the key tuple and call reviewApi.getDiff(path) (or alternatively make the fetcher derive workspacePath from the single-string key); adjust the useSWR invocation and the fetcher signature accordingly (references: useSWR, reviewApi.getDiff, workspacePath).
231-232: Redundant calculation in height class.
h-[calc(100vh-0px)]is equivalent toh-screen. Consider simplifying.♻️ Suggested simplification
- <main className="flex h-[calc(100vh-0px)] flex-col bg-background"> + <main className="flex h-screen flex-col bg-background">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/review/page.tsx` around lines 231 - 232, The className on the main element uses a redundant height calc—replace the h-[calc(100vh-0px)] utility with the simpler Tailwind h-screen to achieve the same full-viewport height; update the JSX in the return (the <main className="..."> element in page.tsx) to remove the calc variant and use h-screen alongside the existing flex, flex-col, and bg-background classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/app/review/page.tsx`:
- Around line 184-193: handleFileSelect uses substring matching via
getFilePath(f).includes(filePath) || filePath.includes(getFilePath(f)), which
can pick the wrong diffFiles entry for similarly named files; change the lookup
in handleFileSelect to an exact match against getFilePath(f) (or a normalized
exact path comparison) when computing idx so setSelectedFileIndex points to the
correct file, leaving setSelectedFile unchanged.
- Around line 260-274: The code may access diffFiles[selectedFileIndex]
out-of-bounds if selectedFileIndex is stale when diffFiles shrinks; update the
currentFileName prop passed to DiffNavigation to guard against invalid indices
by computing a safe index (e.g. clamp selectedFileIndex to the range
0..diffFiles.length-1) or by conditionally passing an empty string when
selectedFileIndex is outside bounds; reference symbols: diffFiles,
selectedFileIndex, getFilePath, and DiffNavigation (keep
handlePrevFile/handleNextFile unchanged).
---
Duplicate comments:
In `@web-ui/src/components/review/DiffViewer.tsx`:
- Around line 120-122: The highlight logic in DiffViewer.tsx uses substring
matching causing false positives; update the isHighlighted calculation to use
exact equality between key and selectedFile (replace the current
key.includes/selectedFile.includes checks) so it matches the scroll behavior
(key === selectedFile) and consistently highlights only the selected file.
---
Nitpick comments:
In `@web-ui/src/app/review/page.tsx`:
- Around line 55-69: The SWR key string and the fetcher are inconsistent: useSWR
currently uses a URL string key while the fetcher ignores that key and reads
workspacePath directly; update the call so the key and fetcher align — for
example, make the key an array like workspacePath ? ['/api/v2/review/diff',
workspacePath] : null and change the fetcher to accept the key tuple and call
reviewApi.getDiff(path) (or alternatively make the fetcher derive workspacePath
from the single-string key); adjust the useSWR invocation and the fetcher
signature accordingly (references: useSWR, reviewApi.getDiff, workspacePath).
- Around line 231-232: The className on the main element uses a redundant height
calc—replace the h-[calc(100vh-0px)] utility with the simpler Tailwind h-screen
to achieve the same full-viewport height; update the JSX in the return (the
<main className="..."> element in page.tsx) to remove the calc variant and use
h-screen alongside the existing flex, flex-col, and bg-background classes.
| const handleFileSelect = useCallback( | ||
| (filePath: string) => { | ||
| setSelectedFile(filePath); | ||
| const idx = diffFiles.findIndex( | ||
| (f) => getFilePath(f).includes(filePath) || filePath.includes(getFilePath(f)) | ||
| ); | ||
| if (idx >= 0) setSelectedFileIndex(idx); | ||
| }, | ||
| [diffFiles] | ||
| ); |
There was a problem hiding this comment.
Substring matching in handleFileSelect can cause incorrect index selection.
Similar to the DiffViewer issue, the findIndex logic uses substring matching which can produce false positives with similar file names.
🛠️ Suggested fix: use exact matching
const handleFileSelect = useCallback(
(filePath: string) => {
setSelectedFile(filePath);
const idx = diffFiles.findIndex(
- (f) => getFilePath(f).includes(filePath) || filePath.includes(getFilePath(f))
+ (f) => getFilePath(f) === filePath
);
if (idx >= 0) setSelectedFileIndex(idx);
},
[diffFiles]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/review/page.tsx` around lines 184 - 193, handleFileSelect uses
substring matching via getFilePath(f).includes(filePath) ||
filePath.includes(getFilePath(f)), which can pick the wrong diffFiles entry for
similarly named files; change the lookup in handleFileSelect to an exact match
against getFilePath(f) (or a normalized exact path comparison) when computing
idx so setSelectedFileIndex points to the correct file, leaving setSelectedFile
unchanged.
| {diffFiles.length > 0 && ( | ||
| <div className="shrink-0 px-4 pt-2"> | ||
| <DiffNavigation | ||
| currentFileIndex={selectedFileIndex} | ||
| totalFiles={diffFiles.length} | ||
| currentFileName={ | ||
| diffFiles.length > 0 | ||
| ? getFilePath(diffFiles[selectedFileIndex]) | ||
| : '' | ||
| } | ||
| onPrevious={handlePrevFile} | ||
| onNext={handleNextFile} | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Potential out-of-bounds access in navigation filename.
The condition checks diffFiles.length > 0 but accesses diffFiles[selectedFileIndex]. If selectedFileIndex hasn't been clamped yet after diffFiles shrinks, this could access an invalid index.
🛠️ Suggested fix: add bounds check
<DiffNavigation
currentFileIndex={selectedFileIndex}
totalFiles={diffFiles.length}
currentFileName={
- diffFiles.length > 0
+ diffFiles.length > 0 && selectedFileIndex < diffFiles.length
? getFilePath(diffFiles[selectedFileIndex])
: ''
}
onPrevious={handlePrevFile}
onNext={handleNextFile}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/review/page.tsx` around lines 260 - 274, The code may access
diffFiles[selectedFileIndex] out-of-bounds if selectedFileIndex is stale when
diffFiles shrinks; update the currentFileName prop passed to DiffNavigation to
guard against invalid indices by computing a safe index (e.g. clamp
selectedFileIndex to the range 0..diffFiles.length-1) or by conditionally
passing an empty string when selectedFileIndex is outside bounds; reference
symbols: diffFiles, selectedFileIndex, getFilePath, and DiffNavigation (keep
handlePrevFile/handleNextFile unchanged).
|
Test comment - please ignore |
1 participant
Summary
Implements #334: [Phase 3] Review & Commit View - Diff Viewer & Git Actions
Backend
get_diff_stats,get_patch,generate_commit_messagetocore/git.pyreview_v2.py:/api/v2/review/diff,/patch,/commit-messageFrontend
FileTreePanel,DiffViewer,DiffNavigation,ReviewHeader,CommitPanel,ExportPatchModal,PRCreatedModaldiffParser.ts) with hunk/line parsingreviewApi,gatesApi,gitApi,prApi/reviewroute with three-panel layoutTests
Acceptance Criteria
Test Plan
uv run pytest tests/core/test_git_review.py)/reviewrouteCloses #334
Summary by CodeRabbit
New Features
Tests