QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-08-03 23:52:13 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(accounts): make the sync toolbar and toast agree, fix toast overlap (#2813)

Browse Source 
* 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 <body>-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
<main> 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 <main> 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.
This commit is contained in:
Guillem Arias Fauste
2026-07-30 02:57:16 +02:00
committed by Juan José Mata
parent e2343fc9d2
commit 5bb9d0881f
12 changed files with 198 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
app/javascript/controllers/notification_tray_controller.js Normal file
View File

@@ -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 <main> catches every case that
// moves its bounds — window resize, sidebar drag-resize, sidebar
// collapse/expand — since all of those already reflow <main> 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();
}
}

15
app/models/family/sync_complete_event.rb
View File

@@ -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)

31
app/views/accounts/_sync_controls.html.erb Normal file
View File

@@ -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. %>
<div id="accounts-sync-controls" class="flex items-center gap-2 shrink-0">
<%= 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 %>
</div>

19
app/views/accounts/index.html.erb
View File

@@ -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" %>
<span><%= t(".cancel_sync") %></span>
<% 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),

4
app/views/layouts/application.html.erb
View File

@@ -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 %>
<div
class="flex flex-col lg:flex-row h-full bg-surface"
data-controller="app-layout privacy-mode sidebar-resize"
@@ -145,7 +145,7 @@ end %>
<% 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 %>
<div class="hidden lg:flex gap-2 items-center justify-between mb-6 sticky top-0 z-10 -mx-3 lg:-mx-10 px-3 lg:px-10 py-4 bg-surface border-b border-tertiary">
<div class="flex items-center gap-2">

4
app/views/layouts/settings.html.erb
View File

@@ -1,4 +1,4 @@
<%= render "layouts/shared/htmldoc" do %>
<%= render "layouts/shared/htmldoc", sidebar_aware: true do %>
<div class="flex flex-col md:flex-row h-full bg-surface pt-[env(safe-area-inset-top)]">
<%= link_to t("layouts.application.skip_to_main"), "#main",
class: "sr-only focus:not-sr-only focus:fixed focus:top-2 focus:left-2 focus:z-50 focus:px-3 focus:py-2 focus:rounded-lg focus:bg-container focus:text-primary focus:shadow-border-xs" %>
@@ -9,7 +9,7 @@
</div>
</div>
<main id="main" class="grow flex h-full">
<main id="main" class="grow flex h-full" data-notification-tray-target="main">
<div class="relative max-w-4xl mx-auto flex flex-col w-full h-full">
<div data-controller="settings-scroll" class="grow flex flex-col overflow-y-auto overflow-x-hidden overscroll-contain [-webkit-overflow-scrolling:touch]">
<div class="sticky top-0 z-10 px-3 md:px-10 pt-1.5 md:pt-3 pb-3 bg-surface border-b border-tertiary shrink-0">

24
app/views/layouts/shared/_htmldoc.html.erb
View File

@@ -1,3 +1,4 @@
<%# locals: (sidebar_aware: false) %>
<!DOCTYPE html>
<% theme = Current.user&.theme || "system" %>
@@ -13,20 +14,11 @@
<%= yield :head %>
</head>
<body class="h-[var(--app-height)] overflow-hidden antialiased">
<body class="h-[var(--app-height)] overflow-hidden antialiased"<% if sidebar_aware %> data-controller="notification-tray"<% end %>>
<% if Rails.env.development? %>
<button hidden data-controller="hotkey" data-hotkey="t t /" data-action="theme#toggle"></button>
<% end %>
<div class="fixed z-50 top-6 md:top-4 left-1/2 -translate-x-1/2 w-full md:w-80 px-4 md:px-0 mx-auto md:mx-0 md:right-auto pt-[env(safe-area-inset-top)]">
<div id="notification-tray" class="space-y-1 w-full">
<%= render_flash_notifications %>
<div id="sync-toast"></div>
<div id="cta"></div>
</div>
</div>
<% if Current.family %>
<%= turbo_stream_from Current.family %>
<% end %>
@@ -41,5 +33,17 @@
<%= render "impersonation_sessions/approval_bar" if Current.true_user&.impersonated_support_sessions&.initiated&.any? %>
<%= yield %>
<%# `left-1/2 -translate-x-1/2` centers on the viewport by default, correct
for every single-column layout. Layouts with a persistent sidebar
(settings, application) pass sidebar_aware: true, which wires up
notification_tray_controller.js to override `left` with the actual
content pane's center instead — see that controller for why a
ResizeObserver beats hand-rolled sidebar-width math, and for why it
also has to guard that inline style against turbo_refreshes_with's
morph reconciliation. %>
<div class="fixed z-50 top-6 md:top-4 left-1/2 -translate-x-1/2 w-full md:w-80 px-4 md:px-0 mx-auto md:mx-0 md:right-auto pt-[env(safe-area-inset-top)]"<% if sidebar_aware %> data-notification-tray-target="tray" data-action="turbo:before-morph-attribute->notification-tray#preserveStyle"<% end %>>
<%= render "layouts/shared/notification_tray" %>
</div>
</body>
</html>

9
app/views/layouts/shared/_notification_tray.html.erb Normal file
View File

@@ -0,0 +1,9 @@
<%# Rendered inside the fixed-position wrapper in _htmldoc.html.erb, which
floats above the page — see that file for how it centers itself on
layouts with a sidebar. %>
<div id="notification-tray" class="space-y-1 w-full">
<%= render_flash_notifications %>
<div id="sync-toast"></div>
<div id="cta"></div>
</div>

1
config/locales/views/accounts/ca.yml
View File

@@ -59,6 +59,7 @@ ca:
opening_balance_date_label: Data del saldo inicial
index:
accounts: Comptes
cancel_sync: Cancel·la la sincronització
manual_accounts:
other_accounts: Altres comptes
new_account: Nou compte

3
config/locales/views/shared/ca.yml
View File

@@ -29,6 +29,9 @@ ca:
money_field:
label: Import
require_admin: Només els administradors poden fer aquesta acció
sync_toast:
message: Hi ha dades noves disponibles
refresh: Actualitza
syncing_notice:
syncing: S'estan sincronitzant les dades dels comptes...
transaction_tabs:

24
test/models/family/sync_complete_event_test.rb Normal file
View File

@@ -0,0 +1,24 @@
require "test_helper"
class Family::SyncCompleteEventTest < ActiveSupport::TestCase
fixtures :families
test "broadcast replaces both the sync toast and the accounts page's own sync controls" do
family = families(:dylan_family)
family.expects(:broadcast_replace_to).with(
family,
target: "sync-toast",
partial: "shared/notifications/sync_toast"
).once
family.expects(:broadcast_replace_to).with(
family,
target: "accounts-sync-controls",
partial: "accounts/sync_controls",
locals: { family: family }
).once
Family::SyncCompleteEvent.new(family).broadcast
end
end

56
test/system/accounts_sync_ui_test.rb Normal file
View File

@@ -0,0 +1,56 @@
require "application_system_test_case"
class AccountsSyncUiTest < ApplicationSystemTestCase
setup do
@user = users(:family_admin)
sign_in @user
end
test "idle state shows a plain refresh trigger with no cancel control" do
visit accounts_path
within "#accounts-sync-controls" do
assert_selector "button[aria-label='#{I18n.t("accounts.index.sync")}']"
assert_no_text I18n.t("accounts.index.cancel_sync")
end
end
test "an in-progress family sync shows a spinner and a matching Cancel sync button" do
Sync.create!(syncable: @user.family, status: :syncing)
visit accounts_path
within "#accounts-sync-controls" do
# `icon(..., as_button: true, class: "animate-spin")` renders a
# DS::Button whose icon classes are fully internal to the component —
# the caller's `class:` lands on the button element itself, not the
# inner svg. Same visual result for a centered square icon button.
assert_selector "button.animate-spin[disabled]"
assert_selector "button", text: I18n.t("accounts.index.cancel_sync")
end
end
test "cancelling a sync shows a flash notice, and the tray centers on the content pane" do
Sync.create!(syncable: @user.family, status: :syncing)
visit accounts_path
within "#accounts-sync-controls" do
click_on I18n.t("accounts.index.cancel_sync")
end
assert_text I18n.t("syncs.cancel.cancelled")
# The tray is a fixed overlay, positioned by notification_tray_controller.js
# to center on <main> (the actual content pane next to the sidebar) rather
# than the full viewport — assert the two centers actually match, not just
# that the tray exists somewhere on screen.
tray_rect = page.evaluate_script("document.querySelector('[data-notification-tray-target=\"tray\"]').getBoundingClientRect()")
main_rect = page.evaluate_script("document.querySelector('#main').getBoundingClientRect()")
tray_center = tray_rect["left"] + tray_rect["width"] / 2.0
main_center = main_rect["left"] + main_rect["width"] / 2.0
assert_in_delta main_center, tray_center, 1.0,
"expected the tray (center: #{tray_center}) to align with the content pane (center: #{main_center}), not drift to viewport-center"
end
end