-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
narrow: Fix combined feed selecting random messages on narrow. #30062
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: web/src/narrow.js
Did you find this useful? React with a 👍 or 👎 |
for (const [id, msg_list] of rendered_message_lists) { | ||
if (id !== current.id && msg_list.data.filter.is_in_home()) { | ||
msg_list.view.$list.remove(); | ||
rendered_message_lists.delete(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the consequences of removing it from $list
and deleting it from rendered_message_lists
? Will it get added back to that structure if visited again later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, read this more, and I understand rendered_message_lists.delete
; I see that we'll potentially save this replaced message list for the combined feed. But not sure I understand the other half of what msg_list.view.$list.remove();
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Since we change `current` message list in the function, we need to decide if the
// current message list needs to be cached or discarded.
//
// If we are caching the current message list, we need to remove any other message lists
// that we have cached with the same filter.
//
// If we are discarding the current message list, we need to remove the
// current message list from the DOM.
Added this comment at the top of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but here's what I don't understand: What happens when we visit the Combined Feed at some future point after doing that rendered_message_lists.delete
operation? Does it just populate its message_list_data
object from all_messages_data
just like the very first time and end up in the same state as if we'd not done these deletions, or are there potential weirder consequences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in that case we have combined feed with first_unread
cached as we only delete when we are preserving another list.
- If user just clicks on
combined feed
in left sidebar, we narrow to first unread. - If user clicks browser back /
escape
key, we narrow with athen_select_id
. If cachedlist.msg_id_in_fetched_range(opts.then_select_id)
, then we use the cached list, otherwise we try to populate a new list like any other. That would involve looking atall_messages_data
to see if it has thethen_select_id
.
Let me work a bit on #30008 (comment) and maybe there is a better solution here we can use. |
c1f33fc
to
61b6a12
Compare
Is the next step to rework this following #30060 to reuse that code path? |
61b6a12
to
cd07904
Compare
Yeah, updated to use |
cd07904
to
f70e07b
Compare
Reproducer: * Have some unreads in the Combined feed view. * Scroll up and select a message that was not part of initial fetch. * Reload. * Go a another narrow. * Come back to combined feed to find your at a random message. This message is actually last message of the current fetch of combined feed view which was returned via `first_unread_message_id`.
f70e07b
to
b0d3e5e
Compare
Pushed to resolve conflicts. |
OK, I think this is correct; this code path feels very hard to understand, and I suspect there's a way to make it cleaner, but that can come as part of work on next transitions like #15131. |
Opened for further discussion after #30008 (comment)
#30008 (comment) still needs to be fixed.
https://chat.zulip.org/#narrow/stream/6-frontend/topic/fetching.20message.20history
Reproducer:
first_unread_message_id
.