fix: store group notifications LID mappings#2617
Conversation
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGroup notification handling now collects LID↔PN mappings from acting participants, group owners, participant list changes, and membership requests; the handler is async and persists collected mappings via signalRepository.lidMapping.storeLIDPNMappings after processing. ChangesGroup Notification LID/PN Mapping Collection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Socket/messages-recv.ts`:
- Around line 966-968: handleGroupNotification currently awaits the ancillary
call signalRepository.lidMapping.storeLIDPNMappings(lidPnMappings) which can
throw and abort the main notification flow; change this so mapping persistence
is isolated: wrap the call to
signalRepository.lidMapping.storeLIDPNMappings(...) (and the lidPnMappings
check) in a try/catch inside handleGroupNotification (or immediately around the
existing await), log any error but do not rethrow, or run the store call
asynchronously without awaiting it so transient failures don’t cause
handleGroupNotification/handleNotification to reject and prevent the synthetic
group event upsert from occurring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5cdb7b3a-fe88-4e40-966c-aeb0c318988d
📒 Files selected for processing (1)
src/Socket/messages-recv.ts
jlucaso1
left a comment
There was a problem hiding this comment.
The changes seem good to me. Could you add some tests to ensure I don't regret it later? I'm thinking of refactoring all of these lid-pn learning paths later, as we're unnecessarily duplicating a lot of logic
…ated + WhiskeySockets#2617 follow-up (#500) fix(newsletter): empirical PR WhiskeySockets#2434 + WhiskeySockets#2617 follow-up + auto-image link upgrade
Summary by cubic
Persist LID↔PN mappings from group notifications to fix missing ID resolution in group events. Group
w:gp2notifications now extract and save mappings for all relevant participants, and continue processing even if mapping storage fails.handleGroupNotificationasync and awaited in thew:gp2path.signalRepository.lidMapping.storeLIDPNMappings; on failure, log and proceed without dropping the notification.LIDMappingtype and appliedisLidUser/isPnUserguards to avoid invalid entries.Written for commit 722af7f. Summary will update on new commits.
Summary by CodeRabbit