Skip to content

Bank sync — design review cleanup#1713

Closed
gariasf wants to merge 1 commit intowe-promise:feat/surface-provider-statusfrom
gariasf:pr-1710-design-review
Closed

Bank sync — design review cleanup#1713
gariasf wants to merge 1 commit intowe-promise:feat/surface-provider-statusfrom
gariasf:pr-1710-design-review

Conversation

@gariasf
Copy link
Copy Markdown
Collaborator

@gariasf gariasf commented May 9, 2026

Summary

Implements the agreed scope of Claude Design's review of #1710 — block-tier DS fixes plus the UX simplifications we landed on. Targeted at #1710's head branch so it can fold in before merge.

DS / i18n

  • text-whitetext-inverse on the provider logo glyph.
  • Card hover: drop shadow-border-sm lift; switch to bg-container-hover (DS card hover is colour-only).
  • Sentence-case the page title (Bank SyncBank sync); nav label follows.
  • i18n the page title, lede, and provider maturity badges (Beta / Alpha) — they were hard-coded English strings.
  • Decouple settings/_section from settings/providers/_status_pill: callers pass a pre-rendered status_pill local instead of a raw status string, matching the actions: / badge: pattern.

UX simplifications

  • Drop the four health-summary tiles (per-row warn/err pills already surface the signal at the scale this app sees). Removes Settings::HealthSummary, the @health_counts controller block, and the unused health.* locale keys.
  • Hide the "Your connections" heading + empty-state line when no providers are connected — the lede already invites a connect.
  • Whole-tile click target on each provider card (was an inner "Connect →" link only — hit-target inversion).
  • Drop the redundant "Free" tier from per-card meta lines (printed 10× for one fact); "Paid" still surfaces on Plaid.

Out of scope per the review's suggested merge order

  • Search-input replacement of the "Add another provider" CTA — own PR.
  • Drawer-footer status cleanup across the eleven panels — own PR.
  • Per-provider raw-color palette in Provider::Metadata — bigger discussion.

Test plan

  • bin/rails test (1 pre-existing env error — libvips missing for an Active Storage variant test, unrelated)
  • DISABLE_PARALLELIZATION=true bin/rails test test/system/settings/providers_test.rb — 10 runs, 0 failures
  • DISABLE_PARALLELIZATION=true bin/rails test test/system/settings_test.rb — 4 runs, 0 failures
  • bin/rubocop -f github -a — clean
  • bundle exec erb_lint ./app/views/settings/_section.html.erb ./app/views/settings/providers/show.html.erb ./app/components/settings/provider_card.html.erb -a — clean
  • bin/brakeman --no-pager — 0 warnings

Implements the agreed scope of Claude Design's review of maybe-finance#1710 —
block-tier DS fixes plus the UX simplifications we landed on.

DS / i18n
- Logo glyph: text-white -> text-inverse.
- Card hover: drop the shadow-border-sm lift; switch to
  bg-container-hover (DS card hover is colour-only).
- Sentence-case the page title ("Bank Sync" -> "Bank sync"); nav
  label follows.
- i18n the page title, lede, and provider maturity badges (Beta /
  Alpha) — they were hard-coded English strings.
- Decouple settings/_section from settings/providers/_status_pill:
  callers pass a pre-rendered status_pill local instead of a raw
  status string, matching the actions/badge pattern.

UX simplifications
- Drop the four health-summary tiles. Per-row warn/err pills already
  surface the signal at the scale this app sees (1–4 connections);
  removes Settings::HealthSummary, the @health_counts block, and the
  unused health.* locale keys.
- Hide the "Your connections" heading + empty-state line when no
  providers are connected — the lede already invites a connect.
- Whole-tile click target on each provider card (was an inner
  "Connect ->" link only — hit-target inversion).
- Drop the redundant "Free" tier from per-card meta lines (printed
  10x for one fact); "Paid" still surfaces on Plaid.

Out of scope per the review's suggested merge order: search-input
replacement of the "Add another provider" CTA, drawer-footer status
cleanup across the eleven panels, and the per-provider raw-color
palette discussion.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 9, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cf7c77fc-6d91-485b-82c6-358bf8fb815d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brin-security-scanner brin-security-scanner Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 9, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d0ae6b3c4

ℹ️ 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".

api_keys_label: API Key
appearance_label: Appearance
bank_sync_label: Bank Sync
bank_sync_label: Bank sync
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep Bank sync casing consistent in settings navigation

Changing bank_sync_label to sentence case updates the sidebar/page heading, but the next/previous footer cards still read from SETTINGS_ORDER where this entry remains hard-coded as "Bank Sync" (app/helpers/settings_helper.rb:5). Users will see mixed labels for the same destination depending on which navigation control they use, and localization remains bypassed in that footer path.

Useful? React with 👍 / 👎.

@gariasf gariasf closed this May 9, 2026
@gariasf
Copy link
Copy Markdown
Collaborator Author

gariasf commented May 9, 2026

Closing — write access propagated, replaying remaining design-review items directly on feat/surface-provider-status on top of jjmata's review-feedback commit.

@gariasf gariasf deleted the pr-1710-design-review branch May 9, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant