From ad61d92b32870a0a1d7c25e1b991f82c41c0b7a4 Mon Sep 17 00:00:00 2001 From: Developer Date: Thu, 9 Jul 2026 23:40:39 +0000 Subject: [PATCH] spec(per-instance-hook-scoping): add tasks (single slice, ~257 lines) Backend backup endpoint+store filter (subquery for runs/alerts); frontend 6 hooks + 7 API fns + 3 tabs. fetchBackupDashboard excluded (widget path). Each gate green. --- .../per-instance-hook-scoping/tasks.md | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 openspec/changes/per-instance-hook-scoping/tasks.md diff --git a/openspec/changes/per-instance-hook-scoping/tasks.md b/openspec/changes/per-instance-hook-scoping/tasks.md new file mode 100644 index 0000000..4985762 --- /dev/null +++ b/openspec/changes/per-instance-hook-scoping/tasks.md @@ -0,0 +1,139 @@ +# SDD Tasks: Per-Instance Hook Scoping + +**Change:** `per-instance-hook-scoping` +**Phase:** tasks +**Date:** 2026-07-09 + +## Review Workload Forecast + +| Field | Value | +|-------|-------| +| Estimated changed lines | ~257 (frontend-dominant wiring + backend filter) | +| 400-line budget risk | Low | +| Chained PRs recommended | No | +| Suggested split | Single PR | +| Delivery strategy | single-pr | +| Chain strategy | pending | + +```text +Decision needed before apply: No +Chained PRs recommended: No +Chain strategy: pending +400-line budget risk: Low +``` + +--- + +## Slice ordering rationale + +This change is small (~257 lines) and cohesive — all tasks serve one goal (scope observability + backup hooks to `instance.id`). A single slice is well within the 400-line budget. Splitting would create artificial boundaries (e.g. backend filter before frontend wiring) where a half-applied state has no user-visible benefit. + +--- + +## Slice 1: Per-instance hook scoping (full change) + +**Exit gate:** `PYTHONPATH=src python3 -m pytest -q` + `ruff check` green (backend/); `npm run build` + `npm run lint` + `npx vitest run` green (frontend/). All existing tests stay green (non-regression: PI-117, PI-118). No `usePrometheusTargets` / `useMonitoringMachines` / `fetchBackupDashboard` changes. + +### Backend: backup store filter (PI-111) + +- [ ] **1.1 Add `service_id` filter to `list_backup_jobs`** + - Files: `backend/src/media_library_viewer_api/services/settings_store.py` + - Details: Add `service_id: str | None = None` parameter to `list_backup_jobs`. When truthy (non-None, non-empty), append `WHERE service_id = ?` clause + param. When falsy, return all rows (backward-compat). The `backup_jobs` table HAS the `service_id` column (direct WHERE, no subquery needed). (design §4.2 decision 4) + +- [ ] **1.2 Add `service_id` filter to `list_backup_runs` (subquery)** + - Files: `backend/src/media_library_viewer_api/services/settings_store.py` + - Details: Add `service_id: str | None = None` parameter. When truthy, append `job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)` to the `clauses` list + param. The `backup_runs` table has NO `service_id` column (schema asymmetry — design §0 source finding 2), so a subquery through `backup_jobs` is required. When falsy, no filter. (design §4.2 decision 4) + +- [ ] **1.3 Add `service_id` filter to `list_backup_alerts` (subquery)** + - Files: `backend/src/media_library_viewer_api/services/settings_store.py` + - Details: Same subquery pattern as 1.2: `job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)` appended to `clauses` when truthy. The `backup_alerts` table has NO `service_id` column. When falsy, no filter. + +- [ ] **1.4 Add backend tests for backup service scoping** + - Files: `backend/tests/test_backups.py` + - Details: Add `TestBackupServiceScoping` class with: + - `test_list_backup_jobs_filtered_by_service` — seed jobs for svc-a + svc-b; assert `service_id="svc-a"` returns only svc-a's jobs. + - `test_list_backup_jobs_unfiltered_returns_all` — assert `service_id=None` returns all AND `service_id=""` returns all (empty-string treated as no filter). + - `test_list_backup_runs_filtered_by_service` — seed jobs (svc-a + svc-b) each with a run; assert `service_id="svc-a"` returns only svc-a's runs (subquery works). + - `test_list_backup_alerts_filtered_by_service` — seed jobs + alerts; assert `service_id="svc-a"` returns only svc-a's alerts (subquery works). + - `test_list_backup_runs_unfiltered_returns_all` — assert `service_id=None` returns all runs. + (PI-111, PI-119) + +### Backend: backup endpoint threading (PI-110) + +- [ ] **1.5 Add `service_id` query param to backup endpoints** + - Files: `backend/src/media_library_viewer_api/routers/backups.py` + - Details: Add `service_id: str | None = None` to the signatures of `get_backup_jobs`, `get_backup_runs`, `get_backup_alerts`. Thread it into the corresponding `store.list_backup_*` calls as `service_id=service_id`. Do NOT add it to `get_backup_dashboard` (design §5 — excluded, widget path). (PI-110) + +- [ ] **1.6 Add backend test for endpoint threading** + - Files: `backend/tests/test_backups.py` + - Details: Test that `GET /api/backups/jobs?service_id=svc-a` filters correctly via the test client (asserts the param reaches the store). Keep it lightweight — the store-level tests (1.4) are the thorough ones. (PI-110, PI-119) + +### Backend: verify Alertmanager/Prometheus (zero change — PI-112) + +- [ ] **1.7 Confirm Alertmanager/Prometheus endpoints need no change** + - Files: `backend/src/media_library_viewer_api/routers/monitoring.py` (read-only check) + - Details: Verify `get_alertmanager_alerts`, `get_alertmanager_status`, `get_prometheus_status` already accept `service_id: str | None = None` and resolve via `resolve_service_record`. No edit. If source confirms, mark done. (PI-112 — documented asymmetry, zero backend work) + +### Frontend: API client functions (PI-108, PI-109) + +- [ ] **1.8 Add `serviceId` param to observability fetch functions** + - Files: `frontend/src/api/client.ts` + - Details: `fetchAlertmanagerAlerts`, `fetchAlertmanagerStatus`, `fetchPrometheusStatus` each gain `(serviceId?: string)`. Use conditional spread with the existing `get(path, params?)` helper: `serviceId ? { service_id: serviceId } : undefined`. No new URL helper. (PI-108, design §3.1 decision 2) + +- [ ] **1.9 Add `serviceId` param to backup fetch functions (NOT Dashboard)** + - Files: `frontend/src/api/backups.ts` + - Details: `fetchBackupJobs`, `fetchBackupRuns` (3rd arg), `fetchBackupAlerts` (4th arg) each gain `serviceId?: string`. Append `service_id` to the existing params object via conditional spread. `fetchBackupRuns` and `fetchBackupAlerts` already build a params object — append to it. Do NOT modify `fetchBackupDashboard` (design decision 5 — excluded, widget path). (PI-109, design §3.2) + +### Frontend: hooks (PI-101..PI-107) + +- [ ] **1.10 Add `serviceId` to `useObservability` hooks** + - Files: `frontend/src/hooks/useObservability.ts` + - Details: `useAlertmanagerAlerts`, `useAlertmanagerStatus`, `usePrometheusStatus` each gain `(serviceId?: string)`. Include `serviceId ?? ""` as the **last element** of the `queryKey` tuple (design decision 1 — empty-string default for a single stable undefined key, separate key per instance). Change `queryFn` from a direct reference to `() => fetchXxx(serviceId)` (capture serviceId in closure). Do NOT modify `usePrometheusTargets` or `useMonitoringMachines` (PI-107 — global by design). (PI-101, PI-102, PI-103, PI-107) + +- [ ] **1.11 Add `serviceId` to `useBackups` hooks** + - Files: `frontend/src/hooks/useBackups.ts` + - Details: `useBackupJobs` gains `(serviceId?: string)`. `useBackupRuns` gains `(jobId?, status?, serviceId?)`. `useBackupAlerts` gains `(jobId?, acknowledged?, severity?, serviceId?)`. Each includes `serviceId ?? ""` in `queryKey` as the last element. Change `queryFn` to arrow function capturing `serviceId`. Do NOT modify `useBackupDashboard` or `useBackupJob` (design §2.3 — dashboard path, single-ID fetch). (PI-104, PI-105, PI-106) + +### Frontend: tabs pass `instance.id` (PI-113..PI-116) + +- [ ] **1.12 Wire `instance.id` into AlertsTab** + - Files: `frontend/src/pages/service-tabs/AlertsTab.tsx` + - Details: Replace `void instance;` + `useAlertmanagerAlerts()` with `useAlertmanagerAlerts(instance.id)`. Same for `useAlertmanagerStatus(instance.id)`. Remove the TODO comment / file-docstring note about hooks being global/first-configured. (PI-113) + +- [ ] **1.13 Wire `instance.id` into MetricsTab** + - Files: `frontend/src/pages/service-tabs/MetricsTab.tsx` + - Details: Replace `void instance;` + `usePrometheusStatus()` with `usePrometheusStatus(instance.id)`. `usePrometheusTargets()` stays unchanged (global — PI-107). Remove the TODO comment / file-docstring note. (PI-114, PI-107) + +- [ ] **1.14 Wire `instance.id` into JobsTab** + - Files: `frontend/src/pages/service-tabs/JobsTab.tsx` + - Details: Replace `void instance;` + `useBackupJobs()` with `useBackupJobs(instance.id)`. `useBackupRuns()` → `useBackupRuns(undefined, undefined, instance.id)`. `useBackupAlerts(undefined, false)` → `useBackupAlerts(undefined, false, undefined, instance.id)`. Remove the TODO comment / file-docstring note. (PI-115) + +### Frontend: tests (PI-121) + +- [ ] **1.15 Add hook test for per-instance queryKey** + - Files: `frontend/src/hooks/__tests__/useBackups.test.ts` (new or existing) + - Details: Assert that calling a representative hook (e.g. `useBackupJobs`) with different `serviceId` values produces different `queryKey`s. Mock the query client to inspect keys, or use `queryClient.getQueryData` to verify isolation. Also assert `serviceId=undefined` produces a stable key. (PI-121, design §7.2) + +- [ ] **1.16 Add tab test for `instance.id` wiring** + - Files: `frontend/src/pages/service-tabs/__tests__/AlertsTab.test.tsx` (or equivalent existing tab test) + - Details: Assert that at least one tab (e.g. AlertsTab) passes its received `instance.id` to the scoped hook. Cleanest approach: `vi.spyOn(useObservability, "useAlertmanagerAlerts")`, render the tab with a known instance, assert the spy was called with `instance.id`. (PI-121, design §7.2) + +### Integration verification + +- [ ] **1.17 Verify Slice 1 (build + lint + test green)** + - Commands: + - `cd backend && PYTHONPATH=src python3 -m pytest -q` — all pass incl. new `TestBackupServiceScoping` (PI-119) + - `cd backend && PYTHONPATH=src python3 -m ruff check src tests` — clean + - `cd frontend && npm run build` — exit 0 (PI-120) + - `cd frontend && npm run lint` — 0 errors (PI-120) + - `cd frontend && npx vitest run` — all pass incl. new hook + tab tests (PI-121) + - Details: Confirm all existing tests stay green (PI-117 non-regression, PI-118 backward-compat). No widget test modified. `usePrometheusTargets` / `useMonitoringMachines` / `fetchBackupDashboard` unchanged. + +--- + +## Risk flags + +1. **(a) Subquery correctness for runs/alerts.** `backup_runs` and `backup_alerts` do NOT have a `service_id` column (schema asymmetry — design §0 source finding 2). The filter uses `job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)`. Get the SQL right — test it explicitly (1.4). A wrong JOIN/subquery shape would silently filter incorrectly or throw. +2. **(b) Don't accidentally scope the dashboard widget path.** `fetchBackupDashboard` / `useBackupDashboard` / `build_backup_dashboard_summary` stay global (design decision 5). They feed `BackupDashboardWidget` (a dashboard component), NOT `JobsTab`. Scoping them would be a PI-117 regression risk for zero tab benefit. +3. **(c) queryKey consistency.** Every modified hook MUST include `serviceId ?? ""` in its `queryKey` — a forgotten one causes silent cross-instance cache hits. Covered by task 1.15. +4. **(d) `void instance;` removal.** Each tab currently has `void instance;` to suppress the unused-var lint. Removing it + passing `instance.id` removes the need. Ensure no lint regression.