feat: hide the network settings a device does not report as capabilities - #1658
adamshiervani wants to merge 1 commit into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a4a68f. Configure here.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a4a68f689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { value: "local", label: m.network_domain_local() }, | ||
| { value: "custom", label: m.network_domain_custom() }, | ||
| ]} | ||
| disabled={readOnlyFields.has("domain")} |
There was a problem hiding this comment.
When a device reports a custom domain and includes domain in read_only_fields, this disables only the selector; the InputFieldWithLabel rendered for domain === "custom" remains editable and updates customDomain. If the user then changes any writable setting, prepareSettings includes that edited domain in setNetworkSettings, causing the device to reject the whole save—the failure this change is intended to prevent. The custom-domain input should use the same read-only condition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf9be1a: the custom domain field uses the same read-only condition as the selector. Checked on a JetKVM with getNetworkSettings rewritten in the browser to domain "lab.example.com": with read_only_fields ["domain"] the field shows the value and is disabled; with [] it is editable.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf9be1a5e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { value: "ipv4_only", label: m.network_mdns_ipv4_only() }, | ||
| { value: "ipv6_only", label: m.network_mdns_ipv6_only() }, | ||
| ]} | ||
| disabled={readOnlyFields.has("mdns_mode")} |
There was a problem hiding this comment.
When another device returns "unknown" (which is explicitly allowed by the mDNSMode type) or a device-specific mode while also marking mdns_mode read-only, this disabled select has no matching option and therefore renders blank. The same problem affects the newly disabled time-sync, IPv6, and LLDP selects; unlike the DHCP select, they do not add a fallback option for the reported value, so users cannot see the current setting. Add the reported value as an option whenever it is absent from each fixed option list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 52872f2: withReported() adds a reported value that a select does not list as its own option, for the mDNS, time sync, DHCP client, IPv6 and LLDP selects (it replaces the DHCP client's own version of the rule). Checked on a JetKVM with getNetworkSettings rewritten to mdns_mode "unknown", ipv6_mode "dhcpv6", dhcp_client "esp-netif", all read-only: each select shows the reported value and is disabled.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The JetKVM reports four more capabilities: http_proxy, domain, mdns and ipv6. The Network page shows each of those settings only when the device reports it, as other pages already do for shell, extensions and custom_edid. A DHCP client value the page does not offer (a device with one fixed client) shows as text instead of a select. Hidden settings stay in the form with the values the device reported, so a save sends them unchanged.
52872f2 to
ffa7153
Compare
adamshiervani
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Problem
The same UI runs on devices with a smaller network stack than the JetKVM's. The JetKVM Mini, a smaller JetKVM with its own firmware that serves this UI, has no HTTP proxy, mDNS or IPv6 settings, a fixed domain, and one built-in DHCP client,
lwip. On such a device the Network settings page offers controls whose changes the device refuses on save, and the DHCP client select is blank.Cause
Pages already hide the controls for features that a device does not list in
getDeviceCapabilitiesand thedeviceCapabilitiesevent, for exampleshellandcustom_edid, but no capability exists for the network settings. The DHCP client select offers onlyjetdhcpcandudhcpc, so a device that reportslwipmatches neither option and the select shows no value.Fix
rpcGetDeviceCapabilities()injsonrpc.gonow also returnshttp_proxy,domain,mdnsandipv6, with the comment above it updated, andstores.tsadds the four names to theCapabilitytype. The JetKVM can change all four settings, so its page is unchanged.devices.$id.settings.network.tsxshows each setting only when the device reports its capability:http_proxythe HTTP proxy field,domainthe domain select and custom domain field,mdnsthe mDNS select, andipv6the IPv6 mode select and the IPv6 card. The DHCP client gets no capability, because a device with one fixed client has no choice to offer. When the device reports a client outside the newDhcpClientOptionslist (jetdhcpc,udhcpc), the page shows its name as text instead of the select.A hidden control stays in the form with the value that the device reported, so a save sends that value unchanged and a device that cannot change the setting accepts the save.
oxfmtalso rewrapped the@hooks/storesimport and two longifconditions in the same file.Testing
The new spec
ui/e2e/network-capabilities.spec.tsruns against a JetKVM. The first test checks thatgetDeviceCapabilitiescontains the four capabilities and that the page shows the four settings and the DHCP client select. The second test removes the four capabilities in the browser and replaces the network settings with the Mini's values (dhcp_client: "lwip",http_proxy: "",domain: "local",mdns_mode: "disabled",ipv6_mode: "disabled"). It checks that the settings are hidden and thatlwipis shown as text, then changes the hostname and checks that the save, which the browser answers, carries the reported values for the hidden settings and the DHCP client.Both builds were installed on a JetKVM with
dev_deploy.sh --install:dev(before)getDeviceCapabilitieshas nohttp_proxylwiptext, the DHCP client select is shownOn this branch, the new spec,
network-settings.spec.tsandnetwork-lease-timer.spec.tseach ran 3 more times, and all 12 test executions passed.tsc(app and e2e),oxlint,oxfmtandgo vetpass.Not in this PR
LLDP gets no capability, because the page already hides it with
isLLDPAvailable = falseand the JetKVM does not implement it.