Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a comprehensive Out-Of-Box Experience (OOBE) setup wizard for MaiChartManager, designed to simplify the initial configuration process for new users. It introduces a guided flow for selecting the game directory and choosing between local and remote operation modes. Alongside this, the PR centralizes configuration saving logic, enhances UI context management for better window handling, and integrates a dynamic language switcher into the application's frontend. These changes aim to improve user onboarding and overall application usability. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a significant and well-structured "Out-of-Box Experience" (OOBE) for new users, guiding them through initial setup with extensive changes to the C# backend and Vue/TSX frontend, including a new OobeController and a multi-step UI flow. A security audit identified critical issues such as broken access control on OOBE endpoints, potential remote denial of service, and insecure storage of administrative credentials in plain text, particularly when in "Remote Mode". Further review highlights concerns with UI responsiveness, exception handling in background tasks, and opportunities to improve type safety and user experience.
| public void Save() | ||
| { | ||
| var json = JsonSerializer.Serialize(this); | ||
| File.WriteAllText(Path.Combine(StaticSettings.appData, "config.json"), json); | ||
| } |
There was a problem hiding this comment.
The Config.Save() method stores sensitive administrative credentials (AuthPassword) in plain text within config.json, posing a significant security risk as any user with file system access can compromise the application. It is crucial to encrypt sensitive fields using platform-specific secure storage. Furthermore, the synchronous file I/O in Save() can block the UI thread, causing freezes, and the lack of indentation in the saved configuration reduces readability. It is recommended to use asynchronous file operations and restore WriteIndented = true for improved performance and maintainability.
public async Task SaveAsync()
{
var json = JsonSerializer.Serialize(this, new JsonSerializerOptions { WriteIndented = true });
await File.WriteAllTextAsync(Path.Combine(StaticSettings.appData, "config.json"), json);
}| public string? OpenFolderDialog() | ||
| { | ||
| string? result = null; | ||
| AppMain.UiContext?.Send(_ => | ||
| { | ||
| var dialog = new FolderBrowserDialog | ||
| { | ||
| ShowNewFolderButton = false, | ||
| }; | ||
| if (dialog.ShowDialog(AppMain.ActiveForm) == DialogResult.OK) | ||
| { | ||
| result = dialog.SelectedPath; | ||
| } | ||
| }, null); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The OpenFolderDialog endpoint triggers a blocking Windows FolderBrowserDialog on the server machine. If the application is configured for LAN access ("Remote Mode"), a remote attacker can trigger this dialog repeatedly. This causes UI disruption for the local user and can lead to a Denial of Service as the UI thread may become blocked. Similarly, InitializeGameData can trigger a MessageBox and call Application.Exit() on error, allowing a remote attacker to terminate the application.
Recommendation: Restrict UI-triggering endpoints to local requests only or ensure they are protected by strong authentication. Avoid calling Application.Exit() or showing blocking dialogs in response to remote API calls.
| public async Task<object> GetStartupStatus() | ||
| { | ||
| var startupTask = await Windows.ApplicationModel.StartupTask.GetAsync("MaiChartManagerStartupId"); | ||
| return new | ||
| { | ||
| enabled = startupTask.State == Windows.ApplicationModel.StartupTaskState.Enabled | ||
| || startupTask.State == Windows.ApplicationModel.StartupTaskState.EnabledByPolicy, | ||
| canChange = startupTask.State != Windows.ApplicationModel.StartupTaskState.DisabledByUser | ||
| && startupTask.State != Windows.ApplicationModel.StartupTaskState.DisabledByPolicy | ||
| && startupTask.State != Windows.ApplicationModel.StartupTaskState.EnabledByPolicy, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This action returns a Task<object> with an anonymous type. This can cause issues with OpenAPI/Swagger client generation, often leading to client-side code using any or void and losing type safety (as seen in ModeSelectPage.tsx). It's better to define a specific DTO class or record for the response to ensure type safety on the client side.
public record StartupStatusResponse(bool enabled, bool canChange);
[HttpGet]
public async Task<StartupStatusResponse> GetStartupStatus()
{
var startupTask = await Windows.ApplicationModel.StartupTask.GetAsync("MaiChartManagerStartupId");
return new StartupStatusResponse(
enabled: startupTask.State == Windows.ApplicationModel.StartupTaskState.Enabled
|| startupTask.State == Windows.ApplicationModel.StartupTaskState.EnabledByPolicy,
canChange: startupTask.State != Windows.ApplicationModel.StartupTaskState.DisabledByUser
&& startupTask.State != Windows.ApplicationModel.StartupTaskState.DisabledByPolicy
&& startupTask.State != Windows.ApplicationModel.StartupTaskState.EnabledByPolicy
);
}| while (retries < maxRetries) { | ||
| try { | ||
| const res = await api.GetLanAddresses(); | ||
| lanAddresses.value = res.data || []; | ||
| remoteReady.value = true; | ||
| completing.value = false; | ||
| break; | ||
| } catch { | ||
| retries++; | ||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||
| } | ||
| } | ||
| if (retries >= maxRetries) { | ||
| completing.value = false; | ||
| } |
There was a problem hiding this comment.
The polling loop in handleComplete to wait for the server restart has a catch {} block that swallows errors. If an error occurs during polling (e.g., network issue), it will be ignored. Also, if the polling times out after maxRetries, the completing state is simply turned off without notifying the user that something went wrong. This can leave the user in a confusing state.
It would be better to log errors and show a message to the user if the process fails.
while (retries < maxRetries) {
try {
const res = await api.GetLanAddresses();
lanAddresses.value = res.data || [];
remoteReady.value = true;
completing.value = false;
break;
} catch (err) {
console.error('Polling for server restart failed:', err);
retries++;
await new Promise(resolve => setTimeout(resolve, 500));
}
}
if (retries >= maxRetries) {
// TODO: Show an error message to the user.
completing.value = false;
}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…on, and splash gate
…t, remove startup checkbox
| public void InitializeGameData() | ||
| { | ||
| try | ||
| { |
There was a problem hiding this comment.
Redundant GetGameVersion calls in initialization paths
Low Severity
InitializeGameData() calls GetGameVersion() then RescanAll(), but RescanAll() itself now also calls GetGameVersion() at its start (newly added). The same double invocation occurs in the constructor. This parses the game's XML config file twice on every initialization.
Additional Locations (1)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured Out-of-Box Experience (OOBE) flow, reworking the application's startup and lifecycle management. The introduction of AppLifecycleManager, OobeBrowser, and WebViewHelper centralizes and cleans up window and process management logic. The backend and frontend changes to support asynchronous URL injection are robust. My review focuses on improving error handling in some of the new asynchronous and background operations to prevent silent failures and improve debuggability. I've also noted a couple of minor opportunities for code simplification and consistency.
| _ = Task.Run(async () => | ||
| { | ||
| await Task.Delay(100); | ||
| if (ServerManager.IsRunning) | ||
| { | ||
| await ServerManager.StopAsync(); | ||
| } | ||
| ServerManager.StartApp(request.Export, (url) => | ||
| { | ||
| if (StaticSettings.Config.Export) return; | ||
| AppLifecycleManager.ShowBrowser(url); | ||
| AppMain.UiContext?.Post(_ => | ||
| { | ||
| AppMain.OobeBrowser?.Dispose(); | ||
| AppMain.OobeBrowser = null; | ||
| }, null); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
A fire-and-forget task initiated with Task.Run can crash the entire application if an unhandled exception occurs within it. The server restart logic is critical and should be resilient to failures. Please wrap the code inside the Task.Run with a try-catch block to handle any potential exceptions and log them.
_ = Task.Run(async () =>
{
try
{
await Task.Delay(100);
if (ServerManager.IsRunning)
{
await ServerManager.StopAsync();
}
ServerManager.StartApp(request.Export, (url) =>
{
if (StaticSettings.Config.Export) return;
AppLifecycleManager.ShowBrowser(url);
AppMain.UiContext?.Post(_ =>
{
AppMain.OobeBrowser?.Dispose();
AppMain.OobeBrowser = null;
}, null);
});
}
catch (Exception ex)
{
logger.LogError(ex, "Failed to restart server after setup completion.");
}
});|
|
||
| public void Save() | ||
| { | ||
| var json = JsonSerializer.Serialize(this); |
There was a problem hiding this comment.
The Save() method is missing JsonSerializerOptions { WriteIndented = true }. This makes the saved config.json file a single line, which is harder for users to read and edit manually compared to the previous indented format. It's recommended to restore the indented serialization for better usability and consistency with how it was handled before this refactoring.
var json = JsonSerializer.Serialize(this, new JsonSerializerOptions { WriteIndented = true });| try | ||
| { | ||
| var startupTask = await Windows.ApplicationModel.StartupTask.GetAsync("MaiChartManagerStartupId"); | ||
| if (request.Export && request.StartupEnabled) | ||
| await startupTask.RequestEnableAsync(); | ||
| else | ||
| startupTask.Disable(); | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
This empty catch block hides potential issues with setting the startup task. Even if exceptions are expected in some environments (e.g., when not running as a packaged app), it's better to log them at a Warning or Debug level. This makes it clear why the startup task wasn't configured and helps with debugging, without treating it as a critical error.
try
{
var startupTask = await Windows.ApplicationModel.StartupTask.GetAsync("MaiChartManagerStartupId");
if (request.Export && request.StartupEnabled)
await startupTask.RequestEnableAsync();
else
startupTask.Disable();
}
catch (Exception ex)
{
logger.LogWarning(ex, "Failed to update startup task state. This can be expected if not running as a packaged app.");
}| } catch { | ||
| retries++; | ||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||
| } | ||
| } | ||
| if (retries >= maxRetries) { | ||
| completing.value = false; | ||
| } |
There was a problem hiding this comment.
The retry loop for waiting on the server restart has a couple of issues:
- The
catchblock is empty. It's good practice to log the error to aid in debugging, e.g.,console.error(e). - If the server fails to restart after all retries (
retries >= maxRetries), thecompletingflag is simply set tofalse, leaving the user on the current screen without any indication of what went wrong. You should provide feedback to the user, for example, by setting an error message in a state variable and displaying it in the UI.
|
|
||
| public void RescanAll() | ||
| { | ||
| GetGameVersion(); |
There was a problem hiding this comment.
The call to GetGameVersion() here is redundant. All call sites of RescanAll() (in the constructor and in InitializeGameData) already call GetGameVersion() just before calling RescanAll(). Removing this duplicate call will prevent unnecessary work.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| // Load icon from Launcher's embedded resources (avoids duplicating base64 data) | ||
| var rm = new ComponentResourceManager(typeof(Launcher)); | ||
| var icon = (Icon?)rm.GetObject("notifyIcon1.Icon"); |
There was a problem hiding this comment.
Indentation error in tray icon setup code
Low Severity
Line 25 (var icon = ...) has zero indentation while the surrounding code inside the Post lambda is properly indented. This appears to be an accidentally committed formatting issue.
| const localeOptions = availableLocales.map(l => ({ | ||
| label: localeLabels[l], | ||
| value: l, | ||
| })); |
There was a problem hiding this comment.
Duplicate locale label definitions across two components
Low Severity
The localeLabels record and localeOptions array are identically defined in both WelcomePage.tsx and Settings/index.tsx. This duplicated locale-mapping logic could diverge over time and would be better extracted into a shared constant.
Additional Locations (1)
| } catch (e: any) { | ||
| pathError.value = t('oobe.pathNotValid'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
OOBE API calls lack backend URL readiness guard
Medium Severity
In the first-time OOBE flow, the OobeBrowser is created before the server starts, and backendUrl is injected asynchronously later. The GameDirPage calls api.OpenFolderDialog() and api.SetGamePath() without awaiting ensureBackendUrl() first. Similarly goNext calls api.InitializeGameData() without the guard. If the user navigates quickly past the Welcome page and clicks Browse before the server is ready, the API calls hit mcm.invalid (the virtual host) instead of the actual backend, and silently fail. The ensureBackendUrl utility exists and is used in Index.tsx and StartupErrorDialog but is missing here.
Additional Locations (1)
1 participant
Note
Medium Risk
Changes app startup/lifecycle and server startup/restart behavior, plus adds new OOBE endpoints that modify config/auth/startup-task settings; regressions could prevent UI launch or leave background processes running.
Overview
Adds a new WebView2-driven OOBE/setup flow (frontend routes +
OobeBrowser) to select game directory, choose local vs LAN (export) mode, optionally enable basic auth, and show server-ready LAN addresses.Reworks desktop startup/lifecycle: introduces
AppLifecycleManagerfor tray icon + window activation + exit rules;AppMainnow conditionally starts in OOBE, main UI, or tray-only mode and injectsbackendUrlinto WebViews afterServerManager.StartAppreports the loopback URL.Backend/infra updates include new
OobeControllerAPIs (folder dialog, game path, initialize scan, complete setup w/ server restart and startup-task toggling), delayedStaticSettingsscanning viaInitializeGameData, centralizedConfig.Save(), and a sharedWebViewHelper; the frontend also waits for injectedbackendUrl(ensureBackendUrl) and updates the API client base URL dynamically.Written by Cursor Bugbot for commit 18a2f2f. This will update automatically on new commits. Configure here.