Local image url warnings #711
Conversation
Summary of ChangesHello @tavdog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience by proactively alerting users to potential configuration problems. It identifies when a device's image URL is set to a local address (localhost or 127.0.0.1), which is typically problematic for devices operating outside the server's local machine. By displaying a clear warning, the system guides users to correct their settings, preventing future connectivity issues without requiring them to discover the problem through trial and error. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds helpful warnings to the UI when a device's image URL is configured to use localhost or 127.0.0.1, which is a great improvement for user experience. The implementation is straightforward.
My review includes a few suggestions:
- Refactoring duplicated logic for checking the URL into a helper function.
- Improving how localization strings are handled in the templates for better maintainability.
- Addressing a critical JavaScript bug in
update.htmlwhere functions are being redefined, which would break existing functionality. - Fixing a minor UI issue in
update.htmlwhere two different sections have the same heading.
Overall, the core feature is well-implemented, and with these fixes, the PR will be in great shape.
web/templates/manager/update.html
Outdated
| <script> | ||
| function resetImgUrl() { | ||
| const imageUrl = document.getElementById("img_url").value.trim(); | ||
| if (imageUrl) { | ||
| document.getElementById("reported_img_url").value = imageUrl; | ||
| updateFirmwareSettings({image_url: imageUrl}); | ||
| } | ||
| } | ||
|
|
||
| function resetWsUrl() { | ||
| const wsUrl = document.getElementById("ws_url").value.trim(); | ||
| if (wsUrl) { | ||
| document.getElementById("reported_ws_url").value = wsUrl; | ||
| updateFirmwareSettings({ws_url: wsUrl}); | ||
| } | ||
| } | ||
| </script> |
There was a problem hiding this comment.
This script block redefines the functions resetImgUrl and resetWsUrl, which are already defined earlier in this file. This will break the "Reset" button functionality for the main URL fields.
The new functions also have misleading names; they seem to be for syncing URLs from the main device settings to the live firmware settings, not for resetting them.
To fix this, you should rename these new functions to something more descriptive (e.g., syncImgUrlToFirmware) and ensure they don't conflict with existing functions. If these new functions are intended to be used, they should be called by appropriate UI elements.
<script>
function syncImgUrlToFirmware() {
const imageUrl = document.getElementById("img_url").value.trim();
if (imageUrl) {
document.getElementById("reported_img_url").value = imageUrl;
updateFirmwareSettings({image_url: imageUrl});
}
}
function syncWsUrlToFirmware() {
const wsUrl = document.getElementById("ws_url").value.trim();
if (wsUrl) {
document.getElementById("reported_ws_url").value = wsUrl;
updateFirmwareSettings({ws_url: wsUrl});
}
}
</script>| // Check if URL contains localhost | ||
| imgURL := device.ImgURL | ||
| var urlWarning string | ||
| if strings.Contains(imgURL, "localhost") || strings.Contains(imgURL, "127.0.0.1") { | ||
| urlWarning = "localhost" | ||
| } |
There was a problem hiding this comment.
The logic to check for "localhost" or "127.0.0.1" in the image URL is duplicated from internal/server/firmware.go. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a shared helper function.
For example, you could create a function like isLocalhostURL(url string) bool in internal/server/helpers.go and call it from both handleUpdateDeviceGet and handleFirmwareGenerateGet. This would centralize the logic and make future changes easier.
| <i class="fa-solid fa-triangle-exclamation"></i> {{ t .Localizer "Warning: Localhost URL Detected" }} | ||
| </h4> | ||
| <p> | ||
| {{ t .Localizer "The firmware image URL uses localhost (" }}<strong>{{ .Device.ImgURL }}</strong>{{ t .Localizer ") which will not work when the device is running on a different network. Either replace localhost with the IP or hostname of this server, or connect to this server over its IP or hostname, edit the device and click the refresh buttons by the devices image URLs, then return here." }} |
There was a problem hiding this comment.
The warning message is constructed by concatenating multiple translated strings and variables. This is not ideal for localization as it makes it difficult for translators to provide accurate and natural-sounding translations for different languages. It's better to use a single translation key with placeholders. This also applies to web/templates/manager/update.html.
A better approach would be:
<p>
{{ t .Localizer "LocalhostURLWarning" (dict "ImgURL" .Device.ImgURL) }}
</p>And in your en.json file, you would have a single entry:
"LocalhostURLWarning": {
"other": "The firmware image URL uses localhost (<strong>{{.ImgURL}}</strong>) which will not work when the device is running on a different network. Either replace localhost with the IP or hostname of this server, or connect to this server over its IP or hostname, edit the device and click the refresh buttons by the devices image URLs, then return here."
}This makes the string a single unit for translators.
web/templates/manager/update.html
Outdated
| </script> | ||
| <div class="device-settings-section"> | ||
| <h2>{{ t .Localizer "Dim Mode Settings" }}</h2> | ||
| <h2>{{ t .Localizer "Display Settings" }}</h2> |
There was a problem hiding this comment.
This section heading has been changed from "Dim Mode Settings" to "Display Settings". However, another section further down (previously "Firmware Features") has also been renamed to "Display Settings" (line 721). Having two sections with the same title on the same page is confusing for users.
I recommend reverting this heading back to "Dim Mode Settings" to maintain clarity and distinguish it from the other "Display Settings" section.
<h2>{{ t .Localizer "Dim Mode Settings" }}</h2>
warning banners in proper locations
tavdog
force-pushed
the
local_image_url_warnings
branch
from
3ff65f6 to
22b056d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a helpful warning to the UI when a device's image URL is set to localhost or 127.0.0.1. The implementation in the Go handlers is sound, but there's an opportunity to reduce code duplication by creating a shared helper function. My main concerns are with the template changes. In firmware.html, the form has been restructured in a way that will prevent it from submitting correctly, which is a critical issue. In update.html, there are a couple of issues: a JavaScript function is being redefined, which will cause bugs, and a section has been renamed to a title that's already in use, which will be confusing for users. Please see my detailed comments for suggestions on how to address these issues.
web/templates/manager/firmware.html
Outdated
| <form method="post" id="firmware-form"> | ||
| <div class="device-settings-section"> | ||
| <div class="config-management-container"> | ||
| <button class="w3-button w3-blue config-management-btn" type="submit"> | ||
| <i class="fa-solid fa-microchip" aria-hidden="true"></i> {{ t .Localizer "Generate Firmware File" }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </form> | ||
| <hr> | ||
| <script> | ||
| </form> |
There was a problem hiding this comment.
The <form> tag has been moved to only wrap the submit button. This means that all the other input fields (like WiFi credentials and Image URL) are now outside the form and will not be submitted when the 'Generate Firmware File' button is clicked. This will break the firmware generation functionality. The <form> tag should be moved back to its original position at line 60 to wrap all the necessary input fields.
web/templates/manager/update.html
Outdated
| <script> | ||
| function resetImgUrl() { | ||
| const imageUrl = document.getElementById("img_url").value.trim(); | ||
| if (imageUrl) { | ||
| document.getElementById("reported_img_url").value = imageUrl; | ||
| updateFirmwareSettings({image_url: imageUrl}); | ||
| } | ||
| } | ||
|
|
||
| function resetWsUrl() { | ||
| const wsUrl = document.getElementById("ws_url").value.trim(); | ||
| if (wsUrl) { | ||
| document.getElementById("reported_ws_url").value = wsUrl; | ||
| updateFirmwareSettings({ws_url: wsUrl}); | ||
| } | ||
| } | ||
| </script> |
There was a problem hiding this comment.
You are defining resetImgUrl and resetWsUrl functions here, but these functions are already defined earlier in this file (lines 163-170). This re-definition will override the original functions and likely cause unintended behavior for the 'Reset' buttons. The new functions also have misleading names as they don't 'reset' but rather seem to sync values. Please rename these new functions to avoid conflict and clarify their purpose, for example syncImgUrlToFirmware and syncWsUrlToFirmware. Also, the resetWsUrl function references an element with ID reported_ws_url which does not exist in the page, this will cause a runtime error.
web/templates/manager/update.html
Outdated
| <div class="device-settings-section"> | ||
| <h2>{{ t .Localizer "Display Settings" }}</h2> |
There was a problem hiding this comment.
This section was previously titled 'Firmware Features' and is now 'Display Settings'. However, there is already another section titled 'Display Settings' earlier in the file (line 173). Having two sections with the same title can be confusing for users. Please consider using a more distinct title for this section, like 'Advanced Firmware Settings' or similar, to differentiate it from the other 'Display Settings' section.
Furthermore, the wrapping condition for this block was changed from if .Device.SupportsFirmwareFeatures to if .Device.OTACapable. This makes this section visible for devices with older firmware that might not support these features, which could lead to issues. It's recommended to revert to the more restrictive if .Device.SupportsFirmwareFeatures condition to ensure these settings are only shown for compatible devices.
| if strings.Contains(imgURL, "localhost") || strings.Contains(imgURL, "127.0.0.1") { | ||
| urlWarning = "localhost" | ||
| } |
There was a problem hiding this comment.
This logic to check for localhost URLs is also present in internal/server/handlers_device.go. To avoid code duplication, consider extracting this into a shared helper function, for example in helpers.go.
Additionally, urlWarning is used as a boolean flag in the template. It would be more idiomatic to use a bool type for this instead of a string. The helper function could return a boolean, and the URLWarning field in TemplateData could be changed to bool.
| if strings.Contains(imgURL, "localhost") || strings.Contains(imgURL, "127.0.0.1") { | ||
| urlWarning = "localhost" | ||
| } |
There was a problem hiding this comment.
This logic to check for localhost URLs is duplicated from internal/server/firmware.go. To improve maintainability and avoid code duplication, it would be better to extract this into a shared helper function.
A boolean flag would also be more idiomatic here instead of a string. You could create a helper function that returns a boolean, and update the URLWarning field in TemplateData to be of type bool.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a helpful warning to the UI when an image URL contains 'localhost' or '127.0.0.1', which is a common source of issues. The implementation is mostly correct, but I've identified a few areas for improvement. There's some duplicated code in the Go backend that could be refactored into a helper function for better maintainability. More critically, I found a couple of issues in the update.html template: a JavaScript bug that breaks the 'Reset to default' URL functionality, and a change in template logic that could incorrectly show the 'Firmware Features' section for unsupported devices. Addressing these points will improve the robustness and maintainability of the code.
web/templates/manager/update.html
Outdated
| <script> | ||
| function resetImgUrl() { | ||
| const imageUrl = document.getElementById("img_url").value.trim(); | ||
| if (imageUrl) { | ||
| document.getElementById("reported_img_url").value = imageUrl; | ||
| updateFirmwareSettings({image_url: imageUrl}); | ||
| } | ||
| } | ||
|
|
||
| function resetWsUrl() { | ||
| const wsUrl = document.getElementById("ws_url").value.trim(); | ||
| if (wsUrl) { | ||
| document.getElementById("reported_ws_url").value = wsUrl; | ||
| updateFirmwareSettings({ws_url: wsUrl}); | ||
| } | ||
| } | ||
| </script> |
There was a problem hiding this comment.
This script block redefines the resetImgUrl and resetWsUrl functions with incorrect logic for the "Reset to default" buttons. The new functions do not reset the URLs but instead copy the current value from one input to another and trigger a firmware settings update. This breaks the reset functionality. The original functions, which are still present earlier in the file, are correct. This new block should be removed to restore the intended behavior.
| </div> | ||
| </div> | ||
| {{ end }} | ||
| {{ if .Device.SupportsFirmwareFeatures }} |
There was a problem hiding this comment.
The "Firmware Features" section was previously guarded by {{ if .Device.SupportsFirmwareFeatures }}. This change removes that specific guard and includes the section within the broader {{ if .Device.OTACapable }} block. The SupportsFirmwareFeatures check is more restrictive and accurate for that section's content. Removing it may cause the UI to display these features for devices that do not support them, leading to user confusion and potential errors. The original conditional logic should be restored, with the "Firmware Features" section wrapped in its own {{ if .Device.SupportsFirmwareFeatures }} ... {{ end }} block, separate from the {{ if .Device.OTACapable }} block.
| // Check if URL contains localhost | ||
| imgURL := device.ImgURL | ||
| var urlWarning string | ||
| if strings.Contains(imgURL, "localhost") || strings.Contains(imgURL, "127.0.0.1") { | ||
| urlWarning = "localhost" | ||
| } |
There was a problem hiding this comment.
This logic to check for "localhost" or "127.0.0.1" in the URL is also present in internal/server/handlers_device.go. To avoid code duplication and improve maintainability, this check should be extracted into a shared helper function, for example in helpers.go.
| // Check if URL contains localhost | ||
| imgURL := device.ImgURL | ||
| var urlWarning string | ||
| if strings.Contains(imgURL, "localhost") || strings.Contains(imgURL, "127.0.0.1") { | ||
| urlWarning = "localhost" | ||
| } |
There was a problem hiding this comment.
This logic to check for "localhost" or "127.0.0.1" is duplicated from internal/server/firmware.go. To follow the DRY (Don't Repeat Yourself) principle, this should be extracted into a common helper function.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a warning banner on the firmware and device edit pages when the image URL contains localhost or 127.0.0.1. The implementation is generally good, but there are a couple of issues. There's some code duplication in the Go backend for checking the URL, which should be refactored into a helper function. More critically, there's an HTML issue in firmware.html where a change has resulted in a nested form, a duplicate element ID, and an unclosed form tag, which will break the page's functionality. I've provided suggestions to address these points.
web/templates/manager/firmware.html
Outdated
| <form method="post" id="firmware-form"> | ||
| <div class="device-settings-section"> | ||
| <div class="config-management-container"> | ||
| <button class="w3-button w3-blue config-management-btn" type="submit"> | ||
| <i class="fa-solid fa-microchip" aria-hidden="true"></i> {{ t .Localizer "Generate Firmware File" }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </form> | ||
| <hr> | ||
| <script> | ||
| </form> |
There was a problem hiding this comment.
This change introduces invalid HTML. It creates a nested <form> element (which is not allowed), duplicates the id="firmware-form", and removes the closing tag for the outer form that starts on line 60, leaving it unclosed.
The original structure was correct, with the button inside the main form. I suggest reverting this part of the change to restore the valid HTML structure.
| <form method="post" id="firmware-form"> | |
| <div class="device-settings-section"> | |
| <div class="config-management-container"> | |
| <button class="w3-button w3-blue config-management-btn" type="submit"> | |
| <i class="fa-solid fa-microchip" aria-hidden="true"></i> {{ t .Localizer "Generate Firmware File" }} | |
| </button> | |
| </div> | |
| </div> | |
| </div> | |
| </form> | |
| <hr> | |
| <script> | |
| </form> | |
| <div class="device-settings-section"> | |
| <div class="config-management-container"> | |
| <button class="w3-button w3-blue config-management-btn" type="submit"> | |
| <i class="fa-solid fa-microchip" aria-hidden="true"></i> {{ t .Localizer "Generate Firmware File" }} | |
| </button> | |
| </div> | |
| </div> | |
| </form> | |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a warning banner on the firmware and device edit pages when the image URL contains localhost or 127.0.0.1. The implementation is generally good, but I've identified a few areas for improvement regarding code duplication, internationalization, and a critical bug in one of the HTML templates that would break form submission. Please see my detailed comments.
web/templates/manager/firmware.html
Outdated
| <form method="post" id="firmware-form"> | ||
| <div class="device-settings-section"> | ||
| <div class="config-management-container"> | ||
| <button class="w3-button w3-blue config-management-btn" type="submit"> | ||
| <i class="fa-solid fa-microchip" aria-hidden="true"></i> {{ t .Localizer "Generate Firmware File" }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </form> | ||
| <hr> | ||
| <script> | ||
| </form> |
There was a problem hiding this comment.
This change introduces a nested <form> element with a duplicate ID, which is invalid HTML and will break form submission. The outer <form> tag from line 60 is also left unclosed. This appears to be unintentional and should be reverted to the original structure where the button is inside the main form, which is closed properly.
<div class="device-settings-section">
<div class="config-management-container">
<button class="w3-button w3-blue config-management-btn" type="submit">
<i class="fa-solid fa-microchip" aria-hidden="true"></i> {{ t .Localizer "Generate Firmware File" }}
</button>
</div>
</div>
</form>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a warning banner on the firmware and device edit pages when the image URL contains 'localhost' or '127.0.0.1'. The implementation is correct, but there are several instances of code duplication in both the Go backend and the HTML templates. My review includes suggestions to refactor the duplicated logic into helper functions and partial templates to improve maintainability. I've also pointed out an issue with missing localization strings for the new UI text.
warning banners on both firmware page and device edit page about localhost/127 in the image url