-
-
Notifications
You must be signed in to change notification settings - Fork 276
[MainUI] Item persistence status in item detail #3765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
#4480 Bundle Size — 12.78MiB (+0.02%).668282b(current) vs 5f64317 main#4472(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch mherwege:item_persistence_status Project dashboard Generated by RelativeCI Documentation Report issue |
87b6002 to
12d9579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a persistence status section to the item detail screen, showing which persistence services are configured for an item, the strategies being used, and whether the item is actually persisted with a count of stored values.
Changes:
- Added a new Persistence section to the item details page showing persistence service configurations
- Created a new component to display persistence service details including configuration status, strategies, and persisted item counts
- Integrated the component with links to persistence service configurations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| bundles/org.openhab.ui/web/src/pages/settings/items/item-details.vue | Adds the persistence section to the item details page with conditional rendering for non-Group items or Groups with a groupType |
| bundles/org.openhab.ui/web/src/components/persistence/item-persistence-details.vue | New component that fetches and displays persistence service information, including matching strategies and persisted status with badges |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return { | ||
| ...service, | ||
| configs: serviceConfig.configs || [], |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an extra space before the 'or' operator in this line. While this doesn't affect functionality, it's inconsistent with common JavaScript formatting conventions.
| configs: serviceConfig.configs || [], | |
| configs: serviceConfig.configs || [], |
| async load () { | ||
| this.loaded = false | ||
| const services = await this.$oh.api.get('/rest/persistence') | ||
| this.ready = false |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'ready' is being set but is never declared in the data() function. This will create an undeclared reactive property and could lead to unexpected behavior. The 'ready' property should either be added to the data() function or this line should be removed if it's not needed.
| this.ready = false |
| this.services = await Promise.all( | ||
| services.map((service) => this.loadService(service)) | ||
| ) | ||
| this.currentState = this.item.state |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'currentState' property is set but never used anywhere in the component. Consider removing this unused property to keep the code clean and maintainable.
| configs: serviceConfig.configs || [], | ||
| aliases: serviceConfig.aliases, | ||
| editable: serviceConfig.editable === undefined ? true : serviceConfig.editable, | ||
| persisted: itemsPersisted.findIndex((item) => item.name === this.item.name) >= 0 |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'persisted' property is incorrectly set to a boolean value based on whether the item is found in the array. However, the 'persistedBadges' computed property expects 'persisted' to be an object with a 'count' property (line 53). This mismatch will cause the badge to always show 'values' instead of the actual count when an item is persisted. The API response from '/rest/persistence/items' should be stored with its full structure, not just a boolean indicating presence.
| persisted: itemsPersisted.findIndex((item) => item.name === this.item.name) >= 0 | |
| persisted: itemsPersisted.find((item) => item.name === this.item.name) || null |
| configuredServices () { | ||
| return this.services.filter((service) => this.strategies[service.id]?.length) | ||
| }, |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The computed property 'configuredServices' is defined but never used in the component. Consider removing this unused computed property to reduce code complexity and improve maintainability.
| configuredServices () { | |
| return this.services.filter((service) => this.strategies[service.id]?.length) | |
| }, |
| serviceStrategies (serviceId) { | ||
| let strategies = [] | ||
| const persistenceConfig = this.services.find((persistence) => persistence.id === serviceId) | ||
| persistenceConfig?.configs?.forEach((config) => { | ||
| const items = config.items | ||
| if (items && config.strategies?.length) { | ||
| let match = false | ||
| // First find all positive matches for the item | ||
| if (items.includes('*') || items.includes(this.item.name) || items.filter((configItem) => !configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(0, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { | ||
| match = true | ||
| } | ||
| // Remove negative matches | ||
| if (items.includes('!' + this.item.name) || items.filter((configItem) => configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(1, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This complex matching logic is difficult to read and maintain due to multiple chained method calls on a single line. The logic for matching group names with wildcards should be extracted into a separate helper method to improve readability and testability. Additionally, the same pattern is repeated on line 116 for negative matches, which creates code duplication.
| serviceStrategies (serviceId) { | |
| let strategies = [] | |
| const persistenceConfig = this.services.find((persistence) => persistence.id === serviceId) | |
| persistenceConfig?.configs?.forEach((config) => { | |
| const items = config.items | |
| if (items && config.strategies?.length) { | |
| let match = false | |
| // First find all positive matches for the item | |
| if (items.includes('*') || items.includes(this.item.name) || items.filter((configItem) => !configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(0, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { | |
| match = true | |
| } | |
| // Remove negative matches | |
| if (items.includes('!' + this.item.name) || items.filter((configItem) => configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(1, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { | |
| matchesPositiveItems (items, itemName, groupNames) { | |
| if (!items || !Array.isArray(items)) { | |
| return false | |
| } | |
| if (items.includes('*') || items.includes(itemName)) { | |
| return true | |
| } | |
| const positiveGroupPatterns = items.filter((configItem) => !configItem.startsWith('!') && configItem.endsWith('*')) | |
| return positiveGroupPatterns | |
| .map((configItem) => configItem.slice(0, -1)) | |
| .some((groupName) => groupNames.includes(groupName)) | |
| }, | |
| matchesNegativeItems (items, itemName, groupNames) { | |
| if (!items || !Array.isArray(items)) { | |
| return false | |
| } | |
| if (items.includes('!' + itemName)) { | |
| return true | |
| } | |
| const negativeGroupPatterns = items.filter((configItem) => configItem.startsWith('!') && configItem.endsWith('*')) | |
| return negativeGroupPatterns | |
| .map((configItem) => configItem.slice(1, -1)) | |
| .some((groupName) => groupNames.includes(groupName)) | |
| }, | |
| serviceStrategies (serviceId) { | |
| let strategies = [] | |
| const persistenceConfig = this.services.find((persistence) => persistence.id === serviceId) | |
| const itemName = this.item.name | |
| const groupNames = this.item.groupNames | |
| persistenceConfig?.configs?.forEach((config) => { | |
| const items = config.items | |
| if (items && config.strategies?.length) { | |
| let match = false | |
| // First find all positive matches for the item | |
| if (this.matchesPositiveItems(items, itemName, groupNames)) { | |
| match = true | |
| } | |
| // Remove negative matches | |
| if (this.matchesNegativeItems(items, itemName, groupNames)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there still is a lot of code duplication in your proposal and the names and arguments of the match methods do not clearly convey the meaning. I have created an alternative.
| if (items.includes('*') || items.includes(this.item.name) || items.filter((configItem) => !configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(0, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { | ||
| match = true | ||
| } | ||
| // Remove negative matches | ||
| if (items.includes('!' + this.item.name) || items.filter((configItem) => configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(1, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { |
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code assumes 'this.item.groupNames' exists and is an array, but it may be undefined for items that don't belong to any groups. This will cause a runtime error when calling '.includes()' on undefined. The code should use optional chaining or a nullish coalescing operator to handle this case (e.g., 'this.item.groupNames?.includes(groupName)' or 'this.item.groupNames || []').
| if (items.includes('*') || items.includes(this.item.name) || items.filter((configItem) => !configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(0, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { | |
| match = true | |
| } | |
| // Remove negative matches | |
| if (items.includes('!' + this.item.name) || items.filter((configItem) => configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(1, -1)).find((groupName) => this.item.groupNames.includes(groupName))) { | |
| if (items.includes('*') || items.includes(this.item.name) || items.filter((configItem) => !configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(0, -1)).find((groupName) => this.item.groupNames?.includes(groupName))) { | |
| match = true | |
| } | |
| // Remove negative matches | |
| if (items.includes('!' + this.item.name) || items.filter((configItem) => configItem.startsWith('!') && configItem.endsWith('*')).map((configItem) => configItem.slice(1, -1)).find((groupName) => this.item.groupNames?.includes(groupName))) { |
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
5eb197f to
bea9622
Compare
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
e96e4b7 to
668282b
Compare
This PR adds a section on persistence in item detail screen. It will:
Here is a screenshot:
