Drop WinForms so Release MSIX can trim#126
Conversation
Replace folder/file dialogs and clipboard with Win32/COM interop, use System.Drawing.Common for icons, and serialize digests through a dedicated JSON context that preserves the legacy compact payload bytes. Co-authored-by: Cursor <cursoragent@cursor.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces WinForms dialogs and clipboard access with native interop, introduces source-generated JSON serialization for choices and digest compatibility, enables trimming-related build configuration, configures CodeQL, adds System.Drawing.Common, and corrects release documentation links. ChangesSerialization compatibility
Windows interop migration
Build and release configuration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with 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.
Inline comments:
In `@QuickShell.Core/Models/FormChoiceJson.cs`:
- Around line 13-15: Change FormChoiceJson in
QuickShell.Core/Models/FormChoiceJson.cs lines 13-15 and TaskTypeChoiceJson in
QuickShell.Core/Models/TaskTypeChoiceJson.cs lines 13-16 to internal readonly
record struct declarations, preserving their existing properties and JSON
property names.
In `@QuickShell/Interop/ShellFileDialog.cs`:
- Around line 20-24: Consolidate the duplicated ShellFileDialog and
Win32Clipboard interop implementations, including their hand-written COM/vtable
helpers, into QuickShell.Core or a single shared linked source location. Apply
this across QuickShell/Interop/ShellFileDialog.cs (anchor),
QuickShell.Run/Interop/ShellFileDialog.cs, QuickShell/Interop/Win32Clipboard.cs,
and QuickShell.Run/Interop/Win32Clipboard.cs; remove the duplicate
implementations and ensure both hosts reference the shared types so they cannot
diverge.
In `@QuickShell/Interop/Win32Clipboard.cs`:
- Around line 38-51: Bound the clipboard text read in the Win32 clipboard
interop block by obtaining GlobalSize(handle) / sizeof(char) and using the
length-taking Marshal.PtrToStringUni overload instead of the unbounded overload.
Apply this change in QuickShell/Interop/Win32Clipboard.cs lines 38-51 and
QuickShell.Run/Interop/Win32Clipboard.cs lines 38-51, or consolidate the shared
implementation so both sites inherit the bounded behavior.
- Around line 59-124: Replace direct OpenClipboard calls in SetText and GetText
with a shared TryOpenClipboard helper that retries approximately 10 times with a
short delay before failing. Apply this in QuickShell/Interop/Win32Clipboard.cs
(lines 59-124, plus GetText) and QuickShell.Run/Interop/Win32Clipboard.cs (lines
59-124), or ensure the second implementation inherits the shared helper;
preserve existing clipboard operations after a successful open.
In `@QuickShell/QuickShell.csproj`:
- Around line 159-167: Add a runtime smoke-test gate for the trimmed Release
MSIX produced by the PublishTrimmed property group: install the exact generated
package, verify COM activation, and exercise extension commands through the
extension activation path before considering the deployment change
release-ready. Keep this validation specific to the non-WinGet Release
configuration and fail the release process when any smoke-test step fails.
In `@QuickShell/Services/FolderPickerService.cs`:
- Around line 55-57: The timeout recovery in TryCloseForegroundDialog must stop
targeting an arbitrary foreground window. In
QuickShell/Services/FolderPickerService.cs lines 55-57 and
QuickShell.Run/Services/FolderPickerService.cs lines 51-53, resolve the picker
dialog by enumerating windows belonging to the picker STA thread, selecting its
`#32770` dialog HWND, and post WM_CLOSE only to that window; preserve the existing
timeout behavior otherwise.
In `@QuickShell/Services/ShortcutFilePickerService.cs`:
- Around line 29-35: Update the picker methods around RunOnStaThread to capture
GetForegroundWindow() before starting the STA lambda and pass that fixed owner
handle into ShellFileDialog. Also update RunOnStaThread timeout handling to
request dialog closure with the existing FolderPickerService WM_CLOSE/unblock
approach, then re-join the STA thread before returning null so late selections
are not silently discarded.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98a6cdba-02d0-412d-9d26-4495d8e02f2c
📒 Files selected for processing (25)
.github/workflows/release-extension.yml.github/workflows/release-run-plugin.ymlDirectory.Packages.propsQuickShell.Core.Tests/WorkspaceSecurityPolicyTests.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Models/WorkspaceDigestPayload.csQuickShell.Core/QuickShell.Core.csprojQuickShell.Core/QuickShellDigestJsonContext.csQuickShell.Core/QuickShellJsonContext.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell.Run/Interop/ShellFileDialog.csQuickShell.Run/Interop/Win32Clipboard.csQuickShell.Run/QuickShell.Run.csprojQuickShell.Run/Services/FolderPickerService.csQuickShell.Run/Services/StaClipboard.csQuickShell/ILLink.Descriptors.xmlQuickShell/Interop/ShellFileDialog.csQuickShell/Interop/Win32Clipboard.csQuickShell/QuickShell.csprojQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.csQuickShell/Services/StaClipboard.cs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze C# with CodeQL
- GitHub Check: .NET build and test
- GitHub Check: Performance harness (artifacts)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
Keep
SessionWorkflowStagemembers in strictly ascending order:Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum member names rather than raw integer literals. When adding or renumbering members, provide a legacy-compatible JSON converter for old numeric values; when reordering, verify all inequalities across the solution retain their original semantic meaning.
Files:
QuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/QuickShellDigestJsonContext.csQuickShell.Core/Models/WorkspaceDigestPayload.csQuickShell.Core/QuickShellJsonContext.csQuickShell.Core.Tests/WorkspaceSecurityPolicyTests.csQuickShell.Run/Services/StaClipboard.csQuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell/Services/StaClipboard.csQuickShell.Run/Interop/Win32Clipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Interop/ShellFileDialog.csQuickShell/Interop/Win32Clipboard.csQuickShell/Services/ShortcutFilePickerService.csQuickShell.Run/Interop/ShellFileDialog.csQuickShell.Run/Services/FolderPickerService.cs
QuickShell.Core/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Keep
QuickShell.Corefree of CmdPal SDK dependencies; expose domain behavior through host-independent services and interfaces.
Files:
QuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/QuickShellDigestJsonContext.csQuickShell.Core/Models/WorkspaceDigestPayload.csQuickShell.Core/QuickShellJsonContext.csQuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Services/WorkspaceSecurityPolicy.cs
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: Use namespaces that mirror folders and preserve the repository'sQuickShell.*namespace hierarchy.
Keep one type per file; make most typesinternal, useinternal static classfor stateless helpers, andinternal sealed classfor stateful singletons.
Prefer pure logic ininternal statichelpers and swappable dependencies as interfaces registered through dependency injection.
Use explicit DI factory lambdas where appropriate for AOT and trimming friendliness; avoid reflection-based registration.
Preserve existing#region agent loginstrumentation blocks andAgentDebugLogcalls.
Files:
QuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/QuickShellDigestJsonContext.csQuickShell.Core/Models/WorkspaceDigestPayload.csQuickShell.Core/QuickShellJsonContext.csQuickShell.Core.Tests/WorkspaceSecurityPolicyTests.csQuickShell.Run/Services/StaClipboard.csQuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell/Services/StaClipboard.csQuickShell.Run/Interop/Win32Clipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Interop/ShellFileDialog.csQuickShell/Interop/Win32Clipboard.csQuickShell/Services/ShortcutFilePickerService.csQuickShell.Run/Interop/ShellFileDialog.csQuickShell.Run/Services/FolderPickerService.cs
QuickShell.Core/Models/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Use
readonly record structfor value/result DTOs,recordfor richer results, and mutable classes for entities such asTerminalShortcutandWorkspaceEntry.
Files:
QuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/Models/WorkspaceDigestPayload.csQuickShell.Core/Models/TaskTypeChoiceJson.cs
QuickShell.Core.Tests/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
QuickShell.Core.Tests/**/*.cs: Use xUnit tests with underscore-separated method names; do not introduce Moq or FluentAssertions.
Prefer real services and the established seams (LaunchExecutorTestEnvironment,FakeShortcutRepository, andAgentCliCatalog.AgentCli...Override) instead of mocks; group tests using shared process-wide seams with[Collection].
Files:
QuickShell.Core.Tests/WorkspaceSecurityPolicyTests.cs
QuickShell.Core/Classification/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Project analysis should be composed from
IProjectClassifierimplementations and detector services rather than embedding host-specific logic.
Files:
QuickShell.Core/Classification/ProjectAnalysisService.cs
QuickShell/**/*.csproj
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
After deploying the extension, use the Command Palette Reload command to refresh it.
Files:
QuickShell/QuickShell.csproj
QuickShell/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
QuickShell/**/*.cs: Implement CmdPal pages asICommandimplementations so they can be used anywhere commands are used.
Use “workspace” as the UI term, while retaining%LOCALAPPDATA%\QuickShell\shortcuts.jsonas the on-disk filename and location.Do not modify the COM-hosting pattern in
QuickShell/Program.csor the[Guid]CLSID inQuickShell/QuickShell.cs; the CLSID must matchPackage.appxmanifest.
Files:
QuickShell/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Interop/ShellFileDialog.csQuickShell/Interop/Win32Clipboard.csQuickShell/Services/ShortcutFilePickerService.cs
🔍 Remote MCP GitHub Copilot
Additional review context
- The repo is split into separate
QuickShell.Core,QuickShell.Run, and rootQuickShellprojects;QuickShell.CorehasModels,Services, andClassification, whileQuickShell.RunhasServicesandRunFileDialogs.cs. This confirms the PR crosses shared-core and Run-host boundaries. QuickShell.Services.WorkspaceSecurityPolicy.ComputeDigestcurrently hashes a JSON-serialized anonymous object withId,Name,Directory,Command,Terminal,WtProfile,RunAsAdmin,Launches,DevServerUrl,OpenDevServerOnLaunch,RepoUrl,CompanionApps,OpenCompanionAppOnLaunch,CompanionAppPath, andCompanionAppArguments, then SHA-256s the UTF-8 bytes.CreateReviewTokenandMatchesReviewTokenboth depend on that digest, so this exact field set is the compatibility boundary.
🔇 Additional comments (24)
.github/workflows/release-extension.yml (1)
109-109: LGTM!.github/workflows/release-run-plugin.yml (1)
95-95: LGTM!QuickShell.Core/Models/WorkspaceDigestPayload.cs (1)
10-25: LGTM!QuickShell.Core/QuickShellDigestJsonContext.cs (1)
13-21: LGTM!QuickShell.Core/QuickShellJsonContext.cs (1)
27-30: LGTM!QuickShell.Core/Classification/ProjectAnalysisService.cs (1)
2-2: LGTM!Also applies to: 88-110
QuickShell.Core/Services/CompanionAppCatalog.cs (1)
2-2: LGTM!Also applies to: 168-171
QuickShell.Core/Services/WorkspaceSecurityPolicy.cs (1)
412-433: LGTM!QuickShell.Core.Tests/WorkspaceSecurityPolicyTests.cs (1)
273-343: LGTM!Directory.Packages.props (1)
17-17: LGTM!QuickShell.Core/QuickShell.Core.csproj (1)
10-16: LGTM!QuickShell.Run/QuickShell.Run.csproj (1)
10-11: LGTM!QuickShell/QuickShell.csproj (3)
17-18: LGTM!
159-167: 🎯 Functional CorrectnessVerify that the MSIX pipeline does not set
WinGetBuild=true.These trimming and root-descriptor settings apply only when
$(WinGetBuild) != 'true'. If the release packaging workflow sets that property, the produced MSIX will bypass the new trimming configuration despite the PR objective.
108-113: 🩺 Stability & AvailabilityKeep
IL2104gated until the underlying trim warnings are reviewed.
IL2104is only the assembly-level rollup, so demoting it can hide the specific trim warnings that still need attention inMicrosoft.Windows.SDK.NETandWinRT.Runtime.QuickShell/ILLink.Descriptors.xml (1)
1-15: LGTM!QuickShell.Run/Interop/ShellFileDialog.cs (2)
104-143: LGTM!Also applies to: 145-168, 170-210, 212-219
232-244: 📐 Maintainability & Code QualityNo change needed for the COM inheritance chain.
QuickShell/Interop/ShellFileDialog.cs (1)
46-102: LGTM!Also applies to: 104-143, 170-210
QuickShell.Run/Services/FolderPickerService.cs (1)
19-40: LGTM!Also applies to: 42-49
QuickShell/Services/ShortcutFilePickerService.cs (1)
9-19: LGTM!Also applies to: 64-65
QuickShell/Services/FolderPickerService.cs (1)
18-40: LGTM!Also applies to: 42-53
QuickShell.Run/Services/StaClipboard.cs (1)
2-2: LGTM!Also applies to: 19-19
QuickShell/Services/StaClipboard.cs (1)
32-40: LGTM!
| <PropertyGroup Condition="'$(Configuration)'=='Release' and '$(WinGetBuild)'!='true'"> | ||
| <PublishTrimmed>false</PublishTrimmed> | ||
| <ILLinkTreatWarningsAsErrors>false</ILLinkTreatWarningsAsErrors> | ||
| <!-- WinForms ships many localized satellite assemblies. List only the cultures Quick Shell | ||
| actually ships so MakePRI stays quiet and the MSIX stays lean. --> | ||
| <PublishTrimmed>true</PublishTrimmed> | ||
| <ILLinkTreatWarningsAsErrors>true</ILLinkTreatWarningsAsErrors> | ||
| <!-- List only the cultures Quick Shell actually ships so MakePRI stays quiet and the MSIX stays lean. --> | ||
| <SatelliteResourceLanguages>en;fr;de;it;ru;zh-Hans;zh-Hant;es;ja;ko;pt-BR</SatelliteResourceLanguages> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup Condition="'$(Configuration)'=='Release' and '$(WinGetBuild)'!='true'"> | ||
| <TrimmerRootDescriptor Include="ILLink.Descriptors.xml" /> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Gate the trimmed MSIX with a runtime smoke test.
A successful trimmed publish does not prove that COM activation and extension commands still work after linking. Install the exact Release MSIX and exercise the extension activation path before marking this deployment change release-ready.
🤖 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 `@QuickShell/QuickShell.csproj` around lines 159 - 167, Add a runtime
smoke-test gate for the trimmed Release MSIX produced by the PublishTrimmed
property group: install the exact generated package, verify COM activation, and
exercise extension commands through the extension activation path before
considering the deployment change release-ready. Keep this validation specific
to the non-WinGet Release configuration and fail the release process when any
smoke-test step fails.
Move ShellFileDialog/Win32Clipboard into Core, bound clipboard reads with GlobalSize, retry OpenClipboard, close timed-out pickers via the STA thread's dialog HWND, and capture file-picker owners before starting the STA thread. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the actionable CodeRabbit items in d330c69:
Skipped: trimmed Release MSIX runtime smoke-test gate in |
Remove generic catch around Win32Clipboard, centralize GetForegroundWindow, document intentional unmanaged call sites, and exclude the quality-only cs/call-to-unmanaged-code query via CodeQL config. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed GitHub Advanced Security notes in 30f1720:
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
QuickShell/Services/FolderPickerService.cs (1)
22-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame
nativeThreadIdvisibility gap as theQuickShell.Runcopy.See consolidated comment.
🤖 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 `@QuickShell/Services/FolderPickerService.cs` around lines 22 - 42, Update the nativeThreadId handoff in the folder-picker flow around PickFolderOnStaThread and StaDialogCloser.TryCloseThreadDialog so the worker thread’s assignment is reliably visible to the joining thread. Use the same synchronization or memory-visibility approach applied in the QuickShell.Run copy, while preserving the existing timeout and dialog-close behavior.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@QuickShell.Core/Interop/ShellFileDialog.cs`:
- Line 5: Update the namespace declaration in ShellFileDialog to
QuickShell.Core.Interop so it mirrors the QuickShell.Core/Interop folder and
preserves the repository’s namespace hierarchy.
- Around line 170-210: Update ApplyFilters so the allocated filter array and
strings remain alive after SetFileTypes returns, and return or otherwise retain
their cleanup state for the caller. In PickOpenFile and PickSaveFile, move
freeing that native memory into the finally block that executes after
ShowAndGetPath completes, ensuring cleanup occurs on both success and failure.
In `@QuickShell.Core/Interop/StaDialogCloser.cs`:
- Line 3: Align the namespace declaration in StaDialogCloser.cs with its
QuickShell.Core folder and the namespace used by ShellFileDialog.cs, correcting
the shared namespace/folder mismatch without changing the class behavior.
In `@QuickShell.Core/Interop/Win32Clipboard.cs`:
- Around line 49-58: Update the clipboard string conversion in the Win32
clipboard read path to truncate at the first embedded '\0' rather than calling
TrimEnd('\0'). Preserve the GlobalSize-based bounds and null/empty handling, but
ensure any data after the first terminator is excluded from the returned text.
In `@QuickShell.Run/Services/FolderPickerService.cs`:
- Around line 23-43: Update the nativeThreadId assignment in the STA thread
callback to use a matching Volatile.Write or Interlocked operation, preserving
the existing Volatile.Read before StaDialogCloser.TryCloseThreadDialog. Apply
the same synchronization fix to the corresponding FolderPickerService and
ShortcutFilePickerService implementations.
In `@QuickShell/Services/ShortcutFilePickerService.cs`:
- Around line 84-102: Extract the shared STA-thread, timeout, and dialog-close
flow from this method and the corresponding implementations in both
FolderPickerService files into one reusable helper. In that helper, ensure the
thread ID is safely published before timeout handling reads it, while preserving
the existing action result, timeout, and dialog-closing behavior; update all
three callers to use the helper.
---
Outside diff comments:
In `@QuickShell/Services/FolderPickerService.cs`:
- Around line 22-42: Update the nativeThreadId handoff in the folder-picker flow
around PickFolderOnStaThread and StaDialogCloser.TryCloseThreadDialog so the
worker thread’s assignment is reliably visible to the joining thread. Use the
same synchronization or memory-visibility approach applied in the QuickShell.Run
copy, while preserving the existing timeout and dialog-close behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93572b1b-9dc0-4d63-8228-475868bc80a6
📒 Files selected for processing (11)
QuickShell.Core/Interop/ShellFileDialog.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/QuickShell.Core.csprojQuickShell.Run/QuickShell.Run.csprojQuickShell.Run/Services/FolderPickerService.csQuickShell/QuickShell.csprojQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.cs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
💤 Files with no reviewable changes (2)
- QuickShell.Run/QuickShell.Run.csproj
- QuickShell/QuickShell.csproj
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze Raycast TypeScript with CodeQL
- GitHub Check: Analyze C# with CodeQL
- GitHub Check: .NET build and test
- GitHub Check: Performance harness (artifacts)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
Keep
SessionWorkflowStagemembers in strictly ascending order:Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum member names rather than raw integer literals. When adding or renumbering members, provide a legacy-compatible JSON converter for old numeric values; when reordering, verify all inequalities across the solution retain their original semantic meaning.
Files:
QuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell/Services/ShortcutFilePickerService.csQuickShell.Core/Interop/ShellFileDialog.csQuickShell/Services/FolderPickerService.csQuickShell.Run/Services/FolderPickerService.cs
QuickShell.Core/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Keep
QuickShell.Corefree of CmdPal SDK dependencies; expose domain behavior through host-independent services and interfaces.
Files:
QuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell.Core/Interop/ShellFileDialog.cs
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: Use namespaces that mirror folders and preserve the repository'sQuickShell.*namespace hierarchy.
Keep one type per file; make most typesinternal, useinternal static classfor stateless helpers, andinternal sealed classfor stateful singletons.
Prefer pure logic ininternal statichelpers and swappable dependencies as interfaces registered through dependency injection.
Use explicit DI factory lambdas where appropriate for AOT and trimming friendliness; avoid reflection-based registration.
Preserve existing#region agent loginstrumentation blocks andAgentDebugLogcalls.
Files:
QuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Models/FormChoiceJson.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell/Services/ShortcutFilePickerService.csQuickShell.Core/Interop/ShellFileDialog.csQuickShell/Services/FolderPickerService.csQuickShell.Run/Services/FolderPickerService.cs
QuickShell.Core/Models/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Use
readonly record structfor value/result DTOs,recordfor richer results, and mutable classes for entities such asTerminalShortcutandWorkspaceEntry.
Files:
QuickShell.Core/Models/TaskTypeChoiceJson.csQuickShell.Core/Models/FormChoiceJson.cs
QuickShell/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
QuickShell/**/*.cs: Implement CmdPal pages asICommandimplementations so they can be used anywhere commands are used.
Use “workspace” as the UI term, while retaining%LOCALAPPDATA%\QuickShell\shortcuts.jsonas the on-disk filename and location.Do not modify the COM-hosting pattern in
QuickShell/Program.csor the[Guid]CLSID inQuickShell/QuickShell.cs; the CLSID must matchPackage.appxmanifest.
Files:
QuickShell/Services/ShortcutFilePickerService.csQuickShell/Services/FolderPickerService.cs
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- The actual PR is #126, targeting
master; the provided Babel-Player repository is unrelated and unavailable in DeepWiki. - PR checks are not complete:
.NET build and test, both CodeQL jobs, and the performance harness remain in progress; Raycast lint/tests/build and macOS tests have passed. ShellFileDialogis internal despite the generated summary describing its methods as public. It manually defines source-generated COM vtables, allocates filter strings withCoTaskMem, and releases COM wrappers throughIDisposable; this is the highest-risk architectural area to validate with the pending Windows build and picker smoke tests.- The picker timeout path posts
WM_CLOSEto a dialog found by STA thread enumeration, with a#32770preference and visible-window fallback. Review should verify that this fallback cannot close an unrelated window on the STA thread. - Clipboard reads are bounded by
GlobalSize, while writes transfer ownership only after successfulSetClipboardData; clipboard and picker behavior still lack completed smoke-test evidence in the PR description. - The digest parity test explicitly hashes the legacy anonymous serialization shape, and the new digest context is configured compact with nulls included, preserving the stated compatibility boundary.
- Related PR
#117is merged and established the shared workspace-trust kill-switch behavior; this PR’s digest changes should therefore be checked against that merged baseline.
🔇 Additional comments (24)
QuickShell.Core/Models/FormChoiceJson.cs (1)
13-15: LGTM!QuickShell.Core/Models/TaskTypeChoiceJson.cs (1)
13-16: LGTM!QuickShell.Core/QuickShell.Core.csproj (1)
12-13: LGTM!QuickShell.Core/Interop/ShellFileDialog.cs (7)
1-44: LGTM!
47-103: LGTM on the three entry points themselves; see theApplyFilterscomment below for the underlying issue they trigger throughPickOpenFile/PickSaveFile.
104-143: LGTM!
145-168: LGTM!
212-224: LGTM!
225-230: LGTM!
231-360: 🩺 Stability & Availability | 🏗️ Heavy liftVtable order checks out — worth a real Windows run before merge.
Manually cross-checked
IFileDialog/IFileOpenDialog/IFileSaveDialog/IShellItemmember order against the known ShObjIdl vtable layout and it lines up correctly. This is exactly the kind of code where a single misordered slot silently corrupts calls at runtime rather than failing to compile, so it's worth confirming the pending Windows build/smoke tests actually exercise every picker (open, save, folder) rather than just building.QuickShell.Core/Interop/StaDialogCloser.cs (2)
16-72: LGTM! Thread-scoped#32770lookup with a top-level/visible filter is a solid improvement over targeting the foreground window.
74-96: 🩺 Stability & Availability | ⚡ Quick winConfirm the delegate-marshaled
EnumThreadWindowsP/Invoke survives full trimming/AOT.This file mixes classic
[DllImport]with a managed delegate parameter (EnumThreadWndProc), whileShellFileDialog.cs/Win32Clipboard.csin the same PR moved to[LibraryImport]specifically for AOT/trim safety (per this PR's stated goal of re-enabling MSIX trimming).LibraryImportdoesn't support delegate marshaling directly, so[DllImport]here is likely the pragmatic choice, but worth confirming the trimmed Release MSIX build (listed as pending in the PR) actually exercises this callback path rather than just building.QuickShell.Run/Services/FolderPickerService.cs (2)
4-4: LGTM!
47-58: LGTM! Confirms the past fix (owner handle captured before STA-thread start, no moreFolderBrowserDialog/WinForms lifecycle).QuickShell/Services/FolderPickerService.cs (2)
3-3: LGTM!
46-61: LGTM! This resolves the prior review's "posts WM_CLOSE to an arbitrary foreground window" finding — timeout now targets a dialog owned by the STA thread instead.QuickShell/Services/ShortcutFilePickerService.cs (4)
3-23: LGTM!
24-56: LGTM! Owner handle is now captured on the calling thread beforeRunOnStaThreadstarts the STA lambda — resolves the prior "focus may have moved" finding.
58-75: LGTM!
106-107: LGTM!QuickShell.Core/Interop/Win32Clipboard.cs (4)
21-45: LGTM!
71-136: LGTM! Ownership-transfer-on-success semantics forSetClipboardDataare correctly handled — free only on failure paths.
137-159: LGTM! This resolves the prior review's missing-retry finding.
160-197: LGTM!
| using System.Runtime.InteropServices; | ||
| using System.Runtime.InteropServices.Marshalling; | ||
|
|
||
| namespace QuickShell.Interop; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Namespace doesn't mirror the folder (QuickShell.Core/Interop → QuickShell.Interop).
File lives under QuickShell.Core/Interop/ but declares namespace QuickShell.Interop;, dropping Core. Should be QuickShell.Core.Interop.
As per coding guidelines, "Use namespaces that mirror folders and preserve the repository's QuickShell.* namespace hierarchy."
🤖 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 `@QuickShell.Core/Interop/ShellFileDialog.cs` at line 5, Update the namespace
declaration in ShellFileDialog to QuickShell.Core.Interop so it mirrors the
QuickShell.Core/Interop folder and preserves the repository’s namespace
hierarchy.
Source: Coding guidelines
| @@ -0,0 +1,96 @@ | |||
| using System.Runtime.InteropServices; | |||
|
|
|||
| namespace QuickShell.Interop; | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Same namespace/folder mismatch as ShellFileDialog.cs.
See the consolidated comment for this shared root cause.
🤖 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 `@QuickShell.Core/Interop/StaDialogCloser.cs` at line 3, Align the namespace
declaration in StaDialogCloser.cs with its QuickShell.Core folder and the
namespace used by ShellFileDialog.cs, correcting the shared namespace/folder
mismatch without changing the class behavior.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@QuickShell.Run/Services/FolderPickerService.cs`:
- Around line 23-27: All three STA worker threads must catch picker exceptions
and propagate them to the calling thread after the thread join, or translate
them into the service’s existing failure result. Update the worker around
PickFolderOnStaThread in QuickShell.Run/Services/FolderPickerService.cs lines
23-27, the corresponding PickFolderOnStaThread worker in
QuickShell/Services/FolderPickerService.cs lines 22-26, and the action() worker
in QuickShell/Services/ShortcutFilePickerService.cs lines 85-89; preserve
successful picker behavior while preventing unhandled exceptions from
terminating the process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bbcb473d-0a0b-42a0-ac53-00dbbf9beb18
📒 Files selected for processing (9)
.github/codeql/codeql-config.yml.github/workflows/codeql.ymlQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Run/Services/FolderPickerService.csQuickShell.Run/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.csQuickShell/Services/StaClipboard.cs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze C# with CodeQL
🧰 Additional context used
📓 Path-based instructions (4)
QuickShell/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
QuickShell/**/*.cs: Implement CmdPal pages asICommandimplementations so they can be used anywhere commands are used.
Use “workspace” as the UI term, while retaining%LOCALAPPDATA%\QuickShell\shortcuts.jsonas the on-disk filename and location.Do not modify the COM-hosting pattern in
QuickShell/Program.csor the[Guid]CLSID inQuickShell/QuickShell.cs; the CLSID must matchPackage.appxmanifest.
Files:
QuickShell/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.cs
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
Keep
SessionWorkflowStagemembers in strictly ascending order:Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum member names rather than raw integer literals. When adding or renumbering members, provide a legacy-compatible JSON converter for old numeric values; when reordering, verify all inequalities across the solution retain their original semantic meaning.
Files:
QuickShell/Services/StaClipboard.csQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Run/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.csQuickShell.Run/Services/FolderPickerService.cs
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: Use namespaces that mirror folders and preserve the repository'sQuickShell.*namespace hierarchy.
Keep one type per file; make most typesinternal, useinternal static classfor stateless helpers, andinternal sealed classfor stateful singletons.
Prefer pure logic ininternal statichelpers and swappable dependencies as interfaces registered through dependency injection.
Use explicit DI factory lambdas where appropriate for AOT and trimming friendliness; avoid reflection-based registration.
Preserve existing#region agent loginstrumentation blocks andAgentDebugLogcalls.
Files:
QuickShell/Services/StaClipboard.csQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Run/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.csQuickShell.Run/Services/FolderPickerService.cs
QuickShell.Core/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Keep
QuickShell.Corefree of CmdPal SDK dependencies; expose domain behavior through host-independent services and interfaces.
Files:
QuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.cs
🔍 Remote MCP GitHub Copilot
Additional review context
- PR
#126targetsmaster; the latest.NET build and test, performance harness, and Raycast checks passed. C# CodeQL remains in progress; the overall CodeRabbit status is pending. - The related PR
#117is merged intomasterand introduced the shared workspace-trust kill switch. Digest compatibility should therefore be validated against that merged baseline. - The highest-risk changes remain the new COM-based
ShellFileDialogand STA timeout cleanup: they manually define COM vtables, marshal native filter structures, and release generated COM wrappers. Windows picker smoke tests are still important despite the passing .NET checks. StaDialogCloserprefers visible top-level#32770windows but falls back to the first eligible visible top-level window on the picker thread; review should verify that fallback cannot target unrelated UI.- No DeepWiki research was performed because the required Babel-Player repository is unrelated to this QuickShell PR and the changes do not involve the specified C#/Python architectural boundaries.
🔇 Additional comments (13)
.github/codeql/codeql-config.yml (1)
1-12: LGTM!.github/workflows/codeql.yml (3)
14-22: LGTM!
56-56: LGTM!
92-92: LGTM!QuickShell.Core/Interop/NativeForegroundWindow.cs (2)
3-3: Keep the Core interop namespace aligned with its folder.
QuickShell.Core/Interop/NativeForegroundWindow.csdeclaresQuickShell.Interop, droppingCorefrom the required folder-to-namespace hierarchy. UseQuickShell.Core.Interopconsistently across the Core interop files and update consumers together.Source: Coding guidelines
5-19: LGTM!QuickShell.Run/Services/FolderPickerService.cs (1)
25-26: PublishnativeThreadIdwith a matching volatile write.Line 42 uses
Volatile.Read, but Line 25 performs an ordinary write. On timeout, the caller can observe0and skip dialog closure. UseVolatile.WriteorInterlocked.Exchange.QuickShell/Services/FolderPickerService.cs (1)
24-25: PublishnativeThreadIdwith a matching volatile write.The plain assignment at Line 24 is paired with
Volatile.Readduring timeout handling. UseVolatile.WriteorInterlocked.Exchange; otherwise the close path can receive0and leave the modal picker blocked.QuickShell/Services/ShortcutFilePickerService.cs (1)
87-88: PublishnativeThreadIdwith a matching volatile write.
Volatile.Readis used after the timeout, but Line 87 performs a plain write. UseVolatile.WriteorInterlocked.Exchangeso timeout cleanup cannot observe0and skip closing the picker.QuickShell.Core/Interop/StaDialogCloser.cs (2)
25-52: 🩺 Stability & AvailabilityProve that the fallback HWND belongs to the picker.
When no
#32770window is found, the fallback selects any visible top-level window on the thread and postsWM_CLOSEto it. Thread ownership alone does not prove that the window belongs to thisIFileDialog. Either track a dialog-specific/new HWND or explicitly enforce and test the dedicated-thread invariant on all supported Windows versions.Source: MCP tools
61-106: LGTM!QuickShell.Run/Services/StaClipboard.cs (1)
17-18: LGTM!QuickShell/Services/StaClipboard.cs (1)
30-34: LGTM!
Keep COMDLG filter native memory alive until after Show, truncate clipboard text at the first null, and share STA timeout/close/exception handling via StaModalDialogRunner with Volatile thread-id publication. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the latest CodeRabbit follow-ups in the just-pushed commit: Fixed
Skipped
|
|
Note Docstrings generation - SUCCESS |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>
Docstrings generation was requested by @tonythethompson. The following files were modified: * `QuickShell.Core/Classification/ProjectAnalysisService.cs` * `QuickShell.Core/Interop/NativeForegroundWindow.cs` * `QuickShell.Core/Interop/ShellFileDialog.cs` * `QuickShell.Core/Interop/StaDialogCloser.cs` * `QuickShell.Core/Interop/Win32Clipboard.cs` * `QuickShell.Core/Services/CompanionAppCatalog.cs` * `QuickShell.Core/Services/StaModalDialogRunner.cs` * `QuickShell.Core/Services/WorkspaceSecurityPolicy.cs` * `QuickShell.Run/Services/FolderPickerService.cs` * `QuickShell.Run/Services/StaClipboard.cs` * `QuickShell/Services/FolderPickerService.cs` * `QuickShell/Services/ShortcutFilePickerService.cs` * `QuickShell/Services/StaClipboard.cs` These files were ignored: * `QuickShell.Core.Tests/WorkspaceSecurityPolicyTests.cs` These file types are not supported: * `.github/codeql/codeql-config.yml` * `.github/workflows/codeql.yml` * `.github/workflows/release-extension.yml` * `.github/workflows/release-run-plugin.yml` * `Directory.Packages.props` * `QuickShell.Core/QuickShell.Core.csproj` * `QuickShell.Run/QuickShell.Run.csproj` * `QuickShell/ILLink.Descriptors.xml` * `QuickShell/QuickShell.csproj`
Status vs the pasted CodeRabbit reviews (8638919 / d330c69)Those review runs are already addressed on current HEAD. Mapping:
No further code changes needed for those two review posts beyond what’s already landed. |
Address leftover GHAS notes from the StaModalDialogRunner follow-up without reopening already-fixed CodeRabbit findings on older commits. Co-authored-by: Cursor <cursoragent@cursor.com>
Exclude cs/unmanaged-code alongside call-site alerts, catch known picker failures specifically, keep a documented background STA catch-all, and document that EnumThreadWindows needs the native thread id. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed remaining GitHub Advanced Security notes:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@QuickShell.Core/Services/StaModalDialogRunner.cs`:
- Line 4: Update the namespace declaration in StaModalDialogRunner to
QuickShell.Core.Services so it mirrors the file’s folder and preserves the
repository’s QuickShell.* namespace hierarchy, matching the convention used by
ShellFileDialog and StaDialogCloser.
- Around line 57-62: Update the timeout and recovery sequence in the STA dialog
runner method containing the shown thread.Join and
StaDialogCloser.TryCloseThreadDialog calls: first join only for dialogTimeout,
then invoke TryCloseThreadDialog when that wait expires, and finally allow at
most one additional join for joinGracePeriod. Preserve the existing success
return and fault handling.
In `@QuickShell/Services/StaClipboard.cs`:
- Around line 35-40: Update SetTextOnStaThread in
QuickShell/Services/StaClipboard.cs and its corresponding TrySetText wrapper in
QuickShell.Run/Services/StaClipboard.cs to handle null text before forwarding to
Win32Clipboard.SetText, returning false instead of allowing
ArgumentNullException to escape. Apply the same null-safe boolean-failure
behavior in both files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e0bc499-4d8a-4dac-a9f9-c1e13bce756b
📒 Files selected for processing (13)
QuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/ShellFileDialog.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Services/StaModalDialogRunner.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell.Run/Services/FolderPickerService.csQuickShell.Run/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.csQuickShell/Services/StaClipboard.cs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze Raycast TypeScript with CodeQL
- GitHub Check: Analyze C# with CodeQL
- GitHub Check: .NET build and test
- GitHub Check: Performance harness (artifacts)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
Keep
SessionWorkflowStagemembers in strictly ascending order:Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum member names rather than raw integer literals. When adding or renumbering members, provide a legacy-compatible JSON converter for old numeric values; when reordering, verify all inequalities across the solution retain their original semantic meaning.
Files:
QuickShell.Core/Services/StaModalDialogRunner.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell.Run/Services/StaClipboard.csQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell/Services/StaClipboard.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Run/Services/FolderPickerService.csQuickShell/Services/FolderPickerService.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell.Core/Interop/ShellFileDialog.csQuickShell/Services/ShortcutFilePickerService.cs
QuickShell.Core/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Keep
QuickShell.Corefree of CmdPal SDK dependencies; expose domain behavior through host-independent services and interfaces.
Files:
QuickShell.Core/Services/StaModalDialogRunner.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell.Core/Interop/ShellFileDialog.cs
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: Use namespaces that mirror folders and preserve the repository'sQuickShell.*namespace hierarchy.
Keep one type per file; make most typesinternal, useinternal static classfor stateless helpers, andinternal sealed classfor stateful singletons.
Prefer pure logic ininternal statichelpers and swappable dependencies as interfaces registered through dependency injection.
Use explicit DI factory lambdas where appropriate for AOT and trimming friendliness; avoid reflection-based registration.
Preserve existing#region agent loginstrumentation blocks andAgentDebugLogcalls.
Files:
QuickShell.Core/Services/StaModalDialogRunner.csQuickShell.Core/Services/CompanionAppCatalog.csQuickShell.Core/Services/WorkspaceSecurityPolicy.csQuickShell.Run/Services/StaClipboard.csQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell/Services/StaClipboard.csQuickShell.Core/Classification/ProjectAnalysisService.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Run/Services/FolderPickerService.csQuickShell/Services/FolderPickerService.csQuickShell.Core/Interop/Win32Clipboard.csQuickShell.Core/Interop/ShellFileDialog.csQuickShell/Services/ShortcutFilePickerService.cs
QuickShell/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
QuickShell/**/*.cs: Implement CmdPal pages asICommandimplementations so they can be used anywhere commands are used.
Use “workspace” as the UI term, while retaining%LOCALAPPDATA%\QuickShell\shortcuts.jsonas the on-disk filename and location.Do not modify the COM-hosting pattern in
QuickShell/Program.csor the[Guid]CLSID inQuickShell/QuickShell.cs; the CLSID must matchPackage.appxmanifest.
Files:
QuickShell/Services/StaClipboard.csQuickShell/Services/FolderPickerService.csQuickShell/Services/ShortcutFilePickerService.cs
QuickShell.Core/Classification/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Project analysis should be composed from
IProjectClassifierimplementations and detector services rather than embedding host-specific logic.
Files:
QuickShell.Core/Classification/ProjectAnalysisService.cs
🔍 Remote MCP GitHub Copilot
Additional review context
-
StaModalDialogRunner.RunwaitsdialogTimeout + joinGracePeriodbefore attempting dialog closure, then waits anotherjoinGracePeriod. This means dialog recovery begins later than the documented “initial timeout” behavior and may block for up todialogTimeout + 2 × joinGracePeriod. -
StaDialogCloserexcludes only the owner HWND, invisible windows, and child windows. If no#32770dialog exists, it postsWM_CLOSEto the first visible top-level window on the picker thread. This fallback remains potentially broad and warrants targeted timeout smoke testing. -
Win32Clipboard.cscontains a malformed XML documentation opening tag: the first<summary>beforeTryOpenClipboardis not closed with</summary>before another<summary>begins. This may produce documentation/compiler warnings and should be corrected. -
ShellFileDialogmanually defines generated COM interfaces and vtable-preserving methods, allocates filter-spec memory, and releases COM wrappers explicitly. The implementation keeps filter memory alive throughShow, but Windows picker smoke tests remain important because these paths are not covered by the reported checks. -
At retrieval time,
.NET build and test, both CodeQL jobs, Raycast lint/build, and the performance harness were still in progress; the “changes” and macOS Raycast test jobs had passed.
🔇 Additional comments (15)
QuickShell.Core/Interop/Win32Clipboard.cs (2)
21-30: 🎯 Functional CorrectnessVerify the intentional loss of ANSI clipboard compatibility.
GetText()now returnsnullunlessCF_UNICODETEXTis present, while the removed WinFormsClipboard.ContainsText()/GetText()path handledTextorUnicodeTextdepending on the OS. ACF_TEXT-only clipboard can therefore become unreadable. Add an ANSI fallback or document and test this as an intentional contract change. (learn.microsoft.com)
62-71: LGTM!Also applies to: 84-156, 161-180, 182-254
QuickShell.Run/Services/StaClipboard.cs (1)
8-20: LGTM!QuickShell/Services/StaClipboard.cs (1)
2-2: LGTM!Also applies to: 20-32, 42-46
QuickShell.Core/Classification/ProjectAnalysisService.cs (1)
2-2: LGTM!Also applies to: 82-101, 111-125
QuickShell.Core/Services/CompanionAppCatalog.cs (1)
2-2: LGTM!Also applies to: 168-181
QuickShell.Core/Services/WorkspaceSecurityPolicy.cs (1)
410-420: LGTM!Also applies to: 435-438
QuickShell.Core/Interop/ShellFileDialog.cs (2)
52-267: LGTM!
304-638: LGTM!QuickShell.Core/Interop/StaDialogCloser.cs (1)
16-142: LGTM!QuickShell.Core/Interop/NativeForegroundWindow.cs (1)
12-27: LGTM!QuickShell.Run/Services/FolderPickerService.cs (1)
11-39: LGTM!QuickShell/Services/FolderPickerService.cs (1)
10-42: LGTM!QuickShell/Services/ShortcutFilePickerService.cs (1)
1-99: LGTM!QuickShell.Core/Services/StaModalDialogRunner.cs (1)
30-33: 🩺 Stability & AvailabilitySTA callers still bypass recovery.
TryInvoke(action)skips the timeout/join-grace watchdog on theApartmentState.STAfast-path, so a hung native dialog can block that thread forever. No explicit STA setup is visible in the repo, so this needs host-level confirmation before treating it as a reachable hang.
| using System.Threading; | ||
| using QuickShell.Interop; | ||
|
|
||
| namespace QuickShell.Services; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Namespace drops Core, same defect as ShellFileDialog.cs/StaDialogCloser.cs.
File is QuickShell.Core/Services/StaModalDialogRunner.cs but declares namespace QuickShell.Services;. This both breaks the folder-mirroring convention and collides with the identically-named QuickShell.Services namespace used by QuickShell/Services/FolderPickerService.cs. Should be QuickShell.Core.Services.
🏷️ Proposed fix
-namespace QuickShell.Services;
+namespace QuickShell.Core.Services;As per coding guidelines, "Use namespaces that mirror folders and preserve the repository's QuickShell.* namespace hierarchy."
🤖 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 `@QuickShell.Core/Services/StaModalDialogRunner.cs` at line 4, Update the
namespace declaration in StaModalDialogRunner to QuickShell.Core.Services so it
mirrors the file’s folder and preserves the repository’s QuickShell.* namespace
hierarchy, matching the convention used by ShellFileDialog and StaDialogCloser.
Source: Coding guidelines
| if (thread.Join(dialogTimeout + joinGracePeriod)) | ||
| { | ||
| return fault is null ? selected : null; | ||
| } | ||
|
|
||
| StaDialogCloser.TryCloseThreadDialog(Volatile.Read(ref nativeThreadId), ownerHandle); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recovery close fires later than documented, and worst-case wait grows to dialogTimeout + 2×joinGracePeriod.
The doc comments (lines 18-19) describe dialogTimeout as the "initial" wait and joinGracePeriod as the "additional" recovery/cleanup allowance, but the code joins for dialogTimeout + joinGracePeriod before even attempting TryCloseThreadDialog, then joins again for another joinGracePeriod. TryCloseThreadDialog should fire after dialogTimeout alone.
🕒 Proposed fix
- if (thread.Join(dialogTimeout + joinGracePeriod))
+ if (thread.Join(dialogTimeout))
{
return fault is null ? selected : null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (thread.Join(dialogTimeout + joinGracePeriod)) | |
| { | |
| return fault is null ? selected : null; | |
| } | |
| StaDialogCloser.TryCloseThreadDialog(Volatile.Read(ref nativeThreadId), ownerHandle); | |
| if (thread.Join(dialogTimeout)) | |
| { | |
| return fault is null ? selected : null; | |
| } | |
| StaDialogCloser.TryCloseThreadDialog(Volatile.Read(ref nativeThreadId), ownerHandle); |
🤖 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 `@QuickShell.Core/Services/StaModalDialogRunner.cs` around lines 57 - 62,
Update the timeout and recovery sequence in the STA dialog runner method
containing the shown thread.Join and StaDialogCloser.TryCloseThreadDialog calls:
first join only for dialogTimeout, then invoke TryCloseThreadDialog when that
wait expires, and finally allow at most one additional join for joinGracePeriod.
Preserve the existing success return and fault handling.
Source: MCP tools
| // Win32Clipboard returns false/null on failure and does not throw for the clipboard | ||
| /// <summary> | ||
| /// Attempts to place text on the Windows clipboard. | ||
| /// </summary> | ||
| /// <returns><c>true</c> if the text was set successfully; <c>false</c> otherwise.</returns> | ||
| private static bool SetTextOnStaThread(string text) => Win32Clipboard.SetText(text); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'QuickShell/Services/StaClipboard.cs' 'QuickShell.Run/Services/StaClipboard.cs' '*Win32Clipboard*' '*Clipboard*' | sed 's#^`#-` #'
printf '\n== Outline StaClipboard.cs ==\n'
ast-grep outline QuickShell/Services/StaClipboard.cs --view expanded || true
printf '\n== Outline QuickShell.Run/Services/StaClipboard.cs ==\n'
ast-grep outline QuickShell.Run/Services/StaClipboard.cs --view expanded || true
printf '\n== Search Win32Clipboard / SetText ==\n'
rg -n "class Win32Clipboard|static .*SetText|SetText\\(" QuickShell QuickShell.Run -g '*.cs' || true
printf '\n== Read relevant file sections ==\n'
for f in QuickShell/Services/StaClipboard.cs QuickShell.Run/Services/StaClipboard.cs; do
echo "--- $f"
nl -ba "$f" | sed -n '1,220p'
done
printf '\n== Read Win32Clipboard definitions ==\n'
for f in $(git ls-files '*Win32Clipboard*.cs'); do
echo "--- $f"
nl -ba "$f" | sed -n '1,240p'
doneRepository: tonythethompson/QuickShell
Length of output: 2152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in QuickShell/Services/StaClipboard.cs QuickShell.Run/Services/StaClipboard.cs QuickShell.Core/Interop/Win32Clipboard.cs; do
echo "--- $f"
sed -n '1,220p' "$f" | cat -n
doneRepository: tonythethompson/QuickShell
Length of output: 12702
Keep the STA clipboard wrappers on the boolean-failure path.
Win32Clipboard.SetText throws ArgumentNullException for null input, so both StaClipboard.TrySetText wrappers can leak an exception instead of returning false. Add a null guard or catch ArgumentNullException before forwarding in:
QuickShell/Services/StaClipboard.csQuickShell.Run/Services/StaClipboard.cs
📍 Affects 2 files
QuickShell/Services/StaClipboard.cs#L35-L40(this comment)QuickShell.Run/Services/StaClipboard.cs#L22-L23
🤖 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 `@QuickShell/Services/StaClipboard.cs` around lines 35 - 40, Update
SetTextOnStaThread in QuickShell/Services/StaClipboard.cs and its corresponding
TrySetText wrapper in QuickShell.Run/Services/StaClipboard.cs to handle null
text before forwarding to Win32Clipboard.SetText, returning false instead of
allowing ArgumentNullException to escape. Apply the same null-safe
boolean-failure behavior in both files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@QuickShell.Core/Services/StaModalDialogRunner.cs`:
- Around line 93-101: Update StaModalDialogRunner’s background-thread exception
handling and IsPickerFailure so only recognized picker/cancellation exceptions
return the canceled-pick result; propagate all other faults, including
OutOfMemoryException, SEHException, and unexpected bugs, through an explicit
failure path instead of collapsing them to null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18f11274-4d82-4d6b-9e3d-94787bdfd55f
📒 Files selected for processing (4)
.github/codeql/codeql-config.ymlQuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Services/StaModalDialogRunner.cs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze C# with CodeQL
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
Keep
SessionWorkflowStagemembers in strictly ascending order:Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum member names rather than raw integer literals. When adding or renumbering members, provide a legacy-compatible JSON converter for old numeric values; when reordering, verify all inequalities across the solution retain their original semantic meaning.
Files:
QuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Services/StaModalDialogRunner.cs
QuickShell.Core/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Keep
QuickShell.Corefree of CmdPal SDK dependencies; expose domain behavior through host-independent services and interfaces.
Files:
QuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Services/StaModalDialogRunner.cs
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: Use namespaces that mirror folders and preserve the repository'sQuickShell.*namespace hierarchy.
Keep one type per file; make most typesinternal, useinternal static classfor stateless helpers, andinternal sealed classfor stateful singletons.
Prefer pure logic ininternal statichelpers and swappable dependencies as interfaces registered through dependency injection.
Use explicit DI factory lambdas where appropriate for AOT and trimming friendliness; avoid reflection-based registration.
Preserve existing#region agent loginstrumentation blocks andAgentDebugLogcalls.
Files:
QuickShell.Core/Interop/NativeForegroundWindow.csQuickShell.Core/Interop/StaDialogCloser.csQuickShell.Core/Services/StaModalDialogRunner.cs
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- The specified DeepWiki repository
mta1124-1629472/Babel-Playerwas not found, so it provided no architectural context. - The actual repository is
tonythethompson/QuickShell; its guidelines describe it as a Windows-only .NET 10 application with an out-of-process COM/MSIX CmdPal host.QuickShellExtensionis registered byProgram.cs, making the new linker root directly relevant to COM activation. QuickShell.Coreis intentionally host-independent and shared by CmdPal, PowerToys Run, and other surfaces. Removing WinForms from Core therefore affects multiple hosts, not only CmdPal.- The repository requires Windows validation for the full solution and specifically requires
-p:Platform=x64for build/test commands. Core-only builds can run with Windows targeting enabled, but extension, Run-plugin, tests, and packaging require Windows verification. - Workspace review tokens hash a fixed ordered payload containing workspace fields, launch entries, and companion-app data. The current implementation uses anonymous-object serialization, confirming that field order, null handling, and JSON naming are compatibility-sensitive.
- Repository guidance treats
Directory.Packages.propsas the central NuGet version source and notes that analyzers are enabled; package and trimming changes should therefore be checked with the Windows Release build rather than only unit tests.
🔀 Multi-repo context tonythethompson/numan
Linked repositories findings
tonythethompson/numan
packaging/winget/README.md:5directly links to QuickShell’s WinGet manifest under themasterbranch, consistent with the PR’s release-link correction. [::tonythethompson/numan::]- No consumers of changed QuickShell APIs, JSON contracts, or digest behavior were found.
tonythethompson/dependency-chain-substrate
- No references to QuickShell, CmdPal, workspace digests, picker/clipboard APIs, or related release links were found.
🔇 Additional comments (4)
.github/codeql/codeql-config.yml (1)
7-16: LGTM!QuickShell.Core/Interop/NativeForegroundWindow.cs (1)
18-19: LGTM!Also applies to: 26-27
QuickShell.Core/Interop/StaDialogCloser.cs (1)
30-31: LGTM!Also applies to: 35-36, 41-42, 66-69, 71-80, 94-95
QuickShell.Core/Services/StaModalDialogRunner.cs (1)
1-1: LGTM!Also applies to: 10-10, 41-50, 83-87, 89-92
| private static bool IsPickerFailure(Exception ex) => | ||
| ex is COMException | ||
| or ExternalException | ||
| or SEHException | ||
| or InvalidOperationException | ||
| or ArgumentException | ||
| or OutOfMemoryException | ||
| or NotSupportedException | ||
| or UnauthorizedAccessException; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
sed -n '1,220p' QuickShell.Core/Services/StaModalDialogRunner.cs | cat -n
echo
echo "== references =="
rg -n "IsPickerFailure|StaModalDialogRunner|return null|catch .*COMException|SEHException|OutOfMemoryException" QuickShell.Core -S || trueRepository: tonythethompson/QuickShell
Length of output: 14809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== StaModalDialogRunner call sites =="
rg -n "StaModalDialogRunner\.Run\(" -S .
echo
echo "== ShellFileDialog =="
sed -n '1,260p' QuickShell.Core/Interop/ShellFileDialog.cs | cat -n
echo
echo "== Win32Clipboard =="
sed -n '1,180p' QuickShell.Core/Interop/Win32Clipboard.cs | cat -nRepository: tonythethompson/QuickShell
Length of output: 20160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
QuickShell/Services/FolderPickerService.cs \
QuickShell/Services/ShortcutFilePickerService.cs \
QuickShell.Run/Services/FolderPickerService.cs
do
echo "== $f =="
sed -n '1,220p' "$f" | cat -n
echo
doneRepository: tonythethompson/QuickShell
Length of output: 9059
Don't collapse STA worker faults into picker cancellation. In QuickShell.Core/Services/StaModalDialogRunner.cs, the background-thread catch blocks return null for every exception, so OutOfMemoryException, SEHException, and ordinary bugs all look like a canceled pick. Split cancellation from failure and surface an explicit fault path instead.
🤖 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 `@QuickShell.Core/Services/StaModalDialogRunner.cs` around lines 93 - 101,
Update StaModalDialogRunner’s background-thread exception handling and
IsPickerFailure so only recognized picker/cancellation exceptions return the
canceled-pick result; propagate all other faults, including
OutOfMemoryException, SEHException, and unexpected bugs, through an explicit
failure path instead of collapsing them to null.
2 participants
Summary
System.Drawing.Common(icon cache only).PublishTrimmed) with an ILLink root forQuickShellExtension, plus trim-safe source-generated JSON for form choices.QuickShellDigestJsonContext(compact + nulls) and a parity test against the old anonymousSerializeshape.main(masteris the default branch).Test plan
dotnet testsecurity/digest filters (Debug):ComputeDigest_matches_legacy+WorkspaceSecurityPolicyTests+ adversarialdotnet build QuickShell.RunRelease x64dotnet build QuickShellRelease x64 withWinGetBuild=truedotnet build -c Releasehits MSB4216 ILLink task-host locally)WorkspaceReviewTokenstill matchesMade with Cursor
Summary by cubic
Dropped WinForms and replaced dialogs/clipboard with trim‑safe Win32/COM interop so Release MSIX can trim without changing workspace trust digest bytes. Added a shared STA dialog runner with safe exception-to-null handling and documented unmanaged call sites.
Refactors
ShellFileDialog,Win32Clipboard,StaDialogCloser,StaModalDialogRunner, andNativeForegroundWindowin Core (source‑generated COM +LibraryImport) and replaced all WinForms dialogs/clipboard in both hosts.usingscope; close timed‑out pickers via the STA thread’s dialog HWND; publish native thread id viaVolatile; capture owner before starting STA; bound CF_UNICODETEXT byGlobalSizeand truncate at first null; retryOpenClipboard; centralize STA exceptions to return null; added XML docstrings and inline CodeQL annotations.FormChoiceJson,TaskTypeChoiceJson,WorkspaceDigestPayload) viaQuickShellJsonContext/QuickShellDigestJsonContext; tests pin legacy digest bytes.Dependencies
PublishTrimmed=true), rootedQuickShellExtensionviaILLink.Descriptors.xml, and treat trim warnings as errors (suppressedIL2104). AddedSystem.Drawing.Common@9.0.8; continue usingSystem.Text.Json@9.0.8with source‑gen contexts.cs/call-to-unmanaged-codeandcs/unmanaged-code. Fixed release workflow links tomaster.Written for commit e331b7a. Summary will update on new commits.