Connection settings

[jgeudens]

Summary by CodeRabbit

• New Features

  • Optional human-readable name fields for connections and devices; improved default naming for new items.
  • Tab titles update in real time as connection/device names are edited.

• Behaviour Changes

  • New items use a monotonic next ID strategy (max+1) for IDs.
  • Closing tabs removes the corresponding device and accepted changes persist; cancelling avoids ID leakage.

• Tests & Documentation

  • Expanded tests and spec docs to cover naming, ID allocation and tab-title behaviour.

[coderabbitai]

Walkthrough

Adds optional name fields to adapter connection/device schemas and defaults; new signals (DeviceConfigTab::nameChanged, SchemaFormWidget::fieldChanged) and wiring to update tab titles; relaxed conditional-schema parsing; multiple dialogs and widgets updated to use widget pointers for tab-close and to compute new IDs using max+1 semantics; extensive tests added.

Changes

Cohort / File(s)	Summary
Adapter schema & docs adapters/describe.json, adapters/json-rpc-spec.md	Introduce optional connections[].name (string) with defaults; bump top-level version metadata; document name default "Connection <id>".
Schema form widget src/customwidgets/schemaformwidget.h, src/customwidgets/schemaformwidget.cpp	Add fieldChanged(const QString&, const QString&) signal and wireFieldChanged() helper; wire field widgets to emit fieldChanged; relax conditional key derivation logic.
Device form & config tab src/customwidgets/deviceform.h, src/customwidgets/deviceform.cpp, src/customwidgets/deviceconfigtab.h, src/customwidgets/deviceconfigtab.cpp	Add onNameChanged handler and DeviceConfigTab::deviceId() accessor; emit nameChanged from name edits and persist name to SettingsModel; prefer deleteLater() for schema form cleanup.
Addable tab widget / tab-close API src/customwidgets/addabletabwidget.h, src/customwidgets/addabletabwidget.cpp	Change tabClosed signal to emit QWidget* and emit widget pointer before tab removal.
Adapter/Device dialogs src/dialogs/adaptersettings.h, src/dialogs/adaptersettings.cpp, src/dialogs/adapterdevicesettings.h, src/dialogs/adapterdevicesettings.cpp, src/dialogs/devicesettings.h, src/dialogs/devicesettings.cpp	Wire tab-name tracking: connect fieldChanged/nameChanged to update tab labels; compute valid adapter IDs; compute new device IDs as max(existing)+1; handle tab close via QWidget* and remove associated devices; simplify acceptValues() to write per-adapter effectiveConfig().
SettingsModel ID behaviour src/models/settingsmodel.cpp	Change addNewDevice() to use monotonic next ID = lastKey+1 rather than gap-filling first available ID.
Tests — schema widget tests/customwidgets/tst_schemaformwidget.h, tests/customwidgets/tst_schemaformwidget.cpp	Add helper for conditional schema without if.required; add tests for conditional behaviour and fieldChanged emission on string edits.
Tests — adapter settings & devices tests/dialogs/tst_adaptersettings.h, tests/dialogs/tst_adaptersettings.cpp, tests/dialogs/tst_adapterdevicesettings.h, tests/dialogs/tst_adapterdevicesettings.cpp	Large set of new tests covering ID allocation (max+1), default name initialisation ("Connection <id>"), tab-name initialisation and live updates, tab-close removal effects, persistence across accept/reopen and cancel flows.

Sequence Diagrams

sequenceDiagram
    actor User
    participant DeviceConfigTab
    participant AdapterDeviceSettings
    participant TabBar

    User->>DeviceConfigTab: Edit name field
    DeviceConfigTab->>DeviceConfigTab: _pNameEdit::textChanged
    DeviceConfigTab->>AdapterDeviceSettings: nameChanged(name)
    AdapterDeviceSettings->>TabBar: setTabText(index, name)

Loading

sequenceDiagram
    actor User
    participant SchemaFormWidget
    participant AdapterSettings
    participant TabBar

    User->>SchemaFormWidget: Edit name QLineEdit
    SchemaFormWidget->>SchemaFormWidget: wireFieldChanged()
    SchemaFormWidget->>AdapterSettings: fieldChanged("name", value)
    AdapterSettings->>TabBar: setTabText(index, value)
    Note over AdapterSettings: Falls back to formatTabName() if empty

Loading

Possibly related PRs

• Dev/UI type dependant #15: Modifies the same adapter JSON schema and SchemaFormWidget conditional/wiring logic — strong overlap.
• UI names #13: Adds and propagates device name through DeviceConfigTab and settings dialogs with tab-title syncing.
• Update device + remove connection #14: Changes adapter/device tab naming and tab-close handling across AdapterSettings/AdapterDeviceSettings and related widgets.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Connection settings' is directly related to the primary changes in the changeset, which adds a 'name' field to connection configurations across adapters and updates connection-related UI components.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch dev/connection_settings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/dialogs/adaptersettings.cpp (1)

82-90: ⚠️ Potential issue | 🟡 Minor

Avoid naive singularisation of every trailing s.

status becomes Statu 1, bus becomes Bu 1, and a one-character key like s can produce an empty label before label[0]. Prefer preserving the key unless there is an explicit known singular label.

🐛 Proposed fix

-    QString label = _propertyKey.endsWith('s') ? _propertyKey.chopped(1) : _propertyKey;
+    QString label = _propertyKey;
+    if (label == QStringLiteral("connections"))
+    {
+        label.chop(1);
+    }
     return QString("%1 %2").arg(label[0].toUpper() + label.mid(1)).arg(index);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/dialogs/adaptersettings.cpp` around lines 82 - 90, The current
AdapterSettings::formatTabName naively chops a trailing 's' from _propertyKey
causing wrong labels (e.g., "status" -> "Statu") and potential empty-label
access; change the logic to only singularize when _propertyKey.length() > 1 and
it actually looks like a plural you intend to handle (e.g., not one-character
keys and not known exceptions), and ensure label is non-empty before accessing
label[0]; update the code in formatTabName to compute label = _propertyKey (or
singularized only under the guarded conditions), then capitalize safely (check
label.isEmpty() first) and format the tab name.

🧹 Nitpick comments (4)

src/customwidgets/schemaformwidget.h (1)

47-49: Add \brief to the new signal comment.

The new signal is useful, but the comment should match the repository’s Qt Doxygen convention.

Proposed Doxygen style fix

 signals:
-    //! Emitted when a string field's value changes; \a key is the property name.
+    //! \brief Emitted when a string field's value changes; \a key is the property name.
     void fieldChanged(const QString& key, const QString& value);

As per coding guidelines, “Use Qt Doxygen style comments: /*! */ for multi-line blocks, //! for single-line; use \brief, \param, \return with backslash prefix (not @)”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/customwidgets/schemaformwidget.h` around lines 47 - 49, The signal
comment for fieldChanged lacks a \brief tag and should follow the Qt Doxygen
convention; update the comment immediately above the void fieldChanged(const
QString& key, const QString& value); declaration to a single-line Qt Doxygen
comment using //! \brief ... and include a brief description (and optional
\param entries for key and value if desired) so it matches the repository's Qt
Doxygen style.

src/customwidgets/deviceconfigtab.h (1)

49-51: Add \brief to the new signal comment.

The signal itself looks fine, but the new Doxygen comment should follow the project’s //! \brief ... style.

Proposed Doxygen style fix

 signals:
-    //! Emitted when the device name field changes.
+    //! \brief Emitted when the device name field changes.
     void nameChanged(const QString& name);

As per coding guidelines, “Use Qt Doxygen style comments: /*! */ for multi-line blocks, //! for single-line; use \brief, \param, \return with backslash prefix (not @)”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/customwidgets/deviceconfigtab.h` around lines 49 - 51, Update the
single-line Doxygen comment for the signal `nameChanged(const QString& name)` to
use the project Qt Doxygen style by replacing the current comment with a
single-line `//! \brief ...` form (e.g., `//! \brief Emitted when the device
name field changes.`) so it follows the `//! \brief`, backslash-escaped commands
convention used in `deviceconfigtab.h`.

src/dialogs/adaptersettings.cpp (1)

92-103: Move the tab-name update body out of the lambda.

The current lambda has multiple statements and nested control flow. Keep the connection lambda as a one-line dispatcher and put the logic in a named helper. As per coding guidelines, Avoid lambda expressions with more than 2 captures or multiple statements; use named functions instead for clarity.

♻️ Proposed refactor

 void AdapterSettings::connectTabNameTracking(SchemaFormWidget* form)
 {
-    connect(form, &SchemaFormWidget::fieldChanged, this, [this, form](const QString& key, const QString& value) {
-        if (key == "name")
-        {
-            const int tabIndex = _pItemTabs->indexOf(form);
-            if (tabIndex >= 0)
-            {
-                _pItemTabs->setTabName(tabIndex, value.isEmpty() ? formatTabName(tabIndex + 1) : value);
-            }
-        }
-    });
+    connect(form, &SchemaFormWidget::fieldChanged, this,
+            [this, form](const QString& key, const QString& value) { updateTabNameFromField(form, key, value); });
+}
+
+void AdapterSettings::updateTabNameFromField(SchemaFormWidget* form, const QString& key, const QString& value)
+{
+    if (key == "name")
+    {
+        const int tabIndex = _pItemTabs->indexOf(form);
+        if (tabIndex >= 0)
+        {
+            _pItemTabs->setTabName(tabIndex, value.isEmpty() ? formatTabName(tabIndex + 1) : value);
+        }
+    }
 }

Also add the private helper declaration to adaptersettings.h.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/dialogs/adaptersettings.cpp` around lines 92 - 103, The lambda passed in
AdapterSettings::connectTabNameTracking contains multi-statement logic updating
tab names; extract that body into a new private helper method (e.g.,
AdapterSettings::onSchemaFieldNameChanged) and leave the connect call as a
one-line dispatcher calling that method. Specifically, create a helper that
takes (SchemaFormWidget* form, const QString& key, const QString& value),
performs the existing key == "name" check, computes tabIndex via
_pItemTabs->indexOf(form), and calls _pItemTabs->setTabName(tabIndex,
value.isEmpty() ? formatTabName(tabIndex + 1) : value) when tabIndex >= 0;
update the connect invocation in connectTabNameTracking to call this helper from
the lambda, and add the helper declaration to adaptersettings.h as a private
method.

tests/customwidgets/tst_schemaformwidget.cpp (1)

512-525: Use QSignalSpy instead of a multi-statement capture lambda.

This makes the signal assertion clearer and avoids the guideline violation in the current lambda body. As per coding guidelines, Avoid lambda expressions with more than 2 captures or multiple statements; use named functions instead for clarity.

♻️ Proposed refactor

+#include <QSignalSpy>
 `#include` <QSpinBox>
 `#include` <QTest>

-    QString emittedKey;
-    QString emittedValue;
-    connect(&w, &SchemaFormWidget::fieldChanged,
-            [&emittedKey, &emittedValue](const QString& key, const QString& value) {
-                emittedKey = key;
-                emittedValue = value;
-            });
+    QSignalSpy spy(&w, &SchemaFormWidget::fieldChanged);
 
     auto* edit = w.findChild<QLineEdit*>();
     QVERIFY(edit != nullptr);
     edit->setText("updated");
 
-    QCOMPARE(emittedKey, QStringLiteral("host"));
-    QCOMPARE(emittedValue, QStringLiteral("updated"));
+    QCOMPARE(spy.count(), 1);
+    const QList<QVariant> arguments = spy.takeFirst();
+    QCOMPARE(arguments.at(0).toString(), QStringLiteral("host"));
+    QCOMPARE(arguments.at(1).toString(), QStringLiteral("updated"));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/customwidgets/tst_schemaformwidget.cpp` around lines 512 - 525, Replace
the multi-statement capture lambda used to connect to
SchemaFormWidget::fieldChanged with a QSignalSpy on the widget instance (w) to
capture the signal emission; create a QSignalSpy listening for
SchemaFormWidget::fieldChanged, trigger the change by calling QLineEdit::setText
on the found edit, then assert the spy recorded exactly one invocation and
compare the spy's first call arguments to "host" and "updated". Use the spy's
recorded arguments rather than captured variables (remove
emittedKey/emittedValue and the lambda connect) and keep the remaining
assertions using QCOMPARE on the spy results.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@adapters/describe.json`:
- Line 268: The "version" field currently contains an undocumented suffix
("0.0.1-master:ac7eec7"); update the value of the "version" property in
adapters/describe.json to follow the JSON-RPC spec debug format
"<semver>-<git-branch>" (e.g., "0.0.1-master") by removing the ":<hash>" suffix
so consumers that parse or compare the "version" key will match the documented
format.

In `@src/dialogs/adapterdevicesettings.cpp`:
- Around line 110-114: Update AdapterDeviceSettings::connectTabNameTracking so
the nameChanged lambda uses a fallback when the provided name is empty: compute
a label = name.isEmpty() ? constructTabName(tab) : name and pass that label to
_pDeviceTabs->setTabName(_pDeviceTabs->indexOf(tab), label). Also add the
private helper declaration for constructTabName(...) to adapterdevicesettings.h
so the method is visible to the implementation.

In `@src/dialogs/adaptersettings.cpp`:
- Around line 116-133: The code currently sets new item ids using
_nextItemTabIndex which can collide when tabs were deleted or configs imported;
change the id generation in the block that builds defaultValues (where
_itemSchema, defaultValues, _nextItemTabIndex and formatTabName are used) to
scan existing items/forms for the highest current "id" and set
defaultValues["id"] = maxId + 1 instead of using _nextItemTabIndex; keep
existing logic for default name (formatTabName) and calls to
form->setSchema(...) and connectTabNameTracking(...). Also add a regression test
that creates non-contiguous existing ids (e.g. [1,3]) and asserts the new id
becomes 4.

---

Outside diff comments:
In `@src/dialogs/adaptersettings.cpp`:
- Around line 82-90: The current AdapterSettings::formatTabName naively chops a
trailing 's' from _propertyKey causing wrong labels (e.g., "status" -> "Statu")
and potential empty-label access; change the logic to only singularize when
_propertyKey.length() > 1 and it actually looks like a plural you intend to
handle (e.g., not one-character keys and not known exceptions), and ensure label
is non-empty before accessing label[0]; update the code in formatTabName to
compute label = _propertyKey (or singularized only under the guarded
conditions), then capitalize safely (check label.isEmpty() first) and format the
tab name.

---

Nitpick comments:
In `@src/customwidgets/deviceconfigtab.h`:
- Around line 49-51: Update the single-line Doxygen comment for the signal
`nameChanged(const QString& name)` to use the project Qt Doxygen style by
replacing the current comment with a single-line `//! \brief ...` form (e.g.,
`//! \brief Emitted when the device name field changes.`) so it follows the `//!
\brief`, backslash-escaped commands convention used in `deviceconfigtab.h`.

In `@src/customwidgets/schemaformwidget.h`:
- Around line 47-49: The signal comment for fieldChanged lacks a \brief tag and
should follow the Qt Doxygen convention; update the comment immediately above
the void fieldChanged(const QString& key, const QString& value); declaration to
a single-line Qt Doxygen comment using //! \brief ... and include a brief
description (and optional \param entries for key and value if desired) so it
matches the repository's Qt Doxygen style.

In `@src/dialogs/adaptersettings.cpp`:
- Around line 92-103: The lambda passed in
AdapterSettings::connectTabNameTracking contains multi-statement logic updating
tab names; extract that body into a new private helper method (e.g.,
AdapterSettings::onSchemaFieldNameChanged) and leave the connect call as a
one-line dispatcher calling that method. Specifically, create a helper that
takes (SchemaFormWidget* form, const QString& key, const QString& value),
performs the existing key == "name" check, computes tabIndex via
_pItemTabs->indexOf(form), and calls _pItemTabs->setTabName(tabIndex,
value.isEmpty() ? formatTabName(tabIndex + 1) : value) when tabIndex >= 0;
update the connect invocation in connectTabNameTracking to call this helper from
the lambda, and add the helper declaration to adaptersettings.h as a private
method.

In `@tests/customwidgets/tst_schemaformwidget.cpp`:
- Around line 512-525: Replace the multi-statement capture lambda used to
connect to SchemaFormWidget::fieldChanged with a QSignalSpy on the widget
instance (w) to capture the signal emission; create a QSignalSpy listening for
SchemaFormWidget::fieldChanged, trigger the change by calling QLineEdit::setText
on the found edit, then assert the spy recorded exactly one invocation and
compare the spy's first call arguments to "host" and "updated". Use the spy's
recorded arguments rather than captured variables (remove
emittedKey/emittedValue and the lambda connect) and keep the remaining
assertions using QCOMPARE on the spy results.

🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d7307832-08f7-4e8d-848b-50c33579b3b5

📥 Commits

Reviewing files that changed from the base of the PR and between ca8a9c1 and 8107c9b.

⛔ Files ignored due to path filters (2)

• adapters/dummymodbusadapter.exe is excluded by !**/*.exe
• adapters/modbusadapter.exe is excluded by !**/*.exe

📒 Files selected for processing (16)

• adapters/describe.json
• adapters/dummymodbusadapter
• adapters/json-rpc-spec.md
• adapters/modbusadapter
• src/customwidgets/deviceconfigtab.cpp
• src/customwidgets/deviceconfigtab.h
• src/customwidgets/schemaformwidget.cpp
• src/customwidgets/schemaformwidget.h
• src/dialogs/adapterdevicesettings.cpp
• src/dialogs/adapterdevicesettings.h
• src/dialogs/adaptersettings.cpp
• src/dialogs/adaptersettings.h
• tests/customwidgets/tst_schemaformwidget.cpp
• tests/customwidgets/tst_schemaformwidget.h
• tests/dialogs/tst_adaptersettings.cpp
• tests/dialogs/tst_adaptersettings.h

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Keep the version format aligned with the JSON-RPC spec.

Line 268 now advertises 0.0.1-master:ac7eec7, but the spec documents debug versions as "<semver>-<git-branch>". The extra :<hash> suffix is undocumented and can break consumers that parse or compare the version using the published format.

Proposed format alignment

-    "version": "0.0.1-master:ac7eec7"
+    "version": "0.0.1-master"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@adapters/describe.json` at line 268, The "version" field currently contains
an undocumented suffix ("0.0.1-master:ac7eec7"); update the value of the
"version" property in adapters/describe.json to follow the JSON-RPC spec debug
format "<semver>-<git-branch>" (e.g., "0.0.1-master") by removing the ":<hash>"
suffix so consumers that parse or compare the "version" key will match the
documented format.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

tests/dialogs/tst_adapterdevicesettings.cpp (1)

596-633: Consider also asserting the leaked device is reaped on reopen.

The comment on line 614 notes that "leaked device 2 remains in model" after cancel, and the test then relies on the second dialog open trimming the model back to the adapter config so that the next add gets id 2. It would be worth adding an explicit QVERIFY(!model.hasDevice(2)) immediately after constructing w2 to pin down the "orphan removal on reopen" contract — that way a regression in the constructor's sync logic would fail here with a clearer message than the final QCOMPARE(assignedId, 2).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/dialogs/tst_adapterdevicesettings.cpp` around lines 596 - 633, Add an
explicit assertion that the leaked device was removed from the SettingsModel
when reopening the dialog: after constructing AdapterDeviceSettings w2(&model)
and obtaining tabs, call model.hasDevice(2) and QVERIFY that it returns false
(e.g., QVERIFY(!model.hasDevice(2))). This should be placed just before emit
tabs->addTabRequested() in
TestAdapterDeviceSettings::cancelAndReopenDoesNotLeakDeviceIds so the test fails
clearly if the constructor/sync logic does not reap the orphaned device id.

src/dialogs/adapterdevicesettings.cpp (1)

181-183: Minor: use the tab widget as receiver context, not the tab content.

Using tab (a DeviceConfigTab*) as the context object works, but the signal-receiver pattern is more conventional with this as context and a guarded lookup. Current code is functionally correct because the connection is auto-disconnected when tab is destroyed; feel free to leave as-is.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/dialogs/adapterdevicesettings.cpp` around lines 181 - 183, Change the
connection to use this as the receiver/context instead of tab: connect(tab,
&DeviceConfigTab::nameChanged, this, [this, tab]() { int idx =
_pDeviceTabs->indexOf(tab); if (idx != -1) _pDeviceTabs->setTabName(idx,
constructTabName(tab)); }); This keeps the guarded lookup via
_pDeviceTabs->indexOf(tab) before calling setTabName and ensures the slot is
tied to the adapter object rather than the tab widget.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/customwidgets/deviceconfigtab.cpp`:
- Around line 106-113: The issue is that _deviceId is captured once and becomes
stale causing onNameChanged, constructTabName and
AdapterDeviceSettings::handleCloseTab to operate on the wrong device; fix by
keeping the tab's id in sync with the schema form: either subscribe to
SchemaFormWidget::fieldChanged for the "id" field (update the tab's _deviceId
whenever that field changes) or, alternatively, stop using the stored _deviceId
in onNameChanged, constructTabName and where handleCloseTab reads
tab->deviceId() and instead read the current id from
_pSchemaForm->values().value("id") (cast to deviceId_t) immediately before any
model operations (also ensure acceptValues() still writes the new id to the
adapter config).

In `@src/dialogs/adaptersettings.cpp`:
- Around line 88-93: The fallback label logic that sets QString label = (...)
overly aggressively chops a trailing 's' from _propertyKey (producing "Bu 1"
from "bus"); update the condition used to derive label so you only remove the
final 's' for likely plurals (e.g., require _propertyKey.length() > 3 and
exclude common non-plural endings like "ss", "us", "is" or "es" that form
singulars differently), or alternatively prefer an explicit display name when
available; modify the expression that assigns label (and related branch that
returns QString("%1 %2").arg(...).arg(index)) to use that safer heuristic so
short or special-case keys are not incorrectly truncated.

In `@src/models/settingsmodel.cpp`:
- Line 55: The device ID allocation in settingsmodel.cpp uses deviceId_t newId =
_devices.isEmpty() ? Device::cFirstDeviceId :
static_cast<deviceId_t>(_devices.lastKey() + 1) and lacks a guard against
overflow; add a runtime check against the maximum representable deviceId_t (e.g.
compare _devices.lastKey() to std::numeric_limits<deviceId_t>::max() or
UINT32_MAX) and handle the exhaustion case explicitly (reject the allocation by
returning an error/optional, throw an exception, or log and abort) instead of
allowing wrap-around that would let _devices[newId] overwrite an existing entry;
update the allocation code path that uses deviceId_t, _devices,
Device::cFirstDeviceId and lastKey() to perform this check and choose the
failure behavior consistent with the surrounding API.

---

Nitpick comments:
In `@src/dialogs/adapterdevicesettings.cpp`:
- Around line 181-183: Change the connection to use this as the receiver/context
instead of tab: connect(tab, &DeviceConfigTab::nameChanged, this, [this, tab]()
{ int idx = _pDeviceTabs->indexOf(tab); if (idx != -1)
_pDeviceTabs->setTabName(idx, constructTabName(tab)); }); This keeps the guarded
lookup via _pDeviceTabs->indexOf(tab) before calling setTabName and ensures the
slot is tied to the adapter object rather than the tab widget.

In `@tests/dialogs/tst_adapterdevicesettings.cpp`:
- Around line 596-633: Add an explicit assertion that the leaked device was
removed from the SettingsModel when reopening the dialog: after constructing
AdapterDeviceSettings w2(&model) and obtaining tabs, call model.hasDevice(2) and
QVERIFY that it returns false (e.g., QVERIFY(!model.hasDevice(2))). This should
be placed just before emit tabs->addTabRequested() in
TestAdapterDeviceSettings::cancelAndReopenDoesNotLeakDeviceIds so the test fails
clearly if the constructor/sync logic does not reap the orphaned device id.

🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7c4b9322-a02e-47d7-bfd4-cc1fd0bc690b

📥 Commits

Reviewing files that changed from the base of the PR and between 8107c9b and 80eb9f8.

📒 Files selected for processing (18)

• src/customwidgets/addabletabwidget.cpp
• src/customwidgets/addabletabwidget.h
• src/customwidgets/deviceconfigtab.cpp
• src/customwidgets/deviceconfigtab.h
• src/customwidgets/deviceform.cpp
• src/customwidgets/deviceform.h
• src/dialogs/adapterdevicesettings.cpp
• src/dialogs/adapterdevicesettings.h
• src/dialogs/adaptersettings.cpp
• src/dialogs/adaptersettings.h
• src/dialogs/devicesettings.cpp
• src/dialogs/devicesettings.h
• src/models/settingsmodel.cpp
• tests/customwidgets/tst_schemaformwidget.cpp
• tests/dialogs/tst_adapterdevicesettings.cpp
• tests/dialogs/tst_adapterdevicesettings.h
• tests/dialogs/tst_adaptersettings.cpp
• tests/dialogs/tst_adaptersettings.h

🚧 Files skipped from review as they are similar to previous changes (3)

• tests/customwidgets/tst_schemaformwidget.cpp
• src/dialogs/adapterdevicesettings.h
• tests/dialogs/tst_adaptersettings.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Name edits and tab close write to a stale _deviceId after the user changes the id field.

_deviceId is captured once in the constructor from deviceValues["id"] and never updated, but the schema form exposes id as an editable spinbox (test addTabAfterIdEditDoesNotDuplicate exercises that path). Once the user edits the id:

• onNameChanged writes setName(name) to the model entry keyed by the old _deviceId rather than the currently displayed id.
• AdapterDeviceSettings::handleCloseTab calls removeDevice(tab->deviceId()), so closing the tab removes the wrong (old) device from the model while acceptValues() persists the new id into adapter config.
• The tab title from constructTabName is also resolved against the stale id.

Consider keeping _deviceId in sync with the schema form (e.g. listen to SchemaFormWidget::fieldChanged for the id field, or always re-read _pSchemaForm->values().value("id") before using it), or disable editing of the id field in the UI so the two sources of truth cannot diverge.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/customwidgets/deviceconfigtab.cpp` around lines 106 - 113, The issue is
that _deviceId is captured once and becomes stale causing onNameChanged,
constructTabName and AdapterDeviceSettings::handleCloseTab to operate on the
wrong device; fix by keeping the tab's id in sync with the schema form: either
subscribe to SchemaFormWidget::fieldChanged for the "id" field (update the tab's
_deviceId whenever that field changes) or, alternatively, stop using the stored
_deviceId in onNameChanged, constructTabName and where handleCloseTab reads
tab->deviceId() and instead read the current id from
_pSchemaForm->values().value("id") (cast to deviceId_t) immediately before any
model operations (also ensure acceptValues() still writes the new id to the
adapter config).