Skip to content
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

[$250] iOS - Track expense - App crashes after deleting expense offline #48197

Closed
1 of 6 tasks
IuliiaHerets opened this issue Aug 28, 2024 · 25 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 28, 2024

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


Version Number: 9.0.25.11
Reproducible in staging?: Y
Reproducible in production?: N
Issue reported by: Applause Internal Team

Action Performed:

  1. Open the app and log in
  2. Navigate to self-DM
  3. Tap + > Track expense > Manual
  4. Create an expense
  5. Disable the internet connection
  6. Long tap the expense > Delete expense

Expected Result:

The expense is greyed out, the amount is crossed out

Actual Result:

The app crashes

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6585585_1724865546739.IMG_9404.mp4

2808.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013b920bb98479136b
  • Upwork Job ID: 1828958726626801528
  • Last Price Increase: 2024-08-29
  • Automatic offers:
    • jjcoffee | Contributor | 103818924
Issue OwnerCurrent Issue Owner: @jjcoffee
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 28, 2024
Copy link

melvin-bot bot commented Aug 28, 2024

Triggered auto assignment to @VictoriaExpensify (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 melvin-bot bot added the Daily KSv2 label Aug 28, 2024
@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@stitesExpensify
Copy link
Contributor

Switching this to deployblockercash since this is iOS only

@stitesExpensify stitesExpensify added DeployBlockerCash This issue or pull request should block deployment and removed DeployBlocker Indicates it should block deploying the API labels Aug 28, 2024
Copy link

melvin-bot bot commented Aug 28, 2024

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

Copy link
Contributor

👋 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.

@chiragsalian chiragsalian added the External Added to denote the issue can be worked on by a contributor label Aug 29, 2024
@melvin-bot melvin-bot bot changed the title iOS - Track expense - App crashes after deleting expense offline [$250] iOS - Track expense - App crashes after deleting expense offline Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Job added to Upwork: https://www.upwork.com/jobs/~013b920bb98479136b

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox (External)

@suneox
Copy link
Contributor

suneox commented Aug 29, 2024

This issue look like can not reproduce on the latest main due to context menu doesn't show delete option

Screen.Recording.2024-08-29.at.11.17.36.mp4

@NJ-2020

This comment was marked as outdated.

@NJ-2020
Copy link
Contributor

NJ-2020 commented Aug 29, 2024

Yes issue are not reproducible from both non-staging and staging server

@tsa321
Copy link
Contributor

tsa321 commented Aug 29, 2024

To test, should set the USE_REACT_STRICT_MODE to false to display the context menu properly.
I can reproduce the crash issue in main.

@chiragsalian
Copy link
Contributor

Given this is an edge case and we're having a bit of a difficult time solving this immediately, we'll be demoting it to daily, removing blocker and requesting help with this.

@chiragsalian chiragsalian removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Aug 30, 2024
@chiragsalian chiragsalian added the Daily KSv2 label Aug 30, 2024
@hannojg
Copy link
Contributor

hannojg commented Aug 31, 2024

cc @kirillzyusko can you comment here so you can be assigned?

@kirillzyusko
Copy link
Contributor

Sure, feel free to assign this on me 🙌

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@suneox
Copy link
Contributor

suneox commented Sep 2, 2024

@kirillzyusko will work on this issue, so I’d like to self-unassign.​

@melvin-bot melvin-bot bot removed the Overdue label Sep 2, 2024
@suneox suneox removed their assignment Sep 2, 2024
@kirillzyusko
Copy link
Contributor

Few things that I spotted:

  • the crash happens because we are trying to unmount a component that already has been unmounted;
  • the component that triggers a crash is Paragraph (i. e. Text)
  • if I remove all Text elements in MoneyRequestPreviewContent component then crash goes away;
  • if I clean-up the logic in OfflineWithFeedback:
<View style={style}>
    {!hideChildren && (
        children
    )}
    {shouldShowErrorMessages && (
        <ErrorMessageRow
            errors={errors}
            errorRowStyles={errorRowStyles}
            onClose={onClose}
            canDismissError={canDismissError}
        />
    )}
</View>

then crash also goes away.

I think the problem is hidden somewhere in this code:

<View
    style={[needsOpacity ? styles.offlineFeedback.pending : {}, contentContainerStyle]}
    needsOffscreenAlphaCompositing={shouldRenderOffscreen ? needsOpacity && needsOffscreenAlphaCompositing : undefined}
>

Because as soon as I remove it the crash is not happening anymore.

I'll continue my investigation tomorrow and hopefully will provide a fix!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 3, 2024
@kirillzyusko
Copy link
Contributor

I opened a PR with a fix here: #48474

@jjcoffee
Copy link
Contributor

jjcoffee commented Sep 4, 2024

@chiragsalian Can you assign me here as I've been auto-assigned for reviewing the PR? Thanks! 🙏

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 4, 2024
Copy link

melvin-bot bot commented Sep 4, 2024

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

@jjcoffee
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: N/A - not possible to determine
  • 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: N/A
  • A discussion in #expensify-bugs 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: N/A
  • Determine if we should create a regression test for this bug. Yes
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

(Native mobile only)

  1. Navigate to self-DM
  2. Tap + > Track expense > Manual
  3. Create an expense
  4. Disable the internet connection
  5. Long tap the expense > Delete expense
  6. Verify that the app does not crash

Do we agree 👍 or 👎

@jjcoffee
Copy link
Contributor

@VictoriaExpensify This was deployed Sept 12, so should be ready for payment now.

@jjcoffee
Copy link
Contributor

@VictoriaExpensify Friendly bump for the payment on this. Thanks! 🙏

@VictoriaExpensify
Copy link
Contributor

Payment summary:
Contributor: @kirillzyusko - agency
Contributor+: @jjcoffee paid $250 via Upwork

@jjcoffee
Copy link
Contributor

@VictoriaExpensify I think you reopened this one by mistake?

@VictoriaExpensify
Copy link
Contributor

Ha I guess I did!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants