From eba84418dd6223f6639b1605e9a20878ea7793ec Mon Sep 17 00:00:00 2001 From: Xin Date: Wed, 16 Sep 2026 00:28:49 +0100 Subject: [PATCH 1/2] feat: inline edit for local diffs Uses the @pierre/diffs 1.4 edit-session API. Edit file from the file actions menu starts a session (local diffs only); Save writes the new file contents back to the worktree via PUT /api/worktree-file, Cancel reverts. Diff reloads are deferred while a session is active, and the n/p/m shortcuts are suppressed so they don't steal editor keystrokes. Local-mode hydration now reads the new side from the worktree, since the index-line oid of a worktree diff is generally not in the object database. --- src/git.rs | 43 ++++ src/server.rs | 29 ++- web/src/components/DiffView.tsx | 187 ++++++++++++++++-- .../components/diff-view/FileActionsMenu.tsx | 3 + 4 files changed, 242 insertions(+), 20 deletions(-) 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..e93038a 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( diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index eaa52c0..2b71123 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,100 @@ 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. + const canEditFile = useCallback( + (fileDiff: FileDiffMetadata): boolean => isLocal && fileDiff.type !== "deleted", + [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); + // updateItem ignores records whose version is unchanged; bump it. + viewer.updateItem({ ...item, 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 +1387,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 +1441,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 +1631,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} From 2d6bb50a7479556331ac93b0bec720106301f254 Mon Sep 17 00:00:00 2001 From: Xin Date: Wed, 23 Sep 2026 23:07:58 +0100 Subject: [PATCH 2/2] fix: refresh diffs after edits that restore HEAD; allow editing new files - watcher: also broadcast when a touched path was changed in the previous status but is clean now (e.g. saved back to its HEAD content), so the file drops out of the diff instead of lingering as an empty entry - edit: new-file diffs parse as partial and Pierre never hydrates them, so their edit session never started; mark them complete (with a fresh cacheKey) for the session since the patch already holds every line - edit: hide Edit file for pure renames, which have no rows to edit --- src/server.rs | 64 ++++++++++++++++++++++++++++++++- web/src/components/DiffView.tsx | 26 ++++++++++++-- 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/src/server.rs b/src/server.rs index e93038a..0421ca1 100644 --- a/src/server.rs +++ b/src/server.rs @@ -863,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; @@ -885,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 { @@ -1210,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 2b71123..e8331f4 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -1194,9 +1194,11 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" // 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. + // 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: FileDiffMetadata): boolean => + isLocal && fileDiff.type !== "deleted" && fileDiff.type !== "rename-pure", [isLocal], ); @@ -1207,8 +1209,26 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" 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, edit: true, version: (item.version ?? 0) + 1 }); + viewer.updateItem({ + ...item, + ...(fileDiff ? { fileDiff } : {}), + edit: true, + version: (item.version ?? 0) + 1, + }); }, []); const finishEditingFile = useCallback((itemId: string, decision: "accept" | "reject") => {