# 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()`) 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 `` 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` 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`) 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` 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 mechanical cleanup: items 11, 13, 15, 16, 17. - **PR D** — Phase 2 item 9: per-request unit of work / cache. Split off from PR C because it is invasive enough to warrant its own review. - **PR E** — Phase 3 item 22: ChartService cleanup. Promoted ahead of async because it fixes an active correctness bug — `GetAccountExpensesPerMonth(accountId, year)` was mutating `ISettingsService` on the singleton to hop years, with the year parameter always hardcoded to `DateTime.Now.Year`, so the Spendings-over-time chart discarded the user's header year selection. - **PR F** — Phase 3 item 21: precomputed dictionaries in `ViewService` to eliminate O(N·M) `Single(...)` scans and per-account transaction filtering. - **Item 19 (async I/O)** — deferred. After PR D, each XML file is loaded at most once per SignalR circuit, and this app is single-user local Blazor Server. Converting every repository/service method to async would touch ~40 files for negligible user-visible benefit and real regression risk. Revisit if the app is ever hosted for multiple concurrent users. - **PR G** — Phase 4 (tests) — locks in behavior before further invasive changes.