Fix -ui-dev serving 502 for every page

libs/services/web_server.go

@@ -86,10 +86,14 @@ func (s *WebServer) SetupRoutes(r *gin.Engine) error {
 		s.logger.LogAttrs(context.Background(), slog.LevelInfo, "setting up HTTP server routes to proxy to UI dev server")
 
 		target, _ := url.Parse("http://localhost:4199")
-		proxy := httputil.NewSingleHostReverseProxy(target)
-		proxy.Rewrite = func(pr *httputil.ProxyRequest) {
-			pr.SetURL(target)
-			pr.Out.Host = target.Host
+		// Built with Rewrite alone rather than from NewSingleHostReverseProxy:
+		// that constructor sets Director, and ReverseProxy refuses to serve
+		// anything when both are set, which turned every page into a 502.
+		proxy := &httputil.ReverseProxy{
+			Rewrite: func(pr *httputil.ProxyRequest) {
+				pr.SetURL(target)
+				pr.Out.Host = target.Host
+			},
 		}
 
 		r.NoRoute(func(c *gin.Context) {

libs/services/web_server_test.go

@@ -0,0 +1,129 @@
+package services
+
+import (
+	"context"
+	"io"
+	"log/slog"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/gin-gonic/gin"
+)
+
+func testLogger() *slog.Logger {
+	return slog.New(slog.DiscardHandler)
+}
+
+// newTestWebServer builds a WebServer and returns the gin engine its routes
+// were installed on, so requests can be driven without binding a port.
+func newTestWebServer(t *testing.T, config WebServerConfig) (*gin.Engine, error) {
+	t.Helper()
+	gin.SetMode(gin.TestMode)
+
+	if config.SetupAPIRoutes == nil {
+		config.SetupAPIRoutes = func(*gin.RouterGroup) {}
+	}
+
+	server := &WebServer{config: config, logger: testLogger()}
+	engine := gin.New()
+	if err := server.SetupRoutes(engine); err != nil {
+		return nil, err
+	}
+	return engine, nil
+}
+
+// TestDevModeProxiesNonAPIPaths covers the branch taken by -ui-dev: everything
+// that is not /api goes to the vite dev server instead of to disk.
+//
+// It runs against a real server because ReverseProxy takes a different path
+// when the request context has no Done channel, which is what a synthetic
+// httptest request has -- and that difference is part of why the broken proxy
+// went unnoticed for so long.
+func TestDevModeProxiesNonAPIPaths(t *testing.T) {
+	engine, err := newTestWebServer(t, WebServerConfig{DevMode: true})
+	if err != nil {
+		t.Fatalf("SetupRoutes: %v", err)
+	}
+
+	server := httptest.NewServer(engine)
+	defer server.Close()
+
+	get := func(path string) (int, string) {
+		t.Helper()
+		request, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL+path, nil)
+		if err != nil {
+			t.Fatalf("new request: %v", err)
+		}
+		response, err := server.Client().Do(request)
+		if err != nil {
+			t.Fatalf("GET %s: %v", path, err)
+		}
+		defer response.Body.Close()
+		body, err := io.ReadAll(response.Body)
+		if err != nil {
+			t.Fatalf("read body: %v", err)
+		}
+		return response.StatusCode, string(body)
+	}
+
+	// API paths are answered here rather than forwarded: the dev server serves
+	// the UI, and the API belongs to this process.
+	if status, body := get("/api/unknown"); status != http.StatusNotFound {
+		t.Errorf("api status = %d, want 404 (body %s)", status, body)
+	}
+
+	// The dev-server target is hardcoded, so standing in for it means binding
+	// that exact port. Skipping when it is taken keeps the suite usable on a
+	// machine that happens to be running the real dev server.
+	listener, err := net.Listen("tcp", "localhost:4199")
+	if err != nil {
+		t.Skipf("cannot bind the dev server port to stand in for vite: %v", err)
+	}
+
+	var gotHost, gotPath atomic.Pointer[string]
+	devServer := &http.Server{
+		ReadHeaderTimeout: 5 * time.Second,
+		Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			host, path := r.Host, r.URL.Path
+			gotHost.Store(&host)
+			gotPath.Store(&path)
+			_, _ = io.WriteString(w, "vite")
+		}),
+	}
+	go func() { _ = devServer.Serve(listener) }()
+	defer func() {
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		_ = devServer.Shutdown(ctx)
+	}()
+
+	status, body := get("/some/page")
+	if status != http.StatusOK || body != "vite" {
+		t.Fatalf("page = %d %q, want the dev server's response", status, body)
+	}
+	if got := gotPath.Load(); got == nil || *got != "/some/page" {
+		t.Errorf("dev server saw path %v, want /some/page", got)
+	}
+	// Host is rewritten to the target so the dev server sees a request for
+	// itself rather than for the rotom listener.
+	if got := gotHost.Load(); got == nil || *got != "localhost:4199" {
+		t.Errorf("dev server saw Host %v, want localhost:4199", got)
+	}
+}
+
+// TestSetupRoutesRejectsNoUISource covers the other way SetupRoutes can fail,
+// so a change to it cannot pass on the dev-mode path alone.
+func TestSetupRoutesRejectsNoUISource(t *testing.T) {
+	_, err := newTestWebServer(t, WebServerConfig{})
+	if err == nil {
+		t.Fatal("SetupRoutes succeeded with no UI at all, want an error")
+	}
+	if !strings.Contains(err.Error(), "no embedded UI") {
+		t.Errorf("error = %v, want it to name the missing UI", err)
+	}
+}