Persistence config in setup wizard #3737
Conversation
#4313 Bundle Size — 12.73MiB (+0.11%).2f6b3b1(current) vs 710f811 main#4299(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #4313 |
Baseline #4299 |
|
|---|---|---|
 Initial JS |
1.55MiB |
1.55MiB |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
6.97% |
6.95% |
| Chunks |
615(+0.16%) |
614 |
| Assets |
696(+0.14%) |
695 |
| Modules |
2437(+0.08%) |
2435 |
| Duplicate Modules |
0 |
0 |
| Duplicate Code |
0% |
0% |
| Packages |
128 |
128 |
| Duplicate Packages |
1 |
1 |
Bundle size by type 2 changes 
2 regressions
| Current #4313 |
Baseline #4299 |
|
|---|---|---|
| JS |
11.02MiB (+0.12%) |
11.01MiB |
| CSS |
876.94KiB (+0.06%) |
876.4KiB |
| Fonts |
526.1KiB |
526.1KiB |
| Media |
295.6KiB |
295.6KiB |
| IMG |
45.73KiB |
45.73KiB |
| Other |
847B |
847B |
Bundle analysis report Branch mherwege:wizard_persistence Project dashboard
Generated by RelativeCI Documentation Report issue
mherwege
force-pushed
the
wizard_persistence
branch
from
4d855df to
e572ae3
Compare
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
mherwege
force-pushed
the
wizard_persistence
branch
from
e572ae3 to
6a5edd2
Compare
|
The Main UI check fails with a formatting problem. I can't say what it is. I tried running it locally and all files in the src folder in my repository fail, but no details about the issue. Apart from the formatting issue, this is ready for review. |
A new formatting tool (oxfmt) was added. So, |
I saw that and did install oxfmt. I did do: npm run format. But that basically changed all files in the src folder. |
Possibly. I did run format on your PR and it only touched the 2 new files (macos). Can you share an example of what oxfmt did to one of the other files? |
I see this reference to a potential issue in oxfmt: oxc-project/oxc#17856. Not sure if this is what we are seeing here. |
Signed-off-by: Mark Herwege <[email protected]>
|
I didn't see a visual difference in VScode, and also git did not detect a difference. So it is likely the eof issue. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new persistence configuration step to the setup wizard, allowing users to configure installed persistence services immediately after addon installation. The wizard now shows a "Configure Persistence Services" screen where users can set the default persistence service, choose which items to persist (all, group, or none), and select persistence strategies for each installed service.
Changes:
- Added a new persistence configuration tab to the setup wizard workflow between addon installation and the final welcome screen
- Created a new reusable component for persistence service configuration in the wizard
- Updated i18n files with new translation strings for the persistence configuration step
- Added a convenience link to re-run the setup wizard from the About page
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| setup-wizard.vue | Added persistence-config tab, new state variables (addonsReady, persistenceConfigConfirm), and flow methods to show/skip persistence configuration |
| persistence-config-setup-wizard.vue | New component that handles loading persistence services, displaying configuration options, and saving configurations via REST API |
| setup-wizard/en.json | Added translation strings for persistence configuration UI elements, strategies, and help text |
| about/en.json | Added "Setup Wizard" button translation |
| about.vue | Added button to navigate back to setup wizard |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| progressDialog.close() | ||
| progressDialog.destroy() | ||
| this.showFinish() | ||
| this.addonsInstalled = true |
There was a problem hiding this comment.
The variable addonsInstalled is being set to true but is not declared in the component's data object. This will cause the property to not be reactive and could lead to unexpected behavior. Add addonsInstalled: false to the data() method.
There was a problem hiding this comment.
Not required anymore.
| "setupwizard.persistence-config.services.items.none": "None", | ||
| "setupwizard.persistence-config.services.advancedConfig": "The existing persistence configuration has configuration settings that cannot be reconfigured in the wizard. Use Settings -> Persistence for configuration or reset the configuration.", | ||
| "setupwizard.persistence-config.services.resetConfig": "Reset Configuration", | ||
| "setupwizard.persistence-config.services.resetConfig.confirm": "Are you sure you want to reset the {config} configuration ?", |
There was a problem hiding this comment.
There should be no space before the question mark. Change 'configuration ?' to 'configuration?'
| "setupwizard.persistence-config.services.resetConfig.confirm": "Are you sure you want to reset the {config} configuration ?", | |
| "setupwizard.persistence-config.services.resetConfig.confirm": "Are you sure you want to reset the {config} configuration?", |
| try { | ||
| const suggestions = await this.$oh.api.get('/rest/persistence/strategysuggestions?serviceId=' + service.id) | ||
| this.suggestedStrategies[service.id] = suggestions.map((suggestion) => suggestion.name) | ||
| } catch { | ||
| // Ignore if suggestions not found | ||
| } |
There was a problem hiding this comment.
The catch blocks silently ignore errors without any user feedback or logging. For better debugging and user experience, consider logging the error or providing user feedback when loading persistence configuration fails.
There was a problem hiding this comment.
The console would show the call and its success failure anyway, so no extra logging needed. From a user perspective, this should not block as it should work without suggestions being shown.
| // we don't show any configuration option in the wizard to avoid overwriting the existing configuration. | ||
| if (!this.isEditable(configuration)) return false | ||
| const configs = configuration.configs | ||
| if (!configs?.length === 1) return false |
There was a problem hiding this comment.
The condition !configs?.length === 1 will always evaluate to false due to operator precedence. The negation operator (!) has higher precedence than the equality operator (===). This should be configs?.length !== 1 to check if the length is not equal to 1.
| if (!configs?.length === 1) return false | |
| if (configs?.length !== 1) return false |
| return strategies | ||
| }, | ||
| resetConfiguration (serviceId) { | ||
| const label = this.services[this.services.findIndex((service) => service.id === serviceId)] || serviceId |
There was a problem hiding this comment.
The variable assignment retrieves a service object using findIndex, but findIndex returns the index (number), not the service object. This should use find() instead of accessing the array by index: this.services.find((service) => service.id === serviceId)?.label
| const label = this.services[this.services.findIndex((service) => service.id === serviceId)] || serviceId | |
| const label = this.services.find((service) => service.id === serviceId)?.label || serviceId |
| updatePersistenceConfig () { | ||
| this.$oh.api.put('/rest/services/' + this.persistenceServiceId + '/config', { | ||
| default: this.defaultPersistence | ||
| }) | ||
| const groupsToAdd = [] | ||
| this.services.map((service) => service.id) | ||
| .filter((serviceId) => this.isBasicConfig(this.configuration[serviceId])) | ||
| .forEach((serviceId) => { | ||
| this.configuration[serviceId] = this.updateOrCreateConfiguration(serviceId) | ||
| // Define all common cron strategies in the persistence configuration | ||
| const cronStrategies = this.configuration[serviceId].cronStrategies || [] | ||
| const cronStrategyNames = cronStrategies.map((strategy) => strategy.name) | ||
| this.CommonCronStrategies.forEach((strategy) => { | ||
| if (!cronStrategyNames.includes(strategy.name)) { | ||
| cronStrategies.push(strategy) | ||
| } | ||
| }) | ||
| this.configuration[serviceId].cronStrategies = cronStrategies | ||
| this.$oh.api.put('/rest/persistence/' + serviceId, this.configuration[serviceId]) | ||
| if (this.items[serviceId].length > 1) { | ||
| const groupName = this.items[serviceId].slice(0, -1) | ||
| if (!this.groupExists[groupName]) { | ||
| groupsToAdd.push({ | ||
| type: 'Group', | ||
| name: groupName, | ||
| label: this.groupLabels[groupName] | ||
| }) | ||
| } | ||
| } | ||
| }) | ||
| if (groupsToAdd.length) { | ||
| this.$oh.api.put('/rest/items', groupsToAdd) | ||
| } | ||
| } |
There was a problem hiding this comment.
The API calls in updatePersistenceConfig are not awaited and lack error handling. If any of these PUT requests fail, the user won't be notified and the persistence configuration may be in an inconsistent state. Consider using async/await with try-catch blocks to handle potential failures.
There was a problem hiding this comment.
I am not sure there is much to be gained to add this. I don't think this can create an inconsistent state.
| const items = configs[0].items || [''] | ||
| this.items[service.id] = items[0] | ||
| if (items[0].match(/[^!].+\*/)) { | ||
| this.itemPersistGroups[service.id] = items[0].slice(0, -1) |
There was a problem hiding this comment.
The regex pattern may not safely match the intended group pattern. If items[0] is undefined or an empty string, calling match() could throw an error. Consider adding a null check before calling match().
| const items = configs[0].items || [''] | |
| this.items[service.id] = items[0] | |
| if (items[0].match(/[^!].+\*/)) { | |
| this.itemPersistGroups[service.id] = items[0].slice(0, -1) | |
| const rawItems = configs[0].items | |
| const firstItem = Array.isArray(rawItems) && rawItems.length > 0 ? rawItems[0] : '' | |
| this.items[service.id] = firstItem | |
| if (typeof firstItem === 'string' && firstItem.match(/[^!].+\*/)) { | |
| this.itemPersistGroups[service.id] = firstItem.slice(0, -1) |
There was a problem hiding this comment.
items[0] cannot be undefined. I don't see why it would throw if items[0] is an empty string with the given regex. Empty string will not match.
| resetConfiguration (serviceId) { | ||
| const label = this.services[this.services.findIndex((service) => service.id === serviceId)] || serviceId | ||
| f7.dialog.confirm(this.t('setupwizard.persistence-config.services.resetConfig.confirm', label), () => { | ||
| this.updateOrCreateConfiguration(serviceId) |
There was a problem hiding this comment.
The resetConfiguration method does not actually update the configuration. It calls updateOrCreateConfiguration but doesn't assign the returned value back to this.configuration[serviceId]. The configuration should be stored after updating it.
| this.updateOrCreateConfiguration(serviceId) | |
| this.configuration[serviceId] = this.updateOrCreateConfiguration(serviceId) |
| strategies.splice(strategies.indexOf('restoreOnStartup'), 1) | ||
| } | ||
| if (service.type !== 'Modifiable') { | ||
| strategies.splice(strategies.indexOf('forecast'), 1) |
There was a problem hiding this comment.
The splice operation may fail if the strategy doesn't exist in the array. indexOf returns -1 if the element is not found, and splicing at -1 removes the last element. Consider checking if the index is >= 0 before calling splice.
| strategies.splice(strategies.indexOf('restoreOnStartup'), 1) | |
| } | |
| if (service.type !== 'Modifiable') { | |
| strategies.splice(strategies.indexOf('forecast'), 1) | |
| const restoreIndex = strategies.indexOf('restoreOnStartup') | |
| if (restoreIndex >= 0) { | |
| strategies.splice(restoreIndex, 1) | |
| } | |
| } | |
| if (service.type !== 'Modifiable') { | |
| const forecastIndex = strategies.indexOf('forecast') | |
| if (forecastIndex >= 0) { | |
| strategies.splice(forecastIndex, 1) | |
| } |
| // If there is an existing configuration, we will only present it for configuration in the wizard if it has a basic configuration: | ||
| // - Editable (not defined in a file) | ||
| // - A single configuration | ||
| // - No, or a single item defition (all, or group) |
There was a problem hiding this comment.
Corrected spelling from 'defition' to 'definition'.
| // - No, or a single item defition (all, or group) | |
| // - No, or a single item definition (all, or group) |
Signed-off-by: Mark Herwege <[email protected]>
| "setupwizard.persistence-config.default.title": "Default Service", | ||
| "setupwizard.persistence-config.default.header": "The default persistence service will be used when retrieving persisted Item states and no service is explicitly specified.", | ||
| "setupwizard.persistence-config.services.title": "Services", | ||
| "setupwizard.persistence-config.services.header1": "Each persistence service needs to be configured with a Strategies and Items definition.", |
There was a problem hiding this comment.
Recommend re-ordering (Items and Strategies) to match the order of explanation below.
| "setupwizard.persistence-config.default.header": "The default persistence service will be used when retrieving persisted Item states and no service is explicitly specified.", | ||
| "setupwizard.persistence-config.services.title": "Services", | ||
| "setupwizard.persistence-config.services.header1": "Each persistence service needs to be configured with a Strategies and Items definition.", | ||
| "setupwizard.persistence-config.services.header2": "The Items definition defines what Items will be persisted. You can persist all Items or a group of Items.", |
There was a problem hiding this comment.
Just wondering if there is a way to simplify given this will be one of the initial screens users will see when setting up and concerned they could be intimated by the length? Thoughts?
Choose which Items to persist: all Items, a group of Items, or none. If you select a group, a Group Item will be created and you'll need to assign Items to it. You can refine persistence settings later in Persistence Settings.
There was a problem hiding this comment.
Thanks for that. Indeed, shorter is better. There was some feedback that the messages where not sufficient to understand, so it lead me to put a bit more, but it is about striken the right balance.
That's another topic, but I think there is more work that can be done in this wizard to resquence certain steps and insert some pages explaining some basic concepts for a first time user. I will create a separate issue (and potentially separate PR's) for that.
| "setupwizard.persistence-config.services.header5": "Strategies define when Item states will be persisted.", | ||
| "setupwizard.persistence-config.services.header6": "Common strategies are: Every Update (persist on Item state update), Every Change (persist on Item state change) and Every Minute (persist Item state every minute).", | ||
| "setupwizard.persistence-config.services.header7": "The restoreOnStartup strategy is used to restore Item states from persistence when starting openHAB.", | ||
| "setupwizard.persistence-config.services.header8": "A forecast strategy is used to persist future values (e.g. weather forecasts or future prices).", | ||
| "setupwizard.persistence-config.services.header9": "You can accept the provided defaults or adjust according to your requirements.", | ||
| "setupwizard.persistence-config.services.items.all": "All", |
There was a problem hiding this comment.
same here (I just plugged into AI to simplify, so might be missing something)
Strategies define when Item states are persisted. Common options include persisting on every update, every change, or at regular intervals. The restoreOnStartup strategy restores Item states when openHAB starts, while forecast strategies persist future values (i.e. weather forecasts or future prices). Accept the defaults or adjust as needed.
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
3 participants
This adds an extra step at the end of the setup wizard, after installing the addons, to do a simple configuration of the installed persistence addons.
This is still WIP.
Here is an early look at it:
and (required scrolling):