Skip to content

Add a cancel button to the onboarding device name step - #6067

Merged
bgoncal merged 12 commits into
home-assistant:mainfrom
RaHehl:onboarding-dead-ends
Oct 15, 2026
Merged

bgoncal merged 12 commits into
home-assistant:mainfrom
RaHehl:onboarding-dead-ends

Conversation

@RaHehl

@RaHehl RaHehl commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

AI Policy

Select exactly one option that describes AI usage in this contribution:

  • I have not used AI for this contribution.
  • AI assistance was used for this contribution.
  • AI fully generated the code for this contribution, but I've reviewed and understood it before submitting and will respond without AI during review.

Summary

The device name screen has had no way out since #5178. If saving keeps failing, the app has to be killed.

  • Adds a Cancel button.
  • A failure before the server is saved no longer waits for the token revocation and no longer deletes an existing server with the same instance ID.

Tested in the simulator, including the invitation flow.

Screenshots

Device name

device-name-cancel.mp4

Invitation

invitation-cancel-frozen.mp4

Second server

second-server-cancel.mp4

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

Permissions back button: #6071. Android already has a back button here.

Since the onboarding rewrite pushed this screen with its navigation bar hidden, it had no
way out: a save that keeps failing (for example when the server is unreachable) left the
user stuck. Show the bar with a Cancel button that ends the auth flow and leaves the screen
right away instead of waiting for the token revocation.
The permissions steps are not on a navigation stack, so a choice could not be revised:
after picking the most secure local access, the home network step could only be left by
entering a network name. Show a back button in the navigation bar from the second step on.
Where the flow is opened from the app with a close button, that button moves to the
trailing edge.
@RaHehl
RaHehl marked this pull request as ready for review October 14, 2026 18:51
Copilot AI balanced review requested due to automatic review settings October 14, 2026 18:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Delayed callbacks can disrupt a subsequent login or leave home-network navigation unable to advance.

2 open findings
What changed in this PR

Restores exits from the companion app’s onboarding screens after the SwiftUI navigation rewrite.

Changes:

  • Adds device-name cancellation and immediate return to the servers list.
  • Adds back navigation within permissions and moves the container close button.
  • Adds navigation and cancellation tests plus a localized Back label.
File Description
Tests/​App/​Onboarding/​OnboardingPermissionsNavigationViewModelTests.swift Tests back navigation and state reset.
Tests/​App/​Auth/​OnboardingAuthStepDeviceNaming.test.swift Tests cancellation guards.
Sources/​Shared/​Resources/​Swiftgen/​Strings.swift Adds generated Back accessor.
Sources/​App/​Resources/​en.lproj/​Localizable.strings Adds English Back label.
Sources/​App/​Onboarding/​Steps/​Permissions/​OnboardingPermissionsNavigationViewModel.swift Adds previous-step navigation.
Sources/​App/​Onboarding/​Steps/​Permissions/​OnboardingPermissionsNavigationView.swift Adds permissions back button.
Sources/​App/​Onboarding/​Steps/​DeviceName/​DeviceNameView.swift Adds Cancel button.
Sources/​App/​Onboarding/​Container/​OnboardingNavigationView.swift Keeps navigation bars visible.
Sources/​App/​Onboarding/​API/​Steps/​OnboardingAuthStepDeviceNaming.swift Pops authentication immediately on cancellation.
Sources/​App/​Onboarding/​API/​OnboardingDeviceNameRequest.swift Renames and documents cancellation API.
Sources/​App/​Container/​ContainerView.swift Moves close button to trailing edge.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Sources/App/Onboarding/API/Steps/OnboardingAuthStepDeviceNaming.swift Outdated
@bgoncal

bgoncal commented Oct 14, 2026

Copy link
Copy Markdown
Member

The onboarding was designed to be one direction only to simplify edge cases. I can review this change but it will require testing all possible scenarios, with the conditional screens that we show or hide depending on the user’s setup.

On top of that, since this was done in coordination with Android, you will need to open a PR in there as well to keep the feature parity

A cancelled auth flow settles only after its cleanup, so its result could pop the pages of
a flow the user started in the meantime; only the latest flow's result is handled now. A
home network save that lands after going back no longer marks the step as done.
@RaHehl

RaHehl commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Makes sense. I opened this because I got stuck myself: I was on VPN to a remote site I wanted to add, picked the location-based option because I thought it meant geofencing, and the next step then had my home Wi-Fi filled in. There was no way back to change my answer, so I had to kill the app in the middle of onboarding.

What's your take: should I prepare the same for Android, or do you see this as working as designed?

@bgoncal

bgoncal commented Oct 14, 2026

Copy link
Copy Markdown
Member

It's not nice to be locked into a situation like you were so at least for the naming device I would be Ok with, the permissions flow I truly have to spend time testing it to see if there are not gaps, if you could help me out, asking AI to map the possible flows and test if your implementation introduces a regression is also helpful.

Perhaps you can split this PR in 2? Then the same for Android

@bgoncal

bgoncal commented Oct 14, 2026

Copy link
Copy Markdown
Member

Please also test the secondary server onboard (when you add a second server to the app)

This pull request keeps only the device name cancel.
@RaHehl

RaHehl commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Split: this PR now only has the device name cancel. The permissions back button is in #6071.

@RaHehl RaHehl changed the title Restore exits from the onboarding device name and permissions steps Add a cancel button to the onboarding device name step Oct 15, 2026
Cancelling the device name step removed any server stored under the onboarded instance's identifier, though this flow had not saved anything yet.
@bgoncal
bgoncal requested a balanced review from Copilot October 15, 2026 07:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Delayed cancellation can still delete a successful retry’s server, dismiss a replacement flow, or leave invitation controls disabled.

3 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread Sources/App/Onboarding/API/Steps/OnboardingAuthStepDeviceNaming.swift Outdated
Comment thread Sources/App/Onboarding/API/Steps/OnboardingAuthStepDeviceNaming.swift Outdated
Comment thread Sources/App/Onboarding/Steps/Servers/OnboardingServersListViewModel.swift Outdated
…ection

Webhooks sent during onboarding cache an API for the unsaved server; a failure before saving now evicts only that one. A cancelled flow that settles while a newer selection still waits for discovery no longer resets the newer flow's loading state.
@RaHehl
RaHehl requested a balanced review from Copilot October 15, 2026 07:35

@bgoncal bgoncal left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up and for the quick follow-ups on the cleanup path.

Comment thread Sources/App/Onboarding/API/Steps/OnboardingAuthStepDeviceNaming.swift Outdated
@home-assistant
home-assistant Bot marked this pull request as draft October 15, 2026 07:39
@home-assistant

Copy link
Copy Markdown

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cancellation crosses navigation lifecycles and delayed authentication cleanup, requiring human confirmation of lifecycle safety.

3 open findings

🧠 Review effort: Balanced

…is gone

Cancel pops the auth pages long before the cancelled flow settles, so the invitation buttons and the manual URL spinner stayed disabled until token revocation returned. A result arriving after the servers list was released no longer pops a flow started from a new list.
With nothing saved there is nothing to roll back, so revoking the token runs in the background and the flow ends right away. That makes the early pop, the attempt tracking and the loader callback unnecessary, so they are removed again.
@RaHehl

RaHehl commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Tested both, cancel returns right away, retry works, existing server untouched. Videos are in the description.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused cancellation and cleanup changes have regression coverage, with no unresolved blocking findings.

0 open findings

3 resolved since last review

🧠 Review effort: Balanced

@RaHehl
RaHehl marked this pull request as ready for review October 15, 2026 09:05
@home-assistant
home-assistant Bot requested a review from bgoncal October 15, 2026 09:05

@bgoncal bgoncal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side, have you checked android?

@RaHehl

RaHehl commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Android already has a back button

@bgoncal
bgoncal enabled auto-merge (squash) October 15, 2026 09:51
@codecov

codecov Bot commented Oct 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.99%. Comparing base (4bdec75) to head (83d6e45).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...p/Onboarding/Steps/DeviceName/DeviceNameView.swift 0.00% 6 Missing ⚠️
...nboarding/Container/OnboardingNavigationView.swift 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (63.15%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6067      +/-   ##
==========================================
+ Coverage   54.81%   55.99%   +1.18%     
==========================================
  Files        1248     1254       +6     
  Lines       82776    83262     +486     
==========================================
+ Hits        45370    46622    +1252     
+ Misses      37406    36640     -766     
Files with missing lines Coverage Δ
Sources/App/Onboarding/API/OnboardingAuth.swift 99.09% <100.00%> (+0.03%) ⬆️
...p/Onboarding/API/OnboardingDeviceNameRequest.swift 100.00% <100.00%> (+10.52%) ⬆️
...nboarding/Container/OnboardingNavigationView.swift 32.53% <0.00%> (ø)
...p/Onboarding/Steps/DeviceName/DeviceNameView.swift 0.00% <0.00%> (ø)

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bgoncal
bgoncal merged commit faffb63 into home-assistant:main Oct 15, 2026
8 of 9 checks passed
bgoncal added a commit that referenced this pull request Oct 21, 2026
<!-- Thank you for submitting a Pull Request and helping to improve Home
Assistant. Please complete the following sections to help the processing
and review of your changes. Please do not delete anything from this
template. -->

## AI Policy
<!-- Please read our AI policy before contributing:
https://contributors.home-assistant.io/start/ai-policy/ -->
- [x] I have read the [Open Home Foundation AI
Policy](https://contributors.home-assistant.io/start/ai-policy/).

Select exactly one option that describes AI usage in this contribution:

- [ ] I have not used AI for this contribution.
- [ ] AI assistance was used for this contribution.
- [x] AI fully generated the code for this contribution, but I've
reviewed and understood it before submitting and will respond without AI
during review.

## Summary
<!-- Provide a brief summary of the changes you have made and most
importantly what they aim to achieve -->
After "Most secure", the home network step had no way back. It now has a
Back button in the navigation bar, on this step only. Going back
restores the previous security level; a location grant or Wi-Fi save
arriving afterwards no longer moves the flow on.

The back transition needed a fix: a removed view animates with its last
render, so the page being left is redrawn with the back direction first.
The direction now lives in the view.

Android counterpart: home-assistant/android#7642. Android keeps "Most
secure" selected instead of restoring the level.

### Flow map

```mermaid
flowchart TD
    A([Onboarding]) -->|no remote access set up| DIS[Local-only disclaimer]
    A -->|remote access set up| LOC
    DIS --> LOC["Location (first server)<br/>Privacy choices (added server)"]
    LOC -->|server has an HTTPS URL| DONE([Done])
    LOC -->|HTTP URLs only| LA[Local access]
    W([Frontend, HTTP-only server]) -->|location not decided| LOC2[Location]
    W -->|security level not decided| LA
    LOC2 --> LA
    LA -->|Less secure| DONE
    LA -->|Most secure, location denied| LA
    LA -->|Most secure, location allowed| HN[Home network]
    HN -->|Wi-Fi name saved| DONE
    HN ==>|"Back (this PR)"| LA
    linkStyle 12 stroke:#03a9f4,stroke-width:3px
```

### Tested

| # | Start | Path | Back shown | Result |
|---|---|---|---|---|
| 1 | First server, location allowed | Home network → Back → Less secure
| Home network only | Less secure |
| 2 | First server, location allowed | Home network → Back → Most secure
→ Wi-Fi name | Home network only | Most secure, SSID saved, no second
location prompt |
| 3 | First server, location denied | Most secure stays on Local access
→ Less secure | never | Less secure |
| 4 | Second server | Privacy → Local access → Home network → Back →
Less secure | Home network only | Second: Less secure, first server
unchanged |
| 5 | Frontend, location reset | Location → Local access → Home network
→ Back → Most secure → Wi-Fi name | Home network only, Close still there
| Less → Most secure, other server unchanged |

Simulator, result checked in the server settings. The new unit tests
fail without their change. The E2E test now goes back once, green in
light and dark.

## Screenshots
<!-- If this is a user-facing change not in the frontend, please include
screenshots in light and dark mode. -->
| | Light | Dark |
|---|---|---|
| Onboarding, home network | <img width="270" alt="Home network, light"
src="https://github.com/user-attachments/assets/5268aaf1-9cb8-47f5-a147-8699ab8bb2e7"
/> | <img width="270" alt="Home network, dark"
src="https://github.com/user-attachments/assets/e61fbbcb-e605-4046-98d3-9741f978b2c9"
/> |
| Frontend entry (Close and Back) | <img width="300" alt="Frontend
entry, Close and Back"
src="https://github.com/user-attachments/assets/4f9a5015-2389-42cc-9a1b-04d3f8f51b23"
/> | |

Back and forward again, light:


https://github.com/user-attachments/assets/6e36274f-dcfb-4ab4-9d54-551d34ddb6f3

Dark:


https://github.com/user-attachments/assets/fb3a53c5-d605-4848-b893-81eaea9cf6cf

## Link to pull request in Documentation repository
<!-- Pull requests that add, change or remove functionality must have a
corresponding pull request in the Companion App Documentation repository
(https://github.com/home-assistant/companion.home-assistant). Please add
the number of this pull request after the "#" -->
Documentation: home-assistant/companion.home-assistant#

## Any other notes
<!-- If there is any other information of note, like if this Pull
Request is part of a bigger change, please include it here. -->
Split from #6067.

---------

Co-authored-by: Bruno Pantaleão Gonçalves <5808343+bgoncal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants