Single executable: GUI + CLI in AsusFanControl.exe - #25
Conversation
Remove the legacy console project and build AsusFanControl.exe from the WinForms app (AssemblyName AsusFanControl). Add CliProgram for CLI mode with optional --debug-log, attach console when args are present, and Costura.Fody to embed managed dependencies. Update the solution to Core + GUI only, tune CI artifact staging for the new output layout, and document same-exe usage in the README. Made-with: Cursor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThe pull request consolidates separate CLI and GUI executables into a single dual-mode executable. The Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Main as AsusFanControl.exe<br/>(Main)
participant GUI as WinForms<br/>(Form1)
participant CLI as CliProgram
participant Controller as AsusControl<br/>(IFanController)
alt No Arguments (GUI Mode)
User->>Main: Double-click or no args
Main->>Main: EnableVisualStyles,<br/>SetCompatibleTextRenderingDefault
Main->>GUI: Run Form1
GUI->>User: Display GUI Window
else Arguments Present (CLI Mode)
User->>Main: AsusFanControl.exe --get-fan-speeds
Main->>Main: ExtractDebugLogFlag<br/>(parse arguments)
Main->>Main: Create DebugLogSession<br/>(if --debug-log)
Main->>Main: AttachConsole/AllocConsole
Main->>CLI: Run(args)
CLI->>Controller: new AsusControl()
Controller->>CLI: IFanController instance
CLI->>Controller: Execute commands<br/>(--get-fan-speeds,<br/>--set-fan-speed, etc.)
Controller->>CLI: Return results
CLI->>User: Write to Console
Note over CLI: Register AppDomain handlers<br/>for ProcessExit & Exception
CLI->>Controller: Reset to defaults<br/>(if not --skip-reset)
CLI->>Controller: Dispose
CLI->>Main: Exit code
Main->>User: Return exit code
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consolidates the CLI and GUI versions of AsusFanControl into a single executable. The GUI project now serves as the entry point, detecting command-line arguments to switch between modes. Feedback focuses on correctly initializing the console for CLI mode in a Windows application, specifically by redirecting standard streams and ensuring the console is attached before logging begins. Additionally, it is recommended to use synchronous methods instead of wrapping them in tasks during process exit to avoid redundancy.
| static void EnsureConsoleForCli() | ||
| { | ||
| if (!AttachConsole(ATTACH_PARENT_PROCESS)) | ||
| AllocConsole(); | ||
| } |
There was a problem hiding this comment.
In a WinExe project, calling AttachConsole or AllocConsole is not enough to enable Console.WriteLine because the standard streams are initially directed to a null device. You must also redirect the C# Console output and error streams to the newly available console handles.
static void EnsureConsoleForCli()
{
if (!AttachConsole(ATTACH_PARENT_PROCESS))
AllocConsole();
// Redirect standard streams to the console
var standardOutput = new System.IO.StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true };
Console.SetOut(standardOutput);
var standardError = new System.IO.StreamWriter(Console.OpenStandardError()) { AutoFlush = true };
Console.SetError(standardError);
}| args = CliProgram.ExtractDebugLogFlag(args, out string debugLogFile); | ||
| using (debugLogFile != null ? CliProgram.DebugLogSession.Create(debugLogFile) : null) | ||
| { | ||
| if (debugLogFile != null) | ||
| Console.WriteLine("[debug-log] Writing diagnostics to: " + System.IO.Path.GetFullPath(debugLogFile)); | ||
|
|
||
| if (args.Length < 1) | ||
| { | ||
| EnsureConsoleForCli(); | ||
| CliProgram.PrintUsage(); | ||
| return 1; | ||
| } | ||
|
|
||
| EnsureConsoleForCli(); | ||
| return CliProgram.Run(args); | ||
| } |
There was a problem hiding this comment.
The current placement of EnsureConsoleForCli() causes several issues in CLI mode:
- The diagnostic message on line 44 is printed before the console is attached, so it will be lost when running from a standard command prompt.
DebugLogSession.CreatecapturesConsole.Outbefore the console is attached. Since this is aWinExe,Console.Outis initially a null writer. Even after the console is attached later, theTeeTextWriterwill continue to use the null writer as its primary output, resulting in no console output.- Moving
EnsureConsoleForCli()to the top of the CLI branch ensures that all subsequentConsolecalls (including those insideDebugLogSession) work correctly.
EnsureConsoleForCli();
args = CliProgram.ExtractDebugLogFlag(args, out string debugLogFile);
using (debugLogFile != null ? CliProgram.DebugLogSession.Create(debugLogFile) : null)
{
if (debugLogFile != null)
Console.WriteLine("[debug-log] Writing diagnostics to: " + System.IO.Path.GetFullPath(debugLogFile));
if (args.Length < 1)
{
CliProgram.PrintUsage();
return 1;
}
return CliProgram.Run(args);
}| { | ||
| if (!isDisposed && !skipResetOnExit) | ||
| { | ||
| try { asusControl.ResetToDefaultAsync().GetAwaiter().GetResult(); } |
There was a problem hiding this comment.
Using ResetToDefaultAsync().GetAwaiter().GetResult() is redundant and potentially problematic during process exit, as it unnecessarily wraps a synchronous operation in a Task.Run. Use the synchronous ResetToDefault() method directly.
try { asusControl.ResetToDefault(); }| if (!skipResetOnExit) | ||
| { | ||
| try { asusControl.ResetToDefaultAsync().GetAwaiter().GetResult(); } | ||
| catch (Exception ex) { Console.Error.WriteLine("[UnhandledException] Reset error: " + ex.Message); } |
There was a problem hiding this comment.
Using ResetToDefaultAsync().GetAwaiter().GetResult() is redundant here. Use the synchronous ResetToDefault() method directly.
try { asusControl.ResetToDefault(); }| { | ||
| if (!skipResetOnExit) | ||
| { | ||
| try { asusControl.ResetToDefaultAsync().GetAwaiter().GetResult(); } |
There was a problem hiding this comment.
Using ResetToDefaultAsync().GetAwaiter().GetResult() is redundant here. Use the synchronous ResetToDefault() method directly.
try { asusControl.ResetToDefault(); }![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| catch (Exception ex) { Console.Error.WriteLine("[ProcessExit] Reset error: " + ex.Message); } | ||
| } | ||
| }; | ||
| AppDomain.CurrentDomain.UnhandledException += (s, e) => |
There was a problem hiding this comment.
CRITICAL: Resource cleanup logic has double disposal and potential double reset.
In the UnhandledException event handler, asusControl.Dispose() is called without checking isDisposed, which can lead to double disposal when the finally block also calls Dispose(). Additionally, both the UnhandledException handler and the finally block may call ResetToDefaultAsync() if skipResetOnExit is false, causing a double reset. This could result in exceptions or undefined behavior. Ensure cleanup logic executes exactly once by setting isDisposed = true before calling cleanup methods, or by checking isDisposed in all handlers.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)None. Files Reviewed (2 files)
Reviewed by trinity-large-thinking · 866,929 tokens |
![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 the current code and only fix it if needed.
Inline comments:
In `@AsusFanControlGUI/AsusFanControlGUI.csproj`:
- Around line 8-10: The project uses OutputType WinExe so console is not
attached by default; move the console-attachment logic
(EnsureConsoleForCli()/AttachConsole/AllocConsole calls) in Program.cs to
execute before any debug/log messages that write to the console (specifically
the debug-log path message currently printed early in Main), so that those
messages are not lost — call EnsureConsoleForCli() at the very start of the CLI
path in Main (before the debug/log path output) and ensure any helper like
EnsureConsoleForCli or AttachConsole is idempotent and safe to call from GUI
mode.
In `@AsusFanControlGUI/CliProgram.cs`:
- Around line 14-44: TeeTextWriter currently doesn't override Flush so flush
calls won't be forwarded to both underlying writers; add an override for Flush()
in the sealed class (and optionally override FlushAsync(CancellationToken) if
async flushing is used) that calls _primary.Flush() and _secondary.Flush(); also
ensure any Dispose/Close paths in TeeTextWriter forward to both _primary and
_secondary if not already handled.
- Around line 95-118: The Create method in DebugLogSession should defensively
handle empty or whitespace-only path input: check
string.IsNullOrWhiteSpace(path) at the top of DebugLogSession.Create and, if
true, replace path with DefaultDebugLogPath() (or throw ArgumentException based
on desired behavior), then proceed to compute dir via
Path.GetDirectoryName(Path.GetFullPath(path)); ensure you trim the path before
use so Directory.CreateDirectory(dir) and new StreamWriter(path, ...) never
receive an empty string; update references to path, dir, and fileWriter
accordingly.
- Around line 217-240: The parsed speed value for --set-fan-speeds is not
range-validated; update the parsing block that reads parts[1] and the code path
that calls asusControl.SetFanSpeeds(newSpeed) to ensure newSpeed is between 0
and 100 inclusive, printing an error like "Error: Speed must be between 0 and
100" and not calling SetFanSpeeds or setting skipResetOnExit when out of range;
apply the same 0–100 validation to the separate --set-fan-speed parsing branch
(the other handler that calls asusControl.SetFanSpeeds) so negative or >100
values are rejected and only valid values trigger the Test mode/new speed
messages.
- Around line 189-206: The UnhandledException handler can call Dispose twice
because it doesn't check isDisposed; update the
AppDomain.CurrentDomain.UnhandledException lambda to mirror the ProcessExit
handler by checking isDisposed (and skipResetOnExit where already used) before
calling asusControl.ResetToDefaultAsync() and before calling
asusControl.Dispose(), and ensure both
ResetToDefaultAsync().GetAwaiter().GetResult() and asusControl.Dispose() are
wrapped in try/catch blocks that log exceptions; reference the
UnhandledException handler, isDisposed, skipResetOnExit,
asusControl.ResetToDefaultAsync(), and asusControl.Dispose() when making this
change.
- Around line 340-341: The Run method currently always returns 0 even on
parse/argument errors; update Run (in CliProgram.Run) to return a non-zero exit
code on failure cases: detect parsing failures or unknown arguments from the
parser result and return 1 (or other defined error codes) instead of 0, and
ensure callers (e.g., Main) propagate that code to Environment.Exit or return it
to the OS; modify any catch blocks in Run to return a non-zero code on
exceptions and document/centralize the numeric code(s) used for different
failure types.
In `@AsusFanControlGUI/Program.cs`:
- Around line 40-55: The debug-log path write happens before a console is
guaranteed to exist so the message can be lost; after calling
CliProgram.ExtractDebugLogFlag(...) call EnsureConsoleForCli() immediately
(before creating the DebugLogSession and before any Console.WriteLine), then
proceed to create the debug session and write the "[debug-log]" message only
after EnsureConsoleForCli() has been invoked; update the Program.Main flow
around debugLogFile, CliProgram.DebugLogSession.Create, and EnsureConsoleForCli
to ensure console attachment occurs prior to any Console.WriteLine.
🪄 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
Run ID: 92fa1d17-e97f-4f36-8de0-3f8ac036b9f3
📒 Files selected for processing (12)
.github/workflows/dotnet-desktop.ymlAsusFanControl.slnAsusFanControl/App.configAsusFanControl/AsusFanControl.csprojAsusFanControl/Program.csAsusFanControl/Properties/AssemblyInfo.csAsusFanControlGUI/AsusFanControlGUI.csprojAsusFanControlGUI/CliProgram.csAsusFanControlGUI/FodyWeavers.xmlAsusFanControlGUI/Program.csAsusFanControlGUI/Properties/AssemblyInfo.csREADME.md
💤 Files with no reviewable changes (4)
- AsusFanControl/App.config
- AsusFanControl/AsusFanControl.csproj
- AsusFanControl/Properties/AssemblyInfo.cs
- AsusFanControl/Program.cs
* Add profile manager core types and profile editor UI Introduce ProfileManager and FanProfile serialization in Core, a Windows Task Scheduler helper for future automation hooks, and WinForms pieces (DarkMenuRenderer, ProfileEditorDialog) compiled into the GUI project. Form wiring can follow in a later change. Made-with: Cursor * Apply reviewer suggestions for PR #26
|
/gemini review |
| public class ProfileManager | ||
| { | ||
| private readonly List<FanProfile> _profiles = new List<FanProfile>(); | ||
| private readonly object _lock = new object(); |
There was a problem hiding this comment.
WARNING: Unused lock object (_lock) declared but never used. This suggests the class was intended to be thread-safe but the locking was omitted. If accessed from multiple threads concurrently, this could lead to race conditions and data corruption.
There was a problem hiding this comment.
Code Review
This pull request refactors the application to merge the CLI and GUI into a single executable and introduces a profile management system for automatic fan curve switching based on active processes. It also includes a new utility for managing auto-start tasks via the Windows Task Scheduler. The review feedback highlights several compatibility issues where C# 8.0 syntax and .NET Core-specific methods were used in a .NET Framework 4.7.2 project, which will lead to compilation errors. Additionally, the feedback points out a lack of thread safety in the profile manager, restrictive path validation in task registration, and potential performance issues related to frequent process enumeration.
| if (string.IsNullOrEmpty(processName)) return null; | ||
| var name = processName; | ||
| if (name.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) | ||
| name = name[..^4]; |
There was a problem hiding this comment.
The project targets .NET Framework 4.7.2 and explicitly sets <LangVersion>7.3</LangVersion> in the project file. The range operator [..^4] is a C# 8.0 feature and requires the System.Index type, which is not available in .NET Framework 4.7.2. This will cause a compilation error.
if (name.EndsWith(".exe", StringComparison.OrdinalIgnoreCase))
name = name.Substring(0, name.Length - 4);| { | ||
| if (!proc.WaitForExit(timeoutMs)) | ||
| { | ||
| try { proc.Kill(true); } catch { } |
There was a problem hiding this comment.
The proc.Kill(true) overload (which kills the process tree) was introduced in .NET Core 3.0 and .NET Standard 2.1. It is not available in .NET Framework 4.7.2, which this project targets. This will result in a compilation error.
try { proc.Kill(); } catch { }| public class ProfileManager | ||
| { | ||
| private readonly List<FanProfile> _profiles = new List<FanProfile>(); | ||
| private readonly object _lock = new object(); |
There was a problem hiding this comment.
The _lock object is defined but never used. Since _profiles is a List<FanProfile>, and CheckActiveProfile might be called from a background thread while the UI thread modifies the list (via AddProfile or RemoveProfile), this class is currently not thread-safe and could throw an InvalidOperationException during iteration.
| var args = exePath.IndexOfAny(new[] { '"', '\n', '\r', ';', '&', '|', '>', '<' }) >= 0; | ||
| if (args) | ||
| return false; |
There was a problem hiding this comment.
This validation is overly restrictive. Including the double quote character " in the IndexOfAny check will cause RegisterTask to return false if the user provides a quoted path, which is common for paths containing spaces. Additionally, schtasks.exe correctly handles quoted paths in the /TR argument.
var args = exePath.IndexOfAny(new[] { '\n', '\r', ';', '&', '|', '>', '<' }) >= 0;
if (args)
return false;| try | ||
| { | ||
| var runningProcesses = new HashSet<string>( | ||
| Process.GetProcesses().Select(p => |
There was a problem hiding this comment.
Process.GetProcesses() is an expensive operation as it enumerates all system processes. If CheckActiveProfile is called frequently (e.g., from a timer), this will lead to high CPU usage. Consider using a more efficient way to check for specific running processes, such as WMI queries or P/Invoke with EnumProcesses.
| { | ||
| if (!isDisposed && !skipResetOnExit) | ||
| { | ||
| try { asusControl.ResetToDefaultAsync().GetAwaiter().GetResult(); } |
There was a problem hiding this comment.
There is no need to use GetAwaiter().GetResult() on the async method here, as a synchronous ResetToDefault() method is already available in the IFanController interface. Blocking on async tasks in this manner can lead to deadlocks in certain synchronization contexts.
try { asusControl.ResetToDefault(); }
1 participant
This pull request removes the separate console project and ships one
AsusFanControl.exebuilt from the WinForms project (withAssemblyNameset to AsusFanControl). Command-line mode runs when any arguments are passed; double-click or no args starts the GUI.Changes include
CliProgram(CLI logic, optional--debug-log), console attach for CLI, Costura.Fody to embed dependencies, solution updated to Core + GUI only, CI artifact paths aligned with the new output, and README updates for same-exe usage.Made with Cursor
Summary by CodeRabbit
New Features
Documentation