[$500] Expense - Receipts in the expense preview do not update after adding/deleting receipt

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.16-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Precondition: There are more than 3 requests in the workspace chat and 3 of them have a receipt

1. Navigate to the workspace chat
2. Tap on expense preview
3. Tap on any request
4. Add a receipt to the request
5. Tap back button on the top left twice

Expected Result:

The receipts in the expense preview in the workspace chat will update and +1 counter will show

Actual Result:

The receipts in the expense preview in workspace chat does not update. It only shows +1 counter after refreshing the page

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/da6c2953-9e6a-4c52-8b94-7b09c3462651

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0198c10fd73498b28a
• Upwork Job ID: 1738598664125079552
• Last Price Increase: 2023-12-30
• Automatic offers:
• cubuspl42 | Reviewer | 28102438
• bernhardoj | Contributor | 28102439

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0198c10fd73498b28a

[melvin-bot[bot]]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External)

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The number of transactions with receipts on the report preview doesn't update.

What is the root cause of that problem?

The transaction with a receipt is calculated here. https://github.com/Expensify/App/blob/b74be723548d45d47dde7fe29aff5eafe16b290f/src/components/ReportActionItem/ReportPreview.js#L144

In getTransactionWithReceipts, we get all transactions that are linked with the passed IOU report id. https://github.com/Expensify/App/blob/b74be723548d45d47dde7fe29aff5eafe16b290f/src/libs/ReportUtils.ts#L1898-L1901

However, it won't rerender when the transaction collection is updated (added/removed) because we aren't subscribing the component with the transaction collection. Instead, we use a different technique here by using onyxSubscribe. https://github.com/Expensify/App/blob/b74be723548d45d47dde7fe29aff5eafe16b290f/src/components/ReportActionItem/ReportPreview.js#L209-L229

(here is the reason why we do this)

What changes do you think we should make in order to solve the problem?

If we want to keep the onyxSubscribe, then we should change transactionsWithReceipts to a state and update it inside onyxSubscribe.

const [transactionsWithReceipts, setTransactionsWithReceipts] = useState(ReportUtils.getTransactionsWithReceipts(props.iouReportID));

// inside onyxSubscribe
setTransactionsWithReceipts(ReportUtils.getTransactionsWithReceipts(props.iouReportID))

What alternative solutions did you explore? (Optional)

Otherwise, we can subscribe to the whole collection and use a selector to only select the transaction that is linked with the IOU report id. We will need to make a lot of changes to the code too, for example, we need to pass the transaction array to getTransactionsWithReceipts instead of the iou report id (we can technically keep it as it is).

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The receipts in the expense preview in workspace chat does not update. It only shows +1 counter after refreshing the page

What is the root cause of that problem?

We use transactionsWithReceipts to get the receipts here

Here's the detail implementation of getTransactionsWithReceipts, we get all transactions from onyx.connect

https://github.com/Expensify/App/blob/7b836cfcee79084177191e3dfb4a927698b09eb4/src/libs/ReportUtils.ts#L1898-L1901

In theory, when the transactions get changed, transactions from Onyx.connect will be updated first, and then re-render ReportPreview so when re-calculating getTransactionsWithReceipts, we can get the updated transactions

But in ReportPreview, we just subscribed ONYXKEYS.COLLECTION.TRANSACTION in onyxSubscribe, so when transactions get changed, we did not update transactionsWithReceipts

What changes do you think we should make in order to solve the problem?

We should cover all places that use data from Onyx.connect, we have iouSettled and transactionsWithReceipts

• For iouSettled:

we're getting report from allReports but it works well for now, because we passed iouReport to ReportPreview so this component is re-rendered when iouReport get changed

• For transactionsWithReceipts:

If we just create the new state for it and update in onyxSubscribe, we will break the advantage of onyxSubscribe, because transactionsWithReceipts is the array (not just boolean as hasMissingSmartscanFields, areAllRequestsBeingSmartScanned,...) and updating to new array will make the component re-render every times

To address this issue, we should create the new state for transactionsWithReceipts and just get the necessary properties from transactions. We just update transactionsWithReceipts in onyxSubscribe when it's really changed

const [transactionsWithReceipts, setTransactionsWithReceipts] = useState([]);
const transactionsWithReceiptsRef = useRef([])

...
 useEffect(() => { 
     const unsubscribeOnyxTransaction = onyxSubscribe({ 
         key: ONYXKEYS.COLLECTION.TRANSACTION, 
         waitForCollectionCallback: true, 
         callback: (allTransactions) => { 
            const newTransactionsWithReceipts = ReportUtils.getTransactionsWithReceipts(props.iouReportID).map(t=>({
receipt: t.receipt,
filename: t.filename,
pendingFields: t.pendingFields,
}))

     if(!_.isEqual(newTransactionsWithReceipts, transactionsWithReceiptsRef.current)){ // deep comparison
setTransactionsWithReceipts(newTransactionsWithReceipts)
transactionsWithReceiptsRef.current = newTransactionsWithReceipts
}

...

What alternative solutions did you explore? (Optional)

NA

[melvin-bot[bot]]

@cubuspl42, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

[cubuspl42]

@bernhardoj @tienifr Would you consider adding to your GitHub profile...

• an avatar, doesn't have to be an actual photo (helps to distinguish avatars for colorblind and partially colorblind people like me)
• preferred pronouns

[bernhardoj]

@cubuspl42 sorry to hear that, I have added a profile picture

[cubuspl42]

@tienifr

Can you use transactionsWithReceipts inside onyxSubscribe's callback anonymous function just like that?

The surrounding useEffect has empty deps ([]), there's no useCallback involved.

[tienifr]

@cubuspl42 Ah, I mean using transactionsWithReceiptsRef, we can use the previous transactionsWithReceipts through ref in callback function. I updated my proposal to use ref as well

[cubuspl42]

@tienifr

Now transactionsWithReceipts seems to be unused...

const [transactionsWithReceipts, setTransactionsWithReceipts] = useState([]);

Have you tested these changes? While I agree that it might be beneficial to give some focus to the performance here, the code must first and foremost work.

Would you share the branch with the code you used for testing that the proposed solution works as expected?

[tienifr]

@cubuspl42 Here you are: https://github.com/tienifr/App/tree/fix/33550

Evidence video:

https://github.com/Expensify/App/assets/113963320/c33662b8-96d0-4e52-85ad-1f015a17c6e8

[cubuspl42]

Thank you!

I'm still in two minds with this one, though... While it's good that the proposal tries to focus on providing good performance, the exact solution feels somewhat hacky.

We're using both a ref and a state property to store transactionsWithReceipts. I know that this pattern is sometimes used in the React world, but it feels at least workaroundish and I wouldn't apply it unless really necessary.

@bernhardoj

Do you know what's the performance characteristic of your solution? How often would setTransactionsWithReceipts be called? Only once per specific user interaction (like adding/deleting a receipt), or tens/hundreds/thousands of times per second?

[bernhardoj]

@cubuspl42 because we subscribe to the whole transaction, setTransactionsWithReceipts will be called whenever the collection is updated. It can be updated by:

1. Add/edit/delete a money request
2. Add/replace/remove a receipt
3. etc.

I guess we can do something like this to prevent the re-render if the array contents are the same.

setTransactionsWithReceipts((prevTransactions) => {
    const newTransactions = ReportUtils.getTransactionsWithReceipts(props.iouReportID);
    return _.isEqual(prevTransactions, newTransactions) ? prevTransactions : newTransactions;
})

Talking about the performance, the current usage of onyxSubscribe is not really performant. https://github.com/Expensify/App/blob/4a7014b980c6dc50cab671525ba017ceec5a8bdd/src/components/ReportActionItem/ReportPreview.js#L214-L226

Even though it doesn't cause a re-render, the four util functions make the same multiple loops to get all or linked transactions.

If we really want to improve the performance here, we can create like a wrapper for report preview like below. The 4 util functions above now only need to accept the transactions collection instead of the iou report ID.

function ReportPreviewWrapper(props) {
    const linkedTransactions = useMemo(() => Object.values(props.transactions).filter((transaction) => `${transaction.reportID}` === `${props.iouReportID}`), [props.transactions, props.iouReportID]);
    const transactionsWithReceipts = useMemo(() => linkedTransactions.filter((transaction) => TransactionUtils.hasReceipt(transaction)), [linkedTransactions]);
    const hasMissingSmartscanFields = ...
    const areAllRequestsBeingSmartScanned = ...
    const hasOnlyDistanceRequests = ...
    const hasNonReimbursableTransactions = ...

    return <ReportPreview {...props} transactionsWithReceipts={transactionsWithReceipts} ... />
}

export default withOnyx({
    transactions: {
        key: ONYXKEYS.COLLECTION.TRANSACTION,
    },
})(ReportPreviewWrapper);

And then memoized the ReportPreview.

memo(ReportPreview, (prevProps, nextProps) => _.isEqual(prevProps, nextProps))

Many things can be done to improve the performance, theoretically (because I didn't do any measurement).

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@cubuspl42, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@cubuspl42, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[cubuspl42]

I decided that we should go with the proposal by @bernhardoj.

Thank you for raising the performance concerns, @tienifr. Unfortunately, the provided solution itself has issues, in my opinion, and I believe we should prioritize the simplicity here.

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

Triggered auto assignment to @puneetlath, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tienifr]

@cubuspl42 We still use ref and state in our App like useSingleExecution, ... So I think it's the norm. I believe the performance issue is critical in this component because the transaction collection is so big and the small change can cause the component re-render unexpectedly. As I mentioned above, we not only compare the previous transactionsWithReceipts with the new one, but also restrict it with the necessary fields

[cubuspl42]

@tienifr I hear you.

Still, this is not an issue related to the performance improvement project.

We do subscribe to the transaction collection inside each ReportPreview and it's beyond the scope of this issue to change that.

Every change to the transaction collection will trigger transaction-processing computations inside each ReportPreview, and it's beyond the scope of this issue to change that.

We do trigger React state setters inside the Onyx callback, so React will be forced to perform diffing in each ReportPreview instance.

You're right, though, that after our changes, the triggered JS computations would be bigger, and the render would propagate.

I think that a relatively low-hanging fruit here is a trick...

We could split ReportPreview into two components. An outer one which would exist for performance optimization reasons, and the inner one with the actual, more complex logic.

Inside the outer one, we could destructure the recent transactionsWithReceipts.

const [recentTransaction1, recentTransaction2, recentTransaction3]  = transactionsWithReceipts.slice(-3);

And we could cut the re-render based on the identity of these recent transactions. As we know, React prefers primitives and identity-stable objects when it comes to preventing re-renders.

We could experiment with this during the PR phase.

I could suggest a split compensation if the BugZero team agrees:

• $375 for @bernhardoj for his original proposal and implementation efforts
• $250 for @tienifr for investigating the performance issues and raising valid concerns
• $375 for the C+ review (me)

[Christinadobrzyn]

@puneetlath could you give your thoughts on this decision? https://github.com/Expensify/App/issues/33550#issuecomment-1873764619 Thank you!

[cubuspl42]

I think we've done things like this in the past when a solution included a non-trivial part of a non-selected proposal. The issue budget stays the same. But if this is not fine by the process, we'll have to live with it.

[melvin-bot[bot]]

@puneetlath @cubuspl42 @Christinadobrzyn 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!

[Christinadobrzyn]

I'm going to be ooo next week so assigning a buddy to monitor this.

status- I think we're reviewing this - https://github.com/Expensify/App/issues/33550#issuecomment-1873764619

[melvin-bot[bot]]

Current assignee @puneetlath is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot[bot]]

@puneetlath, @cubuspl42, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[puneetlath]

The proposal and the proposed compensation sounds good to me. Good by you @bernhardoj? If so, I'll assign you.

[bernhardoj]

@puneetlath I'm good with that

[melvin-bot[bot]]

@puneetlath @cubuspl42 @Christinadobrzyn this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot[bot]]

📣 @cubuspl42 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[puneetlath]

Assigning @bernhardoj. Agreed payment breakdown here: https://github.com/Expensify/App/issues/33550#issuecomment-1873818960

[bernhardoj]

Actually, I wasn't able to reproduce this anymore because in this PR, we connect to the whole transactions collection 😅

[cubuspl42]

Oh my, so this was one of those that you can wait out...

[puneetlath]

Ah, wow ok. So sounds like nothing to do here and we should just close the issue out?

Thanks for all the discussion though!

[melvin-bot[bot]]

@puneetlath @cubuspl42 @bernhardoj @Christinadobrzyn this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @cubuspl42 is eligible for the Internal assigner, not assigning anyone new.

[cubuspl42]

Internal Slack thread

[Christinadobrzyn]

@cubuspl42 can you let us know what you think is fair for the partial payment?

[Christinadobrzyn]

Based on this Slack convo - https://expensify.slack.com/archives/C02NK2DQWUX/p1705917174041169

I paid out @cubuspl42 $125 (25% of $500) for the job. The payment is through Upwork.

Feel free to reach out with any questions!