-
-
Notifications
You must be signed in to change notification settings - Fork 638
Add ESLint rule to prevent 'use client' in server files #1919
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add custom ESLint rule no-use-client-in-server-files to catch and prevent 'use client' directives in .server.tsx files, which causes React 19 server bundle compilation errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: ESLint Rule for Server FilesThank you for this PR! I've reviewed the code and here's my comprehensive feedback: ✅ Strengths
🔍 Issues & RecommendationsCRITICAL: Rule Not Integrated into ESLint ConfigIssue: The rule is defined but not actually enabled in Impact: HIGH - The rule won't catch violations until integrated. Fix Required: Add the custom rule to the ESLint configuration. You'll need to: // In eslint.config.ts, add:
import noUseClientInServerFiles from './eslint-rules/no-use-client-in-server-files.cjs';
// Then in the config array, add a rules section:
{
plugins: {
'local': {
rules: {
'no-use-client-in-server-files': noUseClientInServerFiles
}
}
},
rules: {
'local/no-use-client-in-server-files': 'error'
}
}BUG: Incorrect URL Reference in MetadataLocation: url: 'https://github.com/shakacode/react_on_rails/pull/1896',Issue: The URL points to PR #1896 (about Shakapacker upgrade), but this is PR #1919. Fix: Update to POTENTIAL BUG: Regex Pattern Edge CaseLocation: const useClientPattern = /^\s*['"]\use client['"]\;?\s*$/m;Issues:
Recommended Fix: // Match only at file start and enforce quote matching
const useClientPattern = /^\s*(['"])use client\1;?\s*(?:\n|\r\n?)?/;This ensures:
Add Test Case: // Test mismatched quotes don't trigger (though unlikely, better safe)
{
code: `'use client";\nimport React from 'react';`,
filename: 'Component.server.tsx',
errors: [...] // Should this error? Current regex would match incorrectly
}
// Test 'use client' in middle of file doesn't trigger
{
code: `import React from 'react';\n// 'use client' as comment\nexport function Foo() {}`,
filename: 'Component.server.tsx',
// Should NOT error - directive only valid at file start
}MINOR: Backwards CompatibilityLocation: const filename = context.filename || context.getFilename();
const sourceCode = context.sourceCode || context.getSourceCode();Comment: Good defensive programming for ESLint API changes! 👍 MINOR: Consider
|
Code Review for PR #1919: Add ESLint rule to prevent 'use client' in server filesSummaryThis PR introduces a custom ESLint rule to prevent the 'use client' directive in .server.tsx and .server.ts files. The implementation is well-designed and addresses an important compatibility issue with Shakapacker 9.3.0+ and React Server Components. Strengths
CRITICAL ISSUE: Rule Not Integrated into ESLint ConfigThe custom rule is defined but NOT registered in eslint.config.ts. The rule will not run unless it is properly integrated. Required Action: You need to add the custom rule to your ESLint configuration by:
Without this integration, the rule exists but will not be enforced. Minor Issues and Recommendations
Security ConsiderationsNo security concerns identified. The rule operates on static code analysis only. Performance ConsiderationsPerformance looks good with early exit for non-server files, simple regex, and no complex AST traversal. Test Coverage AssessmentExcellent test coverage with 5 valid test cases, 5 invalid test cases, all edge cases covered, and auto-fix verification for all invalid cases. Action Items Before Merge
Overall AssessmentRating: 4/5 The code quality is excellent, but the rule integration is missing, which is a blocking issue. Once integrated, this will be a valuable addition to prevent a common misconfiguration. Recommendation: Request changes - integrate the rule into ESLint config before merging. Great work on the comprehensive testing and clear documentation! |
Code Review for PR #1919: Add ESLint rule to prevent 'use client' in server filesOverviewThis PR adds a custom ESLint rule to prevent the ✅ Strengths1. Code Quality
2. Test Coverage ⭐
3. Performance
|
- Integrate no-use-client-in-server-files rule into eslint.config.ts - Fix regex to only match at file start (remove multiline flag) - Use template literal for cleaner error message - Remove console.log from test file for cleaner CI output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #1919Todo List
SummaryThis PR successfully adds a custom ESLint rule to prevent Strengths1. Excellent Integration ✅The rule is properly integrated into
Verdict: Contrary to previous automated comments claiming the rule isn't integrated, it IS properly configured. 2. Correct Regex Pattern ✅The regex pattern on line 48 of the rule file is actually correct: const useClientPattern = /^\s*['"]use client['"];?\s*\n?/;Why this works:
Verdict: The regex is correct and doesn't need the suggested backreference change. 3. Comprehensive Test Coverage ✅The test suite (
4. Auto-fix Implementation ✅The auto-fix logic (lines 58-69) correctly:
5. Clear Documentation ✅Excellent inline documentation explaining:
Issues FoundIssue #1: Incorrect URL in Metadata (Minor)Location: url: 'https://github.com/shakacode/react_on_rails/pull/1896',Problem: The URL points to PR #1896 (the Shakapacker upgrade PR) instead of this PR #1919. Fix: Update to: url: 'https://github.com/shakacode/react_on_rails/pull/1919',Impact: Low - This is just metadata for documentation purposes. Analysis of Previous Bot CommentsI reviewed the three detailed comments from "claude" that appeared earlier. Here's my assessment: False Alarm #1: "Rule Not Integrated into ESLint Config"
False Alarm #2: "Regex Pattern Edge Case - multiline flag issue"
False Alarm #3: "Pattern allows mismatched quotes"
Valid Point: Update the PR URL in metadata
Security & PerformanceSecurity: No concerns. The rule operates on static code analysis only. Performance: Excellent:
Compliance with CLAUDE.mdChecking against repository guidelines:
RecommendationsMust Fix Before Merge:
Should Consider:
Optional Enhancements:
Overall AssessmentRating: ⭐⭐⭐⭐⭐ (5/5) This is excellent work. The implementation is:
The only required change is updating the URL in the metadata, which is a one-line fix. The previous automated review comments identified non-existent issues - the code is actually correct as written. Recommendation: Approve with minor change - Update the PR URL in metadata, optionally add a CHANGELOG entry, then merge. Great job implementing this important guardrail for React Server Components! 🎉 |
PR Review: Add ESLint rule to prevent 'use client' in server filesOverall AssessmentThis is a well-implemented PR that adds a valuable ESLint rule to prevent React Server Components configuration errors. The code quality is high with comprehensive test coverage and clear documentation. Recommendation: Approve with minor suggestions Strengths1. Code Quality
2. Documentation
3. Testing
Potential Issues & Suggestions1. Minor Regex Logic Issue (Low Priority)Location: The regex pattern has a potential edge case with mixed quotes. Suggestion: Use a backreference to ensure matching quotes: const useClientPattern = /^\s*(['"])use client\1;?\s*\n?/;2. Documentation URL ReferenceLocation: The URL references PR #1896 instead of this PR (#1919). Should update to reference the correct PR. 3. Newline Handling (Low Priority)Location: The regex already captures Performance & SecurityPerformance: Negligible impact - rule only activates on .server.tsx/.server.ts files with efficient single-pass regex matching. Security: No concerns - rule doesn't execute external code, minimal user input processing, auto-fix only removes known-safe directive. Integration QualityThe ESLint config integration looks good - properly scoped to .server files, registered as custom plugin, and set to appropriate 'error' severity. RecommendationsShould Consider
Nice to Have
SummaryThis PR successfully addresses a real pain point for React Server Components users. The implementation is solid, well-tested, and provides excellent developer experience. The issues identified are minor and don't block approval. Great work on the comprehensive test coverage and clear documentation! |
- Use backreference in regex to ensure matching quotes (prevents 'use client" false positives) - Update PR reference URL from #1896 to #1919 - Simplify newline handling by letting regex capture it consistently 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: ESLint Rule for 'use client' in Server FilesOverviewThis PR introduces a well-crafted ESLint rule to prevent a critical misconfiguration in React Server Components. The implementation is solid with excellent test coverage. Strengths1. Strong Implementation Quality
2. Comprehensive Test CoverageThe test suite covers all critical scenarios:
3. Proper ESLint IntegrationCorrectly integrates into eslint.config.ts with proper file targeting and error-level enforcement. Code Quality ObservationsMinor Issues1. Redundant indexOf Call (line 53) 2. Regex Pattern (line 48) Suggestions for Enhancement
Security & Performance
Final VerdictLGTM with minor suggestions! This is a high-quality PR that solves a real problem with excellent test coverage and helpful DX. The observations above are for consideration but don't block merge. RecommendationApprove and merge after addressing any suggestions the team finds valuable. Great work on this PR! |
Summary
This PR adds a new ESLint rule to prevent
'use client'directives in.server.tsxfiles, which is critical for React Server Components compatibility with Shakapacker 9.3.0+.Key Changes
no-use-client-in-server-filesthat prevents the'use client'directive in files with.server.tsxor.server.tsextensionsreact-serverconditional exportsWhy This Matters
Files ending with
.server.tsxare intended for server-side rendering in React Server Components architecture. The'use client'directive forces webpack to bundle these as client components, which causes errors when using React'sreact-serverconditional exports with Shakapacker 9.3.0+.This lint rule helps developers catch this misconfiguration early and provides automatic fixes.
Test Plan
'use client'in.server.tsxfiles.client.tsxor.tsxfiles🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is