Skip to content

[HOLD] decompose ReportActionsList#87506

Draft
adhorodyski wants to merge 35 commits intoExpensify:mainfrom
callstack-internal:decompose/report-actions-list
Draft

[HOLD] decompose ReportActionsList#87506
adhorodyski wants to merge 35 commits intoExpensify:mainfrom
callstack-internal:decompose/report-actions-list

Conversation

@adhorodyski
Copy link
Copy Markdown
Contributor

@adhorodyski adhorodyski commented Apr 9, 2026

Explanation of Change

PR 3 of the ReportActionsList decomposition series (#84895).

Decomposes the monolithic ReportActionsList into a clean, hook-driven pipeline and completes the InvertedFlashList migration using built-in APIs rather than workarounds. Net: ~−600 lines across the list surface, one new hack removed from scope, dozens of pre-migration hacks gone.

The pipeline

ReportActionsList.tsx is now a thin orchestrator that composes seven focused hooks, each with a single responsibility:

Hook Responsibility
useReportActionsPagination Paginated fetch + optimistic CREATED + transaction thread stitching
useTransactionThread Derive transaction thread report and its actions
useLoadReportActions loadOlderChats / loadNewerChats imperative API for the list
useReportActionsVisibility Concierge filtering, "show full history" toggle, visibility rules
useUnreadMarker Compute marker position, subscribe to read events, track unreadMarkerTime
useMarkAsRead Report-change and visibility-change read triggers
useReportActionsScroll All scroll state, Pusher new-action handler, scroll imperatives

Plus two extracted presentation components: ReportActionsListHeader, ShowPreviousMessagesButton.

Hacks removed vs. main

Hack Why it was there How it's gone now
StaticReportActionsPreview hidden-render FlatList initialScrollIndex broken on inverted lists (RN #56237, #54409, #41163) — mount items hidden, scrollToEnd, reveal Replaced by maintainVisibleContentPosition={{ startRenderingFromBottom: true }} — a first-class FlashList API that mounts at the last index and renders items from there first
Imperative scrollToEnd on mount Paired with the hide-render workaround No longer needed — declarative mount position
200ms LIST_SCROLLING_DEBOUNCE_TIME timer Mask race between "scroll started" and "scroll ended" on reveal (web has no onMomentumScrollEnd) Gone with the hide-render mechanism
shouldHideContent, shouldDisableVisibleContentPosition, flex0 style conditional, custom showsVerticalScrollIndicator toggle FlatList-specific props to support the hide-render All obsolete — removed
shouldEnableAutoScrollToTopThreshold={!hasNewerActions} Prop we'd added to work around mount jitter Upstream removed the prop; our simplification died with it
getInitialNumReportActionsToRender + 3-branch initialNumToRender logic "Web needs ≥50 to avoid onStartReached spam" (FlatList maxToRenderPerBatch era) FlashList has no maxToRenderPerBatch; preview-slice sizing gone
listOldID random int + regenerate-on-link-change Forced list remount when deep-linking to a comment key={reportID} + initialScrollKey={linkedReportActionID} — FlashList's hook anchors via data slicing + MVCP, no forced remount
${reportID}-${linkedReportActionID} concatenated key Same — forced remount on link change Dropped for the same reason
hasNewestReportActionRef + sortedVisibleReportActionsRef + render-time writes "Latest value" refs so the Pusher-registered callback sees fresh state despite stale closure Replaced by useEffectEvent — stable callback identity, native latest-state access, zero refs
prevUnreadMarkerReportActionID ref + render-time read/write Self-referential state to gate own-message marker suppression "only on initial open" Behavior change, documented inline: the gate removed; marker suppression now consistent regardless of prior marker (previous behavior allowed the marker to land on user's own just-sent message in narrow edge cases — arguably a bug)
prevHandleReportChangeMarkAsRead !== prev.current identity guards in useMarkAsRead Intended to skip redundant handler invocations when handler identities didn't change Always-true comparisons post-decomposition (handlers were no longer useCallback-wrapped). isMarkedAsRead short-circuit is the real safety; guards removed as noise
isLoadingInitialReportActions = false dead constant Leftover after skeleton handling moved to router Removed, branch collapsed
Initial scrollToBottom imperative on mount Pre-migration safety net FlashList's inverted mount already lands at offset 0 = visual bottom; imperative dropped

Hacks introduced (scoped, unavoidable)

  • InvertedFlashList/useInvertedWheelHandler.web.ts — FlashList uses its own RecyclerView on web and bypasses react-native-web's invertedWheelEventHandler patch (which RN-Web built into VirtualizedList for exactly this reason). Without it, wheel scroll feels reversed on every inverted chat on web. The hook ports the RN-Web patch into our InvertedFlashList wrapper. Applicable to all inverted lists in the app, not just ReportActionsList. Ideally lands upstream; this is our local compensation until then.

New built-in APIs in play

Instead of the workarounds:

<InvertedFlashList
    data={sortedVisibleReportActions}
    key={reportID}
    getItemType={(item) => item.actionName}          // FlashList recycling by type
    drawDistance={1500}                              // prefetch window for smooth scroll
    initialScrollKey={linkedReportActionID}          // deep-link anchor
    maintainVisibleContentPosition={                 // mount-at-top for transaction thread / money request reports
        shouldFocusToTopOnMount && !linkedReportActionID
            ? {startRenderingFromBottom: true}
            : undefined
    }
/>

useEffectEvent replaces two latest-value refs in useReportActionsScroll. usePrevious replaces manual prev-render refs wherever the pattern fits.

Dead code deleted

  • ReportActionsView.tsx (old monolith)
  • StaticReportActionsPreview/ (hide-render hack)
  • getReportActionsListInitialNumToRender.ts + its test
  • tests/ui/ReportActionsViewTest.tsx + tests/perf-test/ReportActionsList.perf-test.tsx (replaced by the new decomposition; coverage moved to hook-level unit tests)
  • Multiple dead returns and params across hooks (boolean returns nobody read, string params that reduced to null-checks, etc.)

Behavior changes (to QA)

  1. Unread marker suppression — no longer gated on !prevUnreadMarkerReportActionID. Rule is now consistent: a marker never lands on the current user's own new or just-confirmed message. Previously, once a marker existed, it could jump onto the user's own subsequent message in narrow scenarios (prior marker read, only own unread remains). Documented inline in shouldDisplayNewMarkerOnReportAction.ts.
  2. Mount position — declarative via startRenderingFromBottom for transaction thread / money request reports. Visual result identical to the hide-render-reveal pattern, but without the flicker risk and without a reveal timer.
  3. Wheel direction on web — inverted list now scrolls in the same direction as every other scrollable in the app. Previously felt reversed post-FlashList migration.

Risk

  • The unread marker behavior change is the main one. QA should verify the marker never appears on user's own sent messages, including the sequence: external message arrives, marker appears → user reads it → user sends own message → marker should NOT move to own message (should disappear entirely).
  • Web wheel — should feel natural across every chat, regardless of mount path (fresh load, deep link, chat swap).
  • Mount position — verify transaction thread / money request reports land at the visual top, deep links land on the linked message, regular chats land at newest.

Open app — branch (decompose/report-actions-list) vs exfy/main

Measured with React DevTools Profiler, 5 trials each.

Summary

Metric Baseline Branch Delta Delta %
Total commits 83.6 75.6 -8.0 -9.6%
Total commit duration (ms) 1120.6 1044.7 -75.9 -6.8%
Components rendered 6547 6416 -131 -2.0%

Improved

Component Baseline renders Branch renders Baseline self (ms) Branch self (ms) Delta self (ms) Delta %
fiber#891 31.4 1 6.2 0.0 -6.2 -100.0%
fiber#153 5 5 13.5 7.7 -5.8 -42.9%
fiber#4274 7 7 5.8 0.0 -5.7 -99.3%
fiber#3128 7 1 5.3 0.0 -5.3 -100.0%
fiber#1267 13 10 5.5 0.2 -5.3 -96.0%
fiber#1257 12 8 4.6 0.5 -4.1 -89.2%
fiber#1258 13 8 3.9 0.3 -3.6 -93.3%
fiber#3587 3 8 3.8 0.2 -3.6 -95.7%
fiber#897 5 1 3.5 0.0 -3.4 -98.9%
fiber#1334 13 10 3.7 0.4 -3.3 -89.7%

Regressed

Component Baseline renders Branch renders Baseline self (ms) Branch self (ms) Delta self (ms) Delta %
fiber#3242 1 10 0.0 8.1 +8.1 N/A
fiber#2090 11 10 0.3 8.0 +7.7 +2412.5%
fiber#893 10 26 0.1 5.1 +5.1 +8466.7%
fiber#2552 1 4 0.0 4.1 +4.1 +20400.0%
fiber#4461 1 21 0.0 3.8 +3.8 N/A
fiber#899 5 6 0.3 4.1 +3.8 +1175.0%
fiber#4477 1 6 0.0 3.6 +3.6 +9000.0%
fiber#4484 1 5 0.0 3.2 +3.2 N/A
fiber#873 10.2 16 0.0 3.2 +3.2 +15800.0%
fiber#1259 13 9 1.0 4.0 +4.0 +296.0%

Regressed entries are anonymous fibers with sub-1 render counts per trial — trial-to-trial variance rather than real component regressions. No named component appears in the regressed table.


Fixed Issues

$ #88320

Tests

  • Verify that no errors appear in the JS console
  • Open a regular chat — messages render, scroll works, new messages auto-scroll
  • Open a transaction thread report — thread actions merge into the list
  • Open concierge side panel — session filtering works, "Show Previous Messages" button appears
  • Click a linked message notification — list scrolls to the linked action
  • Go offline — skeleton shows in footer, actions still display
  • Send a message while at the bottom — list auto-scrolls to show it

Offline tests

  • Open a report while offline — skeleton shows, no crash
  • Go offline mid-conversation — footer skeleton appears
  • Send a message while offline — optimistic action appears immediately

QA Steps

Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios
    • I turned off my network connection and tested it while offline
    • I tested this PR with a High Traffic account against the staging or production API
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors
  • I followed proper code patterns
  • I verified all code is DRY
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

adhorodyski and others added 19 commits April 8, 2026 18:59
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…er branches

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bled

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@adhorodyski adhorodyski changed the title [No QA] Refactor ReportActionsList — cleanup, fix timing skeleton, document workarounds [HOLD] decompose ReportActionsList Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant