Oops, we did an in-place sort on the search variables we passed to Apollo! This meant that Apollo's first read of the variables wouldn't match later reads, so it would always decide the variables had changed, causing an infinite re-render loop.
Remember to copy existing arrays before sorting! 😅
Incidentally, this only happened for Markings, by coincidence: it's the only (I think) searchable zone label with multiple zone IDs, that don't sort alphabetically the same as they sort numerically. This `.sort()` sorts them alphabetically, whereas they come in numerical order in `allZones`, because that's the order the GQL server returns them in `build-cached-data.js`.
There have been usability problems with this search filter UI, and I think they mostly come down to people accidentally selecting filters when they don't mean to—sometimes pressing Enter to indicate that they're done typing, but accidentally selecting something.
Here, we remove that behavior, and additionally add a new behavior to clear the suggestions on pressing Enter.
We've been serving images directly from `impress-asset-images.s3.amazonaws.com` for a long time. While they serve with long-lasting HTTP cache headers, and the app requests them with the `updated_at` timestamp in the query string; each GET request still executes a full S3 ReadObject operation to get the latest version.
In the past, this was only relevant to users on Image Mode, not Flash Mode. But now that everyone's on Image Mode, this matters a lot more!
Now, we've configured a Fastly host at `impress-asset-images.openneo.net`, to sit in front of our S3 bucket. This should dramatically reduce the GET requests to S3 itself, as our cache warms up and gains copies of the most common asset PNGs.
That said, I'm not sure how much actual cost impact this change will have. Our AWS console isn't configured to differentiate cost by bucket yet—I've started this process, but it might take a few days to propagate. All I know is that our current costs are $35/mo data transfer + $20/mo storage, and that outfit images are responsible for most of the storage cost. I hypothesize that `impress-asset-images` is responsible for most of the reads and data transfers, but I'm not sure!
In the future, I think we'll be able to bring our AWS costs to near-zero, by:
- Obsolete `impress-asset-images`, by using the official Neopets PNGs instead, after the HTML5 conversion completes.
- Obsolete `impress-outfit-images`, by using a Node endpoint to generate the images, fronted by a CDN cache. (Transfer the actual data to a long-term storage backup, and replace the S3 objects with redirects, so that old S3 URLs will still work.)
I hope this will be a big slice of the costs though! 🤞
(Note: I'll be deploying this on a bit of a delay, because I want to see the DNS propagate across the globe before flipping to a new domain!)
Boom, pulled some stuff out into util! Now we also have error boundaries in both panels of the wardrobe page.
You can test this one by visiting `/outfits/new?send-test-error-for-sentry`, just like on the home page! Util component for fake errors owo
Previously, we would tear down to a blank white page. Now, for errors within most page content, we show a cute error message with a grundo programmer!
To test, visit `/?send-test-error-for-sentry`, which will trigger an intentional render error on the home page.
Note that this does _not_ cover pages that don't use PageLayout, namely the wardrobe page! I'll want to add other boundaries there…
Crashes on clearing the search box. I really thought I fixed this when I refactored `forceReset` to be a function, but I guess I missed it when I went back-and-forth deciding whether to actually do the refactor!
Before this, if you made a change while the outfit was auto-saving, it would reset your changes back and forth in an infinite loop, oops!
This was because the response from the save would reset the outfit state to match, but the _debounced_ outfit state would still show the user's changes, so we'd trigger another save. And then the same thing would happen in reverse, and back and forth again!
Oops, it was possible after saving an outfit to get into a state where we would show the `<OutfitThumbnailIfCached />` behind the outfit even after it was saved, and then removing items would look weird until auto-saving caught up.
We had used the `backdrop` property because we wanted smoother partial load-ins, but for now I'm just fixing this by switching it to `placeholder`, which already has the right loading-only behavior.
This was also the only call site for `backdrop`, so I've removed it!
Oops, the sequence here was:
1) Save a new outfit
2) The debounced outfit state still contains id=null, which doesn't match the saved outfit, which triggers an auto-save
3) And now again, the debounced outfit state contains the _previous_ saved outfit ID, but the saved outfit has a _new_ ID, so we save the _previous_ outfit again
and back and forth forever.
Right, ok, simple change: if the saved outfit ID changes, reset the debounced state immediately, so it can't even be out of sync in the first place! (I also considered checking it in the condition, but I didn't really understand what the timing properties of being out of sync due to debouncing would be, and it seemed to not represent the reality I want.)
Hope it actually work-works lol
Did some refactors in useOutfitState to support the new reset action we do after auto-saving, in case the server tweaked things like the name.
Note that we implemented the actual horn behavior described in the message, simply by marking the yellow horn appearance glitched for Fem, but not for Masc! Also, we don't have a yellow-horn Sick Masc model, so it's blue too.
It wouldn't open, because I'd set `isLazy` on the popover, so opening the modal would close and UNMOUNT the popover, which unmounted the modal!
Now, we use the new `lazyBehavior` prop to keep it mounted _after_ the first time it opens. This is why I needed to upgrade Chakra!
Oops, I made a recent change to automatically add `appearanceId` to the outfit state when you open the Support pose picker, to avoid navigation issues.
But I didn't realize this happened _silently_ when you open the page as a Support user, because the Popover preloads!
Now, the Popover doesn't preload its content. This is probably better for normal users too, the PosePicker UI is a bit heavier with 6 previews than I really want!
Oops, our "items to reconsider" feature was preventing unwearing/removing items you're already wearing!
This feature helps you try stuff in Search, without disrupting your outfit. e.g. if you try on a new Background, then change your mind and unwear it, then we reapply whatever old Background you had on the outfit before.
But this made it impossible to remove your _current_ background from the search page if you went back and searched for it again, because we would remove it and then reconsider and reapply it 😅
Now we, um, stop that!
Huh, dunno when I regressed this! Or maybe I never did it for search results, just the main items page? But we're needlessly re-rendering the entire search results list when you wear/unwear something, because `onRemove` always changes, and that breaks the `React.useMemo` on `Item`.
Now, we cache the `onRemove` callback with `React.useCallback`, so perf is much happier!
Oops, we extracted Support fields out from the default `appearanceLayerFragment`!
This was causing the page to silently fail to show any changes, because `layer.remoteId` was evaluating to `undefined` rather than one of the ID numbers in the range.
Here, I've added both `remoteId` explictly because we use it directly, and also the support fields because that's what the layer support UI needs!
Oops, making changes in PosePickerSupport would sometimes trigger a re-fetch in PosePicker.
Specifically, PosePicker needs some fields that PosePickerSupport doesn't, so changing the canonical poses causes PosePicker to ask for stuff again—which will probably serve a SWR'd cached version that doesn't reflect the Support changes!
Here, we update the PosePickerSupport query to prefetch all the fields the PosePicker _would_ want for any of these poses. That way, if we swap in a new one as the canonical appearance for a pose, there's no refetch needed, and therefore no risk of hitting a stale cache.
We move to an actual GQL query, instead of approximating with /api/validPetPoses.
Notable changes are omitting glitched states from UNKNOWN, so we don't prompt Support users to fill in missing states with bad states; and omitting glitched states from standard, so that we _do_ prompt Support users to check UNKNOWN states for new _non-glitched_ versions we can start to use.
Now, when viewing a saved outfit that you own, you'll see a "Saved" indicator if it matches the version on the server, or a temporary UI of "Not saved" and a tooltip if not.
Auto-save coming next!
Previously, the PNG link for a pet layer would show the 150x150 version. This was both an inconvenient size, but also not reflective of how the layer actually behaved, because we only use Neopets's official PNG for the 600x600 version!
Ah, oops, the `id` field from `useOutfitState` went missing and I didn't notice, so `useOutfitSaving` didn't correctly detect that this was an existing outfit!
This made saves on existing outfits create new copies, which isn't a bad behavior exactly, but I don't want to go there; saving a copy is just gonna pollute people's outfit lists rn, worse than no option imo.
Just a basic e2e starting point! Simple logic, with simple gates to prevent saving outfits we're not ready for. Safe to ship, despite being very incomplete!
The layer preloader already takes advantage of, and primes, the HTTP cache.
But we still do duplicate work, on every OutfitPreview render, to re-execute movie clip libraries, and create a movie clip to test for animations. The former is nontrivial cost, and the latter is often large cost. This can make even basic outfit changes slow, when there's no change to the movie clip layers and the player is paused!
Here, we add an LRU cache for movie clip libraries, and for the question of "is it animated?". This should speed up a number of places where we would reload the movie (including between toggling the item), and various changes that were triggering full movie clip rebuilds unnecessarily.
We _aren't_ solving here for the fact that toggling an animated item requires rebuilding the movie clip, which could conceivably be cached—but with some state management trickiness, because ideally it should be a separate clip for each context where it's being shown. Imo not yet worth the effort! (esp because I think users understand that toggling an animated item can be slow, whereas this was affecting _other_ actions way too much)
This is a glitchy state that pets can get into! `spankaroonie` is an example, at time of writing.
Before, we would crash on loading downstream fields for the pet's color. Now, we don't! We also fix an oversight in the pet's `petAppearance` field, to trigger the "not yet modeled" error when the pet type doesn't exist.
Here, we consolidate useOutfitState to get its base `outfitState` from a few different places: parsing the URL, and processing the saved outfit data, return an object of the same shape as the state stored in the reducer.
This enables us to just pick one of the three, instead of our kinda awkward individual-field fallbacks.
This will also help us with some upcoming work to make Back/Forward navigation work better.
Now we show a message for pet layers with the DISPLAYS_INCORRECTLY_BUT_CAUSE_UNKNOWN glitch applied! Previously, there would just be no message, because we only had UI logic for when it was applied to an _item_ layer.
Some of the "MiniMME11-S1: Approaching Eventide Skirt" visuals are pretty clearly glitched on TNT's end, like the Jubjub, which just has a single flat version of the dress floating in the corner of the screen.
This is a message to make that case even clearer!
I'm applying this to the "MiniMME11-S1: Approaching Eventide Skirt" on the Acara, which seems to load all 1000 images from the manifest, but then show no animation and no errors. Not sure what's up, and not inclined to deep-debug until we have a check on whether it works on-site!
Huh, so apparently the "MiniMME11-S1: Approaching Eventide Skirt" on the Acara has 1000 layer images lol.
This caused the manifest string to overflow the MySQL `TEXT` field, and fail to parse as valid JSON when loading it back for the client.
I've updated the database to use `MEDIUMTEXT` instead, and added a warning message & skip behavior when the manifest size would exceed the database limit, and added graceful error handling for the invalid JSON scenario. Now, we don't crash, and the data self-repairs, and keeps in a better state in the first place!
But I'm also worried about this asset, it doesn't play correctly anyway, and I'm not sure if that's an overload on our end, or just a flat problem in the JS. (There's no error message on the client, it just… loads all the layers, then shows no play button, seemingly self-satisfied.)
This was causing a bug where unlabeled poses would cause pet lookups to fail!
Now, we return the actual pet appearance for the pet, if we have it, by matching against asset IDs first.
I'm setting up the app in a fresh box, and I noticed that the Auth0 credentials are an immediate crasher if not present. That doesn't seem ideal to me for something only used in support actions! I'd rather just have that support mutation crash, if we happen to call it.
Oh right, our preview deploy was loading the prod allWakaValues data! Now it uses the VERCEL_URL env variable to request from the current deployment instead.
One day is too long! I'd prefer 1min for just the value itself, but I don't want to bog down all the other metadata with it, it's not _essential_ for it to be faster.
I wanted the ability to clear out closet list text for Support users, and figured I should just build the UI for end users too, and grant Support users the same access!
I narrowed down the problem to the fact that we were joining in pet types against assets, and *then* running GROUP and DISTINCT and everything. Assets x compatible species/color pairs is a LOT of rows!
Here, we instead get all the relevant body IDs first, and *then* match them against pet types—which we fetch in one batch to match body to canonical species/color.
I'm also trashing the weird caching mechanism we did here, because in practice it doesn't seem reliable anyway. If anything, I'd want to look at stronger CDN caching. (I made a small improvement to the caching annotation, but ultimately it still doesn't matter, because this query uses logged-in stuff and always comes out max-age=0 anyway.)
This helps with items like "Living in Watermelon Foreground and Background", which has a species-specific foreground and bodyId=0 background.
With this flag set on the background, it won't appear for pets that don't _also_ have something else that fits. In this case, it hides it from Standard Vandas, and all non-standard colors.
There's some hacky limitations here: the item page still highlights the Vanda, even though clicking gives nothing; and the zone info for it is messy too, with the Background claiming to fit all species, and the LFI claiming to fit 54 specific species. But those don't seem important enough to code for!
The other day, I deleted what was apparently a load-bearing glitch row, lol 😂
We had a row in pet_types that somehow had `body_id = 0`. And I guess that was causing this query to return some species, even though that body has no species.
Here, I'm adding support for the special `representsAllBodies` body's species to be null. The client seems chill with it, we weren't using that property in that situation anyway!
I like making these more concise and consistent across the dropdown and the cards, I'm still not 100% sold on the design but it seems ok for now!
At first I had it in the dropdown as "Background (3)", but realized that conflicts with the usual pattern of like, saying how many items match a certain filter…
Using this to get an at-a-glance check on how Neopets IDs are typically assigned for body-specific items… looks like it's increasing, and alphabetical by species? (not by species ID?)
I think this will be generally useful to minimize switching around for common operations, but also I'm thinking of building a bulk assign tool for things with broken body IDs, and this will be the place for it to live, I think
Oops, we weren't doing a good job encapsulating the different conditions in item search. The `OR` in the NC condition was causing a precedence problem!
Now, we wrap all the conditions in parens at the interpolation site, to make it really clear that they all need to be made safe like that!
Now, there's not a bunch of "??????" entries in NC search, oops 😅
Oops, the HTML5Badge was using the presence of an `svgUrl` to decide if the item is converted!
Here, we add an extra condition that if the OFFICIAL_SVG_IS_INCORRECT glitch is applied, then *that* indicates HTML5 conversion, too.
This was one more bit that needed fixing for "Flying in an Airplane": it wasn't just the official SVG that was incorrect, but also the official SWF. So our converted PNG was also incorrect!
Here, we now try to use the official Neopets PNG when the manifest provides it, instead of our own.
Inspired by the "Flying in an Airplane" bug (item 82287), where the official SVG (and I think SWF) were visually glitched and included both zones in the image, but the official PNG was correct.
This flag lets us use the PNG, like the official player does—but only for this item, while still keeping SVGs for everyone else!
I've decided that covering up the species faces with other species info is too weird! It feels like it's removing some ability to cross-reference.
A cool UI affordance would be to have this and the faces interact with each other, like you can hover to highlight the relevant species faces, or even vice-versa, to show the relevant zones for this species. But that's probably way overkill for this relatively niche feature.
Some looked really bad in the new design, like Jewelled Staff, which was breaking between the words "2 species", making a real bad tooltip target too.
Now, there's no line breaks allowed inside a list item at all! We force it to break between items, instead. (Could have also maybe implemented this with flex wrapping? This seemed like a straighter path, but…)
Oops, I never actually saw the practically invisible text in light mode! Let's make it actually dark in light mode item pages, and still dark in all wardrobe pages!
Here, we offer a second syntax for `<OutfitPreview />`: a hook that offers the same UI as `preview`, but _also_ shares the `appearance` data.
This makes it easier to have UI that depends on the outfit appearance, without having to commit to all the `useOutfitAppearance` stuff in the parent. Same easy syntax! :3
I've refactored the item page to use this for compatibility testing, instead of using the Apollo cache (which was also cute and same perf impact, but more overhead!)
Oops, I got distracted partway through typing the domain, lol! They point to a real place now, lol! (not a very helpful place, but at least the real one I intended! :p)
When I added this new error case in the last change, I made it log to Sentry, because I don't think this should be possible under our data set, so if it happens I want to hear about it. Same is true for this error case, so let's log it too!
A crasher, fixed! :) I made Jetsam Lunch Lady Gloves no longer crash the page, lol - its thumbnail URL is "/items/clo_jetsam_lunchladygloves.gif", with no host specified. The shoes are the same!
I also added a fallback, to return a placeholder error URL instead of just letting the URL through as-is—and I updated the other error case to behave the same. I'd rather have a specific isolated feature get crashy, than have the mixed content warning pop up, or let through some mystery unparseable URL that, idk, might be part of an attack?? Seems better to fail hard-but-small than easy-but-potentially-leakily.
I'm gonna extend `itemSearch` to also look up the total number of results, and the fragmentation between `itemSearch` and `itemSearchToFit` finally caught up with me :p
I've deprecated `itemSearchToFit`, and moved the fit parameters into a new optional `fitsPet` parameter for `itemSearch`.
I'm going to keep `itemSearchToFit` around for now, because old JS builds still use it, and I'd like to avoid disrupting folks. But I'm not going to add the new total results field to the results object it returns, and that's gonna be okay!
I was starting to write a Cypress test, and noticed there was no placeholder to use for searching, and I don't know how that escaped my notice for so long! I guess I commented it out for some reason, but I forget why, and this seems fine now! (Looks like we removed it when we added zone suggestions? Idk!)
I haven't been keeping these up to date, so at this point they're more overhead than they're worth.
Helpful in the early days when we were iterating fast and making more mistakes, but now we're more solid (and I learned how to just resend queries from devtools :p)
Oops, the new `canonicalAppearance` arguments couldn't handle being omitted rather than being null, due to a low-level SQL call site that cares about the difference.
This meant that loading an item page in an old tab, with an old copy of our JS, could cause a crash.
Now, the backend will be okay with queries from old pages, and respond the same as before!
This isn't a huge deal, because once everyone is on the new JS we won't send queries without this parameter anyway… but I like my optional arguments to actually BE optional, without surprises lurking >.>
Oh right, I left in a hack to just always pick HAPPY_MASC or HAPPY_FEM, back when it was just the basic colors. Now that we're all the colors, we need to be able to handle fallbacks for missing or unlabeled poses, too!!
Previously, I kept this constrained to valid species/color pairs only, because we didn't have a great error state. But at this point, it feels worse to throw people out of the color they're looking at, and to make it harder for them to pick the color in the first place!
Wowie, this was hard to get right, but I'm very pleased with where it ended up!! React `key` stuff was a total brainwave, and even though it depends on kinda obscure knowledge, it made this whole thing WAAAY easier, omg
Ok cool, so apparently another win we get from using `ts-node` is that I can finally easily use some non-native-Node features like ES module import syntax, for consistency with what I'm doing in the main app source! That was getting on my nerves tbh. Ooh I bet I can finally use `?.` too, I've had to rewrite that a bunch…
`yarn build` was crashing on my `build-cached-data` script, because we were trying to run the Typescript file uncompiled!
Now, we run it with `ts-node`, which transparently compiles Typescript files before execution. Phew!
`react-scripts build` made some automated changes to `tsconfig.json` for compatibility with `create-react-app`, and I also added a `ts-node` section to override one of them so we can compile to CommonJS for `ts-node` script execution!
Oh yay, I'm pleased with this! I hope it works out well!
stale-while-revalidate is an HTTP caching feature that gives us the ability to still serve relatively static content like item pages ASAP, while also making sure users generally see updates quickly.
The trick is that we declare a period of time where, you can still serve the data from the cache, but you should _then_ go re-fetch the latest data in the background for next time. This works on end users and on the CDN!
I've scanned the basic wardrobe and homepage stuff and brought them up-to-date, and gave particular attention to the item page, which I hope can be very very snappy now! :3
Note to self: Vercel says we can manually clear out a stale-while-revalidate resource by requesting it with `Pragma: no-cache`. I'm not sure it will listen to us for _fresh_ resources, though, so I'm not sure we can actually use that to flush things out in the way I had been hoping until writing this sentence lol :p
Trying to get that Item page fast!
I don't really want to ship this as-is, because I'd really like to get stale-while-revalidate working before shipping a 1-week cache timer… will be tricky though!
The Tooltip elements seem to still be taking a bit, I don't really know a great workaround for that… could maybe split it out into a separate box and defer the rendering on it, so it doesn't block the first pageload?
Before the tooltips, I thought the focus state wasn't clear enough at a glance, so I added an extra focus outline to the species faces picker area. Now, I think it's clear enough with the species name tooltip popping up!
Now, when a species isn't compatible with an item, we gray out and sadden the pet, like on Classic DTI!
For now, I've hardcoded only the Zafara body ID to match. Let's do server connection next!
Didn't realize there was a convenient 150x150 face thumbnail we could use, so hey! Nice!
At one point I was considering generating our own thumbnails, but this is making me increasingly interested in just scraping the Rainbow Pool or something :p
Sentry issue IMPRESS-2020-20 doesn't have a clear backtrace, but it looks like the usual thing where we trigger an Apollo query directly, and forget to catch a potential error in the returned promise. I noticed that the last thing the user did was type in the search bar, and got a _caught_ error for the initial search!
Scanning the SearchPanel file, I think it's likely that this was a failure in `fetchMore` for the infinite pagination.
I'm a bit worried as to _why_ we were doing infinite scrolling stuff when there were no results? I wasn't able to repro a scroll event on the empty results list, but it's plausible that it could happen. I've added a gate to not send this request when there's an error!
I think what's happening in Sentry error IMPRESS-2020-1F is that mobile devices are running out of memory, so `canvas.getContext("2d")` returns null.
Now, we have a UI affordance to let you know when this is probably what's happening!
Also, when researching this, I learned about a Safari bug where you need to manually garbage-collect your own canvas data. It's possible that Safari users have been having particular trouble with memory leaks over long sessions? I'm not sure, but it seems like a good idea to add this small garbage-collection code!
Ok I think I've finally narrowed this bug down! We had one more loading case: the items page needs time to figure out which species/color to default the fields to, and passes null into the component while this loads. Now, we wait for that!
Previously, if you typed a pet name on the homepage, but its pose wasn't labeled in our database, you'd get a black empty screen. Now, we redirect to the UNKNOWN pose, or whatever exists for us to use!
I think it's confusing that the poses in the dropdown start with the emotion word, but are grouped by the gender presentation word! It's also different than the precedence order! I've reordered them.
We're occasionally getting errors on the homepage, of the new message I added: `Error loading valid poses for species=, color=108: byteOffset cannot be negative`.
So ok, now we know it's a species undefined bug, coming from `onChangeSpecies`! That suggests we're not finding by ID correctly?
So I'm adding some new logging to help me understand the sequence of actions leading up to this point, and the species data state when the error itself happens!
I've been getting more Sentry errors about JS chunk errors after deploys, and finally looked into it!
Turns out that, our try/catch handling was working great, and the page was reloading correctly for users as expected. But in these scenarios we would _also_ throw and log two uncaught errors!
The first is that, because we're a single-page app, unrecognized routes fall back to the index.html by default (to power our custom client-side routes, like /outfits/123 etc). So this meant that missing JS files, which _should_ be returning a 404, were instead returning 200 OK and an HTML file, which failed to parse. (And running the script isn't included in the catchable part of the `import` promise!)
Now, in our `vercel.json` config, we catch those paths specifically and 404 them. (The exact choice of path is important: on dev, all these routes run _before_ the dev server, which is responsible for serving the static files - but dev doesn't include hashes in the JS file names, so this 404 route only matches built prod JS files, not local dev JS files.)
The second is that we failed to return anything to `@loadable/component` in the error case, so it would try to render `undefined` as if it were a component class. Now, we return a trivial component class that returns null!
I'm getting some vague errors in Sentry about `canvas.getContext` returning null? Weird. (IMPRESS-2020-1F)
I'm not sure what that's about, so I don't want to stop sending it to Sentry. But I do want to make sure we handle this kind of error gracefully! (I'm thinking about how, while I don't think this one was, in the future this _could_ be caused by errors in Neopets movie clip JS, and I don't want our app to start messing up because of it!)
Here, we make sure to log the error to the console with more detail (the library URL), and show feedback to the user, and only log the error once per clip (so that animated ones don't like, send a bunch).
Huh, I'm not sure why Apollo is returning `data: undefined`, when the server is definitely returning the correct no-user-found data in the shape I expect... suspicious :/ well, let's at least stop crashing!
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!
