Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Implement the importing of optics #167

Merged
merged 20 commits into from
Feb 28, 2024

Conversation

lamemakes
Copy link
Contributor

@lamemakes lamemakes commented Feb 23, 2024

Closes #148. This is a rough first pass at handling the importing of blocked/liked/disliked sites via .optic files from previous exports or other means.

Changes TLDR:

  • Allow for the "uploading" of .optic files (all optic file parsing is client side)
  • Fixed the way the server exports optic files to ensure the expected formatting is received (axum was trying to export JSON, causing escaped quotes and other weird stuff)
  • Replaced the Ranked type with an enum to (hopefully?) simplify some readability & programmatic things now/down the line. If this is undesired it can totally be flipped back.
  • Once extracted & imported, sites are appended to the existing store as suggested to avoid weird overwrites
  • Fixed a typo. SiteRakings -> SiteRankings

Screenshots:
Screenshot from 2024-02-22 23-53-32
disclaimer: no beef or love for any of these websites, first ones to come up searches

Needs more cleaning up, commenting and to be a bit more robust but wanted to snag some early feedback before committing to it more. Are y'all doing JSDoc? I just did inline comments for now but planned to go back with docstrings.

Cheers!

@lamemakes
Copy link
Contributor Author

That failure is odd, I can't seem to recreate it locally.

Copy link
Member

@mikkeldenker mikkeldenker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is a great first pass! I fully agree the Ranked enum is more readable, and nice catch with the axum return type.

I think there is a weird non-deterministic bug in the test that failed in the CI. I re-ran it and everything seems to be fine. I'll look into it a bit more thoroughly next week and see if I can track it down.

frontend/src/lib/rankings.ts Outdated Show resolved Hide resolved
frontend/src/routes/settings/sites/+page.svelte Outdated Show resolved Hide resolved
@lamemakes
Copy link
Contributor Author

Alright, new updates and I think I covered most of my bases. wasm was a great call, made life so much easier in terms of optic parsing.

New TLDR:

  • Added a crate to be used for general client-side WASM bindings. I figured this would add less complexity if any other client side features were added, as a new struct can just be created in that crate. Let me know if you'd rather them broken out. This crate houses the Optics struct that contains the associated function parse_preference_optic.
  • Additions to vite + svelte configs to allow for packaging & execution of WASM code
  • Extraction of button tailwind logic to themes.ts to allow for reuse by labels and other elements
  • Updated CONTRIBUTING.md to reflect the need for wasm-pack

Let me know what you think! Only issue I saw was there's an issue when it comes to importing the blocked sites. I think that maybe stems from here, as it comes back as an empty array. I didn't really have the historical context to know what $discard meant in that comment.

@lamemakes
Copy link
Contributor Author

also kept the Cargo.lock as I considered the generated WASM a binary, can toss it if you'd prefer though.

@lamemakes
Copy link
Contributor Author

gahhh this killed me tonight. Running the CI scripts locally all is passing. Throwing in the towel for now - the only issue that's left is that the CSP typing doesn't allow ['*']. I can't typecast either because it's a JS file which is so weird to me. All should pass outside of that.

The wasm-pack has this awful build log though (that's pretty well complained about) that I can't seem to suppress either.

./scripts/ci/check seems to fail if it tries to install wasm-pack while it is already installed (at least on my machine). as it is already added as a step in CONTRIBUTING.md we can assume it has been installed on the system
@mikkeldenker mikkeldenker marked this pull request as ready for review February 28, 2024 15:45
Copy link
Member

@mikkeldenker mikkeldenker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the work on this! I had a few minor changes, especially related to migrating a vite plugin from #109 which improves the development experience by automatically updating the package when changes are made to it, and makes sure this gets reloaded in the frontend. They were a bit difficult to explain so I hope it's okay that I just went ahead and implemented them.

I totally agree that the best structure is to have a single wasm crate that we can extend with further functionality when needed, so we don't have to duplicate all the glue code.

LGTM 🚀

@mikkeldenker mikkeldenker merged commit 25c0344 into StractOrg:main Feb 28, 2024
2 checks passed
@lamemakes
Copy link
Contributor Author

Thank you so much for the assist! I completely missed #109! That would've been a fantastic resource if I had found it. did you see my comment regarding the blocked sites? Not sure if you had a different experience:

Only issue I saw was there's an issue when it comes to importing the blocked sites. I think that maybe stems from here, as it comes back as an empty array. I didn't really have the historical context to know what $discard meant in that comment.

I can write-up an issue if needed. Cheers!

@mikkeldenker
Copy link
Member

Oh, I totally forgot about that! You are completely right.
The comment in the code is way out of date syntax wise, but this is indeed what's causing this issue. The problem essentially is that blocked sites are stored in the optic as Rule { Matches { Site("|...|") }, Action(Discard) } rules and these simply get parsed directly into the rules vec so I thought there was no need to add them to the HostRankings as the rules would already block the sites.

I think the best solution is to add a special check for these rules when RawOptic is converted to Optic and simply add these sites to the blocked vec instead. They would still generate exactly the same tantivy queries as before and still correctly block the sites, but this would allow us to extract the sites during import. It would probably also make it easier to merge optics, so I think it's just a better approach all-around.

No need to open an issue, I'll fix it right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import liked/disliked/blocked sites from an optic
2 participants