Conversation
Added cJSONGuard wrapper to ensure proper cleanup of cJSON objects in updateFromJson, deserializeFromJson, and serializeToJson, preventing memory leaks on early returns or exceptions.
Invalidate NVS handle and flag after closing to prevent potential double-free if destructor is called multiple times.
Explicitly deleted copy and move operations to prevent unsafe duplication of NVS handle and non-movable mutex.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
main/ConfigManager.cpp (4)
1078-1082: Fix error message inconsistency.The error message at line 1080 says "key" but the code accepts "privateKey" (line 1079).
Apply this diff:
- ESP_LOGE(TAG, "Invalid certificate type: %s. Must be 'ca', 'client', or 'key'.", certType.c_str()); + ESP_LOGE(TAG, "Invalid certificate type: %s. Must be 'ca', 'client', or 'privateKey'.", certType.c_str());Based on past review comments.
1-19: Add missing standard headers.The code uses
std::span(line 418),std::remove_if(line 1185), and ctype functions::isspace/isalnum(lines 1185, 1204) but doesn't include the required headers.Apply this diff:
#include "ConfigManager.hpp" #include "cJSON.h" #include "config.hpp" +#include <algorithm> +#include <cctype> #include <ranges> +#include <span> #include <string> #include <vector>Based on past review comments.
1184-1208: Fix unsafe ctype function calls with signed char arguments.Using
::isspace(line 1185) andisalnum(line 1204) withchararguments causes undefined behavior whencharis signed and contains negative values.Apply this diff:
- base64Content.erase(std::remove_if(base64Content.begin(), base64Content.end(), ::isspace), base64Content.end()); + base64Content.erase(std::remove_if(base64Content.begin(), base64Content.end(), + [](char ch){ return std::isspace(static_cast<unsigned char>(ch)); }), base64Content.end());- } else if (!isalnum(c) && c != '+' && c != '/') { + } else if (!std::isalnum(static_cast<unsigned char>(c)) && c != '+' && c != '/') {Based on past review comments.
1104-1108: Fix error message inconsistency (duplicate of line 1080 issue).The error message at line 1106 says "key" but the code accepts "privateKey" (line 1105).
Apply this diff:
- ESP_LOGE(TAG, "Invalid certificate type: %s. Must be 'ca', 'client', or 'key'.", certType.c_str()); + ESP_LOGE(TAG, "Invalid certificate type: %s. Must be 'ca', 'client', or 'privateKey'.", certType.c_str());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main/ConfigManager.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
main/ConfigManager.cpp (5)
main/include/ConfigManager.hpp (21)
ConfigManager(24-24)ConfigManager(25-25)ConfigManager(28-28)ConfigManager(32-32)getConfig(38-38)deleteConfig(41-41)saveConfig(44-44)key(76-76)key(77-77)obj(71-71)serialize(74-74)updateFromJson(53-53)serializeToJson(50-50)deserializeFromJson(47-47)certType(55-55)certType(56-56)certType(79-79)certContent(58-58)certContent(80-80)certContent(81-81)keyContent(82-82)main/HomeKitLock.cpp (2)
begin(204-242)begin(204-204)main/MqttManager.cpp (2)
begin(95-153)begin(95-95)main/HardwareManager.cpp (2)
begin(106-222)begin(106-106)main/ReaderDataManager.cpp (2)
begin(40-56)begin(40-40)
🪛 Clang (14.0.6)
main/ConfigManager.cpp
[error] 1-1: 'ConfigManager.hpp' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: esp32s3
- GitHub Check: esp32c6
- GitHub Check: esp32
- GitHub Check: esp32c3
…management in certificate and key validation functions.
…d error handling, and explicit commit.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
main/HardwareManager.cpp (1)
461-465: Bug: stopping the wrong timer in SUCCESS feedback pathIn the SUCCESS case, you correctly check
m_pixelSuccessTimerbut stopm_gpioFailTimer(Line 464). This leaves the pixel success timer running and can cause the NeoPixel success indication to overrun or get stuck.case FeedbackType::SUCCESS: ESP_LOGD(TAG, "Executing SUCCESS feedback sequence."); if(esp_timer_is_active(m_gpioSuccessTimer)) esp_timer_stop(m_gpioSuccessTimer); - if(esp_timer_is_active(m_pixelSuccessTimer)) esp_timer_stop(m_gpioFailTimer); + if(esp_timer_is_active(m_pixelSuccessTimer)) esp_timer_stop(m_pixelSuccessTimer);
🧹 Nitpick comments (5)
docs/content/updates.md (3)
95-102: Clarify the purpose of flashing firmware to two offsets (app0 and app1).The command flashes
firmware.binto both0x20000(app0) and0x200000(app1). While this is now clarified as intentional for OTA redundancy, the documentation does not explain why. Users will likely assume it's a typo or mistake.Add a brief note explaining the dual-partition strategy, for example:
> [!NOTE] > This command flashes the firmware to both OTA partitions (app0 and app1) to ensure the device can safely boot from either partition during updates.Alternatively, if you prefer to keep this section simple, consider recommending the merged firmware approach instead (which is mentioned in the prerequisites at line 19 but not clearly linked here).
46-46: Use descriptive link text instead of "here".The link should describe its destination rather than using generic text like "here". Change line 46 to use a descriptive label.
-* `espota` tool (available as a Windows executable or a Python script for Linux/macOS, both available [here](https://github.com/espressif/arduino-esp32/tree/master/tools)). +* `espota` tool (available as a Windows executable or a Python script for Linux/macOS, both available [in the Arduino-ESP32 tools repository](https://github.com/espressif/arduino-esp32/tree/master/tools)).
51-51: Fix list indentation.Line 51 uses 4 spaces for indentation; Markdown convention for this list is 2 spaces.
* The OTA password, if you've set one in the [Configuration Guide](../configuration#homespan). - * The default OTA password is `homespan-ota` + * The default OTA password is `homespan-ota`main/HardwareManager.cpp (2)
50-71: Remove straybreakafterTAG_TAPcaseThere is an extra
break;after theTAG_TAPcase’s closing brace (Line 68). It’s harmless but dead code and can be removed to simplify the switch body.case TAG_TAP: { EventTagTap s = alpaca::deserialize<EventTagTap>(event.data, ec); if(!ec){ if (m_feedbackQueue != nullptr) { FeedbackType feedback = FeedbackType::TAG_EVENT; xQueueSend(m_feedbackQueue, &feedback, 0); } } break; } - break; default: break;
291-296: Guardm_pixelbefore using it inhandleTimerThe PIXEL_* timer branch unconditionally calls
i->m_pixel->off();. Today, the corresponding timers are only started whenm_pixel != nullptr, but this relies on that invariant never changing. A small future change could make this a null‑dereference.Adding a simple guard keeps the timer handler robust:
- case TimerSources::PIXEL_S: - case TimerSources::PIXEL_F: - case TimerSources::PIXEL_TAG_EVENT: - i->m_pixel->off(); - ESP_LOGD(TAG, "PIXEL"); - break; + case TimerSources::PIXEL_S: + case TimerSources::PIXEL_F: + case TimerSources::PIXEL_TAG_EVENT: + if (i->m_pixel != nullptr) { + i->m_pixel->off(); + } + ESP_LOGD(TAG, "PIXEL"); + break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/content/updates.md(1 hunks)main/HardwareManager.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
main/HardwareManager.cpp (2)
main/include/HardwareManager.hpp (7)
HardwareManager(26-26)state(37-37)initiator_isr_handler(90-90)arg(88-88)instance(82-82)instance(85-85)instance(92-92)main/include/LockManager.hpp (2)
state(58-58)instance(79-79)
🪛 Clang (14.0.6)
main/HardwareManager.cpp
[error] 1-1: 'event_manager.hpp' file not found
(clang-diagnostic-error)
🪛 markdownlint-cli2 (0.18.1)
docs/content/updates.md
46-46: Link text should be descriptive
(MD059, descriptive-link-text)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: esp32c6
- GitHub Check: esp32s3
- GitHub Check: esp32c3
- GitHub Check: esp32
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (43)
data/src/lib/stores/system.svelte.test.js (1)
5-15: Incomplete state reset in beforeEach.Beyond the
free_heapfield already flagged in the past review, the reset is also missinglog_levelandchip_model, both present in the store definition (Lines 18-28 in system.svelte.js). This omission can cause state leakage between tests.Apply this diff to include all missing fields:
beforeEach(() => { // Reset system info to initial state before each test updateSystemInfo({ deviceName: '', version: '', uptime: 0, + free_heap: 0, wifi_ssid: '', wifi_rssi: 0, eth_enabled: false, + log_level: 2, + chip_model: 0 }); });.github/workflows/esp32.yml (1)
187-188: Fix inverted file deletion regex—currently deletes compressed files.The regex pattern matches both compressed (
.html.gz,.json.gz,.br) and uncompressed (.css,.js) files, contradicting the step intent to keep compressed files. The command will delete both types, leaving the filesystem empty.To keep only compressed files and remove uncompressed originals, invert the logic:
- name: Remove extra, only keep compressed - run: find build -regex ".*\.\(html.gz\|css\|br\|js\|json.gz\)" -delete + run: find build -type f ! \( -name '*.gz' -o -name '*.br' \) -delete working-directory: dataThis deletes everything except
.gzand.brfiles, preserving space efficiency.README.md (3)
82-82: Remove hardcoded HomeKit setup code; security risk.Publishing a default setup code (
466-37-726) encourages insecure defaults and is a known security anti-pattern. Users should see the code in the device UI and change it immediately after first pairing.4. **Initial Setup** - Connect to the device's WiFi AP (`HomeSpan-Setup` / `homespan`) - Access the web interface at `http://192.168.4.1` - Configure your WiFi credentials and HomeKit setup code - - Pair with Apple Home using code: `466-37-726` + - Pair with Apple Home using the setup code shown in the Web UI + - **Important**: Change the setup code immediately after first pairing.
1-14: Fix markdownlint violations in header: MD023 & MD036.The centered div wrapper causes two linting errors:
- MD023 (line 4): Heading inside div doesn't start at column 1.
- MD036 (line 9): Emphasis used instead of a proper heading.
Restructure to satisfy linting while preserving visual centering:
-<div align="center"> - <img src="https://github.com/user-attachments/assets/fc93a70a-ef1e-4390-9067-6fafb255e5ac" alt="HomeKey-ESP32" width="200"/> - - # HomeKey-ESP32 - [](https://discord.com/invite/VWpZ5YyUcm) - [](https://github.com/rednblkx/HomeKey-ESP32/actions/workflows/esp32.yml) - [](LICENSE) - - **Apple HomeKey functionality for the rest of us** - - - [Documentation](https://rednblkx.github.io/HomeKey-ESP32/) • [Quick Start](#-60-second-quick-start) • [Setup Guide](docs/setup.md) • [Discord](https://discord.com/invite/VWpZ5YyUcm) - -</div> +# HomeKey-ESP32 + +<div align="center"> + <img src="https://github.com/user-attachments/assets/fc93a70a-ef1e-4390-9067-6fafb255e5ac" alt="HomeKey-ESP32" width="200"/> + + [](https://discord.com/invite/VWpZ5YyUcm) + [](https://github.com/rednblkx/HomeKey-ESP32/actions/workflows/esp32.yml) + [](LICENSE) + + Apple HomeKey functionality for the rest of us + + [Documentation](https://rednblkx.github.io/HomeKey-ESP32/) • [Quick Start](#quick-start) • [Setup Guide](https://rednblkx.github.io/HomeKey-ESP32/setup/) • [Discord](https://discord.com/invite/VWpZ5YyUcm) + +</div>
12-12: Fix broken link fragment:docs/setup.mddoesn't exist.The link points to a relative path that doesn't exist in the repo (docs are published via Hugo). Update to the published documentation URL:
- [Documentation](https://rednblkx.github.io/HomeKey-ESP32/) • [Quick Start](#-60-second-quick-start) • [Setup Guide](docs/setup.md) • [Discord](https://discord.com/invite/VWpZ5YyUcm) + [Documentation](https://rednblkx.github.io/HomeKey-ESP32/) • [Quick Start](#quick-start) • [Setup Guide](https://rednblkx.github.io/HomeKey-ESP32/setup/) • [Discord](https://discord.com/invite/VWpZ5YyUcm)data/src/lib/components/AppOTA.svelte (1)
229-234: State cleanup inconsistency between upload functions.The
uploadFirmwarefinally block only resets state whenskipRebootis false, butuploadLittleFS(lines 284-287) always resets state. IfuploadFirmwareis called withskipReboot=trueand an error occurs,uploadingandcurrentUploadTyperemain set, potentially leaving the UI stuck in an uploading state.While
uploadBoth's finally block (lines 333-336) provides a safety net for sequential uploads, this creates fragile implicit coupling.Consider always resetting state on error to prevent stuck UI:
} finally { - if (!skipReboot) { - uploading = false; - currentUploadType = ""; - } + // Always reset on error to prevent stuck UI + if (!skipReboot || otaStatus.error) { + uploading = false; + currentUploadType = ""; + } }docs/content/prerequisites.md (1)
109-109: Fix GitHub capitalization.Line 109 uses lowercase "Github"; the official name is "GitHub".
Apply this diff:
- * **PCB Files:** Available on Github [here](https://github.com/lollokara/HomeKey-ESP32-PCB) and you can manufacture it where you want, or it can be ordered from PCBWay [here](https://www.pcbway.com/project/shareproject/ESP32_Homekey_77a119d7.html). + * **PCB Files:** Available on GitHub [here](https://github.com/lollokara/HomeKey-ESP32-PCB) and you can manufacture it where you want, or it can be ordered from PCBWay [here](https://www.pcbway.com/project/shareproject/ESP32_Homekey_77a119d7.html).data/src/lib/stores/websocket.svelte.test.js (2)
93-100: Backoff max‑delay test still doesn’t exercise capping logic.This test sets
backoffCurrentDelayto40000and asserts that same value, so it only verifies that the store can hold values above the max, not that any reconnection/backoff logic is capped atbackoffMaxDelay. Either call the actual backoff/capping logic and assert the resulting delay is<= backoffMaxDelay, or rename the test to clarify that it only checks that the state can store values above the max.
162-196: “Reactive behavior” tests still don’t validate Svelte 5 reactivity.These tests manually call a local
effect()function reading fromwebsocketState; they don’t go through Svelte 5’s$statereactivity, so they only prove that state reads reflect prior writes. Either rename this block to something like “state read/write behavior” or add component‑level tests (e.g., via Svelte Testing Library) that assert UI or derived values update whenwebsocketStatechanges.data/src/lib/components/NavigationMenu.svelte (1)
64-71: Fix ARIA roles: use plain list semantics instead ofmenu/menuitem.This is a standard navigation list, not an application menu.
role="menu"/role="menuitem"is semantically incorrect and was already flagged earlier; native<nav>+<ul>/<li>/<a>is sufficient.Suggested change:
- <ul - class="menu flex-1 p-4 w-full bg-base-100 lg:bg-base-200" - role="menu" - onclick={onClose} - > - <li class="my-1" role="menuitem"> + <ul + class="menu flex-1 p-4 w-full bg-base-100 lg:bg-base-200" + onclick={onClose} + > + <li class="my-1"> @@ - <li class="my-1" role="menuitem"> + <li class="my-1"> @@ - <li class="my-1" role="menuitem"> + <li class="my-1"> @@ - <li class="my-1" role="menuitem"> + <li class="my-1"> @@ - <li class="my-1" role="menuitem"> + <li class="my-1"> @@ - <li class="my-1" role="menuitem"> + <li class="my-1"> @@ - <li class="lg:mt-auto hidden lg:inline-grid" role="menuitem"> + <li class="lg:mt-auto hidden lg:inline-grid">Also applies to: 91-91, 111-111, 131-131, 157-157, 177-177, 197-197
data/src/lib/components/AppMisc.svelte (4)
59-71: Normalizehk_key_colorto a number and derivehkFinishImageas a value.
hkFinishImagecurrently comparesc.value(number) tomiscConfig.hk_key_color, which can become a string from the radiobind:group, so the lookup can fail and always fall back toHKFinish0. Also,hkFinishImageis used as a function in the template.You can both normalize the type and simplify usage:
-const hkFinishColors = [ +const hkFinishColors = [ { name: "Tan", value: 0, image: HKFinish0 }, { name: "Gold", value: 1, image: HKFinish1 }, { name: "Silver", value: 2, image: HKFinish2 }, { name: "Black", value: 3, image: HKFinish3 }, ]; -let hkFinishImage = $derived(() => { - const color = hkFinishColors.find( - (c) => c.value === miscConfig.hk_key_color, - ); - return color?.image || HKFinish0; -}); +let hkFinishImage = $derived( + hkFinishColors.find( + (c) => c.value === Number(miscConfig.hk_key_color), + )?.image ?? HKFinish0, +); @@ - const saveMiscConfig = async (e: any) => { + const saveMiscConfig = async (e: any) => { e.preventDefault(); try { if (!miscConfig || !misc) return; + miscConfig.hk_key_color = Number(miscConfig.hk_key_color); const result = await saveConfig("misc", diff(misc, miscConfig)); @@ - <img - src={hkFinishImage()} + <img + src={hkFinishImage}This fixes the preview and ensures the saved config carries a numeric
hk_key_color.Also applies to: 80-88, 429-454
96-114: Fix NFC preset handling for string indices and disabled states.
miscConfig.nfcPinsPresetcomes from a<select>binding, so it will be a string. Strict comparisons with255and indexing with a string will behave inconsistently, and the “Custom” sentinel (255) branch never runs when the value is"255". Thedisabledchecks on the NFC pin inputs have the same issue.Suggested normalization:
- const handleNfcPresetChange = () => { - if ( - miscConfig.nfcPinsPreset !== undefined && - miscConfig.nfcPinsPreset !== 255 && - nfcPresets - ) { - const preset = nfcPresets.presets[miscConfig.nfcPinsPreset]; - if (preset) { - miscConfig.nfcGpioPins = [ - preset.gpioPins[0], - preset.gpioPins[1], - preset.gpioPins[2], - preset.gpioPins[3], - ]; - } - } else if (miscConfig.nfcPinsPreset === 255) { - miscConfig.nfcGpioPins = misc.nfcGpioPins; - } - }; + const handleNfcPresetChange = () => { + const presetIndex = Number(miscConfig.nfcPinsPreset); + + if ( + Number.isInteger(presetIndex) && + presetIndex !== 255 && + nfcPresets + ) { + const preset = nfcPresets.presets[presetIndex]; + if (preset) { + miscConfig.nfcGpioPins = [ + preset.gpioPins[0], + preset.gpioPins[1], + preset.gpioPins[2], + preset.gpioPins[3], + ]; + } + } else if (presetIndex === 255 && misc) { + miscConfig.nfcGpioPins = misc.nfcGpioPins; + } + }; @@ - <select - bind:value={miscConfig.nfcPinsPreset} - onchange={handleNfcPresetChange} + <select + bind:value={miscConfig.nfcPinsPreset} + onchange={handleNfcPresetChange} @@ - type="number" - disabled={miscConfig.nfcPinsPreset !== - 255} + type="number" + disabled={Number(miscConfig.nfcPinsPreset) !== 255} @@ - type="number" - disabled={miscConfig.nfcPinsPreset !== - 255} + type="number" + disabled={Number(miscConfig.nfcPinsPreset) !== 255} @@ - type="number" - disabled={miscConfig.nfcPinsPreset !== - 255} + type="number" + disabled={Number(miscConfig.nfcPinsPreset) !== 255} @@ - type="number" - disabled={miscConfig.nfcPinsPreset !== - 255} + type="number" + disabled={Number(miscConfig.nfcPinsPreset) !== 255}This makes presets and the “Custom” mode behave correctly regardless of string/number representation.
Also applies to: 462-479, 486-488, 502-504, 518-520, 534-536
73-78: Normalize Ethernet preset indices and fixcurrentEthChipfor index 0 and strings.
ethActivePresetandethPhyTypecome from<select>bindings, so they are strings. Comparisons with255and indexing intoboardPresets/supportedChipswill misbehave (e.g., preset"255"not treated as custom, index0skipped, disabled checks failing).Also,
currentEthChipuses a truthy check onethPhyType, which breaks for index0.Proposed changes:
- let currentEthChip = $derived(() => { - if (miscConfig.ethPhyType) { - return ethConfig!.supportedChips?.[miscConfig.ethPhyType]; - } - return null; - }); + const currentEthChip = () => { + const idx = Number(miscConfig.ethPhyType); + return Number.isInteger(idx) && idx >= 0 + ? ethConfig!.supportedChips?.[idx] ?? null + : null; + }; @@ - const handleEthPresetChange = () => { - if ( - miscConfig.ethActivePreset !== undefined && - miscConfig.ethActivePreset !== 255 && - ethConfig!.boardPresets - ) { - const preset = ethConfig!.boardPresets[miscConfig.ethActivePreset]; + const handleEthPresetChange = () => { + const presetIndex = Number(miscConfig.ethActivePreset); + + if ( + Number.isInteger(presetIndex) && + presetIndex !== 255 && + ethConfig!.boardPresets + ) { + const preset = ethConfig!.boardPresets[presetIndex]; if (preset) { miscConfig.ethPhyType = preset.ethChip.phy_type; @@ - $effect(() => { - if (misc?.ethActivePreset !== 255) { - handleEthPresetChange(); - } - }); + $effect(() => { + if ( + misc?.ethActivePreset != null && + Number(misc.ethActivePreset) !== 255 + ) { + handleEthPresetChange(); + } + }); @@ - <select - bind:value={ - miscConfig.ethActivePreset - } - onchange={handleEthPresetChange} + <select + bind:value={ + miscConfig.ethActivePreset + } + onchange={handleEthPresetChange} @@ - <select - bind:value={ - miscConfig.ethPhyType - } - disabled={miscConfig.ethActivePreset !== - 255} + <select + bind:value={ + miscConfig.ethPhyType + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethRmiiConfig![0] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethRmiiConfig![0] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethRmiiConfig![1] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethRmiiConfig![1] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethRmiiConfig![2] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethRmiiConfig![2] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethRmiiConfig![3] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethRmiiConfig![3] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethRmiiConfig![4] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethRmiiConfig![4] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![0] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![0] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![1] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![1] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![2] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![2] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![3] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![3] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![4] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![4] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![5] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![5] + } + disabled={Number(miscConfig.ethActivePreset) !== 255} @@ - bind:value={ - miscConfig - .ethSpiConfig![6] - } - disabled={miscConfig.ethActivePreset !== - 255} + bind:value={ + miscConfig + .ethSpiConfig![6] + } + disabled={Number(miscConfig.ethActivePreset) !== 255}This makes presets work for index
0, handles string values, and ensures “Custom” behaves correctly.Also applies to: 116-153, 161-165, 625-626, 634-639, 659-665, 676-793, 795-936
942-985: Make WebUI credentials required only when authentication is enabled.
miscConfig.webAuthEnabledtoggles WebUI auth, but the Username and Password inputs are alwaysrequired. When auth is disabled this will still block form submission.Condition these attributes on
webAuthEnabled:<div class="form-control mb-4"> @@ <input type="text" bind:value={miscConfig.webUsername} placeholder="admin" class="input input-bordered w-full" - required + disabled={!miscConfig.webAuthEnabled} + required={miscConfig.webAuthEnabled} /> @@ <input type="password" bind:value={miscConfig.webPassword} placeholder="password" class="input input-bordered w-full" - required + disabled={!miscConfig.webAuthEnabled} + required={miscConfig.webAuthEnabled} />data/src/lib/components/AppMqtt.svelte (1)
74-91: Fix certificate upload error/progress bindings for client and private key.The upload handler and UI currently hard‑code the CA slot:
- Invalid file types always set
uploadErrors.ca, regardless of which input was used.- The Client Certificate and Private Key sections bind to
uploadProgress.ca/uploadErrors.ca, so their own progress and errors are never shown correctly.Suggested fixes:
@@ - if (!validExtensions.includes(fileExtension)) { - uploadErrors.ca = - "Invalid file type. Must be .pem, .crt, .cer, .der, or .key"; - return; - } + if (!validExtensions.includes(fileExtension)) { + uploadErrors[type] = + "Invalid file type. Must be .pem, .crt, .cer, .der, or .key"; + return; + } @@ CA certificate section stays as-is - <input + <input type="file" @@ - disabled={uploadProgress.ca > 0 && - uploadProgress.ca < 100} + disabled={uploadProgress.ca > 0 && + uploadProgress.ca < 100} @@ - {#if uploadProgress.ca > 0 && uploadProgress.ca < 100} + {#if uploadProgress.ca > 0 && uploadProgress.ca < 100} @@ - {#if uploadErrors.ca} + {#if uploadErrors.ca} @@ Client Certificate - <input + <input type="file" @@ - disabled={uploadProgress.ca > 0 && - uploadProgress.ca < 100} + disabled={uploadProgress.client > 0 && + uploadProgress.client < 100} data-type="client" /> - {#if uploadProgress.ca > 0 && uploadProgress.ca < 100} + {#if uploadProgress.client > 0 && uploadProgress.client < 100} @@ - style="width: {uploadProgress.ca}%" + style="width: {uploadProgress.client}%" @@ - {#if uploadErrors.ca} + {#if uploadErrors.client} @@ - {uploadErrors.ca} + {uploadErrors.client} @@ Private Key - <input + <input type="file" @@ - disabled={uploadProgress.ca > 0 && - uploadProgress.ca < 100} + disabled={uploadProgress.privateKey > 0 && + uploadProgress.privateKey < 100} data-type="privateKey" /> - {#if uploadProgress.ca > 0 && uploadProgress.ca < 100} + {#if uploadProgress.privateKey > 0 && uploadProgress.privateKey < 100} @@ - style="width: {uploadProgress.ca}%" + style="width: {uploadProgress.privateKey}%" @@ - {#if uploadErrors.ca} + {#if uploadErrors.privateKey} @@ - {uploadErrors.ca} + {uploadErrors.privateKey}This aligns validation, progress, and error display with the actual input being used.
Also applies to: 557-584, 585-612, 613-640
data/src/lib/components/LogViewer.svelte (1)
8-18: FixvirtualListReftyping and guard scroll-to-bottom usage
virtualListRefis typed asReturnType<typeof VirtualList>butVirtualListis a Svelte component, not a function. This doesn’t match whatbind:thissets and can cause TS issues. The scroll-to-bottom handler also assumes the ref is always non-null.Consider:
- let virtualListRef: ReturnType<typeof VirtualList>; + let virtualListRef: InstanceType<typeof VirtualList> | null = null;and:
- onclick={async () => { - await virtualListRef.scroll({ - index: filteredLogs().length - 1, - }); + onclick={async () => { + const list = virtualListRef; + const lastIndex = filteredLogs().length - 1; + if (!list || lastIndex < 0) return; + await list.scroll({ + index: lastIndex, + }); autoScrollActive = true; }}This aligns the type with the Svelte instance and prevents a possible runtime error when the button is clicked before the list is mounted or when there are no logs.
Does Svelte's `bind:this` on a component instance assign the component instance type (usable with `InstanceType<typeof Component>`) and can it be `null` before mount?Also applies to: 446-457
data/src/routes/+layout.svelte (1)
4-8: Fix websocket store import path to match actual file and store conventionsAll other stores are imported with the
.svelte.jssuffix, butwebsocketStateis imported from$lib/stores/websocket.svelte. Previous tooling indicated the actual file iswebsocket.svelte.js, and mixing paths can create duplicate module instances under ESM or simply fail to resolve.Update to:
- import { websocketState } from '$lib/stores/websocket.svelte'; + import { websocketState } from '$lib/stores/websocket.svelte.js';and keep this consistent with other imports across the codebase.
In a SvelteKit + Vite project, can importing the same JS module once with an explicit `.js` extension and once without cause duplicate module instances or resolution errors?Also applies to: 12-16
data/src/lib/components/AppActions.svelte (2)
58-63: Use Svelteon:submit|preventDefaultinstead ofonsubmitThe form currently uses:
<form onsubmit={saveActionsConfig} class="flex flex-col items-center">In Svelte, event listeners should use
on:eventsyntax, andpreventDefaultis better expressed via a modifier. Also, you already calle.preventDefault()inside the handler.Prefer:
- <form onsubmit={saveActionsConfig} class="flex flex-col items-center"> + <form + on:submit|preventDefault={saveActionsConfig} + class="flex flex-col items-center" + >and then you can drop the explicit
e.preventDefault()insidesaveActionsConfigif you like.According to the Svelte docs, is `on:submit={handler}` (with optional `|preventDefault`) the correct way to attach submit handlers, and does using plain `onsubmit` work as expected?Also applies to: 86-87
31-43: Fix<select>bindings so booleans and enums don’t become stringsSeveral
<select>s bind directly to boolean or numeric fields:
nfcSuccessHL,nfcFailHL,tagEventHLgpioActionLockState,gpioActionUnlockStatehkGpioControlledState,hkDumbSwitchModegpioActionMomentaryEnabled(two places)hkAltActionGpioStateIn HTML, option values are strings, so after user interaction these fields will become
"true","false","0","1", etc., instead of booleans/integers expected by your config and backend.Use string-backed intermediates plus explicit coercion, e.g.:
- nfcSuccessHL: false, + nfcSuccessHL: false, @@ - nfcFailHL: false, + nfcFailHL: false, @@ - tagEventHL: false, + tagEventHL: false, @@ - gpioActionLockState: false, + gpioActionLockState: false, @@ - gpioActionUnlockState: false, + gpioActionUnlockState: false, @@ - hkGpioControlledState: false, + hkGpioControlledState: false, @@ - hkAltActionGpioState: 0, + hkAltActionGpioState: 0,Script additions (pattern):
let nfcSuccessHLStr = String(actionsConfig.nfcSuccessHL); let nfcFailHLStr = String(actionsConfig.nfcFailHL); let tagEventHLStr = String(actionsConfig.tagEventHL); let gpioLockStateStr = String(actionsConfig.gpioActionLockState); let gpioUnlockStateStr = String(actionsConfig.gpioActionUnlockState); let hkGpioControlledStateStr = String(actionsConfig.hkGpioControlledState); let hkDumbSwitchModeStr = String(actionsConfig.hkDumbSwitchMode); let gpioMomentaryStr = String(actionsConfig.gpioActionMomentaryEnabled); let hkAltActionGpioStateStr = String(actionsConfig.hkAltActionGpioState);Template examples:
- <select - bind:value={actionsConfig.nfcSuccessHL} - class="select select-bordered w-full" - > - <option value={false}>LOW</option> - <option value={true}>HIGH</option> - </select> + <select + bind:value={nfcSuccessHLStr} + on:change={() => + (actionsConfig.nfcSuccessHL = nfcSuccessHLStr === "true")} + class="select select-bordered w-full" + > + <option value="false">LOW</option> + <option value="true">HIGH</option> + </select>- <select - bind:value={actionsConfig.gpioActionMomentaryEnabled} - class="select select-bordered w-full" - > - <option value={0}>Disabled</option> - <option value={1}>Home App Only</option> - <option value={2}>Home Key Only</option> - <option value={3}>Home App + Home Key</option> - </select> + <select + bind:value={gpioMomentaryStr} + on:change={() => + (actionsConfig.gpioActionMomentaryEnabled = Number(gpioMomentaryStr))} + class="select select-bordered w-full" + > + <option value="0">Disabled</option> + <option value="1">Home App Only</option> + <option value="2">Home Key Only</option> + <option value="3">Home App + Home Key</option> + </select>Apply the same pattern for all boolean and enum selects so your config preserves the intended types.
How does Svelte handle `<select bind:value>` when the bound variable is boolean or number—does it coerce option values to strings by default?Also applies to: 400-435, 469-479, 621-627, 696-735, 741-751, 783-788
main/LockManager.cpp (3)
185-195: SimplifymomentarySourcescalculation for correctness and maintainabilityThe current expression combines bitwise OR, equality, bitwise AND, and logical negation in a ternary, which is hard to reason about and easy to misinterpret:
uint8_t momentarySources = (((m_actionsConfig.gpioActionMomentaryEnabled | m_actionsConfig.gpioActionPin) == 255) & !m_actionsConfig.hkDumbSwitchMode) ? 0 : m_actionsConfig.gpioActionMomentaryEnabled;Refactor to explicit booleans that capture the intent (configured pin, not dumb mode, momentary enabled).
- uint8_t momentarySources = (((m_actionsConfig.gpioActionMomentaryEnabled | - m_actionsConfig.gpioActionPin) == 255) & - !m_actionsConfig.hkDumbSwitchMode) - ? 0 - : m_actionsConfig.gpioActionMomentaryEnabled; + // Momentary is active only when: + // 1) GPIO action pin is configured (not 255), and + // 2) Not in dumb‑switch mode. + bool gpioConfigured = (m_actionsConfig.gpioActionPin != 255); + bool notDumbMode = !m_actionsConfig.hkDumbSwitchMode; + uint8_t momentarySources = + (gpioConfigured && notDumbMode) ? m_actionsConfig.gpioActionMomentaryEnabled : 0;
49-63: DuplicateEventLockState sdeclaration (compile error and confusing logic)Within the
"lock/updateState"subscriber,EventLockState sis declared fromalpaca::deserialize, then redeclared with an aggregate initializer in the same scope. This is illegal in C++ and also discards the originalsourcevalue.Reuse the existing
sobject instead of redeclaring it.espp::EventManager::get().add_subscriber( "lock/updateState", "LockManager", [&](const std::vector<uint8_t> &data) { std::error_code ec; EventLockState s = alpaca::deserialize<EventLockState>(data, ec); if (!ec) { m_currentState = s.currentState; - s.currentState = m_currentState; - s.targetState = m_targetState; - EventLockState s{ - .currentState = static_cast<uint8_t>(m_currentState), - .targetState = static_cast<uint8_t>(m_targetState), - .source = LockManager::INTERNAL - }; + s.currentState = static_cast<uint8_t>(m_currentState); + s.targetState = static_cast<uint8_t>(m_targetState); std::vector<uint8_t> d; alpaca::serialize(s, d); espp::EventManager::get().publish("lock/stateChanged", d); } }, 4096);
88-94: Unqualified enum values (LOCKED/INTERNAL/NFC) will not compile with scoped enumsThis file mixes
lockStates::LOCKEDwith bareLOCKED, and usesLockManager::INTERNALplusNFCwithout theSource::qualifier, while other code (e.g.,HomeKitLock.cpp) usesSource::NFC. Withenum classor namespaced enums this fails to compile.Align all uses with the defined enum scopes.
LockManager::LockManager(const espConfig::misc_config_t& miscConfig, const espConfig::actions_config_t& actionsConfig) : m_miscConfig(miscConfig), m_actionsConfig(actionsConfig), m_currentState(lockStates::LOCKED), m_targetState(lockStates::LOCKED) @@ m_currentState = s.currentState; s.currentState = m_currentState; s.targetState = m_targetState; - EventLockState s{ - .currentState = static_cast<uint8_t>(m_currentState), - .targetState = static_cast<uint8_t>(m_targetState), - .source = LockManager::INTERNAL - }; + EventLockState s{ + .currentState = static_cast<uint8_t>(m_currentState), + .targetState = static_cast<uint8_t>(m_targetState), + .source = Source::INTERNAL, + }; @@ void LockManager::begin() { @@ EventLockState s{ .currentState = static_cast<uint8_t>(m_currentState), .targetState = static_cast<uint8_t>(m_targetState), - .source = LockManager::INTERNAL + .source = Source::INTERNAL, }; @@ void LockManager::handleTimer(void* instance){ - static_cast<LockManager*>(instance)->setTargetState(LOCKED, INTERNAL); + static_cast<LockManager*>(instance)->setTargetState(lockStates::LOCKED, Source::INTERNAL); } @@ EventLockState s{ .currentState = m_currentState, .targetState = m_targetState, - .source = LockManager::INTERNAL + .source = Source::INTERNAL, }; @@ - } else if((source == NFC && m_actionsConfig.hkGpioControlledState) || source != NFC) { + } else if ((source == Source::NFC && m_actionsConfig.hkGpioControlledState) || source != Source::NFC) { @@ EventLockState s{ .currentState = static_cast<uint8_t>(m_currentState), .targetState = static_cast<uint8_t>(m_targetState), - .source = LockManager::INTERNAL + .source = Source::INTERNAL, };Also applies to: 96-107, 120-122, 167-171, 217-221
main/HomeKitLock.cpp (2)
55-60: Surface deserialization errors forhomekit/eventinstead of failing silentlyBoth
HomekitEventandEventValueChangeddeserialization paths simplyreturnon error, with no logging. This makes debugging malformed HomeKit events difficult.Consider logging failures (including
ec.message()andevent.typewhere available) before returning so you have observability into bad payloads.
54-75: Potential null dereference when processing battery‑related HomeKit eventsIn the
HomeKitLockconstructor’s"homekit/event"subscriber, theBTR_PROP_CHANGEDbranch unconditionally dereferencesm_statusLowBatteryandm_batteryLevel:if (s.name == "btrLevel") { updateBatteryStatus(s.newValue, m_statusLowBattery->getVal()); } else if (s.name == "btrLowThreshold") { updateBatteryStatus(m_batteryLevel->getVal(), s.newValue); }These members are only initialized when
proxBatEnabledis true (seebegin()andPhysicalLockBatteryService), so with battery service disabled they remain null and this path will crash.Guard the pointers (and/or reuse
updateBatteryStatusdefaults) before reading from them.- case HomekitEventType::BTR_PROP_CHANGED: - if(s.name == "btrLevel") { - updateBatteryStatus(s.newValue, m_statusLowBattery->getVal()); - } else if(s.name == "btrLowThreshold"){ - updateBatteryStatus(m_batteryLevel->getVal(), s.newValue); - } + case HomekitEventType::BTR_PROP_CHANGED: + if (s.name == "btrLevel") { + bool low = m_statusLowBattery ? static_cast<bool>(m_statusLowBattery->getVal()) : false; + updateBatteryStatus(static_cast<uint8_t>(s.newValue), low); + } else if (s.name == "btrLowThreshold") { + uint8_t level = m_batteryLevel ? static_cast<uint8_t>(m_batteryLevel->getVal()) : 0; + updateBatteryStatus(level, static_cast<bool>(s.newValue)); + } break;data/src/lib/services/ws.js (1)
82-120: Manual reconnects permanently disable auto‑reconnect
disconnect()flips_shouldReconnecttofalse, butconnect()never turns it back on. After using the UI to disconnect and thenconnect()again, future drops will not schedule_handleConnectionLoss().Set
_shouldReconnect = trueat the start ofconnect()so manual reconnects restore auto behavior./** * Establishes a WebSocket connection. * @param {string} [url] - The WebSocket URL to connect to. */ connect(url) { + // Manual reconnects should restore auto‑reconnect behavior + this._shouldReconnect = true; + if (this._lastState === 'connecting' || this._reconnectTimer) { console.log('Connection or reconnection attempt already in progress.'); return; @@ /** * Disconnects the WebSocket connection gracefully. */ disconnect() { console.log('Manual disconnection initiated.'); this._shouldReconnect = false;Also applies to: 122-136
main/CMakeLists.txt (1)
7-15: CI still skips filesystem image, and asset‑prune regex removes compressed assets
littlefs_create_partition_imageis inside theif(NOT DEFINED ENV{CI})block, so CI never builds the LittleFS image. Thefindregex also deletes.html.gz/.json.gzalongside uncompressed CSS/JS, which is the opposite of typical intent.Consider:
- Always creating the partition image (only gate the webui build on CI).
- Pruning only uncompressed
.html/.css/.js/.jsonand keeping precompressed*.gz/*.br.-if(NOT DEFINED ENV{CI}) - add_custom_target(webui - WORKING_DIRECTORY ${COMPONENT_DIR}/../data - COMMAND bun run build - COMMAND bash -c "find build -regex '.*\\.\\(html\\.gz\\|css\\|br\\|js\\|json\\.gz\\)' -delete" - VERBATIM - ) - littlefs_create_partition_image(spiffs ../data/build FLASH_IN_PROJECT DEPENDS webui) -endif() +if(NOT DEFINED ENV{CI}) + add_custom_target(webui + WORKING_DIRECTORY ${COMPONENT_DIR}/../data + COMMAND bun run build + # Remove only uncompressed assets; keep *.gz/*.br + COMMAND bash -c "find build -type f \\( -name '*.html' -o -name '*.css' -o -name '*.js' -o -name '*.json' \\) -delete" + VERBATIM + ) + littlefs_create_partition_image(spiffs ../data/build FLASH_IN_PROJECT DEPENDS webui) +else() + # CI: web UI is expected to be built by the pipeline; still package it. + littlefs_create_partition_image(spiffs ../data/build FLASH_IN_PROJECT) +endif()data/src/lib/types/api.ts (3)
130-146: NeoPixel color tuples should be RGB triplets, not 2‑element tuples
neopixelSuccessColor,neopixelFailureColor, andneopixelTagEventColorare documented as RGB values but typed as[number, number][]. This contradicts the intended 3‑channel color representation and will break any code indexing[r, g, b].Update them to 3‑element tuples.
/** RGB color values for successful NFC operations */ - neopixelSuccessColor: [number, number][]; + neopixelSuccessColor: [number, number, number][]; /** RGB color values for failed NFC operations */ - neopixelFailureColor: [number, number][]; + neopixelFailureColor: [number, number, number][]; @@ /** RGB color values for tag event LED animation */ - neopixelTagEventColor: [number, number][]; + neopixelTagEventColor: [number, number, number][];
119-123: RMII MDC pin name typo (pin_mcd→pin_mdc)The RMII config comment and
EthConfigtype usepin_mcdfor the MDC clock pin, which appears to be a typo. Earlier tooling already flagged this.Rename to
pin_mdcand ensure any backend JSON and UI usage are updated accordingly.- /** RMII configuration [phy_addr, pin_mcd, pin_mdio, pin_power, pin_rmii_clock] */ + /** RMII configuration [phy_addr, pin_mdc, pin_mdio, pin_power, pin_rmii_clock] */ ethRmiiConfig: [number, number, number, number, number]; @@ rmii_conf: { /** PHY address */ phy_addr: number; /** MDC pin number */ - pin_mcd: number; + pin_mdc: number;Also applies to: 197-233
413-420: Typo in ApiSuccess JSDocMinor doc issue: "Message decribing result" → "Message describing result".
/** The response data payload */ data: T; - /** Message decribing result */ + /** Message describing result */ message: string;main/HKServices.cpp (2)
11-15: Invalid numeric literal26CinCUSTOM_CHAR(HardwareFinish, …)
CUSTOM_CHAR(HardwareFinish, 26C, ...)uses26Cas a numeric literal, which is invalid C++ and will fail to compile. It should be a proper integer value for the HAP UUID, e.g.0x026C.-CUSTOM_CHAR(HardwareFinish, 26C, PR, TLV_ENC, NULL_TLV, NULL_TLV, NULL_TLV, true) +CUSTOM_CHAR(HardwareFinish, 0x026C, PR, TLV_ENC, NULL_TLV, NULL_TLV, NULL_TLV, true)
58-60: Make FirmwareRevision parsing robust to missing prefix/dashFirmware revision is currently derived via a chained
erase/find_first_of:std::string app_desc = esp_app_get_description()->version; new Characteristic::FirmwareRevision(app_desc.erase(app_desc.erase(0,1).find_first_of("-")).c_str());If the version string is empty, lacks the leading character, or has no
'-', this can invokeerasewithnposand throw. It’s also hard to read.Use explicit checks for leading
v/Vand'-'before erasing.- std::string app_desc = esp_app_get_description()->version; - new Characteristic::FirmwareRevision(app_desc.erase(app_desc.erase(0,1).find_first_of("-")).c_str()); + std::string ver = esp_app_get_description()->version; + // Strip optional leading 'v' and any suffix after the first '-' + if (!ver.empty() && (ver.front() == 'v' || ver.front() == 'V')) { + ver.erase(0, 1); + } + auto dash = ver.find('-'); + if (dash != std::string::npos) { + ver.erase(dash); + } + new Characteristic::FirmwareRevision(ver.c_str());main/ReaderDataManager.cpp (6)
126-135: Fix msgpack offset usage to avoid UB/crashLine 128 still passes NULL as the offset pointer to
msgpack_unpack_next, which is unsafe and can crash with some builds.Apply this diff:
msgpack_unpacked unpacked; msgpack_unpacked_init(&unpacked); - bool success = msgpack_unpack_next(&unpacked, (const char*)buffer.data(), buffer.size(), NULL); + size_t off = 0; + bool success = msgpack_unpack_next(&unpacked, (const char*)buffer.data(), buffer.size(), &off); if(success) {Based on past review comments.
192-209: Return status should reflect save outcomeLine 205 calls
saveData()but ignores its return value; lines 207-208 log success unconditionally even if the save failed.Apply this diff:
m_readerData.reader_sk = {}; ESP_LOGI(TAG, "In-memory reader key cleared."); - saveData(); - - ESP_LOGI(TAG, "Reader key successfully erased from NVS."); - return true; + if (saveData() == nullptr) { + ESP_LOGE(TAG, "Failed to persist erased reader key to NVS."); + return false; + } + ESP_LOGI(TAG, "Reader key successfully erased from NVS."); + return true;Based on past review comments.
288-290: Use msgpack_pack_uint8 for bytes (current symbol likely undefined)Lines 289, 308, 315, and 322 call
msgpack_pack_unsigned_charwhich is not provided by msgpack-c; the correct API ismsgpack_pack_uint8for unsigned 8-bit values.Apply this diff at each occurrence:
- std::ranges::for_each(endpoint.endpoint_id, [&pk](const auto&o){ - msgpack_pack_unsigned_char(pk, o); + std::ranges::for_each(endpoint.endpoint_id, [&pk](uint8_t o){ + msgpack_pack_uint8(pk, o); });Repeat for
endpoint_pk,endpoint_pk_x, andendpoint_prst_k. Based on past review comments.Also applies to: 307-309, 314-316, 321-323
363-368: Unpack accepts only positive ints but you pack signedLines 363 and 366 only check
MSGPACK_OBJECT_POSITIVE_INTEGERforcounterandkey_type, but the corresponding pack function (lines 298, 302) usesmsgpack_pack_int(signed).Apply this diff:
- if (obj_map.count("counter") && obj_map["counter"].type == MSGPACK_OBJECT_POSITIVE_INTEGER) { - endpoint.counter = obj_map["counter"].via.i64; + if (obj_map.count("counter") && (obj_map["counter"].type == MSGPACK_OBJECT_POSITIVE_INTEGER || obj_map["counter"].type == MSGPACK_OBJECT_NEGATIVE_INTEGER)) { + endpoint.counter = static_cast<int>(obj_map["counter"].via.i64); } - if (obj_map.count("key_type") && obj_map["key_type"].type == MSGPACK_OBJECT_POSITIVE_INTEGER) { - endpoint.key_type = obj_map["key_type"].via.i64; + if (obj_map.count("key_type") && (obj_map["key_type"].type == MSGPACK_OBJECT_POSITIVE_INTEGER || obj_map["key_type"].type == MSGPACK_OBJECT_NEGATIVE_INTEGER)) { + endpoint.key_type = static_cast<int>(obj_map["key_type"].via.i64); }Based on past review comments.
402-404: Same uint8 packer issue in hkIssuer packingLines 403, 410, and 417 use
msgpack_pack_unsigned_charwhich is not provided by msgpack-c. Replace withmsgpack_pack_uint8.Apply this diff:
- std::ranges::for_each(issuer.issuer_id, [&pk](const auto&o){ - msgpack_pack_unsigned_char(pk, o); + std::ranges::for_each(issuer.issuer_id, [&pk](uint8_t o){ + msgpack_pack_uint8(pk, o); });Repeat for
issuer_pkandissuer_pk_x. Based on past review comments.Also applies to: 409-411, 416-418
498-500: Same uint8 packer issue in readerData packingLines 499, 506, 513, 520, and 527 use
msgpack_pack_unsigned_charfor byte arrays. Replace withmsgpack_pack_uint8.Apply this diff for each field:
- std::ranges::for_each(reader_data.reader_sk, [&pk](const auto&o){ - msgpack_pack_unsigned_char(pk, o); + std::ranges::for_each(reader_data.reader_sk, [&pk](uint8_t o){ + msgpack_pack_uint8(pk, o); });Repeat for
reader_pk,reader_pk_x,reader_gid, andreader_id. Based on past review comments.Also applies to: 505-507, 512-514, 519-521, 526-528
main/HardwareManager.cpp (1)
463-464: Bug: stopping the wrong timer (success path).Line 464 checks if
m_pixelSuccessTimeris active but stopsm_gpioFailTimerinstead. This prevents the pixel success timer from being cleared and can leave the pixel stuck.Apply this diff:
if(esp_timer_is_active(m_gpioSuccessTimer)) esp_timer_stop(m_gpioSuccessTimer); - if(esp_timer_is_active(m_pixelSuccessTimer)) esp_timer_stop(m_gpioFailTimer); + if(esp_timer_is_active(m_pixelSuccessTimer)) esp_timer_stop(m_pixelSuccessTimer);Based on past review comments.
main/MqttManager.cpp (3)
77-84: Add EventManager subscriber cleanup to destructor.The destructor (lines 77-84) stops and destroys the MQTT client but doesn't remove the three EventManager subscribers registered at lines 35-74 ("lock/stateChanged", "lock/altAction", "nfc/event"). This leaves dangling subscribers.
Add before the closing brace:
espp::EventManager::get().remove_subscriber("lock/stateChanged", "MqttManager"); espp::EventManager::get().remove_subscriber("lock/altAction", "MqttManager"); espp::EventManager::get().remove_subscriber("nfc/event", "MqttManager");Based on past review comments.
464-475: Hex formatting drops leading bytes/doesn't format per-byte.Lines 466-468 use
fmt::format("{:02X}", fmt::join(...))which applies formatting to the entire joined string rather than each byte individually, producing incorrect hex output.Replace with a helper that formats each byte:
+ auto bytesToHex = [](const std::vector<uint8_t>& bytes) { + std::string out; + out.reserve(bytes.size() * 2); + for (auto b : bytes) fmt::format_to(std::back_inserter(out), "{:02X}", b); + return out; + }; cJSON *doc = cJSON_CreateObject(); - cJSON_AddStringToObject(doc, "issuerId", fmt::format("{:02X}", fmt::join(issuerId, "")).c_str()); - cJSON_AddStringToObject(doc, "endpointId", fmt::format("{:02X}", fmt::join(endpointId, "")).c_str()); - cJSON_AddStringToObject(doc, "readerId", fmt::format("{:02X}", fmt::join(readerId, "")).c_str()); + cJSON_AddStringToObject(doc, "issuerId", bytesToHex(issuerId).c_str()); + cJSON_AddStringToObject(doc, "endpointId", bytesToHex(endpointId).c_str()); + cJSON_AddStringToObject(doc, "readerId", bytesToHex(readerId).c_str());Also apply to
publishUidTap(lines 493-496). Based on past review comments.
597-604: Client certificates should be optional.Lines 601-603 return
falseif client certificate or key is missing, preventing standard server-side TLS (where only the server has a certificate). Most MQTT brokers don't require mutual TLS.Apply this diff:
if (!m_mqttSslConfig->clientCert.empty() && !m_mqttSslConfig->clientKey.empty()) { mqtt_cfg.credentials.authentication.certificate = m_mqttSslConfig->clientCert.c_str(); mqtt_cfg.credentials.authentication.key = m_mqttSslConfig->clientKey.c_str(); - ESP_LOGI(TAG, "TLS authentication configured"); + ESP_LOGI(TAG, "Mutual TLS authentication configured"); + } else if (!m_mqttSslConfig->clientCert.empty() || !m_mqttSslConfig->clientKey.empty()) { + ESP_LOGW(TAG, "Client certificate or key is incomplete - mutual TLS disabled"); } else { - ESP_LOGI(TAG, "No client certificate and/or private key configured"); - return false; + ESP_LOGI(TAG, "No client certificate configured - using server-side TLS only"); }Based on past review comments.
main/ConfigManager.cpp (2)
1-19: Add missing standard headers.The file uses
std::span(line 477),std::remove_if(line 1244), and ctype functions (isspace,isalnumat lines 1244, 1263) but doesn't include the required headers.Apply this diff:
#include "ConfigManager.hpp" #include "cJSON.h" #include "config.hpp" +#include <algorithm> +#include <cctype> #include <ranges> +#include <span> #include <string>Based on past review comments.
1244-1244: Fix unsafe ctype function calls with signed char arguments.Lines 1244 and 1263 use ctype functions directly with
chararguments, causing undefined behavior whencharis signed and contains negative values.Apply these diffs:
Line 1244:
- base64Content.erase(std::remove_if(base64Content.begin(), base64Content.end(), ::isspace), base64Content.end()); + base64Content.erase(std::remove_if(base64Content.begin(), base64Content.end(), + [](char ch){ return std::isspace(static_cast<unsigned char>(ch)); }), base64Content.end());Line 1263:
- } else if (!isalnum(c) && c != '+' && c != '/') { + } else if (!std::isalnum(static_cast<unsigned char>(c)) && c != '+' && c != '/') {Based on past review comments.
Also applies to: 1263-1263
| @@ -0,0 +1 @@ | |||
| engine-strict=true | |||
There was a problem hiding this comment.
🧩 Analysis chain
Good practice: enforce strict engine checking.
Enabling engine-strict=true ensures all developers and CI use a compatible Node.js version, preventing environment-specific issues during the Svelte 5 frontend refactor.
Please verify that data/package.json defines a compatible Node.js version in the engines field. For reference, run:
🏁 Script executed:
#!/bin/bash
# Verify engines field in data/package.json
cat data/package.json | jq '.engines'Length of output: 4
The verification reveals a critical issue: data/package.json does not define an engines field, making engine-strict=true non-functional. Let me gather more information about the package.json structure.
🏁 Script executed:
#!/bin/bash
# Check full structure of data/package.json
cat data/package.json | jq '.' | head -30Length of output: 955
🏁 Script executed:
#!/bin/bash
# Check if root package.json has engines defined
if [ -f package.json ]; then
echo "=== Root package.json engines field ==="
cat package.json | jq '.engines'
fiLength of output: 0
Let me check the monorepo structure and whether a root package.json exists:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all package.json files
echo "=== All package.json files ==="
find . -maxdepth 2 -name "package.json" -type f 2>/dev/null | sort
# Check if root package.json exists
if [ -f ./package.json ]; then
echo ""
echo "=== Root package.json engines field ==="
cat ./package.json | jq '.engines // "not defined"'
else
echo ""
echo "No root package.json found"
fi
Length of output: 78
Add engines field to data/package.json or remove engine-strict=true from .npmrc.
The engine-strict=true setting in data/.npmrc requires an engines field in data/package.json to function. Currently, data/package.json does not define engines, making the strict engine checking non-functional.
Add to data/package.json:
"engines": {
"node": ">=X.Y.Z"
}Replace X.Y.Z with the minimum Node.js version required for Svelte 5 and your dependencies (e.g., >=18.0.0).
🤖 Prompt for AI Agents
In data/.npmrc around line 1, engine-strict=true is enabled but
data/package.json has no engines field, so strict engine checking is
ineffective; either add an "engines" entry to data/package.json (e.g., "node":
">=18.0.0" or whatever minimum Node version your project requires for Svelte 5
and dependencies) or remove engine-strict=true from data/.npmrc; update
data/package.json to include the appropriate engines object with the chosen node
version and commit, or delete the line from data/.npmrc if you don't want engine
enforcement.
| let { | ||
| actions, | ||
| error, | ||
| }: { actions: ActionsConfig | null; error?: string | null } = $props(); | ||
|
|
||
| let actionsConfig = $state<ActionsConfig>( | ||
| actions ?? { | ||
| neopixelSuccessColor: [ |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid aliasing actions into actionsConfig and fix Reset behavior
When actions is non-null, actionsConfig is initialized to the same object:
let actionsConfig = $state<ActionsConfig>(actions ?? { ...defaults... });This means:
- All
bind:value={actionsConfig.*}mutations also modifyactions, sodiff(actions, actionsConfig)often sees no changes. resetForm = () => { actionsConfig = actions!; }just reassigns the already-mutated object; Reset doesn’t actually roll back.
Suggested pattern:
- let actionsConfig = $state<ActionsConfig>(
- actions ?? {
- // defaults...
- },
- );
+ const defaultActions: ActionsConfig = {
+ // existing defaults...
+ };
+
+ let initialActions = $state<ActionsConfig>(
+ actions ? structuredClone(actions) : defaultActions,
+ );
+
+ let actionsConfig = $state<ActionsConfig>(structuredClone(initialActions));
@@
- const saveActionsConfig = async (e: any): Promise<void> => {
+ const saveActionsConfig = async (e: SubmitEvent): Promise<void> => {
@@
- if (!actionsConfig || !actions) return;
- const result = await saveConfig("actions", diff(actions, actionsConfig));
+ const result = await saveConfig(
+ "actions",
+ diff(initialActions, actionsConfig),
+ );
if (result.success) {
- actionsConfig = result.data;
- actions = result.data;
+ initialActions = structuredClone(result.data);
+ actionsConfig = structuredClone(result.data);
+ actions = result.data;
}
@@
- const resetForm = (): void => {
- actionsConfig = actions!;
- };
+ const resetForm = (): void => {
+ actionsConfig = structuredClone(initialActions);
+ };This keeps an immutable snapshot for diffing and Reset while allowing the form to edit a separate working copy.
Also applies to: 41-56, 58-76
🌐 Web query:
In Svelte (with TypeScript), does mutating a prop-derived object used both as "original" and "edited" state break diff-based save logic (e.g., diff(old, new)) as described here?
💡 Result:
Short answer: Yes — if your "original" and "edited" vars reference the same object, in-place mutations will make them identical by reference and your diff(old, new) will miss changes (or your reactive reads won't update) — so the save/diff logic breaks.
Why (brief):
- JS objects are reference-shared; mutating a prop-derived object changes the original reference the parent provided. [1]
- Svelte's reactivity/derived values rely on assignments and referential differences; mutating nested properties without creating a new object can skip updates and cause derived values to appear unchanged. [2][3]
Fixes:
- Clone the prop before editing (structuredClone, JSON, deep-clone, or shallow spread for top-level) so old and edited are different objects.
- Make edits by replacing the object (obj = {...obj, changed: val}) so Svelte sees the assignment.
- Or keep an immutable “original” copy and a separate “edited” copy for diffing.
Sources:
[1] Svelte docs — derived and update propagation. (svelte.dev)
[2] Practical note on mutating objects and needing reassignment in Svelte. (dev.to)
[3] Svelte 5 migration/reactivity notes (assignments, $state/$derived). (svelte.jp)
Citations:
- 1: https://svelte.dev/docs/svelte/%24derived?utm_source=openai
- 2: https://dev.to/jdelvx/svelte-from-a-react-developer-s-perspective-reactivity-props-4ike?utm_source=openai
- 3: https://svelte.jp/docs/svelte/v5-migration-guide?utm_source=openai
Clone actions to prevent mutations from breaking diff and Reset logic
When actions is non-null, actionsConfig is initialized to the same object. This causes all form mutations to also modify actions, breaking the diff-based save and Reset:
diff(actions, actionsConfig)compares the same object (no changes detected)resetFormreassigns the already-mutated object (cannot revert)
Use structuredClone to maintain an immutable snapshot for diffing and Reset:
+ const defaultActions: ActionsConfig = {
+ // existing defaults...
+ };
+
+ let initialActions = $state<ActionsConfig>(
+ actions ? structuredClone(actions) : defaultActions,
+ );
+
- let actionsConfig = $state<ActionsConfig>(
- actions ?? {
- // defaults...
- },
- );
+ let actionsConfig = $state<ActionsConfig>(structuredClone(initialActions));
@@
- const saveActionsConfig = async (e: any): Promise<void> => {
+ const saveActionsConfig = async (e: SubmitEvent): Promise<void> => {
@@
- if (!actionsConfig || !actions) return;
- const result = await saveConfig("actions", diff(actions, actionsConfig));
+ const result = await saveConfig(
+ "actions",
+ diff(initialActions, actionsConfig),
+ );
if (result.success) {
- actionsConfig = result.data;
- actions = result.data;
+ initialActions = structuredClone(result.data);
+ actionsConfig = structuredClone(result.data);
+ actions = result.data;
}
@@
- const resetForm = (): void => {
- actionsConfig = actions!;
- };
+ const resetForm = (): void => {
+ actionsConfig = structuredClone(initialActions);
+ };Also applies to: lines 41–56, 58–76
🤖 Prompt for AI Agents
In data/src/lib/components/AppActions.svelte around lines 6 to 13 (and also
apply the same change at lines 41–56 and 58–76), the component assigns
actionsConfig to the same object reference as props.actions which allows
in-place mutations to alter the original props and break diff-based save and
Reset logic; fix by creating a deep clone of actions when initializing
actionsConfig (use structuredClone(actions) when actions is non-null, otherwise
keep the default literal), and do the same wherever actionsConfig is initialized
or re-assigned so diff(actions, actionsConfig) compares different objects and
resetForm can restore the original snapshot.
| let currentTime = $state(new Date()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Use browser-safe timeout types and initialize resizeObserver defensively
NodeJS.Timeout isn’t ideal in browser code and resizeObserver is declared but not initialized as nullable, yet checked in onDestroy.
Recommend:
- let currentTime = $state(new Date());
+ let currentTime = $state(new Date());
@@
- let timeUpdateInterval = $state<NodeJS.Timeout>();
- var resizeObserver : ResizeObserver;
+ let timeUpdateInterval = $state<ReturnType<typeof setInterval> | null>(null);
+ let resizeObserver: ResizeObserver | null = null;
@@
- onDestroy(() => {
- if (timeUpdateInterval) {
- clearInterval(timeUpdateInterval);
- }
+ onDestroy(() => {
+ if (timeUpdateInterval) {
+ clearInterval(timeUpdateInterval);
+ timeUpdateInterval = null;
+ }
@@
- if (resizeObserver) {
- resizeObserver.disconnect();
- }
+ if (resizeObserver) {
+ resizeObserver.disconnect();
+ resizeObserver = null;
+ }This avoids Node.js typings in the browser and matches the nullable runtime checks.
Also applies to: 149-151, 224-234
🌐 Web query:
In a Svelte (browser) project, is `ReturnType<typeof setTimeout>` the recommended type for timer handles instead of `NodeJS.Timeout`?
💡 Result:
Short answer: Yes — using ReturnType is the safest/recommended choice for a browser Svelte project if you want a portable, correct type. It matches whatever the host environment's declaration is (number in the DOM, NodeJS.Timeout in Node), avoiding errors when @types/node leaks into your build. If you are absolutely certain your compile environment uses only the DOM lib, window.setTimeout() (or number) is fine; otherwise prefer ReturnType (or window.setTimeout for browser-only code). [1][2][3]
Sources:
- Stack Overflow discussion (ReturnType solution and browser vs Node differences). [1]
- Explanation and examples recommending ReturnType to avoid @types/node leakage. [2]
- Practical guide describing ReturnType workaround and window.setTimeout alternative. [3]
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the LogViewer.svelte file at the specified lines
fd -n LogViewer.svelte --type f --exec cat {} \; | head -250 | tail -220
Length of output: 230
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display LogViewer.svelte
fd LogViewer.svelte --type f -x cat {}
Length of output: 13605
Fix timer type and initialize resizeObserver with proper null handling
Verified: The code uses NodeJS.Timeout in a browser Svelte project (lines 149) and declares resizeObserver without initialization or nullable type annotation (line 150). Web search confirms ReturnType<typeof setInterval> is the recommended approach for browser code to avoid Node.js type leakage.
- let timeUpdateInterval = $state<NodeJS.Timeout>();
- var resizeObserver : ResizeObserver;
+ let timeUpdateInterval = $state<ReturnType<typeof setInterval> | null>(null);
+ let resizeObserver: ResizeObserver | null = null;
@@
- onDestroy(() => {
- if (timeUpdateInterval) {
- clearInterval(timeUpdateInterval);
- }
+ onDestroy(() => {
+ if (timeUpdateInterval) {
+ clearInterval(timeUpdateInterval);
+ timeUpdateInterval = null;
+ }
@@
- if (resizeObserver) {
- resizeObserver.disconnect();
- }
+ if (resizeObserver) {
+ resizeObserver.disconnect();
+ resizeObserver = null;
+ }Also applies to: 149-151, 224-234
🤖 Prompt for AI Agents
In data/src/lib/components/LogViewer.svelte around lines 31-32 and also apply to
149-151 and 224-234: the timer and resizeObserver types are incorrect for
browser Svelte code—replace any NodeJS.Timeout annotations with
ReturnType<typeof setInterval> for browser-safe typing, declare and initialize
resizeObserver as ResizeObserver | null = null, and update any code that
sets/clears the interval or uses the observer to handle the nullable type (check
for null before calling .disconnect() or .observe()). Ensure interval variables
are assigned from setInterval and cleared with clearInterval, and add proper
null checks where resizeObserver is used.
| let timeUpdateInterval = $state<NodeJS.Timeout>(); | ||
| var resizeObserver : ResizeObserver; | ||
|
|
||
| onMount(() => { | ||
| const savedLogLevels = localStorage.getItem("logviewer-levels"); | ||
| if (savedLogLevels) { | ||
| logLevels = JSON.parse(savedLogLevels); | ||
| } | ||
| timeUpdateInterval = setInterval(() => { | ||
| currentTime = new Date(); | ||
| }, 1000); | ||
| lastScrollTop = | ||
| document.querySelector("#virtual-list-viewport")?.scrollTop || 0; | ||
| const contentList = document.getElementById("virtual-list-content"); | ||
| if (contentList) { | ||
| resizeObserver = new ResizeObserver(() => { | ||
| atBottom = | ||
| document.querySelector("#virtual-list-viewport")! | ||
| .scrollTop >= | ||
| document | ||
| .querySelector("#virtual-list-content")! | ||
| .getBoundingClientRect().height - | ||
| document | ||
| .querySelector("#virtual-list-container")! | ||
| .getBoundingClientRect().height; | ||
| }); | ||
| resizeObserver.observe(contentList); | ||
| } | ||
| document | ||
| .querySelector("#virtual-list-viewport") | ||
| ?.addEventListener("scroll", (e) => { | ||
| const target = e.target as HTMLDivElement; | ||
| const scrollTop = target.scrollTop; | ||
| const contentHeight = document | ||
| .querySelector("#virtual-list-content")! | ||
| .getBoundingClientRect().height; | ||
| const containerHeight = document | ||
| .querySelector("#virtual-list-container")! | ||
| .getBoundingClientRect().height; | ||
| atBottom = scrollTop >= contentHeight - containerHeight; | ||
| const scrollDelta = lastScrollTop - scrollTop; | ||
| const currentDirection = | ||
| scrollDelta > 0 ? 1 : scrollDelta < 0 ? -1 : 0; | ||
|
|
||
| if ( | ||
| currentDirection !== 0 && | ||
| currentDirection !== lastScrollDirection | ||
| ) { | ||
| cumulativeScrollDistance = 0; | ||
| lastScrollDirection = currentDirection; | ||
| } | ||
|
|
||
| if (currentDirection !== 0) { | ||
| cumulativeScrollDistance += Math.abs(scrollDelta); | ||
| } | ||
|
|
||
| if (cumulativeScrollDistance > SCROLL_THRESHOLD) { | ||
| if (scrollDebounceTimer) { | ||
| clearTimeout(scrollDebounceTimer); | ||
| } | ||
|
|
||
| scrollDebounceTimer = setTimeout(() => { | ||
| if (currentDirection === 1) { | ||
| scrollDirection = 1; // Scrolling up | ||
| } else if (currentDirection === -1) { | ||
| scrollDirection = 0; // Scrolling down | ||
| } | ||
| cumulativeScrollDistance = 0; | ||
| }, 30); | ||
| } | ||
|
|
||
| lastScrollTop = scrollTop; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid brittle global document.querySelector lookups and missing IDs
The scroll/resize logic relies on document.querySelector("#virtual-list-viewport"), #virtual-list-content, and #virtual-list-container, all asserted non-null with !, but this component’s template does not define elements with those IDs. If the virtual list implementation or surrounding layout does not provide matching IDs, these non-null assertions will throw at runtime.
Safer approaches:
- Attach
bind:thisrefs to the actual container/viewport elements you control and use those instead of global selectors. - If you must query by selector, remove the
!and handlenullcases gracefully before readingscrollToporgetBoundingClientRect(). - Store the scroll handler function in a variable and remove it in
onDestroywithremoveEventListenerto avoid leaks.
At minimum, please ensure these IDs are actually present in the rendered DOM and guard against null before dereferencing.
🌐 Web query:
Does `@humanspeak/svelte-virtual-list` expose fixed IDs like `#virtual-list-viewport` and `#virtual-list-content`, or should consumers manage their own container refs?
💡 Result:
Short answer: No — it doesn't expose fixed DOM IDs. The package exposes class/test-id props (containerClass, viewportClass, contentClass, itemsClass, testId) so you can style or target elements, and consumers should provide/manage their own refs if they need direct element references. [1][2]
Sources:
- package README / npm (props: containerClass, viewportClass, contentClass, itemsClass, testId). [1]
- older npm/version listing showing same props and usage. [2]
The virtual list library does not expose fixed DOM IDs—these selectors will fail at runtime
The @humanspeak/svelte-virtual-list package does not expose fixed DOM IDs like #virtual-list-viewport and #virtual-list-content. Instead, it provides class and testId props (containerClass, viewportClass, contentClass, itemsClass, testId), and consumers must manage their own refs if they need direct element references.
The current code assumes these IDs exist and uses non-null assertions (!), which will throw at runtime when querySelector returns null.
Required fixes:
- Remove all
document.querySelectorlookups for these IDs or guard each with null checks before dereferencing - Use
bind:thisto capture actual viewport/content/container element refs from the template - Reference those refs directly instead of relying on global selectors
- Add the
onDestroycleanup to remove the scroll event listener
| let statusColor = $derived((): string => { | ||
| switch (websocketState.state) { | ||
| case "open": | ||
| return "text-success"; | ||
| case "disconnected": | ||
| return "text-error"; | ||
| case "failed": | ||
| return "text-error"; | ||
| case "error": | ||
| return "text-warning"; | ||
| case "connecting": | ||
| return "text-info"; | ||
| default: | ||
| return "text-base-content"; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Make statusColor a derived string, not a function.
statusColor is currently a $derived of an arrow function, so it holds the function itself. Using {statusColor} in the class attribute will yield a function string instead of "text-success", "text-error", etc.
Refactor statusColor to derive the class string directly:
-let statusColor = $derived((): string => {
- switch (websocketState.state) {
- case "open":
- return "text-success";
- case "disconnected":
- return "text-error";
- case "failed":
- return "text-error";
- case "error":
- return "text-warning";
- case "connecting":
- return "text-info";
- default:
- return "text-base-content";
- }
-});
+let statusColor = $derived(
+ websocketState.state === "open"
+ ? "text-success"
+ : websocketState.state === "disconnected" ||
+ websocketState.state === "failed"
+ ? "text-error"
+ : websocketState.state === "error"
+ ? "text-warning"
+ : websocketState.state === "connecting"
+ ? "text-info"
+ : "text-base-content",
+);The template can then keep:
<div class="stat-value text-sm {statusColor}">
{websocketState.state}
</div>Also applies to: 291-299
I need to verify the Svelte 5 $derived behavior and examine the actual file usage.<!-- [scratchpad_update]
[task review_validation_1/2 verify_derived_behavior]
[observations]
- Web search confirms: "$derived(expression)" evaluates the expression and returns its value
- Per Svelte docs 1-5: "$derived(expression) is equivalent to $derived.by(() => expression)"
- This means if you write
$derived((): string => { ... }), you're passing a function definition as the expression - The result would be the function object itself, NOT the return value of calling the function
[analysis] - Review is CORRECT:
statusColor = $derived((): string => { ... })does make statusColor a function - The fix should wrap the switch statement properly so
$derivedreceives the return value, not the function itself - Example fix: either
$derived.by(() => { switch ... })OR remove the arrow function syntax entirely
[pending] - Verify actual file usage in template and confirm lines 291-299 issue
[actions] - Run script to examine WebSocketTest.svelte file
[scratchpad_end] -->
Now let me examine the actual file to verify the template usage and the issue at lines 291-299:
<!-- [scratchpad_update]
[task review_validation_1/2 verify_derived_behavior_file_check]
[observations]
- Need to locate WebSocketTest.svelte file in repo first
[actions]
- Run script to find and examine WebSocketTest.svelte
[scratchpad_end] -->
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the WebSocketTest.svelte file to verify the issue
fd -e svelte -n WebSocketTest | head -1 | xargs cat
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for WebSocketTest.svelte in the repository
fd -type f -name 'WebSocketTest.svelte' 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find and examine WebSocketTest.svelte
fd -type f -name 'WebSocketTest.svelte' | head -1 | xargs catLength of output: 170
Let me verify the Svelte 5 $derived behavior and try a different approach to locate the file:
🏁 Script executed:
#!/bin/bash
# Search for statusColor in the codebase to locate the file
rg -l "statusColor" --type svelteLength of output: 30
🌐 Web query:
Svelte 5 $derived rune arrow function behavior
💡 Result:
Short answer: $derived expects an expression (not a callback). If you pass an arrow function itself you get that function as the derived value (it won't be auto-invoked or tracked). For multiline or callback-style computations use $derived.by (or the documented helper) so Svelte will call the function and track dependencies. Also: derived expressions must be free of side effects (you cannot mutate $state inside them).
Examples:
- Correct (expression): let doubled = $derived(count * 2). [1][2]
- Wrong if you expect it to compute: let doubled = $derived(() => count * 2) — this sets doubled to a function. Use $derived.by(() => { ...; return value; }) for callback-style logic. [1][4]
- Note: side effects inside derived expressions are disallowed. [1]
Sources: Svelte docs (/$derived) and Svelte runes announcement; GitHub discussion about callback-style $derived. [1][2][4]
🏁 Script executed:
#!/bin/bash
# Search for statusColor without type filter
rg "statusColor" -C2 | head -50Length of output: 759
🏁 Script executed:
#!/bin/bash
# Search in data/src directory specifically
find data/src -name "WebSocketTest.svelte" 2>/dev/nullLength of output: 44
---
🏁 Script executed:
```shell
#!/bin/bash
# View lines 291-299 of WebSocketTest.svelte
sed -n '285,305p' data/src/lib/components/WebSocketTest.svelte
Length of output: 687
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all $derived declarations in WebSocketTest.svelte to identify the second issue
rg '\$derived' data/src/lib/components/WebSocketTest.svelte -n -A5
Length of output: 187
---
🏁 Script executed:
```shell
#!/bin/bash
# Get all $derived declarations in the file with line numbers
rg '\$derived' data/src/lib/components/WebSocketTest.svelte -n
Length of output: 46
Make statusColor a derived string, not a function.
statusColor is currently a $derived of an arrow function, so it holds the function itself. Using {statusColor} in the class attribute will yield a function string instead of "text-success", "text-error", etc.
Refactor statusColor to derive the class string directly:
-let statusColor = $derived((): string => {
- switch (websocketState.state) {
- case "open":
- return "text-success";
- case "disconnected":
- return "text-error";
- case "failed":
- return "text-error";
- case "error":
- return "text-warning";
- case "connecting":
- return "text-info";
- default:
- return "text-base-content";
- }
-});
+let statusColor = $derived.by(() => {
+ switch (websocketState.state) {
+ case "open":
+ return "text-success";
+ case "disconnected":
+ return "text-error";
+ case "failed":
+ return "text-error";
+ case "error":
+ return "text-warning";
+ case "connecting":
+ return "text-info";
+ default:
+ return "text-base-content";
+ }
+});This ensures statusColor evaluates to the returned string value, not the function itself. The template at line 294 will then correctly interpolate the class string.
📝 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.
| let statusColor = $derived((): string => { | |
| switch (websocketState.state) { | |
| case "open": | |
| return "text-success"; | |
| case "disconnected": | |
| return "text-error"; | |
| case "failed": | |
| return "text-error"; | |
| case "error": | |
| return "text-warning"; | |
| case "connecting": | |
| return "text-info"; | |
| default: | |
| return "text-base-content"; | |
| } | |
| }); | |
| let statusColor = $derived.by(() => { | |
| switch (websocketState.state) { | |
| case "open": | |
| return "text-success"; | |
| case "disconnected": | |
| return "text-error"; | |
| case "failed": | |
| return "text-error"; | |
| case "error": | |
| return "text-warning"; | |
| case "connecting": | |
| return "text-info"; | |
| default: | |
| return "text-base-content"; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In data/src/lib/components/WebSocketTest.svelte around lines 29–44, statusColor
is currently assigned using $derived of an arrow function which stores the
function itself instead of a derived string; replace this with a proper derived
store that takes the websocketState store as input (e.g. derived(websocketState,
ws => { switch (ws.state) { ... } })) so the derived returns the class string
directly, remove the leading $ when calling derived, and ensure derived is
imported from 'svelte/store' and the result is typed/returned as a string.
| ## 1. Overview | ||
|
|
||
| The `HomeKitLock` class serves as the central bridge between the application's core logic and the Apple HomeKit ecosystem, facilitated by the HomeSpan library. It is designed as a singleton and is responsible for initializing the HomeKit accessory, defining its services and characteristics, managing the network connection (Wi-Fi or Ethernet), and synchronizing the lock's state with HomeKit. | ||
|
|
||
| A critical function of this class is managing the lifecycle of HomeKey issuers. It listens for changes in paired HomeKit controllers and automatically updates the `ReaderDataManager` with the necessary cryptographic keys (LTPK), ensuring that newly paired devices can use HomeKey. | ||
|
|
||
| ### Key Responsibilities: | ||
|
|
||
| * **HomeSpan Initialization:** Configures and starts the HomeSpan framework, setting up the device as a HomeKit lock accessory. | ||
| * **Accessory Definition:** Creates all necessary HomeKit services, including the Lock Mechanism, Lock Management, NFC Access, and an optional Battery Service. | ||
| * **State Synchronization:** Subscribes to internal application events to update the lock's current state, target state, and battery status in HomeKit in real-time. | ||
| * **Network Management:** Handles the initialization of an Ethernet connection if configured, with a fallback to Wi-Fi managed by HomeSpan. | ||
| * **HomeKey Issuer Management:** Automatically synchronizes the list of trusted HomeKey issuers with the list of paired HomeKit admin controllers. | ||
| * **Debug Interface:** Provides a set of serial commands for runtime debugging and diagnostics. | ||
|
|
||
| ## 2. Public API |
There was a problem hiding this comment.
🧩 Analysis chain
Docs look consistent; just re-run markdownlint to confirm
The HomeKitLock API doc reads clearly and headings/sections look structurally sound in this version. The earlier markdownlint hints (MD023/MD036/MD051) don’t obviously match the current content, so they may be stale.
I’d just re-run markdownlint-cli2 locally or rely on CI; if it still flags specific lines, we can adjust those headings or any implicit “emphasis as heading” it calls out, but from this snippet the structure is fine.
Also applies to: 44-50, 115-132
🌐 Web query:
What exact rules and sample corrections does markdownlint-cli2 suggest for MD023, MD036, and MD051 on documents similar to this HomeKitLock API doc?
💡 Result:
Here are the exact rule summaries plus the typical failing and corrected examples markdownlint/markdownlint-cli2 reports for MD023, MD036 and MD051 (suitable for API docs like a HomeKitLock spec).
- MD023 — “Headings must start at the beginning of the line”
- What it checks: any heading line that is indented (leading space(s)) is flagged because many parsers will not treat it as a heading.
- Failing example:
(indented heading)
# Indented heading - Corrected example:
(heading at line start)Indented heading
- Autofix: tooling removes the leading spaces so the heading starts at column 1.
- Sources: markdownlint heading rules / docs. [1][2]
- MD036 — “Emphasis used instead of a heading”
- What it checks: single-line paragraphs made up entirely of emphasized text (bold/italic) used as section separators — these should be real headings instead. The rule ignores emphasized text that ends with punctuation (configurable).
- Failing example:
My document
Lorem ipsum…
Another section - Corrected example:
My document
Lorem ipsum…Another section
- Autofix: not always automatic — the suggested correction is to replace the emphasized paragraph with an appropriate ATX/SetExt heading. (Rule has a punctuation parameter to allow trailing punctuation.)
- Sources: markdownlint rule docs. [3]
- MD051 — “Link fragments should be valid” (aka link-fragments)
- What it checks: internal link fragments (#[anchor]) must match a heading anchor target in the same document (or valid HTML id anchors). Broken/changed headings that no longer produce the linked fragment are flagged.
- Failing example:
See the section ← no matching heading in the doc - Corrected examples:
- Update link to match existing heading: See the section
- Or add the matching heading: ## Non-existent heading
- Or replace with a full URL if the target is in another file.
- Autofix: not generally auto-fixed; the checklist is to either change the link text/fragment to match a real heading or restore the heading/HTML anchor. Recent markdownlint improvements expanded HTML anchor support and matching behavior.
- Sources: markdownlint rule list and changelog/issue notes about MD051 improvements. [4][5]
If you want, I can run markdownlint-cli2 --fix (or a dry run) on your actual HomeKitLock API Markdown file and show the exact violations plus suggested edits (I’ll include the precise lines and proposed replacements).
I have the markdownlint-cli2 rule definitions now. To verify whether the original review comment is accurate, I need to inspect the actual HomeKitLock.md file at the line ranges mentioned (especially 44-50 and 115-132 where issues were flagged).
🏁 Script executed:
#!/bin/bash
# Retrieve the HomeKitLock.md file and examine the specified line ranges
fd -t f "HomeKitLock.md" | head -1 | xargs -I {} sh -c '
echo "=== Lines 5-20 ==="
sed -n "5,20p" "$1"
echo ""
echo "=== Lines 44-50 ==="
sed -n "44,50p" "$1"
echo ""
echo "=== Lines 115-132 ==="
sed -n "115,132p" "$1"
' _ {}Length of output: 3041
The file content reveals a real violation. Let me scan for all standalone bold/italic lines that would trigger MD036:
🏁 Script executed:
#!/bin/bash
# Find all standalone bold/italic lines that could be MD036 violations
# (lines that are entirely or primarily bold/italic, not within lists or inline)
fd -t f "HomeKitLock.md" | head -1 | xargs -I {} sh -c '
echo "=== Checking for standalone bold/italic lines (MD036 candidates) ==="
rg -n "^\s*\*\*[^*]+\*\*\s*$|^\s*__[^_]+__\s*$|^\s*_[^_]+_\s*$|^\s*\*[^*]+\*\s*$" "$1"
' _ {}Length of output: 280
Now let me check for MD051 violations (broken link fragments):
🏁 Script executed:
#!/bin/bash
# Find internal markdown links with fragments to check for MD051 violations
fd -t f "HomeKitLock.md" | head -1 | xargs -I {} sh -c '
echo "=== Checking for internal links with fragments (MD051 candidates) ==="
rg -n "\[.*\]\(#[^)]+\)" "$1"
' _ {}Length of output: 69
Convert bold section labels to proper heading syntax
Verification confirms MD036 violations at lines 28, 38, 50, 58, 69, 74, 82, 87, 97, and 106. These bold lines (**Signature:**, **Parameters:**, etc.) are being used as section separators and must be converted to proper markdown heading syntax (### or ####) instead. MD023 and MD051 are not violated.
Replace each standalone bold line with an appropriate heading level. For example:
**Signature:**→### Signature**Parameters:**→### Parameters**It performs the following key actions:**→### It performs the following key actions
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
12-12: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
In docs/content/API/HomeKitLock.md around lines 5 to 20, standalone bold section
labels (e.g., **Signature:**, **Parameters:**, **It performs the following key
actions:**) must be converted to proper Markdown headings to address MD036;
replace each bold-only line with an appropriate heading (for example convert
`**Signature:**` and `**Parameters:**` to `### Signature` / `### Parameters`,
and convert phrase labels like `**It performs the following key actions:**` to
`### It performs the following key actions` or a lower-level heading if nested),
ensure you remove the surrounding asterisks and trailing colon and keep the rest
of the text unchanged, preserving indentation and any following content.
| | Topic | Description | Payload Examples | | ||
| | :---- | :---------- | :--------------- | | ||
| | `<CLIENT_ID>/homekit/state` | **Publishes** the current state of the lock. This topic is retained, meaning new subscribers will immediately receive the last known state. | `0` (UNLOCKED), `1` (LOCKED), `2` (JAMMED), `3` (UNKNOWN), `4` (UNLOCKING), `5` (LOCKING) | | ||
| | `<CLIENT_ID>/homekit/set_state` | **Subscribes** to this topic to set both the current and target state of the lock. | To unlock: `0` To lock: `1` | | ||
| | `<CLIENT_ID>/homekit/set_current_state` | **Subscribes** to this topic to set the current state of the lock. Useful for external systems to update the device's perceived state. | `0` (UNLOCKED), `1` (LOCKED), `2` (JAMMED), `3` (UNKNOWN) | | ||
| | `<CLIENT_ID>/homekit/set_target_state` | **Subscribes** to this topic to set the target state of the lock. | To unlock: `0` To lock: `1` | | ||
| | `<CLIENT_ID>/homekit/set_battery_lvl` | **Subscribes** to this topic to set the battery level to be shown in HomeKit. This is used if "Proxy Battery Enabled" is active in the WebUI. | `85` (for 85% battery) | | ||
| | `<CLIENT_ID>/alt_action` | **Publishes** the status of the Alternate Action. | (Specific payload depends on configuration) | | ||
|
|
||
| > [!NOTE] | ||
| > You will notice all topics set by default are prefixed by the client id, this was done so the topics are nicely organized under a unique identifier, however, you can set the topics to whatever you wish from the [WebUI](../configuration#core-topics) | ||
|
|
||
| **Example: Manually Locking Your Door via MQTT** | ||
|
|
||
| Let's say you want to lock your door using a command-line MQTT client like `mosquitto_pub`. | ||
|
|
||
| ```bash | ||
| mosquitto_pub -h your_mqtt_broker_address -p 1883 -t homekit/set_state -m "1" | ||
| ``` | ||
| (Replace `your_mqtt_broker_address` with your actual broker's IP or hostname.) | ||
|
|
||
| **Example: Subscribing to Lock State Updates** | ||
|
|
||
| To see your lock's current state in real-time: | ||
|
|
||
| ```bash | ||
| mosquitto_sub -h your_mqtt_broker_address -p 1883 -t homekit/state | ||
| ``` |
There was a problem hiding this comment.
Align CLI MQTT topic examples with <CLIENT_ID> prefix usage.
The core topics table uses <CLIENT_ID>/homekit/..., but the mosquitto_pub/mosquitto_sub examples later use homekit/set_state and homekit/state without the prefix. For readers who copy‑paste these commands, this mismatch can be confusing. Either:
- Include the
<CLIENT_ID>/prefix in the CLI examples (e.g.,-t <CLIENT_ID>/homekit/set_state), or - Add a short note that the example assumes you’ve customized topics to drop the client‑ID prefix.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
53-53: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/content/mqtt.md around lines 30 to 57, the CLI examples omit the
<CLIENT_ID>/ prefix used in the topics table which can confuse readers; update
the two command examples so they reference the prefixed topics (e.g., use
<CLIENT_ID>/homekit/set_state for mosquitto_pub and <CLIENT_ID>/homekit/state
for mosquitto_sub) or alternatively add a one‑line note above the examples
stating that the shown commands assume topics were customized to drop the
client-id prefix — make the change consistently in both commands and keep the
placeholder text (<CLIENT_ID>) so users can replace it with their actual client
id.
| HardwareManager::HardwareManager(const espConfig::actions_config_t& miscConfig) | ||
| : m_miscConfig(miscConfig), | ||
| m_feedbackTaskHandle(nullptr), | ||
| m_feedbackQueue(nullptr), | ||
| m_lockControlTaskHandle(nullptr), | ||
| m_lockControlQueue(nullptr) | ||
| { | ||
| espp::EventManager::get().add_subscriber( | ||
| "lock/action", "HardwareManager", | ||
| [&](const std::vector<uint8_t> &data) { | ||
| std::error_code ec; | ||
| EventLockState s = alpaca::deserialize<EventLockState>(data, ec); | ||
| if(!ec) { | ||
| setLockOutput(s.targetState); | ||
| } | ||
| }, 3072); | ||
| espp::EventManager::get().add_subscriber("nfc/event", "HardwareManager", [&](const std::vector<uint8_t> &data){ | ||
| std::error_code ec; | ||
| NfcEvent event = alpaca::deserialize<NfcEvent>(data, ec); | ||
| if(ec) return; | ||
| switch(event.type) { | ||
| case HOMEKEY_TAP: { | ||
| EventHKTap s = alpaca::deserialize<EventHKTap>(event.data, ec); | ||
| if(!ec){ | ||
| if(s.status) {showSuccessFeedback();triggerAltAction();} else showFailureFeedback(); | ||
| } | ||
| break; | ||
| } | ||
| case TAG_TAP: { | ||
| EventTagTap s = alpaca::deserialize<EventTagTap>(event.data, ec); | ||
| if(!ec){ | ||
| if (m_feedbackQueue != nullptr) { | ||
| FeedbackType feedback = FeedbackType::TAG_EVENT; | ||
| xQueueSend(m_feedbackQueue, &feedback, 0); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| }, 4096); | ||
| espp::EventManager::get().add_subscriber( | ||
| "hardware/gpioPinChanged", "HardwareManager", | ||
| [&](const std::vector<uint8_t> &data) { | ||
| std::error_code ec; | ||
| EventValueChanged s = alpaca::deserialize<EventValueChanged>(data, ec); | ||
| if(!ec) { | ||
| uint8_t state = 0; | ||
| if(s.oldValue != 255 && s.oldValue < GPIO_NUM_MAX){ | ||
| state = gpio_get_level(gpio_num_t(s.oldValue)); | ||
| gpio_reset_pin(gpio_num_t(s.oldValue)); | ||
| gpio_pullup_dis(gpio_num_t(s.oldValue)); | ||
| } | ||
| if(s.newValue != 255){ | ||
| pinMode(s.newValue, OUTPUT); | ||
| if(s.name == "gpioActionPin") | ||
| digitalWrite(s.newValue, state); | ||
| } | ||
| } | ||
| }, 3072); | ||
| espp::EventManager::get().add_publisher("lock/updateState", "HardwareManager"); | ||
| espp::EventManager::get().add_publisher("lock/altAction", "HardwareManager"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify EventManager subscriber cleanup in destructor.
The constructor registers three EventManager subscribers ("lock/action", "nfc/event", "hardware/gpioPinChanged") at lines 37-91 with [&] capture. If the HardwareManager destructor doesn't remove these subscribers, they will outlive the object.
Verify the destructor removes all three subscribers:
If cleanup is missing, add removal calls for all three topics in the destructor.
🏁 Script executed:
#!/bin/bash
# Check if HardwareManager destructor removes EventManager subscribers
rg -n "~HardwareManager" main/HardwareManager.cpp -A 15
rg -n "remove_subscriber.*HardwareManager" main/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for HardwareManager class definition and destructor in header and cpp
echo "=== Searching for HardwareManager class definition ==="
fd -e h -e hpp -e cpp | xargs rg -l "class HardwareManager"
echo -e "\n=== Checking for destructor in header files ==="
fd -e h -e hpp | xargs rg -n "~HardwareManager|class HardwareManager" -B 2 -A 10
echo -e "\n=== Checking entire cpp file for destructor pattern ==="
rg -n "HardwareManager::" main/HardwareManager.cpp | head -20
echo -e "\n=== Searching for any destructor pattern in HardwareManager files ==="
rg -n "^\s*~" main/HardwareManager.cppLength of output: 3550
🏁 Script executed:
#!/bin/bash
# Read the complete HardwareManager header file
echo "=== HardwareManager.hpp content ==="
cat git/main/include/HardwareManager.hpp
echo -e "\n=== HardwareManager.cpp content ==="
cat git/main/HardwareManager.cppLength of output: 202
🏁 Script executed:
#!/bin/bash
# Understand directory structure and locate files
echo "=== Current directory ==="
pwd
echo -e "\n=== List root directory contents ==="
ls -la
echo -e "\n=== Find HardwareManager files ==="
find . -name "*HardwareManager*" -type f 2>/dev/null
echo -e "\n=== Check if git directory exists ==="
ls -la git/ 2>/dev/null || echo "No git directory"Length of output: 1681
🏁 Script executed:
#!/bin/bash
# Read the complete HardwareManager header and implementation files
echo "=== HardwareManager.hpp content ==="
cat ./main/include/HardwareManager.hpp
echo -e "\n=== HardwareManager.cpp content ==="
cat ./main/HardwareManager.cppLength of output: 25478
Add destructor to remove EventManager subscribers.
The HardwareManager constructor registers three EventManager subscribers ("lock/action", "nfc/event", "hardware/gpioPinChanged") with [&] capture, but the class has no destructor to clean them up. When the HardwareManager object is destroyed, these subscribers remain registered with stale references, causing use-after-free when events are published to these topics.
Add a destructor in HardwareManager.hpp and implement it in HardwareManager.cpp:
// Header
~HardwareManager();
// Implementation
HardwareManager::~HardwareManager() {
espp::EventManager::get().remove_subscriber("lock/action", "HardwareManager");
espp::EventManager::get().remove_subscriber("nfc/event", "HardwareManager");
espp::EventManager::get().remove_subscriber("hardware/gpioPinChanged", "HardwareManager");
}🤖 Prompt for AI Agents
In main/HardwareManager.cpp around lines 30 to 94, the constructor registers
three EventManager subscribers with lambda captures but there is no destructor
to unregister them, leaving dangling subscribers after object destruction; add a
destructor declaration ~HardwareManager() to HardwareManager.hpp and implement
it in HardwareManager.cpp to call espp::EventManager::get().remove_subscriber
for "lock/action", "nfc/event", and "hardware/gpioPinChanged" using the same
subscriber name "HardwareManager" so the callbacks are removed when the object
is destroyed.
| NfcManager::NfcManager(ReaderDataManager& readerDataManager,const std::array<uint8_t, 4> &nfcGpioPins) | ||
| : nfcGpioPins(nfcGpioPins), | ||
| m_readerDataManager(readerDataManager), | ||
| m_pollingTaskHandle(nullptr), | ||
| m_retryTaskHandle(nullptr), | ||
| m_ecpData({ 0x6A, 0x2, 0xCB, 0x2, 0x6, 0x2, 0x11, 0x0 }) | ||
| { | ||
| espp::EventManager::get().add_publisher("nfc/event", "NfcManager"); | ||
| espp::EventManager::get().add_subscriber("homekit/internal", "NfcManager", [&](const std::vector<uint8_t> &data){ | ||
| std::error_code ec; | ||
| HomekitEvent event = alpaca::deserialize<HomekitEvent>(data, ec); | ||
| if(ec) return; | ||
| switch(event.type) { | ||
| case ACCESSDATA_CHANGED: | ||
| updateEcpData(); | ||
| break; | ||
| case DEBUG_AUTH_FLOW: { | ||
| EventValueChanged s = alpaca::deserialize<EventValueChanged>(event.data, ec); | ||
| if(!ec){ | ||
| authFlow = KeyFlow(s.newValue); | ||
| } | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| } | ||
| }, 3072); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify EventManager subscriber cleanup in destructor.
The constructor registers an EventManager subscriber at lines 37-55 with [&] capture. If the NfcManager destructor doesn't call espp::EventManager::get().remove_subscriber("homekit/internal", "NfcManager"), the subscriber will outlive the object and access dangling references.
Verify the destructor removes this subscriber:
If the destructor doesn't remove the subscriber, add cleanup similar to the MqttManager pattern.
🏁 Script executed:
#!/bin/bash
# Check if NfcManager destructor removes EventManager subscribers
rg -n "~NfcManager" main/NfcManager.cpp -A 10
rg -n "remove_subscriber.*NfcManager" main/Length of output: 0
I need to search more comprehensively. Let me check the header file and verify the destructor definition:
Now let me search the codebase more broadly for the NfcManager header and any destructor:
🏁 Script executed:
#!/bin/bash
# Find NfcManager header file
find main -name "*.h" -type f | xargs grep -l "class NfcManager"
# Also check for destructor in any C++ files for NfcManager
find main -type f \( -name "*.cpp" -o -name "*.h" \) | xargs grep -n "~NfcManager"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Broader search for NfcManager files anywhere
find . -type f \( -name "*.h" -o -name "*.cpp" -o -name "*.hpp" \) | xargs grep -l "class NfcManager\|~NfcManager" 2>/dev/null | head -20
# Also check what files exist in main directory
ls -la main/ | grep -i nfcLength of output: 94
🏁 Script executed:
#!/bin/bash
# Read the NfcManager header file
cat ./main/include/NfcManager.hppLength of output: 1288
Now let me verify that the EventManager API has a remove_subscriber method and check the MqttManager pattern for reference:
🏁 Script executed:
#!/bin/bash
# Check MqttManager for the correct pattern
grep -n "remove_subscriber" main/MqttManager.cpp -B 2 -A 2
# Also search for remove_subscriber usage patterns in codebase
rg -n "remove_subscriber" --type cpp --type hpp -C 3Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for remove_subscriber without file type restriction
rg -n "remove_subscriber" --type-list | head -5
# Try simpler approach - just search for the string
rg -n "remove_subscriber" -C 3
# Also check MqttManager destructor
rg -n "~MqttManager" -A 10Length of output: 2808
🏁 Script executed:
#!/bin/bash
# Search for EventManager usage in MqttManager
rg -n "EventManager" main/MqttManager.cpp -B 2 -A 2
# Search for EventManager API definition
find . -name "*.h" -o -name "*.hpp" | xargs grep -l "class EventManager" 2>/dev/null
# Search for any add_subscriber calls to see patterns
rg -n "add_subscriber" -C 2Length of output: 8112
🏁 Script executed:
#!/bin/bash
# Search for espp library files that might define EventManager
find . -path "*/espp/*" -name "*.h" -o -path "*/espp/*" -name "*.hpp" | head -20
# Also check if there's documentation or comments about cleanup
rg -n "unsubscribe\|remove_subscriber\|cleanup.*subscriber" -i
# Check MqttManager destructor to confirm it does NOT clean EventManager subscribers
cat main/MqttManager.cpp | grep -A 15 "MqttManager::~MqttManager"Length of output: 575
🌐 Web query:
espp EventManager add_subscriber remove_subscriber API documentation
💡 Result:
The EventManager API (ESPP) exposes these relevant functions:
-
bool add_subscriber(const std::string &topic, const std::string &component, espp::EventManager::event_callback_fn callback, espp::Task::BaseConfig task_config = {}); — register a subscriber callback for topic (returns true on success). [1]
-
bool remove_subscriber(const std::string &topic, const std::string &component); — remove the subscriber previously registered for topic/component (returns true if removed). [1]
Reference docs (API page + component registry). [1][2]
Sources
[1] Event Manager APIs — ESPP (API reference & examples).
[2] espp/event_manager — ESP Component Registry (version info).
Add destructor to NfcManager to unsubscribe from EventManager.
The constructor at lines 37-55 registers an EventManager subscriber with [&] reference capture. The espp::EventManager API provides remove_subscriber() to clean up subscribers, but NfcManager lacks a destructor. When the object is destroyed, the subscriber remains active and any subsequent "homekit/internal" events will invoke the lambda with dangling references to destroyed members.
Add a destructor to main/include/NfcManager.hpp:
~NfcManager();And implement it in main/NfcManager.cpp:
NfcManager::~NfcManager() {
espp::EventManager::get().remove_subscriber("homekit/internal", "NfcManager");
}🤖 Prompt for AI Agents
In main/NfcManager.cpp around lines 29-56 and corresponding header
main/include/NfcManager.hpp, the constructor registers a "homekit/internal"
subscriber with a lambda that captures by reference but there is no destructor
to remove the subscriber, causing potential use-after-free when NfcManager is
destroyed; add a destructor declaration (~NfcManager();) to
main/include/NfcManager.hpp and implement NfcManager::~NfcManager() in
main/NfcManager.cpp to call
espp::EventManager::get().remove_subscriber("homekit/internal", "NfcManager") so
the subscriber is removed during object teardown.
|
|
||
| ### Project Structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language specifier to code fence (MD040).
The project structure code block is missing a language identifier, triggering markdownlint MD040:
-```
+```text
HomeKey-ESP32/
├── main/ # Core ESP32 application🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
228-228: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In README.md around line 228, the code fence for the project structure is
missing a language specifier which triggers markdownlint MD040; update the
opening triple-backtick to include a language identifier (e.g., ```text or
```bash) so the fenced code block has an explicit language, leaving the block
content unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/content/setup.md (1)
106-106: Fix GitHub capitalization (duplicate issue).Line 106 uses lowercase "Github"; the official name is "GitHub". This was previously flagged and should have been corrected.
- * Download from the Github releases page [here](https://github.com/espressif/esptool/releases/latest) + * Download from the GitHub releases page [here](https://github.com/espressif/esptool/releases/latest)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/content/images/IMG_4025.jpegis excluded by!**/*.jpeg
📒 Files selected for processing (2)
.github/workflows/esp32.yml(8 hunks)docs/content/setup.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/esp32.yml
🧰 Additional context used
🪛 LanguageTool
docs/content/setup.md
[uncategorized] ~14-~14: The official name of this software platform is spelled with a capital “H”.
Context: ...kx/HomeKey-ESP32/releases/latest" icon="github" >}} 2. **Download the Latest Release:*...
(GITHUB)
[uncategorized] ~106-~106: The official name of this software platform is spelled with a capital “H”.
Context: ... ``` Or * Download from the Github releases page [here](https://github.com...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/content/setup.md
14-14: Bare URL used
(MD034, no-bare-urls)
34-34: Blank line inside blockquote
(MD028, no-blanks-blockquote)
106-106: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: esp32s3
- GitHub Check: esp32c3
- GitHub Check: esp32
- GitHub Check: esp32c6
| > [!NOTE] | ||
| > Both the ESP32 and the PN532 must use the same power supply to communicate. | ||
|
|
||
| > [!IMPORTANT] | ||
| > The PN532 has to be set to SPI mode, not I2C mode. | ||
| > The common red board has a DIP switch on a cornerside of the board that switches between I2C and SPI mode. See the image below for reference. | ||
| >  |
There was a problem hiding this comment.
Remove blank line inside blockquote.
Markdown blockquotes should not contain blank lines between consecutive blockquote statements. Line 34 creates an unintended separation between the NOTE and IMPORTANT blockquotes.
> [!NOTE]
>
> Both the ESP32 and the PN532 must use the same power supply to communicate.
-
-> [!IMPORTANT]
+> [!IMPORTANT]
>
> The PN532 has to be set to SPI mode, not I2C mode.📝 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.
| > [!NOTE] | |
| > Both the ESP32 and the PN532 must use the same power supply to communicate. | |
| > [!IMPORTANT] | |
| > The PN532 has to be set to SPI mode, not I2C mode. | |
| > The common red board has a DIP switch on a cornerside of the board that switches between I2C and SPI mode. See the image below for reference. | |
| >  | |
| > [!NOTE] | |
| > Both the ESP32 and the PN532 must use the same power supply to communicate. | |
| > [!IMPORTANT] | |
| > The PN532 has to be set to SPI mode, not I2C mode. | |
| > The common red board has a DIP switch on a cornerside of the board that switches between I2C and SPI mode. See the image below for reference. | |
| >  |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
34-34: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
In docs/content/setup.md around lines 32 to 38, there is an extra blank line
between two Markdown blockquote lines causing the NOTE and IMPORTANT blockquotes
to be separated; remove the blank line (delete the empty line at line 34) so the
blockquote lines are consecutive and render as intended.
* 📝 Add docstrings to `dev` Docstrings generation was requested by @rednblkx. * rednblkx#205 (comment) The following files were modified: * `data/src/services/api.js` * `main/ConfigManager.cpp` * `main/HKServices.cpp` * `main/HardwareManager.cpp` * `main/HomeKitLock.cpp` * `main/LockManager.cpp` * `main/MqttManager.cpp` * `main/NfcManager.cpp` * `main/ReaderDataManager.cpp` * `main/WebServerManager.cpp` * `main/WebSocketLogSinker.cpp` * `main/include/WebServerManager.hpp` * `main/include/WebSocketLogSinker.h` * `main/include/config.hpp` * `main/include/utils.hpp` * `main/main.cpp` * docs: fix wording --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: White Red <rednblkx@protonmail.com>
Modularize backend and refactor frontend to Svelte 5 An extensize refactoring of the codebase that restructures it into 8 pillars of management to implement a separation of responsibilities as follows: - ConfigManager: to handle passing configurations to other components - HardwareManager: handling hardware actions (Simple GPIO and Pixel) - LockManager: handling the internal state for the virtual lock that gets synced with HomeKit and Home Assistant - NfcManager: handling all the NFC communications - MqttManager: handling MQTT communications - HomeKitLock: the star of the show, handling all HomeKit-related stuff - ReaderDataManager: handling the private authentication data for HomeKey - WebServerManager: handling the Web interface and APIs The web interface also gets a new look using the Svelte 5 web framework and TailwindCSS + DaisyUI for the styling.
2 participants
An extensize refactoring of the codebase that restructures it into 8 pillars of management to implement a separation of responsibilities as follows:
The web interface also gets a new look using the Svelte 5 web framework and TailwindCSS + DaisyUI for the styling.
Summary by CodeRabbit
New Features
CI / Build
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.