Decompose ReportActionsList: 6#93694
Conversation
LukasMod
changed the title
LukasMod
force-pushed
the
decompose/ral-6
branch
from
be8dc63 to
846c47a
Compare
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
LukasMod
force-pushed
the
decompose/ral-6
branch
from
38389bf to
614d282
Compare
LukasMod
changed the title
LukasMod
changed the title
Add src/pages/inbox/ReportActions.tsx as the route-only orchestrator that owns the coarse branching (skeleton / money-request table view / chat list) and subscribes to only what those branches need. ReportScreen now mounts it instead of the old 58-LOC inbox/ReportActionsList.tsx wrapper, which is removed. ReportActionsView stays for now (still used by MoneyRequestReportView) and is deleted in the next commit. The app-load skeleton is hoisted out of the hook-driven body into the orchestrator so the body's data hooks/effects never run during app boot. It is evaluated on the chat path only (after the money-request branch) to preserve the prior behavior, where that skeleton lived inside the chat-only ReportActionsView, and it owns the warm:false markOpenReportEnd telemetry for the branch it gates. Add tests/ui/ReportActionsTest.tsx covering the orchestrator's branching decisions and the app-load telemetry mark.
LukasMod
force-pushed
the
decompose/ral-6
branch
from
614d282 to
5ec39cb
Compare
|
Regardless failing perf test, check PR description section about it. I compared sending messages with profiler and spans and didn't spot difference in performance (only noise) 
|
melvin-bot
Bot
requested review from
DylanDylann and
heyjennahay
and removed request for
a team
|
@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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ec39cb02e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e93921114a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <> | ||
| <ReportActionsList | ||
| key={report.reportID} | ||
| reportID={report.reportID} |
There was a problem hiding this comment.
When switching between reports, the Onyx subscription for the new key can temporarily expose the previous report while the new value is loading (the repo already guards this state in useIsReportReadyToDisplay by comparing report.reportID with the route ID). Passing report.reportID here makes the chat list subscribe to and render the old report during that transition; the previous ReportActionsView was invoked with the route ID, so it could stay on the requested report key and show a skeleton instead of flashing the wrong chat. Please pass/guard against reportIDFromRoute here so report transitions don't attach the list to stale report data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Subscription returns either the new report or undefined, never the previous one and we keep relay on report around instead. I will keep it as it is
|
@DylanDylann Could you take a look on this? |
|
Sure, I will complete my review this week |
2 participants
Explanation of Change
Splits the old
ReportActionsViewinto two clear pieces: a thin route-only orchestrator that decides what to render, and a hook-driven body that renders the chat list. No user-facing behavior changes — this is a structural refactor.Changes
src/pages/inbox/ReportActions.tsx— a route-only orchestrator. It owns the coarse branching: the skeleton states, the money-request-vs-chat decision, mountingUserTypingEventListener, and the app-load open-report telemetry mark (markOpenReportEnd(report, {warm: false})).src/pages/inbox/report/ReportActionsList.tsx(the body) is now hook-driven — its props shrink to{reportID, onLayout?}and it self-fetches via the PRs 2–5 hooks (pagination, visibility, unread markers, scroll, concierge) instead of receiving ~18 props.ReportScreennow mounts<ReportActions/>instead of the old wrapper.MoneyRequestReportViewmounts the body (ReportActionsList) directly and mountsUserTypingEventListeneritself — it intentionally does not route through the orchestrator (MRR branching stays put, per the blueprint).ReportActionsView.tsx, the old 58-LOCinbox/ReportActionsList.tsxwrapper, andgetReportActionsListInitialNumToRender.ts(+ its test).Unit Tests
tests/ui/ReportActionsTest.tsx— orchestrator: skeleton states, money-request-vs-chat branch, typing mount, app-loadwarm: falsetelemetry.tests/ui/ReportActionsListTest.tsx— body: concierge / fine-grained skeleton behavior (mounts the body directly with a seeded Onyx store).tests/ui/MoneyRequestReportViewTest.tsx— the second consumer mounts the body + typing listener (chat path) and the table view (money-request path).tests/perf-test/ReportActionsList.perf-test.tsx— rewritten against the new body surface (see below).Perf — re-baselined (not a regression)
The perf test was rewritten to match the new hook-driven body. The render number went 17.7 ms → 37 ms, and render from 3->6 but this is a re-baseline, not a regression — the two harnesses measure different things.
getSortedReportActionsForDisplay(sorting 500 actions),getContinuousChain(pagination), anduseReportActionsVisibility(the concierge/session/visible filter). The component was handed finished data and just rendered it.REPORT_ACTIONSinto Onyx, so the hook-driven body runs that entire pipeline on mount — exactly what production does on every report open. The extra ~19 ms is the cost of the sort + paginate + filter that the old test hid, plus the per-hookuseOnyxsubscriptions the decomposition introduced.Production always paid that ~37 ms; the old test just never measured it. The old 17.7 ms figure is discarded — it's not comparable. To be able to see [render] times in perf tests I applied this config fix, as now it seems to be broken on main: #93573
Fixed Issues
$ #89767
PROPOSAL:
Tests
Test 1: Open a regular chat
Test 2: Deep-link to an old message
Test 3: Open an expense report with multiple transactions
MoneyRequestReportActionsList), not the chat list.Test 4: Open an expense report that renders chat-style
Test 5: Open a single expense / transaction-thread report
Test 6: Typing indicator in a 1:1 chat
Test 7: Typing indicator in a transaction-thread report
Test 8: Open the Concierge DM
Test 9: Open an unread report
Test 10: Scroll up to load older messages, then back down
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: Native
andoird.native.mov
Android: mWeb Chrome
android.chrome.mov
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.safari.mov
MacOS: Chrome / Safari
web1.mov
web2.mov