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

[HOLD for payment 2024-07-10] Thread - Thread title not updated in header and in LHN after editing parent message #44430

Closed
3 of 6 tasks
izarutskaya opened this issue Jun 26, 2024 · 20 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Jun 26, 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.2-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4675207
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Open any chat
  3. Send a message and create a thread
  4. Change the parent message to anything

Expected Result:

The thread title is updated in the header and in LHN

Actual Result:

The thread title still displays the parent message before editing. It updates in the header after opening the thread again, but does not change in the LHN

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

Bug6524884_1719386508521.Recording__578.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @trjExpensify
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

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

Copy link

melvin-bot bot commented Jun 26, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Jun 26, 2024
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.

@izarutskaya
Copy link
Author

@trjExpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@izarutskaya
Copy link
Author

We think this issue might be related to the #vip-vsb

@bernhardoj
Copy link
Contributor

bernhardoj commented Jun 26, 2024

Proposal

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

Thread title and LHN title doesn't update immediately when updating the thread parent message.

What is the root cause of that problem?

For the thread title, we get the title from getReportActionMessage which uses parseReportActionHtmlToText. parseReportActionHtmlToText has a cache mechanism with the cache key being the combination of the reportID, reportActionID, and the report action lastModified.

App/src/libs/ReportUtils.ts

Lines 3238 to 3242 in 0c20881

const key = `${reportID}_${reportAction.reportActionID}_${reportAction.lastModified}`;
const cachedText = parsedReportActionMessageCache[key];
if (cachedText !== undefined) {
return cachedText;
}

There are 2 problems here. First, we don't optimistically update the lastModified when editing a message, so we rely on the BE response to update it. Second, we get the report action by using the deprecated getParentReportAction and since we enable the react-compiler in #42287, it means the component won't react to the parent report action updates because ReportUtils.getReportName only use the report data.

App/src/libs/ReportUtils.ts

Lines 3322 to 3324 in 0c20881

function getReportName(report: OnyxEntry<Report>, policy?: OnyxEntry<Policy>): string {
let formattedName: string | undefined;
const parentReportAction = ReportActionsUtils.getParentReportAction(report);

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

First, update the lastModified optimistically (and revert if fails).

App/src/libs/actions/Report.ts

Lines 1530 to 1543 in 0c20881

const optimisticReportActions: PartialDeep<ReportActions> = {
[reportActionID]: {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
message: [
{
...originalMessage,
type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
isEdited: true,
html: htmlForNewComment,
text: reportComment,
},
],
},
};

This change alone is enough because the report data is also updated which triggers the getReportName, but we should start passing the parent report action as the 3rd param.

const title = ReportUtils.getReportName(reportHeaderData);

const title = ReportUtils.getReportName(reportHeaderData, undefined, parentReportAction);

@trjExpensify
Copy link
Contributor

Have we identified the PR on the staging checklist that broke this?

@bernhardoj
Copy link
Contributor

I think it's the react-compiler.

In prod, I can see that the thread and LHN title are late to update.

Screen.Recording.2024-06-26.at.20.56.40.mov

But in main (or staging), the thread title is never updated which is worse than prod.

Screen.Recording.2024-06-26.at.21.21.22.mov

So, there are 2 problems, 1 is an old problem and the other one is caused by the react-compiler. (NOTE: it shows that the react-compiler did its job)

I have updated my proposal to mention the react-compiler PR.

@mountiny
Copy link
Contributor

@kirillzyusko Could you check this out too please?

@mountiny
Copy link
Contributor

FWIW the proposal from @bernhardoj makes sense

@yuwenmemon yuwenmemon removed the DeployBlocker Indicates it should block deploying the API label Jun 26, 2024
@kirillzyusko
Copy link
Contributor

@mountiny yeah, the proposal looks good for me as well 👍

@bernhardoj
Copy link
Contributor

PR is ready

@yuwenmemon yuwenmemon removed the DeployBlockerCash This issue or pull request should block deployment label Jun 27, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title Thread - Thread title not updated in header and in LHN after editing parent message [HOLD for payment 2024-07-10] Thread - Thread title not updated in header and in LHN after editing parent message Jul 3, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

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

Copy link

melvin-bot bot commented Jul 3, 2024

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

If no regressions arise, payment will be issued on 2024-07-10. 🎊

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

Copy link

melvin-bot bot commented Jul 3, 2024

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:

  • [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
  • [@rushatgabhane] 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:
  • [@rushatgabhane] 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:
  • [@rushatgabhane] Determine if we should create a regression test for this bug.
  • [@rushatgabhane] 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:

@rushatgabhane
Copy link
Member

  1. The PR that introduced the bug has been identified. Link to the PR: the bug stems from react compiler pr - [NoQA] feat: react-compiler #42287

  2. 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: [NoQA] feat: react-compiler #42287 (review)

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

  4. Determine if we should create a regression test for this bug. No because caught by QA and rca was react compiler which is one time thing.

  5. 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 - N.A.

@trjExpensify
Copy link
Contributor

Payment summary as follows:

Feel free to go ahead and request, both of you. Closing!

@bernhardoj
Copy link
Contributor

Requested in ND.

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

@JmillsExpensify
Copy link

$250 approved for @rushatgabhane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants