Remove children from AnimatedCollapsible if isExpanded false#72338
Remove children from AnimatedCollapsible if isExpanded false#72338luacmartins merged 11 commits into
Conversation
Codecov Report❌ Patch coverage is
... and 12 files with indirect coverage changes 🚀 New features to boost your workflow:
|
LukasMod
force-pushed
the
perf/report-transaction-rows
branch
from
fd6b0b0 to
11c1776
Compare
melvin-bot
Bot
requested review from
ikevin127
and removed request for
a team
|
@ikevin127 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] |
|
Just curious, why are all of the Submit buttons disabled in your videos above? |
JS00001
left a comment
JS00001
left a comment
There was a problem hiding this comment.
A few small comments, this feels really nice on larger accounts, great work
| }); | ||
| }, [groupItem.transactionsQueryJSON, newTransactionID, transactionsSnapshot?.search?.offset, isExpanded]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
We shouldnt need an effect for this, can we move this logic into handleToggle please?
There was a problem hiding this comment.
Ah, of course! Done
| <Button | ||
| text={translate('common.showMore')} | ||
| onPress={() => { | ||
| if (isGroupByReports) { |
There was a problem hiding this comment.
NAB can we move this logic out of the JSX and as a 'showMore' method?
|
@shawnborton It's because I use imported onyx state with heavy data here. Using the imported state automatically switches the app to offline mode (there is red alert with indicator at the bottom) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
| import ReportListItemHeader from './ReportListItemHeader'; | ||
| import WithdrawalIDListItemHeader from './WithdrawalIDListItemHeader'; | ||
|
|
||
| const TRANSACTIONS_PAGE_SIZE = 20; |
There was a problem hiding this comment.
We usually define these constants in COST.ts. Additionally, other groups have a 50 item limit. So let's make sure they are all consistent.
There was a problem hiding this comment.
I moved that to CONST.TRANSACTION.RESULTS_PAGE_SIZE but left it at 20 for now, as 50 seems too much for smooth animation to handle. We can easily adjust it later.
There was a problem hiding this comment.
Discussing in Slack, but I think 20 should be enough for this case and I dont think we need to only have one const for this as the usecases are not the same. Design team is also in the thread so we can discuss there https://expensify.slack.com/archives/C05LX9D6E07/p1760454907635449?thread_ts=1760397163.992789&cid=C05LX9D6E07
| {shouldDisplayShowMoreButton && !shouldDisplayLoadingIndicator && ( | ||
| <View style={[styles.w100, styles.flexRow, isLargeScreenWidth && styles.pl10]}> | ||
| <Button | ||
| text={translate('common.showMore')} | ||
| onPress={onShowMoreButtonPress} | ||
| link | ||
| shouldUseDefaultHover={false} | ||
| isNested | ||
| medium | ||
| innerStyles={[styles.ph3]} | ||
| textStyles={[styles.fontSizeNormal]} | ||
| /> | ||
| </View> | ||
| )} |
There was a problem hiding this comment.
🟡 UX when offline: Show more appears and is clickable but is a noop.
Why it matters:
◦ For non-grouped-by-report results, the button shows and is interactive while offline, but pressing it does nothing. This is confusing UX and wastes user taps.
Fix options:
◦ Disable the button with a tooltip like translate('common.offline'), or
◦ Hide the button when offline for server-paginated lists.
There was a problem hiding this comment.
Right, the button will now be hidden for non-grouped-by-report cases 👍 Fixed
| const visibleTransactions = useMemo(() => { | ||
| if (isGroupByReports) { | ||
| return transactions.slice(0, transactionsVisibleLimit); | ||
| } | ||
| return transactions; | ||
| }, [transactions, transactionsVisibleLimit, isGroupByReports]); |
There was a problem hiding this comment.
🟢 Minor optimization: only compute visibleTransactions when expanded.
Why it matters:
◦ Slicing and mapping small arrays is cheap, but for very large lists this work can add up when items are not expanded.
Fix:
◦ Compute visibleTransactions based on isExpanded for grouped-by-report so we do no extra work when collapsed.
| const visibleTransactions = useMemo(() => { | |
| if (isGroupByReports) { | |
| return transactions.slice(0, transactionsVisibleLimit); | |
| } | |
| return transactions; | |
| }, [transactions, transactionsVisibleLimit, isGroupByReports]); | |
| const visibleTransactions = useMemo(() => { | |
| if (!isExpanded && isGroupByReports) { | |
| return []; // nothing rendered when collapsed | |
| } | |
| if (isGroupByReports) { | |
| return transactions.slice(0, transactionsVisibleLimit); | |
| } | |
| return transactions; | |
| }, [transactions, transactionsVisibleLimit, isGroupByReports, isExpanded]); |
There was a problem hiding this comment.
Good catch! I left if (!isExpanded) to works for both cases.
| ? CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE | ||
| : undefined; | ||
|
|
||
| const renderExpandedContent = () => { |
There was a problem hiding this comment.
🟢 Consistency: renderExpandedContent is recreated on each render.
Why it matters:
◦ Not a functional bug; just a small re-render optimization. Since it closes over a lot of state, memoizing it provides limited benefit unless you pass it down. Optional.
• Optionally wrap in useCallback if you observe measurable re-renders.
There was a problem hiding this comment.
I decided to take it a step further with idea from your comment about isExpanded condition. I moved this logic into separate component, so all unnecessary functions are not called when the content is hidden. It improved list switching by another 50ms in my tests (from ~800ms to ~750ms). And it's also easier to follow up with smaller components now. But I didn't notice any rerender benefits so far from this move, it's the same with all solutions I tried like with useCallback.
| const isAnimating = useSharedValue(false); | ||
| const hasExpanded = useSharedValue(false); | ||
| const isExpandedFirstTime = useRef(false); | ||
| const hasExpanded = useSharedValue(isExpanded); |
There was a problem hiding this comment.
🔴 Permanent overflow: 'hidden' can clip content that should escape the container after animations finish.
ℹ️ While the issue is not visible in our testing steps, this is how I tested / found this issue:
- in the
TransactionGroupListItem.tsxcomponent where we map / rendervisibleTransactionsI added an extra piece of absolute-positioned UI (a badge)
Code spoiler
{visibleTransactions.map((transaction) => (
<OfflineWithFeedback
pendingAction={transaction.pendingAction}
key={transaction.transactionID}
>
<View style={{position: 'relative'}}>
<TransactionItemRow
report={transaction.report}
transactionItem={transaction}
violations={getTransactionViolations(transaction, violations)}
isSelected={!!transaction.isSelected}
dateColumnSize={dateColumnSize}
amountColumnSize={amountColumnSize}
taxAmountColumnSize={taxAmountColumnSize}
shouldShowTooltip={showTooltip}
shouldUseNarrowLayout={!isLargeScreenWidth}
shouldShowCheckbox={!!canSelectMultiple}
onCheckboxPress={() => onCheckboxPress?.(transaction as unknown as TItem)}
columns={currentColumns}
onButtonPress={() => {
openReportInRHP(transaction);
}}
style={[styles.noBorderRadius, shouldUseNarrowLayout ? [styles.p3, styles.pt2] : [styles.ph3, styles.pv1Half], isGroupByReports && styles.pr10]}
isReportItemChild
isInSingleTransactionReport={groupItem.transactions.length === 1}
areAllOptionalColumnsHidden={areAllOptionalColumnsHidden}
/>
{/* Debug badge that extends outside the row/container */}
<View
testID="overflowBadge"
style={{
position: 'absolute',
bottom: -24, // extend beyond the container
right: 16,
width: 90,
height: 24,
backgroundColor: 'red',
alignItems: 'center',
justifyContent: 'center',
}}
>
<Text style={{color: 'white'}}>Badge</Text>
</View>
</View>
</OfflineWithFeedback>
))}Why it matters:
- Components inside the collapsible (e.g., tooltips, popovers, absolute-positioned buttons/menus) may be visually clipped if overflow stays hidden after expand. Previously, overflow was set to 'hidden' only while animating, and then 'visible' again.
- Concrete example: If a row inside the collapsible opens a context menu or a tooltip that extends beyond the container’s bounds, it will be cut off, especially on mobile (strict iOS: Native) where these overlays frequently extend beyond their parent.
Fix approach:
- Compute animation progress and only set overflow: 'hidden' while the height is mid-transition. Once fully expanded or fully collapsed, set it to 'visible' (expanded) or 'hidden' (collapsed).
- This avoids storing an extra isAnimating shared value and keeps logic declarative.
Example fix
// Replace the withTiming(height)-only approach with a normalized progress
// so you can infer "isAnimating" from progress.
const expandedSV = useSharedValue(isExpanded);
useEffect(() => {
expandedSV.value = isExpanded;
}, [isExpanded, expandedSV]);
const progress = useDerivedValue(() => {
// 0 = collapsed, 1 = expanded
return withTiming(expandedSV.value ? 1 : 0, {duration, easing});
}, [duration]);
const contentAnimatedStyle = useAnimatedStyle(() => {
const h = contentHeight.value * progress.value;
const isAnimating = progress.value > 0 && progress.value < 1;
const progressVisibility = progress.value === 1 ? 'visible' : 'hidden';
return {
height: h,
overflow: isAnimating ? 'hidden' : progressVisibility,
};
});Results
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
The animation looks the same with overflow: "visible". I'll skip this logic and not manipulate that style at all, leaving it at the default. Final effect is like "After" in your comment.
| const isAnimating = useSharedValue(false); | ||
| const hasExpanded = useSharedValue(false); | ||
| const isExpandedFirstTime = useRef(false); | ||
| const hasExpanded = useSharedValue(isExpanded); |
There was a problem hiding this comment.
🟡 Conditional unmounting can cause “first expand” jank and misses the “keep-mounted-while-collapsing” behavior
In addition to the ☝️ above comment:
Why it matters:
- With mount-on-expand, the very first expand has to measure content. animatedHeight tries to animate to contentHeight, but contentHeight is 0 until onLayout runs. This can lead to a two-phase expand where height goes 0 → 0 → measuredHeight, which feels like a hitch on slower devices.
- When collapsing, we remove children immediately (conditional render), but we also rely on FadeOut. Reanimated exit animations will run, but the container height is also animating to 0. If we ever remove entering/exiting or it fails on web, the collapse might snap shut visually.
Fix approach:
- Keep children rendered until the collapse animation (progress) finishes, then unmount. This preserves smooth transitions and avoids measuring hitches.
- We can keep your FadeIn/FadeOut and still unmount at the end of animation to keep the performance win.
Example fix
const [isRendered, setIsRendered] = React.useState(isExpanded);
useEffect(() => {
if (isExpanded) {
setIsRendered(true);
}
}, [isExpanded]);
// When animation completes to 0, unmount using runOnJS to avoid UI-thread state updates
useAnimatedReaction(
() => progress.value,
(p, prev) => {
if (prev !== undefined && prev > 0 && p === 0) {
runOnJS(setIsRendered)(false);
}
},
[],
);
// Use isRendered for conditional rendering instead of isExpanded directly
<Animated.View style={[contentAnimatedStyle, contentStyle]}>
{isRendered ? (
<Animated.View
style={styles.stickToTop}
entering={FadeIn}
exiting={FadeOut}
onLayout={(e) => {
const h = e.nativeEvent.layout.height;
if (h) {
contentHeight.value = h;
}
}}
>
...
</Animated.View>
) : null}
</Animated.View>Here's a complete example with code changes combined from the ☝️ above comment + this comment's suggested fix:
Code spoiler
import React, {useEffect, useState} from 'react';
import {View} from 'react-native';
import Animated, {
FadeIn,
FadeOut,
runOnJS,
useAnimatedReaction,
useAnimatedStyle,
useDerivedValue,
useSharedValue,
withTiming,
} from 'react-native-reanimated';
import {easing} from '@components/Modal/ReanimatedModal/utils';
import useThemeStyles from '@hooks/useThemeStyles';
type Props = {
isExpanded: boolean;
header: React.ReactNode;
children: React.ReactNode;
duration?: number;
style?: any;
contentStyle?: any;
};
function AnimatedCollapsible({isExpanded, children, header, duration = 300, style, contentStyle}: Props) {
const styles = useThemeStyles();
// Measure + animation state
const contentHeight = useSharedValue(0);
const hasMeasured = useSharedValue(false);
const isExpandedSV = useSharedValue(isExpanded);
// Keep children mounted while collapsing; unmount only after progress reaches 0
const [isRendered, setIsRendered] = useState(isExpanded);
useEffect(() => {
isExpandedSV.value = isExpanded;
if (isExpanded) {
// Mount immediately so we can measure before animating
setIsRendered(true);
}
}, [isExpanded, isExpandedSV]);
// 0 = collapsed, 1 = expanded; wait for measurement before animating open
const progress = useDerivedValue(() => {
const target = isExpandedSV.value && hasMeasured.value ? 1 : 0;
return withTiming(target, {duration, easing});
}, [duration]);
// When collapse animation finishes (progress -> 0), unmount content
useAnimatedReaction(
() => progress.value,
(p, prev) => {
if (prev !== undefined && prev > 0 && p === 0) {
runOnJS(setIsRendered)(false);
}
},
[],
);
const contentAnimatedStyle = useAnimatedStyle(() => {
const h = hasMeasured.value ? contentHeight.value * progress.value : 0;
const isAnimating = progress.value > 0 && progress.value < 1;
return {
height: h,
// Hide overflow during the transition; once fully expanded, allow overflow
overflow: isAnimating ? 'hidden' : progress.value === 1 ? 'visible' : 'hidden',
};
});
return (
<View style={style}>
<View style={[headerStyle, styles.flexRow, styles.alignItemsCenter]}>
<View style={[styles.flex1]}>{header}</View>
<PressableWithFeedback
onPress={onPress}
disabled={disabled}
style={[styles.p3, styles.justifyContentCenter, styles.alignItemsCenter, expandButtonStyle]}
accessibilityRole={CONST.ROLE.BUTTON}
accessibilityLabel={isExpanded ? 'Collapse' : 'Expand'}
>
{({hovered}) => (
<Icon
src={isExpanded ? Expensicons.UpArrow : Expensicons.DownArrow}
fill={hovered ? theme.textSupporting : theme.icon}
small
/>
)}
</PressableWithFeedback>
</View>
<Animated.View style={[contentAnimatedStyle, contentStyle]}>
{isRendered ? (
<Animated.View
style={styles.stickToTop}
entering={FadeIn}
exiting={FadeOut}
onLayout={(e) => {
const h = e.nativeEvent.layout.height;
if (h) {
contentHeight.value = h;
hasMeasured.value = true;
}
}}
>
<View style={[styles.pv2, styles.ph3]}>
<View style={[styles.borderBottom]} />
</View>
{children}
</Animated.View>
) : null}
</Animated.View>
</View>
);
}
export default AnimatedCollapsible;There was a problem hiding this comment.
- I added additional conditions to animatedHeight to skip the initial no-op animation as proposed, but I used contentHeight value to not introduce a new one.
if (!contentHeight.get()) {
return 0;
}
- I introduced
const [isRendered, setIsRendered] = React.useState(isExpanded);as requested, to delay removing the content until the animation finishes.
I use it as{isExpanded || isRendered ? <... />}to remove the content later, but mount it as soon as possible.
Without the isExpanded check, there was a weird effect on iOS where the content expanded quickly but rendered slightly too late. - I replaced FadeOut/FadeIn with animatedOpacity.get()
🧪 Testing recommendationsAnimatedCollapsible
TransactionGroupListItem
|
|
🟢 Completed reviewer checklist. 🔄 Overall the improvement is solid - though I dropped some important comments above targeting both logic and performance + 🧪 testing recommendations around the changes, which should probably be addressed before merging, as while testing: I noticed jankiness in the animations of expanding reports with lots of transactions, as well as further expanding via ☝️ The animation jankiness can be observed in my checklist testing videos (I know that things are probably slower on DEV then they will be on production builds). ♻️ I will defer approval until all comments are addressed, in case I need to retest (code changes). |
|
LGTM |
. Thank you for your hard work!
|
@ikevin127 Thanks for the great CR! I’ve replied to all threads and pushed changes.
I will think about some unit tests for TransactionGroupListItem or TransactionGroupListExpanded tomorrow |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
@ikevin127 Thanks for a thorough review, can you please give it a test after the recent changes? thanks!
|
🚧 @mountiny 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, Desktop, and Web. Happy testing! 🧪🧪
|
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
@ikevin127 Thanks for a thorough review, can you please give it a test after the recent changes? thanks!
Sure, just did and it LGTM 🟢 Thanks for carefully considering the requested changes / improvements 🚀
|
Gonna merge this one. We can easily change the number of transactions we display per group |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.2.32-0 🚀
|
|
@m-natarajan Click on the file to download, then Settings > Troubleshoot > Import Onyx state and import the file. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.32-6 🚀
|
| createAndOpenSearchTransactionThread(transactionItem, iouAction, currentSearchHash, backTo); | ||
| return; | ||
| } | ||
| markReportIDAsExpense(reportID); |
There was a problem hiding this comment.
Coming from #72178, we need to check if the report is invoice or task, if so, we shouldn't mark it as expense
There was a problem hiding this comment.
This PR focused on another issue, logic mentioned is only copied from src/components/SelectionListWithSections/Search/TransactionGroupListItem.tsx
9 participants
Explanation of Change
Optimized rendering performance in the
To-doreports views (eg.Submit,Pay). Previously, all transaction rows for each report were rendered upfront, even if hidden, causing significant overhead (especially for high-value accounts). The new approach delays rendering transaction rows until the user expands a report and addsShow morelogic similar to other transaction groups (Accountinglists likeUnapproved cash) if there is more data. Pagination size set to 20 for To-do reports.Fixed Issues
$ #72025
PROPOSAL:
Tests
onyx-state.txt
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
android-native.mov
Android: mWeb Chrome
android-chrome.mov
iOS: Native
ios-native.mov
iOS: mWeb Safari
ios-safari.mov
MacOS: Chrome / Safari
web-chrome.mov
web-safari.mov
MacOS: Desktop
desktop.mov