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

Handle moved messages in unreads data #901

Closed
2 tasks done
gnprice opened this issue Aug 21, 2024 · 4 comments · Fixed by #1311
Closed
2 tasks done

Handle moved messages in unreads data #901

gnprice opened this issue Aug 21, 2024 · 4 comments · Fixed by #1311
Assignees
@PIG208
Labels
a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) help wanted
Milestone
M5a: Launch blockers

Comments

@gnprice
Copy link
Member

gnprice commented Aug 21, 2024 •
edited
Loading

With #787 we implemented most of #150, handling moved messages. But as that PR description said:

Fixes most of #150. (After this lands we should do a quick scan to identify anything else that needs updating on message moves — there are several new data structures in the store that we've merged in recent weeks.)

Then we didn't do that scan.

Places that still need updating on message moves include:

  • Unreads (which has a TODO comment for it)
  • RecentSenders (that's one of those recently-merged data structures) → split to its own issue: Handle moved messages in recent-senders data #1460
  • … OK and I think that's it, having now scanned through what's in the store. (Another recently-merged data structure is RecentDmConversationsView, but that's only about DMs and those can't be moved.)

Related issues:
@gnprice gnprice added a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) labels Aug 21, 2024
@gnprice gnprice added this to the Beta 3: Summer 2024 milestone Aug 21, 2024
gnprice added a commit that referenced this issue Aug 21, 2024
@gnprice
unread [nfc]: Update TODO comments on moved messages, pointing to #901
4f5269d
@gnprice gnprice modified the milestones: Beta 3: Summer 2024, Launch Aug 23, 2024
@gnprice gnprice changed the title Handle moved messages, part 2 Handle moved messages in unreads and recent-senders data Nov 21, 2024
@gnprice
Copy link
Member Author

gnprice commented Dec 18, 2024

The key user-facing scenario where this comes up, which I happened to see again earlier today:

  • You're reading messages in Combined feed.
  • A topic gets resolved, containing some unread messages.
  • The messages are still unread, and shown with the blue unread marker at left. If you tap the recipient header to navigate to their conversation, the unread marker appears, as it should.
  • But the conversation doesn't have a mark-as-read button. As a result there's no way for you to dismiss those unread markers (short of marking read the entire combined feed, or entire channel).

What's happening is that the Unreads data structure still has the messages under their old, unresolved topic; so when viewing the new resolved topic, it finds no unreads for that topic.

@gnprice gnprice modified the milestones: M5: Launch, M5-a: Server 10 Jan 14, 2025
@PIG208 PIG208 self-assigned this Jan 24, 2025
@PIG208
Copy link
Member

PIG208 commented Jan 24, 2025

I will work on this next as with #1301 this scenario might come up more often.

This was referenced Jan 24, 2025
Merged
Merged
Open
@PIG208
Copy link
Member

PIG208 commented Jan 28, 2025

TypingStatus also refers to data that can be moved (TopicNarrows), but it appears that the web app doesn't move an active typing indicator even if propagate mode is 'all'. It could be a deliberate UX or a known/unknown issue. Anyway, this is pretty subtle and we shouldn't need to consider it when working on this.

PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Mar 18, 2025
@PIG208
wip; recent_senders: Handle moved messages
94e8c9b 
TODO explain implementation

Fixes: zulip#901

Signed-off-by: Zixuan James Li <[email protected]>
@PIG208 PIG208 mentioned this issue Mar 18, 2025
Draft
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Mar 18, 2025
@PIG208
recent_senders: Handle moved messages
9896329 
This is similar to how we add support to handling moves for unreads
(commit 34e6201), also with optimizations to avoid making unnecessary
copies of message IDs when the entire conversation is moved (e.g.
resolve/unresolve topic).

An alternative approach to this that we didn't take is extracting
helpers from handleMessages and handleDeleteMessageEvent and combining
the two to handle moves, like web does
(https://github.com/zulip/zulip/blob/bd04a30b/web/src/recent_senders.ts#L165-L190),
but it appears easier to create a dedicated helper to meet our
performance need.

Fixes: zulip#901
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Mar 18, 2025
@PIG208
recent_senders: Handle moved messages
adf3690 
This is similar to how we add support to handling moves for unreads
(commit 34e6201), also with optimizations to avoid making unnecessary
copies of message IDs when the entire conversation is moved (e.g.
resolve/unresolve topic).

An alternative approach to this that we didn't take is extracting
helpers from handleMessages and handleDeleteMessageEvent and combining
the two to handle moves, like web does
(https://github.com/zulip/zulip/blob/bd04a30b/web/src/recent_senders.ts#L165-L190),
but creating a dedicated helper makes it more sraightforward to
optimize for our performance need.

Fixes: zulip#901
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Mar 18, 2025
@PIG208
recent_senders: Handle moved messages
ed9452d 
The tests do not have to use PerAccountStore, but this setup makes it a
bit more integrated.

This is similar to how we add support to handling moves for unreads
(commit 34e6201), also with optimizations to avoid making unnecessary
copies of message IDs when the entire conversation is moved (e.g.
resolve/unresolve topic).

An alternative approach to this that we didn't take is extracting
helpers from handleMessages and handleDeleteMessageEvent and combining
the two to handle moves, like web does
(https://github.com/zulip/zulip/blob/bd04a30b/web/src/recent_senders.ts#L165-L190),
but creating a dedicated helper makes it more sraightforward to
optimize for our performance need.

Fixes: zulip#901
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Mar 28, 2025
@PIG208
recent_senders: Handle moved messages
e7ee8d4 
This is similar to how we add support to handling moves for unreads
(commit 34e6201), especially the optimizations to avoid making
unnecessary copies of message IDs when the entire conversation is moved
(e.g. resolve/unresolve topic).

An alternative approach to this is extracting helpers from
handleMessages and handleDeleteMessageEvent and combining the two to
handle moves, like web does
(https://github.com/zulip/zulip/blob/bd04a30b/web/src/recent_senders.ts#L165-L190).

Compared to that, creating a dedicated helper (this commit) makes it
more straightforward to optimize for our performance needs.

(The tests do not have to use PerAccountStore, but this setup makes it a
bit more integrated.)

Fixes: zulip#901
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Apr 1, 2025
@PIG208
recent_senders: Handle moved messages
c02565c 
This is similar to how we add support to handling moves for unreads
(commit 34e6201), especially the optimizations to avoid making
unnecessary copies of message IDs when the entire conversation is moved
(e.g. resolve/unresolve topic).

An alternative approach to this is extracting helpers from
handleMessages and handleDeleteMessageEvent and combining the two to
handle moves, like web does
(https://github.com/zulip/zulip/blob/bd04a30b/web/src/recent_senders.ts#L165-L190).

Compared to that, creating a dedicated helper (this commit) makes it
more straightforward to optimize for our performance needs.

(The tests do not have to use PerAccountStore, but this setup makes it a
bit more integrated.)

Fixes: zulip#901
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Apr 3, 2025
@PIG208
recent_senders: Handle moved messages
951dc30 
This is similar to how we add support to handling moves for unreads
(commit 34e6201), especially the optimizations to avoid making
unnecessary copies of message IDs when the entire conversation is moved
(e.g. resolve/unresolve topic).

An alternative approach to this is extracting helpers from
handleMessages and handleDeleteMessageEvent and combining the two to
handle moves, like web does
(https://github.com/zulip/zulip/blob/bd04a30b/web/src/recent_senders.ts#L165-L190).

Compared to that, creating a dedicated helper (this commit) makes it
more straightforward to optimize for our performance needs.

(The tests do not have to use PerAccountStore, but this setup makes it a
bit more integrated.)

Fixes: zulip#901
@gnprice gnprice mentioned this issue Apr 3, 2025
Open
@gnprice gnprice changed the title Handle moved messages in unreads and recent-senders data Handle moved messages in unreads data Apr 3, 2025
@gnprice
Copy link
Member Author

gnprice commented Apr 3, 2025

I've split out the recent-senders part of this issue as its own, M6 post-launch issue #1460, for the reasons at #1418 (comment) .

This issue is now purely about the unreads data, which was an M5a launch-blocker issue. It was fixed by #1311.

@gnprice gnprice closed this as completed Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PIG208 PIG208

Labels
a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) help wanted
Projects
Flutter app
Status: Done
Milestone
M5a: Launch blockers
Development

Successfully merging a pull request may close this issue.

Handle moved messages for unreads PIG208/zulip-flutter
2 participants
@gnprice @PIG208