Enhance configuration management and add user prompts for missing values#1538
Enhance configuration management and add user prompts for missing values#1538KalinduGandara wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mi-extension/src/debugger/activate.ts`:
- Around line 435-467: Wrap the synchronous reads in the configurables-loading
function, including the optional env file read and required config file read, in
a try/catch so filesystem errors do not propagate or crash debug configuration
resolution. On failure, return the established graceful fallback (an empty
configurables list), while preserving the current parsing behavior for
successfully read files.
- Around line 500-513: Update the Promise waiting for viewReady in the debug
resolution flow to include a bounded timeout, ensuring it resolves when the
state machine never transitions or the overview closes. Clean up the transition
listener on both the viewReady path and timeout path, while preserving the
immediate resolution when currentState is already viewReady.
🪄 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: fd7251b1-3837-4a9e-8f67-17e3b59b02d2
📒 Files selected for processing (1)
packages/mi-extension/src/debugger/activate.ts
| if (fs.existsSync(envPath)) { | ||
| for (const line of fs.readFileSync(envPath, 'utf-8').split('\n')) { | ||
| const trimmedLine = line.trim(); | ||
| if (!trimmedLine || trimmedLine.startsWith('#')) { | ||
| continue; | ||
| } | ||
| const separatorIndex = trimmedLine.indexOf('='); | ||
| if (separatorIndex === -1) { | ||
| continue; | ||
| } | ||
| const key = trimmedLine.substring(0, separatorIndex).trim(); | ||
| const value = trimmedLine.substring(separatorIndex + 1).trim(); | ||
| if (key && value) { | ||
| envValues.set(key, value); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const configurables: PomNodeDetails[] = []; | ||
| for (const line of fs.readFileSync(configFilePath, 'utf-8').split('\n')) { | ||
| const trimmedLine = line.trim(); | ||
| if (!trimmedLine || trimmedLine.startsWith('#')) { | ||
| continue; | ||
| } | ||
| const separatorIndex = trimmedLine.indexOf(':'); | ||
| const key = (separatorIndex === -1 ? trimmedLine : trimmedLine.substring(0, separatorIndex)).trim(); | ||
| const type = separatorIndex === -1 ? 'string' : (trimmedLine.substring(separatorIndex + 1).trim() || 'string'); | ||
| if (key) { | ||
| configurables.push({ key, type, value: envValues.get(key) ?? '' }); | ||
| } | ||
| } | ||
| return configurables; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle potential file read exceptions.
Synchronous file reads using fs.readFileSync can throw exceptions if the file is locked or if there are permission issues. An unhandled exception here will propagate and crash the debug configuration resolution. Wrap the file reading operations in a try/catch block to ensure a graceful fallback if the files are inaccessible. As per path instructions, ensure correctness and best practices.
🛡️ Proposed fix to handle file read exceptions
const envValues: Map<string, string> = new Map();
const envPath = path.join(projectUri, '.env');
if (fs.existsSync(envPath)) {
+ try {
for (const line of fs.readFileSync(envPath, 'utf-8').split('\n')) {
const trimmedLine = line.trim();
if (!trimmedLine || trimmedLine.startsWith('#')) {
continue;
}
const separatorIndex = trimmedLine.indexOf('=');
if (separatorIndex === -1) {
continue;
}
const key = trimmedLine.substring(0, separatorIndex).trim();
const value = trimmedLine.substring(separatorIndex + 1).trim();
if (key && value) {
envValues.set(key, value);
}
}
+ } catch (error) {
+ console.error(`Failed to read .env file at ${envPath}:`, error);
+ }
}
const configurables: PomNodeDetails[] = [];
+ try {
for (const line of fs.readFileSync(configFilePath, 'utf-8').split('\n')) {
const trimmedLine = line.trim();
if (!trimmedLine || trimmedLine.startsWith('#')) {
continue;
}
const separatorIndex = trimmedLine.indexOf(':');
const key = (separatorIndex === -1 ? trimmedLine : trimmedLine.substring(0, separatorIndex)).trim();
const type = separatorIndex === -1 ? 'string' : (trimmedLine.substring(separatorIndex + 1).trim() || 'string');
if (key) {
configurables.push({ key, type, value: envValues.get(key) ?? '' });
}
}
+ } catch (error) {
+ console.error(`Failed to read config.properties at ${configFilePath}:`, error);
+ }
return configurables;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fs.existsSync(envPath)) { | |
| for (const line of fs.readFileSync(envPath, 'utf-8').split('\n')) { | |
| const trimmedLine = line.trim(); | |
| if (!trimmedLine || trimmedLine.startsWith('#')) { | |
| continue; | |
| } | |
| const separatorIndex = trimmedLine.indexOf('='); | |
| if (separatorIndex === -1) { | |
| continue; | |
| } | |
| const key = trimmedLine.substring(0, separatorIndex).trim(); | |
| const value = trimmedLine.substring(separatorIndex + 1).trim(); | |
| if (key && value) { | |
| envValues.set(key, value); | |
| } | |
| } | |
| } | |
| const configurables: PomNodeDetails[] = []; | |
| for (const line of fs.readFileSync(configFilePath, 'utf-8').split('\n')) { | |
| const trimmedLine = line.trim(); | |
| if (!trimmedLine || trimmedLine.startsWith('#')) { | |
| continue; | |
| } | |
| const separatorIndex = trimmedLine.indexOf(':'); | |
| const key = (separatorIndex === -1 ? trimmedLine : trimmedLine.substring(0, separatorIndex)).trim(); | |
| const type = separatorIndex === -1 ? 'string' : (trimmedLine.substring(separatorIndex + 1).trim() || 'string'); | |
| if (key) { | |
| configurables.push({ key, type, value: envValues.get(key) ?? '' }); | |
| } | |
| } | |
| return configurables; | |
| } | |
| if (fs.existsSync(envPath)) { | |
| try { | |
| for (const line of fs.readFileSync(envPath, 'utf-8').split('\n')) { | |
| const trimmedLine = line.trim(); | |
| if (!trimmedLine || trimmedLine.startsWith('#')) { | |
| continue; | |
| } | |
| const separatorIndex = trimmedLine.indexOf('='); | |
| if (separatorIndex === -1) { | |
| continue; | |
| } | |
| const key = trimmedLine.substring(0, separatorIndex).trim(); | |
| const value = trimmedLine.substring(separatorIndex + 1).trim(); | |
| if (key && value) { | |
| envValues.set(key, value); | |
| } | |
| } | |
| } catch (error) { | |
| console.error(`Failed to read .env file at ${envPath}:`, error); | |
| } | |
| } | |
| const configurables: PomNodeDetails[] = []; | |
| try { | |
| for (const line of fs.readFileSync(configFilePath, 'utf-8').split('\n')) { | |
| const trimmedLine = line.trim(); | |
| if (!trimmedLine || trimmedLine.startsWith('#')) { | |
| continue; | |
| } | |
| const separatorIndex = trimmedLine.indexOf(':'); | |
| const key = (separatorIndex === -1 ? trimmedLine : trimmedLine.substring(0, separatorIndex)).trim(); | |
| const type = separatorIndex === -1 ? 'string' : (trimmedLine.substring(separatorIndex + 1).trim() || 'string'); | |
| if (key) { | |
| configurables.push({ key, type, value: envValues.get(key) ?? '' }); | |
| } | |
| } | |
| } catch (error) { | |
| console.error(`Failed to read config.properties at ${configFilePath}:`, error); | |
| } | |
| return configurables; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 435-435: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(envPath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 453-453: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(configFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mi-extension/src/debugger/activate.ts` around lines 435 - 467, Wrap
the synchronous reads in the configurables-loading function, including the
optional env file read and required config file read, in a try/catch so
filesystem errors do not propagate or crash debug configuration resolution. On
failure, return the established graceful fallback (an empty configurables list),
while preserving the current parsing behavior for successfully read files.
Source: Path instructions
| await new Promise<void>((resolve) => { | ||
| const stateMachine = getStateMachine(projectUri); | ||
| const currentState = stateMachine.state() as any; | ||
| if (typeof currentState === 'object' && currentState?.ready === 'viewReady') { | ||
| return resolve(); | ||
| } | ||
| const listener = (state: { value: { ready?: string; }; }) => { | ||
| if (state?.value?.ready === 'viewReady') { | ||
| stateMachine.service().off(listener); | ||
| resolve(); | ||
| } | ||
| }; | ||
| stateMachine.service().onTransition(listener); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent potential hang during debug resolution with a timeout.
The Promise waiting for the viewReady state lacks a timeout. If the state machine fails to transition (e.g., an error during view load) or if the user closes the overview before it finishes loading, the promise will hang indefinitely. Because this function is awaited in the debug configuration resolution path, an unresolved promise will block resolveDebugConfiguration and leave the debugging session in a locked, unresponsive state. As per path instructions, ensure correctness and best practices.
⏱️ Proposed fix to add a timeout
await new Promise<void>((resolve) => {
const stateMachine = getStateMachine(projectUri);
const currentState = stateMachine.state() as any;
if (typeof currentState === 'object' && currentState?.ready === 'viewReady') {
return resolve();
}
+ let timeoutId: ReturnType<typeof setTimeout>;
const listener = (state: { value: { ready?: string; }; }) => {
if (state?.value?.ready === 'viewReady') {
+ clearTimeout(timeoutId);
stateMachine.service().off(listener);
resolve();
}
};
+ timeoutId = setTimeout(() => {
+ stateMachine.service().off(listener);
+ console.warn('Timeout waiting for state machine to reach viewReady');
+ resolve();
+ }, 5000);
stateMachine.service().onTransition(listener);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise<void>((resolve) => { | |
| const stateMachine = getStateMachine(projectUri); | |
| const currentState = stateMachine.state() as any; | |
| if (typeof currentState === 'object' && currentState?.ready === 'viewReady') { | |
| return resolve(); | |
| } | |
| const listener = (state: { value: { ready?: string; }; }) => { | |
| if (state?.value?.ready === 'viewReady') { | |
| stateMachine.service().off(listener); | |
| resolve(); | |
| } | |
| }; | |
| stateMachine.service().onTransition(listener); | |
| }); | |
| await new Promise<void>((resolve) => { | |
| const stateMachine = getStateMachine(projectUri); | |
| const currentState = stateMachine.state() as any; | |
| if (typeof currentState === 'object' && currentState?.ready === 'viewReady') { | |
| return resolve(); | |
| } | |
| let timeoutId: ReturnType<typeof setTimeout>; | |
| const listener = (state: { value: { ready?: string; }; }) => { | |
| if (state?.value?.ready === 'viewReady') { | |
| clearTimeout(timeoutId); | |
| stateMachine.service().off(listener); | |
| resolve(); | |
| } | |
| }; | |
| timeoutId = setTimeout(() => { | |
| stateMachine.service().off(listener); | |
| console.warn('Timeout waiting for state machine to reach viewReady'); | |
| resolve(); | |
| }, 5000); | |
| stateMachine.service().onTransition(listener); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mi-extension/src/debugger/activate.ts` around lines 500 - 513,
Update the Promise waiting for viewReady in the debug resolution flow to include
a bounded timeout, ensuring it resolves when the state machine never transitions
or the overview closes. Clean up the transition listener on both the viewReady
path and timeout path, while preserving the immediate resolution when
currentState is already viewReady.
Source: Path instructions
| context.subscriptions.push(vscode.debug.registerDebugAdapterDescriptorFactory('mi', factory)); | ||
| } | ||
|
|
||
| function getConfigurablesWithValues(projectUri: string): PomNodeDetails[] { |
There was a problem hiding this comment.
There should be a LS method that we could use for this. Please check
| return false; | ||
| } | ||
|
|
||
| async function openManageConfigurables(projectUri: string, configs: PomNodeDetails[]) { |
There was a problem hiding this comment.
Without opening the same configuration popup page shall we open a dialog box to get the values only? Refer to the Remote Deployment feature.
2 participants
This pull request introduces a pre-launch check for missing configurable values in
.envfiles for each project before starting a debug session. If any required configurables are missing, the user is prompted to either proceed or add the missing values using a popup visual editor. The main changes add utility functions to parse and validate configurables, integrate the check into the debug configuration provider, and handle the popup UI flow.Configurable value validation and UI integration:
MiConfigurationProvider.resolveDebugConfigurationto check for missing configurable values in.envfiles for each project and prompt the user to add them before launching the debugger.getConfigurablesWithValuesto parse required configurables fromconfig.propertiesand match them with values from.env.confirmConfigurableValuesto prompt the user if any configurables are missing, offering the option to open the Manage Configurables popup or proceed.openManageConfigurablesto open the appropriate visual editor for managing configurables, ensuring the webview is ready before opening the popup.Imports and dependencies: