Skip to content
76 changes: 73 additions & 3 deletions src/libs/Middleware/HandleUnusedOptimisticID.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,58 @@ Onyx.connectWithoutView({
});

// Local cache of reportID to optimistic Onyx data
const reportOptimisticData = new Map<string, {settledPersonalDetails: OnyxEntry<PersonalDetailsList>; redundantParticipants: Record<number, null>} | undefined>();
const reportOptimisticData = new Map<
string,
{settledPersonalDetails: OnyxEntry<PersonalDetailsList>; redundantParticipants: Record<number, null>; missingLoginParticipants: number[]; invitedEmails: string[]} | undefined
>();

function isRecord(value: unknown): value is Record<string, unknown> {
return !!value && typeof value === 'object';
}

// Pairs the login invited via emailList with the one participant that has no login. Restores only the unambiguous 1:1 case.
function getRestoredPersonalDetails(
responseOnyxData: AnyOnyxUpdate[],
reportID: string,
redundantParticipants: Record<number, null>,
missingLoginParticipants: number[],
invitedEmails: string[],
): PersonalDetailsList {
const invitedEmail = invitedEmails.length === 1 ? invitedEmails.at(0) : undefined;
if (!invitedEmail) {
return {};
}

const settledParticipantIDs = new Set<string>();
const missingLoginDetailIDs = new Set<string>();
for (const update of responseOnyxData) {
if (update.key === `${ONYXKEYS.COLLECTION.REPORT}${reportID}` && isRecord(update.value) && isRecord(update.value.participants)) {
for (const accountID of Object.keys(update.value.participants)) {
settledParticipantIDs.add(accountID);
}
}
if (update.key === ONYXKEYS.PERSONAL_DETAILS_LIST && isRecord(update.value)) {
for (const [accountID, detail] of Object.entries(update.value)) {
// An explicit login in the response, even '', is respected; only an omitted key marks a candidate
if (!isRecord(detail) || 'login' in detail) {
continue;
}
missingLoginDetailIDs.add(accountID);
}
}
}

for (const accountID of missingLoginParticipants) {
settledParticipantIDs.add(String(accountID));
missingLoginDetailIDs.add(String(accountID));
}
const candidates = [...settledParticipantIDs].filter((accountID) => missingLoginDetailIDs.has(accountID) && !(accountID in redundantParticipants));
const settledAccountID = candidates.length === 1 ? Number(candidates.at(0)) : undefined;
if (!settledAccountID) {
return {};
}
return {[settledAccountID]: {accountID: settledAccountID, login: invitedEmail}};
}

/**
* This middleware checks for the presence of a field called preexistingReportID in the response.
Expand All @@ -57,7 +108,10 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe
// We're opening a new report, which can be a new or preexisting report
// For new report, clean up optimistic data after this request returned successfully
// For report redirect a preexisting report, clean up optimistic data after the request of preexisting report returned successfully
reportOptimisticData.set(currentRequestReportID, prepareOnyxDataForCleanUpOptimisticParticipants(currentRequestReportID, allPersonalDetails, currentUserAccountID));
const cleanupData = prepareOnyxDataForCleanUpOptimisticParticipants(currentRequestReportID, allPersonalDetails, currentUserAccountID);
const emailList = request.data?.emailList;
const invitedEmails = typeof emailList === 'string' ? emailList.split(',').filter(Boolean) : [];
reportOptimisticData.set(currentRequestReportID, cleanupData ? {...cleanupData, invitedEmails} : undefined);
}

const responseOnyxData = response?.onyxData ?? [];
Expand Down Expand Up @@ -102,7 +156,7 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe
}

if (!!currentRequestReportID && request?.command === WRITE_COMMANDS.OPEN_REPORT && !!response?.onyxData && reportOptimisticData.has(currentRequestReportID)) {
const {settledPersonalDetails, redundantParticipants} = reportOptimisticData.get(currentRequestReportID) ?? {};
const {settledPersonalDetails, redundantParticipants, missingLoginParticipants, invitedEmails} = reportOptimisticData.get(currentRequestReportID) ?? {};
reportOptimisticData.delete(currentRequestReportID);
if (!isEmptyObject(settledPersonalDetails) && !isEmptyObject(redundantParticipants)) {
(response.onyxData as AnyOnyxUpdate[]).push(
Expand All @@ -120,6 +174,22 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe
},
);
}

// The cleanup above deletes the only record carrying the typed login, so restore it onto the settled participant
const restoredPersonalDetails = getRestoredPersonalDetails(
response.onyxData as AnyOnyxUpdate[],
currentRequestReportID,
redundantParticipants ?? {},
missingLoginParticipants ?? [],
invitedEmails ?? [],
);
if (!isEmptyObject(restoredPersonalDetails)) {
(response.onyxData as AnyOnyxUpdate[]).push({
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: restoredPersonalDetails,
});
}
}
return response;
});
Expand Down
14 changes: 8 additions & 6 deletions src/libs/actions/Report/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2126,27 +2126,29 @@ function prepareOnyxDataForCleanUpOptimisticParticipants(
reportID: string,
personalDetails: OnyxEntry<PersonalDetailsList>,
currentUserAccountID: number | undefined,
): {settledPersonalDetails: OnyxEntry<PersonalDetailsList>; redundantParticipants: Record<number, null>} | undefined {
): {settledPersonalDetails: OnyxEntry<PersonalDetailsList>; redundantParticipants: Record<number, null>; missingLoginParticipants: number[]} | undefined {
const existingReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
if (!existingReport?.participants) {
return undefined;
}
const settledPersonalDetails: OnyxEntry<PersonalDetailsList> = {};
const redundantParticipants: Record<number, null> = {};
const missingLoginParticipants: number[] = [];
for (const accountID in existingReport.participants) {
// Never clean up the current user's own personal details. Removing them here (even momentarily) drops the
// current user's avatar down to the fallback avatar until the real details settle, which is what caused the
// avatar to flicker between the user initials and the default avatar. See https://github.com/Expensify/App/issues/95427
if (Number(accountID) === currentUserAccountID) {
continue;
}
if (!personalDetails?.[accountID]?.isOptimisticPersonalDetail) {
continue;
if (personalDetails?.[accountID]?.isOptimisticPersonalDetail) {
settledPersonalDetails[accountID] = null;
redundantParticipants[accountID] = null;
} else if (personalDetails?.[accountID] && !Object.hasOwn(personalDetails[accountID], 'login')) {
missingLoginParticipants.push(Number(accountID));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in Onyx the two are indistinguishable

This doesn't seem correct to me. They are distinguishable by checking whether the login key exists. You should use !Object.hasOwn(personalDetail, 'login') instead of !personalDetails[accountID].login.

The point is login: '' can be intentional, so we should not treat it as missing or delete it from responses. Please apply the same “missing key only” rule to the OpenPublicProfilePage handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yes that makes sense to me. thanks

}
settledPersonalDetails[accountID] = null;
redundantParticipants[accountID] = null;
}
return {settledPersonalDetails, redundantParticipants};
return {settledPersonalDetails, redundantParticipants, missingLoginParticipants};
}

/**
Expand Down
6 changes: 4 additions & 2 deletions tests/actions/ReportTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9137,10 +9137,11 @@ describe('actions/Report', () => {
CURRENT_USER_ACCOUNT_ID,
);

// Then only the optimistic participant is marked for clean up
// Then only the optimistic participant is marked for clean up, and the settled login-less participant is reported
expect(result).toEqual({
settledPersonalDetails: {[OPTIMISTIC_ACCOUNT_ID]: null},
redundantParticipants: {[OPTIMISTIC_ACCOUNT_ID]: null},
missingLoginParticipants: [SETTLED_ACCOUNT_ID],
});
});

Expand All @@ -9155,7 +9156,7 @@ describe('actions/Report', () => {

const result = Report.prepareOnyxDataForCleanUpOptimisticParticipants(REPORT_ID, {[SETTLED_ACCOUNT_ID]: {accountID: SETTLED_ACCOUNT_ID}}, CURRENT_USER_ACCOUNT_ID);

expect(result).toEqual({settledPersonalDetails: {}, redundantParticipants: {}});
expect(result).toEqual({settledPersonalDetails: {}, redundantParticipants: {}, missingLoginParticipants: [SETTLED_ACCOUNT_ID]});
});

it('returns undefined when the report has no participants', async () => {
Expand Down Expand Up @@ -9208,6 +9209,7 @@ describe('actions/Report', () => {
expect(result).toEqual({
settledPersonalDetails: {[OPTIMISTIC_ACCOUNT_ID]: null},
redundantParticipants: {[OPTIMISTIC_ACCOUNT_ID]: null},
missingLoginParticipants: [],
});
});
});
Expand Down
Loading
Loading