Remove Pro license key-file fallback and add migration guidance#2454
Remove Pro license key-file fallback and add migration guidance#2454
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidates license loading to use only the REACT_ON_RAILS_PRO_LICENSE environment variable, removes file-system-based loading and fs/path usage in the Node renderer, adds legacy config-file detection and migration notices in the Rails engine, and updates tests and documentation accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
Greptile SummaryThis PR removes the legacy file-based license configuration ( Key Changes:
Issues Found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App Startup] --> B{License Status Check}
B -->|Valid License| C[App Runs Normally]
B -->|Missing License| D{Legacy File Exists?}
D -->|Yes| E[Log Migration Warning]
D -->|No| F[Log Missing License]
E --> G{Environment}
F --> G
G -->|Production| H[Warn: License Violation]
G -->|Dev/Test| I[Info: No License Required]
H --> J[App Continues]
I --> J
B -->|Expired/Invalid| K[Log License Issue]
K --> G
style E fill:#ff9
style H fill:#faa
style I fill:#afa
Last reviewed commit: 4e09661 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR Review: Remove Pro License Key-File FallbackOverall, the goal of this PR is sound — simplifying the license loading path to env-var-only and adding a migration warning is the right approach. The Ruby implementation in Critical: Test Expectations Are Wrong (engine_spec.rb)The two new spec contexts ( The problem: expect(mock_logger).to receive(:warn).with(/legacy license file/) # expects call #1
expect(mock_logger).to receive(:warn).with(/REACT_ON_RAILS_PRO_LICENSE/) # expects call #2
described_class.log_license_statusWhat actually happens when
Result: The same problem affects the development context with Fix: Combine into a single expectation that matches the full migration notice, and use it "logs migration warning" do
expect(mock_logger).to receive(:warn)
.with(/Detected legacy license file.*REACT_ON_RAILS_PRO_LICENSE/m)
allow(mock_logger).to receive(:warn) # allow the license-issue warning
described_class.log_license_status
endOr, if you want to verify both messages separately, use ordered expectations and match each call's actual content. Minor: Empty
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts (1)
55-55:getLicenseOrganization/getLicensePlancache misses onundefinedresults — re-verifies JWT on every call.The new comment at Line 55 emphasises "once per worker" deterministic caching, but
cachedLicenseOrganizationandcachedLicensePlanuseundefinedas both the uninitialized sentinel and the valid "not found" return value. WhendetermineLicenseOrganization()/determineLicensePlan()returnundefined(missing or invalid license), the check!== undefinedis alwaysfalse, so JWT verification runs on every invocation.getLicenseStatus()doesn't have this problem because its return type (LicenseStatus) is neverundefined.Consider using a dedicated "not yet computed" sentinel:
♻️ Suggested fix (sentinel pattern)
+const NOT_COMPUTED = Symbol('NOT_COMPUTED'); + -let cachedLicenseOrganization: string | undefined; -let cachedLicensePlan: ValidPlan | undefined; +let cachedLicenseOrganization: string | undefined | typeof NOT_COMPUTED = NOT_COMPUTED; +let cachedLicensePlan: ValidPlan | undefined | typeof NOT_COMPUTED = NOT_COMPUTED;export function getLicenseOrganization(): string | undefined { - if (cachedLicenseOrganization !== undefined) { + if (cachedLicenseOrganization !== NOT_COMPUTED) { return cachedLicenseOrganization; } cachedLicenseOrganization = determineLicenseOrganization(); return cachedLicenseOrganization; }export function getLicensePlan(): ValidPlan | undefined { - if (cachedLicensePlan !== undefined) { + if (cachedLicensePlan !== NOT_COMPUTED) { return cachedLicensePlan; } cachedLicensePlan = determineLicensePlan(); return cachedLicensePlan; }export function reset(): void { cachedLicenseStatus = undefined; - cachedLicenseOrganization = undefined; - cachedLicensePlan = undefined; + cachedLicenseOrganization = NOT_COMPUTED; + cachedLicensePlan = NOT_COMPUTED; }Also applies to: 233-240, 271-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts` at line 55, cachedLicenseOrganization and cachedLicensePlan use undefined as both "not yet computed" and "computed-but-not-found", causing determineLicenseOrganization()/determineLicensePlan() to re-run JWT verification on every call; change the cache to use a dedicated uninitialized sentinel (e.g., a module-level constant like UNINITIALIZED) instead of undefined, initialize cachedLicenseOrganization/cachedLicensePlan to that sentinel, update getLicenseOrganization/getLicensePlan to check !== UNINITIALIZED before returning cached values, and when computing assign either the actual value (string) or undefined to the cache; apply the same sentinel change to the other cache instances referenced around the other blocks (the ones flagged in the comment) so computed-undefined is stored and not retriggered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 30-31: Add an Unreleased → Pro changelog entry in CHANGELOG.md
noting the breaking change "Removing the legacy key-file fallback" under the Pro
section; format the entry exactly as "[PR
<number>](https://github.com/shakacode/react_on_rails/pull/<number>) by
[username](https://github.com/username)" (no “#” before the PR number) and
include the PR number and author for traceability.
In `@react_on_rails_pro/lib/react_on_rails_pro/license_validator.rb`:
- Around line 235-241: The load_license_string method returns the raw ENV value
which may include trailing whitespace/newlines causing token verification to
fail; update load_license_string to strip the fetched
ENV["REACT_ON_RAILS_PRO_LICENSE"] (e.g., call .strip) before returning and
return nil if the stripped string is empty so downstream verification uses a
clean token; refer to the load_license_string method when making this change.
---
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts`:
- Line 55: cachedLicenseOrganization and cachedLicensePlan use undefined as both
"not yet computed" and "computed-but-not-found", causing
determineLicenseOrganization()/determineLicensePlan() to re-run JWT verification
on every call; change the cache to use a dedicated uninitialized sentinel (e.g.,
a module-level constant like UNINITIALIZED) instead of undefined, initialize
cachedLicenseOrganization/cachedLicensePlan to that sentinel, update
getLicenseOrganization/getLicensePlan to check !== UNINITIALIZED before
returning cached values, and when computing assign either the actual value
(string) or undefined to the cache; apply the same sentinel change to the other
cache instances referenced around the other blocks (the ones flagged in the
comment) so computed-undefined is stored and not retriggered.
Review: Remove Pro license key-file fallbackThe overall direction of this PR is good — removing the file-based fallback simplifies the license loading path and the migration warning is a thoughtful addition. A few issues worth addressing before merging: Breaking change missing from CHANGELOGThe #### Pro
- **Breaking**: Removed file-based license key fallback (`config/react_on_rails_pro_license.key`). License must now be configured via the `REACT_ON_RAILS_PRO_LICENSE` environment variable. See the [migration guide](react_on_rails_pro/docs/updating.md) for details.Node.js side has no migration warning
Minor:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails_pro/spec/react_on_rails_pro/engine_spec.rb (1)
63-67: Redundantallowstubs beforehave_receivedassertions.
let(:mock_logger) { instance_double(Logger, warn: nil, info: nil) }already installsallow-style stubs with call-tracking for bothwarnandinfo, so re-issuingallow(mock_logger).to receive(:warn)(line 63) andallow(mock_logger).to receive(:info)(line 194) before the assertions is redundant.have_receivedworks on the existing stubs without the extraallow.♻️ Proposed cleanup
it "logs migration warning for env-var setup" do - allow(mock_logger).to receive(:warn) described_class.log_license_status expect(mock_logger).to have_received(:warn).with(/legacy license file/) expect(mock_logger).to have_received(:warn).with(/REACT_ON_RAILS_PRO_LICENSE/) endit "logs migration info for env-var setup" do - allow(mock_logger).to receive(:info) described_class.log_license_status expect(mock_logger).to have_received(:info).with(/legacy license file/) expect(mock_logger).to have_received(:info).with(/REACT_ON_RAILS_PRO_LICENSE/) endAlso applies to: 194-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/react_on_rails_pro/engine_spec.rb` around lines 63 - 67, Remove the redundant explicit stubs that re-call allow(mock_logger).to receive(:warn) and allow(mock_logger).to receive(:info); the let-created instance_double mock_logger (defined with warn: nil, info: nil) already provides call-tracking for have_received, so delete those extra allow(...) lines and leave the described_class.log_license_status invocation and the expect(mock_logger).to have_received(:warn).with(...) / expect(mock_logger).to have_received(:info).with(...) assertions as-is to rely on the original instance_double stubs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails_pro/spec/react_on_rails_pro/engine_spec.rb`:
- Around line 63-67: Remove the redundant explicit stubs that re-call
allow(mock_logger).to receive(:warn) and allow(mock_logger).to receive(:info);
the let-created instance_double mock_logger (defined with warn: nil, info: nil)
already provides call-tracking for have_received, so delete those extra
allow(...) lines and leave the described_class.log_license_status invocation and
the expect(mock_logger).to have_received(:warn).with(...) /
expect(mock_logger).to have_received(:info).with(...) assertions as-is to rely
on the original instance_double stubs.
Review: Remove Pro license key-file fallbackOverall this is a clean, well-scoped change with good rationale. The env-var-only approach is simpler, more secure, and consistent with modern deployment practices. The migration notice and deprecation policy doc are thoughtful additions. Positive highlights
Issues1. Test coverage gap in the legacy-file warning contexts (engine_spec.rb) The new nested contexts only assert the migration message was warned/info'd, but don't verify that the primary Suggested addition to each new context: expect(mock_logger).to have_received(:warn).with(/No license found/) # production context
# or
expect(mock_logger).to have_received(:info).with(/No license found/) # dev context2. Migration warning silently skipped for expired/invalid licenses
3. Minor: redundant return envLicense || undefined;
4. The now-removed line |
PR Review: Remove Pro license key-file fallbackOverall this is a clean, well-structured change. The env-var-only approach is simpler and more secure, the migration notice in the engine is appropriately scoped by environment, and the Ruby/Node implementations are kept in sync. A few issues worth addressing before merge: Bug: Stale Rails.root stub in
|
ihabadham
force-pushed
the
ihabadham/chore/remove-license-key-file-option-ror
branch
from
17ab66e to
8668993
Compare
Review NotesThe overall approach is sound: dropping file-based license loading simplifies the codebase, reduces the attack surface, and the runtime migration notice is a good UX touch for users who haven't migrated yet. A few things worth addressing before merge: Missing CHANGELOG entryThis PR introduces a breaking change for any users still relying on TODO without an issue reference (
|
Review SummaryOverall the PR is clean and well-structured. The approach of env-var-only validation (removing the file fallback) is the right direction, and the runtime migration warning with a TODO-with-date is good practice. A few items to address: Must FixMissing CHANGELOG entry. This is a breaking change: users relying on Migration hint missing from rake task output. The Already Flagged by Other Reviewers (Agree)
|
| @@ -42,7 +42,6 @@ def print_status_line(status) | |||
|
|
|||
| puts "" | |||
| puts "No license found. Set REACT_ON_RAILS_PRO_LICENSE" | |||
There was a problem hiding this comment.
The rake task outputs directly to stdout, bypassing Rails.logger. When a user runs rake react_on_rails_pro:verify_license with only the legacy key file present (no env var), the Rails log will contain the migration notice but this stdout output will just say "No license found. Set REACT_ON_RAILS_PRO_LICENSE" — giving the user no indication that the legacy file was detected but ignored.
Consider adding a migration hint here when the legacy file exists, for example:
def print_status_line(status)
puts "Status: #{status.to_s.upcase}"
return unless status == :missing
puts ""
puts "No license found. Set REACT_ON_RAILS_PRO_LICENSE"
# TODO: Remove after 16.5.0 stable
legacy_file = Rails.root.join("config", "react_on_rails_pro_license.key")
if legacy_file.exist?
puts "NOTE: Detected legacy config/react_on_rails_pro_license.key — this file is no longer read."
puts " Move the token to REACT_ON_RAILS_PRO_LICENSE."
end
endOr pass the hint in from the rake task itself (keeping the formatter dependency-free).
Code ReviewOverall, this is a clean and well-reasoned PR. Removing the file-based license fallback and migrating to env-var-only is a good security improvement. The implementation is solid. A few observations below. ✅ What's done well
|
|
All five items reviewed — no changes needed:
|
ihabadham
deleted the
ihabadham/chore/remove-license-key-file-option-ror
branch
Remove legacy config/react_on_rails_pro_license.key fallback. License must now be configured via REACT_ON_RAILS_PRO_LICENSE environment variable. Includes migration warning at startup when legacy file is detected, UNINITIALIZED sentinel fix for TS caching bug, and whitespace/trim handling.
1 participant
Summary
config/react_on_rails_pro_license.key(Ruby + Node validators now env-var only)Why
Align runtime behavior and documentation, and reduce upgrade surprises for existing installs that still rely on the legacy key-file path.
Testing
pnpm exec jest tests/licenseValidator.test.ts --runInBandbundle exec rubocop --force-exclusion --display-cop-names -- react_on_rails_pro/lib/react_on_rails_pro/engine.rb react_on_rails_pro/spec/react_on_rails_pro/engine_spec.rbconfig/react_on_rails_pro_license.keyand no env var, Ruby and Node both report missingNotes
react_on_rails_pro/spec/react_on_rails_pro/engine_spec.rbend-to-end in this environment due missing PostgreSQL headers (libpq-fe.h) for the Pro bundle context.Summary by CodeRabbit
Breaking Changes
Documentation
Behavior