feat(handler): unify ?server= / ?box_id= via resolveActiveBox helper (#112)

Three resolution conventions co-existed pre-#112 Phase 4:

  1. `?server=<id>` — gear-settings deep links and the OS-Updates page.
  2. `?box_id=<id>` — InjectIntegrationStatus middleware (the
     header-pill URL persistence).
  3. `gearbox_active_box` cookie — the sticky pill selection.

Handlers that wanted to honor the pill had to write the same
"check ?server=, then cookie, then default" cascade. Handlers that
followed the middleware convention got `?box_id=` for free but lost
gear-settings links. The resolveBoxIDFromRequest helper only knew
`?server=`.

Changes:

  - resolveBoxIDFromRequest now accepts BOTH `?server=` and `?box_id=`
    as synonyms (precedence: server > box_id > cookie > default).
    Handlers can use whichever name fits the surrounding code without
    losing pill-aware routing.
  - New resolveActiveBox(r) returns the full *models.BoxConfig along
    with a presence boolean, so handlers that need an agent URL or
    APIKey don't have to chase a separate getServerConfig call. Folds
    the "single helper for active box" invariant from the issue
    proposal into one call site.

Four tests cover the new contract: ?server= path, ?box_id= path,
precedence when both supplied, and the full-BoxConfig return.

Phase 4 of #112.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox/internal/framework/handler/handler.go

@@ -277,19 +277,29 @@ func (h *Handler) getDefaultServerID() string {
 }
 
 // resolveBoxIDFromRequest picks the box ID to operate on, in priority:
-//  1. `?server=<id>` query param — explicit per-link override.
+//  1. `?server=<id>` or `?box_id=<id>` query param — explicit per-link
+//     override. Both names accepted as synonyms so handlers can use
+//     whichever convention the surrounding code prefers; the middleware
+//     in InjectIntegrationStatus uses `?box_id=` for URL-persisted
+//     pill switches, while older gear-settings links use `?server=`.
 //  2. `gearbox_active_box` cookie — the header pill's selection.
 //  3. The first enabled server — first-login fallback.
 //
 // Previously the Gears settings handlers only consulted `?server=` and fell
 // straight through to the first server when it was missing, which made the
 // page show the first box's gears even while the header pill was on a
 // different box (issue #71 item 1). Reading the cookie aligns these
-// handlers with the pill that's actually visible to the user.
+// handlers with the pill that's actually visible to the user. Recognizing
+// `?box_id=` as a synonym keeps the box-resolver consistent across the
+// dashboard's handler / middleware / gear-plugin layers (issue #112
+// Phase 4).
 func (h *Handler) resolveBoxIDFromRequest(r *http.Request) string {
 	if id := r.URL.Query().Get("server"); id != "" {
 		return id
 	}
+	if id := r.URL.Query().Get("box_id"); id != "" {
+		return id
+	}
 	if c, err := r.Cookie(activeBoxCookieName); err == nil && c.Value != "" {
 		// Only honor the cookie if it still resolves to an enabled server —
 		// stale cookies (deleted/disabled boxes) shouldn't dictate behavior.
@@ -302,6 +312,24 @@ func (h *Handler) resolveBoxIDFromRequest(r *http.Request) string {
 	return h.getDefaultServerID()
 }
 
+// resolveActiveBox returns the full BoxConfig for the box the request is
+// acting on, using resolveBoxIDFromRequest for resolution. Returns
+// (nil, false) when no enabled box matches the resolved ID — this is
+// the all-boxes / no-box context (e.g. /bx, /settings).
+//
+// Handlers that need an agent client, an agent URL, or other BoxConfig
+// fields should prefer this over resolveBoxIDFromRequest + a separate
+// getServerConfig call: it makes the "single helper for active box"
+// invariant from issue #112 Phase 4 visible in the code, and it folds
+// the existence check into one call site.
+func (h *Handler) resolveActiveBox(r *http.Request) (*models.BoxConfig, bool) {
+	boxID := h.resolveBoxIDFromRequest(r)
+	if boxID == "" {
+		return nil, false
+	}
+	return h.getServerConfig(boxID)
+}
+
 // activeBoxCookieName is the cookie key that persists the user's selected
 // box across navigations. Lets gear links (e.g. /haproxy) drop the verbose
 // `?box_id=` query string and still resolve the active context.

gearbox/internal/framework/handler/resolve_box_test.go

@@ -0,0 +1,79 @@
+package handler
+
+import (
+	"net/http/httptest"
+	"testing"
+
+	"github.com/sarg3nt/gearbox/internal/framework/models"
+)
+
+// newResolveTestHandler builds the minimum Handler state the resolver
+// needs for the URL-query-param paths: a static `servers` slice (so the
+// cookie path can validate against enabled servers if exercised). Tests
+// that exercise the cookie or first-server fallback would also need a
+// DB; those branches aren't covered here.
+func newResolveTestHandler(servers ...models.BoxConfig) *Handler {
+	return &Handler{servers: servers}
+}
+
+// TestResolveBoxIDFromRequest_ServerParam covers the historical
+// `?server=<id>` path — gear-settings links and the OS-Updates page
+// still emit this form.
+func TestResolveBoxIDFromRequest_ServerParam(t *testing.T) {
+	h := newResolveTestHandler()
+	r := httptest.NewRequest("GET", "/anything?server=mjolnir", nil)
+	if got := h.resolveBoxIDFromRequest(r); got != "mjolnir" {
+		t.Errorf("resolveBoxIDFromRequest with ?server=mjolnir = %q, want %q", got, "mjolnir")
+	}
+}
+
+// TestResolveBoxIDFromRequest_BoxIDParam covers the synonym alias added
+// in issue #112 Phase 4. `?box_id=` is the form the header-pill
+// middleware writes; pre-Phase-4 the handler-level resolver ignored it
+// because it only knew `?server=`. Recognizing both keeps the resolver
+// consistent across the handler / middleware / gear-plugin layers.
+func TestResolveBoxIDFromRequest_BoxIDParam(t *testing.T) {
+	h := newResolveTestHandler()
+	r := httptest.NewRequest("GET", "/anything?box_id=mjolnir", nil)
+	if got := h.resolveBoxIDFromRequest(r); got != "mjolnir" {
+		t.Errorf("resolveBoxIDFromRequest with ?box_id=mjolnir = %q, want %q", got, "mjolnir")
+	}
+}
+
+// TestResolveBoxIDFromRequest_ServerWinsOverBoxID is the precedence
+// guard: when a request happens to carry both (rare — typically only
+// when an operator hand-edits a URL), `?server=` wins because it's the
+// older, more-explicit convention used by gear-settings deep links.
+func TestResolveBoxIDFromRequest_ServerWinsOverBoxID(t *testing.T) {
+	h := newResolveTestHandler()
+	r := httptest.NewRequest("GET", "/anything?server=alpha&box_id=beta", nil)
+	if got := h.resolveBoxIDFromRequest(r); got != "alpha" {
+		t.Errorf("resolveBoxIDFromRequest with both params = %q, want %q", got, "alpha")
+	}
+}
+
+// TestResolveActiveBox_ReturnsServerConfig verifies that the new
+// resolveActiveBox returns the full BoxConfig (not just an ID) by
+// matching against the handler's static `servers` slice.
+func TestResolveActiveBox_ReturnsServerConfig(t *testing.T) {
+	h := newResolveTestHandler(
+		models.BoxConfig{ID: "mjolnir", Name: "Mjolnir", AgentURL: "https://10.0.0.1:8405"},
+	)
+	r := httptest.NewRequest("GET", "/anything?box_id=mjolnir", nil)
+	box, ok := h.resolveActiveBox(r)
+	if !ok {
+		t.Fatalf("resolveActiveBox = (_, false), want a BoxConfig")
+	}
+	if box.ID != "mjolnir" || box.AgentURL != "https://10.0.0.1:8405" {
+		t.Errorf("resolveActiveBox returned %+v, want {ID:mjolnir, AgentURL:https://10.0.0.1:8405}", box)
+	}
+}
+
+// The "resolved ID doesn't match any configured server" branch isn't
+// covered here because exercising it would require a DB stub —
+// getServerConfig falls back to getEnabledServers (a DB read) when the
+// static list misses, and the DB layer panics on nil. The static-list
+// happy path above guards the helper's primary contract (an ID that
+// matches a configured server returns the right BoxConfig); the
+// dynamic-server path is exercised by existing handler integration
+// tests that run against the full Handler fixture.