Feature: FoliCon icon overlay plugin ecosystem - #312
Conversation
- Introduced multiple new overlay definitions including "Alternate", "Faelpessoal", "Legacy", "Liaher", and "Windows 11" with respective properties and configurations. - Updated the PreviewerViewModel to load available overlays dynamically. - Enhanced PosterIconConfigViewModel to manage overlay selection with a new OverlayItemViewModel for better data binding. - Implemented a dynamic poster icon renderer that builds its visual tree based on the selected overlay definition. - Refactored the poster icon configuration view to utilize an ItemsControl for overlay selection, improving UI scalability and maintainability.
- Add Tests for the plugin feature
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 4 medium |
| ErrorProne | 1 high |
| CodeStyle | 5 minor |
🟢 Metrics 1488 complexity · 64 duplication
Metric Results Complexity 1488 Duplication 64
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Release 1: Core Plugin System (MVP) — ✅ COMPLETEAll 7 phases done. Build verified, 58 tests passing.
|
…y overlays via a GitHub-backed catalog.
```
GitHub (FoliCon-Overlays repo)
catalog.json ──→ OverlayRepositoryService ──→ OverlayStoreViewModel ──→ OverlayStore.xaml
overlays/{id}/ ↕
manifest.json OverlayProvider (reads from %AppData%/FoliCon/Overlays/)
overlay.json DynamicPosterIcon (renders previews)
preview.png OverlayPreviewCache (caches rendered previews)
*.png (assets)
```
Release 2: Overlay Store + Repository — ✅ COMPLETEPhase 1: Data Models — ✅ DONE
Phase 2: Repository Service — ✅ DONE
Phase 3: ViewModels — ✅ DONE
Phase 4: Overlay Store UI — ✅ DONE
Phase 5: Wire Up UI + DI — ✅ DONE
Phase 6: GitHub Repo Prep — ✅ DONE
Phase 7: Update Checker — ✅ DONE
Community Overlay Support — ✅ DONE
Testing — ✅ DONE
|
…ster both works add braces to if statements
…bmission with relevant tests
- Updated OverlayDesigner.xaml.cs to use localized title for color picker. - Enhanced OverlayStore.xaml with localization for various UI elements including buttons and tooltips. - Modified posterIconConfig.xaml to utilize localized strings for tooltips and button content. - Added localization tests to ensure all overlay strings are translated across multiple cultures. - Introduced smoke tests for OverlayDesigner and OverlayStore views to validate XAML loading and layout. - Implemented a new XamlLoadingCollection to prevent concurrent loading issues in tests. - Improved WpfTestHost to set up application resources for testing localized strings.
|
Release 3: Overlay Designer (v1.1) — ✅ COMPLETE (5 of 5 steps)Scope changed during planning: GitHub PR automation moved out to Release 3.1 (externally blocked on OAuth App
An author can now create an overlay from a template, edit it on a canvas, save drafts, export a store-ready package, Tests: 453 total (81 from Releases 1–2 + 372 added across Release 3 and the store rework), 0 warnings, 0 errors, Completion pass (2026-07-26): layer reordering and per-corner clip radius editing exposed in the designer; Store tag filter reworked (2026-07-26): was a single-select Defects found and fixedThirteen in total across Release 3 — three by tests, ten by manual use, which is the ratio worth remembering:
Two of these were more than UI polish:
|
|
This feature was worked in 3 Releases and many phases of each release, though it will be merged only once everything is completed Release 1: Core Plugin System (MVP)Release 2: Overlay Store + Repository
Release 3: Overlay Designer |
DineshSolanki
changed the title
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
FoliCon/internal-nlog.txt-1-5 (1)
1-5: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the generated runtime log from the repository.
FoliCon/internal-nlog.txtcontains verbose NLog diagnostics and a developer-specific absolute path on Line 5. Remove the file from version control and ignore it in.gitignoreto prevent future reintroduction.🤖 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 `@FoliCon/internal-nlog.txt` around lines 1 - 5, Remove the generated runtime log file FoliCon/internal-nlog.txt from version control, then add that filename to the repository’s .gitignore so future NLog diagnostics are not reintroduced.FoliCon/Modules/Overlays/OverlayProvider.cs-67-76 (1)
67-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn an absolute path for built-in overlays.
GetOverlayFolderPathreturnsResources\Overlays\<id>for built-in IDs. That path is relative.IOverlayProvider.GetOverlayFolderPathdocuments "the full path to an overlay's folder". Any caller that passes the result toFile.Exists,Directory.GetFiles, orPath.GetFullPathresolves it against the current working directory. The working directory is not the application directory when the app starts from a shortcut, from a shell with a differentcwd, or from a file-dialog callback.Base the built-in path on
AppContext.BaseDirectory.🐛 Proposed fix
if (OverlayConstants.BuiltInOverlayIds.Contains(id, StringComparer.OrdinalIgnoreCase)) { - return Path.Combine("Resources", "Overlays", id); + return Path.Combine(AppContext.BaseDirectory, "Resources", "Overlays", id); }🤖 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 `@FoliCon/Modules/Overlays/OverlayProvider.cs` around lines 67 - 76, Update GetOverlayFolderPath so built-in overlay IDs return an absolute path rooted at AppContext.BaseDirectory, while preserving the existing Resources/Overlays/<id> structure; leave user overlay path handling unchanged.FoliCon/Modules/Overlays/Internal/OverlayValidator.cs-182-189 (1)
182-189: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the pack-path bypass to built-in overlays.
ValidateAssetReferencereturns early for anyassetPaththat starts with/.ValidateDetailedruns for user-installed packages too, throughOverlayProvider.LoadUserOverlays. A community overlay can therefore set"imagePath": "/Resources/Overlays/liaher/base.png"and skip every asset check: relative-path safety, PNG extension, existence, and per-image size limit. The overlay then renders app-internal resources that the package does not ship.Gate the early return on
definition.IsBuiltIn, or reject leading-slash paths when the overlay folder is a user overlay folder.🛡️ Proposed fix
- private static void ValidateAssetReference(string overlayFolder, string assetPath, string field, OverlayValidationResult result) + private static void ValidateAssetReference(string overlayFolder, string assetPath, string field, + OverlayValidationResult result, bool isBuiltIn) { // Built-in overlays reference embedded resources with a leading slash; those are // resolved by DynamicPosterIcon against pack URIs and never touch the overlay folder. - if (assetPath.StartsWith('/')) + if (isBuiltIn && assetPath.StartsWith('/')) { return; }Pass
definition.IsBuiltIndown fromValidateDetailedthroughValidateLayers,ValidateLayer, andValidatePosterConfig.🤖 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 `@FoliCon/Modules/Overlays/Internal/OverlayValidator.cs` around lines 182 - 189, Restrict the leading-slash asset bypass in ValidateAssetReference to built-in overlays only. Propagate definition.IsBuiltIn from ValidateDetailed through ValidateLayers, ValidateLayer, and ValidatePosterConfig into ValidateAssetReference, and require it for the early return so user-installed overlays still undergo all path, extension, existence, and size validation.FoliCon/Modules/Overlays/OverlayConstants.cs-58-61 (1)
58-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a case-insensitive, read-only set for
BuiltInOverlayIds.The set uses the default ordinal comparer and is a mutable
HashSet<string>exposed as a public static field.Two consequences follow:
OverlayProvider.LoadUserOverlays(line 145) callsOverlayConstants.BuiltInOverlayIds.Contains(definition.Id). That call is case-sensitive. A community overlay with"id": "Liaher"passes the reserved-ID check.GetOverlayByIdmatches withOrdinalIgnoreCase, so the community overlay can then shadow or conflict with the built-inliaher. Note thatOverlayValidator.IdRegexrejects uppercase IDs only when the overlay reaches validation, which happens after this check and only produces a warning-level skip for other reasons.- Any caller can mutate the shared set at runtime.
🛡️ Proposed fix
- public static readonly HashSet<string> BuiltInOverlayIds = - [ - "legacy", "alternate", "liaher", "faelpessoal", "faelpessoal-horizontal", "windows11" - ]; + public static readonly IReadOnlySet<string> BuiltInOverlayIds = + new HashSet<string>(StringComparer.OrdinalIgnoreCase) + { + "legacy", "alternate", "liaher", "faelpessoal", "faelpessoal-horizontal", "windows11" + };🤖 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 `@FoliCon/Modules/Overlays/OverlayConstants.cs` around lines 58 - 61, Update BuiltInOverlayIds to use OrdinalIgnoreCase comparison and expose it as a read-only set rather than a mutable public HashSet. Preserve the existing built-in IDs and ensure OverlayProvider.LoadUserOverlays.Contains applies the same case-insensitive comparer.FoliCon/Modules/Overlays/OverlayProvider.cs-114-177 (1)
114-177: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGuard
_userOverlaysagainst concurrent reload and reads.
OverlayProvideris registered as a singleton inFoliCon/App.xaml.cs.Refresh()callsLoadUserOverlays(), which clears_userOverlaysand refills it item by item.OverlayUpdateChecker.CheckForUpdatesAsyncruns on a background task and callsGetUserOverlays(), and the store view models callGetAllOverlays(). A read that overlaps a refresh can throwInvalidOperationExceptionfrom the enumerator, or observe an empty or partial list and drop installed overlays from the UI.Two changes are needed:
- Build the new list locally and publish it with a single reference assignment, or protect all reads and writes with a lock.
- Set
definition.OverlayFolderPathbefore you add the definition to the list. Line 167 adds first and assigns at line 168, so a concurrent reader can observe a definition whoseOverlayFolderPathis still null.DynamicPosterIconthen cannot resolve relative image paths for that overlay.🔒️ Proposed fix sketch
- private readonly List<PosterOverlayDefinition> _userOverlays = []; + private volatile IReadOnlyList<PosterOverlayDefinition> _userOverlays = [];private void LoadUserOverlays() { - _userOverlays.Clear(); + var loaded = new List<PosterOverlayDefinition>(); if (!Directory.Exists(_userOverlaysPath)) { Logger.Debug("User overlays directory does not exist: {Path}", _userOverlaysPath); + _userOverlays = loaded; return; } @@ - _userOverlays.Add(definition); - definition.OverlayFolderPath = folder; + definition.OverlayFolderPath = folder; + loaded.Add(definition); } catch (Exception ex) { Logger.Error(ex, "Failed to load overlay from '{Path}'", jsonPath); } } - Logger.Info("Loaded {Count} user overlays", _userOverlays.Count); + _userOverlays = loaded; + Logger.Info("Loaded {Count} user overlays", loaded.Count); }Apply the same pattern to
_builtInOverlays.🤖 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 `@FoliCon/Modules/Overlays/OverlayProvider.cs` around lines 114 - 177, Update LoadUserOverlays to build a complete local collection, set definition.OverlayFolderPath before adding each definition, then publish the collection with one reference assignment instead of clearing and mutating _userOverlays. Apply the same atomic publication pattern to _builtInOverlays, and ensure GetUserOverlays and GetAllOverlays read stable published collections so refreshes cannot expose partial data or invalidate enumeration.FoliCon/Modules/Overlays/IOverlayRepositoryService.cs-45-70 (1)
45-70: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
_availableUpdatesthread-safe.
OverlayRepositoryServiceis a singleton.OverlayUpdateCheckerwrites_availableUpdateswhile the store reads it throughIsUpdateAvailable.UninstallOverlayAsyncandInvalidateCachealso mutate it. UseConcurrentDictionaryor protect every access with one lock.🤖 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 `@FoliCon/Modules/Overlays/IOverlayRepositoryService.cs` around lines 45 - 70, Make the _availableUpdates accesses in OverlayRepositoryService thread-safe because the singleton is concurrently read and mutated by OverlayUpdateChecker, IsUpdateAvailable, UninstallOverlayAsync, and InvalidateCache. Replace the backing collection with ConcurrentDictionary or consistently protect every read, write, and removal with a shared lock while preserving existing update-state behavior.FoliCon/Modules/Overlays/OverlayPreviewCache.cs-103-115 (1)
103-115: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDispose the
PosterIconafter each render.
PosterIconimplementsIDisposableand holds aMemoryStreamwith the full poster bytes (FoliCon/Models/Data/PosterIcon.cs, lines 61-69). This method creates one per overlay and never disposes it, soGetPreviewsAsyncleaks one stream for every overlay rendered.OverlayDesignerPreviewRenderer.RenderOnStaAsyncalready usesusing var posterIconfor the same reason.Create the
PosterIcononce perGetPreviewsAsynccall, or dispose it here.🐛 Proposed fix
return await StaRenderer.Default.EnqueueRender(() => { // Create PosterIcon on the STA thread (WPF objects require STA) - var posterIcon = CreatePosterIcon(posterPath, rating, ratingVisibility, mockupVisibility); + using var posterIcon = CreatePosterIcon(posterPath, rating, ratingVisibility, mockupVisibility); var dynamicIcon = new DynamicPosterIcon(overlay, posterIcon); using var bitmap = dynamicIcon.RenderToBitmap();🤖 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 `@FoliCon/Modules/Overlays/OverlayPreviewCache.cs` around lines 103 - 115, Dispose each PosterIcon created during the render flow to release its underlying MemoryStream. Update the render lambda in GetPreviewsAsync (or the enclosing render helper) to scope the CreatePosterIcon result with disposal, while preserving the existing DynamicPosterIcon and bitmap conversion behavior.FoliCon/Modules/Overlays/OverlayRepositoryService.cs-417-428 (1)
417-428: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProtect the rollback so a failure does not delete the previous version.
RollbackUpdateruns inside acatchblock. IfDirectory.DeleteorDirectory.Movethrows, for example because a file is locked by the running application, the new exception replaces the original failure. The user then loses the installed overlay, and the backup stays behind as{overlayId}_previous.UninstallOverlayAsynclater deletes that folder, so the previous version is unrecoverable.Catch and log inside
RollbackUpdateso the original exception still surfaces.🛡️ Proposed fix
- private static void RollbackUpdate(string finalDir, string backupDir) + private static void RollbackUpdate(string finalDir, string backupDir) { - if (Directory.Exists(finalDir)) - { - Directory.Delete(finalDir, true); - } - - if (Directory.Exists(backupDir)) - { - Directory.Move(backupDir, finalDir); - } + try + { + if (Directory.Exists(finalDir)) + { + Directory.Delete(finalDir, true); + } + + if (Directory.Exists(backupDir)) + { + Directory.Move(backupDir, finalDir); + } + } + catch (Exception ex) + { + Logger.Error(ex, "Rollback failed. Backup remains at {BackupDir}", backupDir); + } }🤖 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 `@FoliCon/Modules/Overlays/OverlayRepositoryService.cs` around lines 417 - 428, Update RollbackUpdate to handle exceptions from deleting finalDir or moving backupDir without allowing rollback failures to replace the original update exception. Catch and log rollback errors within RollbackUpdate, preserving the backup directory when restoration cannot complete so it remains recoverable by later cleanup or recovery logic.FoliCon/Modules/Overlays/OverlayRepositoryService.cs-313-333 (1)
313-333: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject assets without a declared hash, and cap the
overlay.jsonsize.Two gaps exist here:
- The SHA256 check runs only when
manifest.Sha256contains the asset key. A manifest that omits an entry installs the file with no integrity check. Fail the install instead when a hash is missing.- The size check excludes
OverlayConstants.overlayJsonFileName, sooverlay.jsonhas no upper bound.DownloadAssetsAsyncbuffers the whole response withGetByteArrayAsync, so a large file consumes unbounded memory.🛡️ Proposed change
- if (bytes.Length > OverlayConstants.maxImageSizeBytes && asset != OverlayConstants.overlayJsonFileName) + var maxBytes = asset == OverlayConstants.overlayJsonFileName + ? OverlayConstants.maxDefinitionSizeBytes + : OverlayConstants.maxImageSizeBytes; + if (bytes.Length > maxBytes) { throw new InvalidOperationException(string.Format( - Lang.OverlayInstallAssetTooLarge, asset, OverlayConstants.maxImageSizeBytes / 1024 / 1024)); + Lang.OverlayInstallAssetTooLarge, asset, maxBytes / 1024 / 1024)); } - // SHA256 verification - if (manifest.Sha256.TryGetValue(asset, out var expectedHash)) + if (!manifest.Sha256.TryGetValue(asset, out var expectedHash)) { - var actualHash = Convert.ToHexString(SHA256.HashData(bytes)).ToLowerInvariant(); - if (!string.Equals(actualHash, expectedHash, StringComparison.OrdinalIgnoreCase)) - { - Logger.Error("SHA256 mismatch for '{Asset}': expected {Expected}, got {Actual}", - asset, expectedHash, actualHash); - throw new InvalidOperationException( - string.Format(Lang.OverlayInstallHashMismatch, asset)); - } + Logger.Error("Manifest declares no SHA256 for '{Asset}'", asset); + throw new InvalidOperationException(string.Format(Lang.OverlayInstallHashMismatch, asset)); + } + + var actualHash = Convert.ToHexString(SHA256.HashData(bytes)).ToLowerInvariant(); + if (!string.Equals(actualHash, expectedHash, StringComparison.OrdinalIgnoreCase)) + { + Logger.Error("SHA256 mismatch for '{Asset}': expected {Expected}, got {Actual}", + asset, expectedHash, actualHash); + throw new InvalidOperationException( + string.Format(Lang.OverlayInstallHashMismatch, asset)); }
OverlayConstants.maxDefinitionSizeBytesis illustrative. Use the existing constant if one is already defined.🤖 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 `@FoliCon/Modules/Overlays/OverlayRepositoryService.cs` around lines 313 - 333, Update the asset validation in the method containing the “Size check” and “SHA256 verification” blocks to enforce the maximum size for every asset, including OverlayConstants.overlayJsonFileName, using the existing applicable size constant. Require manifest.Sha256 to contain every asset; when an entry is missing, reject the install with the existing hash-mismatch error flow, and retain the current case-insensitive comparison and logging for declared hashes that do not match.FoliCon/Modules/Overlays/OverlayRepositoryService.cs-509-534 (1)
509-534: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CheckForUpdatesnever runs on the cached catalog paths.
FetchCatalogFromNetworkAsyncis the only caller.FetchCatalogAsyncreturns early from the in-memory cache at line 108 and from the disk cache at line 132. After an application restart the disk cache is fresh for 24 hours, so_availableUpdatesstays empty andIsUpdateAvailablereturnsfalsefor every overlay. The store then reports no updates although newer versions exist in the catalog.Call
CheckForUpdateson the cached return paths as well.🐛 Proposed fix
if (_cachedCatalog != null && DateTime.UtcNow - _cacheTimestamp < CacheTtl) { Logger.Debug("Returning in-memory cached catalog"); + CheckForUpdates(_cachedCatalog); return _cachedCatalog; }Apply the same call before the disk-cache return at line 132.
🤖 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 `@FoliCon/Modules/Overlays/OverlayRepositoryService.cs` around lines 509 - 534, Update FetchCatalogAsync so both the in-memory cache return and the disk-cache return invoke CheckForUpdates with the cached catalog before returning. Preserve the existing FetchCatalogFromNetworkAsync behavior and ensure _availableUpdates is refreshed for every cached catalog path.FoliCon/ViewModels/PreviewerViewModel.cs-47-51 (1)
47-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
MediaTitleno longer affects the rendered previews.The
MediaTitlesetter only callsSetProperty. It does not callRebuildPreviewsAsync.OverlayPreviewCache.GetPreviewsAsyncis called with the poster path, rating, and the two visibility values, but not the title.
OverlayPreviewContextcarries aMediaTitlefield (seeFoliCon/Modules/Overlays/Designer/OverlayDesignerPreviewRenderer.csLines 8-33), so the renderer supports a title. The previewer's title input is now inert.Pass the title into the preview request and rebuild on change, or remove the input from
Previewer.xaml.🐛 Proposed fix
public string MediaTitle { get => _mediaTitle; - set => SetProperty(ref _mediaTitle, value); + set + { + if (SetProperty(ref _mediaTitle, value)) + { + _ = RebuildPreviewsAsync(); + } + } }Also applies to: 84-89
🤖 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 `@FoliCon/ViewModels/PreviewerViewModel.cs` around lines 47 - 51, Update PreviewerViewModel.MediaTitle so changing the title triggers RebuildPreviewsAsync, and include the current title when calling OverlayPreviewCache.GetPreviewsAsync to populate OverlayPreviewContext.MediaTitle. Preserve the existing preview rebuild behavior and ensure title changes are reflected in rendered previews.FoliCon/Views/OverlayDesigner.xaml-64-76 (1)
64-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winArrow-key
KeyBindings atUserControllevel will block text editing.
InputBindingson theUserControlare evaluated on the bubbling route from the focused element. The arrow keys therefore fireNudgeCommandwhile focus is inside aTextBoxor anhc:NumericUpDown.
NudgeCommandexecutes wheneverSelectedElement != null(OverlayDesignerViewModel.csLine 135), which is the normal editor state. The command marks the key handled, so the caret does not move inDisplayName(Line 492),OverlayId(Line 496),Description(Line 511),TagsText(Line 516), or the geometry editors (Lines 457-480).Move the arrow-key bindings onto
CanvasRoot, or gateNudgeCommandon canvas focus.🤖 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 `@FoliCon/Views/OverlayDesigner.xaml` around lines 64 - 76, Move the arrow-key KeyBindings from UserControl.InputBindings to the CanvasRoot input scope so they only invoke NudgeCommand while the canvas is focused. Preserve the existing direction and shift CommandParameters, and leave the UndoCommand, RedoCommand, and OpenHelpCommand bindings unchanged.FoliCon/ViewModels/posterIconConfigViewModel.cs-47-56 (1)
47-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
IconOverlaywhen the store removes the active overlay.
RemoveOverlayfalls back to the default when it deletes the active overlay (Lines 105-110). The store callback at Lines 50-55 does not.If a user uninstalls the active overlay inside the overlay store,
LoadOverlaysrebuildsAvailableOverlayswithout that ID.IconOverlaykeeps the stale ID, so no item hasIsActive == trueand theRadioButtongroup inFoliCon/Views/posterIconConfig.xaml(Lines 63-75) shows nothing selected. Icon generation then falls back silently at render time.Add the same check to
LoadOverlays, which covers both callbacks.🐛 Proposed fix
AvailableOverlays.Clear(); foreach (var overlay in allOverlays) { ... AvailableOverlays.Add(item); } + + // A removed overlay must not stay selected: the picker would show no + // checked item and icon generation would fall back without telling anyone. + if (AvailableOverlays.Count > 0 && AvailableOverlays.All(o => !o.IsActive)) + { + IconOverlay = OverlayConstants.DefaultOverlayId; + } }Also applies to: 142-167
🤖 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 `@FoliCon/ViewModels/posterIconConfigViewModel.cs` around lines 47 - 56, Update LoadOverlays to detect when the current IconOverlay is no longer present in AvailableOverlays and reset it to the default overlay, matching RemoveOverlay’s fallback behavior. Keep the existing store callback and removal flow unchanged so both paths use the centralized validation.FoliCon/ViewModels/PreviewerViewModel.cs-79-109 (1)
79-109: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftGuard
LoadPreviewsAsyncagainst overlapping invocations.
Rating,RatingVisibility, andOverlayVisibilityeach startRebuildPreviewsAsyncwithout awaiting it.SelectImagestarts another. Two runs can therefore be in flight at once.Both runs execute
OverlayPreviewItems.Clear()and thenAddper item around anawait. If the second run clears the collection while the first run is still adding, the bound list ends with a partial or duplicated set.InvalidateAllin one run also discards cache entries the other run just populated, so every overlay is re-rendered.If
Ratingis bound withUpdateSourceTrigger=PropertyChanged, each keystroke starts a full re-render of every overlay.Add a
CancellationTokenSourceper request and a short debounce, then apply the results only when the token is still current.🤖 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 `@FoliCon/ViewModels/PreviewerViewModel.cs` around lines 79 - 109, Update LoadPreviewsAsync and RebuildPreviewsAsync to serialize preview refresh requests using a per-request CancellationTokenSource and short debounce. Cancel and replace the previous request when Rating, RatingVisibility, OverlayVisibility, or SelectImage triggers a rebuild, pass the current token through loading, and only clear/add OverlayPreviewItems if that request remains current; ensure cancelled or stale requests do not apply results or invalidate newer cache data.FoliCon/Views/OverlayDesigner.xaml.cs-65-81 (1)
65-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
OnLoadedsubscribes the canvas handlers on everyLoadedevent.The
Containsguard at Line 67 protects the adorner creation only. Lines 73-76 run unconditionally.
FrameworkElement.Loadedcan raise more than once for the same instance, for example after the element is removed from and re-added to the visual tree. Each raise adds another set of handlers toCanvasRoot.OnCanvasMouseMovethen callsViewModel.ApplyGesturetwice per pointer move, and the handlers are never removed.🐛 Proposed fix
private void OnLoaded(object sender, RoutedEventArgs e) { if (!AdornerLayer.Children.Contains(_selectionOutline)) { AdornerLayer.Children.Add(_selectionOutline); CreateHandles(); + + CanvasRoot.MouseLeftButtonDown += OnCanvasMouseDown; + CanvasRoot.MouseMove += OnCanvasMouseMove; + CanvasRoot.MouseLeftButtonUp += OnCanvasMouseUp; + CanvasRoot.MouseLeave += OnCanvasMouseLeave; } - CanvasRoot.MouseLeftButtonDown += OnCanvasMouseDown; - CanvasRoot.MouseMove += OnCanvasMouseMove; - CanvasRoot.MouseLeftButtonUp += OnCanvasMouseUp; - CanvasRoot.MouseLeave += OnCanvasMouseLeave; - // Arrow-key nudge is bound at the dialog level, so focus must land here. Focus(); UpdateAdorner(); }🤖 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 `@FoliCon/Views/OverlayDesigner.xaml.cs` around lines 65 - 81, Update OnLoaded so CanvasRoot mouse handlers are subscribed only once, guarding the subscriptions with the same initialization state used for the adorner or a dedicated flag; ensure repeated Loaded events do not duplicate handlers while preserving the existing adorner setup and focus/update behavior.FoliCon/ViewModels/posterIconConfigViewModel.cs-220-267 (1)
220-267: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCache the community preview instead of decoding it in the property getter.
DemoIconPathis a computed property with no backing field and no change notification. Every read of the_branch callsLoadCommunityOverlayPreview, which performsFile.Existsand a full PNG decode.WPF re-reads a bound getter whenever the binding refreshes or the container is re-templated, for example when the
ItemsControlinFoliCon/Views/posterIconConfig.xaml(Lines 48-100) virtualizes or re-applies its template. The decode then runs again for each visible overlay.Store the result in a lazily initialized field.
♻️ Proposed fix
+ private object? _demoIconPath; + public object DemoIconPath => OverlayId switch { "legacy" => "/Resources/mockup_demos/simple/PosterIcon.ico", ... - _ => (object?)LoadCommunityOverlayPreview(OverlayId, IsBuiltIn) ?? "/Resources/icons/NoPosterAvailable.png" + _ => _demoIconPath ??= + (object?)LoadCommunityOverlayPreview(OverlayId, IsBuiltIn) + ?? "/Resources/icons/NoPosterAvailable.png" };🤖 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 `@FoliCon/ViewModels/posterIconConfigViewModel.cs` around lines 220 - 267, Cache the community preview result used by DemoIconPath in a lazily initialized backing field so repeated getter reads do not rerun LoadCommunityOverlayPreview, including its file check and PNG decode. Preserve the existing built-in mappings and fallback behavior, and ensure the cached value is initialized only for the current OverlayId/IsBuiltIn state using the existing LoadCommunityOverlayPreview method.FoliCon/ViewModels/OverlayDesignerViewModel.cs-144-144 (1)
144-144: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap the async command body so exceptions cannot escape as
async void.
new DelegateCommand(async () => await ExportPackageAsync(), ...)compiles the lambda toasync void. Any exception thatExportPackageAsyncdoes not catch is rethrown on the synchronization context and terminates the process.
ExportPackageAsyncfilters onlyIOExceptionandUnauthorizedAccessException.OpenSubmissionPanelAsync(Line 1086) awaits_submissionGuide.CheckAsync, which performs network work and can throwHttpRequestExceptionorTaskCanceledException. Those escape the filter.🛡️ Proposed fix
- ExportPackageCommand = new DelegateCommand(async () => await ExportPackageAsync(), () => CanExport && !IsBusy); + ExportPackageCommand = new DelegateCommand(() => _ = RunExportAsync(), () => CanExport && !IsBusy);Add the guarded wrapper:
private async Task RunExportAsync() { try { await ExportPackageAsync(); } catch (Exception ex) { Logger.Error(ex, "Unhandled error while exporting overlay '{Id}'", _document.Id); StatusMessage = string.Format(Lang.OverlayDesignerExportFailedWithReason, ex.Message); IsBusy = false; } }🤖 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 `@FoliCon/ViewModels/OverlayDesignerViewModel.cs` at line 144, Update ExportPackageCommand to invoke a guarded Task-returning wrapper instead of passing ExportPackageAsync directly as an async void lambda. Add RunExportAsync near the command-related methods, await ExportPackageAsync inside it, catch any unhandled Exception, log it with the document ID, update the export failure status, and reset IsBusy before returning.FoliCon/Modules/Overlays/Designer/OverlayDraftStore.cs-157-177 (1)
157-177: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftConstrain every path component to the drafts root before the store writes or deletes.
OverlayDraftStorebuilds file and folder paths from values that originate inoverlay.json— the asset names and the overlay ID — and neither value is checked for path separators,..segments, or a rooted path.Path.Combinethen resolves outsideDraftsRoot, and the store copies, moves, or recursively deletes there.
FoliCon/Modules/Overlays/Designer/OverlayDraftStore.cs#L157-L177: reject rooted asset paths and verifyPath.GetFullPath(target)stays understagingPathbeforeFile.Copy.FoliCon/Modules/Overlays/Designer/OverlayDraftStore.cs#L53-L59: reject adocument.Idthat is not a plain folder name, sofinalPath,Commit, andDeletecannot act outside_draftsRoot.FoliconTest/OverlayDraftStoreTests.cs#L37-L46: add tests that save a document with an escaping asset path and with an escaping ID, and assert nothing is created outsideDraftsRoot.🤖 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 `@FoliCon/Modules/Overlays/Designer/OverlayDraftStore.cs` around lines 157 - 177, Constrain all paths derived from overlay.json to DraftsRoot: in FoliCon/Modules/Overlays/Designer/OverlayDraftStore.cs lines 157-177, reject rooted or escaping asset paths and verify the full target remains under stagingPath before File.Copy; in FoliCon/Modules/Overlays/Designer/OverlayDraftStore.cs lines 53-59, validate document.Id is a plain folder name so finalPath, Commit, and Delete remain within _draftsRoot; in FoliconTest/OverlayDraftStoreTests.cs lines 37-46, add save tests for escaping asset paths and IDs and assert nothing is created outside DraftsRoot.FoliCon/Modules/Overlays/Designer/OverlayExporter.cs-235-251 (1)
235-251: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftNested asset paths are copied but never manifested or installed.
CopyReferencedAssetscreates subdirectories at line 248, so an asset such asart/base.pngis copied into{staging}/art/base.png.Two later steps only handle a flat layout:
WriteManifestat line 277 usesDirectory.GetFiles(stagingPath)without recursion. A nested asset is missing fromAssetsand fromSha256.InstallLocallyat line 164 usesDirectory.GetFiles(packagePath)without recursion. A nested asset is not installed, so the installed overlay fails to render.Choose one of two fixes. Either reject nested asset paths at export time, or enumerate recursively in both places and store relative paths.
🐛 Option A: recursive enumeration with relative keys
- var files = Directory.GetFiles(stagingPath) - .Select(Path.GetFileName) - .OfType<string>() + var files = Directory.GetFiles(stagingPath, "*", SearchOption.AllDirectories) + .Select(f => Path.GetRelativePath(stagingPath, f).Replace('\\', '/')) // Stable order so the manifest is byte-identical across exports. .OrderBy(f => f, StringComparer.Ordinal) .ToArray();- foreach (var file in Directory.GetFiles(packagePath)) + foreach (var file in Directory.GetFiles(packagePath, "*", SearchOption.AllDirectories)) { - File.Copy(file, Path.Combine(staging, Path.GetFileName(file)), overwrite: true); + var target = Path.Combine(staging, Path.GetRelativePath(packagePath, file)); + Directory.CreateDirectory(Path.GetDirectoryName(target)!); + File.Copy(file, target, overwrite: true); }Run the following script to check whether the validator already forbids nested asset paths:
#!/bin/bash # Look for path-separator rules on imagePath / opacityMaskPath / fontSource. fd -i 'OverlayValidator*.cs' --exec rg -n -C 6 'imagePath|ImagePath|OpacityMaskPath|FontSource|DirectorySeparator|Contains\(.[/\\]' {}🤖 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 `@FoliCon/Modules/Overlays/Designer/OverlayExporter.cs` around lines 235 - 251, Update WriteManifest and InstallLocally to enumerate staging/package files recursively so nested assets are included in the manifest, hashes, and local installation. Store each file using its path relative to the respective root, preserving nested paths such as art/base.png; keep the existing flat-file behavior unchanged.FoliconTest/OverlayTemplateProviderTests.cs-12-15 (1)
12-15: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the
XamlLoadingCollectionattribute to this class.This class creates a
WpfTestHoston line 15 and callsWpfTestHost.Invokein ten tests. Every other suite in this change that usesWpfTestHostdeclares[Collection(XamlLoadingCollection.name)]:OverlayTemplatePickerTestsline 13,OverlayDesignerGeometryTestsline 14,OverlayDesignerIntegrationTestsline 22,OverlayDesignerPreviewRendererTestsline 12, andOverlayDesignerViewSmokeTestsline 22.Without the attribute, xUnit runs this class in parallel with those collections. It then initializes a second WPF host and queues work on the shared STA thread while another collection uses it. That causes intermittent, order-dependent failures.
💚 Proposed fix
+[Collection(XamlLoadingCollection.name)] public class OverlayTemplateProviderTests : IDisposable {🤖 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 `@FoliconTest/OverlayTemplateProviderTests.cs` around lines 12 - 15, Add the xUnit [Collection(XamlLoadingCollection.name)] attribute to the OverlayTemplateProviderTests class so its WpfTestHost usage is serialized with the other XAML-loading test suites; leave the existing test implementation unchanged.FoliconTest/OverlayDesignerViewSmokeTests.cs-62-86 (1)
62-86: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the Prism view-model factory after the test.
SetDefaultViewModelFactorychanges process-wide static state. This test leaves a factory that returns the disposedviewModel. A laterAutoWireViewModelpath can receive that instance. Restore Prism 9's default factory infinally:finally { + ViewModelLocationProvider.SetDefaultViewModelFactory( + type => Activator.CreateInstance(type)); viewModel.Dispose(); }🤖 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 `@FoliconTest/OverlayDesignerViewSmokeTests.cs` around lines 62 - 86, Restore Prism’s default view-model factory in the finally block after disposing the test view model, using the Prism 9 reset/default-factory API. Keep the existing SetDefaultViewModelFactory setup and test flow unchanged, ensuring later AutoWireViewModel calls cannot receive the disposed viewModel.FoliconTest/OverlayStoreViewModelTests.cs-405-409 (1)
405-409: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun the tag-filter helpers on the dispatcher thread.
VisibleOverlaysis created byCollectionViewSource.GetDefaultView(Overlays)inside the view-model constructor, andLoadTagFixtureAsyncconstructs the view model throughWpfTestHost.Invoke(Line 427). The resultingICollectionViewhas thread affinity to the STA dispatcher thread.VisibleandSelectrun on the xUnit thread instead, as dovm.SearchQuery(Line 398),vm.ClearTagFiltersCommand.Execute()(Line 364), andvm.RefreshCommand.Execute()(Line 385). Each of these reaches the collection view from the wrong thread and can throw at runtime. The tests earlier in this file (Lines 105-115, 132-134) already wrap the same operations inWpfTestHost.Invoke. Make the tag-filter tests consistent.🔒️ Proposed fix
private static List<OverlayCardViewModel> Visible(OverlayStoreViewModel vm) => - [.. vm.VisibleOverlays.Cast<OverlayCardViewModel>()]; + WpfTestHost.Invoke(() => vm.VisibleOverlays.Cast<OverlayCardViewModel>().ToList()); private static void Select(OverlayStoreViewModel vm, string tag, bool selected = true) => - vm.TagFilters.Single(t => string.Equals(t.Tag, tag, StringComparison.OrdinalIgnoreCase)).IsSelected = selected; + WpfTestHost.Invoke(() => + vm.TagFilters.Single(t => string.Equals(t.Tag, tag, StringComparison.OrdinalIgnoreCase)).IsSelected = selected);Wrap the command executions and the
SearchQueryassignment inWpfTestHost.Invokeas well.🤖 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 `@FoliconTest/OverlayStoreViewModelTests.cs` around lines 405 - 409, Update the tag-filter tests to access the dispatcher-affine collection view only through WpfTestHost.Invoke. Wrap the Visible and Select helper bodies, plus SearchQuery assignments and ClearTagFiltersCommand.Execute and RefreshCommand.Execute calls, in dispatcher invocations while preserving their existing assertions and test behavior.FoliconTest/WpfTestHost.cs-24-69 (1)
24-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAn initialization failure hangs the whole test run.
ready.Set()runs at Line 58, after the application setup at Lines 33-54. If the reflection call or a themeResourceDictionaryload throws,Set()never runs andready.Wait()at Line 67 blocks forever. The test process then hangs instead of failing. Capture the exception, signal the event in afinally, and rethrow on the calling thread.🛡️ Proposed fix
Dispatcher dispatcher = null!; + Exception? startupFailure = null; using var ready = new ManualResetEventSlim(false); var staThread = new Thread(() => { - // Creating Application sets Application.Current (required for relative pack URIs). - // Set BaseUri to FoliCon assembly so relative URIs like /Resources/... resolve - // against FoliCon.dll resources, not the test runner assembly. - if (Application.Current == null) + try { - ... + // Creating Application sets Application.Current (required for relative pack URIs). + if (Application.Current == null) + { + // ... existing setup ... + } + dispatcher = Dispatcher.CurrentDispatcher; } - - dispatcher = Dispatcher.CurrentDispatcher; - // ReSharper disable once AccessToDisposedClosure - Set() runs before ready is disposed - ready.Set(); + catch (Exception ex) + { + startupFailure = ex; + } + finally + { + // ReSharper disable once AccessToDisposedClosure - Set() runs before ready is disposed + ready.Set(); + } + + if (startupFailure != null) + { + return; + } Dispatcher.Run(); }) @@ ready.Wait(); + if (startupFailure != null) + { + throw new InvalidOperationException("WPF test host failed to start.", startupFailure); + } return dispatcher;🤖 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 `@FoliconTest/WpfTestHost.cs` around lines 24 - 69, Update StartDispatcherThread so exceptions during application setup, including BaseUri reflection or ThemeSources resource loading, are captured from the STA thread, and always signal ready in a finally block. After ready.Wait() returns, rethrow the captured exception on the calling thread before returning the dispatcher, while preserving normal Dispatcher.Run startup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 53e3c24e-db2d-4c7a-9232-50573b38f66f
⛔ Files ignored due to path filters (6)
FoliconTest/Resources/ReferenceOverlays/alternate_reference.pngis excluded by!**/*.pngFoliconTest/Resources/ReferenceOverlays/faelpessoal-horizontal_reference.pngis excluded by!**/*.pngFoliconTest/Resources/ReferenceOverlays/faelpessoal_reference.pngis excluded by!**/*.pngFoliconTest/Resources/ReferenceOverlays/legacy_reference.pngis excluded by!**/*.pngFoliconTest/Resources/ReferenceOverlays/liaher_reference.pngis excluded by!**/*.pngFoliconTest/Resources/ReferenceOverlays/windows11_reference.pngis excluded by!**/*.png
📒 Files selected for processing (118)
.gitignoreFoliCon/App.xamlFoliCon/App.xaml.csFoliCon/FoliCon.csprojFoliCon/Models/Constants/GlobalVariables.csFoliCon/Models/Data/OverlayCatalog.csFoliCon/Models/Data/OverlayLayerConfig.csFoliCon/Models/Data/OverlayManifest.csFoliCon/Models/Data/OverlayStatusFilterOption.csFoliCon/Models/Data/PosterOverlayDefinition.csFoliCon/Models/Enums/IconOverlay.csFoliCon/Models/Enums/OverlayStatusFilter.csFoliCon/Models/Enums/OverlayStoreSection.csFoliCon/Models/Usings.csFoliCon/Modules/Convertor/LocalizedFormatConverter.csFoliCon/Modules/Extension/DialogServiceExtensions.csFoliCon/Modules/Extension/StreamExtension.csFoliCon/Modules/LangProvider.csFoliCon/Modules/Overlays/Designer/IOverlayEditCommand.csFoliCon/Modules/Overlays/Designer/OverlayDesignerDocument.csFoliCon/Modules/Overlays/Designer/OverlayDesignerPreviewRenderer.csFoliCon/Modules/Overlays/Designer/OverlayDraftStore.csFoliCon/Modules/Overlays/Designer/OverlayEditHistory.csFoliCon/Modules/Overlays/Designer/OverlayElementKind.csFoliCon/Modules/Overlays/Designer/OverlayExporter.csFoliCon/Modules/Overlays/Designer/OverlayGeometry.csFoliCon/Modules/Overlays/Designer/OverlayPackageLoader.csFoliCon/Modules/Overlays/Designer/OverlayPackageSerializer.csFoliCon/Modules/Overlays/Designer/OverlaySubmissionGuide.csFoliCon/Modules/Overlays/Designer/OverlayTemplateProvider.csFoliCon/Modules/Overlays/IOverlayProvider.csFoliCon/Modules/Overlays/IOverlayRepositoryService.csFoliCon/Modules/Overlays/Internal/OverlayValidator.csFoliCon/Modules/Overlays/OverlayConstants.csFoliCon/Modules/Overlays/OverlayPreviewCache.csFoliCon/Modules/Overlays/OverlayProvider.csFoliCon/Modules/Overlays/OverlayRepositoryService.csFoliCon/Modules/Overlays/OverlayUpdateChecker.csFoliCon/Modules/Overlays/OverlayValidationResult.csFoliCon/Modules/Validation/ApiKeyValidator.csFoliCon/Modules/utils/IconUtils.csFoliCon/Properties/Langs/Lang.Designer.csFoliCon/Properties/Langs/Lang.ar.resxFoliCon/Properties/Langs/Lang.es.resxFoliCon/Properties/Langs/Lang.hi.resxFoliCon/Properties/Langs/Lang.ja.resxFoliCon/Properties/Langs/Lang.pt.resxFoliCon/Properties/Langs/Lang.resxFoliCon/Properties/Langs/Lang.ru.resxFoliCon/Properties/Langs/Lang.zh.resxFoliCon/Resources/Overlays/alternate/overlay.jsonFoliCon/Resources/Overlays/faelpessoal-horizontal/overlay.jsonFoliCon/Resources/Overlays/faelpessoal/overlay.jsonFoliCon/Resources/Overlays/legacy/overlay.jsonFoliCon/Resources/Overlays/liaher/overlay.jsonFoliCon/Resources/Overlays/windows11/overlay.jsonFoliCon/ViewModels/MainWindowViewModel.csFoliCon/ViewModels/OverlayCardViewModel.csFoliCon/ViewModels/OverlayDesignerViewModel.csFoliCon/ViewModels/OverlayElementViewModel.csFoliCon/ViewModels/OverlayStoreViewModel.csFoliCon/ViewModels/OverlayTagFilterViewModel.csFoliCon/ViewModels/OverlayTemplateCardViewModel.csFoliCon/ViewModels/PreviewerViewModel.csFoliCon/ViewModels/posterIconConfigViewModel.csFoliCon/Views/DynamicPosterIcon.xamlFoliCon/Views/DynamicPosterIcon.xaml.csFoliCon/Views/MainWindow.xamlFoliCon/Views/OverlayDesigner.xamlFoliCon/Views/OverlayDesigner.xaml.csFoliCon/Views/OverlayStore.xamlFoliCon/Views/OverlayStore.xaml.csFoliCon/Views/PosterIcon.xamlFoliCon/Views/PosterIcon.xaml.csFoliCon/Views/PosterIconAlt.xamlFoliCon/Views/PosterIconAlt.xaml.csFoliCon/Views/PosterIconFaelpessoal.xamlFoliCon/Views/PosterIconFaelpessoal.xaml.csFoliCon/Views/PosterIconFaelpessoalHorizontal.xamlFoliCon/Views/PosterIconFaelpessoalHorizontal.xaml.csFoliCon/Views/PosterIconLiaher.xamlFoliCon/Views/PosterIconLiaher.xaml.csFoliCon/Views/PosterIconWindows11.xamlFoliCon/Views/PosterIconWindows11.xaml.csFoliCon/Views/Previewer.xamlFoliCon/Views/posterIconConfig.xamlFoliCon/internal-nlog.txtFolicon.slnFoliconTest/DynamicPosterIconParityTests.csFoliconTest/FoliconTest.csprojFoliconTest/GlobalUsings.csFoliconTest/GoldenImageParityTests.csFoliconTest/OverlayDesignerDocumentTests.csFoliconTest/OverlayDesignerGeometryTests.csFoliconTest/OverlayDesignerIntegrationTests.csFoliconTest/OverlayDesignerPreviewRendererTests.csFoliconTest/OverlayDesignerViewModelTests.csFoliconTest/OverlayDesignerViewSmokeTests.csFoliconTest/OverlayDraftStoreTests.csFoliconTest/OverlayEditCommandTests.csFoliconTest/OverlayExporterTests.csFoliconTest/OverlayLocalizationTests.csFoliconTest/OverlayManifestTests.csFoliconTest/OverlayPackageLoaderTests.csFoliconTest/OverlayProviderTests.csFoliconTest/OverlayRepositoryServiceTests.csFoliconTest/OverlayStoreStyleResolutionTests.csFoliconTest/OverlayStoreViewModelTests.csFoliconTest/OverlayStoreViewSmokeTests.csFoliconTest/OverlaySubmissionGuideTests.csFoliconTest/OverlayTemplatePickerTests.csFoliconTest/OverlayTemplateProviderTests.csFoliconTest/OverlayValidatorDetailedTests.csFoliconTest/OverlayValidatorTests.csFoliconTest/PosterIconConfigViewSmokeTests.csFoliconTest/WpfTestHost.csFoliconTest/XamlLoadingCollection.csREADME.md
💤 Files with no reviewable changes (2)
- FoliCon/Modules/Validation/ApiKeyValidator.cs
- FoliCon/Models/Enums/IconOverlay.cs
| var finalPath = Path.Combine(destinationRoot, document.Id); | ||
| if (Directory.Exists(finalPath) && !overwrite) | ||
| { | ||
| return OverlayExportResult.Failure( | ||
| string.Format(Lang.OverlayExportFolderExists, document.Id)); | ||
| } | ||
|
|
||
| // Staged as a sibling so the final move is a rename on the same volume. | ||
| var stagingPath = Path.Combine(destinationRoot, $".{document.Id}.export-tmp"); | ||
|
|
||
| try | ||
| { | ||
| PrepareStagingFolder(stagingPath); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Validate document.Id before you compose any path.
Line 87 and line 95 place document.Id directly into Path.Combine and into an interpolated folder name. The ID is only validated at line 107, after PrepareStagingFolder already ran. PrepareStagingFolder calls SafeDelete, which performs Directory.Delete(path, recursive: true).
If document.Id contains a path separator or .., the staging path escapes destinationRoot. The recursive delete then targets a folder outside the destination. The author controls this value through the designer.
The existing test uses "Not A Valid Id", which contains no separator, so this case is not covered.
Add an ID format check before line 87.
🔒️ Proposed fix: reject unsafe IDs first
if (string.IsNullOrWhiteSpace(document.Id))
{
return OverlayExportResult.Failure(Lang.OverlayExportIdRequired);
}
+ // The ID becomes a folder name. Reject anything that could escape destinationRoot
+ // before it reaches Path.Combine or the recursive staging cleanup.
+ if (document.Id != Path.GetFileName(document.Id) ||
+ document.Id is "." or ".." ||
+ document.Id.AsSpan().IndexOfAny(Path.GetInvalidFileNameChars()) >= 0)
+ {
+ return OverlayExportResult.Failure(Lang.OverlayExportIdRequired);
+ }
+
var finalPath = Path.Combine(destinationRoot, document.Id);Run the following script to check whether any caller validates the ID before calling ExportAsync:
#!/bin/bash
# Find ExportAsync call sites and any ID validation rules.
rg -n -C 6 --type=cs '\.ExportAsync\s*\('
# Inspect the validator's ID rules.
fd -i 'OverlayValidator*.cs' --exec rg -n -C 4 -i 'id' {}🤖 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 `@FoliCon/Modules/Overlays/Designer/OverlayExporter.cs` around lines 87 - 99,
Validate document.Id before constructing finalPath or stagingPath in
OverlayExporter’s export flow, rejecting path separators, traversal segments,
and other values not permitted by the existing overlay ID rules. Reuse the
established validator or validation logic, and return the appropriate
OverlayExportResult.Failure without invoking PrepareStagingFolder when
validation fails.
| var tmpDir = Path.Combine(_userOverlaysDir, $"{entry.Id}.tmp"); | ||
| var finalDir = Path.Combine(_userOverlaysDir, entry.Id); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Path traversal through remote-controlled identifiers in OverlayRepositoryService. The catalog and the manifest are fetched over HTTP, and their Id and asset-name strings are passed straight to Path.Combine without confinement. Path.Combine returns the second argument when it is rooted, and it does not collapse .. segments. A compromised repository, a hijacked FOLICON_OVERLAY_REPO_URL, or a malicious .overlay-repo-url file can therefore delete and write files anywhere the user can.
FoliCon/Modules/Overlays/OverlayRepositoryService.cs#L227-L228: validateentry.Idagainst a strict slug pattern before you buildtmpDirandfinalDir, and apply the same validation inUpdateOverlayAsync,UninstallOverlayAsync,IsOverlayInstalled, andGetInstalledVersion.FoliCon/Modules/Overlays/OverlayRepositoryService.cs#L305-L307: resolvePath.Combine(targetDir, asset)withPath.GetFullPathand reject the asset when the result is not undertargetDir, beforeDirectory.CreateDirectoryandFile.WriteAllBytesAsyncrun.
📍 Affects 1 file
FoliCon/Modules/Overlays/OverlayRepositoryService.cs#L227-L228(this comment)FoliCon/Modules/Overlays/OverlayRepositoryService.cs#L305-L307
🤖 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 `@FoliCon/Modules/Overlays/OverlayRepositoryService.cs` around lines 227 - 228,
Constrain remote-controlled paths in
FoliCon/Modules/Overlays/OverlayRepositoryService.cs at lines 227-228 by
validating entry.Id against a strict slug pattern before constructing tmpDir and
finalDir, and apply the same validation in UpdateOverlayAsync,
UninstallOverlayAsync, IsOverlayInstalled, and GetInstalledVersion. At lines
305-307, resolve each asset path with Path.GetFullPath against targetDir, reject
paths outside targetDir before Directory.CreateDirectory or
File.WriteAllBytesAsync, and preserve safe asset writes.
| if (overlayDefinition == null) | ||
| { | ||
| IconOverlay.Legacy => await StaRenderer.Default.EnqueueRender(() => | ||
| new Views.PosterIcon(new PosterIcon(filmFolderPath, rating, ratingVisibility, mockupVisibility)) | ||
| .RenderToBitmap()), | ||
| IconOverlay.Alternate => await StaRenderer.Default.EnqueueRender(() => | ||
| new PosterIconAlt(new PosterIcon(filmFolderPath, rating, ratingVisibility, mockupVisibility)) | ||
| .RenderToBitmap()), | ||
| IconOverlay.Liaher => await StaRenderer.Default.EnqueueRender(() => | ||
| new PosterIconLiaher(new PosterIcon(filmFolderPath, rating, ratingVisibility, mockupVisibility)) | ||
| .RenderToBitmap()), | ||
| IconOverlay.Faelpessoal => await StaRenderer.Default.EnqueueRender(() => new PosterIconFaelpessoal(new PosterIcon( | ||
| filmFolderPath, rating, | ||
| ratingVisibility, mockupVisibility, mediaTitle)).RenderToBitmap()), | ||
| IconOverlay.FaelpessoalHorizontal => await StaRenderer.Default.EnqueueRender(() => new PosterIconFaelpessoalHorizontal( | ||
| new PosterIcon( | ||
| filmFolderPath, rating, | ||
| ratingVisibility, mockupVisibility, mediaTitle)).RenderToBitmap()), | ||
| IconOverlay.Windows11 => await StaRenderer.Default.EnqueueRender(() => | ||
| new PosterIconWindows11(new PosterIcon(filmFolderPath, rating, ratingVisibility, mockupVisibility)) | ||
| .RenderToBitmap()), | ||
| _ => await StaRenderer.Default.EnqueueRender(() => | ||
| new Views.PosterIcon(new PosterIcon(filmFolderPath, rating, ratingVisibility, mockupVisibility)) | ||
| .RenderToBitmap()) | ||
| }; | ||
| Logger.Warn("No overlay definition provided for {FilmFolderPath}. Skipping icon creation.", filmFolderPath); | ||
| return; | ||
| } | ||
|
|
||
| icon = await StaRenderer.Default.EnqueueRender(() => | ||
| new DynamicPosterIcon(overlayDefinition, new PosterIcon(filmFolderPath, rating, ratingVisibility, mockupVisibility, mediaTitle)) | ||
| .RenderToBitmap()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The caller treats this early return as success and deletes the source PNG.
When overlayDefinition is null, BuildFolderIco returns before it creates the .ico. BuildFolderIco returns Task, so TryCreateIconFromPng cannot detect the skip. Lines 87-90 then log "Icon Created", delete the downloaded PNG, and return true. The processed count reported to the user is wrong, and the poster is lost with no icon produced. The PNG File Not Found early return at line 119 has the same effect.
Return a status from BuildFolderIco and act on it.
🐛 Proposed fix
- private static async Task BuildFolderIco(IconProperties iconProperties, PosterOverlayDefinition? overlayDefinition)
+ private static async Task<bool> BuildFolderIco(IconProperties iconProperties, PosterOverlayDefinition? overlayDefinition) if (overlayDefinition == null)
{
Logger.Warn("No overlay definition provided for {FilmFolderPath}. Skipping icon creation.", filmFolderPath);
- return;
+ return false;
}Return false at line 119 as well, return true at the end, and gate the caller on the result:
var iconProperties = new IconProperties(iconMode, pngFilePath, item.Rating, ratingVisibility, mockupVisibility, item.Title);
- await BuildFolderIco(iconProperties, overlayDefinition);
+ if (!await BuildFolderIco(iconProperties, overlayDefinition))
+ {
+ return false;
+ }
Logger.Info("Icon Created for Folder: {Folder}", item.FolderName);🤖 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 `@FoliCon/Modules/utils/IconUtils.cs` around lines 140 - 148, Change
BuildFolderIco to return a success status: return false when the PNG is missing
or overlayDefinition is null, and return true only after the ICO is created.
Update TryCreateIconFromPng to await this result and log, delete the source PNG,
increment processing state, and return true only on success; preserve the
failure path without deleting the PNG.
What
Folicon Plugin ecosystem rebuilt from scratch after the previous discarded dll approch.
Why
This is part of folicon plugin feature ecosystem.
How
Implemented multiple overlay definitions with respective properties, updated the PreviewerViewModel for dynamic loading, and enhanced the PosterIconConfigViewModel for better data binding. Refactored the UI for improved scalability for multiple overlay, overlays are now controlled by json
Testing
dotnet build)Screenshots
Summary by CodeRabbit