Open
Conversation
|
@yuzhichang is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded an AI Review feature: a modal UI to configure depth/provider, orchestration code to launch review agents and PTYs, message polling and parsing, result aggregation, modal registration, UI wiring, session/terminal support, and related infrastructure/error-handling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Modal as AI Review Modal
participant Renderer as Renderer Process
participant RPC as Database / RPC
participant MainProc as Main Process (PTY)
participant Agent as Review Agent (PTY)
User->>Modal: Configure review (depth, provider) and Start
Modal->>Renderer: launchReviewAgents(config, taskId, taskPath)
loop For each agent
Renderer->>RPC: rpc.db.createConversation(metadata)
RPC-->>Renderer: conversationId
Renderer->>MainProc: ptyStartDirect({conversationId, prompt, provider})
MainProc->>Agent: initialize PTY session
Agent-->>MainProc: pty started
MainProc-->>Renderer: ptyId
rect rgba(200,150,100,0.5)
loop Polling
Renderer->>RPC: rpc.db.getMessages(conversationId, since)
RPC-->>Renderer: messages
end
end
end
Renderer->>Renderer: parseReviewMessages(allMessages)
Renderer->>Renderer: aggregateReviewResults(parsedPerAgent)
Renderer-->>Modal: AIReviewResult { issues, summary, agentIds }
Modal-->>User: Show review started/completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/shared/reviewPreset.ts (1)
62-118: Review prompts don't specify output format, leading to unpredictable parsing.The prompts instruct "Provide your review in a structured format" but don't specify JSON schema or expected structure. This forces
aiReview.tsto implement fragile fallback parsing (JSON → markdown). Consider adding explicit format instructions:♻️ Example: Add JSON schema instruction to prompts
comprehensive: `You are an expert code reviewer conducting a comprehensive review. Review the following file changes for: - All correctness issues including edge cases ... -Provide your review in a structured format with specific issues found, including severity and category.`, +Provide your review as a JSON array of issues with this schema: +[{ "severity": "critical|major|minor|info", "category": "string", "title": "string", "description": "string", "filePath": "string?", "lineRange": {"start": number, "end": number}?, "codeSnapshot": "string?", "fixPrompt": "string?" }]`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/reviewPreset.ts` around lines 62 - 118, Update the REVIEW_PROMPTS constant so each prompt variant (fileChanges.quick/focused/comprehensive and agentOutput.quick/focused/comprehensive) explicitly requires a machine-readable JSON output schema; change the prompt text to append a short JSON schema specification (e.g., an array of issue objects with fields like "id", "severity", "category", "location", "description", "suggestion") and a one-line instruction "Return only valid JSON" to avoid freeform text; ensure both fileChanges and agentOutput templates include the same schema and fields so downstream parsing in aiReview.ts can rely on a stable structure.src/renderer/lib/aiReview.ts (2)
124-135: Sequential agent launches could be parallelized.Launching agents sequentially with
for...ofandawaitadds unnecessary latency when depth is "focused" (3 agents) or "comprehensive" (5 agents). Consider usingPromise.allfor parallel startup.♻️ Parallel agent launch
- for (let i = 0; i < agentCount; i++) { - const prompt = buildReviewPrompt(config.reviewType, config.depth, content); - const { conversationId, ptyId } = await launchReviewAgent({ - taskId, - taskPath, - reviewId, - agent: config.providerId, - prompt, - }); - conversationIds.push(conversationId); - ptyIds.push(ptyId); - } + const prompt = buildReviewPrompt(config.reviewType, config.depth, content); + const launches = Array.from({ length: agentCount }, () => + launchReviewAgent({ + taskId, + taskPath, + reviewId, + agent: config.providerId, + prompt, + }) + ); + const results = await Promise.all(launches); + for (const { conversationId, ptyId } of results) { + conversationIds.push(conversationId); + ptyIds.push(ptyId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/aiReview.ts` around lines 124 - 135, The loop that calls buildReviewPrompt and await launchReviewAgent (using agentCount and config) runs agents sequentially; change it to start all agent launches in parallel by creating an array of launch promises (e.g., map over indices to call buildReviewPrompt and launchReviewAgent) and await Promise.all on that array, then extract conversationId and ptyId from the resolved results to populate conversationIds and ptyIds in the same order; ensure any thrown errors are handled or propagated appropriately (refer to buildReviewPrompt, launchReviewAgent, conversationIds, ptyIds, agentCount, and config).
208-284: Add test coverage for the markdown parsing function, particularly for edge cases with flexible regex patterns.The
parseMarkdownIssuesfunction parses variable markdown formats using multiple regex patterns with optional groups (e.g.,/\[?(\w+)\]?/,/[:\-]?/). No test coverage currently exists for this critical parser. Consider adding tests for edge cases such as malformed headers, boundary conditions in code block handling, and variations in file path and category formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/aiReview.ts` around lines 208 - 284, Add unit tests for parseMarkdownIssues covering edge cases of its flexible regexes and code-block handling: write tests that feed malformed headers (e.g., missing brackets, extra punctuation), headers with different casing/spacing that exercise headerMatch and bulletMatch, and ensure normalizeSeverity and generateId behavior is controlled (mock or stub generateId for deterministic assertions). Include tests for code-fence boundaries (adjacent fences, empty code blocks, indented code vs fenced code), variations of File:/Path:/Location: and Category:/Type: lines parsing into filePath and category, and multi-issue inputs to assert currentCodeBlock and description are correctly accumulated and pushed as AIReviewIssue objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AIReviewConfigModal.tsx`:
- Around line 96-109: The results modal is opened with
showModal('aiReviewResultsModal') but no onFixIssue handler is provided, so
AIReviewResultsModal's "Fix" button is a no-op; update the showModal call (and
the similar showResultsModal helper) to pass an onFixIssue callback that either
reopens the aiReviewConfigModal with the selected issue context or dispatches
the existing fix flow: implement onFixIssue = (issue) =>
showModal('aiReviewConfigModal', { taskId, taskPath, availableAgents,
installedAgents, issue, onSuccess: () => {} }) (or call the shared fix routine
used elsewhere) so the button becomes functional and receives the issue payload.
- Around line 143-149: The current heuristic in the loop that sets hasResponses
(iterating conversationIds, calling pollReviewMessages, then checking
messages.some(m => m.sender === 'agent' && m.content.length > 50)) is brittle;
replace the length check with a structural/completion check: for each message
from sender 'agent' validate the content by either attempting to parse JSON and
confirm expected fields (e.g., checking parsed.content.response, parsed.result,
or a "status":"completed" marker) or by looking for explicit completion
markers/phrases (e.g., "Response:", "### End", or absence of error indicators
like "error"/"traceback"); update the condition used in the messages.some(...)
call to use this validator so hasResponses is true only for messages that match
the structured/completion criteria rather than a raw length check.
- Around line 283-294: In AIReviewConfigModal, the AgentDropdown can be empty
because the current fallback (availableAgents.length > 0 ? availableAgents :
installedAgents) doesn’t handle both arrays being empty; update the render for
the 'agent-output' branch to detect when both availableAgents and
installedAgents are empty and show a user-facing message (e.g., "No agents
available — install or enable an agent") instead of the dropdown; if you prefer
keeping the dropdown, pass an empty list and disable it and ensure
selectedAgentId is cleared or set to null to avoid invalid selection. Target the
JSX around AgentDropdown, the availableAgents/installedAgents check, and any
state setting for selectedAgentId to implement this behavior.
- Around line 120-164: pollForResults currently schedules recursive setTimeout
calls and calls showResultsModal even after the component unmounts; change
polling to be cancellable by tracking mounted/cancel state (e.g., an isMounted
or isCancelled ref or an AbortController-like flag) and check it before any
state/modal calls and before scheduling the next setTimeout, also store and
clear the timeout handle (from setTimeout) on unmount so pending timers are
cleared; update functions pollForResults, the setTimeout callback, and any
useEffect that starts polling to respect the cancel flag and avoid calling
showResultsModal or collectResults when cancelled.
In `@src/renderer/components/AIReviewResultsModal.tsx`:
- Around line 272-276: The "Run Another Review" Button in AIReviewResultsModal
calls the optional prop onRunAnotherReview directly which can throw if
undefined; update the Button's onClick to guard the call (use optional chaining
or a no-op fallback) so it only invokes onRunAnotherReview when defined (e.g.,
onClick={() => onRunAnotherReview?.()}), and ensure any TypeScript/JSX
references to onRunAnotherReview are adjusted accordingly to avoid runtime
errors.
- Around line 180-186: sortedIssues is not being filtered by the current
activeTab, so the issue list always shows all issues; update the logic that
computes or supplies sortedIssues (used in the map inside AIReviewResultsModal
where sortedIssues.map(...) and activeTab/expandedIssues are referenced) to
apply a filter: when activeTab is the "All Issues" value keep all issues,
otherwise restrict to issues whose agent identifier matches activeTab (e.g.,
issue.agentId or the property your issue objects use for agent ownership).
Ensure the filtered array is then sorted the same way as before and used in the
existing map so selecting an agent tab shows only that agent's issues.
In `@src/renderer/components/FileChangesPanel.tsx`:
- Around line 265-276: The modal is being opened with installedAgents: [] which
causes AgentDropdown to render no options; update handleOpenAIReview so it
supplies the real installed agents instead of an empty array — obtain them
either by adding an installedAgents prop to FileChangesPanel (and passing it
from the parent RightSidebar) or derive them inside FileChangesPanel via the app
settings hook/context (e.g., useAppSettings or the existing app context) and
then call showModal('aiReviewConfigModal', { taskId: resolvedTaskId, taskPath:
safeTaskPath, installedAgents: installedAgents, onSuccess: ... }). Ensure you
reference the handleOpenAIReview function and the showModal call when modifying
the code so AgentDropdown receives the correct installedAgents set.
In `@src/renderer/lib/aiReview.ts`:
- Line 14: The renderer code imports the Message type from main process code
(Message in aiReview.ts), creating an unwanted cross-boundary dependency; fix by
extracting or re-exporting the Message interface into a shared types module and
update the import in aiReview.ts to import Message from that shared module (or
alternatively declare the IPC/renderer typing for Message in your renderer
declaration file such as electron-api.d.ts) so the renderer no longer imports
from main process sources.
---
Nitpick comments:
In `@src/renderer/lib/aiReview.ts`:
- Around line 124-135: The loop that calls buildReviewPrompt and await
launchReviewAgent (using agentCount and config) runs agents sequentially; change
it to start all agent launches in parallel by creating an array of launch
promises (e.g., map over indices to call buildReviewPrompt and
launchReviewAgent) and await Promise.all on that array, then extract
conversationId and ptyId from the resolved results to populate conversationIds
and ptyIds in the same order; ensure any thrown errors are handled or propagated
appropriately (refer to buildReviewPrompt, launchReviewAgent, conversationIds,
ptyIds, agentCount, and config).
- Around line 208-284: Add unit tests for parseMarkdownIssues covering edge
cases of its flexible regexes and code-block handling: write tests that feed
malformed headers (e.g., missing brackets, extra punctuation), headers with
different casing/spacing that exercise headerMatch and bulletMatch, and ensure
normalizeSeverity and generateId behavior is controlled (mock or stub generateId
for deterministic assertions). Include tests for code-fence boundaries (adjacent
fences, empty code blocks, indented code vs fenced code), variations of
File:/Path:/Location: and Category:/Type: lines parsing into filePath and
category, and multi-issue inputs to assert currentCodeBlock and description are
correctly accumulated and pushed as AIReviewIssue objects.
In `@src/shared/reviewPreset.ts`:
- Around line 62-118: Update the REVIEW_PROMPTS constant so each prompt variant
(fileChanges.quick/focused/comprehensive and
agentOutput.quick/focused/comprehensive) explicitly requires a machine-readable
JSON output schema; change the prompt text to append a short JSON schema
specification (e.g., an array of issue objects with fields like "id",
"severity", "category", "location", "description", "suggestion") and a one-line
instruction "Return only valid JSON" to avoid freeform text; ensure both
fileChanges and agentOutput templates include the same schema and fields so
downstream parsing in aiReview.ts can rely on a stable structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26ef21c0-39e3-4447-86b4-9f979f8f5395
📒 Files selected for processing (6)
src/renderer/components/AIReviewConfigModal.tsxsrc/renderer/components/AIReviewResultsModal.tsxsrc/renderer/components/FileChangesPanel.tsxsrc/renderer/contexts/ModalProvider.tsxsrc/renderer/lib/aiReview.tssrc/shared/reviewPreset.ts
yuzhichang
force-pushed
the
emdash/ai-review-1m8
branch
from
4cfb22b to
16e4d96
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/sidebar/LeftSidebar.tsx (1)
473-482:⚠️ Potential issue | 🔴 CriticalProject collapsibles are locked closed; users cannot manually expand them.
At Line 473,
open={forceOpenIds.has(typedProject.id)}fully controls the collapsible state. However,onOpenChange(Lines 474–482) only removes IDs and never adds them, leaving projects unexpandable unless automatically opened by the task-count effect (Lines 314–320). This is a regression; the code previously usedopen={forceOpenIds.has(typedProject.id) ? true : undefined}to allow uncontrolled mode for items not inforceOpenIds, enabling user toggles.Restore the ternary pattern to re-enable user-controlled toggles:
Fix
- open={forceOpenIds.has(typedProject.id)} + open={forceOpenIds.has(typedProject.id) ? true : undefined}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sidebar/LeftSidebar.tsx` around lines 473 - 482, The collapsible for projects is being forced closed because open is set to open={forceOpenIds.has(typedProject.id)}, which prevents user toggles; change it back to use the ternary pattern so items not in forceOpenIds become uncontrolled: set open to forceOpenIds.has(typedProject.id) ? true : undefined and keep the existing onOpenChange logic (which already removes IDs from forceOpenIds) so manually expanding/collapsing works again for projects outside the forced-open set (references: forceOpenIds, typedProject.id, open prop, onOpenChange).src/main/services/ptyManager.ts (1)
502-510:⚠️ Potential issue | 🔴 CriticalFix encoding pattern inconsistency in Claude session file paths.
Line 504 in
claudeSessionFileExistsusescwd.replace(/[^a-zA-Z0-9]/g, '-')while line 525 indiscoverExistingClaudeSessionusescwd.replace(/[:\\/]/g, '-'). These patterns will encode the same path differently, causing session discovery to fail.Example:
cwd="/home/user/my project"
- Line 504 encodes to:
-home-user-my-project(replaces space)- Line 525 encodes to:
-home-user-my project(preserves space)Update line 525 to use the same pattern as line 504:
Diff
function discoverExistingClaudeSession(cwd: string, excludeUuids: Set<string>): string | null { try { - const encoded = cwd.replace(/[:\\/]/g, '-'); + const encoded = cwd.replace(/[^a-zA-Z0-9]/g, '-'); const projectDir = path.join(os.homedir(), '.claude', 'projects', encoded);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/ptyManager.ts` around lines 502 - 510, The encoding regex used for cwd is inconsistent between claudeSessionFileExists and discoverExistingClaudeSession causing mismatched filenames; update discoverExistingClaudeSession to use the same replacement pattern as claudeSessionFileExists (i.e., replace all non-alphanumeric chars with '-') so both functions produce identical encoded directory names (locate the cwd.replace(...) call inside discoverExistingClaudeSession and change its regex to the one used in claudeSessionFileExists).
🧹 Nitpick comments (1)
src/renderer/lib/aiReview.ts (1)
139-150: Sequential agent launch with identical prompts may be redundant.All agents receive the same prompt built at line 140. Launching multiple agents with identical prompts to review the same content seems redundant—they'll likely produce similar results.
Consider either:
- Varying prompts per agent (e.g., different focus areas for comprehensive review)
- Using parallel launch with
Promise.allif agents should run concurrently- Documenting that redundancy is intentional for consensus/voting purposes
♻️ Example: Parallel launch (if agents are independent)
- for (let i = 0; i < agentCount; i++) { - const prompt = buildReviewPrompt(config.reviewType, config.depth, content); - const { conversationId, ptyId } = await launchReviewAgent({ - taskId, - taskPath, - reviewId, - agent: config.providerId, - prompt, - }); - conversationIds.push(conversationId); - ptyIds.push(ptyId); - } + const prompt = buildReviewPrompt(config.reviewType, config.depth, content); + const launches = await Promise.all( + Array.from({ length: agentCount }, () => + launchReviewAgent({ + taskId, + taskPath, + reviewId, + agent: config.providerId, + prompt, + }) + ) + ); + + for (const { conversationId, ptyId } of launches) { + conversationIds.push(conversationId); + ptyIds.push(ptyId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/aiReview.ts` around lines 139 - 150, The current loop builds the same prompt once per agent and launches agents sequentially, causing redundant identical reviews; update the logic to (1) generate distinct prompts per agent (e.g., extend buildReviewPrompt to accept an agent index or a focusArea list derived from config and call buildReviewPrompt(config.reviewType, config.depth, content, i) to vary each prompt) and (2) launch agents in parallel using Promise.all over an array of launchReviewAgent calls so you await all {conversationId, ptyId} results together and then push their ids into conversationIds and ptyIds; alternatively, if identical prompts are intentional, add a short comment documenting consensus/voting purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AIReviewConfigModal.tsx`:
- Around line 80-85: The dropdown is storing task terminal IDs in the
{taskId}::term::... format but captureAgentOutput expects PTY IDs like
{providerId}-{kind}-{id}, so snapshot capture fails; fix by handling terminal
selections specially: when building opts in the taskTerminals loop (the code
that pushes { id: term.id, name: ... }), also include and store the terminal's
PTY identifier (e.g., add a ptyId or value field populated from the terminal
object or constructed as {providerId}-{kind}-{id}) and ensure the selection
component returns that PTY ID to the backend, or alternatively update
captureAgentOutput (the check at aiReview.ts:52-54) to detect the ::term::
pattern, look up the corresponding terminal entry (from taskTerminalsStore) and
translate it to the expected PTY ID before proceeding.
In `@src/renderer/lib/aiReview.ts`:
- Around line 46-56: The current captureAgentOutput uses fragile substring
checks to decide if agentOrPtyId is already a ptyId; replace that with explicit
ID-type discrimination: add/inline a helper like isPtyId/isTerminalPtyId that
returns true for known pty ID patterns (e.g. terminal store IDs containing
'::term::' and structured pty IDs matching a stricter regex like
/^.+-(main|chat)-.+$/) and false for provider IDs, then in captureAgentOutput
use that helper to either pass agentOrPtyId through as ptyId or call
makePtyId(agentOrPtyId as ProviderId, kind, taskId); reference function
captureAgentOutput and makePtyId so reviewers can find and update the logic.
- Around line 268-278: The parser fails to track code block state because it
only checks currentCodeBlock.length; add a boolean flag (e.g., inCodeBlock) and
update the fence handling in the code that processes lines: when encountering a
``` fence, toggle inCodeBlock (set true when starting, false when ending), and
on end set currentIssue.codeSnapshot = currentCodeBlock.join('\n') and reset
currentCodeBlock; then replace checks of currentCodeBlock.length (and the
indentation-based push) with checks against inCodeBlock so lines inside fences
are collected via currentCodeBlock.push(line) while inCodeBlock is true; ensure
currentIssue and currentCodeBlock handling remains the same when starting/ending
blocks.
---
Outside diff comments:
In `@src/main/services/ptyManager.ts`:
- Around line 502-510: The encoding regex used for cwd is inconsistent between
claudeSessionFileExists and discoverExistingClaudeSession causing mismatched
filenames; update discoverExistingClaudeSession to use the same replacement
pattern as claudeSessionFileExists (i.e., replace all non-alphanumeric chars
with '-') so both functions produce identical encoded directory names (locate
the cwd.replace(...) call inside discoverExistingClaudeSession and change its
regex to the one used in claudeSessionFileExists).
In `@src/renderer/components/sidebar/LeftSidebar.tsx`:
- Around line 473-482: The collapsible for projects is being forced closed
because open is set to open={forceOpenIds.has(typedProject.id)}, which prevents
user toggles; change it back to use the ternary pattern so items not in
forceOpenIds become uncontrolled: set open to forceOpenIds.has(typedProject.id)
? true : undefined and keep the existing onOpenChange logic (which already
removes IDs from forceOpenIds) so manually expanding/collapsing works again for
projects outside the forced-open set (references: forceOpenIds, typedProject.id,
open prop, onOpenChange).
---
Nitpick comments:
In `@src/renderer/lib/aiReview.ts`:
- Around line 139-150: The current loop builds the same prompt once per agent
and launches agents sequentially, causing redundant identical reviews; update
the logic to (1) generate distinct prompts per agent (e.g., extend
buildReviewPrompt to accept an agent index or a focusArea list derived from
config and call buildReviewPrompt(config.reviewType, config.depth, content, i)
to vary each prompt) and (2) launch agents in parallel using Promise.all over an
array of launchReviewAgent calls so you await all {conversationId, ptyId}
results together and then push their ids into conversationIds and ptyIds;
alternatively, if identical prompts are intentional, add a short comment
documenting consensus/voting purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0da32db0-9075-46d6-b843-687ecdab8f04
📒 Files selected for processing (11)
package.jsonsrc/main/services/ptyManager.tssrc/main/services/ssh/SshService.tssrc/renderer/components/AIReviewConfigModal.tsxsrc/renderer/components/AIReviewResultsModal.tsxsrc/renderer/components/FileChangesPanel.tsxsrc/renderer/components/sidebar/LeftSidebar.tsxsrc/renderer/contexts/ModalProvider.tsxsrc/renderer/lib/aiReview.tssrc/renderer/terminal/TerminalSessionManager.tssrc/shared/reviewPreset.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/contexts/ModalProvider.tsx
- src/renderer/components/AIReviewResultsModal.tsx
yuzhichang
force-pushed
the
emdash/ai-review-1m8
branch
from
16e4d96 to
60a8bc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/renderer/lib/aiReview.ts (1)
249-268:⚠️ Potential issue | 🟡 MinorTrack fenced-code state explicitly.
The opening fence on Line 250 never records that the parser is inside a code block; it only starts collecting once
currentCodeBlock.length > 0. Unindented lines inside fenced blocks are therefore dropped, socodeSnapshotcan end up empty or truncated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/aiReview.ts` around lines 249 - 268, The parser fails to track fenced-code state because it only begins collecting when currentCodeBlock has content; update the logic around the line.startsWith('```') fence handling to use an explicit boolean (e.g., inCodeBlock) that flips true on an opening fence and false on a closing fence, assign currentIssue.codeSnapshot when closing the fence (as currently done), and when inCodeBlock push every line into currentCodeBlock regardless of indentation (remove the dependency on line.match(/^\s{2,}/) for fenced blocks) while still skipping the fence line itself; reference the symbols currentCodeBlock, currentIssue, line.startsWith('```'), and codeSnapshot when making these changes.src/renderer/components/AIReviewConfigModal.tsx (1)
39-52:⚠️ Potential issue | 🟠 MajorDon't fake an installed-provider list.
Per the provided PR context,
FileChangesPanelopens this modal withinstalledAgents: [], so Lines 48-52 make every entry inagentConfigselectable while Line 40 still defaults to'claude'. Users can therefore start a review with a CLI that is not actually installed and only discover it after launch begins. Keep the list empty until real provider status is loaded, render a disabled empty state, and derive the default from the first installed agent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AIReviewConfigModal.tsx` around lines 39 - 52, The current logic fakes an installed list by falling back to Object.keys(agentConfig) (effectiveInstalledAgents/effectiveAvailableAgents) and also defaults providerId to 'claude', allowing selection of providers that aren't actually installed; change this so installedAgents is trusted as-is (do not substitute agentConfig), keep effectiveInstalledAgents empty until real provider status is loaded, derive the initial providerId from the first real installed agent (or null if none) and render a disabled/empty-state UI when installedAgents is empty; update state initialization for providerId and any UI controls that use effectiveInstalledAgents/effectiveAvailableAgents (e.g., providerId, setProviderId, effectiveInstalledAgents, effectiveAvailableAgents) so users cannot select or start a review with non-installed providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/hostPreviewService.ts`:
- Around line 257-266: The emitted HostPreviewEvent uses the wrong property
name: change the emitted object in the catch block inside the emit('event', {
... }) call so it sets line: e instanceof Error ? e.message : String(e) instead
of error, keeping the same type 'setup', taskId and status 'error' to match the
HostPreviewEvent interface; leave the log.error and returned { ok: false, error:
... } behavior unchanged.
In `@src/main/services/ptyManager.ts`:
- Around line 504-505: The project path encoding for Claude session files is
inconsistent: discoverExistingClaudeSession and claudeSessionFileExists (and the
Claude checks in ptyIpc.ts) use different encoders, causing mismatched session
detection; factor out a single encoder function (e.g.,
createClaudeProjectEncoder or normalizeClaudeProjectPath) in
src/main/services/ptyManager.ts, replace the inline encoding
(cwd.replace(/[^a-zA-Z0-9]/g, '-')) in discoverExistingClaudeSession and
claudeSessionFileExists, and update the Claude codepaths in ptyIpc.ts to call
that shared encoder so all places compute identical encoded project directories
and sessionFile paths.
In `@src/renderer/components/AIReviewConfigModal.tsx`:
- Around line 63-67: AIReviewConfigModal currently hardcodes reviewType to
'file-changes' in the config object, preventing users from selecting the
alternate 'agent-output' mode; update the component (AIReviewConfigModal) to
expose reviewType as a selectable prop/state and use that value when building
the AIReviewConfig (the config constant), e.g., add a controlled field or prop
named reviewType and replace the hardcoded 'file-changes' with the chosen value
(ensure types still align with AIReviewConfig['reviewType'] and that providerId,
depth remain unchanged).
- Around line 69-80: The code calls launchReviewAgents(config, taskId, taskPath)
and ignores the returned reviewId and conversationIds; update the try block to
persist those IDs and kick off the results UX: pass reviewId and conversationIds
into the onSuccess callback (e.g., onSuccess(config, { reviewId, conversationIds
})), and then immediately open the results modal or start the polling flow (call
your existing openResultsModal(reviewId, conversationIds) or
startPollingReviewResults(reviewId)) so the app can poll for completion and show
the final notification instead of stopping at the "Review Started" toast; ensure
toast remains.
In `@src/renderer/lib/aiReview.ts`:
- Around line 197-202: The normalizeSeverity function currently maps both
'minor' and 'info' to 'minor', losing true informational findings; update
normalizeSeverity so that 'info' returns 'info' and 'minor' returns 'minor'
(e.g., separate the checks for 'minor' and 'info' or explicitly handle 'info'
before the default), ensuring the function returns one of 'critical' | 'major' |
'minor' | 'info' and preserving informational findings for badge/count
aggregation.
- Around line 41-48: The function buildReviewPrompt currently always reads
REVIEW_PROMPTS.fileChanges[...] which ignores the caller's requested review
family; update buildReviewPrompt to accept a reviewType parameter (e.g.,
reviewType: ReviewType or string) and use that as the key when selecting the
template (use REVIEW_PROMPTS[reviewType][depth]); also add a safe fallback or
validation if REVIEW_PROMPTS[reviewType] or the template is missing to avoid
runtime errors and keep the existing content concatenation behavior for the
content parameter.
- Around line 84-105: The conversation-changed event is emitted before the
reviewer PTY is started and without validating conversation?.id, causing orphan
conversations when startReviewAgentPty() fails; change the flow in the
createConversation + PTY startup block to first call rpc.db.createConversation
and assert conversation?.id, then await startReviewAgentPty(...) and only after
it successfully returns (and you have a ptyId) dispatch the
CONVERSATIONS_CHANGED_EVENT; if startReviewAgentPty throws, catch the error and
roll back by deleting the created conversation via
rpc.db.deleteConversation(conversation.id) (or equivalent) and rethrow or
propagate the error so launchReviewAgents() can fail cleanly.
- Around line 22-38: captureTerminalSnapshot currently treats its argument as a
pty key but should use the taskId that TerminalSessionManager and
terminalSessionRegistry use; parse the taskId from the given ptyId using the
same encoding used by makePtyId (e.g. split/join or a helper inverse) and then
call terminalSessionRegistry.getSession(taskId) and
window.electronAPI.ptyGetSnapshot({ id: taskId }) instead of using the raw ptyId
so the live session and persisted snapshot (saved under taskId) are found.
---
Duplicate comments:
In `@src/renderer/components/AIReviewConfigModal.tsx`:
- Around line 39-52: The current logic fakes an installed list by falling back
to Object.keys(agentConfig) (effectiveInstalledAgents/effectiveAvailableAgents)
and also defaults providerId to 'claude', allowing selection of providers that
aren't actually installed; change this so installedAgents is trusted as-is (do
not substitute agentConfig), keep effectiveInstalledAgents empty until real
provider status is loaded, derive the initial providerId from the first real
installed agent (or null if none) and render a disabled/empty-state UI when
installedAgents is empty; update state initialization for providerId and any UI
controls that use effectiveInstalledAgents/effectiveAvailableAgents (e.g.,
providerId, setProviderId, effectiveInstalledAgents, effectiveAvailableAgents)
so users cannot select or start a review with non-installed providers.
In `@src/renderer/lib/aiReview.ts`:
- Around line 249-268: The parser fails to track fenced-code state because it
only begins collecting when currentCodeBlock has content; update the logic
around the line.startsWith('```') fence handling to use an explicit boolean
(e.g., inCodeBlock) that flips true on an opening fence and false on a closing
fence, assign currentIssue.codeSnapshot when closing the fence (as currently
done), and when inCodeBlock push every line into currentCodeBlock regardless of
indentation (remove the dependency on line.match(/^\s{2,}/) for fenced blocks)
while still skipping the fence line itself; reference the symbols
currentCodeBlock, currentIssue, line.startsWith('```'), and codeSnapshot when
making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ebd1daf-d9ad-41ea-80bd-c640baaab341
📒 Files selected for processing (13)
package.jsonsrc/main/services/hostPreviewService.tssrc/main/services/ptyManager.tssrc/main/services/ssh/SshService.tssrc/renderer/components/AIReviewConfigModal.tsxsrc/renderer/components/FileChangesPanel.tsxsrc/renderer/components/sidebar/LeftSidebar.tsxsrc/renderer/contexts/ModalProvider.tsxsrc/renderer/lib/aiReview.tssrc/renderer/lib/quickPreview.tssrc/renderer/lib/reviewChat.tssrc/renderer/terminal/TerminalSessionManager.tssrc/shared/reviewPreset.ts
✅ Files skipped from review due to trivial changes (5)
- package.json
- src/renderer/lib/quickPreview.ts
- src/renderer/components/FileChangesPanel.tsx
- src/renderer/components/sidebar/LeftSidebar.tsx
- src/main/services/ssh/SshService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/reviewPreset.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/renderer/lib/aiReview.ts (2)
294-299: Function isasyncbut contains noawait.
aggregateReviewResultsis declaredasyncbut performs only synchronous operations. This works but is misleading.♻️ Make function synchronous
-export async function aggregateReviewResults( +export function aggregateReviewResults( results: Array<{ conversationId: string; messages: ReviewMessage[] }>, config: AIReviewConfig, reviewId: string, durationMs: number -): Promise<AIReviewResult> { +): AIReviewResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/aiReview.ts` around lines 294 - 299, The function aggregateReviewResults is declared async but does not use await; remove the misleading async and make it a synchronous function by deleting the async keyword and changing the return type from Promise<AIReviewResult> to AIReviewResult in the function signature (keep parameter types AIReviewConfig, ReviewMessage, and reviewId/durationMs as-is); then update any call sites that await aggregateReviewResults to call it synchronously (remove await or handle the direct AIReviewResult return) so callers match the new synchronous contract.
19-21: Weak randomness for ID generation.
Math.random()is not cryptographically secure and can produce collisions. For review IDs and issue IDs, consider usingcrypto.randomUUID()which is available in modern browsers and Electron.♻️ Suggested improvement
function generateId(): string { - return Math.random().toString(36).substring(2, 15); + return crypto.randomUUID(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/aiReview.ts` around lines 19 - 21, The generateId() function uses Math.random() which is weak and collision-prone; update generateId() to use crypto.randomUUID() for secure, collision-resistant IDs (e.g., in aiReview.generateId or wherever generateId is referenced) and add a minimal fallback if crypto.randomUUID is not available (for example, use crypto.getRandomValues-based UUID or a Promise to polyfill) so callers of generateId() continue to receive stable string IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/lib/aiReview.ts`:
- Around line 136-141: The current Promise.all over launches will abort on the
first rejection and leave any already-started agents orphaned; replace
Promise.all with Promise.allSettled on the launches array, collect successful
results (from settled results with status === "fulfilled") into conversationIds
and ptyIds, and if any settled result has status === "rejected" perform a
rollback: call the module's existing agent cleanup/teardown functions for each
fulfilled entry (e.g., the same functions used to close conversations or dispose
PTYs — reference the cleanup helpers used elsewhere such as
terminateAgent/closeConversation/disposePty) and then propagate or return an
error; ensure the function that contains launches, results, conversationIds,
ptyIds and the final return (the enclosing function that returns { reviewId,
conversationIds, ptyIds }) implements this allSettled+cleanup logic so no
started agents remain orphaned on partial failure.
In `@src/shared/reviewPreset.ts`:
- Line 29: ReviewType currently lists 'agent-output' but REVIEW_PROMPTS only
defines fileChanges and buildReviewPrompt in aiReview.ts maps both cases to
fileChanges; either remove 'agent-output' from the ReviewType union to reflect
current implementation, or implement full support by adding an agentOutput entry
to REVIEW_PROMPTS (mirroring the fileChanges template keys) and update
buildReviewPrompt to map reviewType correctly (e.g., use 'fileChanges' when
reviewType === 'file-changes' else 'agentOutput'); ensure any imports/usages in
aiReview.ts reference the new agentOutput key if you choose to implement it.
---
Nitpick comments:
In `@src/renderer/lib/aiReview.ts`:
- Around line 294-299: The function aggregateReviewResults is declared async but
does not use await; remove the misleading async and make it a synchronous
function by deleting the async keyword and changing the return type from
Promise<AIReviewResult> to AIReviewResult in the function signature (keep
parameter types AIReviewConfig, ReviewMessage, and reviewId/durationMs as-is);
then update any call sites that await aggregateReviewResults to call it
synchronously (remove await or handle the direct AIReviewResult return) so
callers match the new synchronous contract.
- Around line 19-21: The generateId() function uses Math.random() which is weak
and collision-prone; update generateId() to use crypto.randomUUID() for secure,
collision-resistant IDs (e.g., in aiReview.generateId or wherever generateId is
referenced) and add a minimal fallback if crypto.randomUUID is not available
(for example, use crypto.getRandomValues-based UUID or a Promise to polyfill) so
callers of generateId() continue to receive stable string IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c9e3217-cf22-418c-ac52-c9ef8b3ba675
📒 Files selected for processing (2)
src/renderer/lib/aiReview.tssrc/shared/reviewPreset.ts
yuzhichang
force-pushed
the
emdash/ai-review-1m8
branch
from
2d93733 to
1a75b8f
Compare
- Change TCP probe to HTTP HEAD request for dev servers - Replace raw kill(0) checks with exitCode/signalCode cross-platform checks - Handle potential duplicate CLI flag appending in hostPreview - Add missing error emission for failed auto-installs - Explicitly throw errors when provider status check fails in reviewChat - Add null check for conversation creation in review chat
- Add JSON schema to REVIEW_PROMPTS for structured output - Parallelize agent launches with Promise.all in launchReviewAgents - Fix normalizeSeverity to properly return 'info' instead of mapping to 'minor' - Add explicit inCodeBlock boolean flag for fenced code block parsing - Add ReviewMessage type to shared types (fixes cross-boundary import) - Update buildReviewPrompt to accept reviewType parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use Promise.allSettled instead of Promise.all to handle partial failures - When some agents fail to launch, log warnings and proceed with successful ones - Remove incomplete 'agent-output' review type (only 'file-changes' is supported) - Simplify buildReviewPrompt now that only file-changes type exists Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yuzhichang
force-pushed
the
emdash/ai-review-1m8
branch
from
1a75b8f to
e561801
Compare
…iewType handling - Extract normalizeClaudeProjectPath() in ptyManager.ts to replace inconsistent inline encoders across claudeSessionFileExists, discoverExistingClaudeSession, and ptyIpc.ts - Fix buildReviewPrompt() to accept reviewType parameter and use it for template selection with safe fallback to fileChanges Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
git diffdirectlyFiles Changed
src/shared/reviewPreset.ts- Types and prompt templatessrc/renderer/lib/aiReview.ts- Core review logicsrc/renderer/components/AIReviewConfigModal.tsx- Configuration modalsrc/renderer/components/AIReviewResultsModal.tsx- Results display modalsrc/renderer/components/FileChangesPanel.tsx- AI Review buttonsrc/renderer/contexts/ModalProvider.tsx- Modal registryTest plan
Closes #562
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements