use v19.0.3 of react-on-rails-rsc as git dep #2197
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
PR Review: Use v19.0.3 of react-on-rails-rsc as git dependencySummaryThis PR switches the Issues Found🚨 CRITICAL: Missing Changelog EntryPer
|
size-limit report 📦
|
Code ReviewThank you for this PR! I've reviewed the changes to update
|
Code Review for PR #2197SummaryThis PR changes the Critical Issues🚨 1. Incomplete PR Description
Impact: Reviewers and future developers can't understand the purpose of this change. Recommendation: Update the PR description to explain:
🚨 2. PeerDependency Version MismatchFile: The "peerDependencies": {
"react-on-rails-rsc": "19.0.3"
}But the actual dependency installed is now from a git branch which reports version "react": "^19.2.1",
"react-dom": "^19.2.1"Problem:
Impact: Users installing Recommendation: "peerDependencies": {
"react": ">= 19.0.1", // Update if 19.2.1 is now required
"react-dom": ">= 19.0.1",
"react-on-rails-rsc": "19.0.3 || git+https://github.com/shakacode/react_on_rails_rsc"
}Or mark as fully optional if the git URL is temporary.
|
Code Review for PR #2197SummaryThis PR updates
|
Fresh Code Review - PR #2197I notice this PR has received several AI reviews already. Let me provide a concise, actionable summary and highlight issues that haven't been fully addressed. 🎯 The Core Question: Why This Change?The PR title says: "use v19.0.3 of react-on-rails-rsc as git dep" Missing context: What specific issue requires switching from the published npm package to a git dependency? The PR description needs to explain:
|
Code ReviewSummaryThis PR updates the 🔴 Critical Issues1. Incomplete PR TemplateThe PR description still contains all placeholder text and hasn't been filled out. Please provide:
2. Missing CHANGELOG EntryAccording to
3. Git Dependency ConcernsUsing a git dependency instead of a published version has several implications: Testing Requirements (from # Step 1: Test clean install (CRITICAL)
rm -rf node_modules
pnpm install --frozen-lockfile
# Step 2: Test build scripts
pnpm run build
# Step 3: Test package scripts
pnpm run prepack
pnpm run yalc:publishProduction Concerns:
Recommendation: If this is for testing a pre-release version of
4. PeerDependency Version MismatchThe "react-on-rails-rsc": "19.0.3"But you're now using a git dependency that may have different requirements. The lockfile shows the git version expects: react: ^19.2.1
react-dom: ^19.2.1This is stricter than your current devDependencies (
|
Code Review - PR #2197SummaryThis PR upgrades the
|
Code ReviewSummaryThis PR switches the Critical Issues🔴 Version Mismatch in peerDependenciesLocation: The "peerDependencies": {
"react-on-rails-rsc": "19.0.3"
}But the actual dependency is now a git branch. This creates an inconsistency:
Impact: When users install Recommendation: "peerDependencies": {
"react-on-rails-rsc": ">=19.0.3"
}Or if you want to be more specific: "peerDependencies": {
"react-on-rails-rsc": "^19.0.3 || git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"
}
|
Code Review - PR #2197I notice this PR has received several automated reviews already, so I'll provide a concise summary focusing on actionable items. 📋 What This PR DoesChanges Files changed:
🚨 Issues That Must Be Addressed1. Missing Context in PR DescriptionThe PR description is still template boilerplate. Please explain:
2. Peer Dependency Version MismatchIn
Problem: The git branch may have a different version or different peer dependency requirements than "react": "^19.2.1",
"react-dom": "^19.2.1"But your devDependencies use Action needed:
3. CI Failures Need InvestigationFour CI jobs failed:
Per CLAUDE.md testing requirements, before claiming these are fixed: # 1. Check if master is passing these same workflows
gh run list --workflow="Integration Tests" --branch master --limit 3
# 2. Test locally (CRITICAL - simulate CI environment)
rm -rf node_modules
pnpm install --frozen-lockfile
pnpm run build
pnpm run test
# 3. Or use automated tool
bin/ci-rerun-failures 2197If you cannot test locally (e.g., Conductor workspace), please state why in the PR description and mark the fixes as UNTESTED. 4. Mandatory Build Script TestingPer # Remove node_modules to simulate CI
rm -rf node_modules
# Test clean install (MOST CRITICAL)
pnpm install --frozen-lockfile
# Test build
pnpm run build
# Test yalc publish (critical for Pro package)
cd packages/react-on-rails-pro && pnpm run yalc:publishPlease confirm you've run these and they passed. 📝 Documentation Requirements5. CHANGELOG Entry MissingPer This is a Pro package change, so it should be documented in #### [Unreleased]
##### Pro-Only
- [PR 2197](https://github.com/shakacode/react_on_rails/pull/2197) by [AbanoubGhadban](https://github.com/AbanoubGhadban): Temporarily use git dependency for react-on-rails-rsc to support React 19.2.1 peer dependencies(Adjust wording based on whether this is temporary)
|
Code ReviewSummaryThis PR updates the Critical Issues1. Incomplete PR Checklist ❌The PR description still contains template boilerplate and none of the checklist items are checked:
Action Required:
2. peerDependencies Mismatch
|
Code Review - PR #2197SummaryThis PR changes the 🚨 Critical Issues1. Inconsistent peerDependency DeclarationLocation: "peerDependencies": {
"react": ">= 16",
"react-dom": ">= 16",
"react-on-rails-rsc": "19.0.3" // ⚠️ Exact version specified
}Problem: The
Impact: This may cause peer dependency warnings or failures when users install the package. Recommendation: "peerDependencies": {
"react": ">= 16",
"react-dom": ">= 16",
"react-on-rails-rsc": "^19.0.3" // Use semver range instead
}2. Git Dependencies are Anti-Pattern for ProductionProblem: Using git dependencies (especially feature branches) introduces several risks:
Current state: According to the diff, you're pointing to commit Best practices:
Recommendation: If this change is needed for testing unreleased features:
3. Missing PR Description and ContextProblem: The PR body is still the default template with no actual description:
Required information:
4. Test Failures in CIAccording to the CI results, there are FAILURES:
Critical: Before this PR can be approved, you MUST:
📋 Checklist IssuesFrom the PR template, you need to:
Note: According to CLAUDE.md, you should update the appropriate changelog. Since this affects the Pro package, you may need to update 🔍 Code Quality NotesPositive aspects:
Concerns:
🧪 Testing Requirements (from CLAUDE.md)Before pushing, you MUST test: # 1. Clean install (CRITICAL - this is what CI does)
rm -rf node_modules
pnpm install --frozen-lockfile
# 2. Build packages
pnpm run build
# 3. Test yalc publish (validates package structure)
pnpm run yalc:publish
# 4. Run affected tests
cd react_on_rails_pro/spec/dummy
pnpm test:e2e # If E2E tests are affectedFrom CLAUDE.md:
💡 RecommendationsImmediate actions:
If this is temporary (recommended):// packages/react-on-rails-pro/package.json
"devDependencies": {
// TODO: Revert to npm version once [email protected] is published
// Temporary git dependency for testing React 19.2.x compatibility
"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"
}If this must be permanent:Consider forking and publishing your own scoped version: npm publish @your-org/[email protected]📚 References
✅ Approval CriteriaThis PR can be approved when:
Recommendation: Do NOT merge this PR to master with git dependencies unless it's explicitly temporary and well-documented. |
Code Review for PR #2197SummaryThis PR updates the
Issues Found🚨 CRITICAL: Incomplete PR TemplateThe PR body contains the default template text without being filled out:
Action Required: Please fill out the PR template completely, especially:
|
Code ReviewThank you for the PR! I've reviewed the changes and have the following feedback: ✅ What looks good:
|
Code Review SummaryThis PR switches react-on-rails-rsc from npm version ^19.0.3 to a git dependency. While the changes are technically correct, several critical requirements from CLAUDE.md are not met. Blocking Issues1. CI Failures - Must Investigate FirstPer CLAUDE.md Before You Start: Check CI Status - Before investigating failures, verify they are not pre-existing. Currently failing:
Action Required: Determine if these failures are from your changes or pre-existing. If pre-existing, document this in the PR. If from your changes, fix them locally first using bin/ci-rerun-failures 2197 2. Missing PR DescriptionThe PR contains only template placeholders. Required information:
3. Peer Dependency Mismatchpackages/react-on-rails-pro/package.json:
The git dependency expects React ^19.2.1 and react-dom ^19.2.1, but your package tests with React 19.0.1. Action Required: Either update React to 19.2.1 or explain why 19.0.1 is intentional. 4. Missing Mandatory Local TestingPer CLAUDE.md Testing Build and Package Scripts, you MUST test clean install, build, and yalc:publish locally. Action Required: Document that you have run these commands and they passed. 5. Missing CHANGELOG EntryPer CLAUDE.md, Pro changes need documentation in react_on_rails_pro/CHANGELOG.md. This is user-facing because it changes which version gets installed and upgrades React peer dependencies. Action Items Before Merge
Status: Not ready to merge until these items are addressed. Review following CLAUDE.md guidelines for dependency changes and CI failure handling. |
AbanoubGhadban
force-pushed
the
upgrade-to-react-on-rails-rsc-v19.2.1
branch
from
52fbcd2 to
ab20262
Compare
Code Review for PR #2197SummaryThis PR upgrades to React 19.2.1 and react-on-rails-rsc v19.2.1 (from git dependency), removes 🚨 Critical Issues1. Missing Return Statement in
|
AbanoubGhadban
force-pushed
the
upgrade-to-react-on-rails-rsc-v19.2.1
branch
from
5e36ea6 to
e823223
Compare
Code Review for PR #2197Thank you for this PR upgrading to React 19.2.1 and using the git dependency for 🔴 Critical Issues1. Broken Function in
|
Code Review for PR #2197Thank you for working on upgrading the React dependency! I've reviewed the changes and have several critical concerns that need to be addressed before this PR can be merged. 🚨 Critical Issues1. Incomplete PR DescriptionThe PR description still contains template placeholders. Please:
2. Missing CHANGELOG EntryAccording to
Use the 3. Debug Code Must Be Removed
// Fail to findout the chunks content on CI
expect(JSON.stringify(jsonChunks, undefined, 2)).toContain('nnnnnnnnnnnnnnnnnn');This is clearly debug code that will always fail. This must be removed before merging. 4. Disabled Test Assertions
// expect.assertions(258); // ← COMMENTED OUTWhy was this disabled? If the test now produces a different number of assertions with React 19.2.1, the comment should explain why. If this is a temporary workaround, it's a red flag. 5. Broken Function - Returns Undefined
const removeRSCChunkStack = (chunk: string) => {
chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
// ⚠️ MISSING RETURN STATEMENT - This function returns undefined!
};Critical bug: This function is missing a Fix: const removeRSCChunkStack = (chunk: string) => {
return chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};
|
Code ReviewThank you for upgrading to 🚨 Critical Issues1. Broken Function - Missing Return StatementLocation: const removeRSCChunkStack = (chunk: string) => {
chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};Problem: This function doesn't return anything! It should be: const removeRSCChunkStack = (chunk: string) => {
return chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};Impact: This will cause the function to return 2. Debugging Code Left in TestLocation: // Fail to findout the chunks content on CI
expect(JSON.stringify(jsonChunks, undefined, 2)).toContain('nnnnnnnnnnnnnnnnnn');Problem: This is clearly debugging code meant to force a failure and inspect chunk contents on CI. Action Required: Either:
This will always fail and should not be merged. 3. Commented Out AssertionLocation: // expect.assertions(258);Problem: Commented out assertion count suggests the test behavior changed. Questions:
4. Logic Error in AsyncQueueLocation: const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;
if (bufferValueIfExist) {
resolve(bufferValueIfExist as T);Problem: This changes the buffer handling from Questions:
5. Incomplete Buffer Handling LogicLocation: The buffer handling for partial chunks looks correct in principle, but: if (!decodedData.endsWith('\n')) {
buffer = decodedChunksFromData.pop() ?? '';
} else {
buffer = '';
}Concern: What happens if Suggestion: Add a test case specifically for partial chunk handling.
|
Code Review - PR #2197Thank you for working on this upgrade to React 19.2.1 and the react-on-rails-rsc dependency! I've reviewed the changes and have several critical findings that need to be addressed before merging. 🚨 Critical Issues1. Bug in
|
AbanoubGhadban
force-pushed
the
upgrade-to-react-on-rails-rsc-v19.2.1
branch
from
6fbfbb8 to
8759ecd
Compare
Code Review - PR #2197: Use v19.0.3 of react-on-rails-rsc as git depThank you for working on this update! I've reviewed the changes and have several important findings: 🚨 Critical Issues1. Missing Return Statement in
|
PR Review: React 19.2.1 and react-on-rails-rsc Git Dependency UpgradeSummaryThis PR upgrades React to 19.2.1 and switches react-on-rails-rsc to a git dependency. The changes include test timing adjustments, VM context additions, and fixes for chunk parsing logic. 🔴 CRITICAL ISSUES1. Missing Return Statement in
|
Code Review FeedbackThank you for working on this PR! I've reviewed the changes and have several important observations: 🐛 Critical Bug - Missing Return Statement
The const removeRSCChunkStack = (chunk: string) => {
chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};This should be: const removeRSCChunkStack = (chunk: string) => {
return chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};Without the
|
Code Review FeedbackThank you for the pull request upgrading to
|
Code Review - PR #2197Critical Issues Found1. Missing Return Statement (packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39) 2. Type Safety Issue (packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32) 3. Dependency Mismatch (packages/react-on-rails-pro/package.json) Missing Requirements
Recommendation🔴 Request Changes - Fix the missing return statement and type safety issues before merging. Update CHANGELOG_PRO.md and resolve the dependency strategy (either publish react-on-rails-rsc to npm or document the git dependency approach). |
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 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.