content: Record href and src straightforwardly on ImagePreviewNode #2077

chrisbobbe · 2026-01-13T19:19:27Z

Notable commit message:

content [nfc]: Record href and src straightforwardly on ImagePreviewNode

I've frequently gotten confused about the roles of `srcUrl` and
`thumbnail`. The former has been especially frustrating while
thinking through all the cases we have to handle. The name `srcUrl`
seemed to suggest (a) parsing something into a Uri object and (b)
representing the `src` attribute in the HTML. Neither was true: it
was a String, and it held the HTML's `href` at least as commonly as
`src`.

To straighten this out, just have ImagePreviewNode hold `href` and
`src`, and transcribe the fields' meanings from the API doc and CZO
discussions. The `src` field is a new type ImagePreviewNodeSrc with
two subclasses corresponding to the thumbnail- and not-thumbnail-URL
cases.

chrisbobbe · 2026-01-13T19:20:33Z

Prompted by #2067 (comment).

gnprice

Thanks for this refactor! Seems helpful.

In retrospect when we changed the meaning of srcUrl (in 7db3044, #820) it would have been good to find a clearer name for its new meaning. I agree that this name really sounds like it means the value of the src attribute, given that this is in the context of something that parsed an HTML img element.

/cc @rajveermalviya: it'll be helpful for you to follow these changes, since you've been involved in this part of the code.

gnprice · 2026-01-13T20:42:01Z

test/model/content_test.dart

+    // and the message in the message event was already in the non-loading form.)
+    null,
+    '<div class="message_inline_image">'
+      '<a href="/user_uploads/path/to/example.png" title="example.jpg">'


nit:

Suggested change

'<a href="/user_uploads/path/to/example.png" title="example.jpg">'

'<a href="/user_uploads/path/to/example.png" title="example.png">'

(matches example in docs, and seems logical)

gnprice · 2026-01-13T22:44:47Z

test/widgets/content_test.dart

+      doTest('thumbnail',
+        rawHref: '/user_uploads/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg',
+        rawSrc: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp',
+        expectUrlInPreview: eg.realmUrl.resolve('/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'),
+        expectLoadingIndicator: false,
+        expectUrlInLightbox: eg.realmUrl.resolve('/user_uploads/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg'),
+        expectThumbnailUrlInLightbox: eg.realmUrl.resolve('/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'),
+        example: ContentExample.imagePreviewSingle);


Recording here a remark I made on our call today: I think for reading these tests it would help to pull out these URLs as local variables. That way it's easier to spot what the pattern is of which of the different expectations are meant to match which of the two URLs at the top. So for example:

Suggested change

doTest('thumbnail',

rawHref: '/user_uploads/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg',

rawSrc: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp',

expectUrlInPreview: eg.realmUrl.resolve('/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'),

expectLoadingIndicator: false,

expectUrlInLightbox: eg.realmUrl.resolve('/user_uploads/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg'),

expectThumbnailUrlInLightbox: eg.realmUrl.resolve('/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'),

example: ContentExample.imagePreviewSingle);

final rawHref = '/user_uploads/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg';

final rawSrc = '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp';

doTest('thumbnail',

rawHref: rawHref,

rawSrc: rawSrc,

expectLoadingIndicator: false,

expectUrlInPreview: eg.realmUrl.resolve(rawSrc),

expectUrlInLightbox: eg.realmUrl.resolve(rawHref),

expectThumbnailUrlInLightbox: eg.realmUrl.resolve(rawSrc),

example: ContentExample.imagePreviewSingle);

Then further changes:

Can make the helper something that's called from the testWidgets body, rather than calling testWidgets itself; that makes a handy scope per test for defining such variables.

Can tweak the API so that the helper takes care of calling eg.realmUrl.resolve.

Moving expectLoadingIndicator out from in between the URLs might help a bit further in seeing the pattern of the URLs.

Oh and one that just came to mind, not from the call:

The thumbnail in lightbox logically comes before the main URL there, right? It's what's used while the other is still loading. So could reorder that earlier.

Can tweak the API so that the helper takes care of calling eg.realmUrl.resolve.

I think I'd prefer if the helper could take the expectFooUrl params literally, so not a string but a Uri, and not a variable with "raw" in it. But the eg.realmUrl.resolve calls do make for a lot of repeated logic. How about a separate helper that deduplicates those calls, like this:

Future<void> doTest() {} // etc. Uri url(String reference) => eg.realmUrl.resolve(reference); testWidgets('thumbnail', (tester) async { final rawHref = '/user_uploads/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg'; final rawSrc = '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'; await doTest(tester, rawHref: rawHref, rawSrc: rawSrc, expectLoadingIndicator: false, expectUrlInPreview: url(rawSrc), expectThumbnailUrlInLightbox: url('/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'), expectUrlInLightbox: url(rawHref), example: ContentExample.imagePreviewSingle); });

Sure, that works.

gnprice · 2026-01-13T23:38:36Z

test/widgets/content_test.dart

+          if (expectUrlInLightbox != null) {
+            check(lightboxPage).isNotNull();
+            check(lightboxPage!.src).equals(expectUrlInLightbox);
+          } else {
+            check(lightboxPage).isNull();
+          }


And recording a nit/suggestion I made on our call: I believe this is equivalent:

Suggested change

if (expectUrlInLightbox != null) {

check(lightboxPage).isNotNull();

check(lightboxPage!.src).equals(expectUrlInLightbox);

} else {

check(lightboxPage).isNull();

}

check(lightboxPage?.src).equals(expectUrlInLightbox);

gnprice · 2026-01-13T23:39:30Z

test/widgets/content_test.dart

+        required String rawSrc,
+        required Uri? expectUrlInPreview,
+        required bool expectLoadingIndicator,
+        required Uri? expectThumbnailUrlInLightbox,


also from our call: it looks like this parameter doesn't get used

gnprice · 2026-01-13T23:43:30Z

lib/model/content.dart

+  ///     when the original image is in an uncommon format, like TIFF.
+  ///     This isn't implemented yet; it's #1268.
+  // TODO(#1268) implement transcoded-image feature; update dartdoc
+  final String href;


Another item from our call: it sounds like when extending this to the new inline images you'll want to call this more like originalSrc; in that case it's probably clearest to call it that within this PR. That's quite closely aligned with what the dartdoc describes this as (the "canonical source URL" vs the "original source"), and it's also closer to the status quo ante than href is.

gnprice · 2026-01-13T23:55:19Z

lib/widgets/content.dart

-      onTap: resolvedSrcUrl == null ? null : () { // TODO(log)
+      onTap: () {


Can some of these rearrangements of logic get pulled into an NFC prep commit? I think that'd help with reading the substantive changes in this file.

I'm thinking of:

this conditional moving to an early return above

this variable being renamed

the node.loading conditional being moved upward

and there are likely another item or two like those. They're all small and probably fit neatly all together into one commit, but it'd be helpful to separate that from the behavior changes.

gnprice · 2026-01-13T23:57:23Z

test/model/content_test.dart

        ImagePreviewNode(
-          srcUrl: '/external_content/de28eb3abf4b7786de4545023dc42d434a2ea0c2/68747470733a2f2f75706c6f61642e77696b696d656469612e6f72672f77696b6970656469612f636f6d6d6f6e732f372f37382f566572726567656e64655f626c6f656d5f76616e5f65656e5f48656c656e69756d5f253237456c5f446f7261646f2532372e5f32322d30372d323032332e5f253238642e6a2e622532392e6a7067',
-          thumbnail: null,
+          href: 'https://upload.wikimedia.org/wikipedia/commons/7/78/Verregende_bloem_van_een_Helenium_%27El_Dorado%27._22-07-2023._%28d.j.b%29.jpg',
+          src: ImagePreviewNodeSrcOther('/external_content/de28eb3abf4b7786de4545023dc42d434a2ea0c2/68747470733a2f2f75706c6f61642e77696b696d656469612e6f72672f77696b6970656469612f636f6d6d6f6e732f372f37382f566572726567656e64655f626c6f656d5f76616e5f65656e5f48656c656e69756d5f253237456c5f446f7261646f2532372e5f32322d30372d323032332e5f253238642e6a2e622532392e6a7067'),


Hmm, interesting that this exposes some URLs we'd been completely ignoring before (the href here).

chrisbobbe · 2026-01-15T02:10:53Z

Thanks! Revision pushed, with quite a few more NFC commits, and the behavior change we discussed isolated in its own commit, plus another I noticed when I fixed a bug in the tests 🙂.

The two behavior changes are (excluding ones caused by bad server behavior, which also have their own commits):

content: Stop dropping LightboxHero in a case where we offer the lightbox
content: Show thumbnail in image preview even if original URL didn't parse

chrisbobbe · 2026-01-15T02:22:58Z

Oops, noted a few fixes after pushing, and pushed again. Dinner time, so I'll leave this here for now.

chrisbobbe · 2026-01-15T21:44:19Z

@rajveermalviya would you give this a review? Greg is out-of-office for several days and won't see this until he gets back. 🙂

rajveermalviya

This is a very clean refactor, thanks @chrisbobbe! Reviewed through all commits and it all looks good to me.

chrisbobbe · 2026-01-22T17:53:23Z

Thanks for the review!

gnprice

Thanks for the revision! This all looks good modulo the comments below. One of them means I haven't yet fully read the next-to-last commit:
4497ea1 content [nfc]: In image-preview widget, make variables for src and href

gnprice · 2026-01-23T02:18:16Z

lib/widgets/content.dart

          message: message,
          messageImageContext: context,
-          src: resolvedSrcUrl,
-          thumbnailUrl: resolvedThumbnailUrl,


I'm looking at this next-to-last commit:
4497ea1 content [nfc]: In image-preview widget, make variables for src and href

to try to confirm it's NFC, and I'm not sure it is.

There's a lot there, so I started by breaking it down into cases: first, what if node.loading is true? (Because that's the first case in the if/else chain this commit introduces.)

In that case, the old code would set thumbnailUrl to node.thumbnail?.resolve( etc. The new code looks like it will set it always to null. So whenever node.thumbnail is non-null and node.loading is true, then it looks like the behavior will differ.

Ah yeah, that makes sense. (As we discussed in the office, I think this particular case isn't a behavior change in this commit because the parser doesn't produce a node whose thumbnail is non-null and loading is true, but this separate commit doesn't increase the branch's readability, so for the next revision I've squashed it with the main commit that follows it.)

gnprice · 2026-01-23T02:22:32Z

lib/model/content.dart

+  /// This is expected to be true (in 2026-01)
+  /// while an uploaded image is being thumbnailed.


nit:

Suggested change

/// This is expected to be true (in 2026-01)

/// while an uploaded image is being thumbnailed.

/// This is expected to be true (as of 2026-01)

/// while an uploaded image is being thumbnailed.

The difference is subtle, but to me "as of 2026-01" suggests (correctly) that we expect the same situation will probably continue for some time, even if we don't know when it might change; whereas "in 2026-01" suggests that we expect the situation is specific to this month.

Makes sense!

gnprice · 2026-01-23T02:24:08Z

lib/model/content.dart

+  ///
+  /// Clients are expected to use this URL when saving the image to the device.
+  ///
+  /// For images processed in modern thumbnailing (2026-01),


nit: something like

Suggested change

/// For images processed in modern thumbnailing (2026-01),

/// For images processed in modern thumbnailing (as of 2026-01),

As is, it sounds like it's saying "modern thumbnailing" itself dates to 2026-01. In fact it's been stable for some time; I believe it dates to Zulip Server 9.0, in 2024.

We don't intend to *use* the `src` in the loading case; it's documented as being a hard-coded "spinner" image URL: https://zulip.com/api/message-formatting#image-loading-placeholders But moving this stanza downward is helpful on the way to unifying the image-preview parsing code with new code for the "inline image" feature, coming up.

This makes a few tests redundant; we'll remove them in a later commit, to not distract from the story of these tests. These tests will help verify an upcoming refactor, where we align ImagePreviewNode more closely with the HTML it's meant to represent.

This refactor helps toward unifying the image-preview parsing code with new code for the "inline image" feature, coming up. The `originalWidth` and `originalHeight` fields may now sometimes be present in the loading case. The widget code doesn't read them -- MessageMediaContainer has fixed dimensions, 100px by 150px -- but it's harmless to let them be there. This commit is NFC with servers with expected behavior, but will start rejecting content in the loading case if `data-original-dimensions` is present and malformed (i.e. not in the "{width}x{height}" format).

I've made a note of this on zulip#297 "Set mouse cursor style for links in message content" so we eventually consider it: zulip#297 (comment) It's not something we'll get around to in the short term, though, so I think the TODO is more distracting than it's worth.

The instance will always have a format we can fall back to, i.e., defaultFormatSrc, so there's no need for this method's return type to be nullable.

…tbox

First, the thing that's meant to be wrapped in a LightboxHero (when we have data for that). Then the LightboxHero, unless we got nothing from the first step, which happens when the URL for the image preview was invalid (and we have a TODO for that).

This highlights the desired behavior in the image preview of not ever falling back to a thumbnailed image's original image.

…parse

…heck And repeat its implementation in the place we compute the URL for the image preview.

I've frequently gotten confused about the roles of `srcUrl` and `thumbnail`. The former has been especially confusing while thinking through all the cases we have to handle. The name `srcUrl` seemed to suggest (a) parsing something into a Uri object and (b) representing the `src` attribute in the HTML. Neither was true: it was a String, and it held the HTML's `href` at least as commonly as `src`. To straighten this out, just have ImagePreviewNode hold `href` and `src`, and transcribe the fields' meanings from the API doc and CZO discussions. The `src` field is a new type ImagePreviewNodeSrc with two subclasses corresponding to the thumbnail- and not-thumbnail-URL cases.

chrisbobbe · 2026-01-23T23:56:37Z

Thanks for the review! Revision pushed.

gnprice · 2026-01-24T01:27:28Z

That's helpful, thanks! I was able to verify that the new squashed last commit is NFC, after working through its logic for three cases:

loading true;
loading false, but thumbnail non-null;
loading false, and thumbnail null.

Looks good; merging.

chrisbobbe · 2026-01-24T01:27:58Z

Thanks!

chrisbobbe requested a review from gnprice January 13, 2026 19:19

chrisbobbe assigned gnprice Jan 13, 2026

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jan 13, 2026

chrisbobbe mentioned this pull request Jan 13, 2026

content: Support inline images using the ![alt text](url) syntax #2067

Open

gnprice reviewed Jan 14, 2026

View reviewed changes

chrisbobbe force-pushed the pr-inline-image-prep branch from 4d63302 to f7726af Compare January 15, 2026 02:07

chrisbobbe force-pushed the pr-inline-image-prep branch 3 times, most recently from 79276d1 to 1f28169 Compare January 15, 2026 02:22

chrisbobbe requested a review from rajveermalviya January 15, 2026 21:43

rajveermalviya approved these changes Jan 19, 2026

View reviewed changes

gnprice reviewed Jan 23, 2026

View reviewed changes

chrisbobbe added 13 commits January 23, 2026 13:23

content [nfc]: Repeat UnimplementedBlockContentNode less

8d26ac3

content test: Add some more image-preview examples, and more comments

159f0f9

content test: Remove some now-redundant tests

6f38a43

image [nfc]: Fix return type of ImageThumbnailLocatorExtension.resolve

2f1efba

The instance will always have a format we can fall back to, i.e., defaultFormatSrc, so there's no need for this method's return type to be nullable.

content [nfc]: s/resolvedSrcUrl/lightboxDisplayUrl/

d023b49

content [nfc]: Factor a nested conditional as a switch/case

b91ee58

content [nfc]: Move a TODO and add issue number; add some missing TODOs

2aa1bd2

content: Stop dropping LightboxHero in a case where we offer the ligh…

48546ac

…tbox

content [nfc]: Pull out a child variable in MessageImagePreview.build

98fffe9

chrisbobbe added 8 commits January 23, 2026 15:36

content [nfc]: Early return when we don't have a URL for the lightbox

8f4d2f2

content [nfc]: Move second step of child computation downward

66b2d7b

content [nfc]: Spell out some logic more explicitly

5f00490

This highlights the desired behavior in the image preview of not ever falling back to a thumbnailed image's original image.

content: Show thumbnail in image preview even if original URL didn't …

0fd6e7e

…parse

content [nfc]: Define lightboxDisplayUrl lower, just above its null c…

ef6003e

…heck And repeat its implementation in the place we compute the URL for the image preview.

content [nfc]: Inline srcUrl and thumbnailLocator

a048bda

chrisbobbe force-pushed the pr-inline-image-prep branch from 1f28169 to df60c98 Compare January 23, 2026 23:55

gnprice merged commit df60c98 into zulip:main Jan 24, 2026
1 check passed

chrisbobbe deleted the pr-inline-image-prep branch January 24, 2026 01:27

	'<a href="/user_uploads/path/to/example.png" title="example.jpg">'
	'<a href="/user_uploads/path/to/example.png" title="example.png">'

		onTap: resolvedSrcUrl == null ? null : () { // TODO(log)
		onTap: () {

		/// This is expected to be true (in 2026-01)
		/// while an uploaded image is being thumbnailed.

	/// For images processed in modern thumbnailing (2026-01),
	/// For images processed in modern thumbnailing (as of 2026-01),

content: Record href and src straightforwardly on ImagePreviewNode #2077

content: Record href and src straightforwardly on ImagePreviewNode #2077

Conversation

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

Uh oh!

Uh oh!

chrisbobbe commented Jan 13, 2026

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!

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!

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

Uh oh!

Uh oh!

chrisbobbe commented Jan 15, 2026

Uh oh!

chrisbobbe commented Jan 15, 2026

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jan 22, 2026

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!

chrisbobbe commented Jan 23, 2026

Uh oh!

gnprice commented Jan 24, 2026

Uh oh!

Uh oh!

chrisbobbe commented Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Jan 13, 2026 •

edited

Loading

chrisbobbe commented Jan 15, 2026 •

edited

Loading