fix(loggers): extract SessionDataRepository and surface session-delete failures (#592)#607
Conversation
…e failures (#592) Extract the session read/delete path out of the 1,721-line DatabaseLogger god class into a testable SessionDataRepository (issue #592), and fix the latent data-integrity bug the extraction unblocks. Extraction (behavior-preserving): - SessionDataRepository owns the EF/ADO read path: channel discovery + the fast initial batch (LoadInitialSession), the full-range sampled load (LoadSampledData), the single-timestamp value spread, per-device frequency, and the transactional delete. Constructor-injected with only IDbContextFactory<LoggingContext> + IAppLogger, so it is unit-testable with no WPF/OxyPlot-engine dependency (the SessionSampleWriter playbook applied to the read side). - DatabaseLogger.DisplayLoggingSession becomes a thin orchestrator that builds the plot series/legend around the points the repository returns; the moved methods become repository calls. DatabaseLogger drops from 1,721 to 1,376 lines. AddChannelSeries no longer seeds the point dictionary (the repository owns that now). - The internal-static test seams (DeduplicateChannelInfo, LoadSingleTickValueSpread) move with the read path; their #572 regression tests are renamed and repointed to SessionDataRepository. Bug fix ("delete failure undetected"): - SessionDataRepository.DeleteSession now logs AND rethrows on failure instead of swallowing the exception. Previously DatabaseLogger.DeleteLoggingSession logged-and-swallowed, so LoggingSessionListViewModel removed the bound row even when the SQL delete failed: the row vanished from the UI while its data remained, reappearing on the next reload. With the rethrow, the view model's existing delete gating keeps the row when the delete actually fails. Tests: - New SessionDataRepositoryTests (real temp-SQLite) cover discovery, initial batch, sampled load, single-timestamp null, per-device frequency, full delete, and the delete-failure-rethrows guard. Existing #572 regression tests moved/renamed. Unit gate: 421/421 green. - New FlaUI scenario ViewLoggedSession_LoadsStoredSessionOntoLoggedDataPlot drives the logged-session viewer end-to-end against real hardware: it loads a stored session onto the plot (via an invisible DisplaySessionButton invoke seam, since the user-facing whole-row trigger is a MouseBinding with no InvokePattern and out-of-process physical clicks are unreliable) and asserts via a new LoggedSessionStatsText UIA hook that DisplayLoggingSession ran. This viewer scenario and the existing delete scenario (which exercises the bug fix) both pass on the attached device. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoExtract SessionDataRepository and propagate session delete failures Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
48 rules 1.
|
Qodo review (#607): the empty-session sentinel was a cached static singleton carrying a mutable Points dictionary, so any accidental mutation of the empty result would leak into every later empty-session load. Return a new instance on each access instead. Today's empty path never mutates Points, but this removes the latent shared-mutable-state footgun. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @qodo-code-review — went through both findings: 2. Mutable empty singleton ( 1. Empty catch in |
That rationale makes sense.
So I’m fine with leaving |
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 39.5%
Daqifi.Desktop.Common - 40.5%
Daqifi.Desktop.IO - 100%
Coverage report generated by ReportGenerator • View full report in build artifacts |
Summary
Extracts the session read/delete path out of the 1,721-line
DatabaseLoggergod class into a testableSessionDataRepository(the next #592 extraction), and fixes the latent delete-failure-undetected data-integrity bug the extraction unblocks.DatabaseLogger.cs: 1,721 → 1,376 lines.Extraction (behavior-preserving)
SessionDataRepositoryowns the EF/ADO read path — channel discovery + the fast initial batch (LoadInitialSession), the full-range sampled load (LoadSampledData), the single-timestamp value spread, per-device frequency, and the transactional delete. Constructor-injected with onlyIDbContextFactory<LoggingContext>+IAppLogger, so it's unit-testable with no WPF/OxyPlot-engine dependency — theSessionSampleWriterplaybook applied to the read side.DisplayLoggingSessionbecomes a thin orchestrator that builds the plot series/legend around the points the repository returns;AddChannelSeriesno longer seeds the point dictionary (the repository owns that).internal statictest seams (DeduplicateChannelInfo,LoadSingleTickValueSpread) move with the read path; their System.ArgumentException: An item with the same key has already been added. Key: (9090684023231015079, AI0) #572 regression tests are renamed and repointed.Bug fix — delete failure undetected
SessionDataRepository.DeleteSessionnow logs and rethrows on failure instead of swallowing. PreviouslyDeleteLoggingSessionlogged-and-swallowed, soLoggingSessionListViewModelremoved the bound row even when the SQL delete failed — the row vanished from the UI while its data remained, reappearing on the next reload. With the rethrow, the view model's existing delete gating keeps the row when the delete actually fails.Tests
Unit (421/421 green):
SessionDataRepositoryTestsagainst real temp-SQLite: discovery, initial batch, sampled load, single-timestamp null, per-device frequency, full delete, and the delete-failure-rethrows guard.SessionDataRepository*.Integration (FlaUI, run against the attached device — both pass):
StartLoggingSession_…RunsStopsAndDeletesSessionexercises the delete bug fix end-to-end (DB-level delete proven from the app's log lines).ViewLoggedSession_LoadsStoredSessionOntoLoggedDataPlotdrives the logged-session viewer end-to-end: loads a stored session onto the plot and asserts via a newLoggedSessionStatsTextUIA hook thatDisplayLoggingSession(→SessionDataRepository) ran. Because the whole-row "view" trigger is aMouseBindingwith no InvokePattern and out-of-process physical clicks are unreliable, the row carries an invisibleDisplaySessionButtoninvoke-seam bound to the same command.Notes
LoggedDataPanePrototype.xaml(an invisible read hook + an invisible invoke seam), both invisible/non-interactive and documented inline, following the existingPlotStatsTextpattern.CLAUDE.md's OxyPlot/ADR-001 plot-gotchas key files are unchanged — the viewport/plot machinery did not move in this PR.🤖 Generated with Claude Code