Repository: zulip/zulip
Pull request: compose_validate: Use correct stream_id value for different context.
This PR addresses the issue of relying on `compose_state` for retrieving the `stream_id` to display warning banners. Previously, when the compose box was closed, the warnings were not shown in the message edit box. Now, we utilize the `get_stream_id_for_textarea` function to obtain the correct `stream_id` value and resolve this problem.

Fixes: #25410.

**Screenshots and screen captures:**

<details>
<summary>Warning with compose box closed</summary>

| Before | After |
|--------|--------|
| ![before](https://github.com/zulip/zulip/assets/53193850/5e0f5f17-03df-44ca-974c-95f6cbdbbd45) | ![after](https://github.com/zulip/zulip/assets/53193850/bae67cba-071a-43f8-a37c-2d2778e96508) | 
</details>

<details>
<summary>Private stream mention with compose box closed</summary>

| Before | After |
|--------|--------|
| ![before](https://github.com/zulip/zulip/assets/53193850/fa4d4092-e4bb-4e0d-8d82-8bb352ec820d) | ![after](https://github.com/zulip/zulip/assets/53193850/240ce7e1-c143-4fa6-bed7-d41fa700aad9) | 
</details>

<details>
<summary>Different context</summary>

| Before | After |
|--------|--------|
| ![before](https://github.com/zulip/zulip/assets/53193850/0820e27d-0966-4f08-9a2c-9d4d84f6782a) | ![after](https://github.com/zulip/zulip/assets/53193850/4e1b9110-1ce5-413a-87e9-f7436b130748) | 
</details>


Problem statement:
Edit message UI should use correct context for warnings
When composing a message, we give warnings for some potentially problematic situations:

1. Mentioning a private stream to an audience where not everyone is subscribed to it.
2. Mentioning a user who is not subscribed to the stream being composed to.

These warning are not triggered correctly in the message editing UI. In particular, the set of warnings shown is the ones that are applicable to the current compose box context, *not* to the context of the message being edited (which may be to a different narrow). If the compose box is closed, these warnings aren't shown at all while editing a message.

Hints from issue discussion:
Hello @zulip/server-message-view members, this issue was labeled with the "area: message-editing" label, so you may want to check it out!

<!-- areaLabelAddition -->

@shameondev Could you please add a this to your todo list, to pick up when you free up if nobody else fixes it in the meantime? It's not a release goal, but should be fixed while we're thinking about this area.
In both these cases, the core issue is that we need the call from `composebox_typeahead` into the `compose_banner` module to pass metadata about what component one is typing in, specifically the target stream, rather than having the `compose_banner` logic just look up the current subscribed stream. 

I'm not entirely sure how best to plumb this through the typeahead module; @N-Shar-ma or @aero31aero might have ideas, but worst case, `initialize_compose_typeahead` could accept a parameter that's the message ID being edited, if any, and either store that on the typeahead object or probably better just use a lambda to pass that through to `content_typeahead_selected`.

The other idea, which might be even simpler, is to just find the `.message-row` ancestor, if any, if the edit form element, and use that to look up its message ID and from there determine the stream/PM context here instead of looking at the current compose recipient.

Commit message: compose_validate: Extract the logic of getting stream_id.

Patch to review:
diff --git a/web/src/compose_validate.js b/web/src/compose_validate.js
index b7f72d16d0f98..7b7eb3717f85a 100644
--- a/web/src/compose_validate.js
+++ b/web/src/compose_validate.js
@@ -59,6 +59,16 @@ export function needs_subscribe_warning(user_id, stream_id) {
     return true;
 }
 
+function get_stream_id() {
+    const stream_name = compose_state.stream_name();
+
+    if (!stream_name) {
+        return undefined;
+    }
+
+    return stream_data.get_sub(stream_name).stream_id;
+}
+
 export function warn_if_private_stream_is_linked(linked_stream, $textarea) {
     // For PMs, we currently don't warn about links to private
     // streams, since you are specifically sharing the existence of
@@ -67,9 +77,8 @@ export function warn_if_private_stream_is_linked(linked_stream, $textarea) {
     if (compose_state.get_message_type() !== "stream") {
         return;
     }
-
-    const compose_stream = stream_data.get_sub(compose_state.stream_name());
-    if (compose_stream === undefined) {
+    const stream_id = get_stream_id();
+    if (!stream_id) {
         // We have an invalid stream name, don't warn about this here as
         // we show an error to the user when they try to send the message.
         return;
@@ -93,7 +102,7 @@ export function warn_if_private_stream_is_linked(linked_stream, $textarea) {
     // knows it exists.  (But always warn Zephyr users, since
     // we may not know their stream's subscribers.)
     if (
-        peer_data.is_subscriber_subset(compose_stream.stream_id, linked_stream.stream_id) &&
+        peer_data.is_subscriber_subset(stream_id, linked_stream.stream_id) &&
         !page_params.realm_is_zephyr_mirror_realm
     ) {
         return;
@@ -124,19 +133,13 @@ export function warn_if_mentioning_unsubscribed_user(mentioned, $textarea) {
         return; // don't check if @all/@everyone/@stream
     }
 
-    const stream_name = compose_state.stream_name();
-
-    if (!stream_name) {
-        return;
-    }
-
-    const sub = stream_data.get_sub(stream_name);
+    const stream_id = get_stream_id();
 
-    if (!sub) {
+    if (!stream_id) {
         return;
     }
 
-    if (needs_subscribe_warning(user_id, sub.stream_id)) {
+    if (needs_subscribe_warning(user_id, stream_id)) {
         const $banner_container = compose_banner.get_compose_banner_container($textarea);
         const $existing_invites_area = $banner_container.find(
             `.${CSS.escape(compose_banner.CLASSNAMES.recipient_not_subscribed)}`,
@@ -151,7 +154,7 @@ export function warn_if_mentioning_unsubscribed_user(mentioned, $textarea) {
         if (!existing_invites.includes(user_id)) {
             const context = {
                 user_id,
-                stream_id: sub.stream_id,
+                stream_id,
                 banner_type: compose_banner.WARNING,
                 button_text: can_subscribe_other_users
                     ? $t({defaultMessage: "Subscribe them"})


Code review comments:
diff_hunk: @@ -59,7 +59,16 @@ export function needs_subscribe_warning(user_id, stream_id) {
     return true;
 }
 
-function get_stream_id() {
+function get_stream_id_for_textarea($textarea) {
+    const is_in_editing_area = $textarea.closest(".message_row").length > 0;
+
+    if (is_in_editing_area) {
+        return Number.parseInt(
+            $textarea.closest(".recipient_row").find(".message_header").attr("data-stream-id"),
+            10,
+        );
path: web/src/compose_validate.js
text: @user1:
This is not great in the event that the message is a direct message -- we end up returning `NaN`, not `undefined`, which is falsey but seems like the kind of thing that would lead to bugs in the future.

I wrote a proper comment documenting this function's interface, and also changed it to return a consistent type.