local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

PIG208 · 2025-04-01T22:06:09Z

Work toward Simplified local echo, with retry #1441
Prep for local echo (n/n): Support simplified version of local echo #1453

rajveermalviya

Thanks for working on this! This LGTM.

(Also, for the PR title you probably meant 'local echo' instead of 'locale echo').

gnprice

Thanks! Comments below.

gnprice · 2025-04-02T18:42:25Z

lib/api/model/events.dart

-// TODO use [JsonSerializable] here too, using its customization features,
-//   in order to skip the boilerplate in [fromJson] and [toJson].


Cool, nice to have taken care of this.

gnprice · 2025-04-02T18:43:21Z

lib/api/model/events.dart

@@ -665,8 +665,7 @@ class UserTopicEvent extends Event {
 }

 /// A Zulip event of type `message`: https://zulip.com/api/get-events#message
-// TODO use [JsonSerializable] here too, using its customization features,
-//   in order to skip the boilerplate in [fromJson] and [toJson].
+@JsonSerializable(fieldRename: FieldRename.snake)


commit-message nits:

It will make it easier to add addditional fields on the event later

needs period (commit-message bodies use complete sentences); and s/ddd/dd/

gnprice · 2025-04-02T18:45:05Z

test/model/recent_dm_conversations_test.dart

@@ -1,6 +1,5 @@
 import 'package:checks/checks.dart';
 import 'package:test/scaffolding.dart';
-import 'package:zulip/api/model/events.dart';


commit-message nit:

A neat side-effect is that we can cut some imports from lib/api/model/.

s/from/of/

I parsed "cut some imports from $file" as "cut X from Y", meaning that Y had some X which got cut. Then was puzzled why an example_data change would affect that file, then saw the file indeed wasn't touched, then realized what you meant 🙂

gnprice · 2025-04-02T18:49:28Z

test/model/message_list_test.dart

@@ -367,8 +367,7 @@ void main() {
      List.generate(30, (i) => eg.streamMessage(stream: stream)));

    check(model).messages.length.equals(30);
-    await store.handleEvent(MessageEvent(id: 0,
-      message: eg.streamMessage(stream: stream)));
+    await store.handleEvent(eg.messageEvent(eg.streamMessage(stream: stream)));


Let's use the handy helper that simplifies these:

Suggested change

await store.handleEvent(eg.messageEvent(eg.streamMessage(stream: stream)));

await store.addMessage(eg.streamMessage(stream: stream));

In fact maybe switch to that in a preceding prep commit, and then the bulk of these don't need to change in the eg.messageEvent commit.

gnprice · 2025-04-02T18:54:37Z

test/api/route/messages_test.dart

-            'queue_id': '"abc:123"',
-            'local_id': '"456"',
+            'queue_id': 'abc:123',


Hmm, is this really NFC?

api [nfc]: Use RawParameter when passing queue_id

Seems like it changes what we'll actually send to the server.

I guess it's NFC for the actual app, in that we don't currently pass this parameter in calls to sendMessage. Still I'd call this just api:, not api [nfc]:, because it's definitely a functional change to the API bindings.

Strings in general should not be JSON-encoded in Zulip APIs.

It's true that a string as a top-level parameter to a Zulip API endpoint is typically sent as itself, not JSON-encoded. That isn't always true, though. Right in this same endpoint, when passing a channel name to to, that's expected to be as JSON and not a raw string.

(The server does currently accept a raw stream name there, apparently, but that isn't and fundamentally can't be reliable.)

Moreover our docs don't really cover where this is the case or isn't, except via the curl examples; and the curl example for this endpoint doesn't have queue_id.

So for this change, I think the key point is that you've tested and the raw string is what works.

gnprice · 2025-04-02T19:19:40Z

lib/api/model/events.dart

+  // The format of this is documented to be chosen freely by the client.
+  //
+  // When present, this corresponds to the JSON-encoded int, "local_id",
+  // from a previous [sendMessage] call by us.
+  @JsonKey(fromJson: _localMessageIdFromJson, toJson: _localMessageIdToJson)
+  final int? localMessageId;


Since this isn't yet covered by the API docs (which are linked from this class's dartdoc), let's include dartdoc with a link to the best reference we have. That'd be the link in the commit message:
https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/local_id.2C.20queue_id.2Fsender_queue_id/near/2135340

gnprice · 2025-04-02T19:26:26Z

lib/api/route/messages.dart

+    // This is documented to be an optional String whose format is chosen freely
+    // by the client.  Use a JSON-encoded int consistently, so that we know how
+    // to decode it when we receive the corresponding "message" event.
+    // See also: [MessageEvent.localMessageId]


In general I'd rather keep these bindings as close as possible to the underlying Zulip API — adapting the API to reasonable Dart idioms, but generally not encoding our particular choices of how the app uses the API.

One reason is that it saves us from needing this sort of comment to explain how and why the internal API differs from the one that's in the API docs 🙂 (or in this case hopefully will be documented there).

What if we just leave this as a string? The MessageStore code that calls this endpoint, and then consumes the local_message_id in the resulting MessageEvent, will end up converting an int message ID to string and back, but that's fine — it's really that code that is responsible for matching those requests and events up with each other, so that's a natural place to put the responsibility for deciding what these strings should look like.

Yeah, I agree it will be cleaner when we handle that encoding at a different layer. In this case it should be self-contained in MessageStore.

gnprice · 2025-04-02T19:34:49Z

test/example_data.dart

@@ -620,6 +620,9 @@ UserTopicEvent userTopicEvent(
  );
 }

+MessageEvent messageEvent(Message message) =>


commit-message nit:

example_data [nfc]: Add messageEvent

Better:

test [nfc]: Add eg.messageEvent

For examples of previous summary lines, try this command:

$ git log --format='%>(20)%an %cs %h %s' --grep 'eg\.|example' origin \ | grep -P '\bexample|\beg\b'

Thanks! I added a -E flag to the git log command to get this working.

Ah yeah, thanks. I have this in my ~/.gitconfig, which I recommend:

[grep] # "Basic" regexps are a relic. Extended would be fine; Perl is better. patternType = perl

That's equivalent to a -P flag, which goes farther in the same direction as -E.

PIG208 · 2025-04-02T20:51:20Z

Updated the PR. This should be ready for review. Thanks!

gnprice

Thanks! This looks good; just small comments below.

gnprice · 2025-04-03T06:09:40Z

lib/api/model/events.dart

+  // The format of this is documented to be chosen freely by the client.
+  //
+  // When present, this corresponds to the JSON-encoded int, "local_id",
+  // from a previous [sendMessage] call by us.


Suggested change

// The format of this is documented to be chosen freely by the client.

//

// When present, this corresponds to the JSON-encoded int, "local_id",

// from a previous [sendMessage] call by us.

// When present, this equals the "local_id" parameter

// from a previous [sendMessage] call by us.

The details about what we choose to put into "local_id" are no longer something this needs in order to justify how this code works, so this can stick to documenting the API itself.

gnprice · 2025-04-03T06:12:07Z

lib/api/route/messages.dart

-    if (queueId != null) 'queue_id': queueId, // TODO should this use RawParameter?
-    if (localId != null) 'local_id': localId, // TODO should this use RawParameter?
+    if (queueId != null) 'queue_id': RawParameter(queueId),
+    if (localId != null) 'local_id': RawParameter(localId),


The change to localId here on this route is now totally parallel to the change to queueId, so probably best done in the same commit.

Then the change adding localMessageId on the event can remain in its own subsequent commit.

It will make it easier to add additional fields on the event later.

This leaves some tests that handle MessageEvent untouched, as they conceptually aren't just setting up the store, and the helper adds a layer of indirection (or they simply don't have access to a PerAccountStore).

MessageEvent constructor calls are generally quite short, and there are no boring required fields other than an integer id. However, as we add localMessageId to the event, it would be expected to have a boring value most of the time, so prepare for that. A neat side-effect is that we can cut some imports of lib/api/model/. (Some places that use non-zero event IDs now use 0, but we don't care much about this value anyway.)

PIG208 · 2025-04-03T17:55:46Z

Thanks! Updated the PR.

gnprice · 2025-04-03T18:48:59Z

Thanks! A commit message needs updating:

api: Use RawParameter when passing queue_id

This endpoint in particular expects queue_id to be a String just
passed as itself, not JSON-encoded.

We will handle local_id separately because that's a bit different.

as that commit now handles local_id too :-)

Otherwise looks all ready.

This endpoint in particular expects queue_id and local_id each to be a String just passed as itself, not JSON-encoded.

This will be used for local echoing. local_message_id on message event is not yet fully documented; see CZO discussion: https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/local_id.2C.20queue_id.2Fsender_queue_id/near/2135340

PIG208 · 2025-04-03T18:54:58Z

Oops. Just pushed another update.

gnprice · 2025-04-03T19:38:21Z

Thanks! Looks good; merging.

PIG208 changed the title ~~Add API support for local_id/local_message_id and queue_id~~ Add API support for local_id/local_message_id and queue_id (1/n) Apr 1, 2025

PIG208 changed the title ~~Add API support for local_id/local_message_id and queue_id (1/n)~~ locale echo (1/n): Add API support for local_id/local_message_id and queue_id Apr 1, 2025

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Apr 1, 2025

PIG208 assigned rajveermalviya Apr 1, 2025

PIG208 requested a review from rajveermalviya April 1, 2025 22:16

rajveermalviya approved these changes Apr 2, 2025

View reviewed changes

rajveermalviya assigned gnprice and unassigned rajveermalviya Apr 2, 2025

rajveermalviya requested a review from gnprice April 2, 2025 12:00

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 2, 2025

PIG208 changed the title ~~locale echo (1/n): Add API support for local_id/local_message_id and queue_id~~ local echo (1/n): Add API support for local_id/local_message_id and queue_id Apr 2, 2025

gnprice reviewed Apr 2, 2025

View reviewed changes

PIG208 force-pushed the pr-echo-1 branch from 9e841e8 to 4be6e68 Compare April 2, 2025 20:46

PIG208 requested a review from gnprice April 2, 2025 20:51

gnprice reviewed Apr 3, 2025

View reviewed changes

PIG208 added 3 commits April 3, 2025 13:53

api [nfc]: Use JsonSerializable with MessageEvent

450d476

It will make it easier to add additional fields on the event later.

test [nfc]: Switch from store.handleEvent to store.addMessage(s)

1c077cc

This leaves some tests that handle MessageEvent untouched, as they conceptually aren't just setting up the store, and the helper adds a layer of indirection (or they simply don't have access to a PerAccountStore).

PIG208 force-pushed the pr-echo-1 branch from 4be6e68 to 2a5a5cb Compare April 3, 2025 17:55

PIG208 added 2 commits April 3, 2025 14:54

api: Use RawParameter when passing queue_id and local_id

c982795

This endpoint in particular expects queue_id and local_id each to be a String just passed as itself, not JSON-encoded.

api: Add localMessageId to MessageEvent

d224a5b

This will be used for local echoing. local_message_id on message event is not yet fully documented; see CZO discussion: https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/local_id.2C.20queue_id.2Fsender_queue_id/near/2135340

PIG208 force-pushed the pr-echo-1 branch from 2a5a5cb to d224a5b Compare April 3, 2025 18:54

gnprice merged commit d224a5b into zulip:main Apr 3, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

PIG208 commented Apr 1, 2025

rajveermalviya left a comment

gnprice left a comment

gnprice Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

PIG208 Apr 2, 2025

gnprice Apr 2, 2025

PIG208 Apr 2, 2025

gnprice Apr 3, 2025

PIG208 commented Apr 2, 2025

gnprice left a comment

gnprice Apr 3, 2025

gnprice Apr 3, 2025

PIG208 commented Apr 3, 2025

gnprice commented Apr 3, 2025

PIG208 commented Apr 3, 2025

gnprice commented Apr 3, 2025

		// TODO use [JsonSerializable] here too, using its customization features,
		// in order to skip the boilerplate in [fromJson] and [toJson].

	await store.handleEvent(eg.messageEvent(eg.streamMessage(stream: stream)));
	await store.addMessage(eg.streamMessage(stream: stream));

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

Conversation

PIG208 commented Apr 1, 2025

rajveermalviya left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Apr 2, 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

PIG208 commented Apr 3, 2025

gnprice commented Apr 3, 2025

PIG208 commented Apr 3, 2025

gnprice commented Apr 3, 2025