Repository navigation
Remove unsafe type assertions from card messages and UI helpers - #100798
Conversation
Seatbelt-Operation: sign-273bdc63-e974836a-ffe5-4d41-b642-361af944871a
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.
|
Seatbelt-Operation: sign-repair-273bdc63-5f4a77f5-11f8-4586-b0d5-133db827eda9
KJ21-ENG
changed the title
Resolve receipt hover and middle ellipsis ref conflicts by retaining upstream ComponentRef signatures and the reviewed DOM guards.
Seatbelt-Operation: main-update-100798-20261016-sign-40962e38
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (isLeaf) { | ||
| if (stopNode.type === ElementType.Text && anchor.textEndIdx !== undefined) { | ||
| const truncated = {...stopNode, data: stopNode.data.slice(0, anchor.textEndIdx)} as unknown as AnyNode; | ||
| const truncated = Object.assign(stopNode.cloneNode(false), stopNode); |
There was a problem hiding this comment.
❌ CONSISTENCY-18 (docs)
Object.assign(stopNode.cloneNode(false), stopNode) is a non-obvious idiom. A reader cannot tell why the node is cloned and then immediately overwritten with the source node's own properties. The reason, that cloneNode(false) yields a value with the correct domhandler prototype/methods while Object.assign makes it an independent mutable copy so the data/children mutation below does not affect the original node, is not recoverable from the code. The same idiom is repeated at line 91, so a single comment explaining it helps both sites.
Add a short comment above the construct, e.g.:
// cloneNode(false) keeps the correct node prototype/methods; Object.assign copies the
// original's own properties so this is an independent, mutable shallow copy we can edit
// without mutating the shared source node.
const truncated = Object.assign(stopNode.cloneNode(false), stopNode);
truncated.data = stopNode.data.slice(0, anchor.textEndIdx);Reviewed at: 3cacff1 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Replaced Object.assign with direct shallow cloning. Kept an explicit parent link because the installed serializer uses it to preserve escaping and namespace behavior. Added regression coverage for both cases.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-17.at.18.26.51.mov |
| const styles = useThemeStyles(); | ||
| const session = useSession(); | ||
| const assigneeAccountID = (getOriginalMessage(action) as IssueNewCardOriginalMessage)?.assigneeAccountID; | ||
| const cardIssuedActionOriginalMessage = isCardIssuedAction(action) ? getOriginalMessage(action) : undefined; |
There was a problem hiding this comment.
🟡 src/components/ReportActionItem/IssueCardMessage.tsx:40
The only caller (ActionContentRouter.tsx:489) already renders this component behind if (isCardIssuedAction(action)), so checking isCardIssuedAction again here repeats a check the caller has already done.
Can we narrow the prop type instead, so getOriginalMessage(action) is typed directly ?
type IssueCardMessageProps = {
action: ReportAction<
| typeof CONST.REPORT.ACTIONS.TYPE.CARD_ISSUED
| typeof CONST.REPORT.ACTIONS.TYPE.CARD_ISSUED_VIRTUAL
| typeof CONST.REPORT.ACTIONS.TYPE.CARD_MISSING_ADDRESS
| typeof CONST.REPORT.ACTIONS.TYPE.CARD_ASSIGNED
| typeof CONST.REPORT.ACTIONS.TYPE.CARD_REPLACED_VIRTUAL
| typeof CONST.REPORT.ACTIONS.TYPE.CARD_REPLACED
>;
...
};
const originalMessage = getOriginalMessage(action);
const assigneeAccountID = originalMessage?.assigneeAccountID;
const companyCard = originalMessage ? cardList?.[originalMessage.cardID] : undefined;This also means a non-card action can't be passed in by mistake: today that would type-check and just render with no card or assignee.
There was a problem hiding this comment.
Narrowed IssueCardMessage's action prop to the six card-issued action variants already checked by its caller. The component now reads originalMessage directly.
| if (isLeaf) { | ||
| if (stopNode.type === ElementType.Text && anchor.textEndIdx !== undefined) { | ||
| const truncated = {...stopNode, data: stopNode.data.slice(0, anchor.textEndIdx)} as unknown as AnyNode; | ||
| const truncated = Object.assign(stopNode.cloneNode(false), stopNode); |
There was a problem hiding this comment.
🟡 src/libs/ReportActionFollowupUtils/tokenizeForReveal.ts:81
Following up on the bot's comment here: rather than adding a comment, I think Object.assign can just go. It copies every own field of stopNode back over the clone (attribs, children, parent, prev, next), which undoes most of what cloneNode(false) just did.
cloneNode already returns a typed Text / Element, so this does the same job:
const truncated = stopNode.cloneNode(false);
truncated.data = stopNode.data.slice(0, anchor.textEndIdx);
...
const partialElem = stopNode.cloneNode(false);
partialElem.children = innerChildren;dom-serializer only reads type, name, attribs, data and children, so the parent/prev/next links aren't needed. The same applies to :92.
There was a problem hiding this comment.
Replaced Object.assign with direct cloneNode calls. I retained each clone's parent link: removing it changes serialization for noscript text and MathML line breaks. Added tests covering those cases at every reveal stage; all 254 selected tests pass.
|
@KJ21-ENG Tag me once the (2) code-comments are addressed 🙌 |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Seatbelt-Operation: reviewer-fix-100798-dd013b03-51e7-45d0-bb8f-222283b35d62
|
@ikevin127 Could you please check this once you get a chance, Thanks! |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Both of comments were addressed, and on the parent link the author was right and I wasn't. Thanks for addressing everything!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 blimpich 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.5.15-1 🚀
|
|
No help site update is needed. This PR removes unsafe type assertions and adds type guards, so users see no change in behavior or names. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Use existing card-action and HTML-element guards in card messages, receipt hover, input-focus tracking, and filename measurement. Build progressive-reveal nodes with typed shallow clones. Supported card messages and HTML controls keep their existing behavior; non-HTML targets are ignored.
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
Relevant upstream TypeScript and Jest checks passed.
tests/unit/libs/ReportActionFollowupUtils/tokenizeForRevealTest.tscovers progressive text and nested/atomic markup;tests/unit/hooks/usePendingConciergeResponse.test.tscovers reveal completion and original HTML.Run on supported web browsers. The receipt-hover case requires a pointer-capable desktop.
Receipt hover
Setup: Open an editable expense thread with a receipt and Add additional receipt available.
Long filename
Setup: Use an account already eligible for Wallet → bank account → Enable global reimbursements → Agreements. Do not accept agreements or submit the document.
Offline tests
Not applicable: these changes guard existing DOM targets and clone parsed HTML; network requests, persistence, and reconnect behavior are unchanged.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Recordings (1)
Windows desktop Chrome: receipt-button hover, picker selection and hover recovery were documented as passing. Receipt images loaded unreliably, and console errors were observed without confirmed attribution to this PR. Filename resizing and mobile-web/Safari results are unverified.
01-receipt-hover-single-selection-desktop.mp4