fix: Support VS Code profiles for prompts sync#15
Conversation
- Detect active profile from storage.json (primary method) - Add fallback detection via process args and file system - Support default and named profiles with backward compatibility - Add debug logging for profile detection Fixes prompts syncing to wrong directory when using VS Code profiles.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds profile-aware prompts directory resolution in ConfigManager, introduces a constructor accepting the VS Code extension context and logger usage, updates activation to pass that context, and adjusts CHANGELOG vNext header and a VS Code Profile Support entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant VS as "VS Code"
participant Ext as "Extension"
participant CM as "ConfigManager"
participant FS as "File System"
User->>VS: launch
VS->>Ext: activate(context)
Ext->>CM: new ConfigManager(context)
note right of CM: stores extensionContext + logger
Ext->>CM: getPromptsDirectory()
rect rgb(245,250,255)
note over CM,FS: Profile-aware resolution (hierarchical fallbacks)
CM->>FS: check custom path (if set) — log and return
alt no custom path
CM->>FS: read storage.json / lastKnownMenubarData
alt storage data identifies profile
CM->>CM: build profile-specific prompts path
else
CM->>Ext: consult argv/env vars for profile
alt env/argv identifies profile
CM->>CM: build profile-specific prompts path
else
CM->>CM: use extensionContext.storageUri or globalStorageUri
alt usable storage path
CM->>CM: derive per-user prompts path
else
CM->>FS: fallback to hardcoded prompts directory
end
end
end
end
end
CM-->>Ext: return resolved prompts directory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 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.
Pull Request Overview
This PR fixes VS Code profile support for prompts synchronization by implementing multiple profile detection methods. The main issue was that the extension wasn't correctly detecting and using profile-specific directories when VS Code profiles were active.
- Added comprehensive profile detection logic with primary method reading VS Code's storage.json file
- Implemented fallback detection via process arguments and file system scanning
- Added extensive debug logging to trace profile detection attempts and results
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/extension.ts | Sets extension context on ConfigManager to enable profile-aware functionality |
| src/configManager.ts | Implements multi-method profile detection with storage.json parsing, process args scanning, and file system fallbacks |
| CHANGELOG.md | Documents the profile support fix and detection methods added |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
CHANGELOG.md (1)
9-17: Polish wording and consistency for clarityTighten phrasing, fix minor grammar, and use consistent terms (filesystem, arguments). No behavior change.
-**VS Code Profile Support**: Fixed issue where extension didn't work properly with multiple VS Code profiles - - **Primary Method**: Reads VS Code's `storage.json` file to detect the active profile (most reliable) - - Parses `lastKnownMenubarData.menus.Preferences.items` to find checked profile - - Extracts profile ID and constructs correct profile-specific path - - **Fallback Methods**: Process arguments scanning and file system detection - - Comprehensive logging shows all detection attempts and results - - Maintains backward compatibility with default profile support - - Works around VS Code Extension API limitation where `storageUri` and `globalStorageUri` are not profile-aware +**VS Code Profile Support**: Fixes prompts syncing to the wrong directory when using multiple profiles + - **Primary method**: Reads VS Code’s `storage.json` to detect the active profile (most reliable) + - Parses `lastKnownMenubarData.menus.Preferences.items` to find the checked profile + - Extracts the profile ID and constructs the correct profile‑specific path + - **Fallback methods**: Process‑argument scanning and filesystem detection + - Comprehensive logging shows all detection attempts and results + - Backward compatible with the default profile + - Works around the VS Code API limitation where `storageUri`/`globalStorageUri` are not profile‑aware -- Enhanced logging with detailed profile detection information showing each detection step +- Enhanced logging with detailed profile‑detection information that shows each detection step - Added storage.json parsing as primary profile detection method - Added multiple fallback detection methods for robustness -- Verifies profile directory existence before using detected path +- Verifies the profile directory exists before using the detected pathAlso applies to: 20-23
src/configManager.ts (3)
191-218: Deduplicate storage.json path constructionBoth storageJsonExists and detectProfileFromStorageJson build the same path. Extract a private helper (computeStorageJsonPath) to avoid drift.
Also applies to: 233-248
95-110: Route logs through Logger and/or gate by configUse a shared Logger (as in extension.ts) or guard console logs behind promptitude.debug to reduce noise.
Example:
// add near class fields // private logger = Logger.get('ConfigManager'); // then replace console.log(...) with: // if (this.debug) this.logger.debug('message');This keeps verbose profile‑detection traces only when needed.
Also applies to: 120-156, 161-179, 521-548
233-248: Derive VS Code user directory from extensionContext to support Insiders/OSS/VSCodium
storage.json, getBaseUserPath and promptsPath all hard-code 'Code' (src/configManager.ts) and omit 'Code – Insiders', 'Code – OSS', 'VSCodium', etc.
• When extensionContext is available, extract the base User path from extensionContext.globalStorageUri.fsPath (e.g. strip/globalStorageor/prompts)
• Fall back to current platform mappings only if extensionContext isn’t provided
• Apply this change in detectProfileFromStorageJson, getBaseUserPath and promptsPath logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/configManager.ts(2 hunks)src/extension.ts(6 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/configManager.ts
[error] 193-193: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 227-227: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 342-343: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 501-501: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 526-526: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...eferences.items` to find checked profile - Extracts profile ID and constructs corre...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...constructs correct profile-specific path - Fallback Methods: Process arguments sc...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...ments scanning and file system detection - Comprehensive logging shows all detectio...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...shows all detection attempts and results - Maintains backward compatibility with de...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...mpatibility with default profile support - Works around VS Code Extension API limit...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...ction step - Added storage.json parsing as primary profile detection method - Adde...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...fies profile directory existence before using detected path ## [1.4.0] - 2025-10-03 ...
(QB_NEW_EN)
🔇 Additional comments (1)
src/extension.ts (1)
31-31: Good wiring: pass ExtensionContext into ConfigManagerThis enables profile‑aware path resolution early in activation. LGTM.
- Add getNodeRequire() helper method to safely access Node.js require
- Check globalThis.require first, then fall back to global require
- Replace all eval('require') calls with safe helper method
- Addresses security concern from PR review
This removes the security risk of using eval() while maintaining
compatibility with the VS Code extension runtime environment.
Safwane-GJ
force-pushed
the
fix-vscode-profile-prompts-sync
branch
from
63eea32 to
3a44b10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/configManager.ts (5)
99-114: Use Logger instead of console.log for consistency.The extension already has a Logger utility that should be used for diagnostic messages instead of console.log.
Based on the Logger implementation in
src/utils/logger.ts, replace console.log calls with Logger methods:getPromptsDirectory(): string { if (this.customPath) { - console.log(`[Promptitude] Using custom prompts path: ${this.customPath}`); + Logger.debug(`Using custom prompts path: ${this.customPath}`); return this.customPath; } // Use profile-aware path if extension context is available if (this.extensionContext) { - console.log('[Promptitude] Extension context available, using profile-aware path'); + Logger.debug('Extension context available, using profile-aware path'); return this.getProfileAwarePromptsDirectory(); } // Fallback to hardcoded paths for backward compatibility - console.log('[Promptitude] No extension context available, using fallback path'); + Logger.debug('No extension context available, using fallback path'); return this.getFallbackPromptsDirectory(); }
116-170: Replace console.log with Logger for consistency.Throughout this method, console.log is used for diagnostic messages. Use the Logger utility instead for consistency with the rest of the extension.
Apply this pattern throughout the method:
- console.log('[Promptitude] Attempting profile-aware path detection...'); + Logger.debug('Attempting profile-aware path detection...');if (this.debug) { - console.log(`[Promptitude] ✅ Profile-specific path detected: ${detectedProfilePath}`); + Logger.debug(`✅ Profile-specific path detected: ${detectedProfilePath}`); }Continue this pattern for all console.log statements in this method (lines 124, 131, 136, 142, 147, 155, 162, 168).
228-349: Use Logger and gate all debug output consistently.This method has extensive diagnostic logging. Apply these improvements:
- Replace all console.log with Logger.debug
- Gate detailed debug output consistently behind
this.debugExample transformations:
- console.log(`[Promptitude] Checking storage.json at: ${storageJsonPath}`); + if (this.debug) { + Logger.debug(`Checking storage.json at: ${storageJsonPath}`); + }- console.log(`[Promptitude] Successfully parsed storage.json`); + Logger.debug('Successfully parsed storage.json');Apply this pattern throughout the method. Keep informational messages (like profile detection success on lines 267, 311) ungated, but gate verbose diagnostics behind
this.debug.
351-512: Good debug gating for sensitive data, but use Logger throughout.Positive: Lines 382-400 now correctly gate potentially sensitive process.argv and env logging behind the debug flag, addressing previous security concerns.
However, the method still uses console.log extensively instead of the Logger utility.
Apply Logger consistently:
- console.log(`[Promptitude] Attempting profile detection from VS Code storage...`); + Logger.debug('Attempting profile detection from VS Code storage...');- console.log(`[Promptitude] ✅ Named profile detected from storage.json: ${storageJsonResult}`); + Logger.debug(`✅ Named profile detected from storage.json: ${storageJsonResult}`);Continue for all console.log statements in this method.
530-562: Good path construction, but use Logger.The method correctly uses path.join for cross-platform compatibility (addressing previous comments), but should use Logger instead of console.log.
- console.log('[Promptitude] Using fallback hardcoded prompts directory paths'); + Logger.debug('Using fallback hardcoded prompts directory paths');if (this.debug) { - console.log(`[Promptitude] Fallback prompts directory: ${promptsPath}`); + Logger.debug(`Fallback prompts directory: ${promptsPath}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configManager.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (3)
src/configManager.ts (3)
26-33: LGTM!Clean API design for receiving the extension context from the activation flow.
195-200: LGTM!Correctly handles both Windows and Unix path separators when checking for profile-specific segments.
514-528: LGTM!Platform-specific path construction correctly uses path.join for cross-platform compatibility.
- Prevent logging of full process.argv (may contain sensitive paths/tokens) - Prevent logging of environment variables without debug mode - Gate full file system paths behind debug flag - Only log detailed profile paths when debug mode is enabled - Keep essential logs (platform, profile found/not found) always visible Sensitive information like user home directories, profile IDs, and process arguments are now only logged when promptitude.debug is true.
- Import os, path, fs, process at module level instead of dynamic require - Remove getNodeRequire() helper method (no longer needed) - Cleaner, more performant, and better for bundling - Reduces bundle size from 59.03 KB to 58.64 KB - Addresses PR review feedback This is the recommended approach for VS Code extensions running in Node.js environment where these modules are always available.
- Replace string concatenation with '/prompts' with path.join()
- Fixes potential mixed separator issues on Windows
- Ensures proper path separators on all platforms (\, /)
- Applied to all 3 return statements in getProfileAwarePromptsDirectory()
String concatenation like `${path}/prompts` can produce invalid paths
on Windows. path.join() handles platform-specific separators correctly.
Safwane-GJ
force-pushed
the
fix-vscode-profile-prompts-sync
branch
from
3a44b10 to
71e6b42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/configManager.ts (5)
99-114: Replace console.log with Logger for consistency.The method uses
console.logfor diagnostic messages. For consistency with the codebase and to respect user preferences, use theLoggerutility instead.Apply this diff:
getPromptsDirectory(): string { if (this.customPath) { - console.log(`[Promptitude] Using custom prompts path: ${this.customPath}`); + Logger.debug(`Using custom prompts path: ${this.customPath}`); return this.customPath; } // Use profile-aware path if extension context is available if (this.extensionContext) { - console.log('[Promptitude] Extension context available, using profile-aware path'); + Logger.debug('Extension context available, using profile-aware path'); return this.getProfileAwarePromptsDirectory(); } // Fallback to hardcoded paths for backward compatibility - console.log('[Promptitude] No extension context available, using fallback path'); + Logger.debug('No extension context available, using fallback path'); return this.getFallbackPromptsDirectory(); }Import Logger at the top if not already present:
import { Logger } from './utils/logger';
116-170: Guard informational logs with debug flag.Lines 124, 136, and 168 use
console.logwithout checking the debug flag, while other diagnostic messages are properly guarded. For consistency and to avoid cluttering the console for users who haven't enabled debug mode, guard all diagnostic messages with the debug flag.Apply this diff to guard the informational logs:
private getProfileAwarePromptsDirectory(): string { if (!this.extensionContext) { throw new Error('Extension context not available'); } - console.log('[Promptitude] Attempting profile-aware path detection...'); + if (this.debug) { + console.log('[Promptitude] Attempting profile-aware path detection...'); + } // Method 1: Detect profile from storage.json, process args, or file system (MOST RELIABLE) // This method includes storage.json parsing as primary, with fallbacks const detectedProfilePath = this.detectProfileFromEnvironment(); if (detectedProfilePath) { if (this.debug) { console.log(`[Promptitude] ✅ Profile-specific path detected: ${detectedProfilePath}`); } return path.join(detectedProfilePath, 'prompts'); } - console.log('[Promptitude] No profile detected, checking Extension API paths...'); + if (this.debug) { + console.log('[Promptitude] No profile detected, checking Extension API paths...'); + } // ... rest of method // Final fallback to hardcoded paths (should rarely be needed) - console.log('[Promptitude] ⚠️ All detection methods failed, using hardcoded fallback paths'); + if (this.debug) { + console.log('[Promptitude] ⚠️ All detection methods failed, using hardcoded fallback paths'); + } return this.getFallbackPromptsDirectory(); }Alternatively, consider migrating all console.log calls to Logger.debug for better consistency with the codebase.
228-349: Replace console.log with Logger throughout the method.The method uses console.log extensively for diagnostic messages. For consistency with the codebase and to provide users with better control over logging verbosity, replace all console.log calls with appropriate Logger methods (e.g., Logger.debug, Logger.error).
Key locations to update:
- Lines 248, 253, 258, 267, 271, 276, 282-283, 287, 296, 311: Use
Logger.debug- Lines 305-307, 321-323, 326-328, 334-336: Already guarded by debug, convert to
Logger.debug- Lines 343-345: Error logging should use
Logger.errorExample for error handling:
} catch (error) { - console.log(`[Promptitude] Error reading storage.json: ${error}`); - if (this.debug && error instanceof Error && error.stack) { - console.log(`[Promptitude] Error stack: ${error.stack}`); - } + Logger.error(`Error reading storage.json`, error as Error); return null; }Import Logger if not already present:
import { Logger } from './utils/logger';
351-512: Use Logger and consider breaking down method complexity.The method contains extensive diagnostic logging using console.log. Replace these with Logger calls for consistency. Additionally, consider extracting some of the detection strategies into separate helper methods to reduce complexity.
For logging consistency:
Replace console.log calls with Logger methods:
- Lines 356-357, 369-370, 373-374, 378, 414, 430, 438, 455, 460, 466, 493, 503: Use
Logger.debug- Lines 505-508: Use
Logger.errorfor error loggingExample for error handling:
} catch (error) { - console.log(`[Promptitude] Error during profile detection: ${error}`); - if (error instanceof Error && error.stack) { - console.log(`[Promptitude] Error stack: ${error.stack}`); - } + Logger.error(`Error during profile detection`, error as Error); }Optional refactor for complexity:
The method is quite long. Consider extracting these detection strategies into separate methods:
detectProfileFromProcessArgs()(lines 403-435)detectProfileFromFileSystem()(lines 438-496)This would improve readability and testability.
530-562: Guard or migrate log statement on line 534.Line 534 uses console.log without checking the debug flag. For consistency with lines 553-555 which are properly guarded, either guard this log or migrate to Logger.debug.
Apply this diff:
private getFallbackPromptsDirectory(): string { - console.log('[Promptitude] Using fallback hardcoded prompts directory paths'); + if (this.debug) { + console.log('[Promptitude] Using fallback hardcoded prompts directory paths'); + } try { let promptsPath: string;Or migrate to Logger:
private getFallbackPromptsDirectory(): string { - console.log('[Promptitude] Using fallback hardcoded prompts directory paths'); + Logger.debug('Using fallback hardcoded prompts directory paths'); try { let promptsPath: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configManager.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (3)
src/configManager.ts (3)
195-200: LGTM!The path profile detection correctly checks for both forward and backward slashes, ensuring cross-platform compatibility.
202-226: LGTM!Proper platform-specific path construction with appropriate error handling.
514-528: LGTM!Platform-specific base path construction is correct and uses path.join appropriately.
- Replace setExtensionContext() setter with constructor parameter - Make extensionContext required (not optional) in ConfigManager - Simplify getPromptsDirectory() by removing fallback path logic - Update extension.ts to pass context during instantiation
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Add Logger import and create logger instance - Replace all console.log statements with appropriate logger methods - Use logger.info for informational messages - Use logger.debug for debug-level messages - Use logger.warn for warnings - Use logger.error for errors
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/configManager.ts (2)
8-8: Remove redundant process import.The
processobject is globally available in Node.js and does not require an explicit import. This was flagged in a previous review.Apply this diff:
-import * as process from 'process';
160-181: Fix hardcoded path separator (previous review issue).Line 180 still uses hardcoded
'/'separator instead ofpath.join, which was flagged in a previous review. This can cause path inconsistencies on Windows.Apply this diff:
private extractUserDirectoryFromStoragePath(storagePath: string, storageType: string): string | null { const pathSegments = storagePath.split(/[/\\]/); const userIndex = pathSegments.findIndex((segment: string) => segment === 'User'); if (userIndex === -1) { this.logger.debug(`Could not find 'User' directory in ${storageType} path: ${pathSegments.join(', ')}`); return null; } const storageIndex = pathSegments.findIndex((segment: string) => segment === storageType); if (storageIndex === -1) { this.logger.debug(`Could not find '${storageType}' directory in path: ${pathSegments.join(', ')}`); return null; } // Take all segments up to (but not including) the storage directory const userDirectorySegments = pathSegments.slice(0, storageIndex); - return userDirectorySegments.join('/'); + return path.join(...userDirectorySegments); }
🧹 Nitpick comments (1)
src/configManager.ts (1)
183-188: Consider path-agnostic profile detection.The method uses hardcoded path separators. While functional, it could be more robust by normalizing the path first.
Apply this diff for better cross-platform handling:
private hasProfileInPath(path: string): boolean { - return path.includes('/profiles/') || path.includes('\\profiles\\'); + const normalized = path.replace(/\\/g, '/'); + return normalized.includes('/profiles/'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configManager.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/configManager.ts (1)
src/utils/logger.ts (2)
Logger(3-88)error(68-77)
🔇 Additional comments (8)
src/configManager.ts (8)
27-32: LGTM! Constructor properly initialized.The constructor now requires
vscode.ExtensionContextand initializes it, which aligns with the previous review feedback and enables profile-aware path detection.
98-106: LGTM! Clean delegation pattern.The method properly uses the Logger and delegates to profile-aware detection when no custom path is configured.
108-158: LGTM! Robust fallback hierarchy.The method implements a well-thought-out detection strategy with multiple fallbacks, proper logging, and correct cross-platform path handling using
path.join.
190-214: LGTM! Solid platform detection.The method correctly constructs platform-specific paths using
path.joinand handles errors gracefully with a safe default.
216-337: LGTM! Comprehensive profile detection.This method implements the primary detection strategy by parsing VS Code's
storage.json. The logic is well-structured with:
- Proper error handling
- Appropriate debug logging
- Graceful handling of default profile (returns null)
- Smart fallback to label-based lookup when ID-based path doesn't exist
339-500: LGTM! Well-implemented fallback detection.The method provides multiple fallback strategies:
- Primary: storage.json parsing
- Fallback 1: Process arguments (
--user-data-dir,--profile)- Fallback 2: Filesystem scanning with heuristics
The debug logging is properly gated behind
this.debugflag (lines 370, 380), addressing previous security concerns about logging potentially sensitive paths.
502-516: LGTM! Clean platform-specific paths.The method correctly constructs base user paths for all major platforms using
path.join.
518-550: LGTM! Appropriate final fallback.The method provides a reasonable fallback with proper warning logging. The error thrown at line 548 is appropriate for the extremely rare case where Node.js environment is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/configManager.ts (1)
159-180: Fix hardcoded path separator (regression from previous fix).Line 179 still uses hardcoded
'/'separator instead ofpath.join, which was flagged and supposedly fixed in previous reviews. This causes path inconsistencies on Windows.Apply this diff:
// Take all segments up to (but not including) the storage directory const userDirectorySegments = pathSegments.slice(0, storageIndex); - return userDirectorySegments.join('/'); + return path.join(...userDirectorySegments);
🧹 Nitpick comments (2)
src/configManager.ts (2)
4-7: Consider using 'node:' prefix for built-in imports.Modern TypeScript/Node.js practice recommends using the
node:prefix for built-in modules to clearly distinguish them from third-party packages.Apply this diff:
import { Logger } from './utils/logger'; -import * as os from 'os'; -import * as path from 'path'; -import * as fs from 'fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import * as fs from 'node:fs';
182-187: Consider using path-based check instead of string matching.The current approach works but could be more robust by normalizing the path first.
Apply this diff:
private hasProfileInPath(path: string): boolean { - return path.includes('/profiles/') || path.includes('\\profiles\\'); + const normalized = path.split(/[/\\]/).join(path.sep); + return normalized.includes(`${path.sep}profiles${path.sep}`); }Alternatively, keep the current defensive approach if cross-platform path formats in strings are expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configManager.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/configManager.ts (1)
src/utils/logger.ts (2)
Logger(3-88)error(68-77)
🔇 Additional comments (8)
src/configManager.ts (8)
26-31: LGTM!The constructor properly accepts
ExtensionContextand initializes the logger with appropriate scope. This addresses the previous feedback about simplifying code by requiring context at construction time.
97-105: LGTM!Clean delegation to profile-aware detection with appropriate logging. The custom path takes precedence as expected.
107-157: LGTM!Solid hierarchical detection strategy with appropriate fallbacks. The method correctly uses
path.joinfor all path construction and includes helpful debug logging.
189-213: LGTM!Platform-specific path resolution is correct and uses
path.joinproperly. Error handling ensures graceful degradation.
215-336: LGTM!Comprehensive storage.json parsing with proper error handling. The fallback to profile label (lines 318-325) when ID-based directory doesn't exist is a smart defensive measure.
338-499: LGTM!Excellent multi-layered detection strategy. The debug logging for
process.argvand environment variables (lines 369-387) is now properly gated behind the debug flag, addressing previous security concerns. The filesystem heuristics for single-profile and most-recent-profile detection provide useful fallbacks.
501-515: LGTM!Platform-specific base paths are correctly constructed using
path.join. The default case provides a reasonable fallback for unknown platforms.
517-549: LGTM!Fallback implementation correctly uses
path.joinfor all platform-specific paths. The warning log and final error provide clear diagnostic information.
…-platform path joining in ConfigManager
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/configManager.ts (7)
195-207: De‑duplicate storage.json path construction and rely on getBaseUserPath()Build the path from the same base used elsewhere; reduces drift and improves maintainability.
- let storageJsonPath: string; - switch (process.platform) { - case 'win32': - storageJsonPath = path.join(os.homedir(), 'AppData', 'Roaming', 'Code', 'User', 'globalStorage', 'storage.json'); - break; - case 'darwin': - storageJsonPath = path.join(os.homedir(), 'Library', 'Application Support', 'Code', 'User', 'globalStorage', 'storage.json'); - break; - case 'linux': - storageJsonPath = path.join(os.homedir(), '.config', 'Code', 'User', 'globalStorage', 'storage.json'); - break; - default: - return false; - } + const storageJsonPath = path.join(this.getBaseUserPath(), 'globalStorage', 'storage.json');- let storageJsonPath: string; - switch (process.platform) { - case 'win32': - storageJsonPath = path.join(os.homedir(), 'AppData', 'Roaming', 'Code', 'User', 'globalStorage', 'storage.json'); - break; - case 'darwin': - storageJsonPath = path.join(os.homedir(), 'Library', 'Application Support', 'Code', 'User', 'globalStorage', 'storage.json'); - break; - case 'linux': - storageJsonPath = path.join(os.homedir(), '.config', 'Code', 'User', 'globalStorage', 'storage.json'); - break; - default: - this.logger.debug(`Unsupported platform for storage.json detection: ${process.platform}`); - return null; - } + const storageJsonPath = path.join(this.getBaseUserPath(), 'globalStorage', 'storage.json');Also applies to: 224-237
504-515: Support Insiders/VSCodium/OSS by deriving the product dir from vscode.env.appNameHard‑coding “Code” misses other distributions and portable builds.
+ private getProductDirName(): string { + const app = (vscode.env?.appName ?? '').toLowerCase(); + if (app.includes('insiders')) return 'Code - Insiders'; + if (app.includes('vscodium')) return 'VSCodium'; + if (app.includes('oss')) return 'Code - OSS'; + return 'Code'; + } + private getBaseUserPath(): string { - switch (process.platform) { + const product = this.getProductDirName(); + switch (process.platform) { case 'win32': - return path.join(os.homedir(), 'AppData', 'Roaming', 'Code', 'User'); + return path.join(os.homedir(), 'AppData', 'Roaming', product, 'User'); case 'darwin': - return path.join(os.homedir(), 'Library', 'Application Support', 'Code', 'User'); + return path.join(os.homedir(), 'Library', 'Application Support', product, 'User'); case 'linux': - return path.join(os.homedir(), '.config', 'Code', 'User'); + return path.join(os.homedir(), '.config', product, 'User'); default: - return path.join(os.homedir(), '.vscode', 'User'); + return path.join(os.homedir(), '.vscode', 'User'); } }Please validate on:
- VS Code Stable and Insiders
- VSCodium (if supported)
- Default profile and at least one named profile
185-187: Rename param to avoid shadowing and use a path‑separator aware checkThe parameter name shadows the imported path module; a regex over separators is more robust than hardcoded substrings.
- private hasProfileInPath(path: string): boolean { - return path.includes('/profiles/') || path.includes('\\profiles\\'); - } + private hasProfileInPath(targetPath: string): boolean { + return /[\\/](profiles)[\\/]/.test(targetPath); + }
369-387: Optional: sanitize debug logging of argv/envEven in debug, prefer redacting home paths and obvious secrets to reduce accidental leakage in logs.
// Example helper: const redact = (s: string) => s.replace(os.homedir(), '~').replace(/(?<=token=)[^&]+/gi, '***');Use redact(arg) and redact(process.env[envVar]!) before logging.
518-523: Docstring nit: context is now requiredComment mentions “when context is not available” but the constructor requires it. Update for accuracy.
- /** - * Fallback to hardcoded paths (for backward compatibility when context is not available) - */ + /** + * Fallback to hardcoded paths (used only if all detections fail) + */
5-7: Optional: use node: specifiersUsing node: URLs is the current Node.js recommendation; no behavior change.
-import * as os from 'os'; -import * as path from 'path'; -import * as fs from 'fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import * as fs from 'node:fs';
220-336: Synchronous fs is acceptable here; consider async laterOne‑shot detection on activation is fine; if this ever runs on hot paths, migrate to fs.promises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configManager.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/configManager.ts (1)
src/utils/logger.ts (2)
Logger(3-88)error(68-77)
🔇 Additional comments (2)
src/configManager.ts (2)
107-157: Good layering and fallbacksThe staged detection (storage.json → storageUri → globalStorageUri → hardcoded) with path.join and Logger is solid.
Consider adding a brief unit/integration probe in activation that logs the chosen path once per session in debug to aid field troubleshooting.
239-247: Code is properly guarded; no changes needed.The implementation correctly handles the internal
lastKnownMenubarDatastructure with comprehensive error handling. The function wraps file I/O and JSON parsing in a try-catch block (lines 328–333), uses optional chaining for nested property access (line 259), and returns null as a safe fallback on any failure. VS Code >=1.90 does persist User/globalStorage/storage.json with lastKnownMenubarData.menus.Preferences.items and profile data, confirming the structure being accessed exists, though undocumented. The guards and fallbacks are already active as intended.
| getPromptsDirectory(): string { | ||
| if (this.customPath) { | ||
| this.logger.info(`Using custom prompts path: ${this.customPath}`); | ||
| return this.customPath; | ||
| } | ||
|
|
||
| // Get VS Code user data directory | ||
| const os = require('os'); | ||
| const path = require('path'); | ||
|
|
||
| this.logger.info('Using profile-aware path detection'); | ||
| return this.getProfileAwarePromptsDirectory(); | ||
| } |
There was a problem hiding this comment.
Validate customPath and fall back if invalid
Avoid returning a bad path; check existence and directory type before using it.
- if (this.customPath) {
- this.logger.info(`Using custom prompts path: ${this.customPath}`);
- return this.customPath;
- }
+ if (this.customPath) {
+ const resolved = path.resolve(this.customPath);
+ try {
+ const stat = fs.existsSync(resolved) ? fs.statSync(resolved) : undefined;
+ if (!stat || !stat.isDirectory()) {
+ this.logger.warn(`Custom prompts path is invalid or not a directory: ${resolved}. Falling back to profile-aware detection.`);
+ } else {
+ this.logger.info(`Using custom prompts path: ${resolved}`);
+ return resolved;
+ }
+ } catch {
+ this.logger.warn(`Custom prompts path could not be accessed: ${resolved}. Falling back to profile-aware detection.`);
+ }
+ }🤖 Prompt for AI Agents
In src/configManager.ts around lines 97 to 105, the method returns
this.customPath without validating it; change it to verify the path exists and
is a directory before using it: if this.customPath is set, perform a try/catch
around fs.existsSync (or fs.statSync) and confirm
fs.statSync(this.customPath).isDirectory() (or similar async variant) — if
valid, log the info and return it; if not, log a warning/error and fall back to
the profile-aware path by calling this.getProfileAwarePromptsDirectory(); ensure
any fs errors are caught and handled so the method always returns a valid
directory string.
| if (storageJsonExists) { | ||
| // storage.json exists and was parsed successfully | ||
| if (storageJsonResult) { | ||
| // Named profile is active | ||
| this.logger.info(`✅ Named profile detected from storage.json: ${storageJsonResult}`); | ||
| return storageJsonResult; | ||
| } else { | ||
| // Default profile is active (null is intentional) | ||
| this.logger.info('✅ Default profile is active (from storage.json)'); | ||
| return null; // This will use the default User/prompts path | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Don’t treat “storage.json exists but parse/shape failed” as “default profile”; continue fallbacks
Current flow returns null early, skipping argv/filesystem detection whenever storage.json exists but isn’t parseable or the shape changed. That can misroute prompts.
- if (storageJsonExists) {
- // storage.json exists and was parsed successfully
- if (storageJsonResult) {
- // Named profile is active
- this.logger.info(`✅ Named profile detected from storage.json: ${storageJsonResult}`);
- return storageJsonResult;
- } else {
- // Default profile is active (null is intentional)
- this.logger.info('✅ Default profile is active (from storage.json)');
- return null; // This will use the default User/prompts path
- }
- } else {
+ if (storageJsonExists) {
+ if (storageJsonResult) {
+ this.logger.info(`✅ Named profile detected from storage.json: ${storageJsonResult}`);
+ return storageJsonResult;
+ }
+ // Do not assume default; proceed with argv/fs fallbacks.
+ this.logger.debug('storage.json did not yield a named profile; continuing with fallback detection...');
+ } else {
// storage.json doesn't exist or failed to parse, try fallback methods
this.logger.debug('storage.json not available, trying alternative methods...');
}🤖 Prompt for AI Agents
In src/configManager.ts around lines 352 to 363, the code currently treats any
time storage.json exists but storageJsonResult is falsy as the “default profile”
and returns null early; instead only return null when storage.json was
successfully parsed and explicitly indicates the default profile—if storage.json
exists but failed to parse or failed shape validation, do not return early, log
a warning/error about the parse/shape failure, and allow the function to
continue to argv/filesystem fallback detection; change the condition to
differentiate "parsed successfully and value === null" from "exists but
parse/shape failed", remove the early return for the latter, and add a clear
logger.warn/logger.error before falling through to other lookup steps.
Fixes prompts syncing to wrong directory when using VS Code profiles.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation