Skip to content

feat(announcements): dismissible home announcement carousel - #2484

Merged
i5hi merged 4 commits into
payjoin-salvagefrom
pj/07-announcements
Jul 24, 2026
Merged

feat(announcements): dismissible home announcement carousel#2484
i5hi merged 4 commits into
payjoin-salvagefrom
pj/07-announcements

Conversation

@ethicnology

Copy link
Copy Markdown
Member

What

Adds a paged carousel of dismissible announcements on the wallet home, shown between the balance header and the wallet list. It ships two compile-time announcements:

  • Payjoin privacy — appears once the wallet has transaction history and payjoin is still off, nudging the user to enable it.
  • Autoswap active — appears while autoswap is enabled, so the user is aware and can learn what it does.

How it works

  • Each card is tappable to its target (payjoin / autoswap settings) and dismissible via an explicit × with a Read / Dismiss confirmation dialog; Read opens the same target as tapping the card.
  • Dismissals persist in the dismissed_announcements table, with a per-announcement policy (permanent or snooze).
  • The carousel adapts its height to the user's text-scale setting and collapses to nothing when no announcement is visible (including the moment the last one is dismissed).
  • Built on bull_ui (BullInfoCard + PageView), wired via a thin cubit that re-evaluates on payjoin-setting changes and on wallet-sync completion (so the payjoin card shows up after a fresh recovery once the wallet syncs).

Architecture

Self-contained feature under lib/features/announcements/ following the standard ui → cubit → usecase → repository → datasource layering:

  • domain/Announcement entity + catalog (compile-time registry with trigger predicates), sealed AnnouncementsFailure, GetVisibleAnnouncementsUsecase / DismissAnnouncementUsecase returning Result.
  • data/ — dismissal repository + datasource over the dismissed_announcements Drift table (model + mapper).
  • presentation/ — thin AnnouncementsCubit + state + *_l10n extensions.
  • ui/AnnouncementCarousel, AnnouncementCard, dismiss dialog.

Notes

Adds a paged carousel of dismissible announcements on the wallet home,
shown between the balance header and the wallet list. Ships two
compile-time announcements: a payjoin-privacy nudge (once the wallet has
transaction history and payjoin is off) and an autoswap-active notice.

Each card is tappable to its target (payjoin/autoswap settings) and
dismissible via an explicit x with a Read/Dismiss confirmation; the
Read option opens the same target as tapping. Dismissals persist in the
dismissed_announcements table with a permanent/snooze policy per
announcement. The carousel adapts its height to the text scale and
collapses to nothing when no announcement is visible.

Built on bull_ui (BullInfoCard + PageView) and wired via a thin cubit
that re-evaluates on payjoin-setting changes and wallet-sync completion.
@ethicnology ethicnology self-assigned this Jul 21, 2026
@ethicnology
ethicnology changed the base branch from develop to pj/00-settings-db July 21, 2026 02:07
@ethicnology

Copy link
Copy Markdown
Member Author

@claude review this PR

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 4m 1s —— View job


Code review — feat(announcements): dismissible home announcement carousel

  • Read repo guidelines (AGENTS.md / ARCHITECTURE.md)
  • Review PR diff against pj/00-settings-db
  • Review domain layer (entities, catalog, usecases)
  • Review data layer (repository, datasource, mapper)
  • Review presentation/UI layer (cubit, carousel, dialog)
  • Review tests and localization
  • Post review feedback

Overall this is a well-structured feature: clean ui → cubit → usecase → repository → datasource layering, forward-compatible handling of unknown dismissal ids on downgrade, exhaustive switches over closed enums for l10n, mounted checks after awaits, subscription cleanup in close(), and solid domain test coverage. One architecture-rule violation and a few robustness items below.


🔴 Architecture: domain/ imports another feature's ui/ internals

lib/features/announcements/domain/entities/announcement_catalog.dart:2 imports package:bb_mobile/features/settings/ui/settings_router.dart to build NavigateAction(SettingsRoute.payjoinSettings.name).

This breaks two binding rules from AGENTS.md:

  • Rule Use Sync in isolate #1 — never import another feature's internals (ui/, domain/, presentation/); only a public/ facade is importable. I checked: no other feature's domain/ imports another feature's ui/ — this would be the first.
  • Domain stays Flutter-freesettings_router.dart pulls in GoRouter/Flutter, so the announcements domain layer is no longer pure Dart (the Announcement doc comment itself promises domain/ stays Flutter-free).

The comment on NavigateAction says route names avoid coupling to path layout, but the import itself is the coupling. Suggested fix, mirroring what you already do for strings in announcement_l10n.dart: keep the domain semantic (drop the route name from the catalog, or have NavigateAction carry nothing/an abstract target) and resolve AnnouncementId → SettingsRoute in the ui layer, e.g. an announcement_navigation.dart extension next to the carousel with an exhaustive switch (announcement.id). Fix this →

🟡 Robustness

  1. emit after close is possibleAnnouncementsCubit.refresh() and dismiss() (announcements_cubit.dart:52-67) emit after an await with no isClosed guard. An in-flight refresh completing after the cubit closes throws a StateError. Add if (isClosed) return; before the emits.
  2. No coalescing of sync-triggered refreshes — every finished wallet sync fires a full refresh() (settings fetch + all wallet transactions + autoswap settings + DB read). With several wallets syncing back-to-back you get redundant, overlapping loads, and two overlapping execute()s can emit out of order (stale-last). A small debounce or an in-flight guard on refresh() would cover both.
  3. Perf nitGetVisibleAnnouncementsUsecase loads the entire transaction list just to compute transactions.isNotEmpty (get_visible_announcements_usecase.dart:34). Fine for now, but a cheap "has any transaction" query would avoid materializing every tx on each refresh. The four awaits could also run in parallel (Future.wait / records) since they're independent.

🟡 Failure modeling (AGENTS.md rule #11)

  • AnnouncementUnexpectedFailure is declared but never constructed — both usecases map every throw to AnnouncementStorageFailure, including non-storage errors from settings/tx/autoswap sources (get_visible_announcements_usecase.dart:63). Either use AnnouncementUnexpectedFailure as the catch-all (matching its doc comment) or drop it.
  • The failure is held in state but never rendered: nothing consumes state.failure or announcements_failure_l10n.dart, so a failed dismissal is silent (card just stays). Probably acceptable for a nudge — but then the l10n extension is dead code; either wire a snackbar on dismissal failure or trim the unused plumbing.

🟢 Minor / nits

  • Rule Trigger wallet sync after import or recovery #14 folder shape — inverted in both directions: two usecases sit directly in domain/ (a second file of a kind means domain/usecases/ should exist), while data/datasources/, data/models/, data/mappers/ are each a folder of one file (single files should live directly in data/ with the role suffix).
  • UTC mismatch — the Drift table doc says dismissedAt is UTC, but the repository writes local DateTime.now() (announcement_dismissal_repository_impl.dart:25) and the usecase compares against local now. Self-consistent today, but it contradicts the docs and makes future snooze intervals fuzzy across timezone changes — use .toUtc() on both sides (or fix the table comment).
  • assert-based invariants are debug-onlyAnnouncement, NavigateAction, SnoozeDismiss enforce invariants via assert, which is stripped in release builds; the tests pin AssertionError. Given the repo's "self-validating entity" stance (rules Add app version at bottom of settings page #6/When a BIP21 description is added to invoice, make the private label the same as description #9), consider throwing ArgumentError instead so invariants hold in release too.
  • Dead code left behindWalletHomeState.showAutoSwapActiveStatus() (lib/features/wallet/presentation/bloc/wallet_state.dart:83) has no callers after its card was removed from HomeWarnings; remove it in this PR since this PR orphaned it.
  • FEATURES.md scope — besides the ANNOUNCEMENTS edges, the diff adds PAYJOIN → LABELS, RECEIVE → SETTINGS, RECEIVE → TX_HISTORY, SEND → TX_HISTORY. If those are graph corrections for pre-existing deps, fine — just confirming they're intentional in this PR.
  • Tests — domain coverage is good (triggers, dismissal suppression, snooze re-arm, priority ordering, failure paths). Cheap gaps worth filling: the mapper's unknown-id → null forward-compat behavior (it's a subtle, deliberate contract), and a cubit test for dismiss-refresh flow.

Not verified

No Flutter/fvm toolchain is available on this runner and no CI runs were reported for this PR yet, so I could not run make analyze / make unit-test — findings are from code reading. The required this._field private named parameters and @Deprecated-free sealed hierarchies all look consistent with the Dart 3.10+ toolchain the repo pins.


Summary: solid, well-tested feature; the one thing I'd block on is the domain → settings/ui import in the catalog (rule #1 + Flutter-free domain), with the isClosed guard and refresh coalescing as strong recommendations.

@ethicnology ethicnology linked an issue Jul 21, 2026 that may be closed by this pull request
@ethicnology

Copy link
Copy Markdown
Member Author

@bullock can you assess and fix the review made in the previous comment ?

@github-actions

Copy link
Copy Markdown

🐂 Done — Bullock opened #2497 targeting pj/07-announcements. Review it before merging.

@i5hi
i5hi changed the base branch from pj/00-settings-db to payjoin-salvage July 24, 2026 08:55
@i5hi
i5hi merged commit 68c037b into payjoin-salvage Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

payjoin improments

3 participants