Personal Finance AI #1985

[ActiveJob] [ProcessAiResponseJob] [2909c435-6d15-40fe-b895-1519239cfd38] Error generating AI response: undefined method 'previous_month' for class Period
16:28:49 web.1    | [ActiveJob] [ProcessAiResponseJob] [2909c435-6d15-40fe-b895-1519239cfd38] /Users/zachgoll/Documents/apps/maybe/app/models/ai/financial_assistant.rb:520:in 'Ai::FinancialAssistant#get_period_from_param'

When using the chat, I ran into an application error pretty much any time I asked about IncomeStatement items that had a period: ``` [ActiveJob] [ProcessAiResponseJob] [2909c435-6d15-40fe-b895-1519239cfd38] Error generating AI response: undefined method 'previous_month' for class Period 16:28:49 web.1 | [ActiveJob] [ProcessAiResponseJob] [2909c435-6d15-40fe-b895-1519239cfd38] /Users/zachgoll/Documents/apps/maybe/app/models/ai/financial_assistant.rb:520:in 'Ai::FinancialAssistant#get_period_from_param' ```

app/views/chats/index.html.erb

zachgoll (Migrated from github.com) commented

2025-03-12 04:28:02 +08:00

Should we drop this route / controller / view for the moment?

I ran into several issues here and I'm not sure how critical it is to have. Seems like we could simplify the overall feature by removing this and sticking with the sidebar chat only.

Should we drop this route / controller / view for the moment? I ran into several issues here and I'm not sure how critical it is to have. Seems like we could simplify the overall feature by removing this and sticking with the sidebar chat only.

app/views/layouts/shared/_ai_consent.html.erb

						
				@@ -0,0 +18,4 @@

				  <p class="text-gray-600 mb-6 text-sm">

				    NOTE: During beta testing, we'll be monitoring usage and AI conversations to improve the assistant.

				  </p>

zachgoll (Migrated from github.com) commented

2025-03-12 04:30:03 +08:00

We should probably remove this for self hosted users as we won't have any way of monitoring usage and I wouldn't want any self hosted users getting the wrong impression.

config/routes.rb

						
				@@ -11,0 +14,4 @@

				    member do

				      post :clear

				    end

				  end

zachgoll (Migrated from github.com) commented

2025-03-12 04:52:16 +08:00

Just a suggestion, but as I mentioned above, I feel like for this first pass we could remove the following entirely:

Index page
Clear chat - I think users can just delete / create a new one

Just a suggestion, but as I mentioned above, I feel like for this first pass we could _remove_ the following entirely: - Index page - Clear chat - I think users can just delete / create a new one

db/schema.rb

zachgoll (Migrated from github.com) commented

2025-03-12 03:45:18 +08:00

Since user_id has a clear relationship with family, I think we can remove this family_id FK. If we need to access family chats, I'd just set that up directly through users:

class Family < ApplicationRecord
  has_many :chats, through: :users
end

Since `user_id` has a clear relationship with family, I think we can remove this `family_id` FK. If we need to access family chats, I'd just set that up directly through users: ``` class Family < ApplicationRecord has_many :chats, through: :users end ```

zachgoll (Migrated from github.com) reviewed 2025-03-12 23:35:28 +08:00

app/controllers/chat_controller.js

zachgoll (Migrated from github.com) commented

2025-03-12 22:46:22 +08:00

Is this controller used at all? I think we can delete it?

app/views/chats/index.html.erb

zachgoll (Migrated from github.com) commented

2025-03-12 23:20:48 +08:00

After spending some more time understanding the overall flow, I think this may be one of the bigger areas with an opportunity to simplify and make the overall view organization a bit clearer with Turbo frames.

Right now, we're putting a ton of view logic in the layout, which I think we should consolidate into the app/views/chats directory and leverage Turbo frames. My main concern is that we're loading the chat on every request, and for longer chats, this will incur a performance hit on every page.

I'm imagining the layout could just be consolidated in application.html.erb to:

<%= tag.div class: class_names("py-4 shrink-0 h-full overflow-y-auto transition-all duration-300", right_sidebar_open ? "w-[375px]" : "w-0"), data: { sidebar_target: "rightPanel" } do %>
  <%= render "chats/chat_nav" %>
  
  <%= turbo_frame_tag "chats", src: chats_controller, loading: "lazy" do %>
    <p>Loading chats...</p>
  <% end %>
<% end %>

Then all of the AI related stuff would go in views/chats, and index.html.erb effectively becomes what is the _ai_sidebar.html.erb right now. This will have several benefits:

Makes each chat request async
- When chat is collapsed, request won't be made at all (that way users with this feature hidden won't incur any performance hit from it)
Consolidates the index view into one and aligns better with the domain by putting all the view logic in views/chats/
Keeps our layouts generic and easier to reason about

So in summary, here's the view organization that I'm imagining for the entire feature:

views/layouts/application.html.erb
- Loads chats/chat_nav
- Loads turbo_frame_tag "chats", src: chats_path
views/chats/
- _chat_nav.html.erb - a shared partial, rendered at top of index and show for navigation of chats
- _chat.html.erb - for list view
- _chat_consent.html.erb
- _chat_greeting.html.erb
- _chat_form.html.erb - better aligns with an entire "chat"
- index.html.erb
  - Renders _chat_consent
  - Renders a list of _chat.html.erb partials for user
- show.html.erb
  - Renders _chat_greeting
  - Renders array of messages/_message.html.erb partials
    - Updated by streams
  - Renders _chat_form
views/messages
- _message.html.erb

After spending some more time understanding the overall flow, I think this may be one of the bigger areas with an opportunity to simplify and make the overall view organization a bit clearer with Turbo frames. Right now, we're putting a ton of view logic in the layout, which I think we should consolidate into the `app/views/chats` directory and leverage Turbo frames. My main concern is that we're loading the chat on every request, and for longer chats, this will incur a performance hit on every page. I'm imagining the layout could just be consolidated in `application.html.erb` to: ```erb <%= tag.div class: class_names("py-4 shrink-0 h-full overflow-y-auto transition-all duration-300", right_sidebar_open ? "w-[375px]" : "w-0"), data: { sidebar_target: "rightPanel" } do %> <%= render "chats/chat_nav" %> <%= turbo_frame_tag "chats", src: chats_controller, loading: "lazy" do %> <p>Loading chats...</p> <% end %> <% end %> ``` Then all of the AI related stuff would go in `views/chats`, and `index.html.erb` effectively becomes what is the `_ai_sidebar.html.erb` right now. This will have several benefits: - Makes each chat request async - When chat is collapsed, request won't be made at all (that way users with this feature hidden won't incur any performance hit from it) - Consolidates the `index` view into one and aligns better with the domain by putting all the view logic in `views/chats/` - Keeps our layouts generic and easier to reason about So in summary, here's the view organization that I'm imagining for the entire feature: - `views/layouts/application.html.erb` - Loads `chats/chat_nav` - Loads `turbo_frame_tag "chats", src: chats_path` - `views/chats/` - `_chat_nav.html.erb` - a shared partial, rendered at top of `index` and `show` for navigation of chats - `_chat.html.erb` - for list view - `_chat_consent.html.erb` - `_chat_greeting.html.erb` - `_chat_form.html.erb` - better aligns with an entire "chat" - `index.html.erb` - Renders `_chat_consent` - Renders a list of `_chat.html.erb` partials for user - `show.html.erb` - Renders `_chat_greeting` - Renders array of `messages/_message.html.erb` partials - Updated by streams - Renders `_chat_form` - `views/messages` - `_message.html.erb`

db/schema.rb

zachgoll (Migrated from github.com) commented

2025-03-12 21:22:06 +08:00

Could we remove user_id and internal and represent Message as this?

t.text "content", null: false
t.string "role", null: false
t.uuid "chat_id", null: false
t.boolean "debug_mode", null: false, default: false

While reading through, it was a little hard to understand how role, internal, and user_id were playing together. With the model above, I think it becomes easier to reason about:

"Role" - who the message came from
- user - this message came from a Maybe user
- assistant - this message came from OpenAI as a direct response to a user message
- system - messages used for giving context to OpenAI, but invisible to the user in the UI
  - debug_mode: true - a completely separate and explicit concept indicating that this system message is debug only and shouldn't be shown.

Then in our Message model, the scope used in the view becomes a bit clearer:

# chat.messages.conversation - what user sees
scope :conversation, -> { where(debug_mode: false, role: [ :user, :assistant ]) }

Could we remove `user_id` and `internal` and represent `Message` as this? ```rb t.text "content", null: false t.string "role", null: false t.uuid "chat_id", null: false t.boolean "debug_mode", null: false, default: false ``` While reading through, it was a little hard to understand how `role`, `internal`, and `user_id` were playing together. With the model above, I think it becomes easier to reason about: - "Role" - who the message came from - `user` - this message came from a Maybe user - `assistant` - this message came from OpenAI as a direct response to a user message - `system` - messages used for giving context to OpenAI, but _invisible_ to the user in the UI - `debug_mode: true` - a completely separate and explicit concept indicating that this system message is debug only and shouldn't be shown. Then in our `Message` model, the scope used in the view becomes a bit clearer: ```rb # chat.messages.conversation - what user sees scope :conversation, -> { where(debug_mode: false, role: [ :user, :assistant ]) } ```

test/models/concerns/promptable_test.rb

zachgoll (Migrated from github.com) commented

2025-03-12 21:13:57 +08:00

I'm thinking to start out, this file can be reduced quite a bit as just a simple sanity check on the interface:

require "test_helper"

class PromptableTest < ActiveSupport::TestCase
  test "balance sheet implements promptable interface" do
    family = families(:dylan_family)
    @balance_sheet = BalanceSheet.new(family)

    assert_respond_to @balance_sheet, :to_ai_readable_hash
    assert_respond_to @balance_sheet, :detailed_summary
    assert_respond_to @balance_sheet, :financial_insights
    assert_respond_to @balance_sheet, :to_ai_response
  end

  test "income statement implements promptable interface" do
    family = families(:dylan_family)
    @income_statement = IncomeStatement.new(family)

    assert_respond_to @income_statement, :to_ai_readable_hash
    assert_respond_to @income_statement, :detailed_summary
    assert_respond_to @income_statement, :financial_insights
    assert_respond_to @income_statement, :to_ai_response
  end
end

I'm thinking to start out, this file can be reduced quite a bit as just a simple sanity check on the interface: ```rb require "test_helper" class PromptableTest < ActiveSupport::TestCase test "balance sheet implements promptable interface" do family = families(:dylan_family) @balance_sheet = BalanceSheet.new(family) assert_respond_to @balance_sheet, :to_ai_readable_hash assert_respond_to @balance_sheet, :detailed_summary assert_respond_to @balance_sheet, :financial_insights assert_respond_to @balance_sheet, :to_ai_response end test "income statement implements promptable interface" do family = families(:dylan_family) @income_statement = IncomeStatement.new(family) assert_respond_to @income_statement, :to_ai_readable_hash assert_respond_to @income_statement, :detailed_summary assert_respond_to @income_statement, :financial_insights assert_respond_to @income_statement, :to_ai_response end end ```

zachgoll (Migrated from github.com) reviewed 2025-03-13 00:42:07 +08:00

app/views/chats/index.html.erb

zachgoll (Migrated from github.com) commented

2025-03-13 00:42:07 +08:00

I went ahead and mocked this up to see how it would look. Here's the branch and demo:

https://github.com/maybe-finance/maybe/compare/main...zachgoll/ai-skeleton

AI sidebar uses data-turbo-permanent so it doesn't reload the frames on each navigation
We keep track of the "last viewed chat" for each user
- Load last viewed chat by default
- If user clicks "all chats", it sets the last viewed chat to nil and the next page load will show "all chats" again
- Layout encapsulates the logic that determines whether to show the "current chat" or "all chats"

https://github.com/user-attachments/assets/c08b19c8-9c4e-4f15-83b7-6ab04c912d1e

I went ahead and mocked this up to see how it would look. Here's the branch and demo: https://github.com/maybe-finance/maybe/compare/main...zachgoll/ai-skeleton - AI sidebar uses `data-turbo-permanent` so it doesn't reload the frames on each navigation - We keep track of the "last viewed chat" for each user - Load last viewed chat by default - If user clicks "all chats", it sets the last viewed chat to `nil` and the next page load will show "all chats" again - Layout encapsulates the logic that determines whether to show the "current chat" or "all chats" https://github.com/user-attachments/assets/c08b19c8-9c4e-4f15-83b7-6ab04c912d1e

zachgoll commented

2025-03-24 22:05:36 +08:00

(Migrated from github.com)

#2022 is a direct checkout of this branch with some additional documentation added, so closing this one out and will use #2022 for final merge.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1985