Conversation
Lifts the per-panel status indicator up to each collapsed accordion header so admins can see at a glance which providers are connected without expanding every section. Connected providers sort first. - Add optional status: and meta: locals to settings/_section partial; pill hides via group-open:hidden when the section is expanded - New settings/providers/_status_pill partial (ok/warn/err/off states) - Add SettingsHelper#provider_summary to centralise the connected-vs-not logic already scattered across panel partials - Refactor show.html.erb to pass status to every section and sort family_panels by connection state - Add settings.providers.status.* i18n keys - Add system tests asserting pill renders and sort order https://claude.ai/code/session_01KW2HCN9rP1fiyQuw7Cju9D
Partition the provider list in the controller into @connected_providers and @available_providers based on provider_summary status, and render each group under its own heading with a count. Auto-open the section when only one provider is connected. Adds an empty-state line when nothing is connected yet. Co-Authored-By: Claude Opus 4.7 <[email protected]>
… error surfacing - Extend provider_summary to return :err/:warn with meta text by checking latest sync per item (window function, same pattern as ProviderConnectionStatus) and Enable Banking session expiry within 7 days - Partition provider entries into three groups: Connected (:ok), Action needed (:warn/:err, auto-opened), Available (:off) - Add Settings::HealthSummary ViewComponent — four-tile grid showing Connected, Action needed, Errors, and Accounts synced counts - Render health strip directly under page description; omit Action needed heading when group is empty - Add i18n keys for tile labels, group heading, and all meta strings Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ect drawer - Add Provider::Metadata registry with static display data (region, kind, tier, maturity, logo) for all 11 providers - Add Settings::ProviderCard ViewComponent rendering logo square, name, Beta/Alpha pill, meta line (region · type · tier), tagline, and Connect link - Add connect_form action + route (GET /settings/providers/:key/connect_form) that opens the existing panel partial or config form in a DS::Dialog drawer - Replace the Available accordion loop with a 2-column responsive card grid; empty state when all providers are connected - Fix layout override: use turbo_rails/frame layout for frame requests so the drawer response is not wrapped in the full settings layout (was causing Turbo to pick the empty outer drawer frame instead of the filled one) - Add SyncAllProvidersJob and last_sync_all_attempted_at migration (sync-all throttle support) - Unify Connected + Action needed into a single "Your connections" section; items with warn/err status auto-open - Fix Enable Banking grouping: items with expired sessions were returning :off (Available) instead of :warn (Your connections); gate now checks any? instead of any?(&:session_valid?) - Add reconsent_required locale key for fully-expired EB sessions - Surface Beta/Alpha maturity pills on connected provider accordion rows via new badge: param on settings_section helper - Add i18n taglines for all 11 providers; add connect and empty_available keys Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- Delete Settings::BankSyncController and its views (the providers page is
now a strict superset of what bank_sync offered)
- Add permanent 301 redirect: GET /settings/bank_sync → /settings/providers
- Collapse nav to a single "Bank Sync" entry pointing at /settings/providers;
remove the duplicate admin-only "Providers" entry from the Advanced section
- Remove "Providers" from SETTINGS_ORDER; point "Bank Sync" at
settings_providers_path for next/prev navigation
- Rename page title to "Bank Sync"; replace admin-credential lede with
user-facing copy ("Connect external accounts…")
- Update breadcrumb: Home → Bank sync
- Add controller test asserting 301 status and Location header
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
brin-security-scanner
Bot
added
contributor:verified
|
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:
📝 WalkthroughWalkthroughConsolidates bank-sync into a unified Providers flow: adds Provider::Metadata and migration, computes provider health across family panels, adds sync/connect controller actions and SyncAllProvidersJob, introduces view components/partials and routes, updates i18n, and adds tests. ChangesProvider Management UI Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
![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: 2b59dd64c8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| items: [ | ||
| { label: t(".accounts_label"), path: accounts_path, icon: "layers" }, | ||
| { label: t(".bank_sync_label"), path: settings_bank_sync_path, icon: "banknote" }, | ||
| { label: t(".bank_sync_label"), path: settings_providers_path, icon: "banknote" }, |
There was a problem hiding this comment.
This link is rendered in the general settings nav for every user, but Settings::ProvidersController#show is still protected by ensure_admin, which redirects unauthorized users back to settings_providers_path. For family members, clicking the new Bank Sync nav item therefore lands on an admin-only route that redirects to itself rather than showing the old informational page or a usable destination.
Useful? React with 👍 / 👎.
|
|
||
| def perform(family_id) | ||
| family = Family.find(family_id) | ||
| family.sync_later |
There was a problem hiding this comment.
For families with Binance connections, the new Sync all flow enqueues this job but then delegates to family.sync_later; Family::Syncer::SYNCABLE_ITEM_ASSOCIATIONS does not include :binance_items, even though this page now treats Binance as a connected/syncable provider via PANEL_SYNCABLE_TYPES. In that context the UI says all connected providers are syncing, but Binance is never scheduled unless the user clicks its per-row sync button.
Useful? React with 👍 / 👎.
Signed-off-by: Juan José Mata <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
db/schema.rb (2)
1390-1413:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
account_number_maskcolumn insophtron_accountsis the direct cause of the CI failure.
account_number_maskis defined at line 1406 (pre-existing) and again at line 1409 (marked as changed). Railsdb:schema:loadraisesArgumentError: you can't define an already defined column 'account_number_mask' on 'sophtron_accounts'— confirmed by the pipeline failure.🐛 Proposed fix — remove the duplicate line
t.string "customer_id" t.string "member_id" t.string "account_number_mask" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "account_number_mask" t.index ["account_id"], name: "index_sophtron_accounts_on_account_id"🤖 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 `@db/schema.rb` around lines 1390 - 1413, The schema defines the column account_number_mask twice on the sophtron_accounts table which causes Rails to raise an ArgumentError during db:schema:load; remove the duplicate definition so account_number_mask is only declared once in the create_table block for sophtron_accounts (update the create_table "sophtron_accounts" block to delete the redundant t.string "account_number_mask" entry), then run schema lint/tests to confirm the CI error is resolved.
1432-1452:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
sophtron_itemshas 8 columns duplicated — will cause a seconddb:schema:loadfailure aftersophtron_accountsis fixed.
customer_id,customer_name,raw_customer_payload,user_institution_id,current_job_id,job_status,raw_job_payload, andlast_connection_errorall appear at lines 1432–1439 (existing) and again at lines 1442–1449 (marked as new). Additionally,last_connection_errorsilently changes type fromtexttostringbetween the two occurrences.The only genuinely new additions to this table should be
manual_sync(line 1450) andcurrent_job_sophtron_account_id(line 1451) plus the index (line 1452). The entire block at lines 1442–1449 should be removed.🐛 Proposed fix — remove the duplicated lines 1442–1449
t.text "last_connection_error" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "customer_id" - t.string "customer_name" - t.jsonb "raw_customer_payload" - t.string "user_institution_id" - t.string "current_job_id" - t.string "job_status" - t.jsonb "raw_job_payload" - t.string "last_connection_error" t.boolean "manual_sync", default: false, null: false t.uuid "current_job_sophtron_account_id" t.index ["current_job_sophtron_account_id"], ...If the intent was also to change
last_connection_errorfromtexttostring, that requires a separate migration withchange_column.🤖 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 `@db/schema.rb` around lines 1432 - 1452, The schema contains duplicated column definitions for the sophtron_items table; remove the repeated block that re-declares customer_id, customer_name, raw_customer_payload, user_institution_id, current_job_id, job_status, raw_job_payload, and last_connection_error so only the original column definitions remain and only add the new columns manual_sync and current_job_sophtron_account_id plus its index; ensure last_connection_error stays as text in the schema (any type change must be implemented via a separate migration using change_column rather than editing schema.rb).app/controllers/settings/providers_controller.rb (1)
143-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid redirecting unauthorized users back into the same protected action.
On
GET /settings/providers, a non-admin will hitensure_admin, get redirected tosettings_providers_path, and immediately hitensure_adminagain. That creates a redirect loop instead of denying access.Suggested fix
def ensure_admin - redirect_to settings_providers_path, alert: "Not authorized" unless Current.user.admin? + return if Current.user.admin? + + redirect_to root_path, alert: t("settings.providers.not_authorized") end🤖 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/controllers/settings/providers_controller.rb` around lines 143 - 145, The ensure_admin before_action is redirecting non-admins back to settings_providers_path, causing a redirect loop; update ensure_admin (method ensure_admin in providers_controller) to either render a 403/forbidden response (render plain: "Not authorized", status: :forbidden) or redirect to a safe non-protected route (e.g., root_path or dashboard_path) with the alert, instead of redirecting to settings_providers_path, so unauthorized users are not sent back into the same protected action.
🧹 Nitpick comments (5)
app/views/settings/providers/_status_pill.html.erb (1)
2-8: ⚡ Quick winCase statement computing CSS classes belongs in a helper or ViewComponent, not the template.
Per 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." This partial is also reused across multiple views and has variants by status—criteria that specifically recommend a ViewComponent.
♻️ Suggested refactor — extract to a helper
# app/helpers/settings_helper.rb (or providers_helper.rb) def status_pill_classes(status) case status.to_sym when :ok then { dot: "bg-success", pill: "bg-success/10 text-success" } when :warn then { dot: "bg-warning", pill: "bg-warning/10 text-warning" } when :err then { dot: "bg-destructive", pill: "bg-destructive/10 text-destructive" } else { dot: "bg-gray-400", pill: "bg-gray-100 text-secondary" } end end<%# locals: (status:) %> -<% - dot_class, pill_class = case status.to_sym - when :ok then [ "bg-success", "bg-success/10 text-success" ] - when :warn then [ "bg-warning", "bg-warning/10 text-warning" ] - when :err then [ "bg-destructive", "bg-destructive/10 text-destructive" ] - else [ "bg-gray-400", "bg-gray-100 text-secondary" ] - end -%> +<% classes = status_pill_classes(status) %> +<% dot_class, pill_class = classes[:dot], classes[:pill] %>🤖 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/views/settings/providers/_status_pill.html.erb` around lines 2 - 8, The template currently contains presentation logic (the case statement building CSS classes) which should be moved to a helper or ViewComponent; extract that logic into a new helper method (e.g., status_pill_classes(status)) or a StatusPillComponent that returns the dot and pill class values (as keys like :dot and :pill), then update the partial to call status_pill_classes(status) and use the returned classes instead of the inline case; ensure the helper/component handles symbolizing the status and preserves the same mappings for :ok, :warn, :err and the default case.app/jobs/sync_all_providers_job.rb (1)
6-6: ⚡ Quick winUse
find_bywith a nil guard to avoid unrecoverable job failures when a family is deleted mid-queue.
Family.findraisesActiveRecord::RecordNotFoundif the record is gone by the time the job runs. With Sidekiq's default retry behavior this leads to repeated failures until retries are exhausted.♻️ Proposed fix
- family = Family.find(family_id) - family.sync_later + family = Family.find_by(id: family_id) + family&.sync_later🤖 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/jobs/sync_all_providers_job.rb` at line 6, Replace the unsafe Family.find lookup with a nil-safe lookup and early return: change the Family.find(family_id) call to Family.find_by(id: family_id) and add a guard like return unless family (or log and return) in the job's perform method (the place where family = Family.find(...) is used) so the job exits gracefully if the family was deleted instead of raising ActiveRecord::RecordNotFound and retrying.app/views/settings/_section.html.erb (1)
19-27: ⚡ Quick winGeneric section partial couples to a provider-specific partial.
_section.html.erbis a shared layout component, but line 24 hard-codes a dependency on"settings/providers/status_pill". If any other settings section ever needs astatus:value (e.g., a future "Accounts" health section), it would either not render correctly or render an unrelated provider pill. Theactionslocal already demonstrates the right pattern — pass pre-rendered content in from the caller.♻️ Suggested approach
Pass the rendered pill from the call site (e.g.
settings_section(status: render("settings/providers/status_pill", status: ...))), and drop the inlinerenderfrom the generic partial:- <%= render "settings/providers/status_pill", status: status %> + <%= status if status.present? %>The caller can then pass
status: capture { render "settings/providers/status_pill", status: :ok }or any other rendered content, keeping_section.html.erbsubsystem-agnostic.🤖 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/views/settings/_section.html.erb` around lines 19 - 27, The shared partial _section.html.erb currently hard-codes render "settings/providers/status_pill" for the status display, coupling this generic layout to a provider-specific partial; change it to accept rendered status content from the caller (like actions already does) by removing the inline render and using the passed-in status local as-is, and update callers to pass status: render("settings/providers/status_pill", status: ...) or status: capture { render "settings/providers/status_pill", status: :ok } so _section.html.erb remains subsystem-agnostic and callers control the actual markup.app/views/settings/providers/show.html.erb (1)
1-18: ⚡ Quick winMove the new page title and intro copy into i18n.
"Bank Sync"and the lede are hard-coded English while the surrounding UI already uses translations. Please switch these tot(...)so this page stays consistent with the rest of settings copy.As per coding guidelines, "Always use
t()helper for user-facing strings. Updateconfig/locales/en.ymlfor new strings."🤖 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/views/settings/providers/show.html.erb` around lines 1 - 18, Replace the hard-coded page title and intro copy in the view: change content_for :page_title, "Bank Sync" to use t("settings.providers.bank_sync.page_title") and replace the paragraph text ("Connect external accounts so transactions, balances and holdings flow into Sure automatically.") with t("settings.providers.bank_sync.lede"); then add corresponding entries under settings.providers.bank_sync in config/locales/en.yml (page_title and lede) with the existing English strings as values so the view uses the t(...) helper and stays consistent with other translations.app/components/settings/provider_card.rb (1)
2-18: ⚡ Quick winLocalize the maturity badge labels instead of hard-coding English here.
These strings are rendered in the UI, so storing
"Beta"/"Alpha"in the component bypasses locale switching. Keep translation keys (or symbols) in the constant and resolve them witht(...)at render time.As per coding guidelines, "Always use
t()helper for user-facing strings. Updateconfig/locales/en.ymlfor new strings."🤖 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/components/settings/provider_card.rb` around lines 2 - 18, Replace hard-coded English labels in MATURITY_LABELS with translation keys (e.g., :beta and :alpha or symbol strings) and resolve them via the Rails i18n helper in the maturity_label method; specifically, keep MATURITY_LABELS as a mapping of maturity symbols to translation keys, then update the maturity_label method to call t(...) on the mapped key (using the t helper), and add the corresponding entries under config/locales/en.yml so the UI strings are localized.
🤖 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.
Inline comments:
In `@app/components/settings/health_summary.html.erb`:
- Line 1: The grid currently forces four columns on all viewports (the <div
class="grid grid-cols-4 gap-2">) causing very narrow tiles on mobile; change the
grid classes to be responsive so small screens use two columns (or one) and
larger screens use four — e.g. replace or augment grid-cols-4 with responsive
Tailwind classes like grid-cols-2 sm:grid-cols-4 (or grid-cols-1 md:grid-cols-4)
on that same div to ensure tiles have a mobile-friendly layout.
In `@app/components/settings/provider_card.html.erb`:
- Line 4: Replace the raw Tailwind utility "text-white" with the design-system
functional token "text-inverse" in the span that renders the logo text (the span
showing <%= logo_text %>), i.e., update the class attribute on that span to
remove "text-white" and add "text-inverse" so it follows the functional token
convention used by the design system.
In `@app/controllers/settings/providers_controller.rb`:
- Around line 80-89: The sync_all action is race-prone because the
present?/>update pair can be run concurrently; make it atomic by performing a
single conditional DB update on the Family row and only enqueueing
SyncAllProvidersJob when that update succeeds. Replace the current
check+family.update!(last_sync_all_attempted_at: ...) with a conditional update
scoped to Current.family (e.g. a where on Family.id and a predicate that
last_sync_all_attempted_at is nil or <= 30.seconds.ago) and proceed to call
SyncAllProvidersJob.perform_later(family.id) only if the conditional update
affected one row; otherwise redirect with the "sync_all_recently" notice. Ensure
you reference sync_all, Current.family, last_sync_all_attempted_at, and
SyncAllProvidersJob in the change.
In `@app/views/settings/providers/_group_heading.html.erb`:
- Line 2: The div currently always emits id="" when the local variable anchor is
nil; change the ERB so the id attribute is only rendered when anchor is present
(e.g., use a conditional like `if anchor.present?` or an inline ERB conditional)
so the <div> element does not produce an empty id and is only anchorable when a
non-empty anchor value exists; update the line that generates the div (the one
referencing anchor) to conditionally include the id attribute rather than always
outputting id.
In `@app/views/settings/providers/_sync_button.html.erb`:
- Around line 3-9: The button created by button_to
(sync_provider_settings_providers_path) is icon-only and needs an explicit
accessible name; add an aria-label (or include hidden sr-only text) that matches
the sync action instead of relying on title. Update the button_to call to
include aria-label: recently_synced ? t("settings.providers.recently_synced") :
t("settings.providers.sync_provider") (or render an adjacent sr-only <span> with
the same translated text) so screen readers receive a clear label while keeping
the existing title, disabled, provider_key, and form attributes intact.
In `@db/migrate/20260508120000_add_last_sync_all_attempted_at_to_families.rb`:
- Line 1: The migration class AddLastSyncAllAttemptedAtToFamilies currently
inherits from ActiveRecord::Migration[8.0]; change its superclass to
ActiveRecord::Migration[7.2] so it matches the project's schema version and
coding guidelines (replace [8.0] with [7.2] in the class declaration). Ensure no
other parts of this file reference migration version 8.0.
In `@test/system/settings/providers_test.rb`:
- Around line 162-173: The test "clicking a provider card connect link opens the
connect drawer" is clicking the first Connect link in the grid
(find("a[data-turbo-frame='drawer']", match: :first)) which can target the wrong
provider; instead scope the click to the SimpleFIN card by locating the provider
card element that contains the "SimpleFIN" text (e.g., within the card element
matching its title/label) and then call find("a[data-turbo-frame='drawer']",
text: /Connect/).click inside that scoped block so the test always opens
SimpleFIN's drawer before asserting dialog[open] and "Setup Token".
- Around line 20-38: The tests "shows not configured pill for an unconfigured
provider" and "connected providers render before unconfigured ones" still query
accordion markup; update them to target the new available-provider card layout:
replace within("details", text: "SimpleFIN") with a within(...) that scopes to
the available-provider card container (e.g. the container/class that holds
`@available` provider cards) and assert the "Not configured" pill inside that card
for SimpleFIN; similarly change the second test to collect the visible provider
card headers (replace all("details summary") with all(...) selecting the
provider card header/title elements) and compare the index of the SimpleFIN card
vs the Binance card to assert ordering. Also apply the same selector changes to
the related assertions around lines 52-70 (other tests expecting
accordion/summary markup).
---
Outside diff comments:
In `@app/controllers/settings/providers_controller.rb`:
- Around line 143-145: The ensure_admin before_action is redirecting non-admins
back to settings_providers_path, causing a redirect loop; update ensure_admin
(method ensure_admin in providers_controller) to either render a 403/forbidden
response (render plain: "Not authorized", status: :forbidden) or redirect to a
safe non-protected route (e.g., root_path or dashboard_path) with the alert,
instead of redirecting to settings_providers_path, so unauthorized users are not
sent back into the same protected action.
In `@db/schema.rb`:
- Around line 1390-1413: The schema defines the column account_number_mask twice
on the sophtron_accounts table which causes Rails to raise an ArgumentError
during db:schema:load; remove the duplicate definition so account_number_mask is
only declared once in the create_table block for sophtron_accounts (update the
create_table "sophtron_accounts" block to delete the redundant t.string
"account_number_mask" entry), then run schema lint/tests to confirm the CI error
is resolved.
- Around line 1432-1452: The schema contains duplicated column definitions for
the sophtron_items table; remove the repeated block that re-declares
customer_id, customer_name, raw_customer_payload, user_institution_id,
current_job_id, job_status, raw_job_payload, and last_connection_error so only
the original column definitions remain and only add the new columns manual_sync
and current_job_sophtron_account_id plus its index; ensure last_connection_error
stays as text in the schema (any type change must be implemented via a separate
migration using change_column rather than editing schema.rb).
---
Nitpick comments:
In `@app/components/settings/provider_card.rb`:
- Around line 2-18: Replace hard-coded English labels in MATURITY_LABELS with
translation keys (e.g., :beta and :alpha or symbol strings) and resolve them via
the Rails i18n helper in the maturity_label method; specifically, keep
MATURITY_LABELS as a mapping of maturity symbols to translation keys, then
update the maturity_label method to call t(...) on the mapped key (using the t
helper), and add the corresponding entries under config/locales/en.yml so the UI
strings are localized.
In `@app/jobs/sync_all_providers_job.rb`:
- Line 6: Replace the unsafe Family.find lookup with a nil-safe lookup and early
return: change the Family.find(family_id) call to Family.find_by(id: family_id)
and add a guard like return unless family (or log and return) in the job's
perform method (the place where family = Family.find(...) is used) so the job
exits gracefully if the family was deleted instead of raising
ActiveRecord::RecordNotFound and retrying.
In `@app/views/settings/_section.html.erb`:
- Around line 19-27: The shared partial _section.html.erb currently hard-codes
render "settings/providers/status_pill" for the status display, coupling this
generic layout to a provider-specific partial; change it to accept rendered
status content from the caller (like actions already does) by removing the
inline render and using the passed-in status local as-is, and update callers to
pass status: render("settings/providers/status_pill", status: ...) or status:
capture { render "settings/providers/status_pill", status: :ok } so
_section.html.erb remains subsystem-agnostic and callers control the actual
markup.
In `@app/views/settings/providers/_status_pill.html.erb`:
- Around line 2-8: The template currently contains presentation logic (the case
statement building CSS classes) which should be moved to a helper or
ViewComponent; extract that logic into a new helper method (e.g.,
status_pill_classes(status)) or a StatusPillComponent that returns the dot and
pill class values (as keys like :dot and :pill), then update the partial to call
status_pill_classes(status) and use the returned classes instead of the inline
case; ensure the helper/component handles symbolizing the status and preserves
the same mappings for :ok, :warn, :err and the default case.
In `@app/views/settings/providers/show.html.erb`:
- Around line 1-18: Replace the hard-coded page title and intro copy in the
view: change content_for :page_title, "Bank Sync" to use
t("settings.providers.bank_sync.page_title") and replace the paragraph text
("Connect external accounts so transactions, balances and holdings flow into
Sure automatically.") with t("settings.providers.bank_sync.lede"); then add
corresponding entries under settings.providers.bank_sync in
config/locales/en.yml (page_title and lede) with the existing English strings as
values so the view uses the t(...) helper and stays consistent with other
translations.
🪄 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: e3452438-7448-40e0-b004-bbd46d7796fd
📒 Files selected for processing (26)
app/components/settings/health_summary.html.erbapp/components/settings/health_summary.rbapp/components/settings/provider_card.html.erbapp/components/settings/provider_card.rbapp/controllers/settings/bank_sync_controller.rbapp/controllers/settings/providers_controller.rbapp/helpers/settings_helper.rbapp/jobs/sync_all_providers_job.rbapp/models/provider/metadata.rbapp/views/settings/_section.html.erbapp/views/settings/_settings_nav.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/bank_sync/show.html.erbapp/views/settings/providers/_add_provider_cta.html.erbapp/views/settings/providers/_group_heading.html.erbapp/views/settings/providers/_status_pill.html.erbapp/views/settings/providers/_sync_button.html.erbapp/views/settings/providers/connect_form.html.erbapp/views/settings/providers/show.html.erbconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260508120000_add_last_sync_all_attempted_at_to_families.rbdb/schema.rbtest/controllers/settings/providers_controller_test.rbtest/system/settings/providers_test.rbtest/system/settings_test.rb
💤 Files with no reviewable changes (3)
- app/controllers/settings/bank_sync_controller.rb
- app/views/settings/bank_sync/_provider_link.html.erb
- app/views/settings/bank_sync/show.html.erb
| @@ -0,0 +1,8 @@ | |||
| <div class="grid grid-cols-4 gap-2"> | |||
There was a problem hiding this comment.
grid-cols-4 has no mobile fallback — tiles will be very narrow on small screens.
On narrow viewports the four tiles will each be ~70px wide with padding, which can truncate count labels or cause overflow. Consider a two-column layout on small screens.
🛠️ Proposed fix
-<div class="grid grid-cols-4 gap-2">
+<div class="grid grid-cols-2 sm:grid-cols-4 gap-2">🤖 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/components/settings/health_summary.html.erb` at line 1, The grid
currently forces four columns on all viewports (the <div class="grid grid-cols-4
gap-2">) causing very narrow tiles on mobile; change the grid classes to be
responsive so small screens use two columns (or one) and larger screens use four
— e.g. replace or augment grid-cols-4 with responsive Tailwind classes like
grid-cols-2 sm:grid-cols-4 (or grid-cols-1 md:grid-cols-4) on that same div to
ensure tiles have a mobile-friendly layout.
| <div class="bg-container shadow-border-xs rounded-xl p-4 flex flex-col gap-3 hover:shadow-border-sm transition-shadow"> | ||
| <div class="flex items-start gap-3"> | ||
| <div class="w-9 h-9 rounded-lg flex items-center justify-center shrink-0 <%= logo_bg %>"> | ||
| <span class="text-xs font-bold text-white"><%= logo_text %></span> |
There was a problem hiding this comment.
text-white violates the functional token convention — use text-inverse.
As per coding guidelines, raw Tailwind utilities like text-white should be replaced with design system functional tokens.
🛠️ Proposed fix
- <span class="text-xs font-bold text-white"><%= logo_text %></span>
+ <span class="text-xs font-bold text-inverse"><%= logo_text %></span>As per coding guidelines: "Use functional tokens defined in design system such as text-primary instead of text-white".
📝 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.
| <span class="text-xs font-bold text-white"><%= logo_text %></span> | |
| <span class="text-xs font-bold text-inverse"><%= logo_text %></span> |
🤖 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/components/settings/provider_card.html.erb` at line 4, Replace the raw
Tailwind utility "text-white" with the design-system functional token
"text-inverse" in the span that renders the logo text (the span showing <%=
logo_text %>), i.e., update the class attribute on that span to remove
"text-white" and add "text-inverse" so it follows the functional token
convention used by the design system.
| def sync_all | ||
| family = Current.family | ||
|
|
||
| if family.last_sync_all_attempted_at.present? && family.last_sync_all_attempted_at > 30.seconds.ago | ||
| return redirect_to settings_providers_path, notice: t("settings.providers.sync_all_recently") | ||
| end | ||
|
|
||
| family.update!(last_sync_all_attempted_at: Time.current) | ||
| SyncAllProvidersJob.perform_later(family.id) | ||
| redirect_to settings_providers_path, notice: t("settings.providers.sync_all_in_progress") |
There was a problem hiding this comment.
Make the sync-all throttle atomic.
This check/update pair is race-prone: two concurrent POSTs can both pass Line 83, both write last_sync_all_attempted_at, and both enqueue SyncAllProvidersJob. That defeats the throttle and can hammer downstream providers.
Suggested fix
def sync_all
family = Current.family
+ throttled = false
- if family.last_sync_all_attempted_at.present? && family.last_sync_all_attempted_at > 30.seconds.ago
- return redirect_to settings_providers_path, notice: t("settings.providers.sync_all_recently")
- end
-
- family.update!(last_sync_all_attempted_at: Time.current)
+ family.with_lock do
+ throttled = family.last_sync_all_attempted_at.present? &&
+ family.last_sync_all_attempted_at > 30.seconds.ago
+ family.update!(last_sync_all_attempted_at: Time.current) unless throttled
+ end
+
+ return redirect_to(settings_providers_path, notice: t("settings.providers.sync_all_recently")) if throttled
+
SyncAllProvidersJob.perform_later(family.id)
redirect_to settings_providers_path, notice: t("settings.providers.sync_all_in_progress")
end🤖 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/controllers/settings/providers_controller.rb` around lines 80 - 89, The
sync_all action is race-prone because the present?/>update pair can be run
concurrently; make it atomic by performing a single conditional DB update on the
Family row and only enqueueing SyncAllProvidersJob when that update succeeds.
Replace the current check+family.update!(last_sync_all_attempted_at: ...) with a
conditional update scoped to Current.family (e.g. a where on Family.id and a
predicate that last_sync_all_attempted_at is nil or <= 30.seconds.ago) and
proceed to call SyncAllProvidersJob.perform_later(family.id) only if the
conditional update affected one row; otherwise redirect with the
"sync_all_recently" notice. Ensure you reference sync_all, Current.family,
last_sync_all_attempted_at, and SyncAllProvidersJob in the change.
| @@ -0,0 +1,12 @@ | |||
| <%# locals: (title:, count: nil, description: nil, anchor: nil) %> | |||
| <div id="<%= anchor %>" class="flex items-baseline justify-between gap-3 mt-2 mb-1 px-1"> | |||
There was a problem hiding this comment.
id="" renders invalid HTML when anchor is nil.
<%= nil %> outputs an empty string, producing id="". The HTML5 spec requires id values to be non-empty. The element also won't be targetable as an anchor.
🐛 Proposed fix
-<div id="<%= anchor %>" class="flex items-baseline justify-between gap-3 mt-2 mb-1 px-1">
+<div <%= "id=\"#{anchor}\"".html_safe if anchor.present? %> class="flex items-baseline justify-between gap-3 mt-2 mb-1 px-1">📝 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.
| <div id="<%= anchor %>" class="flex items-baseline justify-between gap-3 mt-2 mb-1 px-1"> | |
| <div <%= "id=\"#{anchor}\"".html_safe if anchor.present? %> class="flex items-baseline justify-between gap-3 mt-2 mb-1 px-1"> |
🤖 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/views/settings/providers/_group_heading.html.erb` at line 2, The div
currently always emits id="" when the local variable anchor is nil; change the
ERB so the id attribute is only rendered when anchor is present (e.g., use a
conditional like `if anchor.present?` or an inline ERB conditional) so the <div>
element does not produce an empty id and is only anchorable when a non-empty
anchor value exists; update the line that generates the div (the one referencing
anchor) to conditionally include the id attribute rather than always outputting
id.
| <%= button_to sync_provider_settings_providers_path(provider_key: provider_key), | ||
| method: :post, | ||
| disabled: recently_synced, | ||
| title: recently_synced ? t("settings.providers.recently_synced") : t("settings.providers.sync_provider"), | ||
| class: "inline-flex items-center p-1 rounded text-secondary hover:text-primary hover:bg-alpha-black-50 transition-colors disabled:opacity-40 disabled:cursor-not-allowed", | ||
| form: { onclick: "event.stopPropagation()", class: "inline-flex" } do %> | ||
| <%= icon "refresh-cw", class: "w-3.5 h-3.5" %> |
There was a problem hiding this comment.
Add an explicit accessible name to this icon-only button.
title is not a reliable accessible name, so screen-reader users may only get an unlabeled “button” here. Please add an aria-label (or sr-only text) that matches the sync action.
Suggested fix
<% recently_synced = last_synced_at.present? && last_synced_at > 60.seconds.ago %>
+<% button_label = recently_synced ? t("settings.providers.recently_synced") : t("settings.providers.sync_provider") %>
<%= button_to sync_provider_settings_providers_path(provider_key: provider_key),
method: :post,
disabled: recently_synced,
- title: recently_synced ? t("settings.providers.recently_synced") : t("settings.providers.sync_provider"),
+ title: button_label,
+ aria: { label: button_label },
class: "inline-flex items-center p-1 rounded text-secondary hover:text-primary hover:bg-alpha-black-50 transition-colors disabled:opacity-40 disabled:cursor-not-allowed",
form: { onclick: "event.stopPropagation()", class: "inline-flex" } do %>🤖 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/views/settings/providers/_sync_button.html.erb` around lines 3 - 9, The
button created by button_to (sync_provider_settings_providers_path) is icon-only
and needs an explicit accessible name; add an aria-label (or include hidden
sr-only text) that matches the sync action instead of relying on title. Update
the button_to call to include aria-label: recently_synced ?
t("settings.providers.recently_synced") : t("settings.providers.sync_provider")
(or render an adjacent sr-only <span> with the same translated text) so screen
readers receive a clear label while keeping the existing title, disabled,
provider_key, and form attributes intact.
|
CI root cause: Generated by Claude Code |
|
Great consolidation — the unified provider page and health strip are a meaningful UX improvement. One design-system violation to fix before merge:
<span class="text-xs font-bold text-white"><%= logo_text %></span>
Replace with the appropriate functional token — likely (The Generated by Claude Code Generated by Claude Code |
3 participants
Summary
Completes the "Option A" Claude Design redesign of the Bank Sync settings page.
DS::Dialogdrawer;Provider::Metadataregistry for all 11 providers; maturity pills also surface on connected accordion rows:off(Available) instead of:warn(Your connections)/settings/bank_sync: 301 permanent redirect; nav collapsed from two entries to a single "Bank Sync"; page title and lede refreshedTest plan
/settings/bank_sync→ 301 redirect to/settings/providersbin/rails testgreenDISABLE_PARALLELIZATION=true bin/rails test:systemgreen🤖 Generated with Claude Code
Summary by CodeRabbit