Martino FerrariandClaude Sonnet 4.6
1f8592f854
- calibration.js: fix baseSignalName('[0]') parity with Go/C++ (>= 0 not > 0)
- calibration.test.js: add assertions for '[0]' edge case in two existing tests
- app.js: remove stale typeof guard around refreshVScaleMenu (always defined)
- app.js: call refreshTrigThresholdField on trig-signal change (both assignment sites)
- index.html: drop maxlength='16' on unit input; normaliseCal is the sole enforcer
- configcheck/main.go: delete dead nextOneOf function (no callers)
- hub_calibration_test.go: delete orphaned waitBroadcast comment (function never existed)
- calibration.go: correct arrayIndexSuffix comment to document known Go/C++ difference
- Docs/StreamHub-API.md: add calibration entry count and unit byte limits to §5 table
- spec: fix configReloaded missing path field, '16 chars'→'16 UTF-8 bytes', StreamString→char[], chain scenario→configcheck program, four→five new frames
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
204 lines
6.9 KiB
Markdown
204 lines
6.9 KiB
Markdown
# Final code review fix — report 2
|
|
|
|
Date: 2026-08-17
|
|
Branch: feature/signal-calibration-config
|
|
|
|
## Summary
|
|
|
|
Nine findings from the final code review of the signal-calibration feature.
|
|
No C++ files were modified (those were already fixed in a separate commit).
|
|
|
|
---
|
|
|
|
## Finding 1 — `calibration.js:20` cross-hub parity break in `baseSignalName`
|
|
|
|
**File:** `Client/udpstreamer/static/calibration.js`
|
|
|
|
**Change:** Line 20: `open > 0` → `open >= 0`.
|
|
|
|
An input of `"[0]"` has the `[` at index 0. The old guard `open > 0` let it
|
|
fall through and return `"[0]"` unchanged, which then passed the non-empty check
|
|
in `normaliseCal` and was accepted as a signal name. Go's `arrayIndexSuffix`
|
|
regexp and the C++ `strchr` truncation both reduce `"[0]"` to the empty string
|
|
and reject the entry. The fix makes JS match.
|
|
|
|
Two assertions added to the existing tests in
|
|
`Client/udpstreamer/test/calibration.test.js`:
|
|
|
|
- In `'baseSignalName strips an element suffix'`:
|
|
`assert.strictEqual(C.baseSignalName('[0]'), '');`
|
|
- In `'normaliseCal rejects invalid entries'`:
|
|
`assert.strictEqual(C.normaliseCal({source: 'w', signal: '[0]'}), null);`
|
|
|
|
---
|
|
|
|
## Finding 2 — `app.js:3470` stale transitional guard
|
|
|
|
**File:** `Client/udpstreamer/static/app.js`
|
|
|
|
**Change:** Replaced
|
|
```js
|
|
if (typeof refreshVScaleMenu === 'function') refreshVScaleMenu(); // Task 8
|
|
```
|
|
with:
|
|
```js
|
|
refreshVScaleMenu();
|
|
```
|
|
`refreshVScaleMenu` is a hoisted function declaration and is always defined.
|
|
The guard and the task-number comment were both remnants of incremental
|
|
development and are now removed.
|
|
|
|
---
|
|
|
|
## Finding 3 — `index.html:223` wrong length cap on unit input
|
|
|
|
**File:** `Client/udpstreamer/static/index.html`
|
|
|
|
**Change:** Removed `maxlength="16"` from `<input id="vscale-cal-unit">`.
|
|
|
|
`maxlength` counts UTF-16 code units, not UTF-8 bytes, so it under-counted
|
|
multi-byte characters. `normaliseCal` (using `TextEncoder`) is the correct and
|
|
sole enforcement point.
|
|
|
|
---
|
|
|
|
## Finding 4 — stale trigger-threshold unit hint on signal change
|
|
|
|
**File:** `Client/udpstreamer/static/app.js`
|
|
|
|
**Change:** Added `refreshTrigThresholdField();` at both places where
|
|
`trig.signal` is assigned in the `trig-signal` change handler (lines ~2555
|
|
and ~2565). Previously the hint was only updated when calibration changed,
|
|
not when the user picked a different trigger signal, leaving a stale unit name
|
|
in the tooltip.
|
|
|
|
---
|
|
|
|
## Finding 5 — `configcheck/main.go:159` dead code
|
|
|
|
**File:** `Test/E2E/suite/client/configcheck/main.go`
|
|
|
|
**Change:** Deleted `func (c *conn) nextOneOf(...)` (33 lines) and its preceding
|
|
doc comment. The function had no callers; the actual reload check uses
|
|
`c.next("configReloaded")` and a separate `c.nextWithin("sources", ...)` peek.
|
|
No import became orphaned.
|
|
|
|
---
|
|
|
|
## Finding 6 — `hub_calibration_test.go:132-134` orphaned comment
|
|
|
|
**File:** `Common/Client/go/wshub/hub_calibration_test.go`
|
|
|
|
**Change:** Deleted the three-line comment block that introduced `waitBroadcast`,
|
|
a function that was never written. The comment sat directly above
|
|
`func sleepMillis(...)` and served no purpose.
|
|
|
|
---
|
|
|
|
## Finding 7 — `calibration.go:14` false parity claim in comment
|
|
|
|
**File:** `Common/Client/go/wshub/calibration.go`
|
|
|
|
**Change:** Rewrote the comment above `arrayIndexSuffix`. The old comment said
|
|
the regexp "Mirrors the C++ `strchr(signal,'[')` truncation" — which is false.
|
|
The regexp is anchored to the end of the string and requires digits; `strchr`
|
|
finds the first `[` anywhere in the name. For `A[1]B`, Go's regexp leaves it
|
|
unchanged while C++ truncates to `A`. The new comment describes both
|
|
implementations accurately and calls out the known difference explicitly.
|
|
|
|
---
|
|
|
|
## Finding 8 — `Docs/StreamHub-API.md` missing calibration limits
|
|
|
|
**File:** `Docs/StreamHub-API.md`
|
|
|
|
**Change:** Added two rows to the §5 Limits table:
|
|
|
|
| Limit | Value |
|
|
|-------|-------|
|
|
| Calibration entries | 256 (C++ hub, `kMaxCalibration`); unbounded (Go hub) |
|
|
| Calibration unit override | 16 UTF-8 bytes |
|
|
|
|
---
|
|
|
|
## Finding 9 — spec discrepancies
|
|
|
|
**File:** `docs/superpowers/specs/2026-08-16-udpstreamer-signal-calibration-and-config-design.md`
|
|
|
|
Five sub-items:
|
|
|
|
**9a** — `configReloaded` row missing `path` field.
|
|
Both hubs always include `path` in `configReloaded` (verified: Go
|
|
`buildConfigAckMsg` passes `sm.Path()` as path; C++ `BroadcastConfigAck`
|
|
formats `sourcesFile_` as `"path"` in the ok branch for `configReloaded`).
|
|
Fixed: added `"path":string` to the `configReloaded` row in the WebSocket
|
|
protocol table.
|
|
|
|
**9b** — "max 16 chars" → "max 16 UTF-8 bytes".
|
|
Fixed the unit validation cell in the data-model table.
|
|
|
|
**9c** — `StreamString` fields → fixed `char[]` arrays.
|
|
`CalibrationEntry` in `StreamHub.h` uses `char source[128]`, `char signal[128]`,
|
|
`char unit[17]` — no `StreamString`. Updated the C++ hub-implementation
|
|
paragraph to name the struct and its actual field types.
|
|
|
|
**9d** — E2E chain scenario → standalone `configcheck` program.
|
|
Replaced the description of a chain-scenario extension with an accurate
|
|
description of `Test/E2E/suite/client/configcheck/`, quoting its actual behaviour
|
|
(connects to either hub, asserts calibration protocol, exits non-zero on failure).
|
|
|
|
**9e** — "four new frames" → five.
|
|
Updated the documentation paragraph to list all five frames by name:
|
|
`calibration`, `setCalibration`, `configSaved`, `reloadConfig`, `configReloaded`.
|
|
|
|
---
|
|
|
|
## Verification output
|
|
|
|
### `node --check` + `node --test`
|
|
|
|
```
|
|
(node --check static/app.js && node --check static/calibration.js) → SYNTAX OK
|
|
|
|
TAP version 13
|
|
ok 1 - baseSignalName strips an element suffix
|
|
ok 2 - calKey is stable and separates the two fields
|
|
ok 3 - normaliseCal accepts a valid entry and fills defaults
|
|
ok 4 - normaliseCal strips an element suffix from the signal name
|
|
ok 5 - normaliseCal truncates an over-long unit
|
|
ok 6 - normaliseCal leaves short non-ASCII units untouched
|
|
ok 7 - normaliseCal truncates an over-long ASCII unit to exactly 16 bytes
|
|
ok 8 - normaliseCal cuts a mid-rune byte boundary back to the last complete rune
|
|
ok 9 - normaliseCal leaves a unit that is exactly 16 bytes ending on a complete multi-byte rune untouched
|
|
ok 10 - normaliseCal rejects invalid entries
|
|
ok 11 - applyCal and invertCal round-trip
|
|
ok 12 - applyCal passes non-finite samples through untouched
|
|
ok 13 - calRange re-orders when the scale is negative
|
|
ok 14 - CalTable.get returns IDENTITY for an unknown signal
|
|
ok 15 - CalTable.get resolves an element name to its base signal
|
|
ok 16 - CalTable.set stores, overwrites, and deletes identity entries
|
|
ok 17 - CalTable.replaceAll drops the previous contents
|
|
ok 18 - CalTable.list is sorted by source then signal
|
|
1..18
|
|
# tests 18
|
|
# pass 18
|
|
# fail 0
|
|
```
|
|
|
|
Note: 18 tests (unchanged from before) because the new assertions were added
|
|
inside two existing test functions, not as separate test cases.
|
|
|
|
### Go wshub
|
|
|
|
```
|
|
cd Common/Client/go && go build ./... && go vet ./... && go test ./wshub/
|
|
ok marte2/common/wshub 1.288s
|
|
```
|
|
|
|
### Go configcheck
|
|
|
|
```
|
|
cd Test/E2E/suite/client/configcheck && go build ./... && go vet ./...
|
|
(silent — CONFIGCHECK OK)
|
|
```
|