Add error haptic feedback to showErrorDialog by sharayuwadghule · Pull Request #2342 · zulip/zulip-flutter

sharayuwadghule · 2026-06-19T07:45:25Z

Add error haptic feedback when displaying an error dialog.

Currently, showErrorDialog provides only visual feedback. This change calls HapticFeedback.errorNotification() when showing an error dialog, providing platform-appropriate tactile feedback on Android and iOS.

As requested in the issue, the change is scoped only to showErrorDialog and does not affect showSuggestedActionDialog.

Fixes #2338

Testing

Added test coverage in test/widgets/dialog_test.dart.
Verified that the platform channel receives a HapticFeedback.vibrate call with HapticFeedbackType.errorNotification.
Ran:

flutter test test/widgets/dialog_test.dart
flutter test

Ruhdee · 2026-06-22T08:46:16Z

Thank you for this PR @sharayuwadghule! This looks promising. Please make the following changes so that this PR is reviewable by the maintainers:

Please go through https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-messages and rewrite your commit message according to it.
If you're using LLMs, please also go through https://zulip.readthedocs.io/en/latest/contributing/contributing.html#ai-use-policy-and-guidelines.
I think it would be best to add an independent test for haptic feedback in the showErrorDialog group instead of folding it into an existing test.

sharayuwadghule · 2026-06-22T10:48:09Z

@Ruhdee Thanks for the review!

I've updated the commit message to follow the commit-discipline guidelines and moved the haptic feedback verification into a dedicated test in the showErrorDialog group.

I've also reviewed the AI-use policy and verified the implementation and test behavior myself. The test suite passes successfully.

Please let me know if there are any other changes needed.

Ruhdee

Thank you for the revision! Some small comments below:

Ruhdee · 2026-06-23T10:20:38Z

+          return null;
+        },
+      );


nit:

Suggested change

return null;

},

);

return null;

});

Bumping this again, we generally prefer to avoid this code style pattern (unnecessary trailing comma + newline + closing-parenthesis).

Ruhdee · 2026-06-23T10:28:12Z

+        ..has((c) => c.method, 'method').equals('HapticFeedback.vibrate')
+        ..has((c) => c.arguments, 'arguments').equals('HapticFeedbackType.errorNotification'));


It is more idiomatic to add extensions in this codebase so that we can cascade properties and check directly:

extension MethodCallChecks on Subject<MethodCall> { Subject<String> get method => has((c) => c.method, 'method'); Subject<Object?> get arguments => has((c) => c.arguments as Object?, 'arguments'); }

I believe the 'flutter/services.dart' section of test/flutter_checks.dart would be the right location for it.

Ruhdee · 2026-06-23T10:30:10Z

+      check(calls).any((it) => it
+        ..has((c) => c.method, 'method').equals('HapticFeedback.vibrate')
+        ..has((c) => c.arguments, 'arguments').equals('HapticFeedbackType.errorNotification'));


I think there is only one method call being received, so why not something like this:

Suggested change

check(calls).any((it) => it

..has((c) => c.method, 'method').equals('HapticFeedback.vibrate')

..has((c) => c.arguments, 'arguments').equals('HapticFeedbackType.errorNotification'));

check(calls).single

..method.equals('HapticFeedback.vibrate')

..arguments.equals('HapticFeedbackType.errorNotification');

sharayuwadghule · 2026-06-23T11:19:48Z

@Ruhdee Updated, thanks for the suggestion.

Ruhdee · 2026-06-24T04:07:05Z

There are a lot of unrelated formatting changes. Please revert them.

sharayuwadghule · 2026-06-24T08:18:11Z

@Ruhdee Thanks! I've addressed the comments and reverted the unrelated formatting changes.

Ruhdee

I strongly urge you to self-review your code before asking others for review. The changes given below could have easily been spotted in a self-review!🙂

Ruhdee · 2026-06-26T06:25:16Z

  FirebaseInstallations: 4e6e162aa4abaaeeeb01dd00179dfc5ad9c2194e
  FirebaseMessaging: 341004946fa7ffc741344b20f1b667514fc93e31
-  Flutter: cabc95a1d2626b1b06e7179b784ebcf0c0cde467
+  Flutter: 71a624a5bc0c04062bf19101d501e466baf2fb47


Revert this unrelated change.

Ruhdee · 2026-06-26T06:30:50Z

+import 'package:flutter/services.dart';
+

 import '../generated/l10n/zulip_localizations.dart';


nit:

Suggested change

import 'package:flutter/services.dart';

import '../generated/l10n/zulip_localizations.dart';

import 'package:flutter/services.dart';

import '../generated/l10n/zulip_localizations.dart';

Ruhdee · 2026-06-26T06:52:05Z

Also, the commit author email has a malformed domain. Please fix it or the commit won't be attributed to you.

sharayuwadghule · 2026-06-26T13:35:00Z

@Ruhdee Thanks! I've reverted the unintended ios/Podfile.lock changes, removed the extra blank line, and fixed the commit author email. I also checked the current CI failures locally and found that tools/check l10n fails on a clean checkout of upstream/main as well, so the remaining CI failures appear unrelated to this PR.

Ruhdee · 2026-06-26T20:18:07Z

Thanks! The CI failures do seem to be unrelated to this PR. Moving to maintainer review.

rajveermalviya

Thanks for working on this @sharayuwadghule! Overall LGTM, just one nit below.

rajveermalviya · 2026-07-03T12:07:42Z

+      final messenger = tester.binding.defaultBinaryMessenger;
+      messenger.setMockMethodCallHandler(SystemChannels.platform, (methodCall) async {
+        calls.add(methodCall);
+        return null;
+      });


nit: inline variable

Suggested change

final messenger = tester.binding.defaultBinaryMessenger;

messenger.setMockMethodCallHandler(SystemChannels.platform, (methodCall) async {

calls.add(methodCall);

return null;

});

tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(

SystemChannels.platform,

(methodCall) async {

calls.add(methodCall);

return null;

});

sharayuwadghule · 2026-07-03T14:22:00Z

@rajveermalviya addressed! thanks.

rajveermalviya

Thanks for the revision! And thanks @Ruhdee for previous reviews. Just one nit below, otherwise looks good.

Moving over to Chris's review.

rajveermalviya · 2026-07-03T14:26:43Z

+          return null;
+        },
+      );


Bumping this again, we generally prefer to avoid this code style pattern (unnecessary trailing comma + newline + closing-parenthesis).

sharayuwadghule · 2026-07-03T16:25:58Z

@rajveermalviya Thanks for pointing this out. I've fixed the formatting issue and pushed the changes.

chrisbobbe

Thanks @sharayuwadghule, and thanks @Ruhdee and @rajveermalviya for the reviews! Small comments below.

chrisbobbe · 2026-07-07T22:38:48Z

  String? message,
  Uri? learnMoreButtonUrl,
 }) {
+  HapticFeedback.errorNotification();


Let's mention this new behavior in the dartdoc of showErrorDialog.

chrisbobbe · 2026-07-07T22:41:56Z

+      check(calls).single
+        ..method.equals('HapticFeedback.vibrate')
+        ..arguments.equals('HapticFeedbackType.errorNotification');


I think this can just be:

check(calls).single.isMethodCall('HapticFeedback.vibrate', arguments: 'HapticFeedbackType.errorNotification');

with no need for our own MethodCallChecks in test/flutter_checks.dart.

chrisbobbe · 2026-07-07T22:45:12Z

+      tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(
+        SystemChannels.platform,
+        (methodCall) async {
+          calls.add(methodCall);
+          return null;
+        });


I think we need an addTearDown for this?

sharayuwadghule · 2026-07-08T14:50:40Z

@chrisbobbe Thanks for the review! Addressed all the comments:

added the dartdoc note about the error haptic feedback,
switched the test to use isMethodCall and removed the custom MethodCallChecks,
added addTearDown to clean up the mock method call handler.

chrisbobbe

Thanks! Just a few nits below.

chrisbobbe · 2026-07-08T17:38:04Z

 /// Displays an [AlertDialog] with a dismiss button
 /// and optional "Learn more" button.
 ///
+/// Triggers error haptic feedback notification when shown.


nit: can say the same thing in fewer words, e.g.:

Suggested change

/// Displays an [AlertDialog] with a dismiss button

/// and optional "Learn more" button.

///

/// Triggers error haptic feedback notification when shown.

/// Displays an [AlertDialog] with a dismiss button

/// and optional "Learn more" button, and gives haptic feedback.

chrisbobbe · 2026-07-08T17:39:26Z

 import 'package:zulip/model/settings.dart';
 import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/dialog.dart';
-


Don't remove this blank line, which separates package: imports from relative imports.

chrisbobbe · 2026-07-08T17:40:45Z

+        tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(
+          SystemChannels.platform,
+          null,
+        );


nit: can use fewer lines:

Suggested change

tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(

SystemChannels.platform,

null,

);

tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(

SystemChannels.platform, null);

sharayuwadghule · 2026-07-09T09:39:53Z

@chrisbobbe Thanks! Done with the expected changes.

chrisbobbe · 2026-07-09T18:55:58Z

+      check(calls).single.isMethodCall(
+        'HapticFeedback.vibrate',
+        arguments: 'HapticFeedbackType.errorNotification',
+      );


Bump on code style from #2342 (comment) (use two lines instead of four)

sharayuwadghule · 2026-07-12T04:54:59Z

@chrisbobbe dart format reformats this back to four lines. Is there a formatter setting or different style you'd prefer here?

chrisbobbe · 2026-07-15T19:55:39Z

What does the project README say about dart format?

sharayuwadghule · 2026-07-17T11:15:18Z

@chrisbobbe I checked the README,it explicitly says not to use dart format and to follow the repository's existing style. I'll update this to match the two-line formatting from #2342. Thanks!

chrisbobbe · 2026-07-18T02:22:03Z

If you rebase atop current main it should fix the CI failure: #mobile-team > Multiple CI failures @ 💬

sharayuwadghule · 2026-07-18T16:43:43Z

@chrisbobbe I rebased onto the current main and confirmed that my branch was already up to date. The CI log I initially checked was for the older commit c086365 and failed because pod install modified ios/Podfile.lock. The latest CI run is now testing my updated commit, but the iOS job is still failing, so I'll check the latest job's error details.

chrisbobbe · 2026-07-22T21:36:12Z

I triggered a rerun of the CI jobs; hopefully that fixes it.

chrisbobbe · 2026-07-23T01:48:37Z

Did you git fetch upstream main (or swap in the right remote name, maybe git fetch origin main) before rebasing? It looks like your branch is based on an outdated main, 01907d8.

Previously, showErrorDialog provided only visual feedback when displaying an error. This can make error notifications easier to miss on devices that support haptic feedback. Call HapticFeedback.errorNotification() when showing an error dialog so users receive platform-appropriate tactile feedback in addition to the visual dialog. Fixes zulip#2338.

sharayuwadghule · 2026-07-25T10:14:01Z

@chrisbobbe Thanks, fixed - I had fetched from origin instead of upstream. Rebased onto the latest upstream/main now.

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from 5209ea7 to 505e167 Compare June 22, 2026 10:36

Ruhdee suggested changes Jun 23, 2026

View reviewed changes

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from 505e167 to 6965eaf Compare June 23, 2026 11:05

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch 4 times, most recently from 51d9673 to d4aa384 Compare June 24, 2026 08:04

Ruhdee reviewed Jun 26, 2026

View reviewed changes

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch 3 times, most recently from f0674cf to f4c2a68 Compare June 26, 2026 12:22

sharayuwadghule requested a review from Ruhdee June 26, 2026 13:34

Ruhdee approved these changes Jun 26, 2026

View reviewed changes

Ruhdee assigned rajveermalviya Jun 26, 2026

Ruhdee added the maintainer review PR ready for review by Zulip maintainers label Jun 26, 2026

Ruhdee mentioned this pull request Jun 29, 2026

Use fixed height and derive width from aspect ratio for MessageImagePreview #2344

Open

rajveermalviya requested changes Jul 3, 2026

View reviewed changes

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from f4c2a68 to 6c1064f Compare July 3, 2026 14:09

rajveermalviya approved these changes Jul 3, 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 Jul 3, 2026

rajveermalviya assigned chrisbobbe and unassigned rajveermalviya Jul 3, 2026

rajveermalviya requested a review from chrisbobbe July 3, 2026 14:29

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from 6c1064f to b5e8358 Compare July 3, 2026 16:24

chrisbobbe reviewed Jul 7, 2026

View reviewed changes

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from b5e8358 to 3fa6dbd Compare July 8, 2026 14:48

chrisbobbe reviewed Jul 8, 2026

View reviewed changes

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from 3fa6dbd to ba220b2 Compare July 9, 2026 06:55

chrisbobbe reviewed Jul 9, 2026

View reviewed changes

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from ba220b2 to 6d72e05 Compare July 17, 2026 11:14

sharayuwadghule force-pushed the issue-2338-error-haptic-feedback branch from 6d72e05 to 30443ef Compare July 25, 2026 10:09

		..has((c) => c.method, 'method').equals('HapticFeedback.vibrate')
		..has((c) => c.arguments, 'arguments').equals('HapticFeedbackType.errorNotification'));

		import 'package:flutter/services.dart';


		import '../generated/l10n/zulip_localizations.dart';

Uh oh!

Conversation

sharayuwadghule commented Jun 19, 2026

Testing

Uh oh!

Ruhdee commented Jun 22, 2026

Uh oh!

sharayuwadghule commented Jun 22, 2026

Uh oh!

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

Ruhdee Jun 23, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

sharayuwadghule commented Jun 23, 2026

Uh oh!

Ruhdee commented Jun 24, 2026

Uh oh!

sharayuwadghule commented Jun 24, 2026

Uh oh!

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

Ruhdee commented Jun 26, 2026

Uh oh!

sharayuwadghule commented Jun 26, 2026

Uh oh!

Ruhdee commented Jun 26, 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!

sharayuwadghule commented Jul 3, 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!

sharayuwadghule commented Jul 3, 2026

Uh oh!

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

sharayuwadghule commented Jul 8, 2026

Uh oh!

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

sharayuwadghule commented Jul 9, 2026

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Ruhdee Jun 23, 2026 •

edited

Loading

chrisbobbe Jul 9, 2026 •

edited

Loading