manage/openspec/changes/archive/2026-07-09-per-instance-hook-scoping/design.md

# SDD Design: Per-Instance Hook Scoping

**Change:** `per-instance-hook-scoping`
**Phase:** design
**Date:** 2026-07-09

## 0. Source findings (read before anything else)

The proposal and spec were written against a mental model. Reading actual source surfaced deviations the design must account for. Trust source, not assumptions.

| Spec/proposal claim | Actual source reality | Design impact |
|---|---|---|
| PI-112: "Alertmanager/Prometheus status endpoints already accept `service_id`" | **CONFIRMED.** `get_alertmanager_alerts(service_id: str \| None = None)`, `get_alertmanager_status(service_id: str \| None = None)`, `get_prometheus_status(service_id: str \| None = None)` in `monitoring.py` all resolve via `resolve_service_record(store, "<type>", service_id)`. | Zero backend change for observability. Only frontend hooks + fetch fns need wiring. |
| PI-111: "backup_jobs/backup_runs tables already carry `service_id`" | **PARTIALLY WRONG.** Only `backup_jobs` has a `service_id TEXT` column (added via `ALTER TABLE backup_jobs ADD COLUMN service_id TEXT`). `backup_runs` and `backup_alerts` do **NOT** have `service_id` — they are attributed via the FK chain: `backup_alerts → backup_runs.job_id → backup_jobs.id → backup_jobs.service_id`. | `list_backup_jobs` filters directly. `list_backup_runs` and `list_backup_alerts` must use a subquery or JOIN through `backup_jobs` to filter by service. See §3.2. |
| Spec PI-110: "get_backup_dashboard gains service_id" / PI-109: "fetchBackupDashboard gains serviceId" | **`useBackupDashboard` is NOT used by any instance-scoped tab.** It feeds the old `BackupDashboardWidget` component (dashboard widget path), not JobsTab. The dashboard endpoint is `GET /api/dashboard/backups` → `build_backup_dashboard_summary(store)` in `routers/dashboard.py`. | **EXCLUDED from this change.** Scoping it would require modifying `build_backup_dashboard_summary` and risk PI-117 regression. It is not consumed by AlertsTab/MetricsTab/JobsTab. See §5. |
| Proposal: "use `?service_id=<id>` … use `URLSearchParams` or the existing pattern" | The `get<T>(path, params?)` helper in `api/shared.ts` already takes a `Record<string, string>` params object and builds the URL correctly (handles `?`/`&`, skips undefined/empty). | No new URL helper needed. Fetch fns just add `service_id` to the params object conditionally, matching the existing spread pattern already used by `fetchBackupRuns` / `fetchBackupAlerts`. |
| Spec PI-115: "JobsTab calls useBackupRuns(\<jobId\>, \<status\>, instance.id)" | JobsTab currently calls `useBackupRuns()` with NO arguments (line 34): `const { data: runsData } = useBackupRuns();`. The hook signature is `useBackupRuns(jobId?, status?)`. | The 3rd param (serviceId) is appended after the existing two. JobsTab will call `useBackupRuns(undefined, undefined, instance.id)`. |
| Spec PI-115: "JobsTab calls useBackupAlerts(\<jobId\>, \<acknowledged\>, \<severity\>, instance.id)" | JobsTab calls `useBackupAlerts(undefined, false)` (line 37). Hook signature is `useBackupAlerts(jobId?, acknowledged?, severity?)`. | serviceId is the 4th param. JobsTab will call `useBackupAlerts(undefined, false, undefined, instance.id)`. |

No proposal/spec scope change is required — the *intent* (scope tabs to instance.id) holds. The findings above refine the backend filter mechanism (JOIN vs direct column) and exclude `fetchBackupDashboard`.

---

## 1. Architecture overview

This is a thin wiring change. The backend Alertmanager/Prometheus endpoints already resolve `service_id`. The backend backup endpoints need a small filter addition. The frontend work is: 6 hooks gain an optional `serviceId`, 7 fetch functions pass it as `?service_id=`, and 3 tabs stop using `void instance;` and start passing `instance.id`.

```
BEFORE                                    AFTER
──────                                    ─────
AlertsTab                                 AlertsTab
  └► useAlertmanagerAlerts()  ──┐           └► useAlertmanagerAlerts(instance.id)
  └► useAlertmanagerStatus()    │           └► useAlertmanagerStatus(instance.id)
     hooks: no serviceId        │              hooks: serviceId in queryKey + fetch
     fetch: no service_id       │              fetch: ?service_id=<id>
     backend: ignores it ◄──────┘              backend: resolve_service_record(…, service_id) ✓ already exists

MetricsTab                                MetricsTab
  └► usePrometheusStatus()     ──┐           └► usePrometheusStatus(instance.id)
  └► usePrometheusTargets()      │           └► usePrometheusTargets()        ← stays global
     hooks: no serviceId          │              hook: serviceId in queryKey + fetch
     backend: ignores it ◄────────┘              backend: resolve_service_record(…, service_id) ✓

JobsTab                                   JobsTab
  └► useBackupJobs()           ──┐           └► useBackupJobs(instance.id)
  └► useBackupRuns()             │           └► useBackupRuns(undefined, undefined, instance.id)
  └► useBackupAlerts(undefined, false)       └► useBackupAlerts(undefined, false, undefined, instance.id)
     hooks: no serviceId          │              hooks: serviceId in queryKey + fetch
     fetch: no service_id         │              fetch: ?service_id=<id>
     backend: no filter ◄─────────┘              backend: WHERE backup_jobs.service_id = ? (NEW)
```

Dashboard widgets (`useWidgetData`) are on a completely separate code path and are untouched.

---

## 2. Frontend hook changes (PI-101..PI-107)

**File:** `frontend/src/hooks/useObservability.ts`
**File:** `frontend/src/hooks/useBackups.ts`

### 2.1 Design decision 1 — queryKey shape

Every modified hook appends `serviceId ?? ""` as the **last element** of the `queryKey` tuple:

```typescript
// BEFORE
queryKey: ["observability", "alerts"],
queryFn: fetchAlertmanagerAlerts,

// AFTER
export function useAlertmanagerAlerts(serviceId?: string) {
    return useQuery({
        queryKey: ["observability", "alerts", serviceId ?? ""],
        queryFn: () => fetchAlertmanagerAlerts(serviceId),
        retry: 2,
        staleTime: 10_000,
        refetchInterval: 15_000,
    });
}
```

**Why `serviceId ?? ""` (empty-string default), not omitting it:**

- When `serviceId` is undefined, the key is `["observability", "alerts", ""]`. This is a **single stable key** — all undefined-serviceId callers share one cache entry, matching today's behavior.
- When `serviceId` is `"svc-a"`, the key is `["observability", "alerts", "svc-a"]` — a different key, so a separate cache entry. No cross-instance cache hit.
- This avoids the alternative of conditionally appending (which produces `["observability", "alerts"]` vs `["observability", "alerts", "svc-a"]` — React Query treats these as different-length arrays, which also works, but the empty-string form is more uniform and easier to assert in tests).

**The `queryFn` changes from a direct function reference to an arrow function** because it must capture `serviceId`:

```typescript
queryFn: () => fetchAlertmanagerAlerts(serviceId),
```

### 2.2 All 6 modified hooks (exact signatures)

| Hook | New signature | queryKey (after) |
|---|---|---|
| `useAlertmanagerAlerts` | `(serviceId?: string)` | `["observability", "alerts", serviceId ?? ""]` |
| `useAlertmanagerStatus` | `(serviceId?: string)` | `["observability", "alertmanager-status", serviceId ?? ""]` |
| `usePrometheusStatus` | `(serviceId?: string)` | `["observability", "prometheus-status", serviceId ?? ""]` |
| `useBackupJobs` | `(serviceId?: string)` | `["backups", "jobs", serviceId ?? ""]` |
| `useBackupRuns` | `(jobId?: string, status?: string, serviceId?: string)` | `["backups", "runs", jobId, status, serviceId ?? ""]` |
| `useBackupAlerts` | `(jobId?: string, acknowledged?: boolean, severity?: string, serviceId?: string)` | `["backups", "alerts", jobId, acknowledged, severity, serviceId ?? ""]` |

### 2.3 Hooks deliberately NOT modified (PI-107)

| Hook | Reason |
|---|---|
| `usePrometheusTargets` | Returns Node Exporter scrape targets for external Prom instances via `http_sd_configs` — cross-instance by design. |
| `useMonitoringMachines` | Machines are a global cross-service concept. |
| `useBackupDashboard` | Feeds the dashboard widget path, not any instance-scoped tab. See §5. |
| `useBackupJob` | Fetches a single job by ID (job IDs are globally unique). Not used by JobsTab for list views. |

---

## 3. API client changes (PI-108, PI-109)

**File:** `frontend/src/api/client.ts`
**File:** `frontend/src/api/backups.ts`

### 3.1 Design decision 2 — reuse the existing `get<T>(path, params?)` pattern

No new URL helper. The existing `get<T>` in `shared.ts` takes a `Record<string, string>` and handles `?`/`&`/encoding/empty-skip. Fetch functions use the conditional-spread pattern already proven in `fetchBackupRuns`:

```typescript
// BEFORE (api/client.ts)
export const fetchAlertmanagerAlerts = () =>
    get<AlertmanagerAlertSummary>("/api/monitoring/alerts");

// AFTER
export const fetchAlertmanagerAlerts = (serviceId?: string) =>
    get<AlertmanagerAlertSummary>(
        "/api/monitoring/alerts",
        serviceId ? { service_id: serviceId } : undefined,
    );
```

### 3.2 All 7 modified fetch functions

| Fetch function | File | New param | Pattern |
|---|---|---|---|
| `fetchAlertmanagerAlerts` | `client.ts` | `serviceId?: string` | `serviceId ? { service_id: serviceId } : undefined` |
| `fetchAlertmanagerStatus` | `client.ts` | `serviceId?: string` | same |
| `fetchPrometheusStatus` | `client.ts` | `serviceId?: string` | same |
| `fetchBackupJobs` | `backups.ts` | `serviceId?: string` | same |
| `fetchBackupRuns` | `backups.ts` | `serviceId?: string` (3rd arg) | append to existing params: `{...(jobId ? {job_id: jobId} : {}), ...(status ? {status} : {}), ...(serviceId ? {service_id: serviceId} : {})}` |
| `fetchBackupAlerts` | `backups.ts` | `serviceId?: string` (4th arg) | append to existing params |
| `fetchBackupDashboard` | `backups.ts` | **NOT MODIFIED** | Excluded — see §5 |

---

## 4. Backend backup endpoint + store changes (PI-110, PI-111)

**File:** `backend/src/media_library_viewer_api/routers/backups.py`
**File:** `backend/src/media_library_viewer_api/services/settings_store.py`

### 4.1 Design decision 3 — endpoint signatures

Three endpoints gain `service_id: str | None = None`:

```python
@router.get("/jobs")
def get_backup_jobs(
    service_id: str | None = None,
    store: SettingsStore = Depends(get_settings_store),
) -> list[dict[str, Any]]:
    jobs = store.list_backup_jobs(service_id=service_id)
    return jobs

@router.get("/runs")
def get_backup_runs(
    job_id: str | None = None,
    status: str | None = None,
    limit: int = 50,
    service_id: str | None = None,
    store: SettingsStore = Depends(get_settings_store),
) -> list[BackupRunResponse]:
    runs = store.list_backup_runs(job_id=job_id, status=status, limit=limit, service_id=service_id)
    return [BackupRunResponse(**run) for run in runs]

@router.get("/alerts")
def get_backup_alerts(
    job_id: str | None = None,
    acknowledged: bool | None = None,
    severity: str | None = None,
    service_id: str | None = None,
    store: SettingsStore = Depends(get_settings_store),
) -> list[BackupAlertResponse]:
    alerts = store.list_backup_alerts(job_id=job_id, acknowledged=acknowledged, severity=severity, service_id=service_id)
    return [BackupAlertResponse(**alert) for alert in alerts]
```

### 4.2 Design decision 4 — store filter: direct column for jobs, subquery for runs/alerts

The schema asymmetry (source finding §0) means different filter strategies:

**`list_backup_jobs`** — `backup_jobs` HAS the `service_id` column:

```python
def list_backup_jobs(self, service_id: str | None = None) -> list[dict[str, Any]]:
    self.init_schema()
    where = ""
    params: list[Any] = []
    if service_id:  # truthy = non-None AND non-empty
        where = "WHERE service_id = ?"
        params.append(service_id)
    sql = f"SELECT * FROM backup_jobs {where} ORDER BY created_at DESC"
    with self.connect() as conn:
        rows = conn.execute(sql, params).fetchall()
    return [self._row_to_job(row) for row in rows]
```

**`list_backup_runs`** — `backup_runs` has NO `service_id`. Filter via subquery against `backup_jobs`:

```python
def list_backup_runs(
    self,
    job_id: str | None = None,
    status: str | None = None,
    limit: int = 50,
    service_id: str | None = None,
) -> list[dict[str, Any]]:
    self.init_schema()
    clauses: list[str] = []
    params: list[Any] = []
    if job_id:
        clauses.append("job_id = ?")
        params.append(job_id)
    if status:
        clauses.append("status = ?")
        params.append(status)
    if service_id:
        clauses.append("job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)")
        params.append(service_id)
    where = f"WHERE {' AND '.join(clauses)}" if clauses else ""
    sql = f"SELECT * FROM backup_runs {where} ORDER BY created_at DESC LIMIT ?"
    params.append(max(1, min(int(limit), 200)))
    with self.connect() as conn:
        rows = conn.execute(sql, params).fetchall()
    return [self._row_to_run(row) for row in rows]
```

**`list_backup_alerts`** — `backup_alerts` has NO `service_id`. Same subquery:

```python
def list_backup_alerts(
    self,
    job_id: str | None = None,
    acknowledged: bool | None = None,
    severity: str | None = None,
    service_id: str | None = None,
) -> list[dict[str, Any]]:
    self.init_schema()
    clauses: list[str] = []
    params: list[Any] = []
    if job_id:
        clauses.append("job_id = ?")
        params.append(job_id)
    if acknowledged is not None:
        clauses.append("acknowledged = ?")
        params.append(1 if acknowledged else 0)
    if severity:
        clauses.append("severity = ?")
        params.append(severity)
    if service_id:
        clauses.append("job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)")
        params.append(service_id)
    where = f"WHERE {' AND '.join(clauses)}" if clauses else ""
    sql = f"SELECT * FROM backup_alerts {where} ORDER BY created_at DESC"
    with self.connect() as conn:
        rows = conn.execute(sql, params).fetchall()
    return [self._row_to_alert(row) for row in rows]
```

**Why subquery, not JOIN:** the existing query patterns all use flat `SELECT * FROM <table> WHERE ...`. A subquery is the minimal-diff approach — one extra clause appended to the existing `clauses` list, no structural query change. A JOIN would change the SELECT shape and risk the `_row_to_*` mappers.

**Backward-compat (PI-111):** `if service_id:` is truthy-check — both `None` and `""` skip the filter, returning all rows. Matches spec requirement.

### 4.3 Alertmanager/Prometheus — zero backend change (PI-112, confirmed)

Source verified: `get_alertmanager_alerts`, `get_alertmanager_status`, `get_prometheus_status` in `monitoring.py` all already have `service_id: str | None = None` and call `resolve_service_record(store, "<type>", service_id)`. No change needed.

---

## 5. fetchBackupDashboard excluded (design decision 5)

**`fetchBackupDashboard` / `useBackupDashboard` / `GET /api/dashboard/backups` / `build_backup_dashboard_summary` are NOT modified.**

| Path | Used by | Instance-scoped? | In this change? |
|---|---|---|---|
| `useBackupDashboard` → `fetchBackupDashboard` → `/api/dashboard/backups` → `build_backup_dashboard_summary(store)` | `BackupDashboardWidget` (dashboard component, not a tab) | No — dashboard widgets resolve via `useWidgetData`, not this hook | **Excluded** |

JobsTab does NOT call `useBackupDashboard` — it calls `useBackupJobs`, `useBackupRuns`, `useBackupAlerts` for its three sub-tables. The dashboard summary is a separate aggregation used by the main dashboard. Scoping it would require modifying `build_backup_dashboard_summary` to accept a `service_id` filter and would affect the dashboard widget path — a PI-117 risk for zero tab benefit.

---

## 6. Tab changes (PI-113, PI-114, PI-115, PI-116)

**File:** `frontend/src/pages/service-tabs/AlertsTab.tsx`
**File:** `frontend/src/pages/service-tabs/MetricsTab.tsx`
**File:** `frontend/src/pages/service-tabs/JobsTab.tsx`

The changes are mechanical: remove `void instance;`, remove the TODO docstring/comment, pass `instance.id`.

### 6.1 AlertsTab

```typescript
// BEFORE (lines 82-90)
export function AlertsTab({ instance }: { instance: ServiceInstance }) {
    void instance;
    const { data: alertsSummary, ... } = useAlertmanagerAlerts();
    const { data: status, ... } = useAlertmanagerStatus();

// AFTER
export function AlertsTab({ instance }: { instance: ServiceInstance }) {
    const { data: alertsSummary, ... } = useAlertmanagerAlerts(instance.id);
    const { data: status, ... } = useAlertmanagerStatus(instance.id);
```

Also update the file docstring (lines 1-11): remove "The hooks ... are global / first-configured for now ... Wiring `instance.id` into them is a documented follow-up" and state that hooks are now instance-scoped.

### 6.2 MetricsTab

```typescript
// BEFORE (lines 45-50)
export function MetricsTab({ instance }: { instance: ServiceInstance }) {
    void instance;
    const { data: status, ... } = usePrometheusStatus();
    const { data: targets, ... } = usePrometheusTargets();  // stays global

// AFTER
export function MetricsTab({ instance }: { instance: ServiceInstance }) {
    const { data: status, ... } = usePrometheusStatus(instance.id);
    const { data: targets, ... } = usePrometheusTargets();  // unchanged — global
```

Update the file docstring similarly. Note: `usePrometheusTargets()` stays global (PI-107) — it returns scrape targets for external Prom instances, not instance-scoped UI data.

### 6.3 JobsTab

```typescript
// BEFORE (lines 27-39)
export function JobsTab({ instance }: { instance: ServiceInstance }) {
    void instance;
    const { data: jobsData, isLoading: jobsLoading } = useBackupJobs();
    const { data: runsData, isLoading: runsLoading } = useBackupRuns();
    const { data: alertsData, isLoading: alertsLoading } = useBackupAlerts(
        undefined,
        false,
    );

// AFTER
export function JobsTab({ instance }: { instance: ServiceInstance }) {
    const { data: jobsData, isLoading: jobsLoading } = useBackupJobs(instance.id);
    const { data: runsData, isLoading: runsLoading } = useBackupRuns(undefined, undefined, instance.id);
    const { data: alertsData, isLoading: alertsLoading } = useBackupAlerts(
        undefined,
        false,
        undefined,
        instance.id,
    );
```

Update the file docstring (lines 1-10): remove the NOTE about "hooks currently query globally."

### 6.4 Instance switcher re-scoping (PI-116)

Falls out naturally from the `instance.id` in the hook `queryKey`s. When the `ServicePage` instance switcher changes the active instance, the `instance` prop changes → the hook `queryKey` changes → React Query triggers a fresh fetch for the new key. No additional wiring needed.

---

## 7. Tests (PI-119, PI-120, PI-121)

### 7.1 Backend tests (`test_backups.py` or equivalent)

Add to existing backup tests:

```python
class TestBackupServiceScoping:
    def test_list_backup_jobs_filtered_by_service(self, store):
        # seed jobs for svc-a and svc-b
        store.upsert_backup_job({"name": "job-a", "service_id": "svc-a"})
        store.upsert_backup_job({"name": "job-b", "service_id": "svc-b"})
        assert len(store.list_backup_jobs(service_id="svc-a")) == 1
        assert len(store.list_backup_jobs(service_id="svc-b")) == 1

    def test_list_backup_jobs_unfiltered_returns_all(self, store):
        store.upsert_backup_job({"name": "job-a", "service_id": "svc-a"})
        store.upsert_backup_job({"name": "job-b", "service_id": "svc-b"})
        assert len(store.list_backup_jobs()) == 2           # None
        assert len(store.list_backup_jobs(service_id="")) == 2  # empty string

    def test_list_backup_runs_filtered_by_service(self, store):
        # seed job-a (svc-a) + run, job-b (svc-b) + run
        ...
        assert len(store.list_backup_runs(service_id="svc-a")) == 1

    def test_list_backup_alerts_filtered_by_service(self, store):
        # seed job-a (svc-a) + alert, job-b (svc-b) + alert
        ...
        assert len(store.list_backup_alerts(service_id="svc-a")) == 1
```

### 7.2 Frontend tests

**Hook test** (new test file or existing hook test): assert queryKey differs by serviceId:

```typescript
it("produces different query keys for different serviceIds", () => {
    const { result: a } = renderHook(() => useBackupJobs("svc-a"));
    const { result: b } = renderHook(() => useBackupJobs("svc-b"));
    // queryCache keys differ — mock the query client to inspect keys
    expect(a).toBeDefined();
    expect(b).toBeDefined();
});
```

**Tab test** (update existing tab test or add): assert `instance.id` is passed. The cleanest approach is to mock the hook and assert it receives `instance.id`:

```typescript
it("passes instance.id to scoped hooks", () => {
    const spy = vi.spyOn(useObservability, "useAlertmanagerAlerts");
    render(<AlertsTab instance={{ id: "svc-1", ... }} />);
    expect(spy).toHaveBeenCalledWith("svc-1");
});
```

---

## 8. Slice plan

**Single slice** (~250–350 lines). The change is small and cohesive:

| Area | Files | Est. lines |
|---|---|---|
| Backend store filter | `settings_store.py` (3 methods) | ~40 |
| Backend endpoints | `backups.py` (3 endpoints) | ~12 |
| Backend tests | `test_backups.py` | ~60 |
| Frontend hooks | `useObservability.ts`, `useBackups.ts` | ~30 |
| Frontend API fns | `client.ts`, `backups.ts` | ~20 |
| Frontend tabs | `AlertsTab.tsx`, `MetricsTab.tsx`, `JobsTab.tsx` | ~15 |
| Frontend tests | hook test + tab test | ~80 |
| **Total** | | **~257** |

Fits comfortably within the 400-line review budget. No need to split.

---

## 9. Risks and mitigations

| Risk | Mitigation |
|------|------------|
| **Cache-key collision** if serviceId omitted from queryKey in some hook. | Mandatory: every modified hook includes `serviceId ?? ""` in queryKey. Covered by a hook test asserting different keys for different serviceIds. |
| **Subquery performance** — `job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)`. | Negligible: `backup_jobs` is small (tens of rows in a homelab). The subquery is indexed on PK. No concern. |
| **Backup store filter regresses existing callers.** `list_backup_*` with `service_id=None` must return everything. | Explicit tests for `None` AND `""` (both must return all rows). PI-111 scenarios. |
| **Dashboard widget regression** (widgets use different hooks). | PI-117: widgets use `useWidgetData`, not the modified hooks. Widget tests unchanged. Defensive: grep confirms no widget imports any of the 6 modified hooks. |
| **`build_backup_dashboard_summary` callers.** It calls `store.list_backup_jobs()` with no args — the new default `service_id=None` preserves this. | Backward-compat by design (§4.2 truthy-check). |

---

## Summary of key design decisions

1. **QueryKey:** append `serviceId ?? ""` as last tuple element — single stable key for undefined, separate key per instance.
2. **API client:** reuse `get<T>(path, params?)` with conditional spread — no new helper.
3. **Endpoint signatures:** `service_id: str | None = None` appended to existing params, threaded into store.
4. **Store filter:** direct `WHERE service_id = ?` for `list_backup_jobs`; subquery `job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)` for runs/alerts (schema asymmetry). Truthy-check: both `None` and `""` skip the filter.
5. **`fetchBackupDashboard` excluded:** feeds the dashboard widget path, not any tab. Scoping it would risk PI-117.
6. **Single slice:** ~257 lines, well within budget.