Reorganize Settings sections + add LLM model/prompt configs #116
Conversation
WalkthroughAdds admin-facing exports to the Imports page with lazy loading and delete; introduces Settings pages for AI Prompts and Guides; reorganizes Settings navigation and labels; changes OpenAI model defaults and exposes prompt instructions; updates FamilyExport attachment purge behavior, routes, views, locales, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Browser
participant ImportsCtrl as ImportsController
participant ImportsView as Imports View
participant TurboFrame as Turbo Frame "family_exports"
participant FamilyExportsCtrl as FamilyExportsController
Admin->>Browser: GET /imports
Browser->>ImportsCtrl: request
ImportsCtrl-->>Browser: renders index with @imports (+ @exports for admins)
Browser->>ImportsView: render page
ImportsView->>TurboFrame: lazy load /family_exports
TurboFrame->>FamilyExportsCtrl: GET /family_exports (index)
FamilyExportsCtrl-->>TurboFrame: renders exports list (statuses, actions)
rect rgba(200,255,200,0.12)
Admin->>Browser: Click "New Export" (modal)
Browser->>FamilyExportsCtrl: GET/POST new/create
FamilyExportsCtrl-->>Browser: redirect to /imports
end
rect rgba(255,230,190,0.12)
Admin->>Browser: Click delete on export
Browser->>FamilyExportsCtrl: DELETE /family_exports/:id
FamilyExportsCtrl-->>Browser: destroy, purge attachment (async), redirect to /imports with notice
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
For the order, i think it would be better to have:
- Profil Info
- Preferences
- Security
- Accounts
- Bank Sync
This order look a lot more like some other settings, with the "User related" settings in first
Also the SETTINGS_ORDER do not seems to be used in the Settings Section Sidebar, i think it would be best to use one var to manage this, for readability
There was a problem hiding this comment.
so also maybe changing the object like:
SETTINGS_ORDER = [
"GENERAL" : [
{ name: "Profile Info", path: :settings_profile_path },
{ name: "Preferences", path: :settings_preferences_path },
{ name: "Accounts", path: :accounts_path },
{ name: "Bank Sync", path: :settings_bank_sync_path },
{ name: "Security", path: :settings_security_path },
],
"TRANSACTIONS" : [
{ name: "Billing", path: :settings_billing_path, condition: :not_self_hosted? },
{ name: "Categories", path: :categories_path },
{ name: "Tags", path: :tags_path },
{ name: "Rules", path: :rules_path },
{ name: "Merchants", path: :family_merchants_path },
],
"ADVANCED" : [
{ name: "AI Prompts", path: :settings_ai_prompts_path },
{ name: "API Key", path: :settings_api_key_path },
{ name: "Self-Hosting", path: :settings_hosting_path, condition: :self_hosted? },
{ name: "Imports", path: :imports_path },
{ name: "SimpleFin", path: :simplefin_items_path },
],
"MORE" : [
{ name: "Guides", path: :settings_guides_path },
{ name: "What's new", path: :changelog_path },
{ name: "Feedback", path: :feedback_path }
]
]
| { name: "API Key", path: :settings_api_key_path }, | ||
| { name: "Self-Hosting", path: :settings_hosting_path, condition: :self_hosted? }, | ||
| { name: "Imports", path: :imports_path }, | ||
| { name: "SimpleFin", path: :simplefin_items_path }, |
There was a problem hiding this comment.
Why having a SimpleFin section as it is already in the Bank Sync?
There was a problem hiding this comment.
The plan is for this to go away indeed, once #104 goes in we can take of it (just wanted to avoid the merge conflict!)
|
For the OpenAI key, why not allow the user to update his key in the |
|
For the |
Yeah. That's a great way to do it and would be easy to implement as-is (feel free to PR it!) Only thing that slowed me down is that is doesn't solve the bigger problem: make model/provider configurable. Even OpenRouter support might be a better solution that gets you both and some observability! |
Yeah, that is my goal. I've had versions that collapse the DIVs but then I realized the design was not consistent with other Settings UI (just added the provider section yesterday to prepare for multiple prompt "families" by provider) One could actually argue that even within a provider the prompts are specific to a model "family" (reasoning model vs. not?) But the complication was starting to grow in my mind (the persistence layer will have to support multiple versions of the prompt and get read at runtime, right?) So anyway, yes ... feel free to work on my branch if you have ideas. :-) The thoughts you had are very similar to the ones I put in the first comment already yesterday as "subtask reminders" for myself. |
|
Thinking of leaving these for a later time:
They are "unrelated" to the task at hand (LLM configs) and the bulk of what I set out to do is done. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/settings/api_keys/show.html.erb (1)
111-126: Plain API key is exposed outside the one-time reveal flowThe current-API-key branch renders
@current_api_key.plain_key, which defeats “show once then mask” and risks leakage (DOM, screenshots, logs). Show the real key only in the newly created flow; otherwise mask or omit.Minimal safe patch to mask the key and example in non-new flows:
- <code id="api-key-display" class="font-mono text-sm text-primary break-all" data-clipboard-target="source"><%= @current_api_key.plain_key %></code> + <code id="api-key-display" class="font-mono text-sm text-primary break-all" data-clipboard-target="source">••••••••••••</code> @@ - curl -H "X-Api-Key: <%= @current_api_key.plain_key %>" <%= request.base_url %>/api/v1/accounts + curl -H "X-Api-Key: <API_KEY>" <%= request.base_url %>/api/v1/accountsFollow-ups:
- If available, replace the placeholder with a masked hint (e.g., last 4 chars) from a helper or model attribute, not the plain key.
- Ensure the controller action sets headers to prevent caching on the reveal page:
# app/controllers/settings/api_keys_controller.rb def show # ... response.headers["Cache-Control"] = "no-store" endI can wire up a
masked_api_key(@current_api_key)helper and locales if you want.Also applies to: 128-134, 136-148
♻️ Duplicate comments (2)
app/helpers/settings_helper.rb (2)
2-25: Model sections explicitly instead of relying on comment blocksIf the UI groups items into “General”, “Transactions”, “Advanced”, and “More”, encode that structure. This reduces drift between the sidebar and footer and makes reordering easier from one source of truth.
Sketch:
SETTINGS_SECTIONS = { "GENERAL" => [ { name_key: "settings.nav.accounts", path: :accounts_path }, # ... ], "TRANSACTIONS" => [ ... ], "ADVANCED" => [ ... ], "MORE" => [ ... ] }Then derive both the sidebar and the flattened sequence (for previous/next) from this constant.
19-20: Avoid duplication: “SimpleFin” appears both under Bank Sync and AdvancedYou acknowledged this earlier will be removed after #104. Keeping both temporarily may confuse users.
If #104 is imminent, consider hiding the Advanced “SimpleFin” item behind the same condition as “Bank Sync” or temporarily removing it to reduce duplication.
🧹 Nitpick comments (42)
app/controllers/settings/api_keys_controller.rb (3)
11-12: Localize breadcrumb label instead of hardcoding "API Key".Use I18n so the breadcrumb aligns with locales (and the recent singularization work).
- [ "API Key", nil ] + [ t("settings.settings_nav.api_key_label"), nil ]
32-39: Wrap temporary revocation + create in a transaction to avoid partial state on failure.Current flow revokes keys with update_column, then manually restores on failure. A transaction lets you avoid the explicit restore and reduces risk from mid-request failures.
- # Temporarily revoke existing keys for validation to pass - existing_keys = Current.user.api_keys.active - existing_keys.each { |key| key.update_column(:revoked_at, Time.current) } - - if @api_key.save - flash[:notice] = "Your API key has been created successfully" - redirect_to settings_api_key_path - else - # Restore existing keys if new key creation failed - existing_keys.each { |key| key.update_column(:revoked_at, nil) } - render :new, status: :unprocessable_entity - end + ActiveRecord::Base.transaction do + # Temporarily revoke existing keys for validation to pass + existing_keys = Current.user.api_keys.active + existing_keys.update_all(revoked_at: Time.current) + + if @api_key.save + flash[:notice] = I18n.t("settings.api_keys.create.success") + redirect_to settings_api_key_path and return + else + # Transaction rollback will revert the temporary revocations + raise ActiveRecord::Rollback + end + end + # Only reached if save failed + flash.now[:alert] = I18n.t("settings.api_keys.create.failure") + render :new, status: :unprocessable_entityOptional: add row locks if you want stronger guarantees under concurrent requests:
ActiveRecord::Base.transaction do existing_keys = Current.user.api_keys.active.lock # SELECT ... FOR UPDATE existing_keys.update_all(revoked_at: Time.current) ... end
33-47: Localize flash messages for consistency with the rest of Settings.Hardcoded English strings bypass your locales and will be inconsistent for non-English users.
- flash[:notice] = "Your API key has been created successfully" + flash[:notice] = I18n.t("settings.api_keys.create.success") ... - flash[:notice] = "API key has been revoked successfully" + flash[:notice] = I18n.t("settings.api_keys.destroy.success") ... - flash[:alert] = "Failed to revoke API key" + flash[:alert] = I18n.t("settings.api_keys.destroy.failure")Follow up: add the corresponding keys under config/locales/views/settings/{en,nb,tr}.yml.
config/locales/views/imports/nb.yml (1)
62-62: Capitalization change to "Ny Import" looks intentional.If you follow Title Case for action buttons across locales, this is consistent. If not, double-check against your Norwegian style guide.
docs/onboarding/guide.md (2)
35-37: Normalize callout syntax: use [!NOTE] (uppercase) for consistencyGitHub-flavored callouts are conventionally uppercase. Using [!NOTE] aligns with [!IMPORTANT] and [!TIP] used elsewhere and avoids renderer quirks.
Apply this diff:
-> [!Note] +> [!NOTE]
39-44: Avoid broken in-app links for Guides until routing is readyThe link to /docs/hosting/plaid.md will likely 404 inside the app’s Guides page since the controller currently renders a single hardcoded doc. Until multiple guides are routed, prefer a stable external URL or mark as WIP without a link.
Option A (stable link to GitHub):
-> - [**Plaid**](/docs/hosting/plaid.md) +> - [**Plaid**](https://github.com/we-promise/sure/blob/main/docs/hosting/plaid.md)Option B: I can help wire up dynamic guide routing (slug-based) and a markdown link rewriter so relative /docs links map to settings/guides/:slug. Want me to draft that?
app/helpers/settings_helper.rb (2)
53-57: Guard against nil in concat to prevent template errorsadjacent_setting can return nil; concat(nil) can be brittle depending on Rails version. Use safe_join with compact to avoid errors and keep markup clean.
Apply this diff:
content_tag :div, class: "hidden md:flex flex-row justify-between gap-4" do - concat(previous_setting) - concat(next_setting) + safe_join([previous_setting, next_setting].compact) end @@ content_tag :div, class: "md:hidden flex flex-col gap-4" do - concat(previous_setting) - concat(next_setting) + safe_join([previous_setting, next_setting].compact) endAlso applies to: 63-67
2-25: Localize navigation labels for i18n consistencyLabels like “Accounts”, “Bank Sync”, “Guides”, “What’s new”, “Feedback” are hardcoded English. Since the app ships with multiple locales, consider moving these to locale files and storing a key instead of a string in SETTINGS_ORDER. Resolve the display text via I18n where rendered.
Example approach:
- Change entries to
{ name_key: "settings.nav.accounts", path: :accounts_path }- In views/helpers, render
t(entry[:name_key])instead ofentry[:name]app/views/users/_user_menu.html.erb (1)
33-33: Confirm landing destination for “Settings”Link now points to accounts_path, which is outside the settings layout. If the intent is to send users into the Settings flow, consider a Settings page (e.g., Profile Info) instead; otherwise, this is fine given the new IA.
Optional tweak (drop unused param unless Accounts uses it):
-<% menu.with_item(variant: "link", text: "Settings", icon: "settings", href: accounts_path(return_to: request.fullpath)) %> +<% menu.with_item(variant: "link", text: "Settings", icon: "settings", href: accounts_path) %>app/controllers/settings/profiles_controller.rb (2)
8-11: Localize breadcrumb labelsHardcoded “Home” and “Profile Info” break i18n. Use translation keys so the breadcrumb follows the selected locale.
Apply this diff:
- @breadcrumbs = [ - [ "Home", root_path ], - [ "Profile Info", nil ] - ] + @breadcrumbs = [ + [ t("breadcrumbs.home"), root_path ], + [ t("settings.profiles.show.breadcrumb"), nil ] + ]You’ll need to add these keys in locale files if they don’t exist yet.
29-35: Localize success and error flash messages in destroyTwo messages are hardcoded in English whereas others already use I18n.
Apply this diff:
- if @user.destroy - # Also destroy the invitation associated with this user for this family - Current.family.invitations.find_by(email: @user.email)&.destroy - flash[:notice] = "Member removed successfully." - else - flash[:alert] = "Failed to remove member." - end + if @user.destroy + # Also destroy the invitation associated with this user for this family + Current.family.invitations.find_by(email: @user.email)&;destroy + flash[:notice] = t("settings.profiles.destroy.member_removed") + else + flash[:alert] = t("settings.profiles.destroy.failed") + endAnd add to locales, for example:
en: settings: profiles: destroy: member_removed: "Member removed successfully." failed: "Failed to remove member."app/views/settings/api_keys/new.html.erb (2)
3-3: Confirm thatsettings_sectionhandlestitle: nilcleanly (no empty header space).If the helper supports a “hide title” flag, prefer that over passing nil; otherwise, consider pulling the title from I18n for consistency with the rest of the API key pages.
Would you like me to switch this to an I18n-backed title (e.g., t("settings.api_keys.new.title"))?
54-57: Align the button label with I18n and the rest of the flow.The UI now says “Save API Key” while locales under settings.api_keys.new still expose “Create API Key”. To avoid drift, wire the button label to I18n and add a dedicated key for “Save API Key”.
Apply this diff here:
- <%= render DS::Button.new( - text: "Save API Key", + <%= render DS::Button.new( + text: t("settings.api_keys.new.save_key"), variant: "primary", type: "submit" ) %>And add the missing locale key in config/locales/views/settings/api_keys/en.yml (see my comment there). Additionally, consider switching the page title to I18n as well:
<%= content_for :page_title, t("settings.api_keys.new.title") %>app/controllers/imports_controller.rb (2)
2-2:include SettingsHelperappears unused in the controller.Helpers are typically for views. Since no helper methods are invoked here, consider removing to keep the controller surface minimal.
Apply this diff:
- include SettingsHelper
17-20: Use I18n for breadcrumb labels.Hardcoded strings make localization harder. Prefer I18n so Settings layout can render localized crumbs.
Apply this diff:
- @breadcrumbs = [ - [ "Home", root_path ], - [ "Import/Export", imports_path ] - ] + @breadcrumbs = [ + [ t("common.home"), root_path ], + [ t("imports.breadcrumbs.index"), imports_path ] + ]Ensure these keys exist (or add them) in the locales:
- common.home
- imports.breadcrumbs.index
app/views/settings/guides/show.html.erb (2)
1-1: Prefer I18n for the page title.Use a locale key to match navigation labels and enable translations.
Apply this diff:
-<%= content_for :page_title, "Guides" %> +<%= content_for :page_title, t("settings.guides.title") %>
3-5: Avoidhtml_safe; use semantic HTML and sanitize Markdown output.Even if the content source is trusted, sanitizing is safer and aligns with best practices. Also prefer semantic
<article>over a generic<div>.Apply this diff:
-<div class="bg-container rounded-xl shadow-border-xs p-4 prose prose-sm max-w-none"> - <%= @guide_content.html_safe %> -</div> +<article class="bg-container rounded-xl shadow-border-xs p-4 prose prose-sm max-w-none"> + <%= sanitize(@guide_content) %> +<article>config/locales/views/settings/api_keys/en.yml (1)
44-61: Add a translation for the “Save API Key” button to avoid hardcoded strings in the view.The view uses “Save API Key”, but the
newsection only has “Create API Key” keys. Add a dedicated key and update the view to use it.Apply this diff:
new: title: "Create API Key" create_new_key: "Create New API Key" description: "Configure your new API key with a descriptive name and appropriate permissions." name_label: "API Key Name" name_placeholder: "e.g., Production App, Analytics Dashboard" name_help: "Choose a descriptive name to help you identify this key's purpose." permissions_label: "Permissions" permissions_help: "Select the permissions your API key needs. You can always create a new key with different permissions." + save_key: "Save API Key" scope_details: read_accounts: "View account information, balances, and account-level data"Then wire it in the view as suggested in my comment on app/views/settings/api_keys/new.html.erb.
config/routes.rb (1)
66-67: Nit: consider singular resource naming for consistencyYou’re using singular resources under settings (e.g., api_key) but pluralized names for singular endpoints: ai_prompts and guides. Not functionally wrong, but renaming to ai_prompt and guide would align with Rails conventions and existing settings resources.
Apply if desired:
- resource :ai_prompts, only: :show - resource :guides, only: :show + resource :ai_prompt, only: :show + resource :guide, only: :showNote: This changes route helpers (settings_ai_prompt_path, settings_guide_path) and requires updating nav, controllers, and tests accordingly.
app/controllers/settings/ai_prompts_controller.rb (1)
4-11: Breadcrumbs and Current usage are fine; consider trimming unused ivars@family is set but not referenced by the view (per the AI summary). If unused, remove to keep controller lean.
- @family = Current.familyapp/controllers/settings/guides_controller.rb (1)
9-17: Handle missing guide file and reuse the existing markdown helper
- File.read will 500 if docs/onboarding/guide.md is missing.
- You already have ApplicationHelper#markdown with safer defaults (hard_wrap, link rel attributes, etc.). Reuse it to avoid duplicating renderer config and to keep link attributes consistent.
Apply this diff:
- markdown = Redcarpet::Markdown.new(Redcarpet::Render::HTML, - autolink: true, - tables: true, - fenced_code_blocks: true, - strikethrough: true, - superscript: true - ) - @guide_content = markdown.render(File.read(Rails.root.join("docs/onboarding/guide.md"))) + source_path = Rails.root.join("docs/onboarding/guide.md") + begin + raw = File.read(source_path) + rescue Errno::ENOENT + raw = "# Guides\n\nThe onboarding guide is missing at #{source_path}." + end + @guide_content = helpers.markdown(raw)app/controllers/family_exports_controller.rb (1)
36-39: Consider guarding deletion + add Turbo Stream response for smoother UXTwo small improvements:
- Prevent deleting in-progress exports (can avoid job errors/orphans).
- Support Turbo Stream redirect to match create/index patterns.
Proposed change:
def destroy - @export.destroy - redirect_to imports_path, notice: "Export deleted successfully" + if @export.status != "completed" + return redirect_to imports_path, alert: "Export can only be deleted after completion" + end + + @export.destroy + respond_to do |format| + format.html { redirect_to imports_path, notice: "Export deleted successfully" } + format.turbo_stream { stream_redirect_to imports_path, notice: "Export deleted successfully" } + end endtest/controllers/family_exports_controller_test.rb (1)
74-84: Happy path delete covered; consider adding a guard-path test if you enforce “completed-only” deleteIf you adopt the controller guard to block deleting non-completed exports, add a test asserting no deletion and a suitable alert for processing/pending exports.
app/models/provider/openai/auto_merchant_detector.rb (2)
2-3: DEFAULT_MODEL introduced — good; optionally centralize or document per-concept defaultsThe local DEFAULT_MODEL fallback is reasonable. If you intend different defaults per concept (categorizer vs merchant detector), keep as-is; otherwise consider centralizing under Provider::Openai to avoid drift.
31-70: Tighten JSON schema to enforce “1 result per transaction” and URL shapeThe instructions ask for 1 result per transaction and apex domains. Add schema constraints to reduce LLM latitude:
- merchants: enforce minItems/maxItems = transactions.size and uniqueItems: true.
- business_url: add a basic domain pattern (still allow null).
Example changes (outside this hunk, in json_schema):
def json_schema { type: "object", properties: { merchants: { type: "array", description: "An array of auto-detected merchant businesses for each transaction", minItems: transactions.size, maxItems: transactions.size, uniqueItems: true, items: { type: "object", properties: { transaction_id: { type: "string", description: "The internal ID of the original transaction", enum: transactions.map { |t| t[:id] } }, business_name: { type: ["string", "null"], description: "The detected business name of the transaction, or `null` if uncertain" }, business_url: { type: ["string", "null"], description: "The URL of the detected business (apex domain only), or `null` if uncertain", pattern: '^[a-z0-9.-]+\.[a-z]{2,}$' } }, required: ["transaction_id", "business_name", "business_url"], additionalProperties: false } } }, required: ["merchants"], additionalProperties: false } endOptionally, post-normalize the URL to strip protocol and “www.” to reduce reliance on prompt adherence:
def normalize_domain(value) return nil if value.nil? host = value.to_s.downcase.strip.sub(%r{\Ahttps?://}i, "").split("/").first host&.sub(/\Awww\./, "") endapp/models/provider/openai.rb (2)
17-17: Deferring model default to helpers is fine; log the resolved model for observabilityRight now, logs will contain the raw param (possibly empty string). Consider resolving the model here for logging parity with what the helper actually used:
Example (outside this line):
def auto_categorize(transactions: [], user_categories: [], model: "") with_provider_response do raise Error, "Too many transactions to auto-categorize. Max is 25 per request." if transactions.size > 25 selected_model = model.presence || AutoCategorizer::DEFAULT_MODEL result = AutoCategorizer.new( client, model: selected_model, transactions: transactions, user_categories: user_categories ).auto_categorize log_langfuse_generation( name: "auto_categorize", model: selected_model, input: { transactions: transactions, user_categories: user_categories }, output: result.map(&:to_h) ) result end end
39-39: Same for merchants: resolve-and-log the effective modelMirror the pattern for auto_detect_merchants to keep logs accurate:
def auto_detect_merchants(transactions: [], user_merchants: [], model: "") with_provider_response do raise Error, "Too many transactions to auto-detect merchants. Max is 25 per request." if transactions.size > 25 selected_model = model.presence || AutoMerchantDetector::DEFAULT_MODEL result = AutoMerchantDetector.new( client, model: selected_model, transactions: transactions, user_merchants: user_merchants ).auto_detect_merchants log_langfuse_generation( name: "auto_detect_merchants", model: selected_model, input: { transactions: transactions, user_merchants: user_merchants }, output: result.map(&:to_h) ) result end endtest/system/settings_test.rb (1)
31-35: Make path assertions resilient to query paramsSome settings pages may append query params (e.g., filter or pagination), which can cause flaky failures. Recommend ignoring query params for all iterations.
Apply this diff:
- assert_current_path path + assert_current_path path, ignore_query: trueapp/views/settings/_settings_nav.html.erb (1)
23-32: Localize “SimpleFin” label for consistency with other itemsAll other labels use translations; “SimpleFin” is a hardcoded string. Use a locale key for consistency and i18n.
Apply this diff:
- { label: "SimpleFin", path: simplefin_items_path, icon: "building-2" } + { label: t(".simplefin_label"), path: simplefin_items_path, icon: "building-2" }Follow-up: add settings.settings_nav.simplefin_label to en/nb/tr locale files.
I can add the locale keys across locales if you want.
app/views/imports/_import.html.erb (4)
43-50: Revert button icon color mismatches the button colorThe button uses orange text, but the icon is forced to destructive (red). Make the icon inherit the parent color for consistency.
Apply this diff:
- <%= icon "rotate-ccw", class: "w-5 h-5 text-destructive" %> + <%= icon "rotate-ccw", class: "w-5 h-5", color: "current" %>
56-63: Avoid duplicating destructive color on icon; inherit from parentThe delete button sets both the parent and the icon to destructive. Inherit color from the parent for cleaner styling.
Apply this diff:
- <%= icon "trash-2", class: "w-5 h-5 text-destructive" %> + <%= icon "trash-2", class: "w-5 h-5", color: "current" %>
43-50: Add accessible labels to icon-only buttonsIcon-only controls should expose aria-labels for screen readers.
Apply this diff:
- <%= button_to revert_import_path(import), + <%= button_to revert_import_path(import), method: :put, - class: "flex items-center gap-2 text-orange-500 hover:text-orange-600", + class: "flex items-center gap-2 text-orange-500 hover:text-orange-600", + aria: { label: t(".revert_action_label", default: "Revert import") }, data: { turbo_confirm: "This will delete transactions that were imported, but you will still be able to review and re-import your data at any time." } do %>And for the delete and view controls:
- <%= button_to import_path(import), + <%= button_to import_path(import), method: :delete, class: "flex items-center gap-2 text-destructive hover:text-destructive-hover", + aria: { label: t(".delete_action_label", default: "Delete import") }, data: { turbo_confirm: CustomConfirm.for_resource_deletion("import") } do %> ... - <%= link_to import_path(import), + <%= link_to import_path(import), class: "flex items-center gap-2 text-primary hover:text-primary-hover" do %> + <span class="sr-only"><%= t(".view_action_label", default: "View import") %></span>Follow-up: add translation keys if desired instead of defaults.
46-48: Prefer localized confirm message over hardcoded copyUse i18n for the confirm text to enable translation and easier copy updates.
Apply this diff:
- data: { - turbo_confirm: "This will delete transactions that were imported, but you will still be able to review and re-import your data at any time." - } do %> + data: { + turbo_confirm: t(".revert_confirm", default: "This will delete transactions that were imported, but you will still be able to review and re-import your data at any time.") + } do %>I can add the locale key to the appropriate en.yml if you want.
config/locales/views/settings/en.yml (1)
90-90: Standardize on api_keys_label for the API Key labelThe only code reference is the plural key in
- app/views/settings/_settings_nav.html.erb:27 (
t(".api_keys_label"))The singular
api_key_labelisn’t used in any view calls—only present in locale files—so let’s consolidate to avoid drift:• Remove
api_key_labelfrom all locales:
- config/locales/views/settings/en.yml (line 90)
- config/locales/views/settings/tr.yml (line 70)
- config/locales/views/settings/nb.yml (line 72)
• Add
api_keys_labeltranslations in tr.yml and nb.yml matching the en.yml value, e.g.:api_keys_label: API Anahtarı # tr.yml api_keys_label: API-nøkkel # nb.ymlI can follow up with a cleanup PR to apply these changes and update other locales.
app/models/provider/openai/auto_categorizer.rb (1)
55-64: Minor: place constants at the top for discoverabilityAutoCategorization alias nested under private is fine functionally (Ruby ignores private for constants), but moving constants (DEFAULT_MODEL, AutoCategorization) together near the top enhances readability.
I can follow up with a small refactor if you prefer.
app/views/family_exports/_list.html.erb (3)
12-14: Localize date/time instead of hardcoded strftimeUse Rails’
l(...)(I18n) so dates respect locale and time zone.- <p class="text-sm font-medium text-primary">Export from <%= export.created_at.strftime("%B %d, %Y at %I:%M %p") %></p> + <p class="text-sm font-medium text-primary"> + <%= t(".export_from", date: l(export.created_at, format: :long)) %> + </p>Follow-up: Add
export_from:toconfig/locales/views/family_exports/*.yml, e.g.export_from: "Export from %{date}".
31-35: Externalize “Exporting…” to I18nKeep strings consistent with the rest of the view.
- <span class="text-sm">Exporting...</span> + <span class="text-sm"><%= t(".exporting") %></span>Follow-up: Add
exporting:toconfig/locales/views/family_exports/*.yml.
42-44: Confirm messages should use I18nHardcoded English confirm messages break localization.
- turbo_confirm: "Are you sure you want to delete this export? This action cannot be undone.", + turbo_confirm: t(".confirm_delete_completed"), @@ - turbo_confirm: "Are you sure you want to delete this failed export?", + turbo_confirm: t(".confirm_delete_failed"),Follow-up: Add
confirm_delete_completedandconfirm_delete_failedlocale keys.Also applies to: 64-66
app/views/imports/index.html.erb (1)
24-28: Improve accessibility for the loading spinnerAdd
aria-liveand an SR-only label for screen reader users while the frame lazy-loads.- <%= turbo_frame_tag "family_exports", src: family_exports_path, loading: :lazy do %> - <div class="mt-4 text-center text-secondary py-8"> + <%= turbo_frame_tag "family_exports", src: family_exports_path, loading: :lazy do %> + <div class="mt-4 text-center text-secondary py-8" aria-live="polite" aria-busy="true"> <div class="animate-spin inline-block h-4 w-4 border-2 border-secondary border-t-transparent rounded-full"></div> + <span class="sr-only"><%= t(".loading_exports") %></span> </div> <% end %>Follow-up: Add
loading_exports:to the imports locale file.app/views/settings/api_keys/show.html.erb (1)
3-4: Localize static strings (headers, labels, button texts)Several UI strings are hardcoded. Move them to I18n to match the rest of Settings and existing tests.
Illustrative changes:
- <h1 class="text-primary text-xl font-medium">API Key Created Successfully</h1> + <h1 class="text-primary text-xl font-medium"><%= t(".created.title") %></h1> @@ - <h1 class="text-primary text-xl font-medium">Your API Key</h1> + <h1 class="text-primary text-xl font-medium"><%= t(".current.title") %></h1>Apply similarly for:
- “Copy API Key”, “How to use your API key”, “Continue to API Key Settings”
- “Create New Key”, “Permissions”, “Active”, “Revoke Key”
- “API Key”, “Create API Key”, feature bullet points
I can generate a full diff plus en/nb/tr locale entries if helpful.
Also applies to: 59-66, 90-93, 96-107, 111-126, 128-134, 136-146, 151-157, 175-196
app/views/settings/ai_prompts/show.html.erb (2)
14-16: Replace raw color class with design tokenUse functional tokens (from maybe-design-system.css) instead of raw
bg-gray-25per guidelines.- <div class="w-9 h-9 rounded-full bg-gray-25 flex justify-center items-center"> + <div class="w-9 h-9 rounded-full bg-container-inset flex justify-center items-center">Apply to all three section icons.
Also applies to: 42-44, 70-72
29-33: Keep domain logic out of views; avoid instantiating service objects in ERB
- Rendering constants and creating provider instances in the view couples presentation to provider internals and risks side effects/boot costs.
- Push these into the controller (or a helper), and pass plain data to the view.
Example controller setup:
# app/controllers/settings/ai_prompts_controller.rb def show @assistant_config = Assistant.config_for(OpenStruct.new(user: Current.user)) @available_models = Provider::Openai::MODELS @categorizer_model = Provider::Openai::AutoCategorizer::DEFAULT_MODEL @merchant_model = Provider::Openai::AutoMerchantDetector::DEFAULT_MODEL @categorizer_instructions = @assistant_config[:auto_categorizer]&.instructions || Provider::Openai::AutoCategorizer.new(nil).instructions @merchant_instructions = @assistant_config[:auto_merchant]&.instructions || Provider::Openai::AutoMerchantDetector.new(nil, model: "", transactions: [], user_merchants: []).instructions endThen in the view, reference
@available_models,@categorizer_model, and the precomputed instruction strings.Also applies to: 57-60, 85-88
| def destroy | ||
| @export.destroy | ||
| redirect_to imports_path, notice: "Export deleted successfully" | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify attachments are purged on destroy (avoid orphaned blobs/storage costs)
If FamilyExport has an attached export_file, ensure destroying the record purges the blob (via has_one_attached :export_file, dependent: :purge_later or a destroy callback). If not configured, the blob can linger.
Run this to check and confirm the model setup:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate FamilyExport model and inspect attachment + destroy callbacks
fd -a -t f 'family_export.rb' | while read -r file; do
echo "==> Inspecting $file"
rg -n -C3 'has_one_attached\s+:export_file' "$file" || true
rg -n -C2 'after_destroy|before_destroy|purge_later|purge' "$file" || true
doneLength of output: 570
Ensure export_file attachments are purged on destroy
Active Storage won’t automatically delete blobs unless you configure it. Right now, in app/models/family_export.rb you have:
has_one_attached :export_fileWithout a dependent: option or a callback, deleting a FamilyExport record will leave its blob orphaned, incurring ongoing storage costs. Please update as follows:
• In app/models/family_export.rb
- has_one_attached :export_file
+ has_one_attached :export_file, dependent: :purge_later• Alternatively, if you prefer callbacks:
after_destroy_commit :purge_export_file
private
def purge_export_file
export_file.purge_later
endThis ensures any attached blob is enqueued for deletion when the record is destroyed.
🤖 Prompt for AI Agents
In app/controllers/family_exports_controller.rb around lines 36-39: the destroy
action deletes the FamilyExport record but leaves its Active Storage blob
orphaned; update app/models/family_export.rb to ensure attachments are purged on
destroy by either adding the dependent option to the attachment (use
has_one_attached :export_file, dependent: :purge_later) or adding an
after_destroy_commit callback that calls export_file.purge_later (or purge) in a
private method so the blob is enqueued for deletion when the record is
destroyed.
| [ "AI Prompts", nil ] | ||
| ] | ||
| @family = Current.family | ||
| @assistant_config = Assistant.config_for(OpenStruct.new(user: Current.user)) |
There was a problem hiding this comment.
Missing require for OpenStruct may raise NameError in production
OpenStruct is in Ruby’s stdlib but not auto-required. Without require "ostruct", constants may be unavailable depending on boot order.
Apply this diff:
+require "ostruct"
class Settings::AiPromptsController < ApplicationController
layout "settings"Alternatively (no stdlib dependency):
- @assistant_config = Assistant.config_for(OpenStruct.new(user: Current.user))
+ ChatContext = Struct.new(:user)
+ @assistant_config = Assistant.config_for(ChatContext.new(Current.user))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @assistant_config = Assistant.config_for(OpenStruct.new(user: Current.user)) | |
| require "ostruct" | |
| class Settings::AiPromptsController < ApplicationController | |
| layout "settings" | |
| def show | |
| @assistant_config = Assistant.config_for(OpenStruct.new(user: Current.user)) | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/controllers/settings/ai_prompts_controller.rb around line 10, the code
uses OpenStruct but does not require the stdlib, which can raise NameError in
some boot orders; either add require "ostruct" at the top of the file (or in an
initializer) so OpenStruct is always loaded, or refactor to avoid the stdlib
dependency (e.g., replace OpenStruct.new(user: Current.user) with a simple PORO
or Struct.new(:user).new(Current.user)) to ensure the constant is available in
production.
| end | ||
|
|
||
| def auto_categorize(transactions: [], user_categories: [], model: "gpt-4.1-mini") | ||
| def auto_categorize(transactions: [], user_categories: [], model: "") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
MODELS vs DEFAULT_MODEL mismatch — ensure supported models include the default
UI now shows DEFAULT_MODEL = "gpt-4.1-mini" at the concept level, but Provider::Openai::MODELS only lists "gpt-4.1". This can confuse users and invalidate supports_model? checks.
- Either add "gpt-4.1-mini" to MODELS, or clarify that MODELS is “selectable” while concept defaults may diverge.
Quick check + follow-up:
Also applies to: 39-39
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where MODELS is referenced and if 'mini' is included anywhere
rg -n -C2 '\bMODELS\b|gpt-4\.1-mini|supports_model\?' --type rb
# Show where DEFAULT_MODEL is used in views to understand UX impact
fd -a -t f 'show.html.erb' | xargs rg -n -C2 'DEFAULT_MODEL|Provider::Openai::MODELS' || trueLength of output: 112
I’ll retry the search more broadly to locate the constants and their usage:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show definition of MODELS in the OpenAI provider
rg -n 'MODELS\s*=' --glob 'app/models/provider/openai.rb'
# Show definition of DEFAULT_MODEL anywhere in the repo
rg -n 'DEFAULT_MODEL\s*=' .
# Find any literal “gpt-4.1-mini” mentions
rg -n 'gpt-4\.1-mini' .
# Look in all view templates for DEFAULT_MODEL or Provider::Openai::MODELS
rg -n -C2 'DEFAULT_MODEL|Provider::Openai::MODELS' --glob '*.html.erb'Length of output: 3444
Ensure “gpt-4.1-mini” is included in the supported MODELS
The defaults in your auto-categorizer and auto-merchant detector classes use DEFAULT_MODEL = "gpt-4.1-mini", but in app/models/provider/openai.rb:
# line 7
MODELS = %w[gpt-4.1]This mismatch will:
- Omit “gpt-4.1-mini” from the UI’s selectable list (
Provider::Openai::MODELS.join(", ")inapp/views/settings/ai_prompts/show.html.erblines 29, 57, and 85). - Cause
supports_model?to reject the default variant.
Actions:
- Update the MODELS constant to include the mini variant:
# app/models/provider/openai.rb - MODELS = %w[gpt-4.1] + MODELS = %w[gpt-4.1 gpt-4.1-mini]
- If the selector list must remain distinct, add a comment/docs to clarify that defaults may diverge from “selectable” models.
🤖 Prompt for AI Agents
In app/models/provider/openai.rb around line 17, the MODELS constant currently
lacks "gpt-4.1-mini" causing the default model to be unsupported and missing
from UI selectors; update the MODELS array to include "gpt-4.1-mini" (e.g., add
it alongside "gpt-4.1"), and optionally add a brief comment noting that defaults
(used by auto-categorizer/auto-merchant) may include variants not shown in the
selectable list so maintainers know the divergence.
| settings: | ||
| ai_prompts: | ||
| show: | ||
| page_title: AI Prompts | ||
| openai_label: OpenAI | ||
| prompt_instructions: Prompt Instructions | ||
| main_system_prompt: | ||
| title: Main System Prompt | ||
| subtitle: Core instructions that define how the AI assistant behaves in all chat conversations | ||
| transaction_categorizer: | ||
| title: Transaction Categorizer | ||
| subtitle: AI automatically categorizes your transactions based on your defined categories | ||
| merchant_detector: | ||
| title: Merchant Detector | ||
| subtitle: AI identifies and enriches transaction data with merchant information | ||
| billings: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure nb/tr locales include the new/renamed keys
To avoid missing translation warnings, verify parity for:
- settings.ai_prompts.*
- settings.settings_nav.advanced_section_title, ai_prompts_label, guides_label, imports_label, profile_label/api_key(s)_label
- profiles.show.page_title/profile_title renames
Also applies to: 86-113
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
pip install --quiet pyyaml > /dev/null
python - <<'PY'
import os, sys, yaml, glob
def flatten(prefix, obj):
out = {}
if isinstance(obj, dict):
for k, v in obj.items():
out.update(flatten(f"{prefix}.{k}" if prefix else k, v))
else:
out[prefix] = True
return out
def load_yml(path):
with open(path, 'r', encoding='utf-8') as f:
return yaml.safe_load(f)
en_file = "config/locales/views/settings/en.yml"
if not os.path.exists(en_file):
print("en.yml not found at config/locales/views/settings/en.yml", file=sys.stderr)
sys.exit(1)
en = load_yml(en_file) or {}
en_keys = set(k for k in flatten("", en).keys() if k.startswith("en.settings."))
locale_files = glob.glob("config/locales/views/settings/*.yml") + glob.glob("config/locales/views/settings/**/*.yml")
missing = {}
for lf in locale_files:
if lf.endswith("/en.yml"): continue
data = load_yml(lf) or {}
if not isinstance(data, dict): continue
root = next(iter(data.keys()), None)
if not root or root == "en": continue
keys = set(k for k in flatten("", data).keys() if k.startswith(f"{root}.settings."))
missing_keys = sorted([k.replace("en.", f"{root}.") for k in en_keys if k.replace("en.", f"{root}.") not in keys])
if missing_keys:
missing[lf] = missing_keys
if missing:
print("Missing translation keys per file:")
for lf, keys in missing.items():
print(f"\n{lf}")
for k in keys:
print(f" - {k}")
else:
print("All locales under config/locales/views/settings/ contain the settings.* keys present in en.yml.")
PYLength of output: 27568
Add missing translation keys for AI Prompts and Settings Navigation in nb/tr locales
The parity check shows that the following locale files are missing the new keys introduced under settings.* in config/locales/views/settings/en.yml:
- config/locales/views/settings/nb.yml
- config/locales/views/settings/tr.yml
- config/locales/views/settings/api_keys/nb.yml
- config/locales/views/settings/api_keys/tr.yml
- config/locales/views/settings/hostings/nb.yml
- config/locales/views/settings/hostings/tr.yml
- config/locales/views/settings/securities/nb.yml
- config/locales/views/settings/securities/tr.yml
Please add the missing key groups to each file:
-
settings.ai_prompts.show:
- page_title
- openai_label
- prompt_instructions
- main_system_prompt.title
- main_system_prompt.subtitle
- transaction_categorizer.title
- transaction_categorizer.subtitle
- merchant_detector.title
- merchant_detector.subtitle
-
settings.settings_nav:
- advanced_section_title
- ai_prompts_label
- api_key_label / api_keys_label
- guides_label
- imports_label
- bank_sync_label
-
settings.preferences.show:
- default_account_order
-
profiles.show renames:
- page_title → profile_title
Ensuring these keys exist in nb and tr locales will prevent missing translation warnings.
🤖 Prompt for AI Agents
In config/locales/views/settings/en.yml around lines 3 to 18, new translation
groups under settings.ai_prompts.show and new settings keys were added;
replicate these exact key structures into the following locale files:
config/locales/views/settings/nb.yml, config/locales/views/settings/tr.yml,
config/locales/views/settings/api_keys/nb.yml,
config/locales/views/settings/api_keys/tr.yml,
config/locales/views/settings/hostings/nb.yml,
config/locales/views/settings/hostings/tr.yml,
config/locales/views/settings/securities/nb.yml, and
config/locales/views/settings/securities/tr.yml by adding
settings.ai_prompts.show with page_title, openai_label, prompt_instructions,
main_system_prompt.title and .subtitle, transaction_categorizer.title and
.subtitle, and merchant_detector.title and .subtitle; also add
settings.settings_nav with advanced_section_title, ai_prompts_label,
api_key_label (and api_keys_label where appropriate), guides_label,
imports_label, and bank_sync_label; add
settings.preferences.show.default_account_order; and in profiles.show rename
page_title to profile_title (remove or replace page_title) so the nb and tr
files mirror the en keys and prevent missing translation warnings.
| Once Sure is installed, open a browser and navigate to [localhost:3000](http://localhost:3000/sessions/new).<br /> | ||
| You will see the **login page** (pictured below). Since we do not have an account yet, click on **Sign Up** to begin. |
There was a problem hiding this comment.
Fix misleading link label vs destination URL
The anchor text says “localhost:3000” but links to “/sessions/new”. Make the label and target consistent to avoid confusing readers.
Apply this diff:
-Once Sure is installed, open a browser and navigate to [localhost:3000](http://localhost:3000/sessions/new).<br />
+Once Sure is installed, open a browser and navigate to http://localhost:3000/sessions/new.<br />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Once Sure is installed, open a browser and navigate to [localhost:3000](http://localhost:3000/sessions/new).<br /> | |
| You will see the **login page** (pictured below). Since we do not have an account yet, click on **Sign Up** to begin. | |
| Once Sure is installed, open a browser and navigate to http://localhost:3000/sessions/new.<br /> | |
| You will see the **login page** (pictured below). Since we do not have an account yet, click on **Sign Up** to begin. |
🤖 Prompt for AI Agents
In docs/onboarding/guide.md around lines 21 to 22, the anchor text displays
"localhost:3000" but the href points to "/sessions/new", causing a mismatch;
update the anchor so its label and target are consistent — either change the
href to "http://localhost:3000" (or "http://localhost:3000/sessions/new" if you
want to include the path) to match the visible "localhost:3000" text, or change
the visible text to "localhost:3000/sessions/new" or "Sign In" to match the
existing "/sessions/new" link target.
Is there already a PR implementing OpenRouter or can i start one? |
No OpenRouter work has been started yet that I know of, no. Go for it! Please make sure you maintain LangFuse observability if you do that one! See #86 for details. The one issue I see with OpenRouter is that the "naive" implementation (just use the OpenAI-compatible endpoint) assumes that all models/providers work the same. To do it right you have to implement your own |
|
Thanks for the review, @matthieuEv ... this is good to move things forward for now. |
|
Actually, let me add some export tests ... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/models/family_export.rb (1)
4-4: No custom Active Storage queues detectedI searched your
config/initializersandconfig/environmentsdirectories for anyconfig.active_storage.queuessettings and found none. As a result,purge_laterjobs will run on the default queue and could contend with your export-generation jobs.To isolate purge jobs onto their own queue, consider:
Defining a dedicated purge queue in an initializer, for example:
# config/initializers/active_storage.rb Rails.application.config.active_storage.queues.purge = :active_storage_purgeUpdating your background processor’s configuration (e.g., Sidekiq, Resque) to include the
active_storage_purgequeue with appropriate concurrency settings.Let me know if you’d like to add this configuration or need further verification.
test/controllers/family_exports_controller_test.rb (4)
74-84: Happy path delete covered; consider adding a cross-family authorization test.You cover non-admin denial, but an additional check that an admin from another family cannot delete exports they don’t own would harden authorization.
Would you like me to draft a test for “admin of Family A cannot delete Family B’s export”?
85-101: Assert the purge job is enqueued when deleting with an attachment.Since the model uses
dependent: :purge_later, tighten the test to ensure a purge job is queued.Apply this diff inside the test to also assert the job enqueue:
@@ - assert export.export_file.attached? - assert_difference "@family.family_exports.count", -1 do - delete family_export_path(export) - end + assert export.export_file.attached? + assert_difference "@family.family_exports.count", -1 do + assert_enqueued_with(job: ActiveStorage::PurgeJob) do + delete family_export_path(export) + end + end
102-118: Mirror purge-job assertion for failed exports.Same rationale as above; failed exports with files should enqueue a purge on delete.
@@ - assert export.export_file.attached? - assert_difference "@family.family_exports.count", -1 do - delete family_export_path(export) - end + assert export.export_file.attached? + assert_difference "@family.family_exports.count", -1 do + assert_enqueued_with(job: ActiveStorage::PurgeJob) do + delete family_export_path(export) + end + end
119-142: Optionally also verify blob purge by performing jobs.If you want to go beyond attachment removal and assert the blob is purged, you can perform enqueued jobs and check the blob record too.
@@ - # Delete the export - delete family_export_path(export) + # Delete the export and run purge jobs + blob_id = export.export_file.blob.id + perform_enqueued_jobs do + assert_enqueued_with(job: ActiveStorage::PurgeJob) do + delete family_export_path(export) + end + end @@ - # Verify the Active Storage attachment is also gone - # Note: Active Storage purges files asynchronously with `dependent: :purge_later` - # In tests, we can check that the attachment record is gone - assert_not ActiveStorage::Attachment.exists?(file_id) + # Verify the Active Storage attachment and blob are gone + assert_not ActiveStorage::Attachment.exists?(file_id) + assert_not ActiveStorage::Blob.exists?(blob_id)test/models/family_export_test.rb (2)
37-74: Reduce duplication across downloadable? cases (optional).Several tests repeat attach/update patterns. A table-driven subtest or helper could cut duplication and speed the suite.
Example shape:
{ ["completed", true] => true, ["pending", true] => false, ["completed", false] => false, ["failed", true] => false }.each do |(status, with_file), expected| subtest_name = "downloadable? #{status} #{with_file ? 'with' : 'without'} file" test subtest_name do @export.update!(status:) @export.export_file.attach(io: StringIO.new("x"), filename: "t.zip", content_type: "application/zip") if with_file assert_equal expected, @export.downloadable? end end
76-100: Also assert that a purge job is enqueued and executed.Since the model now purges asynchronously, it’s useful to assert both enqueue and effect. You’re already capturing
blob_id.@@ - # Destroy the export - @export.destroy! + # Destroy the export and run purge jobs + perform_enqueued_jobs do + assert_enqueued_with(job: ActiveStorage::PurgeJob) do + @export.destroy! + end + end @@ - # Verify the Active Storage attachment is gone - assert_not ActiveStorage::Attachment.exists?(file_id) + # Verify the Active Storage attachment and blob are gone + assert_not ActiveStorage::Attachment.exists?(file_id) + assert_not ActiveStorage::Blob.exists?(blob_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/models/family_export.rb(1 hunks)test/controllers/family_exports_controller_test.rb(2 hunks)test/models/family_export_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
{app,lib}/**/*.{rb,erb}
📄 CodeRabbit inference engine (CLAUDE.md)
{app,lib}/**/*.{rb,erb}: Use Current.user for the authenticated user; do not use current_user
Use Current.family for the current family; do not use current_family
Files:
app/models/family_export.rb
{app/**/*.rb,app/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
{app/**/*.rb,app/**/*.erb}: UseCurrent.userfor the current user; do not usecurrent_userin Rails code and templates
UseCurrent.familyfor the current family; do not usecurrent_familyin Rails code and templates
Files:
app/models/family_export.rb
app/models/**/{*,**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use Rails concerns and POROs organized around model traits in app/models and app/models/concerns
Files:
app/models/family_export.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/models/**/*.rb: Models should answer questions about themselves (e.g., Account#balance_series) rather than external service objects
Use ActiveRecord validations for complex validations and business logic (may mirror DB constraints for UX)Domain models should not call Provider::Registry directly; instead, add a Provided concern within the model’s namespace to choose providers and expose convenience methods (e.g., app/models//provided.rb).
Files:
app/models/family_export.rb
{app/helpers/**/*.rb,app/models/**/*.rb,app/views/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates server-side (helpers/models) and pass formatted values to Stimulus for display only
Files:
app/models/family_export.rb
{app/models/**/*.rb,app/controllers/**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/family_export.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*_test.rb: Use VCR for external API testing
Use the mocha gem for stubs and mocks in tests
Files:
test/models/family_export_test.rbtest/controllers/family_exports_controller_test.rb
test/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
test/**/*.rb: Write tests with Minitest; do not use RSpec in this codebase
Use fixtures for test data; do not use factories
For tests needing many records, use Rails helpers to create them and inline the creation in the test (e.g., helpers in test/support)
Test boundaries correctly: for query methods, assert returned output; for command methods, assert the command was invoked with correct params
Do not test one class’s implementation details in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for simple mock instances, or define a small mock class for complex cases
Only mock what’s necessary; don’t mock return values you don’t assert on
Files:
test/models/family_export_test.rbtest/controllers/family_exports_controller_test.rb
🧬 Code graph analysis (1)
test/models/family_export_test.rb (3)
test/controllers/family_exports_controller_test.rb (1)
setup(3-153)test/jobs/family_data_export_job_test.rb (1)
setup(3-32)app/models/family_export.rb (2)
filename(15-17)downloadable?(19-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (6)
test/controllers/family_exports_controller_test.rb (3)
36-37: Redirect target updated correctly to Imports.The redirect to
imports_pathafter creating an export matches the PR’s navigation changes. Looks good.
70-72: Consistent UX for incomplete downloads.Redirecting incomplete downloads back to
imports_pathwith a clear alert keeps the flow coherent with the new location.
143-152: Non-admin delete denial looks good.Covers the negative path and keeps users scoped correctly.
test/models/family_export_test.rb (3)
4-7: Solid test setup.Fixtures + a fresh export per test keeps things isolated and readable.
29-35: Deterministic filename test is on point.Freezing time to validate the timestamped name avoids flakiness.
115-130: Ordered scope test reads well and is robust.Backdating via
update_columnmakes intent explicit and avoids time helpers leakage between tests.
Issues pending:
DIVs collapsible.gpt-4vs.gpt-4.1vs.gpt-4.1-mini)/settings/profileto/importsplaid.md,chats.md, etc.) to/settings/guidesThis is where we are going:
Summary by cubic
Reorganized the Settings navigation by adding an Advanced section to group admin-level options. Updated translations for English, Norwegian Bokmål, and Turkish.
Summary by CodeRabbit
New Features
Improvements
Localization