From d70b6cd8ea0c7eeeb78d75eae01aef35c3b1d763 Mon Sep 17 00:00:00 2001 From: Andre Staltz Date: Wed, 26 Oct 2022 17:45:01 +0300 Subject: [PATCH] ux: bug fix: memory leak in the connections tab --- package-lock.json | 5 + package.json | 1 + .../screens/central/connections-tab/model.ts | 22 ++- .../screens/central/connections-tab/view.ts | 4 +- src/frontend/screens/central/index.ts | 3 +- src/frontend/screens/central/model.ts | 162 ++++++++++++++---- src/frontend/screens/central/top-bar/index.ts | 111 ++++++------ src/frontend/screens/central/view.ts | 60 +++---- 8 files changed, 242 insertions(+), 126 deletions(-) diff --git a/package-lock.json b/package-lock.json index b6daae0be..04ec968b2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15614,6 +15614,11 @@ "resolved": "https://registry.npmjs.org/extsprintf/-/extsprintf-1.3.0.tgz", "integrity": "sha1-lpGEQOMEGnpBT4xS48V06zw+HgU=" }, + "fast-deep-equal": { + "version": "3.1.3", + "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz", + "integrity": "sha512-f3qQ9oQy9j2AhBe/H9VC91wLmKBCCU/gDOnKNAYG5hswO7BLKj09Hc5HYNz9cGI++xlpDCIgDaitVs03ATR84Q==" + }, "fast-json-stable-stringify": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/fast-json-stable-stringify/-/fast-json-stable-stringify-2.0.0.tgz", diff --git a/package.json b/package.json index 5d4acf4c3..a83208163 100644 --- a/package.json +++ b/package.json @@ -96,6 +96,7 @@ "cycle-native-toastandroid": "1.1.0", "electron-context-menu": "3.1.2", "electron-ssb-client": "2.0.0", + "fast-deep-equal": "3.1.3", "fix-webm-duration": "1.0.4", "hermes-engine": "0.9.0", "i18n-js": "3.8.0", diff --git a/src/frontend/screens/central/connections-tab/model.ts b/src/frontend/screens/central/connections-tab/model.ts index b34271cb1..c61af5959 100644 --- a/src/frontend/screens/central/connections-tab/model.ts +++ b/src/frontend/screens/central/connections-tab/model.ts @@ -6,6 +6,7 @@ import xs, {Stream} from 'xstream'; import sample from 'xstream-sample'; import concat from 'xstream/extra/concat'; import {FeedId} from 'ssb-typescript'; +import deepEquals = require('fast-deep-equal'); import {State as AppState} from '~frontend/drivers/appstate'; import {NetworkSource} from '~frontend/drivers/network'; import {SSBSource} from '~frontend/drivers/ssb'; @@ -172,6 +173,10 @@ function reevaluateStatus(prev: State): State { return prev; } +function pickId(peer: PeerKV): string { + return peer[0]; +} + export default function model( ssbSource: SSBSource, networkSource: NetworkSource, @@ -231,7 +236,15 @@ export default function model( const rooms = allPeers.filter( ([, data]) => (data.type as any) === 'room', ); - return {...prev, peers, rooms, timestampPeersAndRooms: Date.now()}; + if ( + deepEquals(prev.peers.map(pickId), peers.map(pickId)) && + deepEquals(prev.rooms.map(pickId), rooms.map(pickId)) + ) { + // Optimization: nothing changed, so don't emit a new state object + return prev; + } else { + return {...prev, peers, rooms, timestampPeersAndRooms: Date.now()}; + } }, ); @@ -281,7 +294,12 @@ export default function model( return peer; } }); - return {...prev, peers, timestampPeersAndRooms: Date.now()}; + if (deepEquals(prev.peers.map(pickId), peers.map(pickId))) { + // Optimization: nothing changed, so don't emit a new state object + return prev; + } else { + return {...prev, peers, timestampPeersAndRooms: Date.now()}; + } }, ); diff --git a/src/frontend/screens/central/connections-tab/view.ts b/src/frontend/screens/central/connections-tab/view.ts index 1f98b4d08..69a83f0ea 100644 --- a/src/frontend/screens/central/connections-tab/view.ts +++ b/src/frontend/screens/central/connections-tab/view.ts @@ -452,8 +452,8 @@ export default function view(state$: Stream) { (p) => p[1].state === 'connected', ); - return h(View, {style: styles.container}, [ - h(View, {style: styles.innerContainer}, [ + return h(View, {key: 'conntab', style: styles.container}, [ + h(View, {key: 'ic', style: styles.innerContainer}, [ h(Summary, {connectedPeers}), h(Scenario, {status, scenario}), h(Recommendations, { diff --git a/src/frontend/screens/central/index.ts b/src/frontend/screens/central/index.ts index 724b9333a..d4a82b1c1 100644 --- a/src/frontend/screens/central/index.ts +++ b/src/frontend/screens/central/index.ts @@ -136,7 +136,8 @@ export function central(sources: Sources): Sinks { ? privateTabSinks.fab : connectionsTabSinks.fab, ) - .flatten(); + .flatten() + .compose(dropRepeats()); const command$ = navigation( state$, diff --git a/src/frontend/screens/central/model.ts b/src/frontend/screens/central/model.ts index 25f41b622..7091724a2 100644 --- a/src/frontend/screens/central/model.ts +++ b/src/frontend/screens/central/model.ts @@ -6,6 +6,7 @@ import xs, {Stream} from 'xstream'; import {Reducer, Lens} from '@cycle/state'; import {Animated} from 'react-native'; import {FeedId, MsgId} from 'ssb-typescript'; +import deepEquals = require('fast-deep-equal'); import {SSBSource} from '~frontend/drivers/ssb'; import progressCalculation, { State as ProgressState, @@ -54,14 +55,26 @@ export const publicTabLens: Lens = { const isVisible = parent.currentTab === 'public'; const {selfFeedId, selfAvatarUrl, canPublishSSB} = parent; if (parent.publicTab) { - return { - ...parent.publicTab, - isVisible, - selfFeedId, - selfAvatarUrl, - canPublishSSB, - }; + const prev = parent.publicTab; + if ( + prev.isVisible === isVisible && + prev.selfFeedId === selfFeedId && + prev.selfAvatarUrl === selfAvatarUrl && + prev.canPublishSSB === canPublishSSB + ) { + // Optimization: nothing changed + return prev; + } else { + return { + ...parent.publicTab, + isVisible, + selfFeedId, + selfAvatarUrl, + canPublishSSB, + }; + } } else { + // Initialize the public tab state return { isVisible, selfFeedId, @@ -79,13 +92,23 @@ export const publicTabLens: Lens = { }, set: (parent: State, child: PublicTabState): State => { - return { - ...parent, - initializedSSB: child.initializedSSB, - numOfPublicUpdates: child.numOfUpdates, - lastSessionTimestamp: child.lastSessionTimestamp, - publicTab: child, - }; + if ( + parent.initializedSSB === child.initializedSSB && + parent.numOfPublicUpdates === child.numOfUpdates && + parent.lastSessionTimestamp === child.lastSessionTimestamp && + deepEquals(parent.publicTab, child) + ) { + // Optimization: nothing changed in the child, so don't update the parent + return parent; + } else { + return { + ...parent, + initializedSSB: child.initializedSSB, + numOfPublicUpdates: child.numOfUpdates, + lastSessionTimestamp: child.lastSessionTimestamp, + publicTab: child, + }; + } }, }; @@ -94,8 +117,24 @@ export const privateTabLens: Lens = { const isVisible = parent.currentTab === 'private'; const {selfFeedId, selfAvatarUrl} = parent; if (parent.privateTab) { - return {...parent.privateTab, isVisible, selfFeedId, selfAvatarUrl}; + const prev = parent.privateTab; + if ( + prev.isVisible === isVisible && + prev.selfFeedId === selfFeedId && + prev.selfAvatarUrl === selfAvatarUrl + ) { + // Optimization: nothing changed + return prev; + } else { + return { + ...parent.privateTab, + isVisible, + selfFeedId, + selfAvatarUrl, + }; + } } else { + // Initialize the private tab state return { isVisible, selfFeedId, @@ -109,11 +148,19 @@ export const privateTabLens: Lens = { }, set: (parent: State, child: PrivateTabState): State => { - return { - ...parent, - numOfPrivateUpdates: child.updates.size, - privateTab: child, - }; + if ( + parent.numOfPrivateUpdates === child.updates.size && + deepEquals(parent.privateTab, child) + ) { + // Optimization: nothing changed in the child, so don't update the parent + return parent; + } else { + return { + ...parent, + numOfPrivateUpdates: child.updates.size, + privateTab: child, + }; + } }, }; @@ -122,8 +169,24 @@ export const activityTabLens: Lens = { const isVisible = parent.currentTab === 'activity'; const {selfFeedId, selfAvatarUrl} = parent; if (parent.activityTab) { - return {...parent.activityTab, isVisible, selfFeedId, selfAvatarUrl}; + const prev = parent.activityTab; + if ( + prev.isVisible === isVisible && + prev.selfFeedId === selfFeedId && + prev.selfAvatarUrl === selfAvatarUrl + ) { + // Optimization: nothing changed + return prev; + } else { + return { + ...parent.activityTab, + isVisible, + selfFeedId, + selfAvatarUrl, + }; + } } else { + // Initialize the activity tab state return { isVisible, selfFeedId, @@ -137,11 +200,19 @@ export const activityTabLens: Lens = { }, set: (parent: State, child: ActivityTabState): State => { - return { - ...parent, - numOfActivityUpdates: child.numOfUpdates, - activityTab: child, - }; + if ( + parent.numOfActivityUpdates === child.numOfUpdates && + deepEquals(parent.activityTab, child) + ) { + // Optimization: nothing changed in the child, so don't update the parent + return parent; + } else { + return { + ...parent, + numOfActivityUpdates: child.numOfUpdates, + activityTab: child, + }; + } }, }; @@ -150,14 +221,26 @@ export const connectionsTabLens: Lens = { const isVisible = parent.currentTab === 'connections'; const {selfFeedId, selfAvatarUrl, initializedSSB} = parent; if (parent.connectionsTab) { - return { - ...parent.connectionsTab, - isVisible, - selfFeedId, - selfAvatarUrl, - initializedSSB, - }; + const prev = parent.connectionsTab; + if ( + prev.isVisible === isVisible && + prev.selfFeedId === selfFeedId && + prev.selfAvatarUrl === selfAvatarUrl && + prev.initializedSSB === initializedSSB + ) { + // Optimization: nothing changed + return prev; + } else { + return { + ...parent.connectionsTab, + isVisible, + selfFeedId, + selfAvatarUrl, + initializedSSB, + }; + } } else { + // Initialize the connections tab state return { isVisible, selfFeedId, @@ -180,10 +263,15 @@ export const connectionsTabLens: Lens = { }, set: (parent: State, child: ConnectionsTabState): State => { - return { - ...parent, - connectionsTab: child, - }; + if (deepEquals(parent.connectionsTab, child)) { + // Optimization: nothing changed in the child, so don't update the parent + return parent; + } else { + return { + ...parent, + connectionsTab: child, + }; + } }, }; diff --git a/src/frontend/screens/central/top-bar/index.ts b/src/frontend/screens/central/top-bar/index.ts index 45693acf2..d62a2ddc6 100644 --- a/src/frontend/screens/central/top-bar/index.ts +++ b/src/frontend/screens/central/top-bar/index.ts @@ -3,6 +3,7 @@ // SPDX-License-Identifier: MPL-2.0 import xs, {Stream} from 'xstream'; +import dropRepeatsByKeys from 'xstream-drop-repeats-by-keys'; import {ReactSource} from '@cycle/react'; import {h} from '@cycle/react'; import {StateSource} from '@cycle/state'; @@ -183,59 +184,65 @@ function view(state$: Stream) { let hideYWhenScrolling: Animated.AnimatedMultiplication | null = null; let hideOpacityWhenScrolling: Animated.AnimatedMultiplication | null = null; - return state$.map((state) => { - // Avoid re-instantiating a new animated value on every stream emission - if (!hideYWhenScrolling) { - hideYWhenScrolling = calcTranslateY(state.scrollHeaderBy); - } - if (!hideOpacityWhenScrolling) { - hideOpacityWhenScrolling = calcOpacity(hideYWhenScrolling); - } - - const translateY = state.currentTab === 'public' ? hideYWhenScrolling : 0; - const opacity = - state.currentTab === 'public' ? hideOpacityWhenScrolling : 1; - - return h( - Animated.View, - {style: [styles.container, {transform: [{translateY}]}]}, - [ - h(View, {style: styles.innerContainer}, [ - Platform.OS === 'web' - ? null - : h(Animated.View, {style: {opacity}}, [ - HeaderMenuButton('menuButton'), - state.hasNewVersion ? h(View, {style: styles.updateDot}) : null, - ]), - h( - Animated.Text, - {style: [styles.title, {opacity}]}, - tabTitle(state.currentTab), - ), - h( - Animated.View, - { - style: [ - styles.publicRightSide, - state.currentTab === 'public' - ? styles.publicRightSideShown - : styles.publicRightSideHidden, - {opacity}, + return state$ + .compose( + dropRepeatsByKeys(['scrollHeaderBy', 'currentTab', 'hasNewVersion']), + ) + .map((state) => { + // Avoid re-instantiating a new animated value on every stream emission + if (!hideYWhenScrolling) { + hideYWhenScrolling = calcTranslateY(state.scrollHeaderBy); + } + if (!hideOpacityWhenScrolling) { + hideOpacityWhenScrolling = calcOpacity(hideYWhenScrolling); + } + + const translateY = state.currentTab === 'public' ? hideYWhenScrolling : 0; + const opacity = + state.currentTab === 'public' ? hideOpacityWhenScrolling : 1; + + return h( + Animated.View, + {style: [styles.container, {transform: [{translateY}]}]}, + [ + h(View, {style: styles.innerContainer}, [ + Platform.OS === 'web' + ? null + : h(Animated.View, {style: {opacity}}, [ + HeaderMenuButton('menuButton'), + state.hasNewVersion + ? h(View, {style: styles.updateDot}) + : null, + ]), + h( + Animated.Text, + {style: [styles.title, {opacity}]}, + tabTitle(state.currentTab), + ), + h( + Animated.View, + { + style: [ + styles.publicRightSide, + state.currentTab === 'public' + ? styles.publicRightSideShown + : styles.publicRightSideHidden, + {opacity}, + ], + }, + [ + h(HeaderButton, { + sel: 'search', + icon: IconNames.search, + side: 'right', + accessibilityLabel: t('public.search.accessibility_label'), + }), ], - }, - [ - h(HeaderButton, { - sel: 'search', - icon: IconNames.search, - side: 'right', - accessibilityLabel: t('public.search.accessibility_label'), - }), - ], - ), - ]), - ], - ); - }); + ), + ]), + ], + ); + }); } export function topBar(sources: Sources): Sinks { diff --git a/src/frontend/screens/central/view.ts b/src/frontend/screens/central/view.ts index b38c1d00e..45419ee0e 100644 --- a/src/frontend/screens/central/view.ts +++ b/src/frontend/screens/central/view.ts @@ -3,6 +3,7 @@ // SPDX-License-Identifier: MPL-2.0 import xs, {Stream} from 'xstream'; +import dropRepeatsByKeys from 'xstream-drop-repeats-by-keys'; import { ReactElement, Fragment, @@ -239,49 +240,32 @@ class MobileProgressPill extends Component<{progress: number}> { } } -class MobileTabsBar extends Component { - public shouldComponentUpdate(nextProps: MobileTabsBar['props']) { - const prevProps = this.props; - if (nextProps.currentTab !== prevProps.currentTab) return true; - if (nextProps.numOfPublicUpdates !== prevProps.numOfPublicUpdates) { - return true; - } - if (nextProps.numOfPrivateUpdates !== prevProps.numOfPrivateUpdates) { - return true; - } - if (nextProps.numOfActivityUpdates !== prevProps.numOfActivityUpdates) { - return true; - } - if (nextProps.connectionsTab !== prevProps.connectionsTab) { - return true; - } - if (nextProps.initializedSSB !== prevProps.initializedSSB) { - return true; - } - if (nextProps.combinedProgress !== prevProps.combinedProgress) { - return true; - } - return false; - } - +class MobileTabsBar extends PureComponent { public render() { - const {currentTab, connectionsTab, initializedSSB, combinedProgress} = - this.props; + const { + currentTab, + connectionsTab, + numOfPublicUpdates, + numOfPrivateUpdates, + numOfActivityUpdates, + initializedSSB, + combinedProgress, + } = this.props; const status = connectionsTab?.status ?? 'bad'; return h(View, {style: styles.tabBar}, [ h(PublicTabIcon, { isSelected: currentTab === 'public', - numOfUpdates: this.props.numOfPublicUpdates, + numOfUpdates: numOfPublicUpdates, }), h(PrivateTabIcon, { isSelected: currentTab === 'private', - numOfUpdates: this.props.numOfPrivateUpdates, + numOfUpdates: numOfPrivateUpdates, }), h(ActivityTabIcon, { isSelected: currentTab === 'activity', - numOfUpdates: this.props.numOfActivityUpdates, + numOfUpdates: numOfActivityUpdates, }), h(ConnectionsTabIcon, { isSelected: currentTab === 'connections', @@ -310,9 +294,21 @@ export default function view( activityTab$: Stream>, connectionsTab$: Stream>, ) { + const viewState$ = state$.compose( + dropRepeatsByKeys([ + 'currentTab', + 'numOfPublicUpdates', + 'numOfPrivateUpdates', + 'numOfActivityUpdates', + (state) => state.connectionsTab?.status ?? 'bad', + 'initializedSSB', + 'combinedProgress', + ]), + ); + return xs .combine( - state$, + viewState$, fabProps$, topBar$, publicTab$.startWith($(View)), @@ -344,5 +340,5 @@ export default function view( Platform.OS === 'web' ? null : h(MobileTabsBar, state), ]), ) - .startWith($(View, {key: 'tbs', style:styles.topBarStub})); + .startWith($(View, {key: 'tbs', style: styles.topBarStub})); }