Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

narrow_state: Remove narrowed_to_topic function. #30186

Merged
merged 1 commit into from
May 24, 2024

Conversation

evykassirer
Copy link
Collaborator

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, so it makes sense to just call stream_sub() directly.

Fixes this bug:
https://zulip.sentry.io/issues/5367251929/events/40073ecf007a4a9798e728061a576377/?project=450455688282112

Copy link

sentry-io bot commented May 23, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: web/src/compose_closed_ui.ts

Function Unhandled Issue
get_recipient_label Error: Assertion failed get_recipient_label(src/c...
Event Count: 99 Affected Users: 47

Did you find this useful? React with a 👍 or 👎

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
@evykassirer evykassirer force-pushed the compose-closed-ui-bug-followup branch from 82db939 to 671c2e1 Compare May 23, 2024 22:20
@timabbott timabbott merged commit 825fa6f into zulip:main May 24, 2024
6 checks passed
@timabbott
Copy link
Sponsor Member

Looks good, merged, thanks @evykassirer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants