Previously, the query wouldn't fill into the search box or page title
if e.g. parsing had failed. Now it does!
I'm not sure why the rescue strategy we previously had here doesn't
work anymore (I'm sure it must've in the past sometime?), but this is
simpler anyway, let's go!
I think this is a bit clearer and lets us clean up some of the syntax a
bit (don't need to always say `filters <<`), and also it will let us
use `return`, which I'm interested in for my next change!
Right, fitting isn't just body_id = this one, it's also body_id=0!
Anyway, doing this query on its own is still deathly slow, I wonder if
the idea I had about left joins (back when I was still working in a
Rails version that didn't support it lol) could help! Might poke at
that a smidge.
Oh right, `imageUrl` is the name of the field relative to what the app
expects, but under the hood `useOutfitAppearance` actually makes that
an alias for `imageUrlV2(idealSize: SIZE_600)`.
So we need to cache it as the same field with the same params, rather
than as just plain `imageUrl`!
This fixes the bug where wearing an item from search would require a
network round-trip and visually remove all items in the meantime.
(Also, none of this issue was visible to most users, because item
search is still feature-flagged onto the old GQL one for most people!)
This makes clicking on search results in the new mode actually work! It
correctly adds it to the outfit, and removes other items.
The thing that's behaving strangely is that, when you add the item, we
visually remove all items until we can finish a fresh network request
for what they should all look like. This probably means that the cache
lookup for `useOutfitAppearance` is not as satisfied with what we cache
here as `findItemConflicts` is? Something to investigate!
It'd be nice to customize the message a bit, but this should be rare
and I'd prefer the simplicity of just going with the default text.
I ran into this when I made a mistake in how I process the return value
of search results, so React Query caught and raised the error via
React, as intended! And I was annoyed that it wasn't logged anywhere,
so that's my motivation for this change—but also, the old message is
pretty meh and has some layout problems anyway.
The database did a weird thing today, where it wouldn't even respond to
the usual stop signal, I had to fully `kill -9` it??
I didn't see anything in the logs indicating what it was busy doing,
and people online seem to describe having this problem sometimes but
with no obvious solution.
For now, I'll try turning on the slow query logger, to see if that
might give us hints about whether there was like a denial-of-service
query attack hitting us or something?
I feel like this was part of `will_paginate` back before the Rails
community had itself figured out about what belongs in a model?
But yeah, a default per-page value for search results does not belong
here. And I don't think anything references it anymore, because we pass
`per_page` to the `paginate` call in `ItemsController` explicitly! So,
goodbye!
First off, I think our code has converged on a convention of gracefully
returning `nil` for manifest-less situations, so we can do that instead
of raise! And then that lets us just simplify this check to whether
`manifest` is present, instead of `manifest_url`, so we stop crashing
in cases where we get to this point in the code and there's a manifest
URL but not a manifest.
This was a bit tricky! When I initially turned it on, running
`rails swf_assets:manifests:load` would trigger database errors of "oh
no we can't get a connection from the pool!", because too many records
were trying to concurrently save at once.
So now, we give ourselves the ability to say `save_changes: false`, and
then save them all in one batch after! That way, we're still saving by
default in the edge cases where we're downloading and saving a manifest
on the fly, but batching them in cases where we're likely to be dealing
with a lot of them!
Now we're *really* duplicating with Impress 2020's system lol, but I
need a way to not keep trying to load manifests that are actually 404,
which are surprisingly plentiful!
This doesn't actually stop us from loading anything yet, it just tracks
the timestamps and the HTTP status! But next I'll add logic to skip
when it was 4xx recently.
This is both unnecessary now, but also caused a bug in the new search
stuff where searching by zone would pass an extra `locale` argument to
a filter that doesn't need it!
Idk when this regressed exactly, but probably people didn't super
notice because I don't think it's a very common thing to type directly
into the Infinite Closet search box! (It used to be crucial to the old
wardrobe app.)
But I'm using it in the wardrobe app again now, so, fixed!
For now, I'm doing it with a secret feature flag, since I want to be
committing but it isn't all quite working yet!
Search works right, and the appearance data is getting returned, but I
don't have the Apollo Cache integrations yet, which we rely on more
than I remembered!
Also, alt styles will crash it for now!
`is:np` now means "is not NC and is not PB".
Note that it might be good to make NC and PB explicitly mutually
exclusive too? It would complicate queries though, and not matter in
most cases… the Burlap Usul Bow is the only item that we currently
return for `is:pb is:nc`, which is probably because of a rarity issue?
Adding new functionality to the item search JSON endpoint, and adding
an adapter layer to match the GQL format!
Hopefully this will be pretty drop-in-able, we'll see!
Clearing the way to be able to delete the announcement banner, which is
currently the only link!
I feel like there's room to redo the site layout to find a place to
more properly link to this from, but I don't have one yet! And this is
enough of a niche reference that I think this is good enough?
A bit of a hack, because the thing triggering it was also a bit of a
hack? I feel like there's something we gotta do with refactoring how
our multiple concepts of state are managed… but in any case! This seems
to keep basic outfit-loading working, while no longer getting us
trapped in autosave loops!
Here's how I reproduced the bug:
1. Open a saved outfit.
2. Set the browser devtools to apply a latency of 5sec to all requests.
3. Add an item to the outfit, and wait for the autosave to start.
4. While it still says "Saving", remove the item again.
5. Watch how, when the first autosave request comes in, the item is
re-applied to the outfit, then autosave gets stuck looping forever.
The issue was that, when an outfit finishes saving, the change in
outfit data was triggering this effect in `useOutfitState` that was
*meant* to *initialize* local state from the saved outfit, not to keep
them in sync all the time. (In general, when saved outfit data comes
back from the server, we don't want to use it to "fix" local outfit
state in the case of discrepancies, because the most common source of
discrepancy will be the user having made further changes!)
But anyway, one thing I didn't realize is that we *were* depending on
this hacky hook to do more than I thought: it was responsible for
syncing `id` and `appearanceId` to the local state after saving the
outfit. So, I replaced the `rename` action dispatch here with a new
action that explicitly sets all fields the server is responsible for!
Ah yeah, if you're not on the wardrobe page (so we don't need the Alt
Styles UI), and the outfit's `altStyleId` is null (as is the case for
the item preview page), then there's no need to load the alt styles for
that species.
So before this change, going to `/items/123` would include an XHR
request to `/species/<id>/alt-styles.json`, which would not be used for
anything. After this change, that request is no longer sent. Hooray!
The alt styles controller is the one place we use this right now, but
I'm planning to generalize this to loading appearances during item
search, too!
I also add more `only` fields to the alt styles `as_json` call, because
idk it feels like good practice to both 1) say what we need in this
endpoint, rather than rely on default behavior upstream, and 2) to
avoid leaking fields we didn't realize were on there. (And also to
preserve bandwidth, too!)
I think there's no call sites for these anymore, so now I can start
repurposing these methods for the new API endpoints I'm planning! :3
Now, `SwfAsset#image_url` approximately matches Impress 2020 logic: use
the thumbnail PNG from the manifest if one exists, or the Impress 2020
converter for canvas movies, or the old AWS copy generated by gnash if
necessary, or return nil.
I built this API endpoint in anticipation of a change I never actually
made! I'll just remove it for now, leaning toward cleanuppery over
holding onto something I'm not sure about.
I think this used to be used in an API endpoint we've now deleted? I'm
just cleaning up call sites because I intend to refactor the `urls`
method and stuff, so I'm removing cruft that would complicate it!
I'm not certain-certain this is unused, but I did a global search for
`\bimages\b` in the codebase, and didn't find anything that looked like
a match to me!
Doing that sweet, sweet backfill!! It's not exactly *fast*, since
there's about 570k records to work through, but it's pretty good all
things considered! Thanks, surprisingly-reusable async code!
I'm gonna also use this for a task to try to warm up *all* the
manifests in the database! But to start, just a simple one, to prepare
the alt styles page quickly on first run. (This doesn't really matter
in production now that I've already visited the page once, but it helps
when resetting things in dev, and I think more it's about establishing
the pattern!)
The Neopets Media Archive is a service that mirrors `images.neopets.com`
over time! Right now we're starting by just loading manifests, and
using them to replace the hacks we used for determining the Alt Style
PNG and SVG URLs; but with time, I want to load *all* customization
media files, to have our own secondary file source that isn't dependent
on Neopets to always be up.
Impress 2020 already caches manifest files, but this strategy is
different in two ways:
1. We're using the filesystem rather than a database column. (That is,
manifest data is kinda duplicated in the system right now!) This is
because I intend to go in a more file-y way long-term anyway, to
load more than just the manifests.
2. Impress 2020 guesses at the manifest URLs by pattern, and reloads
them on a regular basis. Instead, we use the modeling system: when
TNT changes the URL of a manifest by appending a new `?v=` query
string to it, this system will consider it a new URL, and will load
the new copy accordingly.
Fun fact, I actually have been prototyping some of this stuff in a side
project I'd named `impress-media-server`! It's a little Sinatra app
that indeed *does* save all the files needed for customization, and can
generate lightweight lil preview iframes and images pretty easily. I
had initially been planning this as a separate service, but after
thinking over the arch a bit, I think it'll go smoother to just give
the main app all the same access and awareness—and I wrote it all in
Ruby and plain HTML/JS/CSS, so it should be pretty easy to port over
bit-by-bit!
Anyway, only Alt Styles use this for now, but my motivation is to be
able to use more-correct asset URL logic to be able to finally swap
over wardrobe-2020's item search to impress.openneo.net's item search
API endpoint—which will get "Items You Own" searches working again, and
whittle down one of the last big things Impress 2020 can do that the
main app can't. Let's see how it goes!
Preparing to finally move wardrobe-2020's item search to use the main
app's API endpoints instead!
One blocker I forgot about here: Impress 2020 has actual support for
knowing an item's true appearance, like by reading the manifest and
stuff, that we haven't really ported over. I feel like maybe I should
pause and work on the changes to manifest-archiving that I'd been
planning anyway? I'll think about it.
I changed the type of this tag without realizing the JS references it
by both class and `div`!
I think at the time this was a perf suggestion for jQuery, because the
best way to query by class name was to query by tag first then filter?
It's possible our jQuery still does this, but I don't imagine it's very
relevant today, so I'll just remove that for better guarding against
similar bugs in the future instead.
I've moved the support secret into the encrypted credentials file, and
moved the origin into a top-level custom config value in the
environment files, with different defaults per environment but still
the ability to override it. (I don't use this, but it feels polite to
not actually *demand* that people use port 4000, y'know?)
There's a bit happening behind the scenes of this change. Previously,
we kept a `SECRET_TOKEN` environment variable in `production.env`, and
used a `secret_token.rb` initializer to wire it up as the
`secret_key_base`.
In this change, we move to Rails's new-ish (two years old :p) encrypted
credentials system. Now, we set a `RAILS_MASTER_KEY` environment
variable in the deployed `production.env` instead (and in our local
`.env.production` in the project root for managing it), and we can run
`rails credentials:edit` to open the encrypted file in a text editor.
Inside, the content is just:
```yml
secret_key_base: "<OUR_SECRET_KEY>"
```
This indirection doesn't exactly do much for us functionally; it's just
the more standard way of achieving what our `secret_token.rb` situation
was achieving.
We could also migrate other secrets into there, and I just might! That
would simplify duplication between `/deploy/files/production.env` and
`/.env.production`, at any rate! The main notable one is
`MATCHU_EMAIL_PASSWORD` for sending auth emails from
`matchu@openneo.net` (and there's also a Stripe token that we don't
actually use in the app these days, those codepaths are old bones). Oh
and there's also the `IMPRESS_2020_SUPPORT_SECRET`!
Anyway, the motivation for this was to remove the warning when starting
the app that Devise is trying to use the deprecated
`Rails.application.secrets` method. I was expecting to have to do
[the workaround shared here](https://github.com/heartcombo/devise/issues/5644#issuecomment-1804626431),
but it turns out whatever default behavior Devise does under the hood
is happy enough with our new decision to use the credentials file, and
the deprecation warning is gone! Ok neat!
I hadn't realized for a while that we weren't already doing this lol, I
had noticed that `bundle install` in production was slower than I
expected when adding new stuff, but it was when we did this big recent
`bundle update` that I really noticed the difference.
Fixed now, I think! Though the real test will come when we actually
have a new gem to install, since this was a no-op case.
As the comment in `deploy.yml` explains, this was a multi-step process,
but it went very smoothly as planned, hooray!!
I noticed again while making this change that Bundler doesn't seem to
be availing itself of the checked-in dependencies in `vendor/cache`. I
think I know the fix for this, I'll toss it into an upcoming change and
see if it works!
