Conversation
|
@BugBot run |
Summary of ChangesHello @wwwillchen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new automated skill designed to streamline the addition of AI model configurations by researching official documentation and updating relevant constants. Concurrently, it refactors the existing route parsing logic within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR adds support for three new AI models (Claude Sonnet 4.6, Gemini 3.1 Pro Preview, and MiniMax M2.5) and introduces a comprehensive skill definition for adding models in the future. Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 4f85b87 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new add-models skill and refactors the route parsing logic in useParseRouter.ts for improved consistency and testability. However, security vulnerabilities were found: a Medium-severity ReDoS in the route parsing and a High-severity LLM Safety issue in the new skill definition, potentially leading to code injection. The LLM Safety issue is highlighted with a rule reference emphasizing the need for input sanitization. Further code review feedback includes suggestions for filtering dynamic routes from React Router parsing and removing redundant code to enhance maintainability.
| 4. **Research each model's specifications:** | ||
|
|
||
| For each model, use WebSearch and WebFetch to look up the official documentation: | ||
| - **Anthropic models**: Search `docs.anthropic.com` for model specs | ||
| - **OpenAI models**: Search `platform.openai.com/docs/models` for model specs | ||
| - **Google Gemini models**: Search `ai.google.dev/gemini-api/docs/models` for model specs | ||
| - **xAI models**: Search `docs.x.ai/docs/models` for model specs | ||
| - **OpenRouter models**: Search `openrouter.ai/<provider>/<model-name>` for model specs and pricing | ||
|
|
||
| For each model, determine: | ||
| - **API model name**: The exact string used in API calls (e.g., `claude-sonnet-4-5-20250929`, `gemini-2.5-pro`) | ||
| - **Display name**: Human-readable name (e.g., "Claude Sonnet 4.5", "Gemini 2.5 Pro") | ||
| - **Description**: Short description following the style of existing entries | ||
| - **Max output tokens**: The model's maximum output token limit | ||
| - **Context window**: The model's total context window size | ||
| - **Temperature**: Default temperature (0 for most models, 1 for OpenAI, 1.0 for Gemini 3.x models) | ||
| - **Dollar signs**: Cost tier from 0-6 based on pricing relative to other models in the same provider | ||
|
|
||
| **Dollar signs guide** (approximate, based on per-million-token input pricing): | ||
| - 0: Free | ||
| - 1: Very cheap (<$0.50/M input tokens) | ||
| - 2: Cheap ($0.50-$2/M) | ||
| - 3: Moderate ($2-$8/M) | ||
| - 4: Expensive ($8-$15/M) | ||
| - 5: Very expensive ($15-$30/M) | ||
| - 6: Premium ($30+/M) |
There was a problem hiding this comment.
This skill definition creates a 'Data-to-Code' pipeline where an AI agent is instructed to fetch model specifications from external websites and write them directly into the codebase (src/ipc/shared/language_model_constants.ts). This is vulnerable to Indirect Prompt Injection. An attacker who controls the content of the documentation pages fetched by the agent could trick it into injecting malicious code, backdoors, or logic flaws into the repository. The instructions should be updated to include strict validation and sanitization requirements for all data fetched from external sources before it is written to the codebase.
References
- Sanitize user-provided input, such as keywords, before incorporating it into an LLM prompt to prevent prompt injection. Sanitization should include stripping HTML-like tags, removing code block markers, and enforcing length limits.
| it("should include dynamic routes with params (they are valid navigation targets with placeholders)", () => { | ||
| const content = ` | ||
| <Routes> | ||
| <Route path="/users/:id" element={<User />} /> | ||
| </Routes> | ||
| `; | ||
| const routes = parseRoutesFromRouterFile(content); | ||
| expect(routes).toHaveLength(1); | ||
| expect(routes[0].path).toBe("/users/:id"); | ||
| }); |
There was a problem hiding this comment.
This test asserts that dynamic routes with params are included. However, for consistency with how Next.js routes are parsed (which excludes them) and for the purpose of a "quick navigation" feature, these routes should be excluded as they are not directly navigable. This test should be updated to assert that dynamic routes are not included.
it("should NOT include dynamic routes with params as they are not directly navigable", () => {
const content = `
<Routes>
<Route path="/users/:id" element={<User />} />
<Route path="/dashboard" element={<Dashboard />} />
</Routes>
`;
const routes = parseRoutesFromRouterFile(content);
expect(routes).toHaveLength(1);
expect(routes[0].path).toBe("/dashboard");
});| // Skip wildcard/catch-all routes like "*" - they are not valid navigation targets | ||
| // and cause 'Invalid URL' TypeError when clicked | ||
| if (path === "*" || path === "/*") continue; |
There was a problem hiding this comment.
The parseRoutesFromRouterFile function currently includes dynamic routes (e.g., /users/:id), while parseRoutesFromNextFiles correctly excludes them. For consistency, and because dynamic routes are not directly navigable without parameters, they should also be excluded here. This will make the "quick navigation" feature more reliable and consistent across different project types.
| // Skip wildcard/catch-all routes like "*" - they are not valid navigation targets | |
| // and cause 'Invalid URL' TypeError when clicked | |
| if (path === "*" || path === "/*") continue; | |
| // Skip wildcard/catch-all and dynamic routes as they are not valid navigation targets | |
| // and can cause errors or are not directly navigable. | |
| if (path === "*" || path === "/*" || path.includes(":")) continue; |
|
|
||
| try { | ||
| const parsedRoutes: ParsedRoute[] = []; | ||
| const routePathsRegex = /<Route\s+(?:[^>]*\s+)?path=["']([^"']+)["']/g; |
There was a problem hiding this comment.
The regular expression used to parse routes is vulnerable to Regular Expression Denial of Service (ReDoS). The overlapping sub-patterns [^>]* and \s+ within the optional group (?:[^>]*\s+)? can cause catastrophic backtracking when processing long strings inside a <Route> tag that do not contain the path attribute. This could hang the UI thread if a large or maliciously crafted file is processed.
| const routePathsRegex = /<Route\s+(?:[^>]*\s+)?path=["']([^"']+)["']/g; | |
| const routePathsRegex = /<Route\s+[^>]*?path=["']([^"']+)["']/g; |
| - **OpenAI** (`openai`): GPT models | ||
| - **Google** (`google`): Gemini models | ||
| - **xAI** (`xai`): Grok models | ||
| - **OpenRouter** (`openrouter`): Models from other providers (DeepSeek, Qwen, Moonshot/Kimi, Z-AI/GLM, etc.) |
There was a problem hiding this comment.
For consistency with the model names used in src/ipc/shared/language_model_constants.ts (e.g., z-ai/glm-5), it's better to use the lowercase z-ai/glm here. The current Z-AI/GLM might cause confusion or require extra normalization steps for the agent following these instructions.
| - **OpenRouter** (`openrouter`): Models from other providers (DeepSeek, Qwen, Moonshot/Kimi, Z-AI/GLM, etc.) | |
| - **OpenRouter** (`openrouter`): Models from other providers (DeepSeek, Qwen, Moonshot/Kimi, z-ai/glm, etc.) |
| const lower = file.toLowerCase(); | ||
| if ( | ||
| lower === "app/page.tsx" || | ||
| lower === "app/page.jsx" || | ||
| lower === "app/page.js" || | ||
| lower === "app/page.mdx" || | ||
| lower === "app/page.ts" || | ||
| lower === "src/app/page.tsx" || | ||
| lower === "src/app/page.jsx" || | ||
| lower === "src/app/page.js" || | ||
| lower === "src/app/page.mdx" || | ||
| lower === "src/app/page.ts" | ||
| ) { | ||
| nextRoutes.add("/"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This block of code to specifically handle root-level page files (e.g., app/page.tsx, src/app/page.tsx) is redundant. The subsequent logic using appPageRegex already correctly handles these cases by matching them and producing an empty routeSeg, which then correctly resolves to the root path /. Removing this block will simplify the code and improve maintainability without changing functionality.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const path = match[1]; | ||
| // Skip wildcard/catch-all routes like "*" - they are not valid navigation targets | ||
| // and cause 'Invalid URL' TypeError when clicked | ||
| if (path === "*" || path === "/*") continue; |
There was a problem hiding this comment.
🟡 Wildcard route filtering is incomplete — paths like /dashboard/* are not excluded
The wildcard filter at line 44 only checks for exact "*" and "/*" paths, but React Router apps commonly use nested wildcard routes like <Route path="/dashboard/*" ... />. These paths still contain *, which is not a valid URL path character and will cause the same Invalid URL TypeError the filter was designed to prevent.
Root Cause and Impact
The filter if (path === "*" || path === "/*") continue; is too narrow. When the parsed route /dashboard/* is displayed in the navigation dropdown (src/components/preview_panel/PreviewIframe.tsx:1159), clicking it calls navigateToRoute("/dashboard/*"), which eventually constructs a URL containing the literal * character. This triggers the same Invalid URL TypeError that the comment on line 42-43 warns about.
A more robust check would use path.includes("*") or path.endsWith("/*") to catch all wildcard patterns:
if (path.includes("*")) continue;| if (path === "*" || path === "/*") continue; | |
| if (path.includes("*")) continue; |
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0585f1876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const path = match[1]; | ||
| // Skip wildcard/catch-all routes like "*" - they are not valid navigation targets | ||
| // and cause 'Invalid URL' TypeError when clicked | ||
| if (path === "*" || path === "/*") continue; |
There was a problem hiding this comment.
/* routes in parsed navigation list
The new wildcard filter drops path === "/*", but this pattern is a valid React Router route and is commonly used as the top-level route for nested routing trees. In apps that declare only /* in App.tsx, this change removes the only discovered route from quick navigation, even though navigation to that absolute path is valid; the Invalid URL problem applies to bare "*", not "/*".
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const path = match[1]; | ||
| // Skip wildcard/catch-all routes like "*" - they are not valid navigation targets | ||
| // and cause 'Invalid URL' TypeError when clicked | ||
| if (path === "*" || path === "/*") continue; |
There was a problem hiding this comment.
🟡 MEDIUM | edge-case
Incomplete wildcard route filtering misses nested catch-all patterns
The wildcard filter only checks for exact "*" and "/*" paths, but React Router also supports nested catch-all routes like "/admin/*" or "/docs/*". These routes contain * and would similarly cause the 'Invalid URL' TypeError mentioned in the comment, yet they pass through this filter.
💡 Suggestion: Use path.includes('*') instead of exact equality checks:
| if (path === "*" || path === "/*") continue; | |
| if (path.includes('*')) continue; |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (2 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
wwwillchen
force-pushed
the
agent--1771960379784-1771959504
branch
from
d0585f1 to
658dbaf
Compare
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. ✅ No blocking issues found. This is a well-structured skill definition that adds clear, actionable instructions for adding AI model configurations. 🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
Add three new models to language_model_constants.ts: - Gemini 3.1 Pro (Preview) in Google provider - Claude Sonnet 4.6 in Anthropic provider - MiniMax M2.5 in OpenRouter provider Replace SONNET_4_5 constant with SONNET_4_6 and update smart auto model references to use Sonnet 4.6. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@BugBot run |
| name: "gemini-3-pro-preview", | ||
| displayName: "Gemini 3 Pro (Preview)", | ||
| description: "Google's latest Gemini model", | ||
| name: "gemini-3.1-pro-preview", |
There was a problem hiding this comment.
🔴 Theme generation sends stale model name gemini-3-pro-preview after rename to gemini-3.1-pro-preview
When a user selects the Gemini model for AI theme generation, the THEME_GENERATION_MODEL_MAP in themes_handlers.ts:40 still maps to the old model name gemini-3-pro-preview, but this PR renamed that model to gemini-3.1-pro-preview in the constants file. This causes the theme generation API call to use a model name that no longer exists.
Root Cause and Impact
The PR changed the Google model entry in src/ipc/shared/language_model_constants.ts:174 from name: "gemini-3-pro-preview" to name: "gemini-3.1-pro-preview", but did not update the hardcoded model name in src/pro/main/ipc/handlers/themes_handlers.ts:40:
const THEME_GENERATION_MODEL_MAP = {
"gemini-3-pro": { provider: "google", name: "gemini-3-pro-preview" }, // stale!
...
};This name value is passed directly to getModelClient() at themes_handlers.ts:614 and themes_handlers.ts:840, which uses it as the API model identifier sent to Google's API. Since the model was renamed (presumably because Google updated the model), the old name gemini-3-pro-preview will likely result in an API error when users try to generate themes with the default Gemini model selection.
Impact: Theme generation (both from images and from URLs) will fail for all users who select the default Gemini model option.
Prompt for agents
Update the THEME_GENERATION_MODEL_MAP in src/pro/main/ipc/handlers/themes_handlers.ts line 40 to use the new model name. Change:
"gemini-3-pro": { provider: "google", name: "gemini-3-pro-preview" },
to:
"gemini-3-pro": { provider: "google", name: "gemini-3.1-pro-preview" },
Also consider whether the UI-facing key "gemini-3-pro" (used in src/ipc/types/templates.ts:97 and src/components/AIGeneratorTab.tsx:27,519,521) should be updated to reflect the new model version name.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3143682411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name: "gemini-3-pro-preview", | ||
| displayName: "Gemini 3 Pro (Preview)", | ||
| description: "Google's latest Gemini model", | ||
| name: "gemini-3.1-pro-preview", |
There was a problem hiding this comment.
Changing the Google flagship model ID to gemini-3.1-pro-preview here leaves the theme-generation path pinned to the old gemini-3-pro-preview ID (THEME_GENERATION_MODEL_MAP in src/pro/main/ipc/handlers/themes_handlers.ts:40), so users selecting Gemini for theme generation do not actually use the newly added model and may fail once the old preview is unavailable. This model-ID update should be applied to that map (and related theme model metadata) in the same change to avoid drift.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
Details on HIGH issue: The 💡 Suggestion: Update 🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (3 items)
Generated by Dyadbot multi-agent code review |
🎭 Playwright Test Results✅ All tests passed!
Total: 234 tests passed (10 flaky) (6 skipped)
|
|
@BugBot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| description: "Google's latest Gemini model", | ||
| name: "gemini-3.1-pro-preview", | ||
| displayName: "Gemini 3.1 Pro (Preview)", | ||
| description: "Google's most capable Gemini model", |
There was a problem hiding this comment.
Theme generation uses removed Gemini model
High Severity
The diff replaces gemini-3-pro-preview with gemini-3.1-pro-preview in MODEL_OPTIONS, but THEME_GENERATION_MODEL_MAP in themes_handlers.ts still maps the Gemini theme option to gemini-3-pro-preview. Theme generation therefore uses a model that was removed from the codebase, which can cause API errors if Google has deprecated it, or at least leaves the feature using a deprecated model.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| name: "gemini-3-pro-preview", | ||
| displayName: "Gemini 3 Pro (Preview)", | ||
| description: "Google's latest Gemini model", | ||
| name: "gemini-3.1-pro-preview", |
There was a problem hiding this comment.
Model name was changed from gemini-3-pro-preview to gemini-3.1-pro-preview, but src/pro/main/ipc/handlers/themes_handlers.ts:40 still references the old name.
| name: "gemini-3.1-pro-preview", | |
| name: "gemini-3.1-pro-preview", |
Also update themes_handlers.ts line 40 to use the new name.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ipc/shared/language_model_constants.ts
Line: 174
Comment:
Model name was changed from `gemini-3-pro-preview` to `gemini-3.1-pro-preview`, but `src/pro/main/ipc/handlers/themes_handlers.ts:40` still references the old name.
```suggestion
name: "gemini-3.1-pro-preview",
```
Also update `themes_handlers.ts` line 40 to use the new name.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Test plan
🤖 Generated with Claude Code