fix: Attachment - Edit comment option is shown for .doc file#90203
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa1b8bcdb8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…ent-Edit-comment-option-is-shown-for-.doc-file
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21b9a79815
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d42f03f154
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Code ReviewOverall: Solid fix with good test coverage. The root cause analysis is correct — All CI checks pass. A few items to consider:
|
| let attachmentCount = 0; | ||
| let hasOtherContent = false; | ||
| // Skip the filename text inside an open <a>/<video> so it isn't counted as user content. | ||
| let openAttachmentTag: string | null = null; |
There was a problem hiding this comment.
isInsideAttachmentTag makes more sense.
Also do perf regression test with very long chat history.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
This happens in production so out of scope but preview changes to normal text even after renaming file only. bug.mov |
| // Skip the filename text inside an open <a>/<video> so it isn't counted as user content. | ||
| let openAttachmentTag: string | null = null; | ||
|
|
||
| const parser = new HtmlParser({ |
There was a problem hiding this comment.
Minor concern — per-call parser instantiation: A new
HtmlParseris created on every call. This function is called in hot paths:PureReportActionItem(list renders),ReportActionItemFragment,OptionsListUtils(LHN). For typical volumes this is fine — the codebase already accepts this pattern inextractAttachments.ts— but worth noting for future perf profiling if list-heavy chats feel slow.
@MelvinBot suggest changes for code optimization to reduce performance impact.
Especially, early return if possible to avoid calling HtmlParser unnecessarily.
|
Two optimizations that avoid the 1. Early return when Non-attachment messages will never have this attribute. A simple substring check rejects them before parser instantiation: if (!message.html.includes(CONST.ATTACHMENT_SOURCE_ATTRIBUTE)) {
return false;
}This should go right after the existing 2. Early return for After the server drops if (message.text === CONST.ATTACHMENT_MESSAGE_TEXT && message.html.includes(CONST.ATTACHMENT_SOURCE_ATTRIBUTE)) {
return true;
}This preserves the old fast path for images/common attachments while only falling through to the parser for Together these two checks would eliminate parser instantiation for ~95%+ of calls (all non-attachment messages + standard image attachments), limiting the |
…ent-Edit-comment-option-is-shown-for-.doc-file # Conflicts: # Mobile-Expensify # android/app/build.gradle # ios/NewExpensify/Info.plist # ios/NotificationServiceExtension/Info.plist # ios/ShareViewController/Info.plist # package-lock.json # package.json # patches/react-native-onyx/details.md # patches/react-native-onyx/react-native-onyx+3.0.69.patch # src/ROUTES.ts # src/SCREENS.ts # src/components/Icon/DefaultBotAvatars.ts # src/hooks/useParticipantSubmission.ts # src/languages/de.ts # src/languages/en.ts # src/languages/es.ts # src/languages/fr.ts # src/languages/it.ts # src/languages/ja.ts # src/languages/nl.ts # src/languages/pl.ts # src/languages/pt-BR.ts # src/languages/zh-hans.ts # src/libs/API/parameters/index.ts # src/libs/API/types.ts # src/libs/Avatars/PresetAvatarCatalog.types.ts # src/libs/ChronosUtils.ts # src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx # src/libs/Navigation/linkingConfig/RELATIONS/SETTINGS_TO_RHP.ts # src/libs/Navigation/linkingConfig/config.ts # src/libs/Navigation/types.ts # src/libs/PolicyUtils.ts # src/libs/actions/Agent.ts # src/libs/actions/IOU/MoneyRequest.ts # src/libs/actions/IOU/index.ts # src/libs/actions/connections/index.ts # src/pages/iou/request/step/IOURequestStepAmount.tsx # src/pages/iou/request/step/IOURequestStepCategory.tsx # src/pages/iou/request/step/IOURequestStepCategoryCreate.tsx # src/pages/iou/request/step/IOURequestStepDestination.tsx # src/pages/iou/request/step/IOURequestStepDistance.tsx # src/pages/iou/request/step/IOURequestStepDistanceOdometer.tsx # src/pages/iou/request/step/IOURequestStepDistanceRate.tsx # src/pages/iou/request/step/IOURequestStepHours.tsx # src/pages/iou/request/step/IOURequestStepPerDiemWorkspace.tsx # src/pages/iou/request/step/IOURequestStepReport/hooks/useReportSelectionActions.ts # src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx # src/pages/iou/request/step/IOURequestStepTimeWorkspace.tsx # src/pages/iou/request/step/IOURequestStepUpgrade.tsx # src/pages/settings/Agents/AddAgentPage.tsx # src/pages/settings/Agents/AgentsPage.tsx # src/pages/settings/Agents/EditAgentPage.tsx # src/pages/settings/InitialSettingsPage.tsx # src/pages/settings/Profile/ProfilePage.tsx # src/pages/workspace/WorkspaceInitialPage.tsx # src/pages/workspace/travel/WorkspaceTravelInvoicingSection.tsx # src/types/onyx/AgentPrompt.ts # tests/actions/IOU/IOUSettersTest.ts # tests/actions/IOUTest.ts # tests/unit/AgentActionTest.ts # tests/unit/pages/settings/EditAgentPageTest.tsx
Should we tackle it here since we got familiar with the flow for this issue? |
No, follow-up if needed. This also requires approval from product team. |
…ent-Edit-comment-option-is-shown-for-.doc-file # Conflicts: # Mobile-Expensify # android/app/build.gradle # ios/NewExpensify/Info.plist # ios/NotificationServiceExtension/Info.plist # ios/ShareViewController/Info.plist # package-lock.json # package.json
Updated Code Review (post May 17 changes)Since the initial review, the PR addressed all feedback — renaming What changed since last review:
Review of current state: The implementation is clean and well-structured. The layered fast-paths ( The No issues found. Looks good to merge. |
Regression AnalysisI investigated all callers of the two modified functions ( TL;DR: No data-corruption regressions found. All No regressions found (all positive behavioral changes)Every caller that now receives
WeakMap memoization: safeOnyx's Items to confirm are intentional (not regressions per se)
Next Steps: Reply with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b2d71d8a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ent-Edit-comment-option-is-shown-for-.doc-file
…ent-Edit-comment-option-is-shown-for-.doc-file
…ent-Edit-comment-option-is-shown-for-.doc-file
|
Please merge main |
Regression AnalysisI reviewed the diff and traced all call sites. Here are the potential regressions, ranked by severity: 1.
|
| # | Issue | Severity | Functional risk |
|---|---|---|---|
| 1 | translationKey check no longer validates text |
Medium | Could misclassify attachment+text as attachment-only |
| 2 | WeakMap cache miss on OptionsListUtils ad-hoc objects | Low-Medium | Perf regression on LHN for high-traffic accounts |
| 3 | Case-sensitive attribute matching | Low | Unlikely server casing mismatch |
| 4 | Synced attachments now editable | Low | Intentional, matches product intent |
No showstoppers found. Item #1 is the most worth guarding against — a unit test for {text: "user text", translationKey: "common.attachment", html: "<a data-expensify-source=...>file</a>"} would confirm whether the current behavior is intentional or a gap.
…ent-Edit-comment-option-is-shown-for-.doc-file # Conflicts: # Mobile-Expensify # android/app/build.gradle # cspell.json # ios/NewExpensify/Info.plist # ios/NotificationServiceExtension/Info.plist # ios/ShareViewController/Info.plist # package-lock.json # package.json # src/components/HTMLEngineProvider/HTMLRenderers/VictoryChartRenderer/components/VictoryChartBar.tsx # src/components/HTMLEngineProvider/HTMLRenderers/VictoryChartRenderer/components/VictoryChartLine.tsx # src/components/MoneyReportHeaderPrimaryAction/SubmitPrimaryAction.tsx # src/libs/Navigation/AppNavigator/Navigators/TabNavigatorBar.tsx # src/libs/ReportUtils.ts # src/libs/actions/IOU/MoneyRequestBuilder.ts # src/pages/TransactionDuplicate/DuplicateTransactionItem.tsx
|
Invalid |
|
🚧 @stitesExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.3.90-0 🚀
Bundle Size Analysis (Sentry): |
Help site review: no documentation changes requiredI reviewed the changes in this PR against the help articles under Why: This is an internal bug fix to attachment-detection and edit-gating logic. It changes no user-facing labels, tabs, buttons, settings, or workflows, and it adds no new feature:
The fix restores the already-agreed product behavior (attachments are editable after sync; only blocked during the optimistic upload window) and corrects the styling/LHN misclassification — none of which alters documented behavior. Because no changes are required, I did not create a draft help site PR. If you'd like the Edit or Delete Messages section expanded to explicitly call out that attachment messages can be edited after they finish uploading, let me know with @TaduJR, please confirm you agree no help site changes are needed for this PR. If you'd like a docs PR drafted anyway, reply and I'll create one. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.90-3 🚀
|
5 participants
Explanation of Change
Fixes the "Edit comment" option incorrectly appearing for
.doc/.pdfattachment-only messages after navigating away and back, plus the related (pre-edit) styling/LHN misclassification.Two interacting failures in the original code:
isReportMessageAttachmentreturnedfalsefor.docattachment-only messages after theOpenReportresponse. The server returnsmessage.text = "filename URL"for documents (instead of"[Attachment]"like it does for images), so the existingtext === "[Attachment]"andStr.isVideo(text)checks both missed it — cascading into wrong styling (TextCommentFragmentinstead ofAttachmentCommentFragment) and wrong LHN text.canEditReportActionhad two guards that both got bypassed:!isReportMessageAttachment(message)(from 1) and((!isAttachmentWithText && !isAttachmentOnly) || !isOptimisticAction)—isOptimisticActionis cleared tonullbysuccessData, so!null = truerendered theisAttachmentOnlycheck meaningless after sync.Changes (6 files):
src/libs/isReportMessageAttachment.ts— replaced the brittletext-based logic with structural detection:data-expensify-source="token (precise — leading space +="so a URL query param isn't a false positive), it's not an attachment. Skips the parser for the vast majority of messages.text === "[Attachment]"→ attachment-only (covers standard image/video). Only.doc/.pdf(text = "filename URL") reach the parser.htmlparser2parse (already used inextractAttachments.ts,SelectionScraper,ReportActionFollowupUtils): attachment-only = one or more attachment tags and nothing else; any text/element outside them → attachment+text. A depth counter ensures markup nested inside an<a>/<video>(filename, formatting) isn't counted as user content.WeakMapmemoization keyed by the immutable OnyxMessageobject: list re-renders pass the same object, so the parser runs at most once per message rather than once per render.src/libs/ReportUtils.ts(canEditReportAction) — removedisReportMessageAttachmentfrom edit gating (it's a display/LHN signal, not a permission gate — conflating them was the root cause) and replaced the flawed flags check with a singleisOptimisticAttachmentguard. Per the agreed product behavior, all attachments are editable after sync; only block during the optimistic upload window.cspell.json— whitelistontext(htmlparser2 callback, alongside existingonopentag/onclosetag).tests/unit/isReportMessageAttachmentTest.ts(19 cases) &tests/unit/ReportUtilsTest.ts(+5canEditReportActioncases) — cover doc/pdf/video/image attachment-only & attachment+text, the two codex-bot regression guards (leading user link, trailing caption), nested markup,translationKey ≠ ATTACHMENT, undefined/empty input, and the optimistic-vs-synced edit matrix.tests/perf-test/isReportMessageAttachment.perf-test.ts(new) — Reassure benchmark over a 10k-message chat history (the hot path situchan flagged).Performance: measured vs the original regex (≈ what's on
main): the final fast-path + memoized version is −69.5% 🟢🟢 on the 10k-message pass — i.e., faster thanmainon the realistic hot path (list re-renders of stable objects), and correct everywhere.Fixed Issues
$ #74031
PROPOSAL: #74031 (comment)
Tests
.docfile (no text).doc→ "Edit comment" IS shown.docrenders with its attachment border + icon (not plain text)[Attachment]Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android-Native.mp4
Android: mWeb Chrome
Android-mWeb.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
iOS-Safari.mp4
MacOS: Chrome / Safari
Mac-Chrome.mp4