XKNX / XKNX/xknxtoolkit

[Detail Bug] GUI: Nested submodule parameters cannot be edited due to duplicated qualified ref IDs

Open
#102 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
4
Forks
0
Avg merge
15h 38m
Merged PRs (30d)
37

Description

Detail Bug Report

https://app.detail.dev/org_62aa40f5-2c23-4914-a665-3bb2068af20e/bugs/bug_119867ed-6127-4757-93d5-a9ecb9eee116

Introduced in 67c5f5469b469965161345313a1ff178558685c5 by @kewde on Jun 15, 2026

Summary

  • Context: packages/product/src/xknxmono/product/parser_v2/state.py builds and queries a tree of ParameterState (GlobalStateModuleState → nested ModuleState children) used by parser_v2 to expose a dynamic UI (DynamicUI), resolve user parameter edits (set_parameter_ref), and gate encoding.
  • Bug: ModuleState._qualify mis-qualifies def-relative parameter ref IDs when the surrounding scope is a nested submodule, producing a qualified ID with a duplicated _SM-… segment.
  • Actual vs. expected: Actual: qualify(local) returns …_MI-1_SM-1_M-…_MI-1_SM-1_P-… (duplicated _SM-1), and set_parameter_ref writes the new value under a stale local key (…_SM-1_SM-1_P-…) that the read path never uses. Expected: qualify(local) returns …_MI-1_SM-1_M-…_MI-1_P-… (single _SM-1), and set_parameter_ref updates the canonical def-relative key so reads immediately reflect the edit.
  • Impact: parameter refs on nested-submodule parameters are silently un-editable through the GUI; edits are stored under an orphan/stale key and never take effect in UI re-rendering or calculations.

Code with Bug

# packages/product/src/xknxmono/product/parser_v2/state.py

class ModuleState(ParameterState):
    def _qualify(self, ref_id: str) -> str:
        i, n = 0, min(len(self.module_instance_id), len(ref_id))
        while i < n and self.module_instance_id[i] == ref_id[i]:
            i += 1
        return self.module_instance_id + ref_id[i - 1 :]  # <-- BUG 🔴 LCP slice re-emits nested `_SM-…` segment, duplicating it

Explanation

_qualify attempts to build a fully-qualified UI ref ID by taking a longest-common-prefix between module_instance_id and a def-relative ref_id, then appending ref_id[i-1:]. This happens to work for top-level modules, where the common prefix ends exactly at the module def ID boundary.

For nested submodules, the common prefix stops earlier (at the parent ModuleDef boundary where instance tokens diverge from submodule-def tokens), so ref_id[i-1:] includes the submodule token that is already present in module_instance_id. The qualified ID therefore contains a duplicated _SM-… segment.

The write path for GUI edits (DynamicUI.set_parameter_refstate.set_instance_ref) uses find_scope_for_qualified to invert the qualified ID back into a local key. That inversion is not symmetric with _qualify for nested submodules: it selects the correct child scope but reconstructs a non-canonical local key (…_SM-1_SM-1_P-…). The new value is written under that stale key, while all reads (child.get(canonical_local)) continue to use the canonical def-relative key (…_SM-1_P-…), so the UI immediately re-renders the old value and calculations see the old value as well.

Recommended Fix

Replace the LCP character-scan with a prefix-based slice that mirrors find_scope_for_qualified:

def _qualify(self, ref_id: str) -> str:
    if self.ref_id is None or not ref_id.startswith(self.ref_id):
        return ref_id
    return self.module_instance_id + ref_id[len(self.ref_id):]  # <-- FIX 🟢 append only the def-relative suffix

Add a regression test for nested submodules asserting:

  • child.qualify(local) contains only a single _SM-… segment
  • child.find_scope_for_qualified(child.qualify(local)) == (child, local) (round-trip identity)

History

This bug was introduced in commit 67c5f54. The refactor added ModuleState._qualify using a longest-common-prefix scan plus ref_id[i - 1:], which works for top-level modules but duplicates the _SM-… token for nested submodules. A later commit, 7bb6a6a, introduced the asymmetric inverse find_scope_for_qualified and the set_parameter_ref/set_instance_ref routing path that flows through it, making the duplicate-segment issue manifest as a silent edit failure for nested-submodule parameters.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in packages/product/src/xknxmono/product/parser_v2/state.py at ModuleState._qualify, then trace DynamicUI.set_parameter_ref and find_scope_for_qualified. Add the nested-submodule regression coverage described in the issue, including the qualification and round-trip assertions; done means edits resolve to the canonical key and reads reflect the new value.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
frontend, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.