-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LOW: [$250] Stop using reportAction.originalMessage
or reportAction.message.text
#39797
Comments
text
from the reportActions
sent to the client to reduce payload sizetext
from the reportActions
sent to the client to reduce payload size
Job added to Upwork: https://www.upwork.com/jobs/~01856d1399295ef760 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 ( |
Hey I’m going to investigate why the payload is so big in Onyx storage. Possibly track what change exactly caused the problem. Then either revert that piece of change or use one of suggestions by @thienlnam Contributor details |
📣 @maxim-grin! 📣
|
Hey I’m going to investigate why the payload is so big in Onyx storage. Possibly track what change exactly caused the problem. Then either revert that piece of change or use one of suggestions by @thienlnam Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
ProposalPlease re-state the problem that we are trying to solve in this issue.We don't need to use What is the root cause of that problem?New change.
What changes do you think we should make in order to solve the problem?We have text defined here: App/src/types/onyx/ReportAction.ts Lines 18 to 19 in f150229
Example usage:
We can change this to:
Add the above in a common method Finally, we can remove There's one instance of
|
ProposalPlease re-state the problem that we are trying to solve in this issue.We want to remove the usage of What is the root cause of that problem?This is new feature request. What changes do you think we should make in order to solve the problem?
In here, replace
There're some places that are using
After phase 2 is done, we can just change the implementation of
We might need to replace them with Where
To be compatible with both current format of What alternative solutions did you explore? (Optional)NA |
Are you saying to literally repeat this code snippet in all places? Couldn't we refactor the code to avoid this repetition? As the scope of the issue is well-defined and rather narrow, I believe this part of the plan can be a part of the proposal. Aren't you quoting browser-specific local notifications code? 🤔 As I understand it, the ultimate goal is to optimize the payload size of remote push notifications. |
@cubuspl42 In the OP it's mentioned
So I'm quoting the place that we're showing notification based on It's only 1 part of the proposal though, what do you think about the rest of my proposal?
@cubuspl42 My alternative solution achieves this |
ProposalUpdated. @cubuspl42 We can add a method in ReportUtils, updated the proposal. |
Would you summarize the Expensify notifications architecture in a few sentences so that we can be more confident with proposal preparation and review? As I understand that most (if not all?) notification processing on mobile Native is server-side (push-based), and we're talking about optimizing the pushed payload size. But I'm not 100% confident how that relates to the Onyx schema. Also, there were some doubts about the scope...
Would you clarify this too? My intuition was that we wanted to nuke all usage of the |
text
from the reportActions
sent to the client to reduce payload sizereportAction.message
and only use reportAction.originalMessage
Great question. I've updated the original post with a more detailed description of the problem and solutions. Thanks! |
reportAction.message
and only use reportAction.originalMessage
reportAction.originalMessage
or reportAction.message.text
Proposal |
Updated the proposal again (sorry!!!) in light of |
Proposal updated after latest requirements update |
@nkdengineer I think your proposal shows more preparation. How is your capacity, though? Would you be able to start working on this within 24-48 hours? |
@cubuspl42 Yes, I definitely can |
I approve the proposal by @nkdengineer C+ reviewed 🎀 👀 🎀 |
Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat ( |
@parasharrajat reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks. PR is |
reportAction.originalMessage
or reportAction.message.text
reportAction.originalMessage
or reportAction.message.text
Sorry, I don't have the bandwidth to work on this. I was OOO since Thursday. let me request someone to push this forward. |
@c3024 will review this. |
Thanks @parasharrajat , 👋 @c3024 |
reportAction.originalMessage
or reportAction.message.text
reportAction.originalMessage
or reportAction.message.text
This issue has not been updated in over 15 days. @thienlnam, @c3024, @nkdengineer eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
@c3024 looks like the PR is awaiting an action from you |
The issue talks about only
but Also in phase 2
Can you please update phase 1 and 2 of the issue description to include these? |
Triggered auto assignment to @puneetlath ( |
Added |
I've updated the description |
I did some exploratory work on this topic in #41872 - maybe useful for you @nkdengineer @c3024 @thienlnam. The main idea is that we make |
self-assigning as I've been pulled into the PR for review |
@puneetlath, @thienlnam, @roryabraham, @c3024, @nkdengineer Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@puneetlath, @thienlnam, @roryabraham, @c3024, @nkdengineer Huh... This is 4 days overdue. Who can take care of this? |
Problem:
Each comment in the server database is stored in
reportAction.message
in HTML form. However, for some reason lost to time, when we send that same comment out as an Onyx update, we:reportAction.originalMessage
in the update (which is confusing)message
object that contains a copy of thehtml
message, as well as a strippedtext
version.message
object inside a totally unnecessary one-element arrayThat means every comment is actually sent out three times (four, if you include the update to the
report
object, but that's out of scope for now). This is clearly wasteful in a number of ways:Additionally, it's particularly problematic when sending Onyx updates out via UrbanAirship, which is extremely limited in the payload sizes allowed -- with anything over a certain limit just dropped quietly and never delivered, causing "gaps" in our update stream (which require more network calls to "backfill" the Onyx data on app open, which makes things slow). Basically, this was just a mistake introduced for reasons we can't remember, but that we want to undo.
Solution:
To solve this, please:
originalMessage
, start using array-message
originalMessage.html
to usemessage.html || message[0].html
message.text
to dynamically strip the text frommessage.html || message[0].html
originalMessage
ormessage.text
or any other key within originalMessage:originalMessage
message
without an arrayThis issue is for phase 1 and phase 3.
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: