One notification system, and it fits a phone - #229
Merged
Conversation
Four phones on the real chrome - 44pt safe area, the 60pt header, the phase-24
dock - showing today's toast beside a card density for alerts and a compact
density for confirmations, in both appearance modes.
The colours are not redesigned. Every value is the shipping 23-03 token; the
only change is the 2px full ring becoming a 4px leading edge, which is what
lets the card stop being full-bleed.
What the investigation found, and why the sketch argues for two densities: the
26 toast calls are 14 success, 13 warning, 3 error, while the 32 SnackBars are
mostly "Link copied" and "Order ${status}". Those are two jobs. A bordered card
with a bold title and a close button is shouting a receipt at someone who just
tapped copy.
The app shipped two notification systems. The custom toast had 26 call sites; Material's SnackBar had 32 - and `snackBarTheme` was never defined, so those 32 rendered as the default grey Roboto pill at the bottom of the screen, under the dock. Half the app's messages were off-brand and nobody could see it from any one screen. Now there is `showToast(context, message)`. `showAppSnackBar` is deleted, and every one of its callers plus the six direct `showSnackBar` calls go through the same function. Density is chosen by whether there is a title: without one the toast is a confirmation and gets a compact pill, with one it is an alert and gets the card, the dismiss button and the longer read. Nothing passed an action - that parameter was dead - so there is no action slot. The mobile problems this fixes, none of them cosmetic: - `top: 100` was a literal, and the file had no reference to MediaQuery.padding anywhere, so the toast landed differently on every device. It now derives from padding.top + the header + space4. - It was full-bleed edge to edge, which is why it needed a 2px ring to look contained. Inset by the page gutter, the ring becomes a 4px leading edge. - The close target was ~28pt against the 44 phase 25 set for filter chips. - It was never announced. No Semantics at all, so VoiceOver and TalkBack users did not receive "Verification failed". That is the most serious item here and it is invisible in a screenshot. - The stack was uncapped at 85px a step: the sixth toast was off screen. Capped at three, oldest evicted, with a density-aware stride. - One 5s duration for everything, too long for "Link copied" and too short for a two-line error. - The 300ms slide ignored the OS reduce-motion setting. Off mobile it keeps a horizontal entrance and a real width ceiling rather than running the window: 420 at the top right, sliding in from the edge it is anchored to. The auto-dismiss timer moved from the manager onto the widget State. Held by the singleton it outlived its own overlay - a pending timer in 58 widget tests, and in the app a callback into a dead route. One test changed rather than being written around: the 23-03 contrast test asserted a SelectableText, and the message is a plain Text now, because text selection inside a five-second transient fights the swipe that dismisses it. The contrast claim it makes is untouched.
Three things from Braian's desktop walk. The title and message were coming out with a yellow double underline. That is Flutter's "no Material ancestor" fallback: a toast is inserted straight into the root Overlay, which has no Material above it, and the typography tokens set colour and size but not `decoration`, so the fallback's underline was inherited through the merge. Not a desktop problem - both branches render the same widget in the same overlay, and the old toast had the same exposure on its title; the message escaped only because SelectableText resolves its default differently, which is why swapping it for Text is what surfaced this. One `Material(type: transparency)` fixes it, and gives the 44pt dismiss button something to paint its ink into - it had nothing before. A test pins the decoration and fails with TextDecoration.underline without the wrapper. The 4px leading status edge is gone. The icon already carries the status, and the three glyphs differ by shape rather than colour - 1.4.1 is satisfied by the glyph - so the edge was a second coloured object saying nothing new. The card is two rows instead of two columns. The icon and the title are one statement and share a line; the message spans the full width beneath. Side by side, a 20px icon sat against a two-line block with dead space under it, and the message paid ~32px of indent it needs more on a phone. The title takes ellipsis now that it shares a row with the dismiss button. Also recorded: the dismiss button keeps its 44pt box and its glyph sits ~25px from the card edge. Pulling it flush wanted a negative right margin, which Container turns into a Padding and asserts on - analyze passes and every card toast throws at runtime. Caught by the tests, and the hack is gone rather than worked around. Sketch 079's manifest row carries all of this, since it draws a leading edge that no longer ships.
The migration scripts in the previous commit were Python, and Python's `write_text()` on Windows translates "\n" to "\r\n" without saying so. Twenty three source files came out CRLF, so every line of each read as changed: a whitespace diff big enough to bury the real one, and a CI quality failure that looked like a content problem. The repo is 1155 LF against 29 CRLF, and every CRLF file in this branch was one this branch created — so this restores what was already the convention rather than choosing one. `theme_contrast_test.dart` is deliberately left CRLF: it was already CRLF on develop, and normalising it here would put a 1420-line whitespace diff in a toast PR. Pinning the convention in `.gitattributes` and renormalising the 29 files that predate it is its own change, not this one.
braianxde
marked this pull request as ready for review
August 8, 2026 18:47
EduMenges
approved these changes
Aug 10, 2026
Contributor
|
@codex review |
![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: a96fc0c601
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// `OverlayEntry` holding a `Column`, at which point the stack lays itself out | ||
| /// and these disappear. Not done here because it rewrites the entry lifecycle | ||
| /// for a case only large text scale reaches. | ||
| const double _kCardStride = 84.0; |
There was a problem hiding this comment.
Whenever two titled toasts coexist, the second starts only 84 px below the first, but _Card is at least 92 px tall at normal text scale: 24 px vertical padding, a 44 px dismiss-button row, a 4 px gap, and a 20 px message line. The cards therefore overlap even before wrapping or large-text scaling increases the height; use layout-derived stacking rather than this stride.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment.
Confirmed, and the arithmetic is exactly yours: 24 padding + 44 dismiss row + 4 gap + 20 bodySm line = 92 against an 84 stride, so two cards overlapped by 8px before anything wrapped. The 44pt row arrived in 2850399, after the stride did, which is why the comment above it did not admit this.
Landed the constant at 96 rather than layout-derived stacking, deliberately: the laid-out fix is one shared OverlayEntry holding a Column, which rewrites the entry lifecycle the tests cover, and the remaining ceiling (a message that wraps, or large text scale) is now stated honestly in the ponytail comment.
Review on #229 found four real things and one misread. An error with no title got the compact pill: one ellipsized line, no dismiss button, gone in two seconds - which is how "Failed to load tokens. Check your connection and try again." loses the half that says what to do. ToastType.error now always gets the card, titled 'Error', the title bridge_screen had already written by hand. Seven call sites lost their severity in the migration. showAppSnackBar was a neutral grey pill, so an untyped showToast turned "Failed to load tokens" into a green check. swap_screen x2, custom_future_builder, handle_banxa_drawer, sdk_account_manager x2 and account_drawer now say which kind they are. _kCardStride was 84 against a card that is 92 at 1.0x - space6 padding twice, the 44pt dismiss row, space2, one bodySm line. Two stacked cards overlapped by 8px before anything wrapped, which the ponytail comment did not admit because the 44pt row arrived after the stride did. And the placement read its own breakpoint: a literal 600 where the app has GeniusBreakpoints. useDesktopLayout also forces the phone branch on native iOS/Android whatever the width, which is what the safe-area maths is there for. Codex's P1 is wrong: the generated packages are the root banxa/ and squidrouter/ (OpenAPI, gen-code.sh), not lib/banxa, which is app code both of us have hand-edited for months. flutter analyze 0 issues - flutter test 1216 passing, 3 skipped (was 1214/3) - brace, raw-colour, seed-safety, key-logging and agent-rules gates exit 0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The app had two notification systems, and you could only notice by visiting two different screens. Now it has one.
What changed
Verification
flutter analyze0 issues ·flutter test1199 passing, 3 skipped (develop: 1192/3) · brace, raw-colour, seed-safety and key-logging gates exit 0. Walked on Windows desktop in both appearance modes, and at phone width by resizing.Deliberately not here
check_agent_rules_sync.shfails. Pre-existing — identical on cleandevelopatfa74006d, and neitherAGENTS.mdnor the generated Copilot copy is in this diff.debugPrintonly. Its own change.