Order carousel as images appear in items / markdown

[ed-kung]

Description

Closes #2197

Uses rehype-sn to add an imgIndex property to each image node in the HTML-AST. A global image index is then created from the formula globalImgIndex = itemId * 100 + imgIndex, which the carousel will sort by.

Carousel sort order will become unpredictable if there are more than 100 images in an item, but otherwise the carousel sorts first by item#, then by the order in markdown

Screenshots

Before:

carousel-before-480.mov

After:

carousel-after-480.mov

Checklist

Are your changes backwards compatible? Please answer below:

yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

7

I did my best to add an index everywhere an image might be rendered, but I'm not 100% sure I got all of them.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

Did you introduce any new environment variables? If so, call them out explicitly here:

no

[ekzyis]

A global image index is then created from the formula globalImgIndex = itemId * 100 + imgIndex, which the carousel will sort by.

Doesn't ordering by this formula assume that items will be shown in the order of their ids? But when you sort by hot, that's not necessarily the case.

[ed-kung]

That's true, let me see if i can preserve item ordering as it appears on the screen.

[ed-kung]

Ok, now it works like this:

• CarouselContext maintains a mapping from itemId to itemOrder, the order in which the items call addItem.
• Based on the order in which ItemFull calls items, this should be the same order as how they appear on screen.
• rehype-sn tags all images in the markdown with an imgIndex property, the order in which they appear.
• itemId and imgIndex are passed to any calls of addMedia, so that a sortKey can be included in the media array: sortKey = itemOrder * 100 + imgIndex
• The carousel's media array is sorted by sortKey which should be consistent with the order images appear on screen
• If the sort field in the router query changes, the Carousel is re-rendered so that images can be re-ordered according to the new order.

carousel-sort-robust-480.mov

[ed-kung]

Hmm, not sure when Cursor started reviewing our code. To the above points, here are my responses:

• Based on my understanding of the code, I don't think addMedia would ever be called before addItem for the same itemId.
• It shouldn't matter if non-image autolinks get an imgIndex, as long as imgIndex is monotonically increasing it doesn't matter if some numbers are skipped.

[ekzyis]

Mhh, I wonder if we can fix #2197 without requiring the Carousel component to be aware of items and having to call addItem in three different places.

As far as I understood, the problem is that the MediaOrLink component calls addMedia as an effect after the image is loaded. Since image load order is non-deterministic, the carousel order is non-deterministic.

However, I think the order of MediaOrLink is deterministic and in the order we want.

So the question is: Do we really have to wait for the image to load before we call addMedia?

I didn't try this out myself, but from my understanding so far, I think we could call addMedia immediately and if the image fails to load, we remove it. The code even already does that.

What do you think?

---

Update:

This is unrelated to the bug and is another of my mistakes in #1425, but I think we also don't need to use useRef for the media map¹ but can use useState. This would fix the anti-pattern of treating refs as dependencies.

I believe I used useRef because I didn't want the whole tree to re-render just because I added a link to the map.
However, this is also possible with state updater functions, so we don't need to pass down the state itself, only setState (which has a stable identity):

If you pass a function as nextState, it will be treated as an updater function. It must be pure, should take the pending state as its only argument, and should return the next state. React will put your updater function in a queue and re-render your component. During the next render, React will calculate the next state by applying all of the queued updaters to the previous state. See an example below.

-- https://react.dev/reference/react/useState#setstate

You can ignore this since this is not really part of fixing #2197. Just wanted to write this down here because reviewing this PR made me realize this.

Footnotes

1. Why is it even a map?? lol ↩

[ed-kung]

Thinking about this more, is it correct to say that addMedia will only wait for the image to load if the src is not from a trusted imgproxy?

addMedia is called when media.image flips to true:

stacker.news/components/media-or-link.js

Lines 80 to 83 in de01d94

	useEffect(() => {
	if (!media.image) return
	addMedia({ src: media.bestResSrc, originalSrc: media.originalSrc, rel: props.rel })
	}, [media.image])

and afaict is this the only place where it would wait for the image to render before setting isImage to true, and this line isn't reached if the src is from a trusted imgproxy:

stacker.news/components/media-or-link.js

Lines 137 to 138 in de01d94

	img.decode().then(() => { // decoding beforehand to prevent wrong image cropping
	setIsImage(true)

Does my assessment sound correct? Just want to make sure I understand the reason for the error. Also, if I'm correct, does that mean the carousel should test for whether the media is an image or a video and then remove if video?

[ekzyis]

Does my assessment sound correct? Just want to make sure I understand the reason for the error.

Yeah, I looked at the code and came to the same conclusion

Also, if I'm correct, does that mean the carousel should test for whether the media is an image or a video and then remove if video?

Yes, the carousel should only show images. Maybe that will change at some point, but for now, I think it's best to keep it that way and only fix #2197.