fix(contacts): hole in condition checking for unknown contact message#7517
Merged
Merged
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR tightens the “latest pending contact request” lookup logic so Accept/Dismiss can proceed even when persistence returns an empty contact request ID (e.g., due to the underlying message being missing), and adds regression tests for that edge case.
Changes:
- Treat an empty
contactRequestIDfrom persistence as “no pending request” and fall back todefaultContactRequestID(...). - Ensure the returned error is cleared when falling back to the default ID.
- Add tests covering accept/dismiss behavior when the contact request message is missing from persistence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
protocol/messenger_contacts.go |
Updates latest-contact-request-ID retrieval to fall back to a default ID when persistence returns an empty ID. |
protocol/messenger_contact_requests_test.go |
Adds regression tests for accepting/dismissing when the contact request message has been deleted/missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Jenkins Builds
|
jrainville
force-pushed
the
fix/missing-contact-request-message
branch
from
June 3, 2026 18:27
180fbb3 to
05501ff
Compare
caybro
approved these changes
Jun 3, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/10.34.x #7517 +/- ##
==================================================
Coverage ? 61.21%
==================================================
Files ? 851
Lines ? 119270
Branches ? 0
==================================================
Hits ? 73007
Misses ? 38768
Partials ? 7495
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes status-im/status-app#21067
I had previously done a fix that addresses this issue: #7165
But turns out there was still a hole in the logic, because the persistence could return just an empty ID, so it would skip my bypass and just error later. Plus, I had forgotten to reset the error.
This fix addresses the hole and adds tests.
I wasn't able to actually test this e2e, because the bug is almost impossible to reproduce, since it requires paired devices and lost messages.