Skip to content

Commit b43d988

Browse files
authored
fix: address review feedback for terminal path normalization and switch-hook context
Agent-Logs-Url: https://github.com/InterestingSoftware/SproutGit/sessions/55f34c67-940b-41ff-9398-342e241c4971
1 parent 3d9e648 commit b43d988

5 files changed

Lines changed: 131 additions & 15 deletions

File tree

src-tauri/src/hooks.rs

Lines changed: 100 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2085,8 +2085,14 @@ pub async fn run_worktree_switch_hooks(
20852085

20862086
#[cfg(test)]
20872087
mod tests {
2088-
use super::{ensure_dependency_triggers_compatible, normalize_hook_script};
2088+
use super::{
2089+
build_switch_hook_session_key, ensure_dependency_triggers_compatible,
2090+
mark_switch_hook_ran_this_session, normalize_hook_script, normalize_switch_auto_run_flags,
2091+
parse_switch_hook_source, should_skip_switch_hook_once_per_session,
2092+
validate_switch_once_per_session, HookExecutionContext, RuntimeHook, SWITCH_HOOK_SESSION_RUNS,
2093+
};
20892094
use std::collections::HashMap;
2095+
use std::path::PathBuf;
20902096

20912097
#[test]
20922098
fn normalize_hook_script_allows_multiline_scripts() {
@@ -2136,4 +2142,97 @@ mod tests {
21362142
Err(err) => panic!("manual dependency should be allowed: {err}"),
21372143
}
21382144
}
2145+
2146+
#[test]
2147+
fn parse_switch_hook_source_defaults_to_manual_when_missing_or_blank() {
2148+
assert!(matches!(parse_switch_hook_source(None), Ok(super::SwitchHookSource::Manual)));
2149+
assert!(matches!(
2150+
parse_switch_hook_source(Some(" ")),
2151+
Ok(super::SwitchHookSource::Manual)
2152+
));
2153+
}
2154+
2155+
#[test]
2156+
fn parse_switch_hook_source_rejects_unknown_value() {
2157+
match parse_switch_hook_source(Some("invalid")) {
2158+
Ok(_) => panic!("invalid source should be rejected"),
2159+
Err(err) => assert!(err.contains("Unsupported switch hook source")),
2160+
}
2161+
}
2162+
2163+
#[test]
2164+
fn validate_switch_once_per_session_allows_only_switch_triggers() {
2165+
match validate_switch_once_per_session("before_worktree_switch", true) {
2166+
Ok(value) => assert!(value),
2167+
Err(err) => panic!("switch trigger should allow once-per-session: {err}"),
2168+
}
2169+
2170+
match validate_switch_once_per_session("before_worktree_create", true) {
2171+
Ok(_) => panic!("non-switch trigger should reject once-per-session"),
2172+
Err(err) => assert!(err.contains("switchOncePerSession can only be enabled")),
2173+
}
2174+
}
2175+
2176+
#[test]
2177+
fn normalize_switch_auto_run_flags_defaults_for_non_switch_triggers() {
2178+
assert_eq!(
2179+
normalize_switch_auto_run_flags("before_worktree_create", false, true),
2180+
(true, false)
2181+
);
2182+
assert_eq!(
2183+
normalize_switch_auto_run_flags("before_worktree_switch", false, true),
2184+
(false, true)
2185+
);
2186+
}
2187+
2188+
#[test]
2189+
fn switch_once_per_session_marks_and_skips_after_first_run() {
2190+
if let Ok(mut runs) = SWITCH_HOOK_SESSION_RUNS.lock() {
2191+
runs.clear();
2192+
} else {
2193+
panic!("failed to clear switch hook session runs");
2194+
}
2195+
2196+
let context = HookExecutionContext {
2197+
workspace_path: PathBuf::from("/tmp/workspace"),
2198+
trigger_worktree_path: Some(PathBuf::from("/tmp/workspace/worktrees/feature-a")),
2199+
initiating_worktree_path: None,
2200+
source_ref: None,
2201+
};
2202+
let hook = RuntimeHook {
2203+
id: "hook-1".to_string(),
2204+
name: "hook".to_string(),
2205+
scope: "workspace".to_string(),
2206+
trigger: "before_worktree_switch".to_string(),
2207+
execution_target: "trigger_worktree".to_string(),
2208+
execution_mode: "inline".to_string(),
2209+
shell: "bash".to_string(),
2210+
script: "echo hi".to_string(),
2211+
critical: false,
2212+
switch_once_per_session: true,
2213+
keep_open_on_completion: false,
2214+
timeout_seconds: 60,
2215+
switch_run_on_create: true,
2216+
switch_run_on_delete: false,
2217+
};
2218+
2219+
assert!(!should_skip_switch_hook_once_per_session(
2220+
"before_worktree_switch",
2221+
&hook,
2222+
&context
2223+
));
2224+
mark_switch_hook_ran_this_session("before_worktree_switch", &hook.id, &context);
2225+
assert!(should_skip_switch_hook_once_per_session(
2226+
"before_worktree_switch",
2227+
&hook,
2228+
&context
2229+
));
2230+
2231+
if let Some(key) = build_switch_hook_session_key("before_worktree_switch", &hook.id, &context)
2232+
{
2233+
if let Ok(mut runs) = SWITCH_HOOK_SESSION_RUNS.lock() {
2234+
runs.remove(&key);
2235+
}
2236+
}
2237+
}
21392238
}

src-tauri/src/terminal.rs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::collections::HashMap;
22
use std::io::{BufRead, BufReader, Read, Write};
3-
use std::path::PathBuf;
3+
use std::path::{Path, PathBuf};
44
use std::process::Stdio;
55
use std::sync::{Arc, Mutex};
66
use std::time::{SystemTime, UNIX_EPOCH};
@@ -9,7 +9,7 @@ use portable_pty::{native_pty_system, Child, CommandBuilder, MasterPty, PtySize}
99
use tauri::{AppHandle, Emitter};
1010

1111
use crate::git::helpers::{
12-
command_exists, shell_candidates_for_current_os, validate_no_control_chars,
12+
command_exists, shell_candidates_for_current_os, strip_win_prefix, validate_no_control_chars,
1313
};
1414

1515
// ── Shell detection ──────────────────────────────────────────────────────────
@@ -114,6 +114,13 @@ fn validate_shell_for_terminal(shell: &str) -> Result<String, String> {
114114
Ok(s)
115115
}
116116

117+
fn normalize_session_cwd(cwd_path: &Path) -> PathBuf {
118+
cwd_path
119+
.canonicalize()
120+
.map(strip_win_prefix)
121+
.unwrap_or_else(|_| cwd_path.to_path_buf())
122+
}
123+
117124
fn validate_spawn_env_vars(
118125
env_vars: Option<HashMap<String, String>>,
119126
) -> Result<HashMap<String, String>, String> {
@@ -351,7 +358,7 @@ pub async fn spawn_terminal(
351358
master: Mutex::new(pair.master),
352359
writer: Mutex::new(writer),
353360
child: Mutex::new(child),
354-
initial_cwd: cwd_path.clone(),
361+
initial_cwd: normalize_session_cwd(&cwd_path),
355362
});
356363

357364
{

src/lib/components/TerminalDock.svelte

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
getTerminalShellOptions,
77
getWorkspaceTerminalSnapshots,
88
} from '$lib/workspace-terminals.svelte';
9+
import { pathsEqual } from '$lib/path-utils';
910
1011
const terminalRouteActive = $derived($page.url.pathname.startsWith('/workspace'));
1112
const activeWorkspacePath = $derived(getActiveWorkspacePath());
@@ -15,15 +16,15 @@
1516
terminalRouteActive
1617
? workspaceSnapshots.find(
1718
snapshot =>
18-
snapshot.workspacePath === activeWorkspacePath &&
19+
pathsEqual(snapshot.workspacePath, activeWorkspacePath) &&
1920
snapshot.activeTab === 'terminal' &&
2021
snapshot.initializedPaths.length > 0
2122
) ?? null
2223
: null
2324
);
2425
2526
function getSnapshotStyles(workspacePath: string, activeWorkspacePath: string | null) {
26-
return activeTerminalSnapshot && activeWorkspacePath === workspacePath ? 'flex' : 'none';
27+
return activeTerminalSnapshot && pathsEqual(activeWorkspacePath, workspacePath) ? 'flex' : 'none';
2728
}
2829
</script>
2930

@@ -39,7 +40,7 @@
3940
{#each workspaceState.initializedPaths as wtPath (wtPath)}
4041
<div
4142
class="flex min-h-0 flex-1 flex-col overflow-hidden"
42-
style:display={workspaceState.activeTerminalPath === wtPath ? 'flex' : 'none'}
43+
style:display={pathsEqual(workspaceState.activeTerminalPath, wtPath) ? 'flex' : 'none'}
4344
>
4445
<TerminalContainer
4546
defaultShell={shellOptions.defaultShell}
@@ -51,4 +52,4 @@
5152
{/each}
5253
</div>
5354
{/each}
54-
</div>
55+
</div>

src/lib/terminal-session-cache.svelte.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { pathKey } from '$lib/path-utils';
2+
13
type CachedSession = {
24
id: string;
35
shell: string;
@@ -38,13 +40,13 @@ function cloneState(state: CachedContainerState): CachedContainerState {
3840
}
3941

4042
export function getTerminalContainerCache(cwd: string): CachedContainerState {
41-
return cloneState(cacheByCwd.get(cwd) ?? EMPTY_STATE);
43+
return cloneState(cacheByCwd.get(pathKey(cwd)) ?? EMPTY_STATE);
4244
}
4345

4446
export function setTerminalContainerCache(cwd: string, nextState: CachedContainerState) {
45-
cacheByCwd.set(cwd, cloneState(nextState));
47+
cacheByCwd.set(pathKey(cwd), cloneState(nextState));
4648
}
4749

4850
export function clearTerminalContainerCache(cwd: string) {
49-
cacheByCwd.delete(cwd);
50-
}
51+
cacheByCwd.delete(pathKey(cwd));
52+
}

src/routes/workspace/+page.svelte

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1554,7 +1554,8 @@
15541554
}
15551555
15561556
// Ensure the shared terminal dock tracks this worktree path.
1557-
if (!terminalInitializedPaths.has(cwd)) {
1557+
const hasInitializedPath = [...terminalInitializedPaths].some(path => pathsEqual(path, cwd));
1558+
if (!hasInitializedPath) {
15581559
terminalInitializedPaths = new Set([...terminalInitializedPaths, cwd]);
15591560
}
15601561
@@ -1745,6 +1746,7 @@
17451746
worktreeChangeCounts = remainingChangeCounts;
17461747
17471748
const deletingActiveWorktree = !!activeWorktreePath && pathsEqual(activeWorktreePath, wt.path);
1749+
const activeWorktreePathBeforeDelete = activeWorktreePath;
17481750
const shouldRunDeleteSwitchHooks = deletingActiveWorktree && !!nextWorktreePath;
17491751
const operationTriggers: WorkspaceHookTrigger[] = shouldRunDeleteSwitchHooks
17501752
? [
@@ -1777,14 +1779,19 @@
17771779
await runWorktreeSwitchHooks(
17781780
workspace!.workspacePath,
17791781
nextWorktreePath,
1780-
activeWorktreePath ?? null,
1782+
activeWorktreePathBeforeDelete ?? null,
17811783
'delete'
17821784
);
17831785
}
17841786
17851787
clearTerminalStateForPath(wt.path);
17861788
await closeTerminalsForPath(wt.path);
1787-
await deleteManagedWorktree(workspace!.rootPath, wt.path, true, activeWorktreePath);
1789+
await deleteManagedWorktree(
1790+
workspace!.rootPath,
1791+
wt.path,
1792+
true,
1793+
activeWorktreePathBeforeDelete
1794+
);
17881795
toast.success(`Deleted worktree: ${label}`);
17891796
try {
17901797
await refreshWorkspaceData();

0 commit comments

Comments
 (0)