Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughFour notification event types now use generated network models for parsing and domain mapping. The change removes their dedicated DTOs and updates event adapters, mappings, and test fixtures. ChangesNotification event migration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The migrated notification events preserve the expected data for the inspected payloads, with no identified issue that needs to be fixed before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The existing notification delivery path and user identity checks remain in place. The main uncertainty is compatibility: stricter payload requirements can reject notifications previously accepted, and complete recovery from missed or repeated events is not established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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. A rabbit checks each event’s name, Comment |
aleksandar-apostolov
left a comment
aleksandar-apostolov
left a comment
There was a problem hiding this comment.
The generated NotificationThreadMessageNewEvent makes message_id, thread_id and watcher_count required, and the fixture had to grow all three — but none of them reach the domain event. Does that mean a notification.thread_message_new missing any one of them now gets dropped instead of mapped? If the spec marks them required because feeds share the event, would defaulting the ones we don't read be safer?
|
We decided to keep the generated models strict on Android and fix the producer when one is wrong, rather than default fields. None of the three reach the domain event, so a default wouldn't change what apps see; it would only hide a producer sending a malformed event. |
This branch has not been deployed
2 participants
Goal
Parse the
notification.thread_message_new,notification.mark_unread,notification.mutes_updatedandnotification.channel_mutes_updatedevents with the generated event models.Part of AND-1291
Implementation
NotificationThreadMessageNewEvent,NotificationMarkUnreadEvent,NotificationMutesUpdatedEventandNotificationChannelMutesUpdatedEvent, and delete the hand-written DTOs.notification.thread_message_new; the cid, user, first unread message, last read date and unread messages onnotification.mark_unread), and reject an event missing one.notification.thread_message_newmaps its channel and message with the generated channel and message mappers, and the mute events mapmewith the generated own user mapper.Testing
GeneratedNotificationStateEventParsingTestcovers each event, a nanosecondcreated_at, and a missing required field.On a device: received a new thread reply notification, a channel and a thread marked unread, and a user and a channel muted. Each event carried the right channel, message, user, unread counts and mutes.
🤖 Generated with Claude Code
Summary by CodeRabbit