Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/JsonSettingsIO.cpp (1)

90-123: Consider marking needsResave when defaults/clamps are applied.
Right now only obfuscation fallback triggers resave, so missing/out-of-range numeric values are corrected in memory but not persisted. Marking needsResave in those cases would let the file self-heal.

💡 Possible adjustment

-      uint8_t v = doc[info.key] | info.defaultValue;
+      bool corrected = !doc.containsKey(info.key);
+      uint8_t v = doc[info.key] | info.defaultValue;
       if (info.type == SettingType::ENUM) {
-        v = clamp(v, (uint8_t)info.enumValues.size(), info.defaultValue);
+        uint8_t clamped = clamp(v, (uint8_t)info.enumValues.size(), info.defaultValue);
+        corrected = corrected || (clamped != v);
+        v = clamped;
       } else if (info.type == SettingType::TOGGLE) {
-        v = clamp(v, (uint8_t)2, info.defaultValue);
+        uint8_t clamped = clamp(v, (uint8_t)2, info.defaultValue);
+        corrected = corrected || (clamped != v);
+        v = clamped;
       } else if (info.type == SettingType::VALUE) {
-        if (v < info.valueRange.min)
-          v = info.valueRange.min;
-        else if (v > info.valueRange.max)
-          v = info.valueRange.max;
+        if (v < info.valueRange.min) {
+          v = info.valueRange.min;
+          corrected = true;
+        } else if (v > info.valueRange.max) {
+          v = info.valueRange.max;
+          corrected = true;
+        }
       }
+      if (corrected && needsResave) *needsResave = true;
       s.*(info.valuePtr) = v;

🤖 Prompt for AI Agents

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

In `@src/JsonSettingsIO.cpp` around lines 90 - 123, When reading numeric settings
in the loop returned by getSettingsList(), if a setting's value is replaced by
its default or clamped into range you should set *needsResave = true so the
corrected value is persisted; specifically, in the else branch where you compute
uint8_t v = doc[info.key] | info.defaultValue and then adjust v for
SettingType::ENUM (using info.enumValues.size()), SettingType::TOGGLE, or
SettingType::VALUE (using info.valueRange.min/max), detect when the final v
differs from the raw document value (e.g. compare to doc[info.key] or the
default that was used) and if so set *needsResave = true before assigning into s
through the pointer (s.*(info.valuePtr)).

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/JsonSettingsIO.cpp (1)

90-125: Good job addressing the previous review feedback.

The fieldDefault pattern at lines 96 and 111 correctly preserves struct-initializer defaults when JSON keys are absent, and the per-type clamping (ENUM → default, TOGGLE → default, VALUE → boundary) is a sensible approach.

One minor defensive concern: if a SettingsList entry ever has stringMaxLen == 0, line 108's strncpy(..., info.stringMaxLen - 1) underflows to SIZE_MAX on unsigned types, causing a buffer overrun. Consider guarding:

🛡️ Optional defensive guard

+    if (info.stringMaxLen == 0) continue;
     strncpy(info.stringPtr, val.c_str(), info.stringMaxLen - 1);
     info.stringPtr[info.stringMaxLen - 1] = '\0';

🤖 Prompt for AI Agents

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

In `@src/JsonSettingsIO.cpp` around lines 90 - 125, The loop writes into
info.stringPtr using strncpy with info.stringMaxLen-1 which underflows when
info.stringMaxLen == 0; before the strncpy call in the stringPtr branch (inside
getSettingsList() iteration, including the obfuscated path that reads
doc[info.key+"_obf"]), add a guard that if info.stringMaxLen == 0 then skip
writing to info.stringPtr (or treat as a zero-length buffer) and avoid using
strncpy, and ensure you still handle needsResave/val logic as before; this
prevents the unsigned underflow and a potential buffer overrun while preserving
the existing default/obfuscation behavior.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/activities/settings/SettingsActivity.h (1)

107-107: Prefer uintptr_t over size_t for pointer-to-integer arithmetic.

size_t is not guaranteed to be large enough to hold a pointer on all platforms (though it is on most 32-bit embedded targets). uintptr_t from <cstdint> is the standard-defined type for this purpose.

♻️ Suggested fix

-    s.stringOffset = (size_t)ptr - (size_t)&SETTINGS;
+    s.stringOffset = reinterpret_cast<uintptr_t>(ptr) - reinterpret_cast<uintptr_t>(&SETTINGS);

(Also ensure #include <cstdint> is present, or that uintptr_t is available via another header.)

🤖 Prompt for AI Agents

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

In `@src/activities/settings/SettingsActivity.h` at line 107, The current
pointer-to-integer conversion uses size_t which may not be large enough on all
platforms; change the calculation of s.stringOffset to use uintptr_t by casting
ptr and &SETTINGS to uintptr_t (i.e., s.stringOffset = (uintptr_t)ptr -
(uintptr_t)&SETTINGS) and ensure <cstdint> is included so uintptr_t is
available; update the code referencing s.stringOffset, ptr and SETTINGS
accordingly.

src/JsonSettingsIO.cpp (1)

97-98: Consider using destPtr consistently instead of computing two pointers from the same offset.

Lines 97 and 110 both compute a pointer from info.stringOffset — one const for reading the default, one non-const for writing. This is fine for correctness, but you could compute it once as char* and use it for both purposes.

♻️ Minor simplification

     if (info.stringOffset) {
-      const char* strPtr = (const char*)&s + info.stringOffset;
-      const std::string fieldDefault = strPtr;  // current buffer = struct-initializer default
+      char* fieldPtr = (char*)&s + info.stringOffset;
+      const std::string fieldDefault(fieldPtr);  // current buffer = struct-initializer default
       std::string val;
       ...
-      char* destPtr = (char*)&s + info.stringOffset;
-      strncpy(destPtr, val.c_str(), info.stringMaxLen - 1);
-      destPtr[info.stringMaxLen - 1] = '\0';
+      strncpy(fieldPtr, val.c_str(), info.stringMaxLen - 1);
+      fieldPtr[info.stringMaxLen - 1] = '\0';
     }

🤖 Prompt for AI Agents

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

In `@src/JsonSettingsIO.cpp` around lines 97 - 98, Compute the pointer from
info.stringOffset only once and reuse it for both reading the default and
writing the destination instead of creating two separate pointers; for example,
replace separate uses of "const char* strPtr = (const char*)&s +
info.stringOffset" and the later non-const "destPtr" by a single "char* basePtr
= (char*)&s + info.stringOffset" (or similar name) and then use a const
cast/view when reading the default and the non-const pointer when writing, so
the same offset calculation is not duplicated.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro