refactor: remove dead code and fix bugs in GeneralUpdate.Core#405
Merged
Conversation
…ethods - Remove #define AOT constant and SignalR Compile Remove entries from csproj - Remove Condition on SignalR.Client PackageReference for AOT builds - Remove #if !AOT blocks in GeneralUpdateBootstrap.cs (HubDownloadSource + LaunchSilentAsync) - Delete deprecated abstract/override methods (ExecuteStrategy, ExecuteStrategyAsync, StrategyFactory) Closes #402
- Add PacketDTO to HttpParameterJsonContext for source-generated AOT deserialization - Replace reflection-based Deserialize<PacketDTO> with source-gen version in HubDownloadSource - Remove try/catch in UpgradeHubService.StartAsync so hub connection failures propagate
- Fix WindowsStrategy/MacStrategy Execute() missing or broken overrides - Fix DownloadProgressReporter all-completed event firing prematurely - Fix StorageManager GetAllfiles null return causing NRE - Fix FileNode GetHashCode violating Equals contract - Fix ObjectTranslator unsafe cast, IsComplated typo - Delete unused FileTreeCore duplicate types, IEventManager interface - Remove dead fields: _diffMode, _script, _compressionStrategy, _hooks/_reporter - Remove empty InfixOrder methods, duplicate DriverDirectory, dead ctor - Extract shared StreamDownloadAsync to eliminate download loop duplication - Wire _customSkipOption into bootstrap execution flow - Clean stale XML doc params Closes #404
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs a cleanup/refactor of GeneralUpdate.Core by removing unused/dead code, deduplicating download logic, and addressing several correctness issues in strategies, download progress/events, hashing, and configuration.
Changes:
- Fixed strategy execution paths (Windows/macOS) and hardened a few runtime behaviors (e.g.,
StorageManager.GetAllfiles, saferObjectTranslatorcast). - Refactored multi-download completion signaling so the “all completed” event is dispatched once after orchestration completes, and deduplicated stream download loops.
- Removed dead/duplicate code and unused members across configuration, filesystem, events, and bootstrap layers.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Core/Strategy/WindowsStrategy.cs | Adds missing Execute() override to run the async pipeline synchronously. |
| src/c#/GeneralUpdate.Core/Strategy/MacStrategy.cs | Simplifies Execute() to delegate to ExecuteAsync(). |
| src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs | Removes unused diff-mode field. |
| src/c#/GeneralUpdate.Core/Strategy/AbstractStrategy.cs | Introduces optional Hooks/Reporter fields for strategy subclasses. |
| src/c#/GeneralUpdate.Core/Silent/SilentPollOrchestrator.cs | Removes now-unused hooks/reporter injection API. |
| src/c#/GeneralUpdate.Core/Network/VersionService.cs | Removes unused private constructor. |
| src/c#/GeneralUpdate.Core/JsonContext/HttpParameterJsonContext.cs | Extends source-gen JSON context to include PacketDTO types. |
| src/c#/GeneralUpdate.Core/Hubs/UpgradeHubService.cs | Simplifies StartAsync() logging/flow (exception now propagates). |
| src/c#/GeneralUpdate.Core/GeneralUpdate.Core.csproj | Adjusts package/compile conditions (removes AOT-specific exclusions). |
| src/c#/GeneralUpdate.Core/FileSystem/StorageManager.cs | Returns empty list instead of null from GetAllfiles() on exception. |
| src/c#/GeneralUpdate.Core/FileSystem/FileTreeCore/FileTreeEnumerator.cs | Removes duplicate/unused file tree enumerator and related types. |
| src/c#/GeneralUpdate.Core/FileSystem/FileTree.cs | Removes unused traversal method. |
| src/c#/GeneralUpdate.Core/FileSystem/FileNode.cs | Updates GetHashCode() implementation to align with equality intent. |
| src/c#/GeneralUpdate.Core/Event/IEventManager.cs | Removes unused event manager interface. |
| src/c#/GeneralUpdate.Core/Download/Sources/HubDownloadSource.cs | Switches to source-generated JSON deserialization for packets. |
| src/c#/GeneralUpdate.Core/Download/Progress/DownloadProgressReporter.cs | Removes per-asset “all completed” behavior and adds a static dispatcher. |
| src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs | Dispatches “all completed” once after WhenAll() completes. |
| src/c#/GeneralUpdate.Core/Download/MultiEventArgs/MutiDownloadCompletedEventArgs.cs | Renames completion flag parameter/property (but currently inconsistent). |
| src/c#/GeneralUpdate.Core/Download/Executors/OssDownloadExecutor.cs | Uses shared stream download loop helper. |
| src/c#/GeneralUpdate.Core/Download/Executors/HttpDownloadExecutor.cs | Extracts shared StreamDownloadAsync() loop. |
| src/c#/GeneralUpdate.Core/Configuration/ProcessInfo.cs | Removes stale XML doc parameter reference. |
| src/c#/GeneralUpdate.Core/Configuration/ObjectTranslator.cs | Makes packet hash extraction safe via pattern matching. |
| src/c#/GeneralUpdate.Core/Configuration/GlobalConfigInfo.cs | Removes redundant/hiding DriverDirectory property. |
| src/c#/GeneralUpdate.Core/Configuration/ConfiginfoBuilder.cs | Removes unused script field and builder method. |
| src/c#/GeneralUpdate.Core/Configuration/AbstractBootstrap.cs | Removes protected abstract members from base class API. |
| src/c#/GeneralUpdate.Core/Compress/CompressProvider.cs | Removes static strategy field; uses local strategy instance. |
| src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs | Wires custom skip option into execution; removes AOT preprocessor guards and dead overrides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
48
to
54
| public static void DispatchAllCompleted(bool success, List<(object, string)> details) | ||
| { | ||
| EventManager.Instance.Dispatch(null!, | ||
| new MultiAllDownloadCompletedEventArgs(success, details ?? new List<(object, string)>())); | ||
| } | ||
|
|
||
| /// <summary> |
| sw.Stop(); | ||
|
|
||
| // Dispatch all-completed event ONCE after all assets finish | ||
| var details = results.Select(r => ((object)r.Url, r.Success ? "success" : r.ErrorMessage ?? "failed")).ToList(); |
| } | ||
|
|
||
| public override int GetHashCode() => base.GetHashCode(); | ||
| public override int GetHashCode() => (Name?.GetHashCode() ?? 0) ^ (Hash?.GetHashCode() ?? 0); |
| public object Version { get; private set; } = version; | ||
|
|
||
| public bool IsComplated { get; private set; } = isComplated; | ||
| public bool isCompleted { get; private set; } = isCompleted; |
Comment on lines
26
to
30
| } | ||
|
|
||
| public abstract Task<TBootstrap> LaunchAsync(); | ||
| protected abstract void ExecuteStrategy(); | ||
| protected abstract Task ExecuteStrategyAsync(); | ||
| protected abstract TBootstrap StrategyFactory(); | ||
|
|
||
| public TBootstrap Option<T>(UpdateOption<T> option, T value) |
Previously hooks and reporter were injected into SilentPollOrchestrator but never actually used. This commit completes the integration: - Hooks.OnBeforeUpdateAsync() called before download (allows cancellation) - Hooks.OnDownloadCompletedAsync() called after successful download - Hooks.OnAfterUpdateAsync() called after pipeline completes - Hooks.OnUpdateErrorAsync() called on download or pipeline failures - Reporter reports: UpdateStarted, DownloadCompleted, UpdateApplied, UpdateFailed - Hooks & reporter calls are wrapped in try/catch so they never break the flow Closes #404
- Fix MutiDownloadCompletedEventArgs.IsCompleted PascalCase (was isCompleted) - Fix EventListenerTests to use IsCompleted instead of IsComplated - Fix DispatchAllCompleted to accept sender param (was passing null) - Fix DefaultDownloadOrchestrator to filter only failed results for FailedVersions - Fix FileNode.GetHashCode to use OrdinalIgnoreCase (match Equals contract)
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
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.
Closes #404
Summary
Systematic code review and cleanup — 22 files changed, +106/-292 lines. Build: 0 errors, 0 warnings.
Bug Fixes (7)
Dead Code Removed (9)
Deduplication (3)