Repository navigation
Fix theme preview using the previous theme's border style - #4666
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesTheme preview border styling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The preview now uses the selected theme’s border style, with regression coverage for theme-order changes. No concrete merge-blocking risk is evident from the supplied context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Test/ThemePreviewTest.cs">
<violation number="1" location="Flow.Launcher.Test/ThemePreviewTest.cs:40">
P3: This fixture permanently leaves a live WPF `Application` and its STA dispatcher thread running for the rest of the test process, and `GetApplicationAsync` unconditionally reuses any pre-existing `Application.Current` without verifying its dispatcher is still healthy. NUnit may run the whole assembly in one process, so after this test any other fixture that constructs `System.Windows.Application` (including the real Flow `App`, which derives from it) throws "Cannot create more than one Application instance in the same AppDomain", and the background thread is never joined or shut down. Guard the reuse path (check `HasShutdownStarted` / `Dispatcher.HasShutdownPendingShutdown` and create a fresh app instead) so the fixture does not poison the process for later tests.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (Application.Current != null) | ||
| return Task.FromResult(Application.Current); | ||
|
|
||
| var ready = new TaskCompletionSource<Application>(TaskCreationOptions.RunContinuationsAsynchronously); |
There was a problem hiding this comment.
P3: This fixture permanently leaves a live WPF Application and its STA dispatcher thread running for the rest of the test process, and GetApplicationAsync unconditionally reuses any pre-existing Application.Current without verifying its dispatcher is still healthy. NUnit may run the whole assembly in one process, so after this test any other fixture that constructs System.Windows.Application (including the real Flow App, which derives from it) throws "Cannot create more than one Application instance in the same AppDomain", and the background thread is never joined or shut down. Guard the reuse path (check HasShutdownStarted / Dispatcher.HasShutdownPendingShutdown and create a fresh app instead) so the fixture does not poison the process for later tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher.Test/ThemePreviewTest.cs, line 40:
<comment>This fixture permanently leaves a live WPF `Application` and its STA dispatcher thread running for the rest of the test process, and `GetApplicationAsync` unconditionally reuses any pre-existing `Application.Current` without verifying its dispatcher is still healthy. NUnit may run the whole assembly in one process, so after this test any other fixture that constructs `System.Windows.Application` (including the real Flow `App`, which derives from it) throws "Cannot create more than one Application instance in the same AppDomain", and the background thread is never joined or shut down. Guard the reuse path (check `HasShutdownStarted` / `Dispatcher.HasShutdownPendingShutdown` and create a fresh app instead) so the fixture does not poison the process for later tests.</comment>
<file context>
@@ -0,0 +1,160 @@
+ if (Application.Current != null)
+ return Task.FromResult(Application.Current);
+
+ var ready = new TaskCompletionSource<Application>(TaskCreationOptions.RunContinuationsAsynchronously);
+ // WPF permits only one Application per process. Keep its STA dispatcher alive for reuse.
+ var thread = new Thread(() =>
</file context>
There was a problem hiding this comment.
Addressed the unsafe reuse path in 6c15eb1: the helper now rejects a dispatcher whose shutdown has started/finished or whose thread is no longer alive, with an explicit fresh-process diagnostic.
The suggested shutdown-and-recreate approach is not valid for WPF: only one Application can be created per AppDomain, including after shutdown (https://learn.microsoft.com/en-us/dotnet/api/system.windows.application#remarks). The existing background dispatcher is intentionally retained for the repeated regression test; resources, MainWindow and ShutdownMode are restored after each verification.
This change does not provide process isolation for another fixture that independently constructs an Application. I am leaving the thread open rather than claiming that broader lifetime concern is resolved. The new commit has not been run locally on Windows; its CI result still needs verification.
Fixes #4655.
Theme previews could inherit border styling from the previously selected theme because
ApplyPreviewBackgroundreadWindowBorderStylefrom application resources before the new theme's resources were installed.Pass the selected theme's border style from
ColorizeWindowinto the preview helper. This preserves the existing resource-loading order, preview background handling, and blur-specific corner rules.Adds a WPF regression covering selection order (A → B → A and B after A, C, or B), inherited border properties, light/dark backgrounds, blur overrides, and previous styles containing runtime shadow effects and enlarged margins.
Before / after
Selecting Sublime immediately after Darker. The original preview misses Sublime’s gray outline; the fixed preview uses it. These are matching crops from the original Windows screenshots, enlarged 4× with arrows added.
Validation
git diff --checkpasses.Visual checks used an isolated portable profile. A temporary local plugin opened Settings through the public API; it is not included in this PR. The full repository test suite and exhaustive native backdrop rendering checks were not run. No recording is included.
Summary by cubic
Summary of changes
Fixes #4655. Theme previews now use the selected theme's border style instead of inheriting the previously selected theme's style.
ApplyPreviewBackgroundnow receives the selected theme'sWindowBorderStylefromColorizeWindowinstead of reading it fromApplication.Current.Resources.Flow.Launcher.Test/ThemePreviewTest.cs, a WPF regression test covering selection order, inherited border properties, light/dark backgrounds, blur overrides, and prior styles with runtime shadow effects and enlarged margins. The test refuses to run when the WPF application dispatcher is unavailable.Application.Current.Resourceslookup andContainscheck that caused the previous theme's style to be copied into the preview.Release Note
Theme previews now show the border style of the theme you select instead of borrowing the border from the theme you had open before.
Written for commit 6c15eb1. Summary will update on new commits.