search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

iOS - Conversation - If the last message is deleted, the previous one becomes partially hidden #10765

Closed kbecciv closed 1 year ago

kbecciv commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Open the App and login with any account
  2. Go to any conversation
  3. Send 2 or more messages
  4. Delete the latest message

Expected Result:

The previous message should not be hidden

Actual Result:

The previous message becomes partially hidden

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.96.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/188025493-15d3a984-ccec-4e20-8706-7deae4c33626.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

ctkochan22 commented 2 years ago

@JmillsExpensify let me know when you post the job. Description is clear and it is reproducible

JmillsExpensify commented 2 years ago

Upwork job is here: https://www.upwork.com/jobs/~012e25f97da880eedd. We're open for proposals.

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

melvin-bot[bot] commented 2 years ago

Current assignee @ctkochan22 is eligible for the Exported assigner, not assigning anyone new.

JmillsExpensify commented 2 years ago

Still open for proposals.

JmillsExpensify commented 2 years ago

Bumping this to $500 to encourage proposals.

ctkochan22 commented 2 years ago

Not overdue, waiting proposals

Santhosh-Sellavel commented 2 years ago

@kbecciv is this reproducible now?

I am unable to reproduce this issue

advaitasol9 commented 2 years ago

Proposal:

Changing "contentContainerStyle" of InvertedFlatList in ReportActionsList.js as follows fixes the issue.

Screenshot 2022-09-22 at 9 57 37 AM

The issue arrived because the padding between InvertedFlatList and TextInputContainer was reduced to 0 (as visible in above image) to accommodate Recipient's Local Time. Which wasn't the correct change as the padding in absence of Recipient's Local Time is 16, https://github.com/Expensify/App/blob/a24de554bd2e70575f4389942d6132d7ca9ea082/src/styles/styles.js#L1268-L1272 so counting marginTop = 5 of Recipient's Local Time's View Container, https://github.com/Expensify/App/blob/a24de554bd2e70575f4389942d6132d7ca9ea082/src/pages/home/report/ParticipantLocalTime.js#L63 https://github.com/Expensify/App/blob/a24de554bd2e70575f4389942d6132d7ca9ea082/src/styles/styles.js#L643-L647 the correct padding should be 11 and pt3 is the closest to that.

Before: https://user-images.githubusercontent.com/93445984/191662026-7189564b-380d-462e-b55e-bc6661aabc7b.mp4

After: https://user-images.githubusercontent.com/93445984/191663285-2d90fb36-4129-4bcb-97e2-ab56956aadc6.mp4

advaitasol9 commented 2 years ago

Hey @Santhosh-Sellavel you can reproduce the issue by following the steps below:

  1. Send multiple messages.
  2. Keep the textinput focused, so that keyboard stays open.
  3. Delete the last message.
kbecciv commented 2 years ago

@Santhosh-Sellavel Checking with team, will update you shortly

kbecciv commented 2 years ago

@Santhosh-Sellavel Issue is still reproducible.

https://user-images.githubusercontent.com/93399543/191787415-178d6a23-1583-412f-be6e-31300d04a8a4.MP4

Santhosh-Sellavel commented 2 years ago

@Santhosh-Sellavel Issue is still reproducible.

RPReplay_Final1663859877.MP4

@kbecciv this looks likely from an simulator is this reproducible on any real device

kbecciv commented 2 years ago

No, this is real device.

Santhosh-Sellavel commented 2 years ago

All right no luck for me though, thanks for checking @kbecciv.

melvin-bot[bot] commented 2 years ago

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 2 years ago

@JmillsExpensify, @ctkochan22, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

JmillsExpensify commented 2 years ago

We're still open for proposals! Issue is still reproducible.

advaitasol9 commented 2 years ago

Hi, @JmillsExpensify @ctkochan22 @Santhosh-Sellavel @kbecciv can you take a look at my proposal?

Santhosh-Sellavel commented 2 years ago

Will have a look @advaitasol9, thanks for the bump!

mvtglobally commented 2 years ago

Issue not reproducible during KI retests. (First week)

Santhosh-Sellavel commented 2 years ago

@advaitasol9 is this issue reproducible for you on staging/ latest main.

advaitasol9 commented 2 years ago

Hey @Santhosh-Sellavel you can reproduce the issue by following the steps below:

  1. Send multiple messages.
  2. Keep the textinput focused, so that keyboard stays open.
  3. Delete the last message.

Yes @Santhosh-Sellavel, below is a video from few days ago while I was testing on main branch. In my testing I found the issue is only happening when we delete the message while the keyboard is open.

https://user-images.githubusercontent.com/93445984/192520952-a0c19e92-d477-4de2-8cb5-38bb3c5c1625.mp4

Santhosh-Sellavel commented 2 years ago

Yes @Santhosh-Sellavel, below is a video from few days ago while I was testing on main branch. In my testing I found the issue is only happening when we delete the message while the keyboard is open.

@advaitasol9 Can you check now on staging build or latest main

advaitasol9 commented 2 years ago

Sure @Santhosh-Sellavel , will get back to you.

advaitasol9 commented 2 years ago

@Santhosh-Sellavel I am able to reproduce on latest main.

ctkochan22 commented 2 years ago

@JmillsExpensify @advaitasol9 's proposal looks good.

Santhosh-Sellavel commented 2 years ago

@JmillsExpensify @advaitasol9 's proposal looks good.

@ctkochan22 thanks for the quick review, if possible can you wait one more day? I'm investigating this & other related issues.

Thanks!

Santhosh-Sellavel commented 2 years ago

@advaitasol9

The issue arrived because the padding between InvertedFlatList and TextInputContainer was reduced to 0 (as visible in the above image)

Do you think this was a regression, when was it reduced?

advaitasol9 commented 2 years ago

Sorry @Santhosh-Sellavel , I meant to say the padding right now is set to 0 which needs to be 12.

ctkochan22 commented 2 years ago

@Santhosh-Sellavel Sorry hat other related issues are you investigating?

Santhosh-Sellavel commented 2 years ago

@ctkochan22 Yeah, I'm C+ there too this might be related to this issue as the scroll position is not correct for the last item in both issues but on different use cases.

JmillsExpensify commented 2 years ago

I like trying to fix the scroll position issues more holistically, though perhaps for clarity, is it only happening for the delete case and pay?

Santhosh-Sellavel commented 2 years ago

I like trying to fix the scroll position issues more holistically

Yeah, exactly

neil-marcellini commented 2 years ago

I don't think #10283 is related to this since I believe the root cause there is about the content changing size and here the chats do not change size. I think @advaitasol9 might be on the right track.

JmillsExpensify commented 2 years ago

Ok cool. Thanks for your patience @advaitasol9. For the sake of clarity, what is the formal proposal?

advaitasol9 commented 2 years ago

Proposal:

Changing "contentContainerStyle" of InvertedFlatList in ReportActionsList.js as follows fixes the issue. Screenshot 2022-09-22 at 9 57 37 AM

The issue arrived because the padding between InvertedFlatList and TextInputContainer was reduced to 0 (as visible in above image) to accommodate Recipient's Local Time. Which wasn't the correct change as the padding in absence of Recipient's Local Time is 16,

https://github.com/Expensify/App/blob/a24de554bd2e70575f4389942d6132d7ca9ea082/src/styles/styles.js#L1268-L1272

so counting marginTop = 5 of Recipient's Local Time's View Container, https://github.com/Expensify/App/blob/a24de554bd2e70575f4389942d6132d7ca9ea082/src/pages/home/report/ParticipantLocalTime.js#L63

https://github.com/Expensify/App/blob/a24de554bd2e70575f4389942d6132d7ca9ea082/src/styles/styles.js#L643-L647

the correct padding should be 11 and pt3 is the closest to that. Before: https://user-images.githubusercontent.com/93445984/191662026-7189564b-380d-462e-b55e-bc6661aabc7b.mp4

After: https://user-images.githubusercontent.com/93445984/191663285-2d90fb36-4129-4bcb-97e2-ab56956aadc6.mp4

here's the proposal @JmillsExpensify.

JmillsExpensify commented 2 years ago

@neil-marcellini @Santhosh-Sellavel Are both of you cool with this proposal? I'd love to keep things moving forward! 🙌🏼

neil-marcellini commented 2 years ago

I'll leave it up to CME @ctkochan22, I was commenting from another linked issue.

JmillsExpensify commented 2 years ago

Oh shoot. Good catch. @ctkochan22 can you chime in here so that we can keep this one moving?

ctkochan22 commented 2 years ago

I agree with @neil-marcellini here. I don't think the two are not related. I'm good with the proposal, lets move forward

JmillsExpensify commented 2 years ago

Cool. I'll add @advaitasol9 to the issue. Can you also make sure to the Upwork job as well please? cc @Santhosh-Sellavel

melvin-bot[bot] commented 2 years ago

📣 @advaitasol9 You have been assigned to this job by @JmillsExpensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

advaitasol9 commented 2 years ago

Hi @JmillsExpensify , i've applied on upwork. I'll create a PR by 12 october.

JmillsExpensify commented 2 years ago

Awesome, thank you! @Santhosh-Sellavel just waiting on you to apply.

JmillsExpensify commented 2 years ago

@Santhosh-Sellavel Reminder to apply to the Upwork job when you have a chance.

Santhosh-Sellavel commented 2 years ago

Done thanks @JmillsExpensify

mvtglobally commented 2 years ago

Issue not reproducible during KI retests. (First week)