Remove onyx connect for the key: ONYXKEYS.PERSONAL_DETAILS_LIST in Parser lib - Part 1 - #92758
Conversation
Adds a new OnyxDerived value that builds a Record<accountID, name> from PERSONAL_DETAILS_LIST, replacing ad-hoc Onyx.connect calls that were scattered across modules. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Task title/description pages now pass the accountIDToNameMap derived value to htmlToMarkdown so user @mentions render as names instead of raw HTML in task fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tIDToName Covers mention resolution via accountIDToName map and the @hidden fallback when the accountID is absent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Switch to import type, and use a buildList helper to avoid numeric object literal keys that violate the naming-convention rule.
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
…l-details-parser
|
No new product considerations - removing my assignment and unsubscribing. |
|
@shubham1206agra Hi, could you take over this? |
|
@hoangzinh Please fix conflicts here |
| key: ONYXKEYS.DERIVED.ACCOUNT_ID_TO_NAME_MAP, | ||
| dependencies: [ONYXKEYS.PERSONAL_DETAILS_LIST], | ||
| compute: ([personalDetailsList]) => { | ||
| const accountIDToNameMap: Record<string, string> = {}; |
There was a problem hiding this comment.
This will not create a stable reference. Will cause performance issues. cc @TMisiukiewicz for confirmation.
There was a problem hiding this comment.
sorry @shubham1206agra. Can you share your suggestion here?
There was a problem hiding this comment.
@MelvinBot Do you have any suggestion for this?
There was a problem hiding this comment.
The concern is valid. compute rebuilds the entire map into a brand-new object on every change to PERSONAL_DETAILS_LIST. That key is high-traffic (presence, avatars, timezone, lastVisited, etc. all live on personal details), so any unrelated update produces a new top-level reference for ACCOUNT_ID_TO_NAME_MAP. Every consumer that does useOnyx(ONYXKEYS.DERIVED.ACCOUNT_ID_TO_NAME_MAP) — the Task pages here — then re-renders and re-runs Parser.htmlToMarkdown even when no name actually changed.
Suggested fix: use the compute context to do an incremental update instead of a full rebuild. compute already receives {currentValue, sourceValues} (types.ts:32), so you can update only the accountIDs that appear in sourceValues and return currentValue untouched when nothing name-relevant changed — keeping the reference stable across unrelated personal-details churn.
reportAttributes.ts is the reference pattern in the codebase: it uses hasKeyTriggeredCompute(ONYXKEYS.PERSONAL_DETAILS_LIST, sourceValues) and bails out early returning currentValue when there's no relevant change (reportAttributes.ts:233, :240).
Sketch
compute: ([personalDetailsList], {currentValue, sourceValues}) => {
const changed = sourceValues?.[ONYXKEYS.PERSONAL_DETAILS_LIST];
// First run / full rebuild when we have no prior value or no delta to work from.
if (!currentValue || !changed) {
const map: Record<string, string> = {};
for (const details of Object.values(personalDetailsList ?? {})) {
if (!details) {
continue;
}
map[details.accountID] = details.login ?? details.displayName ?? '';
}
return map;
}
// Incremental: only touch the accountIDs that actually changed.
const next = {...currentValue};
let dirty = false;
for (const details of Object.values(changed)) {
if (!details) {
continue;
}
const name = details.login ?? details.displayName ?? '';
if (next[details.accountID] !== name) {
next[details.accountID] = name;
dirty = true;
}
}
return dirty ? next : currentValue; // stable reference when nothing relevant changed
},One more thing worth noting: the sibling loginToAccountIDMap.ts (already merged) has the exact same full-rebuild pattern, so if you standardize on the incremental approach it's worth applying there too in a follow-up.
There was a problem hiding this comment.
@MelvinBot I was looking for ref stability, not compute perf per se.
There was a problem hiding this comment.
Introduce a new derived Onyx key
I know this is not stated anywhere (yet), but the overall tendency is to not introduce any new derived values as they can cause more harm than benefit and will have to be migrated away from either way.
There was a problem hiding this comment.
Yeah I'm not sure about moving forward with this PR if we are planning to deprecate derived values
There was a problem hiding this comment.
have to be migrated away from either way
@adhorodyski, do we have the alternative for derived values, or is it still being discussed?
There was a problem hiding this comment.
@adhorodyski should we temporarily accept this approach until we find an alternative solution for this?
There was a problem hiding this comment.
@hoangzinh Use provider for this approach. It was discussed here https://expensify.slack.com/archives/C05LX9D6E07/p1784815483529589
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-07-14.at.10.43.28.PM.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #66387 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Replace the ONYXKEYS.DERIVED.ACCOUNT_ID_TO_NAME_MAP derived value (added earlier on this branch) with a React context provider, following the review direction that new Onyx derived values are being deprecated. Derived values also can't hand consumers a stable reference: setDerivedValue writes with `skipCacheCheck: true`, so every PERSONAL_DETAILS_LIST write re-broadcasts a fresh map. The provider computes the map in a useMemo instead. - Add src/hooks/useAccountIDToNameMap.tsx and register the provider in AuthScreens ComposeProviders - Point the five Task pages at the new hook - Remove the DERIVED.ACCOUNT_ID_TO_NAME_MAP key, config, and type - Replace the derived-value unit test with a provider/hook test
14eef32 to
ff71cba
Compare
|
Tested again 
|
|
@shubham1206agra I updated to use the context provider approach |
There was a problem hiding this comment.
@hoangzinh Maybe use a selector at this point, and can you maybe get this benchmarked first?
There was a problem hiding this comment.
ah yes, I will look into this today
There was a problem hiding this comment.
@shubham1206agra I asked Claude and here is result:
Perf test script
import {AccountIDToNameMapContextProvider, useAccountIDToNameMap} from '@hooks/useAccountIDToNameMap';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetails, PersonalDetailsList} from '@src/types/onyx';
import React, {useMemo} from 'react';
import {View} from 'react-native';
// eslint-disable-next-line no-restricted-imports
import Onyx, {useOnyx} from 'react-native-onyx';
import {measureFunction, measureRenders} from 'reassure';
import createPersonalDetails from '../utils/collections/personalDetails';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';
import wrapOnyxWithWaitForBatchedUpdates from '../utils/wrapOnyxWithWaitForBatchedUpdates';
// Large-account sizing: TMisiukiewicz noted real customer accounts carry ~18-20k personal details.
const PD_COUNT = 20000;
// Fewer accounts for the render-count comparison - this measures re-render behaviour, not compute scale.
const PD_COUNT_RENDER = 2000;
const CHURN_COUNT = 10;
/** Candidate selector: the same map-building logic, run inside useOnyx's selector. */
function toAccountIDToNameMap(personalDetailsList: PersonalDetailsList | undefined): Record<string, string> {
const map: Record<string, string> = {};
for (const personalDetails of Object.values(personalDetailsList ?? {})) {
if (!personalDetails) {
continue;
}
map[personalDetails.accountID] = personalDetails.login ?? personalDetails.displayName ?? '';
}
return map;
}
function buildPersonalDetailsList(count: number): PersonalDetailsList {
const list: PersonalDetailsList = {};
for (let i = 1; i <= count; i++) {
list[String(i)] = createPersonalDetails(i);
}
return list;
}
// A monotonic counter guarantees every write sets a brand-new value (so no write is a deep-equal no-op).
let churnCounter = 0;
// Await after each merge so every write flushes as its own broadcast (otherwise sequential merges to
// this single key coalesce into one). This lets measureRenders observe the per-write re-render behaviour.
/** Merge a name-IRRELEVANT field (phoneNumber) so the accountID->name map content is unchanged. */
const writeUnrelatedChanges = async () => {
for (let i = 1; i <= CHURN_COUNT; i++) {
churnCounter += 1;
const patch: PersonalDetailsList = {[i]: {phoneNumber: `irrelevant-${churnCounter}`} as PersonalDetails};
// eslint-disable-next-line no-await-in-loop
await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, patch);
// eslint-disable-next-line no-await-in-loop
await waitForBatchedUpdates();
}
};
/** Merge a name-RELEVANT field (login) - control: every variant must re-render on these. */
const writeNameChanges = async () => {
for (let i = 1; i <= CHURN_COUNT; i++) {
churnCounter += 1;
const patch: PersonalDetailsList = {[i]: {login: `changed-${churnCounter}@example.com`} as PersonalDetails};
// eslint-disable-next-line no-await-in-loop
await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, patch);
// eslint-disable-next-line no-await-in-loop
await waitForBatchedUpdates();
}
};
beforeAll(() => Onyx.init({keys: ONYXKEYS}));
beforeEach(() => {
wrapOnyxWithWaitForBatchedUpdates(Onyx);
});
afterEach(() => Onyx.clear());
describe('useAccountIDToNameMap', () => {
describe('compute cost (measureFunction)', () => {
test(`builds the map from ${PD_COUNT} personal details`, async () => {
const personalDetailsList = buildPersonalDetailsList(PD_COUNT);
await measureFunction(() => toAccountIDToNameMap(personalDetailsList));
});
});
function SelectorConsumer() {
const [accountIDToName] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {selector: toAccountIDToNameMap});
return <View testID={String(Object.keys(accountIDToName ?? {}).length)} />;
}
function ProviderConsumer() {
const accountIDToName = useAccountIDToNameMap();
return <View testID={String(Object.keys(accountIDToName).length)} />;
}
function RawConsumer() {
const [personalDetailsList] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
const accountIDToName = useMemo(() => toAccountIDToNameMap(personalDetailsList), [personalDetailsList]);
return <View testID={String(Object.keys(accountIDToName).length)} />;
}
const providerTree = (
<AccountIDToNameMapContextProvider>
<ProviderConsumer />
</AccountIDToNameMapContextProvider>
);
const seed = async () => {
await Onyx.set(ONYXKEYS.PERSONAL_DETAILS_LIST, buildPersonalDetailsList(PD_COUNT_RENDER));
await waitForBatchedUpdates();
};
// Name-IRRELEVANT churn: the key changes but no name does. The selector's deep-equal cache should
// keep the consumer from re-rendering; the provider and raw approaches recompute a new reference and re-render.
describe(`render count on ${CHURN_COUNT} name-irrelevant PERSONAL_DETAILS_LIST writes`, () => {
test('selector: useOnyx(PERSONAL_DETAILS_LIST, {selector})', async () => {
await seed();
await measureRenders(<SelectorConsumer />, {scenario: writeUnrelatedChanges});
});
test('context provider: useAccountIDToNameMap()', async () => {
await seed();
await measureRenders(providerTree, {scenario: writeUnrelatedChanges});
});
test('raw: useOnyx(PERSONAL_DETAILS_LIST) + useMemo', async () => {
await seed();
await measureRenders(<RawConsumer />, {scenario: writeUnrelatedChanges});
});
});
// Name-RELEVANT churn (control): every variant must re-render since the map content changes.
describe(`render count on ${CHURN_COUNT} name-relevant PERSONAL_DETAILS_LIST writes (control)`, () => {
test('selector: useOnyx(PERSONAL_DETAILS_LIST, {selector})', async () => {
await seed();
await measureRenders(<SelectorConsumer />, {scenario: writeNameChanges});
});
test('raw: useOnyx(PERSONAL_DETAILS_LIST) + useMemo', async () => {
await seed();
await measureRenders(<RawConsumer />, {scenario: writeNameChanges});
});
});
});
it seems selector is winner
There was a problem hiding this comment.
To transform or reshape data without reducing its size — subscribe without a selector and transform inline instead
Ref: https://github.com/Expensify/App/blob/main/.claude/skills/coding-standards/rules/perf-11-optimize-data-selection.md#L22
Yeah, but it violates our rule of using selector in useOnyx hook. I asked Claude to do a perf test on Parser.htmlToMarkdown on both selector and raw inline (which is "transform inline" in our doc). And here is the result:
So "transform inline" is the genuine winner here. Should I go with "useOnyx with transform inline" instead?
…ountID-to-name map Switch AccountIDToNameMapContextProvider to a plain useOnyx + useMemo hook. Benchmarking showed neither the provider nor a useOnyx selector is worth it: - a selector returning this same-size map runs deepEqual over the whole map on every PERSONAL_DETAILS_LIST write (~2.2ms at 20k PDs) and violates PERF-11 - the context provider re-renders consumers on every PD write anyway - same as the plain hook - while adding an always-mounted global computation The hook reads PERSONAL_DETAILS_LIST and reshapes it in a useMemo (no selector). - Replace src/hooks/useAccountIDToNameMap.tsx (provider) with .ts (hook) - Remove the provider from AuthScreens ComposeProviders - Switch the five Task pages to the default-exported hook - Drop the provider wrapper from the hook unit test
|
|
|
Looks like we're awaiting your review, @youssef-lr |
|
🚧 youssef-lr has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.4.65-0 🚀
|
|
No help site changes are required for this PR. This is an internal refactor with no user-facing behavior change, so nothing under WhyThe PR replaces a module-level What changed:
What did not change:
Help site articles document product behavior and UI, not internal state-management patterns, so there is nothing here to document. @hoangzinh, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.65-3 🚀
Bundle Size Analysis (Sentry): |
9 participants
Explanation of Change
This is Part 1 of a series focused on removing Onyx Connect for the key ONYXKEYS.PERSONAL_DETAILS_LIST in the Parser library. In this pull request, we will:
Why a plain useOnyx + useMemo hook (not a selector or a context provider)? I benchmarked all three (Reassure, on a 20k-PD account) before deciding:
Selector — rejected. A
useOnyx(PERSONAL_DETAILS_LIST, {selector})returning this same-size map makes Onyx rundeepEqualover the whole map on everyPERSONAL_DETAILS_LISTwrite, related or not. That's~2.24 ms/writeat 20k PDs — versus the~0.02 msParser.htmlToMarkdown re-run it would save by skipping a re-render. It also violatesPERF-11("selector maps an entire collection → expensive deepEqual").Context provider — rejected. It gives no re-render benefit: its useMemo recomputes a new map reference on every PD write, so consumers re-render exactly as often as the plain hook (11 vs 11 in the benchmark). It only adds indirection plus an always-mounted global computation that runs even when no Task page is open.
Raw inline hook — chosen. No large-map deepEqual (cheap shallowEqual), PERF-11-compliant, and only computes while a Task page is mounted. The extra re-renders it "costs" are trivial (the Parser reparse is ~0.02 ms), far cheaper than the selector's per-write comparison.
See: #92758 (comment)
Fixed Issues
$ #66387
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
@<user_login> please complete the report by Friday.@<user_login> please complete the report by Friday.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Screen.Recording.2026-06-05.at.17.31.15.mov