anchors 10/n: Open /near/ links at message #1566
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 12, 2025
chrisbobbe
reviewed
Jun 12, 2025
lib/model/internal_link.dart
Outdated
Comment on lines
224
to
228
| case _NarrowOperator.near: // TODO(#82): support for near | ||
| continue; | ||
| case _NarrowOperator.near: | ||
| if (nearMessageId != null) return null; | ||
| nearMessageId = int.tryParse(operand, radix: 10); |
There was a problem hiding this comment.
Could reject the link (i.e. return null) if the int.tryParse returns null, like we do for /with/sdlfkjasdlkf in the case _NarrowOperator.with_ above this
There was a problem hiding this comment.
Yeah, good thought — will do.
This is NFC as to the live app, because nothing yet passes this argument.
When the message list is truly far back in history -- for example, at first unread in the combined feed or a busy channel, for a user who has some old unreads going back months and years -- trying to scroll smoothly to the bottom is hopeless. The only way to get to the newest messages in any reasonable amount of time is to jump there. So, do that.
This will give us room to start returning additional information from parsing the URL, in particular the /near/ operand.
Fixes zulip#82. One TODO comment for this issue referred to an aspect I'm leaving out of scope for now, namely when opening a notification rather than an internal Zulip link. So I've filed a separate issue zulip#1565 for that, and this updates that comment to point there.
|
Thanks for the review! Made that tweak, and I also went through and converted the TODOs in commit messages into TODO comments pointing to #1569 or #1570. (Plus made the actual test changes in one commit, where they were simple: That way I think we can go ahead and merge this. |
|
Yep! Please do, maybe waiting till CI passes. (The range-diff looks fine to me.) |
|
Thanks! Merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #82.
This is the next round after #1515 — and completes #82, as well as bringing us close to #80. (These changes were previously sent as part of #1517.)
This branch is a draft only because of a lack of tests. The changes it does have are ready for review.
This doesn't implement the other aspect of the behavior of /near/ links, i.e. to follow the message if it's been moved to another conversation. That's tracked as #683.