Track `realmName` and `realmIcon` in database #1860

rajveermalviya · 2025-09-19T19:19:00Z

This doesn't yet handle RealmUpdateEvent, will send a follow-up PR for it.

Fixes: #1036

chrisbobbe

Thanks, this will be helpful! Comments below.

Also:

Does the 'basic happy case' test in test/widgets/login_test.dart need to be updated in order to catch a bug where the values aren't stored properly on login? (Or to be more explicit about testing that?)
Let's leave a TODO(#668) to remember to update the database on events

chrisbobbe · 2025-09-19T20:25:03Z

lib/api/model/initial_snapshot.dart


  final bool realmEnableReadReceipts;

+  final Uri realmIconUrl;


api: Add realmName and realmIcon to InitialSnapshot

nit: realmIconUrl in commit message, right?

chrisbobbe · 2025-09-19T20:28:11Z

test/example_data.dart

    realmUrl: realmUrl ?? _realmUrl,
    realmName: realmName ?? 'Example Zulip organization',
-    realmIcon: realmIcon ?? '$realmUrl/icon.png',
+    realmIcon: realmIcon ?? _realmIcon,


_realmIcon is a relative URL, right? So this changes the behavior when realmIcon isn't passed to eg.serverSettings: before this change, it was an absolute URL (starting with realmUrl); now, it's a relative URL. Does the API specify what kind to expect? We should match that if so, otherwise probably just match the observed behavior. (Same with the initial snapshot.)

API docs don't specify that, but the example response there have "realm_icon": "https://secure.gravatar.com/…" URL, and on CZO the observed URL is /user_avatars/2/realm/icon.png?version=3. So, I guess it can be either ot those (absolute or relative).

Also GetServerSettingsResult.realmIcon was never read before so this change should be nfc, added a note for that in the commit message.

And separated this change to 6e0d9e4.

chrisbobbe · 2025-09-19T20:31:00Z

lib/model/database.dart

+  /// This corresponds to [GetServerSettingsResult.realmName].
+  Column<String> get realmName => text().nullable()();
+
+  /// The organization icon URL of the Zulip realm this account is on.


nit: can remove "organization", it's redundant with "Zulip realm"

chrisbobbe · 2025-09-19T20:31:26Z

lib/model/database.dart

+  /// The name of the Zulip realm this account is on.
+  ///
+  /// This corresponds to [GetServerSettingsResult.realmName].
+  Column<String> get realmName => text().nullable()();


Should we add a line like

/// Nullable just because older versions of the app didn't store this.

here and on realmIcon?

And in migrateLegacyAppData, maybe add lines like these:

--- lib/model/legacy_app_data.dart +++ lib/model/legacy_app_data.dart @@ -94,6 +94,8 @@ Future<void> migrateLegacyAppData(AppDatabase db) async { try { await db.createAccount(AccountsCompanion.insert( realmUrl: account.realm, + // no realmName; legacy app didn't record it + // no realmIcon; legacy app didn't record it userId: account.userId!, email: account.email, apiKey: account.apiKey,

chrisbobbe · 2025-09-19T20:39:34Z

lib/model/database.dart

 import 'package:drift/remote.dart';
 import 'package:sqlite3/common.dart';

+import '../api/route/realm.dart';


db: Store realmName and realmIcon in the Accounts table And update account creation during login to populate these fields from server settings. Updates: #1036

Hmm I haven't seen an Updates: <issue> line before; what does that mean?

Hmm, I just meant to relate to the issue that this commit partly fixes. I guess Related: or the more direct Fixes-partly: fits better here.

chrisbobbe · 2025-09-19T20:45:40Z

lib/widgets/login.dart

+        realmName: Value(widget.serverSettings.realmName),
+        realmIcon: Value(widget.serverSettings.realmIcon),


db: Store realmName and realmIcon in the Accounts table

At this commit, the values are never updated when they change on the server, even when the user registers an event queue. So they can potentially be out-of-date by weeks, months, years; so let's add a TODO here to update them on /register, and remove the TODO in the next commit when that gets done.

chrisbobbe · 2025-09-19T21:11:16Z

lib/model/store.dart

+    required String realmName,
+    required Uri realmIcon,
+  }) async {
+    assert(_accounts.containsKey(accountId));


There's an async gap before this caller, and normally we consider it possible that the account was logged out during an async gap. But I guess in this case, the async gap is for updating an account (await globalStore.updateZulipVersionData), and that can't successfully happen concurrently with deleting the account? Meaning this assert should actually always hold?

Added a check at the callsite of updateRealmData.

rajveermalviya · 2025-09-23T15:24:10Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

chrisbobbe · 2025-09-24T01:16:07Z

Thanks! Looks good except it looks like these are still pending, from #1860 (review) :

Also:

Does the 'basic happy case' test in test/widgets/login_test.dart need to be updated in order to catch a bug where the values aren't stored properly on login? (Or to be more explicit about testing that?)

Let's leave a TODO(#668) to remember to update the database on events

rajveermalviya · 2025-09-25T14:27:37Z

Thanks @chrisbobbe! Sorry for missing those. Pushed another update, PTAL.

chrisbobbe · 2025-09-25T21:06:46Z

Thanks, LGTM! Marking for Greg's review.

gnprice

Thanks @rajveermalviya for building this, and @chrisbobbe for the previous reviews! Apologies this review is a few days delayed.

gnprice · 2025-10-02T00:44:27Z

lib/api/route/realm.dart


  final String realmName;
-  final String realmIcon;
+  final Uri realmIcon;


nit: do this before adding the corresponding field on InitialSnapshot; that way they agree from the beginning

gnprice · 2025-10-02T00:45:25Z

test/example_data.dart

-    realmIcon: realmIcon ?? '$realmUrl/icon.png',
+    realmIcon: realmIcon ?? _realmIcon,


nit: similarly, reorder so this and eg.realmIcon agree from the beginning

gnprice · 2025-10-02T00:52:56Z

test/widgets/login_test.dart

+          realmName: Value('Some organization'),
+          realmIcon: Value(Uri.parse('/some-image.png'))));


This seems like it distracts from the narrative of this test (which is about the web-auth flow, not about the realm name and icon). This is a classic example of data that should exist in order to make things realistic, but is boring.

Can we arrange for eg.selfAccount to already have these fields, with the same boring values that eg.serverSettings puts there? For example that's how this test already handles realmUrl, which is similarly boring.

gnprice · 2025-10-02T01:00:33Z

test/model/store_test.dart

+    check(globalStore.getAccount(selfAccount.id)).identicalTo(updated);
+    check(updated).equals(selfAccount.copyWith(


nit: can make the relationships here more explicit:

Suggested change

check(globalStore.getAccount(selfAccount.id)).identicalTo(updated);

check(updated).equals(selfAccount.copyWith(

check(globalStore.getAccount(selfAccount.id))

..identicalTo(updated)

..equals(selfAccount.copyWith(

gnprice · 2025-10-02T01:06:48Z

lib/model/store.dart

      await globalStore.updateZulipVersionData(accountId, zulipVersionData);
      connection.zulipFeatureLevel = zulipVersionData.zulipFeatureLevel;
    }
+    if (globalStore.getAccount(accountId) != null) {


Can this condition ever fail? /cc @chrisbobbe since you had a related question above.

Before this change, the next step here was to call PerAccountStore.fromInitialSnapshot, which already assumes (and asserts) that the account exists. So if there's a reason that can be untrue, then that's an existing bug.

But I don't think it can be. The step before this was either the previous conditional — which has a ! that throws if the account doesn't exist — or globalStore.updateZulipVersionData, which calls updateAccount, which returns the updated account and so guarantees that it exists.

rajveermalviya · 2025-10-07T13:14:00Z

Thanks for the review @gnprice! Pushed an update, PTAL.

gnprice

Thanks for the revision! This is quite close now; comments below.

What's the reason the PR and commit are labeled as fixing #1036 only partly — what's the remaining task you see as open before closing that issue?

gnprice · 2025-10-07T18:30:10Z

test/example_data.dart

-    realmIcon: realmIcon ?? '$realmUrl/icon.png',
+    realmIcon: realmIcon ?? _realmIcon,


This commit message doesn't really capture what this change does:

example data [nfc]: Use _realmIcon as default value in eg.serverSettings NFC, because GetServerSettingsResult.realmIcon was never being read.

It's true that the new value is called _realmIcon here; but more significant is that it's a different value, and in particular a relative URL where it previously was absolute.

(Related previous subthread above: #1860 (comment) .)

Then as a nit, I'd label this as not NFC. It's true that it's NFC for the tests as a whole, if this field is never read; but the change is already labeled as being a change to example data, and it does change the interface presented by example_data.dart: this piece of the data is now a relative URL instead of absolute.

gnprice · 2025-10-07T18:41:38Z

test/widgets/login_test.dart

+            id: testBinding.globalStore.accounts.single.id,
+            // TODO store value from server settings during login flow
+            realmName: Value(null),
+            // TODO store value from server settings during login flow
+            realmIcon: Value(null)));


These get added in one commit and removed in the next. I think what that's pointing to is that the intermediate state between these two commits is somewhat inconsistent. Let's squash them together:
afbb80f db: Allow storing realmName and realmIcon to the Accounts table
ee1993f login: Store realmName and realmIcon while creating account

The resulting diff is only very slightly bigger than the first commit, because the second commit is fairly small already and much of it is reverting parts of the first one.

gnprice · 2025-10-07T18:43:19Z

test/widgets/login_test.dart

+          .equals(eg.selfAccount.copyWith(
+            id: testBinding.globalStore.accounts.single.id,
+            realmName: Value('Some organization'),
+            realmIcon: Value(Uri.parse('/some-image.png')),
+            zulipFeatureLevel: 427,
+            zulipVersion: '12.0-dev-524-ga557b1e721',


Ah, good idea to test all these.

This could also go as a prep commit (with just the three version fields) — then the commit that adds the name/icon fields just adds those to this test case, rather than introducing the whole test case.

Observed value for CZO is: /user_avatars/2/realm/icon.png?version=3 A relative URL rather than absolute, so change it to match that observed value. API docs do not specify a format, see: zulip#1860 (comment) Also `GetServerSettingsResult.realmIcon` was never read before so it's NFC change in regards to the tests.

rajveermalviya · 2025-10-08T13:43:11Z

Thanks for the review @gnprice! Pushed an update, PTAL.

what's the remaining task you see as open before closing that issue?

This PR doesn't include handling of realm update events for these fields, so I used Fixes-partly instead of Fixes.

gnprice · 2025-10-08T22:42:46Z

Got it, thanks.

We have #668 tracking that, and this PR adds a TODO(#668) comment to ensure we see these fields when we go to do that issue. So I think we can declare #1036 complete without that part — the motivation for doing #1036 now is that we need the data for uses like #1779 and #1038, and for those it's perfectly fine if the name and icon don't update until the next fresh event queue.

Otherwise, this revision all looks good! So I'll edit that bit of metadata (in the commit message and the PR description), and merge.

Observed value for CZO is: /user_avatars/2/realm/icon.png?version=3 A relative URL rather than absolute, so change it to match that observed value. API docs do not specify a format, see: zulip#1860 (comment) Also `GetServerSettingsResult.realmIcon` was never read before so it's NFC change in regards to the tests.

And update login flow to store realmName and realmIcon while creating account in the database. Fixes: zulip#1036

This will populate these fields for the currently opened account in the database when the PerAccountStore loads.

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Sep 19, 2025

rajveermalviya assigned chrisbobbe Sep 19, 2025

rajveermalviya requested a review from chrisbobbe September 19, 2025 19:22

chrisbobbe reviewed Sep 19, 2025

View reviewed changes

rajveermalviya force-pushed the pr-realm-name-icon branch 2 times, most recently from 3abe521 to 4403816 Compare September 23, 2025 15:20

rajveermalviya requested a review from chrisbobbe September 23, 2025 15:24

rajveermalviya force-pushed the pr-realm-name-icon branch from 4403816 to 035eb20 Compare September 23, 2025 19:51

rajveermalviya force-pushed the pr-realm-name-icon branch from 035eb20 to 8315782 Compare September 25, 2025 14:26

chrisbobbe requested review from gnprice and removed request for chrisbobbe September 25, 2025 21:06

chrisbobbe assigned gnprice and unassigned chrisbobbe Sep 25, 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 Sep 25, 2025

rajveermalviya force-pushed the pr-realm-name-icon branch from 8315782 to 765d19e Compare September 30, 2025 14:24

rajveermalviya mentioned this pull request Sep 30, 2025

share: Support switching accounts #1883

Merged

gnprice reviewed Oct 2, 2025

View reviewed changes

rajveermalviya force-pushed the pr-realm-name-icon branch from 765d19e to 4889669 Compare October 7, 2025 13:13

gnprice reviewed Oct 7, 2025

View reviewed changes

rajveermalviya force-pushed the pr-realm-name-icon branch from 4889669 to ea0b4d5 Compare October 8, 2025 13:35

api: Change GetServerSettingsResult.realmIcon type to Uri, from String

66a0c17

rajveermalviya added 6 commits October 8, 2025 15:42

api: Add realmName and realmIconUrl to InitialSnapshot

d4d844a

login test: Ensure account created with data from server settings

32557ca

db: Allow storing realmName and realmIcon to the Accounts table

c29934a

And update login flow to store realmName and realmIcon while creating account in the database. Fixes: zulip#1036

store: Update account with realmName and realmIcon from InitialSnapshot

a8b236d

This will populate these fields for the currently opened account in the database when the PerAccountStore loads.

store [nfc]: Add getters for realmName and realmIcon

673ff5b

gnprice force-pushed the pr-realm-name-icon branch from ea0b4d5 to 673ff5b Compare October 8, 2025 22:44

gnprice merged commit 673ff5b into zulip:main Oct 8, 2025
1 check passed

gnprice mentioned this pull request Oct 8, 2025

Track each realm's name and icon in database #1036

Closed

		realmName: Value(widget.serverSettings.realmName),
		realmIcon: Value(widget.serverSettings.realmIcon),

		realmIcon: realmIcon ?? '$realmUrl/icon.png',
		realmIcon: realmIcon ?? _realmIcon,

		realmName: Value('Some organization'),
		realmIcon: Value(Uri.parse('/some-image.png'))));

		check(globalStore.getAccount(selfAccount.id)).identicalTo(updated);
		check(updated).equals(selfAccount.copyWith(

Track realmName and realmIcon in database #1860

Track realmName and realmIcon in database #1860

Uh oh!

Conversation

rajveermalviya commented Sep 19, 2025 • edited by gnprice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

rajveermalviya commented Sep 23, 2025

Uh oh!

chrisbobbe commented Sep 24, 2025

Uh oh!

rajveermalviya commented Sep 25, 2025

Uh oh!

chrisbobbe commented Sep 25, 2025

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!

rajveermalviya commented Oct 7, 2025

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!

rajveermalviya commented Oct 8, 2025

Uh oh!

gnprice commented Oct 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Track `realmName` and `realmIcon` in database #1860

Track `realmName` and `realmIcon` in database #1860

rajveermalviya commented Sep 19, 2025 •

edited by gnprice

Loading