[Feature] The proxy configuration values are now hidden by default

[MrSnix]

Feature request
The information in the proxy settings field shouldn't be visible due to security concerns.

Feature implementation
The proxy input field is now hidden by default

Showcase

Closes #3138

[develohpanda]

Thanks for working through this! It looks pretty good and appears to work as well. 🙌🏽

I have a couple of bits of feedback for the direction we are trying to take the General preferences component in, which I will explain below.

Firstly, we are looking to move from the renderBooleanSetting() class method approach to a standalone FC approach - a BooleanSetting. We want to do this for the other ones too (renderNumberSetting and renderTextSetting) but haven't been able to yet.

Since this PR introduces a new one (renderPasswordSetting), I suggest we refactor that to a MaskedSetting (renamed to be more generic) similar to the BooleanSetting. This way the new component also manages it's own state, allowing us to remove the fieldsHidden state property!

Secondly, at the top of the general preferences there is an option to Reveal Passwords, which controls the password visibility everywhere in the app. I recorded a demo of that to show how it works the the password field in the Basic Auth form.

Ideally, this MaskedSetting component also adheres to that setting (which is fairly straightforward to achieve). The Basic Auth password field (amongst other places) uses the PasswordEditor component. This, under the hood, uses code-mirror so our templating engine works, but that is not necessary for preferences, which should just use a regular input as per your current implementation.

tl;dr:

1. Take inspiration from BooleanSetting to create a MaskedSetting component
2. Take inspiration from PasswordEditor to connect the MaskedSetting component to the Reveal Passwords setting.

How does that sound? 😄

[MrSnix]

EDIT: This was fixed

Hi dear @develohpanda, I applied all the requested changes 😄 but ... something went wrong 😞 (?) when I refactored the input field as a React component and I can't figure out the reason why 🧐 . After the component was created, it started suffering from input lag. I tried several options but none of them worked (e.g React.PureComponent, React.useMemo).
Seems like updating the setting value takes time and this causes the lag (?) (but it was great before I refactored it to a component). The input lag is strong when I keep pressing the backspace button 🔙 to delete the current value and gets worse and worse with time. (it gets progressively stuck more and more). I wonder if the BooleanSetting suffers from the same issue but we can't see it

I can debounce the input ... but is it the right solution?

Here is an attached .gif of the behaviour:

[MrSnix]

My previous commit had some mistakes, I fixed the input lag.
I apologise.

I simply need some feedback about the current implementation

Thanks

[develohpanda]

Making really good progress 🙌🏽 A couple more notes please!

[develohpanda]

@MrSnix I pushed a couple of commits, have a look and let me know if you have any questions 😄 if not, I'll do a final review

[MrSnix]

@develohpanda LGTM!

Feel free to go :D

Thanks as always for the time spent on this

[develohpanda]

Excellent, thank you! This is a great improvement 👏🏽

[develohpanda]

Looks and works great! This will probably get some more work put into it soon as we migrate the remaining settings into standalone components. Thanks for working through it!

https://www.loom.com/share/5c07f57a39e9452e91c3c8f8b0d7b119

[dimitropoulos]

I think we should be very careful about using props (unqualified) as a prop name. That creates a situation where we have Component.props.props.someValue. Not saying this is the same thing since we're destructuring, but it's unusual to do this, all the same.

What I suggest is we do something like "inputProps".

Another approach would be to do something like MUI does, componentProps.input could contain the overrides.

---

However, from another perspective, why is disabled and placeholder being wrapped in the first place? They seem like normal props that are very much "on topic" for the MaskedSetting component. From that perspective, we should just remove the props wrapper altogether.

That would be my vote, which, thankfully, sidesteps the entire issue of what to name the wrapper prop.

We're planning to loop back on these settings components soon, so we can make this adjustment then, it doesn't need to block this PR at this moment.

[dimitropoulos]

One non-blocking thing I noticed: it's good practice when making a password input like this to not leak the value of the input to the DOM if the password is hidden. For example, the password fields for Okta, 1Password, GitHub, Amazon, and many more all work like this. This is also what we do for PasswordViewer, if you wanna take a look, although it's a little easier there because it's not an input.

[dimitropoulos]

as always, @MrSnix, thank you for the contribution!

[MrSnix]

Hi @dimitropoulos, I am glad to see your feedback on this one.

The password fields for Okta, 1Password, GitHub, Amazon, and many more all work like this.
This is also what we do for PasswordViewer if you wanna take a look.

I just took a look and it was enlightening, I wasn't aware of this or better I thought about something like this in the past but I didn't know it was a thing. Thanks a lot, I just learnt something new :D

As always, @MrSnix, thank you for the contribution!

It is a pleasure to see that contributing has a positive impact on the community.
Even small changes, if I can address them, I try to.

[arafattechstyle]

Hi, do we know if this is going to be part of which release?

[develohpanda]

Hi @arafattechstyle, this will be part of a release coming this week!