Skip to content

fix(#327): LoggedDataPanePrototype's 102 bindings were all on reflection — root scope, two declared template scopes, two existing item scopes - #396

Merged
tylerkron merged 3 commits into
mainfrom
issue-327-loggeddatapaneprototype
Sep 19, 2026
Merged

tylerkron merged 3 commits into
mainfrom
issue-327-loggeddatapaneprototype

Conversation

@tylerkron

@tylerkron tylerkron commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

refs #327 — not merging, for review.

What was wrong

LoggedDataPanePrototype.axaml is the desktop Logged Data tab. It declared neither x:DataType nor x:CompileBindings, and AvaloniaUseCompiledBindingsByDefault is false. So all 102 of its bindings were resolved by reflection. That count comes from parsing the file: 102 {Binding}s, no MultiBinding, plus 16 TemplateBindings that are not counted. A renamed or deleted view-model member would render blank and every head would still build green. For the 25 IsVisible targets it is worse: the control stays at its default of true. That includes the session-settings drawer and the confirm dialog, so a typo in either would pin that surface open over the pane. The loading overlay is the exception, because its FallbackValue=False would hide it.

Was main broken at runtime? No. I found no dead binding. Every member name compiles on this branch unchanged, and no binding path was edited. This PR is protection against future breakage. It does not fix a bug.

The fix

The root gets x:DataType="vm:DaqifiViewModel" + x:CompileBindings="True". I read the host code but did not observe it at runtime:

  • The pane's only host is MainWindow's Logged Data tab.
  • That tab's ContentControl Content="{Binding}" hands the pane the window's DaqifiViewModel. MainWindowBindingTests already pins this.
  • The code-behind has no OnLoaded and swaps in no view model. It only reads DataContext as DaqifiViewModel back in OnSessionNameChanged.
  • Nothing else constructs the pane.

The file has five scopes:

scope declaration why
root vm:DaqifiViewModel (new) the host above
OxyPlot tracker ControlTemplate oxycore:TrackerHitResult (new) PlotBase.ShowTracker assigns a TrackerHitResult. Same as #386
session list ListBoxItem ControlTheme logger:LoggingSession (new) a ListBoxItem's DataContext is its item from LoggingSessions. One declaration covers the theme's AutomationProperties.Name setter and the row ControlTemplate under it
legend group DataTemplate DataType="{x:Type logger:DeviceLegendGroup}" (existing, untouched)
legend item DataTemplate DataType="{x:Type logger:LoggedSeriesLegendItem}" (existing, untouched)

Both new nested declarations are required, per rule (e): a ControlTemplate is not a new scope. I measured this by removing both declarations, which gave 13 AVLN2000 errors, all on DaqifiViewModel:

  • 3 from the tracker template.
  • 10 from the ListBoxItem theme: the setter's AccessibilitySummary plus 9 row members. The row's other 8 bindings are four bare {Binding} and four $parent[UserControl].DataContext.* commands. They compile under either scope.

The five $parent[UserControl].DataContext.* commands (four on the row, one in the legend) are unchanged. $parent is the same logical-tree hop under compiled bindings, and the compiler resolves the hop against the root's DaqifiViewModel (see the control below). No binding was rewritten. The file opens no escape hatch.

Verification

One-build positive control on both sides.

  • origin/main: one compilation carrying two binding typos (IsLoggingSessionSettingsOpenZZZ at the root, SampleCountDisplayZZZ in the row template) plus a bogus BogusPropZZZ="1" on the same Grid as the first typo. It produced 1 error, only BogusPropZZZ at line 896. Both dead bindings were silent in the same compilation.
  • This branch: one compilation carrying nine typos, one per scope or shape. It produced 9 errors, each on the intended type:
typo reported on
IsLoggingSessionSettingsOpenZZZ DaqifiViewModel
DbLogger.PlotModelZZZ DatabaseLogger
TextZZZ (tracker) OxyPlot.TrackerHitResult
AccessibilitySummaryZZZ (theme setter) LoggingSession
SampleCountDisplayZZZ (row template) LoggingSession
$parent[UserControl].DataContext.DisplayLoggingSessionCommandZZZ (row) DaqifiViewModel
$parent[UserControl].DataContext.ToggleLoggedSeriesVisibilityCommandZZZ (legend) DaqifiViewModel
TruncatedSerialNoZZZ DeviceLegendGroup
ChannelNameZZZ LoggedSeriesLegendItem

Runtime: the populated pane, observed. HeadlessBench --scripted session-10h seeds a throwaway DAQIFI_DATA_DIR with one 108 000-sample session and opens it in the pane:

  • LOGGED-PLOT/limits passed.
  • I added a temporary probe to the rig and reverted it; it is not in this diff. It read the realised controls:
    • DisplaySessionButton, ExportSessionButton and DeleteSessionButton each hold the shell's own command instance, with the row's LoggingSession as CommandParameter.
    • The legend button holds ToggleLoggedSeriesVisibilityCommand with its LoggedSeriesLegendItem as parameter.
    • The ListBoxItem's AutomationProperties.Name equals session.AccessibilitySummary.
    • LoggedSessionStatsText reads 108000.
    • SessionList.SelectedItem is the session, so the TwoWay binding holds.

The screenshot shows the header strip, the legend and the selected row (attached).

Capture: tools/parity-audit/run.sh --check-baseline passed 39/39. The committed macos-arm64.sha256 is 39 lines, and the run included desktop-2-loggeddata and desktop-min-2-loggeddata (the empty-state pane). I ran it alone, with no other capture running.

Test: LoggedDataPanePrototypeBindingTests is new (+62 lines, 4 test cases). BindingFacts.cs is untouched because open PR #387 edits it. The test checks:

  • the root declarations, via AssertRootDeclares (parsed);
  • AssertNoEscapeHatch;
  • the nested scopes. After Qodo round 1, this is a parse inside the test itself: it asserts the exact set of non-root x:DataType declarations as parent/element[TargetType] type.

Controls, each on a fresh edit and reverted:

  • On the first version: dropping the root x:CompileBindings and repointing the ListBoxItem scope failed the two tests that pin those changes.
  • On the parsed version: the scope repointed with the original tag kept verbatim in a comment fails. The old substring form would have passed, because the literal is still in the file. A third x:DataType added to the legend DataTemplate also fails.

Gate: the desktop build, iOS restore, and iOS Release iossimulator-arm64 --no-restore build all exited 0. dotnet test at the final head 8f4f29e: 997/997. That is one fewer than the first commit's 998 because two theory cases became one fact. No lock-file changes. I did not build Android locally; CI covers it.

Warnings: derived from CI's own logs at both ends, with no local rebuild. main at the merge base 642caef (run 35377977069) vs this branch at 8f4f29e (run 35398429908):

  • The all-project totals are identical on every head: desktop 169, iOS 170, Android 175.
  • The first-party warning set, de-duplicated with third_party/ excluded, is identical on every job: 0 added, 0 removed. That is 149 distinct on desktop and iOS, 151 on the library job and 153 on Android.
  • 169 = 149 first-party + 20 vendored OxyPlot.

Diff: 2 files, +90/−3. The view is +28/−3, of which +21 is the explanatory comment; the test is +62.

CI is green on 8f4f29e, the current head. Qodo:

  • Round 1 (Balanced, 025a1f7) raised 1 finding, conceded and fixed.
  • Round 2 (Fast, 8f4f29e) raised 0 findings and settled clean on both surfaces.
  • Round 3, on the same unchanged head, was requested at 21:58:58Z and fired at 21:59:31Z. It raised 0 findings and reported "No changes from previous review", so it adds no grade beyond Fast. I read the 8f4f29e delta by hand: it is test-only.
  • Round 4 (shepherd, same unchanged head), requested at 22:19:57Z, fired at 22:20:41Z (Fast, 0 new findings). It settled 3.5 min later with the summary unchanged and 0 unresolved qodo threads. The shepherd hand-reviewed the parsed check and ran two mutations against it, each on a fresh edit: a repointed ListBoxItem scope with the old tag kept in a comment fails the nested-scope test, and unparseable markup fails all 4 tests with XmlException. Neither case skips.

9-way merge of main 642caef with #387, #389, #390, #391, #392, #393, #394, #395 and #396: a clean octopus merge, with the same tree 94b85fe in forward and reverse order. Desktop and iOS Release builds exit 0 with 0 errors, no lock-file changes, and 1018/1018 tests, which reconciles exactly as 993 + 0 (#387) + 3 × 7 (#389–#395) + 4 (#396). #392 and #394 add the same IChannel.ScaleExpressionError in a byte-identical blob, so that file merges cleanly.

🤖 Generated with Claude Code

Logged Data pane on this branch with a seeded ten-hour session: header strip, legend and selected row all bound

tylerkron and others added 2 commits September 18, 2026 11:33
…ion — root scope, two declared template scopes, two existing item scopes

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

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Compile bindings in LoggedDataPanePrototype

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Compile all 102 Logged Data pane bindings against explicit root and nested data scopes.
• Add regression tests preserving compiler opt-in, scope declarations, and escape-hatch prohibition.
Diagram

graph TD
  Host["MainWindow Tab"] --> Pane["Logged Data Pane"] --> Root["Root Data Type"] --> Compiler["XAML Compiler"]
  Pane --> Tracker["Tracker Scope"] --> Compiler
  Pane --> Session["Session Scope"] --> Compiler
  Tests["Binding Tests"] --> Pane
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Enable compiled bindings project-wide
  • ➕ Removes the need for per-view x:CompileBindings declarations
  • ➕ Provides consistent compile-time validation across all Avalonia views
  • ➖ Expands the change beyond the Logged Data pane
  • ➖ May expose unrelated binding failures requiring a coordinated migration
  • ➖ Makes rollback and review substantially broader

Recommendation: Keep the PR's localized opt-in. Explicit root and nested data types provide compile-time protection for this pane without forcing an application-wide migration; focused declaration tests appropriately guard assumptions the XAML compiler cannot verify against runtime template data contexts.

Files changed (2) +67 / -3

Enhancement (1) +28 / -3
LoggedDataPanePrototype.axamlCompile Logged Data pane bindings with explicit scopes +28/-3

Compile Logged Data pane bindings with explicit scopes

• Enables compiled bindings against DaqifiViewModel at the view root and types the OxyPlot tracker and session item scopes. Existing binding paths and runtime behavior remain unchanged while future invalid member references become build failures.

Daqifi.Avalonia/Daqifi.Desktop/View/Prototype/LoggedDataPanePrototype.axaml

Tests (1) +39 / -0
LoggedDataPanePrototypeBindingTests.csGuard compiled-binding declarations and nested scopes +39/-0

Guard compiled-binding declarations and nested scopes

• Adds tests asserting the pane's root data type and compiled-binding opt-in. It also pins the tracker and session scope declarations and verifies that no reflection-binding escape hatch is introduced.

Daqifi.Avalonia.Tests/Views/LoggedDataPanePrototypeBindingTests.cs

@qodo-code-review

qodo-code-review Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Nested scope tests pass falsely ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The_nested_scopes_name_what_the_runtime_assigns passes each declaration to AssertBinds, which
searches raw source text instead of parsed XML elements. If an old tag remains in a comment while
the live scope is repointed, the test still passes despite no matching active declaration.
Code

Daqifi.Avalonia.Tests/Views/LoggedDataPanePrototypeBindingTests.cs[R32-35]

+    [InlineData("<ControlTemplate x:DataType=\"oxycore:TrackerHitResult\">")]
+    [InlineData("<ControlTheme TargetType=\"ListBoxItem\" x:DataType=\"logger:LoggingSession\">")]
+    public void The_nested_scopes_name_what_the_runtime_assigns(string declaration) =>
+        BindingFacts.AssertBinds(View, declaration);
Relevance

●●● Strong

Recent reviews accept replacing raw-text guards with parsed XML to prevent comment-based false
positives.

PR-#325
PR-#372
PR-#375

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test invokes AssertBinds for declarations, while that helper performs Assert.Contains
against the entire raw source. The same helper file already documents and implements parsed XML
inspection specifically because declaration literals in comments can make substring-based guards
pass falsely.

Daqifi.Avalonia.Tests/Views/LoggedDataPanePrototypeBindingTests.cs[31-35]
Daqifi.Avalonia.Tests/BindingFacts.cs[61-72]
Daqifi.Avalonia.Tests/BindingFacts.cs[74-103]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The nested-scope tests use a raw substring assertion, allowing declaration text inside comments to satisfy the guard even when the active XML declaration was changed.

## Fix Focus Areas
- Daqifi.Avalonia.Tests/Views/LoggedDataPanePrototypeBindingTests.cs[31-35]
- Daqifi.Avalonia.Tests/BindingFacts.cs[62-72]

## Recommended Fix
Add a parsed-XML helper that locates the expected `ControlTemplate` or `ControlTheme` element and verifies its XAML `DataType` attribute and relevant element attributes. Replace the two `AssertBinds` calls with that structural assertion so comments and unrelated text cannot satisfy the tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Cross-repo context — repo relationships
Review mode: 🚀 Fast: This is a localized test-only change that strengthens XML-based assertions in one file without altering runtime behavior or high-risk surfaces.

Grey Divider

Tip of the day
💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

…sed markup, not the raw text

Qodo round 1 on PR #396: a copy of either tag in a comment satisfied the substring guard while the live declaration had been repointed.

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

Copy link
Copy Markdown
Contributor Author

Round 1 (Balanced, 025a1f7): one finding, conceded. The nested-scope guard is now a parsed exact-set check (8f4f29e). Controls are in the thread reply. Gate re-run: desktop and iOS Release exit 0, dotnet test 997/997. The count dropped by one because two theory cases became one fact.

@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 8f4f29e

@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 8f4f29e

@tylerkron

Copy link
Copy Markdown
Contributor Author

Qodo-clean, CI green, ready for review. Head 8f4f29e: CI green on that SHA, and rounds 2 and 3 both clean with a settled re-check (0 unresolved qodo threads). The review grade is Fast on the final test-only delta; the view change itself was graded Balanced in round 1. Not merging.

@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 8f4f29e

@tylerkron

Copy link
Copy Markdown
Contributor Author

Qodo-clean, CI green, ready for review. Not merging.

  • CI: run 35398429908 is green on 8f4f29e, which is the current head. It checked out Merge 8f4f29e into 642caef, today's main. I pushed nothing, so no newer commit exists for CI to have missed.
  • Qodo: shepherd round 4, requested at 22:19:57Z on the unchanged head, fired at 22:20:41Z anchored to 8f4f29e with 0 new findings. It stayed unchanged through a 3.5 min settle, with 0 unresolved qodo threads on both surfaces. The standing grade is Fast, which covers only the test-only delta from round 1's fix. The view change itself was graded Balanced in round 1.
  • Hand review covering the Fast grade: the parsed nested-scope check is small (one XDocument query and one exact-list assert). It re-parses the file inline because BindingFacts.ViewRoot and XamlNamespace are private. No existing helper states this: AssertTemplateScopedTo covers only DataTemplates and requires every one of them to be declared, which would fail here because the two legend templates use DataType="{x:Type}" rather than x:DataType. So there is a small duplication of the parse call, but not of a helper. I ran two mutations, each a fresh edit: (1) Qodo's comment bypass, with the ListBoxItem scope repointed to vm:DaqifiViewModel and the old tag kept verbatim in a comment, fails The_nested_scopes_name_what_the_runtime_assigns (4 tests ran, 1 failed); (2) unparseable markup fails all 4 tests with XmlException. Neither case is skipped.
  • Scopes re-checked against the code: the root is DaqifiViewModel because the code-behind never assigns a DataContext. It has no OnLoaded, and it only reads DataContext as DaqifiViewModel. It inherits the shell VM from MainWindow's ContentControl Content="{Binding}". fix(#327): ProfilesPane's 76 bindings were all on reflection — one declared scope, five inferred, three $parent hops #393/fix(#327): ChannelsPanePrototype's 81 bindings were all on reflection — three declared templates, and a drawer label bound to a member IChannel never declared #394/fix(#327): DevicesPanePrototype's 83 bindings were all on reflection — one declared scope, one DataType-typed tile template #395 are different: their panes' OnLoaded replaces the DataContext with their own pane VM. The ControlTheme declaration logger:LoggingSession matches DaqifiViewModel.LoggingSessions, which is an ObservableCollection<LoggingSession>.
  • 9-way merge: see the separate comment. Clean, 1018/1018, and the count reconciles.

@tylerkron

Copy link
Copy Markdown
Contributor Author

9-way merge build, 2026-09-18 (sweep via #396's shepherd). I merged main 642caef with #387 259a829, #389 808752a, #390 a7c89ec, #391 4370e80, #392 01764b9, #393 f1599e0, #394 4b13ea8, #395 9a0bfa4 and #396 8f4f29e on a throwaway branch. Nothing was pushed.

@tylerkron

Copy link
Copy Markdown
Contributor Author

Sweep (13-way merge build): all 13 PRs are clean together on main 642caef

Covered: #387 259a829, #389 808752a, #390 a7c89ec, #391 4370e80, #392 01764b9, #393 f1599e0, #394 4b13ea8, #395 9a0bfa4, #396 8f4f29e, #398 8e2bb7e, #399 df20ae1, #400 c810ddf, #401 998da59.

Per PR: every head matches its ready note. Every PR is MERGEABLE/CLEAN, and its Build run on that exact SHA passed. No PR has an unresolved qodo review thread. I validated the thread-author filter against #196 first: it finds 3 qodo threads there, as expected. None of the green runs are stale: the last workflow change on main is 94fcac3 (09-08), older than all of them.

Merge build: I merged 642caef ← 387, 389–396 first, as a checkpoint. It produced tree 94b85fe, the same tree the previous sweep reported, so that result now holds up. Adding #398, #399, #400 and #401 gave tree b0f2033. Merging all 13 in reverse order gives the same b0f2033. Neither order had a text conflict.

  • Desktop build: exit 0.
  • iOS Release build (iossimulator-arm64, --no-restore): exit 0.
  • Tests: 1039/1039 passed. No lock-file drift.

Test count reconciles exactly: 993 on main + #387 0 + #389–#394 +3 each (+18) + #395 +3 + #396 +4 + #398 +13 + #399 0 + #400 +8 + #401 0 = 1039. Each delta comes from that PR's own CI total minus its base's total. For #387 that is 989 − 989 on 25df7b8.

Cross-PR hazards checked:

No merge was performed and nothing was pushed to any PR branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant