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

feat: Windows support #42

Merged
merged 8 commits into from
Jan 1, 2024
Merged

Conversation

rllyy97
Copy link
Contributor

@rllyy97 rllyy97 commented Dec 31, 2023

Main Changes

Added Windows build support!
Builds with the NSIS windows target.

Adjustments for Windows:

  • Filepaths
    • Some replace / join fixes to split by '/' and '', and join using the proper character
  • Title bar buttons
    • Had to make some changes with the right aligned windows control buttons
  • No transparency (only for Windows)
    • Electron doesn't natively have the blur that Macs have, so it's very hard to see certain content if it's transparent
    • There are packages out there to get the blur but it looks like it has decent performance implications
  • Dependency patches
    • The unmaintained pdf-parse package that is used in the llamaindex dependency has a bug that crashes the app when built
    • It does not look like this will be fixed so I've patched the package locally as a workaround

I'm not exactly sure how you go about publishing the packaged files, but just to test I was running package:win and then running the resulting setup .exe file and it all seemed to work great

Screenshots

image
image

((( If you want any more help with this project just lmk )))

@rllyy97 rllyy97 marked this pull request as ready for review December 31, 2023 23:36
@rllyy97 rllyy97 mentioned this pull request Dec 31, 2023
@parisosuch-dev
Copy link

This is great man, thank you for doing this. I've been busy and was hoping someone else took an interest in making this happen :)

@UdaraJay
Copy link
Owner

UdaraJay commented Jan 1, 2024

This looks great! Thank you for taking it on :)

I'll check this out soon and test it, if it's all good I'll publish it via the releases on this repo.

@GTruss
Copy link

GTruss commented Jan 1, 2024

@rllyy97 Thank you so much for making a PR with Windows support! I'm testing the changes now but I get the following error when I add a reply to an existing post:

usePost.js:106 Error: The "path" argument must be of type string. Received an instance of Array
at addReplyToParent (usePost.js:113:47)
at usePost.js:95:17
at async index.jsx:182:7

This happens when I "Add another entry" or "Reflect".

Could this be related to the path changes in the PR? Let me know if you need more info/repro steps. Thanks again!

@rllyy97
Copy link
Contributor Author

rllyy97 commented Jan 1, 2024

@UdaraJay I've cleaned up the patch file and fixed the issue brought up by @GTruss, should be good now.
I've also modified some of the error catching around not having an API key. It doesn't change the experience when built at all, but the project would consistently throw errors during dev before since I didn't have a key set up. This behavior isn't anything specific to my changes, I noticed it first when I cloned + ran the project locally.
(I'd honestly recommend creating an application state for when the user isn't set up with the ai features so the app doesn't attempt to do them on each action)

@UdaraJay
Copy link
Owner

UdaraJay commented Jan 1, 2024

Thanks @rllyy97! the patch looks good and everything works– it'll be published after some more testing on a windows machine!

The AI setup state is available from AIContext, but def a good idea to explicitly disable the reflect and embedding generation when there is no key. Your change is good for now– I'll hope to consolidate the AI APIs into one state in the future.

@UdaraJay UdaraJay merged commit 0bc44d7 into UdaraJay:main Jan 1, 2024
2 of 5 checks passed
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.

4 participants