Skip to content

Commit

Permalink
narrow_state: Remove narrowed_to_topic function.
Browse files Browse the repository at this point in the history
This was only being used in one place in compose_closed_ui
to create the label for the closed composebox. But it
only checked if the `channel` and `topic` filters existed,
while `stream_sub` can return `undefined` for a few other
reasons. To ensure that we're catching the undefined sub
while also avoiding duplicate work, it makes sense to just
call `stream_sub()` directly.

Fixes this bug:
https://zulip.sentry.io/issues/5367251929/events/40073ecf007a4a9798e728061a576377/?project=450455688282112
  • Loading branch information
evykassirer committed May 23, 2024
1 parent 959c78e commit 82db939
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 20 deletions.
11 changes: 3 additions & 8 deletions web/src/compose_closed_ui.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import $ from "jquery";
import assert from "minimalistic-assert";

import * as compose_actions from "./compose_actions";
import {$t} from "./i18n";
Expand Down Expand Up @@ -28,7 +27,6 @@ export function get_recipient_label(message?: ComposeClosedMessage): string {
// actual message objects with fake objects containing just a
// couple fields, both those constructed here and potentially
// passed in.

if (message_lists.current === undefined) {
return "";
}
Expand All @@ -38,12 +36,9 @@ export function get_recipient_label(message?: ComposeClosedMessage): string {
// For empty narrows where there's a clear reply target,
// i.e. stream+topic or a single direct message conversation,
// we label the button as replying to the thread.
if (narrow_state.narrowed_to_topic()) {
const stream = narrow_state.stream_sub();
assert(stream !== undefined);
const stream_id = stream.stream_id;
const topic = narrow_state.topic();
assert(topic !== undefined);
const stream_id = narrow_state.stream_sub()?.stream_id;
const topic = narrow_state.topic();
if (stream_id !== undefined && topic !== undefined) {
return format_stream_recipient_label(stream_id, topic);
} else if (narrow_state.pm_ids_string()) {
const user_ids = people.user_ids_string_to_ids_array(narrow_state.pm_ids_string()!);
Expand Down
8 changes: 1 addition & 7 deletions web/src/narrow_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export function stream_sub(
return undefined;
}
const stream_operands = current_filter.operands("channel");

if (stream_operands.length !== 1) {
return undefined;
}
Expand Down Expand Up @@ -357,13 +358,6 @@ export function narrowed_by_stream_reply(current_filter: Filter | undefined = fi
return terms.length === 1 && current_filter.operands("channel").length === 1;
}

export function narrowed_to_topic(current_filter: Filter | undefined = filter()): boolean {
if (current_filter === undefined) {
return false;
}
return current_filter.has_operator("channel") && current_filter.has_operator("topic");
}

export function is_for_stream_id(stream_id: number, filter?: Filter): boolean {
// This is not perfect, since we still track narrows by
// name, not id, but at least the interface is good going
Expand Down
5 changes: 0 additions & 5 deletions web/tests/narrow_state.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ test("narrowed", () => {
assert.ok(!narrow_state.narrowed_by_reply());
assert.ok(!narrow_state.narrowed_by_pm_reply());
assert.ok(!narrow_state.narrowed_by_topic_reply());
assert.ok(!narrow_state.narrowed_to_topic());
assert.ok(!narrow_state.narrowed_by_stream_reply());
assert.equal(narrow_state.stream_sub(), undefined);

Expand All @@ -83,15 +82,13 @@ test("narrowed", () => {
assert.ok(!narrow_state.narrowed_by_reply());
assert.ok(!narrow_state.narrowed_by_pm_reply());
assert.ok(!narrow_state.narrowed_by_topic_reply());
assert.ok(!narrow_state.narrowed_to_topic());
assert.ok(narrow_state.narrowed_by_stream_reply());

set_filter([["dm", "steve@zulip.com"]]);
assert.ok(narrow_state.narrowed_to_pms());
assert.ok(narrow_state.narrowed_by_reply());
assert.ok(narrow_state.narrowed_by_pm_reply());
assert.ok(!narrow_state.narrowed_by_topic_reply());
assert.ok(!narrow_state.narrowed_to_topic());
assert.ok(!narrow_state.narrowed_by_stream_reply());

set_filter([
Expand All @@ -110,15 +107,13 @@ test("narrowed", () => {
assert.ok(!narrow_state.narrowed_by_reply());
assert.ok(!narrow_state.narrowed_by_pm_reply());
assert.ok(!narrow_state.narrowed_by_topic_reply());
assert.ok(!narrow_state.narrowed_to_topic());
assert.ok(!narrow_state.narrowed_by_stream_reply());

set_filter([["is", "starred"]]);
assert.ok(!narrow_state.narrowed_to_pms());
assert.ok(!narrow_state.narrowed_by_reply());
assert.ok(!narrow_state.narrowed_by_pm_reply());
assert.ok(!narrow_state.narrowed_by_topic_reply());
assert.ok(!narrow_state.narrowed_to_topic());
assert.ok(!narrow_state.narrowed_by_stream_reply());
});

Expand Down

0 comments on commit 82db939

Please sign in to comment.