Skip to content

Disable Reply in thread on moved system messages - #99721

Draft
MelvinBot wants to merge 2 commits into
mainfrom
claude-disableThreadOnMovedSystemMessages
Draft

Disable Reply in thread on moved system messages#99721
MelvinBot wants to merge 2 commits into
mainfrom
claude-disableThreadOnMovedSystemMessages

Conversation

@MelvinBot

Copy link
Copy Markdown
Contributor

Explanation of Change

Moved system messages (MOVED / MOVEDTRANSACTION) can't be threaded server-side, but the app still offered "Reply in thread" on them. Tapping it fired a thread-creation call against an action the server never adopted, which failed and surfaced "Unexpected error creating this chat. Please try again later."

The earlier fix (Expensify/App#95979) only closed the narrow "create report from a draft expense" repro. This change gates moved system messages out of the thread UI at the source, in shouldDisableThread, so the doomed thread-creation call is never made.

I used the guarded approach from the issue rather than blanket-adding the actions to THREAD_DISABLED: threading is disabled only when the moved action has no threaded child (!reportAction?.childVisibleActionCount), mirroring how deleted/archived actions are already handled. This preserves any moved actions users have legitimately threaded.

As noted in the issue, the backend root-cause (why the moved action can't be threaded at all) is tracked internally and is out of scope here — this is the defensive FE fix that stops the user-facing error.

AI Tests run locally
  • npm test -- tests/unit/ReportUtilsTest.ts -t "shouldDisableThread" → pass (17 tests, incl. 4 new)
  • npm run typecheck → pass
  • npx eslint on changed files → 0 errors (only pre-existing grandfathered warnings)
  • oxfmt formatting → clean

Fixed Issues

$ #99653
PROPOSAL: #99653

Tests

  1. Open a workspace chat that has an expense report.
  2. Move an expense from one already-submitted report to another (so a moved system message appears that legitimately renders in the chat).
  3. Long-press / hover the moved system message and open the context menu.
  4. Verify "Reply in thread" is not offered on the moved system message.
  5. Verify the "Unexpected error creating this chat. Please try again later." error no longer appears.
  6. Verify a moved action that already has an existing thread still shows/opens its thread normally.
  7. Verify that no errors appear in the JS console.
  • Verify that no errors appear in the JS console

Offline tests

Same as tests.

QA Steps

  1. Open a workspace chat that has an expense report.
  2. Move an expense between two already-submitted reports so a moved system message appears in the chat.
  3. Long-press / hover the moved system message and open the context menu.
  4. Verify "Reply in thread" is not offered.
  5. Verify "Unexpected error creating this chat. Please try again later." does not appear.
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

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

Gate MOVED / MOVEDTRANSACTION system messages out of the thread UI unless they already have a threaded child, so the doomed thread-creation call is never made and the Unexpected error creating this chat message stops appearing.

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Comment thread src/libs/ReportUtils.ts Outdated
Comment on lines +11700 to +11701
// Moved system messages can't be threaded server-side, so offering "Reply in thread" leads to a failed thread-creation call.
// Gate them out of the thread UI, but keep any moved actions that have already been legitimately threaded.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MelvinBot Better to add in function jsdoc similar to others, not here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — moved the note into the shouldDisableThread JSDoc as a new bullet alongside the other conditions, and removed the inline comment. Pushed as 4da019c.


view run

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Ran the testing steps from this PR on Web.

Outcome: ⚠️ Partial — the move flow works cleanly (moved system message renders, no "Unexpected error creating this chat" toast, no failed network calls, no console-level failures), but I could not open the per-message context menu in this headless web session (hover, right-click, and long-press were all no-ops / unsupported), so I couldn't directly screenshot that "Reply in thread" is absent. That specific step should be confirmed by a human. The source diff does gate it: shouldDisableThread in src/libs/ReportUtils.ts now adds (isMovedSystemMessage && !reportAction?.childVisibleActionCount).

Setup performed: created a workspace, two manual expenses (Test Merchant A €25, Test Merchant B €40) each in its own report, submitted both reports, then used Spend → Expenses → "Move to report" to move Test Merchant A's expense into the Test Merchant B report — producing a MOVED system message.

Step Status Observation
1. Moved system message appears after moving an expense between two already-submitted reports ✅ Pass Detail shows "…moved this expense from Expense Report 2026-08-28"; Report B grew to 2 expenses (€65.00), confirming the move.
2. Context menu does NOT offer "Reply in thread" on the moved message ⚠️ Unverified in-UI Could not open the context menu in this headless session (hover/right-click/long-press unsupported), so the menu's absence wasn't screenshotted. No "X replies" thread affordance renders under the moved message, and the code gates the item off — but needs human confirmation.
3. "Unexpected error creating this chat. Please try again later." does not appear ✅ Pass Not observed anywhere across the full move flow; post-move network dump showed only 200s, zero 4xx/5xx.
4. A moved action with an existing thread still shows/opens its thread ⚠️ Not reproducible No UI path remains to create a pre-existing threaded moved action in a fresh env; the !childVisibleActionCount guard exists to preserve such threads, but none existed here to exercise.
5. No errors appear in the JS console ✅ Pass (proxy) Direct console capture isn't supported for --platform web; as a proxy, the post-move network dump had zero failed requests and no error banners appeared.
Screenshots

Moved expense system message rendered in chat

Moved expense system message rendered in chat

Report B now holds 2 expenses (€65.00) after the move — no error toast

Report B now holds 2 expenses after the move

Hover over the moved system message — no context-menu toolbar rendered

Hover attempt over the moved system message

Baseline hover on a control message — also no toolbar (session-wide limitation)

Baseline hover attempt showing session-wide hover limitation

view run

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants