docs: add Notifications System comprehensive spec - #33
Conversation
- New NOTIFICATIONS_SYSTEM.md covering complete notification architecture: - Data models (NotificationModel + 7 NotificationTypes) - Services (PushNotificationService, FCMService, BackgroundNotificationService) - State management (NotificationsNotifier + providers) - Persistence (NotificationsHistoryRepository) - NotificationsScreen UI (/notifications route) - Push flow: registration, token refresh, unregistration - Security (Phase 3 plaintext, Phase 5 encrypted tokens planned) - Integration with settings, trades, deep links - Updated README.md with NOTIFICATIONS_SYSTEM.md entry - Linked /notifications route in NAVIGATION_ROUTES.md to spec - Added cross-references table entry
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces comprehensive documentation for the v1 notification system architecture, including persistent in-app history and FCM-based push delivery. The documentation is added as a new reference file with corresponding updates to navigation routes and the documentation index. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.specify/v1-reference/README.md (1)
67-67: Tighten V2 relevance wording for notifications to match the migration section.Line 67 may read as conflicting with the later note that notifications stay in Flutter. Consider narrowing this to: “FCM/platform handling in Flutter; event/domain mapping shared with Rust core (where applicable).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.specify/v1-reference/README.md at line 67, The table row for NOTIFICATIONS_SYSTEM.md currently implies full notification handling in Rust/Flutter; update the wording in that row (the table entry for [NOTIFICATIONS_SYSTEM.md]) to the suggested tighter phrasing: “FCM/platform handling in Flutter; event/domain mapping shared with Rust core (where applicable).” Replace the existing cell content exactly with that sentence to align with the migration note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.specify/v1-reference/NOTIFICATIONS_SYSTEM.md:
- Line 40: Replace the inconsistent transport-security wording: locate the
phrase "**Purpose:** Register device tokens with Mostro push server (plaintext
HTTP in Phase 3)." and the other occurrence that says "encrypted in transit via
HTTPS" and change both to a single clear sentence such as "HTTPS transport with
plaintext registration fields" (or equivalent wording) so both references
consistently state that the connection is over HTTPS while the registration
payload fields are sent in plaintext.
---
Nitpick comments:
In @.specify/v1-reference/README.md:
- Line 67: The table row for NOTIFICATIONS_SYSTEM.md currently implies full
notification handling in Rust/Flutter; update the wording in that row (the table
entry for [NOTIFICATIONS_SYSTEM.md]) to the suggested tighter phrasing:
“FCM/platform handling in Flutter; event/domain mapping shared with Rust core
(where applicable).” Replace the existing cell content exactly with that
sentence to align with the migration note.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c43f1b2-04ce-464a-9f78-77d7f9f967d4
📒 Files selected for processing (3)
.specify/v1-reference/NAVIGATION_ROUTES.md.specify/v1-reference/NOTIFICATIONS_SYSTEM.md.specify/v1-reference/README.md
- NOTIFICATIONS_SYSTEM.md: unified transport security wording to 'HTTPS transport with plaintext registration fields' (consistent across both occurrences) - README.md: updated NOTIFICATIONS_SYSTEM.md table row with tighter migration guidance
Summary
Documents the complete Notifications System (point 12 from the feature list) including in-app notification history, push delivery via FCM, server registration, and background handling.
New File: NOTIFICATIONS_SYSTEM.md
Comprehensive spec covering:
Architecture Components
1. Data Models
NotificationModel— id, type, action, title, message, timestamp, isRead, orderId, dataNotificationTypeenum with 7 categories:2. Services Layer
PushNotificationServiceFCMService— Firebase Cloud Messaging integration (see FCM_IMPLEMENTATION.md)BackgroundNotificationService— Process notifications when app inactive3. State Management
NotificationsNotifiermanaging notification list + unread countNotificationsState— immutable state (list, unread, loading)4. Persistence
NotificationsHistoryRepository— local database (Sembast)5. Utilities
notification_message_mapper— Action + payload → localized title/messagenotification_data_extractor— Extract orderId, disputeId, deep-link dataNotificationsScreen (/notifications)
Features:
NotificationItem Widget:
Push Notification Flow
Registration:
Token Refresh:
Unregistration:
Security
Phase 3 (Current):
Phase 5 (Planned):
Updated Files
README.md
NAVIGATION_ROUTES.md
Cross-References
All specs properly linked:
No orphaned documents.
Summary by CodeRabbit