action_sheet: Implement resolve/unresolve in topic action sheet #1301

chrisbobbe · 2025-01-24T03:04:42Z

Fixes: #744

The long-awaited resolve/unresolve button! If it's helpful, feel free to review just some of the prep commits, for a first round. I thought about sending them as a separate PR, but figured it might be helpful to see them with the context of the work they're preparing for.

Screenshots coming soon.

chrisbobbe · 2025-01-24T03:09:14Z

PIG208

Thanks for the PR! I went through all the commits before the last two. Generally this looks good to me.

f200ff9 action_sheet test [nfc]: Have showFromAppBar take messages, not message
7921790 action_sheet test: Move some logic out of checkButtons helper
271ae08 test [nfc]: Comment on where to find topic-action-sheet tests
3474058 action_sheet test [nfc]: Pull out showFromRecipientHeader helper
c918101 action_sheet test: Cut assumption of topic recipient header in topic narrow
3c40517 action_sheet test [nfc]: Pull out showFromAppBar helper
d0517df action_sheet test [nfc]: Pull out showFromInbox helper
56b9f82 action_sheet test [nfc]: Simplify some setup by using someChannel
ad3c6f6 action_sheet test [nfc]: Pull out a prepare function and reuse
3fc9405 action_sheet test: Use eg.otherUser for a sender in setupToTopicActionSheet
efe1381 action_sheet test [nfc]: Add visibilityPolicy to a prepare
1933a25 action_sheet test [nfc]: Add subscription-related params to a prepare
754caac action_sheet test [nfc]: Add zulipFeatureLevel param to a prepare
3331c25 action_sheet test [nfc]: Pull out someMessage variable
d108b96 action_sheet test [nfc]: Parameterize a prepare by channel and topic
3c1818b action_sheet test: Make some show-from-inbox setup optional in a prepare
4e92e91 action_sheet test [nfc]: Add new '{topic,message} action sheet' groups
b7ddabc action_sheet test [nfc]: Make checkButtons helper for showTopicActionSheet
41c7e3d action_sheet test: Make the app-bar topic-row finder more precise
e205d74 icons: Add resolve/unresolve-topic icons, from Figma

I also tested this manually. It works great except that #150 and #862 become more noticeable. So maybe we should prioritize handling those as well (#150 is already M5-a, #862 is currently M7). Will look at the implementation during the next round of review.

PIG208 · 2025-01-24T22:03:24Z

test/widgets/action_sheet_test.dart

+            ErrorHint(
+              'Before calling showFromInbox, ensure that [Unreads] '
+              'has an unread message in the relevant topic. ',
+            ),


The error looks useful! Would it be necessary to wrap it in the assert((){}), though, as this only gets run in tests?

Oh indeed; I think it doesn't need the assert.

PIG208 · 2025-01-24T22:27:47Z

test/widgets/action_sheet_test.dart


      Future<void> prepare({
+        ZulipStream? channel,


I think none of the callers of prepare ends up needing channel to be a specific value other than someChannel, so maybe we can get this removed?

I think I slightly prefer leaving it in, in case later tests want to use it? It would be easy to remove, but harder to add back correctly (without forgetting e.g. to use effectiveChannel everywhere instead of just channel, which the analyzer wouldn't help with because they're both in scope).

PIG208 · 2025-01-24T22:33:23Z

test/widgets/action_sheet_test.dart

+      bool? isChannelMuted,
+      UserTopicVisibilityPolicy? visibilityPolicy,
+      UnreadMessagesSnapshot? unreadMsgs,
+      int? zulipFeatureLevel,


I agree that these will potentially become useful as we add more. Maybe at some point it will be helpful to extract some of this to test/example_data.dart as a helper to set up global store, along with some invariant checks. Out of scope for this PR, though.

chrisbobbe · 2025-01-24T23:12:01Z

Thanks for the review!

I also tested this manually. It works great except that #150 and #862 become more noticeable.

Is #150 the issue you meant? That issue is closed as fixed.

gnprice

Thanks! Two comments from a high-level skim. One comment below, and:

Let's make a first PR have the series of test refactors, and then a second PR on top have the other commits. I think that'll be a good split.

(Sending both PRs at once can be a good way to handle the point you mentioned about seeing the context of how the early changes end up getting used.)

gnprice · 2025-01-24T23:13:33Z

lib/widgets/inbox.dart

+  static InboxPageState ancestorOf(BuildContext context) {
+    final state = context.findAncestorStateOfType<_InboxPageState>();
+    assert(state != null, 'No InboxPageBody ancestor');
+    return state!;
+  }


As we discussed just now in the office, the problem this is solving makes sense but I'd like to avoid having to add these for all the different page widgets when there isn't another reason for them. And one way to handle that generically would be:

make a marker widget with a name like PageRoot — can even be an inherited widget

put that in AccountPageRouteMixin.buildPage, inside the PerAccountStoreWidget and just outside the child widget built for the page contents

also put that in _MessageListPageState.build, around everything

then just look up the ancestor widget of that type.

PIG208 · 2025-01-24T23:19:59Z

Is #150 the issue you meant? That issue is closed as fixed.

Ah, I meant #901. The issue for updating unreads.

chrisbobbe · 2025-01-24T23:36:01Z

Let's make a first PR have the series of test refactors, and then a second PR on top have the other commits. I think that'll be a good split.

Sure! I've just sent #1306 with the test refactors, revised after Zixuan's review above.

PIG208 · 2025-01-28T19:30:21Z

lib/widgets/action_sheet.dart

+      // The message's topic doesn't match what it was when we opened the
+      // action sheet. So either the resolve/unresolve action was already done
+      // or the topic was renamed; either way, tell the user and don't make a
+      // request.


This is a good thought. I haven't gone through the PR yet, but we don't already do this for other topic actions like changing topic visibility. Maybe that can be a follow-up issue? Some bits from this if-statement might be reusable in that case.

Added a comment about this.

Zixuan points out that this could be helpful here: zulip#1301 (comment) This probably makes sense to not do until zulip#1078, when we'll have separate buttons for each of the four possible user-topic states. This same comment *could* be added on various other action-sheet buttons. But realistically it wouldn't make a difference on any that we've implemented so far: all the others (besides this and ResolveUnresolveButton) are on the message action sheet, and they're all about actions that would only be done by the self-user -- starring, marking as unread, etc. -- so they can't be done without the user's awareness, and will rarely be done anyway because you'd need to do it on a different client.

chrisbobbe · 2025-01-29T01:35:36Z

Thanks for the reviews! Revision pushed.

PIG208

Thanks for the update! This looks promising. Posted some comments about certain edge cases, and a question on the implementation of PageRoot.

PIG208 · 2025-01-29T19:46:15Z

lib/widgets/message_list.dart

-          ])));
+    // The Builder enables MessageListPage.ancestorOf
+    // on the context provided to the message action sheet by PageRoot.
+    return Builder(builder: (context) => PageRoot(context: context,


PageRoot sounds like a neat concept!

If I understand it correctly, MessageListPage is already an ancestor of PageRoot, so it is a bit confusing that MessageListPage.ancestorOf wouldn't work for PageRoot. After reading it for a bit, it makes sense to me that the context in the build method's scope doesn't have that information.

Does having a PageRootWidget and a _PageRootInheritedWidget work? I think that does not require a builder and gives us access to the context with have MessageListPage as an ancestor.

Or perhaps something like:

class PageRoot extends InheritedWidget { const PageRoot({super.key, required super.child}); @override bool updateShouldNotify(covariant PageRoot oldWidget) => !identical(oldWidget, this); static BuildContext contextOf(BuildContext context) { context.dependOnInheritedWidgetOfExactType<PageRoot>(); final element = context.getElementForInheritedWidgetOfExactType<PageRoot>(); assert(element != null, 'No PageRoot ancestor'); return element!; } }

which should build dependency the same way as we did before, with an additional constant factor of time spent on lookup. I didn't find a more convenient method that handles the dependency, sets _hadUnsatisfiedDependencies, but returns an element instead.

@override T? dependOnInheritedWidgetOfExactType<T extends InheritedWidget>({Object? aspect}) { assert(_debugCheckStateIsActiveForAncestorLookup()); final InheritedElement? ancestor = _inheritedElements?[T]; if (ancestor != null) { return dependOnInheritedElement(ancestor, aspect: aspect) as T; } _hadUnsatisfiedDependencies = true; return null; } @override InheritedElement? getElementForInheritedWidgetOfExactType<T extends InheritedWidget>() { assert(_debugCheckStateIsActiveForAncestorLookup()); return _inheritedElements?[T]; }

PageRoot sounds like a neat concept!

Yeah! Greg's idea: #1301 (comment)

After talking with him in the office just now, the conclusion is that we don't need contextOf to add a build dependency (with something like dependOnInheritedWidgetOfExactType).

So just getElementForInheritedWidgetOfExactType should suffice.

PIG208 · 2025-01-29T19:46:58Z

lib/widgets/page.dart

-      child: super.buildPage(context, animation, secondaryAnimation));
+      // The Builder enables PerAccountStoreWidget.of
+      // on the context provided by PageRoot (e.g. to action-sheet buttons).
+      child: Builder(builder: (context) => PageRoot(context: context,


Similarly, PageRootWidget/PageRoot would parallel PerAccountStoreWidget.

PIG208 · 2025-01-29T20:34:13Z

lib/api/model/model.dart

@@ -695,6 +695,12 @@ extension type const TopicName(String _value) {
  /// The key to use for "same topic as" comparisons.
  String canonicalize() => apiName.toLowerCase();

+  /// Whether the topic starts with [resolvedTopicPrefixRegexp].
+  bool get isResolved => resolvedTopicPrefixRegexp.hasMatch(_value);


The web app has a different implementation for this, that uses only startsWith:

https://github.com/zulip/zulip/blob/1fac997338e63a76c8223b8bdd009521828ed93c/web/shared/src/resolved_topic.ts#L14-L16

Effectively they should get the same result, but I assume that startsWith will be a tiny bit faster.

Similar discussion: #1242 (comment). The pros vs. cons you mentioned there are helpful in the context of topicMoveWasResolveOrUnresolve, but for isResolved maybe startsWith without the regex is sufficient for correctness.

However, isResolved is not in a hot codepath, so performance doesn't matter that much (unless it starts getting used in one).

Indeed, thanks for the catch!!

PIG208 · 2025-01-29T20:50:01Z

lib/widgets/action_sheet.dart

+    final store = PerAccountStoreWidget.of(pageContext);
+    final message = store.messages[someMessageIdInTopic] as StreamMessage?;
+
+    if (message != null && !message.topic.isSameAs(topic)) {


This is a bit of an edge case:

Open the action sheet from a recipient header so that the first message was picked at that time as someMessageIdInTopic

Move all but that message with propagate_mode=change_after
(the MessageListPage follows the move to the new topic narrow)

Click the resolve button
(the old topic gets resolved)

I'm not entirely sure what the right fix for this would be. We could leave a note and make this a follow-up. Potentially we can just dismiss the topic action sheet if such a move happens.

This sounds very uncommon.

One thought that this case prompts: while this revision correctly shows these error dialogs when they apply:

'This topic is already marked as unresolved.'

'This topic is already marked as resolved.'

'This topic has been renamed.'

it's missing a condition for an error dialog like:

'One or more messages in this topic were moved.'

That's a valid thing that can happen, and in principle perhaps it matters as much as the other cases do. But it's not practical to implement fully correctly, because we'd need to check each message with the old topic, and we don't have all that message data. We don't have that data (and might not want to fetch it) for good reasons; it could be a huge number of messages.

In your repro recipe, it seems like the unexpected behavior (from the user's view) doesn't involve any messages being set to a topic the user didn't intend, right? The old (not-moved) message(s) get their topic set appropriately, and the later (moved) messages don't. But the user is now looking at the later messages (or might be—see #1008), and they can choose to resolve that new topic too if they want to.

The old (not-moved) message(s) get their topic set appropriately, and the later (moved) messages don't.

This was unexpected for me because I though the topic action sheet (when opened from a MessageListPage) refers to the topic of the active MessageListPage, whether it has been moved or not.

So anything that happens as a result of an interaction with the action sheet is immediately visible to the user. In this case, that happens in background, on a topic that is no longer on screen.

Hmm, I see; the change of context (changing the narrow in response to the bulk-move) makes things confusing at the moment the action completes, because the effect isn't visible in the new context.

PIG208 · 2025-01-29T20:54:41Z

test/widgets/action_sheet_test.dart

+        checkRequest(message.id, '✔ zulip');
+      });
+
+      testWidgets('unresolve: happy path', (tester) async {


Let's have a test for unresolving something like ✔ ✔topic.

PIG208 · 2025-01-29T20:56:46Z

test/widgets/action_sheet_test.dart

+        connection.prepare(json: UpdateMessageResult().toJson());
+        await tester.tap(findButtonForLabel('Mark as resolved'));
+        await tester.pumpAndSettle();
+        checkNoErrorDialog(tester);


nit: spacing for this test is inconsistent with others

Zixuan points out that this could be helpful here: zulip#1301 (comment) This probably makes sense to not do until zulip#1078, when we'll have separate buttons for each of the four possible user-topic states. This same comment *could* be added on various other action-sheet buttons. But realistically it wouldn't make a difference on any that we've implemented so far: all the others (besides this and ResolveUnresolveButton) are on the message action sheet, and they're all about actions that would only be done by the self-user -- starring, marking as unread, etc. -- so they can't be done without the user's awareness, and will rarely be done anyway because you'd need to do it on a different client.

chrisbobbe · 2025-01-30T02:41:05Z

Thanks for the review, revision pushed!

PIG208

Thanks for the revision! Looks good to me except one more nit.

PIG208 · 2025-01-30T02:52:06Z

assets/l10n/app_en.arb

+  },
+  "topicRenamedMessage": "This topic has been renamed.",
+  "@topicRenamedMessage": {
+    "description": "Error title when trying to 'Mark as resolved' or 'Mark as unresolved' a topic that was renamed by someone else."


nit: "Error title" -> "Error message"

Zixuan points out that this could be helpful here: zulip#1301 (comment) This probably makes sense to not do until zulip#1078, when we'll have separate buttons for each of the four possible user-topic states. This same comment *could* be added on various other action-sheet buttons. But realistically it wouldn't make a difference on any that we've implemented so far: all the others (besides this and ResolveUnresolveButton) are on the message action sheet, and they're all about actions that would only be done by the self-user -- starring, marking as unread, etc. -- so they can't be done without the user's awareness, and will rarely be done anyway because you'd need to do it on a different client.

chrisbobbe · 2025-01-30T03:07:22Z

Thanks! Revision pushed, and I see you've marked it for Greg's review. Highlighting the above thread for Greg in case he has thoughts: #1301 (comment)

gnprice

Thanks both! Comments below.

About to switch to another task, so posting this without having yet read the tests or the small last commit.

gnprice · 2025-01-30T19:17:41Z

lib/widgets/action_sheet.dart

  final isComposeBoxOffered = messageListPage.composeBoxController != null;

  final isMessageRead = message.flags.contains(MessageFlag.read);
  final markAsUnreadSupported = store.connection.zulipFeatureLevel! >= 155; // TODO(server-6)
  final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead;

  final optionButtons = [
-    ReactionButtons(message: message, pageContext: context),
-    StarButton(message: message, pageContext: context),
+    ReactionButtons(message: message, pageContext: pageRootContext),


nit: let's just call this "pageContext" — it's shorter and matches the name we're already using

Suggested change

ReactionButtons(message: message, pageContext: pageRootContext),

ReactionButtons(message: message, pageContext: pageContext),

gnprice · 2025-01-30T19:19:37Z

lib/widgets/action_sheet.dart

-/// Must have a [MessageListPage] ancestor.
+/// Must have a [MessageListPage] ancestor and a [PageRoot] ancestor.
 void showMessageActionSheet({required BuildContext context, required Message message}) {


I think specifically this implementation needs a PageRoot ancestor that has a MessageListPage ancestor.

But given that MessageListPage inserts a PageRoot, that just means a MessageListPage ancestor, right?

gnprice · 2025-01-30T19:20:45Z

lib/widgets/message_list.dart

@@ -277,7 +277,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
            narrow: ChannelNarrow(streamId)))));
    }

-    return Scaffold(
+    return PageRoot(child: Scaffold(


This probably deserves a brief comment saying why this needs a PageRoot here. (It's so that the PageRoot context can be used to find the MessageListPageState.)

gnprice · 2025-01-30T19:22:55Z

test/widgets/action_sheet_test.dart

+      // ZulipApp instead of TestZulipApp so that we can push a route to the nav,
+      // exercising HomePage.buildRoute which (via AccountPageRouteMixin)
+      // makes a PageRoot, which the action sheet needs.
+      await tester.pumpWidget(ZulipApp());


Hmm, do we push a route to the nav in these tests? I don't see where.

Ah indeed; yeah it turned out we don't need to push a route in order to show the inbox, since it's on HomePage, which automatically appears. We still need ZulipApp instead of TestZulipApp, though, to get the PageRoot included, since that happens via HomePage.buildRoute (called in ZulipApp's onGenerateInitialRoutes).

I think it'd make sense to have TestZulipApp provide a PageRoot too. It's setting home, which is basically a simplified version of the job that onGenerateInitialRoutes does.

Sure, seems reasonable.

gnprice · 2025-01-30T19:29:59Z

lib/widgets/action_sheet.dart

+    required super.pageContext,
+  }) : _actionIsResolve = !topic.isResolved;
+
+  final bool _actionIsResolve;


nit: order this private field after the public fields that appear in the constructor

gnprice · 2025-01-30T19:38:01Z

lib/widgets/action_sheet.dart

+    if (message != null && !message.topic.isSameAs(topic)) {
+      // TODO also interrupt on a channel move, with a specific message?
+
+      // The message's topic doesn't match what it was when we opened the
+      // action sheet. So either the resolve/unresolve action was already done,
+      // the topic was renamed, or at least one message in it including
+      // someMessageIdInTopic was moved. In any case, tell the user and don't
+      // make a request.


What happens if we leave out this whole conditional, and this situation arises?

Whatever happens then is the same thing that will happen in this version if the user is a bit quicker to hit the resolve/unresolve button (so that they do so before the event reaches the client to say the message was moved), or if the other move happens a bit later (ditto). So we'll need to be sure something reasonable happens in that case.

We proceed with the request, and then it's up to the server how to handle it.

On CZO, when I let a second resolve-topic request go through, I get an API error, which we show this way. I think it's sort of reasonable, just not as polished/user-friendly:

I produced this by commenting out this whole conditional, also commenting out the line in ActionSheetMenuItemButton._handlePressed that immediately closes the action sheet, and tapping "Resolve topic" twice.

Here's an example with unresolve-topic:

No such luck in the case where the topic was renamed, though. My steps were, still with this conditional commented out:

(With the topic unresolved)

Open the topic action sheet, from a recipient header in a topic narrow

Rename the topic from the web app ("Move topic" in the UI)

Wait for event to be processed in the app (topic-narrow msglist renarrows)

Tap "Resolve topic" in the action sheet

Actual: No API error, and all the messages in the post-rename topic get moved to the original topic plus the "✔ " prefix.

Better, I guess, if the server gave an API error, but I don't see how it could do that with the current request params. Maybe if it let clients say what they assume the topic is at the time of applying the update, so it could give an error if it's not a match?

Actual: No API error, and all the messages in the post-rename topic get moved to the original topic plus the "✔ " prefix.

Better, I guess, if the server gave an API error, but I don't see how it could do that with the current request params.

Hmm, yeah.

In principle that's a problem and it'd be good to solve it. I agree this is the right way to solve it:

Maybe if it let clients say what they assume the topic is at the time of applying the update, so it could give an error if it's not a match?

I'm not sure I've ever seen that happen or heard a complaint about it happening, though. So it's not a priority to solve.

(There is a related race I believe we see happen occasionally: someone replies to a topic, concurrent with it getting resolved or otherwise renamed, and their reply lands at the old name. Because that's more common — which is natural, as people reply to threads more often than they rename or resolve them — that'd be something to prioritize first if we did pursue fixing this.)

Does the web app have logic like this for trying to catch the race on the client side? I guess that'd influence how to interpret the observation that we don't seem to see the race in practice.

Does the web app have logic like this for trying to catch the race on the client side? I guess that'd influence how to interpret the observation that we don't seem to see the race in practice.

No I don't think so. There's a function message_edit.toggle_resolve_topic, which takes an old_topic_name and makes the request with the toggled version of that:

export function toggle_resolve_topic( message_id: number, old_topic_name: string, report_errors_in_global_banner: boolean, $row?: JQuery, ): void { let new_topic_name; const topic_is_resolved = resolved_topic.is_resolved(old_topic_name); if (topic_is_resolved) { new_topic_name = resolved_topic.unresolve_name(old_topic_name); } else { new_topic_name = resolved_topic.resolve_name(old_topic_name); } if ( !topic_is_resolved && onboarding_steps.ONE_TIME_NOTICES_TO_DISPLAY.has("intro_resolve_topic") ) { show_intro_resolve_topic_modal(old_topic_name, () => { do_toggle_resolve_topic( message_id, new_topic_name, topic_is_resolved, report_errors_in_global_banner, $row, ); }); onboarding_steps.post_onboarding_step_as_read("intro_resolve_topic"); return; } do_toggle_resolve_topic( message_id, new_topic_name, topic_is_resolved, report_errors_in_global_banner, $row, ); } function do_toggle_resolve_topic( message_id: number, new_topic_name: string, topic_is_resolved: boolean, report_errors_in_global_banner: boolean, $row?: JQuery, ): void { if ($row) { show_toggle_resolve_topic_spinner($row); } const request = { propagate_mode: "change_all", topic: new_topic_name, send_notification_to_old_thread: false, send_notification_to_new_thread: true, }; void channel.patch({ url: "/json/messages/" + message_id, data: request, success() { if ($row) { const $spinner = $row.find(".toggle_resolve_topic_spinner"); loading.destroy_indicator($spinner); } }, error(xhr) { if ($row) { const $spinner = $row.find(".toggle_resolve_topic_spinner"); loading.destroy_indicator($spinner); } if (xhr.responseJSON) { const {code} = z.object({code: z.string()}).parse(xhr.responseJSON); if (code === "MOVE_MESSAGES_TIME_LIMIT_EXCEEDED") { handle_resolve_topic_failure_due_to_time_limit(topic_is_resolved); return; } if (report_errors_in_global_banner) { const {msg} = z.object({msg: z.string()}).parse(xhr.responseJSON); ui_report.generic_embed_error(msg, 3500); } } }, }); }

The old_topic_name is always taken straight from a UI element (specifically an element's data-topic-name attribute). In one case, there's a round-trip to fetch a message ID when there isn't one on hand (what we call someMessageIdInTopic).

OK. What happens if (on web) a user opens the menu for a topic in the left sidebar, then lets that sit for a while, and then comes back and hits "Resolve topic"? Does that use the data the client had when they opened the menu, or the data the client has when they hit the button?

gnprice · 2025-01-30T19:40:05Z

lib/widgets/action_sheet.dart

+      String? errorMessage;
+
+      switch (e) {


nit:

Suggested change

String? errorMessage;

switch (e) {

String? errorMessage;

switch (e) {

since this switch is the block that sets this variable

Taken from Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2064-417365&m=dev https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6329-127618&m=dev

Zixuan points out that this could be helpful here: zulip#1301 (comment) This probably makes sense to not do until zulip#1078, when we'll have separate buttons for each of the four possible user-topic states. This same comment *could* be added on various other action-sheet buttons. But realistically it wouldn't make a difference on any that we've implemented so far: all the others (besides this and ResolveUnresolveButton) are on the message action sheet, and they're all about actions that would only be done by the self-user -- starring, marking as unread, etc. -- so they can't be done without the user's awareness, and will rarely be done anyway because you'd need to do it on a different client.

chrisbobbe · 2025-01-30T22:07:58Z

Thanks for the review! Revision pushed, and replied above.

gnprice · 2025-01-30T22:16:03Z

Thanks! The revision looks good modulo the two open subthreads above, and the nit #1301 (comment) . (And I haven't yet read further than before.)

Without this, the upcoming resolve/unresolve feature would have a bug where certain error dialogs would fail to appear. (Specifically the ones that explain to the user that a topic has changed between opening the action sheet and pressing the resolve/unresolve button.)

Fixes: zulip#744

Zixuan points out that this could be helpful here: zulip#1301 (comment) This probably makes sense to not do until zulip#1078, when we'll have separate buttons for each of the four possible user-topic states. This same comment *could* be added on various other action-sheet buttons. But realistically it wouldn't make a difference on any that we've implemented so far: all the others (besides this and ResolveUnresolveButton) are on the message action sheet, and they're all about actions that would only be done by the self-user -- starring, marking as unread, etc. -- so they can't be done without the user's awareness, and will rarely be done anyway because you'd need to do it on a different client.

chrisbobbe · 2025-01-30T23:13:51Z

Thanks! Revision pushed.

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jan 24, 2025

chrisbobbe requested a review from PIG208 January 24, 2025 03:04

chrisbobbe assigned PIG208 Jan 24, 2025

PIG208 mentioned this pull request Jan 24, 2025

Handle moved messages in unreads and recent-senders data #901

Open

2 tasks

PIG208 reviewed Jan 24, 2025

View reviewed changes

gnprice reviewed Jan 24, 2025

View reviewed changes

chrisbobbe mentioned this pull request Jan 24, 2025

Mark as resolved/unresolved prep 2 #1306

Merged

PIG208 reviewed Jan 28, 2025

View reviewed changes

chrisbobbe force-pushed the pr-resolve-topic branch from 5c7b14f to faa4707 Compare January 29, 2025 01:35

PIG208 reviewed Jan 29, 2025

View reviewed changes

chrisbobbe force-pushed the pr-resolve-topic branch from faa4707 to de9205e Compare January 30, 2025 02:41

PIG208 reviewed Jan 30, 2025

View reviewed changes

PIG208 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 Jan 30, 2025

PIG208 requested a review from gnprice January 30, 2025 02:57

PIG208 assigned gnprice and unassigned PIG208 Jan 30, 2025

chrisbobbe force-pushed the pr-resolve-topic branch from de9205e to 8fefcbd Compare January 30, 2025 03:06

gnprice reviewed Jan 30, 2025

View reviewed changes

icons: Add resolve/unresolve-topic icons, from Figma

fb25c21

Taken from Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2064-417365&m=dev https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6329-127618&m=dev

chrisbobbe force-pushed the pr-resolve-topic branch from 8fefcbd to ad5723d Compare January 30, 2025 22:07

chrisbobbe added 3 commits January 30, 2025 15:06

action_sheet: Implement resolve/unresolve in topic action sheet

a9edcd4

Fixes: zulip#744

chrisbobbe force-pushed the pr-resolve-topic branch from ad5723d to 79e0108 Compare January 30, 2025 23:13

PIG208 mentioned this pull request Jan 30, 2025

Support general chat #1297

Open

	ReactionButtons(message: message, pageContext: pageRootContext),
	ReactionButtons(message: message, pageContext: pageContext),

action_sheet: Implement resolve/unresolve in topic action sheet #1301

Are you sure you want to change the base?

action_sheet: Implement resolve/unresolve in topic action sheet #1301

Conversation

chrisbobbe commented Jan 24, 2025

chrisbobbe commented Jan 24, 2025 • edited Loading

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 24, 2025 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jan 24, 2025 • edited Loading

chrisbobbe commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 29, 2025

PIG208 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 30, 2025

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 30, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 30, 2025

gnprice commented Jan 30, 2025

chrisbobbe commented Jan 30, 2025

chrisbobbe commented Jan 24, 2025 •

edited

Loading

chrisbobbe commented Jan 24, 2025 •

edited

Loading

PIG208 commented Jan 24, 2025 •

edited

Loading

PIG208 left a comment •

edited

Loading

PIG208 Jan 29, 2025 •

edited

Loading

PIG208 Jan 29, 2025 •

edited

Loading

chrisbobbe Jan 30, 2025 •

edited

Loading

PIG208 Jan 30, 2025 •

edited

Loading

chrisbobbe Jan 30, 2025 •

edited

Loading

chrisbobbe Jan 30, 2025 •

edited

Loading