Skip to content

Conversation

@justin808
Copy link
Member

@justin808 justin808 commented Nov 5, 2025

Summary

This PR adds a new ESLint rule to prevent 'use client' directives in .server.tsx files, which is critical for React Server Components compatibility with Shakapacker 9.3.0+.

Key Changes

  • New ESLint Rule: no-use-client-in-server-files that prevents the 'use client' directive in files with .server.tsx or .server.ts extensions
  • Comprehensive Tests: 161 lines of test coverage for the new rule with various edge cases
  • Auto-fix Support: The rule includes an auto-fix that removes the invalid directive
  • Shakapacker 9.3.0 Compatibility: Resolves bundling errors when using React's react-server conditional exports

Why This Matters

Files ending with .server.tsx are 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's react-server conditional exports with Shakapacker 9.3.0+.

This lint rule helps developers catch this misconfiguration early and provides automatic fixes.

Test Plan

  • All existing tests pass
  • New ESLint rule tests validate:
    • Detection of 'use client' in .server.tsx files
    • Support for both single and double quotes
    • Support for directives with or without semicolons
    • Proper handling of whitespace
    • Correct auto-fix behavior
    • No false positives for .client.tsx or .tsx files

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com


This change is Reviewable

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>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 5, 2025

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 777bee2 and 9a3ff07.

📒 Files selected for processing (3)
  • eslint-rules/no-use-client-in-server-files.cjs (1 hunks)
  • eslint-rules/no-use-client-in-server-files.test.cjs (1 hunks)
  • eslint.config.ts (2 hunks)
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin808/no-use-client-lint-rule

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Nov 5, 2025

Code Review: ESLint Rule for Server Files

Thank you for this PR! I've reviewed the code and here's my comprehensive feedback:

Strengths

  1. Clear Purpose & Documentation: The rule has excellent inline documentation explaining why it exists and the specific Shakapacker 9.3.0+ compatibility issue it solves.

  2. Comprehensive Test Coverage: 161 lines of tests covering:

    • Both single and double quotes
    • Directives with/without semicolons
    • Leading whitespace handling
    • Both .server.tsx and .server.ts extensions
    • Proper negative cases (.client.tsx, .tsx)
    • Auto-fix functionality
  3. Auto-fix Support: The rule includes automatic fixing, which is excellent UX for developers.

  4. Proper Error Messaging: The error message is clear and provides actionable guidance.

🔍 Issues & Recommendations

CRITICAL: Rule Not Integrated into ESLint Config

Issue: The rule is defined but not actually enabled in eslint.config.ts. This means it won't run during linting.

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 Metadata

Location: eslint-rules/no-use-client-in-server-files.cjs:23

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 https://github.com/shakacode/react_on_rails/pull/1919

POTENTIAL BUG: Regex Pattern Edge Case

Location: eslint-rules/no-use-client-in-server-files.cjs:50

const useClientPattern = /^\s*['"]\use client['"]\;?\s*$/m;

Issues:

  1. The multiline flag (m) makes ^ match start of any line, not just file start. This could match 'use client' in the middle of a file (e.g., in a comment or string).
  2. Pattern requires the opening and closing quotes to match implicitly but doesn't enforce it (can match 'use client" or "use client').

Recommended Fix:

// Match only at file start and enforce quote matching
const useClientPattern = /^\s*(['"])use client\1;?\s*(?:\n|\r\n?)?/;

This ensures:

  • Match only at the start of the file (no m flag)
  • Opening and closing quotes must match (\1 backreference)
  • Handles different newline styles

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 Compatibility

Location: eslint-rules/no-use-client-in-server-files.cjs:36,45

const filename = context.filename || context.getFilename();
const sourceCode = context.sourceCode || context.getSourceCode();

Comment: Good defensive programming for ESLint API changes! 👍

MINOR: Consider .jsx and .js extensions

The rule only checks .server.tsx and .server.ts. Should it also check:

  • .server.jsx
  • .server.js

If these file patterns are used in the codebase, they should be included.

🔒 Security Considerations

No security issues identified. The rule operates on static file content only.

Performance Considerations

  • Good: Early return if filename doesn't match pattern (line 38-41)
  • Good: Simple regex matching - no performance concerns
  • Optimization: Consider caching the filename check result if create() is called multiple times per file (unlikely but possible in some ESLint configs)

🧪 Test Coverage

Excellent coverage overall, but missing:

  1. Edge case: 'use client' in middle of file (should NOT error)
  2. Edge case: Mismatched quotes (current regex might match incorrectly)
  3. Multiple 'use client' directives (pathological but should handle gracefully)
  4. Files with BOM (byte order mark) at start

📝 Documentation

Needed:

  1. README or docs explaining how to enable this rule
  2. Migration guide for existing codebases with violations
  3. Update CLAUDE.md if this rule should be part of mandatory checks

Action Items

Must Fix Before Merge:

  1. ❗ Integrate rule into eslint.config.ts
  2. ❗ Fix URL in metadata (PR Upgrade Shakapacker to 9.3.0 #1896Add ESLint rule to prevent 'use client' in server files #1919)
  3. ⚠️ Fix regex pattern to only match at file start with proper quote matching
  4. ⚠️ Add test cases for edge cases mentioned above

Should Consider:

  1. Add support for .server.jsx and .server.js if used
  2. Add documentation on enabling/using the rule
  3. Run the rule on the entire codebase to verify no false positives

🎯 Overall Assessment

This is a well-designed rule solving a real problem with good tests and auto-fix support. The main blocker is that the rule isn't actually enabled, and there's a regex bug that could cause false positives. Once these critical issues are addressed, this will be a valuable addition to the codebase.

Recommendation: Request changes for the critical issues, then approve after fixes.


Review performed using Claude Code with reference to CLAUDE.md guidelines

@claude
Copy link

claude bot commented Nov 5, 2025

Code Review for PR #1919: Add ESLint rule to prevent 'use client' in server files

Summary

This 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

  1. Clear Purpose and Documentation: The rule has excellent inline documentation explaining why it exists and when it applies.

  2. Comprehensive Test Coverage: The test file includes valid cases, invalid cases with auto-fix verification, edge cases for different quote styles, semicolons, whitespace handling, and both .server.tsx and .server.ts extensions.

  3. Auto-fix Functionality: The rule includes a fixer that automatically removes the problematic directive, improving developer experience.

  4. Pattern Matching: The regex pattern properly handles leading whitespace, single or double quotes, optional semicolons, and trailing whitespace.

CRITICAL ISSUE: Rule Not Integrated into ESLint Config

The 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:

  1. Importing the custom rule
  2. Registering it as a plugin
  3. Enabling it in the rules configuration

Without this integration, the rule exists but will not be enforced.

Minor Issues and Recommendations

  1. Regex Could Match False Positives: The current regex with multiline flag could match the directive anywhere in the file on its own line, not just at the top. Consider making it more strict to only match at the file start.

  2. Code Style: The error message uses string concatenation across multiple lines. Consider using template literals for better readability.

  3. Documentation: Add a README or update CLAUDE.md to document how to run this specific rule, where it is configured, and how developers can test it locally.

  4. Test Output: The test file has a console.log at the end which could be removed for cleaner test output in CI.

Security Considerations

No security concerns identified. The rule operates on static code analysis only.

Performance Considerations

Performance looks good with early exit for non-server files, simple regex, and no complex AST traversal.

Test Coverage Assessment

Excellent 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

  1. MUST: Integrate the rule into eslint.config.ts
  2. MUST: Verify the rule runs with yarn lint
  3. SHOULD: Test the rule on actual .server.tsx files in the codebase
  4. SHOULD: Update CLAUDE.md or documentation with rule information
  5. OPTIONAL: Consider the regex strictness recommendation
  6. OPTIONAL: Clean up the console.log in tests

Overall Assessment

Rating: 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!

@claude
Copy link

claude bot commented Nov 5, 2025

Code Review for PR #1919: Add ESLint rule to prevent 'use client' in server files

Overview

This PR adds a custom ESLint rule to prevent the 'use client' directive in .server.tsx and .server.ts files. The implementation is clean and well-tested, with good attention to edge cases.


✅ Strengths

1. Code Quality

  • Clear documentation: Excellent JSDoc comments explaining the rule's purpose and the problem it solves
  • Clean implementation: Simple, focused logic that does one thing well
  • Error message quality: The error message is comprehensive and actionable, guiding developers to rename the file if needed
  • Auto-fix support: Including an automatic fix is a great developer experience enhancement

2. Test Coverage

  • Comprehensive test suite: 161 lines of tests covering multiple scenarios
  • Edge cases handled:
    • Single and double quotes
    • With and without semicolons
    • Leading whitespace
    • Different file extensions (.server.tsx and .server.ts)
  • Valid cases tested: Ensures no false positives for .client.tsx and regular .tsx files
  • Auto-fix validation: Each invalid case includes output verification

3. Performance

  • Efficient pattern matching: Uses a simple regex that only runs on relevant files
  • Early return: Exits immediately for non-server files, avoiding unnecessary processing
  • Minimal overhead: Only processes the Program node once per file

⚠️ Issues & Recommendations

1. CRITICAL: Rule Not Enabled in ESLint Config

The PR adds the rule files but doesn't integrate them into eslint.config.ts. The rule won't actually run without this integration.

Required changes:

// In eslint.config.ts, add near the top:
import noUseClientInServerFiles from './eslint-rules/no-use-client-in-server-files.cjs';

// Then in the config array, add a new configuration object:
{
  files: ['**/*.server.ts', '**/*.server.tsx'],
  plugins: {
    'local': {
      rules: {
        'no-use-client-in-server-files': noUseClientInServerFiles,
      },
    },
  },
  rules: {
    'local/no-use-client-in-server-files': 'error',
  },
},

2. Bug: Regex Pattern Edge Case

The regex pattern /^\s*['"]use client['"];?\s*$/m has a potential issue:

// This would match:
'use client"  // Mixed quotes!

// The pattern allows: ['use client"] or ["use client']

Recommended fix:

const useClientPattern = /^\s*(['"])use client\1;?\s*$/m;

This uses a backreference (\1) to ensure the same quote type is used for opening and closing.

3. Enhancement: Support for More Directive Formats

Current implementation might miss some edge cases:

// Might not catch these:
  'use client'  ;  // Space before semicolon
'use client'     // Multiple trailing spaces before newline

Consider using:

const useClientPattern = /^\s*['"]use client['"]\s*;?\s*$/m;

This allows optional spaces before the semicolon.

4. Minor: Test Suite Enhancement

Add a test case for mixed quotes to ensure the regex behaves correctly:

{
  code: `'use client";\n\nimport React from 'react';`,
  filename: 'Component.server.tsx',
  // Should this error? Currently it would.
}

5. Documentation Gap

The PR doesn't update:

  • CHANGELOG.md - Should document this new feature
  • README or docs - Should mention the new ESLint rule for developers

According to CLAUDE.md:

Update CHANGELOG.md for user-visible changes only (features, bug fixes, breaking changes, deprecations, performance improvements)

This qualifies as a feature addition.

6. Consideration: File Location

The eslint-rules/ directory is at the project root, but it's not clear:

  • Is this directory already established for custom rules?
  • Should it have its own README explaining local rules?
  • Should the test file be in a tests/ subdirectory?

🔒 Security Considerations

No security concerns identified. The rule:

  • Only reads filenames and file content (standard ESLint behavior)
  • Doesn't execute any dynamic code
  • Doesn't modify files outside the auto-fix scope

🎯 Recommendations Summary

Must Fix Before Merge:

  1. Integrate rule into eslint.config.ts - Without this, the rule won't run
  2. Fix regex to use backreferences - Prevents mixed-quote matches
  3. Add CHANGELOG entry - Required per project guidelines

Should Consider:

  1. Add test case for mixed quotes
  2. Update documentation/README
  3. Consider more flexible whitespace handling in regex

✨ Final Assessment

Overall: Strong implementation with excellent test coverage, but missing critical integration step.

The code quality is high, and the test suite is exemplary. However, the rule won't actually function without being added to the ESLint configuration. Once the ESLint config integration is added and the regex pattern is fixed, this will be ready to merge.

Estimated effort to address: ~15 minutes for required fixes


Review generated with attention to the project's CLAUDE.md guidelines regarding formatting, linting, and changelog requirements.

- 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>
@justin808
Copy link
Member Author

@claude review this PR

@claude
Copy link

claude bot commented Nov 5, 2025

Claude finished @justin808's task —— View job


Code Review for PR #1919

Todo List

  • Read repository guidelines (CLAUDE.md)
  • Review changed files
  • Analyze implementation and test coverage
  • Provide comprehensive feedback

Summary

This PR successfully adds a custom ESLint rule to prevent 'use client' directives in .server.tsx and .server.ts files. The implementation is well-designed, properly integrated, and thoroughly tested. I reviewed the concerns raised in previous automated comments and found that most of them are actually non-issues - the implementation is correct as-is.


Strengths

1. Excellent Integration

The rule is properly integrated into eslint.config.ts (lines 170-181):

  • Correctly scoped to only run on **/*.server.ts and **/*.server.tsx files
  • Registered as a plugin under the react-on-rails namespace
  • Enabled as an error-level rule

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:

  • ^ without the m flag matches ONLY at the start of the file (not the start of any line)
  • The pattern correctly handles both single and double quotes
  • Previous comments suggested this could match mismatched quotes like 'use client", but that's not a problem - mismatched quotes are syntax errors anyway, and ESLint won't reach this rule if the file can't parse
  • The pattern correctly handles optional semicolons and trailing newlines

Verdict: The regex is correct and doesn't need the suggested backreference change.

3. Comprehensive Test Coverage

The test suite (no-use-client-in-server-files.test.cjs) is excellent:

  • 5 valid test cases covering proper server files, client files, and regular files
  • 5 invalid test cases with auto-fix verification
  • Edge cases covered: single/double quotes, with/without semicolons, leading whitespace
  • Tests both .server.tsx and .server.ts extensions

4. Auto-fix Implementation

The auto-fix logic (lines 58-69) correctly:

  • Removes the entire 'use client' directive
  • Handles trailing newlines properly
  • Preserves the rest of the file content

5. Clear Documentation

Excellent inline documentation explaining:

  • Why the rule exists (React Server Components + Shakapacker 9.3.0+ compatibility)
  • What the rule does
  • Error message provides actionable guidance

Issues Found

Issue #1: Incorrect URL in Metadata (Minor)

Location: eslint-rules/no-use-client-in-server-files.cjs:23

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 Comments

I reviewed the three detailed comments from "claude" that appeared earlier. Here's my assessment:

False Alarm #1: "Rule Not Integrated into ESLint Config"

  • Status: ❌ INCORRECT
  • Reality: The rule IS properly integrated in eslint.config.ts lines 170-181

False Alarm #2: "Regex Pattern Edge Case - multiline flag issue"

  • Status: ❌ INCORRECT
  • Reality: The regex does NOT use the m flag. The pattern /^\s*['"]use client['"];?\s*\n?/ correctly matches only at file start

False Alarm #3: "Pattern allows mismatched quotes"

  • Status: ❌ NOT A CONCERN
  • Reality: While technically the pattern could match 'use client", this is a syntax error anyway and won't be valid JavaScript that ESLint would process

Valid Point: Update the PR URL in metadata

  • Status: ✅ CORRECT
  • This should be fixed

Security & Performance

Security: No concerns. The rule operates on static code analysis only.

Performance: Excellent:

  • Early return for non-server files (line 36-37)
  • Simple regex matching with no complex AST traversal
  • Minimal overhead

Compliance with CLAUDE.md

Checking against repository guidelines:

  • Formatting: Files end with newlines
  • Test Coverage: Comprehensive Jest tests included
  • ⚠️ Changelog: Not updated (but may not be required yet if PR is still in progress)
  • Code Quality: Clean, well-documented implementation

Recommendations

Must Fix Before Merge:

  1. Update URL in metadata from PR Upgrade Shakapacker to 9.3.0 #1896 to Add ESLint rule to prevent 'use client' in server files #1919 in eslint-rules/no-use-client-in-server-files.cjs:23

Should Consider:

  1. Add CHANGELOG.md entry - Per CLAUDE.md guidelines, this is a user-visible feature addition that should be documented:
    #### New Features
    - [PR 1919](https://github.com/shakacode/react_on_rails/pull/1919) by [justin808](https://github.com/justin808): Add ESLint rule to prevent 'use client' in server files for React Server Components compatibility with Shakapacker 9.3.0+

Optional Enhancements:

  1. Test the rule on actual codebase - Run yarn run lint to ensure it works as expected on real files
  2. Consider .server.jsx and .server.js - If these patterns are used in the codebase, add support for them

Overall Assessment

Rating: ⭐⭐⭐⭐⭐ (5/5)

This is excellent work. The implementation is:

  • ✅ Properly integrated into ESLint
  • ✅ Correctly implemented with sound regex logic
  • ✅ Thoroughly tested with great edge case coverage
  • ✅ Well-documented with clear purpose and usage
  • ✅ Includes helpful auto-fix functionality

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! 🎉


@claude
Copy link

claude bot commented Nov 5, 2025

PR Review: Add ESLint rule to prevent 'use client' in server files

Overall Assessment

This 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


Strengths

1. Code Quality

  • Clean implementation with straightforward logic
  • Excellent test coverage: 159 lines of tests covering edge cases
  • Auto-fix support for automatic remediation
  • Clear error messages explaining the problem and suggesting solutions

2. Documentation

  • Comprehensive JSDoc comments
  • Comments explain the "why" behind the rule
  • Helpful error message guides developers to rename files if needed

3. Testing

  • Wide edge case coverage (single/double quotes, with/without semicolons, leading whitespace)
  • Tests both .server.tsx and .server.ts extensions
  • Validates non-triggering cases (.client.tsx and regular .tsx files)
  • Auto-fix verification included

Potential Issues & Suggestions

1. Minor Regex Logic Issue (Low Priority)

Location: eslint-rules/no-use-client-in-server-files.cjs:48

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 Reference

Location: eslint-rules/no-use-client-in-server-files.cjs:23

The URL references PR #1896 instead of this PR (#1919). Should update to reference the correct PR.

3. Newline Handling (Low Priority)

Location: eslint-rules/no-use-client-in-server-files.cjs:63-66

The regex already captures \n? at the end, then code removes another newline. Consider letting the regex handle all whitespace consistently.


Performance & Security

Performance: 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 Quality

The ESLint config integration looks good - properly scoped to .server files, registered as custom plugin, and set to appropriate 'error' severity.


Recommendations

Should Consider

  1. Update documentation URL to reference PR Add ESLint rule to prevent 'use client' in server files #1919
  2. Fix regex to use backreferences for matching quotes

Nice to Have

  1. Streamline newline handling in auto-fix
  2. Add test for mixed quote edge case

Summary

This 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>
@claude
Copy link

claude bot commented Nov 5, 2025

Code Review: ESLint Rule for 'use client' in Server Files

Overview

This PR introduces a well-crafted ESLint rule to prevent a critical misconfiguration in React Server Components. The implementation is solid with excellent test coverage.

Strengths

1. Strong Implementation Quality

  • Clear purpose: Solves a specific problem with Shakapacker 9.3.0+ compatibility
  • Good documentation: JSDoc comments explain the rationale and technical context
  • Helpful error message at line 26 guides developers toward the solution
  • Auto-fix support: Automatic removal of invalid directive reduces friction

2. Comprehensive Test Coverage

The test suite covers all critical scenarios:

  • Valid cases: .client.tsx, .tsx, and .server.tsx without directive
  • Invalid cases: Both single/double quotes, with/without semicolons
  • Edge cases: Whitespace handling
  • File extension coverage: Both .server.ts and .server.tsx

3. Proper ESLint Integration

Correctly integrates into eslint.config.ts with proper file targeting and error-level enforcement.

Code Quality Observations

Minor Issues

1. Redundant indexOf Call (line 53)
Since the regex uses ^ anchor, the match always starts at index 0. Could simplify to fixer.removeRange([0, match[0].length])

2. Regex Pattern (line 48)
Pattern allows optional trailing newline but only handles Unix/Mac line endings. Consider whether Windows \r\n support is needed.

Suggestions for Enhancement

  1. Edge Case: Comments Before Directive - Should comments before the directive be allowed?
  2. Test Case: Multiple Directives - Consider adding test for multiple directives
  3. Documentation URL (line 23) - Currently points to this PR, consider updating to permanent docs after merge

Security & Performance

  • No security concerns
  • Performance is excellent: simple regex, runs only on .server.ts(x) files
  • No breaking changes

Final Verdict

LGTM 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.

Recommendation

Approve and merge after addressing any suggestions the team finds valuable.

Great work on this PR!

@justin808 justin808 merged commit fd23d07 into master Nov 5, 2025
26 checks passed
@justin808 justin808 deleted the justin808/no-use-client-lint-rule branch November 5, 2025 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants