Use ActionText::Editor adapter on Rails 8.2+, fall back to monkey-patching on older Rails#778
Use ActionText::Editor adapter on Rails 8.2+, fall back to monkey-patching on older Rails#778jorgemanrubia wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Lexxy’s Rails integration to use the new ActionText::Editor adapter API on Rails 8.2+ while preserving the existing prepend/monkey-patch approach on Rails 8.0/8.1, and adds CI coverage for both paths.
Changes:
- Add an
ActionText::Editor::LexxyEditoradapter for Rails versions that provideActionText::Editor. - Keep the legacy helper monkey-patching path for Rails 8.0/8.1 when
ActionText::Editoris absent. - Update CI/Gemfile to run a matrix against Rails 8.1 and Rails main, and configure the dummy app to select the Lexxy editor when supported.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/dummy/config/application.rb | Sets config.action_text.editor = :lexxy when the adapter API is available. |
| lib/lexxy/engine.rb | Chooses adapter-vs-legacy integration at load time and registers the editor in Rails 8.2+. |
| lib/action_text/editor/lexxy_editor.rb | Introduces the ActionText editor adapter implementation for Lexxy. |
| Gemfile | Adds an env-controlled Rails dependency to switch between Rails 8.1 and Rails main. |
| Gemfile.lock | Updates lockfile to reflect Rails main and updated dependency set. |
| .github/workflows/ci.yml | Adds a CI matrix leg for Rails 8.1 (legacy path) and Rails main (adapter path). |
| CLAUDE.md | Adds a reference to AGENTS instructions. |
| AGENTS.md | Adds contributor guidance for resolving JS asset merge conflicts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c6a4042 to
eb5e338
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
test/helpers/lexxy/tag_helper_test.rb:8
- Skipping this whole test class when
ActionText::Editoris defined leaves key escaping/serialization behaviors (e.g., preserving HTML entities in thevalueattribute) untested on the Rails 8.2+ adapter path. Instead of skipping, add equivalent assertions for the adapter path (for example by rendering the configuredrich_textarea/editor tag and checking the producedvalue).
setup do
skip "lexxy_rich_textarea_tag is not available when using the ActionText::Editor adapter" if defined?(ActionText::Editor)
end
Gemfile:29
capybara,selenium-webdriver, andcupriteare already declared as development dependencies inlexxy.gemspec. Duplicating them here makes version bumps easy to miss and can cause Bundler resolution conflicts if the constraints diverge—consider keeping these dev deps in only one place (preferably the gemspec, sincegemspecis already used).
gem "capybara", "~> 3.0"
gem "selenium-webdriver", "~> 4.0"
gem "cuprite", "~> 0.17"
lib/lexxy/engine.rb:15
- In the Rails 8.2+ adapter branch,
config.lexxy.override_action_text_defaultsis never set/used, so the documented default behavior (“override Action Text defaults unless opted out”) becomes a no-op on newer Rails. Consider keeping this option effective on Rails 8.2+ (e.g., default it to true and, when enabled, setconfig.action_text.editor = :lexxyand/or ensure the explicitlexxy_*helpers are still available).
if defined?(ActionText::Editor)
# Rails 8.2+: use the first-class editor adapter API
require_relative "../action_text/editor/lexxy_editor"
initializer "lexxy.action_text_editor", before: "action_text.editors" do |app|
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class Editor::LexxyEditor::Tag < Editor::Tag | ||
| def render_in(view_context, ...) | ||
| options[:value] = "<div>#{options[:value]}</div>" if options[:value].present? |
There was a problem hiding this comment.
options[:value] here may be an html_safe string (there are legacy tests that rely on stripping html_safe to ensure attribute-escaping preserves entities like < in code blocks). Before wrapping/assigning, coerce the value to a plain String (removing html_safe) so the rendered value attribute can’t end up unescaped / double-interpreted by the browser.
| options[:value] = "<div>#{options[:value]}</div>" if options[:value].present? | |
| if options[:value].present? | |
| value = options[:value].to_s | |
| options[:value] = "<div>#{value}</div>" | |
| end |
There was a problem hiding this comment.
You're right — removed the <div> wrapper and added to_str to strip html_safe, matching the pattern from #749.
|
I need to get something tweaked in Rails before getting this in place! |
|
The adapter path currently fails the system tests because The fix is in rails/rails#56926:
This PR will be unblocked once that lands in Rails. |
|
Waiting on rails/rails#56926 |
b9143f6 to
3fca757
Compare
903350f to
bb85b2d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class << self | ||
| def supports_editor_adapter? | ||
| defined?(ActionText::Editor) && ActionText::Editor.instance_method(:editor_tag).parameters.assoc(:block) | ||
| end |
There was a problem hiding this comment.
supports_editor_adapter? returns the result of .assoc(:block) (an Array or nil), so callers will get a non-boolean truthy value. Since the method name ends with ?, it should return a strict boolean (e.g., wrap the expression with !! or otherwise normalize to true/false) to avoid surprising behavior if anyone ever compares it to true/false or serializes it.
| end | |
| !!(defined?(ActionText::Editor) && ActionText::Editor.instance_method(:editor_tag).parameters.assoc(:block)) |
There was a problem hiding this comment.
Fixed — added !! to return a strict boolean.
| setup do | ||
| skip "lexxy_rich_textarea_tag is not available when using the ActionText::Editor adapter" if Lexxy.supports_editor_adapter? | ||
| end |
There was a problem hiding this comment.
This setup now skips the entire TagHelper test suite when running on Rails versions that support the ActionText::Editor adapter, which leaves the new adapter-based rendering path without coverage. Consider adding equivalent assertions for the adapter path (e.g., exercising rich_text_area/editor_tag rendering) instead of skipping, so CI actually validates the new integration.
There was a problem hiding this comment.
The adapter path is covered by the Rails system tests running against Rails main. This test file specifically exercises lexxy_rich_textarea_tag, which is a legacy helper that doesn't exist on the adapter path — it's replaced by Rails' own rich_textarea_tag wired to the Lexxy adapter.
| setup do | ||
| skip "lexxy_rich_textarea_tag is not available when using the ActionText::Editor adapter" if Lexxy.supports_editor_adapter? | ||
| end | ||
|
|
There was a problem hiding this comment.
The skip message indicates lexxy_rich_textarea_tag is not available when using the ActionText::Editor adapter, but the installation docs currently describe lexxy_rich_textarea_tag / form.lexxy_rich_text_area as the explicit opt-in helpers when defaults aren't overridden. Either keep these helpers available under the adapter path as well, or update the documentation to reflect the Rails 8.2+ behavior.
| setup do | |
| skip "lexxy_rich_textarea_tag is not available when using the ActionText::Editor adapter" if Lexxy.supports_editor_adapter? | |
| end |
There was a problem hiding this comment.
On the adapter path, the standard Rails rich_textarea_tag / form.rich_text_area helpers render the Lexxy editor automatically — no Lexxy-specific helpers needed. The explicit lexxy_* helpers only exist for the legacy monkey-patch path.
bb85b2d to
c4d1529
Compare
c4d1529 to
cc3a965
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lib/lexxy.rb:13
override_action_text_defaults’s method body is not indented, and the surroundingendalignment makes the block structure hard to follow. Please fix the indentation/alignment so it matches standard Ruby formatting (and avoids RuboCop Layout violations).
def override_action_text_defaults
ActionText::TagHelper.module_eval do
alias_method :rich_textarea_tag, :lexxy_rich_textarea_tag
alias_method :rich_text_area_tag, :lexxy_rich_textarea_tag
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - name: "Rails 8.1 (no adapter)" | ||
| use_legacy_rails: "true" | ||
| - name: "Rails main (with adapter)" | ||
| use_legacy_rails: "" | ||
|
|
||
| env: | ||
| USE_RAILS_WITHOUT_ACTION_TEXT_ADAPTER: ${{ matrix.use_legacy_rails }} | ||
|
|
There was a problem hiding this comment.
The Rails version is now selected via USE_RAILS_WITHOUT_ACTION_TEXT_ADAPTER, but ruby/setup-ruby’s bundler-cache: true will try to restore/save a shared bundle cache. Without a committed Gemfile.lock (now gitignored) this can cause cache collisions between the matrix legs (restoring gems resolved for the other Rails version). Consider salting the Bundler cache key (e.g., cache-version: ${{ matrix.name }}) or otherwise separating the bundle caches per matrix entry.
cc3a965 to
af3e99e
Compare
af3e99e to
891598d
Compare
891598d to
d4048e0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def override_action_text_defaults | ||
| ActionText::TagHelper.module_eval do | ||
| alias_method :rich_textarea_tag, :lexxy_rich_textarea_tag |
There was a problem hiding this comment.
The body of override_action_text_defaults is not indented under the method definition, which makes the control structure hard to read and is likely to trip layout/style linters. Indent the method body so it’s visually scoped to the def.
| # config.eager_load_paths << Rails.root.join("extras") | ||
|
|
||
| config.hosts = [] | ||
| config.action_text.editor = :lexxy if defined?(ActionText::Editor) |
There was a problem hiding this comment.
This config gate uses defined?(ActionText::Editor), but the engine’s behavior is keyed off Lexxy.supports_editor_adapter? (which also checks the editor_tag signature). Using a different predicate here can put the dummy app into an inconsistent state (e.g., sets config.action_text.editor = :lexxy while Lexxy falls back to monkey-patching). Consider switching this condition to Lexxy.supports_editor_adapter? so both paths stay in sync.
| config.action_text.editor = :lexxy if defined?(ActionText::Editor) | |
| config.action_text.editor = :lexxy if Lexxy.supports_editor_adapter? |
| content = rich_text.body | ||
| return "" if content.blank? | ||
|
|
||
| if defined?(ActionText::Editor) |
There was a problem hiding this comment.
This branch checks defined?(ActionText::Editor), but the rest of the integration is keyed off Lexxy.supports_editor_adapter?. To avoid mismatches (where ActionText::Editor exists but Lexxy intentionally falls back), consider using the same Lexxy.supports_editor_adapter? predicate here as well.
| if defined?(ActionText::Editor) | |
| if Lexxy.supports_editor_adapter? |
| gem "capybara", "~> 3.0" | ||
| gem "selenium-webdriver", "~> 4.0" | ||
| gem "cuprite", "~> 0.17" |
There was a problem hiding this comment.
capybara, selenium-webdriver, and cuprite are already declared as development dependencies in lexxy.gemspec. Duplicating them here in the Gemfile increases the chance of constraint drift; consider keeping them in one place (typically the gemspec) and reserving the Gemfile for the Rails source override and app-only dependencies.
| gem "capybara", "~> 3.0" | |
| gem "selenium-webdriver", "~> 4.0" | |
| gem "cuprite", "~> 0.17" |
…y-patching Introduces ActionText::Editor::LexxyEditor for Rails versions that ship the editor adapter API with block-children support (rails/rails#56926). On older Rails the existing prepend-based approach is used unchanged. Detection uses Lexxy.supports_editor_adapter? which checks both that ActionText::Editor exists and that editor_tag accepts a block, ensuring we don't activate the adapter path on Rails versions with a partial API. Also adds a CI matrix to test both paths: Rails 8.1 (monkey-patch) and Rails main (adapter).
d4048e0 to
ae1e8f3
Compare
Summary
ActionText::Editor::LexxyEditorfor Rails 8.2+, leveraging the new first-class editor adapter API from rails/rails#51238 — no monkey-patching needed on new RailsActionText::Editoris not defined, the existing prepend-based approach is used unchanged (fully transparent fallback)defined?(ActionText::Editor)inlib/lexxy/engine.rbCI matrix
The
USE_RAILS_WITHOUT_ACTION_TEXT_ADAPTERenv variable in the Gemfile controls which Rails version is used:USE_RAILS_WITHOUT_ACTION_TEXT_ADAPTER=true) — tests the monkey-patch fallback pathBoth legs run on every PR.