Skip to content

Commit d1e32b5

Browse files
sarg3ntclaude
andcommitted
fix(bx): address Copilot review findings on #61
- statusMonitor.Start now guards against multi-start with sync.Once (matches its docstring; previously spawned a new goroutine per call). - bx.Gear.Start passes its lifecycle ctx through to the monitor instead of context.Background(), so framework shutdown actually cancels it. - The per-probe 5s budget is now enforced: probe uses agent.NewClientWithTimeout(..., m.timeout) instead of the default 30s agent client. Removed the unused pctx that the old comment promised. - canViewIntegration("bx") now checks bx:view via auth.HasPermissionFromContext, matching the handler-side gate and the SidebarItem.RequiresPermission — so the sidebar no longer renders a Bx link for users who'd just get a 403 clicking it. - Fix stale comment on boxTable: the page reconciles via the SSE snapshot pushed on (re)connect, not a /bx/api/status fetch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1d1652c commit d1e32b5

4 files changed

Lines changed: 17 additions & 17 deletions

File tree

gearbox/internal/framework/templates/layouts/base.templ

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1758,7 +1758,11 @@ func firstEnabledIntegrationPath(ctx context.Context) string {
17581758
func canViewIntegration(ctx context.Context, integrationName string) bool {
17591759
switch integrationName {
17601760
case "bx":
1761-
return true // Bx (fleet view) is the box switcher's home — always visible.
1761+
// Bx (fleet view) is gated on bx:view in handlers and the sidebar
1762+
// config (SidebarItem.RequiresPermission); mirror that here so the
1763+
// sidebar link doesn't render for users who'd get a 403 clicking it.
1764+
// `models.Component("bx")` matches the gear-declared permission name.
1765+
return auth.HasPermissionFromContext(ctx, models.Component("bx"), models.PermissionView)
17621766
case "home":
17631767
return true // Home is visible to all authenticated users when enabled.
17641768
case "metrics":

gearbox/internal/gears/bx/gear.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func (g *Gear) Initialize(ctx context.Context, deps gear.Dependencies) error {
5757
// Start launches the per-box status poller.
5858
func (g *Gear) Start(ctx context.Context) error {
5959
if g.monitor != nil {
60-
g.monitor.Start(context.Background())
60+
g.monitor.Start(ctx)
6161
}
6262
return nil
6363
}

gearbox/internal/gears/bx/pages.templ

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,8 @@ templ emptyState() {
129129
}
130130

131131
// boxTable renders the fleet view. Server-side render is the source of truth
132-
// for initial paint; the page's JS reconciles rows with /bx/api/status and
133-
// listens on /bx/api/events for live updates.
132+
// for initial paint; the page's JS subscribes to /bx/api/events (SSE), which
133+
// pushes a full snapshot on every (re)connect and live transitions thereafter.
134134
//
135135
// Column order is deliberately minimal: status dot, name, location, agent
136136
// host, latency, last-checked. Anything heavier (per-box CPU/mem

gearbox/internal/gears/bx/status.go

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,9 @@ type statusMonitor struct {
5757
mu sync.RWMutex
5858
statuses map[string]BoxStatus // keyed by BoxConfig.ID (UUID string)
5959

60-
stop chan struct{}
61-
once sync.Once
60+
stop chan struct{}
61+
startOnce sync.Once
62+
stopOnce sync.Once
6263

6364
subsMu sync.Mutex
6465
subs map[int64]chan BoxStatus
@@ -83,14 +84,14 @@ func newStatusMonitor(deps gear.Dependencies) *statusMonitor {
8384
}
8485

8586
// Start launches the polling loop. Safe to call multiple times — only the
86-
// first call wins.
87+
// first call wins; subsequent calls are no-ops.
8788
func (m *statusMonitor) Start(ctx context.Context) {
88-
go m.run(ctx)
89+
m.startOnce.Do(func() { go m.run(ctx) })
8990
}
9091

9192
// Stop signals the polling loop to exit. Subsequent calls are no-ops.
9293
func (m *statusMonitor) Stop() {
93-
m.once.Do(func() { close(m.stop) })
94+
m.stopOnce.Do(func() { close(m.stop) })
9495
}
9596

9697
func (m *statusMonitor) run(ctx context.Context) {
@@ -164,14 +165,9 @@ func (m *statusMonitor) probe(ctx context.Context, b *database.BoxDB, apiKey str
164165
return bs
165166
}
166167

167-
// Per-probe context bounded by the timeout; do not let a single hung
168-
// agent stall the whole poll cycle.
169-
pctx, cancel := context.WithTimeout(ctx, m.timeout)
170-
defer cancel()
171-
_ = pctx // reserved for when the agent client supports ctx; the http
172-
// client's per-request timeout already provides the upper bound.
173-
174-
client := agent.NewClient(b.AgentURL, apiKey)
168+
// Per-probe timeout bounds the HTTP request itself so a single hung
169+
// agent can't stall the poll cycle past m.timeout.
170+
client := agent.NewClientWithTimeout(b.AgentURL, apiKey, m.timeout)
175171
t0 := time.Now()
176172
_, err := client.Health()
177173
bs.LatencyMs = time.Since(t0).Milliseconds()

0 commit comments

Comments
 (0)