Fix Shortcuts support for deleted homes with fallback mechanism #1005
Conversation
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements a fallback mechanism for Shortcuts support when a previously configured home has been deleted, preventing shortcuts from failing. The solution falls back to the first available home when the originally configured home is not found.
Key Changes:
- Added fallback logic across all intent handlers to use the first available home when the configured home is deleted
- Updated
Preferences.listStoredHomes()to be async to properly handle actor isolation - Added comprehensive logging throughout intent handling and item caching for better debugging
- Enhanced
OpenHABItemCachewith validation checks to ensure homes exist before attempting operations - Removed unnecessary
@MainActorannotation fromOpenHABIntentHelper
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 35 comments.
Show a summary per file
| File | Description |
|---|---|
| openHABIntents/SetSwitchStateIntentHandler.swift | Implements home fallback mechanism and replaces homeId with actualHomeId throughout the handler |
| openHABIntents/SetStringValueIntentHandler.swift | Implements home fallback mechanism and replaces homeId with actualHomeId throughout the handler |
| openHABIntents/SetNumberValueIntentHandler.swift | Implements home fallback mechanism and replaces homeId with actualHomeId throughout the handler |
| openHABIntents/SetDimmerRollerValueIntentHandler.swift | Implements home fallback mechanism and replaces homeId with actualHomeId throughout the handler |
| openHABIntents/SetContactStateValueIntentHandler.swift | Implements home fallback mechanism and replaces homeId with actualHomeId throughout the handler |
| openHABIntents/SetColorValueIntentHandler.swift | Implements home fallback mechanism and replaces homeId with actualHomeId throughout the handler |
| openHABIntents/OpenHABIntentHelper.swift | Removes @mainactor annotation, adds fallback logic to resolveHome and getAllItems, makes getHomeOptions async, adds extensive logging |
| openHABIntents/IntentHandler.swift | Adds logging for initialization diagnostics |
| openHABIntents/GetItemStateIntentHandler.swift | Implements home fallback mechanism, adds logging, replaces homeId with actualHomeId |
| openHAB/HomeSelectionView.swift | Updates loadHomesList to properly await async listStoredHomes call |
| openHAB.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | Updates dependency versions and changes file format from version 3 to version 2 |
| OpenHABCore/Sources/OpenHABCore/Util/Preferences.swift | Makes listStoredHomes async to properly handle actor isolation |
| OpenHABCore/Sources/OpenHABCore/Util/OpenHABItemCache.swift | Adds home existence validation before operations, enhances error handling, improves logging for diagnostics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| actualHomeId = homeId | ||
| } else { | ||
| Logger.intentHandling.warning("Home \(homeId) not found in handle. Falling back to first available home") | ||
| guard let firstHome = storedHomes.first else { |
There was a problem hiding this comment.
Using .first on a dictionary provides non-deterministic ordering, which means the fallback home selection could vary unpredictably between runs. This could lead to inconsistent behavior when the intended home is deleted. Consider sorting the homes (e.g., by name or creation date) before selecting the first one to ensure consistent fallback behavior:
guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else {| guard let firstHome = storedHomes.first else { | |
| guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else { |
| actualHomeId = homeId | ||
| } else { | ||
| Logger.intentHandling.warning("Home \(homeId) not found in handle. Falling back to first available home") | ||
| guard let firstHome = storedHomes.first else { |
There was a problem hiding this comment.
Using .first on a dictionary provides non-deterministic ordering, which means the fallback home selection could vary unpredictably between runs. This could lead to inconsistent behavior when the intended home is deleted. Consider sorting the homes (e.g., by name or creation date) before selecting the first one to ensure consistent fallback behavior:
guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else {| guard let firstHome = storedHomes.first else { | |
| guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else { |
| actualHomeId = homeId | ||
| } else { | ||
| Logger.intentHandling.warning("Home \(homeId) not found in handle. Falling back to first available home") | ||
| guard let firstHome = storedHomes.first else { |
There was a problem hiding this comment.
Using .first on a dictionary provides non-deterministic ordering, which means the fallback home selection could vary unpredictably between runs. This could lead to inconsistent behavior when the intended home is deleted. Consider sorting the homes (e.g., by name or creation date) before selecting the first one to ensure consistent fallback behavior:
guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else {| guard let firstHome = storedHomes.first else { | |
| guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else { |
| actualHomeId = homeId | ||
| } else { | ||
| Logger.intentHandling.warning("Home \(homeId) not found in handle. Falling back to first available home") | ||
| guard let firstHome = storedHomes.first else { |
There was a problem hiding this comment.
Using .first on a dictionary provides non-deterministic ordering, which means the fallback home selection could vary unpredictably between runs. This could lead to inconsistent behavior when the intended home is deleted. Consider sorting the homes (e.g., by name or creation date) before selecting the first one to ensure consistent fallback behavior:
guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else {| guard let firstHome = storedHomes.first else { | |
| guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first else { |
| actualHomeId = homeId | ||
| } else { | ||
| Logger.intentHandling.warning("Home \(homeId) not found in storedHomes. Available homes: \(storedHomes.keys)") | ||
| guard let firstHome = storedHomes.keys.first else { |
There was a problem hiding this comment.
Using .keys.first on a dictionary provides non-deterministic ordering, which means the fallback home selection could vary unpredictably between runs. This could lead to inconsistent behavior when the intended home is deleted. Consider sorting the homes (e.g., by name) before selecting the first one to ensure consistent fallback behavior:
guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first?.key else {| guard let firstHome = storedHomes.keys.first else { | |
| // Select fallback home deterministically by sorting by homeName | |
| guard let firstHome = storedHomes.sorted(by: { $0.value.homeName < $1.value.homeName }).first?.key else { |
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
| for homeId in homes { | ||
| items[homeId] = loadedItems[homeId] ?? [] | ||
| } |
There was a problem hiding this comment.
I liked the one liner more, I think it should work with the fallback, too. But feel free to write it according to your taste.
| private func loadItems(homeId: UUID) async -> [OpenHABItem]? { | ||
| guard let networkTracker = await assureNetworkTracker(homeId: homeId) else { | ||
| Logger.itemCache.error("Home \(homeId) not reachable") | ||
| Logger.itemCache.error("Home \(homeId) not reachable - not found in storedHomes") |
There was a problem hiding this comment.
I am not sure whether that new part in the log line is accurate, there may be other reasons like timeouts which cause the network tracker to be nil. Maybe you can replace the "-" with ", potentially because" or just log the failure in the assureNetworkTracker method if you don´t do that already anyway.
| do { | ||
| let items = try await networkTracker.getStaticItems() | ||
| Logger.itemCache.info("Successfully loaded \(items.count) items for home \(homeId)") | ||
| return items | ||
| } catch { | ||
| Logger.itemCache.error("Failed to load items for home \(homeId): \(error.localizedDescription)") |
There was a problem hiding this comment.
Instead of the verbose do-catch syntax we can do a guard let items = try?... here with log and return in the else branch?
| let homes = await Preferences.shared.listStoredHomes() | ||
| Logger.intentHandling.info("IntentHandler init: Found \(homes.count) stored homes: \(homes)") |
There was a problem hiding this comment.
Why are these two lines necessary in this method? I would not call it just for logging.
|
I am not fully convinced that falling back to a random (deterministically chosen) home when a home is deleted is the right approach. If the item names in homes are distinct, a user of shortcuts can just leave the home selection empty, which would have the desired effect. If the item name is not distinct, the choice for a certain home was most likely made with determination, and just using the other home with the same item name might have undesired effects. I would rather like to warn the user of the problem that a home was not found and make him figure out the solution (change or remove the home from his shortcuts, or remove the obsolete shortcut). |
No description provided.