Skip to content

Commit

Permalink
Don't unnecessarily fetch lines for notification
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mhoran committed Apr 13, 2024
1 parent 27b09f4 commit d19657f
Show file tree
Hide file tree
Showing 7 changed files with 164 additions and 102 deletions.
59 changes: 1 addition & 58 deletions __tests__/usecase/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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(
<App
disconnect={() => {}}
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();
});
});
});
1 change: 1 addition & 0 deletions __tests__/usecase/buffers/ui/Buffer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ describe(Buffer, () => {
parseArgs={[]}
bufferId={''}
fetchMoreLines={() => {}}
clearNotification={() => {}}
/>
);

Expand Down
117 changes: 109 additions & 8 deletions __tests__/usecase/buffers/ui/BufferContainer.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@
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 {
bufferNotificationAction,
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,
Expand All @@ -40,7 +44,97 @@ describe('BufferContainer', () => {
}
});

Buffer.prototype.scrollToLine = jest.fn();
render(
<BufferContainer
bufferId={bufferId}
showTopic={false}
sendMessage={() => {}}
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(
<BufferContainer
Expand All @@ -56,16 +150,19 @@ describe('BufferContainer', () => {
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([
{
Expand All @@ -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();
});
});
24 changes: 12 additions & 12 deletions src/store/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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) => {
Expand Down
6 changes: 1 addition & 5 deletions src/usecase/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,7 @@ class App extends React.Component<Props, State> {
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;
}
Expand Down
Loading

0 comments on commit d19657f

Please sign in to comment.