From d19657f10364be03e2ab23719b912bfabc99ab6d Mon Sep 17 00:00:00 2001 From: Matthew Horan Date: Sat, 13 Apr 2024 15:34:34 -0400 Subject: [PATCH] Don't unnecessarily fetch lines for notification When a notification is received for the current buffer and we're still connected (but in the background), new lines will already have loaded when the app resumes However, the lines may not have rendered yet, so they cannot be scrolled to reliably. Work around this by changing the FlatList key, resetting its internal measurement state. Subsequent scroll will then either make it to the target or cycle trigger onScrollToIndexFailed. --- __tests__/usecase/App.tsx | 59 +-------- __tests__/usecase/buffers/ui/Buffer.tsx | 1 + .../usecase/buffers/ui/BufferContainer.tsx | 117 ++++++++++++++++-- src/store/app.ts | 24 ++-- src/usecase/App.tsx | 6 +- src/usecase/buffers/ui/Buffer.tsx | 37 +++++- src/usecase/buffers/ui/BufferContainer.tsx | 22 ++-- 7 files changed, 164 insertions(+), 102 deletions(-) diff --git a/__tests__/usecase/App.tsx b/__tests__/usecase/App.tsx index 15517e4..ef6ce13 100644 --- a/__tests__/usecase/App.tsx +++ b/__tests__/usecase/App.tsx @@ -5,10 +5,7 @@ import { act, render } from '../../src/test-utils'; import { configureStore } from '@reduxjs/toolkit'; import { reducer } from '../../src/store'; import { AppState } from '../../src/store/app'; -import { - bufferNotificationAction, - changeCurrentBufferAction -} from '../../src/store/actions'; +import { bufferNotificationAction } from '../../src/store/actions'; jest.mock('react-native-drawer-layout'); @@ -68,59 +65,5 @@ describe('App', () => { expect(clearHotlistForBuffer).toHaveBeenCalledWith(null); expect(fetchBufferInfo).toHaveBeenCalledWith(bufferId); }); - - it('fetches buffer info if the notification is for the current buffer', () => { - const bufferId = '86c417600'; - const store = configureStore({ - reducer, - preloadedState: { - buffers: { - [bufferId]: { - full_name: 'irc.libera.#weechat', - hidden: 0, - id: bufferId, - local_variables: { - channel: '#weechat', - name: 'libera.#weechat', - plugin: 'irc', - type: 'channel' - }, - notify: 3, - number: 2, - pointers: [bufferId], - short_name: '#weechat', - title: '', - type: 0 - } - } - } - }); - const fetchBufferInfo = jest.fn(); - const clearHotlistForBuffer = jest.fn(); - - render( - {}} - fetchBufferInfo={fetchBufferInfo} - sendMessageToBuffer={() => {}} - clearHotlistForBuffer={clearHotlistForBuffer} - />, - { store } - ); - - act(() => { - store.dispatch(changeCurrentBufferAction(bufferId)); - store.dispatch( - bufferNotificationAction({ - bufferId, - lineId: '8580dcc40', - identifier: '1fb4fc1d-530b-466f-85be-de27772de0a9' - }) - ); - }); - - expect(fetchBufferInfo).toHaveBeenCalledWith(bufferId); - expect(clearHotlistForBuffer).not.toHaveBeenCalled(); - }); }); }); diff --git a/__tests__/usecase/buffers/ui/Buffer.tsx b/__tests__/usecase/buffers/ui/Buffer.tsx index c0ff93c..d1cef97 100644 --- a/__tests__/usecase/buffers/ui/Buffer.tsx +++ b/__tests__/usecase/buffers/ui/Buffer.tsx @@ -54,6 +54,7 @@ describe(Buffer, () => { parseArgs={[]} bufferId={''} fetchMoreLines={() => {}} + clearNotification={() => {}} /> ); diff --git a/__tests__/usecase/buffers/ui/BufferContainer.tsx b/__tests__/usecase/buffers/ui/BufferContainer.tsx index 003ce51..65ae57c 100644 --- a/__tests__/usecase/buffers/ui/BufferContainer.tsx +++ b/__tests__/usecase/buffers/ui/BufferContainer.tsx @@ -1,6 +1,7 @@ import 'react-native'; +import Buffer from '../../../../src/usecase/buffers/ui/Buffer'; import BufferContainer from '../../../../src/usecase/buffers/ui/BufferContainer'; -import { act, render } from '../../../../src/test-utils'; +import { act, render, screen } from '../../../../src/test-utils'; import { reducer } from '../../../../src/store'; import { configureStore } from '@reduxjs/toolkit'; import { @@ -8,12 +9,15 @@ import { changeCurrentBufferAction, fetchLinesAction } from '../../../../src/store/actions'; -import Buffer from '../../../../src/usecase/buffers/ui/Buffer'; -jest.mock('.../../../../src/usecase/buffers/ui/Buffer'); +jest.mock('../../../../src/usecase/buffers/ui/Buffer'); describe('BufferContainer', () => { - it('scrolls to line and clears notification after fetching lines', () => { + beforeEach(() => { + jest.mocked(Buffer).mockImplementation(); + }); + + it('defers notification until buffer change and line fetch', () => { const bufferId = '86c417600'; const store = configureStore({ reducer, @@ -40,7 +44,97 @@ describe('BufferContainer', () => { } }); - Buffer.prototype.scrollToLine = jest.fn(); + render( + {}} + fetchMoreLines={() => {}} + />, + { store } + ); + + const bufferContainer = screen.UNSAFE_getByType( + BufferContainer.WrappedComponent + ); + + act(() => { + store.dispatch( + bufferNotificationAction({ + bufferId, + lineId: '86c2ff040', + identifier: '1fb4fc1d-530b-466f-85be-de27772de0a9' + }) + ); + }); + + expect(bufferContainer.props.notification).toBeNull(); + + act(() => { + store.dispatch(changeCurrentBufferAction(bufferId)); + }); + + expect(bufferContainer.props.notification).toBeNull(); + + act(() => { + store.dispatch( + fetchLinesAction([ + { + buffer: '86c417600', + date: '2024-04-05T02:40:09.000Z', + date_printed: '2024-04-06T17:20:30.000Z', + displayed: 1, + highlight: 0, + message: 'Second message', + pointers: ['86c417600', '8580eeec0', '8580dcc40', '86c2ff040'], + prefix: 'user', + tags_array: ['irc_privmsg', 'notify_message'] + } as WeechatLine + ]) + ); + }); + + expect(bufferContainer.props.notification).toEqual({ + bufferId, + lineId: '86c2ff040', + identifier: '1fb4fc1d-530b-466f-85be-de27772de0a9' + }); + }); + + it('clears notification after being handled by the buffer component', () => { + const ActualBuffer = jest.requireActual< + typeof import('../../../../src/usecase/buffers/ui/Buffer') + >('../../../../src/usecase/buffers/ui/Buffer').default; + jest.mocked(Buffer).mockImplementation((props) => { + return new ActualBuffer(props); + }); + jest.spyOn(ActualBuffer.prototype, 'componentDidUpdate'); + + const bufferId = '86c417600'; + const store = configureStore({ + reducer, + preloadedState: { + buffers: { + [bufferId]: { + full_name: 'irc.libera.#weechat', + hidden: 0, + id: bufferId, + local_variables: { + channel: '#weechat', + name: 'libera.#weechat', + plugin: 'irc', + type: 'channel' + }, + notify: 3, + number: 2, + pointers: [bufferId], + short_name: '#weechat', + title: '', + type: 0 + } + } + } + }); render( { store.dispatch( bufferNotificationAction({ bufferId, - lineId: '8580dcc40', + lineId: '86c2ff040', identifier: '1fb4fc1d-530b-466f-85be-de27772de0a9' }) ); }); - expect(Buffer.prototype.scrollToLine).not.toHaveBeenCalled(); + expect(store.getState().app.notification).not.toBeNull(); act(() => { store.dispatch(changeCurrentBufferAction(bufferId)); + }); + + act(() => { store.dispatch( fetchLinesAction([ { @@ -83,7 +180,11 @@ describe('BufferContainer', () => { ); }); - expect(Buffer.prototype.scrollToLine).toHaveBeenCalledWith('8580dcc40'); + expect(ActualBuffer.prototype.componentDidUpdate).toHaveBeenCalledWith( + expect.objectContaining({ notificationLineId: '86c2ff040' }), + expect.anything(), + undefined + ); expect(store.getState().app.notification).toBeNull(); }); }); diff --git a/src/store/app.ts b/src/store/app.ts index eee7c4c..a60f6a1 100644 --- a/src/store/app.ts +++ b/src/store/app.ts @@ -15,21 +15,22 @@ export type AppState = { connected: boolean; currentBufferId: string | null; notification: { bufferId: string; lineId: string; identifier: string } | null; - notificationBufferLinesFetched: boolean; + currentBufferLinesFetched: boolean; }; const initialState: AppState = { connected: false, currentBufferId: null, notification: null, - notificationBufferLinesFetched: false + currentBufferLinesFetched: false }; export const app = createReducer(initialState, (builder) => { builder.addCase(disconnectAction, (state) => { return { ...state, - connected: false + connected: false, + currentBufferLinesFetched: false }; }); builder.addCase(fetchVersionAction, (state) => { @@ -41,30 +42,29 @@ export const app = createReducer(initialState, (builder) => { builder.addCase(changeCurrentBufferAction, (state, action) => { return { ...state, - currentBufferId: action.payload + currentBufferId: action.payload, + currentBufferLinesFetched: false }; }); builder.addCase(fetchLinesAction, (state, action) => { return { ...state, - notificationBufferLinesFetched: - (state.notification && - action.payload[0].buffer === state.notification.bufferId) || - state.notificationBufferLinesFetched + currentBufferLinesFetched: + (state.currentBufferId && + action.payload[0].buffer === state.currentBufferId) || + state.currentBufferLinesFetched }; }); builder.addCase(bufferNotificationAction, (state, action) => { return { ...state, - notification: action.payload, - notificationBufferLinesFetched: false + notification: action.payload }; }); builder.addCase(clearBufferNotificationAction, (state) => { return { ...state, - notification: null, - notificationBufferLinesFetched: false + notification: null }; }); builder.addCase(bufferClosedAction, (state, action) => { diff --git a/src/usecase/App.tsx b/src/usecase/App.tsx index dc10fb2..260b281 100644 --- a/src/usecase/App.tsx +++ b/src/usecase/App.tsx @@ -159,11 +159,7 @@ class App extends React.Component { notification && notification.identifier !== prevProps.notification?.identifier ) { - if (currentBufferId !== notification.bufferId) - this.changeCurrentBuffer(notification.bufferId); - else { - this.props.fetchBufferInfo(notification.bufferId); - } + this.changeCurrentBuffer(notification.bufferId); return; } diff --git a/src/usecase/buffers/ui/Buffer.tsx b/src/usecase/buffers/ui/Buffer.tsx index 8e2f803..3455de1 100644 --- a/src/usecase/buffers/ui/Buffer.tsx +++ b/src/usecase/buffers/ui/Buffer.tsx @@ -20,6 +20,8 @@ interface Props { parseArgs: ParseShape[]; bufferId: string; fetchMoreLines: (lines: number) => void; + notificationLineId?: string; + clearNotification: () => void; } const keyExtractor = (line: WeechatLine) => @@ -55,6 +57,7 @@ const Header: React.FC = ({ bufferId, lines, fetchMoreLines }) => { interface State { nickWidth: number; + listReset: boolean; } export default class Buffer extends React.PureComponent { @@ -62,10 +65,31 @@ export default class Buffer extends React.PureComponent { linesList = React.createRef>(); - state = { - nickWidth: 0 + state: State = { + nickWidth: 0, + listReset: false }; + componentDidUpdate( + prevProps: Readonly, + prevState: Readonly + ): void { + const { notificationLineId, clearNotification } = this.props; + const { listReset } = this.state; + if ( + notificationLineId && + notificationLineId !== prevProps.notificationLineId + ) { + this.setState({ listReset: true }); + } + + if (notificationLineId && listReset && listReset !== prevState.listReset) { + this.scrollToLine(notificationLineId); + clearNotification(); + this.setState({ listReset: false }); + } + } + onCellLayout?: (index: number) => void; onScrollToIndexFailed = async (info: { @@ -92,7 +116,7 @@ export default class Buffer extends React.PureComponent { }); }; - scrollToLine = (lineId: string) => { + scrollToLine = async (lineId: string) => { const index = this.props.lines.findIndex( (line) => line.pointers[line.pointers.length - 1] === lineId ); @@ -148,7 +172,8 @@ export default class Buffer extends React.PureComponent { }; render() { - const { bufferId, lines, fetchMoreLines } = this.props; + const { bufferId, lines, fetchMoreLines, notificationLineId } = this.props; + const resetList = notificationLineId && !this.state.listReset; if (!this.state.nickWidth) { return ( @@ -169,8 +194,8 @@ export default class Buffer extends React.PureComponent { ({ mediaUploadOptions: state.connection.mediaUploadOptions, notification: bufferId === state.app.notification?.bufferId && - state.app.notificationBufferLinesFetched + state.app.currentBufferLinesFetched ? state.app.notification : null })); @@ -80,17 +80,6 @@ class BufferContainer extends React.Component { buffer = React.createRef(); - componentDidUpdate(prevProps: Readonly): void { - const { notification } = this.props; - if ( - notification && - notification.identifier !== prevProps.notification?.identifier - ) { - this.buffer.current?.scrollToLine(notification.lineId); - this.props.dispatch(clearBufferNotificationAction()); - } - } - handleOnFocus = () => { this.setState({ showTabButton: true @@ -210,6 +199,10 @@ class BufferContainer extends React.Component { ); }; + clearNotification = () => { + this.props.dispatch(clearBufferNotificationAction()); + }; + render() { const { bufferId, @@ -217,7 +210,8 @@ class BufferContainer extends React.Component { showTopic, lines, fetchMoreLines, - mediaUploadOptions + mediaUploadOptions, + notification } = this.props; const { textValue, showTabButton } = this.state; @@ -240,6 +234,8 @@ class BufferContainer extends React.Component { onLongPress={this.onLongPress} parseArgs={this.parseArgs} fetchMoreLines={fetchMoreLines} + notificationLineId={notification?.lineId} + clearNotification={this.clearNotification} />