search pills: Validate pills with shake animation when invalid.

web/e2e-tests/message-basics.test.ts

@@ -209,39 +209,6 @@ async function search_silent_user(page: Page, str: string, item: string): Promis
     await expect_home(page);
 }
 
-async function expect_non_existing_user(page: Page): Promise<void> {
-    await common.get_current_msg_list_id(page, true);
-    await page.waitForSelector(".empty_feed_notice", {visible: true});
-    const expected_message = "This user does not exist!";
-    assert.strictEqual(
-        await common.get_text_from_selector(page, ".empty_feed_notice"),
-        expected_message,
-    );
-}
-
-async function expect_non_existing_users(page: Page): Promise<void> {
-    await common.get_current_msg_list_id(page, true);
-    await page.waitForSelector(".empty_feed_notice", {visible: true});
-    const expected_message = "One or more of these users do not exist!";
-    assert.strictEqual(
-        await common.get_text_from_selector(page, ".empty_feed_notice"),
-        expected_message,
-    );
-}
-
-async function search_non_existing_user(page: Page, str: string, item: string): Promise<void> {
-    await page.click(".search_icon");
-    await page.waitForSelector(".navbar-search.expanded", {visible: true});
-    // Close the "in: home" pill
-    await page.click(".navbar-search .pill-close-button");
-    await common.select_item_via_typeahead(page, "#search_query", str, item);
-    // Enter to trigger search
-    await page.keyboard.press("Enter");
-    await expect_non_existing_user(page);
-    await un_narrow(page);
-    await expect_home(page);
-}
-
 async function search_tests(page: Page): Promise<void> {
     await search_and_check(
         page,
@@ -292,24 +259,6 @@ async function search_tests(page: Page): Promise<void> {
     );
 
     await search_silent_user(page, "sender:emailgateway@zulip.com", "");
-
-    await search_non_existing_user(page, "sender:dummyuser@zulip.com", "");
-
-    await search_and_check(
-        page,
-        "dm:dummyuser@zulip.com",
-        "",
-        expect_non_existing_user,
-        "Invalid user - Zulip Dev - Zulip",
-    );
-
-    await search_and_check(
-        page,
-        "dm:dummyuser@zulip.com,dummyuser2@zulip.com",
-        "",
-        expect_non_existing_users,
-        "Invalid users - Zulip Dev - Zulip",
-    );
 }
 
 async function expect_all_direct_messages(page: Page): Promise<void> {

web/src/search.ts

@@ -210,7 +210,12 @@ export function initialize({on_narrow_search}: {on_narrow_search: OnNarrowSearch
             if (e.key === "Escape" && $search_query_box.is(":focus")) {
                 exit_search({keep_search_narrow_open: false});
             } else if (keydown_util.is_enter_event(e) && $search_query_box.is(":focus")) {
-                narrow_or_search_for_term({on_narrow_search});
+                const text_terms = Filter.parse(get_search_bar_text());
+                if (text_terms.every((term) => Filter.is_valid_search_term(term))) {
+                    narrow_or_search_for_term({on_narrow_search});
+                } else {
+                    $("#search_query").addClass("shake");
+                }
             }
         });
 
@@ -287,7 +292,11 @@ function reset_searchbox(): void {
     assert(search_pill_widget !== null);
     search_pill_widget.clear();
     search_input_has_changed = false;
-    search_pill.set_search_bar_contents(narrow_state.search_terms(), search_pill_widget);
+    search_pill.set_search_bar_contents(
+        narrow_state.search_terms(),
+        search_pill_widget,
+        set_search_bar_text,
+    );
 }
 
 function exit_search(opts: {keep_search_narrow_open: boolean}): void {

web/src/search_pill.ts

@@ -1,4 +1,4 @@
-import assert from "minimalistic-assert";
+import $ from "jquery";
 
 import {Filter} from "./filter";
 import * as input_pill from "./input_pill";
@@ -39,8 +39,12 @@ type SearchPill =
 
 export type SearchPillWidget = InputPillContainer<SearchPill>;
 
-export function create_item_from_search_string(search_string: string): SearchPill {
+export function create_item_from_search_string(search_string: string): SearchPill | undefined {
     const search_terms = Filter.parse(search_string);
+    if (!search_terms.every((term) => Filter.is_valid_search_term(term))) {
+        // This will cause pill validation to fail and trigger a shake animation.
+        return undefined;
+    }
     const description_html = Filter.search_description_as_html(search_terms);
     return {
         display_value: search_string,
@@ -60,6 +64,9 @@ export function create_pills($pill_container: JQuery): SearchPillWidget {
         get_text_from_item: get_search_string_from_item,
         split_text_on_comma: false,
     });
+    // We don't automatically create pills on paste. When the user
+    // presses enter, we validate the input then.
+    pills.createPillonPaste(() => false);
     return pills;
 }
 
@@ -96,42 +103,53 @@ const user_pill_operators = new Set(["dm", "dm-including", "sender"]);
 export function set_search_bar_contents(
     search_terms: NarrowTerm[],
     pill_widget: SearchPillWidget,
-    set_search_bar_text?: (text: string) => void,
+    set_search_bar_text: (text: string) => void,
 ): void {
     pill_widget.clear();
     let partial_pill = "";
+    const invalid_inputs = [];
+
     for (const term of search_terms) {
-        if (user_pill_operators.has(term.operator) && term.operand !== "") {
-            const user_emails = term.operand.split(",");
-            const users = user_emails.map((email) => {
-                const user = people.get_by_email(email);
-                assert(user !== undefined);
-                return user;
-            });
-            append_user_pill(users, pill_widget, term.operator, term.negated ?? false);
-            continue;
-        }
         const input = Filter.unparse([term]);
+
         // If the last term looks something like `dm:`, we
         // don't want to make it a pill, since it isn't isn't
         // a complete search term yet.
         // Instead, we keep the partial pill to the end of the
         // search box as text input, which will update the
         // typeahead to show operand suggestions.
-        if (
+        if (input.at(-1) === ":" && term.operand === "" && term === search_terms.at(-1)) {
-            set_search_bar_text !== undefined &&
-            input.at(-1) === ":" &&
-            term.operand === "" &&
-            term === search_terms.at(-1)
-        ) {
             partial_pill = input;
             continue;
         }
+
+        if (!Filter.is_valid_search_term(term)) {
+            invalid_inputs.push(input);
+            continue;
+        }
+
+        if (user_pill_operators.has(term.operator) && term.operand !== "") {
+            const users = term.operand.split(",").map((email) => {
+                // This is definitely not undefined, because we just validated it
+                // with `Filter.is_valid_search_term`.
+                const user = people.get_by_email(email)!;
+                return user;
+            });
+            append_user_pill(users, pill_widget, term.operator, term.negated ?? false);
+            continue;
+        }
+
         pill_widget.appendValue(input);
     }
     pill_widget.clear_text();
-    if (set_search_bar_text !== undefined) {
+
-        set_search_bar_text(partial_pill);
+    const search_bar_text_strings = [...invalid_inputs];
+    if (partial_pill !== "") {
+        search_bar_text_strings.push(partial_pill);
+    }
+    set_search_bar_text(search_bar_text_strings.join(" "));
+    if (invalid_inputs.length) {
+        $("#search_query").addClass("shake");
     }
 }
 

web/tests/search.test.js

@@ -11,6 +11,7 @@ const search_suggestion = mock_esm("../src/search_suggestion");
 
 const search = zrequire("search");
 const search_pill = zrequire("search_pill");
+const stream_data = zrequire("stream_data");
 
 function stub_pills() {
     const $pill_container = $("#searchbox-input-container.pill-container");
@@ -25,6 +26,14 @@ set_global("getSelection", () => ({
 
 let typeahead_forced_open = false;
 
+const verona = {
+    subscribed: true,
+    color: "blue",
+    name: "Verona",
+    stream_id: 1,
+};
+stream_data.add_sub(verona);
+
 run_test("initialize", ({override, override_rewire, mock_template}) => {
     const $search_query_box = $("#search_query");
     const $searchbox_form = $("#searchbox_form");
@@ -211,7 +220,11 @@ run_test("initialize", ({override, override_rewire, mock_template}) => {
                 for (const pill of pills) {
                     pill.$element.remove = noop;
                 }
-                search_pill.set_search_bar_contents(terms, search.search_pill_widget);
+                search_pill.set_search_bar_contents(
+                    terms,
+                    search.search_pill_widget,
+                    $search_query_box.text,
+                );
             };
 
             terms = [