Fixed ESLint bugs that required ts-specific rules for js functions and added prettier formatting (re-open #3184) #3229
Conversation
WalkthroughThe pull request involves migrating a localStorage usage check script from JavaScript to TypeScript. The changes span multiple files including Changes
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
scripts/githooks/check-localstorage-usage.js (1)
Line range hint
21-40: Update file pattern to include .js filesThe grep pattern
.tsx\\?$only matches.tsand.tsxfiles, but based on the PR objectives, we need to check JavaScript files as well.Update the pattern to include .js files:
- const result = execSync('git ls-files | grep ".tsx\\?$"', { + const result = execSync('git ls-files | grep "\\.(js|tsx?)$"', {
🧹 Nitpick comments (1)
scripts/githooks/check-localstorage-usage.js (1)
Line range hint
45-78: Enhance localStorage detection robustnessThe current checks might miss some localStorage usages. Consider these improvements:
if ( - content.includes('localStorage.getItem') || - content.includes('localStorage.setItem') || - content.includes('localStorage.removeItem') + content.match(/\blocalStorage\.(getItem|setItem|removeItem|clear|key|\[["'].*?["']\])/g) || + content.match(/\bwindow\.localStorage\b/g) ) {This will also catch:
- Direct property access (e.g.,
localStorage['key'])localStorage.clear()andlocalStorage.key()- References through
window.localStorage
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/githooks/check-localstorage-usage.js(4 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/githooks/check-localstorage-usage.js (1)
Learnt from: MayankJha014
PR: PalisadoesFoundation/talawa-admin#2612
File: scripts/githooks/check-localstorage-usage.js:10-18
Timestamp: 2024-12-07T14:25:09.928Z
Learning: TypeScript type annotations should not be added to JavaScript (`.js`) files in this project. Type annotations can only be used in TypeScript (`.ts` or `.tsx`) files.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
scripts/githooks/check-localstorage-usage.js (3)
Line range hint
10-20: Well-implemented skip comment mechanism!The function provides a clean way to handle exceptions with proper error handling and follows JavaScript conventions correctly.
92-95: LGTM! Clear and well-formatted messagesThe console messages are properly formatted and provide helpful guidance about using custom hooks.
Line range hint
10-78: Verify all TypeScript-specific ESLint rulesLet's ensure no other TypeScript-specific rules are being enforced on this JavaScript file.
✅ Verification successful
TypeScript ESLint rules are properly handled
The only TypeScript-specific rule being enforced is
@typescript-eslint/explicit-function-return-type, which is appropriately disabled for each arrow function. No other TypeScript-specific rules are being enforced on this JavaScript file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TypeScript-specific ESLint rules in the ESLint config files echo "Checking ESLint configs for TypeScript-specific rules..." fd -e json -e js -e yaml -e yml . | xargs rg "@typescript-eslint|parserOptions.*typescript" -A 5 -B 5 # Verify if there are any other TypeScript-related ESLint errors in the file echo "Checking for TypeScript-related ESLint errors..." npx eslint scripts/githooks/check-localstorage-usage.js --format json | jq '.[] | select(.messages[].ruleId | contains("@typescript"))'Length of output: 20683
|
Hello @palisadoes, Since modifying the ESLint configuration file was causing linting errors in other PRs, I opted to ignore the rule only in the conflicting file rather than updating the entire ESLint configuration. Please review the changes in these files and let me know if there are any suggestions for improving the PR. Thanks! |
palisadoes
changed the title
|
Hello @palisadoes, any updates regarding the review of these changes? |
palisadoes
added
the
ignore-sensitive-files-pr
|
|
Hello @palisadoes, Thank you for the clarifications! Since my last PR caused linting issues in other PRs, I want to be more cautious with my commits this time. I’d appreciate your guidance on the best approach. Here are two solutions I’ve considered: The simplest option would be adding the script to the ignorePatterns section in the ESLint configuration file. |
|
I provided my feedback previously |
…or the target file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.eslintrc.json (1)
157-164: Good approach for handling JavaScript files, but consider TypeScript migration.The override configuration is a cleaner solution compared to using eslint-disable comments. However, since the project uses TypeScript, consider migrating this script to TypeScript for consistency and type safety.
If you decide to keep it in JavaScript, the current override is well-scoped and appropriate. If you choose to migrate to TypeScript, I can help with the migration process.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.eslintrc.json(1 hunks)scripts/githooks/check-localstorage-usage.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/githooks/check-localstorage-usage.js
🔇 Additional comments (1)
.eslintrc.json (1)
159-159: Verify the target file and its current state.Let's ensure the file exists and check its current state regarding ESLint comments.
✅ Verification successful
ESLint configuration is correctly targeting an existing, well-structured file
The specified file exists and contains properly defined functions without any ESLint disable comments. The ESLint configuration is appropriate for this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the target file and its ESLint configuration # Check if the file exists and show its content echo "Checking file existence and content:" cat scripts/githooks/check-localstorage-usage.js # Check for any eslint-disable comments that should be removed echo -e "\nChecking for eslint-disable comments that should be removed:" rg "eslint-disable" scripts/githooks/check-localstorage-usage.js # Check for function declarations that would trigger these rules echo -e "\nChecking for function declarations:" ast-grep --pattern 'function $FNAME($_) { $$$ }' scripts/githooks/check-localstorage-usage.jsLength of output: 2985
|
Hello @palisadoes, I have removed the eslint-ignore comments and added a script to override the TypeScript rule for the specific conflicting file instead of applying it to all JavaScript files that caused linting errors. Let me know if any further changes are needed. |
|
Please fix the conflicting file |
|
@palisadoes , fixing the conflicting file with return types would give the following error since it is not a typescript file=> That's why typically the best approach is to keep it as pure JavaScript and disable TypeScript rules |
|
Convert the file to typescript then. |
|
|
@palisadoes I'm still analyzing the code, but converting it to TypeScript seems like a viable option. Since the script is a standalone Git hook that: it should be manageable. However, I'm not 100% certain yet, so I'm continuing my review. I’ll most likely add a commit by tomorrow for you to review. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/githooks/check-localstorage-usage.ts (3)
16-19: Improve error logging format.The error message format can be improved for better readability and consistency.
- console.error( - `Error reading file ${file}:`, - error instanceof Error ? error.message : error, - ); + console.error(`Error reading file ${file}: ${error instanceof Error ? error.message : String(error)}`);🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Command 'tsx' not found. The required TSX package is not installed in the CI environment.
29-30: Consider using a more robust file pattern.The current grep pattern
.tsx\\?$might miss some edge cases.- const result = execSync('git ls-files | grep ".tsx\\?$"', options); + const result = execSync('git ls-files "*.ts" "*.tsx"', options);🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Command 'tsx' not found. The required TSX package is not installed in the CI environment.
81-84: Duplicate error handling pattern.This error handling block is duplicated across multiple functions. Consider extracting it to a helper function.
const logError = (context: string, error: unknown): void => { console.error(`${context}: ${error instanceof Error ? error.message : String(error)}`); };🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Command 'tsx' not found. The required TSX package is not installed in the CI environment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pull-request.yml(1 hunks).lintstagedrc.json(1 hunks)package.json(1 hunks)scripts/githooks/check-localstorage-usage.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/githooks/check-localstorage-usage.ts (1)
Learnt from: MayankJha014
PR: PalisadoesFoundation/talawa-admin#2612
File: scripts/githooks/check-localstorage-usage.js:10-18
Timestamp: 2024-12-07T14:25:09.928Z
Learning: TypeScript type annotations should not be added to JavaScript (`.js`) files in this project. Type annotations can only be used in TypeScript (`.ts` or `.tsx`) files.
🪛 GitHub Actions: PR Workflow
scripts/githooks/check-localstorage-usage.ts
[error] Command 'tsx' not found. The required TSX package is not installed in the CI environment.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
scripts/githooks/check-localstorage-usage.ts (1)
26-27: LGTM! Good use of TypeScript types.The
ExecSyncOptionsWithStringEncodingtype ensures type safety for the execSync options.🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Command 'tsx' not found. The required TSX package is not installed in the CI environment.
package.json (1)
95-95: Verify tsx package version.The script command has been correctly updated, but let's ensure we're using a compatible version of tsx.
✅ Verification successful
tsx version is compatible
The current version (^4.19.1) is recent and compatible with the script usage. No version update is necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the latest stable version of tsx npm view tsx versions --json | jq -r '.[-1]'Length of output: 53
Script:
#!/bin/bash # Check current tsx version in package.json jq -r '.devDependencies.tsx' package.jsonLength of output: 49
.lintstagedrc.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "**/*.{ts,tsx,yml}": "eslint --fix", | |||
| "**/*.{ts,tsx,json,scss,css,yml}": "prettier --write", | |||
| "**/*.{ts,tsx}": "node scripts/githooks/check-localstorage-usage.js" | |||
| "**/*.{ts,tsx}": "tsx scripts/githooks/check-localstorage-usage.ts" | |||
There was a problem hiding this comment.
Fix the pipeline failure by installing tsx.
The pipeline is failing because the tsx package is not installed in the CI environment.
Add tsx to the devDependencies in package.json and ensure it's installed in the CI workflow:
+"tsx": "^4.7.0",Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/pull-request.yml
Outdated
| chmod +x scripts/githooks/check-localstorage-usage.ts | ||
| tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update file permissions handling.
The current approach of using chmod on a TypeScript file is unconventional.
Consider one of these approaches:
- Move the shebang to a shell script wrapper
- Use
npxto execute the TypeScript file directly:
- chmod +x scripts/githooks/check-localstorage-usage.ts
- tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo
+ npx tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo📝 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.
| chmod +x scripts/githooks/check-localstorage-usage.ts | |
| tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo | |
| npx tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3229 +/- ##
=====================================================
+ Coverage 10.78% 89.80% +79.02%
=====================================================
Files 308 331 +23
Lines 7838 8589 +751
Branches 1709 1912 +203
=====================================================
+ Hits 845 7713 +6868
+ Misses 6932 614 -6318
- Partials 61 262 +201
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
scripts/githooks/check-localstorage-usage.ts (1)
Add unit tests for the localStorage usage check script
The script performs critical pre-commit validation but lacks test coverage. Consider adding tests for:
- File filtering logic (skipped files, extensions)
- Skip comment detection
- localStorage usage detection patterns
- Different execution modes (--scan-entire-repo vs staged files)
🔗 Analysis chain
Line range hint
8-85: Add unit tests for the script.The script lacks test coverage. Consider adding unit tests to ensure reliability.
Would you like me to help create unit tests for this script?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test file fd "check-localstorage-usage.test.ts" tests/ scripts/Length of output: 106
Script:
#!/bin/bash # Search for test files related to the script fd -e ts -e tsx . scripts/githooks/ | grep -i "test\|spec" # Look for test configuration files fd "jest.config|vitest.config|test.config" . # Check content of package.json for test setup cat package.json | jq -r '.scripts | select(.test != null)'Length of output: 1342
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-9: scripts/githooks/check-localstorage-usage.ts#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 11-11: scripts/githooks/check-localstorage-usage.ts#L11
Added line #L11 was not covered by tests
[warning] 16-16: scripts/githooks/check-localstorage-usage.ts#L16
Added line #L16 was not covered by tests
[warning] 24-24: scripts/githooks/check-localstorage-usage.ts#L24
Added line #L24 was not covered by tests
[warning] 26-26: scripts/githooks/check-localstorage-usage.ts#L26
Added line #L26 was not covered by tests
[warning] 29-29: scripts/githooks/check-localstorage-usage.ts#L29
Added line #L29 was not covered by tests
[warning] 33-33: scripts/githooks/check-localstorage-usage.ts#L33
Added line #L33 was not covered by tests
[warning] 36-36: scripts/githooks/check-localstorage-usage.ts#L36
Added line #L36 was not covered by tests
[warning] 42-42: scripts/githooks/check-localstorage-usage.ts#L42
Added line #L42 was not covered by tests
[warning] 45-46: scripts/githooks/check-localstorage-usage.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 48-48: scripts/githooks/check-localstorage-usage.ts#L48
Added line #L48 was not covered by tests
🧹 Nitpick comments (3)
scripts/githooks/check-localstorage-usage.ts (3)
11-21: Enhance error handling robustness.While the error handling is improved with the
instanceof Errorcheck, consider adding file existence validation before reading.const containsSkipComment = (file: string): boolean => { try { + if (!existsSync(file)) { + console.error(`File ${file} does not exist`); + return false; + } const content = readFileSync(file, 'utf-8'); return content.includes('// SKIP_LOCALSTORAGE_CHECK'); } catch (error) { console.error( `Error reading file ${file}:`, error instanceof Error ? error.message : error, ); return false; } };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-11: scripts/githooks/check-localstorage-usage.ts#L11
Added line #L11 was not covered by tests
[warning] 16-16: scripts/githooks/check-localstorage-usage.ts#L16
Added line #L16 was not covered by tests
24-42: Handle empty git command results.The function should handle cases where git commands return empty results.
const getModifiedFiles = (): string[] => { try { const options: ExecSyncOptionsWithStringEncoding = { encoding: 'utf-8' }; if (scanEntireRepo) { const result = execSync('git ls-files | grep ".tsx\\?$"', options); - return result.trim().split('\n'); + const files = result.trim(); + return files ? files.split('\n') : []; } const result = execSync('git diff --cached --name-only', options); - return result.trim().split('\n'); + const files = result.trim(); + return files ? files.split('\n') : []; } catch (error) { console.error( 'Error fetching modified files:', error instanceof Error ? error.message : error, ); process.exit(1); } return []; };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-24: scripts/githooks/check-localstorage-usage.ts#L24
Added line #L24 was not covered by tests
[warning] 26-26: scripts/githooks/check-localstorage-usage.ts#L26
Added line #L26 was not covered by tests
[warning] 29-29: scripts/githooks/check-localstorage-usage.ts#L29
Added line #L29 was not covered by tests
[warning] 33-33: scripts/githooks/check-localstorage-usage.ts#L33
Added line #L33 was not covered by tests
[warning] 36-36: scripts/githooks/check-localstorage-usage.ts#L36
Added line #L36 was not covered by tests
[warning] 42-42: scripts/githooks/check-localstorage-usage.ts#L42
Added line #L42 was not covered by tests
Line range hint
48-85: Improve file path handling.Consider using
path.resolvefor consistent file path handling across different platforms.-const checkLocalStorageUsage = (file: string): void => { +const checkLocalStorageUsage = (filePath: string): void => { if (!file) { return; } - const fileName = path.basename(file); + const resolvedPath = path.resolve(filePath); + const fileName = path.basename(resolvedPath); // Skip files with specific names or containing a skip comment if ( fileName === 'check-localstorage-usage.ts' || fileName === 'useLocalstorage.test.ts' || fileName === 'useLocalstorage.ts' || - containsSkipComment(file) + containsSkipComment(resolvedPath) ) { - console.log(`Skipping file: ${file}`); + console.log(`Skipping file: ${resolvedPath}`); return; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-81: scripts/githooks/check-localstorage-usage.ts#L81
Added line #L81 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pull-request.yml(1 hunks).lintstagedrc.json(1 hunks)package.json(1 hunks)scripts/githooks/check-localstorage-usage.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/githooks/check-localstorage-usage.ts (1)
Learnt from: MayankJha014
PR: PalisadoesFoundation/talawa-admin#2612
File: scripts/githooks/check-localstorage-usage.js:10-18
Timestamp: 2024-12-07T14:25:09.928Z
Learning: TypeScript type annotations should not be added to JavaScript (`.js`) files in this project. Type annotations can only be used in TypeScript (`.ts` or `.tsx`) files.
🪛 GitHub Check: codecov/patch
scripts/githooks/check-localstorage-usage.ts
[warning] 8-9: scripts/githooks/check-localstorage-usage.ts#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 11-11: scripts/githooks/check-localstorage-usage.ts#L11
Added line #L11 was not covered by tests
[warning] 16-16: scripts/githooks/check-localstorage-usage.ts#L16
Added line #L16 was not covered by tests
[warning] 24-24: scripts/githooks/check-localstorage-usage.ts#L24
Added line #L24 was not covered by tests
[warning] 26-26: scripts/githooks/check-localstorage-usage.ts#L26
Added line #L26 was not covered by tests
[warning] 29-29: scripts/githooks/check-localstorage-usage.ts#L29
Added line #L29 was not covered by tests
[warning] 33-33: scripts/githooks/check-localstorage-usage.ts#L33
Added line #L33 was not covered by tests
[warning] 36-36: scripts/githooks/check-localstorage-usage.ts#L36
Added line #L36 was not covered by tests
[warning] 42-42: scripts/githooks/check-localstorage-usage.ts#L42
Added line #L42 was not covered by tests
[warning] 45-46: scripts/githooks/check-localstorage-usage.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 48-48: scripts/githooks/check-localstorage-usage.ts#L48
Added line #L48 was not covered by tests
[warning] 81-81: scripts/githooks/check-localstorage-usage.ts#L81
Added line #L81 was not covered by tests
🔇 Additional comments (4)
scripts/githooks/check-localstorage-usage.ts (1)
6-9: LGTM! Type annotations improve code safety.The added TypeScript type annotations enhance type safety and code maintainability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-9: scripts/githooks/check-localstorage-usage.ts#L8-L9
Added lines #L8 - L9 were not covered by tests.lintstagedrc.json (1)
4-4: LGTM! Command updated correctly.The command has been properly updated to use
tsxfor executing the TypeScript file.package.json (1)
95-95: LGTM! Script command updated correctly.The script command has been properly updated to use
tsx, and the required dependency is present in devDependencies..github/workflows/pull-request.yml (1)
69-70: 🛠️ Refactor suggestionImprove script execution approach.
Using
chmodon a TypeScript file is unconventional. Consider usingnpxdirectly.- chmod +x scripts/githooks/check-localstorage-usage.ts - npx tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo + npx tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo⛔ Skipped due to learnings
Learnt from: MayankJha014 PR: PalisadoesFoundation/talawa-admin#2612 File: scripts/githooks/check-localstorage-usage.js:10-18 Timestamp: 2024-12-07T14:25:09.928Z Learning: TypeScript type annotations should not be added to JavaScript (`.js`) files in this project. Type annotations can only be used in TypeScript (`.ts` or `.tsx`) files.
|
|
@palisadoes, the PR was failing the Codecov test. Would writing test cases be applicable for this type of file? Apologies for the inconvenience—otherwise, it would have failed the test. |
|
Please remove the statement. I'll reopen the PR |
What kind of change does this PR introduce?
BugFix
Issue Number:
Fixes #3184
Did you add tests for your changes?
No
Snapshots/Videos:
Before
After
If relevant, did you update the documentation?
Not applicable
Does this PR introduce a breaking change?
No
Other information
Problem 1 : ESLint was configured to check typescript-specific return type for javascript file.
So i added comments to ignore the rule for javascript rules.
Problem 2: Prettier Test was failing since there were some missing commas in console.info
So i ran the prettier write command to fix that format to a better format.
Have you read the contributing guide?
Yes
Summary by CodeRabbit