Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds project-scoped environment-variable parsing, persistence, and resolution; applies resolved env when spawning/restoring terminals, adds .env import UI and file-select API, plus tests and related type updates. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as ProjectSettings UI
participant Dialog as Dialog API
participant FS as Filesystem API
participant Parser as Env Parser
participant Store as Project Store
User->>UI: Click "Import from .env"
UI->>Dialog: selectFile()
Dialog-->>UI: file path (or cancel)
UI->>FS: readFile(path)
FS-->>UI: content
UI->>Parser: parseEnvFile(content)
Parser-->>UI: vars + invalidLines
UI->>Parser: mergeEnvVars(existing, imported)
Parser-->>UI: merged envVars
UI->>Store: save project with envVars
Store-->>UI: persisted
sequenceDiagram
participant Layout as WorkspaceLayout / Restore flow
participant Store as Project Store
participant Parser as Env Parser
participant TerminalAPI as Terminal API
participant Process as Spawned Process
participant TerminalComp as ConnectedTerminal
Layout->>Store: get active project / layout
Store-->>Layout: project (with envVars)
Layout->>Parser: resolveEnvForSpawn(project.envVars, inheritedEnv)
Parser-->>Layout: { env, hasProjectEnv }
Layout->>TerminalAPI: spawn({ shell, cwd, env })
TerminalAPI->>Process: spawn process with env
Process-->>TerminalAPI: spawned
TerminalAPI-->>TerminalComp: spawn success
TerminalComp->>TerminalComp: write cyan info line (env count)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
gnoviawan
added a commit
that referenced
this pull request
Mar 10, 2026
Implements Issue #41 - Project-specific environment variable runtime support Changes: - Add PersistedEnvVariable type and envVars field to PersistedProject - Add .env file parsing utility with parseEnvFile and mergeEnvVars functions - Add env-resolution helper for terminal spawn with variable expansion - Extend DialogApi with selectFile method for .env file selection - Update ProjectSettings to support .env import with error handling - Pass resolved env vars through all terminal spawn paths: - WorkspaceLayout handleCreateTerminalInPane - use-terminal-restore restoreFromLayout and createDefaultTerminal - use-snapshots restoreFromSnapshot - Print one-time terminal info line when env vars are applied - Add comprehensive test coverage for env-parser utility Technical details: - Supports Unix ($VAR, ${VAR}) and Windows (%VAR%) variable references - Only expands against inherited system/process env, not other project vars - Normalizes env vars on save (trims keys, filters empty) - Shows warnings for invalid lines during .env import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Open a draft branch for the project-specific environment variable runtime support work before implementation begins. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements Issue #41 - Project-specific environment variable runtime support Changes: - Add PersistedEnvVariable type and envVars field to PersistedProject - Add .env file parsing utility with parseEnvFile and mergeEnvVars functions - Add env-resolution helper for terminal spawn with variable expansion - Extend DialogApi with selectFile method for .env file selection - Update ProjectSettings to support .env import with error handling - Pass resolved env vars through all terminal spawn paths: - WorkspaceLayout handleCreateTerminalInPane - use-terminal-restore restoreFromLayout and createDefaultTerminal - use-snapshots restoreFromSnapshot - Print one-time terminal info line when env vars are applied - Add comprehensive test coverage for env-parser utility Technical details: - Supports Unix ($VAR, ${VAR}) and Windows (%VAR%) variable references - Only expands against inherited system/process env, not other project vars - Normalizes env vars on save (trims keys, filters empty) - Shows warnings for invalid lines during .env import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/types/persistence.types.ts (1)
63-79:⚠️ Potential issue | 🟠 Major
isSecretvalues are still persisted in plaintext.This shape stores
valuefor every env var, including ones markedisSecret, in the project persistence payload. That puts imported credentials into clear-text project storage and backups, which weakens the secret flag considerably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/persistence.types.ts` around lines 63 - 79, PersistedEnvVariable currently stores plaintext value even when isSecret is true; change the type to avoid storing raw secrets (e.g., make value optional or add encryptedValue/placeholder) and modify the persistence code that writes/read PersistedProject to omit or encrypt the value for entries where PersistedEnvVariable.isSecret === true (store a placeholder or encrypted blob instead), and ensure any load/path that expects value handles missing/placeholder secrets (prompt for secret at runtime or decrypt with the secret store). Update all serialization/deserialization and any saveProject/loadProject code to follow the new shape and never write plaintext secret values to disk/backups.
🤖 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/renderer/components/terminal/ConnectedTerminal.tsx`:
- Around line 617-621: The one-time env-applied notice currently runs only when
ConnectedTerminal spawns the process; move that logic so it runs whenever a
terminal is bound/attached (including externalTerminalId flows). Specifically,
take the env-notice block that checks memoizedSpawnOptions?.env and calls
terminal.write and relocate it into the shared terminal bind/attach path (the
effect/handler that processes externalTerminalId or the function that finalizes
terminal binding), or invoke it from the same function that handles both
internal spawns and external attaches; reference memoizedSpawnOptions,
terminal.write, and externalTerminalId when locating where to add the message.
In `@src/renderer/hooks/use-projects-persistence.ts`:
- Around line 16-21: The envVars mapping in use-projects-persistence currently
persists envVar.value for secrets (envVar.isSecret) — stop storing secret values
in the project blob: when mapping project.envVars in the persistence code (the
envVars: project.envVars?.map(...) block and its twin at lines ~33-38), persist
only metadata (key and isSecret) and omit or null out value for entries where
isSecret===true; move the actual secret values into secure OS storage
(keyring/secureStore) instead, storing a reference or the key identifier in the
project record if needed, and update the save/load flow to read secrets from
that secure store rather than from the project file.
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Line 423: The code always extracts and forwards env from
resolveEnvForSpawn(project?.envVars) even when no project env exists; change
callers (e.g., where env is passed to terminalApi.spawn and similar calls) to
check the returned hasProjectEnv flag and only include the env argument when
hasProjectEnv is true so ConnectedTerminal isn't misled by an empty env; locate
resolveEnvForSpawn usage sites (the lines handling const { env } =
resolveEnvForSpawn(...), and calls to terminalApi.spawn) and conditionally pass
env (or omit it) based on hasProjectEnv.
In `@src/renderer/lib/env-parser.ts`:
- Around line 133-156: The helper resolveEnvForSpawn currently defaults
inheritedEnv to an empty object which causes platform vars like $HOME or
%USERPROFILE% to be lost; change it to source the real environment instead of an
empty default by replacing the default value with a conversion of process.env
(e.g. const defaultEnv =
Object.fromEntries(Object.entries(process.env).filter(([,v]) => v !=
null).map(([k,v]) => [k, String(v)])) or make inheritedEnv required and update
callers; ensure expandEnvReferences(envVar.value, inheritedEnv) receives that
real env so references like HOME and USERPROFILE are preserved.
In `@src/renderer/pages/ProjectSettings.tsx`:
- Around line 105-145: The handler handleImportEnvFile currently updates local
state (setImportError, setImportWarnings, setEnvVars, setHasChanges) after
multiple awaits using a captured snapshot envVars, which can apply changes to
the wrong project if the user switches while
dialogApi.selectFile/filesystemApi.readFile are in flight; fix by capturing the
current project id (or a unique request token) at the start of
handleImportEnvFile, then after every await verify the current project id/token
still matches before proceeding, and when merging use the latest state via the
functional updater form (prev => mergeEnvVars(prev, parseResult.vars)) instead
of the captured envVars so concurrent edits aren’t lost; abort or no-op the rest
of the handler (including setting errors/warnings) if the token check fails.
In `@src/shared/types/ipc.types.ts`:
- Around line 104-107: The DialogApi implementation for selectFile must be
updated to match the shared signature: change the exported selectFile
implementation (in the other DialogApi) to accept options?: { filters?: Array<{
name: string; extensions: string[] }>; title?: string } and to return
Promise<IpcResult<string>> (not string | null); ensure the implementation uses
the title option when invoking the platform dialog and converts any nullable
result into an IpcResult error or success string so the runtime return type is
non-nullable and matches the selectFile declaration.
---
Outside diff comments:
In `@src/shared/types/persistence.types.ts`:
- Around line 63-79: PersistedEnvVariable currently stores plaintext value even
when isSecret is true; change the type to avoid storing raw secrets (e.g., make
value optional or add encryptedValue/placeholder) and modify the persistence
code that writes/read PersistedProject to omit or encrypt the value for entries
where PersistedEnvVariable.isSecret === true (store a placeholder or encrypted
blob instead), and ensure any load/path that expects value handles
missing/placeholder secrets (prompt for secret at runtime or decrypt with the
secret store). Update all serialization/deserialization and any
saveProject/loadProject code to follow the new shape and never write plaintext
secret values to disk/backups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7834d39-e3fe-446b-88ee-463e0e73083b
📒 Files selected for processing (11)
src/renderer/components/terminal/ConnectedTerminal.tsxsrc/renderer/hooks/use-projects-persistence.tssrc/renderer/hooks/use-snapshots.tssrc/renderer/hooks/use-terminal-restore.tssrc/renderer/layouts/WorkspaceLayout.tsxsrc/renderer/lib/dialog-api.tssrc/renderer/lib/env-parser.test.tssrc/renderer/lib/env-parser.tssrc/renderer/pages/ProjectSettings.tsxsrc/shared/types/ipc.types.tssrc/shared/types/persistence.types.ts
Comment on lines
+16
to
+21
| defaultShell: project.defaultShell, | ||
| envVars: project.envVars?.map((envVar) => ({ | ||
| key: envVar.key, | ||
| value: envVar.value, | ||
| isSecret: envVar.isSecret | ||
| })) |
There was a problem hiding this comment.
Don't persist secret env values in the plain project store.
value is now round-tripped for every env var, including ones marked isSecret. That makes project secrets readable from the normal persisted projects file, which is a security/compliance regression. Please move secret values to secure OS storage or keep them out of this blob entirely.
Also applies to: 33-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/use-projects-persistence.ts` around lines 16 - 21, The
envVars mapping in use-projects-persistence currently persists envVar.value for
secrets (envVar.isSecret) — stop storing secret values in the project blob: when
mapping project.envVars in the persistence code (the envVars:
project.envVars?.map(...) block and its twin at lines ~33-38), persist only
metadata (key and isSecret) and omit or null out value for entries where
isSecret===true; move the actual secret values into secure OS storage
(keyring/secureStore) instead, storing a reference or the key identifier in the
project record if needed, and update the save/load flow to read secrets from
that secure store rather than from the project file.
Comment on lines
+104
to
+107
| selectFile: (options?: { | ||
| filters?: Array<{ name: string; extensions: string[] }> | ||
| title?: string | ||
| }) => Promise<IpcResult<string>> |
There was a problem hiding this comment.
Keep the other DialogApi implementation in sync with this signature.
src/renderer/lib/tauri-dialog-api.ts still exposes selectFile(options?: { filters?: ... }): Promise<IpcResult<string | null>>. After this contract change it no longer accepts title and no longer matches the declared return type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/types/ipc.types.ts` around lines 104 - 107, The DialogApi
implementation for selectFile must be updated to match the shared signature:
change the exported selectFile implementation (in the other DialogApi) to accept
options?: { filters?: Array<{ name: string; extensions: string[] }>; title?:
string } and to return Promise<IpcResult<string>> (not string | null); ensure
the implementation uses the title option when invoking the platform dialog and
converts any nullable result into an IpcResult error or success string so the
runtime return type is non-nullable and matches the selectFile declaration.
gnoviawan
force-pushed
the
feat/issue-41-project-env-runtime-support
branch
from
79e844f to
d1539d1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/renderer/hooks/use-terminal-restore.ts (1)
500-501:⚠️ Potential issue | 🟡 MinorOnly pass
envwhen project vars actually exist.These paths still ignore
hasProjectEnvand always forwardenv, so a project with no configured env vars will still look like an env-injected spawn downstream and can trigger the one-time info line on restore/default creation.Suggested fix
- const { env } = resolveEnvForSpawn(project?.envVars) + const { env, hasProjectEnv } = resolveEnvForSpawn(project?.envVars) @@ - const spawnResult = await terminalApi.spawn({ - shell: normalizedShell, - cwd: persistedTerminal.cwd, - env - }) + const spawnResult = await terminalApi.spawn({ + shell: normalizedShell, + cwd: persistedTerminal.cwd, + ...(hasProjectEnv ? { env } : {}) + }) @@ - const { env } = resolveEnvForSpawn(project?.envVars) + const { env, hasProjectEnv } = resolveEnvForSpawn(project?.envVars) @@ - const spawnResult = await terminalApi.spawn({ - shell, - cwd: project?.path, - env - }) + const spawnResult = await terminalApi.spawn({ + shell, + cwd: project?.path, + ...(hasProjectEnv ? { env } : {}) + })Also applies to: 555-559, 716-718, 724-728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-terminal-restore.ts` around lines 500 - 501, The code always forwards the resolved env from resolveEnvForSpawn(project?.envVars) even when no project vars exist, causing downstream logic to treat spawns as env-injected; only pass env when the project actually has env vars. Update places using the pattern (e.g., the call that does “const { env } = resolveEnvForSpawn(...)” and subsequent spawn/restoration calls in use-terminal-restore) to check hasProjectEnv (or the resolver return flag) and only include env in the spawn/options object when hasProjectEnv is true; apply the same conditional change to the other occurrences noted (around the blocks corresponding to the ranges you flagged).src/shared/types/ipc.types.ts (1)
104-107:⚠️ Potential issue | 🟠 MajorKeep every
DialogApi.selectFileimplementation aligned with this contract.Lines 104-107 now require
titlesupport and a non-nullIpcResult<string>, butsrc/renderer/lib/tauri-dialog-api.ts:18-28still omitstitleand returnsIpcResult<string | null>. That leaves one runtime implementation out of contract and can break callers such assrc/renderer/pages/ProjectSettings.tsx:105-125that assume a selected path is always a string on success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/ipc.types.ts` around lines 104 - 107, DialogApi.selectFile implementation in tauri-dialog-api.ts must match the updated contract: accept an optional title parameter and return Promise<IpcResult<string>> (no null on success). Update the tauri dialog function (the selectFile implementation) to add title?: string to its options, pass that title into the Tauri dialog invocation, and ensure the returned IpcResult uses the success variant with a non-null string path on success and a failure/error variant when cancelled or errored so callers like ProjectSettings can rely on a string on success.src/renderer/lib/env-parser.ts (1)
133-156:⚠️ Potential issue | 🟠 MajorDon't resolve against
{}by default.All current callers use the one-argument form, so Lines 135-149 expand
$HOME,${HOME}, and%USERPROFILE%against an empty map and silently erase those references before spawn. That means project vars likeFOO=$HOME/appare corrupted at runtime even though the child process later inherits the real environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/env-parser.ts` around lines 133 - 156, resolveEnvForSpawn is currently defaulting inheritedEnv to {} which causes expandEnvReferences to overwrite unresolved references like $HOME; make inheritedEnv optional (remove the {} default) and only call expandEnvReferences when an inheritedEnv is actually supplied — otherwise leave envVar.value untouched. Update the function signature resolveEnvForSpawn(...) to accept inheritedEnv?: Record<string,string> and change the loop to use expandedValue = inheritedEnv ? expandEnvReferences(envVar.value, inheritedEnv) : envVar.value so unresolved references are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 500-501: The code always forwards the resolved env from
resolveEnvForSpawn(project?.envVars) even when no project vars exist, causing
downstream logic to treat spawns as env-injected; only pass env when the project
actually has env vars. Update places using the pattern (e.g., the call that does
“const { env } = resolveEnvForSpawn(...)” and subsequent spawn/restoration calls
in use-terminal-restore) to check hasProjectEnv (or the resolver return flag)
and only include env in the spawn/options object when hasProjectEnv is true;
apply the same conditional change to the other occurrences noted (around the
blocks corresponding to the ranges you flagged).
In `@src/renderer/lib/env-parser.ts`:
- Around line 133-156: resolveEnvForSpawn is currently defaulting inheritedEnv
to {} which causes expandEnvReferences to overwrite unresolved references like
$HOME; make inheritedEnv optional (remove the {} default) and only call
expandEnvReferences when an inheritedEnv is actually supplied — otherwise leave
envVar.value untouched. Update the function signature resolveEnvForSpawn(...) to
accept inheritedEnv?: Record<string,string> and change the loop to use
expandedValue = inheritedEnv ? expandEnvReferences(envVar.value, inheritedEnv) :
envVar.value so unresolved references are preserved.
In `@src/shared/types/ipc.types.ts`:
- Around line 104-107: DialogApi.selectFile implementation in
tauri-dialog-api.ts must match the updated contract: accept an optional title
parameter and return Promise<IpcResult<string>> (no null on success). Update the
tauri dialog function (the selectFile implementation) to add title?: string to
its options, pass that title into the Tauri dialog invocation, and ensure the
returned IpcResult uses the success variant with a non-null string path on
success and a failure/error variant when cancelled or errored so callers like
ProjectSettings can rely on a string on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b8b9223-9a1d-461d-8ebc-e51ae3e9c93d
📒 Files selected for processing (11)
src/renderer/components/terminal/ConnectedTerminal.tsxsrc/renderer/hooks/use-projects-persistence.tssrc/renderer/hooks/use-snapshots.tssrc/renderer/hooks/use-terminal-restore.tssrc/renderer/layouts/WorkspaceLayout.tsxsrc/renderer/lib/dialog-api.tssrc/renderer/lib/env-parser.test.tssrc/renderer/lib/env-parser.tssrc/renderer/pages/ProjectSettings.tsxsrc/shared/types/ipc.types.tssrc/shared/types/persistence.types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/lib/dialog-api.ts
- src/renderer/layouts/WorkspaceLayout.tsx
- src/renderer/hooks/use-projects-persistence.ts
- src/shared/types/persistence.types.ts
- src/renderer/pages/ProjectSettings.tsx
- src/renderer/components/terminal/ConnectedTerminal.tsx
- src/renderer/hooks/use-snapshots.ts
- Fix race condition in handleImportEnvFile by capturing project ID and using functional state updates - Make inheritedEnv parameter required in resolveEnvForSpawn and add TODO for fetching system env from backend - Use hasProjectEnv flag to conditionally pass env to terminal spawn - Add env notice check in ConnectedTerminal for external terminal ID path - Add TODO comments about secure storage for secret env var values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_bmad-output/implementation-artifacts/tech-spec-complete-project-specific-environment-variable-runtime-support.mdTest plan
.envimport behavior🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UX
Tests