feat(themes): add editing buffer with begin/update/save/discard

HellionChat/Themes/ThemeRegistry.cs

@@ -1,3 +1,4 @@
+using System.Text.Json;
 using HellionChat.Themes.Builtin;
 using Microsoft.Extensions.Logging;
 
@@ -42,6 +43,37 @@ public sealed class ThemeRegistry
     private long _crossfadeStartTickMs = long.MinValue;
     private const int CrossfadeDurationMs = 300;
 
+    private Theme? _editingThemeBuffer;
+    public Theme? EditingThemeBuffer => _editingThemeBuffer;
+    public event Action? OnEditingBufferChanged;
+
+    // Shared slug guard for any code path that turns a slug into a filename.
+    // Both SaveEditingBuffer (F1) and ImportFromPath (M6) call this so the
+    // path-traversal/invalid-char rules live in exactly one place.
+    //
+    // Whitespace rejection is intentional: Path.GetInvalidFileNameChars on
+    // POSIX only flags NUL and '/', so a slug like "foo bar" would pass the
+    // platform check yet break URL-safety and cross-platform portability.
+    // Slugs are user-visible identifiers that may end up in filenames on
+    // Windows + Linux, in config keys, and in JSON — keeping them whitespace-
+    // free dodges the whole class of "did the user mean this or that" bugs.
+    internal static bool IsSafeThemeSlug(string? slug)
+    {
+        if (string.IsNullOrWhiteSpace(slug))
+            return false;
+
+        foreach (var c in slug)
+        {
+            if (char.IsWhiteSpace(c))
+                return false;
+        }
+
+        return !slug.Contains("..", StringComparison.Ordinal)
+            && !slug.Contains('/')
+            && !slug.Contains('\\')
+            && slug.IndexOfAny(Path.GetInvalidFileNameChars()) < 0;
+    }
+
     public ThemeRegistry(string? customThemesDir = null, ILogger<ThemeRegistry>? logger = null)
     {
         _logger = logger;
@@ -73,6 +105,49 @@ public sealed class ThemeRegistry
 
     public Theme Active => _active;
 
+    // Read-only exposure of the configured custom themes directory.
+    // M6 ThemeImportExportRow opens this path via Process.Start.
+    public string? CustomThemesDir => _customThemesDir;
+
+    // Read-only enumeration of all built-in theme slugs. T2 ThemePickerCategoryStep
+    // diffs this set against ThemePicker.CategoryMapSlugs to enforce coverage.
+    public IEnumerable<string> BuiltinSlugs => _builtIns.Keys;
+
+    // True try-pattern lookup: returns false when neither built-in nor custom
+    // cache holds the slug, no fallback to default. M3 ThemePicker uses this
+    // for card-rendering, M6 ThemeImportExportRow for fork-slug collisions.
+    // Cold-cache fallback: LoadCustomBySlug only reverse-iterates the
+    // pre-populated _customCache (see ThemeRegistry.cs:263-280). If a freshly
+    // imported file has not been enumerated yet (or no warm-up ran), the first
+    // lookup would miss silently. Drain RefreshCustomCache once on miss so the
+    // custom file gets picked up before the second lookup.
+    public bool TryGet(string slug, out Theme theme)
+    {
+        if (_builtIns.TryGetValue(slug, out var b))
+        {
+            theme = b;
+            return true;
+        }
+
+        var custom = LoadCustomBySlug(slug, out _);
+        if (custom is null)
+        {
+            // Force-enumerate the yield-iterator so _customCache picks up any
+            // file that landed in the themes dir since the last warm-up.
+            foreach (var _ in RefreshCustomCache()) { }
+            custom = LoadCustomBySlug(slug, out _);
+        }
+
+        if (custom is not null)
+        {
+            theme = custom;
+            return true;
+        }
+
+        theme = null!;
+        return false;
+    }
+
     public Theme Get(string slug)
     {
         if (_builtIns.TryGetValue(slug, out var b))
@@ -102,6 +177,12 @@ public sealed class ThemeRegistry
         if (string.Equals(_active.Slug, slug, StringComparison.OrdinalIgnoreCase))
             return;
 
+        if (_editingThemeBuffer is not null)
+        {
+            DiscardEditingBuffer();
+            _logger?.LogWarning("Theme switch to {Slug} discarded unsaved edits", slug);
+        }
+
         ArmCrossfade();
 
         if (_builtIns.TryGetValue(slug, out var builtin))
@@ -142,6 +223,11 @@ public sealed class ThemeRegistry
         if (string.Equals(_active.Slug, slug, StringComparison.OrdinalIgnoreCase))
             return;
 
+        if (_editingThemeBuffer is not null)
+        {
+            DiscardEditingBuffer();
+        }
+
         if (_builtIns.TryGetValue(slug, out var builtin))
         {
             _active = builtin;
@@ -165,6 +251,211 @@ public sealed class ThemeRegistry
         _activeCustomPath = null;
     }
 
+    public void BeginEditing(Theme source)
+    {
+        // Shallow record-with-clone: Theme.Colors gets an explicit second-level
+        // with-copy so ColorPicker edits never mutate the source record. Layout
+        // and Typography are value-record-clean (only primitive fields). Chat-
+        // Colors stays a reference share — fine for v1.7.0 because the editor
+        // never touches ChatColors. If a future cycle adds a ChatColors editor,
+        // BeginEditing must also clone the channel dictionary
+        // (ThemeChatColors holds IReadOnlyDictionary<ChatType, uint>).
+        _editingThemeBuffer = source with
+        {
+            Colors = source.Colors with { },
+        };
+    }
+
+    public void UpdateEditingBuffer(ThemeColors newColors)
+    {
+        if (_editingThemeBuffer is null)
+        {
+            return;
+        }
+
+        _editingThemeBuffer = _editingThemeBuffer with { Colors = newColors };
+        OnEditingBufferChanged?.Invoke();
+    }
+
+    // CALLER CONTRACT: the buffer slug must NOT collide with a built-in slug.
+    // Switch() prefers built-ins over custom themes with the same slug
+    // (ThemeRegistry.cs:107-112), so saving a custom file under a built-in
+    // slug persists the file but leaves the built-in active — looks green,
+    // behaves broken. M4 ColorPicker DrawIdleState forks built-in themes
+    // into a custom slug before BeginEditing, M6 ImportFromPath renames
+    // built-in-colliding imports to <slug>_imported. New call-sites must
+    // either fork first or rename to a non-built-in slug.
+    public bool SaveEditingBuffer(out string targetPath)
+    {
+        targetPath = string.Empty;
+        if (_editingThemeBuffer is null || _customThemesDir is null)
+        {
+            return false;
+        }
+
+        // Slug ends up as a filename below — refuse anything that contains path
+        // separators, parent-directory tokens, or platform-invalid filename chars.
+        // Without this guard an imported theme with Slug "../../../etc/passwd"
+        // would let Path.Combine escape _customThemesDir entirely. Shared helper
+        // so M6 ImportFromPath uses the exact same rule set.
+        var safeSlug = _editingThemeBuffer.Slug;
+        if (!IsSafeThemeSlug(safeSlug))
+        {
+            _logger?.LogWarning(
+                "Refusing to save editing buffer with unsafe slug {Slug}",
+                safeSlug
+            );
+            return false;
+        }
+
+        // Safe-by-construction: refuse any slug that collides with a built-in
+        // BEFORE we touch the disk. Switch() prefers built-ins over custom files
+        // with the same slug (ThemeRegistry.cs:107-112). Without this reject a
+        // mis-routed caller (or a future bug in ImportFromPath) could persist a
+        // custom file under a built-in slug — the file lands on disk, Switch
+        // keeps the built-in active, and the post-save active-slug check below
+        // returns false. The caller then sees "save failed" while a garbage file
+        // accumulates in the themes dir on every retry. M4 ColorPicker forks
+        // built-in themes into a custom slug before BeginEditing, M6 ImportFromPath
+        // renames built-in-colliding imports to <slug>_imported, so production
+        // paths already steer clear; this guard catches everything else.
+        if (_builtIns.ContainsKey(safeSlug))
+        {
+            _logger?.LogWarning(
+                "Refusing to save editing buffer under built-in slug {Slug}",
+                safeSlug
+            );
+            return false;
+        }
+
+        try
+        {
+            targetPath = Path.Combine(_customThemesDir, $"{safeSlug}.json");
+
+            // Defence in depth: even after the character-level scrub above, make
+            // sure the resolved full path is still rooted in _customThemesDir.
+            // Catches edge cases like alternate data streams or symlink-style
+            // tricks the loader could otherwise follow.
+            var fullDir = Path.GetFullPath(_customThemesDir);
+            var fullTarget = Path.GetFullPath(targetPath);
+            if (
+                !fullTarget.StartsWith(
+                    fullDir + Path.DirectorySeparatorChar,
+                    StringComparison.OrdinalIgnoreCase
+                )
+            )
+            {
+                _logger?.LogWarning(
+                    "Theme save target {Target} escapes themes dir {Dir}",
+                    fullTarget,
+                    fullDir
+                );
+                return false;
+            }
+
+            var json = ThemeJsonWriter.Serialize(_editingThemeBuffer);
+
+            // Atomic-replace: write to a sibling .tmp file first, then File.Move
+            // with overwrite=true. POSIX rename() and Windows MoveFileEx with
+            // MOVEFILE_REPLACE_EXISTING are both atomic on the same volume — a
+            // mid-write crash (power loss, Wine kill, OOM) leaves either the
+            // previous content or the new content on disk, never a partial JSON
+            // that would silently disappear at next Plugin-Start through the
+            // ThemeJsonLoader catch-and-continue path (ThemeRegistry.cs:319-322).
+            var tmpPath = targetPath + ".tmp";
+            File.WriteAllText(tmpPath, json);
+            File.Move(tmpPath, targetPath, overwrite: true);
+
+            // Note: the redundant `_lastActiveStamp = DateTime.MinValue` reset from
+            // the earlier plan-draft was removed — Switch() itself already resets
+            // _lastActiveStamp on the custom-theme path (ThemeRegistry.cs:124) as
+            // part of the active-switch, so a pre-Switch reset is overwritten anyway.
+
+            // RefreshCustomCache is a yield-iterator (ThemeRegistry.cs:282) — a bare
+            // call would build the iterator but never enumerate it, so the cache
+            // side-effect (_customCache[key] = (theme, stamp)) would never run.
+            // Force-enumerate so the subsequent Switch() finds the freshly saved file.
+            foreach (var _ in RefreshCustomCache()) { }
+
+            // Use the sanitised slug for Switch() too — the buffer's raw Slug
+            // already passed the guard, but staying on safeSlug keeps the lookup
+            // value consistent with the on-disk filename we just wrote.
+            var targetSlug = safeSlug;
+
+            // CRITICAL: null the buffer BEFORE Switch() so the Switch-Guard
+            // (step 3d) does not fire on our own save-internal Switch call.
+            // Without this pre-nullify the guard would log a misleading
+            // "discarded unsaved edits" warning on every save and run
+            // DiscardEditingBuffer twice (once in the guard, once at method end).
+            _editingThemeBuffer = null;
+
+            Switch(targetSlug);
+
+            // Same-slug in-place edit: Switch() hits the Same-Slug-Noop-Return
+            // (ThemeRegistry.cs:102-103) and leaves _active pointing at the
+            // PRE-edit Theme reference. The newly saved colours would only
+            // surface on the next RefreshActiveIfStale tick (1Hz-throttled,
+            // up to ~1s lag). Force-pull the freshly-cached Theme directly so
+            // the post-Save UI sees the edit in the next frame.
+            if (string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase))
+            {
+                var reloaded = LoadCustomBySlug(targetSlug, out _);
+                if (reloaded is not null)
+                {
+                    reloaded.RecomputeAbgrCache();
+                    _active = reloaded;
+                }
+            }
+
+            // Switch() falls back to DefaultSlug when neither built-in nor custom
+            // matches (ThemeRegistry.cs:128-132). Verify we actually landed on the
+            // intended theme before reporting success — a silent fallback to the
+            // default would otherwise mask a save that did persist the file but
+            // failed to become active (e.g. cache race on slow disks).
+            if (!string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase))
+            {
+                _logger?.LogWarning(
+                    "SaveEditingBuffer persisted {Path} but Switch landed on {Active} instead of {Target}",
+                    targetPath,
+                    _active.Slug,
+                    targetSlug
+                );
+                return false;
+            }
+
+            return true;
+        }
+        catch (IOException ex)
+        {
+            _logger?.LogWarning(ex, "I/O error saving editing buffer to {Path}", targetPath);
+            return false;
+        }
+        catch (UnauthorizedAccessException ex)
+        {
+            _logger?.LogWarning(ex, "Access denied saving editing buffer to {Path}", targetPath);
+            return false;
+        }
+        catch (JsonException ex)
+        {
+            // ThemeJsonWriter.Serialize could in principle throw on malformed
+            // theme graphs; keep this granular so transient I/O and serialisation
+            // failures don't get lumped together with future structural bugs.
+            // Requires `using System.Text.Json;` at the top of ThemeRegistry.cs
+            // — verify before saving and add the import if it's not yet present.
+            _logger?.LogWarning(
+                ex,
+                "JSON serialisation failed for editing buffer at {Path}",
+                targetPath
+            );
+            return false;
+        }
+    }
+
+    public void DiscardEditingBuffer()
+    {
+        _editingThemeBuffer = null;
+    }
+
     // Captures the AbgrCache snapshot that PushGlobal should fade FROM.
     // If a crossfade is already mid-flight (second Switch within 300ms),
     // the current lerped state replaces the snapshot -- the next fade