Review Agent #298
Review Agent #298
Conversation
WalkthroughThis update introduces a new automated GitHub pull request review agent, including its type definitions, control flow, and integration with OpenAI for code review generation. It adds a new agents page to the web app, updates workspace and dependency configurations, and implements a GitHub webhook API route for handling PR events. The Fly.io deployment configuration for staging is removed. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant WebhookAPI as Webhook API Route
participant ReviewAgent as Review Agent
participant Octokit
participant OpenAI
GitHub->>WebhookAPI: POST /api/(server)/webhook (PR event)
WebhookAPI->>Octokit: Authenticate as GitHub App
WebhookAPI->>ReviewAgent: processGitHubPullRequest(octokit, payload)
ReviewAgent->>Octokit: githubPrParser (fetch PR diff)
ReviewAgent->>ReviewAgent: generatePrReviews (for each file diff)
loop For each diff
ReviewAgent->>WebhookAPI: fetchFileContent (context)
ReviewAgent->>ReviewAgent: generateDiffReviewPrompt
ReviewAgent->>OpenAI: invokeDiffReviewLlm(prompt)
OpenAI-->>ReviewAgent: Diff review response
end
ReviewAgent->>Octokit: githubPushPrReviews (post review comments)
WebhookAPI-->>GitHub: 200 OK
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageTooldocs/docs/more/mcp-server.mdx[misspelling] ~10-~10: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (13)
packages/agents/reviewAgent/.gitignore (2)
83-83: Remove redundant.cacheentriesThe
.cacheentry under the parcel-bundler section (line 83) and the.cacheentry under the VuePress v2 section (line 106) both duplicate the global.cacheignore. Consider removing these duplicates to declutter the file.- .cache # parcel-bundler cache (line 83) ... - .cache # vuepress v2.x temp and cache directory (line 106)Also applies to: 106-106
136-137: Add OS-specific ignore patternsTo prevent committing common machine-specific files, consider adding entries for macOS and Windows artifacts at the end of the file:
+# macOS +.DS_Store + +# Windows +Thumbs.db +ehthumbs.dbpackages/agents/reviewAgent/tsconfig.json (1)
1-114: TypeScript configuration looks solid, consider enabling additional checks for production codeThe TypeScript configuration is well-structured with appropriate target (ES2020) and module settings (NodeNext). The basic strict type checking is enabled, which is excellent.
Consider enabling these additional compiler options for a more robust codebase:
- // "sourceMap": true, /* Create source map files for emitted JavaScript files. */ + "sourceMap": true, /* Create source map files for emitted JavaScript files. */ - // "noUnusedLocals": true, /* Enable error reporting when local variables aren't read. */ + "noUnusedLocals": true, /* Enable error reporting when local variables aren't read. */ - // "noUnusedParameters": true, /* Raise an error when a function parameter isn't read. */ + "noUnusedParameters": true, /* Raise an error when a function parameter isn't read. */ - // "noImplicitReturns": true, /* Enable error reporting for codepaths that do not explicitly return in a function. */ + "noImplicitReturns": true, /* Enable error reporting for codepaths that do not explicitly return in a function. */packages/agents/reviewAgent/nodes/generate_pr_reviews.ts (1)
6-6: Validate input rules arrayThe function takes a
rulesarray but doesn't validate that it's non-empty or contains valid rules.Consider adding validation for the rules array:
export const generate_pr_reviews = async (pr_payload: sourcebot_pr_payload, rules: string[]): Promise<sourcebot_file_diff_review[]> => { console.log("Executing generate_pr_reviews"); + + if (!rules || rules.length === 0) { + console.warn("No review rules provided, reviews may be less focused"); + }packages/agents/reviewAgent/package.json (1)
4-4: Add a meaningful descriptionThe description field is empty. Adding a clear description will help others understand what this package does.
- "description": "", + "description": "GitHub pull request review agent using LLM for automated code reviews",packages/agents/reviewAgent/nodes/github_push_pr_reviews.ts (1)
10-29: Consider rate limiting for GitHub API callsThe function makes multiple API calls in a loop without any rate limiting or batching, which could hit GitHub's rate limits if there are many reviews.
Consider adding a delay between API calls or implementing a batching mechanism:
import { App } from "octokit"; import { sourcebot_pr_payload, sourcebot_file_diff_review } from "../types.js"; + // Helper function to add delay between API calls + const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); export const github_push_pr_reviews = async (app: App, pr_payload: sourcebot_pr_payload, file_diff_reviews: sourcebot_file_diff_review[]) => { console.log("Executing github_push_pr_reviews"); const installationId = pr_payload.installation_id; const installation = await app.getInstallationOctokit(installationId); for (const file_diff_review of file_diff_reviews) { for (const review of file_diff_review.reviews) { await installation.rest.pulls.createReviewComment({ owner: pr_payload.owner, repo: pr_payload.repo, pull_number: pr_payload.number, body: review.review, path: file_diff_review.filename, commit_id: pr_payload.head_sha, side: "RIGHT", ...(review.line_start === review.line_end ? { line: review.line_start } : { start_line: review.line_start, line: review.line_end, start_side: "RIGHT", }), }); + // Add a small delay between API calls to avoid rate limits + await delay(100); } }packages/agents/reviewAgent/nodes/generate_diff_review_prompt.ts (2)
8-9: Fix typo in prompt text.There's a typo in the prompt text: "excells" should be "excels".
- You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide + You are an expert software engineer that excels at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide
4-40: Consider enhancing prompt structure for maintainability.The prompt is constructed using template literals with complex nesting, which could make maintenance difficult. Consider using an array-based approach with join operations for better maintainability.
I recommend restructuring the prompt construction like this:
export const generate_diff_review_prompt = async (diff: sourcebot_diff, context: sourcebot_context[], rules: string[]) => { console.log("Executing generate_diff_review_prompt"); const promptSections = [ `You are an expert software engineer that excels at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide must conform to all of the rules defined below. The output format of your review must conform to the output format defined below.`, `# Input`, `The input is the old and new code snippets, which represent a single hunk from a git diff. The old code snippet is the code before the changes were made, and the new code snippet is the code after the changes were made. Each code snippet is a sequence of lines each with a line number.`, `## Old Code Snippet`, "```", diff.oldSnippet, "```", `## New Code Snippet`, "```", diff.newSnippet, "```", `# Additional Context`, context.length > 0 ? context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n") : "No additional context provided.", `# Rules`, rules.map(rule => `- ${rule}`).join("\n"), `# Output Format (JSON Schema)`, `The output must be a valid JSON object that conforms to the following JSON schema. Do NOT respond with anything other than the JSON object. Do NOT respond with the JSON object in a markdown code block.`, JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2) ]; const prompt = promptSections.join("\n\n"); console.log("Completed generate_diff_review_prompt"); return prompt; }This structure makes it easier to modify individual sections of the prompt.
packages/agents/reviewAgent/nodes/invoke_diff_review_llm.ts (1)
10-10: Remove unused filename parameter.The
filenameparameter is declared but never used in the function. Either use it or remove it.-export const invoke_diff_review_llm = async (prompt: string, filename: string): Promise<sourcebot_diff_review> => { +export const invoke_diff_review_llm = async (prompt: string): Promise<sourcebot_diff_review> => {packages/agents/reviewAgent/nodes/github_pr_parser.ts (1)
45-45: Use type predicates instead of type assertions.The code uses a type assertion to convert the filtered array, but a type predicate would be safer and more idiomatic.
- const filteredSourcebotFileDiffs: sourcebot_file_diff[] = sourcebotFileDiffs.filter((file) => file !== null) as sourcebot_file_diff[]; + function isNotNull(file: sourcebot_file_diff | null): file is sourcebot_file_diff { + return file !== null; + } + const filteredSourcebotFileDiffs: sourcebot_file_diff[] = sourcebotFileDiffs.filter(isNotNull);packages/agents/reviewAgent/types.ts (3)
3-48: Use PascalCase for type names to follow TypeScript conventionsThe file uses snake_case for type names (e.g.,
sourcebot_diff), which is unusual in TypeScript. TypeScript convention typically uses PascalCase for types and interfaces.import { z } from "zod"; export const sourcebot_diff_schema = z.object({ oldSnippet: z.string(), newSnippet: z.string(), }); -export type sourcebot_diff = z.infer<typeof sourcebot_diff_schema>; +export type SourcebotDiff = z.infer<typeof sourcebot_diff_schema>; export const sourcebot_file_diff_schema = z.object({ from: z.string(), to: z.string(), diffs: z.array(sourcebot_diff_schema), }); -export type sourcebot_file_diff = z.infer<typeof sourcebot_file_diff_schema>; +export type SourcebotFileDiff = z.infer<typeof sourcebot_file_diff_schema>; export const sourcebot_pr_payload_schema = z.object({ title: z.string(), description: z.string(), hostDomain: z.string(), owner: z.string(), repo: z.string(), file_diffs: z.array(sourcebot_file_diff_schema), number: z.number(), head_sha: z.string(), installation_id: z.number(), }); -export type sourcebot_pr_payload = z.infer<typeof sourcebot_pr_payload_schema>; +export type SourcebotPrPayload = z.infer<typeof sourcebot_pr_payload_schema>; export const sourcebot_context_schema = z.object({ type: z.enum(["pr_title", "pr_description", "pr_summary", "comment_chains", "file_content"]), description: z.string().optional(), context: z.string(), }); -export type sourcebot_context = z.infer<typeof sourcebot_context_schema>; +export type SourcebotContext = z.infer<typeof sourcebot_context_schema>; export const sourcebot_diff_review_schema = z.object({ line_start: z.number(), line_end: z.number(), review: z.string(), }); -export type sourcebot_diff_review = z.infer<typeof sourcebot_diff_review_schema>; +export type SourcebotDiffReview = z.infer<typeof sourcebot_diff_review_schema>; export const sourcebot_file_diff_review_schema = z.object({ filename: z.string(), reviews: z.array(sourcebot_diff_review_schema), }); -export type sourcebot_file_diff_review = z.infer<typeof sourcebot_file_diff_review_schema>; +export type SourcebotFileDiffReview = z.infer<typeof sourcebot_file_diff_review_schema>;
3-48: Add JSDoc comments to improve code documentationThe file lacks JSDoc comments to explain the purpose and usage of each type and schema. Adding documentation would improve code maintainability.
Here's an example of how you could add JSDoc comments:
import { z } from "zod"; +/** + * Schema for code diff representation containing old and new code snippets + */ export const sourcebot_diff_schema = z.object({ oldSnippet: z.string(), newSnippet: z.string(), }); export type sourcebot_diff = z.infer<typeof sourcebot_diff_schema>; +/** + * Schema for file-level diff containing path information and an array of code diffs + */ export const sourcebot_file_diff_schema = z.object({ from: z.string(), to: z.string(), diffs: z.array(sourcebot_diff_schema), }); export type sourcebot_file_diff = z.infer<typeof sourcebot_file_diff_schema>; // Add similar JSDoc comments for the remaining schemas
36-40: Add validation constraints for line numbersThe line numbers in the diff review schema should be positive integers and line_end should be greater than or equal to line_start. Adding these constraints would prevent invalid data.
export const sourcebot_diff_review_schema = z.object({ - line_start: z.number(), - line_end: z.number(), + line_start: z.number().int().positive(), + line_end: z.number().int().positive().refine(val => val >= this.line_start, { + message: "line_end must be greater than or equal to line_start" + }), review: z.string(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/agents/reviewAgent/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/agents/reviewAgent/.gitignore(1 hunks)packages/agents/reviewAgent/app.ts(1 hunks)packages/agents/reviewAgent/nodes/fetch_file_content.ts(1 hunks)packages/agents/reviewAgent/nodes/generate_diff_review_prompt.ts(1 hunks)packages/agents/reviewAgent/nodes/generate_pr_reviews.ts(1 hunks)packages/agents/reviewAgent/nodes/github_pr_parser.ts(1 hunks)packages/agents/reviewAgent/nodes/github_push_pr_reviews.ts(1 hunks)packages/agents/reviewAgent/nodes/invoke_diff_review_llm.ts(1 hunks)packages/agents/reviewAgent/package.json(1 hunks)packages/agents/reviewAgent/tsconfig.json(1 hunks)packages/agents/reviewAgent/types.ts(1 hunks)staging/fly.toml(0 hunks)
💤 Files with no reviewable changes (1)
- staging/fly.toml
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/agents/reviewAgent/nodes/fetch_file_content.ts (3)
packages/web/src/features/search/schemas.ts (1)
fileSourceResponseSchema(101-104)packages/web/src/lib/utils.ts (1)
base64Decode(191-194)packages/agents/reviewAgent/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_context(34-34)
packages/agents/reviewAgent/nodes/invoke_diff_review_llm.ts (1)
packages/agents/reviewAgent/types.ts (2)
sourcebot_diff_review(41-41)sourcebot_diff_review_schema(36-40)
packages/agents/reviewAgent/nodes/github_pr_parser.ts (1)
packages/agents/reviewAgent/types.ts (3)
sourcebot_pr_payload(27-27)sourcebot_file_diff(14-14)sourcebot_diff(7-7)
packages/agents/reviewAgent/nodes/generate_diff_review_prompt.ts (1)
packages/agents/reviewAgent/types.ts (3)
sourcebot_diff(7-7)sourcebot_context(34-34)sourcebot_diff_review_schema(36-40)
packages/agents/reviewAgent/nodes/github_push_pr_reviews.ts (1)
packages/agents/reviewAgent/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_file_diff_review(47-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
packages/agents/reviewAgent/.gitignore (1)
1-137: Comprehensive .gitignore coverageThe new
.gitignorecovers logs, diagnostic reports, runtime data, dependency directories, build outputs, and tool caches for various frameworks and bundlers, aligning well with Node.js and TypeScript project best practices.packages/agents/reviewAgent/package.json (2)
11-20: Repository URLs don't match current project structureThe repository URLs point to "sourcebot-dev/review-agent" on GitHub, but this code appears to be located within "packages/agents/reviewAgent" in a different repository.
Please verify if these repository settings need to be updated to match the current project's structure and hosting location.
24-24: ```shell
npm view @octokit/webhooks-definitions version</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (8)
packages/agents/reviewAgent/src/nodes/invoke_diff_review_llm.ts (2)
33-36: Improve error handlingThe current error handling logs the entire error object, which might expose sensitive information in production. Consider sanitizing the error message.
} catch (error) { - console.error('Error calling OpenAI:', error); + const errorMessage = error instanceof Error ? error.message : String(error); + console.error('Error calling OpenAI:', errorMessage); throw error; }
10-37: Implement retry mechanism for API failuresThe function doesn't have any retry logic for handling transient API failures, which are common with external services.
Consider implementing a simple retry mechanism with exponential backoff:
export const invoke_diff_review_llm = async (prompt: string, filename: string, maxRetries = 3): Promise<sourcebot_diff_review> => { console.log("Executing invoke_diff_review_llm"); let retries = 0; while (true) { try { const completion = await openai.chat.completions.create({ model: MODEL, timeout: TIMEOUT_MS, messages: [ { role: "system", content: prompt } ] }); const openaiResponse = completion.choices[0].message.content; console.log("OpenAI response: ", openaiResponse); const diffReviewJson = JSON.parse(openaiResponse || '{}'); const diffReview = sourcebot_diff_review_schema.safeParse(diffReviewJson); if (!diffReview.success) { throw new Error(`Invalid diff review format: ${diffReview.error}`); } console.log("Completed invoke_diff_review_llm"); return diffReview.data; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); console.error(`Error calling OpenAI (attempt ${retries + 1}/${maxRetries}):`, errorMessage); if (retries >= maxRetries) { throw error; } // Exponential backoff const delay = Math.pow(2, retries) * 1000; console.log(`Retrying in ${delay}ms...`); await new Promise(resolve => setTimeout(resolve, delay)); retries++; } } }packages/agents/reviewAgent/src/nodes/generate_diff_review_prompt.ts (2)
30-30: Handle empty context or context with special charactersThe context mapping doesn't handle the case where a context item might be empty or contain special characters that could interfere with the prompt formatting.
- ${context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n")} + ${context.length === 0 ? 'No additional context provided.' : + context.map(c => { + const safeContext = (c.context || '').replace(/`/g, '\\`'); // Escape backticks + return `${c.type || 'Unknown'}: ${c.description || ''}\n\n${safeContext}`; + }).join("\n\n----------------------\n\n")}
34-34: Handle empty rules listSimilar to the context, the rules mapping doesn't handle the case where the rules array might be empty.
- - ${rules.join("\n- ")} + ${rules.length === 0 ? 'No specific rules provided.' : rules.map(rule => `- ${rule}`).join("\n")}packages/agents/reviewAgent/src/nodes/generate_pr_reviews.ts (1)
30-33: Add specific error handling for LLM invocationThe current error handling only catches errors from fetching file content, but errors from generating prompts or invoking the LLM are not specifically caught and handled.
try { const fileContentContext = await fetch_file_content(pr_payload, file_diff.to); const context: sourcebot_context[] = [ // ...context definitions... ]; - const prompt = await generate_diff_review_prompt(diff, context, rules); - - const diffReview = await invoke_diff_review_llm(prompt, file_diff.to); - reviews.push(diffReview); + try { + const prompt = await generate_diff_review_prompt(diff, context, rules); + + try { + const diffReview = await invoke_diff_review_llm(prompt, file_diff.to); + reviews.push(diffReview); + } catch (llmError) { + console.error(`Error invoking LLM for ${file_diff.to}: ${llmError}`); + } + } catch (promptError) { + console.error(`Error generating prompt for ${file_diff.to}: ${promptError}`); + } } catch (error) { console.error(`Error fetching file content for ${file_diff.to}: ${error}`); }packages/agents/reviewAgent/src/nodes/github_push_pr_reviews.ts (2)
30-32: Implement retry mechanism for failed API callsThe current implementation doesn't retry failed API calls, which could lead to missing review comments for transient API failures.
try { await limiter.schedule(() => installation.rest.pulls.createReviewComment({ // ... comment parameters })); successCount++; } catch (error) { console.error(`Error pushing pr reviews for ${file_diff_review.filename}: ${error}`); - failCount++; + // Retry the API call up to 3 times with exponential backoff + let retryCount = 0; + const maxRetries = 3; + let success = false; + + while (retryCount < maxRetries && !success) { + const delay = Math.pow(2, retryCount) * 1000; // 1s, 2s, 4s + console.log(`Retrying (${retryCount + 1}/${maxRetries}) after ${delay}ms...`); + + await new Promise(resolve => setTimeout(resolve, delay)); + + try { + await limiter.schedule(() => installation.rest.pulls.createReviewComment({ + // ... same comment parameters + owner: pr_payload.owner, + repo: pr_payload.repo, + pull_number: pr_payload.number, + body: review.review, + path: file_diff_review.filename, + commit_id: pr_payload.head_sha, + side: "RIGHT", + ...(review.line_start === review.line_end + ? { line: review.line_start } + : { + start_line: review.line_start, + line: review.line_end, + start_side: "RIGHT", + }), + })); + success = true; + successCount++; + console.log(`Retry successful for comment on ${file_diff_review.filename}`); + } catch (retryError) { + console.error(`Retry failed (${retryCount + 1}/${maxRetries}): ${retryError}`); + retryCount++; + } + } + + if (!success) { + failCount++; + console.error(`All retries failed for comment on ${file_diff_review.filename}`); + } }
4-40: Batch review comments for efficiencyThe current implementation creates individual review comments for each review, which is inefficient for PRs with many reviews. Consider batching reviews using GitHub's Pull Request Review API.
While the current approach of pushing individual review comments works, GitHub also offers a more efficient way to post multiple comments at once using the
pulls.createReviewendpoint. This can reduce the number of API calls and better handle rate limits.Consider refactoring to use the batched approach. Here's a simplified example:
// Group comments by file const commentsByFile = new Map<string, Array<{line: number, body: string}>>(); for (const file_diff_review of file_diff_reviews) { for (const review of file_diff_review.reviews) { if (!commentsByFile.has(file_diff_review.filename)) { commentsByFile.set(file_diff_review.filename, []); } commentsByFile.get(file_diff_review.filename)?.push({ line: review.line_end, body: review.review }); } } // Create a single review with all comments await installation.rest.pulls.createReview({ owner: pr_payload.owner, repo: pr_payload.repo, pull_number: pr_payload.number, commit_id: pr_payload.head_sha, event: "COMMENT", comments: Array.from(commentsByFile.entries()).flatMap(([path, comments]) => comments.map(comment => ({ path, line: comment.line, body: comment.body })) ) });This approach would significantly reduce API calls and improve performance for larger PRs.
packages/agents/reviewAgent/src/nodes/github_pr_parser.ts (1)
22-41: Add a size limit for snippets to avoid memory issuesFor large diffs, the accumulated snippets could consume excessive memory. Consider limiting their size or using a more memory-efficient approach.
const diffs: sourcebot_diff[] = file.chunks.map((chunk) => { let oldSnippet = ""; let newSnippet = ""; + const MAX_SNIPPET_SIZE = 10000; // Limit snippet size to avoid memory issues + let snippetSizeExceeded = false; for (const change of chunk.changes) { + // Check if we've already exceeded the size limit + if (snippetSizeExceeded) continue; + if (change.type === "normal") { oldSnippet += change.ln1 + ":" + change.content; newSnippet += change.ln2 + ":" + change.content; } else if (change.type === "add") { newSnippet += change.ln + ":" + change.content; } else if (change.type === "del") { oldSnippet += change.ln + ":" + change.content; } + + // Check if we've exceeded the size limit after adding this change + if (oldSnippet.length > MAX_SNIPPET_SIZE || newSnippet.length > MAX_SNIPPET_SIZE) { + snippetSizeExceeded = true; + oldSnippet += "\n... (snippet truncated due to size)"; + newSnippet += "\n... (snippet truncated due to size)"; + } } return { oldSnippet: oldSnippet, newSnippet: newSnippet, } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/agents/reviewAgent/nodes/fetch_file_content.ts(1 hunks)packages/agents/reviewAgent/package.json(1 hunks)packages/agents/reviewAgent/src/app.ts(1 hunks)packages/agents/reviewAgent/src/nodes/fetch_file_content.ts(1 hunks)packages/agents/reviewAgent/src/nodes/generate_diff_review_prompt.ts(1 hunks)packages/agents/reviewAgent/src/nodes/generate_pr_reviews.ts(1 hunks)packages/agents/reviewAgent/src/nodes/github_pr_parser.ts(1 hunks)packages/agents/reviewAgent/src/nodes/github_push_pr_reviews.ts(1 hunks)packages/agents/reviewAgent/src/nodes/invoke_diff_review_llm.ts(1 hunks)packages/agents/reviewAgent/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/agents/reviewAgent/package.json
- packages/agents/reviewAgent/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/agents/reviewAgent/nodes/fetch_file_content.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/agents/reviewAgent/src/nodes/fetch_file_content.ts (4)
packages/agents/reviewAgent/nodes/fetch_file_content.ts (1)
fetch_file_content(15-49)packages/agents/reviewAgent/src/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_context(34-34)packages/web/src/features/search/schemas.ts (1)
fileSourceResponseSchema(101-104)packages/web/src/lib/utils.ts (1)
base64Decode(191-194)
packages/agents/reviewAgent/src/app.ts (3)
packages/agents/reviewAgent/src/nodes/github_pr_parser.ts (1)
github_pr_parser(6-63)packages/agents/reviewAgent/src/nodes/generate_pr_reviews.ts (1)
generate_pr_reviews(6-49)packages/agents/reviewAgent/src/nodes/github_push_pr_reviews.ts (1)
github_push_pr_reviews(4-40)
packages/agents/reviewAgent/src/nodes/generate_diff_review_prompt.ts (1)
packages/agents/reviewAgent/src/types.ts (3)
sourcebot_diff(7-7)sourcebot_context(34-34)sourcebot_diff_review_schema(36-40)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
packages/agents/reviewAgent/src/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/agents/reviewAgent/src/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/agents/reviewAgent/src/nodes/generate_diff_review_prompt.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (18)
packages/web/src/app/[domain]/agents/page.tsx (6)
1-7: Remove unused imports.There are several imports that are never used in the component:
Header(line 1)Image(line 3)FaRobot(line 5)This increases bundle size unnecessarily.
-import { Header } from "../components/header"; +import { NavigationMenu } from "../components/navigationMenu"; import Link from "next/link"; -import Image from "next/image"; -import { NavigationMenu } from "../components/navigationMenu"; -import { FaRobot, FaCogs } from "react-icons/fa"; +import { FaCogs } from "react-icons/fa"; import { MdRocketLaunch } from "react-icons/md";
8-17: Move agent data outside the component for better maintainability.The agents array is defined inline within the module but outside of the component, which is good. However, consider moving this data to a separate file or service as the number of agents grows, especially since the comment suggests more agents will be added.
As the application grows, you might want to fetch this data from an API endpoint rather than hardcoding it. If you decide to keep it as a static array for now, consider:
+// Consider moving to a separate file like: src/app/[domain]/agents/agentConfig.ts const agents = [ { id: "review-agent", name: "Review Agent", description: "An agent that reviews your PRs. Uses the code indexed on Sourcebot to provide codebase wide context.", deployUrl: "/agents/review-agent/deploy", configureUrl: "/agents/review-agent/configure", }, // Add more agents here as needed ];
26-38: Simplify conditional class logic with a more maintainable approach.The conditional class logic is repeated in two places, making the code less DRY. Consider extracting this logic or using a utility like
clsxortailwind-mergeto handle conditional classes more elegantly.+// Add at the top of your file +import clsx from 'clsx'; // Then in the component <div - className={ - agents.length === 1 - ? "flex justify-center items-center min-h-[60vh]" - : "grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-10" - } + className={clsx( + agents.length === 1 + ? "flex justify-center items-center min-h-[60vh]" + : "grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-10" + )} > {agents.map((agent) => ( <div key={agent.id} - className={ - agents.length === 1 - ? "relative flex flex-col items-center border border-border rounded-2xl p-8 bg-card shadow-xl w-full max-w-xl" - : "relative flex flex-col items-center border border-border rounded-2xl p-8 bg-card shadow-xl" - } + className={clsx( + "relative flex flex-col items-center border border-border rounded-2xl p-8 bg-card shadow-xl", + agents.length === 1 && "w-full max-w-xl" + )} >
45-47: Ensure description height is consistent for better UI alignment.The description has a
min-heightof 56px, which helps maintain consistent card heights. However, if descriptions vary significantly in length, consider adding truncation or handling overflow gracefully.-<p className="text-base text-muted-foreground text-center mb-4 min-h-[56px]"> +<p className="text-base text-muted-foreground text-center mb-4 min-h-[56px] line-clamp-2"> {agent.description} </p>This will limit the description to two lines and add an ellipsis if it's longer, maintaining visual consistency.
21-22: Add page title and improve layout structure.The page is missing a title section. For better UX and accessibility, add a page title that clearly indicates what this page is about.
<div className="flex flex-col items-center overflow-hidden min-h-screen"> <NavigationMenu domain={domain} /> + <div className="w-full max-w-6xl px-4 mt-8"> + <h1 className="text-3xl font-bold text-foreground mb-4">AI Agents</h1> + <p className="text-muted-foreground mb-8">Deploy and configure AI agents to enhance your workflow.</p> + </div> <div className="w-full max-w-6xl px-4 mt-12 mb-24">
50-63: Improve button accessibility.The action buttons look good visually, but we should ensure they're fully accessible. Consider adding aria labels for screen readers, especially since the buttons use icons.
<Link href={agent.deployUrl} + aria-label={`Deploy ${agent.name}`} className="flex items-center justify-center gap-2 px-5 py-2.5 rounded-md bg-primary text-primary-foreground font-mono font-semibold text-base border border-primary shadow-sm hover:bg-primary/80 focus:outline-none focus:ring-2 focus:ring-primary/60 transition w-1/2" > <MdRocketLaunch className="text-lg" /> Deploy </Link> <Link href={agent.configureUrl} + aria-label={`Configure ${agent.name}`} className="flex items-center justify-center gap-2 px-5 py-2.5 rounded-md bg-muted text-foreground font-mono font-semibold text-base border border-border shadow-sm hover:bg-card/80 focus:outline-none focus:ring-2 focus:ring-border/60 transition w-1/2" > <FaCogs className="text-lg" /> Configure </Link>packages/agents/review-agent/src/nodes/invoke_diff_review_llm.ts (1)
22-22: Limit logging of OpenAI response for security and performance.Logging the entire OpenAI response could expose sensitive information and clutter logs with large amounts of data.
- console.log("OpenAI response: ", openaiResponse); + console.log("OpenAI response received with length: ", openaiResponse?.length || 0);packages/agents/review-agent/src/nodes/generate_diff_review_prompt.ts (3)
7-40: Consider using a more structured approach for prompt generation.The current large template literal with many interpolated values could become difficult to maintain as the prompt complexity grows.
Consider refactoring to use a more structured approach:
- const prompt = ` - You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide - must conform to all of the rules defined below. The output format of your review must conform to the output format defined below. - - # Input - - The input is the old and new code snippets, which represent a single hunk from a git diff. The old code snippet is the code before the changes were made, and the new code snippet is the code after the changes were made. Each code snippet - is a sequence of lines each with a line number. - - ## Old Code Snippet - - \`\`\` - ${diff.oldSnippet} - \`\`\` - - ## New Code Snippet - - \`\`\` - ${diff.newSnippet} - \`\`\` - - # Additional Context - - ${context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n")} - - # Rules - - - ${rules.join("\n- ")} - - # Output Format (JSON Schema) - The output must be a valid JSON object that conforms to the following JSON schema. Do NOT respond with anything other than the JSON object. Do NOT respond with - the JSON object in a markdown code block. - ${JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2)} - `; + const sections = { + introduction: "You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide must conform to all of the rules defined below. The output format of your review must conform to the output format defined below.", + input: { + description: "The input is the old and new code snippets, which represent a single hunk from a git diff. The old code snippet is the code before the changes were made, and the new code snippet is the code after the changes were made. Each code snippet is a sequence of lines each with a line number.", + oldSnippet: diff.oldSnippet, + newSnippet: diff.newSnippet + }, + additionalContext: context.map(c => ({ type: c.type, description: c.description, context: c.context })), + rules: rules, + outputFormat: { + description: "The output must be a valid JSON object that conforms to the following JSON schema. Do NOT respond with anything other than the JSON object. Do NOT respond with the JSON object in a markdown code block.", + schema: JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2) + } + }; + + const formatSection = (title, content) => `# ${title}\n\n${content}`; + + // Cache the JSON schema conversion to avoid doing it on every call + const jsonSchema = JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2); + + const prompt = [ + sections.introduction, + formatSection("Input", sections.input.description), + "## Old Code Snippet", + "```", + sections.input.oldSnippet, + "```", + "## New Code Snippet", + "```", + sections.input.newSnippet, + "```", + formatSection("Additional Context", sections.additionalContext.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n")), + formatSection("Rules", `- ${sections.rules.join("\n- ")}`), + formatSection("Output Format (JSON Schema)", `${sections.outputFormat.description}\n${jsonSchema}`) + ].join("\n\n");You could also consider moving this prompt template to a separate file for better maintainability.
39-39: Cache the JSON schema conversion.Converting the Zod schema to a JSON schema on every function call could be inefficient, especially for complex schemas.
+// Move outside the function to cache the conversion +const diffReviewJsonSchema = JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2); export const generate_diff_review_prompt = async (diff: sourcebot_diff, context: sourcebot_context[], rules: string[]) => { // ... // Then use the cached schema in the template - ${JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2)} + ${diffReviewJsonSchema}
8-8: Typo in prompt text.There's a spelling error in the prompt text.
- You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide + You are an expert software engineer that excels at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you providepackages/agents/review-agent/src/nodes/generate_pr_reviews.ts (1)
34-36: Improve error tracking and reporting.Currently, errors during processing are logged but not aggregated, making it hard to determine the overall success rate.
+ const errors: {filename: string, error: string}[] = []; for (const file_diff of pr_payload.file_diffs) { // ... existing code } catch (error) { console.error(`Error fetching file content for ${file_diff.to}: ${error}`); + errors.push({filename: file_diff.to, error: error.message || String(error)}); } } console.log("Completed generate_pr_reviews"); + if (errors.length > 0) { + console.log(`Completed with ${errors.length} errors out of ${pr_payload.file_diffs.length} files`); + }packages/agents/review-agent/src/nodes/github_push_pr_reviews.ts (2)
21-21: Make diff side configurable.The side is hardcoded to "RIGHT", assuming reviews are always on the new version of the file.
- side: "RIGHT", + side: process.env.GITHUB_REVIEW_SIDE || "RIGHT",
30-32: Improve error message detail.The current error message doesn't provide enough context about which specific review comment failed.
- console.error(`Error pushing pr reviews for ${file_diff_review.filename}: ${error}`); + console.error(`Error pushing review comment for ${file_diff_review.filename} at line ${review.line_start}-${review.line_end}: ${error.status ? `HTTP ${error.status}: ` : ''}${error.message || String(error)}`);packages/agents/review-agent/src/nodes/github_pr_parser.ts (2)
26-35: Improve snippet formatting with newlines for better readability.The current implementation concatenates line content without newlines, making the snippets harder to read.
for (const change of chunk.changes) { if (change.type === "normal") { - oldSnippet += change.ln1 + ":" + change.content; - newSnippet += change.ln2 + ":" + change.content; + oldSnippet += change.ln1 + ":" + change.content + "\n"; + newSnippet += change.ln2 + ":" + change.content + "\n"; } else if (change.type === "add") { - newSnippet += change.ln + ":" + change.content; + newSnippet += change.ln + ":" + change.content + "\n"; } else if (change.type === "del") { - oldSnippet += change.ln + ":" + change.content; + oldSnippet += change.ln + ":" + change.content + "\n"; } }
61-61: Avoid non-null assertion for installation ID.Using the non-null assertion operator (!) is discouraged as it bypasses TypeScript's null checks. Since you've already validated the installation exists, simply use it directly.
- installation_id: payload.installation!.id, + installation_id: payload.installation.id,packages/agents/review-agent/src/nodes/fetch_file_content.ts (1)
15-23: Add timeout and retry logic for fetch operations.Network requests can fail or timeout. Adding timeout and retry logic improves resilience.
+ const MAX_RETRIES = 3; + const TIMEOUT_MS = 10000; + + let retries = 0; + let response; + + while (retries < MAX_RETRIES) { + try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), TIMEOUT_MS); + + response = await fetch('http://localhost:3000/api/source', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-Org-Domain': '~' + }, + body: JSON.stringify(fileSourceRequest), + signal: controller.signal + }); + + clearTimeout(timeoutId); + + if (response.ok) break; + retries++; + + if (retries < MAX_RETRIES) { + console.log(`Retry ${retries}/${MAX_RETRIES} for ${filename}`); + await new Promise(resolve => setTimeout(resolve, 1000 * retries)); + } + } catch (error) { + retries++; + if (error instanceof DOMException && error.name === 'AbortError') { + console.error(`Request timed out for ${filename}`); + } else { + console.error(`Error fetching ${filename}:`, error); + } + + if (retries >= MAX_RETRIES) throw error; + + console.log(`Retry ${retries}/${MAX_RETRIES} for ${filename}`); + await new Promise(resolve => setTimeout(resolve, 1000 * retries)); + } + } + - if (!response.ok) { + if (!response || !response.ok) { throw new Error(`Failed to fetch file content for ${filename} from ${repoPath}: ${response.statusText}`); }packages/agents/review-agent/src/types.ts (2)
17-18: Allow empty strings for PR title and description.In some cases, PRs might have empty descriptions. Consider using
z.string().optional().default("")for more flexibility.- title: z.string(), - description: z.string(), + title: z.string().min(1, "PR title cannot be empty"), + description: z.string().optional().default(""),
29-34: Add documentation for context type enum values.The enum for context types would benefit from documentation explaining what each type represents.
export const sourcebot_context_schema = z.object({ - type: z.enum(["pr_title", "pr_description", "pr_summary", "comment_chains", "file_content"]), + type: z.enum([ + "pr_title", // The title of the pull request + "pr_description", // The description/body of the pull request + "pr_summary", // A generated summary of the pull request + "comment_chains", // Existing comment threads on the pull request + "file_content" // The content of a specific file + ]), description: z.string().optional(), context: z.string(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.env.development(1 hunks)package.json(1 hunks)packages/agents/review-agent/.gitignore(1 hunks)packages/agents/review-agent/package.json(1 hunks)packages/agents/review-agent/src/app.ts(1 hunks)packages/agents/review-agent/src/env.ts(1 hunks)packages/agents/review-agent/src/nodes/fetch_file_content.ts(1 hunks)packages/agents/review-agent/src/nodes/generate_diff_review_prompt.ts(1 hunks)packages/agents/review-agent/src/nodes/generate_pr_reviews.ts(1 hunks)packages/agents/review-agent/src/nodes/github_pr_parser.ts(1 hunks)packages/agents/review-agent/src/nodes/github_push_pr_reviews.ts(1 hunks)packages/agents/review-agent/src/nodes/invoke_diff_review_llm.ts(1 hunks)packages/agents/review-agent/src/types.ts(1 hunks)packages/agents/review-agent/tsconfig.json(1 hunks)packages/web/src/app/[domain]/agents/page.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- package.json
- .env.development
- packages/agents/review-agent/src/env.ts
- packages/agents/review-agent/tsconfig.json
- packages/agents/review-agent/.gitignore
- packages/agents/review-agent/package.json
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/agents/review-agent/src/nodes/github_push_pr_reviews.ts (1)
packages/agents/review-agent/src/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_file_diff_review(47-47)
packages/agents/review-agent/src/app.ts (3)
packages/agents/review-agent/src/nodes/github_pr_parser.ts (1)
github_pr_parser(6-63)packages/agents/review-agent/src/nodes/generate_pr_reviews.ts (1)
generate_pr_reviews(6-49)packages/agents/review-agent/src/nodes/github_push_pr_reviews.ts (1)
github_push_pr_reviews(4-40)
packages/agents/review-agent/src/nodes/fetch_file_content.ts (3)
packages/agents/review-agent/src/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_context(34-34)packages/web/src/features/search/schemas.ts (1)
fileSourceResponseSchema(101-104)packages/web/src/lib/utils.ts (1)
base64Decode(191-194)
packages/agents/review-agent/src/nodes/github_pr_parser.ts (1)
packages/agents/review-agent/src/types.ts (3)
sourcebot_pr_payload(27-27)sourcebot_file_diff(14-14)sourcebot_diff(7-7)
packages/agents/review-agent/src/nodes/generate_diff_review_prompt.ts (1)
packages/agents/review-agent/src/types.ts (3)
sourcebot_diff(7-7)sourcebot_context(34-34)sourcebot_diff_review_schema(36-40)
packages/agents/review-agent/src/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/agents/review-agent/src/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/agents/review-agent/src/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/agents/review-agent/src/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (15)
packages/web/src/features/agents/review-agent/nodes/github_push_pr_reviews.ts (1)
4-37: Well-structured GitHub review comment creation with proper error handling.The implementation efficiently posts review comments to GitHub PRs with:
- Appropriate handling of single vs. multi-line comments
- Error handling at both individual comment and overall process levels
- Proper logging for tracking execution flow
Consider adding a delay between API calls to avoid hitting GitHub's rate limits:
try { await octokit.rest.pulls.createReviewComment({ owner: pr_payload.owner, repo: pr_payload.repo, pull_number: pr_payload.number, body: review.review, path: file_diff_review.filename, commit_id: pr_payload.head_sha, side: "RIGHT", ...(review.line_start === review.line_end ? { line: review.line_start } : { start_line: review.line_start, line: review.line_end, start_side: "RIGHT", }), }); + // Add a small delay to avoid hitting GitHub's rate limits + await new Promise(resolve => setTimeout(resolve, 1000)); } catch (error) {packages/web/src/app/api/(server)/webhook/route.ts (2)
10-24: Consider moving GitHub App initialization to a separate utility function.The module-level initialization of the GitHub App is appropriate but could be improved for maintainability.
-let githubApp: App | undefined; -if (env.GITHUB_APP_ID && env.GITHUB_APP_WEBHOOK_SECRET && env.GITHUB_APP_PRIVATE_KEY_PATH) { - try { - const privateKey = fs.readFileSync(env.GITHUB_APP_PRIVATE_KEY_PATH, "utf8"); - githubApp = new App({ - appId: env.GITHUB_APP_ID, - privateKey: privateKey, - webhooks: { - secret: env.GITHUB_APP_WEBHOOK_SECRET, - }, - }); - } catch (error) { - console.error(`Error initializing GitHub app: ${error}`); - } -} +import { initializeGitHubApp } from "@/utils/github"; + +// This will be lazily initialized when needed +let githubApp: App | undefined; + +function getGitHubApp(): App | undefined { + if (githubApp === undefined) { + githubApp = initializeGitHubApp( + env.GITHUB_APP_ID, + env.GITHUB_APP_WEBHOOK_SECRET, + env.GITHUB_APP_PRIVATE_KEY_PATH + ); + } + return githubApp; +}
26-28: Enhanced type guard for pull request events.The current implementation correctly identifies pull request events for "opened" and "synchronize" actions.
Consider adding support for more PR events like "reopened" that might also benefit from automated reviews:
function isPullRequestEvent(eventHeader: string, payload: unknown): payload is WebhookEventDefinition<"pull-request-opened"> | WebhookEventDefinition<"pull-request-synchronize"> { - return eventHeader === "pull_request" && typeof payload === "object" && payload !== null && "action" in payload && typeof payload.action === "string" && (payload.action === "opened" || payload.action === "synchronize"); + const supportedActions = ["opened", "synchronize", "reopened"]; + return eventHeader === "pull_request" && + typeof payload === "object" && + payload !== null && + "action" in payload && + typeof payload.action === "string" && + supportedActions.includes(payload.action); }packages/web/src/features/agents/review-agent/nodes/generate_diff_review_prompt.ts (4)
4-4: Function can be synchronousThis function doesn't perform any asynchronous operations internally, so it doesn't need to be marked as
async. While it works as-is, removing theasynckeyword would better reflect the function's synchronous nature.-export const generate_diff_review_prompt = async (diff: sourcebot_diff, context: sourcebot_context[], rules: string[]) => { +export const generate_diff_review_prompt = (diff: sourcebot_diff, context: sourcebot_context[], rules: string[]) => {
8-9: Fix typo in prompt textThere's a spelling error in the word "excells" which should be "excels".
- You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide + You are an expert software engineer that excels at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide
7-40: Consider reducing extra indentation in the prompt templateThe template string has 4-space indentation that will be included in the prompt sent to the LLM. This extra indentation isn't necessary and might make the prompt longer than needed. Consider removing the leading spaces to create a cleaner prompt.
const prompt = ` - You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide - must conform to all of the rules defined below. The output format of your review must conform to the output format defined below. +You are an expert software engineer that excels at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide +must conform to all of the rules defined below. The output format of your review must conform to the output format defined below. - # Input +# Input - The input is the old and new code snippets, which represent a single hunk from a git diff. The old code snippet is the code before the changes were made, and the new code snippet is the code after the changes were made. Each code snippet - is a sequence of lines each with a line number. +The input is the old and new code snippets, which represent a single hunk from a git diff. The old code snippet is the code before the changes were made, and the new code snippet is the code after the changes were made. Each code snippet +is a sequence of lines each with a line number.
30-30: Consider handling empty context arrayThe current implementation doesn't handle the case where the
contextarray is empty, which could result in an empty "Additional Context" section. Consider adding a check to output a meaningful message when no context is available.- ${context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n")} + ${context.length > 0 + ? context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n") + : "No additional context available."}packages/web/src/features/agents/review-agent/nodes/generate_pr_reviews.ts (2)
14-36: Handle errors from invoke_diff_review_llm separatelyThe current implementation catches all errors in the same catch block, not distinguishing between errors from
fetch_file_contentandinvoke_diff_review_llm. This could lead to confusion during debugging.Consider separating the error handling logic for better clarity:
try { const fileContentContext = await fetch_file_content(pr_payload, file_diff.to); const context: sourcebot_context[] = [ /* context items */ ]; const prompt = await generate_diff_review_prompt(diff, context, rules); + try { const diffReview = await invoke_diff_review_llm(prompt); reviews.push(diffReview); + } catch (llmError) { + console.error(`Error invoking LLM for review of ${file_diff.to}: ${llmError}`); + } } catch (error) { - console.error(`Error fetching file content for ${file_diff.to}: ${error}`); + console.error(`Error fetching file content or generating prompt for ${file_diff.to}: ${error}`); }
41-42: Consider handling renamed files explicitlyThe current implementation only uses
file_diff.tofor the filename, which might not properly handle renamed files. Consider keeping track of both the original and new filenames.file_diff_reviews.push({ filename: file_diff.to, + originalFilename: file_diff.from, + wasRenamed: file_diff.from !== file_diff.to, reviews: reviews, });packages/web/src/features/agents/review-agent/nodes/fetch_file_content.ts (1)
5-40: Consider implementing caching for repeated file content requestsThe current implementation fetches file content for each diff chunk individually, which could result in multiple requests for the same file. Consider implementing a caching mechanism to avoid redundant fetches:
+// Add to imports +import { LRUCache } from 'lru-cache'; +// Create a cache for file content (outside the function) +const fileContentCache = new LRUCache<string, sourcebot_context>({ + max: 100, // Maximum number of items to store + ttl: 1000 * 60 * 5, // 5 minute TTL +}); export const fetch_file_content = async (pr_payload: sourcebot_pr_payload, filename: string): Promise<sourcebot_context> => { console.log("Executing fetch_file_content"); + // Create a cache key combining repository and filename + const cacheKey = `${pr_payload.hostDomain}/${pr_payload.owner}/${pr_payload.repo}/${filename}`; + + // Check if file content is already in cache + const cachedContent = fileContentCache.get(cacheKey); + if (cachedContent) { + console.log(`Using cached content for ${filename}`); + return cachedContent; + } // Original fetch logic... const fileContentContext: sourcebot_context = { type: "file_content", description: `The content of the file ${filename}`, context: fileContent, } + // Store in cache + fileContentCache.set(cacheKey, fileContentContext); console.log("Completed fetch_file_content"); return fileContentContext; }packages/web/src/features/agents/review-agent/nodes/github_pr_parser.ts (3)
61-61: Avoid non-null assertion for installation_idThe code uses a non-null assertion (
!) for the installation ID, even though you've already checked for its presence earlier. Consider using a safer approach:-installation_id: payload.installation!.id, +installation_id: payload.installation?.id ?? + (() => { throw new Error("Installation ID should be present but is missing"); })(),Or alternatively, since you've already checked for the installation's presence at the start of the function, you can simply use:
-installation_id: payload.installation!.id, +installation_id: payload.installation.id,
16-48: Consider detecting and skipping binary filesThe current implementation doesn't handle binary files explicitly, which could lead to issues when trying to parse them as text. Consider adding detection and skipping of binary files:
const sourcebotFileDiffs: (sourcebot_file_diff | null)[] = parsedDiff.map((file) => { if (!file.from || !file.to) { console.log(`Skipping file due to missing from (${file.from}) or to (${file.to})`) return null; } + // Skip binary files that shouldn't be diffed + if (file.binary) { + console.log(`Skipping binary file: ${file.to}`); + return null; + } + + // Check file extension for common binary types + const binaryExtensions = ['.png', '.jpg', '.jpeg', '.gif', '.pdf', '.zip', '.exe']; + if (binaryExtensions.some(ext => file.to.toLowerCase().endsWith(ext))) { + console.log(`Skipping likely binary file based on extension: ${file.to}`); + return null; + } const diffs: sourcebot_diff[] = file.chunks.map((chunk) => { // Existing code }); return { from: file.from, to: file.to, diffs: diffs, } });
22-41: Handle large diffs more efficientlyThe current implementation might struggle with very large diffs. Consider adding limits or warnings for unusually large diffs:
const diffs: sourcebot_diff[] = file.chunks.map((chunk) => { + // Check for excessive chunk size that might impact performance + if (chunk.changes.length > 300) { + console.warn(`Large diff chunk with ${chunk.changes.length} changes in ${file.to}. This may impact performance.`); + } let oldSnippet = ""; let newSnippet = ""; + // Limit very large diffs to a reasonable size if needed + const changesToProcess = chunk.changes.length > 500 ? + chunk.changes.slice(0, 500) : + chunk.changes; + + if (changesToProcess.length < chunk.changes.length) { + console.warn(`Truncated diff chunk from ${chunk.changes.length} to ${changesToProcess.length} changes for ${file.to}`); + oldSnippet += "// Note: This diff was truncated as it was too large\n"; + newSnippet += "// Note: This diff was truncated as it was too large\n"; + } - for (const change of chunk.changes) { + for (const change of changesToProcess) { // Existing code for processing changes } return { oldSnippet: oldSnippet, newSnippet: newSnippet, } });packages/web/src/features/agents/review-agent/app.ts (2)
8-14: Consider extracting rules to a separate module.The rules array defines important constraints for the review agent, but it's embedded directly in this file. Extracting it to a separate module (e.g.,
review-rules.ts) would improve maintainability and allow reuse across different parts of the application.- const rules = [ - "Do NOT provide general feedback, summaries, explanations of changes, or praises for making good additions.", - "Do NOT provide any advice that is not actionable or directly related to the changes.", - "Focus solely on offering specific, objective insights based on the given context and refrain from making broad comments about potential impacts on the system or question intentions behind the changes.", - "Keep comments concise and to the point. Every comment must highlight a specific issue and provide a clear and actionable solution to the developer.", - "If there are no issues found on a line range, do NOT respond with any comments. This includes comments such as \"No issues found\" or \"LGTM\"." - ]Then import it:
import { REVIEW_RULES } from "@/features/agents/review-agent/constants/review-rules";
16-16: Add JSDoc comment to document the function.Adding a JSDoc comment would help explain the purpose, parameters, and expected behavior of this function.
+/** + * Processes a GitHub pull request webhook event to generate and post automated code reviews. + * Only processes events with action types "opened" or "synchronize". + * + * @param octokit - The authenticated Octokit instance for GitHub API calls + * @param payload - The GitHub webhook payload containing pull request data + */ export async function processGitHubPullRequest(octokit: Octokit, payload: WebhookEventDefinition<"pull-request-opened"> | WebhookEventDefinition<"pull-request-synchronize">) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
package.json(2 hunks)packages/web/package.json(2 hunks)packages/web/src/app/[domain]/browse/[...path]/page.tsx(1 hunks)packages/web/src/app/api/(server)/webhook/route.ts(1 hunks)packages/web/src/env.mjs(1 hunks)packages/web/src/features/agents/review-agent/app.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/fetch_file_content.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generate_diff_review_prompt.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generate_pr_reviews.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/github_pr_parser.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/github_push_pr_reviews.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/invoke_diff_review_llm.ts(1 hunks)packages/web/src/features/agents/review-agent/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/web/src/app/[domain]/browse/[...path]/page.tsx
- packages/web/src/env.mjs
- packages/web/src/features/agents/review-agent/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/web/src/app/api/(server)/webhook/route.ts (2)
packages/backend/src/env.ts (1)
env(22-52)packages/web/src/features/agents/review-agent/app.ts (1)
processGitHubPullRequest(16-27)
packages/web/src/features/agents/review-agent/nodes/github_push_pr_reviews.ts (1)
packages/web/src/features/agents/review-agent/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_file_diff_review(47-47)
packages/web/src/features/agents/review-agent/nodes/fetch_file_content.ts (3)
packages/web/src/features/agents/review-agent/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_context(34-34)packages/web/src/features/search/schemas.ts (1)
fileSourceResponseSchema(101-104)packages/web/src/lib/utils.ts (1)
base64Decode(191-194)
packages/web/src/features/agents/review-agent/nodes/generate_diff_review_prompt.ts (1)
packages/web/src/features/agents/review-agent/types.ts (3)
sourcebot_diff(7-7)sourcebot_context(34-34)sourcebot_diff_review_schema(36-40)
packages/web/src/features/agents/review-agent/nodes/github_pr_parser.ts (2)
packages/web/src/features/agents/review-agent/types.ts (3)
sourcebot_pr_payload(27-27)sourcebot_file_diff(14-14)sourcebot_diff(7-7)packages/web/src/app/error.tsx (1)
Error(14-54)
packages/web/src/features/agents/review-agent/nodes/generate_pr_reviews.ts (4)
packages/web/src/features/agents/review-agent/types.ts (4)
sourcebot_pr_payload(27-27)sourcebot_file_diff_review(47-47)sourcebot_diff_review(41-41)sourcebot_context(34-34)packages/web/src/features/agents/review-agent/nodes/fetch_file_content.ts (1)
fetch_file_content(5-40)packages/web/src/features/agents/review-agent/nodes/generate_diff_review_prompt.ts (1)
generate_diff_review_prompt(4-44)packages/web/src/features/agents/review-agent/nodes/invoke_diff_review_llm.ts (1)
invoke_diff_review_llm(5-41)
🔇 Additional comments (3)
packages/web/package.json (2)
121-123: Dependencies for GitHub integration and AI code reviews look appropriate.The added dependencies align with the new review agent feature:
octokitfor GitHub API interactionsopenaifor AI model accessparse-difffor handling Git diffs
141-142:✅ Verification successful
Ensure zod-to-json-schema is compatible with your zod version.
The addition of
zod-to-json-schemaalongside the existingzodpackage is appropriate for generating JSON schemas from Zod schemas, which will be useful for validating API responses.
🏁 Script executed:
#!/bin/bash # Check if zod-to-json-schema version 3.24.5 is compatible with zod version 3.24.2 npm view [email protected] peerDependenciesLength of output: 72
zod-to-json-schema Compatibility Verified
[email protected] declares a peerDependency of
zod: ^3.24.1, and your project useszod: ^3.24.2, which satisfies that range. No further action is needed.packages/web/src/features/agents/review-agent/nodes/invoke_diff_review_llm.ts (1)
8-11: Good error handling for missing API key.The check for the presence of the OPENAI_API_KEY environment variable is appropriate, failing fast with a clear error message.
packages/web/src/features/agents/review-agent/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/invoke_diff_review_llm.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/generate_pr_reviews.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/fetch_file_content.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/fetch_file_content.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/github_pr_parser.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/github_pr_parser.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (12)
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (2)
19-42: Add better error handling for OpenAI API responsesThe current implementation handles API errors but could be more robust when dealing with invalid or unexpected responses.
Enhance error handling, particularly for JSON parsing:
try { const completion = await openai.chat.completions.create({ model: "gpt-4.1-mini", messages: [ { role: "system", content: prompt } ] }); const openaiResponse = completion.choices[0].message.content; console.log("OpenAI response: ", openaiResponse); - const diffReviewJson = JSON.parse(openaiResponse || '{}'); + if (!openaiResponse) { + throw new Error("Empty response from OpenAI API"); + } + + let diffReviewJson; + try { + diffReviewJson = JSON.parse(openaiResponse); + } catch (parseError) { + console.error("Failed to parse OpenAI response as JSON:", openaiResponse); + throw new Error(`Invalid JSON response: ${parseError.message}`); + } + const diffReview = sourcebot_diff_review_schema.safeParse(diffReviewJson); if (!diffReview.success) { throw new Error(`Invalid diff review format: ${diffReview.error}`); } console.log("Completed invoke_diff_review_llm"); return diffReview.data; } catch (error) { console.error('Error calling OpenAI:', error); throw error; }
6-41: Consider using a structured logging approachThe code contains multiple console.log/console.error statements that would be better handled with a proper logging library.
Replace direct console logging with a structured logging approach that supports different environments and log levels. This would make debugging easier and allow for better production monitoring.
packages/web/src/features/agents/review-agent/nodes/generateDiffReviewPrompt.ts (2)
7-40: Improve prompt template maintainabilityThe current prompt is constructed as a large template literal, which can be difficult to maintain, test, and update.
Consider moving the prompt template to a separate file or breaking it into smaller, more manageable sections. This would make it easier to update and maintain the prompt structure.
For example:
+ import { readFileSync } from 'fs'; + import path from 'path'; + + const PROMPT_TEMPLATE = readFileSync( + path.join(process.cwd(), 'src/features/agents/review-agent/templates/diff_review_prompt.txt'), + 'utf8' + ); + export const generateDiffReviewPrompt = async (diff: sourcebot_diff, context: sourcebot_context[], rules: string[]) => { console.log("Executing generate_diff_review_prompt"); - const prompt = ` - You are an expert software engineer that excells at reviewing code changes. Given the input, additional context, and rules defined below, review the code changes and provide a detailed review. The review you provide - must conform to all of the rules defined below. The output format of your review must conform to the output format defined below. - - # Input - - The input is the old and new code snippets, which represent a single hunk from a git diff. The old code snippet is the code before the changes were made, and the new code snippet is the code after the changes were made. Each code snippet - is a sequence of lines each with a line number. - - ## Old Code Snippet - - \`\`\` - ${diff.oldSnippet} - \`\`\` - - ## New Code Snippet - - \`\`\` - ${diff.newSnippet} - \`\`\` - - # Additional Context - - ${context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n")} - - # Rules - - - ${rules.join("\n- ")} - - # Output Format (JSON Schema) - The output must be a valid JSON object that conforms to the following JSON schema. Do NOT respond with anything other than the JSON object. Do NOT respond with - the JSON object in a markdown code block. - ${JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2)} - `; + const contextSection = context.length > 0 + ? context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n") + : "No additional context provided."; + + const rulesSection = rules.length > 0 + ? `- ${rules.join("\n- ")}` + : "No specific rules provided."; + + const schemaJson = JSON.stringify(zodToJsonSchema(sourcebot_diff_review_schema), null, 2); + + const prompt = PROMPT_TEMPLATE + .replace('{{OLD_SNIPPET}}', diff.oldSnippet) + .replace('{{NEW_SNIPPET}}', diff.newSnippet) + .replace('{{CONTEXT}}', contextSection) + .replace('{{RULES}}', rulesSection) + .replace('{{JSON_SCHEMA}}', schemaJson);Then create a template file
diff_review_prompt.txtwith placeholders.
30-30: Add safeguards for empty context arraysThe context mapping doesn't check if the context array is empty, which could lead to unexpected results.
Add a check for empty context arrays:
- ${context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n")} + ${context.length > 0 + ? context.map(c => `${c.type}: ${c.description}\n\n${c.context}`).join("\n\n----------------------\n\n") + : "No additional context provided."}packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts (1)
8-32: Consider batch processing reviews for efficiencyThe current implementation processes reviews sequentially in nested loops, which could be inefficient for PRs with many files and comments.
Consider implementing batch processing or parallel processing with controlled concurrency:
import { Octokit } from "octokit"; import { sourcebot_pr_payload, sourcebot_file_diff_review } from "@/features/agents/review-agent/types"; +import pLimit from 'p-limit'; export const githubPushPrReviews = async (octokit: Octokit, pr_payload: sourcebot_pr_payload, file_diff_reviews: sourcebot_file_diff_review[]) => { console.log("Executing github_push_pr_reviews"); + const limit = pLimit(3); // Limit concurrent requests to 3 + const promises = []; try { for (const file_diff_review of file_diff_reviews) { for (const review of file_diff_review.reviews) { - try { - await octokit.rest.pulls.createReviewComment({ + promises.push(limit(() => octokit.rest.pulls.createReviewComment({ owner: pr_payload.owner, repo: pr_payload.repo, pull_number: pr_payload.number, body: review.review, path: file_diff_review.filename, commit_id: pr_payload.head_sha, side: "RIGHT", ...(review.line_start === review.line_end ? { line: review.line_start } : { start_line: review.line_start, line: review.line_end, start_side: "RIGHT", }), - }); - } catch (error) { - console.error(`Error pushing pr reviews for ${file_diff_review.filename}: ${error}`); - } + }).catch(error => { + console.error(`Error pushing pr reviews for ${file_diff_review.filename}: ${error}`); + }) + )); } } + + await Promise.all(promises); } catch (error) { console.error(`Error pushing pr reviews: ${error}`); } console.log("Completed github_push_pr_reviews"); }This would require adding
p-limitas a dependency, but it provides a clean way to handle concurrent requests with a limit.packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts (1)
34-36: Enhance error handling with retriesThe current error handling for file content fetching simply logs the error without attempting to retry or recover.
Implement a retry mechanism for transient failures:
import { sourcebot_pr_payload, sourcebot_diff_review, sourcebot_file_diff_review, sourcebot_context } from "@/features/agents/review-agent/types"; import { generateDiffReviewPrompt } from "@/features/agents/review-agent/nodes/generateDiffReviewPrompt"; import { invokeDiffReviewLlm } from "@/features/agents/review-agent/nodes/invokeDiffReviewLlm"; import { fetchFileContent } from "@/features/agents/review-agent/nodes/fetchFileContent"; +// Helper function to retry operations +const retryOperation = async <T>( + operation: () => Promise<T>, + retries = 3, + delay = 1000, + backoff = 2 +): Promise<T> => { + let lastError: Error; + for (let attempt = 0; attempt < retries; attempt++) { + try { + return await operation(); + } catch (error) { + lastError = error; + console.log(`Attempt ${attempt + 1} failed, retrying in ${delay}ms...`); + await new Promise(resolve => setTimeout(resolve, delay)); + delay *= backoff; // Exponential backoff + } + } + throw lastError; +}; export const generatePrReviews = async (pr_payload: sourcebot_pr_payload, rules: string[]): Promise<sourcebot_file_diff_review[]> => { console.log("Executing generate_pr_reviews"); const file_diff_reviews: sourcebot_file_diff_review[] = []; for (const file_diff of pr_payload.file_diffs) { const reviews: sourcebot_diff_review[] = []; for (const diff of file_diff.diffs) { try { - const fileContentContext = await fetchFileContent(pr_payload, file_diff.to); + const fileContentContext = await retryOperation(() => fetchFileContent(pr_payload, file_diff.to)); const context: sourcebot_context[] = [ { type: "pr_title", description: "The title of the pull request", context: pr_payload.title, }, { type: "pr_description", description: "The description of the pull request", context: pr_payload.description, }, fileContentContext, ]; const prompt = await generateDiffReviewPrompt(diff, context, rules); - const diffReview = await invokeDiffReviewLlm(prompt); + const diffReview = await retryOperation(() => invokeDiffReviewLlm(prompt)); reviews.push(diffReview); } catch (error) { console.error(`Error fetching file content for ${file_diff.to}: ${error}`); } }This implementation adds a retry mechanism with exponential backoff for operations that might fail due to transient issues.
packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts (1)
6-6: Use structured logging for better traceabilityThe console logs are basic strings without structured data, making it harder to trace and filter logs. Consider using a structured logging approach with consistent fields.
- console.log("Executing fetch_file_content"); + console.log({ + action: "fetch_file_content_start", + filename, + repo: `${pr_payload.owner}/${pr_payload.repo}` + }); // Later in the code: - console.log(JSON.stringify(fileSourceRequest, null, 2)); + console.log({ + action: "fetch_file_content_request", + request: fileSourceRequest + }); // And at the end: - console.log("Completed fetch_file_content"); + console.log({ + action: "fetch_file_content_complete", + filename, + success: true + });Also applies to: 13-13, 38-38
packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts (5)
18-18: Incomplete log message for skipped filesThe log message when skipping files with missing properties doesn't include the file name, making it difficult to identify which file was skipped.
- console.log(`Skipping file due to missing from (${file.from}) or to (${file.to})`) + console.log(`Skipping file ${file.index ?? 'unknown'} due to missing from (${file.from}) or to (${file.to})`)
28-29: Inconsistent line content formatThe code constructs line-prefixed snippets with a colon (
ln:content) but doesn't prefix the content with diff markers (+/-) that would help the AI understand line changes.for (const change of chunk.changes) { if (change.type === "normal") { - oldSnippet += change.ln1 + ":" + change.content + "\n"; - newSnippet += change.ln2 + ":" + change.content + "\n"; + oldSnippet += change.ln1 + ": " + change.content + "\n"; + newSnippet += change.ln2 + ": " + change.content + "\n"; } else if (change.type === "add") { - newSnippet += change.ln + ":" + change.content + "\n"; + newSnippet += change.ln + ": + " + change.content + "\n"; } else if (change.type === "del") { - oldSnippet += change.ln + ":" + change.content + "\n"; + oldSnippet += change.ln + ": - " + change.content + "\n"; } }Also applies to: 31-33
49-49: Type assertion can be avoided with proper filterThe code uses a type assertion to convert from
(sourcebot_file_diff | null)[]tosourcebot_file_diff[]. This can be avoided by using a type predicate function.- const filteredSourcebotFileDiffs: sourcebot_file_diff[] = sourcebotFileDiffs.filter((file) => file !== null) as sourcebot_file_diff[]; + // Type guard function + const isNotNull = <T>(value: T | null): value is T => value !== null; + const filteredSourcebotFileDiffs: sourcebot_file_diff[] = sourcebotFileDiffs.filter(isNotNull);
61-61: Non-null assertion can be avoidedThe code uses a non-null assertion on
payload.installation!.iddespite already checking for its existence earlier. This is unnecessary and could be avoided.- installation_id: payload.installation!.id, + installation_id: payload.installation.id,
7-7: Consider more informative loggingLike in the other file, the logging here is very basic. Consider using structured logging with more context about the PR being processed.
- console.log("Executing github_pr_parser"); + console.log({ + action: "github_pr_parser_start", + pr_number: payload.pull_request.number, + repo: `${payload.repository.owner.login}/${payload.repository.name}` + }); // And at the end: - console.log("Completed github_pr_parser"); + console.log({ + action: "github_pr_parser_complete", + pr_number: payload.pull_request.number, + files_processed: filteredSourcebotFileDiffs.length + });Also applies to: 51-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/web/src/app/api/(server)/webhook/route.ts(1 hunks)packages/web/src/features/agents/review-agent/app.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generateDiffReviewPrompt.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/app/api/(server)/webhook/route.ts
- packages/web/src/features/agents/review-agent/app.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/web/src/features/agents/review-agent/nodes/generateDiffReviewPrompt.ts (1)
packages/web/src/features/agents/review-agent/types.ts (3)
sourcebot_diff(7-7)sourcebot_context(34-34)sourcebot_diff_review_schema(36-40)
packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts (2)
packages/web/src/features/agents/review-agent/types.ts (3)
sourcebot_pr_payload(27-27)sourcebot_file_diff(14-14)sourcebot_diff(7-7)packages/web/src/app/error.tsx (1)
Error(14-54)
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (2)
packages/web/src/features/agents/review-agent/types.ts (2)
sourcebot_diff_review(41-41)sourcebot_diff_review_schema(36-40)packages/backend/src/env.ts (1)
env(22-52)
packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts (1)
packages/web/src/features/agents/review-agent/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_file_diff_review(47-47)
packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts (3)
packages/web/src/features/agents/review-agent/types.ts (2)
sourcebot_pr_payload(27-27)sourcebot_context(34-34)packages/web/src/features/search/schemas.ts (1)
fileSourceResponseSchema(101-104)packages/web/src/lib/utils.ts (1)
base64Decode(191-194)
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts
Outdated
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/web/src/app/[domain]/agents/page.tsx (1)
21-67: Add error handling and empty state handlingThe component doesn't handle cases where the agents array might be empty. While it's statically defined now, if this data eventually comes from an API, it's good practice to handle empty states.
<div className="flex flex-col items-center overflow-hidden min-h-screen"> <NavigationMenu domain={domain} /> <div className="w-full max-w-6xl px-4 mt-12 mb-24"> + {agents.length === 0 ? ( + <div className="flex flex-col items-center justify-center min-h-[60vh]"> + <p className="text-lg text-muted-foreground">No agents available at this time.</p> + </div> + ) : ( <div className={ agents.length === 1 ? "flex justify-center items-center min-h-[60vh]" : "grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-10" } > {agents.map((agent) => ( // Agent card content ))} </div> + )} </div> </div>
🧹 Nitpick comments (4)
packages/web/src/app/[domain]/agents/page.tsx (4)
7-15: Consider making the agents array configurable or fetched from an APIThe agents are currently hardcoded in the component. As more agents are added in the future, you might want to consider making this array configurable through environment variables, a configuration file, or fetched from an API.
49-53: Improve environment variable check logic and add more detailed status informationThe current environment variable check is complex and only verifies existence but not validity. Consider adding more detailed status information to help users troubleshoot configuration issues.
-{agent.requiredEnvVars.every(envVar => envVar in env && env[envVar as keyof typeof env] !== undefined) ? ( - <div className="text-green-500 font-semibold"> - Agent is configured and accepting requests on /api/webhook - </div> +{agent.requiredEnvVars.every(envVar => { + const value = env[envVar as keyof typeof env]; + return value !== undefined && value !== ''; +}) ? ( + <div className="flex items-center gap-2 text-green-500 font-semibold"> + <MdRocketLaunch className="text-lg" /> + <span>Agent is configured and accepting requests on /api/webhook</span> + </div>The
MdRocketLaunchicon is already imported but not used - this gives it a purpose.
54-61: Enhance accessibility for the Configure buttonWhile the button styling is good, consider adding ARIA attributes to improve accessibility. Also, add a tooltip or visual indicator that this link opens in a new tab.
<Link href={agent.configureUrl} target="_blank" rel="noopener noreferrer" - className="flex items-center justify-center gap-2 px-5 py-2.5 rounded-md bg-primary text-primary-foreground font-mono font-semibold text-base border border-primary shadow-sm hover:bg-primary/80 focus:outline-none focus:ring-2 focus:ring-primary/60 transition w-1/2" + className="flex items-center justify-center gap-2 px-5 py-2.5 rounded-md bg-primary text-primary-foreground font-mono font-semibold text-base border border-primary shadow-sm hover:bg-primary/80 focus:outline-none focus:ring-2 focus:ring-primary/60 transition w-1/2" + aria-label={`Configure ${agent.name}`} > - <FaCogs className="text-lg" /> Configure + <FaCogs className="text-lg" aria-hidden="true" /> + <span>Configure</span> + <span className="sr-only">(opens in a new tab)</span> </Link>
30-37: Extract card component for better maintainabilityThe agent card is a complex component that could be extracted to improve maintainability, especially as more agents are added.
You could create a separate
AgentCardcomponent to encapsulate this logic. For example:// components/AgentCard.tsx import Link from "next/link"; import { FaCogs } from "react-icons/fa"; import { MdRocketLaunch } from "react-icons/md"; import { env } from "@/env.mjs"; interface AgentProps { agent: { id: string; name: string; description: string; requiredEnvVars: string[]; configureUrl: string; }; fullWidth?: boolean; } export function AgentCard({ agent, fullWidth = false }: AgentProps) { const isConfigured = agent.requiredEnvVars.every( (envVar) => envVar in env && env[envVar as keyof typeof env] !== undefined ); return ( <div className={`relative flex flex-col items-center border border-border rounded-2xl p-8 bg-card shadow-xl ${ fullWidth ? "w-full max-w-xl" : "" }`} > {/* Card content... */} </div> ); }Then use it in your main component:
{agents.map((agent) => ( <AgentCard key={agent.id} agent={agent} fullWidth={agents.length === 1} /> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web/src/app/[domain]/agents/page.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/web/src/app/[domain]/agents/page.tsx (1)
packages/backend/src/env.ts (1)
env(22-52)
🔇 Additional comments (3)
packages/web/src/app/[domain]/agents/page.tsx (3)
1-6: LGTM: Clean and appropriate importsThe imports are well-organized and appropriate for the component's needs.
17-20: LGTM: Component signature and initial structureThe component signature is well-defined with proper TypeScript typing for the parameters.
38-46: LGTM: Clean name and description sectionThe name and description section is well-structured with appropriate styles and semantic HTML.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/docs/agents/overview.mdx (3)
10-11: Use correct verb form 'set up' instead of 'setup'.In line 10, “Once you've setup Sourcebot…” should use the verb phrase “set up” rather than the noun “setup”.
Apply this diff:
- Once you've setup Sourcebot, check out the + Once you've set up Sourcebot, check out the🧰 Tools
🪛 LanguageTool
[style] ~10-~10: In American English, “you” and “have” do not usually form a contraction unless they’re followed by a past participle.
Context: ...rcebot to perform a specific task. Once you've setup Sourcebot, check out the guides b...(IVE_I_HAVE_AMERICAN_STYLE)
10-11: Clarify the reference to 'guides below'.The text refers to “guides below” but the page displays agent cards, not guides. Consider replacing “guides” with “agent cards” or “agents” for accuracy.
Apply this diff:
- Once you've set up Sourcebot, check out the guides below to configure additional agents. + Once you've set up Sourcebot, check out the agent cards below to configure additional agents.🧰 Tools
🪛 LanguageTool
[style] ~10-~10: In American English, “you” and “have” do not usually form a contraction unless they’re followed by a past participle.
Context: ...rcebot to perform a specific task. Once you've setup Sourcebot, check out the guides b...(IVE_I_HAVE_AMERICAN_STYLE)
7-7: Ensure consistent punctuation in the note.The sentence in the
<Note>block should end with a period for readability.Apply this diff:
- Have an idea for an agent that we haven't built? Submit a [feature request](https://github.com/sourcebot-dev/sourcebot/discussions/categories/feature-requests) on our GitHub + Have an idea for an agent that we haven't built? Submit a [feature request](https://github.com/sourcebot-dev/sourcebot/discussions/categories/feature-requests) on our GitHub.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/images/github_app_private_key.pngis excluded by!**/*.pngdocs/images/review_agent_configured.pngis excluded by!**/*.pngdocs/images/review_agent_example.pngis excluded by!**/*.png
📒 Files selected for processing (5)
docs/docs.json(1 hunks)docs/docs/agents/overview.mdx(1 hunks)docs/docs/agents/review-agent.mdx(1 hunks)packages/web/src/app/[domain]/agents/page.tsx(1 hunks)packages/web/src/app/[domain]/components/navigationMenu.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/docs.json
- packages/web/src/app/[domain]/components/navigationMenu.tsx
- docs/docs/agents/review-agent.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/app/[domain]/agents/page.tsx
🧰 Additional context used
🪛 LanguageTool
docs/docs/agents/overview.mdx
[style] ~10-~10: In American English, “you” and “have” do not usually form a contraction unless they’re followed by a past participle.
Context: ...rcebot to perform a specific task. Once you've setup Sourcebot, check out the guides b...
(IVE_I_HAVE_AMERICAN_STYLE)
|
@CodeRabbit /review |
|
Tip For best results, initiate chat on the files or code changes. @msukkari I've received your request to review this pull request. I'll trigger an incremental review of your PR now. The review will analyze the changes you've made to implement the new GitHub PR review agent feature. The review will examine:
The review process may take a few minutes. Once complete, you'll receive comments on specific parts of your code if any issues are found. If you have any specific questions about your implementation while waiting, feel free to ask! |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/backend/src/index.ts (1)
3-11: 🛠️ Refactor suggestionUnused import: ArgumentParser
The
ArgumentParserimport fromargparseis no longer used after removing command-line argument parsing, but it's still being imported.import * as Sentry from "@sentry/node"; -import { ArgumentParser } from "argparse"; import { existsSync } from 'fs'; import { mkdir } from 'fs/promises'; import path from 'path'; import { AppContext } from "./types.js"; import { main } from "./main.js" import { PrismaClient } from "@sourcebot/db"; import { env } from "./env.js";
♻️ Duplicate comments (1)
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (1)
23-24: Consider making the model configurable instead of hardcodingThe model name "gpt-4.1-mini" is hardcoded, which could create maintenance challenges when new models are released or if different models are needed for different environments.
🧹 Nitpick comments (4)
packages/backend/src/index.ts (1)
40-40: Debug log statement should use a loggerA raw
console.logcall to print the current working directory appears to be a debug statement. Consider using a proper logging framework with appropriate log levels to avoid polluting production logs.-console.log(process.cwd()); +logger.debug(`Current working directory: ${process.cwd()}`);packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (1)
18-20: Consider the security and performance impact of extensive loggingLogging entire prompts and responses to files could lead to security concerns (if prompts contain sensitive data) and performance issues (with large responses).
Consider adding truncation or sanitization for logs:
if (reviewAgentLogPath) { - fs.appendFileSync(reviewAgentLogPath, `\n\nPrompt:\n${prompt}`); + // Truncate long prompts in logs to prevent excessive file sizes + const truncatedPrompt = prompt.length > 1000 ? prompt.substring(0, 1000) + '... [truncated]' : prompt; + fs.appendFileSync(reviewAgentLogPath, `\n\nPrompt:\n${truncatedPrompt}`); } // Similar change for response loggingAlso applies to: 31-33
packages/web/src/app/api/(server)/webhook/route.ts (2)
69-74: Extract repeated installation validation into a helper functionThe installation validation logic is duplicated in both the pull request and issue comment event handlers.
+ function validateInstallation(installation: unknown): number { + if (!installation) { + throw new Error('Installation object is not present'); + } + return (installation as { id: number }).id; + } // Then in the PR handler: - if (!body.installation) { - console.error('Received github pull request event but installation is not present'); - return Response.json({ status: 'ok' }); - } - - const installationId = body.installation.id; + let installationId; + try { + installationId = validateInstallation(body.installation); + } catch (error) { + console.error(`Error in pull request handler: ${error.message}`); + return Response.json({ status: 'ok' }); + } // And similarly in the issue comment handlerAlso applies to: 91-94
42-44: Consider refactoring type guards to improve readabilityThe current implementation of the type guards is hard to read due to multiple conditions on a single line.
- function isPullRequestEvent(eventHeader: string, payload: unknown): payload is WebhookEventDefinition<"pull-request-opened"> | WebhookEventDefinition<"pull-request-synchronize"> { - return eventHeader === "pull_request" && typeof payload === "object" && payload !== null && "action" in payload && typeof payload.action === "string" && (payload.action === "opened" || payload.action === "synchronize"); - } + function isPullRequestEvent( + eventHeader: string, + payload: unknown + ): payload is WebhookEventDefinition<"pull-request-opened"> | WebhookEventDefinition<"pull-request-synchronize"> { + if (eventHeader !== "pull_request") return false; + if (typeof payload !== "object" || payload === null) return false; + if (!("action" in payload) || typeof (payload as any).action !== "string") return false; + + const action = (payload as any).action; + return action === "opened" || action === "synchronize"; + }Apply similar formatting to the
isIssueCommentEventfunction.Also applies to: 46-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.env.development(3 hunks)docs/docs/agents/review-agent.mdx(1 hunks)packages/backend/src/env.ts(1 hunks)packages/backend/src/index.ts(2 hunks)packages/web/src/app/api/(server)/webhook/route.ts(1 hunks)packages/web/src/env.mjs(2 hunks)packages/web/src/features/agents/review-agent/app.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts(1 hunks)packages/web/src/features/agents/review-agent/types.ts(1 hunks)supervisord.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .env.development
- packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts
- packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts
- docs/docs/agents/review-agent.mdx
- packages/web/src/features/agents/review-agent/app.ts
- packages/web/src/env.mjs
- packages/web/src/features/agents/review-agent/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/backend/src/index.ts (1)
packages/backend/src/env.ts (1)
env(22-54)
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (2)
packages/web/src/features/agents/review-agent/types.ts (2)
sourcebot_diff_review(43-43)sourcebot_diff_review_schema(38-42)packages/backend/src/env.ts (1)
env(22-54)
🔇 Additional comments (2)
supervisord.conf (1)
25-25: Command-line argument removal aligns with environment variable changeThe removal of the
--cacheDircommand-line argument from the backend startup command matches the addition of theDATA_CACHE_DIRenvironment variable in the environment schema. This change centralizes configuration through environment variables rather than CLI arguments.packages/backend/src/index.ts (1)
42-42: Environment variable replaces command-line argumentUsing
env.DATA_CACHE_DIRinstead of CLI arguments is a good improvement that centralizes configuration. This change aligns with modern practices for containerized applications and makes configuration more consistent.
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts
Show resolved
Hide resolved
packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Improvements
Chores