-
Notifications
You must be signed in to change notification settings - Fork 75
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
Update documentation about UI bug in configuration editor #463
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request updates the Tailscale add-on documentation for Home Assistant by adding three distinct cautionary notes. These notes advise users to avoid using the "Show unused optional configuration options" switch, require manual updates to the YAML configuration for optional parameters, and warn that the Home Assistant UI may misrepresent the state of certain options. No changes were made to any exported or public entities. Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tailscale/DOCS.md (3)
86-89
: Caution Block on UI Default Option Warning
This block clearly warns users not to use the "Show unused optional configuration options" switch, which is essential given the UI limitations. Please verify that no unintended blank lines or extra spaces exist within the block, as markdownlint flagged potential issues.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
90-95
: Simplify Wording for Optional Configuration Change
Consider rewording line 91 to improve clarity—replacing "change the default behaviour of any of these optional configuration options" with "change the default behaviour of these optional configuration options" makes the message more concise. Also, double-check that the blockquote does not include any extra blank lines that could trigger markdownlint's MD028 warning.🧰 Tools
🪛 LanguageTool
[style] ~91-~91: Consider simply using “of” instead.
Context: ...ou want to change the default behaviour of any of these optional > configuration options,...(OF_ANY_OF)
🪛 markdownlint-cli2 (0.17.2)
95-95: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
96-104
: Refine Warning Block for UI Misrepresentation
The warning block effectively informs users about potential misrepresentation of option states in the Home Assistant UI. For enhanced readability, consider breaking up long sentences or simplifying the language. Additionally, ensure the blockquote format is consistent (i.e., no extra blank lines within the quoted text) to address markdownlint suggestions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/DOCS.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[style] ~91-~91: Consider simply using “of” instead.
Context: ...ou want to change the default behaviour of any of these optional > configuration options,...
(OF_ANY_OF)
🪛 markdownlint-cli2 (0.17.2)
tailscale/DOCS.md
89-89: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
95-95: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
> [!CAUTION] | ||
> Due to limitations in Home Assistant's UI, **do not use** the "Show unused | ||
> optional configuration options" switch on the Configuration tab! |
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.
This is not something I want to document, I'm strongly against documenting bugs, I believe in fixing them.
It is a problem upstream.
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.
How else warn users who spend hours to figure out, that the UI does the exact opposite what it shows?
Thats why nearly all the yaml configs users share show they enable the already by-default-enabled options, and they notice only after hours that the UI never saves the seemingly disabled options, the options they originally wanted to disable when they opened the config tab.
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.
How else warn users who spend hours to figure out, that the UI does the exact opposite what it shows?
This is a concern for Home Assistant, not add-ons.
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 is an open RFC about add-on config options, this is opened 4 years ago, nothing happened. home-assistant/supervisor#2640
Meanwhile de facto issues/bugfixes are closed, that it is better to implement the RFC. home-assistant/supervisor#4606 home-assistant/supervisor#4607
Even your issue isn't fixed correctly. home-assistant/frontend#8614
If even you can't get something fixed by the frontend team, it will never be fixed.
17.000+ users of this add-on, fighting for hours to configure even the simplest thing, they deserve a warning about this issue.
When it get fixed, I will remove this warning from the docs.
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.
Complete newbie to Home Assistant here so take my opinion with a handful of salt—I was trying to figure out how to stop my HAOS instance from trying to advertise itself as an exit node, and was able to figure out how only thanks to this PR. Having this kind of warning front and center would've sped things up!
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.
Sorry, I'm not adding this.
Working around issues will never fix 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.
Complete newbie to Home Assistant here so take my opinion with a handful of salt—I was trying to figure out how to stop my HAOS instance from trying to advertise itself as an exit node, and was able to figure out how only thanks to this PR. Having this kind of warning front and center would've sped things up!
Please report the issue upstream. Learning to workaround the problem will help no-one.
../Frenck
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tailscale/DOCS.md (1)
86-104
: Improve documentation structure and formatting.To enhance readability and maintain consistency:
- Consider moving these warnings to the beginning of the Configuration section (before line 41)
- Use consistent formatting - either all
[!CAUTION]
or all[!WARNING]
- Consider adding a table of contents to help users navigate the configuration options
## Configuration + +> [!CAUTION] +> When configuring optional parameters: +> - Do not use the "Show unused optional configuration options" switch +> - Add options to the YAML configuration manually using "Edit in YAML" +> - Be aware that disabled options in the UI may not be saved to the configuration + This add-on has almost no additional configuration options for the add-on itself.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
95-95: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/DOCS.md
(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
tailscale/DOCS.md
89-89: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
95-95: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
🔇 Additional comments (1)
tailscale/DOCS.md (1)
86-104
: Consider alternative approaches to address UI confusion.While these warnings aim to help users avoid common configuration pitfalls, documenting upstream UI limitations may not be the preferred long-term solution, as indicated by previous feedback. Consider these alternatives:
- File an upstream issue to improve the Home Assistant UI's handling of optional configuration options
- Enhance the add-on's configuration validation to detect and warn about common misconfigurations
- Consolidate the three warnings into a single, more concise note focused on the correct way to configure options rather than UI limitations
Let's check if this issue has already been reported upstream:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for existing issues about UI configuration options gh issue list -R home-assistant/frontend --search "in:title optional configuration options show unused" --state allLength of output: 117
Let's further verify whether any upstream issue regarding the UI showing unused optional configuration options exists by broadening our search queries. Please run the following shell script:
Once we have the results of these queries, we can further assess whether there is an existing upstream report or if filing a new issue is warranted.
🏁 Script executed:
#!/bin/bash echo "Search for issues in home-assistant/frontend for 'Show unused optional configuration options'" gh issue list -R home-assistant/frontend --search "Show unused optional configuration options" --state all echo "Search for issues in home-assistant/frontend for 'optional configuration options show unused'" gh issue list -R home-assistant/frontend --search "optional configuration options show unused" --state all echo "Search for issues in home-assistant/frontend for 'unused optional configuration'" gh issue list -R home-assistant/frontend --search "unused optional configuration" --state allLength of output: 1614
Upstream issue not found – consolidating warnings remains advisable.
After broadening the search, no upstream issues have been identified regarding the Home Assistant UI misrepresenting optional configuration options. This confirms that the current documentation addressing these UI limitations is necessary. However, given the previous feedback and the potential for confusion, consider one or more of the following actions:
- Consolidate warnings: Merge the three cautionary notes into a single, concise note to improve clarity.
- File an upstream issue: Despite no matching issue currently, consider filing one to draw attention to the UI limitation.
- Enhance configuration validation: Implement additional validation in the add-on to preempt user misconfiguration.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
95-95: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
fe16e0c
to
401127d
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tailscale/DOCS.md (1)
91-95
: Consider adding a step-by-step guide for YAML configuration.While the caution provides good guidance about using YAML configuration, it could be more helpful with a concrete example.
Consider adding an example like this:
> [!CAUTION] > When you want to change the default behaviour of these optional configuration > options, **add them to the YAML add-on configuration manually**, by using the > "Edit in YAML" in the ... menu on the right and save them! Use the UI to edit > them only after you added them manually! +> +> Example: +> 1. Click the ... menu on the right +> 2. Select "Edit in YAML" +> 3. Add your configuration: +> ```yaml +> accept_dns: false +> advertise_routes: [] +> ``` +> 4. Click "Save"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/DOCS.md
(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
tailscale/DOCS.md
90-90: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
96-96: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
🔇 Additional comments (3)
tailscale/DOCS.md (3)
87-89
: Clear and concise warning about UI limitations.The caution effectively warns users about a specific UI element that could cause confusion. This aligns well with the PR objectives of preventing user confusion.
97-104
: Comprehensive explanation of the UI behavior issue.The warning effectively explains:
- The misleading UI representation
- The actual behavior
- The impact on configuration
- The end result
This directly addresses the PR objective of helping users understand why their configuration changes might not take effect.
87-104
: Well-structured and hierarchical presentation of warnings.The sequence and placement of the warnings create a logical flow:
- What to avoid
- What to do instead
- Why this matters
This structure helps users understand the issue and its solution before diving into specific configuration options.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
90-90: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
96-96: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
Thinking about this more, let's remove the optional flags. That should take care of everything. If this is problematic, lets just make everything mandatory. |
1 option is problematic: Where:
Here I can't figure out a nice way to configure the current default behaviour, ie. using the supported interfaces. The Converting it into an empty list, would break existing configs. Using:
And populating it on startup can work, but fails if user tries to edit the config before starting the add-on up. I run out of ideas. |
Proposed Changes
This issue is much more serious than it seems, users always show configurations where most of the default enabled options are enabled manually, and complain that disabled options are not working or hard to make them to work.
They waste hours on forums and debugging a configuration issue caused by a very misleading UI config editor!
Related Issues
fixes #462
Summary by CodeRabbit
Summary by CodeRabbit