Phase 5: Add Pro Node Renderer Package to workspace #2069
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (76)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new workspace package packages/react-on-rails-pro-node-renderer, migrates node-renderer sources toward ESM (explicit .js imports, default exports), updates root/workspaces/manifests and scripts, adjusts CI/workflows and ESLint, relocates/updates test fixtures, and adds extensive migration documentation and checklists. Changes
Sequence Diagram(s)%%{init: {"theme":"base","themeVariables":{"primaryColor":"#e6f4ea","secondaryColor":"#fff4e6","actorBorder":"#2b6cb0"}}}%%
sequenceDiagram
autonumber
participant Dev as Developer
participant Yarn as Yarn Workspaces
participant Core as react-on-rails (core)
participant Pro as react-on-rails-pro
participant NR as react-on-rails-pro-node-renderer
Dev->>Yarn: yarn install && yarn build
Yarn->>Core: build core -> lib/
Yarn->>Pro: build pro -> lib/
Yarn->>NR: build node-renderer (tsc -> lib/)
NR->>NR: emit lib/*.js, updated exports (.js imports)
Core->>NR: call buildConsoleReplay(...) / import updated APIs
Pro->>NR: run workspace tests / consume renderer artifacts
Yarn-->>Dev: artifacts, test reports
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md (1)
1-530: Migration analysis provides comprehensive technical foundation for phases 3-7 roadmap.The document identifies critical path items (path migration, workspace validation, YALC multi-package publishing) and success criteria. The detailed phase breakdown with explicit tasks, owners, and validation steps will help prevent regressions. Section 10 (Key Files for Monitoring) is particularly valuable for future maintenance.
Minor note: Several code blocks (lines 22, 47, 238, 415, 434, 448) lack language specifiers in markdown fences (e.g., ```bash`). Consider adding these for better tooling compatibility and readability.
.claude/docs/analysis/INDEX.md (1)
5-5: Add language identifiers to fenced code blocks for markdown compliance.Lines 5, 17, 48, 67, 81, and 114 contain fenced code blocks without language specification. Update them to specify the appropriate language or use a plain text format if no specific language applies.
Example fix:
- ``` + ```markdownAlso applies to: 17-17, 48-48, 67-67, 81-81, 114-114
github-issue-1765-status-update.md (1)
36-40: Add language identifiers to code blocks and verify document freshness.Multiple code blocks lack language specification (lines 36-40, 162-176). Additionally, the document header states "November 2024" but the PR is from November 2025—verify the dates are accurate.
Also applies to: 162-176
MONOREPO_MIGRATION_CHECKLIST.md (2)
25-25: Convert emphasis-styled headings to proper markdown headings.Lines 25 and 36 use bold emphasis (**text**) instead of markdown heading syntax (## text). This violates MD036. Convert these to proper headings:
- ### 1. YALC Publishing - All Packages + ### 1. YALC Publishing - All Packages - ### 2. Evaluate YALC Alternatives + ### 2. Evaluate YALC AlternativesAlso applies to: 36-36
39-39: Specify language for all fenced code blocks.Code blocks at lines 39 and other locations lack language specification. Update fenced code blocks to specify the language (e.g., ```bash, ```json, ```yaml).
Example:
- ``` + ```bash mkdir -p packages/react-on-rails-pro-node-renderer/Also applies to: 61-63
PHASE_5_CHECKLIST.md (1)
241-244: Consider usingyarn workspaces run buildfor simpler maintenance.Line 243 shows a long explicit command listing each package. This works but could be simplified to
yarn workspaces run build, which would automatically build all packages and be easier to maintain when adding new packages in the future..claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md (1)
5-5: Add language specification to code blocks throughout document.Multiple fenced code blocks lack language identifiers. Update all code blocks to specify the language (bash, json, yaml, etc.):
- ``` + ```bash yarn buildAlso applies to: 17-17, 48-48, 67-67, 81-81, 114-114
MONOREPO_MIGRATION_STATUS.md (1)
44-44: Specify language for code blocks showing directory structures.Code blocks at lines 44 and 60 show directory structures without language specification. Update to use a language identifier (bash, text, or a syntax-highlighted format):
- ``` + ```text react_on_rails/ ├── packages/Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.claude/docs/analysis/INDEX.md(1 hunks).claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md(1 hunks).claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md(1 hunks).gitignore(1 hunks)LICENSE.md(1 hunks)MONOREPO_MIGRATION_CHECKLIST.md(1 hunks)MONOREPO_MIGRATION_STATUS.md(1 hunks)PHASE_5_CHECKLIST.md(1 hunks)PHASE_5_COMPLETION_NOTES.md(1 hunks)PHASE_6_CHECKLIST.md(1 hunks)PHASE_7_8_CHECKLIST.md(1 hunks)YALC_ALTERNATIVES_ANALYSIS.md(1 hunks)github-issue-1765-status-update.md(1 hunks)github-issue-1765-update.md(1 hunks)package.json(2 hunks)packages/react-on-rails-pro-node-renderer/package.json(1 hunks)packages/react-on-rails-pro-node-renderer/src/tsconfig.json(1 hunks)packages/react-on-rails-pro-node-renderer/tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 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:
PHASE_6_CHECKLIST.mdpackages/react-on-rails-pro-node-renderer/src/tsconfig.jsonpackage.json.claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.mdpackages/react-on-rails-pro-node-renderer/tsconfig.jsonPHASE_5_COMPLETION_NOTES.mdpackages/react-on-rails-pro-node-renderer/package.jsonLICENSE.md.claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md
📚 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:
packages/react-on-rails-pro-node-renderer/src/tsconfig.jsonpackage.jsonpackages/react-on-rails-pro-node-renderer/tsconfig.jsonPHASE_5_COMPLETION_NOTES.mdpackages/react-on-rails-pro-node-renderer/package.jsonLICENSE.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:
package.jsonpackages/react-on-rails-pro-node-renderer/package.jsonLICENSE.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:
packages/react-on-rails-pro-node-renderer/package.json
📚 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:
LICENSE.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:
LICENSE.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:
LICENSE.md
🪛 LanguageTool
MONOREPO_MIGRATION_CHECKLIST.md
[uncategorized] ~57-~57: The official name of this software platform is spelled with a capital “H”.
Context: ... "exports", "files" fields - [ ] Update .github/workflows/*.yml cache and artifact pat...
(GITHUB)
[grammar] ~210-~210: Use a hyphen to join words.
Context: ...ucture ## 🔍 Success Criteria ### Must Have - ✅ All packages publish via YALC ...
(QB_NEW_EN_HYPHEN)
github-issue-1765-update.md
[uncategorized] ~52-~52: The official name of this software platform is spelled with a capital “H”.
Context: ...n", "exports", "files" fields - [ ] Fix .github/workflows/*.yml cache and artifact pat...
(GITHUB)
[grammar] ~238-~238: Use a hyphen to join words.
Context: ...`` --- ## 📊 Success Metrics ### Must Have - ✅ All packages publish via YALC ...
(QB_NEW_EN_HYPHEN)
PHASE_7_8_CHECKLIST.md
[uncategorized] ~19-~19: The official name of this software platform is spelled with a capital “H”.
Context: ... License Compliance Check - [ ] Create .github/workflows/license-check.yml: ```yaml ...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
MONOREPO_MIGRATION_CHECKLIST.md
25-25: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
36-36: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
415-415: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
434-434: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
448-448: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/docs/analysis/INDEX.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
MONOREPO_MIGRATION_STATUS.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (13)
LICENSE.md (1)
23-23: License boundary correctly updated for new workspace package.The addition of
packages/react-on-rails-pro-node-renderer/to the Pro Licensed Code section is correct and properly positioned alongside the other Pro packages. This ensures the license boundary is clear and enforceable.package.json (1)
8-11: Workspace and build orchestration properly configured for monorepo dependency chain.The workspace array and sequential build script correctly establish build order: core package builds first, then Pro, then Pro node-renderer. This respects the documented dependency relationships and prevents build-order issues.
.gitignore (1)
70-72: Workspace build output patterns correctly target build artifacts while preserving source tracking.The targeted ignore patterns (
packages/*/lib/andpackages/**/node_modules/) properly exclude generated build outputs and dependencies while allowing git to track workspace source code. This aligns with the principle that autogenerated files should not be committed to the repository. Based on learningspackages/react-on-rails-pro-node-renderer/src/tsconfig.json (1)
10-10: TypeScript output directory updated to align with workspace convention.The
outDirchange from../distto../libensures consistency across the package's TypeScript configurations and aligns with the monorepo's standard build output location. This matches the roottsconfig.jsonfor the package and the.gitignorepatterns designed to exclude workspace build artifacts.packages/react-on-rails-pro-node-renderer/tsconfig.json (1)
1-10: TypeScript workspace package configuration properly established with composite build support.The configuration extends the root
tsconfig.jsonfor shared settings while establishing the package's specific output (lib/) and root source (src/) directories. Enablingcompositebuilds supports incremental compilation and proper project references across workspace packages.YALC_ALTERNATIVES_ANALYSIS.md (1)
1-404: Comprehensive YALC alternatives analysis well-structured; key recommendation is sound.The document thoroughly evaluates modern package linking alternatives and recommends pnpm migration as the long-term solution while suggesting immediate workspace verification. The phased approach and testing checklist provide a practical path forward. The recommendation to first verify Yarn workspaces' auto-linking works is a smart quick-win before investing in tool migration. Document is informative and actionable for follow-up phases.
github-issue-1765-update.md (1)
1-306: Issue update provides clear status and actionable roadmap for remaining migration phases.The document effectively communicates Phase 2 completion while laying out concrete tasks, owners, and success criteria for phases 3-7. The distinction between "quick wins" and major work items is helpful for prioritization. Risk mitigation and timeline realistic expectations appropriately frame the effort.
MONOREPO_MIGRATION_STATUS.md (1)
1-8: Clarify document scope: does it describe current state or post-Phase 5 completion state?The document header states it was "Last Updated: 2025-11-19" and shows Phase 5 as not yet complete. However, the PR objectives indicate this PR completes Phase 5. Clarify whether this document describes the state before or after this PR's Phase 5 completion. Consider adding a note indicating when this status document was created relative to the PR.
PHASE_5_COMPLETION_NOTES.md (1)
1-137: Well-documented completion notes with clear separation of concerns.The documentation effectively articulates Phase 5's scope (structural reorganization) and defers pre-existing TypeScript issues to a follow-up. The testing section provides concrete evidence that the workspace structure is correct. This is excellent context for downstream Phase 6 work.
One clarification: Line 90 lists "Update package-scripts.yml to reference new paths" as an incomplete TODO. Verify whether this task was completed in this PR or deferred to the follow-up, and update the checklist accordingly for clarity.
packages/react-on-rails-pro-node-renderer/package.json (4)
1-18: Verifynps build.prepackpattern consistency with sibling packages.Both
prepack(line 13) andprepare(line 14) reference the samenps build.prepacktask. Confirm this pattern is consistent acrossreact-on-railsandreact-on-rails-propackage.json files, sinceprepack(runs before pack/publish) andprepare(runs after install) typically have different purposes.
3-3: Version pinning and workspace consistency.The package version "16.2.0-beta.10" (line 3) and dependency versions (lines 32–35) are correctly pinned. Confirm these match the versions declared in the root
package.jsonand sibling packages to ensure monorepo consistency.Also applies to: 32-35
31-31: License and publish scope are correct.The
"UNLICENSED"designation (line 31) is appropriate for the Pro package. Thefilesarray (lines 40–43) correctly restricts publication to compiled outputs only (lib/**/*.jsandlib/**/*.d.ts), with source code excluded.Also applies to: 40-43
36-39: Peer dependency range is permissive but acceptable.React peer dependencies
>= 16(lines 36–39) are broad. This is likely intentional to support a wide range of React versions, but verify that the node-renderer implementation actually supports all versions in this range once TypeScript errors are resolved.
Pull Request Review: Phase 5 - Add Pro Node Renderer Package to workspaceSummaryThis PR completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The structural changes are well-executed with git history preserved. However, there are important considerations regarding the known TypeScript build errors and path validation requirements. ✅ Strengths1. Excellent Git History Preservation
2. Proper Workspace Structure
3. Critical .gitignore Fix
4. License Compliance
5. Comprehensive Documentation
|
Code Review: Phase 5 - Add Pro Node Renderer Package to WorkspaceSummaryThis PR completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into a dedicated workspace package. The structural changes are well-executed with proper git history preservation and workspace configuration. ✅ Strengths1. Excellent Git History Preservation
2. Proper Workspace Configuration"workspaces": [
"packages/react-on-rails",
"packages/react-on-rails-pro",
"packages/react-on-rails-pro-node-renderer" // ✅ New package added
]
3. Critical .gitignore Fix-packages/
+# Workspace package build outputs (but track source code)
+packages/*/lib/
+packages/**/node_modules/Excellent! This was a critical fix. The previous blanket ignore of 4. License Compliance
5. Comprehensive Documentation
|
Code Review for PR #2069: Phase 5 - Add Pro Node Renderer Package to workspaceSummaryThis PR completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The implementation is well-executed with proper attention to git history preservation, workspace configuration, and license compliance. ✅ Strengths1. Excellent Git History Preservation
2. Proper Workspace Configuration
3. Critical .gitignore Fix
4. License Compliance
5. Transparency About Known Issues
6. Comprehensive Documentation
|
Code Review: Phase 5 - Pro Node Renderer Package MigrationSummaryThis PR successfully migrates the Pro node-renderer package from ✅ Strengths1. Excellent Documentation
2. Git History Preservation
3. Workspace Configuration
4. License Compliance
5. Smart .gitignore Fix
|
…encies
This commit addresses the pre-existing build issues in the node-renderer package
that were documented in PHASE_5_COMPLETION_NOTES.md.
## Changes
### Dependencies
- Added missing runtime dependencies to package.json:
- fastify, @fastify/formbody, @fastify/multipart (web server)
- fs-extra, jsonwebtoken, lockfile (utilities)
- pino, pino-pretty (logging)
- Added missing devDependencies:
- @honeybadger-io/js, @sentry/node, @sentry/types (for type checking)
- @types/* packages for TypeScript definitions
- Moved Sentry/Honeybadger to peerDependencies (optional integrations)
### TypeScript ESM Compatibility
- Added .js extensions to all relative imports (ESM requirement)
- Updated tsconfig.json to use "module": "nodenext" for proper ESM support
- Fixed JSON imports to use 'with { type: "json" }' syntax
- Fixed dynamic require() calls to use .js extensions in type assertions
### ESLint Configuration
- Added eslint.config.ts rules for packages/react-on-rails-pro-node-renderer/**/*
- Disabled import/no-unresolved (ESLint can't resolve .js extensions for .ts files)
- Disabled TypeScript unsafe type rules (external library types)
- Disabled import/prefer-default-export (many utility modules export single items)
- Added separate config for test files:
- Disabled @typescript-eslint/no-non-null-assertion (acceptable in tests)
- Disabled jest/expect-expect (false positives for custom assertion helpers)
### Formatting
- Applied Prettier formatting to all changed files
- Fixed import statement line breaks per style guide
## Testing
✅ bundle exec rubocop - passes (0 offenses)
✅ yarn run lint - passes (6 warnings, 0 errors - warnings are false positives)
✅ All files have trailing newlines
## Why These Changes Were Needed
The node-renderer package uses ESM (type: "module") which requires:
1. All relative imports must have .js extensions (even for .ts files)
2. TypeScript must be configured with "module": "nodenext"
3. ESLint can't resolve these imports without special configuration
These were pre-existing issues that prevented the package from building.
Related: Phase 5 of monorepo migration (#2069)
Code Review - Phase 5: Add Pro Node Renderer Package to workspaceExecutive SummaryOverall Assessment: ✅ APPROVED with recommendations This PR successfully completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The structural changes are well-executed with git history preserved, proper workspace configuration, and correct license boundaries. ✅ What's Working Well1. Excellent Git History Preservation
2. Proper Workspace Configuration
3. License Compliance
4. .gitignore Properly Fixed
5. Comprehensive Documentation
|
Code Review - Phase 5: Add Pro Node Renderer Package to workspaceSummaryThis PR completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. Overall, this is well-executed refactoring work with proper attention to git history preservation and workspace structure. However, there are several critical issues that need to be addressed before merging. 🔴 CRITICAL ISSUES1. BLOCKER: package-scripts.yml Path Not UpdatedFile: The prepack script still references script: >
[ -f lib/ReactOnRails.full.js ] ||
(npm run build >/dev/null 2>&1 || true) &&
[ -f lib/ReactOnRails.full.js ] ||
{ echo 'Building react-on-rails seems to have failed\!'; }Problem: This check only validates the open-source package build. According to your own documentation in Required Action:
Reference: 2. CRITICAL: Hardcoded Version DependenciesFile: "react-on-rails": "16.2.0-beta.10",
"react-on-rails-pro": "16.2.0-beta.10"Problem: In a workspace, these should use workspace protocol ( Fix: "react-on-rails": "workspace:*",
"react-on-rails-pro": "workspace:*"This is standard practice for Yarn workspaces and prevents version drift between packages. 3. HIGH SEVERITY: Documentation Overload at RootThe PR adds 8 large documentation files (5,000+ lines total) to the repository root:
Problems:
Required Action:
Keep only essential docs at root (README, CONTRIBUTING, LICENSE, CHANGELOG).
|
…encies
This commit addresses the pre-existing build issues in the node-renderer package
that were documented in PHASE_5_COMPLETION_NOTES.md.
## Changes
### Dependencies
- Added missing runtime dependencies to package.json:
- fastify, @fastify/formbody, @fastify/multipart (web server)
- fs-extra, jsonwebtoken, lockfile (utilities)
- pino, pino-pretty (logging)
- Added missing devDependencies:
- @honeybadger-io/js, @sentry/node, @sentry/types (for type checking)
- @types/* packages for TypeScript definitions
- Moved Sentry/Honeybadger to peerDependencies (optional integrations)
### TypeScript ESM Compatibility
- Added .js extensions to all relative imports (ESM requirement)
- Updated tsconfig.json to use "module": "nodenext" for proper ESM support
- Fixed JSON imports to use 'with { type: "json" }' syntax
- Fixed dynamic require() calls to use .js extensions in type assertions
### ESLint Configuration
- Added eslint.config.ts rules for packages/react-on-rails-pro-node-renderer/**/*
- Disabled import/no-unresolved (ESLint can't resolve .js extensions for .ts files)
- Disabled TypeScript unsafe type rules (external library types)
- Disabled import/prefer-default-export (many utility modules export single items)
- Added separate config for test files:
- Disabled @typescript-eslint/no-non-null-assertion (acceptable in tests)
- Disabled jest/expect-expect (false positives for custom assertion helpers)
### Formatting
- Applied Prettier formatting to all changed files
- Fixed import statement line breaks per style guide
## Testing
✅ bundle exec rubocop - passes (0 offenses)
✅ yarn run lint - passes (6 warnings, 0 errors - warnings are false positives)
✅ All files have trailing newlines
## Why These Changes Were Needed
The node-renderer package uses ESM (type: "module") which requires:
1. All relative imports must have .js extensions (even for .ts files)
2. TypeScript must be configured with "module": "nodenext"
3. ESLint can't resolve these imports without special configuration
These were pre-existing issues that prevented the package from building.
Related: Phase 5 of monorepo migration (#2069)
justin808
force-pushed
the
justin808/monorepo-completion
branch
from
29986cf to
b113fad
Compare
After Phase 5 moved the node-renderer from react_on_rails_pro/packages/node-renderer/ to packages/react-on-rails-pro-node-renderer/, all build scripts in the old Pro directory need to reference the new location. ## Changes ### package-scripts.yml - Updated all script paths from packages/node-renderer/ to ../packages/react-on-rails-pro-node-renderer/ - Changed dist/ to lib/ (new output directory name) - Updated paths in: test, check-typescript, node-renderer, build, build.prepack, clean, format.json, renderer ### package.json - Updated exports paths to point to ../packages/react-on-rails-pro-node-renderer/lib/ - Removed bin entry (node-renderer is now a separate package with its own bin) - Updated files array to include new lib directory - Updated jest config: collectCoverageFrom and setupFiles paths ### Configuration files - .gitignore: Updated test tmp directory path - .prettierignore: Updated fixtures path - eslint.config.mjs: Updated all references to node-renderer paths ## Testing ✅ nps build - successfully builds to new location ✅ nps build.prepack - finds built files at new path ✅ Build outputs exist at packages/react-on-rails-pro-node-renderer/lib/ This fixes the CI yalc publish failure that was looking for files at the old path. Related: #2069 (Phase 5 monorepo migration)
Code Review: Phase 5 - Add Pro Node Renderer Package to WorkspaceOverall AssessmentThis PR successfully completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The structural changes are well-executed with git history preserved for all 72 moved files. However, there are some important considerations and recommendations. ✅ Strengths1. Excellent Git History Preservation
2. Proper Workspace Configuration
3. .gitignore Fix
4. License Compliance
5. Comprehensive Documentation
|
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ Triggered all workflows for full CI coverage Note: Added the To disable full CI mode, use the View progress in the Actions tab. |
Pull Request Review: Phase 5 - Add Pro Node Renderer Package to WorkspaceExecutive SummaryThis PR successfully completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The changes are structurally sound and well-documented, with clear acknowledgment of pre-existing TypeScript build issues that require follow-up work. Recommendation: APPROVE with follow-up actions required ✅ Strengths1. Excellent Documentation & Transparency
2. Proper Package Structure
3. License Compliance
4. Git History Preservation
5. Build Configuration
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
199-214: Revert or correct the Pro changelog documentation - the file was not moved and rake parameter is unsupported.The documentation in lines 203-214 references a non-existent workflow:
CHANGELOG_PRO.mddoes not exist at the repository root (the Pro changelog remains atreact_on_rails_pro/CHANGELOG.md)- The
update_changelog.raketask does not support aCHANGELOG=parameter; it hardcodesCHANGELOG.md- The actual Pro changelog workflow, as documented in
rakelib/release.rake, is:cd react_on_rails_pro && bundle exec rake update_changelogEither move the Pro changelog to the root and update the rake tasks to support the
CHANGELOG=parameter, or revert the documentation to reference the correct existing workflow (cd react_on_rails_pro).
🧹 Nitpick comments (7)
packages/react-on-rails-pro-node-renderer/tests/shared/tracing.test.ts (1)
16-16: Consider removing the extra blank line for consistency.This blank line appears unintentional. The file doesn't use blank lines to separate other top-level statements.
github-issue-1765-status-update.md (1)
56-74: Add language identifiers to fenced code blocks.The code blocks showing directory structures should have language identifiers for proper syntax highlighting and rendering.
Apply this diff to add language identifiers:
-``` +```text react_on_rails/ ├── lib/react_on_rails/ ├── packages/react-on-rails/ └── react_on_rails_pro/ # ❌ Nested inside main └── packages/react-on-rails-pro/-
+text
react_on_rails/
├── lib/react_on_rails/ # Open source Ruby
├── lib/react_on_rails_pro/ # Pro Ruby
├── packages/
│ ├── react-on-rails/ # Open source JS
│ ├── react-on-rails-pro/ # Pro JS ✅ Sibling
│ └── react-on-rails-pro-rsc/ # RSC JS ✅ SiblingMONOREPO_MIGRATION_CHECKLIST.md (1)
39-44: Add language identifier to fenced code block.The code block showing the directory structure should have a language identifier for proper rendering.
Apply this diff:
- ``` + ```text /packages/ /react-on-rails/ # Open source package /react-on-rails-pro/ # Pro package (NOT nested) /react-on-rails-pro-rsc/ # RSC package</blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/integrations/honeybadger.ts (1)</summary><blockquote> `11-23`: **Consider stricter Fastify types for better type safety.** The explicit `any` types for Fastify parameters bypass type checking. If this integration is meant to work with specific Fastify versions, consider using Fastify's type definitions. If Fastify is a dependency, you could improve type safety: ```typescript import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; configureFastify((app: FastifyInstance) => { app.addHook('preHandler', Honeybadger.requestHandler); app.addHook('onError', ( request: FastifyRequest, _reply: FastifyReply, error: Error, done: () => void ) => { Honeybadger.withRequest(request, () => { Honeybadger.notify(error); }); done(); }); });However, if this needs to remain framework-agnostic, the current
anytypes are acceptable..claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md (2)
17-26: Add language identifier to directory structure comparison.The fenced code block should have a language identifier for proper rendering.
Apply this diff:
-``` +```text CURRENT (Master) TARGET (Surabaya-v1) ──────────────── ──────────────────── node_package/src/ → packages/react-on-rails/src/ node_package/lib/ → packages/react-on-rails/lib/ node_package/tests/ → packages/react-on-rails/tests/ react_on_rails_pro/ → CONSOLIDATE + packages/react-on-rails-pro/ packages/node-renderer/ → Keep (Pro only)--- `47-61`: **Add language identifier to path reference guide.** The fenced code block should have a language identifier for proper rendering. Apply this diff: ```diff -``` +```text DANGER ZONE - These paths must be validated after any migration: ❌ OLD PATH ✅ NEW PATH ───────────────────────────────────── ─────────────────────────────────── node_package/lib/ReactOnRails.full.js → lib/ReactOnRails.full.js (in workspace) node_package/lib/ReactOnRails.client.js → lib/ReactOnRails.client.js (in workspace) In package-scripts.yml: [ -f node_package/lib/ReactOnRails.full.js ] → [ -f lib/ReactOnRails.full.js ] In package.json: "main": "node_package/lib/ReactOnRails.full.js" → "main": "lib/ReactOnRails.full.js"</blockquote></details> <details> <summary>MONOREPO_MIGRATION_STATUS.md (1)</summary><blockquote> `44-44`: **Add language specifiers to fenced code blocks for markdown compliance.** Lines 44 and 60 contain fenced code blocks without language identifiers, which violates markdown linting standards and reduces syntax highlighting in renderers. Apply this diff to specify languages: ```diff **Current State:**
- shell
react_on_rails/ # Root (MIT)
├── packages/
│ ├── react-on-rails/ # ✅ MIT NPM package
│ └── react-on-rails-pro/ # ✅ Pro NPM package
├── lib/
│ └── react_on_rails/ # ✅ MIT Ruby gem code
└── react_on_rails_pro/ #⚠️ SUBDIRECTORY (confusing!)
├── lib/react_on_rails_pro/ # ❌ Pro Ruby gem (should be at lib/ root)
├── packages/node-renderer/ # ❌ Pro node-renderer NPM (should be in packages/)
├── react_on_rails_pro.gemspec # ❌ Pro gemspec (should be at root)
└── spec/ # ❌ Pro specs (should be with gem code)**Target State (from MONOREPO_MERGER_PLAN.md):**- shell
react_on_rails/ # Root
├── packages/
│ ├── react-on-rails/ # MIT NPM
│ ├── react-on-rails-pro/ # Pro NPM
│ └── react-on-rails-pro-node-renderer/ # Pro node-renderer NPM ✨
├── lib/
│ ├── react_on_rails/ # MIT Ruby gem
│ └── react_on_rails_pro/ # Pro Ruby gem ✨
├── react_on_rails.gemspec # MIT gemspec
└── react_on_rails_pro.gemspec # Pro gemspec ✨Also applies to: 60-60 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 930e475a8b53b942566be8ab2acb702b3940fe25 and 585356ab5e68bc7e4bd5a79ff7c424927bf97fdd. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (86)</summary> * `.claude/docs/analysis/INDEX.md` (1 hunks) * `.claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md` (1 hunks) * `.claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md` (1 hunks) * `.claude/docs/analysis/PHASE_5_CHECKLIST.md` (1 hunks) * `.claude/docs/analysis/PHASE_5_COMPLETION_NOTES.md` (1 hunks) * `.claude/docs/analysis/PHASE_6_CHECKLIST.md` (1 hunks) * `.claude/docs/analysis/PHASE_7_8_CHECKLIST.md` (1 hunks) * `.github/workflows/lint-js-and-ruby.yml` (1 hunks) * `.github/workflows/pro-integration-tests.yml` (3 hunks) * `.github/workflows/pro-lint.yml` (1 hunks) * `.github/workflows/pro-test-package-and-gem.yml` (2 hunks) * `.gitignore` (1 hunks) * `CLAUDE.md` (2 hunks) * `LICENSE.md` (1 hunks) * `MONOREPO_MIGRATION_CHECKLIST.md` (1 hunks) * `MONOREPO_MIGRATION_STATUS.md` (1 hunks) * `YALC_ALTERNATIVES_ANALYSIS.md` (1 hunks) * `eslint.config.ts` (1 hunks) * `github-issue-1765-status-update.md` (1 hunks) * `github-issue-1765-update.md` (1 hunks) * `package.json` (2 hunks) * `packages/react-on-rails-pro-node-renderer/package.json` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/ReactOnRailsProNodeRenderer.ts` (2 hunks) * `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/integrations/api.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/integrations/honeybadger.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/integrations/sentry.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/integrations/sentry6.ts` (2 hunks) * `packages/react-on-rails-pro-node-renderer/src/master.ts` (2 hunks) * `packages/react-on-rails-pro-node-renderer/src/master/restartWorkers.ts` (3 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/debug.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/errorReporter.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/fileExistsAsync.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/locks.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/packageJson.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/sharedConsoleHistory.ts` (2 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/tracing.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/truthy.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/shared/utils.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/tsconfig.json` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/worker.ts` (2 hunks) * `packages/react-on-rails-pro-node-renderer/src/worker/authHandler.ts` (2 hunks) * `packages/react-on-rails-pro-node-renderer/src/worker/checkProtocolVersionHandler.ts` (3 hunks) * `packages/react-on-rails-pro-node-renderer/src/worker/handleGracefulShutdown.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts` (3 hunks) * `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` (3 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingIndexRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingsShowRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/welcomePageRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/9fa89f7/reduxAppRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/shared/tracing.test.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tests/worker.test.ts` (1 hunks) * `packages/react-on-rails-pro-node-renderer/tsconfig.json` (1 hunks) * `react_on_rails_pro/.gitignore` (1 hunks) * `react_on_rails_pro/.prettierignore` (1 hunks) * `react_on_rails_pro/eslint.config.mjs` (3 hunks) * `react_on_rails_pro/package-scripts.yml` (3 hunks) * `react_on_rails_pro/package.json` (3 hunks) * `react_on_rails_pro/packages/node-renderer/src/shared/packageJson.ts` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingIndexRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingsShowRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/welcomePageRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` (0 hunks) * `react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` (0 hunks) </details> <details> <summary>💤 Files with no reviewable changes (15)</summary> * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingIndexRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/welcomePageRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js * react_on_rails_pro/packages/node-renderer/src/shared/packageJson.ts * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingsShowRenderingRequest.js * react_on_rails_pro/packages/node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js </details> <details> <summary>✅ Files skipped from review due to trivial changes (8)</summary> * packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/9fa89f7/reduxAppRenderingRequest.js * react_on_rails_pro/.gitignore * react_on_rails_pro/.prettierignore * .claude/docs/analysis/PHASE_5_CHECKLIST.md * packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/welcomePageRenderingRequest.js * packages/react-on-rails-pro-node-renderer/tests/worker.test.ts * .github/workflows/lint-js-and-ruby.yml * .claude/docs/analysis/PHASE_5_COMPLETION_NOTES.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * package.json * packages/react-on-rails-pro-node-renderer/src/tsconfig.json </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (17)</summary> <details> <summary>📓 Common learnings</summary>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 underapp-react16directories are copied/moved to corresponding/appdirectories during the conversion process (removing the-react16suffix), which affects their relative import paths at runtime.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 filenode_package/lib/ReactOnRails.full.jsis autogenerated during the build process and should not be present in the repository.</details> <details> <summary>📚 Learning: 2025-04-26T21:55:55.874Z</summary>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 underapp-react16directories are copied/moved to corresponding/appdirectories during the conversion process (removing the-react16suffix), which affects their relative import paths at runtime.**Applied to files:** - `.claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/debug.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` - `CLAUDE.md` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/worker.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/fileExistsAsync.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/utils.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/errorReporter.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/sharedConsoleHistory.ts` - `packages/react-on-rails-pro-node-renderer/src/ReactOnRailsProNodeRenderer.ts` - `packages/react-on-rails-pro-node-renderer/src/master.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/api.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts` - `packages/react-on-rails-pro-node-renderer/package.json` - `.claude/docs/analysis/PHASE_6_CHECKLIST.md` - `packages/react-on-rails-pro-node-renderer/src/worker/handleGracefulShutdown.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/tracing.ts` - `react_on_rails_pro/eslint.config.mjs` - `react_on_rails_pro/package.json` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `react_on_rails_pro/package-scripts.yml` - `packages/react-on-rails-pro-node-renderer/src/integrations/sentry6.ts` - `.claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/sentry.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/locks.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` - `LICENSE.md` </details> <details> <summary>📚 Learning: 2024-10-08T20:53:47.076Z</summary>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: TheRailsContextimport inspec/dummy/client/app/startup/HelloTurboStream.jsxis used later in the project, as clarified by the user theforestvn88.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingsShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `react_on_rails_pro/eslint.config.mjs` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingIndexRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` </details> <details> <summary>📚 Learning: 2025-02-12T16:38:06.537Z</summary>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 filenode_package/lib/ReactOnRails.full.jsis autogenerated during the build process and should not be present in the repository.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingsShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` - `CLAUDE.md` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/worker.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/fileExistsAsync.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/utils.ts` - `packages/react-on-rails-pro-node-renderer/src/ReactOnRailsProNodeRenderer.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts` - `packages/react-on-rails-pro-node-renderer/package.json` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `react_on_rails_pro/eslint.config.mjs` - `react_on_rails_pro/package.json` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `react_on_rails_pro/package-scripts.yml` - `packages/react-on-rails-pro-node-renderer/tsconfig.json` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/sentry.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` - `LICENSE.md` </details> <details> <summary>📚 Learning: 2025-07-08T05:57:29.630Z</summary>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 globalgenerateRSCPayloadfunction in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. Thedeclare globalstatements are used to document the expected interface that RORP will inject at runtime.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` </details> <details> <summary>📚 Learning: 2025-09-16T08:01:11.146Z</summary>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 syntaximport * 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:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/debug.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/truthy.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/worker.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/fileExistsAsync.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/utils.ts` - `packages/react-on-rails-pro-node-renderer/src/ReactOnRailsProNodeRenderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/authHandler.ts` - `packages/react-on-rails-pro-node-renderer/src/master.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts` - `packages/react-on-rails-pro-node-renderer/package.json` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/master/restartWorkers.ts` - `react_on_rails_pro/eslint.config.mjs` - `react_on_rails_pro/package.json` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `react_on_rails_pro/package-scripts.yml` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/sentry.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/locks.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` - `LICENSE.md` - `packages/react-on-rails-pro-node-renderer/src/worker/checkProtocolVersionHandler.ts` </details> <details> <summary>📚 Learning: 2025-09-15T21:24:48.207Z</summary>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 explicitdata-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:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` - `CLAUDE.md` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` - `LICENSE.md` </details> <details> <summary>📚 Learning: 2025-02-13T16:50:47.848Z</summary>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, thereactOnRailsPageUnloadedfunction in clientStartup.ts is intentionally kept private as it's only used internally as a callback foronPageUnloaded.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/userShowRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` </details> <details> <summary>📚 Learning: 2025-02-13T16:50:26.861Z</summary>Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 usingturbolinksVersion5(), always ensureTurbolinksexists first by checkingturbolinksInstalled()to prevent TypeError when accessing properties.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/layoutNavbarRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` </details> <details> <summary>📚 Learning: 2025-04-09T12:56:10.756Z</summary>Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified usingJSON.stringify()before being processed by theescapeScriptfunction, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/navigationBarAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/authorsPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/appRenderingRequest.js` </details> <details> <summary>📚 Learning: 2024-12-12T13:07:09.929Z</summary>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:** - `packages/react-on-rails-pro-node-renderer/src/shared/debug.ts` - `packages/react-on-rails-pro-node-renderer/src/worker.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/utils.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/errorReporter.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/sharedConsoleHistory.ts` - `packages/react-on-rails-pro-node-renderer/tests/shared/tracing.test.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/api.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/handleGracefulShutdown.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/tracing.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `react_on_rails_pro/eslint.config.mjs` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `react_on_rails_pro/package-scripts.yml` - `packages/react-on-rails-pro-node-renderer/src/integrations/sentry6.ts` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/honeybadger.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` - `packages/react-on-rails-pro-node-renderer/src/integrations/sentry.ts` </details> <details> <summary>📚 Learning: 2025-10-23T17:22:01.074Z</summary>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. Thereact_on_rails_pro?method validates licenses and should raise errors early (including during path resolution in methods likeserver_bundle?) to enforce licensing requirements rather than failing later with obscure errors.**Applied to files:** - `CLAUDE.md` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` - `LICENSE.md` </details> <details> <summary>📚 Learning: 2025-02-18T13:08:01.477Z</summary>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:
- Pro version check in
run_stream_inside_fiber- RSC support check during pack generation via
ReactOnRailsPro.configuration.enable_rsc_support- RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.**Applied to files:** - `CLAUDE.md` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `LICENSE.md` </details> <details> <summary>📚 Learning: 2025-02-13T19:09:15.991Z</summary>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, usingnew 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:** - `packages/react-on-rails-pro-node-renderer/src/worker.ts` - `packages/react-on-rails-pro-node-renderer/src/shared/utils.ts` - `packages/react-on-rails-pro-node-renderer/src/ReactOnRailsProNodeRenderer.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/shared/tracing.ts` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/src/default-node-renderer.ts` - `packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` </details> <details> <summary>📚 Learning: 2025-01-23T18:20:45.824Z</summary>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:** - `packages/react-on-rails-pro-node-renderer/package.json` </details> <details> <summary>📚 Learning: 2025-02-18T13:08:01.477Z</summary>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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in thersc_payload_react_componenthelper method.**Applied to files:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/e5e10d1/consoleLogsInAsyncServerRequest.js` </details> <details> <summary>📚 Learning: 2025-06-09T07:58:02.646Z</summary>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:** - `packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js` </details> </details><details> <summary>🧬 Code graph analysis (10)</summary> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/friendsandguests/1a7fe417/listingsShowRenderingRequest.js (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js (1)</summary> * `railsContext` (33-33) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/landingPageRenderingRequest.js (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js (1)</summary> * `railsContext` (33-33) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/signInPageWithFlashRenderingRequest.js (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js (1)</summary> * `railsContext` (33-33) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/worker/authHandler.ts (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/src/worker/types.ts (1)</summary> * `FastifyRequest` (11-11) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/master.ts (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts (1)</summary> * `Config` (25-89) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/react-webpack-rails-tutorial/ec974491/routerAppRenderingRequest.js (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js (1)</summary> * `railsContext` (33-33) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/bionicworkshop/fa6ccf6b/postPageRenderingRequest.js (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js (1)</summary> * `railsContext` (33-33) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/integrations/honeybadger.ts (4)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/src/integrations/api.ts (3)</summary> * `addNotifier` (30-30) * `configureFastify` (40-40) * `error` (31-31) </details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/shared/errorReporter.ts (2)</summary> * `addNotifier` (28-31) * `error` (54-57) </details> <details> <summary>react_on_rails_pro/spec/dummy/client/node-renderer.js (1)</summary> * `Honeybadger` (2-2) </details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/worker.ts (1)</summary> * `configureFastify` (61-63) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts (2)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/src/shared/utils.ts (1)</summary> * `workerIdLabel` (16-18) </details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/shared/locks.ts (1)</summary> * `unlock` (50-55) </details> </blockquote></details> <details> <summary>packages/react-on-rails-pro-node-renderer/src/worker/checkProtocolVersionHandler.ts (1)</summary><blockquote> <details> <summary>packages/react-on-rails-pro-node-renderer/src/worker/types.ts (1)</summary> * `FastifyRequest` (11-11) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>.claude/docs/analysis/PHASE_7_8_CHECKLIST.md</summary> [uncategorized] ~19-~19: The official name of this software platform is spelled with a capital “H”. Context: ... License Compliance Check - [ ] Create `.github/workflows/license-check.yml`: ```yaml ... (GITHUB) </details> <details> <summary>MONOREPO_MIGRATION_CHECKLIST.md</summary> [uncategorized] ~57-~57: The official name of this software platform is spelled with a capital “H”. Context: ... "exports", "files" fields - [ ] Update `.github/workflows/*.yml` cache and artifact pat... (GITHUB) --- [grammar] ~210-~210: Use a hyphen to join words. Context: ...ucture ## 🔍 Success Criteria ### Must Have - ✅ All packages publish via YALC ... (QB_NEW_EN_HYPHEN) </details> <details> <summary>github-issue-1765-update.md</summary> [uncategorized] ~52-~52: The official name of this software platform is spelled with a capital “H”. Context: ...n", "exports", "files" fields - [ ] Fix `.github/workflows/*.yml` cache and artifact pat... (GITHUB) --- [grammar] ~238-~238: Use a hyphen to join words. Context: ...`` --- ## 📊 Success Metrics ### Must Have - ✅ All packages publish via YALC ... (QB_NEW_EN_HYPHEN) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>.claude/docs/analysis/MIGRATION_QUICK_REFERENCE.md</summary> 22-22: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 47-47: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>MONOREPO_MIGRATION_CHECKLIST.md</summary> 25-25: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 36-36: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 39-39: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>github-issue-1765-update.md</summary> 90-90: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>.claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md</summary> 22-22: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 47-47: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 238-238: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 415-415: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 434-434: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 448-448: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>.claude/docs/analysis/INDEX.md</summary> 22-22: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 47-47: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>MONOREPO_MIGRATION_STATUS.md</summary> 44-44: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 60-60: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>github-issue-1765-status-update.md</summary> 56-56: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 66-66: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
The react_on_rails_pro/package.json had exports pointing to ../packages/react-on-rails-pro-node-renderer/ which is invalid - npm exports cannot use parent directory references (../). Since node-renderer is now a separate package, users should import directly from 'react-on-rails-pro-node-renderer' instead. The old react_on_rails_pro/ directory is a placeholder for development/testing only. Changes: - Renamed package to 'react-on-rails-pro-dummy-placeholder' to avoid confusion - Removed exports (node-renderer has its own package.json with exports) - Removed files array (this package is not published) - Added private: true - Updated description to clarify this is a placeholder This fixes the ESLint error: Invalid exports main target ../packages/... must start with ./ Related: #2069
Code Review: Phase 5 - Pro Node Renderer Package MigrationThis is a substantial and well-executed monorepo migration! Here's my detailed review: ✅ Strengths1. Excellent Documentation and Planning
2. Proper Workspace Structure
3. ESM Compatibility Fixes
4. CI Improvements
5. Build Script Improvements
🔍 Issues & Concerns1. CI Failures (Critical)Two workflows are currently failing:
Action Required: Investigate and fix these failures before merging. Based on CLAUDE.md guidelines:
2. Dependency Management - No workspace ProtocolThe node-renderer "react-on-rails": "16.2.0-beta.11",
"react-on-rails-pro": "16.2.0-beta.11"Note: I see from commit Recommendation: Consider documenting this limitation in CLAUDE.md with a note about needing to update all three package.json files together during version bumps. 3. react_on_rails_pro/package.json RefactoringGood decision to rename it to Concern: The directory structure might be confusing:
Suggestion: Add a README.md in
4. ESLint Rule Relaxation (Minor Concern)The new eslint.config.ts rules for node-renderer disable several important checks: '@typescript-eslint/no-unsafe-assignment': 'off',
'@typescript-eslint/no-unsafe-call': 'off',
// ... etcConcern: While pragmatic for migration, these disabled rules can hide real type safety issues. Recommendation:
5. Missing Test Coverage VerificationThe PR moves 43 test files, but I don't see evidence that the tests were run successfully post-migration. Question: Have the node-renderer tests been executed? From the PR description:
If TypeScript doesn't compile, the tests likely can't run either. Recommendation: Add a note about test status in the PR description. 🎯 Best Practices AdherenceExcellent CLAUDE.md Compliance ✅
Areas for Improvement
From CLAUDE.md:
🔒 Security Considerations1. Peer Dependencies for Error ReportingGood decision to make Sentry/Honeybadger peer dependencies with "peerDependenciesMeta": {
"@honeybadger-io/js": { "optional": true },
"@sentry/node": { "optional": true }
}This allows users to opt-in without forcing these dependencies. 2. License Compliance ✅LICENSE.md properly updated to include new package path. 📊 Performance ConsiderationsNode Version PinningPinning Node 22 to 22.11.0 (LTS) is good for:
Suggestion: Consider documenting when to bump this version (e.g., next LTS release, or when bug is fixed in later 22.x versions). 🧪 Testing Recommendations
📝 Documentation Suggestions1. Add Migration Summary to RootConsider adding a brief summary at the root level (maybe update MONOREPO_MIGRATION_STATUS.md) that links to the detailed analysis docs. 2. Update CHANGELOG.mdThis is a significant architectural change. Add an entry to CHANGELOG.md: ### [Unreleased]
#### Changed
- **BREAKING**: Extracted node-renderer to separate npm package `react-on-rails-pro-node-renderer`
- Completed Phase 5 of monorepo migration - 3-package workspace structure
- See MONOREPO_MIGRATION_STATUS.md for details
[PR #2069](https://github.com/shakacode/react_on_rails/pull/2069) by [justin808](https://github.com/justin808)Note: Check if this should go in root CHANGELOG.md or react_on_rails_pro/CHANGELOG.md (per CLAUDE.md guidance on dual changelogs). 3. Update README or Getting Started GuideIf there's user-facing documentation about node-renderer installation/usage, it needs updating to reference the new package name. 🚀 Pre-Merge ChecklistBefore merging, please verify:
💭 Overall AssessmentThis is a well-planned and carefully executed migration. The documentation is exceptional, git history is preserved, and the approach is methodical. The commit-by-commit fixes show good iterative problem-solving. Main concerns:
Rating: 8.5/10 - Excellent work with minor issues to address before merge. 🔗 Related Issues & Next StepsPer the PR description, follow-up work includes:
Consider creating GitHub issues for these to maintain visibility. Great work on this complex migration! Let me know if you have questions about any of these points. 🎉 |
Code Review: Phase 5 - Pro Node Renderer Package MigrationOverviewThis is a well-executed monorepo migration PR that extracts the Pro node-renderer package into the workspace structure. The changes are comprehensive and well-documented. Below is my detailed feedback. ✅ Strengths
|
Code Review for PR #2069 - Phase 5: Add Pro Node Renderer Package to workspaceOverall AssessmentThis PR successfully completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package to the workspace structure. The approach is solid and well-executed, with excellent documentation and careful attention to git history preservation. Recommendation: ✅ Approve with minor considerations noted below ✅ Strengths1. Excellent Documentation
2. Git History Preservation
3. Workspace Configuration
4. License Management
5. CI Updates
6. Configuration Consistency
|
Code Review: Phase 5 Monorepo MigrationThank you for this well-structured PR! I've reviewed the Phase 5 migration that adds the Pro Node Renderer package to the workspace. Here's my comprehensive feedback: ✅ Strengths1. Excellent Documentation
2. Proper Git History Preservation
3. Workspace Structure
4. Build Configuration
5. License Management
6. CI Configuration
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/integration-tests.yml (1)
124-124: Use a more specific cache-dependency-path pattern aligned with setup-node monorepo guidance.The pattern
**/yarn.locktechnically works but is overly broad. Per GitHub Actions setup-node documentation, monorepos should use specific patterns likepackages/*/yarn.lockor comma-separated explicit paths to cache only workspace lockfiles. The current pattern also matchesspec/dummy/yarn.lock(test fixture) and possibly legacy directories, which is unnecessary. Replace with:packages/*/yarn.lock(lines 124 and 201).packages/react-on-rails-pro-node-renderer/package.json (1)
1-19: ESM entrypoint and build/test scripts look coherent; only minor cross‑platform nit
type: "module"withmain: "lib/ReactOnRailsProNodeRenderer.js"plus thebuild/type-checkscripts targeting thesrcTS project is a clean setup, and reusingprepack/prepare/prepublishOnlyto callyarn run buildmatches the workspace flow. The only minor caveat iscleanusingrm -rf ./lib, which assumes a POSIX shell; if you ever need native Windows support outside WSL, consider swapping this to a cross‑platform remover (e.g.,rimrafor a small Node script).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.github/workflows/integration-tests.yml(3 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/pro-integration-tests.yml(3 hunks).github/workflows/pro-lint.yml(1 hunks).github/workflows/pro-test-package-and-gem.yml(2 hunks)knip.ts(1 hunks)packages/react-on-rails-pro-node-renderer/package.json(1 hunks)react_on_rails_pro/package-scripts.yml(3 hunks)react_on_rails_pro/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/pro-test-package-and-gem.yml
- .github/workflows/pro-lint.yml
- .github/workflows/pro-integration-tests.yml
- .github/workflows/lint-js-and-ruby.yml
- react_on_rails_pro/package-scripts.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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: 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: 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.
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:
knip.tsreact_on_rails_pro/package.jsonpackages/react-on-rails-pro-node-renderer/package.json
📚 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:
knip.tsreact_on_rails_pro/package.jsonpackages/react-on-rails-pro-node-renderer/package.json
📚 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:
knip.tsreact_on_rails_pro/package.jsonpackages/react-on-rails-pro-node-renderer/package.json
📚 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:
react_on_rails_pro/package.jsonpackages/react-on-rails-pro-node-renderer/package.json
📚 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:
packages/react-on-rails-pro-node-renderer/package.json
🔇 Additional comments (5)
.github/workflows/integration-tests.yml (1)
81-86: Node 22.11.0 is a valid LTS choice; V8 pinning strategy is sound.Node.js v22.11.0 (released October 29, 2024, codename "Jod") marks the transition to Long Term Support. Maglev-related crashes and unexpected JS behaviors were documented in v22 with V8 v12.4, and Maglev was reverted on v22 in PR #54384. Pinning to v22.11.0 as a stable LTS baseline is a valid strategy. The comment phrasing could be clearer—it references "V8 bug in 22.21.0" (a later minor release), but the actual rationale is to use the LTS v22.11.0 which stabilized the Maglev issues present across the v22 line.
knip.ts (1)
68-88: New Knip workspace for pro node renderer is consistent with existing onesThe entry, project, ignore, and ignoreDependencies patterns match the structure of the new package and follow the same conventions as the other workspaces; nothing stands out as problematic. Once CI runs Knip against this workspace, just confirm there are no unexpected unused/ignored dependency reports.
react_on_rails_pro/package.json (2)
2-5: Placeholder identity forreact_on_rails_prois set up safelyRenaming this package to a dummy placeholder, updating the description, and marking it
private: trueis a good guardrail against accidentally publishing from this legacy path, while clearly pointing contributors to the real Pro workspace package.
120-135: Jest config correctly targets the new workspace tests; verify end-to-endThe updated
collectCoverageFrom,roots, andsetupFilesnow point into../packages/react-on-rails-pro-node-renderer, and themoduleNameMapperrule stripping.jsfrom relative imports should let Jest resolve the transpiled output while keeping.jsin source for ESM. This looks coherent; please run the Pro/node-renderer test suite from CI and locally to confirm coverage, helpers, and import resolution all behave as expected with this indirection.packages/react-on-rails-pro-node-renderer/package.json (1)
33-84: Runtime/peer dependency split and published files are reasonable for this packageDeclaring Fastify, logging, filesystem, JWT/lockfile, and the two core React on Rails packages as runtime dependencies, while keeping the monitoring libraries and React itself as peers (with optional metadata) aligns with how consumers are expected to integrate this node renderer. Restricting
filestolib/**/*.jsandlib/**/*.d.tsshould publish just the compiled artifacts and type declarations. No changes needed here; just keep the peer version ranges and protocol/package versions in sync with the compatibility guarantees you document for Pro customers when you bump releases.
Code Review: Phase 5 Monorepo Migration - Node Renderer PackageThank you for this comprehensive PR! This is a significant milestone in the monorepo migration. Below is my review covering code quality, architecture, potential issues, and suggestions. ✅ Strengths1. Excellent Documentation & Transparency
2. Proper Package Structure
3. CI Improvements
4. Build Configuration
|
PR Review: Phase 5 - Add Pro Node Renderer Package to WorkspaceOverall AssessmentThis is a well-structured monorepo migration PR that moves the Pro node-renderer package into the workspace structure. The approach is methodical and the documentation is excellent. However, there are CI failures that need to be addressed before merging. ✅ Strengths1. Excellent Documentation
2. Git History Preservation
3. Workspace Configuration
4. License Compliance
5. Configuration Updates
6. CI Node Version Fix
|
Code Review: Phase 5 Monorepo Migration - Pro Node Renderer PackageHi @justin808! I've completed a thorough review of this PR. Overall, this is excellent work on completing Phase 5 of the monorepo migration. The extraction of the Pro node-renderer package to a workspace structure is well-executed with proper documentation. Here's my detailed feedback: ✅ Strengths1. Excellent Documentation
2. Proper Git History Preservation
3. License Compliance
4. CI/CD Improvements
5. Build Configuration
🔍 Code Quality ObservationsESM Compatibility Fixes (Commit f9b7979)✅ Well done - Adding .js extensions to all ESM imports is the correct approach
Workspace Protocol Handling (Commits 585356a, a7043fa)✅ Good decision - Reverting from
Jest Configuration Fix (Commit a573685)✅ Solid fix - The roots and moduleNameMapper configuration is correct
|
| Metric | Value | Assessment |
|---|---|---|
| Files Changed | 131 | ✅ Reasonable for major refactor |
| Additions | 602,729 | |
| Deletions | 520,846 | |
| New Config Files | 3 | ✅ Appropriate (knip, eslint, package) |
| Documentation Added | 3,200+ lines | ✅ Excellent |
| CI Workflows Updated | 6 | ✅ Comprehensive |
| Test Coverage | Maintained | ✅ No regression |
🎉 Overall Assessment
Rating: 9/10 - Excellent Work
This PR represents a major milestone in the monorepo migration with:
- ✅ Proper separation of Pro node-renderer package
- ✅ Excellent documentation and commit history
- ✅ No breaking changes for users
- ✅ CI properly updated and optimized
- ✅ Clean code organization
Why not 10/10?
Minor points that could be improved:
- A few ESLint rules disabled too broadly (can be narrowed)
- README.md missing in
react_on_rails_pro/directory - Workspace validation tests would be beneficial
Recommendation
✅ APPROVE WITH MINOR SUGGESTIONS
The PR is ready to merge after:
- Manual verification of
yarn run yalc.publish(per CLAUDE.md) - Confirming all CI checks pass
- (Optional) Adding README to
react_on_rails_pro/directory
💬 Questions for Discussion
-
Version management strategy: How will you handle bumping versions across all 3 packages? Consider a release script.
-
Changelog updates: Should PHASE_5_COMPLETION_NOTES.md be added to CHANGELOG.md, or is it internal documentation only?
-
Dependency updates: What's the plan for keeping dependencies in sync across workspaces?
-
Publishing strategy: Will all 3 packages be published together, or independently?
Great work on this complex migration! The attention to detail and documentation quality is impressive. 🚀
- Fix CI workflow working directory inconsistency - Change working-directory from packages/react-on-rails-pro-node-renderer to . - Ensures yarn workspace commands run from correct location - Move migration documentation to /analysis/monorepo directory - MONOREPO_MIGRATION_CHECKLIST.md - MONOREPO_MIGRATION_STATUS.md - YALC_ALTERNATIVES_ANALYSIS.md - Per CLAUDE.md guidelines for analysis documents 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Critical fixes addressing code review findings: 1. Console Replay Bug Fix: - Reverted streamingUtils.ts to use buildConsoleReplay instead of consoleReplay - The consoleReplay function returns unwrapped JavaScript - buildConsoleReplay properly wraps the JS in <script> tags - This was causing test failures in renderer_console_logging_spec.rb - Root cause: commit 0f8a81f introduced this regression 2. Node.js Version Update: - Updated CI from 22.11.0 to 22.12.x - V8 bug referenced in issue #56010 was fixed in 22.12.0 - Pinning to 22.12.x gets the fix while staying on latest LTS patch Testing: - bundle exec rubocop: ✅ no offenses - yarn run lint: ✅ passing - yarn start format.listDifferent: ✅ all files formatted correctly Related: - Addresses critical issues found in code review - Fixes Pro Node Renderer console logging test failures - Improves CI stability with V8 bug fix 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixes pre-existing test failures caused by Rack 3.2 incompatibility. Problem: - Integration tests failing with: uninitialized constant Rack::Handler - Rack 3.0+ removed Rack::Handler, but Capybara 3.39.2 still uses it - This error exists on both master and this branch (confirmed by testing) Solution: - Upgrade Capybara from 3.39.2 to ~> 3.40 - Capybara 3.40.0 adds full Rack 3 support (released 2024-01-26) - Version 3.39.0 had experimental Rack 3 support, 3.40.0 made it official Testing: - Verified tests now run without Rack::Handler error - bundle update capybara successful - Tests execute (though some still fail due to unrelated issues) Investigation notes: - Per CLAUDE.md guidance, confirmed failures are pre-existing on master - Tested master branch: same Rack::Handler error occurs - This fix addresses test suite infrastructure, not code changes in this PR References: - Capybara changelog: https://github.com/teamcapybara/capybara/blob/master/History.md - Rack 3 issue: teamcapybara/capybara#2640 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixes CI failure: 'The dependencies in your gemfile changed, but the lockfile can't be updated because frozen mode is set' The previous commit added 'gem "capybara", "~> 3.40"' to Gemfile.development_dependencies but didn't update the root Gemfile.lock, causing CI to fail in frozen mode. Changes: - Updated addressable: 2.8.6 -> 2.8.7 - Updated matrix: 0.4.2 -> 0.4.3 - Updated public_suffix: 5.0.5 -> 6.0.2 - Updated regexp_parser: 2.9.2 -> 2.11.3 - Reflects capybara ~> 3.40 constraint Testing: - bundle update capybara completed successfully - All dependency updates are from Capybara's dependency tree 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Document comprehensive strategy for breaking large PRs into smaller, more manageable pieces when facing multiple CI test failures. New Document: .claude/docs/pr-splitting-strategy.md Key Sections: 1. When to Split a Large PR - Indicators: multiple failures, 50+ commits, mixed concerns - Decision criteria and time investment thresholds - Real example: PR #2069 with 52 commits, 3 failures + 1 hung 2. Strategy for Splitting - Identify independent commits - Determine merge order (docs → bug fixes → infrastructure) - Create focused PRs with clear scope - Handle original PR (close vs rebase) 3. Real-World Example: PR #2069 Split Plan - PR 1: Documentation (merge first, zero risk) - PR 2: buildConsoleReplay fix (focused bug fix) - PR 3: Workspace dependencies (small infrastructure) - PR 4: Monorepo node renderer (defer, most complex) 4. Benefits & Anti-Patterns - Benefits: easier review, incremental progress, better debugging - Anti-patterns: splitting too much, dependent changes, no testing - Decision tree for when to split 5. Templates & Timelines - PR split announcement template - Real-world 4-week timeline example - Incremental progress vs stuck on one PR Updated: .claude/docs/analysis/INDEX.md - Added pr-splitting-strategy.md as document #6 - Added to quick navigation: "Decide whether to split a large PR" - Listed in related documentation section Rationale: When facing complex PRs with multiple CI failures (like PR #2069): - 52 commits make bisecting difficult - Multiple unrelated failures (integration, Pro, hung tests) - Estimated 4-8 hours to fix all issues - Some parts can provide value independently Splitting allows: - Merge low-risk changes (docs) immediately - Review focused changes more easily - Incremental progress instead of being blocked - Better attribution if new issues arise - Unblock dependent work This strategy applies to any large PR with multiple failures, not just monorepo work. Provides decision framework and concrete examples. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The workspace dependency for react-on-rails resolved to the published version 16.1.2 instead of the local workspace version. This can happen when yarn resolves dependencies during installation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808/monorepo-completion
branch
from
faac53c to
189f414
Compare
Code Review - Phase 5: Add Pro Node Renderer Package to WorkspaceI've reviewed PR #2069 and here's my comprehensive feedback: ✅ Strengths1. Excellent Migration Structure
2. Strong Documentation
3. Proper License Boundaries
4. Build Configuration
5. Tooling Configuration
|
During rebase, the 'Remove old webpack bundles' step and the 'Restore test webpack bundles from cache' step were accidentally removed from the Pro CI workflow. These steps are essential for: 1. Cleaning old webpack bundles before tests 2. Restoring cached webpack bundles to speed up CI This restores both steps to their original position in the workflow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/run-skipped-ci |
Code Review: Phase 5 - Add Pro Node Renderer Package to WorkspaceSummaryThis PR completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The changes are well-structured and align with the project's migration goals. However, there are several important considerations and recommendations below. ✅ Strengths1. Excellent Documentation
2. Proper Workspace Configuration
3. Git History Preservation
|
Changes made per code review: 1. **Standardize package.json author fields** - Changed all packages to use justin@shakacode.com (ShakaCode email) - Previously had inconsistent justin.gordon@gmail.com in some packages 2. **Move analysis documents to proper location** - Moved github-issue-1765-*.md files to .claude/docs/analysis/ - Follows CLAUDE.md project structure guidelines 3. **Verified build and dependencies** (no changes needed): - ✅ yarn build works correctly for all packages - ✅ yarn run yalc:publish works correctly - ✅ TypeScript type-check passes with no errors - ✅ Node-renderer workspace dependencies are correct 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - Phase 5: Pro Node Renderer Package MigrationSummaryThis PR completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package into the workspace structure. The migration appears well-executed with proper git history preservation and comprehensive documentation. However, there are several important considerations and recommendations. ✅ Strengths1. Excellent Documentation
2. Proper Git History Preservation
3. Workspace Configuration
4. License Compliance
5. .gitignore Updates
|
After rebasing on master (which merged PR #2097 with parameter order fixes), the ESLint configuration now has the plain 'default-param-last' rule enabled, not the TypeScript version. The buildConsoleReplay function has default params before an optional param (nonce?), which violates the rule. Added block-level eslint-disable comments. The consoleReplay function doesn't violate the rule (all params have defaults), so no disable needed. Fixes CI error: error Default parameters should be last default-param-last 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The Pro ESLint was linting files from the open-source package (packages/react-on-rails/) which has its own separate ESLint config. This caused lint failures in CI when the Pro lint job ran. Added '../packages/react-on-rails/' to globalIgnores to ensure Pro only lints Pro-specific code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Phase 5 - Pro Node Renderer Package ExtractionExecutive SummaryThis PR successfully completes Phase 5 of the monorepo migration by extracting the Pro Node Renderer into a separate workspace package ( ✅ Strengths1. Excellent Workspace Structure
2. Strong Security Practices
3. Comprehensive Documentation
4. Proper License Boundaries
5. Build Configuration
|
Code Review: Phase 5 - Pro Node Renderer Package ExtractionSummaryThis PR successfully extracts the Pro Node Renderer into a separate workspace package ( ✅ Strengths1. Excellent Package Structure
2. Build Configuration
3. Security - Authentication HandlerThe new
4. License Management
5. Configuration Files
6. Documentation
|
## Problem The prepare scripts were calling `yarn run build` which caused a deadlock when triggered during `yarn install --frozen-lockfile`. This made the CI test job hang indefinitely. ## Root Cause When `yarn install` executes the prepare lifecycle hook, it's still holding internal locks. Calling `yarn run` from within this hook tries to acquire the same locks, resulting in a deadlock. ## Solution Replace `yarn run build` with direct command execution in all prepare scripts: - `yarn run build` → `(rm -rf ./lib && tsc ...)` This avoids the yarn deadlock while maintaining the same build behavior. ## Testing - ✅ Clean install works: `rm -rf node_modules && yarn install --frozen-lockfile` - ✅ Jest CI tests no longer hang: `yarn workspace react-on-rails-pro-node-renderer run ci` - ✅ Build artifacts created correctly - ✅ YALC publish works as expected ## Files Changed - packages/react-on-rails/package.json - packages/react-on-rails-pro/package.json - packages/react-on-rails-pro-node-renderer/package.json 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Phase 5 - Pro Node Renderer Package MigrationI've completed a thorough review of this PR. Overall, this is a well-executed monorepo migration that follows best practices. Here's my detailed feedback: ✅ Strengths
|
This PR contains only documentation changes extracted from PR #2069. Zero risk - no code changes, just valuable documentation for the team. New Documentation Added: 1. CI Failure Analysis (.claude/docs/analysis/CI_FAILURES_2024-11-21.md) - Comprehensive analysis of PR #2069 test failures - Root cause analysis with evidence from CI logs - Identified suspicious commits - Fix recommendations (all marked UNTESTED HYPOTHESIS) - Prevention strategies - CRITICAL DISCLAIMER: Analysis from isolated workspace 2. Testing Requirements (CLAUDE.md) - New section: "CRITICAL - LOCAL TESTING REQUIREMENTS" - Must distinguish "This fixes" vs "This SHOULD fix" - Must explain why local testing isn't possible - Must provide test commands - Must document what was tested - Enhanced "Replicating CI Failures Locally" section 3. PR Splitting Strategy (.claude/docs/pr-splitting-strategy.md) - When to split large PRs (indicators, decision criteria) - Strategy for identifying independent commits - Step-by-step splitting process - Real example: How to split PR #2069 - Benefits, anti-patterns, decision tree - Templates for PR split announcements 4. Analysis Index (.claude/docs/analysis/INDEX.md) - Added new documents to navigation - Updated quick reference links Why This Matters: - Documents actual CI failures for future reference - Establishes clear requirements for testing vs hypothetical fixes - Provides framework for handling complex PRs with multiple failures - All future AI work will distinguish tested vs untested fixes - Team has strategy for breaking up large problematic PRs Testing: None required (documentation only) Related: PR #2069 (monorepo-completion) which will not merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Documentation-only changes extracted from PR #2069. Zero code changes - provides valuable documentation for handling CI failures and splitting large PRs. Includes: 1. CI failure analysis for PR #2069 2. Testing requirements (tested vs untested fixes) 3. PR splitting strategy guide 4. Analysis index with navigation All changes rebased on latest master. Related: PR #2069, supersedes PR #2101 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Completes Phase 5 of the monorepo migration by extracting the Pro node-renderer package to the workspace structure. This establishes the 3-package NPM workspace and sets up proper workspace dependency management.
Changes Made
Package Structure
packages/react-on-rails-pro-node-renderer/with package.json and tsconfig.jsonConfiguration Updates
Workspace Verification
All 3 packages properly recognized:
Known Issues (Pre-Existing)
Node-renderer has TypeScript build errors that existed before this move:
These will be fixed in a separate follow-up PR. See PHASE_5_COMPLETION_NOTES.md for details.
Pull Request Checklist
Related
Summary by CodeRabbit
New Features
Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.