elizaOS
fix: re-enable coverage report upload to Codecov in CI workflow
Relates to:
Continuous Integration workflow improvements
Risks
Low - Changes to CI workflow configuration only. Main risk would be potential CI pipeline failures if any configuration is incorrect.
Background
What does this PR do?
Updates the GitHub Actions CI workflow configuration with the following changes:
- Adds code coverage reporting to Codecov
What kind of change is this?
Improvements (misc. changes to existing features)
- Enhances the existing CI pipeline with better package management and test coverage reporting
Documentation changes needed?
My changes do not require a change to the project documentation.
Testing
Where should a reviewer start?
- Review the .github/workflows/ci.yaml file
- Check the workflow runs on both push to main and pull requests
- Verify all job steps are properly configured
Detailed testing steps
- Create a test PR to verify the workflow triggers correctly
- Ensure all steps complete successfully:
- PNPM installation
- Dependencies installation
- Prettier checks
- Linting
- Test environment setup
- Test execution
- Package building
- Code coverage reporting
- Verify Codecov integration is working by checking coverage reports
The automated CI workflow will validate all changes.
Deploy Notes
No special deployment needed - changes only affect CI pipeline configuration.
Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick!
🚨 Edited by proposal-police: This proposal was edited at 2024-12-27 02:22:35 UTC.
🚨 Edited by proposal-police: This proposal was edited at 2024-12-27 02:22:35 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
When clicking Approve button, it is changed from loading to Approve and then Pay despite of needed data given.
What is the root cause of that problem?
There is no flag and trigger to decide whether the loading icon should show or not.
isActionLoading and action merged here:
https://github.com/Expensify/App/blob/1a6f48ea0e2139a82d0a96578badbf3f761bcf00/src/libs/actions/Search.ts#L245-L262
When Approve button is clicked then createActionLoadingData(true); is activated to save Onyx data as optimisticData so it gets loading icon. After getting the response from BE, it will be createActionLoadingData(false); as finallyData.
[info] [Network] Finished API request in 356ms - {"command":"ApproveMoneyRequestOnSearch","jsonCode":200,"requestID":"8ea2c808c9103152-ICN"}
Just after the response, before the response data is updated into Onyx, isActionLoading is updated to false already but text Approve of the button isn't changed yet by Onyx update because Onyx data of the http response isn't merged in the client app yet.
What changes do you think we should make in order to solve the problem?
We should add a flag/trigger to delay isActionLoading to be false untill ApproveMoneyRequestOnSearch API is fnished to be updated in Onyx.
When the user clicks Approve button, then we should save the action Approve of the row as previousActionItem and should update isActionLoading to true in Onyx. When the action is changed into Pay from BE API then in useEffect, previousActionItem will be reset and isActionLoading will be set to false at Onyx.
const [previousActionItem, setPreviousActionItem] = useState('')
useEffect(() => {
// 2) When transactionItem.action is changed, then `isActionLoading: false` and it resets `previousActionItem`.
if (previousActionItem && transactionItem.action !== previousActionItem && transactionItem.isActionLoading) {
setPreviousActionItem('');
setIsActionLoading(currentSearchHash, transactionItem, false);
}
}, [transactionItem.action, currentSearchHash, item, previousActionItem, transactionItem])
// 1) When the button is clicked then `isActionLoading: true` and previousActionItem is set.
const handlePreviousActionItem = useCallback(() => {
if (!previousActionItem && !transactionItem.isActionLoading) {
setIsActionLoading(currentSearchHash, transactionItem, true);
setPreviousActionItem(transactionItem.action);
}
}, [currentSearchHash, transactionItem])
...
<TransactionListItemRow
onButtonPress={handlePreviousActionItem}
/>
...
We add setIsActionLoading function as libs/actions/Search.ts. This function will take a part to update isActionLoading Onyx in the search tab.
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/libs/actions/Search.ts#L390
function setIsActionLoading(hash: number, item: TransactionListItemType | ReportListItemType, isActionLoading: boolean) {
const reportIDList = [item?.reportID]
const transactionIDList = isTransactionListItemType(item) ? [item.transactionID] : undefined;
hash)
Onyx.merge(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`, {
data: transactionIDList
? (Object.fromEntries(transactionIDList.map((transactionID) => [`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {isActionLoading}])) as Partial<SearchTransaction>)
: (Object.fromEntries(reportIDList.map((reportID) => [`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {isActionLoading}])) as Partial<SearchReport>),
});
}
export {
setIsActionLoading,
...
We don't need to update isActionLoading as optimistic or finally data so delete it below:
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/libs/actions/Search.ts#L287-L289
Since Submit, Pay and Approve use isActionLoading process too, we should apply these 3 functions below. We should delete optimisticData and finallyData which handle isActionLoading:
approveMoneyRequestOnSearch:
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/libs/actions/Search.ts#L287-L291
submitMoneyRequestOnSearch:
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/libs/actions/Search.ts#L272
payMoneyRequestOnSearch`:
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/libs/actions/Search.ts#L307-L315
Above 3 are leads to this handleActionButtonPress. It handle the button press logic.
Details:
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/libs/actions/Search.ts#L64-L71
which is triggered here:
https://github.com/Expensify/App/blob/1721cf48574b1093556a36405d9f4774df375b29/src/components/SelectionList/Search/TransactionListItem.tsx#L78-L82
Then whenever clicking the button in search tab to update isActionLoading: true, if action item is different to previous action item, then it will be isActionLoading: false at Onyx.
Here is draft PR
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
N/A
What alternative solutions did you explore? (Optional)
N/A
Job added to Upwork: https://www.upwork.com/jobs/~021863569891865520990
Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)
@isabelastisser, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!
On @jacobkim9881 's proposal here https://github.com/Expensify/App/issues/53265#issuecomment-2511453251, I think the root cause seems to be correct. But I don't agree on the solutions suggested, seems kind of a workaround solutions to solve this issue.
@abdulrahuman5196 The alternative way is setting and loading values on FE side. It will work on when clicking buttons on Search by using useRef and useMemo on every button components on Search panel.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@abdulrahuman5196 The alternative way is setting and loading values on FE side. It will work on when clicking buttons on Search by using
useRefanduseMemoon every button components on Search panel.
The alternative option is also a workaround solution to solve this issue. I would prefer some solid solution to fix the issue.
@abdulrahuman5196, please provide an update. Thanks!
No proposal to review or approve yet.
@isabelastisser @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@isabelastisser, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Since RCA isActionLoading is not synced with approveMoneyRequestOnSearch API, we can get a response of isActionLoading from BE.
Delete finallyData from API then it will not change into Approve back. And then get isActionLoading: false from the response of approveMoneyRequestOnSearch API.
From
https://github.com/Expensify/App/blob/0d17435fa6e7fa79865de14aaec08e4b1240386c/src/libs/actions/Search.ts#L288-L291
to
const failureData: OnyxUpdate[] = createOnyxData({hasError: true, isActionLoading: false});
API.write(WRITE_COMMANDS.APPROVE_MONEY_REQUEST_ON_SEARCH, {hash, reportIDList}, {optimisticData, failureData});
To make BE send isActionLoading: false as a response, BE should fix the code too.
To make BE send isActionLoading: false as a response, BE should fix the code too.
@jacobkim9881 , IMO isActionLoading is a frontend flag to show the loader, not a backend responsibility AFAIK and backend doesn't know about this flag. I am not fully inclined for a backend change also for this. This seems like a classic concurrency issue. Do you see any other place in the app, we use the same pattern you are suggesting as a fix for showing loader properly?
Edited by proposal-police: This proposal was edited at 2024-12-17 14:15:08 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Search - Approve button appears briefly and changes to Pay after clicking Approve
What is the root cause of that problem?
On approve request we set isActionLoading to true so that we show the loading indicator until the respond finishes and when the request succeeds we set the isActionLoading on the transaction item back to false optimistically here
https://github.com/Expensify/App/blob/1a6f48ea0e2139a82d0a96578badbf3f761bcf00/src/libs/actions/Search.ts#L245-L262
But the way we determine the action for the transaction is via getAction here
https://github.com/Expensify/App/blob/4b0f250c71d4cc214107ff84797b24365964a5d0/src/libs/SearchUIUtils.ts#L227
which determines the action based on the expense report status linked to the transaction.
In order to set the action to pay canIOUBePaid should be true here
https://github.com/Expensify/App/blob/4b0f250c71d4cc214107ff84797b24365964a5d0/src/libs/SearchUIUtils.ts#L303-L306
but the problem is the update for statusNum of the expense report arrives a little bit late and it is still in submitted state (statusNum = 1) when the API response arrives and I have checked that the update for the expense report doesn't even get returned as part of ApproveMoneyRequestOnSearch API response. It arrives separately after a while so we briefly see the approve button again.
What changes do you think we should make in order to solve the problem?
Option 1: return the updated statusNum of the expense report as part of the ApproveMoneyRequestOnSearch API response and this problem will be solved
Option 2: If we want to fix from FE what we can do is set statusNum and stateNum of the expense report to some unusual value of -1 for instance optimistically here
https://github.com/Expensify/App/blob/1a6f48ea0e2139a82d0a96578badbf3f761bcf00/src/libs/actions/Search.ts#L245-L262
and set it back to the original value on failureData and then for ActionCell
https://github.com/Expensify/App/blob/476f3979f1ae6b89eee5b853f7368603d50403b0/src/components/SelectionList/Search/TransactionListItemRow.tsx#L455
we will add || condition to display loading also for this case of statusNum of the expense report is -1 so that it will display loading until the BE update of the status arrives 👍
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
What alternative solutions did you explore? (Optional)
@abdulrahuman5196 Thanks for reviewing. I agree with that we should approach from FE. We can change isActionLoading: true from false when action is changed. After BE gives a response Onyx updates and then action item will be changed on components. Before we have defined previous action item by useCallback. We compare previous action item and current changed item then if it has been changed, update isActionLoading: true to false of Onyx.
@isabelastisser, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
FYI, I will be OOO from Dec 23 to Jan 6, so please reassign the issue to another team member for urgency, IF needed.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸