Skip to content

Conversation

@chrisbobbe
Copy link
Collaborator

Fixes #979.

Discussion:
#979 (comment)


Tested manually, with and without this change:

  1. Log out from CZO (too active; events coming in frequently); connect to a not-very-active org

  2. Make an artificial network error in the getEvents binding:

diff --git lib/api/route/events.dart lib/api/route/events.dart
index bd14521c7..da7bf4fea 100644
--- lib/api/route/events.dart
+++ lib/api/route/events.dart
@@ -1,6 +1,7 @@
 import 'package:json_annotation/json_annotation.dart';
 
 import '../core.dart';
+import '../exception.dart';
 import '../model/events.dart';
 import '../model/initial_snapshot.dart';
 
@@ -27,6 +28,7 @@ Future<InitialSnapshot> registerQueue(ApiConnection connection) {
 Future<GetEventsResult> getEvents(ApiConnection connection, {
   required String queueId, int? lastEventId, bool? dontBlock,
 }) {
+  throw NetworkException(routeName: 'foo', message: 'foo', cause: 'foo');
   return connection.get('getEvents', GetEventsResult.fromJson, 'events', {
     'queue_id': RawParameter(queueId),
     if (lastEventId != null) 'last_event_id': lastEventId,
  1. Watch the logs to see retries going by
  2. Comment out the throw NetworkException line and do a hot-reload
  3. Watch the logs for dont_block in the GET /events, and watch the app to see if the loading indicator goes away quickly
    • Without the change, I don't see dont_block, and the loading indicator doesn't go away until the next event comes in
    • With the change, I see dont_block, and the loading indicator goes away quickly

@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jul 7, 2025
@chrisbobbe
Copy link
Collaborator Author

I found a comment that needed updating; revision pushed.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning to include this PR in a v30.0.261 release tomorrow. It'd be great to have it merged first.

Just read through the code; comments below. I've also included this in a build I'm using now on my own phone.

return awaitFakeAsync((async) async {
await prepare();

bool expectDontBlock = false; // (Not on the first attempt, but after that)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit confusing because of what I think is a double negative. This value takes effect precisely on the first attempt, right? Then it's set to a different value for the next attempt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think probably just dropping this comment clears things up. The reader can see a few lines below how this gets mutated after the first attempt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value takes effect precisely on the first attempt, right? Then it's set to a different value for the next attempt.

Yeah, that's right.

@chrisbobbe
Copy link
Collaborator Author

Thanks!! Revision pushed.

@gnprice
Copy link
Member

gnprice commented Jul 10, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit f8b0e8b into zulip:main Jul 10, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer review PR ready for review by Zulip maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loading indicator slow to go away when returning to app, though in fact up to date

3 participants