Skip to content
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.

Sign up for GitHub

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

Jump to bottom

model: Make REGISTER_COMPLETE reset some state we forgot about #5613

Merged
merged 4 commits into from
Dec 29, 2022
Merged

model: Make REGISTER_COMPLETE reset some state we forgot about #5613

merged 4 commits into from
Dec 29, 2022

Conversation

chrisbobbe
Copy link
Contributor

This addresses the second item in the list at #5605:

  • REGISTER_COMPLETE should reset or replace all server data
    and server-data metadata, but it isn't in some cases:
    - server data: topics, typing (should reset)
    - server-data metadata: fetching (should reset)

Although, I've left a comment on fetchingReducer, where doing so
felt a bit funny because morally it shouldn't be necessary.

Fixes-partly: #5605

gnprice
gnprice reviewed Dec 16, 2022
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing these! Small comments below.

src/chat/__tests__/fetchingReducer-test.js Outdated
Comment on lines 60 to 73
fetchingReducer(
deepFreeze({ [HOME_NARROW_STR]: { older: false, newer: false } }),
deepFreeze({ ...eg.action.message_fetch_start, narrow: SEARCH_NARROW('some query') }),
),
).toEqual(prevState);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument to fetchingReducer is meant to be prevState, right? (In order to express that we're ignoring this action.)

src/chat/__tests__/fetchingReducer-test.js Outdated
Comment on lines 17 to 33
test('REGISTER_COMPLETE', () => {
expect(
[
{ type: MESSAGE_FETCH_START, narrow: HOME_NARROW, numBefore: 10, numAfter: 10 },
eg.action.register_complete,
].reduce(fetchingReducer, eg.baseReduxState.fetching),
).toEqual(eg.baseReduxState.fetching);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as at #5612 (comment) .

src/topics/topicsReducer.js Outdated
Comment on lines 49 to 54
case RESET_ACCOUNT_DATA:

// Reset to clear stale data; payload has no initial data for this model
case REGISTER_COMPLETE: // eslint-disable-line no-fallthrough
return initialState;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably clearer to just have two cases here:

Suggested change
case RESET_ACCOUNT_DATA:
// Reset to clear stale data; payload has no initial data for this model
case REGISTER_COMPLETE: // eslint-disable-line no-fallthrough
return initialState;
case RESET_ACCOUNT_DATA:
return initialState;
case REGISTER_COMPLETE:
// Reset to clear stale data; payload has no initial data for this model
return initialState;

It's true that the implementation of the two cases is the same; but that one line isn't much code to duplicate.

And as the comment indicates, the reason why the two different action types lead to that result are different. So it's cleanest to think about them separately, and not try to make a common abstraction they share.

src/chat/fetchingReducer.js Outdated
Comment on lines 72 to 76
// Reset just because `fetching` is server-data metadata, and we're
// resetting the server data it's supposed to apply to… but really, if
// there's an in-progress message fetch now, it's a bug, perhaps #5609.
// TODO(#5609): Remove "perhaps #5609" when that specific bug is fixed.
case REGISTER_COMPLETE: // eslint-disable-line no-fallthrough
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed at #5609 (comment) just now, I think it's actually fine for there to still be an ongoing fetch up to this point.

There's still the issue that even though we reset this state to say there's no longer an ongoing fetch, there may still be an ongoing fetch even after this point. That's #5609. But even if there is one, I think we probably don't want to be showing loading indicators for it in the UI, so it's right to be resetting state.fetching here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we do cancel the fetches, we can do a MESSAGE_FETCH_ABORT or some-such action so that state.fetching should be all false when REGISTER_COMPLETE comes along, right, what do you think?

That way it's more transparent that a given fetch has false in state.fetching because we aborted that fetch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that seems reasonable. We'll see what feels cleanest when we get to that point, I guess.

@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Revision pushe.d

gnprice
gnprice reviewed Dec 22, 2022
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good except one comment below — please go ahead and merge after fixing.

src/typing/__tests__/typingReducer-test.js
});
const prevState = typingReducer(initialState, action1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also have a line checking prevState is not equal to initialState, right? Like with fetchingReducer above.

@chrisbobbe
fetching tests [nfc]: Avoid misleading initialState name
06d2f84
@chrisbobbe
fetching tests [nfc]: Tighten code style a bit
692a881
@chrisbobbe
fetching tests [nfc]: File an ungrouped test under MESSAGE_FETCH_COMP…
d88ec6a 
…LETE
@chrisbobbe
model: Make REGISTER_COMPLETE reset some state we forgot about
77b7e9c 
This addresses the second item in the list at zulip#5605:

> - [ ] `REGISTER_COMPLETE` should reset or replace all server data
>       and server-data metadata, but it isn't in some cases:
>       - server data: `topics`, `typing` (should reset)
>       - server-data metadata: `fetching` (should reset)

Although, I've left a comment on `fetchingReducer`, where doing so
felt a bit funny because morally it shouldn't be necessary.

Fixes-partly: zulip#5605
@chrisbobbe chrisbobbe merged commit 77b7e9c into zulip:main Dec 29, 2022
@chrisbobbe
Copy link
Contributor Author

Looks good except one comment below — please go ahead and merge after fixing.

Thanks for the review! Done.

@chrisbobbe chrisbobbe deleted the pr-register branch December 29, 2022 15:39
@chrisbobbe chrisbobbe mentioned this pull request Dec 30, 2022
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

Assignees
No one assigned
Labels
P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisbobbe @gnprice