Repository navigation
Part 1 - Remove onyx connect for the key: ONYXKEYS.COLLECTION.REPORT in Parser lib - #100121
Merged
inimaga merged 8 commits intoOct 2, 2026
Merged
Conversation
|
\o/ thanks for letting me know |
hoangzinh
force-pushed
the
66386-remove-onyx-connect-report-pr1
branch
from
September 17, 2026 17:03
86d67fe to
25c0fb7
Compare
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
- Create useReportIDToNameMap hook mirroring useAccountIDToNameMap - Add Parser and hook unit tests for reportIDToName
hoangzinh
force-pushed
the
66386-remove-onyx-connect-report-pr1
branch
from
September 23, 2026 13:03
25c0fb7 to
1ec31d3
Compare
Part of removing the module-level Onyx.connect for COLLECTION.REPORT in src/libs/Parser.ts. Only HTML that reaches the client from the server carries the `<mention-report reportID="..."/>` form that reportIDToName resolves; the client's own markdown-to-HTML rule emits `<mention-report>#room</mention-report>`, which round-trips without the map. Report action comment HTML is the field confirmed to carry the reportID form, so its six call sites now pass the map explicitly rather than relying on the module-level fallback. Components and hooks read the map from useReportIDToNameMap. Module-scope helpers and the action layer take it as a parameter instead: getClipboardText, setClipboardMessage and setClipboardMessageWithCleanedMentions gain an optional argument so the many callers that copy translated strings stay untouched, and editReportComment receives it from useEditMessage so the unchanged-message guard resolves mentions the same way the draft was seeded. Seeding the draft and diffing against it must use the same map, or an untouched save persists "#Hidden" over the original mention. Behavior is unchanged while Parser still falls back to the module-level map. Refs: Expensify#66386
Two failures introduced in 25c0fb7 that block CI. ParserTest used a literal computed key (`{['42']: ...}`), which trips no-useless-computed-key; writing the quoted form instead trips the numeric-key naming rule. Assign after construction, matching the style the accountIDToName tests in the same file already use. useReportIDToNameMapTest passed a record of template-literal keys to Onyx.multiSet, whose `Partial<OnyxInputKeyValueMapping>` parameter rejects the resulting `[x: string]` index signature. Use one Onyx.set per report: the keys narrow correctly and the null entry still exercises the skip path.
hoangzinh
changed the title
hoangzinh
marked this pull request as ready for review
September 23, 2026 14:41
melvin-bot
Bot
requested review from
dylanexpensify and
shubham1206agra
and removed request for
a team
September 23, 2026 14:41
|
@shubham1206agra 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
CONSISTENCY-15: spell out the reportID-to-name relationship in the doc comments instead of using an arrow. CLEAN-REACT-PATTERNS-0: drop the manual useMemo. The file compiles with React Compiler under both Babel and OXC (verified with check-compiler.sh), so the derived map is memoized automatically.
Contributor
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-09-24.at.5.15.30.PM.mov |
shubham1206agra
approved these changes
Sep 24, 2026
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #66386 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
dylanexpensify
left a comment
dylanexpensify
left a comment
Contributor
There was a problem hiding this comment.
Product input not required.
inimaga
requested changes
Sep 25, 2026
| @@ -42,6 +44,8 @@ const insertAtCaret = (target: HTMLElement, insertedText: string, maxLength: num | |||
| }; | |||
|
|
|||
| const useHtmlPaste: UseHtmlPaste = (textInputRef, preHtmlPasteCallback, isActive = false, maxLength = CONST.MAX_COMMENT_LENGTH + 1) => { | |||
| const reportIDToName = useReportIDToNameMap(); | |||
Contributor
There was a problem hiding this comment.
Can we avoid subscribing to reports here when HTML paste is disabled? This hook also runs for regular text fields, so updates to any report now make each field rebuild the full report-name map, even though it never uses it. That adds unnecessary work for accounts with lots of reports.
Contributor
Author
There was a problem hiding this comment.
Good catch @inimaga . There are 23 type="markdown" inputs, so the hook does need the map for those, but every other text field in the app is paying for nothing.
Digging into where the cost actually is: the subscription itself is cheap — Onyx keeps one frozen collection snapshot per key (OnyxCache.js) and hands the same reference to every subscriber (OnyxUtils.js), so the reports aren't duplicated. What duplicates is the derived map: on a 20k-report fixture it's ~3.3 ms to rebuild and ~176 KB retained, per mounted input, on every report write.
So rather than gating the subscription, I'd move the build into the paste callback and resolve only handlePastedHTML is triggered:
const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
const handlePastedHTML = useCallback(
(html: string) => {
paste(Parser.htmlToMarkdown(html, {reportIDToName: buildReportIDToNameMap(reports)}));
},
[paste, reports],
);It runs once per paste instead of once per report write.
Being upfront about what that doesn't solve: the hook still subscribes, so regular text fields still re-render on report writes — just without the rebuild. If you'd rather they hold no subscription at all, I can additionally move the useHtmlPaste call into a small child rendered only when isMarkdownEnabled (conditional render instead of conditional hook).
// in BaseTextInput/implementation/index.tsx, replacing the direct useHtmlPaste call
{isMarkdownEnabled && <HtmlPasteHandler inputRef={input} maxLength={maxLength} />}
function HtmlPasteHandler({inputRef, maxLength}: HtmlPasteHandlerProps) {
const reportIDToName = useReportIDToNameMap();
useHtmlPaste(inputRef, undefined, true, maxLength, reportIDToName);
return null;
}Happy to do any option; let me know what your preferences are.
Contributor
There was a problem hiding this comment.
Thanks for digging into this. Let’s use the small child component so regular text fields don’t subscribe to reports at all. Building the map only when HTML is pasted also makes sense. That avoids both the unused subscriptions and repeated map builds. The child should call the revised useHtmlPaste directly, without calling useReportIDToNameMap.
Contributor
Author
…HTML Addresses review feedback: useHtmlPaste is shared by every TextInput, so reading the report collection in the hook body made plain text fields subscribe to reports and rebuild the name map on every report write, even though a non-markdown input never registers the paste listener at all. Two changes: - Build the map inside handlePastedHTML rather than in render, so it costs nothing until a paste happens. buildReportIDToNameMap is exported from useReportIDToNameMap for this; the hook itself is unchanged. - Move the useHtmlPaste call into a small HtmlPasteHandler child that BaseTextInput renders only when the input is markdown. A hook cannot be called conditionally, but a component can be rendered conditionally, so plain text fields now mount nothing and hold no subscription. Applied to both platform files even though useHtmlPaste is a no-op on native, to keep the two implementations parallel.
Contributor
|
Conflicts 😬 |
Neither behaviour was covered: the existing useHtmlPaste suite only exercises handlePastePlainText, so every test passed both before and after the paste path started resolving report mentions, and would still pass if the map were dropped. Adds two guards, each verified to fail when its change is reverted: - useHtmlPasteTest gains a handlePastedHTML block that dispatches the text/html clipboard flavour carrying <mention-report reportID>, asserting it lands as the room name, plus the #Hidden case for a report absent from Onyx. Removing the map turns the first into "Hey #Hidden team". - BaseTextInputImplementationTest asserts the paste hook mounts for type="markdown" and not for a plain input, guarding the report subscription against leaking back into every text field in the app. The second is a new file because BaseTextInputTest mocks BaseTextInput/implementation away to inspect forwarded props, so anything needing the real implementation rendered cannot live there.
# Conflicts: # src/components/TextInput/BaseTextInput/implementation/index.tsx
hoangzinh
force-pushed
the
66386-remove-onyx-connect-report-pr1
branch
from
September 30, 2026 23:32
138be75 to
6e75639
Compare
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Main added two member-work-arrangement cases to ContextMenuActionsCopyMessageTest that assert the single-argument getClipboardText. setClipboardMessage now always forwards a second argument, so these go the same way as the other translated-string branches: the explicit `undefined` records that those paths deliberately carry no report name map.
Contributor
Author
@inimaga I resolved conflicts 😬 |
inimaga
approved these changes
Oct 2, 2026
Contributor
|
Thanks! LGTM |
Contributor
|
🚧 inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
59 tasks done
Contributor
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.5.2-0 🚀
|
Contributor
|
No help site update is needed. This PR is an internal refactor: it replaces |
Contributor
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This is Part 1 of a series focused on removing
Onyx.connectfor the keyONYXKEYS.COLLECTION.REPORTin the Parser library. In this pull request, we will:useReportIDToNameMapWhy a plain
useOnyx+useMemohook (not a selector or a context provider)? We benchmarked all three (Reassure, on a 20k-report account) before deciding:Selector — rejected. A
useOnyx(COLLECTION.REPORT, {selector})returning a same-size map makes Onyx rundeepEqualover the entire report collection on everyCOLLECTION.REPORTwrite, related or not. That's~7.70 ms/writeat 20k reports (~1.44 msfor deepEqual alone +~5.85 msfor map rebuild) — versus the~0.03 msParser.htmlToMarkdownre-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
useMemorecomputes a new map reference on every report write, so consumers re-render exactly as often as the plain hook. It only adds indirection plus an always-mounted global computation that runs even when no consumer is open.Raw inline hook — chosen. No large-map
deepEqual(cheapshallowEqual), PERF-11-compliant, and only computes while a consumer component is mounted. The extra re-renders it "costs" are trivial (the Parser reparse is~0.03 ms), far cheaper than the selector's per-write comparison.What's next: validate group B/C/D here
Fixed Issues
$ #66386
PROPOSAL: #66386
Tests
#general.Hey #general team.#generalrenders as a room-mention link.getClipboardText): right-click the message -> Copy to clipboard, then paste into something outside the app — a plain text editor, or the browser address bar. Verify it readsHey #general teamand notHey #Hidden teamuseHtmlPaste): with the same message still on the clipboard, paste into the app's main composer. Verify#generalagain.Hey #general team, not#Hidden. Press Escape.Hey #general team.(edited)label appears, the message is unchanged.please, and save. Verify the message becomesHey #general team pleasewith#generalstill rendering as a mention link.Offline tests
Same as tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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-09-24.at.00.36.05.mov