refactor: split plugin.py behind the XML wiring (#146) - #172
Merged
Conversation
Steps 1-2 of the split: the pairing HTML template (QR_VIEWER_URL, _escape, _pairing_html) moves to pairing_page.py; module constants plus server_location/sanitize_host move to plugin_constants.py. plugin.py re-exports the names the test suite reaches for (blueprint §2a). Bodies moved verbatim; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Step 3: the five Actions.xml HTTP handlers (http_status/commission/ decommission/diagnostics/pairing) and their sync/async bodies move to http_api_mixin.py; Plugin composes the mixin so every callback stays resolvable by name. conftest's mock_indigo_base now evicts all plugin- composed modules from sys.modules so each test's indigo mock is fresh (blueprint §2c). Bodies moved verbatim; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
…gMixin (#146) Step 4: the 23 export-dialog members (candidates/roles/current lists, add/update/remove callbacks, summaries, warnings, reconcile machinery) move to export_dialog_mixin.py. The one write-hazard monkeypatch site (EXPORT_PICKER_LIMIT) retargets to the mixin module; every other test line is untouched. Bodies moved verbatim; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
…ixin (#146) Step 5: Pair Matter Bridge, Unpair an Ecosystem, the fabric picker/cache and the fabric backup/restore menus move to pairing_menu_mixin.py. Zero test-file changes — the bridge_agent patch sites target the shared module object and keep working. Bodies moved verbatim; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
) Step 6: matter-server install/reinstall/restart/logs, manual commissioning/decommission, export-bridge recovery (rebuild map, reset pairings) and the bridge-node LaunchAgent machinery move to server_menu_mixin.py. The 12 ServerProcess monkeypatch sites that drive the moved methods retarget to the mixin module; the startup-fixture sites stay on plugin. Bodies moved verbatim; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
…, 2026.9.2 (#146) Step 7: plugin.py's import block drops what the mixins took with them; a new guard test asserts no Server Plugin module back-imports plugin (the dependency arrows must keep pointing away); the module docstring and CLAUDE.md's architecture table now describe the seven-module shape. PluginVersion 2026.9.1 -> 2026.9.2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe plugin’s HTTP, pairing, export, and server-management responsibilities now reside in dedicated mixins and helper modules. Shared constants and pairing-page rendering moved out of ChangesPlugin responsibility split
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
indigo-matter.indigoPlugin/Contents/Server Plugin/export_dialog_mixin.py (1)
235-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the unpair seeding branch out of the export mixin.
get_menu_action_config_ui_valuesis a single Indigo callback name. This mixin now owns it, but it seedsMENU_UNPAIR_ECOSYSTEM, which belongs toPairingMenuMixin. If a future change adds the same method to another mixin, MRO resolves only one definition and the other branch disappears without an error.A dispatch approach keeps each mixin responsible for its own dialog:
Plugin(or one mixin) owns the callback and delegates to per-mixin seed helpers such as_seed_export_dialogand_seed_unpair_dialog. No behavior change is required for this PR; a comment naming the constraint would also be enough.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_dialog_mixin.py around lines 235 - 258, Refactor get_menu_action_config_ui_values so callback ownership and dialog seeding are not split across competing mixins: move the MENU_UNPAIR_ECOSYSTEM handling into PairingMenuMixin via a dedicated helper such as _seed_unpair_dialog, keep export initialization in an export-specific helper, and have the single callback owner dispatch to both without changing returned values.tests/test_plugin_module.py (1)
589-597: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the glob matched files.
If
SERVER_PLUGINever resolves to the wrong directory,glob("*.py")returns nothing and this test passes without checking anything. The guard then stops protecting the dependency direction and no test fails to say so.Add a non-empty check on the scanned paths.
🧪 Proposed change
def test_no_mixin_module_imports_plugin(): """The dependency arrows point away from plugin.py — a back-import would make the extraction a cycle waiting to happen (issue `#146`).""" - for path in SERVER_PLUGIN.glob("*.py"): + paths = sorted(SERVER_PLUGIN.glob("*.py")) + assert paths, f"no modules found under {SERVER_PLUGIN} — the guard would pass vacuously" + for path in paths: if path.name == "plugin.py": continue src = path.read_text(encoding="utf-8") offenders = [line for line in src.splitlines() if _BACK_IMPORT_RE.match(line)] assert not offenders, f"{path.name} back-imports plugin: {offenders}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_plugin_module.py` around lines 589 - 597, Update test_no_mixin_module_imports_plugin to assert that SERVER_PLUGIN.glob("*.py") returns at least one path before scanning imports, so an incorrect or empty directory cannot make the test pass vacuously.tests/conftest.py (1)
78-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving
_PLUGIN_MODULESfrom the mixin file names.The tuple is maintained by hand. If a later change adds a mixin and does not add it here, that module stays in
sys.modulesbound to a previous test'sindigomock whilepluginis reloaded against the new one. The resulting cross-test leak is hard to attribute.
tests/test_plugin_module.pyalready resolvesSERVER_PLUGIN, so the names can be globbed instead._PLUGIN_MODULES = ("plugin", "plugin_constants", "pairing_page") + tuple( path.stem for path in SERVER_PLUGIN.glob("*_mixin.py"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 78 - 97, Update `_PLUGIN_MODULES` used by `mock_indigo_base` to derive mixin module names from `SERVER_PLUGIN.glob("*_mixin.py")` instead of maintaining those names manually. Preserve the explicit core modules and append each matched path’s stem so newly added mixins are evicted from `sys.modules` before reloading `plugin`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/http_api_mixin.py:
- Around line 135-143: Update the decommission flow around remove_node to catch
Matter-server transport failures such as ConnectionError separately from
device-removal errors, and raise MatterUnavailable so the request produces the
expected 503 response. Preserve the existing fabric_removed=False handling for
non-transport removal failures and only continue device-sync cleanup when
appropriate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/pairing_menu_mixin.py:
- Around line 191-192: Update pairing_menu_mixin.py at lines 191-192 by adding
the Ruff noqa suppressions N802 and A002 to the getBridgeFabrics signature,
preserving its Indigo-required filter argument and method name. Also update line
380 by adding the BLE001 noqa alongside the existing broad-except pylint
suppression; no other constructs require changes.
- Around line 486-491: Update the success logging around restore_backup to
handle result["moved_aside_to"] being None: omit the “previous fabric preserved
at …” phrase when no directory was moved aside, while retaining it with the path
when present. Match the conditional message treatment already used for
bridge_moved_aside_to in the nearby bridge branch.
- Around line 428-438: Widen the try block in the backup-options callback to
include storage-path resolution, fabric_backup.list_backups(storage_path), and
option/label construction, returning an empty list when any of these operations
raises. Keep the existing exception logging behavior and successful options
return unchanged, matching the scope used by getBridgeFabrics.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/plugin_constants.py:
- Around line 81-88: Update server_location in plugin_constants.py at lines
81-88 to pass the configured matterServerHost through sanitize_host before
comparing loopback values. Update sanitize_host at lines 100-106 to extract
bracketed IPv6 literals before checking for an embedded port, so values such as
[::1]:8176 normalize correctly; apply both changes in the named locations.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/server_menu_mixin.py:
- Around line 111-127: Use the tri-state result from
ServerProcess.ensure_installed() in the controller install flow: return without
calling restart() when it is None, and skip the explicit restart when it is
True; call restart() only when the result is False. Align this branching with
menuRestartMatterServer and _install_bridge_node, preserve the existing failure
handling for the required restart, and update the test stub in
test_plugin_behaviour.py to return False explicitly.
---
Nitpick comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_dialog_mixin.py:
- Around line 235-258: Refactor get_menu_action_config_ui_values so callback
ownership and dialog seeding are not split across competing mixins: move the
MENU_UNPAIR_ECOSYSTEM handling into PairingMenuMixin via a dedicated helper such
as _seed_unpair_dialog, keep export initialization in an export-specific helper,
and have the single callback owner dispatch to both without changing returned
values.
In `@tests/conftest.py`:
- Around line 78-97: Update `_PLUGIN_MODULES` used by `mock_indigo_base` to
derive mixin module names from `SERVER_PLUGIN.glob("*_mixin.py")` instead of
maintaining those names manually. Preserve the explicit core modules and append
each matched path’s stem so newly added mixins are evicted from `sys.modules`
before reloading `plugin`.
In `@tests/test_plugin_module.py`:
- Around line 589-597: Update test_no_mixin_module_imports_plugin to assert that
SERVER_PLUGIN.glob("*.py") returns at least one path before scanning imports, so
an incorrect or empty directory cannot make the test pass vacuously.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7aca745b-ddf6-45ae-9054-20a1952fda6e
📒 Files selected for processing (13)
CLAUDE.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/export_dialog_mixin.pyindigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.pyindigo-matter.indigoPlugin/Contents/Server Plugin/pairing_menu_mixin.pyindigo-matter.indigoPlugin/Contents/Server Plugin/pairing_page.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin_constants.pyindigo-matter.indigoPlugin/Contents/Server Plugin/server_menu_mixin.pytests/conftest.pytests/test_export_menu.pytests/test_plugin_behaviour.pytests/test_plugin_module.py
Comment on lines
+135
to
+143
| await self.matter.remove_node(node_id) | ||
| except Exception as exc: # noqa: BLE001 | ||
| self.logger.warning("remove_node failed (device may be offline): %s", exc) | ||
| fabric_removed = False | ||
| # Only forget the node if it actually left the fabric; otherwise it must | ||
| # stay listed so the user can retry. | ||
| removed_ids = self.device_sync.delete_node(node_id, forget=fabric_removed) | ||
| if not removed_ids and not fabric_removed and not known: | ||
| return None # genuinely unknown and unreachable → 404 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'matter*.py' 'http_handlers.py' 'test*.py' . | while IFS= read -r file; do
rg -n -C 3 'async def remove_node|def remove_node|ConnectionError|MatterUnavailable|_decommission' "$file" || true
doneRepository: simons-plugins/indigo-matter
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked relevant files:"
git ls-files | rg -n '(^|/)(matter.*\.py|http_handlers\.py|http_api_mixin\.py|test.*\.py)$|indigo-matter\.indigoPlugin/Contents/Server Plugin/http_api_mixin\.py' || true
echo
echo "Inspect target file outline:"
ast-grep outline indigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.py --view expanded || true
echo
echo "Relevant target lines:"
cat -n indigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.py | sed -n '100,170p'Repository: simons-plugins/indigo-matter
Length of output: 3597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect target file with escaped path:"
git ls-files -- 'indigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.py' | cat
wc -l 'indigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.py'
sed -n '80,170p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.py' | nl -ba -v80
echo
echo "Search remove_node and MatterUnavailable definitions/usages:"
rg -n -C 4 'remove_node|class MatterUnavailable|MatterUnavailable|transport|ConnectionError|ProtocolError|_decommission|fabric_removed|delete_node' indigo-matter.indigoPlugin/Contents/Server\ Plugin tests || trueRepository: simons-plugins/indigo-matter
Length of output: 385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
P='indigo-matter.indigoPlugin/Contents/Server Plugin/http_api_mixin.py'
echo "Target snippet:"
sed -n '80,180p' "$P"
echo
echo "Remove_node/MatterUnavailable/fabric removal search:"
rg -n -C 4 'remove_node|class MatterUnavailable|MatterUnavailable|connection|transport|Protocol|fabric_removed|delete_node|_decommission' indigo-matter.indigoPlugin/Contents/Server\ Plugin tests || true
echo
echo "Matter client declarations:"
P2='indigo-matter.indigoPlugin/Contents/Server Plugin/matter_client.py'
if [ -f "$P2" ]; then
wc -l "$P2"
sed -n '1,260p' "$P2"
fi
echo
echo "HTTP exception models:"
rg -n -C 3 'MatterUnavailable|MatterError|Exception|return Response|status_code|HTTP' indigo-matter.indigoPlugin/Contents/Server\ Plugin/http_api_mixin.py indigo-matter.indigoPlugin/Contents/Server\ Plugin/http_handlers.py tests || trueRepository: simons-plugins/indigo-matter
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
P='indigo-matter.indigoPlugin/Contents/Server Plugin'
echo "matter_client.py outline:"
ast-grep outline "$P/matter_client.py" --view expanded || true
echo
echo "matter_client.py remove_node implementation:"
sed -n '1,260p' "$P/matter_client.py"
echo
echo "http_handlers.py MatterUnavailable implementation:"
fd -i 'http_handlers.py' "$P" -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {}
echo
echo "decommission tests around remove_node failures:"
sed -n '1230,1256p' tests/test_device_sync.py
rg -n -C 8 'remove_node.*transport|decommission.*remove_node|remove_node.*fail|fabric_removed|removedIndigoDeviceIds' tests "$P/http_api_mixin.py" "$P/matter_client.py" || trueRepository: simons-plugins/indigo-matter
Length of output: 30068
Raise MatterUnavailable for Matter-server transport errors during decommission.
remove_node() uses request(), which can raise transport failures such as ConnectionError before the device is removed. Catch those separately and raise MatterUnavailable instead of returning a 200 response with fabricRemoved: False, which allows deleted Indigo devices to be recreated and contradicts the Domio 503 return contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/http_api_mixin.py around
lines 135 - 143, Update the decommission flow around remove_node to catch
Matter-server transport failures such as ConnectionError separately from
device-removal errors, and raise MatterUnavailable so the request produces the
expected 503 response. Preserve the existing fabric_removed=False handling for
non-transport removal failures and only continue device-sync cleanup when
appropriate.
Comment on lines
+191
to
+192
| def getBridgeFabrics(self, filter="", valuesDict=None, typeId="", targetId=0): | ||
| # pylint: disable=redefined-builtin, unused-argument |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff suppressions were not carried over consistently into the new module. Both sites keep a pylint pragma but lack the matching ruff noqa, so ruff reports findings in this newly extracted file. Neither construct should change — the filter argument name is fixed by Indigo's list-callback contract, and the broad catch is deliberate and documented.
indigo-matter.indigoPlugin/Contents/Server Plugin/pairing_menu_mixin.py#L191-L192: append# noqa: N802, A002to thegetBridgeFabricssignature, matchinggetFabricBackupsat Line 426.indigo-matter.indigoPlugin/Contents/Server Plugin/pairing_menu_mixin.py#L380-L380: append# noqa: BLE001alongside the existing# pylint: disable=broad-except.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 191-191: Function argument filter is shadowing a Python builtin
(A002)
📍 Affects 1 file
indigo-matter.indigoPlugin/Contents/Server Plugin/pairing_menu_mixin.py#L191-L192(this comment)indigo-matter.indigoPlugin/Contents/Server Plugin/pairing_menu_mixin.py#L380-L380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/pairing_menu_mixin.py
around lines 191 - 192, Update pairing_menu_mixin.py at lines 191-192 by adding
the Ruff noqa suppressions N802 and A002 to the getBridgeFabrics signature,
preserving its Indigo-required filter argument and method name. Also update line
380 by adding the BLE001 noqa alongside the existing broad-except pylint
suppression; no other constructs require changes.
Source: Linters/SAST tools
Comment on lines
+428
to
+438
| try: | ||
| storage_path = self._resolve_storage_path() | ||
| except Exception as exc: # noqa: BLE001 | ||
| self.logger.exception(exc) | ||
| return [] | ||
| options = [] | ||
| for entry in fabric_backup.list_backups(storage_path): | ||
| when = datetime.fromtimestamp(entry["mtime"], timezone.utc).strftime("%Y-%m-%d %H:%M UTC") | ||
| label = f"{entry['filename']} — {self._human_size(entry['size_bytes'])} — {when}" | ||
| options.append((entry["path"], label)) | ||
| return options |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Widen the try block to cover the backup enumeration.
The try covers only _resolve_storage_path(). fabric_backup.list_backups(storage_path) and the label construction run outside it. list_backups performs filesystem I/O, so an unreadable or non-directory backups path raises OSError out of this callback. Indigo runs list callbacks on the UI thread while the dialog opens, so the exception surfaces there instead of an empty picker.
getBridgeFabrics wraps its whole body for this reason. Apply the same scope here.
🛡️ Proposed fix
def getFabricBackups(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002
"""List-callback populating the restore picker (newest first)."""
try:
storage_path = self._resolve_storage_path()
+ options = []
+ for entry in fabric_backup.list_backups(storage_path):
+ when = datetime.fromtimestamp(entry["mtime"], timezone.utc).strftime("%Y-%m-%d %H:%M UTC")
+ label = f"{entry['filename']} — {self._human_size(entry['size_bytes'])} — {when}"
+ options.append((entry["path"], label))
+ return options
except Exception as exc: # noqa: BLE001
self.logger.exception(exc)
return []
- options = []
- for entry in fabric_backup.list_backups(storage_path):
- when = datetime.fromtimestamp(entry["mtime"], timezone.utc).strftime("%Y-%m-%d %H:%M UTC")
- label = f"{entry['filename']} — {self._human_size(entry['size_bytes'])} — {when}"
- options.append((entry["path"], label))
- return options📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| storage_path = self._resolve_storage_path() | |
| except Exception as exc: # noqa: BLE001 | |
| self.logger.exception(exc) | |
| return [] | |
| options = [] | |
| for entry in fabric_backup.list_backups(storage_path): | |
| when = datetime.fromtimestamp(entry["mtime"], timezone.utc).strftime("%Y-%m-%d %H:%M UTC") | |
| label = f"{entry['filename']} — {self._human_size(entry['size_bytes'])} — {when}" | |
| options.append((entry["path"], label)) | |
| return options | |
| try: | |
| storage_path = self._resolve_storage_path() | |
| options = [] | |
| for entry in fabric_backup.list_backups(storage_path): | |
| when = datetime.fromtimestamp(entry["mtime"], timezone.utc).strftime("%Y-%m-%d %H:%M UTC") | |
| label = f"{entry['filename']} — {self._human_size(entry['size_bytes'])} — {when}" | |
| options.append((entry["path"], label)) | |
| return options | |
| except Exception as exc: # noqa: BLE001 | |
| self.logger.exception(exc) | |
| return [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/pairing_menu_mixin.py
around lines 428 - 438, Widen the try block in the backup-options callback to
include storage-path resolution, fabric_backup.list_backups(storage_path), and
option/label construction, returning an empty list when any of these operations
raises. Keep the existing exception logging behavior and successful options
return unchanged, matching the scope used by getBridgeFabrics.
Comment on lines
+486
to
+491
| self.logger.info( | ||
| "Fabric restored from %s; previous fabric preserved at %s. matter-server is " | ||
| "restarting — watch the log for 'reconciled N node(s)' to confirm the devices " | ||
| "came back.", | ||
| result["restored_from"], result["moved_aside_to"], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle a None moved_aside_to in the success message.
restore_backup documents moved_aside_to as None when no existing controller storage dir was preserved. This message interpolates it unconditionally, so that case logs "previous fabric preserved at None". The bridge branch at Lines 499-503 already suppresses the equivalent phrase when bridge_moved_aside_to is None. Apply the same treatment here.
🐛 Proposed fix
+ preserved_fabric = (
+ f"; previous fabric preserved at {result['moved_aside_to']}"
+ if result["moved_aside_to"] else "")
self.logger.info(
- "Fabric restored from %s; previous fabric preserved at %s. matter-server is "
+ "Fabric restored from %s%s. matter-server is "
"restarting — watch the log for 'reconciled N node(s)' to confirm the devices "
"came back.",
- result["restored_from"], result["moved_aside_to"],
+ result["restored_from"], preserved_fabric,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.logger.info( | |
| "Fabric restored from %s; previous fabric preserved at %s. matter-server is " | |
| "restarting — watch the log for 'reconciled N node(s)' to confirm the devices " | |
| "came back.", | |
| result["restored_from"], result["moved_aside_to"], | |
| ) | |
| preserved_fabric = ( | |
| f"; previous fabric preserved at {result['moved_aside_to']}" | |
| if result["moved_aside_to"] else "") | |
| self.logger.info( | |
| "Fabric restored from %s%s. matter-server is " | |
| "restarting — watch the log for 'reconciled N node(s)' to confirm the devices " | |
| "came back.", | |
| result["restored_from"], preserved_fabric, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/pairing_menu_mixin.py
around lines 486 - 491, Update the success logging around restore_backup to
handle result["moved_aside_to"] being None: omit the “previous fabric preserved
at …” phrase when no directory was moved aside, while retaining it with the path
when present. Match the conditional message treatment already used for
bridge_moved_aside_to in the nearby bridge branch.
Comment on lines
+81
to
+88
| loc = str(prefs.get("serverLocation") or "").strip().lower() | ||
| if loc in ("local", "remote"): | ||
| return loc | ||
| if prefs.get("manageLaunchAgent", False): | ||
| return "local" | ||
| host = str(prefs.get("matterServerHost") or "").strip().lower() | ||
| if host and host not in ("localhost", "127.0.0.1", "::1"): | ||
| return "remote" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize the host before migration classification.
server_location treats http://localhost:8176 as remote, although sanitize_host documents that pasted URLs are valid input. sanitize_host also leaves [::1]:8176 unchanged because it has multiple colons. A migrated local configuration can therefore become remote and block managed-server actions.
indigo-matter.indigoPlugin/Contents/Server Plugin/plugin_constants.py#L81-L88: callsanitize_hostbefore the loopback comparison.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin_constants.py#L100-L106: extract bracketed IPv6 literals before testing for an embedded port.
📍 Affects 1 file
indigo-matter.indigoPlugin/Contents/Server Plugin/plugin_constants.py#L81-L88(this comment)indigo-matter.indigoPlugin/Contents/Server Plugin/plugin_constants.py#L100-L106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/plugin_constants.py around
lines 81 - 88, Update server_location in plugin_constants.py at lines 81-88 to
pass the configured matterServerHost through sanitize_host before comparing
loopback values. Update sanitize_host at lines 100-106 to extract bracketed IPv6
literals before checking for an embedded port, so values such as [::1]:8176
normalize correctly; apply both changes in the named locations.
Comment on lines
+111
to
+127
| self.server_process = ServerProcess(self._server_prefs(), self.logger) # pylint: disable=no-member | ||
| self.server_process.ensure_installed() | ||
| # Restart matter-server onto the just-installed version — otherwise the | ||
| # newly-installed package sits on disk while the OLD process keeps running | ||
| # (a running LaunchAgent doesn't pick up new files). This is what makes the | ||
| # menu action a one-click, no-CLI update. | ||
| self._expect_restart() # pylint: disable=no-member | ||
| if not self.server_process.restart(): | ||
| # Don't claim success: the new version may not be running. | ||
| self._restart_expected_until = 0.0 # let the crash diagnostic work | ||
| self.logger.error( | ||
| "matter-server was installed and pinned to node at %s, but the " | ||
| "restart onto the new version FAILED — the old version may still be " | ||
| "running. Use Plugins ▸ Matter ▸ Restart the Matter controller, or " | ||
| "reload the plugin.", sp.resolved_bin_dir, | ||
| ) | ||
| return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Gate the restart on ensure_installed()'s tri-state result, as the other two call sites do.
Line 112 discards the return value of ensure_installed(). That value is tri-state:
None— preflight failed and the plist was removed.restart()then logs "nothing to restart" and this block reports "the restart onto the new version FAILED — the old version may still be running", which names the wrong cause.True— launchd was already reloaded onto the just-written plist, so the new version is already running.restart()then boots the job out and back in a second time. That is an extra outage and every device session drops twice.False— the job was left untouched, sorestart()is required.
menuRestartMatterServer (Lines 186-189) and _install_bridge_node (Lines 734-743) both branch on this value. The controller install path is the only one that does not.
Note that tests/test_plugin_behaviour.py:1193 stubs ensure_installed=Mock(), which returns a truthy Mock, so it must be updated to Mock(return_value=False) alongside this change.
🛠️ Proposed fix
self.server_process = ServerProcess(self._server_prefs(), self.logger) # pylint: disable=no-member
- self.server_process.ensure_installed()
+ applied = self.server_process.ensure_installed()
+ if applied is None:
+ self.logger.error(
+ "matter-server was installed and pinned to node at %s, but its LaunchAgent "
+ "could not be written — see the reason above. The package is on disk; fix "
+ "that, then reload the plugin.", sp.resolved_bin_dir,
+ )
+ return
# Restart matter-server onto the just-installed version — otherwise the
# newly-installed package sits on disk while the OLD process keeps running
# (a running LaunchAgent doesn't pick up new files). This is what makes the
# menu action a one-click, no-CLI update.
self._expect_restart() # pylint: disable=no-member
- if not self.server_process.restart():
+ if applied is False and not self.server_process.restart():🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/server_menu_mixin.py
around lines 111 - 127, Use the tri-state result from
ServerProcess.ensure_installed() in the controller install flow: return without
calling restart() when it is None, and skip the explicit restart when it is
True; call restart() only when the result is False. Align this branching with
menuRestartMatterServer and _install_bridge_node, preserve the existing failure
handling for the required restart, and update the test stub in
test_plugin_behaviour.py to return False explicitly.
…-up) The plugin.py docstring's 'everything XML-named moved' claim now names its exception (Set Sensitivity Level stays with the action bridge); device_sync/export_bridge/launch_agent pointers follow the moved code; server_menu_mixin docs and the CLAUDE.md row name the commissioning menus; the pairing-mixin ServerProcess construction warns which module a test must patch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
…Backups (#146 follow-up) Pre-existing inconsistency on main, surfaced by CodeRabbit on the move. Pragma only, no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Owner
Author
|
Re the CodeRabbit findings: this PR is a verbatim code-motion refactor (issue #146's contract is explicitly no behaviour change — bodies were AST-verified identical to main). Five of the six findings describe behaviour that exists identically on main and merely moved files, so they are out of scope here:
These are candidates for follow-up issues rather than silent fixes inside a refactor diff. The sixth (missing 🤖 Generated with Claude Code |
…ish (#146 follow-up) /review-pr round 2: _PLUGIN_MODULES uses raising=False so a typo'd or renamed entry would silently no-op forever — a new test pins each entry to a real Server Plugin file, and the tuple's comment now states its rule honestly (constants/template modules bind no indigo). The back-import guard walks rglob so matter_handlers/ is covered; the two test-patching imports carry pylint pragmas (the repo lints with pylint, which ignores noqa); CLAUDE.md row wording de-confused. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #146.
plugin.pywas 3,170 lines carrying six responsibilities. It is now 996 lines of genuine lifecycle glue, with the menu/dialog/HTTP bands extracted into mixin modules that keep every XML-referenced callback resolvable as an attribute onPluginvia MRO — no XML edits, no renamed callbacks, no behaviour change.Shape
plugin_constants.pyserver_location/sanitize_host(needed by two+ modules — the alternative was a forbidden back-import)pairing_page.pyhttp_api_mixin.py_parse_request/_replyexport_dialog_mixin.pypairing_menu_mixin.pyserver_menu_mixin.pyclass Plugin(HttpApiMixin, ExportDialogMixin, PairingMenuMixin, ServerMenuMixin, indigo.PluginBase)— zero method-name collisions across the mixins (verified); mixins define no__init__/lifecycle overrides so thesuper()chain reachesPluginBaseuntouched.One commit per extraction step
Bodies moved verbatim (smallest-risk-first order from the issue), each step landing with the full suite green: constants+template → HTTP → export dialog → pairing/fabric → server/bridge menus → sweep.
Beyond the issue's plan (found during research)
mock_indigo_baseevicted onlypluginfromsys.modules; the new mixin modules bindindigoat import and would have cached the first test's mock forever. The fixture now evicts every plugin-composed module.ServerProcess, 1 ×EXPORT_PICKER_LIMIT):setattr(plugin_mod, …)rebinds thepluginmodule global, which a moved method no longer reads. Reads are covered instead by an explicit re-export block inplugin.py(the module namespace is part of the test contract — 19 names).test_no_mixin_module_imports_pluginkeeps the dependency arrows pointing away fromplugin.py.Verification
test_menu_callbacks_exist_on_plugin,test_action_callbacks_exist_on_plugin,test_dynamic_list_methods_exist_on_plugin) pin the XML contract.PluginVersion2026.9.1 → 2026.9.2. CLAUDE.md architecture table updated to the seven-module shape.🤖 Generated with Claude Code
https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Summary by CodeRabbit
New Features
Improvements
Chores