fix: hide apply buttons for nested composite features #404
Conversation
- Updated isFeatureRoot to detect nested composites (composites with composite parents) - Fixed parentFeatures chain to include current feature when rendering sub-features - Fixed state preservation when nested composites call onChange to prevent clearing other fields
| setState((prev) => ({ ...prev, ...value })); | ||
| setState((prev) => { | ||
| const newState = { ...prev, ...value }; | ||
|
|
||
| if (!isRoot) { | ||
| if (type === "composite") { | ||
| onChange(property ? { [property]: { ...state, ...value } } : value); | ||
| } else { | ||
| onChange(value); | ||
| if (!isRoot) { | ||
| if (type === "composite") { | ||
| // For nested composites, deviceState is already scoped to this property | ||
| // Merge with existing values from both deviceState and previous local state to preserve all values | ||
| const existingFromDevice = | ||
| deviceState && typeof deviceState === "object" && !Array.isArray(deviceState) | ||
| ? (deviceState as Record<string, unknown>) | ||
| : {}; | ||
| const existingFromState = prev; | ||
| const allExisting = { ...existingFromDevice, ...existingFromState }; | ||
| const mergedState = { ...allExisting, ...value }; | ||
| onChange(property ? { [property]: mergedState } : mergedState); | ||
| } else { | ||
| onChange(value); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return newState; | ||
| }); | ||
| }, | ||
| [state, type, property, isRoot, onChange], | ||
| [type, property, isRoot, onChange, deviceState], |
There was a problem hiding this comment.
It was needed alongside the onChange changes to ensure that the correct values were available in the component. If it wasn't there, the component didn't start out populated if I remember it correctly.
There was a problem hiding this comment.
The whole change I meant (L65-93).
There was a problem hiding this comment.
Without those changes as soon as you update any value in the nested composite, all of the other fields lose their value which you don't want.
There was a problem hiding this comment.
Is this happening with a real network, or the dev mocks? We should not be sending whole states to Z2M, which is what this would do if I glanced at it correctly.
There was a problem hiding this comment.
Real network. I tested it by connecting it to my existing Z2M installation that is running Koenkk/zigbee-herdsman-converters#9899 branch.
Again, I'm not a frontend expert so this is me googling and looking at suggestions + AI advice. Feel free to adjust if you have a better way to do it.
There was a problem hiding this comment.
Can you download the state (Settings>Tools) and grab the stuff for the Inovelli device with that converter?
Would need the appropriate sections in bridgeDefinitions (custom_clusters I assume it has some), devices, deviceStates.
I'll add it to the mocks and run some tests.
There was a problem hiding this comment.
Hopefully this has everything relevant in it:
simplified_states.json
There was a problem hiding this comment.
Can you remove the changes of this whole block from the PR?
I wrote some tests and added the behavior in #406 that way we can better see if anything breaks with the other two changes from this PR. We'll merge it first, then rebase this one.
There was a problem hiding this comment.
Yep, reverted in latest commit.
| feature={feature} | ||
| parentFeatures={parentFeatures ?? []} | ||
| feature={subFeature} | ||
| parentFeatures={[...(parentFeatures ?? []), feature]} |
There was a problem hiding this comment.
This changes the FeatureSubFeatures>Features to always have at least 1 parent.
I'm not sure of the impact.
I think we'll need to add a couple mock devices to get a better idea of before/after.
2 participants
Fixes an issue where nested composite features (composites within composites) were showing redundant inner Apply buttons that were non-functional.
Reference: Koenkk/zigbee-herdsman-converters#9899 (comment)
Changes
Testing