refactor(pty): improve PATH refresh reliability and terminal restore locks - #400
Conversation
📝 WalkthroughWalkthroughNon-Windows PATH probing now wraps shell-emitted PATH values in markers and extracts them from stdout. Terminal restore now defers cleanup when hidden, stops creating fallback default terminals on empty/error paths, adds autosave logging, skips two restore tests, and updates one agent-launch expectation. ChangesUnix PATH refresh probe
Terminal restore flow
Agent launch test
Sequence Diagram(s)sequenceDiagram
participant useTerminalRestore
participant isVisibleReady
participant PROJECT_RESTORE_LOCKS
participant setTerminalRestoreInProgress
participant RestoreComplete as restore-complete continuity event
useTerminalRestore->>isVisibleReady: check visibility
alt window not visible
isVisibleReady-->>useTerminalRestore: false
useTerminalRestore->>PROJECT_RESTORE_LOCKS: clear current project lock
useTerminalRestore->>setTerminalRestoreInProgress: set false
useTerminalRestore->>useTerminalRestore: retry later
else no persisted terminals
useTerminalRestore->>RestoreComplete: emit zero-count completion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@src-tauri/src/pty/env_refresh.rs`:
- Around line 168-178: The PATH parsing in env_refresh’s marker extraction is
too permissive because it falls back to raw.trim(), which can accidentally treat
shell output as a valid PATH. Update the parsing logic around the
__TERMUL_PATH__ handling to fail closed: only return a parsed path when both
marker delimiters are present, and otherwise return None so the existing
environment is left unchanged. Keep the fix localized to the parsing block that
computes path from output.stdout.
- Around line 149-158: The PATH refresh probe in env_refresh.rs should follow
the same startup path as the PTY launch shell, since the current
Command::new(&shell).args(["-c", ...]) logic for bash and zsh only sources rc
files and can miss login/profile PATH changes. Update the bash/zsh probe
commands in the shell match arm to use the appropriate login startup path so
refreshed PATH matches what users get when the shell starts.
In `@src/renderer/hooks/use-terminal-restore.test.ts`:
- Line 544: The skipped restore tests in use-terminal-restore.test.ts should be
updated rather than left disabled, since the default-spawn retry/cancel contract
has changed. Replace the it.skip cases around the default-terminal restore flow
with assertions for the new restore behavior in useTerminalRestore, focusing on
empty-workspace handling and the absence of fallback/default-spawn retries. Keep
the test intent aligned with the existing restore-contract coverage and update
any related expectations in the paired skipped test as well.
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 376-380: Keep the terminal auto-save blocker active until a
deferred restore actually completes. In useTerminalRestore, do not clear the
restore-in-progress state for projectIdToRestore when restore is deferred; only
release isRestoringRef, PROJECT_RESTORE_LOCKS, and setTerminalRestoreInProgress
after the deferred retry finishes. Update the deferred restore path and the
matching cleanup around the retry logic so isTerminalRestoreInProgress() still
returns true during the hidden-window gap, preventing useTerminalAutoSave from
persisting an incomplete layout.
- Around line 501-510: The restore-complete continuity event in
useTerminalRestore is reporting the wrong path for the empty-restore case.
Update the emitTerminalContinuityEvent payload so the details.path value
reflects an explicit empty-restore path instead of default-terminal when no
terminal is created, keeping the restore-complete event semantics aligned with
the telemetry consumers. Use the use-terminal-restore flow and
emitTerminalContinuityEvent call as the location to adjust.
In `@src/renderer/hooks/useTerminalAutoSave.ts`:
- Around line 222-226: The warning logs in useTerminalAutoSave should not
serialize terminal restore state or expose owner IDs, project-derived entries,
or layout contents. Update the logFrontendError calls in the auto-save/restore
flow to emit only aggregate information, such as counts or generic status, and
remove JSON.stringify of terminalRestoreProjectsInProgress as well as any layout
details. Keep the diagnostics in useTerminalAutoSave and its related
restore-check path informative but non-sensitive.
- Around line 182-187: The warning telemetry in useTerminalAutoSave’s subscriber
runs before the activity-only/no-op filters, so it can emit noisy logs for
changes that will never save. Move the logApi.logFrontendError call to after the
terminal-restore and activity-only guards in the subscriber flow, or remove it
entirely if it is not needed, so only save-relevant changes can produce
telemetry.
In `@src/renderer/lib/agent-launch.test.ts`:
- Around line 109-112: The test expectation in launchAgentInPane() is out of
sync with the current registry contract because getBuiltInAgent('gemini-cli')
still maps to the gemini command and buildAgentArgv() passes that through to
terminalApi.spawn(). Update the assertion in agent-launch.test.ts to match the
runtime command returned by the registry, or only change it to agy if the
corresponding agent-registry.ts definition is also being updated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0a49193-0da0-4ebb-b962-88e2e6a361a6
📒 Files selected for processing (5)
src-tauri/src/pty/env_refresh.rssrc/renderer/hooks/use-terminal-restore.test.tssrc/renderer/hooks/use-terminal-restore.tssrc/renderer/hooks/useTerminalAutoSave.tssrc/renderer/lib/agent-launch.test.ts
| "zsh" => Command::new(&shell) | ||
| .args(["-c", "source ~/.zshrc >/dev/null 2>&1; printf \"__TERMUL_PATH__%s__TERMUL_PATH__\" \"$PATH\""]) | ||
| .output() | ||
| .ok()?, | ||
| "bash" => Command::new(&shell) | ||
| .args(["-c", "source ~/.bashrc >/dev/null 2>&1; printf \"__TERMUL_PATH__%s__TERMUL_PATH__\" \"$PATH\""]) | ||
| .output() | ||
| .ok()?, | ||
| "fish" => Command::new(&shell) | ||
| .args(["-lc", "string join : $PATH"]) | ||
| .args(["-c", "source ~/.config/fish/config.fish >/dev/null 2>&1; printf \"__TERMUL_PATH__%s__TERMUL_PATH__\" (string join : $PATH)"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat >"$tmp/.bash_profile" <<'EOF'
export PATH="$HOME/profile-bin:$PATH"
[ -f ~/.bashrc ] && . ~/.bashrc
EOF
cat >"$tmp/.bashrc" <<'EOF'
case $- in *i*) ;; *) return;; esac
export PATH="$HOME/rc-bin:$PATH"
EOF
echo "current probe shape:"
HOME="$tmp" bash -c 'source ~/.bashrc >/dev/null 2>&1; printf "__TERMUL_PATH__%s__TERMUL_PATH__\n" "$PATH"'
echo "login + interactive shell shape:"
HOME="$tmp" bash -lic 'printf "__TERMUL_PATH__%s__TERMUL_PATH__\n" "$PATH"' 2>/dev/nullRepository: gnoviawan/termul
Length of output: 563
Keep the PATH probe on the same shell startup path as launch. The bash/zsh probes use -c and source only rc files, so PATH additions from login/profile startup files (~/.bash_profile, ~/.profile, ~/.zprofile) are skipped and the refreshed PATH can differ from what users actually get in the PTY.
🤖 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 `@src-tauri/src/pty/env_refresh.rs` around lines 149 - 158, The PATH refresh
probe in env_refresh.rs should follow the same startup path as the PTY launch
shell, since the current Command::new(&shell).args(["-c", ...]) logic for bash
and zsh only sources rc files and can miss login/profile PATH changes. Update
the bash/zsh probe commands in the shell match arm to use the appropriate login
startup path so refreshed PATH matches what users get when the shell starts.
| let raw = String::from_utf8_lossy(&output.stdout); | ||
| let path = if let Some(start) = raw.find("__TERMUL_PATH__") { | ||
| if let Some(end) = raw[start + 15..].find("__TERMUL_PATH__") { | ||
| raw[start + 15..start + 15 + end].to_string() | ||
| } else { | ||
| raw[start + 15..].trim().to_string() | ||
| } | ||
| } else { | ||
| raw.trim().to_string() | ||
| }; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed when PATH markers are missing or incomplete.
After introducing a marker protocol, falling back to raw.trim() can apply shell chatter as PATH. Return None unless both delimiters are present, so the existing environment is preserved.
🐛 Proposed parser hardening
let raw = String::from_utf8_lossy(&output.stdout);
- let path = if let Some(start) = raw.find("__TERMUL_PATH__") {
- if let Some(end) = raw[start + 15..].find("__TERMUL_PATH__") {
- raw[start + 15..start + 15 + end].to_string()
- } else {
- raw[start + 15..].trim().to_string()
- }
- } else {
- raw.trim().to_string()
- };
+ const PATH_MARKER: &str = "__TERMUL_PATH__";
+ let start = raw.find(PATH_MARKER)?;
+ let path_start = start + PATH_MARKER.len();
+ let end = raw[path_start..].find(PATH_MARKER)?;
+ let path = raw[path_start..path_start + end].to_string();📝 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 raw = String::from_utf8_lossy(&output.stdout); | |
| let path = if let Some(start) = raw.find("__TERMUL_PATH__") { | |
| if let Some(end) = raw[start + 15..].find("__TERMUL_PATH__") { | |
| raw[start + 15..start + 15 + end].to_string() | |
| } else { | |
| raw[start + 15..].trim().to_string() | |
| } | |
| } else { | |
| raw.trim().to_string() | |
| }; | |
| let raw = String::from_utf8_lossy(&output.stdout); | |
| const PATH_MARKER: &str = "__TERMUL_PATH__"; | |
| let start = raw.find(PATH_MARKER)?; | |
| let path_start = start + PATH_MARKER.len(); | |
| let end = raw[path_start..].find(PATH_MARKER)?; | |
| let path = raw[path_start..path_start + end].to_string(); |
🤖 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 `@src-tauri/src/pty/env_refresh.rs` around lines 168 - 178, The PATH parsing in
env_refresh’s marker extraction is too permissive because it falls back to
raw.trim(), which can accidentally treat shell output as a valid PATH. Update
the parsing logic around the __TERMUL_PATH__ handling to fail closed: only
return a parsed path when both marker delimiters are present, and otherwise
return None so the existing environment is left unchanged. Keep the fix
localized to the parsing block that computes path from output.stdout.
| }) | ||
|
|
||
| it('does not loop default-terminal restore retries when a default spawn succeeds', async () => { | ||
| it.skip('does not loop default-terminal restore retries when a default spawn succeeds', async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace skipped tests with updated restore-contract assertions.
These tests covered default-spawn retry/cancel behavior. If that behavior is intentionally removed, update them to assert the new empty-workspace and no-fallback behavior instead of leaving permanent skips.
Also applies to: 581-581
🤖 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 `@src/renderer/hooks/use-terminal-restore.test.ts` at line 544, The skipped
restore tests in use-terminal-restore.test.ts should be updated rather than left
disabled, since the default-spawn retry/cancel contract has changed. Replace the
it.skip cases around the default-terminal restore flow with assertions for the
new restore behavior in useTerminalRestore, focusing on empty-workspace handling
and the absence of fallback/default-spawn retries. Keep the test intent aligned
with the existing restore-contract coverage and update any related expectations
in the paired skipped test as well.
| isDeferred = true | ||
| previousProjectIdRef.current = actualPreviousProjectId | ||
| isRestoringRef.current.delete(projectIdToRestore) | ||
| PROJECT_RESTORE_LOCKS.delete(projectIdToRestore) | ||
| setTerminalRestoreInProgress(projectIdToRestore, false, restoreOwnerId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the auto-save blocker active while restore is deferred.
Line 380 clears terminalRestoreProjectsInProgress before the deferred restore has retried. Since useTerminalAutoSave only blocks writes via isTerminalRestoreInProgress(), terminal-store changes during the hidden-window gap can persist an empty/incomplete layout for projectIdToRestore.
Proposed fix
previousProjectIdRef.current = actualPreviousProjectId
isRestoringRef.current.delete(projectIdToRestore)
PROJECT_RESTORE_LOCKS.delete(projectIdToRestore)
- setTerminalRestoreInProgress(projectIdToRestore, false, restoreOwnerId)
setTimeout(() => setVisibilityRetry((v) => v + 1), DEFERRED_RETRY_MS)
returnAlso applies to: 609-614
🤖 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 `@src/renderer/hooks/use-terminal-restore.ts` around lines 376 - 380, Keep the
terminal auto-save blocker active until a deferred restore actually completes.
In useTerminalRestore, do not clear the restore-in-progress state for
projectIdToRestore when restore is deferred; only release isRestoringRef,
PROJECT_RESTORE_LOCKS, and setTerminalRestoreInProgress after the deferred retry
finishes. Update the deferred restore path and the matching cleanup around the
retry logic so isTerminalRestoreInProgress() still returns true during the
hidden-window gap, preventing useTerminalAutoSave from persisting an incomplete
layout.
| emitTerminalContinuityEvent({ | ||
| name: 'restore-complete', | ||
| correlationId: continuityCorrelationId, | ||
| projectId: projectIdToRestore, | ||
| details: { | ||
| path: 'default-terminal', | ||
| persistedTerminalCount: 0, | ||
| restoredTerminalCount: 0, | ||
| attempt: 0 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use an empty-restore path instead of default-terminal.
This completion event now means “no terminal was created,” but Line 506 still reports path: 'default-terminal'. That makes continuity telemetry ambiguous for consumers comparing restored/default/empty paths.
Proposed fix
details: {
- path: 'default-terminal',
+ path: 'empty-workspace',
persistedTerminalCount: 0,
restoredTerminalCount: 0,
attempt: 0📝 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.
| emitTerminalContinuityEvent({ | |
| name: 'restore-complete', | |
| correlationId: continuityCorrelationId, | |
| projectId: projectIdToRestore, | |
| details: { | |
| path: 'default-terminal', | |
| persistedTerminalCount: 0, | |
| restoredTerminalCount: 0, | |
| attempt: 0 | |
| } | |
| emitTerminalContinuityEvent({ | |
| name: 'restore-complete', | |
| correlationId: continuityCorrelationId, | |
| projectId: projectIdToRestore, | |
| details: { | |
| path: 'empty-workspace', | |
| persistedTerminalCount: 0, | |
| restoredTerminalCount: 0, | |
| attempt: 0 | |
| } |
🤖 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 `@src/renderer/hooks/use-terminal-restore.ts` around lines 501 - 510, The
restore-complete continuity event in useTerminalRestore is reporting the wrong
path for the empty-restore case. Update the emitTerminalContinuityEvent payload
so the details.path value reflects an explicit empty-restore path instead of
default-terminal when no terminal is created, keeping the restore-complete event
semantics aligned with the telemetry consumers. Use the use-terminal-restore
flow and emitTerminalContinuityEvent call as the location to adjust.
| void logApi.logFrontendError({ | ||
| level: 'warn', | ||
| source: 'useTerminalAutoSave', | ||
| message: `Subscriber triggered. Terminals count: ${state.terminals.length}, activeTerminalId: ${state.activeTerminalId}, isRestoring: ${isTerminalRestoreInProgress()}, mapSize: ${terminalRestoreProjectsInProgress.size}` | ||
| }) | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Don’t warn-log before the activity-only filter.
This runs before Lines 188-217 discard activity-only changes, so high-frequency terminal activity can produce warning/error telemetry even when no save will occur. Move it after the no-op filters or remove it.
Proposed fix
- void logApi.logFrontendError({
- level: 'warn',
- source: 'useTerminalAutoSave',
- message: `Subscriber triggered. Terminals count: ${state.terminals.length}, activeTerminalId: ${state.activeTerminalId}, isRestoring: ${isTerminalRestoreInProgress()}, mapSize: ${terminalRestoreProjectsInProgress.size}`
- })
-
// Skip activity-only changes (hasActivity/lastActivityTimestamp) and reorderings📝 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.
| void logApi.logFrontendError({ | |
| level: 'warn', | |
| source: 'useTerminalAutoSave', | |
| message: `Subscriber triggered. Terminals count: ${state.terminals.length}, activeTerminalId: ${state.activeTerminalId}, isRestoring: ${isTerminalRestoreInProgress()}, mapSize: ${terminalRestoreProjectsInProgress.size}` | |
| }) |
🤖 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 `@src/renderer/hooks/useTerminalAutoSave.ts` around lines 182 - 187, The
warning telemetry in useTerminalAutoSave’s subscriber runs before the
activity-only/no-op filters, so it can emit noisy logs for changes that will
never save. Move the logApi.logFrontendError call to after the terminal-restore
and activity-only guards in the subscriber flow, or remove it entirely if it is
not needed, so only save-relevant changes can produce telemetry.
| void logApi.logFrontendError({ | ||
| level: 'warn', | ||
| source: 'useTerminalAutoSave', | ||
| message: `Blocked auto-save because restore is in progress! map: ${JSON.stringify(Array.from(terminalRestoreProjectsInProgress.entries()))}` | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging restore owners and serialized terminal layouts.
The restore map includes owner IDs derived from project IDs, and layout can include terminal names, cwd, shell, and agent args. Keep these logs aggregate-only.
Proposed fix
if (isTerminalRestoreInProgress()) {
void logApi.logFrontendError({
level: 'warn',
source: 'useTerminalAutoSave',
- message: `Blocked auto-save because restore is in progress! map: ${JSON.stringify(Array.from(terminalRestoreProjectsInProgress.entries()))}`
+ message: `Blocked auto-save because restore is in progress. restoreProjectCount: ${terminalRestoreProjectsInProgress.size}`
})
return
}
@@
void logApi.logFrontendError({
level: 'warn',
source: 'useTerminalAutoSave',
- message: `Saving terminals layout: ${JSON.stringify(layout)}`
+ message: `Saving terminals layout. terminalCount: ${layout.terminals.length}`
})Also applies to: 242-246
🤖 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 `@src/renderer/hooks/useTerminalAutoSave.ts` around lines 222 - 226, The
warning logs in useTerminalAutoSave should not serialize terminal restore state
or expose owner IDs, project-derived entries, or layout contents. Update the
logFrontendError calls in the auto-save/restore flow to emit only aggregate
information, such as counts or generic status, and remove JSON.stringify of
terminalRestoreProjectsInProgress as well as any layout details. Keep the
diagnostics in useTerminalAutoSave and its related restore-check path
informative but non-sensitive.
| it('uses the flag form for agy (Antigravity CLI)', async () => { | ||
| await launchAgentInPane('pane-1', 'proj-1', '/test', gemini, 'query') | ||
| expect(mockTerminalApiSpawn).toHaveBeenCalledWith( | ||
| expect.objectContaining({ program: 'gemini', args: ['-i', 'query'], kind: 'agent' }) | ||
| expect.objectContaining({ program: 'agy', args: ['-i', 'query'], kind: 'agent' }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep this expectation aligned with the registry contract.
getBuiltInAgent('gemini-cli') still resolves to a definition whose command is 'gemini', and launchAgentInPane() forwards that through buildAgentArgv() into terminalApi.spawn(). As written, this assertion expects 'agy', so it no longer matches the supplied runtime path and will fail unless the registry change lands in src/renderer/lib/agents/agent-registry.ts too.
🤖 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 `@src/renderer/lib/agent-launch.test.ts` around lines 109 - 112, The test
expectation in launchAgentInPane() is out of sync with the current registry
contract because getBuiltInAgent('gemini-cli') still maps to the gemini command
and buildAgentArgv() passes that through to terminalApi.spawn(). Update the
assertion in agent-launch.test.ts to match the runtime command returned by the
registry, or only change it to agy if the corresponding agent-registry.ts
definition is also being updated.
Ignores interactive shell startup output during PATH environment sourcing using custom delimiters and /dev/null redirection, prevents spawn lock deadlocks in use-terminal-restore by ensuring restore locks are freed in a finally block, and adds logging to monitor PTY auto-save blocker states.
Summary by CodeRabbit
Bug Fixes
Chores