Support reporting a message by chrisbobbe · Pull Request #2284 · zulip/zulip-flutter

chrisbobbe · 2026-04-21T20:59:42Z

Fixes #1972.

Screenshots:

Details

Light	Dark

chrisbobbe · 2026-04-21T21:07:39Z

Oops, the dropdown menu should have 4.0 elevation instead of 3.0. Fixed now and pushed, but four of those screenshots are out of date. Here are new ones:

Light	Dark

gnprice

(Only did a brief skim; comments from that below.)

gnprice · 2026-04-21T21:04:12Z

@@ -394,6 +394,47 @@ abstract final class ZulipAction {
      showErrorDialog(context: context, title: title, message: errorMessage);
    }
  }
+
+  /// Report a message to the server admins.


The realm admins, right?

gnprice · 2026-04-21T21:06:41Z

+        final counter = Semantics(
+          container: true,
+          liveRegion: true,
+          child: Text(
+            '$length/$maxLengthCodePoints',


Do we want to be showing the counter unconditionally? I think in many places we prefer showing it only when the input is approaching (or has reached) the limit.

Actually this is the first time we'll be showing a counter anywhere :) so there isn't a consistency constraint here.

I think it's helpful to show it unconditionally, in case the interaction goes something unlike "start typing from zero until I get warned about length", e.g. if it involves deciding whether there's room to paste in some text, like to focus attention on an excerpt when you're reporting a very long message.

I guess the counters I'm thinking of may all be in the web app — e.g. I think we have one on message content. It would definitely be annoying if we showed a persistent counter there.

I don't have a strong preference on whether to show one here.

chrisbobbe · 2026-04-24T22:53:30Z

Thanks for the skim, Greg! Revision pushed, fixing the nit and adding a comment about the counter:

          // Could show this conditionally, just as you approach the limit;
          // I lean toward always showing it in case you want to make an early
          // decision on whether to try pasting in some text, for example.

Since this is in maintainer review, I'll await @rajveermalviya's review next.

rajveermalviya

Thanks @chrisbobbe! All LGTM, small comments below.

Moving over to Greg's review.

rajveermalviya · 2026-04-27T17:32:10Z

@@ -395,7 +395,7 @@ abstract final class ZulipAction {
    }
  }

-  /// Report a message to the server admins.
+  /// Report a message to the realm admins.


report_message: Support reporting a message. …

nit: I think this change was meant to be part of a previous commit, namely:

actions: Add ZulipAction.reportMessage

?

rajveermalviya · 2026-04-27T18:10:55Z

+                _ReportTypeDropdown(
+                  selectedType: _selectedType,
+                  requestInProgress: _requestInProgress,
+                  onSelected: (value) {
+                    setState(() => _selectedType = value);
+                  }),


On web the report type defaults to "Spam", see: https://github.com/zulip/zulip/blob/7b72738eccb30ef3f818534048ffeccb4b329ffa/web/src/message_report.ts#L230

Should we follow that here?

Sure, that's reasonable. I'll have it default to the first item in server_report_message_types.

rajveermalviya · 2026-04-27T18:17:31Z

+    if (success) {
+      Navigator.pop(context);
+    } else {
+      setState(() => _requestInProgress = false);


nit (after async call):

Suggested change

setState(() => _requestInProgress = false);

if (mounted) setState(() => _requestInProgress = false);

Isn't this handled already, just above the if/else?

if (!mounted) return; if (success) { Navigator.pop(context); } else { setState(() => _requestInProgress = false); }

Ah oops. I somehow missed that line, please ignore the comment.

chrisbobbe · 2026-04-27T20:00:03Z

Thanks for the review! Revision pushed.

gnprice

Thanks for building this, and @rajveermalviya for the previous review! Various comments below; I think all are small.

I haven't tried manually playing with the UI.

gnprice · 2026-04-27T20:35:25Z

-      case (ZulipWebUiKitButtonAttention.minimal, ZulipWebUiKitButtonIntent.warning):
-      case (ZulipWebUiKitButtonAttention.low, ZulipWebUiKitButtonIntent.warning):
-        throw UnimplementedError();
-      case (ZulipWebUiKitButtonAttention.medium, ZulipWebUiKitButtonIntent.warning):
-        return designVariables.btnLabelAttMediumIntWarning;


nit: I think this is easier to read when each of the pairs is a case line in the same format, rather than some coming after an ||.

That can be arranged by saying:

Color result; switch ((attention, intent)) { // … case (ZulipWebUiKitButtonAttention.medium, ZulipWebUiKitButtonIntent.warning): result = designVariables.btnLabelAttMediumIntWarning; // … }

and then the analyzer will verify that result is always set before it gets used.

gnprice · 2026-04-27T20:37:41Z

+      titlePadding: EdgeInsets.fromLTRB(16, 12, 16, 4),
+      backgroundColor: designVariables.bgContextMenu,
+      shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(20)),
+      title: Text(title,


nit: put titlePadding next to title?

(maybe shape, then background, then title, content, actions, since shape seems like it's the outermost aspect)

gnprice · 2026-04-27T20:43:04Z

+      ).copyWith(backgroundColor: WidgetStateColor.resolveWith((states) {
+        Color result = _backgroundColor(designVariables).resolve(states);
+        if (states.contains(WidgetState.disabled)) {
+          result = result.withFadedAlpha(0.5);
+        }
+        return result;


nit: can this be folded into _backgroundColor?

I agree it's good to keep this out of the switch statement, rather than make each of those cases more complex. Seems like it could go after the switch, though.

Yeah that's a nice cleanup, thanks!

gnprice · 2026-04-27T20:49:09Z

+      if (!context.mounted) return false;
+      final zulipLocalizations = ZulipLocalizations.of(context);
+      ScaffoldMessenger.of(context).showSnackBar(
+        SnackBar(behavior: SnackBarBehavior.floating,
+          content: Text(zulipLocalizations.reportMessageSuccess)));
+      return true;


I think this part belongs outside the try. If showing the snack bar somehow fails, it doesn't seem helpful to show an error dialog instead (if we even managed to do so in that situation).

This is an example of keeping the try block tightly scoped:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#catch-specific

gnprice · 2026-04-27T20:52:38Z

+      return tester.widget<EditableText>(find.byType(EditableText)).cursorColor;
+    }
+
+    testWidgets('smoke: empty controller shows 0/max', (tester) async {


nit: the specifics seem clear enough:

Suggested change

testWidgets('smoke: empty controller shows 0/max', (tester) async {

testWidgets('empty controller shows 0/max', (tester) async {

gnprice · 2026-04-27T21:02:43Z

+          child: SingleChildScrollView(
+            padding: EdgeInsets.symmetric(vertical: 8),
+            child: Column(
+              mainAxisSize: MainAxisSize.min,


nit: shorthand

(possibly we should enable a lint rule for that; not sure if there are many cases where it'd make the code less rather than more readable)

gnprice · 2026-04-27T21:09:00Z

+      validator: (value) {
+        if (value == null) {
+          return zulipLocalizations.reportMessageReasonRequired;
+        }
+        return null;
+      },


This should be impossible now that it has a default, right?

gnprice · 2026-04-27T21:16:45Z

+  /// This is a [ValueGetter] so that the [validator] reads the latest value
+  /// at call time. When the dropdown value changes, [Form] validates all
+  /// fields synchronously via [FormField.didChange] before the parent has
+  /// a chance to rebuild this widget with updated props.
+  final ValueGetter<String?> selectedType;


Huh, interesting subtlety.

This will change a bit in the next revision:

/// This is a [ValueGetter] so that [TextFormField.validator] reads the /// latest value at call time. When the dropdown value changes, [Form] /// triggers all its fields' validators *before* this widget rebuilds /// and receives the new [selectedType].

because Claude pointed out that "synchronously" isn't quite right:

Details

❯ OK, and the reasoning in that dartdoc is sound? ⏺ Let me check Flutter's actual behavior to verify the reasoning. Searched for 5 patterns, read 3 files (ctrl+o to expand) ⏺ The conclusion is sound, but one phrase is misleading. Let me walk through what actually happens: When the dropdown value changes: 1. DropdownMenuFormField's state calls field.didChange(value), which calls FormFieldState.didChange → Form._fieldDidChange() → Form._forceRebuild() (a setState). Then it calls the user's onSelected (lib/.../dropdown_menu_form_field.dart:224-228), which triggers our parent's setState updating _selectedType — synchronously. 2. On the next frame, parent rebuilds first; during parent's rebuild it calls updateChild on the Form element. Form is already dirty, so _FormState.build runs. 3. Inside _FormState.build, the autovalidate switch (form.dart:280-294) calls _validate before returning the new child widget tree. _validate iterates _fields and calls each field.validate(), which uses widget.validator from the FormFieldState's current widget — which is still the old TextFormField from the previous build. 4. Only after Form.build returns does the framework propagate updates down to the description field, giving it a fresh TextFormField with a validator closure that captures the new selectedType. So a plain String? selectedType prop would be stale at validate time (the closure captured the old prop value). A ValueGetter reads _selectedType from parent state at call time, and parent state was already updated synchronously in step 1, so it sees the new value. The conclusion (use a ValueGetter) holds. The misleading bit is "[Form] validates all fields synchronously via [FormField.didChange]". FormField.didChange doesn't validate synchronously — it calls Form._fieldDidChange, which only does setState. Validation runs later, during Form.build on the next frame. The key property isn't synchrony; it's that Form.build calls _validate before its child subtree gets rebuilt with updated props. Suggested rewording: /// This is a [ValueGetter] so that [TextFormField.validator] reads the /// latest value at call time. When the dropdown value changes, [Form] /// schedules a rebuild during which autovalidation runs [Form._validate] /// against all fields' current validators — before the parent's rebuild /// has propagated updated props down to this widget. Want me to apply that? ✻ Brewed for 4m 25s ※ recap: Fixing failing tests in commit 8d99a7c on the report_message dialog; bug is fixed and tests pass. Next: decide whether to apply the proposed dartdoc rewording on `_ReportDescriptionField.selectedType`. (disable recaps in /config) ❯ Is there a simpler way to explain it? Maybe: "This is a [ValueGetter] so that [TextFormField.validator] reads the latest value at call time. When the dropdown value changes, [Form] triggers all its fields' validators *before* this widget rebuilds and receives the new [selectedType]." Feel very free to push back or tweak as needed, I'm just thinking about what's most useful and comprehensible to the reader. ⏺ Yes, that's better — it gets the load-bearing fact (validators run before this widget rebuilds with new props) without dragging the reader through Form internals. I'd apply it as-is.

gnprice · 2026-04-27T21:18:19Z

+        validator: (value) {
+          if (selectedType() == kMessageReportTypeOther
+              && (value == null || value.trim().isEmpty)) {
+            return zulipLocalizations.reportMessageDescriptionRequired;
+          }
+          return null;
+        },


nit: this might be clearest as a private helper method

… oh I see, but it needs the context to get the localizations; and this is a stateless widget so methods can't just get the context from the state. Probably this is simplest, then.

(If this were 2-3x longer than it is, a private method would still be worth it; this would just have to say something like (value) => _validate(context, value).)

gnprice · 2026-04-27T21:32:53Z

+    Future<void> showFromMessageList(WidgetTester tester, {
+      required StreamMessage message,


This feature exists for DMs too, right? Ideally we'd have a test that exercises that, to ensure we don't accidentally make some sort of assumption that would break there.

(For example, if we were to add a rendering of the message at the top of the dialog — seems like a reasonable thing to do for confirmation that the user is acting on the message they intend to — then that might come with an indication of where it was sent, and that would involve some different code paths for DMs vs. channel/stream messages.)

chrisbobbe · 2026-04-28T00:47:31Z

Thanks for the review! Revision pushed.

gnprice

Looks great! One nit; please merge at will.

gnprice · 2026-04-28T03:46:44Z

+      title: Text(title,
+        style: TextStyle(
+          color: designVariables.title,
+          fontSize: 22,
+          height: 30 / 22,
+        ).merge(weightVariableTextStyle(context, wght: 600))),
+      titlePadding: EdgeInsets.fromLTRB(16, 12, 16, 4),


nit: I'd be inclined to put the padding before the thing it's padding around — that's the order they'd appear if using a Padding widget directly, and fits the usual Flutter order of wrapper/metadata first, child/payload later

This is web/icons/flag.svg at current zulip/zulip main: https://github.com/zulip/zulip/blob/a8d809eb0/web/icons/flag.svg

…pper Drives a text field's counter widget, cursor color, and currentValueLength/maxValueLength semantics from a Unicode code-point count, matching what the Zulip server counts when validating text input (e.g. for report-message `description`). Uses a single listener on the caller's TextEditingController to rebuild the counter and to toggle an ambient TextSelectionThemeData override for the cursor color. Replaces what would otherwise be a reliance on Flutter's TextField.maxLength machinery, which counts grapheme clusters rather than code points (flutter/flutter#182907).

This can be triggered in English with the label 'Your Zulip organization URL' when you crank up the text-size setting.

Fixes zulip#1972. Adds a "Report message" item in the message action sheet, gated on the realm having a moderation-request channel configured. Tapping it opens the new ReportMessageDialog, with a reason dropdown and an optional description; submitting posts via ZulipAction.reportMessage. The description field uses ZulipCodePointLengthLimit, so the "n/1000" counter and the cursor's over-limit color are both driven by Unicode code points, matching the server's `description` limit. Over-limit input isn't prevented locally; submissions are rejected by the server and surfaced as an error dialog.

chrisbobbe · 2026-04-29T23:21:50Z

Thanks! Done.

chrisbobbe force-pushed the pr-report-message branch from 688302b to 1eb6b67 Compare April 21, 2026 21:06

chrisbobbe requested review from alya and rajveermalviya April 21, 2026 21:08

chrisbobbe assigned rajveermalviya Apr 21, 2026

chrisbobbe added maintainer review PR ready for review by Zulip maintainers product review Added by maintainers when a PR needs product review labels Apr 21, 2026

gnprice reviewed Apr 21, 2026

View reviewed changes

chrisbobbe force-pushed the pr-report-message branch 2 times, most recently from d9b291a to 4c9561d Compare April 24, 2026 22:52

rajveermalviya approved these changes Apr 27, 2026

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 Apr 27, 2026

rajveermalviya assigned gnprice and unassigned rajveermalviya Apr 27, 2026

rajveermalviya requested a review from gnprice April 27, 2026 18:27

chrisbobbe force-pushed the pr-report-message branch from 4c9561d to 2cc985f Compare April 27, 2026 19:59

gnprice reviewed Apr 27, 2026

View reviewed changes

chrisbobbe force-pushed the pr-report-message branch from 2cc985f to cd0770e Compare April 28, 2026 00:47

gnprice approved these changes Apr 28, 2026

View reviewed changes

chrisbobbe added 7 commits April 29, 2026 16:13

model: Add data for report-message to RealmStore

d06bf39

button: Add "disabled" state to ZulipWebUiKitButton

b462e4c

dialog: Add ZulipDialog, to use for the report-message feature

093d370

icons: Add flag, from web

ad9125e

This is web/icons/flag.svg at current zulip/zulip main: https://github.com/zulip/zulip/blob/a8d809eb0/web/icons/flag.svg

input [nfc]: Expose style attributes of PopupMenuList, as a MenuStyle

af15b52

button: Add info intent button for low attention

5f5f9c6

actions: Add ZulipAction.reportMessage

4366807

chrisbobbe added 4 commits April 29, 2026 16:13

login: Let inputs' label text wrap to multiple lines when unfocused

4e80862

This can be triggered in English with the label 'Your Zulip organization URL' when you crank up the text-size setting.

report_message: Select 'spam' initially instead of null

a48fad2

chrisbobbe force-pushed the pr-report-message branch from cd0770e to a48fad2 Compare April 29, 2026 23:21

chrisbobbe merged commit a48fad2 into zulip:main Apr 29, 2026
2 checks passed

chrisbobbe deleted the pr-report-message branch April 29, 2026 23:21

chrisbobbe mentioned this pull request Apr 29, 2026

action : support message reporting (#1972) #2073

Closed

	setState(() => _requestInProgress = false);
	if (mounted) setState(() => _requestInProgress = false);

	testWidgets('smoke: empty controller shows 0/max', (tester) async {
	testWidgets('empty controller shows 0/max', (tester) async {

		Future<void> showFromMessageList(WidgetTester tester, {
		required StreamMessage message,

Conversation

chrisbobbe commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Apr 21, 2026

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!

chrisbobbe commented Apr 24, 2026

Uh oh!

rajveermalviya 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!

chrisbobbe commented Apr 27, 2026

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!

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!

chrisbobbe commented Apr 28, 2026

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbobbe commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

chrisbobbe commented Apr 21, 2026 •

edited

Loading