Repository navigation
Conversation
![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 mechanism to ensure configuration is loaded from disk before services are initialized in the main process. It also removes redundant cache clearing logic in the ProjectScanner. A critical issue was identified regarding a potential race condition where the imported configManager instance might not be synchronized with the state of the configManagerPromise, and a suggestion was provided to initialize the instance directly to ensure consistency.
| logger.info('App ready, initializing...'); | ||
| try { | ||
| // Wait for config to finish loading from disk before using it | ||
| await configManagerPromise; |
There was a problem hiding this comment.
There is a potential race condition in ConfigManager.ts that may cause the configManager instance imported at line 63 to be different from the one initialized by configManagerPromise.
Specifically, ConfigManager.getInstance() (which populates the configManager export) may create a default instance before ConfigManager.initializeInstance() (which populates configManagerPromise) finishes and overwrites the static singleton. Since the configManager export is a const binding to the object returned by getInstance() at load time, it will not be updated when the singleton is overwritten. Consequently, initializeServices() might still use default settings even after the await.
To fix this properly, ConfigManager.ts should be updated to ensure both exports share the same instance. As a workaround in this file, you can initialize the imported instance directly to ensure it has the latest data from disk.
| await configManagerPromise; | |
| await configManager.initialize(); |
There was a problem hiding this comment.
Told Claude about this. Update coming.
There was a problem hiding this comment.
Claude's update is the change in src/main/services/infrastructure/ConfigManager.ts. I republished and redeployed and it is still working.
There was a problem hiding this comment.
I told Claude: Gemini suggested await configManagerPromise; -> await configManager.initialize();
📝 WalkthroughWalkthroughAwait configuration loading at startup by awaiting Changes
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/services/infrastructure/ConfigManager.ts (1)
970-974: Dual export pattern is appropriate; consider clarifying usage in the comment.The pattern correctly provides both a promise for initialization sequencing and synchronous access to the same instance. The comment on line 973 accurately states the instance "uses defaults until initialized."
For maintainability, consider expanding the comment to explicitly state that callers should
await configManagerPromisebefore usingconfigManager:📝 Suggested clarification
/** Singleton instance promise - resolves to an initialized ConfigManager */ export const configManagerPromise = ConfigManager.initializeInstance(); -/** Singleton instance for convenience (synchronous access, uses defaults until initialized) */ +/** + * Singleton instance for convenience (synchronous access, uses defaults until initialized). + * Callers must await configManagerPromise before relying on disk-loaded config. + */ export const configManager = ConfigManager.getInstance();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/infrastructure/ConfigManager.ts` around lines 970 - 974, Update the comment above the dual exports to explicitly document usage: state that configManagerPromise is the initialization promise from ConfigManager.initializeInstance() and that callers should await configManagerPromise before relying on runtime-loaded configuration from the synchronous export configManager (from ConfigManager.getInstance()), otherwise configManager will return default values until initialization completes; mention both exports reference the same singleton instance so awaiting the promise ensures the same instance is fully initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/services/infrastructure/ConfigManager.ts`:
- Around line 970-974: Update the comment above the dual exports to explicitly
document usage: state that configManagerPromise is the initialization promise
from ConfigManager.initializeInstance() and that callers should await
configManagerPromise before relying on runtime-loaded configuration from the
synchronous export configManager (from ConfigManager.getInstance()), otherwise
configManager will return default values until initialization completes; mention
both exports reference the same singleton instance so awaiting the promise
ensures the same instance is fully initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1005c645-cc47-4e0e-bddf-6bd26840b6ef
📒 Files selected for processing (1)
src/main/services/infrastructure/ConfigManager.ts
| for (const key of this.sessionMetadataCache.keys()) { | ||
| if (key.startsWith(prefix)) this.sessionMetadataCache.delete(key); | ||
| } | ||
| for (const key of this.sessionPreviewCache.keys()) { |
There was a problem hiding this comment.
Claude says: Removed two references to sessionPreviewCache which was never declared, causing a TypeScript compile error.
| logger.info('App ready, initializing...'); | ||
| try { | ||
| // Wait for config to finish loading from disk before using it | ||
| await configManagerPromise; |
There was a problem hiding this comment.
Told Claude about this. Update coming.
| logger.info('App ready, initializing...'); | ||
| try { | ||
| // Wait for config to finish loading from disk before using it | ||
| await configManagerPromise; |
There was a problem hiding this comment.
Claude's update is the change in src/main/services/infrastructure/ConfigManager.ts. I republished and redeployed and it is still working.
|
Actually, I am not sure it is working. Converted to draft. |
- Await configManagerPromise in app.whenReady() so config is loaded from disk before initializeServices() and getConfig() are called - Fix ConfigManager singleton race: set instance synchronously before awaiting initialize() so getInstance() returns the same object - Remove undeclared sessionPreviewCache references (TS error) - Load config from disk in standalone/Docker mode; use CLAUDE_ROOT path when set so Docker mounts are respected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| @@ -13,14 +13,15 @@ | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
Fixes for running in Docker.
| @@ -61,6 +61,7 @@ process.on('uncaughtException', (error) => { | |||
| import { HttpServer } from './services/infrastructure/HttpServer'; | |||
| import { | |||
There was a problem hiding this comment.
Fixes for running in Electron.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/infrastructure/ConfigManager.ts`:
- Around line 357-362: Make initializeInstance single-flight and guard against
conflicting configPath values: add a static initializingPromise (or similar)
that you set before calling instance.initialize() so concurrent callers await
the same promise instead of starting parallel initialize() flows; if
initializeInstance is called while initializingPromise exists, compare the
incoming configPath to the already-registered instance.configPath (or the
initial configPath passed when creating the first ConfigManager) and reject or
normalize if they differ to avoid out-of-order side effects (e.g.,
setClaudeBasePathOverride). Ensure you clear initializingPromise on
completion/failure and keep the assignment to ConfigManager.instance only once
(first-writer wins) so subsequent callers get the same instance.
In `@src/main/standalone.ts`:
- Around line 90-99: The startup config initialization in async function start
currently awaits ConfigManager.initializeInstance(...) without catching errors,
which can leave the process partially started; wrap the initializeInstance call
in a try/catch inside start and on error log the error (use console.error and/or
logger.error) and exit the process with a non-zero code (e.g., process.exit(1));
ensure you reference the ConfigManager.initializeInstance call and preserve
existing logging around logger.info('Starting standalone server...') while
failing fast when initialization throws.
🪄 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: CHILL
Plan: Pro
Run ID: 36328b3e-5f9e-4bad-9417-11bb793bf63c
📒 Files selected for processing (4)
src/main/index.tssrc/main/services/discovery/ProjectScanner.tssrc/main/services/infrastructure/ConfigManager.tssrc/main/standalone.ts
💤 Files with no reviewable changes (1)
- src/main/services/discovery/ProjectScanner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/index.ts
| static async initializeInstance(configPath?: string): Promise<ConfigManager> { | ||
| const instance = new ConfigManager(configPath); | ||
| await instance.initialize(); | ||
| ConfigManager.instance = instance; | ||
| await instance.initialize(); | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
Prevent concurrent initializeInstance() calls from racing with different config paths.
Publishing the instance before await fixes the previous duplication bug, but this still allows parallel initializations (e.g., module-level configManagerPromise plus standalone Line 98). That can run two initialize() flows and apply side effects out of order (setClaudeBasePathOverride), making startup nondeterministic.
💡 Suggested fix (single-flight initialization + path consistency guard)
export class ConfigManager {
private config: AppConfig;
private readonly configPath: string;
private static instance: ConfigManager | null = null;
+ private static initializePromise: Promise<ConfigManager> | null = null;
+ private static initializePath: string | undefined;
private triggerManager: TriggerManager;
@@
static async initializeInstance(configPath?: string): Promise<ConfigManager> {
+ if (ConfigManager.initializePromise) {
+ if (ConfigManager.initializePath !== configPath) {
+ throw new Error(
+ `ConfigManager already initializing with a different path: ${ConfigManager.initializePath ?? '<default>'}`
+ );
+ }
+ return ConfigManager.initializePromise;
+ }
+
const instance = new ConfigManager(configPath);
ConfigManager.instance = instance;
- await instance.initialize();
- return instance;
+ ConfigManager.initializePath = configPath;
+ ConfigManager.initializePromise = instance
+ .initialize()
+ .then(() => instance)
+ .catch((error) => {
+ ConfigManager.instance = null;
+ throw error;
+ })
+ .finally(() => {
+ ConfigManager.initializePromise = null;
+ });
+ return ConfigManager.initializePromise;
}
@@
static resetInstance(): void {
ConfigManager.instance = null;
+ ConfigManager.initializePromise = null;
+ ConfigManager.initializePath = undefined;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/infrastructure/ConfigManager.ts` around lines 357 - 362,
Make initializeInstance single-flight and guard against conflicting configPath
values: add a static initializingPromise (or similar) that you set before
calling instance.initialize() so concurrent callers await the same promise
instead of starting parallel initialize() flows; if initializeInstance is called
while initializingPromise exists, compare the incoming configPath to the
already-registered instance.configPath (or the initial configPath passed when
creating the first ConfigManager) and reject or normalize if they differ to
avoid out-of-order side effects (e.g., setClaudeBasePathOverride). Ensure you
clear initializingPromise on completion/failure and keep the assignment to
ConfigManager.instance only once (first-writer wins) so subsequent callers get
the same instance.
| async function start(): Promise<void> { | ||
| logger.info('Starting standalone server...'); | ||
|
|
||
| // Load config from disk before anything else uses it. | ||
| // When CLAUDE_ROOT is set (e.g. Docker), config lives there rather than ~/.claude. | ||
| const configPath = CLAUDE_ROOT | ||
| ? path.join(CLAUDE_ROOT, 'claude-devtools-config.json') | ||
| : undefined; | ||
| await ConfigManager.initializeInstance(configPath); | ||
|
|
There was a problem hiding this comment.
Wrap startup config initialization in try/catch and fail fast.
If initializeInstance() rejects, void start() falls through to global unhandled rejection logging and can leave the process in a partial startup state. Handle this locally and exit with a non-zero code.
🛠️ Suggested patch
async function start(): Promise<void> {
- logger.info('Starting standalone server...');
+ try {
+ logger.info('Starting standalone server...');
@@
- const configPath = CLAUDE_ROOT
- ? path.join(CLAUDE_ROOT, 'claude-devtools-config.json')
- : undefined;
- await ConfigManager.initializeInstance(configPath);
+ const configPath = CLAUDE_ROOT
+ ? path.join(CLAUDE_ROOT, 'claude-devtools-config.json')
+ : undefined;
+ await ConfigManager.initializeInstance(configPath);
@@
- const port = await httpServer.start(services, modeSwitchHandler, PORT, HOST);
- logger.info(`Standalone server running at http://${HOST}:${port}`);
- logger.info('Open in your browser to view Claude Code sessions');
+ const port = await httpServer.start(services, modeSwitchHandler, PORT, HOST);
+ logger.info(`Standalone server running at http://${HOST}:${port}`);
+ logger.info('Open in your browser to view Claude Code sessions');
+ } catch (error) {
+ console.error('Standalone startup failed:', error);
+ logger.error('Standalone startup failed:', error);
+ process.exit(1);
+ }
}As per coding guidelines "Implement error handling with try/catch in main process and console.error for debugging".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/standalone.ts` around lines 90 - 99, The startup config
initialization in async function start currently awaits
ConfigManager.initializeInstance(...) without catching errors, which can leave
the process partially started; wrap the initializeInstance call in a try/catch
inside start and on error log the error (use console.error and/or logger.error)
and exit the process with a non-zero code (e.g., process.exit(1)); ensure you
reference the ConfigManager.initializeInstance call and preserve existing
logging around logger.info('Starting standalone server...') while failing fast
when initialization throws.
|
Fixes it in Docker and the AppImage running on Linux. I don't know Typescript so I don't know if coderabbitai's comments are valid. Docker tested in Linux with... docker run --rm --name claude-devtools -p 3456:3456 -v /home/nevd/.claude:/data/.claude claude-devtoolsApp tested in Linux with... > curl -L https://github.com/nevdelap/claude-devtools/releases/download/v0.4.10/claude-devtools-0.4.10.AppImage -o /tmp/claude-devtools.AppImage
> nix-shell -p appimage-run --run "appimage-run /tmp/claude-devtools.AppImage"Both read and save the config and retain the config changes between restarts, tested by switching between light and dark modes. |
I am not a Typescript guy. This is done by Claude. I use it with Docker and that works. Cheers!
Claude says:
Description:
Settings saved to
~/.claude/claude-devtools-config.jsonwere not applied on startup — the app always launched with defaults despite the file being correctly persisted.Three bugs combined to cause this:
1. Standalone mode never loaded config from disk
standalone.tsnever calledConfigManager.initializeInstance(), soregisterConfigRoutescreated a defaultConfigManagerinstance viagetInstance()and the config file was never read. Addedawait ConfigManager.initializeInstance(configPath)at startup, usingCLAUDE_ROOTto resolve the correct path when set (e.g. Docker).2. Race condition in
ConfigManagersingleton initialisationinitializeInstance()created a new instance and setConfigManager.instanceonly afterawait initialize()completed.getInstance()(called synchronously on the next line) sawinstanceas null and created a second default instance. The two exports referred to different objects, so awaiting the promise didn't help the synchronous export. Fixed by settingConfigManager.instancesynchronously before the await, so both exports share the same object.3. Electron entry point didn't wait for config before use
app.whenReady()was synchronous and calledinitializeServices()/getConfig()while the async disk read was still in flight. Made the callbackasyncand addedawait configManagerPromisebeforeinitializeServices().Changes:
src/main/standalone.ts: initialiseConfigManagerfrom disk before starting the HTTP server; derive config path fromCLAUDE_ROOTwhen setsrc/main/services/infrastructure/ConfigManager.ts: set singleton instance synchronously beforeawait initialize()to eliminate the two-instance racesrc/main/index.ts: awaitconfigManagerPromiseinapp.whenReady()before initialising servicessrc/main/services/discovery/ProjectScanner.ts: remove references to undeclaredsessionPreviewCache(TypeScript compile error)Summary by CodeRabbit
Chores
Changes