fix(windows): clear the build warning and honor start_menu_folder - #23
fix(windows): clear the build warning and honor start_menu_folder#23Firnschnee wants to merge 1 commit into
Conversation
Two gaps between the Windows scaffold and its own documented contract, found while verifying the beta on Windows 11 hardware. wrapper.csproj: every build emitted WFAC010. UseWindowsForms (present only for the tray NotifyIcon) pulls in the WinForms SDK DPI analyzer, which asks for ApplicationHighDpiMode / Application.SetHighDpiMode. The host is WPF-first and declares PerMonitorV2 in app.manifest, the standard WPF way, so the manifest is correct and SetHighDpiMode is never called. Suppress WFAC010 with a rationale instead of moving DPI config into a WinForms API this app does not use. No change to DPI behavior. desktop-install.ps1: app-it.config.example.json documents platform.windows.start_menu_folder as configurable (default "app-it"), but the installer hardcoded "app-it" and never read the config, so the field was silently ignored. Read it per app and resolve the Start Menu folder accordingly. APP_IT_INSTALL_DIR still overrides everything with a single explicit target, unchanged.
Reviewer's GuideFixes a Windows wrapper build warning by suppressing a WinForms DPI analyzer rule with justification, and updates the Windows desktop installer script to honor per-app start_menu_folder from app-it.config.json while preserving APP_IT_INSTALL_DIR override behavior. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider adding a small try/catch around
Get-Content | ConvertFrom-Jsonso that a malformedapp-it.config.jsonresults in a clear, user-friendly error instead of a generic PowerShell stack trace. - The deeply nested
PSObject.Properties.Name -containschecks in theplatform.windows.start_menu_folderresolution block are a bit hard to read; simplifying them (e.g., by checking for$a.platform,$a.platform.windows, and then directly accessingstart_menu_folder) or extracting into a helper function would improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a small try/catch around `Get-Content | ConvertFrom-Json` so that a malformed `app-it.config.json` results in a clear, user-friendly error instead of a generic PowerShell stack trace.
- The deeply nested `PSObject.Properties.Name -contains` checks in the `platform.windows.start_menu_folder` resolution block are a bit hard to read; simplifying them (e.g., by checking for `$a.platform`, `$a.platform.windows`, and then directly accessing `start_menu_folder`) or extracting into a helper function would improve maintainability.
## Individual Comments
### Comment 1
<location path="plugins/app-it-windows/skills/app-it-windows/templates/desktop-install.ps1" line_range="43-44" />
<code_context>
+$defaultFolder = 'app-it'
+$folderForApp = @{}
+if (Test-Path $ConfigFile) {
+ $cfg = Get-Content -Raw $ConfigFile | ConvertFrom-Json
+ foreach ($a in $cfg.apps) {
+ $folder = $defaultFolder
+ if ($a.PSObject.Properties.Name -contains 'platform' -and $a.platform -and
</code_context>
<issue_to_address>
**issue (bug_risk):** Wrap config parsing in error handling to avoid unhelpful failures on malformed JSON
`ConvertFrom-Json` will throw on malformed or unreadable `app-it.config.json`, causing the script to terminate with a PowerShell stack trace. Please wrap the read/parse in a `try/catch` that reports a clear error (e.g., "Invalid app-it.config.json") and exits cleanly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Landed on `main` in 5fd973d (cherry-picked with `-x`, your authorship preserved) — thank you, @Firnschnee. Both fixes shipped as-is: `start_menu_folder` is now read per app (with `APP_IT_INSTALL_DIR` still overriding and now validated), and WFAC010 is suppressed with your rationale comment. CHANGELOG updated under the Windows beta line. Closing since it's merged. |
Bump 0.1.0 -> 0.2.0 across marketplace.json (top-level + all three plugin entries) and all six plugin.json manifests. Move the entire CHANGELOG [Unreleased] block (static companion, doctor, verify, JSON output, fixed-port, hosted-URL wrappers, native bootstrap, ownership- safe cleanup, the Windows beta fixes) into a dated "## 0.2.0 - 2026-06-20" section; leave a fresh empty [Unreleased]. Reconcile the marketplace listing's Windows status line, the only outlier: it still said "untested on real hardware" while README and the CHANGELOG already reflect that real-hardware fixes (#8/#17/#18, #22/#23) have landed. Calibrate the listing text (marketplace.json + its two byte-identical app-it-windows plugin.json mirrors) to "Beta - first real-hardware fixes landed - maintainer wanted" — true, does not claim "tested", keeps the maintainer-wanted ask. The ~25 deliberate beta/maintainer-wanted placements in the deeper docs/ADR/ SKILL/contract files are left untouched by design. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Part of #21. Two gaps between the Windows scaffold and its own documented contract,
found while verifying the beta on Windows 11.
wrapper.csproj: WFAC010 on every build
UseWindowsForms (present only for the tray NotifyIcon) pulls in the WinForms SDK DPI
analyzer, which asks for ApplicationHighDpiMode / Application.SetHighDpiMode. The
host is WPF-first and declares PerMonitorV2 in app.manifest, the standard WPF way,
so the manifest is correct and SetHighDpiMode is never called. Suppressed WFAC010
with a rationale comment instead of moving DPI config into a WinForms API this app
does not use. No change to DPI behavior.
Glad to switch to the ApplicationHighDpiMode approach instead if you would rather
settle the deferred DPI question that way.
desktop-install.ps1: start_menu_folder was ignored
app-it.config.example.json documents platform.windows.start_menu_folder as
configurable (default "app-it"), but the installer hardcoded "app-it" and never read
the config, so the field was silently ignored. Now it reads the field per app and
resolves the Start Menu folder accordingly. APP_IT_INSTALL_DIR still overrides
everything with a single explicit target, unchanged.
Verified on Windows 11: custom folder, default folder, and APP_IT_INSTALL_DIR
override all land the .lnk in the right place; lints clean.
Summary by Sourcery
Honor Windows Start Menu folder configuration and silence a noisy Windows build warning.
Bug Fixes:
Enhancements: