composebox_typeahead: Convert module to typescript. #29783

evykassirer · 2024-04-18T17:54:02Z

Fully ready for review!

evykassirer · 2024-04-24T18:15:21Z

Most recent push includes a bunch of changes in test files to get all node tests passing. The next thing I'll do is probably try to separate out more prep commits, and probably read over the big change to find things I can clean up or make more legible. This is a really big change.

evykassirer · 2024-04-25T22:38:19Z

web/shared/src/typeahead.ts

@@ -44,6 +46,10 @@ type UnicodeEmoji = {
    emoji_code: string;
    reaction_type: "unicode_emoji";
    is_realm_emoji: false;
+    emoji_url?: string;


It doesn't seem like emoji_url was used in this file, but it will be used by future code that uses EmojiSuggestion

I feel like this is unlikely to be properly an optional field; either we are setting this value for every UnicodeEmoji this file interacts with, or none of them.

The logic for making this optional came from update_emoji_data -- the type EmojiDict has an optional url field, and emoji_collection which is type Emoji is pushed an object without the emoji_url when emoji_dict.is_realm_emoji is `false.

I added console.log(emoji_dict.is_realm_emoji, emoji_dict.url, emoji_dict.name) to the for loop of this function and got false undefined for all the non-realm emoji, and this for the two realm emojis in the dev environment:

true /user_avatars/2/emoji/images/1.png green_tick true /static/generated/emoji/images/emoji/unicode/zulip.png zulip

This confirms my understanding that what I put is the correct type given the way the code is currently set up.

OK there's probably something to clean up there but I'm not sure what.

evykassirer · 2024-04-25T22:45:17Z

web/src/composebox_typeahead.ts

+
+type PillItem =
+    | UserGroupPillData
+    | UserOrMentionPillData


I'm a bit torn on how to name this. I feel like they're not really pills for this typeahead, since it often just inserts text after. So I was thinking of calling them Suggestions. But then would it make sense to rename the types used in the pill typeahead to be Suggestion?

evykassirer · 2024-04-25T22:54:57Z

web/src/composebox_typeahead.ts

+    const filtered_candidates = candidates;
+    // TypeScript doesn't understand that filtering a list that is one of several times
+    // cannot return a list of mixed types.
+    // eslint-disable-next-line @typescript-eslint/consistent-type-assertions


Discussed on CZO here

evykassirer · 2024-04-25T22:55:28Z

web/src/typeahead_helper.ts

+    // TODO(evy) look into removing the delivery email check
+    if (
+        (person.type === "user" || person.is_broadcast === undefined) &&
+        Boolean(person.delivery_email)


I'd like to remove this Boolean check and just check email for every User but that actually changes the behavior of this slightly and I'm not yet sure if that's okay.

Probably worth starting a little follow-up thread on this detail.

https://chat.zulip.org/#narrow/stream/6-frontend/topic/query_matches_person.20delivery.20email

web/src/composebox_typeahead.ts

evykassirer · 2024-05-03T04:22:02Z

In the most recent push: I've now created all the prep commits I was considering creating, and this PR is fully ready for review. @andersk can you take a look?

web/shared/src/typeahead.ts

web/src/composebox_typeahead.ts

evykassirer · 2024-05-15T21:17:59Z

Test failures look unrelated. @andersk ready for your review again!

timabbott · 2024-05-16T21:33:05Z

web/src/composebox_typeahead.js

-        const val = $(this).val();
-        const recipients = typeahead_helper.get_cleaned_pm_recipients(val);
-        $(this).val(recipients.join(", "));
-    });


Just a comment on the commit message -- it's not relevant that the block was introduced 10 years ago -- most code introduced at that date does something important -- but more that it should have been removed when we migrated this input to use pills.

That makes sense, I meant to mention it should have been removed when we moved to pills. Thanks for calling this out.

timabbott · 2024-05-16T21:40:43Z

web/src/composebox_typeahead.ts

    if (!stream_id) {
        return [];
    }

    // Fetch topic history from the server, in case we will need it soon.
-    stream_topic_history_util.get_server_history(stream_id, () => {});
+    stream_topic_history_util.get_server_history(stream_id, () => {
+        /* Don't do anything with the results for now.  */


A better comment would be "We'll use these extended results in the next keypress, but we don't try to live-update what's already shown, which can be annoying if one was about to select/click a visible option."

This will be used for when we suggest emojis in the composebox typeahead.

Since #private_message_recipient is a <div contenteditable>, not an <input> or <textarea>, it doesn’t make sense to call .val() (it will always give the empty string). It looks like this line is from over 10 years ago (zulip@09deef9#diff-f2b22549dc2fb4824b37e787d964a2c9f4f580e767f86b6d7ded1d0ff37d5a62R291) and probably before we had pills here. We can just remove this whole block.

This simplifies some of the logic and will make it easier to convert to TypeScript.

timabbott · 2024-05-16T21:54:12Z

Merged, after adding a comment as suggested in #29783 (comment).

Huge thanks for slogging through this @evykassirer and @andersk!

zulipbot added the size: XL label Apr 18, 2024

evykassirer added the area: typescript migration Issues for migrating Zulip to TypeScript label Apr 18, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch 3 times, most recently from 8cdf63e to f0bc166 Compare April 24, 2024 18:12

evykassirer force-pushed the composebox-typeahead-typescript branch 2 times, most recently from 77992e6 to 2ad7836 Compare April 24, 2024 18:45

zulipbot added the has conflicts label Apr 24, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch from 2ad7836 to 3b05fbc Compare April 25, 2024 17:39

zulipbot removed the has conflicts label Apr 25, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch 2 times, most recently from 174acb7 to cf43a97 Compare April 25, 2024 22:36

evykassirer commented Apr 25, 2024

View reviewed changes

evykassirer force-pushed the composebox-typeahead-typescript branch 2 times, most recently from 2a34c60 to 831334d Compare April 25, 2024 22:43

evykassirer commented Apr 25, 2024

View reviewed changes

evykassirer force-pushed the composebox-typeahead-typescript branch from 831334d to 4fc6815 Compare April 25, 2024 22:56

evykassirer commented Apr 25, 2024

View reviewed changes

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

zulipbot added the has conflicts label Apr 30, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch from 4fc6815 to 387685e Compare May 3, 2024 04:21

zulipbot removed the has conflicts label May 3, 2024

evykassirer marked this pull request as ready for review May 3, 2024 04:22

evykassirer force-pushed the composebox-typeahead-typescript branch from 387685e to 48a25a5 Compare May 3, 2024 05:23

zulipbot added the has conflicts label May 6, 2024

timabbott reviewed May 7, 2024

View reviewed changes

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

evykassirer force-pushed the composebox-typeahead-typescript branch from 48a25a5 to 9cf02aa Compare May 9, 2024 05:03

zulipbot added the has conflicts label May 14, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch from 21318d7 to 3a90d7b Compare May 15, 2024 01:36

zulipbot removed the has conflicts label May 15, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch from 3a90d7b to 9fb6961 Compare May 15, 2024 01:39

evykassirer commented May 15, 2024

View reviewed changes

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

zulipbot added the has conflicts label May 15, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch from 9fb6961 to 0908744 Compare May 15, 2024 21:07

zulipbot removed the has conflicts label May 15, 2024

evykassirer force-pushed the composebox-typeahead-typescript branch from 0908744 to 1a1edef Compare May 15, 2024 23:39

andersk force-pushed the composebox-typeahead-typescript branch 4 times, most recently from 6c1b0a9 to 3020b1a Compare May 16, 2024 16:57

andersk approved these changes May 16, 2024

View reviewed changes

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

timabbott reviewed May 16, 2024

View reviewed changes

evykassirer added 8 commits May 16, 2024 14:51

typeahead: Create new EmojiSuggestion type.

bccdec8

This will be used for when we suggest emojis in the composebox typeahead.

emoji: Restructure EmojiDict for compatibility with typeahead.Emoji.

297fd26

emoji: Restrict reaction_type for compatibility with typeahead.Emoji.

7a9a445

composebox_typeahead tests: Create user_or_mention items directly.

e08c754

composebox_typeahead: Return mentions from get_candidates.

96abc52

composebox_typeahead: Filter and sort candidates within get_candidates.

7f9361a

This simplifies some of the logic and will make it easier to convert to TypeScript.

composebox_typeahead: Convert module to typescript.

96c9950

timabbott force-pushed the composebox-typeahead-typescript branch from 3020b1a to 96c9950 Compare May 16, 2024 21:53

timabbott merged commit 96c9950 into zulip:main May 16, 2024
5 of 6 checks passed

evykassirer deleted the composebox-typeahead-typescript branch May 16, 2024 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

composebox_typeahead: Convert module to typescript. #29783

composebox_typeahead: Convert module to typescript. #29783

evykassirer commented Apr 18, 2024 •

edited

evykassirer commented Apr 24, 2024

evykassirer Apr 25, 2024

timabbott May 7, 2024

evykassirer May 11, 2024

timabbott May 16, 2024

evykassirer Apr 25, 2024

evykassirer Apr 25, 2024

evykassirer Apr 25, 2024

timabbott May 16, 2024

evykassirer May 16, 2024

evykassirer commented May 3, 2024

evykassirer commented May 15, 2024

timabbott May 16, 2024

evykassirer May 16, 2024

timabbott May 16, 2024 •

edited

timabbott commented May 16, 2024

composebox_typeahead: Convert module to typescript. #29783

composebox_typeahead: Convert module to typescript. #29783

Conversation

evykassirer commented Apr 18, 2024 • edited

evykassirer commented Apr 24, 2024

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

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

Choose a reason for hiding this comment

evykassirer commented May 3, 2024

evykassirer commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott May 16, 2024 • edited

Choose a reason for hiding this comment

timabbott commented May 16, 2024

evykassirer commented Apr 18, 2024 •

edited

timabbott May 16, 2024 •

edited