There are some places where we use <img> tags where I think it's actually just the right thing to do.
`next/image` is good for image optimization, but I don't think it's worth the proxying for Neopets art images that don't actually always have a higher-res version to begin with.
Idk, maybe the species faces could be a decent choice, but right now we're solving it with `srcSet`, and that's fine. Doesn't seem worth migrating, let's just move on with our lives! 😅
I'm still leaving the lint rule on though, because I think it's a helpful reminder. (I don't think it catches `<Box as="img" />` though, which is a shame, because that would be our natural default in this app!)
Yeah, cool, now we use the `next/image` tag, and our images are showing up again!
There's still lint errors for using bare img tags in some places, but I'm not sure I really care…
This was a fun journey! Turns out Next 12 is using a new faster JS compiler called SWC, which had a compiler bug that triggered here!
The incorrect looping behavior caused `libraryUrl` to sometimes be `null` by the time the movie promise completes, because `layer` was set to whatever the `last` layer in the list had been. https://github.com/swc-project/swc/issues/2624
Anyway, turns out this code has been through a few refactors, and the `async` function wrapper is extraneous now! So I've just deleted it and inlined its code. Ta da! lol
Tweaked some of the default Next.js rules, fixed lint-staged for `next lint`, made a few small easy lint fixes. Feels good!
Note that using the `dirs` option in `next.config.js` was causing `lint-staged` to lint _everything_. That's why I edited `yarn lint` to specify the dirs instead: that way, that command will lint all those dirs, but they won't get included in invocations with `--file`.
There are still a few lint errors left after this commit, because our <img> tags aren't working (@next/next/no-img-element). I'll fix those when we figure out what's wrong with images!
Even in production, scrolling is a bit slow! This will preload the pagination one click ahead.
There is a bit of a perf downside, in that if you click through the pages too fast, you'll trigger _extra_ requests. I think that's a net win though, and I'm not gonna try to get cleverer than this right now.
My main inspiration for doing this is actually our potentially-huge upcoming Vercel bill lol
From inspecting my Honeycomb dashboard, it looks like the main offender for backend CPU time usage is outfit images. And it looks like they come in big spikes, of lots of low usage and then suddenly 1,000 requests in one minute.
My suspicion is that this is from users with many saved outfits loading their outfit page, which previously would show all of them at once.
We do have `loading="lazy"` set, but not all browsers support that yet, and I've had trouble pinning down the exact behavior anyway!
Anyway, paginating makes for a better experience for those huge-list users anyway. We've been meaning to do it, so here we go!
My hope is that this drastically decreases backend CPU hours immediately 🤞 If not, we'll need to investigate in more detail where these outfit image requests are actually coming from!
Note that I added the pagination to the existing `outfits` GraphQL endpoint, rather than creating a new one. I felt comfortable doing this because it requires login anyway, so I'm confident that other clients aren't using it; and because, while this kind of thing often creates a risk of problems with frontend and backend code getting out of sync, I think someone running old frontend code will just see only their first 30 outfits (but no pagination toolbar), and get confused and refresh the page, at which point they'll see all of them. (And I actually _prefer_ that slightly confusing UX, to avoid getting more giant spikes of outfit image requests, lol :p)
This is a minor change to clear a console warning, and make intended behavior clearer! You're not supposed to pass `null` as a select value, because it's ambiguous about whether you're looking for the first option or to make this an "uncontrolled component".
Here, I now provide a fallback value, which is an explicit string for the placeholder option. I made the string very explicit, to aid in debugging if it somehow leaks out from where it's supposed to be! (But I also added gating in the `onChange` event, just to be extra sure.)
Huh. `flexAlign` isn't a real Chakra style prop, because it's not a real CSS style. I wonder if I meant `alignItems`? Anyway, this was getting passed down to the DOM element and triggering a console warning. Removed!
Oops, our cutesy feature to show an outfit thumbnail sa a placeholder while the rest of the data is loading was making spurious requests!
I put the `skip` in the wrong place 😅
This caused a request to https://outfits.openneo-assets.net/outfits/null/v/NaN/300.png, which would return a 500.
The user wouldn't see anything, because the image wouldn't show because it failed. But it's a mistake, and it's sending extra requests from the client and to the server, and it's a good one to fix!
Oh oops, I missed this path change when I changed the route to `/user/:id/lists`! This caused searching by email to redirect to the homepage, but with a valid URL in the address bar; and refreshing the page would hit the redirect defined in `vercel.json`, redirect to the new route, and load the correct page.
Fixed!
Like, the little magnifying glass in the "Search all items", you can click it to get taken to the _big_ search page with the autocomplete filters and stuff
Did some stuff in here for parsing the default list ID too. We skipped that when making the new list index page, but now maybe you could reasonably link to the default list? 🤔 not sure it's a huge deal though
I noticed someone using `<pre>` for styling, and thought, sure why not!
I haven't added support for the code block indent thing, and I think that's probably fine?
A lot of DTI lists use old URLs to anchor-link between lists! Here, we rewrite those URLs to match what DTI 2020 expects, so that they actually correctly jump you across the page and aren't filtered out!
The old URLs were glitchy because we weren't escaping the `layerUrls` param… and this will let us take better advantage of the same shared caching as other stuff!
Whoops, `Promise.race` isn't quite what I wanted here. This meant that, if the image promise _fails_ before the movie _succeeds_, the outfit would crash even though it doesn't need to. (And this was happening too often, due to a bug in /api/assetImage!)
Now, we accept whichever _successful_ result loads first, or reject if they _both_ fail.
I tested this by having /api/assetImage always throw, and confirmed that it crashed the outfit before this change, and no longer does after this change!
We update /api/assetImage to accept size as a parameter (I make it mandatory to push people into HTTP caching happy paths), and we update the GraphQL thing to use it in those cases too!
This also means that, if these images seem to go well, we could swap Classic DTI over to them… I want to turn off those RAM-heavy image converters on the VPS lol
Marking this glitch on the Yellow Lutari head today, and oops there isn't UI copy for it yet! Added!
Also fixed some bugs in here, like old text about the position of the pose picker relative to the glitch badge, and I noticed while debugging that `layerUsesHTML5` returns a truthy string instead of a boolean which seems error-prone!
Hmm, the item page in prod is slower than it is in dev? In dev, most items are satisfied by the preloading in ItemPagePreview, but in prod, those same items need to send a separate OutfitItemsAppearance query _way_ after (which, I think just due to queueing, waits for all the items to wait too).
There's an obvious issue in the case of all the Maraquan items lately, because we just don't do the clever cache lookups for non-standard colors at all. But I don't understand why even standard items like the 17th Birthday Party Hat are struggling!
These are just some simple debug statements, hopefully they'll tell us something about the basics of what's happening!
I didn't want to use the word "basic", since "basic colors" generally means like Blue, Red, Green, Yellow… but it was the only one that fit in the space lol
I tried a lot of stuff with "Fits standard pets" and stuff and couldn't get it to work well
Just a little display bug on the homepage. For an item like the "Evil Coconut Half Mask", which was specifically drawn for the standard _and_ major special colors, our previous logic would have said "Baby only" or "Maraquan only" or whatever special color it happened to find first.
Now, we only show the case "Baby only" if it _doesn't_ fit standard pets too.
Note that the Maraquan case is tricky, because the Blue Mynci can also wear Maraquan items lol! For this reason, we check for two standard bodies before declaring that it's meant for standard pets.
I wasn't sure how to fill the space for items that are fully modeled, then realized some basic at-a-glance "who does this fit" would help!
The load time isn't great, I think I need to break out that dependent subquery, but maybe the stale-while-revalidate will cover it well enough at first.
Add a skeleton stripe for the modeling data! Won't show up in most cases because we load fast, but it helps things a lot when it does. (Also, will we keep loading fast with the cache changes on this query?)
To make this fast, I had to tweak the GraphQL resolver a bit to run a filtered version of the query for `newestItems` instead of scanning the full database! But yeah, looking good!
I think I'm gonna want to swap out "Fully modeled" for some insight about who it fits
Okay cool, I noticed that "A Warm Winters Night Background" sometimes animates when other things are playing, but the animations aren't _detected_. (Huh, I actually thought we just didn't schedule ticks in that case? But maybe I'm missing something.)
Anyway, some movies don't use the built-in frames construct to animate, and instead use tweens that hook into the timeline and mutate the stage. Okay! Now we detect those.
This _did_ enable the Play/Pause button on some items that don't actually animate in practice, like the "#1 Fan Room Background", which seems to have an animated string of lights in the corner that got layered incorrectly. Maybe we should add a new glitch type, to flag movies that don't actually animate?
Doing this for two reasons! One is that I want the movie layer component to be a bit thinner in general - I think we might even want to move the fallback image logic out, too.
The second is that I want the onError for something else soon!
Oops, my inbox was getting full of uncaught promise rejections of `loadImage`!
I'm pretty sure they're caused when multiple images in a movie fail to load (e.g. network problems), but we fail to cancel them. So, the first failure would be caught as a part of `Promise.all` in `loadMovieLibrary`, but then subsequent failures wouldn't be caught by anything, and would propagate up to the console and to Sentry as uncaught errors.
In this change, we make a number of improvements to cancellation. The most relevant change for this bug is that `loadMovieLibrary` will now automatically cancel all resource promises when it throws an error! But this improved robustness also enabled us to finally offer a simple `cancel()` method on movie library promises, which we now available ourselves of at call sites, too.
Experiment! Let's see if them being more prominent like this is helpful or annoying 😅
I think this is clunkier in the HTML5 Green Happy Path, but worth it for bringing attention in the error cases.
But I feel like we might tweak this over time!
This is because I want to try adding a search footer to the two-column layout, like in Classic DTI—and so I want more screens that _can_ support two-column layout to use it.
Right, cool, yes, this is the thing about partial data; you need to define the loading condition as "relevant data is missing, _and_ loading is still happening".
Ah right okay, when the `ItemSearchResultV2` doesn't have an `id`, Apollo Cache isn't quite so strong about caching conflicting-y fields, like the different parameterizations of `items`.
With this change, we give the search result object an ID, which helps Apollo cache more confidently!
It's just a serialization of the relevant search fields 😅
That's the last itemSearch call site! I'll probably keep it up for other clients for a while though, esp since it doesn't depend on any additional loaders or anything, it's pretty small overall
Updated the comments to reflect this, and also remembered to make them real docstrings lol!
Now, when you click Prev/Next, we show the page number while the items load, rather than blink it in and out!
This is because we're using itemSearchV2, which makes `numTotalItems` cacheable separately from the paginated `items`. Apollo Cache pretty much does this with zero config, we just have to ask for `returnPartialData`!
I'm gonna make this a bit more powerful later, but just for now, the text "Page 1 of 27" shows up!
I also don't like that the page number has to blink out while we load the new stuff; there are multiple solutions, but tbh I think the Apollo Cache should be the one to handle this, and that we can do it by refactoring the query structure a bit!
I'm seeing uncaught promise rejections in `loadImage`? It's hard to know exactly where it's actually coming from, those _should_ be caught?
My guess is that it's coming from canceled images, which are throwing errors even after loading? I don't totally understand how, because looking back, I don't think the `cancel` method was actually called???
Anyway, I fixed it so cancel actually _is_ called, and that we don't throw errors when the canceled image _correctly_ fails to load.
This should be more robust either way, but hopefully it also stops the flow of errors?
Right, when there are zero layers, we shouldn't say we're loading!
This is a consequence of the HACK below. If we didn't short-circuit the effect when length == 0, then we would go through and successfully load 0 layers.
Movies often have a lot of assets, which are more likely to be cache misses, and take script time to render! So the time until the user sees something is often huge.
Here, we start loading our PNG image at the same time. This is a filesize loading increase, but even in slow connections, it's generally worth it as a _sharp_ improvement in time until you get to see something!
One noteworthy UI weakness here is that we don't show _any_ loading indicator while the image is visible and the movie is still loading. This makes sense from a practical standpoint, but could be a problem when a movie takes a particularly long amount of time. I also want to be cognizant of whether the blink-of-content ever gets annoying! (We could make it fade out 🤔)
In my last change, I didn't try to change the APIs too much, and kept the concept of `crossOrigin` running through `getBestImageUrlForLayer`.
Now, I've moved the `safeImageUrl` call _outside_ `getBestImageUrlForLayer`, by putting it at the call site: We now call `safeImageUrl` from `loadImage` (which needs to know the `crossOrigin` flag anyway!), and at the `img` tag call site.
This simplifies all of the call sites a lot, I think!
I've noticed that our Fastly proxy adds a surprising amount of latency on cache misses (500-1000ms). And, while our overall hit ratio of 80% is pretty good, most misses happen at inopportune times, like loading items from search.
But now that the Neopets CDN supports HTTPS, we can safely switch back to theirs for *most* image loads. (Some features, like downloads and movies, still require CORS headers, which our proxy is still reponsible for adding.)
This forgoes some minor performance wins (like the Download button now requires separate network requests), and some potential filesize reduction opportunities (like Fastly's auto-gzip which we're today using for SVGs, and eventually using their Image Optimizer for assets), to decrease latency. We could still potentially do something more powerful for low-power connections someday… but for now, with the cache miss latency being *so* heavy, this seems like the clear win for almost certainly *all* users today.
I have a hunch that people aren't finding the Download button! I'm not 100% sure what to do about that, but to start, I want right-clicking the image to give you a hint about it 😅
Woo, it's the big UI experiment! Let's see how it plays for folks 😅
Scrolling through a big lists page right now, I think this is a _huge_ improvement, I can get a sense of the lists and what's in them fast, and see matches fast, and dive quickly and with no extra load time when I want more. I'm pleased tbh!
Pulled MarkdownAndSafeHTML into a shared component, and use it on the single list page now too!
I also simplified some of the logic for the item list, because I figure we'll have to give the trade matching stuff its own pass, y'know?
I'm not sure real item data "should" ever do this? Our "Written Word Shower" had a blank description, but I think that was an error on our end.
Anyway, it's clearer than showing infinite loading, so!
Oops, okay, I guess I didn't test the new preview centering stuff with 1200x1200 images, like the Usul's Damask Markings.
Now, I apply a max size to the whole-ass container, and make the parent responsible for centering it.
So I finally started looking into the race condition that makes item previews sometimes fail to load, and as expected, it was that we were trying to load the movie before CreateJS had necessarily loaded. Usually the timing worked out, esp after a reload, but not under certain circumstances!
Anyway, I've been wanting for a while to just bundle them instead. That'll help us more eagerly load them when we need them, and not depend on external CDNs, and remove a bunch of loading state!
So yeah, I had to learn how the `easeljs` and `tweenjs` NPM packages did their bundling, and how to use `imports-loader` to let them just register straight onto `window`! But we got there and it's pretty nice tbh!
Woof, the "Swirl of Power Effect" item tanks my CPU waaay too much
(I bet it's those 7000x7000 PNGs lolol 😬)
Anyway, before thinking about optimizing specific issues, I'm just adding this emergency switch: if we detect FPS < 2 on any layer, we just pause the whole outfit, until the user decides to unpause.
Oh hey, turns out I was missing a step in movie clip stuff! Images aren't just for sprite sheets, but also sometimes they just want the raw images!
Here, we register them, uwu
Items like "Swirl of Power Effect" and probably others work correctly now!
That said, Swirl of Power takes WAY too much CPU lmao, I want to maybe add some kind of automatic kill switch lol
There are a couple spots where we parse SWF URLs to get the ID out! Most visibly, our Support tools were crashing on it. And internally, manifest loading wasn't working. (I'm not sure if this got caught or if it caused crashes in user space? I didn't see them when wearing a failing item)
Anyway, fixed now!
This is because it's the terminology I'm using elsewhere ("Items" and "Closet" are too overloaded in the UI), and because I want to start putting specific lists at like `/user/:userId/lists/:listId`!
I also create a redirect from the old URL, and also from the DTI Classic variant of the URL
Ah right, React state batching doesn't always work how I expect it to. The separate state caused the hook to return and cache `{loading: false, error: null, data: null}`, and then on a _later_ tick the data value showed up, but only _after_ the response was already cached!
This broken a bunch of species/color picker stuff, now it's fixed!
Okay, so getting the initial render down time for these faces is annoying, though I might come back to it…
But actually, the _worst_ part isn't the _initial_ render, which just kinda gets processed as part of the page navigation, right?
The _worst_ part is that we render it slowly _twice_: once on page load, as we send the `useAllValidPetPoses` fetch request; and then again when the fetch request ~instantly comes back from the network cache.
The fact that this requires a double-render, instead of just rendering with the cached valids data in the first place (like how our GraphQL client does), causes a second and highly-visible render of a slow-to-render UI!
So, here we update `useAllValidPetPoses` to cache its response in JS memory, similar in principle to how Apollo Client does. That way, we can return the valids instantly on the first render, if you already loaded them from the homepage or the wardrobe page or another item page!
This is a pretty easy change, that makes re-renders faster when something about the item preview state changes!
That said, the initial render is still pretty slow, too, and that's the one that's bothering me more lol
Ah oops, because I forgot to set `hiResMode` here in the image preloader, we would preload the PNG, and _then_ load the SVG separately.
This doubled the effective image loading time in Hi-Res Mode!
Now, the image preloader respects hi-res mode, and will preload the SVG in the SVG case, and the PNG in the PNG case.
If you're not in hi-res mode, then you don't care about broken SVGs, because you wouldn't have seen them anyway!
We also update the message to reference Hi-Res Mode.
We're just having too many glitchy SVGs for my taste, esp since TNT seems to just be using PNGs for now?
This change defaults us to using PNGs for users by default, with the option to use SVGs as a new "hi-res mode" setting.
This is our first ever setting, wow!
I'm also envisioning that like, if we get Fastly Image Optimizer set up, this could be a way to tune the quality of the incoming images.
We could also consider a setting to turn off animations altogether—like, just download the PNG instead of the movie, whereas right now we download the movie on the assumption that you might play it at any time.
The way we were checking for UC compatibility issues, was also triggering while the appearance was still loading, so items didn't have any appearance layers yet!
Now, we check for loading before testing for that glitch.
