Cluster Feature Page - Enable feature toggling and sync with device type feature page #1604

ethanzhouyc · 2025-06-05T15:52:47Z

gemini-code-assist

gemini-code-assist

gemini-code-assist · 2025-06-05T15:55:33Z

src-electron/validation/conformance-checker.js

+    Object.keys(featuresToUpdate).length == 0
  ) {


The condition Object.keys(featuresToUpdate).length == 0 has been added to determine if a feature change should be enabled. This is a good check to prevent changes if they would require other features to be updated first. Could you confirm if there are scenarios where featuresToUpdate might be populated but the change should still be allowed, perhaps with a different type of warning, or is disabling the change always the correct action in this case?

gemini-code-assist · 2025-06-05T15:55:33Z

src-electron/validation/conformance-expression-evaluator.js

+/**
+ * Check which features need to be updated or have conformance changes due to the updated feature.
+ *
+ * @param {*} updatedFeatureCode
+ * @param {*} clusterFeatures
+ * @param {*} elementMap
+ * @returns {Object} Contains:
+ *   - updatedFeatures: Map of features with incorrect conformance that need to be updated.
+ *   - changedConformFeatures: Array of feature objects with conformance value changed after the feature update.
+ */
+function checkFeaturesToUpdate(
+  updatedFeatureCode,
+  clusterFeatures,
+  elementMap
+) {
+  // build a map of feature codes to their conformance expressions
+  let featureConformance = clusterFeatures.reduce((map, feature) => {
+    map[feature.code] = feature.conformance
+    return map
+  }, {})
+
+  let updatedFeatures = {}
+  let changedConformFeatures = []
+  for (let [featureCode, expression] of Object.entries(featureConformance)) {
+    if (featureCode == updatedFeatureCode) {
+      continue
+    }
+
+    // skip if conformance is not related to the updated feature
+    let terms = getTermsFromExpression(expression)
+    if (!terms.includes(updatedFeatureCode)) {
+      continue
+    }
+
+    let conformance = evaluateConformanceExpression(expression, elementMap)
+
+    if (conformance == 'mandatory' && !elementMap[featureCode]) {
+      updatedFeatures[featureCode] = true
+    }
+    if (conformance == 'notSupported' && elementMap[featureCode]) {
+      updatedFeatures[featureCode] = false
+    }
+
+    let oldElementMap = { ...elementMap }
+    oldElementMap[updatedFeatureCode] = !oldElementMap[updatedFeatureCode]
+    let oldConformance = evaluateConformanceExpression(
+      expression,
+      oldElementMap
+    )
+
+    // compare conformance before and after the feature update
+    if (conformance !== oldConformance) {
+      changedConformFeatures.push(featureCode)
+    }
+  }
+
+  changedConformFeatures = changedConformFeatures.map((code) => {
+    return clusterFeatures.find((feature) => feature.code == code)
+  })
+
+  return { updatedFeatures, changedConformFeatures }
+}


The new checkFeaturesToUpdate function introduces logic to identify features whose conformance state or value might change due to a feature toggle. This is a key part of the improved warning system.

Given its complexity and importance:

Are there specific edge cases around complex conformance expressions (e.g., nested conditions, multiple dependencies) that were particularly challenging to test for this function?

The function identifies updatedFeatures (features that must change state) and changedConformFeatures (features whose conformance value changes). Is the distinction clear and consistently used in the warning generation logic in conformance-checker.js? It appears to be, with updatedFeatures leading to disabling the primary change and changedConformFeatures contributing to outdatedWarningPatterns.

gemini-code-assist · 2025-06-05T15:55:33Z

src/util/feature-mixin.js

+          if (this.featureMapValue) {
+            feature.featureMapValue = this.featureMapValue
+            feature.featureMapAttributeId = this.featureMapAttribute.id
+          }


In the clusterFeatures computed property, feature.featureMapValue and feature.featureMapAttributeId are assigned only if this.featureMapValue is truthy. If this.featureMapValue is 0 (a valid state meaning no features are enabled via the map), these properties on the feature object won't be set. Is this the intended behavior? It seems featureMapValue would still be relevant as 0 for determining enabled bits, and featureMapAttributeId might still be needed if the user intends to enable a feature on a map that's currently 0.

gemini-code-assist · 2025-06-05T15:55:33Z

src/util/feature-mixin.js

+    // build new feature map after both device type and cluster feature updates
+    buildFeatureMap(inclusionList, updatedFeature = null) {
+      let featureMap = {}
+      if (updatedFeature) {
+        // for device type feature updates, merge cluster feature data into inclusionList
+        let clusterInclusionList = [...this.enabledClusterFeatures]
+        // handle edge case for disabling a device type feature: remove it from cluster inclusionList
+        if (!this.featureIsEnabled(updatedFeature, inclusionList)) {
+          clusterInclusionList = clusterInclusionList.filter(
+            (featureId) => featureId !== updatedFeature.featureId
+          )
+        }
+        inclusionList = [...clusterInclusionList, ...inclusionList]
+      }
+      this.clusterFeatures.forEach((feature) => {
+        featureMap[feature.code] = this.featureIsEnabled(feature, inclusionList)
+      })
+      return featureMap


The logic in buildFeatureMap for merging enabledClusterFeatures with inclusionList when updatedFeature is present (i.e., a toggle from the device type page) is crucial for consistency. The handling of disabling a device type feature by filtering it out of clusterInclusionList seems correct. This ensures the featureMap accurately reflects the intended combined state. This is a well-thought-out detail.

src-electron/db/query-attribute.js

src-electron/db/query-command.js

src-electron/db/query-event.js

test/multi-protocol.test.js

test/query.test.js

paulr34 · 2025-06-05T18:08:03Z

ethanzhouyc · 2025-06-25T16:00:04Z

ethanzhouyc · 2025-06-25T16:06:05Z

ethanzhouyc · 2025-06-26T17:40:51Z

ethanzhouyc added 2 commits June 4, 2025 18:31

cluster feature page phase 2

4533ba1

add and fix unit tests

31fa538

ethanzhouyc requested review from paulr34, tecimovic and brdandu as code owners June 5, 2025 15:52

ethanzhouyc requested a review from dhchandw June 5, 2025 15:53

gemini-code-assist bot reviewed Jun 5, 2025

View reviewed changes

paulr34 approved these changes Jun 5, 2025

View reviewed changes

clean up

4a5cb76

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cluster Feature Page - Enable feature toggling and sync with device type feature page #1604

Cluster Feature Page - Enable feature toggling and sync with device type feature page #1604

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Cluster Feature Page - Enable feature toggling and sync with device type feature page #1604

Are you sure you want to change the base?

Cluster Feature Page - Enable feature toggling and sync with device type feature page #1604

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!