Add support for ActionText rich content

[raghubetina]

Summary

• Add automatic detection and processing of ActionText::RichText content
• Extract text and embedded images from ActionText HTML content
• Preserve structure of content with images between text blocks
• Made ActionText support optional with fallbacks (no hard dependency)

Test plan

• Run the existing test suite: bundle exec rspec
• Test the ActionText-specific tests: bundle exec rspec spec/ai/chat/actiontext_support_spec.rb
• Test with a Rails app that uses ActionText by passing message objects with rich text content

---

Important

Adds optional support for ActionText rich content in AI::Chat, enabling text and image extraction from ActionText::RichText objects.

• Behavior:
  • Adds support for ActionText rich content in AI::Chat, extracting text and images from ActionText::RichText.
  • Preserves content structure with images between text blocks.
  • ActionText support is optional and loaded if available.
• Implementation:
  • Introduces ActionTextSupport module in actiontext_support.rb for processing ActionText content.
  • Updates messages= method in chat.rb to handle ActionText::RichText objects.
• Testing:
  • Adds actiontext_support_spec.rb for testing ActionText content extraction.
  • Includes a test program test_actiontext.rb for manual testing.
• Documentation:
  • Updates README.md to include instructions for using ActionText support.

^{This description was created by for 4bbc595. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 6671c6b in 2 minutes and 56 seconds. Click for details.

• Reviewed 697 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 11 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. README.md:207

• Draft comment:
  Clear examples are added for custom attribute mappings and ActionText support. Verify that the docs remain consistent with runtime behavior.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. lib/ai/chat.rb:293

• Draft comment:
  The messages= method handles multiple content types (plain, array, ActionText). Consider refactoring some of the nested logic into smaller helper methods for clarity.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

3. lib/ai/chat.rb:308

• Draft comment:
  Good use of checking for ActionText::RichText and fallback to plain text. Ensure this branch is tested comprehensively in production.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. lib/ai/chat/actiontext_support.rb:64

• Draft comment:
  Catching exceptions silently in the attachment processing can hide issues. Consider logging the exception at a debug level to aid troubleshooting.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The silent exception handling appears to be an intentional design choice to gracefully handle cases where attachments can't be loaded, treating them as skippable content. Adding logging could help with debugging but might also add noise since these exceptions are expected in normal operation. The comment's suggestion isn't wrong but may not align with the intentional design choice. The code's current behavior might make troubleshooting harder if there are unexpected issues. However, the silent failure could be a deliberate choice for graceful degradation. While debug logging could help troubleshooting, the explicit comment and repeated pattern suggests this is an intentional design choice to handle expected failure cases silently. The comment should be removed as it questions an intentional design choice that is clearly documented in the code comments.

5. spec/ai/chat/actiontext_support_spec.rb:90

• Draft comment:
  Tests using skip conditions for ActionText support are clear. Ensure that the ActionText environment is available when running these tests.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

6. spec/ai/chat/messages_assignment_spec.rb:40

• Draft comment:
  The tests for handling images and multiple images look good. Verify that the expected array structure in message content matches runtime behavior consistently.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

7. README.md:173

• Draft comment:
  Excellent detailed examples for ActionText support. Consider adding a note about version or dependency requirements for ActionText.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

8. lib/ai/chat.rb:12

• Draft comment:
  Optional loading of ActionText support is handled with a rescue. Consider logging the rescue error for easier debugging.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a common pattern for optional dependency loading. The LoadError here is expected and normal when ActionText isn't available - it's not an error condition that needs debugging. Adding logging would create noise in logs for a normal situation. The comment "ActionText support is optional" is sufficient to explain what's happening. Perhaps in some cases developers would want to know why ActionText failed to load, if they expected it to be available. The error details could help diagnose configuration issues. If ActionText was expected to be available but failed to load, this would manifest as feature unavailability that would be obvious elsewhere. Adding noise to logs for an intentionally optional feature doesn't improve debugging. The comment should be deleted. The rescue block is handling an expected case for optional functionality, and adding error logging would create unnecessary noise.

9. lib/ai/chat.rb:289

• Draft comment:
  The messages= method now uses attribute mappings and supports ActionText content. Consider validating that required attributes (like role and content) are present to avoid unexpected nil values.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code already handles nil values gracefully through safe navigation and fallbacks. The messages= method is designed to be flexible with different input types. Adding validation might make the code more rigid without clear benefits. The current approach of graceful handling seems intentional and appropriate. The comment identifies a potential issue with data validation, which could prevent bugs. However, the flexible handling of nil values might be an intentional design choice. The code's current approach of handling nil values gracefully appears to be a deliberate design choice that provides flexibility when working with different input types and structures. The comment should be deleted as it suggests adding validation that may go against the intentionally flexible design of the code.

10. lib/ai/chat.rb:353

• Draft comment:
  configure_attributes works well. You might consider supporting string values for mapping definitions if needed.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

11. spec/ai/chat/messages_assignment_spec.rb:40

• Draft comment:
  The tests for message assignment and custom attribute mappings are comprehensive. Good job covering both hash and ActiveRecord-like objects.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_alDuQtMmSSG8qufx

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

In process_with_nokogiri, errors in processing attachments are silently ignored. Logging these errors might help with troubleshooting in production.

Suggested change

	# Silently continue if attachment can't be loaded
	Rails.logger.error("Failed to load attachment: \\#{e.message}") if defined?(Rails) && Rails.respond_to?(:logger)

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed d592b86 in 2 minutes and 22 seconds. Click for details.

• Reviewed 542 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. test_program/test_actiontext.rb:105

• Draft comment:
  Consider using a robust HTML parser (e.g. Nokogiri) instead of regex splitting for extracting text and images. Using regex (lines 105-112) may be fragile with complex HTML.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a test file that mocks ActionText behavior for testing purposes. The HTML parsing is simple and limited to specific tag patterns. The current regex approach works for the test cases shown. Adding Nokogiri would add complexity and a dependency just for tests. The current approach is good enough for test purposes. The comment has merit - regex for HTML parsing can be fragile. However, this is test code with controlled input, not production code parsing arbitrary HTML. While Nokogiri would be more robust, the added complexity isn't justified for test code that only needs to handle a few specific HTML patterns in a controlled environment. Delete the comment. The current regex approach is sufficient for test code, and adding Nokogiri would be overengineering for this specific use case.

2. test_program/test_actiontext.rb:125

• Draft comment:
  Consider supporting tags outside of tags. Currently, only blocks (lines 125-129) are parsed for images.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

3. test_program/test_actiontext.rb:105

• Draft comment:
  Consider using an HTML parser (e.g., Nokogiri) instead of regex in extract_actiontext_content for more robust HTML extraction.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While Nokogiri is generally better for complex HTML parsing, this is a very specific case with a simple pattern match. The regex is handling a very limited subset of HTML with specific tag patterns. The code is in a test file and is working with mock data. The current approach seems adequate for the use case. Using regex for HTML parsing can be fragile and miss edge cases. Nokogiri would be more robust for general HTML parsing. This is test code working with controlled mock data and very specific tag patterns. The complexity of adding Nokogiri as a dependency outweighs the benefits in this case. The comment suggests a more robust but overly complex solution for a simple test case. The current regex approach is adequate.

4. test_program/test_actiontext.rb:22

• Draft comment:
  Using the name 'system' for handling system messages might conflict with Ruby’s Kernel#system. Consider renaming this method to avoid ambiguity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While there is technically a potential for confusion with Kernel#system, several factors make this less concerning: 1) The context is clearly about chat message roles 2) This is in a test file 3) The method is part of a class-specific implementation 4) Ruby's method resolution would use the instance method before Kernel#system 5) 'system' is a standard term in AI chat contexts for this type of message. The comment raises a valid point about potential namespace conflicts. In a large codebase, having methods with the same names as core Ruby methods could lead to confusion. However, in this specific context, 'system' is a standard term in AI chat implementations, and the method scope makes conflicts unlikely. Renaming would actually reduce clarity by deviating from standard terminology. The comment should be deleted. While technically correct, the suggested change would reduce code clarity by deviating from standard AI chat terminology, and the risk of actual conflicts is minimal.

5. test_program/test_actiontext.rb:507

• Draft comment:
  Local variable 'test_image1' is redefined in Test 10, shadowing the earlier global definition. Consider using a distinct name to avoid confusion.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Variable shadowing can lead to confusion and bugs. However, this is a test file and each test is in its own begin block scope. The global variable is only used in earlier tests, and Test 10 explicitly needs its own local version of these variables. The shadowing doesn't cause any actual problems here. The comment is technically correct - there is variable shadowing. And variable shadowing is generally considered a code smell that should be avoided. However, this is a test file where each test is independent and the shadowing is intentional to keep the test self-contained. The shadowing doesn't cause any actual issues. While technically correct, this comment is overly pedantic for a test file where the shadowing is intentional and harmless. The comment should be removed.

Workflow ID: wflow_uabyzNkgOLW1k5uY

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

The check unless respond_to?(:extract_actiontext_content) may not correctly verify instance method availability. Consider using a check on instance_methods instead.

Suggested change

	unless respond_to?(:extract_actiontext_content)
	unless instance_methods.include?(:extract_actiontext_content)

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 4bbc595 in 57 seconds. Click for details.

• Reviewed 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. lib/ai/chat.rb:7

• Draft comment:
  Using attr_accessor for :messages now instead of attr_reader. Ensure this change doesn't expose unwanted write access and update tests if they expect a read-only interface.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. lib/ai/chat.rb:307

• Draft comment:
  In ActionText branch, using extract_actiontext_content if defined is good. Consider handling cases where to_plain_text might return nil or empty strings, ensuring robust fallback.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. lib/ai/chat.rb:7

• Draft comment:
  Including :messages in the attr_accessor now allows external assignment. Ensure this is intentional and that the custom messages= setter (defined later) is always used for proper processing.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. lib/ai/chat.rb:8

• Draft comment:
  Ensure consistency: by changing the declaration order, the custom setter for messages (defined later) will override the default writer. Confirm that exposing messages as an accessor aligns with intended usage.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_eYXWT3rpDip6NrKz

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}