Oops, we were getting errors when people tried to change the species/color picker before the valid pose data was ready!
This was only happening on the homepage and item page, because on the wardrobe page we wait for the valids to load before showing the picker at all (`showPlaceholders` is false).
To fix this, we add the valid poses loading state to our existing `isLoading` state on the selects, which disables the element and adds a loading spinner cursor. This prevents interactions we're not ready for!
I'm not sure why, but people are seeing errors when reading from the /api/validPetPoses binary blob. I think it's the picker not handling loading states well?
In this change, we start by just giving it graceful handling, and improving the logging. I'll also try to fix the cause in the next change!
Oops, we were removing the last word of the search query if you picked a suggestion from Advanced Search! That behavior is meant for the case where you're _typing_ a filter name 😅
Sooo, I added this more graceful regex and error logging… then realized that this shouldn't be happening in the first place, because we should only be removing the last word of the query if you picked the filter via typing, not advanced search!
I'm glad to have the assertion error and the new handler, but I'll fix the cause too in the next change :p
We were getting away with singular stuff like "Hat" in the filter text for a while, but once it became "Hat you own" it got too weird imo!
Now, we say "Hats" and "Hats you own" in the filter text. We keep the singular in the search suggestion, but with the "Zone:" prefix, which is something I've been wanting anyway. (It should help with the show all suggestions UI coming soon, too.)
Oops, the old condition depending on `queryFilterText` to implicitly check for filter presence. But now that we always show "Items" as prefix text for the filters on this page, the reset button was always showing!
Use our new util function instead.
1. Search for something
2. Clear the search bar
3. Quickly start typing something new
Before this change, the results would clear on #2, but then the old results would show up again during #3, before the loading state for the new query.
This matches the logic, right? We hid the results when both the current and debounced query were empty, and, during that time, neither is empty.
Instead, here we update the `useDebounce` hook to have a `forceReset` option, to immediately clear out the value instead of waiting.
I switched from my `_NoAuthRequired` opname hack, to a more robust `context` argument, and it's opt-in!
This should make queries without user data faster by default. We'll need to remember to specify this in order to get user data, but it shouldn't be something we'd like, ship without remembering—the feature just won't work until we do!
Still getting some chunk load errors in my Sentry reporting! My hunch is these are the culprit. I hooope that after this the errors are pretty much gone! If not, then I'm missing something about what causes these failures…
So, we've been behind the latest conversion data for a while anyway, because I don't run the sync very often. But I ran the sync today and noticed that the newly converted SVGs weren't showing up in DTI!
This is because TNT changed the asset manifest structure they use for SVG-only assets. Now, we support both!
To test, I checked the Blue Acara (old-style SVG manifest), the Blue Chia (new-style SVG manifest), and the Floating Negg Faerie Doll (animated clip).
Not really sure how to scale these over time, I feel like some amount of history + blog cuteness could be fun? And like, the ability to catch up if you come back after a couple weeks could be nice. But this seems like a really useful at-a-glancer for folks!
Someone wrote in how, when your search query ends with a string that creates Advanced Search suggestions, clicking on items in the list requires two clicks: one to blur and dismiss the suggestions, and one to actually click the item.
Here, I'm experimenting with just leaving the suggestions open. It doesn't feel _great_, but it definitely feels _better_ than before on this edge case, and I thiiink this only affects this edge case in practice? We'll see if it feels goofy in some cases I forgot tho!
Huh, looks like it's possible for a user's NeopetsConnection record to be missing, despite having the ID on their User record!
Here, we handle that case.
Oops, we hit our Sentry transaction limit after 3 days!
This is in part because we selected a 100% sample rate to start, but also because our app has a lot of client-side navs that don't represent real navigation, and are just to update the state in the URL.
I'm not using most of Sentry's performance tracing features, though! I don't have logging that helps us understand once the page is really done, and I'm only really able to use Web Vitals right now - which only applies to first-time pageload events, anyway. So, that's now all we track!
Woo, it's looking pretty good, I think!
I didn't bother with pagination yet, since I feel like that'll be a bit of a design and eng lift unto itself... but I figured people would appreciate the ability to look up individual items, even if the rest isn't ready yet 😅
When building the code to await auth before sending _any_ GraphQL queries, I didn't realize that auth might be kinda slow. So, I've added a hack to let me mark queries with no user-specific data to skip auth, and applied that to the main queries on the homepage.
I think this is a hint that we might want to change our strategy - e.g. to flip it to hackily mark that auth _is_ required, or to create wrappers or option-builder helpers for logged-in queries, etc.
I also notice that SSR would have resolved this particular case...
This UI generally loads very fast, thanks to the CDN cache, so the flash of skeleton content is more distracting than anything else! We still show it quickly after 300ms, but good network connections should reliably get it loaded before then.
Our host, Vercel, doesn't keep old build files on its CDN after a deploy for very long. This means that, after a deploy that changes a page's bundle, existing sessions that attempt to navigate to it for the first time will fail on the dynamic `import`, because the filename hash has changed.
The best fix I'm aware of for this is to just, reload the page when this happens!
To test this, I did the following:
1. Use `yarn build` to build a prod copy of the site.
2. Use `serve -s build` to start serving it on its own port. (API endpoints won't work, and that's okay!)
3. Don't touch the open copy of the site yet.
4. Make a change to `PrivacyPolicyPage.js`, and `yarn build` again. This simulates a deploy under similar circumstances.
5. Open the Console, tick the "Persist Logs" option, and try to navigate to Privacy Policy. Observe that it logs a ChunkLoadError in the console, and smoothly reloads the page to show you the updated Privacy Policy page.
6. Undo your change 😅
Previously, when you navigated directly to an outfit by typing the URL into the browser or following an external link, the name would stay as "Untitled outfit", even after the outfit loaded.
This was because, when you render an `Editable` Chakra component with `value={undefined}`, it permanently enters "uncontrolled" mode, and providing a value later doesn't change that.
But tbh passing `undefined` down from outfit state wasn't my intention! But yeah, turns out the `?.` operator returns `undefined` rather than `null`, which I guess makes sense!
So, I've fixed this on both ends. I'm now passing more `null`s down via outfit state, because I think that's a more expected value in general.
But also, for the `Editable`, I'm making a point of passing in an empty string as `value`, so that this component will be resilient to upstream changes in the future. (It's pretty brittle to _depend_ on the difference between `null` and `undefined`, as we saw here 😅)
Previously, when you clicked on a saved outfit from Your Outfits, the back button would take you back to the homepage, which was confusing for scanning through stuff! Now, it goes back to Your Outfits if it's yours.
I'm not suuure this is the behavior we want? But it seems intuitive enough!
Previously, if you navigated to /outfits/new without a species or color in the query string, we'd show a blank outfit page, with the species/color picker hidden. Now, we default to a Blue Acara instead!
We don't do anything to handle _invalid_ species/color IDs, but I don't super mind that, because in practice that would require some call site to malform the URL, and I don't super expect that.
This resolves more of the _cause_ of Sentry issue IMPRESS-2020-8, but I'm still wondering how a user got to the URL `/outfits/new?[object+Object]=&objects[]=35185&objects[]=67084`. I'm wondering if the pet loader on the homepage has a bug in Safari? I feel like I heard something like that from the feedback form, too...
If the species/color of the current outfit aren't available yet (e.g. a saved outfit is still loading in), hide the picker altogether. This is because the picker can't handle change events during that time, and it's easier to just hide all this than to add special case handlers like disabled states! (And, while placeholders are often helpful, I'm not sure the placeholder dropdowns are any better than empty space in this case.)
This can also happen when the user loads a page without a species/color ready, like just going straight to `/outfits/new`. I think I might want to add a handler for that, though.
Resolves the direct cause of Sentry issue IMPRESS-2020-8, though I'm not sure how the user got to the URL `/outfits/new?[object+Object]=&objects[]=35185&objects[]=67084` in the first place...
Two fixes in here, for when image downloads fail!
1) Actually catch the error, and show UI feedback
2) Throw it as an actual exception, so the console message will have a stack trace
Additionally, debugging this was a bit trickier than normal, because I didn't fully understand that the image `onerror` argument is an error _event_, not an Error object. So, Sentry captured the uncaught promise rejection, but it didn't have trace information, because it wasn't an Error. Whereas now, if I forget to catch `loadImage` calls in the future, we'll get a real trace! both in the console for debugging, and in Sentry if it makes it to prod :)
I haven't seen anything come in from prod yet, and it's hard to trigger one, maybe because the integration is React-specific? Or maybe it's... not working :p
I can send errors from dev! But just haven't _seeeen_ a prod error come in yet.
Maybe we're just squeaky clean tho :3
My main reason for adding this now is that I'm getting some scattered reports of things not displaying correctly, and I want to start gathering some browser data on that...
I recently confirmed that animations work on iOS (at least one did!), which was going to be my guess of what was breaking...
I took out virtualization for now too, I wanna see how this non-Chakra UI version, with fewer nodes and no tooltips etc, performs on large lists in production.
Huh, I'm not sure why SVGs ever didn't have `crossOrigin: "anonymous"`? The old commit isn't really super helpful for understanding that. Maybe I just didn't notice the problem in that case?
Well, whatever. Let's see if this breaks something else! (I'm also wondering if we should just like, _always_ ask for things with crossOrigin set?)
Oops, if you try to show PosePicker before we have a species/color ready, it sends a bad GraphQL request. No visible user impact, just an unnecessary network call and an error in the console! This happens when you're loading an outfit by ID.
Here, we hide PosePicker if there's no species/color ready yet. This stops the extra request from firing!
When loading an outfit in the wardrobe page, there was an awkward state where the outfit preview loading spinner would vanish and then reappear.
This was because `useOutfitState` briefly reported `loading: false`, then fixed itself after almost immediately—but our OutfitPreview component has a delay before re-showing the spinner.
In this change, we smooth out the loading state, by enabling the second GQL request to start immediately once the first request is done, instead of waiting on a callback to finish.
Oops, when refactoring and adding alt text, I didn't realize the padding for the text would affect the images too! And I forgot to add `overflow: hidden` to round the image's corners. Fixed!
To help the load time for outfits feel shorter, we now reuse the outfit thumbnail from the Your Outfits page as a placeholder!
This doesn't add any overhead when the thumbnail data _isn't_ in your session cache, e.g. if you navigate to the outfit directly. But if we have the thumbnail on hand already, we just show it, easy peasy!
Oops, when switching to @emotion/react, it turns out they no longer support that cute hack I was doing to append suffixes to class names!
Here, I change strategy and let `CSSTransition` apply the plain `exit` and `exit-active` classes to its children, and apply Emotion styles to the child to check for _also_ having those classes.
Oops, the GROUP_CONCAT string was getting cut off! This caused an error trying to look up the name of an item ID that didn't exist, because the ID got truncated partway through.
Oh right, we can't cache objects well when they're missing their ID!
Before this change, selecting an outfit then navigating back would require the outfits to reload. Now, they stay!
That'll still show up when the outfit is still loading, but this lets us use the Apollo cache to show the name instantly if you're clicking through a link from Your Outfits
Still a pretty limited early version, no saving _back_ to the server. But you can click from the Your Outfits page and see the outfit for real! :3 We have a WIPCallout explaining the basics.
This seemed to only show up in dev? But right, I guess it's not happy about passing stuff from ClassNames into a Popover Portal. Move it inside, fixed!
This has been bothering me for a long time, but I couldn't really figure out what to do about it. But tweaking the site bg color a smidge has helped us really add texture to the cards I want to have pop out, like the outfit polaroids!
I kinda went all-in in a burst, but tbh I think it looks great :3
I haven't really touched the wardrobe page with it yet though, that'll probably need some tweaking... for now I'm overriding it to keep the old background!
Looks like there was some kind of runtime conflict when running @emotion/css and @emotion/react at the same time in this app? Some styles would just get clobbered, making things look all weird.
Here, I've removed our @emotion/css dependency, and use the `<ClassNames>` utility element from `@emotion/react` instead. I'm not thrilled about the solution, but it seems okay for now...
...one other thing I tried was passing a `css` prop to Chakra elements, which seemed to work, but to clobber the element's own Emotion-based styles. I assumed that the Babel macro wouldn't help us, and wouldn't convert css props to className props for non-HTML elements... but I suppose I'm not sure!
Anyway, I don't love this syntax... but I'm happy for the site to be working again. I wonder if we can find something better.
Oops, creating a new `SpeciesColorPicker` fn on each render meant that React treated it as a whole new dropdown each time. I've extracted it out into a stable component class, and just pass in the extra props now!
This bug caused changes to kick you out of focus for the dropdown, because it had unmounted and remounted.
This query was very slow! I added an index, and now it's fast!
This code change doesn't actually affect anything, but the comment helps explain what happened, since the index isn't stored in code. (Todo: should I start defining some indexes in our setup files?)
We do animation detection during the preload now, but this wasn't always working correctly: some movies don't actually fully mount the children onto the stage until we start playing. This caused the play/pause button to be missing on the outfit page and the item page, but the animations would still play, depending on the user's saved play/pause state in localStorage.
I saw the short-near-the-front and it just frankly looked awkward? Not sure why I liked it before?
I think this medium at the end of the list is better aesthatically, though it's starting to get a bit messy with the different colors mixed around… but I think there's also a semantic argument that we're keeping the facts about the item together, and the _user-specific_ stuff separate at the end… (putting it at the front would be a good semantic argument too, but I think the NC/NP alignment is too important)
In a previous change, I moved the margin for item badges onto an ItemBadge element… but I didn't think through how that would break the spacing for the loading state of ItemPage. Now, the loading skeleton items _contained_ the badge margin, and so the spacing between badges was shiny skeleton-y.
Here, I replace ZoneBadgesList with a function that just returns the elements, and go back to using Chakra's Wrap component. That will apply the margin to direct children, and the zone badges are direct children now.
One option I'm thinking of in hindsight is an idea I had earlier: Chakra hacks the margin onto _React_ children, but could we use CSS direct child selector instead? A bit trickier to resolve the margin size to the theme's value, but plenty doable… something to consider!
In the previous impl, the buttons variant of the menu would appear on first render, and then the breakpoint stuff would adjust and re-render as the compact nav menu. Now I'm using CSS to show/hide instead!
"Beautiful Green Painting Background" wasn't loading! https://impress-2020.openneo.net/items/75594
```Error building movie clips Error: Expected JS movie library http://images.neopets.com/cp/items/data/000/000/491/491273_31368b3745/491273_2_HTML5%20Canvas.js to contain a constructor named _491273_2_HTML5%20Canvas, but it did not: ssMetadata,Bitmap3,Bitmap5,CachedTexturedBitmap_4183,CachedTexturedBitmap_4184,CachedTexturedBitmap_4185,CachedTexturedBitmap_4186,CachedTexturedBitmap_4187,Symbol20,Symbol8,Symbol4,Symbol7,Symbol2,Symbol1,Symbol9,Symbol2copy,Symbol2_1,_491273_2_HTML5Canvas,properties,Stage```
We already had code to strip out spaces, but not encoded spaces like %20. Now, we decode the URL first, so that space-stripping will work even if it was encoded.
That is, if you're browsing a trade list and you go "oh actually, I _do_ want that!", and click the item page to mark it, then click Back, we'll now update the matching stuff on the trade list page to reflect that it's now a match.
This was just a matter of simplifying the GraphQL query, I think the `currentUserOwnsThis` and `currentUserWantsThis` fields just didn't exist at the time?
We _don't_ yet update your _own_ trade list, if you click through to an item to remove it or something like that. The cache update function isn't too tricky, but it's a bit verbose to implement in Apollo, so I'm not bothering right now!
Oops, the <Wrap> component is nice, but it uses React.Children to apply margin to its _direct_ children, and our badges are not always direct children! (See the new `ZoneBadgeList`.)
I poked my head into how `Wrap` works, and it's honestly pretty simple, so I've applied the same styles manually. Ta da!
Not sure why movie clip building is failing! But it happened outside our try-catch, so it left us in an infinite spinner state.
The repro item is the Spring Topiary Garden Background!
The AMFPHP gateway's json.php endpoint has always had a problem parsing pets whose names start with digits… I've dug into it before, and checked again today, and there really is just no way around it: d584b58e95/core/json/app/Actions.php (L43)
And there aren't any reliable AMFPHP Node libraries out there to make the actual native AMF call.
Buuuut! In today's investigation, I noticed the xmlrpc.php endpoint for the first time. And, wouldn't you know it, there's //great// reliability for something as enterprise-standard as that!
So here, I've switched over to using an xmlrpc client library, which simplifies our calling code //and// makes number pets work correctly 😁 I wouldn't have done it just for the simplification, I think bringing in a library is net more complexity… but getting this finally right is a big relief.
When you navigated directly to ItemPage, the new `safeImageUrl` function would crash during the loading state, because it was trying to safe-ify `undefined`.
Now, I've just made `safeImageUrl` more resilient to that particular kind of unexpected input, by passing through null-y values without change.
When we decided to start out with /api/assetProxy, we didn't know how much the load would be in practice, so we just went ahead and tried it! Turns out, it was too high, and Vercel shut down our deployment 😅
Now, we've off-loaded this to a Fastly CDN proxy, which should run even faster and more efficiently, without adding pressure to Vercel servers and pushing our usage numbers! And I suspect we're gonna stay comfortably in Fastly's free tier :) but we'll see!
(Though, as always, if Neopets can finally upgrade their own stuff to HTTPS, we'll get to tear down this whole proxy altogether!)
I guess something got more picky about the loading sequencing: the fade in animation was happening faster than the cached image could load. Now, we explicitly wait for the image to load (even though we know it's probably cached) before fading it in.
I noticed that, if you're _reading_ the beta callout it's obviously a feedback link, but it's easy to glaze over "Tell us what you think". Here, I've added the word "feedback" to make it stand out on scanning the page, while adding "Got ideas?" to keep it feeling colloquial.
Oops, our movie layer promises don't have a .cancel() method, so calling it crashed our error handler. Now, when there's an error loading a layer and there are HTML5 layers visible, we'll correctly show the "Could not load preview. Try again?" message.
So I broke the Download button when we switched to impress-2020.openneo.net, and I forgot to update the Amazon S3 config.
But in addition to that, I'm making some code changes here, to make downloads faster: we now use exactly the same URL and crossOrigin configuration between the <img> tag on the page, and the image that the Download button requests, which ensures that it can use the cached copy instead of loading new stuff. (There were two main cases: 1. it always loaded the PNGs instead of the SVG, which doesn't matter for quality if we're rendering a 600x600 bitmap anyway, but is good caching, and 2. send `crossOrigin` on the <img> tag, which isn't necessary there, but is necessary for Download, and having them match means we can use the cached copy.)
Oops, I shipped with the images.neopets.com TODO undone! Also, the white background was intersecting with the close X for the feedback form.
In this change, we move the xwee image into our bundle instead of depending on images.neopets.com, and we edit it to have a transparent background, which looks nicer for dark mode. (And we do a srcset!)
Previously I tried to be clever and pre-optimize by putting all the layers onto one canvas… I think this probably helped by batching their paints, but it made fades less smooth by not taking advantage of native CSS transitions, and it made us dip into JS way more often than necessary.
Here, I take the simpler approach: just layers of <img> and <canvas> tags, with each animated layer on its own canvas, and letting the browser handle transitions and compositing, and separate `setInterval` timers to manage their framerates.
I have a suspicion that batching the paints could help performance more, but honestly, maybe that batching is already happening somehow, because things look pretty great on my big-screen stress test now; and so if it _is_ relevant, I want to wait and see after testing on low-power devices.
Not 100% sure on the copy, but I like that it's a bit clearer about the value prop. I tried to work in customization for SEO, but it feels too clunky in a sentence, might need to put it elsewhere in the copy!
Yeah, mm, turns out I don't think it's actually viable to model from Impress 2020, because we can't reasonably set up the SWFs and PNGs in the ways we need, especially for compatibility with Classic DTI.
We can turn this on again later, once Classic DTI is gone, and all assets are converted to HTML5 -- or if we build some kind of bridge to Classic's asset code, or we write new PNG conversion code.
These changes are most relevant for playing around in the dev server, modeing against an empty database. But they'll also help in real-world modeling scenarios! e.g. modeling a new species/color combo is now a bit nicer, we don't show a blank entry in the color picker
this is the last one to get parity with current modeling, I think?? I'm gonna add one more feature though: removing no-longer-used assets from the item
Oops, when building the Support tool to label pet appearances, I didn't realize that there's also a boolean `labeled` field that needs to be true for labeled appearances. Without it, the old app shows the appearance as "Unlabeled".
I also ran this query to fix the rows we'd incorrectly written:
```
mysql> UPDATE pet_states SET labeled = 1 WHERE mood_id IS NOT NULL;
Query OK, 158 rows affected (0.14 sec)
Rows matched: 19640 Changed: 158 Warnings: 0
```
This is mostly because I want to chain the rels after both items and assets save, and I want to be able to specify that stuff a bit more precisely, rather than the like, layers-of-awaits we were building up.
