[tylerkron]

Not merging — for review.

Closes #206. Ports daqifi-desktop#691.

What this is

This removes dead weight and a latent trap. It changes nothing a user can see.

Daqifi.Desktop.Channel.Channel is an EF entity that was never constructed, had no DbSet, and whose table has no writer and no reader. It existed only to give LoggingSession.Channels a type, and that navigation property has no reader either. Its SetColor is genuinely broken — color = ChannelColorBrush;, writing the by-value parameter and discarding the value — but it has zero call sites in production, tests or .axaml, and no reflective access. #206 has the full proof, including an executed contract test that fails on this implementor and passes on the other.

Channel colour works correctly today and is untouched by this. The live path is ChannelsPaneViewModel.SetColor, bound from ChannelsPanePrototype.axaml and ChannelsMobileView.axaml as SetColorCommand, and it assigns SelectedChannel.ChannelColorBrush directly. That property and every XAML binding to it survive verbatim. There is no screenshot here on purpose: nothing renders differently, so a picture of unchanged UI would be a cost rather than a courtesy.

What got smaller

git diff --numstat origin/main...HEAD — 10 files, +552 / −255. Split by what it is:

	files	+	−
The deletion	7	4	255
Migration (DropChannelTable + designer)	2	288	0
Tests	1	260	0

The deletion half, in detail:

file	+	−
Channel/Channel.cs (deleted)	0	120
Migrations/LoggingContextModelSnapshot.cs	0	100
Configuration/IColorable.cs (deleted)	0	16
Channel/AbstractChannel.cs	0	8
Loggers/LoggingContext.cs	0	6
Loggers/LoggingSession.cs	0	3
Channel/IChannel.cs	4	2

Two whole types go, and one layer with them: IColorable is a one-member interface every channel was forced to carry. Upstream had to keep it, because their test doubles implement IChannel by hand and so must implement SetColor; this repo's fake (LoggingManagerSubscribedChannelsTests.TestChannel) derives from AbstractChannel, so nothing here needs the member at all. IChannel stops deriving from anything, and AbstractChannel.SetColor — the correct implementation, also with zero callers — goes too.

The +4 on IChannel.cs is a comment saying why this repo diverges from upstream at that line.

The database change, plainly

This is the part with real consequences, so here is what happens to each shape of database a user can have. The table is always empty in anything this app or the upstream WPF app produced — nothing has ever written a row — so no logged data is at stake either way. Sessions, samples and device metadata are untouched.

• A database that has the table (the normal case): the table is dropped, along with its two indexes, which SQLite removes with it.
• A database that does not have the table — hand-modified, restored from a partial dump, or baselined out of EnsureCreated() by DatabaseMigrator.SeedMigrationHistoryIfNeeded against a model that no longer declares it: the IF EXISTS guard makes this a no-op. An unguarded migrationBuilder.DropTable("Channel") would throw here, and the throw travels out of startup, leaving the user unable to reach any of their logs. Upstream added this guard in response to their own review; it is the single most important line in the migration, and it is pinned by a test.
• A database interrupted mid-migration: Migrate() runs each migration in a transaction and only records it in __EFMigrationsHistory on success, so an interruption rolls this one back and the next launch retries from a database that still has the table. Retrying after a partial success is safe for the same reason the missing-table case is — the statement is idempotent, and that is pinned too. DatabaseMigrator also copies the file to *.migration-backup before migrating and restores it if migration throws.

What Down() can and cannot do. It recreates the table with the exact column, key, foreign-key and index shape InitialSQLiteMigration gave it, so an older build finds the schema it expects and starts. It does not restore rows — DROP TABLE is not reversible and nothing retains them. In practice that loses nothing, because the table has no writer in any released version, but the down migration is a schema rollback and should not be read as a data rollback. The test asserts the restored table is empty, which is the honest half of that claim rather than an oversight.

Down() is deliberately unguarded where Up() is guarded: Up has to tolerate databases it did not create, while Down only ever runs against a database this migration has already dropped the table from, so a Channel table standing there means something else made it and silently merging with it would be worse than failing.

Deliberately narrower than upstream

Upstream generated their migration with dotnet ef migrations add, which diffed the whole model and swept in an unrelated rename of the Samples composite index (IX_Samples_LoggingSessionID_TimestampTicks → IX_Samples_SessionTime) plus a drop of IX_Samples_LoggingSessionID. That drift exists in this repo too — LoggingContext asks for IX_Samples_SessionTime while AddSamplesSessionTimeIndex creates the longer name — and it is left alone here. Renaming an index on a user's database has its own risk profile and deserves its own justification; it does not belong inside a dead-code deletion. The model snapshot is therefore hand-edited to remove only the Channel entity, which leaves the pre-existing drift exactly as visible, and as suppressed via RelationalEventId.PendingModelChangesWarning, as it is on main today.

The migration and its designer are hand-written for a mechanical reason: dotnet ef shells out to the dotnet on PATH, which cannot satisfy this repo's pinned global.json SDK, so the tool cannot run here at all. The shape is verified by execution instead of by generation — see below.

Tests

Daqifi.Avalonia.Tests/Loggers/DropChannelTableMigrationTests.cs, 5 tests, driving EF's own IMigrator over the real migration chain against real SQLite files:

1. Fresh database — the whole chain applies, no Channel table survives, nothing left pending.
2. A database that has the table with rows in it — table gone, session and sample rows intact.
3. A database at the pre-drop migration whose table is already gone — the migration is a no-op instead of a failure. This is the IF EXISTS guard's test.
4. Rolling back restores the table's exact columns and both indexes, and restores it empty.
5. Applying the drop repeatedly is idempotent.

These also cover something a hand-written migration can get wrong invisibly, and which no compile-time check would catch: EF builds the target model of every migration it applies, including the designer snapshots of older migrations, which still name the now-deleted Daqifi.Desktop.Channel.Channel type. If those could no longer be built, a fresh install would fail on first launch with a green build behind it.

Verification

This branch has been rebased onto 5e4e1a3, current origin/main. It was originally cut at cd44a11, six commits behind (#138, #203, #204, #205, #209, #211). The rebase was clean, and git diff --numstat origin/main...HEAD is byte-identical before and after it — same 10 files, same +552/−255 — so the rebase moved the base without altering the change. Everything below is measured on the rebased branch, which now is the merged state rather than a projection of it.

The rebase was not cosmetic. It matters for two independent reasons:

1. It puts this PR on the current CI pipeline. ci: prove the macos-arm64 baseline on a second machine, and keep it proved #204 added a macOS step, "Committed baseline must be reproduced on this runner", that landed on main after this branch was cut. While this PR sat on cd44a11 that step was not failing — it was absent from the job entirely, so the PR's green tick was earned on a pipeline that no longer exists. On the rebased head it is present and passing (step 10 of Desktop head on macOS).
2. It compiles this deletion against the code that merged after it. A deletion of public members can be broken by code that merged after it branched without ever touching the same file — PR fix(layering): a view-model was hand-writing SCPI bytes at the transport #189 sat MERGEABLE/CLEAN with zero file overlap and still failed the merge queue with CS0115. Rebasing turns that class of breakage into a local compile error instead of a merge-queue ejection.

• dotnet build Daqifi.Avalonia.Desktop on the rebased branch — succeeded.
• dotnet restore + dotnet build Daqifi.Avalonia.iOS -c Release -r iossimulator-arm64 --no-restore on the rebased branch — succeeded. (Android is not buildable on this machine; CI covers it, and the Android head passes on this PR. No locally-built Android status is claimed.)
• dotnet test on the rebased branch — 391 passed, 0 failed, against 386 on 5e4e1a3: exactly the 5 new tests, no collisions with the 42 DiskSpaceMonitor tests test(disk-space): pin DiskSpaceMonitor, the gate on whether a session starts and whether one gets killed #211 added after this branched.
• packages.lock.json unchanged everywhere — no -r on any restore.
• XAML swept by hand, because bindings are not compile-checked and a deleted member fails silently at runtime with both heads green: every .axaml grepped for Channel, SetColor, IColorable, Channels and every removed member. The only SetColor hits are the view-model's SetColorCommand (ChannelsPanePrototype.axaml:624, ChannelsMobileView.axaml:396) — untouched. All six colour bindings resolve to ChannelColorBrush, which stays on IChannel. No .axaml references the Daqifi.Desktop.Channel namespace, nothing binds LoggingSession.Channels, there is no Include(s => s.Channels), and no raw SQL against the Channel table.
• Warnings, measured on both sides with the same command after clearing third_party/oxyplot-avalonia/obj on both, comparing the distinct first-party set (third_party/ excluded): 175 on 5e4e1a3 → 167 on the rebased branch, −8. Every one of the eight is a CS8618 in the deleted Channel.cs. Three LoggingSession.cs warnings shifted three lines up because a property was removed above them; same codes, same members, so the set is otherwise identical. Counted at file+line granularity instead the same measurement reads 222 → 214 — the same −8, the same eight warnings.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Remove vestigial Channel EF entity and database table

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Removes the unused Channel persistence entity, navigation, and IColorable contract.
• Drops the vestigial Channel table safely when present.
• Verifies fresh installs, upgrades, missing tables, retries, and rollback against SQLite.

Diagram

graph TD
  A["Application startup"] --> B["EF migrator"] --> C["Guarded drop"] -->|"drop if present"| D[("SQLite logs")]
  E["Current EF model"] --> B
  F["Migration tests"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use EF DropTable

• ➕ Uses the conventional provider-generated migration operation.
• ➕ Requires less custom SQL.
• ➖ Emits an unguarded drop that fails when the table is already absent.
• ➖ Could prevent users from opening existing logs after startup migration failure.

2. Retain deprecated persistence types

• ➕ Avoids changing existing database schemas.
• ➕ Eliminates migration and rollback risk.
• ➖ Preserves an unused entity, table, navigation, and interface indefinitely.
• ➖ Leaves a broken SetColor implementation as a maintenance trap.
• ➖ Provides no compatibility benefit because the table has no reader or writer.

Recommendation: Keep the PR's guarded raw-SQL migration and remove the dead persistence layer. It safely handles heterogeneous legacy databases, preserves all active logging and color behavior, and the real-SQLite tests mitigate the additional risk of a hand-authored migration and designer.

Files changed (8) +552 / -119

Refactor (4) +4 / -19

AbstractChannel.csRemove the unused channel color setter +0/-8

Remove the unused channel color setter

• Removes the uncalled SetColor implementation inherited through IColorable. Direct ChannelColorBrush assignment remains available for the active UI color path.

Daqifi.Avalonia/Daqifi.Desktop/Channel/AbstractChannel.cs

IChannel.csDetach IChannel from the removed IColorable contract +4/-2

Detach IChannel from the removed IColorable contract

• Stops IChannel from inheriting IColorable and documents why this port intentionally differs from upstream. The existing ChannelColorBrush property remains unchanged.

Daqifi.Avalonia/Daqifi.Desktop/Channel/IChannel.cs

LoggingContext.csRemove the vestigial Channel entity mapping +0/-6

Remove the vestigial Channel entity mapping

• Deletes the explicit EF mapping for the obsolete Channel entity and table. Logging sessions, samples, and device metadata remain in the active model.

Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggingContext.cs

LoggingSession.csRemove the unused persisted Channels navigation +0/-3

Remove the unused persisted Channels navigation

• Removes the never-populated LoggingSession.Channels collection that was the obsolete entity's only model entry point.

Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggingSession.cs

Tests (1) +260 / -0

DropChannelTableMigrationTests.csExercise Channel table migration against real SQLite databases +260/-0

Exercise Channel table migration against real SQLite databases

• Adds integration tests covering fresh migration chains, upgrades with preserved logging data, databases where the table is already absent, repeated application, and rollback schema restoration. The tests also validate that historical migration target models remain buildable after deleting the CLR entity.

Daqifi.Avalonia.Tests/Loggers/DropChannelTableMigrationTests.cs

Other (3) +288 / -100

20260902230000_DropChannelTable.Designer.csDefine the post-removal migration target model +149/-0

Define the post-removal migration target model

• Adds the migration designer model containing sessions, samples, and device metadata without the Channel entity, relationships, or navigation. It deliberately retains the pre-existing Samples index-name drift.

Daqifi.Avalonia/Daqifi.Desktop/Migrations/20260902230000_DropChannelTable.Designer.cs

20260902230000_DropChannelTable.csSafely drop the obsolete Channel table +139/-0

Safely drop the obsolete Channel table

• Adds an idempotent SQLite migration using DROP TABLE IF EXISTS so databases without the table do not fail at startup. The down migration reconstructs the original table, foreign keys, and indexes while explicitly not restoring dropped rows.

Daqifi.Avalonia/Daqifi.Desktop/Migrations/20260902230000_DropChannelTable.cs

LoggingContextModelSnapshot.csRemove Channel persistence metadata from the model snapshot +0/-100

Remove Channel persistence metadata from the model snapshot

• Deletes the Channel entity definition, its sample and session relationships, and the LoggingSession.Channels navigation from the current EF snapshot.

Daqifi.Avalonia/Daqifi.Desktop/Migrations/LoggingContextModelSnapshot.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 922b742 ⚖️ Balanced

Results up to commit 1f4d9a3 ⚖️ Balanced

No changes from previous review

[tylerkron]

Qodo-clean, CI green — ready for review. Qodo's review is anchored to 1f4d9a3 with 0 bugs / 0 rule violations / 0 requirement gaps, no unresolved inline threads, and it held through a settle re-check. main moved six commits since this branched (#138, #203, #204, #205, #209, #211), so verification was redone on a test-merge onto 5e4e1a3 rather than on the branch alone: conflict-free, desktop + iOS both build, 391 tests pass against 386 on main (exactly the 5 new ones), warnings 175 → 167. Body updated to match. Not merging.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 922b742

[tylerkron]

Rebased onto 5e4e1a3 (current main) and re-verified — this PR now runs the current pipeline, which was the one thing its previous green tick could not claim.

It had branched at cd44a11, before #204, so the macOS "Committed baseline must be reproduced on this runner" step was not failing on it — it was absent from the job entirely. On the rebased head that step is present, ran, and passed: all 18 screens match tools/parity-audit/baselines/macos-arm64.sha256 (baseline OK: every screen … matches, on macOS 26.5.2 arm64, runner image macos26/20260728.0273.1).

• Rebase clean, no conflicts. git diff --numstat origin/main...HEAD is byte-identical before and after — same 10 files, same +552/−255 — so the base moved and the change did not.
• CI green on 922b742: all six checks, Android included.
• Qodo re-run on the new head (the rebase invalidated the old round): 0 bugs / 0 rule violations / 0 requirement gaps, 0 inline threads, marker anchored to 922b742, not a skipped review — and it held through a settle re-check on both surfaces.
• Rebuilt, not just rebased: desktop + iOS both green; 391 tests pass against 386 on main; warnings 175 → 167; no packages.lock.json churn.
• XAML re-swept on the rebased tree: all six colour bindings still resolve to ChannelColorBrush, no .axaml touches IColorable or the Channel namespace.

Body updated to match. Not merging.