Clean up draft message subscriptions

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1724334129739879

I'm creating this issue to perform some code cleanup / likely performance improvement in some cases.

• ReportActionItem subscribes to the full collection of all draft reportActions across all reports
• on every render, we extract just the reportActionDraft for the single report

Let's simplify this and make it more efficient by having ReportActionItem only subscribe to the draft message for the given action (if present):

const originalReportID = useMemo(() => ReportUtils.getOriginalReportID(report.reportID, action) ?? '-1', [report.reportID, action]);
const [draftMessage] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`, {selector: (draftMessagesForReport) => {
    const matchingDraftMessage = draftMessagesForReport?.[action.reportActionID];
    return typeof matchingDraftMessage === 'string' ? matchingDraftMessage : matchingDraftMessage?.message;
}});

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @hungvu193

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @abekkala (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.

[cretadn22]

Proposal

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

Clean up draft message subscriptions

What is the root cause of that problem?

New refactor

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

We need to remove

App/src/pages/home/report/ReportActionItem.tsx

Line 189 in be7cb7e

const draftMessage = useMemo(() => getDraftMessage(reportActionDrafts, report.reportID, action), [action, report.reportID, reportActionDrafts]);

and update to

    const originalReportID = useMemo(() => ReportUtils.getOriginalReportID(report.reportID, action) ?? '-1', [report.reportID, action]);
    const [draftMessage] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`, {selector: (draftMessagesForReport) => {
        const matchingDraftMessage = draftMessagesForReport?.[action.reportActionID];
        return typeof matchingDraftMessage === 'string' ? matchingDraftMessage : matchingDraftMessage?.message;
    }});

As recommended by @roryabraham, I gave it a try and it worked perfectly.

What alternative solutions did you explore? (Optional)

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2023-10-02T00:00:00Z.

Proposal

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

Clean up draft message subscriptions

What is the root cause of that problem?

This is an improvement

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

I think we can create a new hook like useReportActionsDraft with two params reportID and reportActionID then in this hook we will use useOnyx with reportID to subscribe the draftMessage of the report and then return the draftMessage of the report action that has the ID is reportActionID param. With this hook we can use this in other places that we want and no need to get the draft by selector every time we use it.

This hook will be like this:

function useReportActionDraft(reportID?: string, reportActionID?: string) {
    const [draftMessageReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID || -1}`);
    const draftMessage = draftMessageReport?.[reportActionID || '-1'];
    return typeof draftMessage === 'string' ? draftMessage : draftMessage?.message;
}

export default useReportActionDraft;

Then for example we can use this in ReportActionItem

const originalReportID = ReportUtils.getOriginalReportID(report.reportID, action);
const draftMessage = useReportActionDraft(originalReportID, action?.reportActionID);

App/src/pages/home/report/ReportActionItem.tsx

Line 188 in be7cb7e

const reportActionDrafts = useReportActionsDrafts();

What alternative solutions did you explore? (Optional)

NA

[nkdengineer]

Updated proposal

• Update example code for hook.

[roryabraham]

I think @cretadn22's proposal is good enough. Don't think we need a custom hook for this one (not sure if we'll use it anywhere else)

[roryabraham]

@cretadn22 let us know when we can expect to have a PR ready for review

[cretadn22]

PR is ready now

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[roryabraham]

The PR for this did cause a regression that was fixed in #48096

[abekkala]

@roryabraham the PR above was never deployed to production.
As the PR caused regression (which was fixed)

Do I need to wait for PR 47866 to go to prod? Or do I submit payment at 50%?

[roryabraham]

Sorry for the confusion since it never got its Deployed to production comment, but I can confirm that #47866 has been on prod since Aug 28th. It caused a deploy blocker which was fixed on staging in #48096, which was deployed to production in the same release.

So the next step here is to submit payment at 50%

[abekkala]

PAYMENT SUMMARY:

• Fix: @cretadn22 [50% due to regression] $125 Offer
• PR Review: @hungvu193 [50% due to regression] $125 Payment via NewDot

[cretadn22]

@abekkala Accepted

[abekkala]

@cretadn22 payment sent and contract ended - thank you! 🎉

[abekkala]

@hungvu193 you can request payment via newdot

[garrettmknight]

$125 approved for @hungvu193