[feature] Display skipped entries counters in inventory status#280
[feature] Display skipped entries counters in inventory status#280marwannettour merged 18 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds “skipped entries” counters to inventory status: a local counter during identification (from runtime monitor data) and a global aggregated counter in the final inventory statistics shown in the global status view.
Changes:
- Propagate and expose
SkippedEntriesCountviaInventoryMonitorData→ local identification view model + view (visible only when > 0). - Aggregate per-part skipped counts (
InventoryPart.SkippedCount) intoInventoryStatistics.TotalSkippedEntriesand surface it in the global status view model + view (visible only when > 0). - Extend unit/integration tests and add EN/FR localization strings for the new labels.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ByteSync.Client.UnitTests/ViewModels/Sessions/Inventories/InventoryLocalIdentificationViewModelTests.cs | Adds coverage for local skipped counter and visibility behavior. |
| tests/ByteSync.Client.UnitTests/ViewModels/Sessions/Inventories/InventoryGlobalStatusViewModelTests.cs | Adds coverage for global skipped counter derived from stats. |
| tests/ByteSync.Client.UnitTests/Services/TimeTracking/TimeTrackingComputerTests.cs | Adjusts timing/waiting strategy for time-tracking observable tests. |
| tests/ByteSync.Client.UnitTests/Services/Inventories/InventoryStatisticsServiceTests.cs | Adds coverage for aggregating skipped counts from inventory parts into global stats. |
| tests/ByteSync.Client.UnitTests/Business/Inventories/InventoryProcessDataTests.cs | Adds coverage ensuring monitor skipped count updates when recording skipped entries. |
| tests/ByteSync.Client.UnitTests/Business/Inventories/InventoryMonitorDataTests.cs | Extends HasNonZeroProperty test coverage to include skipped entries count. |
| tests/ByteSync.Client.IntegrationTests/Services/Inventories/InventoryBuilderAccessHandling_IntegrationTests.cs | Makes relative-path matching stricter/less ambiguous in access-handling tests. |
| src/ByteSync.Client/Views/Sessions/Inventories/InventoryLocalIdentificationView.axaml | Displays local skipped entries badge when ShowSkippedEntriesCount is true. |
| src/ByteSync.Client/Views/Sessions/Inventories/InventoryGlobalStatusView.axaml | Displays global skipped entries badge when ShowGlobalSkippedEntries is true. |
| src/ByteSync.Client/ViewModels/Sessions/Inventories/InventoryLocalIdentificationViewModel.cs | Adds SkippedEntriesCount + derived visibility flag sourced from monitor data. |
| src/ByteSync.Client/ViewModels/Sessions/Inventories/InventoryGlobalStatusViewModel.cs | Adds GlobalSkippedEntries + derived visibility flag sourced from computed stats. |
| src/ByteSync.Client/Services/Inventories/InventoryStatisticsService.cs | Aggregates skipped counts from inventory parts into InventoryStatistics. |
| src/ByteSync.Client/Services/Inventories/FullInventoryRunner.cs | Updates runner to use InventoryProcessData.GetInventories() API. |
| src/ByteSync.Client/Services/Inventories/BaseInventoryRunner.cs | Updates runner to use InventoryProcessData.GetInventories() API. |
| src/ByteSync.Client/Business/Inventories/InventoryStatistics.cs | Adds TotalSkippedEntries to global inventory statistics model. |
| src/ByteSync.Client/Business/Inventories/InventoryProcessData.cs | Adds monitor propagation for skipped entries; changes inventories accessor to GetInventories(). |
| src/ByteSync.Client/Business/Inventories/InventoryMonitorData.cs | Adds SkippedEntriesCount and includes it in HasNonZeroProperty(). |
| src/ByteSync.Client/Assets/Resources/Resources.resx | Adds EN localization keys for skipped entries labels (local/global). |
| src/ByteSync.Client/Assets/Resources/Resources.fr.resx | Adds FR localization keys for skipped entries labels (local/global). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ByteSync.Client/ViewModels/Sessions/Inventories/InventoryLocalIdentificationViewModel.cs
Show resolved
Hide resolved
tests/ByteSync.Client.UnitTests/Services/TimeTracking/TimeTrackingComputerTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This method maps InventoryBuildersto aList, but it returns nullwhenInventoryBuildersisnull. Is that intentional, or should it return an empty list instead? Also, are we sure every InventoryBuilderand itsInventory property are non-null here?
There was a problem hiding this comment.
Good catch, thanks. I updated this to make the contract explicit and non-nullable:
InventoryProcessData.InventoryBuildersis now initialized to an empty list ([]) and is non-nullable.GetInventories()now returnsList<Inventory>(non-nullable) and returns an empty list when there are no builders.- Call sites in
BaseInventoryRunnerandFullInventoryRunnerwere updated to remove null-forgiving operators.
On the second point: IInventoryBuilder.Inventory is non-nullable in the interface and InventoryBuilder initializes it in its constructor, so we rely on that invariant here.
There was a problem hiding this comment.
L.150 : Why do we keep both _skippedCount and SkippedEntriesCount? Is one of them really necessary in addition to the other?
There was a problem hiding this comment.
Good point, I simplified this.
_skippedCount was redundant, so I removed it and now SkippedCount is derived from _skippedCountsByReason.Values.Sum().
We still keep InventoryMonitorData.SkippedEntriesCount separately because it belongs to the observable monitor snapshot used by the UI stream, while SkippedCount/SkippedCountsByReason represent the process-level counters.
So there is now no duplicated private counter field in InventoryProcessData.
There was a problem hiding this comment.
It looks like InventoryProcessDatanow stores InventoryBuilders instead of a direct Inventories list, so GetInventories() is used to derive the actual Inventory objects. Is that intentional? Also, should GetInventories() return an empty list instead ofnullto avoid using! here?
There was a problem hiding this comment.
Yes, this is intentional.
InventoryProcessData keeps InventoryBuilders because the full-inventory phase needs more than the Inventory object itself (notably InventoryIndexer and SessionSettings).
I also applied your nullable-safety point:
InventoryBuildersis now non-nullable and initialized to[]GetInventories()now returns a non-nullList<Inventory>(empty when there are no builders)- null-forgiving operators were removed at call sites
|
marwannettour
left a comment
There was a problem hiding this comment.
Reviewed and everything looks good. No issues found.



Summary:
Key changes:
Notes/risks: