Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.43k stars 2.8k forks source link

[$250] Subject of the room briefly displayed {"html": "subject here"} before it finished rendering #49355

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks ago

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: Reproducible in staging?: Needs Reproduction Reproducible in production?: Needs Reproduction 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: @coleaeason @jliexpensify Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1726583603519899

Action Performed:

  1. Go the social room using very slow internet connection

    Expected Result:

    Room details loads without any error

    Actual Result:

    Subject of the room briefly displayed {"html": "subject here"} before it finished rendering

    Workaround:

    Unknown

    Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838604064730634601
  • Upwork Job ID: 1838604064730634601
  • Last Price Increase: 2024-10-01
Issue OwnerCurrent Issue Owner: @abdulrahuman5196
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @techievivek (AutoAssignerNewDotQuality)

melvin-bot[bot] commented 2 weeks ago

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

MelvinBot commented 2 weeks ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

sonialiap commented 2 weeks ago

Jli is BZ and reported this issue so I'll unassign myself

jliexpensify commented 2 weeks ago

Hmm this isn't an issue for me anymore - I had a period of slow internet while I was at a cafe yesterday and didn't see any problems. @coleaeason what about you - still seeing it?

jliexpensify commented 1 week ago

Had a report of this issue over the weekend from DB

jliexpensify commented 1 week ago

I just got this again, as I started this morning:

image

Precondition: Offline from ND for an extended period (e.g. 8 hours/overnight)

  1. Open laptop and click on #social
  2. Observe HTML and also notice no chats loaded since 2:38am AEST
  3. Type message and refresh
  4. Wait ~60 seconds
  5. See the channel title reflect properly

Is this something you can look into @techievivek?

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

muttmuure commented 1 week ago

Since this keeps happening, let's add the EXTERNAL label and see if we can get anyone in the community to identify what is going on here

truph01 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-09-25 09:14:43 UTC.

Proposal

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

What is the root cause of that problem?

{"html":"<strong>Everything about nothing</strong>"}

but our function to get the report's description text is:

https://github.com/Expensify/App/blob/48e9c176b3111870ebe5e6193312334e32c249ea/src/libs/ReportUtils.ts#L4100-L4106

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

function getReportDescriptionText(report: OnyxEntry<Report>): string {
    if (!report?.description) {
        return '';
    }
    try {
        const reportDescription = report?.description;
        const objectDescription = JSON.parse(reportDescription);
        if (objectDescription?.html) {
            return Parser.htmlToText(objectDescription.html);
        }
    } catch (e) {
        return Parser.htmlToText(report?.description);
    }
}
techievivek commented 1 week ago

Thanks for the proposal.

- Something, BE return the report's description is: ``` {"html":"Everything about nothing"} ```

Though we can handle this in the frontend, I think we can update the backend to be consistent here.

truph01 commented 1 week ago

@techievivek Sure. I just wanted to share my investigation and suggested solution. If a backend fix is more appropriate, please proceed with that.

techievivek commented 1 week ago

Hmm, I checked the backend, and it seems like there isn't much consistency in how this is handled. For example, here we extract the HTML part of the description: https://github.com/Expensify/Auth/blob/86e9858b23fdd6a3cf15932cf315a80616832083/auth/lib/Report.cpp#L9202, but in other places, we simply return the rnvp as it is, like here: https://github.com/Expensify/Auth/blob/86e9858b23fdd6a3cf15932cf315a80616832083/auth/command/PreviewReport.cpp#L33 and https://github.com/Expensify/Auth/blob/86e9858b23fdd6a3cf15932cf315a80616832083/auth/lib/ReportAction.cpp#L1805. I think the best solution would be to support both versions, as @truph01 suggested above.

CC @abdulrahuman5196, could you please review the above proposal?

melvin-bot[bot] commented 1 week ago

@coleaeason, @jliexpensify, @techievivek, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

jliexpensify commented 5 days ago

Waiting on a review from @abdulrahuman5196

abdulrahuman5196 commented 4 days ago

Checking now

abdulrahuman5196 commented 4 days ago

@techievivek / @jliexpensify Does there need to be any condition for us to repro this issue?

I tried creating a new room and provided some bold title and some other formatted subject. But I am unable to repro this issue. Even tried with slow network. No luck. Does it need to be in that same room? Or some other condition?

truph01 commented 4 days ago

@abdulrahuman5196 After creating a new room, you can try sending a new message to the room and see if the bug appears.

jliexpensify commented 4 days ago

Honestly, I am not sure - I have only seen this HTML issue for the #social channel. If I had to guess, it would be a channel with thousands of chats/threads? @techievivek what about you? Have you seen this pop up anywhere else or do you have any theories?

techievivek commented 3 days ago

As @truph01 mentioned, this issue can be reproduced by sending a new message to a room. The backend includes the description with an html key in every report update for new actions, so that seems to be how it's triggered. I could have updated the backend to send only plain text in the description, but there are other similar cases where the description is sent as an object with the html key so it's better to just handle both the variant in frontend.

melvin-bot[bot] commented 3 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 3 days ago

@coleaeason @jliexpensify @techievivek @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

quinthar commented 3 days ago

It feels like we should fix the bug in the back end, not create a weird front-end hack.

techievivek commented 2 days ago

@quinthar Aaah, I totally agree with your point and suggested the same here https://github.com/Expensify/App/issues/49355#issuecomment-2373515129. However, after digging deeper into the codebase, I noticed that the issue is actually rooted in the DB structure itself. For example, reports like tasks store the description as a plain string, while for rooms with HTML content, it’s stored under a separate html key. Because of this inconsistency in the DB, our backend is already trying to handle it in multiple places as mentioned here https://github.com/Expensify/App/issues/49355#issuecomment-2376801009

Since fixing it in the backend alone won’t fully resolve the issue without making changes to the way the data is stored, I think a better short-term solution is to make the frontend flexible enough to support both formats. This way, we won’t have to worry about breaking other parts of the app, and it’ll be a lot safer and quicker to implement. Let me know what you think, thanks.

quinthar commented 2 days ago

Well let's make changes to how the data is stored -- if we are storing it inconsistently in the database, let's fix that. Why are we storing HTML content in a JSON object with a single field? That feels like a bad design we should unwind.

techievivek commented 2 days ago

Yeah, let's clean this from the root, I have started a discussion on the #eng-chat here https://expensify.slack.com/archives/C03TQ48KC/p1727928821339289 to understand the historical reason and also discuss the potential solution.

melvin-bot[bot] commented 1 day ago

@coleaeason, @jliexpensify, @techievivek, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

abdulrahuman5196 commented 13 hours ago

Not overdue. Seems an internal discussion going on here. https://github.com/Expensify/App/issues/49355#issuecomment-2390481233