From ea51612ac710e6232aa32bc456facb647bff4349 Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Sat, 23 May 2026 09:23:30 +0200 Subject: [PATCH] refactor(views): migrate 6 residual inline alerts to DS::Alert (#1933) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(views): migrate 6 residual inline alerts to DS::Alert PR #1731 extended DS::Alert and migrated 9 inline alert blocks. Six hand-rolled alert blocks slipped through that sweep and stayed on raw palette tokens with no `theme-dark:` variants: - `app/views/settings/llm_usages/show.html.erb` — "About Cost Estimates" blue info block. Most visible offender: `bg-blue-50 border border-blue-200` + `text-blue-900 / text-blue-700 / text-blue-600` rendered as a bright white-blue island in dark mode (the bug spotted on the LLM usage page). - `app/views/accounts/confirm_unlink.html.erb` — yellow warning with bullet list. - `app/views/oidc_accounts/new_user.html.erb` — blue info heading. - `app/views/oidc_accounts/link.html.erb` — two blocks (yellow verify warning + blue create info). Also flips the file's pre-existing `text-gray-600` hint paragraph to `text-secondary` (caught by the `DeprecatedClasses` erb_lint rule on save). - `app/views/rules/confirm.html.erb` — AI cost notice. - `app/views/rules/confirm_all.html.erb` — AI cost notice. All six migrate to `DS::Alert.new(title:, variant:)` (with a block content slot for the rich/conditional bodies). DS::Alert resolves `bg-info/10`, `border-info/20`, etc. from the `@theme` semantic tokens, so dark mode now renders a subtle blue/yellow tint over the page surface instead of a hardcoded light-mode pill. Out of scope (left as-is, not alert-shaped): - `app/views/assistant_messages/_tool_calls.html.erb` — a tool-call display panel (not an alert; needs its own token sweep). - `app/views/import/rows/_form.html.erb` — inline cell-error tooltip (`bg-red-50 border border-red-200`) — also not alert-shaped; a future PR can swap it to `bg-destructive/10 border-destructive-subtle` once #1932 lands. Surfaced while scanning DS drift for the LLM usage page bug. Tracking issue: #1715 (closed but conceptually relevant) / #1911 (active drift patrol). * fix(oidc): keep alert description in

, retarget tests for DS::Alert title CI on #1933 caught three test failures introduced by migrating the two OIDC link alerts and the verify-redirect copy from hand-rolled `

` / `

` markup to `DS::Alert`: 1. `OidcAccountsControllerTest#test_should_show_create_account_option_for_new_user` 2. `OidcAccountsControllerTest#test_does_not_show_create_account_button_when_JIT_link-only_mode` 3. `SessionsControllerTest#test_redirects_to_account_linking_when_no_OIDC_identity_exists` DS::Alert renders its `title:` slot as a `

` (semantically the alert heading lives on the container's `aria-labelledby`, not on a heading tag) and renders block / message content directly inside a `

`, not a `

`. The pre-migration markup used `

` for the heading and `

` for the description, so the tests above asserted those specific tags. Two fixes: - `app/views/oidc_accounts/link.html.erb` — wrap the html_safe description bodies in explicit `

` tags inside the DS::Alert block. Restores the `

` element the session-redirect test asserts on, and keeps the description as a semantic paragraph rather than a bare text node inside the alert container. - `test/controllers/oidc_accounts_controller_test.rb` — flip the two `assert_select "h3", text: "Create New Account"` calls to match the DS::Alert title `

`. The test was asserting an implementation detail of the pre-migration markup; switching to the new tag keeps the assertion meaningful (the heading text still has to render) without re-introducing an `

` outside of DS::Alert. * fix(test): match Create New Account title with regex (sr-only "Info:" prefix) DS::Alert prepends `Info:` inside the title `

`, so the full text content is "Info: Create New Account", not "Create New Account". `assert_select "p", text: "Create New Account"` requires an exact text match and rejected the prefixed string. Switch to a regex match — keeps the heading-text assertion meaningful without coupling to the screen-reader prefix. --- app/views/accounts/confirm_unlink.html.erb | 22 ++++----- app/views/oidc_accounts/link.html.erb | 20 ++++---- app/views/oidc_accounts/new_user.html.erb | 11 +++-- app/views/rules/confirm.html.erb | 42 ++++++++--------- app/views/rules/confirm_all.html.erb | 46 +++++++++---------- app/views/settings/llm_usages/show.html.erb | 17 +++---- .../oidc_accounts_controller_test.rb | 4 +- 7 files changed, 71 insertions(+), 91 deletions(-) diff --git a/app/views/accounts/confirm_unlink.html.erb b/app/views/accounts/confirm_unlink.html.erb index 5b77c4808..e61cfd925 100644 --- a/app/views/accounts/confirm_unlink.html.erb +++ b/app/views/accounts/confirm_unlink.html.erb @@ -6,19 +6,15 @@ <%= t("accounts.confirm_unlink.description_html", account_name: @account.name, provider_name: @account.provider_name) %>

-
-
- <%= icon "alert-triangle", class: "w-4 h-4 text-yellow-600 mt-0.5 flex-shrink-0" %> -
-

<%= t("accounts.confirm_unlink.warning_title") %>

-
    -
  • <%= t("accounts.confirm_unlink.warning_no_sync") %>
    • -
  • <%= t("accounts.confirm_unlink.warning_manual_updates") %>
    • -
  • <%= t("accounts.confirm_unlink.warning_transactions_kept") %>
    • -
  • <%= t("accounts.confirm_unlink.warning_can_delete") %>
    • -
-
-
+
+ <%= render DS::Alert.new(title: t("accounts.confirm_unlink.warning_title"), variant: :warning) do %> +
    +
  • <%= t("accounts.confirm_unlink.warning_no_sync") %>
    • +
  • <%= t("accounts.confirm_unlink.warning_manual_updates") %>
    • +
  • <%= t("accounts.confirm_unlink.warning_transactions_kept") %>
    • +
  • <%= t("accounts.confirm_unlink.warning_can_delete") %>
    • +
+ <% end %>
<%= render DS::Button.new( diff --git a/app/views/oidc_accounts/link.html.erb b/app/views/oidc_accounts/link.html.erb index c4bf1aaf3..ba15010d5 100644 --- a/app/views/oidc_accounts/link.html.erb +++ b/app/views/oidc_accounts/link.html.erb @@ -3,12 +3,11 @@ %> <% if @user_exists %> -
-

<%= t("oidc_accounts.link.verify_heading") %>

-

+

+ <%= render DS::Alert.new(title: t("oidc_accounts.link.verify_heading"), variant: :warning) do %> <% email_suffix = @pending_auth["email"].present? ? t("oidc_accounts.link.email_suffix_html", email: @pending_auth["email"]) : "" %> - <%= t("oidc_accounts.link.verify_description_html", provider: @pending_auth["provider"], email_suffix: email_suffix).html_safe %> -

+

<%= t("oidc_accounts.link.verify_description_html", provider: @pending_auth["provider"], email_suffix: email_suffix).html_safe %>

+ <% end %>
<%= styled_form_with url: create_link_oidc_account_path, class: "space-y-4", data: { turbo: false } do |form| %> @@ -26,18 +25,17 @@ placeholder: t("oidc_accounts.link.password_placeholder"), autocomplete: "current-password" %> -
+

<%= t("oidc_accounts.link.verify_hint") %>

<%= form.submit t("oidc_accounts.link.submit_link") %> <% end %> <% else %> -
-

<%= t("oidc_accounts.link.create_heading") %>

-

- <%= t("oidc_accounts.link.create_description_html", email: @pending_auth["email"], provider: @pending_auth["provider"]).html_safe %> -

+
+ <%= render DS::Alert.new(title: t("oidc_accounts.link.create_heading"), variant: :info) do %> +

<%= t("oidc_accounts.link.create_description_html", email: @pending_auth["email"], provider: @pending_auth["provider"]).html_safe %>

+ <% end %>
diff --git a/app/views/oidc_accounts/new_user.html.erb b/app/views/oidc_accounts/new_user.html.erb index 4656d4dba..6a7f365f6 100644 --- a/app/views/oidc_accounts/new_user.html.erb +++ b/app/views/oidc_accounts/new_user.html.erb @@ -2,11 +2,12 @@ header_title t("oidc_accounts.new_user.title") %> -
-

<%= t("oidc_accounts.new_user.heading") %>

-

- <%= t("oidc_accounts.new_user.description", provider: @pending_auth["provider"]) %> -

+
+ <%= render DS::Alert.new( + title: t("oidc_accounts.new_user.heading"), + message: t("oidc_accounts.new_user.description", provider: @pending_auth["provider"]), + variant: :info + ) %>
<%= styled_form_with model: @user, url: create_user_oidc_account_path, class: "space-y-4", data: { turbo: false } do |form| %> diff --git a/app/views/rules/confirm.html.erb b/app/views/rules/confirm.html.erb index 13947e59f..775846f6a 100644 --- a/app/views/rules/confirm.html.erb +++ b/app/views/rules/confirm.html.erb @@ -15,31 +15,25 @@ <% if @rule.actions.any? { |a| a.action_type == "auto_categorize" } %> <% affected_count = @rule.affected_resource_count %> -
-
- <%= icon "info", class: "w-4 h-4 text-blue-600 mt-0.5 flex-shrink-0" %> -
-

<%= t(".ai_cost_title") %>

- <% if @estimated_cost.present? %> -

- <%= t(".ai_cost_with_estimate_html", count: affected_count, cost: sprintf("%.4f", @estimated_cost)) %> -

- <% else %> -

- <%= t(".ai_cost_no_estimate_html", count: affected_count) %> - <% if @selected_model.present? %> - <%= t(".cost_unavailable_model", model: @selected_model) %> - <% else %> - <%= t(".cost_unavailable_no_provider") %> - <% end %> - <%= t(".cost_warning") %> -

- <% end %> -

- <%= link_to t(".view_usage_history"), settings_llm_usage_path, class: "underline hover:text-blue-800" %> +

+ <%= render DS::Alert.new(title: t(".ai_cost_title"), variant: :info) do %> + <% if @estimated_cost.present? %> +

<%= t(".ai_cost_with_estimate_html", count: affected_count, cost: sprintf("%.4f", @estimated_cost)) %>

+ <% else %> +

+ <%= t(".ai_cost_no_estimate_html", count: affected_count) %> + <% if @selected_model.present? %> + <%= t(".cost_unavailable_model", model: @selected_model) %> + <% else %> + <%= t(".cost_unavailable_no_provider") %> + <% end %> + <%= t(".cost_warning") %>

-
-
+ <% end %> +

+ <%= link_to t(".view_usage_history"), settings_llm_usage_path, class: "text-link underline" %> +

+ <% end %>
<% end %> diff --git a/app/views/rules/confirm_all.html.erb b/app/views/rules/confirm_all.html.erb index c88778767..d3b230766 100644 --- a/app/views/rules/confirm_all.html.erb +++ b/app/views/rules/confirm_all.html.erb @@ -9,32 +9,28 @@

<% if @rules.any? { |r| r.actions.any? { |a| a.action_type == "auto_categorize" } } %> -
-
- <%= icon "info", class: "w-4 h-4 text-blue-600 mt-0.5 flex-shrink-0" %> -
-

<%= t("rules.apply_all.ai_cost_title") %>

- <% if @estimated_cost.present? %> -

- <%= t("rules.apply_all.ai_cost_message", transactions: @total_affected_count) %> - <%= t("rules.apply_all.estimated_cost", cost: sprintf("%.4f", @estimated_cost)) %> -

- <% else %> -

- <%= t("rules.apply_all.ai_cost_message", transactions: @total_affected_count) %> - <% if @selected_model.present? %> - <%= t("rules.apply_all.cost_unavailable_model", model: @selected_model) %> - <% else %> - <%= t("rules.apply_all.cost_unavailable_no_provider") %> - <% end %> - <%= t("rules.apply_all.cost_warning") %> -

- <% end %> -

- <%= link_to t("rules.apply_all.view_usage"), settings_llm_usage_path, class: "underline hover:text-blue-800" %> +

+ <%= render DS::Alert.new(title: t("rules.apply_all.ai_cost_title"), variant: :info) do %> + <% if @estimated_cost.present? %> +

+ <%= t("rules.apply_all.ai_cost_message", transactions: @total_affected_count) %> + <%= t("rules.apply_all.estimated_cost", cost: sprintf("%.4f", @estimated_cost)) %>

-
-
+ <% else %> +

+ <%= t("rules.apply_all.ai_cost_message", transactions: @total_affected_count) %> + <% if @selected_model.present? %> + <%= t("rules.apply_all.cost_unavailable_model", model: @selected_model) %> + <% else %> + <%= t("rules.apply_all.cost_unavailable_no_provider") %> + <% end %> + <%= t("rules.apply_all.cost_warning") %> +

+ <% end %> +

+ <%= link_to t("rules.apply_all.view_usage"), settings_llm_usage_path, class: "text-link underline" %> +

+ <% end %>
<% end %> diff --git a/app/views/settings/llm_usages/show.html.erb b/app/views/settings/llm_usages/show.html.erb index dfb11ead1..39929a55b 100644 --- a/app/views/settings/llm_usages/show.html.erb +++ b/app/views/settings/llm_usages/show.html.erb @@ -165,16 +165,11 @@
- -
-
- <%= icon "info", class: "w-5 h-5 text-blue-600 mt-0.5" %> -
-

<%= t(".cost_estimates_title") %>

-

- <%= t(".cost_estimates_description") %> -

-
-
+
+ <%= render DS::Alert.new( + title: t(".cost_estimates_title"), + message: t(".cost_estimates_description"), + variant: :info + ) %>
diff --git a/test/controllers/oidc_accounts_controller_test.rb b/test/controllers/oidc_accounts_controller_test.rb index bd38aedcc..a24079931 100644 --- a/test/controllers/oidc_accounts_controller_test.rb +++ b/test/controllers/oidc_accounts_controller_test.rb @@ -115,7 +115,7 @@ class OidcAccountsControllerTest < ActionController::TestCase get :link assert_response :success - assert_select "h3", text: "Create New Account" + assert_select "p", text: /Create New Account/ assert_select "strong", text: new_user_auth["email"] end @@ -128,7 +128,7 @@ class OidcAccountsControllerTest < ActionController::TestCase get :link assert_response :success - assert_select "h3", text: "Create New Account" + assert_select "p", text: /Create New Account/ # No create account button rendered assert_select "button", text: "Create Account", count: 0 assert_select "p", text: /New account creation via single sign-on is disabled/