|
|
@@ -1,6 +1,8 @@
|
|
|
# MPCkit review fixes: work plan
|
|
|
|
|
|
-Written 2026-11-12; PR 8b added 2026-11-13. Ten PRs, worked one at a time, fix the findings of the MPCkit source code review of `e32802a` (v0.5.1, 2026-11-12). PRs 1-4 ship the security fixes as **0.5.2**; PRs 5-9, including 8b, ship the hardening work as **0.5.3**. Finding IDs (S1, M3, R1 and so on) refer to that review. Each PR below repeats what an agent needs from it, so the plan works without access to the review.
|
|
|
+Written 2026-11-12; PR 8b added 2026-11-13; PRs 10-12 (readability) added 2026-11-13. Ten PRs (1-9 and 8b), worked one at a time, fix the findings of the MPCkit source code review of `e32802a` (v0.5.1, 2026-11-12). PRs 1-4 ship the security fixes as **0.5.2**; PRs 5-9, including 8b, ship the hardening work as **0.5.3**. Finding IDs (S1, M3, R1 and so on) refer to that review. Each PR below repeats what an agent needs from it, so the plan works without access to the review.
|
|
|
+
|
|
|
+PRs 1-9 and 8b are done: 0.5.2 and 0.5.3 are released. PRs 10-12 tidy the code for human review (R2-R7). They change no behaviour and no version.
|
|
|
|
|
|
## Decisions
|
|
|
|
|
|
@@ -14,6 +16,9 @@ These were settled with the user on 2026-11-12. Agents follow them and do not re
|
|
|
| Releases | 0.5.2 after PRs 1-3, 0.5.3 after PRs 5-8 and 8b. PRs in between do not change the version. |
|
|
|
| CI (S6) | Out of scope. No CI files, no `task check`. |
|
|
|
| PR records (R8) | Keep the current layout. R8 is dropped. |
|
|
|
+| Formatter (R2) | Approved: add `prettier` and `prettier-plugin-svelte` as dev dependencies and reformat the whole frontend in one PR (PR 11). Decided 2026-11-13. |
|
|
|
+| Readability PRs (10-12) | No version bump and no release of their own; they ship in whichever release comes next. Decided 2026-11-13. |
|
|
|
+| S5, M9, M10, M11 | Skipped for now (2026-11-13). They stay in the Backlog and need decisions before they can be planned. |
|
|
|
| Linked WAVs (PR 3) | A WAV the user picks that is a link is followed at assign: the pool file is named after the link, and its source is the file the link points to. Export refuses any source that is a link or sits in a linked folder. Decided 2026-11-12. |
|
|
|
| New sample names (PR 3) | Besides `/`, `\` and control characters, the characters the device's FAT card forbids (`: * ? " < > \|`) also become `_`, and a name Windows reserves for a device (`CON.wav`, `aux.wav`, `com1.wav`) gets `_` added (`CON_.wav`). Decided 2026-11-12 and 2026-11-13. |
|
|
|
|
|
|
@@ -47,11 +52,13 @@ The VPS has Node 24, the GTK 4 and WebKitGTK 6 build files and Xvfb, so all of t
|
|
|
|
|
|
```
|
|
|
export PATH=$PATH:~/go/bin
|
|
|
+export GOTOOLCHAIN=go1.27.4 # openSUSE's Go defaults to local and ignores go.mod's toolchain line
|
|
|
go test ./...
|
|
|
go vet ./...
|
|
|
staticcheck ./...
|
|
|
govulncheck ./...
|
|
|
(cd frontend && npm ci && npm run check && npm run build)
|
|
|
+(cd frontend && npm run format:check) # from PR 11 on
|
|
|
wails3 build
|
|
|
```
|
|
|
|
|
|
@@ -78,9 +85,11 @@ The README's "Releasing" paragraph lists the files that carry the version; chang
|
|
|
PR 1 harden-load → PR 2 limit-decompression → PR 3 safe-export → PR 4 release-0-5-2 (tag v0.5.2)
|
|
|
0.5.3: hardening
|
|
|
PR 5 unbind-paths → PR 6 atomic-edits → PR 7 a1-labels → PR 8 build-cleanup → PR 8b bound-surface → PR 9 release-0-5-3 (tag v0.5.3)
|
|
|
+Readability (no release)
|
|
|
+ PR 10 go-readability → PR 11 frontend-format → PR 12 shared-modal
|
|
|
```
|
|
|
|
|
|
-Each PR starts from `main` after the previous one is merged. The order matters in four places: PR 3 reuses PR 1's file name check, PR 6 builds on the nodes PR 1 stores on `Project`, PRs 5 and 7 both change `internal/app` and the frontend, and PR 8b finishes what PR 5 started and must be merged before PR 9.
|
|
|
+Each PR starts from `main` after the previous one is merged. The order matters in four places: PR 3 reuses PR 1's file name check, PR 6 builds on the nodes PR 1 stores on `Project`, PRs 5 and 7 both change `internal/app` and the frontend, and PR 8b finishes what PR 5 started and must be merged before PR 9. PR 12 starts after PR 11, so the new modal is written in the Prettier style.
|
|
|
|
|
|
## PRs and tasks
|
|
|
|
|
|
@@ -96,6 +105,9 @@ Each PR starts from `main` after the previous one is merged. The order matters i
|
|
|
| 8 | `build-cleanup` | S6 (without CI), M7, M8, R1 (rest) | 0.5.3 |
|
|
|
| 8b | `bound-surface` | S4 (rest) | 0.5.3 |
|
|
|
| 9 | `release-0-5-3` | Version 0.5.3 | 0.5.3 |
|
|
|
+| 10 | `go-readability` | R3 (Go), R4, R5, R6 | none |
|
|
|
+| 11 | `frontend-format` | R2, R3 (Svelte) | none |
|
|
|
+| 12 | `shared-modal` | R7 | none |
|
|
|
|
|
|
### PR 1. Reject malformed projects at load (`harden-load`)
|
|
|
|
|
|
@@ -235,9 +247,67 @@ Each PR starts from `main` after the previous one is merged. The order matters i
|
|
|
|
|
|
Same steps as PR 4, with 0.5.3. The PR record lists what changed for testers, including the new "A1" labels. After the merge, tag `v0.5.3` ("MPCkit 0.5.3 (beta)").
|
|
|
|
|
|
+### Readability PRs: what "no behaviour change" means
|
|
|
+
|
|
|
+PRs 10-12 only rename, move, reformat and comment code. Each one proves that nothing changed:
|
|
|
+
|
|
|
+- `go test ./...` passes with only renamed identifiers changed in the tests; no test is removed or loosened.
|
|
|
+- **Exports are byte-identical.** Before the first commit, on `main`, run `go run ./cmd/mpckit-cli -load testdata/device/DemoFile.xpj -out <before>`; after the last, run it again into `<after>`, then `cmp <before>/DemoFile.xpj <after>/DemoFile.xpj` and `diff -r` the two data folders. Note the result in Testing.
|
|
|
+- No bound method changes: `TestBoundMethods` passes untouched and `frontend/bindings` does not change.
|
|
|
+- The PR record has a **Testing** list for the laptop that walks both tabs and every dialog.
|
|
|
+
|
|
|
+### PR 10. Go readability (`go-readability`)
|
|
|
+
|
|
|
+**Why.** Several names and functions make review harder than it needs to be. `state()` in `internal/app/app.go` uses `p`, `v`, `q`, `f`, `m` and `st` across one long function (R3). `wrap` builds a `Project` in `project` but prefixes an error in `xpj`; `edit` is an error-collecting type in `project` and a lock-and-return method on `Service` (R4). `main()` in `main.go` is 95 lines and mixes window sizing, the close guard, the file-drop handler and the panic handler; the CLI's `init()` in `cmd/mpckit-cli/main.go` registers nine flags as closures in 103 lines (R5). `uniqueFile` in `internal/project/pads.go` keeps file names and sample names in one map, told apart by a `"\x00"` suffix, and `!(bpm >= MinBPM && bpm <= MaxBPM)` in `sequences.go` is written that way to reject NaN with no comment saying so (R6).
|
|
|
+
|
|
|
+**Tasks**
|
|
|
+
|
|
|
+- [ ] Split `state()` into short helpers (for example `limits()`, `missingSources()`, `padState(slot)`, `sequenceStates()`) with descriptive local names.
|
|
|
+- [ ] Rename `xpj.wrap` to `wrapErr`, `project.wrap` to `fromFile`, `Service.edit` to `apply`, and the `project.edit` type to `setter` (or names of the same clarity; say which in the record). Update the comments that mention them.
|
|
|
+- [ ] In `main.go`, move the window sizing, the close guard and the file-drop handler into named functions, each with the comment that is there now. `main()` should read as a list of steps.
|
|
|
+- [ ] In the CLI, give each flag its own named parse function (`parsePadOp`, `parseTuneOp`, ...) and register them from a short table in `init()`. Flag names, usage text and output stay exactly the same.
|
|
|
+- [ ] Replace the `"\x00"` keys in `uniqueFile` with two maps, `takenFiles` and `takenNames`.
|
|
|
+- [ ] Add a comment to the BPM check saying that it is written this way so NaN fails it.
|
|
|
+
|
|
|
+**Done when** the "no behaviour change" checks pass, `go run ./cmd/mpckit-cli -h` prints the same text as before, and no function in `main.go`, `cmd/mpckit-cli/main.go` or `internal/app/app.go` is longer than 60 lines.
|
|
|
+
|
|
|
+### PR 11. Format the frontend with Prettier (`frontend-format`)
|
|
|
+
|
|
|
+**Why.** Eight markup lines are over 160 characters, the longest 250 (`App.svelte`, `SequencePanel.svelte`, `PadPanel.svelte`); a change to one attribute shows as the whole line changed (R2). There is no formatter, so style drifts. `save()` in `SequencePanel.svelte` uses `s`, `n`, `b` and `l` for slot, name, BPM and bars (R3).
|
|
|
+
|
|
|
+**Tasks**
|
|
|
+
|
|
|
+- [ ] Add `prettier` and `prettier-plugin-svelte` as dev dependencies (approved; take the current releases that `min-release-age` allows) and commit the updated lockfile.
|
|
|
+- [ ] Add `frontend/.prettierrc` with `"printWidth": 120`, `"plugins": ["prettier-plugin-svelte"]`, and options that match the current style (double quotes, semicolons, trailing commas); add a `.prettierignore` for `dist`, `bindings` and `node_modules`.
|
|
|
+- [ ] Add `"format": "prettier --write ."` and `"format:check": "prettier --check ."` to `frontend/package.json`.
|
|
|
+- [ ] Commit the reformat on its own, with nothing else in that commit, so the PR has one large mechanical commit and small reviewable ones.
|
|
|
+- [ ] In a separate commit, rename the one-letter variables in `save()` and `remove()` in `SequencePanel.svelte` (`slot`, `newName`, `newBpm`, `newBars`).
|
|
|
+- [ ] Watch for whitespace changes in markup: Prettier can move text next to inline elements. Check each tab and dialog on the laptop for changed spacing and fix any with Prettier's own ignore comments only where needed.
|
|
|
+
|
|
|
+**Done when** `npm run format:check` passes, no line under `frontend/src` is longer than 120 characters except where Prettier cannot break it, `npm run check` is clean, and the laptop walk-through shows no visual change.
|
|
|
+
|
|
|
+### PR 12. One modal component (`shared-modal`)
|
|
|
+
|
|
|
+**Why.** `AboutModal`, `ConfirmModal` and `PadPicker` each repeat the same backdrop, dialog box, Escape handling, focus-on-open and styles (R7). A fix to one is easily missed in the others.
|
|
|
+
|
|
|
+**Tasks**
|
|
|
+
|
|
|
+- [ ] Add `frontend/src/lib/Modal.svelte`: backdrop, dialog box and shared styles; props for the title id, an optional description id, `role` (`dialog` or `alertdialog`), `onclose` (called on Escape) and `closeOnBackdrop` (default false); the content as a snippet. The opener passes the element to focus, or `Modal` focuses the first focusable element.
|
|
|
+- [ ] Rebuild the three dialogs on it, keeping each one's current behaviour exactly:
|
|
|
+
|
|
|
+| Dialog | role | Escape | Click outside | Focus on open |
|
|
|
+| --- | --- | --- | --- | --- |
|
|
|
+| About | `dialog` | closes | closes | Close button |
|
|
|
+| Confirm | `alertdialog`, with `aria-describedby` | Cancel | nothing | Cancel button |
|
|
|
+| Move/Copy to pad | `dialog` | closes | nothing | Bank select |
|
|
|
+
|
|
|
+- [ ] Remove the styles the three no longer need.
|
|
|
+
|
|
|
+**Done when** `npm run check` and `npm run format:check` pass, and on the laptop each dialog matches the table: it opens with focus where the table says, Escape and clicking outside do what it says, Tab stays usable, and screen-reader names are unchanged (check with Narrator or the browser's accessibility tree in `wails3 dev`).
|
|
|
+
|
|
|
## Backlog
|
|
|
|
|
|
-Not scheduled. Each can become its own PR later, following the same guidelines; none blocks a release.
|
|
|
+Not scheduled; skipped for now (2026-11-13). Each needs decisions before it can be planned; none blocks a release.
|
|
|
|
|
|
| Finding | What | Where |
|
|
|
| --- | --- | --- |
|
|
|
@@ -245,11 +315,5 @@ Not scheduled. Each can become its own PR later, following the same guidelines;
|
|
|
| M9 | Dialog cancel is detected by comparing error text; pin it with a test | `dialogs.go` |
|
|
|
| M10 | Internal error text (`xpj: path ...`) reaches the user; show one plain sentence and log the detail | `internal/app`, `main.go` |
|
|
|
| M11 | `state()` stats every pool file and scans all events on each call, under the lock | `internal/app/app.go` |
|
|
|
-| R2 | Markup lines up to 250 characters; no formatter configured | `App.svelte`, `SequencePanel.svelte` |
|
|
|
-| R3 | One-letter names in long scopes (`s`, `n`, `b`, `l` in `save()`) | `SequencePanel.svelte`, `app.go` |
|
|
|
-| R4 | Same helper names with different meanings (`wrap`, `edit`) | `internal/project`, `internal/xpj`, `internal/app` |
|
|
|
-| R5 | Long setup functions; split into named helpers | `main.go`, `cmd/mpckit-cli/main.go` |
|
|
|
-| R6 | Clever code without a note (`"\x00"` map keys, NaN-safe BPM check, order of checks in `serveAudio`) | `pads.go`, `sequences.go`, `audio.go` |
|
|
|
-| R7 | Three modals repeat the same backdrop, dialog and Escape code | `frontend/src/lib/*Modal.svelte`, `PadPicker.svelte` |
|
|
|
-
|
|
|
-Dropped: R8 (PR record layout) and CI (part of S6), by decision.
|
|
|
+
|
|
|
+R2-R7 are planned as PRs 10-12. Dropped: R8 (PR record layout) and CI (part of S6), by decision.
|
