New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eventQueueId in appReducers #1076

Merged
merged 4 commits into from Aug 21, 2017

Conversation

Projects
None yet
2 participants
@kunall17
Contributor

kunall17 commented Aug 21, 2017

No description provided.

@borisyankov

Minor notes.

actions.appOnline(isConnected);
if (isConnected) {
if (isHydrated && !needsInitialFetch && isConnected) {

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

I don't think needsInitialFetch should be a factor.

This comment has been minimized.

@kunall17

kunall17 Aug 21, 2017

Contributor

I added there just to make sure the initial Fetch is completed!

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

Yes, but your trySendMessages does not need the initial fetch to complete.

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

Also this

state.outbox.forEach(async item => {
  await sendMessageApi(
   ...

does not need the await.

This comment has been minimized.

@kunall17

kunall17 Aug 21, 2017

Contributor

needsInitialFetch is just to make sure that dispatch(realmInit(initData)); has been called as this make's sure that queue id saved to the state!

@@ -123,12 +123,12 @@ export const fetchRestOfInitialData = (): Action => async (
if (auth.apiKey !== '' && pushToken === '') {
refreshNotificationToken();
}
dispatch(trySendMessages());

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

Why move it here?

This comment has been minimized.

@kunall17

kunall17 Aug 21, 2017

Contributor

While doing doInitialFetch the state.app.eventQueue is null hence the event which triggers EVENT_NEW_MESSAGE was not being received

@@ -19,6 +19,8 @@ export default class MessageListItem extends PureComponent {
if (type === 'time') {
return <TimeRow key={`time${timestamp}`} timestamp={timestamp} />;
} else if (message.isOutbox) {
return <MessageContainer isBrief={isBrief} message={message} />;

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

What happens if you let it be wrapped in a TaggedView?

This comment has been minimized.

@kunall17

kunall17 Aug 21, 2017

Contributor

It get's picked up by the native component and comes in the list of visibleId's here https://github.com/zulip/zulip-mobile/blob/master/src/message/MessageListContainer.js#L22 and then there's an attempt to send this as an unread id!

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

Just wanted to check. Then do a e.visibleIds.filter() just before the map instead of rendering a different component.

This comment has been minimized.

@kunall17

kunall17 Aug 21, 2017

Contributor

The function in the MessageListContainer is called very very frequently onScroll, so it can be more expensive (even though there will be very less number of outbox & visibleIds)

Is their some performance bottleneck for rendering of a different component?

This comment has been minimized.

@borisyankov

borisyankov Aug 21, 2017

Contributor

That will allow you to add tests for that too.

This comment has been minimized.

@kunall17

kunall17 Aug 21, 2017

Contributor

Test's for (like for what specific task)?

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 21, 2017

Looks good!

@borisyankov borisyankov merged commit a08d596 into zulip:master Aug 21, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage increased (+2.3%) to 48.63%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment