fix: line breaks for adding new environment variables#604
fix: line breaks for adding new environment variables#604CSchulz wants to merge 1 commit intositeboon:mainfrom
Conversation
📝 WalkthroughWalkthroughThe environment variable parsing logic in the MCP form modal has been enhanced to detect trailing newlines in textarea input and handle duplicate key entries by appending underscores to prevent overwrites. The updated parsing flow maintains the existing pattern of calling Changes
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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
🤖 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/components/settings/view/modals/ClaudeMcpFormModal.tsx`:
- Line 419: The current isNewLine computation (const isNewLine =
event.target.value.lastIndexOf('\n') === event.target.value.length - 1;) treats
an empty string as ending with a newline; update the check to guard against
empty values by verifying the value has length > 0 before comparing indices
(e.g., use value.length > 0 && value.lastIndexOf('\n') === value.length - 1 or
value.length > 0 && value.endsWith('\n')). Apply this change where the input
change handler computes isNewLine (the line defining isNewLine in
ClaudeMcpFormModal.tsx) so clearing the textarea no longer creates an empty
entry.
- Around line 431-433: The code in ClaudeMcpFormModal.tsx is inserting an empty
key into the env object when isNewLine is true (env[''] = ''), which creates
malformed CLI args; change the logic in the parsing/updating path (where env is
constructed) to skip any entries with empty keys (filter out key === '' or
trim() === '') before saving/updating the config or before submission (or
alternatively keep the raw textarea string in a separate state for display and
only parse/submit valid key/value pairs), and ensure any helper function that
builds the env object (the code block handling isNewLine and env) never writes
an empty-string key.
- Around line 423-427: The code that builds unique environment keys currently
appends a single underscore to trimmedKey when env[trimmedKey] exists, but may
still collide; update the logic in ClaudeMcpFormModal.tsx where trimmedKey is
computed so that you loop until the key is unique (e.g., let trimmedKey =
key.trim(); while (env[trimmedKey]) { trimmedKey += '_'; }) ensuring you check
env for existence each iteration before assigning to env[trimmedKey] to avoid
overwriting existing entries.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6f27d012-db40-4e99-927d-dce53da2d686
📒 Files selected for processing (1)
src/components/settings/view/modals/ClaudeMcpFormModal.tsx
| value={Object.entries(formData.config.env).map(([key, value]) => `${key}=${value}`).join('\n')} | ||
| onChange={(event) => { | ||
| const env: Record<string, string> = {}; | ||
| const isNewLine = event.target.value.lastIndexOf('\n') === event.target.value.length - 1; |
There was a problem hiding this comment.
Edge case: empty textarea incorrectly detected as ending with newline.
When event.target.value is an empty string (""), lastIndexOf('\n') returns -1 and length - 1 also equals -1, so the condition evaluates to true. This would incorrectly add an empty entry when the field is cleared.
Proposed fix: add length guard
- const isNewLine = event.target.value.lastIndexOf('\n') === event.target.value.length - 1;
+ const value = event.target.value;
+ const isNewLine = value.length > 0 && value.endsWith('\n');📝 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.
| const isNewLine = event.target.value.lastIndexOf('\n') === event.target.value.length - 1; | |
| const value = event.target.value; | |
| const isNewLine = value.length > 0 && value.endsWith('\n'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/view/modals/ClaudeMcpFormModal.tsx` at line 419, The
current isNewLine computation (const isNewLine =
event.target.value.lastIndexOf('\n') === event.target.value.length - 1;) treats
an empty string as ending with a newline; update the check to guard against
empty values by verifying the value has length > 0 before comparing indices
(e.g., use value.length > 0 && value.lastIndexOf('\n') === value.length - 1 or
value.length > 0 && value.endsWith('\n')). Apply this change where the input
change handler computes isNewLine (the line defining isNewLine in
ClaudeMcpFormModal.tsx) so clearing the textarea no longer creates an empty
entry.
| let trimmedKey = key.trim(); | ||
| if (env[trimmedKey]) { | ||
| // if key already exists, add an underscore to prevent data loss | ||
| trimmedKey += '_' | ||
| } |
There was a problem hiding this comment.
Duplicate key collision when three or more identical keys exist.
The current logic appends a single _ when a duplicate is found, but doesn't re-check whether the new key also collides. For example, with lines KEY=1, KEY=2, KEY=3:
- First:
env['KEY'] = '1' - Second: collision →
env['KEY_'] = '2' - Third: collision on
KEY→ becomesKEY_, which overwrites the second entry
Use a loop to find a truly unique key.
Proposed fix: loop until unique key is found
if (key && key.trim()) {
let trimmedKey = key.trim();
- if (env[trimmedKey]) {
- // if key already exists, add an underscore to prevent data loss
- trimmedKey += '_'
- }
+ // Find a unique key by appending underscores until no collision
+ while (env[trimmedKey]) {
+ trimmedKey += '_';
+ }
env[trimmedKey] = valueParts.join('=').trim();
}📝 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.
| let trimmedKey = key.trim(); | |
| if (env[trimmedKey]) { | |
| // if key already exists, add an underscore to prevent data loss | |
| trimmedKey += '_' | |
| } | |
| let trimmedKey = key.trim(); | |
| // Find a unique key by appending underscores until no collision | |
| while (env[trimmedKey]) { | |
| trimmedKey += '_'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/view/modals/ClaudeMcpFormModal.tsx` around lines 423
- 427, The code that builds unique environment keys currently appends a single
underscore to trimmedKey when env[trimmedKey] exists, but may still collide;
update the logic in ClaudeMcpFormModal.tsx where trimmedKey is computed so that
you loop until the key is unique (e.g., let trimmedKey = key.trim(); while
(env[trimmedKey]) { trimmedKey += '_'; }) ensuring you check env for existence
each iteration before assigning to env[trimmedKey] to avoid overwriting existing
entries.
| if (isNewLine) { | ||
| env[''] = ''; | ||
| } |
There was a problem hiding this comment.
Empty key '' breaks downstream CLI argument construction.
Per the server code in server/routes/mcp.js, Object.entries(env) iterates all entries including the empty key, producing a malformed CLI argument -e '='. This will either fail or create an invalid environment variable in the child process.
Filter out empty keys before storing, or strip them before submission rather than using an empty key as a sentinel for "trailing newline."
Proposed fix: filter empty keys before updating config
if (isNewLine) {
- env[''] = '';
+ // Sentinel to preserve trailing newline in display
+ env[''] = '';
}
- updateConfig('env', env);
+ // Filter empty keys before storing to prevent broken CLI args
+ const cleanEnv = Object.fromEntries(
+ Object.entries(env).filter(([key]) => key !== '')
+ );
+ updateConfig('env', cleanEnv);Note: This removes the trailing-newline preservation, which may require a different approach (e.g., storing the raw textarea value separately for display purposes while only parsing valid entries for submission).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/view/modals/ClaudeMcpFormModal.tsx` around lines 431
- 433, The code in ClaudeMcpFormModal.tsx is inserting an empty key into the env
object when isNewLine is true (env[''] = ''), which creates malformed CLI args;
change the logic in the parsing/updating path (where env is constructed) to skip
any entries with empty keys (filter out key === '' or trim() === '') before
saving/updating the config or before submission (or alternatively keep the raw
textarea string in a separate state for display and only parse/submit valid
key/value pairs), and ensure any helper function that builds the env object (the
code block handling isNewLine and env) never writes an empty-string key.
1 participant
Fixes #600
This implementation adds an empty entry for a line break.
Known issues:
If you paste env variables with a line break at the end, it would create a new line.
Summary by CodeRabbit