refactor(aichat): move request controller helpers to chat helpers#70249
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors AI chat request handling by extracting controller helper methods into the AichatAiHelper module for better separation of concerns and reusability. The refactoring consolidates error handling, request building, and message filtering logic into shared helper methods that can be used across both the controller and background jobs.
Key Changes:
- Extracted request building and error handling logic from controller into
AichatAiHelper - Consolidated duplicate error handling code between controller and job
- Added comprehensive test coverage for the newly extracted helper methods
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dashboard/app/helpers/aichat_ai_helper.rb | Added helper methods for error handling, request attribute building, message filtering, and project ID extraction |
| dashboard/app/controllers/aichat_requests_controller.rb | Refactored to use new helper methods and added parameter whitelisting with create_request method |
| dashboard/app/jobs/aichat_request_chat_completion_job.rb | Simplified error handling by delegating to new AichatAiHelper.handle_error method |
| dashboard/test/helpers/aichat_ai_helper_test.rb | Added comprehensive test coverage for all new helper methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5ec65ec to
7bf7065
Compare
7bf7065 to
70baea8
Compare
| # Filter out non-OK messages (e.g. errors). | ||
| Array(stored_messages).select do |message| | ||
| message[:status] == SharedConstants::AI_INTERACTION_STATUS[:OK] && | ||
| message[:chatMessageText].present? |
There was a problem hiding this comment.
@drizco just noting that the existing logic does not check message[:chatMessageText].present?. Could you talk about scenarios that we'd have an OK status but no chatMessageText and why you believe this wasn't filtered before?
There was a problem hiding this comment.
yeah I think they are being filtered on the front end in submitChatContents.ts, this is just a guard since we're filtering already
| end | ||
| end | ||
|
|
||
| def self.project_id_from_channel_id(context) |
There was a problem hiding this comment.
nit: could this be project_id_from_context as we don't pass in a channel id directly?
There was a problem hiding this comment.
I'm just noticing a pr went in yesterday that moves us toward using uuid channel ids rather than encrypted channel ids. will adjust!
| ) | ||
| end | ||
|
|
||
| def self.build_request_attributes(user_id, data) |
There was a problem hiding this comment.
nit: is there a more descriptive name we could use rather than data? It looks like we pass in safe_params so perhaps just params?
| new_message: params[:newMessage], | ||
| level_id: context[:currentLevelId], | ||
| script_id: context[:scriptId], | ||
| project_id: get_project_id(context) |
There was a problem hiding this comment.
looks like get_project_id is no longer used. Any reason to keep around the definition?
| @default_aichat_context = { | ||
| currentLevelId: @level.id, | ||
| scriptId: @script.id, | ||
| channelId: 'test', |
There was a problem hiding this comment.
Curious, was this just never needed?
There was a problem hiding this comment.
oof, I don't remember why I removed this, but I'm adding it back now to handle the uuid vs encrypted ids situation
edcodedotorg
left a comment
There was a problem hiding this comment.
@drizco thanks for refactoring to better share code with your streaming additions! Just a few nits and questions to make sure we're not changing anything unintentionally, but looks good!
70baea8 to
4612ee1
Compare
This pull request refactors and improves the handling of AI chat requests, focusing on centralizing attribute building and error handling logic, as well as adding comprehensive tests for helper methods. The main changes move request attribute construction and error reporting into helper methods, streamline controller logic, and add thorough test coverage for the new and existing helper methods.
Refactoring and Logic Centralization:
AichatRequestattributes out of the controller into a new helper methodAichatAiHelper.build_request_attributes, and replaced inline logic inAichatRequestsControllerwith a call to a newcreate_requestmethod.AichatAiHelper.handle_error.Helper Method Enhancements:
AichatAiHelper.successful_stored_chat_messagesto filter out non-OK or blank chat messages, ensuring only valid messages are stored.AichatAiHelper.project_id_from_contextto reliably extract the project ID from context, supporting both direct and encrypted channel ID sources.Testing Improvements:
aichat_ai_helper_test.rbcovering model selection, message formatting, JSON schema conversion, context/config generation, attribute building, message filtering, and project ID extraction forAichatAiHelper.Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Creation Checklist: