[HOLD for payment 2024-06-11] App crashes due to trying to access accountIdToName prop in epensimark on an undefined variable

[iwiznia]

Context https://expensify.slack.com/archives/C04878MDF34/p1715699038416249

Issue Owner

Current Issue Owner: @twisterdotcom

[melvin-bot]

Triggered auto assignment to @twisterdotcom (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.

[war-in]

Hi, I'm Marcin from Software Mansion and I would like to work on this

[twisterdotcom]

Doesn't look like I'm needed here as no C+ reviewed these PRs, but LMK if anybody needs paying.

[ahmedGaber93]

@robertKozik @war-in I think we still have a type error as here we use accountIdToName with Id, but its type here defines as accountIDToName with ID

[robertKozik]

Yeah you're right, there is typo in the types. I'll spin up the PR soon

[melvin-bot]

⚠️ 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.

[MonilBhavsar]

We've reverted PR #42176 that updated expensify-common version in the App, because it caused multiple crashes.

The crash will happen again if we update expensify-common version in the App for some reason. Can anyone please fix the issue from root cause i.e. in expensify-common please. Thanks!

[twisterdotcom]

Who is responsible for this now @iwiznia or @war-in - or should we take this External for some help?

[iwiznia]

@war-in said they would work on it and I assigned them.

[twisterdotcom]

Nice, okay @war-in let us know if you need any help here.

[war-in]

@iwiznia @twisterdotcom I've found what the bug is. This PR introduced Log to expensify-common (ExpensiMark.js) and it turned out it doesn't work on mobile (jquery is not present there).
To quick-fix it we can change Log.alert to console.warn as the information provided by the Logger won't be helpful until my PR enabling editing mentions is merged.

Please share your thoughts 🙏

[iwiznia]

nd it turned out it doesn't work on mobile (jquery is not present there).

I don't get this. What part of Log.alert uses jquery?

[war-in]

Based on this error I found the line which uses jquery. Network is used by Log.alert

Screen.Recording.2024-06-03.at.12.58.10.mov

[iwiznia]

Hmmmm why is it using that? We call Log.alert in app already. I think the OG PR imported the wrong log library. It imported Log.jsx and should've imported Logger.jsx?

[blazejkustra]

I think the callstack looks like this:

$.ajax(...);
Network.post(...)
API.performPOSTRequest(...)
API.logToServer(...)
Logger.serverLoggingCallback(...)
Logger.logToServer(...)
Logger.add(...)
Log.alert(...)

Where serverLoggingCallback in this case is defined in lib/Log.jsx:

function serverLoggingCallback(logger, params) {
    return API(Network('/api/')).logToServer(params);
}

export default new Logger({
    serverLoggingCallback,
    clientLoggingCallback,
    isDebug: isWindowAvailable() ? window.DEBUG : false,
});

[iwiznia]

The Log we use in App is defined here 🤔

[blazejkustra]

The Log in expensify-common uses Logger:

export default new Logger({
    serverLoggingCallback,
    clientLoggingCallback,
    isDebug: isWindowAvailable() ? window.DEBUG : false,
});

[blazejkustra]

Looking at the code serverLoggingCallback won't work on mobile because it is using jquery, and clientLoggingCallback doesn't really differ from using console.warn as suggested here 🤔

Also Log isn't used anywhere in expensify-common, there is only one invocation in PubSub module which isn't imported in E/App, any suggestions how to proceed?

[melvin-bot]

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

• Bump expensify-common version #42176

If no regressions arise, payment will be issued on 2024-06-11. 🎊

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

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

[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:

• [@iwiznia] The PR that introduced the bug has been identified. Link to the PR:
• [@iwiznia] 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:
• [@iwiznia] 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:
• [@war-in] Determine if we should create a regression test for this bug.
• [@war-in] 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.
• [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ikevin127]

1st PR for this was reverted.

[sonialiap]

I can't access a chat with a client, possible same issue https://github.com/Expensify/Expensify/issues/403390

[iwiznia]

@war-in @blazejkustra we are missing re-updating the version of expensify commons the App uses, no?

[war-in]

Afaik @blazejkustra updated the expensify-common in this PR. It includes a fix for this bug.
And I'm almost finished with the mentions editing PR here

[twisterdotcom]

Okay, Is this ready for payment yet or not? Does it even need payment? Perhaps only for @ikevin127 somewhere?

[ikevin127]

@twisterdotcom I reviewed the initial PR as C+ but it was reverted in #42914, therefore no payment is due here and the hold for payment can be removed.

Not sure if I'm still needed as reviewer for PR number 2 🤔

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[sonialiap]

I was able to load the chat today without issue 😁 thank you for the fix!

[twisterdotcom]

Looks like it was internally reviewed. Okay, closing this then.