diff --git a/src/git.rs b/src/git.rs index f9e072b..deef451 100644 --- a/src/git.rs +++ b/src/git.rs @@ -441,6 +441,28 @@ pub fn read_worktree_file(cwd: impl AsRef, rel_path: &str) -> Result, rel_path: &str, contents: &[u8]) -> Result<()> { + if !is_safe_repo_path(rel_path) { + return Err(GitError::InvalidRepoPath); + } + let root = root(cwd)?; + let canonical_root = root.canonicalize().map_err(|_| GitError::NoWorkdir)?; + let candidate = root.join(rel_path); + let canonical = candidate + .canonicalize() + .map_err(|_| GitError::FileNotFound)?; + if !canonical.starts_with(&canonical_root) { + return Err(GitError::InvalidRepoPath); + } + if !canonical.is_file() { + return Err(GitError::FileNotFound); + } + std::fs::write(&canonical, contents).map_err(|_| GitError::FileNotFound) +} + #[cfg(test)] mod tests { use super::*; @@ -675,6 +697,27 @@ mod tests { )); } + #[test] + fn write_worktree_file_overwrites_existing_files_only() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + git(&root, &["init", "-b", "main"]); + fs::write(root.join("existing.txt"), "before\n").unwrap(); + + write_worktree_file(&root, "existing.txt", b"after\n").unwrap(); + assert_eq!(fs::read(root.join("existing.txt")).unwrap(), b"after\n"); + + assert!(matches!( + write_worktree_file(&root, "created.txt", b"nope"), + Err(GitError::FileNotFound) + )); + assert!(!root.join("created.txt").exists()); + assert!(matches!( + write_worktree_file(&root, "../outside.txt", b"nope"), + Err(GitError::InvalidRepoPath) + )); + } + #[test] fn read_worktree_file_rejects_symlink_escape() { let outside = tempfile::tempdir().unwrap(); diff --git a/src/server.rs b/src/server.rs index 35cebb0..0421ca1 100644 --- a/src/server.rs +++ b/src/server.rs @@ -13,7 +13,7 @@ use axum::{ IntoResponse, Response, Sse, sse::{Event, KeepAlive}, }, - routing::{delete, get, post}, + routing::{delete, get, post, put}, }; use notify::{RecursiveMode, Watcher}; use serde::{Deserialize, Serialize}; @@ -119,6 +119,12 @@ struct PullFileQuery { side: Option, } +#[derive(Debug, Deserialize)] +struct WorktreeWriteBody { + path: String, + contents: String, +} + pub struct RunningServer { pub router: Router, _watcher: Option, @@ -183,6 +189,7 @@ pub fn new(cfg: ServerConfig) -> anyhow::Result { ) .route("/api/patch/{org}/{repo}/{number}", get(handle_patch)) .route("/api/blob", get(handle_blob)) + .route("/api/worktree-file", put(handle_write_worktree_file)) .route( "/api/pull/{org}/{repo}/{number}/file", get(handle_pull_file), @@ -557,6 +564,26 @@ async fn handle_blob(State(state): State, Query(query): Query, + Json(body): Json, +) -> Response { + let path = body.path.trim(); + if path.is_empty() { + return error(StatusCode::BAD_REQUEST, "path is required"); + } + if body.contents.len() > MAX_BLOB_BYTES { + return error(StatusCode::PAYLOAD_TOO_LARGE, "contents too large"); + } + match git::write_worktree_file(&state.cwd, path, body.contents.as_bytes()) { + Ok(()) => StatusCode::NO_CONTENT.into_response(), + Err(err) => blob_error(err), + } +} + /// Fetches a single file's raw content from one side of a pull request, for /// `loadDiffFiles` hydration of PR diffs. async fn handle_pull_file( @@ -836,6 +863,10 @@ fn start_watcher( std::thread::spawn(move || { // Repository handle for `git status` lookups; lives only on this thread. let repo = git::discover(&status_cwd).ok(); + // Status as of the previous tick. A touched path that was changed then + // but is clean now (e.g. saved back to its HEAD content) drops out of + // the current status, yet its removal still changes the diff. + let mut last_status = repo.as_ref().and_then(|repo| git::status_map(repo).ok()); loop { let mut pending: BTreeSet = BTreeSet::new(); let mut git_state = false; @@ -858,7 +889,11 @@ fn start_watcher( Some(map) => changed_files_for_events(&pending, map), None => changed_files_from_events(&pending), }; - let broadcast = !changed.is_empty() || git_state; + let reverted = last_status + .as_ref() + .is_some_and(|prev| !changed_files_for_events(&pending, prev).is_empty()); + last_status = status; + let broadcast = !changed.is_empty() || reverted || git_state; if broadcast { let _ = events.send(()); if let Some(on_change) = &on_change { @@ -1183,6 +1218,60 @@ mod tests { assert!(!is_structurally_ignored(FsPath::new("/repo/my.git/x"))); } + #[test] + fn watcher_broadcasts_when_file_returns_to_head() { + use std::process::Command; + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + let git = |args: &[&str]| { + let ok = Command::new("git") + .args(args) + .current_dir(&root) + .status() + .unwrap() + .success(); + assert!(ok, "git {args:?} failed"); + }; + git(&["init", "-q", "-b", "main"]); + std::fs::write(root.join("a.txt"), "base\n").unwrap(); + git(&["add", "a.txt"]); + git(&[ + "-c", + "user.email=t@t", + "-c", + "user.name=t", + "commit", + "-qm", + "init", + ]); + + let (events, mut rx) = broadcast::channel(16); + let _watcher = start_watcher(root.clone(), events, None).unwrap(); + let expect_broadcast = |rx: &mut broadcast::Receiver<()>, what: &str| { + let deadline = std::time::Instant::now() + Duration::from_secs(10); + loop { + match rx.try_recv() { + Ok(()) => break, + Err(broadcast::error::TryRecvError::Empty) + if std::time::Instant::now() < deadline => + { + std::thread::sleep(Duration::from_millis(20)) + } + Err(err) => panic!("no broadcast after {what}: {err:?}"), + } + } + // Let the burst settle and drop duplicates before the next step. + std::thread::sleep(WATCH_DEBOUNCE * 4); + while rx.try_recv().is_ok() {} + }; + + std::fs::write(root.join("a.txt"), "edited\n").unwrap(); + expect_broadcast(&mut rx, "modifying a.txt"); + // Back to HEAD content: a.txt leaves `git status`, but the diff changed. + std::fs::write(root.join("a.txt"), "base\n").unwrap(); + expect_broadcast(&mut rx, "restoring a.txt to HEAD"); + } + #[test] fn changed_files_for_events_intersects_status() { let mut status = BTreeMap::new(); diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index eaa52c0..e8331f4 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -20,7 +20,14 @@ import { type FileDiffMetadata, type SelectedLineRange, } from "@pierre/diffs"; -import { CodeView, type CodeViewHandle, useWorkerPool } from "@pierre/diffs/react"; +import { + CodeView, + type CodeViewHandle, + type CodeViewItemEditCompleteHandler, + EditProvider, + useWorkerPool, +} from "@pierre/diffs/react"; +import { Editor, type EditorFactory, type FileDiffEditCompleteEvent } from "@pierre/diffs/edit"; import { applyColorScheme, initialColorScheme, @@ -39,7 +46,7 @@ import { IconExternalLink, IconFileX, } from "@tabler/icons-react"; -import { buttonVariants } from "@/components/ui/button"; +import { Button, buttonVariants } from "@/components/ui/button"; import { Empty, EmptyContent, @@ -379,6 +386,13 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" } | null>(null); const repoContextRequested = useRef(false); const viewerRef = useRef | null>(null); + const [editingItemId, setEditingItemId] = useState(null); + // Mirrors editingItemId for callbacks that must not re-subscribe (SSE load). + const editingItemIdRef = useRef(null); + // Set by Save/Cancel just before toggling edit off; read in onItemEditComplete. + const editDecisionRef = useRef<"accept" | "reject">("reject"); + const pendingDiffReloadRef = useRef(false); + const reloadDiffRef = useRef<(() => void) | null>(null); const codeViewAreaRef = useRef(null); const currentFileRef = useRef(null); const programmaticScrollAtRef = useRef(0); @@ -497,6 +511,12 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" let fallbackInterval: number | undefined; const load = () => { + // A reload replaces the patch and remounts CodeView, which would tear + // down an active edit session; defer until the session completes. + if (editingItemIdRef.current != null) { + pendingDiffReloadRef.current = true; + return; + } const endpoint = isBranch ? `/api/branch-diff?base=${encodeURIComponent(baseRef)}${includeDirty ? "&dirty=1" : ""}` : isLocal @@ -527,6 +547,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" return; } if (!usesLocalStore && (!org || !repo || !number)) return; + reloadDiffRef.current = load; load(); if (usesLocalStore) { eventSource = new EventSource("/api/events"); @@ -901,6 +922,9 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" ]); const onKeyDown = useEffectEvent((e: KeyboardEvent) => { if (e.metaKey || e.ctrlKey || e.altKey) return; + // The inline editor's input lives in shadow DOM, so `target` is the + // shadow host, not an editable element; never steal keys mid-edit. + if (editingItemIdRef.current != null) return; const target = e.target as HTMLElement | null; if (target && (target.isContentEditable || EDITABLE_TAGS.test(target.tagName))) { return; @@ -1153,13 +1177,120 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" } const [oldFile, newFile] = await Promise.all([ loadBlobFileContents(prevObjectId, oldName), - newObjectId && !isZeroOid(newObjectId) - ? loadBlobFileContents(newObjectId, newName) - : loadWorktreeFileContents(newName), + // In local mode the new side is the working tree itself; its `index` + // oid is computed on the fly and generally absent from the object + // database, so read the file instead of the blob. + isLocal && fileDiff.type !== "deleted" + ? loadWorktreeFileContents(newName) + : newObjectId && !isZeroOid(newObjectId) + ? loadBlobFileContents(newObjectId, newName) + : loadWorktreeFileContents(newName), ]); return { oldFile, newFile }; }, - [usesLocalStore, org, repo, number], + [usesLocalStore, isLocal, org, repo, number], + ); + + // Inline editing targets the worktree. The local diff is HEAD → worktree + // (see git::local_diff), so the new side of every non-deleted file is the + // working-tree file itself; PR and branch diffs review committed states and + // stay read-only. Pure renames have no hunks, so the diff view has no rows + // to edit. + const canEditFile = useCallback( + (fileDiff: FileDiffMetadata): boolean => + isLocal && fileDiff.type !== "deleted" && fileDiff.type !== "rename-pure", + [isLocal], + ); + + const startEditingFile = useCallback((itemId: string) => { + const viewer = viewerRef.current; + const item = viewer?.getItem(itemId); + if (!viewer || !item) return; + editDecisionRef.current = "reject"; + editingItemIdRef.current = itemId; + setEditingItemId(itemId); + // Edit sessions need a fully hydrated diff, but Pierre only hydrates + // change/rename diffs. A new file's patch already holds every line, so + // mark it complete for the session instead of leaving it stuck partial. + // The copy needs its own cacheKey: Pierre treats diffs with equal keys as + // the same target and would keep the partial one. + const fileDiff = + item.type === "diff" && item.fileDiff.type === "new" && item.fileDiff.isPartial + ? { + ...item.fileDiff, + isPartial: false, + cacheKey: `${item.fileDiff.cacheKey ?? item.id}:complete`, + } + : undefined; + // updateItem ignores records whose version is unchanged; bump it. + viewer.updateItem({ + ...item, + ...(fileDiff ? { fileDiff } : {}), + edit: true, + version: (item.version ?? 0) + 1, + }); + }, []); + + const finishEditingFile = useCallback((itemId: string, decision: "accept" | "reject") => { + editDecisionRef.current = decision; + const viewer = viewerRef.current; + const item = viewer?.getItem(itemId); + if (viewer && item) { + // Toggling edit off ends the session; handleItemEditComplete decides. + viewer.updateItem({ ...item, edit: false, version: (item.version ?? 0) + 1 }); + } else { + editingItemIdRef.current = null; + setEditingItemId(null); + } + }, []); + + const saveWorktreeFile = useCallback(async (path: string, contents: string) => { + try { + await apiFetch("/api/worktree-file", { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ path, contents }), + }); + } catch (err) { + console.error(`Failed to save ${path}:`, err); + // Re-sync the view with the worktree, which still has the old content. + reloadDiffRef.current?.(); + } + }, []); + + const handleItemEditComplete = useCallback< + CodeViewItemEditCompleteHandler + >( + (event, item) => { + if (editingItemIdRef.current === item.id) { + editingItemIdRef.current = null; + setEditingItemId(null); + } + const decision = editDecisionRef.current; + editDecisionRef.current = "reject"; + const hadPendingReload = pendingDiffReloadRef.current; + pendingDiffReloadRef.current = false; + // On accept the save itself triggers a watcher reload, so a deferred + // reload only needs to run explicitly on the reject paths. + const reject = () => { + if (hadPendingReload) reloadDiffRef.current?.(); + return "reject" as const; + }; + if (decision !== "accept" || item.type !== "diff") return reject(); + const completed = event as FileDiffEditCompleteEvent; + if (!completed.newFile) return reject(); + // The event is frozen; re-key the accepted diff in place (per the API + // contract) so keyed render caching doesn't serve the replaced value. + completed.fileDiff.cacheKey = `edited:${item.id}:${Date.now()}`; + void saveWorktreeFile(completed.fileDiff.name, completed.newFile.contents); + return "accept"; + }, + [saveWorktreeFile], + ); + + const createEditor = useCallback>( + (editorType, options, editStateKey) => new Editor(editorType, options, editStateKey), + [], ); const codeViewOptions = useMemo( @@ -1276,6 +1407,27 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" const renderHeaderMetadata = useCallback( (item: CodeViewItem) => { if (item.type !== "diff" || !item.fileDiff) return null; + if (editingItemId === item.id) { + return ( +
e.stopPropagation()} + > + + +
+ ); + } const sig = fileSignatures.get(item.fileDiff.name); const isReviewed = sig != null && reviewed.map.get(item.fileDiff.name) === sig; return ( @@ -1309,11 +1461,25 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" startEditingFile(item.id) + : undefined + } /> ); }, - [fileSignatures, reviewed, toggleReviewed, filePatchSections], + [ + fileSignatures, + reviewed, + toggleReviewed, + filePatchSections, + editingItemId, + canEditFile, + startEditingFile, + finishEditingFile, + ], ); if (loading) { @@ -1485,18 +1651,21 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" ) : ( - - key={codeViewKey} - ref={viewerRef} - initialItems={initialItems} - selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} - style={codeViewStyle} - options={codeViewOptions} - renderAnnotation={renderAnnotation} - renderHeaderPrefix={renderHeaderPrefix} - renderHeaderMetadata={renderHeaderMetadata} - /> + + + key={codeViewKey} + ref={viewerRef} + initialItems={initialItems} + selectedLines={selectedLines} + onSelectedLinesChange={setSelectedLines} + style={codeViewStyle} + options={codeViewOptions} + renderAnnotation={renderAnnotation} + renderHeaderPrefix={renderHeaderPrefix} + renderHeaderMetadata={renderHeaderMetadata} + onItemEditComplete={handleItemEditComplete} + /> + )} diff --git a/web/src/components/diff-view/FileActionsMenu.tsx b/web/src/components/diff-view/FileActionsMenu.tsx index 31fda36..9f08d61 100644 --- a/web/src/components/diff-view/FileActionsMenu.tsx +++ b/web/src/components/diff-view/FileActionsMenu.tsx @@ -21,9 +21,11 @@ interface CopyAction { export function FileActionsMenu({ path, diffText, + onEdit, }: { path: string; diffText: string | undefined; + onEdit?: () => void; }) { const [copiedKey, setCopiedKey] = useState(null); const resetTimer = useRef(0); @@ -81,6 +83,7 @@ export function FileActionsMenu({ ); })} + {onEdit && Edit file}