- Notifications
You must be signed in to change notification settings - Fork6
preview/ast-grep#65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
coderabbitaibot commentedJul 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughThis update introduces new configuration and linting rules for JavaScript and TypeScript codebases, focusing on variable naming and console usage. It also adds new utility functions, a React component, and a TypeScript async function for demonstration. The configuration enables early access features and integrates custom AST-grep rules and packages. Changes
Sequence Diagram(s)sequenceDiagram participant User participant greet.js participant Console participant HelloWorld (React) User->>greet.js: call greet(name, surname) greet.js->>Console: print greeting message User->>greet.js: call welcome(name, surname) greet.js->>Console: print welcome message User->>HelloWorld (React): render <HelloWorld greeting, greeted, ...> HelloWorld (React)->>Console: (optional) print based on propssequenceDiagram participant Developer participant AST-grep Linter participant Source Files Developer->>AST-grep Linter: run lint rules AST-grep Linter->>Source Files: scan for console usage and const naming AST-grep Linter-->>Developer: report violations or suggest fixesPoem
Impact AnalysisNew greeting utilities and HelloWorld React componentℹ️ Low 🔗 Impacts API 🔄 Impacts behavior Adds a new module exporting console-based greeting functions ( Review prop defaults, styling, and event handling in the Test console output of 🔍 Related Files
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 YAMLlint (1.37.1).coderabbit.yaml[Errno 2] No such file or directory: '.coderabbit.yaml' rules/no-console-except-error.yml[Errno 2] No such file or directory: 'rules/no-console-except-error.yml' rules/replace-unnecessary-type-definition-use-state.yml[Errno 2] No such file or directory: 'rules/replace-unnecessary-type-definition-use-state.yml'
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport 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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
.coderabbit.yaml (1)
1-14:Missing ast-grep rule directories in configurationThe directories referenced under
ast-grep.rule_dirsdo not exist in the repo, so your custom rules won’t be loaded:
- “rules/”
- “test-missing-dir/”
Also ensure your custom package is installed or reachable:
test-peter-rabbit/test-ast-grep-custom-packagePlease update your
.coderabbit.yamlor repo structure accordingly:• Create the missing directories and add your AST-Grep rules there
• Or remove/adjust the paths inast-grep.rule_dirs
• Confirm that the packagetest-peter-rabbit/test-ast-grep-custom-packageis available (e.g., installed or in your project’s path)[coderabbit.yaml lines 7–11]
🧹 Nitpick comments (1)
rules/replace-unnecessary-type-definition-use-state.yml (1)
1-8:Effective rule for removing unnecessary TypeScript type annotations.The rule correctly identifies and fixes unnecessary explicit
<string>type parameters inuseStatecalls, allowing TypeScript to infer the type from the initial value.Consider extending this rule to handle other common unnecessary type annotations:
rule: any: - pattern: useState<string>($A)+ - pattern: useState<number>($A)+ - pattern: useState<boolean>($A)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.coderabbit.yaml(1 hunks)greet.js(1 hunks)jsx_ast_grep_failure.ts(1 hunks)rules/no-console-except-error.yml(1 hunks)rules/replace-unnecessary-type-definition-use-state.yml(1 hunks)rules/ts-const.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts}
Instructions used from:
Sources:
⚙️CodeRabbit Configuration File
🪛 ast-grep (0.38.6)
jsx_ast_grep_failure.ts
[hint] 11-11:
Context: await (new Promise(() => {console.log("logging this long task")}))
Suggested replacement: (new Promise(() => {console.log("logging this long task")}))
(no-await-in-promise-all-custom)
🪛 Biome (1.9.4)
greet.js
[error] 75-75: Illegal return statement outside of a function
(parse)
[error] 75-76: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
🔇 Additional comments (5)
rules/no-console-except-error.yml (1)
1-14:Well-structured ast-grep rule for console usage restrictions.The rule correctly implements the intended behavior:
- Restricts
console.log,console.debug, andconsole.warneverywhere- Allows
console.erroronly inside catch clauses- Uses appropriate pattern matching with constraints and negation logic
The implementation follows ast-grep best practices with clear pattern definitions and proper constraint usage.
rules/ts-const.yaml (1)
3-16:Well-designed rule for enforcing uppercase constants.The rule correctly:
- Matches both typed and untyped const declarations
- Uses regex to identify non-uppercase variable names
- Restricts checking to top-level program scope
- Provides clear warning messages
The regex pattern
.*[^A-Z_].*effectively catches any name containing non-uppercase characters or characters other than underscores.greet.js (3)
58-63:PropTypes definition looks good.The PropTypes are properly defined and cover all the props used in the component.
98-104:Correct console usage in catch block.The function correctly uses
console.errorin the catch block, which is allowed by the AST-grep linting rules.
6-8:Console.log usage violates linting rules.The
console.logstatement violates the AST-grep linting rule that disallowsconsole.log,console.debug, andconsole.warnexcept forconsole.errorinside catch clauses.Consider replacing with
console.errorif this is for error logging, or remove the console statement if it's for debugging purposes:-console.log(`Hello, ${name} ${surname}!`)+console.error(`Hello, ${name} ${surname}!`)Likely an incorrect or invalid review comment.
| @@ -0,0 +1,15 @@ | |||
| id:ts-const | |||
| language:Typescript | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fix language specification capitalization.
The language should be lowercase "typescript" to match ast-grep's language specifications.
-language: Typescript+language: typescript
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| language:Typescript | |
| language:typescript |
🤖 Prompt for AI Agents
In rules/ts-const.yaml at line 2, change the language specification from"Typescript" to lowercase "typescript" to comply with ast-grep's expectedlanguage format.| @@ -0,0 +1,22 @@ | |||
| import{useState}from'react' | |||
| constgreeting:string="Hello, world!"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Constant variable name violates coding guidelines.
According to the coding guidelines, all constant variables in JS/TS files must be defined using UPPER CASE letters.
-const greeting: string = "Hello, world!";+const GREETING: string = "Hello, world!";
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constgreeting:string="Hello, world!"; | |
| constGREETING:string="Hello, world!"; |
🤖 Prompt for AI Agents
In jsx_ast_grep_failure.ts at line 3, the constant variable name 'greeting'violates the coding guideline requiring constant variables to be in UPPER CASE.Rename the variable to 'GREETING' to comply with the naming convention.| const[state,setState]=useState<string>("test string") | ||
| const[state2,setState2]=useState<string>("test string") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove unnecessary TypeScript type annotations.
The explicit<string> type parameters are unnecessary since TypeScript can infer the type from the initial value. This violates thereplace-unnecessary-type-definition-use-state rule.
- const [state, setState] = useState<string>("test string")+ const [state, setState] = useState("test string")- const [state2, setState2] = useState<string>("test string")+ const [state2, setState2] = useState("test string")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const[state,setState]=useState<string>("test string") | |
| const[state2,setState2]=useState<string>("test string") | |
| const[state,setState]=useState("test string") | |
| const[state2,setState2]=useState("test string") |
🤖 Prompt for AI Agents
In jsx_ast_grep_failure.ts around lines 7 to 9, remove the explicit <string>type annotations from the useState calls because TypeScript can infer the typefrom the initial string value. Simply call useState with the initial stringwithout specifying the generic type parameter to comply with thereplace-unnecessary-type-definition-use-state rule.| const[first,second]=awaitPromise.all([ | ||
| await(newPromise(()=>{console.log("logging this long task")})), | ||
| newPromise(()=>{console.log("logging another long task")}), | ||
| ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fix Promise.all usage and console.log violations.
Multiple issues with this code:
console.logusage violates theno-console-except-errorrule- Double
awaiton the first promise is incorrect - Promises don't resolve/reject, causing them to hang indefinitely
- const [first, second] = await Promise.all([- await (new Promise(() => {console.log("logging this long task")})),- new Promise(() => {console.log("logging another long task")}),- ])+ const [first, second] = await Promise.all([+ new Promise((resolve) => {+ // Removed console.log to comply with no-console-except-error rule+ resolve("first task completed");+ }),+ new Promise((resolve) => {+ // Removed console.log to comply with no-console-except-error rule+ resolve("second task completed");+ }),+ ])
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const[first,second]=awaitPromise.all([ | |
| await(newPromise(()=>{console.log("logging this long task")})), | |
| newPromise(()=>{console.log("logging another long task")}), | |
| ]) | |
| const[first,second]=awaitPromise.all([ | |
| newPromise((resolve)=>{ | |
| // Removed console.log to comply with no-console-except-error rule | |
| resolve("first task completed"); | |
| }), | |
| newPromise((resolve)=>{ | |
| // Removed console.log to comply with no-console-except-error rule | |
| resolve("second task completed"); | |
| }), | |
| ]) |
🧰 Tools
🪛 ast-grep (0.38.6)
[hint] 11-11:
Context: await (new Promise(() => {console.log("logging this long task")}))
Suggested replacement: (new Promise(() => {console.log("logging this long task")}))
(no-await-in-promise-all-custom)
🤖 Prompt for AI Agents
In jsx_ast_grep_failure.ts around lines 11 to 14, fix the Promise.all usage byremoving the redundant double await on the first promise and ensure bothpromises properly resolve or reject to avoid hanging. Replace console.log callswith an allowed logging method or remove them to comply with theno-console-except-error rule.| export function findInString(text, string, logFn = console.error) { | ||
| logFn("text", text) | ||
| return text.includes(string) | ||
| } | ||
| return text.includes(string) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fix duplicate code causing parse errors.
There are duplicate lines 75-76 that repeat the return statement and closing brace, causing parse errors as flagged by static analysis.
Remove the duplicate lines:
export function findInString(text, string, logFn = console.error) { logFn("text", text) return text.includes(string) }-return text.includes(string)-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exportfunctionfindInString(text,string,logFn=console.error){ | |
| logFn("text",text) | |
| returntext.includes(string) | |
| } | |
| returntext.includes(string) | |
| } | |
| exportfunctionfindInString(text,string,logFn=console.error){ | |
| logFn("text",text) | |
| returntext.includes(string) | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Illegal return statement outside of a function
(parse)
[error] 75-76: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
🤖 Prompt for AI Agents
In greet.js around lines 71 to 76, there is a duplicate return statement andclosing brace causing parse errors. Remove the repeated lines at 75 and 76 sothat the function has only one return statement and one closing brace to fix thesyntax error.| export function welcome(name, surname) { | ||
| console.log(`Welcome, ${name} ${surname}!`) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Console.log usage violates linting rules.
Theconsole.log statement violates the AST-grep linting rule that disallowsconsole.log,console.debug, andconsole.warn except forconsole.error inside catch clauses.
Consider replacing withconsole.error if this is for error logging, or remove the console statement if it's for debugging purposes:
-console.log(`Welcome, ${name} ${surname}!`)+console.error(`Welcome, ${name} ${surname}!`)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exportfunctionwelcome(name,surname){ | |
| console.log(`Welcome,${name}${surname}!`) | |
| } | |
| exportfunctionwelcome(name,surname){ | |
| console.error(`Welcome,${name}${surname}!`) | |
| } |
🤖 Prompt for AI Agents
In greet.js around lines 15 to 17, the use of console.log violates linting rulesthat disallow console.log outside of catch clauses. To fix this, either removethe console.log statement if it is for debugging or replace it withconsole.error only if it is intended for error logging inside a catch block.Adjust the code accordingly to comply with the linting restrictions.| if (!greeting) { | ||
| console.log("No greeting") | ||
| return null | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Console.log usage violates linting rules.
Theconsole.log statement violates the AST-grep linting rule that disallowsconsole.log,console.debug, andconsole.warn except forconsole.error inside catch clauses.
Consider replacing withconsole.error:
-console.log("No greeting")+console.error("No greeting")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(!greeting){ | |
| console.log("No greeting") | |
| returnnull | |
| } | |
| if(!greeting){ | |
| -console.log("No greeting") | |
| +console.error("No greeting") | |
| returnnull | |
| } |
🤖 Prompt for AI Agents
In greet.js around lines 28 to 31, the use of console.log violates linting rulesthat only allow console.error inside catch clauses. Replace the console.log("Nogreeting") statement with console.error("No greeting") to comply with thelinting rules.| export async function findInStringAsync(text, string) { | ||
| try { | ||
| return text.includes(string) | ||
| } catch (error) { | ||
| console.log("error", error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Console.log in catch block violates linting rules.
Theconsole.log statement in the catch block violates the AST-grep linting rule. Onlyconsole.error is allowed in catch blocks.
Replace withconsole.error:
} catch (error) {-console.log("error", error)+console.error("error", error) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exportasyncfunctionfindInStringAsync(text,string){ | |
| try{ | |
| returntext.includes(string) | |
| }catch(error){ | |
| console.log("error",error) | |
| } | |
| } | |
| exportasyncfunctionfindInStringAsync(text,string){ | |
| try{ | |
| returntext.includes(string) | |
| }catch(error){ | |
| console.error("error",error) | |
| } | |
| } |
🤖 Prompt for AI Agents
In greet.js around lines 84 to 90, replace the console.log statement in thecatch block with console.error to comply with linting rules that only allowconsole.error in catch blocks. This means changing console.log("error", error)to console.error("error", error).| // TODO: Don't use random in render | ||
| const num = Math.floor(Math.random() * 1e7) | ||
| .toString() | ||
| .replace(/.d+/gi, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fix regex pattern and address TODO comment.
Two issues need attention:
- The regex pattern
/.d+/giis incorrect - it should be/\d+/gito match digits - The TODO comment highlights a valid concern about using
Math.random()in render, which can cause unnecessary re-renders
Apply this diff to fix the regex:
-.replace(/.d+/gi, "")+.replace(/\d+/gi, "")
Would you like me to suggest an alternative approach to generate the visitor number that doesn't use random in render?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Don't use random in render | |
| constnum=Math.floor(Math.random()*1e7) | |
| .toString() | |
| .replace(/.d+/gi,"") | |
| // TODO: Don't use random in render | |
| constnum=Math.floor(Math.random()*1e7) | |
| .toString() | |
| .replace(/\d+/gi,"") |
🤖 Prompt for AI Agents
In greet.js around lines 33 to 36, fix the regex pattern by replacing `/.d+/gi`with `/\d+/gi` to correctly match digits. Additionally, address the TODO byremoving the use of Math.random() inside the render function to preventunnecessary re-renders; instead, generate the visitor number outside the renderor use a stable unique identifier approach.
Uh oh!
There was an error while loading.Please reload this page.
Summary by CodeRabbit
New Features
Chores