Add terminal profile settings groundwork#1589
Add terminal profile settings groundwork#1589gmackie wants to merge 2 commits intopingdotgg:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository 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 |
github-actions
bot
added
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| return yield* Effect.promise(() => | ||
| discoverTerminalShells({ | ||
| platform: process.platform, | ||
| env: process.env, | ||
| probe: (candidatePath) => Effect.runPromise(fileSystem.exists(candidatePath)), | ||
| }), |
There was a problem hiding this comment.
🟡 Medium src/wsServer.ts:314
In loadTerminalDiscovery, the probe callback returns Effect.runPromise(fileSystem.exists(candidatePath)). If fileSystem.exists throws (e.g., permission denied), the promise rejects instead of resolving to false. Since discoverTerminalShells expects Promise<boolean> and doesn't catch probe rejections, this causes the entire terminal discovery to fail as a defect, breaking serverGetConfig instead of gracefully marking shells as unavailable. Consider catching errors and returning false.
probe: (candidatePath) => Effect.runPromise(fileSystem.exists(candidatePath).pipe(Effect.catch(() => Effect.succeed(false)))),🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/wsServer.ts around lines 314-319:
In `loadTerminalDiscovery`, the `probe` callback returns `Effect.runPromise(fileSystem.exists(candidatePath))`. If `fileSystem.exists` throws (e.g., permission denied), the promise rejects instead of resolving to `false`. Since `discoverTerminalShells` expects `Promise<boolean>` and doesn't catch probe rejections, this causes the entire terminal discovery to fail as a defect, breaking `serverGetConfig` instead of gracefully marking shells as unavailable. Consider catching errors and returning `false`.
Evidence trail:
apps/server/src/wsServer.ts:318 - probe callback uses Effect.runPromise(fileSystem.exists(candidatePath))
apps/server/src/wsServer.ts:267 - fileSystem from Effect's FileSystem.FileSystem
apps/server/src/terminal/terminalProfile.ts:186-196 - firstExistingPath has no try-catch around await probe(candidate)
apps/server/src/terminal/terminalProfile.ts:198-230 - discoverWindowsTerminalShells has no try-catch around firstExistingPath calls
apps/server/src/terminal/terminalProfile.ts:233-262 - discoverTerminalShells has no try-catch around discoverWindowsTerminalShells call
apps/server/src/terminal/terminalProfile.ts:25 - TerminalShellPathProbe type expects Promise<boolean>
apps/server/src/wsServer.ts:328-330 - shows Error handling pattern with Effect.catch for fileSystem.stat used elsewhere
| [settings.terminal.profile, updateSettings], | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟠 High settings/SettingsPanels.tsx:668
When the user types a newline in the "Shell arguments" or "Environment variables" textareas, the newline is immediately deleted. This happens because parseTerminalShellArgs filters empty lines (stripping the trailing newline), then formatTerminalShellArgs joins without trailing newlines, and the useEffect at lines 668–675 immediately overwrites the draft with the reformatted value. Users cannot type a newline to start a new entry.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/components/settings/SettingsPanels.tsx around line 668:
When the user types a newline in the "Shell arguments" or "Environment variables" textareas, the newline is immediately deleted. This happens because `parseTerminalShellArgs` filters empty lines (stripping the trailing newline), then `formatTerminalShellArgs` joins without trailing newlines, and the `useEffect` at lines 668–675 immediately overwrites the draft with the reformatted value. Users cannot type a newline to start a new entry.
Evidence trail:
apps/web/src/components/settings/SettingsPanels.tsx lines 442-444 (formatTerminalShellArgs), lines 446-450 (parseTerminalShellArgs), lines 668-670 (useEffect that overwrites draft), lines 1528-1531 (onChange handler that parses and updates settings). apps/web/src/hooks/useSettings.ts lines 100-125 (updateSettings uses setQueryData creating new references).
There was a problem hiding this comment.
I wasn't able to reproduce this locally. I tightened the browser test to try the more realistic interaction path as well, focusing the existing shell-args textarea and sending an Enter key instead of just using fill(), and it still preserved the trailing newline. Since I couldn't get a failing repro from the current Electron/browser harness, I left the production UI code unchanged rather than landing a speculative state-sync fix.
If there's a concrete repro path or platform-specific case you're seeing, I can take another pass with that exact flow.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
1 participant
Summary
Notes
Test Plan
Refs #292
Note
Add terminal profile settings for shell path, args, and environment variables
TerminalProfileSettingsschema topackages/contracts/src/settings.tswithshellPath,shellArgs, andenvfields, extendingServerSettingsandServerSettingsPatchto support terminal profile configuration.GeneralSettingsPanelwhere users can configure shell path, arguments, and environment variables, persisted viaupdateSettings.terminalProfile.tswithresolveTerminalShellSpawnConfig,discoverTerminalShells, anddiscoverWindowsTerminalShellsto resolve which shell and args to use when spawning terminals.TerminalManagerRuntimeinManager.tsto source shell config from the terminal profile resolver at spawn time, merging profile env with runtime env.server.getConfiginwsServer.tsto include aterminalfield withplatform,currentShell, anddiscoveredShells.📊 Macroscope summarized 7bdfbf6. 13 files reviewed, 3 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues