Add and handle live-updates to saved snippets data #1511

PIG208 · 2025-05-13T18:50:04Z

This prepares the data model and API bindings for #1391 towards #863.

chrisbobbe

Thanks!! Small comments below.

chrisbobbe · 2025-05-20T19:24:01Z

lib/api/model/events.dart

@@ -37,6 +37,13 @@ sealed class Event {
          case 'update': return RealmUserUpdateEvent.fromJson(json);
          default: return UnexpectedEvent.fromJson(json);
        }
+      case 'saved_snippets':


api: Add save_snippets events and handle live-updates

Commit-message nit: saved_snippets

Also this is labeled as an "api:" commit, but it also adds to model code; how about separate api: and model: commits?

(Or perhaps just one model: commit with the API-binding additions lumped into that same commit—but if doing that, it would be logical to lump all the new API-binding code into that commit, including the initial-snapshot code.)

chrisbobbe · 2025-05-20T19:32:55Z

lib/model/saved_snippet.dart

+  Map<int, SavedSnippet> get savedSnippets;
+}
+
+class SavedSnippetStoreImpl with SavedSnippetStore {


Normally a substore would extend PerAccountStoreBase…it seems we don't depend on that here though 🙂 but maybe it's helpful to do it anyway, in case it's needed in the future, and also as an example for future substore implementations?

diff --git lib/model/saved_snippet.dart lib/model/saved_snippet.dart index 66b50781d..92cb253ef 100644 --- lib/model/saved_snippet.dart +++ lib/model/saved_snippet.dart @@ -2,14 +2,18 @@ import 'package:collection/collection.dart'; import '../api/model/events.dart'; import '../api/model/model.dart'; +import 'store.dart'; mixin SavedSnippetStore { Map<int, SavedSnippet> get savedSnippets; } -class SavedSnippetStoreImpl with SavedSnippetStore { - SavedSnippetStoreImpl({required Iterable<SavedSnippet> savedSnippets}) - : _savedSnippets = { +class SavedSnippetStoreImpl extends PerAccountStoreBase with SavedSnippetStore { + SavedSnippetStoreImpl({ + required super.core, + required Iterable<SavedSnippet> savedSnippets, + }) : + _savedSnippets = { for (final savedSnippet in savedSnippets) savedSnippet.id: savedSnippet, };

chrisbobbe · 2025-05-20T19:33:45Z

test/model/saved_snippet.dart

+      savedSnippets: [eg.savedSnippet(id: 101)]));
+    check(store).savedSnippets.values.single.id.equals(101);
+
+    await store.handleEvent(SavedSnippetsAddEvent(id: 1,savedSnippet:


nit: space after comma; actually maybe put savedSnippet: on the next line, with eg.savedSnippet(?

PIG208 · 2025-05-20T20:45:19Z

Thanks! Updated the PR.

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2025-05-20T22:16:13Z

lib/api/route/saved_snippets.dart

+part 'saved_snippets.g.dart';
+
+/// https://zulip.com/api/create-saved-snippet
+Future<CreateSavedSnippetResult> createSavedSnippet(ApiConnection connection, {


api: Add createSavedSnippet route

This needs a simple smoke test in test/api/route/.

chrisbobbe · 2025-05-20T22:20:39Z

lib/model/store.dart

+      case SavedSnippetsEvent():
+        assert(debugLog('server event: saved_snippets/${event.op}'));
+        _savedSnippets.handleSavedSnippetsEvent(event);
+        notifyListeners();


Ah I guess this case needs to be handled (as a no-op) in the earlier commit—

api: Add saved_snippets events

—to satisfy the analyzer there.

chrisbobbe · 2025-05-20T22:22:07Z

lib/model/saved_snippet.dart

+  Map<int, SavedSnippet> get savedSnippets;
+}
+
+class SavedSnippetStoreImpl extends PerAccountStoreBase with SavedSnippetStore {


model: Handle live-updates to savedSnippets

Commit-message nit: this commit does more than just handle live-updates to savedSnippets—it's what adds savedSnippets into the codebase in the first place :)

Could be:

model: Add PerAccountStore.savedSnippets, updating with events

or similar.

PIG208 · 2025-05-20T23:01:55Z

Thanks! Pushed an update.

chrisbobbe

Thanks! One nit below, and I'll mark for Greg's review.

chrisbobbe · 2025-05-20T23:15:03Z

test/api/model/saved_snippets_test.dart

+      check(connection.takeRequests()).single.isA<http.Request>()
+        .bodyFields.deepEquals({
+        'title': 'test saved snippet',
+        'content': 'content',
+      });


Suggested change

check(connection.takeRequests()).single.isA<http.Request>()

.bodyFields.deepEquals({

'title': 'test saved snippet',

'content': 'content',

});

check(connection.takeRequests()).single.isA<http.Request>()

.bodyFields.deepEquals({

'title': 'test saved snippet',

'content': 'content',

});

(I think)

Oh right. Oops 😬

gnprice

Thanks to you both! I've done a quick skim, and this structure looks good. I like the organization with the separate API and model/store commits (as per #1511 (comment)), with this to decouple the API event commit from the store changes:

      case SavedSnippetsEvent():
        // TODO handle
        break;

I'll aim to do a full review tomorrow.

gnprice · 2025-05-21T05:20:05Z

lib/api/model/events.dart

+/// The corresponding API docs are in several places for
+/// different values of `op`; see subclasses.
+sealed class SavedSnippetsEvent extends Event {


nit: It looks like these docs are actually next to each other, so this class can usefully link to the one that comes first. (The subclasses can still link to the individual sections.)

This wording was originally for realm_user, which is actually scattered across two widely-separated places in https://zulip.com/api/get-events . (And I see I wrote it at the same time for stream but it's not true of stream now; not sure if that was an oversight or that one got fixed since then.)

gnprice

OK, and here's a full review — just small comments below.

(And I see you've already revised to handle my one comment above, thanks.)

gnprice · 2025-05-21T21:48:33Z

lib/model/store.dart

+  @override
+  Map<int, SavedSnippet> get savedSnippets => _savedSnippets.savedSnippets;
+  final SavedSnippetStoreImpl _savedSnippets;
+
  final UserSettings? userSettings; // TODO(server-5)


nit: let's put the new members after userSettings — the user's overall settings are a more fundamental piece of data than the saved snippets specifically (which are basically a particular setting)

gnprice · 2025-05-21T21:54:52Z

test/api/model/model_checks.dart

+extension SavedSnippetChecks on Subject<SavedSnippet> {
+  Subject<int> get id => has((x) => x.id, 'id');
+  Subject<String> get title => has((x) => x.title, 'title');
+  Subject<String> get content => has((x) => x.content, 'content');
+}


nit: let's include the remaining field too, even if tests don't currently use it — completeness saves the need to go back and add it later

BTW in general I find it efficient to write these in batch: first get to a set of lines like

int id; String title; String content; int dateCreated;

Then go into multi-cursor mode, with a cursor at the start of each of those lines: for example, in Android Studio on Linux, I double-tap Ctrl, hold the second tap, then hit Down a few times to make more cursors.

Then Ctrl+Left / Ctrl+Right to move by a word at a time; Ctrl+Shift+Right to do so while selecting, to select the name; the usual Ctrl+C for copy will copy the set of different names, and Ctrl+V will paste the different names on their respective lines. So the rest of the line, like Subject<, > get, => has((x) => x., , ', '), I type just once.

That's neat! I usually went line by line replacing the symbols and the string. This sounds more efficient when you have multiple fields.

gnprice · 2025-05-21T21:56:22Z

test/model/saved_snippet.dart

+import '../example_data.dart' as eg;
+import 'store_checks.dart';
+
+void main() {


filename should end with _test.dart

Otherwise I believe flutter test won't include it. (Though even if it did, following a uniform pattern is helpful for us human readers.)

gnprice · 2025-05-21T22:05:23Z

lib/api/route/saved_snippets.dart

+}) {
+  return connection.post('createSavedSnippet', CreateSavedSnippetResult.fromJson, 'saved_snippets', {


Suggested change

}) {

return connection.post('createSavedSnippet', CreateSavedSnippetResult.fromJson, 'saved_snippets', {

}) {

assert(connection.zulipFeatureLevel! >= 297); // TODO(server-10)

return connection.post('createSavedSnippet', CreateSavedSnippetResult.fromJson, 'saved_snippets', {

That doubles as a reminder, when writing code that calls this endpoint, of what feature level to check for before offering the feature in the UI.

gnprice · 2025-05-21T22:06:15Z

test/api/model/saved_snippets_test.dart

+    return FakeApiConnection.with_((connection) async {
+      connection.prepare(
+        json: CreateSavedSnippetResult(savedSnippetId: 123).toJson());
+      final result = await createSavedSnippet(connection,


this file goes in test/api/route/ :slightly_smiling_face: (not …/model/)

gnprice · 2025-05-21T22:09:24Z

test/api/model/saved_snippets_test.dart

+      check(connection.takeRequests()).single.isA<http.Request>()
+        .bodyFields.deepEquals({


As long as we're writing this test, it's only a couple more lines to have it check the other key details of the request too: the method and URL path. So let's include those too.

gnprice · 2025-05-21T22:10:54Z

test/api/model/saved_snippets_test.dart

+      check(result).savedSnippetId.equals(123);
+      check(connection.takeRequests()).single.isA<http.Request>()


nit: the order is a bit more logical if we check the request first, then the response — after all, the request had to happen before we would receive the response

PIG208 · 2025-05-22T01:00:58Z

Thanks! Updated the PR.

PIG208 added the maintainer review PR ready for review by Zulip maintainers label May 13, 2025

PIG208 requested a review from chrisbobbe May 13, 2025 18:50

PIG208 assigned chrisbobbe May 13, 2025

PIG208 force-pushed the pr-saved-prep branch 2 times, most recently from c27690a to c5cc482 Compare May 16, 2025 23:17

PIG208 mentioned this pull request May 16, 2025

Support saved snippets #1391

Open

3 tasks

chrisbobbe reviewed May 20, 2025

View reviewed changes

api: Add savedSnippets to initial snapshot

2842a41

PIG208 force-pushed the pr-saved-prep branch from c5cc482 to bfa5040 Compare May 20, 2025 20:44

chrisbobbe reviewed May 20, 2025

View reviewed changes

PIG208 force-pushed the pr-saved-prep branch from f58db13 to 4facbde Compare May 20, 2025 23:00

chrisbobbe reviewed May 20, 2025

View reviewed changes

chrisbobbe requested a review from gnprice May 20, 2025 23:15

chrisbobbe assigned gnprice and unassigned chrisbobbe May 20, 2025

chrisbobbe 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 May 20, 2025

PIG208 force-pushed the pr-saved-prep branch from 4facbde to 86873a4 Compare May 21, 2025 00:07

gnprice reviewed May 21, 2025

View reviewed changes

api: Add saved_snippets events

882f3aa

PIG208 force-pushed the pr-saved-prep branch from 86873a4 to 92d7489 Compare May 21, 2025 21:06

gnprice reviewed May 21, 2025

View reviewed changes

PIG208 added 2 commits May 21, 2025 20:54

model: Add PerAccountStore.savedSnippets, updating with events

d2b21ea

api: Add createSavedSnippet route

de9ab12

PIG208 force-pushed the pr-saved-prep branch from 92d7489 to de9ab12 Compare May 22, 2025 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and handle live-updates to saved snippets data #1511

Add and handle live-updates to saved snippets data #1511

PIG208 commented May 13, 2025

chrisbobbe left a comment

chrisbobbe May 20, 2025

chrisbobbe May 20, 2025

chrisbobbe May 20, 2025

chrisbobbe May 20, 2025

PIG208 commented May 20, 2025

chrisbobbe left a comment

chrisbobbe May 20, 2025

chrisbobbe May 20, 2025

chrisbobbe May 20, 2025

PIG208 commented May 20, 2025

chrisbobbe left a comment

chrisbobbe May 20, 2025

PIG208 May 20, 2025

gnprice left a comment

gnprice May 21, 2025

gnprice left a comment

gnprice May 21, 2025

gnprice May 21, 2025

PIG208 May 22, 2025

gnprice May 21, 2025

gnprice May 21, 2025

gnprice May 21, 2025

gnprice May 21, 2025

gnprice May 21, 2025

PIG208 commented May 22, 2025

		}) {
		return connection.post('createSavedSnippet', CreateSavedSnippetResult.fromJson, 'saved_snippets', {

		check(connection.takeRequests()).single.isA<http.Request>()
		.bodyFields.deepEquals({

		check(result).savedSnippetId.equals(123);
		check(connection.takeRequests()).single.isA<http.Request>()

Add and handle live-updates to saved snippets data #1511

Are you sure you want to change the base?

Add and handle live-updates to saved snippets data #1511

Conversation

PIG208 commented May 13, 2025

chrisbobbe 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

PIG208 commented May 20, 2025

chrisbobbe 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

PIG208 commented May 20, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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

PIG208 commented May 22, 2025