Merge branch 'unbind-paths' of bajdev/mpckit into main

frontend/bindings/git.monoexp.com/bajdev/mpckit/internal/app/service.ts

@@ -39,21 +39,16 @@ export function AssignSampleDialog(slot: number): $CancellablePromise<$models.St
 }
 
 /**
- * AssignSamplePath puts the WAV at path on the pad in slot. File drop uses it.
- */
-export function AssignSamplePath(slot: number, path: string): $CancellablePromise<$models.State> {
-    return $Call.ByID(614211513, slot, path);
-}
-
-/**
- * ChooseExportFolder asks for the folder to export into; "" if cancelled.
+ * ChooseExportFolder asks for the folder to export into and remembers it
+ * for ExportFolder. It returns the folder, or "" if cancelled.
  */
 export function ChooseExportFolder(): $CancellablePromise<string> {
     return $Call.ByID(3758813274);
 }
 
 /**
- * ChooseZipPath asks where to save the zip; "" if cancelled.
+ * ChooseZipPath asks where to save the zip and remembers it for ExportZip.
+ * It returns the path, or "" if cancelled.
  */
 export function ChooseZipPath(): $CancellablePromise<string> {
     return $Call.ByID(2500682668);
@@ -82,19 +77,20 @@ export function CopyPad($from: number, to: number): $CancellablePromise<$models.
 }
 
 /**
- * ExportFolder writes the project into dir. If the project is already
- * there and overwrite is false, nothing is written and Exists lists it.
+ * ExportFolder writes the project into the folder ChooseExportFolder
+ * chose. If the project is already there and overwrite is false, nothing
+ * is written and Exists lists it.
  */
-export function ExportFolder(dir: string, overwrite: boolean): $CancellablePromise<$models.ExportResult> {
-    return $Call.ByID(230458675, dir, overwrite);
+export function ExportFolder(overwrite: boolean): $CancellablePromise<$models.ExportResult> {
+    return $Call.ByID(230458675, overwrite);
 }
 
 /**
- * ExportZip writes the project as a zip at path. If the file exists and
- * overwrite is false, nothing is written and Exists lists it.
+ * ExportZip writes the project as a zip where ChooseZipPath chose. If the
+ * file exists and overwrite is false, nothing is written and Exists lists it.
  */
-export function ExportZip(path: string, overwrite: boolean): $CancellablePromise<$models.ExportResult> {
-    return $Call.ByID(3485442350, path, overwrite);
+export function ExportZip(overwrite: boolean): $CancellablePromise<$models.ExportResult> {
+    return $Call.ByID(3485442350, overwrite);
 }
 
 /**
@@ -105,13 +101,6 @@ export function Load(): $CancellablePromise<$models.LoadResult> {
     return $Call.ByID(3449372205);
 }
 
-/**
- * LoadPath opens the project at path in place of the current one.
- */
-export function LoadPath(path: string): $CancellablePromise<$models.LoadResult> {
-    return $Call.ByID(1860148430, path);
-}
-
 /**
  * MovePad moves the pad in from to slot to, replacing what was there.
  * Recorded notes stay on their slots; the frontend warns first.

frontend/src/lib/store.svelte.ts

@@ -143,15 +143,17 @@ export function confirm(c: Omit<Confirm, "resolve">): Promise<boolean> {
   });
 }
 
+// exportWith asks where to export, then writes there. Go remembers the
+// chosen target; the frontend never passes a path back.
 async function exportWith(
   choose: () => Promise<string>,
-  write: (target: string, overwrite: boolean) => Promise<ExportResult>,
+  write: (overwrite: boolean) => Promise<ExportResult>,
 ): Promise<void> {
   if (app.state?.missingCount) return; // the buttons are disabled too
   try {
     const target = await choose();
     if (!target) return; // dialog cancelled
-    let res = await write(target, false);
+    let res = await write(false);
     if (res.exists?.length) {
       const ok = await confirm({
         title: "Replace existing files?",
@@ -160,7 +162,7 @@ async function exportWith(
         action: "Replace",
       });
       if (!ok) return;
-      res = await write(target, true);
+      res = await write(true);
     }
     app.error = "";
     app.notice = "Exported to " + res.path;
@@ -171,9 +173,9 @@ async function exportWith(
 }
 
 export function exportFolder(): Promise<void> {
-  return exportWith(() => Service.ChooseExportFolder(), (dir, overwrite) => Service.ExportFolder(dir, overwrite));
+  return exportWith(() => Service.ChooseExportFolder(), (overwrite) => Service.ExportFolder(overwrite));
 }
 
 export function exportZip(): Promise<void> {
-  return exportWith(() => Service.ChooseZipPath(), (path, overwrite) => Service.ExportZip(path, overwrite));
+  return exportWith(() => Service.ChooseZipPath(), (overwrite) => Service.ExportZip(overwrite));
 }

internal/app/app.go

@@ -127,6 +127,11 @@ type Service struct {
 	p       *project.Project
 	dialogs Dialogs
 	closer  func() // closes the window without asking; set by main.go
+
+	// Where the next export goes, as chosen in the dialog. The frontend
+	// never passes a path itself, so a script in the window cannot make
+	// MPCkit write (or, with overwrite, delete) anywhere else.
+	exportDir, zipPath string
 }
 
 // SetCloser sets what CloseWindow calls.
@@ -163,7 +168,14 @@ func (s *Service) reset() {
 	if err != nil {
 		panic(err) // the embedded template and a constant name
 	}
+	s.use(p)
+}
+
+// use makes p the current project and forgets the export targets chosen
+// for the previous one. Callers hold s.mu.
+func (s *Service) use(p *project.Project) {
 	s.p = p
+	s.exportDir, s.zipPath = "", ""
 }
 
 // New replaces the current project with a new, empty one.
@@ -290,7 +302,10 @@ func (s *Service) AssignSampleDialog(slot int) (State, error) {
 	return s.AssignSamplePath(slot, path)
 }
 
-// AssignSamplePath puts the WAV at path on the pad in slot. File drop uses it.
+// AssignSamplePath puts the WAV at path on the pad in slot. main.go's file
+// drop handler uses it; it is not bound, so the window cannot pass a path.
+//
+//wails:ignore
 func (s *Service) AssignSamplePath(slot int, path string) (State, error) {
 	return s.edit(func(p *project.Project) error { return p.AssignSample(slot, path) })
 }
@@ -310,36 +325,63 @@ func (s *Service) SetMuteGroup(slot, group int) (State, error) {
 	return s.edit(func(p *project.Project) error { return p.SetMuteGroup(slot, group) })
 }
 
-// ChooseExportFolder asks for the folder to export into; "" if cancelled.
+// ChooseExportFolder asks for the folder to export into and remembers it
+// for ExportFolder. It returns the folder, or "" if cancelled.
 func (s *Service) ChooseExportFolder() (string, error) {
-	return s.dialogs.ChooseFolder()
+	dir, err := s.dialogs.ChooseFolder()
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.exportDir = ""
+	if err == nil {
+		s.exportDir = dir
+	}
+	return dir, err
 }
 
-// ChooseZipPath asks where to save the zip; "" if cancelled.
+// ChooseZipPath asks where to save the zip and remembers it for ExportZip.
+// It returns the path, or "" if cancelled.
 func (s *Service) ChooseZipPath() (string, error) {
 	s.mu.Lock()
 	name := s.p.Name
 	s.mu.Unlock()
-	return s.dialogs.SaveZip(name + ".zip")
+	path, err := s.dialogs.SaveZip(name + ".zip")
+	if path != "" && !strings.EqualFold(filepath.Ext(path), ".zip") {
+		path += ".zip"
+	}
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.zipPath = ""
+	if err == nil {
+		s.zipPath = path
+	}
+	return path, err
 }
 
-// ExportFolder writes the project into dir. If the project is already
-// there and overwrite is false, nothing is written and Exists lists it.
-func (s *Service) ExportFolder(dir string, overwrite bool) (ExportResult, error) {
+// errNoTarget is returned by an export with no target chosen.
+var errNoTarget = errors.New("choose where to export first")
+
+// ExportFolder writes the project into the folder ChooseExportFolder
+// chose. If the project is already there and overwrite is false, nothing
+// is written and Exists lists it.
+func (s *Service) ExportFolder(overwrite bool) (ExportResult, error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	path, err := export.Folder(s.p, dir, overwrite)
+	if s.exportDir == "" {
+		return ExportResult{}, errNoTarget
+	}
+	path, err := export.Folder(s.p, s.exportDir, overwrite)
 	return s.exported(path, err)
 }
 
-// ExportZip writes the project as a zip at path. If the file exists and
-// overwrite is false, nothing is written and Exists lists it.
-func (s *Service) ExportZip(path string, overwrite bool) (ExportResult, error) {
+// ExportZip writes the project as a zip where ChooseZipPath chose. If the
+// file exists and overwrite is false, nothing is written and Exists lists it.
+func (s *Service) ExportZip(overwrite bool) (ExportResult, error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if !strings.EqualFold(filepath.Ext(path), ".zip") {
-		path += ".zip"
+	if s.zipPath == "" {
+		return ExportResult{}, errNoTarget
 	}
+	path := s.zipPath
 	err := export.Zip(s.p, path, overwrite)
 	return s.exported(path, err)
 }
@@ -353,6 +395,7 @@ func (s *Service) exported(path string, err error) (ExportResult, error) {
 		return ExportResult{}, err
 	}
 	s.p.Dirty = false
+	s.exportDir, s.zipPath = "", "" // the next export asks again
 	return ExportResult{Path: path}, nil
 }
 
@@ -382,7 +425,11 @@ func (s *Service) Load() (LoadResult, error) {
 	return s.LoadPath(path)
 }
 
-// LoadPath opens the project at path in place of the current one.
+// LoadPath opens the project at path in place of the current one. Load
+// uses it after the dialog; it is not bound, so the window cannot pass a
+// path.
+//
+//wails:ignore
 func (s *Service) LoadPath(path string) (LoadResult, error) {
 	p, warnings, err := project.Load(path)
 	if err != nil {
@@ -390,7 +437,7 @@ func (s *Service) LoadPath(path string) (LoadResult, error) {
 	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.p = p
+	s.use(p)
 	if warnings == nil {
 		warnings = []string{}
 	}

internal/app/app_test.go

@@ -184,7 +184,7 @@ func TestExport(t *testing.T) {
 	d.folder = t.TempDir()
 	dir, err := s.ChooseExportFolder()
 	must(t, err)
-	res, err := s.ExportFolder(dir, false)
+	res, err := s.ExportFolder(false)
 	must(t, err)
 	if res.Path != filepath.Join(dir, "Kit.xpj") || len(res.Exists) != 0 {
 		t.Fatalf("export: %+v", res)
@@ -196,13 +196,20 @@ func TestExport(t *testing.T) {
 		t.Error(err)
 	}
 
-	// Second time: reported, not written, until overwrite.
-	res, err = s.ExportFolder(dir, false)
+	// The target is forgotten after a successful export.
+	if _, err := s.ExportFolder(false); err == nil || err.Error() != "choose where to export first" {
+		t.Errorf("export without choosing again: %v", err)
+	}
+	// Second time: reported, not written, until overwrite. The confirmed
+	// call uses the same target.
+	_, err = s.ChooseExportFolder()
+	must(t, err)
+	res, err = s.ExportFolder(false)
 	must(t, err)
 	if res.Path != "" || len(res.Exists) != 2 {
 		t.Errorf("re-export: %+v", res)
 	}
-	res, err = s.ExportFolder(dir, true)
+	res, err = s.ExportFolder(true)
 	must(t, err)
 	if res.Path == "" {
 		t.Error("overwrite export failed")
@@ -211,24 +218,82 @@ func TestExport(t *testing.T) {
 	d.zip = filepath.Join(dir, "kit-export") // no extension: .zip is added
 	zpath, err := s.ChooseZipPath()
 	must(t, err)
-	if d.zipDefault != "Kit.zip" {
-		t.Errorf("zip dialog default %q", d.zipDefault)
+	if d.zipDefault != "Kit.zip" || zpath != filepath.Join(dir, "kit-export.zip") {
+		t.Errorf("zip dialog default %q, chosen %q", d.zipDefault, zpath)
 	}
-	res, err = s.ExportZip(zpath, false)
+	res, err = s.ExportZip(false)
 	must(t, err)
 	if res.Path != filepath.Join(dir, "kit-export.zip") {
 		t.Errorf("zip: %+v", res)
 	}
-	if res, _ = s.ExportZip(zpath, false); len(res.Exists) != 1 {
+	_, err = s.ChooseZipPath()
+	must(t, err)
+	if res, _ = s.ExportZip(false); len(res.Exists) != 1 {
 		t.Errorf("zip re-export: %+v", res)
 	}
 
 	// Missing folder: a real error.
-	if _, err := s.ExportFolder(filepath.Join(dir, "missing", "deeper"), false); err == nil {
+	d.folder = filepath.Join(dir, "missing", "deeper")
+	_, err = s.ChooseExportFolder()
+	must(t, err)
+	if _, err := s.ExportFolder(false); err == nil {
 		t.Error("export into a missing folder succeeded")
 	}
 }
 
+func TestExportTarget(t *testing.T) {
+	s, d := newService()
+	for _, export := range []func(bool) (ExportResult, error){s.ExportFolder, s.ExportZip} {
+		if _, err := export(false); err == nil || err.Error() != "choose where to export first" {
+			t.Errorf("export with nothing chosen: %v", err)
+		}
+	}
+
+	// A cancelled dialog forgets an earlier choice.
+	d.folder, d.zip = t.TempDir(), filepath.Join(t.TempDir(), "kit.zip")
+	_, err := s.ChooseExportFolder()
+	must(t, err)
+	_, err = s.ChooseZipPath()
+	must(t, err)
+	d.folder, d.zip = "", ""
+	_, err = s.ChooseExportFolder()
+	must(t, err)
+	_, err = s.ChooseZipPath()
+	must(t, err)
+	if s.exportDir != "" || s.zipPath != "" {
+		t.Errorf("cancel kept %q, %q", s.exportDir, s.zipPath)
+	}
+
+	// New and Load forget the targets chosen for the old project.
+	d.folder, d.zip = t.TempDir(), filepath.Join(t.TempDir(), "kit.zip")
+	choose := func() {
+		_, err := s.ChooseExportFolder()
+		must(t, err)
+		_, err = s.ChooseZipPath()
+		must(t, err)
+	}
+	choose()
+	s.New()
+	if s.exportDir != "" || s.zipPath != "" {
+		t.Error("New kept the export targets")
+	}
+	choose()
+	_, err = s.LoadPath(filepath.Join(testdata, "device", "DemoBlank.xpj"))
+	must(t, err)
+	if s.exportDir != "" || s.zipPath != "" {
+		t.Error("Load kept the export targets")
+	}
+}
+
+// exportTo chooses dir in the fake dialog and exports there.
+func exportTo(t *testing.T, s *Service, dir string) (ExportResult, error) {
+	t.Helper()
+	s.dialogs.(*fakeDialogs).folder = dir
+	_, err := s.ChooseExportFolder()
+	must(t, err)
+	return s.ExportFolder(false)
+}
+
 func TestExportMatchesCLIRecipe(t *testing.T) {
 	// Same edits as MpckitT1's pads, through the service.
 	s, _ := newService()
@@ -241,7 +306,7 @@ func TestExportMatchesCLIRecipe(t *testing.T) {
 	_, err = s.SetTune(16, 5, 0)
 	must(t, err)
 	dir := t.TempDir()
-	res, err := s.ExportFolder(dir, false)
+	res, err := exportTo(t, s, dir)
 	must(t, err)
 
 	p, err := project.New("MpckitT1")
@@ -428,7 +493,7 @@ func TestGoldenT1ThroughService(t *testing.T) {
 		_, err := c()
 		must(t, err)
 	}
-	res, err := s.ExportFolder(t.TempDir(), false)
+	res, err := exportTo(t, s, t.TempDir())
 	must(t, err)
 	if !bytes.Equal(gunzip(t, res.Path), gunzip(t, filepath.Join(testdata, "accepted", "MpckitT1.xpj"))) {
 		t.Error("differs from testdata/accepted/MpckitT1.xpj")
@@ -475,7 +540,7 @@ func TestMissingFiles(t *testing.T) {
 
 	// Export names the missing file and writes nothing.
 	out := t.TempDir()
-	if _, err := s.ExportFolder(out, false); err == nil || !strings.Contains(err.Error(), "80s-Snr-Dreamer SA.wav") {
+	if _, err := exportTo(t, s, out); err == nil || !strings.Contains(err.Error(), "80s-Snr-Dreamer SA.wav") {
 		t.Errorf("export with a missing file: %v", err)
 	}
 	if ents, _ := os.ReadDir(out); len(ents) != 0 {
@@ -488,7 +553,7 @@ func TestMissingFiles(t *testing.T) {
 	if st.MissingCount != 0 || st.Pads[1].Missing {
 		t.Error("still missing after replace")
 	}
-	if _, err := s.ExportFolder(out, false); err != nil {
+	if _, err := exportTo(t, s, out); err != nil {
 		t.Error(err)
 	}
 }

pull-requests/unbind-paths/stop-exposing-raw-paths-to-the-window.md

@@ -0,0 +1,57 @@
+# Stop exposing raw paths to the window
+
+Branch: `unbind-paths` → `main`
+
+PR 5 of the review fixes plan (`docs/review-fixes-plan.md`), the first of the 0.5.3 series; the version does not change in this PR.
+
+## What changed
+
+**No bound method takes a path any more (S4).** Every exported method of the Go service can be called from the window. Before, four of them took a path, so any script running there could pass one:
+- `LoadPath` and `AssignSamplePath` could read any file.
+- `ExportFolder` and `ExportZip` could write anywhere. With overwrite set, `ExportFolder` replaced (deleted) a folder of the caller's choosing.
+
+Now:
+- **`LoadPath` and `AssignSamplePath` are no longer bound** (`//wails:ignore`). Go still uses them: `Load` after the open dialog, and `main.go`'s file-drop handler. The frontend never called them.
+- **Export goes where the dialog chose.**
+  - `ChooseExportFolder` and `ChooseZipPath` remember the chosen target on the service. They still return it, so the frontend can tell a cancelled dialog.
+  - `ExportFolder(overwrite)` and `ExportZip(overwrite)` use that target. With nothing chosen, they fail with "choose where to export first".
+  - The confirm-and-overwrite call uses the same target as the first try.
+- **When targets are forgotten:**
+  - after a successful export, so the next export asks again;
+  - on New and Load, so a target chosen for one project is never used for another;
+  - when the dialog is cancelled.
+- **Zip extension:** `.zip` is now added when the zip path is chosen, rather than at export, so the path shown and the path written are the same.
+- **Frontend:** `exportWith` in `frontend/src/lib/store.svelte.ts` passes only `overwrite`. The bindings are regenerated, and the generated `service.ts` has no method with a path parameter.
+
+**Tests.**
+- `TestExport` now goes through the dialogs, and checks that the target is forgotten after a successful export.
+- New `TestExportTarget` covers:
+  - export with nothing chosen fails;
+  - a cancelled dialog clears an earlier choice;
+  - New and Load clear both targets.
+- Other tests use a small `exportTo` helper that chooses the folder in the fake dialog first.
+
+## Notes
+
+- **A target survives a failed export.** It is kept if the export fails for a reason other than "already exists", for example a missing sample, so trying again after fixing the problem does not need the dialog. The plan clears it only after success and on New and Load.
+- **`docs/build-plan.md` is out of date here.** Its API sketch from M4 still lists `AssignSamplePath` and older export signatures. I left the build plan as the historical design. Say if it should be brought up to date.
+- **A GTK warning seen under Xvfb is not new.** In a headless start on the server, two `Gtk-CRITICAL` lines (`gtk_widget_get_display`, `gtk_native_get_surface`) sometimes appear before the window shows: in 3 of 12 runs of this branch and 1 of 18 runs of `main`. The app starts normally either way. This PR does not touch window code, so it looks like a Wails/GTK startup race in a headless session. It has not been seen on Windows.
+
+## Testing
+
+**On the server, every command with `GOTOOLCHAIN=go1.27.4`:**
+- `go test ./...` passes.
+- `go vet ./...` (also with `GOOS=windows`) and `staticcheck ./...` are clean.
+- `govulncheck ./...`: "No vulnerabilities found."
+- `npm install`, `npm run check` (0 errors, 0 warnings) and `npm run build` pass. The lockfile was not committed.
+- `wails3 build` builds, and the app starts under Xvfb.
+
+**On the laptop (PowerShell, in the repo):**
+1. `git fetch; git checkout unbind-paths`
+2. `cd frontend; npm run check; cd ..`: 0 errors, 0 warnings.
+3. `wails3 dev`: assign a WAV to a pad, then choose Export to folder and pick an empty folder. It exports, and the notice shows where.
+4. Export to folder again into the same folder. The "Replace existing files?" prompt lists the `.xpj` and data folder. Choose Replace: it exports. Do it once more and choose Cancel: nothing changes.
+5. Export zip: pick a name without `.zip`. The file is written with `.zip` added. Export zip again to the same name: it asks before replacing.
+6. Cancel the folder dialog: nothing happens and no error shows.
+7. Drop a WAV file onto a pad: it is assigned as before.
+8. Load a project, then export it: the dialog asks for a folder again.