Cards - Take migration info into consideration; Add "Used plugins/zipmods" #235

2025-10-30T02:15:10Z

ManlyMarco commented

2025-10-30 02:15:10 +00:00

(Migrated from github.com)

Also improves card loading speed.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-10-30 02:19:04 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

This PR enhances card dependency tracking by adding support for zipmod GUID migration and introducing new properties to track all used plugins and zipmods (not just missing ones). The changes optimize the dependency checking logic by using HashSet-based lookups instead of repeated LINQ queries.

Added support for zipmod GUID migration to correctly identify when old GUIDs should be considered "existing"
Introduced UsedPlugins and UsedZipmods properties to track all dependencies, not just missing ones
Optimized performance by pre-computing GUID HashSets instead of repeatedly iterating through collections

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
CardLoader.cs	Enhanced zipmod migration support, optimized dependency checking with HashSet lookups, and populated new UsedPlugins/UsedZipmods properties
Card.cs	Added UsedZipmods and UsedPlugins properties to track all card dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview This PR enhances card dependency tracking by adding support for zipmod GUID migration and introducing new properties to track all used plugins and zipmods (not just missing ones). The changes optimize the dependency checking logic by using HashSet-based lookups instead of repeated LINQ queries. - Added support for zipmod GUID migration to correctly identify when old GUIDs should be considered "existing" - Introduced `UsedPlugins` and `UsedZipmods` properties to track all dependencies, not just missing ones - Optimized performance by pre-computing GUID HashSets instead of repeatedly iterating through collections ### Reviewed Changes Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments. | File | Description | | ---- | ----------- | | CardLoader.cs | Enhanced zipmod migration support, optimized dependency checking with HashSet lookups, and populated new UsedPlugins/UsedZipmods properties | | Card.cs | Added UsedZipmods and UsedPlugins properties to track all card dependencies | --- 💡 <a href="/IllusionMods/KKManager/new/master/.github?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

src/KKManager.Core/Data/Cards/CardLoader.cs

					
				@ -84,3 +94,4 @@

				                                    CheckIfRequiredModsExist(card, allPlugins, allZipmods, existingPluginGuids, existingZipmodGuids);

				                            }

				                            catch (TargetInvocationException ex)

				                            {

The condition logic for checking MigrationType.StripAll is inconsistent with the code comment on line 86. The comment states 'No easy way to check StripAll so just assume the required zipmod is present', but the code includes StripAll without checking if the new GUID exists. Consider either: (1) removing the StripAll case from the condition to match the comment's intent, or (2) updating the comment to clarify that StripAll migrations are always included regardless of target GUID existence.

                                // For StripAll migrations, always include the old GUID regardless of whether the new GUID exists.

The condition logic for checking MigrationType.StripAll is inconsistent with the code comment on line 86. The comment states 'No easy way to check StripAll so just assume the required zipmod is present', but the code includes StripAll without checking if the new GUID exists. Consider either: (1) removing the StripAll case from the condition to match the comment's intent, or (2) updating the comment to clarify that StripAll migrations are always included regardless of target GUID existence. ```suggestion // For StripAll migrations, always include the old GUID regardless of whether the new GUID exists. ```

src/KKManager.Core/Data/Cards/CardLoader.cs

					
				@ -84,3 +94,4 @@

				                                    CheckIfRequiredModsExist(card, allPlugins, allZipmods, existingPluginGuids, existingZipmodGuids);

				                            }

				                            catch (TargetInvocationException ex)

				                            {

The migrated GUIDs query could enumerate MigrationList multiple times. Consider adding .ToList() after SelectMany(x => x.Manifest.MigrationList) to materialize the collection before filtering, or use a single pass with a foreach loop that directly adds to existingZipmodGuids. This would prevent potential multiple enumerations if MigrationList properties return IEnumerable without caching.

                                var migratedGuids = allZipmods.SelectMany(x => x.Manifest.MigrationList)
                                                              .ToList()

The migrated GUIDs query could enumerate MigrationList multiple times. Consider adding `.ToList()` after `SelectMany(x => x.Manifest.MigrationList)` to materialize the collection before filtering, or use a single pass with a `foreach` loop that directly adds to `existingZipmodGuids`. This would prevent potential multiple enumerations if MigrationList properties return IEnumerable without caching. ```suggestion var migratedGuids = allZipmods.SelectMany(x => x.Manifest.MigrationList) .ToList() ```

Sign in to join this conversation.

No reviewers