WiFi dialog: add dialog to the UI

[jotaen4tinypilot]

Related #131, part (d). Stacked on #1812.

This PR adds a new dialog “Wi-Fi” to the “System→Network” menu, which makes the Wi-Fi feature available to end-users.

Summary of the discussion below:

• We made the wording so that it’s clear that we only store the WiFi credentials, but we don’t verify whether they are actually accepted by the WiFi router, or whether the WiFi is reachable at all. For further clarification, we also added a success state to the dialog.
• In a subsequent PR, we’ll turn the “country code” free text field into a dropdown, to make it easier for users to enter their correct and applicable value.

[jotaen4tinypilot]

@cghague Thanks for your feedback!

• Wording of Intro Text: Changed
• SSID Field Label: for space efficiency, I went with Network Name for the input field label, and used Wi-Fi SSID as placeholder.
• Country Code Field: that sounds sensible – I’ll make it a dropdown via a subsequent PR (this one is already pretty large). I’ve added it as subtask (g) to the main issue.
• “No Ethernet” Warning Message: Changed
• Wi-Fi vs WiFi: I’ve changed it to Wi-Fi in the UI, however I suggest to leave the internal code occurrences as they are, to avoid code churn. I left a code comment about this decision, similar to the note in the mass storage / virtual media dialog.

[jdeanwallace]

Automated comment from CodeApprove ➜

---

In: Discussion
Edge case scenario:

1. Device is connected via wifi only
2. User reads warning message about only being connected via wifi
3. User connects ethernet cable to device
4. User opens wifi dialog and disables wifi
5. User reopens wifi dialog
6. Web UI hangs and struggles to connect to device

Video demo:
Screen Recording 2024-07-04 at 14.41.19.mov

I think the main issue with this scenario is that even though we're able to detect network status of the ethernet and wifi connection, we don't know for sure which connection we're currently using.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 66
      device using Wi-Fi. If you change your settings, you might get

Should we stress that they are only connected using wifi?

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 132
  </div>

While testing on device, I was expecting a success page, but after apply the new wifi settings the dialog seemingly just closed.

In reality, the "Applying Changes" screen just flashed really quickly then closed.

Can we add a success/done screen like we have after an update has completed?

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 231
          const networkStatus = await getNetworkStatus();

Can we catch fatal network errors in this method?

I experienced an infinite loading screen after changing the wifi settings then reopening the wifi dialog:

(failed)net::ERR_TIMED_OUT

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 253
          });

We can also use Array.some for this, similar to what we did in video-settings-dialog:

tinypilot/app/templates/custom-elements/video-settings-dialog.html

Lines 507 to 516 in c046544

	// Save Button: only enable if the user actually changed some value.
	const hasChangedValues = [
	[this._getStreamingMode(), this._initialSettings.streamingMode],
	[this._getFrameRate(), this._initialSettings.frameRate],
	[this._getMjpegQuality(), this._initialSettings.mjpegQuality],
	[this._getH264Bitrate(), this._initialSettings.h264Bitrate],
	[h264StunServer, this._initialSettings.h264StunServer],
	[h264StunPort, this._initialSettings.h264StunPort],
	].some(([actualValue, initialValue]) => actualValue !== initialValue);
	this._elements.saveButton.disabled = !hasChangedValues;

---

👀 @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

---

In: Discussion
Yeah, I’m not sure we can really do something about this, but I’m also wondering whether we have to. My thinking is that – as a user – if I make changes to the network setup, then it’s somewhat expected that the browser connection may glitch or be interrupted for a while, and I’d probably naively try to reload the page until the connection is stable again.

I think this issue should also be alleviated by us guarding the dialog initialization in a try/catch (your other review comment), because then at least the dialog would error out once the timeout kicks in.

---

In: app/templates/custom-elements/wifi-dialog.html:
Yes, I’ve wrapped both network calls into a try/catch.

---

In: app/templates/custom-elements/wifi-dialog.html:
Ah right, that’s nicer – done.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 66
      device using Wi-Fi. If you change your settings, you might get

To me, the current phrasing would be clear, but I see your point that we might want to consider over-emphasizing this, to make sure the user really understands the situation. @cghague could you weigh in here?

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 132
  </div>

I was also debating this, but I’m actually not sure here. As far as I know, the update dialog is the only dialog where we provide a dedicated success state, but we don’t have such a state in any other transactional dialog that I can think of:

• The video settings dialog just closes after the operation went through
• The hostname dialog and the static IP dialog implicitly close, due to the redirect kicking in.
  • The static IP dialog at least displays a short success note for a brief moment before the redirect, but it’s actually a combination of a success message and an (auto-)confirm prompt.

To me, it would feel slightly more consistent right now to just close the WiFi dialog. Another point to consider would be that if we display a success state, then we need to make sure to communicate that we only successfully stored the WiFi settings, but that doesn’t mean that the WiFi connection is active now.

I’m still open to a success state, I just wanted to raise the above points for consideration.

Regarding the flickering, since we know that the enable/disable operations are rather short (due to the “fire and forget” mode), we could also consider to optimistically omit the loading state altogether.

Just for the records: related issue are #1639, #714 (comment).

---

👀 @jdeanwallace it's your turn please take a look

[jdeanwallace]

Automated comment from CodeApprove ➜

---

In: Discussion
Yeah this reminds me of the risk of setting a static IP (i.e., the user may lose connection to their device). For setting a static IP, we went through a lot of effort to make sure settings were reverted if they were incorrect. Are we sure we don't want to do the same here?

If the user doesn't have physical access to their device, then the risk feels similar to that of setting a static IP. However, if the user does have physical access then simply connecting an ethernet cable will resolve the issue.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 132
  </div>

...we need to make sure to communicate that we only successfully stored the WiFi settings, but that doesn’t mean that the WiFi connection is active now.

Yeah I think this is an important point. I felt that setting wifi credentials without knowing if the settings were correct (either by visually confirming that the password I typed was correct or the system giving me feedback that the credentials were correct) was slightly strange. No feedback makes me think the credential were correct, so I think perhaps we should make it clear that we don't actually verify their credentials, in our help text.

Regarding the flickering, since we know that the enable/disable operations are rather short (due to the “fire and forget” mode), we could also consider to optimistically omit the loading state altogether.

Yeah I think this can work. Without a results screen, a loading state seems pointless.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 225
            networkStatus = await getNetworkStatus();

(nit) Can we do this in parallel using Promise.all?

---

👀 @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

---

In: Discussion

Are we sure we don't want to do the same here?

We briefly discussed this question, and I think the situation with WiFi is slightly different than with static IP. While the user may lock themselves out with both features, the implications are slightly different:

• For fixing WiFi credentials, the user can plug in an Ethernet cable to regain access. This should be an obvious fix, because the user already needed an Ethernet cable to configure WiFi in the first place. So Ethernet is kind of the base case for connecting to the device, and the WiFi connection is kind of “optional”/additional.
• For fixing Static IP (as far as I know) you’d have to mount the SD card somewhere, and then manually revert the DHCP config via the file system. This is probably not obvious to most users, and it’s rather difficult to accomplish. It also may not be obvious that you may lock yourself out to begin with (i.e., without any fallback) when setting a Static IP. In that sense, the Static IP setting is not optional, but it’s more like “either it works, or you are in trouble” (at least seemingly).

The other aspect is that it’s legitimate for users to enter WiFi credentials ahead of time, e.g. without the respective WiFi network being reachable. In this case, it would be undesired for them if we just reverted the WiFi settings. I think this use-case will also become more apparent, if we’d decide to allow storing multiple WiFi connections. In this case, it may be even more likely that the user might want to manage their WiFi settings without being in reach of the respective networks. So if we’d issue a check of the credentials, we probably have to make it either optional or subtle enough that it also satisfies this use-case. We created #1814 to track this issue separately.

---

In: app/templates/custom-elements/wifi-dialog.html:
Sure!

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 132
  </div>

No feedback makes me think the credential were correct, so I think perhaps we should make it clear that we don't actually verify their credentials, in our help text.

I agree that it’s not optimal to not provide any feedback at all at the moment. We are currently discussing the “feedback” topic and exploring UI options in #1814.

The tricky thing with WiFi is that the user may decide to preconfigure their WiFi credentials ahead of time (while not being in reach of the WiFi yet), or that the WiFi network might be unavailable at config-time for other reasons. So I think we cannot trivially yield an error saying “your WiFi credentials are incorrect”, because we still can store the WiFi settings and use them for trying to connect.

So the process of enabling WiFi is actually two-fold: (a) storing and enabling the WiFi settings and (b) successfully connecting to the WiFi network. So maybe we can consider to clarify this distinction a bit better, to make the workings of the WiFi feature in that regard more transparent. If we’d wanted to do that, I’d currently see two options (UI mock-ups only for demo purposes, not meant as literal suggestion):

• We try to clarify how the feature works in the initial intro text of the dialog.
• We introduce a “success” state that the dialog transitions to after enabling WiFi, which includes a more or less subtle hint for clarification.

This also may be a question for @cghague or @shalver-tp to weigh in.

---

👀 @jdeanwallace it's your turn please take a look

[cghague]

Automated comment from CodeApprove ➜

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 66
      device using Wi-Fi. If you change your settings, you might get

The only situation when a user would be connected using "only" Wi-Fi is if they're using the access point mode, which could make this new wording misleading. I can also foresee the new wording confusing users using services like Tailscale. I recommend leaving this as it is.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 132
  </div>

Great discussion! I've read your comments, looked at the mockups, and reviewed the associated issues. I think the confusion comes from treating this as a tool to connect to a network when it's more accurately a tool for storing network credentials.

I, therefore, suggest changing the button labels to "Remove" and "Save" and updating the header to "Wi-Fi Credentials". This change will make what the feature does more obvious while keeping it recognizable and understandable. Most users will enter their credentials and connect immediately, but it will still be clear what's happening for those preconfiguring a device.

If we couple the above with @jotaen4tinypilot's "success state" mockup, there would be no ambiguity, and we'll have covered both use cases. I appreciate that we don't have a "success state" anywhere else, but similar scenarios are likely as we add more features, so this may simply be the first!

---

👀 @jdeanwallace, @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 134
  </div>

I’ve updated the wording and added in a success state, the flow now looks like this:

Screen Recording 2024-07-05 at 07.24.03.mov

@cghague could you do a final double-check of the wording of all texts, particularly the new success state ones? Shall we also make an adjustment to the initial intro text, to emphasize that we only store the credentials but don’t check whether they are sound?

---

👀 @jdeanwallace it's your turn please take a look

[jdeanwallace]

Automated comment from CodeApprove ➜

⏳ Approval Pending (2 unresolved comments)
Approval will be granted automatically when all comments are resolved

Nice work!

---

In: Discussion
Thanks for explanation and links. I'm glad the risks have been brought up previously and accepted. I'm happy to proceed as is.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 134
  </div>

The updated flow looks great to me, thanks!

You can resolve this discussion when ready.

---

👀 @jotaen4tinypilot it's your turn please take a look

[cghague]

Automated comment from CodeApprove ➜

⏳ Approval Pending (4 unresolved comments)
Approval will be granted automatically when all comments are resolved

LGTM

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 134
  </div>

Thanks, @jotaen4tinypilot; the new flow looks excellent! I've added two review comments with suggested wording. I recommend leaving the initial introduction text as it is, as too much detail on the process here might confuse users. I have some ideas for improving debug logs to track failed connection attempts, which should remove some ambiguity without overwhelming the interface. I'll create issues for the new ideas soon.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 139
      Your Wi-Fi credentials have been saved. Your TinyPilot device will now

Suggest:

Your Wi-Fi credentials have been saved. When your TinyPilot device is in range of the wireless network, it will automatically try to connect to it.

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 148
    <p>Your Wi-Fi credentials have been removed.</p>

Suggest:

Your Wi-Fi credentials have been removed. Your TinyPilot device will no longer try to connect to the wireless network.

---

👀 @jdeanwallace, @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

---

In: app/templates/custom-elements/wifi-dialog.html:
Resolved

---

In: app/templates/custom-elements/wifi-dialog.html:
Resolved

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 70
      device using Wi-Fi. If you change your settings, you might get

Resolved

---

In: app/templates/custom-elements/wifi-dialog.html:

> Line 134
  </div>

Alright, awesome, I’ve taken over the new wording!

[jdeanwallace]

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

[cghague]

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

Review approved on CodeApprove

Review approved on CodeApprove