fix(diff-viewer): keep trailing deleted lines reachable by scroll, scrollbar, and keyboard

apps/staged/src-tauri/src/managed_acp_tools.rs

@@ -1523,16 +1523,22 @@ mod tests {
     }
 
     /// Writes `contents` to `path` as a 0755 script and returns only once it
-    /// can actually be exec'd. On Linux, a test on another thread that forks
-    /// while our write handle is open inherits it until its own exec, and
-    /// exec of a file anyone still holds open for writing fails with
-    /// `ETXTBSY` — which `npm_view_version` would faithfully report as
-    /// "unknown" and fail the test. Probe-spawn (and immediately kill) the
-    /// script until that window has passed.
+    /// can actually be exec'd (see [`wait_until_executable`]).
     fn write_executable(path: &Path, contents: &str) {
         std::fs::write(path, contents).unwrap();
         use std::os::unix::fs::PermissionsExt;
         std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap();
+        wait_until_executable(path);
+    }
+
+    /// Returns only once the freshly written script at `path` can actually be
+    /// exec'd. On Linux, a test on another thread that forks while our write
+    /// handle is open inherits it until its own exec, and exec of a file
+    /// anyone still holds open for writing fails with `ETXTBSY` — which the
+    /// code under test would faithfully report and fail the test. Probe-spawn
+    /// (and immediately kill) the script until that window has passed, so the
+    /// script must be harmless to run with no arguments.
+    fn wait_until_executable(path: &Path) {
         for _ in 0..1000 {
             match std::process::Command::new(path)
                 .stdin(std::process::Stdio::null())

apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs

@@ -3,6 +3,15 @@ use super::*;
 const TIMEOUT: Duration = super::super::validation::VALIDATION_TIMEOUT;
 const MISSING_NATIVE: &str = "echo 'native binary not found for this platform' >&2\nexit 1\n";
 
+/// Installs `contents` as the fake `node` under `bin_dir`, through the
+/// production shim writer, and waits out the `ETXTBSY` window before the
+/// validation probe execs it. A busy exec would otherwise fail validation
+/// with `could not start: Text file busy`.
+fn write_node_shim(bin_dir: &Path, contents: &str) {
+    write_shim(bin_dir, "node", contents).unwrap();
+    wait_until_executable(&bin_dir.join("node"));
+}
+
 fn write_probe(node: &Path, template: &Path, tool: &ManagedTool, script: &str) {
     write_fixture_install(template, tool, "1.2.4");
     std::fs::write(npm_entrypoint(template, tool.package), script).unwrap();
@@ -14,15 +23,14 @@ fn write_probe(node: &Path, template: &Path, tool: &ManagedTool, script: &str) {
         "codex-acp" => "[ \"$2\" = cli ] && [ \"$3\" = -V ]",
         _ => panic!("missing probe fixture for {}", tool.id),
     };
-    write_shim(
+    // With no arguments this exits 91 at once, so it is safe to probe-spawn.
+    write_node_shim(
         &node.join("bin"),
-        "node",
         &format!(
             "#!/bin/sh\ncase \"$1\" in /*) ;; *) exit 91 ;; esac\n[ \"$1\" -ef \"$PWD/node_modules/{}/dist/index.js\" ] || exit 91\n{args_check} && [ $# = 3 ] || exit 92\nexec /bin/sh \"$@\"\n",
             tool.package
         ),
-    )
-    .unwrap();
+    );
 }
 
 #[tokio::test]
@@ -113,12 +121,7 @@ async fn rejected_upgrade(
     let live = tool_install_dir(&root, tool.id);
     let entrypoint = npm_entrypoint(&live, tool.package);
     std::fs::write(&entrypoint, "echo '1.2.3'\n").unwrap();
-    write_shim(
-        &old_node.join("bin"),
-        "node",
-        "#!/bin/sh\nexec /bin/sh \"$@\"\n",
-    )
-    .unwrap();
+    write_node_shim(&old_node.join("bin"), "#!/bin/sh\nexec /bin/sh \"$@\"\n");
     let shim = shim_bin_dir(&root).join(tool.binary);
     let shim_before = std::fs::read(&shim).unwrap();
     let state_before = std::fs::read(state_path(&root)).unwrap();
@@ -245,7 +248,10 @@ async fn personal_overrides_cannot_mask_missing_native_binary() {
 const HANGING_NATIVE: &str =
     "/bin/sleep 60 &\necho \"$$ $!\" > ../../validation-pids\necho 'waiting for native CLI' >&2\n";
 
-async fn probe_pids(root: &Path) -> Vec<i32> {
+/// The bridge and native-child PIDs the hanging probe records, or `None` if
+/// it has not recorded them within 5 seconds. Callers attach whatever they
+/// know about why validation never got that far.
+async fn probe_pids(root: &Path) -> Option<Vec<i32>> {
     tokio::time::timeout(Duration::from_secs(5), async {
         loop {
             if let Ok(contents) = tokio::fs::read_to_string(root.join("validation-pids")).await {
@@ -261,7 +267,7 @@ async fn probe_pids(root: &Path) -> Vec<i32> {
         }
     })
     .await
-    .expect("probe did not spawn its native child")
+    .ok()
 }
 
 async fn assert_probe_tree_stopped(pids: &[i32]) {
@@ -300,7 +306,9 @@ async fn timeout_preserves_install_and_kills_tree(bridge_exit: &str) {
             Duration::from_secs(1),
         )
         .await;
-        let pids = probe_pids(&dir.path().join("packages")).await;
+        let pids = probe_pids(&dir.path().join("packages"))
+            .await
+            .unwrap_or_else(|| panic!("probe did not spawn its native child: {error}"));
         assert_probe_tree_stopped(&pids).await;
         assert!(error.contains("timed out after 1 seconds"), "{error}");
         assert!(error.contains("waiting for native CLI"), "{error}");
@@ -326,10 +334,17 @@ async fn cancellation_kills_bridge_and_native_child() {
         let node = root.join("node");
         let staging = staging_install_dir(&root, tool.id);
         write_probe(&node, &staging, &tool, &format!("{HANGING_NATIVE}wait\n"));
-        let probe = tokio::spawn(async move {
+        let mut probe = tokio::spawn(async move {
             super::super::validation::validate_staged_tool(&node, &staging, &tool, TIMEOUT).await
         });
-        let pids = probe_pids(&root).await;
+        // Validation should hang until aborted. If it returns first (say, the
+        // node shim failed to start), report why instead of a bare timeout.
+        let pids = tokio::select! {
+            pids = probe_pids(&root) => pids.expect("probe did not spawn its native child"),
+            result = &mut probe => {
+                panic!("validation finished before spawning its native child: {result:?}")
+            }
+        };
         probe.abort();
         assert!(probe.await.unwrap_err().is_cancelled());
         assert_probe_tree_stopped(&pids).await;

packages/diff-viewer/src/lib/components/DiffViewer.svelte

@@ -57,6 +57,7 @@
   import type { SearchState, FileSearchResult } from '../state/searchState.svelte';
   import type { SearchMatch, MatchLocation } from '../utils/diffSearch';
   import {
+    anchorSideForHunk,
     buildAfterMarkers,
     buildBeforeMarkers,
     buildLineToAlignmentMap,
@@ -302,6 +303,8 @@
   let isNewFile = $derived(diff !== null && !isEmptyDiff && diff.before === null);
   let isDeletedFile = $derived(diff !== null && !isEmptyDiff && diff.after === null);
   let isTwoPaneMode = $derived(!isNewFile && !isDeletedFile);
+  /** The pane that exists for every file: only a deleted file has no after pane. */
+  const renderedPaneSide = (): 'before' | 'after' => (isDeletedFile ? 'before' : 'after');
   let isBinary = $derived(diff !== null && isBinaryDiff(diff));
   let isImage = $derived(diff !== null && isImageDiff(diff));
 
@@ -473,14 +476,16 @@
     // Wait for next frame to ensure dimensions are set
     requestAnimationFrame(() => {
       const firstHunk = changedAlignments[0].alignment;
-      // Scroll to first change in the after pane (or before pane for deleted files)
-      const side = isDeletedFile ? 'before' : 'after';
-      const startRow = side === 'before' ? firstHunk.before.start : firstHunk.after.start;
-      scrollController.scrollToRow(startRow, side);
+      // Scroll to the first change, anchored on the pane that has its rows
+      // (pure deletions, including whole deleted files, only exist on the before side).
+      const side = anchorSideForHunk(firstHunk);
+      scrollController.scrollToRow(firstHunk[side].start, side);
     });
   });
 
-  // Update dimensions when panes are available or content changes
+  // Update dimensions when panes are available or content changes. A pane that
+  // is not rendered (created/deleted files) or has no lines is cleared so the
+  // other pane's effective extent does not inherit a previous file's geometry.
   $effect(() => {
     if (beforePane && beforeLines.length > 0) {
       const lineHeight = measureLineHeight(beforePane);
@@ -494,6 +499,8 @@
         viewportWidth: beforePane.clientWidth,
         contentWidth,
       });
+    } else {
+      scrollController.clearDimensions('before');
     }
   });
 
@@ -510,6 +517,8 @@
         viewportWidth: afterPane.clientWidth,
         contentWidth,
       });
+    } else {
+      scrollController.clearDimensions('after');
     }
   });
 
@@ -531,6 +540,12 @@
   let beforeContentHeight = $state(0);
   let afterContentHeight = $state(0);
 
+  // Scrollbars run over each pane's effective extent (own range plus the edge
+  // handoff headroom) so rows that exist on one side alone stay reachable by
+  // thumb drag and track click, not just by wheel.
+  let beforeScrollExtent = $derived(scrollController.getEffectiveExtent('before'));
+  let afterScrollExtent = $derived(scrollController.getEffectiveExtent('after'));
+
   // Reactive line/viewport metrics that drive the rendered window (Phase 2
   // virtualization). Kept in sync wherever dimensions are measured below so the
   // window deriveds recompute on resize and font changes, not just on scroll.
@@ -1219,22 +1234,32 @@
     handleWheel('after', e);
   }
 
-  function handleBeforeScrollbarScroll(deltaY: number) {
-    scrollController.scrollBy('before', deltaY);
+  function syncOverlaysAfterScrollbar() {
     redrawConnectorsImpl();
     updateToolbarPosition();
     updateCommentEditorPosition();
     updateLineSelectionToolbar();
     updateLineCommentEditorPosition();
   }
 
+  function handleBeforeScrollbarScroll(deltaY: number) {
+    scrollController.scrollBy('before', deltaY);
+    syncOverlaysAfterScrollbar();
+  }
+
+  function handleBeforeScrollbarScrollTo(scrollY: number) {
+    scrollController.scrollToEffective('before', scrollY);
+    syncOverlaysAfterScrollbar();
+  }
+
   function handleAfterScrollbarScroll(deltaY: number) {
     scrollController.scrollBy('after', deltaY);
-    redrawConnectorsImpl();
-    updateToolbarPosition();
-    updateCommentEditorPosition();
-    updateLineSelectionToolbar();
-    updateLineCommentEditorPosition();
+    syncOverlaysAfterScrollbar();
+  }
+
+  function handleAfterScrollbarScrollTo(scrollY: number) {
+    scrollController.scrollToEffective('after', scrollY);
+    syncOverlaysAfterScrollbar();
   }
 
   function handleHorizontalScrollbarScroll(deltaX: number) {
@@ -2169,8 +2194,10 @@
       scrollToRow: (row, side) => scrollController.scrollToRow(row, side),
       scrollBy: (deltaY) => scrollController.scrollBy('after', deltaY),
       getCurrentScrollY: () => scrollController.afterScrollY,
-      getLineHeight: () => scrollController.getDimensions('after').lineHeight,
-      getViewportHeight: () => scrollController.getDimensions('after').viewportHeight,
+      getBeforeScrollY: () => scrollController.beforeScrollY,
+      // Both panes share a viewport height; read the one that is rendered.
+      getLineHeight: () => scrollController.getDimensions(renderedPaneSide()).lineHeight,
+      getViewportHeight: () => scrollController.getDimensions(renderedPaneSide()).viewportHeight,
       startCommentOnHunk: (hunkIndex) => {
         if (!commentingEnabled) return;
         commentingOnRange = hunkIndex;
@@ -2180,6 +2207,7 @@
       onHunkFocus: (hunkIndex) => {
         focusedHunkIndex = hunkIndex;
       },
+      getFocusedHunkIndex: () => focusedHunkIndex,
     });
 
     document.addEventListener('copy', handleCopy);
@@ -2309,11 +2337,13 @@
               </div>
             {:else}
               <Scrollbar
-                scrollY={scrollController.beforeScrollY}
-                contentHeight={beforeContentHeight}
+                scrollY={beforeScrollExtent.scrollY}
+                contentHeight={beforeScrollExtent.contentHeight}
                 viewportHeight={beforePane?.clientHeight ?? 0}
+                markerRange={{ top: beforeScrollExtent.contentOffset, height: beforeContentHeight }}
                 side="left"
                 onScroll={handleBeforeScrollbarScroll}
+                onScrollTo={handleBeforeScrollbarScrollTo}
                 markers={beforeMarkers}
               />
               <div class="code-container" bind:this={beforePane}>
@@ -2406,11 +2436,13 @@
           </div>
           <div class="code-area" onwheel={handleBeforeWheel}>
             <Scrollbar
-              scrollY={scrollController.beforeScrollY}
-              contentHeight={beforeContentHeight}
+              scrollY={beforeScrollExtent.scrollY}
+              contentHeight={beforeScrollExtent.contentHeight}
               viewportHeight={beforePane?.clientHeight ?? 0}
+              markerRange={{ top: beforeScrollExtent.contentOffset, height: beforeContentHeight }}
               side="left"
               onScroll={handleBeforeScrollbarScroll}
+              onScrollTo={handleBeforeScrollbarScrollTo}
               markers={[]}
             />
             <div class="code-container" bind:this={beforePane}>
@@ -2574,11 +2606,13 @@
                 {/if}
               </div>
               <Scrollbar
-                scrollY={scrollController.afterScrollY}
-                contentHeight={afterContentHeight}
+                scrollY={afterScrollExtent.scrollY}
+                contentHeight={afterScrollExtent.contentHeight}
                 viewportHeight={afterPane?.clientHeight ?? 0}
+                markerRange={{ top: afterScrollExtent.contentOffset, height: afterContentHeight }}
                 side="right"
                 onScroll={handleAfterScrollbarScroll}
+                onScrollTo={handleAfterScrollbarScrollTo}
                 markers={afterMarkers}
               />
               <HorizontalScrollbar
@@ -2655,11 +2689,13 @@
               {/if}
             </div>
             <Scrollbar
-              scrollY={scrollController.afterScrollY}
-              contentHeight={afterContentHeight}
+              scrollY={afterScrollExtent.scrollY}
+              contentHeight={afterScrollExtent.contentHeight}
               viewportHeight={afterPane?.clientHeight ?? 0}
+              markerRange={{ top: afterScrollExtent.contentOffset, height: afterContentHeight }}
               side="right"
               onScroll={handleAfterScrollbarScroll}
+              onScrollTo={handleAfterScrollbarScrollTo}
               markers={[]}
             />
             <HorizontalScrollbar