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

kuv2707 · 2024-03-14T16:45:05Z

Pasting a link to the same zulip server transforms it to the #**stream>topic** syntax.
The link may or may not contain topic, but must contain a stream (else it is unaffected).
Ctrl+Z replaces the transformed link with the original pasted url.
The feature works even when only the stream id is present in the pasted url, and not the name.
Invalid urls are not transformed (a url with valid syntax but invalid stream id is considered invalid too).

Fixes #29136

CZO discussion

Screenshots and screen captures

Sample input:

Compose box:

After being sent:

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.

web/shared/src/internal_url.ts

web/src/copy_and_paste.js

web/shared/src/internal_url.ts

web/src/hash_util.ts

timabbott · 2024-03-15T18:30:16Z

@kuv2707 posted a next comment; as a side note, you should always post a top-level comment with a detailed update on the changes when you're ready for a next review on something; that habit is super helpful in getting timely reviews on you work. (GitHub doesn't notify anyone if you just push updates, so it's by chance that I came back to this PR today)

kuv2707 · 2024-03-15T19:25:41Z

Got it.

alya · 2024-03-20T06:47:11Z

@kuv2707 As @timabbott noted, you need to comment that your PR is ready for review here, not just on CZO.

alya · 2024-03-20T06:51:33Z

These changes incorrectly process message near: links into invalid syntax:

alya · 2024-03-20T06:56:31Z

Works for me otherwise! I like how this feature feels.

kuv2707 · 2024-03-20T07:18:35Z

I haven't dealt with :near links yet. Won't be a big issue I guess.

kuv2707 · 2024-03-20T07:20:11Z

@kuv2707 As @timabbott noted, you need to comment that your PR is ready for review here, not just on CZO.

Oh I'm sorry, it slipped my mind. I'll be more cautious from now onwards.

kuv2707 · 2024-03-20T13:05:37Z

Added a commit which covers near: links.
Additionally, we're now verifying whether the stream id in the pasted url corresponds to a valid stream or not.

alya · 2024-03-22T23:28:20Z

How are :near links being handled? Are they being pasted as-is without transformation?

kuv2707 · 2024-03-23T05:21:14Z

The stream and topic names are being extracted from them and then transformed.
I think it would be better to not transform them at all, to avoid having to Ctrl+Z each time we want to link to a message.

kuv2707 · 2024-03-24T08:05:31Z

near/ links are not transformed.

alya · 2024-03-25T22:19:03Z

OK, please make sure all of @timabbott 's comments above are addressed in replies, and then this PR should be ready for another review.

kuv2707 · 2024-03-26T15:24:10Z

I've addressed all the comments. The PR is ready for another review.
If the implementation is fine, we can proceed to write tests.

timabbott · 2024-04-18T00:03:50Z

web/src/copy_and_paste.js

+        const init_cursor_pos = text_area.selectionStart;
+        document.execCommand("insertText", false, text);
+        const new_cursor_pos = text_area.selectionStart;
+        text_area.setSelectionRange(init_cursor_pos, new_cursor_pos);


This use of execCommand is I think deprecated? https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand

In any case, our general policy is for any manipulation of the compose area use the compose_ui module and its text-field-edit library, and we should follow that here as well.

Sure, I'll switch to that.
There were a few places in the codebase where execCommand is still used.

kuv2707 · 2024-05-09T10:05:38Z

@timabbott I have made the suggested changes.
I was wondering whether #19873 would be a blocker for this PR because of the encoding bugs.

kuv2707 · 2024-05-11T15:43:53Z

@N-Shar-ma Can you review this PR? A review from you would be greatly helpful since this area has many contributions from you.

N-Shar-ma

Have left a comment suggesting a refactor. Also, please specify in your commit subject line that its a stream / topic url in the same organisation. not just any url.

Otherwise LGTM and tests well!

N-Shar-ma · 2024-05-13T17:13:24Z

web/src/copy_and_paste.js

+                event.stopPropagation();
+                // Try to transform copied url text to #**stream>topic** syntax
+                // if it is a valid url.
+                const text = try_stream_topic_syntax_text(trimmed_paste_text, true);


Reading this block of code, without the definition of try_stream_topic_syntax_text I wouldn't guess this function isn't pure (has side effects - inserting and selecting the url text). Also I'm not sure about testing this function as if it is pure.

I suggest keeping this a pure function which takes in the copied url and simply returns the stream topic syntax or null. The insertion of the copied url, followed by the stream topic syntax (if needed) should be done in another step.

Sure, I've separated the text insertion part into a different function. Thanks for the input!

kuv2707 · 2024-05-14T09:58:26Z

#30071 is a blocker for this PR since we need to check whether #**stream>topic** syntax generates incorrect links here too.

N-Shar-ma · 2024-05-14T15:55:35Z

web/src/copy_and_paste.js

+                if (syntax_text) {
+                    // It is a nice touch to get back the actual pasted url on undoing.
+                    add_text_and_select(trimmed_paste_text, $textarea);
+                }


I think it'd be cleaner to insert the syntax text and return inside this block itself

Ttransforming valid stream/topic urls to the - A valid url contains a stream and optionally a topic but nothing else, and in that order. It must belong to the same origin as the Zulip server. The stream id present in the pasted url should correspond to an actual stream in the current server. - `near` links are not transformed. - Use-mention distinction is respected by not transforming a valid url if pasted using `Ctrl+Shift+V`. - No transformation occurs inside a code block. - On pressing `Ctrl+Z` after pasting, the actual pasted link is restored. Fixes zulip#29136

alya · 2024-05-16T21:27:46Z

@timabbott This PR has been reviewed/approved by @N-Shar-ma . I just tested it as well.

alya · 2024-05-16T21:29:34Z

I'm not sure how we should handle pasting into the parens in a named link [](). Currently, the URL gets converted, but perhaps it shouldn't be? Note that we're mindful of treating that syntax specially when pasting regular URLs.

kuv2707 · 2024-05-17T08:29:30Z

Currently, the user can Ctrl+Shift+V to avoid formatting, or Ctrl+Z after pasting to restore the original url.
I agree that the extra chore is not desirable.

I think we can check if we are inside the parens and decide not to format.

@alya What do you think we should do?

zulipbot added size: L area: compose (misc) labels Mar 14, 2024

timabbott reviewed Mar 14, 2024

View reviewed changes

web/shared/src/internal_url.ts Outdated Show resolved Hide resolved

timabbott reviewed Mar 14, 2024

View reviewed changes

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

timabbott reviewed Mar 14, 2024

View reviewed changes

web/shared/src/internal_url.ts Outdated Show resolved Hide resolved

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch 2 times, most recently from 0614e9a to b617509 Compare March 15, 2024 12:42

timabbott reviewed Mar 15, 2024

View reviewed changes

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

kuv2707 changed the title ~~Paste Formatting: Implement formatting of stream and topics.~~ Compose Box: Implement formatting of stream and topics. Mar 15, 2024

kuv2707 changed the title ~~Compose Box: Implement formatting of stream and topics.~~ Compose Box: Implement formatting of stream and topics on paste Mar 15, 2024

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch 3 times, most recently from f83920c to 68629ae Compare March 15, 2024 20:51

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch 2 times, most recently from 1009ef6 to 3527ff6 Compare March 22, 2024 17:22

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch from 3527ff6 to 86a8e7d Compare March 24, 2024 08:04

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch from 86a8e7d to 24f50be Compare March 28, 2024 10:50

timabbott reviewed Apr 18, 2024

View reviewed changes

zulipbot added the has conflicts label May 2, 2024

timabbott removed the integration review Added by maintainers when a PR may be ready for integration. label May 2, 2024

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch from 4d13458 to 3f75458 Compare May 9, 2024 08:48

zulipbot removed the has conflicts label May 9, 2024

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch 3 times, most recently from 3201f66 to 73126b9 Compare May 9, 2024 10:04

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch from 73126b9 to 790f023 Compare May 11, 2024 15:40

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch from 790f023 to 319fbe0 Compare May 12, 2024 14:55

kuv2707 mentioned this pull request May 13, 2024

composebox_typeahead: Avoid generating broken links. #30071

Open

12 tasks

N-Shar-ma suggested changes May 13, 2024

View reviewed changes

kuv2707 changed the title ~~Compose Box: Implement formatting of stream and topics on paste~~ Compose Box: Implement formatting of channels and topics on paste May 13, 2024

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch 2 times, most recently from 417300b to 345918c Compare May 14, 2024 09:40

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch 2 times, most recently from fc23d64 to 76f1db4 Compare May 14, 2024 12:32

N-Shar-ma reviewed May 14, 2024

View reviewed changes

zulipbot added the has conflicts label May 15, 2024

kuv2707 force-pushed the 29136_paste_links_nice_formatting branch from 76f1db4 to f8f9d1c Compare May 16, 2024 12:14

zulipbot removed the has conflicts label May 16, 2024

N-Shar-ma approved these changes May 16, 2024

View reviewed changes

alya added the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kuv2707 commented Mar 14, 2024 •

edited

timabbott commented Mar 15, 2024

kuv2707 commented Mar 15, 2024

alya commented Mar 20, 2024

alya commented Mar 20, 2024

alya commented Mar 20, 2024 •

edited

kuv2707 commented Mar 20, 2024

kuv2707 commented Mar 20, 2024

kuv2707 commented Mar 20, 2024

alya commented Mar 22, 2024

kuv2707 commented Mar 23, 2024 •

edited

kuv2707 commented Mar 24, 2024

alya commented Mar 25, 2024

kuv2707 commented Mar 26, 2024

timabbott Apr 18, 2024

kuv2707 Apr 18, 2024

kuv2707 commented May 9, 2024

kuv2707 commented May 11, 2024

N-Shar-ma left a comment

N-Shar-ma May 13, 2024

kuv2707 May 14, 2024

kuv2707 commented May 14, 2024

N-Shar-ma May 14, 2024

kuv2707 May 16, 2024

alya commented May 16, 2024

alya commented May 16, 2024 •

edited

kuv2707 commented May 17, 2024 •

edited

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

Are you sure you want to change the base?

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

Conversation

kuv2707 commented Mar 14, 2024 • edited

timabbott commented Mar 15, 2024

kuv2707 commented Mar 15, 2024

alya commented Mar 20, 2024

alya commented Mar 20, 2024

alya commented Mar 20, 2024 • edited

kuv2707 commented Mar 20, 2024

kuv2707 commented Mar 20, 2024

kuv2707 commented Mar 20, 2024

alya commented Mar 22, 2024

kuv2707 commented Mar 23, 2024 • edited

kuv2707 commented Mar 24, 2024

alya commented Mar 25, 2024

kuv2707 commented Mar 26, 2024

timabbott Apr 18, 2024

Choose a reason for hiding this comment

kuv2707 Apr 18, 2024

Choose a reason for hiding this comment

kuv2707 commented May 9, 2024

kuv2707 commented May 11, 2024

N-Shar-ma left a comment

Choose a reason for hiding this comment

N-Shar-ma May 13, 2024

Choose a reason for hiding this comment

kuv2707 May 14, 2024

Choose a reason for hiding this comment

kuv2707 commented May 14, 2024

N-Shar-ma May 14, 2024

Choose a reason for hiding this comment

kuv2707 May 16, 2024

Choose a reason for hiding this comment

alya commented May 16, 2024

alya commented May 16, 2024 • edited

kuv2707 commented May 17, 2024 • edited

kuv2707 commented Mar 14, 2024 •

edited

alya commented Mar 20, 2024 •

edited

kuv2707 commented Mar 23, 2024 •

edited

alya commented May 16, 2024 •

edited

kuv2707 commented May 17, 2024 •

edited