`ppm` Themes searching uses different methodology than implemented

asiloisad commented 9 months ago

Thanks in advance for your bug report!

[X] Have you reproduced issue in safe mode?
[X] Have you used the debugging guide to try to resolve the issue?
[X] Have you checked our FAQs to make sure your question isn't answered there?
[X] Have you checked to make sure your issue does not already exist?
[X] Have you checked you are on the latest release of Pulsar?

What happened?

The Packages & Themes buttons of Settings/Install doesn't have effect.

Pulsar version

1.109.0

Which OS does this happen on?

🪟 Windows

OS details

10 22H2

Which CPU architecture are you running this on?

64-bit(x86_64)

What steps are needed to reproduce this?

Go to Settings -> Install -> type any text e.g. Hydrogen
Check switch to Packages
Check switch to Themes
Observe the results are the same, so the switch doesn't have effect

Additional Information:

No response

Spiker985 commented 9 months ago

Interestingly, if you delete your inquiry and then toggle the type - it doesn't clear the search

But even more interesting. If you close settings, reopen it, change it to theme (which works correctly initially) and then add an inquiry - it fails again

So it seems that it has to somehow be a function of our backend?

@confused-Techie Want me to be a guinea pig for logging purposes?

Spiker985 commented 9 months ago

I also found an unused filterPackages function inside install-panel.js which doesn't appear to be in use (or at least has zero code references inside that file outside of its definition)

confused-Techie commented 9 months ago

Thanks for pinging me @Spiker985.

I've taken a look at this, and while I can't account for some of this behavior (although I can account for most of it, at least the reported issue portion), it does seem there's a disparity from what PPM is doing, and what the backend is doing.

Which unfortunately comes again from me referencing bad Atom docs to create the backend, that don't match what was actually happening.

Since when we reference the original Atom docs about the backend for searching we get:

Which made me assume that the filtering of packages or themes was happening on the request string, rather than any query parameters.

Meaning to search packages on the backend, currently, you'd use:

https://api.pulsar-edit.dev/api/packages/search?q=<SEARCH_TERM>

And to search themes on the backend, currently, you'd use:

https://api.pulsar-edit.dev/api/themes/search?q=<SEARCH_TERM>

Which this method does work properly.

But, it seems that PPM actually uses some undocumented query parameter of filter, which we can see here that is actually what filters the search, meaning that PPM is sending a request such as:

https://api.pulsar-edit.dev/api/packages/search?q=<SEARCH_TERM>&filter=themes

Which this search still just preforms the same search on all packages, which is why the results don't change.

So I can't speak to why this would behave differently the first time around, but can confirm that themes searching does work on the backend, but only in the way I assumed it did. But turns out it's actually implemented in some totally new, undocumented way via PPM.

So to fix this we could either add support for filter query parameter within the backend, or change PPM to utilize the search via the path itself.

Neither option is better, except that changing it in the backend would mean this immediately starts working for every version of Pulsar, although I'd still like to make the change in PPM and eventually deprecate the usage of the query parameter here. Since it's a little known fact, but every single endpoint of PPR can actually support using either themes or packages in the request path, and this does (When relevant) filter results as one would expect. So I'd like to keep that behave if possible, while still fixing this for the most users.

An aside I will add for @asiloisad and others, that when you search for packages, you also get themes results back. So searching packages includes all packages, it's only when you search themes that only themes are returned. So if you really wanted to find only themes, including syntax or theme would likely be helpful here, if you want results today

Spiker985 commented 9 months ago

Clarification: What I misconstrued as "initially working" is actually just the featured themes, which are on a completely different endpoint

But this begs the question, was this a regression from the ppm decaff changes?

confused-Techie commented 9 months ago

@Spiker985 Ahh that makes sense, and reassuring then that we know exactly what's going on.

And just double checked, the logic is identical prior to decaf. So no regressions here, searching for themes has just never worked on our new backend the way PPM does it, simply because Atom's docs for the backend are terrible, and I had to make a lot up on how things worked, and here it seems I got it wrong.

I'll go ahead and transfer this issue over to package-backend and see what I can do about resolving it.

Spiker985 commented 9 months ago

Interesting

How in the world did we not know until now that the themes searching was busted in the editor? (Not blaming you, just for clarity)

We've had several people question things about custom themes and syntax highlighting before now 🤔

I wonder if people were finding the themes on the web and installing from there, installing them as github packages, or manually installing them 🤷‍♂️

But additionally, how do we prevent this regression in the future? Do we have a few static theme entries we explicitly test for? (this could balloon costs)

confused-Techie commented 9 months ago

@Spiker985 We probably never noticed because searching for packages still returns themes.

We only exclusively return themes on the themes endpoint, but the packages endpoint returns both. So you can still find themes by searching for theme, syntax or ui.

Otherwise it's a bit difficult to test. Since this is one of those cross-cutting concerns, where something can't really be decomposed into a test. Since to test something like this, we need to make sure that settings-view does it properly, and that ppm does it properly, and that package-backend does it properly. EDIT: Which is to say, that the definition of properly could change at any time in any one of these different systems, so it'd be rather difficult to define 'properly' without good docs.

I really think the best way is just having proper documentation. Since if Atom's Server Side docs had been accurate, we would have never had this issue. They simple didn't document many endpoints, and many query parameters.

Potentially we could have PPM test hitting actual endpoints (Which it does already do a few) and one of those test for finding a specific package, and ensuring we don't find another particular package, but that isn't always the easiest. And like you said would increase costs.

Also just to mention, the website doesn't actually even support any kind of filtering, it only searches the packages endpoint, which returns everything, so it's just as if you searched only packages in the settings UI.

I feel like the best step to fixing this would be to reduce the amount of 'hops' involved in this logic, such as moving PPM into core, or having settings-view make requests like this one itself, rather than shelling out to PPM. But not sure if that's totally worth it, as it would still have to shell out for many other tasks, at least until we fully move PPM into core.

Spiker985 commented 9 months ago

Yeah, while trying to debug this I was consistently confused by seeing zero network traffic (outside of user icons) inside the dev tools - I actually hadn't realized it was shelling out to ppm to begin without

Something I do worry about (not necessarily regarding this particular issue itself, but for Pulsar as a whole) is the security risks involved with the outdated-ness of the editor coupled with web requests

It's arguably something we're working towards rectifying overall, but still - makes me think sometimes. Specifically, it makes me wonder if they tried to keep as many "web" calls relegated to ppm, so as not to treat the editor (which is basically a mini web browser) as an actual web browser

pulsar-edit / package-backend