App icon indicating copy to clipboard operation
App copied to clipboard

Hybrid - Android - Settings- General section on settings disappears when navigating back from profile

Open IuliiaHerets opened this issue 1 year ago β€’ 16 comments

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


Version Number: 9.0.69-1 Reproducible in staging?: Y Reproducible in production?: N If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Open the Expensify app.
  2. Tap on "Settings" on the bottom of the screen.
  3. Tap on "Profile"
  4. Tap on the search icon on the top right corner and select any chat from search results.
  5. Tap on compose box to open the keyboard.
  6. Tap on the arrow on the top left corner.
  7. When landing on "Profile" again, tap fast on the arrow on the top left corner again.
  8. Verify you land on "Settings" tab and all the sections are correctly displayed.

Expected Result:

When navigating back to settings from profile, all the tab should be completely visible and no section should disappear.

Actual Result:

When navigating back fast to "Settings" from profile, after opening a chat from search results, the "General" section disappears.

Workaround:

Unknown

Platforms:

  • [ ] Android: Standalone
  • [x] Android: HybridApp
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [ ] iOS: HybridApp
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/e802b74e-4c79-407e-9ee5-4a0a3c86e40d

View all open jobs on GitHub

IuliiaHerets avatar Dec 02 '24 12:12 IuliiaHerets

Triggered auto assignment to @flodnv (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] avatar Dec 02 '24 12:12 melvin-bot[bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Dec 02 '24 12:12 melvin-bot[bot]

πŸ’¬ A slack conversation has been started in #expensify-open-source

melvin-bot[bot] avatar Dec 02 '24 12:12 melvin-bot[bot]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

github-actions[bot] avatar Dec 02 '24 12:12 github-actions[bot]

Hi πŸ‘‹ I tried to go through steps on the newest main branches and can't reproduce the issue

https://github.com/user-attachments/assets/5891dc41-ee0f-4474-b82f-5d2dc94c3dbf

war-in avatar Dec 02 '24 13:12 war-in

Our testers reported that this issue is not consistently reproducible. It's reproduced 1 out of 5 times. 3 Testers were able to reproduce the issue. Here's a video of another attempt:

https://github.com/user-attachments/assets/abad604e-d2f1-423d-8ca8-bf81a10dc5d4

isagoico avatar Dec 02 '24 14:12 isagoico

Thanks! I am adding it to the quality project and marking as critical, but given this does not block the user and its not reliably reproducible i dont think it has to be a blocker.

@kirillzyusko any ideas why the keyboard might be causing this? Probably long shot as you dont have access to hybrid

mountiny avatar Dec 02 '24 15:12 mountiny

I found out that if we set

shouldEnableKeyboardAvoidingView={false}

in ScreenWrapper props the issue is gone. Based on that, the bug is somewhere there: https://github.com/Expensify/App/blob/cb0613c083baa987c63a5bdd724a729c5a1795b8/src/components/KeyboardAvoidingView/index.android.tsx

war-in avatar Dec 02 '24 16:12 war-in

@war-in @mountiny Interesting - looks like KeyboardAvoidingView can not be resized properly πŸ€” But very curious why it happens only in Hybrid app?..

kirillzyusko avatar Dec 02 '24 16:12 kirillzyusko

After some KeyboardAvoidingView investigation I can tell that the issue comes from the wrong bottom value which is computed here

I think the problem is with the progress value of the keyboard const

keyboard.progress.get()

It returns 1 but I guess it should return 0 when there is no keyboard πŸ€”

war-in avatar Dec 02 '24 16:12 war-in

I created a draft PR fixing the issue for now but I think we should fix the real cause of it somewhere in the progress logic of useKeyboardAnimation function

cc @kirillzyusko What do you think? Any ideas on what could be the root cause of it?

war-in avatar Dec 02 '24 16:12 war-in

What do you think? Any ideas on what could be the root cause of it?

@war-in First of all I would log values from onStart/onMove/onEnd handlers to understand which handler is broken. Once we understnad that we can go to the native code to see how it's getting calculated so that we can fix it.

But you are right - even when the problem gets fixed on a single page, there is no guarantee that the same problem will not re-appear on other pages. And you are also right - if keyboard is closed then progress should be 0 πŸ™‚

Feel free to drop me any messages in Slack - I'm responding much faster there πŸ‘€

kirillzyusko avatar Dec 02 '24 17:12 kirillzyusko

Sure! I'll have more time tomorrow to jump into this code so expect me in your DMs 🀞

war-in avatar Dec 02 '24 17:12 war-in

After some investigation, I found that when we repeat the steps, the onStart and onEnd events for InitialSettingsPage are sometimes missing and the initialProgress value is used to determine the paddingBottom value. We could find the cause of those missing events by diving deeper into the react-native-keyboard-controller codebase but I wonder if it's worth it since the fix has been merged and this was the only place the bug showed up.

What do you think @kirillzyusko @mountiny?

war-in avatar Dec 03 '24 14:12 war-in

@war-in I think we can see if this will be a problem later and only then dig into this more

mountiny avatar Dec 04 '24 12:12 mountiny

there are no payments to handle on this PR, and the issue was coming from a large native stack PR, and the authors are aware (contributed in this discussion). I don't think we need a regression step for this so we can close once the checklist is added

mountiny avatar Dec 04 '24 12:12 mountiny

⚠️ Looks like this issue was linked to a Deploy Blocker 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 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] avatar Dec 04 '24 20:12 melvin-bot[bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] avatar Dec 05 '24 15:12 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.71-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

  • https://github.com/Expensify/App/pull/53393

If no regressions arise, payment will be issued on 2024-12-12. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @war-in does not require payment (Contractor)

melvin-bot[bot] avatar Dec 05 '24 15:12 melvin-bot[bot]

@mountiny @slafortune @war-in The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Dec 05 '24 15:12 melvin-bot[bot]

BugZero Checklist:

  • [ ] [Contributor] Classify the bug:
Bug classification

Source of bug:

  • [ ] 1a. Result of the original design (eg. a case wasn't considered)
  • [ ] 1b. Mistake during implementation
  • [ ] 1c. Backend bug
  • [ ] 1z. Other:

Where bug was reported:

  • [ ] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • [ ] 2b. Reported on staging (eg. found during regression or PR testing)
  • [ ] 2d. Reported on a PR
  • [ ] 2z. Other:

Who reported the bug:

  • [ ] 3a. Expensify user
  • [ ] 3b. Expensify employee
  • [ ] 3c. Contributor
  • [ ] 3d. QA
  • [ ] 3z. Other:
  • [ ] [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment:

  • [ ] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion:

  • [ ] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree πŸ‘ or πŸ‘Ž

slafortune avatar Dec 05 '24 20:12 slafortune

Closing - https://github.com/Expensify/App/issues/53380#issuecomment-2517163504

slafortune avatar Dec 05 '24 20:12 slafortune