Переглянути джерело

Bind only what the window needs

//wails:ignore only hides a method from the generated bindings; Wails
binds every exported method of Service by reflection, so the window
could still call LoadPath and AssignSamplePath by name (S4). Move the
Go-only methods to an unbound Host that shares the Service's state, and
pin Service's methods with TestBoundMethods. Add CancelExport, called
when the user declines to replace an export, so the target is
forgotten.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude 2 годин тому
батько
коміт
cd198bb23c

+ 14 - 2
frontend/bindings/git.monoexp.com/bajdev/mpckit/internal/app/service.ts

@@ -2,8 +2,11 @@
 // This file is automatically generated. DO NOT EDIT
 
 /**
- * Service is registered with Wails; its exported methods become the
- * frontend bindings.
+ * Service is the type main.go binds. Wails registers every exported method
+ * of a bound type by reflection, whatever the comments say, so the window's
+ * scripts can call each one by name. Service therefore has only the methods
+ * the frontend needs, none of which takes a file path; TestBoundMethods
+ * keeps that list on purpose. Go-only methods are on Host.
  * @module
  */
 
@@ -38,6 +41,15 @@ export function AssignSampleDialog(slot: number): $CancellablePromise<$models.St
     return $Call.ByID(254710842, slot);
 }
 
+/**
+ * CancelExport forgets the chosen export targets. The frontend calls it
+ * when the user declines to replace an existing export, so that a later
+ * ExportFolder(true) or ExportZip(true) cannot overwrite it without asking.
+ */
+export function CancelExport(): $CancellablePromise<void> {
+    return $Call.ByID(1632066973);
+}
+
 /**
  * ChooseExportFolder asks for the folder to export into and remembers it
  * for ExportFolder. It returns the folder, or "" if cancelled.

+ 5 - 1
frontend/src/lib/store.svelte.ts

@@ -161,7 +161,11 @@ async function exportWith(
         items: res.exists,
         action: "Replace",
       });
-      if (!ok) return;
+      if (!ok) {
+        // Go forgets the target, so nothing can overwrite it without asking.
+        await Service.CancelExport();
+        return;
+      }
       res = await write(true);
     }
     app.error = "";

+ 42 - 27
internal/app/app.go

@@ -120,8 +120,11 @@ type Dialogs interface {
 	SaveZip(defaultName string) (string, error)
 }
 
-// Service is registered with Wails; its exported methods become the
-// frontend bindings.
+// Service is the type main.go binds. Wails registers every exported method
+// of a bound type by reflection, whatever the comments say, so the window's
+// scripts can call each one by name. Service therefore has only the methods
+// the frontend needs, none of which takes a file path; TestBoundMethods
+// keeps that list on purpose. Go-only methods are on Host.
 type Service struct {
 	mu      sync.Mutex
 	p       *project.Project
@@ -134,18 +137,28 @@ type Service struct {
 	exportDir, zipPath string
 }
 
+// Host gives main.go what the window must not reach: loading and
+// assigning by path, the dirty flag, the window closer and the audio
+// server. It shares the Service's state but is not bound.
+type Host struct{ s *Service }
+
 // SetCloser sets what CloseWindow calls.
-//
-//wails:ignore
-func (s *Service) SetCloser(f func()) { s.closer = f }
+func (h *Host) SetCloser(f func()) { h.s.closer = f }
 
 // Dirty reports whether the project has changes not yet exported.
-//
-//wails:ignore
-func (s *Service) Dirty() bool {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	return s.p.Dirty()
+func (h *Host) Dirty() bool {
+	h.s.mu.Lock()
+	defer h.s.mu.Unlock()
+	return h.s.p.Dirty()
+}
+
+// LoadPath opens the project at path in place of the current one.
+func (h *Host) LoadPath(path string) (LoadResult, error) { return h.s.loadPath(path) }
+
+// AssignSamplePath puts the WAV at path on the pad in slot. The file drop
+// handler uses it.
+func (h *Host) AssignSamplePath(slot int, path string) (State, error) {
+	return h.s.assignSample(slot, path)
 }
 
 // CloseWindow closes the app without asking about unsaved changes. The
@@ -156,11 +169,12 @@ func (s *Service) CloseWindow() {
 	}
 }
 
-// NewService returns a service holding a new, empty project.
-func NewService(d Dialogs) *Service {
+// NewService returns a service holding a new, empty project, and the Host
+// that shares it. Bind only the Service.
+func NewService(d Dialogs) (*Service, *Host) {
 	s := &Service{dialogs: d}
 	s.reset()
-	return s
+	return s, &Host{s: s}
 }
 
 func (s *Service) reset() {
@@ -290,14 +304,10 @@ func (s *Service) AssignSampleDialog(slot int) (State, error) {
 	if err != nil || path == "" {
 		return s.State(), err
 	}
-	return s.AssignSamplePath(slot, path)
+	return s.assignSample(slot, path)
 }
 
-// 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) {
+func (s *Service) assignSample(slot int, path string) (State, error) {
 	return s.edit(func(p *project.Project) error { return p.AssignSample(slot, path) })
 }
 
@@ -377,6 +387,15 @@ func (s *Service) ExportZip(overwrite bool) (ExportResult, error) {
 	return s.exported(path, err)
 }
 
+// CancelExport forgets the chosen export targets. The frontend calls it
+// when the user declines to replace an existing export, so that a later
+// ExportFolder(true) or ExportZip(true) cannot overwrite it without asking.
+func (s *Service) CancelExport() {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.exportDir, s.zipPath = "", ""
+}
+
 func (s *Service) exported(path string, err error) (ExportResult, error) {
 	var exists *export.ExistsError
 	if errors.As(err, &exists) {
@@ -413,15 +432,11 @@ func (s *Service) Load() (LoadResult, error) {
 	if err != nil || path == "" {
 		return LoadResult{State: s.State()}, err
 	}
-	return s.LoadPath(path)
+	return s.loadPath(path)
 }
 
-// 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) {
+// loadPath opens the project at path in place of the current one.
+func (s *Service) loadPath(path string) (LoadResult, error) {
 	p, warnings, err := project.Load(path)
 	if err != nil {
 		return LoadResult{State: s.State()}, fmt.Errorf("%s: %w", filepath.Base(path), err)

+ 73 - 20
internal/app/app_test.go

@@ -11,6 +11,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"testing"
 
@@ -35,9 +36,13 @@ func (f *fakeDialogs) SaveZip(name string) (string, error) {
 
 func newService() (*Service, *fakeDialogs) {
 	d := &fakeDialogs{}
-	return NewService(d), d
+	s, _ := NewService(d)
+	return s, d
 }
 
+// hostOf returns the Host that shares s, as NewService does.
+func hostOf(s *Service) *Host { return &Host{s: s} }
+
 func must(t *testing.T, err error) {
 	t.Helper()
 	if err != nil {
@@ -110,7 +115,7 @@ func TestPadEdits(t *testing.T) {
 		t.Errorf("after assign: %+v", p)
 	}
 
-	st, err = s.AssignSamplePath(16, kick)
+	st, err = hostOf(s).AssignSamplePath(16, kick)
 	must(t, err)
 	if st.Pads[16].File != "Linn 9000 kick.wav" || st.SampleCount != 1 {
 		t.Error("drop of the same file did not share the pool entry")
@@ -141,7 +146,7 @@ func TestPadEdits(t *testing.T) {
 	}
 
 	// Errors still return the current state.
-	st, err = s.AssignSamplePath(1, filepath.Join(testdata, "nope.wav"))
+	st, err = hostOf(s).AssignSamplePath(1, filepath.Join(testdata, "nope.wav"))
 	if err == nil || st.Pads[16].File == "" {
 		t.Errorf("bad path: %v", err)
 	}
@@ -178,7 +183,7 @@ func TestExport(t *testing.T) {
 	s, d := newService()
 	_, err := s.SetName("Kit")
 	must(t, err)
-	_, err = s.AssignSamplePath(0, filepath.Join(kickDir, "Linn 9000 kick.wav"))
+	_, err = hostOf(s).AssignSamplePath(0, filepath.Join(kickDir, "Linn 9000 kick.wav"))
 	must(t, err)
 
 	d.folder = t.TempDir()
@@ -278,7 +283,7 @@ func TestExportTarget(t *testing.T) {
 		t.Error("New kept the export targets")
 	}
 	choose()
-	_, err = s.LoadPath(filepath.Join(testdata, "device", "DemoBlank.xpj"))
+	_, err = hostOf(s).LoadPath(filepath.Join(testdata, "device", "DemoBlank.xpj"))
 	must(t, err)
 	if s.exportDir != "" || s.zipPath != "" {
 		t.Error("Load kept the export targets")
@@ -299,9 +304,9 @@ func TestExportMatchesCLIRecipe(t *testing.T) {
 	s, _ := newService()
 	_, err := s.SetName("MpckitT1")
 	must(t, err)
-	_, err = s.AssignSamplePath(0, filepath.Join(kickDir, "Linn 9000 kick.wav"))
+	_, err = hostOf(s).AssignSamplePath(0, filepath.Join(kickDir, "Linn 9000 kick.wav"))
 	must(t, err)
-	_, err = s.AssignSamplePath(16, filepath.Join(kickDir, "Linn 9000 kick001.wav"))
+	_, err = hostOf(s).AssignSamplePath(16, filepath.Join(kickDir, "Linn 9000 kick001.wav"))
 	must(t, err)
 	_, err = s.SetTune(16, 5, 0)
 	must(t, err)
@@ -324,11 +329,11 @@ func TestExportMatchesCLIRecipe(t *testing.T) {
 func TestAudio(t *testing.T) {
 	s, _ := newService()
 	kick := filepath.Join(kickDir, "Linn 9000 kick.wav")
-	st, err := s.AssignSamplePath(3, kick)
+	st, err := hostOf(s).AssignSamplePath(3, kick)
 	must(t, err)
 
 	next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("asset")) })
-	h := s.AudioMiddleware(next)
+	h := hostOf(s).AudioMiddleware(next)
 	get := func(url string) *httptest.ResponseRecorder {
 		rec := httptest.NewRecorder()
 		h.ServeHTTP(rec, httptest.NewRequest("GET", url, nil))
@@ -373,11 +378,11 @@ func TestMoveCopySwap(t *testing.T) {
 	s, _ := newService()
 	kick := filepath.Join(kickDir, "Linn 9000 kick.wav")
 	kick2 := filepath.Join(kickDir, "Linn 9000 kick001.wav")
-	_, err := s.AssignSamplePath(0, kick)
+	_, err := hostOf(s).AssignSamplePath(0, kick)
 	must(t, err)
 	_, err = s.SetTune(0, 3, 0)
 	must(t, err)
-	_, err = s.AssignSamplePath(1, kick2)
+	_, err = hostOf(s).AssignSamplePath(1, kick2)
 	must(t, err)
 
 	// Copy across banks keeps the tuning and shares the sample.
@@ -434,7 +439,7 @@ func TestLoad(t *testing.T) {
 	}
 
 	// A bad file keeps the current project.
-	_, err = s.LoadPath(filepath.Join(testdata, "device", "DemoFile_[ProjectData]", "Bass-Pure C.wav"))
+	_, err = hostOf(s).LoadPath(filepath.Join(testdata, "device", "DemoFile_[ProjectData]", "Bass-Pure C.wav"))
 	if err == nil || s.State().Name != "DemoFile" {
 		t.Errorf("bad load: %v", err)
 	}
@@ -482,8 +487,12 @@ func TestGoldenT1ThroughService(t *testing.T) {
 	s, _ := newService()
 	calls := []func() (State, error){
 		func() (State, error) { return s.SetName("MpckitT1") },
-		func() (State, error) { return s.AssignSamplePath(0, filepath.Join(kickDir, "Linn 9000 kick.wav")) },
-		func() (State, error) { return s.AssignSamplePath(16, filepath.Join(kickDir, "Linn 9000 kick001.wav")) },
+		func() (State, error) {
+			return hostOf(s).AssignSamplePath(0, filepath.Join(kickDir, "Linn 9000 kick.wav"))
+		},
+		func() (State, error) {
+			return hostOf(s).AssignSamplePath(16, filepath.Join(kickDir, "Linn 9000 kick001.wav"))
+		},
 		func() (State, error) { return s.SetTune(16, 5, 0) },
 		func() (State, error) { return s.SetSequence(0, "Verse", 96, 8) },
 		func() (State, error) { return s.AddSequence(1) },
@@ -524,7 +533,7 @@ func TestMissingFiles(t *testing.T) {
 	must(t, os.Remove(filepath.Join(dir, "DemoFile"+project.DataSuffix, "80s-Snr-Dreamer SA.wav")))
 
 	s, _ := newService()
-	res, err := s.LoadPath(path)
+	res, err := hostOf(s).LoadPath(path)
 	must(t, err)
 	st := res.State
 	if st.MissingCount != 1 {
@@ -548,7 +557,7 @@ func TestMissingFiles(t *testing.T) {
 	}
 
 	// Replacing the pad's sample fixes it.
-	st, err = s.AssignSamplePath(1, filepath.Join(kickDir, "Linn 9000 kick.wav"))
+	st, err = hostOf(s).AssignSamplePath(1, filepath.Join(kickDir, "Linn 9000 kick.wav"))
 	must(t, err)
 	if st.MissingCount != 0 || st.Pads[1].Missing {
 		t.Error("still missing after replace")
@@ -562,12 +571,12 @@ func TestCloseWindow(t *testing.T) {
 	s, _ := newService()
 	s.CloseWindow() // no closer set: does nothing
 	closed := false
-	s.SetCloser(func() { closed = true })
-	if s.Dirty() {
+	hostOf(s).SetCloser(func() { closed = true })
+	if hostOf(s).Dirty() {
 		t.Error("new project dirty")
 	}
 	_, _ = s.SetName("Other")
-	if !s.Dirty() {
+	if !hostOf(s).Dirty() {
 		t.Error("not dirty after rename")
 	}
 	s.CloseWindow()
@@ -587,9 +596,53 @@ func TestLinkedFileIsMissing(t *testing.T) {
 		t.Skipf("cannot create a symlink: %v", err)
 	}
 	s, _ := newService()
-	res, err := s.LoadPath(path)
+	res, err := hostOf(s).LoadPath(path)
 	must(t, err)
 	if st := res.State; st.MissingCount != 1 || !st.Pads[1].Missing {
 		t.Errorf("missing count %d, A2 missing %v; want 1, true", st.MissingCount, st.Pads[1].Missing)
 	}
 }
+
+// TestBoundMethods pins the methods the window can call. Wails binds every
+// exported method of Service by reflection, so each one is reachable from
+// any script in the window by name.
+func TestBoundMethods(t *testing.T) {
+	want := []string{
+		"About", "AddSequence", "AssignSampleDialog", "CancelExport",
+		"ChooseExportFolder", "ChooseZipPath", "ClearPad", "CloseWindow",
+		"CopyPad", "ExportFolder", "ExportZip", "Load", "MovePad", "New",
+		"RemoveSequence", "SetMuteGroup", "SetName", "SetSequence", "SetTune",
+		"State", "SwapPads",
+	}
+	typ := reflect.TypeOf(&Service{})
+	var got []string
+	for i := range typ.NumMethod() {
+		got = append(got, typ.Method(i).Name)
+	}
+	if strings.Join(got, ",") != strings.Join(want, ",") {
+		t.Errorf("Service's exported methods are\n  %v\nwant\n  %v\n"+
+			"Every exported method of Service can be called by any script in the window. "+
+			"A new one must not take a file path (Go-only methods belong on Host), "+
+			"and must be added to this list on purpose.", got, want)
+	}
+}
+
+func TestCancelExport(t *testing.T) {
+	// Declining "Replace existing files?" forgets the target, so a later
+	// overwrite cannot reach it without the dialog.
+	s, d := newService()
+	d.folder, d.zip = t.TempDir(), filepath.Join(t.TempDir(), "kit.zip")
+	_, err := s.ChooseExportFolder()
+	must(t, err)
+	_, err = s.ChooseZipPath()
+	must(t, err)
+	s.CancelExport()
+	for _, export := range []func(bool) (ExportResult, error){s.ExportFolder, s.ExportZip} {
+		if _, err := export(true); err == nil || err.Error() != "choose where to export first" {
+			t.Errorf("overwrite after cancel: %v", err)
+		}
+	}
+	if ents, _ := os.ReadDir(d.folder); len(ents) != 0 {
+		t.Errorf("export after cancel wrote %d entries", len(ents))
+	}
+}

+ 3 - 5
internal/app/audio.go

@@ -18,16 +18,14 @@ import (
 const AudioPrefix = "/audio/"
 
 // AudioMiddleware serves AudioPrefix and passes every other request on.
-// It is for main.go's asset server, not the frontend.
-//
-//wails:ignore
-func (s *Service) AudioMiddleware(next http.Handler) http.Handler {
+// It is for main.go's asset server.
+func (h *Host) AudioMiddleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if !strings.HasPrefix(r.URL.Path, AudioPrefix) {
 			next.ServeHTTP(w, r)
 			return
 		}
-		s.serveAudio(w, r)
+		h.s.serveAudio(w, r)
 	})
 }
 

+ 5 - 5
main.go

@@ -42,7 +42,7 @@ func init() {
 
 func main() {
 	d := &dialogs{}
-	svc := app.NewService(d)
+	svc, host := app.NewService(d)
 	var a *application.App
 	a = application.New(application.Options{
 		Name:        "MPCkit",
@@ -59,7 +59,7 @@ func main() {
 		},
 		Assets: application.AssetOptions{
 			Handler:    application.AssetFileServerFS(assets),
-			Middleware: svc.AudioMiddleware,
+			Middleware: host.AudioMiddleware,
 		},
 		Mac: application.MacOptions{
 			ApplicationShouldTerminateAfterLastWindowClosed: true,
@@ -95,12 +95,12 @@ func main() {
 	// user chooses to discard them, the frontend calls CloseWindow, which
 	// closes for real.
 	var closeConfirmed atomic.Bool
-	svc.SetCloser(func() {
+	host.SetCloser(func() {
 		closeConfirmed.Store(true)
 		w.Close()
 	})
 	w.RegisterHook(events.Common.WindowClosing, func(e *application.WindowEvent) {
-		if closeConfirmed.Load() || !svc.Dirty() {
+		if closeConfirmed.Load() || !host.Dirty() {
 			return
 		}
 		e.Cancel()
@@ -121,7 +121,7 @@ func main() {
 		}
 		for _, f := range ctx.DroppedFiles() {
 			if strings.EqualFold(filepath.Ext(f), ".wav") {
-				st, err := svc.AssignSamplePath(slot, f)
+				st, err := host.AssignSamplePath(slot, f)
 				a.Event.Emit(eventState, st)
 				if err != nil {
 					a.Event.Emit(eventError, err.Error())

+ 73 - 0
pull-requests/bound-surface/bind-only-what-the-window-needs.md

@@ -0,0 +1,73 @@
+# Bind only what the window needs
+
+Branch: `bound-surface` → `main`
+
+PR 8b of the review fixes plan (`docs/review-fixes-plan.md`), part of the 0.5.3 series; the version does not change in this PR.
+
+## What changed
+
+**The window can no longer reach the Go-only methods (S4, rest).**
+- **The problem:** PR 5 hid `LoadPath` and `AssignSamplePath` with `//wails:ignore`. That only keeps them out of the generated bindings. Wails 3.1.0 binds every exported method of a bound type by reflection, so any script in the window could still call them by name with any path.
+- **Reproduced first:** on `main`, a scratch build confirmed it; see Testing.
+- **The fix:**
+  - **`Service`**, the type `main.go` binds, now has only the 21 methods the frontend calls. None takes a file path.
+  - **The Go-only methods move to a new, unbound `Host`:** `LoadPath`, `AssignSamplePath`, `Dirty`, `SetCloser` and `AudioMiddleware`. `Host` shares the `Service`'s state, and `NewService` returns both.
+  - **Shared helpers:** `Service.Load` and `AssignSampleDialog` call unexported helpers (`loadPath`, `assignSample`) that `Host` uses too.
+  - **No more `//wails:ignore`:** nothing on `Service` needs it now.
+- **`main.go`** uses `Host` for the close hook, the file-drop handler and the audio middleware, and binds only `Service`.
+
+**Declining "Replace existing files?" forgets the target.**
+- Before, Go kept the chosen folder or zip path, so a later `ExportFolder(true)` would have overwritten it without asking.
+- The new bound `CancelExport()` forgets both targets. `exportWith` in `store.svelte.ts` calls it when the user declines.
+
+**Tests.**
+- **`TestBoundMethods`** lists `*Service`'s exported methods by reflection and compares them with an explicit list. If they differ, the message explains:
+  - every exported method can be called by any script in the window;
+  - a new one must not take a file path, since Go-only methods belong on `Host`;
+  - it must be added to the list on purpose.
+  
+  With `LoadPath(string)` temporarily added back to `Service`, it fails as intended.
+- **`TestCancelExport`:** after `CancelExport`, `ExportFolder(true)` and `ExportZip(true)` fail with "choose where to export first" and write nothing.
+- **Existing tests** reach the Go-only methods through `hostOf(s)`.
+
+## Notes
+
+- **The scratch probe.** These lines were appended to `frontend/src/main.ts` and not committed; the file is back to its committed state:
+  - It calls `Call.ByName("git.monoexp.com/bajdev/mpckit/internal/app.Service.LoadPath", <path>)`, and in the fixed build also `AssignSamplePath`.
+  - The webview's console can't be seen in a headless run, so the probe reports through timing, using the bound `CloseWindow`:
+    - if a call succeeds, it closes the app at once;
+    - if every call is rejected with "unknown bound method name", it closes after 6 seconds;
+    - anything else leaves the app running until the 15–20 second timeout.
+  - Startup takes about 4 seconds, so success is about 4 s and rejection about 10 s.
+- **The probe results:**
+
+  | Build | Probe | Result |
+  | --- | --- | --- |
+  | `main` before this PR | `LoadPath` | Closed after **4 s**: the call succeeded and Go loaded the file. |
+  | `main` before this PR | control: a method that does not exist | Closed after **10 s**: rejected as unknown. |
+  | This branch | `LoadPath` and `AssignSamplePath` | Closed after **10 s**, twice: both rejected with "unknown bound method name". |
+- **`CloseWindow` stays bound**, because the frontend needs it after "Close without exporting?". It takes no arguments and only closes the window.
+
+## Testing
+
+**On the server, every command with `GOTOOLCHAIN=go1.27.4`:**
+- `go test ./...` passes, including `TestBoundMethods` and `TestCancelExport`.
+- `go vet ./...` (also with `GOOS=windows`) and `staticcheck ./...` are clean.
+- `govulncheck ./...`: "No vulnerabilities found."
+- In `frontend`: `npm ci`, `npm run check` (0 errors, 0 warnings) and `npm run build`.
+- `wails3 build` builds, and the app starts under Xvfb.
+- The scratch probe, as above.
+
+**On the laptop (PowerShell, in the repo):**
+1. `git fetch; git checkout bound-surface`
+2. `go test ./internal/app`: `ok`.
+3. `wails3 dev`, then:
+   - **Load:** load `testdata\device\DemoFile.xpj`. It opens as before.
+   - **Preview:** click A1, then B2. Each plays.
+   - **File drop:** drop a WAV file onto an empty pad. It is assigned.
+   - **Close prompt:** change something, then close the window. "Close without exporting?" appears. Cancel keeps the app open; choosing to close closes it.
+4. Run `wails3 dev` again, assign a WAV, and export:
+   - **Export to folder** into an empty folder: it exports.
+   - **Export to folder** into the same folder again, and choose **Replace**: it exports.
+   - Once more, and choose **Cancel** at "Replace existing files?": nothing is written, and no error shows.
+   - **Export zip:** the same three steps (new file, Replace, Cancel).