Spike: Prism-based Gemfile rewriter for Pro migration (#3313)#3349
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:
WalkthroughAdds a Prism-based Gemfile rewriter prototype, decision record, README, benchmark, RSpec spike tests covering the behavior matrix and parse-failure policy, and excludes spike files from linting and gem packaging. ChangesPrism Gemfile Rewriter Spike Evaluation
sequenceDiagram
participant Rewriter as PrismGemfileRewriter
participant Parser as Prism Parser
participant Collector as Gem Call Collector
participant Editor as Edit Generator
participant Applier as Edit Applier
participant Collapser as Conditional Collapser
Rewriter->>Parser: parse(source)
Parser-->>Rewriter: AST
Rewriter->>Collector: collect gem nodes for react_on_rails / react_on_rails_pro
Collector-->>Rewriter: node locations + literals
Rewriter->>Editor: build replacement/removal edits
Editor-->>Rewriter: edits (byte ranges + replacements)
Rewriter->>Applier: apply_edits(source, edits)
Applier-->>Rewriter: spliced source
Rewriter->>Collapser: collapse_dead_conditionals(updated_source)
Collapser-->>Rewriter: final source
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4851ca25c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| end | ||
|
|
||
| def statement_byte_range(source, node) | ||
| start_offset = line_start_offset(source, node.location.start_offset) |
There was a problem hiding this comment.
When has_active_pro is true, statement_byte_range starts deletion at the beginning of the line and ends at newline, so a base gem on a shared line (e.g. semicolon-separated declarations or one-line conditionals) removes unrelated statements too, including react_on_rails_pro. For example, gem "react_on_rails_pro"; gem "react_on_rails" is rewritten to an empty line, which can silently drop required dependencies.
Useful? React with 👍 / 👎.
|
|
||
| def has_user_version_pin?(call) | ||
| positional_args = call.arguments.arguments.drop(1).reject { |a| a.is_a?(Prism::KeywordHashNode) } | ||
| positional_args.any? { |a| a.is_a?(Prism::StringNode) } |
There was a problem hiding this comment.
has_user_version_pin? only recognizes Prism::StringNode pins, so non-literal requirements (for example gem "react_on_rails", SOME_VERSION) are treated as unpinned and get an extra default version injected. That changes dependency semantics by adding an additional constraint and can produce conflicting or invalid requirements after rewrite.
Useful? React with 👍 / 👎.
Code ReviewSummary: Well-executed research spike that directly answers #3313. The decision record is thorough, the recommendation is clearly reasoned, and the prototype does real work. Net change to the production surface is exactly one RuboCop exclusion line — appropriately scoped for exploratory work. What works well
Issues worth noting before promotion to
|
Greptile SummaryThis PR delivers the decision record requested by #3313: a working Prism-based Gemfile rewriter prototype, a 32-case behavior matrix, a parse-and-rewrite benchmark, and the recommendation to keep the current scanner until
Confidence Score: 4/5Safe to merge — all changes are in a clearly-isolated spike directory and the only production file touched is a one-line .rubocop.yml exclusion. The prototype code is well-structured and the 32-case spec gives good confidence in its logic. Two minor gaps worth noting before a future production port: enclosing_statement_node is an unfulfilled stub whose misleading comment could confuse a future implementer, and has_user_version_pin? injects a default version alongside git:/path: source options. Neither affects the spike conclusions or the production lib/ code. prism_gemfile_rewriter.rb lines 603-631 for the two clarity/logic notes; all other files are straightforward. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rewrite source] --> B{Prism.parse}
B -- failure? --> C[Return original + parse_failed: true]
B -- success --> D[collect_gem_calls\nbase_calls / pro_calls]
D --> E{base_calls empty?}
E -- yes --> F[Return source unchanged]
E -- no --> G{pro_calls any?}
G -- yes: has_active_pro --> H[removal_edit per base call\nline_start to next newline]
G -- no --> I[replacement_edit per base call\nreplace first StringNode only]
H --> J[apply_edits reverse order]
I --> J
J --> K{has_active_pro?}
K -- yes --> L[collapse_dead_conditionals\nloop + re-parse]
K -- no --> M[Return Result]
L --> N{find_collapse_edit}
N -- found --> O{single Pro gem\nin surviving branch?}
O -- yes --> P[collapse entire conditional\nto gem declaration]
O -- no --> Q[remove empty branch only]
P --> L
Q --> L
N -- none --> M
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@react_on_rails/spike/3313_prism_gemfile_rewriter/benchmark.rb`:
- Line 95: The long `%w[...]` iterator lines (the array literal followed by
`.each do |g|`) exceed the 120-char limit; split the array across multiple lines
or assign it to a local constant/variable (e.g., gems = %w[ ... ]) and then call
`.each do |g|` on the next line so the array elements are wrapped, and do the
same fix for the second occurrence around line 103 to keep each line under 120
characters.
In
`@react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb`:
- Line 21: The failing spec has overly long expectation strings (the line
building "expected parseable Ruby, got errors:
#{parse_result.errors.map(&:message).join(', ')}" and a similar long string at
line 300); break these into multiple concatenated string literals or use
multi-line string interpolation so each line stays under 120 chars. Locate the
expectation that references parse_result.errors.map(&:message).join(', ') in
prism_gemfile_rewriter_spec.rb and split the message into two or more quoted
pieces (or interpolate the joined errors into a shorter variable) so the
combined output is unchanged but no source line exceeds 120 characters; apply
the same wrap strategy to the other long expectation at line 300.
In `@react_on_rails/spike/3313_prism_gemfile_rewriter/README.md`:
- Around line 20-24: The fenced code block in README.md containing the shell
commands is missing a language identifier; update the opening triple-backticks
to include a language tag (e.g., change "```" to "```bash") for the block that
contains the three commands (cd react_on_rails, bundle exec rspec ..., bundle
exec ruby ...) so markdownlint MD040 is satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a300e3c-bf9f-4384-bd9f-454676ab6a70
📒 Files selected for processing (6)
react_on_rails/.rubocop.ymlreact_on_rails/spike/3313_prism_gemfile_rewriter/DECISION.mdreact_on_rails/spike/3313_prism_gemfile_rewriter/README.mdreact_on_rails/spike/3313_prism_gemfile_rewriter/benchmark.rbreact_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rbreact_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def enclosing_statement_node(call) | ||
| # We do not have an explicit parent pointer; the caller passes the CallNode, so | ||
| # we need to detect a postfix modifier by looking at the source slice. Prism's | ||
| # location for the CallNode does NOT include the modifier, so we re-locate by | ||
| # inspecting the source after the call's end_offset. | ||
| call | ||
| end |
There was a problem hiding this comment.
This stub works today because statement_byte_range does a forward-scan from node.location.end_offset to the next \n, which incidentally captures any postfix modifier text ( if ENV["X"]). The forward-scan compensates correctly as long as postfix modifiers are single-line, which is always true in practice.
For the production promotion, the right fix is to build a parent_map while collecting gem calls so you can walk upward to the actual IfNode / UnlessNode wrapper:
def collect_gem_calls(node, base_calls, pro_calls, parent_map = {})
return unless node
node.compact_child_nodes.each do |child|
parent_map[child] = node
if gem_call?(child)
# ...
end
collect_gem_calls(child, base_calls, pro_calls, parent_map)
end
endNot a concern for the spike, but noting it for the DECISION.md step 3 implementation.
| def collapse_dead_conditionals(source) | ||
| loop do | ||
| parse_result = Prism.parse(source) | ||
| break source if parse_result.failure? | ||
|
|
||
| edit = find_collapse_edit(source, parse_result.value) | ||
| break source unless edit | ||
|
|
||
| source = source[0...edit[:start_offset]] + edit[:replacement] + source[edit[:end_offset]..] | ||
| end |
There was a problem hiding this comment.
If any of the splice edits produce invalid Ruby (shouldn't happen with well-formed input, but defensive to consider), this loop exits on parse_result.failure? and returns the partially-modified source. The outer rewrite call then returns that content with parse_failed: false, which could mislead the caller.
For production, consider threading a failure flag back out:
def collapse_dead_conditionals(source)
loop do
parse_result = Prism.parse(source)
return [source, true] if parse_result.failure? # signal internal failure
edit = find_collapse_edit(source, parse_result.value)
return [source, false] unless edit
source = source[0...edit[:start_offset]] + edit[:replacement] + source[edit[:end_offset]..]
end
endThen the outer rewrite can set parse_failed: true on the result. Fine to leave as-is for the spike.
| puts "Per-rewrite cost (scanner / prism):" | ||
| GEMFILES.each do |label, src| | ||
| scanner_seconds = measure(ITERATIONS) { scanner.rewrite(src) } | ||
| prism_seconds = measure(ITERATIONS) { prism.rewrite(src) } | ||
| printf(" %-50s %8.3fms / %8.3fms\n", | ||
| "#{label} (#{src.lines.size}l)", | ||
| (scanner_seconds / ITERATIONS) * 1000, | ||
| (prism_seconds / ITERATIONS) * 1000) |
There was a problem hiding this comment.
This second measurement loop re-measures without the 3-iteration warm-up that the first loop uses (line 130), so the first Gemfile in this block will see first-call overhead that the first table avoided. The numbers in DECISION.md come from this block ("Per-rewrite cost"), so they may be slightly pessimistic for both implementations on the first entry.
Easy fix for when the numbers are formally cited:
puts "Per-rewrite cost (scanner / prism):"
GEMFILES.each do |label, src|
3.times { scanner.rewrite(src); prism.rewrite(src) } # warm up
scanner_seconds = measure(ITERATIONS) { scanner.rewrite(src) }
prism_seconds = measure(ITERATIONS) { prism.rewrite(src) }
printf(" %-50s %8.3fms / %8.3fms\n",
"#{label} (#{src.lines.size}l)",
(scanner_seconds / ITERATIONS) * 1000,
(prism_seconds / ITERATIONS) * 1000)
end![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed:
UnlessNodelackssubsequentmethod, crashes at runtime- Added an
IfNode/UnlessNodeelse-clause helper so conditional collapse no longer callssubsequentonUnlessNode.
- Added an
- ✅ Fixed: Spike files included in published gem package
- Updated the gemspec file filter to exclude the
spike/directory from packaged gem files.
- Updated the gemspec file filter to exclude the
- ✅ Fixed: Byte offsets used as character indices corrupts non-ASCII files
- Replaced character-index slicing and newline scans with byte-based helpers using
byteslice,bytesize, andgetbyte.
- Replaced character-index slicing and newline scans with byte-based helpers using
Or push these changes by commenting:
@cursor push e8672e795d
Preview (e8672e795d)
diff --git a/react_on_rails/react_on_rails.gemspec b/react_on_rails/react_on_rails.gemspec
--- a/react_on_rails/react_on_rails.gemspec
+++ b/react_on_rails/react_on_rails.gemspec
@@ -26,7 +26,7 @@
s.files = Dir.chdir(__dir__) do
`git ls-files -z`.split("\x0").reject do |f|
- f.match(%r{^(spec|tmp)/})
+ f.match(%r{^(spec|tmp|spike)/})
end
end
s.bindir = "exe"
diff --git a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb
--- a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb
+++ b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb
@@ -13,6 +13,7 @@
class PrismGemfileRewriter
BASE_GEM_NAME = "react_on_rails"
PRO_GEM_NAME = "react_on_rails_pro"
+ NEWLINE_BYTE = "\n".ord
Result = Struct.new(
:content,
@@ -130,7 +131,7 @@
end
def quote_char(source, string_node)
- source[string_node.location.start_offset]
+ source.byteslice(string_node.location.start_offset, 1)
end
def removal_edit(source, call)
@@ -160,18 +161,28 @@
# Walk forward from the node's end_offset to the end of the source line so that
# we also delete trailing modifier text (e.g. `if ENV["X"]`) and the newline.
scan = node.location.end_offset
- scan += 1 while scan < source.length && source[scan] != "\n"
- scan += 1 if scan < source.length && source[scan] == "\n"
+ scan += 1 while scan < source.bytesize && source.getbyte(scan) != NEWLINE_BYTE
+ scan += 1 if scan < source.bytesize && source.getbyte(scan) == NEWLINE_BYTE
[start_offset, scan]
end
def apply_edits(source, edits)
sorted = edits.sort_by { |e| -e[:start_offset] }
sorted.reduce(source) do |acc, edit|
- acc[0...edit[:start_offset]] + edit[:replacement] + acc[edit[:end_offset]..]
+ splice_source(acc, edit[:start_offset], edit[:end_offset], edit[:replacement])
end
end
+ def byte_slice(source, start_offset, end_offset)
+ source.byteslice(start_offset, end_offset - start_offset) || ""
+ end
+
+ def splice_source(source, start_offset, end_offset, replacement)
+ prefix = source.byteslice(0, start_offset) || ""
+ suffix = source.byteslice(end_offset, source.bytesize - end_offset) || ""
+ prefix + replacement + suffix
+ end
+
# After removing base gem statements, conditionals like
# if ENV["PRO"]
# gem "react_on_rails_pro", "16.0.0"
@@ -200,7 +211,7 @@
edit = find_collapse_edit(source, parse_result.value)
break source unless edit
- source = source[0...edit[:start_offset]] + edit[:replacement] + source[edit[:end_offset]..]
+ source = splice_source(source, edit[:start_offset], edit[:end_offset], edit[:replacement])
end
end
@@ -226,8 +237,9 @@
return nil unless node.respond_to?(:end_keyword_loc) && node.end_keyword_loc
then_branch = node.statements
- has_else_node = node.subsequent.is_a?(Prism::ElseNode)
- else_statements = has_else_node ? node.subsequent.statements : nil
+ else_node = conditional_else_node(node)
+ has_else_node = !else_node.nil?
+ else_statements = has_else_node ? else_node.statements : nil
then_empty = branch_empty?(then_branch)
else_empty = has_else_node && branch_empty?(else_statements)
@@ -250,7 +262,7 @@
return nil unless single_pro_gem_call?(branch_statements)
gem_call = branch_statements.body.first
- gem_text = source[gem_call.location.start_offset...gem_call.location.end_offset]
+ gem_text = byte_slice(source, gem_call.location.start_offset, gem_call.location.end_offset)
# Preserve indentation of the original conditional.
indent = leading_indentation(source, conditional.location.start_offset)
{
@@ -270,9 +282,10 @@
def remove_empty_else_branch(source, node)
# Find the `else` keyword location and remove from there to just before `end`.
- return nil unless node.subsequent.is_a?(Prism::ElseNode)
+ else_node = conditional_else_node(node)
+ return nil unless else_node
- else_loc = node.subsequent.else_keyword_loc
+ else_loc = else_node.else_keyword_loc
end_loc = node.end_keyword_loc
return nil unless else_loc && end_loc
@@ -294,21 +307,29 @@
def leading_indentation(source, offset)
line_start = line_start_offset(source, offset)
- slice = source[line_start...offset]
+ slice = byte_slice(source, line_start, offset)
slice[/\A\s*/].to_s
end
def line_start_offset(source, offset)
return 0 if offset.zero?
- idx = source.rindex("\n", offset - 1)
+ idx = source.b.rindex("\n".b, offset - 1)
idx ? idx + 1 : 0
end
def line_end_offset(source, offset)
- idx = source.index("\n", offset)
- idx ? idx + 1 : source.length
+ idx = source.b.index("\n".b, offset)
+ idx ? idx + 1 : source.bytesize
end
+
+ def conditional_else_node(node)
+ if node.is_a?(Prism::IfNode)
+ node.subsequent if node.subsequent.is_a?(Prism::ElseNode)
+ elsif node.is_a?(Prism::UnlessNode)
+ node.else_clause
+ end
+ end
end
end
end
diff --git a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
--- a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
+++ b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
@@ -54,6 +54,18 @@
result = rewriter.rewrite(src)
expect(result.content).to eq("gem \"react_on_rails_pro\", \"#{default_pro_version}\"\n")
end
+
+ it "rewrites declarations after UTF-8 content using byte offsets" do
+ src = <<~RUBY
+ # Dépendances
+ gem "react_on_rails", "~> 16.0"
+ RUBY
+ result = rewriter.rewrite(src)
+ expect(result.content).to eq(<<~RUBY)
+ # Dépendances
+ gem "react_on_rails_pro", "~> 16.0"
+ RUBY
+ end
end
context "kwargs and options" do
@@ -286,6 +298,24 @@
expect_parseable_ruby(result.content)
end
+ it "does not crash on unrelated unless blocks when removing stale base" do
+ src = <<~RUBY
+ unless ENV["CI"]
+ gem "pry"
+ end
+ gem "react_on_rails", "~> 16.0"
+ gem "react_on_rails_pro", "~> 16.0"
+ RUBY
+ result = rewriter.rewrite(src)
+ expect(result.content).to eq(<<~RUBY)
+ unless ENV["CI"]
+ gem "pry"
+ end
+ gem "react_on_rails_pro", "~> 16.0"
+ RUBY
+ expect_parseable_ruby(result.content)
+ end
+
it "rewrites base declarations in both conditional branches" do
src = <<~RUBY
if ENV["ROR_EDGE"]
@@ -318,6 +348,23 @@
expect_parseable_ruby(result.content)
end
+ it "collapses stale base conditionals after UTF-8 content using byte offsets" do
+ src = <<~RUBY
+ # Dépendances
+ if ENV["PRO"]
+ gem "react_on_rails_pro", "16.0.0"
+ else
+ gem "react_on_rails", "16.0.0"
+ end
+ RUBY
+ result = rewriter.rewrite(src)
+ expect(result.content).to eq(<<~RUBY)
+ # Dépendances
+ gem "react_on_rails_pro", "16.0.0"
+ RUBY
+ expect_parseable_ruby(result.content)
+ end
+
it "preserves indentation when collapsing an indented conditional" do
src = <<~RUBY
group :development doYou can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Semicolon scan corrupts gem removal
- Updated statement-boundary detection to ignore semicolons inside quoted strings and comments, with regression specs covering stale base removal.
Or push these changes by commenting:
@cursor push 9dc374d521
Preview (9dc374d521)
diff --git a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb
--- a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb
+++ b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb
@@ -15,6 +15,10 @@
PRO_GEM_NAME = "react_on_rails_pro"
NEWLINE_BYTE = "\n".ord
SEMICOLON_BYTE = ";".ord
+ COMMENT_BYTE = "#".ord
+ BACKSLASH_BYTE = "\\".ord
+ DOUBLE_QUOTE_BYTE = '"'.ord
+ SINGLE_QUOTE_BYTE = "'".ord
SOURCE_OPTION_KEYS = %w[git github path].freeze
Result = Struct.new(
@@ -156,20 +160,19 @@
def statement_byte_range(source, node)
line_start = line_start_offset(source, node.location.start_offset)
line_end = line_end_offset(source, node.location.end_offset)
- statement_end = node.location.end_offset
+ next_semicolon = next_semicolon_offset(
+ source,
+ line_start_offset(source, node.location.end_offset),
+ line_end,
+ node.location.end_offset
+ )
+ statement_end = next_semicolon || line_terminator_offset(source, line_end)
- while statement_end < line_end
- byte = source.getbyte(statement_end)
- break if byte == NEWLINE_BYTE || byte == SEMICOLON_BYTE
-
- statement_end += 1
- end
-
previous_semicolon = previous_semicolon_offset(source, line_start, node.location.start_offset)
if previous_semicolon
[previous_semicolon, statement_end]
- elsif statement_end < line_end && source.getbyte(statement_end) == SEMICOLON_BYTE
- end_offset = statement_end + 1
+ elsif next_semicolon
+ end_offset = next_semicolon + 1
end_offset += 1 while end_offset < line_end && horizontal_space?(source.getbyte(end_offset))
[line_start, end_offset]
else
@@ -178,16 +181,45 @@
end
def previous_semicolon_offset(source, line_start, statement_start)
- scan = statement_start - 1
- while scan >= line_start
- return scan if source.getbyte(scan) == SEMICOLON_BYTE
+ statement_separator_offsets(source, line_start, statement_start).last
+ end
- scan -= 1
+ def next_semicolon_offset(source, line_start, line_end, statement_end)
+ statement_separator_offsets(source, line_start, line_end).find { |offset| offset >= statement_end }
+ end
+
+ def statement_separator_offsets(source, line_start, line_end)
+ offsets = []
+ quote_byte = nil
+ scan = line_start
+ while scan < line_end
+ byte = source.getbyte(scan)
+ break if byte == NEWLINE_BYTE || (!quote_byte && byte == COMMENT_BYTE)
+
+ if quote_byte
+ if byte == BACKSLASH_BYTE
+ scan += 2
+ next
+ end
+
+ quote_byte = nil if byte == quote_byte
+ elsif byte == DOUBLE_QUOTE_BYTE || byte == SINGLE_QUOTE_BYTE
+ quote_byte = byte
+ elsif byte == SEMICOLON_BYTE
+ offsets << scan
+ end
+
+ scan += 1
end
- nil
+ offsets
end
+ def line_terminator_offset(source, line_end)
+ newline_offset = line_end - 1
+ newline_offset >= 0 && source.getbyte(newline_offset) == NEWLINE_BYTE ? newline_offset : line_end
+ end
+
def horizontal_space?(byte)
byte == " ".ord || byte == "\t".ord
end
diff --git a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
--- a/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
+++ b/react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
@@ -309,6 +309,23 @@
expect_parseable_ruby(result.content)
end
+ it "ignores semicolons in trailing comments when removing a stale base declaration" do
+ src = <<~RUBY
+ gem "react_on_rails", "~> 16.0" # note; keep removing the whole line
+ gem "react_on_rails_pro", "~> 16.0"
+ RUBY
+ result = rewriter.rewrite(src)
+ expect(result.content).to eq("gem \"react_on_rails_pro\", \"~> 16.0\"\n")
+ expect_parseable_ruby(result.content)
+ end
+
+ it "ignores semicolons in quoted trailing guards when removing a stale base declaration" do
+ src = "gem \"react_on_rails\" if ENV[\"ROR;ENABLED\"]; gem \"react_on_rails_pro\", \"~> 16.0\"\n"
+ result = rewriter.rewrite(src)
+ expect(result.content).to eq("gem \"react_on_rails_pro\", \"~> 16.0\"\n")
+ expect_parseable_ruby(result.content)
+ end
+
it "removes stale base when Pro is parenthesized with comments" do
src = <<~RUBY
source "https://rubygems.org"You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 19d9605. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d9605674
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while statement_end < line_end | ||
| byte = source.getbyte(statement_end) | ||
| break if byte == NEWLINE_BYTE || byte == SEMICOLON_BYTE |
There was a problem hiding this comment.
When has_active_pro is true, statement_byte_range stops at the first ; byte after the base gem call, but that byte is not guaranteed to be a statement separator. If the stale line has a trailing comment or postfix expression containing ; (e.g. gem "react_on_rails", "~> 16.0" # old; remove), the splice removes only part of the line and leaves tail text behind (like remove), changing Gemfile semantics or producing invalid Ruby. This should use AST locations/token context (or at least comment/string-aware scanning) instead of raw semicolon scanning.
Useful? React with 👍 / 👎.
Code Review — Spike: Prism-based Gemfile rewriter (#3313)OverallThis is a well-executed spike. The non-goal (no changes to The configuration changes ( What's done well
IssuesMinor bugs / inaccuracies:
Style:
Design notes (non-blocking):
VerdictApprove as a spike artifact. The decision record and prototype deliver exactly what #3313 asked for. No production code is touched. The two minor benchmark inaccuracies are worth a fix before merge so the numbers in the repo match the actual fixture sizes. |
| gem "jbuilder" | ||
| gem "bcrypt" | ||
| RUBY | ||
| "medium (80 lines, 1 ror entry, groups)" => begin |
There was a problem hiding this comment.
The label says 80 lines but this fixture generates ~45 lines (1 source + 10 base gems + 1 react_on_rails + 8 additional gems + 3 group headers + 3 ends + 3 blank lines + ~16 grouped gems). DECISION.md correctly says "~45 lines" in its performance table — the two should agree.
| "medium (80 lines, 1 ror entry, groups)" => begin | |
| "medium (~45 lines, 1 ror entry, groups)" => begin |
| end | ||
| end | ||
|
|
||
| def has_user_version_pin?(call) |
There was a problem hiding this comment.
Ruby convention for predicate methods drops the has_ prefix — user_version_pin? reads more naturally and matches the style of the rest of the class (e.g. gem_call?, branch_empty?, source_option_present?). Worth fixing if this moves to lib/.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bc28000cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start_offset: line_start_offset(source, conditional.location.start_offset), | ||
| end_offset: line_end_offset(source, conditional.location.end_offset), | ||
| replacement: "#{indent}#{gem_text}\n" |
There was a problem hiding this comment.
collapse_to_branch replaces from line_start_offset of the if to line_end_offset of the end, which assumes the conditional occupies the whole line; if there are sibling statements on the same line (for example end; gem "rails"), those statements are deleted during collapse. I verified this by running the rewriter on an if/else pro/base block followed by ; gem "rails", and the output dropped the rails dependency entirely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rb`:
- Around line 58-59: The current logic uses
preexisting_empty_conditional_fingerprints (from
empty_conditional_fingerprints(source, program)) and checks via include? which
only matches fingerprint values, causing duplicate identical conditionals to be
treated as already-collapsed; change this to track occurrences/counts instead of
simple membership: build a frequency map (or list of consumed indices) of
fingerprints from empty_conditional_fingerprints and then, when scanning
conditionals (the same places that call
preexisting_empty_conditional_fingerprints and use include?), consume/decrement
the count (or mark the specific occurrence) for each matched fingerprint so
identical fingerprints are only considered preexisting as many times as they
actually appear; apply the same occurrence-aware logic wherever the code
currently relies on include? (the variables/methods
preexisting_empty_conditional_fingerprints, empty_conditional_fingerprints, and
the surrounding conditional-collapsing checks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4b378e5-b8ff-49f3-a400-1acdeb5acf1e
📒 Files selected for processing (2)
react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter.rbreact_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
Code Review: Spike — Prism-based Gemfile rewriter (#3313)Overall verdict: ship it. The spike is well-scoped, the decision record is thorough, and the recommendation (wait for the Ruby floor to rise to 3.3) is well-reasoned. Anyone who revisits this later won't have to redo the analysis. What works well
Issues worth noting before merging1. If a Gemfile uses an if ENV["PRO"]
gem "react_on_rails_pro"
elsif ENV["EDGE"]
gem "react_on_rails", path: "../ror"
end
if ENV["PRO"]
gem "react_on_rails_pro"
elsif ENV["EDGE"]
endValid Ruby, but an empty 2. Minor spec and benchmark issues — see inline comments. Nit
|
| gem "jbuilder" | ||
| gem "bcrypt" | ||
| RUBY | ||
| "medium (80 lines, 1 ror entry, groups)" => begin |
There was a problem hiding this comment.
The "80 lines" in this label is inaccurate — the generated body is ~45 lines (1 source line + 10 base gems + 1 ROR gem + 8 more gems + blank + group header + 6 dev/test gems + end/blank/group + 6 dev gems + end/blank/group + 4 test gems + end). The per-run printout uses #{src.lines.size}l so the output is correct, but the hash key itself misleads anyone reading the source.
| "medium (80 lines, 1 ror entry, groups)" => begin | |
| "medium (~45 lines, 1 ror entry, groups)" => begin |
| def collapse_dead_conditionals(source, preexisting_empty_conditional_fingerprints) | ||
| loop do | ||
| parse_result = Prism.parse(source) | ||
| break source if parse_result.failure? | ||
|
|
||
| edit = find_collapse_edit(source, parse_result.value, preexisting_empty_conditional_fingerprints) | ||
| break source unless edit | ||
|
|
||
| source = splice_source(source, edit[:start_offset], edit[:end_offset], edit[:replacement]) | ||
| end |
There was a problem hiding this comment.
The termination argument is correct — each iteration either finds no edit (breaks) or splices out one collapsible conditional, so the editable-conditional count is monotonically decreasing. But for future-proofing, a hard iteration cap makes the safety guarantee explicit and catches any accidental cycle introduced later:
| def collapse_dead_conditionals(source, preexisting_empty_conditional_fingerprints) | |
| loop do | |
| parse_result = Prism.parse(source) | |
| break source if parse_result.failure? | |
| edit = find_collapse_edit(source, parse_result.value, preexisting_empty_conditional_fingerprints) | |
| break source unless edit | |
| source = splice_source(source, edit[:start_offset], edit[:end_offset], edit[:replacement]) | |
| end | |
| def collapse_dead_conditionals(source, preexisting_empty_conditional_fingerprints) | |
| max_iterations = 100 # one per collapsible conditional; a Gemfile can't realistically have more | |
| max_iterations.times do | |
| parse_result = Prism.parse(source) | |
| break if parse_result.failure? | |
| edit = find_collapse_edit(source, parse_result.value, preexisting_empty_conditional_fingerprints) | |
| break unless edit | |
| source = splice_source(source, edit[:start_offset], edit[:end_offset], edit[:replacement]) | |
| end | |
| source | |
| end |
| # Prism currently treats `gem("react_on_rails",)` as a syntax error in some Ruby | ||
| # versions but accepts it as valid in 3.3+. We document the behavior either way. | ||
| it "either rewrites cleanly or reports a parse failure" do | ||
| src = "gem(\"react_on_rails\",)\n" | ||
| result = rewriter.rewrite(src) | ||
| if result.parse_failed | ||
| expect(result.content).to eq(src) | ||
| else | ||
| expect(result.content).to include('"react_on_rails_pro"') | ||
| expect_parseable_ruby(result.content) | ||
| end | ||
| end |
There was a problem hiding this comment.
The "either/or" form is the right documentation choice for a spike running across Ruby versions, but if this test suite is ever run on Ruby 3.3+ exclusively (CI, or when promoting the spike to production), the ambiguity will mask a regression. Consider tagging the version boundary explicitly so the assertion tightens on modern Ruby:
it "either rewrites cleanly or reports a parse failure" do
src = "gem(\"react_on_rails\",)\n"
result = rewriter.rewrite(src)
if result.parse_failed
# Ruby < 3.3: trailing comma in parenthesised gem call is a syntax error
expect(result.content).to eq(src)
else
# Ruby 3.3+: trailing comma is accepted
expect(result.content).to include('"react_on_rails_pro"')
expect_parseable_ruby(result.content)
end
endOr, if the suite always runs on a fixed Ruby version, pin the expectation directly:
if RUBY_VERSION >= "3.3"
it "rewrites trailing-comma declarations on Ruby 3.3+" do
src = "gem(\"react_on_rails\",)\n"
result = rewriter.rewrite(src)
expect(result.parse_failed).to be(false)
expect(result.content).to include('"react_on_rails_pro"')
expect_parseable_ruby(result.content)
end
end
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 488b2ba310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| seen_empty_conditional_fingerprints[fingerprint] += 1 | ||
| return nil if seen_empty_conditional_fingerprints[fingerprint] <= | ||
| preexisting_empty_counts[fingerprint].to_i |
There was a problem hiding this comment.
The preexisting-empty-branch guard is keyed only by fingerprint counts, so it can skip a newly emptied conditional and instead collapse a different preexisting conditional with the same fingerprint when their order is reversed. In practice, if conditional A becomes empty after stale-base removal and conditional B was already empty before rewrite, this counter logic can leave A unchanged and collapse B, which incorrectly removes B's runtime guard and makes gem "react_on_rails_pro" unconditional.
Useful? React with 👍 / 👎.
| elsif byte == DOUBLE_QUOTE_BYTE || byte == SINGLE_QUOTE_BYTE | ||
| quote_byte = byte | ||
| elsif byte == SEMICOLON_BYTE | ||
| offsets << scan |
There was a problem hiding this comment.
Statement-boundary scanning only treats single/double quotes as string contexts, so semicolons inside percent-string literals are misread as separators. With gem "react_on_rails" if ENV[%q[a;b]]; gem "react_on_rails_pro", "~> 16.0", stale-base removal starts splicing at the semicolon inside %q[...], producing corrupted output (b]]; ...) instead of removing the base gem statement.
Useful? React with 👍 / 👎.
| replacement: "#{new_name_literal}, #{quote}#{default_pro_version}#{quote}" | ||
| } | ||
| end | ||
| end |
There was a problem hiding this comment.
Ruby convention discourages the has_ prefix for predicate methods. When this gets promoted to production, prefer user_version_pin?:
| end | |
| def user_version_pin?(call) |
(Also update the call site at line 122: if user_version_pin?(call))
|
|
||
| def next_semicolon_offset(source, line_start, line_end, statement_end) | ||
| statement_separator_offsets(source, line_start, line_end).find { |offset| offset >= statement_end } | ||
| end |
There was a problem hiding this comment.
The quote-tracking scanner handles "..." and '...' but not heredocs (<<~RUBY ... RUBY). A gem option value expressed as a heredoc (contrived but syntactically valid) would cause the scanner to scan past the heredoc delimiter into subsequent lines and potentially misidentify semicolons.
In practice this is extremely unlikely in a Gemfile, but worth documenting as a known gap before promoting this to production:
# NOTE: does not handle heredoc string delimiters; a gem kwarg value
# expressed as <<~HEREDOC would cause semicolons on subsequent lines to
# be misidentified as statement separators.
def statement_separator_offsets(source, line_start, line_end)
| end | ||
|
|
||
| puts | ||
| puts "Per-rewrite cost (scanner / prism):" |
There was a problem hiding this comment.
The warm-up has already run for these inputs before the "total" measurement block above. Re-warming here is harmless but redundant — the JIT/cache state carries over between the two GEMFILES.each loops. You can drop this second warm-up if you pull the script forward to production.
| replacement: "" | ||
| } | ||
| end | ||
|
|
There was a problem hiding this comment.
The stub is documented and acceptable for the spike. When promoting to production, the unless ENV["PRO"]; (empty); else gem_pro; end → if ENV["PRO"]; gem_pro; end transformation would be a natural follow-up — it's semantically equivalent (the empty unless branch is just dead code) and eliminates another category of post-migration ugliness the way collapse_to_branch does for the if/else case.
…sh (#3349) Addresses 11 review comments on the Prism Gemfile rewriter spike. Correctness fixes (verified bugs reproduced before the change): - statement_byte_range: when a stale base gem sits inside a same-line block conditional with a sibling branch (e.g. `if X then gem ror_pro else gem ror end`), remove only the call's own byte range so the surrounding conditional and the active Pro gem survive. The previous line-fallback dropped the whole conditional, silently corrupting the Gemfile. - remove_empty_else_branch + collapse_dead_conditionals: return nil from the removal helper when the computed splice would be a no-op (inline conditional where `else` and `end` share a line). The collapse loop additionally bounds itself with MAX_COLLAPSE_PASSES and guards against zero-length empty edits. Previously `rewrite` hung indefinitely on inline conditionals whose then branch contained more than one statement. Production-readiness polish (spike scope, for the future migration): - empty_conditional_ranges: drop the mutable default-argument pattern. - statement_separator_offsets: NOTE comment that parenthesis depth is not tracked (Bundler DSL does not produce ambiguous shapes in practice). - collapse_edit_for_conditional: caller-side note explaining the remove_empty_then_branch stub asymmetry. - benchmark.rb: single measurement pass; the totals/ratio table and the per-rewrite cost table now derive from the same timings. - README: document the transitive prism dev dependency on Ruby < 3.3. Spec additions: - Regression specs for both correctness fixes (inline if/then/else with sibling Pro gem; inline conditional whose else becomes empty post-removal, with Timeout to fail loud if the hang regresses). - unless/else collapse spec exercising the UnlessNode#else_clause path. - Defensive spec for bare /regex/ literals in line preceding a stale base. - Trailing-comma test split into RUBY_VERSION-gated tests instead of an either-or assertion that passes regardless of branch. 55 examples, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Re: the two new items in your latest review summary (#4531915486 —
The other items in the summary are tracked separately via the inline review comments and replied to in each thread. |
|
All 11 active review comments from the previous round (and the 2 duplicates) addressed in Mattered (verified bugs reproduced before fixing)
Production-readiness polish (spike scope, applied now to keep the prototype clean)
Skipped
55 examples, 0 failures. Scan window: comments posted after the previous |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bb18b8fd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start_offset: line_start, | ||
| end_offset: line_end, |
There was a problem hiding this comment.
When collapse_replacement_edit takes the no-semicolon branch, it replaces from line_start to line_end, which assumes the if/unless ... end occupies the entire line. For valid Ruby like chosen = if ENV["PRO"] ... else ... end, conditional.location.start_offset begins at if, but line_start_offset rewinds to column 0, so collapse drops the chosen = prefix and rewrites the line to just gem "react_on_rails_pro".... This silently changes semantics for expression-form conditionals instead of only simplifying the conditional node.
Useful? React with 👍 / 👎.
Spike for #3313. Standalone Prism prototype + 32-case behavior matrix + benchmark + decision record. Lives under react_on_rails/spike/ so it does not touch the production scanner from #3232 (per the issue's non-goals). Recommendation: keep the current text scanner. Revisit when the Ruby floor rises to 3.3 and Prism becomes a free runtime dependency. The empty-else case the issue calls out is solvable cleanly by the Prism prototype (conditional collapse), but the cost of adding prism as a runtime gem for Ruby 3.0-3.2 users outweighs the cleanup for a generator that runs once per migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sh (#3349) Addresses 11 review comments on the Prism Gemfile rewriter spike. Correctness fixes (verified bugs reproduced before the change): - statement_byte_range: when a stale base gem sits inside a same-line block conditional with a sibling branch (e.g. `if X then gem ror_pro else gem ror end`), remove only the call's own byte range so the surrounding conditional and the active Pro gem survive. The previous line-fallback dropped the whole conditional, silently corrupting the Gemfile. - remove_empty_else_branch + collapse_dead_conditionals: return nil from the removal helper when the computed splice would be a no-op (inline conditional where `else` and `end` share a line). The collapse loop additionally bounds itself with MAX_COLLAPSE_PASSES and guards against zero-length empty edits. Previously `rewrite` hung indefinitely on inline conditionals whose then branch contained more than one statement. Production-readiness polish (spike scope, for the future migration): - empty_conditional_ranges: drop the mutable default-argument pattern. - statement_separator_offsets: NOTE comment that parenthesis depth is not tracked (Bundler DSL does not produce ambiguous shapes in practice). - collapse_edit_for_conditional: caller-side note explaining the remove_empty_then_branch stub asymmetry. - benchmark.rb: single measurement pass; the totals/ratio table and the per-rewrite cost table now derive from the same timings. - README: document the transitive prism dev dependency on Ruby < 3.3. Spec additions: - Regression specs for both correctness fixes (inline if/then/else with sibling Pro gem; inline conditional whose else becomes empty post-removal, with Timeout to fail loud if the hang regresses). - unless/else collapse spec exercising the UnlessNode#else_clause path. - Defensive spec for bare /regex/ literals in line preceding a stale base. - Trailing-comma test split into RUBY_VERSION-gated tests instead of an either-or assertion that passes regardless of branch. 55 examples, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg-conductor/3313-fix
branch
from
3bb18b8 to
adef418
Compare
Code Review: Spike — Prism-based Gemfile rewriter (#3313)OverviewWell-executed spike. The prototype is viable, the 32-case test matrix is comprehensive, the documentation is thorough, and the recommendation to wait is well-reasoned. Infrastructure changes are minimal and correct. Infrastructure Changes ✅
Core Algorithm ✅The byte-offset splice approach is sound — untouched bytes are preserved by construction, sidestepping an entire class of formatter-induced bugs. The two-phase design (direct edits — collapse pass) is a clean separation of concerns, and the Issues Found1.
|
Code Review — Spike: Prism-based Gemfile rewriter (#3313)OverviewThis PR adds a well-scoped exploratory spike evaluating a Prism AST-based replacement for the text scanner in the Pro migration generator. All spike artifacts live under The recommendation is sound and well-supported. The packaging changes (gemspec + RuboCop exclusion) are correct and minimal. Strengths
IssuesMinor — worth fixing before production promotion1. 2. 3. Top-level Informational — not blocking4. Benchmark's 5. Packaging changesBoth packaging changes are correct:
Bottom lineThe spike meets its stated goals. The decision record is the deliverable, and it's solid. The three issues above (#1–#3) should be addressed before the prototype code is promoted to |
| def apply_edits(source, edits) | ||
| sorted = edits.sort_by { |e| -e[:start_offset] } | ||
| sorted.reduce(source) do |acc, edit| | ||
| splice_source(acc, edit[:start_offset], edit[:end_offset], edit[:replacement]) | ||
| end | ||
| end |
There was a problem hiding this comment.
apply_edits silently tolerates overlapping ranges. Applying two edits that cover overlapping byte ranges (e.g., two react_on_rails gems on a single semicolon-separated line sharing a statement_byte_range) would produce garbled output without any warning.
statement_byte_range prevents this in practice, but a defensive guard here would catch any future regression immediately:
| def apply_edits(source, edits) | |
| sorted = edits.sort_by { |e| -e[:start_offset] } | |
| sorted.reduce(source) do |acc, edit| | |
| splice_source(acc, edit[:start_offset], edit[:end_offset], edit[:replacement]) | |
| end | |
| end | |
| def apply_edits(source, edits) | |
| sorted = edits.sort_by { |e| -e[:start_offset] } | |
| sorted.each_cons(2) do |later, earlier| | |
| raise ArgumentError, "overlapping edits at #{earlier[:start_offset]}..#{later[:end_offset]}" if earlier[:start_offset] < later[:end_offset] | |
| end | |
| sorted.reduce(source) do |acc, edit| | |
| splice_source(acc, edit[:start_offset], edit[:end_offset], edit[:replacement]) | |
| end | |
| end |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adef418894
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start_offset: conditional.location.start_offset, | ||
| end_offset: conditional.location.end_offset, | ||
| replacement: inline_replacement |
There was a problem hiding this comment.
In collapse_replacement_edit, the semicolon path replaces if/unless ... end with the raw gem call text (inline_replacement) whenever a trailing separator exists, but this can break valid expression-form conditionals. For example, arr << if ENV['PRO'] then gem 'react_on_rails_pro' else gem 'react_on_rails' end; gem "rails" rewrites to arr << gem 'react_on_rails_pro'; gem "rails", which is no longer parseable Ruby. This means parseable Gemfiles can become syntactically invalid when a collapsible conditional is used as an expression and followed by a semicolon-separated statement.
Useful? React with 👍 / 👎.
| end | ||
| end | ||
|
|
||
| def has_user_version_pin?(call) |
There was a problem hiding this comment.
Ruby predicate convention (and RuboCop Naming/PredicateName) says methods should not start with has_. This is safe while the file is RuboCop-excluded in the spike, but needs renaming before promotion to lib/.
| def has_user_version_pin?(call) | |
| def user_version_pin?(call) |
(The call site at line 133 would need to be updated to match.)
| # empty-`then` conditional is left in the output. See the stub itself | ||
| # for what production should do. | ||
| if then_empty && has_else_node && !else_empty | ||
| collapse_to_branch(source, node, else_statements) || remove_empty_then_branch(source, node) |
There was a problem hiding this comment.
The remove_empty_then_branch stub always returns nil (see line 629), so this whole branch produces no edit and the conditional is silently left unchanged. That's the correct spike policy, but without reading the stub body it looks like it might actually do something. A short note at the call site would make the intent explicit:
| collapse_to_branch(source, node, else_statements) || remove_empty_then_branch(source, node) | |
| collapse_to_branch(source, node, else_statements) || | |
| remove_empty_then_branch(source, node) # spike stub: always nil — conditional left as-is |
| } | ||
| end | ||
|
|
||
| def remove_empty_then_branch(_source, _node) |
There was a problem hiding this comment.
This stub returning nil is intentional for the spike, but worth flagging for the production cutover: collapse_dead_conditionals will silently stop trying on if X; (empty); else BODY end each run with no user-visible feedback. Production code should return a structured error here rather than nil, so the generator can surface a "please update your Gemfile manually" message when this case is encountered.
| # it as a syntax error and Prism reports a parse failure. Split into two gated | ||
| # tests so each Ruby tightens to a single expected outcome instead of a | ||
| # "does-not-crash" passthrough that hides a real regression. | ||
| if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.3.0") |
There was a problem hiding this comment.
A top-level if/else inside the describe block creates two structurally different test suites — a different total example count and a different assertion — depending on Ruby version. This makes the "32 examples, 0 failures" claim in the README version-dependent and can make CI output confusing across Ruby matrix runs.
Prefer a version guard inside the test using skip:
it "handles trailing comma in parenthesized call" do
src = "gem(\"react_on_rails\",)\n"
if Gem::Version.new(RUBY_VERSION) < Gem::Version.new("3.3.0")
result = rewriter.rewrite(src)
expect(result.parse_failed).to be(true)
expect(result.content).to eq(src)
else
result = rewriter.rewrite(src)
expect(result.parse_failed).to be(false)
expect(result.content).to include('"react_on_rails_pro"')
expect_parseable_ruby(result.content)
end
endThis keeps the example count constant across Ruby versions and makes both branches visible in a single test run.
| # NOTE: tracks string and percent-literal contexts but does not track | ||
| # parenthesis depth. A semicolon inside an unquoted call argument | ||
| # (e.g. `gem "ror" if func(a; b)`) would be misidentified as a | ||
| # statement separator. Bundler's Gemfile DSL does not produce such | ||
| # patterns in practice; revisit if the rewriter moves outside Gemfiles. |
There was a problem hiding this comment.
The NOTE covers parenthesis depth but not regex literal context. A bare /re;gex/ on a semicolon-separated line would have the interior ; misidentified as a statement separator.
This is safe in practice because removable_statement_node returns the full IfNode for postfix-guarded calls, keeping the regex's ; inside the node's byte range and below the statement_end threshold — but that coupling is non-obvious from this method alone. Suggest extending the NOTE:
| # NOTE: tracks string and percent-literal contexts but does not track | |
| # parenthesis depth. A semicolon inside an unquoted call argument | |
| # (e.g. `gem "ror" if func(a; b)`) would be misidentified as a | |
| # statement separator. Bundler's Gemfile DSL does not produce such | |
| # patterns in practice; revisit if the rewriter moves outside Gemfiles. | |
| # NOTE: tracks string and percent-literal contexts but does not track | |
| # parenthesis or regex-literal depth. A semicolon inside a call argument | |
| # (e.g. `gem "ror" if func(a; b)`) would be misidentified as a statement | |
| # separator. Bare regex postfix guards (`gem "ror" if /a;b/.match?(x)`) | |
| # are safe only because `removable_statement_node` returns the enclosing | |
| # IfNode, placing the regex's `;` inside the node's byte range and below | |
| # the `statement_end` threshold. Bundler Gemfile DSL does not produce | |
| # bare-paren patterns; revisit if the rewriter moves outside Gemfiles. |
| on `prism` being present transitively through RuboCop-related dev dependencies | ||
| in this monorepo's bundle — `bundle exec` works today, but a future RuboCop | ||
| upgrade could drop that transitive dep. If a `LoadError` for `prism` appears, | ||
| add `gem "prism"` to `react_on_rails/Gemfile.development_dependencies` (the | ||
| spike is intentionally not promoting `prism` to a production runtime dep until |
There was a problem hiding this comment.
The fix for the fragile transitive dep is already documented here, but it's left for contributors to discover at LoadError time. Consider applying it proactively — add gem "prism" to Gemfile.development_dependencies in the same commit so Ruby < 3.3 contributors can run the spike without detective work.
1 participant
Summary
Spike for #3313: a working Prism-based Gemfile rewriter prototype, a 32-case behavior matrix, a parse+rewrite benchmark vs. the current scanner, and a decision record.
All artifacts live under
react_on_rails/spike/3313_prism_gemfile_rewriter/and the directory is excluded from RuboCop — this is exploratory code, not production, andlib/is untouched (per the issue's "Do not rewrite #3232" non-goal).What the spike found
if ENV["PRO"]; gem_pro; else; gem_base; endcollapses to a singlegem "react_on_rails_pro"declaration instead of leaving an ugly emptyelse.prismas a runtime dependency for the ~30% of users still on Ruby 3.0–3.2 (Prism is bundled with CRuby 3.3+).Recommendation
Keep the current scanner. Revisit when
react_on_railsdrops Ruby 3.0–3.2 support — at that point Prism becomes a free runtime dep and the cutover is a 4–8 hour job using the prototype as the starting point.Full reasoning, behavior matrix, performance numbers, conditional/empty-branch policy, and parse-failure policy in
DECISION.md.Not closing #3313
The issue is a discussion/enhancement asking for a decision record. This PR delivers that decision record. The maintainer can close #3313 once the recommendation is accepted (or push back / pick a different path).
Test plan
bundle exec rspec react_on_rails/spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb— 32 examples, 0 failuresbundle exec ruby react_on_rails/spike/3313_prism_gemfile_rewriter/benchmark.rb— clean output, ratios within expected rangebundle exec rubocop react_on_rails/lib/react_on_rails/pro_migration.rb— RuboCop config change does not break linting of the production code path🤖 Generated with Claude Code
Note
Low Risk
No production migration or lib changes; only spike artifacts plus gem/RuboCop exclusions for non-shipped code.
Overview
Adds an exploratory spike under
react_on_rails/spike/3313_prism_gemfile_rewriter/for issue #3313: a Prism-based Gemfile rewriter prototype, a large behavior-matrix spec, a benchmark vs. the existing text scanner, andDECISION.mdrecommending keep the current scanner until the gem’s Ruby floor is 3.3+ (avoid a runtimeprismdep on 3.0–3.2).Packaging / tooling:
spike/is excluded from the published gem (react_on_rails.gemspec) and from RuboCop (.rubocop.yml).lib/and the shipped Pro migration rewrite are unchanged.Reviewed by Cursor Bugbot for commit adef418. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit