Profile - Timezone changes are not updated on secondary device until page refresh

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Login with expensifail account on https://staging.new.expensify.com/ on 2 devices
2. Open Settings/Profile on both devices
3. Change Timezone on primary device and click Save

Expected Result:

Timezone should update on secondary device right-away.

Actual Result:

Timezone is not updated on secondary device until page refresh.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.21.4

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/198678376-3c6e3da9-d14d-47ee-be4b-4f92021e3b4f.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @tjferriss (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[tjferriss]

I was able to reproduce the bug on staging between the web app and mobile app.

Though I'm going to tag in engineering here for a few questions...

• Is this really a bug? I'm not sure I expect any app to immediately refresh like this, though I rarely if ever test apps out like this. I mean in an ideal world changes should update this quickly without a refresh so I love the goal here.
• I was able to reproduce this bug with other fields (e.g. first name, last name, pronoun) so I don't think we should focus on only fixing the timezone. If we fix this bug I think we should fix it across the app. How would that even work?
• Finally, per the SO I'm really not sure if this is an internal or external bug so any guidance there would be helpful.

[melvin-bot[bot]]

Triggered auto assignment to @aldo-expensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[aldo-expensify]

We are sending the pusher events to update the personal details on all clients in the backend, the problem is that the UI does not update even if the personal details in Onyx change because of the pushed update.

This could be worked External, but I'll do it myself to work a bit on contributions.

[aldo-expensify]

I drafted a PR which seems to do the work, but I still want to confirm that it is going in the right direction

[aldo-expensify]

• Is this really a bug? I'm not sure I expect any app to immediately refresh like this, though I rarely if ever test apps out like this. I mean in an ideal world changes should update this quickly without a refresh so I love the goal here.

I think your are right on that most apps wouldn't support this, but in our case, the support is 95% there since we are already sending the updates to all devices through the Pusher

• I was able to reproduce this bug with other fields (e.g. first name, last name, pronoun) so I don't think we should focus on only fixing the timezone. If we fix this bug I think we should fix it across the app. How would that even work?

Agreed on that we should fix all fields in this form if we decide to fix one. The problem is the form inputs not updating when we receive new data from the pusher because at some point we decided they would be "uncontrolled" for performance reasons.

[aldo-expensify]

I'm not entirely sure of if we want to do fix this or not as I think it has very low ROI. @JmillsExpensify can I have your opinion on this 🙏 ?

[JmillsExpensify]

So I kind of think we have to solve this, especially as we're moving to infinite sessions right? Otherwise you make a change on mobile and then the change never propagates to desktop. Is that right? The issue is that it's kind of a big one, would require a pre-design and need to be (initially) handled internally.

[aldo-expensify]

So I kind of think we have to solve this, especially as we're moving to infinite sessions right? Otherwise you make a change on mobile and then the change never propagates to desktop. Is that right? The issue is that it's kind of a big one, would require a pre-design and need to be (initially) handled internally.

It is not that the changes don't completely propagate, the data is sent to all device correctly, it is just the UI in this case that is not updating when the device receives new data. If the user closes the personal details modal and opens it again, it will show the most up to date data, or if the user didn't have the personal details modal open, and they open it after updating the personal data in another device, they will also see the most up to date data.

[aldo-expensify]

Lowering the priority while we decide what to do. I also think this a rare edge case.

[trjExpensify]

at some point we decided they would be "uncontrolled" for performance reasons.

Is there any more context on this somewhere? Like, did we benchmark the performance concerns or something?

I think the most visible instances of this I've seen previously are things like:

• You add a new avatar and it doesn't update everywhere until a refresh
• You change a name (I.e to add a status) and it's not updated until a refresh

I feel like we're just chipping away at these as they come/get noticed, whereas if it transpires there are minimal performance concerns with moving to "update immediately" - it would be good to just implement it consistently throughout App.

[JmillsExpensify]

That's interesting. I'm actually fine with the case @aldo-expensify clarified for Settings, but I agree with @trjExpensify that it is awkward when data points like your avatar and name don't propagate when you update those.

[tjferriss]

Sounds good. This feels like the kind of polish we want so I'm all for getting this fixed, especially the examples @trjExpensify shared.

[trjExpensify]

Yeah, to be clear, I think we've fixed both of those after we discovered them, but the point is if we can universally solve for this across App instead of chipping away at different use-cases, I think it's worthwhile.

[aldo-expensify]

at some point we decided they would be "uncontrolled" for performance reasons.

Is there any more context on this somewhere? Like, did we benchmark the performance concerns or something?

Context:

• This design doc: https://docs.google.com/document/d/1DI4jQ0nyfi2JczhPT440orwlXsTP_O-aGvg-HLmcbwI/edit# proposed the change to use "uncontrolled' inputs I think
• There have been other conversations in slack about controlled/uncontrolled inputs like this one: https://expensify.slack.com/archives/C01GTK53T8Q/p1643302140102200?thread_ts=1640793789.214900&cid=C01GTK53T8Q

In my opinion, it is true that controlled inputs, if not done well, can have a significant performance impact: each key stroke in a text input translate in re-render a big chunk of the app <- expensive! but, I also think that this can be somewhat easily solved by "debouncing" the update of the state higher in the component tree. With debouncing, instead of causing an expensive re-render on each key stroke, you end up re-rendering after 500ms, or some other number of ms, of the last key stroke). Debouncing can give you a controlled input that doesn't have a big performance impact.

If interested in reading about what is debouncing / controlled inputs / uncontrolled inputs, this seems to talk about all that: https://dev.to/bugs_bunny/debouncing-react-controlled-components-588i (I didn't read it completely on detail though)

I think the most visible instances of this I've seen previously are things like:

• You add a new avatar and it doesn't update everywhere until a refresh

I don't think that this is the same because this is not an input. My guess is that in this case we are really not pushing the updated avatar using pusher. If my guess is correct, it could require a simple fix in the API (internal).

• You change a name (I.e to add a status) and it's not updated until a refresh

Would have to look case by case. I think we store the "name" of an account in the "personal details". We would have to make sure that each time we update a name of the account, to push it to every device that has our personal details loaded. This would probably mean pushing the update to every account we have a chat with, or a room in common, or a workspace in common, etc.

I feel like we're just chipping away at these as they come/get noticed, whereas if it transpires there are minimal performance concerns with moving to "update immediately" - it would be good to just implement it consistently throughout App.

Considering the cases above, I feel like each of these cases will be somewhat different and won't have a common single solution. The controlled vs uncontrolled inputs only applies where you have inputs (text input/select input/checkboxes)

[michaelhaxhiu]

I'm tempted to switch this to NewFeature instead of leaving it as a Bug. Would ya'll agree?

Also let's not be shy in surfacing a summary in #expensify-open-source or #product about this if we need to get consensus on a path forward.

[aldo-expensify]

I'm tempted to switch this to NewFeature instead of leaving it as a Bug. Would ya'll agree?

I see your point, but I don't know what difference it makes :), so, up to you

Also let's not be shy in surfacing a summary in #expensify-open-source or #product about this if we need to get consensus on a path forward.

I asked here: https://expensify.slack.com/archives/C01GTK53T8Q/p1667247847771699, but maybe we should ask again with a summary as you say

[michaelhaxhiu]

Sorry missed the slack link you posted above! That's great, and a summary could help if you need consensus 👍 . I think yall have good momentum moving it forward and just helping push things forward.

@JmillsExpensify & @tjferriss I'm gonna make this newfeature because I think it's in that territory. It's not a big deal either way, just think this will require more than a bug fix would require. TJ astutely raised this Q in the very beginning - so we are all thinking about the right stuff! 👍

[trjExpensify]

Fair point about NewFeature as it was an intentional implementation for uncontrolled versus controlled inputs

This is super helpful context btw Aldo, thanks for doing that! This is also a great point on those other cases I highlighted that have the same effect, but a different cause:

Considering the cases above, I feel like each of these cases will be somewhat different and won't have a common single solution. The controlled vs uncontrolled inputs only applies where you have inputs (text input/select input/checkboxes)

Okay cool, so it sounds like you and @luacmartins had a pretty good discussion here, and the path we're on will apply to the profile page inputs, is that right? That's pretty good initial improvement in itself. 👍

[aldo-expensify]

This is another somewhat related issue: https://github.com/Expensify/App/issues/12600

Also, @Beamanator told me that a new page with a form to update the timezone was on the works here: https://github.com/Expensify/App/pull/12201. As this is an edge case (not high priority), I think we should wait for that PR to be finished before looking more into this, as some things may get resolved.

Meanwhile, sorry to open the discussion about what to do again, but, in the context of forms, should we consider doing nothing for these issues? I don't think it is obvious that desirable behaviour is to have the form updated... if you have the form open with some changes typed in, you may not want to lose those changes because you submitted the same form in a different device. My opinion on this is:

• This is very much an edge case, low ROI.
• Having the forms synchronizing introduces a bit more of complexity that may just not be worth it.
• Not convinced that the user really would like the forms to update automatically in this edge case

What I'm saying above doesn't apply to things that are not forms.

cc @francoisl as you have that other issue

[Beamanator]

Agreed that this specific issue can be closed b/c we're redesigning the Timezone page (in the PR linked ^ - cc @cristipaval )

And I also agree it makes sense NOT to necessarily auto-update forms in all cases, I think @francoisl might bring up that convo in slack to discuss 🙏

[JmillsExpensify]

I like bringing this up in Slack based on the trade-offs. At that point we can decide wether to move forward or close.

[aldo-expensify]

Following the slack conversation here, we decide to close this