Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mikehardy
force-pushed
the
@mikehardy/properly-remove-android-messages
branch
from
8bd80f4 to
9f556ae
Compare
it was duplicated, which means any fixes have to be in multiple spots
previously the remove was not applied, so it likely never persisted to storage, leading to infinite growth and possible OOM
previously, stored messages were cleared one at a time each time a message came in, opening the possibility that the store would contain more messages then the limit and never get back down to the limit. now it will attempt to shrink down to the limit every time a message comes in ensuring that if there is an over limit condition it will be fixed quickly
previously we added before purging, so if close to an OOM we would trigger it, if an app update is pushed with this change, it will ideally recover completely from this situation by purging the store down before any new data is added
mikehardy
force-pushed
the
@mikehardy/properly-remove-android-messages
branch
from
9f556ae to
7e8975e
Compare
mikehardy
added
platform: android
plugin: messaging
mikehardy
commented
Nov 26, 2025
| preferences.setStringValue(S_KEY_ALL_NOTIFICATION_IDS, notificationIds); | ||
|
|
||
| // check and remove old notifications message | ||
| // remove old notifications message before store to free space as needed |
Collaborator
Author
There was a problem hiding this comment.
the diff for the 5th commit is easier to understand in split view, it's mostly code motion swapping the two chunks of logic, not code change
This was referenced Nov 26, 2025
sean5940
approved these changes
Nov 26, 2025
Contributor
sean5940
left a comment
sean5940
left a comment
There was a problem hiding this comment.
LGTM 👍🏻
This PR provides a necessary and correct fix for the recurring OOM risk caused by uncontrolled remote message storage in SharedPreferences.
MichaelVerdon
approved these changes
Nov 26, 2025
russellwheatley
approved these changes
Nov 26, 2025
mikehardy
removed
the
Workflow: Needs Review
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
4 participants
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.
Description
@sean5940 noticed a few issues with our remote message storage implementation while collaborating on the related PR
They are serious enough I'd like to get fixes for them out as soon as possible, independent of the work on that PR
There are 5 tiny commits here so that the problems were hopefully immediately obvious and the fixes are hopefully obviously correct
First two are just refactors for clarity.
The last three are the fixes
Related issues
Release Summary
two refactor commits / three fix commits, it will generate a fix release
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Hoping for @sean5940 to try these with the patch-package set that the PR run generates to see what he thinks, or you could pull the branch directly?
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter