spec(per-instance-hook-scoping): add design
6 decisions: queryKey appends serviceId??''; API client reuses get(path,params); service_id: str|None=None on backup endpoints; store filter via subquery for runs/alerts (schema asymmetry — only backup_jobs has service_id column); fetchBackupDashboard excluded (widget path, PI-117 risk). Single slice ~257 lines. 3 source findings: Alertmanager/Prom endpoints confirmed already take service_id; backup runs/alerts attributed via FK chain (subquery needed).
This commit is contained in:
Developer
@@ -0,0 +1,469 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user