Skip to content

Conversation

@ihabadham
Copy link
Contributor

@ihabadham ihabadham commented Nov 4, 2025

Summary

This PR implements the switch from GitHub Packages to public distribution for React on Rails Pro packages and gem.

Changes

1. Node Renderer NPM Package (react_on_rails_pro/package.json)

  • Removed publishConfig section to publish to npmjs.org instead of GitHub Packages
  • Updated repository URL to reflect monorepo location

2. Release Script (rakelib/release.rake)

  • Updated node-renderer publishing logic (remove GitHub Packages messaging, add npmjs.org OTP prompt)
  • Updated Pro gem publishing: removed --key github --host arguments to publish to RubyGems.org
  • Updated documentation header to list all packages as PUBLIC
  • Updated success message to reflect unified public distribution

3. Documentation Updates

  • Removed all GitHub PAT authentication instructions
  • Added license token security warning
  • Updated all package name references to unscoped version
  • Simplified installation flow

Distribution Strategy

Before:

  • Pro packages published to GitHub Packages (private)
  • Customers need GitHub PAT + JWT license token
  • Manual PAT generation by Justin for each customer

After:

  • All packages published to public registries (npmjs.org + RubyGems.org)
  • Customers only need JWT license token
  • Runtime enforcement via JWT validation (unchanged)
  • Frictionless installation with gem install and npm install

Breaking Change

⚠️ Existing customers using GitHub Packages will need to update their .npmrc configuration after this release. Justin will communicate migration steps directly to customers.

Security

Runtime enforcement remains completely unchanged:

  • JWT license validation at Rails startup (Ruby side)
  • JWT license validation at Node renderer startup (Node side)
  • Grace period system still in place
  • Attribution system still in place

Testing Plan

  • Dry run: rake release[16.2.0,true]
  • Verdaccio test: rake release[16.2.0-test.1,false,verdaccio]
  • Verify packages publish successfully to public registries
  • Test installation without GitHub credentials
  • Test runtime enforcement still works

🤖 Generated with Claude Code


This change is Reviewable

Summary by CodeRabbit

  • Chores

    • Publishing consolidated: node package and gems are now published publicly (npmjs.org & RubyGems.org); private GitHub Packages references removed and post-publish messaging unified; package names simplified (unscoped).
  • Documentation

    • Install, release, and node-renderer docs updated to reflect public publishing, unscoped package name, simplified release commands, license-token runtime flow, clearer startup/config examples, revised install/import instructions, and updated error/tracing guidance.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

Changed publishing from scoped/private GitHub Packages to public registries: the pro node renderer package was renamed to react-on-rails-pro-node-renderer and published to npmjs.org; Ruby gems are published to RubyGems.org; release scripts, examples, imports, and docs updated to use public names and simplified messages.

Changes

Cohort / File(s) Summary
Release workflow
rakelib/release.rake
Removed GitHub Packages/Verdaccio branches and registry-specific messaging; updated prompts and flow so node renderer publishes to npmjs.org and gems to RubyGems.org; post-publish summary updated.
Pro package metadata
react_on_rails_pro/package.json
Renamed package from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer and removed publishConfig (GitHub Packages registry).
Top-level docs
CONTRIBUTING.md, docs/contributor-info/releasing.md
Replaced scoped/private package references with public npm/rubygems guidance; removed private-registry auth instructions; reordered release item list to reflect public packages.
Pro package docs & contributing
react_on_rails_pro/CONTRIBUTING.md, react_on_rails_pro/docs/contributors-info/releasing.md
Removed GitHub Packages authentication/config instructions; added explicit rake release[...] commands and public-publish guidance.
Installation & node-renderer docs
react_on_rails_pro/docs/installation.md, react_on_rails_pro/docs/node-renderer/*.md, react_on_rails_pro/docs/code-splitting-loadable-components.md, react_on_rails_pro/docs/node-renderer/js-configuration.md, react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
Switched install commands and import/require examples from scoped GitHub/Git URLs to react-on-rails-pro-node-renderer; added license-token and env-driven renderer config examples; updated startup scripts and examples to reference public package name.
Examples & tests
react_on_rails_pro/spec/dummy/package.json, react_on_rails_pro/spec/dummy/client/node-renderer.js
Updated local link dependency and preinstall script to use react-on-rails-pro-node-renderer; adjusted imports in dummy client to the unscoped package and integrations.
Inline doc comments
react_on_rails_pro/packages/node-renderer/src/integrations/api.ts
Updated documentation example import paths from scoped to unscoped package name.
Packaging checks
react_on_rails_pro/package-scripts.yml
Updated prepack error message to reference unscoped package name.

Sequence Diagram(s)

sequenceDiagram
  participant Dev as Release task
  participant NPM as npmjs.org
  participant Ruby as RubyGems.org

  rect rgb(220,245,255)
    Note over Dev,NPM: Node renderer publish (PUBLIC)
    Dev->>NPM: npm publish react-on-rails-pro-node-renderer
    NPM-->>Dev: OTP / success
  end

  rect rgb(245,245,220)
    Note over Dev,Ruby: Ruby gems publish (PUBLIC)
    Dev->>Ruby: gem push react_on_rails-*.gem / react_on_rails_pro-*.gem
    Ruby-->>Dev: OTP / success
  end

  Note over Dev: Post-release summary references npmjs.org and RubyGems.org only
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

  • Focus areas: rakelib/release.rake (registry logic, OTP prompts, messaging).
  • Also review: react_on_rails_pro/package.json rename and any remaining scoped references in docs and examples.
  • Verify CI/preinstall linkage in react_on_rails_pro/spec/dummy/package.json.

Possibly related PRs

Suggested reviewers

  • justin808
  • alexeyr-ci2
  • Judahmeek

Poem

🐰 I hopped through names and tidied the trail,
Unscoped my package and sent out the mail,
npm and gems now greet the open sky,
Docs aligned and scripts waved bye‑bye,
Carrots, code, and one cheerful tail 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main objective: switching React on Rails Pro packages from private GitHub Packages to public registries (npmjs.org and RubyGems.org), which is reflected throughout all changes in the changeset.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ 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 switch-to-public-distribution

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 4, 2025

Code Review - PR #1901: Switch Pro packages to public distribution

Overview

This PR transitions React on Rails Pro packages from GitHub Packages (private) to public registries (npmjs.org + RubyGems.org), maintaining runtime JWT enforcement for license validation. This is a strategic change that simplifies customer onboarding while keeping the security model intact.


✅ Strengths

1. Clean, Focused Changes

  • The code changes are minimal and surgical (13 additions, 27 deletions)
  • Only touches what's necessary: package.json publishConfig and release.rake script
  • Good separation of concerns - doesn't touch the runtime enforcement logic

2. Well-Documented Approach

  • Excellent PR description with clear before/after comparison
  • Testing plan included with multiple validation steps
  • References to supporting analysis documents (good due diligence)

3. Consistent with Established Patterns

  • Follows @AbanoubGhadban's original pattern from react-on-rails-pro NPM package
  • Aligns with industry standards (GitLab, Elastic, Sentry, Sidekiq)

⚠️ Issues & Recommendations

CRITICAL: Documentation is Now Outdated

The code changes look good, but multiple documentation files still reference GitHub Packages and will mislead customers after this change:

1. Installation Documentation (react_on_rails_pro/docs/installation.md)

Lines 1-86 contain extensive GitHub Packages setup instructions that will be completely wrong after this PR:

  • Line 2: "Since the repository is private, you will get a GitHub Personal Access Token..."
  • Lines 23-45: GitHub Packages Gemfile configuration
  • Lines 65-85: GitHub Packages .npmrc configuration

Action Required: This entire file needs to be rewritten to reflect:

  • Public installation via gem install react_on_rails_pro
  • Public NPM installation without authentication
  • JWT license token setup (the only authentication needed)
  • Remove all GitHub PAT instructions

2. Release Documentation (docs/contributor-info/releasing.md)

Lines 63, 158-179 still list Pro packages as "PRIVATE (GitHub Packages)":

  • Line 63: "PRIVATE (GitHub Packages): 4. @shakacode-tools/react-on-rails-pro-node-renderer..."
  • Lines 158-179: GitHub Packages authentication instructions

Action Required: Update to match the new public distribution model.

3. Pro Release Documentation (react_on_rails_pro/docs/contributors-info/releasing.md)

Lines 39-40 still reference GitHub Packages.


Code Quality Issues

1. Minor: Inconsistent Messaging

# Line 218: release.rake
puts "Carefully add your OTP for NPM when prompted." unless use_verdaccio

This message is good, but consider making it more consistent with the RubyGems message at line 239. Suggestion:

puts "You will be prompted for your NPM OTP (one-time password)." unless use_verdaccio

Testing & Validation Concerns

1. Testing Plan is Good but Could Be Enhanced

Your testing plan includes:

  • ✅ Dry run
  • ✅ Verdaccio test
  • ✅ Verify public publishing
  • ✅ Test installation without GitHub credentials
  • ✅ Test runtime enforcement

Additional test to consider:

  • Test what happens when an existing customer with GitHub Packages configuration tries to upgrade
  • Does their old .npmrc with GitHub Packages registry cause issues?
  • Should we provide migration instructions?

2. Rollback Plan

Consider documenting:

  • What if we need to rollback after publishing to public registries?
  • Can we unpublish from npmjs.org if there's an issue?
  • Timeline/window for catching issues?

Security Considerations

Security Model is Sound

The runtime JWT enforcement remains unchanged, which is correct:

  • Ruby gem validates JWT at Rails startup
  • Node renderer validates JWT at startup
  • Grace period system intact
  • Attribution system intact

🤔 Consider: Public Visibility of Code

  • Once published publicly, anyone can download and inspect the Pro packages
  • This is standard for commercial software (as noted: GitLab, Sidekiq, etc.)
  • Ensure there are no embedded secrets, API keys, or sensitive implementation details
  • The preinstall.js script and license validation should be reviewed for any leakage

Recommendation: Do a final audit of what's in the published packages:

# Before publishing, check what files are included
cd react_on_rails_pro && npm pack --dry-run

Performance Considerations

✅ No performance impact - this is purely a distribution change.


📋 Action Items Before Merge

Must-Have (Blocking):

  1. Update react_on_rails_pro/docs/installation.md - Complete rewrite required
  2. Update docs/contributor-info/releasing.md - Remove GitHub Packages references
  3. Update react_on_rails_pro/docs/contributors-info/releasing.md - Update package list
  4. Security audit - Review published package contents for any sensitive data

Nice-to-Have:

  1. 📝 Migration guide - Help existing customers transition from GitHub Packages config
  2. 📝 Rollback plan - Document contingency if issues arise post-release
  3. 🧪 Test existing customer upgrade path - Simulate upgrading with old .npmrc

🎯 Summary

Code Quality: ✅ Excellent - clean, minimal, focused changes
Best Practices: ✅ Follows industry patterns and existing codebase patterns
Security: ✅ Runtime enforcement model is sound
Documentation:Critical issue - Must be updated before merge
Testing: ⚠️ Good plan, could be enhanced with migration testing


Recommendation

Request Changes - The code changes are solid, but the documentation is critically outdated and will cause customer confusion and support burden. Once the documentation is updated to reflect public distribution, this will be ready to merge.

The strategic direction is sound and aligns with industry best practices. This will significantly improve the developer experience for Pro customers. 🚀


Related Files to Update

react_on_rails_pro/docs/installation.md (lines 1-86+)
docs/contributor-info/releasing.md (lines 63, 158-179)
react_on_rails_pro/docs/contributors-info/releasing.md (lines 39-40)

Great work on the analysis and implementation! Let me know if you'd like help updating the documentation files.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f195a7 and e050df6.

📒 Files selected for processing (2)
  • rakelib/release.rake (4 hunks)
  • react_on_rails_pro/package.json (0 hunks)
💤 Files with no reviewable changes (1)
  • react_on_rails_pro/package.json
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.

Applied to files:

  • rakelib/release.rake
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Applied to files:

  • rakelib/release.rake
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.

Applied to files:

  • rakelib/release.rake
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.

Applied to files:

  • rakelib/release.rake
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.

Applied to files:

  • rakelib/release.rake
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Applied to files:

  • rakelib/release.rake
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.

Applied to files:

  • rakelib/release.rake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: build
  • GitHub Check: lint-js-and-ruby
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.2, latest)
  • GitHub Check: rspec-package-tests (3.4, minimum)
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
  • GitHub Check: claude-review
  • GitHub Check: build-dummy-app-webpack-test-bundles

Comment on lines +217 to 221
puts "\nPublishing #{node_renderer_name}@#{actual_npm_version}..."
puts "Carefully add your OTP for NPM when prompted." unless use_verdaccio
sh_in_dir(pro_gem_root,
"yarn publish --new-version #{actual_npm_version} --no-git-tag-version #{npm_publish_args}")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add --access public when publishing the scoped renderer package

Scoped npm packages default to private. On the first public release, yarn publish without --access public fails with the npm error “You must provide --access public to publish scoped packages.” That blocks the new distribution flow this PR introduces. Please append the flag (while skipping it for Verdaccio runs) so the publish step succeeds.

-    sh_in_dir(pro_gem_root,
-              "yarn publish --new-version #{actual_npm_version} --no-git-tag-version #{npm_publish_args}")
+    publish_command = [
+      "yarn publish",
+      "--new-version #{actual_npm_version}",
+      "--no-git-tag-version"
+    ]
+    publish_command << npm_publish_args unless npm_publish_args.to_s.empty?
+    publish_command << "--access public" unless use_verdaccio
+    sh_in_dir(pro_gem_root, publish_command.join(" "))
🤖 Prompt for AI Agents
In rakelib/release.rake around lines 217 to 221, the yarn publish command needs
the --access public flag for scoped packages; update the sh_in_dir call so that
when use_verdaccio is false you append "--access public" to the yarn publish
arguments (while preserving existing npm_publish_args and --no-git-tag-version),
and do not add the flag when use_verdaccio is true.

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review - PR #1901: Switch Pro packages to public distribution

Summary

This PR transitions the Pro packages from private GitHub Packages distribution to public npm/RubyGems distribution, simplifying customer installation while maintaining runtime JWT license validation.


✅ Code Quality & Best Practices

Strengths:

  • Clean refactoring: Removed unnecessary conditional logic around registry selection
  • Improved messaging: User-facing messages are clearer and more accurate
  • Simplified authentication flow: Eliminates GitHub PAT requirement, reducing customer friction
  • Consistent formatting: Changes follow project conventions

Observations:

  • The removal of publishConfig in package.json is correct for public npm distribution
  • The switch from --key github --host to standard gem release properly targets RubyGems.org

🐛 Potential Issues

Minor:

  1. CI Failure: One test is failing - rspec-dummy-app-node-renderer in React on Rails Pro Integration Tests. This should be investigated before merge.

  2. Typo in testing plan: The PR description shows an incomplete item:

    • "Test runtime enforcement still" (appears truncated)
  3. No rollback plan documented: Consider documenting the rollback procedure if issues arise post-deployment (e.g., yanking versions, reverting to GitHub Packages).


🔒 Security Considerations

Strong points:

  • ✅ Runtime JWT validation remains unchanged (critical security boundary preserved)
  • ✅ Grace period system unaffected
  • ✅ Public distribution doesn't expose any secrets (packages are secured by runtime license checks)

Recommendations:

  1. Verify npmjs.org 2FA: Ensure OTP/2FA is properly configured for the publishing account
  2. Gem credentials security: Confirm ~/.gem/credentials has appropriate permissions (should be 0600)
  3. Consider package immutability: Once published to public registries, versions cannot be unpublished easily - ensure version bump strategy is clear

⚡ Performance Considerations

Positive impacts:

  • ✅ Faster installation for customers (no GitHub API rate limits)
  • ✅ Better CDN performance from npmjs.org vs GitHub Packages
  • ✅ Reduced authentication overhead during CI/CD

Neutral:

  • No performance changes to runtime behavior (as expected)

🧪 Test Coverage

Current state:

  • ✅ Most CI checks passing (20/21 passing)
  • ❌ One failure needs investigation: rspec-dummy-app-node-renderer

Testing plan review:
The PR description mentions testing with:

  • Dry run
  • Verdaccio local testing
  • Public registry verification
  • Installation without GitHub credentials
  • Runtime enforcement validation

Recommendations:

  1. ✅ Complete the testing checklist items before merge
  2. Add a test for the error case: "What happens if JWT license is invalid during install?" (should succeed install, fail at runtime)
  3. Consider testing the actual release process in Verdaccio mode first

📝 Documentation & Messaging

Updates needed:

  1. The script header correctly updates the distribution list
  2. Success messages now properly reflect public distribution
  3. OTP prompts are appropriately added

Suggestions:

  • Consider updating any customer-facing documentation about installation (this PR doesn't show doc changes)
  • Verify that release documentation (docs/ if any) is updated

🎯 Breaking Changes Assessment

For customers:

  • ✅ Non-breaking: Existing installations continue to work
  • ✅ Improved UX: New installations are simpler (no GitHub PAT needed)
  • ✅ Backward compatible: Package names unchanged

For maintainers:

  • ⚠️ Process change: Publishing now requires npmjs.org and RubyGems.org OTP instead of GitHub tokens
  • ⚠️ Credential management: Ensure publishing credentials are properly set up and documented

🚀 Release Readiness

Before merging:

  • Fix the failing rspec-dummy-app-node-renderer test
  • Complete all testing plan checklist items in PR description
  • Verify publishing credentials are configured:
    • npmjs.org account with 2FA
    • RubyGems.org account with OTP
    • Proper .npmrc and ~/.gem/credentials setup
  • Document rollback procedure
  • Update customer-facing installation docs (if applicable)

After merging (first release):


📊 Overall Assessment

Rating: ✅ Approve with minor fixes

This is a well-structured PR that achieves its stated goal of simplifying distribution. The code changes are clean and focused. The main blocker is the failing test, which should be resolved before merge.

The strategic decision to move to public distribution with runtime license enforcement is sound and significantly improves customer experience.

Estimated risk: Low (assuming runtime validation is well-tested)


🤖 Review generated by Claude Code

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review: Switch Pro packages to public distribution

Overview

This PR implements a significant architectural change, moving React on Rails Pro packages from private GitHub Packages to public distribution (npmjs.org + RubyGems.org). I've reviewed the changes against code quality, security, performance, and best practices.


Strengths

1. Clean Implementation

  • The changes are minimal and focused (13 additions, 27 deletions)
  • Simplifies the release process by removing registry-specific conditional logic
  • Reduces friction for customers (no more manual PAT generation)

2. Security Model Intact

  • Runtime JWT validation remains unchanged (verified in license_validator.rb and licenseValidator.ts)
  • Grace period system still in place
  • No changes to the actual enforcement mechanism
  • Security is enforced at runtime, not at distribution time

3. Good Code Quality

  • Consistent messaging updates throughout release.rake
  • Proper removal of GitHub Packages-specific configuration
  • Documentation strings updated to reflect new distribution strategy

⚠️ Critical Issues

1. Missing Documentation Updates (HIGH PRIORITY)

The PR changes the distribution model but doesn't update customer-facing documentation:

Files that MUST be updated:

  • react_on_rails_pro/docs/installation.md (lines 1-80):

    • Still instructs customers to use GitHub PAT tokens
    • Still references npm.pkg.github.com and rubygems.pkg.github.com
    • Should be completely rewritten to show simple gem install and npm install commands
  • react_on_rails_pro/docs/contributors-info/releasing.md (lines 39-40):

    • Still says node-renderer and pro gem are on "GitHub Packages"
    • Should reflect the new public distribution model
  • react_on_rails_pro/CI_SETUP.md (if it exists):

    • Likely contains GitHub Packages configuration
    • Should be updated or removed

Example of what installation.md should look like after this PR:

# Installation

## Ruby Gem

Add to your Gemfile:
```ruby
gem 'react_on_rails_pro', '~> 16.2.0'

Then run:

bundle install

NPM Package (Node Renderer)

Install via npm or yarn:

npm install @shakacode-tools/react-on-rails-pro-node-renderer
# or
yarn add @shakacode-tools/react-on-rails-pro-node-renderer

License Configuration

After installation, configure your JWT license token as described in LICENSE_SETUP.md.

2. Testing Plan Incomplete

The PR description includes a testing checklist, but items are unchecked:

  • Dry run
  • Verdaccio test
  • Verify public registry publishing
  • Test installation without GitHub credentials
  • Test runtime enforcement

Recommendation: Complete all testing steps before merging, especially testing installation without any GitHub credentials.


🔍 Potential Issues

3. Release Script Edge Cases

Issue: The release script still has Verdaccio-specific logic but the messaging might be confusing:

# Line 212-218 (after PR changes)
puts "Publishing PUBLIC node-renderer to #{use_verdaccio ? 'Verdaccio (local)' : 'npmjs.org'}..."

This is correct, but there's potential confusion since the header comment still says node-renderer is "PUBLIC" even in Verdaccio mode. Consider clarifying that Verdaccio is for testing only.

Suggestion:

# Publish node-renderer NPM package (PUBLIC on npmjs.org, Verdaccio for testing only)
puts "\n#{"=" * 80}"
if use_verdaccio
  puts "Publishing node-renderer to Verdaccio (LOCAL TESTING ONLY)..."
else
  puts "Publishing PUBLIC node-renderer to npmjs.org..."
end

4. Preinstall Script Considerations

The react_on_rails_pro/script/preinstall.js runs during npm install and attempts to link local dependencies. This is fine for development, but ensure:

  • It gracefully handles absence of yalc in production (✅ already handles this)
  • It doesn't cause issues when installed from public npm (✅ checks for node_modules)

Status: Already handled correctly, but worth noting in release testing.


💡 Suggestions

5. RuboCop Compliance

Based on CLAUDE.md requirements, ensure you run:

bundle exec rubocop

before committing. The changes look clean, but this is mandatory per project guidelines.

6. Changelog Update

This is a significant user-facing change. Update CHANGELOG.md with:

#### [Unreleased]
##### Changed
- **BREAKING**: Pro packages now distributed via public registries (npmjs.org, RubyGems.org) instead of GitHub Packages. Customers no longer need GitHub PAT tokens - only JWT license token required. [PR 1901](https://github.com/shakacode/react_on_rails/pull/1901) by [ihabadham](https://github.com/ihabadham)

Use the /update-changelog command for proper formatting.

7. Communication Plan

This is a breaking change for installation workflows. Consider:

  • Email announcement to existing customers
  • Migration guide for updating CI/CD pipelines
  • Blog post explaining the simplified installation

🔒 Security Analysis

✅ Runtime Security Unchanged

  • JWT validation remains at Rails startup (lib/react_on_rails_pro/engine.rb)
  • Node renderer validates license on startup (packages/node-renderer/src/master.ts)
  • Grace period and attribution systems intact
  • License validation tests exist (spec/react_on_rails_pro/license_validator_spec.rb)

⚠️ Public Package Implications

Consideration: Making packages public means anyone can download them without authentication.

Mitigation (already in place):

  • Packages are useless without valid JWT license
  • Preinstall script requires valid license at installation time
  • Runtime validation prevents unauthorized use

Recommendation: Ensure LICENSE_SETUP.md clearly documents that the JWT token is required and how to obtain it.


📊 Performance Considerations

Positive Impacts

  • ✅ Faster installation (no GitHub API rate limits)
  • ✅ Better CDN distribution via npm/RubyGems infrastructure
  • ✅ Reduced authentication overhead during bundle install / npm install

No Negative Impacts

  • No performance degradation expected
  • Release process might be slightly faster (fewer registry switches)

🧪 Test Coverage

Existing Coverage

  • ✅ License validation has comprehensive tests
  • ✅ Preinstall script has error handling
  • ✅ Release script has dry-run mode

Recommended Additional Testing

  1. Manual testing:

    • Install on a fresh machine without any GitHub credentials
    • Verify license validation still works
    • Test with expired license to ensure grace period works
  2. CI/CD testing:

    • Ensure GitHub Actions workflows don't need updates
    • Verify builds work without GitHub Packages credentials

📝 Summary & Recommendations

Before Merging (REQUIRED):

  1. Update installation.md - remove all GitHub Packages references
  2. Update releasing.md - reflect new public distribution
  3. Complete testing checklist in PR description
  4. Update CHANGELOG.md with breaking change notice
  5. Run bundle exec rubocop (per CLAUDE.md)
  6. ⚠️ Verify trailing newlines in modified files (per CLAUDE.md)

After Merging (RECOMMENDED):

  1. 📧 Send customer communication about simplified installation
  2. 📝 Update any external documentation/wikis
  3. 🧹 Remove obsolete GitHub Packages credentials from CI

🎯 Verdict

Conditionally Approve - The code changes are solid and well-implemented, but documentation updates are blocking. The technical implementation is sound, security is preserved, and this is a valuable improvement for customers.

Risk Level: Medium (breaking change, but mitigated by clear migration path)

Estimated Effort to Address: 1-2 hours (mostly documentation updates)


Great work on simplifying the installation process! The technical implementation is clean and the security model remains intact. Just need those documentation updates before shipping. 🚀

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: Switch Pro packages to public distribution

I've reviewed PR #1901 comprehensively. Here's my detailed feedback:

✅ Code Quality & Best Practices

Strengths:

  • Clean, focused changes that accomplish exactly what's needed
  • Good separation of concerns - only touching files that need modification
  • Consistent messaging updates throughout the release script
  • Well-structured PR description with clear before/after comparison

Areas of Concern:

  1. Documentation is out of sync - The installation docs (react_on_rails_pro/docs/installation.md) still contain extensive instructions for GitHub Packages authentication, which will be outdated after this PR. This needs updating.
  2. Comment accuracy - Line 211 in release.rake still says "to Verdaccio or GitHub Packages depending on mode" but it should say "to Verdaccio or npmjs.org depending on mode"

🐛 Potential Issues

  1. Missing documentation updates - The following files reference GitHub Packages and need updates:

    • /react_on_rails_pro/docs/installation.md - Contains detailed GitHub PAT setup instructions
    • Need to verify if /react_on_rails_pro/CONTRIBUTING.md needs updates
  2. Incomplete comment cleanup - In release.rake:211, the comment still references GitHub Packages:

    # Publish node-renderer NPM package (to Verdaccio or GitHub Packages depending on mode)

    Should be:

    # Publish node-renderer NPM package (to Verdaccio or npmjs.org depending on mode)
  3. Header documentation mismatch - The header comment at lines 29-35 was updated but line 34 should read:

    - @shakacode-tools/react-on-rails-pro-node-renderer NPM package

    Currently shows it on line 32, but it belongs with the other public packages now.

🔒 Security Considerations

Positive:

  • Runtime JWT validation remains in place (good security-in-depth approach)
  • No changes to actual authentication/authorization logic
  • Grace period system unchanged

Recommendations:

  1. Version pinning strategy - Consider if you want to add .npmrc or similar to prevent accidental installation of pre-public versions
  2. Deprecation notices - Consider adding deprecation notices to old GitHub Packages versions directing users to the public registry
  3. License validation monitoring - Ensure monitoring is in place to detect license violations once packages are publicly accessible

⚡ Performance Considerations

No performance concerns - this is purely a distribution change. The actual package contents and runtime behavior are unchanged.

🧪 Test Coverage

Concerns:

  1. No automated tests for release script - The release.rake file has no test coverage. Consider adding tests for:

    • Version bumping logic
    • Registry selection logic
    • Success/failure message generation
  2. Testing plan in PR is incomplete - The checkboxes are not checked. Before merging:

    • Complete dry run testing
    • Complete Verdaccio testing
    • Verify runtime enforcement still works
    • Test installation from public registries

📝 Recommendations

Critical (Must fix before merge):

  1. Update /react_on_rails_pro/docs/installation.md to remove GitHub Packages instructions and replace with simple public registry installation
  2. Fix comment on line 211 in release.rake
  3. Complete all items in the testing plan

Important (Should fix):

  1. Update header documentation to properly categorize all packages as public
  2. Add deprecation notice strategy for existing GitHub Packages installations
  3. Update CONTRIBUTING.md if it references GitHub Packages setup

Nice to have:

  1. Add automated tests for the release script
  2. Consider adding a MIGRATION.md guide for existing customers switching from GitHub Packages to public distribution
  3. Add logging/monitoring for license validation to detect potential abuse

📊 Overall Assessment

This is a well-executed change that accomplishes its goal cleanly. The code changes are solid, but the documentation is significantly out of date and must be updated before this can be merged. The installation docs still instruct users to set up GitHub PATs and authenticate with GitHub Packages, which will confuse users after the switch.

Recommendation: Request changes - Update documentation before merging.


Review generated by Claude Code - Let me know if you'd like me to help update the documentation files!

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review: Switch Pro packages to public distribution

I've reviewed PR #1901 and have the following feedback:

✅ Strengths

  1. Clear strategic direction: The shift from GitHub Packages to public registries (npmjs.org + RubyGems.org) is well-justified and will significantly improve the developer experience by eliminating the need for GitHub PAT management.

  2. Consistent changes: The modifications correctly update both the package.json configuration and the release script to align with the new distribution strategy.

  3. Security maintained: The PR description clearly states that runtime JWT validation remains unchanged, which is the correct approach for license enforcement.

  4. Good documentation: The PR includes a comprehensive testing plan with specific checkboxes.

🔍 Issues & Concerns

Critical Issues

  1. Documentation not updated ⚠️

    • The file react_on_rails_pro/docs/installation.md still contains instructions for GitHub Packages authentication (lines 1-80)
    • This will cause significant confusion for customers trying to install the packages after this change
    • Action required: Update installation.md to remove GitHub PAT/token instructions and simplify to standard gem install and npm install commands
    • Files to update:
      • /react_on_rails_pro/docs/installation.md - needs complete rewrite of installation sections
      • Any other docs referencing GitHub Packages authentication
  2. Incomplete comment updates in release.rake

    • Line 29-35: The header comment still lists packages as PRIVATE (GitHub Packages) in the current main branch
    • The PR changes this correctly, but verify the comment accurately reflects reality
    • After PR: All listed as PUBLIC - this is correct ✓

Medium Priority Issues

  1. Missing .npmrc cleanup guidance

    • Existing users may still have .npmrc files configured for GitHub Packages
    • Consider adding migration notes or cleanup instructions to the PR description or release notes
    • This could cause confusion during upgrades if users have conflicting registry settings
  2. Test coverage concerns

    • The testing plan mentions Verdaccio testing, but doesn't include:
      • Testing actual installation from public registries after publish
      • Verifying that existing customers with GitHub PAT-based setups can migrate smoothly
      • Testing that JWT validation still works correctly with public packages
    • Recommendation: Add these scenarios to the testing checklist
  3. Release script safety

    • The script removes all GitHub-specific authentication hints but doesn't add verification that the user is authenticated with npmjs.org and RubyGems.org
    • Consider adding checks like npm whoami or equivalent to fail fast if authentication is missing

🔐 Security Considerations

Runtime enforcement preserved: The PR correctly maintains JWT validation at runtime
No credential exposure: Removing GitHub PAT requirements reduces credential management surface area
⚠️ Package ownership verification: Ensure that the npmjs.org and RubyGems.org accounts that will publish these packages have appropriate security controls (2FA, strong credentials, limited access)

📋 Recommendations

Before merging:

  1. MUST: Update react_on_rails_pro/docs/installation.md to reflect the new public installation process
  2. MUST: Search the entire codebase for references to GitHub Packages and update them
  3. SHOULD: Add authentication verification to the release script
  4. SHOULD: Create migration notes for existing customers
  5. SHOULD: Expand the testing plan to include end-to-end installation testing

Testing checklist additions:

  • Remove all GitHub authentication from local environment
  • Install packages from public registries with only JWT token
  • Verify JWT validation triggers correctly
  • Test upgrade path from private to public packages
  • Verify that grace period system still works

📝 Minor Suggestions

  • Consider adding a Migration Guide section to the CHANGELOG for this release
  • Add a note in the commit message about the breaking change to installation instructions
  • Verify that any CI/CD pipelines or automation scripts aren't hardcoded to GitHub Packages

Overall Assessment

This is a well-intentioned and strategically sound change that will significantly improve the user experience. However, it's not ready to merge due to the critical documentation gap. The installation docs must be updated before this can safely go out, as it will cause immediate confusion and support burden.

Recommended path forward:

  1. Update all documentation referencing GitHub Packages
  2. Add comprehensive migration notes
  3. Expand testing to include customer-facing scenarios
  4. Merge after verification

Would you like me to help with updating the installation documentation or creating a migration guide?

ihabadham pushed a commit that referenced this pull request Nov 5, 2025
Following PR #1901's switch from GitHub Packages to public distribution,
update all documentation to reflect that React on Rails Pro packages are
now publicly available on npmjs.org and RubyGems.org.

Changes:
- Remove all GitHub Personal Access Token requirements
- Remove .npmrc and gem credentials configuration steps
- Simplify installation to standard gem/npm commands
- Update releasing docs to show all 5 packages as public
- Remove "private repository" language throughout
- Update troubleshooting to remove token-related issues

Files updated:
- react_on_rails_pro/docs/installation.md (major simplification)
- docs/contributor-info/releasing.md
- react_on_rails_pro/docs/contributors-info/releasing.md
- react_on_rails_pro/docs/contributors-info/onboarding-customers.md
- react_on_rails_pro/docs/troubleshooting.md
- react_on_rails_pro/docs/ruby-api.md
- react_on_rails_pro/docs/code-splitting-loadable-components.md
- react_on_rails_pro/docs/node-renderer/basics.md
- react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
ihabadham and others added 3 commits November 5, 2025 23:05
Remove GitHub Packages configuration to publish Pro packages publicly on npmjs.org and RubyGems.org.

Changes:
- Remove publishConfig from react_on_rails_pro/package.json (node-renderer)
- Update release script to publish Pro gem to RubyGems.org (remove --key github --host)
- Update all messaging from PRIVATE/GitHub Packages to PUBLIC/npmjs.org/RubyGems.org
- Update script documentation to reflect all packages are now public
- Update success message for unified public distribution

This follows Bob's pattern from react-on-rails-pro NPM package (already configured for public).

Runtime enforcement via JWT license validation remains unchanged.
Change package name from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer (unscoped).

Rationale:
- @shakacode-tools scope was only needed for GitHub Packages (requires scoping)
- Now publishing to public npmjs.org where scoping is optional
- Matches naming pattern of react-on-rails and react-on-rails-pro (both unscoped)
- Unscoped packages are public by default (no --access public flag needed)
- @shakacode-tools npm org doesn't exist on npmjs.org

Migration impact: Small number of Pro customers will need to update their node-renderer.js imports when upgrading (Justin will handle directly).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Remove all references to GitHub Packages and private distribution.
Update package name from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer throughout documentation.

Changes:
- Completely rewrite installation.md to remove GitHub PAT authentication
- Simplify to standard gem install and npm install commands
- Add JWT license token configuration section
- Update all code examples with unscoped package name
- Remove GitHub Packages authentication instructions
- Update contributor documentation to reflect all packages are public
- Update CONTRIBUTING.md files to remove private package sections

Documentation files updated:
- react_on_rails_pro/docs/installation.md (major rewrite)
- react_on_rails_pro/docs/node-renderer/basics.md
- react_on_rails_pro/docs/node-renderer/js-configuration.md
- react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
- react_on_rails_pro/docs/code-splitting-loadable-components.md
- docs/contributor-info/releasing.md
- react_on_rails_pro/docs/contributors-info/releasing.md
- CONTRIBUTING.md
- react_on_rails_pro/CONTRIBUTING.md

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
react_on_rails_pro/docs/installation.md (1)

56-68: Clarify initializer requirement vs. environment variable setup.

Line 58 states "You don't need to create an initializer if you are satisfied with the defaults," but the license configuration (lines 39-54) appears to require setting the license token. Clarify whether this statement assumes the license token is always configured via environment variable, and update the text to make this explicit.

Example revision for line 58:
"You don't need to create an initializer if you've configured the license via environment variable and are satisfied with other defaults."

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f421185 and 581605d.

📒 Files selected for processing (9)
  • CONTRIBUTING.md (1 hunks)
  • docs/contributor-info/releasing.md (1 hunks)
  • react_on_rails_pro/CONTRIBUTING.md (1 hunks)
  • react_on_rails_pro/docs/code-splitting-loadable-components.md (1 hunks)
  • react_on_rails_pro/docs/contributors-info/releasing.md (1 hunks)
  • react_on_rails_pro/docs/installation.md (1 hunks)
  • react_on_rails_pro/docs/node-renderer/basics.md (1 hunks)
  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md (7 hunks)
  • react_on_rails_pro/docs/node-renderer/js-configuration.md (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • react_on_rails_pro/docs/code-splitting-loadable-components.md
  • docs/contributor-info/releasing.md
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.

Applied to files:

  • CONTRIBUTING.md
  • react_on_rails_pro/docs/node-renderer/basics.md
  • react_on_rails_pro/docs/node-renderer/js-configuration.md
  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
  • react_on_rails_pro/CONTRIBUTING.md
  • react_on_rails_pro/docs/contributors-info/releasing.md
  • react_on_rails_pro/docs/installation.md
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Applied to files:

  • CONTRIBUTING.md
  • react_on_rails_pro/docs/node-renderer/basics.md
  • react_on_rails_pro/docs/node-renderer/js-configuration.md
  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
  • react_on_rails_pro/CONTRIBUTING.md
  • react_on_rails_pro/docs/contributors-info/releasing.md
  • react_on_rails_pro/docs/installation.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.

Applied to files:

  • CONTRIBUTING.md
  • react_on_rails_pro/docs/node-renderer/basics.md
  • react_on_rails_pro/docs/node-renderer/js-configuration.md
  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
  • react_on_rails_pro/CONTRIBUTING.md
  • react_on_rails_pro/docs/contributors-info/releasing.md
  • react_on_rails_pro/docs/installation.md
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.

Applied to files:

  • CONTRIBUTING.md
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

  • CONTRIBUTING.md
  • react_on_rails_pro/docs/node-renderer/js-configuration.md
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.

Applied to files:

  • react_on_rails_pro/docs/node-renderer/js-configuration.md
  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
  • react_on_rails_pro/CONTRIBUTING.md
  • react_on_rails_pro/docs/contributors-info/releasing.md
  • react_on_rails_pro/docs/installation.md
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.

Applied to files:

  • react_on_rails_pro/docs/node-renderer/js-configuration.md
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.

Applied to files:

  • react_on_rails_pro/docs/node-renderer/js-configuration.md
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.

Applied to files:

  • react_on_rails_pro/docs/node-renderer/js-configuration.md
  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.

Applied to files:

  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.

Applied to files:

  • react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md
  • react_on_rails_pro/CONTRIBUTING.md
  • react_on_rails_pro/docs/installation.md
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Applied to files:

  • react_on_rails_pro/CONTRIBUTING.md
  • react_on_rails_pro/docs/contributors-info/releasing.md
  • react_on_rails_pro/docs/installation.md
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.

Applied to files:

  • react_on_rails_pro/docs/installation.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: build
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: lint-js-and-ruby
  • GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: claude-review
  • GitHub Check: markdown-link-check
🔇 Additional comments (15)
CONTRIBUTING.md (1)

131-131: LGTM.

The yalc workflow documentation now correctly references the public package names (react-on-rails-pro-node-renderer), consistent with the broader PR changes.

react_on_rails_pro/CONTRIBUTING.md (2)

295-301: LGTM.

Prerequisites section correctly updated to reflect public registry authentication (npmjs.org and RubyGems.org), removing private GitHub Packages references. Clear and actionable guidance for contributors.


303-317: LGTM.

Release command section provides clear, concrete examples (dry-run, Verdaccio test, full release) with proper reference to root-level documentation. Aligns with the testing plan outlined in PR objectives.

react_on_rails_pro/docs/node-renderer/js-configuration.md (1)

57-57: LGTM.

Import path updated to reflect the unscoped public package name for consistency with public npm publishing.

react_on_rails_pro/docs/contributors-info/releasing.md (1)

35-40: LGTM.

Package list updated to reflect new public distribution: react-on-rails-pro-node-renderer (NPM) is now listed separately, and Pro gem is correctly listed as RubyGem (removing GitHub Packages reference). Accurate and complete.

react_on_rails_pro/docs/node-renderer/basics.md (2)

36-45: LGTM.

Installation instructions correctly updated to use public package name. Documentation flow is clear and follows standard npm conventions.


45-45: Module import syntax inconsistency across documentation.

Line 45 uses default import: import reactOnRailsProNodeRenderer from 'react-on-rails-pro-node-renderer', but other documentation (e.g., js-configuration.md:57) shows destructured import: import { reactOnRailsProNodeRenderer } from '...'

Verify which is correct and standardize across all docs to avoid user confusion.

react_on_rails_pro/docs/node-renderer/error-reporting-and-tracing.md (1)

17-17: LGTM.

All import paths comprehensively updated to the unscoped public package name across multiple integration examples (Sentry, Honeybadger, BugSnag, Fastify API). Changes are consistent and thorough.

Also applies to: 20-20, 43-43, 52-52, 62-62, 79-79, 120-120, 145-145

react_on_rails_pro/docs/installation.md (7)

3-11: LGTM.

Introduction clearly establishes the public distribution model and license token requirement. Contact information for purchasing is helpful for users.


13-37: LGTM.

Ruby gem installation section is clear and comprehensive, providing both standard gem declaration and direct install options with version guidance. Follows established Ruby patterns.


39-54: LGTM.

License configuration section provides both environment variable (recommended) and initializer approaches, with helpful production guidance. Clear and actionable for users.


70-96: LGTM.

Node package installation section is clear and comprehensive, showing both npm and yarn options with package.json example. Consistent use of public package name.


98-141: Module syntax inconsistency with other documentation.

This section uses CommonJS require/destructuring syntax (line 104: const { reactOnRailsProNodeRenderer } = require(...)), but docs/node-renderer/basics.md:45 uses ES6 default import syntax (import reactOnRailsProNodeRenderer from ...). Both sections describe setting up a custom configuration file for the same package.

Verify the actual package export style and standardize the examples across all documentation to avoid user confusion.


143-175: LGTM.

Node renderer startup and Rails configuration sections are well-structured. package.json script, startup command, and Rails initializer example are clear and correctly reference the configuration options. The password field correctly maps to the example configuration above.


177-195: LGTM.

Configuration options summary is helpful and provides clear bullet-point reference to key settings. Proper links to detailed documentation files.

@ihabadham ihabadham force-pushed the switch-to-public-distribution branch from 581605d to 32ef93d Compare November 5, 2025 21:05
@claude
Copy link

claude bot commented Nov 5, 2025

Code Review - Switch Pro Packages to Public Distribution

I've completed a thorough review of PR #1901. This is a significant architectural change that transitions React on Rails Pro packages from private GitHub Packages distribution to public npmjs.org and RubyGems.org distribution. Overall, the implementation is well-executed and clean, but I have some recommendations and observations.


✅ Strengths

1. Clean Code Changes

  • The removal of publishConfig from package.json is exactly right (line 19-21)
  • Release script refactoring is well-structured and improves readability
  • Consistent package naming change from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer

2. Comprehensive Documentation Updates

  • Installation documentation is significantly improved with clearer, step-by-step instructions
  • Removal of complex GitHub authentication setup will greatly reduce customer friction
  • All references to the old scoped package name have been updated consistently

3. Good Separation of Concerns

  • The PR correctly maintains the distinction between distribution (now public) and authorization (still JWT-based at runtime)
  • Security model remains intact - this is purely a distribution strategy change

🔍 Code Quality & Best Practices

rakelib/release.rake (Lines 210-220)

Good:

  • Clear messaging about what's being published
  • Proper OTP prompts for 2FA
  • Consistent variable naming (node_renderer_name)

Observation:
The release script uses yarn publish for the node-renderer package (line 220) but yarn workspace ... publish for the other packages (lines 202, 208). This is intentional since node-renderer lives in react_on_rails_pro/ directory, but consider adding a comment to explain this difference for future maintainers:

# Note: Using 'yarn publish' here instead of 'yarn workspace' because 
# the node-renderer package.json is in react_on_rails_pro/ root directory
sh_in_dir(pro_gem_root,
          "yarn publish --new-version #{actual_npm_version} --no-git-tag-version #{npm_publish_args}")

Documentation Consistency (installation.md)

Good:

  • The new installation flow is much clearer and follows standard conventions
  • License token configuration is well-documented
  • Both npm and yarn examples provided

Minor suggestions:

  1. Line 102-108: Consider mentioning that the license token should be kept secret and never committed to version control
  2. Line 52: The version constraint "~> 16.1" is good, but consider documenting the versioning strategy (when to use ~> vs exact versions)

⚠️ Potential Issues

1. Breaking Change Documentation

This is a breaking change for existing Pro customers who have their .npmrc configured for GitHub Packages. Consider:

  • Adding a BREAKING CHANGE section to the PR description or CHANGELOG
  • Creating a migration guide for existing customers
  • Documenting what customers need to do:
    • Remove GitHub Package registry configuration from .npmrc
    • Update package references from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer
    • Set license token environment variable

2. Repository URL in package.json (Line 119)

The repository URL still points to:

"url": "git+https://github.com/shakacode-tools/react_on_rails_pro.git"

Since this is now a public package, consider:

  • Should this URL be updated to point to the public shakacode/react_on_rails repository instead?
  • Or should it point to a public-facing Pro repository?
  • The shakacode-tools organization name suggests this is still private

3. Missing Newline at EOF (package.json)

According to the project's CLAUDE.md requirements:

ALWAYS ensure files end with a newline character

The package.json file at line 159 should end with a newline. The diff shows it does have one, so this is OK. ✅


🔒 Security Considerations

Good Security Practices:

  1. ✅ Runtime JWT validation remains unchanged (as documented)
  2. ✅ No sensitive credentials in the codebase
  3. ✅ Clear separation between distribution and authorization

Recommendations:

  1. License Token Security: In the installation docs, add an explicit warning:

    ⚠️ **Security Warning**: Never commit your license token to version control. 
    Always use environment variables or secure secret management systems.
  2. Package Integrity: Consider documenting (if not already done) how customers can verify package integrity when installing from public registries.

  3. Deprecation of Old Package: Consider publishing a final version of @shakacode-tools/react-on-rails-pro-node-renderer that shows a deprecation warning pointing to the new unscoped package.


🧪 Testing Recommendations

The PR description includes a good testing plan. Ensure you also test:

  1. ✅ Fresh installation without GitHub credentials configured
  2. ✅ Upgrade path from old scoped package to new unscoped package
  3. ✅ Runtime license validation still works correctly
  4. ✅ Verdaccio dry-run testing (already mentioned)
  5. Add: Test that old documentation links/references still work or redirect appropriately

📊 Performance Considerations

No performance concerns identified. This is purely a distribution change and doesn't affect runtime behavior.


📝 Minor Observations

  1. CONTRIBUTING.md Changes (Lines 1-13)

    • Good cleanup of GitHub Packages authentication instructions
    • The streamlined prerequisites section is much more approachable
  2. Consistency Across All Docs

    • All 8 documentation files have been updated consistently ✅
    • Package name changes are applied uniformly ✅
  3. Release Script Messages

    • The updated success messages are clear and helpful
    • The Verdaccio testing instructions are comprehensive

🎯 Recommendations Summary

Must Fix:

  1. Verify repository.url in package.json points to the correct public location

Should Consider:

  1. Add breaking change documentation/migration guide for existing customers
  2. Add comment explaining why node-renderer uses different yarn publish command
  3. Add security warning about license token storage in docs
  4. Consider publishing deprecation notice for old scoped package

Nice to Have:

  1. Document versioning strategy (~> vs exact versions)
  2. Add package integrity verification documentation

✅ Overall Assessment

Status:Approved with Minor Recommendations

This is a well-thought-out PR that successfully transitions from private to public distribution while maintaining security through runtime licensing. The code changes are clean, documentation is comprehensive, and the impact is well-understood.

The main concern is ensuring existing customers have a smooth migration path. Once the repository URL is verified and some documentation enhancements are added, this should be good to merge.

Great work on simplifying the customer experience! 🎉


Checklist from CLAUDE.md

  • ✅ Code follows project conventions
  • ⚠️ Need to run bundle exec rubocop before merge (as per CLAUDE.md requirements)
  • ✅ Files end with newlines
  • ✅ Documentation updated appropriately
  • ✅ No obvious security issues
  • ⏭️ Testing plan is comprehensive (pending execution)

Let me know if you'd like me to elaborate on any of these points!

ihabadham and others added 4 commits November 5, 2025 23:11
Fix ESLint errors from CI by updating all remaining references from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer.

Files updated:
- react_on_rails_pro/spec/dummy/client/node-renderer.js (3 requires)
- react_on_rails_pro/packages/node-renderer/src/integrations/api.ts (JSDoc example)
- react_on_rails_pro/spec/dummy/package.json (dependency + yalc link)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Update error message from @shakacode-tools/react-on-rails-pro-node-renderer to react-on-rails-pro-node-renderer in package-scripts.yml

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
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