Fix Dependency Injection Issue #3275
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe pull request introduces enhanced error handling in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🔇 Additional comments (7)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher/App.xaml.cs (1)
105-105: Good addition to fix the dispatcher thread issue.This change correctly initializes the PublicAPIInstance with the MainViewModel after it's fully constructed, which resolves the dispatcher thread issue mentioned in the PR objectives.
However, the explicit cast to
PublicAPIInstancebypasses the abstraction provided by theIPublicAPIinterface. Consider extending theIPublicAPIinterface to include theInitializemethod for better design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
37-37: Field properly changed to support later initialization.Changing from a readonly field to a regular field allows setting the MainViewModel after construction, which helps address the dispatcher thread issue.
41-46: Constructor simplified to remove MainViewModel dependency.Removing the MainViewModel parameter from the constructor is the right approach to fix the dispatcher thread issue, as it allows the MainViewModel to be fully initialized before being assigned to the PublicAPIInstance.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Flow.Launcher/App.xaml.cs (3)
38-48: Good addition of error handling for settings initialization.The addition of try-catch to handle exceptions during settings initialization improves robustness. Consider adding more specific error handling strategies based on exception types, and possibly logging the exception before displaying the message box for better diagnostics.
try { var storage = new FlowLauncherJsonStorage<Settings>(); _settings = storage.Load(); _settings.SetStorage(storage); _settings.WMPInstalled = WindowsMediaPlayerHelper.IsWindowsMediaPlayerInstalled(); } catch (Exception e) { + Log.Error($"|App.ctor|Cannot load setting storage", e); MessageBox.Show($"Cannot load setting storage: {e}"); + // Consider handling critical failures more gracefully + // e.g., attempting to load default settings or terminating the application }
51-72: Enhanced error handling for dependency injection.The try-catch block is a good addition to improve error handling. However, if DI configuration fails, the application is likely to encounter other issues. Consider defining a strategy for critical failures.
try { // DI configuration } catch (Exception e) { + Log.Error($"|App.ctor|Cannot configure dependency injection container", e); MessageBox.Show($"Cannot configure dependency injection container: {e}"); + // Consider whether to continue or terminate the application after serious DI errors + // Environment.Exit(1); }
75-83: Improved error handling for API initialization.Adding error handling for API and settings initialization is important. Like the previous blocks, consider adding logging and a strategy for recovering or gracefully terminating.
try { API = Ioc.Default.GetRequiredService<IPublicAPI>(); _settings.Initialize(); } catch (Exception e) { + Log.Error($"|App.ctor|Cannot initialize public API and settings", e); MessageBox.Show($"Cannot initialize public API and settings: {e}"); + // Consider a recovery strategy or graceful termination }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/App.xaml.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Flow.Launcher/App.xaml.cs (1)
126-126:✅ Verification successful
Good fix for the dispatcher thread issue.
This change addresses the PR objective by deferring the initialization of MainViewModel in the PublicAPIInstance. This approach properly separates object construction from initialization, which is important for resolving dispatcher thread issues.
To ensure this is the best approach, please verify:
🏁 Script executed:
#!/bin/bash # Check if PublicAPIInstance.cs has been updated to support this initialization pattern rg -A 10 "class PublicAPIInstance" | grep -A 10 "Initialize" # Look for other potential dispatcher thread issues with MainViewModel rg "MainViewModel" --context=5Length of output: 18907
Dispatcher Thread Fix Verified
The update in
PublicAPIInstance.csthat introduces theInitialize(MainViewModel mainVM)method has been verified. The method correctly defers the initialization of theMainViewModel, ensuring that object construction and initialization are separated to avoid dispatcher thread issues. Additionally, a review of references toMainViewModelacross the codebase did not reveal any further dispatcher-related concerns.
- Confirmed that
PublicAPIInstance.csnow contains a dedicatedInitialize(MainViewModel mainVM)method.- Verified that
App.xaml.csproperly retrieves theMainViewModelinstance and initializes the PublicAPI accordingly.- No additional dispatcher thread issues were detected throughout the code.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/App.xaml.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (5)
Flow.Launcher/App.xaml.cs (5)
38-49: Improved error handling for settings initialization.Adding error handling around settings initialization is a good practice to prevent crashes due to file access issues or corrupted settings files. The specific error message provides useful guidance to users.
52-74: Enhanced robustness with DI container error handling.The try-catch block around dependency injection configuration improves the application's error resilience. This change aligns with the PR objective to address initialization issues.
77-86: Proper API and settings initialization error handling.This error handling ensures that any exceptions during API and settings initialization are properly caught and displayed to the user, preventing silent failures.
89-92: Good extraction of error display logic.Extracting the error display logic into a separate method follows the DRY principle and ensures consistent error reporting across all initialization stages.
134-134: Fixed dispatcher thread issue in dependency injection.This is the key change that addresses the PR objective. Moving the MainViewModel initialization from the constructor to a post-construction initialization method helps resolve dispatcher thread issues.
Previously, MainViewModel was likely being created on the wrong thread when passed directly to the PublicAPIInstance constructor. The new approach ensures proper thread affinity by explicitly initializing it at the right time in the startup sequence.
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
|
Why would mainviewmodel related to dispatcher? Only the UI code requires a dispatcher. On the other hand, the DI is created on a UI thread. |
Sorry for this confusion. I really do not know how can I name this issue. 😢 I do not know why, but we cannot create MainViewModel instance in PublicAPI constructor... It can cause all functions related to _mainVM cannot work. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/Flow.Launcher.csproj (1)
94-96: Clarify the Rationale for Dependency Downgrade of DI and Hosting Packages.The change downgrades
Microsoft.Extensions.DependencyInjectionandMicrosoft.Extensions.Hostingfrom version7.0.0to6.0.2due to a TaskScheduler incompatibility with the 7.0.x versions. This modification appears appropriate given the issue; however, please ensure that all features requiring these packages are verified against the downgraded versions in your .NET 7 context. Additionally, consider refining the comment wording for clarity for future maintainers. For example:- <!-- Do not upgrade Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Hosting since we are .Net7.0 --> + <!-- Do not upgrade Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Hosting to 7.0.x versions; TaskScheduler incompatibility necessitates using 6.0.2 -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/Flow.Launcher.csproj(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/Flow.Launcher.csproj (1)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-11-12T03:58:56.325Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
This reverts commit d48b4df.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
The fix doesn't make sense to me. Mainviewmodel does not have anything related to UI (all viewmodel operation). Could you help me understand how do you think this is the problem? I build flow with debug/release on dev and run it with/without debugger. Both doesn't show problem. I haven't tested the CI build, and will do that now. |
|
The error shown in event viewer is this Even with this pr, the same error appears. |
Checkout this branch https://github.com/Jack251970/Flow.Launcher/tree/dependency_injection_installer and this is strange behaviour: 2025-02-26.15-12-48.mp4I find the functions like |
This is the issue related with |
|
I tried the debug build on dev and cannot reproduce what you get? |
Well, let us separate this PR. |
_mainVMin api instance cannot work (Tested)System.PlatformNotSupportedExceptionand it was in progess: Microsoft.Extensions.DependencyInjection Combability Issue dahall/TaskScheduler#1001