feat(export): E2 — allow-list store, device catalog, Manage Matter Exports dialog - #122
Conversation
…ports dialog Milestone E2 of PRD-indigo-matter-export: devices become selectable for export with a declared role, and the loop guard (XNG3) goes live. - export_store.py — the allow-list. ExportEntry (indigo device id + §4.2 role + name override + options), RLock'd store persisted as ONE schema-versioned JSON string in pluginPrefs["matterExports"]. A blob it cannot parse is moved aside to "matterExports.corrupt" and the store starts empty: user config is logged and preserved, never silently discarded. - export_catalog.py — PRD §5.2 mapping. Returns EligibleDevice(roles, default) or Excluded(reason). The loop guard is pluginId ONLY (XAC6), checked before any type reasoning; type dispatch walks the IOM class-name chain rather than isinstance, which is unusable against the MagicMock'd indigo module. - MenuItems.xml — "Manage Matter Exports…" (UI-D). No <CallbackMethod>, so the dialog gets a single Close button and its buttons do the work. Master-detail is a filter textfield + Apply-filter button + single-select device menu with dynamicReload (a multi-select list has no CallbackMethod at all), role menu, name/polarity fields, Add/Remove buttons, a readonly status textfield (Indigo labels cannot change at runtime) and a readonly summary list. - plugin.py — builds the store in startup, seeds the dialog via get_menu_action_config_ui_values, and owns the pickers and callbacks. Excluded devices are listed with their reason (XAC9) under an "x-" id and rejected server-side; generators degrade to an empty list, never raise. Tests: 217 new (store CRUD/round-trip/corruption/locking, a §5.2 zoo with five invariants per row, dialog XML shape and every callback). Full suite 1504 green; pylint 9.24 repo-wide (9.21 on main), 10.00 for the two new modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe plugin now classifies Indigo devices for Matter export, persists an export allow-list, and provides a Manage Matter Exports dialog with filtering, role selection, add/update, removal, validation, and startup restoration. Bridge-client integration remains unwired. ChangesMatter export management
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant ManageMatterExports
participant export_catalog
participant IndigoDevices
participant ExportStore
User->>ManageMatterExports: Open manageMatterExports
ManageMatterExports->>IndigoDevices: Read candidate devices
ManageMatterExports->>export_catalog: classify(device)
export_catalog-->>ManageMatterExports: Eligible roles or exclusion reason
User->>ManageMatterExports: Add or update export
ManageMatterExports->>ExportStore: upsert(ExportEntry)
ExportStore-->>ManageMatterExports: Persisted export entry
User->>ManageMatterExports: Remove export
ManageMatterExports->>ExportStore: remove(device_id)
🚥 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: 1
🧹 Nitpick comments (4)
tests/test_export_menu.py (2)
225-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe injected
__iter__does not raise the error the test names.
iter()resolves__iter__on the type, not on the instance.boomis a plainMock, whose type defines no__iter__, sofor dev in indigo.devicesraisesTypeError: object is not iterableinstead of theRuntimeError("boom")you configured. The assertions still pass, becausegetExportCandidatescatches both. UseMagicMockso the failure under test is the one you intend.♻️ Proposed fix
-def test_picker_degrades_to_empty_on_error(plug, mock_indigo_base): - boom = Mock() - boom.__iter__ = Mock(side_effect=RuntimeError("boom")) - mock_indigo_base.devices = boom +def test_picker_degrades_to_empty_on_error(plug, mock_indigo_base): + boom = MagicMock() + boom.__iter__.side_effect = RuntimeError("boom") + mock_indigo_base.devices = boom assert plug.getExportCandidates(valuesDict=_values()) == [] plug.logger.exception.assert_called()Add
MagicMockto theunittest.mockimport at line 20.🤖 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_export_menu.py` around lines 225 - 230, Update test_picker_degrades_to_empty_on_error to create boom with MagicMock instead of Mock, and add MagicMock to the unittest.mock imports so the configured __iter__ raises RuntimeError("boom") as intended.
133-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuff RUF015: replace the single-element slice with
next(...).Both helpers build a full list to take one element.
♻️ Proposed fix
def test_conditional_field_is_counted_in_the_height_calculation(): - invert = [f for f in _menu_item().findall("./ConfigUI/Field") - if f.get("id") == "exportInvert"][0] + invert = next(f for f in _menu_item().findall("./ConfigUI/Field") + if f.get("id") == "exportInvert") assert invert.get("visibleBindingId") == "exportRole" assert invert.get("visibleBindingValue") == "windowCovering" assert invert.get("alwaysUseInDialogHeightCalc") == "true" def test_status_field_is_readonly(): - status = [f for f in _menu_item().findall("./ConfigUI/Field") - if f.get("id") == "exportStatus"][0] + status = next(f for f in _menu_item().findall("./ConfigUI/Field") + if f.get("id") == "exportStatus") assert status.get("readonly") == "true"🤖 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_export_menu.py` around lines 133 - 144, Update the field lookups in test_conditional_field_is_counted_in_the_height_calculation and test_status_field_is_readonly to use next(...) over the matching Field elements instead of building a list and indexing element zero. Preserve the existing predicates and assertions.Source: Linters/SAST tools
indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py (1)
164-169: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
upsertaccepts a role the store itself would reject on reload.
ExportEntry.from_dictrejects a role outsideROLES, but theExportEntryconstructor andupsertdo not. An entry created in code with an invalid role persists successfully, then disappears at the next_loadwith a "dropping an unusable entry" error. Todayplugin.py:exportAddOrUpdatevalidates the role against the catalog before callingupsert, so no current caller can reach this. Enforcing the invariant in the store keeps write-time and read-time validation symmetric for E3's bridge wiring.♻️ Validate the role at construction
indigo_device_id: int role: str name_override: Optional[str] = None options: dict = field(default_factory=dict) + def __post_init__(self) -> None: + if self.role not in ROLES: + raise ValueError(f"unknown role {self.role!r} (device {self.indigo_device_id})") + def to_dict(self) -> dict:🤖 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_store.py around lines 164 - 169, Update ExportEntry construction and the upsert method to validate that role belongs to ROLES before storing or persisting the entry, matching ExportEntry.from_dict validation. Reject invalid roles consistently at write time so unusable entries cannot reach _save.indigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.py (1)
113-120: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe
" lx"hint requires a leading space, so it misses unspaced values.Indigo plugins often format a lux value without a space, for example
"340lx". That text matches no hint, so_numeric_sensor_rolereturnsNoneand_sensorexcludes the device withREASON_SENSOR_UNITS. The leading space exists to avoid matching words that contain "lx", but a trailing-anchored hint is unnecessary here because "lx" rarely appears inside English words used in device names. If you want to keep the guard, add the unspaced form as a separate hint.♻️ Suggested hint addition
- (ROLE_LIGHT_SENSOR, ("lux", " lx", "illuminance", "light level", "luminance")), + (ROLE_LIGHT_SENSOR, ("lux", " lx", "0lx", "1lx", "2lx", "3lx", "4lx", "5lx", + "6lx", "7lx", "8lx", "9lx", + "illuminance", "light level", "luminance")),A cleaner option is a small regex such as
\d\s*lx\bapplied to_unit_text.🤖 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_catalog.py around lines 113 - 120, Update the lux hints in _UNIT_PATTERNS so _numeric_sensor_role recognizes unspaced values such as “340lx” while preserving protection against incidental name matches; either add an unspaced “lx” hint as appropriate or apply a numeric-aware regex to _unit_text in the sensor-role matching logic.
🤖 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 `@tests/test_export_menu.py`:
- Around line 86-91: Add an inline Ruff S314 suppression to the ET.parse call in
_menu_item, recording that MENU_ITEMS_XML is a trusted repository-owned XML
file. Keep the existing menu-item lookup and missing-item assertion unchanged.
---
Nitpick comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_catalog.py:
- Around line 113-120: Update the lux hints in _UNIT_PATTERNS so
_numeric_sensor_role recognizes unspaced values such as “340lx” while preserving
protection against incidental name matches; either add an unspaced “lx” hint as
appropriate or apply a numeric-aware regex to _unit_text in the sensor-role
matching logic.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_store.py:
- Around line 164-169: Update ExportEntry construction and the upsert method to
validate that role belongs to ROLES before storing or persisting the entry,
matching ExportEntry.from_dict validation. Reject invalid roles consistently at
write time so unusable entries cannot reach _save.
In `@tests/test_export_menu.py`:
- Around line 225-230: Update test_picker_degrades_to_empty_on_error to create
boom with MagicMock instead of Mock, and add MagicMock to the unittest.mock
imports so the configured __iter__ raises RuntimeError("boom") as intended.
- Around line 133-144: Update the field lookups in
test_conditional_field_is_counted_in_the_height_calculation and
test_status_field_is_readonly to use next(...) over the matching Field elements
instead of building a list and indexing element zero. Preserve the existing
predicates and assertions.
🪄 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: 66fbc537-ebc4-45e2-9bd8-acabff6894be
📒 Files selected for processing (11)
CLAUDE.mddocs/HANDOVER.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xmlindigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_store.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/fakes.pytests/test_export_catalog.pytests/test_export_menu.pytests/test_export_store.py
| def _menu_item(): | ||
| root = ET.parse(MENU_ITEMS_XML).getroot() | ||
| for item in root.findall("MenuItem"): | ||
| if item.get("id") == "manageMatterExports": | ||
| return item | ||
| raise AssertionError("manageMatterExports menu item missing") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff reports S314 as an error here.
Ruff flags ET.parse for untrusted XML. The parsed file is the repository's own MenuItems.xml, so the rule does not apply. Ruff reports it at error severity, so the lint gate fails until you silence it explicitly.
🔧 Suppress the rule with the reason recorded
def _menu_item():
- root = ET.parse(MENU_ITEMS_XML).getroot()
+ # S314: the parsed file is this repository's own MenuItems.xml, not untrusted input.
+ root = ET.parse(MENU_ITEMS_XML).getroot() # noqa: S314
for item in root.findall("MenuItem"):📝 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.
| def _menu_item(): | |
| root = ET.parse(MENU_ITEMS_XML).getroot() | |
| for item in root.findall("MenuItem"): | |
| if item.get("id") == "manageMatterExports": | |
| return item | |
| raise AssertionError("manageMatterExports menu item missing") | |
| def _menu_item(): | |
| # S314: the parsed file is this repository's own MenuItems.xml, not untrusted input. | |
| root = ET.parse(MENU_ITEMS_XML).getroot() # noqa: S314 | |
| for item in root.findall("MenuItem"): | |
| if item.get("id") == "manageMatterExports": | |
| return item | |
| raise AssertionError("manageMatterExports menu item missing") |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 87-87: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 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_export_menu.py` around lines 86 - 91, Add an inline Ruff S314
suppression to the ET.parse call in _menu_item, recording that MENU_ITEMS_XML is
a trusted repository-owned XML file. Keep the existing menu-item lookup and
missing-item assertion unchanged.
Source: Linters/SAST tools
…batch
Store integrity
- S1 persist-then-commit: _commit writes the pref, flushes through an injected
save_prefs (indigo.server.savePluginPrefs), and only then adopts the new map
in memory, rolling the pref back if the flush raises. Pre-fix a failed
remove-save left memory and prefs divergent — the device vanished from the
dialog and resurrected on the next restart.
- S2 live prefs binding: the store takes a prefs_getter callable, not the
pluginPrefs object, so a PluginConfig save that rebinds pluginPrefs cannot
strand writes on an orphan mapping.
- S3 load_error: a corrupt blob or dropped rows are carried on the store and
surfaced by the dialog, which previously said "Nothing is exported yet." over
an unreadable list — inviting a rebuild whose first save clobbers the rescue.
- S4 first rescue wins: matterExports.corrupt is never overwritten; a second
corruption is logged as NOT preserved.
- S5 restored-entry validation: an injected entry_validator re-runs the loop
guard over entries loaded from prefs (the one write path the dialog's guards
never see), and from_dict enforces the options shape per role — `invert` only
on windowCovering, and only as a bool.
- S6 the Add/Remove callbacks contain a failing store write and report
"FAILED to save the export list", never a stale success.
Dialog
- D1 button callbacks return the values dict only. The (valuesDict, errorsDict)
tuple is the documented shape for validation methods, not for button
CallbackMethods (SDK ConfigUI > Button: "returns a dictionary … containing any
field changes", and the button's own field is read-only). Every refusal now
lands in exportStatus, including the early returns that previously returned
without setting it at all.
- D2 the picker always leads with a real ("0", "— select a device —") row for
the seeded value, and the truncation tail has its own id so "0" is unique.
- D3 per-device error containment in all three list callbacks: one unreadable
device costs one row, not the whole dialog (pre-fix a single raising proxy
made getExportCandidates return []). An outright failure returns a labelled
error row rather than an empty list.
- D4 excluded-but-exported coherence: the "●" marker survives on excluded rows,
the status names the incoherence, and the second excluded branch clears the
stale name/polarity it used to leave behind.
- D5 <Description> removed from the two textfields (undocumented for that type;
it is documented for menus and checkboxes, which keep theirs) into adjacent
label fields.
Catalog
- C1 classify() never propagates: a device whose attribute access raises yields
Excluded("error reading device") with one logged stack. Fail-closed — an
unreadable pluginId cannot prove the loop guard passed.
- C2 unit heuristic matches WORDS: "Fluxcapacitor" was a lightSensor, "Attempt
Counter" a temperatureSensor, "Epsilon Meter" a pressureSensor and "Overflow
Alarm" a flowSensor. Prose needles are \b-anchored; symbol needles keep the
matching that suits them. The device NAME is now the weakest tier, below
declared units and displayStateValUi.
- C3 RGB-only dimmer zoo row (a surviving mutation until now).
- C4 reverse-coverage invariant: the union of eligible roles across the zoo IS
bridge_protocol.ROLES, so a new role cannot ship UI-unreachable. Fixing it
exposed that two zoo sweeps classified against OTHER_PLUGIN_ID — the id the
zoo devices themselves carry — making them near-vacuous.
Reconcile + docs
- R1 report-only startup reconcile: warns per entry whose device is gone, no
longer exportable, or no longer offers its saved role. Never edits the list.
- R2 PRD §5.2 said excluded devices must be "absent from the picker with a
reason shown", contradicting XAC9 ("appear in the picker as excluded, with
reasons"). Aligned to XAC9 — the picker has always shown them.
- R3 XAC10 test: walks every .py under Server Plugin/ and asserts no matter.js
import, matching module names from real import statements against an
npm-shaped denylist (AST, not grep — "matter.js" appears in prose all over
this repo, and matter_client/matter_model/matter_handlers are ours).
- R4 ruff-style noqa in the E2 modules replaced with pylint-readable disables.
HANDOVER.md records the new store contract and, in bold, that the store is NOT
the guard: E3 must re-classify at endpoint-build time.
Tests 1504 -> 1630, all green. pylint 9.31/10 (was 9.24).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
XAC6 requires devices created by this plugin to be ABSENT from the picker, verified by unit test — not merely unpickable. They previously rendered as excluded-with-reason rows like every other exclusion, which was both a criterion violation and pure noise (each one shadows a real device already listed). All other exclusions keep their visible reasons (XAC9). classify() still returns Excluded for our own devices, so the store validator and both callbacks keep rejecting crafted picks — absence is presentation, the guard stays structural. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py (1)
1259-1260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the lint suppressions with the rest of the file. The new export code suppresses only pylint. The existing callbacks in this file also carry
# noqacodes, so Ruff stays quiet there. Ruff reports 7 diagnostics on the changed lines (A002 and BLE001). Add the matching# noqacodes next to the existing pylint disables.
indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1259-L1260: add# noqa: N802, A002to thegetExportCandidatessignature, matchinggetDeviceFoldersat line 976.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1114-L1114: add# noqa: BLE001to the blindexcept Exceptionin_indigo_device.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1293-L1293: add# noqa: BLE001to the per-rowexcept ExceptioningetExportCandidates.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1309-L1310: add# noqa: N802, A002to thegetExportRolessignature.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1332-L1332: add# noqa: BLE001to theexcept Exceptionaroundrole_label.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1339-L1340: add# noqa: N802, A002to thegetCurrentExportssignature.indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py#L1359-L1359: add# noqa: BLE001to the per-rowexcept ExceptioningetCurrentExports.♻️ Example for the first two sites
- except Exception: # pylint: disable=broad-except # KeyError/ValueError/Indigo's own + except Exception: # noqa: BLE001 - KeyError/ValueError/Indigo's own return None- def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0): - # pylint: disable=redefined-builtin, unused-argument + def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002 + # pylint: disable=redefined-builtin, unused-argument🤖 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.py around lines 1259 - 1260, Align Ruff suppressions with the existing pylint disables in plugin.py: add noqa N802, A002 to getExportCandidates, getExportRoles, and getCurrentExports signatures; add noqa BLE001 to the blind exception handlers in _indigo_device, getExportCandidates, getExportRoles’ role_label handling, and getCurrentExports. Apply these changes at plugin.py lines 1259-1260, 1114, 1293, 1309-1310, 1332, 1339-1340, and 1359.Source: Linters/SAST tools
tests/test_export_menu.py (1)
677-681: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
SCHEMA_VERSIONinstead of hardcoding"v": 1.
tests/test_export_store.pybuilds every blob withSCHEMA_VERSION. This test uses the literal1. If the schema version is bumped, the store classifies this blob as a wrong-version corruption, and the test then fails for the wrong reason instead of exercising the dropped-row path.♻️ Proposed change
- blob = json.dumps({"v": 1, "exports": [ + blob = json.dumps({"v": SCHEMA_VERSION, "exports": [ {"indigoDeviceId": 101, "role": "onOffLight"}, {"indigoDeviceId": 102, "role": "teleporter"}, ]})Add the import alongside the existing
PREF_KEYimport:from export_store import PREF_KEY, SCHEMA_VERSION🤖 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_export_menu.py` around lines 677 - 681, Update test_the_dialog_reports_dropped_rows_alongside_the_count to import SCHEMA_VERSION alongside PREF_KEY and use it for the blob’s v field instead of hardcoding 1, so the test remains valid when the schema version changes.
🤖 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 `@docs/PRD-indigo-matter-export.md`:
- Around line 238-239: Update §5.1 in docs/PRD-indigo-matter-export.md to remove
the requirement that indigo-matter’s own devices be filtered out of the
candidate list. Describe the loop guard as classifying those devices and
displaying them as non-selectable rows with the REASON_LOOP_GUARD reason,
consistent with _candidate_row, export_catalog.classify, and exportAddOrUpdate.
---
Nitpick comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/plugin.py:
- Around line 1259-1260: Align Ruff suppressions with the existing pylint
disables in plugin.py: add noqa N802, A002 to getExportCandidates,
getExportRoles, and getCurrentExports signatures; add noqa BLE001 to the blind
exception handlers in _indigo_device, getExportCandidates, getExportRoles’
role_label handling, and getCurrentExports. Apply these changes at plugin.py
lines 1259-1260, 1114, 1293, 1309-1310, 1332, 1339-1340, and 1359.
In `@tests/test_export_menu.py`:
- Around line 677-681: Update
test_the_dialog_reports_dropped_rows_alongside_the_count to import
SCHEMA_VERSION alongside PREF_KEY and use it for the blob’s v field instead of
hardcoding 1, so the test remains valid when the schema version changes.
🪄 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: 5dcb9255-4dbc-499e-a4af-5600ee1639df
📒 Files selected for processing (11)
docs/HANDOVER.mddocs/PRD-indigo-matter-export.mdindigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xmlindigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_store.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/fakes.pytests/test_export_catalog.pytests/test_export_menu.pytests/test_export_store.pytests/test_xac10_no_matter_js.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/HANDOVER.md
- indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml
| Excluded devices must **appear in the picker as excluded, with reasons** (XAC9), | ||
| not silently missing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This XAC9 statement now contradicts §5.1.
Line 178 states the candidate list MUST be filtered by plugin ID to exclude indigo-matter's own devices. Lines 238-239 state excluded devices must appear in the picker with reasons. Devices caught by the loop guard fall under both rules.
The implementation follows the new rule: _candidate_row in plugin.py returns an x--prefixed row labelled with REASON_LOOP_GUARD, so own devices are listed and not selectable. The loop guard remains structural through export_catalog.classify and the server-side refusal in exportAddOrUpdate. Only the §5.1 wording is stale.
Update §5.1 so it describes the loop guard as classification plus a non-selectable row, not as removal from the list.
🤖 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 `@docs/PRD-indigo-matter-export.md` around lines 238 - 239, Update §5.1 in
docs/PRD-indigo-matter-export.md to remove the requirement that indigo-matter’s
own devices be filtered out of the candidate list. Describe the loop guard as
classifying those devices and displaying them as non-selectable rows with the
REASON_LOOP_GUARD reason, consistent with _candidate_row,
export_catalog.classify, and exportAddOrUpdate.
1 participant
Summary
Milestone E2 (PRD §5.1/§5.2): everything between the user and the export set.
export_store.py— RLock'd allow-list (ExportEntry: device id, role, name override, options), schema-versioned JSON under onepluginPrefskey; corrupt blobs preserved undermatterExports.corrupt, never silently discarded; a bad row drops only itselfexport_catalog.py— §5.2 classification: eligible roles + default per device, exclusions with reasons; loop guard is pluginId-first and structural (XNG3); valve/garage/fan never offered as roles with reasons carried inEXCLUDED_ROLESTests
+217 (suite 1504 green): XAC6 (own-plugin device excluded regardless of type — unit-tested per PRD §8), XAC9, store CRUD/locking/corruption, §5.2 zoo-style table test, all dialog callbacks incl. degrade-to-empty generators and the comma/semicolon/empty-id constraints. New modules: pylint 10.00 (repo-wide 9.24 vs 9.21 baseline).
Not in scope
bridge_clientdoesn't consult the store yet — E3 wires deviceUpdated → set_state and command → indigo.device.*.🤖 Generated with Claude Code
https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Summary by CodeRabbit