Skip to content

internal_link: Generate new narrow links with "channel", not "stream" #1795

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
Merged
merged 1 commit into from
Aug 16, 2025
Merged

internal_link: Generate new narrow links with "channel", not "stream" #1795

merged 1 commit into from
Aug 16, 2025

Conversation

sm-sayedi
Copy link
Collaborator

Replaces #781.

Fixes: #633

@sm-sayedi sm-sayedi requested a review from chrisbobbe August 7, 2025 19:02
@sm-sayedi sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Aug 7, 2025
@sm-sayedi sm-sayedi force-pushed the issue-633-narrow-links-with-channel branch 2 times, most recently from a7dcdf3 to ba367d6 Compare August 8, 2025 06:13
chrisbobbe
chrisbobbe approved these changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! Marking for Greg's review.

@chrisbobbe chrisbobbe requested a review from gnprice August 13, 2025 01:01
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Aug 13, 2025
@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 13, 2025
gnprice
gnprice reviewed Aug 14, 2025
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 taking care of this! Generally this looks good; a few comments below.

lib/model/internal_link.dart Outdated
Comment on lines 74 to 75
case ApiNarrowChannelModern():
case ApiNarrowStream():
Copy link
Member

Choose a reason for hiding this comment

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

nit: since these are handled the same way here, they can continue to use the base class with its simpler name:

Suggested change
case ApiNarrowChannelModern():
case ApiNarrowStream():
case ApiNarrowChannel():

(There still shouldn't be an instance of directly the base class here, because the operator getter above would complain. But this code doesn't need to go out of its way to detect that.)

This differs from the ApiNarrowDm logic below, because for that one the operand looks different between the two cases.

test/api/route/messages_test.dart
Comment on lines 237 to +116
checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
{'operator': 'stream', 'operand': 12},
{'operator': 'channel', 'operand': 12},
Copy link
Member

Choose a reason for hiding this comment

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

This test change shows that the effect of this commit is a bit broader than what the commit message says:

internal_link: Generate new narrow links with "channel", not "stream"

It's also changing the get-messages requests we make to the server.

I think it's fine for that to happen in the same commit; given the way ApiNarrow works, I think splitting those changes into separate commits would require complicating the logic. But the commit message should be adjusted to make that clear.

test/model/compose_test.dart Outdated
Comment on lines 357 to 358
check(quoteAndReply(store, message: message, rawContent: 'Hello world!')).equals('''
@_**Full Name|123** [said](${eg.selfAccount.realmUrl}#narrow/stream/1-test-here/topic/some.20topic/near/${message.id}):
@_**Full Name|123** [said](${eg.selfAccount.realmUrl}#narrow/channel/1-test-here/topic/some.20topic/near/${message.id}):
Copy link
Member

Choose a reason for hiding this comment

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

Let's also have a test checking that these still use the old form when talking to an old server.

test/model/internal_link_test.dart Outdated
Comment on lines 100 to 104
test('legacy including "stream" operator', () {
checkNarrow(streamId: 1, name: 'announce', zulipFeatureLevel: 249,
'#narrow/stream/1-announce');
checkNarrow(streamId: 378, name: 'api design', zulipFeatureLevel: 249,
'#narrow/stream/378-api-design');
Copy link
Member

Choose a reason for hiding this comment

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

Most of the different cases within the existing test here (and the new 'modern including "channel" operator' test) are here to exercise things like

  • there's a topic;
  • there's a /near/ operator;
  • the channel name and/or topic have various characters that do, or might, get encoded in nontrivial ways.

Those don't really interact with the channel-vs-stream thing, though. So this "legacy" test can be cut down to just one or two examples; I think that will tell all the same story of how the legacy case differs from the modern case, without extra details that distract from that story.

@sm-sayedi sm-sayedi force-pushed the issue-633-narrow-links-with-channel branch from ba367d6 to 0d29651 Compare August 15, 2025 03:04
@sm-sayedi
Copy link
Collaborator Author

Thanks @gnprice for the review. New revision pushed.

@sm-sayedi sm-sayedi requested a review from gnprice August 15, 2025 03:05
@sm-sayedi sm-sayedi force-pushed the issue-633-narrow-links-with-channel branch 2 times, most recently from c55db48 to 7a81123 Compare August 15, 2025 06:30
gnprice
gnprice reviewed Aug 16, 2025
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! Just one nit below; otherwise all looks good.

lib/api/model/narrow.dart
@@ -93,17 +98,51 @@ sealed class ApiNarrowElement {
};
}

class ApiNarrowStream extends ApiNarrowElement {
@override String get operator => 'stream';
class ApiNarrowChannel extends ApiNarrowElement {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

This change affects few places in the app, for example, generating
internal links and sending api request with the new "channel" operator.

s/few/a few/

Perhaps confusingly, the meaning of "few" and the meaning of "a few" point in opposite directions: "few" means the number is small, while "a few" means it's larger than one or two. So "affects few places" is a way of saying "don't worry, this doesn't affect much", which isn't the point you're aiming to make here.

@sm-sayedi
api: Use "channel" operator over the legacy "stream", where supported
385fb81 
This change affects a few places in the app, for example, generating
internal links and sending api request with the new "channel" operator.

Fixes: zulip#633
@sm-sayedi sm-sayedi force-pushed the issue-633-narrow-links-with-channel branch from 7a81123 to 385fb81 Compare August 16, 2025 01:05
@sm-sayedi
Copy link
Collaborator Author

Thanks for the review and the point mentioned. Changes pushed.

@gnprice
Copy link
Member

gnprice commented Aug 16, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit 385fb81 into zulip:main Aug 16, 2025
1 check passed
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

@chrisbobbe chrisbobbe chrisbobbe approved these changes

Assignees

@gnprice gnprice

Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generate narrow links with "channel", not "stream", where supported
3 participants
@sm-sayedi @gnprice @chrisbobbe