[persistence-edit]: Adds enhanced UI support for configuring persistence#3986
[persistence-edit]: Adds enhanced UI support for configuring persistence#3986jsjames wants to merge 2 commits intoopenhab:mainfrom
Conversation
#5206 Bundle Size — 13.25MiB (+0.11%).973f94a(current) vs 07d4a86 main#5205(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch jsjames:persistence-config Project dashboard Generated by RelativeCI Documentation Report issue |
|
@mherwege - could you try out this PR and check that the persistence editing is as you intended. I'm pretty sure it is based on your initial PR, but would appreciate your help!! BTW, still working on the core edits we discussed ... @florian-h05 - I kept this still in draft as I'd like your feedback on the useDirty and useTabs composables. These need to get "right" as we want to use as the standard across the UI. |
a93967c to
999e7e8
Compare
2a4f962 to
4ef2de5
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the MainUI persistence configuration experience by refactoring the persistence settings/edit pages to TypeScript/composables, adding definition-management popups, and improving navigation safety (dirty tracking + guarded tab switching).
Changes:
- Replaces the legacy dirty-mixin workflow with a new
useDirtycomposable and a global route leave check. - Refactors persistence settings/edit UI to TypeScript, adds popups for managing cron strategy/filter/config definitions, and standardizes v-model/local-copy update flows.
- Adds a
useTabscomposable (with guarded code/design switching) and updates the cron expression editor + generated API types.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.ui/web/src/types/globals.d.ts | Adds TS module augmentation for common injected globals ($device, $t, $fullscreen). |
| bundles/org.openhab.ui/web/src/pages/settings/useDirty.ts | New composable for dirty tracking + leave confirmation; attaches __dirty ref to page element. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/strategy-picker.vue | Replaces SmartSelect with custom popup multi-select and cron strategy creation flow. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/persistence-settings.vue | Refactors persistence settings page to <script setup lang="ts">, adds dirty indicator and Ctrl/⌘+S handling. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/persistence-edit.vue | Major TS refactor; adds code/design tab guard, new configuration/definitions popups, local-copy update pattern. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/filter-popup.vue | Converts filter editor to popup with type selection UI and local-copy editing. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/filter-picker.vue | Replaces SmartSelect with popup-based multi-select and inline “add filter definition” flow. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/definitions-popup.vue | New “Manage Definitions” popup for cron strategies + filters (add/edit/delete). |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/cron-strategy-popup.vue | Refactors cron strategy editor popup to support add/edit via v-model + local copy. |
| bundles/org.openhab.ui/web/src/pages/settings/persistence/configuration-popup.vue | Refactors configuration editor popup to use v-model and propagate definition changes upward. |
| bundles/org.openhab.ui/web/src/pages/settings/items/item-details.vue | Adds explicit back-link URL for consistency/navigation. |
| bundles/org.openhab.ui/web/src/pages/settings/dirty-mixin.js | Minor safety tweak for classList access. |
| bundles/org.openhab.ui/web/src/js/routes.js | Updates global route guard to use useDirty’s __dirty ref (with legacy fallback). |
| bundles/org.openhab.ui/web/src/js/dialog-promises.ts | New promise-based wrappers for confirm/toast/alert dialogs. |
| bundles/org.openhab.ui/web/src/css/app.styl | Adds toast styling and tab highlight styling helpers. |
| bundles/org.openhab.ui/web/src/composables/useTabs.ts | New composable to manage tab switching and optionally block switching when code parsing fails. |
| bundles/org.openhab.ui/web/src/components/persistence/item-persistence-details.vue | Updates SDK call param from itemname to itemName. |
| bundles/org.openhab.ui/web/src/components/config/controls/parameter-cronexpression.vue | Adds update:value for v-model support and simplifies async import. |
| bundles/org.openhab.ui/web/src/components/config/controls/cronexpression-editor.vue | Updates cron editor tabs + restoration logic; makes year optional in generated cron string. |
| bundles/org.openhab.ui/web/src/assets/definitions/persistence.ts | Migrates persistence definitions to TS, adds types/enums, and introduces persistenceKey injection key. |
| bundles/org.openhab.ui/web/src/api/types.gen.ts | Updates generated API types (persistence strategy shape, query param casing, service config types, etc.). |
| bundles/org.openhab.ui/web/src/api/sdk.gen.ts | Updates generated SDK (query param casing, response typing, content-type change, etc.). |
| bundles/org.openhab.ui/web/src/api/index.ts | Regenerated exports/types alignment with updated SDK/types. |
| bundles/org.openhab.ui/web/.vscode/settings.json | Editor formatter overrides per language. |
Comments suppressed due to low confidence (1)
bundles/org.openhab.ui/web/src/assets/definitions/persistence.ts:11
PredefinedStrategieswas changed from astring[]toPersistenceStrategy[]objects. There are still consumers that treat it as a list of strings (e.g., spreading intonew Set([...PredefinedStrategies])), which will now produce mixed types / break lookups. Either keepPredefinedStrategiesasstring[](and map to{ name }only where needed), or update all call sites to usePredefinedStrategies.map(s => s.name).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| } else if (weekExpr?.length) { | ||
| this.day.cronEvery = 4 | ||
| this.week.specificSpecific = weekExpr.split(',').map((v) => v.trim()) | ||
| } else { | ||
| this.day.cronEvery = 5 | ||
| this.day.specificSpecific = dayExpr.split(',').map((v) => parseInt(v)) |
| <script setup lang="ts"> | ||
| import { computed, ref, watch, useAttrs } from 'vue' | ||
|
|
||
| import CronStrategyPopup from '@/pages/settings/persistence/cron-strategy-popup.vue' | ||
| import { PredefinedStrategies } from '@/assets/definitions/persistence' | ||
|
|
||
| import * as api from '@/api' | ||
|
|
||
| // Props and emits | ||
| const cronStrategies = defineModel<api.PersistenceCronStrategy[]>('cron-strategies', { required: true }) | ||
| const selected = defineModel<string[]>('selected-strategies', { required: true }) | ||
| const props = defineProps<{ title?: string }>() |
| router.allowPageChange = true | ||
| resolve() | ||
| } else { | ||
| router.allowPageChange = false |
|
|
||
| function setFilterType (filterTypeName: FilterTypeName) { | ||
| localFilter.value.filterTypeName = filterTypeName | ||
| localFilter.value.filter = { name: filterTypeName } |
| // Check if strategy is used in configurations | ||
| if (_isStrategyUsed(sName)) { | ||
| showConfirmDialog('Strategy used in configuration(s), delete anyway?', "Delete Persistence Strategy").then((confirmed) => { | ||
| if (!confirmed) return | ||
|
|
||
| _deleteModule(ev, type, index) | ||
| _removeStrategyFromConfiguration(sName) | ||
| }) | ||
| } else { | ||
| _deleteModule(ev, type, index) | ||
| } | ||
| } | ||
|
|
||
| function _removeStrategyFromConfiguration (strategy: string) { | ||
| localPersistence.value.configs.forEach((cfg) => { | ||
| const sIndex = cfg.strategies?.findIndex((s) => s === strategy) ?? -1 | ||
| if (sIndex >= 0 && cfg.strategies) cfg.strategies.splice(sIndex, 1) | ||
| }) | ||
| } | ||
|
|
||
| function _isStrategyUsed (csName: string | undefined) { | ||
| return localPersistence.value.configs?.findIndex((cfg) => cfg.strategies?.findIndex((cs) => cs === csName) >= 0) >= 0 |
| const localFiltersDefinitions = ref<FiltersDefinition>({} as FiltersDefinition) | ||
| const localSelected = ref<string[]>([]) | ||
|
|
||
| const definitionsDirty = ref<boolean>(false) | ||
|
|
||
| // Computed | ||
| const allFilters = computed(() => { | ||
| return Object.values(localFiltersDefinitions.value).flat() | ||
| }) |
| .hoursText || '*'} ${this.daysText || '*'} ${this.monthsText || | ||
| '*'} ${this.weeksText || '?'} ${this.yearsText || '*'}` | ||
| .hoursText || '*'} ${this.daysText || '?'} ${this.monthsText || | ||
| '*'} ${this.weeksText || '?'} ${this.yearsText.length ? ' ' : ''}${this.yearsText || ''}` |
|
@florian-h05 - i'm going to break this out into several smaller PRs so we can better trace any regressions. |
|
@jsjames I did a very quick smoke test, trying to add a cron strategy, both through the configurations and definitions path, checking when the configuration goes dirty. At first this looks good and does what I expected. I did switch to the code tab and that generated following error: This is probably unrelated, but still needs to be looked at. |
Did you run npm install after checking it out? That was a recently added module to codemirror in another pr. |
No, I didn’t. So that’s probably it. |
4ef2de5 to
8a9cc80
Compare
|
@florian-h05 - I broke out 2 other PRs that this is dependent on - once those are merged, this is ready as well. |
053e3a4 to
d229e2c
Compare
|
@florian-h05 - this one is ready to go with the merge of the cronexpression refactor. There are several refactors/typescript changes that appear first in this PR, but not yet globally applied (useDirty, useTabs, dialogPromises (greatly simplified toasts/dialogs)). I didn't break these out as separate PRs since they are part of the refactor to typescript - let me know if you think we should try to break out anyways. |
Signed-off-by: Jeff James <jeff@james-online.com> (+5 squashed commits) Squashed commits: [10a262d] WIP Signed-off-by: Jeff James <jeff@james-online.com> [809cb47] WIP (+1 squashed commit) Squashed commits: [0ec8ddd] WIP (+3 squashed commits) Squashed commits: [0b1a9f3c] WIP [7d3dd91b] WIP [783e4ba5] remove skipLoadOnReturn Signed-off-by: Mark Herwege <mark.herwege@telenet.be> parallel load of suggestions Signed-off-by: Mark Herwege <mark.herwege@telenet.be> darkMode Signed-off-by: Mark Herwege <mark.herwege@telenet.be> dark mode Signed-off-by: Mark Herwege <mark.herwege@telenet.be> fix 404 Signed-off-by: Mark Herwege <mark.herwege@telenet.be> make load async Signed-off-by: Mark Herwege <mark.herwege@telenet.be> move PredefinedStrategies to configuration popup Signed-off-by: Mark Herwege <mark.herwege@telenet.be> remove unused filters field from persistence Signed-off-by: Mark Herwege <mark.herwege@telenet.be> cron expression smart select Signed-off-by: Mark Herwege <mark.herwege@telenet.be> make loading local variable Signed-off-by: Mark Herwege <mark.herwege@telenet.be> adjust theme color logic Signed-off-by: Mark Herwege <mark.herwege@telenet.be> Simplified cronexpression-editor Signed-off-by: Jeff James <jeff@james-online.com> avoid error when deleting persistence Signed-off-by: Mark Herwege <mark.herwege@telenet.be> avoid error when backing out of persistence with no configs Signed-off-by: Mark Herwege <mark.herwege@telenet.be> WIP WIP WIP WIP WIP WIP (+3 squashed commits) Squashed commits: [5af2284] avoid definitions nav-right error when not fully initialized Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [8f3fbb2] no more cron expression open method Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [95fbd52] fix documentation formatting Signed-off-by: Mark Herwege <mark.herwege@telenet.be> (+3 squashed commits) Squashed commits: [d515457] architecture documentation Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [aacc047] format Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [97d5be1] fix cronexpression (+1 squashed commit) Squashed commits: [b136ef8] copilot review adjustments Signed-off-by: Mark Herwege <mark.herwege@telenet.be> (+1 squashed commit) Squashed commits: [5601831] format Signed-off-by: Mark Herwege <mark.herwege@telenet.be> (+1 squashed commit) Squashed commits: [5ac8d34] fix cron smart select restore Signed-off-by: Mark Herwege <mark.herwege@telenet.be> (+19 squashed commits) Squashed commits: [0e7006d] fixes Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [ec3e529] fix Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [e4fd80a] alias fixes Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [2039dad] format Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [186c516] field subtitle change Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [417c961] fixes Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [de169aa] use hey api Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [237d8ba] format Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [5006859] refactor to simplify persistence config for user Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [c1ae402] cron expression editor fixes Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [d63d90e] delete confirm dialog Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [db2ed60] move definitions to separate page Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [7405b3a] graphic filter selection Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [01b8c4d] screen space optimization Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [639bc44] improved configuration visualization Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [0cfc446] format Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [aac4ad4] single add filter link Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [ce3b85d] fix Signed-off-by: Mark Herwege <mark.herwege@telenet.be> [680c04c] persistence configuration Signed-off-by: Mark Herwege <mark.herwege@telenet.be> Signed-off-by: Jeff James <jeff@james-online.com> [af41b46] ready for PR Signed-off-by: Jeff James <jeff@james-online.com> [b4ea740] WIP Signed-off-by: Jeff James <jeff@james-online.com> [b3a26a9] Update per openhab-core PR#5404 Signed-off-by: Jeff James <jeff@james-online.com>
|
looking at these errors: not sure why these are showing up with this PR? |
Yes, please do so and break them out individually. |
Probably try ith with rebasing, |
Signed-off-by: Jeff James <jeff@james-online.com>
d229e2c to
973f94a
Compare
|
I had to define this in globals.d.ts: I don't quite understand why this is required for this PR to pass linting for clock.ts (even though this PR does not modify clock.ts) but not currently required in main.ts? Any ideas? |
|
Other version of globals dependency or other type declaration dependencies? |
Dependent on #3992 and #3991
Based on work by @mherwege in PR #3766
Further enhances by: