Skip to content

feat(panel): read-only diff pop-out from Accomplished rows - #20

Merged
SuperLogicAI merged 2 commits into
SuperLogicAI:mainfrom
zeiddata-dev:feat/diff-popout
Sep 9, 2026
Merged

feat(panel): read-only diff pop-out from Accomplished rows#20
SuperLogicAI merged 2 commits into
SuperLogicAI:mainfrom
zeiddata-dev:feat/diff-popout

Conversation

@zeiddata-dev

Copy link
Copy Markdown
Contributor

Closes #10.

Clicking a Wrote/Edited row in the Accomplished panel now opens a read-only pop-out with that file's unified diff.

Read this first: staged-only is a real limit

The issue anticipated it —

git_diff_cached shows staged changes; a file the agent touched may be unstaged... Decide what a row with no available diff does — an empty state saying so is a fine answer, a crash isn't.

— so this PR does exactly that. But it's worth being blunt about how often the empty state wins: an agent that edits a file doesn't stage it, so unless the user has run git add themselves, the common path is the empty state rather than a diff. The plumbing is right; the only available source is usually dry.

git diff HEAD -- <path> would fix it, at the cost of a new Tauri command — scope this issue didn't ask for. Happy to add it here or as a follow-up, whichever you prefer. Flagging it rather than quietly shipping a feature that mostly shows an empty box.

Bridging the signature mismatch

src-tauri/ is untouched — no Rust change, no new Tauri command. The whole bridge is src/lib/diff.ts:

git_diff_cached(cwd) returns a directory's entire staged diff, while a row carries one file path that may live in a different repo than the tab's cwd. So:

  1. dirOf(file_path) gives the row's own parent directory (both separators), and the existing command runs there. git diff --cached ignores the subdirectory for scoping, so this returns the staged diff of the repo that file actually lives in — which solves the different-repo half for free.
  2. extractFileDiff slices out that one file's diff --git section. Split on /^(?=diff --git )/m, so a +diff --git line inside hunk content can't fabricate a section; collect paths from --- a/ / +++ b/, falling back to the header for hunkless sections like a pure rename.
  3. Diff paths are repo-relative and the row's path is absolute, so they're matched by separator-anchored suffixsrc/b/x.ts can never answer for a row about src/a/x.ts. /dev/null never matches; renames match on either side.
  4. loadFileDiff retries once against the tab's cwd when the file's own directory is gone (pruned worktree, deleted dir) but the tab's repo still holds the staged change.

The path reaches git as a Command::arg, never a shell string.

The no-diff case

An empty state worded to name the constraint rather than look broken:

No staged diff for this file. The app can only read staged changes (git diff --cached), so an edit the agent hasn't staged — or a file outside this project's repo — shows nothing here.

Nothing is staged on the user's behalf. That would mutate the index behind a human who may be mid-commit, and the Commit & Push footer's git_add_u/git_add_all are explicitly user-initiated. Read-only stays read-only.

Every failure path — git absent, directory gone, path unparseable — resolves to that same empty state. The lookup is .catch(() => "") at both the invoke and the effect, so nothing can throw into the panel tree, and neither the surrounding panel nor the terminals are affected either way.

Scope

  • DiffModal.tsx mirrors LandingNoteModal/IsolateLoopModal exactly — same overlay geometry (the top-7 titlebar-drag comment preserved), same card classes, overlay-click dismiss plus the window Esc handler, same footer button. Emerald to match the Accomplished section.
  • Raw unified diff in a <pre>, monospace. No highlighting, no colorization, no new dependency.
  • Which rows are clickable is decided in the logic layer, not the component: listToolEvents sets ToolEvent.filePath only for tools that write the file they name (Edit/Write/NotebookEdit/MultiEdit), so Read/Grep/Bash rows stay inert and the panel stays dumb. Row text is unchanged.
  • No schema change, no migration, no npm dependency. The editable code view stays rejected per docs/IDEAS.md.

Verification

  • npx tsc --noEmit — clean.
  • npm run check — 13/13.
  • npm run goldennot run; no extraction prompt touched.

New scripts/diff-check.ts + npm run diff:check, wired into the aggregate npm run check so CI gates it without a ci.yml edit. It covers dirOf on posix/Windows/bare paths, and extractFileDiff on single-section extraction, no bleed into neighbours, the same-basename-in-a-sibling-directory non-match, unstaged → "", empty diff → "", empty path → "", a backslashed Windows path against git's forward slashes, new file with --- /dev/null, a hunkless rename on both sides, and a fake diff --git line inside hunk content.

Draft, because nothing was clicked. This was written on Windows and the app is macOS-only, so the issue's two behavioural criteria — clicking a file row shows its diff and a row with no diff shows an empty state — are not verified. Neither is the modal's visual result: layout, wide-diff overflow, and scroll behaviour at max-h-[80vh] have never been seen. The dir-argument behaviour is reasoned from git semantics, not observed.

Manual steps are in docs/TESTING.md § 26, in that file's style with every box unchecked.

Reading what an agent actually changed meant leaving the app: the
Accomplished panel names the file ("Edited src/bar.rs") but not the change.
Clicking a Wrote/Edited row now opens a read-only pop-out with that file's
unified diff.

The two halves already existed but did not line up: `git_diff_cached(cwd)`
returns a whole directory's staged diff, while a row carries one file path
that may live in a different repo than the tab's cwd. Bridged entirely in
TypeScript, with no new Tauri command: run the existing command in the
file's own directory, then slice that file's section out of the unified
diff in `src/lib/diff.ts`. Diff paths are repo-relative and the row's path
is absolute, so they are matched by separator-anchored suffix — src/b/x.ts
never answers for a row about src/a/x.ts.

Staged-only is a real limit, and it is surfaced rather than worked around:
nothing is staged on the user's behalf (that would mutate the repo behind a
human who is mid-commit), so an unstaged edit, or a file outside any repo
git can see, renders an empty state naming the constraint. Every failure
path — git absent, directory gone, path unparseable — resolves to that same
empty state; the lookup can never throw into the panel tree.

Which rows are diffable is decided in `listToolEvents`, not in the
component: `ToolEvent.filePath` is set only for tools that write the file
they name, so Read/Grep/Bash rows stay plain text and panels stay dumb.

Read-only by design — see docs/IDEAS.md for why the editable code view is
rejected. No schema change, no migration, no new dependency. `diff:check`
covers the slicing and rides the existing `npm run check` gate.

Closes SuperLogicAI#10
@SuperLogicAI

Copy link
Copy Markdown
Owner

Review findings (CI green, but flagging before merge):

src/lib/diff.ts:30sectionPaths' header regexes hardcode literal a//b/ prefixes, so they can't parse git's quoted-path diff headers. core.quotePath=true (git's default) wraps any staged path with a non-ASCII character in double quotes: --- "a/caf\303\251.ts". The regex requires the bare letter right after the space, so it never matches — extractFileDiff finds no section, and the pop-out silently shows "No staged diff for this file" for a file that actually has one. Silent false negative, not a crash, but defeats the feature for any non-ASCII filename.

src/lib/diff.ts:34 — same class of break under diff.mnemonicPrefix=true (diff --git i/foo w/foo instead of a/foo b/foo) on a hunkless section. A server-side git diff --cached -- <path> (thin Rust wrapper in pty.rs, matching how every other git op in this codebase already works) would sidestep both this and the quoting issue structurally, rather than re-parsing unified-diff text for path matching.

src/lib/repo.ts:116WRITES gates the new diff feature on NotebookEdit, but filePath (unchanged this PR) only reads input.file_path. Real NotebookEdit payloads carry the target under notebook_path — same gap already present in src/lib/delta.ts:62. Net effect: a notebook edit's Accomplished row never becomes clickable; the feature silently never fires for this tool.

Minor, non-blocking:

  • DiffModal.tsx is a 4th independent copy of the overlay/Escape-close pattern already in LandingNoteModal/IsolateLoopModal/FanOutModal (plus one inconsistent instance in SidePanel's own Decision modal with no Escape handling) — worth factoring into a shared helper at some point rather than a 5th divergent copy next time.
  • dirOf/extractFileDiff re-derive separator-handling that basename() in repo.ts already solves (and DiffModal.tsx already imports basename from there).
  • WRITES isn't hoisted to module scope like sibling VERB should be — trivial per-call allocation.

@zeiddata-dev

zeiddata-dev commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, the Rust wrapper is the right call. Parsing diff headers to find a file was solving the problem one layer too late. A thin git_diff_file(cwd, path) in pty.rs running git diff --cached -- lets git do the scoping, so quotePath and mnemonicPrefix stop being my problem. sectionPaths, extractFileDiff and most of diff-check.ts go in the bin. dirOf stays so it runs in the file's own repo, tab cwd as fallback.

One question while there's a Rust command in play anyway: do you want git diff HEAD -- in this PR too, so the pop-out isn't an empty box 90% of the time? Or keep this strictly staged and do that separately. Going staged-only unless you say otherwise.

NotebookEdit: reading notebook_path next to file_path in listToolEvents, plus the same one-liner in delta.ts since it's the same hole.

WRITES gets hoisted next to VERB. Modal dedupe I'm leaving alone, this PR doesn't need to grow a refactor.

Still draft until somebody with a Mac actually clicks the thing.

Prompy to Claude to fix the issue dependent on the choice of the head

Replace the client-side diff parsing with a server-side per-file git command. Keep the feature staged-only. Do not touch DiffModal's overlay/Escape pattern beyond what is needed.

1. src-tauri/src/pty.rs: add a new Tauri command directly after git_diff_cached, same shape:

   #[tauri::command]
   pub fn git_diff_file(cwd: String, path: String) -> String

   It runs `git -C <cwd> diff --cached -- <path>` via std::process::Command with each piece as a separate .arg(), never a shell string. Add a doc comment noting the path reaches git as a real argument and that git does the pathspec matching, which is why quoted-path headers (core.quotePath) and diff.mnemonicPrefix no longer matter. On success return stdout lossy; on any failure or non-zero status return String::new().

2. src-tauri/src/lib.rs: register pty::git_diff_file in the invoke_handler list next to pty::git_diff_cached.

3. src/lib/pty.ts: add `export function gitDiffFile(cwd: string, path: string): Promise<string>` invoking "git_diff_file" with { cwd, path }, next to gitDiffCached.

4. src/lib/diff.ts: delete sectionPaths and extractFileDiff entirely. Keep dirOf. Rewrite loadFileDiff(filePath, fallbackCwd) to: compute dirOf(filePath); call gitDiffFile(dir, filePath) with .catch(() => ""); if that returns "" and dir !== fallbackCwd, retry gitDiffFile(fallbackCwd, filePath) with .catch(() => ""); return the result. Update the file header comment to describe the new approach. Drop the gitDiffCached import if unused.

5. scripts/diff-check.ts: remove every extractFileDiff case. Keep the dirOf cases (posix, Windows, bare path). Make sure the script still exits non-zero on failure and that `npm run diff:check` still runs from `npm run check`.

6. src/lib/repo.ts, listToolEvents: hoist `const WRITES = new Set([...])` to module scope next to where VERB should be (hoist VERB too, they are constants). Change the filePath extraction to read input.file_path, falling back to input.notebook_path when file_path is not a string, so NotebookEdit rows become clickable.

7. src/lib/delta.ts around line 62: apply the same file_path-or-notebook_path fallback for the NotebookEdit case. One line, no other changes to that file.

8. Update the empty-state copy in DiffModal.tsx only if it still mentions parsing. It should still say the app only reads staged changes.

9. docs/TESTING.md section 26: adjust any step that referenced extracting a section from the full staged diff; the manual steps for clicking a row and seeing the empty state stay as written, boxes unchecked.

Then run in order and fix anything that fails:
   npx tsc --noEmit
   npm run check
   cd src-tauri && cargo clippy --all-targets -- -D warnings && cargo test

Commit as one commit:
fix(diff): scope the file diff in git instead of parsing headers, read notebook_path for NotebookEdit


Do not push. Show me the full diff and the check output.

package.json: keep both check-script lists, diff:check appended.
docs/TESTING.md: interleave both phases' checklist entries.
SidePanel.tsx: keep both branches' new state hooks (diffPath +
expandedSessions/plannedCard/seededCwdRef), no logic overlap.

Verified: tsc --noEmit clean, npm run check all green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138tkBPv1EsZBHzC445NxeK
@SuperLogicAI
SuperLogicAI marked this pull request as ready for review September 9, 2026 13:10
@SuperLogicAI
SuperLogicAI merged commit db6f5cf into SuperLogicAI:main Sep 9, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Diff preview pop-out from Accomplished panel rows

2 participants