[HOLD for payment 2023-11-07] [$500] Web - App crash on device toolbar toggle

[kbecciv]

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: 1.3.90.1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @Sourcecodedeveloper
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698075798349259

Action Performed:

1. Login to https://staging.new.expensify.com/
2. Press F12 to open console
3. Click on toggle device toolbar

Expected Result:

App is resized to selected device

Actual Result:

The App crashes

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

web.webm

Recording.5133.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013db618c7bafaf3c8
• Upwork Job ID: 1716860148517441536
• Last Price Increase: 2023-10-24

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[situchan]

@kacper-mikolajczak is already working on this - #16161 (comment)
cc: @youssef-lr

[yh-0218]

This error comes from BoundsObserver of BaseTooltip

[iiredalert]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When switching device screen size the application crashes at <BoundsObserver .../> in BaseTooltip.js

What is the root cause of that problem?

The _childRef.current in BoundsObserver is set to null due to the rendered element from the BoundsObserver's render() function being destroyed.

What changes do you think we should make in order to solve the problem?

The logic in @react-ng/bounds-observer is fairly straight forward and doesn't change often. We can implement our own equivalent that simply ignores and does nothing if the callback or childref are undefined.

What alternative solutions did you explore? (Optional)

[youssef-lr]

cc @kacper-mikolajczak @cubuspl42 , what do you guys think about the proposal above?

[kacper-mikolajczak]

IIRC, the library was initially built to serve this precise purpose in Exfy project. Moving it into our codebase would mean that we could easily change its internals, but as stated in the proposal, those does not change often.

I think we can file an issue there.

Wonder what is the reasoning behind throwing the error instead of silently fallbacking to noop behaviour. It adheres to principles of aggressive programming and from that perspective we can see the issue straight away, but is it really an issue in our case? Weird it happens only when toggling the part of the devtools 🤔

[iiredalert]

Could it be related to the library using https://react.dev/reference/react/cloneElement ? I am still trying to figure out why it needs to clone the element and it doesn't just render it.

We could also add a check to see if children.ref is defined before rednering the BaseTooltip. This will prevent the error from happening but the mouse pointer and tooltips are not rendered until we turn off the device toggle mode in chrome.

    // Skip the tooltip and return the children if the text is empty,
    // we don't have a render function or the device does not support hovering
    if ((_.isEmpty(text) && renderTooltipContent == null) || !hasHoverSupport || !children.ref) {
        return children;
    }

[cubuspl42]

Wonder what is the reasoning behind throwing the error instead of silently fallbacking to noop behaviour

A philosophical question... I just chose to do it this way, I guess

I am still trying to figure out why it needs to clone the element and it doesn't just render it.

I wish I remembered... But it has something to do with how difficult it is to create a perfect wrapper for a React/HTML element that doesn't itself break the system it tries to observe. I remember that I had a few approaches to this. Maybe even the commit history holds some clues?

[kacper-mikolajczak]

I am still trying to figure out why it needs to clone the element and it doesn't just render it.

@iiredalert By using cloneElement library "hijacks" the child's ref prop so then it can be used internally by BoundsObserver (here is an actual usage).

[kacper-mikolajczak]

Thanks for sparking the discussion guys - while I was responding to your comments something struck me.

Investigation

The app crash is not caused by any layout changes - Chrome changes the input mode whenever we toggle the device toolbar.

Here is a recording showing that the app does not crash on desktop input mode but does on mobile:

Screen.Recording.2023-10-26.at.20.57.12.mov

Solution

In order to fix this we need to, in early return of Hoverable component:

    if (!DeviceCapabilities.hasHoverSupport()) {
        return child;
    }

hijack the ref of a children as we do in the regular return:

   if (!DeviceCapabilities.hasHoverSupport()) {
       return React.cloneElement(child, {
           ref: (el) => {
               ref.current = el;
               assignRef(child.ref, el);
           },
       });
   }

Here is a recording of the final outcome:

Screen.Recording.2023-10-26.at.21.06.10.mov

@cubuspl42 @iiredalert Thanks a lot once again for the discussion, it was invaluable in this case 🔥 🙇

@youssef-lr I will create a PR with a fix and link it here! 🔧

[melvin-bot]

Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[cubuspl42]

@kacper-mikolajczak It still feels like there is some issue in BoundsObserver, right? The path that threw the exception was not meant to be entered.

[kacper-mikolajczak]

@cubuspl42 We did not pass the ref correctly from child to the BoundsObserver, so it was not caused by BoundsObserver itself.

In case of missing ref, if the intention is to fail fast, then it is working as expected, otherwise we could change the behaviour so it silently opt-out of detecting the bounds when no proper ref is available from children (that I guess might be the favorable for production, so we don't crash entire app).

As a last resort, this might be configurable via props, but I am not sure if that wouldn't unnecessarily extend the complexity.

[trjExpensify]

Just catching up after a couple of days out. I've removed the Help wanted label as this looks to have been handled by @kacper-mikolajczak in this PR.

Also to confirm, this was a known regression coming out of here?

[kacper-mikolajczak]

HI @trjExpensify 👋 That's right, the fix was handled in the linked PR.

I am not sure where this issue was first documented. CC @youssef-lr

[trjExpensify]

Okay, if it wasn't found when executing the issue linked originally, the first report of this would be in #expensify-bugs by @Sourcecodedeveloper.

[youssef-lr]

We encountered this 2 weeks ago and I pinged Kacper here and that's when he started working on a solution. This was first brought to my attention by @situchan when he was testing this revert PR.

Soo I'm not sure, it was officially reported by @Sourcecodedeveloper though, you decide @trjExpensify :D

[situchan]

I don't think bonus applies here as the research already started before bug report on slack.
Similar cases: #22513 (comment), #23821 (comment)

[trjExpensify]

Ah yes, then I agree. If it was found earlier while executing the related PRs then there isn't a bug report bounty to pay out.

[trjExpensify]

@trjExpensify The guidelines indicate that if more than one contributor proposes the same bug, the contributor who posted it first in the #expensify-bugs channel is the one who is eligible for the $50 reward.

Yup, I agree with that. What this means is if two contributors post in #expensify-bugs (as you often see happen) then the one that reported it first is eligible. This isn't to say that the people involved in the PR/issue that introduced the bug need to report it in #expensify-bugs as they prep a follow-up PR, typically linked to the original issue (or a deploy blocker issue created).

That said, reviewing the convos, I can see how this one is a little more ambiguous than usual. A PR wasn't in the works and the investigation to figure out the root cause picked back up here in this issue. We probably wouldn't have got to a resolution on the timeline we did without it, so I think I'm cool with going ahead to pay this bug bounty.

[melvin-bot]

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

[melvin-bot]

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

• [Fix][Web] App crash on device toolbar toggle #30462

If no regressions arise, payment will be issued on 2023-11-07. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @ArekChr does not require payment (Contractor)
• @kacper-mikolajczak does not require payment (Contractor)
• @Sourcecodedeveloper requires payment

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@youssef-lr] The PR that introduced the bug has been identified. Link to the PR: Update version to 1.0.1-490 #1616
• [@youssef-lr] 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: Migrate index.js to function component #16161 (comment)
• [@youssef-lr] 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: https://expensify.slack.com/archives/C01GTK53T8Q/p1699562798622679
• [@ArekChr / @kacper-mikolajczak] Determine if we should create a regression test for this bug.
• [@ArekChr / @kacper-mikolajczak] 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.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[youssef-lr]

@ArekChr @kacper-mikolajczak can we complete checklist please?

[ArekChr]

• Determine if we should create a regression test for this bug.

I believe we might not require dedicated regression tests for this issue, as it appears that it would be detected during our existing testing processes.

[trjExpensify]

Yep, I tend to agree this doesn't need a regression test as a customer isn't likely to be using dev tools in the console.

@Sourcecodedeveloper you need to either provide me with your Upwork account profile or apply to this job for me to pay you the $50 bug bounty.

[trjExpensify]

Thanks, sent an offer.

[trjExpensify]

Settled, closing!