feat(ui): add KeyValueEditor component for managing key-value pairs

openspec/changes/settings-page-redesign/tasks.md → openspec/changes/archive/2026-07-06-settings-page-redesign/tasks.md

@@ -2,22 +2,22 @@
 
 ## 1. Foundations
 
-- [ ] 1.1 Add the shadcn-svelte `field` component (`npx shadcn-svelte@latest add field`)
-- [ ] 1.2 Extract the string-map editor from `SchemaForm.svelte` into a shared `KeyValueEditor` component (entries state, `objectToEntries`/`entriesToObject`, blank-entry validation, optional read-only inherited rows); wire `SchemaForm` to delegate to it
-- [ ] 1.3 Verify pack parameters dialog behavior is unchanged after the extraction (add/remove/validate/save `default_tags`)
-- [ ] 1.4 Align the frontend `AppConfig` TS type with the Go struct (all 8 keys + `default_tags`)
+- [x] 1.1 Add the shadcn-svelte `field` component (`npx shadcn-svelte@latest add field`)
+- [x] 1.2 Extract the string-map editor from `SchemaForm.svelte` into a shared `KeyValueEditor` component (entries state, `objectToEntries`/`entriesToObject`, blank-entry validation, optional read-only inherited rows); wire `SchemaForm` to delegate to it
+- [x] 1.3 Verify pack parameters dialog behavior is unchanged after the extraction (add/remove/validate/save `default_tags`)
+- [x] 1.4 Align the frontend `AppConfig` TS type with the Go struct (all 8 keys + `default_tags`)
 
 ## 2. Settings page rewrite
 
-- [ ] 2.1 Rebuild `src/routes/config/+page.svelte` with four tabs (General default, Default tags, Retention, About) using `Tabs` + `Field.FieldGroup`/`Field.Field`; delete the generic KV loop and sensitive-key masking
-- [ ] 2.2 General tab: parallelism (number), terraform version (text), pack logs (switch), SSH session logging (switch); human labels + descriptions
-- [ ] 2.3 Default tags tab: `KeyValueEditor` for `default_tags` with precedence description; blank entries rejected client-side
-- [ ] 2.4 Retention tab: enable switch + days input pairs; days disabled while switch is off
-- [ ] 2.5 About tab: version, commit, build date, Go version (read-only)
-- [ ] 2.6 Save behavior: switches persist on toggle; per-tab Save writes only dirty keys; failed saves show an Alert and keep edits
+- [x] 2.1 Rebuild `src/routes/config/+page.svelte` with four tabs (General default, Default tags, Retention, About) using `Tabs` + `Field.FieldGroup`/`Field.Field`; delete the generic KV loop and sensitive-key masking
+- [x] 2.2 General tab: parallelism (number), terraform version (text), pack logs (switch), SSH session logging (switch); human labels + descriptions
+- [x] 2.3 Default tags tab: `KeyValueEditor` for `default_tags` with precedence description; blank entries rejected client-side
+- [x] 2.4 Retention tab: enable switch + days input pairs; days disabled while switch is off
+- [x] 2.5 About tab: version, commit, build date, Go version (read-only)
+- [x] 2.6 Save behavior: switches persist on toggle; per-tab Save writes only dirty keys; failed saves show an Alert and keep edits
 
 ## 3. Verification
 
-- [ ] 3.1 `mise run build-frontend` and svelte-check pass
-- [ ] 3.2 Manual flow: edit every setting type (switch, number, text, tags map), reload, confirm persistence; verify save-failure path keeps edits (e.g. invalid retention days rejected by server validation)
-- [ ] 3.3 Verify pack parameters dialog still edits `default_tags` correctly through the shared editor
+- [x] 3.1 `mise run build-frontend` and svelte-check pass
+- [x] 3.2 Manual flow: edit every setting type (switch, number, text, tags map), reload, confirm persistence; verify save-failure path keeps edits (e.g. invalid retention days rejected by server validation)
+- [x] 3.3 Verify pack parameters dialog still edits `default_tags` correctly through the shared editor

openspec/specs/settings-page/spec.md

@@ -0,0 +1,96 @@
+# Settings Page Specification
+
+## Purpose
+Defines the Settings page UI: how app-level admin settings from
+`GET/PUT /api/config` are organized into tabs, rendered as typed
+controls with human-readable labels, and saved. Also covers the
+Default tags editing surface and the shared string-map editor
+component reused by the pack parameters dialog. Backend storage and
+validation of `default_tags` is specified in `app-settings`.
+
+## Requirements
+
+### Requirement: Tabbed Settings Layout
+The Settings page SHALL organize settings into four tabs — General,
+Default tags, Retention, and About — with General as the default tab.
+General SHALL contain parallelism, terraform version, pack logs, and
+SSH session logging. Retention SHALL contain run log retention and run
+retention. About SHALL contain read-only version information (version,
+commit, build date, Go version).
+
+#### Scenario: Landing on General
+- **WHEN** a user navigates to the Settings page
+- **THEN** the General tab is active, showing parallelism, terraform version, pack logs, and SSH session logging controls
+
+#### Scenario: About is read-only
+- **WHEN** a user opens the About tab
+- **THEN** version, commit, build date, and Go version are displayed with no editable controls
+
+### Requirement: Typed Controls with Human Labels
+Each known setting SHALL render with a human-readable label and
+description and a control matching its type: switches for booleans,
+number inputs for integer settings, text inputs for strings. Raw
+`app_config` keys SHALL NOT be used as labels. The system SHALL NOT
+render settings through a generic key/value loop and SHALL NOT apply
+sensitive-key masking.
+
+#### Scenario: Boolean is a switch
+- **WHEN** the General tab renders the pack logs setting (`pack_logs_enabled`)
+- **THEN** it is a switch with a human label, not a text input containing "true"
+
+#### Scenario: Unknown keys not rendered
+- **WHEN** `app_config` contains a key not known to the frontend
+- **THEN** the Settings page does not render a control for it
+
+### Requirement: Save Behavior
+Switch settings SHALL persist immediately on toggle via
+`PUT /api/config`. Tabs containing text or number inputs SHALL provide
+a single Save action that persists only the dirty keys of that tab.
+Failed saves SHALL surface an error without discarding the user's
+edits.
+
+#### Scenario: Toggle saves immediately
+- **WHEN** a user toggles SSH session logging
+- **THEN** the value is persisted via `PUT /api/config` without a separate Save action
+
+#### Scenario: Per-tab save of dirty keys
+- **WHEN** a user edits parallelism on the General tab and clicks Save
+- **THEN** only the changed key is written; unchanged keys are not re-persisted
+
+#### Scenario: Save failure keeps edits
+- **WHEN** a save request fails
+- **THEN** an error is shown and the edited values remain in the form
+
+### Requirement: Retention Enable/Days Pairing
+Each retention setting SHALL render as an enable switch paired with a
+days input, and the days input SHALL be disabled while its switch is
+off.
+
+#### Scenario: Days disabled when retention off
+- **WHEN** run retention is toggled off
+- **THEN** the run retention days input is disabled
+
+### Requirement: Default Tags Tab
+The Default tags tab SHALL edit the org-wide `default_tags` setting
+with a key/value editor and SHALL describe where the tags apply and
+that packs can override individual tags. Blank keys or values SHALL be
+rejected before saving.
+
+#### Scenario: Editing org tags
+- **WHEN** a user adds `owner: secops` in the Default tags tab and saves
+- **THEN** `PUT /api/config` persists `default_tags` as `{"owner": "secops"}`
+
+#### Scenario: Blank entry rejected client-side
+- **WHEN** a user adds an entry with an empty key and saves
+- **THEN** a validation message is shown and no request is sent
+
+### Requirement: Shared String-Map Editor
+The key/value map editor SHALL be a single shared component used by
+both the pack parameters dialog (via `SchemaForm`) and the Default tags
+tab, preserving the existing editor behavior (add/remove entries,
+blank-entry validation) and supporting read-only inherited rows for the
+pack dialog's inheritance display.
+
+#### Scenario: One editor, two surfaces
+- **WHEN** a user edits `default_tags` in the pack parameters dialog and in the Settings Default tags tab
+- **THEN** both use the same key/value editor component with identical add/remove/validation behavior

web/frontend/src/lib/components/KeyValueEditor.svelte

@@ -0,0 +1,117 @@
+<script module lang="ts">
+	export type KeyValueEntry = { key: string; value: string };
+
+	export function objectToEntries(v: unknown): KeyValueEntry[] {
+		if (!v || typeof v !== 'object') return [];
+		return Object.entries(v as Record<string, unknown>).map(([key, value]) => ({
+			key,
+			value: typeof value === 'string' ? value : JSON.stringify(value)
+		}));
+	}
+
+	export function entriesToObject(entries: KeyValueEntry[]): Record<string, string> {
+		const out: Record<string, string> = {};
+		for (const e of entries) {
+			const k = e.key.trim();
+			if (!k) continue;
+			out[k] = e.value;
+		}
+		return out;
+	}
+
+	// Blank keys/values are invalid — Terraform/cloud providers reject empty tag
+	// keys or values, so callers check this before saving.
+	export function hasBlankEntry(map: Record<string, unknown>): boolean {
+		return Object.entries(map).some(
+			([k, v]) => k.trim() === '' || typeof v !== 'string' || v.trim() === ''
+		);
+	}
+</script>
+
+<script lang="ts">
+	import { Input } from '$lib/components/ui/input/index.js';
+	import { Button } from '$lib/components/ui/button/index.js';
+	import PlusIcon from '@lucide/svelte/icons/plus';
+	import XIcon from '@lucide/svelte/icons/x';
+
+	let {
+		value,
+		inherited = {},
+		inheritedLabel = 'Inherited from Settings',
+		onchange
+	}: {
+		value: unknown;
+		inherited?: Record<string, string>;
+		inheritedLabel?: string;
+		onchange: (next: Record<string, string>) => void;
+	} = $props();
+
+	let entries = $derived(objectToEntries(value));
+	let inheritedEntries = $derived(Object.entries(inherited));
+</script>
+
+<div class="space-y-2">
+	{#if inheritedEntries.length > 0}
+		{@const packKeys = new Set(entries.map((e) => e.key.trim()))}
+		<div class="space-y-1">
+			{#each inheritedEntries as [k, v] (k)}
+				{@const overridden = packKeys.has(k)}
+				<div
+					class="grid grid-cols-[1fr_1fr_auto] items-center gap-2 px-3 text-xs text-muted-foreground"
+				>
+					<span class="font-mono truncate {overridden ? 'line-through opacity-60' : ''}">
+						{k}
+					</span>
+					<span class="font-mono truncate {overridden ? 'line-through opacity-60' : ''}">
+						{v}
+					</span>
+					<span class="text-[10px]">{overridden ? 'overridden' : ''}</span>
+				</div>
+			{/each}
+			<p class="px-3 text-[10px] text-muted-foreground/70">{inheritedLabel}</p>
+		</div>
+	{/if}
+	{#each entries as entry, i}
+		<div class="grid grid-cols-[1fr_1fr_auto] gap-2">
+			<Input
+				placeholder="key"
+				value={entry.key}
+				oninput={(e) => {
+					const next = entries.slice();
+					next[i] = { ...next[i], key: (e.target as HTMLInputElement).value };
+					onchange(entriesToObject(next));
+				}}
+			/>
+			<Input
+				placeholder="value"
+				value={entry.value}
+				oninput={(e) => {
+					const next = entries.slice();
+					next[i] = { ...next[i], value: (e.target as HTMLInputElement).value };
+					onchange(entriesToObject(next));
+				}}
+			/>
+			<Button
+				variant="outline"
+				size="sm"
+				class="h-9 w-8 p-0"
+				onclick={() => {
+					const next = entries.filter((_, idx) => idx !== i);
+					onchange(entriesToObject(next));
+				}}
+			>
+				<XIcon size={14} />
+			</Button>
+		</div>
+	{/each}
+	<Button
+		variant="outline"
+		size="sm"
+		onclick={() => {
+			onchange({ ...entriesToObject(entries), '': '' });
+		}}
+	>
+		<PlusIcon data-icon="inline-start" />
+		Add Entry
+	</Button>
+</div>

web/frontend/src/lib/components/PackParametersDialog.svelte

@@ -10,6 +10,7 @@
 		ValidationError
 	} from '$lib/api/client';
 	import SchemaForm from '$lib/components/SchemaForm.svelte';
+	import { hasBlankEntry } from '$lib/components/KeyValueEditor.svelte';
 	import type { AppConfig, PackManifest } from '$lib/types';
 
 	let {
@@ -86,11 +87,8 @@
 			if (prop?.type !== 'object' || prop.additionalProperties?.type !== 'string') continue;
 			const map = values[name];
 			if (!map || typeof map !== 'object') continue;
-			for (const [k, v] of Object.entries(map as Record<string, unknown>)) {
-				if (k.trim() === '' || typeof v !== 'string' || v.trim() === '') {
-					errs[name] = 'Keys and values cannot be empty — remove or fill blank entries.';
-					break;
-				}
+			if (hasBlankEntry(map as Record<string, unknown>)) {
+				errs[name] = 'Keys and values cannot be empty — remove or fill blank entries.';
 			}
 		}
 		return errs;

web/frontend/src/lib/components/SchemaForm.svelte

@@ -7,9 +7,9 @@
 	import { Input } from '$lib/components/ui/input/index.js';
 	import { Label } from '$lib/components/ui/label/index.js';
 	import { Switch } from '$lib/components/ui/switch/index.js';
+	import KeyValueEditor from '$lib/components/KeyValueEditor.svelte';
 	import ChevronRightIcon from '@lucide/svelte/icons/chevron-right';
 	import InfoIcon from '@lucide/svelte/icons/info';
-	import PlusIcon from '@lucide/svelte/icons/plus';
 	import XIcon from '@lucide/svelte/icons/x';
 	import AlertTriangleIcon from '@lucide/svelte/icons/alert-triangle';
 
@@ -92,28 +92,6 @@
 	function isRequired(name: string): boolean {
 		return required.includes(name);
 	}
-
-	// ----- String→string map editor state helpers ----------------------
-
-	type MapEntry = { key: string; value: string };
-
-	function objectToEntries(v: unknown): MapEntry[] {
-		if (!v || typeof v !== 'object') return [];
-		return Object.entries(v as Record<string, unknown>).map(([key, value]) => ({
-			key,
-			value: typeof value === 'string' ? value : JSON.stringify(value)
-		}));
-	}
-
-	function entriesToObject(entries: MapEntry[]): Record<string, string> {
-		const out: Record<string, string> = {};
-		for (const e of entries) {
-			const k = e.key.trim();
-			if (!k) continue;
-			out[k] = e.value;
-		}
-		return out;
-	}
 </script>
 
 {#snippet field(name: string, prop: Property)}
@@ -158,73 +136,11 @@
 				</Select.Content>
 			</Select.Root>
 		{:else if prop.type === 'object' && prop.additionalProperties?.type === 'string'}
-			{@const entries = objectToEntries(value)}
-			{@const inherited = name === 'default_tags' ? Object.entries(inheritedDefaultTags) : []}
-			<div class="space-y-2">
-				{#if inherited.length > 0}
-					{@const packKeys = new Set(entries.map((e) => e.key.trim()))}
-					<div class="space-y-1">
-						{#each inherited as [k, v] (k)}
-							{@const overridden = packKeys.has(k)}
-							<div
-								class="grid grid-cols-[1fr_1fr_auto] items-center gap-2 px-3 text-xs text-muted-foreground"
-							>
-								<span class="font-mono truncate {overridden ? 'line-through opacity-60' : ''}">
-									{k}
-								</span>
-								<span class="font-mono truncate {overridden ? 'line-through opacity-60' : ''}">
-									{v}
-								</span>
-								<span class="text-[10px]">{overridden ? 'overridden' : ''}</span>
-							</div>
-						{/each}
-						<p class="px-3 text-[10px] text-muted-foreground/70">Inherited from Settings</p>
-					</div>
-				{/if}
-				{#each entries as entry, i}
-					<div class="grid grid-cols-[1fr_1fr_auto] gap-2">
-						<Input
-							placeholder="key"
-							value={entry.key}
-							oninput={(e) => {
-								const next = entries.slice();
-								next[i] = { ...next[i], key: (e.target as HTMLInputElement).value };
-								update(name, entriesToObject(next));
-							}}
-						/>
-						<Input
-							placeholder="value"
-							value={entry.value}
-							oninput={(e) => {
-								const next = entries.slice();
-								next[i] = { ...next[i], value: (e.target as HTMLInputElement).value };
-								update(name, entriesToObject(next));
-							}}
-						/>
-						<Button
-							variant="outline"
-							size="sm"
-							class="h-9 w-8 p-0"
-							onclick={() => {
-								const next = entries.filter((_, idx) => idx !== i);
-								update(name, entriesToObject(next));
-							}}
-						>
-							<XIcon size={14} />
-						</Button>
-					</div>
-				{/each}
-				<Button
-					variant="outline"
-					size="sm"
-					onclick={() => {
-						update(name, { ...entriesToObject(entries), '': '' });
-					}}
-				>
-					<PlusIcon data-icon="inline-start" />
-					Add Entry
-				</Button>
-			</div>
+			<KeyValueEditor
+				{value}
+				inherited={name === 'default_tags' ? inheritedDefaultTags : {}}
+				onchange={(next) => update(name, next)}
+			/>
 		{:else}
 			{@const stringValue = typeof value === 'string' ? value : value == null ? '' : String(value)}
 			<Input