Decompose ReportActionsList: 12#94938
Conversation
|
@codex review |
![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: 6a9cb88ad0
ℹ️ 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".
e9315ee to
f40a3fa
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
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.
|
f40a3fa to
bfd4898
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
bfd4898 to
e7465c4
Compare
|
@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] |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@DylanDylann I applied your suggestions, thanks! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
🤖 I couldn't run the manual tests on Android in this run — the Android testing environment isn't provisioned here. Driving the app on Android requires a booted emulator with the standalone NewDot dev build installed, Metro running on To get these run, this needs to go through the dedicated Android testing workflow (the one that boots the emulator and installs the dev build) rather than the standard MelvinBot path. Once that environment is up, the four flows to verify are:
cc |
|
Oh my bad, I added Android video |
This comment was marked as resolved.
This comment was marked as resolved.
|
Will take a look first thing tomorrow, sorry. |
|
Conflicts resolved, updated to latest main |
|
Working on failing test |
|
@rlinoz ready again |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.4.33-0 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against Expensify's help site content under This is an internal engineering refactor with no user-facing behavior change:
No features, tab names, settings labels, buttons, pricing, or workflows described on the help site are added, removed, or altered by these changes, so there is nothing to document. No draft docs PR was created. @LukasMod, flagging for your awareness — no linked help site PR was needed here. If you believe a user-facing behavior did change and warrants a docs update, let me know the specific behavior and I'll draft one. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.33-8 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
Before this PR the report-actions list ref was owned by context.
ActionListContextexposed a sharedflatListRef: RefObject, the scroll manager dereferencedflatListRef.current, and each list attachedthat context ref via
ref={}. Attaching a context-sourced ref toref={}is a render-time ref read, soReact Compiler bailed out of optimizing the list
This PR flips ownership. Each list now owns its ref locally and only publishes it to context.
ActionListContextno longer exposes a shared ref. It exposes two callbacks instead:registerListRef(ref), where a list publishes its locally-owned ref on mount and clears it withnullon unmount.getListRef(), where scroll handlers resolve the ref at call time, never during render.const listRef = useRef<FlatList>(null), attachesref={listRef}(a local ref thecompiler can reason about), and registers it in a layout effect so the ref is live at commit, before the
list's
onLayoutfires and reads it viagetListRef().Context (ownership flip)
src/pages/inbox/ActionListContext.tsxis new. It owns the context, theActionListContextProvider(value built inline from refs and accessors), and the
useActionListContext()consumer hook. The value isonly stable refs and accessors and triggers no re-renders, so it stays a single provider. That's documented
inline with an
eslint-disable rulesdir/context-provider-split-values.src/pages/inbox/ReactionListContext.tsis renamed fromReportScreenContext.ts. The old file mixed theaction-list context with the reaction-list context. It now holds only the reaction-list context and types.
The action-list half moved to
ActionListContext.tsx.src/hooks/useActionListContextValue.tsis deleted. The provider owns its value inline now, so the separatevalue-builder hook is gone, and so is the confusing
useActionListContext/useActionListContextValuename pair.
Scroll manager (resolve ref at call time)
In
src/hooks/useReportScrollManager/index.tsandindex.native.ts, everyscrollTo*now callsconst listRef = getListRef()and guardslistRef?.currentat call time. Thereffield is dropped fromthe manager's return.
In
src/hooks/useReportScrollManager/types.ts,refis removed andscrollToIndexis unified to a singlesignature,
scrollToIndex(index, {isEditing?, animated?}). That replaces the oldscrollToIndex(index, bool)plus a separate
scrollToIndexInstance. Platform defaults foranimatedare preserved: web animates andnative jumps instantly, which matches FlashList's default and the prior native behavior.
Threshold state (drop the render-time ref read)
ReportActionsListderiveshasScrolledOverThreshold— "is the user scrolled more thanACTION_VISIBLE_THRESHOLD(250px) away from the bottom of the inverted list" — and seeds it from thepersisted scroll offset so a remount restores the correct value on the first render. That initial
read used
scrollOffsetRef.currentinside theuseStateinitializer, which is a render-time ref read:React Compiler bailed on it and
react-hooks/refsflagged it ("Cannot access refs during render").ActionListContextnow exposesgetScrollOffset(): number, a snapshot accessor built fromscrollOffsetRef, alongside the raw ref.ReportActionsListinitializes the state fromuseState(() => getScrollOffset() >= CONST.REPORT.ACTIONS.ACTION_VISIBLE_THRESHOLD)and no longerdestructures
scrollOffsetRefat all. Reading through an opaque accessor keeps the render path free of atraceable ref read, so the compiler no longer bails and the lint warning is gone.
Behavior is unchanged: the accessor returns the same value the ref held, still read once synchronously in
the lazy initializer. It is deliberately not moved to an effect — the value must be correct on the
first render because
useMarkAsReadreadsisScrolledToEnd(!hasScrolledOverThreshold) in a mounteffect, and a first-render
falsewould let a scrolled-up report get marked read.Type extraction (fix layering)
src/components/FlashList/types.tsis new.FlatListRefTypelives next to FlashList now instead of beingimported out of a page-level context module, and
InvertedFlashListimports it from here.Fixed Issues
$ #89773
PROPOSAL:
Tests
1. Scroll-to-bottom (floating "new messages" counter)
2. New incoming message auto-scroll (live tail)
3. Deep-link to an old message, then a new message
4. List-switch ref hand-off (timing)
brief unmount→mount window.
Offline tests
QA Steps
Same as tests
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
android.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari
web.mov