refactor: improve message handling in chat services

enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb

@@ -25,8 +25,8 @@ class Api::V1::Accounts::Captain::AssistantsController < Api::V1::Accounts::Base
 
   def playground
     response = Captain::Llm::AssistantChatService.new(assistant: @assistant).generate_response(
-      message_content_multimodal(params[:message_content]),
-      message_history
+      additional_message: params[:message_content],
+      message_history: message_history
     )
 
     render json: response

enterprise/app/helpers/captain/chat_helper.rb

@@ -18,72 +18,6 @@ module Captain::ChatHelper
     raise e
   end
 
-  def message_content_multimodal(message)
-    # If message has text content, start with that
-    content_parts = []
-
-    if message.content.present?
-      content_parts << {
-        type: 'text',
-        text: message.content
-      }
-    end
-
-    # Add image content if available
-    if message.attachments.any?
-      image_attachments = message.attachments.where(file_type: :image)
-
-      image_attachments.each do |attachment|
-        image_url = get_attachment_url(attachment)
-        next unless image_url.present?
-
-        content_parts << {
-          type: 'image_url',
-          image_url: {
-            url: image_url
-          }
-        }
-      end
-
-      # Handle audio transcriptions
-      audio_transcriptions = extract_audio_transcriptions(message.attachments)
-      if audio_transcriptions.present?
-        content_parts << {
-          type: 'text',
-          text: audio_transcriptions
-        }
-      end
-
-      # Handle other attachment types
-      other_attachments = message.attachments.where.not(file_type: [:image, :audio])
-      if other_attachments.any?
-        content_parts << {
-          type: 'text',
-          text: 'User has shared an attachment'
-        }
-      end
-    end
-
-    # Return just text if no special content, otherwise return array for multimodal
-    if content_parts.length == 1 && content_parts.first[:type] == 'text'
-      content_parts.first[:text]
-    elsif content_parts.any?
-      content_parts
-    else
-      'Message without content'
-    end
-  end
-
-  def get_attachment_url(attachment)
-    if attachment.external_url.present?
-      attachment.external_url
-    elsif attachment.file.attached?
-      # For uploaded files, we need to generate a public URL
-      # This will work if the file is stored in a public cloud storage
-      attachment.file.url if attachment.file.respond_to?(:url)
-    end
-  end
-
   def extract_audio_transcriptions(attachments)
     audio_attachments = attachments.where(file_type: :audio)
     return '' if audio_attachments.blank?

enterprise/app/jobs/captain/conversation/response_builder_job.rb

@@ -15,7 +15,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
       generate_and_process_response
     end
   rescue StandardError => e
-    raise e if e.is_a?(ActiveJob::FileNotFoundError)
+    raise e if e.is_a?(ActiveStorage::FileNotFoundError)
 
     handle_error(e)
   ensure
@@ -27,10 +27,8 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
   delegate :account, :inbox, to: :@conversation
 
   def generate_and_process_response
-    latest_message = @conversation.messages.incoming.last
     @response = Captain::Llm::AssistantChatService.new(assistant: @assistant).generate_response(
-      message_content_multimodal(latest_message),
-      collect_previous_messages
+      message_history: collect_previous_messages
     )
 
     return process_action('handoff') if handoff_requested?
@@ -61,6 +59,65 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
     message.message_type == 'incoming' ? 'user' : 'system'
   end
 
+  def message_content_multimodal(message)
+    parts = []
+
+    parts << text_part(message.content) if message.content.present?
+    parts.concat(attachment_parts(message.attachments)) if message.attachments.any?
+
+    finalize_content_parts(parts)
+  end
+
+  def text_part(text)
+    { type: 'text', text: text }
+  end
+
+  def attachment_parts(attachments)
+    [].tap do |parts|
+      parts.concat(image_parts(attachments.where(file_type: :image)))
+
+      transcription = extract_audio_transcriptions(attachments)
+      parts << text_part(transcription) if transcription.present?
+
+      parts << text_part('User has shared an attachment') if attachments.where.not(file_type: %i[image audio]).exists?
+    end
+  end
+
+  def image_parts(image_attachments)
+    image_attachments.each_with_object([]) do |attachment, parts|
+      url = get_attachment_url(attachment)
+      next if url.blank?
+
+      parts << {
+        type: 'image_url',
+        image_url: { url: url }
+      }
+    end
+  end
+
+  def finalize_content_parts(parts)
+    return 'Message without content' if parts.blank?
+    return parts.first[:text] if single_text_part?(parts)
+
+    parts
+  end
+
+  def single_text_part?(parts)
+    parts.one? && parts.first[:type] == 'text'
+  end
+
+  def get_attachment_url(attachment)
+    return attachment.external_url if attachment.external_url.present?
+
+    return unless attachment.file.attached?
+
+    begin
+      attachment.file_url
+    rescue ActiveStorage::FileNotFoundError
+      nil
+    end
+  end
+
   def handoff_requested?
     @response['response'] == 'conversation_handoff'
   end

enterprise/app/services/captain/llm/assistant_chat_service.rb

@@ -12,9 +12,16 @@ class Captain::Llm::AssistantChatService < Llm::BaseOpenAiService
     register_tools
   end
 
-  def generate_response(input, previous_messages = [], role = 'user')
-    @messages += previous_messages
-    @messages << { role: role, content: input } if input.present?
+  # additional_message: A single message (String) from the user that should be appended to the chat.
+  #                    It can be an empty String or nil when you only want to supply historical messages.
+  # message_history:   An Array of already formatted messages that provide the previous context.
+  # role:              The role for the additional_message (defaults to `user`).
+  #
+  # NOTE: Parameters are provided as keyword arguments to improve clarity and avoid relying on
+  # positional ordering.
+  def generate_response(additional_message: nil, message_history: [], role: 'user')
+    @messages += message_history
+    @messages << { role: role, content: additional_message } if additional_message.present?
     request_chat_completion
   end
 

enterprise/lib/chat_gpt.rb

@@ -8,9 +8,9 @@ class ChatGpt
     @messages = [system_message(context_sections)]
   end
 
-  def generate_response(input, previous_messages = [], role = 'user')
-    @messages += previous_messages
-    @messages << { 'role': role, 'content': input } if input.present?
+  def generate_response(additional_message: nil, message_history: [], role: 'user')
+    @messages += message_history
+    @messages << { 'role': role, 'content': additional_message } if additional_message.present?
 
     response = request_gpt
     JSON.parse(response['choices'][0]['message']['content'].strip)

spec/enterprise/controllers/api/v1/accounts/captain/assistants_controller_spec.rb

@@ -211,8 +211,8 @@ RSpec.describe 'Api::V1::Accounts::Captain::Assistants', type: :request do
 
         expect(response).to have_http_status(:success)
         expect(chat_service).to have_received(:generate_response).with(
-          valid_params[:message_content],
-          valid_params[:message_history]
+          additional_message: valid_params[:message_content],
+          message_history: valid_params[:message_history]
         )
         expect(json_response[:content]).to eq('Assistant response')
       end
@@ -232,8 +232,8 @@ RSpec.describe 'Api::V1::Accounts::Captain::Assistants', type: :request do
 
         expect(response).to have_http_status(:success)
         expect(chat_service).to have_received(:generate_response).with(
-          params_without_history[:message_content],
-          []
+          additional_message: params_without_history[:message_content],
+          message_history: []
         )
       end
     end

spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb

@@ -41,11 +41,14 @@ RSpec.describe Captain::Conversation::ResponseBuilderJob, type: :job do
 
       it 'includes image URL directly in the message content for OpenAI vision analysis' do
         # Expect the generate_response to receive multimodal content with image URL
-        expect(mock_llm_chat_service).to receive(:generate_response) do |content, _history|
-          # Content should be an array for multimodal
-          expect(content).to be_an(Array)
-          expect(content.any? { |part| part[:type] == 'text' && part[:text] == 'Can you help with this error?' }).to be true
-          expect(content.any? { |part| part[:type] == 'image_url' && part[:image_url][:url] == 'https://example.com/error.jpg' }).to be true
+        expect(mock_llm_chat_service).to receive(:generate_response) do |**kwargs|
+          history = kwargs[:message_history]
+          last_entry = history.last
+          expect(last_entry[:content]).to be_an(Array)
+          expect(last_entry[:content].any? { |part| part[:type] == 'text' && part[:text] == 'Can you help with this error?' }).to be true
+          expect(last_entry[:content].any? do |part|
+            part[:type] == 'image_url' && part[:image_url][:url] == 'https://example.com/error.jpg'
+          end).to be true
           { 'response' => 'I can see the error in your image. It appears to be a database connection issue.' }
         end