feat(settings): add reviewed bulk cleanup flows#1630
feat(settings): add reviewed bulk cleanup flows#1630JSONbored wants to merge 23 commits intowe-promise:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds bulk-edit UIs and controller actions for account institution domains and family-merchant websites; centralizes domain extraction and Brandfetch logo URL generation on Merchant; normalizes account institution domains; implements Category::Merger with controller wiring, views, routes, locales, Stimulus, and expanded tests. ChangesAccount Bulk Domains & Institution Domain Normalization
Category Merge Workflow
Family Merchant Bulk Websites & Merchant URL/Logo Centralization
Sequence Diagram(s)sequenceDiagram
participant User as User
participant AC as AccountsController
participant Account as Account
participant DB as Database
User->>AC: POST bulk_update_domains (institution_domain, account_ids)
AC->>AC: normalize_institution_domain(domain)
AC->>AC: scope writable accounts for family
AC->>DB: begin transaction
loop per selected writable account
AC->>Account: update!(institution_domain: normalized)
Account-->>DB: write
end
DB->>AC: commit
AC->>User: redirect with success or alert on failure
sequenceDiagram
participant User as User
participant CC as CategoriesController
participant Merger as Category::Merger
participant DB as Database
User->>CC: POST perform_merge (target/new_target, source_ids)
CC->>CC: validate params & resolve/create target
CC->>Merger: new(family, target, sources)
Merger->>Merger: validate ownership & hierarchy
Merger->>DB: begin transaction
Merger->>DB: update transactions to target
Merger->>DB: merge budget_categories
Merger->>DB: reparent children & destroy sources
DB->>Merger: commit
Merger->>CC: return merged_count
CC->>User: redirect with success or alert on failure
sequenceDiagram
participant User as User
participant FMC as FamilyMerchantsController
participant Merchant as Merchant
participant Prov as ProviderMerchant
participant DB as Database
User->>FMC: POST bulk_update_websites (website_url, merchant_ids)
FMC->>Merchant: extract_domain(website_url)
Merchant-->>FMC: normalized domain or nil
FMC->>FMC: validate selection & domain
FMC->>DB: begin transaction
loop per selected merchant in family
FMC->>DB: update merchant.website_url
alt merchant is ProviderMerchant
Prov->>Merchant: brandfetch_logo_url_for(website_url)
Merchant-->>Prov: brandfetch url or nil
Prov->>DB: update logo_url
end
end
DB->>FMC: commit
FMC->>User: redirect with success or alert on failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JSONbored
force-pushed
the
codex/feat-settings-bulk-cleanup
branch
from
366477a to
66b8e96
Compare
brin-security-scanner
Bot
added
contributor:flagged
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66b8e96c82
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
test/controllers/family_merchants_controller_test.rb (1)
48-62: ⚡ Quick winMake the merge preconditions explicit in this test.
This currently depends on
merchants(:amazon)being a current-familyFamilyMerchantand ontransactions(:one)already pointing at it. If either fixture changes, the assertion stops proving the merge behavior. Create the source merchant here and assign the transaction before posting so the test remains self-contained.As per coding guidelines, create edge cases on-the-fly within test context instead of in fixtures; keep fixtures minimal (2-3 per model).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/family_merchants_controller_test.rb` around lines 48 - 62, The test "should merge selected merchants into a new family merchant" relies on fixtures (merchants(:amazon) and transactions(:one)); make it self-contained by creating the source FamilyMerchant and reassigning the transaction within the test before calling post perform_merge_family_merchants_path: instantiate a new FamilyMerchant (associated with `@user.family`) to act as source instead of merchants(:amazon), set transactions(:one).merchant = source and save (or create a new transaction tied to that merchant), then perform the post with source_ids: [source.id] and assert the merge results (FamilyMerchant.find_by!(family: `@user.family`, name: "Streaming"), reload the transaction and check merchant, and assert_not FamilyMerchant.exists?(source.id)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/accounts_controller.rb`:
- Around line 45-57: The bulk_update_domains action reads params directly;
create and use a strong-params helper (e.g., bulk_domain_params) to permit
:institution_domain (string) and account_ids: [] and replace direct uses of
params[:institution_domain] and params[:account_ids] with values from
bulk_domain_params; ensure you call
Account.normalize_institution_domain(bulk_domain_params[:institution_domain])
and query bulk_domain_accounts.where(id: bulk_domain_params[:account_ids]) so
the controller uses permitted input before validation and the transactional
update (accounts.each { |account| account.update!(institution_domain: domain) })
runs only on whitelisted attributes.
In `@app/controllers/categories_controller.rb`:
- Around line 80-83: merge_target_category currently creates a new category when
both target_id and new_target_name are provided, which wrongly ignores the
explicit target; update the validation so that when both params[:target_id] and
params[:new_target_name] are present (in the flow around merge_target_category
and the similar block at lines 125-135), treat this as invalid input and
redirect back with an alert (e.g., t(".conflicting_target_params")) instead of
creating a new category; adjust merge_target_category (and the controller action
that assigns target = merge_target_category) to first check for the conflict and
return nil or raise a controlled error so the existing redirect_to
merge_categories_path, alert: ... path is used.
- Around line 74-100: Wrap the incoming merge payload in a strong-params helper
and use it in perform_merge and the merge_target_category helper: implement a
private method merge_params that does params.require(:merge).permit(source_ids:
[], :target_id, :new_target_name, :new_target_description,
:new_target_other_allowed_fields) (include the actual new_target_* names your
form sends), then replace direct uses of params[:source_ids],
params[:target_id], and params[:new_target_*] in perform_merge and in
merge_target_category with values from merge_params (e.g.
merge_params[:source_ids], merge_params[:target_id],
merge_params[:new_target_name]); ensure perform_merge still looks up sources via
Current.family.categories.where(id: merge_params[:source_ids]) and builds the
Category::Merger with the permitted target and source values.
In `@app/controllers/family_merchants_controller.rb`:
- Around line 109-124: The bulk_update_websites and related actions currently
read merchant_ids, source_ids, target_id and new_target_* directly from params;
create private strong-parameter helper methods (e.g. permitted_bulk_params and
permitted_merge_params) that use params.require(...).permit(merchant_ids: [],
source_ids: [], :target_id, :new_target_name, :new_target_field_x, ...) and
replace direct params access in bulk_update_websites and the merge/similar
methods so only permitted values are consumed; ensure array params use the []
form and that you call the helper (e.g. ids =
permitted_bulk_params[:merchant_ids]) before using the values.
- Around line 136-139: The controller currently lets merge_target_merchant
prefer new_target_name over target_id, which allows both inputs to be submitted
and causes ambiguous/destructive merges; update the controller actions that call
merge_target_merchant (the merge submission endpoints around the current target
resolution and the second similar block later) to validate params and reject
requests that provide both target_id and new_target_name: if both are present,
redirect_to merge_family_merchants_path (or the relevant form path) with an
alert (e.g. t(".ambiguous_target")) and do not proceed to merge_target_merchant;
ensure the check is performed before calling merge_target_merchant so exactly
one mode is supplied.
In `@app/models/account.rb`:
- Around line 141-149: The normalize_institution_domain logic is only a
controller/helper and imports (app/models/family/data_importer.rb) still write
raw values; move the normalization into the Account model so all writes are
canonical: add a before_validation callback (e.g., before_validation
:normalize_institution_domain!) or implement an institution_domain= setter in
Account that calls the existing normalize_institution_domain method, ensure
normalize_institution_domain is instance-level and used by that callback/setter,
and remove direct normalization from controllers/services so every write path
(including Family::DataImporter) uses the model-level normalization.
- Around line 141-149: The rescue in normalize_institution_domain currently
returns the raw URL string (value.to_s.strip), which preserves paths/invalid
chars; change the rescue to produce a host-only, sanitized domain: take s =
value.to_s.strip, remove any leading scheme via s.sub(/\Ahttps?:\/\//i, ""),
then split on / or : and keep the first segment, strip and remove leading "www."
(sub(/\Awww\./, "")), then return presence; update the rescue in
normalize_institution_domain to implement this host-extraction instead of
returning the original string.
In `@app/models/category/merger.rb`:
- Around line 43-49: validate_hierarchy! only checks the immediate parent
(target_category.parent_id == source.id) and misses ancestors, allowing merges
that would create cycles; update validate_hierarchy! to walk the full ancestor
chain of target_category (follow parent_id repeatedly or collect ancestor ids
via target_category.ancestors or a loop) and if any ancestor id equals any
source.id raise UnauthorizedCategoryError, ensuring reparent_subcategories
cannot set a category as its own parent; reference validate_hierarchy!,
source_categories, target_category.parent_id and reparent_subcategories when
making the change.
In `@app/models/merchant.rb`:
- Around line 13-20: The rescue branch in extract_domain currently returns the
raw input; change it to extract and return only a domain (or nil) instead of the
malformed URL. In the extract_domain method, after normalizing the string
(normalized_url), replace the rescue body with logic that strips any leading
scheme (normalized_url.sub(/\Ahttps?:\/\//, "")), takes the first path segment
(split("/").first), removes a leading "www." (sub(/\Awww\./, "")) and then
return that value only if it looks like a host (presence and optionally contains
a dot), otherwise return nil — do not return the original URL string.
---
Nitpick comments:
In `@test/controllers/family_merchants_controller_test.rb`:
- Around line 48-62: The test "should merge selected merchants into a new family
merchant" relies on fixtures (merchants(:amazon) and transactions(:one)); make
it self-contained by creating the source FamilyMerchant and reassigning the
transaction within the test before calling post
perform_merge_family_merchants_path: instantiate a new FamilyMerchant
(associated with `@user.family`) to act as source instead of merchants(:amazon),
set transactions(:one).merchant = source and save (or create a new transaction
tied to that merchant), then perform the post with source_ids: [source.id] and
assert the merge results (FamilyMerchant.find_by!(family: `@user.family`, name:
"Streaming"), reload the transaction and check merchant, and assert_not
FamilyMerchant.exists?(source.id)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7be46bdc-f14b-4d5c-a378-a1c099457660
📥 Commits
Reviewing files that changed from the base of the PR and between 268f5ae and 66b8e96c82aa6e0e3b5fc1288b9f4698e23ada73.
📒 Files selected for processing (22)
app/controllers/accounts_controller.rbapp/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/models/account.rbapp/models/category/merger.rbapp/models/family_merchant.rbapp/models/merchant.rbapp/models/provider_merchant.rbapp/views/accounts/bulk_domains.html.erbapp/views/accounts/index.html.erbapp/views/categories/index.html.erbapp/views/categories/merge.html.erbapp/views/family_merchants/bulk_websites.html.erbapp/views/family_merchants/index.html.erbapp/views/family_merchants/merge.html.erbconfig/locales/views/accounts/en.ymlconfig/locales/views/categories/en.ymlconfig/locales/views/merchants/en.ymlconfig/routes.rbtest/controllers/accounts_controller_test.rbtest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rb
brin-security-scanner
Bot
added
the
contributor:flagged
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/models/account.rb (1)
148-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRescue path still returns a raw URL string instead of a domain (or
nil).For an input like
"https://bank.com/%",URI.parseraisesURI::InvalidURIErrorand the rescue path produces"https://bank.com/%"— a full URL with scheme and path — not a bare domain. That value passes thepresent?check inlogo_urland is embedded directly into a Brandfetch CDN URL, producing a broken string (cdn.brandfetch.io/https://bank.com/%/...).
app/models/merchant.rb'sextract_domain(line 22-23) already fixes this by returningnilon error —normalize_institution_domainshould do the same.🐛 Proposed fix
rescue URI::InvalidURIError - value.to_s.strip.sub(/\Awww\./, "").presence&.downcase + nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/account.rb` around lines 148 - 149, The rescue in normalize_institution_domain currently returns the original value string (e.g. "https://bank.com/%") which yields a full URL instead of a bare domain; change normalize_institution_domain so that on URI::InvalidURIError it returns nil (or delegates to the same behavior as Merchant.extract_domain) rather than value.to_s..., ensuring callers like logo_url won't embed an invalid full URL into the Brandfetch CDN path.
🧹 Nitpick comments (2)
app/views/accounts/bulk_domains.html.erb (1)
16-16: ⚡ Quick winSame
border-gray-300raw color class — use a design token.Mirrors the issue in
app/views/family_merchants/merge.html.erbline 24. The surrounding code already usesborder-secondary(line 13); the checkbox should match.🎨 Proposed fix
- <%= check_box_tag "account_ids[]", account.id, false, class: "rounded border-gray-300" %> + <%= check_box_tag "account_ids[]", account.id, false, class: "rounded border-primary" %>As per coding guidelines: "Use Tailwind CSS design tokens (e.g.,
text-primary,bg-container) instead of raw Tailwind classes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/accounts/bulk_domains.html.erb` at line 16, The checkbox uses a raw Tailwind color class; change the class on the check_box_tag call (the line rendering check_box_tag "account_ids[]", account.id, false) to use the design token used elsewhere (e.g., replace "border-gray-300" with "border-secondary") so the checkbox matches the surrounding styles; update only the class string for that check_box_tag to include "rounded border-secondary" (and keep any existing classes) to conform to the design-token guideline.app/views/family_merchants/merge.html.erb (1)
24-24: ⚡ Quick winReplace
border-gray-300with a design token.
border-gray-300is a raw Tailwind color utility; the design system already providesborder-primary(orborder-secondaryto match the adjacent container border on line 21). Mixing raw color classes breaks theming.🎨 Proposed fix
- <%= check_box_tag "source_ids[]", merchant.id, false, class: "rounded border-gray-300" %> + <%= check_box_tag "source_ids[]", merchant.id, false, class: "rounded border-primary" %>As per coding guidelines: "Use functional tokens defined in design system such as
text-primaryinstead oftext-white,bg-containerinstead ofbg-white,border border-primaryinstead ofborder border-gray-200."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/family_merchants/merge.html.erb` at line 24, The checkbox currently uses a raw Tailwind color utility ("border-gray-300")—update the check_box_tag call so its class uses the design token instead (e.g., replace "border-gray-300" with "border-primary" or "border-secondary" to match the adjacent container border), preserving the other classes like "rounded"; locate the check_box_tag for "source_ids[]" (merchant.id) and change only the border utility to the appropriate token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/accounts_controller.rb`:
- Around line 54-58: The bulk update inside Account.transaction using
accounts.each { |account| account.update!(institution_domain: domain) } can
raise ActiveRecord::RecordInvalid and cause a 500; wrap the transaction in a
begin/rescue that rescues ActiveRecord::RecordInvalid (or
ActiveRecord::RecordInvalid => e), let the transaction rollback, and then
redirect_to accounts_path with an alert (use i18n like t(".failure") and include
e.record.errors.full_messages.join(", ") or e.message) instead of allowing the
exception to bubble; keep using update! so validations still raise but are now
handled in the AccountsController action around the Account.transaction block.
In `@app/controllers/categories_controller.rb`:
- Around line 86-101: The target category is created with merge_target_category
(which calls Current.family.categories.create!) outside the merger's
transaction, so if Category::Merger#merge! later raises (e.g. RecordInvalid) the
newly created target can be left orphaned; fix by wrapping both the creation and
the merge call in a single ActiveRecord transaction: call Category.transaction
do … end around the call to merge_target_category and the subsequent
merger.merge! so the create! and all operations inside Category::Merger#merge!
participate in the same transaction and roll back together on error.
---
Duplicate comments:
In `@app/models/account.rb`:
- Around line 148-149: The rescue in normalize_institution_domain currently
returns the original value string (e.g. "https://bank.com/%") which yields a
full URL instead of a bare domain; change normalize_institution_domain so that
on URI::InvalidURIError it returns nil (or delegates to the same behavior as
Merchant.extract_domain) rather than value.to_s..., ensuring callers like
logo_url won't embed an invalid full URL into the Brandfetch CDN path.
---
Nitpick comments:
In `@app/views/accounts/bulk_domains.html.erb`:
- Line 16: The checkbox uses a raw Tailwind color class; change the class on the
check_box_tag call (the line rendering check_box_tag "account_ids[]",
account.id, false) to use the design token used elsewhere (e.g., replace
"border-gray-300" with "border-secondary") so the checkbox matches the
surrounding styles; update only the class string for that check_box_tag to
include "rounded border-secondary" (and keep any existing classes) to conform to
the design-token guideline.
In `@app/views/family_merchants/merge.html.erb`:
- Line 24: The checkbox currently uses a raw Tailwind color utility
("border-gray-300")—update the check_box_tag call so its class uses the design
token instead (e.g., replace "border-gray-300" with "border-primary" or
"border-secondary" to match the adjacent container border), preserving the other
classes like "rounded"; locate the check_box_tag for "source_ids[]"
(merchant.id) and change only the border utility to the appropriate token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feb9a908-fa61-4c75-b9c1-5aba4a17dc27
📒 Files selected for processing (16)
app/controllers/accounts_controller.rbapp/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/models/account.rbapp/models/category/merger.rbapp/models/merchant.rbapp/views/accounts/bulk_domains.html.erbapp/views/categories/merge.html.erbapp/views/family_merchants/bulk_websites.html.erbapp/views/family_merchants/merge.html.erbconfig/locales/views/categories/en.ymlconfig/locales/views/merchants/en.ymltest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rbtest/models/account_test.rbtest/models/merchant_test.rb
✅ Files skipped from review due to trivial changes (5)
- app/views/categories/merge.html.erb
- test/models/account_test.rb
- config/locales/views/merchants/en.yml
- app/views/family_merchants/bulk_websites.html.erb
- config/locales/views/categories/en.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/family_merchants_controller.rb
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Review: feat(settings) — reviewed bulk cleanup flows
Good overall direction. The domain-extraction consolidation (Merchant.extract_domain / brandfetch_logo_url_for) cleans up a real duplication, the scoping is correct throughout, and the test coverage for the happy/sad paths is solid.
A few things need attention before merge:
Must fix
bulk_update_websitesis missing arescue ActiveRecord::RecordInvalidblock — a validation failure will 500 (see inline comment).reparent_subcategoriesmoves orphaned source children to the target's parent, not to the target itself — needs a decision on intended behaviour and likely a fix (see inline comment).
Should fix
- Three templates use
border-gray-300directly, violating the project's design-system token rule. COLORS.samplefallback is non-deterministic; useCOLORS.firstto match the form default.
Nits / suggestions
perform_mergeinCategoriesControlleruses a confusing variable-mutation-across-rollback pattern; a simpler control flow is suggested inline.Category::Merger#merge!wraps itself in a transaction that's already opened by the controller — pick one owner.- The target dropdown and sources checklist share the same
@categorieslist with no UX indication that selecting the same item in both is a no-op.
Generated by Claude Code
| unless target | ||
| return redirect_to merge_family_merchants_path, alert: t(".target_not_found") | ||
| if conflicting_merge_target?(permitted_params) | ||
| return redirect_to merge_family_merchants_path, alert: t(".conflicting_target") | ||
| end |
There was a problem hiding this comment.
Missing error handling — potential 500.
bulk_update_websites has no rescue ActiveRecord::RecordInvalid block. If a merchant.update! call fails validation, the exception will bubble up and produce a 500. The two other bulk operations (bulk_update_domains, perform_merge) both rescue this; this one should too:
redirect_to family_merchants_path, notice: t(".success", count: merchants.count)
rescue ActiveRecord::RecordInvalid => e
redirect_to bulk_websites_family_merchants_path, alert: e.record.errors.full_messages.to_sentence
endGenerated by Claude Code
| source_budget_category.update!(category_id: target_category.id) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
reparent_subcategories moves children to the target's parent, not to the target itself.
When the target is a subcategory (has a parent_id), the source's children get reassigned to the target's parent, making them siblings of the target rather than children of it. For example: merging Food > Coffee (source) into Food > Restaurants (target) would move Coffee's subcategories up to Food, not down into Restaurants.
Is this intentional? If so, it's worth documenting. If not, the condition should likely be:
def reparent_subcategories(source)
family.categories.where(parent_id: source.id).where.not(id: target_category.id).update_all(parent_id: target_category.id)
endNote also that dependent: :nullify is set on Category#subcategories, so source.destroy! will nullify parent_id on any subcategories not explicitly reparented here — you'd want to ensure the update_all runs before destroy!, which it does in the current code.
Generated by Claude Code
| <p class="text-sm font-medium text-primary"><%= t(".accounts_label") %></p> | ||
| <div class="max-h-64 overflow-y-auto space-y-1 border border-secondary rounded-lg p-2"> | ||
| <% @accounts.each do |account| %> | ||
| <label class="flex items-center gap-2 p-2 hover:bg-surface-hover rounded cursor-pointer"> |
There was a problem hiding this comment.
Design system violation: border-gray-300.
Per the project conventions, raw Tailwind primitives should not be used directly — functional tokens from the design system are required. The same raw class appears in categories/merge.html.erb and family_merchants/bulk_websites.html.erb as well.
Existing usage elsewhere in the app (e.g. family_merchants/merge.html.erb) doesn't use border-gray-300 on checkboxes. The simplest fix is to drop the border class entirely and rely on the browser/OS default checkbox styling, or use whichever design-system utility is appropriate (e.g. border-secondary).
Generated by Claude Code
| def merge_target_category(permitted_params) | ||
| if permitted_params[:new_target_name].present? | ||
| Current.family.categories.create!( | ||
| name: permitted_params[:new_target_name], |
There was a problem hiding this comment.
COLORS.sample fallback picks a random color non-deterministically.
The form pre-populates the color picker with Category::COLORS.first, so the user always sees a chosen color. But if the field arrives empty server-side (e.g. a crafted request), the saved color would be random, producing a mismatch from what was shown.
A safer fallback is the same default the form uses:
color: permitted_params[:new_target_color].presence || Category::COLORS.first,The same issue exists in merge_target_merchant for FamilyMerchant::COLORS.sample.
Generated by Claude Code
| return redirect_to merge_categories_path, alert: t(".invalid_categories") | ||
| end | ||
|
|
||
| target = nil | ||
| merger = nil | ||
| merge_succeeded = false | ||
|
|
||
| Category.transaction do | ||
| target = merge_target_category(permitted_params) | ||
| raise ActiveRecord::Rollback unless target | ||
|
|
||
| merger = Category::Merger.new( | ||
| family: Current.family, | ||
| target_category: target, | ||
| source_categories: sources | ||
| ) | ||
|
|
||
| merge_succeeded = merger.merge! | ||
| raise ActiveRecord::Rollback unless merge_succeeded | ||
| end | ||
|
|
||
| unless target | ||
| return redirect_to merge_categories_path, alert: t(".target_not_found") | ||
| end | ||
|
|
||
| if merge_succeeded | ||
| redirect_to categories_path, notice: t(".success", count: merger.merged_count) | ||
| else | ||
| redirect_to merge_categories_path, alert: t(".no_categories_selected") | ||
| end | ||
| rescue Category::Merger::UnauthorizedCategoryError => e |
There was a problem hiding this comment.
Controller perform_merge is harder to follow than necessary.
Initializing target, merger, and merge_succeeded as nil/false outside the transaction block, then checking them again after the block, is a pattern that relies on Ruby variable mutation across a raise ActiveRecord::Rollback — which is non-obvious and could surprise maintainers who don't know that Rollback silently absorbs the exception rather than propagating it.
A cleaner approach: move the redirect logic so that all branching happens inside the transaction for the happy path, and only rescue/redirect outside. For example:
def perform_merge
permitted_params = category_merge_params
return redirect_to merge_categories_path, alert: t(".conflicting_target") if conflicting_merge_target?(permitted_params)
sources = Current.family.categories.where(id: permitted_params[:source_ids])
return redirect_to merge_categories_path, alert: t(".invalid_categories") unless sources.any?
ActiveRecord::Base.transaction do
target = merge_target_category(permitted_params) or raise ActiveRecord::Rollback
merger = Category::Merger.new(family: Current.family, target_category: target, source_categories: sources)
merger.merge! or raise ActiveRecord::Rollback
# if we reach here, commit and redirect
return redirect_to categories_path, notice: t(".success", count: merger.merged_count)
end
redirect_to merge_categories_path, alert: t(".no_categories_selected")
rescue Category::Merger::UnauthorizedCategoryError => e
redirect_to merge_categories_path, alert: e.message
rescue ActiveRecord::RecordInvalid => e
redirect_to merge_categories_path, alert: e.record.errors.full_messages.to_sentence
endThis makes the control flow explicit: return from inside the transaction on success, fall through to an alert redirect if the transaction was rolled back.
Generated by Claude Code
|
|
||
| def merge! | ||
| return false if source_categories.empty? | ||
|
|
There was a problem hiding this comment.
Category::Merger#merge! opens its own transaction, but the controller already wraps the call in one.
Nested ActiveRecord::Base.transaction blocks use savepoints in PostgreSQL (and are no-ops in SQLite unless requires_new: true is set). The inner transaction here doesn't add real isolation — the outer transaction in the controller is what ultimately governs rollback. This double-wrapping is a code smell and could cause confusion if the behaviour ever needs to differ between the two.
Consider either:
- having the controller manage the transaction and
merge!operate without one (caller's responsibility), or - having
merge!own the transaction and the controller not wrap it.
Since Category::Merger is a domain object that may be called from contexts other than this controller, having it own the transaction (option 2) is probably the cleaner long-term design — but then the controller shouldn't open its own outer transaction.
Generated by Claude Code
| {} %> | ||
|
|
||
| <div class="space-y-3 rounded-lg border border-secondary p-3"> | ||
| <p class="text-sm font-medium text-primary"><%= t(".new_target_label") %></p> |
There was a problem hiding this comment.
Same @categories list is used for both the target dropdown and the sources checklist — a user can select the same category as both.
Category::Merger silently filters out the target from the sources (sources.reject { |c| c.id == target_category.id }), so there's no data hazard. But from a UX perspective, a user who selects "Dining" as target and also checks "Dining" as a source will see it silently ignored with no feedback — the success message will report one fewer merge than they expected.
A small JS enhancement (or even just a note in the hint text) to clarify that selecting a category as target automatically excludes it from the source list would prevent confusion.
Generated by Claude Code
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
JSONbored
force-pushed
the
codex/feat-settings-bulk-cleanup
branch
from
770b17f to
ccc8fcb
Compare
brin-security-scanner
Bot
added
the
contributor:flagged
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/controllers/accounts_controller_test.rb (1)
17-31: ⚡ Quick winMake the writable vs. read-only setup explicit in the test.
This scenario currently passes or fails based on whatever permissions the fixtures happen to give
accounts(:depository)andaccounts(:credit_card). Since the controller scopes throughwritable_by, it’s safer to create or override the relevantAccountSharerecords inline so the permission boundary is deterministic.As per coding guidelines, “Create edge cases on-the-fly within test context instead of in fixtures; keep fixtures minimal (2-3 per model for base cases).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/accounts_controller_test.rb` around lines 17 - 31, The test "bulk domain update only changes writable accounts" relies on fixture permissions; instead explicitly set up AccountShare records so permissions are deterministic: after signing in users(:family_member) create or update AccountShare for accounts(:depository) to grant writable access for that user (so it appears in writable_by scope) and ensure accounts(:credit_card) either has an AccountShare with read-only permissions or no writable share; then run the same POST to bulk_update_domains_accounts_path and assert the depository's domain changed while the credit_card's did not. Make sure to reference AccountShare and the controller's writable_by scope when creating/updating these shares so the test no longer depends on fixtures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/family_merchants_controller.rb`:
- Around line 203-209: In merge_target_merchant, normalize
permitted_params[:new_target_website_url] using the same canonicalization as
bulk_update_websites before creating the FamilyMerchant: call
Merchant.extract_domain(permitted_params[:new_target_website_url]) (or
equivalent helper) and pass that canonicalized value to website_url (respecting
blank -> nil), while keeping existing color/name handling in the
Current.family.merchants.create! call.
In `@app/models/category/merger.rb`:
- Around line 87-89: The current reparent_subcategories method uses update_all
which bypasses validations and can create an invalid 3-level hierarchy when
target_category already has a parent and source has children; change the flow so
you validate reparenting before performing the bulk update: add a
validate_reparenting! method that raises if target_category.parent_id.present?
AND source.children.exists? (or equivalently source has subcategories), call
validate_reparenting! from initialize (after validate_hierarchy!), and only then
perform the update_all in reparent_subcategories; keep the method names
reparent_subcategories, validate_reparenting!, initialize, validate_hierarchy!,
target_category and source to locate the changes.
In `@app/models/provider_merchant.rb`:
- Around line 27-30: The current guard using
Setting.brand_fetch_client_id.present? prevents clearing an old logo because
Merchant.brandfetch_logo_url_for already returns nil when the client id is
missing; remove that guard so we always call Merchant.brandfetch_logo_url_for
when website_url is present and persist its result (which may be nil) to
logo_url. In other words, change the branch that references website_url.present?
to always run update!(logo_url: Merchant.brandfetch_logo_url_for(website_url))
(and keep the website_url.blank? branch calling update!(logo_url: nil)), so
logo_url is cleared when Brandfetch is unavailable or returns nil.
---
Nitpick comments:
In `@test/controllers/accounts_controller_test.rb`:
- Around line 17-31: The test "bulk domain update only changes writable
accounts" relies on fixture permissions; instead explicitly set up AccountShare
records so permissions are deterministic: after signing in users(:family_member)
create or update AccountShare for accounts(:depository) to grant writable access
for that user (so it appears in writable_by scope) and ensure
accounts(:credit_card) either has an AccountShare with read-only permissions or
no writable share; then run the same POST to bulk_update_domains_accounts_path
and assert the depository's domain changed while the credit_card's did not. Make
sure to reference AccountShare and the controller's writable_by scope when
creating/updating these shares so the test no longer depends on fixtures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7019961-cc90-4ad7-bec1-f600ffba82ee
📥 Commits
Reviewing files that changed from the base of the PR and between 4eaf193fd1dc59800285ba6e5073d90df38ea72a and ccc8fcb4800bb6e4e5499006c772bbf546e48894.
📒 Files selected for processing (24)
app/controllers/accounts_controller.rbapp/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/models/account.rbapp/models/category/merger.rbapp/models/family_merchant.rbapp/models/merchant.rbapp/models/provider_merchant.rbapp/views/accounts/bulk_domains.html.erbapp/views/accounts/index.html.erbapp/views/categories/index.html.erbapp/views/categories/merge.html.erbapp/views/family_merchants/bulk_websites.html.erbapp/views/family_merchants/index.html.erbapp/views/family_merchants/merge.html.erbconfig/locales/views/accounts/en.ymlconfig/locales/views/categories/en.ymlconfig/locales/views/merchants/en.ymlconfig/routes.rbtest/controllers/accounts_controller_test.rbtest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rbtest/models/account_test.rbtest/models/merchant_test.rb
✅ Files skipped from review due to trivial changes (9)
- app/views/categories/merge.html.erb
- app/views/categories/index.html.erb
- app/views/family_merchants/index.html.erb
- app/views/family_merchants/bulk_websites.html.erb
- test/models/merchant_test.rb
- config/locales/views/accounts/en.yml
- config/locales/views/categories/en.yml
- app/views/accounts/index.html.erb
- app/models/family_merchant.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- config/routes.rb
- app/views/accounts/bulk_domains.html.erb
- config/locales/views/merchants/en.yml
- test/controllers/categories_controller_test.rb
- test/controllers/family_merchants_controller_test.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/family_merchants_controller.rb`:
- Around line 203-209: merge_target_merchant sets a user-specified color but
FamilyMerchant#set_default_color currently overwrites it on every validation;
update set_default_color so it only assigns a default when color is nil/blank
(e.g., return if color.present?) instead of always replacing it, keeping the
callback and default logic but guarding the assignment so the color passed from
merge_target_merchant (new_target_color) is preserved.
In `@test/controllers/family_merchants_controller_test.rb`:
- Around line 181-193: The test seeds a stale logo but stubs
Setting.brand_fetch_client_id to nil before creation so the
FamilyMerchant.before_save already clears logo_url; instead create the merchant
with a valid Brandfetch state (i.e., do NOT stub Setting.brand_fetch_client_id)
or create the record first and then stub to nil, then set merchant.logo_url to
the stale URL (or create with the logo and then stub), call
merchant.update!(website_url: "new.example.com") and finally assert_nil
merchant.reload.logo_url; update references:
Setting.stubs(:brand_fetch_client_id), FamilyMerchant.create!, merchant.logo_url
assignment, merchant.update! and the final assert_nil to ensure the stale logo
exists before the update that should clear it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23981fc3-19d4-452d-8789-5473517a666c
📒 Files selected for processing (8)
app/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/models/category/merger.rbapp/models/family_merchant.rbapp/models/provider_merchant.rbconfig/locales/views/categories/en.ymltest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/category/merger.rb
- app/controllers/categories_controller.rb
- test/controllers/categories_controller_test.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/controllers/family_merchants_controller_test.rb (1)
183-197: 💤 Low valueConsider moving this test to a model test file.
This test exercises
ProviderMerchantmodel behavior directly rather than through the controller action. While it validates behavior needed bybulk_update_websites, it would be more appropriately placed intest/models/provider_merchant_test.rbfor consistency with test organization conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/family_merchants_controller_test.rb` around lines 183 - 197, The test belongs in the model suite because it directly exercises ProviderMerchant behavior: move the test from test/controllers/family_merchants_controller_test.rb into test/models/provider_merchant_test.rb, keeping the same setup and assertions for ProviderMerchant.create!, update_column(:logo_url, ...), stubbing Setting.brand_fetch_client_id to nil, calling provider_merchant.update!(website_url: ...) and provider_merchant.generate_logo_url_from_website!, and asserting provider_merchant.reload.logo_url is nil; ensure the test class and file use the correct model test helper and naming conventions so it runs with the model tests.app/controllers/categories_controller.rb (1)
100-101: ⚡ Quick winDon't surface raw merger exception text to users.
These alerts come straight from
Category::Mergerexception strings, so this path skips the locale files and will show hard-coded English copy instead of translated UI text. Please map these failures to translation keys before redirecting. As per coding guidelines,**/*.{erb,rb}: Organize i18n keys hierarchically by feature ... Always uset()helper for user-facing strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/categories_controller.rb` around lines 100 - 101, The rescue in CategoriesController that handles Category::Merger::UnauthorizedCategoryError currently surfaces e.message directly; replace the raw exception text with a translation call using the t() helper (e.g. t('categories.merge.unauthorized') or a namespaced key like t('controllers.categories.merge.unauthorized')) in the redirect_to ... alert: argument, and add the corresponding key to locale files; if the exception carries dynamic data you need to show, pass it as interpolation to t(..., param: value) rather than rendering e.message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/categories_controller.rb`:
- Around line 20-22: The merge action in CategoriesController is rendering with
the default layout instead of the settings shell; update the merge action (def
merge) to render using the settings layout (e.g., render layout: "settings") or
configure the controller to use layout "settings" for this action (or for both
index and merge) so the merge page appears inside the settings UI.
- Around line 102-103: The merge action currently only rescues
ActiveRecord::RecordInvalid; add handling for ActiveRecord::RecordNotDestroyed
so failures from Category::Merger#merge! (which uses destroy!) redirect back to
merge_categories_path with a useful alert. Update the controller to rescue
ActiveRecord::RecordNotDestroyed (either in a separate rescue or combined with
the existing rescue for ActiveRecord::RecordInvalid) and use
e.record.errors.full_messages.to_sentence (or fallback to e.message) in the
redirect alert so users see the destruction validation errors; reference the
merge action / rescue block in categories_controller.rb to locate where to add
this.
---
Nitpick comments:
In `@app/controllers/categories_controller.rb`:
- Around line 100-101: The rescue in CategoriesController that handles
Category::Merger::UnauthorizedCategoryError currently surfaces e.message
directly; replace the raw exception text with a translation call using the t()
helper (e.g. t('categories.merge.unauthorized') or a namespaced key like
t('controllers.categories.merge.unauthorized')) in the redirect_to ... alert:
argument, and add the corresponding key to locale files; if the exception
carries dynamic data you need to show, pass it as interpolation to t(..., param:
value) rather than rendering e.message.
In `@test/controllers/family_merchants_controller_test.rb`:
- Around line 183-197: The test belongs in the model suite because it directly
exercises ProviderMerchant behavior: move the test from
test/controllers/family_merchants_controller_test.rb into
test/models/provider_merchant_test.rb, keeping the same setup and assertions for
ProviderMerchant.create!, update_column(:logo_url, ...), stubbing
Setting.brand_fetch_client_id to nil, calling
provider_merchant.update!(website_url: ...) and
provider_merchant.generate_logo_url_from_website!, and asserting
provider_merchant.reload.logo_url is nil; ensure the test class and file use the
correct model test helper and naming conventions so it runs with the model
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8033254-bc52-4c6c-9187-93e863a2653e
📒 Files selected for processing (5)
app/controllers/categories_controller.rbapp/models/family_merchant.rbconfig/locales/views/categories/en.ymltest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- config/locales/views/categories/en.yml
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Re-review — all previous issues addressed, no new concerns
All 7 issues from the prior review have been fixed, and the additional fixes prompted by CodeRabbit are solid. Summary:
Previously flagged — confirmed resolved:
-
Missing
rescue ActiveRecord::RecordInvalidinbulk_update_websites✅ — rescue added with at(".failure")redirect; locale key included. -
reparent_subcategoriesmoved children to the target's parent instead of the target ✅ — now usesparent_id: target_category.id. A newvalidate_reparenting!guard also prevents merging a source-with-subcategories into a subcategory target (which would have created an illegal 3-level hierarchy viaupdate_all). Two new controller tests confirm the corrected direction and the rejection case. -
border-gray-300design system violation in three templates ✅ — replaced withborder-secondarythroughout. -
COLORS.samplenon-deterministic fallback ✅ —Category::COLORS.firstandFamilyMerchant::COLORS.firstused consistently in both controllers and inFamilyMerchant#set_default_color. -
perform_mergecontroller complexity ✅ — refactored into a cleanmerge_categories!helper that raises typed sentinel exceptions (MergeTargetNotFound,EmptyCategoryMerge), giving explicit control flow without the variable-mutation-across-rollback pattern. -
Nested transaction in
Category::Merger#merge!✅ — the innerCategory.transactionwas removed frommerge!; the outer transaction inmerge_categories!owns it. The transaction boundary is now unambiguous. -
Target/source same-list UX ✅ —
target_selected_as_source?guard added server-side with a redirect and alert; hint text updated to "Do not select the target as a source." Pragmatic fix without requiring JS.
Notable improvements in subsequent commits (CodeRabbit-prompted):
FamilyMerchant#set_default_colornow only sets a default whencolor.blank?— user-specified colors are preserved on the new-target merge form.ProviderMerchant#generate_logo_url_from_website!andFamilyMerchant#generate_logo_url_from_websitenow clear stale logos when Brandfetch is unavailable (removed thebrand_fetch_client_idguard;brandfetch_logo_url_forhandles the nil return internally).Account.normalize_institution_domainnow delegates toMerchant.extract_domain, eliminating the separate implementation and the raw-URL fallback. Extra test coverage for the nil case ("https://bad host") added.ActiveRecord::RecordNotDestroyedis now caught alongsideRecordInvalidincategories#perform_merge.
No new issues found. The code is well-structured. The one open thread from chatgpt-codex-connector (P1 — settings layout on bulk_domains) is outdated; bulk_domains correctly uses render layout: false in the current code.
Generated by Claude Code
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/views/family_merchants/merge.html.erb (1)
16-16: 💤 Low valueConsider moving the default color value out of the template.
FamilyMerchant::COLORS.firstcouples the template to the model's internal array ordering. If the array is reordered or the default-color logic is centralised in the model (e.g.,FamilyMerchant.default_color), this view will silently drift. Pass the default from the controller (e.g.,@default_merchant_color) instead.As per coding guidelines: "Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/family_merchants/merge.html.erb` at line 16, The view currently uses FamilyMerchant::COLORS.first directly in the color field (f.color_field :new_target_color), coupling presentation to model internals; move the default logic into the controller by computing and assigning an instance var (e.g., `@default_merchant_color` = FamilyMerchant.default_color || FamilyMerchant::COLORS.first) in the action that renders this view, and update the template to use that instance var for the value (keep the form field name :new_target_color unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/views/family_merchants/merge.html.erb`:
- Line 16: The view currently uses FamilyMerchant::COLORS.first directly in the
color field (f.color_field :new_target_color), coupling presentation to model
internals; move the default logic into the controller by computing and assigning
an instance var (e.g., `@default_merchant_color` = FamilyMerchant.default_color ||
FamilyMerchant::COLORS.first) in the action that renders this view, and update
the template to use that instance var for the value (keep the form field name
:new_target_color unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63a86c65-fa2f-45ac-a446-8c842a7ef4fb
📥 Commits
Reviewing files that changed from the base of the PR and between 8b3acfe424f4027e99e23d97cc064c647010e015 and 6287683.
📒 Files selected for processing (24)
app/controllers/accounts_controller.rbapp/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/models/account.rbapp/models/category/merger.rbapp/models/family_merchant.rbapp/models/merchant.rbapp/models/provider_merchant.rbapp/views/accounts/bulk_domains.html.erbapp/views/accounts/index.html.erbapp/views/categories/index.html.erbapp/views/categories/merge.html.erbapp/views/family_merchants/bulk_websites.html.erbapp/views/family_merchants/index.html.erbapp/views/family_merchants/merge.html.erbconfig/locales/views/accounts/en.ymlconfig/locales/views/categories/en.ymlconfig/locales/views/merchants/en.ymlconfig/routes.rbtest/controllers/accounts_controller_test.rbtest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rbtest/models/account_test.rbtest/models/merchant_test.rb
✅ Files skipped from review due to trivial changes (9)
- app/views/family_merchants/index.html.erb
- app/views/categories/merge.html.erb
- test/models/merchant_test.rb
- config/locales/views/accounts/en.yml
- config/locales/views/categories/en.yml
- test/models/account_test.rb
- app/views/accounts/index.html.erb
- config/locales/views/merchants/en.yml
- app/controllers/accounts_controller.rb
🚧 Files skipped from review as they are similar to previous changes (7)
- app/views/accounts/bulk_domains.html.erb
- app/views/family_merchants/bulk_websites.html.erb
- app/models/provider_merchant.rb
- app/models/family_merchant.rb
- app/controllers/categories_controller.rb
- app/models/category/merger.rb
- app/controllers/family_merchants_controller.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/family_merchants_controller.rb`:
- Around line 204-210: The merge_target_merchant flow currently calls
Merchant.extract_domain(permitted_params[:new_target_website_url]) and will
create the target even if extract_domain returns nil; change it to fail-fast:
when permitted_params[:new_target_website_url] is present, call
Merchant.extract_domain and if it returns nil do not call
Current.family.merchants.create! but instead redirect back with an alert (same
behavior as bulk_update_websites), preserving the user input and halting the
merge; keep the existing creation logic (Current.family.merchants.create!) only
when extract_domain yields a non-nil domain (use
permitted_params[:new_target_website_color] logic unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c98b2c8-3686-4c2f-a164-43cf3e290ec3
📒 Files selected for processing (8)
app/controllers/family_merchants_controller.rbapp/javascript/controllers/merchant_merge_target_controller.jsapp/models/family_merchant.rbapp/models/merchant.rbapp/views/family_merchants/merge.html.erbconfig/locales/views/merchants/en.ymltest/controllers/family_merchants_controller_test.rbtest/models/merchant_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/merchant.rb
- config/locales/views/merchants/en.yml
Thanks, added the high-level feature objective and screenshots to the PR description. I also attached screenshots for the new visible flows: bulk institution-domain cleanup, bulk merchant website cleanup, merchant merge target selection/new-target creation, and category merge. Latest validation is listed in the PR description as well. |
jjmata
removed
the
contributor:flagged
brin-security-scanner
Bot
added
the
contributor:flagged
sure-design
left a comment
sure-design
left a comment
There was a problem hiding this comment.
Re-review — independent read of current code
I read the code directly (not relying on prior review comments) and confirmed what was fixed vs. what remains.
Previously flagged open threads that are now addressed in the current code
Several jjmata threads are still marked unresolved but the code already has the fix:
- Missing rescue in
bulk_update_websites— Therescue ActiveRecord::RecordInvalidblock is present in the current file. Thread can be closed. border-gray-300design system violation — All three views (bulk_domains,categories/merge,bulk_websites) now useborder-secondary. Thread can be closed.COLORS.samplenon-deterministic fallback — Categories useCategory::COLORS.first; merchants useFamilyMerchant.default_color(=COLORS.first). Thread can be closed.- Double-wrapping transactions —
Category::Merger#merge!has no transaction of its own. Only the controller'sCategory.transactioninmerge_categories!exists. The concern is moot. Thread can be closed. reparent_subcategoriesmoves to target's parent — The code correctly usesupdate_all(parent_id: target_category.id)(children go under the target, not under the target's parent). jjmata's suggested fix is identical to what's already in the code. Thread can be closed.
Real bug — one new issue
FamilyMerchantsController#perform_merge can create an orphaned target merchant
This is the same atomicity issue that CodeRabbit flagged for categories_controller (commit 0a94adb) — and that was fixed by wrapping everything in Category.transaction via merge_categories!. That fix was never applied to the merchant equivalent.
Current flow in perform_merge:
target = merge_target_merchant(valid_merchants, permitted_params) # calls create! — committed immediately
# ...
merger = Merchant::Merger.new(family: Current.family, target_merchant: target, source_merchants: sources)
if merger.merge!
redirect_to family_merchants_path, ...
else
redirect_to merge_family_merchants_path, ...
end
rescue Merchant::Merger::UnauthorizedMerchantError => e
redirect_to merge_family_merchants_path, alert: e.message # target merchant already in DB, orphaned
rescue ActiveRecord::RecordInvalid => e
redirect_to merge_family_merchants_path, alert: ... # same problemIf merger.merge! raises ActiveRecord::RecordInvalid (e.g., a validation fires while reassigning transactions) or UnauthorizedMerchantError, the rescue redirects back with an alert — but the newly created target FamilyMerchant is already committed and persists as an orphan with no transactions assigned to it.
Fix — same pattern as categories: wrap target creation + merge in a transaction:
def perform_merge
permitted_params = merchant_merge_params
return redirect_to merge_family_merchants_path, alert: t(".conflicting_target") if conflicting_merge_target?(permitted_params)
valid_merchants = all_family_merchants
sources = valid_merchants.where(id: permitted_params[:source_ids])
return redirect_to merge_family_merchants_path, alert: t(".invalid_merchants") unless sources.any?
merger = nil
merge_succeeded = false
Merchant.transaction do
target = merge_target_merchant(valid_merchants, permitted_params) ||
(return redirect_to merge_family_merchants_path, alert: t(".target_not_found"))
merger = Merchant::Merger.new(family: Current.family, target_merchant: target, source_merchants: sources)
merge_succeeded = merger.merge!
end
if merge_succeeded
redirect_to family_merchants_path, notice: t(".success", count: merger.merged_count)
else
redirect_to merge_family_merchants_path, alert: t(".no_merchants_selected")
end
rescue Merchant::Merger::UnauthorizedMerchantError => e
redirect_to merge_family_merchants_path, alert: e.message
rescue InvalidMerchantWebsite
redirect_to merge_family_merchants_path, alert: t(".invalid_website")
rescue ActiveRecord::RecordInvalid => e
redirect_to merge_family_merchants_path, alert: e.record.errors.full_messages.to_sentence
endMinor issues
Account#logo_url redundantly normalizes on every call
institution_domain is already normalized on save by before_validation :normalize_institution_domain_value. The extra normalize_institution_domain call in logo_url is a no-op (the value is already canonical or nil), but it runs URI.parse + a regex on every logo_url invocation. Replace with the raw attribute:
def logo_url
domain = institution_domain # already normalized by before_validation
if domain.present? && Setting.brand_fetch_client_id.present?
# ...
end
endbrandfetch_logo_url_for test validates an impossible input path
Merchant.stubs(:extract_domain).returns("example.com/path")The real extract_domain validates with /\A[a-z0-9.-]+\.[a-z0-9-]+\z/i, which rejects forward slashes — so "example.com/path" can never be returned in production. The test confirms the encoding helper works, but it only exercises a code path that doesn't exist without the stub. Consider either testing with a value that extract_domain can actually return, or noting that encoding is a purely defensive layer.
FamilyMerchantsController#merge renders in the default layout, not the settings layout
CategoriesController#merge explicitly does render layout: "settings". FamilyMerchantsController#merge has no render call, so it falls back to the application layout on direct navigation. Accessed via frame: :modal it works fine (Turbo extracts the frame), but the experience is inconsistent if a user lands directly on that URL.
Generated by Claude Code
Generated by Claude Code
jjmata
removed
the
contributor:flagged
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
One final pass? See last Claude Code comment ...
…-bulk-cleanup # Conflicts: # app/views/accounts/index.html.erb
brin-security-scanner
Bot
added
the
contributor:verified
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/models/account.rb (1)
305-311: ⚡ Quick win
logo_urlstill inlines the Brandfetch URL instead of using the new centralized helper.The AI summary states logo_url is computed "via Merchant.brandfetch_logo_url_for", but this code path still constructs the URL by string interpolation and bypasses the encoding
Merchant.brandfetch_logo_url_forapplies (verified by the defensive-encoding test intest/models/merchant_test.rb). Routing both Account and Merchant logos through the same helper keeps the centralization the PR is introducing and avoids divergence if the Brandfetch URL shape changes again.♻️ Proposed refactor
def logo_url domain = institution_domain - if domain.present? && Setting.brand_fetch_client_id.present? - logo_size = Setting.brand_fetch_logo_size - - "https://cdn.brandfetch.io/#{domain}/icon/fallback/lettermark/w/#{logo_size}/h/#{logo_size}?c=#{Setting.brand_fetch_client_id}" + brandfetch_url = Merchant.brandfetch_logo_url_for(domain) if domain.present? + + if brandfetch_url + brandfetch_url elsif provider&.logo_url.present? provider.logo_url elsif logo.attached? Rails.application.routes.url_helpers.rails_blob_path(logo, only_path: true) end end(Adjust to match the actual signature/return contract of
Merchant.brandfetch_logo_url_for.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account.rb` around lines 305 - 311, The logo_url method is still building the Brandfetch URL inline and should delegate to the centralized helper; instead of interpolating the URL in Account#logo_url (using institution_domain and Settings), call Merchant.brandfetch_logo_url_for(...) with the domain and relevant settings (logo_size and client_id) so the helper's encoding/formatting is reused; update Account#logo_url to return the result of Merchant.brandfetch_logo_url_for(institution_domain, logo_size: Setting.brand_fetch_logo_size, client_id: Setting.brand_fetch_client_id) (adjust argument names to match the helper's actual signature).app/models/merchant/merger.rb (1)
37-51: ⚡ Quick winConsider restoring the internal transaction for defensive atomicity.
Removing the internal
Merchant.transactionshifts atomicity responsibility to callers. WhileFamilyMerchantsController#merge_merchants!andCategoriesController#merge_categories!both properly wrap their merger calls inMerchant.transactionandCategory.transactionrespectively, this creates a fragile contract—any future caller must remember to do the same. If a new call site forgets,update_allcould succeed whiledestroy!fails midway through the loop, leaving some transactions reassigned and some source merchants still present. Restoring the internal transaction makes the method atomically safe regardless of context and is low-effort overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/merchant/merger.rb` around lines 37 - 51, The merge! method must be made internally atomic: wrap the main logic (the early return check, the source_merchants.each loop that calls family.transactions.where(merchant_id: source.id).update_all(merchant_id: target_merchant.id), the conditional source.destroy!, and the `@merged_count` increment) in a Merchant.transaction block so all updates and destroys are committed or rolled back together; use Merchant.transaction (optionally with requires_new: true if you want an isolated savepoint when callers also wrap) around the body of merge! to ensure defensive atomicity regardless of callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/models/account.rb`:
- Around line 305-311: The logo_url method is still building the Brandfetch URL
inline and should delegate to the centralized helper; instead of interpolating
the URL in Account#logo_url (using institution_domain and Settings), call
Merchant.brandfetch_logo_url_for(...) with the domain and relevant settings
(logo_size and client_id) so the helper's encoding/formatting is reused; update
Account#logo_url to return the result of
Merchant.brandfetch_logo_url_for(institution_domain, logo_size:
Setting.brand_fetch_logo_size, client_id: Setting.brand_fetch_client_id) (adjust
argument names to match the helper's actual signature).
In `@app/models/merchant/merger.rb`:
- Around line 37-51: The merge! method must be made internally atomic: wrap the
main logic (the early return check, the source_merchants.each loop that calls
family.transactions.where(merchant_id: source.id).update_all(merchant_id:
target_merchant.id), the conditional source.destroy!, and the `@merged_count`
increment) in a Merchant.transaction block so all updates and destroys are
committed or rolled back together; use Merchant.transaction (optionally with
requires_new: true if you want an isolated savepoint when callers also wrap)
around the body of merge! to ensure defensive atomicity regardless of callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d05bbee-5e92-4bcc-a0eb-e6b87325fb89
📒 Files selected for processing (6)
app/controllers/family_merchants_controller.rbapp/models/account.rbapp/models/merchant/merger.rbconfig/routes.rbtest/controllers/family_merchants_controller_test.rbtest/models/merchant_test.rb
|
The A couple of things to verify before jjmata's code review:
Otherwise the description and screenshots are now in good shape — ready for a substantive code review from @jjmata. Code review note from Claude Code Generated by Claude Code |
|
New commits pushed 2026-05-07. The original 7-issue review round was fully resolved and confirmed by @jjmata on 2026-05-03. The remaining open item from the 2026-05-06 re-review is the atomicity bug in Generated by Claude Code Generated by Claude Code |
Thanks, I verified the current branch against those points. The cross-family provider merchant path is covered now: The invalid website path is also visible to the user: invalid reviewed I also confirmed the merchant merge atomicity issue is addressed: target creation and merge execution now run inside Local targeted validation passed:
|
3 participants
Summary
Feature objective
Large imported or synced datasets can create duplicate merchants/categories and incomplete account/provider metadata. This PR adds reviewed, family-scoped bulk cleanup flows so users can clean up common data issues faster without auto-applying destructive changes.
What changed
Screenshots
Why
Large imported or synced datasets often create duplicate merchants, categories, and incomplete account/provider metadata. Reviewed bulk cleanup makes Sure easier to maintain while keeping changes scoped, transactional, and reviewable before applying.
Validation
bin/rails test test/controllers/accounts_controller_test.rb test/controllers/categories_controller_test.rb test/controllers/family_merchants_controller_test.rb test/models/account_test.rb test/models/merchant_test.rbbin/rails test test/controllers/family_merchants_controller_test.rbnpm run lintbin/rubocopgit diff --checkNotes
Summary by CodeRabbit
New Features
Other
Tests