compose: Show a Refresh/Subscribe banner when unsubscribed and can't send #1873

chrisbobbe · 2025-09-25T23:21:37Z

Stacked atop #1871.

This fixes part of the "second buggy behavior" that I described
in #1789: specifically, it fixes that behavior when the self-user
doesn't have permission to send messages in the channel.

Later, for the case where there is permission to send messages, we
should use a different banner label, like "Replies to your messages
will not appear automatically." I think it's smoothest not to touch
that case until we've fixed the "first" and "third" parts of #1798,
because those are bugs in the send-message experience.

Fixes-partly: #1798

chrisbobbe · 2025-09-25T23:23:40Z

Before (well, after #1871)	After

As we do for e.g. ZulipWebUiKitButton.

…iling

Instead of as a base class with subclasses _ErrorBanner and _EditMessageBanner.

Fixes zulip#1870.

rajveermalviya

Thanks @chrisbobbe! LGTM and tests great. Moving over to Greg's review.

(I also see that this needs a rebase, as the base PR has been update with new commits).

We're about to use this more; seems a good time to write a test.

…send This fixes part of the "second buggy behavior" that I described in zulip#1789: specifically, it fixes that behavior when the self-user doesn't have permission to send messages in the channel. Later, for the case where there *is* permission to send messages, we should use a different banner label, like "Replies to your messages will not appear automatically." I think it's smoothest not to touch that case until we've fixed the "first" and "third" parts of zulip#1798, because those are bugs in the send-message experience. Fixes-partly: zulip#1798

chrisbobbe · 2025-10-01T22:40:53Z

I also see that this needs a rebase

Done.

gnprice

Thanks for building this! Comments below.

Should also get a UX review from Alya (unless there already was one in a chat thread, not sure).

gnprice · 2025-10-02T00:00:30Z

test/model/message_list_test.dart

+      // …do something unrelated…
+      connection.prepare(json: olderResult(
+        anchor: 1000, foundOldest: false,
+        messages: List.generate(100,
+          (i) => eg.streamMessage(id: 900 + i, stream: channel)),
+      ).toJson());
+      await model.fetchOlder();
+      checkNotified(count: 2);


What's the role of this step in the narrative of the test? It feels like it'd all work the same way without this.

gnprice · 2025-10-02T00:03:07Z

test/model/message_list_test.dart

+      model.renarrowAndFetch(newNarrow, newAnchor);
+      checkNotifiedOnce();
+      check(model)
+        ..generation.equals(generationBefore + 1)


Ideally the generation would be an implementation detail of the view-model, not something a test inspects.

I think the substance of this check can be gotten by having a fetchOlder call like above, but delaying its result to come in after the renarrowAndFetch (and before the latter fetch completes). Then we can check that after the time the fetchOlder result would have arrived, the list is still empty.

gnprice · 2025-10-02T00:09:56Z

lib/widgets/compose_box.dart

+          // TODO better to refetch around some visible message if any
+          MessageListPage.ancestorOf(pageContext).refresh(AnchorCode.newest);


This TODO appears at both call sites of the new MessageListPageState.refresh. It seems like functionality that naturally belongs inside that method — that widget is closer to the responsibility for managing which messages are visible than these buttons are.

gnprice · 2025-10-02T00:11:40Z

test/widgets/compose_box_test.dart

        });
+      }
+
+      doTestComposeBoxShown(


nit: "do" seems redundant; and this calls testWidgets, so corresponds to one test case, so it's helpful to use the same "testFoo" naming

gnprice · 2025-10-02T00:20:02Z

lib/widgets/compose_box.dart

            byDate: DateTime.now())) {
-          return _ErrorBanner(getLabel: (zulipLocalizations) =>
-            zulipLocalizations.errorBannerCannotPostInChannelLabel);
+          return (channel == null || channel is Subscription)


This condition is a bit puzzling — it combines the two extremes of a spectrum of three cases, treating them the same yet differently from the middle case. Is that the intended behavior?

gnprice · 2025-10-02T00:29:32Z

test/widgets/compose_box_test.dart

+      doTestComposeBoxShown(
+        narrow: topicNarrow,
+        isChannelSubscribed: false,
+        channelPostPolicy: ChannelPostPolicy.administrators,
+        selfUserRole: UserRole.moderator,
+        expected: false);


This is kind of a lot of similar test cases — I think it makes it harder to see the overall picture.

Do we need 4 cases of a topic narrow? I think just one suffices, exercising a banner appearing instead of the compose box. That shows that we haven't forgotten to wire up topic narrows to this logic.

gnprice · 2025-10-02T00:31:47Z

test/widgets/compose_box_test.dart

+        channelPostPolicy: ChannelPostPolicy.moderators,
+        selfUserRole: UserRole.administrator,


Then I think this pair of params could be summarized as a boolean named like canSend. That's all they're expressing, right? That'd reduce the complexity when reading each of these cases, and scanning over them.

gnprice · 2025-10-02T00:40:20Z

test/widgets/compose_box_test.dart

+            delay: Duration(seconds: 1));
+          await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Refresh'));
+          await tester.pump();
+          check(store.debugMessageListViews).single


How about:

Suggested change

check(store.debugMessageListViews).single

final model = MessageListPage.ancestorOf(state.context).model;

check(model)

then reuse model below.

(Oh and I guess do this a bit above, at the first reference to the model.)

alya · 2025-10-02T23:15:10Z

The screenshots look good to me.

chrisbobbe requested a review from rajveermalviya September 25, 2025 23:21

chrisbobbe assigned rajveermalviya Sep 25, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Sep 25, 2025

chrisbobbe added 4 commits September 29, 2025 12:40

compose [nfc]: Factor banner background/label colors as intent enum

fcf01f4

As we do for e.g. ZulipWebUiKitButton.

compose [nfc]: Move PageRoot.contextOf() down into a _Banner.buildTra…

e6fd1d1

…iling

compose [nfc]: Factor _Banner as a concrete widget class with params

7628dd3

Instead of as a base class with subclasses _ErrorBanner and _EditMessageBanner.

compose: Use "info" intent, not "danger", for cannot-post-here banners

bf0b314

Fixes zulip#1870.

rajveermalviya approved these changes Sep 30, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Sep 30, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Sep 30, 2025

rajveermalviya requested a review from gnprice September 30, 2025 12:52

chrisbobbe added 7 commits October 1, 2025 15:40

msglist test: Add smoke test for renarrowAndFetch

c83688b

We're about to use this more; seems a good time to write a test.

msglist [nfc]: Add anchor param to renarrowAndFetch

097df46

msglist: Implement refresh, to be used for unsubscribed channels

03f6c5a

actions [nfc]: Move subscribe-to-channel-with-feedback to ZulipActions

2d3d4a9

compose test: In channel-post-policy tests, use subscribed channels

fb8bde9

compose_box test [nfc]: Factor some tests out of a loop structure

a9b4399

chrisbobbe force-pushed the pr-refresh-subscribe-when-cannot-send branch from b34b3a8 to 34fd9e5 Compare October 1, 2025 22:40

gnprice reviewed Oct 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

compose: Show a Refresh/Subscribe banner when unsubscribed and can't send #1873

compose: Show a Refresh/Subscribe banner when unsubscribed and can't send #1873

Uh oh!

chrisbobbe commented Sep 25, 2025 •

edited

Loading

Uh oh!

chrisbobbe commented Sep 25, 2025

Uh oh!

rajveermalviya left a comment •

edited

Loading

Uh oh!

chrisbobbe commented Oct 1, 2025

Uh oh!

gnprice left a comment

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

gnprice Oct 2, 2025

Uh oh!

alya commented Oct 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

		// TODO better to refetch around some visible message if any
		MessageListPage.ancestorOf(pageContext).refresh(AnchorCode.newest);

		channelPostPolicy: ChannelPostPolicy.moderators,
		selfUserRole: UserRole.administrator,

	check(store.debugMessageListViews).single
	final model = MessageListPage.ancestorOf(state.context).model;
	check(model)

compose: Show a Refresh/Subscribe banner when unsubscribed and can't send #1873

Are you sure you want to change the base?

compose: Show a Refresh/Subscribe banner when unsubscribed and can't send #1873

Uh oh!

Conversation

chrisbobbe commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Sep 25, 2025

Uh oh!

rajveermalviya left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Oct 1, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alya commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

chrisbobbe commented Sep 25, 2025 •

edited

Loading

rajveermalviya left a comment •

edited

Loading

alya commented Oct 2, 2025 •

edited

Loading