Skip to content

Comments

Feature/installer gui ux#878

Merged
bmw merged 4 commits intoEFForg:mainfrom
BeigeBox:feature/installer-gui-ux
Feb 12, 2026
Merged

Feature/installer gui ux#878
bmw merged 4 commits intoEFForg:mainfrom
BeigeBox:feature/installer-gui-ux

Conversation

@BeigeBox
Copy link
Contributor

Pull Request Checklist

  • The Rayhunter team has recently expressed interest in reviewing a PR for this.
    • If not, this PR may be closed due our limited resources and need to prioritize how we spend them.
  • Added or updated any documentation as needed to support the changes in this PR.
  • Code has been linted and run through cargo fmt.
  • If any new functionality has been added, unit tests were also added.
  • CONTRIBUTING.md has been read.

Small fixes to improve input validation, and not have the gui crash if there's an IPC issue.

@BeigeBox
Copy link
Contributor Author

Had to add one more fix, the autocorrect was capitalizing the commands causing them to fail entirely.

cooperq
cooperq previously approved these changes Feb 11, 2026
Copy link
Collaborator

@cooperq cooperq left a comment

Choose a reason for hiding this comment

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

I'm good with this but @bmw should give it a final approval .

app_handle
.emit("installer-output", output)
.expect("Error sending Rayhunter CLI installer output to GUI frontend");
let _ = app_handle.emit("installer-output", output);
Copy link
Member

Choose a reason for hiding this comment

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

oof. i assume you hit this? i had hoped this was the kind of thing that would never fail. did piping the CLI installer output to the GUI ever work for you?

i'm a little hesitant to just silently drop output like this. i agree crashing is bad if something like this can happen, but i worry that missing output will lead to a pretty confusing UX for some users

please let me know your experience here and if you have any other thoughts on a path forward here

other than this, i have no concerns about this PR. thanks again BeigeBox!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't run into it, I just have done some IPC stuff before and saw that it would cause the app to crash. We could extend it in the future to be more communicative. I just had a few moments last night and figured I would get the initial low hanging fruit sorted.

Copy link
Member

@bmw bmw Feb 11, 2026

Choose a reason for hiding this comment

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

gotcha. in that case, i'd personally prefer you leave this line as it is in main and we can revisit it when/if it becomes a problem

if you do that, i'll merge this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@bmw bmw left a comment

Choose a reason for hiding this comment

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

this lgtm!

@bmw bmw merged commit ec6967e into EFForg:main Feb 12, 2026
18 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.

3 participants