fix: graphql render issue caused by illegal Object.includes()

[marckong]

changelog(Fixes): Fixed issue #6857 related to GraphQL render issue caused by illegal Object.includes() method call. Also fixed an issue where GraphQL variables were not properly sent as an object when sending requests (both from Debug and Test view).

It seems we've tried to fix one issue and caused another issue. I took a very conservative approach here to fix the issue on top of the previous fix.

Root Cause

Basically variables are passed as Object. Object does not contain 'includes()' method, and, therefore, it throws an error.

Solution

The fix here is in jsonPrettify method, we should give a default value as an empty string and stringify the object if passed instead of string in trycatch block. If it fails, we should return the default value, so it does not break the app. Ideally, I would prefer to show a toast when it happens, so the user can be aware. Currently it silently fails but the app still runs.

TODO

• solved issue with graphql variables field being sent as a string when it should be sent as an objectg
• added reproduction smoke tests to check cases where we send Integer variables, Object variables, ... in Graphql requests

The codebase has very low test coverage, and in general, we should try increasing the coverage (despite little time we have)

Huge thanks to camiloclc who didn't mind jumping on the call and helped us reproduce it.

Closes #6857 and all the other duplicate issues

[suuunly]

When can we expect this to have been reviewed? :)
Currently, we are unable to work, as we rely heavily on GraphQL :)

[lukepearce]

Your app is broken because it was able to release with basic JS errors?

[mealbarracin10]

Hi all, When can we expect this to have been merged? I’m block 😕

[CosminPerRam]

Your app is broken because it was able to release with basic JS errors?

Irrelevant comment, this could and does happen from time to time in any project, what matters is fixing the problem as fast as possible and analyze how this got through and plan steps/procedures that avoids this from happening again.
Instead of asking aggressive questions we could ask how come this got through and if any plans to avoid this are being planned.

[marckong]

Thank you for your patience and support. We are actively working on testing the side effect of this. Going forward, we will add more Playwright smoke tests as much as we can. Especially, to prevent regression bugs.

@CosminPerRam well said. Thank you for your kindness.

To answer some of points you mentioned, we are working in codebase that lacks unit tests or smoke tests. We are also running thin. Many of us, including myself, were deployed to tackle engineering problems in the backend API side (the API that lives in the cloud, not in the electron codebase). Now some of us are brought back to the desktop app side, so we will do our best to improve as a team. When I hopped on a call with @camiloclc yesterday, I promised that we would put a PR for a fix in few hours and we did. We solved this problem as quickly as we could, and we are doing our best to unblock our users today.

Things will improve. Thanks for the patience.

Also this is not just "Basic JS errors" as it looks. It has multi-layers of things that contributors need to be aware of. This kind of comment is not very productive.

Thanks for collaboration on this - @camiloclc, @ihexxa, @gatzjames, @filfreire

[suuunly]

@marckong thank you for the swift update :)
Most of us know the struggle of surprise bugs. 😆