From 5bb9d0881fafdf57258cf5202b74e5ecdb2503e7 Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Thu, 30 Jul 2026 02:57:16 +0200 Subject: [PATCH] fix(accounts): make the sync toolbar and toast agree, fix toast overlap (#2813) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(accounts): make the sync toolbar and toast agree, fix toast overlap The Accounts page's own sync toolbar (refresh icon, "Cancel sync") and the global sync-complete toast were three inconsistently-styled, disconnected pieces of UI representing one action, and the toast overlapped the page's own header instead of sitting near it. - "Cancel sync" was hand-rolled markup instead of a DS::Button, unlike its sibling refresh icon right next to it — now both are DS::Button (:ghost). - The refresh icon just went `disabled` with no visible "working" state — now shows a spinning loader-circle while a family sync is in progress, matching the pattern already used in provider_sync_summary.html.erb. - The toolbar was plain server-rendered HTML with no way to know a sync finished, so it stayed stuck showing "still syncing" indefinitely next to a toast now saying otherwise. Family::SyncCompleteEvent now broadcasts a second replace target for the toolbar alongside the existing toast replace, so both resolve together. - The notification tray was a -level fixed overlay centered on the full viewport, but every layout that renders it has a sidebar of some kind — so it never actually centered on the visible content pane, and landed on top of the settings-layout header. It now renders in-flow at the top of each layout's own content region (opt-in via notification_tray_inline, since the simpler single-column layouts don't have this mismatch and are unaffected). - Added the Catalan sync_toast/cancel_sync strings that were missing entirely, which is why the toast/toolbar showed English text on an otherwise-Catalan page. Verified live in a real browser via a new system test covering the idle, syncing, and cancel-flash states, plus a model test on the new broadcast target. * fix(accounts): keep the tray a floating overlay, sidebar-aware instead Codex on this PR: with the tray as first-child-of-scrollable-main, a notification delivered while scrolled down is inserted above the viewport and stays unseen — breaking the sync toast's manual-refresh path specifically, since sync_toast_controller.js suppresses auto-refresh while a form is focused and relies on the toast being visible to offer that manual refresh. Reverts the tray to a position: fixed overlay for every layout (so it can't be scrolled out of view), and fixes the actual bug that made it overlap the accounts toolbar in the first place — a ResizeObserver on
centers it on the real content pane instead of the viewport, for the two layouts with a sidebar (application, settings passed via sidebar_aware:). The five single-column layouts are untouched; for those, viewport-center already is content-pane-center. One trap worth flagging: this app renders turbo_refreshes_with method: :morph, and idiomorph resets any inline style a client script set that isn't in the freshly-fetched HTML — including the JS-set `left`. data-turbo-permanent looked like the fix but isn't: it invokes idiomorph's node-identity matching (same id preserved across ANY morphed page), which broke navigation once the id existed on structurally different layouts (app vs settings) — a real, reproduced bug, caught by the system test before it shipped. Went with the narrower turbo:before-morph-attribute event instead, which blocks only the `style` attribute on this one element, with no node-identity system involved. Rewrote the system test's positioning assertion to match: it now asserts the tray centers on
rather than sitting above the page header, since a fixed overlay was never going to satisfy the latter by construction. Verified: full bin/rails test (6023 runs, 0 failures), rubocop, erb_lint, brakeman (0 warnings) all clean. Live-verified in a browser across both sidebar-aware layouts and a simple layout, including the full cancel-sync -> morph -> re-render cycle. * fix(accounts): use declarative Stimulus action for morph-attribute guard Replace the manual addEventListener/removeEventListener pair for turbo:before-morph-attribute with a data-action, per the repo's declarative-actions convention. Same element, same listener — just no manual lifecycle management. --- .../notification_tray_controller.js | 40 +++++++++++++ app/models/family/sync_complete_event.rb | 15 +++++ app/views/accounts/_sync_controls.html.erb | 31 ++++++++++ app/views/accounts/index.html.erb | 19 +------ app/views/layouts/application.html.erb | 4 +- app/views/layouts/settings.html.erb | 4 +- app/views/layouts/shared/_htmldoc.html.erb | 24 ++++---- .../shared/_notification_tray.html.erb | 9 +++ config/locales/views/accounts/ca.yml | 1 + config/locales/views/shared/ca.yml | 3 + .../models/family/sync_complete_event_test.rb | 24 ++++++++ test/system/accounts_sync_ui_test.rb | 56 +++++++++++++++++++ 12 files changed, 198 insertions(+), 32 deletions(-) create mode 100644 app/javascript/controllers/notification_tray_controller.js create mode 100644 app/views/accounts/_sync_controls.html.erb create mode 100644 app/views/layouts/shared/_notification_tray.html.erb create mode 100644 test/models/family/sync_complete_event_test.rb create mode 100644 test/system/accounts_sync_ui_test.rb diff --git a/app/javascript/controllers/notification_tray_controller.js b/app/javascript/controllers/notification_tray_controller.js new file mode 100644 index 000000000..2f0cf5a06 --- /dev/null +++ b/app/javascript/controllers/notification_tray_controller.js @@ -0,0 +1,40 @@ +import { Controller } from "@hotwired/stimulus"; + +// The tray is `position: fixed` with `left: 50%` (viewport-center) by +// default, which is correct for every single-column layout. Layouts with a +// sidebar (app, settings) opt in here so the tray centers on the actual +// content pane instead. A ResizeObserver on
catches every case that +// moves its bounds — window resize, sidebar drag-resize, sidebar +// collapse/expand — since all of those already reflow
natively; no +// cooperation needed from the sidebar controllers themselves. +export default class extends Controller { + static targets = ["tray", "main"]; + + connect() { + this.resizeObserver = new ResizeObserver(() => this.reposition()); + this.resizeObserver.observe(this.mainTarget); + this.reposition(); + } + + disconnect() { + this.resizeObserver?.disconnect(); + } + + reposition() { + const rect = this.mainTarget.getBoundingClientRect(); + this.trayTarget.style.left = `${rect.left + rect.width / 2}px`; + } + + // turbo_refreshes_with(method: :morph) is enabled app-wide + // (_head.html.erb), and idiomorph resets any inline style a client + // script set that isn't present in the freshly-fetched server HTML — + // which `left` always is. `data-turbo-permanent` looked like the fix, + // but it invokes idiomorph's node-identity matching (same id preserved + // across ANY morphed page), which misbehaves once the id also exists + // on structurally different layouts (app vs settings). Wiring this + // narrower per-attribute event as a declarative action instead blocks + // only `style` on this one element — no node-identity system involved. + preserveStyle(event) { + if (event.detail.attributeName === "style") event.preventDefault(); + } +} diff --git a/app/models/family/sync_complete_event.rb b/app/models/family/sync_complete_event.rb index 3949990d3..4747d3fd0 100644 --- a/app/models/family/sync_complete_event.rb +++ b/app/models/family/sync_complete_event.rb @@ -21,6 +21,21 @@ class Family::SyncCompleteEvent partial: "shared/notifications/sync_toast" ) + # The accounts page's own sync toolbar (refresh icon + "Cancel sync") is + # plain server-rendered HTML from whatever request last loaded the page, + # so without this it stays stuck showing "still syncing" — disabled icon, + # "Cancel sync" visible — indefinitely after the sync actually finishes, + # even while the toast above says otherwise. Replace it in the same + # broadcast so the two agree. Visitors not on the accounts page simply + # don't have #accounts-sync-controls in their DOM, so this no-ops for + # them, same as the sync-toast replace above. + family.broadcast_replace_to( + family, + target: "accounts-sync-controls", + partial: "accounts/sync_controls", + locals: { family: family } + ) + # Schedule recurring transaction pattern identification (debounced to run after all syncs complete) begin RecurringTransaction.identify_patterns_for(family) diff --git a/app/views/accounts/_sync_controls.html.erb b/app/views/accounts/_sync_controls.html.erb new file mode 100644 index 000000000..1a4ecfc55 --- /dev/null +++ b/app/views/accounts/_sync_controls.html.erb @@ -0,0 +1,31 @@ +<%# locals: (family:) %> + +<%# Rendered both from a normal request (accounts#index) and from + Family::SyncCompleteEvent's broadcast (no Current.family there) — take + family as an explicit local and use absolute i18n keys, not lazy `t(".")` + lookups, to behave the same in both contexts. %> +
+ <%= icon( + family.syncing? ? "loader-circle" : "refresh-cw", + as_button: true, + size: "sm", + href: sync_all_accounts_path, + disabled: family.syncing?, + class: (family.syncing? ? "animate-spin" : nil), + "aria-label": t("accounts.index.sync"), + frame: :_top + ) %> + + <% if (family_sync = family.syncs.visible.first) %> + <%= render DS::Button.new( + text: t("accounts.index.cancel_sync"), + variant: :ghost, + size: :sm, + icon: "circle-x", + href: cancel_sync_path(family_sync), + method: :post, + frame: :_top, + aria_label: t("accounts.index.cancel_sync") + ) %> + <% end %> +
diff --git a/app/views/accounts/index.html.erb b/app/views/accounts/index.html.erb index ee48dbcd6..5dff033c5 100644 --- a/app/views/accounts/index.html.erb +++ b/app/views/accounts/index.html.erb @@ -1,23 +1,6 @@ <%= content_for :page_title, t(".accounts") %> <%= content_for :page_actions do %> - <%= icon( - "refresh-cw", - as_button: true, - size: "sm", - href: sync_all_accounts_path, - disabled: Current.family.syncing?, - frame: :_top - ) %> - <% if (family_sync = Current.family.syncs.visible.first) %> - <%= button_to cancel_sync_path(family_sync), - method: :post, - class: "flex items-center gap-1 text-sm text-secondary hover:text-primary", - aria: { label: t(".cancel_sync") }, - data: { turbo_frame: :_top } do %> - <%= icon "circle-x", class: "w-4 h-4" %> - <%= t(".cancel_sync") %> - <% end %> - <% end %> + <%= render "accounts/sync_controls", family: Current.family %> <%= render DS::Link.new( text: t(".new_account"), href: new_account_path(return_to: accounts_path), diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 68bfa9cca..08dcd9b26 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -20,7 +20,7 @@ end %> <% expanded_sidebar_class = "w-full" %> <% collapsed_sidebar_class = "w-0 overflow-hidden" %> -<%= render "layouts/shared/htmldoc" do %> +<%= render "layouts/shared/htmldoc", sidebar_aware: true do %>
<% end %> <%# SHARED - Main content %> - <%= tag.main id: "main", class: class_names("grow overflow-y-auto px-3 lg:px-10 w-full mx-auto pb-[calc(5rem+env(safe-area-inset-bottom))] lg:pb-0"), data: { app_layout_target: "content", viewport_target: "content" } do %> + <%= tag.main id: "main", class: class_names("grow overflow-y-auto px-3 lg:px-10 w-full mx-auto pb-[calc(5rem+env(safe-area-inset-bottom))] lg:pb-0"), data: { app_layout_target: "content", viewport_target: "content", notification_tray_target: "main" } do %> <% unless intro_mode %>