Claude finished @Yeraze's task —— View job

PR Review: feat(automations): richer Test-panel dry-run inputs & output (#3653)

Tasks:

• Gather context and understand the request
• Read changed source files
• Review code quality, bugs, performance, security, and test coverage
• Post final review

Overall this is clean, well-scoped frontend work. A few things worth noting:

Potential Bugs

buildNode() uses Number() without isFinite guard for lat/lon (AutomationTester.tsx:96-97):

if (facts.latitude) out.latitude = Number(facts.latitude);
if (facts.longitude) out.longitude = Number(facts.longitude);

If the user types a non-numeric string (e.g. "abc"), Number("abc") → NaN is silently passed to the API. The numFact helper on line 91 guards correctly with isFinite — latitude and longitude should use the same pattern for consistency.

Also, if (facts.latitude) is falsy for 0 — a latitude of 0 (on the equator) would never be sent. Same for longitude. The guard should be facts.latitude !== undefined && facts.latitude !== ''.

buildEvent() for geofence drops the event field (AutomationTester.tsx:83):

case 'geofence':
  return { ...base, nodeNum: numOrUndef(ev.nodeNum) };

The TRIGGER_TOKENS in SubstitutionsHelp.tsx:26 documents event (enter / exit / dwell) as a geofence trigger field, but renderEventInputs (line 214) and buildEvent offer no way to set it. A geofence workflow checking trigger.event == "enter" can never be made true in a dry-run.

viaMqtt sends undefined when unchecked (AutomationTester.tsx:74):

viaMqtt: ev.viaMqtt === 'true' ? true : undefined

undefined is dropped by JSON.stringify, so the engine receives no viaMqtt key when the toggle is off. This is fine if undefined and false are treated equivalently by the simulator — but if the engine tests trigger.viaMqtt === false explicitly, this would behave differently from false. Worth confirming with the engine's substitution logic.

Code Quality / Maintainability

SimResult interface is duplicated — it's defined in AutomationTester.tsx:15-24 but the canonical shape likely already exists in a shared types file (e.g. src/types/automation.ts). Sharing one source of truth avoids drift if the simulator response shape changes.

KIND_BY_TRIGGER map and TRIGGER_TOKENS/TRIGGER_LABEL in SubstitutionsHelp.tsx are parallel structures that must stay in sync. When a new trigger type is added, all three need to be updated independently. Consolidating into a single catalog entry (like the existing BLOCK_BY_TYPE pattern in catalog.ts) would reduce the maintenance surface.

any type in the error handler (AutomationTester.tsx:120):

} catch (e: any) {

The project's TypeScript is otherwise strict. Consider catch (e: unknown) with a type guard, consistent with how other error handlers in the codebase work.

Array index keys on rendered lists (AutomationTester.tsx:287, 299, 313):

key={i}

Used throughout TestResult renders. Fine for purely read-only display (results never reorder), but flags in some lint configurations. Not a functional issue here.

Performance

No concerns — this is all local state manipulation and a single API call. The render paths are straightforward.

Security

No concerns. The Test panel sends to POST /api/automations/test which is documented as performing no mesh IO or persistence, and the inputs are standard form values with no unescaped HTML injection risk (React escapes all rendered strings by default; the only raw interpolation is JSON.stringify output in a <pre>).

The resolvedParams JSON in <pre> inside <details> (line 262) is safe — it's text content, not dangerouslySetInnerHTML.

Test Coverage

No tests for the new components. SubstitutionsHelp.tsx is entirely untested. AutomationTester.tsx has no unit or integration tests (only compile.test.ts exists for this directory).

Given that buildEvent() and buildNode() contain non-trivial data-shaping logic (the latitude-zero bug above, the numOrUndef helper, the viaMqtt coercion), they'd be good candidates for unit tests. Suggested coverage:

• buildEvent() per trigger type, including edge cases (viaMqtt toggle, empty optional fields, non-numeric input for number fields)
• buildNode() with latitude: "0", latitude: "abc", and empty state
• SubstitutionsHelpDrawer renders all token groups and current-trigger ordering

Minor Nits

• ae-builder-hint class added to CSS in AutomationsPage.css — it's not defined there (missing from the diff). If it's inheriting fine from another rule this is benign, but worth checking it exists.
• The ? help button in the Test panel (AutomationTester.tsx:130) has no visible label — keyboard users relying on button text won't discover it. The existing title attribute helps screen readers but a aria-label is more reliable than title.