From d49250826b8c0aad7be78664bd12d0eca7d05b22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Jos=C3=A9=20Mata?= Date: Wed, 29 Apr 2026 17:51:06 +0200 Subject: [PATCH] Improve error handling with user-friendly messages and classification (#1591) * Improve chat LLM error messages * Fix chat visibility regression in tests * Harden chat error handling for review feedback * Fix rubocop private method indentation * Fix nil presentable_error_message, i18n strings, bare rescue - Guard `presentable_error_message` with `return nil if error.blank?` so chats with no error return nil instead of the fallback string; this prevents the API serialisers from emitting a spurious error message and stops the mobile polling guard from firing on every successful chat - Move all hardcoded user-facing error strings into config/locales/models/chat/en.yml and reference them via I18n.t() - Replace bare `rescue` in `error_message_for` with `rescue StandardError` to avoid swallowing system-level exceptions - Update tests to reference I18n keys instead of raw strings, and add tests for the nil-error case and the unrecognized-error fallback https://claude.ai/code/session_01YFMjEds5WVyKPL42xBqMCX --------- Co-authored-by: SureBot Co-authored-by: Claude --- app/models/chat.rb | 97 ++++++++++++++++++++-- app/views/api/v1/chats/_chat.json.jbuilder | 2 +- app/views/api/v1/chats/index.json.jbuilder | 2 +- app/views/chats/_error.html.erb | 4 +- config/locales/models/chat/en.yml | 8 ++ mobile/lib/providers/chat_provider.dart | 10 +++ test/models/chat_test.rb | 52 ++++++++++++ 7 files changed, 165 insertions(+), 10 deletions(-) create mode 100644 config/locales/models/chat/en.yml diff --git a/app/models/chat.rb b/app/models/chat.rb index f97fca68b..9345c9916 100644 --- a/app/models/chat.rb +++ b/app/models/chat.rb @@ -1,6 +1,32 @@ class Chat < ApplicationRecord include Debuggable + RATE_LIMIT_PATTERNS = [ + /\b429\b/i, + /rate limit/i, + /too many requests/i, + /quota exceeded/i + ].freeze + + TEMPORARY_PROVIDER_PATTERNS = [ + /\b5\d\d\b/i, + /service unavailable/i, + /temporarily unavailable/i, + /gateway timeout/i, + /bad gateway/i, + /overloaded/i, + /time(?:out|d?\s*out)/i, + /connection reset/i + ].freeze + + AUTH_CONFIGURATION_PATTERNS = [ + /unauthorized/i, + /authentication/i, + /invalid api key/i, + /incorrect api key/i, + /access token/i + ].freeze + belongs_to :user has_one :viewer, class_name: "User", foreign_key: :last_viewed_chat_id, dependent: :nullify # "Last chat user has viewed" @@ -52,17 +78,26 @@ class Chat < ApplicationRecord end def add_error(e) - update! error: e.to_json + update!(error: build_error_payload(e).to_json) broadcast_append target: "messages", partial: "chats/error", locals: { chat: self } end + def presentable_error_message + return nil if error.blank? + parsed_error_payload["message"].presence || classify_error_message(error) + end + + def technical_error_message + parsed_error_payload["technical_message"].presence || parsed_legacy_error_message || error + end + def clear_error update! error: nil broadcast_remove target: "chat-error" end - def assistant - @assistant ||= Assistant.for_chat(self) + def conversation_messages + messages.where(type: [ "UserMessage", "AssistantMessage" ]) end def ask_assistant_later(message) @@ -74,7 +109,57 @@ class Chat < ApplicationRecord assistant.respond_to(message) end - def conversation_messages - messages.where(type: [ "UserMessage", "AssistantMessage" ]) - end + private + + def build_error_payload(error) + technical_message = error_message_for(error) + + { + message: classify_error_message(technical_message), + technical_message: technical_message, + type: error.class.name + } + end + + def classify_error_message(message) + normalized_message = message.to_s.strip + return I18n.t("chat.errors.default") if normalized_message.blank? + + if RATE_LIMIT_PATTERNS.any? { |pattern| normalized_message.match?(pattern) } + I18n.t("chat.errors.rate_limited") + elsif TEMPORARY_PROVIDER_PATTERNS.any? { |pattern| normalized_message.match?(pattern) } + I18n.t("chat.errors.temporarily_unavailable") + elsif AUTH_CONFIGURATION_PATTERNS.any? { |pattern| normalized_message.match?(pattern) } + I18n.t("chat.errors.misconfigured") + else + I18n.t("chat.errors.default") + end + end + + def parsed_error_payload + return {} if error.blank? + return error if error.is_a?(Hash) + + parsed = JSON.parse(error) + parsed.is_a?(Hash) ? parsed : {} + rescue JSON::ParserError, TypeError + {} + end + + def error_message_for(error) + error.respond_to?(:message) ? error.message.to_s : error.to_s + rescue StandardError + "" + end + + def parsed_legacy_error_message + parsed = JSON.parse(error) + parsed.is_a?(String) ? parsed : nil + rescue JSON::ParserError, TypeError + nil + end + + def assistant + @assistant ||= Assistant.for_chat(self) + end end diff --git a/app/views/api/v1/chats/_chat.json.jbuilder b/app/views/api/v1/chats/_chat.json.jbuilder index a1b520ebf..259e2c5f4 100644 --- a/app/views/api/v1/chats/_chat.json.jbuilder +++ b/app/views/api/v1/chats/_chat.json.jbuilder @@ -2,6 +2,6 @@ json.id chat.id json.title chat.title -json.error chat.error.present? ? chat.error : nil +json.error chat.presentable_error_message json.created_at chat.created_at.iso8601 json.updated_at chat.updated_at.iso8601 diff --git a/app/views/api/v1/chats/index.json.jbuilder b/app/views/api/v1/chats/index.json.jbuilder index c251b4161..9f65a069a 100644 --- a/app/views/api/v1/chats/index.json.jbuilder +++ b/app/views/api/v1/chats/index.json.jbuilder @@ -5,7 +5,7 @@ json.chats @chats do |chat| json.title chat.title json.last_message_at chat.messages.ordered.first&.created_at&.iso8601 json.message_count chat.messages.count - json.error chat.error.present? ? chat.error : nil + json.error chat.presentable_error_message json.created_at chat.created_at.iso8601 json.updated_at chat.updated_at.iso8601 end diff --git a/app/views/chats/_error.html.erb b/app/views/chats/_error.html.erb index bbcb75818..4aa20cb70 100644 --- a/app/views/chats/_error.html.erb +++ b/app/views/chats/_error.html.erb @@ -3,12 +3,12 @@
<% if chat.debug_mode? %>
- <%= chat.error %> + <%= chat.technical_error_message %>
<% end %>
-

Failed to generate response. Please try again.

+

<%= chat.presentable_error_message %>

<%= render DS::Button.new( text: "Retry", diff --git a/config/locales/models/chat/en.yml b/config/locales/models/chat/en.yml new file mode 100644 index 000000000..705495c78 --- /dev/null +++ b/config/locales/models/chat/en.yml @@ -0,0 +1,8 @@ +--- +en: + chat: + errors: + rate_limited: "The AI provider is rate limited right now. Please try again in a few minutes." + temporarily_unavailable: "The AI provider is temporarily unavailable right now. Please try again in a few minutes." + misconfigured: "The AI provider is not configured correctly. Please contact your administrator." + default: "Failed to generate a response. Please try again." diff --git a/mobile/lib/providers/chat_provider.dart b/mobile/lib/providers/chat_provider.dart index e0c467f9b..d68caac28 100644 --- a/mobile/lib/providers/chat_provider.dart +++ b/mobile/lib/providers/chat_provider.dart @@ -345,6 +345,16 @@ class ChatProvider with ChangeNotifier { notifyListeners(); } + if (updatedChat.error != null && updatedChat.error!.isNotEmpty) { + if (!shouldUpdate) { + _currentChat = updatedChat; + } + _stopPolling(); + _errorMessage = updatedChat.error; + notifyListeners(); + return; + } + final lastMessage = updatedChat.messages.lastOrNull; if (lastMessage != null && lastMessage.isAssistant) { final newLen = lastMessage.content.length; diff --git a/test/models/chat_test.rb b/test/models/chat_test.rb index 93ddcf43f..73e7cf350 100644 --- a/test/models/chat_test.rb +++ b/test/models/chat_test.rb @@ -60,4 +60,56 @@ class ChatTest < ActiveSupport::TestCase assert_equal "custom-model", chat.messages.first.ai_model end end + + test "returns nil presentable error message when no error is stored" do + chat = chats(:one) + + chat.update!(error: nil) + + assert_nil chat.presentable_error_message + end + + test "surfaces a friendly rate limit error" do + chat = chats(:one) + + chat.add_error(StandardError.new("OpenAI API error 429: rate limit exceeded")) + + assert_equal I18n.t("chat.errors.rate_limited"), chat.presentable_error_message + assert_match "429", chat.technical_error_message + end + + test "surfaces a friendly temporary provider error" do + chat = chats(:one) + + chat.add_error(StandardError.new("OpenAI API error 503: service unavailable")) + + assert_equal I18n.t("chat.errors.temporarily_unavailable"), chat.presentable_error_message + assert_match "503", chat.technical_error_message + end + + test "surfaces a friendly auth configuration error" do + chat = chats(:one) + + chat.add_error(StandardError.new("OpenAI API error: invalid api key")) + + assert_equal I18n.t("chat.errors.misconfigured"), chat.presentable_error_message + assert_match "invalid api key", chat.technical_error_message + end + + test "surfaces a friendly default error for unrecognized errors" do + chat = chats(:one) + + chat.add_error(StandardError.new("something totally unknown happened")) + + assert_equal I18n.t("chat.errors.default"), chat.presentable_error_message + end + + test "falls back to a friendly message for legacy serialized errors" do + chat = chats(:one) + + chat.update!(error: "OpenAI API error 429: rate limit exceeded".to_json) + + assert_equal I18n.t("chat.errors.rate_limited"), chat.presentable_error_message + assert_equal "OpenAI API error 429: rate limit exceeded", chat.technical_error_message + end end