009: MVVM Architecture Cleanup — Service Container, Event Bus, Module Registry - #10
Closed
MohamedSom3a-00700 wants to merge 9 commits into
Closed
009: MVVM Architecture Cleanup — Service Container, Event Bus, Module Registry#10MohamedSom3a-00700 wants to merge 9 commits into
MohamedSom3a-00700 wants to merge 9 commits into
Conversation
…rors - Add 'Workspace' ribbon group with 'Shell' button on main tab - Wire btnWorkspace_Click to NavigationService.Instance.NavigateTo() - Register initial shell pages (Home, Comparison, Daily Report, Links Mgr) - Create ShellNavigationHelper for ribbon→shell integration - Fix DynamicResource→StaticResource on Binding.Converter (not a DP) - MainWindow.xaml: ProgressWidthConverter on MultiBinding - AssignTradeCodesWindow.xaml: PercentToScaleConverter on Binding - SubDailyReportWindow.xaml: BoolToVisibilityConverter on Binding - Remove duplicate FocusVisualStyle + HighContrastFocusVisualStyle styles in ButtonStyles.xaml (caused 'Item already added' crash) - Fix AccentSwatchBasedOn StaticResource issue in SettingsWindow.xaml - Register ShellStyles.xaml in ThemeResources.xaml Closes #8 — Navigation Shell Platform
- Ribbon1.cs: idempotent page registration, remove fake pages - ShellWindow.xaml.cs: use NavigationService for sidebar/command nav - SidebarControl.xaml.cs: remove OnItemsChanged body - WorkspaceHost.cs: deduplicate OnNavigationCompleted, stable retry - NavigationService.cs: null guards, pushToHistory param, GoBack fix - ShellNavigationHelper.cs: excelWindowHandle for MessageBox, IsPageRegistered - ShellStyles.xaml: add FocusVisualStyle to sidebar/command palette items - data-model.md: add text language specifier to code blocks - plan.md: remove duplicate NavigationService.cs entry - csproj: remove duplicate RelayCommand.cs; delete the file - ThemeResources.xaml: add ShellStyles to header load-order comment
…, duplicate history
…VM cleanup - ServiceContainer: IServiceContainer, IServiceScope with singleton/transient/scoped lifetimes, circular dependency detection, unregistered service detection, observable diagnostics events - EventBus: IEventBus with typed events, weak reference subscriber storage, subscriber isolation, event filtering, diagnostics events - ModuleRegistry: IModule, IModuleRegistry with priority-based initialization, duplicate detection, module failure isolation - ViewModelBase: base class with INotifyPropertyChanged + SetProperty helper - CompositionRoot: centralized registration point in WpfApp2/CompositionRoot.cs - Relocated ViewModels from Models/ to ViewModels/ with proper namespace - Updated AGENTS.md, MVVM_RULES.md with new architecture patterns - Created MVVM_COMPLIANCE.md checklist document
📝 WalkthroughWalkthroughAdds a DI container, event bus, and module registry with composition root and App startup wiring. Refactors many WPF windows to resolve ViewModels via the container and bind commands. Introduces tests for DI/event bus/ViewModelBase. Updates specs/docs. Adds an OCR image-to-Markdown skill (docs, deps, CLI). Cleans add-in resources/ribbon and adds ARM64 config. ChangesArchitecture cleanup, UI refactors, tests, and OCR tool
Sequence Diagram(s)sequenceDiagram
participant App
participant CompositionRoot
participant ServiceContainer
participant ModuleRegistry
App->>CompositionRoot: RegisterServices(container)
CompositionRoot->>ServiceContainer: Register(EventBus, ModuleRegistry, services, VMs)
App->>ServiceContainer: Resolve(IModuleRegistry)
App->>ModuleRegistry: InitializeAll()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
specs/009-mvvm-architecture-cleanup/data-model.md-22-75 (1)
22-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks to satisfy markdown lint.
Three fenced blocks are missing a language tag (
MD040), which can fail documentation linting.Suggested doc fix
-``` +```text Registered → (first resolution) → Instance Created → (subsequent resolution within scope/lifetime) → Same Instance Returned Registered → Resolution Requested → Circular Dependency Detected → Error Reported (no instance returned) Registered → Resolution Requested → Service Not Found → Error Reported (no instance returned)@@
-+text
Publisher → Publish(Event) → Event Bus → Subscriber Alive? → Yes → Invoke Handler
→ Subscriber Alive? → No → Prune Subscription (silent)
→ No Subscribers → Complete (no-op)@@ -``` +```text Registered → Initialize() → Services Registered → Event Subscriptions Active → Module Active Module Active → Application Shutdown → (implicit cleanup — no explicit shutdown required)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@specs/009-mvvm-architecture-cleanup/data-model.mdaround lines 22 - 75, The
three fenced code blocks in the document (the service resolution/state
transitions block, the Event Bus state transitions block, and the Module state
transitions block) are missing language identifiers and trigger MD040; update
each opening triple-backtick to include a language tag (e.g., changetotext) so the blocks becometext ...and satisfy the markdown linter
while preserving the exact block content.</details> </blockquote></details> <details> <summary>specs/007-control-standardization/tasks.md-45-45 (1)</summary><blockquote> `45-45`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove spaces inside the inline code span.** Line 45 currently violates markdownlint MD038 (`<Style ` has a trailing space inside backticks). <details> <summary>Suggested fix</summary> ```diff -- [X] T007 [P] Run grep audit for inline styles `<Style ` inside window files at `WpfApp2/Views/*.xaml` — document which windows have inline styles +- [X] T007 [P] Run grep audit for inline styles `<Style` inside window files at `WpfApp2/Views/*.xaml` — document which windows have inline styles ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/007-control-standardization/tasks.md` at line 45, The inline code span contains a trailing space (`<Style `) which triggers markdownlint MD038; edit the checklist line to remove the trailing space inside the backticks so the span reads `<Style` (i.e., replace `<Style ` with `<Style`) in the tasks entry for T007 and save the markdown. ``` </details> </blockquote></details> <details> <summary>specs/007-control-standardization/data-model.md-113-121 (1)</summary><blockquote> `113-121`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language identifier to the fenced code block.** This block triggers markdownlint MD040 and may fail docs linting. <details> <summary>Suggested fix</summary> ```diff -``` +```text Normal ──→ MouseOver ──→ Pressed ──→ Selected (for list controls) │ │ └──→ Focused │ │ │ └──→ Disabled │ │ KeyboardFocus ──────────────────────────────────────────┘ ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@specs/007-control-standardization/data-model.mdaround lines 113 - 121, The
fenced code block showing the control state diagram is missing a language
identifier (causing markdownlint MD040); update the opening fence from ``` toSelected (for list controls)" is annotated as plain text, and keep the closing ``` unchanged..opencode/skills/image-to-md/SKILL.md-66-71 (1)
66-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
Line 66 uses an unlabeled fence, which violates markdown linting and weakens syntax rendering in some viewers.
Proposed fix
-``` +```text Object reference not set to an instance of an object at MainWindow.xaml.cs:line 55 at App.OnStartup()</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In @.opencode/skills/image-to-md/SKILL.md around lines 66 - 71, The fenced code
block containing the exception trace ("Object reference not set to an instance
of an object" followed by the stack lines including MainWindow.xaml.cs and
App.OnStartup) is missing a language identifier; update that fence in SKILL.md
to include a language tag (for exampletext orconsole) so the block
becomestext ...to satisfy markdown linting and improve syntax
rendering.</details> </blockquote></details> <details> <summary>.opencode/skills/image-to-md/scripts/image_to_md.py-249-251 (1)</summary><blockquote> `249-251`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Rename ambiguous loop variable in character count aggregation.** Line 250 uses `l`, which is ambiguous and flagged by Ruff (`E741`). <details> <summary>Proposed fix</summary> ```diff - sum(len(l) for l in lines), + sum(len(line) for line in lines), ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.opencode/skills/image-to-md/scripts/image_to_md.py around lines 249 - 251, The generator uses an ambiguous loop variable `l` in the char-count aggregation (sum(len(l) for l in lines)) which triggers Ruff E741; change the generator to use a clearer name like `line` (e.g., sum(len(line) for line in lines)) so it no longer uses a single-letter shadowed variable; update the occurrence in the logging call that formats "pytesseract succeeded — %d chars, avg confidence ..." to use the new variable name in the generator expression and run linters to confirm the E741 warning is resolved. ``` </details> </blockquote></details> <details> <summary>WpfApp2/ViewModels/SubDailyReportViewModel.cs-184-213 (1)</summary><blockquote> `184-213`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`CanPreview` won't stay in sync with individual checkbox edits.** The button state depends on `NameItems.Any(x => x.IsChecked)`, but `RecalcButtons()` only runs when the list is rebuilt or the bulk-check commands execute. If the user manually unchecks the last item, `PreviewCommand` can stay enabled until some unrelated property changes. Also applies to: 215-232 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/SubDailyReportViewModel.cs` around lines 184 - 213, RefreshNames currently rebuilds NameItems but doesn't subscribe to each NamePickItem's IsChecked changes so RecalcButtons (and thus CanPreview) won't update when a user toggles a single checkbox; fix by wiring a PropertyChanged handler for IsChecked on each NamePickItem as you add them in RefreshNames (and unsubscribe/clear handlers when clearing NameItems) so the handler calls RecalcButtons whenever IsChecked changes; apply the same subscribe/unsubscribe pattern to the other list-populating method around lines 215-232 as well to keep button state in sync. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>specs/009-mvvm-architecture-cleanup/tasks.md (1)</summary><blockquote> `22-23`: _⚡ Quick win_ **Standardize composition-root references to avoid startup wiring drift.** These sections alternate between `App.xaml.cs` as composition root and introducing `CompositionRoot.cs`. Keep docs consistent: `CompositionRoot.cs` owns registrations/modules, `App.xaml.cs` only invokes startup (`RegisterServices()` / `InitializeModules()` path). Also applies to: 55-56, 77-82, 102-103 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/009-mvvm-architecture-cleanup/tasks.md` around lines 22 - 23, The docs alternate which file is considered the composition root; standardize them so CompositionRoot.cs is the authoritative composition root and App.xaml.cs only invokes startup wiring: update all references (including the occurrences noted around lines ~55-56, ~77-82, ~102-103) to state that CompositionRoot.cs owns registrations/modules and expose methods like RegisterServices() or InitializeModules(), and that App.xaml.cs should call CompositionRoot.RegisterServices()/InitializeModules() during startup rather than containing registration logic itself. ``` </details> </blockquote></details> <details> <summary>specs/009-mvvm-architecture-cleanup/quickstart.md (1)</summary><blockquote> `19-20`: _⚡ Quick win_ **Use a single composition point in the quickstart wording.** This should point to `CompositionRoot.cs` as the registration location, with `App.xaml.cs` only invoking composition startup. “App.xaml.cs or CompositionRoot.cs” weakens the explicit single composition point convention in FR-011. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/009-mvvm-architecture-cleanup/quickstart.md` around lines 19 - 20, Update the quickstart wording to enforce a single composition point: change the step text to explicitly instruct readers to register services in CompositionRoot.cs and state that App.xaml.cs should only call into CompositionRoot to start composition; replace the “App.xaml.cs or CompositionRoot.cs” phrasing with a clear directive naming CompositionRoot.cs as the registration location and mention App.xaml.cs only invokes composition startup (referencing CompositionRoot.cs and App.xaml.cs to locate the code). ``` </details> </blockquote></details> <details> <summary>WpfApp2/XerEditorWindow.xaml.cs (1)</summary><blockquote> `10-10`: _⚡ Quick win_ **Resolve `XerEditorViewModel` via the container instead of `new`.** Line 10 bypasses the centralized DI path introduced in this PR. Prefer resolving the ViewModel from `App.Container` to keep dependency construction consistent. <details> <summary>♻️ Suggested change</summary> ```diff - DataContext = new XerEditorViewModel(); + DataContext = App.Container.Resolve<XerEditorViewModel>(); ``` </details> As per coding guidelines `**/ViewModels/**/*.cs`: All ViewModels must inherit from ViewModelBase, implement INotifyPropertyChanged using SetProperty<T> helper, and inject service dependencies via IServiceContainer in constructor. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/XerEditorWindow.xaml.cs` at line 10, Replace the direct instantiation of XerEditorViewModel with a resolve from the DI container: instead of "new XerEditorViewModel()" use App.Container.Resolve<XerEditorViewModel>() (or the container's equivalent Resolve/ResolveRequired method) when setting DataContext; ensure XerEditorViewModel's constructor follows the ViewModels guideline (inherits ViewModelBase, uses SetProperty<T>, and receives its service dependencies via IServiceContainer) so the container can construct it. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In @.opencode/skills/image-to-md/requirements.txt:
- Around line 1-3: Update the unpinned dependencies in
.opencode/skills/image-to-md/requirements.txt to pinned/ constrained versions:
change markitdown[all] to markitdown[all]==0.1.5, pillow to pillow>=12.2.0,<13,
and pytesseract to pytesseract==0.3.13 so installs are reproducible and include
the known security patch thresholds; edit the requirements.txt entry lines for
markitdown, pillow, and pytesseract accordingly.In @.opencode/skills/image-to-md/scripts/image_to_md.py:
- Around line 232-242: The line-break detection compares data["line_num"][i] (an
int) to a boolean expression, causing incorrect grouping; change the condition
to explicitly check whether this is the first token or the current token's line
number differs from the previous token's line number (e.g., use "if i == 0 or
data['line_num'][i] != data['line_num'][i-1]") so words are started on a new
current_line only when the line number actually changes; keep the existing logic
that appends current_line to lines when switching and ensure
confs.append(float(conf)) remains in the loop.In
@Error/Screenshot2026-05-23 101207.md:
- Line 5: The committed diagnostic contains local absolute paths and a username
in the stack trace; sanitize these before committing by removing or replacing
C:\Users... prefixes and any local usernames. Update the code that generates or
stores this diagnostic (the logging/exception formatting used when
MainWindow.InitializeComponent, MainWindow..ctor or Ribbon1.comparsion_Click
produce XamlParseException) to strip full local paths and emit repo-relative
paths or just filename:linenumber, and mask user directories (e.g., replace
"C:\Users<username>..." with "<USER_HOME>" or ".//...") before
writing to the log/commit. Ensure the sanitizer runs on any captured
Exception.StackTrace used in commit messages or persisted diagnostics.In
@specs/009-mvvm-architecture-cleanup/tasks.md:
- Line 7: Update the “Tests” section (the sentence stating tests are omitted) to
add explicit test tasks that align with the spec’s acceptance model: add
separate verification items for container lifetime/scoping and error handling
(create/tear-down, leak simulations, restart behavior), event-bus isolation and
weak-subscriber pruning (message isolation between modules, subscriber
GC/cleanup behavior under load/failure), and module initialization ordering and
failure isolation (ordered init, partial init rollback, and recovery scenarios);
name these tasks clearly so they map to the spec checkpoints (e.g., “Container
lifetime tests”, “Event bus isolation & pruning tests”, “Module init ordering &
failure isolation tests”) and require acceptance criteria and artifacts for each
(test steps, expected results, and pass/fail conditions).In
@WpfApp2/Services/EventBus.cs:
- Around line 85-96: The SubscriberEntry currently stores a strong Delegate
(Handler) which breaks the weak-subscriber contract and causes leaks and
incorrect removal of static handlers; change SubscriberEntry to store MethodInfo
(for the handler method), a WeakReference for the instance target (or null for
static handlers), and keep an optional direct Delegate only for static handlers;
update the constructor to accept and populate MethodInfo + weak target/direct
static delegate instead of Handler, update the publish logic that inspects
handler.Target and prunes/invokes (the publish/remove code paths referenced) to
reconstruct the delegate via MethodInfo + WeakReference.Target for instance
handlers (and skip pruning static handlers — invoke via stored static delegate),
and ensure dead instance targets are pruned only at publish time by checking
WeakReference.IsAlive before reconstruction and removing entries when dead.In
@WpfApp2/Services/ServiceContainer.cs:
- Around line 222-254: ResolveScoped is creating new objects for
ServiceLifetime.Scoped every time because scope-local caching isn't implemented;
add a Dictionary<Type, object> (or similar) to ServiceScope to act as the scope
cache and ensure RegisterScoped/ResolveScoped use it: in ResolveScoped (and
related resolve paths around CreateInstance and ServiceResolved) check the
current ServiceScope's cache for the requested serviceType and return the cached
instance if present, otherwise CreateInstance, store it in the scope cache,
raise ServiceResolved via ServiceResolutionEventArgs, and return it; ensure
ServiceScope exposes a lookup/insert API so scoped instances are isolated per
scope while singletons still use the global storage and transients always create
new instances.- Line 79: The circular-dependency diagnostic currently uses a HashSet
(_resolutionStack) which loses insertion order and makes the reported dependency
chain non-deterministic; change _resolutionStack to an ordered collection (e.g.,
List or Stack) in the ServiceContainer class, update all uses
(add/push, remove/pop, and Contains checks) to the chosen type, and when
constructing the error message (the code that builds the dependency chain where
it currently iterates _resolutionStack around the code paths referenced) build
the chain by iterating the ordered collection so the thrown
InvalidOperationException contains the deterministic, insertion-order dependency
path.In
@WpfApp2/ViewModels/SubDailyReportViewModel.cs:
- Around line 34-44: When any preview-related input changes (e.g., in the
SelectedPrevFile setter and similarly in SelectedPrevSheet, CountColumn,
StartRow setters), reset the preview state so stale merge results can't be
applied: set _hasPreview = false, clear or null out _merged, and clear
PreviewRows (or reassign to an empty collection), then call OnPropertyChanged/
RecalcButtons as needed; this ensures CanApply becomes false and Apply() cannot
write results from a previous preview.- Around line 17-22: SubDailyReportViewModel currently implements
INotifyPropertyChanged directly, new's SubDlyReportService and mutates backing
fields; change it to inherit ViewModelBase, remove the direct
INotifyPropertyChanged implementation, and accept IServiceContainer (or the
specific service interface) in the constructor to resolve SubDlyReportService
instead of new'ing it; replace direct field updates with calls to SetProperty
for any bindable properties (identify usages around _app, _close, and any
properties updated at lines ~112-129 and ~325-326) so change notifications flow
through ViewModelBase and dependency resolution is container-based.- Around line 242-255: The PreviewAsync method currently calls
_svc.ReadPrevDictFromRange and _svc.ReadTodayFiltered inside Task.Run while
passing STA COM objects (_app, wsCur), which causes COM apartment violations;
change PreviewAsync to perform all Excel COM reads on the UI/STA thread (call
_svc.ReadPrevDictFromRange and _svc.ReadTodayFiltered directly without Task.Run,
or extract raw cell values from wb/wsCur on STA) and only wrap purely in-memory
processing (e.g., merging dictionaries, filtering, building preview objects) in
Task.Run; specifically update calls to _svc.ReadPrevDictFromRange(_app,
SelectedPrevFile!.FullPath, SelectedPrevSheet!, out _prevTopLeftAddr, out
_prevTotal) and _svc.ReadTodayFiltered(wsCur, NameCol!, NameStartRow, CountCol!,
CountStartRow, selectedKeys) so they run on STA, then pass the extracted
primitive data to any background Task.Run work.In
@WpfApp2/ViewModels/UnmergeFillDownViewModel.cs:
- Around line 13-17: UnmergeFillDownViewModel should inherit ViewModelBase
instead of implementing INotifyPropertyChanged directly and must accept an
IServiceContainer in its constructor so service dependencies are injected rather
than created inline; replace the private field new UnmergeFillDownService()
(_service) with a resolved instance from the container (e.g.,
container.Resolve/GetService for UnmergeFillDownService) and update any property
setters to use the ViewModelBase.SetProperty helper; also change the
constructor signature to receive IServiceContainer along with
Excel.Application/_app and Action/_close and assign members from injected values
instead of newing services.- Around line 31-35: The HeaderRow property setter currently enforces a minimum
of 1 but the execution logic still hardcodes startRow: 2, so the user-selected
header row is ignored; locate the call that passes startRow: 2 (the operation
method that performs the unmerge/fill-down) and replace the literal 2 with the
HeaderRow property (or a computed value based on HeaderRow if the operation
expects a 0-based index), e.g. use startRow: HeaderRow (or startRow: HeaderRow -
1 if needed), ensuring any downstream indexing expectations are preserved and
keeping ReloadColumns()/OnPropertyChanged() behavior intact.- Around line 144-151: The code captures Excel.Worksheet via _app.ActiveSheet
then calls _service.UnmergeAndFillDownColumn(ws, ...) inside Task.Run which
moves COM calls off the STA UI thread; instead ensure all Excel interop runs on
the UI/STA thread (or a dedicated STA thread) and only offload non-COM work or
progress reporting to background threads: reorder so UnmergeFillDownViewModel
invokes _service.UnmergeAndFillDownColumn on the dispatcher (or create and run a
dedicated STA Thread for Excel and marshal calls there), keep Progress
to marshal ProgressPercent updates back to the UI, and continue to pass
_cts.Token for cancellation but do not use ws or any Range/Cells/UnMerge calls
from ThreadPool threads.In
@WpfApp2/ViewModels/XerEditorViewModel.cs:
- Around line 12-30: The ViewModel currently constructs dependencies and doesn't
follow the MVVM/DI contract: change class XerEditorViewModel to inherit from
ViewModelBase, remove direct construction of XerParser and instead accept
IServiceContainer (or the specific service interfaces) via the constructor and
resolve _parser from the container, convert public properties (Tables,
_filePath) to use backing fields with SetProperty to raise
INotifyPropertyChanged, and change command initialization to use injected
services/command factory from the container (keep methods Load, ExportExcel,
ImportFromExcel, ExportXer but ensure they use the injected _parser and any
other services); update the constructor signature to accept IServiceContainer
(or required service interfaces) and assign commands using the resolved services
rather than newing objects inside the ViewModel.- Around line 212-230: The loop updates parser tables while calling
RefreshTablesUI() inside the iteration which rebuilds/clears Tables and
invalidates the enumerator and per-item references; fix by first collecting the
selected table names (from Tables.Where(x => x.IsSelected)), then for each name
call excel.ReadTable(name) and update _parser.Tables (remove existing via
_parser.Tables.FirstOrDefault(x => x.Name == name) and add updated) and track
count/status values in a temp list/structure, and only after the loop call
RefreshTablesUI() once and then apply the per-table Count/Status updates back to
the refreshed Tables collection; references: Tables, RefreshTablesUI(),
excel.ReadTable(), _parser.Tables, and the existing variable t.
Minor comments:
In @.opencode/skills/image-to-md/scripts/image_to_md.py:
- Around line 249-251: The generator uses an ambiguous loop variable
lin the
char-count aggregation (sum(len(l) for l in lines)) which triggers Ruff E741;
change the generator to use a clearer name likeline(e.g., sum(len(line) for
line in lines)) so it no longer uses a single-letter shadowed variable; update
the occurrence in the logging call that formats "pytesseract succeeded — %d
chars, avg confidence ..." to use the new variable name in the generator
expression and run linters to confirm the E741 warning is resolved.In @.opencode/skills/image-to-md/SKILL.md:
- Around line 66-71: The fenced code block containing the exception trace
("Object reference not set to an instance of an object" followed by the stack
lines including MainWindow.xaml.cs and App.OnStartup) is missing a language
identifier; update that fence in SKILL.md to include a language tag (for example
text orconsole) so the block becomestext ...to satisfy markdown
linting and improve syntax rendering.In
@specs/007-control-standardization/data-model.md:
- Around line 113-121: The fenced code block showing the control state diagram
is missing a language identifier (causing markdownlint MD040); update the
opening fence fromtotext so the block that starts with "Normal ──→
MouseOver ──→ Pressed ──→ Selected (for list controls)" is annotated as plain
text, and keep the closing ``` unchanged.In
@specs/007-control-standardization/tasks.md:
- Line 45: The inline code span contains a trailing space (
<Style) which
triggers markdownlint MD038; edit the checklist line to remove the trailing
space inside the backticks so the span reads<Style(i.e., replace<Style
with<Style) in the tasks entry for T007 and save the markdown.In
@specs/009-mvvm-architecture-cleanup/data-model.md:
- Around line 22-75: The three fenced code blocks in the document (the service
resolution/state transitions block, the Event Bus state transitions block, and
the Module state transitions block) are missing language identifiers and trigger
MD040; update each opening triple-backtick to include a language tag (e.g.,
changetotext) so the blocks becometext ...and satisfy the
markdown linter while preserving the exact block content.In
@WpfApp2/ViewModels/SubDailyReportViewModel.cs:
- Around line 184-213: RefreshNames currently rebuilds NameItems but doesn't
subscribe to each NamePickItem's IsChecked changes so RecalcButtons (and thus
CanPreview) won't update when a user toggles a single checkbox; fix by wiring a
PropertyChanged handler for IsChecked on each NamePickItem as you add them in
RefreshNames (and unsubscribe/clear handlers when clearing NameItems) so the
handler calls RecalcButtons whenever IsChecked changes; apply the same
subscribe/unsubscribe pattern to the other list-populating method around lines
215-232 as well to keep button state in sync.
Nitpick comments:
In@specs/009-mvvm-architecture-cleanup/quickstart.md:
- Around line 19-20: Update the quickstart wording to enforce a single
composition point: change the step text to explicitly instruct readers to
register services in CompositionRoot.cs and state that App.xaml.cs should only
call into CompositionRoot to start composition; replace the “App.xaml.cs or
CompositionRoot.cs” phrasing with a clear directive naming CompositionRoot.cs as
the registration location and mention App.xaml.cs only invokes composition
startup (referencing CompositionRoot.cs and App.xaml.cs to locate the code).In
@specs/009-mvvm-architecture-cleanup/tasks.md:
- Around line 22-23: The docs alternate which file is considered the composition
root; standardize them so CompositionRoot.cs is the authoritative composition
root and App.xaml.cs only invokes startup wiring: update all references
(including the occurrences noted around lines ~55-56, ~77-82, ~102-103) to state
that CompositionRoot.cs owns registrations/modules and expose methods like
RegisterServices() or InitializeModules(), and that App.xaml.cs should call
CompositionRoot.RegisterServices()/InitializeModules() during startup rather
than containing registration logic itself.In
@WpfApp2/XerEditorWindow.xaml.cs:
- Line 10: Replace the direct instantiation of XerEditorViewModel with a resolve
from the DI container: instead of "new XerEditorViewModel()" use
App.Container.Resolve() (or the container's equivalent
Resolve/ResolveRequired method) when setting DataContext; ensure
XerEditorViewModel's constructor follows the ViewModels guideline (inherits
ViewModelBase, uses SetProperty, and receives its service dependencies via
IServiceContainer) so the container can construct it.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `cdeab4c3-125a-4914-8477-e6600ac76c3e` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3b7114a5263535a5fc68a87466584bff107ab385 and 56a1b027762414f355e46855d2a614ef0072e962. </details> <details> <summary>📒 Files selected for processing (43)</summary> * `.opencode/skills/image-to-md/SKILL.md` * `.opencode/skills/image-to-md/requirements.txt` * `.opencode/skills/image-to-md/scripts/image_to_md.py` * `.specify/feature.json` * `AGENTS.md` * `Docs/Architecture/MVVM_COMPLIANCE.md` * `Docs/Architecture/MVVM_RULES.md` * `Error/Screenshot 2026-05-23 101207.md` * `Error/Screenshot 2026-05-23 101231.md` * `Error/Screenshot 2026-05-23 101251.md` * `Error/Screenshot 2026-05-23 101310.md` * `WpfApp2/App.xaml.cs` * `WpfApp2/CompositionRoot.cs` * `WpfApp2/Fixpiecolors.xaml.cs` * `WpfApp2/Services/EventBus.cs` * `WpfApp2/Services/ModuleRegistry.cs` * `WpfApp2/Services/ServiceContainer.cs` * `WpfApp2/Som3a_WPF_UI.csproj` * `WpfApp2/SubDailyReportWindow.xaml.cs` * `WpfApp2/UnmergeFillDownWindow.xaml.cs` * `WpfApp2/ViewModels/SubDailyReportViewModel.cs` * `WpfApp2/ViewModels/UnmergeFillDownViewModel.cs` * `WpfApp2/ViewModels/ViewModelBase.cs` * `WpfApp2/ViewModels/XerEditorViewModel.cs` * `WpfApp2/XerEditorWindow.xaml.cs` * `extraction_1779466152081.md` * `specs/007-control-standardization/checklists/requirements.md` * `specs/007-control-standardization/data-model.md` * `specs/007-control-standardization/plan.md` * `specs/007-control-standardization/quickstart.md` * `specs/007-control-standardization/research.md` * `specs/007-control-standardization/spec.md` * `specs/007-control-standardization/tasks.md` * `specs/009-mvvm-architecture-cleanup/checklists/requirements.md` * `specs/009-mvvm-architecture-cleanup/contracts/IEventBus.md` * `specs/009-mvvm-architecture-cleanup/contracts/IModule.md` * `specs/009-mvvm-architecture-cleanup/contracts/IServiceContainer.md` * `specs/009-mvvm-architecture-cleanup/data-model.md` * `specs/009-mvvm-architecture-cleanup/plan.md` * `specs/009-mvvm-architecture-cleanup/quickstart.md` * `specs/009-mvvm-architecture-cleanup/research.md` * `specs/009-mvvm-architecture-cleanup/spec.md` * `specs/009-mvvm-architecture-cleanup/tasks.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
WpfApp2/ViewModels/Primavera/PrimaveraCompareViewModel.cs (1)
16-16: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMake
PrimaveraCompareViewModelinheritViewModelBasefor MVVM compliance.This ViewModel still implements
INotifyPropertyChangeddirectly instead of inheritingViewModelBase, which breaks the stated ViewModel standard.As per coding guidelines: "All ViewModels must inherit from ViewModelBase and use constructor injection via IServiceContainer for service dependencies".♻️ Minimal starting diff
-public class PrimaveraCompareViewModel : INotifyPropertyChanged +public class PrimaveraCompareViewModel : ViewModelBase🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/Primavera/PrimaveraCompareViewModel.cs` at line 16, PrimaveraCompareViewModel currently implements INotifyPropertyChanged directly; change its declaration to inherit from ViewModelBase, remove the manual INotifyPropertyChanged implementation, and update its constructor to accept required services via IServiceContainer (constructor injection) forwarding them to base if needed; ensure you reference the class name PrimaveraCompareViewModel, the base class ViewModelBase, and IServiceContainer when locating and modifying the code and adjust any property change calls to use the base class's SetProperty/OnPropertyChanged helpers.WpfApp2/ViewModels/ProjectAnalysisViewModel.cs (1)
12-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSwitch base class to
ViewModelBaseto satisfy MVVM rules.
ProjectAnalysisViewModelcurrently inheritsNotifyBase, but this repository requiresViewModelBasefor all ViewModels.As per coding guidelines: "All ViewModels must inherit from ViewModelBase and use constructor injection via IServiceContainer for service dependencies".Suggested minimal change
-public sealed class ProjectAnalysisViewModel : NotifyBase +public sealed class ProjectAnalysisViewModel : ViewModelBase🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/ProjectAnalysisViewModel.cs` at line 12, Change ProjectAnalysisViewModel to inherit from ViewModelBase instead of NotifyBase and implement constructor injection via IServiceContainer: replace the base class reference NotifyBase with ViewModelBase on the ProjectAnalysisViewModel declaration, add a constructor ProjectAnalysisViewModel(IServiceContainer services) that calls the base constructor if ViewModelBase requires it (or stores/resolves required services from services) and move any service resolution off of property initializers into that constructor; also add any necessary using/import for ViewModelBase and IServiceContainer.WpfApp2/ViewModels/LinksManagerViewModel.cs (1)
18-19: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdopt
ViewModelBasefor this ViewModel.
LinksManagerViewModelstill implementsINotifyPropertyChangeddirectly; this breaks the repo MVVM contract for ViewModels in this path.Suggested direction
-public sealed class LinksManagerViewModel : INotifyPropertyChanged +public sealed class LinksManagerViewModel : ViewModelBaseThen migrate local
Set(...)/OnPropertyChanged(...)usage toSetProperty(...)fromViewModelBase.As per coding guidelines:
WpfApp2/ViewModels/**/*.cs: All ViewModels must inherit from ViewModelBase and use constructor injection via IServiceContainer for service dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/LinksManagerViewModel.cs` around lines 18 - 19, Replace the direct INotifyPropertyChanged implementation in LinksManagerViewModel by inheriting from ViewModelBase, remove/local implementations of Set(...) and OnPropertyChanged(...) and migrate property change calls to ViewModelBase's SetProperty(...); update the constructor to accept dependencies via IServiceContainer (constructor injection) and resolve any services from that container rather than creating them locally, ensuring all property setters call SetProperty(nameof(Property), ref backingField, value) (or the equivalent SetProperty overload) and remove the explicit INotifyPropertyChanged interface declaration.WpfApp2/StyleSelectorWindow.xaml (1)
43-47:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove remaining title-bar/button interactions to VM commands.
This view still uses code-behind event handlers for drag/minimize/close, which violates the View interaction rule for XAML views.
As per coding guidelines:
WpfApp2/**/*.xaml: All user interactions in Views must be handled via ICommand properties in ViewModels, not code-behind event handlers.Also applies to: 67-73, 76-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/StyleSelectorWindow.xaml` around lines 43 - 47, The TitleBar and button interactions wired to code-behind (e.g., TitleBar_MouseLeftButtonDown and the minimize/close click handlers) must be moved to ICommand properties on the ViewModel; add commands like TitleBarDragCommand, MinimizeCommand and CloseCommand to the associated VM, expose them as public ICommand, and implement their logic there, then update the XAML to remove MouseLeftButtonDown and Click handlers and bind the events to these commands using Command on buttons and an event-to-command behavior (e.g., Interaction.Triggers / InvokeCommandAction or an attached behavior) for the TitleBar MouseLeftButtonDown, and finally remove the corresponding methods from the code-behind so the View has no interaction logic.WpfApp2/ViewModels/XerEditorViewModel.cs (1)
56-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Load()bypasses container-resolved_parser.The constructor resolves
_parserfrom the container (Line 29), butLoad()overwrites it withnew XerParser(). This breaks the DI pattern and makes the initial resolution pointless.🐛 Suggested fix
Either remove the container resolution if each load requires a fresh parser, or reuse the resolved instance:
private void Load() { var ofd = new OpenFileDialog { Filter = "XER (*.xer)|*.xer" }; if (ofd.ShowDialog() != true) return; _filePath = ofd.FileName; - _parser = new XerParser(); + _parser = _container.Resolve<XerParser>(); _parser.Parse(_filePath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/XerEditorViewModel.cs` around lines 56 - 59, The Load() method currently overwrites the container-resolved parser by assigning _parser = new XerParser(); — either remove the container resolution in the constructor or stop instantiating a new parser in Load(); to keep DI, delete the new XerParser() assignment in Load() and call _parser.Parse(_filePath) using the already-resolved _parser (or, if each load truly needs a fresh instance, remove the constructor resolution of _parser and always instantiate inside Load()); adjust only the _parser initialization so the constructor-resolved _parser and the Load() method are not in conflict.WpfApp2/ViewModels/FloatPathViewModel.cs (1)
19-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInherit from
ViewModelBaseinstead of implementingINotifyPropertyChangeddirectly.Per coding guidelines, all ViewModels must inherit from
ViewModelBase. This class implementsINotifyPropertyChangeddirectly and uses a manualOnPropertyChangedhelper (lines 388-391) instead of the standardSetProperty<T>fromViewModelBase.🔧 Proposed fix
- public class FloatPathViewModel : INotifyPropertyChanged + public class FloatPathViewModel : ViewModelBaseThen replace manual property setters with
SetProperty(ref _field, value)calls and remove the manualINotifyPropertyChangedimplementation at lines 386-392.As per coding guidelines:
All ViewModels must inherit from ViewModelBase and use constructor injection via IServiceContainer for service dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/FloatPathViewModel.cs` at line 19, The FloatPathViewModel class currently implements INotifyPropertyChanged directly and uses a manual OnPropertyChanged helper; change the class to inherit from ViewModelBase, remove the manual INotifyPropertyChanged implementation and OnPropertyChanged method, and update all property setters to use SetProperty(ref _field, value) (replace direct assignments and manual raises). Also add constructor injection for any services via IServiceContainer in the FloatPathViewModel constructor (migrate any service lookups to constructor parameters) so the class follows the ViewModelBase pattern and coding guidelines.
🧹 Nitpick comments (6)
WpfApp2/ViewModels/WbsStyleSelectorViewModel.cs (1)
32-41: 💤 Low valueUnused
containerparameter should be removed.The
IServiceContainer containerparameter is declared but never used in the constructor. If no services need to be resolved, remove the parameter to avoid dead code and misleading API.♻️ Proposed fix
- public WbsStyleSelectorViewModel(IServiceContainer container) + public WbsStyleSelectorViewModel() { SaveCommand = new RelayCommand(OnSave); CancelCommand = new RelayCommand(() => CloseWindow?.Invoke());Note: If the container is removed from the constructor, update the registration in
CompositionRoot.csaccordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/WbsStyleSelectorViewModel.cs` around lines 32 - 41, The constructor WbsStyleSelectorViewModel currently declares an unused IServiceContainer container parameter; remove the container parameter from the WbsStyleSelectorViewModel constructor signature and any calls/registrations that pass it (e.g., update CompositionRoot.cs registration to construct WbsStyleSelectorViewModel without the container), then rebuild and run tests to ensure no other code expects that parameter; leave all existing logic inside the constructor (SaveCommand, CancelCommand, InitializeOptions, _selectedStyleId, OnPropertyChanged, GeneratePreview) unchanged.WpfApp2/LinksManagerWindow.xaml.cs (1)
19-20: 💤 Low valueConsider resolving
LinksManagerServicefrom the container.The service is instantiated directly with
new LinksManagerService(excelApp)rather than being resolved fromApp.Container. This bypasses the DI infrastructure and makes the service harder to mock/test. If the service needs the Excel app instance, consider registering a factory or using a scoped resolution pattern.However, if the service is intentionally per-window with the Excel instance, this is acceptable as a pragmatic choice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/LinksManagerWindow.xaml.cs` around lines 19 - 20, The code in LinksManagerWindow.xaml.cs directly instantiates LinksManagerService with new LinksManagerService(excelApp) instead of resolving it from the DI container; change this to resolve the service from App.Container (or register a factory that accepts the excelApp) and pass the resolved instance into LinksManagerViewModel so consumers can be mocked/tested. Specifically, register LinksManagerService in the container (or a factory like Func<Application, LinksManagerService> / factory delegate that accepts the excelApp) and replace the direct new with App.Container.Resolve/Resolve<LinksManagerService>(...) or the factory call, keeping the same arguments to LinksManagerViewModel (App.Container, excelApp, this.Dispatcher, svc). Ensure the registration handles the per-window/excelApp lifetime (scoped/transient) so each window gets its own service instance.WpfApp2/ViewModels/UnmergeFillDownViewModel.cs (1)
140-175: ⚖️ Poor tradeoff
asyncmethod withoutawaitblocks the UI thread.
RunAsync()is declaredasync Taskbut no longer contains anyawaitexpressions after removingTask.Run. The Excel COM operation at Line 158 now runs synchronously on the UI thread, which will freeze the UI during long operations.While this fixes the COM threading violation from the previous review, it introduces a UX regression. Consider using a dedicated STA background thread for Excel interop work if responsiveness is important.
♻️ Possible approach using STA thread
private Task RunOnStaThreadAsync(Action action) { var tcs = new TaskCompletionSource<object?>(); var thread = new Thread(() => { try { action(); tcs.SetResult(null); } catch (Exception ex) { tcs.SetException(ex); } }); thread.SetApartmentState(ApartmentState.STA); thread.Start(); return tcs.Task; } private async Task RunAsync() { // ...setup... await RunOnStaThreadAsync(() => { _service.UnmergeAndFillDownColumn(ws, SelectedColumn!, startRow: HeaderRow, progress: progress, token: _cts.Token); }); // ...cleanup... }Note: Progress reporting would need to marshal back to UI thread.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/UnmergeFillDownViewModel.cs` around lines 140 - 175, RunAsync is marked async but calls _service.UnmergeAndFillDownColumn synchronously on the UI thread, causing UI freezes; fix by executing the Excel COM work on a dedicated STA background thread and await it from RunAsync (create a helper like RunOnStaThreadAsync that accepts an Action, sets thread apartment state to STA, runs the action, and returns a Task), invoke _service.UnmergeAndFillDownColumn inside that STA helper so RunAsync can await it, and ensure progress callbacks (the Progress<double> instance) and status updates are marshaled back to the UI thread (Dispatcher/SynchronizationContext) and cancellation via _cts.Token is passed through.WpfApp2/Fixpiecolors.xaml.cs (1)
86-93: 💤 Low valueAvoid passing
null!for event args.Line 92 calls
OnSheetNamesChanged(null, null!)which forces a null-forgiving operator for the event args parameter. Consider extracting the sync logic into a separate method that both the event handler andSyncFromViewModelcan call.♻️ Suggested refactor
+ private void PopulateSheetsComboBox() + { + cmbSheets.Items.Clear(); + foreach (var name in _vm!.SheetNames) + cmbSheets.Items.Add(name); + if (_vm.SheetNames.Count > 0) + cmbSheets.SelectedItem = _vm.SelectedSheet; + } + private void OnSheetNamesChanged(object? sender, NotifyCollectionChangedEventArgs e) { - cmbSheets.Items.Clear(); - foreach (var name in _vm!.SheetNames) - cmbSheets.Items.Add(name); - if (_vm.SheetNames.Count > 0) - cmbSheets.SelectedItem = _vm.SelectedSheet; + PopulateSheetsComboBox(); } private void SyncFromViewModel() { if (_vm == null) return; txtCategoryRange.Text = _vm.CategoryRange; txtColorTableRange.Text = _vm.ColorTableRange; - OnSheetNamesChanged(null, null!); + PopulateSheetsComboBox(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/Fixpiecolors.xaml.cs` around lines 86 - 93, The call SyncFromViewModel currently invokes OnSheetNamesChanged(null, null!) which uses a null-forgiving operator for event args; instead, extract the logic inside OnSheetNamesChanged that updates the UI/state into a new private method (e.g., UpdateSheetNames or ApplySheetNameChanges) that takes no EventArgs, move the shared code there, call that new method from both OnSheetNamesChanged(sender, e) and SyncFromViewModel, and remove the null!/null call so OnSheetNamesChanged is only used as an event handler and SyncFromViewModel directly calls the new helper.WpfApp2/ViewModels/SubDailyReportViewModel.cs (1)
348-372: ⚡ Quick winMove UI helper methods to View code-behind.
ListView_PreviewMouseWheelandFindVisualChild<T>operate onDependencyObject,ScrollViewer,VisualTreeHelper, andMouseWheelEventArgs— all UI-specific types. These belong in the View's code-behind, not the ViewModel. This violates MVVM separation and the coding guideline that Views handle user interactions via commands or the view layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WpfApp2/ViewModels/SubDailyReportViewModel.cs` around lines 348 - 372, The two UI-specific helpers ListView_PreviewMouseWheel and FindVisualChild<T> should be moved out of the ViewModel into the View's code-behind (e.g., SubDailyReportView.xaml.cs): copy ListView_PreviewMouseWheel and FindVisualChild<T> into the view class, wire ListView_PreviewMouseWheel to the ListView.PreviewMouseWheel event in XAML, and remove both methods (and any UI using directives) from SubDailyReportViewModel so the VM no longer references DependencyObject/ScrollViewer/VisualTreeHelper/MouseWheelEventArgs; if the ViewModel needs to communicate vertical offset behavior, expose a simple bindable property or ICommand instead and keep all direct UI handling in the view code-behind.specs/009-mvvm-architecture-cleanup/tasks.md (1)
9-9: 💤 Low valueHeading level skip flagged by markdownlint.
### Test Tasksjumps from implicit h1 (document title) to h3. Consider changing to## Test Tasksto follow heading-increment convention, or ignore if the intent is to nest under the Tests paragraph.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/009-mvvm-architecture-cleanup/tasks.md` at line 9, The markdown heading "### Test Tasks" skips a level from the document title; update the heading to "## Test Tasks" (replace the "### Test Tasks" token) to follow heading-increment convention, or explicitly document intent to nest under the Tests section if you prefer to keep it as an h3; modify the "### Test Tasks" line accordingly to resolve the markdownlint heading-level skip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Som3a.Shared/Models/MainViewModel.cs`:
- Line 12: MainViewModel currently references Som3a_WPF_UI.RelayCommand,
coupling the shared model to the UI; remove the using and RelayCommand usage
from Som3a.Shared.Models.MainViewModel and either define a UI-agnostic command
abstraction in the shared layer (e.g., an ICommand-like interface or callback
delegate used by command properties) or move all RelayCommand
construction/assignment into the WPF project. Concretely, replace any
RelayCommand-typed properties/fields in MainViewModel with a neutral type (an
interface or delegate) or plain methods/events, and create the RelayCommand
instances in the Som3a_WPF_UI layer when wiring the VM to the view so
MainViewModel no longer depends on Som3a_WPF_UI.RelayCommand.
In `@WpfApp2/CompositionRoot.cs`:
- Line 37: LinksManagerViewModel is being registered via
container.RegisterTransient<LinksManagerViewModel, LinksManagerViewModel>() but
its constructor requires ExcelApp and Dispatcher which are not registered; fix
by either registering those dependencies (register ExcelApp and Dispatcher with
the container) or change the LinksManagerViewModel registration to a factory
overload that resolves ExcelApp and Dispatcher from the container and constructs
new LinksManagerViewModel(excelApp, dispatcher) so resolves won't fail at
runtime.
In `@WpfApp2/MainWindow.xaml.cs`:
- Around line 21-23: The MainWindow currently subscribes to
_vm.NotificationRaised with OnViewModelNotification but does not unsubscribe,
which can keep the window alive; update the MainWindow_Closed method to remove
the handler by calling _vm.NotificationRaised -= OnViewModelNotification
(safe-null-check _vm if needed). Also ensure any other VM event subscriptions
added in the constructor (the similar handlers referenced around lines 54-61)
are unsubscribed in MainWindow_Closed using the corresponding -= pairs so all
ViewModel event handlers are detached on window close.
In `@WpfApp2/Services/ServiceContainer.cs`:
- Around line 246-257: CreateInstance is calling Resolve (not ResolveScoped) and
so nested scoped dependencies bypass the scopedInstances cache; add an overload
CreateInstance(Type type, Dictionary<Type, object>? scopedInstances) and update
CreateInstance to pass that scopedInstances down whenever it calls
Resolve/creates nested instances (or have CreateInstance call ResolveScoped
instead) so that the same scopedInstances dictionary used when handling
ServiceLifetime.Scoped is propagated to nested resolutions; update the call site
at the current registration handling (where ServiceLifetime.Scoped sets
scopedInstances[serviceType] = instance and then calls CreateInstance) to pass
the active scopedInstances, ensuring nested scoped dependencies reuse the cache
and keep emitting ServiceResolved as before.
In `@WpfApp2/UI/ProjectAnalysisWindow.xaml.cs`:
- Around line 14-15: The UI is new-ing ExcelProjectAnalysisService directly in
the window; instead register and resolve it from the composition root/container:
add a registration for ExcelProjectAnalysisService (or an interface like
IProjectAnalysisService) in RegisterServices(IServiceContainer), remove the
direct new from ProjectAnalysisWindow (the constructor where DataContext is
set), and obtain the service from App.Container (or let the container construct
ProjectAnalysisViewModel with the service injected) so ProjectAnalysisViewModel
receives the service via DI rather than Window creating it.
In `@WpfApp2/ViewModels/CommandPaletteViewModel.cs`:
- Around line 39-42: Constructor CommandPaletteViewModel should guard against a
null IServiceContainer to avoid opaque NullReferenceExceptions: add a null-check
for the constructor parameter container at the start of
CommandPaletteViewModel(IServiceContainer container) and throw an
ArgumentNullException(nameof(container)) if null before calling
container.Resolve<INavigationService>() and assigning _navigationService; this
ensures clearer failure behavior and a meaningful exception instead of a runtime
NRE.
In `@WpfApp2/ViewModels/FloatPathViewModel.cs`:
- Around line 106-107: Remove all temporary MessageBox.Show debug calls in
FloatPathViewModel (e.g., the call immediately before
SendGraphToUI?.Invoke(GraphHtml) and the other instances listed) and replace
them with non-blocking logging; specifically delete MessageBox.Show usages and
call the existing logging mechanism or Debug/Trace (or an injected ILogger) to
record the same messages without UI interruption, keeping SendGraphToUI,
GraphHtml and other logic unchanged and ensuring no blocking UI dialogs remain
in methods like the graph-send path and other debug locations mentioned.
In `@WpfApp2/ViewModels/Primavera/PrimaveraResultsViewModel.cs`:
- Around line 11-15: Update the class to inherit from ViewModelBase (change the
declaration of PrimaveraResultsViewModel to derive from ViewModelBase) and
ensure the constructor signature remains
PrimaveraResultsViewModel(IServiceContainer container, ComparisonResult result)
while forwarding the service container to the base ViewModelBase (call
base(container) if ViewModelBase exposes that constructor) or otherwise invoke
the appropriate base constructor; keep constructor injection for
IServiceContainer and retain the ComparisonResult parameter and any existing
initialization logic.
In `@WpfApp2/ViewModels/SubDailyReportViewModel.cs`:
- Line 284: The PreviewAsync method is marked async but contains no awaits
(causing CS1998) and blocks the UI during Excel COM work; either convert
PreviewAsync to a synchronous void method (remove async/Task signature) if all
work must run on the UI STA thread, or keep an async signature but perform the
heavy/CPU-bound merge/preview build off the UI thread using await Task.Run(...)
for the non-COM processing while keeping COM interop calls on the STA/UI thread;
update the PreviewAsync declaration and call sites accordingly and ensure any
COM reads remain on STA before awaiting Task.Run for background work.
In `@WpfApp2/ViewModels/ToastViewModel.cs`:
- Around line 25-29: The constructor ToastViewModel(ToastModel model)
dereferences model without a null check; add a guard at the start of the
constructor to validate model (e.g., throw new
ArgumentNullException(nameof(model)) or provide a safe fallback) before
assigning to ToastType, Message, and DurationMs so the constructor never
dereferences a null ToastModel.
In `@WpfApp2/ViewModels/XerEditorViewModel.cs`:
- Around line 237-247: RefreshTablesUI() replaces the TableItemVM instances, so
updating the old items in selected has no UI effect; after calling
RefreshTablesUI() locate the corresponding new TableItemVM instances in the
refreshed Tables collection (match by Name or another unique key) and set their
Count and Status using _parser.Tables.FirstOrDefault(...) to get the row count,
e.g., for each original selected item find var newItem = Tables.FirstOrDefault(x
=> x.Name == t.Name) and update newItem.Count and newItem.Status ("Updated")
instead of modifying the old selected objects.
---
Outside diff comments:
In `@WpfApp2/StyleSelectorWindow.xaml`:
- Around line 43-47: The TitleBar and button interactions wired to code-behind
(e.g., TitleBar_MouseLeftButtonDown and the minimize/close click handlers) must
be moved to ICommand properties on the ViewModel; add commands like
TitleBarDragCommand, MinimizeCommand and CloseCommand to the associated VM,
expose them as public ICommand, and implement their logic there, then update the
XAML to remove MouseLeftButtonDown and Click handlers and bind the events to
these commands using Command on buttons and an event-to-command behavior (e.g.,
Interaction.Triggers / InvokeCommandAction or an attached behavior) for the
TitleBar MouseLeftButtonDown, and finally remove the corresponding methods from
the code-behind so the View has no interaction logic.
In `@WpfApp2/ViewModels/FloatPathViewModel.cs`:
- Line 19: The FloatPathViewModel class currently implements
INotifyPropertyChanged directly and uses a manual OnPropertyChanged helper;
change the class to inherit from ViewModelBase, remove the manual
INotifyPropertyChanged implementation and OnPropertyChanged method, and update
all property setters to use SetProperty(ref _field, value) (replace direct
assignments and manual raises). Also add constructor injection for any services
via IServiceContainer in the FloatPathViewModel constructor (migrate any service
lookups to constructor parameters) so the class follows the ViewModelBase
pattern and coding guidelines.
In `@WpfApp2/ViewModels/LinksManagerViewModel.cs`:
- Around line 18-19: Replace the direct INotifyPropertyChanged implementation in
LinksManagerViewModel by inheriting from ViewModelBase, remove/local
implementations of Set(...) and OnPropertyChanged(...) and migrate property
change calls to ViewModelBase's SetProperty(...); update the constructor to
accept dependencies via IServiceContainer (constructor injection) and resolve
any services from that container rather than creating them locally, ensuring all
property setters call SetProperty(nameof(Property), ref backingField, value) (or
the equivalent SetProperty overload) and remove the explicit
INotifyPropertyChanged interface declaration.
In `@WpfApp2/ViewModels/Primavera/PrimaveraCompareViewModel.cs`:
- Line 16: PrimaveraCompareViewModel currently implements INotifyPropertyChanged
directly; change its declaration to inherit from ViewModelBase, remove the
manual INotifyPropertyChanged implementation, and update its constructor to
accept required services via IServiceContainer (constructor injection)
forwarding them to base if needed; ensure you reference the class name
PrimaveraCompareViewModel, the base class ViewModelBase, and IServiceContainer
when locating and modifying the code and adjust any property change calls to use
the base class's SetProperty/OnPropertyChanged helpers.
In `@WpfApp2/ViewModels/ProjectAnalysisViewModel.cs`:
- Line 12: Change ProjectAnalysisViewModel to inherit from ViewModelBase instead
of NotifyBase and implement constructor injection via IServiceContainer: replace
the base class reference NotifyBase with ViewModelBase on the
ProjectAnalysisViewModel declaration, add a constructor
ProjectAnalysisViewModel(IServiceContainer services) that calls the base
constructor if ViewModelBase requires it (or stores/resolves required services
from services) and move any service resolution off of property initializers into
that constructor; also add any necessary using/import for ViewModelBase and
IServiceContainer.
In `@WpfApp2/ViewModels/XerEditorViewModel.cs`:
- Around line 56-59: The Load() method currently overwrites the
container-resolved parser by assigning _parser = new XerParser(); — either
remove the container resolution in the constructor or stop instantiating a new
parser in Load(); to keep DI, delete the new XerParser() assignment in Load()
and call _parser.Parse(_filePath) using the already-resolved _parser (or, if
each load truly needs a fresh instance, remove the constructor resolution of
_parser and always instantiate inside Load()); adjust only the _parser
initialization so the constructor-resolved _parser and the Load() method are not
in conflict.
---
Nitpick comments:
In `@specs/009-mvvm-architecture-cleanup/tasks.md`:
- Line 9: The markdown heading "### Test Tasks" skips a level from the document
title; update the heading to "## Test Tasks" (replace the "### Test Tasks"
token) to follow heading-increment convention, or explicitly document intent to
nest under the Tests section if you prefer to keep it as an h3; modify the "###
Test Tasks" line accordingly to resolve the markdownlint heading-level skip.
In `@WpfApp2/Fixpiecolors.xaml.cs`:
- Around line 86-93: The call SyncFromViewModel currently invokes
OnSheetNamesChanged(null, null!) which uses a null-forgiving operator for event
args; instead, extract the logic inside OnSheetNamesChanged that updates the
UI/state into a new private method (e.g., UpdateSheetNames or
ApplySheetNameChanges) that takes no EventArgs, move the shared code there, call
that new method from both OnSheetNamesChanged(sender, e) and SyncFromViewModel,
and remove the null!/null call so OnSheetNamesChanged is only used as an event
handler and SyncFromViewModel directly calls the new helper.
In `@WpfApp2/LinksManagerWindow.xaml.cs`:
- Around line 19-20: The code in LinksManagerWindow.xaml.cs directly
instantiates LinksManagerService with new LinksManagerService(excelApp) instead
of resolving it from the DI container; change this to resolve the service from
App.Container (or register a factory that accepts the excelApp) and pass the
resolved instance into LinksManagerViewModel so consumers can be mocked/tested.
Specifically, register LinksManagerService in the container (or a factory like
Func<Application, LinksManagerService> / factory delegate that accepts the
excelApp) and replace the direct new with
App.Container.Resolve/Resolve<LinksManagerService>(...) or the factory call,
keeping the same arguments to LinksManagerViewModel (App.Container, excelApp,
this.Dispatcher, svc). Ensure the registration handles the per-window/excelApp
lifetime (scoped/transient) so each window gets its own service instance.
In `@WpfApp2/ViewModels/SubDailyReportViewModel.cs`:
- Around line 348-372: The two UI-specific helpers ListView_PreviewMouseWheel
and FindVisualChild<T> should be moved out of the ViewModel into the View's
code-behind (e.g., SubDailyReportView.xaml.cs): copy ListView_PreviewMouseWheel
and FindVisualChild<T> into the view class, wire ListView_PreviewMouseWheel to
the ListView.PreviewMouseWheel event in XAML, and remove both methods (and any
UI using directives) from SubDailyReportViewModel so the VM no longer references
DependencyObject/ScrollViewer/VisualTreeHelper/MouseWheelEventArgs; if the
ViewModel needs to communicate vertical offset behavior, expose a simple
bindable property or ICommand instead and keep all direct UI handling in the
view code-behind.
In `@WpfApp2/ViewModels/UnmergeFillDownViewModel.cs`:
- Around line 140-175: RunAsync is marked async but calls
_service.UnmergeAndFillDownColumn synchronously on the UI thread, causing UI
freezes; fix by executing the Excel COM work on a dedicated STA background
thread and await it from RunAsync (create a helper like RunOnStaThreadAsync that
accepts an Action, sets thread apartment state to STA, runs the action, and
returns a Task), invoke _service.UnmergeAndFillDownColumn inside that STA helper
so RunAsync can await it, and ensure progress callbacks (the Progress<double>
instance) and status updates are marshaled back to the UI thread
(Dispatcher/SynchronizationContext) and cancellation via _cts.Token is passed
through.
In `@WpfApp2/ViewModels/WbsStyleSelectorViewModel.cs`:
- Around line 32-41: The constructor WbsStyleSelectorViewModel currently
declares an unused IServiceContainer container parameter; remove the container
parameter from the WbsStyleSelectorViewModel constructor signature and any
calls/registrations that pass it (e.g., update CompositionRoot.cs registration
to construct WbsStyleSelectorViewModel without the container), then rebuild and
run tests to ensure no other code expects that parameter; leave all existing
logic inside the constructor (SaveCommand, CancelCommand, InitializeOptions,
_selectedStyleId, OnPropertyChanged, GeneratePreview) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 530e6dbf-cf19-4b7d-a87a-60d1849c3dd8
📒 Files selected for processing (43)
.gitignore.opencode/skills/image-to-md/requirements.txt.opencode/skills/image-to-md/scripts/image_to_md.pySom3a.Shared/Models/MainViewModel.csWpfApp2/AssignTradeCodesWindow.xaml.csWpfApp2/CompositionRoot.csWpfApp2/Controls/Shell/CommandPalette.xaml.csWpfApp2/Controls/Shell/ShellWindow.xamlWpfApp2/Controls/Shell/ShellWindow.xaml.csWpfApp2/Controls/Toast/ToastWindow.xamlWpfApp2/Controls/Toast/ToastWindow.xaml.csWpfApp2/Fixpiecolors.xaml.csWpfApp2/Float_path.xaml.csWpfApp2/LinksManagerWindow.xaml.csWpfApp2/MainWindow.xaml.csWpfApp2/Services/EventBus.csWpfApp2/Services/ServiceContainer.csWpfApp2/Services/ToastService.csWpfApp2/Som3a_WPF_UI.csprojWpfApp2/StyleSelectorWindow.xamlWpfApp2/StyleSelectorWindow.xaml.csWpfApp2/SubDailyReportWindow.xaml.csWpfApp2/UI/ProjectAnalysisWindow.xaml.csWpfApp2/UnmergeFillDownWindow.xaml.csWpfApp2/ViewModels/CommandPaletteViewModel.csWpfApp2/ViewModels/FixPieColorsViewModel.csWpfApp2/ViewModels/FloatPathViewModel.csWpfApp2/ViewModels/LinksManagerViewModel.csWpfApp2/ViewModels/Primavera/PrimaveraCompareViewModel.csWpfApp2/ViewModels/Primavera/PrimaveraResultsViewModel.csWpfApp2/ViewModels/ProjectAnalysisViewModel.csWpfApp2/ViewModels/SettingsViewModel.csWpfApp2/ViewModels/SubDailyReportViewModel.csWpfApp2/ViewModels/ToastViewModel.csWpfApp2/ViewModels/UnmergeFillDownViewModel.csWpfApp2/ViewModels/WbsStyleSelectorViewModel.csWpfApp2/ViewModels/XerEditorViewModel.csWpfApp2/Views/SettingsWindow.xamlWpfApp2/Views/SettingsWindow.xaml.csWpfApp2/Windows/PrimaveraComparison/PrimaveraCompareWindow.xaml.csWpfApp2/Windows/PrimaveraComparison/PrimaveraResultsWindow.xaml.csWpfApp2/XerEditorWindow.xaml.csspecs/009-mvvm-architecture-cleanup/tasks.md
Comment on lines
+246
to
+257
| _resolutionStack.Add(serviceType); | ||
| try | ||
| { | ||
| var instance = CreateInstance(registration.ImplementationType); | ||
|
|
||
| if (registration.Lifetime == ServiceLifetime.Scoped) | ||
| { | ||
| scopedInstances[serviceType] = instance; | ||
| } | ||
|
|
||
| ServiceResolved?.Invoke(this, new ServiceResolutionEventArgs(serviceType, registration.Lifetime)); | ||
| return instance; |
There was a problem hiding this comment.
Nested scoped dependencies won't share scope cache.
CreateInstance (Line 249) internally calls Resolve() (Line 211), not ResolveScoped(). When a scoped service has constructor dependencies that are also scoped, those nested dependencies will be resolved without the scopedInstances dictionary, creating new instances instead of reusing cached ones within the same scope.
🐛 Suggested approach
Either:
- Add an overload
CreateInstance(Type type, Dictionary<Type, object>? scopedInstances)that passes the cache through nested resolutions, or - Use a thread-local or scope-context pattern to track the active scope during resolution.
- private object CreateInstance(Type type)
+ private object CreateInstance(Type type, Dictionary<Type, object>? scopedInstances = null)
{
var constructors = type.GetConstructors();
if (constructors.Length == 0)
{
return Activator.CreateInstance(type)!;
}
var constructor = constructors[0];
var parameters = constructor.GetParameters();
var resolvedParams = new object[parameters.Length];
for (int i = 0; i < parameters.Length; i++)
{
- resolvedParams[i] = Resolve(parameters[i].ParameterType);
+ resolvedParams[i] = scopedInstances != null
+ ? ResolveScoped(parameters[i].ParameterType, scopedInstances)
+ : Resolve(parameters[i].ParameterType);
}
return constructor.Invoke(resolvedParams);
}Then update Line 249:
- var instance = CreateInstance(registration.ImplementationType);
+ var instance = CreateInstance(registration.ImplementationType, scopedInstances);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WpfApp2/Services/ServiceContainer.cs` around lines 246 - 257, CreateInstance
is calling Resolve (not ResolveScoped) and so nested scoped dependencies bypass
the scopedInstances cache; add an overload CreateInstance(Type type,
Dictionary<Type, object>? scopedInstances) and update CreateInstance to pass
that scopedInstances down whenever it calls Resolve/creates nested instances (or
have CreateInstance call ResolveScoped instead) so that the same scopedInstances
dictionary used when handling ServiceLifetime.Scoped is propagated to nested
resolutions; update the call site at the current registration handling (where
ServiceLifetime.Scoped sets scopedInstances[serviceType] = instance and then
calls CreateInstance) to pass the active scopedInstances, ensuring nested scoped
dependencies reuse the cache and keep emitting ServiceResolved as before.
Comment on lines
+14
to
+15
| var svc = new ExcelProjectAnalysisService(excelApp); | ||
| DataContext = new ProjectAnalysisViewModel(App.Container, excelApp, this, svc); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move ExcelProjectAnalysisService creation behind composition root wiring.
Direct new ExcelProjectAnalysisService(excelApp) in the window bypasses centralized registration/composition and spreads DI policy into UI code.
♻️ Proposed direction
- var svc = new ExcelProjectAnalysisService(excelApp);
- DataContext = new ProjectAnalysisViewModel(App.Container, excelApp, this, svc);
+ var svcFactory = App.Container.Resolve<IExcelProjectAnalysisServiceFactory>();
+ var svc = svcFactory.Create(excelApp);
+ DataContext = new ProjectAnalysisViewModel(App.Container, excelApp, this, svc);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WpfApp2/UI/ProjectAnalysisWindow.xaml.cs` around lines 14 - 15, The UI is
new-ing ExcelProjectAnalysisService directly in the window; instead register and
resolve it from the composition root/container: add a registration for
ExcelProjectAnalysisService (or an interface like IProjectAnalysisService) in
RegisterServices(IServiceContainer), remove the direct new from
ProjectAnalysisWindow (the constructor where DataContext is set), and obtain the
service from App.Container (or let the container construct
ProjectAnalysisViewModel with the service injected) so ProjectAnalysisViewModel
receives the service via DI rather than Window creating it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/EventBusTests.cs`:
- Around line 38-55: T046_WeakReference_PrunesDeadSubscribers currently
subscribes with a static/lambda handler and calls token.Dispose(), so it never
exercises the GC-based weak-reference pruning in EventBus.Subscribe/Publish;
change the test to create a real subscriber instance with an instance method
handler (so handler.Target != null), call var wr = new
WeakReference(subscriberInstance) and do NOT call token.Dispose(), null out the
strong reference to the subscriber instance (keep only the WeakReference and the
Subscription token), force GC (GC.Collect + WaitForPendingFinalizers), call
bus.Publish(new TestEvent()), and assert that the subscriber handler was not
invoked (use a boolean flag set by the instance handler) rather than relying on
EventPublished to prove pruning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff4de9e6-968a-4190-9431-e65e99b20ed7
📒 Files selected for processing (4)
Tests/EventBusTests.csTests/ServiceContainerTests.csTests/Som3a_WPF_UI.Tests.csprojTests/ViewModelBaseTests.cs
Comment on lines
+38
to
+55
| public void T046_WeakReference_PrunesDeadSubscribers() | ||
| { | ||
| var bus = new EventBus(); | ||
| var token = bus.Subscribe<TestEvent>(e => { }); | ||
|
|
||
| var wr = new WeakReference(token); | ||
| token.Dispose(); | ||
| token = null!; | ||
| System.GC.Collect(); | ||
| System.GC.WaitForPendingFinalizers(); | ||
|
|
||
| bus.Publish(new TestEvent()); | ||
|
|
||
| bool eventFired = false; | ||
| bus.EventPublished += (_, _) => eventFired = true; | ||
| bus.Publish(new TestEvent()); | ||
| Assert.IsTrue(eventFired); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EVENTBUS_FILE="$(fd -i '^EventBus\.cs$' | head -n1)"
TEST_FILE="$(fd -i '^EventBusTests\.cs$' | head -n1)"
echo "EventBus: ${EVENTBUS_FILE:-NOT_FOUND}"
echo "TestFile: ${TEST_FILE:-NOT_FOUND}"
if [[ -n "${TEST_FILE:-}" ]]; then
echo "---- Current weak-reference test block ----"
sed -n '38,60p' "$TEST_FILE"
fi
if [[ -n "${EVENTBUS_FILE:-}" ]]; then
echo "---- EventBus weak-ref / subscribe / publish related logic ----"
rg -n -C3 'WeakReference|Subscribe<|Publish\(|Dispose|Unsubscribe|Remove|prun' "$EVENTBUS_FILE"
fiRepository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 4025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EVENTBUS_FILE="$(fd -i '^EventBus\.cs$' | head -n1)"
echo "EventBus: $EVENTBUS_FILE"
# Show Publish implementation and where subscriberRef is used
python3 - <<'PY'
import re,sys,subprocess,os, textwrap
PYRepository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EVENTBUS_FILE="$(fd -i '^EventBus\.cs$' | head -n1)"
# Print a focused range around Publish and SubscriberEntry usage
nl -ba "$EVENTBUS_FILE" | sed -n '110,220p'
nl -ba "$EVENTBUS_FILE" | sed -n '220,320p'Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EVENTBUS_FILE="$(fd -i '^EventBus\.cs$' | head -n1)"
rg -n "void Publish<|Publish\(|SubscriberRef|WeakReference|prun|Dispose\(|class SubscriptionToken|Unsubscribe" "$EVENTBUS_FILE" || true
echo "---- cat -n Publish/WeakReference section ----"
cat -n "$EVENTBUS_FILE" | sed -n '140,260p'
echo "---- cat -n SubscriptionToken section ----"
cat -n "$EVENTBUS_FILE" | sed -n '1,140p'Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 9374
Fix T046_WeakReference_PrunesDeadSubscribers to actually test GC-based weak-reference pruning
- The test subscribes with
e => { }which has no captured instance (handler.Target == null), soEventBus.Subscribestores no weak reference (subscriberRefis null) andPublishnever hits the pruning logic. token.Dispose()explicitly unsubscribes by token id, bypassing the GC-pruning path entirely.- The assertion listens to
EventPublished, which is raised even if no subscriber handler ran, so it can’t prove pruning.
Update T046 to subscribe using a handler with a non-null target (e.g., instance-method delegate), keep only a WeakReference to that target, drop strong refs (but don’t dispose the token), force GC, publish, and assert the subscriber handler was not invoked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/EventBusTests.cs` around lines 38 - 55,
T046_WeakReference_PrunesDeadSubscribers currently subscribes with a
static/lambda handler and calls token.Dispose(), so it never exercises the
GC-based weak-reference pruning in EventBus.Subscribe/Publish; change the test
to create a real subscriber instance with an instance method handler (so
handler.Target != null), call var wr = new WeakReference(subscriberInstance) and
do NOT call token.Dispose(), null out the strong reference to the subscriber
instance (keep only the WeakReference and the Subscription token), force GC
(GC.Collect + WaitForPendingFinalizers), call bus.Publish(new TestEvent()), and
assert that the subscriber handler was not invoked (use a boolean flag set by
the instance handler) rather than relying on EventPublished to prove pruning.
…sets and RelayCommand
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Som3a Addin 2026/Ribbon1.cs (1)
63-90: ⚡ Quick winDon’t swallow ribbon image-loading failures silently.
The empty
catchhides broken/misnamed/corrupt assets and makes missing icons hard to diagnose.Proposed minimal change
- catch - { - // Ignore invalid images - } + catch (Exception ex) + { + System.Diagnostics.Debug.WriteLine( + $"Ribbon image load failed for '{button.Name}': {ex.Message}"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Som3a` Addin 2026/Ribbon1.cs around lines 63 - 90, The current empty catch in the ribbon image-loading block swallows failures; change it to catch Exception (e.g., catch (Exception ex)) and log the error details (including the attempted imagePath and button.Name) instead of ignoring them so corrupt or missing assets are diagnosable; keep the behavior of not crashing the add-in but use a logging call (for example System.Diagnostics.Trace/Debug or your add-in logger) to record ex.Message and ex.ToString(), and leave ResizeRibbonImage and the rest of the flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Errortofix/Screenshot` 2026-05-24 095252.md:
- Around line 1-9: The file "Screenshot 2026-05-24 095252.md" contains a
committed stack-trace with absolute user paths (references like
Som3a_Addin_2026.Ribbon1.addin_setting_Click) and must be removed from the
repository and history: remove the file from the repo (git rm --cached or git
rm), commit and push the deletion, then purge the file from history using git
filter-repo or BFG (targeting the filename and any matching stack-trace strings
such as Som3a_Addin_2026.Ribbon1.addin_setting_Click), add the filename/pattern
to .gitignore to prevent re-commit, and verify no other commits contain the same
absolute paths before pushing the cleaned history.
- Line 5: Remove the committed error dump from Errortofix/Screenshot 2026-05-24
095252.md (strip the local absolute path and any sensitive info), and then fix
the binding issue: locate SettingsViewModel.IsDarkSelected and either make it
writable (add a setter) or change all XAML bindings that target IsDarkSelected
to Mode=OneWay (do not use TwoWay or OneWayToSource). Check
WpfApp2/Views/SettingsWindow.xaml and search all XAML, resource dictionaries and
styles for "IsDarkSelected" (and references from Ribbon1.addin_setting_Click if
it opens the window) and update offending bindings to OneWay or implement a
setter on the SettingsViewModel property so the binding mode used is valid.
In `@WpfApp2/ViewModels/WbsStyleSelectorViewModel.cs`:
- Line 32: The parameterless WbsStyleSelectorViewModel constructor breaks the DI
contract; restore container-based constructor injection by replacing the
parameterless ctor with a constructor that accepts IServiceContainer (e.g.,
WbsStyleSelectorViewModel(IServiceContainer container)), call the base
ViewModelBase constructor appropriately, and resolve or accept required services
from the container (or as explicit ctor parameters) so dependencies are injected
consistently; ensure the class still inherits ViewModelBase and follow the
existing pattern used by other ViewModels for wiring services via
IServiceContainer.
In `@WpfApp2/ViewModels/XerEditorViewModel.cs`:
- Around line 240-245: RefreshTablesUI currently overwrites a table's Status to
"Updated" even when it was "Sheet Not Found"; modify the update logic in the
block that touches Tables and _parser.Tables (the code setting updated.Count and
updated.Status) so it preserves "Sheet Not Found" — e.g., only assign
updated.Status = "Updated" when the existing updated.Status is not "Sheet Not
Found" (or when the parser actually found rows for that table), leaving
updated.Status unchanged otherwise; ensure you reference Tables, _parser.Tables,
updated.Count and updated.Status in the change.
---
Nitpick comments:
In `@Som3a` Addin 2026/Ribbon1.cs:
- Around line 63-90: The current empty catch in the ribbon image-loading block
swallows failures; change it to catch Exception (e.g., catch (Exception ex)) and
log the error details (including the attempted imagePath and button.Name)
instead of ignoring them so corrupt or missing assets are diagnosable; keep the
behavior of not crashing the add-in but use a logging call (for example
System.Diagnostics.Trace/Debug or your add-in logger) to record ex.Message and
ex.ToString(), and leave ResizeRibbonImage and the rest of the flow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 05b76b61-48f3-4b3e-bc73-6003a32399cd
⛔ Files ignored due to path filters (24)
Som3a Addin 2026/Resources/150-1504985_open-split-icon-png.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/Coloringwbs.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/Com_Xer.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/ExportPaletteHex.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/Float_Path.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/GroupWbs.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/Xer_Editor.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/addin_setting.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/audit-data-analysis-icon-financial-assessment-vector-335920716.jpgis excluded by!**/*.jpgSom3a Addin 2026/Resources/btnDailyReport.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/btnLinksManager.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/btnProjectAnalysis.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/btnSafeClone.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/btnUnmergeFillDown.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/btnWorkspace.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/button1.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/button2.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/button22.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/color_setting.jpgis excluded by!**/*.jpgSom3a Addin 2026/Resources/comparsion.jpgis excluded by!**/*.jpgSom3a Addin 2026/Resources/editable-unmerge-table-cell-vector-260nw-2568331085.jpgis excluded by!**/*.jpgSom3a Addin 2026/Resources/pie.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/png-transparent-computer-icons-code-symbol-coding-miscellaneous-angle-text-thumbnail.pngis excluded by!**/*.pngSom3a Addin 2026/Resources/unmerage.pngis excluded by!**/*.png
📒 Files selected for processing (30)
Errortofix/Screenshot 2026-05-24 095252.mdErrortofix/Screenshot 2026-05-24 095334.mdSom3a Addin 2026.slnxSom3a Addin 2026/Properties/Resources.Designer.csSom3a Addin 2026/Properties/Resources.resxSom3a Addin 2026/Ribbon1.Designer.csSom3a Addin 2026/Ribbon1.csSom3a Addin 2026/Som3a Addin 2026.csprojSom3a Addin 2026/ThisAddIn.csSom3a.Shared/Models/AssignTradeCodesViewModel.csSom3a.Shared/Models/MainViewModel.csSom3a.Shared/Models/RelayCommand.csSom3a.Shared/Som3a.Shared.csprojWpfApp2/CompositionRoot.csWpfApp2/Controls/Shell/ShellWindow.xaml.csWpfApp2/Controls/Shell/WorkspaceHost.csWpfApp2/Fixpiecolors.xaml.csWpfApp2/MainWindow.xaml.csWpfApp2/ViewModels/CommandPaletteViewModel.csWpfApp2/ViewModels/FloatPathViewModel.csWpfApp2/ViewModels/LinksManagerViewModel.csWpfApp2/ViewModels/Primavera/PrimaveraCompareViewModel.csWpfApp2/ViewModels/Primavera/PrimaveraResultsViewModel.csWpfApp2/ViewModels/ProjectAnalysisViewModel.csWpfApp2/ViewModels/SubDailyReportViewModel.csWpfApp2/ViewModels/ToastViewModel.csWpfApp2/ViewModels/WbsStyleSelectorViewModel.csWpfApp2/ViewModels/XerEditorViewModel.csWpfApp2/Views/SettingsWindow.xamlspecs/009-mvvm-architecture-cleanup/tasks.md
💤 Files with no reviewable changes (1)
- Som3a Addin 2026/Properties/Resources.Designer.cs
✅ Files skipped from review due to trivial changes (2)
- Errortofix/Screenshot 2026-05-24 095334.md
- specs/009-mvvm-architecture-cleanup/tasks.md
Comment on lines
+1
to
+9
| { | ||
| "version": "2", | ||
| "formats": { | ||
| "markdown": { | ||
| "content": "Settings Error\n\nSystem.InvalidOperationException: A TwoWay or OneWayToSource binding cannot work on the read-only property 'IsDarkSelected' of type 'Som3a_WPF_UI.ViewModels.SettingsViewModel'.\n at MS.Internal.Data.PropertyPathWorker.CheckReadOnly(Object item, Object info)\n at MS.Internal.Data.PropertyPathWorker.ReplaceItem(Int32 k, Object newO, Object parent)\n at MS.Internal.Data.PropertyPathWorker.UpdateSourceValueState(Int32 k, ICollectionView collectionView, Object newValue, Boolean isASubPropertyChange)\n at MS.Internal.Data.ClrBindingWorker.AttachDataltem(Object item)\n at System.Windows.Data.BindingExpression.Activate(Object item)\n at System.Windows.Data.BindingExpression.AttachToContext(AttachAttempt attempt)\n at System.Windows.Data.BindingExpression.MS.Internal.Data.IDataBindEngineClient.AttachToContext(Boolean lastChance)\n at MS.Internal.Data.DataBindEngine.Task.Run(Boolean lastChance)\n at MS.Internal.Data.DataBindEngine.Run(Object arg)\n at System.Windows.ContextLayoutManager.fireLayoutUpdateEvent()\n at System.Windows.ContextLayoutManager.UpdateLayout()\n at System.Windows.Interop.HwndSource.SetLayoutSize()\n at System.Windows.Interop.HwndSource.set_RootVisualInternal(Visual value)\n at System.Windows.Window.SetRootVisual()\n at System.Windows.Window.SetRootVisualAndUpdateSTC()\n at System.Windows.Window.SetupInitialState(Double requestedTop, Double requestedLeft, Double requestedWidth, Double requestedHeight)\n at System.Windows.Window.CreateSourceWindow(Boolean duringShow)\n at System.Windows.Window.ShowHelper(Object booleanBox)\n at System.Windows.Window.ShowDialog()\n at Som3a_Addin_2026.Ribbon1.<>c.<addin_setting_Click>b__38_00 in C:\\Users\\mohamedabdelsamea\\source\\repos\\Som3aAddin 2026\\Som3a Addin 2026\\Ribbon1.cs:line 531\n at System.Windows.Threading.Dispatcher.Invoke(Action callback, DispatcherPriority priority, CancellationToken cancellationToken, TimeSpan timeout)\n at System.Windows.Threading.Dispatcher.Invoke(Action callback)\n at Som3a_Addin_2026.Ribbon1.addin_setting_Click(Object sender, RibbonControlEventArgs e) in C:\\Users\\mohamedabdelsamea\\source\\repos\\Som3a Addin 2026\\Som3a Addin 2026\\Ribbon1.cs:line 500\n\nOK", | ||
| "metadata": {} | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Remove this committed error dump from the repo.
This file contains absolute C:\Users\... stack-trace paths, which exposes personal/environment identifiers in source control history.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Errortofix/Screenshot` 2026-05-24 095252.md around lines 1 - 9, The file
"Screenshot 2026-05-24 095252.md" contains a committed stack-trace with absolute
user paths (references like Som3a_Addin_2026.Ribbon1.addin_setting_Click) and
must be removed from the repository and history: remove the file from the repo
(git rm --cached or git rm), commit and push the deletion, then purge the file
from history using git filter-repo or BFG (targeting the filename and any
matching stack-trace strings such as
Som3a_Addin_2026.Ribbon1.addin_setting_Click), add the filename/pattern to
.gitignore to prevent re-commit, and verify no other commits contain the same
absolute paths before pushing the cleaned history.
| "version": "2", | ||
| "formats": { | ||
| "markdown": { | ||
| "content": "Settings Error\n\nSystem.InvalidOperationException: A TwoWay or OneWayToSource binding cannot work on the read-only property 'IsDarkSelected' of type 'Som3a_WPF_UI.ViewModels.SettingsViewModel'.\n at MS.Internal.Data.PropertyPathWorker.CheckReadOnly(Object item, Object info)\n at MS.Internal.Data.PropertyPathWorker.ReplaceItem(Int32 k, Object newO, Object parent)\n at MS.Internal.Data.PropertyPathWorker.UpdateSourceValueState(Int32 k, ICollectionView collectionView, Object newValue, Boolean isASubPropertyChange)\n at MS.Internal.Data.ClrBindingWorker.AttachDataltem(Object item)\n at System.Windows.Data.BindingExpression.Activate(Object item)\n at System.Windows.Data.BindingExpression.AttachToContext(AttachAttempt attempt)\n at System.Windows.Data.BindingExpression.MS.Internal.Data.IDataBindEngineClient.AttachToContext(Boolean lastChance)\n at MS.Internal.Data.DataBindEngine.Task.Run(Boolean lastChance)\n at MS.Internal.Data.DataBindEngine.Run(Object arg)\n at System.Windows.ContextLayoutManager.fireLayoutUpdateEvent()\n at System.Windows.ContextLayoutManager.UpdateLayout()\n at System.Windows.Interop.HwndSource.SetLayoutSize()\n at System.Windows.Interop.HwndSource.set_RootVisualInternal(Visual value)\n at System.Windows.Window.SetRootVisual()\n at System.Windows.Window.SetRootVisualAndUpdateSTC()\n at System.Windows.Window.SetupInitialState(Double requestedTop, Double requestedLeft, Double requestedWidth, Double requestedHeight)\n at System.Windows.Window.CreateSourceWindow(Boolean duringShow)\n at System.Windows.Window.ShowHelper(Object booleanBox)\n at System.Windows.Window.ShowDialog()\n at Som3a_Addin_2026.Ribbon1.<>c.<addin_setting_Click>b__38_00 in C:\\Users\\mohamedabdelsamea\\source\\repos\\Som3aAddin 2026\\Som3a Addin 2026\\Ribbon1.cs:line 531\n at System.Windows.Threading.Dispatcher.Invoke(Action callback, DispatcherPriority priority, CancellationToken cancellationToken, TimeSpan timeout)\n at System.Windows.Threading.Dispatcher.Invoke(Action callback)\n at Som3a_Addin_2026.Ribbon1.addin_setting_Click(Object sender, RibbonControlEventArgs e) in C:\\Users\\mohamedabdelsamea\\source\\repos\\Som3a Addin 2026\\Som3a Addin 2026\\Ribbon1.cs:line 500\n\nOK", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expectation: no IsDarkSelected binding writes back to source.
rg -n -C3 --type=xaml 'IsDarkSelected|Mode=TwoWay|OneWayToSource'Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate all uses of IsDarkSelected
rg -n --hidden --no-ignore -S 'IsDarkSelected' .
# 2) Look specifically for binding modes around it in common XAML-like patterns
rg -n --hidden --no-ignore -S 'IsDarkSelected\s*.*(Mode\s*=\s*TwoWay|OneWayToSource|OneWayToSource|TwoWay)' .
# 3) Find the property definition in the viewmodel to confirm whether it is read-only
rg -n --hidden --no-ignore -S '(\bpublic\b\s+)?(bool|bool\?)\s+IsDarkSelected\b' .Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 2849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect binding around the reported line
echo "=== SettingsWindow.xaml around IsDarkSelected binding ==="
nl -ba WpfApp2/Views/SettingsWindow.xaml | sed -n '140,200p'
echo "=== SettingsViewModel.cs IsDarkSelected definition ==="
nl -ba WpfApp2/ViewModels/SettingsViewModel.cs | sed -n '1,130p'
echo "=== Errortofix dump file content (first 40 lines) ==="
nl -ba Errortofix/'Screenshot 2026-05-24 095252.md' | head -n 60Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== SettingsWindow.xaml around IsDarkSelected binding ==="
cat -n WpfApp2/Views/SettingsWindow.xaml | sed -n '140,200p'
echo "=== SettingsViewModel.cs IsDarkSelected definition ==="
cat -n WpfApp2/ViewModels/SettingsViewModel.cs | sed -n '1,140p'
echo "=== Errortofix dump file content (first 80 lines) ==="
cat -n "Errortofix/Screenshot 2026-05-24 095252.md" | head -n 80Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 13537
🏁 Script executed:
rg -n -g'*.xaml' --no-ignore 'IsDarkSelected' -C2 .
rg -n -g'*.xaml' --no-ignore -C2 'IsDarkSelected.*(Mode\s*=\s*(TwoWay|OneWayToSource))' .Repository: MohamedSom3a-00700/Som3a-Addin-2026
Length of output: 687
Remove the committed error dump (privacy) and re-check the IsDarkSelected binding mode.
Errortofix/Screenshot 2026-05-24 095252.mdincludes a local absolute path with a user name—remove/clean before release.- Current
WpfApp2/Views/SettingsWindow.xamlbindsIsDarkSelectedwithMode=OneWaywhileSettingsViewModel.IsDarkSelectedis read-only; if the exception is still happening, the offending binding is not theIsDarkSelectedoccurrence in this file and needs to be located in other XAML/style bindings that don’t setMode=OneWay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Errortofix/Screenshot` 2026-05-24 095252.md at line 5, Remove the committed
error dump from Errortofix/Screenshot 2026-05-24 095252.md (strip the local
absolute path and any sensitive info), and then fix the binding issue: locate
SettingsViewModel.IsDarkSelected and either make it writable (add a setter) or
change all XAML bindings that target IsDarkSelected to Mode=OneWay (do not use
TwoWay or OneWayToSource). Check WpfApp2/Views/SettingsWindow.xaml and search
all XAML, resource dictionaries and styles for "IsDarkSelected" (and references
from Ribbon1.addin_setting_Click if it opens the window) and update offending
bindings to OneWay or implement a setter on the SettingsViewModel property so
the binding mode used is valid.
|
|
||
| public Action? CloseWindow { get; set; } | ||
|
|
||
| public WbsStyleSelectorViewModel() |
There was a problem hiding this comment.
Restore container-based constructor injection for this ViewModel.
Switching to a parameterless constructor breaks the ViewModel DI contract used across this cleanup and makes dependency wiring inconsistent.
Suggested fix
- public WbsStyleSelectorViewModel()
+ public WbsStyleSelectorViewModel(IServiceContainer container)
{
SaveCommand = new RelayCommand(OnSave);
CancelCommand = new RelayCommand(() => CloseWindow?.Invoke());As per coding guidelines, **/ViewModels/**/*.cs: ViewModels must inherit from ViewModelBase and use constructor injection via IServiceContainer for all service dependencies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public WbsStyleSelectorViewModel() | |
| public WbsStyleSelectorViewModel(IServiceContainer container) | |
| { | |
| SaveCommand = new RelayCommand(OnSave); | |
| CancelCommand = new RelayCommand(() => CloseWindow?.Invoke()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WpfApp2/ViewModels/WbsStyleSelectorViewModel.cs` at line 32, The
parameterless WbsStyleSelectorViewModel constructor breaks the DI contract;
restore container-based constructor injection by replacing the parameterless
ctor with a constructor that accepts IServiceContainer (e.g.,
WbsStyleSelectorViewModel(IServiceContainer container)), call the base
ViewModelBase constructor appropriately, and resolve or accept required services
from the container (or as explicit ctor parameters) so dependencies are injected
consistently; ensure the class still inherits ViewModelBase and follow the
existing pattern used by other ViewModels for wiring services via
IServiceContainer.
Comment on lines
+240
to
+245
| var updated = Tables.FirstOrDefault(x => x.Name == t.Name); | ||
| if (updated != null) | ||
| { | ||
| updated.Count = _parser.Tables.FirstOrDefault(x => x.Name == t.Name)?.Rows.Count ?? 0; | ||
| updated.Status = "Updated"; | ||
| } |
There was a problem hiding this comment.
Preserve “Sheet Not Found” status after RefreshTablesUI().
Rows that failed excel.ReadTable(...) are currently re-labeled as "Updated" in the post-refresh pass, which masks import failures.
Suggested fix
var selected = Tables.Where(x => x.IsSelected).ToList();
+ var missingSheets = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var t in selected)
{
var updated = excel.ReadTable(t.Name);
if (updated == null)
{
- t.Status = "Sheet Not Found";
+ missingSheets.Add(t.Name);
continue;
}
var existing = _parser.Tables.FirstOrDefault(x => x.Name == t.Name);
@@
foreach (var t in selected)
{
var updated = Tables.FirstOrDefault(x => x.Name == t.Name);
if (updated != null)
{
+ if (missingSheets.Contains(t.Name))
+ {
+ updated.Status = "Sheet Not Found";
+ continue;
+ }
updated.Count = _parser.Tables.FirstOrDefault(x => x.Name == t.Name)?.Rows.Count ?? 0;
updated.Status = "Updated";
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WpfApp2/ViewModels/XerEditorViewModel.cs` around lines 240 - 245,
RefreshTablesUI currently overwrites a table's Status to "Updated" even when it
was "Sheet Not Found"; modify the update logic in the block that touches Tables
and _parser.Tables (the code setting updated.Count and updated.Status) so it
preserves "Sheet Not Found" — e.g., only assign updated.Status = "Updated" when
the existing updated.Status is not "Sheet Not Found" (or when the parser
actually found rows for that table), leaving updated.Status unchanged otherwise;
ensure you reference Tables, _parser.Tables, updated.Count and updated.Status in
the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the MVVM & Architecture Cleanup feature (009) — service container, event bus, module registry, full code-behind refactoring, and 17 unit tests across all core services.
Changes
Infrastructure (Phase 2-3)
MVVM Cleanup (Phase 4-5)
Unit Tests (T041-T049)
Bug Fixes
Verification
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation