Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: a3a9b02f39
ℹ️ 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".
| final gone = [ | ||
| for (final n in state) | ||
| if (n.isAnnouncement && !wantedIds.contains(n.id)) n.id, | ||
| ]; |
There was a problem hiding this comment.
On a cold start where an announcement expired while the app was closed, the first Rust snapshot can reach this method before loadInitialData() finishes. Because gone is computed only from the still-empty notifier state, the expired card is not deleted, and an empty snapshot returns immediately. The pending load then restores that card from Sembast, potentially including its unread critical badge, until another snapshot arrives. Coordinate reconciliation with hydration or reconcile against persisted records so cards absent from Rust's authoritative snapshot cannot be restored by the initial load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79179cd: the notifier keeps the addresses of Rust's last snapshot (_announced); loadInitialData skips every persisted announcement outside it and deletes those records, so a snapshot that lands before the load answers wins. Test a card Rust no longer lists is not brought back by a load in flight.
| recent.sort(criticalFirst); | ||
| return NoticeSections(needsAction: pinned, recent: recent); |
There was a problem hiding this comment.
When a trade needs action, sectionNotices() puts it in needsAction, and NotificationsScreen._buildBody() renders that entire section before recent. Sorting critical announcements only within recent therefore leaves security updates below every actionable trade, potentially off-screen. This contradicts spec §3.4's requirement that critical announcements sort above everything, specifically so users see security warnings before continuing a trade. Render critical announcements ahead of both trade sections rather than only sorting the recent section.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79179cd: NoticeSections has a critical section of its own, and the screen renders it before the needs-action section and the timeline. Test a critical announcement sits above a trade that needs action. Spec §6.2 updated.
|
@coderabbitai review |
|
Step 4 of specs/006 §10. Rust's snapshots become system cards keyed by address: a correction replaces its card unread, a swept address loses it, Rust's read flag wins. The banner paints the level with the Activity chips (lime / amber / red), labels it, and offers the one link as a button; the copy follows the locale on screen. A dismissed critical stays listed, read, sorts first and turns the bell's dot red. Seven locales of chrome.
…ust's snapshot Review round 1 (#890). - A critical announcement was sorted first within the timeline only, so a trade that needs action still rendered above it. It has a section of its own, rendered before both. - Rust's first snapshot could land before the initial load answered, and the load then restored a card the snapshot no longer listed. The notifier keeps the addresses of the last snapshot; a load drops, and deletes, every persisted announcement outside it.
a3a9b02 to
79179cd
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79179cd242
ℹ️ 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".
| state = | ||
| byId.values.toList() | ||
| ..sort((a, b) => b.timestamp.compareTo(a.timestamp)); |
There was a problem hiding this comment.
On cold start, if loadInitialData() finishes while reconciliation is awaiting store.saveAll(changed), hydration adds the persisted trade and chat notices to state, but this assignment replaces them with byId, captured before those awaits. Loads run outside _mutationTail, so serializing mutations does not prevent this race. The existing notification history and its unread count disappear until another hydration or restart, even though the records remain on disk. Rebuild the map from the current state after persistence completes, then apply the announcement changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae00c90: after the store writes the merge starts from the state as it is then, not from the map captured before the awaits, so a hydration that landed meanwhile keeps its trade and chat notices. Test a load that lands while a snapshot is being persisted keeps its notices.
…rsisting `reconcileAnnouncements` rebuilt the list from a map captured before its store writes. Loads run outside the mutation queue, so a hydration that finished during those writes had its trade and chat notices replaced by that stale map until the next load or restart. The merge now starts from the current state once the writes are done. Test: a load that lands while a snapshot is being persisted keeps its notices.
|
@codex review |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae00c90036
ℹ️ 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".
| final card = state.where((n) => n.id == id).firstOrNull; | ||
| if (card != null && card.isAnnouncement) { | ||
| await _dismissAnnouncement(card); |
There was a problem hiding this comment.
If a correction arrives while the swipe's Undo snackbar is open, NotificationsScreen._dismiss still hides the address and later calls delete(id) when the snackbar closes. This lookup then selects the new revision rather than the one the user swiped, and _dismissAnnouncement reports that correction as dismissed to Rust. Non-critical corrections disappear without ever being shown; critical corrections lose their unread security badge. Capture the swiped revision and verify it before committing the delayed dismissal so a replacement remains unread and visible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 262e804: _dismiss records the swiped announcement's revision and passes it to delete(id, announcementRevision:), which leaves a card whose revision differs alone — unread, nothing reported to Rust. Tests skips a correction that replaced the swiped revision and dismisses the revision that was swiped when it is still current.
|
@coderabbitai review |
|
…ped card The Undo snack bar deletes by id when it closes. A correction that landed meanwhile sits at the same address, so the delete found the new revision and reported it dismissed to Rust: a non-critical correction vanished unseen, a critical one lost its unread badge. The screen now records the swiped revision and `delete` compares it with the card's: a different one is left alone, unread, and Rust hears nothing.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Linked issue
Part of #319 (step 4 of
specs/006-announcement-channel/spec.md§10). Stacked on #889; the last PR of the stack closes the issue.Type
feat
What changes and why
Rust's announcement snapshots become cards in Notifications. Each card is keyed by the announcement's address, so a correction (a new revision at the same address) replaces it unread, an address Rust no longer lists loses its card, and Rust's read flag wins. The banner paints the level with the Activity chips —
infolime,warningamber,criticalred — labels it (Announcement / Notice / Security update), shows the copy of the locale on screen, and offers the one link as a button opened externally; the body is never a link. Acriticalsorts above the timeline, turns the bell's dot red, and stays listed, read, once swiped away or on "Clear all". Nothing interrupts: no dialog, no snackbar, no takeover.Affected flow
Notifications screen and the bell (
lib/features/notifications/…,lib/shared/widgets/notification_bell.dart): tapping an announcement reads it and opens nothing; swiping dismisses it (acriticalstays, read); its link button opens the browser. Startup (lib/core/app_bootstrap.dart) subscribesonAnnouncementsChangedbefore the relay pool starts and reads the cache once. No trade flow, no order status.Blast radius
notifications_v2records (type: system,detailholds the revision, level, link andloc:<lang>:title|body);fromJsonof every older record is unchanged. They bypass theprocessed_eventsledger on purpose: Rust is the authority on what exists, andreconcileAnnouncementsmakes the store match each snapshot.NotificationsNotifier.delete/deleteAll/_markReadlearned about announcement cards; every other card takes the same path as before (notifications_*_test.dart, 197 tests, unchanged and green).sectionNoticessorts acriticalannouncement first; every other entry keeps its newest-first order.criticalas it does for the backup reminder; the count badge otherwise.url_launcheris already a dependency and used the same way by About.Platforms tested
Not run on a device by the author of this diff.
flutter analyze,flutter test,dart tool/design_check.dart --base feat/announcements-bridge(no breaks in the 7 files touched) on Linux.Manual testing
Setup
Needs the whole stack built (this branch includes #889) and an allowlisted key, which only #892 adds — so run these on #892's branch, or temporarily add your own
npubtoALLOWLISTin a local build. Publish withcargo run --bin announce(#891) andnak, to the default relays.Steps
infoannouncement with a link. Expected: the bell gets a count badge; Notifications shows a lime card labelled ANNOUNCEMENT with the title and body in the phone's language, and an "Open link" button.idwith a changed body. Expected: the card updates and is unread again.criticalone. Expected: red card labelled SECURITY UPDATE, sorted first; the bell's dot is red. Swipe it away: it stays, read. "Clear all": it stays.warningwithexpires_in_daysof a few minutes; wait past it; resume the app. Expected: the card is gone.main, and the backup-reminder style system banner looks as before.Not covered
iOS, web; a locale this build does not ship.
Screenshots
Not taken (no device run). Design-guide rules touched: DS-COL-1, DS-COL-11 (the banner left
AppColorsforOrderBookPalette/ActivityPalette), DS-CMP-8 (radius 18, padding 14), DS-TYP-4 (10/11/12/13 on the scale).Automated tests
test/features/notifications/announcement_cards_test.dart(16 tests): the card picks the locale's copy with English as fallback, is keyed by address with its revision and link, an unknown level paints as warning, each level has its label, a round trip through the store; a dismissedcriticalkeeps its card and the other levels lose it; reconcile adds, removes and replaces (a correction is unread again, the same revision keeps the card, Rust's read flag wins); read and dismiss reach Rust, a swipedcriticalstays read, "Clear all" keeps acritical;sectionNoticessorts acriticalfirst; the banner shows the level, the Spanish copy and the link, a plain system notice keeps its amber; the screen's tap reads and opens nothing; the bell turns red for acriticaland counts aninfo.flutter test:+3122 ~4 -2(the two10d_notifications_unsupportedgoldens, from #889's row, regenerated by the workflow).Checklist
cargo …: no Rust change in this PRflutter analyzeandflutter testpass locally except the goldens above.arb: regeneratedapp_localizations*.dartcommitted in the same commitAI assistance
Written with Claude Code, the whole diff. Look hardest at
NotificationsNotifier.reconcileAnnouncements/_dismissAnnouncement/deleteAlland at the banner's palette use.