fix(accounts): make the sync toolbar and toast agree, fix toast overlap#2813
fix(accounts): make the sync toolbar and toast agree, fix toast overlap#2813gariasf wants to merge 2 commits into
Conversation
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.
📝 WalkthroughWalkthroughThe PR extracts account sync controls into a reusable partial, updates them after sync completion broadcasts, and adds sidebar-aware notification tray positioning to application and settings layouts. Catalan translations and model/system coverage are included. ChangesSync UI flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FamilySync as Family::SyncCompleteEvent
participant Turbo as Turbo Stream
participant Accounts as Accounts page
FamilySync->>Turbo: Replace sync toast
FamilySync->>Turbo: Replace account sync controls
Turbo->>Accounts: Render updated sync state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 8c28f4ef4d
ℹ️ 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".
|
|
||
| <%# 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 %> | ||
| <%= render "layouts/shared/notification_tray" %> |
There was a problem hiding this comment.
Because the tray is now the first child of the scrollable main element (and is similarly placed inside the settings scroll container), any notification delivered while the user is farther down a page is inserted above the viewport and remains unseen due to scroll anchoring. This particularly breaks the sync toast's mid-form path: sync_toast_controller.js deliberately suppresses auto-refresh while a form is focused and expects the visible toast to offer a manual refresh, but a user editing a form below the fold now receives neither a visible prompt nor refreshed data. Keep asynchronously delivered notifications sticky/fixed within the content pane, or otherwise ensure the scroll position cannot hide them.
Useful? React with 👍 / 👎.
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Good root-cause work — tracing the toast/toolbar overlap back to "three independently-styled pieces of UI representing one sync action, only one of which was updated by the completion broadcast" is the right level of fix rather than just repositioning the toast.
A few things worth calling out as done well:
- Extracting
accounts/_sync_controlsand passingfamily:explicitly (rather thanCurrent.family) is correct and necessary —Family::SyncCompleteEvent#broadcastruns outside a request, soCurrent.familywouldn't be set there. The comment explaining this is helpful for future readers. - The
content_for :notification_tray_inlineopt-in keeps the fix scoped to the two layouts that actually have the sidebar-mismatch problem, and leaves the five single-column layouts' floating behavior untouched — reasonable, low-blast-radius approach. - Deriving both the icon spinner state and the "Cancel sync" visibility from
family.syncing?/family.syncs.visible.firstin the same partial, broadcast-replaced alongside the toast, directly fixes the "toolbar stuck showing syncing after the toast already says done" bug. - Test coverage (system test asserting the tray's bounding rect sits above the header, model test asserting both broadcast targets fire) matches the two behaviors being fixed.
The PR description already flags the remaining sync_toast/cancel_sync locale gap in the other ~15 language files as pre-existing and out of scope — agreed that's reasonable to defer separately rather than block this fix on unrelated translation work.
Generated by Claude Code
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/javascript/controllers/notification_tray_controller.js (1)
27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the Turbo morph handler in the template.
Turbo dispatches
turbo:before-morph-attributeon the element being morphed, so the tray can handle it through a Stimulusdata-actionwithout manual listener lifecycle management. (turbo.hotwired.dev)
app/javascript/controllers/notification_tray_controller.js#L27-L32: remove the manual add/remove listener calls.app/views/layouts/shared/_htmldoc.html.erb#L45-L45: adddata-action="turbo:before-morph-attribute->notification-tray#preserveStyle"alongside the conditional tray target.As per coding guidelines, use declarative actions in Stimulus controllers rather than imperative event listeners.
🤖 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/javascript/controllers/notification_tray_controller.js` around lines 27 - 32, Replace the manual turbo:before-morph-attribute add/remove listener calls in notification_tray_controller.js, including connect and disconnect, with a declarative Stimulus action. In app/views/layouts/shared/_htmldoc.html.erb line 45, add data-action="turbo:before-morph-attribute->notification-tray#preserveStyle" alongside the conditional tray target so preserveStyle remains invoked during morphing.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@app/javascript/controllers/notification_tray_controller.js`:
- Around line 27-32: Replace the manual turbo:before-morph-attribute add/remove
listener calls in notification_tray_controller.js, including connect and
disconnect, with a declarative Stimulus action. In
app/views/layouts/shared/_htmldoc.html.erb line 45, add
data-action="turbo:before-morph-attribute->notification-tray#preserveStyle"
alongside the conditional tray target so preserveStyle remains invoked during
morphing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13e41fe9-79cb-454d-937d-d38624c318a5
📒 Files selected for processing (6)
app/javascript/controllers/notification_tray_controller.jsapp/views/layouts/application.html.erbapp/views/layouts/settings.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/shared/_notification_tray.html.erbtest/system/accounts_sync_ui_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/layouts/application.html.erb
- app/views/layouts/shared/_notification_tray.html.erb
- test/system/accounts_sync_ui_test.rb
2 participants
Summary
Screenshots showed a "New data available" toast crowding the Accounts page's own sync toolbar (refresh icon, "Cancel sync", "+ Nou compte"). Tracing the full click-to-completion flow showed the overlap is a symptom of a bigger issue: the same "sync my accounts" action was represented by three inconsistently-styled, disconnected pieces of UI that don't agree with each other while a sync runs.
DS::Button(:ghost,:sm) instead of hand-rolled markup — it was the only non-DS::Button control in a toolbar where its sibling refresh icon already was one.loader-circlewhileCurrent.family.syncing?, instead of just goingdisabledwith no other visible change. Mirrors the existing pattern inprovider_sync_summary.html.erb.#sync-toastslot was broadcast-replaced onFamily::SyncCompleteEvent, so the toolbar (disabled icon + "Cancel sync") stayed stuck showing "still syncing" indefinitely next to a toast that had already announced completion. The event now also broadcasts a replace for the extractedaccounts/sync_controlspartial, so both update together.<body>-levelfixedoverlay centered on the full browser viewport, but every layout that renders it has a sidebar of some kind (settings nav, icon rail, resizable app sidebars) — so it never actually centered on the visible content pane, and just happened to land on top of the settings-layout header. It now renders in-flow at the top of each layout's own content region. This is opt-in per layout (content_for :notification_tray_inline) — the five simpler single-column layouts (auth, onboarding, wizard, imports, blank) don't have this sidebar mismatch and keep the original floating behavior unchanged.sync_toast/cancel_syncstrings —shared.sync_toastdidn't exist inca.ymlat all, which is why the toast/toolbar showed English text on an otherwise-Catalan page.Note: this same gap exists for
sync_toast/cancel_syncin most of the other 15 locale files (not just Catalan) — pre-existing and out of scope here since I can't reliably hand-author translations for languages I don't speak; flagging for a separate i18n pass.Before / After
Captured against fixture/demo data, not production data.
Shown at 4× crop for the toolbar pair so the change is visible; the loader icon looks like a partial ring because it's a still frame of a spinning animation, not a rendering issue.
Test plan
test/system/accounts_sync_ui_test.rb) covering: idle state (no spinner, no Cancel sync), in-progress state (spinner + matching Cancel sync button), and the positioning fix itself — asserts the notification tray's bounding rect sits above the page header rather than overlapping it.test/models/family/sync_complete_event_test.rb) asserting both broadcast targets fire with correct target/partial/locals.bin/rails test— 6023 runs, 0 failures, 0 errors.erb_linton all touched templates — clean.bin/rubocopon touched/new Ruby files — clean.bin/brakeman— 0 warnings.Summary by CodeRabbit