Skip to content

Conversation

@AbanoubGhadban
Copy link
Collaborator

Summary

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 9, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch upgrade-to-react-on-rails-rsc-v19.2.1

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 Dec 9, 2025

PR Review: Use v19.0.3 of react-on-rails-rsc as git dependency

Summary

This PR switches the react-on-rails-rsc dependency from npm version ^19.0.3 to a git dependency pointing to the upgrade-to-react-v19.2.1 branch. The change affects both the main Pro package and the dummy test app.


Issues Found

🚨 CRITICAL: Missing Changelog Entry

Per CLAUDE.md, this change should be documented in CHANGELOG_PRO.md since it affects the Pro package:

  • Why: This is a user-visible change that affects which version of react-on-rails-rsc gets installed
  • Action Required: Add an entry explaining the temporary git dependency and why it's needed
  • Format: Follow the format in CLAUDE.md (include PR number and author link)

⚠️ WARNING: Incomplete PR Template

The PR body still contains the template placeholder text:

  • Checklist items are not checked
  • No description of what was changed or why
  • No mention of whether this is temporary or permanent

Recommendation: Update the PR description to explain:

  • Why switching to git dependency (e.g., "Need unreleased React 19.2.1 compatibility fixes")
  • Whether this is temporary (will revert to npm version when published?)
  • Any testing performed

🔍 CONCERN: peerDependencies Version Mismatch

In packages/react-on-rails-pro/package.json:

  • Line 63: peerDependencies still specifies "react-on-rails-rsc": "19.0.3" (exact version)
  • Line 83: devDependencies now uses git branch

Issue: This could cause peer dependency warnings when users install the package, since the git dependency version may not match exactly 19.0.3.

Recommendation: If the git branch has a different version (e.g., 19.0.4 or 19.1.0), update the peerDependency to match or use a range like ^19.0.3.

⚠️ Testing Concern: Build Scripts

Per CLAUDE.md testing guidelines, after modifying package.json you should test:

# Remove node_modules to simulate CI
rm -rf node_modules
pnpm install --frozen-lockfile

# Test build scripts
pnpm run build

# Test yalc publish (critical for Pro package)
cd packages/react-on-rails-pro && pnpm run yalc:publish

Question: Were these tested locally? The PR doesn't mention local testing.


Positive Aspects

Correct workspace syntax maintained: Uses workspace:* for internal dependencies
Consistent changes: Updated both Pro package and dummy app
CI appears to be passing: Most checks are green or in progress
pnpm-lock.yaml updated: Correctly reflects the git dependency


Required Actions Before Merge

  1. Add CHANGELOG_PRO.md entry documenting this change
  2. Update PR description with rationale and testing details
  3. Verify peerDependencies version matches the git branch version
  4. Confirm local testing of build and yalc publish scripts
  5. Check PR template items or remove template boilerplate

Questions for Author

  1. Is this git dependency temporary? Will it revert to npm once a new version is published?
  2. What specific fixes in the upgrade-to-react-v19.2.1 branch are needed?
  3. Have you tested the build and yalc publish flows locally?
  4. Does the version in the git branch match the peerDependency specification?

Risk Assessment

Risk Level: Low-Medium

  • Low risk: Git dependencies work fine in pnpm for internal testing
  • Medium concern: If this is intended for release, users may have issues with git dependencies
  • Blocker: Missing changelog entry violates project guidelines

Overall: The code changes are technically correct, but documentation and testing verification are incomplete. Please address the changelog requirement and confirm local testing before merging.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2025

size-limit report 📦

Path Size
react-on-rails/client bundled (gzip) 62.33 KB (0%)
react-on-rails/client bundled (gzip) (time) 62.33 KB (0%)
react-on-rails/client bundled (brotli) 53.48 KB (0%)
react-on-rails/client bundled (brotli) (time) 53.48 KB (0%)
react-on-rails-pro/client bundled (gzip) 63.38 KB (0%)
react-on-rails-pro/client bundled (gzip) (time) 63.38 KB (0%)
react-on-rails-pro/client bundled (brotli) 54.44 KB (0%)
react-on-rails-pro/client bundled (brotli) (time) 54.44 KB (0%)
registerServerComponent/client bundled (gzip) 127.89 KB (+0.49% 🔺)
registerServerComponent/client bundled (gzip) (time) 127.89 KB (+0.49% 🔺)
registerServerComponent/client bundled (brotli) 61.96 KB (+0.88% 🔺)
registerServerComponent/client bundled (brotli) (time) 61.96 KB (+0.88% 🔺)
wrapServerComponentRenderer/client bundled (gzip) 122.5 KB (+0.5% 🔺)
wrapServerComponentRenderer/client bundled (gzip) (time) 122.5 KB (+0.5% 🔺)
wrapServerComponentRenderer/client bundled (brotli) 57.08 KB (+0.84% 🔺)
wrapServerComponentRenderer/client bundled (brotli) (time) 57.08 KB (+0.84% 🔺)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants