[No QA] Investigate workflow job failing on main: typecheck / typecheck#75065
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| }); | ||
| // @ts-expect-error - will be solved in https://github.com/Expensify/App/issues/73830 | ||
| failureData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, |
There was a problem hiding this comment.
⚠️ Incomplete Fix - Multiple @ts-expect-error Comments Remain
Issue: This PR removes only ONE @ts-expect-error comment referencing issue #73830, but there are at least TWO other identical comments in the same updateSplitTransactions function that also reference the same issue:
-
Line 14194:
// @ts-expect-error - will be solved in https://github.com/Expensify/App/issues/73830failureData.push({ onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.TRANSACTION}${originalTransactionID}`, value: originalTransaction, // Can be null/undefined });
-
Line 14242:
// @ts-expect-error - will be solved in https://github.com/Expensify/App/issues/73830failureData.push({ onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`, value: { data: { [`${ONYXKEYS.COLLECTION.TRANSACTION}${originalTransactionID}`]: originalTransaction, }, }, });
Root Cause: The type issue stems from OnyxUpdate's value property not accepting nullable values, while:
originalTransactionis typed asOnyxCollection<OnyxTypes.Transaction>(can benull | undefined)transactionThreadis typed asOnyxEntry<OnyxTypes.Report> | null(can benull)
Recommendation:
- If issue [Due for payment 2026-05-11] [$250] Restore Type Safety for Onyx.update() #73830 is resolved: Remove ALL THREE
@ts-expect-errorcomments, not just this one - If the type issue persists: Keep all suppressions until the underlying type definitions are fixed
- Verify type safety: Run
npm run typecheckto ensure no type errors are introduced
Question: Why was only this specific comment removed while the others remain? This suggests either an incomplete fix or that this specific case is somehow different from the others.
| }); | ||
| // @ts-expect-error - will be solved in https://github.com/Expensify/App/issues/73830 | ||
| failureData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, |
There was a problem hiding this comment.
🚨 Critical: Incomplete Fix - 24 Total Suppressions Remain
Severity: HIGH - This PR appears to be incomplete or investigative only.
Finding: There are 24 total instances of @ts-expect-error comments referencing issue #73830 throughout src/libs/actions/IOU.ts, but this PR removes only 1 of them (line 14224).
All 24 locations:
Lines: 1771, 4568, 4668, 4678, 6424, 8121, 8576, 8745, 8894, 9358, 10316, 10728, 11682, 11696, 12155, 12379, 12628, 12846, 13254, 13595, 13623, 14194, 14224, 14242
Analysis:
- PR Title: "[No QA] Investigate workflow job failing on main: typecheck / typecheck" suggests this is investigative
- Inconsistency: If the underlying type issue was fixed, ALL 24 suppressions should be removable
- Risk: Removing only one suppression while others remain indicates:
- The type issue is NOT fully resolved
- This specific line may now cause a type error
- The typecheck failure investigation may be incomplete
Required Actions:
- ✅ Verify issue [Due for payment 2026-05-11] [$250] Restore Type Safety for Onyx.update() #73830's actual resolution status
- ✅ Run
npm run typecheckto confirm no new type errors are introduced - ✅ Decide:
- If [Due for payment 2026-05-11] [$250] Restore Type Safety for Onyx.update() #73830 is resolved: Remove ALL 24 suppressions in one comprehensive PR
- If [Due for payment 2026-05-11] [$250] Restore Type Safety for Onyx.update() #73830 is NOT resolved: Revert this change or add back the suppression
- ✅ Document: Update the PR description to explain why only this specific instance was removed
Recommendation: Given the PR title mentions "investigating" a typecheck failure, this appears to be exploratory work. Consider whether this single-line removal should be merged without addressing all 24 instances.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ 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/luacmartins in version: 9.2.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.59-5 🚀
|
Explanation of Change
Fixed Issues
$ #75064
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop