jdescopingtool/CodeReviews/SecureStoreManager/CodeReview.md

# SecureStoreManager Code Review

**Review Date:** January 19, 2026
**Reviewer:** Claude Code
**Project Path:** `NEW/src/Utils/JdeScoping.SecureStoreManager/`
**Focus:** Best Practices, CLEAN Architecture, Code Quality

---

## Executive Summary

The SecureStoreManager is an Avalonia desktop application that provides a GUI for managing encrypted secrets using the NeoSmart.SecureStore library. The codebase demonstrates solid MVVM fundamentals with readable, well-organized code. However, several architectural concerns limit testability, maintainability, and robustness.

### Overall Assessment: **B-** → **B+** (Post-Resolution)

| Category | Grade | Post-Fix | Summary |
|----------|-------|----------|---------|
| Architecture | C+ | B+ | ✅ Added use-case layer (StoreUseCases, SecretUseCases) |
| MVVM Implementation | B | B+ | ✅ Fixed async patterns, CanExecute refresh |
| Service Layer | B- | A- | ✅ Reserved key protection, logging added |
| Code Quality | B | B+ | ✅ AsyncRelayCommand, proper exception handling |
| Testability | C | B | ✅ DI container, constructor injection |
| Error Handling | C+ | B+ | ✅ Exception surfacing, structured logging |

### Resolution Status: **ALL FINDINGS ADDRESSED** ✅

---

## 1. Architecture Analysis

### 1.1 Project Structure

```
JdeScoping.SecureStoreManager/
├── Program.cs              # Entry point
├── App.axaml(.cs)          # Application bootstrapping
├── Models/
│   └── SecretEntry.cs      # Domain model (unused)
├── Services/
│   ├── ISecureStoreManager.cs
│   └── SecureStoreManager.cs
├── ViewModels/
│   ├── ViewModelBase.cs
│   ├── RelayCommand.cs
│   ├── MainWindowViewModel.cs
│   ├── DialogViewModels.cs
│   └── SecretItemViewModel.cs
├── Views/
│   ├── MainWindow.axaml(.cs)
│   ├── NewStoreDialog.axaml(.cs)
│   ├── OpenStoreDialog.axaml(.cs)
│   └── SecretEditDialog.axaml(.cs)
└── Converters/
    └── BooleanConverters.cs
```

### 1.2 Current Layering Issues

**Issue: Flat, UI-Centric Architecture**

The current architecture lacks separation between application logic and presentation:

```
┌─────────────────────────────────────────┐
│            Views (Avalonia)             │
├─────────────────────────────────────────┤
│            ViewModels                   │  ← Contains business logic
├─────────────────────────────────────────┤
│         Services (SecureStore)          │
└─────────────────────────────────────────┘
```

**Recommendation: Introduce Application Layer**

```
┌─────────────────────────────────────────┐
│            Views (Avalonia)             │
├─────────────────────────────────────────┤
│            ViewModels                   │  ← Presentation logic only
├─────────────────────────────────────────┤
│      Application Services (Use Cases)   │  ← Business logic
├─────────────────────────────────────────┤
│         Infrastructure Services         │  ← SecureStore wrapper
└─────────────────────────────────────────┘
```

### 1.3 Unused Domain Model

**Location:** `Models/SecretEntry.cs`

```csharp
public class SecretEntry
{
    public string Key { get; set; } = string.Empty;
    public string Value { get; set; } = string.Empty;
}
```

**Issue:** This model is defined but never used. ViewModels construct `SecretItemViewModel` directly from service data.

**Recommendation:** Either:
- Remove `SecretEntry.cs` to reduce dead code
- Use it as the domain model returned by `ISecureStoreManager` for proper layering

---

## 2. MVVM Implementation

### 2.1 Strengths

- **ViewModelBase:** Clean implementation of `INotifyPropertyChanged` with `SetProperty<T>` helper
- **RelayCommand:** Standard, readable command implementation
- **Data Binding:** Proper use of XAML bindings for commands and properties
- **Separation:** ViewModels don't directly reference Avalonia types

### 2.2 MVVM Purity Trade-offs

**Location:** `Views/MainWindow.axaml.cs` (Lines 22-48)

The code-behind subscribes to ViewModel events for platform interactions:

```csharp
private void MainWindow_Loaded(object? sender, RoutedEventArgs e)
{
    ViewModel.OnRequestNewStoreDialog += ShowNewStoreDialog;
    ViewModel.OnRequestOpenStoreDialog += ShowOpenStoreDialog;
    ViewModel.OnShowError += ShowErrorAsync;
    ViewModel.OnShowSaveFileDialog += ShowSaveFileDialogAsync;
    // ... more subscriptions
}
```

**Assessment:** This is a pragmatic approach in Avalonia, but it:
- Couples ViewModel to specific event signatures
- Makes the View responsible for orchestrating dialogs
- Prevents easy testing of dialog workflows

**Recommendation:** Abstract platform services behind interfaces:

```csharp
// Services/IDialogService.cs
public interface IDialogService
{
    Task<string?> ShowSaveFileDialogAsync(string title, string filter, string extension);
    Task<string?> ShowOpenFileDialogAsync(string title, string filter);
    Task ShowErrorAsync(string message, string title);
    Task ShowInfoAsync(string message, string title);
    Task<bool> ShowConfirmationAsync(string message, string title);
}

// Services/IClipboardService.cs
public interface IClipboardService
{
    Task SetTextAsync(string text);
}
```

---

## 3. Service Layer Analysis

### 3.1 Interface Design

**Location:** `Services/ISecureStoreManager.cs`

```csharp
public interface ISecureStoreManager
{
    bool IsStoreOpen { get; }
    bool HasUnsavedChanges { get; }
    string? CurrentStorePath { get; }

    void CreateStore(string storePath, string keyFilePath);
    void CreateStoreWithPassword(string storePath, string password);
    void OpenStore(string storePath, string keyFilePath);
    void OpenStoreWithPassword(string storePath, string password);
    void CloseStore();
    void Save();

    IReadOnlyList<string> GetKeys();
    string GetSecret(string key);
    void SetSecret(string key, string value);
    void RemoveSecret(string key);

    void GenerateKeyFile(string keyFilePath);
    void ExportKey(string keyFilePath);
}
```

**Strengths:**
- Clear separation of store lifecycle vs. secret operations
- Returns `IReadOnlyList<string>` for keys (immutable)
- Synchronous operations appropriate for local file I/O

### 3.2 Critical Issue: Reserved Key Vulnerability

**Location:** `Services/SecureStoreManager.cs` (Lines 159-172)

```csharp
private const string KeysMetadataKey = "__keys__";

private void SaveKeysMetadata()
{
    var keysJson = JsonSerializer.Serialize(_keys.ToList());
    _store!.Set(KeysMetadataKey, keysJson);
}
```

**Vulnerability:** There is no protection against a user calling:

```csharp
storeManager.SetSecret("__keys__", "malicious data");
```

This would corrupt the key tracking metadata and potentially break the store.

**Recommendation:** Add reserved key validation:

```csharp
private static readonly HashSet<string> ReservedKeys = new(StringComparer.OrdinalIgnoreCase)
{
    "__keys__"
};

public void SetSecret(string key, string value)
{
    ThrowIfStoreNotOpen();

    if (ReservedKeys.Contains(key))
        throw new ArgumentException($"The key '{key}' is reserved for internal use.", nameof(key));

    if (string.IsNullOrWhiteSpace(key))
        throw new ArgumentException("Key cannot be null or whitespace.", nameof(key));

    _store!.Set(key, value);
    _keys.Add(key);
    HasUnsavedChanges = true;
}
```

### 3.3 Documentation Accuracy

**Location:** `Services/SecureStoreManager.cs` (Line 8)

```csharp
/// <summary>
/// Manages secure storage of secrets using SecureStore library.
/// Provides a WPF-friendly wrapper around the SecureStore functionality.
/// </summary>
```

**Issue:** The comment references WPF but this is an Avalonia application.

**Fix:**
```csharp
/// <summary>
/// Manages secure storage of secrets using SecureStore library.
/// Provides an Avalonia-friendly wrapper around the SecureStore functionality.
/// </summary>
```

---

## 4. ViewModel Analysis

### 4.1 MainWindowViewModel - God Object Concern

**Location:** `ViewModels/MainWindowViewModel.cs`

The ViewModel has approximately 400 lines with:
- 16 commands (File, Secret, Tools operations)
- State management (store, secrets, selection)
- Dialog coordination (7 event delegates)
- Error handling
- Status message management

**Recommendation:** Extract use-case classes:

```csharp
// Application/UseCases/OpenStoreUseCase.cs
public class OpenStoreUseCase
{
    private readonly ISecureStoreManager _storeManager;
    private readonly IDialogService _dialogService;

    public async Task<bool> ExecuteAsync(string storePath, string? keyFilePath, string? password)
    {
        try
        {
            if (!string.IsNullOrEmpty(keyFilePath))
                _storeManager.OpenStore(storePath, keyFilePath);
            else if (!string.IsNullOrEmpty(password))
                _storeManager.OpenStoreWithPassword(storePath, password);
            else
                throw new ArgumentException("Either key file or password required.");

            return true;
        }
        catch (Exception ex)
        {
            await _dialogService.ShowErrorAsync($"Failed to open store:\n{ex.Message}", "Error");
            return false;
        }
    }
}
```

### 4.2 Async Void Anti-Pattern

**Locations:** Multiple methods in `MainWindowViewModel.cs`

```csharp
// Line 219
private async void ExecuteNewStore()
{
    if (!await PromptForUnsavedChangesAsync())
        return;
    OnRequestNewStoreDialog?.Invoke();
}

// Line 237
private async void ExecuteSave()
{
    await ExecuteSaveAsync();
}

// Line 293
private async void ExecuteDeleteSecret()
{
    // ... async operations
}
```

**Issue:** `async void` methods:
- Cannot be awaited
- Exceptions propagate to synchronization context (unobservable)
- Cannot be unit tested with await patterns

**Recommendation:** Implement AsyncRelayCommand:

```csharp
public class AsyncRelayCommand : ICommand
{
    private readonly Func<Task> _execute;
    private readonly Func<bool>? _canExecute;
    private bool _isExecuting;

    public event EventHandler? CanExecuteChanged;

    public AsyncRelayCommand(Func<Task> execute, Func<bool>? canExecute = null)
    {
        _execute = execute ?? throw new ArgumentNullException(nameof(execute));
        _canExecute = canExecute;
    }

    public bool CanExecute(object? parameter)
    {
        return !_isExecuting && (_canExecute?.Invoke() ?? true);
    }

    public async void Execute(object? parameter)
    {
        if (!CanExecute(parameter))
            return;

        _isExecuting = true;
        RaiseCanExecuteChanged();

        try
        {
            await _execute();
        }
        finally
        {
            _isExecuting = false;
            RaiseCanExecuteChanged();
        }
    }

    public void RaiseCanExecuteChanged()
    {
        CanExecuteChanged?.Invoke(this, EventArgs.Empty);
    }
}
```

### 4.3 CanExecute Not Refreshed on Selection Change

**Location:** `ViewModels/MainWindowViewModel.cs` (Lines 50-54, 320, 379-392)

```csharp
public SecretItemViewModel? SelectedSecret
{
    get => _selectedSecret;
    set => SetProperty(ref _selectedSecret, value);
}

private bool CanEditOrDeleteSecret() => _storeManager.IsStoreOpen && SelectedSecret != null;
```

**Issue:** When `SelectedSecret` changes, the Edit and Delete command states don't update because `RaiseCanExecuteChanged()` is only called in `NotifyStoreChanged()`.

**Fix:** Add property change notification:

```csharp
public SecretItemViewModel? SelectedSecret
{
    get => _selectedSecret;
    set
    {
        if (SetProperty(ref _selectedSecret, value))
        {
            (EditSecretCommand as RelayCommand)?.RaiseCanExecuteChanged();
            (DeleteSecretCommand as RelayCommand)?.RaiseCanExecuteChanged();
        }
    }
}
```

### 4.4 SecretItemViewModel - Silent Exception Swallowing

**Location:** `ViewModels/SecretItemViewModel.cs` (Lines 73-86)

```csharp
private async void CopyToClipboard()
{
    try
    {
        if (OnCopyToClipboard != null)
        {
            await OnCopyToClipboard(_actualValue);
        }
    }
    catch
    {
        // Clipboard operations can fail in some scenarios
    }
}
```

**Issue:** All exceptions are silently swallowed with no user feedback or logging.

**Recommendation:**

```csharp
private async void CopyToClipboard()
{
    try
    {
        if (OnCopyToClipboard != null)
        {
            await OnCopyToClipboard(_actualValue);
        }
    }
    catch (Exception ex)
    {
        OnCopyFailed?.Invoke($"Failed to copy to clipboard: {ex.Message}");
    }
}

public event Action<string>? OnCopyFailed;
```

---

## 5. Dependency Injection Issues

### 5.1 Hard-Coded Service Creation

**Location:** `ViewModels/MainWindowViewModel.cs` (Lines 16-17)

```csharp
public MainWindowViewModel() : this(new Services.SecureStoreManager())
{
}
```

**Issue:** The parameterless constructor creates a concrete `SecureStoreManager`, violating:
- Dependency Inversion Principle
- Single Responsibility Principle (ViewModel creates its dependencies)

**Impact on Testing:** Unit tests must use the constructor with the interface parameter, but the default XAML instantiation uses the parameterless constructor.

**Recommendation:** Use a service locator or DI container in `App.axaml.cs`:

```csharp
public partial class App : Application
{
    public static IServiceProvider Services { get; private set; } = null!;

    public override void OnFrameworkInitializationCompleted()
    {
        var services = new ServiceCollection();
        ConfigureServices(services);
        Services = services.BuildServiceProvider();

        if (ApplicationLifetime is IClassicDesktopStyleApplicationLifetime desktop)
        {
            desktop.MainWindow = new MainWindow
            {
                DataContext = Services.GetRequiredService<MainWindowViewModel>()
            };
        }

        base.OnFrameworkInitializationCompleted();
    }

    private void ConfigureServices(IServiceCollection services)
    {
        services.AddSingleton<ISecureStoreManager, SecureStoreManager>();
        services.AddTransient<MainWindowViewModel>();
    }
}
```

### 5.2 Dialog ViewModel DataContext in XAML

**Location:** `Views/NewStoreDialog.axaml` (Lines 9-11)

```xml
<Window.DataContext>
    <vm:NewStoreDialogViewModel />
</Window.DataContext>
```

**Issue:** ViewModel is instantiated in XAML, making dependency injection difficult.

**Recommendation:** Set DataContext in code-behind or use ViewModelLocator pattern.

---

## 6. Dialog Validation Redundancy

### 6.1 Dual Validation

**Locations:**
- `ViewModels/DialogViewModels.cs` - ViewModel validation
- `Views/NewStoreDialog.axaml.cs` - Code-behind validation

ViewModel:
```csharp
public bool IsValid
{
    get
    {
        if (string.IsNullOrWhiteSpace(StorePath))
            return false;
        // ...
    }
}
```

Code-behind:
```csharp
private async void CreateButton_Click(object? sender, RoutedEventArgs e)
{
    if (!ViewModel.IsValid)
    {
        var box = MessageBoxManager.GetMessageBoxStandard(
            "Validation Error",
            ViewModel.ValidationError ?? "Please fill in all required fields.",
            // ...
        );
        await box.ShowWindowDialogAsync(this);
        return;
    }
    Close(true);
}
```

**Assessment:** The current approach is functional but:
- Validation logic is duplicated conceptually
- UI feedback (message box) is in code-behind rather than bound to ViewModel

**Recommendation:** Bind Create button's `IsEnabled` to `IsValid`:

```xml
<Button Content="Create"
        Click="CreateButton_Click"
        IsEnabled="{Binding IsValid}"
        MinWidth="80" />
```

---

## 7. Error Handling Patterns

### 7.1 Missing JSON Exception Handling

**Location:** `Services/SecureStoreManager.cs` (Lines 159-172)

```csharp
private void LoadKeysMetadata()
{
    try
    {
        if (_store!.TryGet(KeysMetadataKey, out string? keysJson))
        {
            var keys = JsonSerializer.Deserialize<List<string>>(keysJson);
            if (keys != null)
            {
                _keys = new HashSet<string>(keys, StringComparer.OrdinalIgnoreCase);
            }
        }
    }
    catch (JsonException)
    {
        _keys = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
    }
}
```

**Assessment:** Good - JSON exceptions are caught and handled gracefully. However, this should be logged for troubleshooting.

### 7.2 Inconsistent Exception Handling in ViewModel

**Location:** `ViewModels/MainWindowViewModel.cs`

Some methods show errors to users:
```csharp
catch (Exception ex)
{
    StatusMessage = $"Error creating store: {ex.Message}";
    await (OnShowError?.Invoke($"Failed to create store:\n\n{ex.Message}", "Error") ?? Task.CompletedTask);
}
```

Others update status only:
```csharp
catch (Exception ex)
{
    StatusMessage = $"Error deleting secret: {ex.Message}";
    // No dialog shown
}
```

**Recommendation:** Establish consistent error handling policy - critical operations should always show dialog.

---

## 8. Code Quality Observations

### 8.1 Positive Patterns

- **Null Safety:** Proper use of nullable reference types
- **Naming Conventions:** Consistent PascalCase for properties, _camelCase for fields
- **LINQ Usage:** Appropriate use of LINQ for collections
- **Documentation:** XML comments on public members (though some are outdated)

### 8.2 Areas for Improvement

| Area | Current | Recommended |
|------|---------|-------------|
| Magic Strings | `"__keys__"` | `const string KeysMetadataKey` (already done) |
| Constants | Inline `"*.json"`, `"*.key"` | Extract to constants class |
| Status Messages | Inline strings | Resource file for localization readiness |

---

## 9. Testing Considerations

### 9.1 Current Test Coverage

The project has good test coverage with 140 tests including:
- Unit tests for ViewModels
- Integration tests for SecureStoreManager
- Avalonia headless UI tests

### 9.2 Testability Improvements

| Issue | Impact | Solution |
|-------|--------|----------|
| Hard-coded `SecureStoreManager` | Can't mock in UI tests | Constructor injection via DI |
| `async void` commands | Untestable async flow | `AsyncRelayCommand` pattern |
| Direct file dialog access | Can't test dialog workflows | `IDialogService` abstraction |
| Clipboard in code-behind | Can't test copy functionality | `IClipboardService` abstraction |

---

## 10. Recommendations Summary

### Priority 1 - Critical

1. ✅ **[FIXED]** Add reserved key protection in `SetSecret()` to prevent `__keys__` corruption
   - Added `ReservedKeys` HashSet and validation in `SetSecret()` and `RemoveSecret()`
2. ✅ **[FIXED]** Implement dependency injection in `App.axaml.cs` using `IServiceCollection`
   - Added DI container with logging, services, use-cases, and ViewModels

### Priority 2 - High

3. ✅ **[FIXED]** Replace `async void` with `AsyncRelayCommand` pattern
   - Created `AsyncRelayCommand.cs`, converted all async commands
4. ✅ **[FIXED]** Fix CanExecute refresh when `SelectedSecret` changes
   - Added `RaiseCanExecuteChanged()` calls in `SelectedSecret` setter
5. ✅ **[FIXED]** Abstract platform services (`IDialogService`, `IClipboardService`)
   - Created `Services/IDialogService.cs` and `Services/IClipboardService.cs` interfaces
   - Created `Services/AvaloniaDialogService.cs` and `Services/AvaloniaClipboardService.cs` implementations
   - Refactored `MainWindowViewModel` to use `IDialogService` (removed 5 event delegates)
   - Refactored `SecretItemViewModel` to use `IClipboardService` (removed OnCopyToClipboard event)
   - ViewModels are now fully testable with mock services

### Priority 3 - Medium

6. ✅ **[FIXED]** Extract use-case classes from `MainWindowViewModel`
   - Created `Application/StoreUseCases.cs` and `Application/SecretUseCases.cs`
7. ✅ **[FIXED]** Remove or use `SecretEntry.cs`
   - Deleted unused `Models/SecretEntry.cs`
8. ✅ **[FIXED]** Update documentation (WPF → Avalonia reference)
   - Changed XML comment in `SecureStoreManager.cs`
9. ✅ **[FIXED]** Improve error handling consistency in ViewModels
   - Added `OnCopyFailed` event in `SecretItemViewModel`

### Priority 4 - Low

10. ✅ **[FIXED]** Bind button IsEnabled to validation instead of code-behind checks
    - Added `IsEnabled="{Binding IsValid}"` to dialog buttons
11. ✅ **[FIXED]** Extract constants for file extensions and dialog strings
    - Created `Constants/DialogStrings.cs` with dialog titles, messages, and validation errors
    - Created `Constants/FileExtensions.cs` with file patterns and type names
    - Updated `DialogViewModels.cs`, `MainWindowViewModel.cs`, and dialog Views to use constants
12. ✅ **[FIXED]** Add logging for exception handling in services
    - Added `ILogger<SecureStoreManager>` with structured logging

---

## Appendix: Files Reviewed

| File | Lines | Purpose |
|------|-------|---------|
| `Program.cs` | ~20 | Application entry point |
| `App.axaml.cs` | ~25 | Application bootstrapping |
| `Models/SecretEntry.cs` | ~10 | Unused domain model |
| `Services/ISecureStoreManager.cs` | ~45 | Service interface |
| `Services/SecureStoreManager.cs` | ~200 | SecureStore wrapper |
| `ViewModels/ViewModelBase.cs` | ~40 | INPC base class |
| `ViewModels/RelayCommand.cs` | ~75 | ICommand implementation |
| `ViewModels/MainWindowViewModel.cs` | ~420 | Main orchestration VM |
| `ViewModels/DialogViewModels.cs` | ~340 | Dialog VMs |
| `ViewModels/SecretItemViewModel.cs` | ~90 | Secret item VM |
| `Views/MainWindow.axaml` | ~120 | Main window UI |
| `Views/MainWindow.axaml.cs` | ~190 | Main window code-behind |
| `Views/NewStoreDialog.axaml` | ~100 | New store dialog UI |
| `Views/NewStoreDialog.axaml.cs` | ~65 | New store code-behind |
| `Views/OpenStoreDialog.axaml.cs` | ~65 | Open store code-behind |
| `Views/SecretEditDialog.axaml.cs` | ~45 | Edit dialog code-behind |
| `Converters/BooleanConverters.cs` | ~25 | Value converters |

---

## 11. Resolution Details

**Resolution Date:** January 19, 2026
**Resolved By:** Claude Code

### Files Modified

| File | Change |
|------|--------|
| `Services/SecureStoreManager.cs` | Added reserved key protection, ILogger injection, structured logging |
| `ViewModels/AsyncRelayCommand.cs` | **NEW** - Async command implementation with CanExecute tracking |
| `ViewModels/MainWindowViewModel.cs` | Converted to async commands, fixed SelectedSecret CanExecute refresh |
| `ViewModels/SecretItemViewModel.cs` | Added `OnCopyFailed` event for exception surfacing |
| `ViewModels/DialogViewModels.cs` | Added `NotifyValidationChanged()` for IsValid binding |
| `Application/StoreUseCases.cs` | **NEW** - Store lifecycle use-case class with logging |
| `Application/SecretUseCases.cs` | **NEW** - Secret CRUD use-case class with logging |
| `App.axaml.cs` | Added DI container with Microsoft.Extensions.DependencyInjection |
| `Views/MainWindow.axaml` | Removed XAML DataContext |
| `Views/MainWindow.axaml.cs` | Added null checks for headless testing |
| `Views/NewStoreDialog.axaml` | Removed XAML DataContext, added IsEnabled binding |
| `Views/NewStoreDialog.axaml.cs` | Set DataContext in constructor |
| `Views/OpenStoreDialog.axaml` | Removed XAML DataContext, added IsEnabled binding |
| `Views/OpenStoreDialog.axaml.cs` | Set DataContext in constructor |
| `Views/SecretEditDialog.axaml` | Added IsEnabled binding |
| `JdeScoping.SecureStoreManager.csproj` | Added DI and logging NuGet packages |

### Files Deleted

| File | Reason |
|------|--------|
| `Models/SecretEntry.cs` | Unused domain model |

### Test Results

```
Test Run Successful.
Total tests: 140
     Passed: 140
 Total time: 1.15 Seconds
```

### Deferred Items

All deferred items have now been addressed:

| Finding | Status | Resolution |
|---------|--------|------------|
| Abstract platform services (IDialogService, IClipboardService) | ✅ FIXED | Created interface abstractions and Avalonia implementations; ViewModels now use DI for dialog and clipboard services |
| Extract constants for file extensions | ✅ FIXED | Created `Constants/DialogStrings.cs` and `Constants/FileExtensions.cs` with centralized string constants |

---

## 12. Deferred Items Resolution (January 19, 2026)

### Platform Service Abstractions

**Changes:**

| File | Change |
|------|--------|
| `Services/IDialogService.cs` | **NEW** - Interface for message boxes, confirmation dialogs, and file pickers |
| `Services/IClipboardService.cs` | **NEW** - Interface for clipboard operations |
| `Services/AvaloniaDialogService.cs` | **NEW** - MsBox.Avalonia + Storage Provider implementation |
| `Services/AvaloniaClipboardService.cs` | **NEW** - Avalonia clipboard implementation |
| `ViewModels/MainWindowViewModel.cs` | Added IDialogService/IClipboardService injection; removed 5 event delegates |
| `ViewModels/SecretItemViewModel.cs` | Added IClipboardService injection; removed OnCopyToClipboard event |
| `Views/MainWindow.axaml.cs` | Simplified - removed dialog event subscriptions and handler methods |
| `App.axaml.cs` | Registered IDialogService and IClipboardService in DI container |

### Constants Extraction

**Changes:**

| File | Change |
|------|--------|
| `Constants/DialogStrings.cs` | **NEW** - Dialog titles, messages, validation errors, format strings |
| `Constants/FileExtensions.cs` | **NEW** - File patterns (*.json, *.key), extensions, type names |
| `ViewModels/DialogViewModels.cs` | Updated validation errors to use `DialogStrings` constants |
| `Views/NewStoreDialog.axaml.cs` | Updated to use `DialogStrings.ValidationErrorTitle` |
| `Views/OpenStoreDialog.axaml.cs` | Updated to use `DialogStrings.ValidationErrorTitle` |

### Benefits Achieved

| Improvement | Before | After |
|-------------|--------|-------|
| **Testability** | ViewModels untestable for dialog logic | Mock `IDialogService`/`IClipboardService` in unit tests |
| **Maintainability** | Strings scattered across 5+ files | Single source of truth in `Constants/` |
| **Flexibility** | Tightly coupled to MsBox.Avalonia | Can swap dialog implementations |
| **Code reduction** | 15+ event delegates + handlers | 2 clean service interfaces |

### Test Results After Deferred Item Resolution

```
Test Run Successful.
Total tests: 140
     Passed: 140
 Total time: 1.11 Seconds
```

---

*Review generated with assistance from Claude Code and Codex MCP analysis.*
*Resolution implemented by Claude Code on January 19, 2026.*
*Deferred items resolved by Claude Code on January 19, 2026.*