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

[UI/UX] Improve error handling on BE and FE #1363

Merged
merged 35 commits into from
May 27, 2022
Merged

[UI/UX] Improve error handling on BE and FE #1363

merged 35 commits into from
May 27, 2022

Conversation

flavioislima
Copy link
Member

@flavioislima flavioislima commented May 23, 2022

Also Trying to fix #1354 #1360 #1347 #1371

  • Introduces an Error component that we can use to treat front-end errors.
  • I am now checking if no library is available and showing it, but we should start using it on most components that could return an error.
  • Add on gamepage as well for when we get error from backend on info command
  • Added try/catch blocks in several places especially on JSON.parse() to handle corrupted JSON files that makes Heroic not load, even if there is a damn comma by the end of the file.
  • Added more error messages to ErrorHandler to deal with credentials expired for both GOG and Legendary. Let me know if I forgot some.
  • Added some defaults returns to electron-store calls that could make Heroic hangs on start.
  • Added new context menu to settings with options to copy Config (game and global) and open config file with the editor.
  • Added handler to retry a game installation if it fails because of Memory errors. Right now its hardcoded to 5000, can be improved in the future.
  • Changed how the Login Webview works by using events instead of setTimeout. On my tests it seems more reliable.
  • Changed Logout handlers to erase the epic username as well, since this was causing the login state to not always update correctly.
  • Fix [Linux] Steam Runtime setting not updating #1365
  • Implement Detect when a game is missing and suggest the user a fix #1374

image
image


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@flavioislima flavioislima changed the title wip: show error message if no games found Fix #1354 May 23, 2022
@flavioislima flavioislima changed the title Fix #1354 [UI/UX] Introduce Error message component May 23, 2022
@flavioislima flavioislima added the pr:ready-for-review Feature-complete, ready for the grind! :P label May 23, 2022
@flavioislima flavioislima changed the title [UI/UX] Introduce Error message component [UI/UX] Improve error handling on BE and FE May 24, 2022
@flavioislima flavioislima linked an issue May 25, 2022 that may be closed by this pull request
@flavioislima
Copy link
Member Author

I think I am done with this one, check it out when you have the time.

Copy link
Collaborator

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

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

In a lot of try-catch blocks, you're not logging the actual error at all. Might be helpful to have that, to more easily pinpoint what exactly went wrong

electron/launcher.ts Outdated Show resolved Hide resolved
electron/launcher.ts Outdated Show resolved Hide resolved
electron/legendary/games.ts Show resolved Hide resolved
electron/logger/logger.ts Outdated Show resolved Hide resolved
electron/logger/logger.ts Show resolved Hide resolved
electron/main.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

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

This is all it takes to check for MemoryError on install

electron/legendary/games.ts Outdated Show resolved Hide resolved
electron/launcher.ts Outdated Show resolved Hide resolved
electron/launcher.ts Outdated Show resolved Hide resolved
@CommandMC
Copy link
Collaborator

I'd say showing a dialog if a legendary command failed is fine in general, no need to remove that

@flavioislima
Copy link
Member Author

flavioislima commented May 25, 2022

I'd say showing a dialog if a legendary command failed is fine in general, no need to remove that

Yes, I'm not removing it. But leaving it there is causing the error dialog to appear twice because we already use the log error in other places now.

I will test more tomorrow. And also implementing the error handler for the issue that Ariel opened.

Copy link
Collaborator

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

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

Alright, now we just gotta find out why the test is failing

@flavioislima flavioislima linked an issue May 26, 2022 that may be closed by this pull request
@flavioislima
Copy link
Member Author

Alright, now we just gotta find out why the test is failing

Some things in NodeJS makes me crazy.
The test was failing because I was importing mainWindow from main into the utils file. But the error has nothing to do with that, I needed to check the commit that broke the tests and then start changing things until I realized was the import.
🤦🏽

@CommandMC
Copy link
Collaborator

One thing that makes finding errors like that easier is keeping your commits very small. It also keeps everything organized and the number of commits doesn't matter since they're all gonna get squashed at the end anyways

@flavioislima
Copy link
Member Author

@CommandMC runtime was missing for me since #1359 was merged, so I sent a new commit to this PR to use the steamLibraries to find the runtime now. I think should be fine but maybe would be good to test on a VM with Ubuntu or Pop if you have the time.

@CommandMC
Copy link
Collaborator

Works fine on my PopOS VM

@flavioislima flavioislima merged commit ef951a8 into main May 27, 2022
@flavioislima flavioislima deleted the fix/1354 branch May 27, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
2 participants