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:
|
packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts
Show resolved
Hide resolved
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).
packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts
Show resolved
Hide resolved
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
|
… output Remove orphaned mock_root/config_file_path setup from license_task_formatter_spec — load_license_string no longer calls Rails.root after the file-fallback removal. Add negative assertion to verify the key-file path no longer appears in the missing-license output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ihabadham
force-pushed
the
ihabadham/chore/remove-license-key-file-option-ror
branch
from
442de93 to
e825c05
Compare
Review: Remove Pro license key-file fallbackThe overall approach is clean and correct. Removing the file-based fallback simplifies the validator, the UNINITIALIZED sentinel fixes a real caching bug, and the migration warning is a good UX touch. IssuesUX gap – migration notice silently skipped when env var is valid The migration warning in Caching inconsistency between
Missing TypeScript whitespace/trim tests The Ruby spec adds explicit tests for surrounding whitespace and trailing newlines. The TypeScript Minor nit
Positive notes
|
packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts
Show resolved
Hide resolved
packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts
Show resolved
Hide resolved
Users who migrated to REACT_ON_RAILS_PRO_LICENSE but left the old config/react_on_rails_pro_license.key on disk were never prompted to delete it. Now logs an info-level cleanup notice in that case. Also adds TS whitespace/trim tests paralleling the Ruby coverage, and a comment clarifying the || undefined idiom in loadLicenseString. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressing the remaining items from the review above: UX gap — migration notice skipped when env var is valid: Missing TypeScript whitespace/trim tests: |
PR Review: Remove Pro license key-file fallback and add migration guidanceOverall this is a well-executed breaking change. The intent is clear, the logic is correct, and test coverage is solid. A few issues worth addressing before merge: Critical / CorrectnessNone — the behavioral changes are correct. Moderate Issues1. Expired/invalid license + legacy file: no migration notice logged 2. Overlapping matchers in migration warning spec Minor Issues3. Cache sentinel asymmetry in 4. Slightly inaccurate comment on 5. Mixed RSpec patterns Positive Notes
|
| // The caching here is deterministic - given the same environment variable value, every | ||
| // worker will compute the same cached values. Redundant computation across workers | ||
| // is acceptable since license validation is infrequent (once per worker startup). | ||
| let cachedLicenseStatus: LicenseStatus | undefined; |
There was a problem hiding this comment.
The cachedLicenseStatus variable uses undefined as its "not yet initialized" sentinel, while cachedLicenseOrganization and cachedLicensePlan use the UNINITIALIZED symbol specifically to distinguish between "not yet computed" and "computed as undefined".
This inconsistency works today only because LicenseStatus never includes undefined in its union. But if that assumption ever changes, this check silently breaks. For consistency and future safety, consider:
| let cachedLicenseStatus: LicenseStatus | undefined; | |
| const UNINITIALIZED = Symbol('uninitialized'); | |
| let cachedLicenseStatus: LicenseStatus | typeof UNINITIALIZED = UNINITIALIZED; | |
| let cachedLicenseOrganization: string | undefined | typeof UNINITIALIZED = UNINITIALIZED; | |
| let cachedLicensePlan: ValidPlan | undefined | typeof UNINITIALIZED = UNINITIALIZED; |
And update getLicenseStatus() to check cachedLicenseStatus !== UNINITIALIZED, and reset() to set it back to UNINITIALIZED.
| return undefined; | ||
| // `|| undefined` converts an empty/whitespace-only env var to undefined, | ||
| // so it is reported as 'missing' rather than 'invalid'. | ||
| return envLicense || undefined; |
There was a problem hiding this comment.
The comment is slightly inaccurate: .trim() already handles the whitespace removal. By the time || undefined executes, envLicense is either undefined (env var not set) or a trimmed string. The || undefined only converts the empty-string case. Consider:
| return envLicense || undefined; | |
| // `.trim()` strips surrounding whitespace/newlines; `|| undefined` converts the resulting | |
| // empty string to `undefined` so it is reported as 'missing' rather than 'invalid'. | |
| return envLicense || undefined; |
| log_legacy_license_migration_notice if legacy_license_file_present? | ||
| log_license_issue("No license found", "Get a license at #{LICENSE_URL}") | ||
| when :expired | ||
| expiration = ReactOnRailsPro::LicenseValidator.license_expiration |
There was a problem hiding this comment.
The legacy file migration notice is only emitted for :missing and :valid statuses. If a user has the legacy file present AND has set REACT_ON_RAILS_PRO_LICENSE to an expired or invalid token, they get no hint that the legacy file is irrelevant.
While less common, this could confuse users who think the key file might be the fallback. Consider adding the cleanup/migration notice to the :expired and :invalid branches as well, or at minimum add a comment here explaining why those cases are intentionally excluded.
| 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/) |
There was a problem hiding this comment.
The three have_received(:warn) assertions here are misleading — the first two (/legacy license file/ and /REACT_ON_RAILS_PRO_LICENSE/) both match the same single warn call (the migration notice), since the migration message contains both substrings. This makes the test look like it's verifying three distinct log statements when it's really only verifying two.
Consider either combining into one matcher or asserting the exact number of warn calls:
expect(mock_logger).to have_received(:warn).with(/Detected legacy license file.*REACT_ON_RAILS_PRO_LICENSE/m)
expect(mock_logger).to have_received(:warn).with(/No license found/)
expect(mock_logger).to have_received(:warn).exactly(2).times
|
All five items reviewed — no changes needed:
|
ihabadham
deleted the
ihabadham/chore/remove-license-key-file-option-ror
branch
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