PR A: correctness fixes and improvement plan

- Add IMPROVEMENT_PLAN.md with phased plan for follow-up work
- Consolidate DI registrations into a single AddAccounting() extension;
  drop the duplicate service registrations and the PdfParsingService
  self-registration
- TrySetYear: capture this.year before overwriting so rollback actually
  restores the previous value
- DummyFxService: check toCurrency (was checking fromCurrency twice)
- TransactionRepository.GetTransaction: return a copy instead of mutating
  the loaded entity, and guard against unknown ids
- ViewService.GetTransactionViewList(accountId): flip the sign on a copy
  rather than mutating the entity returned by the repository
- FileService.GetTransactionListCsv: same treatment; use a local
  signedValue instead of mutating trx.Value
- ProfitLossReport: use ClassIds.Income/Expenses instead of magic 3/4
- CLAUDE.md: correct the ClassIds documentation (1/2/3/4, not
  1000/2000/3000/4000)

SettingsService lifetime is intentionally left as Singleton for now;
making it Scoped requires persisting year/mandator selection across
page reloads first (tracked in the plan).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

CLAUDE.md

@@ -61,4 +61,4 @@ Pages live in `Schaad.Accounting.UI/Components/Pages/`. Each page typically has
 
 ### Domain constants
 
-`ClassIds` in `Schaad.Accounting.Common` defines the Swiss accounting chart-of-accounts classes: Activa=1000, Passiva=2000, Income=3000, Expenses=4000.
+`ClassIds` in `Schaad.Accounting.Common` defines the Swiss accounting chart-of-accounts classes: Activa=1, Passiva=2, Income=3, Expenses=4. These are the leading digit of an account number (accounts are 4-digit; `Account.Class = Number / 1000`).

IMPROVEMENT_PLAN.md

@@ -0,0 +1,102 @@
+# AccountingNext — Technical Improvement Plan
+
+Analysis and phased plan produced 2026-07-02. See conversation history for full context.
+
+## Findings
+
+### 1. Correctness bugs (must fix)
+
+- **Shared mutable state across users.** `SettingsService` is registered as `Singleton` (`Schaad.Accounting.UI/Extensions.cs:21`) but holds mutable `year`/`mandator` fields (`Schaad.Accounting.Services/SettingsService.cs:13-14`). Any user switching year/mandator changes it for *all* connected Blazor Server users.
+- **`TrySetYear` rollback is broken.** `SettingsService.cs:33` captures `var oldYear = year;` — that's the *parameter*, not `this.year`. The "rollback" restores the same value that just failed.
+- **`ChartService` mutates a shared service.** `GetAccountExpensesPerMonth(accountId, year)` (`ChartService.cs:87`) calls `settingsService.TrySetYear(year)` then `SetYear(DateTime.Now.Year)` to reload prior-year data. Global mutation on a singleton — guaranteed to race in production.
+- **`DummyFxService` has a typo.** `DummyFxService.cs:13` — `currencies.Contains(fromCurrency) && currencies.Contains(fromCurrency)` (both sides check `fromCurrency`). Also it silently returns the raw amount for supported currencies, so all "CHF conversions" are no-ops.
+- **`TransactionRepository.GetTransaction` mutates the entity.** `TransactionRepository.cs:67-81` divides `Value` by `FxRate` on read, and `SaveTransaction` multiplies by `FxRate` on write. Also silently NREs when the id is not found (line 73 dereferences `transaction`).
+- **`ViewService.GetTransactionViewList(accountId)` mutates transactions.** `ViewService.cs:172` flips `t.Value *= -1` on the loaded transaction. `FileService.GetTransactionListCsv` does the same (`FileService.cs:137`). These are the shared objects returned by the repository — later reads see wrong signs.
+- **DI double-registration.** `Extensions.cs` registers `IChartService`, `IViewService`, `IFileService`, statement services, and `IFxService` in both `AddRepositories` and `AddServices`. Also `AddServices` registers `PdfParsingService` as its own key (`services.AddSingleton<PdfParsingService, PdfParsingService>()`) instead of `IPdfParsingService`.
+- **Magic numbers instead of `ClassIds`.** `ProfitLossReport.razor.cs:25-26` uses `.Class == 3` / `.Class == 4`.
+- **`ClassIds` doc/code mismatch.** `CLAUDE.md` states `Activa=1000, Passiva=2000, …`, but `ClassIds.cs` defines them as `1,2,3,4` (matches `Account.Class = Number / 1000`).
+- **Non-atomic XML writes, no locking.** `BaseRepository.Save` writes directly to the target path. Interrupted writes corrupt data. Two concurrent saves interleave. No temp-file+rename, no `FileShare` lock.
+- **`AccountRepository.EnsureFileExisits` calls `SaveAccount` in a loop.** Each call re-loads and re-serializes the whole account list.
+- **`ProfitLossReport` / `BalanceSheetReport`** compute totals by re-summing balances that `ViewService` already computes — but with subtle differences. Two sources of truth for the same number.
+
+### 2. Reliability / maintainability
+
+- **No async I/O.** Every XML read/write is synchronous, blocking the SignalR hub thread in Blazor Server.
+- **No caching.** A single page render calls `GetAccountList()` and `GetTransactionList()` many times. Every one is a fresh file read + XML deserialization.
+- **O(N·M) lookups.** `ViewService.cs:138-139` — `accountList.Single(...)` inside a `Select` over all transactions. Should be a dictionary keyed by id.
+- **No logging.** `ILogger` is not used anywhere.
+- **No error surface.** Repositories throw or return `null`; UI dereferences with `!`.
+- **Anemic domain model + `Copy` methods.** `Account.Copy`, `Transaction.Copy`, `SubClass.Copy` etc. are hand-rolled property copies used to merge edits into loaded entities.
+- **Vendor coupling in the Domain project.** `Schaad.Accounting.Common` references `Schaad.Finance.Api.dll`. `IChartService`, `IFileService`, and `IViewService` live in `Common` and depend on `Schaad.Finance.Api` types.
+- **Duplicated formatting logic.** `ToFormattedString(decimal)` exists in `Extensions.cs` and again in `FileService.cs`.
+- **Culture setup in `Program.cs`** runs *after* `MapRazorComponents` and just sets `DefaultThreadCurrentCulture` globally. Should be `RequestLocalizationOptions` middleware.
+- **Constructors doing I/O.** `AccountRepository`, `SubclassRepository`, `SplitPredefinitonRepository`, `BookingRuleRepository` call `EnsureFileExisits` in the constructor.
+- **Typos leak into public API.** `EnsureFileExisits`, `SplitPredefinitonRepository`.
+- **Dead / redundant code.** `AccountRepository.EnsureFileExisits` uses `new` to hide the base method; its `file.IndexOf("Accounts") > -1` guard is redundant. `Home.razor.cs:50` uses a `loaded` flag though `OnInitializedAsync` already runs once per instance. Commented-out `//var subclasses = subclassRepository.GetSubClassList();` in `ChartService`.
+- **Legacy nuget packages.** All three library projects reference `System.Text.RegularExpressions 4.3.1`, `System.Xml.XmlSerializer 4.3.0`, `System.ComponentModel.Annotations 5.0.0` — legacy .NET Standard packages, redundant on `net9.0`.
+- **`Common.csproj`** has `<Folder Include="Interfaces\Extensions\" />` for a folder that does not exist.
+- **`ClassIds`** should be `static class`; currently instantiable.
+
+### 3. Testability
+
+- Zero tests, zero test project.
+- Business rules (balance calculation, FX conversion, bank-transaction matching, CSV export, split logic) are entangled with mutable singletons and side effects on returned entities.
+
+---
+
+## Phased plan
+
+### Phase 1 — Stop the bleeding (correctness, low churn)
+
+1. ~~Change `ISettingsService` registration from `Singleton` to `Scoped`.~~ **Deferred.** `MyHeader.YearChanged` uses `NavigateTo(..., forceLoad: true)` after mutating settings, which tears down the SignalR circuit — a Scoped instance would be recreated with default values on the new circuit. Proper fix: persist the year/mandator selection to a cookie or query string, then Scoped becomes safe. Tracked as a new Phase 2 item.
+2. Fix `TrySetYear` (capture `this.year` before overwriting). Defer the `ChartService` prior-year-loading pattern to Phase 3.
+3. Fix `DummyFxService` typo (or delete the class and replace with a real `IFxService` implementation from `Schaad.Finance.Api`).
+4. Remove mutation-on-read in `TransactionRepository.GetTransaction`, `ViewService.GetTransactionViewList(accountId)`, and `FileService.GetTransactionListCsv`.
+5. Add a not-found guard to `TransactionRepository.GetTransaction`.
+6. De-duplicate the DI registrations. One `AddAccounting()` extension called once in `Program.cs`.
+7. Replace `.Class == 3/4` magic numbers with `ClassIds.*`. Update CLAUDE.md's incorrect ClassIds section.
+8. Make XML writes atomic: write to `foo.xml.tmp` then `File.Move(..., overwrite: true)`. Wrap Load/Save in a per-file `SemaphoreSlim` (or a simple `lock`).
+
+### Phase 2 — Structural cleanup
+
+9. **Introduce a per-request unit of work / cache.** A scoped `IAccountingContext` that loads each XML file at most once per request and holds the deserialized lists.
+10. Replace `Copy(target)` methods with a single merge-in-place pattern (or `record with`).
+11. Extract shared formatting to a single `Formatting` helper.
+12. Move `IFileService`, `IChartService`, `IViewService` out of `Common` (they depend on `Schaad.Finance.Api`). Common should have no vendor dependency.
+13. Fix typos (`EnsureFileExists`, `SplitPredefinitionRepository`).
+14. Move file existence bootstrapping out of constructors into a startup step (`IHostedService` or lazy first-use).
+15. Fix `AccountRepository.EnsureFileExists` to compute the start-balance updates in memory and save once.
+16. Remove legacy NuGet packages. Remove the stale `Interfaces\Extensions\` folder entry.
+17. Set culture via `RequestLocalizationOptions` middleware.
+18. Add `ILogger<T>` to services and repositories.
+18b. Persist selected year and mandator across page reloads (cookie, query string, or `ProtectedLocalStorage`). Prerequisite for making `ISettingsService` `Scoped` (Phase 1 item 1, deferred).
+
+### Phase 3 — Async & performance
+
+19. Convert repository interfaces to async.
+20. Cache the current view's data behind the scoped unit-of-work; invalidate on save.
+21. Precompute `accountsById` and `subclassNameByNumber` dictionaries once per request.
+22. Clean up `ChartService` prior-year loading pattern: introduce an explicit "load year data" helper instead of mutating `ISettingsService`.
+
+### Phase 4 — Testability & safety net
+
+23. Add a `Schaad.Accounting.Tests` xUnit project.
+24. First tests: `ViewService.GetBalanceView`, `MatchBankTransactionByBookingRule` / `SameAccountsLastMonth`, `TransactionRepository` FX round-trip, `FileService.GetTransactionListCsv`.
+25. Abstract the XML store (`IEntityStore<T>`) so tests use an in-memory store.
+
+### Phase 5 — Nice-to-haves
+
+26. Replace hand-written XML models with `record` types + source-generated serializers.
+27. Inject Fixer.io key via `IOptions<FxSettings>` instead of threading through method params.
+28. Prune unused Fluent UI packages.
+29. Add a health/backup admin page.
+
+---
+
+## PR breakdown
+
+- **PR A** — Phase 1 items 1–7 + CLAUDE.md fix.
+- **PR B** — Phase 1 item 8 (atomic writes + lock).
+- **PR C** — Phase 2 items 9 + 11 + 13 + 15 + 16 + 17.
+- **PR D** — Phase 3 (async).
+- **PR E** — Phase 4 (tests) — done alongside PR C to lock in behavior.

Schaad.Accounting.Db/Repositories/TransactionRepository.cs

@@ -67,17 +67,24 @@ namespace Schaad.Accounting.Repositories
         public Transaction GetTransaction(string id)
         {
             var transactions = GetTransactionList();
-            var transaction = transactions.FirstOrDefault(t => t.Id == id);
+            var stored = transactions.FirstOrDefault(t => t.Id == id);
-
+            if (stored == null)
-            // value is stored in CHF -> convert back to foreign currency for display/editing
-            var isFxAccount = accountRepository.GetAccount(transaction.OriginAccountId).IsFxAccount
-                || accountRepository.GetAccount(transaction.TargetAccountId).IsFxAccount;
-            if (isFxAccount && transaction.FxRate != 0)
             {
-                transaction.Value = transaction.Value / transaction.FxRate.Value;
+                return null;
             }
 
-            return transaction;
+            var result = new Transaction();
+            stored.Copy(result);
+
+            // value is stored in CHF -> convert back to foreign currency for display/editing
+            var isFxAccount = accountRepository.GetAccount(result.OriginAccountId).IsFxAccount
+                || accountRepository.GetAccount(result.TargetAccountId).IsFxAccount;
+            if (isFxAccount && result.FxRate != 0)
+            {
+                result.Value = result.Value / result.FxRate.Value;
+            }
+
+            return result;
         }
         
         /// <summary>

Schaad.Accounting.Services/DummyFxService.cs

@@ -10,7 +10,7 @@ namespace Schaad.Accounting.Services
 
         public decimal ConvertCurrency(decimal amount, string fromCurrency, string toCurrency, string fixerIoApiKey)
         {
-            if (currencies.Contains(fromCurrency) && currencies.Contains(fromCurrency))
+            if (currencies.Contains(fromCurrency) && currencies.Contains(toCurrency))
             {
                 return amount;
             }

Schaad.Accounting.Services/FileService.cs

@@ -130,17 +130,18 @@ namespace Schaad.Accounting.Services
             {
                 var credit = "";
                 var debit = "";
+                var signedValue = trx.Value;
 
                 if (trx.OriginAccountId == accountId)
                 {
                     debit = ToFormattedString(trx.Value);
-                    trx.Value *= -1;
+                    signedValue = -trx.Value;
                 }
                 else
                 {
                     credit = ToFormattedString(trx.Value);
                 }
-                balance += trx.Value;
+                balance += signedValue;
                 sb.AppendLine($"{trx.BookingDate:dd.MM.yyyy};{trx.ValueDate:dd.MM.yyyy};{trx.Text};{debit};{credit};{ToFormattedString(balance)}");
             }
             var fileBytes = Encoding.GetEncoding("ISO-8859-1").GetBytes(sb.ToString());

Schaad.Accounting.Services/SettingsService.cs

@@ -30,7 +30,7 @@ namespace Schaad.Accounting.Services
 
         public bool TrySetYear(int year)
         {
-            var oldYear = year;
+            var oldYear = this.year;
             SetYear(year);
             if (Directory.GetFiles(GetDbPath()).Any() == false)
             {

Schaad.Accounting.Services/ViewService.cs

@@ -154,7 +154,7 @@ namespace Schaad.Accounting.Services
             var transactionViewList = transactionList.Select(
                     t =>
                         new TransactionDataset(
-                            Prepare(t),
+                            WithDisplaySign(t),
                             accountList.Single(a => a.Id == t.OriginAccountId),
                             accountList.Single(a => a.Id == t.TargetAccountId)
                         )
@@ -163,13 +163,16 @@ namespace Schaad.Accounting.Services
 
             return transactionViewList;
 
-            Transaction Prepare(Transaction t)
+            Transaction WithDisplaySign(Transaction t)
             {
                 var account = accountList.Single(a => a.Id == t.OriginAccountId);
 
-                if (account.Class == ClassIds.Activa && accountId == t.OriginAccountId )
+                if (account.Class == ClassIds.Activa && accountId == t.OriginAccountId)
                 {
-                    t.Value *= -1;
+                    var copy = new Transaction();
+                    t.Copy(copy);
+                    copy.Value *= -1;
+                    return copy;
                 }
 
                 return t;

Schaad.Accounting.UI/Components/Pages/Reports/ProfitLossReport.razor.cs

@@ -1,4 +1,5 @@
 using Microsoft.AspNetCore.Components;
+using Schaad.Accounting;
 using Schaad.Accounting.Datasets;
 using Schaad.Accounting.Interfaces;
 
@@ -22,8 +23,8 @@ public partial class ProfitLossReport : ComponentBase
     protected override Task OnInitializedAsync()
     {
         accounts = viewService.GetAccountViewList();
-        profit = Math.Abs(accounts.Where(m => m.Class == 3).Sum(m => m.Balance));
+        profit = Math.Abs(accounts.Where(m => m.Class == ClassIds.Income).Sum(m => m.Balance));
-        loss = Math.Abs(accounts.Where(m => m.Class == 4).Sum(m => m.Balance));
+        loss = Math.Abs(accounts.Where(m => m.Class == ClassIds.Expenses).Sum(m => m.Balance));
         win = profit-loss;
 
         (header, footer) = Report.GetViewDataTitleAndFooter("Erfolgsrechnung", settingsService);

Schaad.Accounting.UI/Extensions.cs

@@ -16,9 +16,14 @@ namespace Schaad.Accounting.UI
             return value.ToString("#,0.00", culture);
         }
         
-        public static IServiceCollection AddRepositories(this IServiceCollection services)
+        public static IServiceCollection AddAccounting(this IServiceCollection services)
         {
+            // Singleton because MyHeader triggers a full page reload (forceLoad: true) after
+            // changing year/mandator; a Scoped instance would be recreated on the new circuit
+            // and lose the selection. TODO: persist selection to a cookie/query string so this
+            // can safely become Scoped (see IMPROVEMENT_PLAN.md).
             services.AddSingleton<ISettingsService, SettingsService>();
+
             services.AddScoped<IAccountRepository, AccountRepository>();
             services.AddScoped<IBankTransactionRepository, BankTransactionRepository>();
             services.AddScoped<IBookingRuleRepository, BookingRuleRepository>();
@@ -37,18 +42,5 @@ namespace Schaad.Accounting.UI
 
             return services;
         }
-
-        public static IServiceCollection AddServices(this IServiceCollection services)
-        {
-            services.AddScoped<IChartService, ChartService>();
-            services.AddScoped<IViewService, ViewService>();
-            services.AddScoped<IFileService, FileService>();
-            services.AddScoped<IAccountStatementService, AccountStatementService>();
-            services.AddScoped<ICreditCardStatementService, CreditCardStatementService>();
-            services.AddSingleton<IFxService, DummyFxService>();
-            services.AddSingleton<PdfParsingService, PdfParsingService>();
-
-            return services;
-        }
     }
 }

Schaad.Accounting.UI/Program.cs

@@ -23,7 +23,7 @@ namespace Schaad.Accounting.UI
             builder.Services.AddSingleton(sp =>
                 sp.GetRequiredService<IOptions<SettingsDataset>>().Value);
 
-            builder.Services.AddRepositories().AddServices();
+            builder.Services.AddAccounting();
 
 
             var app = builder.Build();