Skip to content

[Due for payment 2026-10-14] [$250] Chat - Editing a broken attachment link duplicates the URL #102671

Closed

Description

@luacmartins

Version Number: Not provided
Reproducible in staging?: Yes, confirmed by the reporter
Reproducible in production?: Not confirmed
If this was caught during regression testing, add the test name, ID and link from BrowserStack: Found while testing #101717; no BrowserStack test provided
Email or phone of affected tester (no customers): Not provided
Logs: Not provided
Expensify/Expensify Issue URL: N/A
Issue reported by: @mukhrr
Slack conversation: N/A — original report
App Component: Chat Report View
Device used: Desktop web; OS and browser not specified

Action Performed:

  1. Open a chat on staging and send a file attachment, such as a .csv file. Wait for the upload to finish.
  2. Open the message's context menu and select Edit comment.
  3. Delete the final ) from the attachment's Markdown link, optionally removing a few trailing URL characters as well, and save the message.
  4. Open Edit comment again and inspect the attachment link in the editor.
  5. Save another edit to the malformed link and reopen Edit comment to observe the behavior on subsequent edits.

Expected Result:

Opening Edit comment preserves the saved text, including an incomplete Markdown link. The editor should not duplicate the URL or insert additional Markdown link syntax. Subsequent edits should not accumulate extra copies of the link.

Actual Result:

Reopening a message containing a broken attachment link inserts an additional copy of the URL into the editor, wrapping the URL in Markdown link syntax inside the already incomplete attachment link. Further edits can introduce more nested/duplicated links, as shown in the recording.

For example, saved text resembling [file.csv](https://example.com/file is reopened with the URL expanded into another Markdown link inside that incomplete link.

The issue was discovered during review of #101717 and is also reproducible on staging. A follow-up fix was agreed in the PR discussion.

Workaround:

No confirmed workaround.

Platforms:

Reproduced on desktop web in the attached recording; the exact OS/browser and other platforms have not been confirmed.

  • Android: App
  • Android: mWeb Chrome
  • iOS: App
  • iOS: mWeb Safari
  • iOS: mWeb Chrome
  • Windows: Chrome
  • MacOS: Chrome / Safari

Screenshots/Videos

REC-20260929095734.mp4
Issue OwnerCurrent Issue Owner: @mallenexpensify
Upwork Automation - Do Not Edit

Activity

  1. added
    BugSomething is broken. Auto assigns a BugZero manager.
    on Sep 30, 2026
  2. luacmartins commented on Sep 30, 2026

    @luacmartins
    ContributorAuthor

    @mukhrr please comment on this issue so I can assign it to you

  3. mukhrr commented on Sep 30, 2026

    @mukhrr
    Contributor

    thanks. done

    @mukhrr please comment on this issue so I can assign it to you

  4. MelvinBot commented on Sep 30, 2026

    @MelvinBot
    Contributor

    Proposal

    What is the root cause of that problem?

    This is not specific to attachments. Any Markdown link with its closing ) deleted behaves the same way.

    1. When you save [file.csv](URL (no )), ExpensiMark's link rule no longer matches. The autolink rule then wraps the leftover URL, so the saved HTML is [file.csv](<a href="URL">URL</a>.
    2. When you reopen Edit comment, htmlToMarkdown turns every anchor into [text](href). The draft becomes [file.csv]([URL](URL).
    3. App tries to stop this in handleUserDeletedLinksInHtml, which strips autolinks the user deliberately broke (issues [HOLD for payment 2022-07-15] [$250] When sending text that NewDot thinks is a link, it adds the link markdown automatically and doesn't allow to edit #9090 and [$1000] Auto link does not work in edit mode #13221). But it only strips an anchor whose href exactly matches a link removed in this edit. So the guard fails when:
      • you delete any URL characters along with the ), because the href changes
      • you make any later edit, because the stored text no longer contains a link to compare against

    Each time a ) is broken again, another level of nesting is added.

    Code references and round-trip evidence

    I reproduced this with App's real Parser and handleUserDeletedLinksInHtml:

    Edit Saved HTML Reopened draft
    Delete csv) [file.csv](<a href="…/w_abc123.">…/w_abc123.</a> [file.csv]([…/w_abc123.](…/w_abc123.) ❌
    Delete only ) [file.csv](URL (guard works) [file.csv](URL ✅
    …then any later edit [file.csv](<a href="URL">URL</a> z [file.csv]([URL](URL) z ❌
    Non-attachment [Google](https://www.google.com/search), delete h) — [Google]([https://www.google.com/searc](https://www.google.com/searc) ❌

    This matches the PR discussion, which said the bug exists on staging and is not a regression.

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

    In handleUserDeletedLinksInHtml, after the new comment is parsed to HTML, unwrap any autolink anchor that directly follows a dangling ](, leaving its plain text. This extends the guard's existing intent: "don't autolink a link the user broke."

    Results from a local simulation:

    • Every edit round-trips exactly.
    • Messages that are already corrupted heal on their next save.
    • Typing the ) back creates a normal link again.

    Add unit tests for the cases above, in tests/actions/ReportTest.ts or a similar file.

    What alternative solutions did you explore? (Optional)

    • Fix it in expensify-common: Make the autolink rule, and its candidate scanner findUrlCandidates, skip a URL directly after ](. This also covers newly sent messages and live Markdown. However, it needs a public expensify-common PR and a version bump in App. It is also a product decision: should a dangling [text](url show a clickable URL?
    • Make htmlToMarkdown output a bare URL when the link text equals the href: Not recommended. It breaks existing expensify-common tests and the link-removal behavior App relies on.

    Open question: I couldn't confirm whether the backend re-autolinks the UpdateComment HTML. If it does, even the "delete only )" case would nest on the first reopen. The App-side fix still helps on the next save.


    Next Steps for Contributor+ team:
    To accept: @MelvinBot implement [this](https://github.com/Expensify/App/issues/102671) to create a draft PR.
    To refine: @MelvinBot <your feedback>
    To reject: Explain why you are rejecting Melvin's proposal.

  5. added
    ExternalAdded to denote the issue can be worked on by a contributor
    on Sep 30, 2026
  6. melvin-bot commented on Sep 30, 2026

    @melvin-bot

    Current assignee @brunovjk is eligible for the External assigner, not assigning anyone new.

  7. changed the title [-]Chat - Editing a broken attachment link duplicates the URL[/-] [+][$175] Chat - Editing a broken attachment link duplicates the URL[/+] on Sep 30, 2026
  8. melvin-bot commented on Sep 30, 2026

    @melvin-bot
  9. mukhrr commented on Oct 1, 2026

    @mukhrr
    Contributor

    Melvin's proposal only runs when an edit is saved (handleUserDeletedLinksInHtml), so it misses some cases. I checked on dev:

    • send a new message [Google](https://www.google.com/search and open Edit comment without changing anything, the draft is already [Google]([https://www.google.com/search](https://www.google.com/search)
    • messages that are already broken stay broken in the editor, a save with no changes is skipped in editReportComment

    RCA: when the ) is missing, the autolink rule wraps the URL after ](. On Edit comment, the anchor rule in htmlToMarkdown turns every anchor into [text](href), so URL gets nested inside the broken link. Backend isn't involved, UpdateComment stores the html exactly as we send it.

    Solution: fix the anchor rule in expensify-common, the same way it already returns a bare email for mailto: anchors. If an autolinked anchor comes right after ](, return the bare URL. The draft round-trips, already broken messages come back clean on open, and chat rendering doesn't change. Then bump expensify-common in App.

  10. mukhrr commented on Oct 1, 2026

    @mukhrr
    Contributor

    @brunovjk Expensify/expensify-common#950 is ready in expensify-common repo. Not sure you review it there or not.

  11. brunovjk commented on Oct 6, 2026

    @brunovjk
    Contributor

    Update: PR merged and waiting for next checklist deployment.

  12. 17 remaining items

  13. github-actions commented on Oct 14, 2026

    @github-actions
    Contributor

    🤖 ProposalPolice™ is tracking duplicate proposals for this issue using an OpenAI Conversation. This comment stores that Conversation's ID and can be safely ignored.

  14. changed the title [-][$250] Chat - Editing a broken attachment link duplicates the URL[/-] [+][Due for payment 2026-10-14] [$250] Chat - Editing a broken attachment link duplicates the URL[/+] on Oct 14, 2026
  15. melvin-bot commented on Oct 14, 2026

    @melvin-bot

    Triggered auto assignment to @mallenexpensify (Awaiting Payment)

  16. haiquywork commented on Oct 14, 2026

    @haiquywork

    Proposal

    What is the root cause of that problem?

    On save, the link rule can't match without ), so the autolink rule wraps only the bare URL. The stored HTML is [file.csv](<a href="URL">URL</a>. On Edit, saveReportActionDraft is fed Parser.htmlToMarkdown(getActionHtml(reportAction)), and ExpensiMark's anchor HTML-to-Markdown rule turns every anchor into [text](href). That nests a new link inside the dangling ](.

    const editAction = () => {
    saveReportActionDraft(
    originalReportID ?? reportID,
    reportAction,
    originalReportActions ?? reportActions,
    Parser.htmlToMarkdown(getActionHtml(reportAction), {reportIDToName}),
    isOffline,

    The existing guard handleUserDeletedLinksInHtml only strips anchors whose href matches a link removed in the same edit, so it misses cases where trailing URL characters were also deleted. It also never runs for a later unchanged reopen, because editReportComment returns early when the draft is unchanged.
    function handleUserDeletedLinksInHtml(
    newCommentText: string,
    originalCommentMarkdown: string,
    currentUserLogin: string,
    personalDetails: OnyxEntry<PersonalDetailsList>,
    videoAttributeCache?: Record<string, string>,
    ): string {
    if (newCommentText.length > CONST.MAX_MARKUP_LENGTH) {
    return newCommentText;
    }
    const userEmailDomain = isEmailPublicDomain(currentUserLogin) ? '' : Str.extractEmailDomain(currentUserLogin);
    const allPersonalDetailLogins = Object.values(personalDetails ?? {}).map((personalDetail) => personalDetail?.login ?? '');
    const htmlForNewComment = getParsedMessageWithShortMentions({
    text: newCommentText,
    userEmailDomain,
    availableMentionLogins: allPersonalDetailLogins,
    parserOptions: {
    extras: {videoAttributeCache},
    },
    });
    const removedLinks = Parser.getRemovedMarkdownLinks(originalCommentMarkdown, newCommentText);
    return removeLinksFromHtml(htmlForNewComment, removedLinks);
    }

    const originalCommentHTML = ReportActionsUtils.getReportActionHtml(originalReportAction);
    const originalCommentMarkdown = Parser.htmlToMarkdown(originalCommentHTML ?? '', {reportIDToName}).trim();
    const shouldRemoveQueuedAttachment = isUploadingAttachmentRemovedFromDraft(textForNewComment, originalCommentHTML);
    const draftForNewComment = replaceLocalAttachmentReferences(textForNewComment, originalCommentHTML, originalReportAction.reportActionID);
    // Skip the Edit if draft is not changed
    if (originalCommentMarkdown === draftForNewComment) {
    return;
    }
    const htmlForNewComment = handleUserDeletedLinksInHtml(draftForNewComment, originalCommentMarkdown, currentUserLogin, personalDetails, videoAttributeCache);

    The real defect is that the HTML-to-Markdown conversion isn't a faithful inverse of the autolink step. An anchor that was an autolink (text equals href) and sits right after a literal ]( should go back to the bare URL.

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

    • In expensify-common ExpensiMark.ts, change the anchor rule's replacement for HTML to Markdown. Use the replace offset and the full source string to check the preceding text.
    • If the anchor's text equals its href (an autolink) and the text before it ends with ](, return just the href instead of [href](href).
    const isAutolink = text === href;
    const followsLinkOpener = source.slice(0, offset).endsWith('](');
    if (isAutolink && followsLinkOpener) {
        return href;
    }
    • Bump the expensify-common version in App's package.json. Only the conversion changes, so the edit flow and handleUserDeletedLinksInHtml need no change. The reopened draft then equals the saved text, and repeated edits no longer grow.
    • Add a unit test in expensify-common's ExpensiMark HTML-to-Markdown tests: [file.csv](<a href="U">U</a> converts to [file.csv](U. Add a round-trip test (Markdown to HTML to Markdown) that stays stable across repeated cycles.
    • Add an App test in tests/unit/ReportUtilsTest or the existing edit-comment tests. It checks that Parser.htmlToMarkdown of a message with an unterminated attachment link returns the same text.

    What alternative solutions did you explore? (Optional)

    • Make autolink skip URLs after ](. This changes how messages are rendered and stored, and already-stored messages would still break on edit.
    • Strengthen handleUserDeletedLinksInHtml in App. It only runs on a changed save, so the first reopen is still wrong.
  17. trasnake87 commented on Oct 14, 2026

    @trasnake87
    Contributor

    Withdrawn. This issue was already fixed by @mukhrr and is awaiting payment; the Help Wanted label was only added for a moment to update the price, so it was never an open race. Sorry for the noise.

  18. melvin-bot commented on Oct 14, 2026

    @melvin-bot

    Payment Summary

    • Reviewer: @mukhrr owed $250 via NewDot
    • Reviewer: @brunovjk owed $250 via NewDot

    BugZero Checklist (@mallenexpensify)

    • I have confirmed assignees, roles, and Upwork contracts look correct
    • I have paid out Upwork contracts / manual NewDot requests
    • [BugZero Assignee] I have created a GH issue for creating/updating the regression test once above steps have been agreed upon
  19. mallenexpensify commented on Oct 16, 2026

    @mallenexpensify
    Contributor

    Payment Summary

    Contributor+: @mukhrr due $175 via NewDot

    @brunovjk are you due payment? If so, please link the PR.

  20. mukhrr commented on Oct 17, 2026

    @mukhrr
    Contributor

    payment is due $250 as Melvin was not correct on solution

    Reviewer: @mukhrr owed $250 via NewDot

    cc @mallenexpensify

  21. mallenexpensify commented on Oct 17, 2026

    @mallenexpensify
    Contributor

    payment is due $250 as Melvin was not correct on solution

    @mukhrr I'm unsure. The payment trigger is the Help Wanted label, and that label wasn't added until later.

    CONTRIBUTING.md says to post proposals only after Help Wanted is added. That rule also covers comments that say Melvin's solution is wrong or can be improved. We already get flooded with proposals once Help Wanted is added. If we pay for comments posted before the label, contributors will flood issues with improvements to Melvin's proposals in the hope of getting paid.

    I plan to propose something in the coming weeks to address this.

  22. mukhrr commented on Oct 18, 2026

    @mukhrr
    Contributor

    @mallenexpensify i unserstand your concern! But this is a bit different scenario where we don't "motivate" contributors to flood with proposals.

    I mean I didn't post proposal or comment before Help Wanted here. I got assigned intentionally since I had more context from previous related PR/issue. And I think this is good experience since our main goal on Expensify is to fix bugs faster and more regression free, isn't it?! So this is not something everybody can claim/flood with comments/proposals. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Awaiting PaymentAuto-added when associated PR is deployed to productionBugSomething is broken. Auto assigns a BugZero manager.DailyKSv2ExternalAdded to denote the issue can be worked on by a contributor

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions