composebox_typeahead: Avoid generating broken links. #30071

kuv2707 · 2024-05-13T09:28:54Z

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

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

kuv2707

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.

web/src/composebox_typeahead.js

alya · 2024-05-14T22:09:44Z

@timabbott Do you want to take a look and provide some feedback here?

web/src/composebox_typeahead.ts

kuv2707 · 2024-05-20T08:34:57Z

@timabbott Addressed the suggestion. Please take a look.

web/tests/composebox_typeahead.test.js

kuv2707 · 2024-05-20T17:57:40Z

web/src/composebox_typeahead.ts

@@ -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);


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.

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.

gnprice

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

gnprice · 2024-06-03T19:07:25Z

web/src/composebox_typeahead.ts

@@ -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);


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.

web/src/composebox_typeahead.ts

kuv2707 · 2024-06-04T17:44:59Z

@alya Does this PR have to go through the buddy and mentor review stages? (Since it's already reviewed by maintainers once)

alya · 2024-06-05T18:46:18Z

I think it's OK to skip those at this stage. Has all the feedback above been addressed?

kuv2707 · 2024-06-06T15:13:05Z

@alya I have addresed all the feedback.

Changes:

Simplified the regex.
Included $$ in the regex.
Checking both the stream name and the topic name.
Using HTML character references to escape backticks, dollars etc.

I have updated the PR description with the screenshots.

I've initiated a discussion about some decisions regarding corner cases here

web/src/composebox_typeahead.ts

kuv2707 · 2024-06-08T08:16:30Z

@timabbott I have addressed the feedback.

timabbott · 2024-06-09T23:30:51Z

web/src/composebox_typeahead.ts

+}
+
+function get_stream_name_from_topic_link_syntax(syntax: string): string {
+    const start = syntax.lastIndexOf("#**");


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.

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 renamed it.

timabbott · 2024-06-09T23:31:12Z

web/src/composebox_typeahead.ts

+            case "`":
+                return "&grave;";
+            case ">":
+                return "&gt;";


Why does this have >, when will_produce_broken_stream_topic_link does not? Seems like a comment should document this detail.

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.

A related problem is that we can't topic_jump when a fallback stream link is generated. (initiated a discussion here).

timabbott · 2024-06-09T23:33:21Z

web/src/composebox_typeahead.ts

+    });
+}
+
+function url_syntax(stream_name: string, topic_name: string | undefined): string {


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()

Putting these in a module would be a good idea, since these functions are also required in #29302

kuv2707 · 2024-06-10T07:06:16Z

@timabbott Implemented all the changes suggested. I still don't know what to do about some corner cases (CZO)

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

zulipbot added size: S area: markdown (mentions) Mentions for users/groups/stream/topic/time. bug rust community request Issues of interest to the Rust community labels May 13, 2024

kuv2707 commented May 13, 2024

View reviewed changes

web/src/composebox_typeahead.js Outdated Show resolved Hide resolved

web/src/composebox_typeahead.js Outdated Show resolved Hide resolved

kuv2707 mentioned this pull request May 14, 2024

Compose Box: Implement formatting of channels and topics on paste #29302

Open

12 tasks

alya added the integration review Added by maintainers when a PR may be ready for integration. label May 14, 2024

kuv2707 force-pushed the 19873_topic_name_fix branch from 07cab3f to dbcdfa0 Compare May 17, 2024 10:16

timabbott reviewed May 17, 2024

View reviewed changes

web/src/composebox_typeahead.ts Outdated Show resolved Hide resolved

kuv2707 force-pushed the 19873_topic_name_fix branch from dbcdfa0 to 0a026be Compare May 18, 2024 16:11

zulipbot added size: L and removed size: S labels May 18, 2024

kuv2707 force-pushed the 19873_topic_name_fix branch from 0a026be to ff0ba14 Compare May 20, 2024 08:07

kuv2707 requested a review from timabbott May 20, 2024 08:33

kuv2707 force-pushed the 19873_topic_name_fix branch from ff0ba14 to 7455824 Compare May 20, 2024 08:55

timabbott reviewed May 20, 2024

View reviewed changes

web/tests/composebox_typeahead.test.js Outdated Show resolved Hide resolved

timabbott reviewed May 20, 2024

View reviewed changes

web/tests/composebox_typeahead.test.js Outdated Show resolved Hide resolved

kuv2707 commented May 20, 2024

View reviewed changes

kuv2707 force-pushed the 19873_topic_name_fix branch 2 times, most recently from a3c5613 to 8c243b1 Compare May 21, 2024 12:16

kuv2707 requested a review from timabbott May 21, 2024 15:38

gnprice reviewed Jun 3, 2024

View reviewed changes

kuv2707 force-pushed the 19873_topic_name_fix branch 2 times, most recently from 7704b04 to d15f3fd Compare June 4, 2024 12:32

kuv2707 force-pushed the 19873_topic_name_fix branch from d15f3fd to cbc52bb Compare June 6, 2024 15:11

zulipbot removed the size: L label Jun 6, 2024

zulipbot added the size: XL label Jun 6, 2024

timabbott reviewed Jun 7, 2024

View reviewed changes

web/src/composebox_typeahead.ts Outdated Show resolved Hide resolved

timabbott reviewed Jun 7, 2024

View reviewed changes

web/src/composebox_typeahead.ts Outdated Show resolved Hide resolved

timabbott reviewed Jun 7, 2024

View reviewed changes

web/src/composebox_typeahead.ts Outdated Show resolved Hide resolved

kuv2707 force-pushed the 19873_topic_name_fix branch from cbc52bb to a436ce8 Compare June 8, 2024 08:15

kuv2707 requested a review from timabbott June 8, 2024 08:17

timabbott reviewed Jun 9, 2024

View reviewed changes

kuv2707 force-pushed the 19873_topic_name_fix branch 2 times, most recently from 65e5a3a to 518d296 Compare June 10, 2024 07:09

kuv2707 force-pushed the 19873_topic_name_fix branch from 518d296 to 37103da Compare June 10, 2024 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

composebox_typeahead: Avoid generating broken links. #30071

composebox_typeahead: Avoid generating broken links. #30071

kuv2707 commented May 13, 2024 •

edited

kuv2707 left a comment

alya commented May 14, 2024

kuv2707 commented May 20, 2024

kuv2707 May 20, 2024

gnprice Jun 3, 2024

gnprice left a comment

gnprice Jun 3, 2024

kuv2707 commented Jun 4, 2024

alya commented Jun 5, 2024

kuv2707 commented Jun 6, 2024 •

edited

kuv2707 commented Jun 8, 2024

timabbott Jun 9, 2024

kuv2707 Jun 10, 2024 •

edited

timabbott Jun 9, 2024

kuv2707 Jun 10, 2024 •

edited

kuv2707 Jun 10, 2024

timabbott Jun 9, 2024

kuv2707 Jun 10, 2024

kuv2707 commented Jun 10, 2024

composebox_typeahead: Avoid generating broken links. #30071

Are you sure you want to change the base?

composebox_typeahead: Avoid generating broken links. #30071

Conversation

kuv2707 commented May 13, 2024 • edited

kuv2707 left a comment

Choose a reason for hiding this comment

alya commented May 14, 2024

kuv2707 commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 commented Jun 4, 2024

alya commented Jun 5, 2024

kuv2707 commented Jun 6, 2024 • edited

kuv2707 commented Jun 8, 2024

Choose a reason for hiding this comment

kuv2707 Jun 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 Jun 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuv2707 commented Jun 10, 2024

kuv2707 commented May 13, 2024 •

edited

kuv2707 commented Jun 6, 2024 •

edited

kuv2707 Jun 10, 2024 •

edited

kuv2707 Jun 10, 2024 •

edited