-
-
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
composebox_typeahead: Avoid generating broken links. #30071
base: main
Are you sure you want to change the base?
Conversation
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.
I have made a preliminary implementation and highlighted the reasons behind some technical choices.
Once we decide the correct logic to detect faulty topic names, I'll refactor the functions and put them in a sensible place.
@timabbott Do you want to take a look and provide some feedback here? |
07cab3f
to
dbcdfa0
Compare
dbcdfa0
to
0a026be
Compare
0a026be
to
ff0ba14
Compare
@timabbott Addressed the suggestion. Please take a look. |
ff0ba14
to
7455824
Compare
web/src/composebox_typeahead.ts
Outdated
@@ -1180,6 +1185,31 @@ export function content_typeahead_selected( | |||
return beginning + rest; | |||
} | |||
|
|||
function will_produce_broken_stream_topic_link(topic_name: string): boolean { | |||
return /(\*+)|(.*`.*`.*)/.test(topic_name); |
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.
The links for topic names with double backticks are now generated correctly, but the double backticks cause the enclosed part of the topic name to render as an inline code block.
I don't know what to do in this case since backslash escaping is disabled.
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.
Yeah, I think there's no great answer for that at present. This is #18202.
As long as the link works correctly, though, that's a much less severe issue than #19873 is.
A possible workaround is to use HTML character references, as suggested here: #18202 (comment)
I'd definitely leave that as a separate later commit, though.
a3c5613
to
8c243b1
Compare
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.
Thanks @kuv2707! This is an old issue people have repeatedly run into, and I'm happy to see progress being made on it.
web/src/composebox_typeahead.ts
Outdated
@@ -1180,6 +1185,31 @@ export function content_typeahead_selected( | |||
return beginning + rest; | |||
} | |||
|
|||
function will_produce_broken_stream_topic_link(topic_name: string): boolean { | |||
return /(\*+)|(.*`.*`.*)/.test(topic_name); |
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.
Yeah, I think there's no great answer for that at present. This is #18202.
As long as the link works correctly, though, that's a much less severe issue than #19873 is.
A possible workaround is to use HTML character references, as suggested here: #18202 (comment)
I'd definitely leave that as a separate later commit, though.
7704b04
to
d15f3fd
Compare
@alya Does this PR have to go through the buddy and mentor review stages? (Since it's already reviewed by maintainers once) |
I think it's OK to skip those at this stage. Has all the feedback above been addressed? |
d15f3fd
to
cbc52bb
Compare
@alya I have addresed all the feedback. Changes:
I have updated the PR description with the screenshots. I've initiated a discussion about some decisions regarding corner cases here |
cbc52bb
to
a436ce8
Compare
@timabbott I have addressed the feedback. |
web/src/composebox_typeahead.ts
Outdated
} | ||
|
||
function get_stream_name_from_topic_link_syntax(syntax: string): string { | ||
const start = syntax.lastIndexOf("#**"); |
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.
Why is this lastIndex
? This function would benefit from a comment explaining how it's intended to be used / what assumptions the caller is responsible for checking about the syntax
before using it.
Or maybe cleaner would be to just have it start with assert
statements for those claims.
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.
Both lastIndexOf
and indexOf
would work the same here. This is a remnant from when this piece of code wasn't separated into its own function. I have changed it.
web/src/composebox_typeahead.ts
Outdated
case "`": | ||
return "`"; | ||
case ">": | ||
return ">"; |
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.
Why does this have >
, when will_produce_broken_stream_topic_link
does not? Seems like a comment should document this detail.
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.
I had planned to include >
as an invalid character for stream or topic name, but doing so does not cover all the cases where >
can lead to a broken link.
- Things work fine if we select a stream from the typeahead containing
>
in the name, i.e, a fallback markdown syntax is generated. - But if we then select a topic, such that the final syntax is something like
#**stream>name>topicname**
, a broken link is generated (since we can't know which>
is for the syntax and which>
is a part of the stream/topic name).
I am adding >
in the regex for now, since it does cover some cases.
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.
A related problem is that we can't topic_jump
when a fallback stream link is generated. (initiated a discussion here).
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.
What's the failure mode? I think a reasonable theory is to leave that unsolved, if the result isn't any worse than main
, and then possibly ban >
characters in stream names as part of our solution.
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.
That seems reasonable.
I would say it is better than the current behaviour, since it at least handles stream links for stream names with problematic characters.
@timabbott What is the next step for this PR?
web/src/composebox_typeahead.ts
Outdated
}); | ||
} | ||
|
||
function url_syntax(stream_name: string, topic_name: string | undefined): string { |
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.
Why is this called url_syntax
? A function in the typeahead with this name is going to be assumed to be about something totally different.
Maybe it would also be a good idea to put all these new functions in a new topic_link_util.ts
module?
Then this could be something like topic_link_util.get_fallback_markdown_link()
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.
Putting these in a module would be a good idea, since these functions are also required in #29302
@timabbott Implemented all the changes suggested. I still don't know what to do about some corner cases (CZO) |
518d296
to
37103da
Compare
The #**stream>topic** syntax generates broken links for topics containing two backticks or ending with *, because of architectural flaws in the backend markdown processor. So we avoid generating the syntax for such topics and instead generate the normal link syntax in markdown. Fixes zulip#19873
37103da
to
f289f95
Compare
The
#**stream>topic**
syntax generates broken links for topics containing two backticks or starting/ending with *, because of architectural flaws in the backend markdown processor. So we avoid generating the syntax for such topics and instead generate the normal link syntax in markdown.Fixes #19873
CZO discussion
Screenshots and Screen Captures
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: