From 4f56a9c5d411ae6f1a39b07ff1942237e4321514 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:01:44 +0100 Subject: [PATCH 01/19] docs: YAML config editor spec + implementation plan (issue #95) --- .../plans/2026-11-03-yaml-config-editor.md | 185 ++++++++++++++++++ .../2026-11-03-yaml-config-editor-design.md | 97 +++++++++ 2 files changed, 282 insertions(+) create mode 100644 docs/superpowers/plans/2026-11-03-yaml-config-editor.md create mode 100644 docs/superpowers/specs/2026-11-03-yaml-config-editor-design.md diff --git a/docs/superpowers/plans/2026-11-03-yaml-config-editor.md b/docs/superpowers/plans/2026-11-03-yaml-config-editor.md new file mode 100644 index 0000000..c88f12b --- /dev/null +++ b/docs/superpowers/plans/2026-11-03-yaml-config-editor.md @@ -0,0 +1,185 @@ +# YAML Config Editor Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Edit the active `.profiles` YAML in the web UI via a full-screen CodeMirror 6 modal, applying in memory first and saving to disk only through a validated, explicit save. + +**Architecture:** 5 new routes on the existing `/api/config` FastAPI router, all funneling validation through one helper on top of `ConfigReader.from_content`; the frontend adds a `ConfigEditorModal` reusing the drop-config `load-content` endpoint for Apply. + +**Tech Stack:** FastAPI (existing router), CodeMirror 6 (`codemirror`, `@codemirror/lang-yaml`), React 19 + vitest, pytest. + +**Spec:** `docs/superpowers/specs/2026-11-03-yaml-config-editor-design.md` + +## Global Constraints + +- Extension whitelist for Save As: exactly `.profiles` and `.profiles.yaml` (spec value). +- `POST /validate` always returns HTTP 200; validity is carried in the body. +- Bad YAML never touches disk unless the request carries `force: true`. +- Save As refuses an existing target unless `overwrite: true` is present. +- New code imports from `profiles.core` / its subpackages, not the `config.py` shim. +- Web theming via `data-theme` tokens only (never Tailwind `dark:`); new overlays use z-scale tokens. +- `dist/` rebuilt (`npm run build` in `src/profiles/web`) and committed in the final task. +- Python style: type hints, docstrings on public functions, ruff format/check clean, pytest green. + +## Review Focus + +1. **Session-trial save refusal** — active config from a drop has no disk path; Save must 400, not silently write to a CWD-resolved path. (Task 2 pins `_has_disk_path`.) +2. **Overwrite without flag** — Save As onto an existing file must fail (409) without `overwrite: true`. (Task 4) +3. **Force-save writes invalid content** — the file is written verbatim; a later `GET /content` returns it and Test/Apply report the errors. (Task 2 + Task 5) +4. **Template round-trip** — `STARTER_CONFIG_TEMPLATE.format(...)` output must pass the project's own `/validate` (brace-escaping history — see memory "config template escaping gotchas"). (Task 3) +5. **Dirty-escape guard** — closing with unsaved edits warns instead of losing them. (Task 6) + +--- + +### Task 1: Validation helper + `POST /api/config/validate` + +**Files:** +- Modify: `src/profiles/server/routes/config.py` +- Test: `tests/server/routes/test_config_editor.py` (create; fixture pattern follows `tests/server/routes/test_config_reload.py`) + +**Interfaces:** +- Produces: `validate_issues(content: str, display_name: str) -> list[dict[str, str]]` in `routes/config.py` — returns `[]` when `ConfigReader.from_content` parses, else `[{"severity": "error", "message": }]`. Tasks 2–4 consume it. + +- [ ] **Step 1: Write failing tests** — `test_validate_returns_valid_true_for_starter_template` (template rendered with `STARTER_CONFIG_TEMPLATE.format(cwd=str(tmp_path))`, expect `{"valid": True, "issues": []}`), `test_validate_returns_issues_for_bad_yaml` (`content="defaults: [unclosed"`, expect `valid=False` and `issues[0]["severity"] == "error"` with a non-empty message), `test_validate_does_not_change_state` (assert `state.config` is unchanged after the call). +- [ ] **Step 2: Run** `pytest tests/server/routes/test_config_editor.py -v` — expect FAIL (404 / unknown route). +- [ ] **Step 3: Implement** `validate_issues` + route `POST /validate` with pydantic `ValidateRequest(content: str, filename: str)` returning `{"valid": not issues, "issues": issues}`. +- [ ] **Step 4: Run tests** — PASS. Also run `pytest tests/server/routes/test_config.py -v` (no regression). +- [ ] **Step 5: Commit** `feat(server): config validate endpoint for the YAML editor` + +### Task 2: `GET /api/config/content` + session-trial detection + +**Files:** +- Modify: `src/profiles/server/routes/config.py` +- Test: `tests/server/routes/test_config_editor.py` + +**Interfaces:** +- Produces: `has_disk_path(state: ServerState) -> bool` — True iff `state.config_path` or `state.config.config_path` is set (i.e. NOT a drop-config session trial). Response shape: `{"filename": str, "content": str, "config_path": str | null, "session_trial": bool}`. + +- [ ] **Step 1: Failing tests** — `test_content_returns_active_config_text` (load a real temp `.profiles` via `ConfigReader` into state; assert content matches file text and `session_trial is False`), `test_content_flags_session_trial` (state where only `state.config` is set, no path anywhere; assert `session_trial is True` and `config_path is None`), `test_content_for_missing_file_returns_text_or_error` (path recorded but file deleted — decide + pin one behavior: 404 with detail). +- [ ] **Step 2: Run** — FAIL. +- [ ] **Step 3: Implement** `has_disk_path` + `GET /content` reading `state.active_config_path().read_text(encoding="utf-8")` when it exists. +- [ ] **Step 4: Run** — PASS. +- [ ] **Step 5: Commit** `feat(server): config content endpoint + session-trial detection` + +### Task 3: `POST /api/config/save` (validate → force → write) + +**Files:** +- Modify: `src/profiles/server/routes/config.py` +- Test: `tests/server/routes/test_config_editor.py` + +**Interfaces:** +- Consumes: `validate_issues`, `has_disk_path` (Tasks 1–2). +- Produces: `SaveRequest(content: str, force: bool = False)`; success → `{"status": "SAVED", "path": str, "config": }`; re-reads the saved file via `_apply_config_from_disk` so the live config equals what is on disk. + +- [ ] **Step 1: Failing tests** — `test_save_refused_for_session_trial` (no disk path → 400, nothing written), `test_save_rejects_invalid_content_without_writing` (file bytes unchanged after 400), `test_save_force_writes_invalid_content` (`force=True` → 200, file contains the invalid text), `test_save_writes_and_applies_valid_config` (valid content → 200, file written, `state.config` reflects new content). +- [ ] **Step 2: Run** — FAIL. +- [ ] **Step 3: Implement** the route; gate order: `has_disk_path` check → `validate_issues` → if issues and not `force`: return 400 with the issues list → write → `_apply_config_from_disk`. +- [ ] **Step 4: Run** — PASS. +- [ ] **Step 5: Commit** `feat(server): validated config save endpoint with force escape hatch` + +### Task 4: `POST /api/config/save-as` + +**Files:** +- Modify: `src/profiles/server/routes/config.py` +- Test: `tests/server/routes/test_config_editor.py` + +**Interfaces:** +- Consumes: `validate_issues` (Task 1), `_apply_config_from_disk` (existing). +- Produces: `SaveAsRequest(content: str, filename: str, directory: str, force: bool = False, overwrite: bool = False)`; success → same shape as save. Extension whitelist constant `SAVE_AS_EXTENSIONS = (".profiles", ".profiles.yaml")`. + +- [ ] **Step 1: Failing tests** — `test_save_as_rejects_bad_extension` (`.yml` → 400), `test_save_as_rejects_existing_target_without_overwrite` (409 or 400 — pin 409), `test_save_as_overwrite_replaces_file`, `test_save_as_rejects_invalid_content` (no file created), `test_save_as_force_saves_invalid_content`, `test_save_as_applies_new_config` (after success, `state.active_directory` re-derived, `state.config_path == target`). +- [ ] **Step 2: Run** — FAIL. +- [ ] **Step 3: Implement** — path via `normalize_user_path`; validation gate identical to save; on success write then `_apply_config_from_disk(state, target)`. +- [ ] **Step 4: Run** — PASS + full `pytest tests/server/routes -q` green. +- [ ] **Step 5: Commit** `feat(server): save-as endpoint with extension whitelist and overwrite guard` + +### Task 5: Web API client + types + +**Files:** +- Modify: `src/profiles/web/src/api/types.ts`, `src/profiles/web/src/api/client.ts` +- Test: `src/profiles/web/src/api/client.test.ts` (extend) + +**Interfaces:** +- Consumes: the five routes (Tasks 1–4). Error convention matches existing client helpers (400 detail surface). +- Produces: `fetchConfigContent(): Promise`; `validateConfigContent(filename: string, content: string): Promise<{valid: boolean; issues: ValidationIssue[]}>`; `fetchConfigTemplate(): Promise<{content: string; filename: string}>`; `saveConfigFile(content: string, force: boolean): Promise`; `saveConfigFileAs(params: {content: string; filename: string; directory: string; force: boolean; overwrite: boolean}): Promise`. New types: `ConfigContentData`, `ValidationIssue {severity: "error" | "warning"; message: string}`. + +- [ ] **Step 1: Failing client tests** — one per function asserting method + URL + body passthrough (same mock style as existing `client.test.ts`). +- [ ] **Step 2: Run** `npx vitest run src/api/client.test.ts` (from `src/profiles/web`) — FAIL. +- [ ] **Step 3: Implement** functions + types. +- [ ] **Step 4: Run** — PASS. +- [ ] **Step 5: Commit** `feat(web): API client for config editor endpoints` + +### Task 6: `ConfigEditorModal` — CodeMirror shell, load, dirty/escape guard + +**Files:** +- Create: `src/profiles/web/src/components/ConfigEditorModal.tsx` +- Modify: `src/profiles/web/package.json` (add `codemirror`, `@codemirror/lang-yaml`) +- Test: `src/profiles/web/src/components/ConfigEditorModal.test.tsx` + +**Interfaces:** +- Consumes: `fetchConfigContent` (Task 5). +- Produces: `ConfigEditorModalProps {open: boolean; onClose: () => void; onApplied?: (data: AppConfigData) => void}` — Tasks 7–8 extend this component; App integration is Task 9. Full-screen overlay using the existing modal z-scale token class (pattern from `BarcodeModal.tsx`). + +- [ ] **Step 1: Install deps** — `npm install codemirror @codemirror/lang-yaml` in `src/profiles/web`. +- [ ] **Step 2: Failing component tests** — `test_renders_fullscreen_overlay_when_open`, `test_loads_content_on_open` (mock `fetchConfigContent`, assert editor text set), `test_dirty_indicator_and_escape_warning` (simulate change; press Escape; expect confirm dialog instead of close; decline → stays open), `test_clean_close_without_warning`. +- [ ] **Step 3: Implement** — CodeMirror 6 `EditorView` with `yaml()` language; theme object built from CSS variable tokens (read via `getComputedStyle` of the mounted root or hardcode token var() references in the theme spec); toolbar placeholder row (buttons wired in Tasks 7–8); Escape/✕ handler comparing current doc to loaded text. Modal header bar includes an **"Open in OS"** button calling the existing `openConfigFile()` client function (spec addition 2026-11-03: user asked for an OS-launch button in the modal head bar); test step 2 adds `test_header_open_in_os_calls_open_config_file`. +- [ ] **Step 4: Run** `npx vitest run` — all web tests PASS. +- [ ] **Step 5: Commit** `feat(web): config editor modal shell with CodeMirror and dirty guard` + +### Task 7: Toolbar — New, Test, Revert, Apply + +**Files:** +- Modify: `src/profiles/web/src/components/ConfigEditorModal.tsx` +- Test: `src/profiles/web/src/components/ConfigEditorModal.test.tsx` + +**Interfaces:** +- Consumes: `fetchConfigTemplate`, `validateConfigContent`, `loadConfigContent` (Task 5 + existing). +- Produces: internal state contract reused by Task 8: `status` banner (`{kind: "ok" | "error"; text: string} | null`), `issues: ValidationIssue[]`, `dirty: boolean`. + +- [ ] **Step 1: Failing tests** — `test_new_loads_template_after_confirm` (dirty → ConfirmDialog first), `test_new_without_dirty_loads_immediately`, `test_test_shows_valid_badge`, `test_test_shows_issue_list` (mock invalid result; assert each issue message rendered), `test_apply_calls_load_content_and_reports_success` (assert `onApplied` fired with response), `test_apply_error_shows_server_message`. +- [ ] **Step 2: Run** — FAIL. +- [ ] **Step 3: Implement** the four buttons + inline banner + issue list above the editor. +- [ ] **Step 4: Run** — PASS. +- [ ] **Step 5: Commit** `feat(web): editor toolbar actions New/Test/Revert/Apply` + +### Task 8: Toolbar — Save, Save As with compliance gate and force + +**Files:** +- Modify: `src/profiles/web/src/components/ConfigEditorModal.tsx` +- Create: `src/profiles/web/src/components/SaveAsDialog.tsx` +- Test: `src/profiles/web/src/components/ConfigEditorModal.test.tsx`, `SaveAsDialog.test.tsx` + +**Interfaces:** +- Consumes: `saveConfigFile`, `saveConfigFileAs`, `browseDirectory` (existing, for the directory picker). +- Produces: `SaveAsDialogProps {open; onCancel; onConfirm(params: {filename: string; directory: string; overwrite: boolean})}`; gate helper in ConfigEditorModal: validate → if issues, render issue list + "Go back and fix" / "Force save anyway" buttons. + +- [ ] **Step 1: Failing tests** — modal: `test_save_validates_then_saves`, `test_save_invalid_shows_gate_with_force_option`, `test_force_save_proceeds`, `test_save_disabled_without_disk_path` (session trial → Save hidden/disabled, tooltip explains; `Cmd/Ctrl+S` falls back to Apply). Dialog: `test_lists_directories_via_browse`, `test_filename_extension_restricted` (input accepts only `.profiles`/`.profiles.yaml` suffix — append/normalize on confirm), `test_overwrite_confirmation` (existing file in browse result → "Replace?" confirm before `onConfirm(..., overwrite: true)`), `test_cancel`. +- [ ] **Step 2: Run** — FAIL. +- [ ] **Step 3: Implement** — Save button + gate UI; `SaveAsDialog` reusing the browse-result pattern from `DirectoryPickerModal.tsx`; keyboard binding `Mod-s`. +- [ ] **Step 4: Run** — PASS (full vitest suite). +- [ ] **Step 5: Commit** `feat(web): save/save-as with compliance gate, force option and replace confirmation` + +### Task 9: App integration + documentation + dist + +**Files:** +- Modify: `src/profiles/web/src/App.tsx` — the editor opens from the **existing StatusBar config button** (the `onOpenConfig` prop currently OS-launching the file at `App.tsx:1550`): repurpose it to `setShowConfigEditor(true)`; OS-launch moves into the modal header ("Open in OS", Task 6). +- Modify: `docs/configuration-profile.en.md`, `docs/configuration-profile.fr.md`, `docs/advanced/guide-avance.en.md`, `docs/advanced/guide-avance.fr.md`, `README.md`, `README.fr.md` +- Test: `src/profiles/web/src/components/ConfigEditorModal.test.tsx` (one integration test) + +**Interfaces:** +- Consumes: `ConfigEditorModal` (Tasks 6–8), `AppConfigData.onConfigChanged` path used by `LoadConfigModal.tsx` for refreshing the app after config swaps. + +- [ ] **Step 1: Failing integration test** — `test_status_bar_config_button_opens_editor_and_apply_refreshes_app`: click the StatusBar config button, modal opens, Apply → app state refresh callback invoked. +- [ ] **Step 2: Run** — FAIL. +- [ ] **Step 3: Implement** the StatusBar entry (repurposed `onOpenConfig`) + wire `onApplied` to the same refresh path `LoadConfigModal` uses. +- [ ] **Step 4: Docs** — user-guide section (toolbar incl. "Open in OS", two-step model, gate/force, Save-as extensions); `--init` docs pointer to **New**; advanced guide endpoint list; README bullets (en/fr, keep the 🇬🇧/🇫🇷 switcher pattern). +- [ ] **Step 5: Run** vitest suite PASS; run `uv run pytest -q` full suite PASS; `uv run ruff format . && uv run ruff check .` clean. +- [ ] **Step 6: Build + commit** — `npm run build` in `src/profiles/web`; commit `feat(web): YAML config editor in the UI` including `dist/`. + +--- + +## Self-Review Notes + +- Spec coverage: all 5 endpoints (Tasks 1–4 + reuse), 6 toolbar actions (Tasks 7–8), gate/force (Task 8), overwrite confirm (Task 8), dirty guard (Task 6), docs (Task 9), dist (Task 9). +- `GET /content` on a deleted file: behavior pinned in Task 2 Step 1 (404) rather than left open. +- Severity model: `issues` carries `severity` from day one; only `"error"` is produced today (spec note honored). diff --git a/docs/superpowers/specs/2026-11-03-yaml-config-editor-design.md b/docs/superpowers/specs/2026-11-03-yaml-config-editor-design.md new file mode 100644 index 0000000..8c785f1 --- /dev/null +++ b/docs/superpowers/specs/2026-11-03-yaml-config-editor-design.md @@ -0,0 +1,97 @@ +# YAML Config Editor — Design + +Date: 2026-11-03 +Status: Approved (brainstorming session, same day) + +## Intent + +Edit the active `.profiles` YAML configuration file directly in the web +front-end, with a two-step safety model: edits are first applied **in +memory** (session trial, reusing the drop-config pipeline), and written to +disk only through an explicit **Save** that is gated by server-side +compliance validation. + +## Decisions made + +- **Editor technology**: CodeMirror 6 (`codemirror`, `@codemirror/lang-yaml`) + — lightweight (~150 KB), fits the PWA, themed from the existing + `data-theme` tokens (both light and dark). Monaco (~2 MB) rejected as too + heavy; plain textarea rejected (no highlighting). +- **UX**: full-screen modal, same pattern as the barcode/camera modals + (z-scale token convention). +- **Save model**: apply-in-memory first, save-to-disk as an explicit second + step (option B). + +## Backend — new endpoints on `/api/config` + +1. `GET /content` — raw YAML text + filename + `config_path`; + `session_trial: true` when the active config came from a drop (no disk + path). +2. `POST /validate` — body `{content, filename}` → `200 {valid, issues: + [{severity, message}]}`. Always 200; validity carried in the body so the + modal renders issues and buttons uniformly. Parses via + `ConfigReader.from_content`; no state change. Powers the **Test** button + and the save gates. + - Note: the reader currently only produces *errors* (one `ValueError` + message); `issues` is shaped for future severity levels. The + error/force flow works on errors today. +3. `POST /template` — returns `STARTER_CONFIG_TEMPLATE.format(cwd=active + directory)` rendered in memory; nothing written. Powers **New** (same + content as `--init`). +4. `POST /save` — validate → optional `force` flag → write to + `config_path`. 400 for session-trial configs (no path). Bad YAML never + touches the file without `force`. +5. `POST /save-as` — `{content, filename, directory, overwrite}`. + - `filename` restricted to `.profiles` / `.profiles.yaml` extensions. + - Validated content first; refuses an existing target unless + `overwrite: true` (client confirms "Replace?" first). + - On success it **also applies** the saved config and re-derives the + active directory (`_choose_active_directory`, same path as `/load`). + +**Apply** reuses the existing `POST /load-content` unchanged. + +## Frontend — full-screen modal + +Toolbar: **New** (template; confirms if dirty) · **Test** (badge ✓ valid / +✗ with inline issue list) · **Apply** (session-trial apply via +`load-content`; 400 message shown inline) · **Save** · **Save as** (dialog: +directory browse via existing `/api/fs` + filename input, extension +constraint) · **Revert** (reload from server). Dirty dot in the title; +Escape/✕ warns on unsaved edits; `Cmd/Ctrl+S` → Save (falls back to Apply +when there is no disk path). + +**Save / Save as compliance gate**: server validate first → if issues, +show the issue list and offer "Go back and fix" / "Force save anyway"; +force skips the gate and writes. + +**Save as overwrite**: existing target → "Replace?" confirmation; endpoint +requires explicit `overwrite: true`. + +## Non-goals + +- No form-based GUI editor. +- No schema autocomplete for v1 (server-validated lint feedback is enough). +- No file picker for arbitrary configs (edits the active config only). + +## Tests + +- pytest route tests for the 5 new endpoints (valid/invalid/force/overwrite/ + extension restriction/session-trial refusal). +- vitest for the modal component: open/load, Test error path, Apply, + Save gating, force-save path, Save-as dialog, dirty-escape warning. + +## Documentation (en + fr) + +- User guide: Config Editor section (toolbar, two-step model, compliance + gate, force-save, allowed Save-as extensions). +- `--init` / starter-template docs: pointer to **New** in the UI. +- `docs/advanced/guide-avance.fr.md` + en counterpart: the new + `/api/config/*` endpoints for alternative front-ends. +- README feature bullet (en/fr). + +## Left to the builder + +- Exact split of validation helpers shared between `validate` / `save` / + `save-as` routes (DRY: one helper on top of `ConfigReader.from_content`). +- CodeMirror theme mapping to existing tokens. +- Route test fixtures consistent with `tests/AGENTS.md`. From f2bc348699faec4b8ed9a8a43c6a38c8d0bbff27 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:05:27 +0100 Subject: [PATCH 02/19] feat(server): config validate endpoint for the YAML editor --- src/profiles/server/routes/config.py | 39 ++++++++++++ tests/server/routes/test_config_editor.py | 75 +++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 tests/server/routes/test_config_editor.py diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index c3b2ba4..8de7aef 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -34,6 +34,11 @@ class CreateConfigRequest(BaseModel): directory: str +class ValidateRequest(BaseModel): + content: str + filename: str + + def _is_config_path(path: Path) -> bool: return path.name.endswith(CONFIG_FILE_NAMES) @@ -134,6 +139,29 @@ def _apply_config_from_disk(state: ServerState, config_path: Path) -> dict: return _config_response(state) +def validate_issues(content: str, display_name: str) -> list[dict[str, str]]: + """Validate configuration *content* in memory without touching live state. + + Parses *content* strictly with :meth:`ConfigReader.from_content` and + reports the outcome as a plain issue list, so the YAML editor can + validate content that is not applied to the session. + + Args: + content: Raw YAML configuration content. + display_name: Human-readable file name used in error messages. + + Returns: + ``[]`` when the content parses and validates, otherwise a single + ``{"severity": "error", "message": ...}`` dict describing the + failure. + """ + try: + ConfigReader.from_content(content, display_name=display_name) + except ValueError as exc: + return [{"severity": "error", "message": str(exc)}] + return [] + + @router.get("") async def get_config(request: Request) -> dict: """Return active application configuration and available directories.""" @@ -247,6 +275,17 @@ async def reload_config_endpoint(request: Request) -> dict: return response +@router.post("/validate") +async def validate_config_endpoint(payload: ValidateRequest) -> dict: + """Validate editor content in memory; never applied to the session state. + + Returns ``{"valid": bool, "issues": [...]}`` rather than an HTTP error + status, so invalid drafts reach the editor as data to display. + """ + issues = validate_issues(payload.content, display_name=payload.filename) + return {"valid": not issues, "issues": issues} + + @router.post("/select-directory") async def select_directory(payload: SelectDirectoryRequest, request: Request) -> dict: """Update active directory.""" diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py new file mode 100644 index 0000000..96a98ab --- /dev/null +++ b/tests/server/routes/test_config_editor.py @@ -0,0 +1,75 @@ +"""Tests for POST /api/config/validate — validate editor content in memory.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from httpx import ASGITransport, AsyncClient + +from profiles.core.config.models import AppConfig +from profiles.core.config.template import STARTER_CONFIG_TEMPLATE +from profiles.server.app import create_app +from profiles.server.state import ServerState + + +@pytest.fixture +def config_state(tmp_path: Path) -> ServerState: + config = AppConfig( + title="Editor Session", + search_dir=str(tmp_path), + extensions=(".txt",), + ) + config.config_path = tmp_path / ".profiles" + return ServerState(config=config, active_directory=tmp_path) + + +@pytest.mark.anyio +async def test_validate_returns_valid_true_for_starter_template( + config_state: ServerState, tmp_path: Path +) -> None: + """Verify a freshly rendered starter template validates with no issues.""" + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/validate", + json={ + "content": STARTER_CONFIG_TEMPLATE.format(cwd=str(tmp_path)), + "filename": ".profiles", + }, + ) + + assert response.status_code == 200 + assert response.json() == {"valid": True, "issues": []} + + +@pytest.mark.anyio +async def test_validate_returns_issues_for_bad_yaml(config_state: ServerState) -> None: + """Verify invalid YAML yields a valid=False payload with an error issue.""" + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/validate", + json={"content": "defaults: [unclosed", "filename": ".profiles"}, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["valid"] is False + assert payload["issues"][0]["severity"] == "error" + assert payload["issues"][0]["message"] + + +@pytest.mark.anyio +async def test_validate_does_not_change_state(config_state: ServerState) -> None: + """Verify validation is read-only: the live configuration is untouched.""" + original = config_state.config + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/validate", + json={"content": "defaults: [unclosed", "filename": ".profiles"}, + ) + + assert response.status_code == 200 + assert config_state.config is original From 7fe10a5d67a95ca737dc42ed5dcdeb5b7f95ac70 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:15:53 +0100 Subject: [PATCH 03/19] feat(server): config content endpoint + session-trial detection --- src/profiles/server/routes/config.py | 47 +++++++++++++++++ tests/server/routes/test_config_editor.py | 63 ++++++++++++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index 8de7aef..6f55fdd 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -162,6 +162,20 @@ def validate_issues(content: str, display_name: str) -> list[dict[str, str]]: return [] +def has_disk_path(state: ServerState) -> bool: + """Return True when the active configuration is backed by a recorded file. + + A configuration applied from dropped content (``POST /load-content``) + is a session trial: neither the state nor the loaded + :class:`AppConfig` carries a recorded disk path, so there is no file + to save edits back to. Save endpoints gate on this helper to refuse + writing to a merely CWD-resolved path instead of the trial's source. + """ + if state.config_path: + return True + return bool(getattr(state.config, "config_path", None)) + + @router.get("") async def get_config(request: Request) -> dict: """Return active application configuration and available directories.""" @@ -286,6 +300,39 @@ async def validate_config_endpoint(payload: ValidateRequest) -> dict: return {"valid": not issues, "issues": issues} +@router.get("/content") +async def get_config_content(request: Request) -> dict: + """Return the active configuration file's text for the YAML editor. + + Responds ``{"filename", "content", "config_path", "session_trial"}``. + A session trial (config applied from dropped content, no recorded + path) answers an empty payload flagged ``session_trial=True`` instead + of file text; a recorded-but-missing file answers 404. + """ + state: ServerState = request.app.state.server_state + if not has_disk_path(state): + return { + "filename": "", + "content": "", + "config_path": None, + "session_trial": True, + } + + config_path = state.active_config_path() + if not config_path.exists(): + raise HTTPException( + status_code=404, + detail=f"Configuration file not found: {config_path}", + ) + + return { + "filename": config_path.name, + "content": config_path.read_text(encoding="utf-8"), + "config_path": str(config_path), + "session_trial": False, + } + + @router.post("/select-directory") async def select_directory(payload: SelectDirectoryRequest, request: Request) -> dict: """Update active directory.""" diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py index 96a98ab..b56b706 100644 --- a/tests/server/routes/test_config_editor.py +++ b/tests/server/routes/test_config_editor.py @@ -1,4 +1,4 @@ -"""Tests for POST /api/config/validate — validate editor content in memory.""" +"""Tests for the config editor endpoints: validate + content.""" from __future__ import annotations @@ -8,6 +8,7 @@ from httpx import ASGITransport, AsyncClient from profiles.core.config.models import AppConfig +from profiles.core.config.reader import ConfigReader from profiles.core.config.template import STARTER_CONFIG_TEMPLATE from profiles.server.app import create_app from profiles.server.state import ServerState @@ -73,3 +74,63 @@ async def test_validate_does_not_change_state(config_state: ServerState) -> None assert response.status_code == 200 assert config_state.config is original + + +@pytest.mark.anyio +async def test_content_returns_active_config_text(tmp_path: Path) -> None: + """Verify GET /content returns the on-disk text of the active config file.""" + config_path = tmp_path / ".profiles" + file_text = 'version: 1\ndefaults:\n title: "On Disk"\n' + config_path.write_text(file_text, encoding="utf-8") + loaded = ConfigReader(config_path).load() + state = ServerState(config=loaded, active_directory=tmp_path) + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.get("/api/config/content") + + assert response.status_code == 200 + payload = response.json() + assert payload["content"] == file_text + assert payload["filename"] == ".profiles" + assert payload["config_path"] == str(config_path) + assert payload["session_trial"] is False + + +@pytest.mark.anyio +async def test_content_flags_session_trial(tmp_path: Path) -> None: + """Verify a config with no recorded disk path is reported as a session trial.""" + config = AppConfig( + title="Trial Session", + search_dir=str(tmp_path), + extensions=(".txt",), + ) + # A config applied from dropped content carries no recorded disk path: + # neither the state nor the AppConfig tracks the file it came from. + config.config_path = None + state = ServerState(config=config, active_directory=tmp_path) + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.get("/api/config/content") + + assert response.status_code == 200 + payload = response.json() + assert payload["session_trial"] is True + assert payload["config_path"] is None + + +@pytest.mark.anyio +async def test_content_for_missing_file_returns_text_or_error( + config_state: ServerState, +) -> None: + """Pinned behavior: a recorded-but-deleted config file answers 404.""" + missing_path = config_state.active_config_path() + assert not missing_path.exists() + + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.get("/api/config/content") + + assert response.status_code == 404 + assert str(missing_path) in response.json()["detail"] From e06c39b81bdd89389bbe21511123b453b56ba3d8 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:20:06 +0100 Subject: [PATCH 04/19] fix(server): explicit session-trial flag so save never anchors to a stale path --- src/profiles/server/routes/config.py | 32 ++++++++++++++--------- src/profiles/server/state.py | 3 +++ tests/server/routes/test_config_editor.py | 31 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index 6f55fdd..58d03a4 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -134,6 +134,7 @@ def _apply_config_from_disk(state: ServerState, config_path: Path) -> dict: loaded = ConfigReader(config_path).load() state.config = loaded + state.session_trial = False state.config_path = config_path state.active_directory = _choose_active_directory(loaded, config_path) return _config_response(state) @@ -165,15 +166,17 @@ def validate_issues(content: str, display_name: str) -> list[dict[str, str]]: def has_disk_path(state: ServerState) -> bool: """Return True when the active configuration is backed by a recorded file. - A configuration applied from dropped content (``POST /load-content``) - is a session trial: neither the state nor the loaded - :class:`AppConfig` carries a recorded disk path, so there is no file - to save edits back to. Save endpoints gate on this helper to refuse - writing to a merely CWD-resolved path instead of the trial's source. + The explicit :attr:`ServerState.session_trial` flag takes precedence + over path presence: a configuration applied from dropped content + (``POST /load-content``) is a session trial with no disk anchor, even + though the shared ``_config_response`` helper re-records a resolved + path into the state for every response (existing shared behavior). + Save endpoints gate on this helper to refuse writing to a merely + CWD-resolved path instead of the trial's source. """ - if state.config_path: - return True - return bool(getattr(state.config, "config_path", None)) + return not state.session_trial and ( + state.config_path is not None or getattr(state.config, "config_path", None) is not None + ) @router.get("") @@ -225,6 +228,7 @@ async def load_content_endpoint(payload: LoadContentRequest, request: Request) - raise HTTPException(status_code=400, detail=str(exc)) from exc state.config = loaded + state.session_trial = True state.active_directory = _choose_active_directory(loaded, None) response = _config_response(state) response["session_trial"] = True @@ -249,6 +253,7 @@ async def create_config_endpoint(payload: CreateConfigRequest, request: Request) loaded = ConfigReader(target_config).load() state.config = loaded + state.session_trial = False state.config_path = target_config state.active_directory = _choose_active_directory(loaded, target_config) return _config_response(state) @@ -282,6 +287,7 @@ async def reload_config_endpoint(request: Request) -> dict: loaded.config_path = config_path state.config = loaded + state.session_trial = False state.config_path = config_path state.active_directory = _choose_active_directory(loaded, config_path) response = _config_response(state) @@ -305,14 +311,16 @@ async def get_config_content(request: Request) -> dict: """Return the active configuration file's text for the YAML editor. Responds ``{"filename", "content", "config_path", "session_trial"}``. - A session trial (config applied from dropped content, no recorded - path) answers an empty payload flagged ``session_trial=True`` instead - of file text; a recorded-but-missing file answers 404. + A session trial (config applied from dropped content) answers an + empty body flagged ``session_trial=True`` — ``config_path`` stays + ``None`` so the stale CWD-resolved path never leaks, and ``filename`` + carries the config's display name — instead of file text; a + recorded-but-missing file answers 404. """ state: ServerState = request.app.state.server_state if not has_disk_path(state): return { - "filename": "", + "filename": state.config.title or "session-trial", "content": "", "config_path": None, "session_trial": True, diff --git a/src/profiles/server/state.py b/src/profiles/server/state.py index e521705..45f8be4 100644 --- a/src/profiles/server/state.py +++ b/src/profiles/server/state.py @@ -36,6 +36,9 @@ class ServerState: config: AppConfig active_directory: Path | None = None config_path: Path | None = None + # True while the active config is an in-memory trial from a + # dropped/loaded content payload (no disk anchor). + session_trial: bool = False # Shared secret verifying handoff requests from a second ProFiles # launch (issue #4); None disables the identity/handoff endpoints. instance_token: str | None = None diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py index b56b706..a26a9f4 100644 --- a/tests/server/routes/test_config_editor.py +++ b/tests/server/routes/test_config_editor.py @@ -11,6 +11,7 @@ from profiles.core.config.reader import ConfigReader from profiles.core.config.template import STARTER_CONFIG_TEMPLATE from profiles.server.app import create_app +from profiles.server.routes.config import has_disk_path from profiles.server.state import ServerState @@ -120,6 +121,36 @@ async def test_content_flags_session_trial(tmp_path: Path) -> None: assert payload["config_path"] is None +@pytest.mark.anyio +async def test_content_flags_session_trial_after_live_drop( + config_state: ServerState, +) -> None: + """Pin the live flow: a dropped config must never expose a disk anchor. + + Ruling R4: ``_config_response`` re-records a resolved path into the + state even for a drop, so trial detection must ride the explicit + ``ServerState.session_trial`` flag — not path absence. + """ + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + drop = await client.post( + "/api/config/load-content", + json={"filename": "my.profiles", "content": "defaults:\n title: Dropped\n"}, + ) + assert drop.status_code == 200 + assert drop.json()["session_trial"] is True + + assert has_disk_path(config_state) is False + + response = await client.get("/api/config/content") + + assert response.status_code == 200 + payload = response.json() + assert payload["session_trial"] is True + assert payload["config_path"] is None + assert payload["filename"] == "Dropped" + + @pytest.mark.anyio async def test_content_for_missing_file_returns_text_or_error( config_state: ServerState, From ca9d3c6ae346bf326a642bf618f3106c44ffc9ed Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:29:38 +0100 Subject: [PATCH 05/19] feat(server): validated config save endpoint with force escape hatch --- src/profiles/server/routes/config.py | 39 +++++++ tests/server/routes/test_config_editor.py | 118 +++++++++++++++++++++- 2 files changed, 156 insertions(+), 1 deletion(-) diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index 58d03a4..69f126b 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -39,6 +39,11 @@ class ValidateRequest(BaseModel): filename: str +class SaveRequest(BaseModel): + content: str + force: bool = False + + def _is_config_path(path: Path) -> bool: return path.name.endswith(CONFIG_FILE_NAMES) @@ -341,6 +346,40 @@ async def get_config_content(request: Request) -> dict: } +@router.post("/save") +async def save_config_endpoint(payload: SaveRequest, request: Request) -> dict: + """Write editor content to the active config file and apply it. + + Gate order: session-trial refusal (:func:`has_disk_path`) → + :func:`validate_issues` → 400 with the issues list when invalid and + not ``force`` → write the file → :func:`_apply_config_from_disk`, so + the live configuration is always re-read from what is on disk. + ``force=True`` is the escape hatch that lets a broken draft reach the + disk (it will load as defaults until fixed); without it, bad YAML + never touches the file. + + Returns ``{"status": "SAVED", "path": str, "config": }``; + the apply step clears ``session_trial``, anchoring the saved config + to its file. + """ + state: ServerState = request.app.state.server_state + if not has_disk_path(state): + raise HTTPException( + status_code=400, + detail="Cannot save: the active configuration is a session trial " + "with no file on disk. Load or create a configuration first.", + ) + + config_path = state.active_config_path() + issues = validate_issues(payload.content, display_name=config_path.name) + if issues and not payload.force: + raise HTTPException(status_code=400, detail=issues) + + config_path.write_text(payload.content, encoding="utf-8") + config = _apply_config_from_disk(state, config_path) + return {"status": "SAVED", "path": str(config_path), "config": config} + + @router.post("/select-directory") async def select_directory(payload: SelectDirectoryRequest, request: Request) -> dict: """Update active directory.""" diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py index a26a9f4..262c467 100644 --- a/tests/server/routes/test_config_editor.py +++ b/tests/server/routes/test_config_editor.py @@ -1,4 +1,4 @@ -"""Tests for the config editor endpoints: validate + content.""" +"""Tests for the config editor endpoints: validate + content + save.""" from __future__ import annotations @@ -165,3 +165,119 @@ async def test_content_for_missing_file_returns_text_or_error( assert response.status_code == 404 assert str(missing_path) in response.json()["detail"] + + +@pytest.mark.anyio +async def test_save_refused_for_session_trial(config_state: ServerState, tmp_path: Path) -> None: + """Verify save is refused (400) for a session trial and writes nothing. + + Given: A config applied from dropped content (no disk anchor) + When: POST /save is called + Then: 400 is returned and no file appears on disk + """ + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + drop = await client.post( + "/api/config/load-content", + json={"filename": "my.profiles", "content": "defaults:\n title: Dropped\n"}, + ) + assert drop.status_code == 200 + assert has_disk_path(config_state) is False + + response = await client.post( + "/api/config/save", + json={"content": "defaults:\n title: Saved\n"}, + ) + + assert response.status_code == 400 + assert not (tmp_path / ".profiles").exists() + + +@pytest.mark.anyio +async def test_save_rejects_invalid_content_without_writing(tmp_path: Path) -> None: + """Verify invalid content answers 400 with the issues list, file bytes unchanged. + + Given: A disk-anchored session with an on-disk config file + When: POST /save is called with invalid YAML and no force flag + Then: 400 with the issues list and the file bytes are untouched + """ + config_path = tmp_path / ".profiles" + file_text = 'defaults:\n title: "On Disk"\n' + config_path.write_text(file_text, encoding="utf-8") + loaded = ConfigReader(config_path).load() + state = ServerState(config=loaded, active_directory=tmp_path) + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save", + json={"content": "defaults: [unclosed"}, + ) + + assert response.status_code == 400 + detail = response.json()["detail"] + assert isinstance(detail, list) + assert detail[0]["severity"] == "error" + assert detail[0]["message"] + assert config_path.read_text(encoding="utf-8") == file_text + + +@pytest.mark.anyio +async def test_save_force_writes_invalid_content(tmp_path: Path) -> None: + """Verify force=True writes invalid content to disk and answers SAVED. + + Given: A disk-anchored session and invalid YAML content + When: POST /save is called with force=True + Then: 200 SAVED and the file contains the invalid text verbatim + """ + config_path = tmp_path / ".profiles" + config_path.write_text('defaults:\n title: "On Disk"\n', encoding="utf-8") + loaded = ConfigReader(config_path).load() + state = ServerState(config=loaded, active_directory=tmp_path) + invalid_text = "defaults: [unclosed" + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save", + json={"content": invalid_text, "force": True}, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["status"] == "SAVED" + assert payload["path"] == str(config_path) + assert config_path.read_text(encoding="utf-8") == invalid_text + assert "config" in payload + + +@pytest.mark.anyio +async def test_save_writes_and_applies_valid_config(tmp_path: Path) -> None: + """Verify a valid save writes the file and applies it to the live state. + + Given: A disk-anchored session and valid YAML content + When: POST /save is called without force + Then: 200 SAVED, the file is written, and ``state.config`` reflects + the new content with the session-trial flag cleared + """ + config_path = tmp_path / ".profiles" + config_path.write_text('defaults:\n title: "Old Title"\n', encoding="utf-8") + loaded = ConfigReader(config_path).load() + state = ServerState(config=loaded, active_directory=tmp_path) + valid_text = f"defaults:\n title: Fresh Title\n search_dir: '{tmp_path}'\n" + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save", + json={"content": valid_text}, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["status"] == "SAVED" + assert payload["path"] == str(config_path) + assert config_path.read_text(encoding="utf-8") == valid_text + assert state.config.title == "Fresh Title" + assert payload["config"]["title"] == "Fresh Title" + assert state.session_trial is False From b806dd542cf6c94ecda0c88c2702ee05bff11e13 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:38:51 +0100 Subject: [PATCH 06/19] feat(server): save-as endpoint with extension whitelist and overwrite guard --- src/profiles/server/routes/config.py | 59 +++++++ tests/server/routes/test_config_editor.py | 197 +++++++++++++++++++++- 2 files changed, 255 insertions(+), 1 deletion(-) diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index 69f126b..122e8aa 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -44,6 +44,14 @@ class SaveRequest(BaseModel): force: bool = False +class SaveAsRequest(BaseModel): + content: str + filename: str + directory: str + force: bool = False + overwrite: bool = False + + def _is_config_path(path: Path) -> bool: return path.name.endswith(CONFIG_FILE_NAMES) @@ -380,6 +388,57 @@ async def save_config_endpoint(payload: SaveRequest, request: Request) -> dict: return {"status": "SAVED", "path": str(config_path), "config": config} +SAVE_AS_EXTENSIONS = (".profiles", ".profiles.yaml") + + +@router.post("/save-as") +async def save_as_config_endpoint(payload: SaveAsRequest, request: Request) -> dict: + """Write editor content to a new config file and apply it. + + Unlike :func:`save_config_endpoint`, save-as is anchored to a + user-supplied directory and filename, so it works both to copy a + disk-anchored config and to anchor a session trial onto disk. Gate + order: extension whitelist (:data:`SAVE_AS_EXTENSIONS`) → + :func:`normalize_user_path` + directory existence → + existence/overwrite guard (409 when the target exists without + ``overwrite=True``) → :func:`validate_issues` → 400 with the issues + list when invalid and not ``force`` → write the file → + :func:`_apply_config_from_disk`, so the live configuration is always + re-read from what is on disk (which also clears ``session_trial``). + + Returns ``{"status": "SAVED", "path": str, "config": }``, + the same shape as ``POST /save``. + """ + state: ServerState = request.app.state.server_state + + if not payload.filename.lower().endswith(SAVE_AS_EXTENSIONS): + raise HTTPException( + status_code=400, + detail=f"Not a ProFiles configuration file: {payload.filename}. " + f"Allowed extensions: {', '.join(SAVE_AS_EXTENSIONS)}", + ) + + target_dir = normalize_user_path(payload.directory) + if not target_dir.exists() or not target_dir.is_dir(): + raise HTTPException(status_code=400, detail="Directory does not exist") + + target = target_dir / payload.filename + if target.exists() and not payload.overwrite: + raise HTTPException( + status_code=409, + detail=f"Configuration already exists at {target}. " + "Retry with overwrite=true to replace it.", + ) + + issues = validate_issues(payload.content, display_name=payload.filename) + if issues and not payload.force: + raise HTTPException(status_code=400, detail=issues) + + target.write_text(payload.content, encoding="utf-8") + config = _apply_config_from_disk(state, target) + return {"status": "SAVED", "path": str(target), "config": config} + + @router.post("/select-directory") async def select_directory(payload: SelectDirectoryRequest, request: Request) -> dict: """Update active directory.""" diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py index 262c467..c611c9a 100644 --- a/tests/server/routes/test_config_editor.py +++ b/tests/server/routes/test_config_editor.py @@ -1,4 +1,4 @@ -"""Tests for the config editor endpoints: validate + content + save.""" +"""Tests for the config editor endpoints: validate + content + save + save-as.""" from __future__ import annotations @@ -281,3 +281,198 @@ async def test_save_writes_and_applies_valid_config(tmp_path: Path) -> None: assert state.config.title == "Fresh Title" assert payload["config"]["title"] == "Fresh Title" assert state.session_trial is False + + +@pytest.mark.anyio +async def test_save_as_rejects_bad_extension(config_state: ServerState, tmp_path: Path) -> None: + """Verify save-as refuses filenames outside the extension whitelist. + + Given: A session with any active configuration + When: POST /save-as is called with a ``.yml`` filename + Then: 400 is returned and no file is created on disk + """ + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save-as", + json={ + "content": "defaults:\n title: X\n", + "filename": "config.yml", + "directory": str(tmp_path), + }, + ) + + assert response.status_code == 400 + assert not (tmp_path / "config.yml").exists() + + +@pytest.mark.anyio +async def test_save_as_rejects_existing_target_without_overwrite(tmp_path: Path) -> None: + """Pin 409 Conflict: an existing target requires overwrite=True. + + Given: A target config file that already exists on disk + When: POST /save-as targets the same path without overwrite + Then: 409 is returned naming the path and the file bytes are untouched + """ + target = tmp_path / ".profiles" + file_text = 'defaults:\n title: "Existing"\n' + target.write_text(file_text, encoding="utf-8") + loaded = ConfigReader(target).load() + state = ServerState(config=loaded, active_directory=tmp_path) + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save-as", + json={ + "content": "defaults:\n title: Replaced\n", + "filename": ".profiles", + "directory": str(tmp_path), + }, + ) + + assert response.status_code == 409 + assert str(target) in response.json()["detail"] + assert target.read_text(encoding="utf-8") == file_text + + +@pytest.mark.anyio +async def test_save_as_overwrite_replaces_file(tmp_path: Path) -> None: + """Verify overwrite=True replaces an existing target with the new content. + + Given: An existing target config file + When: POST /save-as targets it with overwrite=True and valid content + Then: 200 SAVED and the file contains the new content verbatim + """ + target = tmp_path / ".profiles" + target.write_text('defaults:\n title: "Old Title"\n', encoding="utf-8") + loaded = ConfigReader(target).load() + state = ServerState(config=loaded, active_directory=tmp_path) + new_text = f"defaults:\n title: Replaced\n search_dir: '{tmp_path}'\n" + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save-as", + json={ + "content": new_text, + "filename": ".profiles", + "directory": str(tmp_path), + "overwrite": True, + }, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["status"] == "SAVED" + assert payload["path"] == str(target) + assert target.read_text(encoding="utf-8") == new_text + + +@pytest.mark.anyio +async def test_save_as_rejects_invalid_content(tmp_path: Path) -> None: + """Verify invalid content answers 400 with the issues list and writes nothing. + + Given: A target path with no existing file + When: POST /save-as is called with invalid YAML and no force flag + Then: 400 with the issues list and the target file is never created + """ + target = tmp_path / "draft.profiles" + state = ServerState( + config=AppConfig(title="Session", search_dir=str(tmp_path), extensions=(".txt",)), + active_directory=tmp_path, + ) + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save-as", + json={ + "content": "defaults: [unclosed", + "filename": "draft.profiles", + "directory": str(tmp_path), + }, + ) + + assert response.status_code == 400 + detail = response.json()["detail"] + assert isinstance(detail, list) + assert detail[0]["severity"] == "error" + assert detail[0]["message"] + assert not target.exists() + + +@pytest.mark.anyio +async def test_save_as_force_saves_invalid_content(tmp_path: Path) -> None: + """Verify force=True writes invalid content to the new path anyway. + + Given: A target path with no existing file + When: POST /save-as is called with invalid YAML and force=True + Then: 200 SAVED and the file contains the invalid text verbatim + """ + target = tmp_path / "broken.profiles" + state = ServerState( + config=AppConfig(title="Session", search_dir=str(tmp_path), extensions=(".txt",)), + active_directory=tmp_path, + ) + invalid_text = "defaults: [unclosed" + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save-as", + json={ + "content": invalid_text, + "filename": "broken.profiles", + "directory": str(tmp_path), + "force": True, + }, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["status"] == "SAVED" + assert payload["path"] == str(target) + assert target.read_text(encoding="utf-8") == invalid_text + assert "config" in payload + + +@pytest.mark.anyio +async def test_save_as_applies_new_config(tmp_path: Path) -> None: + """Verify a save-as anchors the session to the new file and re-derives state. + + Given: A session anchored to an existing config and a new valid draft + whose ``search_dir`` points at a different existing directory + When: POST /save-as writes the draft to a fresh target path + Then: 200 SAVED, ``state.config_path`` is the target, + ``state.active_directory`` is re-derived from the new config, and + the session-trial flag is cleared + """ + target = tmp_path / "copy.profiles" + scan_dir = tmp_path / "scans" + scan_dir.mkdir() + old_path = tmp_path / ".profiles" + old_path.write_text(f"defaults:\n title: Old\n search_dir: '{tmp_path}'\n", encoding="utf-8") + state = ServerState(config=ConfigReader(old_path).load(), active_directory=tmp_path) + valid_text = f"defaults:\n title: Saved As Copy\n search_dir: '{scan_dir}'\n" + + app = create_app(state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post( + "/api/config/save-as", + json={ + "content": valid_text, + "filename": "copy.profiles", + "directory": str(tmp_path), + }, + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["status"] == "SAVED" + assert payload["path"] == str(target) + assert target.read_text(encoding="utf-8") == valid_text + assert state.config_path == target + assert state.active_directory == scan_dir + assert state.session_trial is False + assert payload["config"]["title"] == "Saved As Copy" From bcf6da0e45ef46b3152ef1ef6e35c6c48a55a3a8 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:42:02 +0100 Subject: [PATCH 07/19] fix(server): reject path separators in save-as filename --- src/profiles/server/routes/config.py | 35 ++++++++++++++++++----- tests/server/routes/test_config_editor.py | 30 +++++++++++++++++++ 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index 122e8aa..5e50251 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -56,6 +56,19 @@ def _is_config_path(path: Path) -> bool: return path.name.endswith(CONFIG_FILE_NAMES) +def _is_safe_filename(filename: str) -> bool: + """Return True when *filename* is a bare name that cannot escape its directory. + + Hygiene guard for write endpoints: rejects empty names, ``.``/``..`` + dot segments, and path separators (``/``, ``\\``) so joining the + filename onto a user directory can never traverse (cf. the + path-traversal fix in issue #28). + """ + if not filename or filename in {".", ".."}: + return False + return "/" not in filename and "\\" not in filename + + def _config_response(state: ServerState) -> dict: cfg: AppConfig = state.config config_path = state.active_config_path() @@ -398,19 +411,27 @@ async def save_as_config_endpoint(payload: SaveAsRequest, request: Request) -> d Unlike :func:`save_config_endpoint`, save-as is anchored to a user-supplied directory and filename, so it works both to copy a disk-anchored config and to anchor a session trial onto disk. Gate - order: extension whitelist (:data:`SAVE_AS_EXTENSIONS`) → - :func:`normalize_user_path` + directory existence → - existence/overwrite guard (409 when the target exists without - ``overwrite=True``) → :func:`validate_issues` → 400 with the issues - list when invalid and not ``force`` → write the file → - :func:`_apply_config_from_disk`, so the live configuration is always - re-read from what is on disk (which also clears ``session_trial``). + order: filename hygiene (:func:`_is_safe_filename` — no separators, + dot segments, or empty names) → extension whitelist + (:data:`SAVE_AS_EXTENSIONS`) → :func:`normalize_user_path` + + directory existence → existence/overwrite guard (409 when the target + exists without ``overwrite=True``) → :func:`validate_issues` → 400 + with the issues list when invalid and not ``force`` → write the file + → :func:`_apply_config_from_disk`, so the live configuration is + always re-read from what is on disk (which also clears + ``session_trial``). Returns ``{"status": "SAVED", "path": str, "config": }``, the same shape as ``POST /save``. """ state: ServerState = request.app.state.server_state + if not _is_safe_filename(payload.filename): + raise HTTPException( + status_code=400, + detail=f"Invalid configuration file name: {payload.filename}", + ) + if not payload.filename.lower().endswith(SAVE_AS_EXTENSIONS): raise HTTPException( status_code=400, diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py index c611c9a..8f1ed62 100644 --- a/tests/server/routes/test_config_editor.py +++ b/tests/server/routes/test_config_editor.py @@ -283,6 +283,36 @@ async def test_save_writes_and_applies_valid_config(tmp_path: Path) -> None: assert state.session_trial is False +@pytest.mark.anyio +async def test_save_as_rejects_filename_with_path_separators( + config_state: ServerState, tmp_path: Path +) -> None: + """Verify save-as refuses filenames that could escape the target directory. + + Given: A session and a target directory + When: POST /save-as is called with a filename containing path + separators (forward or backslash) or a dot segment + Then: 400 naming the filename and no file is created anywhere + """ + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + for filename in ("sub/../x.profiles", "nested\\x.profiles", "..", ""): + response = await client.post( + "/api/config/save-as", + json={ + "content": "defaults:\n title: X\n", + "filename": filename, + "directory": str(tmp_path), + }, + ) + assert response.status_code == 400 + assert filename in response.json()["detail"] + + assert not (tmp_path / "x.profiles").exists() + assert not (tmp_path / "sub").exists() + assert not (tmp_path / "nested\\x.profiles").exists() + + @pytest.mark.anyio async def test_save_as_rejects_bad_extension(config_state: ServerState, tmp_path: Path) -> None: """Verify save-as refuses filenames outside the extension whitelist. From a42632a154a760eacff2940a4c1657b6e6f3e20d Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:51:45 +0100 Subject: [PATCH 08/19] feat(web): API client for config editor endpoints --- src/profiles/web/src/api/client.test.ts | 230 +++++++++++++++++++++++- src/profiles/web/src/api/client.ts | 99 +++++++++- src/profiles/web/src/api/types.ts | 18 ++ 3 files changed, 341 insertions(+), 6 deletions(-) diff --git a/src/profiles/web/src/api/client.test.ts b/src/profiles/web/src/api/client.test.ts index 525305d..867d40a 100644 --- a/src/profiles/web/src/api/client.test.ts +++ b/src/profiles/web/src/api/client.test.ts @@ -1,6 +1,17 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { decodeBarcodeImage, exportCsvToDisk, fetchNotifications, reloadConfig } from "./client"; +import { + ApiError, + decodeBarcodeImage, + exportCsvToDisk, + fetchConfigContent, + fetchConfigTemplate, + fetchNotifications, + reloadConfig, + saveConfigFile, + saveConfigFileAs, + validateConfigContent, +} from "./client"; const SAMPLE_CONFIG = { title: "Fresh From Disk", @@ -167,3 +178,220 @@ describe("decodeBarcodeImage", () => { await expect(decodeBarcodeImage({ imageBase64: "QUJD" })).rejects.toThrow("HTTP 400"); }); }); + +describe("fetchConfigContent", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("GETs /api/config/content and returns the editor payload", async () => { + const payload = { + filename: ".profiles", + content: "title: Fresh From Disk\n", + config_path: "/tmp/.profiles", + session_trial: false, + }; + const fetchMock = vi.fn(async () => + new Response(JSON.stringify(payload), { status: 200 }), + ); + vi.stubGlobal("fetch", fetchMock); + + const res = await fetchConfigContent(); + + expect(fetchMock).toHaveBeenCalledWith("/api/config/content"); + expect(res).toEqual(payload); + }); + + it("carries the 404 status when the recorded file was deleted", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => + new Response( + JSON.stringify({ detail: "Configuration file not found: /tmp/.profiles" }), + { status: 404 }, + ), + ), + ); + + const err = (await fetchConfigContent().catch((error: unknown) => error)) as ApiError; + + expect(err).toBeInstanceOf(ApiError); + expect(err.status).toBe(404); + expect(err.message).toContain("Configuration file not found"); + }); +}); + +describe("validateConfigContent", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("POSTs the filename and content to /api/config/validate and returns the issues", async () => { + const issues = [{ severity: "error" as const, message: "unexpected token" }]; + const fetchMock = vi.fn(async () => + new Response(JSON.stringify({ valid: false, issues }), { status: 200 }), + ); + vi.stubGlobal("fetch", fetchMock); + + const res = await validateConfigContent(".profiles", "defaults: [unclosed"); + + expect(fetchMock).toHaveBeenCalledWith("/api/config/validate", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filename: ".profiles", content: "defaults: [unclosed" }), + }); + expect(res.valid).toBe(false); + expect(res.issues).toEqual(issues); + }); +}); + +describe("fetchConfigTemplate", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("POSTs to /api/config/template and returns the starter content", async () => { + const template = { content: "# ProFiles — Starter Configuration\ntitle: ...\n", filename: ".profiles" }; + const fetchMock = vi.fn(async () => + new Response(JSON.stringify(template), { status: 200 }), + ); + vi.stubGlobal("fetch", fetchMock); + + const res = await fetchConfigTemplate(); + + expect(fetchMock).toHaveBeenCalledWith("/api/config/template", { method: "POST" }); + expect(res.content).toBe(template.content); + expect(res.filename).toBe(".profiles"); + }); +}); + +describe("saveConfigFile", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("POSTs the content and force flag to /api/config/save and returns the applied config", async () => { + const fetchMock = vi.fn(async () => + new Response( + JSON.stringify({ status: "SAVED", path: "/tmp/.profiles", config: SAMPLE_CONFIG }), + { status: 200 }, + ), + ); + vi.stubGlobal("fetch", fetchMock); + + const cfg = await saveConfigFile("title: Fresh From Disk\n", true); + + expect(fetchMock).toHaveBeenCalledWith("/api/config/save", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ content: "title: Fresh From Disk\n", force: true }), + }); + expect(cfg.title).toBe("Fresh From Disk"); + expect(cfg.config_path).toBe("C:/tmp/.profiles"); + }); + + it("surfaces the plain-string detail for a session-trial refusal", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => + new Response( + JSON.stringify({ + detail: + "Cannot save: the active configuration is a session trial with no file on disk. Load or create a configuration first.", + }), + { status: 400 }, + ), + ), + ); + + await expect(saveConfigFile("title: Fresh From Disk\n", false)).rejects.toThrow( + "Cannot save: the active configuration is a session trial", + ); + }); + + it("keeps the validation issues list intact on a 400", async () => { + const issues = [ + { severity: "error" as const, message: "title: field required" }, + { severity: "error" as const, message: "unknown field 'directory'" }, + ]; + vi.stubGlobal( + "fetch", + vi.fn(async () => + new Response(JSON.stringify({ detail: issues }), { status: 400 }), + ), + ); + + const err = (await saveConfigFile("defaults: [unclosed", false).catch( + (error: unknown) => error, + )) as ApiError; + + expect(err).toBeInstanceOf(ApiError); + expect(err.status).toBe(400); + expect(err.detail).toEqual(issues); + expect(err.message).toBe("Failed to save configuration"); + }); +}); + +describe("saveConfigFileAs", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("POSTs the save-as params to /api/config/save-as and returns the applied config", async () => { + const fetchMock = vi.fn(async () => + new Response( + JSON.stringify({ status: "SAVED", path: "/tmp/copy/.profiles", config: SAMPLE_CONFIG }), + { status: 200 }, + ), + ); + vi.stubGlobal("fetch", fetchMock); + + const cfg = await saveConfigFileAs({ + content: "title: Fresh From Disk\n", + filename: ".profiles", + directory: "/tmp/copy", + force: false, + overwrite: true, + }); + + expect(fetchMock).toHaveBeenCalledWith("/api/config/save-as", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + content: "title: Fresh From Disk\n", + filename: ".profiles", + directory: "/tmp/copy", + force: false, + overwrite: true, + }), + }); + expect(cfg.title).toBe("Fresh From Disk"); + }); + + it("surfaces the 409 overwrite-guard detail", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => + new Response( + JSON.stringify({ + detail: + "Configuration already exists at /tmp/copy/.profiles. Retry with overwrite=true to replace it.", + }), + { status: 409 }, + ), + ), + ); + + const err = (await saveConfigFileAs({ + content: "title: Fresh From Disk\n", + filename: ".profiles", + directory: "/tmp/copy", + force: false, + overwrite: false, + }).catch((error: unknown) => error)) as ApiError; + + expect(err).toBeInstanceOf(ApiError); + expect(err.status).toBe(409); + expect(err.message).toContain("already exists"); + }); +}); diff --git a/src/profiles/web/src/api/client.ts b/src/profiles/web/src/api/client.ts index 16c3bc0..8e7f39e 100644 --- a/src/profiles/web/src/api/client.ts +++ b/src/profiles/web/src/api/client.ts @@ -1,39 +1,56 @@ import type { AppConfigData, BrowseResult, + ConfigContentData, ExternalEvent, FileMetadata, LogLevel, LogResponse, ScanResultsResponse, SystemInfo, + ValidationIssue, WorkflowSessionState, } from "./types"; /** * Error carrying the HTTP status so callers can react to specific codes — * e.g. 404 from the workflow session routes means the server-side session - * expired or was purged (TTL), and the UI can offer recovery (#79). + * expired or was purged (TTL), and the UI can offer recovery (#79). When + * the body's detail is not a plain string (a validation issue list from + * the config editor save endpoints), it is preserved verbatim in + * `detail` instead of being flattened into the message. */ export class ApiError extends Error { status: number; - constructor(message: string, status: number) { + /** Raw body detail when it is not a plain string (e.g. ValidationIssue[]). */ + detail?: string | ValidationIssue[]; + constructor(message: string, status: number, detail?: string | ValidationIssue[]) { super(message); this.name = "ApiError"; this.status = status; + this.detail = detail; } } /** Build an ApiError from a failed Response, preferring the body's detail. */ async function apiError(res: Response, fallback: string): Promise { - let detail = fallback; + let message = fallback; + let detail: string | ValidationIssue[] | undefined; try { const body = await res.json(); - if (body?.detail) detail = body.detail; + if (typeof body?.detail === "string" && body.detail) { + message = body.detail; + detail = body.detail; + } else if (Array.isArray(body?.detail)) { + // Heterogeneous 400 detail from the config editor save endpoints: a + // validation issue list. Keep the raw array for callers that render + // it as data (editor gate modal) instead of flattening it to text. + detail = body.detail; + } } catch { // keep the fallback message } - return new ApiError(detail, res.status); + return new ApiError(message, res.status, detail); } /** Fetch UI events recorded after *afterId* (config handoff notifications). */ @@ -193,6 +210,78 @@ export async function createConfigFile(directory: string): Promise { + const res = await fetch("/api/config/content"); + if (!res.ok) throw await apiError(res, "Failed to fetch config content"); + return res.json(); +} + +/** + * Validate editor content in memory. Invalid content is answered as data + * (HTTP 200 with `valid: false` and the issue list), never as an HTTP + * error, so drafts can be shown without try/catch. + */ +export async function validateConfigContent( + filename: string, + content: string, +): Promise<{ valid: boolean; issues: ValidationIssue[] }> { + const res = await fetch("/api/config/validate", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filename, content }), + }); + if (!res.ok) throw await apiError(res, "Failed to validate config content"); + return res.json(); +} + +/** Generate starter configuration content without touching the session state. */ +export async function fetchConfigTemplate(): Promise<{ content: string; filename: string }> { + const res = await fetch("/api/config/template", { method: "POST" }); + if (!res.ok) throw await apiError(res, "Failed to fetch config template"); + return res.json(); +} + +/** + * Write editor content to the active configuration file and apply it. + * Rejects with an ApiError whose `detail` is either a plain message + * (e.g. a session-trial refusal) or a `ValidationIssue[]` gate list — + * callers rendering the compliance gate should inspect `detail`. + */ +export async function saveConfigFile(content: string, force: boolean): Promise { + const res = await fetch("/api/config/save", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ content, force }), + }); + if (!res.ok) throw await apiError(res, "Failed to save configuration"); + const payload: { config: AppConfigData } = await res.json(); + return payload.config; +} + +/** + * Write editor content to a new configuration file and apply it; also + * anchors a session trial onto disk. A 409 ApiError means the target + * exists and `overwrite` was false; a 400 carries a plain message or a + * `ValidationIssue[]` gate list in `detail`. + */ +export async function saveConfigFileAs(params: { + content: string; + filename: string; + directory: string; + force: boolean; + overwrite: boolean; +}): Promise { + const res = await fetch("/api/config/save-as", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(params), + }); + if (!res.ok) throw await apiError(res, "Failed to save configuration as"); + const payload: { config: AppConfigData } = await res.json(); + return payload.config; +} + export async function browseForConfigFiles(path: string): Promise { const url = `/api/fs/browse?path=${encodeURIComponent(path)}&include_hidden=true`; const res = await fetch(url); diff --git a/src/profiles/web/src/api/types.ts b/src/profiles/web/src/api/types.ts index 5a81b4b..2f70e96 100644 --- a/src/profiles/web/src/api/types.ts +++ b/src/profiles/web/src/api/types.ts @@ -30,6 +30,24 @@ export interface AppConfigData { row_colors: [string, string][]; } +/** A validation problem reported by the config editor endpoints. */ +export interface ValidationIssue { + severity: "error" | "warning"; + message: string; +} + +/** Response of `GET /api/config/content` — the YAML editor payload. */ +export interface ConfigContentData { + /** Display name of the backing file (config title for a session trial). */ + filename: string; + /** Raw file text; empty for a session trial (config applied from dropped content). */ + content: string; + /** Absolute path of the backing file; null when there is no disk anchor. */ + config_path: string | null; + /** True when the active config came from a dropped file — Save is refused, use Save As. */ + session_trial: boolean; +} + export interface BrowseEntry { name: string; path: string; From 8ae636789dc1addd5ad1542304b8546b0d4b8531 Mon Sep 17 00:00:00 2001 From: falbany Date: Tue, 3 Nov 2026 23:54:43 +0100 Subject: [PATCH 09/19] feat(server): in-memory starter template endpoint for the editor --- src/profiles/server/routes/config.py | 19 ++++++++ tests/server/routes/test_config_editor.py | 55 ++++++++++++++++++++++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/profiles/server/routes/config.py b/src/profiles/server/routes/config.py index 5e50251..7726396 100644 --- a/src/profiles/server/routes/config.py +++ b/src/profiles/server/routes/config.py @@ -10,6 +10,7 @@ from profiles.core.config.io.yaml_io import CONFIG_FILE_NAMES from profiles.core.config.models import AppConfig from profiles.core.config.reader import ConfigReader +from profiles.core.config.template import STARTER_CONFIG_TEMPLATE from profiles.launch.creation import propose_creation from profiles.server.state import ServerState from profiles.utils.path_utils import normalize_user_path @@ -285,6 +286,24 @@ async def create_config_endpoint(payload: CreateConfigRequest, request: Request) return _config_response(state) +@router.post("/template") +async def template_config_endpoint(request: Request) -> dict: + """Render the starter configuration in memory; nothing written, no state change. + + In-memory render of the same content ``--init``/``propose_creation`` + writes to disk (the canonical :data:`STARTER_CONFIG_TEMPLATE`), powering + the editor's New action. The ``{cwd}`` placeholder — the template's + only single-brace placeholder — is substituted with the active + directory so the rendered draft scans where the session points. + """ + state: ServerState = request.app.state.server_state + cwd = str(state.active_directory) if state.active_directory else str(Path.cwd()) + return { + "content": STARTER_CONFIG_TEMPLATE.format(cwd=cwd), + "filename": ".profiles", + } + + @router.post("/reload") async def reload_config_endpoint(request: Request) -> dict: """Re-read the active configuration file from disk. diff --git a/tests/server/routes/test_config_editor.py b/tests/server/routes/test_config_editor.py index 8f1ed62..1cf3186 100644 --- a/tests/server/routes/test_config_editor.py +++ b/tests/server/routes/test_config_editor.py @@ -11,7 +11,7 @@ from profiles.core.config.reader import ConfigReader from profiles.core.config.template import STARTER_CONFIG_TEMPLATE from profiles.server.app import create_app -from profiles.server.routes.config import has_disk_path +from profiles.server.routes.config import has_disk_path, validate_issues from profiles.server.state import ServerState @@ -506,3 +506,56 @@ async def test_save_as_applies_new_config(tmp_path: Path) -> None: assert state.active_directory == scan_dir assert state.session_trial is False assert payload["config"]["title"] == "Saved As Copy" + + +@pytest.mark.anyio +async def test_template_returns_starter_content(config_state: ServerState) -> None: + """Verify POST /template renders starter content that validates clean. + + Given: A session in any state + When: POST /template is called + Then: 200 with filename ".profiles" and content that passes the + project's own validator (the template has brace-escaping history) + """ + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post("/api/config/template") + + assert response.status_code == 200 + payload = response.json() + assert payload["filename"] == ".profiles" + assert validate_issues(payload["content"], display_name=".profiles") == [] + + +@pytest.mark.anyio +async def test_template_substitutes_active_directory( + config_state: ServerState, tmp_path: Path +) -> None: + """Verify the cwd placeholder is substituted with the active directory.""" + scan_dir = tmp_path / "scans" + scan_dir.mkdir() + config_state.active_directory = scan_dir + + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post("/api/config/template") + + assert response.status_code == 200 + search_line = next( + line + for line in response.json()["content"].splitlines() + if line.lstrip().startswith("search_dir:") + ) + assert str(scan_dir) in search_line + + +@pytest.mark.anyio +async def test_template_does_not_change_state(config_state: ServerState) -> None: + """Verify rendering is read-only: the live configuration is untouched.""" + original = config_state.config + app = create_app(config_state) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.post("/api/config/template") + + assert response.status_code == 200 + assert config_state.config is original From cfa4f4167b41e6c5939ca28253a5f7554651ab23 Mon Sep 17 00:00:00 2001 From: falbany Date: Wed, 4 Nov 2026 00:08:44 +0100 Subject: [PATCH 10/19] feat(web): config editor modal shell with CodeMirror and dirty guard --- src/profiles/web/package-lock.json | 179 ++++++++++++ src/profiles/web/package.json | 2 + .../src/components/ConfigEditorModal.test.tsx | 164 +++++++++++ .../web/src/components/ConfigEditorModal.tsx | 276 ++++++++++++++++++ src/profiles/web/tsconfig.tsbuildinfo | 2 +- 5 files changed, 622 insertions(+), 1 deletion(-) create mode 100644 src/profiles/web/src/components/ConfigEditorModal.test.tsx create mode 100644 src/profiles/web/src/components/ConfigEditorModal.tsx diff --git a/src/profiles/web/package-lock.json b/src/profiles/web/package-lock.json index 351093e..8bbbc2f 100644 --- a/src/profiles/web/package-lock.json +++ b/src/profiles/web/package-lock.json @@ -6,8 +6,10 @@ "": { "name": "profiles-web", "dependencies": { + "@codemirror/lang-yaml": "^6.1.3", "@tanstack/react-table": "^8.21.2", "clsx": "^2.1.1", + "codemirror": "^6.0.2", "lucide-react": "^0.477.0", "react": "^19.0.0", "react-dom": "^19.0.0", @@ -1696,6 +1698,109 @@ "specificity": "bin/cli.js" } }, + "node_modules/@codemirror/autocomplete": { + "version": "6.20.4", + "resolved": "https://registry.npmjs.org/@codemirror/autocomplete/-/autocomplete-6.20.4.tgz", + "integrity": "sha512-yF8RhOf2SXlg00+qtupzLhrglcas61boqaONiNcbPIebHCpJocpqTBA6dyrtUrQO47hqyTdeob/3XnYilT7TaQ==", + "license": "MIT", + "dependencies": { + "@codemirror/language": "^6.0.0", + "@codemirror/state": "^6.0.0", + "@codemirror/view": "^6.17.0", + "@lezer/common": "^1.0.0" + } + }, + "node_modules/@codemirror/commands": { + "version": "6.11.1", + "resolved": "https://registry.npmjs.org/@codemirror/commands/-/commands-6.11.1.tgz", + "integrity": "sha512-O/4hG3SC1YwcmQ0d2UVNDs+AsaNWd1iHVxbTeEBuqH+6bExAiPK3iS/BvpY6rZGURALv4ZD3sIgcCmRvw3ehBg==", + "license": "MIT", + "dependencies": { + "@codemirror/language": "^6.0.0", + "@codemirror/state": "^6.7.0", + "@codemirror/view": "^6.27.0", + "@lezer/common": "^1.1.0" + } + }, + "node_modules/@codemirror/lang-yaml": { + "version": "6.1.3", + "resolved": "https://registry.npmjs.org/@codemirror/lang-yaml/-/lang-yaml-6.1.3.tgz", + "integrity": "sha512-AZ8DJBuXGVHybpBQhmZtgew5//4hv3tdkXnr3vDmOUMJRuB6vn/uuwtmTOTlqEaQFg3hQSVeA90NmvIQyUV6FQ==", + "license": "MIT", + "dependencies": { + "@codemirror/autocomplete": "^6.0.0", + "@codemirror/language": "^6.0.0", + "@codemirror/state": "^6.0.0", + "@lezer/common": "^1.2.0", + "@lezer/highlight": "^1.2.0", + "@lezer/lr": "^1.0.0", + "@lezer/yaml": "^1.0.0" + } + }, + "node_modules/@codemirror/language": { + "version": "6.13.1", + "resolved": "https://registry.npmjs.org/@codemirror/language/-/language-6.13.1.tgz", + "integrity": "sha512-zfvU2r3TyylQLMj/te+5CnQ0rSqZi8B1oTCBbQkagcLW4Xc6imab6qN+pEJhsuNOLaJqiRyBXp9UhWA1DhVvDw==", + "license": "MIT", + "dependencies": { + "@codemirror/state": "^6.0.0", + "@codemirror/streamparser": "^6.0.0", + "@codemirror/view": "^6.23.0", + "@lezer/common": "^1.5.0", + "@lezer/highlight": "^1.0.0", + "@lezer/lr": "^1.0.0", + "style-mod": "^4.0.0" + } + }, + "node_modules/@codemirror/lint": { + "version": "6.9.7", + "resolved": "https://registry.npmjs.org/@codemirror/lint/-/lint-6.9.7.tgz", + "integrity": "sha512-28/+iWLYxKxsvGYhSYL7zaCZqLz5+FFFDq9tVsvGv9kv8RY4fFAchJ5WX9M3YrrRlTIsECjsXPqeNgnSmNP2dg==", + "license": "MIT", + "dependencies": { + "@codemirror/state": "^6.0.0", + "@codemirror/view": "^6.42.0", + "crelt": "^1.0.5" + } + }, + "node_modules/@codemirror/search": { + "version": "6.7.2", + "resolved": "https://registry.npmjs.org/@codemirror/search/-/search-6.7.2.tgz", + "integrity": "sha512-gUYkYhT2+n/+VGZ+8EzE5WFkYZUZYm1VOKDudIsNqh42uRVQJ0a6Yss9sdKT3MeOYfuL1N6AZA57oza0Oyr0LA==", + "license": "MIT", + "dependencies": { + "@codemirror/state": "^6.0.0", + "@codemirror/view": "^6.37.0", + "crelt": "^1.0.5" + } + }, + "node_modules/@codemirror/state": { + "version": "6.7.6", + "resolved": "https://registry.npmjs.org/@codemirror/state/-/state-6.7.6.tgz", + "integrity": "sha512-kAz+AncRtKuIknedxT1bq4XwXv4UowhbkHU1myPrtVb/jZtImWuV5BXzv5vK6i3kYACsdiZiQKFQQ5Mq7elW8w==", + "license": "MIT", + "dependencies": { + "@marijn/find-cluster-break": "^1.0.0" + } + }, + "node_modules/@codemirror/streamparser": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/@codemirror/streamparser/-/streamparser-6.0.0.tgz", + "integrity": "sha512-JTFPF6xrfjt0mnfGPgzcLiMwYmvIyNaoizp8BYzv8FE8KrT6GBGDZ190rtt+O7Wke3gG/MFHyiY9YGNeimof4w==", + "license": "MIT" + }, + "node_modules/@codemirror/view": { + "version": "6.43.15", + "resolved": "https://registry.npmjs.org/@codemirror/view/-/view-6.43.15.tgz", + "integrity": "sha512-HHsfpqFeHyXLs983pSRe38Xajs0vFWZQ2BuX0m7Ki8w3jZ5iD8cuR3lBz2/5/X53Dhf32SBHKh6rtSTB6tKeGw==", + "license": "MIT", + "dependencies": { + "@codemirror/state": "^6.7.0", + "crelt": "^1.0.6", + "style-mod": "^4.1.0", + "w3c-keyname": "^2.2.4" + } + }, "node_modules/@csstools/color-helpers": { "version": "6.1.2", "resolved": "https://registry.npmjs.org/@csstools/color-helpers/-/color-helpers-6.1.2.tgz", @@ -2367,6 +2472,47 @@ "@jridgewell/sourcemap-codec": "^1.4.14" } }, + "node_modules/@lezer/common": { + "version": "1.5.5", + "resolved": "https://registry.npmjs.org/@lezer/common/-/common-1.5.5.tgz", + "integrity": "sha512-Pw7PVCndruF1pb3ebCxpSVpkMHSDjwRCLcTRw+RizUKco5QbWylwq2g4lnG/RYN7yTTQDXE9F77MwtkAyphrjg==", + "license": "MIT" + }, + "node_modules/@lezer/highlight": { + "version": "1.2.5", + "resolved": "https://registry.npmjs.org/@lezer/highlight/-/highlight-1.2.5.tgz", + "integrity": "sha512-O1GMVKgtf5YspFaRzpmqkVgMtIk2HG9uMAbH6wdAIdNh3TVzCsch/+cRi+6l2//UGDKe6NB9BW3cvuR5ezMQ0w==", + "license": "MIT", + "dependencies": { + "@lezer/common": "^1.3.0" + } + }, + "node_modules/@lezer/lr": { + "version": "1.4.12", + "resolved": "https://registry.npmjs.org/@lezer/lr/-/lr-1.4.12.tgz", + "integrity": "sha512-On9HU4oMMiHPMkQDgoTnGahjgG5n/MkN38CgG77sMraKLL+3JBbeQNi1FcCXV9QRqwF9/BUXP3caHyJC1fBOTQ==", + "license": "MIT", + "dependencies": { + "@lezer/common": "^1.0.0" + } + }, + "node_modules/@lezer/yaml": { + "version": "1.0.4", + "resolved": "https://registry.npmjs.org/@lezer/yaml/-/yaml-1.0.4.tgz", + "integrity": "sha512-2lrrHqxalACEbxIbsjhqGpSW8kWpUKuY6RHgnSAFZa6qK62wvnPxA8hGOwOoDbwHcOFs5M4o27mjGu+P7TvBmw==", + "license": "MIT", + "dependencies": { + "@lezer/common": "^1.2.0", + "@lezer/highlight": "^1.0.0", + "@lezer/lr": "^1.4.0" + } + }, + "node_modules/@marijn/find-cluster-break": { + "version": "1.0.4", + "resolved": "https://registry.npmjs.org/@marijn/find-cluster-break/-/find-cluster-break-1.0.4.tgz", + "integrity": "sha512-Wy0V7+SGUjnF9/TkiM1hKVDPj7jKXduPNboMVtHTA8dySMURWqfg/JZ9E2Sq8JgSJmkl7k7Qe9FLeMSrSraWmQ==", + "license": "MIT" + }, "node_modules/@napi-rs/lzma-linux-x64-gnu": { "version": "1.5.1", "resolved": "https://registry.npmjs.org/@napi-rs/lzma-linux-x64-gnu/-/lzma-linux-x64-gnu-1.5.1.tgz", @@ -3975,6 +4121,21 @@ "node": ">=6" } }, + "node_modules/codemirror": { + "version": "6.0.2", + "resolved": "https://registry.npmjs.org/codemirror/-/codemirror-6.0.2.tgz", + "integrity": "sha512-VhydHotNW5w1UGK0Qj96BwSk/Zqbp9WbnyK2W/eVMv4QyF41INRGpjUhFJY7/uDNuudSc33a/PKr4iDqRduvHw==", + "license": "MIT", + "dependencies": { + "@codemirror/autocomplete": "^6.0.0", + "@codemirror/commands": "^6.0.0", + "@codemirror/language": "^6.0.0", + "@codemirror/lint": "^6.0.0", + "@codemirror/search": "^6.0.0", + "@codemirror/state": "^6.0.0", + "@codemirror/view": "^6.0.0" + } + }, "node_modules/comma-separated-tokens": { "version": "2.0.3", "resolved": "https://registry.npmjs.org/comma-separated-tokens/-/comma-separated-tokens-2.0.3.tgz", @@ -4026,6 +4187,12 @@ "url": "https://opencollective.com/core-js" } }, + "node_modules/crelt": { + "version": "1.0.7", + "resolved": "https://registry.npmjs.org/crelt/-/crelt-1.0.7.tgz", + "integrity": "sha512-aK6BbWfhf4U/wCcLHKPJl/xa6VkVstRaPywWtMKGwuOLc/wZTyQYuoxgvZnNsBvv7Kg3YTBQYYBCggcviQczuA==", + "license": "MIT" + }, "node_modules/cross-spawn": { "version": "7.0.6", "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", @@ -8181,6 +8348,12 @@ "node": ">=8" } }, + "node_modules/style-mod": { + "version": "4.1.4", + "resolved": "https://registry.npmjs.org/style-mod/-/style-mod-4.1.4.tgz", + "integrity": "sha512-XXWIQt633/EpAFx8aZDOTjBzrCaGmhvEQlQo6MVPfa2OzO2cWo+4hV9h+6UkHYlXGfy+ODXKUdP7Pthmcu5ATw==", + "license": "MIT" + }, "node_modules/style-to-object": { "version": "2.0.2", "resolved": "https://registry.npmjs.org/style-to-object/-/style-to-object-2.0.2.tgz", @@ -8993,6 +9166,12 @@ "@jridgewell/sourcemap-codec": "^1.6.0" } }, + "node_modules/w3c-keyname": { + "version": "2.2.8", + "resolved": "https://registry.npmjs.org/w3c-keyname/-/w3c-keyname-2.2.8.tgz", + "integrity": "sha512-dpojBhNsCNN7T82Tm7k26A6G9ML3NkhDsnw9n/eoxSRlVBB4CEtIQ/KTCLI2Fwf3ataSXRhYFkQi3SlnFwPvPQ==", + "license": "MIT" + }, "node_modules/w3c-xmlserializer": { "version": "5.0.0", "resolved": "https://registry.npmjs.org/w3c-xmlserializer/-/w3c-xmlserializer-5.0.0.tgz", diff --git a/src/profiles/web/package.json b/src/profiles/web/package.json index ce86e57..73fd01a 100644 --- a/src/profiles/web/package.json +++ b/src/profiles/web/package.json @@ -9,8 +9,10 @@ "preview": "vite preview" }, "dependencies": { + "@codemirror/lang-yaml": "^6.1.3", "@tanstack/react-table": "^8.21.2", "clsx": "^2.1.1", + "codemirror": "^6.0.2", "lucide-react": "^0.477.0", "react": "^19.0.0", "react-dom": "^19.0.0", diff --git a/src/profiles/web/src/components/ConfigEditorModal.test.tsx b/src/profiles/web/src/components/ConfigEditorModal.test.tsx new file mode 100644 index 0000000..9d97971 --- /dev/null +++ b/src/profiles/web/src/components/ConfigEditorModal.test.tsx @@ -0,0 +1,164 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { EditorView } from "codemirror"; +import { ConfigEditorModal } from "./ConfigEditorModal"; +import { fetchConfigContent, openConfigFile } from "../api/client"; +import type { ConfigContentData } from "../api/types"; + +vi.mock("../api/client", () => ({ + fetchConfigContent: vi.fn(), + openConfigFile: vi.fn(), +})); + +// jsdom gap (not a CodeMirror mock): the view's measurement code calls +// Range geometry APIs that jsdom does not implement. Empty geometry is +// enough — the editor never becomes visible under jsdom, and every other +// geometry API it uses exists (Element.getClientRects, DOMRect). +const rangeProto = Range.prototype as unknown as { + getClientRects?: () => DOMRect[]; + getBoundingClientRect?: () => DOMRect; +}; +rangeProto.getClientRects ??= () => []; +rangeProto.getBoundingClientRect ??= () => new DOMRect(0, 0, 0, 0); + +const fetchContentSpy = vi.mocked(fetchConfigContent); +const openInOSSpy = vi.mocked(openConfigFile); + +const SAMPLE_YAML = 'release: "1.0"\nsearch_dir: "C:/data"\n'; + +function makeContent(overrides: Partial = {}): ConfigContentData { + return { + filename: "machine.profiles", + content: SAMPLE_YAML, + config_path: "/home/user/.profiles/machine.profiles", + session_trial: false, + ...overrides, + }; +} + +function renderModal(overrides: { open?: boolean } = {}) { + const onClose = vi.fn(); + render(); + return { onClose }; +} + +/** + * Wait for the CodeMirror view to be mounted inside the editor dialog and + * return it. Uses the public `EditorView.findFromDOM` seam — the real + * editor instance, no mocks. + */ +async function readyEditorView(): Promise { + const dialog = await screen.findByRole("dialog", { name: /configuration editor/i }); + let view: EditorView | null = null; + await waitFor(() => { + const editorDom = dialog.querySelector(".cm-editor"); + expect(editorDom).toBeTruthy(); + view = EditorView.findFromDOM(editorDom as HTMLElement) ?? null; + expect(view).not.toBeNull(); + }); + return view as unknown as EditorView; +} + +beforeEach(() => { + vi.clearAllMocks(); + fetchContentSpy.mockResolvedValue(makeContent()); +}); + +afterEach(() => { + cleanup(); + document.body.innerHTML = ""; +}); + +describe("ConfigEditorModal", () => { + it("test_renders_fullscreen_overlay_when_open", async () => { + // Closed: nothing renders at all. + const closed = render(); + expect(screen.queryByRole("dialog")).toBeNull(); + closed.unmount(); + + renderModal(); + const dialog = await screen.findByRole("dialog", { name: /configuration editor/i }); + // Full-screen overlay pinned to the shared modal z-scale token layer + // (issue #59 scale) — same pattern as the barcode modals. + expect(dialog.className).toContain("fixed inset-0"); + expect(dialog.className).toContain("z-[var(--z-modal)]"); + }); + + it("test_loads_content_on_open", async () => { + fetchContentSpy.mockResolvedValue(makeContent({ content: SAMPLE_YAML })); + renderModal(); + + // One fetch per open, and the fetched text lands in the real editor. + await waitFor(() => expect(fetchContentSpy).toHaveBeenCalledTimes(1)); + const view = await readyEditorView(); + expect(view.state.doc.toString()).toBe(SAMPLE_YAML); + // The document is actually drawn into the DOM (first line visible). + const content = document.querySelector(".cm-content"); + expect(content?.textContent).toContain('release: "1.0"'); + // Header identifies the backing file. + expect(screen.getByText("machine.profiles")).toBeTruthy(); + }); + + it("test_dirty_indicator_and_escape_warning", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + const user = userEvent.setup(); + const { onClose } = renderModal(); + + const view = await readyEditorView(); + // Simulate a user edit through the real editor transaction seam. + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "# edited\n" } }); + }); + + // The dirty indicator appears in the toolbar row. + expect(screen.getByRole("status")).toHaveTextContent(/unsaved changes/i); + + // Escape with a dirty doc opens the discard warning, not a close. + screen.getByRole("button", { name: /open in os/i }).focus(); + await user.keyboard("{Escape}"); + const warning = screen.getByRole("dialog", { name: /discard changes\?/i }); + expect(warning).toBeTruthy(); + expect(onClose).not.toHaveBeenCalled(); + + // Declining keeps the editor open (and still dirty). + await user.click(screen.getByRole("button", { name: /keep editing/i })); + expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole("dialog", { name: /configuration editor/i })).toBeTruthy(); + expect(screen.getByRole("status")).toHaveTextContent(/unsaved changes/i); + }); + + it("test_clean_close_without_warning", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + const user = userEvent.setup(); + const { onClose } = renderModal(); + + await readyEditorView(); + // No edits: Escape closes immediately, no discard warning. + screen.getByRole("button", { name: /open in os/i }).focus(); + await user.keyboard("{Escape}"); + expect(onClose).toHaveBeenCalledTimes(1); + expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); + + // The ✕ button shares the same guard-free path when clean. + await user.click(screen.getByRole("button", { name: /close editor/i })); + expect(onClose).toHaveBeenCalledTimes(2); + expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); + }); + + it("test_header_open_in_os_calls_open_config_file", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + openInOSSpy.mockResolvedValue({ status: "SUCCESS", message: "Opened" }); + const user = userEvent.setup(); + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + renderModal(); + + const button = await screen.findByRole("button", { name: /open in os/i }); + await user.click(button); + await waitFor(() => expect(openInOSSpy).toHaveBeenCalledTimes(1)); + expect(consoleSpy).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); +}); diff --git a/src/profiles/web/src/components/ConfigEditorModal.tsx b/src/profiles/web/src/components/ConfigEditorModal.tsx new file mode 100644 index 0000000..cab195e --- /dev/null +++ b/src/profiles/web/src/components/ConfigEditorModal.tsx @@ -0,0 +1,276 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import { basicSetup, EditorView } from "codemirror"; +import { yaml } from "@codemirror/lang-yaml"; +import { CircleDot, ExternalLink, FileCode2, X } from "lucide-react"; + +import { fetchConfigContent, openConfigFile } from "../api/client"; +import type { AppConfigData, ConfigContentData } from "../api/types"; +import { ConfirmDialog } from "./ConfirmDialog"; +import { useModalA11y } from "../utils/useModalA11y"; + +export interface ConfigEditorModalProps { + open: boolean; + onClose: () => void; + /** Called after a save/apply succeeds (Save/Save As — Tasks 7–8). */ + onApplied?: (data: AppConfigData) => void; +} + +/** Load lifecycle of the configuration text behind the editor. */ +type LoadState = + | { status: "loading" } + | { status: "error"; message: string } + | { status: "ready"; data: ConfigContentData }; + +/** + * Editor theme built from the app's `data-theme` CSS tokens (index.css + * `--app-*` variables). Only `var()` references are used — the editor + * follows the light/dark switch automatically, like every other surface. + */ +const configEditorTheme = EditorView.theme({ + "&": { + height: "100%", + fontSize: "12.5px", + color: "var(--app-text-primary)", + backgroundColor: "transparent", + }, + ".cm-scroller": { + fontFamily: "'JetBrains Mono', 'Fira Code', ui-monospace, SFMono-Regular, Menlo, Consolas, monospace", + lineHeight: "1.6", + }, + ".cm-content": { caretColor: "var(--color-indigo-500, #6366f1)" }, + ".cm-cursor, .cm-dropCursor": { borderLeftColor: "var(--color-indigo-500, #6366f1)" }, + "&.cm-focused": { outline: "none" }, + ".cm-selectionBackground, &.cm-focused .cm-selectionBackground": { + backgroundColor: "var(--app-hover)", + }, + ".cm-activeLine": { backgroundColor: "var(--app-elevated)" }, + ".cm-gutters": { + backgroundColor: "transparent", + color: "var(--app-text-faint)", + border: "none", + borderRight: "1px solid var(--app-border)", + }, + ".cm-activeLineGutter": { + backgroundColor: "var(--app-elevated)", + color: "var(--app-text-secondary)", + }, + ".cm-panels": { + backgroundColor: "var(--app-glass-panel-strong)", + color: "var(--app-text-primary)", + }, + ".cm-tooltip": { + border: "1px solid var(--app-border)", + backgroundColor: "var(--app-glass-panel-strong)", + color: "var(--app-text-primary)", + }, +}); + +/** + * Full-screen YAML configuration editor (issue #95). This shell owns the + * CodeMirror 6 instance, load-on-open, dirty tracking and the guarded + * Escape/✕ close; the toolbar action buttons (Save, Reload, …) are wired + * in Tasks 7–8. The header bar carries the "Open in OS" shortcut, which + * hands the file to the OS default editor via the existing client call. + */ +export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: ConfigEditorModalProps) { + const [load, setLoad] = useState({ status: "loading" }); + const [dirty, setDirty] = useState(false); + const [confirmDiscardOpen, setConfirmDiscardOpen] = useState(false); + + const dialogRef = useRef(null); + const editorHostRef = useRef(null); + const editorViewRef = useRef(null); + /** Text the editor was seeded with — the baseline for the dirty check. */ + const loadedTextRef = useRef(""); + + const handleDialogKeyDown = useModalA11y({ + isOpen: open, + onClose: requestClose, + dialogRef, + }); + + // Load-on-open; reset everything on close so a re-open starts clean. + useEffect(() => { + if (!open) { + loadedTextRef.current = ""; + editorViewRef.current = null; + setLoad({ status: "loading" }); + setDirty(false); + setConfirmDiscardOpen(false); + return; + } + let cancelled = false; + fetchConfigContent() + .then((data) => { + if (cancelled) return; + loadedTextRef.current = data.content; + setLoad({ status: "ready", data }); + }) + .catch((err: unknown) => { + if (cancelled) return; + loadedTextRef.current = ""; + setLoad({ status: "error", message: err instanceof Error ? err.message : String(err) }); + }); + return () => { + cancelled = true; + }; + }, [open]); + + // Mount the CodeMirror view once the content is on screen; destroy it on + // close/unmount. Depends on `load` so the host div is guaranteed mounted. + useEffect(() => { + if (!open || load.status !== "ready") return; + const host = editorHostRef.current; + if (!host) return; + const view = new EditorView({ + doc: load.data.content, + extensions: [ + basicSetup, + yaml(), + configEditorTheme, + EditorView.updateListener.of((update) => { + if (update.docChanged) { + setDirty(update.state.doc.toString() !== loadedTextRef.current); + } + }), + ], + parent: host, + }); + editorViewRef.current = view; + return () => { + view.destroy(); + editorViewRef.current = null; + }; + }, [open, load]); + + /** + * Close request shared by Escape, the ✕ button and backdrop clicks. + * A dirty editor (doc ≠ loaded text) gets the discard warning instead; + * a clean editor closes immediately. + */ + function requestClose() { + const view = editorViewRef.current; + if (view && view.state.doc.toString() !== loadedTextRef.current) { + setConfirmDiscardOpen(true); + return; + } + onClose(); + } + + const handleOpenInOS = useCallback(() => { + openConfigFile().catch((err: unknown) => { + console.error("Failed to open config file in OS editor:", err); + }); + }, []); + + if (!open) return null; + + const filename = load.status === "ready" ? load.data.filename : ""; + const sessionTrial = load.status === "ready" ? load.data.session_trial : false; + + return ( + <> +
+
e.stopPropagation()} + > + {/* Header bar */} +
+
+ + + +
+

+ Configuration Editor +

+

+ {filename || "\u2026"} + {sessionTrial ? " • session trial (not anchored to disk)" : ""} +

+
+
+
+ + +
+
+ + {/* Toolbar placeholder — action buttons (Save, Reload, …) are wired + here in Tasks 7–8; for now it carries the dirty indicator. */} +
+ {dirty && ( + + + Unsaved changes + + )} +
+ + {/* Editor body */} +
+ {load.status === "loading" ? ( +
+
+
+ ) : load.status === "error" ? ( +
+
+ {load.message} +
+
+ ) : ( +
+ )} +
+
+
+ + { + setConfirmDiscardOpen(false); + onClose(); + }} + onCancel={() => setConfirmDiscardOpen(false)} + /> + + ); +} diff --git a/src/profiles/web/tsconfig.tsbuildinfo b/src/profiles/web/tsconfig.tsbuildinfo index 42e771f..1a01420 100644 --- a/src/profiles/web/tsconfig.tsbuildinfo +++ b/src/profiles/web/tsconfig.tsbuildinfo @@ -1 +1 @@ -{"root":["./src/app.tsx","./src/main.tsx","./src/setup-tests.ts","./src/vite-env.d.ts","./src/vitest.d.ts","./src/api/client.test.ts","./src/api/client.ts","./src/api/types.ts","./src/components/actionbar.tsx","./src/components/barcodemodal.tsx","./src/components/camerabarcodemodal.tsx","./src/components/confirmdialog.tsx","./src/components/contextmenu.tsx","./src/components/copyableurl.test.tsx","./src/components/copyableurl.tsx","./src/components/directorypickermodal.tsx","./src/components/dropconfigoverlay.tsx","./src/components/editablecombobox.tsx","./src/components/errorboundary.tsx","./src/components/filelisttable.tsx","./src/components/filepropertiesmodal.tsx","./src/components/filtersyntaxhelper.tsx","./src/components/headerbar.tsx","./src/components/initerrorpanel.tsx","./src/components/loadconfigmodal.tsx","./src/components/logviewermodal.tsx","./src/components/markdownmessage.test.tsx","./src/components/markdownmessage.tsx","./src/components/scanstatusstrip.tsx","./src/components/searchbar.tsx","./src/components/shortcutsmodal.tsx","./src/components/singlematchpromptmodal.tsx","./src/components/statusbar.test.tsx","./src/components/statusbar.tsx","./src/components/timermodal.tsx","./src/components/tooltip.tsx","./src/components/workflowrunnermodal.test.tsx","./src/components/workflowrunnermodal.tsx","./src/utils/barcodedecoder.test.ts","./src/utils/barcodedecoder.ts","./src/utils/barcodescanner.ts","./src/utils/configdrop.test.ts","./src/utils/configdrop.ts","./src/utils/exportcsv.ts","./src/utils/fileproperties.test.ts","./src/utils/fileproperties.ts","./src/utils/searchsyntax.test.ts","./src/utils/searchsyntax.ts","./src/utils/theme.test.ts","./src/utils/theme.ts","./src/utils/usemodala11y.test.tsx","./src/utils/usemodala11y.ts"],"version":"5.7.3"} \ No newline at end of file +{"root":["./src/app.tsx","./src/main.tsx","./src/setup-tests.ts","./src/vite-env.d.ts","./src/vitest.d.ts","./src/api/client.test.ts","./src/api/client.ts","./src/api/types.ts","./src/components/actionbar.tsx","./src/components/barcodemodal.tsx","./src/components/camerabarcodemodal.tsx","./src/components/configeditormodal.test.tsx","./src/components/configeditormodal.tsx","./src/components/confirmdialog.tsx","./src/components/contextmenu.tsx","./src/components/copyableurl.test.tsx","./src/components/copyableurl.tsx","./src/components/directorypickermodal.tsx","./src/components/dropconfigoverlay.tsx","./src/components/editablecombobox.tsx","./src/components/errorboundary.tsx","./src/components/filelisttable.tsx","./src/components/filepropertiesmodal.tsx","./src/components/filtersyntaxhelper.tsx","./src/components/headerbar.tsx","./src/components/initerrorpanel.tsx","./src/components/loadconfigmodal.tsx","./src/components/logviewermodal.tsx","./src/components/markdownmessage.test.tsx","./src/components/markdownmessage.tsx","./src/components/scanstatusstrip.tsx","./src/components/searchbar.tsx","./src/components/shortcutsmodal.tsx","./src/components/singlematchpromptmodal.tsx","./src/components/statusbar.test.tsx","./src/components/statusbar.tsx","./src/components/timermodal.tsx","./src/components/tooltip.tsx","./src/components/workflowrunnermodal.test.tsx","./src/components/workflowrunnermodal.tsx","./src/utils/barcodedecoder.test.ts","./src/utils/barcodedecoder.ts","./src/utils/barcodescanner.ts","./src/utils/configdrop.test.ts","./src/utils/configdrop.ts","./src/utils/exportcsv.ts","./src/utils/fileproperties.test.ts","./src/utils/fileproperties.ts","./src/utils/searchsyntax.test.ts","./src/utils/searchsyntax.ts","./src/utils/theme.test.ts","./src/utils/theme.ts","./src/utils/usemodala11y.test.tsx","./src/utils/usemodala11y.ts"],"version":"5.7.3"} \ No newline at end of file From 4f2a92ecfc7780a3a1107ba4131726b877697119 Mon Sep 17 00:00:00 2001 From: falbany Date: Wed, 4 Nov 2026 00:16:11 +0100 Subject: [PATCH 11/19] fix(web): normalize CRLF baseline so untouched files are not dirty --- .../src/components/ConfigEditorModal.test.tsx | 24 +++++++++++++++ .../web/src/components/ConfigEditorModal.tsx | 30 ++++++++++++++----- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/profiles/web/src/components/ConfigEditorModal.test.tsx b/src/profiles/web/src/components/ConfigEditorModal.test.tsx index 9d97971..1b653ce 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.test.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.test.tsx @@ -148,6 +148,30 @@ describe("ConfigEditorModal", () => { expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); }); + it("does not flag a freshly opened CRLF file as dirty (regression)", async () => { + // CodeMirror stores LF line endings; the dirty baseline must be + // normalized the same way or an untouched CRLF .profiles file would + // trigger "Discard changes?" on its first close request. + fetchContentSpy.mockResolvedValue(makeContent({ content: "a: 1\r\nb: 2\r\n" })); + const user = userEvent.setup(); + const { onClose } = renderModal(); + + await readyEditorView(); + // Untouched file: no dirty chip, and the fetched text round-trips + // through the editor as LF. + expect(screen.queryByRole("status")).toBeNull(); + const view = EditorView.findFromDOM( + document.querySelector(".cm-editor") as HTMLElement, + ); + expect(view?.state.doc.toString()).toBe("a: 1\nb: 2\n"); + + // Escape closes immediately — no discard warning. + screen.getByRole("button", { name: /open in os/i }).focus(); + await user.keyboard("{Escape}"); + expect(onClose).toHaveBeenCalledTimes(1); + expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); + }); + it("test_header_open_in_os_calls_open_config_file", async () => { fetchContentSpy.mockResolvedValue(makeContent()); openInOSSpy.mockResolvedValue({ status: "SUCCESS", message: "Opened" }); diff --git a/src/profiles/web/src/components/ConfigEditorModal.tsx b/src/profiles/web/src/components/ConfigEditorModal.tsx index cab195e..44ae420 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.tsx @@ -65,6 +65,22 @@ const configEditorTheme = EditorView.theme({ }, }); +/** + * Normalize fetched content to the LF line endings CodeMirror stores + * (CRLF is normalized away when the doc is created). The dirty check + * compares the doc against this baseline; without it an untouched CRLF + * file — realistic for .profiles files on Windows hosts — would read as + * dirty on its very first Escape/✕/backdrop close. + */ +function toEditorText(content: string): string { + return content.replace(/\r\n?/g, "\n"); +} + +/** True when the editor document differs from the text it was loaded with. */ +function isDirty(view: EditorView | null, loadedText: string): boolean { + return view !== null && view.state.doc.toString() !== loadedText; +} + /** * Full-screen YAML configuration editor (issue #95). This shell owns the * CodeMirror 6 instance, load-on-open, dirty tracking and the guarded @@ -103,8 +119,9 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf fetchConfigContent() .then((data) => { if (cancelled) return; - loadedTextRef.current = data.content; - setLoad({ status: "ready", data }); + const content = toEditorText(data.content); + loadedTextRef.current = content; + setLoad({ status: "ready", data: { ...data, content } }); }) .catch((err: unknown) => { if (cancelled) return; @@ -130,7 +147,7 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf configEditorTheme, EditorView.updateListener.of((update) => { if (update.docChanged) { - setDirty(update.state.doc.toString() !== loadedTextRef.current); + setDirty(isDirty(update.view, loadedTextRef.current)); } }), ], @@ -145,12 +162,11 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf /** * Close request shared by Escape, the ✕ button and backdrop clicks. - * A dirty editor (doc ≠ loaded text) gets the discard warning instead; - * a clean editor closes immediately. + * A dirty editor (per the shared isDirty predicate) gets the discard + * warning instead; a clean editor closes immediately. */ function requestClose() { - const view = editorViewRef.current; - if (view && view.state.doc.toString() !== loadedTextRef.current) { + if (isDirty(editorViewRef.current, loadedTextRef.current)) { setConfirmDiscardOpen(true); return; } From 4da31c2b921070e1e39bd4c421c24a236d76deb4 Mon Sep 17 00:00:00 2001 From: falbany Date: Wed, 4 Nov 2026 00:26:20 +0100 Subject: [PATCH 12/19] feat(web): editor toolbar actions New/Test/Revert/Apply --- .../src/components/ConfigEditorModal.test.tsx | 207 +++++++++++++- .../web/src/components/ConfigEditorModal.tsx | 257 +++++++++++++++++- 2 files changed, 450 insertions(+), 14 deletions(-) diff --git a/src/profiles/web/src/components/ConfigEditorModal.test.tsx b/src/profiles/web/src/components/ConfigEditorModal.test.tsx index 1b653ce..0b10044 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.test.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.test.tsx @@ -4,12 +4,21 @@ import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { EditorView } from "codemirror"; import { ConfigEditorModal } from "./ConfigEditorModal"; -import { fetchConfigContent, openConfigFile } from "../api/client"; -import type { ConfigContentData } from "../api/types"; +import { + fetchConfigContent, + fetchConfigTemplate, + loadConfigContent, + openConfigFile, + validateConfigContent, +} from "../api/client"; +import type { AppConfigData, ConfigContentData } from "../api/types"; vi.mock("../api/client", () => ({ fetchConfigContent: vi.fn(), openConfigFile: vi.fn(), + fetchConfigTemplate: vi.fn(), + validateConfigContent: vi.fn(), + loadConfigContent: vi.fn(), })); // jsdom gap (not a CodeMirror mock): the view's measurement code calls @@ -25,8 +34,14 @@ rangeProto.getBoundingClientRect ??= () => new DOMRect(0, 0, 0, 0); const fetchContentSpy = vi.mocked(fetchConfigContent); const openInOSSpy = vi.mocked(openConfigFile); +const templateSpy = vi.mocked(fetchConfigTemplate); +const validateSpy = vi.mocked(validateConfigContent); +const loadContentSpy = vi.mocked(loadConfigContent); const SAMPLE_YAML = 'release: "1.0"\nsearch_dir: "C:/data"\n'; +/** Starter template served with CRLF endings — realistic for Windows hosts. */ +const TEMPLATE_YAML = 'release: "2.0"\r\nsearch_dir: "D:/data"\r\n'; +const TEMPLATE_YAML_LF = 'release: "2.0"\nsearch_dir: "D:/data"\n'; function makeContent(overrides: Partial = {}): ConfigContentData { return { @@ -38,10 +53,36 @@ function makeContent(overrides: Partial = {}): ConfigContentD }; } +function makeAppConfig(): AppConfigData { + return { + title: "machine", + release: "1.0", + search_dir: "C:/data", + recursive_search: false, + theme: "dark", + directories: [], + extensions: [".mttl"], + filters: [], + active_directory: "C:/data", + config_path: "/home/user/.profiles/machine.profiles", + config_exists: true, + column_headers: [], + column_names: [], + row_colors: [], + }; +} + function renderModal(overrides: { open?: boolean } = {}) { const onClose = vi.fn(); - render(); - return { onClose }; + const onApplied = vi.fn(); + render( + , + ); + return { onClose, onApplied }; } /** @@ -185,4 +226,162 @@ describe("ConfigEditorModal", () => { expect(consoleSpy).not.toHaveBeenCalled(); consoleSpy.mockRestore(); }); + + /** Dispatch a user edit through the real editor transaction seam. */ + async function editDoc(insert: string): Promise { + const view = await readyEditorView(); + await act(async () => { + view.dispatch({ changes: { from: 0, insert } }); + }); + return view.state.doc.toString(); + } + + it("test_new_loads_template_after_confirm", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + templateSpy.mockResolvedValue({ content: TEMPLATE_YAML, filename: "starter.profiles" }); + const user = userEvent.setup(); + const { onClose } = renderModal(); + + await editDoc("# edited\n"); + expect(screen.getByRole("status")).toHaveTextContent(/unsaved changes/i); + + // Dirty: New must ask before replacing. + await user.click(screen.getByRole("button", { name: /^new$/i })); + const warning = screen.getByRole("dialog", { name: /replace current edits\?/i }); + expect(warning).toBeTruthy(); + expect(templateSpy).not.toHaveBeenCalled(); + + // Confirming loads the starter template into the real editor. + await user.click(screen.getByRole("button", { name: /^replace$/i })); + expect(templateSpy).toHaveBeenCalledTimes(1); + await waitFor(() => { + const view = EditorView.findFromDOM(document.querySelector(".cm-editor") as HTMLElement); + expect(view?.state.doc.toString()).toBe(TEMPLATE_YAML_LF); + }); + // Dirty cleared (CRLF template text becomes the new baseline). + expect(screen.queryByRole("status")).toBeNull(); + // Escape now closes immediately — the baseline moved with the template. + screen.getByRole("button", { name: /open in os/i }).focus(); + await user.keyboard("{Escape}"); + expect(onClose).toHaveBeenCalledTimes(1); + expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); + }); + + it("test_new_without_dirty_loads_immediately", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + templateSpy.mockResolvedValue({ content: TEMPLATE_YAML_LF, filename: "starter.profiles" }); + const user = userEvent.setup(); + renderModal(); + + await readyEditorView(); + await user.click(screen.getByRole("button", { name: /^new$/i })); + expect(templateSpy).toHaveBeenCalledTimes(1); + await waitFor(() => { + const view = EditorView.findFromDOM(document.querySelector(".cm-editor") as HTMLElement); + expect(view?.state.doc.toString()).toBe(TEMPLATE_YAML_LF); + }); + expect(screen.queryByRole("dialog", { name: /replace current edits\?/i })).toBeNull(); + expect(screen.queryByRole("status")).toBeNull(); + }); + + it("test_test_shows_valid_badge", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + const user = userEvent.setup(); + renderModal(); + + await readyEditorView(); + await user.click(screen.getByRole("button", { name: /^test$/i })); + await waitFor(() => + expect(validateSpy).toHaveBeenCalledWith("machine.profiles", SAMPLE_YAML), + ); + const banner = await screen.findByTestId("editor-status"); + expect(banner.textContent).toMatch(/valid/i); + expect(banner.className).toContain("status-success"); + expect(screen.queryByTestId("editor-issues")).toBeNull(); + }); + + it("test_test_shows_issue_list", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ + valid: false, + issues: [ + { severity: "error", message: "unknown key 'foo' at line 3" }, + { severity: "warning", message: "search_dir does not exist" }, + ], + }); + const user = userEvent.setup(); + renderModal(); + + await readyEditorView(); + await user.click(screen.getByRole("button", { name: /^test$/i })); + const list = await screen.findByTestId("editor-issues"); + expect(list.textContent).toContain("unknown key 'foo' at line 3"); + expect(list.textContent).toContain("search_dir does not exist"); + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-danger"); + expect(banner.textContent).toMatch(/2/); + }); + + it("test_apply_calls_load_content_and_reports_success", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + const applied = makeAppConfig(); + loadContentSpy.mockResolvedValue(applied); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + // The applied text is the current doc, edits included. + const edited = await editDoc("# edited\n"); + await user.click(screen.getByRole("button", { name: /^apply$/i })); + await waitFor(() => + expect(loadContentSpy).toHaveBeenCalledWith("machine.profiles", edited), + ); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + expect(onApplied.mock.calls[0][0]).toBe(applied); + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-success"); + expect(banner.textContent).toMatch(/applied/i); + }); + + it("test_apply_error_shows_server_message", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + loadContentSpy.mockRejectedValue(new Error("unknown key 'filters_xyz' at line 4")); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + await readyEditorView(); + await user.click(screen.getByRole("button", { name: /^apply$/i })); + const banner = await screen.findByTestId("editor-status"); + expect(banner.textContent).toContain("unknown key 'filters_xyz' at line 4"); + expect(banner.className).toContain("status-danger"); + expect(onApplied).not.toHaveBeenCalled(); + }); + + it("test_revert_reloads_from_server_after_confirm", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + const user = userEvent.setup(); + const { onClose } = renderModal(); + + await editDoc("# edited\n"); + expect(fetchContentSpy).toHaveBeenCalledTimes(1); + + // Dirty: Revert must confirm before discarding, like New. + await user.click(screen.getByRole("button", { name: /^revert$/i })); + expect(screen.getByRole("dialog", { name: /replace current edits\?/i })).toBeTruthy(); + expect(fetchContentSpy).toHaveBeenCalledTimes(1); + + // Confirming re-fetches the server content and restores the doc. + await user.click(screen.getByRole("button", { name: /^replace$/i })); + await waitFor(() => expect(fetchContentSpy).toHaveBeenCalledTimes(2)); + await waitFor(() => { + const view = EditorView.findFromDOM(document.querySelector(".cm-editor") as HTMLElement); + expect(view?.state.doc.toString()).toBe(SAMPLE_YAML); + }); + expect(screen.queryByRole("status")).toBeNull(); + // Clean again: Escape closes immediately. + screen.getByRole("button", { name: /open in os/i }).focus(); + await user.keyboard("{Escape}"); + expect(onClose).toHaveBeenCalledTimes(1); + expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); + }); }); diff --git a/src/profiles/web/src/components/ConfigEditorModal.tsx b/src/profiles/web/src/components/ConfigEditorModal.tsx index 44ae420..6d2b373 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.tsx @@ -1,17 +1,32 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { basicSetup, EditorView } from "codemirror"; import { yaml } from "@codemirror/lang-yaml"; -import { CircleDot, ExternalLink, FileCode2, X } from "lucide-react"; +import { + CircleDot, + ExternalLink, + FileCode2, + FilePlus2, + FlaskConical, + Play, + RotateCcw, + X, +} from "lucide-react"; -import { fetchConfigContent, openConfigFile } from "../api/client"; -import type { AppConfigData, ConfigContentData } from "../api/types"; +import { + fetchConfigContent, + fetchConfigTemplate, + loadConfigContent, + openConfigFile, + validateConfigContent, +} from "../api/client"; +import type { AppConfigData, ConfigContentData, ValidationIssue } from "../api/types"; import { ConfirmDialog } from "./ConfirmDialog"; import { useModalA11y } from "../utils/useModalA11y"; export interface ConfigEditorModalProps { open: boolean; onClose: () => void; - /** Called after a save/apply succeeds (Save/Save As — Tasks 7–8). */ + /** Called after Apply succeeds (Save/Save As fire it too — Task 8). */ onApplied?: (data: AppConfigData) => void; } @@ -21,6 +36,12 @@ type LoadState = | { status: "error"; message: string } | { status: "ready"; data: ConfigContentData }; +/** Inline feedback banner shown above the editor after a toolbar action. */ +type EditorStatus = { kind: "ok" | "error"; text: string } | null; + +/** Toolbar action parked behind the "Replace current edits?" confirmation. */ +type PendingAction = "new" | "revert"; + /** * Editor theme built from the app's `data-theme` CSS tokens (index.css * `--app-*` variables). Only `var()` references are used — the editor @@ -84,14 +105,22 @@ function isDirty(view: EditorView | null, loadedText: string): boolean { /** * Full-screen YAML configuration editor (issue #95). This shell owns the * CodeMirror 6 instance, load-on-open, dirty tracking and the guarded - * Escape/✕ close; the toolbar action buttons (Save, Reload, …) are wired - * in Tasks 7–8. The header bar carries the "Open in OS" shortcut, which - * hands the file to the OS default editor via the existing client call. + * Escape/✕ close. The toolbar carries the content actions — New (starter + * template), Test (in-memory validation), Revert (reload from disk) and + * Apply (session-trial the current text); Save/Save-as arrive with Task 8. + * The header bar carries the "Open in OS" shortcut, which hands the file + * to the OS default editor via the existing client call. */ -export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: ConfigEditorModalProps) { +export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModalProps) { const [load, setLoad] = useState({ status: "loading" }); const [dirty, setDirty] = useState(false); const [confirmDiscardOpen, setConfirmDiscardOpen] = useState(false); + /** Banner above the editor — Test/Apply/New/Revert outcomes. */ + const [status, setStatus] = useState(null); + /** Validation issues from the Test action, listed under the banner. */ + const [issues, setIssues] = useState([]); + /** Action awaiting its "Replace current edits?" confirmation. */ + const [pendingAction, setPendingAction] = useState(null); const dialogRef = useRef(null); const editorHostRef = useRef(null); @@ -113,6 +142,9 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf setLoad({ status: "loading" }); setDirty(false); setConfirmDiscardOpen(false); + setStatus(null); + setIssues([]); + setPendingAction(null); return; } let cancelled = false; @@ -179,10 +211,118 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf }); }, []); + /** Current editor text — falls back to the baseline before the view mounts. */ + function currentDocText(): string { + return editorViewRef.current?.state.doc.toString() ?? loadedTextRef.current; + } + + /** + * Replace the editor document with the starter template (New action). + * The template text becomes the new dirty baseline, so an untouched + * template is not flagged as unsaved. Only `load` fields that describe + * the text are updated — the CodeMirror view remounts from it. + */ + const loadTemplateIntoEditor = useCallback(async () => { + setStatus(null); + setIssues([]); + try { + const template = await fetchConfigTemplate(); + const content = toEditorText(template.content); + loadedTextRef.current = content; + setDirty(false); + setLoad( + (prev): LoadState => + prev.status === "ready" + ? { + status: "ready", + data: { ...prev.data, filename: template.filename, content, config_path: null }, + } + : prev, + ); + setStatus({ kind: "ok", text: "New configuration template loaded." }); + } catch (err: unknown) { + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }, []); + + /** Reload the active configuration file from the server (Revert action). */ + const reloadFromServer = useCallback(async () => { + setStatus(null); + setIssues([]); + try { + const data = await fetchConfigContent(); + const content = toEditorText(data.content); + loadedTextRef.current = content; + setDirty(false); + setLoad({ status: "ready", data: { ...data, content } }); + setStatus({ kind: "ok", text: "Configuration reloaded from disk." }); + } catch (err: unknown) { + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }, []); + + /** New action: a dirty editor confirms before its edits are discarded. */ + function requestNew() { + if (isDirty(editorViewRef.current, loadedTextRef.current)) { + setPendingAction("new"); + return; + } + void loadTemplateIntoEditor(); + } + + /** Revert action: same guarded pattern as New, reloading from the server. */ + function requestRevert() { + if (isDirty(editorViewRef.current, loadedTextRef.current)) { + setPendingAction("revert"); + return; + } + void reloadFromServer(); + } + + /** + * Test action: validate the current text in memory. A valid result + * lights the ok banner; issues are listed above the editor verbatim. + */ + const runValidation = useCallback(async () => { + if (load.status !== "ready") return; + try { + const result = await validateConfigContent(load.data.filename, currentDocText()); + setIssues(result.issues); + setStatus( + result.valid + ? { kind: "ok", text: "Configuration is valid." } + : { + kind: "error", + text: `Validation failed with ${result.issues.length} issue${result.issues.length === 1 ? "" : "s"}.`, + }, + ); + } catch (err: unknown) { + setIssues([]); + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }, [load]); + + /** + * Apply action: trial the current text as the session configuration. + * Success fires `onApplied` with the applied AppConfigData; a 400 + * rejection surfaces the server's plain-string detail in the banner. + */ + const applyContent = useCallback(async () => { + if (load.status !== "ready") return; + try { + const data = await loadConfigContent(load.data.filename, currentDocText()); + setStatus({ kind: "ok", text: "Configuration applied (session trial)." }); + onApplied?.(data); + } catch (err: unknown) { + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }, [load, onApplied]); + if (!open) return null; const filename = load.status === "ready" ? load.data.filename : ""; const sessionTrial = load.status === "ready" ? load.data.session_trial : false; + const toolbarDisabled = load.status !== "ready"; return ( <> @@ -236,12 +376,53 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf
- {/* Toolbar placeholder — action buttons (Save, Reload, …) are wired - here in Tasks 7–8; for now it carries the dirty indicator. */} + {/* Toolbar — content actions; Save/Save-as arrive with Task 8. */}
+
+ + + + +
{dirty && ( + {/* Inline feedback banner + validation issue list (Test outcome). */} + {(status !== null || issues.length > 0) && ( +
+ {status !== null && ( +
+ {status.text} +
+ )} + {issues.length > 0 && ( +
    + {issues.map((issue, index) => ( +
  • + {issue.message} +
  • + ))} +
+ )} +
+ )} + {/* Editor body */}
{load.status === "loading" ? ( @@ -287,6 +503,27 @@ export function ConfigEditorModal({ open, onClose, onApplied: _onApplied }: Conf }} onCancel={() => setConfirmDiscardOpen(false)} /> + + {/* Same discard-warning pattern for New/Revert on a dirty editor. */} + { + const action = pendingAction; + setPendingAction(null); + if (action === "new") void loadTemplateIntoEditor(); + else if (action === "revert") void reloadFromServer(); + }} + onCancel={() => setPendingAction(null)} + /> ); } From 7e3e92e786e5bdefa32bd8e2b4c7f6eb922081dc Mon Sep 17 00:00:00 2001 From: falbany Date: Wed, 4 Nov 2026 00:37:10 +0100 Subject: [PATCH 13/19] fix(web): guard editor toolbar actions against stale and duplicate dispatches --- .../src/components/ConfigEditorModal.test.tsx | 75 ++++++- .../web/src/components/ConfigEditorModal.tsx | 197 ++++++++++++------ 2 files changed, 207 insertions(+), 65 deletions(-) diff --git a/src/profiles/web/src/components/ConfigEditorModal.test.tsx b/src/profiles/web/src/components/ConfigEditorModal.test.tsx index 0b10044..ea3c82e 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.test.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.test.tsx @@ -11,7 +11,7 @@ import { openConfigFile, validateConfigContent, } from "../api/client"; -import type { AppConfigData, ConfigContentData } from "../api/types"; +import type { AppConfigData, ConfigContentData, ValidationIssue } from "../api/types"; vi.mock("../api/client", () => ({ fetchConfigContent: vi.fn(), @@ -357,6 +357,79 @@ describe("ConfigEditorModal", () => { expect(onApplied).not.toHaveBeenCalled(); }); + it("test_apply_double_click_dispatches_once", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + // Deferred apply: the first request stays pending across both clicks. + let resolveApply!: (value: AppConfigData) => void; + loadContentSpy.mockReturnValue( + new Promise((resolve) => { + resolveApply = resolve; + }), + ); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + const applyButton = await screen.findByRole("button", { name: /^apply$/i }); + await user.click(applyButton); + // Second click while the first apply is in flight must be swallowed. + await user.click(applyButton); + expect(loadContentSpy).toHaveBeenCalledTimes(1); + + // The in-flight apply still completes exactly once. + resolveApply(makeAppConfig()); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + expect(loadContentSpy).toHaveBeenCalledTimes(1); + expect(screen.getByTestId("editor-status").textContent).toMatch(/applied/i); + + // The gate is released once the action settles — a follow-up works. + loadContentSpy.mockResolvedValue(makeAppConfig()); + await user.click(applyButton); + await waitFor(() => expect(loadContentSpy).toHaveBeenCalledTimes(2)); + expect(onApplied).toHaveBeenCalledTimes(2); + }); + + it("test_stale_test_resolution_dropped_after_new", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + templateSpy.mockResolvedValue({ content: TEMPLATE_YAML_LF, filename: "starter.profiles" }); + // Deferred validate: the Test round-trip stays pending on purpose. + let resolveValidate!: (value: { valid: boolean; issues: ValidationIssue[] }) => void; + validateSpy.mockReturnValue( + new Promise((resolve) => { + resolveValidate = resolve; + }), + ); + const user = userEvent.setup(); + renderModal(); + + // Dirty doc so New routes through the confirm dialog. + await editDoc("# edited\n"); + + // Test dispatches; its response is still pending… + await user.click(screen.getByRole("button", { name: /^test$/i })); + expect(validateSpy).toHaveBeenCalledTimes(1); + + // …when New + confirm dispatch and complete. + await user.click(screen.getByRole("button", { name: /^new$/i })); + await user.click(screen.getByRole("button", { name: /^replace$/i })); + await waitFor(() => { + const view = EditorView.findFromDOM(document.querySelector(".cm-editor") as HTMLElement); + expect(view?.state.doc.toString()).toBe(TEMPLATE_YAML_LF); + }); + + // The stale Test resolution lands after New completed: it belongs to the + // discarded document and must not render its issues over the template. + await act(async () => { + resolveValidate({ + valid: false, + issues: [{ severity: "error", message: "stale issue from discarded doc" }], + }); + }); + expect(screen.queryByTestId("editor-issues")).toBeNull(); + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-success"); + expect(banner.textContent).toMatch(/template loaded/i); + }); + it("test_revert_reloads_from_server_after_confirm", async () => { fetchContentSpy.mockResolvedValue(makeContent()); const user = userEvent.setup(); diff --git a/src/profiles/web/src/components/ConfigEditorModal.tsx b/src/profiles/web/src/components/ConfigEditorModal.tsx index 6d2b373..0fe0642 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.tsx @@ -42,6 +42,9 @@ type EditorStatus = { kind: "ok" | "error"; text: string } | null; /** Toolbar action parked behind the "Replace current edits?" confirmation. */ type PendingAction = "new" | "revert"; +/** Kinds of the async toolbar actions — the dispatch runner's key. */ +type ToolbarActionKind = "new" | "test" | "revert" | "apply"; + /** * Editor theme built from the app's `data-theme` CSS tokens (index.css * `--app-*` variables). Only `var()` references are used — the editor @@ -127,6 +130,16 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa const editorViewRef = useRef(null); /** Text the editor was seeded with — the baseline for the dirty check. */ const loadedTextRef = useRef(""); + /** + * Monotonic id of the most recent toolbar-action dispatch. Each dispatch + * tags its round trip with the id current at dispatch time; a resolution + * whose id is no longer current — superseded by a newer action (Test + * landing after a New replaced the document) or left over from a closed + * session — is dropped before it can setState. + */ + const actionSeqRef = useRef(0); + /** In-flight registry: action kind → seq of its live dispatch. A kind already registered rejects duplicate dispatches (double-click Apply fires one request). */ + const inFlightActionsRef = useRef>(new Map()); const handleDialogKeyDown = useModalA11y({ isOpen: open, @@ -145,6 +158,10 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa setStatus(null); setIssues([]); setPendingAction(null); + // Invalidate in-flight toolbar actions so a resolution that lands + // after close/reopen cannot leak into the next session's state. + actionSeqRef.current += 1; + inFlightActionsRef.current.clear(); return; } let cancelled = false; @@ -216,50 +233,90 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa return editorViewRef.current?.state.doc.toString() ?? loadedTextRef.current; } + /** + * Shared dispatch runner for the four async toolbar actions. Two guards, + * applied uniformly: + * 1. Duplicate dispatch — a kind already in flight (double-click) is + * swallowed synchronously, so exactly one request leaves per click + * burst and `onApplied` fires once per successful Apply. + * 2. Stale resolution — every dispatch tags its round trip with the + * monotonic seq; `work` checks it before any setState so results from + * a superseded action or a closed session never render. + */ + const runToolbarAction = useCallback( + async (kind: ToolbarActionKind, work: (seq: number) => Promise): Promise => { + if (inFlightActionsRef.current.has(kind)) return; + const seq = ++actionSeqRef.current; + inFlightActionsRef.current.set(kind, seq); + try { + await work(seq); + } finally { + // Release the gate only if this dispatch still owns it — a + // close+reopen may already have started a newer dispatch. + if (inFlightActionsRef.current.get(kind) === seq) { + inFlightActionsRef.current.delete(kind); + } + } + }, + [], + ); + /** * Replace the editor document with the starter template (New action). * The template text becomes the new dirty baseline, so an untouched * template is not flagged as unsaved. Only `load` fields that describe * the text are updated — the CodeMirror view remounts from it. */ - const loadTemplateIntoEditor = useCallback(async () => { - setStatus(null); - setIssues([]); - try { - const template = await fetchConfigTemplate(); - const content = toEditorText(template.content); - loadedTextRef.current = content; - setDirty(false); - setLoad( - (prev): LoadState => - prev.status === "ready" - ? { - status: "ready", - data: { ...prev.data, filename: template.filename, content, config_path: null }, - } - : prev, - ); - setStatus({ kind: "ok", text: "New configuration template loaded." }); - } catch (err: unknown) { - setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); - } - }, []); + const loadTemplateIntoEditor = useCallback( + () => + runToolbarAction("new", async (seq) => { + setStatus(null); + setIssues([]); + try { + const template = await fetchConfigTemplate(); + if (seq !== actionSeqRef.current) return; // superseded — drop stale results + const content = toEditorText(template.content); + loadedTextRef.current = content; + setDirty(false); + setLoad( + (prev): LoadState => + prev.status === "ready" + ? { + status: "ready", + data: { ...prev.data, filename: template.filename, content, config_path: null }, + } + : prev, + ); + setStatus({ kind: "ok", text: "New configuration template loaded." }); + } catch (err: unknown) { + if (seq !== actionSeqRef.current) return; + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }), + [runToolbarAction], + ); /** Reload the active configuration file from the server (Revert action). */ - const reloadFromServer = useCallback(async () => { - setStatus(null); - setIssues([]); - try { - const data = await fetchConfigContent(); - const content = toEditorText(data.content); - loadedTextRef.current = content; - setDirty(false); - setLoad({ status: "ready", data: { ...data, content } }); - setStatus({ kind: "ok", text: "Configuration reloaded from disk." }); - } catch (err: unknown) { - setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); - } - }, []); + const reloadFromServer = useCallback( + () => + runToolbarAction("revert", async (seq) => { + setStatus(null); + setIssues([]); + try { + const data = await fetchConfigContent(); + if (seq !== actionSeqRef.current) return; + const content = toEditorText(data.content); + loadedTextRef.current = content; + setDirty(false); + setLoad({ status: "ready", data: { ...data, content } }); + setStatus({ kind: "ok", text: "Configuration reloaded from disk." }); + } catch (err: unknown) { + if (seq !== actionSeqRef.current) return; + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }), + [runToolbarAction], + ); /** New action: a dirty editor confirms before its edits are discarded. */ function requestNew() { @@ -283,40 +340,52 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa * Test action: validate the current text in memory. A valid result * lights the ok banner; issues are listed above the editor verbatim. */ - const runValidation = useCallback(async () => { - if (load.status !== "ready") return; - try { - const result = await validateConfigContent(load.data.filename, currentDocText()); - setIssues(result.issues); - setStatus( - result.valid - ? { kind: "ok", text: "Configuration is valid." } - : { - kind: "error", - text: `Validation failed with ${result.issues.length} issue${result.issues.length === 1 ? "" : "s"}.`, - }, - ); - } catch (err: unknown) { - setIssues([]); - setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); - } - }, [load]); + const runValidation = useCallback( + () => + runToolbarAction("test", async (seq) => { + if (load.status !== "ready") return; + try { + const result = await validateConfigContent(load.data.filename, currentDocText()); + if (seq !== actionSeqRef.current) return; // superseded — drop stale results + setIssues(result.issues); + setStatus( + result.valid + ? { kind: "ok", text: "Configuration is valid." } + : { + kind: "error", + text: `Validation failed with ${result.issues.length} issue${result.issues.length === 1 ? "" : "s"}.`, + }, + ); + } catch (err: unknown) { + if (seq !== actionSeqRef.current) return; + setIssues([]); + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }), + [load, runToolbarAction], + ); /** * Apply action: trial the current text as the session configuration. * Success fires `onApplied` with the applied AppConfigData; a 400 * rejection surfaces the server's plain-string detail in the banner. */ - const applyContent = useCallback(async () => { - if (load.status !== "ready") return; - try { - const data = await loadConfigContent(load.data.filename, currentDocText()); - setStatus({ kind: "ok", text: "Configuration applied (session trial)." }); - onApplied?.(data); - } catch (err: unknown) { - setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); - } - }, [load, onApplied]); + const applyContent = useCallback( + () => + runToolbarAction("apply", async (seq) => { + if (load.status !== "ready") return; + try { + const data = await loadConfigContent(load.data.filename, currentDocText()); + if (seq !== actionSeqRef.current) return; // superseded — drop stale results + setStatus({ kind: "ok", text: "Configuration applied (session trial)." }); + onApplied?.(data); + } catch (err: unknown) { + if (seq !== actionSeqRef.current) return; + setStatus({ kind: "error", text: err instanceof Error ? err.message : String(err) }); + } + }), + [load, onApplied, runToolbarAction], + ); if (!open) return null; From ceb1350be92b79e3989adf1857261c2333697898 Mon Sep 17 00:00:00 2001 From: falbany Date: Wed, 4 Nov 2026 00:53:50 +0100 Subject: [PATCH 14/19] feat(web): save/save-as with compliance gate, force option and replace confirmation --- .../src/components/ConfigEditorModal.test.tsx | 278 ++++++++++++++- .../web/src/components/ConfigEditorModal.tsx | 322 +++++++++++++++++- .../web/src/components/SaveAsDialog.test.tsx | 170 +++++++++ .../web/src/components/SaveAsDialog.tsx | 314 +++++++++++++++++ 4 files changed, 1073 insertions(+), 11 deletions(-) create mode 100644 src/profiles/web/src/components/SaveAsDialog.test.tsx create mode 100644 src/profiles/web/src/components/SaveAsDialog.tsx diff --git a/src/profiles/web/src/components/ConfigEditorModal.test.tsx b/src/profiles/web/src/components/ConfigEditorModal.test.tsx index ea3c82e..3c77134 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.test.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.test.tsx @@ -1,17 +1,20 @@ // @vitest-environment jsdom import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; +import { act, cleanup, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { EditorView } from "codemirror"; import { ConfigEditorModal } from "./ConfigEditorModal"; import { + browseDirectory, fetchConfigContent, fetchConfigTemplate, loadConfigContent, openConfigFile, + saveConfigFile, + saveConfigFileAs, validateConfigContent, } from "../api/client"; -import type { AppConfigData, ConfigContentData, ValidationIssue } from "../api/types"; +import type { AppConfigData, BrowseResult, ConfigContentData, ValidationIssue } from "../api/types"; vi.mock("../api/client", () => ({ fetchConfigContent: vi.fn(), @@ -19,6 +22,9 @@ vi.mock("../api/client", () => ({ fetchConfigTemplate: vi.fn(), validateConfigContent: vi.fn(), loadConfigContent: vi.fn(), + saveConfigFile: vi.fn(), + saveConfigFileAs: vi.fn(), + browseDirectory: vi.fn(), })); // jsdom gap (not a CodeMirror mock): the view's measurement code calls @@ -37,6 +43,9 @@ const openInOSSpy = vi.mocked(openConfigFile); const templateSpy = vi.mocked(fetchConfigTemplate); const validateSpy = vi.mocked(validateConfigContent); const loadContentSpy = vi.mocked(loadConfigContent); +const saveSpy = vi.mocked(saveConfigFile); +const saveAsSpy = vi.mocked(saveConfigFileAs); +const browseSpy = vi.mocked(browseDirectory); const SAMPLE_YAML = 'release: "1.0"\nsearch_dir: "C:/data"\n'; /** Starter template served with CRLF endings — realistic for Windows hosts. */ @@ -72,6 +81,18 @@ function makeAppConfig(): AppConfigData { }; } +/** Listing served by /api/fs/browse for the Save As dialog. */ +function makeBrowseResult(overrides: Partial = {}): BrowseResult { + return { + current_path: "/home/user/configs", + parent_path: "/home/user", + items: [ + { name: "other.profiles", path: "/home/user/configs/other.profiles", is_dir: false, is_config: true }, + ], + ...overrides, + }; +} + function renderModal(overrides: { open?: boolean } = {}) { const onClose = vi.fn(); const onApplied = vi.fn(); @@ -457,4 +478,257 @@ describe("ConfigEditorModal", () => { expect(onClose).toHaveBeenCalledTimes(1); expect(screen.queryByRole("dialog", { name: /discard changes\?/i })).toBeNull(); }); + + it("test_save_validates_then_saves", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + const savedConfig = makeAppConfig(); + saveSpy.mockResolvedValue(savedConfig); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + // The saved text is the current doc, edits included. + const edited = await editDoc("# edited\n"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + + // The client-side gate validation runs first, on the current doc text… + await waitFor(() => expect(validateSpy).toHaveBeenCalledWith("machine.profiles", edited)); + // …then the save endpoint writes it, unforced. + await waitFor(() => expect(saveSpy).toHaveBeenCalledWith(edited, false)); + expect(saveAsSpy).not.toHaveBeenCalled(); + + // Success surface: ok banner, dirty cleared (baseline = saved text), + // onApplied with the fresh AppConfigData. + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-success"); + expect(banner.textContent).toMatch(/saved/i); + expect(screen.queryByRole("status")).toBeNull(); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + expect(onApplied.mock.calls[0][0]).toBe(savedConfig); + + // Ctrl/Cmd+S on a disk-anchored editor triggers the same save flow. + await user.keyboard("{Control>}s"); + await waitFor(() => expect(validateSpy).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(saveSpy).toHaveBeenCalledTimes(2)); + expect(saveSpy.mock.calls[1]).toEqual([edited, false]); + expect(onApplied).toHaveBeenCalledTimes(2); + }); + + it("test_save_invalid_shows_gate_with_force_option", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ + valid: false, + issues: [ + { severity: "error", message: "unknown key 'foo' at line 3" }, + { severity: "warning", message: "search_dir does not exist" }, + ], + }); + const user = userEvent.setup(); + renderModal(); + + await readyEditorView(); + await user.click(screen.getByRole("button", { name: /^save$/i })); + + // The gate lists the issues and offers both exits; nothing is saved yet. + const gate = await screen.findByRole("dialog", { name: /validation issues/i }); + expect(gate.textContent).toContain("unknown key 'foo' at line 3"); + expect(gate.textContent).toContain("search_dir does not exist"); + expect(screen.getByRole("button", { name: /go back and fix/i })).toBeTruthy(); + expect(screen.getByRole("button", { name: /force save anyway/i })).toBeTruthy(); + expect(saveSpy).not.toHaveBeenCalled(); + + // "Go back and fix" dismisses the gate back to the editor, issues listed. + await user.click(screen.getByRole("button", { name: /go back and fix/i })); + await waitFor(() => + expect(screen.queryByRole("dialog", { name: /validation issues/i })).toBeNull(), + ); + expect(saveSpy).not.toHaveBeenCalled(); + expect(screen.getByTestId("editor-issues").textContent).toContain( + "unknown key 'foo' at line 3", + ); + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-danger"); + expect(banner.textContent).toMatch(/cancel/i); + + // Saving again re-validates and re-opens the gate. + await user.click(screen.getByRole("button", { name: /^save$/i })); + await waitFor(() => expect(validateSpy).toHaveBeenCalledTimes(2)); + expect(await screen.findByRole("dialog", { name: /validation issues/i })).toBeTruthy(); + expect(saveSpy).not.toHaveBeenCalled(); + }); + + it("test_force_save_proceeds", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ + valid: false, + issues: [{ severity: "error", message: "unknown key 'foo' at line 3" }], + }); + saveSpy.mockResolvedValue(makeAppConfig()); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + const edited = await editDoc("# edited\n"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + await screen.findByRole("dialog", { name: /validation issues/i }); + + // Force is the escape hatch: exactly one save call, with force=true. + await user.click(screen.getByRole("button", { name: /force save anyway/i })); + await waitFor(() => expect(saveSpy).toHaveBeenCalledTimes(1)); + expect(saveSpy).toHaveBeenCalledWith(edited, true); + expect(saveSpy.mock.calls[0]).toEqual([edited, true]); + + // Gate cleared, ok banner, dirty cleared, onApplied fired. + expect(screen.queryByRole("dialog", { name: /validation issues/i })).toBeNull(); + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-success"); + expect(banner.textContent).toMatch(/saved/i); + expect(screen.queryByRole("status")).toBeNull(); + expect(screen.queryByTestId("editor-issues")).toBeNull(); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + }); + + it("test_save_disabled_without_disk_path", async () => { + // Session trial: nothing on disk to overwrite — Save must not offer itself. + fetchContentSpy.mockResolvedValue( + makeContent({ filename: "session-trial", config_path: null, session_trial: true }), + ); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + saveSpy.mockResolvedValue(makeAppConfig()); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + await readyEditorView(); + expect(screen.queryByRole("button", { name: /^save$/i })).toBeNull(); + // Save As stays available — it is the way to anchor a trial onto disk. + expect(screen.getByRole("button", { name: /^save as$/i })).toBeTruthy(); + + // Ctrl/Cmd+S falls back to Apply for a session trial. + await user.keyboard("{Control>}s"); + await waitFor(() => + expect(loadContentSpy).toHaveBeenCalledWith("session-trial", SAMPLE_YAML), + ); + expect(saveSpy).not.toHaveBeenCalled(); + expect(saveAsSpy).not.toHaveBeenCalled(); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + }); + + it("test_save_double_click_dispatches_once", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + // Deferred save: the first request stays pending across both clicks. + let resolveSave!: (value: AppConfigData) => void; + saveSpy.mockReturnValue( + new Promise((resolve) => { + resolveSave = resolve; + }), + ); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + const saveButton = await screen.findByRole("button", { name: /^save$/i }); + await user.click(saveButton); + // Second click while the first save is in flight must be swallowed. + await user.click(saveButton); + await waitFor(() => expect(saveSpy).toHaveBeenCalledTimes(1)); + expect(validateSpy).toHaveBeenCalledTimes(1); + + // The in-flight save still completes exactly once. + resolveSave(makeAppConfig()); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + expect(saveSpy).toHaveBeenCalledTimes(1); + expect(screen.getByTestId("editor-status").textContent).toMatch(/saved/i); + + // The gate is released once the action settles — a follow-up works. + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + saveSpy.mockResolvedValue(makeAppConfig()); + await user.click(saveButton); + await waitFor(() => expect(saveSpy).toHaveBeenCalledTimes(2)); + expect(onApplied).toHaveBeenCalledTimes(2); + }); + + it("test_stale_save_resolution_dropped_after_new", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + templateSpy.mockResolvedValue({ content: TEMPLATE_YAML_LF, filename: "starter.profiles" }); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + // Deferred save: the save round-trip stays pending on purpose. + let resolveSave!: (value: AppConfigData) => void; + saveSpy.mockReturnValue( + new Promise((resolve) => { + resolveSave = resolve; + }), + ); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + await editDoc("# edited\n"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + await waitFor(() => expect(saveSpy).toHaveBeenCalledTimes(1)); + + // New + confirm dispatches and completes while the save is pending. + await user.click(screen.getByRole("button", { name: /^new$/i })); + await user.click(screen.getByRole("button", { name: /^replace$/i })); + await waitFor(() => { + const view = EditorView.findFromDOM(document.querySelector(".cm-editor") as HTMLElement); + expect(view?.state.doc.toString()).toBe(TEMPLATE_YAML_LF); + }); + + // The stale save resolution lands after New completed: it belongs to the + // discarded document and must not mark it saved or fire onApplied. + await act(async () => { + resolveSave(makeAppConfig()); + }); + expect(onApplied).not.toHaveBeenCalled(); + const view = EditorView.findFromDOM(document.querySelector(".cm-editor") as HTMLElement); + expect(view?.state.doc.toString()).toBe(TEMPLATE_YAML_LF); + const banner = screen.getByTestId("editor-status"); + expect(banner.textContent).toMatch(/template loaded/i); + expect(screen.queryByRole("status")).toBeNull(); + }); + + it("test_save_as_runs_gate_then_dialog_then_saves", async () => { + fetchContentSpy.mockResolvedValue(makeContent()); + validateSpy.mockResolvedValue({ + valid: false, + issues: [{ severity: "error", message: "unknown key 'foo' at line 3" }], + }); + saveAsSpy.mockResolvedValue(makeAppConfig()); + browseSpy.mockResolvedValue(makeBrowseResult()); + const user = userEvent.setup(); + const { onApplied } = renderModal(); + + const edited = await editDoc("# edited\n"); + await user.click(screen.getByRole("button", { name: /^save as$/i })); + + // The SAME compliance gate runs first — no dialog, no save call yet. + await waitFor(() => + expect(validateSpy).toHaveBeenCalledWith("machine.profiles", edited), + ); + expect(screen.queryByRole("dialog", { name: /save configuration as/i })).toBeNull(); + await user.click(await screen.findByRole("button", { name: /force save anyway/i })); + + // Then the Save As dialog opens, listing directories via browseDirectory. + const dialog = await screen.findByRole("dialog", { name: /save configuration as/i }); + await waitFor(() => expect(browseSpy).toHaveBeenCalled()); + expect(dialog.textContent).toContain("/home/user/configs"); + + // Confirming hands the normalized name to saveConfigFileAs, force carried. + await user.type(await screen.findByLabelText(/file name/i), "copy"); + await user.click(within(dialog).getByRole("button", { name: /^save$/i })); + await waitFor(() => expect(saveAsSpy).toHaveBeenCalledTimes(1)); + expect(saveAsSpy).toHaveBeenCalledWith({ + content: edited, + filename: "copy.profiles", + directory: "/home/user/configs", + force: true, + overwrite: false, + }); + + // Same success surface as Save: banner, clean editor, onApplied. + const banner = screen.getByTestId("editor-status"); + expect(banner.className).toContain("status-success"); + expect(banner.textContent).toMatch(/saved/i); + expect(screen.queryByRole("status")).toBeNull(); + await waitFor(() => expect(onApplied).toHaveBeenCalledTimes(1)); + expect(saveSpy).not.toHaveBeenCalled(); + }); }); diff --git a/src/profiles/web/src/components/ConfigEditorModal.tsx b/src/profiles/web/src/components/ConfigEditorModal.tsx index 0fe0642..a898361 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.tsx @@ -9,24 +9,30 @@ import { FlaskConical, Play, RotateCcw, + Save, + SaveAll, X, } from "lucide-react"; import { + ApiError, fetchConfigContent, fetchConfigTemplate, loadConfigContent, openConfigFile, + saveConfigFile, + saveConfigFileAs, validateConfigContent, } from "../api/client"; import type { AppConfigData, ConfigContentData, ValidationIssue } from "../api/types"; import { ConfirmDialog } from "./ConfirmDialog"; +import { SaveAsDialog } from "./SaveAsDialog"; import { useModalA11y } from "../utils/useModalA11y"; export interface ConfigEditorModalProps { open: boolean; onClose: () => void; - /** Called after Apply succeeds (Save/Save As fire it too — Task 8). */ + /** Called after Apply/Save/Save As succeed, with the fresh AppConfigData. */ onApplied?: (data: AppConfigData) => void; } @@ -43,7 +49,20 @@ type EditorStatus = { kind: "ok" | "error"; text: string } | null; type PendingAction = "new" | "revert"; /** Kinds of the async toolbar actions — the dispatch runner's key. */ -type ToolbarActionKind = "new" | "test" | "revert" | "apply"; +type ToolbarActionKind = "new" | "test" | "revert" | "apply" | "save" | "save-as"; + +/** Target chosen in the Save As dialog, handed back to the save-as flow. */ +type SaveAsTarget = { filename: string; directory: string; overwrite: boolean }; + +/** + * Compliance gate parked on an in-flight save action: the validation + * issues block the write until the user either dismisses the gate + * (resolve(false)) or force-saves (resolve(true)). + */ +type SaveGate = { issues: ValidationIssue[]; resolve: (force: boolean) => void }; + +/** Save As dialog parked on an in-flight save-as action. */ +type SaveAsPrompt = { resolve: (target: SaveAsTarget | null) => void }; /** * Editor theme built from the app's `data-theme` CSS tokens (index.css @@ -105,25 +124,46 @@ function isDirty(view: EditorView | null, loadedText: string): boolean { return view !== null && view.state.doc.toString() !== loadedText; } +/** + * Describe a failed save/save-as for the banner. A 400 whose detail is a + * validation issue list (the compliance gate contract) renders its issues + * as data; every other failure — session-trial refusal, hygiene or + * extension error, 409 existing target, network error — is plain text. + */ +function describeSaveError(err: unknown): { text: string; issues: ValidationIssue[] } { + if (err instanceof ApiError && Array.isArray(err.detail)) { + return { text: err.message, issues: err.detail }; + } + return { text: err instanceof Error ? err.message : String(err), issues: [] }; +} + /** * Full-screen YAML configuration editor (issue #95). This shell owns the * CodeMirror 6 instance, load-on-open, dirty tracking and the guarded * Escape/✕ close. The toolbar carries the content actions — New (starter - * template), Test (in-memory validation), Revert (reload from disk) and - * Apply (session-trial the current text); Save/Save-as arrive with Task 8. - * The header bar carries the "Open in OS" shortcut, which hands the file - * to the OS default editor via the existing client call. + * template), Test (in-memory validation), Revert (reload from disk), + * Apply (session-trial the current text), Save (write the disk-anchored + * file) and Save As (write a new file, which also anchors a session + * trial). Save and Save As run the compliance gate first: the text is + * validated client-side, and issues block the write behind a + * "Go back and fix / Force save anyway" prompt. The header bar carries + * the "Open in OS" shortcut, which hands the file to the OS default + * editor via the existing client call. */ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModalProps) { const [load, setLoad] = useState({ status: "loading" }); const [dirty, setDirty] = useState(false); const [confirmDiscardOpen, setConfirmDiscardOpen] = useState(false); - /** Banner above the editor — Test/Apply/New/Revert outcomes. */ + /** Banner above the editor — toolbar action outcomes. */ const [status, setStatus] = useState(null); - /** Validation issues from the Test action, listed under the banner. */ + /** Validation issues from the Test action and the save gate, listed under the banner. */ const [issues, setIssues] = useState([]); /** Action awaiting its "Replace current edits?" confirmation. */ const [pendingAction, setPendingAction] = useState(null); + /** Compliance gate parked on an in-flight save/save-as action. */ + const [gate, setGate] = useState(null); + /** Save As dialog parked on an in-flight save-as action. */ + const [saveAsPrompt, setSaveAsPrompt] = useState(null); const dialogRef = useRef(null); const editorHostRef = useRef(null); @@ -158,6 +198,8 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa setStatus(null); setIssues([]); setPendingAction(null); + setGate(null); + setSaveAsPrompt(null); // Invalidate in-flight toolbar actions so a resolution that lands // after close/reopen cannot leak into the next session's state. actionSeqRef.current += 1; @@ -387,11 +429,208 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa [load, onApplied, runToolbarAction], ); + /** + * Park the compliance gate on the in-flight action: the gate's buttons + * resolve the promise the save flow is awaiting. Clearing the gate state + * happens inside the resolver so either exit closes it immediately, + * even when the action turns out superseded (stale-seq check at the + * call site drops the write but still closes the dialog). + */ + const awaitGateDecision = useCallback( + (issues: ValidationIssue[]) => + new Promise((resolve) => { + setGate({ + issues, + resolve: (force: boolean) => { + setGate(null); + resolve(force); + }, + }); + }), + [], + ); + + /** Park the Save As dialog on the in-flight save-as action (null = cancelled). */ + const awaitSaveAsTarget = useCallback( + () => + new Promise((resolve) => { + setSaveAsPrompt({ + resolve: (target: SaveAsTarget | null) => { + setSaveAsPrompt(null); + resolve(target); + }, + }); + }), + [], + ); + + /** + * Shared success surface for Save and Save As: the saved text becomes + * the new dirty baseline, the response's fresh AppConfigData refreshes + * the header (a Save As re-anchors the file, re-enabling Save) and + * drives the same `onApplied` path as Apply. + */ + const markSaved = useCallback( + (data: AppConfigData, savedText: string) => { + loadedTextRef.current = savedText; + setDirty(false); + setIssues([]); + setGate(null); + setSaveAsPrompt(null); + if (data.config_path) { + const name = + data.config_path.split(/[\\/]/).pop() || data.title || "configuration"; + setLoad((prev): LoadState => { + if (prev.status !== "ready") return prev; + // Only a changed anchor (Save As re-target, session trial now + // anchored) refreshes `load` — and with it the CodeMirror view, + // which remounts from `data.content`, so the saved text must + // ride along. An unchanged anchor (plain Save) must NOT bump the + // load object: the remount would rebuild the view from the stale + // pre-edit `content` and silently discard the user's edits. + const anchorChanged = + prev.data.filename !== name || + prev.data.config_path !== data.config_path || + prev.data.session_trial; + if (!anchorChanged) return prev; + return { + status: "ready", + data: { + ...prev.data, + filename: name, + config_path: data.config_path, + session_trial: false, + content: savedText, + }, + }; + }); + } + onApplied?.(data); + }, + [onApplied], + ); + + /** Shared error surface for Save and Save As (validation lists render as data). */ + const reportSaveError = useCallback((err: unknown) => { + const described = describeSaveError(err); + setGate(null); + setSaveAsPrompt(null); + setIssues(described.issues); + setStatus({ kind: "error", text: described.text }); + }, []); + + /** + * Validate → decide → write flow shared by Save and Save As. The + * client-side validation runs first so the gate can offer "force" + * before anything is written; the decision (and, for Save As, the + * dialog) parks on the in-flight action, so duplicate clicks are + * swallowed and a superseding action (close, New) invalidates the + * parked flow via the stale-seq check. + */ + const runSaveFlow = useCallback( + (kind: Extract, filename: string) => + runToolbarAction(kind, async (seq) => { + if (load.status !== "ready") return; + const text = currentDocText(); + setStatus(null); + setIssues([]); + try { + const result = await validateConfigContent(filename, text); + if (seq !== actionSeqRef.current) return; // superseded — drop stale results + let force = false; + if (!result.valid) { + setIssues(result.issues); + force = await awaitGateDecision(result.issues); + if (seq !== actionSeqRef.current) return; // superseded while parked + if (!force) { + setStatus({ + kind: "error", + text: "Save cancelled — fix the listed issues, or force-save anyway.", + }); + return; + } + } + if (kind === "save-as") { + const target = await awaitSaveAsTarget(); + if (seq !== actionSeqRef.current) return; // superseded while parked + if (target === null) return; // cancelled — no save call + const data = await saveConfigFileAs({ + content: text, + filename: target.filename, + directory: target.directory, + force, + overwrite: target.overwrite, + }); + if (seq !== actionSeqRef.current) return; + markSaved(data, text); + setStatus({ kind: "ok", text: `Configuration saved as ${target.filename}.` }); + } else { + const data = await saveConfigFile(text, force); + if (seq !== actionSeqRef.current) return; + markSaved(data, text); + setStatus({ kind: "ok", text: `Configuration saved to ${filename}.` }); + } + } catch (err: unknown) { + if (seq !== actionSeqRef.current) return; + reportSaveError(err); + } + }), + [awaitGateDecision, awaitSaveAsTarget, load, markSaved, reportSaveError, runToolbarAction], + ); + + const runSave = useCallback( + () => runSaveFlow("save", load.status === "ready" ? load.data.filename : ""), + [load, runSaveFlow], + ); + + const runSaveAs = useCallback( + () => runSaveFlow("save-as", load.status === "ready" ? load.data.filename : ""), + [load, runSaveFlow], + ); + + /** + * Ctrl/Cmd+S: Save on a disk-anchored config, Apply fallback for a + * session trial (nothing to write to). Capture-phase window listener so + * it wins over CodeMirror and the browser's own save dialog; the + * latest-ref indirection keeps the listener registered once while + * always dispatching through current state. Parked dialogs (gate, Save + * As, discard warnings) swallow the shortcut. + */ + const modSHandlerRef = useRef<() => void>(() => {}); + modSHandlerRef.current = () => { + if (load.status !== "ready") return; + if (gate !== null || saveAsPrompt !== null || confirmDiscardOpen || pendingAction !== null) { + return; + } + if (load.data.config_path !== null) void runSave(); + else void applyContent(); + }; + useEffect(() => { + if (!open) return; + const onKeyDown = (event: KeyboardEvent) => { + if ( + (event.metaKey || event.ctrlKey) && + !event.altKey && + !event.shiftKey && + event.key.toLowerCase() === "s" + ) { + event.preventDefault(); + event.stopPropagation(); + modSHandlerRef.current(); + } + }; + window.addEventListener("keydown", onKeyDown, true); + return () => window.removeEventListener("keydown", onKeyDown, true); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open]); + if (!open) return null; const filename = load.status === "ready" ? load.data.filename : ""; const sessionTrial = load.status === "ready" ? load.data.session_trial : false; const toolbarDisabled = load.status !== "ready"; + /** Save only exists when the text is anchored to a file on disk. */ + const diskAnchored = load.status === "ready" && load.data.config_path !== null; return ( <> @@ -445,7 +684,7 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa
- {/* Toolbar — content actions; Save/Save-as arrive with Task 8. */} + {/* Toolbar — content actions and persistence. */}
Revert + {/* Hidden for a session trial (and after New): nothing on disk + to overwrite — Save As is the way to anchor those. */} + {diskAnchored && ( + + )} + + + + + {browseData?.parent_path && ( + + + + )} +
+ + + {/* Directory listing */} +
+ {loading ? ( +
+ + Loading directories… +
+ ) : error ? ( +
+

{error}

+ +
+ ) : ( +
    + {directories.map((item) => ( +
  • + +
  • + ))} + {directories.length === 0 && ( +
  • + + No subdirectories found +
  • + )} +
+ )} +
+ + {/* Filename + actions */} +
+ + { + setFilename(e.target.value); + setFilenameError(null); + }} + onKeyDown={(e) => { + if (e.key === "Enter" && filename.trim()) handleConfirm(); + }} + placeholder="machine.profiles" + className="h-9 w-full rounded-lg border border-line bg-elevated px-2.5 font-mono text-sm text-ink outline-none placeholder:text-ink-faint transition-colors hover:bg-surface-strong focus-visible:border-indigo-500 focus-visible:ring-2 focus-visible:ring-indigo-500/30" + /> + {filenameError && ( +

+ {filenameError} +

+ )} +
+ + +
+
+ + + + {/* Replace prompt — only when the typed name matches a listed file. */} + { + const target = pendingOverwrite; + setPendingOverwrite(null); + if (target) onConfirm({ ...target, overwrite: true }); + }} + onCancel={() => setPendingOverwrite(null)} + /> + + ); +} From b2bbb6a07de64741864e76d098b73493c280d2de Mon Sep 17 00:00:00 2001 From: falbany Date: Wed, 4 Nov 2026 01:02:21 +0100 Subject: [PATCH 15/19] fix(web): align save-as whitelist parity and scope replace confirmation to the browsed directory --- .../web/src/components/SaveAsDialog.test.tsx | 71 +++++++++++++++++++ .../web/src/components/SaveAsDialog.tsx | 30 ++++++-- 2 files changed, 94 insertions(+), 7 deletions(-) diff --git a/src/profiles/web/src/components/SaveAsDialog.test.tsx b/src/profiles/web/src/components/SaveAsDialog.test.tsx index 16530c1..ad148cc 100644 --- a/src/profiles/web/src/components/SaveAsDialog.test.tsx +++ b/src/profiles/web/src/components/SaveAsDialog.test.tsx @@ -108,6 +108,77 @@ describe("SaveAsDialog", () => { await user.click(screen.getByRole("button", { name: /^save$/i })); await waitFor(() => expect(onConfirm).toHaveBeenCalledTimes(2)); expect(onConfirm.mock.calls[1][0].filename).toBe("backup.profiles.yaml"); + + // The server whitelist is case-insensitive: an uppercase variant is + // accepted and sent verbatim (the server handles its own casing). + await user.clear(input); + await user.type(input, "BACKUP.PROFILES"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + await waitFor(() => expect(onConfirm).toHaveBeenCalledTimes(3)); + expect(onConfirm.mock.calls[2][0].filename).toBe("BACKUP.PROFILES"); + + // An empty stem (bare ".profiles") is rejected client-side… + await user.clear(input); + await user.type(input, ".profiles"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + await screen.findByRole("alert"); + expect(onConfirm).toHaveBeenCalledTimes(3); + + // …as is a stray space before the extension. + await user.clear(input); + await user.type(input, "notes .profiles"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + await screen.findByRole("alert"); + expect(onConfirm).toHaveBeenCalledTimes(3); + }); + + it("test_replace_prompt_only_for_browsed_directory", async () => { + browseSpy.mockResolvedValue(makeBrowse()); + const user = userEvent.setup(); + const { onConfirm } = renderDialog(); + await screen.findByRole("dialog", { name: /save configuration as/i }); + await screen.findByRole("button", { name: /machines/i }); + + // Type a Location without navigating to it: its contents are unknown, + // so the overwrite check must not lean on the browsed listing. + await user.clear(screen.getByLabelText(/location/i)); + await user.type(screen.getByLabelText(/location/i), "/elsewhere"); + await user.type(screen.getByLabelText(/file name/i), "other.profiles"); + await user.click(screen.getByRole("button", { name: /^save$/i })); + + // No Replace prompt — and overwrite:false, leaving the server's 409 + // guard to answer for the unbrowsed directory. + expect(screen.queryByRole("dialog", { name: /replace configuration\?/i })).toBeNull(); + await waitFor(() => expect(onConfirm).toHaveBeenCalledTimes(1)); + expect(onConfirm.mock.calls[0][0]).toEqual({ + filename: "other.profiles", + directory: "/elsewhere", + overwrite: false, + }); + }); + + it("test_save_disabled_while_browse_unavailable", async () => { + browseSpy.mockRejectedValue(new Error("Navigation failed")); + const user = userEvent.setup(); + const { onConfirm } = renderDialog(); + + // Browse failure surfaces as an error state with a retry… + const alert = await screen.findByRole("alert"); + expect(alert.textContent).toMatch(/navigation failed/i); + // …and Save stays disabled even with a well-formed filename, since + // there is no listing to name a target directory from. + await user.type(screen.getByLabelText(/file name/i), "machine.profiles"); + expect(screen.getByRole("button", { name: /^save$/i })).toBeDisabled(); + + // A successful retry restores the listing and re-enables Save. + browseSpy.mockResolvedValue(makeBrowse()); + await user.click(screen.getByRole("button", { name: /retry/i })); + await screen.findByRole("button", { name: /machines/i }); + expect(screen.getByRole("button", { name: /^save$/i })).toBeEnabled(); + + await user.click(screen.getByRole("button", { name: /^save$/i })); + await waitFor(() => expect(onConfirm).toHaveBeenCalledTimes(1)); + expect(onConfirm.mock.calls[0][0].directory).toBe("/home/user/configs"); }); it("test_overwrite_confirmation", async () => { diff --git a/src/profiles/web/src/components/SaveAsDialog.tsx b/src/profiles/web/src/components/SaveAsDialog.tsx index 4e255d1..efc9bdb 100644 --- a/src/profiles/web/src/components/SaveAsDialog.tsx +++ b/src/profiles/web/src/components/SaveAsDialog.tsx @@ -18,15 +18,24 @@ const SAVE_AS_EXTENSIONS = [".profiles", ".profiles.yaml"]; /** * Normalize the user-typed filename against the server's extension - * whitelist: a bare name gets `.profiles` appended; a name already ending - * in a whitelisted extension is kept verbatim; anything else is rejected. - * Returns `null` when the name cannot be saved as-is. + * whitelist, which is case-insensitive (`name.lower().endswith(...)` in + * `routes/config.py`) — mirrored here so e.g. `BACKUP.PROFILES` is + * accepted and sent verbatim (the server handles its own casing). A bare + * name gets `.profiles` appended; an empty stem (`".profiles"`) or a + * stray space before the extension is rejected; anything else is + * rejected. Returns `null` when the name cannot be saved as-is. */ function normalizeSaveAsFilename(raw: string): string | null { const name = raw.trim(); if (!name) return null; - if (SAVE_AS_EXTENSIONS.some((ext) => name.endsWith(ext))) return name; - if (!name.includes(".")) return `${name}.profiles`; + const lowered = name.toLowerCase(); + const matched = SAVE_AS_EXTENSIONS.find((ext) => lowered.endsWith(ext)); + if (matched !== undefined) { + const stem = name.slice(0, name.length - matched.length); + if (!stem.trim() || stem !== stem.trimEnd()) return null; + return name; + } + if (!lowered.includes(".")) return `${name}.profiles`; return null; } @@ -101,7 +110,14 @@ export function SaveAsDialog({ open, onCancel, onConfirm }: SaveAsDialogProps) { } setFilenameError(null); const directory = pathInput.trim() || browseData.current_path; - const exists = browseData.items.some((item) => !item.is_dir && item.name === normalized); + // The overwrite check is only sound for the directory whose listing is + // on screen: a typed-but-not-browsed Location has unknown contents, so + // confirming against it could consent to replacing a file that was + // never listed. Such targets go through with overwrite:false and the + // server's 409 guard answers instead. + const browsedHere = pathInput.trim() === browseData.current_path; + const exists = + browsedHere && browseData.items.some((item) => !item.is_dir && item.name === normalized); if (exists) { setPendingOverwrite({ filename: normalized, directory }); return; @@ -282,7 +298,7 @@ export function SaveAsDialog({ open, onCancel, onConfirm }: SaveAsDialogProps) { + {/* Only meaningful for a disk-anchored config: on a session + trial the open-config action would hand the OS the + CWD-resolved fallback — possibly an unrelated pre-existing + .profiles file or a NOT_FOUND — so the shortcut hides, the + same way Save does. */} + {diskAnchored && ( + + )}