Add read-only Twister configuration UI - #10
Conversation
Add a local, dependency-free web UI (mft-config ui) for visually inspecting a connected MIDI Fighter Twister's complete configuration: banked 4x4 knob grid, persistent active/inactive/detent colors, rotary and push MIDI mappings, switch/movement/indicator/detent/super-knob settings, global settings, device identity and firmware details, compatibility warnings, and raw JSON export/download/offline-import. The UI reuses the existing read-only exporter and protocol decoder. The read-only boundary is enforced at the transport layer, not just in the browser: the server depends on RtMidiReadOnlyBackend, which exposes only discover/connect and cannot construct the separate write-capable connection; every outbound frame is also independently validated by assertReadOnlyRequest immediately before the native MIDI call, and the HTTP surface exposes no plan/apply/write/reset/system/bootloader route. Automated tests assert both layers, including that unknown /api/* mutation-shaped routes 404 without ever opening a MIDI connection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
Workflow actions use mutable tags
The workflow executes actions/checkout@v4 and actions/setup-node@v4 rather than immutable commit SHAs, so upstream tag movement can change code executed with repository read access or alter the checkout and toolchain used by subsequent checks.
How this was verified: Both executable action references use mutable @v4 tags and the workflow grants contents: read.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 30-31
Comment:
**Workflow actions use mutable tags**
The workflow executes `actions/checkout@v4` and `actions/setup-node@v4` rather than immutable commit SHAs, so upstream tag movement can change code executed with repository read access or alter the checkout and toolchain used by subsequent checks.
**How this was verified:** Both executable action references use mutable `@v4` tags and the workflow grants `contents: read`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ed467b60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,38 @@ | |||
| const MF64_HUES = [0, 0, 16, 60, 140, 120, 114, 96, 78, 42, 22, 0, 334, 300, 320]; | |||
There was a problem hiding this comment.
Correct the MF64 hue table for blue and teal
When an MF64 snapshot contains canonical teal (index 33) or blue (index 45), this table makes the viewer render them as hsl(78 …) and hsl(0 …)—yellow-green and red, respectively—even though src/planner.ts maps those indices to teal and blue. Since the knob grid is intended to visually identify stored colors, these common MF64 settings are presented as the wrong colors; update the corresponding hue groups to match the palette.
Useful? React with 👍 / 👎.
An Opus adversarial review of the read-only Twister UI (write-safety boundary itself verified solid, untouched here) found six correctness and robustness issues, all fixed: 1. ui/app.js: assertSnapshot now validates globals.colorMap/superKnob/ brightness/animationChannels/sleep, device.midiPorts, firmware identityBytes, and capturedAt. renderSnapshot is now transactional: it only commits state.snapshot and unhides the viewer after every render function succeeds; a failure rolls back to the previous good snapshot (or the empty state) instead of leaving a spliced mix of new/old content on screen with a corrupt snapshot wired to Download. 2. src/ui-server.ts: /api/export now has a single-flight guard so a second concurrent export request gets 409 EXPORT_IN_PROGRESS instead of racing to open the same MIDI ports and having pullEncoderData match replies across requests by tag alone. 3. src/ui-server.ts rejects requests whose Host header doesn't name this server (400 INVALID_HOST), closing the cross-origin/DNS-rebind path to the local MIDI API. cli.ts now also validates and warns on a non-loopback MFT_CONFIG_UI_HOST. 4. src/midi.ts: RtMidiPorts.send is guarded again (defaults to assertReadOnlyRequest; RtMidiApplyConnection passes the broader assertApplyRequest), restoring "innermost layer is safe by default" even if a future call site bypasses the wrapper classes. 5. src/exporter.ts: pullGlobals/pullDeviceId/pullEncoderData once again check the DJTT vendor header before trusting the command byte, so unrelated SysEx sharing the bus can't be misread as a malformed Twister reply. 6. src/ui-server.ts: /api/export now also accepts the inputPort/ outputPort the browser saw at discovery time and rejects the request (409 DEVICE_LIST_CHANGED) if the freshly re-discovered device at that index no longer matches, instead of silently exporting the wrong (still read-only) device after a replug. Adds test/ui-app.test.ts (jsdom-backed) covering fix #1, and extends test/ui-server.test.ts with coverage for fixes #2, #3, and #6. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up: adversarial review fixesAn Opus adversarial review of this branch found six correctness/robustness issues (the write-safety boundary itself was verified solid and left untouched). All six are addressed in c5c32d8:
Added |
| if ( | ||
| (inputPort !== undefined && inputPort !== device.inputPort.name) || | ||
| (outputPort !== undefined && outputPort !== device.outputPort.name) |
There was a problem hiding this comment.
Optional identity check permits reselection
When an export request omits inputPort and outputPort after device discovery ordering changes, the server skips the identity comparison and exports whichever controller now occupies deviceIndex, causing the UI to return another Twister's configuration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui-server.ts
Line: 166-168
Comment:
**Optional identity check permits reselection**
When an export request omits `inputPort` and `outputPort` after device discovery ordering changes, the server skips the identity comparison and exports whichever controller now occupies `deviceIndex`, causing the UI to return another Twister's configuration.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
mft-config ui, a small local web application for visually inspecting a connected MIDI Fighter Twister's complete configuration: a banked 4x4 knob grid, persistent active/inactive/detent colors, rotary and push MIDI channel/number mappings, switch action and movement mode, indicator/detent/super-knob settings, global settings, device identity/firmware details, compatibility warnings, and the complete raw JSON exportdocs/ui-architecture.md, and link it from the READMEArchitecture
A small local Node HTTP server (
src/ui-server.ts) reuses the existing read-only exporter and protocol decoder that already backmft-config export, and serves dependency-free static HTML/CSS/JS (ui/). The browser never opens MIDI ports; it only talks to two endpoints on the local server:GET /api/devicesandPOST /api/export. This was chosen over a packaged desktop shell as the smallest approach that preserves reliable SysEx access (Node keeps the native MIDI bindings) while keeping the safety boundary in one place.Safety boundary
The read-only guarantee is enforced at the transport layer, not just in the UI:
RtMidiReadOnlyBackend, which exposes onlydiscover/connect— its type has noconnectForApply, so it cannot construct the separate write-capable connection the CLI'sapplycommand usesassertReadOnlyRequestimmediately before the native MIDI output call, allowing only the Universal Identity request, global pull (0x02), and encoder bulk-pull (0x04/0x01) — configuration writes (0x01), system/reset/bootloader-shaped commands, and any other command are rejected with a thrown error/api/*paths return 404 without ever opening a MIDI connectionAutomated tests assert both layers (
test/ui-server.test.ts):0x01write, malformed bulk push,0x03system command) before it reaches the MIDI output, and still permits readsapply,plan,write,reset,bootloader, andsystemAPI paths all 404 and never open a connectionValidation
npm run build— clean TypeScript buildnpm test— 21/21 tests pass, including the 5 UI-server tests abovenpm run test:package— packed tarball smoke test passes with theui/assets anddocs/ui-architecture.mdincludednode dist/ui-server.jswith no hardware attached: index page and/api/devices(empty array) respond200, and a mutation-shaped route (/api/apply) responds404without opening a MIDI connectionCloses #1
Greptile Summary
The PR adds a local read-only Twister configuration viewer, supporting live MIDI exports and offline JSON snapshots, while moving apply state into a per-user directory.
Confidence Score: 1/5
The PR is not safe to merge until stale device selection, legacy consumed-plan detection, and mutable CI action references are fixed.
Export requests can still omit device identity and silently select another controller after discovery ordering changes; apply ignores legacy completion records and can repeat consumed writes; CI continues to execute actions through mutable tags.
Files Needing Attention: src/ui-server.ts, ui/app.js, src/cli.ts, src/journal.ts, .github/workflows/ci.yml
Important Files Changed
Sequence Diagram
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: address adversarial review findings..." | Re-trigger Greptile