bugfix: starred messages: Update messages in starred narrow. #943
Conversation
Abhirup-99
force-pushed
the
fix_bug_starred
branch
from
1fc4804 to
b7752ed
Compare
neiljp
added
the
PR awaiting update
Abhirup-99
force-pushed
the
fix_bug_starred
branch
from
b7752ed to
1cbfdf3
Compare
|
@zulipbot add "PR needs review" |
|
@zulipbot remove "PR awaiting update" |
|
ERROR: Label "PR awaiting feedback" does not exist and was thus not removed from this pull request. |
|
@zulipbot remove "PR awaiting update" |
zulipbot
removed
the
PR awaiting update
Abhirup-99
force-pushed
the
fix_bug_starred
branch
from
1cbfdf3 to
6e87c8d
Compare
There was a problem hiding this comment.
@Abhirup-99 I just tested this manually and the removal appears to work but I think we need to handle addition too?
Have you adjusted the tests to just pass so far? It'd be useful to have test cases for events triggering these change(s).
It's quicker to merge if you can set the commit title to follow the style we have in the rest of our git log (also see the README)
| if top_message_id in ids_to_keep: | ||
| ids_to_keep.remove(top_message_id) # update this id |
There was a problem hiding this comment.
This is to fix a race round condition when inside the current starred narrow, we unstar and remove the first message from the top, but when loading older messages, we try to remove this message_id again which is technically not present in the set anymore throwing errors.
There was a problem hiding this comment.
Update: I tested again without the if block, and found the error.
Steps to check it out:
- Remove the if block.
- Move to the topmost loaded message inside the starred_messages narrow. Ensure that new messages don't get loaded.
- Unstar it.
- Try moving up(loading older messages).
neiljp
added
PR awaiting update
Abhirup-99
force-pushed
the
fix_bug_starred
branch
3 times, most recently
from
e9930ad to
55df59e
Compare
|
@zulipbot add "PR needs review". |
|
@zulipbot remove "PR awaiting update" |
zulipbot
removed
the
PR awaiting update
* Handle starred message addition and removal events. * Fix race around condition to handle removal of last loaded message in the starred narrow without exceptions. Tests updated. Fixes zulip#940.
Abhirup-99
force-pushed
the
fix_bug_starred
branch
from
55df59e to
f0b114c
Compare
|
@neiljp please take a look when you are free. I have updated the commit message to make it more descriptive with all the individual changes I have done in this pr. Thanks :). |
Abhirup-99
changed the title
|
Heads up @Abhirup-99, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
@Abhirup-99 Thanks for working on this, this is a great bugfix to have 👍 As per my message in the stream, I've merged this manually with a few reformatting changes only 🎉 I excluded the view change for now since I think we should address this separately. |
The commit updates ZT behavior in starred message section to match the web app. The current behavior involves removing the message when the user moves out of the starred section.
Fixes #940 .