-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Show local network URL used in Home Assistant URL settings #22379
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a button to copy it to your clipboard? |
103fda9
to
e778c9e
Compare
Made the url inputs always visible with masking when not editing and copy buttons |
May I suggest the following changes:
|
I don't think "Use network settings" is more descriptive. "Automatic" tells me I don't need to worry about it. The port number is included in the link if configured but it is obfuscated with the rest of the url right now. I wondered whether to leave it visible but it's the same level of info as the local address and we obfuscate that |
Lets keep the label Automatic and add a description what this is. So it's easy and gives an insight in the magic box.
If we copy it in the link I would also show the input. We can show it as below for editable and read-only inputs. |
a46c5fc
to
2cb10d2
Compare
There is no need to censor the internal IP, as it cannot be reached externally. |
We discussed this, as there actually is a need for it. As in, people with loopback issues van use different URLs, and split DNS peeps will probably use the same as the external URL. From that perspective, it is hard to make external masked, and internal not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks great 🎉
One main point is missing, disallow empty values.
For both internal and external you come into a strange state when you leave the text input empty and hit save. Please don't allow to hit save when one of them is empty and show a validation info.
this._unmaskedExternalUrl = !this._unmaskedExternalUrl; | ||
} | ||
|
||
private _obfuscateUrl(url: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this different then the remote url at /config/cloud/account?
Would be cool when it is synced, maybe create a util for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There only .ui.nabu.casa urls are handled. Here it could be anything so the logic is different
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Wendelin <12148533+wendevlin@users.noreply.github.com>
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Co-authored-by: Wendelin <12148533+wendevlin@users.noreply.github.com>
Co-authored-by: Wendelin <12148533+wendevlin@users.noreply.github.com>
Empty is a valid value. You need to be able to remove the custom urls |
Proposed change
Show the resolved local URL for easier debugging
Depends on Core PR home-assistant/core#128432
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: